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..e15c215da6b --- /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,217 @@ +// 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 { By } from '@angular/platform-browser'; +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('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(); + + 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..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 @@ -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, @@ -228,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 eb523d91c58..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 @@ -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..241990fd07f --- /dev/null +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.duplicate.spec.ts @@ -0,0 +1,321 @@ +// 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('clears from the keyboard as well as the mouse', () => { + // The options are reachable with Enter; dropping the clear entry's (click) handler in favour + // of (mousedown) must not leave it mouse-only. + const clear = fixture.debugElement.query(By.css('button.dropdown-item.scrollable-dropdown-clear')); + spyOn(comp, 'onSelect'); + + clear.nativeElement.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter', bubbles: true })); + + expect(comp.onSelect).toHaveBeenCalledTimes(1); + }); + + 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(); }));