Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recover deleted draft submissions by soft-deleting them when updating the form draft #1299

Merged
merged 10 commits into from
Nov 26, 2024
8 changes: 7 additions & 1 deletion lib/model/query/forms.js
Original file line number Diff line number Diff line change
Expand Up @@ -486,14 +486,20 @@ 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 the form as either currentDefId or draftDefId AND have no associated submission defs.
const clearUnneededDrafts = (form = null, project = null) => ({ run }) =>
run(sql`
DELETE FROM form_defs
USING forms
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
Expand Down
2 changes: 1 addition & 1 deletion lib/model/query/keys.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ INNER JOIN form_defs
INNER JOIN submission_defs
ON submission_defs."formDefId" = form_defs.id
INNER JOIN submissions
ON submissions.id = submission_defs."submissionId"
ON submissions.id = submission_defs."submissionId" AND submissions."deletedAt" IS NULL
WHERE submission_defs.current = true
AND submission_defs."localKey" IS NOT NULL
AND submissions.draft = ${draft}
Expand Down
9 changes: 5 additions & 4 deletions lib/model/query/submissions.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,12 +115,12 @@ createVersion.audit.withResult = true;
const update = (form, submission, data) => ({ one }) => one(updater(submission, data)).then(construct(Submission));
update.audit = (form, submission, data) => (log) => log('submission.update', form, Object.assign({ submissionId: submission.id, submissionDefId: submission.def.id, instanceId: submission.def.instanceId }, data));

const clearDraftSubmissions = (formId) => ({ run }) =>
run(sql`delete from submissions where "formId"=${formId} and draft=true`);

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 deleteDraftSubmissions = (formId) => ({ run }) =>
run(sql`UPDATE submissions SET "deletedAt"=now() WHERE "formId"=${formId} AND "draft"=true AND "deletedAt" IS NULL`);

////////////////////////////////////////////////////////////////////////////////
// SELECT-MULTIPLE VALUES

Expand Down Expand Up @@ -209,6 +209,7 @@ const getDeleted = (projectId, formId, instanceId) => ({ maybeOne }) =>
and submissions."formId" = ${formId}
and submissions."instanceId" = ${instanceId}
and submissions."deletedAt" IS NOT NULL
and submissions."draft" = false
`)
.then(map(construct(Submission)));

Expand Down Expand Up @@ -475,7 +476,7 @@ select count(*) from deleted_submissions`);

module.exports = {
createNew, createVersion,
update, del, restore, purge, clearDraftSubmissions, clearDraftSubmissionsForProject,
update, del, restore, purge, clearDraftSubmissionsForProject, deleteDraftSubmissions,
setSelectMultipleValues, getSelectMultipleValuesForExport,
getByIdsWithDef, getSubAndDefById,
getByIds, getAllForFormByIds, getById, countByFormId, verifyVersion,
Expand Down
6 changes: 3 additions & 3 deletions lib/resources/forms.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
ktuite marked this conversation as resolved.
Show resolved Hide resolved
Submissions.deleteDraftSubmissions(form.id)
]))
.then(() => Forms.clearUnneededDrafts(form))) // remove drafts made obsolete by new draft
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that this endpoint will no longer result in a new form draft that's ready to be purged, I think we can remove this line. It'd be a nice way to simplify the endpoint. I also think it'd make things clearer if there was only one thing that called clearUnneededDrafts() (namely, the purge task).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One possible reason to leave it here is when there are no submissions, the draft def can be cleaned up right away. There are some tests in "purging form fields of unneeded drafts" (in api: /projects/:id/forms (drafts)) that are checking for these defs to be purged immediately. I could change or remove those tests, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, interesting. No strong feelings, either way!

.then(success)));
Expand All @@ -162,7 +162,7 @@ module.exports = (service, endpoint) => {
.then(() => Forms.getByProjectAndXmlFormId(params.projectId, params.id, false, Form.DraftVersion))
.then(getOrNotFound)
: resolve(form)))
ktuite marked this conversation as resolved.
Show resolved Hide resolved
.then(((form) => Promise.all([ Forms.publish(form), Submissions.clearDraftSubmissions(form.id) ])))
.then(((form) => Promise.all([ Forms.publish(form), Submissions.deleteDraftSubmissions(form.id) ])))
.then(success)));

// Entity/Dataset-specific endpoint that is used to show how publishing
Expand Down Expand Up @@ -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.deleteDraftSubmissions(form.id),
Audits.log(auth.actor, 'form.update.draft.delete', form, { oldDraftDefId: form.draftDefId })
]))
.then(success)));
Expand Down
5 changes: 5 additions & 0 deletions lib/task/purge.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Loading