From 5ae9b4958785246a280933d1df6bffca4bd1feef Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Mon, 25 Nov 2024 16:37:59 +0000 Subject: [PATCH] flag current version for NHR if available --- src/olympia/abuse/cinder.py | 23 +++++++++---- src/olympia/abuse/models.py | 20 +++++------ src/olympia/abuse/tasks.py | 2 +- src/olympia/abuse/tests/test_cinder.py | 43 ++++++++++++++++------- src/olympia/abuse/tests/test_models.py | 47 +++++++++++--------------- 5 files changed, 76 insertions(+), 59 deletions(-) diff --git a/src/olympia/abuse/cinder.py b/src/olympia/abuse/cinder.py index 5660bd90c646..d063ba8bbefc 100644 --- a/src/olympia/abuse/cinder.py +++ b/src/olympia/abuse/cinder.py @@ -495,6 +495,11 @@ def queue_appeal(self): return self.queue def flag_for_human_review(self, *, related_versions, appeal=False, forwarded=False): + """Flag an appropriate version for needs human review so it appears in reviewers + manual revew queue. + + Note: Keep the logic here in sync with `is_individually_actionable_q` - if a + report is individually actionable we must be able to flag for review.""" from olympia.reviewers.models import NeedsHumanReview waffle_switch_name = ( @@ -529,15 +534,21 @@ def flag_for_human_review(self, *, related_versions, appeal=False, forwarded=Fal if related_versions else set() ) - # If we have more versions specified than versions we flagged, flag latest + # If we have more versions specified than versions we flagged, flag current # to be safe. (Either because there was an unknown version, or a None) if len(version_objs) != len(related_versions) or len(related_versions) == 0: - version_objs.add( - self.addon.versions(manager='unfiltered_for_relations') - .filter(channel=amo.CHANNEL_LISTED) - .no_transforms() - .first() + latest_or_current = self.addon.current_version or ( + # for an appeal there may not be a current version, so look for others. + appeal + and ( + self.addon.versions(manager='unfiltered_for_relations') + .filter(channel=amo.CHANNEL_LISTED) + .no_transforms() + .first() + ) ) + if latest_or_current: + version_objs.add(latest_or_current) version_objs = sorted(version_objs, key=lambda v: v.id) log.debug( 'Found %s versions potentially needing NHR [%s]', diff --git a/src/olympia/abuse/models.py b/src/olympia/abuse/models.py index 6792c31430f1..fda2f13e058f 100644 --- a/src/olympia/abuse/models.py +++ b/src/olympia/abuse/models.py @@ -439,16 +439,11 @@ def for_addon(self, addon): return self.get_queryset().for_addon(addon) @classmethod - def is_individually_actionable_q(cls, *, assume_guid_exists=False): + def is_individually_actionable_q(cls): """A Q object to filter on Abuse reports reportable under DSA, so should be sent - to Cinder. - - Set assume_guid_exists=True as an optimization when you're filtering using guids - you know exist.""" - addon_guid_exists = ( - Q() - if assume_guid_exists - else Exists(Addon.unfiltered.filter(guid=OuterRef('guid'))) + to Cinder.""" + current_version = Addon.objects.filter( + guid=OuterRef('guid'), _current_version__isnull=False ) listed_version = Version.unfiltered.filter( addon__guid=OuterRef('guid'), @@ -456,11 +451,12 @@ def is_individually_actionable_q(cls, *, assume_guid_exists=False): channel=amo.CHANNEL_LISTED, ) not_addon = Q(guid__isnull=True) - version_null_or_exists = ( - Q(addon_version='') | Q(addon_version__isnull=True) | Exists(listed_version) + current_or_version_exists = Exists(listed_version) | ( + (Q(addon_version='') | Q(addon_version__isnull=True)) + & Exists(current_version) ) return Q( - not_addon | Q(addon_guid_exists & version_null_or_exists), + not_addon | current_or_version_exists, reason__in=AbuseReport.REASONS.INDIVIDUALLY_ACTIONABLE_REASONS.values, ) diff --git a/src/olympia/abuse/tasks.py b/src/olympia/abuse/tasks.py index da3bf36a771b..c73ebd20b30d 100644 --- a/src/olympia/abuse/tasks.py +++ b/src/olympia/abuse/tasks.py @@ -50,7 +50,7 @@ def flag_high_abuse_reports_addons_according_to_review_tier(): abuse_reports_count_qs = ( AbuseReport.objects.values('guid') .filter( - ~AbuseReportManager.is_individually_actionable_q(assume_guid_exists=True), + ~AbuseReportManager.is_individually_actionable_q(), guid=OuterRef('guid'), created__gte=datetime.now() - timedelta(days=14), ) diff --git a/src/olympia/abuse/tests/test_cinder.py b/src/olympia/abuse/tests/test_cinder.py index 1702fe0b01ef..782cd4d657ac 100644 --- a/src/olympia/abuse/tests/test_cinder.py +++ b/src/olympia/abuse/tests/test_cinder.py @@ -1133,19 +1133,18 @@ def test_report(self): assert addon.current_version.needshumanreview_set.count() == 0 job = CinderJob.objects.create(job_id='1234-xyz') cinder_instance = self.CinderClass(addon) - with self.assertNumQueries(12): + with self.assertNumQueries(11): # - 1 Fetch Cinder Decision - # - 2 Fetch Version - # - 3 Fetch NeedsHumanReview - # - 4 Create NeedsHumanReview - # - 5 Query if due_date is needed for version - # - 6 Query existing versions for due dates to inherit - # - 7 Update due date on Version - # - 8 Fetch task user - # - 9 Create ActivityLog - # - 10 Update ActivityLog - # - 11 Create ActivityLogComment - # - 12 Create VersionLog + # - 2 Fetch NeedsHumanReview + # - 3 Create NeedsHumanReview + # - 4 Query if due_date is needed for version + # - 5 Query existing versions for due dates to inherit + # - 6 Update due date on Version + # - 7 Fetch task user + # - 8 Create ActivityLog + # - 9 Update ActivityLog + # - 10 Create ActivityLogComment + # - 11 Create VersionLog cinder_instance.post_report(job) assert ( addon.current_version.needshumanreview_set.get().reason @@ -1222,7 +1221,9 @@ def test_appeal_logged_in(self): def test_appeal_specific_version(self): addon = self._create_dummy_target() other_version = version_factory( - addon=addon, file_kw={'status': amo.STATUS_AWAITING_REVIEW} + addon=addon, + channel=amo.CHANNEL_UNLISTED, + file_kw={'status': amo.STATUS_AWAITING_REVIEW}, ) self._test_appeal( CinderUser(user_factory()), @@ -1236,6 +1237,22 @@ def test_appeal_specific_version(self): assert not addon.current_version.reload().due_date assert other_version.reload().due_date + def test_appeal_no_current_version(self): + addon = self._create_dummy_target( + status=amo.STATUS_NULL, file_kw={'status': amo.STATUS_DISABLED} + ) + version = addon.versions.last() + assert not addon.current_version + self._test_appeal( + CinderUser(user_factory()), + self.CinderClass(addon), + ) + assert ( + version.needshumanreview_set.get().reason + == NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL + ) + assert version.reload().due_date + @override_switch('dsa-appeals-review', active=False) def test_appeal_waffle_switch_off(self): addon = self._create_dummy_target() diff --git a/src/olympia/abuse/tests/test_models.py b/src/olympia/abuse/tests/test_models.py index 1c3e36e17100..9d5ca2416c20 100644 --- a/src/olympia/abuse/tests/test_models.py +++ b/src/olympia/abuse/tests/test_models.py @@ -612,31 +612,30 @@ def test_for_addon_finds_by_original_guid(self): assert list(AbuseReport.objects.for_addon(addon)) == [report] def test_is_individually_actionable_q(self): + actionable_reason = AbuseReport.REASONS.HATEFUL_VIOLENT_DECEPTIVE user = user_factory() addon = addon_factory(guid='@lol') addon_report = AbuseReport.objects.create( - guid=addon.guid, reason=AbuseReport.REASONS.HATEFUL_VIOLENT_DECEPTIVE - ) - user_report = AbuseReport.objects.create( - user=user, reason=AbuseReport.REASONS.HATEFUL_VIOLENT_DECEPTIVE + guid=addon.guid, reason=actionable_reason ) + user_report = AbuseReport.objects.create(user=user, reason=actionable_reason) collection_report = AbuseReport.objects.create( collection=collection_factory(), - reason=AbuseReport.REASONS.HATEFUL_VIOLENT_DECEPTIVE, + reason=actionable_reason, ) rating_report = AbuseReport.objects.create( rating=Rating.objects.create(user=user, addon=addon, rating=5), - reason=AbuseReport.REASONS.HATEFUL_VIOLENT_DECEPTIVE, + reason=actionable_reason, ) listed_version_report = AbuseReport.objects.create( guid=addon.guid, addon_version=addon.current_version.version, - reason=AbuseReport.REASONS.HATEFUL_VIOLENT_DECEPTIVE, + reason=actionable_reason, ) listed_deleted_version_report = AbuseReport.objects.create( guid=addon.guid, addon_version=version_factory(addon=addon, deleted=True).version, - reason=AbuseReport.REASONS.HATEFUL_VIOLENT_DECEPTIVE, + reason=actionable_reason, ) # some reports that aren't individually actionable: @@ -653,22 +652,30 @@ def test_is_individually_actionable_q(self): reason=AbuseReport.REASONS.FEEDBACK_SPAM, ) # guid doesn't exist - missing_addon_report = AbuseReport.objects.create( - guid='dfdf', reason=AbuseReport.REASONS.HATEFUL_VIOLENT_DECEPTIVE - ) + AbuseReport.objects.create(guid='dfdf', reason=actionable_reason) # unlisted version AbuseReport.objects.create( guid=addon.guid, addon_version=version_factory( addon=addon, channel=amo.CHANNEL_UNLISTED ).version, - reason=AbuseReport.REASONS.HATEFUL_VIOLENT_DECEPTIVE, + reason=actionable_reason, ) # invalid version AbuseReport.objects.create( guid=addon.guid, addon_version='123456', - reason=AbuseReport.REASONS.HATEFUL_VIOLENT_DECEPTIVE, + reason=actionable_reason, + ) + # no version specified for addon with only unlisted versions + AbuseReport.objects.create( + guid=addon_factory(version_kw={'channel': amo.CHANNEL_UNLISTED}).guid, + reason=actionable_reason, + ) + # no version specified for addon with no public versions + AbuseReport.objects.create( + guid=addon_factory(file_kw={'status': amo.STATUS_DISABLED}).guid, + reason=actionable_reason, ) assert set( @@ -684,20 +691,6 @@ def test_is_individually_actionable_q(self): listed_deleted_version_report, } - assert set( - AbuseReport.objects.filter( - AbuseReportManager.is_individually_actionable_q(assume_guid_exists=True) - ) - ) == { - addon_report, - collection_report, - user_report, - rating_report, - listed_version_report, - listed_deleted_version_report, - missing_addon_report, - } - class TestCinderJobManager(TestCase): def test_for_addon(self):