From 49d0acc1e228d88cca68c78f7dce25c1d247732c Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Sat, 6 Mar 2021 14:54:59 +0000 Subject: [PATCH] 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. Conflicts: patchwork/tests/test_detail.py NOTE(stephenfin): Conflicts are due to some differences in imports, however, we also need to drop some usages of f-strings since Patchwork 2.x supported Python 2.x also. We also need to specify an explicit decode value for some strings since Python 2.x defaults to 'ascii', not 'utf-8'. Signed-off-by: Stephen Finucane Closes: #398 Fixes: e5c641fc4 ("Optimise fetching checks when displaying a patch") (cherry picked from commit a43d6acda0939c14e8135549968815876680d497) (cherry picked from commit f4ff605eb449ea56b265ee7a5a516b48d83a5eb0) --- patchwork/models.py | 90 ++++++++++++++++++---------------- patchwork/tests/test_detail.py | 38 ++++++++++++++ patchwork/views/patch.py | 4 +- 3 files changed, 89 insertions(+), 43 deletions(-) diff --git a/patchwork/models.py b/patchwork/models.py index a3c34da7..028beb3c 100644 --- a/patchwork/models.py +++ b/patchwork/models.py @@ -516,50 +516,13 @@ class Patch(Submission): 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 @@ -586,7 +549,50 @@ class Patch(Submission): # 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 92fe2d7e..326e27bc 100644 --- a/patchwork/tests/test_detail.py +++ b/patchwork/tests/test_detail.py @@ -3,13 +3,19 @@ # # 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_comment from patchwork.tests.utils import create_cover from patchwork.tests.utils import create_patch from patchwork.tests.utils import create_project +from patchwork.tests.utils import create_user class CoverLetterViewTest(TestCase): @@ -156,6 +162,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('utf-8').count( + '%s/%s' % (check_a.user, check_a.context) + )) + self.assertEqual( + 1, response.content.decode('utf-8').count( + '%s/%s' % (check_b.user, check_b.context) + )) + class CommentRedirectTest(TestCase): diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py index 470ad197..34934dea 100644 --- a/patchwork/views/patch.py +++ b/patchwork/views/patch.py @@ -123,7 +123,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 -- 2.47.3