Fix broken reviewer queues, refactor manager methods to streamline them (#20286)

* Fix queues broken because of pure unlisted add-ons

src/olympia/addons/models.py

@@ -174,6 +174,20 @@ def clean_slug(instance, slug_field='slug'):
     return instance
 
 
+def first_pending_version_transformer(addons):
+    """Transformer used to attach the special first_pending_version field on
+    addons, used in reviewer queues."""
+    version_ids = {addon.first_version_id for addon in addons}
+    versions = {
+        version.id: version
+        for version in Version.unfiltered.filter(id__in=version_ids)
+        .no_transforms()
+        .select_related('autoapprovalsummary')
+    }
+    for addon in addons:
+        addon.first_pending_version = versions.get(addon.first_version_id)
+
+
 class AddonQuerySet(BaseQuerySet):
     def id_or_slug(self, val):
         """Get add-ons by id or slug."""
@@ -258,6 +272,7 @@ class AddonManager(ManagerBase):
         select_related_fields = [
             'reviewerflags',
             'addonapprovalscounter',
+            'promotedaddon',
         ]
         if select_related_fields_for_listed:
             # Most listed queues need these to avoid extra queries because
@@ -269,7 +284,6 @@ class AddonManager(ManagerBase):
                     '_current_version__autoapprovalsummary',
                     '_current_version__file',
                     '_current_version__reviewerflags',
-                    'promotedaddon',
                 )
             )
         qs = qs.select_related(*select_related_fields)
@@ -284,30 +298,26 @@ class AddonManager(ManagerBase):
         return qs
 
     def get_queryset_for_pending_queues(
-        self, *, admin_reviewer=False, theme_review=False, filters=None, excludes=None
+        self, *, admin_reviewer=False, theme_review=False
     ):
-        def pending_version_transformer(addons):
-            version_ids = {addon.first_version_id for addon in addons}
-            versions = {
-                version.id: version
-                for version in Version.objects.filter(id__in=version_ids)
-                .no_transforms()
-                .select_related('autoapprovalsummary')
+        if theme_review:
+            filters = {
+                'type__in': amo.GROUP_TYPE_THEME,
             }
-            for addon in addons:
-                addon.first_pending_version = versions.get(addon.first_version_id)
-
-        if excludes is None:
-            excludes = {}
-        if filters is None:
-            filters = {}
+        else:
+            filters = {
+                'type__in': amo.GROUP_TYPE_ADDON,
+            }
+        excludes = {
+            'status': amo.STATUS_DISABLED,
+        }
+        filters['versions__due_date__isnull'] = False
         qs = self.get_base_queryset_for_queue(
             admin_reviewer=admin_reviewer,
             theme_review=theme_review,
-            # Extension queue merges listed & unlisted together, so the select
-            # related for listed don't make sense there, but the theme queues
-            # don't do that, so they need them.
-            select_related_fields_for_listed=theme_review,
+            # These queues merge unlisted and listed together, so the
+            # select_related() for listed fields don't make sense.
+            select_related_fields_for_listed=False,
         )
         return (
             qs.filter(**filters)
@@ -324,52 +334,7 @@ class AddonManager(ManagerBase):
                 # related versions.
                 first_version_id=Func(F('versions__id'), function='ANY_VALUE'),
             )
-            .transform(pending_version_transformer)
-        )
-
-    def get_pending_review_queue(self, admin_reviewer=False):
-        filters = {
-            'type__in': amo.GROUP_TYPE_ADDON,
-            'versions__due_date__isnull': False,
-        }
-        excludes = {
-            'status': amo.STATUS_DISABLED,
-        }
-        qs = self.get_queryset_for_pending_queues(
-            admin_reviewer=admin_reviewer, filters=filters, excludes=excludes
-        )
-        return qs.select_related('promotedaddon')
-
-    def get_themes_pending_manual_approval_queue(
-        self,
-        admin_reviewer=False,
-        statuses=amo.VALID_ADDON_STATUSES,
-    ):
-        filters = {
-            'type__in': amo.GROUP_TYPE_THEME,
-            'versions__due_date__isnull': False,
-            'status__in': statuses,
-        }
-        excludes = {
-            'status': amo.STATUS_DISABLED,
-        }
-        return self.get_queryset_for_pending_queues(
-            admin_reviewer=admin_reviewer,
-            theme_review=True,
-            filters=filters,
-            excludes=excludes,
-        )
-
-    def get_addons_with_unlisted_versions_queue(self, admin_reviewer=False):
-        qs = self.get_base_queryset_for_queue(
-            select_related_fields_for_listed=False,
-            admin_reviewer=admin_reviewer,
-        )
-        # Reset *all* select_related() made by get_base_queryset_for_queue(),
-        # we don't want them for the unlisted queue.
-        qs = qs.select_related(None)
-        return qs.filter(versions__channel=amo.CHANNEL_UNLISTED).exclude(
-            status=amo.STATUS_DISABLED
+            .transform(first_pending_version_transformer)
         )
 
     def get_content_review_queue(self, admin_reviewer=False):
