From: Stephen Finucane Date: Sat, 6 Mar 2021 14:54:59 +0000 (+0000) Subject: Only show unique checks X-Git-Tag: v3.1.0~94 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=a43d6acda0939c14e8135549968815876680d497;p=thirdparty%2Fpatchwork.git Only show unique checks Commit e5c641fc4 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 Closes: #398 Fixes: e5c641fc4 ("Optimise fetching checks when displaying a patch") --- diff --git a/patchwork/models.py b/patchwork/models.py index e650e8b4..00273da9 100644 --- a/patchwork/models.py +++ b/patchwork/models.py @@ -518,50 +518,13 @@ class Patch(SubmissionMixin): 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 @@ class Patch(SubmissionMixin): # 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): diff --git a/patchwork/tests/test_detail.py b/patchwork/tests/test_detail.py index 393ebc76..5add40f9 100644 --- a/patchwork/tests/test_detail.py +++ b/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 @@ class PatchViewTest(TestCase): 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, '

Checks

') + + # and it should only show the unique checks + self.assertEqual( + 1, response.content.decode().count( + f'{check_a.user}/{check_a.context}' + )) + self.assertEqual( + 1, response.content.decode().count( + f'{check_b.user}/{check_b.context}' + )) + class CommentRedirectTest(TestCase): diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py index 74495714..9f0b855b 100644 --- a/patchwork/views/patch.py +++ b/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