Facility deletion management command #7033
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In later versions of Django, it now requires an explicit `on_delete` option instead of defaulting to CASCADE
bjester
force-pushed
the
facility-deletion
branch
from
076ab31
to
36af42b
Compare
Codecov Report
|
bjester
added
TODO: needs review
jonboiser
approved these changes
Jun 16, 2020
radinamatic
reviewed
Jun 25, 2020
| post_delete.receivers = self.receivers | ||
| self.receivers = None | ||
|
|
||
|
|
||
| def _interactive_client_facility_selection(): | ||
| facilities = Facility.objects.all().order_by("name") | ||
| message = "Please choose a facility to sync:\n" |
There was a problem hiding this comment.
@bjester Is the use of sync here intentional? This command should delete the facility, so why sync...? 🤔
There was a problem hiding this comment.
@radinamatic Oh good catch! I didn't notice it said "sync". I'll make that dynamic
jamalex
reviewed
Jun 26, 2020
|
|
||
|
|
||
| class Command(AsyncCommand): | ||
| help = "This command initiates the deletion process for a facility and all of it's related data." |
| "ARE YOU SURE? If you do this, there is no way to recover the facility data on this device." | ||
| ) | ||
|
|
||
| # everything should get cascade deleted from the facility, but we'll check anyway |
There was a problem hiding this comment.
Sounds good. The main thing that I could imagine might be less likely to cascade delete would be anonymous ContentSessionLogs, which don't FK onto a user. But they would still have an FK onto the FacilityDataset (as would all of these), so it should all get cascade deleted, yeah.
| update_progress(increment=0, message="Counting database objects") | ||
| total_count = delete_group.count(update_progress) | ||
|
|
||
| # no the deleting step |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
facilitytaskendpoint for initiating a facility deletion taskon_delete=CASCADEto the models since it in future versions of Django, it requires an explicit action it instead of assuming cascade@actiondecorators with preferred alternative since it is deprecatedReviewer guidance
A nice to have feature was to have the command report progress. Relying on CASCADE deletions works just fine for most of the models, but also doesn't give much insight into that progress, as far as I know. This currently goes through all the models, counts them, and then deletes them; through this procedure it can report progress since it's handling the models individually.
Some feedback on whether we care about any of that would be great. It was a nice to have that didn't seem too difficult, but could get stale in the future if adding more related models, although still completely deleting the facility because of the cascade.
References
Notion page
Contributor Checklist
PR process:
If this includes an internal dependency change, a link to the diff is providedTesting:
If there are any front-end changes, before/after screenshots are includedReviewer Checklist
yarnandpip)