@@ -457,12 +422,26 @@ class AddonManager(ManagerBase):
         )
 
     def get_pending_rejection_queue(self, admin_reviewer=False):
-        filter_kwargs = {'versions__reviewerflags__pending_rejection__isnull': False}
         return (
-            self.get_base_queryset_for_queue(admin_reviewer=admin_reviewer)
-            .filter(**filter_kwargs)
-            .order_by('_current_version__reviewerflags__pending_rejection')
-            .distinct()
+            self.get_base_queryset_for_queue(
+                select_related_fields_for_listed=False, admin_reviewer=admin_reviewer
+            )
+            .filter(versions__reviewerflags__pending_rejection__isnull=False)
+            .annotate(
+                first_version_pending_rejection_date=Min(
+                    'versions__reviewerflags__pending_rejection'
+                ),
+                # Because of the Min(), a GROUP BY addon.id is created, and the
+                # versions join will pick the first by due date. Unfortunately
+                # if we were to annotate with just F('versions__<something>')
+                # Django would add version.<something> to the GROUP BY, ruining
+                # it. To prevent that, we wrap it into a harmless Func() - we
+                # need a no-op function to do that, hence the ANY_VALUE().
+                # We'll then grab the id in our transformer to fetch all
+                # related versions.
+                first_version_id=Func(F('versions__id'), function='ANY_VALUE'),
+            )
+            .transform(first_pending_version_transformer)
         )
 
 

src/olympia/addons/tests/test_models.py

@@ -3234,7 +3234,7 @@ class TestExtensionsQueues(TestCase):
                 'needs_admin_code_review': True,
             },
         )
-        addons = Addon.objects.get_pending_review_queue()
+        addons = Addon.objects.get_queryset_for_pending_queues()
         assert list(addons.order_by('pk')) == expected_addons
 
         # Test that we picked the version with the oldest due date and that we
@@ -3248,7 +3248,7 @@ class TestExtensionsQueues(TestCase):
 
         # Admins should be able to see addons needing admin code review.
         expected_addons.append(addon_needing_admin_code_review)
-        addons = Addon.objects.get_pending_review_queue(admin_reviewer=True)
+        addons = Addon.objects.get_queryset_for_pending_queues(admin_reviewer=True)
         assert set(addons) == set(expected_addons)
 
 
@@ -3265,7 +3265,9 @@ class TestThemesPendingManualApprovalQueue(TestCase):
             status=amo.STATUS_NOMINATED,
             file_kw={'status': amo.STATUS_AWAITING_REVIEW},
         )
-        qs = Addon.objects.get_themes_pending_manual_approval_queue().order_by('pk')
+        qs = Addon.objects.get_queryset_for_pending_queues(theme_review=True).order_by(
+            'pk'
+        )
         assert list(qs) == expected
 
     def test_approved_theme_pending_version(self):
@@ -3285,9 +3287,11 @@ class TestThemesPendingManualApprovalQueue(TestCase):
             status=amo.STATUS_NOMINATED,
             file_kw={'status': amo.STATUS_AWAITING_REVIEW},
         )
-        qs = Addon.objects.get_themes_pending_manual_approval_queue(
-            statuses=(amo.STATUS_APPROVED,)
-        ).order_by('pk')
+        qs = (
+            Addon.objects.get_queryset_for_pending_queues(theme_review=True)
+            .filter(status__in=(amo.STATUS_APPROVED,))
+            .order_by('pk')
+        )
         assert list(qs) == expected
 
 

src/olympia/reviewers/tests/test_views.py

@@ -1184,15 +1184,20 @@ class QueueTest(ReviewerTest):
         expected = []
         if not len(self.expected_addons):
             raise AssertionError('self.expected_addons was an empty list')
-        # We typically don't include the channel name if it's the
-        # default one, 'listed'.
-        channel = [] if self.channel_name == 'listed' else [self.channel_name]
         for idx, addon in enumerate(self.expected_addons):
             if self.channel_name == 'unlisted' or dont_expect_version_number:
                 # In unlisted queue we don't display latest version number.
                 name = str(addon.name)
             else:
                 expected_version = self.expected_versions[addon]
