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 DynamicGroup Serialization Bug #4178

Merged

Conversation

bryanculver
Copy link
Member

Closes: #DNE

What's Changed

Worked with @lampwins to reproduce this bug.

TLDR: When a Dynamic Group is being saved, there is an edge case where a Filter Form and a FilterSet Form are not identical. A way to reproduce this is with: https://github.com/nautobot/nautobot/tree/u/bryanculver-dg-bug and creating the necessary relationship to filter on it. What happened is the nested filter through the relationship and ultimately is filtering on the name of an FK, the FilterSet Form expects inputs to be a list of strings (it's a MultiValueCharFilter after all). But for UX we want to have a selection of names of object instances. We can do this by using the to_field_name and processing accordingly. We have logic in Dynamic Groups that does this if the FilterSetForm filed is of the right type. The FilterForm is helping us to this today, but the FilterSetForms need some work to be returning/use the smarter Form field types.

This fix allows to use the FilterForm field first if it's available and falls back if it's not.

Special handling had to be done for Tags because our Tag filter form field isn't a ModelMultipleChoiceField; the FilterSetForm field before was using a ModelMultipleChoiceField.

I'm open to ideas on how to write a test for this which doesn't require a bunch of complex setup like in the branch referenced above.

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

@bryanculver bryanculver requested review from jathanism and a team July 28, 2023 18:15
@bryanculver bryanculver marked this pull request as ready for review July 31, 2023 13:36
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 it make sense to add a unit/integration test to check for this bug?

nautobot/extras/models/groups.py Show resolved Hide resolved
nautobot/extras/models/groups.py Show resolved Hide resolved
nautobot/extras/models/groups.py Show resolved Hide resolved
nautobot/utilities/forms/fields.py Show resolved Hide resolved
@bryanculver bryanculver added the type: bug Something isn't working as expected label Jul 31, 2023
@bryanculver bryanculver requested review from a team and glennmatthews July 31, 2023 20:01
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.

Needs change fragment, otherwise looks solid.

@glennmatthews glennmatthews merged commit aaa8b8e into develop Aug 1, 2023
17 checks passed
@glennmatthews glennmatthews deleted the u/bryanculver-fix-dynamic-group-serialization-bug branch August 1, 2023 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working as expected
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants