From 31ce9ceef7e721baa0cb5f5440ca852e4430a08a Mon Sep 17 00:00:00 2001 From: Kathleen Tuite Date: Fri, 15 Nov 2024 16:53:58 -0800 Subject: [PATCH] wip: soft-delete draft submissions when drafts get replaced --- lib/model/query/forms.js | 8 +++- lib/model/query/submissions.js | 5 ++- lib/resources/forms.js | 6 +-- lib/task/purge.js | 5 +++ test/integration/api/forms/draft.js | 62 +++++++++++++++++++++++++++++ test/integration/api/submissions.js | 2 +- 6 files changed, 82 insertions(+), 6 deletions(-) diff --git a/lib/model/query/forms.js b/lib/model/query/forms.js index be3edc1b3..be489ad0c 100644 --- a/lib/model/query/forms.js +++ b/lib/model/query/forms.js @@ -485,7 +485,8 @@ const _draftFilter = (form, project) => ? sql`and forms."projectId" = ${project.id}` : sql``)); -// NOTE: copypasta alert! The following SQL also appears in 20220209-01-purge-unneeded-drafts.js +// NOTE: copypasta alert! Similar SQL also appears in 20220209-01-purge-unneeded-drafts.js +// Purges draft form defs that are not referenced by any forms AND have no associated submission defs. const clearUnneededDrafts = (form = null, project = null) => ({ run }) => run(sql` DELETE FROM form_defs @@ -493,6 +494,11 @@ DELETE FROM form_defs WHERE form_defs."formId" = forms.id AND form_defs."publishedAt" IS NULL AND form_defs.id IS DISTINCT FROM forms."draftDefId" + AND NOT EXISTS ( + SELECT 1 + FROM submission_defs + WHERE submission_defs."formDefId" = form_defs.id + ) ${_draftFilter(form, project)}`) .then(() => run(sql` DELETE FROM form_schemas diff --git a/lib/model/query/submissions.js b/lib/model/query/submissions.js index 3e70d0f59..c8dd3a954 100644 --- a/lib/model/query/submissions.js +++ b/lib/model/query/submissions.js @@ -121,6 +121,9 @@ const clearDraftSubmissions = (formId) => ({ run }) => const clearDraftSubmissionsForProject = (projectId) => ({ run }) => run(sql`DELETE FROM submissions USING forms WHERE submissions."formId" = forms.id AND forms."projectId" = ${projectId} AND submissions.draft=true`); +const softDeleteDraftSubmissions = (formId) => ({ run }) => + run(sql`UPDATE submissions SET "deletedAt"=now() WHERE "formId"=${formId} AND "draft"=true AND "deletedAt" IS NULL`); + //////////////////////////////////////////////////////////////////////////////// // SELECT-MULTIPLE VALUES @@ -475,7 +478,7 @@ select count(*) from deleted_submissions`); module.exports = { createNew, createVersion, - update, del, restore, purge, clearDraftSubmissions, clearDraftSubmissionsForProject, + update, del, restore, purge, clearDraftSubmissions, clearDraftSubmissionsForProject, softDeleteDraftSubmissions, setSelectMultipleValues, getSelectMultipleValuesForExport, getByIdsWithDef, getSubAndDefById, getByIds, getAllForFormByIds, getById, countByFormId, verifyVersion, diff --git a/lib/resources/forms.js b/lib/resources/forms.js index 45633ab74..6899a22a6 100644 --- a/lib/resources/forms.js +++ b/lib/resources/forms.js @@ -137,7 +137,7 @@ module.exports = (service, endpoint) => { : getPartial(Forms, request, project, Keys))) .then((partial) => Promise.all([ Forms.createVersion(partial, form, false), - Submissions.clearDraftSubmissions(form.id) + Submissions.softDeleteDraftSubmissions(form.id) ])) .then(() => Forms.clearUnneededDrafts(form))) // remove drafts made obsolete by new draft .then(success))); @@ -162,7 +162,7 @@ module.exports = (service, endpoint) => { .then(() => Forms.getByProjectAndXmlFormId(params.projectId, params.id, false, Form.DraftVersion)) .then(getOrNotFound) : resolve(form))) - .then(((form) => Promise.all([ Forms.publish(form), Submissions.clearDraftSubmissions(form.id) ]))) + .then(((form) => Promise.all([ Forms.publish(form), Submissions.softDeleteDraftSubmissions(form.id) ]))) .then(success))); // Entity/Dataset-specific endpoint that is used to show how publishing @@ -244,7 +244,7 @@ module.exports = (service, endpoint) => { .then(rejectIf(((form) => form.draftDefId == null), noargs(Problem.user.notFound))) .then((form) => Promise.all([ Forms.clearDraft(form).then(() => Forms.clearUnneededDrafts(form)), - Submissions.clearDraftSubmissions(form.id), + Submissions.softDeleteDraftSubmissions(form.id), Audits.log(auth.actor, 'form.update.draft.delete', form, { oldDraftDefId: form.draftDefId }) ])) .then(success))); diff --git a/lib/task/purge.js b/lib/task/purge.js index e83ba52ce..9bc0c819b 100644 --- a/lib/task/purge.js +++ b/lib/task/purge.js @@ -21,8 +21,13 @@ const purgeTask = task.withContainer((container) => async (options = {}) => { const count = await Forms.purge(options.force, options.formId, options.projectId, options.xmlFormId); return `Forms purged: ${count}`; } else { + // Purge both Forms and Submissions according to options const formCount = await Forms.purge(options.force, options.formId, options.projectId, options.xmlFormId); const submissionCount = await Submissions.purge(options.force, options.projectId, options.xmlFormId, options.instanceId); + + // Related to form purging: deletes draft form defs that are not in use by any form and have no associated submission defs + await Forms.clearUnneededDrafts(); + return `Forms purged: ${formCount}, Submissions purged: ${submissionCount}`; } } catch (error) { diff --git a/test/integration/api/forms/draft.js b/test/integration/api/forms/draft.js index 48f6bc726..c42d8838b 100644 --- a/test/integration/api/forms/draft.js +++ b/test/integration/api/forms/draft.js @@ -844,6 +844,68 @@ describe('api: /projects/:id/forms (drafts)', () => { .then((counts) => counts.should.eql([ 1, 4, 3 ]))))); }); }); + + describe('preserving submissions from old or deleted drafts', () => { + it('should soft-delete submissions of undeeded draft when a new version is uploaded', testService(async (service, { oneFirst }) => { + const asAlice = await service.login('alice'); + + await asAlice.post('/v1/projects/1/forms/simple/draft') + .send(testData.forms.simple.replace('id="simple"', 'id="simple" version="drafty"')) + .set('Content-Type', 'application/xml') + .expect(200); + + await asAlice.post('/v1/projects/1/forms/simple/draft/submissions') + .send(testData.instances.simple.one) + .set('Content-Type', 'text/xml') + .expect(200); + + let subs = await oneFirst(sql`select count(*) from submissions`); + subs.should.equal(1); + + await asAlice.post('/v1/projects/1/forms/simple/draft') + .send(testData.forms.simple.replace('id="simple"', 'id="simple" version="drafty2"')) + .set('Content-Type', 'application/xml') + .expect(200); + + subs = await oneFirst(sql`select count(*) from submissions`); + subs.should.equal(1); + + const fds = await oneFirst(sql`select count(*) from form_defs as fd join forms as f on fd."formId" = f.id where f."xmlFormId"='simple'`); + fds.should.equal(3); // Old draft has not been deleted. Count also includes published and new draft. + })); + }); + + it('should purge old draft submissions after 30 days', testService(async (service, { oneFirst, run, Forms, Submissions }) => { + const asAlice = await service.login('alice'); + + await asAlice.post('/v1/projects/1/forms/simple/draft') + .send(testData.forms.simple.replace('id="simple"', 'id="simple" version="drafty"')) + .set('Content-Type', 'application/xml') + .expect(200); + + await asAlice.post('/v1/projects/1/forms/simple/draft/submissions') + .send(testData.instances.simple.one) + .set('Content-Type', 'text/xml') + .expect(200); + + let subs = await oneFirst(sql`select count(*) from submissions`); + subs.should.equal(1); + + await asAlice.post('/v1/projects/1/forms/simple/draft') + .send(testData.forms.simple.replace('id="simple"', 'id="simple" version="drafty2"')) + .set('Content-Type', 'application/xml') + .expect(200); + + await run(sql`update submissions set "deletedAt" = '1999-1-1T00:00:00Z' where "deletedAt" is not null`); + await Submissions.purge(); + await Forms.clearUnneededDrafts(); + + subs = await oneFirst(sql`select count(*) from submissions`); + subs.should.equal(0); + + const fds = await oneFirst(sql`select count(*) from form_defs as fd join forms as f on fd."formId" = f.id where f."xmlFormId"='simple'`); + fds.should.equal(2); // Old draft has now been deleted. Count also includes published and new draft. + })); }); }); diff --git a/test/integration/api/submissions.js b/test/integration/api/submissions.js index a031bc77a..d483e6cd7 100644 --- a/test/integration/api/submissions.js +++ b/test/integration/api/submissions.js @@ -3294,7 +3294,7 @@ one,h,/data/h,2000-01-01T00:06,2000-01-01T00:07,-5,-6,,ee,ff .expect(200) .then(({ body }) => { body.should.eql([]); }))))); - it('should not carry over draft keys when a draft is replaced', testService((service) => + it.only('should not carry over draft keys when a draft is replaced', testService((service) => service.login('alice', (asAlice) => asAlice.post('/v1/projects/1/forms?publish=true') .send(testData.forms.encrypted)