Merge lp:~danilo/launchpad/bug-548-use-preference into lp:launchpad/db-devel

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

Mute self-generated bug notifications for accounts that set that option (iow, mute themselves).

= Mute self-generated bug notifications =

For users who choose to not receive any emails resulting from their own activity in the bug tracker we should stop sending emails. This is part of our effort to solve bug 548. It still depends on unlanded work by Gary, and we expect to see another branch which enables setting this through a web page.

== Implementation details ==

I've discussed the implementation with Gavin and Deryck. My initial efforts were focused on not even creating a BugNotification record for a user who decides not to receive these emails, but because of potential indirect subscriptions through team membership, we had to move this to the code actually producing email notifications and "flattening" out recipients based on team membership. This means that it's all in one place now (all 3 lines of code changes), and should have less chances of conflicting with ongoing refactoring work by Gavin (which touches IBug.addChangeNotification).

== Tests ==

bin/test -cvvt no_self_email

== Demo & QA ==

To QA this, we need to set person.selfgenerated_bugnotifications to False (perhaps through a query like "UPDATE PersonSettings SET selfgenerated_bugnotifications=FALSE WHERE person IN (SELECT id FROM Person WHERE name='your-name')"), and then we need to try modifying any bug, and after running cronscripts/send-bug-notifications.py, no notification should go out for our own account.

It's very nice that "cronscripts/send-bug-notifications.py -vv" actually prints out all outgoing notifications, so that will hugely help with QA (no need to have to chase out notifications in the staging inbox).

[Brad Crittenden (bac)]

The change looks good Danilo. As we discussed on IRC please change the variable 'person' in bugnotification.py to something more descriptive that indicates that person is the one who took the action generating the notification. I suggested 'instigator' but there may be something better.

review: Approve (code)

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2011-02-04 17:56:21 +0000
+++ database/schema/security.cfg	2011-02-04 17:56:23 +0000
@@ -578,7 +578,9 @@
 public.milestone                        = SELECT
 public.packagebugsupervisor             = SELECT
 public.person                           = SELECT, INSERT, UPDATE
+public.personsettings                   = SELECT, INSERT
 public.personlanguage                   = SELECT
+public.personsettings                   = SELECT, INSERT
 public.product                          = SELECT, UPDATE
 public.productseries                    = SELECT
 public.project                          = SELECT, UPDATE
@@ -1093,6 +1095,7 @@
 public.packageselection                 = SELECT, INSERT, UPDATE
 public.packaging                        = SELECT, INSERT, UPDATE
 public.person                           = SELECT, INSERT, UPDATE
+public.personsettings                   = SELECT, INSERT, UPDATE
 public.personlanguage                   = SELECT, INSERT, UPDATE
 public.pocketchroot                     = SELECT, INSERT, UPDATE
 public.pocomment                        = SELECT, INSERT, UPDATE
@@ -1211,6 +1214,7 @@
 public.account                          = SELECT, INSERT
 public.accountpassword                  = SELECT, INSERT
 public.person                           = SELECT, INSERT, UPDATE
+public.personsettings                   = SELECT, INSERT
 public.emailaddress                     = SELECT, INSERT, UPDATE
 public.teamparticipation                = SELECT, INSERT
 public.teammembership                   = SELECT
@@ -1323,6 +1327,7 @@
 # Announce handling
 public.account                          = SELECT, INSERT
 public.person                           = SELECT, INSERT
+public.personsettings                   = SELECT, INSERT
 public.emailaddress                     = SELECT, INSERT
 public.teamparticipation                = SELECT, INSERT
 public.teammembership                   = SELECT
@@ -1763,6 +1768,7 @@
 public.mailinglist                      = SELECT, INSERT, UPDATE
 public.mailinglistsubscription          = SELECT, INSERT, UPDATE
 public.person                           = SELECT, INSERT, UPDATE
+public.personsettings                   = SELECT, INSERT
 public.teammembership                   = SELECT, INSERT, UPDATE
 public.teamparticipation                = SELECT, INSERT, UPDATE
 
@@ -1944,6 +1950,7 @@
 groups=script
 public.account                          = SELECT, INSERT, UPDATE
 public.person                           = SELECT, INSERT
+public.personsettings                   = SELECT, INSERT
 public.product                          = SELECT, INSERT, UPDATE
 public.productseries                    = SELECT, INSERT
 public.productlicense                   = SELECT, INSERT

=== added file 'lib/lp/app/widgets/__init__.py'
--- lib/lp/app/widgets/__init__.py	1970-01-01 00:00:00 +0000
+++ lib/lp/app/widgets/__init__.py	2011-02-02 15:37:35 +0000
@@ -0,0 +1,2 @@
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).

=== modified file 'lib/lp/bugs/doc/bugnotification-sending.txt'
--- lib/lp/bugs/doc/bugnotification-sending.txt	2011-02-04 17:56:21 +0000
+++ lib/lp/bugs/doc/bugnotification-sending.txt	2011-02-04 17:56:23 +0000
@@ -30,20 +30,9 @@
     ...     print email_notification.get_payload(decode=True)
     ...     print "-" * 70
 
-We'll also define some helper functions to help us with database users.
-
-    >>> from canonical.config import config
-    >>> from canonical.database.sqlbase import commit
-    >>> from canonical.testing.layers import LaunchpadZopelessLayer
-
-    >>> def switch_db_to_launchpad():
-    ...     commit()
-    ...     LaunchpadZopelessLayer.switchDbUser('launchpad')
-
-    >>> def switch_db_to_bugnotification():
-    ...     commit()
-    ...     LaunchpadZopelessLayer.switchDbUser(
-    ...         config.malone.bugnotification_dbuser)
+We'll also import a helper function to help us with database users.
+
+    >>> from lp.testing.dbuser import lp_dbuser
 
 You'll note that we are printing out an X-Launchpad-Message-Rationale
 header. This header is a simple string that allows people to filter
@@ -356,16 +345,13 @@
     ...     print member.preferredemail.email
     [e-mail]
 
-    >>> switch_db_to_launchpad()
-    >>> bug_one.subscribe(shipit_admins, shipit_admins)
-    <...>
-
-    >>> comment = getUtility(IMessageSet).fromText(
-    ...     'subject', 'a comment.', sample_person,
-    ...     datecreated=ten_minutes_ago)
-    >>> bug_one.addCommentNotification(comment)
-
-    >>> switch_db_to_bugnotification()
+    >>> with lp_dbuser():
+    ...     ignored = bug_one.subscribe(shipit_admins, shipit_admins)
+    ...     comment = getUtility(IMessageSet).fromText(
+    ...         'subject', 'a comment.', sample_person,
+    ...         datecreated=ten_minutes_ago)
+    ...     bug_one.addCommentNotification(comment)
+
     >>> pending_notifications = getUtility(
     ...     IBugNotificationSet).getNotificationsToSend()
     >>> len(pending_notifications)
@@ -398,17 +384,15 @@
     >>> params = CreateBugParams(
     ...     msg=description, owner=sample_person, title='new bug')
 
-    >>> switch_db_to_launchpad()
-    >>> new_bug = ubuntu.createBug(params)
-    >>> switch_db_to_bugnotification()
-    >>> flush_notifications()
+    >>> with lp_dbuser():
+    ...     new_bug = ubuntu.createBug(params)
 
 If a bug is a duplicate of another bug, a marker gets inserted at the
 top of the email:
 
-    >>> switch_db_to_launchpad()
-    >>> new_bug.markAsDuplicate(bug_one)
-    >>> switch_db_to_bugnotification()
+    >>> flush_notifications()
+    >>> with lp_dbuser():
+    ...     new_bug.markAsDuplicate(bug_one)
     >>> comment = getUtility(IMessageSet).fromText(
     ...     'subject', 'a comment.', sample_person,
     ...     datecreated=ten_minutes_ago)
@@ -474,12 +458,11 @@
     ...     'Zero-day on Frobulator', 'Woah.', sample_person,
     ...     datecreated=ten_minutes_ago)
 
