| Message ID | 20180613215405.12113-1-stephen@that.guru |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Don't discard values from 'archive' filter | expand |
On Wed, 2018-06-13 at 22:54 +0100, Stephen Finucane wrote: > 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, adding a test to prevent regressions. 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> > Fixes: d848f046 ("trivial: Don't shadow built-ins") > Closes: #190 I've gone ahead and merged this as a trivial fix. I also backported this and it is one of the three important fixes included in v2.0.3. Stephen
diff --git a/patchwork/filters.py b/patchwork/filters.py index bc8ca41a..8d0f82f2 100644 --- 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]
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, adding a test to prevent regressions. 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> Fixes: d848f046 ("trivial: Don't shadow built-ins") Closes: #190 --- patchwork/filters.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)