launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:merge-people-cancel into launchpad:master.

Commit message:
Add ability to cancel account merge requests

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #74680 in Launchpad itself: "+accountmerge page should present the option to revoke the merge request"
  https://bugs.launchpad.net/launchpad/+bug/74680

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/394580

Otherwise it's impossible to cancel them without SQL surgery, unlike other login tokens.  The simplest way to do this was to rework MergePeopleView as a LaunchpadFormView.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:merge-people-cancel into launchpad:master.

diff --git a/lib/lp/services/verification/browser/logintoken.py b/lib/lp/services/verification/browser/logintoken.py
index 245d8f0..b55648d 100644
--- a/lib/lp/services/verification/browser/logintoken.py
+++ b/lib/lp/services/verification/browser/logintoken.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -477,27 +477,31 @@ class ValidateTeamEmailView(ValidateEmailView):
         self.context.requester.setContactAddress(email)
 
 
-class MergePeopleView(BaseTokenView, LaunchpadView):
+class MergePeopleView(BaseTokenView, LaunchpadFormView):
+
+    schema = Interface
+    field_names = []
     expected_token_types = (LoginTokenType.ACCOUNTMERGE,)
     mergeCompleted = False
     label = 'Merge Launchpad accounts'
 
+    @property
+    def default_next_url(self):
+        return canonical_url(self.context.requester)
+
     def initialize(self):
         self.redirectIfInvalidOrConsumedToken()
         self.dupe = getUtility(IPersonSet).getByEmail(
             self.context.email, filter_status=False)
+        super(MergePeopleView, self).initialize()
 
-    def success(self, message):
-        # We're not a GeneralFormView, so we need to do the redirect
-        # ourselves.
-        BaseTokenView.success(self, message)
-        self.request.response.redirect(canonical_url(self.context.requester))
+    @action(_('Cancel'), name='cancel', validator='validate_cancel')
+    def cancel_action(self, action, data):
+        self._cancel()
 
-    def processForm(self):
+    @action(_('Confirm'), name='confirm')
+    def confirm_action(self, action, data):
         """Perform the merge."""
-        if self.request.method != "POST":
-            return
-
         # Merge requests must have a valid user account (one with a preferred
         # email) as requester.
         assert self.context.requester.preferredemail is not None
diff --git a/lib/lp/services/verification/browser/tests/test_logintoken.py b/lib/lp/services/verification/browser/tests/test_logintoken.py
index 7ed239d..f48e67d 100644
--- a/lib/lp/services/verification/browser/tests/test_logintoken.py
+++ b/lib/lp/services/verification/browser/tests/test_logintoken.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from zope.component import getUtility
@@ -8,6 +8,7 @@ from lp.services.identity.interfaces.account import AccountStatus
 from lp.services.identity.interfaces.emailaddress import EmailAddressStatus
 from lp.services.verification.browser.logintoken import (
     ClaimTeamView,
+    MergePeopleView,
     ValidateEmailView,
     ValidateGPGKeyView,
     )
@@ -59,6 +60,15 @@ class TestCancelActionOnLoginTokenViews(TestCaseWithFactory):
                 LoginTokenType.VALIDATEEMAIL)
             self._testCancelAction(ValidateEmailView, token)
 
+    def test_MergePeopleView(self):
+        dupe = self.factory.makePerson()
+        with person_logged_in(self.person):
+            token = getUtility(ILoginTokenSet).new(
+                self.person, self.email,
+                email=removeSecurityProxy(dupe).preferredemail.email,
+                tokentype=LoginTokenType.ACCOUNTMERGE)
+            self._testCancelAction(MergePeopleView, token)
+
     def _testCancelAction(self, view_class, token):
         """Test the 'Cancel' action of the given view, using the given token.
 
@@ -120,10 +130,10 @@ class MergePeopleViewTestCase(TestCaseWithFactory):
         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 = create_initialized_view(
+                token, name="+accountmerge", principal=claimer,
+                form={'field.actions.confirm': 'Confirm'}, method='POST')
             view.render()
         self.assertIs(True, view.mergeCompleted)
         notifications = view.request.notifications
diff --git a/lib/lp/services/verification/templates/logintoken-mergepeople.pt b/lib/lp/services/verification/templates/logintoken-mergepeople.pt
index 2fa6265..4364127 100644
--- a/lib/lp/services/verification/templates/logintoken-mergepeople.pt
+++ b/lib/lp/services/verification/templates/logintoken-mergepeople.pt
@@ -3,7 +3,6 @@
   xmlns:metal="http://xml.zope.org/namespaces/metal";
   xmlns:i18n="http://xml.zope.org/namespaces/i18n";
   omit-tag="">
-<tal:do-this-first tal:content="view/processForm" />
 
 <html
   metal:use-macro="view/macro:page/main_only"
@@ -17,9 +16,7 @@
       <code tal:content="view/dupe/name">foo</code> into the
       account named <code tal:content="context/requester/name">bar</code>.</p>
 
-      <form name="merge" action="" method="POST">
-        <input type="submit" name="VALIDATE" value="Confirm" />
-      </form>
+      <div metal:use-macro="context/@@launchpad_form/form" />
     </div>
 
 </div>