Merge lp:~danilo/launchpad/proper-bug-muting into lp:launchpad/db-devel

[Данило Шеган]

[r=gmb,stub][bug=772763] Use a separate table for muting bug subscriptions to allow restoring one's subscription after unmuting.

= Bug 772763: step 1 =

At the moment, muting a bug is implemented using BugSubscription by setting bug_notification_level on it to NOTHING. That has a bad side-effect that once someone decides to unmute their subscription, we can't restore the previous one. UI is also confusing ("unmuting" just removes the "mute", without actually re-subscribing you).

== Proposed fix ==

To parallel BugSubscriptionFilterMute, which allows one to mute a structural subscription filter, we implement a BugMute table which allows a single person to mute all email for a bug.

Since we only "expand" subscribers at a very late stage when emails are constructed (in construct_email_notifications), we can either filter muted recipients in there or in getRecipientFilterData which is used by this method with expanded recipients passed in. I did the change in getRecipientFilterData because it seems more sane to have all of the direct DB access in a single place. It's also more easily unit-tested.

I wonder if I should do the migration of NOTHING subscriptions to BugMute records in the DB patch or not. That depends on how many of them there are, but it'd probably be best to do it that way.

== Implementation details ==

 * We provide a very basic BugMute table that is modelled after the BugSubscriptionFilterMute table. Bug muting/unmuting is modified to use it, and test is updated to match the new behaviour. This is all in model/bug.py, interfaces/bug.py and test_bug.py
 * XXXes are there to remind me to talk to the team if we really want to log bug muting/unmuting in the bug activity log (assuming bug.addChange does that), as it used to with the use of subscribe/unsubscribe. I assume we'll just end up removing the XXXes.
 * We modify getRecipientFilterData to accept a bug as the parameter as well, and fetch all muted subscribers so we can easily filter them out.
 * Test in test_bugchanges.py is the integration test (interestingly, one that even failed for the existing implementation, because muting didn't work for all cases).
 * UI is not modified in any way in this branch, which means that it will be a bit confusing; that's left for follow-up branches.
 * Another branch will go on with removing the NOTHING level if possible.
 * Some typos in registry/model/person.py and bugsubscriptionfilter.py are fixed along the way
 * I have another 144 diff of lint fixes for mostly things not caused by my changes, not including it so the branch is smaller; similar holds for sampledata updates, I'll commit them after review.

== Tests ==

bin/test -cvvt TestBugSubscriptionMethods -t getRecipientFilterData -t test_description_changed_no_muted_email

== Demo and Q/A ==

With 'malone.advanced-subscriptions.enabled default 1 on' set as the feature flag, try playing with muting/unmuting (and seeing how your bug subscription gets restored to appropriate "level") on any BugTask:+index page (i.e. a bug page).

Note that until UI is fully adapted as well, you might need to refresh the page between actions.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/model/bugnotification.py
  database/schema/patch-2208-97-0.sql
  lib/lp/bugs/configure.zcml
  database/schema/comments.sql
  lib/lp/bugs/tests/test_bugnotification.py
  database/schema/security.cfg
  lib/lp/bugs/model/bugsubscriptionfilter.py
  lib/lp/bugs/scripts/tests/test_bugnotification.py
  lib/lp/bugs/model/bug.py
  lib/lp/bugs/tests/test_bugchanges.py
  lib/lp/bugs/scripts/bugnotification.py
  lib/lp/bugs/tests/test_bug.py
  lib/lp/registry/model/person.py
  lib/lp/bugs/interfaces/bugnotification.py
  lib/lp/bugs/interfaces/bug.py

== Schema ==

database/sampledata/current.sql
    database/sampledata/lintdata.sql differs from database/sampledata/current.sql.
    Patches to the schema, or manual edits to database/sampledata/current.sql
    do not match the dump of the launchpad_ftest_template database.
    If database/sampledata/lintdata.sql is correct, copy it to
    database/sampledata/current.sql.
    Otherwise update database/sampledata/current.sql and run:
        make schema
        make newsampledata
        cd database/sampledata
        cp newsampledata.sql database/sampledata/current.sql
    Run make schema again to update the test/dev database.

database/sampledata/current.sql
    database/sampledata/lintdata-dev.sql differs from database/sampledata/current-dev.sql.
    Patches to the schema, or manual edits to database/sampledata/current-dev.sql
    do not match the dump of the launchpad_dev_template database.
    If database/sampledata/lintdata-dev.sql is correct, copy it to
    database/sampledata/current-dev.sql.
    Otherwise update database/sampledata/current-dev.sql and run:
        make schema
        make newsampledata
        cd database/sampledata
        cp newsampledata-dev.sql database/sampledata/current-dev.sql
    Run make schema again to update the test/dev database.

./lib/lp/bugs/model/bugnotification.py
     147: local variable 'cached_people' is assigned to but never used
     152: local variable 'cached_bugs' is assigned to but never used
     273: E501 line too long (81 characters)
     192: Line exceeds 78 characters.
     273: Line exceeds 78 characters.
./lib/lp/bugs/tests/test_bugnotification.py
     387: E201 whitespace after '{'
./lib/lp/bugs/scripts/tests/test_bugnotification.py
     175: Line exceeds 78 characters.
./lib/lp/bugs/tests/test_bugchanges.py
     211: local variable 'bug_subscription' is assigned to but never used
     605: local variable 'old_tags' is assigned to but never used
     629: local variable 'old_tags' is assigned to but never used
    1020: local variable 'lifecycle_subscriber' is assigned to but never used
    1742: local variable 'old_description' is assigned to but never used
    1759: local variable 'old_description' is assigned to but never used
    1774: local variable 'old_description' is assigned to but never used
    1788: local variable 'old_description' is assigned to but never used
./lib/lp/registry/model/person.py
    3849: local variable 'karma_total' is assigned to but never used
    3573: W291 trailing whitespace
    3575: W291 trailing whitespace
    3577: W291 trailing whitespace
    3579: W291 trailing whitespace
    3581: W291 trailing whitespace
    3598: W291 trailing whitespace
    3573: Line has trailing whitespace.
    3575: Line has trailing whitespace.
    3577: Line has trailing whitespace.
    3579: Line has trailing whitespace.
    3581: Line has trailing whitespace.
    3598: Line has trailing whitespace.

[Stuart Bishop (stub)]

So we have a UI problem, and the desire to restore subscriptions to the previous value when emails for a bug are reenabled.

Is this feature genuinely interesting enough to add the new table, increasing the complexity of how to determine who gets what email?

The DB patch itself seems generally fine

 - We will want a who-did-it column to track who muted a team subscription if that is possible.
 - You are deleting a load of BugSubscription records. This means the code will need to cope with unmuting a bug with no existing BugSubscription, as well as restoring an existing BugSubscription. Would it not be better to set the affected BugSubscription notification level to something meaningful and avoid the extra code path and tests?

[Данило Шеган (danilo)]

У чет, 12. 05 2011. у 11:56 +0000, Stuart Bishop пише:
> So we have a UI problem, and the desire to restore subscriptions to the
> previous value when emails for a bug are reenabled.

It's not that simple. "Muting" is orthogonal to any subscriptions you
might have and should affect a single bug. In the bug report, Gary did
propose an alternative which is storing an actual muted subscription and
then restoring it when "unmuting", but the yellow team didn't like that
approach much (and it would still mean another new table to store muted
bug subscriptions; it was also only considered as the simpler of the
approaches for implementation, not as the preferred one).

> Is this feature genuinely interesting enough to add the new table,
> increasing the complexity of how to determine who gets what email?

Yes: it has been discussed and priority raised—we've basically inherited
the previous implementation from the Bugs team work in 2010, and didn't
want to change it unless explicitely asked to. Since we are now
explicitely asked to, that's what we are doing.

Also, it was raised as a big problem with the "muting" concept from user
perspective.

> The DB patch itself seems generally fine
>
> - We will want a who-did-it column to track who muted a team subscription if that is possible.

That should not be possible. Perhaps I should assert that in the code.

> - You are deleting a load of BugSubscription records.

Are we? I was expecting only a few of them to have level set to NOTHING
(10) on production. Am I mistaken?

> This means the
> code will need to cope with unmuting a bug with no existing
> BugSubscription, as well as restoring an existing BugSubscription.

That's transparently handled in the backend code (simply removing the
BugMute record restores the existing BugSubscription). And yes, that's
what UI code will have to do anyway if we want this feature, so we are
aware of that.

