Remove use of conditional publish from Forms.createNew (#1143)

* Remove use of conditional publish from Forms.createNew

* Remove publish flag from _createNew query

* Make createNew and createVersion share more utils

* move enketo draft token stuff into createVersion

* Change some variable names and problem wording

* Improving enketo tests a little bit

* add enketo.reset()

* code review fixes

* Sorting out audit order and details

* last fixes from code review

lib/model/query/datasets.js

@@ -43,7 +43,7 @@ const _insertDatasetDef = (dataset, acteeId, actions) => sql`
 
 const _insertProperties = (fields) => sql`
 ,fields("propertyName", "formDefId", "schemaId", path) AS (VALUES      
-  ${sql.join(fields.map(p => sql`( ${sql.join([p.aux.propertyName, p.aux.formDefId, p.schemaId, p.path], sql`,`)} )`), sql`,`)}
+  ${sql.join(fields.map(p => sql`( ${sql.join([p.aux.propertyName, p.aux.formDefId, p.aux.schemaId, p.path], sql`,`)} )`), sql`,`)}
 ),
 insert_properties AS (
     INSERT INTO ds_properties (id, name, "datasetId")
@@ -82,6 +82,7 @@ SELECT "action", "id" FROM ds
 // * information about the dataset parsed from the form XML
 // * form (a Form frame or object containing projectId, formDefId, and schemaId)
 // * array of field objects and property names that were parsed from the form xml
+//   (form fields do not need any extra IDs of the form, form def, or schema.)
 const createOrMerge = (parsedDataset, form, fields) => async ({ one, Actees, Datasets, Projects }) => {
   const { projectId } = form;
   const { id: formDefId, schemaId } = form.def;

lib/resources/forms.js

@@ -113,7 +113,10 @@ module.exports = (service, endpoint) => {
       .then(getOrNotFound)
       .then((project) => auth.canOrReject('form.create', project))
       .then((project) => getPartial(Forms, request, project, Keys)
-        .then((partial) => Forms.createNew(partial, project, isTrue(query.publish))))));
+        .then((partial) => Forms.createNew(partial, project))
+        .then((newForm) => (isTrue(query.publish)
+          ? Forms.publish(newForm, true)
+          : newForm)))));
 
   // can POST empty body to copy the current def to draft.
   service.post('/projects/:projectId/forms/:id/draft', endpoint(({ Forms, Keys, Projects, Submissions }, { params, auth }, request) =>

lib/util/db.js

@@ -184,14 +184,15 @@ const insertMany = (objs) => {
 };
 
 // generic update utility
+// will set updatedAt if it exists, as long as it is not overwritten in the data
 const updater = (obj, data, whereKey = 'id') => {
   const keys = Object.keys(data);
   if (keys.length === 0) return sql`select true`;
   const assigner = _assign(data);
   return sql`
 update ${raw(obj.constructor.table)}
 set ${sql.join(keys.map((k) => sql`${sql.identifier([ k ])}=${assigner(k)}`), sql`,`)}
-${obj.constructor.hasUpdatedAt ? sql`,"updatedAt"=clock_timestamp()` : nothing}
+${(obj.constructor.hasUpdatedAt && !keys.includes('updatedAt')) ? sql`,"updatedAt"=clock_timestamp()` : nothing}
 where ${sql.identifier([ whereKey ])}=${obj[whereKey]}
 returning *`;
 };

lib/util/problem.js

@@ -190,7 +190,7 @@ const problems = {
 
     // special version of uniquenessViolation to be more informative about the publish-version
     // conflict case.
-    versionUniquenessViolation: problem(409.6, ({ xmlFormId, version }) => `You tried to publish the form '${xmlFormId}' with version '${version}', but a published form has already existed in this project with those identifiers. Please choose a new name and try again. You can re-request with ?version=xyz to have the version changed in-place for you.`),
+    versionUniquenessViolation: problem(409.6, ({ xmlFormId, version }) => `You tried to publish the form '${xmlFormId}' with version '${version}', but a published form has already existed in this project with those identifiers.`),
 
     // tried to reuse an instanceId
     instanceIdConflict: problem(409.8, () => 'You tried to update a submission, but the instanceID you provided already exists on this server. Please try again with a unique instanceID.'),

test/integration/api/audits.js

@@ -134,7 +134,7 @@ describe('/audits', () => {
               audits[1].action.should.equal('form.update.publish');
               audits[1].acteeId.should.equal(form.acteeId);
               audits[1].actee.should.eql(plain(form.forApi()));
-              audits[1].details.should.eql({ newDefId: form.currentDefId });
+              audits[1].details.should.eql({ newDefId: form.currentDefId, oldDefId: null });
               audits[1].loggedAt.should.be.a.recentIsoDate();
 
               audits[2].actorId.should.equal(alice.actor.id);
@@ -637,8 +637,8 @@ describe('/audits', () => {
           body.length.should.equal(4);
           body.map(a => a.action).should.eql([
             'form.update.publish',
-            'form.create',
             'dataset.create',
+            'form.create',
             'user.session.create'
           ]);
         });

test/integration/api/datasets.js

@@ -2214,10 +2214,7 @@ describe('datasets and entities', () => {
           </h:head>
         </h:html>`;
 
-        it('should NOT autolink on upload new form and simultaneously publish', testService(async (service) => {
-          // this path of directly publishing a form on upload isn't possible in central
-          // so it's not going to be supported. if it were, logic would go around line #170 in
-          // lib/model/query/forms.js in Forms.createNew after the dataset is published.
+        it('should autolink on upload new form and simultaneously publish', testService(async (service) => {
           const asAlice = await service.login('alice');
 
           await asAlice.post('/v1/projects/1/forms?publish=true')
@@ -2228,7 +2225,7 @@ describe('datasets and entities', () => {
           await asAlice.get('/v1/projects/1/forms/updateEntity/attachments')
             .then(({ body }) => {
               body[0].name.should.equal('people.csv');
-              body[0].datasetExists.should.be.false();
+              body[0].datasetExists.should.be.true();
             });
         }));
 
@@ -3939,6 +3936,38 @@ describe('datasets and entities', () => {
         audit.should.equal(Option.none());
       }));
 
+      it('should log appropriate sequence of form and dataset events', testService(async (service) => {
+
+        const asAlice = await service.login('alice');
+
+        await asAlice.post('/v1/projects/1/forms?publish=true')
+          .send(testData.forms.simpleEntity)
+          .set('Content-Type', 'text/xml')
+          .expect(200);
+
+        await asAlice.post('/v1/projects/1/forms/simpleEntity/draft')
+          .set('Content-Type', 'application/xml')
+          .send(testData.forms.simpleEntity.replace('orx:version="1.0"', 'orx:version="draft1"').replace(/first_name/g, 'nickname'))
+          .expect(200);
+
+        await asAlice.post('/v1/projects/1/forms/simpleEntity/draft/publish');
+
+        await asAlice.get('/v1/audits?action=nonverbose')
+          .expect(200)
+          .then(({ body }) => {
+            body.length.should.equal(7);
+            body.map(a => a.action).should.eql([
+              'form.update.publish',
+              'dataset.update',
+              'form.update.draft.set',
+              'form.update.publish',
+              'dataset.create',
+              'form.create',
+              'user.session.create'
+            ]);
+          });
+      }));
+
       it('should not log dataset modification when no new property is added', testService(async (service, { Audits }) => {
         const asAlice = await service.login('alice');
 
@@ -3958,7 +3987,7 @@ describe('datasets and entities', () => {
         audit.should.equal(Option.none());
       }));
 
-      it('should log dataset publishing in audit log', testService(async (service, { Audits }) => {
+      it('should log dataset publishing with properties in audit log', testService(async (service, { Audits }) => {
 
         const asAlice = await service.login('alice');
 

test/integration/api/forms/draft.js

@@ -3,6 +3,8 @@ const appRoot = require('app-root-path');
 const should = require('should');
 const { testService } = require('../../setup');
 const testData = require('../../../data/xml');
+const { Form } = require(appRoot + '/lib/model/frames');
+
 const { exhaust } = require(appRoot + '/lib/worker/worker');
 const { sql } = require('slonik');
 
@@ -1263,9 +1265,9 @@ describe('api: /projects/:id/forms (drafts)', () => {
           .expect(200);
         should.not.exist(beforeWorker.enketoId);
         should.not.exist(beforeWorker.enketoOnceId);
+        global.enketo.callCount.should.equal(2);
 
         // Second request, from the worker
-        global.enketo.callCount.should.equal(2);
         await exhaust(container);
         global.enketo.callCount.should.equal(3);
         global.enketo.receivedUrl.should.equal(`${container.env.domain}/v1/projects/1`);
@@ -1328,6 +1330,68 @@ describe('api: /projects/:id/forms (drafts)', () => {
                     body[1].details.newDraftDefId.should.equal(form.currentDefId);
                   });
               })))));
+
+      it('should log the correct def ids in the form audit log', testService(async (service, { Forms }) => {
+        const asAlice = await service.login('alice');
+
+        await asAlice.post('/v1/projects/1/forms?publish=true')
+          .send(testData.forms.simple2)
+          .set('Content-Type', 'text/xml')
+          .expect(200);
+
+        const formT1 = await Forms.getByProjectAndXmlFormId(1, 'simple2').then((o) => o.get());
+
+        await asAlice.post('/v1/projects/1/forms/simple2/draft')
+          .set('Content-Type', 'application/xml')
+          .send(testData.forms.simple2.replace('version="2.1"', 'version="2.2"'))
+          .expect(200);
+
+        const formT2 = await Forms.getByProjectAndXmlFormId(1, 'simple2', false, Form.DraftVersion).then((o) => o.get());
+
+        await asAlice.post('/v1/projects/1/forms/simple2/draft')
+          .set('Content-Type', 'application/xml')
+          .send(testData.forms.simple2.replace('version="2.1"', 'version="2.3"'))
+          .expect(200);
+
+        const formT3 = await Forms.getByProjectAndXmlFormId(1, 'simple2', false, Form.DraftVersion).then((o) => o.get());
+
+        await asAlice.post('/v1/projects/1/forms/simple2/draft/publish');
+
+        const formT4 = await Forms.getByProjectAndXmlFormId(1, 'simple2').then((o) => o.get());
+
+        await asAlice.get('/v1/audits?action=nonverbose')
+          .expect(200)
+          .then(({ body }) => {
+            // first form event: form create
+            body[4].action.should.equal('form.create');
+
+            // second form event: form publish
+            body[3].action.should.equal('form.update.publish');
+            // new def id should match form def at time T1
+            body[3].details.should.eql({ newDefId: formT1.def.id, oldDefId: null });
+
+            // third form event: draft
+            body[2].action.should.equal('form.update.draft.set');
+            body[2].details.should.eql({ newDraftDefId: formT2.def.id, oldDraftDefId: null });
+
+            // forth form event: draft
+            body[1].action.should.equal('form.update.draft.set');
+            body[1].details.should.eql({ newDraftDefId: formT3.def.id, oldDraftDefId: formT2.def.id });
+
+            // fifth form event: publish
+            body[0].action.should.equal('form.update.publish');
+            body[0].details.should.eql({ newDefId: formT4.def.id, oldDefId: formT1.def.id });
+
+            body.map(a => a.action).should.eql([
+              'form.update.publish',
+              'form.update.draft.set',
+              'form.update.draft.set',
+              'form.update.publish',
+              'form.create',
+              'user.session.create'
+            ]);
+          });
+      }));
     });
 
     describe('/fields GET', () => {

test/integration/api/forms/forms.js

@@ -74,8 +74,8 @@ describe('api: /projects/:id/forms (create, read, update)', () => {
             .set('Content-Type', 'application/xml')
             .expect(409)
             .then(({ body }) => {
-              body.details.fields.should.eql([ 'projectId', 'xmlFormId', 'version' ]);
-              body.details.values.should.eql([ '1', 'simple', '' ]);
+              body.message.should.eql("You tried to publish the form 'simple' with version '', but a published form has already existed in this project with those identifiers.");
+              body.details.should.eql({ xmlFormId: 'simple', version: '' });
             })))));
 
     it('should return the created form upon success', testService((service) =>
@@ -295,7 +295,35 @@ describe('api: /projects/:id/forms (create, read, update)', () => {
             .expect(200)
             .then(({ body }) => {
               body.publishedAt.should.be.a.recentIsoDate();
-            })))));
+              body.publishedAt.should.eql(body.createdAt);
+              (body.updatedAt == null).should.equal(true);
+            })
+            .then(() => asAlice.get('/v1/audits?action=form')
+              .expect(200)
+              .then(({ body }) => {
+                body.map((a) => a.action).should.eql(['form.update.publish', 'form.create']);
+              }))))));
+
+    it('should have published timestamp different from create timestamp if published separately', testService((service) =>
+      service.login('alice', (asAlice) =>
+        asAlice.post('/v1/projects/1/forms')
+          .send(testData.forms.simple2)
+          .set('Content-Type', 'application/xml')
+          .expect(200)
+          .then(() => asAlice.post('/v1/projects/1/forms/simple2/draft/publish')
+            .expect(200))
+          .then(() => asAlice.get('/v1/projects/1/forms/simple2')
+            .expect(200)
+            .then(({ body }) => {
+              body.publishedAt.should.be.a.recentIsoDate();
+              body.publishedAt.should.not.eql(body.createdAt);
+              body.updatedAt.should.be.a.recentIsoDate();
+            })
+            .then(() => asAlice.get('/v1/audits?action=form')
+              .expect(200)
+              .then(({ body }) => {
+                body.map((a) => a.action).should.eql(['form.update.publish', 'form.create']);
+              }))))));
 
     describe('Enketo ID for draft', () => {
       it('should request an enketoId', testService(async (service, { env }) => {
@@ -380,7 +408,8 @@ describe('api: /projects/:id/forms (create, read, update)', () => {
           .set('Content-Type', 'application/xml')
           .send(testData.forms.simple2)
           .expect(200);
-        global.enketo.callCount.should.equal(1);
+        // This will make a published enketo token and a draft token even though the draft is not used
+        global.enketo.callCount.should.equal(2);
         without(['token'], global.enketo.createData).should.eql({
           openRosaUrl: `${env.domain}/v1/projects/1`,
           xmlFormId: 'simple2'
@@ -392,6 +421,7 @@ describe('api: /projects/:id/forms (create, read, update)', () => {
       it('should return with success even if request to Enketo fails', testService(async (service) => {
         const asAlice = await service.login('alice');
         global.enketo.state = 'error';
+        global.enketo.autoReset = false;
         const { body } = await asAlice.post('/v1/projects/1/forms?publish=true')
           .set('Content-Type', 'application/xml')
           .send(testData.forms.simple2)
@@ -403,6 +433,7 @@ describe('api: /projects/:id/forms (create, read, update)', () => {
       it('should wait for Enketo only briefly @slow', testService(async (service) => {
         const asAlice = await service.login('alice');
         global.enketo.wait = (done) => { setTimeout(done, 600); };
+        global.enketo.autoReset = false;
         const { body } = await asAlice.post('/v1/projects/1/forms?publish=true')
           .set('Content-Type', 'application/xml')
           .send(testData.forms.simple2)
@@ -411,20 +442,36 @@ describe('api: /projects/:id/forms (create, read, update)', () => {
         should.not.exist(body.enketoOnceId);
       }));
 
+      it('should wait for published Enketo only briefly @slow', testService(async (service) => {
+        const asAlice = await service.login('alice');
+        await asAlice.post('/v1/projects/1/forms')
+          .set('Content-Type', 'application/xml')
+          .send(testData.forms.simple2)
+          .expect(200);
+        global.enketo.wait = (done) => { setTimeout(done, 600); };
+        const { body } = await asAlice.post('/v1/projects/1/forms/simple2/draft/publish');
+        should.not.exist(body.enketoId);
+        should.not.exist(body.enketoOnceId);
+      }));
+
       it('should request Enketo IDs from worker if request from endpoint fails', testService(async (service, container) => {
         const asAlice = await service.login('alice');
 
         // First request to Enketo, from the endpoint
         global.enketo.state = 'error';
+        global.enketo.autoReset = false;
         await asAlice.post('/v1/projects/1/forms?publish=true')
           .set('Content-Type', 'application/xml')
           .send(testData.forms.simple2)
           .expect(200);
 
+        // reset enketo mock to its default behavior
+        // also resets callCount
+        global.enketo.reset();
+
         // Second request, from the worker
-        global.enketo.callCount.should.equal(1);
         await exhaust(container);
-        global.enketo.callCount.should.equal(2);
+        global.enketo.callCount.should.equal(1);
         const { body } = await asAlice.get('/v1/projects/1/forms/simple2')
           .expect(200);
         without(['token'], global.enketo.createData).should.eql({
@@ -441,9 +488,9 @@ describe('api: /projects/:id/forms (create, read, update)', () => {
           .set('Content-Type', 'application/xml')
           .send(testData.forms.simple2)
           .expect(200);
-        global.enketo.callCount.should.equal(1);
+        global.enketo.callCount.should.equal(2);
         await exhaust(container);
-        global.enketo.callCount.should.equal(1);
+        global.enketo.callCount.should.equal(2);
       }));
     });
 

test/integration/fixtures/02-forms.js

@@ -6,18 +6,25 @@ const forms = [ simple, withrepeat ];
 module.exports = async ({ Assignments, Forms, Projects, Roles }) => {
   const project = (await Projects.getById(1)).get();
   const { id: formview } = (await Roles.getBySystemName('formview')).get();
+
+  // Create the forms without Enketo IDs in order to maintain existing tests.
+  global.enketo.state = 'error';
+  global.enketo.autoReset = false;
+
   /* eslint-disable no-await-in-loop */
   for (const xml of forms) {
     const partial = await Form.fromXml(xml);
-    // Create the form without Enketo IDs in order to maintain existing tests.
-    global.enketo.state = 'error';
-    const { acteeId } = await Forms.createNew(partial, project, true);
+    const form = await Forms.createNew(partial, project);
+    await Forms.publish(form, true);
 
     // Delete the assignment of the formview actor created by Forms.createNew()
     // in order to maintain existing tests.
-    const [{ actorId }] = await Assignments.getByActeeAndRoleId(acteeId, formview);
+    const [{ actorId }] = await Assignments.getByActeeAndRoleId(form.acteeId, formview);
     await Assignments.revokeByActorId(actorId);
   }
   /* eslint-enable no-await-in-loop */
+
+  // Reset enketo to not affect tests
+  global.enketo.reset();
 };