From a0a74da897cb5e9115019fe8aa0e5c932340c6cd Mon Sep 17 00:00:00 2001 From: Ryan P Kilby Date: Fri, 13 Jul 2018 04:27:04 -0400 Subject: [PATCH] Rework DateRangeFilter (#852) * Replace DateRangeFilter.options w/ choices/filters - Replaces the 'options' data structure with standard form field choices and a 'filters' dict, which contains the date filters separately. - Choices now use descriptive string values instead of integers. * Ensure DateRangeFilter choices and filters match * Add assertion for removed DateRangeFilter.options * Remove duplicate code in DateRangeFilterTests * Mock dates in DateRangeFilterTests * Add 'yesterday' test for DateRangeFilter * Fix 'yesterday' filtering for DateRangeFilter --- django_filters/filters.py | 75 ++++++++++++++++++++------------- tests/test_filtering.py | 89 +++++++++++++++------------------------ tests/test_filters.py | 43 ++++++++++++++----- 3 files changed, 111 insertions(+), 96 deletions(-) diff --git a/django_filters/filters.py b/django_filters/filters.py index 09dade872..cadba4ea9 100644 --- a/django_filters/filters.py +++ b/django_filters/filters.py @@ -398,51 +398,66 @@ def _truncate(dt): class DateRangeFilter(ChoiceFilter): - options = { - '': (_('Any date'), lambda qs, name: qs), - 1: (_('Today'), lambda qs, name: qs.filter(**{ + choices = [ + ('today', _('Today')), + ('yesterday', _('Yesterday')), + ('week', _('Past 7 days')), + ('month', _('This month')), + ('year', _('This year')), + ] + + filters = { + 'today': lambda qs, name: qs.filter(**{ '%s__year' % name: now().year, '%s__month' % name: now().month, '%s__day' % name: now().day - })), - 2: (_('Past 7 days'), lambda qs, name: qs.filter(**{ + }), + 'yesterday': lambda qs, name: qs.filter(**{ + '%s__year' % name: (now() - timedelta(days=1)).year, + '%s__month' % name: (now() - timedelta(days=1)).month, + '%s__day' % name: (now() - timedelta(days=1)).day, + }), + 'week': lambda qs, name: qs.filter(**{ '%s__gte' % name: _truncate(now() - timedelta(days=7)), '%s__lt' % name: _truncate(now() + timedelta(days=1)), - })), - 3: (_('This month'), lambda qs, name: qs.filter(**{ + }), + 'month': lambda qs, name: qs.filter(**{ '%s__year' % name: now().year, '%s__month' % name: now().month - })), - 4: (_('This year'), lambda qs, name: qs.filter(**{ - '%s__year' % name: now().year, - })), - 5: (_('Yesterday'), lambda qs, name: qs.filter(**{ + }), + 'year': lambda qs, name: qs.filter(**{ '%s__year' % name: now().year, - '%s__month' % name: now().month, - '%s__day' % name: (now() - timedelta(days=1)).day, - })), + }), } - def __init__(self, *args, **kwargs): - kwargs['choices'] = [ - (key, value[0]) for key, value in self.options.items()] + def __init__(self, choices=None, filters=None, *args, **kwargs): + if choices is not None: + self.choices = choices + if filters is not None: + self.filters = filters - # empty/null choices not relevant - kwargs.setdefault('empty_label', None) + unique = set([x[0] for x in self.choices]) ^ set(self.filters) + assert not unique, \ + "Keys must be present in both 'choices' and 'filters'. Missing keys: " \ + "'%s'" % ', '.join(sorted(unique)) + + # TODO: remove assertion in 2.1 + assert not hasattr(self, 'options'), \ + "The 'options' attribute has been replaced by 'choices' and 'filters'. " \ + "See: https://django-filter.readthedocs.io/en/master/guide/migration.html" + + # null choice not relevant kwargs.setdefault('null_label', None) - super().__init__(*args, **kwargs) + super().__init__(choices=self.choices, *args, **kwargs) def filter(self, qs, value): - try: - value = int(value) - except (ValueError, TypeError): - value = '' + if not value: + return qs - assert value in self.options - qs = self.options[value][1](qs, self.field_name) - if self.distinct: - qs = qs.distinct() - return qs + assert value in self.filters + + qs = self.filters[value](qs, self.field_name) + return qs.distinct() if self.distinct else qs class DateFromToRangeFilter(RangeFilter): diff --git a/tests/test_filtering.py b/tests/test_filtering.py index 9843911d6..f5610c5ef 100644 --- a/tests/test_filtering.py +++ b/tests/test_filtering.py @@ -1,3 +1,4 @@ +import contextlib import datetime import mock import unittest @@ -5,7 +6,7 @@ from django import forms from django.test import TestCase, override_settings from django.utils import timezone -from django.utils.timezone import now +from django.utils.timezone import make_aware, now from django_filters.filters import ( AllValuesFilter, @@ -762,17 +763,19 @@ class Meta: lambda o: o.title) -# TODO: -# year & month filtering could be better. The problem is that the test dates -# are relative to today, which is always changing. So, two_weeks_ago is not a -# valid date for 'this month' during the first half of the month, but is during -# the second half. Similary, five_days_ago is not during 'this year' when the -# tests are ran on January 1. All we can test is what is absolutely never valid -# eg, a date from two_years_ago is never a valid date for 'this year'. class DateRangeFilterTests(TestCase): - def setUp(self): - today = now().date() + class CommentFilter(FilterSet): + date = DateRangeFilter() + + class Meta: + model = Comment + fields = ['date'] + + @contextlib.contextmanager + def relative_to(self, today): + today = make_aware(today) + yesterday = today - datetime.timedelta(days=1) five_days_ago = today - datetime.timedelta(days=5) two_weeks_ago = today - datetime.timedelta(days=14) two_months_ago = today - datetime.timedelta(days=62) @@ -784,61 +787,37 @@ def setUp(self): Comment.objects.create(date=two_years_ago, author=alex, time=time) Comment.objects.create(date=five_days_ago, author=alex, time=time) Comment.objects.create(date=today, author=alex, time=time) + Comment.objects.create(date=yesterday, author=alex, time=time) Comment.objects.create(date=two_months_ago, author=alex, time=time) - def test_filtering_for_year(self): - class F(FilterSet): - date = DateRangeFilter() - - class Meta: - model = Comment - fields = ['date'] - - f = F({'date': '4'}) # this year + with mock.patch('django_filters.filters.now') as mock_now: + mock_now.return_value = today + yield - # assert what is NOT valid for now. - # self.assertQuerysetEqual(f.qs, [1, 3, 4, 5], lambda o: o.pk, False) - self.assertNotIn(2, f.qs.values_list('pk', flat=True)) + def test_filtering_for_year(self): + f = self.CommentFilter({'date': 'year'}) + with self.relative_to(datetime.datetime(now().year, 4, 1)): + self.assertQuerysetEqual(f.qs, [1, 3, 4, 5, 6], lambda o: o.pk, False) def test_filtering_for_month(self): - class F(FilterSet): - date = DateRangeFilter() - - class Meta: - model = Comment - fields = ['date'] - - f = F({'date': '3'}) # this month - - # assert what is NOT valid for now. - # self.assertQuerysetEqual(f.qs, [1, 3, 4], lambda o: o.pk, False) - self.assertNotIn(2, f.qs.values_list('pk', flat=True)) - self.assertNotIn(5, f.qs.values_list('pk', flat=True)) + f = self.CommentFilter({'date': 'month'}) + with self.relative_to(datetime.datetime(now().year, 4, 21)): + self.assertQuerysetEqual(f.qs, [1, 3, 4, 5], lambda o: o.pk, False) def test_filtering_for_week(self): - class F(FilterSet): - date = DateRangeFilter() - - class Meta: - model = Comment - fields = ['date'] + f = self.CommentFilter({'date': 'week'}) + with self.relative_to(datetime.datetime(now().year, 1, 1)): + self.assertQuerysetEqual(f.qs, [3, 4, 5], lambda o: o.pk, False) - f = F({'date': '2'}) # this week - self.assertQuerysetEqual(f.qs, [3, 4], lambda o: o.pk, False) + def test_filtering_for_yesterday(self): + f = self.CommentFilter({'date': 'yesterday'}) + with self.relative_to(datetime.datetime(now().year, 1, 1)): + self.assertQuerysetEqual(f.qs, [5], lambda o: o.pk, False) def test_filtering_for_today(self): - class F(FilterSet): - date = DateRangeFilter() - - class Meta: - model = Comment - fields = ['date'] - - f = F({'date': '1'}) # today - self.assertQuerysetEqual(f.qs, [4], lambda o: o.pk, False) - - # it will be difficult to test for TZ related issues, where "today" means - # different things to both user and server. + f = self.CommentFilter({'date': 'today'}) + with self.relative_to(datetime.datetime(now().year, 1, 1)): + self.assertQuerysetEqual(f.qs, [4], lambda o: o.pk, False) class DateFromToRangeFilterTests(TestCase): diff --git a/tests/test_filters.py b/tests/test_filters.py index ea7f347f2..a5bae2662 100644 --- a/tests/test_filters.py +++ b/tests/test_filters.py @@ -922,9 +922,15 @@ def test_filtering_ignores_lookup_expr(self): class DateRangeFilterTests(TestCase): def test_creating(self): - f = DateRangeFilter() - self.assertIn('choices', f.extra) - self.assertEqual(len(DateRangeFilter.options), len(f.extra['choices'])) + f = DateRangeFilter(empty_label=None) + self.assertEqual(len(f.choices), 5) + self.assertIs(f.choices, f.extra['choices']) + + f = DateRangeFilter(empty_label=None, choices=[], filters=[]) + self.assertEqual(f.choices, []) + self.assertEqual(f.filters, []) + self.assertEqual(len(f.choices), 0) + self.assertIs(f.choices, f.extra['choices']) def test_default_field(self): f = DateRangeFilter() @@ -943,14 +949,29 @@ def test_filtering_skipped_with_out_of_range_value(self): qs = mock.Mock(spec=[]) f = DateRangeFilter() with self.assertRaises(AssertionError): - f.filter(qs, 999) + f.filter(qs, 'tomorrow') + + def test_choices_and_filters_mismatch(self): + msg = "Keys must be present in both 'choices' and 'filters'. Missing keys: 'a, b'" + with self.assertRaisesMessage(AssertionError, msg): + DateRangeFilter(choices=[('a', 'a')], filters={'b': None}) + + def test_options_removed(self): + msg = "The 'options' attribute has been replaced by 'choices' and 'filters'. " \ + "See: https://django-filter.readthedocs.io/en/master/guide/migration.html" + + class F(DateRangeFilter): + options = None + + with self.assertRaisesMessage(AssertionError, msg): + F() def test_filtering_for_this_year(self): qs = mock.Mock(spec=['filter']) with mock.patch('django_filters.filters.now') as mock_now: now_dt = mock_now.return_value f = DateRangeFilter() - f.filter(qs, '4') + f.filter(qs, 'year') qs.filter.assert_called_once_with( None__year=now_dt.year) @@ -959,7 +980,7 @@ def test_filtering_for_this_month(self): with mock.patch('django_filters.filters.now') as mock_now: now_dt = mock_now.return_value f = DateRangeFilter() - f.filter(qs, '3') + f.filter(qs, 'month') qs.filter.assert_called_once_with( None__year=now_dt.year, None__month=now_dt.month) @@ -971,7 +992,7 @@ def test_filtering_for_7_days(self): mock_d1, mock_d2 = mock.MagicMock(), mock.MagicMock() mock_truncate.side_effect = [mock_d1, mock_d2] f = DateRangeFilter() - f.filter(qs, '2') + f.filter(qs, 'week') self.assertEqual( mock_td.call_args_list, [mock.call(days=7), mock.call(days=1)] @@ -983,7 +1004,7 @@ def test_filtering_for_today(self): with mock.patch('django_filters.filters.now') as mock_now: now_dt = mock_now.return_value f = DateRangeFilter() - f.filter(qs, '1') + f.filter(qs, 'today') qs.filter.assert_called_once_with( None__year=now_dt.year, None__month=now_dt.month, @@ -994,10 +1015,10 @@ def test_filtering_for_yesterday(self): with mock.patch('django_filters.filters.now') as mock_now: now_dt = mock_now.return_value f = DateRangeFilter() - f.filter(qs, '5') + f.filter(qs, 'yesterday') qs.filter.assert_called_once_with( - None__year=now_dt.year, - None__month=now_dt.month, + None__year=(now_dt - timedelta(days=1)).year, + None__month=(now_dt - timedelta(days=1)).month, None__day=(now_dt - timedelta(days=1)).day, )