Merge lp:~gary/launchpad/move-events-to-filters into lp:launchpad/db-devel

[Gary Poster]

[r=allenap,gmb,stub][ui=none][bug=711376] move the event filter enumeration off of the structural subscription and on to the filter object.

Part of https://dev.launchpad.net/LEP/BetterBugSubscriptionsAndNotifications . It is a necessary step to providing the functionality described in the "Must" section of the LEP.

This branch moves the event filter enumeration off of the structural subscription, and on to the filter object. Combined with Gavin's filter work, this makes it possible to, for instance, subscribe to all bug notifications for a project or package with a "ui" tag and also subscribe to only creation/closing bug notifications with a "bugjam" tag.

"make lint" only complains about some circular dependencies that are not an issue with my changes to my knowledge.

All tests I touched pass for me. ec2 is currently running tests for this branch.

This branch is large but I'm not sure how I would have made it smaller. If you'd like to talk it over with me maybe we could come up with a way to divide it?

Thank you

Gary

[Gary Poster (gary)]

A worry: target.addSubscription did result in a subscription that had a level of NOTHING. We never exposed that, to my knowledge, instead relying publicly on addBugSubscription, which always explicitly set a level of COMMENTS. Now, this branch creates subscriptions without filters, which means addBugSubscription has the same result, but addSubscription does not. I don't think it is an issue, but I'd like a sanity check from an expert (gmb? :-D )

[Gary Poster (gary)]

There's one more thing I hope will get someone's attention: the XXX I added. I intend to remove it after discussion and resolution in this review.

Here's the concern. The filter class (lp.bugs.model.bugsubscriptionfilter.BugSubscriptionFilter) has an explicit "delete" method that will delete all of its subobjects (that is, records in the database in a many to one relationship with the filter class). However, the structural subscriptions do not have any similar code for the filters, which are themselves many to one with the structural subscriptions. Why? Is the delete method in BugSubscriptionFilter overkill because we have cascading delete in the database, or do I need to do something similar to the delete method on structural subscriptions? This seems like an elementary question for LP development, but not something I've encountered yet.

Thanks.

[Stuart Bishop (stub)]

Fine. patch-2208-37-0.sql

review: Approve (db)

[Graham Binns (gmb)]

> A worry: target.addSubscription did result in a subscription that had a level
> of NOTHING. We never exposed that, to my knowledge, instead relying publicly
> on addBugSubscription, which always explicitly set a level of COMMENTS. Now,
> this branch creates subscriptions without filters, which means
> addBugSubscription has the same result, but addSubscription does not. I don't
> think it is an issue, but I'd like a sanity check from an expert (gmb? :-D )

I don't think that's going to be an issue (and I'd like to think that tests will blow up if it is).

[Graham Binns (gmb)]

> There's one more thing I hope will get someone's attention: the XXX I added.
> I intend to remove it after discussion and resolution in this review.
>
> Here's the concern. The filter class
> (lp.bugs.model.bugsubscriptionfilter.BugSubscriptionFilter) has an explicit
> "delete" method that will delete all of its subobjects (that is, records in
> the database in a many to one relationship with the filter class). However,
> the structural subscriptions do not have any similar code for the filters,
> which are themselves many to one with the structural subscriptions. Why? Is
> the delete method in BugSubscriptionFilter overkill because we have cascading
> delete in the database, or do I need to do something similar to the delete
> method on structural subscriptions? This seems like an elementary question
> for LP development, but not something I've encountered yet.

I don't know the answer to this, I'm afraid. I don't see anything in the DB that suggests we have a cascading delete set up in the database, and IIRC you'll get an OOPS if you try to delete something that has other DB rows hanging off it. My best suggestion would be to write a test for this and see whether smoke comes out.

[Graham Binns (gmb)]

Hi Gary,

So, I'm broadly-speaking +1 on this branch landing (note that I'm on a fairly narrow bandwidth at the moment, so I've gone straight for understanding the code and not worried too much about formatting issues). However, I don't feel that I have enough knowledge about the way that filters work at the moment to offer a definite r=me. I'd suggest you ask Gavin to cast a weather eye over it too, since the filters were at least in part his work.

Anyway, approval from me. Nice work; I don't think I can see a way to break it up without causing some heartache, so don't worry about the branch size.

review: Approve (code)

[Gavin Panella (allenap)]

This is really cool :) I have an answer for your specific question in
[2], and one other trivial comment.

[1]

+ LaunchpadEditFormView.setUpFields(self)

Consider using super() here.

[2]

+ # XXX Delete all associated filters.
         store = Store.of(subscription_to_remove)
         store.remove(subscription_to_remove)

Mucking about in bin/iharness shows that the filters need to be
explicitly deleted too.

> Is the delete method in BugSubscriptionFilter overkill because we
> have cascading delete in the database, or do I need to do something
> similar to the delete method on structural subscriptions?

Yes :) I never thought to put it in before.

I talked briefly with Aaron about this before. There isn't a clear
story for deleting stuff in Launchpad. We have destroySelf() in some
places, a hang-over from SQLObject, and delete() in other places, and
many places do store.remove(thing) without thinking about it.

It might work to have a __storm_remove__ hook so that things like
BugSubscriptionFilter can take responsibility of its
dependencies. Then we would always use store.remove(). However, that
doesn't work from view code which must not speak of the store. I
suspect a mishmash will rule.

review: Approve (code)

[Gary Poster (gary)]

Thank you for the reviews!

I have changed to use super (I was too lazy at the time to verify that the base class was new-style; in the LP code base, I suppose I should have assumed yes).

In regards to the XXX, I want to do this in a separate branch, since this one was so gigantic. It also is a bug outside of the code I added in this branch, which I'm using to further rationalize the decision. :-) I created bug 711362 and will be beginning a branch for it (with tests as Graham suggested and a delete method as Gavin suggested) within the hour.

=== modified file 'database/sampledata/current-dev.sql'
--- database/sampledata/current-dev.sql	2011-01-18 21:20:48 +0000
+++ database/sampledata/current-dev.sql	2011-02-01 19:35:24 +0000
@@ -3694,10 +3694,10 @@
 
 ALTER TABLE structuralsubscription DISABLE TRIGGER ALL;
 
-INSERT INTO structuralsubscription (id, product, productseries, project, milestone, distribution, distroseries, sourcepackagename, subscriber, subscribed_by, bug_notification_level, date_created, date_last_updated) VALUES (1, NULL, NULL, NULL, NULL, 1, NULL, 1, 16, 16, 40, '2008-01-29 15:12:34.581468', '2008-01-29 15:12:34.581468');
-INSERT INTO structuralsubscription (id, product, productseries, project, milestone, distribution, distroseries, sourcepackagename, subscriber, subscribed_by, bug_notification_level, date_created, date_last_updated) VALUES (2, NULL, NULL, NULL, NULL, 1, NULL, 14, 16, 16, 40, '2008-01-29 15:12:34.581468', '2008-01-29 15:12:34.581468');
-INSERT INTO structuralsubscription (id, product, productseries, project, milestone, distribution, distroseries, sourcepackagename, subscriber, subscribed_by, bug_notification_level, date_created, date_last_updated) VALUES (3, 22, NULL, NULL, NULL, NULL, NULL, NULL, 64, 64, 40, '2008-02-06 12:17:13.030376', '2008-02-06 12:17:13.030376');
-INSERT INTO structuralsubscription (id, product, productseries, project, milestone, distribution, distroseries, sourcepackagename, subscriber, subscribed_by, bug_notification_level, date_created, date_last_updated) VALUES (4, 16, NULL, NULL, NULL, NULL, NULL, NULL, 64, 64, 40, '2008-02-06 12:17:13.030376', '2008-02-06 12:17:13.030376');
+INSERT INTO structuralsubscription (id, product, productseries, project, milestone, distribution, distroseries, sourcepackagename, subscriber, subscribed_by, date_created, date_last_updated) VALUES (1, NULL, NULL, NULL, NULL, 1, NULL, 1, 16, 16, '2008-01-29 15:12:34.581468', '2008-01-29 15:12:34.581468');
+INSERT INTO structuralsubscription (id, product, productseries, project, milestone, distribution, distroseries, sourcepackagename, subscriber, subscribed_by, date_created, date_last_updated) VALUES (2, NULL, NULL, NULL, NULL, 1, NULL, 14, 16, 16, '2008-01-29 15:12:34.581468', '2008-01-29 15:12:34.581468');
+INSERT INTO structuralsubscription (id, product, productseries, project, milestone, distribution, distroseries, sourcepackagename, subscriber, subscribed_by, date_created, date_last_updated) VALUES (3, 22, NULL, NULL, NULL, NULL, NULL, NULL, 64, 64, '2008-02-06 12:17:13.030376', '2008-02-06 12:17:13.030376');
+INSERT INTO structuralsubscription (id, product, productseries, project, milestone, distribution, distroseries, sourcepackagename, subscriber, subscribed_by, date_created, date_last_updated) VALUES (4, 16, NULL, NULL, NULL, NULL, NULL, NULL, 64, 64, '2008-02-06 12:17:13.030376', '2008-02-06 12:17:13.030376');
 
 
 ALTER TABLE structuralsubscription ENABLE TRIGGER ALL;

=== modified file 'database/sampledata/current.sql'
--- database/sampledata/current.sql	2011-01-18 21:20:48 +0000
+++ database/sampledata/current.sql	2011-02-01 19:35:24 +0000
@@ -3694,10 +3694,10 @@
 
 ALTER TABLE structuralsubscription DISABLE TRIGGER ALL;
 
-INSERT INTO structuralsubscription (id, product, productseries, project, milestone, distribution, distroseries, sourcepackagename, subscriber, subscribed_by, bug_notification_level, date_created, date_last_updated) VALUES (1, NULL, NULL, NULL, NULL, 1, NULL, 1, 16, 16, 40, '2008-01-29 15:12:34.581468', '2008-01-29 15:12:34.581468');
-INSERT INTO structuralsubscription (id, product, productseries, project, milestone, distribution, distroseries, sourcepackagename, subscriber, subscribed_by, bug_notification_level, date_created, date_last_updated) VALUES (2, NULL, NULL, NULL, NULL, 1, NULL, 14, 16, 16, 40, '2008-01-29 15:12:34.581468', '2008-01-29 15:12:34.581468');
-INSERT INTO structuralsubscription (id, product, productseries, project, milestone, distribution, distroseries, sourcepackagename, subscriber, subscribed_by, bug_notification_level, date_created, date_last_updated) VALUES (3, 22, NULL, NULL, NULL, NULL, NULL, NULL, 64, 64, 40, '2008-02-06 12:17:13.030376', '2008-02-06 12:17:13.030376');
-INSERT INTO structuralsubscription (id, product, productseries, project, milestone, distribution, distroseries, sourcepackagename, subscriber, subscribed_by, bug_notification_level, date_created, date_last_updated) VALUES (4, 16, NULL, NULL, NULL, NULL, NULL, NULL, 64, 64, 40, '2008-02-06 12:17:13.030376', '2008-02-06 12:17:13.030376');
+INSERT INTO structuralsubscription (id, product, productseries, project, milestone, distribution, distroseries, sourcepackagename, subscriber, subscribed_by, date_created, date_last_updated) VALUES (1, NULL, NULL, NULL, NULL, 1, NULL, 1, 16, 16, '2008-01-29 15:12:34.581468', '2008-01-29 15:12:34.581468');
+INSERT INTO structuralsubscription (id, product, productseries, project, milestone, distribution, distroseries, sourcepackagename, subscriber, subscribed_by, date_created, date_last_updated) VALUES (2, NULL, NULL, NULL, NULL, 1, NULL, 14, 16, 16, '2008-01-29 15:12:34.581468', '2008-01-29 15:12:34.581468');
+INSERT INTO structuralsubscription (id, product, productseries, project, milestone, distribution, distroseries, sourcepackagename, subscriber, subscribed_by, date_created, date_last_updated) VALUES (3, 22, NULL, NULL, NULL, NULL, NULL, NULL, 64, 64, '2008-02-06 12:17:13.030376', '2008-02-06 12:17:13.030376');
+INSERT INTO structuralsubscription (id, product, productseries, project, milestone, distribution, distroseries, sourcepackagename, subscriber, subscribed_by, date_created, date_last_updated) VALUES (4, 16, NULL, NULL, NULL, NULL, NULL, NULL, 64, 64, '2008-02-06 12:17:13.030376', '2008-02-06 12:17:13.030376');
 
 
 ALTER TABLE structuralsubscription ENABLE TRIGGER ALL;

=== modified file 'database/schema/comments.sql'
--- database/schema/comments.sql	2011-01-18 21:20:48 +0000
+++ database/schema/comments.sql	2011-02-01 19:35:24 +0000
@@ -208,6 +208,7 @@
 -- BugSubscriptionFilter
 COMMENT ON TABLE BugSubscriptionFilter IS 'A filter with search criteria. Emails are sent only if the affected bug matches the specified parameters. The parameters are the same as those used for bugtask searches.';
 COMMENT ON COLUMN BugSubscriptionFilter.structuralsubscription IS 'The structural subscription to be filtered.';
+COMMENT ON COLUMN BugSubscriptionFilter.bug_notification_level IS 'The volume and type of bug notifications this filter will allow. The value is an item of the enumeration `BugNotificationLevel`.';
 COMMENT ON COLUMN BugSubscriptionFilter.find_all_tags IS 'If set, search for bugs having all tags specified in BugSubscriptionFilterTag, else search for bugs having any of the tags specified in BugSubscriptionFilterTag.';
 COMMENT ON COLUMN BugSubscriptionFilter.include_any_tags IS 'If True, include messages for bugs having any tag set.';
 COMMENT ON COLUMN BugSubscriptionFilter.exclude_any_tags IS 'If True, exclude bugs having any tag set.';
@@ -2394,7 +2395,6 @@
 COMMENT ON COLUMN StructuralSubscription.sourcepackagename IS 'The subscription\`s target, when it is a source-package';
 COMMENT ON COLUMN StructuralSubscription.subscriber IS 'The person subscribed.';
 COMMENT ON COLUMN StructuralSubscription.subscribed_by IS 'The person initiating the subscription.';
-COMMENT ON COLUMN StructuralSubscription.bug_notification_level IS 'The volume and type of bug notifications this subscription will generate. The value is an item of the enumeration `BugNotificationLevel`.';
 COMMENT ON COLUMN StructuralSubscription.date_created IS 'The date on which this subscription was created.';
 COMMENT ON COLUMN StructuralSubscription.date_last_updated IS 'The date on which this subscription was last updated.';
 

=== added file 'database/schema/patch-2208-37-0.sql'
--- database/schema/patch-2208-37-0.sql	1970-01-01 00:00:00 +0000
+++ database/schema/patch-2208-37-0.sql	2011-02-01 19:35:24 +0000
@@ -0,0 +1,12 @@
+-- Copyright 2011 Canonical Ltd.  This software is licensed under the
+-- GNU Affero General Public License version 3 (see the file LICENSE).
+SET client_min_messages=ERROR;
+
+ALTER TABLE StructuralSubscription
+    DROP COLUMN bug_notification_level;
+ALTER TABLE BugSubscriptionFilter
+    ADD COLUMN bug_notification_level
+        integer DEFAULT 40 NOT NULL;
+CREATE INDEX bugsubscriptionfilter__bug_notification_level__idx ON bugsubscriptionfilter USING btree (bug_notification_level);
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 37, 0);

=== modified file 'lib/canonical/launchpad/mail/helpers.py'
--- lib/canonical/launchpad/mail/helpers.py	2010-11-25 04:42:51 +0000
+++ lib/canonical/launchpad/mail/helpers.py	2011-02-01 19:35:24 +0000
@@ -112,11 +112,7 @@
                     # Is the person one of the package subscribers?
                     bug_sub = bugtask.target.getSubscription(person)
                     if bug_sub is not None:
