Remove report flag for removal #4566
Conversation
|
Terraform plan for meta No changes. Your infrastructure matches the configuration.✅ Plan applied in Deploy to Development and Management Environment #895 |
|
Terraform plan for dev Plan: 1 to add, 0 to change, 1 to destroy.Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
-/+ destroy and then create replacement
Terraform will perform the following actions:
# module.dev.module.cors.null_resource.cors_header must be replaced
-/+ resource "null_resource" "cors_header" {
!~ id = "*******************" -> (known after apply)
!~ triggers = { # forces replacement
!~ "always_run" = "2024-12-30T16:46:34Z" -> (known after apply)
}
}
Plan: 1 to add, 0 to change, 1 to destroy.✅ Plan applied in Deploy to Development and Management Environment #895 |
backend/audit/admin.py
Outdated
| SacValidationWaiver.objects.filter(report_id=sac).delete() | ||
| remove_singleauditreport_pdf(sac) | ||
| remove_workbook_artifacts(sac) | ||
| Access.objects.filter(sac=sac).delete() | ||
| SubmissionEvent.objects.filter(sac=sac).delete() | ||
| ExcelFile.objects.filter(sac=sac).delete() | ||
| SingleAuditReportFile.objects.filter(sac=sac).delete() | ||
| report_ids.append(sac.report_id) | ||
| sac.delete() |
There was a problem hiding this comment.
I am still getting caught up to speed on Django ORM...
It looks like we iterate over all the SAC records, then one by one delete entries with a foreign key relation, followed by deleting the single sac.
Does Django handle all those delete as a single transaction? If no, I am a bit concerned about failures in the middle of this process. For example, what if the SubmissionEvent fails to delete, won't that leave us in an inconsistent state?
If the database has cascading deletes on all the foreign key constraints, we could get away with just deleting all the sacs at once rather than in loop.
There was a problem hiding this comment.
Placing this process under a transaction is generally a good idea to avoid unexpected issues. The current implementation, even without a global transaction, will not leave the database in an inconsistent state. This is because the sac record, which acts as the parent for all the other records in the list, is only removed at the end of the process. If an unexpected issue occurs midway through the process, the sac record will remain intact. When the action is re-run, it will clean up any remaining child records before finally removing the sac. As long as the parent sac record is not removed prematurely, the child records can be handled in subsequent runs of the deletion action.
That said, I still add a global transaction as an extra layer of protection.
There was a problem hiding this comment.
I really like small self contained files like this.
|
I can't seem to get any to delete. Here is my sample set: I also have a question. If an audit is created and never gets past the |
You missed step 3. Reports should be flagged for removal before you can delete them. This holds for your two questions. |
|
Description
This PR introduces the logic change necessary to allow staff users to delete reports flagged for removal for over 6 months using the Django admin interface.
How to Test:
transition_datefield for one of the flagged reports:transition_dateis at least six months prior to the current date.Delete reports flagged for removal for over 6 months.Expected Outcome:
transition_dateless than six months from the current date is not removed.transition_dategreater than six months from the current date is removed.PR Checklist: Submitter
maininto your branch shortly before creating the PR. (You should also be mergingmaininto your branch regularly during development.)git status | grep migrations. If there are any results, you probably need to add them to the branch for the PR. Your PR should have only one new migration file for each of the component apps, except in rare circumstances; you may need to delete some and re-runpython manage.py makemigrationsto reduce the number to one. (Also, unless in exceptional circumstances, your PR should not delete any migration files.)PR Checklist: Reviewer
make docker-clean; make docker-first-run && docker compose up; then rundocker compose exec web /bin/bash -c "python manage.py test"The larger the PR, the stricter we should be about these points.
Pre Merge Checklist: Merger
-/+ resource "null_resource" "cors_header"should be destroying and recreating its self and~ resource "cloudfoundry_app" "clamav_api"might be updating itssha256for thefac-file-scannerandfac-av-${ENV}by default.main.