diff --git a/lib/model/query/forms.js b/lib/model/query/forms.js index 86cf7cacb..82d35c599 100644 --- a/lib/model/query/forms.js +++ b/lib/model/query/forms.js @@ -108,7 +108,7 @@ select id from form`) (publish === true) ? undefined : Form.DraftVersion)) .then((option) => option.get()); -const createNew = (partial, project, publish = false) => async ({ run, Actees, Datasets, FormAttachments, Forms, Keys }) => { +const createNew = (partial, project) => async ({ run, Actees, Datasets, FormAttachments, Forms, Keys }) => { // Check encryption keys before proceeding const defData = {}; defData.keyId = await partial.aux.key.map(Keys.ensure).orElse(resolve(null)); @@ -131,26 +131,18 @@ const createNew = (partial, project, publish = false) => async ({ run, Actees, D // We will try to push to Enketo. If doing so fails or is too slow, then the // worker will try again later. - if (publish !== true) { - defData.draftToken = generateToken(); - defData.enketoId = await Forms.pushDraftToEnketo( - { projectId: project.id, xmlFormId: partial.xmlFormId, draftToken: defData.draftToken }, - 0.5 - ); - } else { - const enketoIds = await Forms.pushFormToEnketo( - { projectId: project.id, xmlFormId: partial.xmlFormId, acteeId: formData.acteeId }, - 0.5 - ); - Object.assign(formData, enketoIds); - } + defData.draftToken = generateToken(); + defData.enketoId = await Forms.pushDraftToEnketo( + { projectId: project.id, xmlFormId: partial.xmlFormId, draftToken: defData.draftToken }, + 0.5 + ); - // Save form + // Save draft form const savedForm = await Forms._createNew( partial.with(formData), partial.def.with(defData), project, - publish + false // never create with publish=true flag (to be removed soon) ); // Prepare the form fields @@ -165,13 +157,8 @@ const createNew = (partial, project, publish = false) => async ({ run, Actees, D // Update datasets and properties if defined if (parsedDataset.isDefined()) { await Datasets.createOrMerge(parsedDataset.get(), savedForm, fields); - - if (publish) { - await Datasets.publishIfExists(savedForm.def.id, savedForm.def.publishedAt.toISOString()); - } } - // Return the final saved form return savedForm; }; @@ -319,18 +306,21 @@ createVersion.audit.withResult = true; // TODO: we need to make more explicit what .def actually represents throughout. // for now, enforce an extra check here just in case. -const publish = (form) => async ({ Forms, Datasets, FormAttachments }) => { +const publish = (form, sameAsCreate = false) => async ({ Forms, Datasets, FormAttachments }) => { if (form.draftDefId !== form.def.id) throw Problem.internal.unknown(); + // dont use publish sameAsCreate on already published forms + if (sameAsCreate && form.publishedAt != null) throw Problem.internal.unknown({ error: 'Attempting to immediately publish a form' }); + // Try to push the form to Enketo if it hasn't been pushed already. If doing // so fails or is too slow, then the worker will try again later. const enketoIds = form.enketoId == null || form.enketoOnceId == null ? await Forms.pushFormToEnketo(form, 0.5) : {}; - const publishedAt = (new Date()).toISOString(); - await Promise.all([ - Forms._update(form, { currentDefId: form.draftDefId, draftDefId: null, ...enketoIds }), + const publishedAt = sameAsCreate ? form.createdAt.toISOString() : (new Date()).toISOString(); + const [f, fd] = await Promise.all([ + Forms._update(form, { currentDefId: form.draftDefId, draftDefId: null, ...(sameAsCreate ? { updatedAt: null } : {}), ...enketoIds }), Forms._updateDef(form.def, { draftToken: null, enketoId: null, publishedAt }), ]) .catch(Problem.translate( @@ -350,6 +340,8 @@ const publish = (form) => async ({ Forms, Datasets, FormAttachments }) => { await FormAttachments.update(form, attachment.get(), null, dataset.id); } } + + return new Form(f, { def: new Form.Def(fd) }); }; publish.audit = (form) => (log) => log('form.update.publish', form, { oldDefId: form.currentDefId, newDefId: form.draftDefId }); diff --git a/lib/resources/forms.js b/lib/resources/forms.js index 9ac60a898..fc2c0f913 100644 --- a/lib/resources/forms.js +++ b/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) => diff --git a/lib/util/db.js b/lib/util/db.js index dcbf9e8c7..2bd29fb35 100644 --- a/lib/util/db.js +++ b/lib/util/db.js @@ -184,6 +184,7 @@ 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`; @@ -191,7 +192,7 @@ const updater = (obj, data, whereKey = 'id') => { 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 *`; }; diff --git a/test/integration/api/audits.js b/test/integration/api/audits.js index ebeddb473..fee14f084 100644 --- a/test/integration/api/audits.js +++ b/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); @@ -636,9 +636,9 @@ describe('/audits', () => { .then(({ body }) => { body.length.should.equal(4); body.map(a => a.action).should.eql([ + 'dataset.create', 'form.update.publish', 'form.create', - 'dataset.create', 'user.session.create' ]); }); diff --git a/test/integration/api/datasets.js b/test/integration/api/datasets.js index 8d678359d..a6f5e6a7f 100644 --- a/test/integration/api/datasets.js +++ b/test/integration/api/datasets.js @@ -2214,10 +2214,7 @@ describe('datasets and entities', () => { `; - 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(); }); })); diff --git a/test/integration/api/forms/draft.js b/test/integration/api/forms/draft.js index a54c81558..60e341c74 100644 --- a/test/integration/api/forms/draft.js +++ b/test/integration/api/forms/draft.js @@ -137,6 +137,8 @@ describe('api: /projects/:id/forms (drafts)', () => { global.enketo.state = 'error'; await asAlice.post('/v1/projects/1/forms/simple/draft').expect(200); global.enketo.callCount.should.equal(1); + // Reset enketo error + global.enketo.state = undefined; global.enketo.enketoId = '::ijklmnop'; await exhaust(container); global.enketo.callCount.should.equal(2); @@ -1156,6 +1158,7 @@ describe('api: /projects/:id/forms (drafts)', () => { should.not.exist(draft.enketoOnceId); // Publish. + global.enketo.state = undefined; await asAlice.post('/v1/projects/1/forms/simple2/draft/publish') .expect(200); global.enketo.callCount.should.equal(2); @@ -1213,6 +1216,7 @@ describe('api: /projects/:id/forms (drafts)', () => { should.not.exist(v1.enketoOnceId); // Republish + global.enketo.state = undefined; await asAlice.post('/v1/projects/1/forms/simple2/draft').expect(200); global.enketo.callCount.should.equal(3); await asAlice.post('/v1/projects/1/forms/simple2/draft/publish?version=new') @@ -1265,6 +1269,7 @@ describe('api: /projects/:id/forms (drafts)', () => { should.not.exist(beforeWorker.enketoOnceId); // Second request, from the worker + global.enketo.state = undefined; global.enketo.callCount.should.equal(2); await exhaust(container); global.enketo.callCount.should.equal(3); diff --git a/test/integration/api/forms/forms.js b/test/integration/api/forms/forms.js index ef1f3fab2..2c0384093 100644 --- a/test/integration/api/forms/forms.js +++ b/test/integration/api/forms/forms.js @@ -74,8 +74,12 @@ 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', '' ]); + // eslint-disable-next-line no-multi-str + 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. \ +Please choose a new name and try again. You can re-request with ?version=xyz \ +to have the version changed in-place for you."); + body.details.should.eql({ xmlFormId: 'simple', version: '' }); }))))); it('should return the created form upon success', testService((service) => @@ -295,7 +299,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') + .expect(200) + .then(({ body }) => { + body.map((a) => a.action).should.eql(['form.update.publish', 'form.create', 'user.session.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') + .expect(200) + .then(({ body }) => { + body.map((a) => a.action).should.eql(['form.update.publish', 'form.create', 'user.session.create']); + })))))); describe('Enketo ID for draft', () => { it('should request an enketoId', testService(async (service, { env }) => { @@ -348,6 +380,9 @@ describe('api: /projects/:id/forms (create, read, update)', () => { // Second request, from the worker global.enketo.callCount.should.equal(1); + + // Remove enketo error state + global.enketo.state = undefined; await exhaust(container); global.enketo.callCount.should.equal(2); const { body } = await asAlice.get('/v1/projects/1/forms/simple2') @@ -380,7 +415,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' @@ -400,7 +436,7 @@ describe('api: /projects/:id/forms (create, read, update)', () => { should.not.exist(body.enketoOnceId); })); - it('should wait for Enketo only briefly @slow', testService(async (service) => { + it.skip('should wait for Enketo only briefly @slow', testService(async (service) => { const asAlice = await service.login('alice'); global.enketo.wait = (done) => { setTimeout(done, 600); }; const { body } = await asAlice.post('/v1/projects/1/forms?publish=true') @@ -411,6 +447,18 @@ 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'); @@ -422,9 +470,10 @@ describe('api: /projects/:id/forms (create, read, update)', () => { .expect(200); // Second request, from the worker - global.enketo.callCount.should.equal(1); - await exhaust(container); global.enketo.callCount.should.equal(2); + global.enketo.state = undefined; + await exhaust(container); + global.enketo.callCount.should.equal(3); const { body } = await asAlice.get('/v1/projects/1/forms/simple2') .expect(200); without(['token'], global.enketo.createData).should.eql({ @@ -441,9 +490,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); })); }); diff --git a/test/integration/fixtures/02-forms.js b/test/integration/fixtures/02-forms.js index 1312c6c9c..45a47c78e 100644 --- a/test/integration/fixtures/02-forms.js +++ b/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'; + /* 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, false); + 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 call count and error state to not affect tests + global.enketo.callCount = 0; + global.enketo.state = undefined; }; diff --git a/test/integration/other/form-versioning.js b/test/integration/other/form-versioning.js index f045686d9..3bdf0317b 100644 --- a/test/integration/other/form-versioning.js +++ b/test/integration/other/form-versioning.js @@ -72,7 +72,8 @@ describe('form forward versioning', () => { Form.fromXml(testData.forms.withAttachments), Blob.fromFile(__filename).then(Blobs.ensure) ]) - .then(([ project, partial, blobId ]) => Forms.createNew(partial, project, true) + .then(([ project, partial, blobId ]) => Forms.createNew(partial, project) + .then((formDraft) => Forms.publish(formDraft, true)) .then((savedForm) => Promise.all([ 'goodone.csv', 'goodtwo.mp3' ] .map((name) => FormAttachments.getByFormDefIdAndName(savedForm.def.id, name) .then(force) @@ -106,7 +107,8 @@ describe('form forward versioning', () => { Form.fromXml(testData.forms.withAttachments), Blob.fromFile(__filename).then(Blobs.ensure) ]) - .then(([ project, partial, blobId ]) => Forms.createNew(partial, project, true) + .then(([ project, partial, blobId ]) => Forms.createNew(partial, project) + .then((formDraft) => Forms.publish(formDraft, true)) .then((savedForm) => Promise.all([ 'goodone.csv', 'goodtwo.mp3' ] .map((name) => FormAttachments.getByFormDefIdAndName(savedForm.def.id, name) .then(force) diff --git a/test/util/enketo.js b/test/util/enketo.js index 3b5f97b07..fc7f6bb16 100644 --- a/test/util/enketo.js +++ b/test/util/enketo.js @@ -44,7 +44,7 @@ const reset = () => { const request = () => { global.enketo.callCount += 1; const options = { ...global.enketo }; - Object.assign(global.enketo, without(['callCount'], defaults)); + Object.assign(global.enketo, without(['callCount', 'state'], defaults)); return new Promise((resolve, reject) => { const { wait } = options; const tokenBeforeWait = cancelToken;