Merge lp:~gmb/launchpad/subscribers-timeout-bug-487015 into lp:launchpad

This branch fixes bug 487015 by adding methods to IBug that allow us to
check what kind of subscription (if any) a Person has to a bug using DB
queries rather than getting the list of subscribers and iterating over
it.

I've added three methods:

 - personIsDirectSubscriber(): returns True only if the Person has an
   explicit BugSubscription for the current bug.
 - personIsSubscribedToDuplicate(): returns True only if the Person has
   a BugSubscription to one of a bug's duplicates.
 - personIsAlsoNotifiedSubscriber(): returns True if the Person doesn't
   have a BugSubscription for a bug but will receive bugmail anyway
   (assignees, contacts and structural subscribers fall into this
   category).

Note that personIsAlsoNotifiedSubscriber() and personIsDirectSubscriber()
are mutually exclusive but personIsDirectSubscriber() and
personIsSubscribedToDuplicate() are not.

For personIsDirectSubscriber() and personIsSubscribedToDuplicate() I've
used Storm to create the necessary DB queries. However, I couldn't do
this for personIsAlsoNotifiedSubscriber() because "also notified
subscribers" come from a mish-mash of sources. I don't think this is a
problem (at least not right now) because there are usually less
"also notified" subscribers than there are subscribers from duplicates,
and the timeouts that raised this bug in the first place are due to the
number of duplicate subscribers and the amount of iteration we do over
that set.

I've updated BugViewMixin.subscription_class, which was where the
timeout problem identified in bug 487015 originated, to use the new
methods when determining what CSS class to return for a subscription.
Hopefully this will speed matters up somewhat.

I've also done a drive-by conversion of bug.txt to ReST.

NOTE: `make lint` shows a lot of "Operator is not preceded by a space"
errors, which look like crack to me and I'm not sure what to do about
them. Any ideas?

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/bugs/configure.zcml
  lib/lp/bugs/browser/bug.py
  lib/lp/bugs/doc/bug.txt
  lib/lp/bugs/interfaces/bug.py
  lib/lp/bugs/model/bug.py

== Pylint notices ==

lib/lp/bugs/browser/bug.py
    28: [F0401] Unable to import 'email.MIMEMultipart' (No module named MIMEMultipart)
    29: [F0401] Unable to import 'email.MIMEText' (No module named MIMEText)
    43: [F0401] Unable to import 'lazr.enum' (No module named enum)
    44: [F0401] Unable to import 'lazr.lifecycle.event' (No module named lifecycle)
    45: [F0401] Unable to import 'lazr.lifecycle.snapshot' (No module named lifecycle)
    46: [F0401] Unable to import 'lazr.restful.interfaces' (No module named restful)

lib/lp/bugs/interfaces/bug.py
    49: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)
    55: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)
    56: [F0401] Unable to import 'lazr.restful.interface' (No module named restful)
    466: [C0322, IBug.addAttachment] Operator not preceded by a space
    comment=Text(), filename=TextLine(), is_patch=Bool(),
    ^
    co...

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.

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.

> else:
> dup_class = 'dup-subscribed-false'
>
> - if subscribed_person in self.direct_subscribers:
> + if bug.personIsDirectSubscriber(subscribed_person):
> return 'subscribed-true %s' % dup_class
> else:
> return 'subscribed-false %s' % dup_class
>
> === modified file 'lib/lp/bugs/configure.zcml'
> --- lib/lp/bugs/configure.zcml 2009-11-20 04:21:24 +0000
> +++ lib/lp/bugs/configure.zcml 2009-11-25 11:02:35 +0000
> @@ -569,7 +569,10 @@
> is_complete
> who_made_private
> date_made_private
> - userCanView"/>
> + userCanView
> + personIsDirectSubscriber
> + personIsAlsoNotifiedSubscriber
> + personIsSubscribedToDuplicate"/>
> <require
> permission="launchpad.View"
> attributes="
>
> === 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.

>
> Bugs are created and retrieved via IBugSet.
>
> @@ -68,7 +70,8 @@
> >>> print result_set.count(...

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

Ah, thanks for the explanation.

> +
> + # Re-fetch the bug so that the fact that it's a duplicate definitely
> + # registers.
> >>> bug = getUtility(IBugSet).get(bug.id)

Although it saves little, out of curiosity I discovered that

  IStore(bug).flush()

also works.

Gavin.