From dc7b4b4810d13c7dfcec4e7154bd23d14b9beb11 Mon Sep 17 00:00:00 2001 From: Jayanth Kumar Date: Sat, 27 Jan 2024 14:08:19 +0530 Subject: [PATCH 1/2] Process deleting projects as background task 1. Fix Default Filtering in ProjectFilterSet: - Modified the `ProjectFilterSet` to include `is_marked_for_deletion` in the default filtering when `is_archived` is not provided. - This ensures that projects marked for deletion are excluded from default views. 2. Migration for `is_marked_for_deletion`: - Added a migration (`0052_project_is_marked_for_deletion.py`) to introduce the `is_marked_for_deletion` field in the `Project` model. 3. Updated `Project` Model: - Added `is_marked_for_deletion` field to the `Project` model with a default value of `False`. 4. Modified `ProjectListView` to Exclude Marked Projects: - Modified the `get_queryset` method in `ProjectListView` to only fetch projects with `is_marked_for_deletion=False`. 5. Introduced `mark_for_deletion` Method: - Added a `mark_for_deletion` method in the `Project` model to set the `is_marked_for_deletion` flag and save the model. 6. Add `delete_in_background` Method in `Project` Model: - Introduced a new `delete_in_background` method that marks the project for deletion and enqueues a background deletion task. 7. Background Deletion Task: - Created a background deletion task using `django_rq` to handle project deletion asynchronously. - Checks if the project is still marked for deletion before proceeding. - If an error occurs during deletion, updates project status and logs an error. 8. Updated `ProjectActionView` Success Message: - Modified the success message in `ProjectActionView` for the "delete" action to provide a more informative message based on the count. Fixes: #1002 Signed-off-by: Jayanth Kumar --- scanpipe/filters.py | 8 ++++++-- .../0052_project_is_marked_for_deletion.py | 18 ++++++++++++++++++ scanpipe/models.py | 12 ++++++++++++ scanpipe/tasks.py | 18 ++++++++++++++++++ scanpipe/tests/test_views.py | 6 ++---- scanpipe/views.py | 7 ++++++- 6 files changed, 62 insertions(+), 7 deletions(-) create mode 100644 scanpipe/migrations/0052_project_is_marked_for_deletion.py diff --git a/scanpipe/filters.py b/scanpipe/filters.py index cdba64d8a..5bfe9a557 100644 --- a/scanpipe/filters.py +++ b/scanpipe/filters.py @@ -349,9 +349,13 @@ def __init__(self, data=None, *args, **kwargs): # Default filtering by "Active" projects. if not data or data.get("is_archived", "") == "": - self.queryset = self.queryset.filter(is_archived=False) + self.queryset = self.queryset.filter( + is_archived=False, is_marked_for_deletion=False + ) - active_count = Project.objects.filter(is_archived=False).count() + active_count = Project.objects.filter( + is_archived=False, is_marked_for_deletion=False + ).count() archived_count = Project.objects.filter(is_archived=True).count() self.filters["is_archived"].extra["widget"] = BulmaLinkWidget( choices=[ diff --git a/scanpipe/migrations/0052_project_is_marked_for_deletion.py b/scanpipe/migrations/0052_project_is_marked_for_deletion.py new file mode 100644 index 000000000..a4e4f3b73 --- /dev/null +++ b/scanpipe/migrations/0052_project_is_marked_for_deletion.py @@ -0,0 +1,18 @@ +# Generated by Django 5.0.1 on 2024-01-26 12:25 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('scanpipe', '0051_rename_pipelines_data'), + ] + + operations = [ + migrations.AddField( + model_name='project', + name='is_marked_for_deletion', + field=models.BooleanField(default=False), + ), + ] diff --git a/scanpipe/models.py b/scanpipe/models.py index e8461cdba..812472c60 100644 --- a/scanpipe/models.py +++ b/scanpipe/models.py @@ -76,6 +76,7 @@ from packageurl.contrib.django.models import PackageURLMixin from packageurl.contrib.django.models import PackageURLQuerySetMixin from rest_framework.authtoken.models import Token +from rq import Queue from rq.command import send_stop_job_command from rq.exceptions import NoSuchJobError from rq.job import Job @@ -534,6 +535,7 @@ class Project(UUIDPKModel, ExtraDataFieldMixin, UpdateMixin, models.Model): labels = TaggableManager(through=UUIDTaggedItem) objects = ProjectQuerySet.as_manager() + is_marked_for_deletion = models.BooleanField(default=False) class Meta: ordering = ["-created_date"] @@ -633,6 +635,16 @@ def delete(self, *args, **kwargs): return super().delete(*args, **kwargs) + def mark_for_deletion(self): + self.is_marked_for_deletion = True + self.save() + + def delete_in_background(self): + # Mark the project for deletion and enqueue background deletion task + self.mark_for_deletion() + q = Queue("default", connection=redis.Redis()) + job = q.enqueue(tasks.background_delete_task, self) + def reset(self, keep_input=True): """ Reset the project by deleting all related database objects and all work diff --git a/scanpipe/tasks.py b/scanpipe/tasks.py index d6379c614..525e39cb3 100644 --- a/scanpipe/tasks.py +++ b/scanpipe/tasks.py @@ -24,6 +24,8 @@ from django.apps import apps +from django_rq import job + logger = logging.getLogger(__name__) @@ -76,3 +78,19 @@ def execute_pipeline_task(run_pk): project.clear_tmp_directory() if next_run := project.get_next_run(): next_run.start() + + +@job +def background_delete_task(project): + # Check if the project is still marked for deletion + if not project.is_marked_for_deletion: + return + + # Perform the deletion process + try: + project.delete() + except Exception as e: + # Handle errors and update project errors or display a banner + project.is_marked_for_deletion = False + project.save() + project.add_error(description=f"Deletion failed: {str(e)}") diff --git a/scanpipe/tests/test_views.py b/scanpipe/tests/test_views.py index 472519a39..e5fbb4dcc 100644 --- a/scanpipe/tests/test_views.py +++ b/scanpipe/tests/test_views.py @@ -183,11 +183,9 @@ def test_scanpipe_views_project_actions_view(self): } response = self.client.post(url, data=data, follow=True) self.assertRedirects(response, reverse("project_list")) - expected = '
1 projects have been delete.
' + expected = '
1 project is being deleted in the background.
' self.assertContains(response, expected, html=True) - expected = ( - f'
Project {random_uuid} does not exist.
' - ) + expected = f"1 project is being deleted in the background." self.assertContains(response, expected, html=True) def test_scanpipe_views_project_details_is_archived(self): diff --git a/scanpipe/views.py b/scanpipe/views.py index e299052fb..68bb62f0f 100644 --- a/scanpipe/views.py +++ b/scanpipe/views.py @@ -1076,7 +1076,10 @@ def perform_action(self, action, project_uuid, action_kwargs=None): try: project = Project.objects.get(pk=project_uuid) - getattr(project, action)(**action_kwargs) + if action == "delete": + project.delete_in_background() + else: + getattr(project, action)(**action_kwargs) return True except Project.DoesNotExist: messages.error(self.request, f"Project {project_uuid} does not exist.") @@ -1086,6 +1089,8 @@ def perform_action(self, action, project_uuid, action_kwargs=None): raise Http404 def get_success_message(self, action, count): + if action == "delete": + return f"{count} project{'s' if count != 1 else ''} {'is' if count == 1 else 'are'} being deleted in the background." return f"{count} projects have been {action}." From bb312f786022d1b8060e4fbdfec40a27467c04ec Mon Sep 17 00:00:00 2001 From: Jayanth Kumar Date: Mon, 26 Feb 2024 22:42:10 +0530 Subject: [PATCH 2/2] feat: Address review comments - Remove comments - Use django rq - Use update instead of save Signed-off-by: Jayanth Kumar --- scanpipe/filters.py | 4 +--- scanpipe/models.py | 11 ++++++----- scanpipe/tasks.py | 6 ++---- scanpipe/tests/test_views.py | 4 ++-- scanpipe/views.py | 2 -- 5 files changed, 11 insertions(+), 16 deletions(-) diff --git a/scanpipe/filters.py b/scanpipe/filters.py index 5bfe9a557..00a93c15f 100644 --- a/scanpipe/filters.py +++ b/scanpipe/filters.py @@ -349,9 +349,7 @@ def __init__(self, data=None, *args, **kwargs): # Default filtering by "Active" projects. if not data or data.get("is_archived", "") == "": - self.queryset = self.queryset.filter( - is_archived=False, is_marked_for_deletion=False - ) + self.queryset = self.queryset.active() active_count = Project.objects.filter( is_archived=False, is_marked_for_deletion=False diff --git a/scanpipe/models.py b/scanpipe/models.py index 812472c60..ae4003634 100644 --- a/scanpipe/models.py +++ b/scanpipe/models.py @@ -489,6 +489,9 @@ def with_counts(self, *fields): return self.annotate(**annotations) + def active(self): + return self.filter(is_archived=False, is_marked_for_deletion=False) + class UUIDTaggedItem(GenericUUIDTaggedItemBase, TaggedItemBase): class Meta: @@ -532,10 +535,10 @@ class Project(UUIDPKModel, ExtraDataFieldMixin, UpdateMixin, models.Model): ) notes = models.TextField(blank=True) settings = models.JSONField(default=dict, blank=True) + is_marked_for_deletion = models.BooleanField(default=False) labels = TaggableManager(through=UUIDTaggedItem) objects = ProjectQuerySet.as_manager() - is_marked_for_deletion = models.BooleanField(default=False) class Meta: ordering = ["-created_date"] @@ -636,14 +639,12 @@ def delete(self, *args, **kwargs): return super().delete(*args, **kwargs) def mark_for_deletion(self): - self.is_marked_for_deletion = True - self.save() + self.update(is_marked_for_deletion=True) def delete_in_background(self): # Mark the project for deletion and enqueue background deletion task self.mark_for_deletion() - q = Queue("default", connection=redis.Redis()) - job = q.enqueue(tasks.background_delete_task, self) + django_rq.enqueue(tasks.background_delete_task, self) def reset(self, keep_input=True): """ diff --git a/scanpipe/tasks.py b/scanpipe/tasks.py index 525e39cb3..de669eb2b 100644 --- a/scanpipe/tasks.py +++ b/scanpipe/tasks.py @@ -86,11 +86,9 @@ def background_delete_task(project): if not project.is_marked_for_deletion: return - # Perform the deletion process try: project.delete() except Exception as e: - # Handle errors and update project errors or display a banner - project.is_marked_for_deletion = False - project.save() + info(f"Deletion failed: {str(e)}", project.pk) + project.update(is_marked_for_deletion=True) project.add_error(description=f"Deletion failed: {str(e)}") diff --git a/scanpipe/tests/test_views.py b/scanpipe/tests/test_views.py index e5fbb4dcc..c9a6ecb1e 100644 --- a/scanpipe/tests/test_views.py +++ b/scanpipe/tests/test_views.py @@ -183,9 +183,9 @@ def test_scanpipe_views_project_actions_view(self): } response = self.client.post(url, data=data, follow=True) self.assertRedirects(response, reverse("project_list")) - expected = '
1 project is being deleted in the background.
' + expected = '
1 projects have been delete.
' self.assertContains(response, expected, html=True) - expected = f"1 project is being deleted in the background." + expected = f"1 projects have been delete." self.assertContains(response, expected, html=True) def test_scanpipe_views_project_details_is_archived(self): diff --git a/scanpipe/views.py b/scanpipe/views.py index 68bb62f0f..d259d31e0 100644 --- a/scanpipe/views.py +++ b/scanpipe/views.py @@ -1089,8 +1089,6 @@ def perform_action(self, action, project_uuid, action_kwargs=None): raise Http404 def get_success_message(self, action, count): - if action == "delete": - return f"{count} project{'s' if count != 1 else ''} {'is' if count == 1 else 'are'} being deleted in the background." return f"{count} projects have been {action}."