Launchpad itself

Merge lp:~allenap/launchpad/bugnomination-timeout-bug-874250-preload-email into lp:launchpad

bugnomination-timeout-bug-874250-preload-email
Merge into devel

Proposed by Gavin Panella on 2012-01-03

Status:

Merged

Approved by:

Gavin Panella on 2012-01-05

Approved revision:

no longer in the source branch.

Merged at revision:

14638

Proposed branch:

lp:~allenap/launchpad/bugnomination-timeout-bug-874250-preload-email

Merge into:

lp:launchpad

Diff against target:

1541 lines (+706/-257)

9 files modified

lib/lp/bugs/configure.zcml (+13/-0)
lib/lp/bugs/doc/bugsubscription.txt (+4/-4)
lib/lp/bugs/interfaces/bug.py (+3/-2)
lib/lp/bugs/interfaces/bugtask.py (+1/-1)
lib/lp/bugs/model/bug.py (+180/-100)
lib/lp/bugs/model/structuralsubscription.py (+84/-41)
lib/lp/bugs/model/tests/test_bug.py (+82/-8)
lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py (+239/-99)
lib/lp/bugs/tests/test_structuralsubscription.py (+100/-2)

To merge this branch:

bzr merge lp:~allenap/launchpad/bugnomination-timeout-bug-874250-preload-email

Critical

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Jeroen T. Vermeulen (community)		2012-01-03	Approve on 2012-01-04
Review via email: mp+87397@code.launchpad.net

Commit message

Load preferredemail when calculating bug subscribers of any kind via BugSubscriptionInfo.

Description of the change

This branch is long, but is actually quite readable, I think :)

One big problem when updating bugs is sending notifications to
subscribers. It looks like preferred email addresses are being loaded
one by one (though I started this branch long enough ago that the
details have faded).

In getting email addresses preloaded I updated BugSubscriptionInfo,
fixed some issues with it, and gotten get_also_notified_subscribers()
using it.

I think BSI is now a fairly good and comprehensive foundation for any
bug subscription calculation. It may not do everything in the minimum
number of queries possible, but it's does everything in a constant
number of queries, and makes it easy for code that uses it to also do
so.

There are still methods in IBug (and probably elsewhere) that could be
refactored to use BSI more directly, but I'll leave that for another
time.

The changes:

- Updates BugSubscriptionInfo:

  - Adds all_direct_subscriptions, all_direct_subscribers,
    direct_subscribers, muted_subscribers, and structural_subscribers
    properties.

- Adds support for returning subscription info for only a single
bugtask as well as all bugtasks of the given bug.

- Adds forLevel() and forTask() methods.

- When loading Person records, load preferred email information too.

- Updates get_also_notified_subscribers():

- Use BugSubscriptionInfo more heavily.

- Adds tests around performance of this function when passed a
recipients argument. This was previously poor (potato potato).

- Updates structural subscriptions:

  Split get_structural_subscribers() into two functions -
  get_structural_subscribers() and get_structural_subscriptions() -
  which both back onto query_structural_subscriptions(), a new
  function. This supports the changes to BugSubscriptionInfo and also
  makes the implementation cleaner.

Revision history for this message

Gavin Panella (allenap) wrote on 2012-01-03:

Fwiw, passes in devel:

Tests started at approximately 2012-01-03 16:07:06.335715
Source: bzr+ssh://bazaar.launchpad.net/~allenap/launchpad/bugnomination-timeout-bug-874250-preload-email r14523
Target: bzr+ssh://bazaar.launchpad.net/~launchpad-pqm/launchpad/devel/ r14623

17326 tests run in 4:21:55.682084, 0 failures, 0 errors

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2012-01-04:

Download full text (22.9 KiB)

Hi Gavin,

This took me a while to review. By and large it looks nice, but I have a few cosmetic issues to rant on. Not all of them are vital, and some will seem more trouble than they're worth — unless they can become habits. It may seem petty of me; but I could give a more effective review without these things in the way.

=== modified file 'lib/lp/bugs/doc/bugsubscription.txt'
--- lib/lp/bugs/doc/bugsubscription.txt 2011-12-24 17:49:30 +0000
+++ lib/lp/bugs/doc/bugsubscription.txt 2012-01-03 17:52:00 +0000
@@ -99,7 +99,7 @@
> getAlsoNotifiedSubscribers() and getSubscribersFromDuplicates().
>
> >>> linux_source_bug.getAlsoNotifiedSubscribers()
> - [<Person at ...>]
> + (<Person at ...>,)
> >>> linux_source_bug.getSubscribersFromDuplicates()
> ()
>
@@ -109,7 +109,7 @@
> >>> from lp.bugs.model.bug import get_also_notified_subscribers
> >>> res = get_also_notified_subscribers(linux_source_bug.bugtasks[0])
> >>> res
> - [<Person at ...>]
> + (<Person at ...>,)

These two original tests were brittle and a bit obscure. They seemed to establish that (1) a list is returned and (2) the list contains one or more Persons. I suspect (1) is of no significance.

Your new tests establish that (1) a tuple is returned and (2) the tuple contains exactly one Person, as implied by the trailing comma. So you test more, but it's still brittle and still a bit obscure.

It's not worth spending too much time on any given instance of these problems, but it's good to have fixes ready when you come across them: listify so that it won't matter what kind of iterable you get, or print just the length, or use a “[foo] = give_me_a_list_or_tuple()” assignment. The big question of course is what the test means to prove and what is incidental. Stay close to the former and away from the latter.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2011-12-30 06:14:56 +0000
+++ lib/lp/bugs/model/bug.py 2012-01-03 17:52:00 +0000

@@ -948,9 +949,10 @@
> BugSubscription.bug_id == self.id).order_by(BugSubscription.id)
> return DecoratedResultSet(results, operator.itemgetter(1))
>
> - def getSubscriptionInfo(self, level=BugNotificationLevel.LIFECYCLE):
> + def getSubscriptionInfo(self, level=None):
> """See `IBug`."""
> - return BugSubscriptionInfo(self, level)
> + return BugSubscriptionInfo(
> + self, BugNotificationLevel.LIFECYCLE if level is None else level)

Found the "if/else" slightly hard to read here. Maybe I'm just not used to the syntax yet, but it made me double-check that nothing else goes on in this line. Please consider putting the if/else on a separate line.

@@ -1002,6 +1004,8 @@
> # the regular proxied object.
> return sorted(
> indirect_subscribers,
> + # XXX: GavinPanella 2011-12-12 bug=???: Should probably use
> + # person_sort_key.
> key=lambda x: removeSecurityProxy(x).displayname)

Don't forget to file that bug!

@@ -2389,48 +2392,44 @@

> - # Direct subscriptions always take precedence over indirect
> - # subscriptions.
> - direct_subscriber...

Hi Gavin,

This took me a while to review.  By and large it looks nice, but I have a few cosmetic issues to rant on.  Not all of them are vital, and some will seem more trouble than they're worth — unless they can become habits.  It may seem petty of me; but I could give a more effective review without these things in the way.

=== modified file 'lib/lp/bugs/doc/bugsubscription.txt'
--- lib/lp/bugs/doc/bugsubscription.txt	2011-12-24 17:49:30 +0000
+++ lib/lp/bugs/doc/bugsubscription.txt	2012-01-03 17:52:00 +0000
@@ -99,7 +99,7 @@
>  getAlsoNotifiedSubscribers() and getSubscribersFromDuplicates().
>  
>      >>> linux_source_bug.getAlsoNotifiedSubscribers()
> -    [<Person at ...>]
> +    (<Person at ...>,)
>      >>> linux_source_bug.getSubscribersFromDuplicates()
>      ()
>  
@@ -109,7 +109,7 @@
>      >>> from lp.bugs.model.bug import get_also_notified_subscribers
>      >>> res = get_also_notified_subscribers(linux_source_bug.bugtasks[0])
>      >>> res
> -    [<Person at ...>]
> +    (<Person at ...>,)
 
These two original tests were brittle and a bit obscure.  They seemed to establish that (1) a list is returned and (2) the list contains one or more Persons.  I suspect (1) is of no significance.

Your new tests establish that (1) a tuple is returned and (2) the tuple contains exactly one Person, as implied by the trailing comma.  So you test more, but it's still brittle and still a bit obscure.

It's not worth spending too much time on any given instance of these problems, but it's good to have fixes ready when you come across them: listify so that it won't matter what kind of iterable you get, or print just the length, or use a “[foo] = give_me_a_list_or_tuple()” assignment.  The big question of course is what the test means to prove and what is incidental.  Stay close to the former and away from the latter.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-12-30 06:14:56 +0000
+++ lib/lp/bugs/model/bug.py	2012-01-03 17:52:00 +0000

@@ -948,9 +949,10 @@
>              BugSubscription.bug_id == self.id).order_by(BugSubscription.id)
>          return DecoratedResultSet(results, operator.itemgetter(1))
>  
> -    def getSubscriptionInfo(self, level=BugNotificationLevel.LIFECYCLE):
> +    def getSubscriptionInfo(self, level=None):
>          """See `IBug`."""
> -        return BugSubscriptionInfo(self, level)
> +        return BugSubscriptionInfo(
> +            self, BugNotificationLevel.LIFECYCLE if level is None else level)

Found the "if/else" slightly hard to read here.  Maybe I'm just not used to the syntax yet, but it made me double-check that nothing else goes on in this line.  Please consider putting the if/else on a separate line.

@@ -1002,6 +1004,8 @@
>          # the regular proxied object.
 >         return sorted(
 >             indirect_subscribers,
> +            # XXX: GavinPanella 2011-12-12 bug=???: Should probably use
> +            # person_sort_key.
>              key=lambda x: removeSecurityProxy(x).displayname)

Don't forget to file that bug!

@@ -2389,48 +2392,44 @@

> -    # Direct subscriptions always take precedence over indirect
> -    # subscriptions.
> -    direct_subscribers = set(bug.getDirectSubscribers())
> -
> -    also_notified_subscribers = set()
> -
> -    for bugtask in bugtasks:
> -        if (bugtask.assignee and
> -            bugtask.assignee not in direct_subscribers):
> -            # We have an assignee that is not a direct subscriber.
> -            also_notified_subscribers.add(bugtask.assignee)
> -            if recipients is not None:
> +    # Subscribers to exclude.
> +    exclude_subscribers = frozenset().union(
> +        info.all_direct_subscribers, info.muted_subscribers)

That's much better.  Makes me wonder though why direct subscribers are excluded.

> +    # Get also notified subscribers at the given level for the given tasks.
> +    also_notified_subscribers = info.also_notified_subscribers

The comment confused me for a moment: the “also” looks like a badly placed part of the sentence.  Maybe hyphenate as “also-notified subscribers”?

>      # This structural subscribers code omits direct subscribers itself.
> -    also_notified_subscribers.update(
> -        get_structural_subscribers(
> -            bug_or_bugtask, recipients, level, direct_subscribers))
> +    # TODO: Pass the info object into get_structural_subscribers for
> +    # efficiency... or do the recipients stuff here.
> +    structural_subscribers = get_structural_subscribers(
> +        bug_or_bugtask, recipients, level, exclude_subscribers)
> +    assert also_notified_subscribers.issuperset(structural_subscribers)

Unfinished business?
 
Any idea how much efficiency the change would gain you?  If you know it's significant, I'd file a bug and make this a proper XXX.  If not, human intuition about performance being what it is, I'd leave out the comment but make a note to have a look during Q/A.

@@ -2573,13 +2573,49 @@
 
     def __init__(self, bug, level):
         self.bug = bug
+        self.bugtask = None  # Implies all.
         assert level is not None
         self.level = level
+        self.cache = {self.cache_key: self}

Wait — what is this cache?  What will it store?  How does it work?

+        # This is often used in event handlers, many of which block implicit
+        # flushes. However, the data needs to be in the database for the
+        # queries herein to give correct answers.
+        Store.of(bug).flush()

Ouch, ouch, ouch.  Well done figuring that out and even more so documenting it.

+    @property
+    def cache_key(self):
+        bugtask_id = None if self.bugtask is None else self.bugtask.id
+        return self.bug.id, bugtask_id, self.level

This could do with a docstring, especially since its usage is also undocumented.

Again I find the if/else hard to read.  Does “getattr(self.bugtask, 'id', None)” cause problems with Storm?  Since this is a generic problem, personally I'd move this out into a helper so I could write "get_id(self.bugtask)."  Keeps the distractions out of the logic.

> +    def forTask(self, bugtask):
> +        """Create a new `BugSubscriptionInfo` limited to `bugtask`.

Our usual naming scheme for methods is to start with a verb, although in this case (and with forLevel) I think it's clear enough that there's no problem in deviating from it.

> +    @property
> +    def all_direct_subscriptions(self):
> +        """The bug's direct subscriptions at all levels.
> +
> +        Excludes muted subscriptions.
> +        """
> +        return self.forLevel(
> +            BugNotificationLevel.LIFECYCLE).direct_subscriptions

This is a problem I often have with the word “all” in identifiers: it often comes with invalidating fineprint.

Which is only to be expected, when you think about how the word got into the identifier in the first place.  We'd be in right trouble if any collection needed the word “all” in the name to stop arbitrary items from falling out!  The “all” is usually there to say “there's some other item with a shorter name which actually had an additional restriction that we neglected to mention.”

Usually the proper solution is to go back to the other item and make the restriction explicit.  Sometimes “all” is an attractive quick fix, but it doesn't apply recursively.

So: how do direct_subscriptions and all_direct_subscriptions differ in purpose?  It's usually just a matter of finding the right term and sticking to it.  We often get to regret cases where we don't make the effort.

>      @cachedproperty
>      def duplicate_subscriptions_and_subscribers(self):
> -        """Subscriptions to duplicates of the bug."""
>          if self.bug.private:
>              return ((), ())
>          else:

That docstring was useful to me, assuming that it's correct: it tells me it's the bugs, not the subscriptions, that are duplicates.

>      @cachedproperty
>      def also_notified_subscribers(self):
> -        """All subscribers except direct and dupe subscribers."""
> +        """All subscribers except direct and dupe subscribers.
> +
> +        Excludes muted subscribers.
> +        """

So it's "all" subscribers, except direct ones, ones that are only for duplicates of this bug, and muted subscribers?  I guess that leaves the non-direct, non-muted, structured subscribers to this precise bug, or is there something else?

=== modified file 'lib/lp/bugs/model/structuralsubscription.py'
--- lib/lp/bugs/model/structuralsubscription.py	2011-12-30 06:14:56 +0000
+++ lib/lp/bugs/model/structuralsubscription.py	2012-01-03 17:52:00 +0000

@@ -588,54 +589,93 @@

> +def query_structural_subscriptions(
> +    what, bug, bugtasks, level, exclude=None):

We have a different formatting rule for function definitions than we do for invocations:

def foo(param1, param2,
               param3, param4):

Note: no line break after the opening parenthesis, and continuation lines aligned with the starting column of param1.  Can't say I'm delighted with the choice, but at least it gives us something consistent.

> +    """Query into structural subscriptions for a given bug.
> +
> +    :param what: The fields to fetch. Choose from `Person`,
> +        `StructuralSubscription`, `BugSubscriptionFilter`, or a combo.
> +    :param bug: An `IBug`
> +    :param bugtasks: An iterable of `IBugTask`.
> +    :param level: A level from `BugNotificationLevel`.

Will this level be matched exactly, or is it a boundary on a set of levels?

