launchpad-reviewers team mailing list archive

[Данило Шеган <danilo@xxxxxxxxxxxxx>]

Данило Шеган has proposed merging lp:~danilo/launchpad/bug-548-use-preference into lp:launchpad/db-devel with lp:~gary/launchpad/bug548-db-2 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #548 Launchpad sends change notification updates to the person who requested the change
  https://bugs.launchpad.net/bugs/548

For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug-548-use-preference/+merge/48616

= 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.
-- 
https://code.launchpad.net/~danilo/launchpad/bug-548-use-preference/+merge/48616
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-548-use-preference into lp:launchpad/db-devel.

=== 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/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-02-04 01:15:13 +0000
+++ lib/lp/bugs/model/bug.py	2011-02-04 14:28:44 +0000
@@ -1034,6 +1034,7 @@
         if recipients is None:
             recipients = self.getBugNotificationRecipients(
                 level=BugNotificationLevel.METADATA)
+
         if when is None:
             when = UTC_NOW
         message = MessageSet().fromText(

=== 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 14:28:44 +0000
@@ -49,7 +49,11 @@
     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.selfgenerated_bugnotifications and
+                person in email_people):
+                email_people.remove(person)
+            for email_person in email_people:
                 recipients[email_person] = recipient
 
     for notification in bug_notifications:

=== 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 14:28:44 +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('foo.bar@xxxxxxxxxxxxx')
+        super(TestBugChanges, self).setUp('foo.bar@xxxxxxxxxxxxx')
         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])