+                if self.channel_name == 'content':
+                    channel = [self.channel_name]
+                elif expected_version.channel == amo.CHANNEL_LISTED:
+                    # We typically don't include the channel name if it's the
+                    # default one, 'listed'.
+                    channel = []
+                else:
+                    channel = ['unlisted']
                 name = f'{str(addon.name)} {expected_version.version}'
             url = reverse('reviewers.review', args=channel + [addon.pk])
             expected.append((name, url))
@@ -1408,9 +1413,9 @@ class TestThemePendingQueue(QueueTest):
             # - 2 for savepoints because we're in tests
             # - 2 for user/groups
             # - 1 for the current queue count for pagination purposes
-            # - 3 for the addons in the queues, their versions/files and
-            #     translations and their files (regardless of how many are in
-            #     the queue - that's the important bit)
+            # - 3 for the addons in the queue, their translations and the
+            #     versions (regardless of how many are in the queue - that's
+            #     the important bit)
             # - 2 for config items (motd / site notice)
             # - 1 for my add-ons in user menu
             self._test_results()
@@ -1454,9 +1459,9 @@ class TestExtensionQueue(QueueTest):
             # - 2 for savepoints because we're in tests
             # - 2 for user/groups
             # - 1 for the current queue count for pagination purposes
-            # - 3 for the addons in the queues, their versions/files and
-            #     translations and their files (regardless of how many are in
-            #     the queue - that's the important bit)
+            # - 3 for the addons in the queue, their translations and the
+            #     versions (regardless of how many are in the queue - that's
+            #     the important bit)
             # - 2 for config items (motd / site notice)
             # - 1 for my add-ons in user menu
             self._test_results()
@@ -1667,9 +1672,9 @@ class TestThemeNominatedQueue(QueueTest):
             # - 2 for savepoints because we're in tests
             # - 2 for user/groups
             # - 1 for the current queue count for pagination purposes
-            # - 3 for the addons in the queues, their versions/files and
-            #     translations and their files (regardless of how many are in
-            #     the queue - that's the important bit)
+            # - 3 for the addons in the queue, their translations and the
+            #     versions (regardless of how many are in the queue - that's
+            #     the important bit)
             # - 2 for config items (motd / site notice)
             # - 1 for my add-ons in user menu
             self._test_results()
@@ -2073,8 +2078,9 @@ class TestContentReviewQueue(QueueTest):
             # - 2 for savepoints because we're in tests
             # - 2 for user/groups
             # - 1 for the current queue count for pagination purposes
-            # - 2 for the addons in the queues and their files (regardless of
-            #     how many are in the queue - that's the important bit)
+            # - 2 for the addons in the queue, their translations and the
+            #     versions (regardless of how many are in the queue - that's
+            #     the important bit)
             # - 2 for config items (motd / site notice)
             # - 1 for my add-ons in user menu
             self._test_results()
@@ -2181,8 +2187,9 @@ class TestScannersReviewQueue(QueueTest):
             # - 2 for savepoints because we're in tests
             # - 2 for user/groups
             # - 1 for the current queue count for pagination purposes
-            # - 2 for the addons in the queues and their files (regardless of
-            #     how many are in the queue - that's the important bit)
+            # - 2 for the addons in the queue, their translations and the
+            #     versions (regardless of how many are in the queue - that's
+            #     the important bit)
             # - 2 for config items (motd / site notice)
             # - 1 for my add-ons in user menu
             response = self.client.get(self.url)
@@ -2303,14 +2310,15 @@ class TestPendingRejectionReviewQueue(QueueTest):
         version_review_flags_factory(
             version=pending_version1, pending_rejection=datetime.now()
         )
-        pending_version2 = version_factory(
+        pending_version_unlisted_addon = version_factory(
             addon=unlisted_addon,
             created=self.days_ago(1),
             version='0.2',
             channel=amo.CHANNEL_UNLISTED,
         )
         version_review_flags_factory(
-            version=pending_version2, pending_rejection=datetime.now()
+            version=pending_version_unlisted_addon,
+            pending_rejection=datetime.now() - timedelta(hours=1),
         )
         version_factory(
             addon=unlisted_addon, version='0.3', channel=amo.CHANNEL_UNLISTED
@@ -2322,20 +2330,25 @@ class TestPendingRejectionReviewQueue(QueueTest):
         # Addon 2 has an older creation date, but what matters for the ordering
         # is the pending rejection deadline.
         self.expected_addons = [unlisted_addon, addon1, addon2]
-        self.expected_versions = self.get_expected_versions(self.expected_addons)
+        self.expected_versions = {
+            unlisted_addon: pending_version_unlisted_addon,
+            addon1: addon1.current_version,
+            addon2: addon2.current_version,
+        }
 
     def test_results(self):
         self.login_as_admin()
         self.generate_files()
-        with self.assertNumQueries(10):
+        with self.assertNumQueries(11):
             # - 2 for savepoints because we're in tests
             # - 2 for user/groups
             # - 1 for the current queue count for pagination purposes
-            # - 2 for the addons in the queues and their files (regardless of
-            #     how many are in the queue - that's the important bit)
+            # - 3 for the addons in the queue, their translations and the
+            #     versions (regardless of how many are in the queue - that's
+            #     the important bit)
             # - 2 for config items (motd / site notice)
             # - 1 for my add-ons in user menu
-            self._test_results(dont_expect_version_number=True)
+            self._test_results()
 
 
 class ReviewBase(QueueTest):

src/olympia/reviewers/utils.py

@@ -158,11 +158,13 @@ class PendingManualApprovalQueueTable(AddonQueueTable):
 
     @classmethod
     def get_queryset(cls, admin_reviewer=False):
-        return Addon.objects.get_pending_review_queue(admin_reviewer=admin_reviewer)
+        return Addon.objects.get_queryset_for_pending_queues(
+            admin_reviewer=admin_reviewer
+        )
 
     def get_version(self, record):
-        # Use the property set by get_pending_review_queue() to display the
-        # right version.
+        # Use the property set by get_queryset_for_pending_queues() to display
+        # the right version.
         return record.first_pending_version
 
     def render_addon_type(self, record):
@@ -183,27 +185,51 @@ class PendingManualApprovalQueueTable(AddonQueueTable):
 
 
 class NewThemesQueueTable(PendingManualApprovalQueueTable):
+    class Meta(AddonQueueTable.Meta):
+        exclude = (
+            'score',
+            'addon_type',
+            'last_human_review',
+            'code_weight',
+            'metadata_weight',
+            'weight',
+        )
+
     @classmethod
     def get_queryset(cls, admin_reviewer=False):
-        return Addon.objects.get_themes_pending_manual_approval_queue(
-            admin_reviewer=admin_reviewer,
-            statuses=(amo.STATUS_NOMINATED,),
-        )
+        return Addon.objects.get_queryset_for_pending_queues(
+            admin_reviewer=admin_reviewer, theme_review=True
+        ).filter(status__in=(amo.STATUS_NOMINATED,))
 
 
 class UpdatedThemesQueueTable(NewThemesQueueTable):
     @classmethod
     def get_queryset(cls, admin_reviewer=False):
-        return Addon.objects.get_themes_pending_manual_approval_queue(
-            admin_reviewer=admin_reviewer,
-            statuses=(amo.STATUS_APPROVED,),
-        )
+        return Addon.objects.get_queryset_for_pending_queues(
+            admin_reviewer=admin_reviewer, theme_review=True
+        ).filter(status__in=(amo.STATUS_APPROVED,))
 
 
 class PendingRejectionTable(AddonQueueTable):
     deadline = tables.Column(
         verbose_name='Pending Rejection Deadline',
-        accessor='_current_version__reviewerflags__pending_rejection',
+        accessor='first_version_pending_rejection_date',
+    )
+    code_weight = tables.Column(
+        verbose_name='Code Weight',
+        accessor='first_pending_version__autoapprovalsummary__code_weight',
+    )
+    metadata_weight = tables.Column(
+        verbose_name='Metadata Weight',
+        accessor='first_pending_version__autoapprovalsummary__metadata_weight',
+    )
+    weight = tables.Column(
+        verbose_name='Total Weight',
+        accessor='first_pending_version__autoapprovalsummary__weight',
+    )
+    score = tables.Column(
+        verbose_name='Maliciousness Score',
+        accessor='first_pending_version__autoapprovalsummary__score',
     )
 
     class Meta(PendingManualApprovalQueueTable.Meta):
@@ -223,20 +249,14 @@ class PendingRejectionTable(AddonQueueTable):
     def get_queryset(cls, admin_reviewer=False):
         return Addon.objects.get_pending_rejection_queue(admin_reviewer=admin_reviewer)
 
+    def get_version(self, record):
+        # Use the property set by get_pending_rejection_queue() to display
+        # the right version.
+        return record.first_pending_version
+
     def render_deadline(self, value):
         return naturaltime(value) if value else ''
 
-    def render_addon_name(self, record):
-        url = self._get_addon_name_url(record)
-        self.increment_item()
-        return markupsafe.Markup(
-            '<a href="%s">%s'
-            % (
-                url,
-                markupsafe.escape(record.name),
-            )
-        )
-
 
 class ContentReviewTable(AddonQueueTable):
     last_updated = tables.DateTimeColumn(verbose_name='Last Updated')