diff mbox series

models: Add State.slug field

Message ID 20191208160014.44465-1-stephen@that.guru
State Superseded
Headers show
Series models: Add State.slug field | expand

Commit Message

Stephen Finucane Dec. 8, 2019, 4 p.m. UTC 
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 <stephen@that.guru>
---
 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

Comments

Andrew Donnellan Dec. 10, 2019, 4:43 a.m. UTC | #1 
On 9/12/19 3:00 am, Stephen Finucane wrote:
> --- /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')
> +
> +    unique_count = len(State.objects.order_by().values_list(
> +        'name', flat=True).distinct())
> +    total_count = State.objects.count()
> +
> +    if unique_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')

I think the migration can still fail without printing a helpful 
exception message if two states have names that are different but still 
slugify to the same thing?
Stephen Finucane Dec. 10, 2019, 3:32 p.m. UTC | #2 
On Tue, 2019-12-10 at 15:43 +1100, Andrew Donnellan wrote:
> On 9/12/19 3:00 am, Stephen Finucane wrote:
> > --- /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')
> > +
> > +    unique_count = len(State.objects.order_by().values_list(
> > +        'name', flat=True).distinct())
> > +    total_count = State.objects.count()
> > +
> > +    if unique_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')
> 
> I think the migration can still fail without printing a helpful 
> exception message if two states have names that are different but still 
> slugify to the same thing?

Good point. I can fix this if we think it's worth it. Personally, I
think the uniqueness check itself is overkill and could probably be
removed.

Stephen
Andrew Donnellan Dec. 11, 2019, 12:32 a.m. UTC | #3 
On 11/12/19 2:32 am, Stephen Finucane wrote:
>> I think the migration can still fail without printing a helpful
>> exception message if two states have names that are different but still
>> slugify to the same thing?
> 
> Good point. I can fix this if we think it's worth it. Personally, I
> think the uniqueness check itself is overkill and could probably be
> removed.

Agreed, though if we're gonna do it I want it done thoroughly ;)
Stephen Finucane Dec. 27, 2019, 1:02 p.m. UTC | #4 
On Sun, 2019-12-08 at 16:00 +0000, Stephen Finucane wrote:
> 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 <stephen@that.guru>

Applied.
diff mbox series

Patch

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 @@ 
   <!-- default states -->
   <object pk="1" model="patchwork.state">
     <field type="CharField" name="name">New</field>
+    <field type="SlugField" name="name">new</field>
     <field type="IntegerField" name="ordering">0</field>
     <field type="BooleanField" name="action_required">True</field>
   </object>
   <object pk="2" model="patchwork.state">
     <field type="CharField" name="name">Under Review</field>
+    <field type="SlugField" name="name">under-review</field>
     <field type="IntegerField" name="ordering">1</field>
     <field type="BooleanField" name="action_required">True</field>
   </object>
   <object pk="3" model="patchwork.state">
     <field type="CharField" name="name">Accepted</field>
+    <field type="SlugField" name="name">accepted</field>
     <field type="IntegerField" name="ordering">2</field>
     <field type="BooleanField" name="action_required">False</field>
   </object>
   <object pk="4" model="patchwork.state">
     <field type="CharField" name="name">Rejected</field>
+    <field type="SlugField" name="name">rejected</field>
     <field type="IntegerField" name="ordering">3</field>
     <field type="BooleanField" name="action_required">False</field>
   </object>
   <object pk="5" model="patchwork.state">
     <field type="CharField" name="name">RFC</field>
+    <field type="SlugField" name="name">rfc</field>
     <field type="IntegerField" name="ordering">4</field>
     <field type="BooleanField" name="action_required">False</field>
   </object>
   <object pk="6" model="patchwork.state">
     <field type="CharField" name="name">Not Applicable</field>
+    <field type="SlugField" name="name">not-applicable</field>
     <field type="IntegerField" name="ordering">5</field>
     <field type="BooleanField" name="action_required">False</field>
   </object>
   <object pk="7" model="patchwork.state">
     <field type="CharField" name="name">Changes Requested</field>
+    <field type="SlugField" name="name">changes-requested</field>
     <field type="IntegerField" name="ordering">6</field>
     <field type="BooleanField" name="action_required">False</field>
   </object>
   <object pk="8" model="patchwork.state">
     <field type="CharField" name="name">Awaiting Upstream</field>
+    <field type="SlugField" name="name">awaiting-upstream</field>
     <field type="IntegerField" name="ordering">7</field>
     <field type="BooleanField" name="action_required">False</field>
   </object>
   <object pk="9" model="patchwork.state">
     <field type="CharField" name="name">Superseded</field>
+    <field type="SlugField" name="name">superseded</field>
     <field type="IntegerField" name="ordering">8</field>
     <field type="BooleanField" name="action_required">False</field>
   </object>
   <object pk="10" model="patchwork.state">
     <field type="CharField" name="name">Deferred</field>
+    <field type="SlugField" name="name">deferred</field>
     <field type="IntegerField" name="ordering">9</field>
     <field type="BooleanField" name="action_required">False</field>
   </object>
diff --git a/patchwork/migrations/0038_state_slug.py b/patchwork/migrations/0038_state_slug.py
new file mode 100644
index 00000000..7615ee4f
--- /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')
+
+    unique_count = len(State.objects.order_by().values_list(
+        'name', flat=True).distinct())
+    total_count = State.objects.count()
+
+    if unique_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,
     }