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

Added a form deletion script for genie requests #35514

Merged
merged 6 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions corehq/apps/cleanup/management/commands/delete_forms.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
from django.core.management.base import BaseCommand
import csv
import itertools
from dimagi.utils.chunked import chunked
from corehq.form_processor.models import XFormInstance


INDEX_FORM_ID = 1
CHUNK_SIZE = 100


class Command(BaseCommand):
def add_arguments(self, parser):
parser.add_argument('domain', help='Domain name that owns the forms to be deleted')
parser.add_argument('filename', help='path to the CSV file')
parser.add_argument('--resume_id', help='form ID to start at, within the CSV file')

def handle(self, domain, filename, resume_id=None, **options):
# expects the filename to have a CSV with a header containing "Deletion Status" and "Form ID" fields
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually use the "Deletion Status" field? I only see the "Form ID" field being used. My thinking here is if there is a Deletion Status field with values under it, someone's going to expect those values to be used for something, and if we're not going to use them, then we should require the CSV not have that column.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is working with the CSV that Enveritas submitted to us. If I simplify this script to expect a sheet without that column, then I'd need to write another script to port the Envertias CSV over. Do you think that's something we should do long term?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 4fccff9. As mentioned offline, I think the process going forward should be that customers making this request provide us a CSV file that only contains a single column representing form ID. In this case, the customer provided an initial column called "Deletion Status" which only seems to contain ready_to_request. However, if one of the rows contained something like do_not_delete instead, we wouldn't want to have the responsibility for interpreting that field.

with open(filename) as csvfile:
reader = csv.reader(csvfile, delimiter=',')
self._process_rows(reader, domain, resume_id)

def _process_rows(self, rows, domain, resume_id):
next(rows) # skip header line

num_deleted = 0

if resume_id:
print('resuming at: ', resume_id)
rows = itertools.dropwhile(lambda row: row[INDEX_FORM_ID] != resume_id, rows)

print('Starting form deletion')
for chunk in chunked(rows, CHUNK_SIZE):
form_ids = [row[INDEX_FORM_ID] for row in chunk]

try:
deleted_form_ids = set(XFormInstance.objects.hard_delete_forms(
Copy link
Member

Choose a reason for hiding this comment

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

I feel squeemish about hard-delete, though obviously it is what we're being asked to do. An alternate pattern would be to soft delete and mark for automatic hard-deletion in 90 days. But that is probably overkill for the present situation. Maybe what I would ask is just to rename the command to hard_delete_forms.py, since in some places delete means soft-delete, and making that name explicit will remove any kind of ambiguity for anyone reusing this script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I can definitely do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed with 4fccff9

domain, form_ids, return_ids=True))
except Exception:
print('failed during processing of: ', form_ids)
raise

for form_id in form_ids:
if form_id in deleted_form_ids:
print('Deleted: ', form_id)
else:
print('Not found:', form_id)

num_deleted += len(deleted_form_ids)

print(f'Complete -- removed {num_deleted} forms')
15 changes: 10 additions & 5 deletions corehq/form_processor/models/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,8 @@ def soft_undelete_forms(self, domain, form_ids):

return count

def hard_delete_forms(self, domain, form_ids, delete_attachments=True, *, publish_changes=True):
def hard_delete_forms(
self, domain, form_ids, return_ids=False, delete_attachments=True, *, publish_changes=True):
"""Delete forms permanently

:param publish_changes: Flag for change feed publication.
Expand All @@ -398,12 +399,16 @@ def hard_delete_forms(self, domain, form_ids, delete_attachments=True, *, publis
assert isinstance(form_ids, list)

deleted_count = 0
deleted_ids = []
for db_name, split_form_ids in split_list_by_db_partition(form_ids):
# cascade should delete the operations
_, deleted_models = self.using(db_name).filter(
domain=domain, form_id__in=split_form_ids
).delete()
query = self.using(db_name).filter(domain=domain, form_id__in=split_form_ids)
if return_ids:
found_forms = list(query.values_list('form_id', flat=True))
_, deleted_models = query.delete()
deleted_count += deleted_models.get(self.model._meta.label, 0)
if return_ids:
deleted_ids.extend(found_forms)

if delete_attachments and deleted_count:
if deleted_count != len(form_ids):
Expand All @@ -421,7 +426,7 @@ def hard_delete_forms(self, domain, form_ids, delete_attachments=True, *, publis
if publish_changes:
self.publish_deleted_forms(domain, form_ids)

return deleted_count
return deleted_ids if return_ids else deleted_count

@staticmethod
def publish_deleted_forms(domain, form_ids):
Expand Down
16 changes: 16 additions & 0 deletions corehq/form_processor/tests/test_forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,22 @@ def test_hard_delete_forms(self):
self.assertEqual(1, len(forms))
self.assertEqual(form_ids[0], forms[0].form_id)

def test_hard_delete_forms_returns_forms_found(self):
for i in range(3):
create_form_for_test(DOMAIN, form_id=str(i))

deleted_form_ids = set(XFormInstance.objects.hard_delete_forms(DOMAIN, ['0', '1', '2'], return_ids=True))

self.assertEqual(deleted_form_ids, {'0', '1', '2'})

def test_hard_delete_forms_does_not_include_missing_form_ids(self):
create_form_for_test(DOMAIN, form_id='1')
create_form_for_test(DOMAIN, form_id='3')

deleted_form_ids = set(XFormInstance.objects.hard_delete_forms(DOMAIN, ['1', '2', '3'], return_ids=True))

self.assertEqual(deleted_form_ids, {'1', '3'})

def assert_form_xml_attachment(self, form):
attachments = XFormInstance.objects.get_attachments(form.form_id)
self.assertEqual([a.name for a in attachments], ["form.xml"])
Expand Down
Loading