Merge lp:~allenap/launchpad/subscribe-to-tag-bug-151129 into lp:launchpad

[Gavin Panella]

[r=henninge][ui=none][bug=151129] Specific tag criteria in structural subscriptions are now taken into consideration. Previously they were ignored.

This adds the ability to subscribe to tags as part of a structural
subscription. It was already possible to subscribe to the presence or
absense of tags, just not specific tags (presence or absense thereof).

The implementation is perhaps interesting. Because set operations need
to be performed, it's not enough to simply left join to the subscribed
tags table (BugSubscriptionFilterTag). So, in this implementation I've
used PostgreSQL ARRAY types.

[Henning Eggers (henninge)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Gavin,
thank you for extending the Launchpad feature set in such a useful way.

  review approve code

There is nothing wrong with the branch but I'd like you to consider some
suggestions of mine, especially with regard to splitting up the tests. I don't
mean to push my coding style on you so I hope you can see what I am trying to
achieve.

Thanks for letting me learn about ARRAY! ;-)

Cheers,
Henning

Am 08.11.2010 14:14, schrieb Gavin Panella:
> === modified file 'lib/lp/registry/model/structuralsubscription.py'
> --- lib/lp/registry/model/structuralsubscription.py 2010-10-25 13:24:39 +0000
> +++ lib/lp/registry/model/structuralsubscription.py 2010-11-08 13:14:20 +0000
> @@ -10,6 +10,7 @@
> And,
> LeftJoin,
> Or,
> + SQL,
> )
> from storm.store import Store
> from zope.component import (
> @@ -484,13 +485,55 @@
> BugSubscriptionFilter.id)),
> ]
>
> + # An ARRAY[] expression for the given bug's tags.
> + tags_array = "ARRAY[%s]::TEXT[]" % ",".join(
> + quote(tag) for tag in bugtask.bug.tags)
> +
> + # The tags a subscription requests for inclusion.
> + tags_include_expr = (
> + "SELECT tag FROM BugSubscriptionFilterTag "
> + "WHERE filter = BugSubscriptionFilter.id AND include")
> + tags_include_array = "ARRAY(%s)" % tags_include_expr
> + tags_include_is_empty = SQL(
> + "ARRAY[]::TEXT[] = ARRAY(%s)" % tags_include_expr)

I don't understand why you are not reusing tags_include_array here.

+ "ARRAY[]::TEXT[] = %s" % tags_include_array)

I think that reads a lot clearer, or am I missing something?

> +
> + # The tags a subscription requests for exclusion.
> + tags_exclude_expr = (
> + "SELECT tag FROM BugSubscriptionFilterTag "
> + "WHERE filter = BugSubscriptionFilter.id AND NOT include")
> + tags_exclude_array = "ARRAY(%s)" % tags_exclude_expr
> + tags_exclude_is_empty = SQL(
> + "ARRAY[]::TEXT[] = ARRAY(%s)" % tags_exclude_expr)

Same here.

> +
> + # Choose the correct expression depending on the find_all_tags flag.
> + def tags_find_all_combinator(find_all_expr, find_any_expr):

"The Combinator!" I like that ... ;)

> + return SQL(
> + "CASE WHEN BugSubscriptionFilter.find_all_tags "
> + "THEN (%s) ELSE (%s) END" % (find_all_expr, find_any_expr))
> +
> if len(bugtask.bug.tags) == 0:
> tag_conditions = [
> BugSubscriptionFilter.include_any_tags == False,
> + # The subscription's required tags must be an empty set.
> + SQL("%s = %s" % (tags_array, tags_include_array)),

I understand that because len(tags) == 0, tags_array is an empty array but had
to wrap my head around it first. Why not use an explicit empty array?

+ SQL("ARRAY[]::TEXT[] = %s" % tags_include_array),

... and when you look at that, you'll quickly notice that it matches the
definition of "tags_include_is_empty", so you could just use that here.

> + # The ...

review: Approve (code)

[Robert Collins (lifeless)]

Why use ARRAY rather than an explicit modelling of tag?
 We model tags explicitly in bugtag....

-Rob

[Gavin Panella (allenap)]

Rob, I don't know! My thinking was already going down one path and I
didn't see the wood for the trees. I'll change it in a follow-on
branch (and I won't land this as it stands).

[Gavin Panella (allenap)]

Henning, thank you for the review. I'm going to switch away from
ARRAYs (unless I hit some big problem), but I'll keep the tests of
course. I'm working on your suggestions :)

[Gavin Panella (allenap)]

> > + tags_include_is_empty = SQL(
> > + "ARRAY[]::TEXT[] = ARRAY(%s)" % tags_include_expr)
>
> I don't understand why you are not reusing tags_include_array here.

Neither do I. Fixed.

> > +
> > + # Choose the correct expression depending on the find_all_tags flag.
> > + def tags_find_all_combinator(find_all_expr, find_any_expr):
>
> "The Combinator!" I like that ... ;)

Hehe. I discovered that it's a term from lambda calculus, which I have
never studied. So I'm probably abusing the term and, worse, abusing it
with flagrant ignorance :)

