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 ManyToMany field default filter type #1287

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

weilu
Copy link

@weilu weilu commented Jan 5, 2022

It's a bug introduced in this commit: 2d4ca0a (part of PR #1119) It manifests as not accepting a list of IDs [ID] but rather a single ID ID as filtering arguments for a m2m field. The bug can be replicated by adding a many-to-many relationship in the cookbook example app. Happy to clean up what I have locally and push it to a branch for quick reference if necessary.

I do want to add test cases to this PR but I thought it's best/fastest to have @zbyte64 or @tcleonard point me to where's the best place to add it as they are more familiar with the existing fixtures & setup. Happy to add on.

Also, side question to @tcleonard on his modification of the function get_filtering_args_from_filterset in the above-mentioned commit, why adding all the type checking branching code in this method itself rather than expanding/modifying model_field.formfield as based on my quick read that seems to be where the logic of translating model_field to form_field is intended to live?

@keithhackbarth
Copy link
Collaborator

@weilu - Great work. As you called out already, it would be helpful to see a breaking test here as an example. You can see the existing tests in filter/tests and I think you can use that format as a guide to how to write one for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants