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

[Daniel Axtens]

Hi Stephen,

I think we're down to the final code-review details - hopefully no more
architectural or design issues!

Here goes:

> v4:
> - Convert 'SeriesRevision'-'Patch' relationship from one-to-many to
>   many-to-many

I missed this when it first went in. My understanding is that this is to
allow partial re-spins: that is:

v1 0/3 blah-cover
v1 1/3 blah 1
v1 2/3 blah 2
  v2 v2/3 blah-blah 2
v1 3/3 blah 3

Is that right?

So you'd have:
 SeriesRevision v1 blah-cover containing v1 1/3, v_1_ 2/3, v1 3/3
 SeriesRevision v2 blah-cover containing v1 1/3, v_2_ 2/3, v1 3/3

I think that makes sense, just checking if that's what you
intended. (And if so, do we have a test for it?)

  
>  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.

Apologies if this has been asked before: but this can't be fixed with a
select_related or a prefetch_related?

> +        """
> +        # 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()
>
You seem to have two copies of this function, one in CoverLetter, and
one in Patch. Is there any way to remove the duplication? I haven't dug
deep into the class structure here, but could it be pushed to a
super-class?

> -    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()
> -

It looks like you're just moving the save method here and below.
 - Is there any functionality change?
 - Is there any reason to move it?

> +    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()


> +    @property
> +    def actual_total(self):
> +        return self.patches.count()
I'm not a fan of the name actual_total here. Could we use
received_total? Otherwise it's not clear to me what the distinction
between 'actual' and non-'actual' is.

> +    @property
> +    def complete(self):
And here - completely_received?
> +        return self.total == self.actual_total


> +        # 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)
Have I got this right?
If there is no recorded name, use the name from the cover letter
Otherwise, if there _is_ a recorded name:
 - let name be the first patch name or None
 - if that is the name we have, set the name to be the name from the
   cover letter.

So, what I think you're doing is using the following priority list:
1) User provided name
2) Cover letter name
3) First patch name
And then you're trying to make sure stuff is updated - if you start with
a patch and then get a cover letter you upgrade to the cover letter name.

Is that what you're trying to do? Could you clarify the comment? (And
maybe restructure the code?)


Anyway, I think we're nearly there!