[PATCH v3 3/5] models: Convert Series-Patch relationship to 1:N

Daniel Axtens dja at axtens.net
Tue Oct 2 00:18:46 AEST 2018


Hi Stephen,

> +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);
> +            """,
I get these two statements, but

> +            """INSERT INTO patchwork_seriespatch
> +                  (patch_id, series_id, number)
> +                SELECT submission_ptr_id, series_alt_id, number
> +                FROM patchwork_patch;
> +            """,

I don't understand this. What is the goal of inserting stuff into
patchwork_seriespatch? Aren't we just about to delete it?

> +        ),
> +    ]
> diff --git a/patchwork/migrations/0033_remove_patch_series_model.py b/patchwork/migrations/0033_remove_patch_series_model.py
> new file mode 100644
> index 00000000..a9ea2e20
> --- /dev/null
> +++ b/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',
> +        ),
Are these 3 migrations required given that we're about to delete the
model? I would have thought Django would be smart enough to delete the
parts of the SeriesPatch model itself...?

> +        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'),

When you created series_alt, it was defined as
> +            field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='patchwork.Series'),
So it looks like you're addig related_name and related_query_name
here. Would there be any issue with:
 - moving the deletion of 'patches' from Series up to migration 1, then
 - defining series_alt with the full related_[query_]name in migration
    1, and
 - just having the rename here?

I haven't tried this, I'm just trying to get the migrations straight in
my head - both for my own satisfaction and in hopes that anyone who has
to apply them manually to a complicated setup will be able to get it right.

Apart from that I think this mostly looks good - I haven't properly
looked at the event handling code or the view changes yet but from a
quick skim they seem in order. I'll give the series a spin on my
instance next.

Regards,
Daniel

> +        ),
> +        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'),
> +        ),
> +    ]
> diff --git a/patchwork/models.py b/patchwork/models.py
> index a043844d..a483dc64 100644
> --- a/patchwork/models.py
> +++ b/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 @@ 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
>
I get a bit lost on who throws and who catches exceptions, but do we
need to be catching an integrity error here if I send out for example 2
patches with the same number? (I've done this in real life before by
mistake.)


>      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'
>  
>  
> - at 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.
> diff --git a/patchwork/parser.py b/patchwork/parser.py
> index 2ba1db74..bc9dae2f 100644
> --- a/patchwork/parser.py
> +++ b/patchwork/parser.py
> @@ -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
> diff --git a/patchwork/signals.py b/patchwork/signals.py
> index b7b8e6f5..536b177e 100644
> --- a/patchwork/signals.py
> +++ b/patchwork/signals.py
> @@ -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)
>  
>  
> - at receiver(post_save, sender=SeriesPatch)
> -def create_patch_completed_event(sender, instance, created, raw, **kwargs):
> -    """Create patch completed event for patches with series."""
> + at 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)
>  
>  
> - at receiver(post_save, sender=SeriesPatch)
> + at 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)
> diff --git a/patchwork/templates/patchwork/download_buttons.html b/patchwork/templates/patchwork/download_buttons.html
> index 32acf26b..21933bd2 100644
> --- a/patchwork/templates/patchwork/download_buttons.html
> +++ b/patchwork/templates/patchwork/download_buttons.html
> @@ -15,22 +15,9 @@
>     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>
> diff --git a/patchwork/templates/patchwork/patch-list.html b/patchwork/templates/patchwork/patch-list.html
> index 71c1ba92..9dcee1c8 100644
> --- a/patchwork/templates/patchwork/patch-list.html
> +++ b/patchwork/templates/patchwork/patch-list.html
> @@ -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>
> diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html
> index f03e1408..ceb88af3 100644
> --- a/patchwork/templates/patchwork/submission.html
> +++ b/patchwork/templates/patchwork/submission.html
> @@ -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>
> diff --git a/patchwork/tests/test_detail.py b/patchwork/tests/test_detail.py
> index 9c44779a..4ca1c9cd 100644
> --- a/patchwork/tests/test_detail.py
> +++ b/patchwork/tests/test_detail.py
> @@ -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):
>  
> diff --git a/patchwork/tests/test_events.py b/patchwork/tests/test_events.py
> index b9952f64..bd4f9be1 100644
> --- a/patchwork/tests/test_events.py
> +++ b/patchwork/tests/test_events.py
> @@ -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)
> diff --git a/patchwork/tests/test_series.py b/patchwork/tests/test_series.py
> index ff3412e6..f5e6b5b0 100644
> --- a/patchwork/tests/test_series.py
> +++ b/patchwork/tests/test_series.py
> @@ -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'
> diff --git a/patchwork/tests/utils.py b/patchwork/tests/utils.py
> index d280fd69..0c3bbff2 100644
> --- a/patchwork/tests/utils.py
> +++ b/patchwork/tests/utils.py
> @@ -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):
> diff --git a/patchwork/views/cover.py b/patchwork/views/cover.py
> index cb8fd3ca..08b633a1 100644
> --- a/patchwork/views/cover.py
> +++ b/patchwork/views/cover.py
> @@ -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)
>  
> diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py
> index 862dc83d..277b2816 100644
> --- a/patchwork/views/patch.py
> +++ b/patchwork/views/patch.py
> @@ -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 '
> diff --git a/patchwork/views/utils.py b/patchwork/views/utils.py
> index a2be2c88..3c5d2982 100644
> --- a/patchwork/views/utils.py
> +++ b/patchwork/views/utils.py
> @@ -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)
> -- 
> 2.17.1


More information about the Patchwork mailing list