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

Closes: #17575 - Add NullableMultipleChoiceFilter and filter form UI to match empty string #17576

Closed
wants to merge 8 commits into from

Conversation

bctiemann
Copy link
Contributor

Closes: #17575

Adds a NullableMultipleChoiceFilter class (patterned on the existing NullableCharFieldFilter) which can be used in a multiple-choice FilterForm input to add the "(unset)" ('') value to a list of acceptable choices in the filter, evaluated in an OR fashion in a single query.

@bctiemann
Copy link
Contributor Author

See #17481 which this is adapted from. Also #17574 should be merged first.

@bctiemann bctiemann self-assigned this Sep 23, 2024
@bctiemann bctiemann force-pushed the 17575-nullable-multiple-choice-filter branch from 2ad87cb to c2473e6 Compare October 11, 2024 17:49
@@ -1980,7 +1980,7 @@ class CableFilterSet(TenancyFilterSet, NetBoxModelFilterSet):
method='_unterminated',
label=_('Unterminated'),
)
type = django_filters.MultipleChoiceFilter(
type = NullableMultipleChoiceFilter(
Copy link
Member

Choose a reason for hiding this comment

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

This addresses the type field on Cable specifically, but I think what we need is a general implementation which covers all non-required choice fields, across all models. Ideally, this should be handled automatically for all model fields with a choices attribute defined, which would obviate the need to manually declare these on the FilterSets as we currently do. (Granted, I'm not sure whether that's feasible.)

Comment on lines +151 to +154
def filter(self, qs, value):
if settings.FILTERS_NULL_CHOICE_VALUE in value:
value.append('')
return super().filter(qs, value)
Copy link
Member

Choose a reason for hiding this comment

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

It feels like we need to further consider the implications of storing empty strings rather than null values for these fields. django-filter has native support for the later; replacing empty strings with null would obviate the need for a custom filter entirely AFAICT. Maybe that should be our preferred strategy.

@jeremystretch jeremystretch added the status: blocked Another issue or external requirement is preventing implementation label Oct 16, 2024
@bctiemann
Copy link
Contributor Author

Closing as this is no longer needed due to #17761.

@bctiemann bctiemann closed this Oct 22, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: blocked Another issue or external requirement is preventing implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to filter for empty string values in an "OR" fashion
2 participants