> Would it not be better to set the affected BugSubscription notification
> level to something meaningful and avoid the extra code path and tests?

That's what the code does today (i.e. uses the "NOTHING" level). Since
"muting" should work regardless of the way you are subscribed to a bug,
that's actually more complex and when it works, it does so "by
accident". For instance, it doesn't work when you are a member of the
team subscribed to the bug.

It also represents exactly the same model as BugSubscriptionFilterMute,
which should make it easier to understand the code.

Also, this makes the logic explicit for muting, and actually *simpler*.
Thus, it's much easier to ensure and prove it is correct. When
something is in practice orthogonal to the problem, it's best to
implement it like that as well.

[Stuart Bishop (stub)]

That all seems fine.

Please add the assert to block teams ending up with mute records. Even if the UI doesn't allow it, exposure by the web service might let bad data in. The current model doesn't support team mutes and that is fine (and probably desirable).

patch-2208-70-0.sql

review: Approve (db)

[Robert Collins (lifeless)]

Why would a team mute be undesirable?

Consider Team inner a member of team outer. And me, an admin of team inner.

If team outer is subscribed to (say) all Launchpad bugs, team inner
may as a whole be uninterested in the larger subscription and want to
mute it.

[Stuart Bishop (stub)]

On Mon, May 16, 2011 at 6:21 AM, Robert Collins
<email address hidden> wrote:

> Why would a team mute be undesirable?

I think it becomes very confusing very quickly. eg. if you are a
member of two teams subscribed to a bug, what happens if one team
subscription is muted? Or if you are both directly subscribed and a
member of a team that is subscribed? If the team is muted, do you
receive an email stating this so team members who do want to receive
email can directly subscribe?

If we do want this in the future, the edge cases and UI need to be
thought through. If this is the future, we might as well add the
muted_by NOT NULL person reference and corresponding index now.

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/

[Данило Шеган (danilo)]

У нед, 15. 05 2011. у 23:21 +0000, Robert Collins пише:
> Why would a team mute be undesirable?

We've only ever planned for personal muting to be possible: fwiw, we
have no UI making team muting possible at all, nor was it ever planned
to have it. I am not opposed, but it would be an entirely new feature,
and I suggest you discuss it with Jono and Gary if you feel like it's
very much desirable.

[Graham Binns (gmb)]

review: Approve (code)

[Robert Collins (lifeless)]

> У нед, 15. 05 2011. у 23:21 +0000, Robert Collins пише:
> > Why would a team mute be undesirable?
>
> We've only ever planned for personal muting to be possible: fwiw, we
> have no UI making team muting possible at all, nor was it ever planned
> to have it. I am not opposed, but it would be an entirely new feature,
> and I suggest you discuss it with Jono and Gary if you feel like it's
> very much desirable.

Uhm, so last week we muted the bug subscription for launchpad-bugs, successfully (well, until the backend blew up :P). I will raise with jono/gary for clarification. No objection to landing this - I was mainly responding to the desirability point raised earlier.

