From 7b629366dbbee1fe3eff449d45531a9ae39933a3 Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Fri, 6 Dec 2024 12:42:23 +0000 Subject: [PATCH 01/10] Add additional dependences to mgirations This is to ensure that the ordering of migrations are correct and that data migrations have access to all of the model fields they require --- api/cases/migrations/0073_licencedecision_denial_reasons.py | 2 ++ api/organisations/migrations/0009_site_records_located_at.py | 1 + 2 files changed, 3 insertions(+) diff --git a/api/cases/migrations/0073_licencedecision_denial_reasons.py b/api/cases/migrations/0073_licencedecision_denial_reasons.py index f04f1800dd..f1dc2b2150 100644 --- a/api/cases/migrations/0073_licencedecision_denial_reasons.py +++ b/api/cases/migrations/0073_licencedecision_denial_reasons.py @@ -8,6 +8,8 @@ class Migration(migrations.Migration): dependencies = [ ("denial_reasons", "0006_populate_uuid_field"), ("cases", "0072_alter_decision_populate_issued_on_appeal"), + ("countries", "0006_update_trading_country_code"), + ("organisations", "0014_alter_organisation_status"), ] operations = [ diff --git a/api/organisations/migrations/0009_site_records_located_at.py b/api/organisations/migrations/0009_site_records_located_at.py index 010abf2640..991c8430cb 100644 --- a/api/organisations/migrations/0009_site_records_located_at.py +++ b/api/organisations/migrations/0009_site_records_located_at.py @@ -47,6 +47,7 @@ class Migration(migrations.Migration): ("organisations", "0008_auto_20200601_0814"), ("compliance", "0002_compliancesitecase"), ("statuses", "0003_auto_20200318_1730"), + ("licences", "0013_auto_20200709_1520"), ] operations = [ From 140afe06cbb55ad8e3f3ab4e7d0d919ec21cd3b5 Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Fri, 6 Dec 2024 12:47:53 +0000 Subject: [PATCH 02/10] Update how status may be set when creating standard application from a factory --- api/applications/tests/factories.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/api/applications/tests/factories.py b/api/applications/tests/factories.py index 43423bafb3..acfc2d0f86 100644 --- a/api/applications/tests/factories.py +++ b/api/applications/tests/factories.py @@ -18,7 +18,6 @@ from api.cases.models import Advice from api.external_data.models import Denial, DenialEntity, SanctionMatch from api.documents.tests.factories import DocumentFactory -from api.staticdata.statuses.models import CaseStatus from api.goods.tests.factories import GoodFactory from api.organisations.tests.factories import OrganisationFactory, SiteFactory, ExternalLocationFactory from api.parties.tests.factories import ConsigneeFactory, EndUserFactory, PartyFactory, ThirdPartyFactory @@ -61,9 +60,8 @@ class Meta: @classmethod def _create(cls, model_class, *args, **kwargs): obj = model_class(*args, **kwargs) - obj.status = get_case_status_by_status(CaseStatusEnum.SUBMITTED) - if "status" in kwargs and isinstance(kwargs["status"], CaseStatus): - obj.status = kwargs["status"] + if "status" not in kwargs: + obj.status = get_case_status_by_status(CaseStatusEnum.SUBMITTED) obj.save() return obj From ef43a3f8ce6533fe1040d6257830a1b5e30b4773 Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Fri, 6 Dec 2024 12:48:35 +0000 Subject: [PATCH 03/10] Remove migration test that has now been run on all environments --- ...t_0070_attach_licence_to_licence_decision.py | 17 ----------------- 1 file changed, 17 deletions(-) delete mode 100644 api/cases/migrations/tests/test_0070_attach_licence_to_licence_decision.py diff --git a/api/cases/migrations/tests/test_0070_attach_licence_to_licence_decision.py b/api/cases/migrations/tests/test_0070_attach_licence_to_licence_decision.py deleted file mode 100644 index 82b4b0077f..0000000000 --- a/api/cases/migrations/tests/test_0070_attach_licence_to_licence_decision.py +++ /dev/null @@ -1,17 +0,0 @@ -import pytest - -from api.cases.enums import LicenceDecisionType - -INITIAL_MIGRATION = "0069_licencedecision_excluded_from_statistics_reason" -MIGRATION_UNDER_TEST = "0070_attach_licence_to_licence_decision" - - -@pytest.mark.django_db() -def test_attach_licence_to_licence_decisions(migrator): - - old_state = migrator.apply_initial_migration(("cases", INITIAL_MIGRATION)) - new_state = migrator.apply_tested_migration(("cases", MIGRATION_UNDER_TEST)) - - LicenceDecision = new_state.apps.get_model("cases", "LicenceDecision") - - assert LicenceDecision.objects.filter(decision=LicenceDecisionType.ISSUED, licence__isnull=True).count() == 0 From e1f8a27d28cf6de414a3af71fbef8cab8427a5bd Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Fri, 6 Dec 2024 12:50:57 +0000 Subject: [PATCH 04/10] Add function to rebuild factories from frozen migration models --- test_helpers/factories.py | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 test_helpers/factories.py diff --git a/test_helpers/factories.py b/test_helpers/factories.py new file mode 100644 index 0000000000..c0181053f0 --- /dev/null +++ b/test_helpers/factories.py @@ -0,0 +1,32 @@ +import factory + + +def get_model_for_factory(factory_class, apps): + model_meta = factory_class._meta.model._meta + + return apps.get_model(model_meta.app_label, model_meta.object_name) + + +def fullname(cls): + return f"{cls.__module__}.{cls.__name__}" + + +def generate_factory(apps, factory_class, **defaults): + for field_name, field_value in factory_class._meta.declarations.items(): + if field_name in defaults: + continue + if not isinstance(field_value, factory.declarations.SubFactory): + continue + generated_factory = generate_factory( + apps, + field_value.get_factory(), + **field_value.defaults, + ) + defaults[field_name] = factory.declarations.SubFactory(generated_factory) + + updated_factory = factory.make_factory( + get_model_for_factory(factory_class, apps), + **defaults, + FACTORY_CLASS=factory_class, + ) + return updated_factory From a8544101b289f04d81f0a8c34e018218ae2b9ea7 Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Tue, 10 Dec 2024 11:55:00 +0000 Subject: [PATCH 05/10] Add migration to update licence decision refusals with their requisite denial reasons --- ..._update_licence_decision_denial_reasons.py | 69 +++++++++++ ..._update_licence_decision_denial_reasons.py | 107 ++++++++++++++++++ api/cases/tests/factories.py | 6 + 3 files changed, 182 insertions(+) create mode 100644 api/cases/migrations/0074_update_licence_decision_denial_reasons.py create mode 100644 api/cases/migrations/tests/test_0074_update_licence_decision_denial_reasons.py diff --git a/api/cases/migrations/0074_update_licence_decision_denial_reasons.py b/api/cases/migrations/0074_update_licence_decision_denial_reasons.py new file mode 100644 index 0000000000..ae5a57f4b9 --- /dev/null +++ b/api/cases/migrations/0074_update_licence_decision_denial_reasons.py @@ -0,0 +1,69 @@ +# Generated by Django 4.2.16 on 2024-12-03 14:48 + +from django.db import migrations + +from api.audit_trail.enums import AuditType +from api.cases.enums import AdviceLevel + + +LICENSING_UNIT_ID = "58e77e47-42c8-499f-a58d-94f94541f8c6" + + +def update_licencedecision_denial_reasons(apps, schema_editor): + LicenceDecision = apps.get_model("cases", "LicenceDecision") + Advice = apps.get_model("cases", "Advice") + + denial_reasons = ( + Advice.objects.filter( + case__licence_decisions__decision="refused", + level=AdviceLevel.FINAL, + team_id=LICENSING_UNIT_ID, # Just care about LU advice + ) + .only("denial_reasons__id", "case__licence_decisions__id") + .exclude(denial_reasons__id__isnull=True) # This removes refusals without any criteria + .values_list("denial_reasons__id", "case__licence_decisions__id") + .order_by() # We need to remove the order_by to make sure the distinct works + .distinct() + ) + + for denial_reason, licence_decision_id in denial_reasons: + licence_decision = LicenceDecision.objects.get(pk=licence_decision_id) + licence_decision.denial_reasons.add(denial_reason) + + updated_cases = set() + for denial_reason, licence_decision_id in denial_reasons: + licence_decision = LicenceDecision.objects.get(pk=licence_decision_id) + licence_decision.denial_reasons.add(denial_reason) + updated_cases.add(licence_decision.case.pk) + + Audit = apps.get_model("audit_trail", "Audit") + Case = apps.get_model("cases", "Case") + ContentType = apps.get_model("contenttypes", "ContentType") + case_content_type = ContentType.objects.get_for_model(Case) + + refusal_criteria_audits = Audit.objects.exclude( + target_content_type_id=case_content_type.pk, + target_object_id__in=updated_cases, + ).filter(verb=AuditType.CREATE_REFUSAL_CRITERIA) + for audit in refusal_criteria_audits: + case = Case.objects.get(pk=audit.target_object_id) + refusal_licence_decisions = case.licence_decisions.filter(decision="refused") + if not refusal_licence_decisions.exists(): + continue + for licence_decision in refusal_licence_decisions: + denial_reasons = audit.payload["additional_text"].replace(".", "").replace(" ", "").split(",") + licence_decision.denial_reasons.set(denial_reasons) + + +class Migration(migrations.Migration): + + dependencies = [ + ("cases", "0073_licencedecision_denial_reasons"), + ] + + operations = [ + migrations.RunPython( + update_licencedecision_denial_reasons, + migrations.RunPython.noop, + ), + ] diff --git a/api/cases/migrations/tests/test_0074_update_licence_decision_denial_reasons.py b/api/cases/migrations/tests/test_0074_update_licence_decision_denial_reasons.py new file mode 100644 index 0000000000..5138843223 --- /dev/null +++ b/api/cases/migrations/tests/test_0074_update_licence_decision_denial_reasons.py @@ -0,0 +1,107 @@ +import pytest + +from api.applications.tests.factories import StandardApplicationFactory +from api.audit_trail.enums import AuditType +from api.audit_trail.tests.factories import AuditFactory +from api.cases.enums import ( + AdviceType, + LicenceDecisionType, +) +from api.cases.tests.factories import ( + FinalAdviceFactory, + LicenceDecisionFactory, + TeamAdviceFactory, + UserAdviceFactory, +) +from api.users.tests.factories import ( + GovUserFactory, + RoleFactory, +) + + +@pytest.mark.django_db() +def test_attach_licence_to_licence_decisions(migrator): + migrator.apply_initial_migration(("cases", "0073_licencedecision_denial_reasons")) + + LICENSING_UNIT_ID = "58e77e47-42c8-499f-a58d-94f94541f8c6" + FCDO_ID = "67b9a4a3-6f3d-4511-8a19-23ccff221a74" + + RoleFactory(id="00000000-0000-0000-0000-000000000001") + lu_user = GovUserFactory(team_id=LICENSING_UNIT_ID) + fcdo_user = GovUserFactory(team_id=FCDO_ID) + + refused_application = StandardApplicationFactory() + refused_licence_decision = LicenceDecisionFactory( + case=refused_application, + decision=LicenceDecisionType.REFUSED, + ) + UserAdviceFactory( + case=refused_application, + denial_reasons=["1"], + team_id=FCDO_ID, + type=AdviceType.REFUSE, + user=fcdo_user, + ) + UserAdviceFactory( + case=refused_application, + denial_reasons=["2"], + team_id=LICENSING_UNIT_ID, + type=AdviceType.REFUSE, + user=lu_user, + ) + TeamAdviceFactory( + case=refused_application, + denial_reasons=["3"], + team_id=FCDO_ID, + type=AdviceType.REFUSE, + user=fcdo_user, + ) + TeamAdviceFactory( + case=refused_application, + denial_reasons=["4"], + team_id=LICENSING_UNIT_ID, + type=AdviceType.REFUSE, + user=lu_user, + ) + FinalAdviceFactory( + case=refused_application, + denial_reasons=["5"], + team_id=LICENSING_UNIT_ID, + type=AdviceType.REFUSE, + user=lu_user, + ) + assert list(refused_licence_decision.denial_reasons.values_list("id", flat=True)) == [] + + migrator.apply_tested_migration(("cases", "0074_update_licence_decision_denial_reasons")) + refused_licence_decision.refresh_from_db() + assert list(refused_licence_decision.denial_reasons.values_list("id", flat=True)) == ["5"] + + +@pytest.mark.django_db() +def test_attach_licence_to_licence_decisions_without_denial_reasons_on_advice(migrator): + old_state = migrator.apply_initial_migration(("cases", "0073_licencedecision_denial_reasons")) + + refused_application = StandardApplicationFactory() + refused_licence_decision = LicenceDecisionFactory( + case=refused_application, + decision=LicenceDecisionType.REFUSED, + ) + assert list(refused_licence_decision.denial_reasons.values_list("id", flat=True)) == [] + assert not refused_application.advice.exists() + + Case = old_state.apps.get_model("cases", "Case") + ContentType = old_state.apps.get_model("contenttypes", "ContentType") + case_content_type = ContentType.objects.get_for_model(Case) + + AuditFactory( + payload={ + "additional_text": "1, 5, 7.", + }, + target_content_type_id=case_content_type.pk, + target_object_id=refused_application.pk, + verb=AuditType.CREATE_REFUSAL_CRITERIA, + ) + + migrator.apply_tested_migration(("cases", "0074_update_licence_decision_denial_reasons")) + refused_licence_decision.refresh_from_db() + assert list(refused_licence_decision.denial_reasons.values_list("id", flat=True)) == ["1", "5", "7"] diff --git a/api/cases/tests/factories.py b/api/cases/tests/factories.py index e4b46507db..fa006a0e21 100644 --- a/api/cases/tests/factories.py +++ b/api/cases/tests/factories.py @@ -12,6 +12,7 @@ EcjuQuery, GoodCountryDecision, DepartmentSLA, + LicenceDecision, ) from api.queues.tests.factories import QueueFactory from api.organisations.tests.factories import OrganisationFactory @@ -125,3 +126,8 @@ class CaseNoteFactory(factory.django.DjangoModelFactory): class Meta: model = CaseNote + + +class LicenceDecisionFactory(factory.django.DjangoModelFactory): + class Meta: + model = LicenceDecision From 7d874883f00313762a600f90b59b10a952574b9f Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Wed, 11 Dec 2024 12:16:49 +0000 Subject: [PATCH 06/10] Add comment about removing order by --- .../0074_update_licence_decision_denial_reasons.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/api/cases/migrations/0074_update_licence_decision_denial_reasons.py b/api/cases/migrations/0074_update_licence_decision_denial_reasons.py index ae5a57f4b9..3256aa415d 100644 --- a/api/cases/migrations/0074_update_licence_decision_denial_reasons.py +++ b/api/cases/migrations/0074_update_licence_decision_denial_reasons.py @@ -22,7 +22,10 @@ def update_licencedecision_denial_reasons(apps, schema_editor): .only("denial_reasons__id", "case__licence_decisions__id") .exclude(denial_reasons__id__isnull=True) # This removes refusals without any criteria .values_list("denial_reasons__id", "case__licence_decisions__id") - .order_by() # We need to remove the order_by to make sure the distinct works + # The AdviceManager orders by `created_at` and this affects the distinct + # so we remove the ordering completely to ensure the distinct workds as + # expected + .order_by() .distinct() ) From e1d69f1fb8635b03aecaac03cb660414e76e78d8 Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Wed, 11 Dec 2024 14:37:06 +0000 Subject: [PATCH 07/10] Use more explicit filtering to remove advice records that aren't refusals --- .../0074_update_licence_decision_denial_reasons.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/api/cases/migrations/0074_update_licence_decision_denial_reasons.py b/api/cases/migrations/0074_update_licence_decision_denial_reasons.py index 3256aa415d..0402cc0352 100644 --- a/api/cases/migrations/0074_update_licence_decision_denial_reasons.py +++ b/api/cases/migrations/0074_update_licence_decision_denial_reasons.py @@ -3,7 +3,10 @@ from django.db import migrations from api.audit_trail.enums import AuditType -from api.cases.enums import AdviceLevel +from api.cases.enums import ( + AdviceLevel, + AdviceType, +) LICENSING_UNIT_ID = "58e77e47-42c8-499f-a58d-94f94541f8c6" @@ -18,9 +21,9 @@ def update_licencedecision_denial_reasons(apps, schema_editor): case__licence_decisions__decision="refused", level=AdviceLevel.FINAL, team_id=LICENSING_UNIT_ID, # Just care about LU advice + type=AdviceType.REFUSE, ) .only("denial_reasons__id", "case__licence_decisions__id") - .exclude(denial_reasons__id__isnull=True) # This removes refusals without any criteria .values_list("denial_reasons__id", "case__licence_decisions__id") # The AdviceManager orders by `created_at` and this affects the distinct # so we remove the ordering completely to ensure the distinct workds as From 3fd4c434075a80bae8a5ecf5ba4d4cec13cf1b7a Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Thu, 12 Dec 2024 13:57:55 +0000 Subject: [PATCH 08/10] Remove unused loop --- .../migrations/0074_update_licence_decision_denial_reasons.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/api/cases/migrations/0074_update_licence_decision_denial_reasons.py b/api/cases/migrations/0074_update_licence_decision_denial_reasons.py index 0402cc0352..3e88b861a9 100644 --- a/api/cases/migrations/0074_update_licence_decision_denial_reasons.py +++ b/api/cases/migrations/0074_update_licence_decision_denial_reasons.py @@ -32,10 +32,6 @@ def update_licencedecision_denial_reasons(apps, schema_editor): .distinct() ) - for denial_reason, licence_decision_id in denial_reasons: - licence_decision = LicenceDecision.objects.get(pk=licence_decision_id) - licence_decision.denial_reasons.add(denial_reason) - updated_cases = set() for denial_reason, licence_decision_id in denial_reasons: licence_decision = LicenceDecision.objects.get(pk=licence_decision_id) From fc551618828c7723c6ace5f6010700ede8da317b Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Wed, 18 Dec 2024 11:17:16 +0000 Subject: [PATCH 09/10] Remove code that wasn't used --- test_helpers/factories.py | 32 -------------------------------- 1 file changed, 32 deletions(-) delete mode 100644 test_helpers/factories.py diff --git a/test_helpers/factories.py b/test_helpers/factories.py deleted file mode 100644 index c0181053f0..0000000000 --- a/test_helpers/factories.py +++ /dev/null @@ -1,32 +0,0 @@ -import factory - - -def get_model_for_factory(factory_class, apps): - model_meta = factory_class._meta.model._meta - - return apps.get_model(model_meta.app_label, model_meta.object_name) - - -def fullname(cls): - return f"{cls.__module__}.{cls.__name__}" - - -def generate_factory(apps, factory_class, **defaults): - for field_name, field_value in factory_class._meta.declarations.items(): - if field_name in defaults: - continue - if not isinstance(field_value, factory.declarations.SubFactory): - continue - generated_factory = generate_factory( - apps, - field_value.get_factory(), - **field_value.defaults, - ) - defaults[field_name] = factory.declarations.SubFactory(generated_factory) - - updated_factory = factory.make_factory( - get_model_for_factory(factory_class, apps), - **defaults, - FACTORY_CLASS=factory_class, - ) - return updated_factory From 656d40c4cb0c4b8bca3bb617c2ccfa292d46b258 Mon Sep 17 00:00:00 2001 From: Kevin Carrogan Date: Wed, 18 Dec 2024 11:20:00 +0000 Subject: [PATCH 10/10] Fix typo --- .../migrations/0074_update_licence_decision_denial_reasons.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/cases/migrations/0074_update_licence_decision_denial_reasons.py b/api/cases/migrations/0074_update_licence_decision_denial_reasons.py index 3e88b861a9..c1a0e5c218 100644 --- a/api/cases/migrations/0074_update_licence_decision_denial_reasons.py +++ b/api/cases/migrations/0074_update_licence_decision_denial_reasons.py @@ -26,7 +26,7 @@ def update_licencedecision_denial_reasons(apps, schema_editor): .only("denial_reasons__id", "case__licence_decisions__id") .values_list("denial_reasons__id", "case__licence_decisions__id") # The AdviceManager orders by `created_at` and this affects the distinct - # so we remove the ordering completely to ensure the distinct workds as + # so we remove the ordering completely to ensure the distinct works as # expected .order_by() .distinct()