launchpad-reviewers team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Brad Crittenden has proposed merging lp:~bac/launchpad/bugjam-579502 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #579502 Do not send notifications to registrants (and never if the pillar does not use bugs)
  https://bugs.launchpad.net/bugs/579502


= Summary =

If product or distro does not use Launchpad for bugs then the owner
should not be notified of bug state changes.

== Proposed fix ==

Check official_malone before adding the registrant.

== Pre-implementation notes ==

Talk with Curtis.

== Implementation details ==

As above.

== Tests ==

bin/test -vvm lp.bugs -t test_bugnotification

== Demo and Q/A ==

Create a distribution doesn't use Launchpad for bugs.  Create a bugtask
affecting the distro as a different user.  Ensure the distro owner does
not get an email.  (Will this work? Need to find out about qastaging and
sending email.)

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/subscribers/bug.py
  lib/lp/bugs/tests/test_bugnotification.py
  lib/lp/bugs/model/bug.py
-- 
https://code.launchpad.net/~bac/launchpad/bugjam-579502/+merge/44302
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bugjam-579502 into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2010-11-05 09:16:14 +0000
+++ lib/lp/bugs/model/bug.py	2010-12-20 23:21:17 +0000
@@ -988,7 +988,7 @@
             # If the target's bug supervisor isn't set,
             # we add the owner as a subscriber.
             pillar = bugtask.pillar
-            if pillar.bug_supervisor is None:
+            if pillar.official_malone and pillar.bug_supervisor is None:
                 also_notified_subscribers.add(pillar.owner)
                 if recipients is not None:
                     recipients.addRegistrant(pillar.owner, pillar)

=== modified file 'lib/lp/bugs/subscribers/bug.py'
--- lib/lp/bugs/subscribers/bug.py	2010-10-25 13:22:46 +0000
+++ lib/lp/bugs/subscribers/bug.py	2010-12-20 23:21:17 +0000
@@ -187,7 +187,7 @@
     # If the target's bug supervisor isn't set,
     # we add the owner as a subscriber.
     pillar = bugtask.pillar
-    if pillar.bug_supervisor is None:
+    if pillar.official_malone and pillar.bug_supervisor is None:
         also_notified_subscribers.add(pillar.owner)
         if recipients is not None:
             recipients.addRegistrant(pillar.owner, pillar)

=== modified file 'lib/lp/bugs/tests/test_bugnotification.py'
--- lib/lp/bugs/tests/test_bugnotification.py	2010-10-19 22:06:16 +0000
+++ lib/lp/bugs/tests/test_bugnotification.py	2010-12-20 23:21:17 +0000
@@ -9,6 +9,7 @@
 
 from lazr.lifecycle.event import ObjectModifiedEvent
 from lazr.lifecycle.snapshot import Snapshot
+from testtools.matchers import Not
 from zope.event import notify
 from zope.interface import providedBy
 
@@ -30,6 +31,7 @@
     )
 from lp.testing import TestCaseWithFactory
 from lp.testing.factory import LaunchpadObjectFactory
+from lp.testing.matchers import Contains
 from lp.testing.mail_helpers import pop_notifications
 
 
@@ -41,12 +43,16 @@
     def setUp(self):
         login('foo.bar@xxxxxxxxxxxxx')
         factory = LaunchpadObjectFactory()
-        self.product = factory.makeProduct()
-        self.product_subscriber = factory.makePerson()
+        self.product_owner = factory.makePerson(name="product-owner")
+        self.product = factory.makeProduct(owner=self.product_owner)
+        self.product_subscriber = factory.makePerson(
+            name="product-subscriber")
         self.product.addBugSubscription(
             self.product_subscriber, self.product_subscriber)
-        self.bug_subscriber = factory.makePerson()
-        self.private_bug = factory.makeBug(product=self.product, private=True)
+        self.bug_subscriber = factory.makePerson(name="bug-subscriber")
+        self.bug_owner = factory.makePerson(name="bug-owner")
+        self.private_bug = factory.makeBug(
+            product=self.product, private=True, owner=self.bug_owner)
         self.reporter = self.private_bug.owner
         self.private_bug.subscribe(self.bug_subscriber, self.reporter)
         [self.product_bugtask] = self.private_bug.bugtasks
@@ -66,7 +72,7 @@
         notified_people = set(
             recipient.person.name
             for recipient in latest_notification.recipients)
-        self.assertEqual(notified_people, self.direct_subscribers)
+        self.assertEqual(self.direct_subscribers, notified_people)
 
     def test_add_comment(self):
         # Comment additions are sent to the direct subscribers only.
@@ -76,7 +82,7 @@
         notified_people = set(
             recipient.person.name
             for recipient in latest_notification.recipients)
-        self.assertEqual(notified_people, self.direct_subscribers)
+        self.assertEqual(self.direct_subscribers, notified_people)
 
     def test_bug_edit(self):
         # Bug edits are sent to direct the subscribers only.
@@ -90,7 +96,7 @@
         notified_people = set(
             recipient.person.name
             for recipient in latest_notification.recipients)
-        self.assertEqual(notified_people, self.direct_subscribers)
+        self.assertEqual(self.direct_subscribers, notified_people)
 
 
 class TestNotificationsSentForBugExpiration(TestCaseWithFactory):
@@ -206,3 +212,79 @@
             recipient.person
             for recipient in latest_notification.recipients)
         self.assertEqual(self.dupe_subscribers, recipients)
+
+
+class TestNotificationsForRegistrants(TestCaseWithFactory):
+    """Test when registrants get notified."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestNotificationsForRegistrants, self).setUp(
+            user='foo.bar@xxxxxxxxxxxxx')
+        self.distro_owner = self.factory.makePerson(name="distro-owner")
+        self.bug_owner = self.factory.makePerson(name="bug-owner")
+        self.distribution = self.factory.makeDistribution(
+            owner=self.distro_owner)
+        self.bug = self.factory.makeBug(
+            distribution=self.distribution,
+            owner=self.bug_owner)
+
+    def test_notification_uses_malone(self):
+        self.distribution.official_malone = True
+        direct = self.bug.getDirectSubscribers()
+        indirect = self.bug.getIndirectSubscribers()
+        self.assertThat(direct, Not(Contains(self.distro_owner)))
+        self.assertThat(indirect, Contains(self.distro_owner))
+
+    def test_notification_does_not_use_malone(self):
+        self.distribution.official_malone = False
+        direct = self.bug.getDirectSubscribers()
+        indirect = self.bug.getIndirectSubscribers()
+        self.assertThat(direct, Not(Contains(self.distro_owner)))
+        self.assertThat(indirect, Not(Contains(self.distro_owner)))
+
+    def test_status_change_uses_malone(self):
+        # Status changes are sent to the direct and indirect subscribers.
+        self.distribution.official_malone = True
+        [bugtask] = self.bug.bugtasks
+        all_subscribers = set(
+            [person.name for person in
+             self.bug.getDirectSubscribers() +
+             self.bug.getIndirectSubscribers()])
+        bugtask_before_modification = Snapshot(
+            bugtask, providing=providedBy(bugtask))
+        bugtask.transitionToStatus(
+            BugTaskStatus.INVALID, self.bug.owner)
+        notify(ObjectModifiedEvent(
+            bugtask, bugtask_before_modification, ['status'],
+            user=self.bug.owner))
+        latest_notification = BugNotification.selectFirst(orderBy='-id')
+        notified_people = set(
+            recipient.person.name
+            for recipient in latest_notification.recipients)
+        self.assertEqual(all_subscribers, notified_people)
+        self.assertThat(all_subscribers, Contains(self.distro_owner.name))
+
+    def test_status_change_does_not_use_malone(self):
+        # Status changes are sent to the direct and indirect subscribers.
+        self.distribution.official_malone = False
+        [bugtask] = self.bug.bugtasks
+        all_subscribers = set(
+            [person.name for person in
+             self.bug.getDirectSubscribers() +
+             self.bug.getIndirectSubscribers()])
+        bugtask_before_modification = Snapshot(
+            bugtask, providing=providedBy(bugtask))
+        bugtask.transitionToStatus(
+            BugTaskStatus.INVALID, self.bug.owner)
+        notify(ObjectModifiedEvent(
+            bugtask, bugtask_before_modification, ['status'],
+            user=self.bug.owner))
+        latest_notification = BugNotification.selectFirst(orderBy='-id')
+        notified_people = set(
+            recipient.person.name
+            for recipient in latest_notification.recipients)
+        self.assertEqual(all_subscribers, notified_people)
+        self.assertThat(
+            all_subscribers, Not(Contains(self.distro_owner.name)))