refactor to make VersionManager.should_have_due_date efficient

src/olympia/addons/models.py

@@ -319,8 +319,7 @@ def get_queryset_for_pending_queues(
             select_related_fields_for_listed=False,
         )
         versions_due_qs = (
-            Version.unfiltered.annotate_due_date_reasons()
-            .filter(due_date__isnull=False, addon=OuterRef('pk'))
+            Version.unfiltered.filter(due_date__isnull=False, addon=OuterRef('pk'))
             .no_transforms()
             .order_by('due_date')
         )
@@ -366,15 +365,10 @@ def get_queryset_for_pending_queues(
                 first_version_id=Subquery(
                     versions_due_qs.filter(addon=OuterRef('pk')).values('pk')[:1]
                 ),
-                needs_human_review_from_cinder=Exists(
-                    versions_due_qs.filter(needs_human_review_from_cinder=True)
-                ),
-                needs_human_review_from_abuse=Exists(
-                    versions_due_qs.filter(needs_human_review_from_abuse=True)
-                ),
-                needs_human_review_from_appeal=Exists(
-                    versions_due_qs.filter(needs_human_review_from_appeal=True)
-                ),
+                **{
+                    name: Exists(versions_due_qs.filter(q))
+                    for name, q in Version.unfiltered.get_due_date_reason_qs().items()
+                },
             )
             .filter(first_version_id__isnull=False)
             .transform(first_pending_version_transformer)

src/olympia/addons/tests/test_models.py

@@ -3600,9 +3600,14 @@ def _test_pending_queue_needs_human_review_from(self, reason, annotated_field):
             nhr_abuse_inactive,
         }
         assert getattr(addons[nhr_abuse.id], annotated_field)
-        assert not getattr(addons[nhr_other.id], annotated_field)
-        assert not getattr(addons[nhr_without_due_date.id], annotated_field)
-        assert not getattr(addons[nhr_abuse_inactive.id], annotated_field)
+        if annotated_field != 'needs_human_review_other':
+            assert not getattr(addons[nhr_other.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 @@ def test_pending_queue_needs_human_review_from_cinder(self):
             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/versions/models.py

@@ -1,3 +1,5 @@
+import functools
+import operator
 import os
 import re
 from base64 import b64encode
@@ -9,7 +11,7 @@
 from django.core.exceptions import ObjectDoesNotExist
 from django.core.files.storage import default_storage as storage
 from django.db import models, transaction
-from django.db.models import Case, Exists, F, OuterRef, Q, When
+from django.db.models import Case, F, Q, When
 from django.dispatch import receiver
 from django.urls import reverse
 from django.utils.encoding import force_str
@@ -176,23 +178,16 @@ def should_have_due_date(self, negate=False):
         """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.annotate_due_date_reasons(), 'exclude' if negate else 'filter'
-        )
+        method = getattr(self, 'exclude' if negate else 'filter')
         return (
             method(
-                Q(needs_human_review_from_cinder=True)
-                | Q(needs_human_review_from_abuse=True)
-                | Q(needs_human_review_from_appeal=True)
-                | Q(needs_human_review_other=True)
-                | Q(is_pre_review_version=True)
-                | Q(has_developer_reply=True)
+                functools.reduce(operator.or_, self.get_due_date_reason_qs().values())
             )
             .using('default')
             .distinct()
         )
 
-    def annotate_due_date_reasons(self):
+    def get_due_date_reason_qs(cls):
         from olympia.reviewers.models import NeedsHumanReview
 
         requires_manual_listed_approval_and_is_listed = Q(
@@ -241,52 +236,39 @@ def annotate_due_date_reasons(self):
         is_needs_human_review = Q(
             ~Q(file__status=amo.STATUS_DISABLED) | Q(file__is_signed=True),
             needshumanreview__is_active=True,
-            id=OuterRef('pk'),
         )
         # Developer replies always trigger a due date even if the version has
         # been disabled and is not signed.
         has_developer_reply = Q(
             needshumanreview__is_active=True,
             needshumanreview__reason=NeedsHumanReview.REASONS.DEVELOPER_REPLY,
         )
-        return self.annotate(
-            needs_human_review_from_cinder=Exists(
-                self.filter(
-                    is_needs_human_review,
-                    needshumanreview__reason=NeedsHumanReview.REASONS.CINDER_ESCALATION,
-                )
-            ),
-            needs_human_review_from_abuse=Exists(
-                self.filter(
-                    is_needs_human_review,
-                    needshumanreview__reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION,
-                )
+        return {
+            'needs_human_review_from_cinder': Q(
+                is_needs_human_review,
+                needshumanreview__reason=NeedsHumanReview.REASONS.CINDER_ESCALATION,
             ),
-            needs_human_review_from_appeal=Exists(
-                self.filter(
-                    is_needs_human_review,
-                    needshumanreview__reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL,
-                )
+            'needs_human_review_from_abuse': Q(
+                is_needs_human_review,
+                needshumanreview__reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION,
             ),
-            needs_human_review_other=Exists(
-                self.filter(
-                    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,
-                        )
-                    ),
-                )
+            'needs_human_review_from_appeal': Q(
+                is_needs_human_review,
+                needshumanreview__reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL,
             ),
-            is_pre_review_version=Exists(
-                self.filter(is_pre_review_version, id=OuterRef('pk'))
-            ),
-            has_developer_reply=models.Exists(
-                self.filter(has_developer_reply, id=OuterRef('pk'))
+            '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,
+                    )
+                ),
             ),
-        )
+            'is_pre_review_version': is_pre_review_version,
+            'has_developer_reply': has_developer_reply,
+        }
 
 
 class UnfilteredVersionManagerForRelations(VersionManager):
@@ -1018,8 +1000,7 @@ def reset_due_date(self, due_date=None):
 
         If due_date is None then a new due date will only be set if the version doesn't
         already have one; otherwise the provided due_date will be be used to overwrite
-        any value.
-        """
+        any value."""
         if self.should_have_due_date:
             # if the version should have a due date and it doesn't, set one
             if not self.due_date or due_date:
@@ -1028,7 +1009,7 @@ def reset_due_date(self, due_date=None):
                 log.info('Version %r (%s) due_date set to %s', self, self.id, due_date)
                 self.update(due_date=due_date, _signal=False)
         elif self.due_date:
-            # otherwise it shouldn't have a due_date so clear it
+            # otherwise it shouldn't have a due_date so clear it.
             log.info(
                 'Version %r (%s) due_date of %s cleared', self, self.id, self.due_date
             )

src/olympia/versions/tests/test_models.py

@@ -462,77 +462,45 @@ def test_should_have_due_date(self):
             multiple,
         ]
 
-    def test_annotate_due_date_reasons(self):
+    def test_get_due_date_reason_qs(self):
         self.test_should_have_due_date()  # to set up the Versions
 
-        qs = Version.objects.annotate_due_date_reasons().order_by('id')
+        qs = Version.objects.all().order_by('id')
         # See test_should_have_due_date for order
         (
-            random_addon,
+            _,  # addon with nothing special set
             other_nhr,
             recommended,
-            strategic,
+            _,  # promoted but not prereview addon
             developer_reply,
             abuse_nhr,
             appeal_nhr,
             multiple,
         ) = list(qs)
 
-        assert random_addon.needs_human_review_from_cinder is False
-        assert random_addon.needs_human_review_from_abuse is False
-        assert random_addon.needs_human_review_from_appeal is False
-        assert random_addon.needs_human_review_other is False
-        assert random_addon.is_pre_review_version is False
-        assert random_addon.has_developer_reply is False
-
-        assert other_nhr.needs_human_review_from_cinder is False
-        assert other_nhr.needs_human_review_from_abuse is False
-        assert other_nhr.needs_human_review_from_appeal is False
-        assert other_nhr.needs_human_review_other is True
-        assert other_nhr.is_pre_review_version is False
-        assert other_nhr.has_developer_reply is False
-
-        assert recommended.needs_human_review_from_cinder is False
-        assert recommended.needs_human_review_from_abuse is False
-        assert recommended.needs_human_review_from_appeal is False
-        assert recommended.needs_human_review_other is False
-        assert recommended.is_pre_review_version is True
-        assert recommended.has_developer_reply is False
-
-        assert strategic.needs_human_review_from_cinder is False
-        assert strategic.needs_human_review_from_abuse is False
-        assert strategic.needs_human_review_from_appeal is False
-        assert strategic.needs_human_review_other is False
-        assert strategic.is_pre_review_version is False
-        assert strategic.has_developer_reply is False
-
-        assert developer_reply.needs_human_review_from_cinder is False
-        assert developer_reply.needs_human_review_from_abuse is False
-        assert developer_reply.needs_human_review_from_appeal is False
-        assert developer_reply.needs_human_review_other is False
-        assert developer_reply.is_pre_review_version is False
-        assert developer_reply.has_developer_reply is True
-
-        assert abuse_nhr.needs_human_review_from_cinder is False
-        assert abuse_nhr.needs_human_review_from_abuse is True
-        assert abuse_nhr.needs_human_review_from_appeal is False
-        assert abuse_nhr.needs_human_review_other is False
-        assert abuse_nhr.is_pre_review_version is False
-        assert abuse_nhr.has_developer_reply is False
-
-        assert appeal_nhr.needs_human_review_from_cinder is False
-        assert appeal_nhr.needs_human_review_from_abuse is False
-        assert appeal_nhr.needs_human_review_from_appeal is True
-        assert appeal_nhr.needs_human_review_other is False
-        assert appeal_nhr.is_pre_review_version is False
-        assert appeal_nhr.has_developer_reply is False
-
-        assert multiple.needs_human_review_from_cinder is True
-        assert multiple.needs_human_review_from_abuse is False
-        assert multiple.needs_human_review_from_appeal is False
-        assert multiple.needs_human_review_other is False
-        assert multiple.is_pre_review_version is True
-        assert multiple.has_developer_reply is True
+        q_objects = Version.objects.get_due_date_reason_qs()
+        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'])) == [
+            multiple,
+            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):