launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/person-index-mixed-visibility into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #884217 in Launchpad itself: "Fix Person+index MixedVisibilityErrors"
  https://bugs.launchpad.net/launchpad/+bug/884217

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/person-index-mixed-visibility/+merge/85427

This branch should fix a large portion of the MixedVisibilityErrors we are seeing from Person:+index. I have rewritten ITeam.super_teams to use Storm, and I have added a super_teams method to the ITeam view that checks permission on the teams. This is effectively what we already do, except the permission check is in the formatter for IPerson and if the user doesn't have permission, <hidden> is returned.

I have cleaned one line of lint, too.
-- 
https://code.launchpad.net/~stevenk/launchpad/person-index-mixed-visibility/+merge/85427
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/person-index-mixed-visibility into lp:launchpad.

=== modified file 'lib/lp/registry/browser/team.py'
--- lib/lp/registry/browser/team.py	2011-12-09 12:52:34 +0000
+++ lib/lp/registry/browser/team.py	2011-12-13 04:59:26 +0000
@@ -1718,6 +1718,13 @@
     """
 
     @property
+    def super_teams(self):
+        """Return only the super teams that the viewer is able to see."""
+        return [
+            team for team in self.context.super_teams
+            if check_permission('launchpad.View', team)]
+
+    @property
     def can_show_subteam_portlet(self):
         """Only show the subteam portlet if there is info to display.
 
@@ -1726,7 +1733,7 @@
         link so that the invitation can be accepted.
         """
         try:
-            return (self.context.super_teams.count() > 0
+            return (len(self.super_teams) > 0
                     or (self.context.open_membership_invitations
                         and check_permission('launchpad.Edit', self.context)))
         except AttributeError, e:

=== modified file 'lib/lp/registry/browser/tests/test_person_view.py'
--- lib/lp/registry/browser/tests/test_person_view.py	2011-12-12 04:47:21 +0000
+++ lib/lp/registry/browser/tests/test_person_view.py	2011-12-13 04:59:26 +0000
@@ -26,7 +26,10 @@
 from canonical.launchpad.interfaces.account import AccountStatus
 from canonical.launchpad.interfaces.authtoken import LoginTokenType
 from canonical.launchpad.interfaces.logintoken import ILoginTokenSet
-from canonical.launchpad.testing.pages import extract_text
+from canonical.launchpad.testing.pages import (
+    extract_text,
+    find_tag_by_id,
+    )
 from canonical.launchpad.webapp import canonical_url
 from canonical.launchpad.webapp.interfaces import ILaunchBag
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
@@ -48,6 +51,7 @@
 from lp.registry.interfaces.person import (
     IPersonSet,
     PersonVisibility,
+    TeamSubscriptionPolicy,
     )
 from lp.registry.interfaces.persontransferjob import IPersonMergeJobSource
 from lp.registry.interfaces.pocket import PackagePublishingPocket
@@ -117,12 +121,64 @@
             Equals(description))
 
 
+class TestPersonIndexVisibilityView(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def createTeams(self):
+        team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
+        private = self.factory.makeTeam(
+            visibility=PersonVisibility.PRIVATE, name='private-team',
+            members=[team])
+        with person_logged_in(team.teamowner):
+            team.acceptInvitationToBeMemberOf(private, '')
+        return team
+
+    def test_private_superteams_anonymous(self):
+        # If the viewer is anonymous, the portlet is not shown.
+        team = self.createTeams()
+        viewer = self.factory.makePerson()
+        view = create_initialized_view(
+            team, '+index', server_url=canonical_url(team), path_info='')
+        html = view()
+        superteams = find_tag_by_id(html, 'subteam-of')
+        self.assertIs(None, superteams)
+
+    def test_private_superteams_hidden(self):
+        # If the viewer has no permission to see any superteams, the portlet
+        # is not shown.
+        team = self.createTeams()
+        viewer = self.factory.makePerson()
+        with person_logged_in(viewer):
+            view = create_initialized_view(
+                team, '+index', server_url=canonical_url(team), path_info='',
+                principal=viewer)
+            html = view()
+        superteams = find_tag_by_id(html, 'subteam-of')
+        self.assertIs(None, superteams)
+
+    def test_private_superteams_shown(self):
+        # When the viewer has permission, the portlet is shown.
+        team = self.createTeams()
+        with person_logged_in(team.teamowner):
+            view = create_initialized_view(
+                team, '+index', server_url=canonical_url(team), path_info='',
+                principal=team.teamowner)
+            html = view()
+            superteams = find_tag_by_id(html, 'subteam-of')
+        self.assertFalse('&lt;hidden&gt;' in superteams)
+        self.assertEqual(
+            '<a href="/~private-team" class="sprite team">Private Team</a>',
+            str(superteams.findNext('a')))
+
+
 class TestPersonViewKarma(TestCaseWithFactory):
 
     layer = LaunchpadZopelessLayer
 
     def setUp(self):
-        TestCaseWithFactory.setUp(self)
+        super(TestPersonViewKarma, self).setUp()
         person = self.factory.makePerson()
         product = self.factory.makeProduct()
         transaction.commit()

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-12-09 12:52:34 +0000
+++ lib/lp/registry/model/person.py	2011-12-13 04:59:26 +0000
@@ -1475,12 +1475,15 @@
     @property
     def super_teams(self):
         """See `IPerson`."""
-        query = """
-            Person.id = TeamParticipation.team AND
-            TeamParticipation.person = %s AND
-            TeamParticipation.team != %s
-            """ % sqlvalues(self.id, self.id)
-        return Person.select(query, clauseTables=['TeamParticipation'])
+        return Store.of(self).using(
+            Join(
+                Person,
+                TeamParticipation,
+                Person.id == TeamParticipation.teamID
+            )).find(
+                Person,
+                TeamParticipation.personID == self.id,
+                TeamParticipation.teamID != self.id)
 
     @property
     def sub_teams(self):

=== modified file 'lib/lp/registry/templates/person-macros.pt'
--- lib/lp/registry/templates/person-macros.pt	2011-11-26 04:03:29 +0000
+++ lib/lp/registry/templates/person-macros.pt	2011-12-13 04:59:26 +0000
@@ -92,18 +92,18 @@
        >
     <h2>Subteam of</h2>
 
-    <p tal:condition="not: context/super_teams">
+    <p tal:condition="not: view/super_teams">
       &#8220;<span tal:replace="context/displayname"/>&#8221;
       itself is not a member of any other team.
     </p>
 
-    <tal:is-a-subteam condition="context/super_teams">
+    <tal:is-a-subteam condition="view/super_teams">
       <p>
         &#8220;<span tal:replace="context/displayname"/>&#8221;
         is a member of these teams:
       </p>
       <ul class="iconed">
-        <li tal:repeat="team context/super_teams">
+        <li tal:repeat="team view/super_teams">
           <tal:block replace="structure team/fmt:link" />
         </li>
       </ul>
@@ -111,7 +111,7 @@
 
     <ul class="horizontal">
       <li
-        tal:condition="context/super_teams"
+        tal:condition="view/super_teams"
         tal:replace="structure overview_menu/memberships/fmt:link" />
       <li
         tal:condition="context/open_membership_invitations"