[PATCH] models: Add Series.is_open
Stephen Finucane
stephen at that.guru
Sat Sep 4 01:51:03 AEST 2021
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 <stephen at that.guru>
---
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 at 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 at 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 <stephen at that.guru>
+#
+# 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)
--
2.31.1
More information about the Patchwork
mailing list