Merge lp:~michael.nelson/launchpad/429263-no-value-option into lp:launchpad
- 429263-no-value-option
- Merge into devel
Proposed by
Michael Nelson
Status: | Merged |
---|---|
Approved by: | Graham Binns |
Approved revision: | no longer in the source branch. |
Merged at revision: | not available |
Proposed branch: | lp:~michael.nelson/launchpad/429263-no-value-option |
Merge into: | lp:launchpad |
Diff against target: | None lines |
To merge this branch: | bzr merge lp:~michael.nelson/launchpad/429263-no-value-option |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Graham Binns (community) | Approve | ||
Review via email: mp+11768@code.launchpad.net |
Commit message
Description of the change
To post a comment you must log in.
Revision history for this message
Michael Nelson (michael.nelson) wrote : | # |
Revision history for this message
Graham Binns (gmb) : | # |
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'lib/lp/soyuz/browser/archive.py' |
2 | --- lib/lp/soyuz/browser/archive.py 2009-09-11 17:55:23 +0000 |
3 | +++ lib/lp/soyuz/browser/archive.py 2009-09-15 08:25:32 +0000 |
4 | @@ -577,7 +577,7 @@ |
5 | this factory can only be used in a class where the context is |
6 | an IArchive. |
7 | """ |
8 | - series_terms = [SimpleTerm(None, token='any', title='Any Series')] |
9 | + series_terms = [] |
10 | for distroseries in context.series_with_sources: |
11 | series_terms.append( |
12 | SimpleTerm(distroseries, token=distroseries.name, |
13 | @@ -585,6 +585,16 @@ |
14 | return SimpleVocabulary(series_terms) |
15 | |
16 | |
17 | +class SeriesFilterWidget(LaunchpadDropdownWidget): |
18 | + """Redefining default display value as 'Any series'.""" |
19 | + _messageNoValue = _("any", "Any series") |
20 | + |
21 | + |
22 | +class StatusFilterWidget(LaunchpadDropdownWidget): |
23 | + """Redefining default display value as 'Any status'.""" |
24 | + _messageNoValue = _("any", "Any status") |
25 | + |
26 | + |
27 | class IPPAPackageFilter(Interface): |
28 | """The interface used as the schema for the package filtering form.""" |
29 | name_filter = TextLine( |
30 | @@ -596,7 +606,6 @@ |
31 | status_filter = Choice(vocabulary=SimpleVocabulary(( |
32 | SimpleTerm(active_publishing_status, 'published', 'Published'), |
33 | SimpleTerm(inactive_publishing_status, 'superseded', 'Superseded'), |
34 | - SimpleTerm(None, 'any', 'Any status') |
35 | )), required=False) |
36 | |
37 | |
38 | @@ -604,6 +613,8 @@ |
39 | """A Form view for filtering and batching source packages.""" |
40 | |
41 | schema = IPPAPackageFilter |
42 | + custom_widget('series_filter', SeriesFilterWidget) |
43 | + custom_widget('status_filter', StatusFilterWidget) |
44 | |
45 | # By default this view will not display the sources with selectable |
46 | # checkboxes, but subclasses can override as needed. |
47 | @@ -625,65 +636,52 @@ |
48 | else: |
49 | return None |
50 | |
51 | - @cachedproperty |
52 | - def selected_status_filter(self): |
53 | - """Return the selected status filter or the default.""" |
54 | - requested_status_filter = self.request.query_string_params.get( |
55 | - 'field.status_filter') |
56 | - |
57 | - # If the request included a status filter, try to use it: |
58 | - selected_status_filter = None |
59 | - if requested_status_filter is not None: |
60 | - selected_status_filter = ( |
61 | - self.widgets['status_filter'].vocabulary.getTermByToken( |
62 | - requested_status_filter[0])) |
63 | - |
64 | - # If the request didn't include a status, or it was invalid, use |
65 | - # the default: |
66 | - if selected_status_filter is None: |
67 | - selected_status_filter = self.default_status_filter |
68 | - |
69 | - return selected_status_filter |
70 | + def getSelectedFilterValue(self, filter_name): |
71 | + """Return the selected filter or the default, given a filter name. |
72 | + |
73 | + This is needed because zope's form library does not consider |
74 | + query string params (GET params) during a post request. |
75 | + """ |
76 | + field_name = 'field.' + filter_name |
77 | + requested_filter = self.request.query_string_params.get(field_name) |
78 | + |
79 | + # If an empty filter was specified, then it's explicitly |
80 | + # been set to empty - so we use None. |
81 | + if requested_filter == ['']: |
82 | + return None |
83 | + |
84 | + # If the requested filter is none, then we use the default. |
85 | + default_filter_attr = 'default_' + filter_name |
86 | + if requested_filter is None: |
87 | + return getattr(self, default_filter_attr) |
88 | + |
89 | + # If the request included a filter, try to use it - if it's |
90 | + # invalid we use the default instead. |
91 | + vocab = self.widgets[filter_name].vocabulary |
92 | + if vocab.by_token.has_key(requested_filter[0]): |
93 | + return vocab.getTermByToken(requested_filter[0]).value |
94 | + else: |
95 | + return getattr(self, default_filter_attr) |
96 | |
97 | @property |
98 | def plain_status_filter_widget(self): |
99 | """Render a <select> control with no <div>s around it.""" |
100 | return self.widgets['status_filter'].renderValue( |
101 | - self.selected_status_filter.value) |
102 | - |
103 | - @property |
104 | - def selected_series_filter(self): |
105 | - """Return the currently selected filter or None.""" |
106 | - requested_series_filter = self.request.query_string_params.get( |
107 | - 'field.series_filter') |
108 | - |
109 | - # If the request included a series filter, try to use it: |
110 | - selected_series_filter = None |
111 | - if requested_series_filter is not None: |
112 | - series_vocabulary = self.widgets['series_filter'].vocabulary |
113 | - selected_series_filter = series_vocabulary.getTermByToken( |
114 | - requested_series_filter[0]) |
115 | - |
116 | - # If the request didn't include a series, or it was invalid, use |
117 | - # the default: |
118 | - if selected_series_filter is None: |
119 | - selected_series_filter = self.default_series_filter |
120 | - |
121 | - return selected_series_filter |
122 | + self.getSelectedFilterValue('status_filter')) |
123 | |
124 | @property |
125 | def plain_series_filter_widget(self): |
126 | """Render a <select> control with no <div>s around it.""" |
127 | return self.widgets['series_filter'].renderValue( |
128 | - self.selected_series_filter.value) |
129 | + self.getSelectedFilterValue('series_filter')) |
130 | |
131 | @property |
132 | def filtered_sources(self): |
133 | """Return the source results for display after filtering.""" |
134 | return self.context.getPublishedSources( |
135 | name=self.specified_name_filter, |
136 | - status=self.selected_status_filter.value, |
137 | - distroseries=self.selected_series_filter.value) |
138 | + status=self.getSelectedFilterValue('status_filter'), |
139 | + distroseries=self.getSelectedFilterValue('series_filter')) |
140 | |
141 | @property |
142 | def default_status_filter(self): |
143 | @@ -692,7 +690,7 @@ |
144 | Subclasses of ArchiveViewBase can override this when required. |
145 | """ |
146 | return self.widgets['status_filter'].vocabulary.getTermByToken( |
147 | - 'published') |
148 | + 'published').value |
149 | |
150 | @property |
151 | def default_series_filter(self): |
152 | @@ -700,7 +698,7 @@ |
153 | |
154 | Subclasses of ArchiveViewBase can override this when required. |
155 | """ |
156 | - return self.widgets['series_filter'].vocabulary.getTermByToken('any') |
157 | + return None |
158 | |
159 | @cachedproperty |
160 | def batchnav(self): |
161 | @@ -769,10 +767,10 @@ |
162 | for term in vocabulary: |
163 | if (term.value is not None and |
164 | term.value.version == version_number): |
165 | - return term |
166 | + return term.value |
167 | |
168 | # Otherwise we default to 'any' |
169 | - return vocabulary.getTermByToken('any') |
170 | + return None |
171 | |
172 | @property |
173 | def archive_description_html(self): |
174 | @@ -990,7 +988,7 @@ |
175 | @property |
176 | def default_status_filter(self): |
177 | """Present records in any status by default.""" |
178 | - return self.widgets['status_filter'].vocabulary.getTermByToken('any') |
179 | + return None |
180 | |
181 | @cachedproperty |
182 | def filtered_sources(self): |
183 | @@ -998,14 +996,11 @@ |
184 | |
185 | This overrides ArchiveViewBase.filtered_sources to use a |
186 | different method on the context specific to deletion records. |
187 | - |
188 | - It expects 'self.selected_status_filter' and |
189 | - 'self.selected_series_filter' to be set. |
190 | """ |
191 | return self.context.getSourcesForDeletion( |
192 | name=self.specified_name_filter, |
193 | - status=self.selected_status_filter.value, |
194 | - distroseries=self.selected_series_filter.value) |
195 | + status=self.getSelectedFilterValue('status_filter'), |
196 | + distroseries=self.getSelectedFilterValue('series_filter')) |
197 | |
198 | @cachedproperty |
199 | def has_sources(self): |
200 | @@ -1093,7 +1088,7 @@ |
201 | def default_status_filter(self): |
202 | """Present published records by default.""" |
203 | return self.widgets['status_filter'].vocabulary.getTermByToken( |
204 | - 'published') |
205 | + 'published').value |
206 | |
207 | def setUpFields(self): |
208 | """Override `ArchiveSourceSelectionFormView`. |
209 | |
210 | === modified file 'lib/lp/soyuz/browser/tests/archive-views.txt' |
211 | --- lib/lp/soyuz/browser/tests/archive-views.txt 2009-09-11 17:55:23 +0000 |
212 | +++ lib/lp/soyuz/browser/tests/archive-views.txt 2009-09-15 08:25:32 +0000 |
213 | @@ -90,14 +90,12 @@ |
214 | ... print term.title |
215 | Published |
216 | Superseded |
217 | - Any status |
218 | |
219 | An ArchiveView inherits the series-filter widget for filtering packages |
220 | by series. |
221 | |
222 | >>> for term in ppa_archive_view.widgets['series_filter'].vocabulary: |
223 | ... print term.title |
224 | - Any Series |
225 | Breezy Badger Autotest |
226 | Warty |
227 | |
228 | @@ -406,8 +404,8 @@ |
229 | The archive index view overrides the default series filter to use the |
230 | distroseries from the browser's user-agent, when applicable. |
231 | |
232 | - >>> print view.default_series_filter.token |
233 | - any |
234 | + >>> print view.default_series_filter |
235 | + None |
236 | |
237 | >>> view_warty = create_view( |
238 | ... cprov.archive, name="+index", |
239 | @@ -416,8 +414,19 @@ |
240 | ... 'Gecko/2009042523 Ubuntu/4.10 (whatever) ' |
241 | ... 'Firefox/3.0.10') |
242 | >>> view_warty.initialize() |
243 | - >>> print view_warty.default_series_filter.token |
244 | - warty |
245 | + >>> print view_warty.default_series_filter.name |
246 | + warty |
247 | + |
248 | +The archive index view also inherits the getSelectedFilterValue() method |
249 | +which can be used to find the currently selected value for both filters. |
250 | + |
251 | + >>> print view_warty.getSelectedFilterValue('series_filter').name |
252 | + warty |
253 | + |
254 | + >>> for status in view_warty.getSelectedFilterValue('status_filter'): |
255 | + ... print status.name |
256 | + PENDING |
257 | + PUBLISHED |
258 | |
259 | |
260 | == ArchivePackageView == |
261 | @@ -558,7 +567,6 @@ |
262 | ... form={ |
263 | ... 'field.actions.delete': 'Delete Packages', |
264 | ... 'field.name_filter': '', |
265 | - ... 'field.status_filter': 'any', |
266 | ... 'field.deletion_comment': 'Go away', |
267 | ... 'field.selected_sources': ['27', '28', '29'], |
268 | ... 'field.selected_sources-empty-marker': 1, |
269 | @@ -579,7 +587,6 @@ |
270 | ... form={ |
271 | ... 'field.actions.delete': 'Delete Packages', |
272 | ... 'field.name_filter': '', |
273 | - ... 'field.status_filter': 'any', |
274 | ... 'field.deletion_comment': 'Go away', |
275 | ... 'field.selected_sources': ['27', '28', '29'], |
276 | ... 'field.selected_sources-empty-marker': 1, |
277 | @@ -1009,7 +1016,7 @@ |
278 | |
279 | >>> view = create_initialized_view( |
280 | ... cprov.archive, name="+copy-packages", |
281 | - ... query_string="field.status_filter=any") |
282 | + ... query_string="field.status_filter=") |
283 | |
284 | >>> [pub.status.name for pub in view.batched_sources] |
285 | ['DELETED', 'DELETED', 'DELETED'] |
286 | @@ -1289,7 +1296,6 @@ |
287 | >>> view = create_initialized_view( |
288 | ... cprov.archive, name="+copy-packages", |
289 | ... form={ |
290 | - ... 'field.status_filter': 'any', |
291 | ... 'field.selected_sources': [str(private_source.id)], |
292 | ... 'field.destination_archive': 'ubuntu-team/ppa', |
293 | ... 'field.destination_series': '', |
294 | |
295 | === modified file 'lib/lp/soyuz/stories/ppa/xx-copy-packages.txt' |
296 | --- lib/lp/soyuz/stories/ppa/xx-copy-packages.txt 2009-09-12 06:49:56 +0000 |
297 | +++ lib/lp/soyuz/stories/ppa/xx-copy-packages.txt 2009-09-15 08:00:26 +0000 |
298 | @@ -559,7 +559,7 @@ |
299 | really deleted. |
300 | |
301 | >>> jblack_browser.getLink('Cancel').click() |
302 | - >>> jblack_browser.getControl(name='field.status_filter').value = ['any'] |
303 | + >>> jblack_browser.getControl(name='field.status_filter').value = [''] |
304 | >>> jblack_browser.getControl('Filter', index=0).click() |
305 | >>> print_ppa_packages(jblack_browser.contents) |
306 | Source Published Status Series Section Build |
307 | @@ -587,7 +587,7 @@ |
308 | Packages in other status can be browsed by adjusting the status |
309 | filter dropdown box. |
310 | |
311 | - >>> jblack_browser.getControl(name='field.status_filter').value = ['any'] |
312 | + >>> jblack_browser.getControl(name='field.status_filter').value = [''] |
313 | >>> jblack_browser.getControl("Filter").click() |
314 | >>> print_ppa_packages(jblack_browser.contents) |
315 | Source Published Status Series Section Build |
316 | @@ -831,7 +831,7 @@ |
317 | >>> jblack_browser.open( |
318 | ... 'http://launchpad.dev/~jblack/+archive/ppa/+packages') |
319 | >>> jblack_browser.getLink('Copy packages').click() |
320 | - >>> jblack_browser.getControl(name='field.status_filter').value = ['any'] |
321 | + >>> jblack_browser.getControl(name='field.status_filter').value = [''] |
322 | >>> jblack_browser.getControl("Filter").click() |
323 | |
324 | >>> print_ppa_packages(jblack_browser.contents) |
325 | @@ -886,7 +886,7 @@ |
326 | >>> jblack_browser.open( |
327 | ... 'http://launchpad.dev/~jblack/+archive/ppa/+packages') |
328 | >>> jblack_browser.getLink('Copy packages').click() |
329 | - >>> jblack_browser.getControl(name='field.status_filter').value = ['any'] |
330 | + >>> jblack_browser.getControl(name='field.status_filter').value = [''] |
331 | >>> jblack_browser.getControl("Filter").click() |
332 | |
333 | >>> deleted_pub_id = getPPAPubIDsFor( |
334 | |
335 | === modified file 'lib/lp/soyuz/stories/ppa/xx-delete-packages.txt' |
336 | --- lib/lp/soyuz/stories/ppa/xx-delete-packages.txt 2009-09-12 01:57:22 +0000 |
337 | +++ lib/lp/soyuz/stories/ppa/xx-delete-packages.txt 2009-09-15 08:06:17 +0000 |
338 | @@ -211,7 +211,7 @@ |
339 | cdrkit - 1.0 2007-07-09 Deleted Breezy-autotest Editors i386 |
340 | |
341 | >>> cprov_browser.getControl( |
342 | - ... name='field.status_filter').value = ['any'] |
343 | + ... name='field.status_filter').value = [''] |
344 | >>> cprov_browser.getControl('Filter', index=0).click() |
345 | >>> print_ppa_packages(cprov_browser.contents) |
346 | Source Published Status Series Section Build |
347 | @@ -274,7 +274,7 @@ |
348 | Any user can see that all packages present in Celso's PPA are deleted. |
349 | |
350 | >>> cprov_browser.getControl( |
351 | - ... name='field.status_filter').value = ['any'] |
352 | + ... name='field.status_filter').value = [''] |
353 | >>> cprov_browser.getControl('Filter', index=0).click() |
354 | >>> print_ppa_packages(cprov_browser.contents) |
355 | Source Published Status Series Section Build |
356 | @@ -421,7 +421,7 @@ |
357 | filter is 'Any Status', but the user can choose another. |
358 | |
359 | >>> print user_browser.getControl(name='field.status_filter').value |
360 | - ['any'] |
361 | + [''] |
362 | |
363 | When the user selects 'Published' filter and update the results, no |
364 | records are presented. |
365 | |
366 | === modified file 'lib/lp/soyuz/stories/ppa/xx-ppa-packages.txt' |
367 | --- lib/lp/soyuz/stories/ppa/xx-ppa-packages.txt 2009-09-12 06:49:56 +0000 |
368 | +++ lib/lp/soyuz/stories/ppa/xx-ppa-packages.txt 2009-09-15 08:06:17 +0000 |
369 | @@ -273,7 +273,7 @@ |
370 | The 'Any Status' filter is also available, so the user can search over |
371 | any package ever published in the context PPA. |
372 | |
373 | - >>> anon_browser.getControl(name='field.status_filter').value = ['any'] |
374 | + >>> anon_browser.getControl(name='field.status_filter').value = [''] |
375 | >>> anon_browser.getControl('Filter', index=0).click() |
376 | >>> print_archive_package_rows(anon_browser.contents) |
377 | Source Published Status Series Section Build |
378 | |
379 | === modified file 'lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt' |
380 | --- lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt 2009-09-12 02:09:47 +0000 |
381 | +++ lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt 2009-09-15 08:06:17 +0000 |
382 | @@ -393,12 +393,12 @@ |
383 | |
384 | == Filtering an archive == |
385 | |
386 | -The default series filter is 'any' which means that by default the |
387 | +The default series filter is '' which means that by default the |
388 | results will include packages from any distro series. A user can |
389 | explicitly set the 'Any Series' filter and get the same result: |
390 | |
391 | >>> anon_browser.getControl(name='field.series_filter').value = ( |
392 | - ... ['any']) |
393 | + ... ['']) |
394 | >>> anon_browser.getControl('Filter', index=0).click() |
395 | >>> print_archive_package_rows(anon_browser.contents) |
396 | Package Version Uploaded by |
= Summary =
This branch is a fix for bug 429263.
Since updating the filtering options for pkgs on ppas to use more
zope.formlib stuff, we can no-longer explicitly declare the null-option,
but need to rely on zope's built-in null option for choices (which gets
added irrespectively for non-required choices).
== Proposed fix ==
Remove our explicit 'any' option for status and series filters and
instead rely on zope's built-in None option.
== Pre-implementation notes ==
Discussed briefly with Celso to see how we had handled this in other
situations.
== Implementation details ==
I ended up with two near identical properties - selected_ status_ filter series_ filter - hence drying up with the erValue( ) method.
and selected_
getSelectedFilt
== Tests ==
bin/test -vv -t archive-views -t stories/ppa
== Demo and Q/A ==
There should no longer be a '(no value)' option at:
Local demo, play with filtering at: /launchpad. dev/~cprov/ +archive/ ppa /launchpad. dev/~cprov/ +archive/ ppa/+packages /launchpad. dev/~cprov/ +archive/ ppa/+copy- packages /launchpad. dev/~cprov/ +archive/ ppa/+delete- packages
https:/
https:/
https:/
https:/
On edge: similar sub-urls from:
https:/ /edge.launchpad .net/~cprov/ +archive/ ppa
= Launchpad lint =
Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.
Linting changed files: soyuz/stories/ ppa/xx- ubuntu- ppas.txt soyuz/browser/ tests/archive- views.txt soyuz/stories/ ppa/xx- delete- packages. txt soyuz/stories/ ppa/xx- copy-packages. txt soyuz/stories/ ppa/xx- ppa-packages. txt soyuz/browser/ archive. py
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
--
Michael