From 45c5f9114c8122f23ac6f4ba0c8e0265098a2bc2 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Wed, 5 Aug 2026 11:59:57 +0200 Subject: [PATCH 01/13] Clarin9/Port per-field type binding (submit.type-bind.field "A=>B") (#876) Upstream type-bind supports exactly one global controlling field. LINDAT needs per-field control, expressed as `submit.type-bind.field = dc.type, dc.language.iso=>edm.type`, and none of that machinery was ported to the v9 branch: `FormFieldModel` had no `typeBindField` (so cerialize dropped the REST value), the parser always stamped the relation with the global type field, and `FormBuilderService` kept a single `typeField` string and a single type bind model. Selecting `edm.type = TEXT` therefore evaluated the relation against the empty `dc.type` model and the dependent language field kept its `d-none` class. - `FormFieldModel.typeBindField` is deserialized again. - `FormBuilderService` keeps a map of controlling fields (default + one entry per `A=>B` override, order-independent, duplicate-safe, trimmed) and a map of registered controlling models, plus a subject that emits every registration. `getTypeBindModel(ref?)` takes an optional ref so all existing call sites and mocks keep working. - `FieldParser.getTypeBindFieldRef()` stamps the relation with the controlling model id when `` is declared, and otherwise with the field's own metadata name, which is resolved against the map later - the property arrives over REST asynchronously. - `DsDynamicTypeBindRelationService` passes the relation id through, no longer dereferences a missing bind model, and attaches to a controlling model that is only registered by a later `modelFromConfiguration()` call. Deliberate deviations from the 7.x implementation (documented in the PR): `findById` and `row-parser` are left untouched, `typeBindField` is not carried on control models, and the inverted second clause of the 7.x `getTypeBindModel` guard is dropped - it is a no-op for the LINDAT config and would otherwise fall back to `dc_type`, reproducing this very bug. Co-Authored-By: Claude Opus 5 (1M context) --- ...dynamic-type-bind-relation.service.spec.ts | 34 ++++ .../ds-dynamic-type-bind-relation.service.ts | 96 ++++++++---- .../form/builder/form-builder.service.spec.ts | 129 +++++++++++++--- .../form/builder/form-builder.service.ts | 146 ++++++++++++++---- .../form/builder/models/form-field.model.ts | 8 + .../form/builder/parsers/field-parser.ts | 22 ++- .../parsers/onebox-field-parser.spec.ts | 21 +++ .../shared/mocks/form-builder-service.mock.ts | 2 + 8 files changed, 375 insertions(+), 83 deletions(-) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts index 883da2295ab..a38edfb6eb7 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts @@ -14,15 +14,18 @@ import { HIDDEN_MATCHER_PROVIDER, REQUIRED_MATCHER_PROVIDER, } from '@ng-dynamic-forms/core'; +import { Subject } from 'rxjs'; import { getMockFormBuilderService } from '../../../mocks/form-builder-service.mock'; import { + dcTypeInputConfig, mockInputWithTypeBindModel, MockRelationModel, } from '../../../mocks/form-models.mock'; import { FormBuilderService } from '../form-builder.service'; import { FormFieldMetadataValueObject } from '../models/form-field-metadata-value.model'; import { DsDynamicTypeBindRelationService } from './ds-dynamic-type-bind-relation.service'; +import { DsDynamicInputModel } from './models/ds-dynamic-input.model'; import { getTypeBindRelations } from './type-bind.utils'; describe('DSDynamicTypeBindRelationService test suite', () => { @@ -87,6 +90,12 @@ describe('DSDynamicTypeBindRelationService test suite', () => { const relatedModels = service.getRelatedFormModel(testModel); expect(relatedModels).toHaveSize(1); }); + it('Should ask the form builder for the model that controls this field', () => { + const testModel = mockInputWithTypeBindModel; + testModel.typeBindRelations = getTypeBindRelations(['boundType'], 'edm_type'); + service.getRelatedFormModel(testModel); + expect((service as any).formBuilderService.getTypeBindModel).toHaveBeenCalledWith('edm_type'); + }); }); describe('Test matchesCondition method', () => { @@ -129,6 +138,31 @@ describe('DSDynamicTypeBindRelationService test suite', () => { } }); + it('Expect hasMatch to be true when the controlling model is not registered (field stays hidden)', () => { + const testModel = mockInputWithTypeBindModel; + testModel.typeBindRelations = getTypeBindRelations(['boundType'], 'edm_type'); + ((service as any).formBuilderService.getTypeBindModel as jasmine.Spy).and.returnValue(undefined); + const relation = dynamicFormRelationService.findRelationByMatcher((testModel as any).typeBindRelations, HIDDEN_MATCHER); + expect(service.matchesCondition(relation, HIDDEN_MATCHER)).toBeTruthy(); + }); + + it('Should attach to the controlling model as soon as it is registered', () => { + const bindModelUpdates = new Subject(); + const formBuilderServiceSpy: any = (service as any).formBuilderService; + formBuilderServiceSpy.getTypeBindModelUpdates.and.returnValue(bindModelUpdates.asObservable()); + formBuilderServiceSpy.getTypeBindModel.and.returnValue(undefined); + + const testModel = mockInputWithTypeBindModel; + testModel.typeBindRelations = getTypeBindRelations(['boundType'], 'edm_type'); + const subscriptions = service.subscribeRelations(testModel, new UntypedFormControl()); + // only the registration listener so far + expect(subscriptions).toHaveSize(1); + + formBuilderServiceSpy.getTypeBindModel.and.returnValue(new DsDynamicInputModel(dcTypeInputConfig)); + bindModelUpdates.next('edm_type'); + expect(subscriptions).toHaveSize(2); + }); + }); }); diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts index 4f8cff747e6..39c79435423 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts @@ -69,13 +69,13 @@ export class DsDynamicTypeBindRelationService { (model as any).typeBindRelations.forEach((relGroup) => relGroup.when.forEach((rel) => { - if (model.id === rel.id) { + const bindModel: DynamicFormControlModel = this.formBuilderService.getTypeBindModel(rel?.id); + + if (hasValue(bindModel) && bindModel.id === model.id) { throw new Error(`FormControl ${model.id} cannot depend on itself`); } - const bindModel: DynamicFormControlModel = this.formBuilderService.getTypeBindModel(); - - if (model && !models.some((modelElement) => modelElement === bindModel)) { + if (hasValue(bindModel) && !models.some((modelElement) => modelElement === bindModel)) { models.push(bindModel); } })); @@ -102,7 +102,14 @@ export class DsDynamicTypeBindRelationService { // like relation group component and submission section form component). // This model (DynamicRelationGroupModel) contains eg. mandatory field, formConfiguration, relationFields, // submission scope, form/section type and other high level properties - const bindModel: any = this.formBuilderService.getTypeBindModel(); + const bindModel: any = this.formBuilderService.getTypeBindModel(condition?.id); + + // The controlling model may not be part of this form at all (e.g. `dc.language.iso=>edm.type` + // is configured deployment-wide but this collection's form has no `edm.type` field), or it may + // not be registered yet. Keep MATCH_VISIBLE fields hidden until it becomes available. + if (hasNoValue(bindModel)) { + return relation.match === matcher.opposingMatch; + } let values: string[]; let bindModelValue = bindModel.value; @@ -180,39 +187,60 @@ export class DsDynamicTypeBindRelationService { */ subscribeRelations(model: DynamicFormControlModel, control: UntypedFormControl): Subscription[] { - const relatedModels = this.getRelatedFormModel(model); const subscriptions: Subscription[] = []; + const attachedModelIds = new Set(); - Object.values(relatedModels).forEach((relatedModel: any) => { - - if (hasValue(relatedModel)) { - const initValue = (hasNoValue(relatedModel.value) || typeof relatedModel.value === 'string') ? relatedModel.value : - (Array.isArray(relatedModel.value) ? relatedModel.value : relatedModel.value.value); - - const updateSubject = (relatedModel.type === 'CHECKBOX_GROUP' ? relatedModel.valueUpdates : relatedModel.valueChanges); - const valueChanges = updateSubject.pipe( - startWith(initValue), - ); - - // Build up the subscriptions to watch for changes; - subscriptions.push(valueChanges.subscribe(() => { - // Iterate each matcher - if (hasValue(this.dynamicMatchers)) { - this.dynamicMatchers.forEach((matcher) => { - // Find the relation - const relation = this.dynamicFormRelationService.findRelationByMatcher((model as any).typeBindRelations, matcher); - // If the relation is defined, get matchesCondition result and pass it to the onChange event listener - if (relation !== undefined) { - const hasMatch = this.matchesCondition(relation, matcher); - matcher.onChange(hasMatch, model, control, this.injector); - } - }); - } - })); - } - }); + const attachRelatedModels = (relatedModels: DynamicFormControlModel[]) => { + relatedModels.forEach((relatedModel: any) => { + + if (hasValue(relatedModel) && !attachedModelIds.has(relatedModel.id)) { + attachedModelIds.add(relatedModel.id); + + const initValue = (hasNoValue(relatedModel.value) || typeof relatedModel.value === 'string') ? relatedModel.value : + (Array.isArray(relatedModel.value) ? relatedModel.value : relatedModel.value.value); + + const updateSubject = (relatedModel.type === 'CHECKBOX_GROUP' ? relatedModel.valueUpdates : relatedModel.valueChanges); + const valueChanges = updateSubject.pipe( + startWith(initValue), + ); + + // Build up the subscriptions to watch for changes; + subscriptions.push(valueChanges.subscribe(() => this.evaluateRelations(model, control))); + } + }); + }; + + attachRelatedModels(this.getRelatedFormModel(model)); + + if (attachedModelIds.size === 0) { + // The controlling model (e.g. `edm_type` for `dc.language.iso=>edm.type`) may only be registered + // by a later modelFromConfiguration() call. Evaluate once so the MATCH_VISIBLE fallback applies, + // then attach as soon as a type bind model shows up. + this.evaluateRelations(model, control); + subscriptions.push(this.formBuilderService.getTypeBindModelUpdates().subscribe(() => { + attachRelatedModels(this.getRelatedFormModel(model)); + })); + } return subscriptions; } + /** + * Re-evaluate every type bind relation of the given model and notify the matchers of the outcome + */ + private evaluateRelations(model: DynamicFormControlModel, control: UntypedFormControl): void { + if (hasValue(this.dynamicMatchers)) { + // Iterate each matcher + this.dynamicMatchers.forEach((matcher) => { + // Find the relation + const relation = this.dynamicFormRelationService.findRelationByMatcher((model as any).typeBindRelations, matcher); + // If the relation is defined, get matchesCondition result and pass it to the onChange event listener + if (relation !== undefined) { + const hasMatch = this.matchesCondition(relation, matcher); + matcher.onChange(hasMatch, model, control, this.injector); + } + }); + } + } + } diff --git a/src/app/shared/form/builder/form-builder.service.spec.ts b/src/app/shared/form/builder/form-builder.service.spec.ts index 1e481391915..8e13b18358e 100644 --- a/src/app/shared/form/builder/form-builder.service.spec.ts +++ b/src/app/shared/form/builder/form-builder.service.spec.ts @@ -58,32 +58,32 @@ import { FormBuilderService } from './form-builder.service'; import { FormFieldModel } from './models/form-field.model'; import { FormFieldMetadataValueObject } from './models/form-field-metadata-value.model'; +const typeFieldProp = 'submit.type-bind.field'; +const typeFieldTestValue = 'dc.type'; +const submissionId = '1234'; + +function testValidator() { + return { testValidator: { valid: true } }; +} + +function testAsyncValidator() { + return new Promise((resolve) => setTimeout(() => resolve(true), 0)); +} + +const createConfigSuccessSpy = (...values: string[]) => jasmine.createSpyObj('configurationDataService', { + findByPropertyName: createSuccessfulRemoteDataObject$({ + ... new ConfigurationProperty(), + name: typeFieldProp, + values: values, + }), +}); + describe('FormBuilderService test suite', () => { let testModel: DynamicFormControlModel[]; let testFormConfiguration: SubmissionFormsModel; let service: FormBuilderService; let configSpy: ConfigurationDataService; - const typeFieldProp = 'submit.type-bind.field'; - const typeFieldTestValue = 'dc.type'; - - const submissionId = '1234'; - - function testValidator() { - return { testValidator: { valid: true } }; - } - - function testAsyncValidator() { - return new Promise((resolve) => setTimeout(() => resolve(true), 0)); - } - - const createConfigSuccessSpy = (...values: string[]) => jasmine.createSpyObj('configurationDataService', { - findByPropertyName: createSuccessfulRemoteDataObject$({ - ... new ConfigurationProperty(), - name: typeFieldProp, - values: values, - }), - }); beforeEach(() => { configSpy = createConfigSuccessSpy(typeFieldTestValue); @@ -927,3 +927,92 @@ describe('FormBuilderService test suite', () => { }); }); + +describe('FormBuilderService per-field type bind test suite', () => { + + let service: FormBuilderService; + let configSpy: ConfigurationDataService; + + const dcTypeModel = new DsDynamicInputModel({ + id: 'dc_type', name: 'dc.type', repeatable: false, metadataFields: [], + submissionId, hasSelectableMetadata: false, + }); + const edmTypeModel = new DsDynamicInputModel({ + id: 'edm_type', name: 'edm.type', repeatable: false, metadataFields: [], + submissionId, hasSelectableMetadata: false, + }); + + const typeBindFormConfiguration = { + name: 'typeBindFormConfiguration', + rows: [ + { fields: [{ + input: { type: 'onebox' }, label: 'Type', mandatory: 'false', repeatable: false, + hints: '', languageCodes: [], selectableMetadata: [{ metadata: 'edm.type' }], + } as FormFieldModel] } as FormRowModel, + { fields: [{ + input: { type: 'onebox' }, label: 'Language', mandatory: 'false', repeatable: false, + hints: '', languageCodes: [], typeBind: ['TEXT'], typeBindField: 'edm.type', + selectableMetadata: [{ metadata: 'dc.language.iso' }], + } as FormFieldModel] } as FormRowModel, + ], + type: 'submissionform', + _links: { self: { href: 'typeBindFormConfiguration.url' } }, + } as any; + + beforeEach(() => { + // dspace.cfg declares the property twice, so the REST payload repeats the default value + configSpy = createConfigSuccessSpy('dc.type', 'dc.type', 'dc.language.iso=>edm.type'); + TestBed.configureTestingModule({ + imports: [ReactiveFormsModule], + providers: [ + { provide: FormBuilderService, useClass: FormBuilderService }, + { provide: DynamicFormValidationService, useValue: {} }, + { provide: NG_VALIDATORS, useValue: testValidator, multi: true }, + { provide: NG_ASYNC_VALIDATORS, useValue: testAsyncValidator, multi: true }, + { provide: ConfigurationDataService, useValue: configSpy }, + { provide: TranslateService, useValue: getMockTranslateService() }, + ], + }); + service = TestBed.inject(FormBuilderService); + }); + + it('should keep "dc_type" as the default type bind field even when the property is duplicated', () => { + expect(service.getTypeField()).toEqual('dc_type'); + }); + + it('should resolve the controlling model per field', () => { + service.setTypeBindModel(dcTypeModel); + service.setTypeBindModel(edmTypeModel); + + expect(service.getTypeBindModel('dc.language.iso')).toBe(edmTypeModel); + expect(service.getTypeBindModel('edm_type')).toBe(edmTypeModel); + expect(service.getTypeBindModel('dc.contributor.author')).toBe(dcTypeModel); + expect(service.getTypeBindModel()).toBe(dcTypeModel); + }); + + it('should fall back to the default model when the controlling model is not part of the form', () => { + service.setTypeBindModel(dcTypeModel); + expect(service.getTypeBindModel('dc.language.iso')).toBe(dcTypeModel); + }); + + it('should emit every registered type bind model id', (done) => { + const emitted: string[] = []; + service.getTypeBindModelUpdates().subscribe((id: string) => { + emitted.push(id); + if (emitted.length === 2) { + expect(emitted).toEqual(['dc_type', 'edm_type']); + done(); + } + }); + service.setTypeBindModel(dcTypeModel); + service.setTypeBindModel(edmTypeModel); + }); + + it('should bind dc.language.iso to edm.type when parsing the form configuration', () => { + const formModel = service.modelFromConfiguration(submissionId, typeBindFormConfiguration, 'testScopeUUID'); + const languageModel = service.findById('dc_language_iso', formModel) as DsDynamicInputModel; + + expect(languageModel.typeBindRelations[0].when[0].id).toEqual('edm_type'); + expect(service.getTypeBindModel('dc.language.iso').id).toEqual('edm_type'); + }); +}); diff --git a/src/app/shared/form/builder/form-builder.service.ts b/src/app/shared/form/builder/form-builder.service.ts index 66c500f23dc..6bfdc1c8b2a 100644 --- a/src/app/shared/form/builder/form-builder.service.ts +++ b/src/app/shared/form/builder/form-builder.service.ts @@ -27,7 +27,12 @@ import { import isObject from 'lodash/isObject'; import isString from 'lodash/isString'; import mergeWith from 'lodash/mergeWith'; +import { + Observable, + Subject, +} from 'rxjs'; +import { FormRowModel } from '../../../core/config/models/config-submission-form.model'; import { SubmissionFormsModel } from '../../../core/config/models/config-submission-forms.model'; import { ConfigurationDataService } from '../../../core/data/configuration-data.service'; import { VIRTUAL_METADATA_PREFIX } from '../../../core/shared/metadata.models'; @@ -55,13 +60,34 @@ import { DynamicQualdropModel } from './ds-dynamic-form-ui/models/ds-dynamic-qua import { DynamicRowArrayModel } from './ds-dynamic-form-ui/models/ds-dynamic-row-array-model'; import { DynamicRelationGroupModel } from './ds-dynamic-form-ui/models/relation-group/dynamic-relation-group.model'; import { DYNAMIC_FORM_CONTROL_TYPE_TAG } from './ds-dynamic-form-ui/models/tag/dynamic-tag.model'; +import { FormFieldModel } from './models/form-field.model'; import { FormFieldMetadataValueObject } from './models/form-field-metadata-value.model'; import { RowParser } from './parsers/row-parser'; +/** + * The key under which the default type bind field is stored in the type field map, e.g. + * {'default' -> 'dc_type'} + */ +export const TYPE_BIND_DEFAULT_KEY = 'default'; + +/** + * Separator used by the `submit.type-bind.field` property to bind one metadata field to a + * controlling field other than the default one, e.g. `dc.language.iso=>edm.type` + */ +const TYPE_BIND_FIELD_SEPARATOR = '=>'; + @Injectable({ providedIn: 'root' }) export class FormBuilderService extends DynamicFormService { - private typeBindModel: DynamicFormControlModel; + /** + * This map contains the models that control type binding, keyed by model id (`dc_type`, `edm_type`) + */ + private typeBindModel: Map; + + /** + * Emits the id of a type bind model whenever one is registered + */ + private typeBindModelUpdates: Subject; /** * This map contains the active forms model @@ -74,9 +100,11 @@ export class FormBuilderService extends DynamicFormService { private formGroups: Map; /** - * This is the field to use for type binding + * The fields to use for type binding: TYPE_BIND_DEFAULT_KEY -> the default controlling model id, + * plus one entry per metadata field that is controlled by another field, e.g. + * `dc.language.iso` -> `edm_type` */ - private typeField: string; + private typeFields: Map; constructor( componentService: DynamicFormComponentService, @@ -87,12 +115,15 @@ export class FormBuilderService extends DynamicFormService { super(componentService, validationService); this.formModels = new Map(); this.formGroups = new Map(); + this.typeFields = new Map(); + this.typeBindModel = new Map(); + this.typeBindModelUpdates = new Subject(); + + this.typeFields.set(TYPE_BIND_DEFAULT_KEY, 'dc_type'); // If optional config service was passed, perform an initial set of type field (default dc_type) for type binds if (hasValue(this.configService)) { this.setTypeBindFieldFromConfig(); } - - } createDynamicFormControlEvent(control: UntypedFormControl, group: UntypedFormGroup, model: DynamicFormControlModel, type: string): DynamicFormControlEvent { @@ -104,12 +135,32 @@ export class FormBuilderService extends DynamicFormService { return { $event, context, control: control, group: group, model: model, type }; } - getTypeBindModel() { - return this.typeBindModel; + /** + * Get the model of the field that controls the type binding of a bound field. + * + * @param typeBindFieldRef either the metadata name of the bound field itself - resolved through the + * `submit.type-bind.field` map, e.g. `dc.language.iso` -> `edm_type` - or, when the submission form + * declares ``, the id of the controlling model itself. When it resolves + * to a model that is not part of the current form, the default (usually `dc_type`) model is returned. + */ + getTypeBindModel(typeBindFieldRef?: string): DynamicFormControlModel { + const defaultModelId = this.typeFields.get(TYPE_BIND_DEFAULT_KEY); + const modelId = this.typeFields.get(typeBindFieldRef) ?? typeBindFieldRef ?? defaultModelId; + return this.typeBindModel.get(modelId) ?? this.typeBindModel.get(defaultModelId); } setTypeBindModel(model: DynamicFormControlModel) { - this.typeBindModel = model; + this.typeBindModel.set(model.id, model); + this.typeBindModelUpdates.next(model.id); + } + + /** + * Emits the id of every type bind model as soon as it is registered, so that fields whose + * controlling model is only parsed later - e.g. because it lives in another form section - can + * still attach to it. + */ + getTypeBindModelUpdates(): Observable { + return this.typeBindModelUpdates.asObservable(); } findById(id: string, groupModel: DynamicFormControlModel[], arrayIndex = null): DynamicFormControlModel | null { @@ -311,16 +362,40 @@ export class FormBuilderService extends DynamicFormService { }); } - if (hasNoValue(typeBindModel)) { - typeBindModel = this.findById(this.typeField, rows); - } - if (hasValue(typeBindModel)) { this.setTypeBindModel(typeBindModel); + } else { + this.getTypeBindModelIds(rawData).forEach((typeBindModelId: string) => { + const foundModel = this.findById(typeBindModelId, rows); + if (hasValue(foundModel)) { + this.setTypeBindModel(foundModel); + } + }); } return rows; } + /** + * Collect the ids of every model that can control type binding for the given form configuration: + * all values of the `submit.type-bind.field` map (the default field plus each `A=>B` override) and + * every `` declared by a field of this configuration. The latter is read + * straight from the REST payload, so a controlling model is registered even when the configuration + * property has not been fetched yet. + */ + private getTypeBindModelIds(rawData: any): string[] { + const ids = new Set(this.typeFields.values()); + const collectFromRows = (formRows: FormRowModel[]): void => { + (formRows || []).forEach((formRow: FormRowModel) => (formRow?.fields || []).forEach((field: FormFieldModel) => { + if (isNotEmpty(field?.typeBindField)) { + ids.add(field.typeBindField.replace(/\./g, '_')); + } + collectFromRows(field?.rows); + })); + }; + collectFromRows(rawData?.rows); + return Array.from(ids); + } + isModelInCustomGroup(model: DynamicFormControlModel): boolean { return this.isCustomGroup((model as any).parent); } @@ -512,7 +587,13 @@ export class FormBuilderService extends DynamicFormService { } /** - * Get the type bind field from config + * Get the type bind field(s) from config. + * + * `submit.type-bind.field` holds the default controlling field and, optionally, one + * `=>` entry per field that is controlled by another field, e.g. + * `submit.type-bind.field = dc.type, dc.language.iso=>edm.type`. The property may legitimately be + * declared more than once (dspace.cfg + local.cfg), so duplicated values must be tolerated and the + * order of the values must not matter. */ setTypeBindFieldFromConfig(): void { this.configService.findByPropertyName('submit.type-bind.field').pipe( @@ -520,30 +601,41 @@ export class FormBuilderService extends DynamicFormService { ).subscribe((remoteData: any) => { // make sure we got a success response from the backend if (!remoteData.hasSucceeded) { - this.typeField = 'dc_type'; + this.typeFields.set(TYPE_BIND_DEFAULT_KEY, 'dc_type'); return; } - // Read type bind value from response and set if non-empty - const typeFieldConfig = remoteData.payload.values[0]; - if (isEmpty(typeFieldConfig)) { - this.typeField = 'dc_type'; - } else { - this.typeField = typeFieldConfig.replace(/\./g, '_'); + const typeFieldConfigValues: string[] = remoteData.payload.values || []; + typeFieldConfigValues.forEach((typeFieldConfig: string) => { + if (typeFieldConfig.includes(TYPE_BIND_FIELD_SEPARATOR)) { + // `dc.language.iso=>edm.type`: this metadata field is controlled by another field + const [boundField, controllingField] = typeFieldConfig.split(TYPE_BIND_FIELD_SEPARATOR); + if (isNotEmpty(boundField?.trim()) && isNotEmpty(controllingField?.trim())) { + this.typeFields.set(boundField.trim(), controllingField.trim().replace(/\./g, '_')); + } + } else if (isNotEmpty(typeFieldConfig?.trim())) { + // `dc.type`: the default controlling field. A duplicated value just overwrites itself. + this.typeFields.set(TYPE_BIND_DEFAULT_KEY, typeFieldConfig.trim().replace(/\./g, '_')); + } + }); + if (hasNoValue(this.typeFields.get(TYPE_BIND_DEFAULT_KEY))) { + this.typeFields.set(TYPE_BIND_DEFAULT_KEY, 'dc_type'); } }); } /** - * Get type field. If the type isn't already set, and a ConfigurationDataService is provided, set (with subscribe) - * from back end. Otherwise, get/set a default "dc_type" value + * Get the default type field. If it isn't already set, and a ConfigurationDataService is provided, + * set (with subscribe) from back end. Otherwise, get/set a default "dc_type" value */ getTypeField(): string { - if (hasValue(this.configService) && hasNoValue(this.typeField)) { - this.setTypeBindFieldFromConfig(); - } else if (hasNoValue(this.typeField)) { - this.typeField = 'dc_type'; + if (hasNoValue(this.typeFields.get(TYPE_BIND_DEFAULT_KEY))) { + if (hasValue(this.configService)) { + this.setTypeBindFieldFromConfig(); + } else { + this.typeFields.set(TYPE_BIND_DEFAULT_KEY, 'dc_type'); + } } - return this.typeField; + return this.typeFields.get(TYPE_BIND_DEFAULT_KEY); } } diff --git a/src/app/shared/form/builder/models/form-field.model.ts b/src/app/shared/form/builder/models/form-field.model.ts index a168124bec8..a8a07391865 100644 --- a/src/app/shared/form/builder/models/form-field.model.ts +++ b/src/app/shared/form/builder/models/form-field.model.ts @@ -132,6 +132,14 @@ export class FormFieldModel { @autoserialize typeBind: string[]; + /** + * The metadata field that controls the type binding of this field, coming from the + * `` attribute in submission-forms.xml. When empty, this field is + * controlled by the default field configured in the `submit.type-bind.field` property. + */ + @autoserialize + typeBindField: string; + /** * Containing the value for this metadata field */ diff --git a/src/app/shared/form/builder/parsers/field-parser.ts b/src/app/shared/form/builder/parsers/field-parser.ts index df0f0389bc5..a76e49c6b5c 100644 --- a/src/app/shared/form/builder/parsers/field-parser.ts +++ b/src/app/shared/form/builder/parsers/field-parser.ts @@ -95,7 +95,7 @@ export abstract class FieldParser { hasSelectableMetadata: isNotEmpty(this.configData.selectableMetadata), isDraggable, typeBindRelations: isNotEmpty(this.configData.typeBind) ? getTypeBindRelations(this.configData.typeBind, - this.parserOptions.typeField) : null, + this.getTypeBindFieldRef()) : null, groupFactory: () => { let model; if ((arrayCounter === 0)) { @@ -290,6 +290,24 @@ export abstract class FieldParser { } } + /** + * Resolve the field that controls the type binding of this field. + * + * When submission-forms.xml declares `` the controlling model is known + * up front, so its model id is returned directly. Otherwise this field's own metadata name is + * returned and {@link FormBuilderService#getTypeBindModel} resolves it against the + * `submit.type-bind.field` property (e.g. `dc.type, dc.language.iso=>edm.type`) at + * relation-evaluation time - that property is fetched asynchronously and may not have arrived yet + * while the form is being parsed. + */ + protected getTypeBindFieldRef(): string { + if (isNotEmpty(this.configData.typeBindField)) { + // input ids don't allow dots, so replace them - this is already a model id + return this.configData.typeBindField.replace(/\./g, '_'); + } + return this.getFieldId() || this.parserOptions.typeField; + } + protected initModel(id?: string, label = true, labelEmpty = false, setErrors = true, hint = true) { const controlModel = Object.create(null); @@ -338,7 +356,7 @@ export abstract class FieldParser { // If typeBind is configured if (isNotEmpty(this.configData.typeBind)) { (controlModel as DsDynamicInputModel).typeBindRelations = getTypeBindRelations(this.configData.typeBind, - this.parserOptions.typeField); + this.getTypeBindFieldRef()); } return controlModel; diff --git a/src/app/shared/form/builder/parsers/onebox-field-parser.spec.ts b/src/app/shared/form/builder/parsers/onebox-field-parser.spec.ts index b2a50395e1a..d008d72f586 100644 --- a/src/app/shared/form/builder/parsers/onebox-field-parser.spec.ts +++ b/src/app/shared/form/builder/parsers/onebox-field-parser.spec.ts @@ -152,4 +152,25 @@ describe('OneboxFieldParser test suite', () => { }); }); + describe('type binding', () => { + it('should use the field named in as the type bind relation id', () => { + field1.typeBind = ['TEXT']; + field1.typeBindField = 'edm.type'; + const parser = new OneboxFieldParser(submissionId, field1, initFormValues, parserOptions, translateService); + + const fieldModel = parser.parse() as DsDynamicInputModel; + + expect(fieldModel.typeBindRelations[0].when[0].id).toBe('edm_type'); + }); + + it('should fall back to the field\'s own metadata name when no is configured', () => { + field1.typeBind = ['TEXT']; + const parser = new OneboxFieldParser(submissionId, field1, initFormValues, parserOptions, translateService); + + const fieldModel = parser.parse() as DsDynamicInputModel; + + expect(fieldModel.typeBindRelations[0].when[0].id).toBe('title'); + }); + }); + }); diff --git a/src/app/shared/mocks/form-builder-service.mock.ts b/src/app/shared/mocks/form-builder-service.mock.ts index 9a0f358da66..fcb30dab2b2 100644 --- a/src/app/shared/mocks/form-builder-service.mock.ts +++ b/src/app/shared/mocks/form-builder-service.mock.ts @@ -2,6 +2,7 @@ import { UntypedFormControl, UntypedFormGroup, } from '@angular/forms'; +import { EMPTY } from 'rxjs'; import { DsDynamicInputModel } from '../form/builder/ds-dynamic-form-ui/models/ds-dynamic-input.model'; import { FormBuilderService } from '../form/builder/form-builder.service'; @@ -45,5 +46,6 @@ export function getMockFormBuilderService(): FormBuilderService { ], }, ), + getTypeBindModelUpdates: EMPTY, }); } From 4e065fc1a4eb64cf4460bc3479d799d584c1435b Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Wed, 5 Aug 2026 12:49:48 +0200 Subject: [PATCH 02/13] Review feedback: fix the subscription hand-off, scope the registry, harden parsing Behaviour: - subscribeRelations now returns a single owning Subscription. The caller spreads the returned array into its own, so a child created after that snapshot - which is exactly what the late-registration listener does - could never be torn down and kept mutating hidden/disabled on a destroyed model. - Always listen for type bind model registrations, not only when nothing was attached: until the real controlling model exists the field is temporarily attached to the default one, and that case was never re-attached (Copilot). - The type bind registry is now dropped when the submission changes. Sections of one submission still share it (a controlling field may live in another section), but a model from the previously opened collection's form can no longer answer lookups and defeat the fall-back-to-default behaviour. - Controlling models are registered again once submit.type-bind.field arrives, so an override that exists only in the property - with no in the XML - still resolves if the config lands after the form was parsed. - A self-referencing type bind is now a console.warn + skip instead of a throw: it can be raised from the registration callback, and one misconfigured field should not take down the whole submission section. - Tolerate blank/whitespace/malformed values in the property (a null entry used to throw inside the subscribe) and trim typeBindField before building the model id (Copilot). getTypeBindModel is typed `| undefined` (Copilot). Tests: late registration now asserts the relation is really re-evaluated and stops on unsubscribe; added the attached-to-default case, cross-submission isolation, a delayed configuration response, malformed values, and a cerialize round-trip proving typeBindField survives deserialization. Co-Authored-By: Claude Opus 5 (1M context) --- ...dynamic-type-bind-relation.service.spec.ts | 49 ++++++++- .../ds-dynamic-type-bind-relation.service.ts | 47 ++++++--- .../form/builder/form-builder.service.spec.ts | 99 ++++++++++++++++++- .../form/builder/form-builder.service.ts | 85 +++++++++++++--- .../builder/models/form-field.model.spec.ts | 34 +++++++ .../form/builder/parsers/field-parser.ts | 5 +- 6 files changed, 281 insertions(+), 38 deletions(-) create mode 100644 src/app/shared/form/builder/models/form-field.model.spec.ts diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts index a38edfb6eb7..2392517aa0c 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts @@ -146,7 +146,7 @@ describe('DSDynamicTypeBindRelationService test suite', () => { expect(service.matchesCondition(relation, HIDDEN_MATCHER)).toBeTruthy(); }); - it('Should attach to the controlling model as soon as it is registered', () => { + it('Should attach to the controlling model as soon as it is registered, and stop when the caller unsubscribes', () => { const bindModelUpdates = new Subject(); const formBuilderServiceSpy: any = (service as any).formBuilderService; formBuilderServiceSpy.getTypeBindModelUpdates.and.returnValue(bindModelUpdates.asObservable()); @@ -154,13 +154,52 @@ describe('DSDynamicTypeBindRelationService test suite', () => { const testModel = mockInputWithTypeBindModel; testModel.typeBindRelations = getTypeBindRelations(['boundType'], 'edm_type'); - const subscriptions = service.subscribeRelations(testModel, new UntypedFormControl()); - // only the registration listener so far - expect(subscriptions).toHaveSize(1); + const dcTypeControl = new UntypedFormControl(); + // the caller (ds-dynamic-form-control-container) spreads the result into its own array, so a + // subscription created later has to hang off something handed over now to ever be torn down + const [subscription] = service.subscribeRelations(testModel, dcTypeControl); + + const controllingModel = new DsDynamicInputModel(dcTypeInputConfig); + formBuilderServiceSpy.getTypeBindModel.and.returnValue(controllingModel); + bindModelUpdates.next('edm_type'); + + controllingModel.value = 'anotherType'; + expect(testModel.hidden).toBeTrue(); + controllingModel.value = 'boundType'; + expect(testModel.hidden).toBeFalse(); + subscription.unsubscribe(); + + controllingModel.value = 'anotherType'; + expect(testModel.hidden).toBeFalse(); + }); + + it('Should attach the real controlling model even when it was first bound to the default one', () => { + // until edm_type is registered, getTypeBindModel falls back to the default dc_type model, so + // a related model IS attached - the late registration must still be picked up + const bindModelUpdates = new Subject(); + const formBuilderServiceSpy: any = (service as any).formBuilderService; + formBuilderServiceSpy.getTypeBindModelUpdates.and.returnValue(bindModelUpdates.asObservable()); formBuilderServiceSpy.getTypeBindModel.and.returnValue(new DsDynamicInputModel(dcTypeInputConfig)); + + const testModel = mockInputWithTypeBindModel; + testModel.typeBindRelations = getTypeBindRelations(['boundType'], 'edm_type'); + const [subscription] = service.subscribeRelations(testModel, new UntypedFormControl()); + + const controllingModel = new DsDynamicInputModel({ + ...dcTypeInputConfig, + id: 'edm_type', + name: 'edm.type', + }); + formBuilderServiceSpy.getTypeBindModel.and.returnValue(controllingModel); bindModelUpdates.next('edm_type'); - expect(subscriptions).toHaveSize(2); + + controllingModel.value = 'anotherType'; + expect(testModel.hidden).toBeTrue(); + controllingModel.value = 'boundType'; + expect(testModel.hidden).toBeFalse(); + + subscription.unsubscribe(); }); }); diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts index 39c79435423..024a1ca9c57 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts @@ -69,13 +69,21 @@ export class DsDynamicTypeBindRelationService { (model as any).typeBindRelations.forEach((relGroup) => relGroup.when.forEach((rel) => { - const bindModel: DynamicFormControlModel = this.formBuilderService.getTypeBindModel(rel?.id); + const bindModel: DynamicFormControlModel | undefined = this.formBuilderService.getTypeBindModel(rel?.id); - if (hasValue(bindModel) && bindModel.id === model.id) { - throw new Error(`FormControl ${model.id} cannot depend on itself`); + if (hasNoValue(bindModel)) { + return; + } + + if (bindModel.id === model.id) { + // A misconfigured pointing at the field itself. Skip the relation + // instead of throwing: this runs during form init and from the type bind model registration + // callback, so throwing would take down the whole submission section over one bad field. + console.warn(`FormControl ${model.id} cannot depend on itself, ignoring its type bind relation`); + return; } - if (hasValue(bindModel) && !models.some((modelElement) => modelElement === bindModel)) { + if (!models.some((modelElement) => modelElement === bindModel)) { models.push(bindModel); } })); @@ -181,13 +189,19 @@ export class DsDynamicTypeBindRelationService { } /** - * Return an array of subscriptions to a calling component + * Return an array of subscriptions to a calling component. + * + * A single owning {@link Subscription} is returned rather than the individual child + * subscriptions: the controlling model may only be registered after this method has returned (see + * below), and callers snapshot the returned array, so any later child has to hang off something + * they already hold in order to be torn down with the component. + * * @param model * @param control */ subscribeRelations(model: DynamicFormControlModel, control: UntypedFormControl): Subscription[] { - const subscriptions: Subscription[] = []; + const subscriptions = new Subscription(); const attachedModelIds = new Set(); const attachRelatedModels = (relatedModels: DynamicFormControlModel[]) => { @@ -205,7 +219,7 @@ export class DsDynamicTypeBindRelationService { ); // Build up the subscriptions to watch for changes; - subscriptions.push(valueChanges.subscribe(() => this.evaluateRelations(model, control))); + subscriptions.add(valueChanges.subscribe(() => this.evaluateRelations(model, control))); } }); }; @@ -213,16 +227,21 @@ export class DsDynamicTypeBindRelationService { attachRelatedModels(this.getRelatedFormModel(model)); if (attachedModelIds.size === 0) { - // The controlling model (e.g. `edm_type` for `dc.language.iso=>edm.type`) may only be registered - // by a later modelFromConfiguration() call. Evaluate once so the MATCH_VISIBLE fallback applies, - // then attach as soon as a type bind model shows up. + // Nothing to listen to yet: evaluate once so the "controlling model missing" fallback applies + // and the field does not stay in whatever state it was rendered in. this.evaluateRelations(model, control); - subscriptions.push(this.formBuilderService.getTypeBindModelUpdates().subscribe(() => { - attachRelatedModels(this.getRelatedFormModel(model)); - })); } - return subscriptions; + // The controlling model (e.g. `edm_type` for `dc.language.iso=>edm.type`) may only be registered + // by a later modelFromConfiguration() call - a form section is parsed at a time, and the + // `submit.type-bind.field` property itself arrives asynchronously. Until then this field either + // has no controlling model at all or is temporarily attached to the default one, so keep + // listening and attach the real one as soon as it shows up. + subscriptions.add(this.formBuilderService.getTypeBindModelUpdates().subscribe(() => { + attachRelatedModels(this.getRelatedFormModel(model)); + })); + + return [subscriptions]; } /** diff --git a/src/app/shared/form/builder/form-builder.service.spec.ts b/src/app/shared/form/builder/form-builder.service.spec.ts index 8e13b18358e..d2d8ae41b2f 100644 --- a/src/app/shared/form/builder/form-builder.service.spec.ts +++ b/src/app/shared/form/builder/form-builder.service.spec.ts @@ -32,6 +32,7 @@ import { DynamicTimePickerModel, } from '@ng-dynamic-forms/core'; import { TranslateService } from '@ngx-translate/core'; +import { Subject } from 'rxjs'; import { FormRowModel } from '../../../core/config/models/config-submission-form.model'; import { SubmissionFormsModel } from '../../../core/config/models/config-submission-forms.model'; @@ -39,7 +40,10 @@ import { ConfigurationDataService } from '../../../core/data/configuration-data. import { ConfigurationProperty } from '../../../core/shared/configuration-property.model'; import { VocabularyOptions } from '../../../core/submission/vocabularies/models/vocabulary-options.model'; import { getMockTranslateService } from '../../mocks/translate.service.mock'; -import { createSuccessfulRemoteDataObject$ } from '../../remote-data.utils'; +import { + createSuccessfulRemoteDataObject, + createSuccessfulRemoteDataObject$, +} from '../../remote-data.utils'; import { DynamicDsDatePickerModel } from './ds-dynamic-form-ui/models/date-picker/date-picker.model'; import { DynamicConcatModel } from './ds-dynamic-form-ui/models/ds-dynamic-concat.model'; import { DsDynamicInputModel } from './ds-dynamic-form-ui/models/ds-dynamic-input.model'; @@ -1015,4 +1019,97 @@ describe('FormBuilderService per-field type bind test suite', () => { expect(languageModel.typeBindRelations[0].when[0].id).toEqual('edm_type'); expect(service.getTypeBindModel('dc.language.iso').id).toEqual('edm_type'); }); + + it('should not let a previous submission\'s controlling model answer lookups', () => { + service.modelFromConfiguration(submissionId, typeBindFormConfiguration, 'testScopeUUID'); + expect(service.getTypeBindModel('dc.language.iso').id).toEqual('edm_type'); + + // another collection, whose form has no edm.type field at all + service.modelFromConfiguration('other-submission', { + name: 'plainFormConfiguration', + rows: [ + { fields: [{ + input: { type: 'onebox' }, label: 'Type', mandatory: 'false', repeatable: false, + hints: '', languageCodes: [], selectableMetadata: [{ metadata: 'dc.type' }], + } as FormFieldModel] } as FormRowModel, + ], + type: 'submissionform', + _links: { self: { href: 'plainFormConfiguration.url' } }, + } as any, 'testScopeUUID'); + + expect(service.getTypeBindModel('dc.language.iso').id).toEqual('dc_type'); + }); +}); + +describe('FormBuilderService per-field type bind with a slow configuration response', () => { + + let service: FormBuilderService; + let configResponse: Subject; + + const overrideOnlyFormConfiguration = { + name: 'overrideOnlyFormConfiguration', + rows: [ + { fields: [{ + input: { type: 'onebox' }, label: 'Type', mandatory: 'false', repeatable: false, + hints: '', languageCodes: [], selectableMetadata: [{ metadata: 'edm.type' }], + } as FormFieldModel] } as FormRowModel, + { fields: [{ + // no : the binding exists only in the submit.type-bind.field property + input: { type: 'onebox' }, label: 'Language', mandatory: 'false', repeatable: false, + hints: '', languageCodes: [], typeBind: ['TEXT'], + selectableMetadata: [{ metadata: 'dc.language.iso' }], + } as FormFieldModel] } as FormRowModel, + ], + type: 'submissionform', + _links: { self: { href: 'overrideOnlyFormConfiguration.url' } }, + } as any; + + beforeEach(() => { + configResponse = new Subject(); + TestBed.configureTestingModule({ + imports: [ReactiveFormsModule], + providers: [ + { provide: FormBuilderService, useClass: FormBuilderService }, + { provide: DynamicFormValidationService, useValue: {} }, + { provide: NG_VALIDATORS, useValue: testValidator, multi: true }, + { provide: NG_ASYNC_VALIDATORS, useValue: testAsyncValidator, multi: true }, + { + provide: ConfigurationDataService, + useValue: jasmine.createSpyObj('configurationDataService', { + findByPropertyName: configResponse.asObservable(), + }), + }, + { provide: TranslateService, useValue: getMockTranslateService() }, + ], + }); + service = TestBed.inject(FormBuilderService); + }); + + it('should register the controlling model once the property arrives after the form was parsed', () => { + service.modelFromConfiguration(submissionId, overrideOnlyFormConfiguration, 'testScopeUUID'); + // the property has not arrived yet, so edm.type is not known to be a controlling field + expect(service.getTypeBindModel('dc.language.iso')).toBeUndefined(); + + configResponse.next(createSuccessfulRemoteDataObject({ + ... new ConfigurationProperty(), + name: 'submit.type-bind.field', + values: ['dc.language.iso=>edm.type', ' dc.type '], + })); + configResponse.complete(); + + expect(service.getTypeField()).toEqual('dc_type'); + expect(service.getTypeBindModel('dc.language.iso').id).toEqual('edm_type'); + }); + + it('should ignore blank and malformed values', () => { + configResponse.next(createSuccessfulRemoteDataObject({ + ... new ConfigurationProperty(), + name: 'submit.type-bind.field', + values: ['', ' ', '=>', 'a=>b=>c', undefined, 'dc.type'], + })); + configResponse.complete(); + + expect(service.getTypeField()).toEqual('dc_type'); + expect((service as any).typeFields.has('a')).toBeFalse(); + }); }); diff --git a/src/app/shared/form/builder/form-builder.service.ts b/src/app/shared/form/builder/form-builder.service.ts index 6bfdc1c8b2a..cffa4a50570 100644 --- a/src/app/shared/form/builder/form-builder.service.ts +++ b/src/app/shared/form/builder/form-builder.service.ts @@ -99,6 +99,20 @@ export class FormBuilderService extends DynamicFormService { */ private formGroups: Map; + /** + * The submission the registered type bind models belong to. A submission is parsed section by + * section and a controlling field may live in another section than the field it controls, so the + * registry survives across `modelFromConfiguration` calls - but only within one submission, + * otherwise a model of the previously opened collection's form would keep answering lookups. + */ + private typeBindModelSubmissionId: string; + + /** + * The parsed rows of the current submission, kept so that controlling models can still be + * registered when the `submit.type-bind.field` property arrives after the form was parsed. + */ + private typeBindParsedRows: DynamicFormControlModel[][]; + /** * The fields to use for type binding: TYPE_BIND_DEFAULT_KEY -> the default controlling model id, * plus one entry per metadata field that is controlled by another field, e.g. @@ -117,6 +131,7 @@ export class FormBuilderService extends DynamicFormService { this.formGroups = new Map(); this.typeFields = new Map(); this.typeBindModel = new Map(); + this.typeBindParsedRows = []; this.typeBindModelUpdates = new Subject(); this.typeFields.set(TYPE_BIND_DEFAULT_KEY, 'dc_type'); @@ -143,13 +158,16 @@ export class FormBuilderService extends DynamicFormService { * declares ``, the id of the controlling model itself. When it resolves * to a model that is not part of the current form, the default (usually `dc_type`) model is returned. */ - getTypeBindModel(typeBindFieldRef?: string): DynamicFormControlModel { + getTypeBindModel(typeBindFieldRef?: string): DynamicFormControlModel | undefined { const defaultModelId = this.typeFields.get(TYPE_BIND_DEFAULT_KEY); const modelId = this.typeFields.get(typeBindFieldRef) ?? typeBindFieldRef ?? defaultModelId; return this.typeBindModel.get(modelId) ?? this.typeBindModel.get(defaultModelId); } setTypeBindModel(model: DynamicFormControlModel) { + if (this.typeBindModel.get(model.id) === model) { + return; + } this.typeBindModel.set(model.id, model); this.typeBindModelUpdates.next(model.id); } @@ -362,19 +380,44 @@ export class FormBuilderService extends DynamicFormService { }); } + this.resetTypeBindModelsOnSubmissionChange(submissionId); + if (hasValue(typeBindModel)) { this.setTypeBindModel(typeBindModel); } else { - this.getTypeBindModelIds(rawData).forEach((typeBindModelId: string) => { - const foundModel = this.findById(typeBindModelId, rows); - if (hasValue(foundModel)) { - this.setTypeBindModel(foundModel); - } - }); + this.typeBindParsedRows.push(rows); + this.registerTypeBindModels(this.getTypeBindModelIds(rawData), rows); } return rows; } + /** + * Drop the type bind models registered by another submission. Sections of the same submission are + * parsed one by one and a field can be controlled by a field in a different section, so the + * registry must survive within a submission - but a model of the previously opened collection's + * form must not keep answering lookups, which would defeat the "controlling field is not part of + * this form, fall back to the default" behaviour for the rest of the session. + */ + private resetTypeBindModelsOnSubmissionChange(submissionId: string): void { + if (this.typeBindModelSubmissionId !== submissionId) { + this.typeBindModelSubmissionId = submissionId; + this.typeBindModel.clear(); + this.typeBindParsedRows = []; + } + } + + /** + * Register every model of the given rows whose id is one of the given controlling field ids + */ + private registerTypeBindModels(modelIds: string[], rows: DynamicFormControlModel[]): void { + modelIds.forEach((typeBindModelId: string) => { + const foundModel = this.findById(typeBindModelId, rows); + if (hasValue(foundModel)) { + this.setTypeBindModel(foundModel); + } + }); + } + /** * Collect the ids of every model that can control type binding for the given form configuration: * all values of the `submit.type-bind.field` map (the default field plus each `A=>B` override) and @@ -605,21 +648,31 @@ export class FormBuilderService extends DynamicFormService { return; } const typeFieldConfigValues: string[] = remoteData.payload.values || []; - typeFieldConfigValues.forEach((typeFieldConfig: string) => { - if (typeFieldConfig.includes(TYPE_BIND_FIELD_SEPARATOR)) { - // `dc.language.iso=>edm.type`: this metadata field is controlled by another field - const [boundField, controllingField] = typeFieldConfig.split(TYPE_BIND_FIELD_SEPARATOR); - if (isNotEmpty(boundField?.trim()) && isNotEmpty(controllingField?.trim())) { - this.typeFields.set(boundField.trim(), controllingField.trim().replace(/\./g, '_')); - } - } else if (isNotEmpty(typeFieldConfig?.trim())) { + typeFieldConfigValues.forEach((rawValue: string) => { + const typeFieldConfig = rawValue?.trim(); + if (isEmpty(typeFieldConfig)) { + return; + } + if (!typeFieldConfig.includes(TYPE_BIND_FIELD_SEPARATOR)) { // `dc.type`: the default controlling field. A duplicated value just overwrites itself. - this.typeFields.set(TYPE_BIND_DEFAULT_KEY, typeFieldConfig.trim().replace(/\./g, '_')); + this.typeFields.set(TYPE_BIND_DEFAULT_KEY, typeFieldConfig.replace(/\./g, '_')); + return; + } + // `dc.language.iso=>edm.type`: this metadata field is controlled by another field + const parts = typeFieldConfig.split(TYPE_BIND_FIELD_SEPARATOR).map((part: string) => part.trim()); + if (parts.length !== 2 || parts.some((part: string) => isEmpty(part))) { + console.warn(`Ignoring malformed submit.type-bind.field value "${rawValue}", expected "${TYPE_BIND_FIELD_SEPARATOR}"`); + return; } + this.typeFields.set(parts[0], parts[1].replace(/\./g, '_')); }); if (hasNoValue(this.typeFields.get(TYPE_BIND_DEFAULT_KEY))) { this.typeFields.set(TYPE_BIND_DEFAULT_KEY, 'dc_type'); } + // Forms parsed before the property arrived could not know about the `A=>B` overrides yet, so + // give their controlling models a second chance to be registered. + const typeBindModelIds = Array.from(this.typeFields.values()); + this.typeBindParsedRows.forEach((rows: DynamicFormControlModel[]) => this.registerTypeBindModels(typeBindModelIds, rows)); }); } diff --git a/src/app/shared/form/builder/models/form-field.model.spec.ts b/src/app/shared/form/builder/models/form-field.model.spec.ts new file mode 100644 index 00000000000..ce3f1333760 --- /dev/null +++ b/src/app/shared/form/builder/models/form-field.model.spec.ts @@ -0,0 +1,34 @@ +import { Deserialize } from 'cerialize'; + +import { FormFieldModel } from './form-field.model'; + +describe('FormFieldModel', () => { + + it('should deserialize the type bind configuration coming from submission-forms.xml', () => { + // shape of a single field in GET /api/config/submissionforms/ + const field = Deserialize({ + label: 'Pick the languages of the TEXT', + mandatory: 'true', + repeatable: true, + selectableMetadata: [{ metadata: 'dc.language.iso' }], + typeBind: ['TEXT'], + typeBindField: 'edm.type', + }, FormFieldModel) as FormFieldModel; + + expect(field.typeBind).toEqual(['TEXT']); + // without @autoserialize on typeBindField the per-field binding is dropped before the parser sees it + expect(field.typeBindField).toEqual('edm.type'); + }); + + it('should leave typeBindField undefined when the field has no ', () => { + const field = Deserialize({ + label: 'Title', + mandatory: 'true', + repeatable: false, + selectableMetadata: [{ metadata: 'dc.title' }], + typeBind: [], + }, FormFieldModel) as FormFieldModel; + + expect(field.typeBindField).toBeUndefined(); + }); +}); diff --git a/src/app/shared/form/builder/parsers/field-parser.ts b/src/app/shared/form/builder/parsers/field-parser.ts index a76e49c6b5c..48140d75032 100644 --- a/src/app/shared/form/builder/parsers/field-parser.ts +++ b/src/app/shared/form/builder/parsers/field-parser.ts @@ -301,9 +301,10 @@ export abstract class FieldParser { * while the form is being parsed. */ protected getTypeBindFieldRef(): string { - if (isNotEmpty(this.configData.typeBindField)) { + const typeBindField = this.configData.typeBindField?.trim(); + if (isNotEmpty(typeBindField)) { // input ids don't allow dots, so replace them - this is already a model id - return this.configData.typeBindField.replace(/\./g, '_'); + return typeBindField.replace(/\./g, '_'); } return this.getFieldId() || this.parserOptions.typeField; } From ed4b0faf7c81cdc526eb2cef433cf33fc84b2832 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Wed, 5 Aug 2026 13:31:47 +0200 Subject: [PATCH 03/13] Copilot follow-up: bound the parsed-rows cache and fix a misleading comment - typeBindParsedRows is only filled until submit.type-bind.field has been processed, and is dropped once it has. A section form re-parses on every data update, so the cache would otherwise keep growing and retain the model graph of every re-parse for the lifetime of the submission. Without a config service the map can never change, so nothing is cached at all. - Reword the matchesCondition guard comment: getTypeBindModel falls back to the default model, so no model at all means neither the field's controlling model nor the default one has been registered yet - not that the controlling field is permanently absent from the form. Co-Authored-By: Claude Opus 5 (1M context) --- .../ds-dynamic-type-bind-relation.service.ts | 7 +++--- .../form/builder/form-builder.service.ts | 22 +++++++++++++++++-- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts index 024a1ca9c57..3719d2b6391 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts @@ -112,9 +112,10 @@ export class DsDynamicTypeBindRelationService { // submission scope, form/section type and other high level properties const bindModel: any = this.formBuilderService.getTypeBindModel(condition?.id); - // The controlling model may not be part of this form at all (e.g. `dc.language.iso=>edm.type` - // is configured deployment-wide but this collection's form has no `edm.type` field), or it may - // not be registered yet. Keep MATCH_VISIBLE fields hidden until it becomes available. + // No model at all: getTypeBindModel falls back to the default controlling model, so this means + // neither the field's own controlling model nor the default one has been registered yet - + // typically because the section that holds them has not been parsed. Keep MATCH_VISIBLE fields + // hidden until one of them shows up. if (hasNoValue(bindModel)) { return relation.match === matcher.opposingMatch; } diff --git a/src/app/shared/form/builder/form-builder.service.ts b/src/app/shared/form/builder/form-builder.service.ts index cffa4a50570..39ed8448ee7 100644 --- a/src/app/shared/form/builder/form-builder.service.ts +++ b/src/app/shared/form/builder/form-builder.service.ts @@ -110,9 +110,16 @@ export class FormBuilderService extends DynamicFormService { /** * The parsed rows of the current submission, kept so that controlling models can still be * registered when the `submit.type-bind.field` property arrives after the form was parsed. + * Dropped - and no longer filled - as soon as that property has been processed. */ private typeBindParsedRows: DynamicFormControlModel[][]; + /** + * Whether the `submit.type-bind.field` property has been processed (successfully or not), i.e. + * whether {@link typeFields} can still change + */ + private typeBindConfigLoaded: boolean; + /** * The fields to use for type binding: TYPE_BIND_DEFAULT_KEY -> the default controlling model id, * plus one entry per metadata field that is controlled by another field, e.g. @@ -135,6 +142,8 @@ export class FormBuilderService extends DynamicFormService { this.typeBindModelUpdates = new Subject(); this.typeFields.set(TYPE_BIND_DEFAULT_KEY, 'dc_type'); + // Without a config service the type field map can never change, so nothing has to be re-scanned + this.typeBindConfigLoaded = hasNoValue(this.configService); // If optional config service was passed, perform an initial set of type field (default dc_type) for type binds if (hasValue(this.configService)) { this.setTypeBindFieldFromConfig(); @@ -385,7 +394,12 @@ export class FormBuilderService extends DynamicFormService { if (hasValue(typeBindModel)) { this.setTypeBindModel(typeBindModel); } else { - this.typeBindParsedRows.push(rows); + if (!this.typeBindConfigLoaded) { + // Only needed until submit.type-bind.field has been processed; after that every controlling + // field id is known at parse time, so there is nothing left to re-scan and holding on to the + // model graphs of re-parsed sections would just grow without bound. + this.typeBindParsedRows.push(rows); + } this.registerTypeBindModels(this.getTypeBindModelIds(rawData), rows); } return rows; @@ -642,9 +656,12 @@ export class FormBuilderService extends DynamicFormService { this.configService.findByPropertyName('submit.type-bind.field').pipe( getFirstCompletedRemoteData(), ).subscribe((remoteData: any) => { + // the type field map cannot change any more, whatever the outcome + this.typeBindConfigLoaded = true; // make sure we got a success response from the backend if (!remoteData.hasSucceeded) { this.typeFields.set(TYPE_BIND_DEFAULT_KEY, 'dc_type'); + this.typeBindParsedRows = []; return; } const typeFieldConfigValues: string[] = remoteData.payload.values || []; @@ -670,9 +687,10 @@ export class FormBuilderService extends DynamicFormService { this.typeFields.set(TYPE_BIND_DEFAULT_KEY, 'dc_type'); } // Forms parsed before the property arrived could not know about the `A=>B` overrides yet, so - // give their controlling models a second chance to be registered. + // give their controlling models a second chance to be registered, then drop the cache. const typeBindModelIds = Array.from(this.typeFields.values()); this.typeBindParsedRows.forEach((rows: DynamicFormControlModel[]) => this.registerTypeBindModels(typeBindModelIds, rows)); + this.typeBindParsedRows = []; }); } From 79f94f17ea2dc3d96f0a1124ff754057dc83adda Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Wed, 5 Aug 2026 13:59:14 +0200 Subject: [PATCH 04/13] Copilot follow-up: trim typeBindField in getTypeBindModelIds too isNotEmpty(' ') is true in this codebase, so a padded `` would have registered the controlling model as ' edm_type ' while FieldParser.getTypeBindFieldRef - which does trim - stamps the relations with 'edm_type'. The lookup would then miss, fall back to dc_type and leave the dependent field permanently hidden, i.e. reproduce the very bug this PR fixes. The existing end-to-end spec now uses a padded value, so it fails without the trim. Co-Authored-By: Claude Opus 5 (1M context) --- src/app/shared/form/builder/form-builder.service.spec.ts | 3 ++- src/app/shared/form/builder/form-builder.service.ts | 7 +++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/app/shared/form/builder/form-builder.service.spec.ts b/src/app/shared/form/builder/form-builder.service.spec.ts index d2d8ae41b2f..98efea3aa5d 100644 --- a/src/app/shared/form/builder/form-builder.service.spec.ts +++ b/src/app/shared/form/builder/form-builder.service.spec.ts @@ -955,7 +955,8 @@ describe('FormBuilderService per-field type bind test suite', () => { } as FormFieldModel] } as FormRowModel, { fields: [{ input: { type: 'onebox' }, label: 'Language', mandatory: 'false', repeatable: false, - hints: '', languageCodes: [], typeBind: ['TEXT'], typeBindField: 'edm.type', + // padded on purpose: the registration path and the relation ids must agree on trimming + hints: '', languageCodes: [], typeBind: ['TEXT'], typeBindField: ' edm.type ', selectableMetadata: [{ metadata: 'dc.language.iso' }], } as FormFieldModel] } as FormRowModel, ], diff --git a/src/app/shared/form/builder/form-builder.service.ts b/src/app/shared/form/builder/form-builder.service.ts index 39ed8448ee7..ae191a587f2 100644 --- a/src/app/shared/form/builder/form-builder.service.ts +++ b/src/app/shared/form/builder/form-builder.service.ts @@ -443,8 +443,11 @@ export class FormBuilderService extends DynamicFormService { const ids = new Set(this.typeFields.values()); const collectFromRows = (formRows: FormRowModel[]): void => { (formRows || []).forEach((formRow: FormRowModel) => (formRow?.fields || []).forEach((field: FormFieldModel) => { - if (isNotEmpty(field?.typeBindField)) { - ids.add(field.typeBindField.replace(/\./g, '_')); + // trim exactly like FieldParser.getTypeBindFieldRef does, otherwise a padded value in the + // XML would register ' edm_type ' while the relations point at 'edm_type' + const typeBindField = field?.typeBindField?.trim(); + if (isNotEmpty(typeBindField)) { + ids.add(typeBindField.replace(/\./g, '_')); } collectFromRows(field?.rows); })); From 9b5b8f498b37a392b4d75f6c2fe624ef09e59c69 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Wed, 5 Aug 2026 14:06:29 +0200 Subject: [PATCH 05/13] Follow the controlling model when its section is re-parsed Attachment was deduped by model id, but setTypeBindModel emits on identity: when the section holding the controlling field is re-parsed (section forms re-parse on every data update) a NEW instance is registered under the same id, and a bound field in another section kept listening to the dead one - so the dependent field stopped reacting, which is the A3 symptom again in a narrower form. Attachment is now keyed by id but compared by identity, and the stale child subscription is removed from the owning Subscription and unsubscribed, so nothing accumulates across re-parses either. Co-Authored-By: Claude Opus 5 (1M context) --- ...dynamic-type-bind-relation.service.spec.ts | 30 +++++++++++++++++++ .../ds-dynamic-type-bind-relation.service.ts | 20 +++++++++---- 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts index 2392517aa0c..72305e7894b 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts @@ -202,6 +202,36 @@ describe('DSDynamicTypeBindRelationService test suite', () => { subscription.unsubscribe(); }); + it('Should follow a re-registered controlling model and drop the stale one', () => { + // re-parsing the section that holds the controlling field yields a new instance under the same id + const bindModelUpdates = new Subject(); + const formBuilderServiceSpy: any = (service as any).formBuilderService; + formBuilderServiceSpy.getTypeBindModelUpdates.and.returnValue(bindModelUpdates.asObservable()); + + const firstInstance = new DsDynamicInputModel({ ...dcTypeInputConfig, id: 'edm_type', name: 'edm.type' }); + formBuilderServiceSpy.getTypeBindModel.and.returnValue(firstInstance); + + const testModel = mockInputWithTypeBindModel; + testModel.typeBindRelations = getTypeBindRelations(['boundType'], 'edm_type'); + const [subscription] = service.subscribeRelations(testModel, new UntypedFormControl()); + + const secondInstance = new DsDynamicInputModel({ ...dcTypeInputConfig, id: 'edm_type', name: 'edm.type' }); + formBuilderServiceSpy.getTypeBindModel.and.returnValue(secondInstance); + bindModelUpdates.next('edm_type'); + + secondInstance.value = 'boundType'; + expect(testModel.hidden).toBeFalse(); + + // the replaced instance must no longer drive the field + firstInstance.value = 'anotherType'; + expect(testModel.hidden).toBeFalse(); + + secondInstance.value = 'anotherType'; + expect(testModel.hidden).toBeTrue(); + + subscription.unsubscribe(); + }); + }); }); diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts index 3719d2b6391..7ebd4036b94 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts @@ -203,13 +203,21 @@ export class DsDynamicTypeBindRelationService { subscribeRelations(model: DynamicFormControlModel, control: UntypedFormControl): Subscription[] { const subscriptions = new Subscription(); - const attachedModelIds = new Set(); + // keyed by model id, but compared by identity: re-parsing the section that holds the controlling + // field produces a NEW model instance under the same id, and this field has to follow it + const attachedModels = new Map(); + const attachedSubscriptions = new Map(); const attachRelatedModels = (relatedModels: DynamicFormControlModel[]) => { relatedModels.forEach((relatedModel: any) => { - if (hasValue(relatedModel) && !attachedModelIds.has(relatedModel.id)) { - attachedModelIds.add(relatedModel.id); + if (hasValue(relatedModel) && attachedModels.get(relatedModel.id) !== relatedModel) { + const staleSubscription = attachedSubscriptions.get(relatedModel.id); + if (hasValue(staleSubscription)) { + subscriptions.remove(staleSubscription); + staleSubscription.unsubscribe(); + } + attachedModels.set(relatedModel.id, relatedModel); const initValue = (hasNoValue(relatedModel.value) || typeof relatedModel.value === 'string') ? relatedModel.value : (Array.isArray(relatedModel.value) ? relatedModel.value : relatedModel.value.value); @@ -220,14 +228,16 @@ export class DsDynamicTypeBindRelationService { ); // Build up the subscriptions to watch for changes; - subscriptions.add(valueChanges.subscribe(() => this.evaluateRelations(model, control))); + const valueChangesSubscription = valueChanges.subscribe(() => this.evaluateRelations(model, control)); + attachedSubscriptions.set(relatedModel.id, valueChangesSubscription); + subscriptions.add(valueChangesSubscription); } }); }; attachRelatedModels(this.getRelatedFormModel(model)); - if (attachedModelIds.size === 0) { + if (attachedModels.size === 0) { // Nothing to listen to yet: evaluate once so the "controlling model missing" fallback applies // and the field does not stay in whatever state it was rendered in. this.evaluateRelations(model, control); From f25829c4c012549f53081b89f21324ad23c30d2b Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Wed, 5 Aug 2026 14:53:00 +0200 Subject: [PATCH 06/13] Copilot follow-up: do not evaluate a self-bound relation at all Skipping the self-reference in getRelatedFormModel only stopped it from being subscribed to; evaluateRelations still ran matchesCondition against the field's own (empty) value on the initial pass, hid the field, and - with nothing attached - never re-evaluated it, so a misconfigured pointing at its own metadata field made that field permanently unreachable. subscribeRelations now detects the self-reference up front, warns once and returns without evaluating or attaching anything, leaving the field exactly as rendered. Co-Authored-By: Claude Opus 5 (1M context) --- ...dynamic-type-bind-relation.service.spec.ts | 16 ++++++++++++ .../ds-dynamic-type-bind-relation.service.ts | 26 ++++++++++++++++--- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts index 72305e7894b..a0deeee49a1 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts @@ -174,6 +174,22 @@ describe('DSDynamicTypeBindRelationService test suite', () => { expect(testModel.hidden).toBeFalse(); }); + it('Should leave a self-bound field untouched instead of hiding it forever', () => { + const formBuilderServiceSpy: any = (service as any).formBuilderService; + const testModel = mockInputWithTypeBindModel; + // the field's own resolves back to the field itself + testModel.typeBindRelations = getTypeBindRelations(['boundType'], testModel.id); + formBuilderServiceSpy.getTypeBindModel.and.returnValue(testModel); + testModel.hidden = false; + + const subscriptions = service.subscribeRelations(testModel, new UntypedFormControl()); + + expect(service.getRelatedFormModel(testModel)).toHaveSize(0); + // nothing is evaluated, so the misconfigured field stays usable instead of being hidden forever + expect(testModel.hidden).toBeFalse(); + subscriptions.forEach((subscription) => subscription.unsubscribe()); + }); + it('Should attach the real controlling model even when it was first bound to the default one', () => { // until edm_type is registered, getTypeBindModel falls back to the default dc_type model, so // a related model IS attached - the late registration must still be picked up diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts index 7ebd4036b94..5cc1a2c40b6 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts @@ -76,10 +76,8 @@ export class DsDynamicTypeBindRelationService { } if (bindModel.id === model.id) { - // A misconfigured pointing at the field itself. Skip the relation - // instead of throwing: this runs during form init and from the type bind model registration - // callback, so throwing would take down the whole submission section over one bad field. - console.warn(`FormControl ${model.id} cannot depend on itself, ignoring its type bind relation`); + // A misconfigured pointing at the field itself - see + // dependsOnItself(), which stops the relation from being evaluated at all. return; } @@ -91,6 +89,15 @@ export class DsDynamicTypeBindRelationService { return models; } + /** + * Whether any type bind relation of the given model resolves to the model itself, i.e. the field + * declares `` pointing at its own metadata field. + */ + private dependsOnItself(model: DynamicFormControlModel): boolean { + return ((model as any).typeBindRelations || []).some((relGroup) => + (relGroup.when || []).some((rel) => this.formBuilderService.getTypeBindModel(rel?.id)?.id === model.id)); + } + /** * Return false if the type bind relation (eg. {MATCH_VISIBLE, OR, ['book', 'book part']}) matches the value in * matcher.match or true if the opposite match. Since this is called with regard to actively *hiding* a form @@ -203,6 +210,17 @@ export class DsDynamicTypeBindRelationService { subscribeRelations(model: DynamicFormControlModel, control: UntypedFormControl): Subscription[] { const subscriptions = new Subscription(); + + if (this.dependsOnItself(model)) { + // Misconfigured pointing at the field itself. Upstream throws here, + // which would take down the whole submission section over one bad field; and merely skipping + // the relation is not enough either - evaluating it would hide the field on the initial pass + // and nothing would ever re-evaluate it, making it permanently unreachable. Leave the field + // exactly as rendered and warn. + console.warn(`FormControl ${model.id} cannot depend on itself, ignoring its type bind relation`); + return [subscriptions]; + } + // keyed by model id, but compared by identity: re-parsing the section that holds the controlling // field produces a NEW model instance under the same id, and this field has to follow it const attachedModels = new Map(); From c0891cff15269e9eeb980c6457c13eaeadfbd8c0 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Wed, 5 Aug 2026 15:48:16 +0200 Subject: [PATCH 07/13] Distinguish a configured self-reference from a not-yet-parsed controlling model The self-reference guard used the runtime lookup, which falls back to the default model. For a field whose own id IS the default model id, a relation whose real target simply had not been parsed yet therefore looked like a misconfiguration: subscribeRelations bailed out before wiring the registration listener, so the field never picked up its controlling model. Resolve the reference from configuration only (new FormBuilderService.resolveTypeBindModelId, order-independent) to decide whether the field really is bound to itself; the transient case now just skips the initial evaluation - so the field is not hidden for no reason - and still attaches when the real model is registered. Also add the spec that actually pins the getTypeBindModelIds trim. The one added in 79f94f17ea did not: the suite's config already contributes 'edm_type' through its A=>B entry, so the padded id was merely an extra miss. The new case configures only 'dc.type', making the padded the sole source of the controlling id - verified to fail with the trim removed. Co-Authored-By: Claude Opus 5 (1M context) --- ...dynamic-type-bind-relation.service.spec.ts | 27 +++++++++++++++++ .../ds-dynamic-type-bind-relation.service.ts | 30 ++++++++++++++----- .../form/builder/form-builder.service.spec.ts | 26 ++++++++++++++++ .../form/builder/form-builder.service.ts | 12 ++++++-- .../shared/mocks/form-builder-service.mock.ts | 1 + 5 files changed, 86 insertions(+), 10 deletions(-) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts index a0deeee49a1..f6f0ea38f73 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts @@ -179,6 +179,7 @@ describe('DSDynamicTypeBindRelationService test suite', () => { const testModel = mockInputWithTypeBindModel; // the field's own resolves back to the field itself testModel.typeBindRelations = getTypeBindRelations(['boundType'], testModel.id); + formBuilderServiceSpy.resolveTypeBindModelId.and.returnValue(testModel.id); formBuilderServiceSpy.getTypeBindModel.and.returnValue(testModel); testModel.hidden = false; @@ -190,6 +191,32 @@ describe('DSDynamicTypeBindRelationService test suite', () => { subscriptions.forEach((subscription) => subscription.unsubscribe()); }); + it('Should not hide a field whose controlling model has not been parsed yet, and attach when it is', () => { + // the real target (edm_type) is not registered, so getTypeBindModel falls back to the default + // model - which in this form happens to BE this field. That is not a misconfiguration. + const bindModelUpdates = new Subject(); + const formBuilderServiceSpy: any = (service as any).formBuilderService; + formBuilderServiceSpy.getTypeBindModelUpdates.and.returnValue(bindModelUpdates.asObservable()); + formBuilderServiceSpy.resolveTypeBindModelId.and.returnValue('edm_type'); + + const testModel = mockInputWithTypeBindModel; + testModel.typeBindRelations = getTypeBindRelations(['boundType'], 'edm_type'); + formBuilderServiceSpy.getTypeBindModel.and.returnValue(testModel); + testModel.hidden = false; + + const [subscription] = service.subscribeRelations(testModel, new UntypedFormControl()); + expect(testModel.hidden).toBeFalse(); + + const controllingModel = new DsDynamicInputModel({ ...dcTypeInputConfig, id: 'edm_type', name: 'edm.type' }); + formBuilderServiceSpy.getTypeBindModel.and.returnValue(controllingModel); + bindModelUpdates.next('edm_type'); + + controllingModel.value = 'anotherType'; + expect(testModel.hidden).toBeTrue(); + + subscription.unsubscribe(); + }); + it('Should attach the real controlling model even when it was first bound to the default one', () => { // until edm_type is registered, getTypeBindModel falls back to the default dc_type model, so // a related model IS attached - the late registration must still be picked up diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts index 5cc1a2c40b6..dd304d0ce10 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts @@ -90,10 +90,22 @@ export class DsDynamicTypeBindRelationService { } /** - * Whether any type bind relation of the given model resolves to the model itself, i.e. the field - * declares `` pointing at its own metadata field. + * Whether the configuration itself binds the given model to its own metadata field, i.e. a + * misconfigured `` pointing at the field it is declared on. Resolved from + * the configuration only, so it cannot be confused with a relation that merely *currently* falls + * back to the default model because its real target has not been parsed yet. */ - private dependsOnItself(model: DynamicFormControlModel): boolean { + private isConfiguredToDependOnItself(model: DynamicFormControlModel): boolean { + return ((model as any).typeBindRelations || []).some((relGroup) => + (relGroup.when || []).some((rel) => this.formBuilderService.resolveTypeBindModelId(rel?.id) === model.id)); + } + + /** + * Whether the model that a relation resolves to *right now* is the model itself - true either for + * the misconfiguration above or, transiently, when the real controlling model has not been + * registered yet and the default one happens to be this very field. + */ + private currentlyResolvesToItself(model: DynamicFormControlModel): boolean { return ((model as any).typeBindRelations || []).some((relGroup) => (relGroup.when || []).some((rel) => this.formBuilderService.getTypeBindModel(rel?.id)?.id === model.id)); } @@ -211,12 +223,12 @@ export class DsDynamicTypeBindRelationService { const subscriptions = new Subscription(); - if (this.dependsOnItself(model)) { + if (this.isConfiguredToDependOnItself(model)) { // Misconfigured pointing at the field itself. Upstream throws here, // which would take down the whole submission section over one bad field; and merely skipping // the relation is not enough either - evaluating it would hide the field on the initial pass - // and nothing would ever re-evaluate it, making it permanently unreachable. Leave the field - // exactly as rendered and warn. + // and nothing would ever re-evaluate it, making it permanently unreachable. No later + // registration can change the configuration, so leave the field exactly as rendered and warn. console.warn(`FormControl ${model.id} cannot depend on itself, ignoring its type bind relation`); return [subscriptions]; } @@ -255,9 +267,11 @@ export class DsDynamicTypeBindRelationService { attachRelatedModels(this.getRelatedFormModel(model)); - if (attachedModels.size === 0) { + if (attachedModels.size === 0 && !this.currentlyResolvesToItself(model)) { // Nothing to listen to yet: evaluate once so the "controlling model missing" fallback applies - // and the field does not stay in whatever state it was rendered in. + // and the field does not stay in whatever state it was rendered in. Skipped when the relation + // currently resolves to this field itself - the real controlling model simply has not been + // parsed yet, and evaluating against our own value would hide the field for no reason. this.evaluateRelations(model, control); } diff --git a/src/app/shared/form/builder/form-builder.service.spec.ts b/src/app/shared/form/builder/form-builder.service.spec.ts index 98efea3aa5d..4e6e0cbbbd3 100644 --- a/src/app/shared/form/builder/form-builder.service.spec.ts +++ b/src/app/shared/form/builder/form-builder.service.spec.ts @@ -1102,6 +1102,32 @@ describe('FormBuilderService per-field type bind with a slow configuration respo expect(service.getTypeBindModel('dc.language.iso').id).toEqual('edm_type'); }); + it('should register a controlling model declared only by a padded ', () => { + // there is no A=>B entry for this field, so the whitespace-padded XML attribute is the ONLY + // source of the controlling model id - it has to be trimmed the same way the relation ids are + configResponse.next(createSuccessfulRemoteDataObject({ + ... new ConfigurationProperty(), + name: 'submit.type-bind.field', + values: ['dc.type'], + })); + configResponse.complete(); + + const paddedConfiguration = { + ...overrideOnlyFormConfiguration, + rows: [ + overrideOnlyFormConfiguration.rows[0], + { fields: [{ + ...overrideOnlyFormConfiguration.rows[1].fields[0], + typeBindField: ' edm.type ', + } as FormFieldModel] } as FormRowModel, + ], + } as any; + + service.modelFromConfiguration(submissionId, paddedConfiguration, 'testScopeUUID'); + + expect(service.getTypeBindModel('edm_type')?.id).toEqual('edm_type'); + }); + it('should ignore blank and malformed values', () => { configResponse.next(createSuccessfulRemoteDataObject({ ... new ConfigurationProperty(), diff --git a/src/app/shared/form/builder/form-builder.service.ts b/src/app/shared/form/builder/form-builder.service.ts index ae191a587f2..692740d7e19 100644 --- a/src/app/shared/form/builder/form-builder.service.ts +++ b/src/app/shared/form/builder/form-builder.service.ts @@ -169,8 +169,16 @@ export class FormBuilderService extends DynamicFormService { */ getTypeBindModel(typeBindFieldRef?: string): DynamicFormControlModel | undefined { const defaultModelId = this.typeFields.get(TYPE_BIND_DEFAULT_KEY); - const modelId = this.typeFields.get(typeBindFieldRef) ?? typeBindFieldRef ?? defaultModelId; - return this.typeBindModel.get(modelId) ?? this.typeBindModel.get(defaultModelId); + return this.typeBindModel.get(this.resolveTypeBindModelId(typeBindFieldRef)) ?? this.typeBindModel.get(defaultModelId); + } + + /** + * Resolve which model id a type bind reference points at, purely from configuration - unlike + * {@link getTypeBindModel} this does not fall back to the default model when the target is not + * (yet) part of the form, so the answer does not depend on parse order. + */ + resolveTypeBindModelId(typeBindFieldRef?: string): string { + return this.typeFields.get(typeBindFieldRef) ?? typeBindFieldRef ?? this.typeFields.get(TYPE_BIND_DEFAULT_KEY); } setTypeBindModel(model: DynamicFormControlModel) { diff --git a/src/app/shared/mocks/form-builder-service.mock.ts b/src/app/shared/mocks/form-builder-service.mock.ts index fcb30dab2b2..91aa2b75def 100644 --- a/src/app/shared/mocks/form-builder-service.mock.ts +++ b/src/app/shared/mocks/form-builder-service.mock.ts @@ -47,5 +47,6 @@ export function getMockFormBuilderService(): FormBuilderService { }, ), getTypeBindModelUpdates: EMPTY, + resolveTypeBindModelId: undefined, }); } From 7312c5bf7b5ab1a4705b66e59c2ea6c320183744 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Wed, 5 Aug 2026 16:28:56 +0200 Subject: [PATCH 08/13] Copilot follow-up: make the default type bind model id explicit resolveTypeBindModelId declared `string` but ended in a bare Map.get, and getTypeBindModel fed the same possibly-undefined value into its fallback lookup. Both now go through getDefaultTypeBindModelId(), which falls back to the TYPE_BIND_DEFAULT_MODEL_ID constant that also replaces the four scattered 'dc_type' literals. Also fix a comment still naming the pre-rename dependsOnItself(). Co-Authored-By: Claude Opus 5 (1M context) --- .../ds-dynamic-type-bind-relation.service.ts | 2 +- .../form/builder/form-builder.service.ts | 27 ++++++++++++++----- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts index dd304d0ce10..3e3c8303903 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts @@ -77,7 +77,7 @@ export class DsDynamicTypeBindRelationService { if (bindModel.id === model.id) { // A misconfigured pointing at the field itself - see - // dependsOnItself(), which stops the relation from being evaluated at all. + // isConfiguredToDependOnItself(), which stops the relation from being evaluated at all. return; } diff --git a/src/app/shared/form/builder/form-builder.service.ts b/src/app/shared/form/builder/form-builder.service.ts index 692740d7e19..a47bb40e90c 100644 --- a/src/app/shared/form/builder/form-builder.service.ts +++ b/src/app/shared/form/builder/form-builder.service.ts @@ -76,6 +76,11 @@ export const TYPE_BIND_DEFAULT_KEY = 'default'; */ const TYPE_BIND_FIELD_SEPARATOR = '=>'; +/** + * Model id of the controlling field used when `submit.type-bind.field` declares no default + */ +const TYPE_BIND_DEFAULT_MODEL_ID = 'dc_type'; + @Injectable({ providedIn: 'root' }) export class FormBuilderService extends DynamicFormService { @@ -141,7 +146,7 @@ export class FormBuilderService extends DynamicFormService { this.typeBindParsedRows = []; this.typeBindModelUpdates = new Subject(); - this.typeFields.set(TYPE_BIND_DEFAULT_KEY, 'dc_type'); + this.typeFields.set(TYPE_BIND_DEFAULT_KEY, TYPE_BIND_DEFAULT_MODEL_ID); // Without a config service the type field map can never change, so nothing has to be re-scanned this.typeBindConfigLoaded = hasNoValue(this.configService); // If optional config service was passed, perform an initial set of type field (default dc_type) for type binds @@ -168,8 +173,8 @@ export class FormBuilderService extends DynamicFormService { * to a model that is not part of the current form, the default (usually `dc_type`) model is returned. */ getTypeBindModel(typeBindFieldRef?: string): DynamicFormControlModel | undefined { - const defaultModelId = this.typeFields.get(TYPE_BIND_DEFAULT_KEY); - return this.typeBindModel.get(this.resolveTypeBindModelId(typeBindFieldRef)) ?? this.typeBindModel.get(defaultModelId); + return this.typeBindModel.get(this.resolveTypeBindModelId(typeBindFieldRef)) + ?? this.typeBindModel.get(this.getDefaultTypeBindModelId()); } /** @@ -178,7 +183,15 @@ export class FormBuilderService extends DynamicFormService { * (yet) part of the form, so the answer does not depend on parse order. */ resolveTypeBindModelId(typeBindFieldRef?: string): string { - return this.typeFields.get(typeBindFieldRef) ?? typeBindFieldRef ?? this.typeFields.get(TYPE_BIND_DEFAULT_KEY); + return this.typeFields.get(typeBindFieldRef) ?? typeBindFieldRef ?? this.getDefaultTypeBindModelId(); + } + + /** + * The id of the model of the default controlling field. Never undefined: the constructor seeds the + * map and {@link setTypeBindFieldFromConfig} restores the entry whatever the configuration says. + */ + private getDefaultTypeBindModelId(): string { + return this.typeFields.get(TYPE_BIND_DEFAULT_KEY) ?? TYPE_BIND_DEFAULT_MODEL_ID; } setTypeBindModel(model: DynamicFormControlModel) { @@ -671,7 +684,7 @@ export class FormBuilderService extends DynamicFormService { this.typeBindConfigLoaded = true; // make sure we got a success response from the backend if (!remoteData.hasSucceeded) { - this.typeFields.set(TYPE_BIND_DEFAULT_KEY, 'dc_type'); + this.typeFields.set(TYPE_BIND_DEFAULT_KEY, TYPE_BIND_DEFAULT_MODEL_ID); this.typeBindParsedRows = []; return; } @@ -695,7 +708,7 @@ export class FormBuilderService extends DynamicFormService { this.typeFields.set(parts[0], parts[1].replace(/\./g, '_')); }); if (hasNoValue(this.typeFields.get(TYPE_BIND_DEFAULT_KEY))) { - this.typeFields.set(TYPE_BIND_DEFAULT_KEY, 'dc_type'); + this.typeFields.set(TYPE_BIND_DEFAULT_KEY, TYPE_BIND_DEFAULT_MODEL_ID); } // Forms parsed before the property arrived could not know about the `A=>B` overrides yet, so // give their controlling models a second chance to be registered, then drop the cache. @@ -714,7 +727,7 @@ export class FormBuilderService extends DynamicFormService { if (hasValue(this.configService)) { this.setTypeBindFieldFromConfig(); } else { - this.typeFields.set(TYPE_BIND_DEFAULT_KEY, 'dc_type'); + this.typeFields.set(TYPE_BIND_DEFAULT_KEY, TYPE_BIND_DEFAULT_MODEL_ID); } } return this.typeFields.get(TYPE_BIND_DEFAULT_KEY); From 9d9c747f5419618726e05580633c212767f9d8b2 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Wed, 5 Aug 2026 17:06:40 +0200 Subject: [PATCH 09/13] Copilot follow-up: route getTypeField through the guaranteed default too It was the one remaining place that returned a bare Map.get for a method declared to return string. Co-Authored-By: Claude Opus 5 (1M context) --- src/app/shared/form/builder/form-builder.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/shared/form/builder/form-builder.service.ts b/src/app/shared/form/builder/form-builder.service.ts index a47bb40e90c..6e5e2b43668 100644 --- a/src/app/shared/form/builder/form-builder.service.ts +++ b/src/app/shared/form/builder/form-builder.service.ts @@ -730,7 +730,7 @@ export class FormBuilderService extends DynamicFormService { this.typeFields.set(TYPE_BIND_DEFAULT_KEY, TYPE_BIND_DEFAULT_MODEL_ID); } } - return this.typeFields.get(TYPE_BIND_DEFAULT_KEY); + return this.getDefaultTypeBindModelId(); } } From 4cde9131880d95c6c5254794a2b1f82c83b4cec8 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Wed, 5 Aug 2026 17:47:17 +0200 Subject: [PATCH 10/13] Copilot follow-up: honest mock return value and an optional typeBindField - The shared FormBuilderService mock returned undefined from resolveTypeBindModelId while the real one always returns a model id. It now mirrors the real implementation for a reference the type field map does not remap, so a caller that starts using the value does not silently get undefined. - typeBindField is genuinely optional in the REST payload and the code already treats it that way (`?.trim()` in FieldParser and in getTypeBindModelIds, and a spec asserting it stays undefined when the attribute is absent). I argued for consistency with the other non-optional @autoserialize members earlier; the usage asymmetry is the stronger argument, so it is now declared optional. Co-Authored-By: Claude Opus 5 (1M context) --- src/app/shared/form/builder/models/form-field.model.ts | 7 ++++--- src/app/shared/mocks/form-builder-service.mock.ts | 7 ++++++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/app/shared/form/builder/models/form-field.model.ts b/src/app/shared/form/builder/models/form-field.model.ts index a8a07391865..7c4fb189fc0 100644 --- a/src/app/shared/form/builder/models/form-field.model.ts +++ b/src/app/shared/form/builder/models/form-field.model.ts @@ -134,11 +134,12 @@ export class FormFieldModel { /** * The metadata field that controls the type binding of this field, coming from the - * `` attribute in submission-forms.xml. When empty, this field is - * controlled by the default field configured in the `submit.type-bind.field` property. + * `` attribute in submission-forms.xml. Optional: when the attribute + * is absent this field is controlled by the default field configured in the + * `submit.type-bind.field` property, so readers have to handle `undefined`. */ @autoserialize - typeBindField: string; + typeBindField?: string; /** * Containing the value for this metadata field diff --git a/src/app/shared/mocks/form-builder-service.mock.ts b/src/app/shared/mocks/form-builder-service.mock.ts index 91aa2b75def..30b0e2b43f0 100644 --- a/src/app/shared/mocks/form-builder-service.mock.ts +++ b/src/app/shared/mocks/form-builder-service.mock.ts @@ -9,7 +9,7 @@ import { FormBuilderService } from '../form/builder/form-builder.service'; export function getMockFormBuilderService(): FormBuilderService { - return jasmine.createSpyObj('FormBuilderService', { + const formBuilderService = jasmine.createSpyObj('FormBuilderService', { modelFromConfiguration: [], createFormGroup: new UntypedFormGroup({}), getValueFromModel: {}, @@ -49,4 +49,9 @@ export function getMockFormBuilderService(): FormBuilderService { getTypeBindModelUpdates: EMPTY, resolveTypeBindModelId: undefined, }); + + // mirror the real implementation for a reference that the type field map does not remap + formBuilderService.resolveTypeBindModelId.and.callFake((typeBindFieldRef: string) => typeBindFieldRef ?? 'dc_type'); + + return formBuilderService; } From b86c0d6c34259d63b840fc12f9626702f3484a84 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Wed, 5 Aug 2026 18:25:32 +0200 Subject: [PATCH 11/13] Copilot follow-up: align the mock signature with the optional parameter resolveTypeBindModelId takes an optional ref, which is what the `??` fallback in the fake is there for. Co-Authored-By: Claude Opus 5 (1M context) --- src/app/shared/mocks/form-builder-service.mock.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/shared/mocks/form-builder-service.mock.ts b/src/app/shared/mocks/form-builder-service.mock.ts index 30b0e2b43f0..2b71d63448b 100644 --- a/src/app/shared/mocks/form-builder-service.mock.ts +++ b/src/app/shared/mocks/form-builder-service.mock.ts @@ -51,7 +51,7 @@ export function getMockFormBuilderService(): FormBuilderService { }); // mirror the real implementation for a reference that the type field map does not remap - formBuilderService.resolveTypeBindModelId.and.callFake((typeBindFieldRef: string) => typeBindFieldRef ?? 'dc_type'); + formBuilderService.resolveTypeBindModelId.and.callFake((typeBindFieldRef?: string) => typeBindFieldRef ?? 'dc_type'); return formBuilderService; } From 79aee3efcdd17d7acd153a176dbfd1c5ba8d7269 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Thu, 6 Aug 2026 09:02:10 +0200 Subject: [PATCH 12/13] Shorten the type bind comments Keep the non-obvious reasoning, drop the prose around it. Co-Authored-By: Claude Opus 5 (1M context) --- .../ds-dynamic-type-bind-relation.service.ts | 51 ++++------ .../form/builder/form-builder.service.ts | 93 +++++++------------ .../form/builder/models/form-field.model.ts | 6 +- .../form/builder/parsers/field-parser.ts | 14 +-- .../shared/mocks/form-builder-service.mock.ts | 2 +- 5 files changed, 60 insertions(+), 106 deletions(-) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts index 3e3c8303903..a652017edb4 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts @@ -76,8 +76,7 @@ export class DsDynamicTypeBindRelationService { } if (bindModel.id === model.id) { - // A misconfigured pointing at the field itself - see - // isConfiguredToDependOnItself(), which stops the relation from being evaluated at all. + // self-bound field, see isConfiguredToDependOnItself() return; } @@ -90,10 +89,9 @@ export class DsDynamicTypeBindRelationService { } /** - * Whether the configuration itself binds the given model to its own metadata field, i.e. a - * misconfigured `` pointing at the field it is declared on. Resolved from - * the configuration only, so it cannot be confused with a relation that merely *currently* falls - * back to the default model because its real target has not been parsed yet. + * Whether the configuration binds the model to its own metadata field, i.e. a `` + * pointing at the field it is declared on. Config-only, so it can't be confused with a relation + * that merely falls back to the default model until its real target is parsed. */ private isConfiguredToDependOnItself(model: DynamicFormControlModel): boolean { return ((model as any).typeBindRelations || []).some((relGroup) => @@ -101,9 +99,8 @@ export class DsDynamicTypeBindRelationService { } /** - * Whether the model that a relation resolves to *right now* is the model itself - true either for - * the misconfiguration above or, transiently, when the real controlling model has not been - * registered yet and the default one happens to be this very field. + * Whether a relation resolves to the model itself *right now* - the misconfiguration above, or + * transiently the default model standing in for a controlling model that isn't parsed yet. */ private currentlyResolvesToItself(model: DynamicFormControlModel): boolean { return ((model as any).typeBindRelations || []).some((relGroup) => @@ -131,10 +128,8 @@ export class DsDynamicTypeBindRelationService { // submission scope, form/section type and other high level properties const bindModel: any = this.formBuilderService.getTypeBindModel(condition?.id); - // No model at all: getTypeBindModel falls back to the default controlling model, so this means - // neither the field's own controlling model nor the default one has been registered yet - - // typically because the section that holds them has not been parsed. Keep MATCH_VISIBLE fields - // hidden until one of them shows up. + // Nothing registered yet, not even the default fallback - the section holding the controlling + // field hasn't been parsed. Keep MATCH_VISIBLE fields hidden until one shows up. if (hasNoValue(bindModel)) { return relation.match === matcher.opposingMatch; } @@ -211,10 +206,8 @@ export class DsDynamicTypeBindRelationService { /** * Return an array of subscriptions to a calling component. * - * A single owning {@link Subscription} is returned rather than the individual child - * subscriptions: the controlling model may only be registered after this method has returned (see - * below), and callers snapshot the returned array, so any later child has to hang off something - * they already hold in order to be torn down with the component. + * One owning {@link Subscription} rather than the individual children: callers snapshot the + * returned array, and children are still added afterwards when a controlling model shows up late. * * @param model * @param control @@ -224,17 +217,13 @@ export class DsDynamicTypeBindRelationService { const subscriptions = new Subscription(); if (this.isConfiguredToDependOnItself(model)) { - // Misconfigured pointing at the field itself. Upstream throws here, - // which would take down the whole submission section over one bad field; and merely skipping - // the relation is not enough either - evaluating it would hide the field on the initial pass - // and nothing would ever re-evaluate it, making it permanently unreachable. No later - // registration can change the configuration, so leave the field exactly as rendered and warn. + // Upstream throws here, taking down the whole section over one bad field. Evaluating the + // relation instead would hide the field forever, so leave it as rendered and warn. console.warn(`FormControl ${model.id} cannot depend on itself, ignoring its type bind relation`); return [subscriptions]; } - // keyed by model id, but compared by identity: re-parsing the section that holds the controlling - // field produces a NEW model instance under the same id, and this field has to follow it + // keyed by id, compared by identity: re-parsing a section yields a new instance under the same id const attachedModels = new Map(); const attachedSubscriptions = new Map(); @@ -268,18 +257,14 @@ export class DsDynamicTypeBindRelationService { attachRelatedModels(this.getRelatedFormModel(model)); if (attachedModels.size === 0 && !this.currentlyResolvesToItself(model)) { - // Nothing to listen to yet: evaluate once so the "controlling model missing" fallback applies - // and the field does not stay in whatever state it was rendered in. Skipped when the relation - // currently resolves to this field itself - the real controlling model simply has not been - // parsed yet, and evaluating against our own value would hide the field for no reason. + // Nothing to listen to yet, so apply the "controlling model missing" fallback once. Skipped + // when the relation resolves to this field itself - matching against our own value would hide + // it while the real controlling model is still unparsed. this.evaluateRelations(model, control); } - // The controlling model (e.g. `edm_type` for `dc.language.iso=>edm.type`) may only be registered - // by a later modelFromConfiguration() call - a form section is parsed at a time, and the - // `submit.type-bind.field` property itself arrives asynchronously. Until then this field either - // has no controlling model at all or is temporarily attached to the default one, so keep - // listening and attach the real one as soon as it shows up. + // The controlling model (e.g. `edm_type`) may only be registered by a later + // modelFromConfiguration() call, so attach to it as soon as it shows up. subscriptions.add(this.formBuilderService.getTypeBindModelUpdates().subscribe(() => { attachRelatedModels(this.getRelatedFormModel(model)); })); diff --git a/src/app/shared/form/builder/form-builder.service.ts b/src/app/shared/form/builder/form-builder.service.ts index 6e5e2b43668..a3dc3626a57 100644 --- a/src/app/shared/form/builder/form-builder.service.ts +++ b/src/app/shared/form/builder/form-builder.service.ts @@ -65,19 +65,17 @@ import { FormFieldMetadataValueObject } from './models/form-field-metadata-value import { RowParser } from './parsers/row-parser'; /** - * The key under which the default type bind field is stored in the type field map, e.g. - * {'default' -> 'dc_type'} + * Key of the default entry in the type field map, e.g. {'default' -> 'dc_type'} */ export const TYPE_BIND_DEFAULT_KEY = 'default'; /** - * Separator used by the `submit.type-bind.field` property to bind one metadata field to a - * controlling field other than the default one, e.g. `dc.language.iso=>edm.type` + * Separator in `submit.type-bind.field`, e.g. `dc.language.iso=>edm.type` */ const TYPE_BIND_FIELD_SEPARATOR = '=>'; /** - * Model id of the controlling field used when `submit.type-bind.field` declares no default + * Controlling model id used when `submit.type-bind.field` declares no default */ const TYPE_BIND_DEFAULT_MODEL_ID = 'dc_type'; @@ -85,7 +83,7 @@ const TYPE_BIND_DEFAULT_MODEL_ID = 'dc_type'; export class FormBuilderService extends DynamicFormService { /** - * This map contains the models that control type binding, keyed by model id (`dc_type`, `edm_type`) + * The models that control type binding, keyed by model id (`dc_type`, `edm_type`) */ private typeBindModel: Map; @@ -105,30 +103,23 @@ export class FormBuilderService extends DynamicFormService { private formGroups: Map; /** - * The submission the registered type bind models belong to. A submission is parsed section by - * section and a controlling field may live in another section than the field it controls, so the - * registry survives across `modelFromConfiguration` calls - but only within one submission, - * otherwise a model of the previously opened collection's form would keep answering lookups. + * The submission {@link typeBindModel} belongs to; the registry is scoped to it */ private typeBindModelSubmissionId: string; /** - * The parsed rows of the current submission, kept so that controlling models can still be - * registered when the `submit.type-bind.field` property arrives after the form was parsed. - * Dropped - and no longer filled - as soon as that property has been processed. + * Rows parsed before `submit.type-bind.field` arrived, re-scanned once it does */ private typeBindParsedRows: DynamicFormControlModel[][]; /** - * Whether the `submit.type-bind.field` property has been processed (successfully or not), i.e. - * whether {@link typeFields} can still change + * Whether `submit.type-bind.field` has been processed, i.e. whether {@link typeFields} can still change */ private typeBindConfigLoaded: boolean; /** - * The fields to use for type binding: TYPE_BIND_DEFAULT_KEY -> the default controlling model id, - * plus one entry per metadata field that is controlled by another field, e.g. - * `dc.language.iso` -> `edm_type` + * The fields to use for type binding: TYPE_BIND_DEFAULT_KEY -> default controlling model id, plus + * one entry per bound field, e.g. `dc.language.iso` -> `edm_type` */ private typeFields: Map; @@ -147,7 +138,7 @@ export class FormBuilderService extends DynamicFormService { this.typeBindModelUpdates = new Subject(); this.typeFields.set(TYPE_BIND_DEFAULT_KEY, TYPE_BIND_DEFAULT_MODEL_ID); - // Without a config service the type field map can never change, so nothing has to be re-scanned + // without a config service the type field map can never change this.typeBindConfigLoaded = hasNoValue(this.configService); // If optional config service was passed, perform an initial set of type field (default dc_type) for type binds if (hasValue(this.configService)) { @@ -165,12 +156,11 @@ export class FormBuilderService extends DynamicFormService { } /** - * Get the model of the field that controls the type binding of a bound field. + * Get the model of the field controlling the type binding of a bound field, falling back to the + * default one when the target isn't part of the current form. * - * @param typeBindFieldRef either the metadata name of the bound field itself - resolved through the - * `submit.type-bind.field` map, e.g. `dc.language.iso` -> `edm_type` - or, when the submission form - * declares ``, the id of the controlling model itself. When it resolves - * to a model that is not part of the current form, the default (usually `dc_type`) model is returned. + * @param typeBindFieldRef the bound field's own metadata name (mapped through + * `submit.type-bind.field`), or the controlling model id when `` is set */ getTypeBindModel(typeBindFieldRef?: string): DynamicFormControlModel | undefined { return this.typeBindModel.get(this.resolveTypeBindModelId(typeBindFieldRef)) @@ -178,17 +168,15 @@ export class FormBuilderService extends DynamicFormService { } /** - * Resolve which model id a type bind reference points at, purely from configuration - unlike - * {@link getTypeBindModel} this does not fall back to the default model when the target is not - * (yet) part of the form, so the answer does not depend on parse order. + * Which model id a type bind reference points at, from configuration only. Unlike + * {@link getTypeBindModel} the answer doesn't depend on what has been parsed so far. */ resolveTypeBindModelId(typeBindFieldRef?: string): string { return this.typeFields.get(typeBindFieldRef) ?? typeBindFieldRef ?? this.getDefaultTypeBindModelId(); } /** - * The id of the model of the default controlling field. Never undefined: the constructor seeds the - * map and {@link setTypeBindFieldFromConfig} restores the entry whatever the configuration says. + * Model id of the default controlling field */ private getDefaultTypeBindModelId(): string { return this.typeFields.get(TYPE_BIND_DEFAULT_KEY) ?? TYPE_BIND_DEFAULT_MODEL_ID; @@ -203,9 +191,8 @@ export class FormBuilderService extends DynamicFormService { } /** - * Emits the id of every type bind model as soon as it is registered, so that fields whose - * controlling model is only parsed later - e.g. because it lives in another form section - can - * still attach to it. + * Emits the id of every type bind model as it is registered, so fields whose controlling model is + * parsed later can still attach to it. */ getTypeBindModelUpdates(): Observable { return this.typeBindModelUpdates.asObservable(); @@ -416,9 +403,7 @@ export class FormBuilderService extends DynamicFormService { this.setTypeBindModel(typeBindModel); } else { if (!this.typeBindConfigLoaded) { - // Only needed until submit.type-bind.field has been processed; after that every controlling - // field id is known at parse time, so there is nothing left to re-scan and holding on to the - // model graphs of re-parsed sections would just grow without bound. + // bounded on purpose: once the property is in, every controlling id is known at parse time this.typeBindParsedRows.push(rows); } this.registerTypeBindModels(this.getTypeBindModelIds(rawData), rows); @@ -427,11 +412,8 @@ export class FormBuilderService extends DynamicFormService { } /** - * Drop the type bind models registered by another submission. Sections of the same submission are - * parsed one by one and a field can be controlled by a field in a different section, so the - * registry must survive within a submission - but a model of the previously opened collection's - * form must not keep answering lookups, which would defeat the "controlling field is not part of - * this form, fall back to the default" behaviour for the rest of the session. + * Drop the models registered by another submission. The registry has to survive across sections of + * one submission (a controlling field may live in another section), but not across submissions. */ private resetTypeBindModelsOnSubmissionChange(submissionId: string): void { if (this.typeBindModelSubmissionId !== submissionId) { @@ -454,18 +436,15 @@ export class FormBuilderService extends DynamicFormService { } /** - * Collect the ids of every model that can control type binding for the given form configuration: - * all values of the `submit.type-bind.field` map (the default field plus each `A=>B` override) and - * every `` declared by a field of this configuration. The latter is read - * straight from the REST payload, so a controlling model is registered even when the configuration - * property has not been fetched yet. + * Ids of every model that can control type binding here: the `submit.type-bind.field` values plus + * every `` of this configuration, read straight from the REST payload so + * they are known even before that property arrives. */ private getTypeBindModelIds(rawData: any): string[] { const ids = new Set(this.typeFields.values()); const collectFromRows = (formRows: FormRowModel[]): void => { (formRows || []).forEach((formRow: FormRowModel) => (formRow?.fields || []).forEach((field: FormFieldModel) => { - // trim exactly like FieldParser.getTypeBindFieldRef does, otherwise a padded value in the - // XML would register ' edm_type ' while the relations point at 'edm_type' + // trim like FieldParser.getTypeBindFieldRef, or a padded value registers ' edm_type ' const typeBindField = field?.typeBindField?.trim(); if (isNotEmpty(typeBindField)) { ids.add(typeBindField.replace(/\./g, '_')); @@ -668,19 +647,16 @@ export class FormBuilderService extends DynamicFormService { } /** - * Get the type bind field(s) from config. - * - * `submit.type-bind.field` holds the default controlling field and, optionally, one - * `=>` entry per field that is controlled by another field, e.g. - * `submit.type-bind.field = dc.type, dc.language.iso=>edm.type`. The property may legitimately be - * declared more than once (dspace.cfg + local.cfg), so duplicated values must be tolerated and the - * order of the values must not matter. + * Get the type bind field(s) from config, e.g. + * `submit.type-bind.field = dc.type, dc.language.iso=>edm.type`: the default controlling field plus + * one `=>` entry per overridden field. The property may be declared + * in both dspace.cfg and local.cfg, so duplicates and order must not matter. */ setTypeBindFieldFromConfig(): void { this.configService.findByPropertyName('submit.type-bind.field').pipe( getFirstCompletedRemoteData(), ).subscribe((remoteData: any) => { - // the type field map cannot change any more, whatever the outcome + // whatever the outcome, the type field map cannot change any more this.typeBindConfigLoaded = true; // make sure we got a success response from the backend if (!remoteData.hasSucceeded) { @@ -695,11 +671,11 @@ export class FormBuilderService extends DynamicFormService { return; } if (!typeFieldConfig.includes(TYPE_BIND_FIELD_SEPARATOR)) { - // `dc.type`: the default controlling field. A duplicated value just overwrites itself. + // `dc.type`: the default controlling field this.typeFields.set(TYPE_BIND_DEFAULT_KEY, typeFieldConfig.replace(/\./g, '_')); return; } - // `dc.language.iso=>edm.type`: this metadata field is controlled by another field + // `dc.language.iso=>edm.type`: this field is controlled by another one const parts = typeFieldConfig.split(TYPE_BIND_FIELD_SEPARATOR).map((part: string) => part.trim()); if (parts.length !== 2 || parts.some((part: string) => isEmpty(part))) { console.warn(`Ignoring malformed submit.type-bind.field value "${rawValue}", expected "${TYPE_BIND_FIELD_SEPARATOR}"`); @@ -710,8 +686,7 @@ export class FormBuilderService extends DynamicFormService { if (hasNoValue(this.typeFields.get(TYPE_BIND_DEFAULT_KEY))) { this.typeFields.set(TYPE_BIND_DEFAULT_KEY, TYPE_BIND_DEFAULT_MODEL_ID); } - // Forms parsed before the property arrived could not know about the `A=>B` overrides yet, so - // give their controlling models a second chance to be registered, then drop the cache. + // forms parsed before the property arrived didn't know the `A=>B` overrides yet const typeBindModelIds = Array.from(this.typeFields.values()); this.typeBindParsedRows.forEach((rows: DynamicFormControlModel[]) => this.registerTypeBindModels(typeBindModelIds, rows)); this.typeBindParsedRows = []; diff --git a/src/app/shared/form/builder/models/form-field.model.ts b/src/app/shared/form/builder/models/form-field.model.ts index 7c4fb189fc0..3c6a6830385 100644 --- a/src/app/shared/form/builder/models/form-field.model.ts +++ b/src/app/shared/form/builder/models/form-field.model.ts @@ -133,10 +133,8 @@ export class FormFieldModel { typeBind: string[]; /** - * The metadata field that controls the type binding of this field, coming from the - * `` attribute in submission-forms.xml. Optional: when the attribute - * is absent this field is controlled by the default field configured in the - * `submit.type-bind.field` property, so readers have to handle `undefined`. + * The field controlling the type binding of this field, from ``. + * Undefined when the attribute is absent: the `submit.type-bind.field` default applies then. */ @autoserialize typeBindField?: string; diff --git a/src/app/shared/form/builder/parsers/field-parser.ts b/src/app/shared/form/builder/parsers/field-parser.ts index 48140d75032..e26e2ccd07e 100644 --- a/src/app/shared/form/builder/parsers/field-parser.ts +++ b/src/app/shared/form/builder/parsers/field-parser.ts @@ -291,19 +291,15 @@ export abstract class FieldParser { } /** - * Resolve the field that controls the type binding of this field. - * - * When submission-forms.xml declares `` the controlling model is known - * up front, so its model id is returned directly. Otherwise this field's own metadata name is - * returned and {@link FormBuilderService#getTypeBindModel} resolves it against the - * `submit.type-bind.field` property (e.g. `dc.type, dc.language.iso=>edm.type`) at - * relation-evaluation time - that property is fetched asynchronously and may not have arrived yet - * while the form is being parsed. + * The field controlling the type binding of this field: the model id when submission-forms.xml + * declares ``, otherwise this field's own metadata name, which + * {@link FormBuilderService#getTypeBindModel} maps through `submit.type-bind.field` later - that + * property is fetched asynchronously and may not have arrived while the form is parsed. */ protected getTypeBindFieldRef(): string { const typeBindField = this.configData.typeBindField?.trim(); if (isNotEmpty(typeBindField)) { - // input ids don't allow dots, so replace them - this is already a model id + // input ids don't allow dots return typeBindField.replace(/\./g, '_'); } return this.getFieldId() || this.parserOptions.typeField; diff --git a/src/app/shared/mocks/form-builder-service.mock.ts b/src/app/shared/mocks/form-builder-service.mock.ts index 2b71d63448b..ce60600f2ce 100644 --- a/src/app/shared/mocks/form-builder-service.mock.ts +++ b/src/app/shared/mocks/form-builder-service.mock.ts @@ -50,7 +50,7 @@ export function getMockFormBuilderService(): FormBuilderService { resolveTypeBindModelId: undefined, }); - // mirror the real implementation for a reference that the type field map does not remap + // as the real implementation behaves for a reference the type field map does not remap formBuilderService.resolveTypeBindModelId.and.callFake((typeBindFieldRef?: string) => typeBindFieldRef ?? 'dc_type'); return formBuilderService; From d210218eddcdc2063adeb5baaaa5175223a1ac85 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Thu, 6 Aug 2026 09:15:20 +0200 Subject: [PATCH 13/13] Shorten the type bind spec comments Co-Authored-By: Claude Opus 5 (1M context) --- .../ds-dynamic-type-bind-relation.service.spec.ts | 9 +++------ src/app/shared/form/builder/form-builder.service.spec.ts | 3 +-- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts index f6f0ea38f73..1ac2b497024 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts @@ -155,8 +155,7 @@ describe('DSDynamicTypeBindRelationService test suite', () => { const testModel = mockInputWithTypeBindModel; testModel.typeBindRelations = getTypeBindRelations(['boundType'], 'edm_type'); const dcTypeControl = new UntypedFormControl(); - // the caller (ds-dynamic-form-control-container) spreads the result into its own array, so a - // subscription created later has to hang off something handed over now to ever be torn down + // the caller spreads the result into its own array, so later children must hang off this one const [subscription] = service.subscribeRelations(testModel, dcTypeControl); const controllingModel = new DsDynamicInputModel(dcTypeInputConfig); @@ -192,8 +191,7 @@ describe('DSDynamicTypeBindRelationService test suite', () => { }); it('Should not hide a field whose controlling model has not been parsed yet, and attach when it is', () => { - // the real target (edm_type) is not registered, so getTypeBindModel falls back to the default - // model - which in this form happens to BE this field. That is not a misconfiguration. + // edm_type is not registered yet, so the fallback default model happens to be this field itself const bindModelUpdates = new Subject(); const formBuilderServiceSpy: any = (service as any).formBuilderService; formBuilderServiceSpy.getTypeBindModelUpdates.and.returnValue(bindModelUpdates.asObservable()); @@ -218,8 +216,7 @@ describe('DSDynamicTypeBindRelationService test suite', () => { }); it('Should attach the real controlling model even when it was first bound to the default one', () => { - // until edm_type is registered, getTypeBindModel falls back to the default dc_type model, so - // a related model IS attached - the late registration must still be picked up + // dc_type is attached as the fallback, so the late edm_type registration must still be picked up const bindModelUpdates = new Subject(); const formBuilderServiceSpy: any = (service as any).formBuilderService; formBuilderServiceSpy.getTypeBindModelUpdates.and.returnValue(bindModelUpdates.asObservable()); diff --git a/src/app/shared/form/builder/form-builder.service.spec.ts b/src/app/shared/form/builder/form-builder.service.spec.ts index 4e6e0cbbbd3..41b14d6c33d 100644 --- a/src/app/shared/form/builder/form-builder.service.spec.ts +++ b/src/app/shared/form/builder/form-builder.service.spec.ts @@ -1103,8 +1103,7 @@ describe('FormBuilderService per-field type bind with a slow configuration respo }); it('should register a controlling model declared only by a padded ', () => { - // there is no A=>B entry for this field, so the whitespace-padded XML attribute is the ONLY - // source of the controlling model id - it has to be trimmed the same way the relation ids are + // no A=>B entry here, so the padded XML attribute is the only source of the controlling model id configResponse.next(createSuccessfulRemoteDataObject({ ... new ConfigurationProperty(), name: 'submit.type-bind.field',