search

silentsokolov / django-admin-rangefilter

A Django app that lets you filter data by date range and numeric range in the admin UI
MIT License
713 stars 106 forks source link

Fix bug with only gte/lte support and refactor queryset method #3

Closed tomekkorbak closed 8 years ago

tomekkorbak commented 8 years ago

There was a bug in queryset method. When only one of the fields field__lte and field__gte is populated and the other is left blank, the blank one was filtered out of the filtered_params dictionary in line 59 and raised KeyError in either line 55 or line 68.

This commit fixes the bug by using gte and lte lookups rather than range and checking if self.lookup_kwarg_(gte|lte) in filter_params. Also, queryset method was refactored and the logic of acquiring filtering parameters for Django ORM was extracted to private methods with separate responsibilities.

silentsokolov commented 8 years ago

Its cool! But now function _get_lookup_parameter excessive and complicated code :( 

make_dt_aware(datetime.datetime.combine(        
    filter_params[self.lookup_kwarg_lte], datetime.time.max     
))

vs

func(params):
  if/else
  get by key
tomekkorbak commented 8 years ago

What is excessive? The alternative to the if-else clause is code duplication for two get-by-keys.

One other option is to provide datetime.time.(max|min) as an argument to _get_lookup_parameter, though it's not perfect encapsulation-wise. Feel free to choose this way if you consider it better.