From f30786d8fed1acd6a064c552b253b7eec439567c Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Fri, 27 Mar 2026 16:42:43 -0400 Subject: [PATCH 1/6] Fixes #21763: Replace M2M selection field with separate add/remove fields --- netbox/circuits/forms/model_forms.py | 23 ++++++++++--- netbox/circuits/tests/test_views.py | 2 +- netbox/dcim/forms/model_forms.py | 24 +++++++++++--- netbox/dcim/tests/test_views.py | 2 +- netbox/ipam/forms/model_forms.py | 32 ++++++++++--------- netbox/netbox/forms/model_forms.py | 12 +++++++ netbox/netbox/views/generic/object_views.py | 11 +++++++ netbox/utilities/forms/rendering.py | 16 ++++++++++ netbox/utilities/templatetags/form_helpers.py | 9 +++++- 9 files changed, 103 insertions(+), 28 deletions(-) diff --git a/netbox/circuits/forms/model_forms.py b/netbox/circuits/forms/model_forms.py index 5a48455d8..a84906ffb 100644 --- a/netbox/circuits/forms/model_forms.py +++ b/netbox/circuits/forms/model_forms.py @@ -22,7 +22,7 @@ from utilities.forms.fields import ( SlugField, ) from utilities.forms.mixins import DistanceValidationMixin -from utilities.forms.rendering import FieldSet, InlineFields +from utilities.forms.rendering import FieldSet, InlineFields, M2MAddRemoveFields from utilities.forms.widgets import DatePicker, HTMXSelect, NumberWithOptions from utilities.templatetags.builtins.filters import bettertitle @@ -43,22 +43,35 @@ __all__ = ( class ProviderForm(PrimaryModelForm): slug = SlugField() - asns = DynamicModelMultipleChoiceField( + add_asns = DynamicModelMultipleChoiceField( queryset=ASN.objects.all(), - label=_('ASNs'), + label=_('Add ASNs'), + required=False + ) + remove_asns = DynamicModelMultipleChoiceField( + queryset=ASN.objects.all(), + label=_('Remove ASNs'), required=False ) fieldsets = ( - FieldSet('name', 'slug', 'asns', 'description', 'tags'), + FieldSet('name', 'slug', M2MAddRemoveFields('asns'), 'description', 'tags'), ) class Meta: model = Provider fields = [ - 'name', 'slug', 'asns', 'description', 'owner', 'comments', 'tags', + 'name', 'slug', 'description', 'owner', 'comments', 'tags', ] + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + if self.instance.pk: + self.fields['remove_asns'].widget.add_query_param('provider_id', self.instance.pk) + else: + self.fields.pop('remove_asns') + self.fields['add_asns'].label = _('ASNs') + class ProviderAccountForm(PrimaryModelForm): provider = DynamicModelChoiceField( diff --git a/netbox/circuits/tests/test_views.py b/netbox/circuits/tests/test_views.py index 6ced9a958..3a3e2a50c 100644 --- a/netbox/circuits/tests/test_views.py +++ b/netbox/circuits/tests/test_views.py @@ -42,7 +42,7 @@ class ProviderTestCase(ViewTestCases.PrimaryObjectViewTestCase): cls.form_data = { 'name': 'Provider X', 'slug': 'provider-x', - 'asns': [asns[6].pk, asns[7].pk], + 'add_asns': [asns[6].pk, asns[7].pk], 'comments': 'Another provider', 'tags': [t.pk for t in tags], } diff --git a/netbox/dcim/forms/model_forms.py b/netbox/dcim/forms/model_forms.py index 741315323..1507bdfe3 100644 --- a/netbox/dcim/forms/model_forms.py +++ b/netbox/dcim/forms/model_forms.py @@ -23,7 +23,7 @@ from utilities.forms.fields import ( NumericArrayField, SlugField, ) -from utilities.forms.rendering import FieldSet, InlineFields, TabbedGroups +from utilities.forms.rendering import FieldSet, InlineFields, M2MAddRemoveFields, TabbedGroups from utilities.forms.widgets import ( APISelect, ClearableFileInput, @@ -137,9 +137,14 @@ class SiteForm(TenancyForm, PrimaryModelForm): required=False, quick_add=True ) - asns = DynamicModelMultipleChoiceField( + add_asns = DynamicModelMultipleChoiceField( queryset=ASN.objects.all(), - label=_('ASNs'), + label=_('Add ASNs'), + required=False + ) + remove_asns = DynamicModelMultipleChoiceField( + queryset=ASN.objects.all(), + label=_('Remove ASNs'), required=False ) slug = SlugField() @@ -151,7 +156,8 @@ class SiteForm(TenancyForm, PrimaryModelForm): fieldsets = ( FieldSet( - 'name', 'slug', 'status', 'region', 'group', 'facility', 'asns', 'time_zone', 'description', 'tags', + 'name', 'slug', 'status', 'region', 'group', 'facility', M2MAddRemoveFields('asns'), 'time_zone', + 'description', 'tags', name=_('Site') ), FieldSet('tenant_group', 'tenant', name=_('Tenancy')), @@ -161,7 +167,7 @@ class SiteForm(TenancyForm, PrimaryModelForm): class Meta: model = Site fields = ( - 'name', 'slug', 'status', 'region', 'group', 'tenant_group', 'tenant', 'facility', 'asns', 'time_zone', + 'name', 'slug', 'status', 'region', 'group', 'tenant_group', 'tenant', 'facility', 'time_zone', 'description', 'physical_address', 'shipping_address', 'latitude', 'longitude', 'owner', 'comments', 'tags', ) widgets = { @@ -177,6 +183,14 @@ class SiteForm(TenancyForm, PrimaryModelForm): ), } + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + if self.instance.pk: + self.fields['remove_asns'].widget.add_query_param('site_id', self.instance.pk) + else: + self.fields.pop('remove_asns') + self.fields['add_asns'].label = _('ASNs') + class LocationForm(TenancyForm, NestedGroupModelForm): site = DynamicModelChoiceField( diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index 1aac72875..3d7426013 100644 --- a/netbox/dcim/tests/test_views.py +++ b/netbox/dcim/tests/test_views.py @@ -160,7 +160,7 @@ class SiteTestCase(ViewTestCases.PrimaryObjectViewTestCase): 'group': groups[1].pk, 'tenant': None, 'facility': 'Facility X', - 'asns': [asns[6].pk, asns[7].pk], + 'add_asns': [asns[6].pk, asns[7].pk], 'time_zone': ZoneInfo('UTC'), 'description': 'Site description', 'physical_address': '742 Evergreen Terrace, Springfield, USA', diff --git a/netbox/ipam/forms/model_forms.py b/netbox/ipam/forms/model_forms.py index 1bdcff2d8..ff3cac503 100644 --- a/netbox/ipam/forms/model_forms.py +++ b/netbox/ipam/forms/model_forms.py @@ -21,7 +21,7 @@ from utilities.forms.fields import ( NumericArrayField, NumericRangeArrayField, ) -from utilities.forms.rendering import FieldSet, InlineFields, ObjectAttribute, TabbedGroups +from utilities.forms.rendering import FieldSet, InlineFields, M2MAddRemoveFields, ObjectAttribute, TabbedGroups from utilities.forms.utils import get_field_value from utilities.forms.widgets import DatePicker, HTMXSelect from utilities.templatetags.builtins.filters import bettertitle @@ -152,36 +152,38 @@ class ASNForm(TenancyForm, PrimaryModelForm): label=_('RIR'), quick_add=True ) - sites = DynamicModelMultipleChoiceField( + add_sites = DynamicModelMultipleChoiceField( queryset=Site.objects.all(), - label=_('Sites'), + label=_('Add sites'), + required=False + ) + remove_sites = DynamicModelMultipleChoiceField( + queryset=Site.objects.all(), + label=_('Remove sites'), required=False ) fieldsets = ( - FieldSet('asn', 'rir', 'sites', 'description', 'tags', name=_('ASN')), + FieldSet('asn', 'rir', M2MAddRemoveFields('sites'), 'description', 'tags', name=_('ASN')), FieldSet('tenant_group', 'tenant', name=_('Tenancy')), ) class Meta: model = ASN fields = [ - 'asn', 'rir', 'sites', 'tenant_group', 'tenant', 'description', 'owner', 'comments', 'tags' + 'asn', 'rir', 'tenant_group', 'tenant', 'description', 'owner', 'comments', 'tags' ] widgets = { 'date_added': DatePicker(), } - def __init__(self, data=None, instance=None, *args, **kwargs): - super().__init__(data=data, instance=instance, *args, **kwargs) - - if self.instance and self.instance.pk is not None: - self.fields['sites'].initial = self.instance.sites.all().values_list('id', flat=True) - - def save(self, *args, **kwargs): - instance = super().save(*args, **kwargs) - instance.sites.set(self.cleaned_data['sites']) - return instance + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + if self.instance.pk: + self.fields['remove_sites'].widget.add_query_param('asn_id', self.instance.pk) + else: + self.fields.pop('remove_sites') + self.fields['add_sites'].label = _('Sites') class RoleForm(OrganizationalModelForm): diff --git a/netbox/netbox/forms/model_forms.py b/netbox/netbox/forms/model_forms.py index a500365f9..97215d55a 100644 --- a/netbox/netbox/forms/model_forms.py +++ b/netbox/netbox/forms/model_forms.py @@ -75,7 +75,19 @@ class NetBoxModelForm( self.instance._m2m_values = {} for field in self.instance._meta.local_many_to_many: if field.name in self.cleaned_data: + # Standard M2M field (set-based) self.instance._m2m_values[field.name] = list(self.cleaned_data[field.name]) + elif f'add_{field.name}' in self.cleaned_data or f'remove_{field.name}' in self.cleaned_data: + # Add/remove M2M field pair: compute the effective set + current = set(getattr(self.instance, field.name).values_list('pk', flat=True)) \ + if self.instance.pk else set() + add_values = set( + v.pk for v in self.cleaned_data.get(f'add_{field.name}', []) + ) + remove_values = set( + v.pk for v in self.cleaned_data.get(f'remove_{field.name}', []) + ) + self.instance._m2m_values[field.name] = list((current | add_values) - remove_values) return super()._post_clean() diff --git a/netbox/netbox/views/generic/object_views.py b/netbox/netbox/views/generic/object_views.py index c6630d341..2aaa92830 100644 --- a/netbox/netbox/views/generic/object_views.py +++ b/netbox/netbox/views/generic/object_views.py @@ -299,6 +299,17 @@ class ObjectEditView(GetReturnURLMixin, BaseObjectView): object_created = form.instance.pk is None obj = form.save() + # Process any add/remove M2M field pairs + for field in obj._meta.local_many_to_many: + add_key = f'add_{field.name}' + remove_key = f'remove_{field.name}' + if add_key in form.cleaned_data or remove_key in form.cleaned_data: + m2m_manager = getattr(obj, field.name) + if add_values := form.cleaned_data.get(add_key): + m2m_manager.add(*add_values) + if remove_values := form.cleaned_data.get(remove_key): + m2m_manager.remove(*remove_values) + # Check that the new object conforms with any assigned object-level permissions if not self.queryset.filter(pk=obj.pk).exists(): raise PermissionsViolation() diff --git a/netbox/utilities/forms/rendering.py b/netbox/utilities/forms/rendering.py index 723e911e6..17235e107 100644 --- a/netbox/utilities/forms/rendering.py +++ b/netbox/utilities/forms/rendering.py @@ -5,6 +5,7 @@ from functools import cached_property __all__ = ( 'FieldSet', 'InlineFields', + 'M2MAddRemoveFields', 'ObjectAttribute', 'TabbedGroups', ) @@ -73,6 +74,21 @@ class TabbedGroups: ] +class M2MAddRemoveFields: + """ + Represents an add/remove field pair for a many-to-many relationship. Rather than rendering + a single multi-select pre-populated with all current values (which can crash the browser for + large datasets), this renders two fields: one for adding new relations and one for removing + existing relations. + + Parameters: + name: The name of the M2M field on the model (e.g. 'asns'). The form must define + corresponding 'add_{name}' and 'remove_{name}' fields. + """ + def __init__(self, name): + self.name = name + + class ObjectAttribute: """ Renders the value for a specific attribute on the form's instance. This may be used to diff --git a/netbox/utilities/templatetags/form_helpers.py b/netbox/utilities/templatetags/form_helpers.py index 0a3078ccf..974fcc633 100644 --- a/netbox/utilities/templatetags/form_helpers.py +++ b/netbox/utilities/templatetags/form_helpers.py @@ -1,6 +1,6 @@ from django import template -from utilities.forms.rendering import InlineFields, ObjectAttribute, TabbedGroups +from utilities.forms.rendering import InlineFields, M2MAddRemoveFields, ObjectAttribute, TabbedGroups __all__ = ( 'getfield', @@ -80,6 +80,13 @@ def render_fieldset(form, fieldset): ('tabs', None, tabs) ) + elif type(item) is M2MAddRemoveFields: + for field_name in (f'add_{item.name}', f'remove_{item.name}'): + if field_name in form.fields: + rows.append( + ('field', None, [form[field_name]]) + ) + elif type(item) is ObjectAttribute: value = getattr(form.instance, item.name) label = value._meta.verbose_name if hasattr(value, '_meta') else item.name From d5f37d7a87eb3d276daceff6daeecb231d1435b7 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Mon, 30 Mar 2026 09:07:15 -0400 Subject: [PATCH 2/6] Use add/remove fields only when assignment count is 100+ --- netbox/circuits/forms/model_forms.py | 14 +++++- netbox/circuits/tests/test_views.py | 2 +- netbox/dcim/forms/model_forms.py | 14 +++++- netbox/dcim/tests/test_views.py | 2 +- netbox/ipam/forms/model_forms.py | 14 +++++- netbox/netbox/forms/model_forms.py | 43 ++++++++++++++----- netbox/netbox/views/generic/object_views.py | 11 ----- netbox/utilities/forms/rendering.py | 18 +++++--- netbox/utilities/templatetags/form_helpers.py | 17 +++++--- 9 files changed, 95 insertions(+), 40 deletions(-) diff --git a/netbox/circuits/forms/model_forms.py b/netbox/circuits/forms/model_forms.py index a84906ffb..dfc41f590 100644 --- a/netbox/circuits/forms/model_forms.py +++ b/netbox/circuits/forms/model_forms.py @@ -43,6 +43,11 @@ __all__ = ( class ProviderForm(PrimaryModelForm): slug = SlugField() + asns = DynamicModelMultipleChoiceField( + queryset=ASN.objects.all(), + label=_('ASNs'), + required=False + ) add_asns = DynamicModelMultipleChoiceField( queryset=ASN.objects.all(), label=_('Add ASNs'), @@ -66,11 +71,16 @@ class ProviderForm(PrimaryModelForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - if self.instance.pk: + if self.instance.pk and self.instance.asns.count() >= M2MAddRemoveFields.THRESHOLD: + # Add/remove mode for large M2M sets + self.fields.pop('asns') self.fields['remove_asns'].widget.add_query_param('provider_id', self.instance.pk) else: + # Simple mode for new objects or small M2M sets + self.fields.pop('add_asns') self.fields.pop('remove_asns') - self.fields['add_asns'].label = _('ASNs') + if self.instance.pk: + self.initial['asns'] = list(self.instance.asns.values_list('pk', flat=True)) class ProviderAccountForm(PrimaryModelForm): diff --git a/netbox/circuits/tests/test_views.py b/netbox/circuits/tests/test_views.py index 3a3e2a50c..6ced9a958 100644 --- a/netbox/circuits/tests/test_views.py +++ b/netbox/circuits/tests/test_views.py @@ -42,7 +42,7 @@ class ProviderTestCase(ViewTestCases.PrimaryObjectViewTestCase): cls.form_data = { 'name': 'Provider X', 'slug': 'provider-x', - 'add_asns': [asns[6].pk, asns[7].pk], + 'asns': [asns[6].pk, asns[7].pk], 'comments': 'Another provider', 'tags': [t.pk for t in tags], } diff --git a/netbox/dcim/forms/model_forms.py b/netbox/dcim/forms/model_forms.py index 1507bdfe3..ee428d241 100644 --- a/netbox/dcim/forms/model_forms.py +++ b/netbox/dcim/forms/model_forms.py @@ -137,6 +137,11 @@ class SiteForm(TenancyForm, PrimaryModelForm): required=False, quick_add=True ) + asns = DynamicModelMultipleChoiceField( + queryset=ASN.objects.all(), + label=_('ASNs'), + required=False + ) add_asns = DynamicModelMultipleChoiceField( queryset=ASN.objects.all(), label=_('Add ASNs'), @@ -185,11 +190,16 @@ class SiteForm(TenancyForm, PrimaryModelForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - if self.instance.pk: + if self.instance.pk and self.instance.asns.count() >= M2MAddRemoveFields.THRESHOLD: + # Add/remove mode for large M2M sets + self.fields.pop('asns') self.fields['remove_asns'].widget.add_query_param('site_id', self.instance.pk) else: + # Simple mode for new objects or small M2M sets + self.fields.pop('add_asns') self.fields.pop('remove_asns') - self.fields['add_asns'].label = _('ASNs') + if self.instance.pk: + self.initial['asns'] = list(self.instance.asns.values_list('pk', flat=True)) class LocationForm(TenancyForm, NestedGroupModelForm): diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index 3d7426013..1aac72875 100644 --- a/netbox/dcim/tests/test_views.py +++ b/netbox/dcim/tests/test_views.py @@ -160,7 +160,7 @@ class SiteTestCase(ViewTestCases.PrimaryObjectViewTestCase): 'group': groups[1].pk, 'tenant': None, 'facility': 'Facility X', - 'add_asns': [asns[6].pk, asns[7].pk], + 'asns': [asns[6].pk, asns[7].pk], 'time_zone': ZoneInfo('UTC'), 'description': 'Site description', 'physical_address': '742 Evergreen Terrace, Springfield, USA', diff --git a/netbox/ipam/forms/model_forms.py b/netbox/ipam/forms/model_forms.py index ff3cac503..2900a1ff3 100644 --- a/netbox/ipam/forms/model_forms.py +++ b/netbox/ipam/forms/model_forms.py @@ -152,6 +152,11 @@ class ASNForm(TenancyForm, PrimaryModelForm): label=_('RIR'), quick_add=True ) + sites = DynamicModelMultipleChoiceField( + queryset=Site.objects.all(), + label=_('Sites'), + required=False + ) add_sites = DynamicModelMultipleChoiceField( queryset=Site.objects.all(), label=_('Add sites'), @@ -179,11 +184,16 @@ class ASNForm(TenancyForm, PrimaryModelForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - if self.instance.pk: + if self.instance.pk and self.instance.sites.count() >= M2MAddRemoveFields.THRESHOLD: + # Add/remove mode for large M2M sets + self.fields.pop('sites') self.fields['remove_sites'].widget.add_query_param('asn_id', self.instance.pk) else: + # Simple mode for new objects or small M2M sets + self.fields.pop('add_sites') self.fields.pop('remove_sites') - self.fields['add_sites'].label = _('Sites') + if self.instance.pk: + self.initial['sites'] = list(self.instance.sites.values_list('pk', flat=True)) class RoleForm(OrganizationalModelForm): diff --git a/netbox/netbox/forms/model_forms.py b/netbox/netbox/forms/model_forms.py index 97215d55a..45948e174 100644 --- a/netbox/netbox/forms/model_forms.py +++ b/netbox/netbox/forms/model_forms.py @@ -2,6 +2,8 @@ import json from django import forms from django.contrib.contenttypes.models import ContentType +from django.db import models +from django.db.models.fields.related import ManyToManyRel from extras.choices import * from utilities.forms.fields import CommentField, SlugField @@ -71,26 +73,47 @@ class NetBoxModelForm( def _post_clean(self): """ Override BaseModelForm's _post_clean() to store many-to-many field values on the model instance. + Handles both forward and reverse M2M relationships, and supports both simple (single field) + and add/remove (dual field) modes. """ self.instance._m2m_values = {} - for field in self.instance._meta.local_many_to_many: - if field.name in self.cleaned_data: - # Standard M2M field (set-based) - self.instance._m2m_values[field.name] = list(self.cleaned_data[field.name]) - elif f'add_{field.name}' in self.cleaned_data or f'remove_{field.name}' in self.cleaned_data: - # Add/remove M2M field pair: compute the effective set - current = set(getattr(self.instance, field.name).values_list('pk', flat=True)) \ + for field in self.instance._meta.get_fields(): + # Determine the accessor name for this M2M relationship + if isinstance(field, models.ManyToManyField): + name = field.name + elif isinstance(field, ManyToManyRel): + name = field.get_accessor_name() + else: + continue + + if name in self.cleaned_data: + # Simple mode: single multi-select field + self.instance._m2m_values[name] = list(self.cleaned_data[name]) + elif f'add_{name}' in self.cleaned_data or f'remove_{name}' in self.cleaned_data: + # Add/remove mode: compute the effective set + current = set(getattr(self.instance, name).values_list('pk', flat=True)) \ if self.instance.pk else set() add_values = set( - v.pk for v in self.cleaned_data.get(f'add_{field.name}', []) + v.pk for v in self.cleaned_data.get(f'add_{name}', []) ) remove_values = set( - v.pk for v in self.cleaned_data.get(f'remove_{field.name}', []) + v.pk for v in self.cleaned_data.get(f'remove_{name}', []) ) - self.instance._m2m_values[field.name] = list((current | add_values) - remove_values) + self.instance._m2m_values[name] = list((current | add_values) - remove_values) return super()._post_clean() + def _save_m2m(self): + """ + Save many-to-many field values that were computed in _post_clean(). This handles M2M fields + not included in Meta.fields (e.g. those managed via M2MAddRemoveFields). + """ + super()._save_m2m() + meta_fields = self._meta.fields + for field_name, values in self.instance._m2m_values.items(): + if not meta_fields or field_name not in meta_fields: + getattr(self.instance, field_name).set(values) + class PrimaryModelForm(OwnerMixin, NetBoxModelForm): """ diff --git a/netbox/netbox/views/generic/object_views.py b/netbox/netbox/views/generic/object_views.py index 2aaa92830..c6630d341 100644 --- a/netbox/netbox/views/generic/object_views.py +++ b/netbox/netbox/views/generic/object_views.py @@ -299,17 +299,6 @@ class ObjectEditView(GetReturnURLMixin, BaseObjectView): object_created = form.instance.pk is None obj = form.save() - # Process any add/remove M2M field pairs - for field in obj._meta.local_many_to_many: - add_key = f'add_{field.name}' - remove_key = f'remove_{field.name}' - if add_key in form.cleaned_data or remove_key in form.cleaned_data: - m2m_manager = getattr(obj, field.name) - if add_values := form.cleaned_data.get(add_key): - m2m_manager.add(*add_values) - if remove_values := form.cleaned_data.get(remove_key): - m2m_manager.remove(*remove_values) - # Check that the new object conforms with any assigned object-level permissions if not self.queryset.filter(pk=obj.pk).exists(): raise PermissionsViolation() diff --git a/netbox/utilities/forms/rendering.py b/netbox/utilities/forms/rendering.py index 17235e107..c7bb324d8 100644 --- a/netbox/utilities/forms/rendering.py +++ b/netbox/utilities/forms/rendering.py @@ -76,15 +76,21 @@ class TabbedGroups: class M2MAddRemoveFields: """ - Represents an add/remove field pair for a many-to-many relationship. Rather than rendering - a single multi-select pre-populated with all current values (which can crash the browser for - large datasets), this renders two fields: one for adding new relations and one for removing - existing relations. + Represents a many-to-many relationship field on a form. It supports two rendering modes: + + 1. Simple mode: A single multi-select field pre-populated with current values. This is used + for new objects or existing objects with fewer than THRESHOLD current assignments. + 2. Add/remove mode: Two separate fields for adding and removing relations. This avoids + crashing the browser when an object has a very large number of current assignments. + + The form must define three fields: '{name}', 'add_{name}', and 'remove_{name}'. The form's + __init__() method determines the mode and removes the unused fields. Parameters: - name: The name of the M2M field on the model (e.g. 'asns'). The form must define - corresponding 'add_{name}' and 'remove_{name}' fields. + name: The name of the M2M field on the model (e.g. 'asns'). """ + THRESHOLD = 100 + def __init__(self, name): self.name = name diff --git a/netbox/utilities/templatetags/form_helpers.py b/netbox/utilities/templatetags/form_helpers.py index 974fcc633..156bd72c2 100644 --- a/netbox/utilities/templatetags/form_helpers.py +++ b/netbox/utilities/templatetags/form_helpers.py @@ -81,11 +81,18 @@ def render_fieldset(form, fieldset): ) elif type(item) is M2MAddRemoveFields: - for field_name in (f'add_{item.name}', f'remove_{item.name}'): - if field_name in form.fields: - rows.append( - ('field', None, [form[field_name]]) - ) + if item.name in form.fields: + # Simple mode: render a single multi-select field + rows.append( + ('field', None, [form[item.name]]) + ) + else: + # Add/remove mode: render separate add and remove fields + for field_name in (f'add_{item.name}', f'remove_{item.name}'): + if field_name in form.fields: + rows.append( + ('field', None, [form[field_name]]) + ) elif type(item) is ObjectAttribute: value = getattr(form.instance, item.name) From 757c4f69d23a660c3a0177d4bb7f4208e9af0966 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Mon, 30 Mar 2026 09:15:35 -0400 Subject: [PATCH 3/6] Annotate current number of assignments if >100 --- netbox/circuits/forms/model_forms.py | 3 ++- netbox/dcim/forms/model_forms.py | 3 ++- netbox/ipam/forms/model_forms.py | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/netbox/circuits/forms/model_forms.py b/netbox/circuits/forms/model_forms.py index dfc41f590..0535bac5c 100644 --- a/netbox/circuits/forms/model_forms.py +++ b/netbox/circuits/forms/model_forms.py @@ -71,10 +71,11 @@ class ProviderForm(PrimaryModelForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - if self.instance.pk and self.instance.asns.count() >= M2MAddRemoveFields.THRESHOLD: + if self.instance.pk and (count := self.instance.asns.count()) >= M2MAddRemoveFields.THRESHOLD: # Add/remove mode for large M2M sets self.fields.pop('asns') self.fields['remove_asns'].widget.add_query_param('provider_id', self.instance.pk) + self.fields['remove_asns'].help_text = _("{count} ASNs currently assigned").format(count=count) else: # Simple mode for new objects or small M2M sets self.fields.pop('add_asns') diff --git a/netbox/dcim/forms/model_forms.py b/netbox/dcim/forms/model_forms.py index ee428d241..cf7a3a80e 100644 --- a/netbox/dcim/forms/model_forms.py +++ b/netbox/dcim/forms/model_forms.py @@ -190,10 +190,11 @@ class SiteForm(TenancyForm, PrimaryModelForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - if self.instance.pk and self.instance.asns.count() >= M2MAddRemoveFields.THRESHOLD: + if self.instance.pk and (count := self.instance.asns.count()) >= M2MAddRemoveFields.THRESHOLD: # Add/remove mode for large M2M sets self.fields.pop('asns') self.fields['remove_asns'].widget.add_query_param('site_id', self.instance.pk) + self.fields['remove_asns'].help_text = _("{count} ASNs currently assigned").format(count=count) else: # Simple mode for new objects or small M2M sets self.fields.pop('add_asns') diff --git a/netbox/ipam/forms/model_forms.py b/netbox/ipam/forms/model_forms.py index 2900a1ff3..a2968e0c9 100644 --- a/netbox/ipam/forms/model_forms.py +++ b/netbox/ipam/forms/model_forms.py @@ -184,10 +184,11 @@ class ASNForm(TenancyForm, PrimaryModelForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - if self.instance.pk and self.instance.sites.count() >= M2MAddRemoveFields.THRESHOLD: + if self.instance.pk and (count := self.instance.sites.count()) >= M2MAddRemoveFields.THRESHOLD: # Add/remove mode for large M2M sets self.fields.pop('sites') self.fields['remove_sites'].widget.add_query_param('asn_id', self.instance.pk) + self.fields['remove_sites'].help_text = _("{count} sites currently assigned").format(count=count) else: # Simple mode for new objects or small M2M sets self.fields.pop('add_sites') From 0154a09856ef830f18f9a9ee5c774543b979f93d Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Mon, 30 Mar 2026 09:22:56 -0400 Subject: [PATCH 4/6] Limit 'add' field choices to objects not already assigned --- netbox/circuits/forms/model_forms.py | 1 + netbox/dcim/forms/model_forms.py | 1 + netbox/ipam/forms/model_forms.py | 1 + 3 files changed, 3 insertions(+) diff --git a/netbox/circuits/forms/model_forms.py b/netbox/circuits/forms/model_forms.py index 0535bac5c..446ec0eed 100644 --- a/netbox/circuits/forms/model_forms.py +++ b/netbox/circuits/forms/model_forms.py @@ -74,6 +74,7 @@ class ProviderForm(PrimaryModelForm): if self.instance.pk and (count := self.instance.asns.count()) >= M2MAddRemoveFields.THRESHOLD: # Add/remove mode for large M2M sets self.fields.pop('asns') + self.fields['add_asns'].widget.add_query_param('provider_id__n', self.instance.pk) self.fields['remove_asns'].widget.add_query_param('provider_id', self.instance.pk) self.fields['remove_asns'].help_text = _("{count} ASNs currently assigned").format(count=count) else: diff --git a/netbox/dcim/forms/model_forms.py b/netbox/dcim/forms/model_forms.py index cf7a3a80e..cb5809d4c 100644 --- a/netbox/dcim/forms/model_forms.py +++ b/netbox/dcim/forms/model_forms.py @@ -193,6 +193,7 @@ class SiteForm(TenancyForm, PrimaryModelForm): if self.instance.pk and (count := self.instance.asns.count()) >= M2MAddRemoveFields.THRESHOLD: # Add/remove mode for large M2M sets self.fields.pop('asns') + self.fields['add_asns'].widget.add_query_param('site_id__n', self.instance.pk) self.fields['remove_asns'].widget.add_query_param('site_id', self.instance.pk) self.fields['remove_asns'].help_text = _("{count} ASNs currently assigned").format(count=count) else: diff --git a/netbox/ipam/forms/model_forms.py b/netbox/ipam/forms/model_forms.py index a2968e0c9..f9f2acf71 100644 --- a/netbox/ipam/forms/model_forms.py +++ b/netbox/ipam/forms/model_forms.py @@ -187,6 +187,7 @@ class ASNForm(TenancyForm, PrimaryModelForm): if self.instance.pk and (count := self.instance.sites.count()) >= M2MAddRemoveFields.THRESHOLD: # Add/remove mode for large M2M sets self.fields.pop('sites') + self.fields['add_sites'].widget.add_query_param('asn_id__n', self.instance.pk) self.fields['remove_sites'].widget.add_query_param('asn_id', self.instance.pk) self.fields['remove_sites'].help_text = _("{count} sites currently assigned").format(count=count) else: From a45e8571da8663036dd8174b241600a2994cc5cd Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Mon, 30 Mar 2026 09:29:08 -0400 Subject: [PATCH 5/6] Revert changes to ASNForm --- netbox/ipam/forms/model_forms.py | 40 +++++++++++--------------------- 1 file changed, 13 insertions(+), 27 deletions(-) diff --git a/netbox/ipam/forms/model_forms.py b/netbox/ipam/forms/model_forms.py index f9f2acf71..1bdcff2d8 100644 --- a/netbox/ipam/forms/model_forms.py +++ b/netbox/ipam/forms/model_forms.py @@ -21,7 +21,7 @@ from utilities.forms.fields import ( NumericArrayField, NumericRangeArrayField, ) -from utilities.forms.rendering import FieldSet, InlineFields, M2MAddRemoveFields, ObjectAttribute, TabbedGroups +from utilities.forms.rendering import FieldSet, InlineFields, ObjectAttribute, TabbedGroups from utilities.forms.utils import get_field_value from utilities.forms.widgets import DatePicker, HTMXSelect from utilities.templatetags.builtins.filters import bettertitle @@ -157,45 +157,31 @@ class ASNForm(TenancyForm, PrimaryModelForm): label=_('Sites'), required=False ) - add_sites = DynamicModelMultipleChoiceField( - queryset=Site.objects.all(), - label=_('Add sites'), - required=False - ) - remove_sites = DynamicModelMultipleChoiceField( - queryset=Site.objects.all(), - label=_('Remove sites'), - required=False - ) fieldsets = ( - FieldSet('asn', 'rir', M2MAddRemoveFields('sites'), 'description', 'tags', name=_('ASN')), + FieldSet('asn', 'rir', 'sites', 'description', 'tags', name=_('ASN')), FieldSet('tenant_group', 'tenant', name=_('Tenancy')), ) class Meta: model = ASN fields = [ - 'asn', 'rir', 'tenant_group', 'tenant', 'description', 'owner', 'comments', 'tags' + 'asn', 'rir', 'sites', 'tenant_group', 'tenant', 'description', 'owner', 'comments', 'tags' ] widgets = { 'date_added': DatePicker(), } - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - if self.instance.pk and (count := self.instance.sites.count()) >= M2MAddRemoveFields.THRESHOLD: - # Add/remove mode for large M2M sets - self.fields.pop('sites') - self.fields['add_sites'].widget.add_query_param('asn_id__n', self.instance.pk) - self.fields['remove_sites'].widget.add_query_param('asn_id', self.instance.pk) - self.fields['remove_sites'].help_text = _("{count} sites currently assigned").format(count=count) - else: - # Simple mode for new objects or small M2M sets - self.fields.pop('add_sites') - self.fields.pop('remove_sites') - if self.instance.pk: - self.initial['sites'] = list(self.instance.sites.values_list('pk', flat=True)) + def __init__(self, data=None, instance=None, *args, **kwargs): + super().__init__(data=data, instance=instance, *args, **kwargs) + + if self.instance and self.instance.pk is not None: + self.fields['sites'].initial = self.instance.sites.all().values_list('id', flat=True) + + def save(self, *args, **kwargs): + instance = super().save(*args, **kwargs) + instance.sites.set(self.cleaned_data['sites']) + return instance class RoleForm(OrganizationalModelForm): From 55daf4c52fba71f89fdeae470af4c6f5a3d0a2d9 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Mon, 30 Mar 2026 10:02:38 -0400 Subject: [PATCH 6/6] Add/fix tests --- netbox/dcim/tests/test_forms.py | 111 ++++++++++++++++++++++++++++- netbox/netbox/forms/model_forms.py | 19 ++--- 2 files changed, 120 insertions(+), 10 deletions(-) diff --git a/netbox/dcim/tests/test_forms.py b/netbox/dcim/tests/test_forms.py index 118c347fd..2f01525fc 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/netbox/dcim/tests/test_forms.py @@ -10,7 +10,8 @@ from dcim.choices import ( ) from dcim.forms import * from dcim.models import * -from ipam.models import VLAN +from ipam.models import ASN, RIR, VLAN +from utilities.forms.rendering import M2MAddRemoveFields from utilities.testing import create_test_device from virtualization.models import Cluster, ClusterGroup, ClusterType @@ -417,3 +418,111 @@ class InterfaceTestCase(TestCase): self.assertNotIn('untagged_vlan', form.cleaned_data.keys()) self.assertNotIn('tagged_vlans', form.cleaned_data.keys()) self.assertNotIn('qinq_svlan', form.cleaned_data.keys()) + + +class SiteFormTestCase(TestCase): + """ + Tests for M2MAddRemoveFields using Site ASN assignments as the test case. + Covers both simple mode (single multi-select field) and add/remove mode (dual fields). + """ + + @classmethod + def setUpTestData(cls): + cls.rir = RIR.objects.create(name='RIR 1', slug='rir-1') + # Create 110 ASNs: 100 to pre-assign (triggering add/remove mode) plus 10 extras + ASN.objects.bulk_create([ASN(asn=i, rir=cls.rir) for i in range(1, 111)]) + cls.asns = list(ASN.objects.order_by('asn')) + + def _site_data(self, **kwargs): + data = {'name': 'Test Site', 'slug': 'test-site', 'status': 'active'} + data.update(kwargs) + return data + + def test_new_site_uses_simple_mode(self): + """A form for a new site uses the single 'asns' field (simple mode).""" + form = SiteForm(data=self._site_data()) + self.assertIn('asns', form.fields) + self.assertNotIn('add_asns', form.fields) + self.assertNotIn('remove_asns', form.fields) + + def test_existing_site_below_threshold_uses_simple_mode(self): + """A form for an existing site with fewer than THRESHOLD ASNs uses simple mode.""" + site = Site.objects.create(name='Site 1', slug='site-1') + site.asns.set(self.asns[:5]) + form = SiteForm(instance=site) + self.assertIn('asns', form.fields) + self.assertNotIn('add_asns', form.fields) + self.assertNotIn('remove_asns', form.fields) + + def test_existing_site_at_threshold_uses_add_remove_mode(self): + """A form for an existing site with THRESHOLD or more ASNs uses add/remove mode.""" + site = Site.objects.create(name='Site 2', slug='site-2') + site.asns.set(self.asns[:M2MAddRemoveFields.THRESHOLD]) + form = SiteForm(instance=site) + self.assertNotIn('asns', form.fields) + self.assertIn('add_asns', form.fields) + self.assertIn('remove_asns', form.fields) + + def test_simple_mode_assigns_asns_on_create(self): + """Saving a new site via simple mode assigns the selected ASNs.""" + asn_pks = [asn.pk for asn in self.asns[:3]] + form = SiteForm(data=self._site_data(asns=asn_pks)) + self.assertTrue(form.is_valid(), form.errors) + site = form.save() + self.assertEqual(set(site.asns.values_list('pk', flat=True)), set(asn_pks)) + + def test_simple_mode_replaces_asns_on_edit(self): + """Saving an existing site via simple mode replaces the current ASN assignments.""" + site = Site.objects.create(name='Site 3', slug='site-3') + site.asns.set(self.asns[:3]) + new_asn_pks = [asn.pk for asn in self.asns[3:6]] + form = SiteForm( + data=self._site_data(name='Site 3', slug='site-3', asns=new_asn_pks), + instance=site + ) + self.assertTrue(form.is_valid(), form.errors) + site = form.save() + self.assertEqual(set(site.asns.values_list('pk', flat=True)), set(new_asn_pks)) + + def test_add_remove_mode_adds_asns(self): + """In add/remove mode, specifying 'add_asns' appends to current assignments.""" + site = Site.objects.create(name='Site 4', slug='site-4') + site.asns.set(self.asns[:M2MAddRemoveFields.THRESHOLD]) + new_asn_pks = [asn.pk for asn in self.asns[M2MAddRemoveFields.THRESHOLD:]] + form = SiteForm( + data=self._site_data(name='Site 4', slug='site-4', add_asns=new_asn_pks), + instance=site + ) + self.assertTrue(form.is_valid(), form.errors) + site = form.save() + self.assertEqual(site.asns.count(), len(self.asns)) + + def test_add_remove_mode_removes_asns(self): + """In add/remove mode, specifying 'remove_asns' drops those assignments.""" + site = Site.objects.create(name='Site 5', slug='site-5') + site.asns.set(self.asns[:M2MAddRemoveFields.THRESHOLD]) + remove_pks = [asn.pk for asn in self.asns[:5]] + form = SiteForm( + data=self._site_data(name='Site 5', slug='site-5', remove_asns=remove_pks), + instance=site + ) + self.assertTrue(form.is_valid(), form.errors) + site = form.save() + self.assertEqual(site.asns.count(), M2MAddRemoveFields.THRESHOLD - 5) + self.assertFalse(site.asns.filter(pk__in=remove_pks).exists()) + + def test_add_remove_mode_simultaneous_add_and_remove(self): + """In add/remove mode, add and remove operations are applied together.""" + site = Site.objects.create(name='Site 6', slug='site-6') + site.asns.set(self.asns[:M2MAddRemoveFields.THRESHOLD]) + add_pks = [asn.pk for asn in self.asns[M2MAddRemoveFields.THRESHOLD:M2MAddRemoveFields.THRESHOLD + 3]] + remove_pks = [asn.pk for asn in self.asns[:3]] + form = SiteForm( + data=self._site_data(name='Site 6', slug='site-6', add_asns=add_pks, remove_asns=remove_pks), + instance=site + ) + self.assertTrue(form.is_valid(), form.errors) + site = form.save() + self.assertEqual(site.asns.count(), M2MAddRemoveFields.THRESHOLD) + self.assertTrue(site.asns.filter(pk__in=add_pks).count() == 3) + self.assertFalse(site.asns.filter(pk__in=remove_pks).exists()) diff --git a/netbox/netbox/forms/model_forms.py b/netbox/netbox/forms/model_forms.py index 45948e174..cfca17255 100644 --- a/netbox/netbox/forms/model_forms.py +++ b/netbox/netbox/forms/model_forms.py @@ -2,7 +2,6 @@ import json from django import forms from django.contrib.contenttypes.models import ContentType -from django.db import models from django.db.models.fields.related import ManyToManyRel from extras.choices import * @@ -77,15 +76,17 @@ class NetBoxModelForm( and add/remove (dual field) modes. """ self.instance._m2m_values = {} - for field in self.instance._meta.get_fields(): - # Determine the accessor name for this M2M relationship - if isinstance(field, models.ManyToManyField): - name = field.name - elif isinstance(field, ManyToManyRel): - name = field.get_accessor_name() - else: - continue + # Collect names to process: local M2M fields (includes TaggableManager from django-taggit) + # plus reverse M2M relations (ManyToManyRel). + names = [field.name for field in self.instance._meta.local_many_to_many] + names += [ + field.get_accessor_name() + for field in self.instance._meta.get_fields() + if isinstance(field, ManyToManyRel) + ] + + for name in names: if name in self.cleaned_data: # Simple mode: single multi-select field self.instance._m2m_values[name] = list(self.cleaned_data[name])