> + SQL("ARRAY[]::TEXT[] = %s" % tags_include_array),
>
> ... and when you look at that, you'll quickly notice that it matches the
> definition of "tags_include_is_empty", so you could just use that here.

Erm, yes, I should. Fixed. Thanks.

> > + # The subscription's excluded tags can be anything.
>
> "..., so no condition is needed for them." This comment at the end got me
> confused at first, so I think it would benefit from the added explanation.

Good suggestion.

> This code from here ...
> > + # If a subscription filter specifies that any of one or more specific
> > + # tags must be present, bugs with any of those tags are matched.
> > + bugtask = self.makeBugTask()
> > +
> > + # Create a new subscription on self.target.
> > + login_person(self.ordinary_subscriber)
> > + subscription = self.target.addSubscription(
> > + self.ordinary_subscriber, self.ordinary_subscriber)
> > + subscription.bug_notification_level = BugNotificationLevel.COMMENTS
> > + subscription_filter = subscription.newBugFilter()
> > +
> > + # Looking for either the "foo" or the "bar" tag.
> > + subscription_filter.tags = [u"foo", u"bar"]
> > + subscription_filter.find_all_tags = False
>
> ... to here is repeated in the next two tests, with only the tags and
> find_all_tags changing. I think that should go into a
> "makeBugTaskAndSubscription" method or similar with parameters for the
> changing values.

I've changed the inheritance of various tests and added several new
TestStructuralSubscriptionFiltersFor$THING classes so that I could put
this common stuff in setUp().

> > + # Without either tag the subscription is not found.
> > + subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
> > + bugtask, BugNotificationLevel.NOTHING)
> > + self.assertEqual([], list(subscriptions_for_bugtask))
> > +
>
> Having a factory method makes it also easy to split the test up into smaller
> chunks for the different conditions under which getSubscriptionsForBugTask is
> being tested. I think you can fulfill the "one assert per test" paradigm here
> easily. ;-)

I've moved this into a new assertSubscriptions() method, and used it
everywhere this pattern appears. That has reduced the line count a
*lot*, more than I realised as I slowly grew this class. Good
suggestion, thanks.

However, I'm going to resist the "one assert per test" thing. I
personally think this is better because it reduces the setup and
teardown costs, makes it no more difficult to ...

=== modified file 'lib/lp/registry/model/structuralsubscription.py'
--- lib/lp/registry/model/structuralsubscription.py	2010-11-09 11:00:55 +0000
+++ lib/lp/registry/model/structuralsubscription.py	2010-11-24 09:39:54 +0000
@@ -10,6 +10,7 @@
     And,
     LeftJoin,
     Or,
+    SQL,
     )
 from storm.store import Store
 from zope.component import (
@@ -487,13 +488,56 @@
                     BugSubscriptionFilter.id)),
             ]
 
+        # An ARRAY[] expression for the given bug's tags.
+        tags_array = "ARRAY[%s]::TEXT[]" % ",".join(
+            quote(tag) for tag in bugtask.bug.tags)
+
+        # The tags a subscription requests for inclusion.
+        tags_include_expr = (
+            "SELECT tag FROM BugSubscriptionFilterTag "
+            "WHERE filter = BugSubscriptionFilter.id AND include")
+        tags_include_array = "ARRAY(%s)" % tags_include_expr
+        tags_include_is_empty = SQL(
+            "ARRAY[]::TEXT[] = %s" % tags_include_array)
+
+        # The tags a subscription requests for exclusion.
+        tags_exclude_expr = (
+            "SELECT tag FROM BugSubscriptionFilterTag "
+            "WHERE filter = BugSubscriptionFilter.id AND NOT include")
+        tags_exclude_array = "ARRAY(%s)" % tags_exclude_expr
+        tags_exclude_is_empty = SQL(
+            "ARRAY[]::TEXT[] = %s" % tags_exclude_array)
+
+        # Choose the correct expression depending on the find_all_tags flag.
+        def tags_find_all_combinator(find_all_expr, find_any_expr):
+            return SQL(
+                "CASE WHEN BugSubscriptionFilter.find_all_tags "
+                "THEN (%s) ELSE (%s) END" % (find_all_expr, find_any_expr))
+
         if len(bugtask.bug.tags) == 0:
             tag_conditions = [
                 BugSubscriptionFilter.include_any_tags == False,
+                # The subscription's required tags must be an empty set.
+                tags_include_is_empty,
+                # The subscription's excluded tags can be anything so no
+                # condition is needed.
                 ]
         else:
             tag_conditions = [
                 BugSubscriptionFilter.exclude_any_tags == False,
+                # The bug's tags must contain the subscription's required tags
+                # if find_all_tags is set, else there must be an intersection.
+                Or(tags_include_is_empty,
+                   tags_find_all_combinator(
+                        "%s @> %s" % (tags_array, tags_include_array),
+                        "%s && %s" % (tags_array, tags_include_array))),
+                # The bug's tags must not contain the subscription's excluded
+                # tags if find_all_tags is set, else there must not be an
+                # intersection.
+                Or(tags_exclude_is_empty,
+                   tags_find_all_combinator(
+                        "NOT (%s @> %s)" % (tags_array, tags_exclude_array),
+                        "NOT (%s && %s)" % (tags_array, tags_exclude_array))),
                 ]
 
         conditions = [

=== modified file 'lib/lp/registry/tests/test_structuralsubscriptiontarget.py'
--- lib/lp/registry/tests/test_structuralsubscriptiontarget.py	2010-10-07 10:07:51 +0000
+++ lib/lp/registry/tests/test_structuralsubscriptiontarget.py	2010-11-24 09:39:54 +0000
@@ -32,7 +32,6 @@
     BugTaskImportance,
     BugTaskStatus,
     )
-from lp.bugs.model.bugsubscriptionfilter import BugSubscriptionFilter
 from lp.bugs.tests.test_bugtarget import bugtarget_filebug
 from lp.registry.enum import BugNotificationLevel
 from lp.registry.errors import (
@@ -66,6 +65,10 @@
         self.team_owner = self.factory.makePerson()
         self.team = self.factory.makeTeam(owner=self.team_owner)
 
+
+class RestrictedStructuralSubscription(StructuralSubscriptionTestBase):
+    # Tests suitable for a target that restricts structural subscriptions.
+
     def test_target_implements_structural_subscription_target(self):
         self.assertTrue(verifyObject(IStructuralSubscriptionTarget,
                                      self.target))
@@ -124,7 +127,9 @@
             self.ordinary_subscriber, self.ordinary_subscriber)
 
 
-class UnrestrictedStructuralSubscription(StructuralSubscriptionTestBase):
+class UnrestrictedStructuralSubscription(RestrictedStructuralSubscription):
+    # Tests suitable for a target that does not restrict structural
+    # subscriptions.
 
     def test_structural_subscription_by_ordinary_user(self):
         # ordinary users can subscribe themselves
@@ -170,195 +175,243 @@
 class FilteredStructuralSubscriptionTestBase(StructuralSubscriptionTestBase):
     """Tests for filtered structural subscriptions."""
 
+    layer = LaunchpadFunctionalLayer
+
+    def makeTarget(self):
+        raise NotImplementedError(self.makeTarget)
+
     def makeBugTask(self):
         return self.factory.makeBugTask(target=self.target)
 
+    def setUp(self):
+        super(FilteredStructuralSubscriptionTestBase, self).setUp()
+        login_person(self.ordinary_subscriber)
+        self.target = self.makeTarget()
+        self.bugtask = self.makeBugTask()
+        self.bug = self.bugtask.bug
+        self.subscription = self.target.addSubscription(
+            self.ordinary_subscriber, self.ordinary_subscriber)
+        self.subscription.bug_notification_level = (
+            BugNotificationLevel.COMMENTS)
+
+    def assertSubscriptions(
+        self, expected_subscriptions, level=BugNotificationLevel.NOTHING):
+        observed_subscriptions = list(
+            self.target.getSubscriptionsForBugTask(self.bugtask, level))
+        self.assertEqual(expected_subscriptions, observed_subscriptions)
+
     def test_getSubscriptionsForBugTask(self):
         # If no one has a filtered subscription for the given bug, the result
         # of getSubscriptionsForBugTask() is the same as for
         # getSubscriptions().
