Przeglądaj źródła

Closes #15165: Pre-release QA (#22888)

Move the `HTMXSelect` configuration for `InterfaceForm` and
`VMInterfaceForm` onto their explicitly declared `mode` fields so that
changing the 802.1Q mode again refreshes the dependent VLAN fields.

Make `HTMXSelect` description-aware, isolate copied description mappings,
and fix the existing shadowed `VirtualChassisForm.master` widget. Remove
other ineffective `ModelForm.Meta` entries.

Add regression coverage for partial and full-form HTMX swaps, together
with a repository-wide guard against declared fields shadowing supported
`ModelForm.Meta` options.
Jason Novinger 1 tydzień temu
rodzic
commit
99f441b090

+ 2 - 7
netbox/dcim/forms/model_forms.py

@@ -1242,6 +1242,7 @@ class VirtualChassisForm(PrimaryModelForm):
         label=_('Master'),
         queryset=Device.objects.all(),
         required=False,
+        widget=SelectWithPK(),
     )
 
     class Meta:
@@ -1249,9 +1250,6 @@ class VirtualChassisForm(PrimaryModelForm):
         fields = [
             'name', 'domain', 'master', 'description', 'owner', 'comments', 'tags',
         ]
-        widgets = {
-            'master': SelectWithPK(),
-        }
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
@@ -2043,6 +2041,7 @@ class InterfaceForm(InterfaceCommonForm, ModularDeviceComponentForm):
         choices=add_blank_choice(InterfaceModeChoices),
         required=False,
         help_text=_('IEEE 802.1Q tagging strategy'),
+        widget=HTMXSelect(hx_target_id='dot1q-switching'),
     )
     rf_role = TypedChoiceField(
         label=_('Wireless role'),
@@ -2186,10 +2185,6 @@ class InterfaceForm(InterfaceCommonForm, ModularDeviceComponentForm):
             'speed': NumberWithOptions(
                 options=InterfaceSpeedChoices
             ),
-            'mode': HTMXSelect(hx_target_id='dot1q-switching'),
-        }
-        labels = {
-            'mode': '802.1Q Mode',
         }
 
 

+ 0 - 5
netbox/extras/forms/model_forms.py

@@ -129,10 +129,6 @@ class CustomFieldForm(ChangelogMessageMixin, OwnerMixin, forms.ModelForm):
         model = CustomField
         fields = '__all__'
         help_texts = {
-            'type': _(
-                "The type of data stored in this field. For object/multi-object fields, select the related object "
-                "type below."
-            ),
             'description': _("This will be displayed as help text for the form field. Markdown is supported.")
         }
 
@@ -641,7 +637,6 @@ class EventRuleForm(OwnerMixin, NetBoxModelForm):
             'action_object_type', 'action_object_id', 'action_data', 'owner', 'comments', 'tags'
         )
         widgets = {
-            'conditions': forms.Textarea(attrs={'class': 'font-monospace'}),
             'action_object_type': forms.HiddenInput,
             'action_object_id': forms.HiddenInput,
         }

+ 177 - 1
netbox/netbox/tests/test_forms.py

@@ -1,8 +1,25 @@
+from django import forms
+from django.forms.models import ALL_FIELDS
 from django.test import TestCase
 
+from circuits.forms import CircuitGroupAssignmentForm, CircuitTerminationForm
+from core.forms import DataSourceForm
 from dcim.choices import InterfaceTypeChoices
-from dcim.forms import InterfaceImportForm
+from dcim.forms import (
+    CableForm,
+    FrontPortCreateForm,
+    InterfaceForm,
+    InterfaceImportForm,
+    ModuleTypeForm,
+    RackForm,
+)
 from dcim.models import Device, DeviceRole, DeviceType, Interface, Manufacturer, Site
+from extras.forms import CustomFieldForm, EventRuleForm
+from ipam.forms import PrefixForm, ServiceForm, VLANGroupForm
+from utilities.forms.widgets import HTMXSelect
+from virtualization.forms import ClusterForm, VirtualMachineForm, VMInterfaceForm
+from vpn.forms import TunnelCreateForm, TunnelTerminationForm
+from wireless.forms import WirelessLANForm
 
 
 class NetBoxModelImportFormCleanTestCase(TestCase):
