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

[Stephen Finucane]

On 2016-10-25 09:27, Daniel Axtens wrote:
> 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?

Yup, that's the idea. We don't use this yet, but we will in the future. 
I figured it was best to set up the models to allow us do this in the 
future, rather than having to do a migration down the line. I'm treating 
versioning as a separate, bonus feature.

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

Yup, that's the intention but I don't have tests as the actual 
functionality isn't implemented yet. The best I could do is test that 
the ManyToMany field works, but I think it's safe to assume it does :)

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

Not that I could see. It's not an issue anyway, once we just don't call 
it from said views (which we don't). Maybe I could just remove the 
warning?

>> +        """
>> +        # 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?

Yeah, Andy suggested the same. We can't do this, however, as 
'series_revisions' isn't a valid field for 'Submission'. I figure 
duplicated code is better than wrong code :)

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

Nope - no functionality change. Looks like a hangover from an earlier 
revision. I'll remove this hunk.

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

Sounds good to me.

>> +    @property
>> +    def complete(self):
> 
> And here - completely_received?

Yup.

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

Yes, exactly.


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

Yup. We allow "upgrading" of the name, i.e. cover letter name -> 
user-provided name, and never "downgrading".

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

Yeah, I'll expand on the comments. Any suggestions for how the code 
could/should be reworked?

> Anyway, I think we're nearly there!

Hopefully :) I'll wait for reviews for the rest of the series before 
sending another revision, if that's ok by you.

Stephen