> +    :param exclude: `Person`s to exclude (e.g. direct subscribers).
> +    """
>      filter_id_query = (
>          _get_structural_subscription_filter_id_query(
> -            bug, bugtasks, level, direct_subscribers))
> +            bug, bugtasks, level, exclude))
>      if not filter_id_query:
>          return EmptyResultSet()

Just to check that this is deliberate: you're forcing an execution of filter_id_query here to check for matching rows, and if it finds any, you'll execute it again later.

Not saying it has to, but it _could_ be more efficient to listify the results here and use that in the bigger query.  It's not just that it saves a query; it also may give the planner slightly more reliable costing information for the big query.

> -    # This is here because of a circular import.
> -    from lp.registry.model.person import Person
> +    from lp.registry.model.person import Person  # Circular.

We normally put these at the top of the function, right below the docstring.

+def resolve_bug_and_bugtasks(bug_or_bugtask):
+    """Return a bug and a list of bugtasks given a bug or a bugtask.

Isn't "resolve" a bit of a reserved word in the domain of bug trackers?

+    :param bug_or_bugtask: An `IBug` or `IBugTask`.
+    :raises ValueError: If `bug_or_bugtask` does not provide `IBug` or
+        `IBugTask`.
+    """

Will that error be handled specially somewhere?  If so, it may be safer to define a custom exception class.  Makes the "except:" clause that much more specific.  Or if not, maybe this should be an assertion?

+    if IBug.providedBy(bug_or_bugtask):
+        return bug_or_bugtask, bug_or_bugtask.bugtasks
+    elif IBugTask.providedBy(bug_or_bugtask):
+        return bug_or_bugtask.bug, [bug_or_bugtask]
+    else:
+        raise ValueError(
+            "Expected bug or bugtask, got %r" % (bug_or_bugtask,))

What does tuple-izing bug_or_bugtask do here?

=== modified file 'lib/lp/bugs/model/tests/test_bug.py'
--- lib/lp/bugs/model/tests/test_bug.py	2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/model/tests/test_bug.py	2012-01-03 17:52:00 +0000

@@ -476,6 +478,75 @@
>          self.assertContentEqual(public_branches, linked_branches)
>          self.assertNotIn(private_branch, linked_branches)
>  
> +    def test_get_direct_subscribers_with_recipients_query_count(self):
> +        # getDirectSubscribers() uses a constant number of queries when given
> +        # a recipients argument regardless of the number of subscribers.

Bit confusing to spell the method's name differently in the name of the test method.  I believe our usual way of doing it is:

def test_getDirectSubscribers_with_recipients_query_count(self):

> +        bug = self.factory.makeBug()
> +
> +        def create_subscriber():
> +            subscriber = self.factory.makePerson()
> +            with person_logged_in(subscriber):
> +                bug.subscribe(subscriber, subscriber)

This looks very generic, and you're repeating it later.  Maybe make it a method?

> +        def get_subscribers():
> +            recipients = BugNotificationRecipients()
> +            bug.getDirectSubscribers(recipients=recipients)

This looks unsafe.  I see that record_two_runs flushes the database.  But imagine getDirectSubscribers returning a SELECT result set… is that guaranteed to be executed?  It might be safter to listify the result.

> +        recorder1, recorder2 = record_two_runs(
> +            get_subscribers, create_subscriber, 3)
> +        self.assertThat(
> +            recorder2, HasQueryCount(Equals(recorder1.count)))

By the way, I wasn't aware of record_two_runs.  Gives me a lot more confidence than our old attempts at testing exact query counts!

=== modified file 'lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py'
--- lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py	2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py	2012-01-03 17:52:00 +0000

@@ -142,16 +141,75 @@
 
> +    def test_forTask(self):
> +        # `forTask` returns a new `BugSubscriptionInfo` narrowed to the given
> +        # bugtask.
> +        info = self.getInfo()
> +        self.assertIs(None, info.bugtask)
> +        # If called with the current bugtask the same `BugSubscriptionInfo`
> +        # instance is returned.
> +        self.assertIs(info, info.forTask(info.bugtask))
> +        # If called with a different bugtask a new `BugSubscriptionInfo` is
> +        # created.
> +        bugtask = self.bug.default_bugtask
> +        info_for_task = info.forTask(bugtask)
> +        self.assertIs(bugtask, info_for_task.bugtask)
> +        self.assertIsNot(info, info_for_task)
> +        # The instances share a cache of `BugSubscriptionInfo` instances.
> +        expected_cache = {
> +            info.cache_key: info,
> +            info_for_task.cache_key: info_for_task,
> +            }
> +        self.assertEqual(expected_cache, info.cache)
> +        self.assertIs(info.cache, info_for_task.cache)
> +        # Calling `forTask` again looks in the cache first.
> +        self.assertIs(info, info_for_task.forTask(info.bugtask))
> +        self.assertIs(info_for_task, info.forTask(info_for_task.bugtask))
> +        # The level is the same.
> +        self.assertEqual(info.level, info_for_task.level)

This looks like multiple tests rolled into one, proving different facts and exercising different scenarios: “self.getInfo() returns an info without a bugtask,” “forTask for the current bugtask returns self,” “forTask for a bugtask other than the current one creates a new BugSubscriptionInfo,” “A new BugSubscriptionInfo created by forTask shares its cache with the original,” “A newly constructed BugSubscriptionInfo is in its own cache,” “A BugSubscriptionInfo created by forTask is in its own cache,” “forTask uses its cache.”

Unless getInfo is particularly expensive, I'd split this up more so that you exercise forTask only in one place per test.  It's also nice to see which properties break, or get fixed, or get implemented, during development.

And finally there's “A BugSubscriptionInfo returned from cache has the same level."  Imagine this property wasn't implemented, so that you just always got the default level.  Wouldn't this test still pass regardless?  This is one reason why in TDD you should watch the test fail before you make it pass.

> +    def test_forLevel(self):
> +        # `forLevel` returns a new `BugSubscriptionInfo` narrowed to the given
> +        # subscription level.
> +        info = self.getInfo()
> +        # If called with the current level the same `BugSubscriptionInfo`
> +        # instance is returned.
> +        self.assertIs(info, info.forLevel(info.level))
> +        # If called with a different level a new `BugSubscriptionInfo` is
> +        # created.
> +        level = BugNotificationLevel.METADATA
> +        info_for_level = info.forLevel(level)
> +        self.assertEqual(level, info_for_level.level)
> +        self.assertIsNot(info, info_for_level)

This does test the right thing.  A very small superficial note though: when you're contrasting a default value (in this case LIFECYCLE) to an explicit different value (in this case METADATA), better make both explicit in the test.  If you pass LIFECYCLE to self.getInfo(), it'll be clearer that info.level != METADATA in that first test.

> +    def test_muted(self):
> +        # The set of muted subscribers for the bug.
> +        subscribers, subscriptions = self._create_direct_subscriptions()
> +        sub1, sub2 = subscribers
> +        with person_logged_in(sub1):
> +            self.bug.mute(sub1, sub1)
> +        self.assertEqual(set([sub1]), self.getInfo().muted_subscribers)

“There's an app for that.” :-)

self.assertContentEqual([sub1], self.getInfo().muted_subscribers)

@@ -159,6 +217,17 @@
         found_subscriptions = self.getInfo().direct_subscriptions
         self.assertEqual(set([subscriptions[1]]), found_subscriptions)
 
+    def test_all_direct(self):
+        # The set of all direct subscribers, regardless of level.
+        subscribers, subscriptions = self._create_direct_subscriptions()
+        # Change the first subscription to be for comments only.
+        sub1, sub2 = subscriptions
+        with person_logged_in(sub1.person):
+            sub1.bug_notification_level = BugNotificationLevel.LIFECYCLE
+        info = self.getInfo(BugNotificationLevel.COMMENTS)
+        self.assertEqual(set((sub2,)), info.direct_subscriptions)
+        self.assertEqual(set((sub1, sub2)), info.all_direct_subscriptions)

Here too, I think self.assertContentEqual could come in handy.  "set((sub2,))" is not very easy on the eyes!

One reason for that on top of the obvious ones is: too many (parentheses, with multiple values), and (trailing commas after one value,) and set() for zero items, when what you mean is basically [a list].  A list is more consistent and more recognizable.

Lists are also more idiomatic if you just want an ordered set.  Think of tuples not as containers, but as juxtapositions of items with different roles.  In python it's okay to use them as immutable lists sometimes, e.g. if you really need to save memory; but when in doubt, optimize for clarity.

>      def test_all_pillar_owners_without_bug_supervisors(self):
>          # The set of owners of pillars for which no bug supervisor is
> -        # configured.
> +        # configured and which use Launchpad for bug tracking.
>          [bugtask] = self.bug.bugtasks
>          found_owners = (
>              self.getInfo().all_pillar_owners_without_bug_supervisors)
>          self.assertEqual(set(), found_owners)
> -        self.assertEqual((), found_owners.sorted)
>          # Clear the supervisor for the first bugtask's target.
>          with person_logged_in(bugtask.target.owner):
>              bugtask.target.setBugSupervisor(None, bugtask.owner)
> +            bugtask.pillar.official_malone = False
> +        # The pillar does not use Launchpad, so the collection is still empty.
> +        found_owners = (
> +            self.getInfo().all_pillar_owners_without_bug_supervisors)
> +        self.assertEqual(set(), found_owners)
> +        # When a pillar does use Launchpad the collection includes the
> +        # pillar's owner.
> +        with person_logged_in(bugtask.target.owner):
> +            bugtask.pillar.official_malone = True
>          found_owners = (
>              self.getInfo().all_pillar_owners_without_bug_supervisors)
>          self.assertEqual(set([bugtask.pillar.owner]), found_owners)
> -        self.assertEqual((bugtask.pillar.owner,), found_owners.sorted)
> -        # Add another bugtask with a bug supervisor.
> -        target2 = self.factory.makeProduct(bug_supervisor=None)
> +        # Add another bugtask for a pillar that uses Launchpad but without a
> +        # bug supervisor.
> +        target2 = self.factory.makeProduct(
> +            bug_supervisor=None, official_malone=True)
>          bugtask2 = self.factory.makeBugTask(bug=self.bug, target=target2)
>          found_owners = (
>              self.getInfo().all_pillar_owners_without_bug_supervisors)
>          self.assertEqual(
>              set([bugtask.pillar.owner, bugtask2.pillar.owner]),
>              found_owners)
> +        # Getting subscription info for just a specific bugtask will yield
> +        # owners for only the pillar associated with that bugtask.
> +        info_for_bugtask2 = self.getInfo().forTask(bugtask2)
>          self.assertEqual(
> -            (bugtask.pillar.owner, bugtask2.pillar.owner),
> -            found_owners.sorted)
> +            set([bugtask2.pillar.owner]),
> +            info_for_bugtask2.all_pillar_owners_without_bug_supervisors)
 
Don't do this!

You're inserting new scenarios into an ongoing test story.  That's the stuff maintenance nightmares are made of.  It makes the test hard to read, and data flows hard to track.

I'm perfectly willing to take it on faith that you're conscientiously not introducing any changes that reverberate through the rest of the test, and that no part of the test relies on anything that happened more than 2 lines or so before.  But the next person to work on this code won't know who wrote this or when.

Since they will have more urgent things on their mind than rewriting the test, they'll either read the whole thing for very little gain, or break it and then fix it up as an afterthought.  Neither is good for the project in the long run.

@@ -444,27 +538,28 @@
 
> -    def exercise_subscription_set(self, set_name):
> +    def exercise_subscription_set(self, set_name, counts=(1, 1, 0)):
>          # Looking up subscriptions takes a single query.
> -        with self.exactly_x_queries(1):
> +        with self.exactly_x_queries(counts[0]):

Could you make the meaning of "counts" explicit in some way?

=== modified file 'lib/lp/bugs/tests/test_structuralsubscription.py'
--- lib/lp/bugs/tests/test_structuralsubscription.py	2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/tests/test_structuralsubscription.py	2012-01-03 17:52:00 +0000

@@ -634,6 +639,99 @@
 
> +class TestGetStructuralSubscriptions(TestCaseWithFactory):
> +
> +    layer = DatabaseFunctionalLayer
>
> +    def make_product_with_bug(self):
> +        product = self.factory.makeProduct()
> +        bug = self.factory.makeBug(product=product)
> +        return product, bug
> +
> +    def test_get_structural_subscriptions_no_subscriptions(self):
> +        # If there are no subscriptions for any of the bug's targets then no
> +        # subscriptions will be returned by get_structural_subscriptions().
> +        product, bug = self.make_product_with_bug()
> +        subscriptions = get_structural_subscriptions(bug, None)
> +        self.assertIsInstance(subscriptions, RESULT_SETS)
> +        self.assertEqual([], list(subscriptions))

Much nicer tests here!  Why is that?

Jeroen

review: Approve

Revision history for this message

Gavin Panella (allenap) wrote on 2012-01-04:

Download full text (28.3 KiB)

> Hi Gavin,
>
> This took me a while to review. By and large it looks nice, but I
> have a few cosmetic issues to rant on. Not all of them are vital,
> and some will seem more trouble than they're worth — unless they can
> become habits. It may seem petty of me; but I could give a more
> effective review without these things in the way.

Thanks for going through this branch!

>
>
> === modified file 'lib/lp/bugs/doc/bugsubscription.txt'
> --- lib/lp/bugs/doc/bugsubscription.txt 2011-12-24 17:49:30 +0000
> +++ lib/lp/bugs/doc/bugsubscription.txt 2012-01-03 17:52:00 +0000
> @@ -99,7 +99,7 @@
> > getAlsoNotifiedSubscribers() and getSubscribersFromDuplicates().
> >
> > >>> linux_source_bug.getAlsoNotifiedSubscribers()
> > - [<Person at ...>]
> > + (<Person at ...>,)
> > >>> linux_source_bug.getSubscribersFromDuplicates()
> > ()
> >
> @@ -109,7 +109,7 @@
> > >>> from lp.bugs.model.bug import get_also_notified_subscribers
> > >>> res = get_also_notified_subscribers(linux_source_bug.bugtasks[0])
> > >>> res
> > - [<Person at ...>]
> > + (<Person at ...>,)
>
> These two original tests were brittle and a bit obscure. They
> seemed to establish that (1) a list is returned and (2) the list
> contains one or more Persons. I suspect (1) is of no significance.
>
> Your new tests establish that (1) a tuple is returned and (2) the
> tuple contains exactly one Person, as implied by the trailing comma.
> So you test more, but it's still brittle and still a bit obscure.
>
> It's not worth spending too much time on any given instance of these
> problems, but it's good to have fixes ready when you come across
> them: listify so that it won't matter what kind of iterable you get,
> or print just the length, or use a “[foo] =
> give_me_a_list_or_tuple()” assignment. The big question of course
> is what the test means to prove and what is incidental. Stay close
> to the former and away from the latter.

Good points, well put. I've amended those.

>
>
> === modified file 'lib/lp/bugs/model/bug.py'
> --- lib/lp/bugs/model/bug.py 2011-12-30 06:14:56 +0000
> +++ lib/lp/bugs/model/bug.py 2012-01-03 17:52:00 +0000
>
> @@ -948,9 +949,10 @@
> > BugSubscription.bug_id == self.id).order_by(BugSubscription.id)
> > return DecoratedResultSet(results, operator.itemgetter(1))
> >
> > - def getSubscriptionInfo(self, level=BugNotificationLevel.LIFECYCLE):
> > + def getSubscriptionInfo(self, level=None):
> > """See `IBug`."""
> > - return BugSubscriptionInfo(self, level)
> > + return BugSubscriptionInfo(
> > + self, BugNotificationLevel.LIFECYCLE if level is None else
> level)
>
> Found the "if/else" slightly hard to read here. Maybe I'm just not
> used to the syntax yet, but it made me double-check that nothing
> else goes on in this line. Please consider putting the if/else on a
> separate line.

Done.

> Hi Gavin,
> 
> This took me a while to review.  By and large it looks nice, but I
> have a few cosmetic issues to rant on.  Not all of them are vital,
> and some will seem more trouble than they're worth — unless they can
> become habits.  It may seem petty of me; but I could give a more
> effective review without these things in the way.

Thanks for going through this branch!

> 
> 
> === modified file 'lib/lp/bugs/doc/bugsubscription.txt'
> --- lib/lp/bugs/doc/bugsubscription.txt 2011-12-24 17:49:30 +0000
> +++ lib/lp/bugs/doc/bugsubscription.txt 2012-01-03 17:52:00 +0000
> @@ -99,7 +99,7 @@
> >  getAlsoNotifiedSubscribers() and getSubscribersFromDuplicates().
> >
> >      >>> linux_source_bug.getAlsoNotifiedSubscribers()
> > -    [<Person at ...>]
> > +    (<Person at ...>,)
> >      >>> linux_source_bug.getSubscribersFromDuplicates()
> >      ()
> >
> @@ -109,7 +109,7 @@
> >      >>> from lp.bugs.model.bug import get_also_notified_subscribers
> >      >>> res = get_also_notified_subscribers(linux_source_bug.bugtasks[0])
> >      >>> res
> > -    [<Person at ...>]
> > +    (<Person at ...>,)
> 
> These two original tests were brittle and a bit obscure.  They
> seemed to establish that (1) a list is returned and (2) the list
> contains one or more Persons.  I suspect (1) is of no significance.
> 
> Your new tests establish that (1) a tuple is returned and (2) the
> tuple contains exactly one Person, as implied by the trailing comma.
> So you test more, but it's still brittle and still a bit obscure.
> 
> It's not worth spending too much time on any given instance of these
> problems, but it's good to have fixes ready when you come across
> them: listify so that it won't matter what kind of iterable you get,
> or print just the length, or use a “[foo] =
> give_me_a_list_or_tuple()” assignment.  The big question of course
> is what the test means to prove and what is incidental.  Stay close
> to the former and away from the latter.

Good points, well put. I've amended those.

> 
> 
> === modified file 'lib/lp/bugs/model/bug.py'
> --- lib/lp/bugs/model/bug.py    2011-12-30 06:14:56 +0000
> +++ lib/lp/bugs/model/bug.py    2012-01-03 17:52:00 +0000
> 
> @@ -948,9 +949,10 @@
> >              BugSubscription.bug_id == self.id).order_by(BugSubscription.id)
> >          return DecoratedResultSet(results, operator.itemgetter(1))
> >
> > -    def getSubscriptionInfo(self, level=BugNotificationLevel.LIFECYCLE):
> > +    def getSubscriptionInfo(self, level=None):
> >          """See `IBug`."""
> > -        return BugSubscriptionInfo(self, level)
> > +        return BugSubscriptionInfo(
> > +            self, BugNotificationLevel.LIFECYCLE if level is None else
> level)
> 
> Found the "if/else" slightly hard to read here.  Maybe I'm just not
> used to the syntax yet, but it made me double-check that nothing
> else goes on in this line.  Please consider putting the if/else on a
> separate line.

Done.

> 
> 
> @@ -1002,6 +1004,8 @@
> >          # the regular proxied object.
>  >         return sorted(
>  >             indirect_subscribers,
> > +            # XXX: GavinPanella 2011-12-12 bug=???: Should probably use
> > +            # person_sort_key.
> >              key=lambda x: removeSecurityProxy(x).displayname)
> 
> Don't forget to file that bug!

Done, https://bugs.launchpad.net/launchpad/+bug/911752.

> 
> 
> @@ -2389,48 +2392,44 @@
> 
> > -    # Direct subscriptions always take precedence over indirect
> > -    # subscriptions.
> > -    direct_subscribers = set(bug.getDirectSubscribers())
> > -
> > -    also_notified_subscribers = set()
> > -
> > -    for bugtask in bugtasks:
> > -        if (bugtask.assignee and
> > -            bugtask.assignee not in direct_subscribers):
> > -            # We have an assignee that is not a direct subscriber.
> > -            also_notified_subscribers.add(bugtask.assignee)
> > -            if recipients is not None:
> > +    # Subscribers to exclude.
> > +    exclude_subscribers = frozenset().union(
> > +        info.all_direct_subscribers, info.muted_subscribers)
> 
> That's much better.  Makes me wonder though why direct subscribers
> are excluded.

It does seem unnecessary, but perhaps it's an optimization, to avoid
complex bug filter calculations for direct subscribers.

> 
> 
> > +    # Get also notified subscribers at the given level for the given tasks.
> > +    also_notified_subscribers = info.also_notified_subscribers
> 
> The comment confused me for a moment: the “also” looks like a badly
> placed part of the sentence.  Maybe hyphenate as “also-notified
> subscribers”?
> 
> 
> >      # This structural subscribers code omits direct subscribers itself.
> > -    also_notified_subscribers.update(
> > -        get_structural_subscribers(
> > -            bug_or_bugtask, recipients, level, direct_subscribers))
> > +    # TODO: Pass the info object into get_structural_subscribers for
> > +    # efficiency... or do the recipients stuff here.
> > +    structural_subscribers = get_structural_subscribers(
> > +        bug_or_bugtask, recipients, level, exclude_subscribers)
> > +    assert also_notified_subscribers.issuperset(structural_subscribers)
> 
> Unfinished business?
> 
> Any idea how much efficiency the change would gain you?  If you know
> it's significant, I'd file a bug and make this a proper XXX.  If
> not, human intuition about performance being what it is, I'd leave
> out the comment but make a note to have a look during Q/A.

The only code that uses get_structural_subscribers() with its
recipients argument is get_also_notified_subscribers(), so I'm tempted
to gut that part of it and put it in get_also_notified_subscribers()
in a follow-up branch. I'll think about it some more...

> 
> 
> @@ -2573,13 +2573,49 @@
> 
>      def __init__(self, bug, level):
>          self.bug = bug
> +        self.bugtask = None  # Implies all.
>          assert level is not None
>          self.level = level
> +        self.cache = {self.cache_key: self}
> 
> Wait — what is this cache?  What will it store?  How does it work?

It holds related BugSubscriptionInfo instances relating to the same
bug but with different levels and/or choice of bugtask. I've added a
comment to that effect.

> 
> 
> +        # This is often used in event handlers, many of which block implicit
> +        # flushes. However, the data needs to be in the database for the
> +        # queries herein to give correct answers.
> +        Store.of(bug).flush()
> 
> Ouch, ouch, ouch.  Well done figuring that out and even more so
> documenting it.
> 
> 
> +    @property
> +    def cache_key(self):
> +        bugtask_id = None if self.bugtask is None else self.bugtask.id
> +        return self.bug.id, bugtask_id, self.level
> 
> This could do with a docstring, especially since its usage is also
> undocumented.

Done.

> 
> Again I find the if/else hard to read.  Does “getattr(self.bugtask,
> 'id', None)” cause problems with Storm?  Since this is a generic
> problem, personally I'd move this out into a helper so I could write
> "get_id(self.bugtask)."  Keeps the distractions out of the logic.

Personally I think this is a good place to use a conditional
expression. I prefer to use getattr only when the name is not known
ahead of time, instead of for its default fallback behaviour. I think
catching AttributeError or LBYL are clearer when the name is known
ahead of time.

> 
> > +    def forTask(self, bugtask):
> > +        """Create a new `BugSubscriptionInfo` limited to `bugtask`.
> 
> Our usual naming scheme for methods is to start with a verb,
> although in this case (and with forLevel) I think it's clear enough
> that there's no problem in deviating from it.
> 
> 
> > +    @property
> > +    def all_direct_subscriptions(self):
> > +        """The bug's direct subscriptions at all levels.
> > +
> > +        Excludes muted subscriptions.
> > +        """
> > +        return self.forLevel(
> > +            BugNotificationLevel.LIFECYCLE).direct_subscriptions
> 
> This is a problem I often have with the word “all” in identifiers:
> it often comes with invalidating fineprint.
> 
> Which is only to be expected, when you think about how the word got
> into the identifier in the first place.  We'd be in right trouble if
> any collection needed the word “all” in the name to stop arbitrary
> items from falling out!  The “all” is usually there to say “there's
> some other item with a shorter name which actually had an additional
> restriction that we neglected to mention.”
> 
> Usually the proper solution is to go back to the other item and make
> the restriction explicit.  Sometimes “all” is an attractive quick
> fix, but it doesn't apply recursively.
> 
> So: how do direct_subscriptions and all_direct_subscriptions differ
> in purpose?  It's usually just a matter of finding the right term
> and sticking to it.  We often get to regret cases where we don't
> make the effort.

