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