Ensure package search form correctly handles errors

We were silently eating errors and just showing a normal package list if
the form didn't validate. Rather than do that, make sure we return no
packages at all and display the form errors back to the user in a sane
fashion. Adjust the validation methods on the 'limit' parameter so any
integer is acceptable.

Signed-off-by: Dan McGee <dan@archlinux.org>

2 files changed, 54 insertions, 38 deletions

diff --git a/packages/views.py b/packages/views.py
index 3cef822..6239f01 100644
--- a/packages/views.py
+++ b/packages/views.py
@@ -126,6 +126,23 @@ def getmaintainer(request, name, repo, arch):
 
     return HttpResponse(str('\n'.join(names)), mimetype='text/plain')
 
+def coerce_limit_value(value):
+    if not value:
+        return 50
+    if value == 'all':
+        return None
+    value = int(value)
+    if value < 0:
+        raise ValueError
+    return value
+
+class LimitTypedChoiceField(forms.TypedChoiceField):
+    def valid_value(self, value):
+        try:
+            return coerce_limit_value(value)
+        except ValueError, TypeError:
+            return False
+
 class PackageSearchForm(forms.Form):
     repo = forms.MultipleChoiceField(required=False)
     arch = forms.MultipleChoiceField(required=False)
@@ -136,25 +153,12 @@ class PackageSearchForm(forms.Form):
     flagged = forms.ChoiceField(
             choices=[('', 'All')] + make_choice(['Flagged', 'Not Flagged']),
             required=False)
-    limit = forms.ChoiceField(
+    limit = LimitTypedChoiceField(
             choices=make_choice([50, 100, 250]) + [('all', 'All')],
+            coerce=coerce_limit_value,
             required=False,
             initial=50)
 
-    def clean_limit(self):
-        limit = self.cleaned_data['limit']
-        if limit == 'all':
-            limit = None
-        elif limit:
-            try:
-                limit = int(limit)
-            except:
-                raise forms.ValidationError("Should be an integer")
-        else:
-            limit = 50
-        return limit
-
-
     def __init__(self, *args, **kwargs):
         super(PackageSearchForm, self).__init__(*args, **kwargs)
         self.fields['repo'].choices = make_choice(
@@ -186,7 +190,8 @@ def search(request, page=None):
                 inner_q = PackageRelation.objects.all().values('pkgbase')
                 packages = packages.exclude(pkgbase__in=inner_q)
             elif form.cleaned_data['maintainer']:
-                inner_q = PackageRelation.objects.filter(user__username=form.cleaned_data['maintainer']).values('pkgbase')
+                inner_q = PackageRelation.objects.filter(
+                        user__username=form.cleaned_data['maintainer']).values('pkgbase')
                 packages = packages.filter(pkgbase__in=inner_q)
 
             if form.cleaned_data['flagged'] == 'Flagged':
@@ -203,6 +208,9 @@ def search(request, page=None):
                 packages = packages.filter(last_update__gte=
                         datetime(lu.year, lu.month, lu.day, 0, 0))
             limit = form.cleaned_data['limit']
+        else:
+            # Form had errors, don't return any results, just the busted form
+            packages = Package.objects.none()
     else:
         form = PackageSearchForm()
 
diff --git a/templates/packages/search.html b/templates/packages/search.html
index 8ea53b8..381ebb0 100644
--- a/templates/packages/search.html
+++ b/templates/packages/search.html
@@ -14,28 +14,36 @@
 
     <h3>Package Search</h3>
 
-	<form id="pkg-search" method="get" action="/packages/">
-		<p><input type="hidden" name="sort" value='{{sort}}' /></p>
-		<fieldset>
-			<legend>Enter search criteria</legend>
-			<div><label for="id_arch" title="Limit results a specific CPU architecture">
-					Arch</label>{{ search_form.arch }}</div>
-			<div><label for="id_repo" title="Limit results to a specific respository">
-					Repository</label>{{ search_form.repo }}</div>
-			<div><label for="id_q" title="Enter keywords as desired">
-					Keywords</label>{{ search_form.q }}</div>
-			<div><label for="id_maintainer" title="Limit results to a specific maintainer">
-					Maintainer</label>{{ search_form.maintainer}}</div>
-			<div><label for="id_last_update" title="Limit results to a date after the date entered">
-					Last Updated After</label>{{ search_form.last_update }}</div>
-			<div><label for="id_flagged" title="Limit results based on out-of-date status">
-					Flagged</label>{{ search_form.flagged }}</div>
-			<div><label for="id_limit" title="Select the number of results to display per page">
-					Per Page</label>{{ search_form.limit }}</div>
-			<div ><label>&nbsp;</label><input title="Search for packages using this criteria"
-				type="submit" value="Search" /></div>
-		</fieldset>
-	</form>
+    <form id="pkg-search" method="get" action="/packages/">
+        <p><input type="hidden" name="sort" value='{{sort}}' /></p>
+        {{ search_form.non_field_errors }}
+        <fieldset>
+            <legend>Enter search criteria</legend>
+            <div>{{ search_form.arch.errors }}
+                <label for="id_arch" title="Limit results a specific CPU architecture">
+                    Arch</label>{{ search_form.arch }}</div>
+            <div>{{ search_form.repo.errors }}
+                <label for="id_repo" title="Limit results to a specific respository">
+                    Repository</label>{{ search_form.repo }}</div>
+            <div>{{ search_form.q.errors }}
+                <label for="id_q" title="Enter keywords as desired">
+                    Keywords</label>{{ search_form.q }}</div>
+            <div>{{ search_form.maintainer.errors }}
+                <label for="id_maintainer" title="Limit results to a specific maintainer">
+                    Maintainer</label>{{ search_form.maintainer}}</div>
+            <div>{{ search_form.last_update.errors }}
+                <label for="id_last_update" title="Limit results to a date after the date entered">
+                    Last Updated After</label>{{ search_form.last_update }}</div>
+            <div>{{ search_form.flagged.errors }}
+                <label for="id_flagged" title="Limit results based on out-of-date status">
+                    Flagged</label>{{ search_form.flagged }}</div>
+            <div>{{ search_form.limit.errors }}
+                <label for="id_limit" title="Select the number of results to display per page">
+                    Per Page</label>{{ search_form.limit }}</div>
+            <div ><label>&nbsp;</label><input title="Search for packages using this criteria"
+                type="submit" value="Search" /></div>
+        </fieldset>
+    </form>
 
 </div><!-- #pkglist-search -->