From 60f80c8ad2baddc0cefc58f7f56def1b825a98b1 Mon Sep 17 00:00:00 2001 From: Martin Hauser Date: Thu, 27 Aug 2026 17:38:50 +0200 Subject: [PATCH] fix(forms): Assign scope before validation in ScopedForm mixin Move scope assignment before validation to prevent stale scope values on instances when validation fails. Refactor VLANGroupForm to inherit from ScopedForm, removing duplicate scope handling code. Add test coverage for scope type changes and validation errors. Fixes #23040 --- netbox/dcim/forms/mixins.py | 7 ++- netbox/ipam/forms/bulk_edit.py | 28 +--------- netbox/ipam/forms/model_forms.py | 40 +------------ netbox/ipam/models/vlans.py | 13 +++-- netbox/ipam/tests/test_forms.py | 96 +++++++++++++++++++++++++++++++- 5 files changed, 112 insertions(+), 72 deletions(-) diff --git a/netbox/dcim/forms/mixins.py b/netbox/dcim/forms/mixins.py index 2b204c08c..515295f9d 100644 --- a/netbox/dcim/forms/mixins.py +++ b/netbox/dcim/forms/mixins.py @@ -54,6 +54,10 @@ class ScopedForm(forms.Form): scope = self.cleaned_data.get('scope') scope_type = self.cleaned_data.get('scope_type') + + # Assign the scope before validating, so a rejected pair leaves no stale scope on the instance + self.instance.scope = scope + if scope_type and not scope: raise ValidationError({ 'scope': _( @@ -61,9 +65,6 @@ class ScopedForm(forms.Form): ).format(scope_type=scope_type.model_class()._meta.model_name) }) - # Assign the selected scope (if any) - self.instance.scope = scope - def _set_scoped_values(self): if scope_type_id := get_field_value(self, 'scope_type'): try: diff --git a/netbox/ipam/forms/bulk_edit.py b/netbox/ipam/forms/bulk_edit.py index fd0afffa1..d5ad48693 100644 --- a/netbox/ipam/forms/bulk_edit.py +++ b/netbox/ipam/forms/bulk_edit.py @@ -1,6 +1,5 @@ from django import forms from django.contrib.contenttypes.models import ContentType -from django.core.exceptions import ObjectDoesNotExist from django.utils.translation import gettext_lazy as _ from dcim.forms.mixins import ScopedBulkEditForm @@ -11,7 +10,7 @@ from ipam.models import * from ipam.models import ASN from netbox.forms import NetBoxModelBulkEditForm, OrganizationalModelBulkEditForm, PrimaryModelBulkEditForm from tenancy.models import Tenant -from utilities.forms import add_blank_choice, get_field_value +from utilities.forms import add_blank_choice from utilities.forms.fields import ( ContentTypeChoiceField, DynamicModelChoiceField, @@ -21,7 +20,6 @@ from utilities.forms.fields import ( ) from utilities.forms.rendering import FieldSet from utilities.forms.widgets import BulkEditNullBooleanSelect, HTMXSelect -from utilities.templatetags.builtins.filters import bettertitle __all__ = ( 'ASNBulkEditForm', @@ -357,20 +355,14 @@ class FHRPGroupBulkEditForm(PrimaryModelBulkEditForm): nullable_fields = ('auth_type', 'auth_key', 'name', 'description', 'comments') -class VLANGroupBulkEditForm(OrganizationalModelBulkEditForm): +class VLANGroupBulkEditForm(ScopedBulkEditForm, OrganizationalModelBulkEditForm): + # Override ScopedBulkEditForm.scope_type to set custom queryset scope_type = ContentTypeChoiceField( queryset=ContentType.objects.filter(model__in=VLANGROUP_SCOPE_TYPES), widget=HTMXSelect(method='post', attrs={'hx-select': '#form_fields'}), required=False, label=_('Scope type') ) - scope = DynamicModelChoiceField( - label=_('Scope'), - queryset=Site.objects.none(), # Initial queryset - required=False, - disabled=True, - selector=True - ) vid_ranges = NumericRangeArrayField( label=_('VLAN ID ranges'), required=False @@ -389,20 +381,6 @@ class VLANGroupBulkEditForm(OrganizationalModelBulkEditForm): ) nullable_fields = ('description', 'scope', 'comments') - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - - if scope_type_id := get_field_value(self, 'scope_type'): - try: - scope_type = ContentType.objects.get(pk=scope_type_id) - model = scope_type.model_class() - self.fields['scope'].queryset = model.objects.all() - self.fields['scope'].widget.attrs['selector'] = model._meta.label_lower - self.fields['scope'].disabled = False - self.fields['scope'].label = _(bettertitle(model._meta.verbose_name)) - except ObjectDoesNotExist: - pass - class VLANBulkEditForm(PrimaryModelBulkEditForm): region = DynamicModelChoiceField( diff --git a/netbox/ipam/forms/model_forms.py b/netbox/ipam/forms/model_forms.py index 1b223ccdc..5fa7ac5b7 100644 --- a/netbox/ipam/forms/model_forms.py +++ b/netbox/ipam/forms/model_forms.py @@ -628,23 +628,17 @@ class FHRPGroupAssignmentForm(forms.ModelForm): return group -class VLANGroupForm(TenancyForm, OrganizationalModelForm): +class VLANGroupForm(TenancyForm, ScopedForm, OrganizationalModelForm): vid_ranges = NumericRangeArrayField( label=_('VLAN IDs') ) + # Override ScopedForm.scope_type to set custom queryset scope_type = ContentTypeChoiceField( queryset=ContentType.objects.filter(model__in=VLANGROUP_SCOPE_TYPES), widget=HTMXSelect(), required=False, label=_('Scope type') ) - scope = DynamicModelChoiceField( - label=_('Scope'), - queryset=Site.objects.none(), # Initial queryset - required=False, - disabled=True, - selector=True - ) fieldsets = ( FieldSet('name', 'slug', 'description', 'tags', name=_('VLAN Group')), @@ -660,36 +654,6 @@ class VLANGroupForm(TenancyForm, OrganizationalModelForm): 'tags', ] - def __init__(self, *args, **kwargs): - instance = kwargs.get('instance') - initial = kwargs.get('initial', {}) - - if instance is not None and instance.scope: - initial['scope'] = instance.scope - kwargs['initial'] = initial - - super().__init__(*args, **kwargs) - - if scope_type_id := get_field_value(self, 'scope_type'): - try: - scope_type = ContentType.objects.get(pk=scope_type_id) - model = scope_type.model_class() - self.fields['scope'].queryset = model.objects.all() - self.fields['scope'].widget.attrs['selector'] = model._meta.label_lower - self.fields['scope'].disabled = False - self.fields['scope'].label = _(bettertitle(model._meta.verbose_name)) - except ObjectDoesNotExist: - pass - - if self.instance and scope_type_id != self.instance.scope_type_id: - self.initial['scope'] = None - - def clean(self): - super().clean() - - # Assign the selected scope (if any) - self.instance.scope = self.cleaned_data.get('scope') - class VLANForm(TenancyForm, PrimaryModelForm): group = DynamicModelChoiceField( diff --git a/netbox/ipam/models/vlans.py b/netbox/ipam/models/vlans.py index 40f56f87f..c294ef39e 100644 --- a/netbox/ipam/models/vlans.py +++ b/netbox/ipam/models/vlans.py @@ -100,13 +100,16 @@ class VLANGroup(OrganizationalModel): verbose_name_plural = _('VLAN groups') def clean(self): - super().clean() - - # Validate scope assignment + # Validate the scope pair first, since BaseModel.clean() keys its errors to scope_id, which forms omit if self.scope_type and not self.scope_id: - raise ValidationError(_("Cannot set scope_type without scope_id.")) + scope_type = self.scope_type.model_class() + raise ValidationError( + _("Please select a {scope_type}.").format(scope_type=scope_type._meta.model_name) + ) if self.scope_id and not self.scope_type: - raise ValidationError(_("Cannot set scope_id without scope_type.")) + raise ValidationError({'scope_type': _("Please select a scope type.")}) + + super().clean() # Validate VID ranges for vid_range in self.vid_ranges: diff --git a/netbox/ipam/tests/test_forms.py b/netbox/ipam/tests/test_forms.py index 6352effb9..322eba263 100644 --- a/netbox/ipam/tests/test_forms.py +++ b/netbox/ipam/tests/test_forms.py @@ -3,8 +3,11 @@ from django.test import TestCase from dcim.constants import InterfaceTypeChoices from dcim.models import Device, DeviceRole, DeviceType, Interface, Location, Manufacturer, Region, Site, SiteGroup -from ipam.forms import PrefixForm, VLANIDBulkCreateForm +from ipam.choices import PrefixStatusChoices +from ipam.constants import VLANGROUP_SCOPE_TYPES +from ipam.forms import PrefixForm, VLANGroupBulkEditForm, VLANGroupForm, VLANIDBulkCreateForm from ipam.forms.bulk_import import IPAddressImportForm +from ipam.models import Prefix, VLANGroup class PrefixFormTestCase(TestCase): @@ -52,6 +55,26 @@ class PrefixFormTestCase(TestCase): }) assert 'data-dynamic-params' not in form.fields['vlan'].widget.attrs + def test_scope_type_change_without_scope(self): + """Changing the scope type without selecting a scope is reported on the scope field.""" + prefix = Prefix.objects.create( + prefix='10.0.0.0/24', + scope_type=ContentType.objects.get_for_model(Site), + scope_id=self.site.pk, + ) + form = PrefixForm( + data={ + 'prefix': '10.0.0.0/24', + 'status': PrefixStatusChoices.STATUS_ACTIVE, + 'scope_type': ContentType.objects.get_for_model(Location).pk, + 'scope': '', + }, + instance=prefix, + ) + + self.assertFalse(form.is_valid()) + self.assertIn('scope', form.errors) + class IPAddressImportFormTestCase(TestCase): """Tests for IPAddressImportForm bulk import behavior.""" @@ -203,3 +226,74 @@ class VLANFormTestCase(TestCase): form = VLANIDBulkCreateForm({'pattern': pattern}) self.assertFalse(form.is_valid()) self.assertIn('pattern', form.errors) + + +class VLANGroupFormTestCase(TestCase): + + @classmethod + def setUpTestData(cls): + cls.site = Site.objects.create(name='Site 1', slug='site-1') + cls.site_type = ContentType.objects.get_for_model(Site) + cls.location_type = ContentType.objects.get_for_model(Location) + cls.vlan_group = VLANGroup.objects.create( + name='VLAN Group 1', + slug='vlan-group-1', + scope=cls.site, + ) + + def test_scope_can_be_cleared(self): + """Clearing scope type and scope on an existing group nulls the assignment.""" + form = VLANGroupForm( + data=self.get_form_data(scope_type='', scope=''), + instance=VLANGroup.objects.get(pk=self.vlan_group.pk), + ) + self.assertTrue(form.is_valid(), form.errors) + + vlan_group = form.save() + vlan_group.refresh_from_db() + self.assertIsNone(vlan_group.scope_type_id) + self.assertIsNone(vlan_group.scope_id) + + def test_scope_required_with_scope_type(self): + """A scope type without a scope is reported on the scope field.""" + forms = { + 'existing group': VLANGroupForm( + data=self.get_form_data(scope=''), + instance=VLANGroup.objects.get(pk=self.vlan_group.pk), + ), + 'new group': VLANGroupForm( + data=self.get_form_data(name='VLAN Group 2', slug='vlan-group-2', scope=''), + ), + 'retyped group': VLANGroupForm( + data=self.get_form_data(scope_type=self.location_type.pk, scope=''), + instance=VLANGroup.objects.get(pk=self.vlan_group.pk), + ), + } + for case, form in forms.items(): + with self.subTest(case=case): + self.assertFalse(form.is_valid()) + self.assertIn('scope', form.errors) + + def test_scope_initial_retained_for_new_group(self): + """A prepopulated scope survives instantiation of an unsaved group.""" + form = VLANGroupForm(initial={'scope_type': self.site_type.pk, 'scope': self.site.pk}) + + self.assertEqual(form.initial['scope'], self.site.pk) + + def test_scope_type_choices(self): + """Both VLAN group forms offer every VLAN group scope type.""" + for form_class in (VLANGroupForm, VLANGroupBulkEditForm): + with self.subTest(form=form_class.__name__): + form = form_class() + models = set(form.fields['scope_type'].queryset.values_list('model', flat=True)) + self.assertEqual(models, set(VLANGROUP_SCOPE_TYPES)) + + def get_form_data(self, **overrides): + return { + 'name': self.vlan_group.name, + 'slug': self.vlan_group.slug, + 'vid_ranges': '1-4094', + 'scope_type': self.site_type.pk, + 'scope': self.site.pk, + **overrides, + }