[v5,2/7] models: Add 'Series' model and related models

Add a series model. This model is intentionally very minimal to allow
as much dynaminism as possible. It is expected that patches will be
migrated between series as new data is provided.

Signed-off-by: Stephen Finucane <stephen@that.guru>
---
v5:
- Store cover letter name in SeriesRevision.name field
- Add warning about using the 'Patch.series' property, which causes a
  new query each time
v4:
- Convert 'SeriesRevision'-'Patch' relationship from one-to-many to
  many-to-many
- Remove 'Series' model, which is not used yet (revisioning is a
  minefield that's being addressed separately)
- Add 'name' field to 'SeriesRevision'
v2:
- Resolve issue with REST API (Andrew Donnellan)
- Use more meaningful names for untitled series (Andrew Donnellan)
v1:
- Rename 'SeriesGroup' to 'Series'
- Rename 'Series' to 'SeriesRevision'
---
 patchwork/admin.py                             |  67 ++++++++++-
 patchwork/migrations/0014_add_series_models.py |  67 +++++++++++
 patchwork/models.py                            | 158 +++++++++++++++++++++++--
 3 files changed, 274 insertions(+), 18 deletions(-)
 create mode 100644 patchwork/migrations/0014_add_series_models.py

On 10/09/2016 05:25 PM, Stephen Finucane wrote:
> Add a series model. This model is intentionally very minimal to allow
> as much dynaminism as possible. It is expected that patches will be
> migrated between series as new data is provided.
> 
> Signed-off-by: Stephen Finucane <stephen@that.guru>
> ---

> diff --git a/patchwork/models.py b/patchwork/models.py
> index 28e9861..4a55c1d 100644
> --- a/patchwork/models.py
> +++ b/patchwork/models.py
> @@ -293,7 +293,7 @@ class EmailMixin(models.Model):

>  class CoverLetter(Submission):
> -    pass
> +
> +    @property
> +    def series(self):
> +        """Get a simple series reference.
> +
> +        Return the last series revision that (ordered by date) that
> +        this submission is a member of.
> +
> +        .. warning::
> +          Be judicious in your use of this. For example, do not use it
> +          in list templates as doing so will result in a new query for
> +          each item in the list.
> +        """
> +        # NOTE(stephenfin): We don't use 'latest()' here, as this can raise an
> +        # exception if no series revisions exist
> +        return self.series_revisions.order_by('-date').first()

I see this method both here and in the Patch class. Couldn't we just put
it in the Submission class once and avoid the duplication?

On 2016-10-11 17:35, Andy Doan wrote:
> On 10/09/2016 05:25 PM, Stephen Finucane wrote:
>> Add a series model. This model is intentionally very minimal to allow
>> as much dynaminism as possible. It is expected that patches will be
>> migrated between series as new data is provided.
>> 
>> Signed-off-by: Stephen Finucane <stephen@that.guru>
>> ---
> 
>> diff --git a/patchwork/models.py b/patchwork/models.py
>> index 28e9861..4a55c1d 100644
>> --- a/patchwork/models.py
>> +++ b/patchwork/models.py
>> @@ -293,7 +293,7 @@ class EmailMixin(models.Model):
> 
>>  class CoverLetter(Submission):
>> -    pass
>> +
>> +    @property
>> +    def series(self):
>> +        """Get a simple series reference.
>> +
>> +        Return the last series revision that (ordered by date) that
>> +        this submission is a member of.
>> +
>> +        .. warning::
>> +          Be judicious in your use of this. For example, do not use 
>> it
>> +          in list templates as doing so will result in a new query 
>> for
>> +          each item in the list.
>> +        """
>> +        # NOTE(stephenfin): We don't use 'latest()' here, as this can 
>> raise an
>> +        # exception if no series revisions exist
>> +        return self.series_revisions.order_by('-date').first()
> 
> I see this method both here and in the Patch class. Couldn't we just 
> put
> it in the Submission class once and avoid the duplication?

Good catch. I don't know why I didn't do this but it looks like it would 
work. I'll fix if I respin, else submit a follow-up patch after the 
fact.

Hi Stephen,

This doesn't have my fix for reducing DB queries:
http://patchwork.ozlabs.org/patch/670292/

If you are respinning, please consider including it. Otherwise it'll
need to be a follow-up patch - preferably merged at the same time.

Regards,
Daniel

Stephen Finucane <stephen@that.guru> writes:

> Add a series model. This model is intentionally very minimal to allow
> as much dynaminism as possible. It is expected that patches will be
> migrated between series as new data is provided.
>
> Signed-off-by: Stephen Finucane <stephen@that.guru>
> ---
> v5:
> - Store cover letter name in SeriesRevision.name field
> - Add warning about using the 'Patch.series' property, which causes a
>   new query each time
> v4:
> - Convert 'SeriesRevision'-'Patch' relationship from one-to-many to
>   many-to-many
> - Remove 'Series' model, which is not used yet (revisioning is a
>   minefield that's being addressed separately)
> - Add 'name' field to 'SeriesRevision'
> v2:
> - Resolve issue with REST API (Andrew Donnellan)
> - Use more meaningful names for untitled series (Andrew Donnellan)
> v1:
> - Rename 'SeriesGroup' to 'Series'
> - Rename 'Series' to 'SeriesRevision'
> ---
>  patchwork/admin.py                             |  67 ++++++++++-
>  patchwork/migrations/0014_add_series_models.py |  67 +++++++++++
>  patchwork/models.py                            | 158 +++++++++++++++++++++++--
>  3 files changed, 274 insertions(+), 18 deletions(-)
>  create mode 100644 patchwork/migrations/0014_add_series_models.py
>
> diff --git a/patchwork/admin.py b/patchwork/admin.py
> index 85ffecf..49bd55b 100644
> --- a/patchwork/admin.py
> +++ b/patchwork/admin.py
> @@ -21,9 +21,20 @@ from __future__ import absolute_import
>  
>  from django.contrib import admin
>  
> -from patchwork.models import (Project, Person, UserProfile, State, Submission,
> -                              Patch, CoverLetter, Comment, Bundle, Tag, Check,
> -                              DelegationRule)
> +from patchwork.models import Bundle
> +from patchwork.models import Check
> +from patchwork.models import Comment
> +from patchwork.models import CoverLetter
> +from patchwork.models import DelegationRule
> +from patchwork.models import Patch
> +from patchwork.models import Person
> +from patchwork.models import Project
> +from patchwork.models import SeriesReference
> +from patchwork.models import SeriesRevision
> +from patchwork.models import State
> +from patchwork.models import Submission
> +from patchwork.models import Tag
> +from patchwork.models import UserProfile
>  
>  
>  class DelegationRuleInline(admin.TabularInline):
> @@ -68,13 +79,22 @@ class SubmissionAdmin(admin.ModelAdmin):
>      search_fields = ('name', 'submitter__name', 'submitter__email')
>      date_hierarchy = 'date'
>  admin.site.register(Submission, SubmissionAdmin)
> -admin.site.register(CoverLetter, SubmissionAdmin)
> +
> +
> +class CoverLetterAdmin(admin.ModelAdmin):
> +    list_display = ('name', 'submitter', 'project', 'date', 'series')
> +    list_filter = ('project', )
> +    readonly_fields = ('series', )
> +    search_fields = ('name', 'submitter__name', 'submitter__email')
> +    date_hierarchy = 'date'
> +admin.site.register(CoverLetter, CoverLetterAdmin)
>  
>  
>  class PatchAdmin(admin.ModelAdmin):
>      list_display = ('name', 'submitter', 'project', 'state', 'date',
> -                    'archived', 'is_pull_request')
> +                    'archived', 'is_pull_request', 'series')
>      list_filter = ('project', 'state', 'archived')
> +    readonly_fields = ('series', )
>      search_fields = ('name', 'submitter__name', 'submitter__email')
>      date_hierarchy = 'date'
>  
> @@ -94,6 +114,43 @@ class CommentAdmin(admin.ModelAdmin):
>  admin.site.register(Comment, CommentAdmin)
>  
>  
> +class PatchInline(admin.StackedInline):
> +    model = SeriesRevision.patches.through
> +    extra = 0
> +
> +
> +class SeriesRevisionAdmin(admin.ModelAdmin):
> +    list_display = ('name', 'date', 'submitter', 'version', 'total',
> +                    'actual_total', 'complete')
> +    readonly_fields = ('actual_total', 'complete')
> +    search_fields = ('submitter_name', 'submitter_email')
> +    exclude = ('patches', )
> +    inlines = (PatchInline, )
> +
> +    def complete(self, series):
> +        return series.complete
> +    complete.boolean = True
> +admin.site.register(SeriesRevision, SeriesRevisionAdmin)
> +
> +
> +class SeriesRevisionInline(admin.StackedInline):
> +    model = SeriesRevision
> +    readonly_fields = ('date', 'submitter', 'version', 'total',
> +                       'actual_total', 'complete')
> +    ordering = ('-date', )
> +    show_change_link = True
> +    extra = 0
> +
> +    def complete(self, series):
> +        return series.complete
> +    complete.boolean = True
> +
> +
> +class SeriesReferenceAdmin(admin.ModelAdmin):
> +    model = SeriesReference
> +admin.site.register(SeriesReference, SeriesReferenceAdmin)
> +
> +
>  class CheckAdmin(admin.ModelAdmin):
>      list_display = ('patch', 'user', 'state', 'target_url',
>                      'description', 'context')
> diff --git a/patchwork/migrations/0014_add_series_models.py b/patchwork/migrations/0014_add_series_models.py
> new file mode 100644
> index 0000000..8d0fffa
> --- /dev/null
> +++ b/patchwork/migrations/0014_add_series_models.py
> @@ -0,0 +1,67 @@
> +# -*- 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', '0013_slug_check_context'),
> +    ]
> +
> +    operations = [
> +        migrations.CreateModel(
> +            name='SeriesReference',
> +            fields=[
> +                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
> +                ('msgid', models.CharField(max_length=255, unique=True)),
> +            ],
> +        ),
> +        migrations.CreateModel(
> +            name='SeriesRevision',
> +            fields=[
> +                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
> +                ('name', models.CharField(blank=True, help_text=b'An optional name to associate with the series, e.g. "John\'s PCI series".', max_length=255, null=True)),
> +                ('date', models.DateTimeField()),
> +                ('version', models.IntegerField(default=1, help_text=b'Version of series revision as indicated by the subject prefix(es)')),
> +                ('total', models.IntegerField(help_text=b'Number of patches in series as indicated by the subject prefix(es)')),
> +                ('cover_letter', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='series_revisions', to='patchwork.CoverLetter')),
> +            ],
> +            options={
> +                'ordering': ('date',),
> +            },
> +        ),
> +        migrations.CreateModel(
> +            name='SeriesRevisionPatch',
> +            fields=[
> +                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
> +                ('number', models.PositiveSmallIntegerField(help_text=b'The number assigned to this patch in the series revision')),
> +                ('patch', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='patchwork.Patch')),
> +                ('revision', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='patchwork.SeriesRevision')),
> +            ],
> +            options={
> +                'ordering': ['number'],
> +            },
> +        ),
> +        migrations.AddField(
> +            model_name='seriesrevision',
> +            name='patches',
> +            field=models.ManyToManyField(related_name='series_revisions', related_query_name=b'series_revision', through='patchwork.SeriesRevisionPatch', to='patchwork.Patch'),
> +        ),
> +        migrations.AddField(
> +            model_name='seriesrevision',
> +            name='submitter',
> +            field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='patchwork.Person'),
> +        ),
> +        migrations.AddField(
> +            model_name='seriesreference',
> +            name='series',
> +            field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='references', related_query_name=b'reference', to='patchwork.SeriesRevision'),
> +        ),
> +        migrations.AlterUniqueTogether(
> +            name='seriesrevisionpatch',
> +            unique_together=set([('revision', 'number'), ('revision', 'patch')]),
> +        ),
> +    ]
> diff --git a/patchwork/models.py b/patchwork/models.py
> index 28e9861..4a55c1d 100644
> --- a/patchwork/models.py
> +++ b/patchwork/models.py
> @@ -293,7 +293,7 @@ class EmailMixin(models.Model):
>  
>  @python_2_unicode_compatible
>  class Submission(EmailMixin, models.Model):
> -    # parent
> +    # parents
>  
>      project = models.ForeignKey(Project)
>  
> @@ -318,11 +318,27 @@ class Submission(EmailMixin, models.Model):
>  
>  
>  class CoverLetter(Submission):
> -    pass
> +
> +    @property
> +    def series(self):
> +        """Get a simple series reference.
> +
> +        Return the last series revision that (ordered by date) that
> +        this submission is a member of.
> +
> +        .. warning::
> +          Be judicious in your use of this. For example, do not use it
> +          in list templates as doing so will result in a new query for
> +          each item in the list.
> +        """
> +        # NOTE(stephenfin): We don't use 'latest()' here, as this can raise an
> +        # exception if no series revisions exist
> +        return self.series_revisions.order_by('-date').first()
>  
>  
>  @python_2_unicode_compatible
>  class Patch(Submission):
> +
>      # patch metadata
>  
>      diff = models.TextField(null=True, blank=True)
> @@ -419,17 +435,6 @@ class Patch(Submission):
>          for tag in tags:
>              self._set_tag(tag, counter[tag])
>  
> -    def save(self, *args, **kwargs):
> -        if not hasattr(self, 'state') or not self.state:
> -            self.state = get_default_initial_patch_state()
> -
> -        if self.hash is None and self.diff is not None:
> -            self.hash = self.hash_diff(self.diff).hexdigest()
> -
> -        super(Patch, self).save(**kwargs)
> -
> -        self.refresh_tag_counts()
> -
>      def is_editable(self, user):
>          if not user.is_authenticated():
>              return False
> @@ -440,6 +445,22 @@ class Patch(Submission):
>          return self.project.is_editable(user)
>  
>      @property
> +    def series(self):
> +        """Get a simple series reference.
> +
> +        Return the last series revision that (ordered by date) that
> +        this submission is a member of.
> +
> +        .. warning::
> +          Be judicious in your use of this. For example, do not use it
> +          in list templates as doing so will result in a new query for
> +          each item in the list.
> +        """
> +        # NOTE(stephenfin): We don't use 'latest()' here, as this can raise an
> +        # exception if no series revisions exist
> +        return self.series_revisions.order_by('-date').first()
> +
> +    @property
>      def filename(self):
>          fname_re = re.compile(r'[^-_A-Za-z0-9\.]+')
>          str = fname_re.sub('-', self.name)
> @@ -546,6 +567,17 @@ class Patch(Submission):
>      def __str__(self):
>          return self.name
>  
> +    def save(self, *args, **kwargs):
> +        if not hasattr(self, 'state') or not self.state:
> +            self.state = get_default_initial_patch_state()
> +
> +        if self.hash is None and self.diff is not None:
> +            self.hash = self.hash_diff(self.diff).hexdigest()
> +
> +        super(Patch, self).save(**kwargs)
> +
> +        self.refresh_tag_counts()
> +
>      class Meta:
>          verbose_name_plural = 'Patches'
>  
> @@ -569,6 +601,106 @@ class Comment(EmailMixin, models.Model):
>          unique_together = [('msgid', 'submission')]
>  
>  
> +@python_2_unicode_compatible
> +class SeriesRevision(models.Model):
> +    """An individual revision of a series."""
> +
> +    # content
> +    cover_letter = models.ForeignKey(CoverLetter,
> +                                     related_name='series_revisions',
> +                                     null=True, blank=True)
> +    patches = models.ManyToManyField(Patch, through='SeriesRevisionPatch',
> +                                     related_name='series_revisions',
> +                                     related_query_name='series_revision')
> +
> +    # metadata
> +    name = models.CharField(max_length=255, blank=True, null=True,
> +                            help_text='An optional name to associate with '
> +                            'the series, e.g. "John\'s PCI series".')
> +    date = models.DateTimeField()
> +    submitter = models.ForeignKey(Person)
> +    version = models.IntegerField(default=1,
> +                                  help_text='Version of series revision as '
> +                                  'indicated by the subject prefix(es)')
> +    total = models.IntegerField(help_text='Number of patches in series as '
> +                                'indicated by the subject prefix(es)')
> +
> +    @property
> +    def actual_total(self):
> +        return self.patches.count()
> +
> +    @property
> +    def complete(self):
> +        return self.total == self.actual_total
> +
> +    def add_cover_letter(self, cover):
> +        """Add a cover letter to the series revision.
> +
> +        Helper method so we can use the same pattern to add both
> +        patches and cover letters.
> +        """
> +        self.cover_letter = cover
> +
> +        if not self.name:  # don't override user-defined names
> +            self.name = cover.name.split(']')[-1]
> +
> +        self.save()
> +
> +    def add_patch(self, patch, number):
> +        """Add a patch to the series revision."""
> +        # see if the patch is already in this series
> +        if SeriesRevisionPatch.objects.filter(revision=self,
> +                                              patch=patch).count():
> +            return
> +
> +        return SeriesRevisionPatch.objects.create(patch=patch,
> +                                                  revision=self,
> +                                                  number=number)
> +
> +    def __str__(self):
> +        return self.name if self.name else 'Untitled series #%d' % self.id
> +
> +    class Meta:
> +        ordering = ('date',)
> +
> +
> +@python_2_unicode_compatible
> +class SeriesRevisionPatch(models.Model):
> +    """A patch in a series revision.
> +
> +    Patches can belong to many series revisions. This allows for things
> +    like auto-completion of partial series.
> +    """
> +    patch = models.ForeignKey(Patch)
> +    revision = models.ForeignKey(SeriesRevision)
> +    number = models.PositiveSmallIntegerField(
> +        help_text='The number assigned to this patch in the series revision')
> +
> +    def __str__(self):
> +        return self.patch.name
> +
> +    class Meta:
> +        unique_together = [('revision', 'patch'), ('revision', 'number')]
> +        ordering = ['number']
> +
> +
> +@python_2_unicode_compatible
> +class SeriesReference(models.Model):
> +    """A reference found in a series.
> +
> +    Message IDs should be created for all patches in a series,
> +    including those of patches that have not yet been received. This is
> +    required to handle the case whereby one or more patches are
> +    received before the cover letter.
> +    """
> +    series = models.ForeignKey(SeriesRevision, related_name='references',
> +                               related_query_name='reference')
> +    msgid = models.CharField(max_length=255, unique=True)
> +
> +    def __str__(self):
> +        return self.msgid
> +
> +
>  class Bundle(models.Model):
>      owner = models.ForeignKey(User)
>      project = models.ForeignKey(Project)
> -- 
> 2.7.4
>
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork

On 2016-10-12 23:26, Daniel Axtens wrote:
> Hi Stephen,
> 
> This doesn't have my fix for reducing DB queries:
> http://patchwork.ozlabs.org/patch/670292/

Oh, is it needed here? We're not modifying the view code quite yet so I 
didn't think it was. I'll test this evening. I do include a variant of 
it in patch 6 [1], which is different thanks to the move from a 
one-to-many to a many-to-many patch-series relationship and the fact we 
now store cover letter name in SeriesRevision.name.

> If you are respinning, please consider including it. Otherwise it'll
> need to be a follow-up patch - preferably merged at the same time.

Yeah, I think the change in patch 6 covers it, but I can fix patch 2 to 
defer series if it's affecting performance for that commit.

Stephen

[1] 
https://lists.ozlabs.org/pipermail/patchwork/2016-October/003511.html

On 11 Oct 12:35, Andy Doan wrote:
> On 10/09/2016 05:25 PM, Stephen Finucane wrote:
> > Add a series model. This model is intentionally very minimal to allow
> > as much dynaminism as possible. It is expected that patches will be
> > migrated between series as new data is provided.
> > 
> > Signed-off-by: Stephen Finucane <stephen@that.guru>
> > ---
> 
> > diff --git a/patchwork/models.py b/patchwork/models.py
> > index 28e9861..4a55c1d 100644
> > --- a/patchwork/models.py
> > +++ b/patchwork/models.py
> > @@ -293,7 +293,7 @@ class EmailMixin(models.Model):
> 
> >  class CoverLetter(Submission):
> > -    pass
> > +
> > +    @property
> > +    def series(self):
> > +        """Get a simple series reference.
> > +
> > +        Return the last series revision that (ordered by date) that
> > +        this submission is a member of.
> > +
> > +        .. warning::
> > +          Be judicious in your use of this. For example, do not use it
> > +          in list templates as doing so will result in a new query for
> > +          each item in the list.
> > +        """
> > +        # NOTE(stephenfin): We don't use 'latest()' here, as this can raise an
> > +        # exception if no series revisions exist
> > +        return self.series_revisions.order_by('-date').first()
> 
> I see this method both here and in the Patch class. Couldn't we just put
> it in the Submission class once and avoid the duplication?

I remembered why I didn't do this: Submission doesn't have a
'series_revisions' field so I couldn't include it. Duplication is bad,
but exposing a non-working function seemed worse.

Stephen