launchpad-reviewers team mailing list archive

[Deryck Hodge <deryck.hodge@xxxxxxxxxxxxx>]

Deryck Hodge has proposed merging lp:~deryck/launchpad/displayname-private-team-oops-634847 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #634847 Altering bug report with subscribed private team : The following errors were encountered: (,'displayname','launchpad.View')
  https://bugs.launchpad.net/bugs/634847


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
-- 
https://code.launchpad.net/~deryck/launchpad/displayname-private-team-oops-634847/+merge/36199
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~deryck/launchpad/displayname-private-team-oops-634847 into lp:launchpad/devel.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2010-09-15 23:40:13 +0000
+++ lib/lp/bugs/model/bug.py	2010-09-21 19:22:43 +0000
@@ -70,6 +70,7 @@
     implements,
     providedBy,
     )
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
 from canonical.database.constants import UTC_NOW
@@ -804,8 +805,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`."""
@@ -946,9 +950,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-21 19:22:43 +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())