Don't discard values from 'archive' filter

The pagination functionality used in the 'patchwork.view.generic_list'
generates the filter querystring from scratch. To do this, it calls the
'patchwork.filters.Filters.params' function, which in turn calls the
'patchwork.filters.Filter.key' function for each filter. If any of these
'key' functions return None, the relevant filter is not included in the
querystring. This ensures we don't end up with a load of filters like
the below:

    ?submitter=&state=&series=&q=&delegate=&archive=both

which would be functionally equivalent to:

    ?archive=both

There is one exception to this rule, however: ArchiveFilter. This is a
little unusual in that it is active by default, excluding patches that
are "archived" from the list. As a result, the 'key' function should
return None for this active state, not for the disabled state. This has
been the case up until commit d848f046 which falsely equated 'is False'
with 'is None'. This small typo resulted in the filter being ignored
when generating pagination links and essentially broke pagination for
some use cases.

Fix this up. We could probably simplify this thing greatly by not
recalculating filters for pagination at least or, better yet, by using
django-filter here too. That is a change for another day though.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Reported-by: John McNamara <john.mcnamara@intel.com>
Reported-by: Eli Schwartz <eschwartz93@gmail.com>
Fixes: d848f046 ("trivial: Don't shadow built-ins")
Closes: #184

diff --git a/patchwork/filters.py b/patchwork/filters.py
index bc8ca41add4aab2ce3324af043f1454750f0a94f..8d0f82f2d9a5772d2d734d3527d5fdff1811fbb5 100644 (file)
--- a/patchwork/filters.py
+++ b/patchwork/filters.py
@@ -340,7 +340,9 @@ class ArchiveFilter(Filter):
         return self.description_map[self.archive_state]
 
     def key(self):
-        if not self.archive_state:
+        # NOTE(stephenfin): this is a shortcut to ensure we don't both
+        # including the 'archive' querystring filter for the default case
+        if self.archive_state is False:
             return None
         return self.param_map[self.archive_state]