-        bugtask = self.makeBugTask()
         subscriptions = self.target.getSubscriptions(
             min_bug_notification_level=BugNotificationLevel.NOTHING)
-        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
-            bugtask, BugNotificationLevel.NOTHING)
-        self.assertEqual(list(subscriptions), list(subscriptions_for_bugtask))
+        self.assertSubscriptions(list(subscriptions))
 
     def test_getSubscriptionsForBugTask_with_filter_on_status(self):
         # If a status filter exists for a subscription, the result of
         # getSubscriptionsForBugTask() may be a subset of getSubscriptions().
-        bugtask = self.makeBugTask()
-
-        # Create a new subscription on self.target.
-        login_person(self.ordinary_subscriber)
-        subscription = self.target.addSubscription(
-            self.ordinary_subscriber, self.ordinary_subscriber)
-        subscription.bug_notification_level = BugNotificationLevel.COMMENTS
 
         # Without any filters the subscription is found.
-        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
-            bugtask, BugNotificationLevel.NOTHING)
-        self.assertEqual([subscription], list(subscriptions_for_bugtask))
+        self.assertSubscriptions([self.subscription])
 
         # Filter the subscription to bugs in the CONFIRMED state.
-        subscription_filter = BugSubscriptionFilter()
-        subscription_filter.structural_subscription = subscription
+        subscription_filter = self.subscription.newBugFilter()
         subscription_filter.statuses = [BugTaskStatus.CONFIRMED]
 
         # With the filter the subscription is not found.
-        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
-            bugtask, BugNotificationLevel.NOTHING)
-        self.assertEqual([], list(subscriptions_for_bugtask))
+        self.assertSubscriptions([])
 
         # If the filter is adjusted, the subscription is found again.
-        subscription_filter.statuses = [bugtask.status]
-        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
-            bugtask, BugNotificationLevel.NOTHING)
-        self.assertEqual([subscription], list(subscriptions_for_bugtask))
+        subscription_filter.statuses = [self.bugtask.status]
+        self.assertSubscriptions([self.subscription])
 
     def test_getSubscriptionsForBugTask_with_filter_on_importance(self):
         # If an importance filter exists for a subscription, the result of
         # getSubscriptionsForBugTask() may be a subset of getSubscriptions().
-        bugtask = self.makeBugTask()
-
-        # Create a new subscription on self.target.
-        login_person(self.ordinary_subscriber)
-        subscription = self.target.addSubscription(
-            self.ordinary_subscriber, self.ordinary_subscriber)
-        subscription.bug_notification_level = BugNotificationLevel.COMMENTS
 
         # Without any filters the subscription is found.
-        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
-            bugtask, BugNotificationLevel.NOTHING)
-        self.assertEqual([subscription], list(subscriptions_for_bugtask))
+        self.assertSubscriptions([self.subscription])
 
         # Filter the subscription to bugs in the CRITICAL state.
-        subscription_filter = BugSubscriptionFilter()
-        subscription_filter.structural_subscription = subscription
+        subscription_filter = self.subscription.newBugFilter()
         subscription_filter.importances = [BugTaskImportance.CRITICAL]
 
         # With the filter the subscription is not found.
-        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
-            bugtask, BugNotificationLevel.NOTHING)
-        self.assertEqual([], list(subscriptions_for_bugtask))
+        self.assertSubscriptions([])
 
         # If the filter is adjusted, the subscription is found again.
-        subscription_filter.importances = [bugtask.importance]
-        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
-            bugtask, BugNotificationLevel.NOTHING)
-        self.assertEqual([subscription], list(subscriptions_for_bugtask))
+        subscription_filter.importances = [self.bugtask.importance]
+        self.assertSubscriptions([self.subscription])
 
     def test_getSubscriptionsForBugTask_with_filter_on_level(self):
         # All structural subscriptions have a level for bug notifications
         # which getSubscriptionsForBugTask() observes.
-        bugtask = self.makeBugTask()
 
