Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Warn about <2024.1 entities spec and update tests #1272

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Warn about <2024.1 entities spec and update tests #1272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion lib/data/dataset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,14 @@ const getDataset = (xml) =>
else if (!semverSatisfies(version.get(), '2022.1.0 - 2024.1.x'))
throw Problem.user.invalidEntityForm({ reason: `Entities specification version [${version.get()}] is not supported.` });

let warnings;
if (semverSatisfies(version.get(), '<2024.1.x'))
warnings = {
Comment on lines +85 to +87
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's nice to avoid a let where possible. How about const + a ternary?

const warnings = semverSatisfies(version.get(), '>=2024.1.x')
  ? null
  : {
    type: 'oldEntityVersion',
    // ...
  };

Comment on lines +85 to +87
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name warnings in the plural makes me think that it should be an array. Just a thought, I also wouldn't mind leaving it as-is until we have a second warning.

type: 'oldEntityVersion',
details: { version: version.get() },
reason: `Entities specification version [${version.get()}] is not compatible with Offline Entities. Please use version 2024.1.0 or later.`
};

const strippedAttrs = Object.create(null);
for (const [name, value] of Object.entries(entityAttrs.get()))
strippedAttrs[stripNamespacesFromPath(name)] = value;
Expand All @@ -101,7 +109,7 @@ const getDataset = (xml) =>
if (actions.length === 0)
throw Problem.user.invalidEntityForm({ reason: 'The form must specify at least one entity action, for example, create or update.' });

return Option.of({ name: datasetName, actions });
return Option.of({ name: datasetName, actions, warnings });
});

module.exports = { getDataset, validateDatasetName, validatePropertyName };
13 changes: 12 additions & 1 deletion lib/model/query/forms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ const createNew = (partial, project) => async ({ Actees, Datasets, FormAttachmen

// Check for xmlFormId collisions with previously deleted forms
await Forms.checkDeletedForms(partial.xmlFormId, project.id);
await Forms.checkDatasetWarnings(parsedDataset);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that the lines above and below this one use await, but if Forms.checkDatasetWarnings() doesn't need to be async and return a promise, I think it'd be nice to remove the await. I think code is often clearer when things can be done sync.

await Forms.rejectIfWarnings();

// Provision Actee for form
Expand Down Expand Up @@ -761,6 +762,16 @@ const checkDeletedForms = (xmlFormId, projectId) => ({ maybeOne, context }) => (
}
}));

const checkDatasetWarnings = (dataset) => ({ context }) => {
if (context.query.ignoreWarnings) return resolve();

if (dataset.isDefined() && dataset.get().warnings != null) {
if (!context.transitoryData.has('workflowWarnings')) context.transitoryData.set('workflowWarnings', []);
context.transitoryData.get('workflowWarnings').push(dataset.get().warnings);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you make warnings an array:

Suggested change
context.transitoryData.get('workflowWarnings').push(dataset.get().warnings);
context.transitoryData.get('workflowWarnings').push(...dataset.get().warnings);

}
return resolve();
};

