-
Notifications
You must be signed in to change notification settings - Fork 40
feat(trees): add tree options for rebuilding names #7307
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: main
Are you sure you want to change the base?
Conversation
Adds descriptions and new options to correct node numbers and rebuild full names, either for accepted nodes or for all nodes, including synonyms.
Triggered by 6a27734 on branch refs/heads/issue-7138
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 adds new tree repair functionality by introducing options to rebuild full name values for tree nodes. It adds two new rebuild options for full names (with and without synonyms) to complement the existing tree repair functionality, along with improved UI elements and permissions.
- Adds API endpoint and backend logic for rebuilding full names of tree nodes
- Introduces new permission system for
rebuild_fullnameoperations - Replaces tree repair icon with a more appropriate wrench icon and adds tree options dropdown
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| specifyweb/specify/urls.py | Adds new URL route for rebuild-full-name endpoint |
| specifyweb/specify/tree_views.py | Implements rebuild_fullname view and adds permissions for all tree types |
| specifyweb/specify/tree_extras.py | Extends set_fullnames function to support synonym filtering and returns row count |
| specifyweb/frontend/js_src/lib/localization/tree.ts | Adds localization strings for rebuild options and descriptions |
| specifyweb/frontend/js_src/lib/localization/header.ts | Adds "Tree Options" localization |
| specifyweb/frontend/js_src/lib/localization/common.ts | Adds "Working…" localization string |
| specifyweb/frontend/js_src/lib/components/Toolbar/TreeRepair.tsx | Implements tree options dropdown with rebuild functionality |
| specifyweb/frontend/js_src/lib/components/Permissions/definitions.ts | Adds rebuild_fullname permission to all tree types |
| specifyweb/frontend/js_src/lib/components/Header/userToolDefinitions.ts | Changes repair tree icon from checkCircle to wrench |
| specifyweb/frontend/js_src/lib/components/Atoms/Icons.tsx | Adds wrench icon SVG |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
specifyweb/frontend/js_src/lib/components/Toolbar/TreeRepair.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/Toolbar/TreeRepair.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/Toolbar/TreeRepair.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
Triggered by d839f48 on branch refs/heads/issue-7138
Triggered by 7646cdb on branch refs/heads/issue-7138
|
@CarolineDenis I've tested this quite a bit for functionality and scope, works very well. Feel free to review and add to any milestone if you'd like to integrate it 👍 😄 |
Triggered by cc6fae0 on branch refs/heads/issue-7138
| const id = treeDefinition.get('id'); | ||
| type RebuildChanged = { readonly accepted: number; readonly synonyms: number; readonly total: number } | ||
| type RebuildResponse = { readonly success?: boolean; readonly rebuild_synonyms?: boolean; readonly changed?: Partial<RebuildChanged> | null } | ||
| const parseRebuildResponse = (raw: unknown): RebuildChanged => { |
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.
we could make this a util function and write a unit test for it (outside of this action menu)
| .then(() => setRepairStatus('success')) | ||
| .finally(() => setIsRunning(false)); | ||
| }; | ||
| type ActionKey = 'rebuildAccepted' | 'rebuildSynonyms' | 'repair'; |
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.
to put outside
| readonly description: () => LocalizedString; | ||
| readonly run: () => void; | ||
| }; | ||
| const actions: readonly ActionDef[] = [ |
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.
use RA<> instead of []
| ]; | ||
| const visibleActions = actions.filter((a) => a.can); | ||
|
|
||
| let status: React.ReactNode = null; |
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.
can be replaced by:
const status: React.ReactNode = isRunning ? (
<span className="text-xs">{commonText.working()}</span>
) : result && canRebuild ? (
result.total > 0 ? (
<div className="text-xs">
{...}
| return ( | ||
| <div className="flex flex-col gap-1 p-2 bg-[color:var(--background)] rounded"> | ||
| <div className="flex flex-col gap-2"> | ||
| {visibleActions.map((a) => ( |
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.
can you use action instead of a?
| }} | ||
| onMouseEnter={(): void => setHoveredAction(a.key)} | ||
| onMouseLeave={(): void => | ||
| setHoveredAction((h) => (h === a.key ? null : h)) |
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.
can you replace h with something more descriptive?
| ); | ||
| } | ||
|
|
||
| function TreeActionsDropdown({ |
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.
could we extract all the treeActions to a new file?
Triggered by ecf79e6 on branch refs/heads/issue-7138
Fixes #7138
This PR introduces changes that add new tree repair options, including support for rebuilding full names for all nodes in a tree, both excluding and including synonyms. Descriptions of each function have been added under a new "Tree Options" menu. It also adds a nice wrench icon more fitting for the repair tree tool.
newTreeOptions.mp4
treeDemoGitHub.mp4
This has its own permission, so if the user doesn't have the
rebuild_full_namesfor a particular tree, every option will be hidden except for the repair tree option (which has its own permission).Checklist
self-explanatory (or properly documented)
Testing instructions
In progress... 🚧
Rebuild Full Names
There is a new option to recompute and update the
fullNamefield for all nodes in a chosen tree definition (Taxon, Geography, Storage, TectonicUnit, GeologicTimePeriod, LithoStrat). It only touches rows whose stored value is missing or differs from the recalculated value.API endpoint:
/api/specify_tree/{tree}/{treedef_id}/rebuild-full-nameOptional parameter:
?rebuild_synonyms=true(also updates synonym nodes; default is accepted/preferred nodes only as when updating a tree definition)Examples:
Accepted only:
/api/specify_tree/taxon/2/rebuild-full-nameAccepted + synonyms:
/api/specify_tree/geography/4/rebuild-full-name?rebuild_synonyms=trueResponse (example):
{ "success": true, "rebuild_synonyms": true, "changed": { "accepted": 95, "synonyms": 25, "total": 120 } }Permissions:
User must have rebuild_fullname on /tree/edit/{tree}. If missing, expect 403.
When to test:
fullNameedits via SQL.Note
You will likely need to edit node
fullNamevalues using SQL to test this effectively. In testing, I used an approach like this one on different trees:This let me update a mix of both synonyms and accepted names to verify counts are appropriate and that only the specified tree is updated.
UI testing:
What to verify:
?rebuild_synonyms=true(when using the appropriate button).Difference from existing “Repair Tree” function:
Repair fixes structural numbering; rebuild-full-name only recalculates display names.