-
Notifications
You must be signed in to change notification settings - Fork 12
feat(hub-common): new sites are fully upgraded to the new catalog, old sites can opt into the upgrade #2013
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
There hasn't been any activity on this pull request in the past 3 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. If you want this PR to never become stale, please apply the "Draft" label. |
|
There hasn't been any activity on this pull request in the past 3 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. If you want this PR to never become stale, please apply the "Draft" label. |
2ef92b8 to
f5c4a61
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2013 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 1071 1116 +45
Lines 19532 19812 +280
Branches 3617 3652 +35
==========================================
+ Hits 19532 19812 +280 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
There hasn't been any activity on this pull request in the past 3 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. If you want this PR to never become stale, please apply the "Draft" label. |
…n, remove hub:feature:catalogs: affects: @esri/hub-common
affects: @esri/hub-common
affects: @esri/hub-common
…d data.catalog in certain circu affects: @esri/hub-common
6950d0b to
aa34138
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements final cleanup for migrating sites to the new catalog spec (v2), allowing full migration to the new catalog format while maintaining backwards compatibility for sites that haven't opted in yet.
- Removes deprecated permission
hub:feature:catalogs:edit:advancedand updates dependent permissions - Modifies site creation to use catalogV2 by default and removes legacy catalog initialization
- Updates site update logic to conditionally handle catalog formats based on
isCatalogV1Enabledflag
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/common/test/sites/HubSites.test.ts | Updates test cases to verify catalog v1/v2 behavior and legacy catalog removal |
| packages/common/src/sites/defaults.ts | Changes default site model to initialize catalogV2 instead of legacy catalog |
| packages/common/src/sites/_internal/getPropertyMap.ts | Removes legacy catalog property mapping |
| packages/common/src/sites/_internal/SiteBusinessRules.ts | Updates catalog upgrade permission to use assertions instead of flag-based availability |
| packages/common/src/sites/HubSites.ts | Major refactor to handle catalog v1/v2 logic in create/update operations |
| packages/common/src/projects/_internal/ProjectBusinessRules.ts | Removes deprecated permission dependency |
| packages/common/src/permissions/_internal/constants.ts | Removes deprecated hub:feature:catalogs:edit:advanced permission |
| packages/common/src/permissions/HubPermissionPolicies.ts | Removes deprecated permission policy and updates appearance permission |
| packages/common/src/initiatives/_internal/InitiativeBusinessRules.ts | Removes deprecated permission dependency |
| packages/common/src/events/_internal/EventBusinessRules.ts | Removes deprecated permission dependency |
| packages/common/src/discussions/_internal/DiscussionBusinessRules.ts | Removes deprecated permission dependency |
knbhagat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job Caleb, LGTM!
|
There hasn't been any activity on this pull request in the past 3 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. If you want this PR to never become stale, please apply the "Draft" label. |
🦋 Changeset detectedLatest commit: ea601c2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
There hasn't been any activity on this pull request in the past 3 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. If you want this PR to never become stale, please apply the "Draft" label. |
|
There hasn't been any activity on this pull request in the past 3 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. If you want this PR to never become stale, please apply the "Draft" label. |
0e9fbe4 to
1afeabb
Compare
|
@knbhagat would you mind re-reviewing the PR? I had to fix a bug that Diana caught in testing. You can check out the new commits here: |
knbhagat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Good!
abp6318
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| modelToUpdate = convertCatalogToLegacyFormat(modelToUpdate, currentModel); | ||
| } else { | ||
| // ensure the old catalog is removed if the site is not using v1 | ||
| delete modelToUpdate.data.catalog; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a bunch of new TODOs in this file. Do we need to document them anywhere before we forget?
|
There hasn't been any activity on this pull request in the past 3 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. If you want this PR to never become stale, please apply the "Draft" label. |
Part of https://devtopia.esri.com/dc/hub/issues/13504
Does the final cleanup needed to allow sites to be fully migrated to the new catalog spec, including:
hub:feature:catalogs:edit:advancedpermission since it wasn't relevant any morehub:site:workspace:catalog:upgradepermissiondata.useCatalogV2field (needed because of template idiosyncrasies)Updated meaningful TSDoc to methods including Parameters and Returns, see Documentation Guide
used semantic commit messages
PR title follows semantic commit format (CRITICAL if the title is not in a semantic format, the release automation will not run!)
Updated
peerDependenciesas needed. CRITICAL our automated release system can not be counted on to updatepeerDependenciesso we must do it manually in our PRs when needed. See the updating peerDependencies section of the release instructions for more details.These changes have been verified by QA using a
?uiVersionthat includes a PR-Preview of this branch and thev_reqlabel has been applied to the issue.OD-UI E2E tests pass against these changes using a
?uiVersionthat includes a PR-Preview of this branch*CRITICAL
If you are making a breaking change, make sure to add the
BREAKING CHANGEcomment in the merge commit message. If this is not done,semantic-releasewill not do the right thing.If you find yourself in this position...
BREAKING CHANGEmessage in the merge commit.npm deprecate @esri/{package-name}@v{version-you-don't-want-out}