Merge pull request #2816 from nautobot/bug-changing_interface_mode

changes/2816.fixed

@@ -0,0 +1 @@
+Fixed issue where changing the interface mode first required removing tagged_vlans in a different request.

nautobot/dcim/api/serializers.py

@@ -975,10 +975,29 @@ class Meta:
         ]
 
 
+class InterfaceCommonSerializer(NautobotModelSerializer, TaggedModelSerializerMixin):
+    def validate(self, data):
+
+        # Validate many-to-many VLAN assignments
+        mode = data.get("mode", getattr(self.instance, "mode", None))
+
+        if mode != InterfaceModeChoices.MODE_TAGGED:
+            if data.get("tagged_vlans"):
+                raise serializers.ValidationError(
+                    {
+                        "tagged_vlans": f"Mode must be set to {InterfaceModeChoices.MODE_TAGGED} when specifying tagged_vlans"
+                    }
+                )
+
+            if data.get("tagged_vlans") != [] and self.instance and self.instance.tagged_vlans.exists():
+                raise serializers.ValidationError({"tagged_vlans": f"Clear tagged_vlans to set mode to {mode}"})
+
+        return super().validate(data)
+
+
 # 2.0 TODO: This becomes non-default in 2.0, removed in 2.2.
 class InterfaceSerializerVersion12(
-    NautobotModelSerializer,
-    TaggedModelSerializerMixin,
+    InterfaceCommonSerializer,
     CableTerminationModelSerializerMixin,
     PathEndpointModelSerializerMixin,
 ):
@@ -1045,16 +1064,6 @@ def validate(self, data):
                 )
 
         # Validate many-to-many VLAN assignments
-        mode = data.get("mode", getattr(self.instance, "mode", None))
-        has_tagged_vlans = data.get("tagged_vlans", False)
-        if not has_tagged_vlans and self.instance and self.instance.tagged_vlans.exists():
-            has_tagged_vlans = True
-
-        if has_tagged_vlans and mode != InterfaceModeChoices.MODE_TAGGED:
-            raise serializers.ValidationError(
-                {"tagged_vlans": f"Mode must be set to {InterfaceModeChoices.MODE_TAGGED} when specifying tagged_vlans"}
-            )
-
         device = self.instance.device if self.instance else data.get("device")
         for vlan in data.get("tagged_vlans", []):
             if vlan.site not in [device.site, None]:

nautobot/dcim/forms.py

@@ -183,17 +183,21 @@ def clean(self):
 
         parent_field = "device" if "device" in self.cleaned_data else "virtual_machine"
         tagged_vlans = self.cleaned_data["tagged_vlans"]
+        mode = self.cleaned_data["mode"]
 
         # Untagged interfaces cannot be assigned tagged VLANs
-        if self.cleaned_data["mode"] == InterfaceModeChoices.MODE_ACCESS and tagged_vlans:
+        if mode == InterfaceModeChoices.MODE_ACCESS and tagged_vlans:
             raise forms.ValidationError({"mode": "An access interface cannot have tagged VLANs assigned."})
 
+        if mode != InterfaceModeChoices.MODE_TAGGED and tagged_vlans:
+            raise forms.ValidationError({"tagged_vlans": f"Clear tagged_vlans to set mode to {self.mode}"})
+
         # Remove all tagged VLAN assignments from "tagged all" interfaces
-        elif self.cleaned_data["mode"] == InterfaceModeChoices.MODE_TAGGED_ALL:
+        elif mode == InterfaceModeChoices.MODE_TAGGED_ALL:
             self.cleaned_data["tagged_vlans"] = []
 
         # Validate tagged VLANs; must be a global VLAN or in the same site
-        elif self.cleaned_data["mode"] == InterfaceModeChoices.MODE_TAGGED:
+        elif mode == InterfaceModeChoices.MODE_TAGGED:
             valid_sites = [None, self.cleaned_data[parent_field].site]
             invalid_vlans = [str(v) for v in tagged_vlans if v.site not in valid_sites]
 
@@ -2689,7 +2693,7 @@ class InterfaceFilterForm(DeviceComponentFilterForm, StatusModelFilterFormMixin)
     tag = TagFilterField(model)
 
 
