From 610622a8ac2de727f482a1604e9fa6a6141c5ef3 Mon Sep 17 00:00:00 2001 From: Dhruv Bhanushali Date: Mon, 24 Jun 2024 14:02:47 +0400 Subject: [PATCH 01/10] Record models for media, sensitive media and deleted media in decision-media through model --- api/api/models/audio.py | 4 ++++ api/api/models/image.py | 4 ++++ api/api/models/media.py | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/api/api/models/audio.py b/api/api/models/audio.py index f1863ea4060..c751850e84f 100644 --- a/api/api/models/audio.py +++ b/api/api/models/audio.py @@ -363,6 +363,10 @@ class AudioDecisionThrough(AbstractMediaDecisionThrough): be referenced by `identifier` rather than an arbitrary `id`. """ + media_class = Audio + sensitive_media_class = SensitiveAudio + deleted_media_class = DeletedAudio + media_obj = models.ForeignKey( Audio, to_field="identifier", diff --git a/api/api/models/image.py b/api/api/models/image.py index 3d361d473f7..2f15dfc62cf 100644 --- a/api/api/models/image.py +++ b/api/api/models/image.py @@ -166,6 +166,10 @@ class ImageDecisionThrough(AbstractMediaDecisionThrough): be referenced by `identifier` rather than an arbitrary `id`. """ + media_class = Image + sensitive_media_class = SensitiveImage + deleted_media_class = DeletedImage + media_obj = models.ForeignKey( Image, to_field="identifier", diff --git a/api/api/models/media.py b/api/api/models/media.py index 86352967b27..aeac6473b64 100644 --- a/api/api/models/media.py +++ b/api/api/models/media.py @@ -348,6 +348,10 @@ class AbstractMediaDecisionThrough(models.Model): media_class: type[models.Model] = None """the model class associated with this media type e.g. ``Image`` or ``Audio``""" + sensitive_media_class: type[models.Model] = None + """the model class associated with this media type e.g. ``SensitiveImage`` or ``SensitiveAudio``""" + deleted_media_class: type[models.Model] = None + """the model class associated with this media type e.g. ``DeletedImage`` or ``DeletedAudio``""" media_obj = models.ForeignKey( AbstractMedia, From 86d013a2582af7762c42f513d554a08ff5cd60b4 Mon Sep 17 00:00:00 2001 From: Dhruv Bhanushali Date: Mon, 24 Jun 2024 14:03:34 +0400 Subject: [PATCH 02/10] Create deleted media and sensitive media models on save --- api/api/models/media.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/api/api/models/media.py b/api/api/models/media.py index aeac6473b64..8d98296c3bc 100644 --- a/api/api/models/media.py +++ b/api/api/models/media.py @@ -335,8 +335,6 @@ class AbstractMediaDecision(OpenLedgerModel): class Meta: abstract = True - # TODO: Implement ``clean`` and ``save``, if needed. - class AbstractMediaDecisionThrough(models.Model): """ @@ -364,6 +362,22 @@ class AbstractMediaDecisionThrough(models.Model): class Meta: abstract = True + def perform_action(self): + """Perform the action specified in the decision.""" + + if self.decision.action in { + DecisionAction.DEINDEXED_SENSITIVE, + DecisionAction.DEINDEXED_COPYRIGHT, + }: + self.deleted_media_class.objects.create(media_obj=self.media_obj) + + if self.decision.action == DecisionAction.MARKED_SENSITIVE: + self.sensitive_media_class.objects.create(media_obj=self.media_obj) + + def save(self, *args, **kwargs): + super().save(*args, **kwargs) + self.perform_action() + class PerformIndexUpdateMixin: @property From b6d944ff33a387a9412d04bc848381e277975da5 Mon Sep 17 00:00:00 2001 From: Dhruv Bhanushali Date: Mon, 24 Jun 2024 14:04:01 +0400 Subject: [PATCH 03/10] Create through models when performing moderation from the admin --- api/api/admin/media_report.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/api/api/admin/media_report.py b/api/api/admin/media_report.py index 09e8bf3ab03..66fc7b10be9 100644 --- a/api/api/admin/media_report.py +++ b/api/api/admin/media_report.py @@ -440,6 +440,10 @@ def decision_create_view(self, request, object_id): if request.method == "POST": media_obj = self.get_object(request, object_id) + through_model = { + "image": ImageDecisionThrough, + "audio": AudioDecisionThrough, + }[self.media_type] form = get_decision_form(self.media_type)(request.POST) if form.is_valid(): decision = form.save(commit=False) @@ -454,9 +458,13 @@ def decision_create_view(self, request, object_id): moderator=request.user.get_username(), ) - decision.media_objs.add(media_obj) + through = through_model.objects.create( + decision=decision, + media_obj=media_obj, + ) logger.info( - "Media linked to decision", + "Through model created", + through=through.id, decision=decision.id, media_obj=media_obj.id, ) From bcb9417488aa3833a955fd9960b62e5e53040696 Mon Sep 17 00:00:00 2001 From: Dhruv Bhanushali Date: Mon, 24 Jun 2024 14:07:47 +0400 Subject: [PATCH 04/10] Update backfill command to perform action associated with through models --- api/api/management/commands/backfillmoderationdecision.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/api/api/management/commands/backfillmoderationdecision.py b/api/api/management/commands/backfillmoderationdecision.py index 2b3598a3a66..c4cead0c5cd 100644 --- a/api/api/management/commands/backfillmoderationdecision.py +++ b/api/api/management/commands/backfillmoderationdecision.py @@ -92,12 +92,16 @@ def handle(self, *args, **options): for report, decision in zip(reports_chunk, decisions): report.decision = decision MediaReport.objects.bulk_update(reports_chunk, ["decision"]) - MediaDecisionThrough.objects.bulk_create( + throughs = MediaDecisionThrough.objects.bulk_create( [ MediaDecisionThrough(media_obj=report.media_obj, decision=decision) for report, decision in zip(reports_chunk, decisions) ] ) + # Bulk create does not call ``save()`` so the mark-sensitive/deindex + # actions will not be undertaken. + for through in throughs: + through.perform_action() t.update(1) t.info( From c9a1fdf50fb0b2c3d690b523b98e1131682c005d Mon Sep 17 00:00:00 2001 From: Dhruv Bhanushali Date: Mon, 24 Jun 2024 18:12:53 +0400 Subject: [PATCH 05/10] All media objects to be deleted without affecting past decisions --- .../0067_loosen_media_obj_in_through_model.py | 24 +++++++++++++++++++ api/api/models/audio.py | 3 ++- api/api/models/image.py | 3 ++- api/api/models/media.py | 3 ++- api/latest_migrations/api | 2 +- 5 files changed, 31 insertions(+), 4 deletions(-) create mode 100644 api/api/migrations/0067_loosen_media_obj_in_through_model.py diff --git a/api/api/migrations/0067_loosen_media_obj_in_through_model.py b/api/api/migrations/0067_loosen_media_obj_in_through_model.py new file mode 100644 index 00000000000..b172c747dfd --- /dev/null +++ b/api/api/migrations/0067_loosen_media_obj_in_through_model.py @@ -0,0 +1,24 @@ +# Generated by Django 4.2.11 on 2024-06-24 12:42 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0066_rename_contentprovider_to_contentsource'), + ] + + operations = [ + migrations.AlterField( + model_name='audiodecisionthrough', + name='media_obj', + field=models.ForeignKey(db_column='identifier', db_constraint=False, on_delete=django.db.models.deletion.DO_NOTHING, to='api.audio', to_field='identifier'), + ), + migrations.AlterField( + model_name='imagedecisionthrough', + name='media_obj', + field=models.ForeignKey(db_column='identifier', db_constraint=False, on_delete=django.db.models.deletion.DO_NOTHING, to='api.image', to_field='identifier'), + ), + ] diff --git a/api/api/models/audio.py b/api/api/models/audio.py index c751850e84f..5d36585009f 100644 --- a/api/api/models/audio.py +++ b/api/api/models/audio.py @@ -370,7 +370,8 @@ class AudioDecisionThrough(AbstractMediaDecisionThrough): media_obj = models.ForeignKey( Audio, to_field="identifier", - on_delete=models.CASCADE, + on_delete=models.DO_NOTHING, + db_constraint=False, db_column="identifier", ) decision = models.ForeignKey(AudioDecision, on_delete=models.CASCADE) diff --git a/api/api/models/image.py b/api/api/models/image.py index 2f15dfc62cf..9232fe5c901 100644 --- a/api/api/models/image.py +++ b/api/api/models/image.py @@ -173,7 +173,8 @@ class ImageDecisionThrough(AbstractMediaDecisionThrough): media_obj = models.ForeignKey( Image, to_field="identifier", - on_delete=models.CASCADE, + on_delete=models.DO_NOTHING, + db_constraint=False, db_column="identifier", ) decision = models.ForeignKey(ImageDecision, on_delete=models.CASCADE) diff --git a/api/api/models/media.py b/api/api/models/media.py index 8d98296c3bc..a9f3edca10d 100644 --- a/api/api/models/media.py +++ b/api/api/models/media.py @@ -354,7 +354,8 @@ class AbstractMediaDecisionThrough(models.Model): media_obj = models.ForeignKey( AbstractMedia, to_field="identifier", - on_delete=models.CASCADE, + on_delete=models.DO_NOTHING, + db_constraint=False, db_column="identifier", ) decision = models.ForeignKey(AbstractMediaDecision, on_delete=models.CASCADE) diff --git a/api/latest_migrations/api b/api/latest_migrations/api index 2f6c01764f5..924c953d164 100644 --- a/api/latest_migrations/api +++ b/api/latest_migrations/api @@ -2,4 +2,4 @@ # If you have a merge conflict in this file, it means you need to run: # manage.py makemigrations --merge # in order to resolve the conflict between migrations. -0066_contentsource_delete_contentprovider +0067_do_nothing_in_through_model_media_obj From 8cea25233121243091418330eca781d81a35b9e8 Mon Sep 17 00:00:00 2001 From: Dhruv Bhanushali Date: Mon, 24 Jun 2024 18:14:27 +0400 Subject: [PATCH 06/10] Update tests to account for media items being deleted --- .../management/commands/test_backfillmoderationdecision.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/api/test/unit/management/commands/test_backfillmoderationdecision.py b/api/test/unit/management/commands/test_backfillmoderationdecision.py index 9c6e517dd91..4c4e91cc88d 100644 --- a/api/test/unit/management/commands/test_backfillmoderationdecision.py +++ b/api/test/unit/management/commands/test_backfillmoderationdecision.py @@ -78,12 +78,13 @@ def test_create_moderation_decision_for_reports( assert f"Created 1 {media_type} moderation decisions from existing reports." in out decision = MediaDecision.objects.first() - assert decision.media_objs.count() == 1 assert decision.action == expected_action assert decision.moderator.username == username + assert MediaDecisionThrough.objects.filter(decision=decision).count() == 1 + decision_through = MediaDecisionThrough.objects.first() - assert decision_through.media_obj == report.media_obj + assert str(decision_through.media_obj_id) == report.media_obj_id assert decision_through.decision == decision From 7737df2022f6ba8f425b702f082d3504849918ad Mon Sep 17 00:00:00 2001 From: Dhruv Bhanushali Date: Mon, 24 Jun 2024 21:30:10 +0400 Subject: [PATCH 07/10] Mock ES to prevent breaking other tests --- .../management/commands/test_backfillmoderationdecision.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/api/test/unit/management/commands/test_backfillmoderationdecision.py b/api/test/unit/management/commands/test_backfillmoderationdecision.py index 4c4e91cc88d..d4b66ebffd0 100644 --- a/api/test/unit/management/commands/test_backfillmoderationdecision.py +++ b/api/test/unit/management/commands/test_backfillmoderationdecision.py @@ -1,4 +1,5 @@ from io import StringIO +from unittest.mock import patch from django.core.management import call_command @@ -44,6 +45,7 @@ def make_reports(media_type, reason: str, status: str, count: int = 1): return ImageReportFactory.create_batch(count, status=status, reason=reason) +@patch("django.conf.settings.ES") @pytest.mark.parametrize( ("reason", "status", "expected_action"), ( @@ -61,7 +63,7 @@ def make_reports(media_type, reason: str, status: str, count: int = 1): @pytest.mark.parametrize(("media_type"), ("image", "audio")) @pytest.mark.django_db def test_create_moderation_decision_for_reports( - media_type, reason, status, expected_action + mock_es, media_type, reason, status, expected_action ): username = "opener" UserFactory.create(username=username) From c5adb41febf5a33c5dc4b53ab2b53c019508b0ee Mon Sep 17 00:00:00 2001 From: Dhruv Bhanushali Date: Tue, 25 Jun 2024 20:25:18 +0400 Subject: [PATCH 08/10] Undo fixes to the backfill command --- .../management/commands/backfillmoderationdecision.py | 6 +----- .../commands/test_backfillmoderationdecision.py | 9 +++------ 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/api/api/management/commands/backfillmoderationdecision.py b/api/api/management/commands/backfillmoderationdecision.py index 3a755360eee..e7d704e6d0b 100644 --- a/api/api/management/commands/backfillmoderationdecision.py +++ b/api/api/management/commands/backfillmoderationdecision.py @@ -92,7 +92,7 @@ def handle(self, *args, **options): for report, decision in zip(reports_chunk, decisions): report.decision = decision MediaReport.objects.bulk_update(reports_chunk, ["decision"]) - throughs = MediaDecisionThrough.objects.bulk_create( + MediaDecisionThrough.objects.bulk_create( [ MediaDecisionThrough( media_obj_id=report.media_obj_id, decision=decision @@ -100,10 +100,6 @@ def handle(self, *args, **options): for report, decision in zip(reports_chunk, decisions) ] ) - # Bulk create does not call ``save()`` so the mark-sensitive/deindex - # actions will not be undertaken. - for through in throughs: - through.perform_action() t.update(1) t.info( diff --git a/api/test/unit/management/commands/test_backfillmoderationdecision.py b/api/test/unit/management/commands/test_backfillmoderationdecision.py index d4b66ebffd0..9c6e517dd91 100644 --- a/api/test/unit/management/commands/test_backfillmoderationdecision.py +++ b/api/test/unit/management/commands/test_backfillmoderationdecision.py @@ -1,5 +1,4 @@ from io import StringIO -from unittest.mock import patch from django.core.management import call_command @@ -45,7 +44,6 @@ def make_reports(media_type, reason: str, status: str, count: int = 1): return ImageReportFactory.create_batch(count, status=status, reason=reason) -@patch("django.conf.settings.ES") @pytest.mark.parametrize( ("reason", "status", "expected_action"), ( @@ -63,7 +61,7 @@ def make_reports(media_type, reason: str, status: str, count: int = 1): @pytest.mark.parametrize(("media_type"), ("image", "audio")) @pytest.mark.django_db def test_create_moderation_decision_for_reports( - mock_es, media_type, reason, status, expected_action + media_type, reason, status, expected_action ): username = "opener" UserFactory.create(username=username) @@ -80,13 +78,12 @@ def test_create_moderation_decision_for_reports( assert f"Created 1 {media_type} moderation decisions from existing reports." in out decision = MediaDecision.objects.first() + assert decision.media_objs.count() == 1 assert decision.action == expected_action assert decision.moderator.username == username - assert MediaDecisionThrough.objects.filter(decision=decision).count() == 1 - decision_through = MediaDecisionThrough.objects.first() - assert str(decision_through.media_obj_id) == report.media_obj_id + assert decision_through.media_obj == report.media_obj assert decision_through.decision == decision From fb8495fbae85971003415c1ff82ebf9cc7e2af4c Mon Sep 17 00:00:00 2001 From: Dhruv Bhanushali Date: Tue, 25 Jun 2024 20:30:29 +0400 Subject: [PATCH 09/10] Avoid unnecessary queries --- api/api/models/media.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/api/api/models/media.py b/api/api/models/media.py index c44fe4f2a32..fc5c290b09d 100644 --- a/api/api/models/media.py +++ b/api/api/models/media.py @@ -363,17 +363,19 @@ class AbstractMediaDecisionThrough(models.Model): class Meta: abstract = True - def perform_action(self): + def perform_action(self, action=None): """Perform the action specified in the decision.""" - if self.decision.action in { + action = self.decision.action if action is None else action + + if action in { DecisionAction.DEINDEXED_SENSITIVE, DecisionAction.DEINDEXED_COPYRIGHT, }: - self.deleted_media_class.objects.create(media_obj=self.media_obj) + self.deleted_media_class.objects.create(media_obj_id=self.media_obj_id) - if self.decision.action == DecisionAction.MARKED_SENSITIVE: - self.sensitive_media_class.objects.create(media_obj=self.media_obj) + if action == DecisionAction.MARKED_SENSITIVE: + self.sensitive_media_class.objects.create(media_obj_id=self.media_obj_id) def save(self, *args, **kwargs): super().save(*args, **kwargs) From 63ce31c315d38a959d92c7f0aaf60f7fe5a04bdf Mon Sep 17 00:00:00 2001 From: Dhruv Bhanushali Date: Tue, 25 Jun 2024 21:31:20 +0400 Subject: [PATCH 10/10] Add helpful messages when redirecting --- api/api/admin/media_report.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/api/api/admin/media_report.py b/api/api/admin/media_report.py index 66fc7b10be9..f441e6665c6 100644 --- a/api/api/admin/media_report.py +++ b/api/api/admin/media_report.py @@ -16,6 +16,7 @@ from elasticsearch import NotFoundError from elasticsearch_dsl import Search +from api.constants.moderation import DecisionAction from api.models import ( Audio, AudioDecision, @@ -377,6 +378,9 @@ def change_view(self, request, object_id, form_url="", extra_context=None): media_obj = self.get_object(request, object_id) if media_obj: extra_context["media_obj"] = media_obj + else: + messages.warning(request, f"No media object found with ID {object_id}.") + return redirect(f"admin:api_{self.media_type}_changelist") tags_by_provider = {} if tags := media_obj.tags: @@ -476,6 +480,22 @@ def decision_create_view(self, request, object_id): report_count=count, decision=decision.id, ) + + if decision.action in { + DecisionAction.DEINDEXED_COPYRIGHT, + DecisionAction.DEINDEXED_SENSITIVE, + }: + messages.info( + request, + "The media object has been deindexed from ES and deleted from DB.", + ) + return redirect(f"admin:api_{self.media_type}_changelist") + + if decision.action == DecisionAction.MARKED_SENSITIVE: + messages.info( + request, + "The media object has been marked as sensitive.", + ) else: logger.warning( "Form is invalid",