From 34af7e660e291f8c3bd6736ab557efd342558829 Mon Sep 17 00:00:00 2001 From: eyelidlessness Date: Tue, 7 Jun 2022 15:34:40 -0700 Subject: [PATCH] Fix: setgeopoint actions correctly identify their affected nodeset Current behavior: 1. Actions defined in the model, or in the body *adjacent* its input, populate the model correctly. 2. Actions nested within their input, with an explicit `ref` attribute, fail to update the view. This is because there is a hidden HTML input representing the action (distinct from the input representing the model value), which the current behavior selects. 3. Actions nested within their input, *without* an explicit `ref` attribute, replace the entire descendant tree of the model with the geopoint value. The cause is identical to the explicit-ref case, but the erroneously selected input has no `name` attribute. This causes the model update, in `Nodeset.setVal`, to evaluate against a nodeset resolving to the root `data` node. --- src/js/calculate.js | 38 ++++++-- test/forms/setgeopoint.xml | 33 +++++-- test/spec/setgeopoint.spec.js | 176 ++++++++++++++++++---------------- 3 files changed, 153 insertions(+), 94 deletions(-) diff --git a/src/js/calculate.js b/src/js/calculate.js index ff6ca8d4f..7955451f7 100644 --- a/src/js/calculate.js +++ b/src/js/calculate.js @@ -204,14 +204,38 @@ export default { const nodes = this._getNodesForAction(action, event); nodes.forEach((actionControl) => { - const name = this.form.input.getName(actionControl); - const dataNodesObj = this.form.model.node(name); + let name = this.form.input.getName(actionControl); + + let affectedControl; + + if ( + name == null || + name === '' || + actionControl.matches( + `[name="${CSS.escape(name)}"] ~ [name="${CSS.escape( + name + )}"]` + ) + ) { + affectedControl = actionControl + .closest('.question') + ?.querySelector( + ':is([data-name], [name]):not([data-name=""], [data-event], [name=""])' + ); + + name = this.form.input.getName(affectedControl); + } else { + affectedControl = actionControl; + } + + // TODO: should this pass `{ onlyLeaf: true }` option? + const dataNodesObj = this.form.model.node(name, null); const dataNodes = dataNodesObj.getElements(); const props = { name, - dataType: this.form.input.getXmlType(actionControl), - relevantExpr: this.form.input.getRelevant(actionControl), + dataType: this.form.input.getXmlType(affectedControl), + relevantExpr: this.form.input.getRelevant(affectedControl), index: event.detail && typeof event.detail.repeatIndex !== 'undefined' @@ -222,7 +246,7 @@ export default { }; if (action === 'setvalue') { - props.expr = actionControl.dataset.setvalue; + props.expr = affectedControl.dataset.setvalue; } if ( @@ -238,7 +262,7 @@ export default { */ dataNodes.forEach((el, index) => { const obj = Object.create(props); - const control = actionControl; + const control = affectedControl; obj.index = index; this._updateCalc(control, obj); }); @@ -253,7 +277,7 @@ export default { } this._updateCalc(control, props); } else if (dataNodes[props.index]) { - const control = actionControl; + const control = affectedControl; this._updateCalc(control, props); } else { console.error( diff --git a/test/forms/setgeopoint.xml b/test/forms/setgeopoint.xml index 04201d7f0..077cfa8af 100644 --- a/test/forms/setgeopoint.xml +++ b/test/forms/setgeopoint.xml @@ -12,7 +12,10 @@ - + + + + @@ -22,18 +25,36 @@ - + + + + - + + - - - + + + + + + + + + + + + + + + + + diff --git a/test/spec/setgeopoint.spec.js b/test/spec/setgeopoint.spec.js index 5374c8ef7..3d4c638aa 100644 --- a/test/spec/setgeopoint.spec.js +++ b/test/spec/setgeopoint.spec.js @@ -12,6 +12,18 @@ import events from '../../src/js/event'; */ describe('setgeopoint action', () => { + const actionInBodyNodes = { + visible_first_load_adjacent_action: 'action defined adjacent to input', + visible_first_load_nested_explicit_ref: + 'action nested in input with explicit ref', + visible_first_load_nested_implied_ref: + 'action nested in input with ref determined by parent', + }; + const firstLoadeNodes = { + ...actionInBodyNodes, + visible_first_load_model_action: 'action defined in model', + }; + describe('first load', () => { const mock = mockGetCurrentPosition( createTestCoordinates({ @@ -37,94 +49,96 @@ describe('setgeopoint action', () => { .then(done, done); }); - it('works for questions with odk-instance-first-load inside of the XForms body', (done) => { - const form1 = loadForm('setgeopoint.xml'); - form1.init(); - - mock.lookup - .then(({ geopoint }) => { - expect( - form1.model.xml.querySelector('visible_first_load') - .textContent - ).to.equal(geopoint); - }) - .then(done, done); - }); + for (const [nodeName, description] of Object.entries( + actionInBodyNodes + )) { + it(`works for questions with odk-instance-first-load inside of the XForms body (${description})`, (done) => { + const form1 = loadForm('setgeopoint.xml'); + form1.init(); + + mock.lookup + .then(({ geopoint }) => { + expect( + form1.model.xml.querySelector(nodeName).textContent + ).to.equal(geopoint); + }) + .then(done, done); + }); + } }); - describe('null `accuracy`', () => { - const mock = mockGetCurrentPosition( - createTestCoordinates({ - latitude: 48.66, - longitude: -120.5, - altitude: 123, - }), - { expectLookup: true } - ); - - it('substitutes a null `accuracy` value with 0.0', (done) => { - const form1 = loadForm('setgeopoint.xml'); - form1.init(); - - mock.lookup - .then(({ geopoint }) => { - expect( - form1.model.xml.querySelector('visible_first_load') - .textContent - ).to.equal(geopoint); - expect(geopoint).to.equal('48.66 -120.5 123 0.0'); - }) - .then(done, done); + for (const [nodeName, description] of Object.entries(firstLoadeNodes)) { + describe(`null 'accuracy' (${description})`, () => { + const mock = mockGetCurrentPosition( + createTestCoordinates({ + latitude: 48.66, + longitude: -120.5, + altitude: 123, + }), + { expectLookup: true } + ); + + it('substitutes a null `accuracy` value with 0.0', (done) => { + const form1 = loadForm('setgeopoint.xml'); + form1.init(); + + mock.lookup + .then(({ geopoint }) => { + expect( + form1.model.xml.querySelector(nodeName).textContent + ).to.equal(geopoint); + expect(geopoint).to.equal('48.66 -120.5 123 0.0'); + }) + .then(done, done); + }); }); - }); - describe('null `altitude`', () => { - const mock = mockGetCurrentPosition( - createTestCoordinates({ - latitude: 48.66, - longitude: -120.5, - accuracy: 2500.12, - }), - { expectLookup: true } - ); - - it('substitutes a null `altitude` value with 0.0', (done) => { - const form1 = loadForm('setgeopoint.xml'); - form1.init(); - - mock.lookup - .then(({ geopoint }) => { - expect( - form1.model.xml.querySelector('visible_first_load') - .textContent - ).to.equal(geopoint); - expect(geopoint).to.include('48.66 -120.5 0.0 2500.12'); - }) - .then(done, done); + describe(`null 'altitude' (${description})`, () => { + const mock = mockGetCurrentPosition( + createTestCoordinates({ + latitude: 48.66, + longitude: -120.5, + accuracy: 2500.12, + }), + { expectLookup: true } + ); + + it('substitutes a null `altitude` value with 0.0', (done) => { + const form1 = loadForm('setgeopoint.xml'); + form1.init(); + + mock.lookup + .then(({ geopoint }) => { + expect( + form1.model.xml.querySelector(nodeName).textContent + ).to.equal(geopoint); + expect(geopoint).to.include('48.66 -120.5 0.0 2500.12'); + }) + .then(done, done); + }); }); - }); - - describe('lookup failure', () => { - const mock = mockGetCurrentPosition( - createGeolocationLookupError('PERMISSION_DENIED'), - { expectLookup: true } - ); - - it('sets an empty string when lookup fails', (done) => { - const form1 = loadForm('setgeopoint.xml'); - form1.init(); - mock.lookup - .then(({ geopoint }) => { - expect( - form1.model.xml.querySelector('visible_first_load') - .textContent - ).to.equal(geopoint); - expect(geopoint).to.include(''); - }) - .then(done, done); + describe(`lookup failure (${description})`, () => { + const mock = mockGetCurrentPosition( + createGeolocationLookupError('PERMISSION_DENIED'), + { expectLookup: true } + ); + + it('sets an empty string when lookup fails', (done) => { + const form1 = loadForm('setgeopoint.xml'); + form1.init(); + + mock.lookup + .then(({ geopoint }) => { + expect( + form1.model.xml.querySelector(nodeName).textContent + ).to.equal(geopoint); + expect(geopoint).to.include(''); + }) + .then(done, done); + }); }); - }); + } }); describe('setgeopoint actions to populate a value if another value changes', () => {