-                        if (bug_sub.bug_notification_level >
-                            BugNotificationLevel.NOTHING):
-                            # The user is subscribed to bug notifications
-                            # for this package
-                            return bugtask
+                        return bugtask
     return None
 
 

=== modified file 'lib/lp/bugs/browser/bugsubscriptionfilter.py'
--- lib/lp/bugs/browser/bugsubscriptionfilter.py	2011-01-14 10:26:37 +0000
+++ lib/lp/bugs/browser/bugsubscriptionfilter.py	2011-02-01 19:35:24 +0000
@@ -22,7 +22,10 @@
     custom_widget,
     LaunchpadEditFormView,
     )
+from lp.registry.enum import BugNotificationLevel
+from lp.services.propertycache import cachedproperty
 from lp.bugs.browser.widgets.bug import BugTagsFrozenSetWidget
+from lp.bugs.browser.bugsubscription import AdvancedSubscriptionMixin
 from lp.bugs.interfaces.bugsubscriptionfilter import IBugSubscriptionFilter
 
 
@@ -74,7 +77,8 @@
         return conditions
 
 
-class BugSubscriptionFilterEditViewBase(LaunchpadEditFormView):
+class BugSubscriptionFilterEditViewBase(LaunchpadEditFormView,
+                                        AdvancedSubscriptionMixin):
     """Base class for edit or create views of `IBugSubscriptionFilter`."""
 
     schema = IBugSubscriptionFilter
@@ -91,6 +95,33 @@
     custom_widget("importances", LabeledMultiCheckBoxWidget)
     custom_widget("tags", BugTagsFrozenSetWidget, displayWidth=35)
 
+    target = None # Define in concrete subclass to be the target of the
+    # structural subscription that we are modifying.
+
+    # This is used by the AdvancedSubscriptionMixin.
+    current_user_subscription = None
+
+    @cachedproperty
+    def _bug_notification_level_descriptions(self):
+        displayname = self.target.displayname
+        return {
+            BugNotificationLevel.LIFECYCLE: (
+                "A bug in %s is fixed or re-opened." % displayname),
+            BugNotificationLevel.METADATA: (
+                "Any change is made to a bug in %s, other than a new "
+                "comment being added." % displayname),
+            BugNotificationLevel.COMMENTS: (
+                "A change is made or a new comment is added to a bug in %s."
+                % displayname),
+            }
+
+    def setUpFields(self):
+        """Set up fields for form.
+
+        Overrides the usual implementation to also set up bug notification."""
+        super(BugSubscriptionFilterEditViewBase, self).setUpFields()
+        self._setUpBugNotificationLevelField()
+
     @property
     def next_url(self):
         """Return to the user's structural subscriptions page."""
@@ -119,6 +150,15 @@
         """Delete the bug filter."""
         self.context.delete()
 
+    @property
+    def current_user_subscription(self):
+        """Return an object that has the value for bug_notification_level."""
+        return self.context
+
+    @property
+    def target(self):
+        return self.context.structural_subscription.target
+
 
 class BugSubscriptionFilterCreateView(
     BugSubscriptionFilterEditViewBase):
@@ -138,3 +178,7 @@
         """Create a new bug filter with the form data."""
         bug_filter = self.context.newBugFilter()
         self.updateContextFromData(data, context=bug_filter)
+
+    @property
+    def target(self):
+        return self.context.target

=== modified file 'lib/lp/bugs/browser/structuralsubscription.py'
--- lib/lp/bugs/browser/structuralsubscription.py	2011-01-21 08:12:29 +0000
+++ lib/lp/bugs/browser/structuralsubscription.py	2011-02-01 19:35:24 +0000
@@ -37,13 +37,11 @@
     custom_widget,
     LaunchpadFormView,
     )
-from lp.bugs.browser.bugsubscription import AdvancedSubscriptionMixin
 from lp.bugs.interfaces.structuralsubscription import (
     IStructuralSubscription,
     IStructuralSubscriptionForm,
     IStructuralSubscriptionTarget,
     )
-from lp.registry.enum import BugNotificationLevel
 from lp.registry.interfaces.distributionsourcepackage import (
     IDistributionSourcePackage,
     )
@@ -65,8 +63,7 @@
         return None
 
 
-class StructuralSubscriptionView(LaunchpadFormView,
-                                 AdvancedSubscriptionMixin):
+class StructuralSubscriptionView(LaunchpadFormView):
 
     """View class for structural subscriptions."""
 
@@ -77,21 +74,6 @@
 
     override_title_breadcrumbs = True
 
-    @cachedproperty
-    def _bug_notification_level_descriptions(self):
-        return {
-            BugNotificationLevel.LIFECYCLE: (
-                "A bug in %s is fixed or re-opened." %
-                self.context.displayname),
-            BugNotificationLevel.METADATA: (
-                "Any change is made to a bug in %s, other than a new "
-                "comment being added." %
-                self.context.displayname),
-            BugNotificationLevel.COMMENTS: (
-                "A change is made or a new comment is added to a bug in %s."
-                % self.context.displayname),
-            }
-
     @property
     def page_title(self):
         return 'Subscribe to Bugs in %s' % self.context.title
@@ -116,7 +98,6 @@
             remove_other = self._createRemoveOtherSubscriptionsField()
             if remove_other:
                 self.form_fields += form.Fields(remove_other)
-        self._setUpBugNotificationLevelField()
 
     def _createTeamSubscriptionsField(self):
         """Create field with a list of the teams the user is a member of.
@@ -204,21 +185,12 @@
         for the context target.
         """
         subscription = self.context.getSubscription(person)
-        if subscription is not None:
-            return (subscription.bug_notification_level >
-                    BugNotificationLevel.NOTHING)
-        else:
-            return False
+        return subscription is not None
 
     def currentUserIsSubscribed(self):
         """Return True, if the current user is subscribed."""
         return self.isSubscribed(self.user)
 
-    @cachedproperty
-    def current_user_subscription(self):
-        """Return the subscription of the current user."""
-        return self.context.getSubscription(self.user)
-
     def userCanAlter(self):
         if self.context.userCanAlterBugSubscription(self.user, self.user):
             return True
@@ -240,9 +212,7 @@
         is_subscribed = self.isSubscribed(self.user)
         subscribe = data['subscribe_me']
         if (not is_subscribed) and subscribe:
-            target.addBugSubscription(
-                self.user, self.user,
-                data.get('bug_notification_level', None))
+            target.addBugSubscription(self.user, self.user)
             self.request.response.addNotification(
                 'You have subscribed to "%s". You will now receive an '
                 'e-mail each time someone reports or changes one of '
@@ -271,8 +241,7 @@
             team for team in teams if self.isSubscribed(team))
 
         for team in form_selected_teams - subscriptions:
-            target.addBugSubscription(
-                team, self.user, data.get('bug_notification_level', None))
+            target.addBugSubscription(team, self.user)
             self.request.response.addNotification(
                 'The %s team will now receive an e-mail each time '
                 'someone reports or changes a public bug in "%s".' % (

=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py'
--- lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py	2011-01-21 08:12:29 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py	2011-02-01 19:35:24 +0000
@@ -28,7 +28,10 @@
 from lp.bugs.browser.structuralsubscription import (
     StructuralSubscriptionNavigation,
     )
+from lp.registry.enum import BugNotificationLevel
 from lp.testing import (
+    feature_flags,
+    set_feature_flag,
     anonymous_logged_in,
     login_person,
     normalize_whitespace,
@@ -435,6 +438,88 @@
         self.assertEqual([], list(self.subscription.bug_filters))
 
 
+class TestBugSubscriptionFilterAdvancedFeatures(TestCaseWithFactory):
+    """A base class for testing advanced structural subscription features."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestBugSubscriptionFilterAdvancedFeatures, self).setUp()
+        self.setUpTarget()
+        with feature_flags():
+            set_feature_flag(u'malone.advanced-subscriptions.enabled', u'on')
+
+    def setUpTarget(self):
+        self.target = self.factory.makeProduct()
+
+    def test_filter_uses_bug_notification_level(self):
+        # When advanced features are turned on for subscriptions a user
+        # can specify a bug_notification_level on the +filter form.
+        with feature_flags():
+            # We don't display BugNotificationLevel.NOTHING as an option.
+            displayed_levels = [
+                level for level in BugNotificationLevel.items
+                if level != BugNotificationLevel.NOTHING]
+            for level in displayed_levels:
+                person = self.factory.makePerson()
+                with person_logged_in(person):
+                    subscription = self.target.addBugSubscription(person, person)
+                    form = {
+                        "field.description": "New description",
+                        "field.statuses": ["NEW", "INCOMPLETE"],
+                        "field.importances": ["LOW", "MEDIUM"],
+                        "field.tags": u"foo bar",
+                        "field.find_all_tags": "on",
+                        'field.bug_notification_level': level.name,
+                        "field.actions.create": "Create",
+                        }
+                    view = create_initialized_view(
+                        subscription, name="+new-filter", form=form)
+
+                filters = subscription.bug_filters
+                self.assertEqual(filters.count(), 1)
+                self.assertEqual(
+                    level, filters[0].bug_notification_level,
+                    "Bug notification level of filter should be %s, "
+                    "is actually %s." % (
+                        level.name, filters[0].bug_notification_level.name))
+
+    def test_nothing_is_not_a_valid_level(self):
+        # BugNotificationLevel.NOTHING isn't considered valid when a
+        # user is subscribing via the web UI.
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            subscription = self.target.addBugSubscription(person, person)
+            form = {
+                "field.description": "New description",
+                "field.statuses": ["NEW", "INCOMPLETE"],
+                "field.importances": ["LOW", "MEDIUM"],
+                "field.tags": u"foo bar",
+                "field.find_all_tags": "on",
+                'field.bug_notification_level': BugNotificationLevel.NOTHING,
+                "field.actions.create": "Create",
+                }
+            with feature_flags():
+                view = create_initialized_view(
+                    subscription, name="+new-filter", form=form)
+                self.assertTrue(view.errors)
+
+    def test_extra_features_hidden_without_feature_flag(self):
+        # If the malone.advanced-subscriptions.enabled flag is turned
+        # off, the bug_notification_level field doesn't appear on the
+        # form.  This is actually not important for the filter, but when
+        # this test fails because we no longer rely on a feature flag, it
+        # can be a reminder to clean up the rest of this test to get
+        # rid of the feature flag code.
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            subscription = self.target.addBugSubscription(person, person)
+            view = create_initialized_view(subscription, name="+new-filter")
+            form_fields = view.form_fields
+            self.assertIs(
+                None, form_fields.get('bug_notification_level'))
+
+
 class TestBugSubscriptionFilterCreateView(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer

=== modified file 'lib/lp/bugs/browser/tests/test_structuralsubscription.py'
--- lib/lp/bugs/browser/tests/test_structuralsubscription.py	2011-01-21 08:12:29 +0000
+++ lib/lp/bugs/browser/tests/test_structuralsubscription.py	2011-02-01 19:35:24 +0000
@@ -33,11 +33,8 @@
 from lp.registry.browser.product import ProductNavigation
 from lp.registry.browser.productseries import ProductSeriesNavigation
 from lp.registry.browser.project import ProjectNavigation
-from lp.registry.enum import BugNotificationLevel
 from lp.testing import (
-    feature_flags,
     person_logged_in,
-    set_feature_flag,
     TestCaseWithFactory,
     ws_object,
     )
@@ -169,136 +166,6 @@
         self.navigation = DistributionSourcePackageNavigation
 
 
-class TestStructuralSubscriptionAdvancedFeaturesBase(TestCaseWithFactory):
-    """A base class for testing advanced structural subscription features."""
-
-    layer = LaunchpadFunctionalLayer
-
-    def setUp(self):
-        super(TestStructuralSubscriptionAdvancedFeaturesBase, self).setUp()
-        self.setUpTarget()
-        with feature_flags():
-            set_feature_flag(u'malone.advanced-subscriptions.enabled', u'on')
-
-    def setUpTarget(self):
-        self.target = self.factory.makeProduct()
-
-    def test_subscribe_uses_bug_notification_level(self):
-        # When advanced features are turned on for subscriptions a user
-        # can specify a bug_notification_level on the +subscribe form.
-        with feature_flags():
-            # We don't display BugNotificationLevel.NOTHING as an option.
-            displayed_levels = [
-                level for level in BugNotificationLevel.items
-                if level != BugNotificationLevel.NOTHING]
-            for level in displayed_levels:
-                person = self.factory.makePerson()
-                with person_logged_in(person):
-                    harness = LaunchpadFormHarness(
-                        self.target, StructuralSubscriptionView)
-                    form_data = {
-                        'field.subscribe_me': 'on',
-                        'field.bug_notification_level': level.name,
-                        }
-                    harness.submit('save', form_data)
-                    self.assertFalse(harness.hasErrors())
-
-                subscription = self.target.getSubscription(person)
-                self.assertEqual(
-                    level, subscription.bug_notification_level,
-                    "Bug notification level of subscription should be %s, "
-                    "is actually %s." % (
-                        level.name, subscription.bug_notification_level.name))
-
-    def test_subscribe_uses_bug_notification_level_for_teams(self):
-        # The bug_notification_level field is also used when subscribing
-        # a team.
-        with feature_flags():
-            displayed_levels = [
-                level for level in BugNotificationLevel.items
-                if level != BugNotificationLevel.NOTHING]
-            for level in displayed_levels:
-                person = self.factory.makePerson()
-                team = self.factory.makeTeam(owner=person)
-                with person_logged_in(person):
-                    harness = LaunchpadFormHarness(
-                        self.target, StructuralSubscriptionView)
-                    form_data = {
-                        'field.subscribe_me': '',
-                        'field.subscriptions_team': team.name,
-                        'field.bug_notification_level': level.name,
-                        }
-                    harness.submit('save', form_data)
-                    self.assertFalse(harness.hasErrors())
-
-                subscription = self.target.getSubscription(team)
-                self.assertEqual(
-                    level, subscription.bug_notification_level,
-                    "Bug notification level of subscription should be %s, "
-                    "is actually %s." % (
-                        level.name, subscription.bug_notification_level.name))
-
-    def test_nothing_is_not_a_valid_level(self):
-        # BugNotificationLevel.NOTHING isn't considered valid when a
-        # user is subscribing via the web UI.
-        person = self.factory.makePerson()
-        with feature_flags():
-            with person_logged_in(person):
-                harness = LaunchpadFormHarness(
-                    self.target, StructuralSubscriptionView)
-                form_data = {
-                    'field.subscribe_me': 'on',
-                    'field.bug_notification_level': (
-                        BugNotificationLevel.NOTHING),
-                    }
-                harness.submit('save', form_data)
-                self.assertTrue(harness.hasErrors())
-
-    def test_extra_features_hidden_without_feature_flag(self):
-        # If the malone.advanced-subscriptions.enabled flag is turned
-        # off, the bug_notification_level field doesn't appear on the
-        # form.
-        person = self.factory.makePerson()
-        with person_logged_in(person):
-            harness = LaunchpadFormHarness(
-                self.target, StructuralSubscriptionView)
-            form_fields = harness.view.form_fields
-            self.assertIs(
-                None, form_fields.get('bug_notification_level'))
-
-
-class TestProductSeriesAdvancedSubscriptionFeatures(
-    TestStructuralSubscriptionAdvancedFeaturesBase):
-    """Test advanced subscription features for ProductSeries."""
-
-    def setUpTarget(self):
-        self.target = self.factory.makeProductSeries()
-
-
-class TestDistributionAdvancedSubscriptionFeatures(
-    TestStructuralSubscriptionAdvancedFeaturesBase):
-    """Test advanced subscription features for distributions."""
-
-    def setUpTarget(self):
-        self.target = self.factory.makeDistribution()
-
-
-class TestDistroSeriesAdvancedSubscriptionFeatures(
-    TestStructuralSubscriptionAdvancedFeaturesBase):
-    """Test advanced subscription features for DistroSeries."""
-
-    def setUpTarget(self):
-        self.target = self.factory.makeDistroSeries()
-
-
-class TestMilestoneAdvancedSubscriptionFeatures(
-    TestStructuralSubscriptionAdvancedFeaturesBase):
-    """Test advanced subscription features for Milestones."""
-
-    def setUpTarget(self):
-        self.target = self.factory.makeMilestone()
-
-
 class TestStructuralSubscriptionView(TestCaseWithFactory):
     """General tests for the StructuralSubscriptionView."""
 

=== modified file 'lib/lp/bugs/doc/bug-change.txt'
--- lib/lp/bugs/doc/bug-change.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/bugs/doc/bug-change.txt	2011-02-01 19:35:24 +0000
@@ -140,6 +140,7 @@
 bug's target for Meta data changes, but not for lifecycle changes.
 
 
+    >>> from lp.testing import person_logged_in
     >>> from lp.registry.enum import BugNotificationLevel
     >>> lifecycle_subscriber = factory.makePerson(
     ...         displayname='Lifecycle subscriber')
@@ -147,10 +148,14 @@
     ...         displayname='Meta-data subscriber')
     >>> subscription = example_bug.bugtasks[0].target.addBugSubscription(
     ...     lifecycle_subscriber, lifecycle_subscriber)
-    >>> subscription.bug_notification_level = BugNotificationLevel.LIFECYCLE
+    >>> with person_logged_in(lifecycle_subscriber):
+    ...     filter = subscription.newBugFilter()
+    ...     filter.bug_notification_level = BugNotificationLevel.LIFECYCLE
     >>> subscription = example_bug.bugtasks[0].target.addBugSubscription(
     ...     metadata_subscriber, metadata_subscriber)
-    >>> subscription.bug_notification_level = BugNotificationLevel.METADATA
+    >>> with person_logged_in(metadata_subscriber):
+    ...     filter = subscription.newBugFilter()
+    ...     filter.bug_notification_level = BugNotificationLevel.METADATA
     >>> example_bug.addChange(
     ...     TestBugChange(when=nowish, person=example_person))
     >>> latest_notification = BugNotification.selectFirst(orderBy='-id')

=== modified file 'lib/lp/bugs/doc/bugnotification-sending.txt'
--- lib/lp/bugs/doc/bugnotification-sending.txt	2011-01-06 14:44:56 +0000
+++ lib/lp/bugs/doc/bugnotification-sending.txt	2011-02-01 19:35:24 +0000
@@ -1118,12 +1118,74 @@
     >>> switch_db_to_bugnotification()
 
 The notifications generated by addCommentNotification() are sent only to
-structural subscribers with the notification level COMMENTS or higher.
-The notification level of Sample Person is COMMENTS, hence he receives
-these notifications.
-
-    >>> print subscription_no_priv.bug_notification_level.name
-    COMMENTS
+structural subscribers with no filters, or with the notification level
+of COMMENTS or higher. Sample Person's subscription currently does not
+have any filters, so he receives these notifications.
+
+    >>> print subscription_no_priv.bug_filters.count()
+    0
+    >>> comment = getUtility(IMessageSet).fromText(
+    ...     'subject', 'another comment.', sample_person,
+    ...     datecreated=ten_minutes_ago)
+    >>> bug_one.addCommentNotification(comment)
+    >>> pending_notifications = getUtility(
+    ...     IBugNotificationSet).getNotificationsToSend()
+    >>> email_notifications = get_email_notifications(pending_notifications)
+    >>> for bug_notifications, messages in email_notifications:
+    ...     for message in messages:
+    ...         print_notification(message)
+    To: [e-mail]
+    ...
+    You received this bug notification because you are subscribed to
+    mozilla-firefox in ubuntu.
+    ...
+    ----------------------------------------------------------------------
+    To: [e-mail]
+    ...
+    You received this bug notification because you are a member of ShipIt
+    Administrators, which is a direct subscriber.
+    ...
+    ----------------------------------------------------------------------
+    To: [e-mail]
+    ...
+    You received this bug notification because you are a bug assignee.
+    ...
+    ----------------------------------------------------------------------
+    To: [e-mail]
+    From: Sample Person <[e-mail]>
+    Subject: [Bug 1] subject
+    X-Launchpad-Message-Rationale: Subscriber (Mozilla Firefox)
+    <BLANKLINE>
+    another comment.
+    <BLANKLINE>
+    --
+    You received this bug notification because you are subscribed to Mozilla
+    Firefox.
+    ...
+    ----------------------------------------------------------------------
+    To: [e-mail]
+    ...
+    You received this bug notification because you are a member of Ubuntu
+    Team, which is the registrant for Ubuntu.
+    ...
+    ----------------------------------------------------------------------
+    To: [e-mail]
+    ...
+    You received this bug notification because you are a direct subscriber
+    of the bug.
+    ...
+    ----------------------------------------------------------------------
+
+If Sample Person gets a filter with an explicit notification level of
+COMMENTS, he also receives these notifications.
+
+
+    >>> flush_notifications()
+    >>> switch_db_to_launchpad()
+    >>> filter = subscription_no_priv.newBugFilter()
+    >>> filter.bug_notification_level = BugNotificationLevel.COMMENTS
+    >>> switch_db_to_bugnotification()
+
     >>> comment = getUtility(IMessageSet).fromText(
     ...     'subject', 'another comment.', sample_person,
     ...     datecreated=ten_minutes_ago)
@@ -1181,8 +1243,7 @@
 
     >>> flush_notifications()
     >>> switch_db_to_launchpad()
-    >>> subscription_no_priv.bug_notification_level = (
-    ...     BugNotificationLevel.METADATA)
+    >>> filter.bug_notification_level = BugNotificationLevel.METADATA
     >>> switch_db_to_bugnotification()
 
     >>> comment = getUtility(IMessageSet).fromText(
@@ -1294,8 +1355,7 @@
 
     >>> flush_notifications()
     >>> switch_db_to_launchpad()
-    >>> subscription_no_priv.bug_notification_level = (
-    ...     BugNotificationLevel.LIFECYCLE)
+    >>> filter.bug_notification_level = BugNotificationLevel.LIFECYCLE
     >>> switch_db_to_bugnotification()
 
     >>> bug_one.addChangeNotification('** Summary changed to: something.',
@@ -1346,3 +1406,66 @@
     of the bug.
     ...
     ----------------------------------------------------------------------
+
+Note that, if two filters exist and they both match the same bug, the
+more inclusive filter wins.  Therefore, while we saw before that the
+filter did not allow the change notification through, if we add another
+filter that includes metadata then the notification will be sent out
+after all.
+
+    >>> flush_notifications()
+    >>> switch_db_to_launchpad()
+    >>> filter2 = subscription_no_priv.newBugFilter()
+    >>> filter2.bug_notification_level = BugNotificationLevel.METADATA
+    >>> switch_db_to_bugnotification()
+
+    >>> bug_one.addChangeNotification('** Summary changed to: whatever.',
+    ...     sample_person, when=ten_minutes_ago)
+    >>> pending_notifications = getUtility(
+    ...     IBugNotificationSet).getNotificationsToSend()
+    >>> email_notifications = get_email_notifications(pending_notifications)
+    >>> for bug_notifications, messages in email_notifications:
+    ...     for message in messages:
+    ...         print_notification(message)
+    To: [e-mail]
+    ...
+    You received this bug notification because you are subscribed to
+    mozilla-firefox in ubuntu.
+    http://bugs.launchpad.dev/bugs/1
+    ...
+    ----------------------------------------------------------------------
+    To: [e-mail]
+    ...
+    You received this bug notification because you are a member of ShipIt
+    Administrators, which is a direct subscriber.
+    ...
+    ----------------------------------------------------------------------
+    To: [e-mail]
+    ...
+    You received this bug notification because you are a bug assignee.
+    ...
+    ----------------------------------------------------------------------
+    To: [e-mail]
+    From: Sample Person <[e-mail]>
+    Subject: [Bug 1] Re: Firefox does not support SVG
+    X-Launchpad-Message-Rationale: Subscriber (Mozilla Firefox)
+    <BLANKLINE>
+    ** Summary changed to: whatever.
+    <BLANKLINE>
+    --
+    You received this bug notification because you are subscribed to Mozilla
+    Firefox.
+    ...
+    ----------------------------------------------------------------------
+    To: [e-mail]
+    ...
+    You received this bug notification because you are a member of Ubuntu
+    Team, which is the registrant for Ubuntu.
+    ...
+    ----------------------------------------------------------------------
+    To: [e-mail]
+    ...
+    You received this bug notification because you are a direct subscriber
+    of the bug.
+    ...
+    ----------------------------------------------------------------------

=== modified file 'lib/lp/bugs/doc/bugsubscription.txt'
--- lib/lp/bugs/doc/bugsubscription.txt	2011-01-27 09:47:06 +0000
+++ lib/lp/bugs/doc/bugsubscription.txt	2011-02-01 19:35:24 +0000
@@ -250,11 +250,14 @@
 have a bug notification level greater than or equal to the value passed
 in the `level` parameter are returned.
 
-The bug notification level of No Privileges Person's structural
-subscription is currently set to COMMENTS.
+Structural subscriptions control their bug notification levels via zero
+or more filters.  If there are no filters, the subscription is
+interpreted to mean that the subscriber wants all notifications.  In the
+case of bug notification levels, that is equivalent to
+BugNotificationLevel.COMMENTS.
 
-    >>> print subscription_no_priv.bug_notification_level.name
-    COMMENTS
+    >>> print subscription_no_priv.bug_filters.count()
+    0
 
 With this subscription level, No Privileges Person is returned for all
 parameter values of level.
@@ -288,11 +291,15 @@
     Sample Person
     Ubuntu Team
 
-If No Privileges Person sets his notification level to LIFECYCLE, he will
-not be included, if the parameter `level` is METADATA or COMMENTS.
+If No Privileges Person created a single filter with a notification
+level set to LIFECYCLE, he will not be included, if the parameter
+`level` is METADATA or COMMENTS.
 
-    >>> subscription_no_priv.bug_notification_level = (
-    ...     BugNotificationLevel.LIFECYCLE)
+    >>> from lp.testing import person_logged_in
+    >>> with person_logged_in(mr_no_privs):
+    ...     filter_no_priv = subscription_no_priv.newBugFilter()
+    ...     filter_no_priv.bug_notification_level = (
+    ...         BugNotificationLevel.LIFECYCLE)
 
     >>> print_displayname(linux_source_bug.getAlsoNotifiedSubscribers(
     ...     level=BugNotificationLevel.LIFECYCLE))

=== modified file 'lib/lp/bugs/doc/structural-subscriptions.txt'
--- lib/lp/bugs/doc/structural-subscriptions.txt	2011-01-21 11:25:22 +0000
+++ lib/lp/bugs/doc/structural-subscriptions.txt	2011-02-01 19:35:24 +0000
@@ -24,8 +24,6 @@
     ...     subscriber=sampleperson, subscribed_by=foobar)
     >>> ff_sub.target
     <Product at ...>
-    >>> ff_sub.bug_notification_level
-    <DBItem BugNotificationLevel.NOTHING, (10) Nothing>
 
     >>> ubuntu_sub = StructuralSubscription(distribution=ubuntu,
     ...     subscriber=sampleperson, subscribed_by=foobar)

=== modified file 'lib/lp/bugs/interfaces/bugsubscriptionfilter.py'
--- lib/lp/bugs/interfaces/bugsubscriptionfilter.py	2011-01-21 08:12:29 +0000
+++ lib/lp/bugs/interfaces/bugsubscriptionfilter.py	2011-02-01 19:35:24 +0000
@@ -25,6 +25,7 @@
     )
 
 from canonical.launchpad import _
+from lp.registry.enum import BugNotificationLevel
 from lp.bugs.interfaces.bugtask import (
     BugTaskImportance,
     BugTaskStatus,
@@ -59,6 +60,13 @@
     exclude_any_tags = Bool(
         title=_("Exclude all tags"),
         required=True, default=False)
+    bug_notification_level = exported(
+        Choice(
+            title=_("Bug notification level"), required=True,
+            vocabulary=BugNotificationLevel,
+            default=BugNotificationLevel.NOTHING,
+            description=_("The volume and type of bug notifications "
+                          "this subscription will generate.")))
 
     description = exported(
         Text(

=== modified file 'lib/lp/bugs/interfaces/structuralsubscription.py'
--- lib/lp/bugs/interfaces/structuralsubscription.py	2011-01-21 08:12:29 +0000
+++ lib/lp/bugs/interfaces/structuralsubscription.py	2011-02-01 19:35:24 +0000
@@ -8,7 +8,6 @@
 __metaclass__ = type
 
 __all__ = [
-    'BugNotificationLevel',
     'IStructuralSubscription',
     'IStructuralSubscriptionForm',
     'IStructuralSubscriptionTarget',
@@ -47,7 +46,6 @@
     )
 
 from canonical.launchpad import _
-from lp.registry.enum import BugNotificationLevel
 from lp.registry.interfaces.person import IPerson
 from lp.services.fields import (
     PersonChoice,
@@ -76,12 +74,6 @@
         title=_('Subscribed by'), required=True,
         vocabulary='ValidPersonOrTeam', readonly=True,
         description=_("The person creating the subscription.")))
-    bug_notification_level = Choice(
-        title=_("Bug notification level"), required=True,
-        vocabulary=BugNotificationLevel,
-        default=BugNotificationLevel.NOTHING,
-        description=_("The volume and type of bug notifications "
-                      "this subscription will generate."))
     date_created = exported(Datetime(
         title=_("The date on which this subscription was created."),
         required=False, readonly=True))
@@ -121,17 +113,11 @@
     Read-only parts.
     """
 
-    # We don't really want to expose the level details yet. Only
-    # BugNotificationLevel.COMMENTS is used at this time.
-    @call_with(
-        min_bug_notification_level=BugNotificationLevel.COMMENTS)
     @operation_returns_collection_of(IStructuralSubscription)
     @export_read_operation()
-    def getSubscriptions(min_bug_notification_level):
+    def getSubscriptions():
         """Return all the subscriptions with the specified levels.
 
-        :min_bug_notification_level: The lowest bug notification level
-          for which subscriptions should be returned.
         :return: A sequence of `IStructuralSubscription`.
         """
 
@@ -172,7 +158,7 @@
         """Add a subscription for this structure.
 
         This method is used to create a new `IStructuralSubscription`
-        for the target, with no levels set.
+        for the target, without filters.
 
         :subscriber: The IPerson who will be subscribed. If omitted,
             subscribed_by will be used.
@@ -189,20 +175,16 @@
             required=False))
     @call_with(subscribed_by=REQUEST_USER)
     @export_factory_operation(IStructuralSubscription, [])
-    def addBugSubscription(subscriber, subscribed_by,
-                           bug_notification_level=None):
+    def addBugSubscription(subscriber, subscribed_by):
         """Add a bug subscription for this structure.
 
         This method is used to create a new `IStructuralSubscription`
-        for the target with the bug notification level set to
-        COMMENTS, the only level currently in use.
+        for the target.  This initially is without filters, which will
+        mean that all notifications will be sent.
 
         :subscriber: The IPerson who will be subscribed. If omitted,
             subscribed_by will be used.
         :subscribed_by: The IPerson creating the subscription.
-        :bug_notification_level: The BugNotificationLevel for the
-            subscription. If omitted, BugNotificationLevel.COMMENTS will be
-            used.
         :return: The new bug subscription.
         """
 
@@ -218,9 +200,7 @@
     def removeBugSubscription(subscriber, unsubscribed_by):
         """Remove a subscription to bugs from this structure.
 
-        If subscription levels for other applications are set,
-        set the subscription's `bug_notification_level` to
-        `NOTHING`, otherwise, destroy the subscription.
+        This will delete all associated filters.
 
         :subscriber: The IPerson who will be unsubscribed. If omitted,
             unsubscribed_by will be used.

=== modified file 'lib/lp/bugs/model/bugsubscriptionfilter.py'
--- lib/lp/bugs/model/bugsubscriptionfilter.py	2011-01-20 19:39:08 +0000
+++ lib/lp/bugs/model/bugsubscriptionfilter.py	2011-02-01 19:35:24 +0000
@@ -17,9 +17,11 @@
     )
 from zope.interface import implements
 
+from canonical.database.enumcol import DBEnum
 from canonical.launchpad import searchbuilder
 from canonical.launchpad.interfaces.lpstorm import IStore
 from lp.bugs.interfaces.bugsubscriptionfilter import IBugSubscriptionFilter
+from lp.registry.enum import BugNotificationLevel
 from lp.bugs.model.bugsubscriptionfilterimportance import (
     BugSubscriptionFilterImportance,
     )
@@ -44,6 +46,10 @@
     structural_subscription = Reference(
         structural_subscription_id, "StructuralSubscription.id")
 
+    bug_notification_level = DBEnum(
+        enum=BugNotificationLevel,
+        default=BugNotificationLevel.COMMENTS,
+        allow_none=False)
     find_all_tags = Bool(allow_none=False, default=False)
     include_any_tags = Bool(allow_none=False, default=False)
     exclude_any_tags = Bool(allow_none=False, default=False)

=== modified file 'lib/lp/bugs/model/structuralsubscription.py'
--- lib/lp/bugs/model/structuralsubscription.py	2011-01-22 02:59:35 +0000
+++ lib/lp/bugs/model/structuralsubscription.py	2011-02-01 19:35:24 +0000
@@ -39,7 +39,6 @@
 from zope.interface import implements
 
 from canonical.database.constants import UTC_NOW
-from canonical.database.enumcol import DBEnum
 from canonical.database.sqlbase import quote
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.interfaces.lpstorm import IStore
@@ -51,7 +50,6 @@
     BugSubscriptionFilterStatus,
     )
 from lp.bugs.model.bugsubscriptionfiltertag import BugSubscriptionFilterTag
-from lp.registry.enum import BugNotificationLevel
 from lp.registry.errors import (
     DeleteSubscriptionError,
     UserCannotSubscribePerson,
@@ -115,10 +113,6 @@
                           validator=validate_public_person)
     subscribed_by = Reference(subscribed_byID, "Person.id")
 
-    bug_notification_level = DBEnum(
-        enum=BugNotificationLevel,
-        default=BugNotificationLevel.NOTHING,
-        allow_none=False)
     date_created = DateTime(
         "date_created", allow_none=False, default=UTC_NOW,
         tzinfo=pytz.UTC)
@@ -395,24 +389,18 @@
                 return False
         return True
 
-    def addBugSubscription(self, subscriber, subscribed_by,
-                           bug_notification_level=None):
+    def addBugSubscription(self, subscriber, subscribed_by):
         """See `IStructuralSubscriptionTarget`."""
         # This is a helper method for creating a structural
-        # subscription and immediately giving it a full
-        # bug notification level. It is useful so long as
-        # subscriptions are mainly used to implement bug contacts.
+        # subscription. It is useful so long as subscriptions are mainly
+        # used to implement bug contacts.
 
         if not self.userCanAlterBugSubscription(subscriber, subscribed_by):
             raise UserCannotSubscribePerson(
                 '%s does not have permission to subscribe %s' % (
                     subscribed_by.name, subscriber.name))
 
-        sub = self.addSubscription(subscriber, subscribed_by)
-        if bug_notification_level is None:
-            bug_notification_level = BugNotificationLevel.COMMENTS
-        sub.bug_notification_level = bug_notification_level
-        return sub
+        return self.addSubscription(subscriber, subscribed_by)
 
     def removeBugSubscription(self, subscriber, unsubscribed_by):
         """See `IStructuralSubscriptionTarget`."""
@@ -425,13 +413,14 @@
                     unsubscribed_by.name, subscriber.name))
 
         subscription_to_remove = self.getSubscriptions(
-            min_bug_notification_level=BugNotificationLevel.METADATA,
             subscriber=subscriber).one()
 
         if subscription_to_remove is None:
             raise DeleteSubscriptionError(
                 "%s is not subscribed to %s." % (
                 subscriber.name, self.displayname))
