From 2cc6b9c132e2a84b6c104b4c7fdb9d9492bb1563 Mon Sep 17 00:00:00 2001 From: Le H Ngo Date: Tue, 16 Apr 2024 13:05:23 +0100 Subject: [PATCH 01/19] Update to Flag model and add method based on it --- api/cases/libraries/finalise.py | 22 +++++ api/cases/tests/test_finalise_advice.py | 81 +++++++++++++++++++ api/cases/views/views.py | 10 ++- .../0006_flag_remove_on_finalised.py | 18 +++++ api/flags/models.py | 1 + 5 files changed, 131 insertions(+), 1 deletion(-) create mode 100644 api/flags/migrations/0006_flag_remove_on_finalised.py diff --git a/api/cases/libraries/finalise.py b/api/cases/libraries/finalise.py index cda61cabfb..58c81826f1 100644 --- a/api/cases/libraries/finalise.py +++ b/api/cases/libraries/finalise.py @@ -1,6 +1,10 @@ +import json + from api.cases.enums import AdviceType, CaseTypeSubTypeEnum, AdviceLevel from api.cases.models import Advice, GoodCountryDecision from api.applications.models import GoodOnApplication +from api.flags.models import Flag +from api.audit_trail.models import Audit def get_required_decision_document_types(case): @@ -40,3 +44,21 @@ def get_required_decision_document_types(case): required_decisions.add(AdviceType.REFUSE) return required_decisions + + +def remove_flags_on_finalisation(case): + case_flags = case.flags.all() + for flag in case_flags: + if flag.remove_on_finalised: + case.flags.remove(flag) + case.save() + + +def remove_flags_from_audit_trail(case): + flags_to_remove = Flag.objects.filter(remove_on_finalised=True) + for flag in flags_to_remove: + audit_logs = Audit.objects.filter(target_object_id=case.id) + for audit_log in audit_logs: + payload_json_string = json.dumps(audit_log.payload) + if flag.name in payload_json_string: + audit_log.delete() diff --git a/api/cases/tests/test_finalise_advice.py b/api/cases/tests/test_finalise_advice.py index 112c6e8c66..fec0da24a7 100644 --- a/api/cases/tests/test_finalise_advice.py +++ b/api/cases/tests/test_finalise_advice.py @@ -1,5 +1,10 @@ from unittest import mock from django.urls import reverse +from api.audit_trail.enums import AuditType +from api.flags.models import Flag +from django.contrib.auth.models import User +from api.users.enums import UserType +from api.users.models import BaseUser from rest_framework import status from api.audit_trail.models import Audit @@ -12,6 +17,8 @@ from api.staticdata.statuses.enums import CaseStatusEnum from api.staticdata.statuses.models import CaseStatus from test_helpers.clients import DataTestClient +from api.audit_trail import service as audit_trail_service +from api.staticdata.statuses.libraries.get_case_status import get_case_status_by_status class RefuseAdviceTests(DataTestClient): @@ -121,6 +128,28 @@ class ApproveAdviceTests(DataTestClient): def setUp(self): super().setUp() self.application = self.create_standard_application_case(self.organisation) + self.original_flag_id = self.application.flags.first().id + + # Add Flag to test with removal after Finalise + self.test_flag = Flag.objects.all().first() + self.test_flag.remove_on_finalised = True + self.test_flag.save() + self.application.flags.add(self.test_flag) + + user = BaseUser(email="test@mail.com", first_name="John", last_name="Smith", type=UserType.SYSTEM) + + case = self.application.get_case() + + self.test_audit = audit_trail_service.create( + actor=user, + verb=AuditType.ADD_FLAGS, + target=case, + payload={ + "added_flags": [self.test_flag.name], + "additional_text": "test", + }, + ) + self.url = reverse("cases:finalise", kwargs={"pk": self.application.id}) FinalAdviceFactory(user=self.gov_user, case=self.application, type=AdviceType.APPROVE) self.template = self.create_letter_template( @@ -151,3 +180,55 @@ def test_approve_standard_application_success( send_exporter_notifications_func.assert_called() assert case.sub_status.name == "Approved" + + @mock.patch("api.cases.views.views.notify_exporter_licence_issued") + @mock.patch("api.cases.generated_documents.models.GeneratedCaseDocument.send_exporter_notifications") + def test_finalised_standard_application_with_flags_removed( + self, + send_exporter_notifications_func, + mock_notify_exporter_licence_issued, + ): + self.gov_user.role.permissions.set([GovPermissions.MANAGE_LICENCE_FINAL_ADVICE.name]) + self.create_generated_case_document(self.application, self.template, advice_type=AdviceType.APPROVE) + + self.assertEqual(self.application.flags.count(), 2) + + response = self.client.put(self.url, data={}, **self.gov_headers) + self.application.refresh_from_db() + + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + self.assertEqual(self.application.status, CaseStatus.objects.get(status=CaseStatusEnum.FINALISED)) + for document in GeneratedCaseDocument.objects.filter(advice_type__isnull=False): + self.assertTrue(document.visible_to_exporter) + + # Make sure Flag was removed and Audit trail was removed + case = get_case(self.application.id) + flag = Flag.objects.filter(id=self.original_flag_id).first() + self.assertEqual(case.flags.count(), 1) + self.assertEqual(case.flags.first(), flag) + + audit_queryset = Audit.objects.filter(target_object_id=case.id) + self.assertNotIn(self.test_audit, audit_queryset) + + def test_standard_application_withdrawn( + self, + ): + url = reverse("applications:manage_status", kwargs={"pk": self.application.id}) + data = {"status": CaseStatusEnum.WITHDRAWN} + + self.assertEqual(self.application.flags.count(), 2) + + response = self.client.put(url, data=data, **self.exporter_headers) + self.application.refresh_from_db() + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(self.application.status, get_case_status_by_status(CaseStatusEnum.WITHDRAWN)) + + # Make sure Flag was removed and Audit trail was removed + case = get_case(self.application.id) + flag = Flag.objects.filter(id=self.original_flag_id).first() + self.assertEqual(case.flags.count(), 1) + self.assertEqual(case.flags.first(), flag) + + audit_queryset = Audit.objects.filter(target_object_id=case.id) + self.assertNotIn(self.test_audit, audit_queryset) diff --git a/api/cases/views/views.py b/api/cases/views/views.py index 8b4673f471..0c14943ec3 100644 --- a/api/cases/views/views.py +++ b/api/cases/views/views.py @@ -30,7 +30,11 @@ from api.cases.generated_documents.serializers import AdviceDocumentGovSerializer from api.cases.helpers import create_system_mention from api.cases.libraries.advice import group_advice -from api.cases.libraries.finalise import get_required_decision_document_types +from api.cases.libraries.finalise import ( + get_required_decision_document_types, + remove_flags_on_finalisation, + remove_flags_from_audit_trail, +) from api.cases.libraries.get_case import get_case, get_case_document from api.cases.libraries.get_destination import get_destination from api.cases.libraries.get_ecju_queries import get_ecju_query @@ -977,6 +981,10 @@ def put(self, request, pk): case.save() logging.info("Case status is now finalised") + # Remove Flags and related Audits when Finalising + remove_flags_on_finalisation(case) + remove_flags_from_audit_trail(case) + decisions = required_decisions.copy() if AdviceType.REFUSE in decisions: diff --git a/api/flags/migrations/0006_flag_remove_on_finalised.py b/api/flags/migrations/0006_flag_remove_on_finalised.py new file mode 100644 index 0000000000..12ad75f443 --- /dev/null +++ b/api/flags/migrations/0006_flag_remove_on_finalised.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.11 on 2024-04-16 12:02 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("flags", "0005_auto_20220824_2337"), + ] + + operations = [ + migrations.AddField( + model_name="flag", + name="remove_on_finalised", + field=models.BooleanField(default=False), + ), + ] diff --git a/api/flags/models.py b/api/flags/models.py index 71bf57940f..0217088a10 100644 --- a/api/flags/models.py +++ b/api/flags/models.py @@ -25,6 +25,7 @@ class Flag(TimestampableModel): priority = models.PositiveSmallIntegerField(default=0) blocks_finalising = models.BooleanField(default=False) removable_by = models.CharField(choices=FlagPermissions.choices, default=FlagPermissions.DEFAULT, max_length=50) + remove_on_finalised = models.BooleanField(default=False) objects = FlagManager() From 3db77a0d5cc36bb1fa5a1f93fe80283b8b3128bb Mon Sep 17 00:00:00 2001 From: Le H Ngo Date: Tue, 16 Apr 2024 13:11:46 +0100 Subject: [PATCH 02/19] Add withdrawn --- api/applications/views/applications.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/api/applications/views/applications.py b/api/applications/views/applications.py index 7c5788cce6..7b5f03cb7b 100644 --- a/api/applications/views/applications.py +++ b/api/applications/views/applications.py @@ -101,6 +101,8 @@ from api.workflow.flagging_rules_automation import apply_flagging_rules_to_case from lite_routing.routing_rules_internal.routing_engine import run_routing_rules +from api.cases.libraries.finalise import remove_flags_on_finalisation, remove_flags_from_audit_trail +from api.staticdata.statuses.models import CaseStatus class ApplicationList(ListCreateAPIView): @@ -415,6 +417,13 @@ def put(self, request, pk): case_status = get_case_status_by_status(data["status"]) data["status"] = str(case_status.pk) + + # Remove needed flags when case is Withdrawn + withdrawn_status = CaseStatus.objects.get(status=CaseStatusEnum.WITHDRAWN) + if case_status == withdrawn_status: + remove_flags_on_finalisation(application.get_case()) + remove_flags_from_audit_trail(application.get_case()) + old_status = application.status serializer = get_application_update_serializer(application) From d6e02a6f01bd37fcfadeb073499345cb53e5c890 Mon Sep 17 00:00:00 2001 From: Le H Ngo Date: Tue, 16 Apr 2024 18:32:11 +0100 Subject: [PATCH 03/19] Adds django command to remove audit and flags --- .../management/commands/removes_flags.py | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 api/cases/management/commands/removes_flags.py diff --git a/api/cases/management/commands/removes_flags.py b/api/cases/management/commands/removes_flags.py new file mode 100644 index 0000000000..e85fa46ac0 --- /dev/null +++ b/api/cases/management/commands/removes_flags.py @@ -0,0 +1,22 @@ +from django.core.management.base import BaseCommand +from api.cases.libraries.finalise import remove_flags_on_finalisation, remove_flags_from_audit_trail +from api.cases.models import Case +from api.staticdata.statuses.models import CaseStatus +from api.staticdata.statuses.enums import CaseStatusEnum + + +class Command(BaseCommand): + help = "Flags to be removed from Case and Audit Trails" + + def handle(self, *args, **options): + count = 0 + withdrawn_status = CaseStatus.objects.get(status=CaseStatusEnum.WITHDRAWN) + finalised_status = CaseStatus.objects.get(status=CaseStatusEnum.FINALISED) + + for case in Case.objects.all(): + if case.status == finalised_status or case.status == withdrawn_status: + remove_flags_on_finalisation(case) + remove_flags_from_audit_trail(case) + count += 1 + + self.stdout.write(self.style.SUCCESS(f"Successfully adjusted {count} cases.")) From 8b5ca3ec99043c156a11c317b7e9f0d82cc9c003 Mon Sep 17 00:00:00 2001 From: Le H Ngo Date: Wed, 17 Apr 2024 23:35:42 +0100 Subject: [PATCH 04/19] Adds ids to audit trail payload --- api/cases/libraries/finalise.py | 14 +++++++------- api/cases/tests/test_finalise_advice.py | 1 + api/flags/views.py | 10 +++++++++- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/api/cases/libraries/finalise.py b/api/cases/libraries/finalise.py index 58c81826f1..7d359a338d 100644 --- a/api/cases/libraries/finalise.py +++ b/api/cases/libraries/finalise.py @@ -1,5 +1,3 @@ -import json - from api.cases.enums import AdviceType, CaseTypeSubTypeEnum, AdviceLevel from api.cases.models import Advice, GoodCountryDecision from api.applications.models import GoodOnApplication @@ -55,10 +53,12 @@ def remove_flags_on_finalisation(case): def remove_flags_from_audit_trail(case): - flags_to_remove = Flag.objects.filter(remove_on_finalised=True) - for flag in flags_to_remove: - audit_logs = Audit.objects.filter(target_object_id=case.id) + flags_to_remove_ids = list(Flag.objects.filter(remove_on_finalised=True).values_list("id", flat=True)) + flags_to_remove_ids = [str(flag_id) for flag_id in flags_to_remove_ids] # Convert UUIDs to strings + audit_logs = Audit.objects.filter(target_object_id=case.id) + + for flag_id in flags_to_remove_ids: for audit_log in audit_logs: - payload_json_string = json.dumps(audit_log.payload) - if flag.name in payload_json_string: + payload = audit_log.payload + if flag_id in payload.get("added_flags_id", []) or flag_id in payload.get("removed_flags_id", []): audit_log.delete() diff --git a/api/cases/tests/test_finalise_advice.py b/api/cases/tests/test_finalise_advice.py index fec0da24a7..a5cce0efc5 100644 --- a/api/cases/tests/test_finalise_advice.py +++ b/api/cases/tests/test_finalise_advice.py @@ -147,6 +147,7 @@ def setUp(self): payload={ "added_flags": [self.test_flag.name], "additional_text": "test", + "added_flags_id": [str(self.test_flag.id)], }, ) diff --git a/api/flags/views.py b/api/flags/views.py index e13af7c87b..4fc3e8950f 100644 --- a/api/flags/views.py +++ b/api/flags/views.py @@ -168,6 +168,9 @@ def _assign_flags(self, flags, level, note, obj, user): ignored_flags = flags + [x for x in previously_assigned_deactivated_team_flags] removed_flag_names = [flag.name for flag in previously_assigned_team_flags if flag not in ignored_flags] + self.added_flags_id = [str(flag.id) for flag in flags if flag not in previously_assigned_team_flags] + self.removed_flags_id = [str(flag.id) for flag in previously_assigned_team_flags if flag not in ignored_flags] + # Add activity item if isinstance(obj, Case): @@ -205,6 +208,7 @@ def _set_case_activity(self, added_flags, removed_flags, case, user, note, **kwa payload={ "added_flags": added_flags, "additional_text": note, + "added_flags_id": self.added_flags_id, }, ) @@ -213,7 +217,11 @@ def _set_case_activity(self, added_flags, removed_flags, case, user, note, **kwa actor=user, verb=AuditType.REMOVE_FLAGS, target=case, - payload={"removed_flags": removed_flags, "additional_text": note}, + payload={ + "removed_flags": removed_flags, + "additional_text": note, + "removed_flags_id": self.removed_flags_id, + }, ) def _set_organisation_activity(self, added_flags, removed_flags, organisation, user, note, **kwargs): From 6865cd747aa8d1b257fefa26b45c3badc3d341db Mon Sep 17 00:00:00 2001 From: Le H Ngo Date: Thu, 18 Apr 2024 13:10:02 +0100 Subject: [PATCH 05/19] Unit test for django command --- .../commands/tests/test_removes_flags.py | 90 +++++++++++++++++++ api/cases/tests/test_finalise_advice.py | 1 - 2 files changed, 90 insertions(+), 1 deletion(-) create mode 100644 api/cases/management/commands/tests/test_removes_flags.py diff --git a/api/cases/management/commands/tests/test_removes_flags.py b/api/cases/management/commands/tests/test_removes_flags.py new file mode 100644 index 0000000000..7347a861d6 --- /dev/null +++ b/api/cases/management/commands/tests/test_removes_flags.py @@ -0,0 +1,90 @@ +from django.core.management import call_command + +from test_helpers.clients import DataTestClient + +from api.flags.models import Flag +from api.users.models import BaseUser +from api.users.enums import UserType +from api.audit_trail.enums import AuditType + +from api.audit_trail import service as audit_trail_service + +from api.cases.tests.factories import FinalAdviceFactory +from api.cases.enums import AdviceType +from api.staticdata.statuses.enums import CaseStatusEnum +from api.staticdata.statuses.models import CaseStatus +from api.audit_trail.models import Audit + + +class TestCommand(DataTestClient): + def setUp(self): + super().setUp() + self.application = self.create_standard_application_case(self.organisation) + self.original_flag_id = self.application.flags.first().id + + # Add Flag to test with removal after Finalise + self.test_flag = Flag.objects.all().first() + self.test_flag.remove_on_finalised = True + self.test_flag.save() + self.application.flags.add(self.test_flag) + + user = BaseUser(email="test@mail.com", first_name="John", last_name="Smith", type=UserType.SYSTEM) + + self.case = self.application.get_case() + + self.test_audit = audit_trail_service.create( + actor=user, + verb=AuditType.ADD_FLAGS, + target=self.case, + payload={ + "added_flags": [self.test_flag.name], + "additional_text": "test", + "added_flags_id": [str(self.test_flag.id)], + }, + ) + + FinalAdviceFactory(user=self.gov_user, case=self.application, type=AdviceType.APPROVE) + + def test_call_command_remove_flags_and_trails_finalised( + self, + ): + status_finalised = CaseStatus.objects.get(status=CaseStatusEnum.FINALISED) + self.case.status = status_finalised + self.case.save() + + audit_queryset = Audit.objects.filter(target_object_id=self.case.id) + + self.assertEqual(self.case.flags.count(), 2) + self.assertEqual(audit_queryset.count(), 2) + + call_command("removes_flags") + + audit_queryset = Audit.objects.filter(target_object_id=self.case.id) + + flag = Flag.objects.get(id=self.original_flag_id) + self.assertEqual(self.case.flags.count(), 1) + self.assertEqual(self.case.flags.first(), flag) + self.assertEqual(audit_queryset.count(), 1) + self.assertNotIn(self.test_audit, audit_queryset) + + def test_call_command_remove_flags_and_trails_withdrawn( + self, + ): + status_withdrawn = CaseStatus.objects.get(status=CaseStatusEnum.WITHDRAWN) + self.case.status = status_withdrawn + self.case.save() + + audit_queryset = Audit.objects.filter(target_object_id=self.case.id) + + self.assertEqual(self.case.flags.count(), 2) + self.assertEqual(audit_queryset.count(), 2) + + call_command("removes_flags") + + audit_queryset = Audit.objects.filter(target_object_id=self.case.id) + + flag = Flag.objects.get(id=self.original_flag_id) + self.assertEqual(self.case.flags.count(), 1) + self.assertEqual(self.case.flags.first(), flag) + self.assertEqual(audit_queryset.count(), 1) + self.assertNotIn(self.test_audit, audit_queryset) diff --git a/api/cases/tests/test_finalise_advice.py b/api/cases/tests/test_finalise_advice.py index a5cce0efc5..1d42c941e5 100644 --- a/api/cases/tests/test_finalise_advice.py +++ b/api/cases/tests/test_finalise_advice.py @@ -2,7 +2,6 @@ from django.urls import reverse from api.audit_trail.enums import AuditType from api.flags.models import Flag -from django.contrib.auth.models import User from api.users.enums import UserType from api.users.models import BaseUser from rest_framework import status From d139aac1dd9a56bcf42fa5e7f9032e0629206ac7 Mon Sep 17 00:00:00 2001 From: Le H Ngo Date: Mon, 22 Apr 2024 16:51:11 +0100 Subject: [PATCH 06/19] Refactor and cleaning code --- api/applications/views/applications.py | 13 ++++++------- api/cases/libraries/finalise.py | 10 +++------- ...py => removes_flags_and_audits_from_cases.py} | 16 ++++++++++++---- ... test_removes_flags_and_audits_from_cases.py} | 4 ++-- 4 files changed, 23 insertions(+), 20 deletions(-) rename api/cases/management/commands/{removes_flags.py => removes_flags_and_audits_from_cases.py} (59%) rename api/cases/management/commands/tests/{test_removes_flags.py => test_removes_flags_and_audits_from_cases.py} (96%) diff --git a/api/applications/views/applications.py b/api/applications/views/applications.py index 7b5f03cb7b..2b95f8c66f 100644 --- a/api/applications/views/applications.py +++ b/api/applications/views/applications.py @@ -417,13 +417,6 @@ def put(self, request, pk): case_status = get_case_status_by_status(data["status"]) data["status"] = str(case_status.pk) - - # Remove needed flags when case is Withdrawn - withdrawn_status = CaseStatus.objects.get(status=CaseStatusEnum.WITHDRAWN) - if case_status == withdrawn_status: - remove_flags_on_finalisation(application.get_case()) - remove_flags_from_audit_trail(application.get_case()) - old_status = application.status serializer = get_application_update_serializer(application) @@ -459,6 +452,12 @@ def put(self, request, pk): data = get_application_view_serializer(application)(application, context={"user_type": request.user.type}).data + # Remove needed flags when case is Withdrawn + withdrawn_status = CaseStatus.objects.get(status=CaseStatusEnum.WITHDRAWN) + if case_status == withdrawn_status: + remove_flags_on_finalisation(application.get_case()) + remove_flags_from_audit_trail(application.get_case()) + return JsonResponse(data={"data": data}, status=status.HTTP_200_OK) diff --git a/api/cases/libraries/finalise.py b/api/cases/libraries/finalise.py index 7d359a338d..104b7b139b 100644 --- a/api/cases/libraries/finalise.py +++ b/api/cases/libraries/finalise.py @@ -45,16 +45,12 @@ def get_required_decision_document_types(case): def remove_flags_on_finalisation(case): - case_flags = case.flags.all() - for flag in case_flags: - if flag.remove_on_finalised: - case.flags.remove(flag) - case.save() + flags_to_remove = Flag.objects.filter(remove_on_finalised=True) + case.flags.remove(*flags_to_remove) def remove_flags_from_audit_trail(case): - flags_to_remove_ids = list(Flag.objects.filter(remove_on_finalised=True).values_list("id", flat=True)) - flags_to_remove_ids = [str(flag_id) for flag_id in flags_to_remove_ids] # Convert UUIDs to strings + flags_to_remove_ids = [str(flag.id) for flag in Flag.objects.filter(remove_on_finalised=True)] audit_logs = Audit.objects.filter(target_object_id=case.id) for flag_id in flags_to_remove_ids: diff --git a/api/cases/management/commands/removes_flags.py b/api/cases/management/commands/removes_flags_and_audits_from_cases.py similarity index 59% rename from api/cases/management/commands/removes_flags.py rename to api/cases/management/commands/removes_flags_and_audits_from_cases.py index e85fa46ac0..3942a21639 100644 --- a/api/cases/management/commands/removes_flags.py +++ b/api/cases/management/commands/removes_flags_and_audits_from_cases.py @@ -1,22 +1,30 @@ +import logging + +from django.db import transaction +from django.db.models import Q + from django.core.management.base import BaseCommand from api.cases.libraries.finalise import remove_flags_on_finalisation, remove_flags_from_audit_trail from api.cases.models import Case from api.staticdata.statuses.models import CaseStatus from api.staticdata.statuses.enums import CaseStatusEnum +logger = logging.getLogger(__name__) + class Command(BaseCommand): - help = "Flags to be removed from Case and Audit Trails" + help = "Command to remove required flags from Cases and Audit Trails for the past cases" def handle(self, *args, **options): count = 0 withdrawn_status = CaseStatus.objects.get(status=CaseStatusEnum.WITHDRAWN) finalised_status = CaseStatus.objects.get(status=CaseStatusEnum.FINALISED) - for case in Case.objects.all(): - if case.status == finalised_status or case.status == withdrawn_status: + cases = Case.objects.filter(Q(status=withdrawn_status) | Q(status=finalised_status)) + with transaction.atomic(): + for case in cases: remove_flags_on_finalisation(case) remove_flags_from_audit_trail(case) count += 1 - self.stdout.write(self.style.SUCCESS(f"Successfully adjusted {count} cases.")) + self.stdout.write(self.style.SUCCESS(f"Successfully adjusted {count} cases.")) diff --git a/api/cases/management/commands/tests/test_removes_flags.py b/api/cases/management/commands/tests/test_removes_flags_and_audits_from_cases.py similarity index 96% rename from api/cases/management/commands/tests/test_removes_flags.py rename to api/cases/management/commands/tests/test_removes_flags_and_audits_from_cases.py index 7347a861d6..8fa3abc7ee 100644 --- a/api/cases/management/commands/tests/test_removes_flags.py +++ b/api/cases/management/commands/tests/test_removes_flags_and_audits_from_cases.py @@ -57,7 +57,7 @@ def test_call_command_remove_flags_and_trails_finalised( self.assertEqual(self.case.flags.count(), 2) self.assertEqual(audit_queryset.count(), 2) - call_command("removes_flags") + call_command("removes_flags_and_audits_from_cases") audit_queryset = Audit.objects.filter(target_object_id=self.case.id) @@ -79,7 +79,7 @@ def test_call_command_remove_flags_and_trails_withdrawn( self.assertEqual(self.case.flags.count(), 2) self.assertEqual(audit_queryset.count(), 2) - call_command("removes_flags") + call_command("removes_flags_and_audits_from_cases") audit_queryset = Audit.objects.filter(target_object_id=self.case.id) From 266e286960f6be6ff73efcaf28dbe152f79bbf80 Mon Sep 17 00:00:00 2001 From: Le H Ngo Date: Tue, 23 Apr 2024 11:11:55 +0100 Subject: [PATCH 07/19] Adds additional requirement status CLOSED --- api/applications/views/applications.py | 6 +-- .../removes_flags_and_audits_from_cases.py | 13 ++++--- ...est_removes_flags_and_audits_from_cases.py | 38 ++++--------------- api/cases/tests/test_finalise_advice.py | 21 +++++----- 4 files changed, 27 insertions(+), 51 deletions(-) diff --git a/api/applications/views/applications.py b/api/applications/views/applications.py index 2b95f8c66f..369454b1b3 100644 --- a/api/applications/views/applications.py +++ b/api/applications/views/applications.py @@ -102,7 +102,6 @@ from lite_routing.routing_rules_internal.routing_engine import run_routing_rules from api.cases.libraries.finalise import remove_flags_on_finalisation, remove_flags_from_audit_trail -from api.staticdata.statuses.models import CaseStatus class ApplicationList(ListCreateAPIView): @@ -452,9 +451,8 @@ def put(self, request, pk): data = get_application_view_serializer(application)(application, context={"user_type": request.user.type}).data - # Remove needed flags when case is Withdrawn - withdrawn_status = CaseStatus.objects.get(status=CaseStatusEnum.WITHDRAWN) - if case_status == withdrawn_status: + # Remove needed flags when case is Withdrawn/Closed + if case_status.status in [CaseStatusEnum.WITHDRAWN, CaseStatusEnum.CLOSED]: remove_flags_on_finalisation(application.get_case()) remove_flags_from_audit_trail(application.get_case()) diff --git a/api/cases/management/commands/removes_flags_and_audits_from_cases.py b/api/cases/management/commands/removes_flags_and_audits_from_cases.py index 3942a21639..a45803fcca 100644 --- a/api/cases/management/commands/removes_flags_and_audits_from_cases.py +++ b/api/cases/management/commands/removes_flags_and_audits_from_cases.py @@ -1,7 +1,6 @@ import logging from django.db import transaction -from django.db.models import Q from django.core.management.base import BaseCommand from api.cases.libraries.finalise import remove_flags_on_finalisation, remove_flags_from_audit_trail @@ -17,14 +16,18 @@ class Command(BaseCommand): def handle(self, *args, **options): count = 0 - withdrawn_status = CaseStatus.objects.get(status=CaseStatusEnum.WITHDRAWN) - finalised_status = CaseStatus.objects.get(status=CaseStatusEnum.FINALISED) + statuses = [ + CaseStatus.objects.get(status=CaseStatusEnum.WITHDRAWN), + CaseStatus.objects.get(status=CaseStatusEnum.FINALISED), + CaseStatus.objects.get(status=CaseStatusEnum.CLOSED), + ] + + cases = Case.objects.filter(status__in=statuses) - cases = Case.objects.filter(Q(status=withdrawn_status) | Q(status=finalised_status)) with transaction.atomic(): for case in cases: remove_flags_on_finalisation(case) remove_flags_from_audit_trail(case) count += 1 - self.stdout.write(self.style.SUCCESS(f"Successfully adjusted {count} cases.")) + logging.info("Successfully adjusted %s cases.", cases.count()) diff --git a/api/cases/management/commands/tests/test_removes_flags_and_audits_from_cases.py b/api/cases/management/commands/tests/test_removes_flags_and_audits_from_cases.py index 8fa3abc7ee..cc8f4b27fd 100644 --- a/api/cases/management/commands/tests/test_removes_flags_and_audits_from_cases.py +++ b/api/cases/management/commands/tests/test_removes_flags_and_audits_from_cases.py @@ -1,4 +1,5 @@ from django.core.management import call_command +from parameterized import parameterized from test_helpers.clients import DataTestClient @@ -45,11 +46,12 @@ def setUp(self): FinalAdviceFactory(user=self.gov_user, case=self.application, type=AdviceType.APPROVE) - def test_call_command_remove_flags_and_trails_finalised( - self, - ): - status_finalised = CaseStatus.objects.get(status=CaseStatusEnum.FINALISED) - self.case.status = status_finalised + @parameterized.expand( + case_status for case_status in [CaseStatusEnum.FINALISED, CaseStatusEnum.WITHDRAWN, CaseStatusEnum.CLOSED] + ) + def test_call_command_removes_flags_and_audits_from_cases(self, case_status): + status_var = CaseStatus.objects.get(status=case_status) + self.case.status = status_var self.case.save() audit_queryset = Audit.objects.filter(target_object_id=self.case.id) @@ -61,30 +63,6 @@ def test_call_command_remove_flags_and_trails_finalised( audit_queryset = Audit.objects.filter(target_object_id=self.case.id) - flag = Flag.objects.get(id=self.original_flag_id) - self.assertEqual(self.case.flags.count(), 1) - self.assertEqual(self.case.flags.first(), flag) - self.assertEqual(audit_queryset.count(), 1) - self.assertNotIn(self.test_audit, audit_queryset) - - def test_call_command_remove_flags_and_trails_withdrawn( - self, - ): - status_withdrawn = CaseStatus.objects.get(status=CaseStatusEnum.WITHDRAWN) - self.case.status = status_withdrawn - self.case.save() - - audit_queryset = Audit.objects.filter(target_object_id=self.case.id) - - self.assertEqual(self.case.flags.count(), 2) - self.assertEqual(audit_queryset.count(), 2) - - call_command("removes_flags_and_audits_from_cases") - - audit_queryset = Audit.objects.filter(target_object_id=self.case.id) - - flag = Flag.objects.get(id=self.original_flag_id) - self.assertEqual(self.case.flags.count(), 1) - self.assertEqual(self.case.flags.first(), flag) + self.assertNotIn(self.test_flag, self.case.flags.all()) self.assertEqual(audit_queryset.count(), 1) self.assertNotIn(self.test_audit, audit_queryset) diff --git a/api/cases/tests/test_finalise_advice.py b/api/cases/tests/test_finalise_advice.py index 1d42c941e5..7d6d4593e6 100644 --- a/api/cases/tests/test_finalise_advice.py +++ b/api/cases/tests/test_finalise_advice.py @@ -1,3 +1,4 @@ +import pytest from unittest import mock from django.urls import reverse from api.audit_trail.enums import AuditType @@ -5,6 +6,7 @@ from api.users.enums import UserType from api.users.models import BaseUser from rest_framework import status +from parameterized import parameterized from api.audit_trail.models import Audit from api.cases.enums import AdviceType, CaseTypeEnum @@ -203,32 +205,27 @@ def test_finalised_standard_application_with_flags_removed( # Make sure Flag was removed and Audit trail was removed case = get_case(self.application.id) - flag = Flag.objects.filter(id=self.original_flag_id).first() - self.assertEqual(case.flags.count(), 1) - self.assertEqual(case.flags.first(), flag) + self.assertNotIn(self.test_flag, case.flags.all()) audit_queryset = Audit.objects.filter(target_object_id=case.id) self.assertNotIn(self.test_audit, audit_queryset) - def test_standard_application_withdrawn( - self, - ): + @parameterized.expand(case_status for case_status in [CaseStatusEnum.WITHDRAWN, CaseStatusEnum.CLOSED]) + def test_standard_application_remove_audit_and_flag_with_statuses(self, case_status): url = reverse("applications:manage_status", kwargs={"pk": self.application.id}) - data = {"status": CaseStatusEnum.WITHDRAWN} + data = {"status": case_status} self.assertEqual(self.application.flags.count(), 2) - response = self.client.put(url, data=data, **self.exporter_headers) + response = self.client.put(url, data=data, **self.gov_headers) self.application.refresh_from_db() self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual(self.application.status, get_case_status_by_status(CaseStatusEnum.WITHDRAWN)) + self.assertEqual(self.application.status, get_case_status_by_status(case_status)) # Make sure Flag was removed and Audit trail was removed case = get_case(self.application.id) - flag = Flag.objects.filter(id=self.original_flag_id).first() - self.assertEqual(case.flags.count(), 1) - self.assertEqual(case.flags.first(), flag) + self.assertNotIn(self.test_flag, case.flags.all()) audit_queryset = Audit.objects.filter(target_object_id=case.id) self.assertNotIn(self.test_audit, audit_queryset) From e13b1c359197a154a018da1d3c8e7d592229fb76 Mon Sep 17 00:00:00 2001 From: Henry Cooksley Date: Tue, 23 Apr 2024 18:17:26 +0100 Subject: [PATCH 08/19] Update CSV uploader to support entity_type field --- api/external_data/helpers.py | 4 +- api/external_data/serializers.py | 4 ++ api/external_data/tests/test_views.py | 87 +++++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 2 deletions(-) diff --git a/api/external_data/helpers.py b/api/external_data/helpers.py index 20c6a0394c..64ae9c48bf 100644 --- a/api/external_data/helpers.py +++ b/api/external_data/helpers.py @@ -1,6 +1,6 @@ def get_denial_entity_type(data): entity_type = "" - for key in ["end_user_flag", "consignee_flag", "other_role"]: + for key in ["end_user_flag", "consignee_flag"]: if data.get(key) and isinstance(data[key], str): data[key] = data[key].lower() == "true" if data.get("end_user_flag", False) is True: @@ -10,7 +10,7 @@ def get_denial_entity_type(data): elif ( data.get("end_user_flag", False) is False and data.get("consignee_flag", False) is False - and data.get("other_role", False) is True + and isinstance(data.get("other_role", None), str) ): entity_type = "Third-party" diff --git a/api/external_data/serializers.py b/api/external_data/serializers.py index 5b1ef5ac09..de4b3b2000 100644 --- a/api/external_data/serializers.py +++ b/api/external_data/serializers.py @@ -126,6 +126,10 @@ def validate_csv_file(self, value): denial_entity, is_denial_entity_created = models.DenialEntity.objects.update_or_create( defaults=serializer.validated_data, denial=denial, **denial_entity_lookup_fields ) + # Store any extra columns in csv not already captured in data dict + denial_entity.data = {field: row[field] for field in row if field not in self.required_headers} + denial_entity.entity_type = get_denial_entity_type(denial_entity.data) + denial_entity.save() if is_denial_entity_created: logging_counts["denial_entity"]["created"] += 1 diff --git a/api/external_data/tests/test_views.py b/api/external_data/tests/test_views.py index 23f80f4517..121ffe666b 100644 --- a/api/external_data/tests/test_views.py +++ b/api/external_data/tests/test_views.py @@ -104,6 +104,93 @@ def test_create_error_missing_required_headers(self): self.assertEqual(response.status_code, 400) self.assertEqual(response.json(), {"errors": {"csv_file": ["Missing required headers in CSV file"]}}) + def test_create_and_set_entity_type(self): + url = reverse("external_data:denial-list") + content = """ + reference,regime_reg_ref,name,address,notifying_government,country,item_list_codes,item_description,consignee_name,end_use,reason_for_refusal,spire_entity_id,end_user_flag,consignee_flag,other_role + DN2000/0000,AB-CD-EF-000,Organisation Name,"1000 Street Name, City Name",Country Name,Country Name,0A00100,Medium Size Widget,Example Name,Used in industry,Risk of outcome,123,TRUE,FALSE, + DN2000/0010,AB-CD-EF-300,Organisation Name 3,"2001 Street Name, City Name 3",Country Name 3,Country Name 3,0A00201,Unspecified Size Widget,Example Name 3,Used in other industry,Risk of outcome 3,125,FALSE,TRUE, + DN2010/0001,AB-XY-EF-900,The Widget Company,"2 Example Road, Example City",Example Country,Country Name X,"catch all",Extra Large Size Widget,Example Name 4,Used in unknown industry,Risk of outcome 4,126,FALSE,FALSE,other + DN3000/0000,AB-CD-EF-100,Organisation Name XYZ,"2000 Street Name, City Name 2",Country Name 2,Country Name 2,0A00200,Large Size Widget,Example Name 2,Used in other industry,Risk of outcome 2,124,TRUE,TRUE, + """ + response = self.client.post(url, {"csv_file": content}, **self.gov_headers) + + self.assertEqual(response.status_code, 201) + self.assertEqual(models.DenialEntity.objects.count(), 4) + self.assertEqual( + list( + models.DenialEntity.objects.values( + *serializers.DenialFromCSVFileSerializer.required_headers, "data", "entity_type" + ) + ), + [ + { + "reference": "DN2000/0000", + "regime_reg_ref": "AB-CD-EF-000", + "name": "Organisation Name", + "address": "1000 Street Name, City Name", + "notifying_government": "Country Name", + "country": "Country Name", + "item_list_codes": "0A00100", + "item_description": "Medium Size Widget", + "consignee_name": "Example Name", + "end_use": "Used in industry", + "reason_for_refusal": "Risk of outcome", + "spire_entity_id": 123, + "data": {"other_role": "", "end_user_flag": True, "consignee_flag": False}, + "entity_type": "End-user", + }, + { + "reference": "DN2000/0010", + "regime_reg_ref": "AB-CD-EF-300", + "name": "Organisation Name 3", + "address": "2001 Street Name, City Name 3", + "notifying_government": "Country Name 3", + "country": "Country Name 3", + "item_list_codes": "0A00201", + "item_description": "Unspecified Size Widget", + "consignee_name": "Example Name 3", + "end_use": "Used in other industry", + "reason_for_refusal": "Risk of outcome 3", + "spire_entity_id": 125, + "data": {"other_role": "", "end_user_flag": False, "consignee_flag": True}, + "entity_type": "Consignee", + }, + { + "reference": "DN2010/0001", + "regime_reg_ref": "AB-XY-EF-900", + "name": "The Widget Company", + "address": "2 Example Road, Example City", + "notifying_government": "Example Country", + "country": "Country Name X", + "item_list_codes": "catch all", + "item_description": "Extra Large Size Widget", + "consignee_name": "Example Name 4", + "end_use": "Used in unknown industry", + "reason_for_refusal": "Risk of outcome 4", + "spire_entity_id": 126, + "data": {"other_role": "other", "end_user_flag": False, "consignee_flag": False}, + "entity_type": "Third-party", + }, + { + "reference": "DN3000/0000", + "regime_reg_ref": "AB-CD-EF-100", + "name": "Organisation Name XYZ", + "address": "2000 Street Name, City Name 2", + "notifying_government": "Country Name 2", + "country": "Country Name 2", + "item_list_codes": "0A00200", + "item_description": "Large Size Widget", + "consignee_name": "Example Name 2", + "end_use": "Used in other industry", + "reason_for_refusal": "Risk of outcome 2", + "spire_entity_id": 124, + "data": {"other_role": "", "end_user_flag": True, "consignee_flag": True}, + "entity_type": "End-user", + }, + ], + ) + def test_update_success(self): url = reverse("external_data:denial-list") content = """ From a8a3c356bb2642b7b150a9b528d7abf53c5111b7 Mon Sep 17 00:00:00 2001 From: Henry Cooksley Date: Wed, 24 Apr 2024 14:32:53 +0100 Subject: [PATCH 09/19] Update tests --- api/external_data/tests/test_views.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/api/external_data/tests/test_views.py b/api/external_data/tests/test_views.py index 121ffe666b..8595a9c229 100644 --- a/api/external_data/tests/test_views.py +++ b/api/external_data/tests/test_views.py @@ -107,11 +107,11 @@ def test_create_error_missing_required_headers(self): def test_create_and_set_entity_type(self): url = reverse("external_data:denial-list") content = """ - reference,regime_reg_ref,name,address,notifying_government,country,item_list_codes,item_description,consignee_name,end_use,reason_for_refusal,spire_entity_id,end_user_flag,consignee_flag,other_role - DN2000/0000,AB-CD-EF-000,Organisation Name,"1000 Street Name, City Name",Country Name,Country Name,0A00100,Medium Size Widget,Example Name,Used in industry,Risk of outcome,123,TRUE,FALSE, - DN2000/0010,AB-CD-EF-300,Organisation Name 3,"2001 Street Name, City Name 3",Country Name 3,Country Name 3,0A00201,Unspecified Size Widget,Example Name 3,Used in other industry,Risk of outcome 3,125,FALSE,TRUE, - DN2010/0001,AB-XY-EF-900,The Widget Company,"2 Example Road, Example City",Example Country,Country Name X,"catch all",Extra Large Size Widget,Example Name 4,Used in unknown industry,Risk of outcome 4,126,FALSE,FALSE,other - DN3000/0000,AB-CD-EF-100,Organisation Name XYZ,"2000 Street Name, City Name 2",Country Name 2,Country Name 2,0A00200,Large Size Widget,Example Name 2,Used in other industry,Risk of outcome 2,124,TRUE,TRUE, + reference,regime_reg_ref,name,address,notifying_government,country,item_list_codes,item_description,end_use,reason_for_refusal,spire_entity_id,end_user_flag,consignee_flag,other_role + DN2000/0000,AB-CD-EF-000,Organisation Name,"1000 Street Name, City Name",Country Name,Country Name,0A00100,Medium Size Widget,Used in industry,Risk of outcome,123,TRUE,FALSE, + DN2000/0010,AB-CD-EF-300,Organisation Name 3,"2001 Street Name, City Name 3",Country Name 3,Country Name 3,0A00201,Unspecified Size Widget,Used in other industry,Risk of outcome 3,125,FALSE,TRUE, + DN2010/0001,AB-XY-EF-900,The Widget Company,"2 Example Road, Example City",Example Country,Country Name X,"catch all",Extra Large Size Widget,Used in unknown industry,Risk of outcome 4,126,FALSE,FALSE,other + DN3000/0000,AB-CD-EF-100,Organisation Name XYZ,"2000 Street Name, City Name 2",Country Name 2,Country Name 2,0A00200,Large Size Widget,Used in other industry,Risk of outcome 2,124,TRUE,TRUE, """ response = self.client.post(url, {"csv_file": content}, **self.gov_headers) @@ -133,7 +133,6 @@ def test_create_and_set_entity_type(self): "country": "Country Name", "item_list_codes": "0A00100", "item_description": "Medium Size Widget", - "consignee_name": "Example Name", "end_use": "Used in industry", "reason_for_refusal": "Risk of outcome", "spire_entity_id": 123, @@ -149,7 +148,6 @@ def test_create_and_set_entity_type(self): "country": "Country Name 3", "item_list_codes": "0A00201", "item_description": "Unspecified Size Widget", - "consignee_name": "Example Name 3", "end_use": "Used in other industry", "reason_for_refusal": "Risk of outcome 3", "spire_entity_id": 125, @@ -165,7 +163,6 @@ def test_create_and_set_entity_type(self): "country": "Country Name X", "item_list_codes": "catch all", "item_description": "Extra Large Size Widget", - "consignee_name": "Example Name 4", "end_use": "Used in unknown industry", "reason_for_refusal": "Risk of outcome 4", "spire_entity_id": 126, @@ -181,7 +178,6 @@ def test_create_and_set_entity_type(self): "country": "Country Name 2", "item_list_codes": "0A00200", "item_description": "Large Size Widget", - "consignee_name": "Example Name 2", "end_use": "Used in other industry", "reason_for_refusal": "Risk of outcome 2", "spire_entity_id": 124, From 6e4208efc582f743f13b902f4b473d75bd28ed14 Mon Sep 17 00:00:00 2001 From: "mark.j0hnst0n" Date: Thu, 25 Apr 2024 10:19:53 +0100 Subject: [PATCH 10/19] added new migration to backfill entity_type empty strings --- .../0025_entity_type_empty_strings.py | 48 ++++++++++ .../test_0025_entity_type_empty_strings.py | 90 +++++++++++++++++++ 2 files changed, 138 insertions(+) create mode 100644 api/external_data/migrations/0025_entity_type_empty_strings.py create mode 100644 api/external_data/migrations/tests/test_0025_entity_type_empty_strings.py diff --git a/api/external_data/migrations/0025_entity_type_empty_strings.py b/api/external_data/migrations/0025_entity_type_empty_strings.py new file mode 100644 index 0000000000..7525efa0ae --- /dev/null +++ b/api/external_data/migrations/0025_entity_type_empty_strings.py @@ -0,0 +1,48 @@ +# Generated by Django 4.2.11 on 2024-04-18 12:00 + +from django.db import migrations +from api.external_data.enums import DenialEntityType + + +def get_denial_entity_type(data): + + if isinstance(data, dict): + entity_type = "" + normalised_entity_type_dict = {keys.lower(): values.lower() for keys, values in data.items()} + + is_end_user_flag = normalised_entity_type_dict.get("end_user_flag", "false") == "true" + is_consignee_flag = normalised_entity_type_dict.get("consignee_flag", "false") == "true" + is_other_role = len(normalised_entity_type_dict.get("other_role", "")) > 0 + + if is_end_user_flag and is_consignee_flag: + entity_type = DenialEntityType.END_USER + elif not is_end_user_flag and is_consignee_flag: + entity_type = DenialEntityType.CONSIGNEE + elif is_end_user_flag and not is_consignee_flag: + entity_type = DenialEntityType.END_USER + elif not is_end_user_flag and not is_consignee_flag and is_other_role: + entity_type = DenialEntityType.THIRD_PARTY + + return entity_type + + +def set_denial_entity_type(apps, schema_editor): + + DenialEntity = apps.get_model("external_data", "DenialEntity") + + for denial_entity in DenialEntity.objects.filter(entity_type=""): + + denial_entity_type = get_denial_entity_type(denial_entity.data) + + if denial_entity_type in ["end_user", "consignee", "third_party"]: + denial_entity.entity_type = denial_entity_type + denial_entity.save() + + +class Migration(migrations.Migration): + + dependencies = [ + ("external_data", "0024_denials_data_migration"), + ] + + operations = [migrations.RunPython(set_denial_entity_type, migrations.RunPython.noop)] diff --git a/api/external_data/migrations/tests/test_0025_entity_type_empty_strings.py b/api/external_data/migrations/tests/test_0025_entity_type_empty_strings.py new file mode 100644 index 0000000000..2e43fde1d7 --- /dev/null +++ b/api/external_data/migrations/tests/test_0025_entity_type_empty_strings.py @@ -0,0 +1,90 @@ +import pytest + +from django_test_migrations.contrib.unittest_case import MigratorTestCase +from api.external_data.enums import DenialEntityType + +test_data = [ + { + "reference": "DN2000/0000", + "regime_reg_ref": "AB-CD-EF-000", + "name": "Organisation Name", + "address": "1000 Street Name, City Name", + "notifying_government": "Country Name", + "country": "Country Name", + "item_list_codes": "0A00100", + "item_description": "Medium Size Widget", + "consignee_name": "Example Name", + "end_use": "Used in industry", + "reason_for_refusal": "Risk of outcome", + "spire_entity_id": 123, + "data": {"END_USER_FLAG": "true", "CONSIGNEE_FLAG": "false", "OTHER_ROLE": ""}, + "entity_type": "", + }, + { + "reference": "DN2000/0010", + "regime_reg_ref": "AB-CD-EF-300", + "name": "Organisation Name 3", + "address": "2001 Street Name, City Name 3", + "notifying_government": "Country Name 3", + "country": "Country Name 3", + "item_list_codes": "0A00201", + "item_description": "Unspecified Size Widget", + "consignee_name": "Example Name 3", + "end_use": "Used in other industry", + "reason_for_refusal": "Risk of outcome 3", + "spire_entity_id": 125, + "data": {"END_USER_FLAG": "false", "CONSIGNEE_FLAG": "true", "OTHER_ROLE": ""}, + "entity_type": "", + }, + { + "reference": "DN2000/0000", + "regime_reg_ref": "AB-CD-EF-000", + "name": "Organisation Name", + "address": "1000 Street Name, City Name", + "notifying_government": "Country Name", + "country": "Country Name", + "item_list_codes": "0A00100", + "item_description": "Medium Size Widget", + "consignee_name": "Example Name", + "end_use": "Used in industry", + "reason_for_refusal": "Risk of outcome", + "spire_entity_id": 123, + "data": {"END_USER_FLAG": "true", "CONSIGNEE_FLAG": "true", "OTHER_ROLE": ""}, + "entity_type": "", + }, + { + "reference": "DN3000/0000", + "regime_reg_ref": "AB-CD-EF-100", + "name": "Organisation Name XYZ", + "address": "2000 Street Name, City Name 2", + "notifying_government": "Country Name 2", + "country": "Country Name 2", + "item_list_codes": "0A00200", + "item_description": "Large Size Widget", + "consignee_name": "Example Name 2", + "end_use": "Used in other industry", + "reason_for_refusal": "Risk of outcome 2", + "spire_entity_id": 124, + "data": {"END_USER_FLAG": "false", "CONSIGNEE_FLAG": "false", "OTHER_ROLE": "other"}, + "entity_type": "", + }, +] + + +@pytest.mark.django_db() +class TestDenialEntityTypeSet(MigratorTestCase): + migrate_from = ("external_data", "0024_denials_data_migration") + migrate_to = ("external_data", "0025_entity_type_empty_strings") + + def prepare(self): + DenialEntity = self.old_state.apps.get_model("external_data", "DenialEntity") + for row in test_data: + DenialEntity.objects.create(**row) + + def test_0025_set_entity_type_empty_strings(self): + DenialEntity = self.new_state.apps.get_model("external_data", "DenialEntity") + + self.assertEqual(DenialEntity.objects.all().count(), 4) + self.assertEqual(DenialEntity.objects.filter(entity_type=DenialEntityType.END_USER).count(), 2) + self.assertEqual(DenialEntity.objects.filter(entity_type=DenialEntityType.CONSIGNEE).count(), 1) + self.assertEqual(DenialEntity.objects.filter(entity_type=DenialEntityType.THIRD_PARTY).count(), 1) From 0d353dccfba2cd11ac5467b96a58a69f0a45bc1f Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 19 Apr 2024 13:46:37 +0000 Subject: [PATCH 11/19] Bump sqlparse from 0.4.4 to 0.5.0 Bumps [sqlparse](https://github.com/andialbrecht/sqlparse) from 0.4.4 to 0.5.0. - [Changelog](https://github.com/andialbrecht/sqlparse/blob/master/CHANGELOG) - [Commits](https://github.com/andialbrecht/sqlparse/compare/0.4.4...0.5.0) --- updated-dependencies: - dependency-name: sqlparse dependency-type: indirect ... Signed-off-by: dependabot[bot] --- Pipfile.lock | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/Pipfile.lock b/Pipfile.lock index 8319c83904..4e4d771b48 100644 --- a/Pipfile.lock +++ b/Pipfile.lock @@ -1259,11 +1259,12 @@ }, "sqlparse": { "hashes": [ - "sha256:5430a4fe2ac7d0f93e66f1efc6e1338a41884b7ddf2a350cedd20ccc4d9d28f3", - "sha256:d446183e84b8349fa3061f0fe7f06ca94ba65b426946ffebe6e3e8295332420c" + "sha256:714d0a4932c059d16189f58ef5411ec2287a4360f17cdd0edd2d09d4c5087c93", + "sha256:c204494cd97479d0e39f28c93d46c0b2d5959c7b9ab904762ea6c7af211c8663" ], - "markers": "python_version >= '3.5'", - "version": "==0.4.4" + "index": "pypi", + "markers": "python_version >= '3.8'", + "version": "==0.5.0" }, "tblib": { "hashes": [ From 528f9b491ef570e6e778bbd0873f7dddf49d2794 Mon Sep 17 00:00:00 2001 From: Gurdeep Atwal Date: Wed, 17 Apr 2024 19:27:42 +0100 Subject: [PATCH 12/19] change search --- api/external_data/documents.py | 67 ++++++++++++++++--- .../management/commands/ingest_denials.py | 2 +- api/external_data/serializers.py | 14 ++-- api/external_data/views.py | 4 +- 4 files changed, 66 insertions(+), 21 deletions(-) diff --git a/api/external_data/documents.py b/api/external_data/documents.py index d976485a76..8e907a82ba 100644 --- a/api/external_data/documents.py +++ b/api/external_data/documents.py @@ -1,8 +1,7 @@ from django.conf import settings from django_elasticsearch_dsl import Document, fields from django_elasticsearch_dsl.registries import registry -from elasticsearch_dsl import analysis - +from elasticsearch_dsl import analysis, InnerDoc from api.external_data import models from api.search.application.documents import lowercase_normalizer @@ -74,27 +73,73 @@ def get_value_from_instance(self, instance, field_value_to_ignore=None): ) -class DenialDocumentType(Document): +class Denial(InnerDoc): + regime_reg_ref = fields.KeywordField() + + +class DenialEnitytDocument(Document): id = fields.KeywordField() name = fields.TextField() address = fields.TextField( analyzer=address_analyzer_no_ngram, ) - reference = fields.KeywordField() - regime_reg_ref = fields.KeywordField() - notifying_government = fields.TextField() country = fields.TextField( fields={ "raw": fields.KeywordField(normalizer=lowercase_normalizer), }, ) - item_list_codes = fields.TextField() - item_description = fields.TextField() - consignee_name = fields.TextField() - end_use = fields.TextField() data = DataField() is_revoked = fields.BooleanField() + denial = fields.ObjectField( + attr="denial", + properties={ + "regime_reg_ref": fields.TextField( + attr="regime_reg_ref", + fields={ + "raw": fields.KeywordField(), + }, + ), + "notifying_government": fields.TextField( + attr="notifying_government", + fields={ + "raw": fields.KeywordField(), + }, + ), + "reference": fields.TextField( + attr="reference", + fields={ + "raw": fields.KeywordField(), + }, + ), + "notifying_government": fields.TextField( + attr="notifying_government", + fields={ + "raw": fields.KeywordField(), + }, + ), + "item_list_codes": fields.TextField( + attr="item_list_codes", + fields={ + "raw": fields.KeywordField(), + }, + ), + "item_description": fields.TextField( + attr="item_description", + fields={ + "raw": fields.KeywordField(), + }, + ), + "end_use": fields.TextField( + attr="end_use", + fields={ + "raw": fields.KeywordField(), + }, + ), + "is_revoked": fields.BooleanField(), + }, + ) + class Index: name = settings.ELASTICSEARCH_DENIALS_INDEX_ALIAS settings = { @@ -142,4 +187,4 @@ class Index: if settings.LITE_API_ENABLE_ES: - registry.register_document(DenialDocumentType) + registry.register_document(DenialEnitytDocument) diff --git a/api/external_data/management/commands/ingest_denials.py b/api/external_data/management/commands/ingest_denials.py index 344c1b601e..b6c62e7aa7 100644 --- a/api/external_data/management/commands/ingest_denials.py +++ b/api/external_data/management/commands/ingest_denials.py @@ -49,7 +49,7 @@ def add_arguments(self, parser): def rebuild_index(self): connection = connections.get_connection() connection.indices.delete(index=settings.ELASTICSEARCH_DENIALS_INDEX_ALIAS, ignore=[404]) - documents.DenialDocumentType.init() + documents.DenialEnitytDocument.init() @staticmethod def add_bulk_errors(errors, row_number, line_errors): diff --git a/api/external_data/serializers.py b/api/external_data/serializers.py index 5b1ef5ac09..b9e68e0b3e 100644 --- a/api/external_data/serializers.py +++ b/api/external_data/serializers.py @@ -175,20 +175,20 @@ def add_bulk_errors(errors, row_number, line_errors): class DenialSearchSerializer(DocumentSerializer): entity_type = serializers.SerializerMethodField() + regime_reg_ref = serializers.ReadOnlyField(source="denial.regime_reg_ref") + reference = serializers.ReadOnlyField(source="denial.reference") + notifying_government = serializers.ReadOnlyField(source="denial.notifying_government") + item_list_codes = serializers.ReadOnlyField(source="denial.item_list_codes") + item_description = serializers.ReadOnlyField(source="denial.item_description") + end_use = serializers.ReadOnlyField(source="denial.end_use") class Meta: - document = documents.DenialDocumentType + document = documents.DenialEnitytDocument fields = ( "id", "address", "country", - "end_use", - "item_description", - "item_list_codes", "name", - "notifying_government", - "reference", - "regime_reg_ref", ) def get_entity_type(self, obj): diff --git a/api/external_data/views.py b/api/external_data/views.py index 59eb136ba6..312e1bf68c 100644 --- a/api/external_data/views.py +++ b/api/external_data/views.py @@ -36,7 +36,7 @@ def perform_create(self, serializer): class DenialSearchView(DocumentViewSet): - document = documents.DenialDocumentType + document = documents.DenialEnitytDocument serializer_class = serializers.DenialSearchSerializer authentication_classes = (GovAuthentication,) pagination_class = MaxPageNumberPagination @@ -56,7 +56,7 @@ class DenialSearchView(DocumentViewSet): ordering = "_score" def filter_queryset(self, queryset): - queryset = queryset.filter("term", is_revoked=False) + queryset = queryset.filter("term", denial__is_revoked=False) return super().filter_queryset(queryset) From 1874fc5051aa6c43f13d0f0d2b4a57729d050c04 Mon Sep 17 00:00:00 2001 From: Gurdeep Atwal Date: Thu, 18 Apr 2024 11:09:46 +0100 Subject: [PATCH 13/19] update serialisers to viw/update from denial object --- api/external_data/documents.py | 11 ----------- .../management/commands/ingest_denials.py | 1 - api/external_data/serializers.py | 18 ++++++++++++++++++ 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/api/external_data/documents.py b/api/external_data/documents.py index 8e907a82ba..ae654d1aa7 100644 --- a/api/external_data/documents.py +++ b/api/external_data/documents.py @@ -72,11 +72,6 @@ def get_value_from_instance(self, instance, field_value_to_ignore=None): filter=["lowercase", "asciifolding"], ) - -class Denial(InnerDoc): - regime_reg_ref = fields.KeywordField() - - class DenialEnitytDocument(Document): id = fields.KeywordField() name = fields.TextField() @@ -100,12 +95,6 @@ class DenialEnitytDocument(Document): "raw": fields.KeywordField(), }, ), - "notifying_government": fields.TextField( - attr="notifying_government", - fields={ - "raw": fields.KeywordField(), - }, - ), "reference": fields.TextField( attr="reference", fields={ diff --git a/api/external_data/management/commands/ingest_denials.py b/api/external_data/management/commands/ingest_denials.py index b6c62e7aa7..609550aa8f 100644 --- a/api/external_data/management/commands/ingest_denials.py +++ b/api/external_data/management/commands/ingest_denials.py @@ -36,7 +36,6 @@ class Command(BaseCommand): "country", "item_list_codes", "item_description", - "consignee_name", "end_use", "reason_for_refusal", "spire_entity_id", diff --git a/api/external_data/serializers.py b/api/external_data/serializers.py index b9e68e0b3e..24f92c874f 100644 --- a/api/external_data/serializers.py +++ b/api/external_data/serializers.py @@ -13,6 +13,15 @@ class DenialEntitySerializer(serializers.ModelSerializer): entity_type = serializers.SerializerMethodField() + regime_reg_ref = serializers.CharField(source="denial.regime_reg_ref") + reference = serializers.CharField(source="denial.reference") + item_list_codes = serializers.CharField(source="denial.item_list_codes") + notifying_government = serializers.CharField(source="denial.notifying_government") + item_description = serializers.CharField(source="denial.item_description") + end_use = serializers.CharField(source="denial.end_use") + is_revoked = serializers.BooleanField(source="denial.is_revoked") + is_revoked_comment = serializers.CharField(source="denial.is_revoked_comment") + reason_for_refusal = serializers.CharField(source="denial.reason_for_refusal") class Meta: model = models.DenialEntity @@ -48,6 +57,15 @@ def validate(self, data): raise serializers.ValidationError({"is_revoked_comment": "This field is required"}) return validated_data + def update(self, instance, validated_data): + # This is required because the flattened columns are required to support the older denial screen + # Serialisers don't support dotted fileds so we need to override. is_revoked and is_revoked_comments + # are the only updates we support so this is ok. + instance.denial.is_revoked = validated_data["denial"]["is_revoked"] + instance.denial.is_revoked_comment = validated_data["denial"]["is_revoked_comment"] + instance.denial.save() + return instance + def get_entity_type(self, obj): return get_denial_entity_type(obj.data) From 9f5fddbe3712e78757d2f403cc5069b876fac28e Mon Sep 17 00:00:00 2001 From: Gurdeep Atwal Date: Tue, 23 Apr 2024 15:20:00 +0100 Subject: [PATCH 14/19] fix csv uploader --- .../tests/test_matching_denials.py | 26 ++++ .../tests/test_external_data_views.py | 1 - api/external_data/documents.py | 15 ++- .../management/commands/ingest_denials.py | 45 ++++--- .../commands/tests/test_ingest_denials.py | 14 +-- api/external_data/serializers.py | 112 ++++++++++++------ api/external_data/tests/test_views.py | 100 +++++++++------- api/external_data/views.py | 4 +- 8 files changed, 208 insertions(+), 109 deletions(-) diff --git a/api/applications/tests/test_matching_denials.py b/api/applications/tests/test_matching_denials.py index d5a3c6323b..f36f69fc82 100644 --- a/api/applications/tests/test_matching_denials.py +++ b/api/applications/tests/test_matching_denials.py @@ -79,6 +79,32 @@ def test_revoke_denial_success(self): self.assertEqual(response["is_revoked"], True) self.assertEqual(response["is_revoked_comment"], "This denial is no longer active") + def test_revoke_denial_active_success(self): + response = self.client.get(reverse("external_data:denial-list"), **self.gov_headers) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.json()["count"], 4) + + denials = response.json()["results"] + + # pick one and revoke it + self.assertEqual(denials[0]["is_revoked"], False) + denialentity = models.DenialEntity.objects.get(pk=denials[0]["id"]) + denialentity.denial.is_revoked = True + denialentity.denial.save() + + response = self.client.patch( + reverse("external_data:denial-detail", kwargs={"pk": denials[0]["id"]}), + { + "is_revoked": False, + }, + **self.gov_headers, + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + response = response.json() + self.assertEqual(response["is_revoked"], False) + self.assertEqual(response["is_revoked_comment"], "") + def test_view_denial_notifications_on_the_application(self): data = [] for index in range(10): diff --git a/api/data_workspace/tests/test_external_data_views.py b/api/data_workspace/tests/test_external_data_views.py index 83d6a11a15..a796cec9d4 100644 --- a/api/data_workspace/tests/test_external_data_views.py +++ b/api/data_workspace/tests/test_external_data_views.py @@ -30,7 +30,6 @@ def test_denial_view(self): "country", "item_list_codes", "item_description", - "consignee_name", "end_use", "data", "is_revoked", diff --git a/api/external_data/documents.py b/api/external_data/documents.py index ae654d1aa7..c1882cb467 100644 --- a/api/external_data/documents.py +++ b/api/external_data/documents.py @@ -1,7 +1,7 @@ from django.conf import settings from django_elasticsearch_dsl import Document, fields from django_elasticsearch_dsl.registries import registry -from elasticsearch_dsl import analysis, InnerDoc +from elasticsearch_dsl import analysis from api.external_data import models from api.search.application.documents import lowercase_normalizer @@ -72,7 +72,8 @@ def get_value_from_instance(self, instance, field_value_to_ignore=None): filter=["lowercase", "asciifolding"], ) -class DenialEnitytDocument(Document): + +class DenialEntityDocument(Document): id = fields.KeywordField() name = fields.TextField() address = fields.TextField( @@ -84,7 +85,7 @@ class DenialEnitytDocument(Document): }, ) data = DataField() - is_revoked = fields.BooleanField() + is_revoked = fields.BooleanField(attr="denial.is_revoked") denial = fields.ObjectField( attr="denial", @@ -125,7 +126,6 @@ class DenialEnitytDocument(Document): "raw": fields.KeywordField(), }, ), - "is_revoked": fields.BooleanField(), }, ) @@ -142,6 +142,11 @@ class Meta: class Django: model = models.DenialEntity + related_models = [models.Denial] + + def get_instances_from_related(self, related_instance): + if isinstance(related_instance, models.Denial): + return related_instance.denial_entity.all() class SanctionDocumentType(Document): @@ -176,4 +181,4 @@ class Index: if settings.LITE_API_ENABLE_ES: - registry.register_document(DenialEnitytDocument) + registry.register_document(DenialEntityDocument) diff --git a/api/external_data/management/commands/ingest_denials.py b/api/external_data/management/commands/ingest_denials.py index 609550aa8f..6097644210 100644 --- a/api/external_data/management/commands/ingest_denials.py +++ b/api/external_data/management/commands/ingest_denials.py @@ -5,7 +5,7 @@ from django.core.management.base import BaseCommand from django.db import transaction from api.applications.models import DenialMatchOnApplication -from api.external_data.serializers import DenialEntitySerializer +from api.external_data.serializers import DenialEntitySerializer, DenialSerializer from rest_framework import serializers from elasticsearch_dsl import connections @@ -27,18 +27,21 @@ def get_json_content_and_delete(filename): class Command(BaseCommand): - required_headers = [ - "reference", - "regime_reg_ref", + required_headers_denial_entity = [ "name", "address", - "notifying_government", "country", + "spire_entity_id", + ] + + required_headers_denial = [ + "reference", + "regime_reg_ref", + "notifying_government", "item_list_codes", "item_description", "end_use", "reason_for_refusal", - "spire_entity_id", ] def add_arguments(self, parser): @@ -48,7 +51,7 @@ def add_arguments(self, parser): def rebuild_index(self): connection = connections.get_connection() connection.indices.delete(index=settings.ELASTICSEARCH_DENIALS_INDEX_ALIAS, ignore=[404]) - documents.DenialEnitytDocument.init() + documents.DenialEntityDocument.init() @staticmethod def add_bulk_errors(errors, row_number, line_errors): @@ -78,20 +81,30 @@ def load_denials(self, filename): ).exists() if exists: continue - serializer = DenialEntitySerializer( - data={ - "data": row, - **{field: row.pop(field, None) for field in self.required_headers}, - } - ) - if serializer.is_valid(): - serializer.save() + denial_entity_data = {field: row.pop(field, None) for field in self.required_headers_denial_entity} + denial_data = {field: row.pop(field, None) for field in self.required_headers_denial} + denial_entity_data["data"] = row + denial_serializer = DenialSerializer(data=denial_data) + denial_entity_serializer = DenialEntitySerializer(data=denial_entity_data) + + is_valid_denial = denial_serializer.is_valid() + is_valid_entity_denial = denial_entity_serializer.is_valid() + + if is_valid_denial and is_valid_entity_denial: + denial = denial_serializer.save() + denial_entity = denial_entity_serializer.save() + denial_entity.denial = denial + denial_entity.save() log.info( "Saved row number -> %s", i, ) else: - self.add_bulk_errors(errors=errors, row_number=i + 1, line_errors=serializer.errors) + self.add_bulk_errors( + errors=errors, + row_number=i + 1, + line_errors={**denial_serializer.errors, **denial_entity_serializer.errors}, + ) if errors: log.exception( diff --git a/api/external_data/management/commands/tests/test_ingest_denials.py b/api/external_data/management/commands/tests/test_ingest_denials.py index fe890bf1a8..b9bd5e7b89 100644 --- a/api/external_data/management/commands/tests/test_ingest_denials.py +++ b/api/external_data/management/commands/tests/test_ingest_denials.py @@ -81,16 +81,16 @@ def test_populate_denials(mock_json_content, mock_delete_file, json_file_data): call_command("ingest_denials", "json_file", rebuild=True) assert DenialEntity.objects.all().count() == 3 denial_record = DenialEntity.objects.all()[0] - assert denial_record.reference == "DN001\/0003" + assert denial_record.denial.reference == "DN001\/0003" assert denial_record.name == "Test1 case" assert denial_record.address == "somewhere\nmid\nlatter\nCairo" - assert denial_record.notifying_government == "United Kingdom" + assert denial_record.denial.notifying_government == "United Kingdom" assert denial_record.country == "United States" - assert denial_record.item_list_codes == "123456" - assert denial_record.item_description == "phone" - assert denial_record.end_use == "locating phone" - assert denial_record.regime_reg_ref == "12" - assert denial_record.reason_for_refusal == "reason a" + assert denial_record.denial.item_list_codes == "123456" + assert denial_record.denial.item_description == "phone" + assert denial_record.denial.end_use == "locating phone" + assert denial_record.denial.regime_reg_ref == "12" + assert denial_record.denial.reason_for_refusal == "reason a" assert denial_record.spire_entity_id == 1234 mock_delete_file.assert_called_with(document_id="json_file", s3_key="json_file") diff --git a/api/external_data/serializers.py b/api/external_data/serializers.py index 24f92c874f..354b75f10b 100644 --- a/api/external_data/serializers.py +++ b/api/external_data/serializers.py @@ -11,17 +11,40 @@ from api.flags.enums import SystemFlags +class DenialSerializer(serializers.ModelSerializer): + class Meta: + model = models.Denial + fields = ( + "id", + "created_by_user", + "reference", + "regime_reg_ref", + "notifying_government", + "item_list_codes", + "item_description", + "end_use", + "is_revoked", + "is_revoked_comment", + "reason_for_refusal", + ) + extra_kwargs = { + "is_revoked": {"required": False}, + "is_revoked_comment": {"required": False}, + "reason_for_refusal": {"required": False}, + } + + class DenialEntitySerializer(serializers.ModelSerializer): entity_type = serializers.SerializerMethodField() - regime_reg_ref = serializers.CharField(source="denial.regime_reg_ref") - reference = serializers.CharField(source="denial.reference") - item_list_codes = serializers.CharField(source="denial.item_list_codes") - notifying_government = serializers.CharField(source="denial.notifying_government") - item_description = serializers.CharField(source="denial.item_description") - end_use = serializers.CharField(source="denial.end_use") - is_revoked = serializers.BooleanField(source="denial.is_revoked") - is_revoked_comment = serializers.CharField(source="denial.is_revoked_comment") - reason_for_refusal = serializers.CharField(source="denial.reason_for_refusal") + regime_reg_ref = serializers.CharField(source="denial.regime_reg_ref", required=False) + reference = serializers.CharField(source="denial.reference", required=False) + item_list_codes = serializers.CharField(source="denial.item_list_codes", required=False) + notifying_government = serializers.CharField(source="denial.notifying_government", required=False) + item_description = serializers.CharField(source="denial.item_description", required=False) + end_use = serializers.CharField(source="denial.end_use", required=False) + is_revoked = serializers.BooleanField(source="denial.is_revoked", required=False) + is_revoked_comment = serializers.CharField(source="denial.is_revoked_comment", required=False) + reason_for_refusal = serializers.CharField(source="denial.reason_for_refusal", required=False) class Meta: model = models.DenialEntity @@ -36,7 +59,6 @@ class Meta: "country", "item_list_codes", "item_description", - "consignee_name", "end_use", "data", "is_revoked", @@ -53,7 +75,9 @@ class Meta: def validate(self, data): validated_data = super().validate(data) - if validated_data.get("is_revoked") and not validated_data.get("is_revoked_comment"): + if validated_data.get("denial", {}).get("is_revoked") and not validated_data.get("denial", {}).get( + "is_revoked_comment" + ): raise serializers.ValidationError({"is_revoked_comment": "This field is required"}) return validated_data @@ -61,9 +85,15 @@ def update(self, instance, validated_data): # This is required because the flattened columns are required to support the older denial screen # Serialisers don't support dotted fileds so we need to override. is_revoked and is_revoked_comments # are the only updates we support so this is ok. - instance.denial.is_revoked = validated_data["denial"]["is_revoked"] - instance.denial.is_revoked_comment = validated_data["denial"]["is_revoked_comment"] - instance.denial.save() + + if validated_data.get("denial", {}).get("is_revoked"): + instance.denial.is_revoked = validated_data["denial"]["is_revoked"] + instance.denial.is_revoked_comment = validated_data["denial"]["is_revoked_comment"] + instance.denial.save() + elif validated_data.get("denial", {}).get("is_revoked") is False: + instance.denial.is_revoked = validated_data["denial"]["is_revoked"] + instance.denial.is_revoked_comment = "" + instance.denial.save() return instance def get_entity_type(self, obj): @@ -74,18 +104,21 @@ class DenialFromCSVFileSerializer(serializers.Serializer): csv_file = serializers.CharField() - required_headers = [ - "reference", - "regime_reg_ref", + required_headers_denial_entity = [ "name", "address", - "notifying_government", "country", + "spire_entity_id", + ] + + required_headers_denial = [ + "reference", + "regime_reg_ref", + "notifying_government", "item_list_codes", "item_description", "end_use", "reason_for_refusal", - "spire_entity_id", ] @transaction.atomic @@ -94,7 +127,7 @@ def validate_csv_file(self, value): reader = csv.DictReader(csv_file) # Check if required headers are present - if not (set(self.required_headers)).issubset(set(reader.fieldnames)): # type: ignore + if not (set(self.required_headers_denial_entity + self.required_headers_denial)).issubset(set(reader.fieldnames)): # type: ignore raise serializers.ValidationError("Missing required headers in CSV file") logging_counts = {"denial": {"created": 0, "updated": 0}, "denial_entity": {"created": 0, "updated": 0}} @@ -105,22 +138,30 @@ def validate_csv_file(self, value): errors = [] for i, row in enumerate(reader, start=1): denial_entity_data = { - **{field: row[field] for field in self.required_headers}, + **{field: row[field] for field in self.required_headers_denial_entity}, "created_by": self.context["request"].user, } + + denial_data = {**{field: row[field].strip() for field in self.required_headers_denial}} + # Create a serializer instance to validate data serializer = DenialEntitySerializer(data=denial_entity_data) - if serializer.is_valid() and isinstance(serializer.validated_data, dict): + denial_serializer = DenialSerializer(data=denial_data) + is_valid_denial_data = denial_serializer.is_valid() + + if len(denial_serializer.errors) == 1 and denial_serializer.errors.get("regime_reg_ref"): + # This is a bit of a hack presently we care about uniqueness but do want to validate the fields + # Hence we override the check + + if ( + hasattr(denial_serializer.errors["regime_reg_ref"][0], "code") + and denial_serializer.errors["regime_reg_ref"][0].code == "unique" + ): + is_valid_denial_data = True + + if serializer.is_valid() and isinstance(serializer.validated_data, dict) and is_valid_denial_data: # Try to update an existing Denial record or create a new one - regime_reg_ref = serializer.validated_data["regime_reg_ref"] - denial_data = { - "reference": serializer.validated_data["reference"], - "notifying_government": serializer.validated_data["notifying_government"], - "item_list_codes": serializer.validated_data["item_list_codes"], - "item_description": serializer.validated_data["item_description"], - "end_use": serializer.validated_data["end_use"], - "reason_for_refusal": serializer.validated_data["reason_for_refusal"], - } + regime_reg_ref = denial_serializer.data["regime_reg_ref"] denial, is_denial_created = models.Denial.objects.update_or_create( regime_reg_ref=regime_reg_ref, defaults=denial_data ) @@ -135,12 +176,13 @@ def validate_csv_file(self, value): # We assume that a DenialEntity object already exists if we can # match on all of the following fields denial_entity_lookup_fields = { - "reference": serializer.validated_data["reference"], - "regime_reg_ref": regime_reg_ref, "name": serializer.validated_data["name"], "address": serializer.validated_data["address"], } # Link the validated DenialEntity data with the Denial + + denial_entity_created_data = serializer.validated_data + denial_entity_created_data["denial"] = denial denial_entity, is_denial_entity_created = models.DenialEntity.objects.update_or_create( defaults=serializer.validated_data, denial=denial, **denial_entity_lookup_fields ) @@ -152,7 +194,7 @@ def validate_csv_file(self, value): logging_counts["denial_entity"]["updated"] += 1 logging_regime_reg_ref_values["denial_entity"]["updated"].append(denial_entity.regime_reg_ref) else: - self.add_bulk_errors(errors, i, serializer.errors) + self.add_bulk_errors(errors, i, {**serializer.errors, **denial_serializer.errors}) if errors: raise serializers.ValidationError(errors) @@ -201,7 +243,7 @@ class DenialSearchSerializer(DocumentSerializer): end_use = serializers.ReadOnlyField(source="denial.end_use") class Meta: - document = documents.DenialEnitytDocument + document = documents.DenialEntityDocument fields = ( "id", "address", diff --git a/api/external_data/tests/test_views.py b/api/external_data/tests/test_views.py index 23f80f4517..31b1458d3c 100644 --- a/api/external_data/tests/test_views.py +++ b/api/external_data/tests/test_views.py @@ -32,64 +32,82 @@ def test_create_success(self): self.assertEqual(response.status_code, 201) self.assertEqual(models.DenialEntity.objects.count(), 4) + self.assertEqual(models.Denial.objects.count(), 4) self.assertEqual( - list(models.DenialEntity.objects.values(*serializers.DenialFromCSVFileSerializer.required_headers, "data")), + list( + models.DenialEntity.objects.values( + *serializers.DenialFromCSVFileSerializer.required_headers_denial_entity, "data" + ) + ), [ { - "reference": "DN2000/0000", - "regime_reg_ref": "AB-CD-EF-000", "name": "Organisation Name", "address": "1000 Street Name, City Name", - "notifying_government": "Country Name", "country": "Country Name", + "spire_entity_id": 123, + "data": {}, + }, + { + "name": "Organisation Name 3", + "address": "2001 Street Name, City Name 3", + "country": "Country Name 3", + "spire_entity_id": 125, + "data": {}, + }, + { + "name": "The Widget Company", + "address": "2 Example Road, Example City", + "country": "Country Name X", + "spire_entity_id": 126, + "data": {}, + }, + { + "name": "Organisation Name XYZ", + "address": "2000 Street Name, City Name 2", + "country": "Country Name 2", + "spire_entity_id": 124, + "data": {}, + }, + ], + ) + self.assertEqual( + list(models.Denial.objects.values(*serializers.DenialFromCSVFileSerializer.required_headers_denial)), + [ + { + "reference": "DN2000/0000", + "regime_reg_ref": "AB-CD-EF-000", + "notifying_government": "Country Name", "item_list_codes": "0A00100", "item_description": "Medium Size Widget", "end_use": "Used in industry", "reason_for_refusal": "Risk of outcome", - "spire_entity_id": 123, - "data": {}, }, { "reference": "DN2000/0010", "regime_reg_ref": "AB-CD-EF-300", - "name": "Organisation Name 3", - "address": "2001 Street Name, City Name 3", "notifying_government": "Country Name 3", - "country": "Country Name 3", "item_list_codes": "0A00201", "item_description": "Unspecified Size Widget", "end_use": "Used in other industry", "reason_for_refusal": "Risk of outcome 3", - "spire_entity_id": 125, - "data": {}, }, { "reference": "DN2010/0001", "regime_reg_ref": "AB-XY-EF-900", - "name": "The Widget Company", - "address": "2 Example Road, Example City", "notifying_government": "Example Country", - "country": "Country Name X", "item_list_codes": "catch all", "item_description": "Extra Large Size Widget", "end_use": "Used in unknown industry", "reason_for_refusal": "Risk of outcome 4", - "spire_entity_id": 126, - "data": {}, }, { "reference": "DN3000/0000", "regime_reg_ref": "AB-CD-EF-100", - "name": "Organisation Name XYZ", - "address": "2000 Street Name, City Name 2", "notifying_government": "Country Name 2", - "country": "Country Name 2", "item_list_codes": "0A00200", "item_description": "Large Size Widget", "end_use": "Used in other industry", "reason_for_refusal": "Risk of outcome 2", - "spire_entity_id": 124, - "data": {}, }, ], ) @@ -110,10 +128,12 @@ def test_update_success(self): reference,regime_reg_ref,name,address,notifying_government,country,item_list_codes,item_description,end_use,reason_for_refusal,spire_entity_id DN2000/0000,AB-CD-EF-000,Organisation Name,"1000 Street Name, City Name",Country Name,Country Name,0A00100,Medium Size Widget,Used in industry,Risk of outcome,123 """ + response = self.client.post(url, {"csv_file": content}, **self.gov_headers) self.assertEqual(response.status_code, 201) self.assertEqual(models.Denial.objects.count(), 1) self.assertEqual(models.DenialEntity.objects.count(), 1) + self.assertEqual( list(models.Denial.objects.values(*denial_data_fields)), [ @@ -129,19 +149,16 @@ def test_update_success(self): ], ) self.assertEqual( - list(models.DenialEntity.objects.values(*serializers.DenialFromCSVFileSerializer.required_headers, "data")), + list( + models.DenialEntity.objects.values( + *serializers.DenialFromCSVFileSerializer.required_headers_denial_entity, "data" + ) + ), [ { - "reference": "DN2000/0000", - "regime_reg_ref": "AB-CD-EF-000", "name": "Organisation Name", "address": "1000 Street Name, City Name", - "notifying_government": "Country Name", "country": "Country Name", - "item_list_codes": "0A00100", - "item_description": "Medium Size Widget", - "end_use": "Used in industry", - "reason_for_refusal": "Risk of outcome", "spire_entity_id": 123, "data": {}, }, @@ -149,7 +166,7 @@ def test_update_success(self): ) updated_content = """ reference,regime_reg_ref,name,address,notifying_government,country,item_list_codes,item_description,end_use,reason_for_refusal,spire_entity_id - DN2000/0000,AB-CD-EF-000,Organisation Name,"1000 Street Name, City Name",Country Name 2,Country Name 2,0A00200,Medium Size Widget 2,Used in industry 2,Risk of outcome 2,124 + DN2000/0000,AB-CD-EF-000,Organisation Name,"1000 Street Name, City Name",Country Name 2,Country Name 2,0A00200,Medium Size Widget 2, Used in industry 2,Risk of outcome 2,124 """ response = self.client.post(url, {"csv_file": updated_content}, **self.gov_headers) self.assertEqual(response.status_code, 201) @@ -166,26 +183,23 @@ def test_update_success(self): "item_description": "Medium Size Widget 2", "end_use": "Used in industry 2", "reason_for_refusal": "Risk of outcome 2", - }, + } ], ) self.assertEqual( - list(models.DenialEntity.objects.values(*serializers.DenialFromCSVFileSerializer.required_headers, "data")), + list( + models.DenialEntity.objects.values( + *serializers.DenialFromCSVFileSerializer.required_headers_denial_entity, "data" + ) + ), [ { - "reference": "DN2000/0000", - "regime_reg_ref": "AB-CD-EF-000", "name": "Organisation Name", "address": "1000 Street Name, City Name", - "notifying_government": "Country Name 2", "country": "Country Name 2", - "item_list_codes": "0A00200", - "item_description": "Medium Size Widget 2", - "end_use": "Used in industry 2", - "reason_for_refusal": "Risk of outcome 2", "spire_entity_id": 124, "data": {}, - }, + } ], ) @@ -246,9 +260,9 @@ def test_populate_denial_entity_objects(self, page_query): # Set one of them as revoked denial_entity = models.DenialEntity.objects.get(name="Organisation Name") - denial_entity.is_revoked = True - denial_entity.save() - + denial_entity.denial.is_revoked = True + denial_entity.denial.save() + # This needs to be fixed we need to rebuild index if child value is updated. # Then only 2 denial entity objects will be returned when searching url = reverse("external_data:denial_search-list") diff --git a/api/external_data/views.py b/api/external_data/views.py index 312e1bf68c..0b0df5cc81 100644 --- a/api/external_data/views.py +++ b/api/external_data/views.py @@ -36,7 +36,7 @@ def perform_create(self, serializer): class DenialSearchView(DocumentViewSet): - document = documents.DenialEnitytDocument + document = documents.DenialEntityDocument serializer_class = serializers.DenialSearchSerializer authentication_classes = (GovAuthentication,) pagination_class = MaxPageNumberPagination @@ -56,7 +56,7 @@ class DenialSearchView(DocumentViewSet): ordering = "_score" def filter_queryset(self, queryset): - queryset = queryset.filter("term", denial__is_revoked=False) + queryset = queryset.filter("term", is_revoked=False) return super().filter_queryset(queryset) From 73be9fa08fd45d1a3c573683dd869108565fb9bf Mon Sep 17 00:00:00 2001 From: Gurdeep Atwal Date: Tue, 30 Apr 2024 08:43:57 +0100 Subject: [PATCH 15/19] entity-type-changes add enum --- api/external_data/helpers.py | 25 ++++++ .../management/commands/ingest_denials.py | 5 +- .../commands/tests/test_ingest_denials.py | 21 ++++- api/external_data/serializers.py | 10 +-- api/external_data/tests/denial_valid.csv | 10 +-- api/external_data/tests/test_views.py | 82 ++++++------------- 6 files changed, 79 insertions(+), 74 deletions(-) diff --git a/api/external_data/helpers.py b/api/external_data/helpers.py index 64ae9c48bf..b6a20332e2 100644 --- a/api/external_data/helpers.py +++ b/api/external_data/helpers.py @@ -1,3 +1,6 @@ +from api.external_data.enums import DenialEntityType + + def get_denial_entity_type(data): entity_type = "" for key in ["end_user_flag", "consignee_flag"]: @@ -15,3 +18,25 @@ def get_denial_entity_type(data): entity_type = "Third-party" return entity_type + + +def get_denial_entity_enum(data): + + if isinstance(data, dict): + entity_type = "" + normalised_entity_type_dict = {keys.lower(): values.lower() for keys, values in data.items()} + + is_end_user_flag = normalised_entity_type_dict.get("end_user_flag", "false") == "true" + is_consignee_flag = normalised_entity_type_dict.get("consignee_flag", "false") == "true" + is_other_role = len(normalised_entity_type_dict.get("other_role", "")) > 0 + + if is_end_user_flag and is_consignee_flag: + entity_type = DenialEntityType.END_USER + elif not is_end_user_flag and is_consignee_flag: + entity_type = DenialEntityType.CONSIGNEE + elif is_end_user_flag and not is_consignee_flag: + entity_type = DenialEntityType.END_USER + elif not is_end_user_flag and not is_consignee_flag and is_other_role: + entity_type = DenialEntityType.THIRD_PARTY + + return entity_type diff --git a/api/external_data/management/commands/ingest_denials.py b/api/external_data/management/commands/ingest_denials.py index 6097644210..d3a6847a2c 100644 --- a/api/external_data/management/commands/ingest_denials.py +++ b/api/external_data/management/commands/ingest_denials.py @@ -8,11 +8,12 @@ from api.external_data.serializers import DenialEntitySerializer, DenialSerializer from rest_framework import serializers + from elasticsearch_dsl import connections from api.documents.libraries import s3_operations from api.external_data import documents - +from api.external_data.helpers import get_denial_entity_enum from api.external_data.models import DenialEntity log = logging.getLogger(__name__) @@ -81,9 +82,11 @@ def load_denials(self, filename): ).exists() if exists: continue + denial_entity_data = {field: row.pop(field, None) for field in self.required_headers_denial_entity} denial_data = {field: row.pop(field, None) for field in self.required_headers_denial} denial_entity_data["data"] = row + denial_entity_data["entity_type"] = get_denial_entity_enum(denial_entity_data["data"]) denial_serializer = DenialSerializer(data=denial_data) denial_entity_serializer = DenialEntitySerializer(data=denial_entity_data) diff --git a/api/external_data/management/commands/tests/test_ingest_denials.py b/api/external_data/management/commands/tests/test_ingest_denials.py index b9bd5e7b89..0154a7a09a 100644 --- a/api/external_data/management/commands/tests/test_ingest_denials.py +++ b/api/external_data/management/commands/tests/test_ingest_denials.py @@ -46,7 +46,7 @@ def json_file_data(): "item_list_codes": "12345\/2009", "item_description": "testing machine", "end_use": "For teaching purposes", - "end_user_flag": "true", + "end_user_flag": "false", "consignee_flag": "true", "reason_for_refusal": "reason b", "spire_entity_id": 1235, @@ -65,6 +65,21 @@ def json_file_data(): "reason_for_refusal": "reason c", "spire_entity_id": 1236, }, + { + "reference": "DN001\/0000", + "regime_reg_ref": "12345", + "name": "Test4 case", + "address": "antartica", + "notifying_government": "United States", + "country": "Italy", + "item_description": "lazer", + "end_use": "testing", + "end_user_flag": "false", + "consignee_flag": "false", + "other_role": "my role", + "reason_for_refusal": "reason c", + "spire_entity_id": 1236, + }, ] ) ) @@ -79,7 +94,7 @@ def test_populate_denials(mock_json_content, mock_delete_file, json_file_data): mock_json_content.return_value = json_file_data call_command("ingest_denials", "json_file", rebuild=True) - assert DenialEntity.objects.all().count() == 3 + assert DenialEntity.objects.all().count() == 4 denial_record = DenialEntity.objects.all()[0] assert denial_record.denial.reference == "DN001\/0003" assert denial_record.name == "Test1 case" @@ -133,7 +148,7 @@ def test_populate_denials_with_existing_matching_records(mock_get_file, mock_del call_command("ingest_denials", "json_file") - assert DenialEntity.objects.all().count() == 3 + assert DenialEntity.objects.all().count() == 4 @pytest.mark.django_db diff --git a/api/external_data/serializers.py b/api/external_data/serializers.py index f85fc9ea1a..305ad2f875 100644 --- a/api/external_data/serializers.py +++ b/api/external_data/serializers.py @@ -35,7 +35,6 @@ class Meta: class DenialEntitySerializer(serializers.ModelSerializer): - entity_type = serializers.SerializerMethodField() regime_reg_ref = serializers.CharField(source="denial.regime_reg_ref", required=False) reference = serializers.CharField(source="denial.reference", required=False) item_list_codes = serializers.CharField(source="denial.item_list_codes", required=False) @@ -96,9 +95,6 @@ def update(self, instance, validated_data): instance.denial.save() return instance - def get_entity_type(self, obj): - return get_denial_entity_type(obj.data) - class DenialFromCSVFileSerializer(serializers.Serializer): @@ -109,6 +105,7 @@ class DenialFromCSVFileSerializer(serializers.Serializer): "address", "country", "spire_entity_id", + "entity_type", ] required_headers_denial = [ @@ -178,6 +175,7 @@ def validate_csv_file(self, value): denial_entity_lookup_fields = { "name": serializer.validated_data["name"], "address": serializer.validated_data["address"], + "entity_type": serializer.validated_data["entity_type"], } # Link the validated DenialEntity data with the Denial @@ -186,10 +184,6 @@ def validate_csv_file(self, value): denial_entity, is_denial_entity_created = models.DenialEntity.objects.update_or_create( defaults=serializer.validated_data, denial=denial, **denial_entity_lookup_fields ) - # Store any extra columns in csv not already captured in data dict - denial_entity.data = {field: row[field] for field in row if field not in self.required_headers} - denial_entity.entity_type = get_denial_entity_type(denial_entity.data) - denial_entity.save() if is_denial_entity_created: logging_counts["denial_entity"]["created"] += 1 diff --git a/api/external_data/tests/denial_valid.csv b/api/external_data/tests/denial_valid.csv index c021b828f3..12d4f8b198 100644 --- a/api/external_data/tests/denial_valid.csv +++ b/api/external_data/tests/denial_valid.csv @@ -1,5 +1,5 @@ -reference,regime_reg_ref,name,address,notifying_government,country,item_list_codes,item_description,end_use,reason_for_refusal,spire_entity_id -DN2000/0000,AB-CD-EF-000,Organisation Name,"1000 Street Name, City Name",Country Name,Country Name,0A00100,Medium Size Widget,Used in industry,Risk of outcome,123 -DN2000/0010,AB-CD-EF-300,Organisation Name 3,"2001 Street Name, City Name 3",Country Name 3,Country Name 3,0A00201,Unspecified Size Widget,Used in other industry,Risk of outcome 3,125 -DN2010/0001,AB-XY-EF-900,The Widget Company,"2 Example Road, Example City",Example Country,Country Name X,"catch all",Extra Large Size Widget,Used in unknown industry,Risk of outcome 4,126 -DN3000/0000,AB-CD-EF-100,Organisation Name XYZ,"2000 Street Name, City Name 2",Country Name 2,Country Name 2,0A00200,Large Size Widget,Used in other industry,Risk of outcome 2,124 +reference,regime_reg_ref,name,address,notifying_government,country,item_list_codes,item_description,end_use,reason_for_refusal,spire_entity_id,entity_type +DN2000/0000,AB-CD-EF-000,Organisation Name,"1000 Street Name, City Name",Country Name,Country Name,0A00100,Medium Size Widget,Used in industry,Risk of outcome,123,end_user +DN2000/0010,AB-CD-EF-300,Organisation Name 3,"2001 Street Name, City Name 3",Country Name 3,Country Name 3,0A00201,Unspecified Size Widget,Used in other industry,Risk of outcome 3,125,consignee +DN2010/0001,AB-XY-EF-900,The Widget Company,"2 Example Road, Example City",Example Country,Country Name X,"catch all",Extra Large Size Widget,Used in unknown industry,Risk of outcome 4,126,end_user +DN3000/0000,AB-CD-EF-100,Organisation Name XYZ,"2000 Street Name, City Name 2",Country Name 2,Country Name 2,0A00200,Large Size Widget,Used in other industry,Risk of outcome 2,124,third_party diff --git a/api/external_data/tests/test_views.py b/api/external_data/tests/test_views.py index d474df1c45..2a5ec08ba0 100644 --- a/api/external_data/tests/test_views.py +++ b/api/external_data/tests/test_views.py @@ -36,7 +36,7 @@ def test_create_success(self): self.assertEqual( list( models.DenialEntity.objects.values( - *serializers.DenialFromCSVFileSerializer.required_headers_denial_entity, "data" + *serializers.DenialFromCSVFileSerializer.required_headers_denial_entity ) ), [ @@ -45,28 +45,28 @@ def test_create_success(self): "address": "1000 Street Name, City Name", "country": "Country Name", "spire_entity_id": 123, - "data": {}, + "entity_type": "end_user", }, { "name": "Organisation Name 3", "address": "2001 Street Name, City Name 3", "country": "Country Name 3", "spire_entity_id": 125, - "data": {}, + "entity_type": "consignee", }, { "name": "The Widget Company", "address": "2 Example Road, Example City", "country": "Country Name X", "spire_entity_id": 126, - "data": {}, + "entity_type": "end_user", }, { "name": "Organisation Name XYZ", "address": "2000 Street Name, City Name 2", "country": "Country Name 2", "spire_entity_id": 124, - "data": {}, + "entity_type": "third_party", }, ], ) @@ -125,11 +125,11 @@ def test_create_error_missing_required_headers(self): def test_create_and_set_entity_type(self): url = reverse("external_data:denial-list") content = """ - reference,regime_reg_ref,name,address,notifying_government,country,item_list_codes,item_description,end_use,reason_for_refusal,spire_entity_id,end_user_flag,consignee_flag,other_role - DN2000/0000,AB-CD-EF-000,Organisation Name,"1000 Street Name, City Name",Country Name,Country Name,0A00100,Medium Size Widget,Used in industry,Risk of outcome,123,TRUE,FALSE, - DN2000/0010,AB-CD-EF-300,Organisation Name 3,"2001 Street Name, City Name 3",Country Name 3,Country Name 3,0A00201,Unspecified Size Widget,Used in other industry,Risk of outcome 3,125,FALSE,TRUE, - DN2010/0001,AB-XY-EF-900,The Widget Company,"2 Example Road, Example City",Example Country,Country Name X,"catch all",Extra Large Size Widget,Used in unknown industry,Risk of outcome 4,126,FALSE,FALSE,other - DN3000/0000,AB-CD-EF-100,Organisation Name XYZ,"2000 Street Name, City Name 2",Country Name 2,Country Name 2,0A00200,Large Size Widget,Used in other industry,Risk of outcome 2,124,TRUE,TRUE, + reference,regime_reg_ref,name,address,notifying_government,country,item_list_codes,item_description,end_use,reason_for_refusal,spire_entity_id,entity_type + DN2000/0000,AB-CD-EF-000,Organisation Name,"1000 Street Name, City Name",Country Name,Country Name,0A00100,Medium Size Widget,Used in industry,Risk of outcome,123,end_user + DN2000/0010,AB-CD-EF-300,Organisation Name 3,"2001 Street Name, City Name 3",Country Name 3,Country Name 3,0A00201,Unspecified Size Widget,Used in other industry,Risk of outcome 3,125,consignee + DN2010/0001,AB-XY-EF-900,The Widget Company,"2 Example Road, Example City",Example Country,Country Name X,"catch all",Extra Large Size Widget,Used in unknown industry,Risk of outcome 4,126,third_party + DN3000/0000,AB-CD-EF-100,Organisation Name XYZ,"2000 Street Name, City Name 2",Country Name 2,Country Name 2,0A00200,Large Size Widget,Used in other industry,Risk of outcome 2,124,end_user """ response = self.client.post(url, {"csv_file": content}, **self.gov_headers) @@ -138,69 +138,37 @@ def test_create_and_set_entity_type(self): self.assertEqual( list( models.DenialEntity.objects.values( - *serializers.DenialFromCSVFileSerializer.required_headers, "data", "entity_type" + *serializers.DenialFromCSVFileSerializer.required_headers_denial_entity ) ), [ { - "reference": "DN2000/0000", - "regime_reg_ref": "AB-CD-EF-000", "name": "Organisation Name", "address": "1000 Street Name, City Name", - "notifying_government": "Country Name", "country": "Country Name", - "item_list_codes": "0A00100", - "item_description": "Medium Size Widget", - "end_use": "Used in industry", - "reason_for_refusal": "Risk of outcome", "spire_entity_id": 123, - "data": {"other_role": "", "end_user_flag": True, "consignee_flag": False}, - "entity_type": "End-user", + "entity_type": "end_user", }, { - "reference": "DN2000/0010", - "regime_reg_ref": "AB-CD-EF-300", "name": "Organisation Name 3", "address": "2001 Street Name, City Name 3", - "notifying_government": "Country Name 3", "country": "Country Name 3", - "item_list_codes": "0A00201", - "item_description": "Unspecified Size Widget", - "end_use": "Used in other industry", - "reason_for_refusal": "Risk of outcome 3", "spire_entity_id": 125, - "data": {"other_role": "", "end_user_flag": False, "consignee_flag": True}, - "entity_type": "Consignee", + "entity_type": "consignee", }, { - "reference": "DN2010/0001", - "regime_reg_ref": "AB-XY-EF-900", "name": "The Widget Company", "address": "2 Example Road, Example City", - "notifying_government": "Example Country", "country": "Country Name X", - "item_list_codes": "catch all", - "item_description": "Extra Large Size Widget", - "end_use": "Used in unknown industry", - "reason_for_refusal": "Risk of outcome 4", "spire_entity_id": 126, - "data": {"other_role": "other", "end_user_flag": False, "consignee_flag": False}, - "entity_type": "Third-party", + "entity_type": "third_party", }, { - "reference": "DN3000/0000", - "regime_reg_ref": "AB-CD-EF-100", "name": "Organisation Name XYZ", "address": "2000 Street Name, City Name 2", - "notifying_government": "Country Name 2", "country": "Country Name 2", - "item_list_codes": "0A00200", - "item_description": "Large Size Widget", - "end_use": "Used in other industry", - "reason_for_refusal": "Risk of outcome 2", "spire_entity_id": 124, - "data": {"other_role": "", "end_user_flag": True, "consignee_flag": True}, - "entity_type": "End-user", + "entity_type": "end_user", }, ], ) @@ -208,8 +176,8 @@ def test_create_and_set_entity_type(self): def test_update_success(self): url = reverse("external_data:denial-list") content = """ - reference,regime_reg_ref,name,address,notifying_government,country,item_list_codes,item_description,end_use,reason_for_refusal,spire_entity_id - DN2000/0000,AB-CD-EF-000,Organisation Name,"1000 Street Name, City Name",Country Name,Country Name,0A00100,Medium Size Widget,Used in industry,Risk of outcome,123 + reference,regime_reg_ref,name,address,notifying_government,country,item_list_codes,item_description,end_use,reason_for_refusal,spire_entity_id,entity_type + DN2000/0000,AB-CD-EF-000,Organisation Name,"1000 Street Name, City Name",Country Name,Country Name,0A00100,Medium Size Widget,Used in industry,Risk of outcome,123,end_user """ response = self.client.post(url, {"csv_file": content}, **self.gov_headers) @@ -234,7 +202,7 @@ def test_update_success(self): self.assertEqual( list( models.DenialEntity.objects.values( - *serializers.DenialFromCSVFileSerializer.required_headers_denial_entity, "data" + *serializers.DenialFromCSVFileSerializer.required_headers_denial_entity ) ), [ @@ -243,13 +211,13 @@ def test_update_success(self): "address": "1000 Street Name, City Name", "country": "Country Name", "spire_entity_id": 123, - "data": {}, + "entity_type": "end_user", }, ], ) updated_content = """ - reference,regime_reg_ref,name,address,notifying_government,country,item_list_codes,item_description,end_use,reason_for_refusal,spire_entity_id - DN2000/0000,AB-CD-EF-000,Organisation Name,"1000 Street Name, City Name",Country Name 2,Country Name 2,0A00200,Medium Size Widget 2, Used in industry 2,Risk of outcome 2,124 + reference,regime_reg_ref,name,address,notifying_government,country,item_list_codes,item_description,end_use,reason_for_refusal,spire_entity_id,entity_type + DN2000/0000,AB-CD-EF-000,Organisation Name,"1000 Street Name, City Name",Country Name 2,Country Name 2,0A00200,Medium Size Widget 2, Used in industry 2,Risk of outcome 2,124,end_user """ response = self.client.post(url, {"csv_file": updated_content}, **self.gov_headers) self.assertEqual(response.status_code, 201) @@ -272,7 +240,7 @@ def test_update_success(self): self.assertEqual( list( models.DenialEntity.objects.values( - *serializers.DenialFromCSVFileSerializer.required_headers_denial_entity, "data" + *serializers.DenialFromCSVFileSerializer.required_headers_denial_entity ) ), [ @@ -281,15 +249,15 @@ def test_update_success(self): "address": "1000 Street Name, City Name", "country": "Country Name 2", "spire_entity_id": 124, - "data": {}, + "entity_type": "end_user", } ], ) def test_create_error_serializer_errors(self): url = reverse("external_data:denial-list") - content = """reference,regime_reg_ref,name,address,notifying_government,country,item_list_codes,item_description,end_use,reason_for_refusal,spire_entity_id - ,AB-CD-EF-000,Organisation Name,"1000 Street Name, City Name",Country Name,Country Name,0A00100,Medium Size Widget,Used in industry,Risk of outcome,123 + content = """reference,regime_reg_ref,name,address,notifying_government,country,item_list_codes,item_description,end_use,reason_for_refusal,spire_entity_id,entity_type + ,AB-CD-EF-000,Organisation Name,"1000 Street Name, City Name",Country Name,Country Name,0A00100,Medium Size Widget,Used in industry,Risk of outcome,123,end_user """ response = self.client.post(url, {"csv_file": content}, **self.gov_headers) self.assertEqual(response.status_code, 400) From c9a2fa40b406936055d2427b0f2c888fe06d18ec Mon Sep 17 00:00:00 2001 From: Henry Cooksley Date: Tue, 30 Apr 2024 14:03:55 +0100 Subject: [PATCH 16/19] Update lite routing sha --- lite_routing | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lite_routing b/lite_routing index 92bfcb2c8f..d7de7afd05 160000 --- a/lite_routing +++ b/lite_routing @@ -1 +1 @@ -Subproject commit 92bfcb2c8f3acbfd72ed9f668d2daad374f9bbef +Subproject commit d7de7afd05ecb50db97426dca99889a057d16453 From 04d571e37a4d09e03862f347a158ae48b81a31dc Mon Sep 17 00:00:00 2001 From: Gurdeep Atwal Date: Wed, 1 May 2024 13:45:57 +0100 Subject: [PATCH 17/19] hotfix --- .../migrations/0024_denials_data_migration.py | 79 +++++++-------- .../tests/test_0024_denials_data_migration.py | 95 ++++++++++++------- 2 files changed, 102 insertions(+), 72 deletions(-) diff --git a/api/external_data/migrations/0024_denials_data_migration.py b/api/external_data/migrations/0024_denials_data_migration.py index 4a7bfafec6..5f18ea5f22 100644 --- a/api/external_data/migrations/0024_denials_data_migration.py +++ b/api/external_data/migrations/0024_denials_data_migration.py @@ -1,60 +1,63 @@ # Generated by Django 4.2.10 on 2024-04-02 14:59 import logging -from django.db import migrations -from django.forms.models import model_to_dict + +from django.db import IntegrityError, migrations, transaction from django.db.models import Q -from django.db import IntegrityError -from django.db import transaction +from django.forms.models import model_to_dict + log = logging.getLogger(__name__) + required_fields = [ - "reference", - "regime_reg_ref", - "notifying_government", - "item_list_codes", - "item_description", - "end_use", - "is_revoked", - "is_revoked_comment", - "reason_for_refusal", + "reference", + "regime_reg_ref", + "notifying_government", + "item_list_codes", + "item_description", + "end_use", + "is_revoked", + "is_revoked_comment", + "reason_for_refusal", ] + def denials_data_migration(apps, schema_editor): DenialEntity = apps.get_model("external_data", "DenialEntity") Denial = apps.get_model("external_data", "Denial") # There are a handfull (10) of regime_reg_refs that are null which was in the initial load - # Assumption here is that they can be deleted since it's erroneous data as we now know - # regime_reg_ref is considered a unique DN record. + # Assumption here is that they can be deleted since it's erroneous data as we now know + # regime_reg_ref is considered a unique DN record. - total_null_regime_reg_ref = DenialEntity.objects.filter(Q(regime_reg_ref__isnull=True) | Q(regime_reg_ref='')) + total_null_regime_reg_ref = DenialEntity.objects.filter(Q(regime_reg_ref__isnull=True) | Q(regime_reg_ref="")) log.info( - "Delete null regime_reg_ref total -> %s", - total_null_regime_reg_ref.count(), + "Delete null regime_reg_ref total -> %s", + total_null_regime_reg_ref.count(), ) total_null_regime_reg_ref.delete() - duplicate_denial_errors = [] - with transaction.atomic(): - sid = transaction.savepoint() - for denial_entity in DenialEntity.objects.all(): - try: - denial_entity_dict = {key:value for (key,value) in model_to_dict(denial_entity).items() if key in required_fields} - denial , _ = Denial.objects.get_or_create(**denial_entity_dict) - denial_entity.denial = denial - denial_entity.save() - except IntegrityError as e: - duplicate_denial_errors.append(denial_entity.regime_reg_ref) - - if duplicate_denial_errors: - log.info( - "There are the following duplicate denials in the database rolling back this migration: -> %s", - duplicate_denial_errors, - ) - transaction.savepoint_rollback(sid) - else: - transaction.savepoint_commit(sid) + + denials_to_create = [] + denials_entitiy_to_update = [] + denials_dict_map = {} + for denial_entity in DenialEntity.objects.all(): + denial_entity_dict = { + key: value for (key, value) in model_to_dict(denial_entity).items() if key in required_fields + } + denials_dict_map[denial_entity_dict["regime_reg_ref"]] = denial_entity_dict + + for denials_dict_map_item in denials_dict_map.values(): + denial = Denial(**denials_dict_map_item) + denials_to_create.append(denial) + + Denial.objects.bulk_create(denials_to_create) + + for denial_entity in DenialEntity.objects.all(): + denial_entity.denial = Denial.objects.get(regime_reg_ref=denial_entity.regime_reg_ref) + denials_entitiy_to_update.append(denial_entity) + + DenialEntity.objects.bulk_update(denials_entitiy_to_update, ["denial"]) class Migration(migrations.Migration): dependencies = [ diff --git a/api/external_data/migrations/tests/test_0024_denials_data_migration.py b/api/external_data/migrations/tests/test_0024_denials_data_migration.py index a17be6567c..e1bfe8e0ba 100644 --- a/api/external_data/migrations/tests/test_0024_denials_data_migration.py +++ b/api/external_data/migrations/tests/test_0024_denials_data_migration.py @@ -4,11 +4,65 @@ test_data = [ -{"reference":"DN2010\/0057","regime_reg_ref":"reg.123.123","name":"name 1","address":"address 1","notifying_government":"UK","country":"UK","item_list_codes":"all","item_description":"desc a","end_use":"use 1","reason_for_refusal":"a"}, -{"reference":"DN2010\/0057","regime_reg_ref":"reg.123.1234","name":"name 2","address":"address 2","notifying_government":"UK","country":"UK","item_list_codes":"all","item_description":"desc a","end_use":"use 1","reason_for_refusal":"a"}, -{"reference":"DN2010\/0057","regime_reg_ref":"reg.123.1234","name":"name 3","address":"address 3","notifying_government":"UK","country":"UK","item_list_codes":"all","item_description":"desc a","end_use":"use 1","reason_for_refusal":"a"}, -{"reference":"DN2010\/0057","regime_reg_ref":"reg.123.1234","name":"name 4","address":"address 4","notifying_government":"UK","country":"UK","item_list_codes":"all","item_description":"desc a","end_use":"use 1","reason_for_refusal":"a"}, -{"reference":"DN2010\/0057","name":"bad record","address":"bad record","notifying_government":"UK","country":"bad","item_list_codes":"all","item_description":"bad","end_use":"bad","reason_for_refusal":"bad "} + { + "reference": "DN2010\/0057", + "regime_reg_ref": "reg.123.123", + "name": "name 1", + "address": "address 1", + "notifying_government": "UK", + "country": "UK", + "item_list_codes": "all", + "item_description": "desc a", + "end_use": "use 1", + "reason_for_refusal": "a", + }, + { + "reference": "DN2010\/0057", + "regime_reg_ref": "reg.123.1234", + "name": "name 2", + "address": "address 2", + "notifying_government": "UK", + "country": "UK", + "item_list_codes": "all", + "item_description": "desc a", + "end_use": "use 1", + "reason_for_refusal": "a", + }, + { + "reference": "DN2010\/0057", + "regime_reg_ref": "reg.123.1234", + "name": "name 3", + "address": "address 3", + "notifying_government": "UK", + "country": "UK", + "item_list_codes": "all", + "item_description": "desc a", + "end_use": "use 1", + "reason_for_refusal": "a", + }, + { + "reference": "DN2010\/0057", + "regime_reg_ref": "reg.123.1234", + "name": "name 4", + "address": "address 4", + "notifying_government": "UK", + "country": "UK", + "item_list_codes": "all", + "item_description": "desc a", + "end_use": "use 1", + "reason_for_refusal": "a", + }, + { + "reference": "DN2010\/0057", + "name": "bad record", + "address": "bad record", + "notifying_government": "UK", + "country": "bad", + "item_list_codes": "all", + "item_description": "bad", + "end_use": "bad", + "reason_for_refusal": "bad ", + }, ] @@ -18,14 +72,10 @@ class TestDenialDataMigration(MigratorTestCase): migrate_from = ("external_data", "0023_set_denial_entity_type") migrate_to = ("external_data", "0024_denials_data_migration") - def prepare(self): DenialEntity = self.old_state.apps.get_model("external_data", "DenialEntity") for row in test_data: - DenialEntity.objects.create(**row) - - - + DenialEntity.objects.create(**row) def test_0023_denials_data_migration(self): DenialEntity = self.new_state.apps.get_model("external_data", "DenialEntity") @@ -33,27 +83,4 @@ def test_0023_denials_data_migration(self): self.assertEqual(DenialEntity.objects.all().count(), 4) self.assertEqual(Denial.objects.all().count(), 2) - self.assertEqual(Denial.objects.get(regime_reg_ref='reg.123.1234').denial_entity.count(), 3) - - -@pytest.mark.django_db() -class TestDenialDataDuplicatesMigration(MigratorTestCase): - - migrate_from = ("external_data", "0023_set_denial_entity_type") - migrate_to = ("external_data", "0024_denials_data_migration") - - - def prepare(self): - DenialEntity = self.old_state.apps.get_model("external_data", "DenialEntity") - for row in test_data: - DenialEntity.objects.create(**row) - test_data[0]["end_use"] = "end_use b" - DenialEntity.objects.create(**test_data[0]) - - - - def test_0024_denials_data_migration_duplicates(self): - DenialEntity = self.new_state.apps.get_model("external_data", "DenialEntity") - Denial = self.new_state.apps.get_model("external_data", "Denial") - self.assertEqual(DenialEntity.objects.all().count(), 5) - self.assertEqual(Denial.objects.all().count(), 0) + self.assertEqual(Denial.objects.get(regime_reg_ref="reg.123.1234").denial_entity.count(), 3) From 63872c4de0386c923e19887d3e57ed423f302342 Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Thu, 25 Apr 2024 14:30:19 +0100 Subject: [PATCH 18/19] Ensure team is taken into account when filtering previous advice for deletion This makes sure that when a user changes team we don't delete advice that they gave for that other team --- api/cases/models.py | 1 + api/cases/tests/test_edit_advice.py | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/api/cases/models.py b/api/cases/models.py index db4c56a583..b19f00ae0c 100644 --- a/api/cases/models.py +++ b/api/cases/models.py @@ -528,6 +528,7 @@ def save(self, *args, **kwargs): case=self.case, good=self.good, user=self.user, + team=self.user.team, level=AdviceLevel.USER, goods_type=self.goods_type, country=self.country, diff --git a/api/cases/tests/test_edit_advice.py b/api/cases/tests/test_edit_advice.py index fa66c5ab6b..b6ba438db5 100644 --- a/api/cases/tests/test_edit_advice.py +++ b/api/cases/tests/test_edit_advice.py @@ -48,6 +48,28 @@ def test_edit_standard_case_advice_twice_only_shows_once(self): # Assert that there's only one piece of advice self.assertEqual(Advice.objects.count(), 1) + def test_edit_standard_case_advice_same_user_on_different_teams_saves_for_each_team(self): + """ + Tests that a gov user can create two pieces of advice on the same + case item (be that a good or destination) as long as they change team + """ + data = { + "type": AdviceType.APPROVE, + "text": "I Am Easy to Find", + "note": "I Am Easy to Find", + "country": "GB", + "level": AdviceLevel.USER, + } + + self.client.post(self.url, **self.gov_headers, data=[data]) + + self.gov_user.team = Team.objects.exclude(pk=self.gov_user.team.pk).first() + self.gov_user.save() + self.assertIsNotNone(self.gov_user.team) + self.client.post(self.url, **self.gov_headers, data=[data]) + + self.assertEqual(Advice.objects.count(), 2) + def test_edit_standard_case_advice_updates_denial_reasons(self): data = { "type": AdviceType.REFUSE, From 43442c230fd9ad4a87dfb1c516e193c2d5eb17bc Mon Sep 17 00:00:00 2001 From: Henry Cooksley Date: Thu, 2 May 2024 15:17:44 +0100 Subject: [PATCH 19/19] Update lite routing sha --- lite_routing | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lite_routing b/lite_routing index d7de7afd05..c9733f331f 160000 --- a/lite_routing +++ b/lite_routing @@ -1 +1 @@ -Subproject commit d7de7afd05ecb50db97426dca99889a057d16453 +Subproject commit c9733f331fdacaba247b6f28226fbac1b5472c3a