-        # Create a new METADATA level subscription on self.target.
-        login_person(self.ordinary_subscriber)
-        subscription = self.target.addSubscription(
-            self.ordinary_subscriber, self.ordinary_subscriber)
-        subscription.bug_notification_level = BugNotificationLevel.METADATA
+        # Adjust the subscription level to METADATA.
+        self.subscription.bug_notification_level = (
+            BugNotificationLevel.METADATA)
 
         # The subscription is found when looking for NOTHING or above.
-        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
-            bugtask, BugNotificationLevel.NOTHING)
-        self.assertEqual([subscription], list(subscriptions_for_bugtask))
+        self.assertSubscriptions(
+            [self.subscription], BugNotificationLevel.NOTHING)
         # The subscription is found when looking for METADATA or above.
-        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
-            bugtask, BugNotificationLevel.METADATA)
-        self.assertEqual([subscription], list(subscriptions_for_bugtask))
+        self.assertSubscriptions(
+            [self.subscription], BugNotificationLevel.METADATA)
         # The subscription is not found when looking for COMMENTS or above.
-        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
-            bugtask, BugNotificationLevel.COMMENTS)
-        self.assertEqual([], list(subscriptions_for_bugtask))
+        self.assertSubscriptions(
+            [], BugNotificationLevel.COMMENTS)
 
     def test_getSubscriptionsForBugTask_with_filter_include_any_tags(self):
         # If a subscription filter has include_any_tags, a bug with one or
         # more tags is matched.
-        bugtask = self.makeBugTask()
 
-        # Create a new subscription on self.target.
-        login_person(self.ordinary_subscriber)
-        subscription = self.target.addSubscription(
-            self.ordinary_subscriber, self.ordinary_subscriber)
-        subscription.bug_notification_level = BugNotificationLevel.COMMENTS
-        subscription_filter = subscription.newBugFilter()
+        subscription_filter = self.subscription.newBugFilter()
         subscription_filter.include_any_tags = True
 
         # Without any tags the subscription is not found.
-        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
-            bugtask, BugNotificationLevel.NOTHING)
-        self.assertEqual([], list(subscriptions_for_bugtask))
+        self.assertSubscriptions([])
 
         # With any tag the subscription is found.
-        bugtask.bug.tags = ["foo"]
-        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
-            bugtask, BugNotificationLevel.NOTHING)
-        self.assertEqual([subscription], list(subscriptions_for_bugtask))
+        self.bug.tags = ["foo"]
+        self.assertSubscriptions([self.subscription])
 
     def test_getSubscriptionsForBugTask_with_filter_exclude_any_tags(self):
         # If a subscription filter has exclude_any_tags, only bugs with no
         # tags are matched.
-        bugtask = self.makeBugTask()
 
-        # Create a new subscription on self.target.
-        login_person(self.ordinary_subscriber)
-        subscription = self.target.addSubscription(
-            self.ordinary_subscriber, self.ordinary_subscriber)
-        subscription.bug_notification_level = BugNotificationLevel.COMMENTS
-        subscription_filter = subscription.newBugFilter()
+        subscription_filter = self.subscription.newBugFilter()
         subscription_filter.exclude_any_tags = True
 
         # Without any tags the subscription is found.
-        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
-            bugtask, BugNotificationLevel.NOTHING)
-        self.assertEqual([subscription], list(subscriptions_for_bugtask))
+        self.assertSubscriptions([self.subscription])
 
         # With any tag the subscription is not found.
