launchpad-reviewers team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Gary Poster has proposed merging lp:~gary/launchpad/bug713382 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #713382 Direct subscription event filters should take precedence over structural subscriptions
  https://bugs.launchpad.net/bugs/713382

For more details, see:
https://code.launchpad.net/~gary/launchpad/bug713382/+merge/49145

This branch fixes the linked bug with a test (from Danilo) and a simple fix.  The use of the temporary recipients object is a bit odd and ugly, but we want to know why the recipients were added, which is the point of the recipients collection, but we don't know yet if we can add them.  Therefore, I did what I did.

make lint is happy.

Run the test with "./bin/test -vvt test_no_lifecycle_email_despite_structural_subscription" as you'd expect.

Thank you

Gary
-- 
https://code.launchpad.net/~gary/launchpad/bug713382/+merge/49145
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug713382 into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-02-07 09:39:54 +0000
+++ lib/lp/bugs/model/bug.py	2011-02-09 21:57:00 +0000
@@ -985,13 +985,24 @@
                         recipients.addRegistrant(pillar.owner, pillar)
 
         # Structural subscribers.
+        if recipients is None:
+            temp_recipients = None
+        else:
+            temp_recipients = BugNotificationRecipients(
+                duplicateof=recipients.duplicateof)
         also_notified_subscribers.update(
             getUtility(IBugTaskSet).getStructuralSubscribers(
-                self.bugtasks, recipients=recipients, level=level))
+                self.bugtasks, recipients=temp_recipients, level=level))
 
         # Direct subscriptions always take precedence over indirect
         # subscriptions.
         direct_subscribers = set(self.getDirectSubscribers())
+        if recipients is not None:
+            # A direct subscriber may have muted this notification.
+            # Therefore, we want to remove any direct subscribers from the
+            # structural subscription recipients before we merge.
+            temp_recipients.remove(direct_subscribers)
+            recipients.update(temp_recipients)
 
         # Remove security proxy for the sort key, but return
         # the regular proxied object.

=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
--- lib/lp/bugs/tests/test_bugchanges.py	2011-02-04 22:32:36 +0000
+++ lib/lp/bugs/tests/test_bugchanges.py	2011-02-09 21:57:00 +0000
@@ -1630,3 +1630,17 @@
         # self.user is not included among the recipients.
         self.assertRecipients(
             [self.product_metadata_subscriber, team.teamowner])
+
+    def test_no_lifecycle_email_despite_structural_subscription(self):
+        # If a person has a structural METADATA subscription,
+        # and a direct LIFECYCLE subscription, they should
+        # get no emails for a non-LIFECYCLE change (bug 713382).
+        self.bug.subscribe(self.product_metadata_subscriber,
+                           self.product_metadata_subscriber,
+                           level=BugNotificationLevel.LIFECYCLE)
+        old_description = self.changeAttribute(
+            self.bug, 'description', 'New description')
+
+        # self.product_metadata_subscriber is not included among the
+        # recipients.
+        self.assertRecipients([self.user])