Merge lp:~bac/launchpad/bug-422974-productrelease into lp:launchpad

[Brad Crittenden]

[Brad Crittenden (bac)]

= Summary =

Bug 422974 addresses updating productrelease templates to UI 3.0.

== Proposed fix ==

See the QA section.

== Tests ==

bin/test -vvm lp.registry -t productrelease

== Demo and Q/A ==

All tests need to be run as <email address hidden>

* productrelease-add-from-series.pt
  * https://launchpad.dev/firefox/1.0/+addrelease
  * Moved page_title to view (ProductReleaseAddViewBase).
  * Converted layout to main_only.
  * Added test for view.page_title

* productrelease-add.pt
  * https://launchpad.dev/firefox/+milestone/1.0/+addrelease
  * Moved page_title to view (ProductReleaseAddViewBase).
  * Converted layout to main_only.
  * Added test for view.label view.page_title

* productrelease-delete.pt
  * https://launchpad.dev/firefox/trunk/0.9.2/+delete
  * Moved page_title to view.
  * Converted layout to main_only.
  * Added new view tests

* productrelease-edit.pt
  * https://launchpad.dev/firefox/trunk/0.9.2/+edit
  * Moved page_title to view.
  * Converted layout to main_only.
  * Added test for view.label view.page_title

* productrelease-file-add.pt
  * https://launchpad.dev/firefox/trunk/0.9.2/+adddownloadfile
  * Converted layout to main_only.
  * Added new view tests

= 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/registry/browser/tests/productrelease-views.txt
lib/lp/registry/stories/productrelease/xx-productrelease-delete.txt
lib/canonical/launchpad/pagetitles.py
lib/lp/registry/templates/productrelease-add.pt
lib/lp/registry/templates/productrelease-edit.pt
lib/lp/registry/templates/productrelease-file-add.pt
lib/lp/registry/browser/productrelease.py
lib/lp/registry/templates/productrelease-add-from-series.pt
lib/lp/registry/templates/productrelease-delete.pt

== Pylint notices ==

lib/lp/registry/browser/productrelease.py
34: [F0401] Unable to import 'lazr.restful.interface' (No module named
restful)

Ugh, unsure why these are picked up. Ignore.

[Curtis Hovey (sinzui)]

Hi Brad.

Thanks for doing this. I am concerned that there is needless use of
cgi.escape. I really do not want us to fix these after users report bugs
about double-escaped title and headings.

> === modified file 'lib/lp/registry/browser/productrelease.py'
> --- lib/lp/registry/browser/productrelease.py 2009-07-17 00:26:05 +0000
> +++ lib/lp/registry/browser/productrelease.py 2009-09-03 22:35:47 +0000
> @@ -136,6 +136,11 @@
> self.context.product.displayname)
>
> @property
> + def page_title(self):
> + return ('Publish the release of %s' %
> + cgi.escape(self.context.displayname))

You did not need to do this in pagetitles.py and you do not need to do
think now. base-layout uses tal:content to ensure the title is escaped.

This will lead to double escaped entities and tags.

> @@ -232,7 +237,13 @@
> @property
> def label(self):
> """The form label."""
> - return 'Edit %s release details' % self.context.title
> + return ('Edit %s release details' %
> + cgi.escape(self.context.title))

This too is unnecessary. No form label every did this in 1.0 or 2.0,
and the same is true in 3.0. You only need to escape content that will
is used by a structured method: notifications and js widgets.

> + @property
> + def page_title(self):
> + return ('Edit details of %s in Launchpad' %
> + cgi.escape(self.context.title))

The 'in launchpad' make no sense. The argument for adding it is for google,
but bots cannot access any add, edit, destroy forms. As I said on the list,
if this rule is needed, base-layout should do it for us.

> @@ -284,6 +295,11 @@
> """The form label."""
> return 'Add a download file to %s' % self.context.title
>
> + @property
> + def page_title(self):
> + """The view page title."""
> + return ('Add a file to %s' % cgi.escape(self.context.displayname))

ditto.

> > @@ -354,7 +370,12 @@
> > @property
> > def label(self):
> > """The form label."""
> > - return 'Delete %s' % self.context.title
> > + return 'Delete %s' % cgi.escape(self.context.title)

ditto.

> + @property
> + def page_title(self):
> + """The view page title."""
> + return '%s in Launchpad' % self.label

ditto.

> === modified file 'lib/lp/registry/browser/tests/productrelease-views.txt'
> --- lib/lp/registry/browser/tests/productrelease-views.txt 2009-08-12 19:52:37 +0000
> +++ lib/lp/registry/browser/tests/productrelease-views.txt 2009-09-05 11:57:27 +0000

...

> @@ -160,3 +172,45 @@
> >>> print release.release_notes
> revised
>
> +
> +Adding a download file to a release
> +-----------------------------------
> +
> + >>> form = {
> + ... 'field.description': 'App 0.1 tarball',
> + ... 'field.contenttype': 'CODETARBALL',
> + ... 'field.actions.add': 'Upload',
> + ... }
> + >>> view = create_initialized_view(
> + ... release, '+adddownloadfile',
> + ... form=form)
> + >>> print view.label
> + Add a download file to App 0.1
> +
> + >>> print view.page_title
> + Add a file to App 0.1
> +
> +
> +...

review: Needs Fixing (code)

[Brad Crittenden (bac)]

On Sep 5, 2009, at 12:01 , Curtis Hovey wrote:

> Review: Needs Fixing code
> Hi Brad.
>
> Thanks for doing this. I am concerned that there is needless use of
> cgi.escape. I really do not want us to fix these after users report
> bugs
> about double-escaped title and headings.

Sorry I forgot this branch had the issue.

>
>
>> === modified file 'lib/lp/registry/browser/productrelease.py'
>> --- lib/lp/registry/browser/productrelease.py 2009-07-17 00:26:05
>> +0000
>> +++ lib/lp/registry/browser/productrelease.py 2009-09-03 22:35:47
>> +0000
>> @@ -136,6 +136,11 @@
>> self.context.product.displayname)
>>
>> @property
>> + def page_title(self):
>> + return ('Publish the release of %s' %
>> + cgi.escape(self.context.displayname))
>
> You did not need to do this in pagetitles.py and you do not need to do
> think now. base-layout uses tal:content to ensure the title is
> escaped.
>
> This will lead to double escaped entities and tags.
>

Right. Fixed.

>> @@ -232,7 +237,13 @@
>> @property
>> def label(self):
>> """The form label."""
>> - return 'Edit %s release details' % self.context.title
>> + return ('Edit %s release details' %
>> + cgi.escape(self.context.title))
>
> This too is unnecessary. No form label every did this in 1.0 or 2.0,
> and the same is true in 3.0. You only need to escape content that will
> is used by a structured method: notifications and js widgets.

Fixed

>
>> + @property
>> + def page_title(self):
>> + return ('Edit details of %s in Launchpad' %
>> + cgi.escape(self.context.title))
>
> The 'in launchpad' make no sense. The argument for adding it is for
> google,
> but bots cannot access any add, edit, destroy forms. As I said on
> the list,
> if this rule is needed, base-layout should do it for us.

Fixed with page_title = label

>
>> @@ -284,6 +295,11 @@
>> """The form label."""
>> return 'Add a download file to %s' % self.context.title
>>
>> + @property
>> + def page_title(self):
>> + """The view page title."""
>> + return ('Add a file to %s' % cgi.escape
>> (self.context.displayname))
>
> ditto.

page_title = label

>
>>> @@ -354,7 +370,12 @@
>>> @property
>>> def label(self):
>>> """The form label."""
>>> - return 'Delete %s' % self.context.title
>>> + return 'Delete %s' % cgi.escape(self.context.title)
>
> ditto.

done

>> + @property
>> + def page_title(self):
>> + """The view page title."""
>> + return '%s in Launchpad' % self.label
>
> ditto.
>
>> === modified file 'lib/lp/registry/browser/tests/productrelease-
>> views.txt'
>> --- lib/lp/registry/browser/tests/productrelease-views.txt
>> 2009-08-12 19:52:37 +0000
>> +++ lib/lp/registry/browser/tests/productrelease-views.txt
>> 2009-09-05 11:57:27 +0000
>
> ...
>
>> @@ -160,3 +172,45 @@
>>>>> print release.release_notes
>> revised
>>
>> +
>> +Adding a download file to a release
>> +-----------------------------------
>> +
>> + >>> form = {
>> + ... 'field.description': 'App 0.1 tarball',
>> + ... 'field.contenttype': 'CODETAR...

[Curtis Hovey (sinzui)]

The changes look good to land.

review: Approve (code)

[Brad Crittenden (bac)]

Screenshot are available at http://people.canonical.com/~bac/bug-422984-productrelease/

[Michael Nelson (michael.nelson)]

Thanks for doing all these bac! Looks great.

Nothing to comment on. If you were really keen you could possibly use smartquote to get pretty left-right quotes, but not necessary.

review: Approve (ui)

[Eleanor Berger (intellectronica)]

review: Approve (ui)

[Brad Crittenden (bac)]

Thanks for the reviews. Final diff at http://pastebin.ubuntu.com/266663/

=== modified file 'lib/canonical/launchpad/pagetitles.py'
--- lib/canonical/launchpad/pagetitles.py	2009-09-05 07:03:47 +0000
+++ lib/canonical/launchpad/pagetitles.py	2009-09-05 11:58:14 +0000
@@ -859,18 +859,8 @@
 
 product_translations = ContextTitle('Translations of %s in Launchpad')
 
-productrelease_add = ContextDisplayName('Publish the release of %s')
-
-productrelease_add_from_series = productrelease_add
-
-productrelease_delete = ContextTitle('Delete %s in Launchpad')
-
-productrelease_file_add = ContextDisplayName('Add a file to %s')
-
 productrelease_admin = ContextTitle('Administer %s in Launchpad')
 
-productrelease_edit = ContextDisplayName('Edit details of %s in Launchpad')
-
 productrelease_index = ContextDisplayName('%s in Launchpad')
 
 products_review_licenses = 'Review projects'

=== modified file 'lib/lp/registry/browser/productrelease.py'
--- lib/lp/registry/browser/productrelease.py	2009-07-17 00:26:05 +0000
+++ lib/lp/registry/browser/productrelease.py	2009-09-03 22:35:47 +0000
@@ -136,6 +136,11 @@
             self.context.product.displayname)
 
     @property
+    def page_title(self):
+        return ('Publish the release of %s' %
+                cgi.escape(self.context.displayname))
+
+    @property
     def cancel_url(self):
         return canonical_url(self.context)
 
@@ -232,7 +237,13 @@
     @property
     def label(self):
         """The form label."""
-        return 'Edit %s release details' % self.context.title
+        return ('Edit %s release details' %
+                cgi.escape(self.context.title))
+
+    @property
+    def page_title(self):
+        return ('Edit details of %s in Launchpad' %
+                cgi.escape(self.context.title))
 
     @action('Change', name='change')
     def change_action(self, action, data):
@@ -284,6 +295,11 @@
         """The form label."""
         return 'Add a download file to %s' % self.context.title
 
+    @property
+    def page_title(self):
+        """The view page title."""
+        return ('Add a file to %s' % cgi.escape(self.context.displayname))
+
     @action('Upload', name='add')
     def add_action(self, action, data):
         form = self.request.form
@@ -354,7 +370,12 @@
     @property
     def label(self):
         """The form label."""
-        return 'Delete %s' % self.context.title
+        return 'Delete %s' % cgi.escape(self.context.title)
+
+    @property
+    def page_title(self):
+        """The view page title."""
+        return '%s in Launchpad' % self.label
 
     @action('Delete this Release', name='delete')
     def delete_action(self, action, data):
@@ -368,4 +389,3 @@
     @property
     def cancel_url(self):
         return canonical_url(self.context)
-

=== modified file 'lib/lp/registry/browser/tests/productrelease-views.txt'
--- lib/lp/registry/browser/tests/productrelease-views.txt	2009-08-12 19:52:37 +0000
+++ lib/lp/registry/browser/tests/productrelease-views.txt	2009-09-05 11:57:27 +0000
@@ -23,6 +23,8 @@
     >>> view = create_initialized_view(milestone, '+addrelease')
     >>> print view.label
     Create a new release for App
+    >>> print view.page_title
+    Publish the release of App 0.1
 
 The view creates a checkbox to allow the user to keep the milestone
 active, which is normally deactivated when a release is made.
@@ -115,6 +117,10 @@
     >>> [field.__name__ for field in view.form_fields]
     ['milestone_for_release', 'keep_milestone_active', 'datereleased',
      'release_notes', 'changelog']
+    >>> print view.label
+    Create a new release for App
+    >>> print view.page_title
+    Publish the release of simple
 
 The rendered template includes a script that adds a js-action link to
 show a formoverlay that updates the milestone_for_release field.
@@ -151,6 +157,12 @@
     ...     'field.actions.change': 'Change',
     ...     }
     >>> view = create_initialized_view(release, '+edit', form=form)
+    >>> print view.label
+    Edit App 0.1 release details
+
+    >>> print view.page_title
+    Edit details of App 0.1 in Launchpad
+
     >>> print view.errors
     []
 
@@ -160,3 +172,45 @@
     >>> print release.release_notes
     revised
 
+
+Adding a download file to a release
+-----------------------------------
+
+    >>> form = {
+    ...     'field.description': 'App 0.1 tarball',
+    ...     'field.contenttype': 'CODETARBALL',
+    ...     'field.actions.add': 'Upload',
+    ...     }
+    >>> view = create_initialized_view(
+    ...     release, '+adddownloadfile',
+    ...     form=form)
+    >>> print view.label
+    Add a download file to App 0.1
+
+    >>> print view.page_title
+    Add a file to App 0.1
+
+
+
+Deleting a product release
+--------------------------
+
+    >>> form = {
+    ...     'field.actions.delete': 'Delete this release',
+    ...     }
+    >>> view = create_initialized_view(release, '+delete', form=form)
+    >>> print view.label
+    Delete App 0.1
+
+    >>> print view.page_title
+    Delete App 0.1 in Launchpad
+
+    >>> print view.errors
+    []
+
+    >>> print view.widget_errors
+    {}
+
+    >>> release = series.getRelease('0.1')
+    >>> print release
+    None

=== modified file 'lib/lp/registry/stories/productrelease/xx-productrelease-delete.txt'
--- lib/lp/registry/stories/productrelease/xx-productrelease-delete.txt	2009-07-10 04:34:11 +0000
+++ lib/lp/registry/stories/productrelease/xx-productrelease-delete.txt	2009-09-05 12:23:00 +0000
@@ -40,9 +40,12 @@
 they will be deleted too.
 
     >>> print extract_text(find_main_content(salgados_browser.contents))
-    Delete Mozilla Firefox 0.9.2 "One (secure) Tree Hill" ...
+    Mozilla Firefox...
+    Delete Mozilla Firefox 0.9.2 "One (secure) Tree Hill"...
+    Are you sure you want to delete the 0.9.2 release of
+    Mozilla Firefox trunk series?
     The following files must be deleted:
-    firefox-0.9.2.orig.tar.gz ...
+    firefox-0.9.2.orig.tar.gz...
 
 Salgado chooses the delete button, then reads that the action is successful.
 
@@ -60,5 +63,3 @@
     ...     find_tag_by_id(salgados_browser.contents, 'series_trunk'))
     Version "Codename"        Expected              Released      Summary
     Mozilla Firefox 0.9.2...  Set  Change details   This is an inactive ...
-
-

=== modified file 'lib/lp/registry/templates/productrelease-add-from-series.pt'
--- lib/lp/registry/templates/productrelease-add-from-series.pt	2009-07-21 17:29:12 +0000
+++ lib/lp/registry/templates/productrelease-add-from-series.pt	2009-09-03 22:35:47 +0000
@@ -3,10 +3,7 @@
   xmlns:tal="http://xml.zope.org/namespaces/tal"
   xmlns:metal="http://xml.zope.org/namespaces/metal"
   xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  xml:lang="en"
-  lang="en"
-  dir="ltr"
-  metal:use-macro="view/macro:page/onecolumn"
+  metal:use-macro="view/macro:page/main_only"
   i18n:domain="launchpad"
 >
 <body>
@@ -86,4 +83,3 @@
 
 </body>
 </html>
-

=== modified file 'lib/lp/registry/templates/productrelease-add.pt'
--- lib/lp/registry/templates/productrelease-add.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/registry/templates/productrelease-add.pt	2009-09-03 22:35:47 +0000
@@ -3,10 +3,7 @@
   xmlns:tal="http://xml.zope.org/namespaces/tal"
   xmlns:metal="http://xml.zope.org/namespaces/metal"
   xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  xml:lang="en"
-  lang="en"
-  dir="ltr"
-  metal:use-macro="view/macro:page/onecolumn"
+  metal:use-macro="view/macro:page/main_only"
   i18n:domain="launchpad"
 >
 

=== modified file 'lib/lp/registry/templates/productrelease-delete.pt'
--- lib/lp/registry/templates/productrelease-delete.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/registry/templates/productrelease-delete.pt	2009-09-03 22:35:47 +0000
@@ -3,10 +3,7 @@
   xmlns:tal="http://xml.zope.org/namespaces/tal"
   xmlns:metal="http://xml.zope.org/namespaces/metal"
   xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  xml:lang="en"
-  lang="en"
-  dir="ltr"
-  metal:use-macro="view/macro:page/onecolumn"
+  metal:use-macro="view/macro:page/main_only"
   i18n:domain="launchpad"
 >
 

=== modified file 'lib/lp/registry/templates/productrelease-edit.pt'
--- lib/lp/registry/templates/productrelease-edit.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/registry/templates/productrelease-edit.pt	2009-09-03 22:35:47 +0000
@@ -3,10 +3,7 @@
   xmlns:tal="http://xml.zope.org/namespaces/tal"
   xmlns:metal="http://xml.zope.org/namespaces/metal"
   xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  xml:lang="en"
-  lang="en"
-  dir="ltr"
-  metal:use-macro="view/macro:page/onecolumn"
+  metal:use-macro="view/macro:page/main_only"
   i18n:domain="launchpad"
 >
 

=== modified file 'lib/lp/registry/templates/productrelease-file-add.pt'
--- lib/lp/registry/templates/productrelease-file-add.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/registry/templates/productrelease-file-add.pt	2009-09-03 22:35:47 +0000
@@ -3,10 +3,7 @@
   xmlns:tal="http://xml.zope.org/namespaces/tal"
   xmlns:metal="http://xml.zope.org/namespaces/metal"
   xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  xml:lang="en"
-  lang="en"
-  dir="ltr"
-  metal:use-macro="view/macro:page/onecolumn"
+  metal:use-macro="view/macro:page/main_only"
   i18n:domain="launchpad">
 
 <body>