Launchpad itself

Merge lp:~adiroiban/launchpad/bug-522188 into lp:launchpad

  1. bug-522188
  2. Merge into devel
Proposed by Adi Roiban
Status: Merged
Approved by: Graham Binns
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~adiroiban/launchpad/bug-522188
Merge into: lp:launchpad
Diff against target: 140 lines (+87/-6)
4 files modified
lib/lp/app/templates/generic-edit-next-url.pt (+19/-0)
lib/lp/translations/browser/configure.zcml (+2/-2)
lib/lp/translations/browser/potemplate.py (+33/-2)
lib/lp/translations/stories/standalone/xx-potemplate-edit.txt (+33/-2)
To merge this branch: bzr merge lp:~adiroiban/launchpad/bug-522188
Reviewer Review Type Date Requested Status
Graham Binns (community) code Approve
Review via email: mp+19395@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Adi Roiban (adiroiban) wrote :

= Bug 522188 =
In the merge review for bug 340664, Curtis noted that the next_url for potemplate edit and admin forms is not using the referer address.

Below are his comments:

The behaviour of these actions is disconcerting. When I edit a a template,
the next_url is the template, but I expected to return to the all templates
view.

== Proposed fix ==
Use HTTP_REFERER in cancel_url and next_url

== Pre-implementation notes ==
In the merge review for bug 340664 Curtis suggested using HTTP_REFERER for cancel_url and next_url.

== Implementation details ==
Since the template edit and admin form are using the generic edit template, I have added the extra_info fill-slot there.

Please let me know if I should create a new template.

Since this looks like a generic behaviour, maybe we can have this implementation in LaunchpadForm and not in each class extending LaunchpadForm

== Tests ==
lp-test -t template-edit

== Demo and Q/A ==
Login as admin

1. Visit a series templates page: ie: https://translations.launchpad.dev/evolution/trunk/+templates

Click 'Edit' or 'Admin' link for a template.

On the form page, click Update or Cancel, it should lead you back to the templates page.

2. Directly access the edit page at https://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2-test/+edit

Click Update or Cancel. It should lead you to the template index page. ie https://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2-test

= 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/app/templates/generic-edit.pt
  lib/lp/translations/browser/potemplate.py
  lib/lp/translations/stories/standalone/xx-potemplate-edit.txt

Revision history for this message
Graham Binns (gmb) wrote :

Hi Adi,

Good branch; I'll run this through ec2 for you.

Approved revision: 10317.

review: Approve (code)
Revision history for this message
Adi Roiban (adiroiban) wrote :
Download full text (4.2 KiB)

Hi Graham,

I had to use another generic-edit template as some views were not compatible with the previous implementation of generic-edit.

Also, since the POTemplate admin page can rename the template name, I had to implement this dirty hack in order to avoid redirecting to a page that is no longer available.
I asked Curtis, and he agree that this is not a clean solution but he also said the code is correct and he is not aware of any other better solution.

Any suggestion is much appreciated.

Can you please review the changes?
Here is the new diff.

=== added file 'lib/lp/app/templates/generic-edit-next-url.pt'
--- lib/lp/app/templates/generic-edit-next-url.pt 1970-01-01 00:00:00 +0000
+++ lib/lp/app/templates/generic-edit-next-url.pt 2010-02-22 22:44:11 +0000
@@ -0,0 +1,19 @@
+<html
+ xmlns="http://www.w3.org/1999/xhtml"
+ xmlns:tal="http://xml.zope.org/namespaces/tal"
+ xmlns:metal="http://xml.zope.org/namespaces/metal"
+ xmlns:i18n="http://xml.zope.org/namespaces/i18n"
+ metal:use-macro="view/macro:page/main_only"
+ i18n:domain="launchpad">
+ <body>
+ <div metal:fill-slot="main">
+ <div metal:use-macro="context/@@launchpad_form/form">
+ <div metal:fill-slot="extra_info">
+ <input type="hidden" name="next_url"
+ tal:condition="view/next_url"
+ tal:attributes="value view/next_url" />
+ </div>
+ </div>
+ </div>
+ </body>
+</html>

=== modified file 'lib/lp/app/templates/generic-edit.pt'
--- lib/lp/app/templates/generic-edit.pt 2010-02-16 12:15:12 +0000
+++ lib/lp/app/templates/generic-edit.pt 2010-02-22 22:43:37 +0000
@@ -7,13 +7,7 @@
   i18n:domain="launchpad">
   <body>
     <div metal:fill-slot="main">
- <div metal:use-macro="context/@@launchpad_form/form">
- <div metal:fill-slot="extra_info">
- <input type="hidden" name="next_url"
- tal:condition="view/next_url"
- tal:attributes="value view/next_url" />
- </div>
- </div>
+ <div metal:use-macro="context/@@launchpad_form/form" />
     </div>
   </body>
 </html>

