diff --git a/api/audit_trail/formatters.py b/api/audit_trail/formatters.py index 1eebbc9c4a..d084e821e5 100644 --- a/api/audit_trail/formatters.py +++ b/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}." diff --git a/api/audit_trail/signals.py b/api/audit_trail/signals.py index 9ba235a4f4..73734eae3c 100644 --- a/api/audit_trail/signals.py +++ b/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}, - ) diff --git a/api/audit_trail/tests/test_formatters.py b/api/audit_trail/tests/test_formatters.py index 089448604f..77275ab329 100644 --- a/api/audit_trail/tests/test_formatters.py +++ b/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): diff --git a/api/core/tests/test_decorators.py b/api/core/tests/test_decorators.py index a7cfc9e698..dccf6a36d8 100644 --- a/api/core/tests/test_decorators.py +++ b/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() diff --git a/api/licences/models.py b/api/licences/models.py index 0f6b042eeb..748c54a97f 100644 --- a/api/licences/models.py +++ b/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 diff --git a/api/licences/serializers/view_licence.py b/api/licences/serializers/view_licence.py index d7b1791de1..d4964b5612 100644 --- a/api/licences/serializers/view_licence.py +++ b/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): diff --git a/api/licences/tests/test_api_to_hmrc_integration.py b/api/licences/tests/test_api_to_hmrc_integration.py index b4846027cd..ff7676bad6 100644 --- a/api/licences/tests/test_api_to_hmrc_integration.py +++ b/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}", diff --git a/api/licences/tests/test_audit_licence.py b/api/licences/tests/test_audit_licence.py index a97c3e2b72..a3a100dbc3 100644 --- a/api/licences/tests/test_audit_licence.py +++ b/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, diff --git a/api/licences/tests/test_get_licences.py b/api/licences/tests/test_get_licences.py index 8b050559d4..27d6fb4b1b 100644 --- a/api/licences/tests/test_get_licences.py +++ b/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}." + )