Merge lp:~bac/launchpad/bugjam-579502 into lp:launchpad

[Brad Crittenden]

[r=sinzui][ui=none][bug=579502] Do not send bug email to distro owner if the distro does not use Lauchpad for bugs.

= 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

[Curtis Hovey (sinzui)]

This looks good to land. I think this will do a lot to stop the avalanche of email.

review: Approve (code)

[Brad Crittenden (bac)]

Curtis I discovered many more test that depended upon the pillar owner receiving email notification. The diff for the fixes is at http://pastebin.ubuntu.com/546917/

=== modified file 'lib/lp/bugs/doc/bug-heat.txt'
--- lib/lp/bugs/doc/bug-heat.txt	2010-12-22 20:46:21 +0000
+++ lib/lp/bugs/doc/bug-heat.txt	2010-12-23 19:45:24 +0000
@@ -80,52 +80,52 @@
 
     >>> changed = bug.setPrivate(True, bug_owner)
     >>> bug.heat
-    158
+    156
 
 Setting the bug as security related adds another 250 heat points.
 
     >>> changed = bug.setSecurityRelated(True)
     >>> bug.heat
-    408
+    406
 
 Marking the bug public removes 150 heat points.
 
     >>> changed = bug.setPrivate(False, bug_owner)
     >>> bug.heat
-    258
+    256
 
 And marking it not security-related removes 250 points.
 
     >>> changed = bug.setSecurityRelated(False)
     >>> bug.heat
-    8
+    6
 
 Adding a subscriber to the bug increases its heat by 2 points.
 
     >>> new_subscriber = factory.makePerson()
     >>> subscription = bug.subscribe(new_subscriber, new_subscriber)
     >>> bug.heat
-    10
+    8
 
 When a user unsubscribes, the bug loses 2 points of heat.
 
     >>> bug.unsubscribe(new_subscriber, new_subscriber)
     >>> bug.heat
-    8
+    6
 
 Should a user mark themselves as affected by the bug, it will gain 4
 points of heat.
 
     >>> bug.markUserAffected(new_subscriber)
     >>> bug.heat
-    12
+    10
 
 If a user who was previously affected marks themself as not affected,
 the bug loses 4 points of heat.
 
     >>> bug.markUserAffected(new_subscriber, False)
     >>> bug.heat
-    8
+    6
 
 If a user who wasn't affected by the bug marks themselve as explicitly
 unaffected, the bug's heat doesn't change.
@@ -133,7 +133,7 @@
     >>> unaffected_person = factory.makePerson()
     >>> bug.markUserAffected(unaffected_person, False)
     >>> bug.heat
-    8
+    6
 
 Marking the bug as a duplicate will set its heat to zero, whilst also
 adding 10 points of heat to the bug it duplicates, 6 points for the
@@ -149,14 +149,14 @@
     0
 
     >>> duplicated_bug.heat
-    16
+    14
 
 Unmarking the bug as a duplicate restores its heat and updates the
 duplicated bug's heat.
 
     >>> bug.markAsDuplicate(None)
     >>> bug.heat
-    8
+    6
 
     >>> duplicated_bug.heat
     6
@@ -184,7 +184,7 @@
     ...     old_value=bug.description, new_value='Some text')
     >>> bug.addChange(change)
     >>> bug.heat
-    8
+    6
 
 
 Getting bugs whose heat is outdated
@@ -359,5 +359,3 @@
     123
     >>> print distro.max_bug_heat
     456
-
-

=== modified file 'lib/lp/bugs/doc/bugnotification-sending.txt'
--- lib/lp/bugs/doc/bugnotification-sending.txt	2010-12-06 17:43:53 +0000
+++ lib/lp/bugs/doc/bugnotification-sending.txt	2010-12-23 19:45:24 +0000
@@ -303,7 +303,7 @@
     <BLANKLINE>
     ** Visibility changed to: Public.
     <BLANKLINE>
-    -- 
+    --
     You received this bug notification because you are a member of Ubuntu
     Team, which is the registrant for Ubuntu.
     http://bugs.launchpad.dev/bugs/1
@@ -582,8 +582,6 @@
     0
     >>> print err
     DEBUG   ...
-    INFO    Notifying [e-mail] about bug 2.
-    ...
     INFO    Notifying [e-mail] about bug 2.
     ...
     INFO    Notifying [e-mail] about bug 2.
