launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/403-private-team-in-page into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/403-private-team-in-page/+merge/87704

Do not assume the team is public when listing teams.

    Launchpad bug: https://bugs.launchpad.net/bugs/911794
    Pre-implementation: no one

Users get a 403 when accessing /projects when the page tries to list
a private team. The error happens when the naive TALES access the
team's datecreated attribute.

--------------------------------------------------------------------

RULES

    * All the team-centric methods in PersonSet use _teamPrivacyQuery()
      to select only teams the logged in user can see, except for
      latest_teams.
      * Add tests for the current behaviour of the method.
      * Update the method to storm...oh, the method should exclude
        merged teams too.
      * Add a test for the private team to reproduce the Forbidden error,
        then update the query to use _teamPrivacyQuery()

    * ADDENDUM: ProductSet tests are in test_person and test_personset.
      test_person has the testcase I want to extend, but after the
      review, move the personset tests to the correct test module.

QA

    * Create a private team on qastaging
      https://qastaging.launchpad.net/people/+newteam
    * Visit https://qastaging.launchpad.net/projects
    * Verify you can see the private team.
    * Logout.
    * Visit https://qastaging.launchpad.net/projects
    * Verify the pages loads, but the private team is not listed.


LINT

    lib/lp/registry/model/person.py
    lib/lp/registry/tests/test_person.py



TEST

    ./bin/test -vvc -t test_latest_teams lp.registry.tests.test_person



IMPLEMENTATION

Added tests and rewrote the query. I considered moving the team sanity
checks (TeamParticipation.team == Person.id, Person.teamowner != None,
Person.merged == None) into _teamPrivacyQuery() because all the callsites
use them, but that made _teamPrivacyQuery() awkward to use.
    lib/lp/registry/model/person.py
    lib/lp/registry/tests/test_person.py
-- 
https://code.launchpad.net/~sinzui/launchpad/403-private-team-in-page/+merge/87704
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/403-private-team-in-page into lp:launchpad.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-12-30 06:47:17 +0000
+++ lib/lp/registry/model/person.py	2012-01-06 01:27:26 +0000
@@ -3587,8 +3587,15 @@
 
     def latest_teams(self, limit=5):
         """See `IPersonSet`."""
-        return Person.select("Person.teamowner IS NOT NULL",
-            orderBy=['-datecreated'], limit=limit)
+        orderby = (Desc(Person.datecreated), Desc(Person.id))
+        result = IStore(Person).find(
+            Person,
+            And(
+                self._teamPrivacyQuery(),
+                TeamParticipation.team == Person.id,
+                Person.teamowner != None,
+                Person.merged == None))
+        return result.order_by(orderby).config(distinct=True)[:limit]
 
     def _merge_person_decoration(self, to_person, from_person, skip,
         decorator_table, person_pointer_column, additional_person_columns):

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2012-01-05 03:11:37 +0000
+++ lib/lp/registry/tests/test_person.py	2012-01-06 01:27:26 +0000
@@ -799,6 +799,43 @@
                 person.preferredemail
         self.assertThat(recorder, HasQueryCount(LessThan(1)))
 
+    def test_latest_teams_public(self):
+        # Anyone can see the latest 5 teams if they are public.
+        teams = []
+        for num in xrange(1, 7):
+            teams.append(self.factory.makeTeam(name='team-%s' % num))
+        teams.reverse()
+        result = self.person_set.latest_teams()
+        self.assertEqual(teams[0:5], list(result))
+
+    def test_latest_teams_private(self):
+        # Private teams are only included in the latest teams if the
+        # user can view the team.
+        teams = []
+        for num in xrange(1, 7):
+            teams.append(self.factory.makeTeam(name='team-%s' % num))
+        owner = self.factory.makePerson()
+        teams.append(
+            self.factory.makeTeam(
+                name='private-team', owner=owner,
+                visibility=PersonVisibility.PRIVATE))
+        teams.reverse()
+        login_person(owner)
+        result = self.person_set.latest_teams()
+        self.assertEqual(teams[0:5], list(result))
+        login_person(self.factory.makePerson())
+        result = self.person_set.latest_teams()
+        self.assertEqual(teams[1:6], list(result))
+
+    def test_latest_teams_limit(self):
+        # The limit controls the number of latest teams returned.
+        teams = []
+        for num in xrange(1, 7):
+            teams.append(self.factory.makeTeam(name='team-%s' % num))
+        teams.reverse()
+        result = self.person_set.latest_teams(limit=3)
+        self.assertEqual(teams[0:3], list(result))
+
 
 class KarmaTestMixin:
     """Helper methods for setting karma."""