Log project name and settings changes (#5193)

kobo/apps/audit_log/audit_actions.py

@@ -13,3 +13,5 @@ class AuditAction(models.TextChoices):
     ARCHIVE = 'archive'
     UNARCHIVE = 'unarchive'
     REDEPLOY = 'redeploy'
+    UPDATE_NAME = 'update-name'
+    UPDATE_SETTINGS = 'update-settings'

kobo/apps/audit_log/base_views.py

@@ -5,16 +5,18 @@ def get_nested_field(obj, field: str):
     """
     Retrieve a period-separated nested field from an object or dict
 
-    Raises an exception if the field is not found
+    Returns None if the field is not found
     """
     split = field.split('.')
-    attribute = getattr(obj, split[0])
-    if len(split) > 1:
-        for inner_field in split[1:]:
-            if isinstance(attribute, dict):
-                attribute = attribute.get(inner_field)
-            else:
-                attribute = getattr(attribute, inner_field)
+    attribute = obj
+
+    for inner_field in split:
+        if attribute is None:
+            break
+        if isinstance(attribute, dict):
+            attribute = attribute.get(inner_field, None)
+        else:
+            attribute = getattr(attribute, inner_field, None)
     return attribute
 
 

kobo/apps/audit_log/models.py

@@ -300,6 +300,8 @@ class Meta:
     def create_from_request(cls, request):
         if request.resolver_match.url_name == 'asset-deployment':
             cls.create_from_deployment_request(request)
+        elif request.resolver_match.url_name == 'asset-detail':
+            cls.create_from_detail_request(request)
 
     @staticmethod
     def create_from_deployment_request(request):
@@ -354,3 +356,74 @@ def create_from_deployment_request(request):
             action=action,
             metadata=metadata,
         )
+
+    @classmethod
+    def create_from_detail_request(cls, request):
+        initial_data = getattr(request, 'initial_data', None)
+        updated_data = getattr(request, 'updated_data', None)
+
+        if initial_data is None or updated_data is None:
+            # Something went wrong with the request, don't try to create a log
+            return
+
+        asset_uid = request.resolver_match.kwargs['uid']
+        object_id = initial_data['id']
+
+        common_metadata = {
+            'asset_uid': asset_uid,
+            'log_subtype': PROJECT_HISTORY_LOG_PROJECT_SUBTYPE,
+            'ip_address': get_client_ip(request),
+            'source': get_human_readable_client_user_agent(request),
+        }
+
+        # always store the latest version uid
+        common_metadata.update(
+            {'latest_version_uid': updated_data['latest_version.uid']}
+        )
+
+        changed_field_to_action_map = {
+            'name': cls.name_change,
+            'settings': cls.settings_change,
+        }
+
+        for field, method in changed_field_to_action_map.items():
+            old_field = initial_data[field]
+            new_field = updated_data[field]
+            if old_field != new_field:
+                action, additional_metadata = method(old_field, new_field)
+                full_metadata = {**common_metadata, **additional_metadata}
+                ProjectHistoryLog.objects.create(
+                    user=request.user,
+                    object_id=object_id,
+                    action=action,
+                    metadata=full_metadata,
+                )
+
+    # additional metadata should generally follow the pattern
+    # 'field': {'old': old_value, 'new': new_value } or
+    # 'field': {'added': [], 'removed'}
+
+    @staticmethod
+    def name_change(old_field, new_field):
+        metadata = {'name': {'old': old_field, 'new': new_field}}
+        return AuditAction.UPDATE_NAME, metadata
+
+    @staticmethod
+    def settings_change(old_field, new_field):
+        settings = {}
+        all_settings = {**old_field, **new_field}.keys()
+        for setting_name in all_settings:
+            old = old_field.get(setting_name, None)
+            new = new_field.get(setting_name, None)
+            if old != new:
+                metadata_field_subdict = {}
+                if isinstance(old, list) and isinstance(new, list):
+                    removed_values = [val for val in old if val not in new]
+                    added_values = [val for val in new if val not in old]
+                    metadata_field_subdict['added'] = added_values
+                    metadata_field_subdict['removed'] = removed_values
+                else:
+                    metadata_field_subdict['old'] = old
+                    metadata_field_subdict['new'] = new
+                settings[setting_name] = metadata_field_subdict
+        return AuditAction.UPDATE_SETTINGS, {'settings': settings}

kobo/apps/audit_log/tests/test_project_history_logs.py

@@ -1,3 +1,5 @@
+import copy
+
 from django.test import override_settings
 from django.urls import reverse
 
@@ -6,6 +8,7 @@
 from kobo.apps.audit_log.tests.test_models import BaseAuditLogTestCase
 from kobo.apps.kobo_auth.shortcuts import User
 from kpi.models import Asset
+from kpi.models.asset import AssetSetting
 
 
 @override_settings(DEFAULT_DEPLOYMENT_BACKEND='mock')
@@ -34,9 +37,7 @@ def _check_common_metadata(self, metadata_dict):
             metadata_dict['latest_version_uid'], self.asset.latest_version.uid
         )
 
-    def _base_endpoint_test(
-        self, patch, url_name, request_data, expected_action, verify_additional_metadata
-    ):
+    def _base_endpoint_test(self, patch, url_name, request_data, expected_action):
         # requests are either patches or posts
         request_method = self.client.patch if patch else self.client.post
         # hit the endpoint with the correct data
@@ -45,6 +46,8 @@ def _base_endpoint_test(
             data=request_data,
             format='json',
         )
+        self.asset.refresh_from_db()
+
         # make sure a log was created
         logs = ProjectHistoryLog.objects.filter(metadata__asset_uid=self.asset.uid)
         self.assertEqual(logs.count(), 1)
@@ -53,26 +56,23 @@ def _base_endpoint_test(
         self._check_common_metadata(log.metadata)
         self.assertEqual(log.object_id, self.asset.id)
         self.assertEqual(log.action, expected_action)
-        verify_additional_metadata(log.metadata)
+        return log.metadata
 
     def test_first_time_deployment_creates_log(self):
         post_data = {
             'active': True,
             'backend': 'mock',
         }
-
-        def verify_metadata(log_metadata):
-            self.assertEqual(
-                log_metadata['latest_deployed_version_uid'],
-                self.asset.latest_version.uid,
-            )
-
-        self._base_endpoint_test(
+        log_metadata = self._base_endpoint_test(
             patch=False,
             url_name=self.deployment_url,
             request_data=post_data,
             expected_action=AuditAction.DEPLOY,
-            verify_additional_metadata=verify_metadata,
+        )
+
+        self.assertEqual(
+            log_metadata['latest_deployed_version_uid'],
+            self.asset.latest_version.uid,
         )
 
     def test_redeployment_creates_log(self):
@@ -82,19 +82,16 @@ def test_redeployment_creates_log(self):
             'active': True,
             'backend': 'mock',
         }
-
-        def verify_metadata(log_metadata):
-            self.assertEqual(
-                log_metadata['latest_deployed_version_uid'],
-                self.asset.latest_version.uid,
-            )
-
-        self._base_endpoint_test(
+        log_metadata = self._base_endpoint_test(
             patch=True,
             url_name=self.deployment_url,
             request_data=request_data,
             expected_action=AuditAction.REDEPLOY,
-            verify_additional_metadata=verify_metadata,
+        )
+
+        self.assertEqual(
+            log_metadata['latest_deployed_version_uid'],
+            self.asset.latest_version.uid,
         )
 
     def test_archive_creates_log(self):
@@ -108,19 +105,18 @@ def test_archive_creates_log(self):
             url_name=self.deployment_url,
             request_data=request_data,
             expected_action=AuditAction.ARCHIVE,
-            verify_additional_metadata=lambda x: None,
         )
         # do it again (archive an already-archived asset)
         self.client.patch(
             reverse('api_v2:asset-deployment', kwargs={'uid': self.asset.uid}),
             data=request_data,
             format='json',
         )
-        unarchived_logs = ProjectHistoryLog.objects.filter(
+        archived_logs = ProjectHistoryLog.objects.filter(
             object_id=self.asset.id, action=AuditAction.ARCHIVE
         )
         # we should log the attempt even if it didn't technically do anything
-        self.assertEqual(unarchived_logs.count(), 2)
+        self.assertEqual(archived_logs.count(), 2)
 
     def test_unarchive_creates_log(self):
         # can only unarchive deployed asset
@@ -133,7 +129,6 @@ def test_unarchive_creates_log(self):
             url_name=self.deployment_url,
             request_data=request_data,
             expected_action=AuditAction.UNARCHIVE,
-            verify_additional_metadata=lambda x: None,
         )
         # do it again (unarchive an already-unarchived asset)
         self.client.patch(
@@ -167,3 +162,144 @@ def test_failed_requests_does_not_create_log(self):
         )
         # no logs should be created
         self.assertEqual(ProjectHistoryLog.objects.count(), 0)
+
+    def test_change_project_name_creates_log(self):
+        old_name = self.asset.name
+
+        log_metadata = self._base_endpoint_test(
+            patch=True,
+            url_name=self.detail_url,
+            request_data={'name': 'new_name'},
+            expected_action=AuditAction.UPDATE_NAME,
+        )
+
+        self.assertEqual(log_metadata['name']['new'], 'new_name')
+        self.assertEqual(log_metadata['name']['old'], old_name)
+
+    def test_change_standard_project_settings_creates_log(self):
+        old_settings = copy.deepcopy(self.asset.settings)
+        # both country and description are in Asset.STANDARDIZED_SETTINGS
+        patch_data = {
+            'settings': {
+                'country': [{'label': 'Albania', 'value': 'ALB'}],
+                'description': 'New description',
+            }
+        }
+
+        log_metadata = self._base_endpoint_test(
+            patch=True,
+            url_name=self.detail_url,
+            request_data=patch_data,
+            expected_action=AuditAction.UPDATE_SETTINGS,
+        )
+
+        # check non-list settings just store old and new information
+        settings_dict = log_metadata['settings']
+        self.assertEqual(
+            settings_dict['description']['old'], old_settings['description']
+        )
+        self.assertEqual(settings_dict['description']['new'], 'New description')
+        # check list settings store added and removed fields
+        self.assertListEqual(
+            settings_dict['country']['added'],
+            [{'label': 'Albania', 'value': 'ALB'}],
+        )
+        self.assertListEqual(
+            settings_dict['country']['removed'],
+            [{'label': 'United States', 'value': 'USA'}],
+        )
+        # check default settings not recorded if not included in request
+        for setting in Asset.STANDARDIZED_SETTINGS:
+            # country codes are updated automatically when country is updated
+            if setting not in ['country', 'settings', 'country_codes']:
+                self.assertNotIn(setting, log_metadata)
+
+    def test_unchanged_settings_not_recorded_on_log(self):
+        """
+        Check settings not included on log if in the request but did not change
+        """
+        patch_data = {
+            'settings': {
+                'sector': self.asset.settings['sector'],
+                'country': self.asset.settings['country'],
+                # only change description
+                'description': 'New description',
+            }
+        }
+        log_metadata = self._base_endpoint_test(
+            patch=True,
+            url_name=self.detail_url,
+            request_data=patch_data,
+            expected_action=AuditAction.UPDATE_SETTINGS,
+        )
+
+        self.assertNotIn('sector', log_metadata)
+        self.assertNotIn('country', log_metadata)
+        self.assertNotIn('operational_purpose', log_metadata)
+        self.assertNotIn('collects_pii', log_metadata)
+
+    def test_no_log_if_settings_unchanged(self):
+        # fill request with only existing values
+        patch_data = {
+            'settings': {
+                'sector': self.asset.settings['sector'],
+                'country': self.asset.settings['country'],
+                'description': self.asset.settings['description'],
+            }
+        }
+
+        self.client.patch(
+            reverse('api_v2:asset-detail', kwargs={'uid': self.asset.uid}),
+            data=patch_data,
+            format='json',
+        )
+
+        self.assertEqual(ProjectHistoryLog.objects.count(), 0)
+
+    def test_nullify_settings_creates_log(self):
+        old_settings = copy.deepcopy(self.asset.settings)
+
+        log_metadata = self._base_endpoint_test(
+            patch=True,
+            url_name=self.detail_url,
+            request_data={'settings': {}},
+            expected_action=AuditAction.UPDATE_SETTINGS,
+        )
+
+        for setting, old_value in old_settings.items():
+            if setting in Asset.STANDARDIZED_SETTINGS:
+                # if the setting is one of the standard ones, the new value after
+                # nulling it out will be whatever is configured as the default
+                setting_configs: AssetSetting = Asset.STANDARDIZED_SETTINGS[setting]
+                new_value = (
+                    setting_configs.default_val(self.asset)
+                    if callable(setting_configs.default_val)
+                    else setting_configs.default_val
+                )
+            else:
+                new_value = None
+
+            if isinstance(new_value, list) and isinstance(old_value, list):
+                removed_values = [val for val in old_value if val not in new_value]
+                added_values = [val for val in new_value if val not in old_value]
+                self.assertListEqual(
+                    log_metadata['settings'][setting]['added'], added_values
+                )
+                self.assertListEqual(
+                    log_metadata['settings'][setting]['removed'], removed_values
+                )
+            else:
+                self.assertEqual(log_metadata['settings'][setting]['new'], new_value)
+                self.assertEqual(log_metadata['settings'][setting]['old'], old_value)
+
+    def test_add_new_settings_creates_log(self):
+        log_metadata = self._base_endpoint_test(
+            patch=True,
+            url_name=self.detail_url,
+            # set a setting not in Asset.STANDARDIZED_SETTINGS
+            request_data={'settings': {'new_setting': 'new_value'}},
+            expected_action=AuditAction.UPDATE_SETTINGS,
+        )
+
+        self.assertEqual(log_metadata['settings']['new_setting']['new'], 'new_value')
+        self.assertEqual(log_metadata['settings']['new_setting']['old'], None)

kpi/fixtures/asset_with_settings_and_qa.json

@@ -33,8 +33,8 @@
                     "label":"Education",
                     "value":"Education"
                 },
-                "operational_purpose":"Purpose",
-                "collects_pii":false
+                "country_codes": ["USA"],
+                "organization" : "An organization"
             },
             "data_sharing":{
                 "fields":[