launchpad-reviewers team mailing list archive

[Graham Binns <graham@xxxxxxxxxxxxx>]

Graham Binns has proposed merging lp:~gmb/launchpad/fix-oops-with-teams-bug-778847 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #778847 in Launchpad itself: "Muting a bug subscription for a team with a contact address crashes getRecipientFilterData"
  https://bugs.launchpad.net/launchpad/+bug/778847

For more details, see:
https://code.launchpad.net/~gmb/launchpad/fix-oops-with-teams-bug-778847/+merge/61600

This branch fixes bug 778847, whereby send-bug-notifications would OOPS if it came across the following situation:

 - Team A has a contact address
 - Team A has a structural subscription to Product B
 - Person C is a member of Team A
 - Person C has muted the emails from Team A's subscription to Product B
 - A Bug is filed (or changed) on Product B.

I've updated the code used to work out which mutes to apply so that it now:

 a) Doesn't try to remove a filter that doesn't exist (which raised the error) and
 b) Doesn't return Person C as a valid recipient in the situation above
    (since that would mean that they would still get emails about the bugs
    to which Team A is subscribed, which they don't want).

I've also added a regression test for the bug.
-- 
https://code.launchpad.net/~gmb/launchpad/fix-oops-with-teams-bug-778847/+merge/61600
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/fix-oops-with-teams-bug-778847 into lp:launchpad.

=== modified file 'lib/lp/bugs/interfaces/bugnotification.py'
--- lib/lp/bugs/interfaces/bugnotification.py	2011-04-05 22:34:35 +0000
+++ lib/lp/bugs/interfaces/bugnotification.py	2011-05-19 15:36:33 +0000
@@ -88,7 +88,7 @@
             (BugNotificationRecipients) that represent the subscriptions that
             caused the notifications to be sent.
         :param notifications: the notifications that are being communicated.
-    
+
         The dict of recipients may have fewer recipients than were
         provided if those users muted all of the subscription filters
         that caused them to be sent.

=== modified file 'lib/lp/bugs/model/bugnotification.py'
--- lib/lp/bugs/model/bugnotification.py	2011-05-12 21:33:10 +0000
+++ lib/lp/bugs/model/bugnotification.py	2011-05-19 15:36:33 +0000
@@ -262,7 +262,15 @@
                 In(BugSubscriptionFilterMute.person_id, recipient_id_map.keys()),
                 In(BugSubscriptionFilterMute.filter_id, filter_ids))
             for person_id, filter_id in mute_data:
-                del recipient_id_map[person_id]['filters'][filter_id]
+                if filter_id in recipient_id_map[person_id]['filters']:
+                    del recipient_id_map[person_id]['filters'][filter_id]
+                # This may look odd, but it's here to prevent members of
+                # a team with a contact address still getting direct
+                # email about a bug after they've muted the
+                # subscription.
+                if no_filter_marker in recipient_id_map[person_id]['filters']:
+                    del recipient_id_map[
+                        person_id]['filters'][no_filter_marker]
         # Now recipient_id_map has all the information we need.  Let's
         # build the final result and return it.
         result = {}

=== modified file 'lib/lp/bugs/tests/test_bugnotification.py'
--- lib/lp/bugs/tests/test_bugnotification.py	2011-05-12 21:33:10 +0000
+++ lib/lp/bugs/tests/test_bugnotification.py	2011-05-19 15:36:33 +0000
@@ -13,6 +13,7 @@
 from lazr.lifecycle.snapshot import Snapshot
 from storm.store import Store
 from testtools.matchers import Not
+from zope.component import getUtility
 from zope.event import notify
 from zope.interface import providedBy
 
@@ -36,7 +37,10 @@
     BugNotificationSet,
     )
 from lp.bugs.model.bugsubscriptionfilter import BugSubscriptionFilterMute
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    TestCaseWithFactory,
+    person_logged_in,
+    )
 from lp.testing.factory import LaunchpadObjectFactory
 from lp.testing.matchers import Contains
 
@@ -167,13 +171,15 @@
                           notification, person,
                           u'reason header', u'reason body'))
 
-    def addNotification(self, person):
+    def addNotification(self, person, bug=None):
         # Add a notification along with recipient data.
         # This is generally done with BugTaskSet.addNotification()
         # but that requires a more complex set-up.
+        if bug is None:
+            bug = self.bug
         message = self.factory.makeMessage()
         notification = BugNotification(
-            message=message, activity=None, bug=self.bug,
+            message=message, activity=None, bug=bug,
             is_comment=False, date_emailed=None)
         self.addNotificationRecipient(notification, person)
         return notification
@@ -191,7 +197,7 @@
             bug_filter = subscription.bug_filters.one()
         if description is not None:
             bug_filter.description = description
-        BugNotificationFilter(
+        return BugNotificationFilter(
             bug_notification=notification,
             bug_subscription_filter=bug_filter)
 
@@ -539,3 +545,44 @@
         return self.factory.makeBug(
             product=self.pillar,
             owner=self.bug_owner)
+
+
+class TestBug778847(TestCaseWithFactory):
+    """Regression tests for bug 778847."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_mutes_for_teams_with_contact_addresses(self):
+        # If a user holds a mute on a Team subscription,
+        # getRecipientFilterData() will handle the mute correctly.
+        team_owner = self.factory.makePerson(name="team-owner")
+        team = self.factory.makeTeam(
+            email="test@xxxxxxxxxxx", owner=team_owner)
+        product = self.factory.makeProduct()
+        with person_logged_in(team_owner):
+            subscription = product.addBugSubscription(
+                team, team_owner)
+            subscription_filter = subscription.bug_filters.one()
+            subscription_filter.mute(team_owner)
+        bug = self.factory.makeBug(product=product)
+        transaction.commit()
+        store = Store.of(bug)
+        store.execute("""
+            UPDATE Message SET
+                datecreated = now() at time zone 'utc' - interval '1 hour'
+            WHERE id IN (
+                SELECT message FROM BugNotification WHERE bug = %s);
+            """ % bug.id)
+        [notification] = BugNotificationSet().getNotificationsToSend()
+        # In this situation, only the team's subscription should be
+        # returned, since the Person has muted the subscription (whether
+        # or not they'll get email from the team contact address is not
+        # covered by this code).
+        self.assertEqual(
+            {team: {
+                'filter descriptions': [],
+                'sources': [notification.recipients[1]]}},
+            BugNotificationSet().getRecipientFilterData(
+            {team.teamowner: [notification.recipients[0]],
+             team: [notification.recipients[1]]},
+            [notification]))

=== modified file 'lib/lp/bugs/tests/test_structuralsubscription.py'
--- lib/lp/bugs/tests/test_structuralsubscription.py	2011-05-13 10:41:46 +0000
+++ lib/lp/bugs/tests/test_structuralsubscription.py	2011-05-19 15:36:33 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 
+from storm.expr import SQL
 from storm.store import (
     EmptyResultSet,
     ResultSet,
@@ -23,6 +24,7 @@
     BugTaskStatus,
     )
 from lp.bugs.mail.bugnotificationrecipients import BugNotificationRecipients
+from lp.bugs.model.bugnotification import BugNotification
 from lp.bugs.model.bugsubscriptionfilter import (
     BugSubscriptionFilter,
     BugSubscriptionFilterMute,
@@ -33,6 +35,7 @@
     get_structural_subscribers,
     get_structural_subscription_targets,
     )
+from lp.bugs.scripts.bugnotification import get_email_notifications
 from lp.testing import (
     anonymous_logged_in,
     login_person,