Make reviewer theme queue orderable (w/ default order by due date) (#…

…22583)

* Make reviewer theme queue orderable (w/ default order by due date)

src/olympia/amo/tests/__init__.py

@@ -147,7 +147,7 @@ def initial(form):
     return data
 
 
-def check_links(expected, elements, selected=None, verify=True):
+def check_links(expected, elements, selected=None):
     """Useful for comparing an `expected` list of links against PyQuery
     `elements`. Expected format of links is a list of tuples, like so:
 
@@ -159,9 +159,6 @@ def check_links(expected, elements, selected=None, verify=True):
 
     If you'd like to check if a particular item in the list is selected,
     pass as `selected` the title of the link.
-
-    Links are verified by default.
-
     """
     for idx, item in enumerate(expected):
         # List item could be `(text, link)`.
@@ -171,19 +168,19 @@ def check_links(expected, elements, selected=None, verify=True):
         elif isinstance(item, str):
             text, link = None, item
 
-        e = elements.eq(idx)
+        elm = elements.eq(idx)
         if text is not None:
-            assert e.text() == text, f'At index {idx}, expected {text}, got {e.text()}'
+            assert (
+                elm.text() == text
+            ), f'At index {idx}, expected {text}, got {elm.text()}'
         if link is not None:
             # If we passed an <li>, try to find an <a>.
-            if not e.filter('a'):
-                e = e.find('a')
-            assert_url_equal(e.attr('href'), link)
-            if verify and link != '#':
-                assert Client().head(link, follow=True).status_code == 200
+            if not elm.filter('a'):
+                elm = elm.find('a')
+            assert_url_equal(elm.attr('href'), link)
         if text is not None and selected is not None:
-            e = e.filter('.selected, .sel') or e.parents('.selected, .sel')
-            assert bool(e.length) == (text == selected)
+            elm = elm.filter('.selected, .sel') or elm.parents('.selected, .sel')
+            assert bool(elm.length) == (text == selected)
 
 
 def assert_url_equal(url, expected, compare_host=False):

src/olympia/amo/tests/test_views.py

@@ -177,7 +177,7 @@ def test_tools_regular_user(self):
             ('Developer Hub', reverse('devhub.index')),
             ('Manage API Keys', reverse('devhub.api_key')),
         ]
-        check_links(expected, pq(response.content)('#aux-nav .tools a'), verify=False)
+        check_links(expected, pq(response.content)('#aux-nav .tools a'))
 
     def test_tools_developer(self):
         # Make them a developer.
@@ -199,7 +199,7 @@ def test_tools_developer(self):
             ('Developer Hub', reverse('devhub.index')),
             ('Manage API Keys', reverse('devhub.api_key')),
         ]
-        check_links(expected, pq(response.content)('#aux-nav .tools a'), verify=False)
+        check_links(expected, pq(response.content)('#aux-nav .tools a'))
 
     def test_tools_reviewer(self):
         user = UserProfile.objects.get(email='[email protected]')
@@ -217,7 +217,7 @@ def test_tools_reviewer(self):
             ('Manage API Keys', reverse('devhub.api_key')),
             ('Reviewer Tools', reverse('reviewers.dashboard')),
         ]
-        check_links(expected, pq(response.content)('#aux-nav .tools a'), verify=False)
+        check_links(expected, pq(response.content)('#aux-nav .tools a'))
 
     def test_tools_developer_and_reviewer(self):
         # Make them a developer.
@@ -239,7 +239,7 @@ def test_tools_developer_and_reviewer(self):
             ('Manage API Keys', reverse('devhub.api_key')),
             ('Reviewer Tools', reverse('reviewers.dashboard')),
         ]
-        check_links(expected, pq(response.content)('#aux-nav .tools a'), verify=False)
+        check_links(expected, pq(response.content)('#aux-nav .tools a'))
 
     def test_tools_admin(self):
         user = UserProfile.objects.get(email='[email protected]')
@@ -261,7 +261,7 @@ def test_tools_admin(self):
             ('Reviewer Tools', reverse('reviewers.dashboard')),
             ('Admin Tools', reverse('admin:index')),
         ]
-        check_links(expected, pq(response.content)('#aux-nav .tools a'), verify=False)
+        check_links(expected, pq(response.content)('#aux-nav .tools a'))
 
     def test_tools_developer_and_admin(self):
         # Make them a developer.
@@ -287,7 +287,7 @@ def test_tools_developer_and_admin(self):
             ('Reviewer Tools', reverse('reviewers.dashboard')),
             ('Admin Tools', reverse('admin:index')),
         ]
-        check_links(expected, pq(response.content)('#aux-nav .tools a'), verify=False)
+        check_links(expected, pq(response.content)('#aux-nav .tools a'))
 
 
 class TestOtherStuff(TestCase):

src/olympia/reviewers/tests/test_views.py

@@ -1176,7 +1176,7 @@ def _test_results(self, dont_expect_version_number=False):
         assert len(rows) == len(self.expected_addons)
         links = doc('#addon-queue tr.addon-row td a:not(.app-icon)')
         assert len(links) == len(self.expected_addons)
