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

[Gary Poster]

This MP is one of several I'll be making to clean up my work on bug723999. I'm dividing up the work to try and make it easier to review.

This branch is about removing the getSubscriptionsForBugTask method on bug targets. It only had existed after my previous work on bug723999 because of tests that I didn't change because of branch size issues earlier.

I replace it with the get_structural_subscribers function. The get_structural_subscribers_for_bugtasks function remains as a wrapper for get_structural_subscribers (again, because of branch review size limitations); it will disappear in subsequent branches, leaving only get_structural_subscribers. As you can tell from the wrapper function, we only use this functionality in LP by either passing a list of a single bugtask, or all of the bugtasks in a bug. I'll be changing the rest of LP to explicitly do that in a future branch.

I adapted the tests to use the new function. Unfortunately, they are in the wrong module, because this code no longer has anything to do with targets directly. I will move the tests to test_structuralsubscription.py in a subsequent branch.

make lint is happy.

Thank you

Gary

[Benji York (benji)]

This branch looks fine, being primarily a mechanical transformation from one perspective to another.

review: Approve (code)

=== modified file 'lib/lp/bugs/interfaces/structuralsubscription.py'
--- lib/lp/bugs/interfaces/structuralsubscription.py	2011-02-04 06:30:13 +0000
+++ lib/lp/bugs/interfaces/structuralsubscription.py	2011-03-07 15:46:18 +0000
@@ -14,10 +14,6 @@
     'IStructuralSubscriptionTargetHelper',
     ]
 
-from lazr.enum import (
-    DBEnumeratedType,
-    DBItem,
-    )
 from lazr.restful.declarations import (
     call_with,
     export_as_webservice_entry,
@@ -41,7 +37,6 @@
     )
 from zope.schema import (
     Bool,
-    Choice,
     Datetime,
     Int,
     )
@@ -149,9 +144,6 @@
     def userHasBugSubscriptions(user):
         """Is `user` subscribed, directly or via a team, to bug mail?"""
 
-    def getSubscriptionsForBugTask(bug, level):
-        """Return subscriptions for a given `IBugTask` at `level`."""
-
 
 class IStructuralSubscriptionTargetWrite(Interface):
     """A Launchpad Structure allowing users to subscribe to it.
 
=== modified file 'lib/lp/bugs/model/structuralsubscription.py'
--- lib/lp/bugs/model/structuralsubscription.py	2011-03-04 02:29:09 +0000
+++ lib/lp/bugs/model/structuralsubscription.py	2011-03-07 15:46:18 +0000
@@ -4,6 +4,7 @@
 __metaclass__ = type
 __all__ = [
     'get_all_structural_subscriptions',
+    'get_structural_subscribers',
     'get_structural_subscribers_for_bugtasks',
     'get_structural_subscription_targets',
     'StructuralSubscription',
@@ -47,6 +48,8 @@
 from canonical.database.sqlbase import quote
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.interfaces.lpstorm import IStore
+from lp.bugs.interfaces.bug import IBug
+from lp.bugs.interfaces.bugtask import IBugTask
 from lp.bugs.interfaces.structuralsubscription import (
     IStructuralSubscription,
     IStructuralSubscriptionTarget,
@@ -476,24 +479,6 @@
                     return True
         return False
 
-    def getSubscriptionsForBugTask(self, bugtask, level):
-        """See `IStructuralSubscriptionTarget`."""
-        # Note that this method does not take into account
-        # structural subscriptions without filters.  Since it is only
-        # used for tests at this point, that's not a problem; moreover,
-        # we intend all structural subscriptions to have filters.
-        candidates, filter_id_query = (
-            _get_structural_subscription_filter_id_query(
-                bugtask.bug, [bugtask], level))
-        if not candidates:
-            return EmptyResultSet()
-        return IStore(StructuralSubscription).find(
-            StructuralSubscription,
-            BugSubscriptionFilter.structural_subscription_id ==
-            StructuralSubscription.id,
-            In(BugSubscriptionFilter.id,
-               filter_id_query)).config(distinct=True)
-
 
 def get_structural_subscription_targets(bugtasks):
     """Return (bugtask, target) pairs for each target of the bugtasks.
@@ -551,7 +536,30 @@
         *conditions)
 
 
-def _get_structural_subscribers(candidates, filter_id_query, recipients):
+def get_structural_subscribers(bug_or_bugtask, recipients, level):
+    """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.
+
+    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')
+    # 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.
+    candidates, filter_id_query = (
+        _get_structural_subscription_filter_id_query(bug, bugtasks, level))
     if not candidates:
         return EmptyResultSet()
     # This is here because of a circular import.
@@ -601,22 +609,26 @@
                                             level=None):
     """Return subscribers for structural filters for the bugtasks at "level".
 
-    :param bugtasks: an iterable of bugtasks.  All must be for the same bug.
+    :param bugtasks: an iterable of bugtasks.  Should be a single bugtask, or
+                     all of the bugtasks from one bug.
     :param recipients: a BugNotificationRecipients object or None.
                        Populates if given.
     :param level: a level from lp.bugs.enum.BugNotificationLevel.
 
     Excludes structural subscriptions for people who are directly subscribed
     to the bug."""
+    bugtasks = list(bugtasks)
     if not bugtasks:
         return EmptyResultSet()
-    bugs = set(bugtask.bug for bugtask in bugtasks)
-    if len(bugs) > 1:
-        raise NotImplementedError('Each bugtask must be from the same bug.')
-    bug = bugs.pop()
-    candidates, query = _get_structural_subscription_filter_id_query(
-        bug, bugtasks, level)
-    return _get_structural_subscribers(candidates, query, recipients)
+    if len(bugtasks) == 1:
+        target = bugtasks[0]
+    else:
+        target = bugtasks[0].bug
+        if target.bugtasks != bugtasks:
+            raise NotImplementedError(
+                'bugtasks must be one, or the full set of bugtasks from one '
+                'bug')
+    return get_structural_subscribers(target, recipients, level)
 
 
 class ArrayAgg(NamedFunc):
 
=== modified file 'lib/lp/bugs/tests/test_structuralsubscriptiontarget.py'
--- lib/lp/bugs/tests/test_structuralsubscriptiontarget.py	2011-03-01 18:58:40 +0000
+++ lib/lp/bugs/tests/test_structuralsubscriptiontarget.py	2011-03-07 15:46:18 +0000
@@ -37,7 +37,10 @@
     IStructuralSubscriptionTarget,
     IStructuralSubscriptionTargetHelper,
     )
-from lp.bugs.model.structuralsubscription import StructuralSubscription
+from lp.bugs.model.structuralsubscription import (
+    get_structural_subscribers,
+    StructuralSubscription,
+    )
 from lp.bugs.tests.test_bugtarget import bugtarget_filebug
 from lp.registry.errors import (
     DeleteSubscriptionError,
@@ -194,98 +197,98 @@
             self.ordinary_subscriber, self.ordinary_subscriber)
         self.initial_filter = self.subscription.bug_filters[0]
 
-    def assertSubscriptions(
-        self, expected_subscriptions, level=BugNotificationLevel.NOTHING):
-        observed_subscriptions = list(
-            self.target.getSubscriptionsForBugTask(self.bugtask, level))
-        self.assertEqual(expected_subscriptions, observed_subscriptions)
+    def assertSubscribers(
+        self, expected_subscribers, level=BugNotificationLevel.NOTHING):
+        observed_subscribers = list(
+            get_structural_subscribers(self.bugtask, None, level))
+        self.assertEqual(expected_subscribers, observed_subscribers)
 
-    def test_getSubscriptionsForBugTask(self):
+    def test_getStructuralSubscribers(self):
         # If no one has a filtered subscription for the given bug, the result
-        # of getSubscriptionsForBugTask() is the same as for
-        # getSubscriptions().
+        # of get_structural_subscribers() is the same as for
+        # the set of people from each subscription in getSubscriptions().
         subscriptions = self.target.getSubscriptions()
-        self.assertSubscriptions(list(subscriptions))
+        self.assertSubscribers([sub.subscriber for sub in subscriptions])
 
-    def test_getSubscriptionsForBugTask_with_filter_on_status(self):
+    def test_getStructuralSubscribers_with_filter_on_status(self):
         # If a status filter exists for a subscription, the result of
-        # getSubscriptionsForBugTask() may be a subset of getSubscriptions().
+        # get_structural_subscribers() may be a subset of getSubscriptions().
 
         # Without any filters the subscription is found.
-        self.assertSubscriptions([self.subscription])
+        self.assertSubscribers([self.ordinary_subscriber])
 
         # Filter the subscription to bugs in the CONFIRMED state.
         self.initial_filter.statuses = [BugTaskStatus.CONFIRMED]
 
         # With the filter the subscription is not found.
-        self.assertSubscriptions([])
+        self.assertSubscribers([])
 
         # If the filter is adjusted, the subscription is found again.
         self.initial_filter.statuses = [self.bugtask.status]
-        self.assertSubscriptions([self.subscription])
+        self.assertSubscribers([self.ordinary_subscriber])
 
-    def test_getSubscriptionsForBugTask_with_filter_on_importance(self):
+    def test_getStructuralSubscribers_with_filter_on_importance(self):
         # If an importance filter exists for a subscription, the result of
-        # getSubscriptionsForBugTask() may be a subset of getSubscriptions().
+        # get_structural_subscribers() may be a subset of getSubscriptions().
 
         # Without any filters the subscription is found.
-        self.assertSubscriptions([self.subscription])
+        self.assertSubscribers([self.ordinary_subscriber])
 
         # Filter the subscription to bugs in the CRITICAL state.
         self.initial_filter.importances = [BugTaskImportance.CRITICAL]
 
         # With the filter the subscription is not found.
-        self.assertSubscriptions([])
+        self.assertSubscribers([])
 
         # If the filter is adjusted, the subscription is found again.
         self.initial_filter.importances = [self.bugtask.importance]
-        self.assertSubscriptions([self.subscription])
+        self.assertSubscribers([self.ordinary_subscriber])
 
-    def test_getSubscriptionsForBugTask_with_filter_on_level(self):
+    def test_getStructuralSubscribers_with_filter_on_level(self):
         # All structural subscriptions have a level for bug notifications
-        # which getSubscriptionsForBugTask() observes.
+        # which get_structural_subscribers() observes.
 
         # Adjust the subscription level to METADATA.
         self.initial_filter.bug_notification_level = (
             BugNotificationLevel.METADATA)
 
         # The subscription is found when looking for NOTHING or above.
-        self.assertSubscriptions(
-            [self.subscription], BugNotificationLevel.NOTHING)
+        self.assertSubscribers(
+            [self.ordinary_subscriber], BugNotificationLevel.NOTHING)
         # The subscription is found when looking for METADATA or above.
-        self.assertSubscriptions(
-            [self.subscription], BugNotificationLevel.METADATA)
+        self.assertSubscribers(
+            [self.ordinary_subscriber], BugNotificationLevel.METADATA)
         # The subscription is not found when looking for COMMENTS or above.
-        self.assertSubscriptions(
+        self.assertSubscribers(
             [], BugNotificationLevel.COMMENTS)
 
-    def test_getSubscriptionsForBugTask_with_filter_include_any_tags(self):
+    def test_getStructuralSubscribers_with_filter_include_any_tags(self):
         # If a subscription filter has include_any_tags, a bug with one or
         # more tags is matched.
 
         self.initial_filter.include_any_tags = True
 
         # Without any tags the subscription is not found.
-        self.assertSubscriptions([])
+        self.assertSubscribers([])
 
         # With any tag the subscription is found.
         self.bug.tags = ["foo"]
-        self.assertSubscriptions([self.subscription])
+        self.assertSubscribers([self.ordinary_subscriber])
 
-    def test_getSubscriptionsForBugTask_with_filter_exclude_any_tags(self):
+    def test_getStructuralSubscribers_with_filter_exclude_any_tags(self):
         # If a subscription filter has exclude_any_tags, only bugs with no
         # tags are matched.
 
         self.initial_filter.exclude_any_tags = True
 
         # Without any tags the subscription is found.
-        self.assertSubscriptions([self.subscription])
+        self.assertSubscribers([self.ordinary_subscriber])
 
         # With any tag the subscription is not found.
         self.bug.tags = ["foo"]
-        self.assertSubscriptions([])
+        self.assertSubscribers([])
 
-    def test_getSubscriptionsForBugTask_with_filter_for_any_tag(self):
+    def test_getStructuralSubscribers_with_filter_for_any_tag(self):
         # If a subscription filter specifies that any of one or more specific
         # tags must be present, bugs with any of those tags are matched.
 
@@ -294,13 +297,13 @@
         self.initial_filter.find_all_tags = False
 
         # Without either tag the subscription is not found.
-        self.assertSubscriptions([])
+        self.assertSubscribers([])
 
         # With either tag the subscription is found.
         self.bug.tags = ["bar", "baz"]
-        self.assertSubscriptions([self.subscription])
+        self.assertSubscribers([self.ordinary_subscriber])
 
-    def test_getSubscriptionsForBugTask_with_filter_for_all_tags(self):
+    def test_getStructuralSubscribers_with_filter_for_all_tags(self):
         # If a subscription filter specifies that all of one or more specific
         # tags must be present, bugs with all of those tags are matched.
 
@@ -309,17 +312,17 @@
         self.initial_filter.find_all_tags = True
 
         # Without either tag the subscription is not found.
-        self.assertSubscriptions([])
+        self.assertSubscribers([])
 
         # Without only one of the required tags the subscription is not found.
         self.bug.tags = ["foo"]
-        self.assertSubscriptions([])
+        self.assertSubscribers([])
 
         # With both required tags the subscription is found.
         self.bug.tags = ["foo", "bar"]
-        self.assertSubscriptions([self.subscription])
+        self.assertSubscribers([self.ordinary_subscriber])
 
-    def test_getSubscriptionsForBugTask_with_filter_for_not_any_tag(self):
+    def test_getStructuralSubscribers_with_filter_for_not_any_tag(self):
         # If a subscription filter specifies that any of one or more specific
         # tags must not be present, bugs without any of those tags are
         # matched.
@@ -329,26 +332,26 @@
         self.initial_filter.find_all_tags = False
 
         # Without either tag the subscription is found.
-        self.assertSubscriptions([self.subscription])
+        self.assertSubscribers([self.ordinary_subscriber])
 
         # With both tags, the subscription is omitted.
         self.bug.tags = ["foo", "bar"]
-        self.assertSubscriptions([])
+        self.assertSubscribers([])
 
         # With only one tag, the subscription is found again.
         self.bug.tags = ["foo"]
-        self.assertSubscriptions([self.subscription])
+        self.assertSubscribers([self.ordinary_subscriber])
 
         # However, if find_all_tags is True, even a single excluded tag
         # causes the subscription to be skipped.
         self.initial_filter.find_all_tags = True
-        self.assertSubscriptions([])
+        self.assertSubscribers([])
 
         # This is also true, of course, if the bug has both tags.
         self.bug.tags = ["foo", "bar"]
-        self.assertSubscriptions([])
+        self.assertSubscribers([])
 
-    def test_getSubscriptionsForBugTask_with_filter_for_not_all_tags(self):
+    def test_getStructuralSubscribers_with_filter_for_not_all_tags(self):
         # If a subscription filter specifies that all of one or more specific
         # tags must not be present, bugs without all of those tags are
         # matched.
@@ -358,64 +361,64 @@
         self.initial_filter.find_all_tags = True
 
         # Without either tag the subscription is found.
-        self.assertSubscriptions([self.subscription])
+        self.assertSubscribers([self.ordinary_subscriber])
 
         # With only one of the excluded tags the subscription is not
         # found--we are saying that we want to find both an absence of foo
         # and an absence of bar, and yet foo exists.
         self.bug.tags = ["foo"]
-        self.assertSubscriptions([])
+        self.assertSubscribers([])
 
         # With both tags the subscription is also not found.
         self.bug.tags = ["foo", "bar"]
-        self.assertSubscriptions([])
+        self.assertSubscribers([])
 
-    def test_getSubscriptionsForBugTask_with_multiple_filters(self):
+    def test_getStructuralSubscribers_with_multiple_filters(self):
         # If multiple filters exist for a subscription, all filters must
         # match.
 
         # Add the "foo" tag to the bug.
         self.bug.tags = ["foo"]
-        self.assertSubscriptions([self.subscription])
+        self.assertSubscribers([self.ordinary_subscriber])
 
         # Filter the subscription to bugs in the CRITICAL state.
         self.initial_filter.statuses = [BugTaskStatus.CONFIRMED]
         self.initial_filter.importances = [BugTaskImportance.CRITICAL]
 
         # With the filter the subscription is not found.
-        self.assertSubscriptions([])
+        self.assertSubscribers([])
 
         # If the filter is adjusted to match status but not importance, the
         # subscription is still not found.
         self.initial_filter.statuses = [self.bugtask.status]
-        self.assertSubscriptions([])
+        self.assertSubscribers([])
 
         # If the filter is adjusted to also match importance, the subscription
         # is found again.
         self.initial_filter.importances = [self.bugtask.importance]
-        self.assertSubscriptions([self.subscription])
+        self.assertSubscribers([self.ordinary_subscriber])
 
         # If the filter is given some tag criteria, the subscription is not
         # found.
         self.initial_filter.tags = [u"-foo", u"bar", u"baz"]
         self.initial_filter.find_all_tags = False
-        self.assertSubscriptions([])
+        self.assertSubscribers([])
 
         # After removing the "foo" tag and adding the "bar" tag, the
         # subscription is found.
         self.bug.tags = ["bar"]
-        self.assertSubscriptions([self.subscription])
+        self.assertSubscribers([self.ordinary_subscriber])
 
         # Requiring that all tag criteria are fulfilled causes the
         # subscription to no longer be found.
         self.initial_filter.find_all_tags = True
-        self.assertSubscriptions([])
+        self.assertSubscribers([])
 
         # After adding the "baz" tag, the subscription is found again.
         self.bug.tags = ["bar", "baz"]
-        self.assertSubscriptions([self.subscription])
+        self.assertSubscribers([self.ordinary_subscriber])
 
-    def test_getSubscriptionsForBugTask_any_filter_is_a_match(self):
+    def test_getStructuralSubscribers_any_filter_is_a_match(self):
         # If a subscription has multiple filters, the subscription is selected
         # when any filter is found to match. Put another way, the filters are
         # ORed together.
@@ -425,24 +428,24 @@
         subscription_filter2.tags = [u"foo"]
 
         # With the filter the subscription is not found.
-        self.assertSubscriptions([])
+        self.assertSubscribers([])
 
         # If the bugtask is adjusted to match the criteria of the first filter
         # but not those of the second, the subscription is found.
         self.bugtask.transitionToStatus(
             BugTaskStatus.CONFIRMED, self.ordinary_subscriber)
-        self.assertSubscriptions([self.subscription])
+        self.assertSubscribers([self.ordinary_subscriber])
 
         # If the filter is adjusted to also match the criteria of the second
         # filter, the subscription is still found.
         self.bugtask.bug.tags = [u"foo"]
-        self.assertSubscriptions([self.subscription])
+        self.assertSubscribers([self.ordinary_subscriber])
 
         # If the bugtask is adjusted to no longer match the criteria of the
         # first filter, the subscription is still found.
         self.bugtask.transitionToStatus(
             BugTaskStatus.INPROGRESS, self.ordinary_subscriber)
-        self.assertSubscriptions([self.subscription])
+        self.assertSubscribers([self.ordinary_subscriber])
 
 
 class TestStructuralSubscriptionForDistro(