launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/unvalidated-email-woes into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/unvalidated-email-woes/+merge/109246

Summary
=======
We have a couple of call sites that don't properly validate email address when searching on them. This updates those call sites to confirm the addresses are VALIDATED or PREFERRED.

Preimp
======
Purple Squad

Implementation
==============
Update the queries to confirm that the EmailAddress.status is one of the two validated statuses. For convenience, a new tuple `VALID_EMAIL_STATUSES` was created for this check.

Tests
=====
bin/test -vvct personset -t person_vocabularies

QA
==
Create an unvalidated email address for a user. Ensure that you cannot search on this address in the person pickers.

Lint
====
This branch is lint free.
-- 
https://code.launchpad.net/~jcsackett/launchpad/unvalidated-email-woes/+merge/109246
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/unvalidated-email-woes into lp:launchpad.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-05-29 03:59:16 +0000
+++ lib/lp/registry/model/person.py	2012-06-07 22:08:19 +0000
@@ -271,6 +271,7 @@
     IEmailAddress,
     IEmailAddressSet,
     InvalidEmailAddress,
+    VALID_EMAIL_STATUSES,
     )
 from lp.services.identity.model.account import Account
 from lp.services.identity.model.emailaddress import (
@@ -3623,6 +3624,7 @@
             Join(EmailAddress, EmailAddress.personID == Person.id)
         ).find(
             (EmailAddress, Person),
+            EmailAddress.status.is_in(VALID_EMAIL_STATUSES),
             EmailAddress.email.lower().is_in(addresses), extra_query)
 
     def _merge_person_decoration(self, to_person, from_person, skip,

=== modified file 'lib/lp/registry/tests/test_person_vocabularies.py'
--- lib/lp/registry/tests/test_person_vocabularies.py	2012-05-31 08:44:50 +0000
+++ lib/lp/registry/tests/test_person_vocabularies.py	2012-06-07 22:08:19 +0000
@@ -21,6 +21,7 @@
     )
 from lp.registry.vocabularies import ValidPersonOrTeamVocabulary
 from lp.services.identity.interfaces.account import AccountStatus
+from lp.services.identity.interfaces.emailaddress import EmailAddressStatus
 from lp.services.webapp.vocabulary import FilteredVocabularyBase
 from lp.testing import (
     login_person,
@@ -370,6 +371,14 @@
         # The vocab shouldn't support person or team filters.
         self.assertEqual([], self.getVocabulary(None).supportedFilters())
 
+    def test_unvalidated_emails_ignored(self):
+        person = self.factory.makePerson()
+        unvalidated_email = self.factory.makeEmail(
+            'fnord@xxxxxxxxxxx',
+            person,
+            email_status=EmailAddressStatus.NEW)
+        search = self.searchVocabulary(None, 'fnord@xxxxxxxxxxx')
+        self.assertEqual([], [s for s in search])
 
 class TestNewPillarShareeVocabulary(VocabularyTestBase,
                                         TestCaseWithFactory):

=== modified file 'lib/lp/registry/tests/test_personset.py'
--- lib/lp/registry/tests/test_personset.py	2012-05-28 10:46:28 +0000
+++ lib/lp/registry/tests/test_personset.py	2012-06-07 22:08:19 +0000
@@ -115,6 +115,15 @@
             "PersonSet.getByEmail() should ignore case and whitespace.")
         self.assertEqual(person1, person2)
 
+    def test_getByEmail_ignores_unvalidated_emails(self):
+        person = self.factory.makePerson()
+        unvalidated_email = self.factory.makeEmail(
+            'fnord@xxxxxxxxxxx',
+            person,
+            email_status=EmailAddressStatus.NEW)
+        found = self.person_set.getByEmail('fnord@xxxxxxxxxxx')
+        self.assertTrue(found is None)
+        
     def test_getPrecachedPersonsFromIDs(self):
         # The getPrecachedPersonsFromIDs() method should only make one
         # query to load all the extraneous data. Accessing the

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2012-06-02 13:55:16 +0000
+++ lib/lp/registry/vocabularies.py	2012-06-07 22:08:19 +0000
@@ -181,7 +181,10 @@
     shortlist,
     )
 from lp.services.identity.interfaces.account import AccountStatus
-from lp.services.identity.interfaces.emailaddress import EmailAddressStatus
+from lp.services.identity.interfaces.emailaddress import (
+    EmailAddressStatus,
+    VALID_EMAIL_STATUSES,
+    )
 from lp.services.identity.model.account import Account
 from lp.services.identity.model.emailaddress import EmailAddress
 from lp.services.propertycache import (
@@ -234,6 +237,7 @@
             # lookup based on that.
             email = IStore(EmailAddress).find(
                 EmailAddress,
+                EmailAddress.status.is_in(VALID_EMAIL_STATUSES),
                 EmailAddress.email.lower() == token.strip().lower()).one()
             if email is None:
                 raise LookupError(token)
@@ -839,6 +843,7 @@
 
             email_storm_query = self.store.find(
                 EmailAddress.personID,
+                EmailAddress.status.is_in(VALID_EMAIL_STATUSES),
                 EmailAddress.email.lower().startswith(text))
             email_subquery = Alias(email_storm_query._get_select(),
                                    'EmailAddress')

=== modified file 'lib/lp/services/identity/interfaces/emailaddress.py'
--- lib/lp/services/identity/interfaces/emailaddress.py	2012-04-16 23:02:44 +0000
+++ lib/lp/services/identity/interfaces/emailaddress.py	2012-06-07 22:08:19 +0000
@@ -11,7 +11,8 @@
     'EmailAddressStatus',
     'IEmailAddress',
     'IEmailAddressSet',
-    'InvalidEmailAddress']
+    'InvalidEmailAddress',
+    'VALID_EMAIL_STATUSES']
 
 from lazr.enum import (
     DBEnumeratedType,
@@ -86,6 +87,10 @@
         receiving notifications from Launchpad.
         """)
 
+VALID_EMAIL_STATUSES = (
+    EmailAddressStatus.VALIDATED,
+    EmailAddressStatus.PREFERRED)
+
 
 class IEmailAddress(IHasOwner):
     """The object that stores the `IPerson`'s emails."""