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

Closes: #17575 - Add NullableMultipleChoiceFilter and filter form UI to match empty string #17576

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
8 changes: 6 additions & 2 deletions netbox/dcim/choices.py
Original file line number Diff line number Diff line change
Expand Up @@ -1514,8 +1514,12 @@ class CableTypeChoices(ChoiceSet):
(TYPE_AOC, 'Active Optical Cabling (AOC)'),
),
),
(TYPE_USB, _('USB')),
(TYPE_POWER, _('Power')),
(
_('Other'), (
(TYPE_USB, _('USB')),
(TYPE_POWER, _('Power')),
)
)
)


Expand Down
4 changes: 2 additions & 2 deletions netbox/dcim/filtersets.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
from users.models import User
from utilities.filters import (
ContentTypeFilter, MultiValueCharFilter, MultiValueMACAddressFilter, MultiValueNumberFilter, MultiValueWWNFilter,
NumericArrayFilter, TreeNodeMultipleChoiceFilter,
NumericArrayFilter, TreeNodeMultipleChoiceFilter, NullableMultipleChoiceFilter,
)
from virtualization.models import Cluster, ClusterGroup
from vpn.models import L2VPN
Expand Down Expand Up @@ -1980,7 +1980,7 @@ class CableFilterSet(TenancyFilterSet, NetBoxModelFilterSet):
method='_unterminated',
label=_('Unterminated'),
)
type = django_filters.MultipleChoiceFilter(
type = NullableMultipleChoiceFilter(
Copy link
Member

Choose a reason for hiding this comment

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

This addresses the type field on Cable specifically, but I think what we need is a general implementation which covers all non-required choice fields, across all models. Ideally, this should be handled automatically for all model fields with a choices attribute defined, which would obviate the need to manually declare these on the FilterSets as we currently do. (Granted, I'm not sure whether that's feasible.)

choices=CableTypeChoices
)
status = django_filters.MultipleChoiceFilter(
Expand Down
4 changes: 2 additions & 2 deletions netbox/dcim/forms/filtersets.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from netbox.forms import NetBoxModelFilterSetForm
from tenancy.forms import ContactModelFilterForm, TenancyFilterForm
from users.models import User
from utilities.forms import BOOLEAN_WITH_BLANK_CHOICES, FilterForm, add_blank_choice
from utilities.forms import BOOLEAN_WITH_BLANK_CHOICES, FilterForm, add_blank_choice, add_empty_filtering_choice
from utilities.forms.fields import ColorField, DynamicModelMultipleChoiceField, TagFilterField
from utilities.forms.rendering import FieldSet
from utilities.forms.widgets import NumberWithOptions
Expand Down Expand Up @@ -1052,7 +1052,7 @@ class CableFilterForm(TenancyFilterForm, NetBoxModelFilterSetForm):
)
type = forms.MultipleChoiceField(
label=_('Type'),
choices=add_blank_choice(CableTypeChoices),
choices=add_empty_filtering_choice(add_blank_choice(CableTypeChoices)),
required=False
)
status = forms.MultipleChoiceField(
Expand Down
7 changes: 4 additions & 3 deletions netbox/dcim/tests/test_filtersets.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from django.test import TestCase
from django.conf import settings

from circuits.models import Circuit, CircuitTermination, CircuitType, Provider
from dcim.choices import *
Expand Down Expand Up @@ -5240,10 +5241,10 @@ def test_length_unit(self):
def test_type(self):
params = {'type': [CableTypeChoices.TYPE_CAT3, CableTypeChoices.TYPE_CAT5E]}
self.assertEqual(self.filterset(params, self.queryset).qs.count(), 4)
params = {'type__empty': 'true'}
params = {'type': [settings.FILTERS_NULL_CHOICE_VALUE]}
self.assertEqual(self.filterset(params, self.queryset).qs.count(), 8)
params = {'type__empty': 'false'}
self.assertEqual(self.filterset(params, self.queryset).qs.count(), 6)
params = {'type': [settings.FILTERS_NULL_CHOICE_VALUE, CableTypeChoices.TYPE_CAT3]}
self.assertEqual(self.filterset(params, self.queryset).qs.count(), 10)

def test_status(self):
params = {'status': [LinkStatusChoices.STATUS_CONNECTED]}
Expand Down
11 changes: 11 additions & 0 deletions netbox/utilities/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
'MultiValueTimeFilter',
'MultiValueWWNFilter',
'NullableCharFieldFilter',
'NullableMultipleChoiceFilter',
'NumericArrayFilter',
'TreeNodeMultipleChoiceFilter',
)
Expand Down Expand Up @@ -143,6 +144,16 @@ def filter(self, qs, value):
return qs.distinct() if self.distinct else qs


class NullableMultipleChoiceFilter(django_filters.MultipleChoiceFilter):
"""
Similar to NullableCharFieldFilter, but allows multiple values including the special NULL string.
"""
def filter(self, qs, value):
if settings.FILTERS_NULL_CHOICE_VALUE in value:
value.append('')
return super().filter(qs, value)
Comment on lines +151 to +154
Copy link
Member

Choose a reason for hiding this comment

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

It feels like we need to further consider the implications of storing empty strings rather than null values for these fields. django-filter has native support for the later; replacing empty strings with null would obviate the need for a custom filter entirely AFAICT. Maybe that should be our preferred strategy.



class NumericArrayFilter(django_filters.NumberFilter):
"""
Filter based on the presence of an integer within an ArrayField.
Expand Down
10 changes: 10 additions & 0 deletions netbox/utilities/forms/utils.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import re

from django import forms
from django.conf import settings
from django.forms.models import fields_for_model
from django.utils.translation import gettext as _

Expand All @@ -10,6 +11,7 @@

__all__ = (
'add_blank_choice',
'add_empty_filtering_choice',
'expand_alphanumeric_pattern',
'expand_ipaddress_pattern',
'form_from_model',
Expand Down Expand Up @@ -189,6 +191,14 @@ def add_blank_choice(choices):
return ((None, '---------'),) + tuple(choices)


def add_empty_filtering_choice(choices):
"""
Add an empty (null) choice to the end of a choices list, to be used in filtering classes
such as NullableMultipleChoiceFilter to match on an empty value.
"""
return tuple(choices) + ((settings.FILTERS_NULL_CHOICE_VALUE, '(unset)'),)


def form_from_model(model, fields):
"""
Return a Form class with the specified fields derived from a model. This is useful when we need a form to be used
Expand Down