← Back to team overview

launchpad-reviewers team mailing list archive

[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