launchpad-reviewers team mailing list archive

[Benji York <benji@xxxxxxxxxxxxx>]

Benji York has proposed merging lp:~benji/launchpad/bug-739141 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #739141 in Launchpad itself: "I get lp bug mail even though  'on own actions' turned off"
  https://bugs.launchpad.net/launchpad/+bug/739141

For more details, see:
https://code.launchpad.net/~benji/launchpad/bug-739141/+merge/59260

Bug 739141 describes a bug notification message being sent to a user about an action that they performed despite the fact that they have the "Send me bug notifications for changes I make" option disabled.

It turns out that in the given situation the user was a new subscriber to the bug.  Those notifications are handled by send_bug_details_to_new_bug_subscribers which did not respect the selfgenerated_bugnotifications flag.

The make lint report is clean.

To run the test:

bin/test -c -t test_self_notification_preference_respected -m lp.bugs.subscribers.tests
-- 
https://code.launchpad.net/~benji/launchpad/bug-739141/+merge/59260
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-739141 into lp:launchpad.

=== modified file 'lib/lp/bugs/subscribers/bug.py'
--- lib/lp/bugs/subscribers/bug.py	2011-03-07 17:39:20 +0000
+++ lib/lp/bugs/subscribers/bug.py	2011-04-27 17:29:35 +0000
@@ -190,17 +190,23 @@
     This function is designed to handle situations where bugtasks get
     reassigned to new products or sourcepackages, and the new bug subscribers
     need to be notified of the bug.
+
+    A boolean is returned indicating whether any emails were sent.
     """
     prev_subs_set = set(previous_subscribers)
     cur_subs_set = set(current_subscribers)
     new_subs = cur_subs_set.difference(prev_subs_set)
 
+    if (event_creator is not None
+            and not event_creator.selfgenerated_bugnotifications):
+        new_subs.discard(event_creator)
+
     to_addrs = set()
     for new_sub in new_subs:
         to_addrs.update(get_contact_email_addresses(new_sub))
 
     if not to_addrs:
-        return
+        return False
 
     from_addr = format_address(
         'Launchpad Bug Tracker',
@@ -226,3 +232,5 @@
             from_addr, to_addr, contents, subject, email_date,
             rationale=rationale, references=references)
         sendmail(msg)
+
+    return True

=== modified file 'lib/lp/bugs/subscribers/tests/test_bug.py'
--- lib/lp/bugs/subscribers/tests/test_bug.py	2011-02-04 01:15:13 +0000
+++ lib/lp/bugs/subscribers/tests/test_bug.py	2011-04-27 17:29:35 +0000
@@ -1,6 +1,8 @@
 # Copyright 2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+__metaclass__ = type
+
 from storm.store import Store
 
 from canonical.launchpad.webapp.publisher import canonical_url
@@ -13,9 +15,17 @@
     BugNotification,
     BugNotificationRecipient,
     )
-from lp.bugs.subscribers.bug import add_bug_change_notifications
+from lp.bugs.subscribers.bug import (
+    add_bug_change_notifications,
+    send_bug_details_to_new_bug_subscribers,
+    )
 from lp.registry.model.person import Person
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    TestCase,
+    TestCaseWithFactory,
+    )
+
+from testtools.matchers import Is
 
 
 class BugSubscriberTestCase(TestCaseWithFactory):
@@ -130,3 +140,20 @@
         # Only METADATA subscribers get notified.
         self.assertContentEqual(
             [self.metadata_subscriber], self.getNotifiedPersons())
+
+
+class FauxPerson:
+    selfgenerated_bugnotifications = False
+
+
+class NewSubscribers(TestCase):
+
+    def test_self_notification_preference_respected(self):
+        # If the person modifying the bug does not want to be notified about
+        # their own changes, they will not be.
+        actor = FauxPerson()
+        any_sent = send_bug_details_to_new_bug_subscribers(
+            None, [], [actor], event_creator=actor)
+        # Since the creator of the event was the only person to be notified
+        # and they don't want self-notifications, no messages were sent.
+        self.assertThat(any_sent, Is(False))