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

Purge forms and associated resources #431

Closed
wants to merge 16 commits into from
Closed

Conversation

ktuite
Copy link
Member

@ktuite ktuite commented Nov 15, 2021

This PR enables the purging of soft-deleted forms and their associated resources (definitions, versions, submissions, attachments, comments, etc.)

Purging follows these steps:

  1. Redact notes about forms from the audit table that reference a form (includes one kind of comment on a submission)
  2. Log the purge in the audit log with actor not set
  3. Update actees table for the specific form to leave some useful information behind
  4. Delete the forms and their resources from the database
  5. Purge unattached blobs

As a side effect of deleting/purging Forms, we needed to address issue getodk/central#127 and decide what to display in the audit log for deleted resources. To do this, we added purgedAt, purgedName, details to Actees to capture information about a permanently removed resources and then exposed this info via the audit log API.

This PR has a number of high-impact migrations:

  • New fields described above on actees table
  • Cascaded delete constraints on all form-related database tables
  • Purging all previously soft-deleted forms regardless of deletion date

Purging is done through a task and command line script that will be called via cron job in the main Central repo. The script will be called daily to purge forms deleted over 30 days prior, but can also be used to purge a specific form by its numeric id.

@ktuite ktuite force-pushed the ktuite/purge-forms branch from 777f320 to 6ebeb28 Compare November 15, 2021 23:44
@ktuite ktuite force-pushed the ktuite/purge-forms branch from 6ebeb28 to 777f61c Compare November 16, 2021 00:51
@ktuite ktuite force-pushed the ktuite/purge-forms branch from 777f61c to bb3258b Compare November 17, 2021 18:34
Copy link
Member Author

@ktuite ktuite left a comment

Choose a reason for hiding this comment

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

interactive review with @issa-tseng

const up = async (db) => {
await db.schema.table('form_defs', (t) => {
t.dropForeign('formId');
t.foreign('formId').references('forms.id').onDelete('cascade');
Copy link
Member Author

Choose a reason for hiding this comment

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

look into doing these with an alter instead of a drop if possible

const del = (form) => ({ run, Assignments }) =>
Promise.all([ run(markDeleted(form)), Assignments.revokeByActeeId(form.acteeId) ]);
const del = (form) => ({ run, Assignments, Forms }) =>
Promise.all([ run(markDeleted(form)), Assignments.revokeByActeeId(form.acteeId) ])
Copy link
Member Author

Choose a reason for hiding this comment

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

consider just purging and not having to check it and mark deleted first

Copy link
Member Author

Choose a reason for hiding this comment

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

might need to revoke by actee id still even if form purged without being deleted first

and ${_trashedFilter(force, id)}`)
.then(() => run(sql`
insert into audits ("action", "acteeId", "loggedAt")
select 'form.purge', "acteeId", clock_timestamp()
Copy link
Member Author

Choose a reason for hiding this comment

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

form.purge verb? maybe doesn't need to be in any verb table, but i'll double check

// 5. Purge unattached blobs
const purge = (force = false, id = null) => ({ run, all, Blobs }) =>
run(sql`
update audits set notes = ''
Copy link
Member Author

Choose a reason for hiding this comment

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

take a stab at combining all these into a single quere

// 3. by a specific form id if deletedAt is also set (again for testing or potential future scenarios)

const DAY_RANGE = 30;
const _trashedFilter = (force, id) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

@issa-tseng's going to look up some resource injection stuff

Copy link
Member Author

@ktuite ktuite left a comment

Choose a reason for hiding this comment

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

more notes from talking to @matthew-white

const del = (form) => ({ run, Assignments }) =>
Promise.all([ run(markDeleted(form)), Assignments.revokeByActeeId(form.acteeId) ]);
const del = (form) => ({ run, Assignments, Forms }) =>
Promise.all([ run(markDeleted(form)), Assignments.revokeByActeeId(form.acteeId) ])
Copy link
Member Author

Choose a reason for hiding this comment

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

might need to revoke by actee id still even if form purged without being deleted first

@ktuite ktuite force-pushed the ktuite/purge-forms branch from 81eb8d5 to 8b32340 Compare November 19, 2021 21:22
@ktuite ktuite marked this pull request as ready for review December 15, 2021 23:43
@ktuite ktuite force-pushed the ktuite/purge-forms branch from 384fbd5 to 61ef815 Compare January 13, 2022 22:58
@ktuite
Copy link
Member Author

ktuite commented Jan 21, 2022

This PR was getting messy and overwhelming so I split it into #443, #444 and #445. Will continue to comments in those branches/PRs. Closing this one.

@ktuite ktuite closed this Jan 21, 2022
@matthew-white matthew-white deleted the ktuite/purge-forms branch January 26, 2022 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants