responding to code review

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)

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')

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;
 
 ////////////////////////////////////////////////////////////////////////////////

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' });
   }
 };
 

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);
 

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(`<?xml version="1.0"?>
@@ -1152,6 +1178,60 @@ testMigration('20240914-02-remove-orphaned-client-audits.js', () => {
 </h:html>`);
         });
 
+      // Draft form looks good, shows version 3.0[upgrade]
+      await asAlice.get('/v1/projects/1/forms/updateEntity/draft.xml')
+        .then(({ text }) => {
+          text.should.equal(`<?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="3.0[upgrade]">
+          <name/>
+          <age/>
+          <hometown/>
+          <meta>
+            <entity dataset="people" id="" update="" baseVersion="" trunkVersion="" branchId="">
+              <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>`);
+        });
+
+      // 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(`<?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="updateEntity2" orx:version="1.0[upgrade]">
+          <name/>
+          <age/>
+          <hometown/>
+          <meta>
+            <entity dataset="people" id="" update="" baseVersion="" trunkVersion="" branchId="">
+              <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>`);
+        });
+
+      // 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));
     }));