From 81b87a2baa2d81199be6724340cd0fdb0087d29c Mon Sep 17 00:00:00 2001 From: Robert Date: Thu, 3 Jul 2025 14:38:57 +0100 Subject: [PATCH 1/9] WIP --- .../views/eligibility.py | 24 +++++++++++++++---- .../views/response_model/eligibility.py | 10 ++++---- .../in_process/test_eligibility_endpoint.py | 6 ++--- 3 files changed, 27 insertions(+), 13 deletions(-) diff --git a/src/eligibility_signposting_api/views/eligibility.py b/src/eligibility_signposting_api/views/eligibility.py index aafe65134..417b5e6c8 100644 --- a/src/eligibility_signposting_api/views/eligibility.py +++ b/src/eligibility_signposting_api/views/eligibility.py @@ -103,11 +103,7 @@ def build_eligibility_response(eligibility_status: EligibilityStatus) -> eligibi statusText=eligibility.StatusText(f"{condition.status}"), # pyright: ignore[reportCallIssue] eligibilityCohorts=build_eligibility_cohorts(condition), # pyright: ignore[reportCallIssue] suitabilityRules=build_suitability_results(condition), # pyright: ignore[reportCallIssue] - actions=( - condition.actions.actions - if condition.actions is not None and condition.actions.actions is not None - else None - ), + actions=build_actions(condition), ) processed_suggestions.append(suggestions) @@ -120,6 +116,24 @@ def build_eligibility_response(eligibility_status: EligibilityStatus) -> eligibi ) +def build_actions(condition: Condition) -> list[eligibility.Action] | None: + if condition.actions is not None and condition.actions.actions is not None: + return [ + eligibility.Action( + actionType=eligibility.ActionType(action.action_type), + actionCode=eligibility.ActionCode(action.action_code), + description=eligibility.Description(action.action_description) + if action.action_description is not None + else None, + urlLink=eligibility.HttpUrl(action.url_link) if action.url_link is not None else None, + urlLabel=eligibility.UrlLabel(action.url_label) if action.url_label is not None else None, + ) + for action in condition.actions.actions + ] + + return None + + def build_eligibility_cohorts(condition: Condition) -> list[eligibility.EligibilityCohort]: """Group Iteration cohorts and make only one entry per cohort group""" diff --git a/src/eligibility_signposting_api/views/response_model/eligibility.py b/src/eligibility_signposting_api/views/response_model/eligibility.py index 6e761de20..58af90904 100644 --- a/src/eligibility_signposting_api/views/response_model/eligibility.py +++ b/src/eligibility_signposting_api/views/response_model/eligibility.py @@ -5,8 +5,6 @@ from pydantic import UUID4, BaseModel, Field, HttpUrl, field_serializer from pydantic_core.core_schema import SerializationInfo -from eligibility_signposting_api.model.eligibility import SuggestedAction - LastUpdated = NewType("LastUpdated", datetime) ConditionName = NewType("ConditionName", str) StatusText = NewType("StatusText", str) @@ -17,6 +15,7 @@ RuleText = NewType("RuleText", str) CohortCode = NewType("CohortCode", str) CohortText = NewType("CohortText", str) +UrlLabel = NewType("UrlLabel", str) class Status(StrEnum): @@ -50,8 +49,9 @@ class SuitabilityRule(BaseModel): class Action(BaseModel): action_type: ActionType = Field(..., alias="actionType") action_code: ActionCode = Field(..., alias="actionCode") - description: Description - url_link: HttpUrl = Field(..., alias="urlLink") + description: Description | None + url_link: HttpUrl | None = Field(..., alias="urlLink") + url_label: UrlLabel | None = Field(..., alias="urlLabel") model_config = {"populate_by_name": True} @@ -62,7 +62,7 @@ class ProcessedSuggestion(BaseModel): status_text: StatusText = Field(..., alias="statusText") eligibility_cohorts: list[EligibilityCohort] = Field(..., alias="eligibilityCohorts") suitability_rules: list[SuitabilityRule] = Field(..., alias="suitabilityRules") - actions: list[SuggestedAction] | None + actions: list[Action] | None model_config = {"populate_by_name": True} diff --git a/tests/integration/in_process/test_eligibility_endpoint.py b/tests/integration/in_process/test_eligibility_endpoint.py index 60d39d48a..833b0b758 100644 --- a/tests/integration/in_process/test_eligibility_endpoint.py +++ b/tests/integration/in_process/test_eligibility_endpoint.py @@ -218,7 +218,7 @@ def test_actionable( "cohortText": "positive_description", } ], - "actions": [{"action_type": "defaultcomms", "action_code": "action_code"}], + "actions": [{"actionType": "defaultcomms", "actionCode": "action_code"}], "suitabilityRules": [], "statusText": "Status.actionable", } @@ -355,7 +355,7 @@ def test_actionable_when_only_magic_cohort_is_present( "cohortText": "magic positive description", } ], - "actions": [{"action_type": "defaultcomms", "action_code": "action_code"}], + "actions": [{"actionType": "defaultcomms", "actionCode": "action_code"}], "suitabilityRules": [], "statusText": "Status.actionable", } @@ -511,7 +511,7 @@ def test_actionable( "condition": "FLU", "status": "Actionable", "eligibilityCohorts": [], - "actions": [{"action_code": "action_code", "action_type": "defaultcomms"}], + "actions": [{"actionCode": "action_code", "actionType": "defaultcomms"}], "suitabilityRules": [], "statusText": "Status.actionable", } From 0bdd52731ec59557baebaf6d937a1c46c286f8d6 Mon Sep 17 00:00:00 2001 From: Robert Date: Thu, 3 Jul 2025 16:46:32 +0100 Subject: [PATCH 2/9] WIP --- src/eligibility_signposting_api/model/eligibility.py | 4 +++- src/eligibility_signposting_api/model/rules.py | 4 ++-- src/eligibility_signposting_api/views/eligibility.py | 2 +- .../services/calculators/test_eligibility_calculator.py | 6 +++--- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/eligibility_signposting_api/model/eligibility.py b/src/eligibility_signposting_api/model/eligibility.py index 7e3582c3b..f50562cfe 100644 --- a/src/eligibility_signposting_api/model/eligibility.py +++ b/src/eligibility_signposting_api/model/eligibility.py @@ -6,6 +6,8 @@ from functools import total_ordering from typing import NewType, Self +from pydantic import HttpUrl + NHSNumber = NewType("NHSNumber", str) DateOfBirth = NewType("DateOfBirth", date) Postcode = NewType("Postcode", str) @@ -17,7 +19,7 @@ ActionType = NewType("ActionType", str) ActionCode = NewType("ActionCode", str) ActionDescription = NewType("ActionDescription", str) -UrlLink = NewType("UrlLink", str) +UrlLink = NewType("UrlLink", HttpUrl) UrlLabel = NewType("UrlLabel", str) diff --git a/src/eligibility_signposting_api/model/rules.py b/src/eligibility_signposting_api/model/rules.py index 473a15f3d..541db6263 100644 --- a/src/eligibility_signposting_api/model/rules.py +++ b/src/eligibility_signposting_api/model/rules.py @@ -9,7 +9,7 @@ from operator import attrgetter from typing import Literal, NewType -from pydantic import BaseModel, Field, RootModel, field_serializer, field_validator, model_validator +from pydantic import BaseModel, Field, HttpUrl, RootModel, field_serializer, field_validator, model_validator from eligibility_signposting_api.config.contants import MAGIC_COHORT_LABEL, RULE_STOP_DEFAULT @@ -132,7 +132,7 @@ class AvailableAction(BaseModel): action_type: str = Field(..., alias="ActionType") action_code: str = Field(..., alias="ExternalRoutingCode") action_description: str | None = Field(None, alias="ActionDescription") - url_link: str | None = Field(None, alias="UrlLink") + url_link: HttpUrl | None = Field(None, alias="UrlLink") url_label: str | None = Field(None, alias="UrlLabel") model_config = {"populate_by_name": True} diff --git a/src/eligibility_signposting_api/views/eligibility.py b/src/eligibility_signposting_api/views/eligibility.py index 417b5e6c8..c509f1a05 100644 --- a/src/eligibility_signposting_api/views/eligibility.py +++ b/src/eligibility_signposting_api/views/eligibility.py @@ -127,7 +127,7 @@ def build_actions(condition: Condition) -> list[eligibility.Action] | None: else None, urlLink=eligibility.HttpUrl(action.url_link) if action.url_link is not None else None, urlLabel=eligibility.UrlLabel(action.url_label) if action.url_label is not None else None, - ) + ).model_dump(exclude_none=True) for action in condition.actions.actions ] diff --git a/tests/unit/services/calculators/test_eligibility_calculator.py b/tests/unit/services/calculators/test_eligibility_calculator.py index f86ca7702..55f3ea41c 100644 --- a/tests/unit/services/calculators/test_eligibility_calculator.py +++ b/tests/unit/services/calculators/test_eligibility_calculator.py @@ -5,7 +5,7 @@ from faker import Faker from freezegun import freeze_time from hamcrest import assert_that, contains_exactly, contains_inanyorder, equal_to, has_item, has_items, is_in -from pydantic import ValidationError +from pydantic import HttpUrl, ValidationError from eligibility_signposting_api.model import rules from eligibility_signposting_api.model import rules as rules_model @@ -51,14 +51,14 @@ def test_get_redirect_rules(): ActionType="ActionType1", ExternalRoutingCode="ActionCode1", ActionDescription="ActionDescription1", - UrlLink="ActionUrl1", + UrlLink=HttpUrl("https://www.ActionUrl1.com"), UrlLabel="ActionLabel1", ), "defaultcomms": AvailableAction( ActionType="ActionType2", ExternalRoutingCode="defaultcomms", ActionDescription="ActionDescription2", - UrlLink="ActionUrl2", + UrlLink=HttpUrl("https://www.ActionUrl2.com"), UrlLabel="ActionLabel2", ), } From 426a3bb611cdbba16eaaec3a1135ad96b3252eeb Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Thu, 3 Jul 2025 23:07:07 +0100 Subject: [PATCH 3/9] factory created for suggested actions --- .../model/eligibility.py | 2 +- .../views/eligibility.py | 2 +- tests/fixtures/builders/model/eligibility.py | 17 ++++++++++++----- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/eligibility_signposting_api/model/eligibility.py b/src/eligibility_signposting_api/model/eligibility.py index f50562cfe..dd952dd8b 100644 --- a/src/eligibility_signposting_api/model/eligibility.py +++ b/src/eligibility_signposting_api/model/eligibility.py @@ -19,7 +19,7 @@ ActionType = NewType("ActionType", str) ActionCode = NewType("ActionCode", str) ActionDescription = NewType("ActionDescription", str) -UrlLink = NewType("UrlLink", HttpUrl) +UrlLink = NewType("UrlLink", str) UrlLabel = NewType("UrlLabel", str) diff --git a/src/eligibility_signposting_api/views/eligibility.py b/src/eligibility_signposting_api/views/eligibility.py index c509f1a05..417b5e6c8 100644 --- a/src/eligibility_signposting_api/views/eligibility.py +++ b/src/eligibility_signposting_api/views/eligibility.py @@ -127,7 +127,7 @@ def build_actions(condition: Condition) -> list[eligibility.Action] | None: else None, urlLink=eligibility.HttpUrl(action.url_link) if action.url_link is not None else None, urlLabel=eligibility.UrlLabel(action.url_label) if action.url_label is not None else None, - ).model_dump(exclude_none=True) + ) for action in condition.actions.actions ] diff --git a/tests/fixtures/builders/model/eligibility.py b/tests/fixtures/builders/model/eligibility.py index a8781eedd..fa4d34420 100644 --- a/tests/fixtures/builders/model/eligibility.py +++ b/tests/fixtures/builders/model/eligibility.py @@ -4,17 +4,24 @@ from polyfactory import Use from polyfactory.factories import DataclassFactory -from eligibility_signposting_api.model.eligibility import CohortGroupResult, Condition, EligibilityStatus +from eligibility_signposting_api.model import eligibility +from eligibility_signposting_api.model.eligibility import UrlLink -class ConditionFactory(DataclassFactory[Condition]): ... +class SuggestedActionFactory(DataclassFactory[eligibility.SuggestedAction]): + url_link = UrlLink("https://test_example.com") +class SuggestedActionsFactory(DataclassFactory[eligibility.SuggestedActions]): + actions = Use(SuggestedActionFactory.batch, size=2) -class EligibilityStatusFactory(DataclassFactory[EligibilityStatus]): - condition = Use(ConditionFactory.batch, size=2) +class ConditionFactory(DataclassFactory[eligibility.Condition]): + actions = SuggestedActionsFactory.build() +class EligibilityStatusFactory(DataclassFactory[eligibility.EligibilityStatus]): + conditions = Use(ConditionFactory.batch, size=2) -class CohortResultFactory(DataclassFactory[CohortGroupResult]): ... + +class CohortResultFactory(DataclassFactory[eligibility.CohortGroupResult]): ... def random_str(length: int) -> str: From 419d394b4e67aacf10651594eb0260093b83301e Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Thu, 3 Jul 2025 23:59:19 +0100 Subject: [PATCH 4/9] unit tests --- .../views/eligibility.py | 2 +- tests/fixtures/matchers/eligibility.py | 6 +- tests/unit/views/test_eligibility.py | 148 +++++++++++++----- 3 files changed, 117 insertions(+), 39 deletions(-) diff --git a/src/eligibility_signposting_api/views/eligibility.py b/src/eligibility_signposting_api/views/eligibility.py index 417b5e6c8..27780f11b 100644 --- a/src/eligibility_signposting_api/views/eligibility.py +++ b/src/eligibility_signposting_api/views/eligibility.py @@ -39,7 +39,7 @@ def check_eligibility(nhs_number: NHSNumber, eligibility_service: Injected[Eligi except UnknownPersonError: return handle_unknown_person_error(nhs_number) else: - eligibility_response = build_eligibility_response(eligibility_status) + eligibility_response: eligibility.EligibilityResponse = build_eligibility_response(eligibility_status) return make_response( eligibility_response.model_dump(by_alias=True, mode="json", exclude_none=True), HTTPStatus.OK ) diff --git a/tests/fixtures/matchers/eligibility.py b/tests/fixtures/matchers/eligibility.py index bf7260df1..4e142495d 100644 --- a/tests/fixtures/matchers/eligibility.py +++ b/tests/fixtures/matchers/eligibility.py @@ -1,7 +1,7 @@ from hamcrest.core.matcher import Matcher from eligibility_signposting_api.model.eligibility import CohortGroupResult, Condition, EligibilityStatus, Reason -from eligibility_signposting_api.views.response_model.eligibility import EligibilityCohort, SuitabilityRule +from eligibility_signposting_api.views.response_model.eligibility import EligibilityCohort, SuitabilityRule, Action from .meta import BaseAutoMatcher @@ -23,6 +23,7 @@ class EligibilityCohortMatcher(BaseAutoMatcher[EligibilityCohort]): ... class SuitabilityRuleMatcher(BaseAutoMatcher[SuitabilityRule]): ... +class ActionMatcher(BaseAutoMatcher[Action]): ... def is_eligibility_status() -> Matcher[EligibilityStatus]: return EligibilityStatusMatcher() @@ -46,3 +47,6 @@ def is_eligibility_cohort() -> Matcher[EligibilityCohort]: def is_suitability_rule() -> Matcher[SuitabilityRule]: return SuitabilityRuleMatcher() + +def is_action() -> Matcher[Action]: + return ActionMatcher() diff --git a/tests/unit/views/test_eligibility.py b/tests/unit/views/test_eligibility.py index 56e1becb2..3bd91d304 100644 --- a/tests/unit/views/test_eligibility.py +++ b/tests/unit/views/test_eligibility.py @@ -1,13 +1,16 @@ +import json import logging +from datetime import datetime from http import HTTPStatus -from unittest.mock import Mock, patch +from unittest.mock import Mock, patch, MagicMock +from uuid import uuid4 import pytest from brunns.matchers.data import json_matching as is_json_that from brunns.matchers.werkzeug import is_werkzeug_response as is_response from flask import Flask, Request from flask.testing import FlaskClient -from hamcrest import assert_that, contains_exactly, has_entries, has_length +from hamcrest import assert_that, contains_exactly, has_entries, has_length, is_, none from wireup.integration.flask import get_app_container from eligibility_signposting_api.model.eligibility import ( @@ -19,21 +22,23 @@ RuleDescription, RuleName, RuleType, - Status, + Status, SuggestedActions, SuggestedAction, ActionType, ActionCode, ActionDescription, UrlLink, UrlLabel, ) from eligibility_signposting_api.services import EligibilityService, UnknownPersonError from eligibility_signposting_api.services.eligibility_services import InvalidQueryParamError from eligibility_signposting_api.views.eligibility import ( build_eligibility_cohorts, build_suitability_results, - get_include_actions_flag, + get_include_actions_flag, build_actions, ) +from eligibility_signposting_api.views.response_model import eligibility +from eligibility_signposting_api.views.response_model.eligibility import Description, EligibilityResponse, LastUpdated from tests.fixtures.builders.model.eligibility import ( CohortResultFactory, ConditionFactory, EligibilityStatusFactory, ) -from tests.fixtures.matchers.eligibility import is_eligibility_cohort, is_suitability_rule +from tests.fixtures.matchers.eligibility import is_eligibility_cohort, is_suitability_rule, is_action logger = logging.getLogger(__name__) @@ -136,44 +141,46 @@ def test_unexpected_error(app: Flask, client: FlaskClient): ("cohort_results", "expected_eligibility_cohorts", "test_comment"), [ ( - [ - CohortResultFactory.build( - cohort_code="CohortCode1", status=Status.not_actionable, description="+ve des 1" - ), - CohortResultFactory.build( - cohort_code="CohortCode2", status=Status.not_actionable, description="+ve des 2" - ), - ], - [ - ("CohortCode1", "NotActionable", "+ve des 1"), - ("CohortCode2", "NotActionable", "+ve des 2"), - ], - "two cohort group codes with same status, nothing is ignored", + [ + CohortResultFactory.build( + cohort_code="CohortCode1", status=Status.not_actionable, description="+ve des 1" + ), + CohortResultFactory.build( + cohort_code="CohortCode2", status=Status.not_actionable, description="+ve des 2" + ), + ], + [ + ("CohortCode1", "NotActionable", "+ve des 1"), + ("CohortCode2", "NotActionable", "+ve des 2"), + ], + "two cohort group codes with same status, nothing is ignored", ), ( - [ - CohortResultFactory.build( - cohort_code="CohortCode1", status=Status.not_actionable, description="+ve des 1" - ), - CohortResultFactory.build(cohort_code="CohortCode2", status=Status.not_actionable, description=None), - CohortResultFactory.build(cohort_code="CohortCode3", status=Status.not_actionable, description=""), - ], - [("CohortCode1", "NotActionable", "+ve des 1")], - "only one cohort has description", + [ + CohortResultFactory.build( + cohort_code="CohortCode1", status=Status.not_actionable, description="+ve des 1" + ), + CohortResultFactory.build(cohort_code="CohortCode2", status=Status.not_actionable, + description=None), + CohortResultFactory.build(cohort_code="CohortCode3", status=Status.not_actionable, description=""), + ], + [("CohortCode1", "NotActionable", "+ve des 1")], + "only one cohort has description", ), ( - [ - CohortResultFactory.build(cohort_code="some_cohort", status=Status.not_actionable, description=""), - ], - [], - "only one cohort but no description, so it is ignored", + [ + CohortResultFactory.build(cohort_code="some_cohort", status=Status.not_actionable, description=""), + ], + [], + "only one cohort but no description, so it is ignored", ), ( - [ - CohortResultFactory.build(cohort_code="some_cohort", status=Status.not_actionable, description=None), - ], - [], - "only one cohort but no description, so it is ignored", + [ + CohortResultFactory.build(cohort_code="some_cohort", status=Status.not_actionable, + description=None), + ], + [], + "only one cohort but no description, so it is ignored", ), ], ) @@ -356,6 +363,73 @@ def test_no_suitability_rules_for_actionable(): assert_that(results, has_length(0)) +@pytest.mark.parametrize( + ("suggested_actions", "expected"), + [ + ( + SuggestedActions( + actions=[ + SuggestedAction( + action_type=ActionType("TYPE_A"), + action_code=ActionCode("CODE123"), + action_description=ActionDescription("Some description"), + url_link=UrlLink("https://example.com"), + url_label=UrlLabel("Learn more"), + ) + ] + ), + [ + eligibility.Action( + actionType=eligibility.ActionType("TYPE_A"), + actionCode=eligibility.ActionCode("CODE123"), + description=eligibility.Description("Some description"), + urlLink=eligibility.HttpUrl("https://example.com"), + urlLabel=eligibility.UrlLabel("Learn more"), + ) + ], + ), + ( + SuggestedActions( + actions=[ + SuggestedAction( + action_type=ActionType("TYPE_B"), + action_code=ActionCode("CODE123"), + action_description=None, + url_link=None, + url_label=None, + ) + ] + ), + [ + eligibility.Action( + actionType=eligibility.ActionType("TYPE_B"), + actionCode=eligibility.ActionCode("CODE123"), + description=None, + urlLink=None, + urlLabel=None, + ) + ], + ), + # Case: SuggestedActions is None + ( + None, + None, + ), + # Case: SuggestedActions.actions is [] + ( + SuggestedActions(actions=[]), + [], + ), + ] +) +def test_build_actions(suggested_actions, expected): + results = build_actions(ConditionFactory.build(actions=suggested_actions)) + if expected is None: + assert_that(results, is_(none())) + else: + assert_that(results, contains_exactly(*expected)) + + def test_nhs_number_and_include_actions_param_given_and_is_yes(app: Flask, client: FlaskClient): # Given with get_app_container(app).override.service(EligibilityService, new=FakeEligibilityService()): From e17cc71eb6cf37fb7f589aa0ddfe1d3fd0bae23a Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Fri, 4 Jul 2025 00:17:29 +0100 Subject: [PATCH 5/9] exclude none test --- .../model/eligibility.py | 2 - tests/fixtures/builders/model/eligibility.py | 3 + tests/fixtures/matchers/eligibility.py | 5 +- tests/unit/views/test_eligibility.py | 221 +++++++++++------- 4 files changed, 143 insertions(+), 88 deletions(-) diff --git a/src/eligibility_signposting_api/model/eligibility.py b/src/eligibility_signposting_api/model/eligibility.py index dd952dd8b..7e3582c3b 100644 --- a/src/eligibility_signposting_api/model/eligibility.py +++ b/src/eligibility_signposting_api/model/eligibility.py @@ -6,8 +6,6 @@ from functools import total_ordering from typing import NewType, Self -from pydantic import HttpUrl - NHSNumber = NewType("NHSNumber", str) DateOfBirth = NewType("DateOfBirth", date) Postcode = NewType("Postcode", str) diff --git a/tests/fixtures/builders/model/eligibility.py b/tests/fixtures/builders/model/eligibility.py index fa4d34420..8a185bd6e 100644 --- a/tests/fixtures/builders/model/eligibility.py +++ b/tests/fixtures/builders/model/eligibility.py @@ -11,12 +11,15 @@ class SuggestedActionFactory(DataclassFactory[eligibility.SuggestedAction]): url_link = UrlLink("https://test_example.com") + class SuggestedActionsFactory(DataclassFactory[eligibility.SuggestedActions]): actions = Use(SuggestedActionFactory.batch, size=2) + class ConditionFactory(DataclassFactory[eligibility.Condition]): actions = SuggestedActionsFactory.build() + class EligibilityStatusFactory(DataclassFactory[eligibility.EligibilityStatus]): conditions = Use(ConditionFactory.batch, size=2) diff --git a/tests/fixtures/matchers/eligibility.py b/tests/fixtures/matchers/eligibility.py index 4e142495d..98bfb4138 100644 --- a/tests/fixtures/matchers/eligibility.py +++ b/tests/fixtures/matchers/eligibility.py @@ -1,7 +1,7 @@ from hamcrest.core.matcher import Matcher from eligibility_signposting_api.model.eligibility import CohortGroupResult, Condition, EligibilityStatus, Reason -from eligibility_signposting_api.views.response_model.eligibility import EligibilityCohort, SuitabilityRule, Action +from eligibility_signposting_api.views.response_model.eligibility import Action, EligibilityCohort, SuitabilityRule from .meta import BaseAutoMatcher @@ -23,8 +23,10 @@ class EligibilityCohortMatcher(BaseAutoMatcher[EligibilityCohort]): ... class SuitabilityRuleMatcher(BaseAutoMatcher[SuitabilityRule]): ... + class ActionMatcher(BaseAutoMatcher[Action]): ... + def is_eligibility_status() -> Matcher[EligibilityStatus]: return EligibilityStatusMatcher() @@ -48,5 +50,6 @@ def is_eligibility_cohort() -> Matcher[EligibilityCohort]: def is_suitability_rule() -> Matcher[SuitabilityRule]: return SuitabilityRuleMatcher() + def is_action() -> Matcher[Action]: return ActionMatcher() diff --git a/tests/unit/views/test_eligibility.py b/tests/unit/views/test_eligibility.py index 3bd91d304..c03394a52 100644 --- a/tests/unit/views/test_eligibility.py +++ b/tests/unit/views/test_eligibility.py @@ -1,8 +1,8 @@ import json import logging -from datetime import datetime +from datetime import UTC, datetime from http import HTTPStatus -from unittest.mock import Mock, patch, MagicMock +from unittest.mock import MagicMock, Mock, patch from uuid import uuid4 import pytest @@ -14,6 +14,9 @@ from wireup.integration.flask import get_app_container from eligibility_signposting_api.model.eligibility import ( + ActionCode, + ActionDescription, + ActionType, CohortGroupResult, Condition, EligibilityStatus, @@ -22,23 +25,27 @@ RuleDescription, RuleName, RuleType, - Status, SuggestedActions, SuggestedAction, ActionType, ActionCode, ActionDescription, UrlLink, UrlLabel, + Status, + SuggestedAction, + SuggestedActions, + UrlLabel, + UrlLink, ) from eligibility_signposting_api.services import EligibilityService, UnknownPersonError from eligibility_signposting_api.services.eligibility_services import InvalidQueryParamError from eligibility_signposting_api.views.eligibility import ( + build_actions, build_eligibility_cohorts, build_suitability_results, - get_include_actions_flag, build_actions, + get_include_actions_flag, ) from eligibility_signposting_api.views.response_model import eligibility -from eligibility_signposting_api.views.response_model.eligibility import Description, EligibilityResponse, LastUpdated from tests.fixtures.builders.model.eligibility import ( CohortResultFactory, ConditionFactory, EligibilityStatusFactory, ) -from tests.fixtures.matchers.eligibility import is_eligibility_cohort, is_suitability_rule, is_action +from tests.fixtures.matchers.eligibility import is_eligibility_cohort, is_suitability_rule logger = logging.getLogger(__name__) @@ -141,46 +148,44 @@ def test_unexpected_error(app: Flask, client: FlaskClient): ("cohort_results", "expected_eligibility_cohorts", "test_comment"), [ ( - [ - CohortResultFactory.build( - cohort_code="CohortCode1", status=Status.not_actionable, description="+ve des 1" - ), - CohortResultFactory.build( - cohort_code="CohortCode2", status=Status.not_actionable, description="+ve des 2" - ), - ], - [ - ("CohortCode1", "NotActionable", "+ve des 1"), - ("CohortCode2", "NotActionable", "+ve des 2"), - ], - "two cohort group codes with same status, nothing is ignored", + [ + CohortResultFactory.build( + cohort_code="CohortCode1", status=Status.not_actionable, description="+ve des 1" + ), + CohortResultFactory.build( + cohort_code="CohortCode2", status=Status.not_actionable, description="+ve des 2" + ), + ], + [ + ("CohortCode1", "NotActionable", "+ve des 1"), + ("CohortCode2", "NotActionable", "+ve des 2"), + ], + "two cohort group codes with same status, nothing is ignored", ), ( - [ - CohortResultFactory.build( - cohort_code="CohortCode1", status=Status.not_actionable, description="+ve des 1" - ), - CohortResultFactory.build(cohort_code="CohortCode2", status=Status.not_actionable, - description=None), - CohortResultFactory.build(cohort_code="CohortCode3", status=Status.not_actionable, description=""), - ], - [("CohortCode1", "NotActionable", "+ve des 1")], - "only one cohort has description", + [ + CohortResultFactory.build( + cohort_code="CohortCode1", status=Status.not_actionable, description="+ve des 1" + ), + CohortResultFactory.build(cohort_code="CohortCode2", status=Status.not_actionable, description=None), + CohortResultFactory.build(cohort_code="CohortCode3", status=Status.not_actionable, description=""), + ], + [("CohortCode1", "NotActionable", "+ve des 1")], + "only one cohort has description", ), ( - [ - CohortResultFactory.build(cohort_code="some_cohort", status=Status.not_actionable, description=""), - ], - [], - "only one cohort but no description, so it is ignored", + [ + CohortResultFactory.build(cohort_code="some_cohort", status=Status.not_actionable, description=""), + ], + [], + "only one cohort but no description, so it is ignored", ), ( - [ - CohortResultFactory.build(cohort_code="some_cohort", status=Status.not_actionable, - description=None), - ], - [], - "only one cohort but no description, so it is ignored", + [ + CohortResultFactory.build(cohort_code="some_cohort", status=Status.not_actionable, description=None), + ], + [], + "only one cohort but no description, so it is ignored", ), ], ) @@ -367,60 +372,58 @@ def test_no_suitability_rules_for_actionable(): ("suggested_actions", "expected"), [ ( - SuggestedActions( - actions=[ - SuggestedAction( - action_type=ActionType("TYPE_A"), - action_code=ActionCode("CODE123"), - action_description=ActionDescription("Some description"), - url_link=UrlLink("https://example.com"), - url_label=UrlLabel("Learn more"), - ) - ] - ), - [ - eligibility.Action( - actionType=eligibility.ActionType("TYPE_A"), - actionCode=eligibility.ActionCode("CODE123"), - description=eligibility.Description("Some description"), - urlLink=eligibility.HttpUrl("https://example.com"), - urlLabel=eligibility.UrlLabel("Learn more"), + SuggestedActions( + actions=[ + SuggestedAction( + action_type=ActionType("TYPE_A"), + action_code=ActionCode("CODE123"), + action_description=ActionDescription("Some description"), + url_link=UrlLink("https://example.com"), + url_label=UrlLabel("Learn more"), ) - ], + ] + ), + [ + eligibility.Action( + actionType=eligibility.ActionType("TYPE_A"), + actionCode=eligibility.ActionCode("CODE123"), + description=eligibility.Description("Some description"), + urlLink=eligibility.HttpUrl("https://example.com"), + urlLabel=eligibility.UrlLabel("Learn more"), + ) + ], ), ( - SuggestedActions( - actions=[ - SuggestedAction( - action_type=ActionType("TYPE_B"), - action_code=ActionCode("CODE123"), - action_description=None, - url_link=None, - url_label=None, - ) - ] - ), - [ - eligibility.Action( - actionType=eligibility.ActionType("TYPE_B"), - actionCode=eligibility.ActionCode("CODE123"), - description=None, - urlLink=None, - urlLabel=None, + SuggestedActions( + actions=[ + SuggestedAction( + action_type=ActionType("TYPE_B"), + action_code=ActionCode("CODE123"), + action_description=None, + url_link=None, + url_label=None, ) - ], + ] + ), + [ + eligibility.Action( + actionType=eligibility.ActionType("TYPE_B"), + actionCode=eligibility.ActionCode("CODE123"), + description=None, + urlLink=None, + urlLabel=None, + ) + ], ), - # Case: SuggestedActions is None ( - None, - None, + None, + None, ), - # Case: SuggestedActions.actions is [] ( - SuggestedActions(actions=[]), - [], + SuggestedActions(actions=[]), + [], ), - ] + ], ) def test_build_actions(suggested_actions, expected): results = build_actions(ConditionFactory.build(actions=suggested_actions)) @@ -558,3 +561,51 @@ def test_query_param_include_actions_flag_with_other_params(): pytest.raises(InvalidQueryParamError), ): get_include_actions_flag() + + +def test_excludes_nulls_via_build_response(client: FlaskClient): + mocked_response = eligibility.EligibilityResponse( + responseId=uuid4(), + meta=eligibility.Meta(lastUpdated=eligibility.LastUpdated(datetime(2023, 1, 1, tzinfo=UTC))), + processedSuggestions=[ + eligibility.ProcessedSuggestion( + condition=eligibility.ConditionName("ConditionA"), + status=eligibility.Status.actionable, + statusText=eligibility.StatusText("Go ahead"), + eligibilityCohorts=[], + suitabilityRules=[], + actions=[ + eligibility.Action( + actionType=eligibility.ActionType("TYPE_A"), + actionCode=eligibility.ActionCode("CODE123"), + description=None, # Should be excluded + urlLink=None, # Should be excluded + urlLabel=None, # Should be excluded + ) + ], + ) + ], + ) + + with ( + patch( + "eligibility_signposting_api.views.eligibility.EligibilityService.get_eligibility_status", + return_value=MagicMock(), # No effect + ), + patch( + "eligibility_signposting_api.views.eligibility.build_eligibility_response", + return_value=mocked_response, + ), + ): + response = client.get("/patient-check/12345") + assert response.status_code == HTTPStatus.OK + + payload = json.loads(response.data) + suggestion = payload["processedSuggestions"][0] + action = suggestion["actions"][0] + + assert action["actionType"] == "TYPE_A" + assert action["actionCode"] == "CODE123" + assert "description" not in action + assert "urlLink" not in action + assert "urlLabel" not in action From fa2cd96db6ff06945a24734d416a86760c450c20 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Fri, 4 Jul 2025 00:25:27 +0100 Subject: [PATCH 6/9] urllink is httpurl everywhere --- src/eligibility_signposting_api/model/eligibility.py | 4 +++- tests/fixtures/builders/model/eligibility.py | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/eligibility_signposting_api/model/eligibility.py b/src/eligibility_signposting_api/model/eligibility.py index 7e3582c3b..f50562cfe 100644 --- a/src/eligibility_signposting_api/model/eligibility.py +++ b/src/eligibility_signposting_api/model/eligibility.py @@ -6,6 +6,8 @@ from functools import total_ordering from typing import NewType, Self +from pydantic import HttpUrl + NHSNumber = NewType("NHSNumber", str) DateOfBirth = NewType("DateOfBirth", date) Postcode = NewType("Postcode", str) @@ -17,7 +19,7 @@ ActionType = NewType("ActionType", str) ActionCode = NewType("ActionCode", str) ActionDescription = NewType("ActionDescription", str) -UrlLink = NewType("UrlLink", str) +UrlLink = NewType("UrlLink", HttpUrl) UrlLabel = NewType("UrlLabel", str) diff --git a/tests/fixtures/builders/model/eligibility.py b/tests/fixtures/builders/model/eligibility.py index 8a185bd6e..758b8b0dd 100644 --- a/tests/fixtures/builders/model/eligibility.py +++ b/tests/fixtures/builders/model/eligibility.py @@ -3,13 +3,14 @@ from polyfactory import Use from polyfactory.factories import DataclassFactory +from pydantic import HttpUrl from eligibility_signposting_api.model import eligibility from eligibility_signposting_api.model.eligibility import UrlLink class SuggestedActionFactory(DataclassFactory[eligibility.SuggestedAction]): - url_link = UrlLink("https://test_example.com") + url_link = UrlLink(HttpUrl("https://test-example.com")) class SuggestedActionsFactory(DataclassFactory[eligibility.SuggestedActions]): From 938edb75b22c142cc8e1873d49b983b876d41eac Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Fri, 4 Jul 2025 02:29:51 +0100 Subject: [PATCH 7/9] test for not null --- tests/unit/views/test_eligibility.py | 49 ++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/tests/unit/views/test_eligibility.py b/tests/unit/views/test_eligibility.py index c03394a52..6d3e81f84 100644 --- a/tests/unit/views/test_eligibility.py +++ b/tests/unit/views/test_eligibility.py @@ -11,6 +11,7 @@ from flask import Flask, Request from flask.testing import FlaskClient from hamcrest import assert_that, contains_exactly, has_entries, has_length, is_, none +from pydantic import HttpUrl from wireup.integration.flask import get_app_container from eligibility_signposting_api.model.eligibility import ( @@ -609,3 +610,51 @@ def test_excludes_nulls_via_build_response(client: FlaskClient): assert "description" not in action assert "urlLink" not in action assert "urlLabel" not in action + + +def test_build_response_include_values_that_are_not_null(client: FlaskClient): + mocked_response = eligibility.EligibilityResponse( + responseId=uuid4(), + meta=eligibility.Meta(lastUpdated=eligibility.LastUpdated(datetime(2023, 1, 1, tzinfo=UTC))), + processedSuggestions=[ + eligibility.ProcessedSuggestion( + condition=eligibility.ConditionName("ConditionA"), + status=eligibility.Status.actionable, + statusText=eligibility.StatusText("Go ahead"), + eligibilityCohorts=[], + suitabilityRules=[], + actions=[ + eligibility.Action( + actionType=eligibility.ActionType("TYPE_A"), + actionCode=eligibility.ActionCode("CODE123"), + description=eligibility.Description("Contact GP"), + urlLink=eligibility.HttpUrl(HttpUrl("https://example.dummy/")), + urlLabel=eligibility.UrlLabel("GP contact"), + ) + ], + ) + ], + ) + + with ( + patch( + "eligibility_signposting_api.views.eligibility.EligibilityService.get_eligibility_status", + return_value=MagicMock(), # No effect + ), + patch( + "eligibility_signposting_api.views.eligibility.build_eligibility_response", + return_value=mocked_response, + ), + ): + response = client.get("/patient-check/12345") + assert response.status_code == HTTPStatus.OK + + payload = json.loads(response.data) + suggestion = payload["processedSuggestions"][0] + action = suggestion["actions"][0] + + assert action["actionType"] == "TYPE_A" + assert action["actionCode"] == "CODE123" + assert action["description"] == "Contact GP" + assert action["urlLink"] == "https://example.dummy/" + assert action["urlLabel"] == "GP contact" From ef8c0290491646dd45cd419dd0b29ce5937fdbbe Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Fri, 4 Jul 2025 02:51:47 +0100 Subject: [PATCH 8/9] code refactoring --- .../model/eligibility.py | 9 +-- .../calculators/eligibility_calculator.py | 15 +++-- .../views/eligibility.py | 4 +- tests/fixtures/builders/model/eligibility.py | 6 +- .../test_eligibility_calculator.py | 60 +++++++++---------- tests/unit/views/test_eligibility.py | 43 ++++++------- 6 files changed, 58 insertions(+), 79 deletions(-) diff --git a/src/eligibility_signposting_api/model/eligibility.py b/src/eligibility_signposting_api/model/eligibility.py index f50562cfe..985d0e561 100644 --- a/src/eligibility_signposting_api/model/eligibility.py +++ b/src/eligibility_signposting_api/model/eligibility.py @@ -81,17 +81,12 @@ class SuggestedAction: url_label: UrlLabel | None -@dataclass -class SuggestedActions: - actions: list[SuggestedAction] - - @dataclass class Condition: condition_name: ConditionName status: Status cohort_results: list[CohortGroupResult] - actions: SuggestedActions | None = None + actions: list[SuggestedAction] | None = None @dataclass @@ -106,7 +101,7 @@ class CohortGroupResult: class IterationResult: status: Status cohort_results: list[CohortGroupResult] - actions: SuggestedActions | None + actions: list[SuggestedAction] | None @dataclass diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index 304261a04..79e6aa849 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -23,7 +23,6 @@ IterationResult, Status, SuggestedAction, - SuggestedActions, UrlLabel, UrlLink, ) @@ -129,7 +128,7 @@ def get_redirect_rules( def evaluate_eligibility(self, *, include_actions_flag: bool = True) -> eligibility.EligibilityStatus: """Iterates over campaign groups, evaluates eligibility, and returns a consolidated status.""" condition_results: dict[ConditionName, IterationResult] = {} - actions: SuggestedActions | None = SuggestedActions([]) + actions: list[SuggestedAction] | None = [] for condition_name, campaign_group in self.campaigns_grouped_by_condition_name: iteration_results: dict[str, tuple[Iteration, IterationResult]] = {} @@ -165,12 +164,12 @@ def evaluate_eligibility(self, *, include_actions_flag: bool = True) -> eligibil final_result = self.build_condition_results(condition_results) return eligibility.EligibilityStatus(conditions=final_result) - def handle_redirect_rules(self, best_active_iteration: Iteration) -> SuggestedActions | None: + def handle_redirect_rules(self, best_active_iteration: Iteration) -> list[SuggestedAction] | None: redirect_rules, action_mapper, default_comms = self.get_redirect_rules(best_active_iteration) priority_getter = attrgetter("priority") sorted_rules_by_priority = sorted(redirect_rules, key=priority_getter) - actions: SuggestedActions | None = self.get_actions_from_comms(action_mapper, default_comms) + actions: list[SuggestedAction] | None = self.get_actions_from_comms(action_mapper, default_comms) for _, rule_group in groupby(sorted_rules_by_priority, key=priority_getter): rule_group_list = list(rule_group) matcher_matched_list = [ @@ -181,7 +180,7 @@ def handle_redirect_rules(self, best_active_iteration: Iteration) -> SuggestedAc comms_routing = rule_group_list[0].comms_routing if comms_routing and all(matcher_matched_list): rule_actions = self.get_actions_from_comms(action_mapper, comms_routing) - if rule_actions and len(rule_actions.actions) > 0: + if rule_actions and len(rule_actions) > 0: actions = rule_actions break @@ -330,12 +329,12 @@ def evaluate_rules_priority_group( return best_status, inclusion_reasons, exclusion_reasons, is_rule_stop @staticmethod - def get_actions_from_comms(action_mapper: ActionsMapper, comms: str) -> SuggestedActions | None: - suggested_actions: SuggestedActions = SuggestedActions([]) + def get_actions_from_comms(action_mapper: ActionsMapper, comms: str) -> list[SuggestedAction] | None: + suggested_actions: list[SuggestedAction] = [] for comm in comms.split("|"): action = action_mapper.get(comm) if action is not None: - suggested_actions.actions.append( + suggested_actions.append( SuggestedAction( action_type=ActionType(action.action_type), action_code=ActionCode(action.action_code), diff --git a/src/eligibility_signposting_api/views/eligibility.py b/src/eligibility_signposting_api/views/eligibility.py index 27780f11b..a313319ae 100644 --- a/src/eligibility_signposting_api/views/eligibility.py +++ b/src/eligibility_signposting_api/views/eligibility.py @@ -117,7 +117,7 @@ def build_eligibility_response(eligibility_status: EligibilityStatus) -> eligibi def build_actions(condition: Condition) -> list[eligibility.Action] | None: - if condition.actions is not None and condition.actions.actions is not None: + if condition.actions is not None: return [ eligibility.Action( actionType=eligibility.ActionType(action.action_type), @@ -128,7 +128,7 @@ def build_actions(condition: Condition) -> list[eligibility.Action] | None: urlLink=eligibility.HttpUrl(action.url_link) if action.url_link is not None else None, urlLabel=eligibility.UrlLabel(action.url_label) if action.url_label is not None else None, ) - for action in condition.actions.actions + for action in condition.actions ] return None diff --git a/tests/fixtures/builders/model/eligibility.py b/tests/fixtures/builders/model/eligibility.py index 758b8b0dd..aef3b0eaf 100644 --- a/tests/fixtures/builders/model/eligibility.py +++ b/tests/fixtures/builders/model/eligibility.py @@ -13,12 +13,8 @@ class SuggestedActionFactory(DataclassFactory[eligibility.SuggestedAction]): url_link = UrlLink(HttpUrl("https://test-example.com")) -class SuggestedActionsFactory(DataclassFactory[eligibility.SuggestedActions]): - actions = Use(SuggestedActionFactory.batch, size=2) - - class ConditionFactory(DataclassFactory[eligibility.Condition]): - actions = SuggestedActionsFactory.build() + actions = Use(SuggestedActionFactory.batch, size=2) class EligibilityStatusFactory(DataclassFactory[eligibility.EligibilityStatus]): diff --git a/tests/unit/services/calculators/test_eligibility_calculator.py b/tests/unit/services/calculators/test_eligibility_calculator.py index 55f3ea41c..1ac57cbdb 100644 --- a/tests/unit/services/calculators/test_eligibility_calculator.py +++ b/tests/unit/services/calculators/test_eligibility_calculator.py @@ -20,7 +20,6 @@ RuleDescription, Status, SuggestedAction, - SuggestedActions, UrlLabel, UrlLink, ) @@ -426,10 +425,7 @@ def test_rule_types_cause_correct_statuses(rule_type: rules_model.RuleType, expe actual, is_eligibility_status().with_conditions( has_item( - is_condition() - .with_condition_name(ConditionName("RSV")) - .and_status(expected_status) - .and_actions(SuggestedActions([])) + is_condition().with_condition_name(ConditionName("RSV")).and_status(expected_status).and_actions([]) ) ), ) @@ -618,11 +614,11 @@ def test_multiple_conditions_where_both_are_actionable(faker: Faker): is_condition() .with_condition_name(ConditionName("RSV")) .and_status(Status.actionable) - .and_actions(SuggestedActions([suggested_action_for_default_comms])), + .and_actions([suggested_action_for_default_comms]), is_condition() .with_condition_name(ConditionName("COVID")) .and_status(Status.actionable) - .and_actions(SuggestedActions([suggested_action_for_book_nbs])), + .and_actions([suggested_action_for_book_nbs]), ) ), ) @@ -1778,7 +1774,7 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcomms", "InternalBookNBS", {"InternalBookNBS": book_nbs_comms, "defaultcomms": default_comms_detail}, - SuggestedActions([suggested_action_for_book_nbs]), + [suggested_action_for_book_nbs], ), ( """Rule match: default_comms_routing has multiple values, @@ -1786,7 +1782,7 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcomms1|defaultcomms2", None, {"defaultcomms1": default_comms_detail, "defaultcomms2": default_comms_detail}, - SuggestedActions([suggested_action_for_default_comms, suggested_action_for_default_comms]), + [suggested_action_for_default_comms, suggested_action_for_default_comms], ), ( """Rule match: default_comms_routing has multiple values, @@ -1794,7 +1790,7 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcomms1", "", {"defaultcomms1": default_comms_detail}, - SuggestedActions([suggested_action_for_default_comms]), + [suggested_action_for_default_comms], ), ( """Rule match: default_comms_routing present, @@ -1802,7 +1798,7 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcomms", "InternalBookNBS", {"defaultcomms": default_comms_detail}, - SuggestedActions([suggested_action_for_default_comms]), + [suggested_action_for_default_comms], ), ( """Rule match: default_comms_routing present, @@ -1810,7 +1806,7 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcomms", "InvalidCode", {"defaultcomms": default_comms_detail}, - SuggestedActions([suggested_action_for_default_comms]), + [suggested_action_for_default_comms], ), ( """Rule match: action_mapper present without url, @@ -1824,17 +1820,15 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( ActionDescription=book_nbs_comms.action_description, ) }, - SuggestedActions( - [ - SuggestedAction( - action_type=ActionType(book_nbs_comms.action_type), - action_code=ActionCode(book_nbs_comms.action_code), - action_description=ActionDescription(book_nbs_comms.action_description), - url_link=None, - url_label=None, - ) - ] - ), + [ + SuggestedAction( + action_type=ActionType(book_nbs_comms.action_type), + action_code=ActionCode(book_nbs_comms.action_code), + action_description=ActionDescription(book_nbs_comms.action_description), + url_link=None, + url_label=None, + ) + ], ), ( """Rule match: default_comms_routing missing, @@ -1842,7 +1836,7 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "", "InternalBookNBS", {}, - SuggestedActions([]), + [], ), ( """Rule match: default_comms_routing missing, but action_mapper present, @@ -1850,7 +1844,7 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "", "InternalBookNBS", {"InternalBookNBS": book_nbs_comms}, - SuggestedActions([suggested_action_for_book_nbs]), + [suggested_action_for_book_nbs], ), ( """Rule match: default_comms_routing present, @@ -1858,7 +1852,7 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcommskeywithoutactionmapper", "InternalBookNBS", {}, - SuggestedActions([]), + [], ), ( """Rule match: default_comms_routing has multiple values, @@ -1866,7 +1860,7 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcomms1|invaliddefault", None, {"defaultcomms1": default_comms_detail}, - SuggestedActions([suggested_action_for_default_comms]), + [suggested_action_for_default_comms], ), ], ) @@ -1875,7 +1869,7 @@ def test_correct_actions_determined_from_redirect_r_rules( # noqa: PLR0913 default_comms_routing: str, comms_routing: str, actions_mapper: ActionsMapper, - expected_actions: SuggestedActions, + expected_actions: list[SuggestedAction], faker: Faker, ): # Given @@ -1969,7 +1963,7 @@ def test_cohort_label_not_supported_used_in_r_rules(test_comment: str, redirect_ is_condition() .with_condition_name(ConditionName("RSV")) .and_status(equal_to(Status.actionable)) - .and_actions(equal_to(SuggestedActions([suggested_action_for_book_nbs]))) + .and_actions(equal_to([suggested_action_for_book_nbs])) ) ), test_comment, @@ -2025,7 +2019,7 @@ def test_multiple_r_rules_match_with_same_priority(faker: Faker): is_condition() .with_condition_name(ConditionName("RSV")) .and_status(equal_to(Status.actionable)) - .and_actions(equal_to(SuggestedActions([suggested_action_for_book_nbs]))) + .and_actions(equal_to([suggested_action_for_book_nbs])) ) ), ) @@ -2079,7 +2073,7 @@ def test_multiple_r_rules_with_same_priority_one_rule_mismatch_should_return_def is_condition() .with_condition_name(ConditionName("RSV")) .and_status(equal_to(Status.actionable)) - .and_actions(equal_to(SuggestedActions([suggested_action_for_default_comms]))) + .and_actions(equal_to([suggested_action_for_default_comms])) ) ), ) @@ -2146,7 +2140,7 @@ def test_only_highest_priority_rule_is_applied_and_return_actions_only_for_that_ is_condition() .with_condition_name(ConditionName("RSV")) .and_status(equal_to(Status.actionable)) - .and_actions(equal_to(SuggestedActions([expected_actions]))) + .and_actions(equal_to([expected_actions])) ) ), ) @@ -2193,7 +2187,7 @@ def test_should_include_actions_when_include_actions_flag_is_true_when_status_is is_condition() .with_condition_name(ConditionName("RSV")) .and_status(equal_to(Status.actionable)) - .and_actions(equal_to(SuggestedActions([suggested_action_for_book_nbs]))) + .and_actions(equal_to([suggested_action_for_book_nbs])) ) ), ) diff --git a/tests/unit/views/test_eligibility.py b/tests/unit/views/test_eligibility.py index 6d3e81f84..36fb887c5 100644 --- a/tests/unit/views/test_eligibility.py +++ b/tests/unit/views/test_eligibility.py @@ -28,7 +28,6 @@ RuleType, Status, SuggestedAction, - SuggestedActions, UrlLabel, UrlLink, ) @@ -373,17 +372,15 @@ def test_no_suitability_rules_for_actionable(): ("suggested_actions", "expected"), [ ( - SuggestedActions( - actions=[ - SuggestedAction( - action_type=ActionType("TYPE_A"), - action_code=ActionCode("CODE123"), - action_description=ActionDescription("Some description"), - url_link=UrlLink("https://example.com"), - url_label=UrlLabel("Learn more"), - ) - ] - ), + [ + SuggestedAction( + action_type=ActionType("TYPE_A"), + action_code=ActionCode("CODE123"), + action_description=ActionDescription("Some description"), + url_link=UrlLink(HttpUrl("https://example.com")), + url_label=UrlLabel("Learn more"), + ) + ], [ eligibility.Action( actionType=eligibility.ActionType("TYPE_A"), @@ -395,17 +392,15 @@ def test_no_suitability_rules_for_actionable(): ], ), ( - SuggestedActions( - actions=[ - SuggestedAction( - action_type=ActionType("TYPE_B"), - action_code=ActionCode("CODE123"), - action_description=None, - url_link=None, - url_label=None, - ) - ] - ), + [ + SuggestedAction( + action_type=ActionType("TYPE_B"), + action_code=ActionCode("CODE123"), + action_description=None, + url_link=None, + url_label=None, + ) + ], [ eligibility.Action( actionType=eligibility.ActionType("TYPE_B"), @@ -421,7 +416,7 @@ def test_no_suitability_rules_for_actionable(): None, ), ( - SuggestedActions(actions=[]), + [], [], ), ], From e4b45dcf898b8d34d27554ca507323ad2f621c43 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Fri, 4 Jul 2025 10:58:38 +0100 Subject: [PATCH 9/9] code cleanup --- tests/integration/lambda/test_app_running_as_lambda.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/integration/lambda/test_app_running_as_lambda.py b/tests/integration/lambda/test_app_running_as_lambda.py index cdd1149d3..000424f61 100644 --- a/tests/integration/lambda/test_app_running_as_lambda.py +++ b/tests/integration/lambda/test_app_running_as_lambda.py @@ -153,7 +153,7 @@ def get_log_messages(flask_function: str, logs_client: BaseClient) -> list[str]: return [e["message"] for e in log_events["events"]] -def test_given_nhs_number_in_path_matches_with_nhs_number_in_headers( # noqa: PLR0913 +def test_given_nhs_number_in_path_matches_with_nhs_number_in_headers_and_check_if_audited( # noqa: PLR0913 lambda_client: BaseClient, # noqa:ARG001 persisted_person: NHSNumber, campaign_config: CampaignConfig, # noqa:ARG001 @@ -176,6 +176,7 @@ def test_given_nhs_number_in_path_matches_with_nhs_number_in_headers( # noqa: P is_response().with_status_code(HTTPStatus.OK).and_body(is_json_that(has_key("processedSuggestions"))), ) + # Then - check if audited objects = s3_client.list_objects_v2(Bucket=audit_bucket).get("Contents", []) object_keys = [obj["Key"] for obj in objects] latest_key = sorted(object_keys)[-1]