-        bugtask.bug.tags = ["foo"]
-        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
-            bugtask, BugNotificationLevel.NOTHING)
-        self.assertEqual([], list(subscriptions_for_bugtask))
+        self.bug.tags = ["foo"]
+        self.assertSubscriptions([])
+
+    def test_getSubscriptionsForBugTask_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.
+
+        # Looking for either the "foo" or the "bar" tag.
+        subscription_filter = self.subscription.newBugFilter()
+        subscription_filter.tags = [u"foo", u"bar"]
+        subscription_filter.find_all_tags = False
+
+        # Without either tag the subscription is not found.
+        self.assertSubscriptions([])
+
+        # With either tag the subscription is found.
+        self.bug.tags = ["bar", "baz"]
+        self.assertSubscriptions([self.subscription])
+
+    def test_getSubscriptionsForBugTask_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.
+
+        # Looking for both the "foo" and the "bar" tag.
+        subscription_filter = self.subscription.newBugFilter()
+        subscription_filter.tags = [u"foo", u"bar"]
+        subscription_filter.find_all_tags = True
+
+        # Without either tag the subscription is not found.
+        self.assertSubscriptions([])
+
+        # Without only one of the required tags the subscription is not found.
+        self.bug.tags = ["foo"]
+        self.assertSubscriptions([])
+
+        # With both required tags the subscription is found.
+        self.bug.tags = ["foo", "bar"]
+        self.assertSubscriptions([self.subscription])
+
+    def test_getSubscriptionsForBugTask_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.
+
+        # Looking to exclude the "foo" or "bar" tags.
+        subscription_filter = self.subscription.newBugFilter()
+        subscription_filter.tags = [u"-foo", u"-bar"]
+        subscription_filter.find_all_tags = False
+
+        # Without either tag the subscription is found.
+        self.assertSubscriptions([self.subscription])
+
+        # With either tag the subscription is no longer found.
+        self.bug.tags = ["foo"]
+        self.assertSubscriptions([])
+
+    def test_getSubscriptionsForBugTask_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.
+
+        # Looking to exclude the "foo" and "bar" tags.
+        subscription_filter = self.subscription.newBugFilter()
+        subscription_filter.tags = [u"-foo", u"-bar"]
+        subscription_filter.find_all_tags = True
+
+        # Without either tag the subscription is found.
+        self.assertSubscriptions([self.subscription])
+
+        # With only one of the excluded tags the subscription is found.
+        self.bug.tags = ["foo"]
+        self.assertSubscriptions([self.subscription])
+
+        # With both tags the subscription is no longer found.
+        self.bug.tags = ["foo", "bar"]
+        self.assertSubscriptions([])
 
     def test_getSubscriptionsForBugTask_with_multiple_filters(self):
         # If multiple filters exist for a subscription, all filters must
         # match.
-        bugtask = self.makeBugTask()
 
-        # Create a new subscription on self.target.
-        login_person(self.ordinary_subscriber)
-        subscription = self.target.addSubscription(
-            self.ordinary_subscriber, self.ordinary_subscriber)
-        subscription.bug_notification_level = BugNotificationLevel.COMMENTS
+        # Add the "foo" tag to the bug.
+        self.bug.tags = ["foo"]
 
         # Filter the subscription to bugs in the CRITICAL state.
-        subscription_filter = BugSubscriptionFilter()
-        subscription_filter.structural_subscription = subscription
+        subscription_filter = self.subscription.newBugFilter()
         subscription_filter.statuses = [BugTaskStatus.CONFIRMED]
         subscription_filter.importances = [BugTaskImportance.CRITICAL]
 
         # With the filter the subscription is not found.
-        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
-            bugtask, BugNotificationLevel.NOTHING)
-        self.assertEqual([], list(subscriptions_for_bugtask))
+        self.assertSubscriptions([])
 
         # If the filter is adjusted to match status but not importance, the
         # subscription is still not found.
-        subscription_filter.statuses = [bugtask.status]
-        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
-            bugtask, BugNotificationLevel.NOTHING)
-        self.assertEqual([], list(subscriptions_for_bugtask))
+        subscription_filter.statuses = [self.bugtask.status]
+        self.assertSubscriptions([])
 
         # If the filter is adjusted to also match importance, the subscription
         # is found again.
-        subscription_filter.importances = [bugtask.importance]
-        subscriptions_for_bugtask = self.target.getSubscriptionsForBugTask(
-            bugtask, BugNotificationLevel.NOTHING)
-        self.assertEqual([subscription], list(subscriptions_for_bugtask))
+        subscription_filter.importances = [self.bugtask.importance]
+        self.assertSubscriptions([self.subscription])
+
+        # If the filter is given some tag criteria, the subscription is not
+        # found.
+        subscription_filter.tags = [u"-foo", u"bar", u"baz"]
+        subscription_filter.find_all_tags = False
+        self.assertSubscriptions([])
+
+        # After removing the "foo" tag and adding the "bar" tag, the
+        # subscription is found.
+        self.bug.tags = ["bar"]
+        self.assertSubscriptions([self.subscription])
+
+        # Requiring that all tag criteria are fulfilled causes the
+        # subscription to no longer be found.
+        subscription_filter.find_all_tags = True
+        self.assertSubscriptions([])
+
+        # After adding the "baz" tag, the subscription is found again.
+        self.bug.tags = ["bar", "baz"]
+        self.assertSubscriptions([self.subscription])
 
 
 class TestStructuralSubscriptionForDistro(
-    FilteredStructuralSubscriptionTestBase, TestCaseWithFactory):
+    RestrictedStructuralSubscription, TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
@@ -421,10 +474,15 @@
             StructuralSubscription)
 
 
+class TestStructuralSubscriptionFiltersForDistro(
+    FilteredStructuralSubscriptionTestBase, TestCaseWithFactory):
+
+    def makeTarget(self):
+        return self.factory.makeDistribution()
+
+
 class TestStructuralSubscriptionForProduct(
-    UnrestrictedStructuralSubscription,
-    FilteredStructuralSubscriptionTestBase,
-    TestCaseWithFactory):
+    UnrestrictedStructuralSubscription, TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
@@ -433,10 +491,15 @@
         self.target = self.factory.makeProduct()
 
 
+class TestStructuralSubscriptionFiltersForProduct(
+    FilteredStructuralSubscriptionTestBase, TestCaseWithFactory):
+
+    def makeTarget(self):
+        return self.factory.makeProduct()
+
+
 class TestStructuralSubscriptionForDistroSourcePackage(
-    UnrestrictedStructuralSubscription,
-    FilteredStructuralSubscriptionTestBase,
-    TestCaseWithFactory):
+    UnrestrictedStructuralSubscription, TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
@@ -446,10 +509,15 @@
         self.target = ProxyFactory(self.target)
 
 
+class TestStructuralSubscriptionFiltersForDistroSourcePackage(
+    FilteredStructuralSubscriptionTestBase, TestCaseWithFactory):
+
+    def makeTarget(self):
+        return self.factory.makeDistributionSourcePackage()
+
+
 class TestStructuralSubscriptionForMilestone(
-    UnrestrictedStructuralSubscription,
-    FilteredStructuralSubscriptionTestBase,
-    TestCaseWithFactory):
+    UnrestrictedStructuralSubscription, TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
@@ -458,15 +526,19 @@
         self.target = self.factory.makeMilestone()
         self.target = ProxyFactory(self.target)
 
+
+class TestStructuralSubscriptionFiltersForMilestone(
+    FilteredStructuralSubscriptionTestBase, TestCaseWithFactory):
+
+    def makeTarget(self):
+        return self.factory.makeMilestone()
+
     def makeBugTask(self):
-        # XXX Should test with target *and* series_target.
         return self.factory.makeBugTask(target=self.target.series_target)
 
 
 class TestStructuralSubscriptionForDistroSeries(
-    UnrestrictedStructuralSubscription,
-    FilteredStructuralSubscriptionTestBase,
-    TestCaseWithFactory):
+    UnrestrictedStructuralSubscription, TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
@@ -476,10 +548,15 @@
         self.target = ProxyFactory(self.target)
 
 
+class TestStructuralSubscriptionFiltersForDistroSeries(
+    FilteredStructuralSubscriptionTestBase, TestCaseWithFactory):
+
+    def makeTarget(self):
+        return self.factory.makeDistroSeries()
+
+
 class TestStructuralSubscriptionForProjectGroup(
-    UnrestrictedStructuralSubscription,
-    FilteredStructuralSubscriptionTestBase,
-    TestCaseWithFactory):
+    UnrestrictedStructuralSubscription, TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
@@ -488,15 +565,20 @@
         self.target = self.factory.makeProject()
         self.target = ProxyFactory(self.target)
 
+
+class TestStructuralSubscriptionFiltersForProjectGroup(
+    FilteredStructuralSubscriptionTestBase, TestCaseWithFactory):
+
+    def makeTarget(self):
+        return self.factory.makeProject()
+
     def makeBugTask(self):
         return self.factory.makeBugTask(
             target=self.factory.makeProduct(project=self.target))
 
 
 class TestStructuralSubscriptionForProductSeries(
-    UnrestrictedStructuralSubscription,
-    FilteredStructuralSubscriptionTestBase,
-    TestCaseWithFactory):
+    UnrestrictedStructuralSubscription, TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
@@ -506,6 +588,13 @@
         self.target = ProxyFactory(self.target)
 
 
+class TestStructuralSubscriptionFiltersForProductSeries(
+    FilteredStructuralSubscriptionTestBase, TestCaseWithFactory):
+
+    def makeTarget(self):
+        return self.factory.makeProductSeries()
+
+
 class TestStructuralSubscriptionTargetHelper(TestCaseWithFactory):
     """Tests for implementations of `IStructuralSubscriptionTargetHelper`."""