-    >>> switch_db_to_launchpad()
-    >>> sec_vuln_bug = ubuntu.createBug(CreateBugParams(
+    >>> with lp_dbuser():
+    ...     sec_vuln_bug = ubuntu.createBug(CreateBugParams(
     ...         msg=sec_vuln_description, owner=sample_person,
     ...         title='Zero-day on Frobulator',
     ...         security_related=True, private=True))
-    >>> switch_db_to_bugnotification()
 
     >>> sec_vuln_bug.security_related
     True
@@ -730,10 +713,8 @@
 The tags will be space-separated to allow the list to be wrapped if it
 gets over-long.
 
-    >>> switch_db_to_launchpad()
-    >>> bug_three.tags = [u'layout-test', u'another-tag', u'yet-another']
-
-    >>> switch_db_to_bugnotification()
+    >>> with lp_dbuser():
+    ...     bug_three.tags = [u'layout-test', u'another-tag', u'yet-another']
 
     >>> bug_three = getUtility(IBugSet).get(3)
     >>> for message in trigger_and_get_email_messages(bug_three):
@@ -743,15 +724,13 @@
 If we remove the tags from the bug, the X-Launchpad-Bug-Tags header
 won't be included.
 
-    >>> switch_db_to_launchpad()
-    >>> bug_three.tags = []
-    >>> switch_db_to_bugnotification()
+    >>> with lp_dbuser():
+    ...     bug_three.tags = []
 
     >>> bug_three = getUtility(IBugSet).get(3)
     >>> for message in trigger_and_get_email_messages(bug_three):
     ...     message.get_all('X-Launchpad-Bug-Tags')
 
-    >>> switch_db_to_launchpad()
     >>> #bug_three.unsubscribe(sample_person, sample_person)
 
 
@@ -771,7 +750,8 @@
 
 Predictably, private bugs are sent with a slightly different header:
 
-    >>> bug_three.setPrivate(True, sample_person)
+    >>> with lp_dbuser():
+    ...     bug_three.setPrivate(True, sample_person)
     True
     >>> bug_three.private
     True
@@ -799,7 +779,8 @@
 The presence of the security flag on a bug is, surprise, denoted by a
 simple "yes":
 
-    >>> bug_three.setSecurityRelated(True)
+    >>> with lp_dbuser():
+    ...     bug_three.setSecurityRelated(True)
     True
     >>> bug_three.security_related
     True
@@ -824,9 +805,9 @@
     >>> foo_bar = getUtility(IPersonSet).getByEmail('[e-mail]')
 
     >>> from lp.bugs.interfaces.bugmessage import IBugMessageSet
-    >>> getUtility(IBugMessageSet).createMessage(
-    ...     'Hungry', bug_three, foo_bar, "Make me a sandwich.")
-    <BugMessage ...>
+    >>> with lp_dbuser():
+    ...     ignored = getUtility(IBugMessageSet).createMessage(
+    ...         'Hungry', bug_three, foo_bar, "Make me a sandwich.")
 
     >>> for message in trigger_and_get_email_messages(bug_three):
     ...     print message.get('X-Launchpad-Bug-Commenters')
@@ -835,9 +816,9 @@
 It only lists each user once, no matter how many comments they've
 made.
 
-    >>> getUtility(IBugMessageSet).createMessage(
-    ...     'Hungry', bug_three, foo_bar, "Make me a sandwich.")
-    <BugMessage ...>
+    >>> with lp_dbuser():
+    ...     ignored = getUtility(IBugMessageSet).createMessage(
+    ...         'Hungry', bug_three, foo_bar, "Make me a sandwich.")
 
     >>> for message in trigger_and_get_email_messages(bug_three):
     ...     print message.get('X-Launchpad-Bug-Commenters')
@@ -868,63 +849,58 @@
 Concise Person does not. We'll also create teams and give them members
 with different verbose_bugnotifications settings.
 
-    >>> switch_db_to_launchpad()
-    >>> bug = factory.makeBug(
-    ...     product=factory.makeProduct(title='Foo'),
-    ...     title='In the beginning, the universe was created. This '
-    ...         'has made a lot of people very angry and has been '
-    ...         'widely regarded as a bad move',
-    ...     description="This is a long description of the bug, which "
-    ...         "will be automatically wrapped by the BugNotification "
-    ...         "machinery. Ain't technology great?")
-
-    >>> verbose_person = factory.makePerson(
-    ...     displayname='Verbose Person', email='[e-mail]')
-    >>> verbose_person.verbose_bugnotifications = True
-    >>> bug.subscribe(verbose_person, verbose_person)
-    <lp.bugs.model.bugsubscription.BugSubscription ...>
-
-    >>> concise_person = factory.makePerson(
-    ...     displayname='Concise Person', email='[e-mail]')
-    >>> concise_person.verbose_bugnotifications = False
-    >>> bug.subscribe(concise_person, concise_person)
-    <lp.bugs.model.bugsubscription.BugSubscription ...>
+    >>> with lp_dbuser():
+    ...     bug = factory.makeBug(
+    ...         product=factory.makeProduct(title='Foo'),
+    ...         title='In the beginning, the universe was created. This '
+    ...             'has made a lot of people very angry and has been '
+    ...             'widely regarded as a bad move',
+    ...         description="This is a long description of the bug, which "
+    ...             "will be automatically wrapped by the BugNotification "
+    ...             "machinery. Ain't technology great?")
+    ...     verbose_person = factory.makePerson(
+    ...         displayname='Verbose Person', email='[e-mail]')
+    ...     verbose_person.verbose_bugnotifications = True
+    ...     ignored = bug.subscribe(verbose_person, verbose_person)
+    ...     concise_person = factory.makePerson(
+    ...         displayname='Concise Person', email='[e-mail]')
+    ...     concise_person.verbose_bugnotifications = False
+    ...     ignored = bug.subscribe(concise_person, concise_person)
 
 
 Concise Team doesn't want verbose notifications, while Concise Team
 Person, a member, does.
 
-    >>> concise_team = factory.makeTeam(
-    ...     name='conciseteam', displayname='Concise Team')
-    >>> concise_team.verbose_bugnotifications = False
-    >>> concise_team_person = factory.makePerson(
-    ...     displayname='Concise Team Person',
-    ...     email='[e-mail]')
-    >>> concise_team_person.verbose_bugnotifications = True
-    >>> ignored = concise_team.addMember(
-    ...     concise_team_person, concise_team_person)
-    >>> bug.subscribe(concise_team, concise_team_person)
-    <lp.bugs.model.bugsubscription.BugSubscription ...>
+    >>> with lp_dbuser():
+    ...     concise_team = factory.makeTeam(
+    ...         name='conciseteam', displayname='Concise Team')
+    ...     concise_team.verbose_bugnotifications = False
+    ...     concise_team_person = factory.makePerson(
+    ...         displayname='Concise Team Person',
+    ...         email='[e-mail]')
+    ...     concise_team_person.verbose_bugnotifications = True
+    ...     ignored = concise_team.addMember(
+    ...         concise_team_person, concise_team_person)
+    ...     ignored = bug.subscribe(concise_team, concise_team_person)
 
 Verbose Team wants verbose notifications, while Verbose Team Person, a
 member, does not.
 
-    >>> verbose_team = factory.makeTeam(
-    ...     name='verboseteam', displayname='Verbose Team')
-    >>> verbose_team.verbose_bugnotifications = True
-    >>> verbose_team_person = factory.makePerson(
-    ...     displayname='Verbose Team Person',
-    ...     email='[e-mail]')
-    >>> verbose_team_person.verbose_bugnotifications = False
-    >>> ignored = verbose_team.addMember(
-    ...     verbose_team_person, verbose_team_person)
-    >>> bug.subscribe(verbose_team, verbose_team_person)
-    <lp.bugs.model.bugsubscription.BugSubscription ...>
+    >>> with lp_dbuser():
+    ...     verbose_team = factory.makeTeam(
+    ...         name='verboseteam', displayname='Verbose Team')
+    ...     verbose_team.verbose_bugnotifications = True
+    ...     verbose_team_person = factory.makePerson(
+    ...         displayname='Verbose Team Person',
+    ...         email='[e-mail]')
+    ...     verbose_team_person.verbose_bugnotifications = False
+    ...     ignored = verbose_team.addMember(
+    ...         verbose_team_person, verbose_team_person)
+    ...     ignored = bug.subscribe(verbose_team, verbose_team_person)
 
 We'll expire all existing notifications since we're not interested in
 them:
 
-    >>> switch_db_to_bugnotification()
     >>> notifications = getUtility(
     ...     IBugNotificationSet).getNotificationsToSend()
     >>> len(notifications)
@@ -1103,19 +1079,22 @@
 
 People (not teams) will have the choice to receive notifications from actions
 they generated.  For now, everyone receives these notifications whether they
-want them or not.  However, we can show that the attribute exists for now.
+want them or not.
 
-    >>> flush_notifications()
-    >>> switch_db_to_launchpad()
     >>> verbose_person.selfgenerated_bugnotifications
     True
+    >>> with lp_dbuser():
+    ...     verbose_person.selfgenerated_bugnotifications = False
 
-Teams do not implement this attribute.
+Teams provide this attribute read-only.
 
     >>> verbose_team.selfgenerated_bugnotifications
+    True
+    >>> with lp_dbuser():
+    ...     verbose_team.selfgenerated_bugnotifications = False
     Traceback (most recent call last):
     ...
-    NotImplementedError: Teams do not support this attribute.
+    NotImplementedError: Teams do not support changing this attribute.
 
 Notification Recipients
 -----------------------
@@ -1125,16 +1104,14 @@
 notification level of the subscription.
 
     >>> flush_notifications()
-    >>> switch_db_to_launchpad()
 
     >>> from lp.bugs.enum import BugNotificationLevel
     >>> from lp.registry.interfaces.product import IProductSet
     >>> firefox = getUtility(IProductSet).getByName('firefox')
     >>> mr_no_privs = getUtility(IPersonSet).getByName('no-priv')
-    >>> subscription_no_priv = firefox.addBugSubscription(
-    ...     mr_no_privs, mr_no_privs)
-
-    >>> switch_db_to_bugnotification()
+    >>> with lp_dbuser():
+    ...     subscription_no_priv = firefox.addBugSubscription(
+    ...         mr_no_privs, mr_no_privs)
 
 The notifications generated by addCommentNotification() are sent only to
 structural subscribers with no filters, or with the notification level
@@ -1200,10 +1177,9 @@
 
 
     >>> flush_notifications()
-    >>> switch_db_to_launchpad()
-    >>> filter = subscription_no_priv.newBugFilter()
-    >>> filter.bug_notification_level = BugNotificationLevel.COMMENTS
-    >>> switch_db_to_bugnotification()
+    >>> with lp_dbuser():
+    ...     filter = subscription_no_priv.newBugFilter()
+    ...     filter.bug_notification_level = BugNotificationLevel.COMMENTS
 
     >>> comment = getUtility(IMessageSet).fromText(
     ...     'subject', 'another comment.', sample_person,
@@ -1261,9 +1237,8 @@
 no comment notifications.
 
     >>> flush_notifications()
-    >>> switch_db_to_launchpad()
-    >>> filter.bug_notification_level = BugNotificationLevel.METADATA
-    >>> switch_db_to_bugnotification()
+    >>> with lp_dbuser():
+    ...     filter.bug_notification_level = BugNotificationLevel.METADATA
 
     >>> comment = getUtility(IMessageSet).fromText(
     ...     'subject', 'no comment for no-priv.', sample_person,
@@ -1373,9 +1348,8 @@
 no notifications created by addChangeNotification().
 
     >>> flush_notifications()
-    >>> switch_db_to_launchpad()
-    >>> filter.bug_notification_level = BugNotificationLevel.LIFECYCLE
-    >>> switch_db_to_bugnotification()
+    >>> with lp_dbuser():
+    ...     filter.bug_notification_level = BugNotificationLevel.LIFECYCLE
 
     >>> bug_one.addChangeNotification('** Summary changed to: something.',
     ...     sample_person, when=ten_minutes_ago)
@@ -1433,10 +1407,9 @@
 after all.
 
     >>> flush_notifications()
-    >>> switch_db_to_launchpad()
-    >>> filter2 = subscription_no_priv.newBugFilter()
-    >>> filter2.bug_notification_level = BugNotificationLevel.METADATA
-    >>> switch_db_to_bugnotification()
+    >>> with lp_dbuser():
+    ...     filter2 = subscription_no_priv.newBugFilter()
+    ...     filter2.bug_notification_level = BugNotificationLevel.METADATA
 
     >>> bug_one.addChangeNotification('** Summary changed to: whatever.',
     ...     sample_person, when=ten_minutes_ago)

=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
--- lib/lp/bugs/scripts/bugnotification.py	2011-01-06 14:43:46 +0000
+++ lib/lp/bugs/scripts/bugnotification.py	2011-02-04 17:56:23 +0000
@@ -39,7 +39,7 @@
     """
     first_notification = bug_notifications[0]
     bug = first_notification.bug
-    person = first_notification.message.owner
+    person_causing_change = first_notification.message.owner
     subject = first_notification.message.subject
 
     comment = None
@@ -49,12 +49,17 @@
     recipients = {}
     for notification in bug_notifications:
         for recipient in notification.recipients:
-            for email_person in emailPeople(recipient.person):
+            email_people = emailPeople(recipient.person)
+            if (not person_causing_change.selfgenerated_bugnotifications and
+                person_causing_change in email_people):
+                email_people.remove(person_causing_change)
+            for email_person in email_people:
                 recipients[email_person] = recipient
 
     for notification in bug_notifications:
         assert notification.bug == bug, bug.id
-        assert notification.message.owner == person, person.id
+        assert notification.message.owner == person_causing_change, (
+            person_causing_change.id)
         if notification.is_comment:
             assert comment is None, (
                 "Only one of the notifications is allowed to be a comment.")
@@ -99,8 +104,9 @@
     messages = []
     mail_wrapper = MailWrapper(width=72)
     content = '\n\n'.join(text_notifications)
-    from_address = get_bugmail_from_address(person, bug)
-    bug_notification_builder = BugNotificationBuilder(bug, person)
+    from_address = get_bugmail_from_address(person_causing_change, bug)
+    bug_notification_builder = BugNotificationBuilder(
+        bug, person_causing_change)
     sorted_recipients = sorted(
         recipients.items(), key=lambda t: t[0].preferredemail.email)
     for email_person, recipient in sorted_recipients:

=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
--- lib/lp/bugs/tests/test_bugchanges.py	2011-02-04 01:15:13 +0000
+++ lib/lp/bugs/tests/test_bugchanges.py	2011-02-04 17:56:23 +0000
@@ -3,8 +3,6 @@
 
 """Tests for recording changes done to a bug."""
 
-import unittest
-
 from lazr.lifecycle.event import (
     ObjectCreatedEvent,
     ObjectModifiedEvent,
@@ -16,7 +14,6 @@
 
 from canonical.launchpad.browser.librarian import ProxiedLibraryFileAlias
 from lp.bugs.model.bugnotification import BugNotification
-from canonical.launchpad.ftests import login
 from canonical.launchpad.webapp.interfaces import ILaunchBag
 from canonical.launchpad.webapp.publisher import canonical_url
 from canonical.testing.layers import LaunchpadFunctionalLayer
@@ -27,18 +24,20 @@
     BugTaskStatus,
     )
 from lp.bugs.interfaces.cve import ICveSet
-from lp.testing.factory import LaunchpadObjectFactory
-from lp.testing import person_logged_in
-
-
-class TestBugChanges(unittest.TestCase):
+from lp.bugs.scripts.bugnotification import construct_email_notifications
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
+
+
+class TestBugChanges(TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
     def setUp(self):
-        login('[e-mail]')
+        super(TestBugChanges, self).setUp('[e-mail]')
         self.admin_user = getUtility(ILaunchBag).user
-        self.factory = LaunchpadObjectFactory()
         self.user = self.factory.makePerson(displayname='Arthur Dent')
         self.product = self.factory.makeProduct(
             owner=self.user, official_malone=True)
@@ -106,6 +105,16 @@
 
         return getattr(obj_before_modification, attribute)
 
+    def getNewNotifications(self, bug=None):
+        if bug is None:
+            bug = self.bug
+        bug_notifications = BugNotification.selectBy(
+            bug=bug, orderBy='id')
+        new_notifications = [
+            notification for notification in bug_notifications
+            if notification.id not in self.old_notification_ids]
+        return new_notifications
+
     def assertRecordedChange(self, expected_activity=None,
                              expected_notification=None, bug=None):
         """Assert that things were recorded as expected."""
@@ -114,11 +123,7 @@
         new_activities = [
             activity for activity in bug.activity
             if activity not in self.old_activities]
-        bug_notifications = BugNotification.selectBy(
-            bug=bug, orderBy='id')
-        new_notifications = [
-            notification for notification in bug_notifications
-            if notification.id not in self.old_notification_ids]
+        new_notifications = self.getNewNotifications(bug)
 
         if expected_activity is None:
             self.assertEqual(len(new_activities), 0)
@@ -173,6 +178,16 @@
                         for recipient in added_notification.recipients),
                     set(expected_recipients))
 
+    def assertRecipients(self, expected_recipients):
+        notifications = self.getNewNotifications()
+        notifications, messages = construct_email_notifications(notifications)
+        recipients = set(message['to'] for message in messages)
+
+        self.assertEqual(
+            set(recipient.preferredemail.email
+                for recipient in expected_recipients),
+            recipients)
+
     def test_subscribe(self):
         # Subscribing someone to a bug adds an item to the activity log,
         # but doesn't send an e-mail notification.
@@ -1586,3 +1601,32 @@
             expected_activity=expected_activity,
             expected_notification=expected_notification,
             bug=new_bug)
+
+    def test_description_changed_no_self_email(self):
+        # Users who have selfgenerated_bugnotifications set to False
+        # do not get any bug email that they generated themselves.
+        self.user.selfgenerated_bugnotifications = False
+
+        old_description = self.changeAttribute(
+            self.bug, 'description', 'New description')
+
+        # self.user is not included among the recipients.
+        self.assertRecipients(
+            [self.product_metadata_subscriber])
+
+    def test_description_changed_no_self_email_indirect(self):
+        # Users who have selfgenerated_bugnotifications set to False
+        # do not get any bug email that they generated themselves,
+        # even if a subscription is through a team membership.
+        team = self.factory.makeTeam()
+        team.addMember(self.user, team.teamowner)
+        self.bug.subscribe(team, self.user)
+
+        self.user.selfgenerated_bugnotifications = False
+
+        old_description = self.changeAttribute(
+            self.bug, 'description', 'New description')
+
+        # self.user is not included among the recipients.
+        self.assertRecipients(
+            [self.product_metadata_subscriber, team.teamowner])

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-02-04 17:56:21 +0000
+++ lib/lp/registry/model/person.py	2011-02-04 17:56:23 +0000
@@ -84,8 +84,10 @@
 from zope.event import notify
 from zope.interface import (
     alsoProvides,
+    directlyProvides,
     implementer,
     implements,
+    providedBy,
     )
 from zope.lifecycleevent import ObjectCreatedEvent
 from zope.publisher.interfaces import Unauthorized
@@ -372,6 +374,35 @@
     selfgenerated_bugnotifications = BoolCol(notNull=True, default=True)
 
 
+class UnwritableStub:
+
+    def __init__(self, error_msg, *interfaces):
+        self._error_msg = error_msg
+        directlyProvides(self, *interfaces)
+
+    def __getattr__(self, name):
+        # If the attribute doesn't exist, we'll get a None, which will not
+        # have "default" and thus will generate an AttributeError, which is
+        # correct for a __getattr__.  If the attribute does exist but is not
+        # a field with a default (such as a method), we will also get a
+        # contractually-acceptable AttributeError.  Therefore, we can be
+        # simple here.
+        return providedBy(self).get(name).default
+
+    def __setattr__(self, name, value):
+        if name.startswith('_'):
+            # The attributes we want to protect from writing are the ones on
+            # the interface, which should not be "underwear" attributes.  We
+            # need to be able to write these, particularly so that
+            # zope.interface machinery works.
+            self.__dict__[name] = value
+        else:
+            raise NotImplementedError(self._error_msg)
+
+
+_unwritable_person_settings_stub = UnwritableStub(
+    'Teams do not support changing this attribute.', IPersonSettings)
+
 class Person(
     SQLBase, HasBugsBase, HasSpecificationsMixin, HasTranslationImportsMixin,
     HasBranchesMixin, HasMergeProposalsMixin, HasRequestedReviewsMixin):
@@ -391,13 +422,11 @@
     @cachedproperty
     def _person_settings(self):
         if self.is_team:
-            # Hopefully no-one ever encounters this.  If someone does,
-            # that means that the code is trying to look at
-            # person-specific attributes on a team, and we should warn
-            # about that explicitly to give a hint about what is wrong
-            # (rather than merely returning None).
-            raise NotImplementedError(
-                'Teams do not support this attribute.')
+            # Teams need to provide these attributes for reading in order for
+            # things like snapshots to work, but they are not actually
+            # pertinent to teams, so they are not actually implemented for
+            # writes.
+            return _unwritable_person_settings_stub
         else:
             # This is a person.
             return IStore(PersonSettings).find(
@@ -2112,6 +2141,7 @@
             ('logintoken', 'requester'),
             ('personlanguage', 'person'),
             ('personlocation', 'person'),
+            ('personsettings', 'person'),
             ('persontransferjob', 'minor_person'),
             ('persontransferjob', 'major_person'),
             ('signedcodeofconduct', 'owner'),

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2011-01-31 09:24:13 +0000
+++ lib/lp/scripts/garbo.py	2011-02-04 17:56:23 +0000
@@ -431,10 +431,13 @@
         for (from_table, from_column, to_table, to_column, uflag, dflag) in (
                 postgresql.listReferences(cursor(), 'person', 'id')):
             # Skip things that don't link to Person.id or that link to it from
-            # TeamParticipation or EmailAddress, as all Person entries will be
-            # linked to from these tables.
+            # TeamParticipation, EmailAddress, as all Person entries will be
+            # linked to from these tables.  Similarly, PersonSettings can
+            # simply be deleted if it exists, because it has a 1 (or 0) to 1
+            # relationship with Person.
             if (to_table != 'person' or to_column != 'id'
-                or from_table in ('teamparticipation', 'emailaddress')):
+                or from_table in ('teamparticipation', 'emailaddress',
+                                  'personsettings')):
                 continue
             self.log.debug(
                 "Populating LinkedPeople from %s.%s"
@@ -510,6 +513,7 @@
                 UPDATE EmailAddress SET person=NULL
                 WHERE person IN (%s)
                 """ % people_ids)
+            # This cascade deletes any PersonSettings records.
             self.store.execute("""
                 DELETE FROM Person
                 WHERE id IN (%s)

=== added file 'lib/lp/testing/dbuser.py'
--- lib/lp/testing/dbuser.py	1970-01-01 00:00:00 +0000
+++ lib/lp/testing/dbuser.py	2011-02-04 17:56:23 +0000
@@ -0,0 +1,40 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Provides a context manager to run parts of a test as a different dbuser."""
+
+__metaclass__ = type
+__all__ = [
+    'dbuser',
+    'lp_dbuser',
+    ]
+
+from contextlib import contextmanager
+
+from canonical.database.sqlbase import commit
+from canonical.testing.layers import LaunchpadZopelessLayer
+
+@contextmanager
+def dbuser(temporary_name):
+    """A context manager that temporarily changes the dbuser.
+    
+    Use with the LaunchpadZopelessLayer layer and subclasses.
+    
+    temporary_name is the name of the dbuser that should be in place for the
+    code in the "with" block.
+    """
+    restore_name = LaunchpadZopelessLayer.txn._dbuser
+    commit()
+    # Note that this will raise an assertion error if the
+    # LaunchpadZopelessLayer is not already set up.
+    LaunchpadZopelessLayer.switchDbUser(temporary_name)
+    yield
+    commit()
+    LaunchpadZopelessLayer.switchDbUser(restore_name)
+
+def lp_dbuser():
+    """A context manager that temporarily changes to the launchpad dbuser.
+    
+    Use with the LaunchpadZopelessLayer layer and subclasses.
+    """
+    return dbuser('launchpad')