models: Convert Series-Patch relationship to 1:N

Late in the development of the series feature, it was decided that there
were advantages to allowing an N:M relationship between series and
patches. This would allow us to do things like create complete series
where a sole vN patch was sent to a list rather than the full series.
After some time using series in the wild, it's apparent that such
features are very difficult to implement correctly and will likely never
be implemented. As such, it's time to start cleaning up the mess, paving
the way for things like an improved tagging feature.

There are some significant changes to the model required:

- models.py, migrations/0027, migrations/0028, migrations/0029

  The migrations make the following changes:

  1. - Add 'Patch.series_alt' and 'Patch.number' fields.
  2. - Populate the 'Patch.series_alt' and 'Patch.number' fields from
       their 'SeriesPatch' equivalents.
  3. - Remove the 'SeriesPatch' model.
     - Rename 'Patch.series_alt' to 'Patch.series'.
     - Change 'Series.cover_letter' to a 'OneToOneField' since a cover
       letter can no longer be assigned to multiple series.

  Note that the migrations have to be split into multiple parts as the
  combined migration raises an OperationalError as below.

    (1072, "Key column 'series_alt_id' doesn't exist in table")

  This is due to Django's penchant for creating indexes for newly
  created fields, as noted here: https://stackoverflow.com/q/35158530/

Aside from the model changes, there are numerous other changes required:

- admin.py

  Reflect model changes for the 'PatchInline' inline used by
  'SeriesAdmin'

- api/cover.py, api/patch.py

  Update the 'series' field for the cover letter and patch resources to
  reflect the model changes. A 'to_representation' function is added in
  both cases to post-process this field and make it look like a list
  again. This is necessary to avoid breaking clients.

- parser.py

  Update to reflect the replacement of 'SeriesPatch' with 'Patch'.

- signals.py

  Update to filter on changes to 'Patch' instead of 'SeriesPatch'. This
  requires some reworking due to how we set these fields now, as we can
  no longer receive on 'post_save' signals for 'SeriesPatch' and must
  instead watch for 'pre_save' on 'Patch', which is what we do for
  delegate and state changes on same.