@@ -301,3 +318,162 @@ class NetBoxModelImportFormCleanTestCase(TestCase):
         )
         self.assertTrue(form.is_valid(), f'Form errors: {form.errors}')
         self.assertIsNone(form.cleaned_data['wwn'])
+
+
+class HTMXPartialSwapRenderingTestCase(TestCase):
+    """Ensure each form field's HTMX swap wiring is present on the widget bound to the field."""
+
+    # (form, bound field name, target fieldset id)
+    PARTIAL_SWAP_FIELDS = (
+        (InterfaceForm, 'mode', 'dot1q-switching'),
+        (VMInterfaceForm, 'mode', 'dot1q-switching'),
+        (ModuleTypeForm, 'profile', 'profile-attributes'),
+        (CableForm, 'a_terminations_type', 'cable-side-a'),
+        (CableForm, 'b_terminations_type', 'cable-side-b'),
+        (VLANGroupForm, 'scope', 'scope'),
+        (ServiceForm, 'parent', 'service'),
+        (CircuitTerminationForm, 'termination', 'circuit-termination'),
+        (CircuitGroupAssignmentForm, 'member', 'circuit-group-assignment'),
+        (TunnelCreateForm, 'termination1_type', 'tunnel-termination1'),
+        (TunnelCreateForm, 'termination2_type', 'tunnel-termination2'),
+        (TunnelTerminationForm, 'type', 'tunnel-termination'),
+        (EventRuleForm, 'action_type', 'event-rule-action'),
+        (ClusterForm, 'scope', 'scope'),
+        (WirelessLANForm, 'scope', 'scope'),
+        (PrefixForm, 'scope', 'scope'),
+    )
+
+    # Fields that intentionally re-render the whole form (#form_fields) rather than a single fieldset.
+    FULL_FORM_FIELDS = (
+        (CustomFieldForm, 'type'),
+        (DataSourceForm, 'type'),
+        (VirtualMachineForm, 'virtual_machine_type'),
+        (RackForm, 'rack_type'),
+        (FrontPortCreateForm, 'device'),
+    )
+
+    # CableForm targets template <div>s, not FieldSet(html_id=...), so its target can't be checked
+    # against form.fieldsets.
+    FIELDSET_ID_EXEMPT = {CableForm}
+
+    @staticmethod
+    def _hx_widget(field):
+        """Return the HTMXSelect carrying the field's HTMX attrs, unwrapping a MultiWidget by type."""
+        widget = field.widget
+        if isinstance(widget, forms.MultiWidget):
+            hx = next((w for w in widget.widgets if isinstance(w, HTMXSelect)), None)
+            assert hx is not None, f'{type(widget).__name__} has no HTMXSelect subwidget'
+            return hx
+        return widget
+
+    def test_partial_swap_fields_target_their_fieldset(self):
+        for form_class, field_name, target_id in self.PARTIAL_SWAP_FIELDS:
+            with self.subTest(form=form_class.__name__, field=field_name):
+                form = form_class()
+                self.assertIn(field_name, form.fields)
+                attrs = self._hx_widget(form.fields[field_name]).attrs
+                # hx-get issues the request; hx-target/hx-select alone are inert.
+                self.assertIn('hx-get', attrs)
+                self.assertEqual(attrs.get('hx-select'), f'#{target_id}')
+                self.assertEqual(attrs.get('hx-target'), f'#{target_id}')
+                # The target must name a declared FieldSet html_id, else the swap fails silently.
+                if form_class not in self.FIELDSET_ID_EXEMPT:
+                    fieldset_ids = {getattr(fs, 'html_id', None) for fs in getattr(form, 'fieldsets', ())}
+                    self.assertIn(target_id, fieldset_ids)
+
+    def test_interface_mode_retains_option_descriptions(self):
+        # Partial swap and option descriptions must coexist on the mode field.
+        for form_class in (InterfaceForm, VMInterfaceForm):
+            with self.subTest(form=form_class.__name__):
+                self.assertTrue(form_class().fields['mode'].widget.descriptions)
+
+    def test_full_form_fields_do_not_partial_swap(self):
+        for form_class, field_name in self.FULL_FORM_FIELDS:
+            with self.subTest(form=form_class.__name__, field=field_name):
+                form = form_class()
+                self.assertIn(field_name, form.fields)
+                attrs = self._hx_widget(form.fields[field_name]).attrs
+                self.assertIn('hx-get', attrs)
+                self.assertEqual(attrs.get('hx-target'), '#form_fields')
+                self.assertNotIn('hx-select', attrs)
+
+
+class MetaShadowingTestCase(TestCase):
+    """Ensure declared fields do not shadow supported `ModelForm.Meta` configuration."""
+    # Known overlaps whose cleanup is deferred, as specific (form, Meta attribute, field) triples so
+    # that any new overlap on the same form is still caught. ConfigRevisionForm builds these fields
+    # via ConfigFormMetaclass; restoring their monospace widget is tracked separately (see #22889).
+    ALLOWED = {
+        ('ConfigRevisionForm', 'widgets', 'BANNER_LOGIN'),
+        ('ConfigRevisionForm', 'widgets', 'BANNER_MAINTENANCE'),
+        ('ConfigRevisionForm', 'widgets', 'BANNER_TOP'),
+        ('ConfigRevisionForm', 'widgets', 'BANNER_BOTTOM'),
+        ('ConfigRevisionForm', 'widgets', 'CUSTOM_VALIDATORS'),
+        ('ConfigRevisionForm', 'widgets', 'PROTECTION_RULES'),
+    }
+
+    @staticmethod
+    def _all_model_forms():
+        # Import every app's forms package so all ModelForm subclasses are registered before walking.
+        import importlib
+        import pkgutil
+        for app in (
+            'circuits', 'core', 'dcim', 'extras', 'ipam', 'tenancy', 'users', 'utilities',
+            'virtualization', 'vpn', 'wireless', 'netbox',
+        ):
+            # Only tolerate an app that has no forms package; a broken import inside a package that
+            # does exist must surface, or the invariant could pass having skipped part of the tree.
+            pkg_name = f'{app}.forms'
+            try:
+                pkg = importlib.import_module(pkg_name)
+            except ModuleNotFoundError as e:
+                if e.name == pkg_name:
+                    continue
+                raise
+            for module in getattr(pkg, '__path__', []) and pkgutil.iter_modules(pkg.__path__) or []:
+                submodule = f'{app}.forms.{module.name}'
+                try:
+                    importlib.import_module(submodule)
+                except ModuleNotFoundError as e:
+                    if e.name == submodule:
+                        continue
+                    raise
+
+        seen, stack = set(), [forms.ModelForm]
+        while stack:
+            for sub in stack.pop().__subclasses__():
+                if sub not in seen:
+                    seen.add(sub)
+                    stack.append(sub)
+        return seen
+
+    @staticmethod
+    def _configured_field_names(configured, declared):
+        # Field names a Meta option targets. localized_fields may be the ALL_FIELDS sentinel
+        # ('__all__') meaning every field; set() of that string would yield its characters, so map
+        # the sentinel to the full declared set instead.
+        if configured == ALL_FIELDS:
+            return set(declared)
+        return set(configured or ())
+
+    def test_meta_config_does_not_shadow_declared_fields(self):
+        for form_class in self._all_model_forms():
+            meta = getattr(form_class, 'Meta', None)
+            declared = set(getattr(form_class, 'declared_fields', {}))
+            if meta is None or not declared:
+                continue
+            for attr in ('widgets', 'labels', 'help_texts', 'error_messages', 'field_classes', 'localized_fields'):
+                configured = self._configured_field_names(getattr(meta, attr, None), declared)
+                overlap = declared & configured
+                overlap -= {f for f in overlap if (form_class.__name__, attr, f) in self.ALLOWED}
+                with self.subTest(form=form_class.__name__, meta=attr):
+                    self.assertEqual(
+                        overlap, set(),
+                        f"{form_class.__module__}.{form_class.__name__} sets Meta.{attr} for "
+                        f"explicitly declared field(s) {sorted(overlap)}; Django discards these. "
+                        f"Move the config onto the declared field or drop the Meta entry."
+                    )
+
+    def test_localized_fields_all_sentinel_expands_to_declared(self):
+        # The ALL_FIELDS sentinel must expand to the declared fields, not char-split into letters.
+        self.assertEqual(self._configured_field_names(ALL_FIELDS, {'name', 'status'}), {'name', 'status'})

+ 8 - 2
netbox/utilities/forms/widgets/select.py

@@ -26,6 +26,11 @@ class AttrSelectMixin:
         super().__init__(*args, **kwargs)
         self.descriptions = descriptions or {}
 
+    def __deepcopy__(self, memo):
+        obj = super().__deepcopy__(memo)
+        obj.descriptions = self.descriptions.copy()
+        return obj
+
     def create_option(self, name, value, label, selected, index, subindex=None, attrs=None):
         option = super().create_option(name, value, label, selected, index, subindex, attrs)
 
@@ -90,9 +95,10 @@ class ColorSelect(forms.Select):
         self.attrs['class'] = 'color-select'
 
 
-class HTMXSelect(forms.Select):
+class HTMXSelect(Select):
     """
-    Selection widget that will re-generate the HTML form upon the selection of a new option.
+    Selection widget that re-generates the HTML form upon selection of a new option, and supports
+    per-option descriptions alongside its HTMX behavior.
     """
     def __init__(self, method='get', hx_url='.', hx_include_id='form_fields', hx_target_id=None, attrs=None, **kwargs):
         method = method.lower()

+ 8 - 0
netbox/utilities/tests/test_forms.py

@@ -1,3 +1,4 @@
+import copy
 import warnings
 from types import SimpleNamespace
 
