-
Notifications
You must be signed in to change notification settings - Fork 42
feat(preferences): Add “Collection Preferences” page with role-gated access #7464
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: issue-7445
Are you sure you want to change the base?
Conversation
specifyweb/frontend/js_src/lib/components/Preferences/CollectionDefinitions.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/Preferences/Editor.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/Preferences/index.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/Preferences/index.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/Preferences/index.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/Preferences/index.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/Preferences/index.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/Preferences/Editor.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/Preferences/index.tsx
Outdated
Show resolved
Hide resolved
emenslin
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.
After opening up the branch I have some concerns about testing with this being the only check:
- Verify the new Collection Preferences menu is visible only to authorized roles, settings can be saved and persist per collection.
The original issue seems to cover a lot of things, which I don't think is captured within the testing instructions. One of the first things I noticed when opening up the PR was that in the original issue it says that the collection preferences page should be accessible via the user tools menu, in this PR it is only accessible by going into app resources first, then opening the collection preferences, if this is expected that's fine, I just think it should be explained in this PR. I also noticed that when opening collection preferences each line includes a link to documentation, however, the link does not take you to relevent documentation, additionally there is no mention of these links in the testing instructions. I think the testing instructions should be expanded on before any further testing reviews to make sure that everything is covered and that any issues can be caught within this PR.
Triggered by e9b8f26 on branch refs/heads/issue-7440
Triggered by 579a30b on branch refs/heads/issue-7440
|
From my last review on the rebased PR for context:
|
|
@Gitesh307 Can you update the instructions so they cover everything properly? From the other PR (#7516 (review)):
|
| 'sp7.allow_adding_child_to_synonymized_parent.ChronosStrat', | ||
| 'sp7.allow_adding_child_to_synonymized_parent.ChronoStrat', | ||
| ), | ||
| 'ChronosStrat': ( |
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.
Why 2? See line 34
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.
Older deployments saved the Chronostrat synonym preference under two different keys, ChronosStrat (the legacy table/view name) and ChronoStrat (the shortened label used in global properties). If we drop one of them, any collection that previously saved the preference under that spelling would lose the ability to add children to synonymized Chronostrat nodes. Keeping both entries ensures _synonym_pref_keys continues to read whatever was persisted historically.
| const props = { | ||
| className: ` | ||
| flex items-start gap-2 md:flex-row flex-col | ||
| ${canEdit ? '' : '!cursor-not-allowed'} |
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.
is this secure enough?
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.
That just affects cursor/UX. The real permission check comes from canEdit flowing into the ReadOnlyContext, which disables the inputs and prevents writes. That matches how the existing user-preferences UI enforces permissions, and the server still validates everything, so the security level is the same as production.
grantfitzsimmons
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.
- Verify the new Collection Preferences menu is visible only to authorized roles, settings can be saved and persist per collection.
After looking at this some more, can you provide some more insight here? It seems like we need to expand or clarify the permission for CollectionPreferences as this is not explicitly added as an option.
What is the criteria for "authorized roles" in this case?
- Modify values in each section, ensure they save and persist after reload, and verify that the Reset button restores defaults correctly.
General
- Scope "Entire Table" Picklists
With this enabled (same behavior with or without this checked):
Seems not to take effect. Have you been able to demonstrate this? Can you add screenshots?
- Make Attachments Public By Default
Tree Management
Confirmed by creating synonyms for nodes with children and added children under existing and new synonyms.
- Taxon
- Geography
- Storage
- Chronostratigraphy
- Lithostratigraphy
- Tectonic Unit
Statistics
- Show Preparation Totals
- Auto-Refresh Rate (Hours)
The following two cannot really be tested at the moment... I think we should consider hiding these from the UI (commented out) but keeping the localized strings? We need to demonstrate or understand how these are being used... 🤷
- GBIF Publishing Organization Key
- GBIF Data Set Key
Catalog Number Inheritance
COGs
- Enable Catalog number Inheritance From Primary Collection Object
Being resolved in #7539, can be addressed outside of this PR.
Components
- Enable Catalog number Inheritance From Parent Collection Object
Works 😄
|
@grantfitzsimmons For the Picklist I am able to achieve the exact behaviour With scope entire table pick list unchecked : For accessibility Institution Admin / Collection Admin: Those roles get the wildcard %/% policy plus full table permissions that means they have the access to the collection preferences by default
|
Triggered by 0964161 on branch refs/heads/issue-7440
|
From my earlier comment:
Will review soon! |
|
@Gitesh307 Can you fix the conflicts? 224 commits behind now |
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.
- Verify the new Collection Preferences menu is visible only to authorized roles, settings can be saved and persist per collection.
- Modify values in each section, ensure they save and persist after reload, and verify that the Reset button restores defaults correctly.
Test each option and verify application behavior responds to the setting
General
- Scope "Entire Table" Picklists
- Make Attachments Public By Default
Tree Management
If enabled, this allows users to add children to synonymized parents and to synonymize a node with children.
- Taxon
- Geography
- Storage
- Chronostratigraphy
- Lithostratigraphy
- Tectonic Unit
Statistics
- Show Preparation Totals
- Auto-Refresh Rate (Hours)
The following two cannot really be tested at the moment... - GBIF Publishing Organization Key
- GBIF Data Set Key
Catalog Number Inheritance
- Enable Catalog number Inheritance From Primary Collection Object
- Enable Catalog number Inheritance From Parent Collection Object
Generally the functionality seems to be working but I did run into some issues.
- First thing I noticed was that Catalog number inheritance appears twice on the side bar
- Also under the catalog number inheritance section shouldn't there be a 'Component' heading above the Parent Collection Object checkbox?
- Trees are not sorted properly
Issue branch:










Fixes #7440
This PR introduces a new feature Collection Preferences page that consolidates collection-level settings such as picklists, attachments, tree management, statistics, Specify Network integration, and catalog number inheritance. Updated localization handling to use explicit function calls, resolving scanner issues. No database changes; existing preferences migrate automatically at the collection level.
Checklist
self-explanatory (or properly documented)
Testing instructions
Test each option and verify application behavior responds to the setting
General
Tree Management
If enabled, this allows users to add children to synonymized parents and to synonymize a node with children.
Statistics
The following two cannot really be tested at the moment...
Catalog Number Inheritance