=== modified file 'lib/lp/translations/browser/configure.zcml'
--- lib/lp/translations/browser/configure.zcml 2010-02-10 13:04:10 +0000
+++ lib/lp/translations/browser/configure.zcml 2010-02-22 22:47:03 +0000
@@ -414,14 +414,14 @@
             for="lp.translations.interfaces.potemplate.IPOTemplate"
             class="lp.translations.browser.potemplate.POTemplateEditView"
             permission="launchpad.Edit"
- template="../../app/templates/generic-edit.pt"
+ template="../../app/templates/generic-edit-next-url.pt"
             layer="canonical.launchpad.layers.TranslationsLayer"/>
         <browser:page
             name="+admin"
             for="lp.translations.interfaces.potemplate.IPOTemplate"
             class="lp.translations.browser.potemplate.POTemplateAdminView"
             permission="launchpad.TranslationsAdmin"
- template="../../app/templates/generic-edit.pt"
+ template="../../app/templates/generic-edit-next-url.pt"
             layer="canonical.launchpad.layers.TranslationsLayer"/>
         <browser:page
             name="...

Read more...

Revision history for this message
Graham Binns (gmb) wrote :

Hi Adi,

That's a horrible, horrible hack, but Curtis is right; the code's fine, we just need a better way of dealing with it.

I'll give this r=me, but before I do please file a bug about that XXX and refer to it in the XXX comment (e.g. XXX AdiRoiban 2010-02-23 bug=yyyyyy...).

review: Needs Fixing (code)
Revision history for this message
Adi Roiban (adiroiban) wrote :

Hi,

Reported bug 526998, and pushed the changes.

I have tested the previous failing tests and everything was fine.

When you have time, can you please try again to land it again.

Thanks!

Revision history for this message
Graham Binns (gmb) :
review: Approve (code)
Revision history for this message
Adi Roiban (adiroiban) wrote :

Hi,

I fix the problem causing the potemplate-admin test to fail.

When you have time, please take a look at this diff and tell me if there is anything I need to do to have this branch landed.

Many thanks!

Here is the diff.
=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py 2010-03-09 14:59:09 +0000
+++ lib/lp/translations/browser/potemplate.py 2010-03-09 15:44:38 +0000
@@ -550,10 +550,19 @@
         # The referer header we want is only available before the view's
         # form submits to itself. This field is a hidden input in the form.
         referrer = self.request.form.get('next_url')
+
         if referrer is None:
             referrer = self.request.getHeader('referer')
+ # If we don't have a referrer in the HTTP header, if referrer
+ # contains the template name or if referrer is outside of our
+ # website, getting the next_url is delayed until the
+ # form is submitted.
+ # It is not computed before the submission, since from this form
+ # the template name can be changed and if referrer url depends on
+ # it, renaming will make the referrer an invalid URL.
             if (referrer is None
- or self.context.name in referrer):
+ or self.context.name in referrer
+ or not referrer.startswith(self.request.getApplicationURL())):
                     # XXX: AdiRoiban 2010-02-32 bug=526998
                     # Since 'referer' can depend on the object name, and
                     # since from this form we can rename the object

Revision history for this message
Graham Binns (gmb) wrote :

