eliminate shadow fields and correct filter names #2847

HanlinMiao · 2022-11-16T04:36:12Z

Closes: #2804

What's Changed

This PR is dedicated to:

Collapse some of the *_id filters with NaturalKeyOrPKMultipleChoiceFilter
Correct shadowed filters e.g. interfaces should not be the same as has_interfaces and should be its own NaturalKeyOrPKMultipleChoiceFilter
Rename some of the filters and filter types.
Not going to add missing filters

See also related PRs -
#2845 -- closed as duplicate to #2798
#2846 -- collapse site_id and region_id to site and region NaturalKeyOrPKMultipleChoiceFilter.

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

Tactical PR to merge into next via #2849

HanlinMiao · 2022-11-16T06:33:14Z

nautobot/dcim/forms.py

@@ -2253,6 +2253,11 @@ class DeviceFilterForm(
        label="Has a primary IP",
        widget=StaticSelect2(choices=BOOLEAN_WITH_BLANK_CHOICES),
    )
+    has_interfaces = forms.NullBooleanField(


I have to add this to make DynamicGroupModelTest pass since DG derives the filters from filterset_form.

Something seems off here. You didn't change the has_interfaces filter definition, so why did this need to be added? And are there a bunch of other filters that also need to be added/updated in the forms now?

has_interfaces is a special case, since it was the one that sparked the “we shouldn’t shadow related names and change their intent/type” initiative when it was originally named interfaces. All of the other has_foo filters were added later if they didn't already exist. For DeviceFilterSet, however, I think this is the only one that didn't already have a has_foo form field because it would have been confusing in v1.x.

glennmatthews · 2022-11-16T14:41:16Z

nautobot/core/tests/test_graphql.py

@@ -1210,7 +1210,7 @@ def test_query_device_role_filter(self):
            # pylint: disable=consider-using-f-string
            """
                query {
-                    devices(role: "%s") {
+                    devices(device_role: "%s") {


I'm wondering - with the proposed shift to a generic Role model replacing DeviceRole and others, maybe this should stay as role? Food for thought.

glennmatthews · 2022-11-16T14:45:00Z

nautobot/dcim/filters.py

        queryset=RackGroup.objects.all(),
        field_name="rack__group",
-        label="Rack group (ID)",
+        label="Rack group (slug or ID)",
    )
    rack_id = django_filters.ModelMultipleChoiceFilter(


Should rack become a NaturalKeyOrPKMultipleChoiceFilter?

glennmatthews · 2022-11-16T14:45:28Z

nautobot/dcim/filters.py

@@ -1002,11 +931,10 @@ class DeviceFilterSet(
        queryset=Cluster.objects.all(),
        label="VM cluster (ID)",


Should cluster become a NaturalKeyOrPKMultipleChoiceFilter?

glennmatthews · 2022-11-16T14:45:42Z

nautobot/dcim/filters.py

@@ -1002,11 +931,10 @@ class DeviceFilterSet(
        queryset=Cluster.objects.all(),
        label="VM cluster (ID)",
    )
-    model = django_filters.ModelMultipleChoiceFilter(
-        field_name="device_type__slug",
+    model = NaturalKeyOrPKMultipleChoiceFilter(


This should become device_type, I believe?

it does have a device_type filter defined on line 912? Could this be something different or a duplicate? Kinda looks like a duplicate.

glennmatthews · 2022-11-16T14:46:03Z

nautobot/dcim/filters.py

        queryset=SecretsGroup.objects.all(),
-        to_field_name="slug",
-        label="Secrets group (slug)",
+        label="Secrets group (slug or ID)",
    )
    virtual_chassis_id = django_filters.ModelMultipleChoiceFilter(


virtual_chassis as NaturalKeyOrPKMultipleChoiceFilter?

glennmatthews · 2022-11-16T14:50:43Z

nautobot/dcim/filters.py

@@ -1156,23 +1118,19 @@ class DeviceComponentFilterSet(CustomFieldModelFilterSet):
        to_field_name="slug",


Not changing the region and site filters? (Ah, I see - that's in #2846. It would be good to update the description of this PR to clearly indicate what it is and is not targeting for change.)

glennmatthews · 2022-11-16T14:57:42Z

nautobot/dcim/filters.py

-        to_field_name="slug",
-        label="Role (slug)",
-    )
+    role = NaturalKeyOrPKMultipleChoiceFilter(queryset=RackRole.objects.all(), label="Role (slug or ID)")
    serial = django_filters.CharFilter(lookup_expr="iexact")


Doesn't serial need to be changed to a MultiValueCharFilter?

glennmatthews · 2022-11-16T14:58:21Z

nautobot/dcim/filters.py

@@ -552,22 +526,17 @@ class RackReservationFilterSet(NautobotFilterSet, TenancyFilterSet):
        field_name="rack__group",


elimination of group_id filter?

glennmatthews · 2022-11-16T14:59:02Z

nautobot/dcim/filters.py

-        to_field_name="slug",
-        label="Manufacturer (slug)",
+    manufacturer = NaturalKeyOrPKMultipleChoiceFilter(
+        queryset=Manufacturer.objects.all(), label="Manufacturer (slug or ID)"
    )
    has_devices = RelatedMembershipBooleanFilter(


Addition of devices and virtual_machines filters?

I think the scope of the PR is to fix incorrect filters and not adding missing ones. I will elaborate this in the description.

glennmatthews · 2022-11-16T15:02:27Z

nautobot/dcim/forms.py

@@ -2253,6 +2253,11 @@ class DeviceFilterForm(
        label="Has a primary IP",
        widget=StaticSelect2(choices=BOOLEAN_WITH_BLANK_CHOICES),
    )
+    has_interfaces = forms.NullBooleanField(


Something seems off here. You didn't change the has_interfaces filter definition, so why did this need to be added? And are there a bunch of other filters that also need to be added/updated in the forms now?

jathanism · 2022-11-16T16:55:29Z

nautobot/dcim/filters.py

@@ -254,7 +248,7 @@ class SiteFilterSet(NautobotFilterSet, TenancyFilterSet, StatusModelFilterSetMix
        label="Time zone",
        null_value="",
    )
-    tag = TagFilter()
+    tags = TagFilter()


Can you please humor me and remove this definition from this filterset and see if it behaves the same?

It's what I referenced yesterday in standup, that TaggableManager fields are already automatically mapped to TagFilter so I'm hoping it should Just Work™. See:

nautobot/nautobot/utilities/filters.py

Line 570 in 4168c37

TaggableManager: {"filter_class": TagFilter},

In other words, as long as "tags" is provided in Meta.fields for the filterset, I'm pretty certain that it won't need to be overloaded on the filterset class itself. Thanks!

jathanism · 2022-11-16T19:18:09Z

In the 2.0 TODO review this came up. Possible related here?

nautobot/extras/tests/test_dynamicgroups.py

# 2.0 TODO(jathan): When "serial" becomes a multi-value filter, this will need to be revised or removed.

nautobot/dcim/filters.py

# 2.0 TODO: Remove me and pass_through_portsin exchange forhas_(front|rear)_ports. - relevant to eliminate shadow fields and correct filter names #2847?

…ltiValueCharFilter

…of NaturalKeyOrMultipleChoiceField

…ceFilterSet

jathanism

I have some minor asks for style, but this looks good-to-go! 🚢 🇮🇹

nautobot/extras/tests/test_dynamicgroups.py

nautobot/utilities/filters.py

…ittest using pass_throgh_ports filter that no longer exists

eliminate shadow fields and correct filter names

9621269

HanlinMiao linked an issue Nov 16, 2022 that may be closed by this pull request

Change Shadowed Fields on Existing DCIM FilterSets #2804

Closed

HanlinMiao mentioned this pull request Nov 16, 2022

Changed shadowed filterset fields in dcim filterset to use NaturalKeyOrPKMultipleChoiceFilter and modified related unittests. #2813

Closed

2 tasks

Hanlin Miao added 6 commits November 15, 2022 23:43

flake8 duplicate unittests

64e421d

fix unittest

aa69f94

unittest

01074df

fix core unittests

b6ec07c

more unittests

19869db

get rid of print statement

f93efcc

HanlinMiao commented Nov 16, 2022

View reviewed changes

bryanculver mentioned this pull request Nov 16, 2022

Fix DCIM Shadowed Filter Fields #2849

Merged

6 tasks

glennmatthews reviewed Nov 16, 2022

View reviewed changes

glennmatthews mentioned this pull request Nov 16, 2022

Review 2.0 TODO Code Comments #2582

Closed

Hanlin Miao added 2 commits November 16, 2022 11:53

revert device_role change and address some feedback

e1cc605

merge region_id and site_id changes

25221af

jathanism reviewed Nov 16, 2022

View reviewed changes

Hanlin Miao added 2 commits November 16, 2022 12:45

elminiate tags filters, unittests and flake8

493d8eb

flake8

f04abf6

Hanlin Miao added 5 commits November 16, 2022 21:21

fix schema and dynamic group tests and changed serial field to use Mu…

9ac6710

…ltiValueCharFilter

make reverse relation fields to use ModelMultipleChoiceField instead …

ecda587

…of NaturalKeyOrMultipleChoiceField

changed unittest based on DeviceFilterSet changes

cad4b9c

black formatting was turned off for some reason

6403742

eliminate pass_through_ports filter and its affiliated method on Devi…

9875062

…ceFilterSet

jathanism approved these changes Nov 17, 2022

View reviewed changes

nautobot/extras/tests/test_dynamicgroups.py Show resolved Hide resolved

nautobot/utilities/filters.py Outdated Show resolved Hide resolved

Eliminate unnecessary todo comments and blank lines and changed DG un…

b1957bf

…ittest using pass_throgh_ports filter that no longer exists

jathanism approved these changes Nov 18, 2022

View reviewed changes

glennmatthews approved these changes Nov 18, 2022

View reviewed changes

HanlinMiao merged commit 2e80839 into hanlin-#2804 Nov 18, 2022

bryanculver deleted the hanlin-#2804-eliminate-shadow-filters branch December 5, 2022 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eliminate shadow fields and correct filter names #2847

eliminate shadow fields and correct filter names #2847

HanlinMiao commented Nov 16, 2022 •

edited

HanlinMiao Nov 16, 2022

glennmatthews Nov 16, 2022

jathanism Nov 16, 2022

glennmatthews Nov 16, 2022

jathanism Nov 16, 2022

glennmatthews Nov 16, 2022

glennmatthews Nov 16, 2022

glennmatthews Nov 16, 2022

HanlinMiao Nov 16, 2022

glennmatthews Nov 16, 2022

glennmatthews Nov 16, 2022

glennmatthews Nov 16, 2022

glennmatthews Nov 16, 2022

glennmatthews Nov 16, 2022

HanlinMiao Nov 16, 2022

glennmatthews Nov 16, 2022

jathanism Nov 16, 2022

jathanism commented Nov 16, 2022 •

edited

jathanism left a comment

		@@ -1002,11 +931,10 @@ class DeviceFilterSet(
		queryset=Cluster.objects.all(),
		label="VM cluster (ID)",

		@@ -1156,23 +1118,19 @@ class DeviceComponentFilterSet(CustomFieldModelFilterSet):
		to_field_name="slug",

		@@ -552,22 +526,17 @@ class RackReservationFilterSet(NautobotFilterSet, TenancyFilterSet):
		field_name="rack__group",

eliminate shadow fields and correct filter names #2847

eliminate shadow fields and correct filter names #2847

Conversation

HanlinMiao commented Nov 16, 2022 • edited

Closes: #2804

What's Changed

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jathanism commented Nov 16, 2022 • edited

jathanism left a comment

Choose a reason for hiding this comment

HanlinMiao commented Nov 16, 2022 •

edited

jathanism commented Nov 16, 2022 •

edited