[PATCH v6 2/8] models: Add 'SeriesRevision' model

[Andrew Donnellan]

On 16/10/16 23:50, Stephen Finucane wrote:
> Add a series revision model. This model is expected to act like a
> collection for patches, similar to bundles but thread-orientated.
>
> Signed-off-by: Stephen Finucane <stephen at that.guru>

Looking pretty good! A few minor things below. I mostly won't repeat 
stuff that Daniel pointed out.

Otherwise, with Daniel's comments fixed:

Reviewed-by: Andrew Donnellan <andrew.donnellan at au1.ibm.com>

> diff --git a/patchwork/models.py b/patchwork/models.py
> index d0ef44d..49572ec 100644
> --- a/patchwork/models.py
> +++ b/patchwork/models.py
> @@ -292,7 +292,7 @@ class EmailMixin(models.Model):
>
>  @python_2_unicode_compatible
>  class Submission(EmailMixin, models.Model):
> -    # parent
> +    # parents

What's this for?

>
>      project = models.ForeignKey(Project)
>
> @@ -317,11 +317,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.
> +        """

I vote for keeping the warning, personally - it looks like the kind of 
thing which someone somewhere down the track might stuff up rather easily.

> +        # 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):
> +

Whitespace code churn

>      def is_editable(self, user):
>          if not user.is_authenticated():
>              return False
> @@ -439,6 +444,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()
> +

When versioning gets added later on, are you planning on calling the 
series group model "Series" as per v2? (FWIW after thinking about it for 
a bit I think I like the v1 naming better than the v2 naming, but I 
digress...)

If so, calling this method "series()" and having it return a 
SeriesRevision is a bit confusing.


>      class Meta:
>          verbose_name_plural = 'Patches'
>
> @@ -568,6 +600,134 @@ class Comment(EmailMixin, models.Model):
>          unique_together = [('msgid', 'submission')]
>
>
> + at 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.
> +        """
> +
> +        def _format_name(obj):
> +            return obj.name.split(']')[-1]
> +
> +        if self.cover_letter:
> +            # TODO(stephenfin): We may wish to raise an exception here in the
> +            # future
> +            return
> +
> +        self.cover_letter = cover
> +
> +        # don't override user-defined names
> +        if not self.name:
> +            self.name = _format_name(cover)
> +        # ...but give cover letter-based names precedence over patch-based
> +        # names
> +        else:
> +            try:
> +                name = SeriesRevisionPatch.objects.get(revision=self,
> +                                                       number=1).patch.name
> +            except SeriesRevisionPatch.DoesNotExist:
> +                name = None
> +
> +            if self.name == name:
> +                self.name = _format_name(cover)
> +
> +        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():
> +            # 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 SeriesRevisionPatch.objects.create(patch=patch,
> +                                                  revision=self,
> +                                                  number=number)
> +
> +    def __str__(self):
> +        return self.name if self.name else 'Untitled series #%d' % self.id

Just to add to Daniel's working-through of the precedence of names... 
"Untitled series X" should only be printed if there is no user defined 
name, no cover letter received yet, and the only patch that has been 
received is not numbered patch 1?

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

See earlier comment about "series" and SeriesRevision.


-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan at au1.ibm.com  IBM Australia Limited