-        check_links(expected, links, verify=False)
+        check_links(expected, links)
         return doc
 
 
@@ -1467,9 +1467,7 @@ def test_results_two_versions(self):
             ),
         ]
         doc = pq(response.content)
-        check_links(
-            expected, doc('#addon-queue tr.addon-row td a:not(.app-icon)'), verify=False
-        )
+        check_links(expected, doc('#addon-queue tr.addon-row td a:not(.app-icon)'))
 
     def test_queue_layout(self):
         self.expected_addons = self.get_expected_addons_by_names(
@@ -1747,6 +1745,34 @@ def test_results(self):
             # - 1 for my add-ons in user menu
             self._test_results()
 
+    def test_queue_ordering_by_due_date(self):
+        # Bump pending one to the top by making the due date of its version
+        # very old, like they have been waiting for a while.
+        version = self.addons['Pending One'].versions.all()[0]
+        version.update(due_date=self.days_ago(365))
+        response = self.client.get(self.url)
+        assert response.status_code == 200
+        expected = [
+            (
+                'Pending One 0.1',
+                reverse('reviewers.review', args=[self.addons['Pending One'].pk]),
+            ),
+            (
+                'Nominated One 0.1',
+                reverse('reviewers.review', args=[self.addons['Nominated One'].pk]),
+            ),
+            (
+                'Nominated Two 0.1',
+                reverse('reviewers.review', args=[self.addons['Nominated Two'].pk]),
+            ),
+            (
+                'Pending Two 0.1',
+                reverse('reviewers.review', args=[self.addons['Pending Two'].pk]),
+            ),
+        ]
+        doc = pq(response.content)
+        check_links(expected, doc('#addon-queue tr.addon-row td a:not(.app-icon)'))
+
     def test_results_two_versions(self):
         version1 = self.addons['Nominated One'].versions.all()[0]
         version2 = self.addons['Nominated Two'].versions.all()[0]
@@ -1784,9 +1810,7 @@ def test_results_two_versions(self):
             ),
         ]
         doc = pq(response.content)
-        check_links(
-            expected, doc('#addon-queue tr.addon-row td a:not(.app-icon)'), verify=False
-        )
+        check_links(expected, doc('#addon-queue tr.addon-row td a:not(.app-icon)'))
 
     def test_queue_layout(self):
         self._test_queue_layout(
@@ -2641,7 +2665,7 @@ def test_files_shown(self):
             ('Open in VSC', None),
             ('Browse contents', None),
         ]
-        check_links(expected, items.find('a'), verify=False)
+        check_links(expected, items.find('a'))
 
     def test_item_history(self, channel=amo.CHANNEL_LISTED):
         self.addons['something'] = addon_factory(
@@ -3080,7 +3104,7 @@ def test_action_links(self):
         expected = [
             ('View Product Page', self.addon.get_url_path()),
         ]
-        check_links(expected, doc('#actions-addon a'), verify=False)
+        check_links(expected, doc('#actions-addon a'))
 
     def test_action_links_as_admin(self):
         self.login_as_admin()
@@ -3093,7 +3117,7 @@ def test_action_links_as_admin(self):
             ('Admin Page', reverse('admin:addons_addon_change', args=[self.addon.id])),
             ('Statistics', reverse('stats.overview', args=[self.addon.id])),
         ]
