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
=== 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-03-09 16:58:26 +0000
@@ -0,0 +1,19 @@
1<html
2 xmlns="http://www.w3.org/1999/xhtml"
3 xmlns:tal="http://xml.zope.org/namespaces/tal"
4 xmlns:metal="http://xml.zope.org/namespaces/metal"
5 xmlns:i18n="http://xml.zope.org/namespaces/i18n"
6 metal:use-macro="view/macro:page/main_only"
7 i18n:domain="launchpad">
8 <body>
9 <div metal:fill-slot="main">
10 <div metal:use-macro="context/@@launchpad_form/form">
11 <div metal:fill-slot="extra_info">
12 <input type="hidden" name="next_url"
13 tal:condition="view/next_url"
14 tal:attributes="value view/next_url" />
15 </div>
16 </div>
17 </div>
18 </body>
19</html>
020
=== modified file 'lib/lp/translations/browser/configure.zcml'
--- lib/lp/translations/browser/configure.zcml 2010-02-18 09:04:51 +0000
+++ lib/lp/translations/browser/configure.zcml 2010-03-09 16:58:26 +0000
@@ -414,14 +414,14 @@
414 for="lp.translations.interfaces.potemplate.IPOTemplate"414 for="lp.translations.interfaces.potemplate.IPOTemplate"
415 class="lp.translations.browser.potemplate.POTemplateEditView"415 class="lp.translations.browser.potemplate.POTemplateEditView"
416 permission="launchpad.Edit"416 permission="launchpad.Edit"
417 template="../../app/templates/generic-edit.pt"417 template="../../app/templates/generic-edit-next-url.pt"
418 layer="canonical.launchpad.layers.TranslationsLayer"/>418 layer="canonical.launchpad.layers.TranslationsLayer"/>
419 <browser:page419 <browser:page
420 name="+admin"420 name="+admin"
421 for="lp.translations.interfaces.potemplate.IPOTemplate"421 for="lp.translations.interfaces.potemplate.IPOTemplate"
422 class="lp.translations.browser.potemplate.POTemplateAdminView"422 class="lp.translations.browser.potemplate.POTemplateAdminView"
423 permission="launchpad.TranslationsAdmin"423 permission="launchpad.TranslationsAdmin"
424 template="../../app/templates/generic-edit.pt"424 template="../../app/templates/generic-edit-next-url.pt"
425 layer="canonical.launchpad.layers.TranslationsLayer"/>425 layer="canonical.launchpad.layers.TranslationsLayer"/>
426 <browser:page426 <browser:page
427 name="+export"427 name="+export"
428428
=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py 2010-02-18 17:38:47 +0000
+++ lib/lp/translations/browser/potemplate.py 2010-03-09 16:58:26 +0000
@@ -539,11 +539,42 @@
539539
540 @property540 @property
541 def cancel_url(self):541 def cancel_url(self):
542 return canonical_url(self.context)542 if self.next_url == "DIRTY_HACK":
543 return canonical_url(self.context)
544 else:
545 return self.next_url
543546
544 @property547 @property
545 def next_url(self):548 def next_url(self):
546 return canonical_url(self.context)549 """See `LaunchpadEditFormView`."""
550 # The referer header we want is only available before the view's
551 # form submits to itself. This field is a hidden input in the form.
552 referrer = self.request.form.get('next_url')
553
554 if referrer is None:
555 referrer = self.request.getHeader('referer')
556 # If we don't have a referrer in the HTTP header, if referrer
557 # contains the template name or if referrer is outside of our
558 # website, getting the next_url is delayed until the
559 # form is submitted.
560 # It is not computed before the submission, since from this form
561 # the template name can be changed and if referrer url depends on
562 # it, renaming will make the referrer an invalid URL.
563 if (referrer is None
564 or self.context.name in referrer
565 or not referrer.startswith(self.request.getApplicationURL())):
566 # XXX: AdiRoiban 2010-02-32 bug=526998
567 # Since 'referer' can depend on the object name, and
568 # since from this form we can rename the object
569 # I was forced to use this dirty hack.
570 return "DIRTY_HACK"
571
572 if (referrer is not None
573 and referrer.startswith(self.request.getApplicationURL())
574 and referrer != self.request.getHeader('location')):
575 return referrer
576 else:
577 return canonical_url(self.context)
547578
548579
549class POTemplateAdminView(POTemplateEditView):580class POTemplateAdminView(POTemplateEditView):
550581
=== modified file 'lib/lp/translations/stories/standalone/xx-potemplate-edit.txt'
--- lib/lp/translations/stories/standalone/xx-potemplate-edit.txt 2010-01-20 20:36:13 +0000
+++ lib/lp/translations/stories/standalone/xx-potemplate-edit.txt 2010-03-09 16:58:26 +0000
@@ -1,5 +1,8 @@
1The POTemplate edit page lets us to edit basic aspect of that object. Only1Editing POTemplates
2product owners, Rosetta Experts or a Launchpad admin are able to use it.2===================
3
4The POTemplate edit page allows editing a subset of potemplate attributes.
5Only product owners, Rosetta Experts or a Launchpad admin are able to use it.
36
4An unpriviledged user cannot reach this page.7An unpriviledged user cannot reach this page.
58
@@ -139,3 +142,31 @@
139 >>> previous_date_last_updated != evolution_template.date_last_updated142 >>> previous_date_last_updated != evolution_template.date_last_updated
140 True143 True
141144
145
146Change and cancel actions
147-------------------------
148
149After changing or canceling a form you will be redirected to the previous page
150from you navigation.
151
152The edit page can be access from the templates list.
153
154 >>> referrer = 'http://translations.launchpad.dev/evolution/trunk/+templates'
155 >>> admin_browser.open(referrer)
156 >>> admin_browser.getLink(
157 ... url='/evolution/trunk/+pots/'
158 ... 'evolution-2.2/+edit').click()
159 >>> admin_browser.getControl('Change').click()
160 >>> admin_browser.url == referrer
161 True
162
163If you are accesing the edit page using a bookmark (in this case there was
164no previous page in the navigation), you will be directed to the template
165index page.
166
167 >>> admin_browser.open(
168 ... 'http://translations.launchpad.dev/evolution/trunk/+pots/'
169 ... 'evolution-2.2/+edit')
170 >>> admin_browser.getLink('Cancel').click()
171 >>> print admin_browser.url
172 http://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2