Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ginkgo backward compatibility fix for Django Filters #266

Merged
merged 1 commit into from
Oct 12, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 37 additions & 14 deletions figures/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
update the test class names in "tests/test_filters.py" to match.
"""

from packaging import version
from django.contrib.auth import get_user_model
from django.contrib.sites.models import Site
from django.db.models import F
Expand All @@ -26,6 +27,7 @@

from student.models import CourseEnrollment # pylint: disable=import-error


from figures.pipeline.course_daily_metrics import get_enrolled_in_exclude_admins
from figures.models import (
CourseDailyMetrics,
Expand All @@ -36,6 +38,27 @@
)


def char_filter(field_name, lookup_expr):
"""For backwards compatibility.

We require both `field_name` and `lookup_expr` to minimize the work this
function needs to do by not needing to conditionally check for the
`field_name` parameter.

Adapted from this PR:
https://github.com/appsembler/figures/pull/264/files#diff-ccfc20c64a04dae3fe94285d727a3aa2R79

And we'll need to replace the code in PR 264 with this function
"""
django_filters_version = version.parse(django_filters.__version__)
if django_filters_version < version.parse('1.0.0'):
return django_filters.CharFilter(name=field_name, lookup_type=lookup_expr)
elif django_filters_version < version.parse('2.0.0'):
return django_filters.CharFilter(name=field_name, lookup_expr=lookup_expr)
else:
return django_filters.CharFilter(field_name=field_name, lookup_expr=lookup_expr)


def char_method_filter(method):
"""
"method" is the method name string
Expand Down Expand Up @@ -77,13 +100,14 @@ class CourseOverviewFilter(django_filters.FilterSet):

'''

display_name = django_filters.CharFilter(lookup_expr='icontains')
org = django_filters.CharFilter(
name='display_org_with_default', lookup_expr='iexact')
number = django_filters.CharFilter(
name='display_number_with_default', lookup_expr='iexact')
number_contains = django_filters.CharFilter(
name='display_number_with_default', lookup_expr='icontains')
display_name = char_filter(field_name='display_name',
lookup_expr='icontains')
org = char_filter(field_name='display_org_with_default',
lookup_expr='iexact')
number = char_filter(field_name='display_number_with_default',
lookup_expr='iexact')
number_contains = char_filter(field_name='display_number_with_default',
lookup_expr='icontains')

class Meta:
model = CourseOverview
Expand Down Expand Up @@ -198,12 +222,11 @@ class UserFilterSet(django_filters.FilterSet):
is_active = django_filters.BooleanFilter(name='is_active')
is_staff = django_filters.BooleanFilter(name='is_staff')
is_superuser = django_filters.BooleanFilter(name='is_superuser')
username = django_filters.CharFilter(lookup_expr='icontains')
email = django_filters.CharFilter(lookup_expr='icontains')
name = django_filters.CharFilter(lookup_expr='icontains', name='profile__name')
username = char_filter(field_name='username', lookup_expr='icontains')
email = char_filter(field_name='email', lookup_expr='icontains')
name = char_filter(field_name='profile__name', lookup_expr='icontains')

country = django_filters.CharFilter(
name='profile__country', lookup_expr='iexact')
country = char_filter(field_name='profile__country', lookup_expr='iexact')
user_ids = char_method_filter(method='filter_user_ids')
enrolled_in_course_id = char_method_filter(method='filter_enrolled_in_course_id')

Expand Down Expand Up @@ -310,8 +333,8 @@ class SiteFilterSet(django_filters.FilterSet):
"""
Note: The Site filter has no knowledge of a default site, nor should it
"""
domain = django_filters.CharFilter(lookup_expr='icontains')
name = django_filters.CharFilter(lookup_expr='icontains')
domain = char_filter(field_name='domain', lookup_expr='icontains')
name = char_filter(field_name='name', lookup_expr='icontains')

class Meta:
model = Site
Expand Down
10 changes: 10 additions & 0 deletions tests/test_filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,16 @@
Currently uses Django TestCase style classes instead of pytest style classes
so that we can use TestCase.assertQuerysetEqual


Test Debt
=========
Field parameters 'lookup_type' and 'lookup_expr'

We are not adequately testing 'lookup_exr', which is supported only in
Django Filters 1.0.0 and greater. Prior to Django Filters 1.0.0, 'lookup_type'
was used.

Open edX Ginkgo uses Django Filteres 0.11.0.
'''

from dateutil.parser import parse as dateutil_parse
Expand Down