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

Fixes to onboarding Cluster, IPAddress, Prefix, Rack to Dynamic Groups #2200

Merged
merged 12 commits into from
Aug 15, 2022

Conversation

jathanism
Copy link
Contributor

@jathanism jathanism commented Aug 12, 2022

Replaces: #2188

What's Changed

  • DynamicGroup._map_filter_fields now explicitly skips any method filters. This is a stop-gap until we are able to vet every method filter on every filterset. This is because we moved to Q object filtering for nested groups.
  • Introduced a new pattern for filterset method filters to be backed by a corresponding generate_query_{filter_method} method that itself emits a Q object that can be used by DynamicGroup.generate_query() machinery
  • Specific filterset fields have been revised to have these generate_query_ filterset methods defined to maintain feature parity w/ v1.3 Dynamic Groups (in the case of Device/VM), and list view filtering (in the case of Rack, IPAddress, Prefix)
  • Fixed a bug in which method filters would only be filtered out if they were found to be a missing field

TODO

  • Explanation of Change(s)
  • 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

mzbroch and others added 3 commits August 11, 2022 22:18
…ic Groups

- `DynamicGroup._map_filter_fields` now explicitly skips any method filters. This is a stop-gap until we are able to vet every method filter on every filterset. This is because we moved to Q object filtering for nested groups.
- Introduced a new pattern for filterset method filters to be backed by a corresponding `generate_query_{filter_method}` method that itself emits a `Q` object that can be used by `DynamicGroup.generate_query()` machinery
 - Specific filterset fields have been revised to have these `generate_query_` filterset methods defined to maintain feature parity w/ v1.3 Dynamic Groups (in the case of Device/VM), and list view filtering (in the case of Rack, IPAddress, Prefix)
- Fixed a bug in which method filters would only be filtered out if they were found to be a missing field
Comment on lines +323 to +324
# This stringification is done to make the `pretty_print_query()` output look human-readable,
# and nothing more. It would work as complex objects but looks bad in the web UI.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still an issue after #2197?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes:

image

Compared to your fixes with leaving the stringification in there:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, thanks. Not blocking but wonder if we can/should fix this in pretty_print_query() itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think that is probably better so it's not up to plugin authors or us in the future.

nautobot/extras/tests/test_dynamicgroups.py Outdated Show resolved Hide resolved
nautobot/extras/tests/test_dynamicgroups.py Show resolved Hide resolved
@bryanculver bryanculver mentioned this pull request Aug 13, 2022
12 tasks
@mzbroch
Copy link
Contributor

mzbroch commented Aug 13, 2022

Thanks Jathan! User experience looks much better now!

nautobot/ipam/models.py Outdated Show resolved Hide resolved
- Parent groups that had null filters were linking to an empty list view
- `DynamicGroup.get_group_membership_url()` has been updated to always link to the "Members" tab on the detail view for that group.
- `DynamicGroup.members_base_url` has been eliminated.
bryanculver and others added 2 commits August 15, 2022 15:36
Co-authored-by: Jathan McCollum <jathan@gmail.com>
@bryanculver bryanculver merged commit 17e8151 into develop Aug 15, 2022
@jathanism jathanism deleted the jathanism-mzb-fixes-v2 branch August 15, 2022 20:45
@bryanculver
Copy link
Member

Note here: We skipped automatic field population on a few of these models due in part to this: #2162 (ex: rack_depth on Rack)

briddo pushed a commit that referenced this pull request Aug 16, 2022
…ic Groups (#2200)

- `DynamicGroup._map_filter_fields` now explicitly skips any method filters. This is a stop-gap until we are able to vet every method filter on every filterset. This is because we moved to Q object filtering for nested groups.
- Introduced a new pattern for filterset method filters to be backed by a corresponding `generate_query_{filter_method}` method that itself emits a `Q` object that can be used by `DynamicGroup.generate_query()` machinery
 - Specific filterset fields have been revised to have these `generate_query_` filterset methods defined to maintain feature parity w/ v1.3 Dynamic Groups (in the case of Device/VM), and list view filtering (in the case of Rack, IPAddress, Prefix)
- Fixed a bug in which method filters would only be filtered out if they were found to be a missing field
- Parent groups that had null filters were linking to an empty list view
- `DynamicGroup.get_group_membership_url()` has been updated to always link to the "Members" tab on the detail view for that group.
- `DynamicGroup.members_base_url` has been eliminated.

Co-authored-by: Marek Zbroch <marek.zbroch@networktocode.com>
Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>
briddo pushed a commit that referenced this pull request Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants