Launchpad itself

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

  1. bug-462923
  2. Merge into devel
Proposed by Guilherme Salgado
Status: Merged
Approved by: Gary Poster
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~salgado/launchpad/bug-462923
Merge into: lp:launchpad
Diff against target: 254 lines
3 files modified
lib/canonical/launchpad/browser/logintoken.py (+39/-13)
lib/canonical/launchpad/browser/tests/test_logintoken.py (+73/-0)
lib/lp/testing/factory.py (+5/-4)
To merge this branch: bzr merge lp:~salgado/launchpad/bug-462923
Reviewer Review Type Date Requested Status
Francis J. Lacoste (community) release-critical Approve
Gary Poster (community) Approve
Review via email: mp+14232@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Guilherme Salgado (salgado) wrote :

= 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

Revision history for this message
Gary Poster (gary) wrote :

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
Revision history for this message
Guilherme Salgado (salgado) wrote :

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.

Revision history for this message
Guilherme Salgado (salgado) wrote :

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)

Revision history for this message
Guilherme Salgado (salgado) wrote :

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

Revision history for this message
Francis J. Lacoste (flacoste) :
review: Approve (release-critical)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/browser/logintoken.py'
2--- lib/canonical/launchpad/browser/logintoken.py 2009-10-23 20:12:27 +0000
3+++ lib/canonical/launchpad/browser/logintoken.py 2009-11-03 18:24:22 +0000
4@@ -110,6 +110,25 @@
5
6 expected_token_types = ()
7 successfullyProcessed = False
8+ # The next URL to use when the user clicks on the 'Cancel' button.
9+ _next_url_for_cancel = None
10+ _missing = object()
11+ # To be overridden in subclasses.
12+ default_next_url = _missing
13+
14+ @property
15+ def next_url(self):
16+ """The next URL to redirect to on successful form submission.
17+
18+ When the cancel action is used, self._next_url_for_cancel won't be
19+ None so we return that. Otherwise we return self.default_next_url.
20+ """
21+ if self._next_url_for_cancel is not None:
22+ return self._next_url_for_cancel
23+ assert self.default_next_url is not self._missing, (
24+ 'The implementation of %s should provide a value for '
25+ 'default_next_url' % self.__class__.__name__)
26+ return self.default_next_url
27
28 @property
29 def page_title(self):
30@@ -147,11 +166,12 @@
31 logInPrincipal(self.request, principal, email)
32
33 def _cancel(self):
34- """Consume the LoginToken and set self.next_url.
35+ """Consume the LoginToken and set self._next_url_for_cancel.
36
37- next_url is set to the home page of this LoginToken's requester.
38+ _next_url_for_cancel is set to the home page of this LoginToken's
39+ requester.
40 """
41- self.next_url = canonical_url(self.context.requester)
42+ self._next_url_for_cancel = canonical_url(self.context.requester)
43 self.context.consume()
44
45 def accountWasSuspended(self, account, reason):
46@@ -200,7 +220,7 @@
47 "you provided when requesting the password reset."))
48
49 @property
50- def next_url(self):
51+ def default_next_url(self):
52 if self.context.redirection_url is not None:
53 return self.context.redirection_url
54 else:
55@@ -236,7 +256,7 @@
56 self.request.response.addInfoNotification(
57 _('Your password has been reset successfully.'))
58
59- @action(_('Cancel'), name='cancel')
60+ @action(_('Cancel'), name='cancel', validator='validate_cancel')
61 def cancel_action(self, action, data):
62 self._cancel()
63
64@@ -271,7 +291,7 @@
65 return {'displayname': self.claimed_profile.displayname}
66
67 @property
68- def next_url(self):
69+ def default_next_url(self):
70 return canonical_url(self.claimed_profile)
71
72 @action(_('Continue'), name='confirm')
73@@ -336,6 +356,10 @@
74 def initial_values(self):
75 return {'teamowner': self.context.requester}
76
77+ @property
78+ def default_next_url(self):
79+ return canonical_url(self.claimed_profile)
80+
81 @action(_('Continue'), name='confirm')
82 def confirm_action(self, action, data):
83 self.claimed_profile.convertToTeam(team_owner=self.context.requester)
84@@ -346,11 +370,10 @@
85 # have to remove its security proxy before we update it.
86 self.updateContextFromData(
87 data, context=removeSecurityProxy(self.claimed_profile))
88- self.next_url = canonical_url(self.claimed_profile)
89 self.request.response.addInfoNotification(
90 _('Team claimed successfully'))
91
92- @action(_('Cancel'), name='cancel')
93+ @action(_('Cancel'), name='cancel', validator='validate_cancel')
94 def cancel_action(self, action, data):
95 self._cancel()
96
97@@ -371,6 +394,10 @@
98 'unexpected token type: %r' % self.context.tokentype)
99 return 'Confirm OpenPGP key'
100
101+ @property
102+ def default_next_url(self):
103+ return canonical_url(self.context.requester)
104+
105 def initialize(self):
106 if not self.redirectIfInvalidOrConsumedToken():
107 if self.context.tokentype == LoginTokenType.VALIDATESIGNONLYGPG:
108@@ -382,13 +409,12 @@
109 if self.context.tokentype == LoginTokenType.VALIDATESIGNONLYGPG:
110 self._validateSignOnlyGPGKey(data)
111
112- @action(_('Cancel'), name='cancel')
113+ @action(_('Cancel'), name='cancel', validator='validate_cancel')
114 def cancel_action(self, action, data):
115 self._cancel()
116
117 @action(_('Continue'), name='continue')
118 def continue_action_gpg(self, action, data):
119- self.next_url = canonical_url(self.context.requester)
120 assert self.gpg_key is not None
121 can_encrypt = (
122 self.context.tokentype != LoginTokenType.VALIDATESIGNONLYGPG)
123@@ -638,7 +664,7 @@
124 pass
125
126 @property
127- def next_url(self):
128+ def default_next_url(self):
129 if self.context.redirection_url is not None:
130 return self.context.redirection_url
131 else:
132@@ -646,7 +672,7 @@
133 "LoginTokens of this type must have a requester")
134 return canonical_url(self.context.requester)
135
136- @action(_('Cancel'), name='cancel')
137+ @action(_('Cancel'), name='cancel', validator='validate_cancel')
138 def cancel_action(self, action, data):
139 self._cancel()
140
141@@ -815,7 +841,7 @@
142 super(NewUserAccountView, self).initialize()
143
144 @property
145- def next_url(self):
146+ def default_next_url(self):
147 if self.context.redirection_url:
148 return self.context.redirection_url
149 elif self.created_person is not None:
150
151=== added file 'lib/canonical/launchpad/browser/tests/test_logintoken.py'
152--- lib/canonical/launchpad/browser/tests/test_logintoken.py 1970-01-01 00:00:00 +0000
153+++ lib/canonical/launchpad/browser/tests/test_logintoken.py 2009-11-03 18:24:22 +0000
154@@ -0,0 +1,73 @@
155+# Copyright 2009 Canonical Ltd. This software is licensed under the
156+# GNU Affero General Public License version 3 (see the file LICENSE).
157+
158+import unittest
159+
160+from zope.component import getUtility
161+
162+from canonical.launchpad.browser.logintoken import (
163+ ClaimTeamView, ResetPasswordView, ValidateEmailView, ValidateGPGKeyView)
164+from canonical.launchpad.ftests import LaunchpadFormHarness
165+from canonical.launchpad.interfaces.authtoken import LoginTokenType
166+from canonical.launchpad.interfaces.logintoken import ILoginTokenSet
167+from lp.testing import TestCaseWithFactory
168+from canonical.testing import DatabaseFunctionalLayer
169+
170+
171+class TestCancelActionOnLoginTokenViews(TestCaseWithFactory):
172+ """Test the 'Cancel' action of LoginToken views.
173+
174+ These views have an action instead of a link to cancel because we want the
175+ token to be consumed (so it can't be used again) when the user hits
176+ Cancel.
177+ """
178+ layer = DatabaseFunctionalLayer
179+
180+ def setUp(self):
181+ TestCaseWithFactory.setUp(self)
182+ self.person = self.factory.makePerson(name='test-user')
183+ self.email = self.person.preferredemail.email
184+ self.expected_next_url = 'http://127.0.0.1/~test-user'
185+
186+ def test_ResetPasswordView(self):
187+ token = getUtility(ILoginTokenSet).new(
188+ self.person, self.email, self.email,
189+ LoginTokenType.PASSWORDRECOVERY)
190+ self._testCancelAction(ResetPasswordView, token)
191+
192+ def test_ClaimTeamView(self):
193+ token = getUtility(ILoginTokenSet).new(
194+ self.person, self.email, self.email, LoginTokenType.TEAMCLAIM)
195+ self._testCancelAction(ClaimTeamView, token)
196+
197+ def test_ValidateGPGKeyView(self):
198+ self.gpg_key = self.factory.makeGPGKey(self.person)
199+ token = getUtility(ILoginTokenSet).new(
200+ self.person, self.email, self.email, LoginTokenType.VALIDATEGPG,
201+ fingerprint=self.gpg_key.fingerprint)
202+ self._testCancelAction(ValidateGPGKeyView, token)
203+
204+ def test_ValidateEmailView(self):
205+ token = getUtility(ILoginTokenSet).new(
206+ self.person, self.email, 'foo@example.com',
207+ LoginTokenType.VALIDATEEMAIL)
208+ self._testCancelAction(ValidateEmailView, token)
209+
210+ def _testCancelAction(self, view_class, token):
211+ """Test the 'Cancel' action of the given view, using the given token.
212+
213+ To test that the action works, we just submit the form with that
214+ action, check that there are no errors and make sure that the view's
215+ next_url is what we expect.
216+ """
217+ harness = LaunchpadFormHarness(token, view_class)
218+ harness.submit('cancel', {})
219+ actions = harness.view.actions.byname
220+ self.assertIn('field.actions.cancel', actions)
221+ self.assertEquals(actions['field.actions.cancel'].submitted(), True)
222+ self.assertEquals(harness.view.errors, [])
223+ self.assertEquals(harness.view.next_url, self.expected_next_url)
224+
225+
226+def test_suite():
227+ return unittest.TestLoader().loadTestsFromName(__name__)
228
229=== modified file 'lib/lp/testing/factory.py'
230--- lib/lp/testing/factory.py 2009-10-26 15:25:10 +0000
231+++ lib/lp/testing/factory.py 2009-11-03 18:24:22 +0000
232@@ -176,9 +176,10 @@
233 :return: A hexadecimal string, with 'a'-'f' in lower case.
234 """
235 hex_number = '%x' % self.getUniqueInteger()
236- if digits is not None:
237- hex_number.zfill(digits)
238- return hex_number
239+ if digits is None:
240+ return hex_number
241+ else:
242+ return hex_number.zfill(digits)
243
244 def getUniqueString(self, prefix=None):
245 """Return a string unique to this factory instance.
246@@ -266,7 +267,7 @@
247 owner.id,
248 keyid=self.getUniqueHexString(digits=8).upper(),
249 fingerprint='A' * 40,
250- keysize=self.factory.getUniqueInteger(),
251+ keysize=self.getUniqueInteger(),
252 algorithm=GPGKeyAlgorithm.R,
253 active=True,
254 can_encrypt=False)