launchpad-reviewers team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Brad Crittenden has proposed merging lp:~bac/launchpad/bug-680461 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #680461 team.participants lies!
  https://bugs.launchpad.net/bugs/680461

For more details, see:
https://code.launchpad.net/~bac/launchpad/bug-680461/+merge/46626

= Summary =

An error in the query was incorrectly causing people with improperly
marked archives from being selected and thus not presented in
team.participants.

== Proposed fix ==

Fix the query to do proper caching, not selection filtering.

== Pre-implementation notes ==

Chats with Curtis and William.

== Implementation details ==

As above.

== Tests ==

bin/test -vvm lp.registry -t TestPersonTeams

== Demo and Q/A ==

ubuntu-dev.participants should include 'jonathan'

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/testing/factory.py
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_person.py
-- 
https://code.launchpad.net/~bac/launchpad/bug-680461/+merge/46626
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-680461 into lp:launchpad.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-01-04 23:32:39 +0000
+++ lib/lp/registry/model/person.py	2011-01-18 16:21:52 +0000
@@ -1697,10 +1697,15 @@
             TeamParticipation.team == self.id,
             # But not the team itself.
             TeamParticipation.person != self.id)
-        return PersonSet()._getPrecachedPersons(
+        # Use a PersonSet object that is not security proxied to allow
+        # manipulation of the object.
+        person_set = PersonSet()
+        return person_set._getPrecachedPersons(
             origin, conditions, store=Store.of(self),
-            need_karma=need_karma, need_ubuntu_coc=need_ubuntu_coc,
-            need_location=need_location, need_archive=need_archive,
+            need_karma=need_karma,
+            need_ubuntu_coc=need_ubuntu_coc,
+            need_location=need_location,
+            need_archive=need_archive,
             need_preferred_email=need_preferred_email,
             need_validity=need_validity)
 
@@ -2614,8 +2619,7 @@
         store = Store.of(self)
         query = And(SignedCodeOfConduct.ownerID == self.id,
             Person._is_ubuntu_coc_signer_condition())
-        # TODO: Using exists would be faster than count().
-        return bool(store.find(SignedCodeOfConduct, query).count())
+        return not store.find(SignedCodeOfConduct, query).is_empty()
 
     @staticmethod
     def _is_ubuntu_coc_signer_condition():
@@ -4120,18 +4124,24 @@
                     PersonLocation.person == Person.id))
             columns.append(PersonLocation)
         if need_archive:
-            # Not everyone has PPA's
+            # Not everyone has PPAs.
             # It would be nice to cleanly expose the soyuz rules for this to
             # avoid duplicating the relationships.
+            archive_conditions = Or(
+                Archive.id == None,
+                And(
+                    Archive.owner == Person.id,
+                    Archive.id == Select(
+                        tables=Archive,
+                        columns=Min(Archive.id),
+                        where=And(
+                            Archive.purpose == ArchivePurpose.PPA,
+                            Archive.owner == Person.id))))
             origin.append(
-                LeftJoin(Archive, Archive.owner == Person.id))
+                LeftJoin(Archive, archive_conditions))
             columns.append(Archive)
-            conditions = And(conditions,
-                Or(Archive.id == None, And(
-                Archive.id == Select(Min(Archive.id),
-                    Archive.owner == Person.id, Archive),
-                Archive.purpose == ArchivePurpose.PPA)))
-        # checking validity requires having a preferred email.
+
+        # Checking validity requires having a preferred email.
         if need_preferred_email and not need_validity:
             # Teams don't have email, so a left join
             origin.append(
@@ -4139,16 +4149,16 @@
             columns.append(EmailAddress)
             conditions = And(conditions,
                 Or(EmailAddress.status == None,
-                    EmailAddress.status == EmailAddressStatus.PREFERRED))
+                   EmailAddress.status == EmailAddressStatus.PREFERRED))
         if need_validity:
             valid_stuff = Person._validity_queries()
             origin.extend(valid_stuff["joins"])
             columns.extend(valid_stuff["tables"])
             decorators.extend(valid_stuff["decorators"])
         if len(columns) == 1:
-            columns = columns[0]
+            column = columns[0]
             # Return a simple ResultSet
-            return store.using(*origin).find(columns, conditions)
+            return store.using(*origin).find(column, conditions)
         # Adapt the result into a cached Person.
         columns = tuple(columns)
         raw_result = store.using(*origin).find(columns, conditions)

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2010-12-10 15:06:12 +0000
+++ lib/lp/registry/tests/test_person.py	2011-01-18 16:21:52 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -62,6 +62,7 @@
 from lp.registry.model.person import Person
 from lp.registry.model.structuralsubscription import StructuralSubscription
 from lp.services.openid.model.openididentifier import OpenIdIdentifier
+from lp.soyuz.enums import ArchivePurpose
 from lp.testing import (
     celebrity_logged_in,
     login_person,
@@ -82,7 +83,7 @@
 
     def setUp(self):
         super(TestPersonTeams, self).setUp()
-        self.user = self.factory.makePerson()
+        self.user = self.factory.makePerson(name="test-member")
         self.a_team = self.factory.makeTeam(name='a')
         self.b_team = self.factory.makeTeam(name='b', owner=self.a_team)
         self.c_team = self.factory.makeTeam(name='c', owner=self.b_team)
@@ -231,6 +232,31 @@
         # team: scopes depend on this.
         self.assertFalse(self.user.inTeam('does-not-exist'))
 
+    def test_inTeam_person_incorrect_archive(self):
+        # If a person has an archive marked incorrectly that person should
+        # still be retrieved by 'all_members_prepopulated'.  See bug #680461.
+        self.factory.makeArchive(
+            owner=self.user, purpose=ArchivePurpose.PARTNER)
+        expected_members = sorted([self.user, self.a_team.teamowner])
+        retrieved_members = sorted(list(self.a_team.all_members_prepopulated))
+        self.assertEqual(expected_members, retrieved_members)
+
+    def test_inTeam_person_no_archive(self):
+        # If a person has no archive that person should still be retrieved by
+        # 'all_members_prepopulated'.
+        expected_members = sorted([self.user, self.a_team.teamowner])
+        retrieved_members = sorted(list(self.a_team.all_members_prepopulated))
+        self.assertEqual(expected_members, retrieved_members)
+
+    def test_inTeam_person_ppa_archive(self):
+        # If a person has a PPA that person should still be retrieved by
+        # 'all_members_prepopulated'.
+        self.factory.makeArchive(
+            owner=self.user, purpose=ArchivePurpose.PPA)
+        expected_members = sorted([self.user, self.a_team.teamowner])
+        retrieved_members = sorted(list(self.a_team.all_members_prepopulated))
+        self.assertEqual(expected_members, retrieved_members)
+
 
 class TestPerson(TestCaseWithFactory):
 

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-01-11 22:05:11 +0000
+++ lib/lp/testing/factory.py	2011-01-18 16:21:52 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=F0401
@@ -2149,7 +2149,7 @@
 
         :param distribution: Supply IDistribution, defaults to a new one
             made with makeDistribution() for non-PPAs and ubuntu for PPAs.
-        :param owner: Supper IPerson, defaults to a new one made with
+        :param owner: Supply IPerson, defaults to a new one made with
             makePerson().
         :param name: Name of the archive, defaults to a random string.
         :param purpose: Supply ArchivePurpose, defaults to PPA.