carltongibson / django-filter

A generic system for filtering Django QuerySets based on user selections
https://django-filter.readthedocs.io/en/main/
Other
4.46k stars 769 forks source link

`filter_overrides` `extra` is not applied when the field has a `choices` attribute #1475

Open Spycho opened 2 years ago

Spycho commented 2 years ago

Observed behaviour

filter_overrides extra is not applied when the field has a choices attribute. I'm trying to use this to add a bootstrap class (form-select) to my select input, but the rendered select has no class.

Expected behaviour

filter_overrides extra is applied in all cases, and my bootstrap class makes it onto the rendered select element.

Cause

I think this stems from lines 406 and 407 of filterset.py:

if lookup_type == 'exact' and getattr(field, 'choices', None):
    return ChoiceFilter, {'choices': field.choices}

This ignores the params attribute which is defined on line 399: params = data.get('extra', lambda field: {})(field)

Fix

I think we need to combine the params dict with the {'choices': field.choices} on line 407, e.g. change line 407 to return ChoiceFilter, {'choices': field.choices} | params (or {'choices': field.choices, **params} if you want to support Python versions <3.9). I'd suggest merging dictionaries in this order, as that would then also allow filter_overrides to override choices.

I've tested this change locally, and it resolves the issue.

I'm happy to make a pull request for that change if that would be of use?

Spycho commented 2 years ago

Added this pull request which fixes the issue: https://github.com/carltongibson/django-filter/pull/1476. Would be useful if we could get this merged and released so I don't have to faff around building and distributing the lib from my fork.

carltongibson commented 2 years ago

Hi @Spycho — Thanks.

…or {'choices': field.choices, **params}

This doesn't look like it breaks anything with a quick look. If you want to open a PR, adding a regression test for the case you're fixing, that would be great.

Spycho commented 2 years ago

Thanks @carltongibson. Already raised PR - #1476. It has the minor change above and a regression test.