feat(DataMapper): wrap collection choice mapping with for-each (#2815)#3213
feat(DataMapper): wrap collection choice mapping with for-each (#2815)#3213mmelko wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughMappingActionService now detects when a collection choice wrapper field is dragged to a collection target field and generates a ChangesCollection-Choice-to-Collection-Target Mapping
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3213 +/- ##
========================================
Coverage 92.25% 92.26%
========================================
Files 636 636
Lines 24633 24649 +16
Branches 5843 5847 +4
========================================
+ Hits 22726 22742 +16
- Misses 1797 1905 +108
+ Partials 110 2 -108 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mmelko
force-pushed
the
fix/2815-collection-choice
branch
from
13880f8 to
c0b607d
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/ui/src/services/visualization/mapping-action.service.ts (1)
244-249: 💤 Low valueOptional: rename parameters for clarity.
Both
parent: MappingItemandtargetItem: MappingItemcarry the same type but very different roles —parentis only used as the constructor parent of the returnedChooseItem, whiletargetItemis only consulted to derive thefieldcontext. A future reader (or accidental swap at a call site) won't get help from the type system. Consider names likechooseParentandfieldContext(orfieldOwner) to make the asymmetry explicit.♻️ Suggested rename
- private static buildChooseFromChoiceMembers( - parent: MappingItem, - sourceField: IField, - targetItem: MappingItem, - ): ChooseItem { - const chooseItem = new ChooseItem(parent, targetItem instanceof FieldItem ? targetItem.field : undefined); + private static buildChooseFromChoiceMembers( + chooseParent: MappingItem, + sourceField: IField, + fieldContext: MappingItem, + ): ChooseItem { + const chooseItem = new ChooseItem( + chooseParent, + fieldContext instanceof FieldItem ? fieldContext.field : undefined, + );🤖 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 `@packages/ui/src/services/visualization/mapping-action.service.ts` around lines 244 - 249, The parameter names in buildChooseFromChoiceMembers are confusing because both parent and targetItem share type MappingItem but serve different roles; rename parent to chooseParent (used only as the ChooseItem constructor parent) and targetItem to fieldContext or fieldOwner (used only to derive the field via targetItem instanceof FieldItem ? targetItem.field : undefined) so the asymmetry is explicit and accidental swaps are harder; update the function signature and all internal references (including the new ChooseItem instantiation) and any call sites that pass these arguments.
🤖 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.
Inline comments:
In
`@packages/ui/src/services/visualization/mapping-action.service.choice.test.ts`:
- Around line 516-547: Replace the loose truthy/contains assertions with exact
XPath equality checks: assert ForEachItem.expression equals the collection
wrapper XPath produced by createMockCollectionChoiceField, and assert the
chooseItem.when[*].expression and the ValueSelector.expression for email/phone
equal the exact member XPaths used in the S1 tests (e.g. '/ns0:email' and
'/ns0:phone'); update the assertions in the test around
MappingActionService.engageMapping, ForEachItem, ChooseItem, and ValueSelector
to compare full expected XPath strings rather than using
toBeTruthy()/toContain().
---
Nitpick comments:
In `@packages/ui/src/services/visualization/mapping-action.service.ts`:
- Around line 244-249: The parameter names in buildChooseFromChoiceMembers are
confusing because both parent and targetItem share type MappingItem but serve
different roles; rename parent to chooseParent (used only as the ChooseItem
constructor parent) and targetItem to fieldContext or fieldOwner (used only to
derive the field via targetItem instanceof FieldItem ? targetItem.field :
undefined) so the asymmetry is explicit and accidental swaps are harder; update
the function signature and all internal references (including the new ChooseItem
instantiation) and any call sites that pass these arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0880649d-1ad3-4c34-becf-9cec4e67633f
📒 Files selected for processing (2)
packages/ui/src/services/visualization/mapping-action.service.choice.test.tspackages/ui/src/services/visualization/mapping-action.service.ts
| it('ForEachItem expression should be the XPath of the collection choice wrapper', () => { | ||
| const collectionChoiceField = createMockCollectionChoiceField([{ name: 'email' }, { name: 'phone' }]); | ||
| const choiceNode = new ChoiceFieldNodeData(sourceDocNode, collectionChoiceField); | ||
| const collectionTargetField = createMockCollectionField(); | ||
| const targetFieldNode = new TargetFieldNodeData(localTargetDocNode, collectionTargetField); | ||
|
|
||
| MappingActionService.engageMapping(tree, choiceNode, targetFieldNode); | ||
|
|
||
| const forEachItem = tree.children[0].children[0] as ForEachItem; | ||
| // ForEachItem expression should be set (XPath to the collection choice wrapper) | ||
| expect(forEachItem.expression).toBeTruthy(); | ||
| expect(forEachItem.expression.length).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| it('WhenItem expressions inside ForEachItem should reference the choice members', () => { | ||
| const collectionChoiceField = createMockCollectionChoiceField([{ name: 'email' }, { name: 'phone' }]); | ||
| const choiceNode = new ChoiceFieldNodeData(sourceDocNode, collectionChoiceField); | ||
| const collectionTargetField = createMockCollectionField(); | ||
| const targetFieldNode = new TargetFieldNodeData(localTargetDocNode, collectionTargetField); | ||
|
|
||
| MappingActionService.engageMapping(tree, choiceNode, targetFieldNode); | ||
|
|
||
| const forEachItem = tree.children[0].children[0] as ForEachItem; | ||
| const chooseItem = forEachItem.children[0] as ChooseItem; | ||
| // WhenItem test expressions reference the choice members | ||
| expect(chooseItem.when[0].expression).toContain('email'); | ||
| expect(chooseItem.when[1].expression).toContain('phone'); | ||
| // WhenItem value selectors should have paths to the members | ||
| const emailSelector = chooseItem.when[0].children.find((c) => c instanceof ValueSelector) as ValueSelector; | ||
| const phoneSelector = chooseItem.when[1].children.find((c) => c instanceof ValueSelector) as ValueSelector; | ||
| expect(emailSelector.expression).toContain('email'); | ||
| expect(phoneSelector.expression).toContain('phone'); |
There was a problem hiding this comment.
Tighten expression assertions so they verify the XPath, not just its existence.
The new S2 tests use toBeTruthy() for the for-each expression and toContain('email') / toContain('phone') for the when/value-selector expressions, whereas the S1 counterparts (lines 128-129, 142-143) assert exact XPath ('/ns0:email'). The loose checks here will pass regardless of whether the engine emits:
/ns0:email(absolute — incorrect when evaluated inside anxsl:for-eachiteration)emailor./email(relative — correct)- the wrapper's path vs a member's path
Given the helper reuses the same MappingService.mapToCondition / mapToField calls as S1, it most likely emits absolute paths, which would generate semantically wrong XSLT inside the for-each. Asserting exact strings would either confirm the implementation is correct or surface the bug.
💚 Suggested assertion tightening
- const forEachItem = tree.children[0].children[0] as ForEachItem;
- // ForEachItem expression should be set (XPath to the collection choice wrapper)
- expect(forEachItem.expression).toBeTruthy();
- expect(forEachItem.expression.length).toBeGreaterThan(0);
+ const forEachItem = tree.children[0].children[0] as ForEachItem;
+ // ForEachItem expression should be the XPath to the collection choice wrapper
+ expect(forEachItem.expression).toEqual('/ns0:__choice__'); // adjust to the actual expected XPath- // WhenItem test expressions reference the choice members
- expect(chooseItem.when[0].expression).toContain('email');
- expect(chooseItem.when[1].expression).toContain('phone');
- // WhenItem value selectors should have paths to the members
- const emailSelector = chooseItem.when[0].children.find((c) => c instanceof ValueSelector) as ValueSelector;
- const phoneSelector = chooseItem.when[1].children.find((c) => c instanceof ValueSelector) as ValueSelector;
- expect(emailSelector.expression).toContain('email');
- expect(phoneSelector.expression).toContain('phone');
+ // Inside an xsl:for-each the test/select must be relative to the current() item
+ expect(chooseItem.when[0].expression).toEqual('email'); // or whatever the relative form should be
+ expect(chooseItem.when[1].expression).toEqual('phone');
+ const emailSelector = chooseItem.when[0].children.find((c) => c instanceof ValueSelector) as ValueSelector;
+ const phoneSelector = chooseItem.when[1].children.find((c) => c instanceof ValueSelector) as ValueSelector;
+ expect(emailSelector.expression).toEqual('email');
+ expect(phoneSelector.expression).toEqual('phone');🤖 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 `@packages/ui/src/services/visualization/mapping-action.service.choice.test.ts`
around lines 516 - 547, Replace the loose truthy/contains assertions with exact
XPath equality checks: assert ForEachItem.expression equals the collection
wrapper XPath produced by createMockCollectionChoiceField, and assert the
chooseItem.when[*].expression and the ValueSelector.expression for email/phone
equal the exact member XPaths used in the S1 tests (e.g. '/ns0:email' and
'/ns0:phone'); update the assertions in the test around
MappingActionService.engageMapping, ForEachItem, ChooseItem, and ValueSelector
to compare full expected XPath strings rather than using
toBeTruthy()/toContain().
…IO#2815) - Detect collection choice wrapper + collection target and wrap choose/when/otherwise inside a for-each - Add tests for collection choice scenarios
mmelko
force-pushed
the
fix/2815-collection-choice
branch
from
c0b607d to
dca9f49
Compare
|
There was a problem hiding this comment.
We might want to cause a validation error when the collection choice field contains any container field(s). We could even make choice field not draggable in that case (MappingValidationService.isDraggable() -> false)
And it makes me also think if we're validating source-choice->target-container on both cases of when source-choice is a collection or non-collection.
2 participants
fixes: #2815
Screen.Recording.2026-05-14.at.10.31.36.mov
Summary by CodeRabbit
Release Notes
Tests
Refactor