=== modified file 'database/schema/comments.sql'
--- database/schema/comments.sql	2011-05-11 10:32:15 +0000
+++ database/schema/comments.sql	2011-05-17 11:44:40 +0000
@@ -190,6 +190,12 @@
 COMMENT ON COLUMN BugJob.job_type IS 'The type of job (enumeration value). Allows us to query the database for a given subset of BugJobs.';
 COMMENT ON COLUMN BugJob.json_data IS 'A JSON struct containing data for the job.';
 
+-- BugMute
+COMMENT ON TABLE BugMute IS 'Mutes for bug notifications.';
+COMMENT ON COLUMN BugMute.person IS 'The person that muted all notifications from this bug.';
+COMMENT ON COLUMN BugMute.bug IS 'The bug of this record';
+COMMENT ON COLUMN BugMute.date_created IS 'The date at which this mute was created.';
+
 -- BugNomination
 COMMENT ON TABLE BugNomination IS 'A bug nominated for fixing in a distroseries or productseries';
 COMMENT ON COLUMN BugNomination.bug IS 'The bug being nominated.';

=== added file 'database/schema/patch-2208-70-0.sql'
--- database/schema/patch-2208-70-0.sql	1970-01-01 00:00:00 +0000
+++ database/schema/patch-2208-70-0.sql	2011-05-17 11:44:40 +0000
@@ -0,0 +1,36 @@
+-- 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;
+
+-- A table to store bug mutes in.
+
+CREATE TABLE BugMute (
+    person integer REFERENCES Person(id)
+        ON DELETE CASCADE NOT NULL,
+    bug integer REFERENCES Bug(id)
+        ON DELETE CASCADE NOT NULL,
+    date_created timestamp without time zone
+        DEFAULT timezone('UTC'::text, now()) NOT NULL,
+    CONSTRAINT bugmute_pkey PRIMARY KEY (person, bug)
+);
+
+-- We don't need an index on person, as the primary key index can be used
+-- for those lookups. We have an index on just the bug, as the bulk of our
+-- lookups will be on bugs.
+CREATE INDEX bugmute__bug__idx
+    ON BugMute(bug);
+
+-- Migrate existing BugSubscription's with
+-- bug_notification_level == NOTHING
+-- to BugMute table.
+INSERT INTO BugMute (person, bug, date_created)
+    SELECT person, bug, date_created
+        FROM BugSubscription
+        WHERE bug_notification_level=10;
+-- Remove 'muting' BugSubscriptions.
+DELETE
+    FROM BugSubscription
+    WHERE bug_notification_level=10;
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 70, 0);

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2011-05-11 16:07:09 +0000
+++ database/schema/security.cfg	2011-05-17 11:44:40 +0000
@@ -1062,6 +1062,7 @@
 public.bugattachment                    = SELECT, INSERT, UPDATE
 public.bugjob                           = SELECT, INSERT
 public.bugmessage                       = SELECT, INSERT, UPDATE
+public.bugmute                          = SELECT, INSERT, UPDATE, DELETE
 public.bugnomination                    = SELECT, INSERT, UPDATE
 public.bugpackageinfestation            = SELECT, INSERT, UPDATE
 public.bugproductinfestation            = SELECT, INSERT, UPDATE
@@ -1445,6 +1446,7 @@
 public.bugattachment                    = SELECT
 public.bugjob                           = SELECT, INSERT
 public.bugmessage                       = SELECT, INSERT
+public.bugmute                          = SELECT
 public.bugnomination                    = SELECT
 public.bugnotification                  = SELECT, INSERT, UPDATE
 public.bugnotificationfilter            = SELECT, INSERT

=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py	2011-05-12 14:59:21 +0000
+++ lib/lp/bugs/browser/bugsubscription.py	2011-05-17 11:44:40 +0000
@@ -216,6 +216,14 @@
                 persons_for_user[person.id] = person
                 person_count += 1
 
+        # The view code previously expected a 'mute' to be a subscription
+        # as well.  Since it is not anymore, we add the user to the
+        # subscribers list as needed.
+        if self.user_is_muted:
+            if self.user.id not in persons_for_user:
+                persons_for_user[self.user.id] = self.user
+                person_count += 1
+
         self._subscriber_count_for_current_user = person_count
         return persons_for_user.values()
 
@@ -254,7 +262,7 @@
             if person.id == self.user.id:
                 if (self._use_advanced_features and
                     (self.user_is_subscribed_directly or
-                    self.user_is_muted)):
+                     self.user_is_muted)):
                         subscription_terms.append(
                             self._update_subscription_term)
                 subscription_terms.insert(
@@ -388,9 +396,14 @@
 
         if (subscription_person == self._update_subscription_term.value and
             (self.user_is_subscribed or self.user_is_muted)):
-            self._handleUpdateSubscription(level=bug_notification_level)
+            if self.user_is_muted:
+                self._handleUnmute()
+            if self.user_is_subscribed:
+                self._handleUpdateSubscription(level=bug_notification_level)
+            else:
+                self._handleSubscribe(level=bug_notification_level)
         elif self.user_is_muted and subscription_person == self.user:
-            self._handleUnsubscribeCurrentUser()
+            self._handleUnmute()
         elif (not self.user_is_subscribed and
             (subscription_person == self.user)):
             self._handleSubscribe(bug_notification_level)
@@ -411,6 +424,10 @@
         else:
             self._handleUnsubscribeOtherUser(user)
 
+    def _handleUnmute(self):
+        """Handle an unmute request."""
+        self.context.bug.unmute(self.user, self.user)
+
     def _handleUnsubscribeCurrentUser(self):
         """Handle the special cases for unsubscribing the current user.
 

=== modified file 'lib/lp/bugs/browser/tests/test_bug_context_menu.py'
--- lib/lp/bugs/browser/tests/test_bug_context_menu.py	2011-04-13 18:03:03 +0000
+++ lib/lp/bugs/browser/tests/test_bug_context_menu.py	2011-05-17 11:44:40 +0000
@@ -47,8 +47,7 @@
         person = self.factory.makePerson()
         with feature_flags():
             with person_logged_in(person):
-                self.bug.subscribe(
-                    person, person, level=BugNotificationLevel.NOTHING)
+                self.bug.mute(person, person)
                 link = self.context_menu.subscription()
                 self.assertEqual('Subscribe', link.text)
 
@@ -65,8 +64,7 @@
                 self.assertEqual("Mute bug mail", link.text)
                 # Once the user has muted the bug, the link text will
                 # change.
-                self.bug.subscribe(
-                    person, person, level=BugNotificationLevel.NOTHING)
+                self.bug.mute(person, person)
                 link = self.context_menu.mute_subscription()
                 self.assertEqual("Unmute bug mail", link.text)
 

=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscription_views.py'
--- lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2011-04-26 11:58:26 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscription_views.py	2011-05-17 11:44:40 +0000
@@ -5,6 +5,8 @@
 
 __metaclass__ = type
 
+import transaction
+
 from canonical.launchpad.ftests import LaunchpadFormHarness
 from canonical.launchpad.webapp import canonical_url
 from canonical.testing.layers import LaunchpadFunctionalLayer
@@ -287,6 +289,7 @@
 
         with FeatureFixture({self.feature_flag: ON}):
             with person_logged_in(self.person):
+                self.bug.mute(self.person, self.person)
                 subscribe_view = create_initialized_view(
                     self.bug.default_bugtask, name='+subscribe')
                 subscription_widget = (
@@ -325,12 +328,10 @@
 
         with FeatureFixture({self.feature_flag: ON}):
             with person_logged_in(self.person):
-                level = BugNotificationLevel.METADATA
                 form_data = {
                     'field.subscription': self.person.name,
                     # Although this isn't used we must pass it for the
                     # sake of form validation.
-                    'field.bug_notification_level': level.title,
                     'field.actions.continue': 'Continue',
                     }
                 create_initialized_view(
@@ -344,7 +345,7 @@
         # muted subscription will update the existing subscription to a
         # new BugNotificationLevel.
         with person_logged_in(self.person):
-            muted_subscription = self.bug.mute(self.person, self.person)
+            self.bug.mute(self.person, self.person)
 
         with FeatureFixture({self.feature_flag: ON}):
             with person_logged_in(self.person):
@@ -357,10 +358,9 @@
                 create_initialized_view(
                     self.bug.default_bugtask, form=form_data,
                     name='+subscribe')
+                transaction.commit()
                 self.assertFalse(self.bug.isMuted(self.person))
                 self.assertTrue(self.bug.isSubscribed(self.person))
-                self.assertEqual(
-                    level, muted_subscription.bug_notification_level)
 
     def test_bug_notification_level_field_has_widget_class(self):
         # The bug_notification_level widget has a widget_class property

=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2011-04-13 18:48:42 +0000
+++ lib/lp/bugs/configure.zcml	2011-05-17 11:44:40 +0000
@@ -645,6 +645,13 @@
           set_schema=".interfaces.bugsubscriptionfilter.IBugSubscriptionFilterAttributes" />
     </class>
 
+    <!-- BugMute -->
+    <class
+        class=".model.bug.BugMute">
+      <allow
+          interface=".interfaces.bug.IBugMute"/>
+    </class>
+
     <!-- BugSubscriptionFilterMute -->
     <class
         class=".model.bugsubscriptionfilter.BugSubscriptionFilterMute">

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2011-05-12 21:33:10 +0000
+++ lib/lp/bugs/interfaces/bug.py	2011-05-17 11:44:40 +0000
@@ -14,6 +14,7 @@
     'IBugAddForm',
     'IBugBecameQuestionEvent',
     'IBugDelta',
+    'IBugMute',
     'IBugSet',
     'IFileBugData',
     'IFrontPageBugAddForm',
@@ -85,6 +86,7 @@
     ContentNameField,
     Description,
     DuplicateBug,
+    PersonChoice,
     PublicPersonChoice,
     Tag,
     Title,
@@ -1210,3 +1212,18 @@
     comments = Attribute("Comments to add to the bug.")
     attachments = Attribute("Attachments to add to the bug.")
     hwdb_submission_keys = Attribute("HWDB submission keys for the bug.")
+
+
+class IBugMute(Interface):
+    """A mute on an IBug."""
+
+    person = PersonChoice(
+        title=_('Person'), required=True, vocabulary='ValidPersonOrTeam',
+        readonly=True, description=_("The person subscribed."))
+    bug = Reference(
+        IBug, title=_("Bug"),
+        required=True, readonly=True,
+        description=_("The bug to be muted."))
+    date_created = Datetime(
+        title=_("The date on which the mute was created."), required=False,
+        readonly=True)

=== modified file 'lib/lp/bugs/interfaces/bugnotification.py'
--- lib/lp/bugs/interfaces/bugnotification.py	2011-04-05 22:34:35 +0000
+++ lib/lp/bugs/interfaces/bugnotification.py	2011-05-17 11:44:40 +0000
@@ -80,15 +80,17 @@
         `BugNotificationRecipient` objects.
         """
 
-    def getRecipientFilterData(recipient_to_sources, notifications):
+    def getRecipientFilterData(bug, recipient_to_sources, notifications):
         """Get non-muted recipients mapped to sources & filter descriptions.
 
+        :param bug:
+            A bug we are collecting filter data for.
         :param recipient_to_sources:
             A dict of people who are to receive the email to the sources
             (BugNotificationRecipients) that represent the subscriptions that
             caused the notifications to be sent.
         :param notifications: the notifications that are being communicated.
-    
+
         The dict of recipients may have fewer recipients than were
         provided if those users muted all of the subscription filters
         that caused them to be sent.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-05-16 18:06:29 +0000
+++ lib/lp/bugs/model/bug.py	2011-05-17 11:44:40 +0000
@@ -11,6 +11,7 @@
     'Bug',
     'BugAffectsPerson',
     'BugBecameQuestionEvent',
+    'BugMute',
     'BugSet',
     'BugTag',
     'FileBugData',
@@ -29,6 +30,7 @@
 from functools import wraps
 from itertools import chain
 import operator
+import pytz
 import re
 
 from lazr.lifecycle.event import (
@@ -63,6 +65,11 @@
     Union,
     )
 from storm.info import ClassAlias
+from storm.locals import (
+    DateTime,
+    Int,
+    Reference,
+    )
 from storm.store import (
     EmptyResultSet,
     Store,
@@ -138,6 +145,7 @@
 from lp.bugs.interfaces.bug import (
     IBug,
     IBugBecameQuestionEvent,
+    IBugMute,
     IBugSet,
     IFileBugData,
     )
@@ -188,6 +196,7 @@
 from lp.registry.interfaces.distroseries import IDistroSeries
 from lp.registry.interfaces.person import (
     IPersonSet,
+    validate_person,
     validate_public_person,
     )
 from lp.registry.interfaces.product import IProduct
@@ -201,6 +210,7 @@
     )
 from lp.registry.model.pillar import pillar_sort_key
 from lp.registry.model.teammembership import TeamParticipation
+from lp.services.database.stormbase import StormBase
 from lp.services.fields import DuplicateBug
 from lp.services.propertycache import (
     cachedproperty,
@@ -843,40 +853,44 @@
         """See `IBug`."""
         return self.personIsSubscribedToDuplicate(person)
 
+    def _getMutes(self, person):
+        store = Store.of(self)
+        mutes = store.find(
+            BugMute,
+            BugMute.bug == self,
+            BugMute.person == person)
+        return mutes
+
     def isMuted(self, person):
         """See `IBug`."""
-        store = Store.of(self)
-        subscriptions = store.find(
-            BugSubscription,
-            BugSubscription.bug == self,
-            BugSubscription.person == person,
-            BugSubscription.bug_notification_level ==
-                BugNotificationLevel.NOTHING)
-        return not subscriptions.is_empty()
+        mutes = self._getMutes(person)
+        return not mutes.is_empty()
 
     def mute(self, person, muted_by):
         """See `IBug`."""
         if person is None:
             # This may be a webservice request.
             person = muted_by
-        # If there's an existing subscription, update it.
-        store = Store.of(self)
-        subscriptions = store.find(
-            BugSubscription,
-            BugSubscription.bug == self,
-            BugSubscription.person == person)
-        if subscriptions.is_empty():
-            return self.subscribe(
-                person, muted_by, level=BugNotificationLevel.NOTHING)
+        assert not person.is_team, (
+            "Muting a subscription for entire team is not allowed.")
+
+        # If it's already muted, ignore the request.
+        mutes = self._getMutes(person)
+        if mutes.is_empty():
+            mute = BugMute(person, self)
+            Store.of(mute).flush()
         else:
-            subscription = subscriptions.one()
-            subscription.bug_notification_level = (
-                BugNotificationLevel.NOTHING)
-            return subscription
+            # It's already muted, pass.
+            pass
 
     def unmute(self, person, unmuted_by):
         """See `IBug`."""
-        self.unsubscribe(person, unmuted_by)
+        store = Store.of(self)
+        if person is None:
+            # This may be a webservice request.
+            person = unmuted_by
+        mutes = self._getMutes(person)
+        store.remove(mutes.one())
 
     @property
     def subscriptions(self):
@@ -2701,3 +2715,29 @@
     def asDict(self):
         """Return the FileBugData instance as a dict."""
         return self.__dict__.copy()
+
+
+class BugMute(StormBase):
+    """Contains bugs a person has decided to block notifications from."""
+
+    implements(IBugMute)
+
+    __storm_table__ = "BugMute"
+
+    def __init__(self, person=None, bug=None):
+        if person is not None:
+            self.person = person
+        if bug is not None:
+            self.bug_id = bug.id
+
+    person_id = Int("person", allow_none=False, validator=validate_person)
+    person = Reference(person_id, "Person.id")
+
+    bug_id = Int("bug", allow_none=False)
+    bug = Reference(bug_id, "Bug.id")
+
+    __storm_primary__ = 'person_id', 'bug_id'
+
+    date_created = DateTime(
+        "date_created", allow_none=False, default=UTC_NOW,
+        tzinfo=pytz.UTC)

=== modified file 'lib/lp/bugs/model/bugnotification.py'
--- lib/lp/bugs/model/bugnotification.py	2011-05-12 21:33:10 +0000
+++ lib/lp/bugs/model/bugnotification.py	2011-05-17 11:44:40 +0000
@@ -144,12 +144,12 @@
         # Now we do some calls that are purely for cacheing.
         # Converting these into lists forces the queries to execute.
         if pending_notifications:
-            cached_people = list(
+            list(
                 getUtility(IPersonSet).getPrecachedPersonsFromIDs(
                     list(people_ids),
                     need_validity=True,
                     need_preferred_email=True))
-            cached_bugs = list(
+            list(
                 IStore(Bug).find(Bug, In(Bug.id, list(bug_ids))))
         pending_notifications.reverse()
         return pending_notifications
@@ -189,11 +189,18 @@
 
         return bug_notification
 
-    def getRecipientFilterData(self, recipient_to_sources, notifications):
+    def getRecipientFilterData(self, bug, recipient_to_sources,
+                               notifications):
         """See `IBugNotificationSet`."""
         if not notifications or not recipient_to_sources:
             # This is a shortcut that will remove some error conditions.
             return {}
+        # Collect bug mute information.
+        from lp.bugs.model.bug import BugMute
+        store = IStore(BugMute)
+        muted_person_ids = set(list(
+            store.find(BugMute.person_id,
+                       BugMute.bug == bug)))
         # This makes two calls to the database to get all the
         # information we need. The first call gets the filter ids and
         # descriptions for each recipient, and then we divide up the
@@ -202,6 +209,8 @@
         source_person_id_map = {}
         recipient_id_map = {}
         for recipient, sources in recipient_to_sources.items():
+            if recipient.id in muted_person_ids:
+                continue
             source_person_ids = set()
             recipient_id_map[recipient.id] = {
                 'principal': recipient,
@@ -231,14 +240,17 @@
             Join(StructuralSubscription,
                  BugSubscriptionFilter.structural_subscription_id ==
                     StructuralSubscription.id))
-        filter_data = source.find(
-            (StructuralSubscription.subscriberID,
-             BugSubscriptionFilter.id,
-             BugSubscriptionFilter.description),
-            In(BugNotificationFilter.bug_notification_id,
-               [notification.id for notification in notifications]),
-            In(StructuralSubscription.subscriberID,
-               source_person_id_map.keys()))
+        if len(source_person_id_map) == 0:
+            filter_data = []
+        else:
+            filter_data = source.find(
+                (StructuralSubscription.subscriberID,
+                 BugSubscriptionFilter.id,
+                 BugSubscriptionFilter.description),
+                In(BugNotificationFilter.bug_notification_id,
+                   [notification.id for notification in notifications]),
+                In(StructuralSubscription.subscriberID,
+                   source_person_id_map.keys()))
         filter_ids = []
         # Record the filters for each source.
         for source_person_id, filter_id, filter_description in filter_data:
@@ -259,7 +271,8 @@
             mute_data = store.find(
                 (BugSubscriptionFilterMute.person_id,
                  BugSubscriptionFilterMute.filter_id),
-                In(BugSubscriptionFilterMute.person_id, recipient_id_map.keys()),
+                In(BugSubscriptionFilterMute.person_id,
+                   recipient_id_map.keys()),
                 In(BugSubscriptionFilterMute.filter_id, filter_ids))
             for person_id, filter_id in mute_data:
                 del recipient_id_map[person_id]['filters'][filter_id]

=== modified file 'lib/lp/bugs/model/bugsubscriptionfilter.py'
--- lib/lp/bugs/model/bugsubscriptionfilter.py	2011-04-05 22:34:35 +0000
+++ lib/lp/bugs/model/bugsubscriptionfilter.py	2011-05-17 11:44:40 +0000
@@ -296,7 +296,7 @@
 
 
 class BugSubscriptionFilterMute(StormBase):
-    """A filter to specialize a *structural* subscription."""
+    """Bug subscription filters a person has decided to block emails from."""
 
     implements(IBugSubscriptionFilterMute)
 
@@ -312,7 +312,7 @@
     person = Reference(person_id, "Person.id")
 
     filter_id = Int("filter", allow_none=False)
-    filter = Reference(filter_id, "StructuralSubscription.id")
+    filter = Reference(filter_id, "BugSubscriptionFilter.id")
 
     __storm_primary__ = 'person_id', 'filter_id'
 

=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
--- lib/lp/bugs/scripts/bugnotification.py	2011-04-05 22:34:35 +0000
+++ lib/lp/bugs/scripts/bugnotification.py	2011-05-17 11:44:40 +0000
@@ -174,7 +174,7 @@
     from_address = get_bugmail_from_address(actor, bug)
     bug_notification_builder = BugNotificationBuilder(bug, actor)
     recipients = getUtility(IBugNotificationSet).getRecipientFilterData(
-        recipients, filtered_notifications)
+        bug, recipients, filtered_notifications)
     sorted_recipients = sorted(
         recipients.items(), key=lambda t: t[0].preferredemail.email)
 

=== modified file 'lib/lp/bugs/scripts/tests/test_bugnotification.py'
--- lib/lp/bugs/scripts/tests/test_bugnotification.py	2011-05-12 21:33:10 +0000
+++ lib/lp/bugs/scripts/tests/test_bugnotification.py	2011-05-17 11:44:40 +0000
@@ -172,7 +172,8 @@
 
     implements(IBugNotificationSet)
 
-    def getRecipientFilterData(self, recipient_to_sources, notifications):
+    def getRecipientFilterData(self, bug, recipient_to_sources,
+                               notifications):
         return dict(
             (recipient, {'sources': sources, 'filter descriptions': []})
             for recipient, sources in recipient_to_sources.items())

=== modified file 'lib/lp/bugs/tests/test_bug.py'
--- lib/lp/bugs/tests/test_bug.py	2011-05-10 14:19:24 +0000
+++ lib/lp/bugs/tests/test_bug.py	2011-05-17 11:44:40 +0000
@@ -8,6 +8,7 @@
 from lazr.lifecycle.snapshot import Snapshot
 from zope.component import getUtility
 from zope.interface import providedBy
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.testing.layers import DatabaseFunctionalLayer
 
@@ -39,11 +40,9 @@
         self.person = self.factory.makePerson()
 
     def test_is_muted_returns_true_for_muted_users(self):
-        # Bug.isMuted() will return True if the passed to it has a
-        # BugSubscription with a BugNotificationLevel of NOTHING.
+        # Bug.isMuted() will return True if the person passed to it is muted.
         with person_logged_in(self.person):
-            self.bug.subscribe(
-                self.person, self.person, level=BugNotificationLevel.NOTHING)
+            self.bug.mute(self.person, self.person)
             self.assertEqual(True, self.bug.isMuted(self.person))
 
     def test_is_muted_returns_false_for_direct_subscribers(self):
@@ -60,15 +59,21 @@
         with person_logged_in(self.person):
             self.assertEqual(False, self.bug.isMuted(self.person))
 
+    def test_mute_team_fails(self):
+        # Muting a subscription for an entire team doesn't work.
+        with person_logged_in(self.person):
+            team = self.factory.makeTeam(owner=self.person)
+            self.assertRaises(AssertionError,
+                              self.bug.mute, team, team)
+
     def test_mute_mutes_user(self):
-        # Bug.mute() adds a muted subscription for the user passed to
-        # it.
+        # Bug.mute() adds a BugMute record for the person passed to it.
         with person_logged_in(self.person):
-            muted_subscription = self.bug.mute(
-                self.person, self.person)
-            self.assertEqual(
-                BugNotificationLevel.NOTHING,
-                muted_subscription.bug_notification_level)
+            self.bug.mute(self.person, self.person)
+            naked_bug = removeSecurityProxy(self.bug)
+            bug_mute = naked_bug._getMutes(self.person).one()
+            self.assertEqual(self.bug, bug_mute.bug)
+            self.assertEqual(self.person, bug_mute.person)
 
     def test_mute_mutes_muter(self):
         # When exposed in the web API, the mute method regards the
@@ -80,14 +85,15 @@
             self.assertTrue(self.bug.isMuted(self.person))
 
     def test_mute_mutes_user_with_existing_subscription(self):
-        # Bug.mute() will update an existing subscription so that it
-        # becomes muted.
+        # Bug.mute() will not touch the existing subscription.
         with person_logged_in(self.person):
-            subscription = self.bug.subscribe(self.person, self.person)
-            muted_subscription = self.bug.mute(self.person, self.person)
-            self.assertEqual(subscription, muted_subscription)
+            subscription = self.bug.subscribe(
+                self.person, self.person,
+                level=BugNotificationLevel.METADATA)
+            self.bug.mute(self.person, self.person)
+            self.assertTrue(self.bug.isMuted(self.person))
             self.assertEqual(
-                BugNotificationLevel.NOTHING,
+                BugNotificationLevel.METADATA,
                 subscription.bug_notification_level)
 
     def test_unmute_unmutes_user(self):

=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
--- lib/lp/bugs/tests/test_bugchanges.py	2011-03-23 18:29:09 +0000
+++ lib/lp/bugs/tests/test_bugchanges.py	2011-05-17 11:44:40 +0000
@@ -208,7 +208,7 @@
         # Unsubscribing someone from a bug adds an item to the activity
         # log, but doesn't send an e-mail notification.
         subscriber = self.factory.makePerson(displayname='Mom')
-        bug_subscription = self.bug.subscribe(self.user, subscriber)
+        self.bug.subscribe(self.user, subscriber)
         self.saveOldChanges()
         # Only the user can unsubscribe him or her self.
         self.bug.unsubscribe(self.user, self.user)
@@ -602,7 +602,7 @@
     def test_tags_added(self):
         # Adding tags to a bug will add BugActivity and BugNotification
         # entries.
-        old_tags = self.changeAttribute(
+        self.changeAttribute(
             self.bug, 'tags', ['first-new-tag', 'second-new-tag'])
 
         tag_change_activity = {
@@ -626,7 +626,7 @@
         # entries.
         self.bug.tags = ['first-new-tag', 'second-new-tag']
         self.saveOldChanges()
-        old_tags = self.changeAttribute(
+        self.changeAttribute(
             self.bug, 'tags', ['first-new-tag'])
 
         tag_change_activity = {
@@ -1017,7 +1017,7 @@
         target = self.factory.makeDistributionSourcePackage()
         metadata_subscriber = self.newSubscriber(
             target, "dsp-metadata", BugNotificationLevel.METADATA)
-        lifecycle_subscriber = self.newSubscriber(
+        self.newSubscriber(
             target, "dsp-lifecycle", BugNotificationLevel.LIFECYCLE)
         new_target = self.factory.makeDistributionSourcePackage(
             distribution=target.distribution)
@@ -1739,7 +1739,7 @@
         # do not get any bug email that they generated themselves.
         self.user.selfgenerated_bugnotifications = False
 
-        old_description = self.changeAttribute(
+        self.changeAttribute(
             self.bug, 'description', 'New description')
 
         # self.user is not included among the recipients.
@@ -1756,7 +1756,22 @@
 
         self.user.selfgenerated_bugnotifications = False
 
-        old_description = self.changeAttribute(
+        self.changeAttribute(
+            self.bug, 'description', 'New description')
+
+        # self.user is not included among the recipients.
+        self.assertRecipients(
+            [self.product_metadata_subscriber, team.teamowner])
+
+    def test_description_changed_no_muted_email(self):
+        # Users who have muted a bug do not get any bug email for a bug,
+        # even if they are subscribed through a team membership.
+        team = self.factory.makeTeam()
+        team.addMember(self.user, team.teamowner)
+        self.bug.subscribe(team, self.user)
+        self.bug.mute(self.user, self.user)
+
+        self.changeAttribute(
             self.bug, 'description', 'New description')
 
         # self.user is not included among the recipients.
@@ -1770,7 +1785,7 @@
         self.bug.subscribe(self.product_metadata_subscriber,
                            self.product_metadata_subscriber,
                            level=BugNotificationLevel.LIFECYCLE)
-        old_description = self.changeAttribute(
+        self.changeAttribute(
             self.bug, 'description', 'New description')
 
         # self.product_metadata_subscriber is not included among the

=== modified file 'lib/lp/bugs/tests/test_bugnotification.py'
--- lib/lp/bugs/tests/test_bugnotification.py	2011-05-12 21:33:10 +0000
+++ lib/lp/bugs/tests/test_bugnotification.py	2011-05-17 11:44:40 +0000
@@ -233,11 +233,11 @@
     def test_getRecipientFilterData_empty(self):
         # When there is empty input, there is empty output.
         self.assertEqual(
-            BugNotificationSet().getRecipientFilterData({}, []),
+            BugNotificationSet().getRecipientFilterData(self.bug, {}, []),
             {})
         self.assertEqual(
             BugNotificationSet().getRecipientFilterData(
-                {}, [self.notification]),
+                self.bug, {}, [self.notification]),
             {})
 
     def test_getRecipientFilterData_other_persons(self):
@@ -259,7 +259,7 @@
              subscriber2: {'sources': sources2,
                            'filter descriptions': [u'Special Filter!']}},
             BugNotificationSet().getRecipientFilterData(
-                {self.subscriber: sources, subscriber2: sources2},
+                self.bug, {self.subscriber: sources, subscriber2: sources2},
                 [self.notification, notification2]))
 
     def test_getRecipientFilterData_match(self):
@@ -271,7 +271,7 @@
             {self.subscriber: {'sources': sources,
              'filter descriptions': ['Special Filter!']}},
             BugNotificationSet().getRecipientFilterData(
-                {self.subscriber: sources}, [self.notification]))
+                self.bug, {self.subscriber: sources}, [self.notification]))
 
     def test_getRecipientFilterData_multiple_notifications_match(self):
         # When there are bug filters for the recipient for multiple
@@ -285,7 +285,7 @@
             {self.subscriber: {'sources': sources,
              'filter descriptions': ['Another Filter!', 'Special Filter!']}},
             BugNotificationSet().getRecipientFilterData(
-                {self.subscriber: sources},
+                self.bug, {self.subscriber: sources},
                 [self.notification, self.notification2]))
 
     def test_getRecipientFilterData_mute(self):
@@ -300,7 +300,7 @@
         self.assertEqual(
             {},
             BugNotificationSet().getRecipientFilterData(
-                {self.subscriber: sources}, [self.notification]))
+                self.bug, {self.subscriber: sources}, [self.notification]))
 
     def test_getRecipientFilterData_mute_one_person_of_two(self):
         self.includeFilterInNotification()
@@ -321,7 +321,7 @@
             {subscriber2: {'sources': sources2,
                            'filter descriptions': [u'Special Filter!']}},
             BugNotificationSet().getRecipientFilterData(
-                {self.subscriber: sources, subscriber2: sources2},
+                self.bug, {self.subscriber: sources, subscriber2: sources2},
                 [self.notification, notification2]))
 
     def test_getRecipientFilterData_mute_one_filter_of_two(self):
@@ -337,7 +337,7 @@
             {self.subscriber: {'sources': sources,
              'filter descriptions': ['Another Filter!']}},
             BugNotificationSet().getRecipientFilterData(
-                {self.subscriber: sources},
+                self.bug, {self.subscriber: sources},
                 [self.notification, self.notification2]))
 
     def test_getRecipientFilterData_mute_both_filters_mutes(self):
@@ -356,9 +356,51 @@
         self.assertEqual(
             {},
             BugNotificationSet().getRecipientFilterData(
-                {self.subscriber: sources},
+                self.bug, {self.subscriber: sources},
                 [self.notification, self.notification2]))
 
+    def test_getRecipientFilterData_mute_bug_mutes(self):
+        # Mute the bug for the subscriber.
+        self.team = self.factory.makeTeam()
+        self.subscriber.join(self.team)
+
+        self.bug.mute(self.subscriber, self.subscriber)
+        sources = list(self.notification.recipients)
+        # Perform the test.
+        self.assertEqual(
+            {},
+            BugNotificationSet().getRecipientFilterData(
+                self.bug, {self.subscriber: sources}, [self.notification]))
+
+    def test_getRecipientFilterData_mute_bug_mutes_only_themselves(self):
+        # Mute the bug for the subscriber.
+        self.bug.mute(self.subscriber, self.subscriber)
+
+        # Notification for the other person still goes through.
+        person = self.factory.makePerson(name='other')
+        self.addNotificationRecipient(self.notification, person)
+
+        sources = list(self.notification.recipients)
+
+        # Perform the test.
+        self.assertEqual(
+            {person: {'filter descriptions': [],
+                      'sources': sources}},
+            BugNotificationSet().getRecipientFilterData(
+                self.bug, {self.subscriber: sources,
+                           person: sources},
+                [self.notification]))
+
+    def test_getRecipientFilterData_mute_bug_mutes_filter(self):
+        # Mute the bug for the subscriber.
+        self.bug.mute(self.subscriber, self.subscriber)
+        self.includeFilterInNotification(description=u'Special Filter!')
+        sources = list(self.notification.recipients)
+        self.assertEqual(
+            {},
+            BugNotificationSet().getRecipientFilterData(
+                self.bug, {self.subscriber: sources}, [self.notification]))
+
 
 class TestNotificationProcessingWithoutRecipients(TestCaseWithFactory):
     """Adding notificatons without any recipients does not cause any harm.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-05-10 18:02:28 +0000
+++ lib/lp/registry/model/person.py	2011-05-17 11:44:40 +0000
@@ -4041,6 +4041,9 @@
         # We ignore BugSubscriptionFilterMutes.
         skip.append(('bugsubscriptionfiltermute', 'person'))
 
+        # We ignore BugMutes.
+        skip.append(('bugmute', 'person'))
+
         self._mergePackageBugSupervisor(cur, from_id, to_id)
         skip.append(('packagebugsupervisor', 'bug_supervisor'))
 
@@ -4663,7 +4666,7 @@
     if person.preferredemail:
         return [person]
     elif person.is_team:
-        # Get transitive members of team with a preferred email.
+        # Get transitive members of team without a preferred email.
         return _get_recipients_for_team(person)
     else:
         return []