One possible solution:

s/all_direct_subscri(ptions|bers)/direct_subscri\1_at_all_levels/g

The unfortunate "Excludes muted subscriptions" around the place are as
a result of someone misunderstanding BugSubscriptionInfo and just
hacking at it until it worked. Instead the muted_subscribers property
should have been created earlier and used in conjunction with the
existing sets as they were. At first sight that would result in two
queries instead of one when calculating some sets, but that's not a
deal-breaker in itself, and efficiencies in the implementation could
be worked on later.

Update: This is more complicated than I had at first thought.
BugSubscriptionInfo.duplicate_subscriptions excludes subscribers where
they've muted the duplicate bug.

Instead we now have the situation where it's not possible to ask
BugSubscriptionInfo for the full lists of subscribers regardless of
muting. Composed sets are also trickier to formulate. Grr.

I feel a bug report coming on.

Argh, I've just noticed another potato hole. The efficiency gained by
querying duplicate_subscriptions and duplicate_subscribers in one go -
i.e. in duplicate_subscriptions_and_subscribers - will be outweighed
by the fact that the subscribers will not have anything preloaded, and
use of duplicate_subscriptions.subscribers property will result in
another query anyway. I need to fix this.

Update: I have fixed this.

> 
> 
> >      @cachedproperty
> >      def duplicate_subscriptions_and_subscribers(self):
> > -        """Subscriptions to duplicates of the bug."""
> >          if self.bug.private:
> >              return ((), ())
> >          else:
> 
> That docstring was useful to me, assuming that it's correct: it
> tells me it's the bugs, not the subscriptions, that are duplicates.

I don't think I meant to remove it. In any case, I'm going to nuke
this method.

Update: I have nuked it.

> 
> 
> >      @cachedproperty
> >      def also_notified_subscribers(self):
> > -        """All subscribers except direct and dupe subscribers."""
> > +        """All subscribers except direct and dupe subscribers.
> > +
> > +        Excludes muted subscribers.
> > +        """
> 
> So it's "all" subscribers, except direct ones, ones that are only
> for duplicates of this bug, and muted subscribers?  I guess that
> leaves the non- direct, non-muted, structured subscribers to this
> precise bug, or is there something else?

Sigh, fixed.

> 
> 
> === modified file 'lib/lp/bugs/model/structuralsubscription.py'
> --- lib/lp/bugs/model/structuralsubscription.py 2011-12-30 06:14:56 +0000
> +++ lib/lp/bugs/model/structuralsubscription.py 2012-01-03 17:52:00 +0000
> 
> @@ -588,54 +589,93 @@
> 
> > +def query_structural_subscriptions(
> > +    what, bug, bugtasks, level, exclude=None):
> 
> We have a different formatting rule for function definitions than we
> do for invocations:
> 
>  def foo(param1, param2,
>                param3, param4):
> 
> Note: no line break after the opening parenthesis, and continuation
> lines aligned with the starting column of param1.  Can't say I'm
> delighted with the choice, but at least it gives us something
> consistent.

I think this rule was devised by Cobol fan on a bad mescaline
trip. It's plain bad. I'd like to ignore it. Consider it a final act
of disobedience in the twilight of our time on Launchpad... at least
until we come back in a few months and wreak havoc once again.

> 
> 
> > +    """Query into structural subscriptions for a given bug.
> > +
> > +    :param what: The fields to fetch. Choose from `Person`,
> > +        `StructuralSubscription`, `BugSubscriptionFilter`, or a combo.
> > +    :param bug: An `IBug`
> > +    :param bugtasks: An iterable of `IBugTask`.
> > +    :param level: A level from `BugNotificationLevel`.
> 
> Will this level be matched exactly, or is it a boundary on a set of
> levels?

/me looks this up.

Filters below this level will be excluded.

Docstrings updated.

> 
> 
> > +    :param exclude: `Person`s to exclude (e.g. direct subscribers).
> > +    """
> >      filter_id_query = (
> >          _get_structural_subscription_filter_id_query(
> > -            bug, bugtasks, level, direct_subscribers))
> > +            bug, bugtasks, level, exclude))
> >      if not filter_id_query:
> >          return EmptyResultSet()
> 
> Just to check that this is deliberate: you're forcing an execution
> of filter_id_query here to check for matching rows, and if it finds
> any, you'll execute it again later.
> 
> Not saying it has to, but it _could_ be more efficient to listify
> the results here and use that in the bigger query.  It's not just
> that it saves a query; it also may give the planner slightly more
> reliable costing information for the big query.

Not my code <slopes shoulders>.

I'm going to leave this; too much else is going on in this branch.

> 
> 
> > -    # This is here because of a circular import.
> > -    from lp.registry.model.person import Person
> > +    from lp.registry.model.person import Person  # Circular.
> 
> We normally put these at the top of the function, right below the
> docstring.

We do? Fixed.

> 
> 
> +def resolve_bug_and_bugtasks(bug_or_bugtask):
> +    """Return a bug and a list of bugtasks given a bug or a bugtask.
> 
> Isn't "resolve" a bit of a reserved word in the domain of bug
> trackers?

Mmm, yes, it is. I've changed it to get_bug_and_bugtasks.

> 
> 
> +    :param bug_or_bugtask: An `IBug` or `IBugTask`.
> +    :raises ValueError: If `bug_or_bugtask` does not provide `IBug` or
> +        `IBugTask`.
> +    """
> 
> Will that error be handled specially somewhere?  If so, it may be
> safer to define a custom exception class.  Makes the "except:"
> clause that much more specific.  Or if not, maybe this should be an
> assertion?

It probably should be an assertion, but this was the logic that was
there before and I'd rather avoid too many drive-bys.

> 
> 
> +    if IBug.providedBy(bug_or_bugtask):
> +        return bug_or_bugtask, bug_or_bugtask.bugtasks
> +    elif IBugTask.providedBy(bug_or_bugtask):
> +        return bug_or_bugtask.bug, [bug_or_bugtask]
> +    else:
> +        raise ValueError(
> +            "Expected bug or bugtask, got %r" % (bug_or_bugtask,))
> 
> What does tuple-izing bug_or_bugtask do here?

Without the extra tuple, string formatting would crash if
bug_or_bugtask were a tuple with 0 or 2 or more elements.

