Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ltd 5681 update licence decision refusal criteria #2333

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
6 changes: 2 additions & 4 deletions api/applications/tests/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
2 changes: 2 additions & 0 deletions api/cases/migrations/0073_licencedecision_denial_reasons.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
# 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,
AdviceType,
)


LICENSING_UNIT_ID = "58e77e47-42c8-499f-a58d-94f94541f8c6"


def update_licencedecision_denial_reasons(apps, schema_editor):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this been run against a recent anonymised copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been.

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
type=AdviceType.REFUSE,
)
.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
# expected
.order_by()
.distinct()
)

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use apps.get_model instead of working on directly-imported ContentType.

Directly importing models in migrations is flaky and in a few migrations time will probably fail because during migrations the code in models.py is out of step with the database schema: the models.py can have a field defined that does not yet exist in the database because the required migration has not yet ran. More.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with this


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(",")
Copy link
Contributor

@currycoder currycoder Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These need to be valid DenialReason.id values, right? Is it worth checking against a list of valid ids first? Or is the thought that this will fail with an IntegrityError or similar on the set() below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It hasn't failed when running on the production-like data so far so I've not worried about this.

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,
),
]

This file was deleted.

Original file line number Diff line number Diff line change
@@ -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"]
6 changes: 6 additions & 0 deletions api/cases/tests/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
EcjuQuery,
GoodCountryDecision,
DepartmentSLA,
LicenceDecision,
)
from api.queues.tests.factories import QueueFactory
from api.organisations.tests.factories import OrganisationFactory
Expand Down Expand Up @@ -125,3 +126,8 @@ class CaseNoteFactory(factory.django.DjangoModelFactory):

class Meta:
model = CaseNote


class LicenceDecisionFactory(factory.django.DjangoModelFactory):
class Meta:
model = LicenceDecision
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand Down
32 changes: 32 additions & 0 deletions test_helpers/factories.py
Original file line number Diff line number Diff line change
@@ -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
Comment on lines +1 to +32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this get used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't currently. I can get rid of it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you plan to use it somehow I'm ok with it staying, was just trying to work out if there was something else going on

Loading