Yep, that change looks fine.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'lib/lp/app/templates/generic-edit-next-url.pt'
2--- lib/lp/app/templates/generic-edit-next-url.pt 1970-01-01 00:00:00 +0000
3+++ lib/lp/app/templates/generic-edit-next-url.pt 2010-03-09 16:58:26 +0000
4@@ -0,0 +1,19 @@
5+<html
6+ xmlns="http://www.w3.org/1999/xhtml"
7+ xmlns:tal="http://xml.zope.org/namespaces/tal"
8+ xmlns:metal="http://xml.zope.org/namespaces/metal"
9+ xmlns:i18n="http://xml.zope.org/namespaces/i18n"
10+ metal:use-macro="view/macro:page/main_only"
11+ i18n:domain="launchpad">
12+ <body>
13+ <div metal:fill-slot="main">
14+ <div metal:use-macro="context/@@launchpad_form/form">
15+ <div metal:fill-slot="extra_info">
16+ <input type="hidden" name="next_url"
17+ tal:condition="view/next_url"
18+ tal:attributes="value view/next_url" />
19+ </div>
20+ </div>
21+ </div>
22+ </body>
23+</html>
24
25=== modified file 'lib/lp/translations/browser/configure.zcml'
26--- lib/lp/translations/browser/configure.zcml 2010-02-18 09:04:51 +0000
27+++ lib/lp/translations/browser/configure.zcml 2010-03-09 16:58:26 +0000
28@@ -414,14 +414,14 @@
29 for="lp.translations.interfaces.potemplate.IPOTemplate"
30 class="lp.translations.browser.potemplate.POTemplateEditView"
31 permission="launchpad.Edit"
32- template="../../app/templates/generic-edit.pt"
33+ template="../../app/templates/generic-edit-next-url.pt"
34 layer="canonical.launchpad.layers.TranslationsLayer"/>
35 <browser:page
36 name="+admin"
37 for="lp.translations.interfaces.potemplate.IPOTemplate"
38 class="lp.translations.browser.potemplate.POTemplateAdminView"
39 permission="launchpad.TranslationsAdmin"
40- template="../../app/templates/generic-edit.pt"
41+ template="../../app/templates/generic-edit-next-url.pt"
42 layer="canonical.launchpad.layers.TranslationsLayer"/>
43 <browser:page
44 name="+export"
45
46=== modified file 'lib/lp/translations/browser/potemplate.py'
47--- lib/lp/translations/browser/potemplate.py 2010-02-18 17:38:47 +0000
48+++ lib/lp/translations/browser/potemplate.py 2010-03-09 16:58:26 +0000
49@@ -539,11 +539,42 @@
50
51 @property
52 def cancel_url(self):
53- return canonical_url(self.context)
54+ if self.next_url == "DIRTY_HACK":
55+ return canonical_url(self.context)
56+ else:
57+ return self.next_url
58
59 @property
60 def next_url(self):
61- return canonical_url(self.context)
62+ """See `LaunchpadEditFormView`."""
63+ # The referer header we want is only available before the view's
64+ # form submits to itself. This field is a hidden input in the form.
65+ referrer = self.request.form.get('next_url')
66+
67+ if referrer is None:
68+ referrer = self.request.getHeader('referer')
69+ # If we don't have a referrer in the HTTP header, if referrer
70+ # contains the template name or if referrer is outside of our
71+ # website, getting the next_url is delayed until the
72+ # form is submitted.
73+ # It is not computed before the submission, since from this form
74+ # the template name can be changed and if referrer url depends on
75+ # it, renaming will make the referrer an invalid URL.
76+ if (referrer is None
77+ or self.context.name in referrer
78+ or not referrer.startswith(self.request.getApplicationURL())):
79+ # XXX: AdiRoiban 2010-02-32 bug=526998
80+ # Since 'referer' can depend on the object name, and
81+ # since from this form we can rename the object
82+ # I was forced to use this dirty hack.
83+ return "DIRTY_HACK"
84+
85+ if (referrer is not None
86+ and referrer.startswith(self.request.getApplicationURL())
87+ and referrer != self.request.getHeader('location')):
88+ return referrer
89+ else:
90+ return canonical_url(self.context)
91
92
93 class POTemplateAdminView(POTemplateEditView):
94
95=== modified file 'lib/lp/translations/stories/standalone/xx-potemplate-edit.txt'
96--- lib/lp/translations/stories/standalone/xx-potemplate-edit.txt 2010-01-20 20:36:13 +0000
97+++ lib/lp/translations/stories/standalone/xx-potemplate-edit.txt 2010-03-09 16:58:26 +0000
98@@ -1,5 +1,8 @@
99-The POTemplate edit page lets us to edit basic aspect of that object. Only
100-product owners, Rosetta Experts or a Launchpad admin are able to use it.
101+Editing POTemplates
102+===================
103+
104+The POTemplate edit page allows editing a subset of potemplate attributes.
105+Only product owners, Rosetta Experts or a Launchpad admin are able to use it.
106
107 An unpriviledged user cannot reach this page.
108
109@@ -139,3 +142,31 @@
110 >>> previous_date_last_updated != evolution_template.date_last_updated
111 True
112
113+
114+Change and cancel actions
115+-------------------------
116+
117+After changing or canceling a form you will be redirected to the previous page
118+from you navigation.
119+
120+The edit page can be access from the templates list.
121+
122+ >>> referrer = 'http://translations.launchpad.dev/evolution/trunk/+templates'
123+ >>> admin_browser.open(referrer)
124+ >>> admin_browser.getLink(
125+ ... url='/evolution/trunk/+pots/'
126+ ... 'evolution-2.2/+edit').click()
127+ >>> admin_browser.getControl('Change').click()
128+ >>> admin_browser.url == referrer
129+ True
130+
131+If you are accesing the edit page using a bookmark (in this case there was
132+no previous page in the navigation), you will be directed to the template
133+index page.
134+
135+ >>> admin_browser.open(
136+ ... 'http://translations.launchpad.dev/evolution/trunk/+pots/'
137+ ... 'evolution-2.2/+edit')
138+ >>> admin_browser.getLink('Cancel').click()
139+ >>> print admin_browser.url
140+ http://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2