@@ -1042,6 +1043,13 @@ class DescriptionSelectTestCase(TestCase):
         self.assertInHTML('<option value="x" data-description="Description X">X</option>', html)
         self.assertInHTML('<option value="y">Y</option>', html)
 
+    def test_deepcopy_does_not_share_descriptions(self):
+        widget = Select(choices=[('x', 'X')], descriptions={'x': 'Description X'})
+        clone = copy.deepcopy(widget)
+        self.assertIsNot(widget.descriptions, clone.descriptions)
+        widget.descriptions['y'] = 'leaked'
+        self.assertNotIn('y', clone.descriptions)
+
     def test_choices_setter_delegates_through_mro(self):
         """
         AttrChoiceMixin must delegate to the parent field's choices setter via the MRO, not a hardcoded base,

+ 1 - 6
netbox/virtualization/forms/model_forms.py

@@ -422,6 +422,7 @@ class VMInterfaceForm(InterfaceCommonForm, VMComponentForm):
         choices=add_blank_choice(InterfaceModeChoices),
         required=False,
         help_text=_('IEEE 802.1Q tagging strategy'),
+        widget=HTMXSelect(hx_target_id='dot1q-switching'),
     )
     primary_mac_address = DynamicModelChoiceField(
         queryset=MACAddress.objects.all(),
@@ -509,12 +510,6 @@ class VMInterfaceForm(InterfaceCommonForm, VMComponentForm):
             'untagged_vlan', 'tagged_vlans', 'qinq_svlan', 'vlan_translation_policy', 'vrf',
             'owner', 'tags',
         ]
-        labels = {
-            'mode': _('802.1Q Mode'),
-        }
-        widgets = {
-            'mode': HTMXSelect(hx_target_id='dot1q-switching'),
-        }
 
 
 class VirtualDiskForm(VMComponentForm):

+ 0 - 4
netbox/wireless/forms/model_forms.py

@@ -228,7 +228,3 @@ class WirelessLinkForm(DistanceValidationMixin, TenancyForm, PrimaryModelForm):
                 attrs={'data-toggle': 'password'}
             ),
         }
-        labels = {
-            'auth_type': 'Type',
-            'auth_cipher': 'Cipher',
-        }