Merge lp:~salgado/launchpad/bug-462923 into lp:launchpad

[Guilherme Salgado]

[Guilherme Salgado (salgado)]

= Summary =

https://bugs.edge.launchpad.net/launchpad-foundations/+bug/462923

== Proposed fix ==

Fix the base class to not write to self.next_url, also changing sub
classes to define a default_next_url, used by the super class' new
next_url.

After fixing this I realized the Cancel actions in all views were using
the same validate() method as the continue action, so the users would
have to enter their email address before they could successfully cancel.

Oh, and I also fixed factory.makeGPGKey(), which was completely broken.

== Pre-implementation notes ==

Discussed with Gary.

== Implementation details ==

== Tests ==

== Demo and Q/A ==

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/testing/factory.py
  lib/canonical/launchpad/browser/logintoken.py
  lib/canonical/launchpad/browser/tests/test_logintoken.py

[Gary Poster (gary)]

Hi Salgado. Great, thank you. Your knowledge of the codebase, and in particular the test harnesses, makes me remember how much I don't know. :-)

As we agreed on IRC, please add something like ``assert self.default_next_url is not None, 'The implementation of %s should provide a value for default_next_url' % (self.__class__.__name__,)`` before line 22 of the diff.

Also, please add a comment here in the MP about why we think it is OK to land this without a concurrent CIP landing.

Thanks again.

review: Approve

[Guilherme Salgado (salgado)]

For context, some of the logintoken views are extended on c-i-p, so we need to fix them too, or else their Cancel action will cause an OOPS like the one on bug 462923. However, we're at the end of week 3 and landing the two branches together would require some coordination with LOSAs, thus making it nearly impossible to get them through before the tree is closed.

Given that *and* the fact that this change doesn't make the situation any worse on c-i-p, we decided to go ahead and land this branch on its on, with a similar change on c-i-p following shortly afterwards.

[Guilherme Salgado (salgado)]

Actually, I just realized the AuthToken views don't have this problem because they don't have a 'Cancel' @action, even though their parent classes do.

That happens because the AuthToken views define a custom 'Continue' @action, and thanks to the way @action behaves, the class gets a new set of actions containing just that custom one. (I think I've encountered a similar problem in the past and might even have filed a bug; I'll see if I can find it)

[Guilherme Salgado (salgado)]

FYI, bug 154001 is the one I was talking about in the previous comment

[Francis J. Lacoste (flacoste)]

review: Approve (release-critical)

=== modified file 'lib/canonical/launchpad/browser/logintoken.py'
--- lib/canonical/launchpad/browser/logintoken.py	2009-10-23 20:12:27 +0000
+++ lib/canonical/launchpad/browser/logintoken.py	2009-11-03 18:24:22 +0000
@@ -110,6 +110,25 @@
 
     expected_token_types = ()
     successfullyProcessed = False
+    # The next URL to use when the user clicks on the 'Cancel' button.
+    _next_url_for_cancel = None
+    _missing = object()
+    # To be overridden in subclasses.
+    default_next_url = _missing
+
+    @property
+    def next_url(self):
+        """The next URL to redirect to on successful form submission.
+
+        When the cancel action is used, self._next_url_for_cancel won't be
+        None so we return that.  Otherwise we return self.default_next_url.
+        """
+        if self._next_url_for_cancel is not None:
+            return self._next_url_for_cancel
+        assert self.default_next_url is not self._missing, (
+            'The implementation of %s should provide a value for '
+            'default_next_url' % self.__class__.__name__)
+        return self.default_next_url
 
     @property
     def page_title(self):
@@ -147,11 +166,12 @@
         logInPrincipal(self.request, principal, email)
 
     def _cancel(self):
-        """Consume the LoginToken and set self.next_url.
+        """Consume the LoginToken and set self._next_url_for_cancel.
 
-        next_url is set to the home page of this LoginToken's requester.
+        _next_url_for_cancel is set to the home page of this LoginToken's
+        requester.
         """
-        self.next_url = canonical_url(self.context.requester)
+        self._next_url_for_cancel = canonical_url(self.context.requester)
         self.context.consume()
 
     def accountWasSuspended(self, account, reason):
@@ -200,7 +220,7 @@
                 "you provided when requesting the password reset."))
 
     @property
-    def next_url(self):
+    def default_next_url(self):
         if self.context.redirection_url is not None:
             return self.context.redirection_url
         else:
@@ -236,7 +256,7 @@
         self.request.response.addInfoNotification(
             _('Your password has been reset successfully.'))
 
-    @action(_('Cancel'), name='cancel')
+    @action(_('Cancel'), name='cancel', validator='validate_cancel')
     def cancel_action(self, action, data):
         self._cancel()
 
@@ -271,7 +291,7 @@
         return {'displayname': self.claimed_profile.displayname}
 
     @property
-    def next_url(self):
+    def default_next_url(self):
         return canonical_url(self.claimed_profile)
 
     @action(_('Continue'), name='confirm')
@@ -336,6 +356,10 @@
     def initial_values(self):
         return {'teamowner': self.context.requester}
 
+    @property
+    def default_next_url(self):
+        return canonical_url(self.claimed_profile)
+
     @action(_('Continue'), name='confirm')
     def confirm_action(self, action, data):
         self.claimed_profile.convertToTeam(team_owner=self.context.requester)
@@ -346,11 +370,10 @@
         # have to remove its security proxy before we update it.
         self.updateContextFromData(
             data, context=removeSecurityProxy(self.claimed_profile))
-        self.next_url = canonical_url(self.claimed_profile)
         self.request.response.addInfoNotification(
             _('Team claimed successfully'))
 
-    @action(_('Cancel'), name='cancel')
+    @action(_('Cancel'), name='cancel', validator='validate_cancel')
     def cancel_action(self, action, data):
         self._cancel()
 
@@ -371,6 +394,10 @@
                 'unexpected token type: %r' % self.context.tokentype)
             return 'Confirm OpenPGP key'
 
+    @property
+    def default_next_url(self):
+        return canonical_url(self.context.requester)
+
     def initialize(self):
         if not self.redirectIfInvalidOrConsumedToken():
             if self.context.tokentype == LoginTokenType.VALIDATESIGNONLYGPG:
@@ -382,13 +409,12 @@
         if self.context.tokentype == LoginTokenType.VALIDATESIGNONLYGPG:
             self._validateSignOnlyGPGKey(data)
 
-    @action(_('Cancel'), name='cancel')
+    @action(_('Cancel'), name='cancel', validator='validate_cancel')
     def cancel_action(self, action, data):
         self._cancel()
 
     @action(_('Continue'), name='continue')
     def continue_action_gpg(self, action, data):
-        self.next_url = canonical_url(self.context.requester)
         assert self.gpg_key is not None
         can_encrypt = (
             self.context.tokentype != LoginTokenType.VALIDATESIGNONLYGPG)
@@ -638,7 +664,7 @@
                 pass
 
     @property
-    def next_url(self):
+    def default_next_url(self):
         if self.context.redirection_url is not None:
             return self.context.redirection_url
         else:
@@ -646,7 +672,7 @@
                 "LoginTokens of this type must have a requester")
             return canonical_url(self.context.requester)
 
-    @action(_('Cancel'), name='cancel')
+    @action(_('Cancel'), name='cancel', validator='validate_cancel')
     def cancel_action(self, action, data):
         self._cancel()
 
@@ -815,7 +841,7 @@
             super(NewUserAccountView, self).initialize()
 
     @property
-    def next_url(self):
+    def default_next_url(self):
         if self.context.redirection_url:
             return self.context.redirection_url
         elif self.created_person is not None:
 
=== added file 'lib/canonical/launchpad/browser/tests/test_logintoken.py'
--- lib/canonical/launchpad/browser/tests/test_logintoken.py	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/browser/tests/test_logintoken.py	2009-11-03 18:24:22 +0000
@@ -0,0 +1,73 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+import unittest
+
+from zope.component import getUtility
+
+from canonical.launchpad.browser.logintoken import (
+    ClaimTeamView, ResetPasswordView, ValidateEmailView, ValidateGPGKeyView)
+from canonical.launchpad.ftests import LaunchpadFormHarness
+from canonical.launchpad.interfaces.authtoken import LoginTokenType
+from canonical.launchpad.interfaces.logintoken import ILoginTokenSet
+from lp.testing import TestCaseWithFactory
+from canonical.testing import DatabaseFunctionalLayer
+
+
+class TestCancelActionOnLoginTokenViews(TestCaseWithFactory):
+    """Test the 'Cancel' action of LoginToken views.
+
+    These views have an action instead of a link to cancel because we want the
+    token to be consumed (so it can't be used again) when the user hits
+    Cancel.
+    """
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        TestCaseWithFactory.setUp(self)
+        self.person = self.factory.makePerson(name='test-user')
+        self.email = self.person.preferredemail.email
+        self.expected_next_url = 'http://127.0.0.1/~test-user'
+
+    def test_ResetPasswordView(self):
+        token = getUtility(ILoginTokenSet).new(
+            self.person, self.email, self.email,
+            LoginTokenType.PASSWORDRECOVERY)
+        self._testCancelAction(ResetPasswordView, token)
+
+    def test_ClaimTeamView(self):
+        token = getUtility(ILoginTokenSet).new(
+            self.person, self.email, self.email, LoginTokenType.TEAMCLAIM)
+        self._testCancelAction(ClaimTeamView, token)
+
+    def test_ValidateGPGKeyView(self):
+        self.gpg_key = self.factory.makeGPGKey(self.person)
+        token = getUtility(ILoginTokenSet).new(
+            self.person, self.email, self.email, LoginTokenType.VALIDATEGPG,
+            fingerprint=self.gpg_key.fingerprint)
+        self._testCancelAction(ValidateGPGKeyView, token)
+
+    def test_ValidateEmailView(self):
+        token = getUtility(ILoginTokenSet).new(
+            self.person, self.email, '[e-mail]',
+            LoginTokenType.VALIDATEEMAIL)
+        self._testCancelAction(ValidateEmailView, token)
+
+    def _testCancelAction(self, view_class, token):
+        """Test the 'Cancel' action of the given view, using the given token.
+        
+        To test that the action works, we just submit the form with that
+        action, check that there are no errors and make sure that the view's
+        next_url is what we expect.
+        """
+        harness = LaunchpadFormHarness(token, view_class)
+        harness.submit('cancel', {})
+        actions = harness.view.actions.byname
+        self.assertIn('field.actions.cancel', actions)
+        self.assertEquals(actions['field.actions.cancel'].submitted(), True)
+        self.assertEquals(harness.view.errors, [])
+        self.assertEquals(harness.view.next_url, self.expected_next_url)
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)
 
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2009-10-26 15:25:10 +0000
+++ lib/lp/testing/factory.py	2009-11-03 18:24:22 +0000
@@ -176,9 +176,10 @@
         :return: A hexadecimal string, with 'a'-'f' in lower case.
         """
         hex_number = '%x' % self.getUniqueInteger()
-        if digits is not None:
-            hex_number.zfill(digits)
-        return hex_number
+        if digits is None:
+            return hex_number
+        else:
+            return hex_number.zfill(digits)
 
     def getUniqueString(self, prefix=None):
         """Return a string unique to this factory instance.
@@ -266,7 +267,7 @@
             owner.id,
             keyid=self.getUniqueHexString(digits=8).upper(),
             fingerprint='A' * 40,
-            keysize=self.factory.getUniqueInteger(),
+            keysize=self.getUniqueInteger(),
             algorithm=GPGKeyAlgorithm.R,
             active=True,
             can_encrypt=False)