Skip to content

Commit

Permalink
Remove use of conditional publish from Forms.createNew
Browse files Browse the repository at this point in the history
  • Loading branch information
@ktuite
ktuite committed May 10, 2024
1 parent b9c50ee commit 5e95823
Show file tree
Hide file tree
Showing 10 changed files with 106 additions and 50 deletions.
42 changes: 17 additions & 25 deletions lib/model/query/forms.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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
Expand All @@ -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;
};

Expand Down Expand Up @@ -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(
Expand All @@ -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 });
Expand Down
5 changes: 4 additions & 1 deletion lib/resources/forms.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) =>
Expand Down
3 changes: 2 additions & 1 deletion lib/util/db.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 *`;
};
Expand Down
4 changes: 2 additions & 2 deletions test/integration/api/audits.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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'
]);
});
Expand Down
7 changes: 2 additions & 5 deletions test/integration/api/datasets.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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();
});
}));

Expand Down
5 changes: 5 additions & 0 deletions test/integration/api/forms/draft.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -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);
Expand Down
67 changes: 58 additions & 9 deletions test/integration/api/forms/forms.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) =>
Expand Down Expand Up @@ -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 }) => {
Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -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'
Expand All @@ -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')
Expand All @@ -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');

Expand All @@ -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({
Expand All @@ -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);
}));
});

Expand Down
15 changes: 11 additions & 4 deletions test/integration/fixtures/02-forms.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
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;
};

6 changes: 4 additions & 2 deletions test/integration/other/form-versioning.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion test/util/enketo.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit 5e95823

Please sign in to comment.