Fixes #5913: Improve change logging (#5924)

* Initial work on #5913
* Provide per-line diff highlighting
* BulkDeteView should delete objects individually to secure a pre-change snapshot
* Add changelog tests for bulk operations

netbox/extras/api/serializers.py

@@ -338,7 +338,7 @@ class ObjectChangeSerializer(serializers.ModelSerializer):
         model = ObjectChange
         fields = [
             'id', 'url', 'time', 'user', 'user_name', 'request_id', 'action', 'changed_object_type',
-            'changed_object_id', 'changed_object', 'object_data',
+            'changed_object_id', 'changed_object', 'prechange_data', 'postchange_data',
         ]
 
     @swagger_serializer_method(serializer_or_field=serializers.DictField)

netbox/extras/migrations/0055_objectchange_data.py

@@ -0,0 +1,28 @@
+# Generated by Django 3.2b1 on 2021-03-03 20:30
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('extras', '0054_standardize_models'),
+    ]
+
+    operations = [
+        migrations.RenameField(
+            model_name='objectchange',
+            old_name='object_data',
+            new_name='postchange_data',
+        ),
+        migrations.AlterField(
+            model_name='objectchange',
+            name='postchange_data',
+            field=models.JSONField(blank=True, editable=False, null=True),
+        ),
+        migrations.AddField(
+            model_name='objectchange',
+            name='prechange_data',
+            field=models.JSONField(blank=True, editable=False, null=True),
+        ),
+    ]

netbox/extras/models/change_logging.py

@@ -67,15 +67,22 @@ class ObjectChange(BigIDModel):
         max_length=200,
         editable=False
     )
-    object_data = models.JSONField(
-        editable=False
+    prechange_data = models.JSONField(
+        editable=False,
+        blank=True,
+        null=True
+    )
+    postchange_data = models.JSONField(
+        editable=False,
+        blank=True,
+        null=True
     )
 
     objects = RestrictedQuerySet.as_manager()
 
     csv_headers = [
         'time', 'user', 'user_name', 'request_id', 'action', 'changed_object_type', 'changed_object_id',
-        'related_object_type', 'related_object_id', 'object_repr', 'object_data',
+        'related_object_type', 'related_object_id', 'object_repr', 'prechange_data', 'postchange_data',
     ]
 
     class Meta:
@@ -114,7 +121,8 @@ class ObjectChange(BigIDModel):
             self.related_object_type,
             self.related_object_id,
             self.object_repr,
-            self.object_data,
+            self.prechange_data,
+            self.postchange_data,
         )
 
     def get_action_class(self):

netbox/extras/signals.py

@@ -36,6 +36,9 @@ def _handle_changed_object(request, sender, instance, **kwargs):
     # Record an ObjectChange if applicable
     if hasattr(instance, 'to_objectchange'):
         objectchange = instance.to_objectchange(action)
+        # TODO: Move this to to_objectchange()
+        if hasattr(instance, '_prechange_snapshot'):
+            objectchange.prechange_data = instance._prechange_snapshot
         objectchange.user = request.user
         objectchange.request_id = request.id
         objectchange.save()
@@ -62,6 +65,9 @@ def _handle_deleted_object(request, sender, instance, **kwargs):
     # Record an ObjectChange if applicable
     if hasattr(instance, 'to_objectchange'):
         objectchange = instance.to_objectchange(ObjectChangeActionChoices.ACTION_DELETE)
+        # TODO: Move this to to_objectchange()
+        if hasattr(instance, '_prechange_snapshot'):
+            objectchange.prechange_data = instance._prechange_snapshot
         objectchange.user = request.user
         objectchange.request_id = request.id
         objectchange.save()

netbox/extras/tests/test_changelog.py

