Closes #23486: Record virtual chassis member changes with prechange data (#23522)

Snapshot members before updates and keep temporary position resets out
of the changelog. Keep chassis deletion and member cleanup in the same
database transaction.

netbox/dcim/models/devices.py

@@ -1344,14 +1344,19 @@ class VirtualChassis(PrimaryModel):
                 set(interfaces),
             )
 
-        # Clear vc_position and vc_priority on member devices BEFORE calling super().delete()
-        # This must be done here because on_delete=SET_NULL executes before pre_delete signal
-        for device in self.members.all():
-            device.vc_position = None
-            device.vc_priority = None
-            device.save()
+        # Core's pre_delete handler detaches each member (clearing virtual_chassis) and records that change. Clear the
+        # now-meaningless position and priority afterwards, as a second change which is valid on its own.
+        using = kwargs.get('using') or router.db_for_write(self.__class__, instance=self)
+        member_ids = list(self.members.values_list('pk', flat=True))
+        with transaction.atomic(using=using):
+            ret = super().delete(*args, **kwargs)
+            for device in Device.objects.using(using).filter(pk__in=member_ids):
+                device.snapshot()
+                device.vc_position = None
+                device.vc_priority = None
+                device.save(using=using)
 
-        return super().delete(*args, **kwargs)
+        return ret
 
 
 class VirtualDeviceContext(PrimaryModel):

netbox/dcim/signals.py

@@ -233,6 +233,7 @@ def assign_virtualchassis_master(instance, created, **kwargs):
     """
     if created and instance.master:
         master = Device.objects.get(pk=instance.master.pk)
+        master.snapshot()
         master.virtual_chassis = instance
         master.vc_position = 1
         master.save()

netbox/dcim/tests/test_api.py

@@ -6328,6 +6328,29 @@ class VirtualChassisTestCase(APIViewTestCases.APIViewTestCase):
         member.refresh_from_db()
         self.assertEqual(member.virtual_chassis.name, 'Virtual Chassis 1')
 
+    @tag('regression')  # Ref: #23486
+    def test_create_with_master_records_prechange_data(self):
+        self.add_permissions('dcim.add_virtualchassis')
+        master = Device.objects.get(name='Device 10')
+
+        data = {'name': 'Virtual Chassis 4', 'master': master.pk}
+        response = self.client.post(self._get_list_url(), data, format='json', **self.header)
+        self.assertHttpStatus(response, status.HTTP_201_CREATED)
+
+        master.refresh_from_db()
+        self.assertEqual(master.virtual_chassis.pk, response.data['id'])
+        self.assertEqual(master.vc_position, 1)
+
+        change = ObjectChange.objects.get(
+            changed_object_type=ContentType.objects.get_for_model(Device),
+            changed_object_id=master.pk,
+        )
+        self.assertEqual(change.action, ObjectChangeActionChoices.ACTION_UPDATE)
+        self.assertIsNone(change.prechange_data['virtual_chassis'])
+        self.assertIsNone(change.prechange_data['vc_position'])
+        self.assertEqual(change.postchange_data['virtual_chassis'], response.data['id'])
+        self.assertEqual(change.postchange_data['vc_position'], 1)
+
 
 class PowerPanelTestCase(APIViewTestCases.APIViewTestCase):
     model = PowerPanel

netbox/dcim/tests/test_views.py

@@ -5748,6 +5748,140 @@ class VirtualChassisTestCase(ViewTestCases.PrimaryObjectViewTestCase):
         self.assertIsNone(member.virtual_chassis)
         self.assertIsNone(member.vc_position)
 
+    def _get_member_changes(self, device):
+        return ObjectChange.objects.filter(
+            changed_object_type=ContentType.objects.get_for_model(Device),
+            changed_object_id=device.pk,
+        ).order_by('pk')
+
+    @tag('regression')  # Ref: #23486
+    def test_edit_member_records_single_change_with_prechange_data(self):
+        self.add_permissions('dcim.change_virtualchassis')
+        vc = VirtualChassis.objects.get(name='VC1')
+        member1, member2, member3 = vc.members.order_by('vc_position')
+
+        data = {
+            'name': vc.name,
+            'domain': vc.domain,
+            'master': vc.master.pk,
+            'form-TOTAL_FORMS': 3,
+            'form-INITIAL_FORMS': 3,
+            'form-MIN_NUM_FORMS': 0,
+            'form-MAX_NUM_FORMS': 1000,
+            'form-0-id': member1.pk,
+            'form-0-vc_position': 1,
+            'form-0-vc_priority': 20,
+            'form-1-id': member2.pk,
+            'form-1-vc_position': 2,
+            'form-1-vc_priority': '',
+            'form-2-id': member3.pk,
+            'form-2-vc_position': 3,
+            'form-2-vc_priority': '',
+        }
+        response = self.client.post(self._get_url('edit', vc), data)
+        self.assertHttpStatus(response, 302)
+
+        member1.refresh_from_db()
+        self.assertEqual(member1.vc_position, 1)
+        self.assertEqual(member1.vc_priority, 20)
+
+        # Exactly one change is recorded, spanning the member's original and final state
+        changes = self._get_member_changes(member1)
+        self.assertEqual(changes.count(), 1)
+        change = changes.first()
+        self.assertEqual(change.action, ObjectChangeActionChoices.ACTION_UPDATE)
+        self.assertEqual(change.prechange_data['vc_position'], 1)
+        self.assertIsNone(change.prechange_data['vc_priority'])
+        self.assertEqual(change.postchange_data['vc_position'], 1)
+        self.assertEqual(change.postchange_data['vc_priority'], 20)
+
+        # Unchanged members produce no changelog entries
+        self.assertFalse(self._get_member_changes(member2).exists())
+        self.assertFalse(self._get_member_changes(member3).exists())
+
+    @tag('regression')  # Ref: #23486
+    def test_edit_member_swap_positions(self):
+        self.add_permissions('dcim.change_virtualchassis')
+        vc = VirtualChassis.objects.get(name='VC1')
+        member1, member2, member3 = vc.members.order_by('vc_position')
+
+        data = {
+            'name': vc.name,
+            'domain': vc.domain,
+            'master': vc.master.pk,
+            'form-TOTAL_FORMS': 3,
+            'form-INITIAL_FORMS': 3,
+            'form-MIN_NUM_FORMS': 0,
+            'form-MAX_NUM_FORMS': 1000,
+            'form-0-id': member1.pk,
+            'form-0-vc_position': 2,
+            'form-0-vc_priority': '',
+            'form-1-id': member2.pk,
+            'form-1-vc_position': 1,
+            'form-1-vc_priority': '',
+            'form-2-id': member3.pk,
+            'form-2-vc_position': 3,
+            'form-2-vc_priority': '',
+        }
+        response = self.client.post(self._get_url('edit', vc), data)
+        self.assertHttpStatus(response, 302)
+
+        member1.refresh_from_db()
+        member2.refresh_from_db()
+        self.assertEqual(member1.vc_position, 2)
+        self.assertEqual(member2.vc_position, 1)
+
+        for member, old_position, new_position in ((member1, 1, 2), (member2, 2, 1)):
+            changes = self._get_member_changes(member)
+            self.assertEqual(changes.count(), 1)
+            change = changes.first()
+            self.assertEqual(change.prechange_data['vc_position'], old_position)
+            self.assertEqual(change.postchange_data['vc_position'], new_position)
+            self.assertEqual(change.postchange_data['virtual_chassis'], vc.pk)
+
+    @tag('regression')  # Ref: #23486
+    def test_delete_records_valid_member_changes(self):
+        self.add_permissions('dcim.delete_virtualchassis')
+        vc = VirtualChassis.objects.get(name='VC1')
+        members = list(vc.members.order_by('vc_position'))
+        Device.objects.filter(pk=members[0].pk).update(vc_priority=10)
+
+        response = self.client.post(self._get_url('delete', vc), {'confirm': True})
+        self.assertHttpStatus(response, 302)
+        self.assertFalse(VirtualChassis.objects.filter(pk=vc.pk).exists())
+
+        for member in members:
+            member.refresh_from_db()
+            self.assertIsNone(member.virtual_chassis)
+            self.assertIsNone(member.vc_position)
+            self.assertIsNone(member.vc_priority)
+
+            # Each member is detached and then cleared in two changes, each with prechange data and valid on
+            # its own (a virtual chassis is never recorded without a position)
+            changes = list(self._get_member_changes(member))
+            self.assertEqual(len(changes), 2)
+            detach, clear = changes
+            self.assertEqual(detach.action, ObjectChangeActionChoices.ACTION_UPDATE)
+            self.assertEqual(detach.prechange_data['virtual_chassis'], vc.pk)
+            self.assertIsNotNone(detach.prechange_data['vc_position'])
+            self.assertIsNone(detach.postchange_data['virtual_chassis'])
+            self.assertEqual(detach.postchange_data['vc_position'], detach.prechange_data['vc_position'])
+            self.assertEqual(clear.action, ObjectChangeActionChoices.ACTION_UPDATE)
+            self.assertIsNone(clear.prechange_data['virtual_chassis'])
+            self.assertEqual(clear.prechange_data['vc_position'], detach.postchange_data['vc_position'])
+            self.assertIsNone(clear.postchange_data['vc_position'])
+            self.assertIsNone(clear.postchange_data['vc_priority'])
+        self.assertEqual(self._get_member_changes(members[0]).first().prechange_data['vc_priority'], 10)
+
+        # The VC deletion is recorded once, before its (former) master is detached
+        vc_change = ObjectChange.objects.get(
+            changed_object_type=ContentType.objects.get_for_model(VirtualChassis),
+            changed_object_id=vc.pk,
+        )
+        self.assertEqual(vc_change.action, ObjectChangeActionChoices.ACTION_DELETE)
+        self.assertEqual(vc_change.prechange_data['master'], members[0].pk)
+        self.assertLess(vc_change.pk, self._get_member_changes(members[0]).first().pk)
+
 
 class PowerPanelTestCase(ViewTestCases.PrimaryObjectViewTestCase):
     model = PowerPanel

netbox/dcim/views.py

@@ -5236,6 +5236,11 @@ class VirtualChassisEditView(ObjectPermissionRequiredMixin, GetReturnURLMixin, V
         vc_form.fields['master'].queryset = members_queryset
         formset = VCMemberFormSet(request.POST, queryset=members_queryset)
 
+        # Snapshot each member before validation applies the submitted data to the instance
+        for form in formset.initial_forms:
+            if form.instance.pk:
+                form.instance.snapshot()
+
         if vc_form.is_valid() and formset.is_valid():
             virtual_chassis._changelog_message = vc_form.cleaned_data.pop('changelog_message', '')
 
@@ -5244,14 +5249,10 @@ class VirtualChassisEditView(ObjectPermissionRequiredMixin, GetReturnURLMixin, V
                 # Save the VirtualChassis
                 vc_form.save()
 
-                # Nullify the vc_position of each member first to allow reordering without raising an IntegrityError on
-                # duplicate positions. Then save each member instance.
+                # Nullify vc_position of each changed member first to allow reordering without an IntegrityError on
+                # duplicate positions. A queryset update keeps this transient state out of the changelog.
                 members = formset.save(commit=False)
-                devices = Device.objects.filter(pk__in=[m.pk for m in members])
-                for device in devices:
-                    device.snapshot()
-                    device.vc_position = None
-                    device.save()
+                Device.objects.filter(pk__in=[m.pk for m in members]).update(vc_position=None)
                 for member in members:
                     member.save()
 

netbox/netbox/tests/test_views.py

@@ -4,12 +4,13 @@ from unittest.mock import patch
 
 from django.contrib.contenttypes.models import ContentType
 from django.core.files.uploadedfile import SimpleUploadedFile
+from django.db.models.signals import pre_delete
 from django.http import HttpResponse
 from django.test import Client, TransactionTestCase, override_settings, tag
 from django.urls import reverse
 from django.utils import timezone
 
-from core.models import DataFile, DataSource
+from core.models import DataFile, DataSource, ObjectChange
 from dcim.choices import DeviceStatusChoices, InterfaceTypeChoices, SiteStatusChoices
 from dcim.models import Device, DeviceRole, DeviceType, Interface, Manufacturer, Site, VirtualChassis
 from extras.events import enqueue_event
@@ -22,6 +23,7 @@ from netbox.choices import CSVDelimiterChoices, ImportFormatChoices, ImportMetho
 from netbox.constants import EMPTY_TABLE_TEXT
 from netbox.search.backends import search_backend
 from users.models import User
+from utilities.exceptions import AbortRequest
 from utilities.testing import TestCase
 from utilities.views import get_action_url
 
@@ -364,12 +366,11 @@ class DeleteAtomicityMixin:
     Scenario shared by the transactional and autocommit variants of the ObjectDeleteView
     atomicity test (Ref: #22934).
 
-    VirtualChassis.delete() clears vc_position/vc_priority on each member device and saves them
-    before calling super().delete(), so an aborted deletion must roll those writes back. Otherwise
-    they commit while their queued events are discarded, leaving a committed change which
-    dispatches no events.
+    Core's pre_delete handler detaches each member device of a virtual chassis (clearing its
+    virtual_chassis) ahead of the deletion itself, so a deletion aborted by a later pre_delete
+    receiver must roll that write back. Otherwise it commits while the deletion's queued events
+    are discarded, leaving a committed change which dispatches no events.
     """
-    PROTECTION_RULES = {'dcim.virtualchassis': [CustomValidator({'name': {'eq': 'Nonexistent'}})]}
 
     def create_virtual_chassis(self):
         site = Site.objects.create(name='Site 1', slug='site-1')
@@ -387,16 +388,40 @@ class DeleteAtomicityMixin:
         return virtual_chassis, device
 
     def delete_virtual_chassis(self, virtual_chassis):
-        return self.client.post(
-            get_action_url(VirtualChassis, action='delete', kwargs={'pk': virtual_chassis.pk}),
-            data={'confirm': True}
-        )
+        """
+        Submit the deletion, aborting it from a pre_delete receiver (as a plugin's might) which fires
+        after core's handler has detached the member. A write ahead of VirtualChassis.delete() is
+        covered only by the view's atomic block, not the model's or the collector's.
+        """
+        def abort(sender, instance, **kwargs):
+            raise AbortRequest("Aborted by receiver")
+
+        original_delete = VirtualChassis.delete
+
+        def delete(instance, *args, **kwargs):
+            instance.members.update(vc_priority=None)
+            return original_delete(instance, *args, **kwargs)
+
+        pre_delete.connect(abort, sender=VirtualChassis)
+        try:
+            with patch.object(VirtualChassis, 'delete', delete):
+                return self.client.post(
+                    get_action_url(VirtualChassis, action='delete', kwargs={'pk': virtual_chassis.pk}),
+                    data={'confirm': True}
+                )
+        finally:
+            pre_delete.disconnect(abort, sender=VirtualChassis)
 
     def assertDeleteRolledBack(self, virtual_chassis, device):
         self.assertTrue(VirtualChassis.objects.filter(pk=virtual_chassis.pk).exists())
         device.refresh_from_db()
+        self.assertEqual(device.virtual_chassis_id, virtual_chassis.pk)
         self.assertEqual(device.vc_position, 1)
         self.assertEqual(device.vc_priority, 10)
+        self.assertFalse(ObjectChange.objects.filter(
+            changed_object_type=ContentType.objects.get_for_model(VirtualChassis),
+            changed_object_id=virtual_chassis.pk,
+        ).exists())
 
 
 class RolledBackWriteEventQueueTestCase(DeleteAtomicityMixin, TestCase):
@@ -515,15 +540,11 @@ class RolledBackWriteEventQueueTestCase(DeleteAtomicityMixin, TestCase):
         self.assertTrue(Device.objects.filter(pk=device.pk).exists())
         self.assertTrue(Interface.objects.filter(pk=interface.pk).exists())
 
-    @override_settings(PROTECTION_RULES=DeleteAtomicityMixin.PROTECTION_RULES)
     def test_object_delete_view_rollback_is_atomic(self):
-        # An aborted delete must not leave behind writes performed by the model's delete() before
-        # super().delete(). Note what this detects: every test here runs inside the harness'
-        # transaction, so an escaped write is rolled back with the test regardless, and the
-        # assertions below cannot see it. Without the view's atomic block, AbortRequest instead
-        # escapes the collector's atomic(savepoint=False) and marks the harness' transaction as
-        # needing rollback, so this test fails with TransactionManagementError. The committed
-        # symptom itself is covered by DeleteAtomicityTestCase below.
+        # An aborted delete must not leave behind writes performed earlier in the deletion. Without
+        # the view's atomic block, the write made ahead of VirtualChassis.delete() survives within
+        # the harness' transaction and the assertions below fail. The committed symptom itself is
+        # covered by DeleteAtomicityTestCase below.
         virtual_chassis, device = self.create_virtual_chassis()
         self.add_permissions('dcim.delete_virtualchassis', 'dcim.view_virtualchassis')
 
@@ -557,8 +578,8 @@ class DeleteAtomicityTestCase(DeleteAtomicityMixin, TransactionTestCase):
     """
     Exercise the autocommit path, which TestCase cannot reach: it wraps every test in a
     transaction, so a write which escapes the view's rollback is rolled back with the test and
-    stays invisible to the assertions. Outside a transaction, a write performed by delete() before
-    super().delete() genuinely commits unless the view opens its own atomic block. (Ref: #22934)
+    stays invisible to the assertions. Outside a transaction, a write performed earlier in the
+    deletion genuinely commits unless the deletion runs in an atomic block. (Ref: #22934)
 
     Note: TransactionTestCase teardown flushes all tables, which removes rows seeded by data
     migrations from a --keepdb database (e.g. the dcim.0206 ModuleTypeProfiles). A fresh test
@@ -571,12 +592,11 @@ class DeleteAtomicityTestCase(DeleteAtomicityMixin, TransactionTestCase):
         self.client = Client()
         self.client.force_login(self.user)
 
-    @override_settings(PROTECTION_RULES=DeleteAtomicityMixin.PROTECTION_RULES)
     def test_object_delete_view_rollback_is_atomic(self):
         virtual_chassis, device = self.create_virtual_chassis()
 
         self.delete_virtual_chassis(virtual_chassis)
 
-        # Without the view's atomic block, the member device's cleared position and priority commit
-        # here, leaving a change which dispatches no events
+        # Without the view's atomic block, the member device's cleared vc_priority commits here,
+        # leaving a change which dispatches no events
         self.assertDeleteRolledBack(virtual_chassis, device)