launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/merge-non-active-person into lp:launchpad.

Commit message:
Allow users to merge non-active accounts.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1057947 in Launchpad itself: "LocationError: (None, 'name') when merging an account with a NEW email address"
  https://bugs.launchpad.net/launchpad/+bug/1057947

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/merge-non-active-person/+merge/127041

MergePeopleView.initialize() uses PersonSet.getByEmail() in a way that
won't find people by a NEW email address, causing merges of unactivated
accounts to fail like OOPS-e2f5d3b6769c012de049daf4a60c0941.

This is basically the same as bug #1019975, just at a later stage.
The fix was to pass filter_status=False to the method so that NEW
addresses can be used to look up the IPerson.

--------------------------------------------------------------------

RULES

    Pre-implementation: no one
    * Add a test for the current case with active accounts and validated
      email addresses.
    * Setup the same test with a NEW email address and non-active account.
      Expect the LocationError: (None, 'name') result.
    * Add filter_status=False to the view's call to PersonSet.getByEmail()
      and verify the test passes.


QA

    * Visit https://qastaging.launchpad.net/~ddw
    * Follow the "Are you Ddw" link and Continue to send an confirmation email.
    * Look in qastaging db for the token associated with your address:
      select * from logintoken where created > '2012-09-28';
    * Visit https://qastaging.launchpad.net/token/<TOKEN/+accountmerge
    * Verify the page loads.
    * Confirm
    * Verify the page loads and you see that the merge is queued.


LINT

    lib/lp/services/verification/browser/logintoken.py
    lib/lp/services/verification/browser/tests/test_logintoken.py


LoC

    I have a 3000 line credit at the moment.

TEST

    ./bin/test -vvc -t MergePeopleView lp.services.verification.browser.tests


IMPLEMENTATION

This is a rare case where the implementation plan was exactly as expected.
    lib/lp/services/verification/browser/logintoken.py
    lib/lp/services/verification/browser/tests/test_logintoken.py
-- 
https://code.launchpad.net/~sinzui/launchpad/merge-non-active-person/+merge/127041
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/merge-non-active-person into lp:launchpad.

=== modified file 'lib/lp/services/verification/browser/logintoken.py'
--- lib/lp/services/verification/browser/logintoken.py	2012-08-14 23:19:36 +0000
+++ lib/lp/services/verification/browser/logintoken.py	2012-09-28 18:09:24 +0000
@@ -535,7 +535,8 @@
 
     def initialize(self):
         self.redirectIfInvalidOrConsumedToken()
-        self.dupe = getUtility(IPersonSet).getByEmail(self.context.email)
+        self.dupe = getUtility(IPersonSet).getByEmail(
+            self.context.email, filter_status=False)
 
     def success(self, message):
         # We're not a GeneralFormView, so we need to do the redirect

=== modified file 'lib/lp/services/verification/browser/tests/test_logintoken.py'
--- lib/lp/services/verification/browser/tests/test_logintoken.py	2012-06-14 05:18:22 +0000
+++ lib/lp/services/verification/browser/tests/test_logintoken.py	2012-09-28 18:09:24 +0000
@@ -4,6 +4,7 @@
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from lp.services.identity.interfaces.account import AccountStatus
 from lp.services.identity.interfaces.emailaddress import EmailAddressStatus
 from lp.services.verification.browser.logintoken import (
     ClaimTeamView,
@@ -18,6 +19,7 @@
     )
 from lp.testing.deprecated import LaunchpadFormHarness
 from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.views import create_initialized_view
 
 
 class TestCancelActionOnLoginTokenViews(TestCaseWithFactory):
@@ -100,3 +102,46 @@
         msgs = self._claimToken(token2)
         self.assertEquals(
             [u'claimee has already been converted to a team.'], msgs)
+
+
+class MergePeopleViewTestCase(TestCaseWithFactory):
+    """Test the view for confirming a merge via login token."""
+
+    layer = DatabaseFunctionalLayer
+
+    def assertWorkflow(self, claimer, dupe):
+        token = getUtility(ILoginTokenSet).new(
+            requester=claimer, requesteremail='me@xxxxxx',
+            email="him@xxxxxx", tokentype=LoginTokenType.ACCOUNTMERGE)
+        view = create_initialized_view(token, name="+accountmerge")
+        self.assertIs(False, view.mergeCompleted)
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            '.*to merge the Launchpad account named.*claimer', view.render())
+        view = create_initialized_view(
+            token, name="+accountmerge", principal=claimer,
+            form={'VALIDATE': 'Confirm'}, method='POST')
+        with person_logged_in(claimer):
+            view.render()
+        self.assertIs(True, view.mergeCompleted)
+        notifications = view.request.notifications
+        self.assertEqual(2, len(notifications))
+        text = notifications[0].message
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            "A merge is queued.*", text)
+
+    def test_confirm_email_for_active_account(self):
+        # Users can confirm they control an email address to merge a duplicate
+        # profile.
+        claimer = self.factory.makePerson(email='me@xxxxxx', name='claimer')
+        dupe = self.factory.makePerson(email='him@xxxxxx', name='dupe')
+        self.assertWorkflow(claimer, dupe)
+
+    def test_confirm_email_for_non_active_account(self):
+        # Users can confirm they control an email address to merge a
+        # non-active duplicate profile.
+        claimer = self.factory.makePerson(email='me@xxxxxx', name='claimer')
+        dupe = self.factory.makePerson(
+            email='him@xxxxxx', name='dupe',
+            email_address_status=EmailAddressStatus.NEW,
+            account_status=AccountStatus.NOACCOUNT)
+        self.assertWorkflow(claimer, dupe)