Expose all the reasons why there is a due_date to the extensions queue (queryset) (#22530)

* set due_date_reasons bitmask when setting due_date

* add no_transforms in Version.should_have_due_date

* Only annotate Version qs when nessecary

* refactor to make VersionManager.should_have_due_date efficient

* Add flags for newly exposed reasons for due_date

* get_due_date_reason_qs -> get_due_date_reason_q_objects; add docstring

* exclude has developer reply from needs_human_review_other

* fix linelength that ruff format didn't autofix

src/olympia/addons/models.py

@@ -307,8 +307,6 @@ class AddonManager(ManagerBase):
         show_temporarily_delayed=True,
         show_only_upcoming=False,
     ):
-        from olympia.reviewers.models import NeedsHumanReview  # circular reference
-
         filters = {
             'type__in': amo.GROUP_TYPE_THEME if theme_review else amo.GROUP_TYPE_ADDON,
             'versions__due_date__isnull': False,
@@ -367,24 +365,12 @@ class AddonManager(ManagerBase):
                 first_version_id=Subquery(
                     versions_due_qs.filter(addon=OuterRef('pk')).values('pk')[:1]
                 ),
-                needs_human_review_from_cinder=Exists(
+                **{
-                    versions_due_qs.filter(
+                    name: Exists(versions_due_qs.filter(q))
-                        needshumanreview__is_active=True,
+                    for name, q in (
-                        needshumanreview__reason=NeedsHumanReview.REASONS.CINDER_ESCALATION,
+                        Version.unfiltered.get_due_date_reason_q_objects().items()
                     )
-                ),
+                },
-                needs_human_review_from_abuse=Exists(
-                    versions_due_qs.filter(
-                        needshumanreview__is_active=True,
-                        needshumanreview__reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION,
-                    )
-                ),
-                needs_human_review_from_appeal=Exists(
-                    versions_due_qs.filter(
-                        needshumanreview__is_active=True,
-                        needshumanreview__reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL,
-                    )
-                ),
             )
             .filter(first_version_id__isnull=False)
             .transform(first_pending_version_transformer)

src/olympia/addons/tests/test_models.py

@@ -3600,9 +3600,14 @@ class TestExtensionsQueues(TestCase):
             nhr_abuse_inactive,
         }
         assert getattr(addons[nhr_abuse.id], annotated_field)
-        assert not getattr(addons[nhr_other.id], annotated_field)
+        if annotated_field != 'needs_human_review_other':
-        assert not getattr(addons[nhr_without_due_date.id], annotated_field)
+            assert not getattr(addons[nhr_other.id], annotated_field)
-        assert not getattr(addons[nhr_abuse_inactive.id], annotated_field)
+            assert not getattr(addons[nhr_without_due_date.id], annotated_field)
+            assert not getattr(addons[nhr_abuse_inactive.id], annotated_field)
+        else:
+            assert getattr(addons[nhr_other.id], annotated_field)
+            assert getattr(addons[nhr_without_due_date.id], annotated_field)
+            assert getattr(addons[nhr_abuse_inactive.id], annotated_field)
 
     def test_pending_queue_needs_human_review_from_abuse(self):
         self._test_pending_queue_needs_human_review_from(
@@ -3621,6 +3626,11 @@ class TestExtensionsQueues(TestCase):
             NeedsHumanReview.REASONS.CINDER_ESCALATION, 'needs_human_review_from_cinder'
         )
 
+    def test_pending_queue_needs_human_review_other(self):
+        self._test_pending_queue_needs_human_review_from(
+            NeedsHumanReview.REASONS.SCANNER_ACTION, 'needs_human_review_other'
+        )
+
     def test_get_pending_rejection_queue(self):
         expected_addons = [
             version_review_flags_factory(

src/olympia/reviewers/models.py

@@ -37,7 +37,10 @@ VIEW_QUEUE_FLAGS = (
         'needs_admin_theme_review',
         'Needs Admin Static Theme Review',
     ),
-    ('sources_provided', 'Source Code Provided'),
+    (
+        'sources_provided',
+        'Source Code Provided',
+    ),
     (
         'auto_approval_disabled',
         'Auto-approval disabled',
@@ -62,6 +65,32 @@ VIEW_QUEUE_FLAGS = (
         'auto_approval_delayed_indefinitely_unlisted',
         'Unlisted Auto-approval delayed indefinitely',
     ),
+    # The following are annotations set by AddonManager.get_queryset_for_pending_queues
+    # See VersionManager.get_due_date_reason_q_objects for the names
+    (
+        'needs_human_review_from_cinder',
+        'Abuse report forwarded from Cinder present',
+    ),
+    (
+        'needs_human_review_from_abuse',
+        'Abuse report present',
+    ),
+    (
+        'needs_human_review_from_appeal',
+        'Appeal on decision present',
+    ),
+    (
+        'needs_human_review_other',
+        'Other NeedsHumanReview flag set',
+    ),
+    (
+        'is_pre_review_version',
+        'Version(s) awaiting pre-approval review',
+    ),
+    (
+        'has_developer_reply',
+        'Outstanding developer reply',
+    ),
 )
 
 
@@ -88,7 +117,7 @@ def set_reviewing_cache(addon_id, user_id):
 def get_flags(addon, version):
     """Return a list of tuples (indicating which flags should be displayed for
     a particular add-on."""
-    flag_filters_by_channel = {
+    exclude_flags_by_channel = {
         amo.CHANNEL_UNLISTED: (
             'auto_approval_disabled',
             'auto_approval_delayed_temporarily',
@@ -105,22 +134,11 @@ def get_flags(addon, version):
         for (prop, title) in VIEW_QUEUE_FLAGS
         if getattr(version, prop, getattr(addon, prop, None))
         and prop
-        not in flag_filters_by_channel.get(getattr(version, 'channel', None), ())
+        not in exclude_flags_by_channel.get(getattr(version, 'channel', None), ())
     ]
     # add in the promoted group flag and return
     if promoted := addon.promoted_group(currently_approved=False):
         flags.append((f'promoted-{promoted.api_name}', promoted.name))
-    if getattr(addon, 'needs_human_review_from_cinder', False):
-        flags.append(
-            (
-                'needs-human-review-from-cinder',
-                'Abuse report forwarded from Cinder present',
-            )
-        )
-    if getattr(addon, 'needs_human_review_from_abuse', False):
-        flags.append(('needs-human-review-from-abuse', 'Abuse report present'))
-    if getattr(addon, 'needs_human_review_from_appeal', False):
-        flags.append(('needs-human-review-from-appeal', 'Appeal on decision present'))
     return flags
 
 

src/olympia/reviewers/tests/test_models.py

@@ -32,6 +32,7 @@ from olympia.files.models import File, FileValidation, WebextPermission
 from olympia.promoted.models import PromotedAddon
 from olympia.ratings.models import Rating
 from olympia.reviewers.models import (
+    VIEW_QUEUE_FLAGS,
     AutoApprovalNoValidationResultError,
     AutoApprovalSummary,
     NeedsHumanReview,
@@ -1704,26 +1705,42 @@ class TestGetFlags(TestCase):
     def test_version_none(self):
         assert get_flags(self.addon, None) == []
 
-    def test_needs_human_review_abuse_flag(self):
+    def test_due_date_reason_flags(self):
         assert get_flags(self.addon, self.addon.current_version) == []
         self.addon.needs_human_review_from_abuse = False
         self.addon.needs_human_review_from_cinder = False
         self.addon.needs_human_review_from_appeal = False
+        self.addon.needs_human_review_other = False
+        self.addon.is_pre_review_version = False
+        self.addon.has_developer_reply = False
         assert get_flags(self.addon, self.addon.current_version) == []
         for attribute, title in (
-            ('cinder', 'Abuse report forwarded from Cinder present'),
+            (
-            ('abuse', 'Abuse report present'),
+                'needs_human_review_from_cinder',
-            ('appeal', 'Appeal on decision present'),
+                'Abuse report forwarded from Cinder present',
+            ),
+            ('needs_human_review_from_abuse', 'Abuse report present'),
+            ('needs_human_review_from_appeal', 'Appeal on decision present'),
+            ('needs_human_review_other', 'Other NeedsHumanReview flag set'),
+            ('is_pre_review_version', 'Version(s) awaiting pre-approval review'),
+            ('has_developer_reply', 'Outstanding developer reply'),
         ):
             self.addon.needs_human_review_from_abuse = False
             self.addon.needs_human_review_from_cinder = False
             self.addon.needs_human_review_from_appeal = False
-            attribute = 'needs_human_review_from_' + attribute
+            self.addon.needs_human_review_other = False
+            self.addon.is_pre_review_version = False
+            self.addon.has_developer_reply = False
             setattr(self.addon, attribute, True)
             assert get_flags(self.addon, self.addon.current_version) == [
                 (attribute.replace('_', '-'), title)
             ]
 
+    def test_all_due_due_reasons_exposed_as_flags(self):
+        assert set(Version.objects.get_due_date_reason_q_objects().keys()).issubset(
+            {flag for flag, _ in VIEW_QUEUE_FLAGS}
+        )
+
 
 class TestNeedsHumanReview(TestCase):
     def setUp(self):

src/olympia/versions/models.py

@@ -1,3 +1,5 @@
+import functools
+import operator
 import os
 import re
 from base64 import b64encode
@@ -176,17 +178,31 @@ class VersionManager(ManagerBase):
         """Returns a queryset filtered to versions that should have a due date set.
         If `negate=True` the queryset will contain versions that should not have a
         due date instead."""
+        method = getattr(self, 'exclude' if negate else 'filter')
+        return (
+            method(
+                functools.reduce(
+                    operator.or_, self.get_due_date_reason_q_objects().values()
+                )
+            )
+            .using('default')
+            .distinct()
+        )
+
+    def get_due_date_reason_q_objects(cls):
+        """Class method that returns a dict with all the Q objects needed to determine
+        if a version should_have_due_date is true as values, and the annotation names
+        for each reason as values."""
         from olympia.reviewers.models import NeedsHumanReview
 
-        method = getattr(self, 'exclude' if negate else 'filter')
         requires_manual_listed_approval_and_is_listed = Q(
             Q(addon__reviewerflags__auto_approval_disabled=True)
             | Q(addon__reviewerflags__auto_approval_disabled_until_next_approval=True)
             | Q(addon__reviewerflags__auto_approval_delayed_until__isnull=False)
             | Q(
-                addon__promotedaddon__group_id__in=(
+                addon__promotedaddon__group_id__in=[
                     g.id for g in PROMOTED_GROUPS if g.listed_pre_review
-                )
+                ]
             )
             | Q(addon__type__in=amo.GROUP_TYPE_THEME),
             addon__status__in=(amo.VALID_ADDON_STATUSES),
@@ -201,9 +217,9 @@ class VersionManager(ManagerBase):
                 addon__reviewerflags__auto_approval_delayed_until_unlisted__isnull=False
             )
             | Q(
-                addon__promotedaddon__group_id__in=(
+                addon__promotedaddon__group_id__in=[
                     g.id for g in PROMOTED_GROUPS if g.unlisted_pre_review
-                )
+                ]
             )
             | Q(addon__type__in=amo.GROUP_TYPE_THEME),
             channel=amo.CHANNEL_UNLISTED,
@@ -232,11 +248,33 @@ class VersionManager(ManagerBase):
             needshumanreview__is_active=True,
             needshumanreview__reason=NeedsHumanReview.REASONS.DEVELOPER_REPLY,
         )
-        return (
+        return {
-            method(is_needs_human_review | is_pre_review_version | has_developer_reply)
+            'needs_human_review_from_cinder': Q(
-            .using('default')
+                is_needs_human_review,
-            .distinct()
+                needshumanreview__reason=NeedsHumanReview.REASONS.CINDER_ESCALATION,
-        )
+            ),
+            'needs_human_review_from_abuse': Q(
+                is_needs_human_review,
+                needshumanreview__reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION,
+            ),
+            'needs_human_review_from_appeal': Q(
+                is_needs_human_review,
+                needshumanreview__reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL,
+            ),
+            'needs_human_review_other': Q(
+                is_needs_human_review,
+                ~Q(
+                    needshumanreview__reason__in=(
+                        NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION.value,
+                        NeedsHumanReview.REASONS.CINDER_ESCALATION.value,
+                        NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL.value,
+                    )
+                )
+                & ~has_developer_reply,
+            ),
+            'is_pre_review_version': is_pre_review_version,
+            'has_developer_reply': has_developer_reply,
+        }
 
 
 class UnfilteredVersionManagerForRelations(VersionManager):

src/olympia/versions/tests/test_models.py

@@ -393,6 +393,112 @@ class TestVersionManager(TestCase):
         version_preview = VersionPreview.objects.get(pk=version_preview.pk)
         assert version_preview.version == version
 
+    def test_should_have_due_date(self):
+        user_factory(pk=settings.TASK_USER_ID)
+        addon_kws = {
+            'file_kw': {'is_signed': True, 'status': amo.STATUS_AWAITING_REVIEW}
+        }
+
+        addon_factory(**addon_kws)  # no due_date
+
+        other_nhr = addon_factory(**addon_kws).current_version
+        # having the needs_human_review flag means a due dute is needed
+        NeedsHumanReview.objects.create(
+            version=other_nhr, reason=NeedsHumanReview.REASONS.SCANNER_ACTION
+        )
+
+        # Or if it's in a pre-review promoted group it will.
+        recommended = addon_factory(**addon_kws).current_version
+        PromotedAddon.objects.create(addon=recommended.addon, group_id=RECOMMENDED.id)
+
+        # And not if it's a non-pre-review group
+        PromotedAddon.objects.create(
+            addon=addon_factory(**addon_kws), group_id=STRATEGIC.id
+        )
+
+        # A disabled version with a developer reply
+        developer_reply = addon_factory(
+            file_kw={'is_signed': False, 'status': amo.STATUS_DISABLED}
+        ).versions.all()[0]
+        NeedsHumanReview.objects.create(
+            version=developer_reply, reason=NeedsHumanReview.REASONS.DEVELOPER_REPLY
+        )
+
+        # dsa related needs_human_review flag means due dates are needed also
+        abuse_nhr = addon_factory(**addon_kws).current_version
+        NeedsHumanReview.objects.create(
+            version=abuse_nhr, reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION
+        )
+        appeal_nhr = addon_factory(**addon_kws).current_version
+        NeedsHumanReview.objects.create(
+            version=appeal_nhr, reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL
+        )
+        # throw in an inactive NHR that should be ignored
+        NeedsHumanReview.objects.create(
+            version=appeal_nhr,
+            reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION,
+            is_active=False,
+        )
+
+        # And a version with multiple reasons
+        multiple = addon_factory(**addon_kws).current_version
+        PromotedAddon.objects.create(addon=multiple.addon, group_id=LINE.id)
+        NeedsHumanReview.objects.create(
+            version=multiple, reason=NeedsHumanReview.REASONS.DEVELOPER_REPLY
+        )
+        NeedsHumanReview.objects.create(
+            version=multiple, reason=NeedsHumanReview.REASONS.CINDER_ESCALATION
+        )
+
+        qs = Version.objects.should_have_due_date().order_by('id')
+        assert list(qs) == [
+            # absent addon with nothing special set
+            other_nhr,
+            recommended,
+            # absent promoted but not prereview addon
+            developer_reply,
+            abuse_nhr,
+            appeal_nhr,
+            multiple,
+        ]
+
+    def test_get_due_date_reason_q_objects(self):
+        self.test_should_have_due_date()  # to set up the Versions
+
+        qs = Version.objects.all().order_by('id')
+        # See test_should_have_due_date for order
+        (
+            _,  # addon with nothing special set
+            other_nhr,
+            recommended,
+            _,  # promoted but not prereview addon
+            developer_reply,
+            abuse_nhr,
+            appeal_nhr,
+            multiple,
+        ) = list(qs)
+
+        q_objects = Version.objects.get_due_date_reason_q_objects()
+        method = Version.objects.filter
+
+        assert list(method(q_objects['needs_human_review_from_cinder'])) == [multiple]
+
+        assert list(method(q_objects['needs_human_review_from_abuse'])) == [abuse_nhr]
+
+        assert list(method(q_objects['needs_human_review_from_appeal'])) == [appeal_nhr]
+
+        assert list(method(q_objects['needs_human_review_other'])) == [other_nhr]
+
+        assert list(method(q_objects['is_pre_review_version'])) == [
+            multiple,
+            recommended,
+        ]
+
+        assert list(method(q_objects['has_developer_reply'])) == [
+            multiple,
+            developer_reply,
+        ]
+
 
 class TestVersion(AMOPaths, TestCase):
     fixtures = ['base/addon_3615', 'base/admin']

static/css/zamboni/reviewers.less

@@ -36,8 +36,9 @@
 .ed-sprite-needs-admin-content-review { background-position: 0 -128px; }
 .ed-sprite-needs-admin-code-review { background-position: 0 -128px; }
 .ed-sprite-needs-admin-theme-review { background-position: 0 -128px; }
-.ed-sprite-reviewer { background-position: 0 -144px; }
+.ed-sprite-has-developer-reply { background-position: 0 -144px; }
-.ed-sprite-info { background-position: 0 -160px; }
+.ed-sprite-needs-human-review-other { background-position: 0 -160px; }
+.ed-sprite-is-pre-review-version { background-position: 0 -160px; }
 .ed-sprite-sources-provided { background-position: 0 -192px; }
 .ed-sprite-webextension { background-position: 0 -208px; }
 .ed-sprite-expired-info { background-position: 0 -272px; }