-        check_links(expected, doc('#actions-addon a'), verify=False)
+        check_links(expected, doc('#actions-addon a'))
 
     def test_unlisted_addon_action_links_as_admin(self):
         """No "View Product Page" link for unlisted addons, "edit"/"manage" links
@@ -3112,7 +3136,7 @@ def test_unlisted_addon_action_links_as_admin(self):
             ('Admin Page', reverse('admin:addons_addon_change', args=[self.addon.id])),
             ('Statistics', reverse('stats.overview', args=[self.addon.id])),
         ]
-        check_links(expected, doc('#actions-addon a'), verify=False)
+        check_links(expected, doc('#actions-addon a'))
 
     def test_mixed_channels_action_links_as_admin(self):
         self.make_addon_unlisted(self.addon)
@@ -3136,7 +3160,7 @@ def test_mixed_channels_action_links_as_admin(self):
             ('Admin Page', reverse('admin:addons_addon_change', args=[self.addon.id])),
             ('Statistics', reverse('stats.overview', args=[self.addon.id])),
         ]
-        check_links(expected, doc('#actions-addon a'), verify=False)
+        check_links(expected, doc('#actions-addon a'))
 
     def test_mixed_channels_action_links_as_admin_on_unlisted_review(self):
         self.make_addon_unlisted(self.addon)
@@ -3158,7 +3182,7 @@ def test_mixed_channels_action_links_as_admin_on_unlisted_review(self):
             ('Admin Page', reverse('admin:addons_addon_change', args=[self.addon.id])),
             ('Statistics', reverse('stats.overview', args=[self.addon.id])),
         ]
-        check_links(expected, doc('#actions-addon a'), verify=False)
+        check_links(expected, doc('#actions-addon a'))
 
     def test_mixed_channels_action_links_as_admin_deleted_addon(self):
         self.make_addon_unlisted(self.addon)
@@ -3182,7 +3206,7 @@ def test_mixed_channels_action_links_as_admin_deleted_addon(self):
             ('Admin Page', reverse('admin:addons_addon_change', args=[self.addon.id])),
             ('Statistics', reverse('stats.overview', args=[self.addon.id])),
         ]
-        check_links(expected, doc('#actions-addon a'), verify=False)
+        check_links(expected, doc('#actions-addon a'))
 
     def test_mixed_channels_action_links_as_admin_unlisted_deleted_addon(self):
         self.make_addon_unlisted(self.addon)
@@ -3203,7 +3227,7 @@ def test_mixed_channels_action_links_as_admin_unlisted_deleted_addon(self):
             ('Admin Page', reverse('admin:addons_addon_change', args=[self.addon.id])),
             ('Statistics', reverse('stats.overview', args=[self.addon.id])),
         ]
-        check_links(expected, doc('#actions-addon a'), verify=False)
+        check_links(expected, doc('#actions-addon a'))
 
     def test_mixed_channels_action_links_as_regular_reviewer(self):
         self.make_addon_unlisted(self.addon)
@@ -3220,7 +3244,7 @@ def test_mixed_channels_action_links_as_regular_reviewer(self):
         expected = [
             ('View Product Page', self.addon.get_url_path()),
         ]
-        check_links(expected, doc('#actions-addon a'), verify=False)
+        check_links(expected, doc('#actions-addon a'))
 
     def test_admin_links_as_non_admin(self):
         self.login_as_reviewer()
@@ -4022,7 +4046,7 @@ def test_compare_link(self):
             ),
         ]
 
-        check_links(expected, links, verify=False)
+        check_links(expected, links)
 
     def test_compare_link_auto_approved_ignored(self):
         first_file = self.addon.current_version.file
@@ -4057,7 +4081,7 @@ def test_compare_link_auto_approved_ignored(self):
                 version_id=new_version.pk,
             ),
         ]
-        check_links(expected, links, verify=False)
+        check_links(expected, links)
 
     def test_compare_link_auto_approved_but_confirmed_not_ignored(self):
         first_file = self.addon.current_version.file
@@ -4098,7 +4122,7 @@ def test_compare_link_auto_approved_but_confirmed_not_ignored(self):
                 version_id=new_version.pk,
             ),
         ]
-        check_links(expected, links, verify=False)
+        check_links(expected, links)
 
     def test_compare_link_not_auto_approved_but_confirmed(self):
         first_file = self.addon.current_version.file
@@ -4131,7 +4155,7 @@ def test_compare_link_not_auto_approved_but_confirmed(self):
                 version_id=new_version.pk,
             ),
         ]
-        check_links(expected, links, verify=False)
+        check_links(expected, links)
 
     def test_download_sources_link(self):
         version = self.addon.current_version
@@ -5375,7 +5399,7 @@ def test_addons_sharing_guid_shown(self):
             (f'{old_two}', reverse('reviewers.review', args=[old_two.id])),
         ]
         doc = pq(response.content)
-        check_links(expected, doc('.addon-addons-sharing-guid a'), verify=False)
+        check_links(expected, doc('.addon-addons-sharing-guid a'))
 
         assert b'Original Add-on ID' in response.content
         assert doc('.addon-guid td').text() == self.addon.guid
@@ -5399,7 +5423,7 @@ def test_addons_sharing_guid_shown(self):
             ),
         ]
         doc = pq(response.content)
-        check_links(expected, doc('.addon-addons-sharing-guid a'), verify=False)
+        check_links(expected, doc('.addon-addons-sharing-guid a'))
 
         # It shouldn't happen nowadays, but make sure an empty guid isn't
         # considered.
@@ -8636,7 +8660,7 @@ def test_results(self):
         doc = pq(response.content)
         links = doc('#addon-queue tr.addon-row td a:not(.app-icon)')
         assert len(links) == len(expected)
-        check_links(expected, links, verify=False)
+        check_links(expected, links)
 
     def test_only_viewable_with_specific_permission(self):
         # Content reviewer does not have access.

src/olympia/reviewers/utils.py

@@ -146,14 +146,15 @@ class PendingManualApprovalQueueTable(AddonQueueTable):
     url = r'^extension$'
     permission = amo.permissions.ADDONS_REVIEW
 
-    class Meta:
+    class Meta(AddonQueueTable.Meta):
         fields = ('addon_name', 'addon_type', 'due_date', 'flags', 'score')
         exclude = (
             'last_human_review',
             'code_weight',
             'metadata_weight',
             'weight',
         )
+        orderable = True
 
     @classmethod
     def get_queryset(self, request, *, upcoming_due_date_focus=False, **kw):
@@ -200,7 +201,7 @@ class ThemesQueueTable(PendingManualApprovalQueueTable):
         verbose_name='Target Date', accessor='first_version_due_date'
     )
 
-    class Meta(AddonQueueTable.Meta):
+    class Meta(PendingManualApprovalQueueTable.Meta):
         exclude = (
             'score',
             'addon_type',