Merge lp:~gary/launchpad/bug723999-2c into lp:launchpad

[Gary Poster]

This branch is slightly meatier than than the previous two in the series, and we're starting to see the light at the end of the tunnel.

This branch makes the following changes.

- I factored the code of bug.getAlsoNotifiedSubscribers into a function, get_also_notified_subscribers. The method now calls the function.
- I made the function accept a bugtask or a bug, rather than only a bug. This meant that I could eliminate an essentially identical function of lp.bugs.subscribers.bug.get_bugtask_indirect_subscribers and reuse the function. I changed the only test of the duplicate function to use the newly shared one.
- I made the get_also_notified_subscribers function proxied so that it returns proxied objects. This maintains the behavior of calling utility methods that are available to everyone, so it is a pattern I'm following in the cours eof this work. You'll see I actually verify the expected behavior of the proxy in some small test changes of the code formerly for get_bugtask_indirect_subscribers.
- I made the get_also_notified_subscribers function handle direct subscriptions more carefully.
- I made the get_also_notified_subscribers function use the relatively new structuralsubscription function get_structural_subscribers directly, rather than go through the middleman of the IBugTaskSet method. As you might guess, one of my upcoming branches will remove that IBugTaskSet method and repoint the pertinent tests.
- I made get_structural_subscribers and friends accept a direct_subscribers argument. If they have already been calculated, as they have in get_also_notified_subscribers, let's use them, and make our queries faster!
- I pulled out two functions from the _get_structural_subscription_filter_id_query function, _calculate_bugtask_condition and _calculate_tag_query. The only point of the refactoring at this time is to try to make the code more understandable by dividing up responsibilities a bit. This is my attempt to respond to jcsackett's review of my original bug723999 branch. If you don't find these changes more comprehensible, I've failed.

Note that I tried to make lint completely happy (particularly notable is that lint is what caused me to move the XXX comment in lib/lp/bugs/interfaces/bug.py), but one complaint left me at a loss.

./lib/lp/bugs/interfaces/bug.py
     435: E301 expected 1 blank line, found 0

Line 435 is the one below beginning with "@operation_parameters":

...
    def newMessage(owner, subject, content):
        """Create a new message, and link it to this object."""

    @operation_parameters(
        person=Reference(IPerson, title=_('Person'), required=True),
...

I don't see what the problem is. I've decided to ignore it.

Thank you!

Gary

[Benji York (benji)]

This branch is good to go. The descriptive and thorough introduction
helped quite a bit in understanding the changes.

As for the lint output, I determined that the comment embedded in the
decorator call is the culprit. I suggest filing a bug against
pocketlint and/or whichever Python linter it's using.

review: Approve (code)

=== modified file 'lib/lp/bugs/doc/bugsubscription.txt'
--- lib/lp/bugs/doc/bugsubscription.txt	2011-02-21 14:32:15 +0000
+++ lib/lp/bugs/doc/bugsubscription.txt	2011-03-07 20:07:18 +0000
@@ -106,10 +106,17 @@
 It is also possible to get the list of indirect subscribers for an
 individual bug task.
 
-    >>> from lp.bugs.subscribers.bug import get_bugtask_indirect_subscribers
-    >>> get_bugtask_indirect_subscribers(linux_source_bug.bugtasks[0])
+    >>> from lp.bugs.model.bug import get_also_notified_subscribers
+    >>> res = get_also_notified_subscribers(linux_source_bug.bugtasks[0])
+    >>> res
     [<Person at ...>]
 
+These are security proxied.
+
+    >>> from zope.security. proxy import Proxy
+    >>> isinstance(res, Proxy)
+    True
+
 The list of all bug subscribers can also be accessed via
 IBugTask.bug_subscribers. Our event handling machinery compares a
 "snapshot" of this value, before a bug was changed, to the current

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2011-03-07 17:54:44 +0000
+++ lib/lp/bugs/interfaces/bug.py	2011-03-07 20:07:18 +0000
@@ -506,7 +506,7 @@
         dupes, etc.
         """
 
-    def getAlsoNotifiedSubscribers():
+    def getAlsoNotifiedSubscribers(recipients=None, level=None):
         """Return IPersons in the "Also notified" subscriber list.
 
         This includes bug contacts and assignees, but not subscribers
@@ -555,7 +555,7 @@
 
     def addCommentNotification(message, recipients=None, activity=None):
         """Add a bug comment notification.
-        
+
         If a BugActivity instance is provided as an `activity`, it is linked
         to the notification."""
 
@@ -1162,6 +1162,9 @@
         """
 
     def dangerousGetAllBugs():
+        # XXX 2010-01-08 gmb bug=505850:
+        #     Note, this method should go away when we have a proper
+        #     permissions system for scripts.
         """DO NOT CALL THIS METHOD.
 
         This method exists solely to allow the bug heat script to grab
@@ -1170,9 +1173,6 @@
         DOING. AND IF YOU KNOW WHAT YOU'RE DOING YOU KNOW BETTER THAN TO
         USE THIS ANYWAY.
         """
-        # XXX 2010-01-08 gmb bug=505850:
-        #     Note, this method should go away when we have a proper
-        #     permissions system for scripts.
 
     def getBugsWithOutdatedHeat(max_heat_age):
         """Return the set of bugs whose heat is out of date.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-03-07 17:54:44 +0000
+++ lib/lp/bugs/model/bug.py	2011-03-07 20:07:18 +0000
@@ -14,6 +14,7 @@
     'BugSet',
     'BugTag',
     'FileBugData',
+    'get_also_notified_subscribers',
     'get_bug_tags',
     'get_bug_tags_open_count',
     ]
@@ -70,7 +71,10 @@
     implements,
     providedBy,
     )
-from zope.security.proxy import removeSecurityProxy
+from zope.security.proxy import (
+    ProxyFactory,
+    removeSecurityProxy,
+    )
 
 from canonical.config import config
 from canonical.database.constants import UTC_NOW
@@ -144,6 +148,7 @@
 from lp.bugs.interfaces.bugnotification import IBugNotificationSet
 from lp.bugs.interfaces.bugtask import (
     BugTaskStatus,
+    IBugTask,
     IBugTaskSet,
     UNRESOLVED_BUGTASK_STATUSES,
     )
@@ -168,6 +173,7 @@
 from lp.bugs.model.bugwatch import BugWatch
 from lp.bugs.model.structuralsubscription import (
     get_all_structural_subscriptions,
+    get_structural_subscribers,
     )
 from lp.hardwaredb.interfaces.hwdb import IHWSubmissionBugSet
 from lp.registry.interfaces.distribution import IDistribution
@@ -982,50 +988,7 @@
         See the comment in getDirectSubscribers for a description of the
         recipients argument.
         """
-        if self.private:
-            return []
-
-        also_notified_subscribers = set()
-
-        for bugtask in self.bugtasks:
-            if bugtask.assignee:
-                also_notified_subscribers.add(bugtask.assignee)
-                if recipients is not None:
-                    recipients.addAssignee(bugtask.assignee)
-
-            # If the target's bug supervisor isn't set,
-            # we add the owner as a subscriber.
-            pillar = bugtask.pillar
-            if pillar.bug_supervisor is None and pillar.official_malone:
-                    also_notified_subscribers.add(pillar.owner)
-                    if recipients is not None:
-                        recipients.addRegistrant(pillar.owner, pillar)
-
-        # Structural subscribers.
-        if recipients is None:
-            temp_recipients = None
-        else:
-            temp_recipients = BugNotificationRecipients(
-                duplicateof=recipients.duplicateof)
-        also_notified_subscribers.update(
-            getUtility(IBugTaskSet).getStructuralSubscribers(
-                self.bugtasks, recipients=temp_recipients, level=level))
-
-        # Direct subscriptions always take precedence over indirect
-        # subscriptions.
-        direct_subscribers = set(self.getDirectSubscribers())
-        if recipients is not None:
-            # A direct subscriber may have muted this notification.
-            # Therefore, we want to remove any direct subscribers from the
-            # structural subscription recipients before we merge.
-            temp_recipients.remove(direct_subscribers)
-            recipients.update(temp_recipients)
-
-        # Remove security proxy for the sort key, but return
-        # the regular proxied object.
-        return sorted(
-            (also_notified_subscribers - direct_subscribers),
-            key=lambda x: removeSecurityProxy(x).displayname)
+        return get_also_notified_subscribers(self, recipients, level)
 
     def getBugNotificationRecipients(self, duplicateof=None, old_bug=None,
                                      level=None,
@@ -2011,6 +1974,66 @@
             operator.itemgetter(0))
 
 
+@ProxyFactory
+def get_also_notified_subscribers(
+    bug_or_bugtask, recipients=None, level=None):
+    """Return the indirect subscribers for a bug or bug task.
+
+    Return the list of people who should get notifications about changes
+    to the bug or task because of having an indirect subscription
+    relationship with it (by subscribing to a target, being an assignee
+    or owner, etc...)
+
+    If `recipients` is present, add the subscribers to the set of
+    bug notification recipients.
+    """
+    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')
+
+    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:
+                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)
+
+    # This structural subscribers code omits direct subscribers itself.
+    also_notified_subscribers.update(
+        get_structural_subscribers(
+            bug_or_bugtask, recipients, level, direct_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)
+
+
 def load_people(*where):
     """Get subscribers from subscriptions.
 

