Skip to content
New issue

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

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

Already on GitHub? # to your account

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

Open
Spycho opened this issue Feb 9, 2022 · 3 comments · May be fixed by #1476
Open

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

Spycho opened this issue Feb 9, 2022 · 3 comments · May be fixed by #1476

Comments

@Spycho
Copy link

Spycho commented Feb 9, 2022

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 pushed a commit to Spycho/django-filter that referenced this issue Feb 9, 2022
@Spycho
Copy link
Author

Spycho commented Feb 9, 2022

Added this pull request which fixes the issue: #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
Copy link
Owner

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
Copy link
Author

Spycho commented Feb 11, 2022

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants