flag current version for NHR if available (#22894)

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]',

src/olympia/abuse/models.py

@@ -439,28 +439,24 @@ 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'),
             version=OuterRef('addon_version'),
             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,
         )
 

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),
         )

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()

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):