From e2f70c5bd106fcd84a315b08be9e7a7a5370984a Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Mon, 27 Jul 2026 09:27:55 +0200 Subject: [PATCH 1/2] fix(submission): stop repeatable dropdowns duplicating and clearing values Follow-up to #26 and #32. Both reported problems still reproduced on datashare-UoEMainLibrary-dspace-8_x, because they are not caused by the dropdown widget alone. Deleting a row silently rebound the surviving rows to the wrong controls ------------------------------------------------------------------------ getControlOfGroup() stamped a `startingIndex` on a group model the first time the row rendered and resolved that row's FormGroup as control.get([startingIndex]) from then on. Nothing re-synced it, while DynamicFormArrayModel re-indexes its groups on every insert/remove/move and the template binds formGroupName to that live index. After deleting a non-last row the two disagreed: surviving rows resolved to another row's control or to null, and after delete+add two rows could alias onto the same control. That is what let a value the user never touched be overwritten, and what made the duplicate check read stale sibling values. The live index is now the single authority. Drag and keyboard reordering moved the group models only, which is what the frozen index had been compensating for, so both now move the control alongside the model (moveGroupAndControl). DsDynamicFormControlContainerComponent.ngOnChanges also dereferenced a null `group` in the tick after a row was removed, throwing inside change detection and aborting the pass for the rest of the field. A click aimed at dismissing the menu erased the value ----------------------------------------------------- The menu is a full-width overlay drawn over the field's own bottom edge and the rows beneath it, and its first entry was the destructive "Clear selection" - so the spot a user naturally clicks to dismiss the dropdown wiped their selection, row after row. Clearing now sits at the end of the menu, visually separated, and the menu keeps a small gap below the input. The clear entry also carried both (click) and (mousedown) and therefore fired twice per interaction; it now has a single handler, like the options. Options deliberately keep committing on (mousedown): blurring the input flips showErrorMessages on a required field and forceShowErrorDetection() destroys and re-creates the control, so the element is gone before a click event could reach it. This is now documented in the template. Duplicate detection ------------------- usedSiblingValues was a snapshot refreshed only in openDropdown()/selectEntry(), so it went stale whenever a row was added, removed or edited - and the toggle caret opens the menu through NgbDropdown without calling openDropdown() at all, leaving the set empty and nothing greyed out. It is now derived from the live models, and every path that opens the menu refreshes the options via (openChange). #32's canonicalKey() keyed on `authority ?? value`, but the same entry arrives as a VocabularyEntry when picked in-session and as a FormFieldMetadataValueObject when rebuilt from stored metadata, and only one side may carry an authority. The keys were then incomparable and the duplicate went through. Identity is now the authority *and* the normalised value, and a match on either is a duplicate. Testing ------- 14 specs written first against the unfixed code and confirmed failing, covering the row/control binding across remove/insert/reorder and every duplicate and clearing path. Full suite: 5478 passing. Verified end to end on a dockerised DSpace 8 backend with the datashare theme: the three Type rows survive the dismissing click, the still-used values are greyed out after delete+add (input and caret alike), and what the server stores matches the form with no duplicate and nothing destroyed. Co-Authored-By: Claude Opus 5 (1M context) --- ...ynamic-form-control-container.component.ts | 5 +- ...c-form-array.component.row-binding.spec.ts | 185 +++++++++++ .../dynamic-form-array.component.ts | 42 ++- ...dynamic-scrollable-dropdown.component.html | 23 +- ...dynamic-scrollable-dropdown.component.scss | 10 + ...amic-scrollable-dropdown.component.spec.ts | 25 +- .../dynamic-scrollable-dropdown.component.ts | 68 ++-- ...amic-scrollable-dropdown.duplicate.spec.ts | 310 ++++++++++++++++++ .../dynamic-scrollable-dropdown.repro.spec.ts | 3 +- 9 files changed, 610 insertions(+), 61 deletions(-) create mode 100644 src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.row-binding.spec.ts create mode 100644 src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.duplicate.spec.ts diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts index 48a8ebbc0cc..cc64784b626 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts @@ -306,7 +306,10 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo } ngOnChanges(changes: SimpleChanges) { - if (changes && !this.isRelationship && hasValue(this.group.get(this.model.id))) { + // `group` can legitimately be null for one tick while a repeatable field is being re-rendered + // (a row was added or removed). Dereferencing it there threw inside change detection and + // aborted the pass for the whole field, so the remaining rows stopped updating. + if (changes && !this.isRelationship && hasValue(this.group) && hasValue(this.group.get(this.model.id))) { super.ngOnChanges(changes); if (this.model && this.model.placeholder) { this.model.placeholder = this.translateService.instant(this.model.placeholder); diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.row-binding.spec.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.row-binding.spec.ts new file mode 100644 index 00000000000..88c991d3bce --- /dev/null +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.row-binding.spec.ts @@ -0,0 +1,185 @@ +// Regression specs for the row -> FormControl binding of a repeatable field. +// +// `getControlOfGroup()` used to stamp a `startingIndex` on the group model the first time a row +// rendered and then resolve that row's FormGroup as `control.get([startingIndex])` forever. +// Nothing re-synced it when the FormArray was mutated, so after removing a non-last row the +// surviving rows bound to the wrong FormGroup (or to null), and after remove+insert two rows +// could alias onto the same FormGroup. +// +// That is what let a value the user never touched be overwritten, and what made the duplicate +// detection in DsDynamicScrollableDropdownComponent read stale sibling values. +import { HttpClient } from '@angular/common/http'; +import { EventEmitter } from '@angular/core'; +import { + ComponentFixture, + inject, + TestBed, +} from '@angular/core/testing'; +import { + ReactiveFormsModule, + UntypedFormArray, +} from '@angular/forms'; +import { + DYNAMIC_FORM_CONTROL_MAP_FN, + DynamicFormLayoutService, + DynamicFormService, + DynamicFormValidationService, + DynamicInputModel, +} from '@ng-dynamic-forms/core'; +import { provideMockStore } from '@ngrx/store/testing'; +import { + TranslateModule, + TranslateService, +} from '@ngx-translate/core'; +import { NgxMaskModule } from 'ngx-mask'; +import { of } from 'rxjs'; + +import { + APP_CONFIG, + APP_DATA_SERVICES_MAP, +} from '../../../../../../../config/app-config.interface'; +import { environment } from '../../../../../../../environments/environment.test'; +import { SubmissionService } from '../../../../../../submission/submission.service'; +import { LiveRegionService } from '../../../../../live-region/live-region.service'; +import { getLiveRegionServiceStub } from '../../../../../live-region/live-region.service.stub'; +import { DsDynamicFormControlContainerComponent } from '../../ds-dynamic-form-control-container.component'; +import { dsDynamicFormControlMapFn } from '../../ds-dynamic-form-control-map-fn'; +import { DynamicRowArrayModel } from '../ds-dynamic-row-array-model'; +import { DsDynamicFormArrayComponent } from './dynamic-form-array.component'; + +describe('DsDynamicFormArrayComponent row/control binding', () => { + const translateServiceStub = { + get: () => of('translated-text'), + instant: () => 'translated-text', + onLangChange: new EventEmitter(), + onTranslationChange: new EventEmitter(), + onDefaultLangChange: new EventEmitter(), + }; + + let component: DsDynamicFormArrayComponent; + let fixture: ComponentFixture; + // FormBuilderService only inherits these from DynamicFormService; using the base service keeps + // the fixture free of the whole submission dependency graph. + let forms: DynamicFormService; + + /** The FormArray backing the rows of the repeatable field. */ + const formArray = (): UntypedFormArray => component.group.get(component.model.id) as UntypedFormArray; + + /** Value currently held by the control that row `i` is bound to. */ + const boundValue = (i: number): any => + (component.getControlOfGroup(component.model.groups[i]) as any)?.get('rowInput')?.value; + + beforeEach(async () => { + await TestBed.configureTestingModule({ + imports: [ + ReactiveFormsModule, + DsDynamicFormArrayComponent, + NgxMaskModule.forRoot(), + TranslateModule.forRoot(), + ], + providers: [ + DynamicFormLayoutService, + DynamicFormValidationService, + provideMockStore(), + { provide: APP_DATA_SERVICES_MAP, useValue: {} }, + { provide: TranslateService, useValue: translateServiceStub }, + { provide: HttpClient, useValue: {} }, + { provide: SubmissionService, useValue: {} }, + { provide: APP_CONFIG, useValue: environment }, + { provide: DYNAMIC_FORM_CONTROL_MAP_FN, useValue: dsDynamicFormControlMapFn }, + { provide: LiveRegionService, useValue: getLiveRegionServiceStub() }, + ], + }).overrideComponent(DsDynamicFormArrayComponent, { + remove: { imports: [DsDynamicFormControlContainerComponent] }, + }).compileComponents(); + }); + + beforeEach(inject([DynamicFormService], + (service: DynamicFormService) => { + forms = service; + + const formModel = [ + new DynamicRowArrayModel({ + id: 'typeArray', + initialCount: 3, + notRepeatable: false, + relationshipConfig: undefined, + submissionId: '1234', + isDraggable: true, + groupFactory: () => [new DynamicInputModel({ id: 'rowInput' })], + required: false, + metadataKey: 'dc.type', + metadataFields: ['dc.type'], + hasSelectableMetadata: true, + showButtons: true, + }), + ]; + + fixture = TestBed.createComponent(DsDynamicFormArrayComponent); + component = fixture.componentInstance; + component.model = formModel[0] as DynamicRowArrayModel; + component.group = service.createFormGroup(formModel); + fixture.detectChanges(); + + // Give each row a distinct value, as if the user had picked three Types. + ['Article', 'Book', 'Dataset'].forEach((value, i) => { + formArray().at(i).get('rowInput').setValue(value); + (component.model.groups[i].group[0] as any).value = value; + }); + + // Render once so every row gets its binding resolved (this is what used to freeze it). + component.model.groups.forEach((g) => component.getControlOfGroup(g)); + })); + + it('binds every row to its own control before any mutation', () => { + expect([boundValue(0), boundValue(1), boundValue(2)]).toEqual(['Article', 'Book', 'Dataset']); + }); + + it('rebinds the surviving rows after the MIDDLE row is removed', () => { + forms.removeFormArrayGroup(1, formArray(), component.model); + fixture.detectChanges(); + + expect(component.model.groups.length).withContext('one row removed').toBe(2); + expect(boundValue(0)).withContext('row 0 keeps Article').toBe('Article'); + expect(boundValue(1)).withContext('row 1 must now resolve to Dataset, not to Book or null').toBe('Dataset'); + }); + + it('never resolves two rows to the same control after remove + insert', () => { + forms.removeFormArrayGroup(1, formArray(), component.model); + forms.insertFormArrayGroup(component.model.groups.length, formArray(), component.model); + fixture.detectChanges(); + + const controls = component.model.groups.map((g) => component.getControlOfGroup(g)); + expect(controls.length).toBe(3); + controls.forEach((c, i) => expect(c).withContext(`row ${i} must be bound to a control`).not.toBeNull()); + expect(new Set(controls).size).withContext('each row must own a distinct FormGroup').toBe(controls.length); + }); + + it('keeps the surviving row\'s value intact after remove + insert', () => { + forms.removeFormArrayGroup(1, formArray(), component.model); + forms.insertFormArrayGroup(component.model.groups.length, formArray(), component.model); + fixture.detectChanges(); + + expect(boundValue(0)).withContext('Article untouched').toBe('Article'); + expect(boundValue(1)).withContext('Dataset must survive the delete+add').toBe('Dataset'); + expect(boundValue(2)).withContext('the freshly added row is empty').toBeFalsy(); + }); + + it('rebinds after the FIRST row is removed', () => { + forms.removeFormArrayGroup(0, formArray(), component.model); + fixture.detectChanges(); + + expect(boundValue(0)).withContext('Book moved up into position 0').toBe('Book'); + expect(boundValue(1)).withContext('Dataset moved up into position 1').toBe('Dataset'); + }); + + it('rebinds after a row is inserted in the middle', () => { + forms.insertFormArrayGroup(1, formArray(), component.model); + fixture.detectChanges(); + + expect(boundValue(0)).toBe('Article'); + expect(boundValue(1)).withContext('the inserted row is empty').toBeFalsy(); + expect(boundValue(2)).withContext('Book shifted down').toBe('Book'); + expect(boundValue(3)).withContext('Dataset shifted down').toBe('Dataset'); + }); +}); diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts index 220143291eb..9d86a808bda 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts @@ -19,10 +19,12 @@ import { } from '@angular/core'; import { ReactiveFormsModule, + UntypedFormArray, UntypedFormGroup, } from '@angular/forms'; import { DynamicFormArrayComponent, + DynamicFormArrayGroupModel, DynamicFormControlCustomEvent, DynamicFormControlEvent, DynamicFormControlLayout, @@ -96,7 +98,7 @@ export class DsDynamicFormArrayComponent extends DynamicFormArrayComponent { return; } - this.model.moveGroup(event.previousIndex, event.currentIndex - event.previousIndex); + this.moveGroupAndControl(event.previousIndex, event.currentIndex); const prevIndex = event.previousIndex; const index = event.currentIndex; @@ -127,16 +129,38 @@ export class DsDynamicFormArrayComponent extends DynamicFormArrayComponent { } /** - * Gets the control of the specified group model. It adds the startingIndex property to the group model if it does not - * already have it. This ensures that the controls are always linked to the correct group model. + * Moves a row, keeping the group models and the form controls in the same order. + * + * Both are addressed by the same live index (see {@link getControlOfGroup}), so reordering only + * the models would leave every row from the drop position onwards pointing at another row's + * control — which is how values ended up moving between rows. + * + * @param from The index the row is moved from. + * @param to The index the row is moved to. + */ + protected moveGroupAndControl(from: number, to: number): void { + const controls = this.control as unknown as UntypedFormArray; + if (hasValue(controls?.at(from))) { + const moved = controls.at(from); + controls.removeAt(from, { emitEvent: false }); + controls.insert(to, moved, { emitEvent: false }); + } + this.model.moveGroup(from, to - from); + } + + /** + * Gets the control of the specified group model. + * + * The group's *live* index is the only authority: `DynamicFormArrayModel` re-indexes its groups + * on every insert/remove/move, and the template binds `formGroupName` to that same index. Caching + * the index of the first render instead would leave a row pointing at another row's control (or at + * none) as soon as a row is added or removed, which silently moves values between rows. + * * @param groupModel The group model to get the control for. * @returns The form control of the specified group model. */ - getControlOfGroup(groupModel: any) { - if (!groupModel.hasOwnProperty('startingIndex')) { - groupModel.startingIndex = groupModel.index; - } - return this.control.get([groupModel.startingIndex]); + getControlOfGroup(groupModel: DynamicFormArrayGroupModel) { + return this.control.get([groupModel.index]); } /** @@ -200,7 +224,7 @@ export class DsDynamicFormArrayComponent extends DynamicFormArrayComponent { } if (this.elementBeingSorted) { - this.model.moveGroup(idx, newIndex - idx); + this.moveGroupAndControl(idx, newIndex); if (hasValue(this.model.groups[newIndex]) && hasValue((this.control as any).controls[newIndex])) { this.onCustomEvent({ previousIndex: idx, diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.html b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.html index eb523d91c58..d834b8482c2 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.html +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.html @@ -1,4 +1,5 @@ -
+
- +
diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.scss b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.scss index c91bf094761..9e8b6eef470 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.scss +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.scss @@ -30,6 +30,16 @@ left: 0 !important; margin-bottom: var(--bs-spacer); z-index: 1000; + // The menu is positioned flush against the input and covers the rows underneath it. Keeping a + // small gap leaves a strip right below the field where a click closes the menu instead of + // activating whichever entry happens to be drawn there. + margin-top: 0.35rem; +} + +// Clearing is separated from the options so it cannot be hit by accident. +.collection-item.scrollable-dropdown-clear { + border-bottom: 0; + border-top: calc(var(--bs-dropdown-border-width) * 2) solid var(--bs-dropdown-border-color); } .scrollable-dropdown-toggle { diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.spec.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.spec.ts index 0a0655c1162..14d96afcc34 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.spec.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.spec.ts @@ -186,17 +186,12 @@ describe('Dynamic Dynamic Scrollable Dropdown component', () => { const selectedValue = Object.assign(new VocabularyEntry(), { authority: 1, display: 'one', value: 1 }); let de: any = scrollableDropdownFixture.debugElement.query(By.css('input.form-control')); - let btnEl = de.nativeElement; - - const mousedownEvent = new MouseEvent('mousedown'); - - btnEl.dispatchEvent(mousedownEvent); + de.nativeElement.click(); scrollableDropdownFixture.detectChanges(); - de = scrollableDropdownFixture.debugElement.queryAll(By.css('button.dropdown-item')); - btnEl = de[1].nativeElement; - - btnEl.dispatchEvent(mousedownEvent); + // Options commit on mousedown by design (the control is re-created on blur). + de = scrollableDropdownFixture.debugElement.queryAll(By.css('button.dropdown-item.collection-item')); + de[0].nativeElement.dispatchEvent(new MouseEvent('mousedown')); scrollableDropdownFixture.detectChanges(); expect((scrollableDropdownComp.model as any).value).toEqual(selectedValue); @@ -244,21 +239,19 @@ describe('Dynamic Dynamic Scrollable Dropdown component', () => { it('should disable an option already selected in another row of the same field', () => { const de = scrollableDropdownFixture.debugElement.query(By.css('input.form-control')); - de.nativeElement.dispatchEvent(new MouseEvent('mousedown')); de.nativeElement.click(); scrollableDropdownFixture.detectChanges(); - expect(scrollableDropdownComp.usedSiblingValues.has(1)).toBeTruthy(); expect(scrollableDropdownComp.isOptionDisabled({ value: 1 })).toBeTruthy(); expect(scrollableDropdownComp.isOptionDisabled({ value: 2 })).toBeFalsy(); + // The options are rendered first; the clear entry is last. const options = scrollableDropdownFixture.debugElement.queryAll(By.css('button.dropdown-item.collection-item')); - expect(hasClass(options[1].nativeElement, 'disabled')).toBeTruthy(); - expect(hasClass(options[2].nativeElement, 'disabled')).toBeFalsy(); + expect(hasClass(options[0].nativeElement, 'disabled')).toBeTruthy(); + expect(hasClass(options[1].nativeElement, 'disabled')).toBeFalsy(); }); it('should ignore selection of a disabled option', () => { - scrollableDropdownComp.usedSiblingValues = new Set([1]); spyOn(scrollableDropdownComp.change, 'emit'); scrollableDropdownComp.onSelect(Object.assign(new VocabularyEntry(), { authority: 1, display: 'one', value: 1 })); @@ -268,7 +261,6 @@ describe('Dynamic Dynamic Scrollable Dropdown component', () => { }); it('should not select a disabled option via keyboard and keep the dropdown open', () => { - scrollableDropdownComp.usedSiblingValues = new Set([1]); scrollableDropdownComp.optionsList = [Object.assign(new VocabularyEntry(), { authority: 1, display: 'one', value: 1 })]; scrollableDropdownComp.selectedIndex = 0; spyOn(scrollableDropdownComp.change, 'emit'); @@ -293,7 +285,6 @@ describe('Dynamic Dynamic Scrollable Dropdown component', () => { }); it('should still allow clearing the value', () => { - scrollableDropdownComp.usedSiblingValues = new Set([1]); spyOn(scrollableDropdownComp.change, 'emit'); scrollableDropdownComp.onSelect(undefined); @@ -304,8 +295,6 @@ describe('Dynamic Dynamic Scrollable Dropdown component', () => { it('should not disable any option for a field without repeatable siblings', () => { scrollableDropdownComp.model = new DynamicScrollableDropdownModel(SD_TEST_MODEL_CONFIG); - scrollableDropdownComp.openDropdown({ open: () => undefined } as any); - expect(scrollableDropdownComp.usedSiblingValues.size).toBe(0); expect(scrollableDropdownComp.isOptionDisabled({ value: 1 })).toBeFalsy(); }); diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts index d5ec8e44996..1d3024b433c 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts @@ -103,8 +103,6 @@ export class DsDynamicScrollableDropdownComponent extends DsDynamicVocabularyCom public selectedIndex = 0; public acceptableKeys = ['Space', 'NumpadMultiply', 'NumpadAdd', 'NumpadSubtract', 'NumpadDecimal', 'Semicolon', 'Equal', 'Comma', 'Minus', 'Period', 'Quote', 'Backquote']; - public usedSiblingValues: Set = new Set(); - /** * If true the component can rely on the findAll method for data loading. * This is a behaviour activated by dependency injection through the dropdown config. @@ -207,20 +205,34 @@ export class DsDynamicScrollableDropdownComponent extends DsDynamicVocabularyCom */ openDropdown(sdRef: NgbDropdown) { if (!this.model.readOnly) { + sdRef.open(); + } + } + + /** + * Called by the NgbDropdown itself, so it covers every way the menu can be opened — the input, + * the keyboard, and the toggle caret, which opens the menu through the directive without going + * through {@link openDropdown} and therefore used to show an unfiltered, stale option list. + * + * @param open Whether the dropdown is now open. + */ + onOpenChange(open: boolean) { + if (open) { this.group.markAsUntouched(); this.inputText = null; - this.usedSiblingValues = this.getUsedSiblingValues(); this.updatePageInfo(this.model.maxOptions, 1); this.loadOptions(false); - sdRef.open(); } } /** - * Build the set of canonical identities already selected in the OTHER rows of - * the same repeatable field, so those options can be disabled/skipped. + * The identities already selected in the OTHER rows of the same repeatable field. + * + * Always read from the live models. A snapshot taken when the dropdown was opened went stale as + * soon as any row was added, removed or edited, which both hid options that had become free again + * and offered options that were in use. */ - private getUsedSiblingValues(): Set { + get usedSiblingValues(): Set { const used = new Set(); const parent = this.model.parent; if (parent instanceof DynamicFormArrayGroupModel) { @@ -230,10 +242,7 @@ export class DsDynamicScrollableDropdownComponent extends DsDynamicVocabularyCom rowGroup.group .filter((siblingModel) => siblingModel.name === this.model.name) .forEach((siblingModel) => { - const canonical = this.canonicalKey((siblingModel as any).value); - if (isNotEmpty(canonical)) { - used.add(canonical); - } + this.identityKeys((siblingModel as any).value).forEach((key) => used.add(key)); }); }); } @@ -241,30 +250,41 @@ export class DsDynamicScrollableDropdownComponent extends DsDynamicVocabularyCom } /** - * Canonical identity of a vocabulary value/entry used for duplicate detection. - * For authority-controlled vocabularies (e.g. Funder) the authority is the - * stable identity; otherwise the plain value is used. A bare string value is - * returned as-is. + * Every identity a vocabulary value can be recognised by. + * + * The same entry reaches this component in two different shapes: as a `VocabularyEntry` when it + * was picked in this session, and as a `FormFieldMetadataValueObject` when it was rebuilt from + * the stored metadata — and only one of the two may carry an authority. Comparing a single + * "canonical" key therefore missed duplicates whenever the two sides disagreed about it, so both + * the authority and the normalised value are emitted and a match on either one is a duplicate. */ - private canonicalKey(entry: any): any { + private identityKeys(entry: any): string[] { if (isEmpty(entry)) { - return null; + return []; } if (typeof entry === 'string') { - return entry; + return [`v:${entry.trim().toLowerCase()}`]; } - return isNotEmpty(entry.authority) ? entry.authority : entry.value; + const keys = []; + if (isNotEmpty(entry.authority)) { + keys.push(`a:${entry.authority}`); + } + if (isNotEmpty(entry.value)) { + keys.push(`v:${String(entry.value).trim().toLowerCase()}`); + } + return keys; } isOptionDisabled(entry: any): boolean { - const canonical = this.canonicalKey(entry); - return isNotEmpty(canonical) && this.usedSiblingValues.has(canonical); + const keys = this.identityKeys(entry); + if (isEmpty(keys)) { + return false; + } + const used = this.usedSiblingValues; + return keys.some((key) => used.has(key)); } selectEntry(entry: any, sdRef: NgbDropdown) { - // Refresh against the live sibling values first, so a value that was chosen - // in another row after this dropdown was opened is still blocked at commit. - this.usedSiblingValues = this.getUsedSiblingValues(); if (this.isOptionDisabled(entry)) { return; } diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.duplicate.spec.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.duplicate.spec.ts new file mode 100644 index 00000000000..bf776a92255 --- /dev/null +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.duplicate.spec.ts @@ -0,0 +1,310 @@ +// Regression specs for the two problems reported against #26/#32 on a repeatable, +// vocabulary-backed submission dropdown (Type, Funder): +// +// BUG A - an option already chosen in another row could be picked again, producing +// duplicate metadata. Three separate gaps: the caret opens the menu without +// refreshing the used-value state, the state was a snapshot that nothing +// invalidated, and the identity used for comparison did not survive a +// server round-trip (authority vs plain value). +// +// BUG B - clicking "next to" an open dropdown wiped the selected values, because the +// options commit on mousedown and the destructive "Clear selection" entry sits +// directly under the input, where a dismissing click lands. +import { + ChangeDetectorRef, + CUSTOM_ELEMENTS_SCHEMA, + Injector, +} from '@angular/core'; +import { + ComponentFixture, + fakeAsync, + TestBed, + tick, + waitForAsync, +} from '@angular/core/testing'; +import { + FormsModule, + ReactiveFormsModule, + UntypedFormControl, + UntypedFormGroup, +} from '@angular/forms'; +import { By } from '@angular/platform-browser'; +import { NgbModule } from '@ng-bootstrap/ng-bootstrap'; +import { + DynamicFormArrayModel, + DynamicFormLayoutService, + DynamicFormsCoreModule, + DynamicFormValidationService, +} from '@ng-dynamic-forms/core'; +import { DynamicFormsNGBootstrapUIModule } from '@ng-dynamic-forms/ui-ng-bootstrap'; +import { TranslateModule } from '@ngx-translate/core'; +import { InfiniteScrollModule } from 'ngx-infinite-scroll'; + +import { APP_DATA_SERVICES_MAP } from '../../../../../../../config/app-config.interface'; +import { VocabularyEntry } from '../../../../../../core/submission/vocabularies/models/vocabulary-entry.model'; +import { VocabularyOptions } from '../../../../../../core/submission/vocabularies/models/vocabulary-options.model'; +import { VocabularyService } from '../../../../../../core/submission/vocabularies/vocabulary.service'; +import { NotificationsService } from '../../../../../notifications/notifications.service'; +import { + mockDynamicFormLayoutService, + mockDynamicFormValidationService, +} from '../../../../../testing/dynamic-form-mock-services'; +import { NotificationsServiceStub } from '../../../../../testing/notifications-service.stub'; +import { VocabularyServiceStub } from '../../../../../testing/vocabulary-service.stub'; +import { FormFieldMetadataValueObject } from '../../../models/form-field-metadata-value.model'; +import { DsDynamicScrollableDropdownComponent } from './dynamic-scrollable-dropdown.component'; +import { DynamicScrollableDropdownModel } from './dynamic-scrollable-dropdown.model'; + +const MODEL_CONFIG = { + vocabularyOptions: { closed: false, name: 'common_types' } as VocabularyOptions, + disabled: false, + errorMessages: { required: 'Required field.' }, + id: 'dropdown', + label: 'Type', + maxOptions: 10, + name: 'dropdown', + placeholder: 'Type', + readOnly: false, + required: false, + repeatable: true, + value: undefined, + metadataFields: [], + submissionId: '1234', + hasSelectableMetadata: false, +}; + +const entry = (value: any, authority: any = null, display?: string) => + Object.assign(new VocabularyEntry(), { authority, value, display: display ?? value }); + +describe('DsDynamicScrollableDropdownComponent duplicate/clearing regressions', () => { + let fixture: ComponentFixture; + let comp: DsDynamicScrollableDropdownComponent; + const vocabularyServiceStub = new VocabularyServiceStub(); + + beforeEach(waitForAsync(() => { + vocabularyServiceStub.setNewPayload([ + entry('Article'), entry('Book'), entry('Dataset'), + ]); + TestBed.configureTestingModule({ + imports: [ + DynamicFormsCoreModule, + DynamicFormsNGBootstrapUIModule, + FormsModule, + InfiniteScrollModule, + ReactiveFormsModule, + NgbModule, + TranslateModule.forRoot(), + DsDynamicScrollableDropdownComponent, + ], + providers: [ + { provide: NotificationsService, useValue: new NotificationsServiceStub() }, + Injector, + ChangeDetectorRef, + DsDynamicScrollableDropdownComponent, + { provide: VocabularyService, useValue: vocabularyServiceStub }, + { provide: DynamicFormLayoutService, useValue: mockDynamicFormLayoutService }, + { provide: DynamicFormValidationService, useValue: mockDynamicFormValidationService }, + { provide: APP_DATA_SERVICES_MAP, useValue: {} }, + ], + schemas: [CUSTOM_ELEMENTS_SCHEMA], + }); + })); + + /** Build a repeatable field of `count` rows, wiring parents the way the form service does. */ + function buildArray(count: number): DynamicFormArrayModel { + const arrayModel = new DynamicFormArrayModel({ + id: 'dropdownArray', + groupFactory: () => [new DynamicScrollableDropdownModel(MODEL_CONFIG)], + initialCount: count, + }); + arrayModel.groups.forEach((g) => g.group.forEach((m) => ((m as any).parent = g))); + return arrayModel; + } + + function bindTo(rowModel: DynamicScrollableDropdownModel) { + fixture = TestBed.createComponent(DsDynamicScrollableDropdownComponent); + comp = fixture.componentInstance; + comp.group = new UntypedFormGroup({ dropdown: new UntypedFormControl() }); + comp.model = rowModel; + fixture.detectChanges(); + } + + const fakeRef: any = { open: () => undefined, close: () => undefined, isOpen: () => false }; + + // ------------------------------------------------------------------ BUG A + describe('BUG A - duplicate values', () => { + + it('disables a sibling value WITHOUT the dropdown having been opened first', fakeAsync(() => { + // The caret (ngbDropdownToggle) opens the menu without going through openDropdown(), + // so the disabled state must not depend on openDropdown() having run. + const arr = buildArray(2); + (arr.get(0).group[0] as any).value = entry('Article'); + bindTo(arr.get(1).group[0] as any); + tick(); + + expect(comp.isOptionDisabled(entry('Article'))) + .withContext('used by the sibling row -> must be disabled even before openDropdown()').toBeTruthy(); + expect(comp.isOptionDisabled(entry('Book'))) + .withContext('free -> selectable').toBeFalsy(); + })); + + it('re-evaluates when a sibling row changes AFTER the dropdown was opened', fakeAsync(() => { + const arr = buildArray(2); + bindTo(arr.get(1).group[0] as any); + comp.openDropdown(fakeRef); + tick(); + + expect(comp.isOptionDisabled(entry('Article'))).withContext('nothing used yet').toBeFalsy(); + + // Another row gains the value while this menu is already open. + (arr.get(0).group[0] as any).value = entry('Article'); + + expect(comp.isOptionDisabled(entry('Article'))) + .withContext('must reflect the sibling chosen after opening').toBeTruthy(); + })); + + it('stops disabling a value once the row that used it is removed', fakeAsync(() => { + const arr = buildArray(3); + (arr.get(0).group[0] as any).value = entry('Article'); + (arr.get(1).group[0] as any).value = entry('Book'); + bindTo(arr.get(2).group[0] as any); + comp.openDropdown(fakeRef); + tick(); + expect(comp.isOptionDisabled(entry('Book'))).withContext('Book is used').toBeTruthy(); + + arr.removeGroup(1); // the user deletes the "Book" row + + expect(comp.isOptionDisabled(entry('Book'))) + .withContext('Book is free again -> must not stay greyed out').toBeFalsy(); + expect(comp.isOptionDisabled(entry('Article'))) + .withContext('Article is still used').toBeTruthy(); + })); + + it('detects a duplicate when the sibling lost its authority in a server round-trip', fakeAsync(() => { + // Picked in-session the value is a VocabularyEntry carrying an authority; rebuilt from the + // server it is a FormFieldMetadataValueObject whose authority may be null. Keying on + // `authority ?? value` made the two incomparable. + const arr = buildArray(2); + (arr.get(0).group[0] as any).value = new FormFieldMetadataValueObject('Article', null, null, 'Article'); + bindTo(arr.get(1).group[0] as any); + comp.openDropdown(fakeRef); + tick(); + + expect(comp.isOptionDisabled(entry('Article', 'auth-article'))) + .withContext('same value, authority only on the option -> still a duplicate').toBeTruthy(); + })); + + it('detects a duplicate when only the sibling carries an authority', fakeAsync(() => { + const arr = buildArray(2); + (arr.get(0).group[0] as any).value = new FormFieldMetadataValueObject('Article', null, 'auth-article', 'Article'); + bindTo(arr.get(1).group[0] as any); + comp.openDropdown(fakeRef); + tick(); + + expect(comp.isOptionDisabled(entry('Article'))) + .withContext('same value, authority only on the sibling -> still a duplicate').toBeTruthy(); + })); + + it('still matches on authority when the displayed values differ', fakeAsync(() => { + const arr = buildArray(2); + (arr.get(0).group[0] as any).value = new FormFieldMetadataValueObject('NSF', null, 'auth-1', 'NSF'); + bindTo(arr.get(1).group[0] as any); + comp.openDropdown(fakeRef); + tick(); + + expect(comp.isOptionDisabled(entry('National Science Foundation', 'auth-1'))) + .withContext('same authority -> same entity -> duplicate').toBeTruthy(); + })); + + it('refuses to commit a duplicate even if the option is somehow activated', fakeAsync(() => { + const arr = buildArray(2); + (arr.get(0).group[0] as any).value = entry('Article'); + const rowModel = arr.get(1).group[0] as any; + bindTo(rowModel); + tick(); + + spyOn(comp.change, 'emit'); + comp.selectEntry(entry('Article'), fakeRef); + + expect(comp.change.emit).not.toHaveBeenCalled(); + expect(rowModel.value).toBeUndefined(); + })); + + it('does not disable anything for a field that is not repeatable', fakeAsync(() => { + bindTo(new DynamicScrollableDropdownModel(MODEL_CONFIG)); + comp.openDropdown(fakeRef); + tick(); + + expect(comp.isOptionDisabled(entry('Article'))).toBeFalsy(); + })); + }); + + // ------------------------------------------------------------------ BUG B + describe('BUG B - values cleared by a click next to the field', () => { + + beforeEach(fakeAsync(() => { + const arr = buildArray(1); + bindTo(arr.get(0).group[0] as any); + comp.optionsList = [entry('Article'), entry('Book'), entry('Dataset')]; + fixture.detectChanges(); + tick(); + })); + + it('clears exactly once for one full mouse interaction, not twice', () => { + // The clear entry used to carry BOTH (click) and (mousedown), so a real mouse press fired + // onSelect(undefined) twice and produced two change events / two JSON patches. + const clear = fixture.debugElement.query(By.css('button.dropdown-item.scrollable-dropdown-clear')); + expect(clear).withContext('the clear entry is rendered').not.toBeNull(); + + spyOn(comp, 'onSelect'); + clear.nativeElement.dispatchEvent(new MouseEvent('mousedown', { bubbles: true })); + clear.nativeElement.dispatchEvent(new MouseEvent('mouseup', { bubbles: true })); + clear.nativeElement.dispatchEvent(new MouseEvent('click', { bubbles: true })); + + expect(comp.onSelect).toHaveBeenCalledTimes(1); + }); + + it('commits an option exactly once for one full mouse interaction', () => { + // Options intentionally commit on mousedown: blurring the input flips showErrorMessages on a + // required field and DsDynamicFormControlContainerComponent then destroys and re-creates this + // control, so the element is gone before a click event could reach it. Committing on + // mousedown must therefore not be paired with a second handler. + const option = fixture.debugElement.queryAll(By.css('button.dropdown-item.collection-item')) + .find((de) => (de.nativeElement.textContent || '').trim() === 'Article'); + expect(option).withContext('the Article option is rendered').toBeDefined(); + + spyOn(comp, 'selectEntry'); + option.nativeElement.dispatchEvent(new MouseEvent('mousedown', { bubbles: true })); + option.nativeElement.dispatchEvent(new MouseEvent('mouseup', { bubbles: true })); + option.nativeElement.dispatchEvent(new MouseEvent('click', { bubbles: true })); + + expect(comp.selectEntry).toHaveBeenCalledTimes(1); + }); + + it('does not put the destructive clear entry directly under the input', () => { + // The menu overlays the field and the rows beneath it, so whatever sits at the top of the + // menu is what a dismissing click lands on. That must never be "Clear selection". + const items = fixture.debugElement.queryAll(By.css('.scrollable-menu button.dropdown-item')); + const firstItem = items[0].nativeElement; + + expect(firstItem.classList.contains('scrollable-dropdown-clear')) + .withContext('the first entry of the menu must not be the clear action').toBeFalsy(); + }); + + it('keeps the clear entry available (further down the menu)', () => { + const clear = fixture.debugElement.query(By.css('button.dropdown-item.scrollable-dropdown-clear')); + expect(clear).withContext('clearing must still be possible').not.toBeNull(); + }); + + it('clears the value when the clear entry is actually chosen', fakeAsync(() => { + (comp.model as any).value = entry('Article'); + spyOn(comp.change, 'emit'); + + comp.onSelect(undefined); + tick(); + + expect(comp.change.emit).toHaveBeenCalled(); + expect((comp.model as any).value).toBeFalsy(); + })); + }); +}); diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.repro.spec.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.repro.spec.ts index 1005a01df25..fcd44d507d3 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.repro.spec.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.repro.spec.ts @@ -152,7 +152,7 @@ describe('REPRO PR#26 – duplicate + clear-on-blur', () => { bindComponentToRow(arr.get(1).group[0] as any); // component = row1 comp.openDropdown(fakeRef); tick(); - expect(comp.usedSiblingValues.has(1)).withContext('value 1 used by row0').toBeTruthy(); + expect(comp.isOptionDisabled(entry(1))).withContext('value 1 used by row0').toBeTruthy(); expect(comp.isOptionDisabled({ value: 1 })).withContext('option 1 disabled').toBeTruthy(); expect(comp.isOptionDisabled({ value: 2 })).withContext('option 2 free').toBeFalsy(); })); @@ -183,7 +183,6 @@ describe('REPRO PR#26 – duplicate + clear-on-blur', () => { bindComponentToRow(arr.get(1).group[0] as any); comp.openDropdown(fakeRef); tick(); - expect(comp.usedSiblingValues.has('one')).withContext('FFMVO.value=one collected').toBeTruthy(); expect(comp.isOptionDisabled({ value: 'one' })).withContext('option one disabled').toBeTruthy(); })); From 86a185ceeb1fae51cca79418a398362b5f668a1b Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Mon, 27 Jul 2026 10:12:37 +0200 Subject: [PATCH 2/2] fix(submission): keep keyboard reorder-cancel and clear-entry consistent Both from the Copilot review of this PR; both are regressions this PR introduced. cancelKeyboardDragAndDrop() still moved the group models only, so Escape after a keyboard reorder restored the model order while leaving the FormArray as it was - the exact desync this PR set out to remove. It now goes through moveGroupAndControl() like the drag and arrow-key paths. Dropping the clear entry's (click) handler to stop it firing twice per mouse interaction also removed its keyboard activation, since a native button turns Enter into a click. It now carries (keydown.enter) alongside (mousedown), which mirrors how the options are bound. Co-Authored-By: Claude Opus 5 (1M context) --- ...c-form-array.component.row-binding.spec.ts | 32 +++++++++++++++++++ .../dynamic-form-array.component.ts | 2 +- ...dynamic-scrollable-dropdown.component.html | 4 ++- ...amic-scrollable-dropdown.duplicate.spec.ts | 11 +++++++ 4 files changed, 47 insertions(+), 2 deletions(-) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.row-binding.spec.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.row-binding.spec.ts index 88c991d3bce..e15c215da6b 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.row-binding.spec.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.row-binding.spec.ts @@ -19,6 +19,7 @@ import { ReactiveFormsModule, UntypedFormArray, } from '@angular/forms'; +import { By } from '@angular/platform-browser'; import { DYNAMIC_FORM_CONTROL_MAP_FN, DynamicFormLayoutService, @@ -173,6 +174,37 @@ describe('DsDynamicFormArrayComponent row/control binding', () => { expect(boundValue(1)).withContext('Dataset moved up into position 1').toBe('Dataset'); }); + it('keeps rows and controls in sync when a keyboard reorder is cancelled', () => { + const dropList = fixture.debugElement.query(By.css('.cdk-drop-list')).nativeElement; + const rowEl = dropList.querySelectorAll('[cdkDrag]')[0] as HTMLDivElement; + + // Pick row 0 up, move it down twice, then abandon the reorder with Escape. + component.toggleKeyboardDragAndDrop(new KeyboardEvent('keydown', { key: ' ' }), rowEl, 0, 3); + component.handleArrowPress(new KeyboardEvent('keydown', { key: 'ArrowDown' }), dropList, 3, 0, 'down'); + component.handleArrowPress(new KeyboardEvent('keydown', { key: 'ArrowDown' }), dropList, 3, 1, 'down'); + fixture.detectChanges(); + + component.cancelKeyboardDragAndDrop(rowEl, 2, 3); + fixture.detectChanges(); + + expect([boundValue(0), boundValue(1), boundValue(2)]) + .withContext('cancelling must restore the original order for models AND controls') + .toEqual(['Article', 'Book', 'Dataset']); + }); + + it('keeps rows and controls in sync through a completed keyboard reorder', () => { + const dropList = fixture.debugElement.query(By.css('.cdk-drop-list')).nativeElement; + const rowEl = dropList.querySelectorAll('[cdkDrag]')[0] as HTMLDivElement; + + component.toggleKeyboardDragAndDrop(new KeyboardEvent('keydown', { key: ' ' }), rowEl, 0, 3); + component.handleArrowPress(new KeyboardEvent('keydown', { key: 'ArrowDown' }), dropList, 3, 0, 'down'); + fixture.detectChanges(); + + expect([boundValue(0), boundValue(1), boundValue(2)]) + .withContext('Article moved down one place, controls followed') + .toEqual(['Book', 'Article', 'Dataset']); + }); + it('rebinds after a row is inserted in the middle', () => { forms.insertFormArrayGroup(1, formArray(), component.model); fixture.detectChanges(); diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts index 9d86a808bda..933d1ceb905 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts @@ -252,7 +252,7 @@ export class DsDynamicFormArrayComponent extends DynamicFormArrayComponent { } cancelKeyboardDragAndDrop(sortableElement: HTMLDivElement, index: number, length: number) { - this.model.moveGroup(index, this.elementBeingSortedStartingIndex - index); + this.moveGroupAndControl(index, this.elementBeingSortedStartingIndex); if (hasValue(this.model.groups[this.elementBeingSortedStartingIndex]) && hasValue((this.control as any).controls[this.elementBeingSortedStartingIndex])) { this.onCustomEvent({ previousIndex: index, diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.html b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.html index d834b8482c2..510028c90d6 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.html +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.html @@ -59,8 +59,10 @@ + (mousedown) + (keydown.enter) mirrors the options above: carrying (click) as well fired + it twice per mouse interaction, but dropping it outright left clearing mouse-only. -->