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

Fix null filtering for *Choice filters #680

Merged
merged 6 commits into from
Aug 31, 2017

Conversation

rpkilby
Copy link
Collaborator

@rpkilby rpkilby commented Apr 8, 2017

This is an update that should supersede/close #639. Thanks @michael-mri for an initial implementation and some test cases.


TODO:

In short, the null option handling is moved to a set of custom fields and out of the ChoiceFilter. This code largely emulates and extends what's done by forms.ModelChoiceField already. eg, the ChoiceIterator basically copies the behavior of ModelChoiceIterator and adds empty_label handling to ChoiceField & MultipleChoiceField. Null choice handling is just an extension of the empty choice.

Notes:

  • The ChoiceIteratorMixin is probably the most confusing bit to reason about, due to how ModelChoiceFields can set both queryset and choices.
  • ModelChoiceField & ModelMultipleChoiceField both validate the incoming values as part of the queryset. This check essentially has to be bypassed for null_values.
  • Unlike, ModelChoiceField, ChoiceField doesn't already support empty_label (added in the PR).
  • MultipleChoiceField explicitly sets empty_label to none, since it's an option that never makes sense. This is the same behavior as forms.ModelMultipleChoiceField.
  • The auto-generation explicitly disables the null choice for non-nullable fields.
  • There is no null_label handling for ManyToManyFields in FILTER_FOR_DBFIELD_DEFAULTS since the null option is not present/implicit.
  • Since the empty option handling is now provided by the field, custom filters can create their own choices without having to explicitly handle these options (eg, AllValuesFilter, which should be fixed).

@codecov-io
Copy link

codecov-io commented Apr 8, 2017

Codecov Report

Merging #680 into develop will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #680      +/-   ##
===========================================
+ Coverage    98.11%   98.21%   +0.09%     
===========================================
  Files           15       15              
  Lines         1117     1179      +62     
===========================================
+ Hits          1096     1158      +62     
  Misses          21       21
Impacted Files Coverage Δ
django_filters/filterset.py 100% <ø> (ø) ⬆️
django_filters/fields.py 100% <100%> (ø) ⬆️
django_filters/filters.py 98.44% <100%> (-0.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 645f24a...50f7275. Read the comment docs.

@rpkilby
Copy link
Collaborator Author

rpkilby commented Jul 25, 2017

Hi @carltongibson - finally got around to writing the test for #710. This PR should be ready for review now.

@carltongibson
Copy link
Owner

OK. Great thanks. I'll review all, and roll a new release shorty*

[*] ≈Before end of Summer

@rpkilby
Copy link
Collaborator Author

rpkilby commented Aug 22, 2017

rebased

Copy link
Owner

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely. Lets have it! 👍

@carltongibson carltongibson merged commit d4b181c into carltongibson:develop Aug 31, 2017
@rpkilby rpkilby deleted the null-filtering branch August 31, 2017 08:07
carltongibson pushed a commit that referenced this pull request Oct 19, 2017
* Add test cases from PR 639. Thanks @michael-mri!

* Fix tests for null choice iterator

* Add more null value tests

* Add null choice to various (Model)ChoiceFields

* Fix django < 1.11 field iterator handling

* Resolve #710 - add test for lazy callable choices
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants