Launchpad itself

Merge lp:~wgrant/launchpad/avoid-createPersonAndEmail into lp:launchpad

  1. avoid-createPersonAndEmail
  2. Merge into devel
Proposed by William Grant
Status: Merged
Merged at revision: 18210
Proposed branch: lp:~wgrant/launchpad/avoid-createPersonAndEmail
Merge into: lp:launchpad
Diff against target: 169 lines (+26/-62)
4 files modified
lib/lp/bugs/scripts/bugimport.py (+12/-28)
lib/lp/bugs/scripts/tests/test_bugimport.py (+6/-19)
lib/lp/registry/doc/vocabularies.txt (+3/-9)
lib/lp/registry/model/person.py (+5/-6)
To merge this branch: bzr merge lp:~wgrant/launchpad/avoid-createPersonAndEmail
Reviewer Review Type Date Requested Status
Colin Watson (community) Approve
Review via email: mp+306101@code.launchpad.net

Commit message

Avoid direct Person.createPersonAndEmail calls.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/bugs/scripts/bugimport.py'
2--- lib/lp/bugs/scripts/bugimport.py 2015-09-30 01:51:52 +0000
3+++ lib/lp/bugs/scripts/bugimport.py 2016-09-19 13:46:01 +0000
4@@ -172,34 +172,18 @@
5 person_set = getUtility(IPersonSet)
6
7 launchpad_id = self.person_id_cache.get(email)
8- if launchpad_id is not None:
9- person = person_set.get(launchpad_id)
10- if person is not None and person.merged is not None:
11- person = None
12- else:
13- person = None
14-
15- if person is None:
16- person = getUtility(IPersonSet).getByEmail(
17- email,
18- filter_status=False)
19-
20- if person is None:
21- self.logger.debug('creating person for %s' % email)
22- # Has the short name been taken?
23- if name is not None and (
24- person_set.getByName(name) is not None):
25- # The short name is already taken, so we'll pass
26- # None to createPersonAndEmail(), which will take
27- # care of creating a unique one.
28- name = None
29- person, address = (
30- person_set.createPersonAndEmail(
31- email=email, name=name, displayname=displayname,
32- rationale=PersonCreationRationale.BUGIMPORT,
33- comment=('when importing bugs for %s' %
34- self.product.displayname)))
35-
36+ person = person_set.get(launchpad_id) if launchpad_id else None
37+ if person is None or person.merged is not None:
38+ if name is not None and person_set.getByName(name) is not None:
39+ # The short name is already taken, so we'll pass None to
40+ # ensurePerson(), which will take care of generating a
41+ # unique one if a new Person is required.
42+ name = None
43+ person = getUtility(IPersonSet).ensurePerson(
44+ email=email, name=name, displayname=displayname,
45+ rationale=PersonCreationRationale.BUGIMPORT,
46+ comment=(
47+ 'when importing bugs for %s' % self.product.displayname))
48 self.person_id_cache[email] = person.id
49
50 # if we are auto-verifying new accounts, make sure the person
51
52=== modified file 'lib/lp/bugs/scripts/tests/test_bugimport.py'
53--- lib/lp/bugs/scripts/tests/test_bugimport.py 2015-07-08 16:05:11 +0000
54+++ lib/lp/bugs/scripts/tests/test_bugimport.py 2016-09-19 13:46:01 +0000
55@@ -41,6 +41,7 @@
56 from lp.registry.interfaces.product import IProductSet
57 from lp.services.config import config
58 from lp.services.database.sqlbase import cursor
59+from lp.services.identity.interfaces.account import AccountStatus
60 from lp.services.identity.interfaces.emailaddress import IEmailAddressSet
61 from lp.testing import (
62 login,
63@@ -209,13 +210,7 @@
64
65 def test_find_existing_person(self):
66 # Test that getPerson() returns an existing person.
67- person = getUtility(IPersonSet).getByEmail('foo@example.com')
68- self.assertEqual(person, None)
69- person, email = getUtility(IPersonSet).createPersonAndEmail(
70- email='foo@example.com',
71- rationale=PersonCreationRationale.OWNER_CREATED_LAUNCHPAD)
72- self.assertNotEqual(person, None)
73-
74+ person = self.factory.makePerson(email='foo@example.com')
75 product = getUtility(IProductSet).getByName('netapplet')
76 importer = bugimport.BugImporter(
77 product, 'bugs.xml', 'bug-map.pickle')
78@@ -257,11 +252,9 @@
79 def test_verify_existing_person(self):
80 # Test that getPerson() will validate the email of an existing
81 # user when verify_users=True.
82- person, email = getUtility(IPersonSet).createPersonAndEmail(
83- rationale=PersonCreationRationale.OWNER_CREATED_LAUNCHPAD,
84- email='foo@example.com')
85+ person = self.factory.makePerson(
86+ email='foo@example.com', account_status=AccountStatus.NOACCOUNT)
87 self.assertEqual(person.preferredemail, None)
88-
89 product = getUtility(IProductSet).getByName('netapplet')
90 importer = bugimport.BugImporter(
91 product, 'bugs.xml', 'bug-map.pickle', verify_users=True)
92@@ -276,15 +269,9 @@
93 def test_verify_doesnt_clobber_preferred_email(self):
94 # Test that getPerson() does not clobber an existing verified
95 # email address when verify_users=True.
96- person, email = getUtility(IPersonSet).createPersonAndEmail(
97- 'foo@example.com',
98- PersonCreationRationale.OWNER_CREATED_LAUNCHPAD)
99- transaction.commit()
100- self.failIf(person.account is None, 'Person must have an account.')
101- email = getUtility(IEmailAddressSet).new(
102- 'foo@preferred.com', person)
103+ person = self.factory.makePerson(email='foo@example.com')
104+ email = getUtility(IEmailAddressSet).new('foo@preferred.com', person)
105 person.setPreferredEmail(email)
106- transaction.commit()
107 self.assertEqual(person.preferredemail.email, 'foo@preferred.com')
108
109 product = getUtility(IProductSet).getByName('netapplet')
110
111=== modified file 'lib/lp/registry/doc/vocabularies.txt'
112--- lib/lp/registry/doc/vocabularies.txt 2016-04-28 02:25:46 +0000
113+++ lib/lp/registry/doc/vocabularies.txt 2016-09-19 13:46:01 +0000
114@@ -564,19 +564,14 @@
115
116 A person with a single and unvalidated email address can be merged.
117
118- >>> from lp.registry.interfaces.person import PersonCreationRationale
119- >>> fooperson, email = person_set.createPersonAndEmail(
120- ... 'foobaz@bar.com', PersonCreationRationale.UNKNOWN,
121- ... name='foobaz', displayname='foo baz')
122- >>> import transaction
123- >>> transaction.commit()
124+ >>> from lp.services.identity.interfaces.account import AccountStatus
125+ >>> fooperson = factory.makePerson(account_status=AccountStatus.NOACCOUNT)
126 >>> fooperson in vocab
127 True
128
129 But any person without a single email address can't.
130
131- >>> email.destroySelf()
132- >>> transaction.commit()
133+ >>> fooperson.guessedemails[0].destroySelf()
134 >>> fooperson in vocab
135 False
136
137@@ -887,7 +882,6 @@
138 ... symbolic_person, 'chat.freenode.net', '%percent')
139 >>> irc_id =ircid_set.new(
140 ... symbolic_person, 'irc.fnord.net', 'question?')
141- >>> transaction.commit()
142
143 >>> for person in vocab.search('%percent'):
144 ... print person.name
145
146=== modified file 'lib/lp/registry/model/person.py'
147--- lib/lp/registry/model/person.py 2016-07-12 13:32:10 +0000
148+++ lib/lp/registry/model/person.py 2016-09-19 13:46:01 +0000
149@@ -3613,15 +3613,14 @@
150 return person
151
152 def ensurePerson(self, email, displayname, rationale, comment=None,
153- registrant=None):
154+ registrant=None, name=None):
155 """See `IPersonSet`."""
156- person = getUtility(IPersonSet).getByEmail(
157- email,
158- filter_status=False)
159+ person = getUtility(IPersonSet).getByEmail(email, filter_status=False)
160 if person is None:
161 person, email_address = self.createPersonAndEmail(
162- email, rationale, comment=comment, displayname=displayname,
163- registrant=registrant, hide_email_addresses=True)
164+ email, rationale, name=name, comment=comment,
165+ displayname=displayname, registrant=registrant,
166+ hide_email_addresses=True)
167 return person
168
169 def getByName(self, name, ignore_merged=True):