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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ktuite
Copy link
Member

@ktuite ktuite commented Nov 16, 2024

Closes getodk/central#749

WIP while adding more tests and thinking about encrypted forms.

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.

Before submitting this PR, please make sure you have:

  • run make test and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code from external sources are properly credited in comments or that everything is internally sourced

lib/resources/forms.js Show resolved Hide resolved
@@ -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
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!

lib/model/query/submissions.js Outdated Show resolved Hide resolved
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.

Explore options for recovering deleted draft submissions
2 participants