Code review comment for lp:~allenap/launchpad/subscribe-to-tag-bug-151129-6

> Hi Gavin,
>
> This looks like a very nice refactoring branch. A few questions inline.
>
> > === modified file 'lib/lp/bugs/browser/bug.py'
> > --- lib/lp/bugs/browser/bug.py 2011-01-18 20:49:35 +0000
> > +++ lib/lp/bugs/browser/bug.py 2011-02-02 11:14:58 +0000
> > @@ -414,39 +414,28 @@
> > """Mix-in class to share methods between bug and portlet views."""
> >
> > @cachedproperty
> > + def subscription_info(self):
> > + return IBug(self.context).getSubscriptionInfo()
> > +
> > + @property
> > def direct_subscribers(self):
>
> Is the change here from @cachedproperty to @property for
> direct_subscribers intentional?

The properties of BugSubscriptionInfo are all cached, so there's no
need to cache here.

>
> > """Return the list of direct subscribers."""
> > - if IBug.providedBy(self.context):
> > - return set(self.context.getDirectSubscribers())
> > - elif IBugTask.providedBy(self.context):
> > - return set(self.context.bug.getDirectSubscribers())
> > - else:
> > - raise NotImplementedError(
> > - 'direct_subscribers is not provided by %s' % self)
> > + return self.subscription_info.direct_subscriptions.subscribers
>
> IOW, it still includes a few indirections, and
> direct_subscriptions.subscribers might be expensive to evaluate.
> Perhaps the code below will answer my question :)

It should be okay to evaluate because of the design of
BugSubscriptionInfo. I doubt that it'll be more expensive.

>
> >
> > - @cachedproperty
> > + @property
> > def duplicate_subscribers(self):
>
> This tells me that these are indeed intentional.

Yes, same as before.

>
> > === modified file 'lib/lp/bugs/model/bug.py'
> ...
> > @@ -851,8 +856,8 @@
> > """
> > # "Also notified" and duplicate subscribers are mutually
> > # exclusive, so return both lists.
> > - indirect_subscribers = (
> > - self.getAlsoNotifiedSubscribers(recipients, level) +
> > + indirect_subscribers = chain(
> > + self.getAlsoNotifiedSubscribers(recipients, level),
> > self.getSubscribersFromDuplicates(recipients, level))
>
> I didn't know about itertools.chain, nice :)

itertools is a great module, though I use chain() and groupby() far
more than the other functions in there.

>
> > @@ -887,37 +892,16 @@
> > See the comment in getDirectSubscribers for a description of the
> > recipients argument.
> > """
> > - if self.private:
> > - return []
> > -
> > - if level is None:
> > - notification_level = BugNotificationLevel.NOTHING
> > - else:
> > - notification_level = level
> > -
> > - dupe_details = dict(
> > - Store.of(self).find(
> > - (Person, Bug),
> > - BugSubscription.person == Person.id,
> > - BugSubscription.bug_id == Bug.id,
> > - BugSubscription.bug_notification_level >=
> notification_level,
> > - Bug.duplicateof == self.id))
> > -
> > - dupe_subscribers = set(dupe_details)
> > -
> > - # Direct and "also notified" subscribers take precedence over
> > - # subscribers from dupes. Note that we don't supply recipients
> > - # here because we are doing this to /remove/ subscribers.
> > - dupe_subscribers -= set(self.getDirectSubscribers())
> > - dupe_subscribers -= set(self.getAlsoNotifiedSubscribers())
> > + info = self.getSubscriptionInfo()
> >
> > if recipients is not None:
> > - for subscriber in dupe_subscribers:
> > + # Pre-load duplicate bugs.
> > + list(self.duplicates)
> > + for subscription in info.duplicate_only_subscriptions:
> > recipients.addDupeSubscriber(
> > - subscriber, dupe_details[subscriber])
> > + subscription.person, subscription.bug)
>
> The only question I've got here is how is the self.private condition
> preserved? I am sure it's going to be answered as I read through the
> patch, but I don't want to forget about it :)

See later.

>
> Other ("only") question I have is what happens with the level
> parameter? It still seems to be passed in, yet not really used
> anywhere. (you can guess why I care :)

I forgot to pass it through. Thanks for spotting that :)

This is tested in test_subscribers_from_dupes_uses_level, which
obviously doesn't work. I've fixed that (and verified that it breaks
when the level is not passed through).

>
> > @@ -2120,6 +2127,22 @@
> > Bug.duplicateof == self.bug)
> >
> > @cachedproperty
> > + @freeze(BugSubscriptionSet)
> > + def duplicate_only_subscriptions(self):
> > + """Subscripitions to duplicates of the bug.
> > +
> > + Excludes subscriptions for people who have a direct subscription or
> > + are also notified for another reason.
> > + """
> > + self.duplicate_subscriptions.subscribers # Pre-load subscribers.
> > + higher_precedence = (
> > + self.direct_subscriptions.subscribers.union(
> > + self.also_notified_subscribers))
> > + return (
> > + subscription for subscription in self.duplicate_subscriptions
> > + if subscription.person not in higher_precedence)
>
> And I actually expected this to be limited on the bug privacy, yet it
> isn't. Am I missing something?

This uses self.duplicate_subscriptions which is where the private flag
is considered.

>
>
> All else seems good, but there are a few important questions that need
> answering first.
>
> review needs-fixing
>
> Cheers,
> Danilo

Thank you for the review!