Only show unique checks

Commit e5c641f optimized fetching of checks when displaying a patch by
prefetching these checks ahead of time. Unfortunately we missed that
this should exclude older versions of checks for a given context. Make
the code used to generate the unique checks generic and allow us to use
that as a filter to the checks provided to the template, restoring the
correct behavior.

Signed-off-by: Stephen Finucane <[email protected]>
Closes: #398
Fixes: e5c641f ("Optimise fetching checks when displaying a patch")
(cherry picked from commit a43d6ac)

Author: stephenfin

patchwork/models.py

@@ -518,50 +518,13 @@ def is_editable(self, user):
             return True
         return False
 
-    @property
-    def combined_check_state(self):
-        """Return the combined state for all checks.
-
-        Generate the combined check's state for this patch. This check
-        is one of the following, based on the value of each unique
-        check:
-
-          * failure, if any context's latest check reports as failure
-          * warning, if any context's latest check reports as warning
-          * pending, if there are no checks, or a context's latest
-              Check reports as pending
-          * success, if latest checks for all contexts reports as
-              success
-        """
-        state_names = dict(Check.STATE_CHOICES)
-        states = [check.state for check in self.checks]
-
-        if not states:
-            return state_names[Check.STATE_PENDING]
-
-        for state in [Check.STATE_FAIL, Check.STATE_WARNING,
-                      Check.STATE_PENDING]:  # order sensitive
-            if state in states:
-                return state_names[state]
-
-        return state_names[Check.STATE_SUCCESS]
-
-    @property
-    def checks(self):
-        """Return the list of unique checks.
-
-        Generate a list of checks associated with this patch for each
-        type of Check. Only "unique" checks are considered,
-        identified by their 'context' field. This means, given n
-        checks with the same 'context', the newest check is the only
-        one counted regardless of its value. The end result will be a
-        association of types to number of unique checks for said
-        type.
-        """
+    @staticmethod
+    def filter_unique_checks(checks):
+        """Filter the provided checks to generate the unique list."""
         unique = {}
         duplicates = []
 
-        for check in self.check_set.all():
+        for check in checks:
             ctx = check.context
             user = check.user_id
 
@@ -588,7 +551,50 @@ def checks(self):
         # prefetch_related.) So, do it 'by hand' in Python. We can
         # also be confident that this won't be worse, seeing as we've
         # just iterated over self.check_set.all() *anyway*.
-        return [c for c in self.check_set.all() if c.id not in duplicates]
+        return [c for c in checks if c.id not in duplicates]
+
+    @property
+    def checks(self):
+        """Return the list of unique checks.
+
+        Generate a list of checks associated with this patch for each
+        type of Check. Only "unique" checks are considered,
+        identified by their 'context' field. This means, given n
+        checks with the same 'context', the newest check is the only
+        one counted regardless of its value. The end result will be a
+        association of types to number of unique checks for said
+        type.
+        """
+        return self.filter_unique_checks(self.check_set.all())
+
+    @property
+    def combined_check_state(self):
+        """Return the combined state for all checks.
+
+        Generate the combined check's state for this patch. This check
+        is one of the following, based on the value of each unique
+        check:
+
+        * failure, if any context's latest check reports as failure
+        * warning, if any context's latest check reports as warning
+        * pending, if there are no checks, or a context's latest check reports
+          as pending
+        * success, if latest checks for all contexts reports as success
+        """
+        state_names = dict(Check.STATE_CHOICES)
+        states = [check.state for check in self.checks]
+
+        if not states:
+            return state_names[Check.STATE_PENDING]
+
+        # order sensitive
+        for state in (
+            Check.STATE_FAIL, Check.STATE_WARNING, Check.STATE_PENDING,
+        ):
+            if state in states:
+                return state_names[state]
+
+        return state_names[Check.STATE_SUCCESS]
 
     @property
     def check_count(self):

patchwork/tests/test_detail.py

@@ -3,14 +3,20 @@
 #
 # SPDX-License-Identifier: GPL-2.0-or-later
 
+from datetime import datetime as dt
+from datetime import timedelta
+
 from django.test import TestCase
 from django.urls import reverse
 
+from patchwork.models import Check
+from patchwork.tests.utils import create_check
 from patchwork.tests.utils import create_cover
 from patchwork.tests.utils import create_cover_comment
 from patchwork.tests.utils import create_patch
 from patchwork.tests.utils import create_patch_comment
 from patchwork.tests.utils import create_project
+from patchwork.tests.utils import create_user
 
 
 class CoverViewTest(TestCase):
@@ -157,6 +163,38 @@ def test_invalid_patch_id(self):
         response = self.client.get(requested_url)
         self.assertEqual(response.status_code, 404)
 
+    def test_patch_with_checks(self):
+        user = create_user()
+        patch = create_patch()
+        check_a = create_check(
+            patch=patch, user=user, context='foo', state=Check.STATE_FAIL,
+            date=(dt.utcnow() - timedelta(days=1)))
+        create_check(
+            patch=patch, user=user, context='foo', state=Check.STATE_SUCCESS)
+        check_b = create_check(
+            patch=patch, user=user, context='bar', state=Check.STATE_PENDING)
+        requested_url = reverse(
+            'patch-detail',
+            kwargs={
+                'project_id': patch.project.linkname,
+                'msgid': patch.url_msgid,
+            },
+        )
+        response = self.client.get(requested_url)
+
+        # the response should contain checks
+        self.assertContains(response, '<h2>Checks</h2>')
+
+        # and it should only show the unique checks
+        self.assertEqual(
+            1, response.content.decode().count(
+                f'<td>{check_a.user}/{check_a.context}</td>'
+            ))
+        self.assertEqual(
+            1, response.content.decode().count(
+                f'<td>{check_b.user}/{check_b.context}</td>'
+            ))
+
 
 class CommentRedirectTest(TestCase):
 

patchwork/views/patch.py

@@ -124,7 +124,9 @@ def patch_detail(request, project_id, msgid):
         related_different_project = []
 
     context['comments'] = comments
-    context['checks'] = patch.check_set.all().select_related('user')
+    context['checks'] = Patch.filter_unique_checks(
+        patch.check_set.all().select_related('user'),
+    )
     context['submission'] = patch
     context['patchform'] = form
     context['createbundleform'] = createbundleform