launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/gPPFID-email-argh into lp:launchpad.

Commit message:
Fix getPrecachedPersonsFromIDs to handle teams with mailing lists.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/gPPFID-email-argh/+merge/331517

Fix getPrecachedPersonsFromIDs to handle teams with mailing lists.

Previously, when need_preferred_email was true, a team with a mailing list but
no contact address (an email address, but only a non-preferred one) would be
filtered out by the WHERE, resulting in no precaching, and indeed not being
returned by the method at all.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/gPPFID-email-argh into lp:launchpad.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2017-06-09 00:03:35 +0000
+++ lib/lp/registry/model/person.py	2017-09-28 18:04:26 +0000
@@ -4012,12 +4012,12 @@
         # Checking validity requires having a preferred email.
         if not need_api and need_preferred_email and not need_validity:
             # Teams don't have email, so a left join
-            origin.append(
-                LeftJoin(EmailAddress, EmailAddress.person == Person.id))
+            origin.append(LeftJoin(
+                EmailAddress,
+                And(
+                    EmailAddress.person == Person.id,
+                    EmailAddress.status == EmailAddressStatus.PREFERRED)))
             columns.append(EmailAddress)
-            conditions = And(conditions,
-                Or(EmailAddress.status == None,
-                   EmailAddress.status == EmailAddressStatus.PREFERRED))
         if need_validity or need_api:
             valid_stuff = Person._validity_queries()
             origin.extend(valid_stuff["joins"])

=== modified file 'lib/lp/registry/tests/test_personset.py'
--- lib/lp/registry/tests/test_personset.py	2016-05-25 06:41:01 +0000
+++ lib/lp/registry/tests/test_personset.py	2017-09-28 18:04:26 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for PersonSet."""
@@ -7,6 +7,7 @@
 
 
 from testtools.matchers import (
+    Equals,
     GreaterThan,
     LessThan,
     )
@@ -154,6 +155,39 @@
                 person.teamowner
         self.assertThat(recorder, HasQueryCount(LessThan(1)))
 
+    def test_getPrecachedPersonsFromIDs_preferred_email(self):
+        # getPrecachedPersonsFromIDs() sets preferredemail to the preferred
+        # address if it exists, but otherwise leaves it as none.
+        team_no_contact = self.factory.makeTeam(email=None)
+        team_contact = self.factory.makeTeam(email=u'team@xxxxxxxxxxx')
+        team_list = self.factory.makeTeam(email=None)
+        self.factory.makeMailingList(team_list, team_list.teamowner)
+        person_normal = self.factory.makePerson()
+        person_unactivated = self.factory.makePerson(
+            account_status=AccountStatus.NOACCOUNT)
+        person_ids = [
+            t.id for t in [
+                team_no_contact, team_contact, team_list, person_normal,
+                person_unactivated]]
+        transaction.commit()
+
+        with StormStatementRecorder() as recorder:
+            list(self.person_set.getPrecachedPersonsFromIDs(
+                person_ids, need_preferred_email=True))
+        self.assertThat(recorder, HasQueryCount(Equals(1)))
+
+        with StormStatementRecorder() as recorder:
+            self.assertIsNone(team_no_contact.preferredemail)
+            self.assertEqual(
+                EmailAddressStatus.PREFERRED,
+                team_contact.preferredemail.status)
+            self.assertIsNone(team_list.preferredemail)
+            self.assertIsNone(person_unactivated.preferredemail)
+            self.assertEqual(
+                EmailAddressStatus.PREFERRED,
+                person_normal.preferredemail.status)
+        self.assertThat(recorder, HasQueryCount(Equals(0)))
+
     def test_getPrecachedPersonsFromIDs_is_ubuntu_coc_signer(self):
         # getPrecachedPersonsFromIDs() sets is_ubuntu_coc_signer
         # correctly.