@@ -727,15 +725,12 @@
     >>> for message in trigger_and_get_email_messages(bug_three):
     ...     message.get_all('X-Launchpad-Bug-Tags')
     [u'layout-test']
-    [u'layout-test']
 
 If we add a tag to bug three that will also be included in the header.
 The tags will be space-separated to allow the list to be wrapped if it
 gets over-long.
 
     >>> switch_db_to_launchpad()
-
-    >>> bug_three.unsubscribe(sample_person, sample_person)
     >>> bug_three.tags = [u'layout-test', u'another-tag', u'yet-another']
 
     >>> switch_db_to_bugnotification()
@@ -756,6 +751,9 @@
     >>> 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)
+
 
 The X-Launchpad-Bug-Private header
 ----------------------------------
@@ -769,7 +767,7 @@
 
     >>> for message in trigger_and_get_email_messages(bug_three):
     ...     print message['To'], message.get_all('X-Launchpad-Bug-Private')
-    [e-mail] ['no']
+    [e-mail] ['no']
 
 Predictably, private bugs are sent with a slightly different header:
 
@@ -780,7 +778,7 @@
 
     >>> for message in trigger_and_get_email_messages(bug_three):
     ...     print message['To'], message.get_all('X-Launchpad-Bug-Private')
-    [e-mail] ['yes']
+    [e-mail] ['yes']
 
 
 The X-Launchpad-Bug-Security-Vulnerability header
@@ -796,7 +794,7 @@
     >>> for message in trigger_and_get_email_messages(bug_three):
     ...     print message['To'], (
     ...         message.get_all('X-Launchpad-Bug-Security-Vulnerability'))
-    [e-mail] ['no']
+    [e-mail] ['no']
 
 The presence of the security flag on a bug is, surprise, denoted by a
 simple "yes":
@@ -809,7 +807,7 @@
     >>> for message in trigger_and_get_email_messages(bug_three):
     ...     print message['To'], (
     ...         message.get_all('X-Launchpad-Bug-Security-Vulnerability'))
-    [e-mail] ['yes']
+    [e-mail] ['yes']
 
 
 The X-Launchpad-Bug-Commenters header
@@ -1238,7 +1236,7 @@
     <BLANKLINE>
     ** Summary changed to: whatever.
     <BLANKLINE>
-    -- 
+    --
     You received this bug notification because you are subscribed to Mozilla
     Firefox.
     ...
@@ -1287,7 +1285,7 @@
     <BLANKLINE>
     ** Summary changed to: something.
     <BLANKLINE>
-    -- 
+    --
     You received this bug notification because you are a member of ShipIt
     Administrators, which is a direct subscriber.
     http://bugs.launchpad.dev/bugs/1

=== modified file 'lib/lp/bugs/doc/bugzilla-import.txt'
--- lib/lp/bugs/doc/bugzilla-import.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/bugs/doc/bugzilla-import.txt	2010-12-23 19:45:24 +0000
@@ -363,7 +363,6 @@
   Reporter: Sample Person
   Created: 2005-04-04 00:00:00+00:00
   Subscribers:
-      Mark Shuttleworth
       Sample Person
       Ubuntu Team
   Task: Evolution
@@ -540,5 +539,3 @@
   True
   >>> bug4.duplicateof == bug3
   True
-
-

=== modified file 'lib/lp/bugs/doc/initial-bug-contacts.txt'
--- lib/lp/bugs/doc/initial-bug-contacts.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/bugs/doc/initial-bug-contacts.txt	2010-12-23 19:45:24 +0000
@@ -348,7 +348,6 @@
 
     >>> print '\n'.join(subscriber_names(bug_two_in_ubuntu.bug))
     Foo Bar
-    Mark Shuttleworth
     Sample Person
     Steve Alexander
     Ubuntu Team

=== 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-23 19:45:24 +0000
@@ -988,10 +988,10 @@
             # 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:
-                also_notified_subscribers.add(pillar.owner)
-                if recipients is not None:
-                    recipients.addRegistrant(pillar.owner, pillar)
+            if pillar.bug_supervisor is None and pillar.official_malone:
+                    also_notified_subscribers.add(pillar.owner)
+                    if recipients is not None:
+                        recipients.addRegistrant(pillar.owner, pillar)
 
         # Structural subscribers.
         also_notified_subscribers.update(
@@ -1790,8 +1790,12 @@
 
     @cachedproperty
     def _known_viewers(self):
-        """A dict of of known persons able to view this bug."""
-        return set()
+        """A set of known persons able to view this bug.
+
+        Seed it by including the list of all owners of bugtasks for the bug.
+        """
+        bugtask_owners = [bt.pillar.owner.id for bt in self.bugtasks]
+        return set(bugtask_owners)
 
     def userCanView(self, user):
         """See `IBug`.

=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt'
--- lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt	2010-10-19 14:55:46 +0000
+++ lib/lp/bugs/stories/bugs/xx-bug-personal-subscriptions.txt	2010-12-23 19:45:24 +0000
@@ -192,7 +192,6 @@
     ...     "http://launchpad.dev/bugs/3/+bug-portlet-subscribers-content")
     >>> print_also_notified(stevea_browser.contents)
     Also notified:
-    Mark Shuttleworth
 
 But if bug #2, a bug that Steve is directly subscribed to, is marked as
 a dupe of bug #3, then Steve gets indirectly subscribed to bug #3, and
@@ -243,7 +242,6 @@
   ...     "http://launchpad.dev/bugs/3/+bug-portlet-subscribers-content")
   >>> print_also_notified(stevea_browser.contents)
   Also notified:
-  Mark Shuttleworth
 
 Let's repeat this example, with Steve subscribed to two different dupes,
 to see how the unsubscribe notification changes slightly, because he
@@ -274,7 +272,6 @@
   ...     "http://launchpad.dev/bugs/3/+bug-portlet-subscribers-content")
   >>> print_also_notified(stevea_browser.contents)
   Also notified:
-  Mark Shuttleworth
 
   >>> stevea_browser.open("http://launchpad.dev/bugs/3")
   >>> stevea_browser.getLink("Unsubscribe").click()
@@ -317,7 +314,6 @@
 
   >>> print_also_notified(foobar_browser.contents)
   Also notified:
-  Mark Shuttleworth
 
 The subscribe link for Foo Bar still says "Subscribe", because
 Foo Bar can subscribe himself directly to this bug. For unsubscribing
@@ -350,7 +346,6 @@
   ...     "http://launchpad.dev/bugs/3/+bug-portlet-subscribers-content")
   >>> print_also_notified(foobar_browser.contents)
   Also notified:
-  Mark Shuttleworth
 
 
 == Displaying subscribers ==

=== 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-23 19:45:24 +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.bug_supervisor is None and pillar.official_malone:
         also_notified_subscribers.add(pillar.owner)
         if recipients is not None:
             recipients.addRegistrant(pillar.owner, pillar)

=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
--- lib/lp/bugs/tests/test_bugchanges.py	2010-10-04 19:50:45 +0000
+++ lib/lp/bugs/tests/test_bugchanges.py	2010-12-23 19:45:24 +0000
@@ -932,7 +932,8 @@
         new_target = self.factory.makeDistributionSourcePackage(
             distribution=target.distribution)
 
-        source_package_bug = self.factory.makeBug(owner=self.user)
+        source_package_bug = self.factory.makeBug(
+            owner=self.user)
         source_package_bug_task = source_package_bug.addTask(
             owner=self.user, target=target)
         self.saveOldChanges(source_package_bug)
@@ -956,8 +957,8 @@
         expected_recipients = [self.user, metadata_subscriber]
         expected_recipients.extend(
             bug_task.pillar.owner
-            for bug_task in source_package_bug.bugtasks)
-
+            for bug_task in source_package_bug.bugtasks
+            if bug_task.pillar.official_malone)
         expected_notification = {
             'text': u"** Package changed: %s => %s" % (
                 bug_task_before_modification.bugtargetname,

=== 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-23 19:45:24 +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('[e-mail]')
         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,104 @@
             recipient.person
             for recipient in latest_notification.recipients)
         self.assertEqual(self.dupe_subscribers, recipients)
+
+
+class NotificationForRegistrantsMixin:
+    """Mixin for testing when registrants get notified."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(NotificationForRegistrantsMixin, self).setUp(
+            user='[e-mail]')
+        self.pillar_owner = self.factory.makePerson(name="distro-owner")
+        self.bug_owner = self.factory.makePerson(name="bug-owner")
+        self.pillar = self.makePillar()
+        self.bug = self.makeBug()
+
+    def test_notification_uses_malone(self):
+        self.pillar.official_malone = True
+        direct = self.bug.getDirectSubscribers()
+        indirect = self.bug.getIndirectSubscribers()
+        self.assertThat(direct, Not(Contains(self.pillar_owner)))
+        self.assertThat(indirect, Contains(self.pillar_owner))
+
+    def test_notification_does_not_use_malone(self):
+        self.pillar.official_malone = False
+        direct = self.bug.getDirectSubscribers()
+        indirect = self.bug.getIndirectSubscribers()
+        self.assertThat(direct, Not(Contains(self.pillar_owner)))
+        self.assertThat(indirect, Not(Contains(self.pillar_owner)))
+
+    def test_status_change_uses_malone(self):
+        # Status changes are sent to the direct and indirect subscribers.
+        self.pillar.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.pillar_owner.name))
+
+    def test_status_change_does_not_use_malone(self):
+        # Status changes are sent to the direct and indirect subscribers.
+        self.pillar.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.pillar_owner.name)))
+
+
+class TestNotificationsForRegistrantsForDistros(
+    NotificationForRegistrantsMixin, TestCaseWithFactory):
+    """Test when distribution registrants get notified."""
+
+    def makePillar(self):
+        return self.factory.makeDistribution(
+            owner=self.pillar_owner)
+
+    def makeBug(self):
+        return self.factory.makeBug(
+            distribution=self.pillar,
+            owner=self.bug_owner)
+
+
+class TestNotificationsForRegistrantsForProducts(
+    NotificationForRegistrantsMixin, TestCaseWithFactory):
+    """Test when product registrants get notified."""
+
+    def makePillar(self):
+        return self.factory.makeProduct(
+            owner=self.pillar_owner)
+
+    def makeBug(self):
+        return self.factory.makeBug(
+            product=self.pillar,
+            owner=self.bug_owner)

