diff --git a/.changeset/breezy-sheep-play.md b/.changeset/breezy-sheep-play.md new file mode 100644 index 000000000..8298e7521 --- /dev/null +++ b/.changeset/breezy-sheep-play.md @@ -0,0 +1,5 @@ +--- +'@getodk/xforms-engine': patch +--- + +Fix: reactive aggregation of validation violations from selects diff --git a/packages/xforms-engine/src/instance/SelectField.ts b/packages/xforms-engine/src/instance/SelectField.ts index e31176ef2..b8b7b95b0 100644 --- a/packages/xforms-engine/src/instance/SelectField.ts +++ b/packages/xforms-engine/src/instance/SelectField.ts @@ -128,12 +128,7 @@ export class SelectField } getViolation(): AnyViolation | null { - // Read engine state to ensure reactivity in engine, Solid-based clients - this.validation.engineState.violation; - - // Read/return client state to ensure client reactivity, regardless of - // client's reactive implementation - return this.validationState.violation; + return this.validation.engineState.violation; } protected getSelectItemsByValue( diff --git a/packages/xforms-engine/test/lib/reactivity/createAggregatedViolations.test.ts b/packages/xforms-engine/test/lib/reactivity/createAggregatedViolations.test.ts index 7689089c5..09fdeb5aa 100644 --- a/packages/xforms-engine/test/lib/reactivity/createAggregatedViolations.test.ts +++ b/packages/xforms-engine/test/lib/reactivity/createAggregatedViolations.test.ts @@ -6,9 +6,11 @@ import { head, html, input, + item, label, mainInstance, model, + select1, t, title, } from '@getodk/common/test/fixtures/xform-dsl/index.ts'; @@ -32,11 +34,17 @@ describe('createAggregatedViolations - reactive aggregated `constraint` and `req t('contactdetails', t('residentialAddress'), t('phone', - t('home'))), + t('home'))), + t('colors-outer', + t('colors-inner', + t('color')), + t('other-color')), t('meta', t('instanceID')))), - bind('/data/contactdetails/residentialAddress').required(), - bind('/data/contactdetails/phone/home').required()) + bind('/data/contactdetails/residentialAddress').required(), + bind('/data/contactdetails/phone/home').required(), + bind('/data/colors-outer/colors-inner/color').required(), + bind('/data/colors-outer/other-color').required()) ), body( group( @@ -50,7 +58,23 @@ describe('createAggregatedViolations - reactive aggregated `constraint` and `req label('Phone numbers'), input('/data/contactdetails/phone/home', - label('Home phone no.')))))); + label('Home phone no.')))), + + group('/data/colors-outer', + label('Colors: outer'), + + group('/data/colors-outer/colors-inner', + label('Colors: inner'), + + select1('/data/colors-outer/colors-inner/color', + label('Color'), + item('red', 'Red'), + item('green', 'Green'))), + + select1('/data/colors-outer/other-color', + label('Other color'), + item('purple', 'Purple'), + item('mauve', 'Mauve'))))); }); interface SimplifiedViolation { @@ -70,7 +94,7 @@ describe('createAggregatedViolations - reactive aggregated `constraint` and `req >; describe('check assumptions: ensure observing violation state is effective', () => { - it('observes violations of direct children', async () => { + it('observes violations of a direct string input child', async () => { const observed = await reactiveTestScope(async ({ effect, mutable }) => { const root = await initializeForm(definition.asXml(), { config: { @@ -132,178 +156,430 @@ describe('createAggregatedViolations - reactive aggregated `constraint` and `req ], ]); }); - }); - it('reactively updates when a direct child becomes valid', async () => { - const observed = await reactiveTestScope(async ({ effect, mutable }) => { - const root = await initializeForm(definition.asXml(), { - config: { - stateFactory: mutable, - }, - }); + it('observes violations of a direct select child', async () => { + const observed = await reactiveTestScope(async ({ effect, mutable }) => { + const root = await initializeForm(definition.asXml(), { + config: { + stateFactory: mutable, + }, + }); - const contactDetails = root.currentState.children[0]; - - if ( - contactDetails?.nodeType !== 'group' || - contactDetails.currentState.reference !== '/data/contactdetails' - ) { - throw new Error('Expected group /data/contactdetails'); - } - - const phone = contactDetails.currentState.children[1]; - - if ( - phone?.nodeType !== 'group' || - phone.currentState.reference !== '/data/contactdetails/phone' - ) { - throw new Error('Expected group /data/contactdetails/phone'); - } - - const observedViolations: ObservedViolationReferences = []; - - effect(() => { - observedViolations.push( - phone.validationState.violations.map((violationReference) => { - const violation: SimplifiedViolation = { - condition: violationReference.violation.condition, - valid: violationReference.violation.valid, - message: violationReference.violation.message.asString, - }; - - return { - reference: violationReference.reference, - violation, - }; - }) - ); - }); + const colorsOuter = root.currentState.children[1]; + + if ( + colorsOuter?.nodeType !== 'group' || + colorsOuter.currentState.reference !== '/data/colors-outer' + ) { + throw new Error('Expected group /data/colors-outer'); + } - const home = phone.currentState.children[0]; + const colorsInner = colorsOuter.currentState.children[0]; - if ( - home?.currentState.reference !== '/data/contactdetails/phone/home' || - home.nodeType !== 'string' - ) { - throw new Error('Expected string node /data/contactdetails/phone/home'); - } + if ( + colorsInner?.nodeType !== 'group' || + colorsInner.currentState.reference !== '/data/colors-outer/colors-inner' + ) { + throw new Error('Expected group /data/colors-outer/colors-inner'); + } - // Satisfy `required` condition - home.setValue('555-867-5309'); + const observedViolations: ObservedViolationReferences = []; - return observedViolations; + effect(() => { + observedViolations.push( + colorsInner.validationState.violations.map((violationReference) => { + const violation: SimplifiedViolation = { + condition: violationReference.violation.condition, + valid: violationReference.violation.valid, + message: violationReference.violation.message.asString, + }; + + return { + reference: violationReference.reference, + violation, + }; + }) + ); + }); + + return observedViolations; + }); + + expect(observed).toEqual([ + // Initially invalid + [ + { + reference: '/data/colors-outer/colors-inner/color', + violation: { + condition: 'required', + valid: false, + message: VALIDATION_TEXT.requiredMsg, + }, + }, + ], + ]); }); + }); - expect(observed).toEqual([ - // Initially invalid - [ - { - reference: '/data/contactdetails/phone/home', - violation: { - condition: 'required', - valid: false, - message: VALIDATION_TEXT.requiredMsg, + describe('direct children', () => { + it('reactively updates when a direct string input child becomes valid', async () => { + const observed = await reactiveTestScope(async ({ effect, mutable }) => { + const root = await initializeForm(definition.asXml(), { + config: { + stateFactory: mutable, }, - }, - ], + }); - // Valid after change - [], - ]); - }); + const contactDetails = root.currentState.children[0]; + + if ( + contactDetails?.nodeType !== 'group' || + contactDetails.currentState.reference !== '/data/contactdetails' + ) { + throw new Error('Expected group /data/contactdetails'); + } + + const phone = contactDetails.currentState.children[1]; + + if ( + phone?.nodeType !== 'group' || + phone.currentState.reference !== '/data/contactdetails/phone' + ) { + throw new Error('Expected group /data/contactdetails/phone'); + } + + const observedViolations: ObservedViolationReferences = []; + + effect(() => { + observedViolations.push( + phone.validationState.violations.map((violationReference) => { + const violation: SimplifiedViolation = { + condition: violationReference.violation.condition, + valid: violationReference.violation.valid, + message: violationReference.violation.message.asString, + }; - it('reactively updates when a deeper descendant becomes valid', async () => { - const observed = await reactiveTestScope(async ({ effect, mutable }) => { - const root = await initializeForm(definition.asXml(), { - config: { - stateFactory: mutable, - }, + return { + reference: violationReference.reference, + violation, + }; + }) + ); + }); + + const home = phone.currentState.children[0]; + + if ( + home?.currentState.reference !== '/data/contactdetails/phone/home' || + home.nodeType !== 'string' + ) { + throw new Error('Expected string node /data/contactdetails/phone/home'); + } + + // Satisfy `required` condition + home.setValue('555-867-5309'); + + return observedViolations; }); - const contactDetails = root.currentState.children[0]; - - if ( - contactDetails?.nodeType !== 'group' || - contactDetails.currentState.reference !== '/data/contactdetails' - ) { - throw new Error('Expected group /data/contactdetails'); - } - - const observedViolations: ObservedViolationReferences = []; - - effect(() => { - observedViolations.push( - contactDetails.validationState.violations.map((violationReference) => { - const violation: SimplifiedViolation = { - condition: violationReference.violation.condition, - valid: violationReference.violation.valid, - message: violationReference.violation.message.asString, - }; - - return { - reference: violationReference.reference, - violation, - }; - }) - ); + expect(observed).toEqual([ + // Initially invalid + [ + { + reference: '/data/contactdetails/phone/home', + violation: { + condition: 'required', + valid: false, + message: VALIDATION_TEXT.requiredMsg, + }, + }, + ], + + // Valid after change + [], + ]); + }); + + it('reactively updates when a direct select child becomes valid', async () => { + const observed = await reactiveTestScope(async ({ effect, mutable }) => { + const root = await initializeForm(definition.asXml(), { + config: { + stateFactory: mutable, + }, + }); + + const colorsOuter = root.currentState.children[1]; + + if ( + colorsOuter?.nodeType !== 'group' || + colorsOuter.currentState.reference !== '/data/colors-outer' + ) { + throw new Error('Expected group /data/colors-outer'); + } + + const colorsInner = colorsOuter.currentState.children[0]; + + if ( + colorsInner?.nodeType !== 'group' || + colorsInner.currentState.reference !== '/data/colors-outer/colors-inner' + ) { + throw new Error('Expected group /data/colors-outer/colors-inner'); + } + + const observedViolations: ObservedViolationReferences = []; + + effect(() => { + observedViolations.push( + colorsInner.validationState.violations.map((violationReference) => { + const violation: SimplifiedViolation = { + condition: violationReference.violation.condition, + valid: violationReference.violation.valid, + message: violationReference.violation.message.asString, + }; + + return { + reference: violationReference.reference, + violation, + }; + }) + ); + }); + + const color = colorsInner.currentState.children[0]; + + if ( + color?.nodeType !== 'select' || + color.currentState.reference !== '/data/colors-outer/colors-inner/color' + ) { + throw new Error('Expected select /data/colors-outer/colors-inner/color'); + } + + const [option] = color.currentState.valueOptions; + + if (option == null) { + throw new Error('Cannot set value of select, no options available'); + } + + // Satisfy `required` condition + color.select(option); + + return observedViolations; }); - const phone = contactDetails.currentState.children[1]; + expect(observed).toEqual([ + // Initially invalid + [ + { + reference: '/data/colors-outer/colors-inner/color', + violation: { + condition: 'required', + valid: false, + message: VALIDATION_TEXT.requiredMsg, + }, + }, + ], + + // Valid after change + [], + ]); + }); + }); + + describe('deeper descendants', () => { + it('reactively updates when a deeper string input descendant becomes valid', async () => { + const observed = await reactiveTestScope(async ({ effect, mutable }) => { + const root = await initializeForm(definition.asXml(), { + config: { + stateFactory: mutable, + }, + }); + + const contactDetails = root.currentState.children[0]; + + if ( + contactDetails?.nodeType !== 'group' || + contactDetails.currentState.reference !== '/data/contactdetails' + ) { + throw new Error('Expected group /data/contactdetails'); + } + + const observedViolations: ObservedViolationReferences = []; + + effect(() => { + observedViolations.push( + contactDetails.validationState.violations.map((violationReference) => { + const violation: SimplifiedViolation = { + condition: violationReference.violation.condition, + valid: violationReference.violation.valid, + message: violationReference.violation.message.asString, + }; + + return { + reference: violationReference.reference, + violation, + }; + }) + ); + }); - if ( - phone?.nodeType !== 'group' || - phone.currentState.reference !== '/data/contactdetails/phone' - ) { - throw new Error('Expected group /data/contactdetails/phone'); - } + const phone = contactDetails.currentState.children[1]; - const home = phone.currentState.children[0]; + if ( + phone?.nodeType !== 'group' || + phone.currentState.reference !== '/data/contactdetails/phone' + ) { + throw new Error('Expected group /data/contactdetails/phone'); + } + + const home = phone.currentState.children[0]; + + if ( + home?.currentState.reference !== '/data/contactdetails/phone/home' || + home.nodeType !== 'string' + ) { + throw new Error('Expected string node /data/contactdetails/phone/home'); + } - if ( - home?.currentState.reference !== '/data/contactdetails/phone/home' || - home.nodeType !== 'string' - ) { - throw new Error('Expected string node /data/contactdetails/phone/home'); - } + // Satisfy `required` condition + home.setValue('555-867-5309'); - // Satisfy `required` condition - home.setValue('555-867-5309'); + return observedViolations; + }); - return observedViolations; + expect(observed).toEqual([ + // Both descendants initially invalid + [ + { + reference: '/data/contactdetails/residentialAddress', + violation: { + condition: 'required', + valid: false, + message: VALIDATION_TEXT.requiredMsg, + }, + }, + { + reference: '/data/contactdetails/phone/home', + violation: { + condition: 'required', + valid: false, + message: VALIDATION_TEXT.requiredMsg, + }, + }, + ], + + // Direct child still invalid; deeper descendant valid after change + [ + { + reference: '/data/contactdetails/residentialAddress', + violation: { + condition: 'required', + valid: false, + message: VALIDATION_TEXT.requiredMsg, + }, + }, + ], + ]); }); - expect(observed).toEqual([ - // Both descendants initially invalid - [ - { - reference: '/data/contactdetails/residentialAddress', - violation: { - condition: 'required', - valid: false, - message: VALIDATION_TEXT.requiredMsg, + it('reactively updates when a deeper select descendant becomes valid', async () => { + const observed = await reactiveTestScope(async ({ effect, mutable }) => { + const root = await initializeForm(definition.asXml(), { + config: { + stateFactory: mutable, + }, + }); + + const colorsOuter = root.currentState.children[1]; + + if ( + colorsOuter?.nodeType !== 'group' || + colorsOuter.currentState.reference !== '/data/colors-outer' + ) { + throw new Error('Expected group /data/colors-outer'); + } + + const observedViolations: ObservedViolationReferences = []; + + effect(() => { + observedViolations.push( + colorsOuter.validationState.violations.map((violationReference) => { + const violation: SimplifiedViolation = { + condition: violationReference.violation.condition, + valid: violationReference.violation.valid, + message: violationReference.violation.message.asString, + }; + + return { + reference: violationReference.reference, + violation, + }; + }) + ); + }); + + const colorsInner = colorsOuter.currentState.children[0]; + + if ( + colorsInner?.nodeType !== 'group' || + colorsInner.currentState.reference !== '/data/colors-outer/colors-inner' + ) { + throw new Error('Expected group /data/colors-outer/colors-inner'); + } + + const color = colorsInner.currentState.children[0]; + + if ( + color?.nodeType !== 'select' || + color.currentState.reference !== '/data/colors-outer/colors-inner/color' + ) { + throw new Error('Expected select /data/colors-outer/colors-inner/color'); + } + + const [option] = color.currentState.valueOptions; + + if (option == null) { + throw new Error('Cannot set value of select, no options available'); + } + + // Satisfy `required` condition + color.select(option); + + return observedViolations; + }); + + expect(observed).toEqual([ + // Both descendants initially invalid + [ + { + reference: '/data/colors-outer/colors-inner/color', + violation: { + condition: 'required', + valid: false, + message: VALIDATION_TEXT.requiredMsg, + }, }, - }, - { - reference: '/data/contactdetails/phone/home', - violation: { - condition: 'required', - valid: false, - message: VALIDATION_TEXT.requiredMsg, + { + reference: '/data/colors-outer/other-color', + violation: { + condition: 'required', + valid: false, + message: VALIDATION_TEXT.requiredMsg, + }, }, - }, - ], - - // Direct child still invalid; deeper descendant valid after change - [ - { - reference: '/data/contactdetails/residentialAddress', - violation: { - condition: 'required', - valid: false, - message: VALIDATION_TEXT.requiredMsg, + ], + + // Direct child still invalid; deeper descendant valid after change + [ + { + reference: '/data/colors-outer/other-color', + violation: { + condition: 'required', + valid: false, + message: VALIDATION_TEXT.requiredMsg, + }, }, - }, - ], - ]); + ], + ]); + }); }); });