launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-1056506 into lp:launchpad.

Commit message:
In AllUserTeamsParticipationVocabulary, join TeamParticipation separately when checking membership so we don't conflict with the private team access check join. Fixes OOPSes when used by a commercial admin.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1056506 in Launchpad itself: "Bug assignee person picker is OOPSing"
  https://bugs.launchpad.net/launchpad/+bug/1056506

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1056506/+merge/126358

AllUserTeamsParticipationVocabulary adds extra constraints on TeamParticipation to restrict results to those teams in which the user participates. But it doesn't join TeamParticipation itself; it relies on the join from the private team access check, which is confusing and breaks if the private team access check isn't there (eg. for commercial admins). I fixed it to constrain against a separately joined TeamParticipation alias, making things clearer and fixing the OOPS.

I added a new test and cleaned up existing ones to achieve LoC neutrality.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-1056506/+merge/126358
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1056506 into lp:launchpad.

=== modified file 'lib/lp/registry/tests/test_user_vocabularies.py'
--- lib/lp/registry/tests/test_user_vocabularies.py	2012-08-20 04:49:45 +0000
+++ lib/lp/registry/tests/test_user_vocabularies.py	2012-09-26 00:57:23 +0000
@@ -9,13 +9,12 @@
 from zope.schema.vocabulary import getVocabularyRegistry
 
 from lp.registry.enums import TeamMembershipPolicy
-from lp.registry.interfaces.person import PersonVisibility
+from lp.registry.interfaces.person import (
+    IPersonSet,
+    PersonVisibility,
+    )
 from lp.registry.model.person import Person
-from lp.services.webapp.interfaces import (
-    DEFAULT_FLAVOR,
-    IStoreSelector,
-    MAIN_STORE,
-    )
+from lp.services.database.lpstorm import IStore
 from lp.testing import (
     ANONYMOUS,
     login,
@@ -140,18 +139,11 @@
     def _vocabTermValues(self):
         """Return the token values for the vocab."""
         # XXX Abel Deuring 2010-05-21, bug 583502: We cannot simply iterate
-        # over the items of AllUserTeamsPariticipationVocabulary as in
-        # class TestUserTeamsParticipationPlusSelfVocabulary.
-        # So we iterate over all Person records and return all terms
-        # returned by vocabulary.searchForTerms(person.name)
+        # over the items of AllUserTeamsPariticipationVocabulary, so
+        # so iterate over all Persons and check membership.
         vocabulary_registry = getVocabularyRegistry()
         vocab = vocabulary_registry.get(None, 'AllUserTeamsParticipation')
-        store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
-        result = []
-        for person in store.find(Person):
-            result.extend(
-                term.value for term in vocab.searchForTerms(person.name))
-        return result
+        return [p for p in IStore(Person).find(Person) if p in vocab]
 
     def test_user_no_team(self):
         user = self.factory.makePerson()
@@ -167,22 +159,15 @@
     def test_user_in_two_teams(self):
         user = self.factory.makePerson()
         login_person(user)
-        team1 = self.factory.makeTeam()
-        user.join(team1)
-        team2 = self.factory.makeTeam()
-        user.join(team2)
-        self.assertEqual(set([team1, team2]), set(self._vocabTermValues()))
+        team1 = self.factory.makeTeam(members=[user])
+        team2 = self.factory.makeTeam(members=[user])
+        self.assertContentEqual([team1, team2], set(self._vocabTermValues()))
 
     def test_user_in_private_teams(self):
         # Private teams are included in the vocabulary.
         user = self.factory.makePerson()
-        team_owner = self.factory.makePerson()
-        login_person(team_owner)
-        team = self.factory.makeTeam(owner=team_owner)
-        team.addMember(person=user, reviewer=team_owner)
-        # Launchpad admin rights are needed to create private teams.
-        login('foo.bar@xxxxxxxxxxxxx')
-        team.visibility = PersonVisibility.PRIVATE
+        team = self.factory.makeTeam(
+            members=[user], visibility=PersonVisibility.PRIVATE)
         login_person(user)
         self.assertEqual([team], self._vocabTermValues())
 
@@ -190,3 +175,11 @@
         # AllUserTeamsPariticipationVocabulary is empty for anoymous users.
         login(ANONYMOUS)
         self.assertEqual([], self._vocabTermValues())
+
+    def test_commercial_admin(self):
+        # The vocab does the membership check for commercial admins too.
+        user = self.factory.makeCommercialAdmin()
+        com_admins = getUtility(IPersonSet).getByName('commercial-admins')
+        team1 = self.factory.makeTeam(members=[user])
+        login_person(user)
+        self.assertContentEqual([com_admins, team1], self._vocabTermValues())

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2012-09-20 13:59:25 +0000
+++ lib/lp/registry/vocabularies.py	2012-09-26 00:57:23 +0000
@@ -531,6 +531,7 @@
     # This is what subclasses must change if they want any extra filtering of
     # results.
     extra_clause = True
+    extra_tables = ()
 
     # Subclasses should override this property to allow null searches to
     # return all results.  If false, an empty result set is returned.
@@ -609,6 +610,7 @@
                      SQL("%s.id = Person.id" % self.cache_table_name)),
                 ]
             tables.extend(private_tables)
+            tables.extend(self.extra_tables)
             result = self.store.using(*tables).find(
                 Person,
                 And(
@@ -716,6 +718,7 @@
                 LeftJoin(EmailAddress, EmailAddress.person == Person.id),
                 LeftJoin(Account, Account.id == Person.accountID),
                 ]
+            public_tables.extend(self.extra_tables)
 
             # If private_tables is empty, we are searching for all private
             # teams. We can simply append the private query component to the
@@ -834,7 +837,7 @@
             Person.merged == None
             )
 
-        tables = [Person] + private_tables
+        tables = [Person] + private_tables + list(self.extra_tables)
 
         if not text:
             query = And(base_query,
@@ -1016,10 +1019,14 @@
         if user is None:
             self.extra_clause = False
         else:
-            self.extra_clause = AND(
-                super(AllUserTeamsParticipationVocabulary, self).extra_clause,
-                TeamParticipation.person == user.id,
-                TeamParticipation.team == Person.id)
+            # TeamParticipation might already be used for private team
+            # access checks, so alias and join it separately here.
+            tp_alias = ClassAlias(TeamParticipation)
+            self.extra_tables = [
+                Join(
+                    tp_alias,
+                    And(tp_alias.teamID == Person.id,
+                        tp_alias.personID == user.id))]
 
 
 class PersonActiveMembershipVocabulary: