Preserve Agent Type when merging agents without auto-populate#7828
Preserve Agent Type when merging agents without auto-populate#7828acwhite211 wants to merge 5 commits into
Conversation
Triggered by 6c3a929 on branch refs/heads/issue-5072
emenslin
left a comment
emenslin
left a comment
There was a problem hiding this comment.
- Verify that Agent Type in the merged record is still Person, not Organization.
- Verify that Agent Type is still Person.
Agent type is still set to Organization, also even with the auto-populate preference disabled, the fields are still auto-populating.
03-23_09.29.mp4
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
bhumikaguptaa
left a comment
bhumikaguptaa
left a comment
There was a problem hiding this comment.
- Verify that Agent Type in the merged record is still Person, not Organization.
- Verify that Agent Type is still Person.
--
Agent type is still set to Organization even with auto-populate being disabled.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughExtracts initial-merged-resource construction into an exported async helper buildInitialMergedResource; the merging dialog imports and uses this helper instead of performing conditional merge/post-processing or calling addMissingFields inline. ChangesInitial merged resource builder extraction
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
specifyweb/frontend/js_src/lib/components/Merging/autoMerge.ts (1)
57-69: 💤 Low valueNice, focused extraction — logic preserves unanimous values when auto-populate is off. 👍
One small thing: the
as SerializedResource<AnySchema>cast on line 68 covers both branches, but only thepostMergeResourcebranch actually needs it (its declared return type isIR<Awaited<ReturnType<typeof mergeField>>>). You can keep the cast scoped to that branch and let theautoMergebranch stay strongly typed.♻️ Optional: narrow the cast to the branch that needs it
export async function buildInitialMergedResource( table: SpecifyTable, resources: RA<SerializedResource<AnySchema>>, shouldAutoPopulate: boolean, targetId?: number ): Promise<SerializedResource<AnySchema>> { - return (shouldAutoPopulate - ? await postMergeResource( - resources, - autoMerge(table, resources, false, targetId) - ) - : autoMerge(table, resources, true, targetId)) as SerializedResource<AnySchema>; + if (!shouldAutoPopulate) return autoMerge(table, resources, true, targetId); + const merged = await postMergeResource( + resources, + autoMerge(table, resources, false, targetId) + ); + return merged as SerializedResource<AnySchema>; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/frontend/js_src/lib/components/Merging/autoMerge.ts` around lines 57 - 69, The returned value from buildInitialMergedResource is being blanket-cast to SerializedResource<AnySchema>, but only the postMergeResource branch actually needs that cast; narrow the cast to the postMergeResource branch by removing the trailing as SerializedResource<AnySchema> and instead apply the cast directly to the await postMergeResource(...) call (keeping autoMerge(...) uncast so its inferred type remains strong). Update inside buildInitialMergedResource, referencing the postMergeResource(...) and autoMerge(...) branches so only the postMergeResource result is cast.specifyweb/frontend/js_src/lib/components/Merging/index.tsx (1)
253-274: ⚡ Quick winConsider reusing
buildInitialMergedResourcehere too for symmetry. 🙂The "Auto Populate" button still inlines
postMergeResource(records, autoMerge(table, records, false, target.id)), which is exactly theshouldAutoPopulate === truebranch of the new helper. Routing this throughbuildInitialMergedResourcekeeps both call sites in lockstep, so any future change (e.g., another preservation tweak like the AgentType fix) only needs to land in one place.Note the subtle pre-existing difference: this button uses the current
recordsrather thaninitialRecords.current— please confirm that's still the intent before swapping in the helper.♻️ Proposed refactor
<Button.Success onClick={(): void => loading( - postMergeResource( - records, - autoMerge(table, records, false, target.id) - ) + buildInitialMergedResource(table, records, true, target.id) .then(async (merged) => { // REFACTOR: move all this to postMergeResource? const mergedResource = deserializeResource( merged as SerializedResource<AnySchema> ); if (merged !== undefined) await runAllFieldChecks(mergedResource); return mergedResource; }) .then(setMerged) ) } >If this lands,
autoMergeandpostMergeResourcecan likely be dropped from the./autoMergeimport list in this file as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/frontend/js_src/lib/components/Merging/index.tsx` around lines 253 - 274, Replace the inline postMergeResource(autoMerge(...)) call inside the Button.Success onClick with a call to the helper buildInitialMergedResource(table, records, true, target.id) (or whatever signature it uses for the shouldAutoPopulate === true path), then pipe its returned resource through the same deserializeResource, runAllFieldChecks (if merged !== undefined) and setMerged logic currently present; make sure to reference and preserve deserializeResource, runAllFieldChecks, and setMerged, and confirm you intentionally want to use the current records variable (not initialRecords.current) before swapping in the helper so behavior remains the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@specifyweb/frontend/js_src/lib/components/Merging/autoMerge.ts`:
- Around line 57-69: The returned value from buildInitialMergedResource is being
blanket-cast to SerializedResource<AnySchema>, but only the postMergeResource
branch actually needs that cast; narrow the cast to the postMergeResource branch
by removing the trailing as SerializedResource<AnySchema> and instead apply the
cast directly to the await postMergeResource(...) call (keeping autoMerge(...)
uncast so its inferred type remains strong). Update inside
buildInitialMergedResource, referencing the postMergeResource(...) and
autoMerge(...) branches so only the postMergeResource result is cast.
In `@specifyweb/frontend/js_src/lib/components/Merging/index.tsx`:
- Around line 253-274: Replace the inline postMergeResource(autoMerge(...)) call
inside the Button.Success onClick with a call to the helper
buildInitialMergedResource(table, records, true, target.id) (or whatever
signature it uses for the shouldAutoPopulate === true path), then pipe its
returned resource through the same deserializeResource, runAllFieldChecks (if
merged !== undefined) and setMerged logic currently present; make sure to
reference and preserve deserializeResource, runAllFieldChecks, and setMerged,
and confirm you intentionally want to use the current records variable (not
initialRecords.current) before swapping in the helper so behavior remains the
same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b91c343b-a434-40fc-b6b1-5bd0068ccbb9
📒 Files selected for processing (2)
specifyweb/frontend/js_src/lib/components/Merging/autoMerge.tsspecifyweb/frontend/js_src/lib/components/Merging/index.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
specifyweb/frontend/js_src/lib/components/Merging/autoMerge.ts (1)
76-83: 💤 Low valueConsider a registry pattern for table-specific preserved fields.
The current implementation hardcodes
'Agent'/agentTypeinline. It works and is appropriately scoped to the reported bug, but as soon as a second table needs similar treatment (e.g., other type-discriminated tables), this conditional will grow into a chain. A small lookup map keyed by table name — mirroring thepostProcessorspattern already used below in this file — would scale better and keepbuildInitialMergedResourcetidy.♻️ Sketch
-const getNonAutoPopulatedFields = ( - table: SpecifyTable, - resources: RA<SerializedResource<AnySchema>>, - targetId?: number -): Partial<SerializedResource<AnySchema>> => - table.name === 'Agent' - ? { agentType: getTargetResource(resources, targetId)?.agentType } - : {}; +const nonAutoPopulatedFieldExtractors: Partial< + RR< + keyof Tables, + ( + resources: RA<SerializedResource<AnySchema>>, + targetId?: number + ) => Partial<SerializedResource<AnySchema>> + > +> = { + Agent: (resources, targetId) => ({ + agentType: getTargetResource(resources, targetId)?.agentType, + }), +}; + +const getNonAutoPopulatedFields = ( + table: SpecifyTable, + resources: RA<SerializedResource<AnySchema>>, + targetId?: number +): Partial<SerializedResource<AnySchema>> => + nonAutoPopulatedFieldExtractors[table.name]?.(resources, targetId) ?? {};Also, the helper name reads as "fields that were not auto-populated"; something like
getPreservedFieldsWithoutAutoPopulatemay communicate intent more directly. Both points are purely stylistic — feel free to defer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/frontend/js_src/lib/components/Merging/autoMerge.ts` around lines 76 - 83, The function getNonAutoPopulatedFields currently hardcodes the Agent/table->agentType mapping; replace this with a small registry lookup (map keyed by table.name) similar to the existing postProcessors pattern and have getNonAutoPopulatedFields consult that registry to return preserved fields (e.g., for 'Agent' return { agentType }) so additions require only registry updates; also consider renaming the helper to getPreservedFieldsWithoutAutoPopulate and update callers such as buildInitialMergedResource to use the registry-backed helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@specifyweb/frontend/js_src/lib/components/Merging/autoMerge.ts`:
- Around line 76-83: The function getNonAutoPopulatedFields currently hardcodes
the Agent/table->agentType mapping; replace this with a small registry lookup
(map keyed by table.name) similar to the existing postProcessors pattern and
have getNonAutoPopulatedFields consult that registry to return preserved fields
(e.g., for 'Agent' return { agentType }) so additions require only registry
updates; also consider renaming the helper to
getPreservedFieldsWithoutAutoPopulate and update callers such as
buildInitialMergedResource to use the registry-backed helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 08dbfad0-bca4-4c2a-a9fd-de540b28ba2a
📒 Files selected for processing (1)
specifyweb/frontend/js_src/lib/components/Merging/autoMerge.ts
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
5 participants
Fixes #5072
Found a simple fix to the issue. When record merging is opened with auto-populate disabled, the merged record no longer starts from schema defaults. It now preserves unanimous values from the selected records, so merging two
Personagents no longer silently changesAgentTypetoOrganizationwhile still leaving real conflicts available for review.Checklist
self-explanatory (or properly documented)
Testing instructions
Confirm it working on any test database, as well as confirming it's working on the
kuaccessions_2024_07_05database.Summary by CodeRabbit