From patchwork Fri Dec 20 10:49:21 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stephen Finucane X-Patchwork-Id: 1214010 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 47fQVp680dz9sNH for ; Fri, 20 Dec 2019 21:49:54 +1100 (AEDT) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=that.guru Authentication-Results: ozlabs.org; dkim=fail reason="key not found in DNS" (0-bit key; unprotected) header.d=that.guru header.i=@that.guru header.b="HU9D8fob"; dkim-atps=neutral Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 47fQVp0GnfzDqwL for ; Fri, 20 Dec 2019 21:49:54 +1100 (AEDT) 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=160.202.107.21; helo=q2relay21.mxroute.com; envelope-from=stephen@that.guru; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=that.guru Authentication-Results: lists.ozlabs.org; dkim=fail reason="key not found in DNS" (0-bit key; unprotected) header.d=that.guru header.i=@that.guru header.b="HU9D8fob"; dkim-atps=neutral Received: from q2relay21.mxroute.com (q2relay21.mxroute.com [160.202.107.21]) (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 47fQVV4s9XzDqsw for ; Fri, 20 Dec 2019 21:49:37 +1100 (AEDT) Received: from filter003.mxroute.com [168.235.111.26] (Authenticated sender: mN4UYu2MZsgR) by q2relay21.mxroute.com (ZoneMTA) with ESMTPSA id 16f22ed3a6500026db.002 for (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES128-GCM-SHA256); Fri, 20 Dec 2019 10:49:29 +0000 X-Zone-Loop: 37bb6e237c5b2653ae417ddb537c2759ca4674dee9c6 X-Originating-IP: [168.235.111.26] Received: from one.mxroute.com (one.mxroute.com [195.201.59.211]) by filter003.mxroute.com (Postfix) with ESMTPS id 800A860005; Fri, 20 Dec 2019 10:49:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=that.guru; s=default; 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=iXjqYWP1n9yb2JNQbLEB9Ktq2jEiM8djvZKTB6CaqLA=; b=HU9D8fob4zZNheTqBHI7y2lfOG mTf9czmEH89vjTy+10a6tcFnZw7UhXw95qOdWhL6JbSuIvUfJSdsB+JJP14TIrR9McZkFAMgrraC9 KopAhFKK3e0kCAe0OjLnT5CTt6lbIhkRafuLlK/zYhp/aD8tHsCAB3k8w5I2Ap89PYJjtw7+AtIh4 u+G4jL+1SvtYnN41DaPfw5UwDGTehXTTfAdItACUThZIJwlL7KcA/sVrb5f/M/U9GtpP4ZHeyhF5O MV9uLl+id4t5FquAnt+rqUNng4Mym6AdyPEietb+NSvXA5gzU74z3l1lL4zOuKgfqB3HCEUizwE5E Ulmh52eg==; From: Stephen Finucane To: patchwork@lists.ozlabs.org Subject: [PATCH v2] models: Add State.slug field Date: Fri, 20 Dec 2019 10:49:21 +0000 Message-Id: <20191220104921.9410-1-stephen@that.guru> X-Mailer: git-send-email 2.24.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: , Cc: Andrew Donnellan Errors-To: patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Patchwork" This reduces a lot of the tech debt we had built up around this. This field is populated from a slugified representation of State.name and has a uniqueness constraint. As a result, we also need to a uniqueness constraint to the State.name field. This shouldn't be an issue and probably should have been used from day one. Note that there is no REST API impact for this since we've been using state slugs all along. Signed-off-by: Stephen Finucane --- v2: - Handle states that have different names but slugify to the same thing. --- patchwork/api/event.py | 6 +-- patchwork/api/patch.py | 18 ++----- patchwork/fixtures/default_states.xml | 10 ++++ patchwork/migrations/0038_state_slug.py | 68 +++++++++++++++++++++++++ patchwork/models.py | 8 ++- patchwork/tests/api/test_patch.py | 4 +- patchwork/tests/utils.py | 1 + 7 files changed, 92 insertions(+), 23 deletions(-) create mode 100644 patchwork/migrations/0038_state_slug.py diff --git a/patchwork/api/event.py b/patchwork/api/event.py index e00ce051..a066faae 100644 --- a/patchwork/api/event.py +++ b/patchwork/api/event.py @@ -8,6 +8,7 @@ from collections import OrderedDict from rest_framework.generics import ListAPIView from rest_framework.serializers import ModelSerializer from rest_framework.serializers import SerializerMethodField +from rest_framework.serializers import SlugRelatedField from patchwork.api.embedded import CheckSerializer from patchwork.api.embedded import CoverLetterSerializer @@ -16,7 +17,6 @@ from patchwork.api.embedded import ProjectSerializer from patchwork.api.embedded import SeriesSerializer from patchwork.api.embedded import UserSerializer from patchwork.api.filters import EventFilterSet -from patchwork.api.patch import StateField from patchwork.models import Event @@ -27,8 +27,8 @@ class EventSerializer(ModelSerializer): patch = PatchSerializer(read_only=True) series = SeriesSerializer(read_only=True) cover = CoverLetterSerializer(read_only=True) - previous_state = StateField() - current_state = StateField() + previous_state = SlugRelatedField(slug_field='slug', read_only=True) + current_state = SlugRelatedField(slug_field='slug', read_only=True) previous_delegate = UserSerializer() current_delegate = UserSerializer() created_check = SerializerMethodField() diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py index d1c9904d..a29a1ab0 100644 --- a/patchwork/api/patch.py +++ b/patchwork/api/patch.py @@ -5,6 +5,7 @@ import email.parser +from django.utils.text import slugify from django.utils.translation import ugettext_lazy as _ from rest_framework.generics import ListAPIView from rest_framework.generics import RetrieveUpdateAPIView @@ -28,10 +29,7 @@ from patchwork.parser import clean_subject class StateField(RelatedField): """Avoid the need for a state endpoint. - NOTE(stephenfin): This field will only function for State names consisting - of alphanumeric characters, underscores and single spaces. In Patchwork - 2.0+, we should consider adding a slug field to the State object and make - use of the SlugRelatedField in DRF. + TODO(stephenfin): Consider switching to SlugRelatedField for the v2.0 API. """ default_error_messages = { 'required': _('This field is required.'), @@ -41,19 +39,13 @@ class StateField(RelatedField): '{data_type}.'), } - @staticmethod - def format_state_name(state): - return ' '.join(state.split('-')) - def to_internal_value(self, data): + data = slugify(data.lower()) try: - data = self.format_state_name(data) - return self.get_queryset().get(name__iexact=data) + return self.get_queryset().get(slug=data) except State.DoesNotExist: self.fail('invalid_choice', name=data, choices=', '.join([ - self.format_state_name(x.name) for x in self.get_queryset()])) - except (TypeError, ValueError): - self.fail('incorrect_type', data_type=type(data).__name__) + x.slug for x in self.get_queryset()])) def to_representation(self, obj): return obj.slug diff --git a/patchwork/fixtures/default_states.xml b/patchwork/fixtures/default_states.xml index 86e1105f..1bbd919d 100644 --- a/patchwork/fixtures/default_states.xml +++ b/patchwork/fixtures/default_states.xml @@ -4,51 +4,61 @@ New + new 0 True Under Review + under-review 1 True Accepted + accepted 2 False Rejected + rejected 3 False RFC + rfc 4 False Not Applicable + not-applicable 5 False Changes Requested + changes-requested 6 False Awaiting Upstream + awaiting-upstream 7 False Superseded + superseded 8 False Deferred + deferred 9 False diff --git a/patchwork/migrations/0038_state_slug.py b/patchwork/migrations/0038_state_slug.py new file mode 100644 index 00000000..500624d1 --- /dev/null +++ b/patchwork/migrations/0038_state_slug.py @@ -0,0 +1,68 @@ +# -*- coding: utf-8 -*- + +from __future__ import unicode_literals + +from django.db import migrations, models, transaction +from django.utils.text import slugify + + +def validate_uniqueness(apps, schema_editor): + """Ensure all State.name entries are unique. + + We need to do this before enforcing a uniqueness constraint. + """ + + State = apps.get_model('patchwork', 'State') + + total_count = State.objects.count() + slugs_count = len(set([ + slugify(state.name) for state in State.objects.all()])) + + if slugs_count != total_count: + raise Exception( + 'You have non-unique States entries that need to be combined ' + 'before you can run this migration. This migration must be done ' + 'by hand. If you need assistance, please contact ' + 'patchwork@ozlabs.org') + + +def populate_slug_field(apps, schema_editor): + + State = apps.get_model('patchwork', 'State') + + with transaction.atomic(): + for state in State.objects.all(): + state.slug = slugify(state.name) + state.save() + + +class Migration(migrations.Migration): + + dependencies = [ + ('patchwork', '0037_event_actor'), + ] + + operations = [ + # Ensure all 'State.name' entries are unique + migrations.RunPython(validate_uniqueness, migrations.RunPython.noop), + # Apply the unique constraint to 'State.name' + migrations.AlterField( + model_name='state', + name='name', + field=models.CharField(max_length=100, unique=True), + ), + # Add a 'State.slug' field but allow it to be nullable + migrations.AddField( + model_name='state', + name='slug', + field=models.SlugField(blank=True, max_length=100, null=True, unique=True), + ), + # Populate the 'State.slug' field + migrations.RunPython(populate_slug_field, migrations.RunPython.noop), + # Make the 'State.slug' field non-nullable + migrations.AlterField( + model_name='state', + name='slug', + field=models.SlugField(max_length=100, unique=True), + ), + ] diff --git a/patchwork/models.py b/patchwork/models.py index 7f0efd48..d95eb0d9 100644 --- a/patchwork/models.py +++ b/patchwork/models.py @@ -215,14 +215,12 @@ models.signals.post_save.connect(_user_saved_callback, sender=User) @python_2_unicode_compatible class State(models.Model): - name = models.CharField(max_length=100) + # Both of these fields should be unique + name = models.CharField(max_length=100, unique=True) + slug = models.SlugField(max_length=100, unique=True) ordering = models.IntegerField(unique=True) action_required = models.BooleanField(default=True) - @property - def slug(self): - return '-'.join(self.name.lower().split()) - def __str__(self): return self.name diff --git a/patchwork/tests/api/test_patch.py b/patchwork/tests/api/test_patch.py index bf3ef9f8..ef418e2e 100644 --- a/patchwork/tests/api/test_patch.py +++ b/patchwork/tests/api/test_patch.py @@ -300,7 +300,7 @@ class TestPatchAPI(utils.APITestCase): self.client.force_authenticate(user=user) resp = self.client.patch(self.api_url(patch.id), - {'state': state.name, 'delegate': user.id}) + {'state': state.slug, 'delegate': user.id}) self.assertEqual(status.HTTP_200_OK, resp.status_code, resp) self.assertEqual(Patch.objects.get(id=patch.id).state, state) self.assertEqual(Patch.objects.get(id=patch.id).delegate, user) @@ -326,7 +326,7 @@ class TestPatchAPI(utils.APITestCase): self.client.force_authenticate(user=user) resp = self.client.patch(self.api_url(patch.id), {'state': 'foobar'}) self.assertEqual(status.HTTP_400_BAD_REQUEST, resp.status_code) - self.assertContains(resp, 'Expected one of: %s.' % state.name, + self.assertContains(resp, 'Expected one of: %s.' % state.slug, status_code=status.HTTP_400_BAD_REQUEST) def test_update_legacy_delegate(self): diff --git a/patchwork/tests/utils.py b/patchwork/tests/utils.py index 577183d0..91bcbe12 100644 --- a/patchwork/tests/utils.py +++ b/patchwork/tests/utils.py @@ -131,6 +131,7 @@ def create_state(**kwargs): values = { 'name': 'state_%d' % num, + 'slug': 'state-%d' % num, 'ordering': num, 'action_required': True, }