UFAL/stabilize form array reorder/delete behavior#1268
UFAL/stabilize form array reorder/delete behavior#1268milanmajchrak merged 1 commit intocustomer/lindatfrom
Conversation
* stabilize form array reorder/delete behavior synchronize FormArray controls with model on reorder, dedupe patch operations by latest op+path, and fix place handling for index 0 to prevent deleted or duplicate values reappearing after save. * Document dedupe behavior for json-patch ops Document dedupe behavior for json-patch ops * Simplify array index presence check Simplify array index presence check (cherry picked from commit b261513) Co-authored-by: Amad Ul Hassan <hassan@ufal.mff.cuni.cz>
There was a problem hiding this comment.
Pull request overview
Stabilizes submission form array behavior by ensuring form model + FormArray controls stay aligned during reorder/delete, and by reducing redundant JSON Patch operations so stale/deleted values don’t reappear after save.
Changes:
- Fix
placehandling for array index0when normalizing object-valued metadata values. - Update array reorder logic to move both
DynamicRowArrayModelgroups and AngularFormArraycontrols together. - Dedupe JSON Patch operations by retaining only the latest operation per
op+path, and update/extend specs accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/app/shared/form/builder/form-builder.service.ts |
Adjusts place derivation logic when normalizing object-valued control values. |
src/app/shared/form/builder/form-builder.service.spec.ts |
Adds regression coverage for preserving place: 0 for object-valued array items. |
src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts |
Reorder now delegates to FormBuilderService.moveFormArrayGroup to keep model and FormArray in sync; simplifies control lookup logic. |
src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.spec.ts |
Updates tests to account for synchronized model/control movement and new dependency injection. |
src/app/core/json-patch/json-patch-operations.reducer.ts |
Reworks JSON Patch dedupe to keep only the latest op+path entry; expands documentation. |
src/app/core/json-patch/json-patch-operations.reducer.spec.ts |
Updates expectations to reflect “latest op+path wins” dedupe behavior. |
| const hasArrayIndex = controlModelIndex !== null; | ||
| const place = hasArrayIndex ? controlModelIndex : (controlValue as any).place; |
There was a problem hiding this comment.
hasArrayIndex is derived from controlModelIndex !== null, but iterateControlModels always passes a number (defaulting to 0), so this condition is always true and the fallback to (controlValue as any).place becomes effectively unreachable. If the intent is to only override place when an array index was explicitly provided (while preserving controlValue.place in non-array contexts), consider making controlModelIndex nullable (default null) and only passing a numeric index from array contexts, or otherwise explicitly distinguishing “no index” from index 0.
| * Note: this function is only called for submission-form patch operations, which always use numeric-indexed or | ||
| * bare metadata paths (e.g. /sections/step/dc.title or /sections/step/dc.title/0). The JSON Patch append | ||
| * notation (path ending in "/-") is intentionally never produced by this pipeline, so collapsing duplicates | ||
| * by op+path is safe here. |
There was a problem hiding this comment.
The docstring claims this dedupe is “only called for submission-form patch operations”, but dedupeOperationEntries is invoked unconditionally from addOperationToList, which is used by newOperation for all patch actions keyed by action.payload.resourceType/resourceId. Consider rewording this comment to reflect the actual call scope (or enforcing the submission-form-only assumption in code) so future usages don’t rely on a guarantee that isn’t implemented here.
| * Note: this function is only called for submission-form patch operations, which always use numeric-indexed or | |
| * bare metadata paths (e.g. /sections/step/dc.title or /sections/step/dc.title/0). The JSON Patch append | |
| * notation (path ending in "/-") is intentionally never produced by this pipeline, so collapsing duplicates | |
| * by op+path is safe here. | |
| * This function assumes that callers only provide operations whose paths are numeric-indexed or bare metadata | |
| * paths (e.g. /sections/step/dc.title or /sections/step/dc.title/0), and that JSON Patch append notation | |
| * (paths ending in "/-") is not used. Under these conditions, collapsing duplicates by op+path is safe. |
Problem description
(cherry picked from commit b261513)
Analysis
(Write here, if there is needed describe some specific problem. Erase it, when it is not needed.)
Problems
(Write here, if some unexpected problems occur during solving issues. Erase it, when it is not needed.)
Sync verification
If en.json5 or cs.json5 translation files were updated:
yarn run sync-i18n -t src/assets/i18n/cs.json5 -ito synchronize messages, and changes are included in this PR.Manual Testing (if applicable)
Copilot review