launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #12055
[Merge] lp:~wallyworld/launchpad/person-merge-1019975 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/person-merge-1019975 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1019975 in Launchpad itself: "AttributeError: 'NoneType' object has no attribute 'displayname' requesting people merge"
https://bugs.launchpad.net/launchpad/+bug/1019975
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/person-merge-1019975/+merge/124589
== Implementation ==
The bug fix was trivial - call getByEmail() with filter_status=False in sendMergeRequestEmail().
== Tests ==
There was a doc test for person merging but nowhere I could see that would allow the new test for this change to be added easily. So I converted the doc test to some new unit test cases:
- TestRequestPeopleMergeSingleEmail
- TestRequestPeopleMergeMultipleEmails
- TestRequestPeopleMergeHiddenEmailAddresses
Add then just added the new test to the TestRequestPeopleMergeSingleEmail test case.
== Lint ==
Linting changed files:
lib/lp/registry/browser/tests/test_peoplemerge.py
lib/lp/services/verification/model/logintoken.py
--
https://code.launchpad.net/~wallyworld/launchpad/person-merge-1019975/+merge/124589
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/person-merge-1019975 into lp:launchpad.
=== modified file 'lib/lp/registry/browser/tests/test_peoplemerge.py'
--- lib/lp/registry/browser/tests/test_peoplemerge.py 2012-08-14 23:27:07 +0000
+++ lib/lp/registry/browser/tests/test_peoplemerge.py 2012-09-17 02:59:19 +0000
@@ -5,6 +5,7 @@
__metaclass__ = type
from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
from lp.registry.enums import (
InformationType,
@@ -12,6 +13,11 @@
)
from lp.registry.interfaces.person import IPersonSet
from lp.registry.interfaces.persontransferjob import IPersonMergeJobSource
+from lp.services.identity.interfaces.emailaddress import EmailAddressStatus
+from lp.services.identity.model.emailaddress import EmailAddressSet
+from lp.services.mail import stub
+from lp.services.verification.tests.logintoken import get_token_url_from_email
+from lp.services.webapp import canonical_url
from lp.testing import (
login_celebrity,
login_person,
@@ -19,12 +25,230 @@
TestCaseWithFactory,
)
from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.matchers import DocTestMatches
+from lp.testing.pages import find_tag_by_id, extract_text
from lp.testing.views import (
create_initialized_view,
create_view,
)
+class RequestPeopleMergeMixin(TestCaseWithFactory):
+
+ def setUp(self):
+ super(RequestPeopleMergeMixin, self).setUp()
+ self.person_set = getUtility(IPersonSet)
+ self.dupe = self.factory.makePerson(
+ name='foo', email='foo@xxxxxxx')
+
+ def tearDown(self):
+ super(RequestPeopleMergeMixin, self).tearDown()
+ stub.test_emails = []
+
+
+class TestRequestPeopleMergeMultipleEmails(RequestPeopleMergeMixin):
+ """ Tests for merging when dupe account has more than one email address."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestRequestPeopleMergeMultipleEmails, self).setUp()
+ EmailAddressSet().new(
+ 'bar.foo@xxxxxxxxxxxxx', person=self.dupe,
+ status=EmailAddressStatus.VALIDATED)
+
+ def _assert_perform_merge_request(self):
+ # Perform a merge request, asserting expected bahviour along the way.
+ # We are redirected to a page displaying the email addresses owned by
+ # the dupe account. The user chooses which one he wants to claim.
+ target = self.factory.makePerson()
+ login_person(target)
+ browser = self.getUserBrowser(
+ canonical_url(self.person_set) + '/+requestmerge', user=target)
+ browser.getControl(
+ 'Duplicated Account').value = 'foo'
+ browser.getControl('Continue').click()
+ explanation = find_tag_by_id(browser.contents, 'explanation')
+ self.assertThat(
+ extract_text(explanation), DocTestMatches(
+ "The account..."
+ "has more than one registered e-mail address..."))
+ email_select_control = browser.getControl(name='selected')
+ for ctrl in email_select_control.controls:
+ ctrl.selected = True
+ browser.getControl('Merge Accounts').click()
+ return browser
+
+ def test_merge_with_multiple_emails_request(self):
+ # Requesting a merge of an account with multiple email addresses
+ # informs the user confirmation emails are sent out.
+ browser = self._assert_perform_merge_request()
+ confirmation = find_tag_by_id(browser.contents, 'confirmation')
+ self.assertThat(
+ extract_text(confirmation), DocTestMatches(
+ "Confirmation email messages were sent to:..."))
+ self.assertIn('foo@xxxxxxx', browser.contents)
+ self.assertIn('bar.foo@xxxxxxxxxxxxx', browser.contents)
+
+ def test_validation_emails_sent(self):
+ # Test that the expected emails are sent out to the selected email
+ # addresses when a merge is requested.
+ self._assert_perform_merge_request()
+ self.assertEqual(2, len(stub.test_emails))
+ emails = [stub.test_emails.pop(), stub.test_emails.pop()]
+ emails.sort()
+ from_addr1, to_addrs1, raw_msg1 = emails.pop()
+ from_addr2, to_addrs2, raw_msg2 = emails.pop()
+ self.assertEqual('bounces@xxxxxxxxxxxxx', from_addr1)
+ self.assertEqual('bounces@xxxxxxxxxxxxx', from_addr2)
+ self.assertEqual(['foo@xxxxxxx'], to_addrs1)
+ self.assertEqual(['bar.foo@xxxxxxxxxxxxx'], to_addrs2)
+ self.assertIn('Launchpad: request to merge accounts', raw_msg1)
+ self.assertIn('Launchpad: request to merge accounts', raw_msg2)
+
+ def _assert_validation_email_confirm(self):
+ # Test that the user can go to the page we sent a link via email to
+ # validate the first claimed email address.
+ browser = self._assert_perform_merge_request()
+ emails = [stub.test_emails.pop(), stub.test_emails.pop()]
+ emails.sort()
+ ignore, ignore2, raw_msg1 = emails.pop()
+ token_url = get_token_url_from_email(raw_msg1)
+ browser.open(token_url)
+ self.assertIn(
+ 'trying to merge the Launchpad account', browser.contents)
+ browser.getControl('Confirm').click()
+ # User confirms the merge request submitting the form, but the merge
+ # wasn't finished because the duplicate account still have a registered
+ # email addresses.
+ self.assertIn(
+ 'has other registered e-mail addresses too', browser.contents)
+ return browser, emails
+
+ def test_validation_email_confirm(self):
+ # Test the validation of the first claimed email address.
+ self._assert_validation_email_confirm()
+
+ def test_validation_email_complete(self):
+ # Test that the merge completes successfully when the user proves that
+ # he's the owner of the second email address of the dupe account.
+ browser, emails = self._assert_validation_email_confirm()
+ ignore, ignore2, raw_msg2 = emails.pop()
+ token_url = get_token_url_from_email(raw_msg2)
+ browser.open(token_url)
+ self.assertIn(
+ 'trying to merge the Launchpad account', browser.contents)
+ browser.getControl('Confirm').click()
+ self.assertIn(
+ 'The accounts have been merged successfully', browser.contents)
+
+
+class TestRequestPeopleMergeSingleEmail(RequestPeopleMergeMixin):
+ """ Tests for merging when dupe account has single email address."""
+
+ layer = DatabaseFunctionalLayer
+
+ def _perform_merge_request(self, dupe):
+ # Perform a merge request.
+ target = self.factory.makePerson()
+ login_person(target)
+ dupe_name = dupe.name
+ browser = self.getUserBrowser(
+ canonical_url(self.person_set) + '/+requestmerge', user=target)
+ browser.getControl(
+ 'Duplicated Account').value = dupe_name
+ browser.getControl('Continue').click()
+ return browser
+
+ def test_merge_request_submit(self):
+ # Test that the expected emails are sent.
+ browser = self._perform_merge_request(self.dupe)
+ self.assertEqual(
+ canonical_url(self.person_set) +
+ '/+mergerequest-sent?dupe=%d' % self.dupe.id,
+ browser.url)
+ self.assertEqual(1, len(stub.test_emails))
+ self.assertIn('An email message was sent to', browser.contents)
+ self.assertIn('<strong>foo@xxxxxxx</strong', browser.contents)
+
+ def test_merge_request_revisit(self):
+ # Test that revisiting the same request gives the same results.
+ browser = self._perform_merge_request(self.dupe)
+ browser.open(
+ canonical_url(self.person_set) +
+ '/+mergerequest-sent?dupe=%d' % self.dupe.id)
+ self.assertEqual(1, len(stub.test_emails))
+ self.assertIn('An email message was sent to', browser.contents)
+ self.assertIn('<strong>foo@xxxxxxx</strong', browser.contents)
+
+ def test_merge_request_unvalidated_email(self):
+ # Test that the expected emails are sent even when the dupe account
+ # does not have a validated email address; the email is sent anyway.
+ dupe = self.factory.makePerson(
+ name='dupe', email='dupe@xxxxxxx',
+ email_address_status=EmailAddressStatus.NEW)
+ browser = self._perform_merge_request(dupe)
+ self.assertEqual(
+ canonical_url(self.person_set) +
+ '/+mergerequest-sent?dupe=%d' % dupe.id,
+ browser.url)
+ self.assertEqual(1, len(stub.test_emails))
+ self.assertIn('An email message was sent to', browser.contents)
+ self.assertIn('<strong>dupe@xxxxxxx</strong', browser.contents)
+
+
+class TestRequestPeopleMergeHiddenEmailAddresses(RequestPeopleMergeMixin):
+ """ Tests for merging when dupe account has hidden email addresses.
+
+ If the duplicate account has multiple email addresses and has chosen
+ to hide them the process is slightly different. We cannot display the
+ hidden addresses so instead we just inform the user to check all of
+ them (and hope they know which ones) and we send merge request
+ messages to them all.
+ """
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestRequestPeopleMergeHiddenEmailAddresses, self).setUp()
+ removeSecurityProxy(self.dupe).hide_email_addresses = True
+ EmailAddressSet().new(
+ 'bar.foo@xxxxxxxxxxxxx', person=self.dupe,
+ status=EmailAddressStatus.VALIDATED)
+
+ def _assert_perform_merge_request(self):
+ # The merge request process does not allow for selecting any email
+ # addresses since they are hidden.
+ target = self.factory.makePerson()
+ login_person(target)
+ browser = self.getUserBrowser(
+ canonical_url(self.person_set) + '/+requestmerge', user=target)
+ browser.getControl(
+ 'Duplicated Account').value = 'foo'
+ browser.getControl('Continue').click()
+ explanation = find_tag_by_id(browser.contents, 'explanation')
+ self.assertThat(
+ extract_text(explanation), DocTestMatches(
+ "The account...has 2 registered e-mail addresses..."))
+ self.assertRaises(LookupError, browser.getControl, 'selected')
+ self.assertNotIn('foo@xxxxxxx', browser.contents)
+ self.assertNotIn('bar.foo@xxxxxxxxxxxxx', browser.contents)
+ browser.getControl('Merge Accounts').click()
+ return browser
+
+ def test_merge_with_hidden_emails_submit(self):
+ # The merge request sends out emails but does not show the hidden email
+ # addresses.
+ browser = self._assert_perform_merge_request()
+ confirmation = find_tag_by_id(browser.contents, 'confirmation')
+ self.assertThat(
+ extract_text(confirmation), DocTestMatches(
+ "Confirmation email messages were sent to the 2 registered "
+ "e-mail addresses..."))
+ self.assertNotIn('foo@xxxxxxx', browser.contents)
+ self.assertNotIn('bar.foo@xxxxxxxxxxxxx', browser.contents)
+
+
class TestValidatingMergeView(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
=== removed file 'lib/lp/registry/stories/person/merge-people.txt'
--- lib/lp/registry/stories/person/merge-people.txt 2012-01-05 00:23:45 +0000
+++ lib/lp/registry/stories/person/merge-people.txt 1970-01-01 00:00:00 +0000
@@ -1,196 +0,0 @@
-Merging people
-==============
-
-Since we may create Launchpad profiles automatically sometimes, we allow
-people to merge two profiles into a single one, if they find there's more
-than one profile representing themselves.
-
-Here we will merge the profile named 'foo' into the one named 'no-priv'.
-
- # First we'll create the 'foo' profile because it doesn't exist yet.
- >>> login('foo.bar@xxxxxxxxxxxxx')
- >>> foo = factory.makePerson(name='foo', email='foo@xxxxxxx')
- >>> logout()
-
-First we have to go to the +requestmerge page.
-
- >>> browser.addHeader('Authorization', 'Basic no-priv@xxxxxxxxxxxxx:test')
- >>> browser.open('http://launchpad.dev/people/+requestmerge')
- >>> print browser.title
- Merge Launchpad accounts
-
-In preparation for the actual merge (below), we add an extra email
-address to the 'foo' account, to show how things work when the dupe
-account has more than one email address.
-
- >>> from lp.services.identity.model.emailaddress import EmailAddressSet
- >>> from lp.registry.model.person import PersonSet
- >>> from lp.services.identity.interfaces.emailaddress import (
- ... EmailAddressStatus)
- >>> foo = PersonSet().getByName('foo')
- >>> email = EmailAddressSet().new(
- ... 'bar.foo@xxxxxxxxxxxxx', person=foo,
- ... status=EmailAddressStatus.VALIDATED)
-
-Then we find the duplicate account and request the merge.
-This redirects to the page which displays all email addresses owned by the
-duplicate account. Here the user choses the ones which he want to claim.
-
- >>> browser.getControl(
- ... 'Duplicated Account').value = 'foo'
- >>> browser.getControl('Continue').click()
- >>> explanation = find_tag_by_id(browser.contents, 'explanation')
- >>> print extract_text(explanation)
- The account...has more than one registered e-mail address...
-
-
-Make sure we haven't got leftovers from a previous test
-
- >>> from lp.services.mail import stub
- >>> len(stub.test_emails)
- 0
-
-Claim all the email addresses
-
- >>> email_select_control = browser.getControl(name='selected')
- >>> for ctrl in email_select_control.controls:
- ... ctrl.selected = True
- >>> browser.getControl('Merge Accounts').click()
- >>> confirmation = find_tag_by_id(browser.contents, 'confirmation')
- >>> print extract_text(confirmation)
- Confirmation email messages were sent to:...
- >>> 'foo@xxxxxxx' in browser.contents
- True
- >>> 'bar.foo@xxxxxxxxxxxxx' in browser.contents
- True
-
-Get the token we'll have to use to finish the registration process.
-
- >>> len(stub.test_emails) == 2
- True
- >>> emails = []
- >>> emails.append(stub.test_emails.pop())
- >>> emails.append(stub.test_emails.pop())
- >>> emails.sort()
- >>> from_addr1, to_addrs1, raw_msg1 = emails.pop()
- >>> from_addr2, to_addrs2, raw_msg2 = emails.pop()
-
- >>> from lp.services.verification.tests.logintoken import (
- ... get_token_url_from_email)
- >>> token_url = get_token_url_from_email(raw_msg1)
-
-Now the user goes to the page we sent a link via email to validate the first
-claimed email address.
-
- >>> browser.open(token_url)
- >>> 'trying to merge the Launchpad account' in browser.contents
- True
-
-User confirms the merge request submitting the form, but the merge wasn't
-finished because the duplicate account still have a registered email addresses.
-
- >>> browser.getControl('Confirm').click()
- >>> 'has other registered e-mail addresses too' in browser.contents
- True
-
- >>> token_url = get_token_url_from_email(raw_msg2)
-
-Now the user proves that he's the owner of the second email address of the
-dupe account. And now the merge is completed successfully.
-
- >>> browser.open(token_url)
- >>> 'trying to merge the Launchpad account' in browser.contents
- True
- >>> browser.getControl('Confirm').click()
- >>> 'The accounts have been merged successfully' in browser.contents
- True
-
-
-If the account we were trying to merge had a single email address, the
-process would be a little simpler.
-
-To demonstrate that, now we'll merge marilize@xxxxxxx into
-no-priv@xxxxxxxxxxxxx.
-
- >>> len(stub.test_emails)
- 0
-
- >>> browser.open('http://launchpad.dev/people/+requestmerge')
- >>> browser.getControl('Duplicated Account').value = 'marilize@xxxxxxx'
- >>> browser.getControl('Continue').click()
- >>> browser.url
- 'http://launchpad.dev/people/+mergerequest-sent?dupe=55'
- >>> len(stub.test_emails)
- 1
- >>> 'An email message was sent to' in browser.contents
- True
- >>> '<strong>marilize@xxxxxxx</strong' in browser.contents
- True
-
-Revisiting that page gives the same results:
-
- >>> browser.open('http://launchpad.dev/people/+mergerequest-sent?dupe=55')
- >>> 'An email message was sent to' in browser.contents
- True
- >>> '<strong>marilize@xxxxxxx</strong' in browser.contents
- True
-
-Get the token we'll have to use to finish the registration process.
-
- >>> from_addr, to_addrs, raw_msg = stub.test_emails.pop()
- >>> assert not stub.test_emails
- >>> token_url = get_token_url_from_email(raw_msg)
-
-If the duplicate account has multiple email addresses and has chosen
-to hide them the process is slightly different. We cannot display the
-hidden addresses so instead we just inform the user to check all of
-them (and hope they know which ones) and we send merge request
-messages to them all.
-
- >>> login('foo.bar@xxxxxxxxxxxxx')
- >>> dupe = factory.makePerson(name="duplicate-person", email="dupe1@xxxxxxxxxxx",
- ... hide_email_addresses=True)
- >>> email = factory.makeEmail(address="dupe2@xxxxxxxxxxx", person=dupe)
-
-Ensure there are no leftover emails.
-
- >>> len(stub.test_emails)
- 0
-
-Open the merge page and merge our duplicate account with hidden email
-addresses.
-
- >>> logout()
- >>> browser.open('http://launchpad.dev/people/+requestmerge')
- >>> browser.getControl(
- ... 'Duplicated Account').value = 'duplicate-person'
- >>> browser.getControl('Continue').click()
-
- >>> explanation = find_tag_by_id(browser.contents, 'explanation')
- >>> print extract_text(explanation)
- The account...has 2 registered e-mail addresses...
-
-
-Since the addresses are hidden we cannot display them therefore there
-are no controls to select.
-
- >>> email_select_control = browser.getControl(name='selected')
- Traceback (most recent call last):
- ...
- LookupError: name 'selected'
-
- >>> browser.getControl('Merge Accounts').click()
-
- >>> len(stub.test_emails)
- 2
-
- >>> confirmation = find_tag_by_id(browser.contents, 'confirmation')
- >>> print extract_text(confirmation)
- Confirmation email messages were sent to the 2 registered e-mail addresses...
-
- >>> maincontent = extract_text(find_main_content(browser.contents))
-
- >>> 'dupe1@xxxxxxxxxxx' in maincontent
- False
- >>> 'dupe2@xxxxxxxxxxx' in maincontent
- False
=== modified file 'lib/lp/services/verification/model/logintoken.py'
--- lib/lp/services/verification/model/logintoken.py 2012-06-13 21:40:11 +0000
+++ lib/lp/services/verification/model/logintoken.py 2012-09-17 02:59:19 +0000
@@ -186,7 +186,8 @@
template = get_email_template('request-merge.txt', app=MAIL_APP)
from_name = "Launchpad Account Merge"
- dupe = getUtility(IPersonSet).getByEmail(self.email)
+ dupe = getUtility(IPersonSet).getByEmail(
+ self.email, filter_status=False)
replacements = {'dupename': "%s (%s)" % (dupe.displayname, dupe.name),
'requester': self.requester.name,
'requesteremail': self.requesteremail,
Follow ups