+        # XXX gary 2011-02-01 bug=711362
+        # We should delete all associated filters.
         store = Store.of(subscription_to_remove)
         store.remove(subscription_to_remove)
 
@@ -444,17 +433,10 @@
         all_subscriptions = self.getSubscriptions(subscriber=person)
         return all_subscriptions.one()
 
-    def getSubscriptions(self,
-                         min_bug_notification_level=
-                         BugNotificationLevel.NOTHING,
-                         subscriber=None):
+    def getSubscriptions(self, subscriber=None):
         """See `IStructuralSubscriptionTarget`."""
         from lp.registry.model.person import Person
-        clauses = [
-            StructuralSubscription.subscriberID==Person.id,
-            (StructuralSubscription.bug_notification_level >=
-             min_bug_notification_level),
-            ]
+        clauses = [StructuralSubscription.subscriberID==Person.id]
         for key, value in self._target_args.iteritems():
             clauses.append(
                 getattr(StructuralSubscription, key)==value)
@@ -470,13 +452,11 @@
     @property
     def bug_subscriptions(self):
         """See `IStructuralSubscriptionTarget`."""
-        return self.getSubscriptions(
-            min_bug_notification_level=BugNotificationLevel.METADATA)
+        return self.getSubscriptions()
 
     def userHasBugSubscriptions(self, user):
         """See `IStructuralSubscriptionTarget`."""
-        bug_subscriptions = self.getSubscriptions(
-            min_bug_notification_level=BugNotificationLevel.METADATA)
+        bug_subscriptions = self.getSubscriptions()
         if user is not None:
             for subscription in bug_subscriptions:
                 if (subscription.subscriber == user or
@@ -523,22 +503,23 @@
         # does not know how to compile an expression with a proxied list.
         self.tags = list(bugtask.bug.tags)
         # Set up common conditions.
-        self.base_conditions = And(
-            StructuralSubscription.bug_notification_level >= level,
-            join_condition)
+        self.base_conditions = join_condition
         # Set up common filter conditions.
+        self.filter_conditions = And(
+            BugSubscriptionFilter.bug_notification_level >= level,
+            self.base_conditions)
         if len(self.tags) == 0:
             self.filter_conditions = And(
                 # When the bug has no tags, filters with include_any_tags set
                 # can never match.
                 Not(BugSubscriptionFilter.include_any_tags),
-                self.base_conditions)
+                self.filter_conditions)
         else:
             self.filter_conditions = And(
                 # When the bug has tags, filters with exclude_any_tags set can
                 # never match.
                 Not(BugSubscriptionFilter.exclude_any_tags),
-                self.base_conditions)
+                self.filter_conditions)
 
     @property
     def subscriptions_without_filters(self):

=== modified file 'lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py'
--- lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py	2010-10-04 13:37:00 +0000
+++ lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py	2011-02-01 19:35:24 +0000
@@ -17,6 +17,7 @@
     BugTaskStatus,
     )
 from lp.bugs.model.bugsubscriptionfilter import BugSubscriptionFilter
+from lp.registry.enum import BugNotificationLevel
 from lp.testing import (
     anonymous_logged_in,
     login_person,
@@ -42,6 +43,8 @@
         # Create.
         bug_subscription_filter = BugSubscriptionFilter()
         bug_subscription_filter.structural_subscription = self.subscription
+        bug_subscription_filter.bug_notification_level = (
+            BugNotificationLevel.METADATA)
         bug_subscription_filter.find_all_tags = True
         bug_subscription_filter.include_any_tags = True
         bug_subscription_filter.exclude_any_tags = True
@@ -61,6 +64,9 @@
         self.assertIs(True, bug_subscription_filter.find_all_tags)
         self.assertIs(True, bug_subscription_filter.include_any_tags)
         self.assertIs(True, bug_subscription_filter.exclude_any_tags)
+        self.assertEqual(
+            BugNotificationLevel.METADATA,
+            bug_subscription_filter.bug_notification_level)
         self.assertEqual(u"foo", bug_subscription_filter.other_parameters)
         self.assertEqual(u"bar", bug_subscription_filter.description)
 
@@ -70,6 +76,9 @@
         bug_subscription_filter = BugSubscriptionFilter()
         bug_subscription_filter.structural_subscription = self.subscription
         # Check.
+        self.assertEqual(
+            BugNotificationLevel.COMMENTS,
+            bug_subscription_filter.bug_notification_level)
         self.assertIs(False, bug_subscription_filter.find_all_tags)
         self.assertIs(False, bug_subscription_filter.include_any_tags)
         self.assertIs(False, bug_subscription_filter.exclude_any_tags)

=== modified file 'lib/lp/bugs/model/tests/test_bugtask.py'
--- lib/lp/bugs/model/tests/test_bugtask.py	2011-01-20 04:50:35 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py	2011-02-01 19:35:24 +0000
@@ -1413,12 +1413,13 @@
         login_person(subscriber)
         product, bug = self.make_product_with_bug()
         subscription = product.addBugSubscription(subscriber, subscriber)
-        subscription.bug_notification_level = BugNotificationLevel.METADATA
+        filter = subscription.newBugFilter()
+        filter.bug_notification_level = BugNotificationLevel.METADATA
         self.assertEqual(
             [subscriber], list(
                 self.getStructuralSubscribers(
                     bug.bugtasks, level=BugNotificationLevel.METADATA)))
-        subscription.bug_notification_level = BugNotificationLevel.METADATA
+        filter.bug_notification_level = BugNotificationLevel.METADATA
         self.assertEqual(
             [], list(
                 self.getStructuralSubscribers(

=== modified file 'lib/lp/bugs/tests/structural-subscription-target.txt'
--- lib/lp/bugs/tests/structural-subscription-target.txt	2011-01-22 02:59:35 +0000
+++ lib/lp/bugs/tests/structural-subscription-target.txt	2011-02-01 19:35:24 +0000
@@ -13,23 +13,19 @@
     >>> target.addBugSubscription(ubuntu_team, foobar)
     <...StructuralSubscription ...>
 
-Trying to remove a bug subscription when notification levels for other
-applications are set, doesn't remove the subscription. Instead the
-notification level for bugs is set to NOTHING.
+We can add and remove these subscriptions.
 
     >>> def print_subscriptions_list(subscriptions):
     ...     for subscription in subscriptions:
-    ...         print '%s %s' % (
-    ...               subscription.subscriber.name,
-    ...               subscription.bug_notification_level.name)
+    ...         print subscription.subscriber.name
 
     >>> subscription = target.addBugSubscription(foobar, foobar)
     >>> print_subscriptions_list(target.getSubscriptions())
-    name16 COMMENTS
-    ubuntu-team COMMENTS
+    name16
+    ubuntu-team
     >>> target.removeBugSubscription(foobar, foobar)
     >>> print_subscriptions_list(target.getSubscriptions())
-    ubuntu-team COMMENTS
+    ubuntu-team
 
 To get a user's subscription to the target, use
 IStructuralSubscriptionTarget.getSubscription.
@@ -39,19 +35,14 @@
     >>> print target.getSubscription(no_priv)
     None
 
-To search for subscriptions with certain level settings on a structure
-we use getSubscriptions.
+To search for all subscriptions on a structure we use getSubscriptions.
 
-    >>> from lp.registry.enum import BugNotificationLevel
+    >>> print_subscriptions_list(target.bug_subscriptions)
+    ubuntu-team
     >>> subscription = target.addSubscription(no_priv, no_priv)
-    >>> subscription.bug_notification_level = BugNotificationLevel.METADATA
     >>> print_subscriptions_list(target.bug_subscriptions)
-    no-priv METADATA
-    ubuntu-team COMMENTS
-    >>> subscriptions = target.getSubscriptions(min_bug_notification_level=
-    ...                         BugNotificationLevel.COMMENTS)
-    >>> print_subscriptions_list(subscriptions)
-    ubuntu-team COMMENTS
+    no-priv
+    ubuntu-team
 
 
 Structural subscriptions and indirect bug subscriptions

=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
--- lib/lp/bugs/tests/test_bugchanges.py	2010-12-23 12:55:53 +0000
+++ lib/lp/bugs/tests/test_bugchanges.py	2011-02-01 19:35:24 +0000
@@ -28,6 +28,7 @@
 from lp.bugs.interfaces.cve import ICveSet
 from lp.registry.enum import BugNotificationLevel
 from lp.testing.factory import LaunchpadObjectFactory
+from lp.testing import person_logged_in
 
 
 class TestBugChanges(unittest.TestCase):
@@ -59,7 +60,9 @@
         # Create a new bug subscription with a new person.
         subscriber = self.factory.makePerson(name=name)
         subscription = target.addBugSubscription(subscriber, subscriber)
-        subscription.bug_notification_level = level
+        with person_logged_in(subscriber):
+            filter = subscription.newBugFilter()
+            filter.bug_notification_level = level
         return subscriber
 
     def saveOldChanges(self, bug=None, append=False):

=== modified file 'lib/lp/bugs/tests/test_structuralsubscriptiontarget.py'
--- lib/lp/bugs/tests/test_structuralsubscriptiontarget.py	2011-01-21 08:12:29 +0000
+++ lib/lp/bugs/tests/test_structuralsubscriptiontarget.py	2011-02-01 19:35:24 +0000
@@ -192,8 +192,6 @@
         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):
@@ -205,8 +203,7 @@
         # If no one has a filtered subscription for the given bug, the result
         # of getSubscriptionsForBugTask() is the same as for
         # getSubscriptions().
-        subscriptions = self.target.getSubscriptions(
-            min_bug_notification_level=BugNotificationLevel.NOTHING)
+        subscriptions = self.target.getSubscriptions()
         self.assertSubscriptions(list(subscriptions))
 
     def test_getSubscriptionsForBugTask_with_filter_on_status(self):
@@ -250,8 +247,8 @@
         # which getSubscriptionsForBugTask() observes.
 
         # Adjust the subscription level to METADATA.
-        self.subscription.bug_notification_level = (
-            BugNotificationLevel.METADATA)
+        filter = self.subscription.newBugFilter()
+        filter.bug_notification_level = BugNotificationLevel.METADATA
 
         # The subscription is found when looking for NOTHING or above.
         self.assertSubscriptions(

=== modified file 'lib/lp/registry/doc/private-team-roles.txt'
--- lib/lp/registry/doc/private-team-roles.txt	2010-08-23 16:51:11 +0000
+++ lib/lp/registry/doc/private-team-roles.txt	2011-02-01 19:35:24 +0000
@@ -157,8 +157,6 @@
     ...     subscriber=priv_team, subscribed_by=team_owner)
     >>> sub.target
     <Product at ...>
-    >>> sub.bug_notification_level
-    <DBItem BugNotificationLevel.NOTHING, (10) Nothing>
 
 
 Structural Subscription to Distributions
@@ -171,8 +169,6 @@
     ...     subscriber=priv_team, subscribed_by=team_owner)
     >>> sub.target
     <Distribution 'Ubuntu' (ubuntu)>
-    >>> sub.bug_notification_level
-    <DBItem BugNotificationLevel.NOTHING, (10) Nothing>
 
 
 Project Roles