[PATCH v3 3/5] models: Convert Series-Patch relationship to 1:N
Stephen Finucane
stephen at that.guru
Tue Oct 2 06:18:56 AEST 2018
On Tue, 2018-10-02 at 00:18 +1000, Daniel Axtens wrote:
> 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?
This is the reverse statement which allows us to undo the migration
(it's a second argument). Where possible, I try to include these as
they're very useful during testing.
>
> > + ),
> > + ]
> > 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...?
To be clear, this bit was autogenerated. From the looks of this and
previous migrations though, it seems Django requires all references to
be removed one by one. I don't really want to twiddle with this lest I
break something, heh.
>
> > + 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?
Hmm, I'm not sure. If I recall correctly, removing the attribute in one
migration would break references to this and therefore prevent us
applying the other two migrations. I'll try it though, to be sure.
> 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.)
In theory, yes, but I remove this whole thing later in the series. If I
need to respin though, I'll add the check to be safe.
Cheers,
Stephen
>
> > 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