]> git.ipfire.org Git - thirdparty/patchwork.git/commitdiff
models: Convert Series-Patch relationship to 1:N
authorStephen Finucane <stephen@that.guru>
Sat, 19 May 2018 02:42:18 +0000 (03:42 +0100)
committerStephen Finucane <stephen@that.guru>
Wed, 17 Oct 2018 17:37:54 +0000 (18:37 +0100)
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>
21 files changed:
lib/sql/grant-all.mysql.sql
lib/sql/grant-all.postgres.sql
patchwork/admin.py
patchwork/api/cover.py
patchwork/api/patch.py
patchwork/migrations/0031_add_patch_series_fields.py [new file with mode: 0644]
patchwork/migrations/0032_migrate_data_from_series_patch_to_patch.py [new file with mode: 0644]
patchwork/migrations/0033_remove_patch_series_model.py [new file with mode: 0644]
patchwork/models.py
patchwork/parser.py
patchwork/signals.py
patchwork/templates/patchwork/partials/download-buttons.html
patchwork/templates/patchwork/partials/patch-list.html
patchwork/templates/patchwork/submission.html
patchwork/tests/test_detail.py
patchwork/tests/test_events.py
patchwork/tests/test_series.py
patchwork/tests/utils.py
patchwork/views/cover.py
patchwork/views/patch.py
patchwork/views/utils.py

index ff962195cd74c8e1d3aeff518209ae28d495d165..02770777c12e4530e1c1602b03ebcb1d1120f42d 100644 (file)
@@ -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;
index 27f55c96d7688998d1b616db37802197992f8d44..8f500e96a46d2df21f02cc63508823c374fffcd5 100644 (file)
@@ -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";
 
index f12b33875b612e859e2d3e042f3d271287fe4669..9254167535c8c3bd78814ec727e400e1be5bcf6a 100644 (file)
@@ -114,7 +114,7 @@ admin.site.register(Comment, CommentAdmin)
 
 
 class PatchInline(admin.StackedInline):
-    model = Series.patches.through
+    model = Patch
     extra = 0
 
 
index 40f8c3515ef0f101019111c3003a078437b30f01..b9e36f23d009422b4e0e8c4f96ab02dec37e3499 100644 (file)
@@ -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 @@ class CoverLetterListSerializer(BaseHyperlinkedModelSerializer):
         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')
index 92423cbf297f5d645052c9a488f488777f4d6069..6367dd5d9014f451a705a7d105593e320d459319 100644 (file)
@@ -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 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer):
                                   "'%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')
diff --git a/patchwork/migrations/0031_add_patch_series_fields.py b/patchwork/migrations/0031_add_patch_series_fields.py
new file mode 100644 (file)
index 0000000..bfe4385
--- /dev/null
@@ -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')]),
+        ),
+    ]
diff --git a/patchwork/migrations/0032_migrate_data_from_series_patch_to_patch.py b/patchwork/migrations/0032_migrate_data_from_series_patch_to_patch.py
new file mode 100644 (file)
index 0000000..78e8642
--- /dev/null
@@ -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;
+            """,
+        ),
+    ]
diff --git a/patchwork/migrations/0033_remove_patch_series_model.py b/patchwork/migrations/0033_remove_patch_series_model.py
new file mode 100644 (file)
index 0000000..a9ea2e2
--- /dev/null
@@ -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'),
+        ),
+    ]
index a043844d51f9990cb465c559a56f95ceba80f5a0..a483dc642ffb8ba1baf92ae3a11d0559907c35fb 100644 (file)
@@ -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 @@ class Patch(Submission):
     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 Comment(EmailMixin, models.Model):
 
 @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 @@ class Series(FilenameMixin, models.Model):
             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 @@ class Series(FilenameMixin, models.Model):
 
     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 Series(FilenameMixin, models.Model):
         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.
index 2ba1db74542cf7a744f30d248f05ab3b95c1a81d..bc9dae2f8b7ec70c5f4854ac5de2982cffa83919 100644 (file)
@@ -27,7 +27,6 @@ from patchwork.models import Person
 from patchwork.models import Project
 from patchwork.models import Series
 from patchwork.models import SeriesReference
-from patchwork.models import SeriesPatch
 from patchwork.models import State
 from patchwork.models import Submission
 
@@ -1034,8 +1033,7 @@ def parse_mail(mail, list_id=None):
         # - there is no existing series to assign this patch to, or
         # - there is an existing series, but it already has a patch with this
         #   number in it
-        if not series or (
-                SeriesPatch.objects.filter(series=series, number=x).count()):
+        if not series or Patch.objects.filter(series=series, number=x).count():
             series = Series(project=project,
                             date=date,
                             submitter=author,
@@ -1069,6 +1067,8 @@ def parse_mail(mail, list_id=None):
         # patch. Don't add unnumbered patches (for example diffs sent
         # in reply, or just messages with random refs/in-reply-tos)
         if series and x:
+            # TODO(stephenfin): Remove 'series' from the conditional as we will
+            # always have a series
             series.add_patch(patch, x)
 
         return patch
index b7b8e6f55cf6573518a0befce2cd757149339ef2..536b177e37cfae1a02d9d6a38e5b53b888dcc3f8 100644 (file)
@@ -15,7 +15,6 @@ from patchwork.models import Event
 from patchwork.models import Patch
 from patchwork.models import PatchChangeNotification
 from patchwork.models import Series
-from patchwork.models import SeriesPatch
 
 
 @receiver(pre_save, sender=Patch)
@@ -133,39 +132,46 @@ def create_patch_delegated_event(sender, instance, raw, **kwargs):
     create_event(instance, orig_patch.delegate, instance.delegate)
 
 
-@receiver(post_save, sender=SeriesPatch)
-def create_patch_completed_event(sender, instance, created, raw, **kwargs):
-    """Create patch completed event for patches with series."""
+@receiver(pre_save, sender=Patch)
+def create_patch_completed_event(sender, instance, raw, **kwargs):
 
-    def create_event(patch, series):
+    def create_event(patch):
         return Event.objects.create(
             category=Event.CATEGORY_PATCH_COMPLETED,
             project=patch.project,
             patch=patch,
-            series=series)
+            series=patch.series)
 
-    # don't trigger for items loaded from fixtures or existing items
-    if raw or not created:
+    # don't trigger for items loaded from fixtures, new items or items that
+    # (still) don't have a series
+    if raw or not instance.pk or not instance.series:
+        return
+
+    orig_patch = Patch.objects.get(pk=instance.pk)
+
+    # we don't currently allow users to change a series, though this might
+    # change in the future. However, we handle that here nonetheless
+    if orig_patch.series == instance.series:
         return
 
     # if dependencies not met, don't raise event. There's also no point raising
     # events for successors since they'll have the same issue
-    predecessors = SeriesPatch.objects.filter(
+    predecessors = Patch.objects.filter(
         series=instance.series, number__lt=instance.number)
     if predecessors.count() != instance.number - 1:
         return
 
-    create_event(instance.patch, instance.series)
+    create_event(instance)
 
     # if this satisfies dependencies for successor patch, raise events for
     # those
     count = instance.number + 1
-    for successor in SeriesPatch.objects.filter(
+    for successor in Patch.objects.order_by('number').filter(
             series=instance.series, number__gt=instance.number):
         if successor.number != count:
             break
 
-        create_event(successor.patch, successor.series)
+        create_event(successor)
         count += 1
 
 
@@ -204,15 +210,9 @@ def create_series_created_event(sender, instance, created, raw, **kwargs):
     create_event(instance)
 
 
-@receiver(post_save, sender=SeriesPatch)
+@receiver(post_save, sender=Patch)
 def create_series_completed_event(sender, instance, created, raw, **kwargs):
 
-    # NOTE(stephenfin): We subscribe to the SeriesPatch.post_save signal
-    # instead of Series.m2m_changed to minimize the amount of times this is
-    # fired. The m2m_changed signal doesn't support a 'changed' parameter,
-    # which we could use to quick skip the signal when a patch is merely
-    # updated instead of added to the series.
-
     # NOTE(stephenfin): It's actually possible for this event to be fired
     # multiple times for a given series. To trigger this case, you would need
     # to send an additional patch to already exisiting series. This pattern
@@ -229,5 +229,5 @@ def create_series_completed_event(sender, instance, created, raw, **kwargs):
     if raw or not created:
         return
 
-    if instance.series.received_all:
+    if instance.series and instance.series.received_all:
         create_event(instance.series)
index 32acf26b6ef7cfb39d162008012b856109cded1b..21933bd28bcb93d12101fbb69e3eb914c31df676 100644 (file)
    class="btn btn-default" role="button" title="Download cover mbox"
    >mbox</a>
   {% endif %}
-  {% if all_series|length == 1 %}
-  {% with all_series|first as series %}
-  <a href="{% url 'series-mbox' series_id=series.id %}"
+  {% if submission.series %}
+  <a href="{% url 'series-mbox' series_id=submission.series.id %}"
    class="btn btn-default" role="button"
    title="Download patch mbox with dependencies">series</a>
-  {% endwith %}
-  {% elif all_series|length > 1 %}
-  <button type="button" class="btn btn-default dropdown-toggle"
-   data-toggle="dropdown">
-   series <span class="caret"></span>
-  </button>
-  <ul class="dropdown-menu" role="menu">
-  {% for series in all_series %}
-   <li><a href="{% url 'series-mbox' series_id=series.id %}"
-    >{{ series }}</a></li>
-  {% endfor %}
-  </ul>
   {% endif %}
 </div>
index 2abb9ec2d019564b6bc05fa52adfb867a229a6b0..2d090d98d619e6eade96caed53f9636871ecaa6e 100644 (file)
@@ -194,13 +194,11 @@ $(document).ready(function() {
     </a>
    </td>
    <td>
-    {% with patch.series.all.0 as series %}
-     {% if series %}
-     <a href="?series={{series.id}}">
-      {{ series|truncatechars:100 }}
-     </a>
-     {% endif %}
-    {% endwith %}
+    {% if patch.series %}
+    <a href="?series={{patch.series.id}}">
+     {{ patch.series|truncatechars:100 }}
+    </a>
+    {% endif %}
    </td>
    <td class="text-nowrap">{{ patch|patch_tags }}</td>
    <td class="text-nowrap">{{ patch|patch_checks }}</td>
index eb5e3583823dab4bd52bd75d7ec80e2ef0da0dc1..b1ae5429191dca7c0114e8bb3d788063dc34c8dc 100644 (file)
@@ -64,25 +64,13 @@ function toggle_div(link_id, headers_id)
    </div>
   </td>
  </tr>
-{% if all_series %}
+{% if submission.series %}
  <tr>
   <th>Series</th>
   <td>
-   <div class="patchrelations">
-    <ul>
-     {% for series in all_series %}
-     <li>
-      {% if forloop.first %}
-       {{ series }}
-      {% else %}
-       <a href="{% url 'patch-list' project_id=project.linkname %}?series={{ series.id }}">
-       {{ series }}
-       </a>
-      {% endif %}
-     </li>
-    {% endfor %}
-    </ul>
-   </div>
+   <a href="{% url 'patch-list' project_id=project.linkname %}?series={{ submission.series.id }}">
+    {{ submission.series }}
+   </a>
   </td>
  </tr>
  <tr>
@@ -92,9 +80,8 @@ function toggle_div(link_id, headers_id)
       href="javascript:toggle_div('togglepatchrelations', 'patchrelations')"
    >show</a>
    <div id="patchrelations" class="patchrelations" style="display:none;">
-    {% for series in all_series %}
     <ul>
-    {% with series.cover_letter as cover %}
+    {% with submission.series.cover_letter as cover %}
      <li>
      {% if cover %}
       {% if cover == submission %}
@@ -107,7 +94,7 @@ function toggle_div(link_id, headers_id)
      {% endif %}
      </li>
     {% endwith %}
-    {% for sibling in series.patches.all %}
+    {% for sibling in submission.series.patches.all %}
      <li>
       {% if sibling == submission %}
        {{ sibling.name|default:"[no subject]"|truncatechars:100 }}
@@ -119,7 +106,6 @@ function toggle_div(link_id, headers_id)
      </li>
     {% endfor %}
     </ul>
-    {% endfor %}
    </div>
   </td>
  </tr>
index 9c44779a829394c8b15ccfbf9a4fe6ed845a1dda..4ca1c9cda2f99191c5d3c3d3ef22ab99984607f0 100644 (file)
@@ -9,7 +9,6 @@ from django.urls import reverse
 from patchwork.tests.utils import create_comment
 from patchwork.tests.utils import create_cover
 from patchwork.tests.utils import create_patch
-from patchwork.tests.utils import create_series
 
 
 class CoverLetterViewTest(TestCase):
@@ -35,21 +34,6 @@ class PatchViewTest(TestCase):
         response = self.client.get(requested_url)
         self.assertRedirects(response, redirect_url)
 
-    def test_series_dropdown(self):
-        patch = create_patch()
-        series = [create_series() for x in range(5)]
-
-        for series_ in series:
-            series_.add_patch(patch, 1)
-
-        response = self.client.get(
-            reverse('patch-detail', kwargs={'patch_id': patch.id}))
-
-        for series_ in series:
-            self.assertContains(
-                response,
-                reverse('series-mbox', kwargs={'series_id': series_.id}))
-
 
 class CommentRedirectTest(TestCase):
 
index b9952f64641f650870c0137cd01a129d1b560d77..bd4f9be1d388d2844e40c8f638e564bed0382183 100644 (file)
@@ -34,8 +34,8 @@ class PatchCreateTest(_BaseTestCase):
         """No series, so patch dependencies implicitly exist."""
         patch = utils.create_patch()
 
-        # This should raise both the CATEGORY_PATCH_CREATED and
-        # CATEGORY_PATCH_COMPLETED events as there are no specific dependencies
+        # This should raise the CATEGORY_PATCH_CREATED event only as there is
+        # no series
         events = _get_events(patch=patch)
         self.assertEqual(events.count(), 1)
         self.assertEqual(events[0].category, Event.CATEGORY_PATCH_CREATED)
@@ -57,6 +57,13 @@ class PatchCreateTest(_BaseTestCase):
         self.assertEventFields(events[0])
         self.assertEventFields(events[1])
 
+        # This shouldn't be affected by another update to the patch
+        series_patch.patch.commit_ref = 'aac76f0b0f8dd657ff07bb'
+        series_patch.patch.save()
+
+        events = _get_events(patch=series_patch.patch)
+        self.assertEqual(events.count(), 2)
+
     def test_patch_dependencies_out_of_order(self):
         series = utils.create_series()
         series_patch_3 = utils.create_series_patch(series=series, number=3)
index ff3412e6e8ff6308479f8691745d584ad3ba52de..f5e6b5b072202f0022e6172659ca17237afecb76 100644 (file)
@@ -73,7 +73,15 @@ class _BaseTestCase(TestCase):
 
             patches_ = patches[start_idx:end_idx]
             for patch in patches_:
-                self.assertEqual(patch.series.first(), series[idx])
+                self.assertEqual(patch.series, series[idx])
+
+                # TODO(stephenfin): Rework this function into two different
+                # functions - we're clearly not always testing patches here
+                if isinstance(patch, models.Patch):
+                    self.assertEqual(series[idx].patches.get(id=patch.id),
+                                     patch)
+                else:
+                    self.assertEqual(series[idx].cover_letter, patch)
 
             start_idx = end_idx
 
@@ -517,7 +525,7 @@ class SeriesTotalTest(_BaseTestCase):
         self.assertSerialized(patches, [1])
         self.assertSerialized(covers, [1])
 
-        series = patches[0].series.first()
+        series = patches[0].series
         self.assertFalse(series.received_all)
 
     def test_complete(self):
@@ -537,7 +545,7 @@ class SeriesTotalTest(_BaseTestCase):
         self.assertSerialized(covers, [1])
         self.assertSerialized(patches, [2])
 
-        series = patches[0].series.first()
+        series = patches[0].series
         self.assertTrue(series.received_all)
 
     def test_extra_patches(self):
@@ -558,7 +566,7 @@ class SeriesTotalTest(_BaseTestCase):
         self.assertSerialized(covers, [1])
         self.assertSerialized(patches, [3])
 
-        series = patches[0].series.first()
+        series = patches[0].series
         self.assertTrue(series.received_all)
 
 
@@ -639,13 +647,13 @@ class SeriesNameTestCase(TestCase):
 
         cover = self._parse_mail(mbox[0])
         cover_name = 'A sample series'
-        self.assertEqual(cover.series.first().name, cover_name)
+        self.assertEqual(cover.series.name, cover_name)
 
         self._parse_mail(mbox[1])
-        self.assertEqual(cover.series.first().name, cover_name)
+        self.assertEqual(cover.series.name, cover_name)
 
         self._parse_mail(mbox[2])
-        self.assertEqual(cover.series.first().name, cover_name)
+        self.assertEqual(cover.series.name, cover_name)
 
         mbox.close()
 
@@ -663,7 +671,7 @@ class SeriesNameTestCase(TestCase):
         mbox = self._get_mbox('base-no-cover-letter.mbox')
 
         patch = self._parse_mail(mbox[0])
-        series = patch.series.first()
+        series = patch.series
         self.assertEqual(series.name, patch.name)
 
         self._parse_mail(mbox[1])
@@ -687,13 +695,13 @@ class SeriesNameTestCase(TestCase):
         mbox = self._get_mbox('base-out-of-order.mbox')
 
         patch = self._parse_mail(mbox[0])
-        self.assertIsNone(patch.series.first().name)
+        self.assertIsNone(patch.series.name)
 
         patch = self._parse_mail(mbox[1])
-        self.assertEqual(patch.series.first().name, patch.name)
+        self.assertEqual(patch.series.name, patch.name)
 
         cover = self._parse_mail(mbox[2])
-        self.assertEqual(cover.series.first().name, 'A sample series')
+        self.assertEqual(cover.series.name, 'A sample series')
 
         mbox.close()
 
@@ -712,7 +720,7 @@ class SeriesNameTestCase(TestCase):
         """
         mbox = self._get_mbox('base-out-of-order.mbox')
 
-        series = self._parse_mail(mbox[0]).series.first()
+        series = self._parse_mail(mbox[0]).series
         self.assertIsNone(series.name)
 
         series_name = 'My custom series name'
index d280fd69abda9b97a4887dbc5412a3cba91c9c17..0c3bbff2ab5797e729bb9e5017255ebb38b74f52 100644 (file)
@@ -19,7 +19,6 @@ from patchwork.models import Patch
 from patchwork.models import Person
 from patchwork.models import Project
 from patchwork.models import Series
-from patchwork.models import SeriesPatch
 from patchwork.models import SeriesReference
 from patchwork.models import State
 from patchwork.tests import TEST_PATCH_DIR
@@ -228,17 +227,24 @@ def create_series(**kwargs):
 
 
 def create_series_patch(**kwargs):
-    """Create 'SeriesPatch' object."""
+    """Create 'Patch' object and associate with a series."""
+    # TODO(stephenfin): Remove this and all callers
     num = 1 if 'series' not in kwargs else kwargs['series'].patches.count() + 1
+    if 'number' in kwargs:
+        num = kwargs['number']
 
-    values = {
-        'series': create_series() if 'series' not in kwargs else None,
-        'number': num,
-        'patch': create_patch() if 'patch' not in kwargs else None,
-    }
-    values.update(**kwargs)
+    series = create_series() if 'series' not in kwargs else kwargs['series']
+    patch = create_patch() if 'patch' not in kwargs else kwargs['patch']
+
+    series.add_patch(patch, num)
+
+    class SeriesPatch(object):
+        """Simple wrapper to avoid needing to update all tests at once."""
+        def __init__(self, series, patch):
+            self.series = series
+            self.patch = patch
 
-    return SeriesPatch.objects.create(**values)
+    return SeriesPatch(series=series, patch=patch)
 
 
 def create_series_reference(**kwargs):
index cb8fd3cab64b25c5382dbc1affd80fb552735b60..08b633a11f784ccad5e56742656f8aa4ee1c723b 100644 (file)
@@ -36,7 +36,6 @@ def cover_detail(request, cover_id):
     comments = comments.only('submitter', 'date', 'id', 'content',
                              'submission')
     context['comments'] = comments
-    context['all_series'] = cover.series.all().order_by('-date')
 
     return render(request, 'patchwork/submission.html', context)
 
index 862dc83dfe79b2bb80dc8d28474fe76511e4f1ca..277b2816e31e2586d0098f21bfffee2f52201aed 100644 (file)
@@ -105,7 +105,6 @@ def patch_detail(request, patch_id):
                              'submission')
 
     context['comments'] = comments
-    context['all_series'] = patch.series.all().order_by('-date')
     context['checks'] = patch.check_set.all().select_related('user')
     context['submission'] = patch
     context['patchform'] = form
@@ -132,7 +131,7 @@ def patch_mbox(request, patch_id):
 
     response = HttpResponse(content_type='text/plain')
     if series_id:
-        if not patch.series.count():
+        if not patch.series:
             raise Http404('Patch does not have an associated series. This is '
                           'because the patch was processed with an older '
                           'version of Patchwork. It is not possible to '
index a2be2c88112547efd37bf7461760a30413deb207..3c5d2982093e150f390b702aea39cd346c5e533a 100644 (file)
@@ -18,7 +18,6 @@ from django.utils import six
 
 from patchwork.models import Comment
 from patchwork.models import Patch
-from patchwork.models import Series
 
 if settings.ENABLE_REST_API:
     from rest_framework.authtoken.models import Token
@@ -128,26 +127,22 @@ def series_patch_to_mbox(patch, series_id):
     Returns:
         A string for the mbox file.
     """
-    if series_id == '*':
-        series = patch.series.order_by('-date').first()
-    else:
+    if series_id != '*':
         try:
             series_id = int(series_id)
         except ValueError:
             raise Http404('Expected integer series value or *. Received: %r' %
                           series_id)
 
-        try:
-            series = patch.series.get(id=series_id)
-        except Series.DoesNotExist:
+        if patch.series.id != series_id:
             raise Http404('Patch does not belong to series %d' % series_id)
 
     mbox = []
 
     # get the series-ified patch
-    number = series.seriespatch_set.get(patch=patch).number
-    for dep in series.seriespatch_set.filter(number__lt=number):
-        mbox.append(patch_to_mbox(dep.patch))
+    for dep in patch.series.patches.filter(
+            number__lt=patch.number).order_by('number'):
+        mbox.append(patch_to_mbox(dep))
 
     mbox.append(patch_to_mbox(patch))
 
@@ -165,7 +160,7 @@ def series_to_mbox(series):
     """
     mbox = []
 
-    for dep in series.seriespatch_set.all():
+    for dep in series.patches.all().order_by('number'):
         mbox.append(patch_to_mbox(dep.patch))
 
     return '\n'.join(mbox)