launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

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

Commit message:
Fix PersonSet._getPrecachedPersons to correctly scope Person in the is_ubuntu_coc_signer query so it doesn't always return true.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1193157 in Launchpad itself: "people.is_ubuntu_coc_signer reports True when actually False"
  https://bugs.launchpad.net/launchpad/+bug/1193157

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

PersonSet._getPrecachedPersons() didn't specify an explicit set of tables for the is_ubuntu_coc_signer subquery, so autotables shadowed Person and it returned true as long as there was at least one signed CoC in the DB.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-1193157/+merge/170722
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1193157 into lp:launchpad.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2013-06-05 07:26:58 +0000
+++ lib/lp/registry/model/person.py	2013-06-21 00:54:28 +0000
@@ -3791,10 +3791,15 @@
                     KarmaTotalCache.person == Person.id))
             columns.append(KarmaTotalCache)
         if need_ubuntu_coc:
-            columns.append(Alias(Exists(Select(SignedCodeOfConduct,
-                And(Person._is_ubuntu_coc_signer_condition(),
-                    SignedCodeOfConduct.ownerID == Person.id))),
-                name='is_ubuntu_coc_signer'))
+            columns.append(
+                Alias(
+                    Exists(Select(
+                        SignedCodeOfConduct,
+                        tables=[SignedCodeOfConduct],
+                        where=And(
+                            Person._is_ubuntu_coc_signer_condition(),
+                            SignedCodeOfConduct.ownerID == Person.id))),
+                    name='is_ubuntu_coc_signer'))
         if need_location:
             # New people have no location rows
             origin.append(

=== modified file 'lib/lp/registry/tests/test_personset.py'
--- lib/lp/registry/tests/test_personset.py	2013-06-05 07:51:14 +0000
+++ lib/lp/registry/tests/test_personset.py	2013-06-21 00:54:28 +0000
@@ -22,12 +22,16 @@
     PersonCreationRationale,
     TeamEmailAddressError,
     )
+from lp.registry.model.codeofconduct import SignedCodeOfConduct
 from lp.registry.model.person import Person
 from lp.services.database.lpstorm import (
     IMasterStore,
     IStore,
     )
-from lp.services.database.sqlbase import cursor
+from lp.services.database.sqlbase import (
+    cursor,
+    flush_database_caches,
+    )
 from lp.services.identity.interfaces.account import (
     AccountCreationRationale,
     AccountStatus,
@@ -105,9 +109,7 @@
         # The getPrecachedPersonsFromIDs() method should only make one
         # query to load all the extraneous data. Accessing the
         # attributes should then cause zero queries.
-        person_ids = [
-            self.factory.makePerson().id
-            for i in range(3)]
+        person_ids = [self.factory.makePerson().id for i in range(3)]
 
         with StormStatementRecorder() as recorder:
             persons = list(self.person_set.getPrecachedPersonsFromIDs(
@@ -126,6 +128,20 @@
                 person.preferredemail
         self.assertThat(recorder, HasQueryCount(LessThan(1)))
 
+    def test_getPrecachedPersonsFromIDs_is_ubuntu_coc_signer(self):
+        # getPrecachedPersonsFromIDs() sets is_ubuntu_coc_signer
+        # correctly.
+        person_ids = [self.factory.makePerson().id for i in range(3)]
+        SignedCodeOfConduct(owner=person_ids[0], active=True)
+        flush_database_caches()
+
+        persons = list(
+            self.person_set.getPrecachedPersonsFromIDs(
+                person_ids, need_ubuntu_coc=True))
+        self.assertContentEqual(
+            zip(person_ids, [True, False, False]),
+            [(p.id, p.is_ubuntu_coc_signer) for p in persons])
+
     def test_getByOpenIDIdentifier_returns_person(self):
         # getByOpenIDIdentifier takes a full OpenID identifier and
         # returns the corresponding person.