launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/mute-when-not-allowed into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #827244 in Launchpad itself: "bug_filters related to teams are said "mutable" when they're not"
  https://bugs.launchpad.net/launchpad/+bug/827244

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/mute-when-not-allowed/+merge/125094

Currently if you call mute on a BugSubscriptionFilter for a team that has a contact address set over the API you get a generic error message, and an OOPS for your trouble.

I have decorated MuteNotAllowed to be a bad request, and raise an error with a hint if the team has a contact address.
-- 
https://code.launchpad.net/~stevenk/launchpad/mute-when-not-allowed/+merge/125094
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/mute-when-not-allowed into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bugsubscriptionfilter.py'
--- lib/lp/bugs/model/bugsubscriptionfilter.py	2012-08-02 04:46:31 +0000
+++ lib/lp/bugs/model/bugsubscriptionfilter.py	2012-09-19 04:47:10 +0000
@@ -12,7 +12,9 @@
     ]
 
 from itertools import chain
+import httplib
 
+from lazr.restful.declarations import error_status
 import pytz
 from storm.locals import (
     Bool,
@@ -44,6 +46,7 @@
 from lp.services.database.stormbase import StormBase
 
 
+@error_status(httplib.BAD_REQUEST)
 class MuteNotAllowed(Exception):
     """Raised when someone tries to mute a filter that can't be muted."""
 
@@ -252,6 +255,10 @@
 
     def mute(self, person):
         """See `IBugSubscriptionFilter`."""
+        subscriber = self.structural_subscription.subscriber
+        if subscriber.is_team and subscriber.preferredemail:
+            raise MuteNotAllowed(
+                "Can not mute, team has a contact address.")
         if not self.isMuteAllowed(person):
             raise MuteNotAllowed(
                 "This subscription cannot be muted for %s" % person.name)

=== modified file 'lib/lp/bugs/tests/test_structuralsubscription.py'
--- lib/lp/bugs/tests/test_structuralsubscription.py	2012-08-08 11:48:29 +0000
+++ lib/lp/bugs/tests/test_structuralsubscription.py	2012-09-19 04:47:10 +0000
@@ -925,12 +925,29 @@
             non_team_subscription = self.target.addBugSubscription(
                 person, person)
         filter = non_team_subscription.bug_filters.one()
+        expected = "This subscription cannot be muted for %s" % person.name
         self.assertFalse(filter.isMuteAllowed(person))
-        self.assertRaises(MuteNotAllowed, filter.mute, person)
+        self.assertRaisesWithContent(
+            MuteNotAllowed, expected, filter.mute, person)
 
     def test_mute_raises_error_for_non_team_members(self):
         # BugSubscriptionFilter.mute() will raise an error if called on
         # a subscription of which the calling person is not a member.
         non_team_person = self.factory.makePerson()
         self.assertFalse(self.filter.isMuteAllowed(non_team_person))
-        self.assertRaises(MuteNotAllowed, self.filter.mute, non_team_person)
+        expected = "This subscription cannot be muted for %s" % (
+            non_team_person.name,)
+        self.assertRaisesWithContent(
+            MuteNotAllowed, expected, self.filter.mute, non_team_person)
+        
+    def test_mute_on_team_with_contact_address(self):
+        # BugSubscriptionFilter.mute() will raise an error if called on
+        # a subscription if the team has a contact address.
+        person = self.factory.makePerson()
+        team = self.factory.makeTeam(email='foo@xxxxxxxxxxx', owner=person)
+        with person_logged_in(person):
+            ss = self.target.addBugSubscription(team, person)
+        filter = ss.bug_filters.one()
+        self.assertRaisesWithContent(
+            MuteNotAllowed, "Can not mute, team has a contact address.",
+            filter.mute, person)