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: #16837 - Add EmptyStringFilter and type__empty filter on CableFilterSet #17481

Closed
wants to merge 11 commits into from

Conversation

bctiemann
Copy link
Contributor

Fixes: #16837

Adds EmptyStringFilter from the django-filter documentation's common solutions: https://django-filter.readthedocs.io/en/main/guide/tips.html#filtering-by-an-empty-string

http://127.0.0.1:8000/dcim/cables/?type__empty=true and =false will now work as expected. Filter naming (type__empty) is open to comments.

This same filter can be added to any other filtersets as needed.

Note that this only adds the ability to filter directly via URL; it does not add the empty value to the filter form.

@bctiemann bctiemann changed the title Fixes: #16837 - Add EmptyStringFilter and type__empty filter on CableFilterSet Closes: #16837 - Add EmptyStringFilter and type__empty filter on CableFilterSet Sep 12, 2024
@bctiemann
Copy link
Contributor Author

Note -- the "Magic Values" solution in the django-filter docs might actually be a better solution here, because then we could add type=EMPTY to the choices available in the filter form as it would still be filtering the type field directly and not using a separate filter path.

@bctiemann bctiemann marked this pull request as draft September 12, 2024 19:51
@bctiemann
Copy link
Contributor Author

bctiemann commented Sep 13, 2024

Changed to NullableMultipleChoiceFilter which adds the ability to filter on a value of '' among other selected values in a multi-select input in the CableFilterForm. Also reorganizes the CableTypeChoices to put USB, Power, and (unset) into "Other" to separate them from the fiber/copper types in the select.

Screenshot 2024-09-13 at 8 51 32 AM

Open questions: we have a lot of filter fields in the various filtersets which use the MultiValueCharFilter etc. series of filter classes which are decorated with @extend_schema_field(OpenApiTypes.STR) etc. It looks like these classes are newer, and they are not used comprehensively; many fields use django_filters.MultipleChoiceField and other "bare" filter classes and not these OpenAPI-decorated ones. If we add NullableMultipleChoiceFilter it feels like this is going in a different direction design-wise from what might be an incomplete migration to the OpenAPI classes, and moreover this new filter is modeled on NullableCharFilter which seems not to be used, but maybe these should be made into the superclass that MultiValueCharFilter et al inherit from?

In other words this PR addresses the immediate need specifically for Cable types but this really ought to be a system-wide design change that is consistently applied, or at least documented.

@bctiemann bctiemann self-assigned this Sep 17, 2024
@bctiemann bctiemann marked this pull request as ready for review September 18, 2024 21:26
@jeremystretch
Copy link
Member

The empty lookup is intended to be set automatically for all choice field filters under BaseFilterSet. However, that's not happening due to this check for a choices parameter. Removing this check yields a TypeError exception, however I suspect that fixing that issue will resolve this.

@bctiemann
Copy link
Contributor Author

I pushed a change that removes that check for choices (it was likely added as a bandaid after the fact to cover over that TypeError, is my gut feeling), and reworked the instantiation of the filter class which is then added to the filters of the filterset class.

However: we need to understand/communicate the difference in use cases here. Only query params where the key is exactly the same are OR'd together; all the rest are AND'ed (each is applied sequentially via a queryset = queryset.filter(...) construct). This means you can do this query:

?type=smf&type=null

And you will get BOTH smf and '' results in your list. But if you go

?type=smf&type__empty=true

Then you won't get any results, because it's trying to AND two separate filter operations together (type and type__empty).

The way I set up the NullableMultipleChoiceFilter for use in the filter form allows the "unset" value to be used as one of several values to be OR'd together: ['smf', 'cat3', '']. While this latest commit enables type__empty=true to work if placed in the URL directly, it isn't a good solution for the original use case of this ticket, and I think the nullable filter still has value we'd otherwise be missing.

@bctiemann
Copy link
Contributor Author

Closing this PR in favor of #17574. Will open a new PR against a feature request for NullableMultipleChoiceFilter.

@bctiemann bctiemann closed this Sep 23, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cable type empty filtering not working
2 participants