=== modified file 'lib/lp/bugs/tests/test_bugs_webservice.py'
--- lib/lp/bugs/tests/test_bugs_webservice.py	2010-10-04 19:50:45 +0000
+++ lib/lp/bugs/tests/test_bugs_webservice.py	2010-12-23 19:45:24 +0000
@@ -166,18 +166,18 @@
         collector.register()
         self.addCleanup(collector.unregister)
         url = '/bugs/%d/attachments?ws.size=75' % self.bug.id
-        #First request
+        # First request.
         store.flush()
         store.reset()
         response = webservice.get(url)
-        self.assertThat(collector, HasQueryCount(LessThan(21)))
+        self.assertThat(collector, HasQueryCount(LessThan(22)))
         with_2_count = collector.count
         self.failUnlessEqual(response.status, 200)
         login(USER_EMAIL)
         for i in range(5):
             self.factory.makeBugAttachment(self.bug)
         logout()
-        #Second request
+        # Second request.
         store.flush()
         store.reset()
         response = webservice.get(url)
@@ -202,11 +202,11 @@
         collector.register()
         self.addCleanup(collector.unregister)
         url = '/bugs/%d/messages?ws.size=75' % bug.id
-        #First request
+        # First request.
         store.flush()
         store.reset()
         response = webservice.get(url)
-        self.assertThat(collector, HasQueryCount(LessThan(21)))
+        self.assertThat(collector, HasQueryCount(LessThan(22)))
         with_2_count = collector.count
         self.failUnlessEqual(response.status, 200)
         login(USER_EMAIL)
@@ -214,7 +214,7 @@
             self.factory.makeBugComment(bug)
         self.factory.makeBugAttachment(bug)
         logout()
-        #Second request
+        # Second request.
         store.flush()
         store.reset()
         response = webservice.get(url)

=== modified file 'lib/lp/bugs/tests/test_bugtask_search.py'
--- lib/lp/bugs/tests/test_bugtask_search.py	2010-12-14 02:18:32 +0000
+++ lib/lp/bugs/tests/test_bugtask_search.py	2010-12-23 19:45:24 +0000
@@ -91,7 +91,8 @@
         # search result.
         user = self.factory.makePerson()
         with person_logged_in(self.owner):
-            self.bugtasks[-1].bug.subscribe(user, self.owner)
+            bug = self.bugtasks[-1].bug
+            bug.subscribe(user, self.owner)
         params = self.getBugTaskSearchParams(user=user)
         self.assertSearchFinds(params, self.bugtasks)