From patchwork Fri Sep 3 15:51:03 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stephen Finucane X-Patchwork-Id: 1524407 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; dkim=fail reason="key not found in DNS" header.d=that.guru header.i=@that.guru header.a=rsa-sha256 header.s=x header.b=X8GsvQCX; dkim-atps=neutral Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=lists.ozlabs.org (client-ip=112.213.38.117; helo=lists.ozlabs.org; envelope-from=patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org; receiver=) Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4H1MjP45Tsz9sPf for ; Sat, 4 Sep 2021 01:51:37 +1000 (AEST) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4H1MjP0sS6z2yNq for ; Sat, 4 Sep 2021 01:51:37 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="key not found in DNS" header.d=that.guru header.i=@that.guru header.a=rsa-sha256 header.s=x header.b=X8GsvQCX; dkim-atps=neutral X-Original-To: patchwork@lists.ozlabs.org Delivered-To: patchwork@lists.ozlabs.org Authentication-Results: lists.ozlabs.org; spf=none (no SPF record) smtp.mailfrom=that.guru (client-ip=136.175.108.193; helo=mail-108-mta193.mxroute.com; envelope-from=stephen@that.guru; receiver=) Authentication-Results: lists.ozlabs.org; dkim=fail reason="key not found in DNS" header.d=that.guru header.i=@that.guru header.a=rsa-sha256 header.s=x header.b=X8GsvQCX; dkim-atps=neutral Received: from mail-108-mta193.mxroute.com (mail-108-mta193.mxroute.com [136.175.108.193]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4H1MjH2T7Wz2yJv for ; Sat, 4 Sep 2021 01:51:29 +1000 (AEST) Received: from filter004.mxroute.com ([149.28.56.236] filter004.mxroute.com) (Authenticated sender: mN4UYu2MZsgR) by mail-108-mta193.mxroute.com (ZoneMTA) with ESMTPSA id 17bac5b79f300074ba.002 for (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES128-GCM-SHA256); Fri, 03 Sep 2021 15:51:21 +0000 X-Zone-Loop: 15bf2aef8af2e9db12e7d54aeb0bb6170d8ca0cb4392 X-Originating-IP: [149.28.56.236] DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=that.guru; s=x; h=Content-Transfer-Encoding:MIME-Version:Message-Id:Date:Subject:Cc:To: From:Sender:Reply-To:Content-Type:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To: References:List-Id:List-Help:List-Unsubscribe:List-Subscribe:List-Post: List-Owner:List-Archive; bh=McP6d+JAN49BCfIlFyk732MFS9JgmTvbet/a62JnZ6c=; b=X 8GsvQCXN1aE9b4daeQ3d05jGZEHq7IRQd1z8pzGLs+jeBE5Vb05CuXLHjB541l+uyaeUUw1Km8Y49 K7jhBF6mmsZIizBc/VkhyvZmdoLsuDVMREZzeITBzeKCF9RcBGSgpHGov+cCVJweDhiX9Gno5K8Cv uNY2xN8x0VWSo76aLYYXgKSJTkKgybsk8Mm4ZQLX8ZAItIbQV0f2GMxgz8Lt7JOgc1lOrtg+5teAd 3wa+M9yrS5+V0jcx+GLj4pEl++a2JoYHKgFjuv6bl69Jb1s3yKiRxGWB+Nrwe2TYUAffu1xhuXGOZ qSWSy9y65+lYnV3gTNAY8Oq3P2U2owF4Q==; From: Stephen Finucane To: patchwork@lists.ozlabs.org Subject: [PATCH] models: Add Series.is_open Date: Fri, 3 Sep 2021 16:51:03 +0100 Message-Id: <20210903155103.39719-1-stephen@that.guru> X-Mailer: git-send-email 2.31.1 MIME-Version: 1.0 X-AuthUser: stephen@that.guru X-BeenThere: patchwork@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Patchwork development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Patchwork" Start making series more useful by tracking whether the state is open, meaning one or more patches are in a state with action_required=True, or not. This means that the 'git-pw series list' command will only list series that are actually actionable. Signed-off-by: Stephen Finucane --- Open questions: - Is there a more efficient way to do the migration than I've done? - Should we expose an 'is_open' boolean attribute of a 'state' (open, closed) attribute for the series views? - Is there a race condition possible in how I've implemented the series state checks? --- docs/api/schemas/latest/patchwork.yaml | 8 +++ docs/api/schemas/patchwork.j2 | 12 ++++ docs/api/schemas/v1.3/patchwork.yaml | 8 +++ patchwork/api/embedded.py | 7 +- patchwork/api/filters.py | 7 +- patchwork/api/series.py | 9 ++- patchwork/migrations/0046_series_is_open.py | 58 +++++++++++++++++ patchwork/models.py | 20 ++++++ patchwork/tests/api/test_series.py | 52 +++++++++++++++ patchwork/tests/test_models.py | 72 +++++++++++++++++++++ 10 files changed, 247 insertions(+), 6 deletions(-) create mode 100644 patchwork/migrations/0046_series_is_open.py create mode 100644 patchwork/tests/test_models.py diff --git docs/api/schemas/latest/patchwork.yaml docs/api/schemas/latest/patchwork.yaml index e3bff990..cb8fcdfa 100644 --- docs/api/schemas/latest/patchwork.yaml +++ docs/api/schemas/latest/patchwork.yaml @@ -2260,6 +2260,10 @@ components: description: > Version of series as indicated by the subject prefix(es). type: integer + is_open: + title: Status + type: boolean + readOnly: true total: title: Total description: > @@ -2610,6 +2614,10 @@ components: Version of series as indicated by the subject prefix(es). type: integer readOnly: true + is_open: + title: Status + type: boolean + readOnly: true mbox: title: Mbox type: string diff --git docs/api/schemas/patchwork.j2 docs/api/schemas/patchwork.j2 index 3b4ad2f6..13610b84 100644 --- docs/api/schemas/patchwork.j2 +++ docs/api/schemas/patchwork.j2 @@ -2354,6 +2354,12 @@ components: description: > Version of series as indicated by the subject prefix(es). type: integer +{% if version >= (1, 3) %} + is_open: + title: Status + type: boolean + readOnly: true +{% endif %} total: title: Total description: > @@ -2718,6 +2724,12 @@ components: Version of series as indicated by the subject prefix(es). type: integer readOnly: true +{% if version >= (1, 3) %} + is_open: + title: Status + type: boolean + readOnly: true +{% endif %} mbox: title: Mbox type: string diff --git docs/api/schemas/v1.3/patchwork.yaml docs/api/schemas/v1.3/patchwork.yaml index 6cbba646..3b38799c 100644 --- docs/api/schemas/v1.3/patchwork.yaml +++ docs/api/schemas/v1.3/patchwork.yaml @@ -2260,6 +2260,10 @@ components: description: > Version of series as indicated by the subject prefix(es). type: integer + is_open: + title: Status + type: boolean + readOnly: true total: title: Total description: > @@ -2610,6 +2614,10 @@ components: Version of series as indicated by the subject prefix(es). type: integer readOnly: true + is_open: + title: Status + type: boolean + readOnly: true mbox: title: Mbox type: string diff --git patchwork/api/embedded.py patchwork/api/embedded.py index 78316979..38a00ac4 100644 --- patchwork/api/embedded.py +++ patchwork/api/embedded.py @@ -181,11 +181,14 @@ class SeriesSerializer(SerializedRelatedField): class Meta: model = models.Series - fields = ('id', 'url', 'web_url', 'date', 'name', 'version', - 'mbox') + fields = ( + 'id', 'url', 'web_url', 'date', 'name', 'version', 'is_open', + 'mbox', + ) read_only_fields = fields versioned_fields = { '1.1': ('web_url', ), + '1.3': ('is_open', ), } extra_kwargs = { 'url': {'view_name': 'api-series-detail'}, diff --git patchwork/api/filters.py patchwork/api/filters.py index d9b65a8f..38de18b6 100644 --- patchwork/api/filters.py +++ patchwork/api/filters.py @@ -8,6 +8,7 @@ from django.core.exceptions import ValidationError from django.db.models import Q from django_filters import rest_framework from django_filters.rest_framework import FilterSet +from django_filters import BooleanFilter from django_filters import CharFilter from django_filters import IsoDateTimeFilter from django_filters import ModelMultipleChoiceFilter @@ -178,10 +179,14 @@ class SeriesFilterSet(TimestampMixin, BaseFilterSet): submitter = PersonFilter(queryset=Person.objects.all(), distinct=False) project = ProjectFilter(queryset=Project.objects.all(), distinct=False) + is_open = BooleanFilter() class Meta: model = Series - fields = ('submitter', 'project') + fields = ('submitter', 'project', 'is_open') + versioned_fields = { + '1.2': ('hash', 'msgid'), + } def msgid_filter(queryset, name, value): diff --git patchwork/api/series.py patchwork/api/series.py index 106e60f0..f3a498e0 100644 --- patchwork/api/series.py +++ patchwork/api/series.py @@ -36,13 +36,16 @@ class SeriesSerializer(BaseHyperlinkedModelSerializer): class Meta: model = Series - fields = ('id', 'url', 'web_url', 'project', 'name', 'date', - 'submitter', 'version', 'total', 'received_total', - 'received_all', 'mbox', 'cover_letter', 'patches') + fields = ( + 'id', 'url', 'web_url', 'project', 'name', 'date', + 'submitter', 'version', 'is_open', 'total', 'received_total', + 'received_all', 'mbox', 'cover_letter', 'patches', + ) read_only_fields = ('date', 'submitter', 'total', 'received_total', 'received_all', 'mbox', 'cover_letter', 'patches') versioned_fields = { '1.1': ('web_url', ), + '1.3': ('is_open', ), } extra_kwargs = { 'url': {'view_name': 'api-series-detail'}, diff --git patchwork/migrations/0046_series_is_open.py patchwork/migrations/0046_series_is_open.py new file mode 100644 index 00000000..c916ed8b --- /dev/null +++ patchwork/migrations/0046_series_is_open.py @@ -0,0 +1,58 @@ +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('patchwork', '0045_addressed_fields'), + ] + + operations = [ + migrations.AddField( + model_name='series', + name='is_open', + field=models.BooleanField( + help_text='Are all patches in this series resolved?', + null=True, + ), + ), + # Each patch in a series is associated with a state. States have an + # 'action_required' attribute. A state with 'action_required=True' is + # effectively "open", since the maintainer(s) still need to do + # something with it. This means the patches associated with these + # states are open, and by extension any series containing patches + # associated with these states are also open. + # + # Got that? Good. All we're doing here is setting the series' new + # 'is_open' attribute by identifying if any of the patches associated + # with each series have a state with 'action_required=True'. We do this + # aggregation using 'MAX', since booleans are stored as integers in + # most DB backends (1/True > 0/False) and ANSI SQL states True > False + + # TODO: Which of these offers the best performance? The first is MySQL + # only fwict + # migrations.RunSQL( + # """ + # UPDATE patchwork_series + # JOIN ( + # SELECT patch.series_id AS series_id, MAX(state.action_required) AS is_open # noqa: E501 + # FROM patchwork_patch patch + # JOIN patchwork_state state ON patch.state_id = state.id + # GROUP BY series_id + # ) AS sub ON patchwork_series.id = sub.series_id + # SET patchwork_series.is_open = sub.is_open; + # """, # noqa: E501 + # ), + migrations.RunSQL( + """ + UPDATE patchwork_series + SET is_open = ( + SELECT MAX(patchwork_state.action_required) + FROM patchwork_patch + JOIN patchwork_state ON state_id = patchwork_state.id + WHERE patchwork_patch.series_id = patchwork_series.id + GROUP BY patchwork_patch.series_id + ); + """, + ), + ] diff --git patchwork/models.py patchwork/models.py index 58e4c51e..7441510b 100644 --- patchwork/models.py +++ patchwork/models.py @@ -14,6 +14,7 @@ from django.conf import settings from django.contrib.auth.models import User from django.core.exceptions import ValidationError from django.db import models +from django.db import transaction from django.urls import reverse from django.utils.functional import cached_property from django.core.validators import validate_unicode_slug @@ -494,6 +495,19 @@ class Patch(SubmissionMixin): for tag in tags: self._set_tag(tag, counter[tag]) + def refresh_series_state(self): + if not self.series: + return + + # If any of the patches in the series is in an "action required" state, + # then the series is still open + # TODO(stephenfin): Can this still race? + with transaction.atomic(): + self.series.is_open = self.series.patches.filter( + state__action_required=True + ).exists() + self.series.save() + def save(self, *args, **kwargs): if not hasattr(self, 'state') or not self.state: self.state = get_default_initial_patch_state() @@ -503,6 +517,7 @@ class Patch(SubmissionMixin): super(Patch, self).save(**kwargs) + self.refresh_series_state() self.refresh_tag_counts() def is_editable(self, user): @@ -772,6 +787,11 @@ class Series(FilenameMixin, models.Model): 'by the subject prefix(es)') total = models.IntegerField(help_text='Number of patches in series as ' 'indicated by the subject prefix(es)') + is_open = models.BooleanField( + default=True, + null=True, + help_text='Are all patches in this series resolved?', + ) @staticmethod def _format_name(obj): diff --git patchwork/tests/api/test_series.py patchwork/tests/api/test_series.py index ef661773..830b2fdb 100644 --- patchwork/tests/api/test_series.py +++ patchwork/tests/api/test_series.py @@ -16,6 +16,7 @@ from patchwork.tests.utils import create_patch from patchwork.tests.utils import create_person from patchwork.tests.utils import create_project from patchwork.tests.utils import create_series +from patchwork.tests.utils import create_state from patchwork.tests.utils import create_user if settings.ENABLE_REST_API: @@ -122,6 +123,57 @@ class TestSeriesAPI(utils.APITestCase): 'submitter': 'test@example.org'}) self.assertEqual(0, len(resp.data)) + def test_list_filter_is_open(self): + """Filter series by status.""" + project_obj = create_project(linkname='myproject') + person_obj = create_person(email='test@example.com') + + # create two series with a single patch in each with different states + # for the two patches + + state_a = create_state(name='New', slug='new', action_required=True) + series_a = create_series(project=project_obj, submitter=person_obj) + create_cover(series=series_a) + create_patch(series=series_a, state=state_a) + self.assertTrue(series_a.is_open) + + state_b = create_state( + name='Accepted', slug='Accepted', action_required=False, + ) + series_b = create_series(project=project_obj, submitter=person_obj) + create_cover(series=series_b) + create_patch(series=series_b, state=state_b) + self.assertFalse(series_b.is_open) + + resp = self.client.get(self.api_url(), {'is_open': 'True'}) + self.assertEqual([series_a.id], [x['id'] for x in resp.data]) + + resp = self.client.get(self.api_url(), {'is_open': 'False'}) + self.assertEqual([series_b.id], [x['id'] for x in resp.data]) + + resp = self.client.get(self.api_url()) + self.assertEqual( + [series_a.id, series_b.id], [x['id'] for x in resp.data] + ) + + @utils.store_samples('series-list-1-2') + def test_list_version_1_2(self): + """List patches using API v1.2. + + Validate that newer fields are dropped for older API versions. + """ + self._create_series() + + resp = self.client.get(self.api_url(version='1.2')) + self.assertEqual(status.HTTP_200_OK, resp.status_code) + self.assertEqual(1, len(resp.data)) + self.assertIn('url', resp.data[0]) + self.assertIn('web_url', resp.data[0]) + self.assertIn('web_url', resp.data[0]['cover_letter']) + self.assertIn('mbox', resp.data[0]['cover_letter']) + self.assertIn('web_url', resp.data[0]['patches'][0]) + self.assertNotIn('is_open', resp.data[0]) + @utils.store_samples('series-list-1-0') def test_list_version_1_0(self): """List patches using API v1.0. diff --git patchwork/tests/test_models.py patchwork/tests/test_models.py new file mode 100644 index 00000000..13db3330 --- /dev/null +++ patchwork/tests/test_models.py @@ -0,0 +1,72 @@ +# Patchwork - automated patch tracking system +# Copyright (C) 2021 Stephen Finucane +# +# SPDX-License-Identifier: GPL-2.0-or-later + +from django.test import TestCase + +from patchwork.tests.utils import create_patch +from patchwork.tests.utils import create_series +from patchwork.tests.utils import create_state + + +class TestPatch(TestCase): + + def test_save_updates_series_state(self): + state_new = create_state( + name='New', + slug='new', + action_required=True, + ) + state_under_review = create_state( + name='Under Review', + slug='under-review', + action_required=True, + ) + state_accepted = create_state( + name='Accepted', + slug='accepted', + action_required=False, + ) + state_rejected = create_state( + name='Rejected', + slug='rejected', + action_required=False, + ) + series = create_series() + patches = [] + + # Create four patches - one in each state + for state in ( + state_new, state_under_review, state_accepted, state_rejected, + ): + patches.append(create_patch(series=series, state=state)) + + # The patches will be in various states so the series should still be + # open + self.assertTrue(patches[0].state.action_required) + self.assertTrue(patches[1].state.action_required) + self.assertFalse(patches[2].state.action_required) + self.assertFalse(patches[3].state.action_required) + self.assertTrue(series.is_open) + + # Now change the first patch to 'accepted'. The series shouldn't change + # state because the second one is still 'under-review' + patches[0].state = state_accepted + patches[0].save() + self.assertFalse(patches[0].state.action_required) + self.assertTrue(patches[1].state.action_required) + self.assertFalse(patches[2].state.action_required) + self.assertFalse(patches[3].state.action_required) + self.assertTrue(series.is_open) + + # Now change the second patch to 'rejected'. The series should finally + # change state because all patches now have a state with + # action_required=False + patches[1].state = state_rejected + patches[1].save() + self.assertFalse(patches[0].state.action_required) + self.assertFalse(patches[1].state.action_required) + self.assertFalse(patches[2].state.action_required) + self.assertFalse(patches[3].state.action_required) + self.assertFalse(series.is_open)