Merge lp:~michael.nelson/launchpad/429263-no-value-option into lp:launchpad

[Michael Nelson]

[Michael Nelson (michael.nelson)]

= 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
and selected_series_filter - hence drying up with the
getSelectedFilterValue() method.

== 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:
https://launchpad.dev/~cprov/+archive/ppa
https://launchpad.dev/~cprov/+archive/ppa/+packages
https://launchpad.dev/~cprov/+archive/ppa/+copy-packages
https://launchpad.dev/~cprov/+archive/ppa/+delete-packages

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:
  lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt
  lib/lp/soyuz/browser/tests/archive-views.txt
  lib/lp/soyuz/stories/ppa/xx-delete-packages.txt
  lib/lp/soyuz/stories/ppa/xx-copy-packages.txt
  lib/lp/soyuz/stories/ppa/xx-ppa-packages.txt
  lib/lp/soyuz/browser/archive.py

--
Michael

[Graham Binns (gmb)]

review: Approve

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2009-09-11 17:55:23 +0000
+++ lib/lp/soyuz/browser/archive.py	2009-09-15 08:25:32 +0000
@@ -577,7 +577,7 @@
             this factory can only be used in a class where the context is
             an IArchive.
         """
-        series_terms = [SimpleTerm(None, token='any', title='Any Series')]
+        series_terms = []
         for distroseries in context.series_with_sources:
             series_terms.append(
                 SimpleTerm(distroseries, token=distroseries.name,
@@ -585,6 +585,16 @@
         return SimpleVocabulary(series_terms)
 
 
+class SeriesFilterWidget(LaunchpadDropdownWidget):
+    """Redefining default display value as 'Any series'."""
+    _messageNoValue = _("any", "Any series")
+
+
+class StatusFilterWidget(LaunchpadDropdownWidget):
+    """Redefining default display value as 'Any status'."""
+    _messageNoValue = _("any", "Any status")
+
+
 class IPPAPackageFilter(Interface):
     """The interface used as the schema for the package filtering form."""
     name_filter = TextLine(
@@ -596,7 +606,6 @@
     status_filter = Choice(vocabulary=SimpleVocabulary((
         SimpleTerm(active_publishing_status, 'published', 'Published'),
         SimpleTerm(inactive_publishing_status, 'superseded', 'Superseded'),
-        SimpleTerm(None, 'any', 'Any status')
         )), required=False)
 
 
@@ -604,6 +613,8 @@
     """A Form view for filtering and batching source packages."""
 
     schema = IPPAPackageFilter
+    custom_widget('series_filter', SeriesFilterWidget)
+    custom_widget('status_filter', StatusFilterWidget)
 
     # By default this view will not display the sources with selectable
     # checkboxes, but subclasses can override as needed.
@@ -625,65 +636,52 @@
         else:
             return None
 
-    @cachedproperty
-    def selected_status_filter(self):
-        """Return the selected status filter or the default."""
-        requested_status_filter = self.request.query_string_params.get(
-            'field.status_filter')
-
-        # If the request included a status filter, try to use it:
-        selected_status_filter = None
-        if requested_status_filter is not None:
-            selected_status_filter = (
-                self.widgets['status_filter'].vocabulary.getTermByToken(
-                    requested_status_filter[0]))
-
-        # If the request didn't include a status, or it was invalid, use
-        # the default:
-        if selected_status_filter is None:
-            selected_status_filter = self.default_status_filter
-
-        return selected_status_filter
+    def getSelectedFilterValue(self, filter_name):
+        """Return the selected filter or the default, given a filter name.
+
+        This is needed because zope's form library does not consider
+        query string params (GET params) during a post request.
+        """
+        field_name = 'field.' + filter_name
+        requested_filter = self.request.query_string_params.get(field_name)
+
+        # If an empty filter was specified, then it's explicitly
+        # been set to empty - so we use None.
+        if requested_filter == ['']:
+            return None
+
+        # If the requested filter is none, then we use the default.
+        default_filter_attr = 'default_' + filter_name
+        if requested_filter is None:
+            return getattr(self, default_filter_attr)
+
+        # If the request included a filter, try to use it - if it's
+        # invalid we use the default instead.
+        vocab = self.widgets[filter_name].vocabulary
+        if vocab.by_token.has_key(requested_filter[0]):
+            return vocab.getTermByToken(requested_filter[0]).value
+        else:
+            return getattr(self, default_filter_attr)
 
     @property
     def plain_status_filter_widget(self):
         """Render a <select> control with no <div>s around it."""
         return self.widgets['status_filter'].renderValue(
-            self.selected_status_filter.value)
-
-    @property
-    def selected_series_filter(self):
-        """Return the currently selected filter or None."""
-        requested_series_filter = self.request.query_string_params.get(
-            'field.series_filter')
-
-        # If the request included a series filter, try to use it:
-        selected_series_filter = None
-        if requested_series_filter is not None:
-            series_vocabulary = self.widgets['series_filter'].vocabulary
-            selected_series_filter = series_vocabulary.getTermByToken(
-                requested_series_filter[0])
-
-        # If the request didn't include a series, or it was invalid, use
-        # the default:
-        if selected_series_filter is None:
-            selected_series_filter = self.default_series_filter
-
-        return selected_series_filter
+            self.getSelectedFilterValue('status_filter'))
 
     @property
     def plain_series_filter_widget(self):
         """Render a <select> control with no <div>s around it."""
         return self.widgets['series_filter'].renderValue(
-            self.selected_series_filter.value)
+            self.getSelectedFilterValue('series_filter'))
 
     @property
     def filtered_sources(self):
         """Return the source results for display after filtering."""
         return self.context.getPublishedSources(
             name=self.specified_name_filter,
-            status=self.selected_status_filter.value,
-            distroseries=self.selected_series_filter.value)
+            status=self.getSelectedFilterValue('status_filter'),
+            distroseries=self.getSelectedFilterValue('series_filter'))
 
     @property
     def default_status_filter(self):
@@ -692,7 +690,7 @@
         Subclasses of ArchiveViewBase can override this when required.
         """
         return self.widgets['status_filter'].vocabulary.getTermByToken(
-            'published')
+            'published').value
 
     @property
     def default_series_filter(self):
@@ -700,7 +698,7 @@
 
         Subclasses of ArchiveViewBase can override this when required.
         """
-        return self.widgets['series_filter'].vocabulary.getTermByToken('any')
+        return None
 
     @cachedproperty
     def batchnav(self):
@@ -769,10 +767,10 @@
         for term in vocabulary:
             if (term.value is not None and
                 term.value.version == version_number):
-                return term
+                return term.value
 
         # Otherwise we default to 'any'
-        return vocabulary.getTermByToken('any')
+        return None
 
     @property
     def archive_description_html(self):
@@ -990,7 +988,7 @@
     @property
     def default_status_filter(self):
         """Present records in any status by default."""
-        return self.widgets['status_filter'].vocabulary.getTermByToken('any')
+        return None
 
     @cachedproperty
     def filtered_sources(self):
@@ -998,14 +996,11 @@
 
         This overrides ArchiveViewBase.filtered_sources to use a
         different method on the context specific to deletion records.
-
-        It expects 'self.selected_status_filter' and
-        'self.selected_series_filter' to be set.
         """
         return self.context.getSourcesForDeletion(
             name=self.specified_name_filter,
-            status=self.selected_status_filter.value,
-            distroseries=self.selected_series_filter.value)
+            status=self.getSelectedFilterValue('status_filter'),
+            distroseries=self.getSelectedFilterValue('series_filter'))
 
     @cachedproperty
     def has_sources(self):
@@ -1093,7 +1088,7 @@
     def default_status_filter(self):
         """Present published records by default."""
         return self.widgets['status_filter'].vocabulary.getTermByToken(
-            'published')
+            'published').value
 
     def setUpFields(self):
         """Override `ArchiveSourceSelectionFormView`.
 
=== modified file 'lib/lp/soyuz/browser/tests/archive-views.txt'
--- lib/lp/soyuz/browser/tests/archive-views.txt	2009-09-11 17:55:23 +0000
+++ lib/lp/soyuz/browser/tests/archive-views.txt	2009-09-15 08:25:32 +0000
@@ -90,14 +90,12 @@
     ...     print term.title
     Published
     Superseded
-    Any status
 
 An ArchiveView inherits the series-filter widget for filtering packages
 by series.
 
     >>> for term in ppa_archive_view.widgets['series_filter'].vocabulary:
     ...     print term.title
-    Any Series
     Breezy Badger Autotest
     Warty
 
@@ -406,8 +404,8 @@
 The archive index view overrides the default series filter to use the
 distroseries from the browser's user-agent, when applicable.
 
-    >>> print view.default_series_filter.token
-    any
+    >>> print view.default_series_filter
+    None
 
     >>> view_warty = create_view(
     ...     cprov.archive, name="+index",
@@ -416,8 +414,19 @@
     ...                     'Gecko/2009042523 Ubuntu/4.10 (whatever) '
     ...                     'Firefox/3.0.10')
     >>> view_warty.initialize()
-    >>> print view_warty.default_series_filter.token
-    warty
+    >>> print view_warty.default_series_filter.name
+    warty
+
+The archive index view also inherits the getSelectedFilterValue() method
+which can be used to find the currently selected value for both filters.
+
+    >>> print view_warty.getSelectedFilterValue('series_filter').name
+    warty
+
+    >>> for status in view_warty.getSelectedFilterValue('status_filter'):
+    ...     print status.name
+    PENDING
+    PUBLISHED
 
 
 == ArchivePackageView ==
@@ -558,7 +567,6 @@
     ...     form={
     ...         'field.actions.delete': 'Delete Packages',
     ...         'field.name_filter': '',
-    ...         'field.status_filter': 'any',
     ...         'field.deletion_comment': 'Go away',
     ...         'field.selected_sources': ['27', '28', '29'],
     ...         'field.selected_sources-empty-marker': 1,
@@ -579,7 +587,6 @@
     ...     form={
     ...         'field.actions.delete': 'Delete Packages',
     ...         'field.name_filter': '',
-    ...         'field.status_filter': 'any',
     ...         'field.deletion_comment': 'Go away',
     ...         'field.selected_sources': ['27', '28', '29'],
     ...         'field.selected_sources-empty-marker': 1,
@@ -1009,7 +1016,7 @@
 
     >>> view = create_initialized_view(
     ...     cprov.archive, name="+copy-packages",
-    ...     query_string="field.status_filter=any")
+    ...     query_string="field.status_filter=")
 
     >>> [pub.status.name for pub in view.batched_sources]
     ['DELETED', 'DELETED', 'DELETED']
@@ -1289,7 +1296,6 @@
     >>> view = create_initialized_view(
     ...     cprov.archive, name="+copy-packages",
     ...     form={
-    ...         'field.status_filter': 'any',
     ...         'field.selected_sources': [str(private_source.id)],
     ...         'field.destination_archive': 'ubuntu-team/ppa',
     ...         'field.destination_series': '',
 
=== modified file 'lib/lp/soyuz/stories/ppa/xx-copy-packages.txt'
--- lib/lp/soyuz/stories/ppa/xx-copy-packages.txt	2009-09-12 06:49:56 +0000
+++ lib/lp/soyuz/stories/ppa/xx-copy-packages.txt	2009-09-15 08:00:26 +0000
@@ -559,7 +559,7 @@
 really deleted.
 
     >>> jblack_browser.getLink('Cancel').click()
-    >>> jblack_browser.getControl(name='field.status_filter').value = ['any']
+    >>> jblack_browser.getControl(name='field.status_filter').value = ['']
     >>> jblack_browser.getControl('Filter', index=0).click()
     >>> print_ppa_packages(jblack_browser.contents)
     Source          Published   Status     Series           Section  Build
@@ -587,7 +587,7 @@
 Packages in other status can be browsed by adjusting the status
 filter dropdown box.
 
-    >>> jblack_browser.getControl(name='field.status_filter').value = ['any']
+    >>> jblack_browser.getControl(name='field.status_filter').value = ['']
     >>> jblack_browser.getControl("Filter").click()
     >>> print_ppa_packages(jblack_browser.contents)
     Source          Published   Status     Series           Section  Build
@@ -831,7 +831,7 @@
     >>> jblack_browser.open(
     ...     'http://launchpad.dev/~jblack/+archive/ppa/+packages')
     >>> jblack_browser.getLink('Copy packages').click()
-    >>> jblack_browser.getControl(name='field.status_filter').value = ['any']
+    >>> jblack_browser.getControl(name='field.status_filter').value = ['']
     >>> jblack_browser.getControl("Filter").click()
 
     >>> print_ppa_packages(jblack_browser.contents)
@@ -886,7 +886,7 @@
     >>> jblack_browser.open(
     ...     'http://launchpad.dev/~jblack/+archive/ppa/+packages')
     >>> jblack_browser.getLink('Copy packages').click()
-    >>> jblack_browser.getControl(name='field.status_filter').value = ['any']
+    >>> jblack_browser.getControl(name='field.status_filter').value = ['']
     >>> jblack_browser.getControl("Filter").click()
 
     >>> deleted_pub_id = getPPAPubIDsFor(
 
=== modified file 'lib/lp/soyuz/stories/ppa/xx-delete-packages.txt'
--- lib/lp/soyuz/stories/ppa/xx-delete-packages.txt	2009-09-12 01:57:22 +0000
+++ lib/lp/soyuz/stories/ppa/xx-delete-packages.txt	2009-09-15 08:06:17 +0000
@@ -211,7 +211,7 @@
     cdrkit - 1.0     2007-07-09  Deleted    Breezy-autotest Editors  i386
 
     >>> cprov_browser.getControl(
-    ...    name='field.status_filter').value = ['any']
+    ...    name='field.status_filter').value = ['']
     >>> cprov_browser.getControl('Filter', index=0).click()
     >>> print_ppa_packages(cprov_browser.contents)
     Source            Published   Status     Series          Section  Build
@@ -274,7 +274,7 @@
 Any user can see that all packages present in Celso's PPA are deleted.
 
     >>> cprov_browser.getControl(
-    ...    name='field.status_filter').value = ['any']
+    ...    name='field.status_filter').value = ['']
     >>> cprov_browser.getControl('Filter', index=0).click()
     >>> print_ppa_packages(cprov_browser.contents)
     Source            Published   Status     Series          Section  Build
@@ -421,7 +421,7 @@
 filter is 'Any Status', but the user can choose another.
 
     >>> print user_browser.getControl(name='field.status_filter').value
-    ['any']
+    ['']
 
 When the user selects 'Published' filter and update the results, no
 records are presented.
 
=== modified file 'lib/lp/soyuz/stories/ppa/xx-ppa-packages.txt'
--- lib/lp/soyuz/stories/ppa/xx-ppa-packages.txt	2009-09-12 06:49:56 +0000
+++ lib/lp/soyuz/stories/ppa/xx-ppa-packages.txt	2009-09-15 08:06:17 +0000
@@ -273,7 +273,7 @@
 The 'Any Status' filter is also available, so the user can search over
 any package ever published in the context PPA.
 
-    >>> anon_browser.getControl(name='field.status_filter').value = ['any']
+    >>> anon_browser.getControl(name='field.status_filter').value = ['']
     >>> anon_browser.getControl('Filter', index=0).click()
     >>> print_archive_package_rows(anon_browser.contents)
     Source             Published    Status     Series      Section  Build
 
=== modified file 'lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt'
--- lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt	2009-09-12 02:09:47 +0000
+++ lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt	2009-09-15 08:06:17 +0000
@@ -393,12 +393,12 @@
 
 == Filtering an archive ==
 
-The default series filter is 'any' which means that by default the
+The default series filter is '' which means that by default the
 results will include packages from any distro series. A user can
 explicitly set the 'Any Series' filter and get the same result:
 
     >>> anon_browser.getControl(name='field.series_filter').value = (
-    ...     ['any'])
+    ...     [''])
     >>> anon_browser.getControl('Filter', index=0).click()
     >>> print_archive_package_rows(anon_browser.contents)
     Package             Version         Uploaded by