Stephen Finucane <step...@that.guru> writes:
> 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.
Ah. D'oh. Silly me. Carry on.
>>
>> > + ),
>> > + ]
>> > 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.
Ok. Fair enough.
>>
>> > + 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.
>
Sure, I thought that might be the case, just thought it would be worth seeing.
>> 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.
This is still my plan.
Thanks,
Daniel
>>
>> 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'
>> >
>> >
>> > -@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)
>> >
>> >
>> > -@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)
>> > 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
_______________________________________________
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