- templates/patchwork/*.html

  Remove logic that handled multiple series in favour of the (simpler)
  single series logic.

- tests/*

  Modify the 'create_series_patch' helper to reflect the removal of the
  'SeriesPatch' model. This entire helper will be removed in a future
  change. Improve some tests to cover edge cases that were highlighted
  during development

Unfortunately, all of the above changes must go in at the same time,
otherwise we end up with either (a) broken views, API etc. or (b) split
brain because we need to keep the new single-series fields alongside the
older multi-series fields and models while we rework the views. It's
unfortunate but there's not much to be done here.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Tested-by: Daniel Axtens <dja@axtens.net>

Author: stephenfin

lib/sql/grant-all.mysql.sql

@@ -27,7 +27,6 @@ GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_patchtag TO 'www-data'@localho
 GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_person TO 'www-data'@localhost;
 GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_project TO 'www-data'@localhost;
 GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_series TO 'www-data'@localhost;
-GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_seriespatch TO 'www-data'@localhost;
 GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_seriesreference TO 'www-data'@localhost;
 GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_state TO 'www-data'@localhost;
 GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_submission TO 'www-data'@localhost;
@@ -42,7 +41,6 @@ GRANT INSERT, SELECT ON patchwork_coverletter TO 'nobody'@localhost;
 GRANT INSERT, SELECT ON patchwork_patch TO 'nobody'@localhost;
 GRANT INSERT, SELECT ON patchwork_person TO 'nobody'@localhost;
 GRANT INSERT, SELECT ON patchwork_series TO 'nobody'@localhost;
-GRANT INSERT, SELECT ON patchwork_seriespatch TO 'nobody'@localhost;
 GRANT INSERT, SELECT ON patchwork_seriesreference TO 'nobody'@localhost;
 GRANT INSERT, SELECT ON patchwork_submission TO 'nobody'@localhost;
 GRANT INSERT, SELECT, UPDATE, DELETE ON patchwork_patchtag TO 'nobody'@localhost;

lib/sql/grant-all.postgres.sql

@@ -27,7 +27,6 @@ GRANT SELECT, UPDATE, INSERT, DELETE ON
 	patchwork_person,
 	patchwork_project,
 	patchwork_series,
-	patchwork_seriespatch,
 	patchwork_seriesreference,
 	patchwork_state,
 	patchwork_submission,
@@ -56,7 +55,6 @@ GRANT SELECT, UPDATE ON
 	patchwork_person_id_seq,
 	patchwork_project_id_seq,
 	patchwork_series_id_seq,
-	patchwork_seriespatch_id_seq,
 	patchwork_seriesreference_id_seq,
 	patchwork_state_id_seq,
 	patchwork_tag_id_seq,
@@ -70,7 +68,6 @@ GRANT INSERT, SELECT ON
 	patchwork_comment,
 	patchwork_coverletter,
 	patchwork_event
-	patchwork_seriespatch,
 	patchwork_seriesreference,
 	patchwork_submission,
 TO "nobody";
@@ -93,7 +90,6 @@ GRANT UPDATE, SELECT ON
 	patchwork_patchtag_id_seq,
 	patchwork_person_id_seq,
 	patchwork_series_id_seq,
-	patchwork_seriespatch_id_seq,
 	patchwork_seriesreference_id_seq,
 TO "nobody";
 

patchwork/admin.py

@@ -114,7 +114,7 @@ class CommentAdmin(admin.ModelAdmin):
 
 
 class PatchInline(admin.StackedInline):
-    model = Series.patches.through
+    model = Patch
     extra = 0
 
 

patchwork/api/cover.py

@@ -24,7 +24,7 @@ class CoverLetterListSerializer(BaseHyperlinkedModelSerializer):
     project = ProjectSerializer(read_only=True)
     submitter = PersonSerializer(read_only=True)
     mbox = SerializerMethodField()
-    series = SeriesSerializer(many=True, read_only=True)
+    series = SeriesSerializer(read_only=True)
     comments = SerializerMethodField()
 
     def get_web_url(self, instance):
@@ -39,6 +39,15 @@ def get_comments(self, cover):
         return self.context.get('request').build_absolute_uri(
             reverse('api-cover-comment-list', kwargs={'pk': cover.id}))
 
+    def to_representation(self, instance):
+        # NOTE(stephenfin): This is here to ensure our API looks the same even
+        # after we changed the series-patch relationship from M:N to 1:N. It
+        # will be removed in API v2
+        data = super(CoverLetterListSerializer, self).to_representation(
+            instance)
+        data['series'] = [data['series']]
+        return data
+
     class Meta:
         model = CoverLetter
         fields = ('id', 'url', 'web_url', 'project', 'msgid', 'date', 'name',
@@ -89,8 +98,8 @@ class CoverLetterList(ListAPIView):
     ordering = 'id'
 
     def get_queryset(self):
-        return CoverLetter.objects.all().prefetch_related('series')\
-            .select_related('project', 'submitter')\
+        return CoverLetter.objects.all()\
+            .select_related('project', 'submitter', 'series')\
             .defer('content', 'headers')
 
 
@@ -100,5 +109,5 @@ class CoverLetterDetail(RetrieveAPIView):
     serializer_class = CoverLetterDetailSerializer
 
     def get_queryset(self):
-        return CoverLetter.objects.all().prefetch_related('series')\
-            .select_related('project', 'submitter')
+        return CoverLetter.objects.all()\
+            .select_related('project', 'submitter', 'series')

patchwork/api/patch.py

@@ -70,7 +70,7 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer):
     submitter = PersonSerializer(read_only=True)
     delegate = UserSerializer(allow_null=True)
     mbox = SerializerMethodField()
-    series = SeriesSerializer(many=True, read_only=True)
+    series = SeriesSerializer(read_only=True)
     comments = SerializerMethodField()
     check = SerializerMethodField()
     checks = SerializerMethodField()
@@ -111,6 +111,14 @@ def validate_delegate(self, value):
                                   "'%s'" % (value, self.instance.project))
         return value
 
+    def to_representation(self, instance):
+        # NOTE(stephenfin): This is here to ensure our API looks the same even
+        # after we changed the series-patch relationship from M:N to 1:N. It
+        # will be removed in API v2
+        data = super(PatchListSerializer, self).to_representation(instance)
+        data['series'] = [data['series']]
+        return data
+
     class Meta:
         model = Patch
         fields = ('id', 'url', 'web_url', 'project', 'msgid', 'date', 'name',
@@ -173,8 +181,9 @@ class PatchList(ListAPIView):
 
     def get_queryset(self):
         return Patch.objects.all()\
-            .prefetch_related('series', 'check_set')\
-            .select_related('project', 'state', 'submitter', 'delegate')\
+            .prefetch_related('check_set')\
+            .select_related('project', 'state', 'submitter', 'delegate',
+                            'series')\
             .defer('content', 'diff', 'headers')
 
 
@@ -186,5 +195,6 @@ class PatchDetail(RetrieveUpdateAPIView):
 
     def get_queryset(self):
         return Patch.objects.all()\
-            .prefetch_related('series', 'check_set')\
-            .select_related('project', 'state', 'submitter', 'delegate')
+            .prefetch_related('check_set')\
+            .select_related('project', 'state', 'submitter', 'delegate',
+                            'series')

patchwork/migrations/0031_add_patch_series_fields.py

@@ -0,0 +1,32 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+from django.db.models import Count
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('patchwork', '0030_add_submission_covering_index'),
+    ]
+
+    operations = [
+        # Add Patch.series_alt, Patch.number fields. This will store the fields
+        # currently stored in SeriesPatch
+        migrations.AddField(
+            model_name='patch',
+            name='number',
+            field=models.PositiveSmallIntegerField(default=None, help_text=b'The number assigned to this patch in the series', null=True),
+        ),
+        migrations.AddField(
+            model_name='patch',
+            name='series_alt',
+            field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='patchwork.Series'),
+        ),
+        migrations.AlterUniqueTogether(
+            name='patch',
+            unique_together=set([('series_alt', 'number')]),
+        ),
+    ]

patchwork/migrations/0032_migrate_data_from_series_patch_to_patch.py

@@ -0,0 +1,35 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+from django.db.models import Count
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('patchwork', '0031_add_patch_series_fields'),
+    ]
+
+    operations = [
+        # Copy SeriesPatch.series, SeriesPatch.number to Patch.series_alt,
+        # Patch.number. Note that there is no uniqueness check here because no
+        # code actually allowed us to save multiple series
+        migrations.RunSQL(
+            """UPDATE patchwork_patch SET series_alt_id =
+                  (SELECT series_id from patchwork_seriespatch
+                   WHERE patchwork_seriespatch.patch_id =
+                            patchwork_patch.submission_ptr_id);
+               UPDATE patchwork_patch SET number =
+                   (SELECT number from patchwork_seriespatch
+                    WHERE patchwork_seriespatch.patch_id =
+                             patchwork_patch.submission_ptr_id);
+            """,
+            """INSERT INTO patchwork_seriespatch
+                  (patch_id, series_id, number)
+                SELECT submission_ptr_id, series_alt_id, number
+                FROM patchwork_patch;
+            """,
+        ),
+    ]

patchwork/migrations/0033_remove_patch_series_model.py

@@ -0,0 +1,58 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('patchwork', '0032_migrate_data_from_series_patch_to_patch'),
+    ]
+
+    operations = [
+        # Remove SeriesPatch
+        migrations.AlterUniqueTogether(
+            name='seriespatch',
+            unique_together=set([]),
+        ),
+        migrations.RemoveField(
+            model_name='seriespatch',
+            name='patch',
+        ),
+        migrations.RemoveField(
+            model_name='seriespatch',
+            name='series',
+        ),
+        migrations.RemoveField(
+            model_name='series',
+            name='patches',
+        ),
+        migrations.DeleteModel(
+            name='SeriesPatch',
+        ),
+        # Now that SeriesPatch has been removed, we can use the now-unused
+        # Patch.series field and add a backreference
+        migrations.RenameField(
+            model_name='patch',
+            old_name='series_alt',
+            new_name='series',
+        ),
+        migrations.AlterField(
+            model_name='patch',
+            name='series',
+            field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='patches', related_query_name='patch', to='patchwork.Series'),
+        ),
+        migrations.AlterUniqueTogether(
+            name='patch',
+            unique_together=set([('series', 'number')]),
+        ),
+        # Migrate CoverLetter to OneToOneField as a cover letter can no longer
+        # be assigned to multiple series
+        migrations.AlterField(
+            model_name='series',
+            name='cover_letter',
+            field=models.OneToOneField(null=True, on_delete=django.db.models.deletion.CASCADE, related_name='series', to='patchwork.CoverLetter'),
+        ),
+    ]

patchwork/models.py

@@ -407,6 +407,15 @@ class Patch(Submission):
     # patches in a project without needing to do a JOIN.
     patch_project = models.ForeignKey(Project, on_delete=models.CASCADE)
 
+    # series metadata
+
+    series = models.ForeignKey(
+        'Series', null=True, blank=True, on_delete=models.CASCADE,
+        related_name='patches', related_query_name='patch')
+    number = models.PositiveSmallIntegerField(
+        default=None, null=True,
+        help_text='The number assigned to this patch in the series')
+
     objects = PatchManager()
 
     @staticmethod
@@ -563,6 +572,7 @@ def __str__(self):
     class Meta:
         verbose_name_plural = 'Patches'
         base_manager_name = 'objects'
+        unique_together = [('series', 'number')]
 
         indexes = [
             # This is a covering index for the /list/ query
@@ -606,19 +616,16 @@ class Meta:
 
 @python_2_unicode_compatible
 class Series(FilenameMixin, models.Model):
-    """An collection of patches."""
+    """A collection of patches."""
 
     # parent
     project = models.ForeignKey(Project, related_name='series', null=True,
                                 blank=True, on_delete=models.CASCADE)
 
     # content
-    cover_letter = models.ForeignKey(CoverLetter,
-                                     related_name='series',
-                                     null=True, blank=True,
-                                     on_delete=models.CASCADE)
-    patches = models.ManyToManyField(Patch, through='SeriesPatch',
-                                     related_name='series')
+    cover_letter = models.OneToOneField(CoverLetter, related_name='series',
+                                        null=True,
+                                        on_delete=models.CASCADE)
 
     # metadata
     name = models.CharField(max_length=255, blank=True, null=True,
@@ -684,9 +691,8 @@ def add_cover_letter(self, cover):
             self.name = self._format_name(cover)
         else:
             try:
-                name = SeriesPatch.objects.get(series=self,
-                                               number=1).patch.name
-            except SeriesPatch.DoesNotExist:
+                name = Patch.objects.get(series=self, number=1).name
+            except Patch.DoesNotExist:
                 name = None
 
             if self.name == name:
@@ -696,20 +702,16 @@ def add_cover_letter(self, cover):
 
     def add_patch(self, patch, number):
         """Add a patch to the series."""
-        # see if the patch is already in this series
-        if SeriesPatch.objects.filter(series=self, patch=patch).count():
-            # TODO(stephenfin): We may wish to raise an exception here in the
-            # future
-            return
-
         # both user defined names and cover letter-based names take precedence
         if not self.name and number == 1:
             self.name = patch.name  # keep the prefixes for patch-based names
             self.save()
 
-        return SeriesPatch.objects.create(series=self,
-                                          patch=patch,
-                                          number=number)
+        patch.series = self
+        patch.number = number
+        patch.save()
+
+        return patch
 
     def get_absolute_url(self):
         # TODO(stephenfin): We really need a proper series view
@@ -727,26 +729,6 @@ class Meta:
         verbose_name_plural = 'Series'
 
 
-@python_2_unicode_compatible
-class SeriesPatch(models.Model):
-    """A patch in a series.
-
-    Patches can belong to many series. This allows for things like
-    auto-completion of partial series.
-    """
-    patch = models.ForeignKey(Patch, on_delete=models.CASCADE)
-    series = models.ForeignKey(Series, on_delete=models.CASCADE)
-    number = models.PositiveSmallIntegerField(
-        help_text='The number assigned to this patch in the series')
-
-    def __str__(self):
-        return self.patch.name
-
-    class Meta:
-        unique_together = [('series', 'patch'), ('series', 'number')]
-        ordering = ['number']
-
-
 @python_2_unicode_compatible
 class SeriesReference(models.Model):
     """A reference found in a series.