Merge lp:~julian-edwards/launchpad/ppa-search-oops-bug-446157 into lp:launchpad

[Julian Edwards]

[Julian Edwards (julian-edwards)]

= Summary =
Fix an oops when someone hand-hacks URLs searching PPAs.

== Proposed fix ==
We need to cope with a list value when expecting a string in the form data.
This is a trivial change in the view code, but I also filed a Foundations bugs
since we should be able to do this generically in the form setup.

== Pre-implementation notes ==
None, I suck.

== Tests ==
bin/test -cvvt xx-ubuntu-ppas

== Demo and Q/A ==
https://launchpad.dev/ubuntu/+ppas?name_filter=packages&name_filter=humanity

= 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/registry/browser/distribution.py

[Michael Nelson (michael.nelson)]

Thanks Julian. Just a suggestion for changing the comment below.

> === modified file 'lib/lp/registry/browser/distribution.py'
> --- lib/lp/registry/browser/distribution.py 2009-09-23 14:58:12 +0000
> +++ lib/lp/registry/browser/distribution.py 2009-10-15 08:23:35 +0000
> @@ -646,6 +646,15 @@
>
> def initialize(self):
> self.name_filter = self.request.get('name_filter')
> + if isinstance(self.name_filter, list):
> + # This happens if someone hand-hacks the URL so that it has
> + # more than one name_filter field.
> + #

Is that normal? I've normally just left a blank line there. I'm just
interested to know.

> + # XXX 2009-10-14 Julian bug=451424
> + # There really should be a way for the form to reject
> + # unexpected input like this before it hits the code here.
> + # See bug 451424.

As per our conversation, I think the issue here is more that we are not
using an LPFormView. We could inherit from LPFormView, have an interface
for the search form defined, use a safe_action and the data would be
validated automatically right? Up to you whether you want to go ahead
and actually do that, or simply update the above comment.

> + self.name_filter = " ".join(self.name_filter)

Great - at first I wondered why you didn't just grab name_filter[0]
(or [-1]), but the doctest has a good justification for joining the
terms.

> self.show_inactive = self.request.get('show_inactive')
>
> @property
>
> === modified file 'lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt'
> --- lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt 2009-09-23 14:58:12 +0000
> +++ lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt 2009-10-15 08:23:35 +0000
> @@ -246,6 +246,20 @@
> 1
>
>
> +=== Hand-hacked search URLs ==
> +
> +If the search term is specified more than once by someone hand-hacking the
> +URL, the page copes gracefully with this by searching for all the terms
> +specified.
> +
> + >>> anon_browser.open(
> + ... "http://launchpad.dev/ubuntu/+ppas"
> + ... "?name_filter=packages&name_filter=friends")
> + >>> [row] = find_tags_by_class(anon_browser.contents, 'ppa_batch_row')
> + >>> print extract_text(row)
> + PPA for Celso Providelo...
> +
> +
> == Owner's PPA pages ==
>
> Let's start by adding an extra package to Celso's archive:

--
Michael

review: Approve (code)

=== modified file 'lib/lp/registry/browser/distribution.py'
--- lib/lp/registry/browser/distribution.py	2009-09-23 14:58:12 +0000
+++ lib/lp/registry/browser/distribution.py	2009-10-15 10:29:13 +0000
@@ -646,6 +646,13 @@
 
     def initialize(self):
         self.name_filter = self.request.get('name_filter')
+        if isinstance(self.name_filter, list):
+            # This happens if someone hand-hacks the URL so that it has
+            # more than one name_filter field.  We could do something
+            # like form.getOne() so that the request would be rejected,
+            # but we can acutally do better and join the terms supplied
+            # instead.
+            self.name_filter = " ".join(self.name_filter)
         self.show_inactive = self.request.get('show_inactive')
 
     @property

=== modified file 'lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt'
--- lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt	2009-09-23 14:58:12 +0000
+++ lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt	2009-10-15 10:29:13 +0000
@@ -246,6 +246,20 @@
     1
 
 
+=== Hand-hacked search URLs ==
+
+If the search term is specified more than once by someone hand-hacking the
+URL, the page copes gracefully with this by searching for all the terms
+specified.
+
+    >>> anon_browser.open(
+    ...     "http://launchpad.dev/ubuntu/+ppas"
+    ...     "?name_filter=packages&name_filter=friends")
+    >>> [row] = find_tags_by_class(anon_browser.contents, 'ppa_batch_row')
+    >>> print extract_text(row)
+    PPA for Celso Providelo...
+
+
 == Owner's PPA pages ==
 
 Let's start by adding an extra package to Celso's archive: