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! _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork