RomanNum3ral
/
netbox
mirror of https://github.com/netbox-community/netbox.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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
This commit is contained in:
Martin Hauser 2026-08-27 17:38:50 +02:00 committed by Jeremy Stretch
parent dcc6afcf30
commit 60f80c8ad2
5 changed files with 112 additions and 72 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
netbox/dcim/forms/mixins.py
View File

@ -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:

28
netbox/ipam/forms/bulk_edit.py
View File

@ -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(

40
netbox/ipam/forms/model_forms.py
View File

@ -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(

13
netbox/ipam/models/vlans.py
View File

@ -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:

96
netbox/ipam/tests/test_forms.py
View File

@ -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,
}