> 
> 
> === modified file 'lib/lp/bugs/model/tests/test_bug.py'
> --- lib/lp/bugs/model/tests/test_bug.py 2012-01-01 02:58:52 +0000
> +++ lib/lp/bugs/model/tests/test_bug.py 2012-01-03 17:52:00 +0000
> 
> @@ -476,6 +478,75 @@
> >          self.assertContentEqual(public_branches, linked_branches)
> >          self.assertNotIn(private_branch, linked_branches)
> >
> > +    def test_get_direct_subscribers_with_recipients_query_count(self):
> > +        # getDirectSubscribers() uses a constant number of queries when
> given
> > +        # a recipients argument regardless of the number of subscribers.
> 
> Bit confusing to spell the method's name differently in the name of
> the test method.  I believe our usual way of doing it is:
> 
>     def test_getDirectSubscribers_with_recipients_query_count(self):

Oops, fixed.

> 
> 
> > +        bug = self.factory.makeBug()
> > +
> > +        def create_subscriber():
> > +            subscriber = self.factory.makePerson()
> > +            with person_logged_in(subscriber):
> > +                bug.subscribe(subscriber, subscriber)
> 
> This looks very generic, and you're repeating it later.  Maybe make
> it a method?

It's convenient because it closes over bug (if that's the correct
terminology). I need to pass a no-arg function to record_two_runs(),
so if I moved this to a method I would have to use a lambda or a
partial... or a local function. Even discounting that, it's such a
short function that I think it's clearer to write it twice.

> 
> 
> > +        def get_subscribers():
> > +            recipients = BugNotificationRecipients()
> > +            bug.getDirectSubscribers(recipients=recipients)
> 
> This looks unsafe.  I see that record_two_runs flushes the database.
> But imagine getDirectSubscribers returning a SELECT result set… is
> that guaranteed to be executed?  It might be safter to listify the
> result.

That's a fair point. I know that it executes the query, but it's worth
ensuring that here. Fixed.

> 
> 
> > +        recorder1, recorder2 = record_two_runs(
> > +            get_subscribers, create_subscriber, 3)
> > +        self.assertThat(
> > +            recorder2, HasQueryCount(Equals(recorder1.count)))
> 
> By the way, I wasn't aware of record_two_runs.  Gives me a lot more
> confidence than our old attempts at testing exact query counts!

That's a great addition made by rvba.

> 
> 
> === modified file 'lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py'
> --- lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py 2012-01-01 02:58:52
> +0000
> +++ lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py 2012-01-03 17:52:00
> +0000
> 
> @@ -142,16 +141,75 @@
> 
> > +    def test_forTask(self):
> > +        # `forTask` returns a new `BugSubscriptionInfo` narrowed to the
> given
> > +        # bugtask.
> > +        info = self.getInfo()
> > +        self.assertIs(None, info.bugtask)
> > +        # If called with the current bugtask the same `BugSubscriptionInfo`
> > +        # instance is returned.
> > +        self.assertIs(info, info.forTask(info.bugtask))
> > +        # If called with a different bugtask a new `BugSubscriptionInfo` is
> > +        # created.
> > +        bugtask = self.bug.default_bugtask
> > +        info_for_task = info.forTask(bugtask)
> > +        self.assertIs(bugtask, info_for_task.bugtask)
> > +        self.assertIsNot(info, info_for_task)
> > +        # The instances share a cache of `BugSubscriptionInfo` instances.
> > +        expected_cache = {
> > +            info.cache_key: info,
> > +            info_for_task.cache_key: info_for_task,
> > +            }
> > +        self.assertEqual(expected_cache, info.cache)
> > +        self.assertIs(info.cache, info_for_task.cache)
> > +        # Calling `forTask` again looks in the cache first.
> > +        self.assertIs(info, info_for_task.forTask(info.bugtask))
> > +        self.assertIs(info_for_task, info.forTask(info_for_task.bugtask))
> > +        # The level is the same.
> > +        self.assertEqual(info.level, info_for_task.level)
> 
> This looks like multiple tests rolled into one, proving different
> facts and exercising different scenarios: “self.getInfo() returns an
> info without a bugtask,” “forTask for the current bugtask returns
> self,” “forTask for a bugtask other than the current one creates a
> new BugSubscriptionInfo,” “A new BugSubscriptionInfo created by
> forTask shares its cache with the original,” “A newly constructed
> BugSubscriptionInfo is in its own cache,” “A BugSubscriptionInfo
> created by forTask is in its own cache,” “forTask uses its cache.”
> 
> Unless getInfo is particularly expensive, I'd split this up more so
> that you exercise forTask only in one place per test.  It's also
> nice to see which properties break, or get fixed, or get
> implemented, during development.

getInfo() is almost free; it's the layer stuff and test setup that's
expensive, but that's not the problem here.

The later tests rely upon the earlier parts of the test. Breaking it
up would create a handful of functions, each of which would be a
subset of the next. I don't see a benefit to that.

> 
> And finally there's “A BugSubscriptionInfo returned from cache has
> the same level."  Imagine this property wasn't implemented, so that
> you just always got the default level.  Wouldn't this test still
> pass regardless?  This is one reason why in TDD you should watch the
> test fail before you make it pass.

BugSubscriptionInfo does not have a default level so I think this is
okay.

> 
> 
> > +    def test_forLevel(self):
> > +        # `forLevel` returns a new `BugSubscriptionInfo` narrowed to the
> given
> > +        # subscription level.
> > +        info = self.getInfo()
> > +        # If called with the current level the same `BugSubscriptionInfo`
> > +        # instance is returned.
> > +        self.assertIs(info, info.forLevel(info.level))
> > +        # If called with a different level a new `BugSubscriptionInfo` is
> > +        # created.
> > +        level = BugNotificationLevel.METADATA
> > +        info_for_level = info.forLevel(level)
> > +        self.assertEqual(level, info_for_level.level)
> > +        self.assertIsNot(info, info_for_level)
> 
> This does test the right thing.  A very small superficial note
> though: when you're contrasting a default value (in this case
> LIFECYCLE) to an explicit different value (in this case METADATA),
> better make both explicit in the test.  If you pass LIFECYCLE to
> self.getInfo(), it'll be clearer that info.level != METADATA in that
> first test.

Fair point, fixed.

> 
> 
> > +    def test_muted(self):
> > +        # The set of muted subscribers for the bug.
> > +        subscribers, subscriptions = self._create_direct_subscriptions()
> > +        sub1, sub2 = subscribers
> > +        with person_logged_in(sub1):
> > +            self.bug.mute(sub1, sub1)
> > +        self.assertEqual(set([sub1]), self.getInfo().muted_subscribers)
> 
> “There's an app for that.” :-)
> 
>     self.assertContentEqual([sub1], self.getInfo().muted_subscribers)

Done.

> 
> 
> @@ -159,6 +217,17 @@
>          found_subscriptions = self.getInfo().direct_subscriptions
>          self.assertEqual(set([subscriptions[1]]), found_subscriptions)
> 
> +    def test_all_direct(self):
> +        # The set of all direct subscribers, regardless of level.
> +        subscribers, subscriptions = self._create_direct_subscriptions()
> +        # Change the first subscription to be for comments only.
> +        sub1, sub2 = subscriptions
> +        with person_logged_in(sub1.person):
> +            sub1.bug_notification_level = BugNotificationLevel.LIFECYCLE
> +        info = self.getInfo(BugNotificationLevel.COMMENTS)
> +        self.assertEqual(set((sub2,)), info.direct_subscriptions)
> +        self.assertEqual(set((sub1, sub2)), info.all_direct_subscriptions)
> 
> Here too, I think self.assertContentEqual could come in handy.
> "set((sub2,))" is not very easy on the eyes!

I like it :)

However, I've changed to assertContentEqual() everywhere in the module
where it makes sense.

> 
> One reason for that on top of the obvious ones is: too many
> (parentheses, with multiple values), and (trailing commas after one
> value,) and set() for zero items, when what you mean is basically [a
> list].  A list is more consistent and more recognizable.
> 
> Lists are also more idiomatic if you just want an ordered set.
> Think of tuples not as containers, but as juxtapositions of items
> with different roles.  In python it's okay to use them as immutable
> lists sometimes, e.g. if you really need to save memory; but when in
> doubt, optimize for clarity.
> 
> 
> >      def test_all_pillar_owners_without_bug_supervisors(self):
> >          # The set of owners of pillars for which no bug supervisor is
> > -        # configured.
> > +        # configured and which use Launchpad for bug tracking.
> >          [bugtask] = self.bug.bugtasks
> >          found_owners = (
> >              self.getInfo().all_pillar_owners_without_bug_supervisors)
> >          self.assertEqual(set(), found_owners)
> > -        self.assertEqual((), found_owners.sorted)
> >          # Clear the supervisor for the first bugtask's target.
> >          with person_logged_in(bugtask.target.owner):
> >              bugtask.target.setBugSupervisor(None, bugtask.owner)
> > +            bugtask.pillar.official_malone = False
> > +        # The pillar does not use Launchpad, so the collection is still
> empty.
> > +        found_owners = (
> > +            self.getInfo().all_pillar_owners_without_bug_supervisors)
> > +        self.assertEqual(set(), found_owners)
> > +        # When a pillar does use Launchpad the collection includes the
> > +        # pillar's owner.
> > +        with person_logged_in(bugtask.target.owner):
> > +            bugtask.pillar.official_malone = True
> >          found_owners = (
> >              self.getInfo().all_pillar_owners_without_bug_supervisors)
> >          self.assertEqual(set([bugtask.pillar.owner]), found_owners)
> > -        self.assertEqual((bugtask.pillar.owner,), found_owners.sorted)
> > -        # Add another bugtask with a bug supervisor.
> > -        target2 = self.factory.makeProduct(bug_supervisor=None)
> > +        # Add another bugtask for a pillar that uses Launchpad but without
> a
> > +        # bug supervisor.
> > +        target2 = self.factory.makeProduct(
> > +            bug_supervisor=None, official_malone=True)
> >          bugtask2 = self.factory.makeBugTask(bug=self.bug, target=target2)
> >          found_owners = (
> >              self.getInfo().all_pillar_owners_without_bug_supervisors)
> >          self.assertEqual(
> >              set([bugtask.pillar.owner, bugtask2.pillar.owner]),
> >              found_owners)
> > +        # Getting subscription info for just a specific bugtask will yield
> > +        # owners for only the pillar associated with that bugtask.
> > +        info_for_bugtask2 = self.getInfo().forTask(bugtask2)
> >          self.assertEqual(
> > -            (bugtask.pillar.owner, bugtask2.pillar.owner),
> > -            found_owners.sorted)
> > +            set([bugtask2.pillar.owner]),
> > +            info_for_bugtask2.all_pillar_owners_without_bug_supervisors)
> 
> Don't do this!
> 
> You're inserting new scenarios into an ongoing test story.  That's the stuff
> maintenance nightmares are made of.  It makes the test hard to read, and data
> flows hard to track.
> 
> I'm perfectly willing to take it on faith that you're conscientiously not
> introducing any changes that reverberate through the rest of the test, and
> that no part of the test relies on anything that happened more than 2 lines or
> so before.  But the next person to work on this code won't know who wrote this
> or when.
> 
> Since they will have more urgent things on their mind than rewriting the test,
> they'll either read the whole thing for very little gain, or break it and then
> fix it up as an afterthought.  Neither is good for the project in the long
> run.

Another fair point :) I've split this into three tests.

> 
> 
> @@ -444,27 +538,28 @@
> 
> > -    def exercise_subscription_set(self, set_name):
> > +    def exercise_subscription_set(self, set_name, counts=(1, 1, 0)):
> >          # Looking up subscriptions takes a single query.
> > -        with self.exactly_x_queries(1):
> > +        with self.exactly_x_queries(counts[0]):
> 
> Could you make the meaning of "counts" explicit in some way?

Done. I've added a docstring to both exercise* methods.

> 
> 
> === modified file 'lib/lp/bugs/tests/test_structuralsubscription.py'
> --- lib/lp/bugs/tests/test_structuralsubscription.py    2012-01-01 02:58:52
> +0000
> +++ lib/lp/bugs/tests/test_structuralsubscription.py    2012-01-03 17:52:00
> +0000
> 
> @@ -634,6 +639,99 @@
> 
> > +class TestGetStructuralSubscriptions(TestCaseWithFactory):
> > +
> > +    layer = DatabaseFunctionalLayer
> >
> > +    def make_product_with_bug(self):
> > +        product = self.factory.makeProduct()
> > +        bug = self.factory.makeBug(product=product)
> > +        return product, bug
> > +
> > +    def test_get_structural_subscriptions_no_subscriptions(self):
> > +        # If there are no subscriptions for any of the bug's targets then
> no
> > +        # subscriptions will be returned by get_structural_subscriptions().
> > +        product, bug = self.make_product_with_bug()
> > +        subscriptions = get_structural_subscriptions(bug, None)
> > +        self.assertIsInstance(subscriptions, RESULT_SETS)
> > +        self.assertEqual([], list(subscriptions))
> 
> Much nicer tests here!  Why is that?

Erm... don't know :) Trick question?

Gavin.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Barki Mustapha

Celso Providelo

Christian Reis

Christy Awad

Colin Watson

Gavin Panella

Harpianto,ANDI

James Troup

John A Meinel

Kevin bush

Launchpad code reviewers

Launchpad code reviewers from Canonical

Matthew Tanner

Maximiliano Bertacchini

Oguz Ersoz

Simon Brakhane

Ubuntu-BR DevOps

William Grant

alhawiti

api.ng

pedro cavazos

todaioan

wenjingwen

to status/vote changes:

Tzaddi

Tzaddi Belding

 === modified file 'lib/lp/bugs/configure.zcml'
 --- lib/lp/bugs/configure.zcml	2011-12-30 08:03:42 +0000
 +++ lib/lp/bugs/configure.zcml	2012-01-04 18:01:30 +0000
@@ -650,6 +650,7 @@
            permission="launchpad.View"
            attributes="
                bug
++              bugtask
                level
                " />
        <!-- Properties -->
@@ -659,12 +660,24 @@
                all_assignees
                all_pillar_owners_without_bug_supervisors
                also_notified_subscribers
++              direct_subscribers
++              direct_subscribers_at_all_levels
                direct_subscriptions
++              direct_subscriptions_at_all_levels
                duplicate_only_subscriptions
                duplicate_subscriptions
                indirect_subscribers
++              muted_subscribers
++              structural_subscribers
                structural_subscriptions
                " />
++      <!-- Methods -->
++      <require
++          permission="launchpad.View"
++          attributes="
++              forLevel
++              forTask
++              " />
      </class>
      <!-- PersonSubscriptionInfo -->
 === modified file 'lib/lp/bugs/doc/bugsubscription.txt'
 --- lib/lp/bugs/doc/bugsubscription.txt	2011-12-24 17:49:30 +0000
 +++ lib/lp/bugs/doc/bugsubscription.txt	2012-01-04 18:01:30 +0000
@@ -98,17 +98,17 @@
  Finer-grained access to indirect subscribers is provided by
  getAlsoNotifiedSubscribers() and getSubscribersFromDuplicates().
--    >>> linux_source_bug.getAlsoNotifiedSubscribers()
++    >>> list(linux_source_bug.getAlsoNotifiedSubscribers())
      [<Person at ...>]
--    >>> linux_source_bug.getSubscribersFromDuplicates()
--    ()
++    >>> list(linux_source_bug.getSubscribersFromDuplicates())
++    []
  It is also possible to get the list of indirect subscribers for an
  individual bug task.
      >>> from lp.bugs.model.bug import get_also_notified_subscribers
      >>> res = get_also_notified_subscribers(linux_source_bug.bugtasks[0])
