From 5e951839214df6f0a7b0d461a49a3fc8a34249b2 Mon Sep 17 00:00:00 2001 From: Clinton Gillespie Date: Fri, 17 Jul 2026 14:47:33 +1000 Subject: [PATCH] fix: repopulate corrupts repeat groups when rows are deleted on the server (issue #1940) The Value Restoration step in getFilteredItemsToRepopulate spliced accepted deletions out of serverQRItems inside the same forward loop that fills unselected slots from current values. Splicing shifted unvisited deletion markers into already-visited indices (so they leaked into the QuestionnaireResponse) and re-opened their vacated tail slots to be re-filled from current values, resurrecting the deleted rows as duplicates. Split the loop into two passes: fill unselected slots first, then filter out all items marked with the mark-as-deleted extension. This also strips the internal marker extension from the final response and drops sparse-array holes. Co-Authored-By: Claude Fable 5 --- .../test/itemsToRepopulateSelector.test.ts | 80 +++++++++++++++++++ .../utils/itemsToRepopulateSelector.ts | 22 ++--- 2 files changed, 91 insertions(+), 11 deletions(-) diff --git a/apps/smart-forms-app/src/features/repopulate/test/itemsToRepopulateSelector.test.ts b/apps/smart-forms-app/src/features/repopulate/test/itemsToRepopulateSelector.test.ts index df308bf09..4497482fb 100644 --- a/apps/smart-forms-app/src/features/repopulate/test/itemsToRepopulateSelector.test.ts +++ b/apps/smart-forms-app/src/features/repopulate/test/itemsToRepopulateSelector.test.ts @@ -238,6 +238,86 @@ describe('getFilteredItemsToRepopulate', () => { // currentQRItem should stay undefined / as per original expect(filtered['63fe14f3-2374-4382-bce7-180e2747c97f'].currentQRItem).toBeUndefined(); }); + + // Regression tests for https://github.com/aehrc/smart-forms/issues/1940 + // Rows deleted on the server between repopulations: the dialog correctly shows the + // deletions, but applying them corrupted the result (deleted rows resurrected/duplicated + // and the internal mark-as-deleted extension leaked into the QuestionnaireResponse). + describe('server-side deletions in repeat groups', () => { + const markAsDeletedUrl = + 'https://smartforms.csiro.au/custom-functionality/repopulation/mark-as-deleted'; + + function buildRepeatGroupRow(value: string) { + return { + linkId: 'problem', + item: [{ linkId: 'problem-name', answer: [{ valueString: value }] }] + }; + } + + const rowA = buildRepeatGroupRow('A'); + const rowB = buildRepeatGroupRow('B'); + const rowC = buildRepeatGroupRow('C'); + + function buildDeletionScenario(): { + tuplesMap: Map; + originalItems: Record; + } { + // Form has rows [A, B, C]; rows A and B were deleted on the server, so it returns [C]. + // Detection (retrieveRepeatGroupCurrentQRItems) aligns matched pairs first: + // currentQRItems = [C, A, B], serverQRItems = [C] + // → dialog child 0 hidden (C unchanged), child 1 "A removed", child 2 "B removed" + const itemToRepopulate: ItemToRepopulate = { + qItem: { linkId: 'problem', text: 'Recorded problems', type: 'group', repeats: true }, + sectionItemText: 'Clinical details', + parentItemText: 'Clinical details', + isInGrid: false, + currentQRItems: [rowC, rowA, rowB], + serverQRItems: [rowC] + }; + + return { + tuplesMap: new Map([['Clinical details', [['problem', itemToRepopulate]]]]), + originalItems: { problem: itemToRepopulate } + }; + } + + it('removes all accepted deletions instead of resurrecting/duplicating rows', () => { + const { tuplesMap, originalItems } = buildDeletionScenario(); + + // Accept both deletions, as shown in the dialog + const selectedKeys = new Set(['heading-0-parent-0-child-1', 'heading-0-parent-0-child-2']); + + const filtered = getFilteredItemsToRepopulate(tuplesMap, selectedKeys, originalItems); + + // The dialog promised only row C remains + expect(filtered['problem'].serverQRItems).toEqual([rowC]); + }); + + it('does not leak the mark-as-deleted extension into the result', () => { + const { tuplesMap, originalItems } = buildDeletionScenario(); + + const selectedKeys = new Set(['heading-0-parent-0-child-1', 'heading-0-parent-0-child-2']); + + const filtered = getFilteredItemsToRepopulate(tuplesMap, selectedKeys, originalItems); + + const leakedMarkers = (filtered['problem'].serverQRItems ?? []).filter((serverQRItem) => + serverQRItem.extension?.some((ext) => ext.url === markAsDeletedUrl) + ); + expect(leakedMarkers).toEqual([]); + }); + + it('keeps unaccepted deletions in the form', () => { + const { tuplesMap, originalItems } = buildDeletionScenario(); + + // Accept only the deletion of row A; leave row B's deletion unselected + const selectedKeys = new Set(['heading-0-parent-0-child-1']); + + const filtered = getFilteredItemsToRepopulate(tuplesMap, selectedKeys, originalItems); + + // Row A removed, row B preserved from current values + expect(filtered['problem'].serverQRItems).toEqual([rowC, rowB]); + }); + }); }); describe('getChipColorByValueChangeMode', () => { diff --git a/apps/smart-forms-app/src/features/repopulate/utils/itemsToRepopulateSelector.ts b/apps/smart-forms-app/src/features/repopulate/utils/itemsToRepopulateSelector.ts index e884dbc86..cfa13867f 100644 --- a/apps/smart-forms-app/src/features/repopulate/utils/itemsToRepopulateSelector.ts +++ b/apps/smart-forms-app/src/features/repopulate/utils/itemsToRepopulateSelector.ts @@ -251,22 +251,22 @@ export function getFilteredItemsToRepopulate( filteredItem.serverQRItems = []; } + // Fill unselected slots with current values first, then drop accepted deletions in a + // separate pass. Doing both in a single loop corrupts the result: splicing shifts later + // items into already-visited indices (so a deletion marker can survive) and re-opens + // their vacated tail slots to be re-filled from current values (resurrecting deleted items). for (let i = 0; i < maxNumberOfItems; i++) { if (filteredItem.serverQRItems[i] === undefined && originalItem.currentQRItems[i]) { filteredItem.serverQRItems[i] = originalItem.currentQRItems[i]; } - - // If the original item is marked as deleted (has the https://smartforms.csiro.au/custom-functionality/repopulation/mark-as-deleted extension), remove the deleted item - if (filteredItem.serverQRItems[i]) { - const hasDeletedExtension = itemHasMarkAsDeletedExtension( - filteredItem.serverQRItems[i] - ); - - if (hasDeletedExtension) { - filteredItem.serverQRItems.splice(i, 1); - } - } } + + // Remove items accepted as deletions (marked with the https://smartforms.csiro.au/custom-functionality/repopulation/mark-as-deleted extension), + // so the marker never leaks into the final QuestionnaireResponse. Also drops any holes left in the sparse array. + filteredItem.serverQRItems = filteredItem.serverQRItems.filter( + (serverQRItem) => + serverQRItem !== undefined && !itemHasMarkAsDeletedExtension(serverQRItem) + ); } } }