Merge lp:~deryck/launchpad/displayname-private-team-oops-634847 into lp:launchpad

[Deryck Hodge]

Fix an OOPS when trying to sort on displayname for bug subscribers.

This work fixes the OOPS reported in bug 634847, where a comment was
not being allowed to post to a bug. The error was because a team was
subscribed that was private, and getAlsoNotifiedSubscribers was
blowing up trying to access a forbidden attribute ("displayname") for
sorting.

This branch adds tests to ensure we can list subscribers when one is
private and fixes the couple spots in the code where this was a
problem. The fix I chose was to use lambda to get an object that was
not security proxied and then pass the displayname attribute as the
sort key. This allows the list that is returned to retain security
proxied objects.

Cheers,
deryck

[Abel Deuring (adeuring)]

review: Approve (code)

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2010-09-22 08:48:24 +0000
+++ lib/lp/bugs/model/bug.py	2010-09-23 12:32:52 +0000
@@ -842,8 +842,11 @@
             self.getAlsoNotifiedSubscribers(recipients, level) +
             self.getSubscribersFromDuplicates(recipients, level))
 
+        # Remove security proxy for the sort key, but return
+        # the regular proxied object.
         return sorted(
-            indirect_subscribers, key=operator.attrgetter("displayname"))
+            indirect_subscribers,
+            key=lambda x: removeSecurityProxy(x).displayname)
 
     def getSubscriptionsFromDuplicates(self, recipients=None):
         """See `IBug`."""
@@ -999,9 +1002,11 @@
         # Direct subscriptions always take precedence over indirect
         # subscriptions.
         direct_subscribers = set(self.getDirectSubscribers())
+        # Remove security proxy for the sort key, but return
+        # the regular proxied object.
         return sorted(
             (also_notified_subscribers - direct_subscribers),
-            key=operator.attrgetter('displayname'))
+            key=lambda x: removeSecurityProxy(x).displayname)
 
     def getBugNotificationRecipients(self, duplicateof=None, old_bug=None,
                                      level=None,
 
=== modified file 'lib/lp/bugs/tests/test_bug.py'
--- lib/lp/bugs/tests/test_bug.py	2010-08-31 00:55:52 +0000
+++ lib/lp/bugs/tests/test_bug.py	2010-09-23 12:32:52 +0000
@@ -6,6 +6,8 @@
 __metaclass__ = type
 
 from canonical.testing import DatabaseFunctionalLayer
+from lp.registry.interfaces.person import PersonVisibility
+from lp.registry.model.structuralsubscription import StructuralSubscription
 from lp.testing import (
     person_logged_in,
     TestCaseWithFactory,
@@ -65,3 +67,44 @@
             set([person, team1, team2]),
             set(real_bug.getSubscribersForPerson(person)))
 
+    def test_get_also_notified_subscribers_with_private_team(self):
+        product = self.factory.makeProduct()
+        bug = self.factory.makeBug(product=product)
+        member = self.factory.makePerson()
+        team = self.factory.makeTeam(
+            owner=member, visibility=PersonVisibility.PRIVATE)
+        StructuralSubscription(
+            product=product, subscriber=team, subscribed_by=member)
+        self.assertTrue(team in bug.getAlsoNotifiedSubscribers())
+
+    def test_get_indirect_subscribers_with_private_team(self):
+        product = self.factory.makeProduct()
+        bug = self.factory.makeBug(product=product)
+        member = self.factory.makePerson()
+        team = self.factory.makeTeam(
+            owner=member, visibility=PersonVisibility.PRIVATE)
+        StructuralSubscription(
+            product=product, subscriber=team, subscribed_by=member)
+        self.assertTrue(team in bug.getIndirectSubscribers())
+
+    def test_get_direct_subscribers_with_private_team(self):
+        product = self.factory.makeProduct()
+        bug = self.factory.makeBug(product=product)
+        member = self.factory.makePerson()
+        team = self.factory.makeTeam(
+            owner=member, visibility=PersonVisibility.PRIVATE)
+        with person_logged_in(member):
+            bug.subscribe(team, member)
+        self.assertTrue(team in bug.getDirectSubscribers())
+
+    def test_get_subscribers_from_duplicates_with_private_team(self):
+        product = self.factory.makeProduct()
+        bug = self.factory.makeBug(product=product)
+        dupe_bug = self.factory.makeBug()
+        member = self.factory.makePerson()
+        team = self.factory.makeTeam(
+            owner=member, visibility=PersonVisibility.PRIVATE)
+        with person_logged_in(member):
+            dupe_bug.subscribe(team, member)
+            dupe_bug.markAsDuplicate(bug)
+        self.assertTrue(team in bug.getSubscribersFromDuplicates())