Code review comment for lp:~julian-edwards/launchpad/ppa-search-oops-bug-446157

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