@@ -40,8 +40,8 @@ class ChangeLogViewTest(ModelViewTestCase):
     def test_create_object(self):
         tags = self.create_tags('Tag 1', 'Tag 2')
         form_data = {
-            'name': 'Test Site 1',
-            'slug': 'test-site-1',
+            'name': 'Site 1',
+            'slug': 'site-1',
             'status': SiteStatusChoices.STATUS_ACTIVE,
             'cf_my_field': 'ABC',
             'cf_my_field_select': 'Bar',
@@ -56,7 +56,7 @@ class ChangeLogViewTest(ModelViewTestCase):
         response = self.client.post(**request)
         self.assertHttpStatus(response, 302)
 
-        site = Site.objects.get(name='Test Site 1')
+        site = Site.objects.get(name='Site 1')
         # First OC is the creation; second is the tags update
         oc_list = ObjectChange.objects.filter(
             changed_object_type=ContentType.objects.get_for_model(Site),
@@ -64,20 +64,21 @@ class ChangeLogViewTest(ModelViewTestCase):
         ).order_by('pk')
         self.assertEqual(oc_list[0].changed_object, site)
         self.assertEqual(oc_list[0].action, ObjectChangeActionChoices.ACTION_CREATE)
-        self.assertEqual(oc_list[0].object_data['custom_fields']['my_field'], form_data['cf_my_field'])
-        self.assertEqual(oc_list[0].object_data['custom_fields']['my_field_select'], form_data['cf_my_field_select'])
+        self.assertEqual(oc_list[0].prechange_data, None)
+        self.assertEqual(oc_list[0].postchange_data['custom_fields']['my_field'], form_data['cf_my_field'])
+        self.assertEqual(oc_list[0].postchange_data['custom_fields']['my_field_select'], form_data['cf_my_field_select'])
         self.assertEqual(oc_list[1].action, ObjectChangeActionChoices.ACTION_UPDATE)
-        self.assertEqual(oc_list[1].object_data['tags'], ['Tag 1', 'Tag 2'])
+        self.assertEqual(oc_list[1].postchange_data['tags'], ['Tag 1', 'Tag 2'])
 
     def test_update_object(self):
-        site = Site(name='Test Site 1', slug='test-site-1')
+        site = Site(name='Site 1', slug='site-1')
         site.save()
         tags = self.create_tags('Tag 1', 'Tag 2', 'Tag 3')
         site.tags.set('Tag 1', 'Tag 2')
 
         form_data = {
-            'name': 'Test Site X',
-            'slug': 'test-site-x',
+            'name': 'Site X',
+            'slug': 'site-x',
             'status': SiteStatusChoices.STATUS_PLANNED,
             'cf_my_field': 'DEF',
             'cf_my_field_select': 'Foo',
@@ -100,14 +101,16 @@ class ChangeLogViewTest(ModelViewTestCase):
         ).first()
         self.assertEqual(oc.changed_object, site)
         self.assertEqual(oc.action, ObjectChangeActionChoices.ACTION_UPDATE)
-        self.assertEqual(oc.object_data['custom_fields']['my_field'], form_data['cf_my_field'])
-        self.assertEqual(oc.object_data['custom_fields']['my_field_select'], form_data['cf_my_field_select'])
-        self.assertEqual(oc.object_data['tags'], ['Tag 3'])
+        self.assertEqual(oc.prechange_data['name'], 'Site 1')
+        self.assertEqual(oc.prechange_data['tags'], ['Tag 1', 'Tag 2'])
+        self.assertEqual(oc.postchange_data['custom_fields']['my_field'], form_data['cf_my_field'])
+        self.assertEqual(oc.postchange_data['custom_fields']['my_field_select'], form_data['cf_my_field_select'])
+        self.assertEqual(oc.postchange_data['tags'], ['Tag 3'])
 
     def test_delete_object(self):
         site = Site(
-            name='Test Site 1',
-            slug='test-site-1',
+            name='Site 1',
+            slug='site-1',
             custom_field_data={
                 'my_field': 'ABC',
                 'my_field_select': 'Bar'
@@ -129,15 +132,83 @@ class ChangeLogViewTest(ModelViewTestCase):
         self.assertEqual(oc.changed_object, None)
         self.assertEqual(oc.object_repr, site.name)
         self.assertEqual(oc.action, ObjectChangeActionChoices.ACTION_DELETE)
-        self.assertEqual(oc.object_data['custom_fields']['my_field'], 'ABC')
-        self.assertEqual(oc.object_data['custom_fields']['my_field_select'], 'Bar')
-        self.assertEqual(oc.object_data['tags'], ['Tag 1', 'Tag 2'])
+        self.assertEqual(oc.prechange_data['custom_fields']['my_field'], 'ABC')
+        self.assertEqual(oc.prechange_data['custom_fields']['my_field_select'], 'Bar')
+        self.assertEqual(oc.prechange_data['tags'], ['Tag 1', 'Tag 2'])
+        self.assertEqual(oc.postchange_data, None)
+
+    def test_bulk_update_objects(self):
+        sites = (
+            Site(name='Site 1', slug='site-1', status=SiteStatusChoices.STATUS_ACTIVE),
+            Site(name='Site 2', slug='site-2', status=SiteStatusChoices.STATUS_ACTIVE),
+            Site(name='Site 3', slug='site-3', status=SiteStatusChoices.STATUS_ACTIVE),
+        )
+        Site.objects.bulk_create(sites)
+
+        form_data = {
+            'pk': [site.pk for site in sites],
+            '_apply': True,
+            'status': SiteStatusChoices.STATUS_PLANNED,
+            'description': 'New description',
+        }
+
+        request = {
+            'path': self._get_url('bulk_edit'),
+            'data': post_data(form_data),
+        }
+        self.add_permissions('dcim.view_site', 'dcim.change_site')
+        response = self.client.post(**request)
+        self.assertHttpStatus(response, 302)
+
+        objectchange = ObjectChange.objects.get(
+            changed_object_type=ContentType.objects.get_for_model(Site),
+            changed_object_id=sites[0].pk
+        )
+        self.assertEqual(objectchange.changed_object, sites[0])
+        self.assertEqual(objectchange.action, ObjectChangeActionChoices.ACTION_UPDATE)
+        self.assertEqual(objectchange.prechange_data['status'], SiteStatusChoices.STATUS_ACTIVE)
+        self.assertEqual(objectchange.prechange_data['description'], '')
+        self.assertEqual(objectchange.postchange_data['status'], form_data['status'])
+        self.assertEqual(objectchange.postchange_data['description'], form_data['description'])
+
+    def test_bulk_delete_objects(self):
+        sites = (
+            Site(name='Site 1', slug='site-1', status=SiteStatusChoices.STATUS_ACTIVE),
+            Site(name='Site 2', slug='site-2', status=SiteStatusChoices.STATUS_ACTIVE),
+            Site(name='Site 3', slug='site-3', status=SiteStatusChoices.STATUS_ACTIVE),
+        )
+        Site.objects.bulk_create(sites)
+
+        form_data = {
+            'pk': [site.pk for site in sites],
+            'confirm': True,
+            '_confirm': True,
+        }
+
+        request = {
+            'path': self._get_url('bulk_delete'),
+            'data': post_data(form_data),
+        }
+        self.add_permissions('dcim.delete_site')
+        response = self.client.post(**request)
+        self.assertHttpStatus(response, 302)
+
+        objectchange = ObjectChange.objects.get(
+            changed_object_type=ContentType.objects.get_for_model(Site),
+            changed_object_id=sites[0].pk
+        )
+        self.assertEqual(objectchange.changed_object_type, ContentType.objects.get_for_model(Site))
+        self.assertEqual(objectchange.changed_object_id, sites[0].pk)
+        self.assertEqual(objectchange.action, ObjectChangeActionChoices.ACTION_DELETE)
+        self.assertEqual(objectchange.prechange_data['name'], sites[0].name)
+        self.assertEqual(objectchange.prechange_data['slug'], sites[0].slug)
+        self.assertEqual(objectchange.postchange_data, None)
 
 
 class ChangeLogAPITest(APITestCase):
 
-    def setUp(self):
-        super().setUp()
+    @classmethod
+    def setUpTestData(cls):
 
         # Create a custom field on the Site model
         ct = ContentType.objects.get_for_model(Site)
@@ -169,8 +240,8 @@ class ChangeLogAPITest(APITestCase):
 
     def test_create_object(self):
         data = {
-            'name': 'Test Site 1',
-            'slug': 'test-site-1',
+            'name': 'Site 1',
+            'slug': 'site-1',
             'custom_fields': {
                 'my_field': 'ABC',
                 'my_field_select': 'Bar',
@@ -195,17 +266,18 @@ class ChangeLogAPITest(APITestCase):
         ).order_by('pk')
         self.assertEqual(oc_list[0].changed_object, site)
         self.assertEqual(oc_list[0].action, ObjectChangeActionChoices.ACTION_CREATE)
-        self.assertEqual(oc_list[0].object_data['custom_fields'], data['custom_fields'])
+        self.assertEqual(oc_list[0].prechange_data, None)
+        self.assertEqual(oc_list[0].postchange_data['custom_fields'], data['custom_fields'])
         self.assertEqual(oc_list[1].action, ObjectChangeActionChoices.ACTION_UPDATE)
-        self.assertEqual(oc_list[1].object_data['tags'], ['Tag 1', 'Tag 2'])
+        self.assertEqual(oc_list[1].postchange_data['tags'], ['Tag 1', 'Tag 2'])
 
     def test_update_object(self):
-        site = Site(name='Test Site 1', slug='test-site-1')
+        site = Site(name='Site 1', slug='site-1')
         site.save()
 
         data = {
-            'name': 'Test Site X',
-            'slug': 'test-site-x',
+            'name': 'Site X',
+            'slug': 'site-x',
             'custom_fields': {
                 'my_field': 'DEF',
                 'my_field_select': 'Foo',
@@ -229,13 +301,13 @@ class ChangeLogAPITest(APITestCase):
         ).first()
         self.assertEqual(oc.changed_object, site)
         self.assertEqual(oc.action, ObjectChangeActionChoices.ACTION_UPDATE)
-        self.assertEqual(oc.object_data['custom_fields'], data['custom_fields'])
-        self.assertEqual(oc.object_data['tags'], ['Tag 3'])
+        self.assertEqual(oc.postchange_data['custom_fields'], data['custom_fields'])
+        self.assertEqual(oc.postchange_data['tags'], ['Tag 3'])
 
     def test_delete_object(self):
         site = Site(
-            name='Test Site 1',
-            slug='test-site-1',
+            name='Site 1',
+            slug='site-1',
             custom_field_data={
                 'my_field': 'ABC',
                 'my_field_select': 'Bar'
@@ -255,6 +327,123 @@ class ChangeLogAPITest(APITestCase):
         self.assertEqual(oc.changed_object, None)
         self.assertEqual(oc.object_repr, site.name)
         self.assertEqual(oc.action, ObjectChangeActionChoices.ACTION_DELETE)
-        self.assertEqual(oc.object_data['custom_fields']['my_field'], 'ABC')
-        self.assertEqual(oc.object_data['custom_fields']['my_field_select'], 'Bar')
-        self.assertEqual(oc.object_data['tags'], ['Tag 1', 'Tag 2'])
+        self.assertEqual(oc.prechange_data['custom_fields']['my_field'], 'ABC')
+        self.assertEqual(oc.prechange_data['custom_fields']['my_field_select'], 'Bar')
+        self.assertEqual(oc.prechange_data['tags'], ['Tag 1', 'Tag 2'])
+        self.assertEqual(oc.postchange_data, None)
+
+    def test_bulk_create_objects(self):
+        data = (
+            {
+                'name': 'Site 1',
+                'slug': 'site-1',
+            },
+            {
+                'name': 'Site 2',
+                'slug': 'site-2',
+            },
+            {
+                'name': 'Site 3',
+                'slug': 'site-3',
+            },
+        )
+        self.assertEqual(ObjectChange.objects.count(), 0)
+        url = reverse('dcim-api:site-list')
+        self.add_permissions('dcim.add_site')
+
+        response = self.client.post(url, data, format='json', **self.header)
+        self.assertHttpStatus(response, status.HTTP_201_CREATED)
+        self.assertEqual(ObjectChange.objects.count(), 3)
+
+        site1 = Site.objects.get(pk=response.data[0]['id'])
+        objectchange = ObjectChange.objects.get(
+            changed_object_type=ContentType.objects.get_for_model(Site),
+            changed_object_id=site1.pk
+        )
+        self.assertEqual(objectchange.changed_object, site1)
+        self.assertEqual(objectchange.action, ObjectChangeActionChoices.ACTION_CREATE)
+        self.assertEqual(objectchange.prechange_data, None)
+        self.assertEqual(objectchange.postchange_data['name'], data[0]['name'])
+        self.assertEqual(objectchange.postchange_data['slug'], data[0]['slug'])
+
+    def test_bulk_edit_objects(self):
+        sites = (
+            Site(name='Site 1', slug='site-1'),
+            Site(name='Site 2', slug='site-2'),
+            Site(name='Site 3', slug='site-3'),
+        )
+        Site.objects.bulk_create(sites)
+
+        data = (
+            {
+                'id': sites[0].pk,
+                'name': 'Site A',
+                'slug': 'site-A',
+            },
+            {
+                'id': sites[1].pk,
+                'name': 'Site B',
+                'slug': 'site-b',
+            },
+            {
+                'id': sites[2].pk,
+                'name': 'Site C',
+                'slug': 'site-c',
+            },
+        )
+        self.assertEqual(ObjectChange.objects.count(), 0)
+        url = reverse('dcim-api:site-list')
+        self.add_permissions('dcim.change_site')
+
+        response = self.client.patch(url, data, format='json', **self.header)
+        self.assertHttpStatus(response, status.HTTP_200_OK)
+        self.assertEqual(ObjectChange.objects.count(), 3)
+
+        objectchange = ObjectChange.objects.get(
+            changed_object_type=ContentType.objects.get_for_model(Site),
+            changed_object_id=sites[0].pk
+        )
+        self.assertEqual(objectchange.changed_object, sites[0])
+        self.assertEqual(objectchange.action, ObjectChangeActionChoices.ACTION_UPDATE)
+        self.assertEqual(objectchange.prechange_data['name'], 'Site 1')
+        self.assertEqual(objectchange.prechange_data['slug'], 'site-1')
+        self.assertEqual(objectchange.postchange_data['name'], data[0]['name'])
+        self.assertEqual(objectchange.postchange_data['slug'], data[0]['slug'])
+
+    def test_bulk_delete_objects(self):
+        sites = (
+            Site(name='Site 1', slug='site-1'),
+            Site(name='Site 2', slug='site-2'),
+            Site(name='Site 3', slug='site-3'),
+        )
+        Site.objects.bulk_create(sites)
+
+        data = (
+            {
+                'id': sites[0].pk,
+            },
+            {
+                'id': sites[1].pk,
+            },
+            {
+                'id': sites[2].pk,
+            },
+        )
+        self.assertEqual(ObjectChange.objects.count(), 0)
+        url = reverse('dcim-api:site-list')
+        self.add_permissions('dcim.delete_site')
+
+        response = self.client.delete(url, data, format='json', **self.header)
+        self.assertHttpStatus(response, status.HTTP_204_NO_CONTENT)
+        self.assertEqual(ObjectChange.objects.count(), 3)
+
+        objectchange = ObjectChange.objects.get(
+            changed_object_type=ContentType.objects.get_for_model(Site),
+            changed_object_id=sites[0].pk
+        )
+        self.assertEqual(objectchange.changed_object_type, ContentType.objects.get_for_model(Site))
+        self.assertEqual(objectchange.changed_object_id, sites[0].pk)
+        self.assertEqual(objectchange.action, ObjectChangeActionChoices.ACTION_DELETE)
+        self.assertEqual(objectchange.prechange_data['name'], 'Site 1')
+        self.assertEqual(objectchange.prechange_data['slug'], 'site-1')
+        self.assertEqual(objectchange.postchange_data, None)

netbox/extras/tests/test_filters.py

@@ -327,7 +327,7 @@ class ObjectChangeTestCase(TestCase):
                 action=ObjectChangeActionChoices.ACTION_CREATE,
                 changed_object=site,
                 object_repr=str(site),
-                object_data={'name': site.name, 'slug': site.slug}
+                postchange_data={'name': site.name, 'slug': site.slug}
             ),
             ObjectChange(
                 user=users[0],
@@ -336,7 +336,7 @@ class ObjectChangeTestCase(TestCase):
                 action=ObjectChangeActionChoices.ACTION_UPDATE,
                 changed_object=site,
                 object_repr=str(site),
-                object_data={'name': site.name, 'slug': site.slug}
+                postchange_data={'name': site.name, 'slug': site.slug}
             ),
             ObjectChange(
                 user=users[1],
@@ -345,7 +345,7 @@ class ObjectChangeTestCase(TestCase):
                 action=ObjectChangeActionChoices.ACTION_DELETE,
                 changed_object=site,
                 object_repr=str(site),
-                object_data={'name': site.name, 'slug': site.slug}
+                postchange_data={'name': site.name, 'slug': site.slug}
             ),
             ObjectChange(
                 user=users[1],
@@ -354,7 +354,7 @@ class ObjectChangeTestCase(TestCase):
                 action=ObjectChangeActionChoices.ACTION_CREATE,
                 changed_object=ipaddress,
                 object_repr=str(ipaddress),
-                object_data={'address': ipaddress.address, 'status': ipaddress.status}
+                postchange_data={'address': ipaddress.address, 'status': ipaddress.status}
             ),
             ObjectChange(
                 user=users[2],
@@ -363,7 +363,7 @@ class ObjectChangeTestCase(TestCase):
                 action=ObjectChangeActionChoices.ACTION_UPDATE,
                 changed_object=ipaddress,
                 object_repr=str(ipaddress),
-                object_data={'address': ipaddress.address, 'status': ipaddress.status}
+                postchange_data={'address': ipaddress.address, 'status': ipaddress.status}
             ),
             ObjectChange(
                 user=users[2],
@@ -372,7 +372,7 @@ class ObjectChangeTestCase(TestCase):
                 action=ObjectChangeActionChoices.ACTION_DELETE,
                 changed_object=ipaddress,
                 object_repr=str(ipaddress),
-                object_data={'address': ipaddress.address, 'status': ipaddress.status}
+                postchange_data={'address': ipaddress.address, 'status': ipaddress.status}
             ),
         )
         ObjectChange.objects.bulk_create(object_changes)

netbox/extras/views.py

@@ -178,16 +178,18 @@ class ObjectChangeView(generic.ObjectView):
         next_change = objectchanges.filter(time__gt=instance.time).order_by('time').first()
         prev_change = objectchanges.filter(time__lt=instance.time).order_by('-time').first()
 
-        if prev_change:
+        if instance.prechange_data and instance.postchange_data:
             diff_added = shallow_compare_dict(
-                prev_change.object_data,
-                instance.object_data,
+                instance.prechange_data or dict(),
+                instance.postchange_data or dict(),
                 exclude=['last_updated'],
             )
-            diff_removed = {x: prev_change.object_data.get(x) for x in diff_added}
+            diff_removed = {
+                x: instance.prechange_data.get(x) for x in diff_added
+            } if instance.prechange_data else {}
         else:
-            # No previous change; this is the initial change that added the object
-            diff_added = diff_removed = instance.object_data
+            diff_added = None
+            diff_removed = None
 
         return {
             'diff_added': diff_added,