launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/avoid-createPersonAndEmail into lp:launchpad.

Commit message:
Avoid direct Person.createPersonAndEmail calls.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/avoid-createPersonAndEmail/+merge/306101

Avoid direct Person.createPersonAndEmail calls. The one non-test public callsite should have been using ensurePerson instead, and lots of tests should have been using factory.makePerson.

-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/avoid-createPersonAndEmail into lp:launchpad.

=== modified file 'lib/lp/bugs/scripts/bugimport.py'
--- lib/lp/bugs/scripts/bugimport.py	2015-09-30 01:51:52 +0000
+++ lib/lp/bugs/scripts/bugimport.py	2016-09-19 13:46:01 +0000
@@ -172,34 +172,18 @@
         person_set = getUtility(IPersonSet)
 
         launchpad_id = self.person_id_cache.get(email)
-        if launchpad_id is not None:
-            person = person_set.get(launchpad_id)
-            if person is not None and person.merged is not None:
-                person = None
-        else:
-            person = None
-
-        if person is None:
-            person = getUtility(IPersonSet).getByEmail(
-                    email,
-                    filter_status=False)
-
-            if person is None:
-                self.logger.debug('creating person for %s' % email)
-                # Has the short name been taken?
-                if name is not None and (
-                    person_set.getByName(name) is not None):
-                    # The short name is already taken, so we'll pass
-                    # None to createPersonAndEmail(), which will take
-                    # care of creating a unique one.
-                    name = None
-                person, address = (
-                    person_set.createPersonAndEmail(
-                        email=email, name=name, displayname=displayname,
-                        rationale=PersonCreationRationale.BUGIMPORT,
-                        comment=('when importing bugs for %s' %
-                                 self.product.displayname)))
-
+        person = person_set.get(launchpad_id) if launchpad_id else None
+        if person is None or person.merged is not None:
+            if name is not None and person_set.getByName(name) is not None:
+                # The short name is already taken, so we'll pass None to
+                # ensurePerson(), which will take care of generating a
+                # unique one if a new Person is required.
+                name = None
+            person = getUtility(IPersonSet).ensurePerson(
+                email=email, name=name, displayname=displayname,
+                rationale=PersonCreationRationale.BUGIMPORT,
+                comment=(
+                    'when importing bugs for %s' % self.product.displayname))
             self.person_id_cache[email] = person.id
 
         # if we are auto-verifying new accounts, make sure the person

=== modified file 'lib/lp/bugs/scripts/tests/test_bugimport.py'
--- lib/lp/bugs/scripts/tests/test_bugimport.py	2015-07-08 16:05:11 +0000
+++ lib/lp/bugs/scripts/tests/test_bugimport.py	2016-09-19 13:46:01 +0000
@@ -41,6 +41,7 @@
 from lp.registry.interfaces.product import IProductSet
 from lp.services.config import config
 from lp.services.database.sqlbase import cursor
+from lp.services.identity.interfaces.account import AccountStatus
 from lp.services.identity.interfaces.emailaddress import IEmailAddressSet
 from lp.testing import (
     login,
@@ -209,13 +210,7 @@
 
     def test_find_existing_person(self):
         # Test that getPerson() returns an existing person.
-        person = getUtility(IPersonSet).getByEmail('foo@xxxxxxxxxxx')
-        self.assertEqual(person, None)
-        person, email = getUtility(IPersonSet).createPersonAndEmail(
-            email='foo@xxxxxxxxxxx',
-            rationale=PersonCreationRationale.OWNER_CREATED_LAUNCHPAD)
-        self.assertNotEqual(person, None)
-
+        person = self.factory.makePerson(email='foo@xxxxxxxxxxx')
         product = getUtility(IProductSet).getByName('netapplet')
         importer = bugimport.BugImporter(
             product, 'bugs.xml', 'bug-map.pickle')
@@ -257,11 +252,9 @@
     def test_verify_existing_person(self):
         # Test that getPerson() will validate the email of an existing
         # user when verify_users=True.
-        person, email = getUtility(IPersonSet).createPersonAndEmail(
-            rationale=PersonCreationRationale.OWNER_CREATED_LAUNCHPAD,
-            email='foo@xxxxxxxxxxx')
+        person = self.factory.makePerson(
+            email='foo@xxxxxxxxxxx', account_status=AccountStatus.NOACCOUNT)
         self.assertEqual(person.preferredemail, None)
-
         product = getUtility(IProductSet).getByName('netapplet')
         importer = bugimport.BugImporter(
             product, 'bugs.xml', 'bug-map.pickle', verify_users=True)
@@ -276,15 +269,9 @@
     def test_verify_doesnt_clobber_preferred_email(self):
         # Test that getPerson() does not clobber an existing verified
         # email address when verify_users=True.
-        person, email = getUtility(IPersonSet).createPersonAndEmail(
-            'foo@xxxxxxxxxxx',
-            PersonCreationRationale.OWNER_CREATED_LAUNCHPAD)
-        transaction.commit()
-        self.failIf(person.account is None, 'Person must have an account.')
-        email = getUtility(IEmailAddressSet).new(
-            'foo@xxxxxxxxxxxxx', person)
+        person = self.factory.makePerson(email='foo@xxxxxxxxxxx')
+        email = getUtility(IEmailAddressSet).new('foo@xxxxxxxxxxxxx', person)
         person.setPreferredEmail(email)
-        transaction.commit()
         self.assertEqual(person.preferredemail.email, 'foo@xxxxxxxxxxxxx')
 
         product = getUtility(IProductSet).getByName('netapplet')

=== modified file 'lib/lp/registry/doc/vocabularies.txt'
--- lib/lp/registry/doc/vocabularies.txt	2016-04-28 02:25:46 +0000
+++ lib/lp/registry/doc/vocabularies.txt	2016-09-19 13:46:01 +0000
@@ -564,19 +564,14 @@
 
 A person with a single and unvalidated email address can be merged.
 
-    >>> from lp.registry.interfaces.person import PersonCreationRationale
-    >>> fooperson, email = person_set.createPersonAndEmail(
-    ...     'foobaz@xxxxxxx', PersonCreationRationale.UNKNOWN,
-    ...     name='foobaz', displayname='foo baz')
-    >>> import transaction
-    >>> transaction.commit()
+    >>> from lp.services.identity.interfaces.account import AccountStatus
+    >>> fooperson = factory.makePerson(account_status=AccountStatus.NOACCOUNT)
     >>> fooperson in vocab
     True
 
 But any person without a single email address can't.
 
-    >>> email.destroySelf()
-    >>> transaction.commit()
+    >>> fooperson.guessedemails[0].destroySelf()
     >>> fooperson in vocab
     False
 
@@ -887,7 +882,6 @@
     ...     symbolic_person, 'chat.freenode.net', '%percent')
     >>> irc_id =ircid_set.new(
     ...     symbolic_person, 'irc.fnord.net', 'question?')
-    >>> transaction.commit()
 
     >>> for person in vocab.search('%percent'):
     ...     print person.name

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2016-07-12 13:32:10 +0000
+++ lib/lp/registry/model/person.py	2016-09-19 13:46:01 +0000
@@ -3613,15 +3613,14 @@
         return person
 
     def ensurePerson(self, email, displayname, rationale, comment=None,
-                     registrant=None):
+                     registrant=None, name=None):
         """See `IPersonSet`."""
-        person = getUtility(IPersonSet).getByEmail(
-                    email,
-                    filter_status=False)
+        person = getUtility(IPersonSet).getByEmail(email, filter_status=False)
         if person is None:
             person, email_address = self.createPersonAndEmail(
-                email, rationale, comment=comment, displayname=displayname,
-                registrant=registrant, hide_email_addresses=True)
+                email, rationale, name=name, comment=comment,
+                displayname=displayname, registrant=registrant,
+                hide_email_addresses=True)
         return person
 
     def getByName(self, name, ignore_merged=True):