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

Issues with draft submissions after project encryption #911

Closed
matthew-white opened this issue Jun 1, 2023 · 8 comments · Fixed by #944
Closed

Issues with draft submissions after project encryption #911

matthew-white opened this issue Jun 1, 2023 · 8 comments · Fixed by #944
Assignees
Labels

Comments

@matthew-white
Copy link
Member

matthew-white commented Jun 1, 2023

From the QA team while completing regression testing for v2023.3:

Disappearing submissions in draft testing after the project becomes encrypted. Steps to reproduce:
  • Add a new draft version of a form
  • Go to the Testing tab and add a submission (click “+New” and submit a form)
  • Go to the project overview, Settings and enable encryption
  • Go back to the Draft of the form and add another submission
  • Check the submission table and download the CSV



These screenshots seem to be from this page: https://test.getodk.cloud/#/projects/451/forms/geojson/draft/testing. It's interesting that the submission count is 3 (on the download button, in the row number), yet only 1 submission is shown. Similarly at https://test.getodk.cloud/#/projects/469/forms/geojson/draft/testing, the submission count is 2, yet no submissions are shown.

@sadiqkhoja was able to reproduce this behavior using v2022.3.1, so it doesn't seem like a regression in the upcoming v2023.3.

@github-project-automation github-project-automation bot moved this to 🕒 backlog in ODK Central Jun 1, 2023
@sadiqkhoja sadiqkhoja self-assigned this Jul 11, 2023
@sadiqkhoja sadiqkhoja self-assigned this Jul 13, 2023
@sadiqkhoja
Copy link
Contributor

Root cause: After enabling managed encryption we delete orphan form drafts that cascades to submission definitions. However, submissions are not deleted so we have orphan submissions without definitions

Potential solution 1: delete draft submissions on project encryption

Potential solution 2: Keep the draft forms which have draft submissions on project encryption. I don't know the full implication of this when form is published.

Solution 1 is simple, is it intuitive enough for the users?

@matthew-white
Copy link
Member Author

matthew-white commented Jul 13, 2023

I think it's safe to say that this isn't a common situation. So if solution 1 is the easiest thing, part of me thinks we should do that.

That said, we also know that users sometimes accidentally collect real data in draft submissions. We could mention this behavior as a warning in the "Enable Encryption" modal in Frontend, but that modal already has a lot of warnings.

Could we explore what would be involved in potential solution 2? We didn't delete form drafts after enabling encryption until #453. What if we reverted that change, so that enabling encryption doesn't delete form drafts? In that case, after enabling encryption:

  • Uploading a new form draft would continue to delete the orphan draft.
  • Deleting the current form draft would continue to delete the orphan draft as well.
  • We would probably need to delete the orphan draft after the current form draft is published.

@matthew-white
Copy link
Member Author

Whether it's solution 1 or 2, should we also have a migration to delete draft submissions that have gotten in this state? Or is it fine to leave them as-is knowing that they'll be deleted once the form draft is published, replaced, or deleted?

@sadiqkhoja
Copy link
Contributor

Option 2 can certainly be implemented; If we don't delete the draft Form on project encryption then we would have two draft Forms i.e. unencrypted and encrypted. And on publish we would need to delete both of them. But is it worth it to protect draft Submissions?

Yes, we would need to write migration to clean up Submissions without any Submission Definition

@matthew-white
Copy link
Member Author

on publish we would need to delete both of them.

Forms.publish() should already handle the encrypted draft. I think it doesn't delete the draft from the form_defs table, but instead sets publishedAt and changes currentDefId and draftDefId on the form. Once Forms.publish() has returned, we could call Forms.clearUnneededDrafts() to delete the unencrypted draft. I'm thinking of options 1 and 2 as involving about the same amount of code, but maybe I'm underthinking it. I doubt it's a common situation, so I reckon both options are probably OK.

@sadiqkhoja
Copy link
Contributor

@getodk/testers this can be tested on staging.

Expected Behaviour

@dbemke
Copy link

dbemke commented Aug 17, 2023

Tested with success!

@srujner
Copy link

srujner commented Aug 17, 2023

Tested with success!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ done
Development

Successfully merging a pull request may close this issue.

4 participants