--    >>> res
++    >>> list(res)
      [<Person at ...>]
  These are security proxied.
 === modified file 'lib/lp/bugs/interfaces/bug.py'
 --- lib/lp/bugs/interfaces/bug.py	2012-01-01 02:58:52 +0000
 +++ lib/lp/bugs/interfaces/bug.py	2012-01-04 18:01:30 +0000
@@ -578,10 +578,11 @@
          If no such `BugSubscription` exists, return None.
          """
--    def getSubscriptionInfo(level):
++    def getSubscriptionInfo(level=None):
          """Return a `BugSubscriptionInfo` at the given `level`.
--        :param level: A member of `BugNotificationLevel`.
++        :param level: A member of `BugNotificationLevel`. Defaults to
++            `BugSubscriptionLevel.LIFECYCLE` if unspecified.
          """
      def getBugNotificationRecipients(duplicateof=None, old_bug=None,
 === modified file 'lib/lp/bugs/interfaces/bugtask.py'
 --- lib/lp/bugs/interfaces/bugtask.py	2012-01-01 02:58:52 +0000
 +++ lib/lp/bugs/interfaces/bugtask.py	2012-01-04 18:01:30 +0000
@@ -560,7 +560,7 @@
              title=_('Assigned to'), required=False,
              vocabulary='ValidAssignee',
              readonly=True))
--    assigneeID = Attribute('The assignee ID (for eager loading)')
++    assigneeID = Int(title=_('The assignee ID (for eager loading)'))
      bugtargetdisplayname = exported(
          Text(title=_("The short, descriptive name of the target"),
               readonly=True),
 === modified file 'lib/lp/bugs/model/bug.py'
 --- lib/lp/bugs/model/bug.py	2011-12-30 06:14:56 +0000
 +++ lib/lp/bugs/model/bug.py	2012-01-04 18:01:30 +0000
@@ -158,6 +158,7 @@
  from lp.bugs.model.bugwatch import BugWatch
  from lp.bugs.model.structuralsubscription import (
      get_structural_subscribers,
++    get_structural_subscriptions,
      get_structural_subscriptions_for_bug,
+     )
  from lp.code.interfaces.branchcollection import IAllBranches
@@ -948,8 +949,10 @@
              BugSubscription.bug_id == self.id).order_by(BugSubscription.id)
          return DecoratedResultSet(results, operator.itemgetter(1))
--    def getSubscriptionInfo(self, level=BugNotificationLevel.LIFECYCLE):
++    def getSubscriptionInfo(self, level=None):
          """See `IBug`."""
++        if level is None:
++            level = BugNotificationLevel.LIFECYCLE
          return BugSubscriptionInfo(self, level)
      def getDirectSubscriptions(self):
@@ -1002,6 +1005,7 @@
          # the regular proxied object.
          return sorted(
              indirect_subscribers,
++            # XXX: GavinPanella 2011-12-12 bug=911752: Use person_sort_key.
              key=lambda x: removeSecurityProxy(x).displayname)
      def getSubscriptionsFromDuplicates(self, recipients=None):
@@ -1030,14 +1034,13 @@
          if level is None:
              level = BugNotificationLevel.LIFECYCLE
          info = self.getSubscriptionInfo(level)
--
          if recipients is not None:
--            # Pre-load duplicate bugs.
--            list(self.duplicates)
++            list(self.duplicates)  # Pre-load duplicate bugs.
++            info.duplicate_only_subscribers  # Pre-load subscribers.
              for subscription in info.duplicate_only_subscriptions:
                  recipients.addDupeSubscriber(
                      subscription.person, subscription.bug)
--        return info.duplicate_only_subscriptions.subscribers.sorted
++        return info.duplicate_only_subscribers.sorted
      def getSubscribersForPerson(self, person):
          """See `IBug."""
@@ -2389,48 +2392,44 @@
      if IBug.providedBy(bug_or_bugtask):
          bug = bug_or_bugtask
          bugtasks = bug.bugtasks
++        info = bug.getSubscriptionInfo(level)
      elif IBugTask.providedBy(bug_or_bugtask):
          bug = bug_or_bugtask.bug
          bugtasks = [bug_or_bugtask]
++        info = bug.getSubscriptionInfo(level).forTask(bug_or_bugtask)
      else:
          raise ValueError('First argument must be bug or bugtask')
      if bug.private:
          return []
--    # Direct subscriptions always take precedence over indirect
--    # subscriptions.
--    direct_subscribers = set(bug.getDirectSubscribers())
--
--    also_notified_subscribers = set()
--
--    for bugtask in bugtasks:
--        if (bugtask.assignee and
--            bugtask.assignee not in direct_subscribers):
--            # We have an assignee that is not a direct subscriber.
--            also_notified_subscribers.add(bugtask.assignee)
--            if recipients is not None:
++    # Subscribers to exclude.
++    exclude_subscribers = frozenset().union(
++        info.direct_subscribers_at_all_levels, info.muted_subscribers)
++    # Get also-notified subscribers at the given level for the given tasks.
++    also_notified_subscribers = info.also_notified_subscribers
++
++    if recipients is not None:
++        for bugtask in bugtasks:
++            assignee = bugtask.assignee
++            if assignee in also_notified_subscribers:
++                # We have an assignee that is not a direct subscriber.
                  recipients.addAssignee(bugtask.assignee)
--
--        # If the target's bug supervisor isn't set...
--        pillar = bugtask.pillar
--        if (pillar.bug_supervisor is None and
--            pillar.official_malone and
--            pillar.owner not in direct_subscribers):
--            # ...we add the owner as a subscriber.
--            also_notified_subscribers.add(pillar.owner)
--            if recipients is not None:
--                recipients.addRegistrant(pillar.owner, pillar)
++            # If the target's bug supervisor isn't set...
++            pillar = bugtask.pillar
++            if pillar.official_malone and pillar.bug_supervisor is None:
++                if pillar.owner in also_notified_subscribers:
++                    # ...we add the owner as a subscriber.
++                    recipients.addRegistrant(pillar.owner, pillar)
      # This structural subscribers code omits direct subscribers itself.
--    also_notified_subscribers.update(
--        get_structural_subscribers(
--            bug_or_bugtask, recipients, level, direct_subscribers))
++    # TODO: Pass the info object into get_structural_subscribers for
++    # efficiency... or do the recipients stuff here.
++    structural_subscribers = get_structural_subscribers(
++        bug_or_bugtask, recipients, level, exclude_subscribers)
++    assert also_notified_subscribers.issuperset(structural_subscribers)
--    # Remove security proxy for the sort key, but return
--    # the regular proxied object.
--    return sorted(also_notified_subscribers,
--                  key=lambda x: removeSecurityProxy(x).displayname)
++    return also_notified_subscribers.sorted
  def load_people(*where):
@@ -2443,7 +2442,8 @@
          `ValidPersonCache` records are loaded simultaneously.
      """
      return PersonSet()._getPrecachedPersons(
--        origin=[Person], conditions=where, need_validity=True)
++        origin=[Person], conditions=where, need_validity=True,
++        need_preferred_email=True)
  class BugSubscriberSet(frozenset):
@@ -2573,13 +2573,57 @@
      def __init__(self, bug, level):
          self.bug = bug
++        self.bugtask = None  # Implies all.
          assert level is not None
          self.level = level
++        # This cache holds related `BugSubscriptionInfo` instances relating to
++        # the same bug but with different levels and/or choice of bugtask.
++        self.cache = {self.cache_key: self}
++        # This is often used in event handlers, many of which block implicit
++        # flushes. However, the data needs to be in the database for the
++        # queries herein to give correct answers.
++        Store.of(bug).flush()
++
++    @property
++    def cache_key(self):
++        """A (bug ID, bugtask ID, level) tuple for use as a hash key.
++
++        This helps `forTask()` and `forLevel()` to be more efficient,
++        returning previously populated instances to avoid running the same
++        queries against the database again and again.
++        """
++        bugtask_id = None if self.bugtask is None else self.bugtask.id
++        return self.bug.id, bugtask_id, self.level
++
++    def forTask(self, bugtask):
++        """Create a new `BugSubscriptionInfo` limited to `bugtask`.
++
++        The given task must refer to this object's bug. If `None` is passed a
++        new `BugSubscriptionInfo` instance is returned with no limit.
++        """
++        info = self.__class__(self.bug, self.level)
++        info.bugtask, info.cache = bugtask, self.cache
++        return self.cache.setdefault(info.cache_key, info)
++
++    def forLevel(self, level):
++        """Create a new `BugSubscriptionInfo` limited to `level`."""
++        info = self.__class__(self.bug, level)
++        info.bugtask, info.cache = self.bugtask, self.cache
++        return self.cache.setdefault(info.cache_key, info)
++
++    @cachedproperty
++    @freeze(BugSubscriberSet)
++    def muted_subscribers(self):
++        muted_people = Select(BugMute.person_id, BugMute.bug == self.bug)
++        return load_people(Person.id.is_in(muted_people))
      @cachedproperty
      @freeze(BugSubscriptionSet)
--    def old_direct_subscriptions(self):
--        """The bug's direct subscriptions."""
++    def direct_subscriptions(self):
++        """The bug's direct subscriptions.
++
++        Excludes muted subscriptions.
++        """
          return IStore(BugSubscription).find(
              BugSubscription,
              BugSubscription.bug_notification_level >= self.level,
@@ -2587,66 +2631,64 @@
              Not(In(BugSubscription.person_id,
                     Select(BugMute.person_id, BugMute.bug_id == self.bug.id))))
--    @cachedproperty
--    def direct_subscriptions_and_subscribers(self):
--        """The bug's direct subscriptions."""
--        res = IStore(BugSubscription).find(
--            (BugSubscription, Person),
--            BugSubscription.bug_notification_level >= self.level,
--            BugSubscription.bug == self.bug,
--            BugSubscription.person_id == Person.id,
--            Not(In(BugSubscription.person_id,
--                   Select(BugMute.person_id,
--                          BugMute.bug_id == self.bug.id))))
--        # Here we could test for res.count() but that will execute another
--        # query.  This structure avoids the extra query.
--        return zip(*res) or ((), ())
--
--    @cachedproperty
--    @freeze(BugSubscriptionSet)
--    def direct_subscriptions(self):
--        return self.direct_subscriptions_and_subscribers[0]
--
--    @cachedproperty
--    @freeze(BugSubscriberSet)
++    @property
      def direct_subscribers(self):
--        return self.direct_subscriptions_and_subscribers[1]
++        """The bug's direct subscriptions.
++
++        Excludes muted subscribers.
++        """
++        return self.direct_subscriptions.subscribers
++
++    @property
++    def direct_subscriptions_at_all_levels(self):
++        """The bug's direct subscriptions at all levels.
++
++        Excludes muted subscriptions.
++        """
++        return self.forLevel(
++            BugNotificationLevel.LIFECYCLE).direct_subscriptions
++
++    @property
++    def direct_subscribers_at_all_levels(self):
++        """The bug's direct subscribers at all levels.
++
++        Excludes muted subscribers.
++        """
++        return self.direct_subscriptions_at_all_levels.subscribers
      @cachedproperty
--    def duplicate_subscriptions_and_subscribers(self):
--        """Subscriptions to duplicates of the bug."""
++    @freeze(BugSubscriptionSet)
++    def duplicate_subscriptions(self):
++        """Subscriptions to duplicates of the bug.
++
++        Excludes muted subscriptions.
++        """
          if self.bug.private:
--            return ((), ())
++            return ()
          else:
--            res = IStore(BugSubscription).find(
--                (BugSubscription, Person),
++            return IStore(BugSubscription).find(
++                BugSubscription,
                  BugSubscription.bug_notification_level >= self.level,
                  BugSubscription.bug_id == Bug.id,
--                BugSubscription.person_id == Person.id,
                  Bug.duplicateof == self.bug,
                  Not(In(BugSubscription.person_id,
                         Select(BugMute.person_id, BugMute.bug_id == Bug.id))))
--        # Here we could test for res.count() but that will execute another
--        # query.  This structure avoids the extra query.
--        return zip(*res) or ((), ())
--
--    @cachedproperty
--    @freeze(BugSubscriptionSet)
--    def duplicate_subscriptions(self):
--        return self.duplicate_subscriptions_and_subscribers[0]
--
--    @cachedproperty
--    @freeze(BugSubscriberSet)
++
++    @property
      def duplicate_subscribers(self):
--        return self.duplicate_subscriptions_and_subscribers[1]
++        """Subscribers to duplicates of the bug.
++
++        Excludes muted subscribers.
++        """
++        return self.duplicate_subscriptions.subscribers
      @cachedproperty
      @freeze(BugSubscriptionSet)
      def duplicate_only_subscriptions(self):
--        """Subscriptions to duplicates of the bug.
++        """Subscriptions to duplicates of the bug only.
--        Excludes subscriptions for people who have a direct subscription or
--        are also notified for another reason.
++        Excludes muted subscriptions, subscriptions for people who have a
++        direct subscription, or who are also notified for another reason.
          """
          self.duplicate_subscribers  # Pre-load subscribers.
          higher_precedence = (
@@ -2656,47 +2698,85 @@
              subscription for subscription in self.duplicate_subscriptions
              if subscription.person not in higher_precedence)
++    @property
++    def duplicate_only_subscribers(self):
++        """Subscribers to duplicates of the bug only.
++
++        Excludes muted subscribers, subscribers who have a direct
++        subscription, or who are also notified for another reason.
++        """
++        return self.duplicate_only_subscriptions.subscribers
++
      @cachedproperty
      @freeze(StructuralSubscriptionSet)
      def structural_subscriptions(self):
--        """Structural subscriptions to the bug's targets."""
--        return list(get_structural_subscriptions_for_bug(self.bug))
++        """Structural subscriptions to the bug's targets.
++
++        Excludes direct subscriptions.
++        """
++        subject = self.bug if self.bugtask is None else self.bugtask
++        return get_structural_subscriptions(subject, self.level)
++
++    @property
++    def structural_subscribers(self):
++        """Structural subscribers to the bug's targets.
++
++        Excludes direct subscribers.
++        """
++        return self.structural_subscriptions.subscribers
      @cachedproperty
      @freeze(BugSubscriberSet)
      def all_assignees(self):
--        """Assignees of the bug's tasks."""
--        assignees = Select(BugTask.assigneeID, BugTask.bug == self.bug)
--        return load_people(Person.id.is_in(assignees))
++        """Assignees of the bug's tasks.
++
++        *Does not* exclude muted subscribers.
++        """
++        if self.bugtask is None:
++            assignees = Select(BugTask.assigneeID, BugTask.bug == self.bug)
++            return load_people(Person.id.is_in(assignees))
++        else:
++            return load_people(Person.id == self.bugtask.assigneeID)
      @cachedproperty
      @freeze(BugSubscriberSet)
      def all_pillar_owners_without_bug_supervisors(self):
--        """Owners of pillars for which no Bug supervisor is configured."""
--        for bugtask in self.bug.bugtasks:
++        """Owners of pillars for which there is no bug supervisor.
++
++        The pillars must also use Launchpad for bug tracking.
++
++        *Does not* exclude muted subscribers.
++        """
++        if self.bugtask is None:
++            bugtasks = self.bug.bugtasks
++        else:
++            bugtasks = [self.bugtask]
++        for bugtask in bugtasks:
              pillar = bugtask.pillar
--            if pillar.bug_supervisor is None:
--                yield pillar.owner
++            if pillar.official_malone:
++                if pillar.bug_supervisor is None:
++                    yield pillar.owner
      @cachedproperty
      def also_notified_subscribers(self):
--        """All subscribers except direct and dupe subscribers."""
++        """All subscribers except direct, dupe, and muted subscribers."""
          if self.bug.private:
              return BugSubscriberSet()
          else:
--            muted = IStore(BugMute).find(
--                Person,
--                BugMute.person_id == Person.id,
--                BugMute.bug == self.bug)
--            return BugSubscriberSet().union(
--                self.structural_subscriptions.subscribers,
++            subscribers = BugSubscriberSet().union(
++                self.structural_subscribers,
                  self.all_pillar_owners_without_bug_supervisors,
--                self.all_assignees).difference(
--                self.direct_subscribers).difference(muted)
++                self.all_assignees)
++            return subscribers.difference(
++                self.direct_subscribers_at_all_levels,
++                self.muted_subscribers)
      @cachedproperty
      def indirect_subscribers(self):
--        """All subscribers except direct subscribers."""
++        """All subscribers except direct subscribers.
++
++        Excludes muted subscribers.
++        """
          return self.also_notified_subscribers.union(
              self.duplicate_subscribers)
 === modified file 'lib/lp/bugs/model/structuralsubscription.py'
 --- lib/lp/bugs/model/structuralsubscription.py	2011-12-30 06:14:56 +0000
 +++ lib/lp/bugs/model/structuralsubscription.py	2012-01-04 18:01:30 +0000
@@ -3,10 +3,11 @@
  __metaclass__ = type
  __all__ = [
++    'get_structural_subscribers',
++    'get_structural_subscription_targets',
++    'get_structural_subscriptions',
      'get_structural_subscriptions_for_bug',
      'get_structural_subscriptions_for_target',
--    'get_structural_subscribers',
--    'get_structural_subscription_targets',
      'StructuralSubscription',
      'StructuralSubscriptionTargetMixin',
+     ]
@@ -478,14 +479,14 @@
      def getSubscriptions(self, subscriber=None):
          """See `IStructuralSubscriptionTarget`."""
          from lp.registry.model.person import Person
--        clauses = [StructuralSubscription.subscriberID==Person.id]
++        clauses = [StructuralSubscription.subscriberID == Person.id]
          for key, value in self._target_args.iteritems():
              clauses.append(
--                getattr(StructuralSubscription, key)==value)
++                getattr(StructuralSubscription, key) == value)
          if subscriber is not None:
              clauses.append(
--                StructuralSubscription.subscriberID==subscriber.id)
++                StructuralSubscription.subscriberID == subscriber.id)
          store = Store.of(self.__helper.pillar)
          return store.find(
@@ -588,54 +589,96 @@
          *conditions)
--def get_structural_subscribers(
--    bug_or_bugtask, recipients, level, direct_subscribers=None):
--    """Return subscribers for bug or bugtask at level.
--
--    :param bug: a bug.
--    :param recipients: a BugNotificationRecipients object or None.
--                       Populates if given.
--    :param level: a level from lp.bugs.enum.BugNotificationLevel.
--    :param direct_subscribers: a collection of Person objects who are
--                               directly subscribed to the bug.
--
--    Excludes structural subscriptions for people who are directly subscribed
--    to the bug."""
--    if IBug.providedBy(bug_or_bugtask):
--        bug = bug_or_bugtask
--        bugtasks = bug.bugtasks
--    elif IBugTask.providedBy(bug_or_bugtask):
--        bug = bug_or_bugtask.bug
--        bugtasks = [bug_or_bugtask]
--    else:
--        raise ValueError('First argument must be bug or bugtask')
++def query_structural_subscriptions(
++    what, bug, bugtasks, level, exclude=None):
++    """Query into structural subscriptions for a given bug.
++
++    :param what: The fields to fetch. Choose from `Person`,
++        `StructuralSubscription`, `BugSubscriptionFilter`, or a combo.
++    :param bug: An `IBug`
++    :param bugtasks: An iterable of `IBugTask`.
++    :param level: A level from `BugNotificationLevel`. Filters below this
++        level will be excluded.
++    :param exclude: `Person`s to exclude (e.g. direct subscribers).
++    """
++    from lp.registry.model.person import Person  # Circular.
      filter_id_query = (
          _get_structural_subscription_filter_id_query(
--            bug, bugtasks, level, direct_subscribers))
++            bug, bugtasks, level, exclude))
      if not filter_id_query:
          return EmptyResultSet()
--    # This is here because of a circular import.
--    from lp.registry.model.person import Person
      source = IStore(StructuralSubscription).using(
          StructuralSubscription,
          Join(BugSubscriptionFilter,
               BugSubscriptionFilter.structural_subscription_id ==
               StructuralSubscription.id),
          Join(Person,
--             Person.id == StructuralSubscription.subscriberID),
--        )
++             Person.id == StructuralSubscription.subscriberID))
++    conditions = In(
++        BugSubscriptionFilter.id, filter_id_query)
++    return source.find(what, conditions)
++
++
++def get_bug_and_bugtasks(bug_or_bugtask):
++    """Return a bug and a list of bugtasks given a bug or a bugtask.
++
++    :param bug_or_bugtask: An `IBug` or `IBugTask`.
++    :raises ValueError: If `bug_or_bugtask` does not provide `IBug` or
++        `IBugTask`.
++    """
++    if IBug.providedBy(bug_or_bugtask):
++        return bug_or_bugtask, bug_or_bugtask.bugtasks
++    elif IBugTask.providedBy(bug_or_bugtask):
++        return bug_or_bugtask.bug, [bug_or_bugtask]
++    else:
++        raise ValueError(
++            "Expected bug or bugtask, got %r" % (bug_or_bugtask,))
++
++
++def get_structural_subscriptions(bug_or_bugtask, level, exclude=None):
++    """Return subscriptions for bug or bugtask at level.
++
++    :param bug_or_bugtask: An `IBug` or `IBugTask`.
++    :param level: A level from `BugNotificationLevel`. Filters below this
++        level will be excluded.
++    :param exclude: `Person`s to exclude (e.g. direct subscribers).
++    """
++    from lp.registry.model.person import Person  # Circular.
++    bug, bugtasks = get_bug_and_bugtasks(bug_or_bugtask)
++    subscriptions = query_structural_subscriptions(
++        StructuralSubscription, bug, bugtasks, level, exclude)
++    # Return only the first subscription and filter per subscriber.
++    subscriptions.config(distinct=(Person.id,))
++    subscriptions.order_by(
++        Person.id, StructuralSubscription.id,
++        BugSubscriptionFilter.id)
++    return subscriptions
++
++
++def get_structural_subscribers(
++    bug_or_bugtask, recipients, level, exclude=None):
++    """Return subscribers for bug or bugtask at level.
++
++    :param bug_or_bugtask: An `IBug` or `IBugTask`.
++    :param recipients: A `BugNotificationRecipients` object or
++        `None`, which will be populated if provided.
++    :param level: A level from `BugNotificationLevel`. Filters below this
++        level will be excluded.
++    :param exclude: `Person`s to exclude (e.g. direct subscribers).
++    """
++    from lp.registry.model.person import Person  # Circular.
++    bug, bugtasks = get_bug_and_bugtasks(bug_or_bugtask)
      if recipients is None:
--        return source.find(
--            Person,
--            In(BugSubscriptionFilter.id,
--               filter_id_query)).config(distinct=True).order_by()
++        subscribers = query_structural_subscriptions(
++            Person, bug, bugtasks, level, exclude)
++        subscribers.config(distinct=True)
++        return subscribers.order_by()
      else:
++        results = query_structural_subscriptions(
++            (Person, StructuralSubscription, BugSubscriptionFilter),
++            bug, bugtasks, level, exclude)
          subscribers = []
--        query_results = source.find(
--            (Person, StructuralSubscription, BugSubscriptionFilter),
--            In(BugSubscriptionFilter.id, filter_id_query))
--        for person, subscription, filter in query_results:
--            # Set up results.
++        for person, subscription, filter in results:
              if person not in recipients:
                  subscribers.append(person)
                  recipients.addStructuralSubscriber(
@@ -839,7 +882,7 @@
              group_by=(BugSubscriptionFilter.id,),
              having=Count(
                  SQL('CASE WHEN BugSubscriptionFilterTag.include '
--                    'THEN BugSubscriptionFilterTag.tag END'))==0)
++                    'THEN BugSubscriptionFilterTag.tag END')) == 0)
      else:
          # The bug has some tags.  This will require a bit of fancy
          # footwork. First, though, we will simply want to leave out
 === modified file 'lib/lp/bugs/model/tests/test_bug.py'
 --- lib/lp/bugs/model/tests/test_bug.py	2012-01-01 02:58:52 +0000
 +++ lib/lp/bugs/model/tests/test_bug.py	2012-01-04 18:01:30 +0000
@@ -22,6 +22,7 @@
  from lp.bugs.errors import BugCannotBePrivate
  from lp.bugs.interfaces.bugnotification import IBugNotificationSet
  from lp.bugs.interfaces.bugtask import BugTaskStatus
++from lp.bugs.mail.bugnotificationrecipients import BugNotificationRecipients
  from lp.bugs.model.bug import (
      BugNotification,
      BugSubscriptionInfo,
@@ -35,6 +36,7 @@
      feature_flags,
      login_person,
      person_logged_in,
++    record_two_runs,
      set_feature_flag,
      StormStatementRecorder,
      TestCaseWithFactory,
@@ -166,7 +168,7 @@
          with StormStatementRecorder() as recorder:
              subscribers = list(bug.getDirectSubscribers())
              self.assertThat(len(subscribers), Equals(10 + 1))
--            self.assertThat(recorder, HasQueryCount(Equals(1)))
++            self.assertThat(recorder, HasQueryCount(Equals(2)))
      def test_mark_as_duplicate_query_count(self):
          bug = self.factory.makeBug()
@@ -476,6 +478,78 @@
          self.assertContentEqual(public_branches, linked_branches)
          self.assertNotIn(private_branch, linked_branches)
++    def test_getDirectSubscribers_with_recipients_query_count(self):
++        # getDirectSubscribers() uses a constant number of queries when given
++        # a recipients argument regardless of the number of subscribers.
++        bug = self.factory.makeBug()
++
++        def create_subscriber():
++            subscriber = self.factory.makePerson()
++            with person_logged_in(subscriber):
++                bug.subscribe(subscriber, subscriber)
++
++        def get_subscribers():
++            recipients = BugNotificationRecipients()
++            subs = bug.getDirectSubscribers(recipients=recipients)
++            list(subs)  # Ensure they're pulled.
++
++        recorder1, recorder2 = record_two_runs(
++            get_subscribers, create_subscriber, 3)
++        self.assertThat(
++            recorder2, HasQueryCount(Equals(recorder1.count)))
++
++    def test_getSubscribersFromDuplicates_with_recipients_query_count(self):
++        # getSubscribersFromDuplicates() uses a constant number of queries
++        # when given a recipients argument regardless of the number of
++        # subscribers.
++        bug = self.factory.makeBug()
++        duplicate_bug = self.factory.makeBug()
++        with person_logged_in(duplicate_bug.owner):
++            duplicate_bug.markAsDuplicate(bug)
++
++        def create_subscriber():
++            subscriber = self.factory.makePerson()
++            with person_logged_in(subscriber):
++                duplicate_bug.subscribe(subscriber, subscriber)
++
++        def get_subscribers():
++            recipients = BugNotificationRecipients()
++            subs = bug.getSubscribersFromDuplicates(recipients=recipients)
++            list(subs)  # Ensure they're pulled.
++
++        recorder1, recorder2 = record_two_runs(
++            get_subscribers, create_subscriber, 3)
++        self.assertThat(
++            recorder2, HasQueryCount(Equals(recorder1.count)))
++
++    def test_getAlsoNotifiedSubscribers_with_recipients_query_count(self):
++        # getAlsoNotifiedSubscribers() uses a constant number of queries when
++        # given a recipients argument regardless of the number of subscribers.
++        bug = self.factory.makeBug()
++
++        def create_stuff():
++            # Create a new bugtask, set its assignee, set its pillar's
++            # official_malone=True, and subscribe someone to its target.
++            bugtask = self.factory.makeBugTask(bug=bug)
++            with person_logged_in(bugtask.owner):
++                bugtask.transitionToAssignee(bugtask.owner)
++            with person_logged_in(bugtask.pillar.owner):
++                bugtask.pillar.official_malone = True
++            subscriber = self.factory.makePerson()
++            with person_logged_in(subscriber):
++                bugtask.target.addSubscription(
++                    subscriber, subscriber)
++
++        def get_subscribers():
++            recipients = BugNotificationRecipients()
++            subs = bug.getAlsoNotifiedSubscribers(recipients=recipients)
++            list(subs)  # Ensure they're pulled.
++
++        recorder1, recorder2 = record_two_runs(
++            get_subscribers, create_stuff, 3)
++        self.assertThat(
++            recorder2, HasQueryCount(Equals(recorder1.count)))
++
  class TestBugPrivateAndSecurityRelatedUpdatesMixin:
@@ -553,7 +627,7 @@
          # If the bug is for a private project, then other direct subscribers
          # should be unsubscribed.
--        (bug, bug_owner,  bugtask_a, bugtask_b, default_bugtask) = (
++        (bug, bug_owner, bugtask_a, bugtask_b, default_bugtask) = (
              self.createBugTasksAndSubscribers())
          initial_subscribers = set((
              self.factory.makePerson(), bugtask_a.owner, bug_owner,
@@ -586,7 +660,7 @@
          # If the bug is for a private project, then other direct subscribers
          # should be unsubscribed.
--        (bug, bug_owner,  bugtask_a, bugtask_b, default_bugtask) = (
++        (bug, bug_owner, bugtask_a, bugtask_b, default_bugtask) = (
              self.createBugTasksAndSubscribers(private_security_related=True))
          initial_subscribers = set((
              self.factory.makePerson(), bug_owner,
@@ -617,10 +691,10 @@
          # If the bug is for a private project, then other direct subscribers
          # should be unsubscribed.
--        (bug, bug_owner,  bugtask_a, bugtask_b, default_bugtask) = (
++        (bug, bug_owner, bugtask_a, bugtask_b, default_bugtask) = (
              self.createBugTasksAndSubscribers(private_security_related=True))
          initial_subscribers = set((
--            self.factory.makePerson(),  bug_owner,
++            self.factory.makePerson(), bug_owner,
              bugtask_a.pillar.security_contact, bugtask_a.pillar.driver,
              bugtask_a.pillar.bug_supervisor))
@@ -644,7 +718,7 @@
          # When a bug is marked as private=false and security_related=false,
          # any existing subscriptions are left alone.
--        (bug, bug_owner,  bugtask_a, bugtask_b, default_bugtask) = (
++        (bug, bug_owner, bugtask_a, bugtask_b, default_bugtask) = (
              self.createBugTasksAndSubscribers(private_security_related=True))
          initial_subscribers = set((
              self.factory.makePerson(), bug_owner,
@@ -751,7 +825,7 @@
          # The bug supervisors are unsubscribed if a bug is made public and an
          # email is sent telling them they have been unsubscribed.
--        (bug, bug_owner,  bugtask_a, bugtask_b, default_bugtask) = (
++        (bug, bug_owner, bugtask_a, bugtask_b, default_bugtask) = (
              self.createBugTasksAndSubscribers(private_security_related=True))
          with person_logged_in(bug_owner):
@@ -796,7 +870,7 @@
          # set to false and an email is sent telling them they have been
          # unsubscribed.
--        (bug, bug_owner,  bugtask_a, bugtask_b, default_bugtask) = (
++        (bug, bug_owner, bugtask_a, bugtask_b, default_bugtask) = (
              self.createBugTasksAndSubscribers(private_security_related=True))
          with person_logged_in(bug_owner):
 === modified file 'lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py'
 --- lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py	2012-01-01 02:58:52 +0000
 +++ lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py	2012-01-04 18:01:30 +0000
@@ -46,7 +46,7 @@
+             ]
          observed = load_people(
              Person.id.is_in(person.id for person in expected))
--        self.assertEqual(set(expected), set(observed))
++        self.assertContentEqual(expected, observed)
  class TestSubscriptionRelatedSets(TestCaseWithFactory):
@@ -128,9 +128,8 @@
          with person_logged_in(self.bug.owner):
              self.bug.unsubscribe(self.bug.owner, self.bug.owner)
--    def getInfo(self):
--        return BugSubscriptionInfo(
--            self.bug, BugNotificationLevel.LIFECYCLE)
++    def getInfo(self, level=BugNotificationLevel.LIFECYCLE):
++        return BugSubscriptionInfo(self.bug, level)
      def _create_direct_subscriptions(self):
          subscribers = (
@@ -142,22 +141,93 @@
                  for subscriber in subscribers)
          return subscribers, subscriptions
++    def test_forTask(self):
++        # `forTask` returns a new `BugSubscriptionInfo` narrowed to the given
++        # bugtask.
++        info = self.getInfo()
++        self.assertIs(None, info.bugtask)
++        # If called with the current bugtask the same `BugSubscriptionInfo`
++        # instance is returned.
++        self.assertIs(info, info.forTask(info.bugtask))
++        # If called with a different bugtask a new `BugSubscriptionInfo` is
++        # created.
++        bugtask = self.bug.default_bugtask
++        info_for_task = info.forTask(bugtask)
++        self.assertIs(bugtask, info_for_task.bugtask)
++        self.assertIsNot(info, info_for_task)
++        # The instances share a cache of `BugSubscriptionInfo` instances.
++        expected_cache = {
++            info.cache_key: info,
++            info_for_task.cache_key: info_for_task,
++            }
++        self.assertEqual(expected_cache, info.cache)
++        self.assertIs(info.cache, info_for_task.cache)
++        # Calling `forTask` again looks in the cache first.
++        self.assertIs(info, info_for_task.forTask(info.bugtask))
++        self.assertIs(info_for_task, info.forTask(info_for_task.bugtask))
++        # The level is the same.
++        self.assertEqual(info.level, info_for_task.level)
++
++    def test_forLevel(self):
++        # `forLevel` returns a new `BugSubscriptionInfo` narrowed to the given
++        # subscription level.
++        info = self.getInfo(BugNotificationLevel.LIFECYCLE)
++        # If called with the current level the same `BugSubscriptionInfo`
++        # instance is returned.
++        self.assertIs(info, info.forLevel(info.level))
++        # If called with a different level a new `BugSubscriptionInfo` is
++        # created.
++        level = BugNotificationLevel.METADATA
++        info_for_level = info.forLevel(level)
++        self.assertEqual(level, info_for_level.level)
++        self.assertIsNot(info, info_for_level)
++        # The instances share a cache of `BugSubscriptionInfo` instances.
++        expected_cache = {
++            info.cache_key: info,
++            info_for_level.cache_key: info_for_level,
++            }
++        self.assertEqual(expected_cache, info.cache)
++        self.assertIs(info.cache, info_for_level.cache)
++        # Calling `forLevel` again looks in the cache first.
++        self.assertIs(info, info_for_level.forLevel(info.level))
++        self.assertIs(info_for_level, info.forLevel(info_for_level.level))
++        # The bugtask is the same.
++        self.assertIs(info.bugtask, info_for_level.bugtask)
++
++    def test_muted(self):
++        # The set of muted subscribers for the bug.
++        subscribers, subscriptions = self._create_direct_subscriptions()
++        sub1, sub2 = subscribers
++        with person_logged_in(sub1):
++            self.bug.mute(sub1, sub1)
++        self.assertContentEqual([sub1], self.getInfo().muted_subscribers)
++
      def test_direct(self):
          # The set of direct subscribers.
          subscribers, subscriptions = self._create_direct_subscriptions()
          found_subscriptions = self.getInfo().direct_subscriptions
--        self.assertEqual(set(subscriptions), found_subscriptions)
--        self.assertEqual(subscriptions, found_subscriptions.sorted)
--        self.assertEqual(set(subscribers), found_subscriptions.subscribers)
--        self.assertEqual(subscribers, found_subscriptions.subscribers.sorted)
++        self.assertContentEqual(subscriptions, found_subscriptions)
++        self.assertContentEqual(subscribers, found_subscriptions.subscribers)
--    def test_muted_direct(self):
++    def test_direct_muted(self):
          # If a direct is muted, it is not listed.
          subscribers, subscriptions = self._create_direct_subscriptions()
          with person_logged_in(subscribers[0]):
              self.bug.mute(subscribers[0], subscribers[0])
          found_subscriptions = self.getInfo().direct_subscriptions
--        self.assertEqual(set([subscriptions[1]]), found_subscriptions)
++        self.assertContentEqual([subscriptions[1]], found_subscriptions)
++
++    def test_all_direct(self):
++        # The set of all direct subscribers, regardless of level.
++        subscribers, subscriptions = self._create_direct_subscriptions()
++        # Change the first subscription to be for comments only.
++        sub1, sub2 = subscriptions
++        with person_logged_in(sub1.person):
++            sub1.bug_notification_level = BugNotificationLevel.LIFECYCLE
++        info = self.getInfo(BugNotificationLevel.COMMENTS)
++        self.assertContentEqual([sub2], info.direct_subscriptions)
++        self.assertContentEqual(
++            [sub1, sub2], info.direct_subscriptions_at_all_levels)
      def _create_duplicate_subscription(self):
          duplicate_bug = self.factory.makeBug(product=self.target)
@@ -171,34 +241,26 @@
      def test_duplicate(self):
          # The set of subscribers from duplicate bugs.
          found_subscriptions = self.getInfo().duplicate_subscriptions
--        self.assertEqual(set(), found_subscriptions)
--        self.assertEqual((), found_subscriptions.sorted)
--        self.assertEqual(set(), found_subscriptions.subscribers)
--        self.assertEqual((), found_subscriptions.subscribers.sorted)
++        self.assertContentEqual([], found_subscriptions)
++        self.assertContentEqual([], found_subscriptions.subscribers)
          duplicate_bug, duplicate_bug_subscription = (
              self._create_duplicate_subscription())
          found_subscriptions = self.getInfo().duplicate_subscriptions
--        self.assertEqual(
--            set([duplicate_bug_subscription]),
++        self.assertContentEqual(
++            [duplicate_bug_subscription],
              found_subscriptions)
--        self.assertEqual(
--            (duplicate_bug_subscription,),
--            found_subscriptions.sorted)
--        self.assertEqual(
--            set([duplicate_bug.owner]),
++        self.assertContentEqual(
++            [duplicate_bug.owner],
              found_subscriptions.subscribers)
--        self.assertEqual(
--            (duplicate_bug.owner,),
--            found_subscriptions.subscribers.sorted)
--    def test_muted_duplicate(self):
++    def test_duplicate_muted(self):
          # If a duplicate is muted, it is not listed.
          duplicate_bug, duplicate_bug_subscription = (
              self._create_duplicate_subscription())
          with person_logged_in(duplicate_bug.owner):
              self.bug.mute(duplicate_bug.owner, duplicate_bug.owner)
          found_subscriptions = self.getInfo().duplicate_subscriptions
--        self.assertEqual(set(), found_subscriptions)
++        self.assertContentEqual([], found_subscriptions)
      def test_duplicate_for_private_bug(self):
          # The set of subscribers from duplicate bugs is always empty when the
@@ -209,10 +271,8 @@
          with person_logged_in(self.bug.owner):
              self.bug.setPrivate(True, self.bug.owner)
          found_subscriptions = self.getInfo().duplicate_subscriptions
--        self.assertEqual(set(), found_subscriptions)
--        self.assertEqual((), found_subscriptions.sorted)
--        self.assertEqual(set(), found_subscriptions.subscribers)
--        self.assertEqual((), found_subscriptions.subscribers.sorted)
++        self.assertContentEqual([], found_subscriptions)
++        self.assertContentEqual([], found_subscriptions.subscribers)
      def test_duplicate_only(self):
          # The set of duplicate subscriptions where the subscriber has no other
@@ -224,8 +284,8 @@
                  duplicate_bug.getSubscriptionForPerson(
                      duplicate_bug.owner))
          found_subscriptions = self.getInfo().duplicate_only_subscriptions
--        self.assertEqual(
--            set([duplicate_bug_subscription]),
++        self.assertContentEqual(
++            [duplicate_bug_subscription],
              found_subscriptions)
          # If a user is subscribed to a duplicate bug and is a bugtask
          # assignee, for example, their duplicate subscription will not be
@@ -234,71 +294,136 @@
              self.bug.default_bugtask.transitionToAssignee(
                  duplicate_bug_subscription.person)
          found_subscriptions = self.getInfo().duplicate_only_subscriptions
--        self.assertEqual(set(), found_subscriptions)
--
--    def test_structural(self):
++        self.assertContentEqual([], found_subscriptions)
++
++    def test_structural_subscriptions(self):
++        # The set of structural subscriptions.
++        subscribers = (
++            self.factory.makePerson(),
++            self.factory.makePerson())
++        with person_logged_in(self.bug.owner):
++            subscriptions = tuple(
++                self.target.addBugSubscription(subscriber, subscriber)
++                for subscriber in subscribers)
++        found_subscriptions = self.getInfo().structural_subscriptions
++        self.assertContentEqual(subscriptions, found_subscriptions)
++
++    def test_structural_subscriptions_muted(self):
++        # The set of structural subscriptions DOES NOT exclude muted
++        # subscriptions.
++        subscriber = self.factory.makePerson()
++        with person_logged_in(subscriber):
++            self.bug.mute(subscriber, subscriber)
++        with person_logged_in(self.bug.owner):
++            subscription = self.target.addBugSubscription(
++                subscriber, subscriber)
++        found_subscriptions = self.getInfo().structural_subscriptions
++        self.assertContentEqual([subscription], found_subscriptions)
++
++    def test_structural_subscribers(self):
          # The set of structural subscribers.
          subscribers = (
              self.factory.makePerson(),
              self.factory.makePerson())
          with person_logged_in(self.bug.owner):
--            subscriptions = tuple(
++            for subscriber in subscribers:
                  self.target.addBugSubscription(subscriber, subscriber)
--                for subscriber in subscribers)
--        found_subscriptions = self.getInfo().structural_subscriptions
--        self.assertEqual(set(subscriptions), found_subscriptions)
--        self.assertEqual(subscriptions, found_subscriptions.sorted)
--        self.assertEqual(set(subscribers), found_subscriptions.subscribers)
--        self.assertEqual(subscribers, found_subscriptions.subscribers.sorted)
++        found_subscribers = self.getInfo().structural_subscribers
++        self.assertContentEqual(subscribers, found_subscribers)
++
++    def test_structural_subscribers_muted(self):
++        # The set of structural subscribers DOES NOT exclude muted
++        # subscribers.
++        subscriber = self.factory.makePerson()
++        with person_logged_in(subscriber):
++            self.bug.mute(subscriber, subscriber)
++        with person_logged_in(self.bug.owner):
++            self.target.addBugSubscription(subscriber, subscriber)
++        found_subscribers = self.getInfo().structural_subscribers
++        self.assertContentEqual([subscriber], found_subscribers)
      def test_all_assignees(self):
          # The set of bugtask assignees for bugtasks that have been assigned.
          found_assignees = self.getInfo().all_assignees
--        self.assertEqual(set(), found_assignees)
--        self.assertEqual((), found_assignees.sorted)
++        self.assertContentEqual([], found_assignees)
          bugtask = self.bug.default_bugtask
          with person_logged_in(bugtask.pillar.bug_supervisor):
              bugtask.transitionToAssignee(self.bug.owner)
          found_assignees = self.getInfo().all_assignees
--        self.assertEqual(set([self.bug.owner]), found_assignees)
--        self.assertEqual((self.bug.owner,), found_assignees.sorted)
++        self.assertContentEqual([self.bug.owner], found_assignees)
          bugtask2 = self.factory.makeBugTask(bug=self.bug)
          with person_logged_in(bugtask2.pillar.owner):
              bugtask2.transitionToAssignee(bugtask2.owner)
          found_assignees = self.getInfo().all_assignees
--        self.assertEqual(
--            set([self.bug.owner, bugtask2.owner]),
++        self.assertContentEqual(
++            [self.bug.owner, bugtask2.owner],
              found_assignees)
--        self.assertEqual(
--            (self.bug.owner, bugtask2.owner),
--            found_assignees.sorted)
++        # Getting info for a specific bugtask will return the assignee for
++        # that bugtask only.
++        self.assertContentEqual(
++            [bugtask2.owner],
++            self.getInfo().forTask(bugtask2).all_assignees)
      def test_all_pillar_owners_without_bug_supervisors(self):
          # The set of owners of pillars for which no bug supervisor is
--        # configured.
++        # configured and which use Launchpad for bug tracking.
          [bugtask] = self.bug.bugtasks
          found_owners = (
              self.getInfo().all_pillar_owners_without_bug_supervisors)
--        self.assertEqual(set(), found_owners)
--        self.assertEqual((), found_owners.sorted)
--        # Clear the supervisor for the first bugtask's target.
++        self.assertContentEqual([], found_owners)
++        # Clear the supervisor for the bugtask's target and ensure that the
++        # project uses Launchpad Bugs.
          with person_logged_in(bugtask.target.owner):
              bugtask.target.setBugSupervisor(None, bugtask.owner)
++            bugtask.pillar.official_malone = True
++        # The collection includes the pillar's owner.
          found_owners = (
              self.getInfo().all_pillar_owners_without_bug_supervisors)
--        self.assertEqual(set([bugtask.pillar.owner]), found_owners)
--        self.assertEqual((bugtask.pillar.owner,), found_owners.sorted)
--        # Add another bugtask with a bug supervisor.
--        target2 = self.factory.makeProduct(bug_supervisor=None)
++        self.assertContentEqual([bugtask.pillar.owner], found_owners)
++        # Add another bugtask for a pillar that uses Launchpad but does not
++        # have a bug supervisor.
++        target2 = self.factory.makeProduct(
++            bug_supervisor=None, official_malone=True)
          bugtask2 = self.factory.makeBugTask(bug=self.bug, target=target2)
          found_owners = (
              self.getInfo().all_pillar_owners_without_bug_supervisors)
--        self.assertEqual(
--            set([bugtask.pillar.owner, bugtask2.pillar.owner]),
++        self.assertContentEqual(
++            [bugtask.pillar.owner, bugtask2.pillar.owner],
              found_owners)
--        self.assertEqual(
--            (bugtask.pillar.owner, bugtask2.pillar.owner),
--            found_owners.sorted)
++
++    def test_all_pillar_owners_without_bug_supervisors_not_using_malone(self):
++        # The set of owners of pillars for which no bug supervisor is
++        # configured and which do not use Launchpad for bug tracking is empty.
++        [bugtask] = self.bug.bugtasks
++        # Clear the supervisor for the first bugtask's target and ensure the
++        # project does not use Launchpad Bugs.
++        with person_logged_in(bugtask.target.owner):
++            bugtask.target.setBugSupervisor(None, bugtask.owner)
++            bugtask.pillar.official_malone = False
++        found_owners = (
++            self.getInfo().all_pillar_owners_without_bug_supervisors)
++        self.assertContentEqual([], found_owners)
++
++    def test_all_pillar_owners_without_bug_supervisors_for_bugtask(self):
++        # The set of the owner of the chosen bugtask's pillar when no bug
++        # supervisor is configured and which uses Launchpad for bug tracking.
++        [bugtask] = self.bug.bugtasks
++        # Clear the supervisor for the bugtask's target and ensure that the
++        # project uses Launchpad Bugs.
++        with person_logged_in(bugtask.target.owner):
++            bugtask.target.setBugSupervisor(None, bugtask.owner)
++            bugtask.pillar.official_malone = True
++        # Add another bugtask for a pillar that uses Launchpad but does not
++        # have a bug supervisor.
++        target2 = self.factory.makeProduct(
++            bug_supervisor=None, official_malone=True)
++        bugtask2 = self.factory.makeBugTask(bug=self.bug, target=target2)
++        # Getting subscription info for just a specific bugtask will yield
++        # owners for only the pillar associated with that bugtask.
++        info_for_bugtask2 = self.getInfo().forTask(bugtask2)
++        self.assertContentEqual(
++            [bugtask2.pillar.owner],
++            info_for_bugtask2.all_pillar_owners_without_bug_supervisors)
      def _create_also_notified_subscribers(self):
          # Add an assignee, a bug supervisor and a structural subscriber.
@@ -318,8 +443,7 @@
      def test_also_notified_subscribers(self):
          # The set of also notified subscribers.
          found_subscribers = self.getInfo().also_notified_subscribers
--        self.assertEqual(set(), found_subscribers)
--        self.assertEqual((), found_subscribers.sorted)
++        self.assertContentEqual([], found_subscribers)
          assignee, supervisor, structural_subscriber = (
              self._create_also_notified_subscribers())
          # Add a direct subscription.
@@ -329,14 +453,11 @@
          # The direct subscriber does not appear in the also notified set, but
          # the assignee, supervisor and structural subscriber do.
          found_subscribers = self.getInfo().also_notified_subscribers
--        self.assertEqual(
--            set([assignee, supervisor, structural_subscriber]),
++        self.assertContentEqual(
++            [assignee, supervisor, structural_subscriber],
              found_subscribers)
--        self.assertEqual(
--            (assignee, supervisor, structural_subscriber),
--            found_subscribers.sorted)
--    def test_muted_also_notified_subscribers(self):
++    def test_also_notified_subscribers_muted(self):
          # If someone is muted, they are not listed in the
          # also_notified_subscribers.
          assignee, supervisor, structural_subscriber = (
@@ -345,8 +466,8 @@
          # the assignee, supervisor and structural subscriber do show up
          # when they are not muted.
          found_subscribers = self.getInfo().also_notified_subscribers
--        self.assertEqual(
--            set([assignee, supervisor, structural_subscriber]),
++        self.assertContentEqual(
++            [assignee, supervisor, structural_subscriber],
              found_subscribers)
          # Now we mute all of the subscribers.
          with person_logged_in(assignee):
@@ -357,8 +478,7 @@
              self.bug.mute(structural_subscriber, structural_subscriber)
          # Now we don't see them.
          found_subscribers = self.getInfo().also_notified_subscribers
--        self.assertEqual(
--            set(), found_subscribers)
++        self.assertContentEqual([], found_subscribers)
      def test_also_notified_subscribers_for_private_bug(self):
          # The set of also notified subscribers is always empty when the master
@@ -370,8 +490,7 @@
          with person_logged_in(self.bug.owner):
              self.bug.setPrivate(True, self.bug.owner)
          found_subscribers = self.getInfo().also_notified_subscribers
--        self.assertEqual(set(), found_subscribers)
--        self.assertEqual((), found_subscribers.sorted)
++        self.assertContentEqual([], found_subscribers)
      def test_indirect_subscribers(self):
          # The set of indirect subscribers is the union of also notified
@@ -384,12 +503,9 @@
          with person_logged_in(duplicate_bug.owner):
              duplicate_bug.markAsDuplicate(self.bug)
          found_subscribers = self.getInfo().indirect_subscribers
--        self.assertEqual(
--            set([assignee, duplicate_bug.owner]),
++        self.assertContentEqual(
++            [assignee, duplicate_bug.owner],
              found_subscribers)
--        self.assertEqual(
--            (assignee, duplicate_bug.owner),
--            found_subscribers.sorted)
  class TestBugSubscriptionInfoPermissions(TestCaseWithFactory):
@@ -406,7 +522,7 @@
          # All attributes require launchpad.View.
          permissions = set(checker.get_permissions.itervalues())
--        self.assertEqual(set(["launchpad.View"]), permissions)
++        self.assertContentEqual(["launchpad.View"], permissions)
          # The security adapter for launchpad.View lets anyone reference the
          # attributes unless the bug is private, in which case only explicit
@@ -444,27 +560,46 @@
              yield recorder
          self.assertThat(recorder, condition)
--    def exercise_subscription_set(self, set_name):
++    def exercise_subscription_set(self, set_name, counts=(1, 1, 0)):
++        """Test the number of queries it takes to inspect a subscription set.
++
++        :param set_name: The name of the set, e.g. "direct_subscriptions".
++        :param counts: A triple of the expected query counts for each of three
++            operations: get the set, get the set's subscribers, get the set's
++            subscribers in order.
++        """
          # Looking up subscriptions takes a single query.
--        with self.exactly_x_queries(1):
++        with self.exactly_x_queries(counts[0]):
              getattr(self.info, set_name)
          # Getting the subscribers results in one additional query.
--        with self.exactly_x_queries(1):
++        with self.exactly_x_queries(counts[1]):
              getattr(self.info, set_name).subscribers
          # Everything is now cached so no more queries are needed.
--        with self.exactly_x_queries(0):
++        with self.exactly_x_queries(counts[2]):
              getattr(self.info, set_name).subscribers
              getattr(self.info, set_name).subscribers.sorted
--    def exercise_subscription_set_sorted_first(self, set_name):
++    def exercise_subscription_set_sorted_first(
++        self, set_name, counts=(1, 1, 0)):
++        """Test the number of queries it takes to inspect a subscription set.
++
++        This differs from `exercise_subscription_set` in its second step, when
++        it looks at the sorted subscription list instead of the subscriber
++        set.
++
++        :param set_name: The name of the set, e.g. "direct_subscriptions".
++        :param counts: A triple of the expected query counts for each of three
++            operations: get the set, get the set in order, get the set's
++            subscribers in order.
++        """
          # Looking up subscriptions takes a single query.
--        with self.exactly_x_queries(1):
++        with self.exactly_x_queries(counts[0]):
              getattr(self.info, set_name)
          # Getting the sorted subscriptions takes one additional query.
--        with self.exactly_x_queries(1):
++        with self.exactly_x_queries(counts[1]):
              getattr(self.info, set_name).sorted
          # Everything is now cached so no more queries are needed.
--        with self.exactly_x_queries(0):
++        with self.exactly_x_queries(counts[2]):
              getattr(self.info, set_name).subscribers
              getattr(self.info, set_name).subscribers.sorted
@@ -476,6 +611,10 @@
          self.exercise_subscription_set_sorted_first(
              "direct_subscriptions")
++    def test_direct_subscriptions_at_all_levels(self):
++        self.exercise_subscription_set(
++            "direct_subscriptions_at_all_levels")
++
      def make_duplicate_bug(self):
          duplicate_bug = self.factory.makeBug(product=self.target)
          with person_logged_in(duplicate_bug.owner):
@@ -501,18 +640,19 @@
              self.info.duplicate_subscriptions.subscribers
      def add_structural_subscriber(self):
--        with person_logged_in(self.bug.owner):
--            self.target.addSubscription(self.bug.owner, self.bug.owner)
++        subscriber = self.factory.makePerson()
++        with person_logged_in(subscriber):
++            self.target.addSubscription(subscriber, subscriber)
      def test_structural_subscriptions(self):
          self.add_structural_subscriber()
          self.exercise_subscription_set(
--            "structural_subscriptions")
++            "structural_subscriptions", (2, 1, 0))
      def test_structural_subscriptions_sorted_first(self):
          self.add_structural_subscriber()
          self.exercise_subscription_set_sorted_first(
--            "structural_subscriptions")
++            "structural_subscriptions", (2, 1, 0))
      def test_all_assignees(self):
          with self.exactly_x_queries(1):
@@ -522,8 +662,8 @@
          # Getting all bug supervisors and pillar owners can take several
          # queries. However, there are typically few tasks so the trade for
          # simplicity of implementation is acceptable. Only the simplest case
--        # is tested here.
--        with self.exactly_x_queries(1):
++        # is tested here (everything is already cached).
++        with self.exactly_x_queries(0):
              self.info.all_pillar_owners_without_bug_supervisors
      def test_also_notified_subscribers(self):
@@ -536,7 +676,7 @@
          self.info.all_assignees
          self.info.all_pillar_owners_without_bug_supervisors
          self.info.direct_subscriptions.subscribers
--        self.info.structural_subscriptions.subscribers
++        self.info.structural_subscribers
          with self.exactly_x_queries(1):
              self.info.also_notified_subscribers
 === modified file 'lib/lp/bugs/tests/test_structuralsubscription.py'
 --- lib/lp/bugs/tests/test_structuralsubscription.py	2012-01-01 02:58:52 +0000
 +++ lib/lp/bugs/tests/test_structuralsubscription.py	2012-01-04 18:01:30 +0000
@@ -27,8 +27,10 @@
  from lp.bugs.model.structuralsubscription import (
      get_structural_subscribers,
      get_structural_subscription_targets,
++    get_structural_subscriptions,
      get_structural_subscriptions_for_bug,
+     )
++from lp.services.database.decoratedresultset import DecoratedResultSet
  from lp.testing import (
      anonymous_logged_in,
      login_person,
@@ -42,6 +44,9 @@
+     )
++RESULT_SETS = ResultSet, EmptyResultSet, DecoratedResultSet
++
++
  class TestStructuralSubscription(TestCaseWithFactory):
      layer = DatabaseFunctionalLayer
@@ -634,6 +639,99 @@
          self.assertEqual(set([self_sub]), set(subscriptions))
++class TestGetStructuralSubscriptions(TestCaseWithFactory):
++
++    layer = DatabaseFunctionalLayer
++
++    def make_product_with_bug(self):
++        product = self.factory.makeProduct()
++        bug = self.factory.makeBug(product=product)
++        return product, bug
++
++    def test_get_structural_subscriptions_no_subscriptions(self):
++        # If there are no subscriptions for any of the bug's targets then no
++        # subscriptions will be returned by get_structural_subscriptions().
++        product, bug = self.make_product_with_bug()
++        subscriptions = get_structural_subscriptions(bug, None)
++        self.assertIsInstance(subscriptions, RESULT_SETS)
++        self.assertEqual([], list(subscriptions))
++
++    def test_get_structural_subscriptions_single_target(self):
++        # Subscriptions for any of the bug's targets are returned.
++        subscriber = self.factory.makePerson()
++        login_person(subscriber)
++        product, bug = self.make_product_with_bug()
++        subscription = product.addBugSubscription(subscriber, subscriber)
++        self.assertContentEqual(
++            [subscription], get_structural_subscriptions(bug, None))
++
++    def test_get_structural_subscriptions_multiple_targets(self):
++        # Subscriptions for any of the bug's targets are returned.
++        actor = self.factory.makePerson()
++        login_person(actor)
++
++        subscriber1 = self.factory.makePerson()
++        subscriber2 = self.factory.makePerson()
++
++        product1 = self.factory.makeProduct(owner=actor)
++        subscription1 = product1.addBugSubscription(subscriber1, subscriber1)
++        product2 = self.factory.makeProduct(owner=actor)
++        subscription2 = product2.addBugSubscription(subscriber2, subscriber2)
++
++        bug = self.factory.makeBug(product=product1)
++        bug.addTask(actor, product2)
++
++        subscriptions = get_structural_subscriptions(bug, None)
++        self.assertIsInstance(subscriptions, RESULT_SETS)
++        self.assertContentEqual(
++            [subscription1, subscription2], subscriptions)
++
++    def test_get_structural_subscriptions_multiple_targets_2(self):
++        # Only the first of multiple subscriptions for a person is returned
++        # when they have multiple matching subscriptions.
++        actor = self.factory.makePerson()
++        login_person(actor)
++
++        subscriber = self.factory.makePerson()
++        product1 = self.factory.makeProduct(owner=actor)
++        subscription1 = product1.addBugSubscription(subscriber, subscriber)
++        product2 = self.factory.makeProduct(owner=actor)
++        product2.addBugSubscription(subscriber, subscriber)
++
++        bug = self.factory.makeBug(product=product1)
++        bug.addTask(actor, product2)
++
++        subscriptions = get_structural_subscriptions(bug, None)
++        self.assertIsInstance(subscriptions, RESULT_SETS)
++        self.assertContentEqual([subscription1], subscriptions)
++
++    def test_get_structural_subscriptions_level(self):
++        # get_structural_subscriptions() respects the given level.
++        subscriber = self.factory.makePerson()
++        login_person(subscriber)
++        product, bug = self.make_product_with_bug()
++        subscription = product.addBugSubscription(subscriber, subscriber)
++        filter = subscription.bug_filters.one()
++        filter.bug_notification_level = BugNotificationLevel.METADATA
++        self.assertContentEqual(
++            [subscription], get_structural_subscriptions(
++                bug, BugNotificationLevel.METADATA))
++        self.assertContentEqual(
++            [], get_structural_subscriptions(
++                bug, BugNotificationLevel.COMMENTS))
++
++    def test_get_structural_subscriptions_exclude(self):
++        # Subscriptions for any of the given excluded subscribers are not
++        # returned.
++        subscriber = self.factory.makePerson()
++        login_person(subscriber)
++        product, bug = self.make_product_with_bug()
++        product.addBugSubscription(subscriber, subscriber)
++        self.assertContentEqual(
++            [], get_structural_subscriptions(
++                bug, None, exclude=[subscriber]))
++
++
  class TestGetStructuralSubscribers(TestCaseWithFactory):
      layer = DatabaseFunctionalLayer
@@ -648,7 +746,7 @@
          # subscribers will be returned by get_structural_subscribers().
          product, bug = self.make_product_with_bug()
          subscribers = get_structural_subscribers(bug, None, None, None)
--        self.assertIsInstance(subscribers, (ResultSet, EmptyResultSet))
++        self.assertIsInstance(subscribers, RESULT_SETS)
          self.assertEqual([], list(subscribers))
      def test_getStructuralSubscribers_single_target(self):
@@ -678,7 +776,7 @@
          bug.addTask(actor, product2)
          subscribers = get_structural_subscribers(bug, None, None, None)
--        self.assertIsInstance(subscribers, ResultSet)
++        self.assertIsInstance(subscribers, RESULT_SETS)
          self.assertEqual(set([subscriber1, subscriber2]), set(subscribers))
      def test_getStructuralSubscribers_recipients(self):