-class InterfaceForm(NautobotModelForm, InterfaceCommonForm):
+class InterfaceForm(InterfaceCommonForm, NautobotModelForm):
     parent_interface = DynamicModelChoiceField(
         queryset=Interface.objects.all(),
         required=False,

nautobot/dcim/models/device_components.py

@@ -519,9 +519,6 @@ def clean(self):
         if not self.mode and self.untagged_vlan is not None:
             raise ValidationError({"untagged_vlan": "Mode must be set when specifying untagged_vlan"})
 
-        if self.tagged_vlans.exists() and self.mode != InterfaceModeChoices.MODE_TAGGED:
-            raise ValidationError({"tagged_vlans": f"Clear tagged_vlans to set mode to {self.mode}"})
-
     def save(self, *args, **kwargs):
 
         if not self.status:

nautobot/dcim/tests/test_api.py

@@ -1839,13 +1839,31 @@ def test_tagged_vlan_raise_error_if_mode_not_set_to_tagged(self):
                 type=InterfaceTypeChoices.TYPE_VIRTUAL,
             )
             interface.tagged_vlans.add(self.vlans[0])
-            payload = {"mode": None}
+            payload = {"mode": None, "tagged_vlans": [self.vlans[2].pk]}
             response = self.client.patch(self._get_detail_url(interface), data=payload, format="json", **self.header)
             self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
             self.assertEqual(
                 response.data["tagged_vlans"][0], "Mode must be set to tagged when specifying tagged_vlans"
             )
 
+    def test_change_mode_from_tagged_to_others(self):
+        self.add_permissions("dcim.change_interface")
+        interface = Interface.objects.first()
+        interface.mode = InterfaceModeChoices.MODE_TAGGED
+        interface.validated_save()
+        interface.tagged_vlans.add(self.vlans[0])
+
+        with self.subTest("Update Fail"):
+            payload = {"mode": InterfaceModeChoices.MODE_ACCESS}
+            response = self.client.patch(self._get_detail_url(interface), data=payload, format="json", **self.header)
+            self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
+            self.assertEqual(response.data["tagged_vlans"][0], "Clear tagged_vlans to set mode to access")
+
+        with self.subTest("Update Successful"):
+            payload = {"mode": InterfaceModeChoices.MODE_ACCESS, "tagged_vlans": []}
+            response = self.client.patch(self._get_detail_url(interface), data=payload, format="json", **self.header)
+            self.assertHttpStatus(response, status.HTTP_200_OK)
+
 
 class InterfaceTestVersion14(InterfaceTestVersion12):
     api_version = "1.4"

nautobot/dcim/tests/test_models.py

@@ -9,7 +9,6 @@
     CableStatusChoices,
     CableTypeChoices,
     DeviceFaceChoices,
-    InterfaceModeChoices,
     InterfaceTypeChoices,
     PortTypeChoices,
     PowerOutletFeedLegChoices,
@@ -1334,20 +1333,6 @@ def test_tagged_vlan_raise_error_if_mode_not_set_to_tagged(self):
             err.exception.message_dict["tagged_vlans"][0], "Mode must be set to tagged when specifying tagged_vlans"
         )
 
-    def test_tagged_vlan_raise_error_if_mode_is_changed_without_clearing_tagged_vlans(self):
-        interface = Interface.objects.create(
-            name="Int2",
-            type=InterfaceTypeChoices.TYPE_VIRTUAL,
-            device=self.device,
-            mode=InterfaceModeChoices.MODE_TAGGED,
-        )
-        interface.tagged_vlans.add(self.vlan)
-
-        interface.mode = InterfaceModeChoices.MODE_ACCESS
-        with self.assertRaises(ValidationError) as err:
-            interface.validated_save()
-        self.assertEqual(err.exception.message_dict["tagged_vlans"][0], "Clear tagged_vlans to set mode to access")
-
 
 class SiteTestCase(TestCase):
     def test_latitude_or_longitude(self):

nautobot/virtualization/api/serializers.py

@@ -11,6 +11,7 @@
     NestedPlatformSerializer,
     NestedSiteSerializer,
 )
+from nautobot.dcim.api.serializers import InterfaceCommonSerializer
 from nautobot.dcim.choices import InterfaceModeChoices
 from nautobot.extras.api.serializers import (
     NautobotModelSerializer,
@@ -167,7 +168,7 @@ def get_config_context(self, obj):
 
 
 # 2.0 TODO: This becomes non-default in 2.0, removed in 2.2.
-class VMInterfaceSerializerVersion12(NautobotModelSerializer, TaggedModelSerializerMixin):
+class VMInterfaceSerializerVersion12(InterfaceCommonSerializer):
     url = serializers.HyperlinkedIdentityField(view_name="virtualization-api:vminterface-detail")
     virtual_machine = NestedVirtualMachineSerializer()
     mode = ChoiceField(choices=InterfaceModeChoices, allow_blank=True, required=False)
@@ -217,17 +218,6 @@ def validate(self, data):
                 )
 
         # Validate many-to-many VLAN assignments
-        # if self.instance and self.instance.tagged_vlans.exists()
-        mode = data.get("mode", getattr(self.instance, "mode", None))
-        has_tagged_vlans = data.get("tagged_vlans", False)
-        if not has_tagged_vlans and self.instance and self.instance.tagged_vlans.exists():
-            has_tagged_vlans = True
-
-        if has_tagged_vlans and mode != InterfaceModeChoices.MODE_TAGGED:
-            raise serializers.ValidationError(
-                {"tagged_vlans": f"Mode must be set to {InterfaceModeChoices.MODE_TAGGED} when specifying tagged_vlans"}
-            )
-
         virtual_machine = self.instance.virtual_machine if self.instance else data.get("virtual_machine")
         for vlan in data.get("tagged_vlans", []):
             if vlan.site not in [virtual_machine.site, None]:

nautobot/virtualization/tests/test_api.py

@@ -383,7 +383,7 @@ def test_untagged_vlan_requires_mode(self):
 
     def test_tagged_vlan_raise_error_if_mode_not_set_to_tagged(self):
         self.add_permissions("virtualization.add_vminterface", "virtualization.change_vminterface")
-        vlan = VLAN.objects.first()
+        vlan = VLAN.objects.get(name="VLAN 1")
         virtualmachine = VirtualMachine.objects.first()
         with self.subTest("On create, assert 400 status."):
             payload = {
@@ -406,13 +406,32 @@ def test_tagged_vlan_raise_error_if_mode_not_set_to_tagged(self):
                 mode=InterfaceModeChoices.MODE_TAGGED,
             )
             interface.tagged_vlans.add(vlan)
-            payload = {"mode": None}
+            payload = {"mode": None, "tagged_vlans": [vlan.pk]}
             response = self.client.patch(self._get_detail_url(interface), data=payload, format="json", **self.header)
             self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
             self.assertEqual(
                 response.data["tagged_vlans"][0], "Mode must be set to tagged when specifying tagged_vlans"
             )
 
+    def test_change_mode_from_tagged_to_others(self):
+        self.add_permissions("virtualization.change_vminterface")
+        vlan = VLAN.objects.get(name="VLAN 1")
+        interface = VMInterface.objects.first()
+        interface.mode = InterfaceModeChoices.MODE_TAGGED
+        interface.validated_save()
+        interface.tagged_vlans.add(vlan)
+
+        with self.subTest("Update Fail"):
+            payload = {"mode": InterfaceModeChoices.MODE_ACCESS}
+            response = self.client.patch(self._get_detail_url(interface), data=payload, format="json", **self.header)
+            self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
+            self.assertEqual(response.data["tagged_vlans"][0], "Clear tagged_vlans to set mode to access")
+
+        with self.subTest("Update Successful"):
+            payload = {"mode": InterfaceModeChoices.MODE_ACCESS, "tagged_vlans": []}
+            response = self.client.patch(self._get_detail_url(interface), data=payload, format="json", **self.header)
+            self.assertHttpStatus(response, status.HTTP_200_OK)
+
 
 class VMInterfaceTestVersion14(VMInterfaceTestVersion12):
     api_version = "1.4"

nautobot/virtualization/tests/test_models.py

@@ -2,7 +2,6 @@
 from django.core.exceptions import ValidationError
 from django.test import TestCase
 
-from nautobot.dcim.choices import InterfaceModeChoices
 from nautobot.dcim.models import Location, LocationType, Site
 from nautobot.extras.models import Status
 from nautobot.ipam.models import VLAN
@@ -88,14 +87,3 @@ def test_tagged_vlan_raise_error_if_mode_not_set_to_tagged(self):
         self.assertEqual(
             err.exception.message_dict["tagged_vlans"][0], "Mode must be set to tagged when specifying tagged_vlans"
         )
-
-    def test_tagged_vlan_raise_error_if_mode_is_changed_without_clearing_tagged_vlans(self):
-        interface = VMInterface.objects.create(
-            virtual_machine=self.virtualmachine, name="Interface 1", mode=InterfaceModeChoices.MODE_TAGGED
-        )
-        interface.tagged_vlans.add(self.vlan)
-
-        interface.mode = InterfaceModeChoices.MODE_ACCESS
-        with self.assertRaises(ValidationError) as err:
-            interface.validated_save()
-        self.assertEqual(err.exception.message_dict["tagged_vlans"][0], "Clear tagged_vlans to set mode to access")