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.

Sign up for GitHub

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

Jump to bottom

Fix BaseFilterSet not using multiple choice filters #3342

Merged
merged 8 commits into from
Mar 2, 2023
Merged

Fix BaseFilterSet not using multiple choice filters #3342

merged 8 commits into from
Mar 2, 2023

Conversation

timizuoebideri1
Copy link
Contributor

@timizuoebideri1 timizuoebideri1 commented Feb 20, 2023 •
edited
Loading

Closes: #3214

What's Changed

  • Override BaseFilterSet.filter_for_lookup to add logic that checks if the lookup type is exact and if the field has attribute choices (as only IntegerField/CharField with choices can have this attribute), and returns MulipleChoiceFilter if the conditions are meet.
  • I also removed this block of code elif existing filter.extra.get("choices") because we now expect the choice field to be MulipleChoiceFilter, which would require all relevant lookup expressions of a MulipleChoiceFilter to be included.

TODO

  • Explanation of Change(s)
  • Added change log fragment(s) (for more information see the documentation)
  • Attached Screenshots, Payload Example
  • Unit, Integration Tests
  • Documentation Updates (when adding/changing features)
  • Example Plugin Updates (when adding/changing features)
  • Outline Remaining Work, Constraints from Design

jathanism
jathanism approved these changes Feb 23, 2023
View reviewed changes
nautobot/utilities/filters.py Outdated Show resolved Hide resolved
glennmatthews
glennmatthews reviewed Feb 23, 2023
View reviewed changes
nautobot/utilities/filters.py Outdated Show resolved Hide resolved
nautobot/utilities/filters.py Outdated Show resolved Hide resolved
nautobot/utilities/tests/test_filters.py Outdated Show resolved Hide resolved
nautobot/extras/tests/test_filters.py Show resolved Hide resolved
@timizuoebideri1 timizuoebideri1 changed the base branch from develop to next February 25, 2023 12:56
timizuoebideri1 and others added 3 commits February 25, 2023 13:57
@timizuoebideri1
Apply suggestions from code review
cf1fa6c
@timizuoebideri1
Merge branch 'next' into fix_basefilterser_multiple_choice_filter
19daae1 
# Conflicts:
#	nautobot/circuits/tests/test_filters.py
#	nautobot/core/filters.py
#	nautobot/core/tests/test_filters.py
#	nautobot/dcim/tests/test_filters.py
@timizuoebideri1
Fix failing test
3ace1a5
glennmatthews
glennmatthews reviewed Feb 27, 2023
View reviewed changes
Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

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

Does this have any impact on the use of these filters in the UI, or how they are presented in the advanced filtering form?

glennmatthews
glennmatthews requested changes Feb 27, 2023
View reviewed changes
Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

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

Would like to clarify the impact of this on the UI and forms before it gets merged.

@jathanism
Copy link
Contributor

Would like to clarify the impact of this on the UI and forms before it gets merged.

With screenshots? :)

@timizuoebideri1
Copy link
Contributor Author

It does not seem to have any impact on UI filtering both on Default and Advance

Screenshot 2023-02-27 at 23 21 39
Screenshot 2023-02-27 at 23 21 45
Screenshot 2023-02-27 at 23 21 49
Screenshot 2023-02-27 at 23 21 57
Screenshot 2023-02-27 at 23 23 07

glennmatthews
glennmatthews approved these changes Feb 28, 2023
View reviewed changes
nautobot/dcim/tests/test_filters.py
@@ -1087,7 +1087,7 @@ def test_outer_unit(self):
with self.subTest():
self.assertEqual(Rack.objects.exclude(outer_unit="").count(), 3)
with self.subTest():
params = {"outer_unit": RackDimensionUnitChoices.UNIT_MILLIMETER}
params = {"outer_unit": [RackDimensionUnitChoices.UNIT_MILLIMETER]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be able to change most of these affected tests to entries in generic_filter_test rather than separate test functions now that these are multi-value filters, though it may take a bit of modifying to the test data to make it diverse enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not need to be done on this PR right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I won't block this PR on waiting for this change but it should be a pretty straightforward one to do. Fine with me if you want to open a new issue or add a comment on #3213 calling out these tests.

@timizuoebideri1 timizuoebideri1 mentioned this pull request Mar 2, 2023
Open
@timizuoebideri1 timizuoebideri1 merged commit 3c7b04b into next Mar 2, 2023
@timizuoebideri1 timizuoebideri1 deleted the fix_basefilterser_multiple_choice_filter branch March 2, 2023 11:50
This was referenced Mar 6, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glennmatthews glennmatthews glennmatthews approved these changes

@bryanculver bryanculver bryanculver approved these changes

@jathanism jathanism Awaiting requested review from jathanism

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BaseFilterSet not using multiple choice filters for CharFields with choices
4 participants
@timizuoebideri1 @jathanism @bryanculver @glennmatthews