On Wed, Nov 25, 2009 at 12:07:21PM -0000, Gavin Panella wrote:
> Review: Needs Information code
> Hi Graham,
>
> I have some comments on how to make two of the new methods a bit
> faster, but otherwise it all looks good.
>
> I'm still concerned that it's doing at least one query for every
> subscriber, rather than batching things up, but at least this branch
> should reduce the overhead a lot... actually, I've suddenly had a
> thought...
>
> BugViewMixin.duplicate_subscribers and direct_subscribers are cached
> properties, so the current implementation of subscription_class (if
> subscribed_person in self.xxx_subscribers) is not inefficient. I'm now
> worried that this branch may actually cause performance to degrade.
Ah, so I'd partially misunderstood what subscription_class does and also
managed to miss a step.
The problem isn't that subscription_class is inefficient per se. It
isn't - for large result sets. However, for a single call it's
massively, massively inefficient, and when the bug page is loaded that's
how many times it gets called - once. No more. It's called when the
portlet contents are set up, but that happens *after* the bug page has
rendered, via AJAX, so that's not a concern. But that one call is
bringing the bug page to its knees.
So, I've added a new @property, current_user_subscription_class, which
uses the new methods to work on the subscription class for that one
call, and made the old subscription_class method into
getSubscriptionClassForUser (which it should have been called in the
first place; one of the things that confused me was that it was a PEP8
method name).
> Gavin.
>
>
> > === modified file 'lib/lp/bugs/browser/bug.py'
> > --- lib/lp/bugs/browser/bug.py 2009-11-17 14:58:39 +0000
> > +++ lib/lp/bugs/browser/bug.py 2009-11-25 11:02:35 +0000
> > @@ -437,12 +437,15 @@
> >
> > For example, "subscribed-false dup-subscribed-true".
> > """
> > - if subscribed_person in self.duplicate_subscribers:
> > + bug = self.context
> > +
> > + if (bug.personIsSubscribedToDuplicate(subscribed_person) or
> > + bug.personIsAlsoNotifiedSubscriber(subscribed_person)):
> > dup_class = 'dup-subscribed-true'
>
> Now that we can distinguish between from-dupe and also-notified, there
> should probably be another CSS class to represent those users, even if
> it visually looks exactly the same as dup-subscribed-true. But don't
> do it - I meant it as an observation - unless you've got very itchy
> fingers and your keyboard has a scratching board attached.
>
Nooooooo. And YAGNI, for the moment anyway.
> > === modified file 'lib/lp/bugs/doc/bug.txt'
> > --- lib/lp/bugs/doc/bug.txt 2009-10-26 17:11:03 +0000
> > +++ lib/lp/bugs/doc/bug.txt 2009-11-25 11:02:35 +0000
> > @@ -1,10 +1,12 @@
> > -= Bugs in Malone =
> > +Bugs in Malone
> > +==============
> >
> > This document describes what a Bug is in Malone, and provides some (currently
> > rather incomplete) info on how to poke at bugs through the Component
> > Architecture.
> >
> > -== Working with Bugs ==
> > +Working with Bugs
> > +-----------------
>
> I didn't know we were definitely moving over to reST, but I assume you
> know better because I suspect I'm a bit behind on that front.
>
We started standardising on it a while back, I think, and I've been
doing drive-by changes as I go. See
https://dev.launchpad.net/TestsStyleGuide#Common%20Conventions.
> > +If the user subscribes to a duplicate of the bug,
> > +personIsSubscribedToDuplicate() will return True.
> > +
> > + >>> dupe = factory.makeBug()
> > + >>> subscription = dupe.subscribe(person, person)
> > +
> > + >>> dupe.duplicateof = bug
> > + >>> bug = getUtility(IBugSet).get(bug.id)
>
> Why do you get the bug again here?
>
Ah, because otherwise the fact that it's been duplicated gets
intermittently lost. Not sure why. I'll add a comment.
> > + >>> bug.personIsSubscribedToDuplicate(person)
> > + True
> > +
> > +personIsSubscribedToDuplicate() will return True regardless of
> > +the result of personIsDirectSubscriber(). personIsAlsoNotifiedSubscriber()
> > +will still return False.
> > +
> > + >>> bug.personIsDirectSubscriber(person)
> > + True
> > +
> > + >>> bug.personIsAlsoNotifiedSubscriber(person)
> > + False
> > +
> > +If the user is subscribed to the bug for a reason other than a direct
> > +BugSubscription or a subscription to a duplicate bug,
> > +personIsAlsoNotifiedSubscriber() will return True, for example if the
> > +user is the assignee for one of the bug's BugTask.
> > +
> > + >>> new_bug = factory.makeBug()
> > + >>> new_bug.bugtasks[0].transitionToAssignee(person)
>
> Just so you know, you can also use new_bug.default_bugtask here.
>
Nice, done.
> > === modified file 'lib/lp/bugs/model/bug.py'
> > --- lib/lp/bugs/model/bug.py 2009-10-26 17:00:08 +0000
> > +++ lib/lp/bugs/model/bug.py 2009-11-25 11:02:35 +0000
> > @@ -1389,6 +1389,40 @@
> > """See `IBug`."""
> > return getUtility(IHWSubmissionBugSet).submissionsForBug(self, user)
> >
> > + def personIsDirectSubscriber(self, person):
> > + """See `IBug`."""
> > + store = Store.of(self)
> > + subscriptions = store.find(
> > + BugSubscription,
> > + BugSubscription.bug == self,
> > + BugSubscription.person == person)
> > +
> > + return subscriptions.count() > 0
>
> I think not subscriptions.is_empty() would be more efficient here.
>
Done.
> > +
> > + def personIsAlsoNotifiedSubscriber(self, person):
> > + """See `IBug`."""
> > + # We have to use getAlsoNotifiedSubscribers() here and iterate
> > + # over what it returns because "also notified subscribers" is
> > + # actually a composite of bug contacts, structural subscribers
> > + # and assignees. As such, it's not possible to get them all with
> > + # one query.
> > + also_notified_subscribers = self.getAlsoNotifiedSubscribers()
>
> getAlsoNotifiedSubscribers() and getSubscribersForTargets() make me a
> little sick, so I can't fault you for staying well away.
>
Yeah.
> > +
> > + return person in also_notified_subscribers
> > +
> > + def personIsSubscribedToDuplicate(self, person):
> > + """See `IBug`."""
> > + store = Store.of(self)
> > + duplicates = store.find(Bug, Bug.duplicateof == self)
> > + duplicate_ids = [dupe.id for dupe in duplicates]
> > +
> > + subscriptions_from_dupes = store.find(
> > + BugSubscription,
> > + BugSubscription.bugID.is_in(duplicate_ids),
> > + BugSubscription.person == person)
>
> I think you could reduce this to one query, so you don't need to get
> the duplicates first:
>
> subscriptions_from_dupes = store.find(
> BugSubscription,
> Bug.duplicateof == self,
> BugSubscription.bugID == Bug.id,
> BugSubscription.person == person)
>
Sweet. Done.
> > +
> > + return subscriptions_from_dupes.count() > 0
>
> Same as before with is_empty().
>
Done.
Incremental diff:
=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py 2009-11-25 08:47:39 +0000
+++ lib/lp/bugs/browser/bug.py 2009-11-25 13:08:05 +0000
@@ -432,20 +432,32 @@
ids[sub.name] = 'subscriber-%s' % sub.id
return ids
- def subscription_class(self, subscribed_person):
+ def getSubscriptionClassForUser(self, subscribed_person):
"""Return a set of CSS class names based on subscription status.
For example, "subscribed-false dup-subscribed-true".
"""
+ if subscribed_person in self.duplicate_subscribers:
+ dup_class = 'dup-subscribed-true'
+ else:
+ dup_class = 'dup-subscribed-false'
+
+ if subscribed_person in self.direct_subscribers:
+ return 'subscribed-true %s' % dup_class
+ else:
+ return 'subscribed-false %s' % dup_class
+
+ @property
+ def current_user_subscription_class(self):
bug = self.context
- if (bug.personIsSubscribedToDuplicate(subscribed_person) or
- bug.personIsAlsoNotifiedSubscriber(subscribed_person)):
+ if (bug.personIsSubscribedToDuplicate(self.user) or
+ bug.personIsAlsoNotifiedSubscriber(self.user)):
dup_class = 'dup-subscribed-true'
else:
dup_class = 'dup-subscribed-false'
- if bug.personIsDirectSubscriber(subscribed_person):
+ if bug.personIsDirectSubscriber(self.user):
return 'subscribed-true %s' % dup_class
else:
return 'subscribed-false %s' % dup_class
=== modified file 'lib/lp/bugs/doc/bug.txt'
--- lib/lp/bugs/doc/bug.txt 2009-11-25 08:58:49 +0000
+++ lib/lp/bugs/doc/bug.txt 2009-11-25 13:15:50 +0000
@@ -1452,6 +1452,9 @@
>>> subscription = dupe.subscribe(person, person)
>>> dupe.duplicateof = bug
+
+ # Re-fetch the bug so that the fact that it's a duplicate definitely
+ # registers.
>>> bug = getUtility(IBugSet).get(bug.id)
>>> bug.personIsSubscribedToDuplicate(person)
True
@@ -1472,7 +1475,7 @@
user is the assignee for one of the bug's BugTask.
>>> new_bug = factory.makeBug()
- >>> new_bug.bugtasks[0].transitionToAssignee(person)
+ >>> new_bug.default_bugtask.transitionToAssignee(person)
>>> new_bug.personIsAlsoNotifiedSubscriber(person)
True
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2009-11-25 08:44:19 +0000
+++ lib/lp/bugs/model/bug.py 2009-11-25 13:20:01 +0000
@@ -1397,7 +1397,7 @@
BugSubscription.bug == self,
BugSubscription.person == person)
- return subscriptions.count() > 0
+ return not subscriptions.is_empty()
def personIsAlsoNotifiedSubscriber(self, person):
"""See `IBug`."""
@@ -1413,15 +1413,13 @@
def personIsSubscribedToDuplicate(self, person):
"""See `IBug`."""
store = Store.of(self)
- duplicates = store.find(Bug, Bug.duplicateof == self)
- duplicate_ids = [dupe.id for dupe in duplicates]
-
subscriptions_from_dupes = store.find(
BugSubscription,
- BugSubscription.bugID.is_in(duplicate_ids),
+ Bug.duplicateof == self,
+ BugSubscription.bugID == Bug.id,
BugSubscription.person == person)
- return subscriptions_from_dupes.count() > 0
+ return not subscriptions_from_dupes.is_empty()
class BugSet:
=== modified file 'lib/lp/bugs/templates/bug-portlet-subscribers-content.pt'
--- lib/lp/bugs/templates/bug-portlet-subscribers-content.pt 2009-11-05 19:01:12 +0000
+++ lib/lp/bugs/templates/bug-portlet-subscribers-content.pt 2009-11-25 12:50:32 +0000
@@ -35,7 +35,7 @@
tal:attributes="
title string:Unsubscribe ${subscription/person/fmt:displayname};
id string:unsubscribe-${subscription/css_name};
- class python: view.subscription_class(subscription.person)
+ class python: view.getSubscriptionClassForUser(subscription.person)
"
>
Subscribing...