Merge pull request #2123 from uktrade/LTD-5115-nt-casenote-change

LTD-5115-nt-casenote-change

api/audit_trail/formatters.py

@@ -228,9 +228,11 @@ def product_reviewed(**payload):
 def licence_status_updated(**payload):
     status = payload["status"].lower()
     licence = payload["licence"]
+    previous_status = payload.get("previous_status")
     if status == LicenceStatus.EXHAUSTED:
         return f"The products for licence {licence} were exported and the status set to '{status}'."
-
+    if previous_status:
+        return f"set the licence status of {licence} from {previous_status} to {status}."
     return f"{status} licence {licence}."
 
 

api/audit_trail/signals.py

@@ -1,13 +1,10 @@
 import logging
 
-from django.db.models.signals import post_save, pre_save
+from django.db.models.signals import post_save
 from django.dispatch import receiver
 
-from api.audit_trail import service as audit_trail_service
-from api.audit_trail.enums import AuditType
 from api.audit_trail.models import Audit
 from api.audit_trail.serializers import AuditSerializer
-from api.licences.models import Licence
 
 
 logger = logging.getLogger(__name__)
@@ -19,21 +16,3 @@ def emit_audit_log(sender, instance, **kwargs):
     text = str(instance)
     extra = AuditSerializer(instance).data
     logger.info(text, extra={"audit": extra})
-
-
-@receiver(pre_save, sender=Licence)
-def audit_licence_status_change(sender, instance, **kwargs):
-    """Audit a licence status change."""
-    try:
-        Licence.objects.get(id=instance.id, status=instance.status)
-    except Licence.DoesNotExist:
-        # The `pre_save` signal is called *before* the save() method is run and
-        # the `instance` is for the object that is about to be saved. In this case,
-        # if a `License` with the given parameters does not exist, it implies
-        # a change in status.
-        audit_trail_service.create_system_user_audit(
-            verb=AuditType.LICENCE_UPDATED_STATUS,
-            action_object=instance,
-            target=instance.case.get_case(),
-            payload={"licence": instance.reference_code, "status": instance.status},
-        )

api/audit_trail/tests/test_formatters.py

@@ -231,6 +231,10 @@ def test_remove_party_audit_message(self, payload, expected_result):
             ({"status": "expired", "licence": "1"}, "expired licence 1."),
             ({"status": "draft", "licence": "1"}, "draft licence 1."),
             ({"status": "expired", "licence": "1"}, "expired licence 1."),
+            (
+                {"status": "suspended", "licence": "1", "previous_status": "issued"},
+                "set the licence status of 1 from issued to suspended.",
+            ),
         ]
     )
     def test_licence_status_updated(self, payload, expected_result):

api/core/tests/test_decorators.py

@@ -266,9 +266,9 @@ def a_view(request, *args, **kwargs):
             [LicenceStatus.CANCELLED, CaseStatusEnum.FINALISED, "The licence status is not editable."],
         ]
     )
-    def test_licence_is_editable_failure(self, license_status, case_status, error_msg):
+    def test_licence_is_editable_failure(self, licence_status, case_status, error_msg):
         application = self.create_standard_application_case(self.organisation)
-        licence = StandardLicenceFactory(case=application, status=license_status)
+        licence = StandardLicenceFactory(case=application, status=licence_status)
         application.status = get_case_status_by_status(case_status)
         application.save()
 

api/licences/models.py

@@ -1,5 +1,6 @@
 import uuid
 from datetime import datetime
+from api.audit_trail.enums import AuditType
 import reversion
 
 from django.db import models
@@ -14,6 +15,7 @@
 from api.licences.managers import LicenceManager
 from api.staticdata.decisions.models import Decision
 from api.cases.notify import notify_exporter_licence_suspended, notify_exporter_licence_revoked
+from api.audit_trail import service as audit_trail_service
 
 
 class HMRCIntegrationUsageData(TimestampableModel):
@@ -50,6 +52,14 @@ class Licence(TimestampableModel):
     def __str__(self):
         return self.reference_code
 
+    def _set_status(self, status, user=None, send_status_change_to_hmrc=True):
+        original_status = self.status
+        self.status = status
+        self.save(send_status_change_to_hmrc=send_status_change_to_hmrc)
+        status_changed = original_status != self.status
+        if status_changed:
+            self.create_licence_change_audit_log(original_status=original_status, user=user)
+
     def hmrc_mail_status(self):
         """
         Fetch mail status from HRMC-integration server
@@ -60,31 +70,46 @@ def hmrc_mail_status(self):
             raise ImproperlyConfigured("Did you forget to switch on LITE_HMRC_INTEGRATION_ENABLED?")
         return get_mail_status(self)
 
-    def surrender(self, send_status_change_to_hmrc=True):
-        self.status = LicenceStatus.SURRENDERED
-        self.save(send_status_change_to_hmrc=send_status_change_to_hmrc)
-
-    def suspend(self):
-        self.status = LicenceStatus.SUSPENDED
-        self.save()
+    def create_licence_change_audit_log(self, original_status=None, user=None):
+        """
+        Generates a system audit log for licence changes
+        """
+        audit_kwargs = {
+            "verb": AuditType.LICENCE_UPDATED_STATUS,
+            "action_object": self,
+            "target": self.case.get_case(),
+            "payload": {"licence": self.reference_code, "status": self.status, "previous_status": original_status},
+        }
+        if user:
+            audit_trail_service.create(
+                **audit_kwargs,
+                actor=user,
+            )
+        else:
+            audit_trail_service.create_system_user_audit(**audit_kwargs)
+
+    def surrender(self, user=None):
+        self._set_status(status=LicenceStatus.SURRENDERED, user=user, send_status_change_to_hmrc=True)
+
+    def suspend(self, user=None):
+        self._set_status(LicenceStatus.SUSPENDED, user, send_status_change_to_hmrc=False)
         notify_exporter_licence_suspended(self)
 
-    def revoke(self, send_status_change_to_hmrc=True):
-        self.status = LicenceStatus.REVOKED
-        self.save(send_status_change_to_hmrc=send_status_change_to_hmrc)
+    def revoke(self, user=None):
+        self._set_status(LicenceStatus.REVOKED, user=user, send_status_change_to_hmrc=True)
         notify_exporter_licence_revoked(self)
 
-    def cancel(self, send_status_change_to_hmrc=True):
-        self.status = LicenceStatus.CANCELLED
-        self.save(send_status_change_to_hmrc=send_status_change_to_hmrc)
+    def cancel(self, user=None, send_status_change_to_hmrc=True):
+        self._set_status(
+            status=LicenceStatus.CANCELLED, user=user, send_status_change_to_hmrc=send_status_change_to_hmrc
+        )
 
-    def reinstate(self):
-        # This supersedes the issue method as it's called explicity on the license
+    def reinstate(self, user=None):
+        # This supersedes the issue method as it's called explicity on the licence
         # Hence the user explicty knows which license is being reinstated
-        self.status = LicenceStatus.REINSTATED
-        self.save()
+        self._set_status(status=LicenceStatus.REINSTATED, user=user, send_status_change_to_hmrc=True)
 
-    def issue(self, send_status_change_to_hmrc=True):
+    def issue(self, user=None, send_status_change_to_hmrc=True):
         # re-issue the licence if an older version exists
         status = LicenceStatus.ISSUED
         old_licence = (
@@ -94,8 +119,7 @@ def issue(self, send_status_change_to_hmrc=True):
             old_licence.cancel(send_status_change_to_hmrc=False)
             status = LicenceStatus.REINSTATED
 
-        self.status = status
-        self.save(send_status_change_to_hmrc=send_status_change_to_hmrc)
+        self._set_status(status=status, user=user, send_status_change_to_hmrc=send_status_change_to_hmrc)
 
     def save(self, *args, **kwargs):
         end_datetime = datetime.strptime(
@@ -104,6 +128,11 @@ def save(self, *args, **kwargs):
         self.end_date = end_datetime.date()
 
         send_status_change_to_hmrc = kwargs.pop("send_status_change_to_hmrc", False)
+
+        # We only require a system log if this is a new save since this isn't user initiated
+
+        if not Licence.objects.filter(id=self.id).exists():
+            self.create_licence_change_audit_log()
         super(Licence, self).save(*args, **kwargs)
 
         # Immediately notify HMRC if needed

api/licences/serializers/view_licence.py

@@ -221,9 +221,9 @@ class LicenceDetailsSerializer(serializers.ModelSerializer):
     status = ChoiceField(choices=LicenceStatus.choices)
 
     action_dict = {
-        "reinstated": lambda instance: Licence.reinstate(instance),
-        "suspended": lambda instance: Licence.suspend(instance),
-        "revoked": lambda instance: Licence.revoke(instance, send_status_change_to_hmrc=True),
+        "reinstated": lambda instance, user: Licence.reinstate(instance, user),
+        "suspended": lambda instance, user: Licence.suspend(instance, user),
+        "revoked": lambda instance, user: Licence.revoke(instance, user),
     }
 
     class Meta:
@@ -239,10 +239,12 @@ class Meta:
     def update(self, instance, validated_data):
         update_action = validated_data.get("status")
         try:
+            request = self.context.get("request")
             action_method = self.action_dict[update_action]
-            action_method(instance)
+            action_method(instance, request.user)
         except KeyError:
             raise serializers.ValidationError(f"Updating licence status: {update_action} not allowed")
+
         return super().update(instance, validated_data)
 
     def get_case_status(self, instance):

api/licences/tests/test_api_to_hmrc_integration.py

@@ -638,8 +638,8 @@ def test_reinstate_licence_success(self, request):
 
         url = reverse("cases:finalise", kwargs={"pk": standard_application.id})
         response = self.client.put(url, data={}, **self.gov_headers)
-
         self.assertEqual(response.status_code, status.HTTP_201_CREATED)
+
         request.assert_called_once_with(
             "POST",
             f"{settings.LITE_HMRC_INTEGRATION_URL}{SEND_LICENCE_ENDPOINT}",

api/licences/tests/test_audit_licence.py

@@ -29,7 +29,7 @@ def test_create_new_licence(self):
         self.assertEqual(audit_records[0].payload["licence"], "test_reference")
         self.assertEqual(audit_records[0].payload["status"], "issued")
 
-    def test_update_existing_licence(self):
+    def test_update_existing_licence_no_revoked_audit(self):
         licence = Licence.objects.create(
             case=self.standard_application,
             status=LicenceStatus.ISSUED,
@@ -39,14 +39,13 @@ def test_update_existing_licence(self):
         )
         licence.status = LicenceStatus.REVOKED
         licence.save()
-
+        # Just one audit created since revoke should be called directly.
         audit_records = Audit.objects.filter(verb=AuditType.LICENCE_UPDATED_STATUS).order_by("created_at")
 
-        self.assertEqual(audit_records.count(), 2)
+        self.assertEqual(audit_records.count(), 1)
         self.assertEqual(audit_records[0].payload["status"], "issued")
-        self.assertEqual(audit_records[1].payload["status"], "revoked")
 
-    def test_update_existing_licence_no_change(self):
+    def test_update_existing_licence_no_change_no_audit(self):
         licence = Licence(
             case=self.standard_application,
             status=LicenceStatus.ISSUED,

api/licences/tests/test_get_licences.py

@@ -1,6 +1,9 @@
 from unittest import mock
 from django.urls import reverse
 from rest_framework import status
+from api.audit_trail.enums import AuditType
+from api.audit_trail.models import Audit
+from api.audit_trail.serializers import AuditSerializer
 from api.core.constants import Roles
 from api.staticdata.statuses.libraries.get_case_status import get_case_status_by_status
 from api.teams.enums import TeamIdEnum
@@ -214,7 +217,7 @@ def setUp(self):
         self.gov_user.role = lu_role
         self.gov_user.save()
 
-    def test_get_license_details(self):
+    def test_get_licence_details(self):
 
         response = self.client.get(self.url, **self.gov_headers)
         response_data = response.json()
@@ -229,7 +232,7 @@ def test_get_license_details(self):
         }
         assert response_data == expected_data
 
-    def test_get_license_details_exporter_not_allowed(self):
+    def test_get_licence_details_exporter_not_allowed(self):
 
         response = self.client.get(self.url, **self.exporter_headers)
         assert response.status_code == status.HTTP_403_FORBIDDEN
@@ -240,7 +243,7 @@ def test_get_license_details_exporter_not_allowed(self):
             [{"status": "reinstated"}, "reinstate"],
         ]
     )
-    def test_update_license_details_message_success(self, data, expect_model_method):
+    def test_update_licence_details_message_success(self, data, expect_model_method):
 
         with mock.patch(f"api.licences.models.Licence.{expect_model_method}") as save_method_mock:
 
@@ -255,11 +258,11 @@ def test_update_license_details_message_success(self, data, expect_model_method)
                 **data,
             }
             save_method_mock.assert_called_once()
-            save_method_mock.assert_called_once_with(self.standard_application_licence)
+            save_method_mock.assert_called_once_with(self.standard_application_licence, self.gov_user.baseuser_ptr)
             assert response.status_code == status.HTTP_200_OK
             assert response_data == expected_data
 
-    def test_update_license_details_revoked_success_send_hmrc(self):
+    def test_update_licence_details_revoked_success_send_hmrc(self):
         data = {"status": "revoked"}
         with mock.patch("api.licences.models.Licence.revoke") as save_method_mock:
             response = self.client.patch(self.url, data, **self.gov_headers)
@@ -272,7 +275,7 @@ def test_update_license_details_revoked_success_send_hmrc(self):
                 **data,
             }
             save_method_mock.assert_called_once()
-            save_method_mock.assert_called_once_with(self.standard_application_licence, send_status_change_to_hmrc=True)
+            save_method_mock.assert_called_once_with(self.standard_application_licence, self.gov_user.baseuser_ptr)
             assert response.status_code == status.HTTP_200_OK
             assert response_data == expected_data
 
@@ -283,11 +286,11 @@ def test_update_license_details_revoked_success_send_hmrc(self):
             [{"status": "suspended"}],
         ]
     )
-    def test_update_license_details_put_fails(self, data):
+    def test_update_licence_details_put_fails(self, data):
         response = self.client.put(self.url, data, **self.gov_headers)
         assert response.status_code == status.HTTP_403_FORBIDDEN
 
-    def test_update_license_details_invalid_status(self):
+    def test_update_licence_details_invalid_status(self):
         data = {"status": "dummy"}
         response = self.client.patch(self.url, data, **self.gov_headers)
         assert response.json()["errors"] == {"status": ['"dummy" is not a valid choice.']}
@@ -302,12 +305,12 @@ def test_update_license_details_invalid_status(self):
             [{"hmrc_integration_sent_at": True}],
         ]
     )
-    def test_update_license_details_not_updated(self, data):
+    def test_update_licence_details_not_updated(self, data):
 
         response = self.client.patch(self.url, data, **self.gov_headers)
         assert response.status_code == status.HTTP_400_BAD_REQUEST
 
-    def test_update_license_details_non_lu_admin_forbidden(self):
+    def test_update_licence_details_non_lu_admin_forbidden(self):
 
         defult_role, _ = Role.objects.get_or_create(
             id=Roles.INTERNAL_DEFAULT_ROLE_ID, name=Roles.INTERNAL_DEFAULT_ROLE_NAME
@@ -327,7 +330,7 @@ def test_update_license_details_non_lu_admin_forbidden(self):
             [CaseStatusEnum.INITIAL_CHECKS, status.HTTP_403_FORBIDDEN],
         ]
     )
-    def test_update_license_details_case_non_finialised(self, case_status, expected_status):
+    def test_update_licence_details_case_non_finialised(self, case_status, expected_status):
         self.standard_application.status = get_case_status_by_status(case_status)
         self.standard_application.save()
 
@@ -347,14 +350,14 @@ def test_update_license_details_case_non_finialised(self, case_status, expected_
             [LicenceStatus.CANCELLED, status.HTTP_403_FORBIDDEN],
         ]
     )
-    def test_update_license_details_case_licence_editable_states(self, licence_status, expected_status):
+    def test_update_licence_details_case_licence_editable_states(self, licence_status, expected_status):
         self.standard_application_licence.status = licence_status
         self.standard_application_licence.save()
 
         response = self.client.patch(self.url, self.status_data, **self.gov_headers)
         assert response.status_code == expected_status
 
-    def test_update_license_details_check_version(self):
+    def test_update_licence_details_check_version(self):
         data_items = [{"status": "reinstated"}, {"status": "suspended"}, {"status": "revoked"}]
 
         response = self.client.patch(self.url, data_items[0], **self.gov_headers)
@@ -371,3 +374,24 @@ def test_update_license_details_check_version(self):
         for counter, version in enumerate(versions, start=1):
             self.assertEqual(version.revision.user, self.gov_user.baseuser_ptr)
             self.assertEqual(version.field_dict["status"], data_items[3 - counter]["status"])
+
+    @parameterized.expand(
+        [
+            [LicenceStatus.REINSTATED],
+            [LicenceStatus.SUSPENDED],
+            [LicenceStatus.REVOKED],
+        ]
+    )
+    def test_update_licence_details_audit_trail(self, licence_status):
+        data = {"status": licence_status}
+
+        response = self.client.patch(self.url, data, **self.gov_headers)
+
+        assert response.status_code == status.HTTP_200_OK
+        audit = Audit.objects.filter(verb=AuditType.LICENCE_UPDATED_STATUS).first()
+        audit_data = AuditSerializer(audit).data
+        assert audit_data["user"]["id"] == self.gov_user.pk
+        assert (
+            audit_data["text"]
+            == f"set the licence status of {self.standard_application_licence.reference_code} from {self.standard_application_licence.status} to {licence_status}."
+        )