| Message ID | 20180921175105.18000-1-stephen@that.guru |
|---|---|
| Headers | show |
| Series | Convert Series-Patch relationship to 1:N | expand |
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' > > > -@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
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' > > > > > > -@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
Stephen Finucane <stephen@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
On Tue, 2018-10-02 at 16:28 +1000, Daniel Axtens wrote: > > > 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 Any updates on this? I'd like to close this out so I can get back to Veronika's improved tagging series. Even if you don't have time, which is fair, I think this is good enough to merge. Stephen
Stephen Finucane <stephen@that.guru> writes: > On Tue, 2018-10-02 at 16:28 +1000, Daniel Axtens wrote: >> > > 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 > > Any updates on this? I'd like to close this out so I can get back to > Veronika's improved tagging series. Even if you don't have time, which > is fair, I think this is good enough to merge. I'm cloning the lkml public-inbox git repos and attempting to import them into patchwork (which requires a new helper script). That'll be over a million messages. Then I will time the migration and watch the memory usage. That's the one thing I want to check; apart from that I have no qualms with the series. If you want to do this yourself and report back I'm very happy to take your word for the results, otherwise it should be no more than a day or two given that I've now finished with the URL schema mangling. Regards, Daniel > > Stephen
On Mon, 2018-10-15 at 23:43 +1100, Daniel Axtens wrote: > Stephen Finucane <stephen@that.guru> writes: > > > On Tue, 2018-10-02 at 16:28 +1000, Daniel Axtens wrote: > > > > > 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 > > > > Any updates on this? I'd like to close this out so I can get back to > > Veronika's improved tagging series. Even if you don't have time, which > > is fair, I think this is good enough to merge. > > I'm cloning the lkml public-inbox git repos and attempting to import > them into patchwork (which requires a new helper script). That'll be > over a million messages. Then I will time the migration and watch the > memory usage. That's the one thing I want to check; apart from that I > have no qualms with the series. > > If you want to do this yourself and report back I'm very happy to take > your word for the results, otherwise it should be no more than a day or > two given that I've now finished with the URL schema mangling. If you're able to do it, I'm happy to wait. There's also the Bootstrap 4 thing but that's more subjective and not as crucial to have eyes on (it's supposed to look almost the same for now). I'll stick the URL patch on my review queue for sometime this week. Cheers, Stephen
Hi Stephen, >> I'm cloning the lkml public-inbox git repos and attempting to import >> them into patchwork (which requires a new helper script). That'll be >> over a million messages. Then I will time the migration and watch the >> memory usage. That's the one thing I want to check; apart from that I >> have no qualms with the series. >> >> If you want to do this yourself and report back I'm very happy to take >> your word for the results, otherwise it should be no more than a day or >> two given that I've now finished with the URL schema mangling. > > If you're able to do it, I'm happy to wait. There's also the Bootstrap > 4 thing but that's more subjective and not as crucial to have eyes on > (it's supposed to look almost the same for now). I have imported 2 of the 7 repos - each import takes 5-6 hours :( In total I now have 340k patches and I think roughly the same number of comments. On my laptop, the db queries to display the list page takes a terrifying 12 seconds, so I have no idea how kernel.org's lkml pw is so fast. (It has almost a million patches!) I then tried to apply the patches but they no longer apply to master. So I pulled out a branch where I had applied some before, applied the remainder, and rebased that. I then ran 'time python3 manage.py migrate'. The migration took 6 and a half minutes on my laptop and nothing was oomkilled. So even for people with significantly more patches, the migration should be doable. Tests pass. Tested-by: Daniel Axtens <dja@axtens.net> (you'll have to add that manually as the patch isn't showing up in pw) Regards, Daniel > > I'll stick the URL patch on my review queue for sometime this week. > > Cheers, > Stephen
On Thu, 2018-10-18 at 01:42 +1100, Daniel Axtens wrote: > Hi Stephen, > > > > I'm cloning the lkml public-inbox git repos and attempting to import > > > them into patchwork (which requires a new helper script). That'll be > > > over a million messages. Then I will time the migration and watch the > > > memory usage. That's the one thing I want to check; apart from that I > > > have no qualms with the series. > > > > > > If you want to do this yourself and report back I'm very happy to take > > > your word for the results, otherwise it should be no more than a day or > > > two given that I've now finished with the URL schema mangling. > > > > If you're able to do it, I'm happy to wait. There's also the Bootstrap > > 4 thing but that's more subjective and not as crucial to have eyes on > > (it's supposed to look almost the same for now). > > I have imported 2 of the 7 repos - each import takes 5-6 hours :( > In total I now have 340k patches and I think roughly the same number of > comments. On my laptop, the db queries to display the list page takes a > terrifying 12 seconds, so I have no idea how kernel.org's lkml pw is so > fast. (It has almost a million patches!) > > I then tried to apply the patches but they no longer apply to master. So > I pulled out a branch where I had applied some before, applied the > remainder, and rebased that. > > I then ran 'time python3 manage.py migrate'. > > The migration took 6 and a half minutes on my laptop and nothing was > oomkilled. So even for people with significantly more patches, the > migration should be doable. > > Tests pass. > > Tested-by: Daniel Axtens <dja@axtens.net> > (you'll have to add that manually as the patch isn't showing up in pw) Hurrah. I'll rebase this and apply it later today. Thanks for the in- depth review. Cheers, Stephen > Regards, > Daniel > > > > I'll stick the URL patch on my review queue for sometime this week. > > > > Cheers, > > Stephen
Daniel Axtens <dja@axtens.net> writes: > Hi Stephen, > >>> I'm cloning the lkml public-inbox git repos and attempting to import >>> them into patchwork (which requires a new helper script). That'll be >>> over a million messages. Then I will time the migration and watch the >>> memory usage. That's the one thing I want to check; apart from that I >>> have no qualms with the series. >>> >>> If you want to do this yourself and report back I'm very happy to take >>> your word for the results, otherwise it should be no more than a day or >>> two given that I've now finished with the URL schema mangling. >> >> If you're able to do it, I'm happy to wait. There's also the Bootstrap >> 4 thing but that's more subjective and not as crucial to have eyes on >> (it's supposed to look almost the same for now). > > I have imported 2 of the 7 repos - each import takes 5-6 hours :( > In total I now have 340k patches and I think roughly the same number of > comments. On my laptop, the db queries to display the list page takes a > terrifying 12 seconds, so I have no idea how kernel.org's lkml pw is so > fast. (It has almost a million patches!) > > I then tried to apply the patches but they no longer apply to master. So > I pulled out a branch where I had applied some before, applied the > remainder, and rebased that. > > I then ran 'time python3 manage.py migrate'. > > The migration took 6 and a half minutes on my laptop and nothing was > oomkilled. So even for people with significantly more patches, the > migration should be doable. > > Tests pass. > > Tested-by: Daniel Axtens <dja@axtens.net> > (you'll have to add that manually as the patch isn't showing up in pw) It seems I forgot to check this (d'oh!), but it now takes 112 db queries for me to load the patch list page. They're almost all of the form SELECT `patchwork_patch`.`submission_ptr_id`, `patchwork_patch`.`series_id` FROM `patchwork_patch` WHERE `patchwork_patch`.`submission_ptr_id` = NNNN Stephen, could you check this out please? Regards, Daniel > > Regards, > Daniel >> >> I'll stick the URL patch on my review queue for sometime this week. >> >> Cheers, >> Stephen
On Sat, 2018-10-27 at 11:26 +1100, Daniel Axtens wrote: > Daniel Axtens <dja@axtens.net> writes: > > > Hi Stephen, > > > > > > I'm cloning the lkml public-inbox git repos and attempting to import > > > > them into patchwork (which requires a new helper script). That'll be > > > > over a million messages. Then I will time the migration and watch the > > > > memory usage. That's the one thing I want to check; apart from that I > > > > have no qualms with the series. > > > > > > > > If you want to do this yourself and report back I'm very happy to take > > > > your word for the results, otherwise it should be no more than a day or > > > > two given that I've now finished with the URL schema mangling. > > > > > > If you're able to do it, I'm happy to wait. There's also the Bootstrap > > > 4 thing but that's more subjective and not as crucial to have eyes on > > > (it's supposed to look almost the same for now). > > > > I have imported 2 of the 7 repos - each import takes 5-6 hours :( > > In total I now have 340k patches and I think roughly the same number of > > comments. On my laptop, the db queries to display the list page takes a > > terrifying 12 seconds, so I have no idea how kernel.org's lkml pw is so > > fast. (It has almost a million patches!) > > > > I then tried to apply the patches but they no longer apply to master. So > > I pulled out a branch where I had applied some before, applied the > > remainder, and rebased that. > > > > I then ran 'time python3 manage.py migrate'. > > > > The migration took 6 and a half minutes on my laptop and nothing was > > oomkilled. So even for people with significantly more patches, the > > migration should be doable. > > > > Tests pass. > > > > Tested-by: Daniel Axtens <dja@axtens.net> > > (you'll have to add that manually as the patch isn't showing up in pw) > > It seems I forgot to check this (d'oh!), but it now takes 112 db queries > for me to load the patch list page. They're almost all of the form Damn, I missed that. Sorry. Going forward, it seems we can test this using assertNumQueries [1] and I'm going to work on introducing this for the REST API test cases at least. In any case, I've the issue at hand resolved now but forgot to CC you. Find it at [2]. Stephen [1] https://docs.djangoproject.com/en/2.1/topics/testing/tools/ [2] http://patchwork.ozlabs.org/patch/990331/ > SELECT > `patchwork_patch`.`submission_ptr_id`, `patchwork_patch`.`series_id` > FROM `patchwork_patch` WHERE `patchwork_patch`.`submission_ptr_id` = NNNN > > Stephen, could you check this out please? > > Regards, > Daniel > > > > > > Regards, > > Daniel > > > > > > I'll stick the URL patch on my review queue for sometime this week. > > > > > > Cheers, > > > Stephen