const checkStructuralChange = (existingFields, fields) => ({ context }) => {
if (context.query.ignoreWarnings) return resolve();

Expand Down Expand Up @@ -829,7 +840,7 @@ module.exports = {
getByProjectId, getByProjectAndXmlFormId, getByProjectAndNumericId,
getAllByAuth,
getFields, getBinaryFields, getStructuralFields, getMergedFields,
rejectIfWarnings, checkMeta, checkDeletedForms, checkStructuralChange, checkFieldDowncast,
rejectIfWarnings, checkMeta, checkDeletedForms, checkStructuralChange, checkFieldDowncast, checkDatasetWarnings,
_newSchema,
lockDefs, getAllSubmitters
};
Expand Down
49 changes: 48 additions & 1 deletion test/data/xml.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,30 @@ module.exports = {
</h:html>`,

simpleEntity: `<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2024.1.0">
<instance>
<data id="simpleEntity" orx:version="1.0">
<name/>
<age/>
<hometown/>
<meta>
<entity dataset="people" id="" create="">
<label/>
</entity>
</meta>
</data>
</instance>
<bind nodeset="/data/name" type="string" entities:saveto="first_name"/>
<bind nodeset="/data/age" type="int" entities:saveto="age"/>
<bind nodeset="/data/hometown" type="string"/>
</model>
</h:head>
</h:html>`,

// Copy of the above form with the original entities-version
simpleEntity2022: `<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2022.1.0">
Expand All @@ -371,7 +395,7 @@ module.exports = {
multiPropertyEntity: `<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2022.1.0">
<model entities:entities-version="2024.1.0">
<instance>
<data id="multiPropertyEntity" orx:version="1.0">
<q1/>
Expand All @@ -394,6 +418,29 @@ module.exports = {
</h:html>`,

updateEntity: `<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2024.1.0">
<instance>
<data id="updateEntity" orx:version="1.0">
<name/>
<age/>
<hometown/>
<meta>
<entity dataset="people" id="" update="" baseVersion="" branchId="" trunkVersion="">
<label/>
</entity>
</meta>
</data>
</instance>
<bind nodeset="/data/name" type="string" entities:saveto="first_name"/>
<bind nodeset="/data/age" type="int" entities:saveto="age"/>
</model>
</h:head>
</h:html>`,

// Copy of the above form with the original entities-version spec
updateEntity2023: `<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2023.1.0">
Expand Down
95 changes: 71 additions & 24 deletions test/integration/api/datasets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2259,15 +2259,15 @@ describe('datasets and entities', () => {
const updateForm = `<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2023.1.0">
<model entities:entities-version="2024.1.0">
<instance>
<data id="updateEntity" orx:version="1.0">
<person/>
<name/>
<age/>
<hometown/>
<meta>
<entity dataset="people" id="" update="" baseVersion="">
<entity dataset="people" id="" update="" baseVersion="" trunkVersion="" branchId="">
<label/>
</entity>
</meta>
Expand Down Expand Up @@ -2347,15 +2347,15 @@ describe('datasets and entities', () => {
const differentDataset = `<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2023.1.0">
<model entities:entities-version="2024.1.0">
<instance>
<data id="updateEntity" orx:version="1.0">
<person/>
<name/>
<age/>
<hometown/>
<meta>
<entity dataset="students" id="" update="" baseVersion="">
<entity dataset="students" id="" update="" baseVersion="" trunkVersion="" branchId="">
<label/>
</entity>
</meta>
Expand Down Expand Up @@ -2490,7 +2490,7 @@ describe('datasets and entities', () => {
const noDataset = `<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2023.1.0">
<model entities:entities-version="2024.1.0">
<instance>
<data id="updateEntity" orx:version="1.0">
<person/>
Expand Down Expand Up @@ -2533,15 +2533,15 @@ describe('datasets and entities', () => {
const caseChange = `<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2023.1.0">
<model entities:entities-version="2024.1.0">
<instance>
<data id="updateEntity" orx:version="1.0">
<person/>
<name/>
<age/>
<hometown/>
<meta>
<entity dataset="people" id="" update="" baseVersion="">
<entity dataset="people" id="" update="" baseVersion="" trunkVersion="" branchId="">
<label/>
</entity>
</meta>
Expand Down Expand Up @@ -2583,7 +2583,7 @@ describe('datasets and entities', () => {
const caseChange = `<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2023.1.0">
<model entities:entities-version="2024.1.0">
<instance>
<data id="updateEntity" orx:version="1.0">
<person/>
Expand Down Expand Up @@ -3442,10 +3442,57 @@ describe('datasets and entities', () => {

describe('parsing datasets on form upload', () => {
describe('parsing datasets at /projects/:id/forms POST', () => {

describe('warnings about entities-version from before 2024.1.0', () => {
it('should warn if the entities-version is 2022.1.0 (earlier than 2024.1.0)', testService(async (service) => {
const asAlice = await service.login('alice');

await asAlice.post('/v1/projects/1/forms')
.send(testData.forms.simpleEntity2022)
.set('Content-Type', 'application/xml')
.expect(400)
.then(({ body }) => {
body.code.should.be.eql(400.16);
body.details.warnings.workflowWarnings[0].should.be.eql({
type: 'oldEntityVersion',
details: { version: '2022.1.0' },
reason: 'Entities specification version [2022.1.0] is not compatible with Offline Entities. Please use version 2024.1.0 or later.'
});
});

await asAlice.post('/v1/projects/1/forms?ignoreWarnings=true')
.send(testData.forms.simpleEntity2022)
.set('Content-Type', 'application/xml')
.expect(200);
}));

it('should warn if the entities-version is 2023.1.0 (earlier than 2024.1.0)', testService(async (service) => {
const asAlice = await service.login('alice');

await asAlice.post('/v1/projects/1/forms')
.send(testData.forms.updateEntity2023)
.set('Content-Type', 'application/xml')
.expect(400)
.then(({ body }) => {
body.code.should.be.eql(400.16);
body.details.warnings.workflowWarnings[0].should.be.eql({
type: 'oldEntityVersion',
details: { version: '2023.1.0' },
reason: 'Entities specification version [2023.1.0] is not compatible with Offline Entities. Please use version 2024.1.0 or later.'
});
});

await asAlice.post('/v1/projects/1/forms?ignoreWarnings=true')
.send(testData.forms.updateEntity)
.set('Content-Type', 'application/xml')
.expect(200);
}));
});

it('should return a Problem if the entity xml has the wrong version', testService((service) =>
service.login('alice', (asAlice) =>
asAlice.post('/v1/projects/1/forms')
.send(testData.forms.simpleEntity.replace('2022.1.0', 'bad-version'))
.send(testData.forms.simpleEntity.replace('2024.1.0', 'bad-version'))
.set('Content-Type', 'text/xml')
.expect(400)
.then(({ body }) => {
Expand Down Expand Up @@ -3509,7 +3556,7 @@ describe('datasets and entities', () => {
const xml = `<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<h:title>nobinds</h:title>
<model entities:entities-version='2022.1.0'>
<model entities:entities-version='2024.1.0'>
<instance>
<data id="nobinds">
<name/>
Expand Down Expand Up @@ -3551,7 +3598,7 @@ describe('datasets and entities', () => {
const alice = await service.login('alice');
const xml = `<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version='2022.1.0'>
<model entities:entities-version='2024.1.0'>
<instance>
<data id="validate_structure">
<name/>
Expand Down Expand Up @@ -3603,7 +3650,7 @@ describe('datasets and entities', () => {
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:entities="http://www.opendatakit.org/xforms/entities" xmlns:ev="http://www.w3.org/2001/xml-events" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:odk="http://www.opendatakit.org/xforms" xmlns:orx="http://openrosa.org/xforms" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
<h:head>
<h:title>Repeat Children Entities</h:title>
<model entities:entities-version="2022.1.0" odk:xforms-version="1.0.0">
<model entities:entities-version="2024.1.0" odk:xforms-version="1.0.0">
<instance>
<data id="repeat_entity" version="2">
<num_children/>
Expand Down Expand Up @@ -4534,12 +4581,12 @@ describe('datasets and entities', () => {
const form = `<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2023.1.0">
<model entities:entities-version="2024.1.0">
<instance>
<data id="brokenForm" orx:version="1.0">
<age foo="bar"/>
<meta>
<entity dataset="people" id="" create="" update="" baseVersion="" />
<entity dataset="people" id="" create="" update="" baseVersion="" trunkVersion="" branchId=""/>
</meta>
</data>
</instance>
Expand Down Expand Up @@ -4599,12 +4646,12 @@ describe('datasets and entities', () => {
const form = `<?xml version="1.0"?>
<h:html xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2023.1.0">
<model entities:entities-version="2024.1.0">
<instance>
<data id="brokenForm" orx:version="1.0">
<age foo="bar"/>
<meta>
<entity dataset="people" id="" create="" update="" baseVersion="" />
<entity dataset="people" id="" create="" update="" baseVersion="" trunkVersion="" branchId="" />
</meta>
</data>
</instance>
Expand All @@ -4616,12 +4663,12 @@ describe('datasets and entities', () => {
const form2 = `<?xml version="1.0"?>
<h:html xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2023.1.0">
<model entities:entities-version="2024.1.0">
<instance>
<data id="brokenForm" orx:version="2.0">
<age foo="bar"/>
<meta>
<entity dataset="people" id="" create="" update="" baseVersion="">
<entity dataset="people" id="" create="" update="" baseVersion="" trunkVersion="" branchId="">
<label/>
</entity>
</meta>
Expand Down Expand Up @@ -4651,12 +4698,12 @@ describe('datasets and entities', () => {
const form = `<?xml version="1.0"?>
<h:html xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2023.1.0">
<model entities:entities-version="2024.1.0">
<instance>
<data id="updateWithoutLabel" orx:version="1.0">
<age foo="bar"/>
<meta>
<entity dataset="people" id="" update="" baseVersion="" />
<entity dataset="people" id="" update="" baseVersion="" trunkVersion="" branchId=""/>
</meta>
</data>
</instance>
Expand All @@ -4674,12 +4721,12 @@ describe('datasets and entities', () => {
const form2 = `<?xml version="1.0"?>
<h:html xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2023.1.0">
<model entities:entities-version="2024.1.0">
<instance>
<data id="updateWithLabel" orx:version="1.0">
<age foo="bar"/>
<meta>
<entity dataset="people" id="" update="" baseVersion="">
<entity dataset="people" id="" update="" baseVersion="" trunkVersion="" branchId="">
<label/>
</entity>
</meta>
Expand Down Expand Up @@ -4720,12 +4767,12 @@ describe('datasets and entities', () => {
const form = `<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2023.1.0">
<model entities:entities-version="2024.1.0">
<instance>
<data id="brokenForm" orx:version="1.0">
<age foo="bar"/>
<meta>
<entity dataset="people" id="" update="" baseVersion="" />
<entity dataset="people" id="" update="" baseVersion="" trunkVersion="" branchId=""/>
</meta>
</data>
</instance>
Expand Down
2 changes: 1 addition & 1 deletion test/integration/other/empty-entity-structure.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const { testService } = require('../setup');
const emptyEntityForm = `<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2023.1.0">
<model entities:entities-version="2024.1.0">
<instance>
<data id="emptyEntity" orx:version="1.0">
<age/>
Expand Down
Loading