From c5590afa28d25333680389f68bbe4d1251df6ea4 Mon Sep 17 00:00:00 2001 From: Kathleen Tuite Date: Wed, 16 Oct 2024 12:38:07 -0700 Subject: [PATCH] responding to code review --- lib/data/schema.js | 16 +++- lib/model/query/audits.js | 2 +- lib/model/query/forms.js | 7 +- lib/worker/form.js | 10 +-- .../other/form-entities-version.js | 33 +++++++- test/integration/other/migrations.js | 82 ++++++++++++++++++- 6 files changed, 134 insertions(+), 16 deletions(-) diff --git a/lib/data/schema.js b/lib/data/schema.js index 237f10545..17659c1e3 100644 --- a/lib/data/schema.js +++ b/lib/data/schema.js @@ -582,6 +582,7 @@ const _versionSplicer = (replace) => (xml, insert) => new Promise((pass, fail) = const addVersionSuffix = _versionSplicer(false); const setVersion = _versionSplicer(true); +// The following helper functions are for a form migration described in issue c#692. // Forms with entity spec version 2023.1.0 that support entity updates need to // be updated to spec version 2024.1.0 and have `branchId` and `trunkVersion` // included alongside the existing `baseVersion`. @@ -599,8 +600,10 @@ const _addBranchIdAndTrunkVersion = (xml) => new Promise((pass, fail) => { onclosetag: () => { stack.pop(); }, - // this isn't a valid xforms xml file? - onend: () => fail(Problem.user.unparseable({ format: 'xforms xml', rawLength: xml.length })) + // If the entity tag can't be found (it should be found in the forms this will run on) + // or there is another xml parsing problem, just fail here. This error will be caught below + // by updateEntityForm. + onend: () => fail(Problem.internal.unknown()) }, { xmlMode: true, decodeEntities: true }); parser.write(xml); @@ -625,14 +628,19 @@ const _updateEntityVersion = (xml, oldVersion, newVersion) => new Promise((pass, onclosetag: () => { stack.pop(); }, - // this isn't a valid xforms xml file? - onend: () => fail(Problem.user.unparseable({ format: 'xforms xml', rawLength: xml.length })) + // If the entities-version attribute can't be found or there is another + // xml parsing problem, just fail here. This error will be caught below + // by updateEntityForm. + onend: () => fail(Problem.internal.unknown()) }, { xmlMode: true, decodeEntities: true }); parser.write(xml); parser.end(); }); +// If there are any problems with updating the XML, this will just +// return the unaltered XML which will then be a clue for the worker +// to not change anything about the Form. const updateEntityForm = (xml, oldVersion, newVersion, suffix) => _updateEntityVersion(xml, oldVersion, newVersion) .then(_addBranchIdAndTrunkVersion) diff --git a/lib/model/query/audits.js b/lib/model/query/audits.js index 2aa47e0d0..e5661fe74 100644 --- a/lib/model/query/audits.js +++ b/lib/model/query/audits.js @@ -46,7 +46,7 @@ const actionCondition = (action) => { else if (action === 'project') return sql`action in ('project.create', 'project.update', 'project.delete')`; else if (action === 'form') - return sql`action in ('form.create', 'form.update', 'form.delete', 'form.restore', 'form.purge', 'form.attachment.update', 'form.submission.export', 'form.update.draft.set', 'form.update.draft.delete', 'form.update.publish')`; + return sql`action in ('form.create', 'form.update', 'form.delete', 'form.restore', 'form.purge', 'form.attachment.update', 'form.submission.export', 'form.update.draft.set', 'form.update.draft.delete', 'form.update.draft.replace', 'form.update.publish', 'upgrade.process.form.entities_version')`; else if (action === 'submission') return sql`action in ('submission.create', 'submission.update', 'submission.update.version', 'submission.attachment.update', 'submission.reprocess', 'submission.delete', 'submission.restore', 'submission.purge')`; else if (action === 'dataset') diff --git a/lib/model/query/forms.js b/lib/model/query/forms.js index 5bc524c44..943fa1111 100644 --- a/lib/model/query/forms.js +++ b/lib/model/query/forms.js @@ -296,15 +296,16 @@ createVersion.audit.logEvenIfAnonymous = true; // This is used in the rare case where we want to change and update a FormDef in place without // creating a new def. This is basically a wrapper around _updateDef that also logs an event. -const replaceDef = (partial, form) => async ({ Forms }) => { +// eslint-disable-next-line no-unused-vars +const replaceDef = (partial, form, details) => async ({ Forms }) => { const { version, hash, sha, sha256 } = partial.def; await Forms._updateDef(form.def, { xml: partial.xml, version, hash, sha, sha256 }); // all this does is changed updatedAt await Forms._update(form, { updatedAt: (new Date()).toISOString() }); }; -replaceDef.audit = (_, form) => (log) => - log('form.update.draft.replace', form, { upgrade: 'Updated entities-version in form to 2024.1' }); +replaceDef.audit = (_, form, details) => (log) => + log('form.update.draft.replace', form, details); replaceDef.audit.logEvenIfAnonymous = true; //////////////////////////////////////////////////////////////////////////////// diff --git a/lib/worker/form.js b/lib/worker/form.js index 277c408c7..02bdd3af2 100644 --- a/lib/worker/form.js +++ b/lib/worker/form.js @@ -54,20 +54,20 @@ const _upgradeEntityVersion = async (form) => { const updateEntitiesVersion = async ({ Forms }, event) => { const { projectId, xmlFormId } = await Forms.getByActeeIdForUpdate(event.acteeId).then(o => o.get()); - const publishedVersion = await Forms.getByProjectAndXmlFormId(projectId, xmlFormId, true, Form.PublishedVersion).then(o => o.orNull()); - if (publishedVersion && publishedVersion.currentDefId != null) { + const publishedVersion = await Forms.getByProjectAndXmlFormId(projectId, xmlFormId, true, Form.PublishedVersion).then(o => o.get()); + if (publishedVersion.currentDefId != null) { const partial = await _upgradeEntityVersion(publishedVersion); if (partial != null) { await Forms.createVersion(partial, publishedVersion, true, true); } } - const draftVersion = await Forms.getByProjectAndXmlFormId(projectId, xmlFormId, true, Form.DraftVersion).then(o => o.orNull()); - if (draftVersion && draftVersion.draftDefId != null) { + const draftVersion = await Forms.getByProjectAndXmlFormId(projectId, xmlFormId, true, Form.DraftVersion).then(o => o.get()); + if (draftVersion.draftDefId != null) { const partial = await _upgradeEntityVersion(draftVersion); // update xml and version in place if (partial != null) - await Forms.replaceDef(partial, draftVersion); + await Forms.replaceDef(partial, draftVersion, { upgrade: 'Updated entities-version in form draft to 2024.1' }); } }; diff --git a/test/integration/other/form-entities-version.js b/test/integration/other/form-entities-version.js index 65b9fd82e..eeae5a613 100644 --- a/test/integration/other/form-entities-version.js +++ b/test/integration/other/form-entities-version.js @@ -441,6 +441,32 @@ describe('Update / migrate entities-version within form', () => { body.updatedAt.should.be.a.recentIsoDate(); }); })); + + it('should set publishedBy to null on the form when updating a published form', testService(async (service, container) => { + const { Forms, Audits } = container; + const asAlice = await service.login('alice'); + + // Upload a form and publish it + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(testData.forms.updateEntity) + .set('Content-Type', 'application/xml') + .expect(200); + + const { acteeId } = await Forms.getByProjectAndXmlFormId(1, 'updateEntity').then(o => o.get()); + await Audits.log(null, 'upgrade.process.form.entities_version', { acteeId }); + + // Run form upgrade + await exhaust(container); + + // publishedBy is null on the latest version of the form + await asAlice.get('/v1/projects/1/forms/updateEntity/versions') + .set('X-Extended-Metadata', 'true') + .expect(200) + .then(({ body }) => { + should(body[0].publishedBy).be.null(); + body[1].publishedBy.should.be.an.Actor(); + }); + })); }); describe('audit logging and errors', () => { @@ -501,6 +527,8 @@ describe('Update / migrate entities-version within form', () => { 'form.create', 'user.session.create' ]); + + body[0].details.should.eql({ upgrade: 'Updated entities-version in form draft to 2024.1' }); }); })); @@ -536,6 +564,8 @@ describe('Update / migrate entities-version within form', () => { 'form.create', 'user.session.create' ]); + + body[0].details.should.eql({ upgrade: 'Updated entities-version in form draft to 2024.1' }); }); })); @@ -604,9 +634,8 @@ describe('Update / migrate entities-version within form', () => { const asAlice = await service.login('alice'); // Publish an XML form of the right version - const invalidForm = testData.forms.offlineEntity; await asAlice.post('/v1/projects/1/forms?publish=true') - .send(invalidForm) + .send(testData.forms.offlineEntity) .set('Content-Type', 'application/xml') .expect(200); diff --git a/test/integration/other/migrations.js b/test/integration/other/migrations.js index fa1db847d..f7aacb384 100644 --- a/test/integration/other/migrations.js +++ b/test/integration/other/migrations.js @@ -1120,13 +1120,39 @@ testMigration('20240914-02-remove-orphaned-client-audits.js', () => { // Mark forms for upgrade await up(); - // Check: 2 forms that need upgradin and 1 2024.1 form that it wont harm to run through the worker + // There should be a total of 3 forms flagged for upgrade: + // The two 2023.1 forms that need to be migrated, and one newer 2024.1 form. + // The latter form will get processed by the worker but it will realize there is + // no work to do so it wont change anything. const audits = await container.oneFirst(sql`select count(*) from audits where action = 'upgrade.process.form.entities_version'`); audits.should.equal(3); // Run upgrade await exhaust(container); + // Check the audit log + await asAlice.get('/v1/audits') + .expect(200) + .then(({ body }) => { + const actions = body.map(a => a.action); + + // worker may not always process forms in the same order + actions.slice(0, 3).should.eqlInAnyOrder([ + 'form.update.draft.replace', // updateEntity2 draft only + 'form.update.draft.replace', // updateEntity draft + 'form.update.publish', // updateEntity published version + ]); + + // Three upgrade events will always be present, though + actions.slice(3, 6).should.eql([ + 'upgrade.process.form.entities_version', + 'upgrade.process.form.entities_version', + 'upgrade.process.form.entities_version' + ]); + }); + + // First form that was upgraded: updateEntity + // Published form looks good, shows version 2.0[upgrade] await asAlice.get('/v1/projects/1/forms/updateEntity.xml') .then(({ text }) => { text.should.equal(` @@ -1152,6 +1178,60 @@ testMigration('20240914-02-remove-orphaned-client-audits.js', () => { `); }); + // Draft form looks good, shows version 3.0[upgrade] + await asAlice.get('/v1/projects/1/forms/updateEntity/draft.xml') + .then(({ text }) => { + text.should.equal(` + + + + + + + + + + + + + + + + + + +`); + }); + + // Second form that was updated with only a draft version + // Draft form XML looks good + await asAlice.get('/v1/projects/1/forms/updateEntity2/draft.xml') + .then(({ text }) => { + text.should.equal(` + + + + + + + + + + + + + + + + + + +`); + }); + + // Third form was already at 2024.1 version so it did not change await asAlice.get('/v1/projects/1/forms/offlineEntity.xml') .then(({ text }) => text.should.equal(testData.forms.offlineEntity)); }));