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()
>      0
>
> -== Bug creation events ==
> +Bug creation events
> +-------------------
>
>  IObjectCreatedEvent events are fired off when a bug is created. First
>  we will register a handler to observe the event.
> @@ -155,7 +158,8 @@
>      True
>
>
> -== Interface check ==
> +Interface check
> +---------------
>
>  It is guaranteed to implement the correct interface, too:
>
> @@ -166,7 +170,9 @@
>  (We grab the object directly from the database here to avoid it being
>  security proxied, which doesn't make sense to test here.)
>
> -== Searching for Bugs ==
> +
> +Searching for Bugs
> +------------------
>
>  To search for bugs matching specific criteria, use IBugSet.searchAsUser:
>
> @@ -198,7 +204,9 @@
>
>      >>> login(ANONYMOUS)
>
> -== Absolute URLs ==
> +
> +Absolute URLs
> +-------------
>
>  For things like bug notification emails, it's handy to be able to
>  include a URL to the bug inside the email.
> @@ -207,7 +215,9 @@
>      >>> print canonical_url(firefox_crashes)
>      http://.../bugs/6
>
> -== Bug Privacy ==
> +
> +Bug Privacy
> +-----------
>
>  A Bug has a "private" field. If Bug.private is False, the bug is
>  publicly visible. If Bug.private is True, only people who are directly
> @@ -503,7 +513,8 @@
>      []
>
>
> -== Prevent reporter from being subscribed to filed bugs ==
> +Prevent reporter from being subscribed to filed bugs
> +----------------------------------------------------
>
>  If necessary, subscriber_reporter may be specified when creating a bug,
>  to prevent the reporter from being subscribed to the bug. This is useful
> @@ -517,7 +528,8 @@
>      []
>
>
> -== Date Last Updated ==
> +Date Last Updated
> +-----------------
>
>  Malone tracks the last time a change was made to a
>  bug. IBug.date_last_updated stores the date when anything is changed or
> @@ -956,7 +968,8 @@
>      True
>
>
> -== Bug Completeness ==
> +Bug Completeness
> +----------------
>
>  A bug is considered "complete" iff all of its bugtasks are themselves
>  complete. The definition of completeness for a bugtask is that the bug
> @@ -987,7 +1000,8 @@
>      mozilla-firefox (Debian) True
>
>
> -== Bug Tasks ==
> +Bug Tasks
> +---------
>
>  A bug can be targeted to more than one product, distribution, or source
>  package. A BugTask is used to represent a target, which has its own
> @@ -1049,7 +1063,8 @@
>      True
>
>
> -== Bug Expiration ==
> +Bug Expiration
> +--------------
>
>  Incomplete bug reports may expire when they become inactive. Expiration
>  is only available to projects that use Launchpad to track bugs. There
> @@ -1128,7 +1143,8 @@
>  that can or cannot expire.
>
>
> -== Bug Comments ==
> +Bug Comments
> +------------
>
>  A bug comment is actually made up of a number of chunks. The
>  IBug.getMessageChunks() method allows you to retreive these chunks in a
> @@ -1166,7 +1182,8 @@
>      2 Strange bug with duplicate messages.        Bug #2 in Tomcat: "Blackhole Trash folder"
>
>
> -== Affected users ==
> +Affected users
> +--------------
>
>  Users can mark bugs as affecting or not affecting them. For each bug we
>  then keep a count of the number of users affected by it, as well as the
> @@ -1235,7 +1252,8 @@
>      [<Person at ...>]
>
>
> -== Getting the distinct set of Bugs for a set of BugTasks ==
> +Getting the distinct set of Bugs for a set of BugTasks
> +------------------------------------------------------
>
>  Sometimes we have a set of BugTasks for which we want to get only the
>  distinct set of bugs, i.e. there are several BugTasks in our set which
> @@ -1339,7 +1357,8 @@
>      New bug 0
>
>
> -== Links to HWDB submissions ==
> +Links to HWDB submissions
> +-------------------------
>
>  We can link a HWDB submission to a bug, indicating that the
>  submission contains information that could help developers
> @@ -1389,3 +1408,81 @@
>      >>> test_bug.unlinkHWSubmission(submission)
>      >>> print test_bug.getHWSubmissions().count()
>      0
> +
> +
> +Discovering subscription types
> +------------------------------
> +
> +It's possible to find out how a person is subscribed to a bug by calling
> +the bug's personIsDirectSubscriber(), personIsAlsoNotifiedSubscriber() or
> +personIsSubscribedToDuplicate() methods.
> +
> +If a person isn't subscribed to a bug, all of these methods will return
> +False.
> +
> +    >>> person = factory.makePerson()
> +    >>> bug = factory.makeBug()
> +
> +    >>> bug.personIsDirectSubscriber(person)
> +    False
> +
> +    >>> bug.personIsSubscribedToDuplicate(person)
> +    False
> +
> +    >>> bug.personIsAlsoNotifiedSubscriber(person)
> +    False
> +
> +If our person subscribes to the bug they'll show up as a direct
> +subscriber.
> +
> +    >>> subscription = bug.subscribe(person, person)
> +    >>> bug.personIsDirectSubscriber(person)
> +    True
> +
> +    >>> bug.personIsSubscribedToDuplicate(person)
> +    False
> +
> +    >>> bug.personIsAlsoNotifiedSubscriber(person)
> +    False
> +
> +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?

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

> +    >>> new_bug.personIsAlsoNotifiedSubscriber(person)
> +    True
> +
> +If the person subscribes directly to the bug,
> +personIsAlsoNotifiedSubscriber() will return False, since direct
> +subscriptions always override indirect ones.
> +
> +    >>> subscription = new_bug.subscribe(person, person)
> +    >>> new_bug.personIsAlsoNotifiedSubscriber(person)
> +    False
> +
> +    >>> new_bug.personIsDirectSubscriber(person)
> +    True
>
> === modified file 'lib/lp/bugs/interfaces/bug.py'
> --- lib/lp/bugs/interfaces/bug.py	2009-11-18 23:20:49 +0000
> +++ lib/lp/bugs/interfaces/bug.py	2009-11-25 11:02:35 +0000
> @@ -945,6 +945,24 @@
>              return Bugs.
>          """
>
> +    def personIsDirectSubscriber(person):
> +        """Return True if the person is a direct subscriber to this `IBug`.
> +
> +        Otherwise, return False.
> +        """
> +
> +    def personIsAlsoNotifiedSubscriber(person):
> +        """Return True if the person is an indirect subscriber to this `IBug`.
> +
> +        Otherwise, return False.
> +        """
> +
> +    def personIsSubscribedToDuplicate(person):
> +        """Return True if the person subscribed to a duplicate of this `IBug`.
> +
> +        Otherwise, return False.
> +        """
> +
>
>  class InvalidBugTargetType(Exception):
>      """Bug target's type is not valid."""
>
> === 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.

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

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

> +
> +        return subscriptions_from_dupes.count() > 0

Same as before with is_empty().

> +
>
>  class BugSet:
>      """See BugSet."""
>