=== modified file 'lib/lp/bugs/model/structuralsubscription.py'
--- lib/lp/bugs/model/structuralsubscription.py	2011-03-07 20:07:17 +0000
+++ lib/lp/bugs/model/structuralsubscription.py	2011-03-07 20:07:18 +0000
@@ -536,13 +536,16 @@
         *conditions)
 
 
-def get_structural_subscribers(bug_or_bugtask, recipients, level):
+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."""
@@ -559,7 +562,8 @@
     # qastaging and production, _get_structural_subscription_filter_id_query
     # can just return the query, and leave the candidates out of it.
     candidates, filter_id_query = (
-        _get_structural_subscription_filter_id_query(bug, bugtasks, level))
+        _get_structural_subscription_filter_id_query(
+            bug, bugtasks, level, direct_subscribers))
     if not candidates:
         return EmptyResultSet()
     # This is here because of a circular import.
@@ -649,16 +653,17 @@
     oper = "&&"
 
 
-def _get_structural_subscription_filter_id_query(bug, bugtasks, level):
+def _get_structural_subscription_filter_id_query(
+    bug, bugtasks, level, direct_subscribers):
     """Helper function.
 
-    This provides the core implementation for
-    get_structural_subscribers_for_bug and
-    get_structural_subscribers_for_bugtask.
+    This provides the core implementation for get_structural_subscribers.
 
     :param bug: a bug.
     :param bugtasks: an iterable of one or more bugtasks of the bug.
     :param level: a notification level.
+    :param direct_subscribers: a collection of Person objects who are
+                               directly subscribed to the bug.
     """
     # We get the ids because we need to use group by in order to
     # look at the filters' tags in aggregate.  Once we have the ids,
@@ -676,18 +681,26 @@
     # See the docstring of get_structural_subscription_targets.
     query_arguments = list(
         get_structural_subscription_targets(bugtasks))
-    assert len(query_arguments) > 0, (
-        'Programmer error: expected query arguments')
+    if not query_arguments:
+        # We have no bugtasks.
+        return None, None
     # With large numbers of filters in the system, it's fastest in our
     # tests if we get a set of structural subscriptions pertinent to the
     # given targets, and then work with that.  It also comes in handy
     # when we have to do a union, because we can share the work across
     # the two queries.
     # We will exclude people who have a direct subscription to the bug.
-    filters = [
-        Not(In(StructuralSubscription.subscriberID,
-               Select(BugSubscription.person_id,
-                      BugSubscription.bug == bug)))]
+    filters = []
+    if direct_subscribers is not None:
+        if direct_subscribers:
+            filters.append(
+                Not(In(StructuralSubscription.subscriberID,
+                       tuple(person.id for person in direct_subscribers))))
+    else:
+        filters.append(
+            Not(In(StructuralSubscription.subscriberID,
+                   Select(BugSubscription.person_id,
+                          BugSubscription.bug == bug))))
     candidates = _get_all_structural_subscriptions(
         StructuralSubscription.id, query_arguments, *filters)
     if not candidates:
@@ -695,25 +708,21 @@
         # then we don't need to look at the importance, status, and
         # tags.  We're done.
         return None, None
-    # The "select_args" dictionary holds the arguments that we will
-    # pass to one or more SELECT clauses.  We start with what will
-    # become the FROM clause.  We always want the following Joins,
-    # so we can add them here at the beginning.
-    select_args = {
-        'tables': [
-            StructuralSubscription,
-            Join(BugSubscriptionFilter,
-                 BugSubscriptionFilter.structural_subscription_id ==
-                 StructuralSubscription.id),
-            LeftJoin(BugSubscriptionFilterStatus,
-                     BugSubscriptionFilterStatus.filter_id ==
-                     BugSubscriptionFilter.id),
-            LeftJoin(BugSubscriptionFilterImportance,
-                     BugSubscriptionFilterImportance.filter_id ==
-                     BugSubscriptionFilter.id),
-            LeftJoin(BugSubscriptionFilterTag,
-                     BugSubscriptionFilterTag.filter_id ==
-                     BugSubscriptionFilter.id)]}
+    # These are tables and joins we will want.
+    tables = [
+        StructuralSubscription,
+        Join(BugSubscriptionFilter,
+             BugSubscriptionFilter.structural_subscription_id ==
+             StructuralSubscription.id),
+        LeftJoin(BugSubscriptionFilterStatus,
+                 BugSubscriptionFilterStatus.filter_id ==
+                 BugSubscriptionFilter.id),
+        LeftJoin(BugSubscriptionFilterImportance,
+                 BugSubscriptionFilterImportance.filter_id ==
+                 BugSubscriptionFilter.id),
+        LeftJoin(BugSubscriptionFilterTag,
+                 BugSubscriptionFilterTag.filter_id ==
+                 BugSubscriptionFilter.id)]
     # The "conditions" list will eventually be passed to a Storm
     # "And" function, and then become the WHERE clause of our SELECT.
     conditions = [In(StructuralSubscription.id, candidates)]
@@ -721,14 +730,35 @@
     if level is not None:
         conditions.append(
             BugSubscriptionFilter.bug_notification_level >= level)
-    # Now we handle importance and status, which are per bugtask.
+    # This handles the bugtask-specific attributes of status and importance.
+    conditions.append(_calculate_bugtask_condition(query_arguments))
+    # Now we handle tags.  This actually assembles the query, because it
+    # may have to union two queries together.
+    # Note that casting bug.tags to a list subtly removes the security
+    # proxy on the list.  Strings are never security-proxied, so we
+    # don't have to worry about them.
+    query = _calculate_tag_query(tables, conditions, list(bug.tags))
+    # XXX gary 2011-03-03 bug 728818
+    # Once we no longer have structural subscriptions without filters in
+    # qastaging and production, _get_structural_subscription_filter_id_query
+    # can just return the query, and leave the candidates out of it.
+    return candidates, query
+
+
+def _calculate_bugtask_condition(query_arguments):
+    """Return a condition matching importance and status for the bugtasks.
+
+    :param query_arguments: an iterable of (bugtask, target) pairs, as
+                            returned by get_structural_subscription_targets.
+    """
+    # This handles importance and status, which are per bugtask.
     # What we do is loop through the collection of bugtask, target
     # in query_arguments.  Each bugtask will have one or more
     # targets that we have to check.  We figure out how to describe each
     # target using the useful IStructuralSubscriptionTargetHelper
     # adapter, which has a "join" attribute on it that tells us how
     # to distinguish that target.  Once we have all of the target
-    # descriptins, we OR those together, and say that the filters
+    # descriptions, we OR those together, and say that the filters
     # for those targets must either have no importance or match the
     # associated bugtask's importance; and have no status or match
     # the bugtask's status.  Once we have looked at all of the
@@ -766,33 +796,40 @@
     # We know there will be at least one bugtask, because we already
     # escaped early "if len(query_arguments) == 0".
     handle_bugtask_conditions(bugtask)
-    conditions.append(Or(*outer_or_conditions))
-    # Now we handle tags.  If the bug has no tags, this is
-    # relatively easy. Otherwise, not so much.
-    tags = list(bug.tags) # This subtly removes the security proxy on
-    # the list.  Strings are never security-proxied, so we don't have
-    # to worry about them.
+    return Or(*outer_or_conditions)
+
+
+def _calculate_tag_query(tables, conditions, tags):
+    """Determine tag-related conditions and assemble a query.
+
+    :param tables: the tables and joins to use in the query.
+    :param conditions: the other conditions that constrain the query.
+    :param tags: the list of tags that the bug has.
+    """
+    # If the bug has no tags, this is relatively easy. Otherwise, not so
+    # much.
     if len(tags) == 0:
         # The bug has no tags.  We should leave out filters that
         # require any generic non-empty set of tags
         # (BugSubscriptionFilter.include_any_tags), which we do with
-        # the conditions.  Then we can finish up the WHERE clause.
-        # Then we have to make sure that the filter does not require
-        # any *specific* tags. We do that with a GROUP BY on the
-        # filters, and then a HAVING clause that aggregates the
-        # BugSubscriptionFilterTags that are set to "include" the
-        # tag.  (If it is not an include, that is an exclude, and a
-        # bug without tags will not have a particular tag, so we can
-        # ignore those in this case.)  This requires a CASE
-        # statement within the COUNT.  After this, we are done, and
-        # we return the fully formed SELECT query object.
+        # the conditions.
         conditions.append(Not(BugSubscriptionFilter.include_any_tags))
-        select_args['where'] = And(*conditions)
-        select_args['group_by'] = (BugSubscriptionFilter.id,)
-        select_args['having'] = Count(
-            SQL('CASE WHEN BugSubscriptionFilterTag.include '
-                'THEN BugSubscriptionFilterTag.tag END'))==0
-        return candidates, Select(BugSubscriptionFilter.id, **select_args)
+        return Select(
+            BugSubscriptionFilter.id,
+            tables=tables,
+            where=And(*conditions),
+            # We have to make sure that the filter does not require
+            # any *specific* tags. We do that with a GROUP BY on the
+            # filters, and then a HAVING clause that aggregates the
+            # BugSubscriptionFilterTags that are set to "include" the
+            # tag.  (If it is not an include, that is an exclude, and a
+            # bug without tags will not have a particular tag, so we can
+            # ignore those in this case.)  This requires a CASE
+            # statement within the COUNT.
+            group_by=(BugSubscriptionFilter.id,),
+            having=Count(
+                SQL('CASE WHEN BugSubscriptionFilterTag.include '
+                    '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
@@ -809,42 +846,32 @@
         # involve another separate Postgres call, and I think that
         # we've already gotten the big win by separating out the
         # structural subscriptions into "candidates," above.
-        #
-        # So, up to now we've been assembling the things that are shared
-        # between the two queries, but now we start working on the
-        # differences between the two unioned queries. "first_select"
-        # will hold one set of arguments, and select_args will hold the
-        # other.
-        first_select = select_args.copy()
+        # When Storm supports the WITH statement, we should reconsider
+        # this (bug 729134).
         # As mentioned, in this first SELECT we handle filters that
         # match any of the filter's tags.  This can be a relatively
         # straightforward query--we just need a bit more added to
         # our WHERE clause, and we don't need a GROUP BY/HAVING.
-        first_select['where'] = And(
-            Or(# We want filters that proclaim they simply want any tags.
-               BugSubscriptionFilter.include_any_tags,
-               # Also include filters that match any tag...
-               And(Not(BugSubscriptionFilter.find_all_tags),
-                   Or(# ...with a positive match...
-                      And(BugSubscriptionFilterTag.include,
-                          In(BugSubscriptionFilterTag.tag, tags)),
-                      # ...or with a negative match...
-                      And(Not(BugSubscriptionFilterTag.include),
-                          Not(In(BugSubscriptionFilterTag.tag, tags))),
-                      # ...or if the filter does not specify any tags.
-                      BugSubscriptionFilterTag.tag == None))),
-            *conditions)
-        first_select = Select(BugSubscriptionFilter.id, **first_select)
+        first_select = Select(
+            BugSubscriptionFilter.id,
+            tables=tables,
+            where=And(
+                Or(# We want filters that proclaim they simply want any tags.
+                   BugSubscriptionFilter.include_any_tags,
+                   # Also include filters that match any tag...
+                   And(Not(BugSubscriptionFilter.find_all_tags),
+                       Or(# ...with a positive match...
+                          And(BugSubscriptionFilterTag.include,
+                              In(BugSubscriptionFilterTag.tag, tags)),
+                          # ...or with a negative match...
+                          And(Not(BugSubscriptionFilterTag.include),
+                              Not(In(BugSubscriptionFilterTag.tag, tags))),
+                          # ...or if the filter does not specify any tags.
+                          BugSubscriptionFilterTag.tag == None))),
+                *conditions))
         # We have our first clause.  Now we start on the second one:
-        # handling filters that match *all* tags. Our WHERE clause
-        # is straightforward and, it should be clear that we are
-        # simply focusing on BugSubscriptionFilter.find_all_tags,
-        # when the first SELECT did not consider it.
-        select_args['where'] = And(
-            BugSubscriptionFilter.find_all_tags, *conditions)
-        # The GROUP BY collects the filters together.
-        select_args['group_by'] = (BugSubscriptionFilter.id,)
-        # Now it is time for the HAVING clause, which is where some
+        # handling filters that match *all* tags.
+        # This second query will have a HAVING clause, which is where some
         # tricky bits happen. We first make a SQL snippet that
         # represents the tags on this bug.  It is straightforward
         # except for one subtle hack: the addition of the empty
@@ -868,39 +895,43 @@
         # clearest alternative is defining a custom Postgres aggregator.
         tags_array = "ARRAY[%s,' ']::TEXT[]" % ",".join(
             quote(tag) for tag in tags)
-        # Now comes the HAVING clause itself.
-        select_args['having'] = And(
-            # The list of tags should be a superset of the filter tags to
-            # be included.
-            ArrayContains(
-                SQL(tags_array),
-                # This next line gives us an array of the tags that the
-                # filter wants to include.  Notice that it includes the
-                # empty string when the condition does not match, per the
-                # discussion above.
-                ArrayAgg(
-                   SQL("CASE WHEN BugSubscriptionFilterTag.include "
-                       "THEN BugSubscriptionFilterTag.tag "
-                       "ELSE ' '::TEXT END"))),
-            # The list of tags should also not intersect with the
-            # tags that the filter wants to exclude.
-            Not(
-                ArrayIntersects(
+        # Now let's build the select itself.
+        second_select = Select(
+            BugSubscriptionFilter.id,
+            tables=tables,
+            # Our WHERE clause is straightforward. We are simply
+            # focusing on BugSubscriptionFilter.find_all_tags, when the
+            # first SELECT did not consider it.
+            where=And(BugSubscriptionFilter.find_all_tags, *conditions),
+            # The GROUP BY collects the filters together.
+            group_by=(BugSubscriptionFilter.id,),
+            having=And(
+                # The list of tags should be a superset of the filter tags to
+                # be included.
+                ArrayContains(
                     SQL(tags_array),
-                    # This next line gives us an array of the tags
-                    # that the filter wants to exclude.  We do not bother
-                    # with the empty string, and therefore allow NULLs
-                    # into the array, because in this case we are
-                    # determining whether the sets intersect, not if the
-                    # first set subsumes the second.
+                    # This next line gives us an array of the tags that the
+                    # filter wants to include.  Notice that it includes the
+                    # empty string when the condition does not match, per the
+                    # discussion above.
                     ArrayAgg(
-                       SQL('CASE WHEN '
-                           'NOT BugSubscriptionFilterTag.include '
-                           'THEN BugSubscriptionFilterTag.tag END')))))
-        # Everything is ready.  Make our second SELECT statement, UNION
-        # it, and return it.
-        return candidates, Union(
-            first_select,
-            Select(
-                BugSubscriptionFilter.id,
-                **select_args))
+                       SQL("CASE WHEN BugSubscriptionFilterTag.include "
+                           "THEN BugSubscriptionFilterTag.tag "
+                           "ELSE ' '::TEXT END"))),
+                # The list of tags should also not intersect with the
+                # tags that the filter wants to exclude.
+                Not(
+                    ArrayIntersects(
+                        SQL(tags_array),
+                        # This next line gives us an array of the tags
+                        # that the filter wants to exclude.  We do not bother
+                        # with the empty string, and therefore allow NULLs
+                        # into the array, because in this case we are
+                        # determining whether the sets intersect, not if the
+                        # first set subsumes the second.
+                        ArrayAgg(
+                           SQL('CASE WHEN '
+                               'NOT BugSubscriptionFilterTag.include '
+                               'THEN BugSubscriptionFilterTag.tag END'))))))
+        # Everything is ready.  Return the union.
+        return Union(first_select, second_select)

=== modified file 'lib/lp/bugs/subscribers/bug.py'
--- lib/lp/bugs/subscribers/bug.py	2011-02-11 22:04:25 +0000
+++ lib/lp/bugs/subscribers/bug.py	2011-03-07 20:07:18 +0000
@@ -5,7 +5,6 @@
 __all__ = [
     'add_bug_change_notifications',
     'get_bug_delta',
-    'get_bugtask_indirect_subscribers',
     'notify_bug_attachment_added',
     'notify_bug_attachment_removed',
     'notify_bug_comment_added',
@@ -16,9 +15,6 @@
 
 
 import datetime
-from operator import attrgetter
-
-from zope.component import getUtility
 
 from canonical.config import config
 from canonical.database.sqlbase import block_implicit_flushes
@@ -35,10 +31,10 @@
     )
 from lp.bugs.adapters.bugdelta import BugDelta
 from lp.bugs.enum import BugNotificationLevel
-from lp.bugs.interfaces.bugtask import IBugTaskSet
 from lp.bugs.mail.bugnotificationbuilder import BugNotificationBuilder
 from lp.bugs.mail.bugnotificationrecipients import BugNotificationRecipients
 from lp.bugs.mail.newbug import generate_bug_add_email
+from lp.bugs.model.bug import get_also_notified_subscribers
 from lp.registry.interfaces.person import IPerson
 
 
@@ -146,46 +142,6 @@
         return None
 
 
-def get_bugtask_indirect_subscribers(bugtask, recipients=None, level=None):
-    """Return the indirect subscribers for a bug task.
-
-    Return the list of people who should get notifications about
-    changes to the task because of having an indirect subscription
-    relationship with it (by subscribing to its target, being an
-    assignee or owner, etc...)
-
-    If `recipients` is present, add the subscribers to the set of
-    bug notification recipients.
-    """
-    if bugtask.bug.private:
-        return set()
-
-    also_notified_subscribers = set()
-
-    # Assignees are indirect subscribers.
-    if bugtask.assignee:
-        also_notified_subscribers.add(bugtask.assignee)
-        if recipients is not None:
-            recipients.addAssignee(bugtask.assignee)
-
-    # Get structural subscribers.
-    also_notified_subscribers.update(
-        getUtility(IBugTaskSet).getStructuralSubscribers(
-            [bugtask], recipients, level))
-
-    # If the target's bug supervisor isn't set,
-    # we add the owner as a subscriber.
-    pillar = bugtask.pillar
-    if pillar.bug_supervisor is None and pillar.official_malone:
-        also_notified_subscribers.add(pillar.owner)
-        if recipients is not None:
-            recipients.addRegistrant(pillar.owner, pillar)
-
-    return sorted(
-        also_notified_subscribers,
-        key=attrgetter('displayname'))
-
-
 def add_bug_change_notifications(bug_delta, old_bugtask=None,
                                  new_subscribers=None):
     """Generate bug notifications and add them to the bug."""
@@ -195,7 +151,7 @@
         level=BugNotificationLevel.METADATA)
     if old_bugtask is not None:
         old_bugtask_recipients = BugNotificationRecipients()
-        get_bugtask_indirect_subscribers(
+        get_also_notified_subscribers(
             old_bugtask, recipients=old_bugtask_recipients,
             level=BugNotificationLevel.METADATA)
         recipients.update(old_bugtask_recipients)