Merge lp:~bac/launchpad/bug-422974-productrelease into lp:launchpad
- bug-422974-productrelease
- Merge into devel
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Eleanor Berger | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | not available | ||||
Proposed branch: | lp:~bac/launchpad/bug-422974-productrelease | ||||
Merge into: | lp:launchpad | ||||
Diff against target: | None lines | ||||
To merge this branch: | bzr merge lp:~bac/launchpad/bug-422974-productrelease | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Eleanor Berger (community) | ui | Approve | |
Michael Nelson (community) | ui | Approve | |
Curtis Hovey (community) | code | Approve | |
Canonical Launchpad Engineering | Pending | ||
Review via email:
|
Commit message
Description of the change
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Brad Crittenden (bac) wrote : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Curtis Hovey (sinzui) wrote : | # |
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/
> --- lib/lp/
> +++ lib/lp/
> @@ -136,6 +136,11 @@
> self.context.
>
> @property
> + def page_title(self):
> + return ('Publish the release of %s' %
> + cgi.escape(
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(
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(
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(
ditto.
> > @@ -354,7 +370,12 @@
> > @property
> > def label(self):
> > """The form label."""
> > - return 'Delete %s' % self.context.title
> > + return 'Delete %s' % cgi.escape(
ditto.
> + @property
> + def page_title(self):
> + """The view page title."""
> + return '%s in Launchpad' % self.label
ditto.
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
...
> @@ -160,3 +172,45 @@
> >>> print release.
> revised
>
> +
> +Adding a download file to a release
> +------
> +
> + >>> form = {
> + ... 'field.
> + ... 'field.
> + ... 'field.
> + ... }
> + >>> view = create_
> + ... 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
> +
> +
> +...
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Brad Crittenden (bac) wrote : | # |
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/
>> --- lib/lp/
>> +0000
>> +++ lib/lp/
>> +0000
>> @@ -136,6 +136,11 @@
>> self.context.
>>
>> @property
>> + def page_title(self):
>> + return ('Publish the release of %s' %
>> + cgi.escape(
>
> 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(
>
> 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(
>
> 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.
>
> 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(
>
> ditto.
done
>> + @property
>> + def page_title(self):
>> + """The view page title."""
>> + return '%s in Launchpad' % self.label
>
> ditto.
>
>> === modified file 'lib/lp/
>> views.txt'
>> --- lib/lp/
>> 2009-08-12 19:52:37 +0000
>> +++ lib/lp/
>> 2009-09-05 11:57:27 +0000
>
> ...
>
>> @@ -160,3 +172,45 @@
>>>>> print release.
>> revised
>>
>> +
>> +Adding a download file to a release
>> +------
>> +
>> + >>> form = {
>> + ... 'field.
>> + ... 'field.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Curtis Hovey (sinzui) wrote : | # |
The changes look good to land.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Brad Crittenden (bac) wrote : | # |
Screenshot are available at http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Michael Nelson (michael.nelson) wrote : | # |
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Eleanor Berger (intellectronica) : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Brad Crittenden (bac) wrote : | # |
Thanks for the reviews. Final diff at http://
Preview Diff
1 | === modified file 'lib/canonical/launchpad/pagetitles.py' |
2 | --- lib/canonical/launchpad/pagetitles.py 2009-09-05 07:03:47 +0000 |
3 | +++ lib/canonical/launchpad/pagetitles.py 2009-09-05 11:58:14 +0000 |
4 | @@ -859,18 +859,8 @@ |
5 | |
6 | product_translations = ContextTitle('Translations of %s in Launchpad') |
7 | |
8 | -productrelease_add = ContextDisplayName('Publish the release of %s') |
9 | - |
10 | -productrelease_add_from_series = productrelease_add |
11 | - |
12 | -productrelease_delete = ContextTitle('Delete %s in Launchpad') |
13 | - |
14 | -productrelease_file_add = ContextDisplayName('Add a file to %s') |
15 | - |
16 | productrelease_admin = ContextTitle('Administer %s in Launchpad') |
17 | |
18 | -productrelease_edit = ContextDisplayName('Edit details of %s in Launchpad') |
19 | - |
20 | productrelease_index = ContextDisplayName('%s in Launchpad') |
21 | |
22 | products_review_licenses = 'Review projects' |
23 | |
24 | === modified file 'lib/lp/registry/browser/productrelease.py' |
25 | --- lib/lp/registry/browser/productrelease.py 2009-07-17 00:26:05 +0000 |
26 | +++ lib/lp/registry/browser/productrelease.py 2009-09-03 22:35:47 +0000 |
27 | @@ -136,6 +136,11 @@ |
28 | self.context.product.displayname) |
29 | |
30 | @property |
31 | + def page_title(self): |
32 | + return ('Publish the release of %s' % |
33 | + cgi.escape(self.context.displayname)) |
34 | + |
35 | + @property |
36 | def cancel_url(self): |
37 | return canonical_url(self.context) |
38 | |
39 | @@ -232,7 +237,13 @@ |
40 | @property |
41 | def label(self): |
42 | """The form label.""" |
43 | - return 'Edit %s release details' % self.context.title |
44 | + return ('Edit %s release details' % |
45 | + cgi.escape(self.context.title)) |
46 | + |
47 | + @property |
48 | + def page_title(self): |
49 | + return ('Edit details of %s in Launchpad' % |
50 | + cgi.escape(self.context.title)) |
51 | |
52 | @action('Change', name='change') |
53 | def change_action(self, action, data): |
54 | @@ -284,6 +295,11 @@ |
55 | """The form label.""" |
56 | return 'Add a download file to %s' % self.context.title |
57 | |
58 | + @property |
59 | + def page_title(self): |
60 | + """The view page title.""" |
61 | + return ('Add a file to %s' % cgi.escape(self.context.displayname)) |
62 | + |
63 | @action('Upload', name='add') |
64 | def add_action(self, action, data): |
65 | form = self.request.form |
66 | @@ -354,7 +370,12 @@ |
67 | @property |
68 | def label(self): |
69 | """The form label.""" |
70 | - return 'Delete %s' % self.context.title |
71 | + return 'Delete %s' % cgi.escape(self.context.title) |
72 | + |
73 | + @property |
74 | + def page_title(self): |
75 | + """The view page title.""" |
76 | + return '%s in Launchpad' % self.label |
77 | |
78 | @action('Delete this Release', name='delete') |
79 | def delete_action(self, action, data): |
80 | @@ -368,4 +389,3 @@ |
81 | @property |
82 | def cancel_url(self): |
83 | return canonical_url(self.context) |
84 | - |
85 | |
86 | === modified file 'lib/lp/registry/browser/tests/productrelease-views.txt' |
87 | --- lib/lp/registry/browser/tests/productrelease-views.txt 2009-08-12 19:52:37 +0000 |
88 | +++ lib/lp/registry/browser/tests/productrelease-views.txt 2009-09-05 11:57:27 +0000 |
89 | @@ -23,6 +23,8 @@ |
90 | >>> view = create_initialized_view(milestone, '+addrelease') |
91 | >>> print view.label |
92 | Create a new release for App |
93 | + >>> print view.page_title |
94 | + Publish the release of App 0.1 |
95 | |
96 | The view creates a checkbox to allow the user to keep the milestone |
97 | active, which is normally deactivated when a release is made. |
98 | @@ -115,6 +117,10 @@ |
99 | >>> [field.__name__ for field in view.form_fields] |
100 | ['milestone_for_release', 'keep_milestone_active', 'datereleased', |
101 | 'release_notes', 'changelog'] |
102 | + >>> print view.label |
103 | + Create a new release for App |
104 | + >>> print view.page_title |
105 | + Publish the release of simple |
106 | |
107 | The rendered template includes a script that adds a js-action link to |
108 | show a formoverlay that updates the milestone_for_release field. |
109 | @@ -151,6 +157,12 @@ |
110 | ... 'field.actions.change': 'Change', |
111 | ... } |
112 | >>> view = create_initialized_view(release, '+edit', form=form) |
113 | + >>> print view.label |
114 | + Edit App 0.1 release details |
115 | + |
116 | + >>> print view.page_title |
117 | + Edit details of App 0.1 in Launchpad |
118 | + |
119 | >>> print view.errors |
120 | [] |
121 | |
122 | @@ -160,3 +172,45 @@ |
123 | >>> print release.release_notes |
124 | revised |
125 | |
126 | + |
127 | +Adding a download file to a release |
128 | +----------------------------------- |
129 | + |
130 | + >>> form = { |
131 | + ... 'field.description': 'App 0.1 tarball', |
132 | + ... 'field.contenttype': 'CODETARBALL', |
133 | + ... 'field.actions.add': 'Upload', |
134 | + ... } |
135 | + >>> view = create_initialized_view( |
136 | + ... release, '+adddownloadfile', |
137 | + ... form=form) |
138 | + >>> print view.label |
139 | + Add a download file to App 0.1 |
140 | + |
141 | + >>> print view.page_title |
142 | + Add a file to App 0.1 |
143 | + |
144 | + |
145 | + |
146 | +Deleting a product release |
147 | +-------------------------- |
148 | + |
149 | + >>> form = { |
150 | + ... 'field.actions.delete': 'Delete this release', |
151 | + ... } |
152 | + >>> view = create_initialized_view(release, '+delete', form=form) |
153 | + >>> print view.label |
154 | + Delete App 0.1 |
155 | + |
156 | + >>> print view.page_title |
157 | + Delete App 0.1 in Launchpad |
158 | + |
159 | + >>> print view.errors |
160 | + [] |
161 | + |
162 | + >>> print view.widget_errors |
163 | + {} |
164 | + |
165 | + >>> release = series.getRelease('0.1') |
166 | + >>> print release |
167 | + None |
168 | |
169 | === modified file 'lib/lp/registry/stories/productrelease/xx-productrelease-delete.txt' |
170 | --- lib/lp/registry/stories/productrelease/xx-productrelease-delete.txt 2009-07-10 04:34:11 +0000 |
171 | +++ lib/lp/registry/stories/productrelease/xx-productrelease-delete.txt 2009-09-05 12:23:00 +0000 |
172 | @@ -40,9 +40,12 @@ |
173 | they will be deleted too. |
174 | |
175 | >>> print extract_text(find_main_content(salgados_browser.contents)) |
176 | - Delete Mozilla Firefox 0.9.2 "One (secure) Tree Hill" ... |
177 | + Mozilla Firefox... |
178 | + Delete Mozilla Firefox 0.9.2 "One (secure) Tree Hill"... |
179 | + Are you sure you want to delete the 0.9.2 release of |
180 | + Mozilla Firefox trunk series? |
181 | The following files must be deleted: |
182 | - firefox-0.9.2.orig.tar.gz ... |
183 | + firefox-0.9.2.orig.tar.gz... |
184 | |
185 | Salgado chooses the delete button, then reads that the action is successful. |
186 | |
187 | @@ -60,5 +63,3 @@ |
188 | ... find_tag_by_id(salgados_browser.contents, 'series_trunk')) |
189 | Version "Codename" Expected Released Summary |
190 | Mozilla Firefox 0.9.2... Set Change details This is an inactive ... |
191 | - |
192 | - |
193 | |
194 | === modified file 'lib/lp/registry/templates/productrelease-add-from-series.pt' |
195 | --- lib/lp/registry/templates/productrelease-add-from-series.pt 2009-07-21 17:29:12 +0000 |
196 | +++ lib/lp/registry/templates/productrelease-add-from-series.pt 2009-09-03 22:35:47 +0000 |
197 | @@ -3,10 +3,7 @@ |
198 | xmlns:tal="http://xml.zope.org/namespaces/tal" |
199 | xmlns:metal="http://xml.zope.org/namespaces/metal" |
200 | xmlns:i18n="http://xml.zope.org/namespaces/i18n" |
201 | - xml:lang="en" |
202 | - lang="en" |
203 | - dir="ltr" |
204 | - metal:use-macro="view/macro:page/onecolumn" |
205 | + metal:use-macro="view/macro:page/main_only" |
206 | i18n:domain="launchpad" |
207 | > |
208 | <body> |
209 | @@ -86,4 +83,3 @@ |
210 | |
211 | </body> |
212 | </html> |
213 | - |
214 | |
215 | === modified file 'lib/lp/registry/templates/productrelease-add.pt' |
216 | --- lib/lp/registry/templates/productrelease-add.pt 2009-07-17 17:59:07 +0000 |
217 | +++ lib/lp/registry/templates/productrelease-add.pt 2009-09-03 22:35:47 +0000 |
218 | @@ -3,10 +3,7 @@ |
219 | xmlns:tal="http://xml.zope.org/namespaces/tal" |
220 | xmlns:metal="http://xml.zope.org/namespaces/metal" |
221 | xmlns:i18n="http://xml.zope.org/namespaces/i18n" |
222 | - xml:lang="en" |
223 | - lang="en" |
224 | - dir="ltr" |
225 | - metal:use-macro="view/macro:page/onecolumn" |
226 | + metal:use-macro="view/macro:page/main_only" |
227 | i18n:domain="launchpad" |
228 | > |
229 | |
230 | |
231 | === modified file 'lib/lp/registry/templates/productrelease-delete.pt' |
232 | --- lib/lp/registry/templates/productrelease-delete.pt 2009-07-17 17:59:07 +0000 |
233 | +++ lib/lp/registry/templates/productrelease-delete.pt 2009-09-03 22:35:47 +0000 |
234 | @@ -3,10 +3,7 @@ |
235 | xmlns:tal="http://xml.zope.org/namespaces/tal" |
236 | xmlns:metal="http://xml.zope.org/namespaces/metal" |
237 | xmlns:i18n="http://xml.zope.org/namespaces/i18n" |
238 | - xml:lang="en" |
239 | - lang="en" |
240 | - dir="ltr" |
241 | - metal:use-macro="view/macro:page/onecolumn" |
242 | + metal:use-macro="view/macro:page/main_only" |
243 | i18n:domain="launchpad" |
244 | > |
245 | |
246 | |
247 | === modified file 'lib/lp/registry/templates/productrelease-edit.pt' |
248 | --- lib/lp/registry/templates/productrelease-edit.pt 2009-07-17 17:59:07 +0000 |
249 | +++ lib/lp/registry/templates/productrelease-edit.pt 2009-09-03 22:35:47 +0000 |
250 | @@ -3,10 +3,7 @@ |
251 | xmlns:tal="http://xml.zope.org/namespaces/tal" |
252 | xmlns:metal="http://xml.zope.org/namespaces/metal" |
253 | xmlns:i18n="http://xml.zope.org/namespaces/i18n" |
254 | - xml:lang="en" |
255 | - lang="en" |
256 | - dir="ltr" |
257 | - metal:use-macro="view/macro:page/onecolumn" |
258 | + metal:use-macro="view/macro:page/main_only" |
259 | i18n:domain="launchpad" |
260 | > |
261 | |
262 | |
263 | === modified file 'lib/lp/registry/templates/productrelease-file-add.pt' |
264 | --- lib/lp/registry/templates/productrelease-file-add.pt 2009-07-17 17:59:07 +0000 |
265 | +++ lib/lp/registry/templates/productrelease-file-add.pt 2009-09-03 22:35:47 +0000 |
266 | @@ -3,10 +3,7 @@ |
267 | xmlns:tal="http://xml.zope.org/namespaces/tal" |
268 | xmlns:metal="http://xml.zope.org/namespaces/metal" |
269 | xmlns:i18n="http://xml.zope.org/namespaces/i18n" |
270 | - xml:lang="en" |
271 | - lang="en" |
272 | - dir="ltr" |
273 | - metal:use-macro="view/macro:page/onecolumn" |
274 | + metal:use-macro="view/macro:page/main_only" |
275 | i18n:domain="launchpad"> |
276 | |
277 | <body> |
= 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 /launchpad. dev/firefox/ 1.0/+addrelease AddViewBase) .
* https:/
* Moved page_title to view (ProductRelease
* Converted layout to main_only.
* Added test for view.page_title
* productrelease- add.pt /launchpad. dev/firefox/ +milestone/ 1.0/+addrelease AddViewBase) .
* https:/
* Moved page_title to view (ProductRelease
* Converted layout to main_only.
* Added test for view.label view.page_title
* productrelease- delete. pt /launchpad. dev/firefox/ trunk/0. 9.2/+delete
* https:/
* Moved page_title to view.
* Converted layout to main_only.
* Added new view tests
* productrelease- edit.pt /launchpad. dev/firefox/ trunk/0. 9.2/+edit
* https:/
* Moved page_title to view.
* Converted layout to main_only.
* Added test for view.label view.page_title
* productrelease- file-add. pt /launchpad. dev/firefox/ trunk/0. 9.2/+adddownloa dfile
* https:/
* 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: registry/ browser/ tests/productre lease-views. txt registry/ stories/ productrelease/ xx-productrelea se-delete. txt launchpad/ pagetitles. py registry/ templates/ productrelease- add.pt registry/ templates/ productrelease- edit.pt registry/ templates/ productrelease- file-add. pt registry/ browser/ productrelease. py registry/ templates/ productrelease- add-from- series. pt registry/ templates/ productrelease- delete. pt
lib/lp/
lib/lp/
lib/canonical/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
== Pylint notices ==
lib/lp/ registry/ browser/ productrelease. py interface' (No module named
34: [F0401] Unable to import 'lazr.restful.
restful)
Ugh, unsure why these are picked up. Ignore.