← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~danilo/launchpad/auto-create-bugsubscriptionfilter into lp:launchpad

 

Данило Шеган has proposed merging lp:~danilo/launchpad/auto-create-bugsubscriptionfilter into lp:launchpad with lp:~danilo/launchpad/devel-bug-720826 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #720826 Add subscription description header for bug notifications
  https://bugs.launchpad.net/bugs/720826

For more details, see:
https://code.launchpad.net/~danilo/launchpad/auto-create-bugsubscriptionfilter/+merge/50371

= Auto-create BugSubscriptionFilter for StructuralSubscriptions =

== Proposed fix ==

Whenever a StructuralSubscription (SS) is created, create a BugSubscriptionFilter (BSF) with no filters and using all the defaults.

== Implementation details ==

Out of several alternative approaches (eg. creating it with a trigger or from within a constructor), I've decided to create a BugSubscriptionFilter in the only place StructuralSubscription is created: in StructuralSubscriptionTarget.addSubscription().  Reason against the trigger is to simplify understanding code (i.e. keep everything in Python) and to not require DB changes (so we can roll this out without waiting for the full LP rollout), and reason against using the constructor is purely personal dislike for that (I like classes not to do too much in the background, especially if that involves creating a DB row).

The approach meant that I had to do away with any other places constructing StructuralSubscriptions directly (well, not really, but I wanted to), and that was just in tests.

A bunch of tests (especially regarding filters themselves) needed updating because they assumed that StructuralSubscriptions initially have no subscriptions.

Deletion is modified so as not to allow deleting the last BugSubscriptionFilter (it is "emptied" though, so it becomes a catch-all filter, like the new ones created when a StructuralSubscription is created).  There is a risk of race conditions if two different filters get deleted at roughly the same time, and I'd like to solve that with a DB trigger (because they are executed atomically as part of the DELETE, there's no race condition there: though not in this branch).

Constraint to check that there is at least one BSF for every SS can't be created "in any sane way" (to quote Stuart B.), and I've already added another "task" in our kanban board for the above DB trigger.

=== Migration ===

For existing SSs without any BSFs, as a special rollout requirement, we'll add them using a query (already approved by Stuart for running on production DB): https://pastebin.canonical.com/43604/

== Tests ==

bin/test -cvvt StructuralSubscription -t BugSubscriptionFilter

== Demo and Q/A ==

Log in as jordi@xxxxxxxxxx:test and go to https://bugs.launchpad.dev/firefox and use "Edit bug subscriptions" to add a structural subscription.  Then go to https://bugs.launchpad.dev/people/+me/+structural-subscriptions and notice how there is already a filter added automatically.  One should also try deleting it (it should stay there), or first editing it to have some actual options, and then try deleting it (watching how only conditions get removed, but the filter remains).

(for qastaging, just use your own account and create a new structural subscription on whatever you want)

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py
  lib/lp/bugs/tests/test_structuralsubscription.py
  lib/lp/bugs/browser/tests/test_structuralsubscription.py
  lib/lp/bugs/model/tests/test_bug.py
  lib/lp/bugs/model/bugsubscriptionfilter.py
  lib/lp/bugs/tests/test_structuralsubscriptiontarget.py
  lib/lp/bugs/model/structuralsubscription.py
  lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py
  lib/lp/bugs/doc/structural-subscriptions.txt
  lib/lp/registry/tests/test_person.py
-- 
https://code.launchpad.net/~danilo/launchpad/auto-create-bugsubscriptionfilter/+merge/50371
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/auto-create-bugsubscriptionfilter into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py'
--- lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py	2011-02-08 16:21:39 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py	2011-02-18 18:19:02 +0000
@@ -52,6 +52,7 @@
         with person_logged_in(self.owner):
             self.subscription = self.structure.addBugSubscription(
                 self.owner, self.owner)
+            self.initial_filter = self.subscription.bug_filters.one()
             self.subscription_filter = self.subscription.newBugFilter()
 
 
@@ -484,7 +485,8 @@
                 self.subscription_filter, name="+edit", form=form)
             self.assertEqual([], view.errors)
         # The subscription filter has been deleted.
-        self.assertEqual([], list(self.subscription.bug_filters))
+        self.assertEqual(
+            [self.initial_filter], list(self.subscription.bug_filters))
 
 
 class TestBugSubscriptionFilterAdvancedFeatures(TestCaseWithFactory):
@@ -514,6 +516,7 @@
                 with person_logged_in(person):
                     subscription = self.target.addBugSubscription(
                         person, person)
+                    initial_filter = subscription.bug_filters.one()
                     form = {
                         "field.description": "New description",
                         "field.statuses": ["NEW", "INCOMPLETE"],
@@ -527,12 +530,14 @@
                         subscription, name="+new-filter", form=form)
 
                 filters = subscription.bug_filters
-                self.assertEqual(filters.count(), 1)
+                new_filter = [filter for filter in filters
+                              if filter != initial_filter][0]
+                self.assertEqual(filters.count(), 2)
                 self.assertEqual(
-                    level, filters[0].bug_notification_level,
+                    level, new_filter.bug_notification_level,
                     "Bug notification level of filter should be %s, "
                     "is actually %s." % (
-                        level.name, filters[0].bug_notification_level.name))
+                        level.name, new_filter.bug_notification_level.name))
 
     def test_nothing_is_not_a_valid_level(self):
         # BugNotificationLevel.NOTHING isn't considered valid when a
@@ -596,7 +601,9 @@
 
     def test_create(self):
         # New filters can be created with +new-filter.
-        self.assertEqual([], list(self.subscription.bug_filters))
+        initial_filter = self.subscription.bug_filters.one()
+        self.assertEqual(
+            [initial_filter], list(self.subscription.bug_filters))
         form = {
             "field.description": "New description",
             "field.statuses": ["NEW", "INCOMPLETE"],
@@ -610,7 +617,9 @@
                 self.subscription, name="+new-filter", form=form)
             self.assertEqual([], view.errors)
         # The subscription filter has been created.
-        subscription_filter = self.subscription.bug_filters.one()
+        subscription_filter = [
+            filter for filter in self.subscription.bug_filters
+            if filter != initial_filter][0]
         self.assertEqual(
             u"New description",
             subscription_filter.description)

=== modified file 'lib/lp/bugs/browser/tests/test_structuralsubscription.py'
--- lib/lp/bugs/browser/tests/test_structuralsubscription.py	2011-02-04 01:15:13 +0000
+++ lib/lp/bugs/browser/tests/test_structuralsubscription.py	2011-02-18 18:19:02 +0000
@@ -10,7 +10,6 @@
 from zope.publisher.interfaces import NotFound
 
 from canonical.launchpad.ftests import (
-    LaunchpadFormHarness,
     login,
     logout,
     )
@@ -19,10 +18,6 @@
 from canonical.testing.layers import (
     AppServerLayer,
     DatabaseFunctionalLayer,
-    LaunchpadFunctionalLayer,
-    )
-from lp.bugs.browser.structuralsubscription import (
-    StructuralSubscriptionView,
     )
 from lp.registry.browser.distribution import DistributionNavigation
 from lp.registry.browser.distributionsourcepackage import (
@@ -243,9 +238,12 @@
         with person_logged_in(self.owner):
             self.subscription = self.structure.addBugSubscription(
                 self.owner, self.owner)
+            self.initial_filter = self.subscription.bug_filters[0]
         transaction.commit()
         self.service = self.factory.makeLaunchpadService(self.owner)
         self.ws_subscription = ws_object(self.service, self.subscription)
+        self.ws_subscription_filter = ws_object(
+            self.service, self.initial_filter)
 
     def test_newBugFilter(self):
         # New bug subscription filters can be created with newBugFilter().
@@ -263,15 +261,18 @@
         bug_filter_links = lambda: set(
             bug_filter.self_link for bug_filter in (
                 self.ws_subscription.bug_filters))
-        self.assertEqual(set(), bug_filter_links())
+        initial_filter_link = self.ws_subscription_filter.self_link
+        self.assertContentEqual(
+            [initial_filter_link], bug_filter_links())
         # A new filter appears in the bug_filters collection.
         ws_subscription_filter1 = self.ws_subscription.newBugFilter()
-        self.assertEqual(
-            set([ws_subscription_filter1.self_link]),
+        self.assertContentEqual(
+            [ws_subscription_filter1.self_link, initial_filter_link],
             bug_filter_links())
         # A second new filter also appears in the bug_filters collection.
         ws_subscription_filter2 = self.ws_subscription.newBugFilter()
-        self.assertEqual(
-            set([ws_subscription_filter1.self_link,
-                 ws_subscription_filter2.self_link]),
+        self.assertContentEqual(
+            [ws_subscription_filter1.self_link,
+             ws_subscription_filter2.self_link,
+             initial_filter_link],
             bug_filter_links())

=== modified file 'lib/lp/bugs/doc/structural-subscriptions.txt'
--- lib/lp/bugs/doc/structural-subscriptions.txt	2011-01-28 12:44:50 +0000
+++ lib/lp/bugs/doc/structural-subscriptions.txt	2011-02-18 18:19:02 +0000
@@ -6,13 +6,11 @@
 distroseries, milestone or a combination of sourcepackagename and
 distribution.
 
+    >>> from lp.testing import person_logged_in
     >>> from zope.component import getUtility
     >>> from lp.registry.interfaces.distribution import IDistributionSet
     >>> from lp.registry.interfaces.person import IPersonSet
     >>> from lp.registry.interfaces.product import IProductSet
-    >>> from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
-    >>> from lp.bugs.model.structuralsubscription import (
-    ...    StructuralSubscription)
 
     >>> person_set = getUtility(IPersonSet)
     >>> foobar = person_set.getByEmail('foo.bar@xxxxxxxxxxxxx')
@@ -20,21 +18,22 @@
     >>> firefox = getUtility(IProductSet).getByName("firefox")
     >>> ubuntu = getUtility(IDistributionSet).getByName("ubuntu")
 
-    >>> ff_sub = StructuralSubscription(product=firefox,
-    ...     subscriber=sampleperson, subscribed_by=foobar)
+    >>> with person_logged_in(foobar):
+    ...     ff_sub = firefox.addBugSubscription(
+    ...         subscriber=sampleperson, subscribed_by=foobar)
     >>> ff_sub.target
     <Product at ...>
 
-    >>> ubuntu_sub = StructuralSubscription(distribution=ubuntu,
-    ...     subscriber=sampleperson, subscribed_by=foobar)
+    >>> with person_logged_in(foobar):
+    ...     ubuntu_sub = ubuntu.addBugSubscription(
+    ...         subscriber=sampleperson, subscribed_by=foobar)
     >>> ubuntu_sub.target
     <Distribution 'Ubuntu' (ubuntu)>
 
-    >>> evolution = getUtility(ISourcePackageNameSet).getOrCreateByName(
-    ...    'evolution')
-    >>> evolution_sub = StructuralSubscription(distribution=ubuntu,
-    ...     sourcepackagename=evolution, subscriber=sampleperson,
-    ...     subscribed_by=foobar)
+    >>> evolution = ubuntu.getSourcePackage('evolution')
+    >>> with person_logged_in(foobar):
+    ...     evolution_sub = evolution.addBugSubscription(
+    ...         subscriber=sampleperson, subscribed_by=foobar)
     >>> evolution_sub.target
     <...DistributionSourcePackage object at ...>
 

=== modified file 'lib/lp/bugs/model/bugsubscriptionfilter.py'
--- lib/lp/bugs/model/bugsubscriptionfilter.py	2011-02-01 04:57:42 +0000
+++ lib/lp/bugs/model/bugsubscriptionfilter.py	2011-02-18 18:19:02 +0000
@@ -190,7 +190,17 @@
         _get_tags, _set_tags, doc=(
             "A frozenset of tags filtered on."))
 
+    def _has_other_filters(self):
+        """Are there other filters for parent `StructuralSubscription`?"""
+        store = Store.of(self)
+        return bool(store.find(
+            BugSubscriptionFilter,
+            (BugSubscriptionFilter.structural_subscription ==
+             self.structural_subscription),
+            BugSubscriptionFilter.id != self.id).any())
+
     def delete(self):
         """See `IBugSubscriptionFilter`."""
         self.importances = self.statuses = self.tags = ()
-        Store.of(self).remove(self)
+        if self._has_other_filters():
+            Store.of(self).remove(self)

=== modified file 'lib/lp/bugs/model/structuralsubscription.py'
--- lib/lp/bugs/model/structuralsubscription.py	2011-02-01 19:06:18 +0000
+++ lib/lp/bugs/model/structuralsubscription.py	2011-02-18 18:19:02 +0000
@@ -177,6 +177,7 @@
             BugSubscriptionFilter.structural_subscription == self).remove()
         store.remove(self)
 
+
 class DistroSeriesTargetHelper:
     """A helper for `IDistroSeries`s."""
 
@@ -373,10 +374,12 @@
         if existing_subscription is not None:
             return existing_subscription
         else:
-            return StructuralSubscription(
+            new_subscription = StructuralSubscription(
                 subscriber=subscriber,
                 subscribed_by=subscribed_by,
                 **self._target_args)
+            subscription_filter = new_subscription.newBugFilter()
+            return new_subscription
 
     def userCanAlterBugSubscription(self, subscriber, subscribed_by):
         """See `IStructuralSubscriptionTarget`."""

=== modified file 'lib/lp/bugs/model/tests/test_bug.py'
--- lib/lp/bugs/model/tests/test_bug.py	2011-02-03 08:38:27 +0000
+++ lib/lp/bugs/model/tests/test_bug.py	2011-02-18 18:19:02 +0000
@@ -5,7 +5,6 @@
 
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.bugs.enum import BugNotificationLevel
-from lp.bugs.model.structuralsubscription import StructuralSubscription
 from lp.bugs.model.bug import BugSubscriptionInfo
 from lp.registry.interfaces.person import PersonVisibility
 from lp.testing import (
@@ -90,8 +89,8 @@
         member = self.factory.makePerson()
         team = self.factory.makeTeam(
             owner=member, visibility=PersonVisibility.PRIVATE)
-        StructuralSubscription(
-            product=product, subscriber=team, subscribed_by=member)
+        with person_logged_in(member):
+            product.addSubscription(team, member)
         self.assertTrue(team in bug.getAlsoNotifiedSubscribers())
 
     def test_get_indirect_subscribers_with_private_team(self):
@@ -100,8 +99,8 @@
         member = self.factory.makePerson()
         team = self.factory.makeTeam(
             owner=member, visibility=PersonVisibility.PRIVATE)
-        StructuralSubscription(
-            product=product, subscriber=team, subscribed_by=member)
+        with person_logged_in(member):
+            product.addSubscription(team, member)
         self.assertTrue(team in bug.getIndirectSubscribers())
 
     def test_get_direct_subscribers_with_private_team(self):

=== modified file 'lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py'
--- lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py	2011-02-01 04:57:42 +0000
+++ lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py	2011-02-18 18:19:02 +0000
@@ -7,7 +7,7 @@
 
 from storm.store import Store
 from zope.security.interfaces import Unauthorized
-from zope.security.proxy import ProxyFactory
+from zope.security.proxy import ProxyFactory, removeSecurityProxy
 
 from canonical.launchpad import searchbuilder
 from canonical.launchpad.interfaces.lpstorm import IStore
@@ -85,6 +85,19 @@
         self.assertIs(None, bug_subscription_filter.other_parameters)
         self.assertIs(None, bug_subscription_filter.description)
 
+    def test_has_other_filters_one(self):
+        # With only the initial, default filter, it returns False.
+        initial_filter = self.subscription.bug_filters.one()
+        naked_filter = removeSecurityProxy(initial_filter)
+        self.assertFalse(naked_filter._has_other_filters())
+
+    def test_has_other_filters_more_than_one(self):
+        # With more than one filter, it returns True.
+        bug_subscription_filter = BugSubscriptionFilter()
+        bug_subscription_filter.structural_subscription = self.subscription
+        naked_filter = removeSecurityProxy(bug_subscription_filter)
+        self.assertTrue(naked_filter._has_other_filters())
+
     def test_delete(self):
         """`BugSubscriptionFilter` objects can be deleted.
 
@@ -103,6 +116,23 @@
         IStore(bug_subscription_filter).flush()
         self.assertIs(None, Store.of(bug_subscription_filter))
 
+    def test_delete_final(self):
+        # Final remaining `BugSubscriptionFilter` can't be deleted.
+        # Only the linked data is removed.
+        bug_subscription_filter = self.subscription.bug_filters.one()
+        bug_subscription_filter.importances = [BugTaskImportance.LOW]
+        bug_subscription_filter.statuses = [BugTaskStatus.NEW]
+        bug_subscription_filter.tags = [u"foo"]
+        IStore(bug_subscription_filter).flush()
+        self.assertIsNot(None, Store.of(bug_subscription_filter))
+        # Delete.
+        bug_subscription_filter.delete()
+        IStore(bug_subscription_filter).flush()
+        self.assertIsNot(None, Store.of(bug_subscription_filter))
+        self.assertContentEqual([], bug_subscription_filter.statuses)
+        self.assertContentEqual([], bug_subscription_filter.importances)
+        self.assertContentEqual([], bug_subscription_filter.tags)
+
     def test_statuses(self):
         # The statuses property is a frozenset of the statuses that are
         # filtered upon.

=== modified file 'lib/lp/bugs/tests/test_structuralsubscription.py'
--- lib/lp/bugs/tests/test_structuralsubscription.py	2011-02-01 20:33:30 +0000
+++ lib/lp/bugs/tests/test_structuralsubscription.py	2011-02-18 18:19:02 +0000
@@ -27,6 +27,7 @@
         with person_logged_in(self.product.owner):
             self.subscription = self.product.addSubscription(
                 self.product.owner, self.product.owner)
+        self.original_filter = self.subscription.bug_filters[0]
 
     def test_delete_requires_Edit_permission(self):
         # delete() is only available to the subscriber.
@@ -62,21 +63,22 @@
                 store.find(
                     BugSubscriptionFilter,
                     BugSubscriptionFilter.structural_subscription_id ==
-                        subscription_id
-                    ).one(),
+                        subscription_id).one(),
                 None)
 
-    def test_bug_filters_empty(self):
-        # The bug_filters attribute is empty to begin with.
-        self.assertEqual([], list(self.subscription.bug_filters))
+    def test_bug_filters_default(self):
+        # The bug_filters attribute has a default non-filtering bug filter
+        # to begin with.
+        self.assertEqual([self.original_filter],
+                         list(self.subscription.bug_filters))
 
     def test_bug_filters(self):
         # The bug_filters attribute returns the BugSubscriptionFilter records
         # associated with this subscription.
         subscription_filter = BugSubscriptionFilter()
         subscription_filter.structural_subscription = self.subscription
-        self.assertEqual(
-            [subscription_filter],
+        self.assertContentEqual(
+            [subscription_filter, self.original_filter],
             list(self.subscription.bug_filters))
 
     def test_newBugFilter(self):
@@ -87,8 +89,8 @@
         self.assertEqual(
             self.subscription,
             subscription_filter.structural_subscription)
-        self.assertEqual(
-            [subscription_filter],
+        self.assertContentEqual(
+            [subscription_filter, self.original_filter],
             list(self.subscription.bug_filters))
 
     def test_newBugFilter_by_anonymous(self):

=== modified file 'lib/lp/bugs/tests/test_structuralsubscriptiontarget.py'
--- lib/lp/bugs/tests/test_structuralsubscriptiontarget.py	2011-02-01 04:57:42 +0000
+++ lib/lp/bugs/tests/test_structuralsubscriptiontarget.py	2011-02-18 18:19:02 +0000
@@ -192,6 +192,7 @@
         self.bug = self.bugtask.bug
         self.subscription = self.target.addSubscription(
             self.ordinary_subscriber, self.ordinary_subscriber)
+        self.initial_filter = self.subscription.bug_filters[0]
 
     def assertSubscriptions(
         self, expected_subscriptions, level=BugNotificationLevel.NOTHING):
@@ -214,14 +215,13 @@
         self.assertSubscriptions([self.subscription])
 
         # Filter the subscription to bugs in the CONFIRMED state.
-        subscription_filter = self.subscription.newBugFilter()
-        subscription_filter.statuses = [BugTaskStatus.CONFIRMED]
+        self.initial_filter.statuses = [BugTaskStatus.CONFIRMED]
 
         # With the filter the subscription is not found.
         self.assertSubscriptions([])
 
         # If the filter is adjusted, the subscription is found again.
-        subscription_filter.statuses = [self.bugtask.status]
+        self.initial_filter.statuses = [self.bugtask.status]
         self.assertSubscriptions([self.subscription])
 
     def test_getSubscriptionsForBugTask_with_filter_on_importance(self):
@@ -232,14 +232,13 @@
         self.assertSubscriptions([self.subscription])
 
         # Filter the subscription to bugs in the CRITICAL state.
-        subscription_filter = self.subscription.newBugFilter()
-        subscription_filter.importances = [BugTaskImportance.CRITICAL]
+        self.initial_filter.importances = [BugTaskImportance.CRITICAL]
 
         # With the filter the subscription is not found.
         self.assertSubscriptions([])
 
         # If the filter is adjusted, the subscription is found again.
-        subscription_filter.importances = [self.bugtask.importance]
+        self.initial_filter.importances = [self.bugtask.importance]
         self.assertSubscriptions([self.subscription])
 
     def test_getSubscriptionsForBugTask_with_filter_on_level(self):
@@ -247,8 +246,8 @@
         # which getSubscriptionsForBugTask() observes.
 
         # Adjust the subscription level to METADATA.
-        filter = self.subscription.newBugFilter()
-        filter.bug_notification_level = BugNotificationLevel.METADATA
+        self.initial_filter.bug_notification_level = (
+            BugNotificationLevel.METADATA)
 
         # The subscription is found when looking for NOTHING or above.
         self.assertSubscriptions(
@@ -264,8 +263,7 @@
         # If a subscription filter has include_any_tags, a bug with one or
         # more tags is matched.
 
-        subscription_filter = self.subscription.newBugFilter()
-        subscription_filter.include_any_tags = True
+        self.initial_filter.include_any_tags = True
 
         # Without any tags the subscription is not found.
         self.assertSubscriptions([])
@@ -278,8 +276,7 @@
         # If a subscription filter has exclude_any_tags, only bugs with no
         # tags are matched.
 
-        subscription_filter = self.subscription.newBugFilter()
-        subscription_filter.exclude_any_tags = True
+        self.initial_filter.exclude_any_tags = True
 
         # Without any tags the subscription is found.
         self.assertSubscriptions([self.subscription])
@@ -293,9 +290,8 @@
         # 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
+        self.initial_filter.tags = [u"foo", u"bar"]
+        self.initial_filter.find_all_tags = False
 
         # Without either tag the subscription is not found.
         self.assertSubscriptions([])
@@ -309,9 +305,8 @@
         # 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
+        self.initial_filter.tags = [u"foo", u"bar"]
+        self.initial_filter.find_all_tags = True
 
         # Without either tag the subscription is not found.
         self.assertSubscriptions([])
@@ -330,9 +325,8 @@
         # 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
+        self.initial_filter.tags = [u"-foo", u"-bar"]
+        self.initial_filter.find_all_tags = False
 
         # Without either tag the subscription is found.
         self.assertSubscriptions([self.subscription])
@@ -347,9 +341,8 @@
         # 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
+        self.initial_filter.tags = [u"-foo", u"-bar"]
+        self.initial_filter.find_all_tags = True
 
         # Without either tag the subscription is found.
         self.assertSubscriptions([self.subscription])
@@ -370,27 +363,26 @@
         self.bug.tags = ["foo"]
 
         # Filter the subscription to bugs in the CRITICAL state.
-        subscription_filter = self.subscription.newBugFilter()
-        subscription_filter.statuses = [BugTaskStatus.CONFIRMED]
-        subscription_filter.importances = [BugTaskImportance.CRITICAL]
+        self.initial_filter.statuses = [BugTaskStatus.CONFIRMED]
+        self.initial_filter.importances = [BugTaskImportance.CRITICAL]
 
         # With the filter the subscription is not found.
         self.assertSubscriptions([])
 
         # If the filter is adjusted to match status but not importance, the
         # subscription is still not found.
-        subscription_filter.statuses = [self.bugtask.status]
+        self.initial_filter.statuses = [self.bugtask.status]
         self.assertSubscriptions([])
 
         # If the filter is adjusted to also match importance, the subscription
         # is found again.
-        subscription_filter.importances = [self.bugtask.importance]
+        self.initial_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.initial_filter.tags = [u"-foo", u"bar", u"baz"]
+        self.initial_filter.find_all_tags = False
         self.assertSubscriptions([])
 
         # After removing the "foo" tag and adding the "bar" tag, the
@@ -400,7 +392,7 @@
 
         # Requiring that all tag criteria are fulfilled causes the
         # subscription to no longer be found.
-        subscription_filter.find_all_tags = True
+        self.initial_filter.find_all_tags = True
         self.assertSubscriptions([])
 
         # After adding the "baz" tag, the subscription is found again.
@@ -411,7 +403,7 @@
         # 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.
-        subscription_filter1 = self.subscription.newBugFilter()
+        subscription_filter1 = self.initial_filter
         subscription_filter1.statuses = [BugTaskStatus.CONFIRMED]
         subscription_filter2 = self.subscription.newBugFilter()
         subscription_filter2.tags = [u"foo"]

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2011-02-16 10:49:11 +0000
+++ lib/lp/registry/tests/test_person.py	2011-02-18 18:19:02 +0000
@@ -38,7 +38,6 @@
     )
 from lp.answers.model.answercontact import AnswerContact
 from lp.blueprints.model.specification import Specification
-from lp.bugs.model.structuralsubscription import StructuralSubscription
 from lp.bugs.interfaces.bugtask import IllegalRelatedBugTasksParams
 from lp.bugs.model.bug import Bug
 from lp.bugs.model.bugtask import get_related_bugtasks_search_params
@@ -404,9 +403,7 @@
         # A PUBLIC team with a structural subscription to a product can
         # convert to a PRIVATE team.
         foo_bar = Person.byName('name16')
-        StructuralSubscription(
-            product=self.bzr, subscriber=self.otherteam,
-            subscribed_by=foo_bar)
+        self.bzr.addSubscription(self.otherteam, foo_bar)
         self.otherteam.visibility = PersonVisibility.PRIVATE
 
     def test_visibility_validator_team_private_to_public(self):