From b19f129c4c63029b54568b77f3dfd4959e52ae88 Mon Sep 17 00:00:00 2001 From: ayeshalshukri1-nhs <112615598+ayeshalshukri1-nhs@users.noreply.github.com> Date: Wed, 25 Jun 2025 17:00:02 +0100 Subject: [PATCH 01/21] Patch - Initial test and AuditService stub. --- .../services/audit_service.py | 44 +++++++++++++++++++ tests/unit/test_audit_logging.py | 34 ++++++++++++++ 2 files changed, 78 insertions(+) create mode 100644 src/eligibility_signposting_api/services/audit_service.py create mode 100644 tests/unit/test_audit_logging.py diff --git a/src/eligibility_signposting_api/services/audit_service.py b/src/eligibility_signposting_api/services/audit_service.py new file mode 100644 index 000000000..14a9468ec --- /dev/null +++ b/src/eligibility_signposting_api/services/audit_service.py @@ -0,0 +1,44 @@ +from dataclasses import dataclass +from datetime import datetime + +from mangum.types import LambdaEvent, LambdaContext + + +@dataclass +class RequestAuditHeader: + xRequestID: str + xCorrelationID: str + nhsdEndUserOrganisationODS: str + + +@dataclass +class RequestAuditQueryParams: + category: str | None + conditions: str | None + include_actions: str | None + + +@dataclass +class RequestAuditData: + nhs_number: int + request_timestamp: str + headers: RequestAuditHeader + query_params: RequestAuditQueryParams + + +class AuditService: + + def create_request_audit_data(self, event: LambdaEvent) -> RequestAuditData: + headers = event.get("headers", {}) + path_params = event.get("pathParameters", {}) + query_params = event.get("queryStringParameters", {}) + + request_context = event.get("requestContext", {}) + return RequestAuditData(int(path_params.get("id")), + request_context.get("time", datetime.now().strftime("%d/%b/%Y:%H:%M:%S %z")), #TODO: check timestamp format + RequestAuditHeader(headers.get("X-Request-ID"), + headers.get("X-Correlation-ID"), + headers.get("NHSD-End-User-Organisation-ODS")), + RequestAuditQueryParams(query_params.get("category"), + query_params.get("conditions"), + query_params.get("include_actions"))) diff --git a/tests/unit/test_audit_logging.py b/tests/unit/test_audit_logging.py new file mode 100644 index 000000000..d5c9b70bb --- /dev/null +++ b/tests/unit/test_audit_logging.py @@ -0,0 +1,34 @@ +from eligibility_signposting_api.services.audit_service import RequestAuditData, RequestAuditHeader, \ + RequestAuditQueryParams, AuditService + +from mangum.types import LambdaEvent + +def test_audit_data_object_populates_response_summary(): + mock_event: LambdaEvent = { + "pathParameters": {"id": "1112223334"}, + "headers": { + "X-Request-ID": "request-uuid", + "X-Correlation-ID": "correlation-uuid", + "NHSD-End-User-Organisation-ODS": "nhs-ods-code-123" + }, + "queryStringParameters": { + "conditions": "RSV", + "category": "Vaccinations", + "include_actions": "Y" + }, + "requestContext": {"time": "01/Jan/2025:00:00:00 +0000"} + } + + header = RequestAuditHeader(xRequestID="request-uuid", xCorrelationID="correlation-uuid", + nhsdEndUserOrganisationODS="nhs-ods-code-123") + + query_params = RequestAuditQueryParams(category="Vaccinations", conditions="RSV", include_actions="Y") + + expected_audit_entry = RequestAuditData(nhs_number=1112223334, request_timestamp="01/Jan/2025:00:00:00 +0000", + headers=header, query_params=query_params) + + audit_service = AuditService() + actual_audit_entry = audit_service.create_request_audit_data(mock_event) + + assert actual_audit_entry == expected_audit_entry + From 7f96315fdd2e4df051e9f02b15b7afb3c87a8fe7 Mon Sep 17 00:00:00 2001 From: ayeshalshukri1-nhs <112615598+ayeshalshukri1-nhs@users.noreply.github.com> Date: Fri, 27 Jun 2025 15:56:30 +0100 Subject: [PATCH 02/21] Patch - Commit 1 Adds flask g and before and after request audit --- src/eligibility_signposting_api/app.py | 18 +++++++- .../services/audit_service.py | 44 ------------------- .../calculators/eligibility_calculator.py | 4 ++ tests/unit/test_audit_logging.py | 2 +- 4 files changed, 21 insertions(+), 47 deletions(-) delete mode 100644 src/eligibility_signposting_api/services/audit_service.py diff --git a/src/eligibility_signposting_api/app.py b/src/eligibility_signposting_api/app.py index 2cf614bea..0eca87376 100644 --- a/src/eligibility_signposting_api/app.py +++ b/src/eligibility_signposting_api/app.py @@ -3,11 +3,12 @@ import wireup.integration.flask from asgiref.wsgi import WsgiToAsgi -from flask import Flask +from flask import Flask, request from mangum import Mangum from mangum.types import LambdaContext, LambdaEvent from eligibility_signposting_api import repos, services +from eligibility_signposting_api.audit.audit_service import AuditService from eligibility_signposting_api.config.config import config, init_logging from eligibility_signposting_api.error_handler import handle_exception from eligibility_signposting_api.views import eligibility_blueprint @@ -16,6 +17,8 @@ init_logging() logger = logging.getLogger(__name__) +app = Flask(__name__) + def main() -> None: # pragma: no cover """Run the Flask app as a local process.""" @@ -23,6 +26,18 @@ def main() -> None: # pragma: no cover app.run(debug=config()["log_level"] == logging.DEBUG) +@app.before_request +def request_audit(): + AuditService.add_request_details(request) + + +@app.after_request +def response_audit(response): + AuditService.add_response_details(response) + # AuditService.audit(asdict(g.audit_log)) + return response + + @validate_matching_nhs_number() def lambda_handler(event: LambdaEvent, context: LambdaContext) -> dict[str, Any]: # pragma: no cover """Run the Flask app as an AWS Lambda.""" @@ -33,7 +48,6 @@ def lambda_handler(event: LambdaEvent, context: LambdaContext) -> dict[str, Any] def create_app() -> Flask: - app = Flask(__name__) logger.info("app created") # Register views & error handler diff --git a/src/eligibility_signposting_api/services/audit_service.py b/src/eligibility_signposting_api/services/audit_service.py deleted file mode 100644 index 14a9468ec..000000000 --- a/src/eligibility_signposting_api/services/audit_service.py +++ /dev/null @@ -1,44 +0,0 @@ -from dataclasses import dataclass -from datetime import datetime - -from mangum.types import LambdaEvent, LambdaContext - - -@dataclass -class RequestAuditHeader: - xRequestID: str - xCorrelationID: str - nhsdEndUserOrganisationODS: str - - -@dataclass -class RequestAuditQueryParams: - category: str | None - conditions: str | None - include_actions: str | None - - -@dataclass -class RequestAuditData: - nhs_number: int - request_timestamp: str - headers: RequestAuditHeader - query_params: RequestAuditQueryParams - - -class AuditService: - - def create_request_audit_data(self, event: LambdaEvent) -> RequestAuditData: - headers = event.get("headers", {}) - path_params = event.get("pathParameters", {}) - query_params = event.get("queryStringParameters", {}) - - request_context = event.get("requestContext", {}) - return RequestAuditData(int(path_params.get("id")), - request_context.get("time", datetime.now().strftime("%d/%b/%Y:%H:%M:%S %z")), #TODO: check timestamp format - RequestAuditHeader(headers.get("X-Request-ID"), - headers.get("X-Correlation-ID"), - headers.get("NHSD-End-User-Organisation-ODS")), - RequestAuditQueryParams(query_params.get("category"), - query_params.get("conditions"), - query_params.get("include_actions"))) diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index 304261a04..d89885b03 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -7,6 +7,8 @@ from itertools import groupby from typing import TYPE_CHECKING, Any +from flask import g + if TYPE_CHECKING: from eligibility_signposting_api.model.rules import ActionsMapper, Iteration, IterationCohort @@ -132,6 +134,8 @@ def evaluate_eligibility(self, *, include_actions_flag: bool = True) -> eligibil actions: SuggestedActions | None = SuggestedActions([]) for condition_name, campaign_group in self.campaigns_grouped_by_condition_name: + g.audit_log.response.responseId = condition_name + iteration_results: dict[str, tuple[Iteration, IterationResult]] = {} for active_iteration in [cc.current_iteration for cc in campaign_group]: diff --git a/tests/unit/test_audit_logging.py b/tests/unit/test_audit_logging.py index d5c9b70bb..b85ef1f6f 100644 --- a/tests/unit/test_audit_logging.py +++ b/tests/unit/test_audit_logging.py @@ -1,4 +1,4 @@ -from eligibility_signposting_api.services.audit_service import RequestAuditData, RequestAuditHeader, \ +from eligibility_signposting_api.audit.audit_service import RequestAuditData, RequestAuditHeader, \ RequestAuditQueryParams, AuditService from mangum.types import LambdaEvent From 02c98a2d2fead47bd687dcaad1615633dbd251d8 Mon Sep 17 00:00:00 2001 From: ayeshalshukri1-nhs <112615598+ayeshalshukri1-nhs@users.noreply.github.com> Date: Mon, 30 Jun 2025 16:59:07 +0100 Subject: [PATCH 03/21] Patch - Added audit record logic. --- src/eligibility_signposting_api/app.py | 23 +++-- .../audit_models.py | 82 +++++++++++++++ .../audit_service.py | 99 +++++++++++++++++++ .../model/eligibility.py | 4 + .../calculators/eligibility_calculator.py | 65 +++++++++--- .../services/calculators/rule_calculator.py | 1 + .../services/eligibility_services.py | 4 +- .../views/eligibility.py | 4 + tests/unit/test_audit_logging.py | 2 +- 9 files changed, 256 insertions(+), 28 deletions(-) create mode 100644 src/eligibility_signposting_api/audit_models.py create mode 100644 src/eligibility_signposting_api/audit_service.py diff --git a/src/eligibility_signposting_api/app.py b/src/eligibility_signposting_api/app.py index 0eca87376..1e2a5d6ab 100644 --- a/src/eligibility_signposting_api/app.py +++ b/src/eligibility_signposting_api/app.py @@ -8,7 +8,6 @@ from mangum.types import LambdaContext, LambdaEvent from eligibility_signposting_api import repos, services -from eligibility_signposting_api.audit.audit_service import AuditService from eligibility_signposting_api.config.config import config, init_logging from eligibility_signposting_api.error_handler import handle_exception from eligibility_signposting_api.views import eligibility_blueprint @@ -17,7 +16,7 @@ init_logging() logger = logging.getLogger(__name__) -app = Flask(__name__) +# app = Flask(__name__) def main() -> None: # pragma: no cover @@ -26,16 +25,15 @@ def main() -> None: # pragma: no cover app.run(debug=config()["log_level"] == logging.DEBUG) -@app.before_request -def request_audit(): - AuditService.add_request_details(request) - - -@app.after_request -def response_audit(response): - AuditService.add_response_details(response) - # AuditService.audit(asdict(g.audit_log)) - return response +# @app.before_request +# def request_audit(): +# AuditService.add_request_details(request) +# +# +# @app.after_request +# def response_audit(response): +# AuditService.audit(asdict(g.audit_log)) +# return response @validate_matching_nhs_number() @@ -48,6 +46,7 @@ def lambda_handler(event: LambdaEvent, context: LambdaContext) -> dict[str, Any] def create_app() -> Flask: + app = Flask(__name__) logger.info("app created") # Register views & error handler diff --git a/src/eligibility_signposting_api/audit_models.py b/src/eligibility_signposting_api/audit_models.py new file mode 100644 index 000000000..c9e646ddf --- /dev/null +++ b/src/eligibility_signposting_api/audit_models.py @@ -0,0 +1,82 @@ +from dataclasses import dataclass, field +from datetime import datetime +from typing import List + + +@dataclass +class RequestAuditHeader: + xRequestID: str + xCorrelationID: str + nhsdEndUserOrganisationODS: str + nhsdApplicationID: str + +@dataclass +class RequestAuditQueryParams: + category: str | None = None + conditions: str | None = None + includeActions: str | None = None + +@dataclass +class RequestAuditData: + requestTimestamp: datetime + headers: RequestAuditHeader + queryParams: RequestAuditQueryParams + nhsNumber: int + +@dataclass +class AuditEligibilityCohorts: + cohortCode: str | None = None + cohortStatus: str | None = None + +@dataclass +class AuditEligibilityCohortGroups: + cohortCode: str | None = None + cohortText: str | None = None + cohortStatus: str | None = None + +@dataclass +class AuditFilterRule: + rulePriority: int | None = None + ruleName: str | None = None + +@dataclass +class AuditSuitabilityRule: + rulePriority: int | None = None + ruleName: str | None = None + ruleMessage: str | None = None + +@dataclass +class AuditRedirectRule: + rulePriority: int | None = None + ruleName: str | None = None + +@dataclass +class AuditAction: + internalName: str | None = None + actionType: str | None = None + actionCode: str | None = None + actionDescription: str | None = None + actionUrl: str | None = None + actionUrlLabel: str | None = None + +@dataclass +class AuditCondition: + campaignID: str + campaignVersion: int + iterationID: str + iterationVersion: int + conditionName: str + status: str + statusText: str + eligibilityCohorts: List[AuditEligibilityCohorts] | None = None + eligibilityCohortGroups: List[AuditEligibilityCohortGroups] | None = None + filterRules: AuditFilterRule | None = None + suitabilityRules: AuditSuitabilityRule | None = None + actionRule: AuditRedirectRule | None = None + actions: List[AuditAction] = field(default_factory=list) + +@dataclass +class ResponseAuditData: + responseId: str | None = None + lastUpdated: str | None = None + condition: List[AuditCondition] = field(default_factory=list) diff --git a/src/eligibility_signposting_api/audit_service.py b/src/eligibility_signposting_api/audit_service.py new file mode 100644 index 000000000..6592ecc2a --- /dev/null +++ b/src/eligibility_signposting_api/audit_service.py @@ -0,0 +1,99 @@ +import logging +from datetime import datetime +from itertools import groupby +from operator import attrgetter + +from eligibility_signposting_api.model.eligibility import CohortGroupResult, Status +from eligibility_signposting_api.audit_models import RequestAuditData, RequestAuditHeader, RequestAuditQueryParams, \ + AuditEligibilityCohorts, AuditEligibilityCohortGroups, AuditAction, AuditCondition, AuditRedirectRule, \ + AuditFilterRule, AuditSuitabilityRule + +logger = logging.getLogger(__name__) + +from flask import Request, g, Response + + +class AuditService: + + @staticmethod + def add_request_details(request: Request) -> None: + resource_id = None + if 'id' in request.view_args: + try: + resource_id = int(request.view_args['id']) + except (ValueError, TypeError): + logger.warning(f"Could not parse 'id' from path parameters: {request.view_args.get('id')}") + g.audit_log.request = RequestAuditData( + nhsNumber=resource_id, + requestTimestamp=datetime.now(), + headers=(RequestAuditHeader( + xRequestID=request.headers.get("X-Request-ID"), + xCorrelationID=request.headers.get("X-Correlation-ID"), + nhsdEndUserOrganisationODS=request.headers.get("NHSD-End-User-Organisation-ODS"), + nhsdApplicationID=request.headers.get("nhsd-application-id") + )), + queryParams=(RequestAuditQueryParams( + category=request.args.get("category"), + conditions=request.args.get("conditions"), + includeActions=request.args.get("include_actions") + )), + ) + + + @staticmethod + def add_response_details(response: Response) -> None: + pass + + @staticmethod + def append_audit_condition(suggested_actions, best_active_iteration, best_candidate, campaign_id, campaign_version, + condition_name, best_cohort_results: dict[str, CohortGroupResult], priority, name): + + audit_eligibility_cohorts, audit_eligibility_cohort_groups, audit_actions = [], [], [] + audit_filter_rule, audit_suitability_rule, audit_redirect_rule = None, None, None + + for value in sorted(best_cohort_results.values(), key=attrgetter("cohort_code")): + audit_eligibility_cohorts.append( + AuditEligibilityCohorts(cohortCode=value.cohort_code, cohortStatus=value.status.name)) + + audit_eligibility_cohort_groups.append( + AuditEligibilityCohortGroups(cohortCode=value.cohort_code, cohortStatus=value.status.name, + cohortText=value.description)) + + # TODO: what if value.audit_reasons is empty? + if value.audit_reasons: + if best_candidate.status.name == Status.not_eligible.name: + audit_filter_rule = AuditFilterRule(rulePriority=value.audit_reasons[0].rule_priority, + ruleName=value.audit_reasons[0].rule_name) + if best_candidate.status.name == Status.not_actionable.name: + audit_suitability_rule = AuditSuitabilityRule(rulePriority=value.audit_reasons[0].rule_priority, + ruleName=value.audit_reasons[0].rule_name, + ruleMessage=value.audit_reasons[0].rule_description) + + if best_candidate.status.name == Status.actionable.name: + audit_redirect_rule = AuditRedirectRule(priority, name) + + if suggested_actions is None or suggested_actions == []: + audit_actions = suggested_actions + + elif len(suggested_actions.actions) > 0: + for action in suggested_actions.actions: + audit_actions.append( + AuditAction(actionCode=action.action_code, actionDescription=action.action_description)) + + audit_condition = AuditCondition(campaignID=campaign_id, + campaignVersion=campaign_version, # TODO: Convert to int + iterationID=best_active_iteration.id, + iterationVersion=best_active_iteration.version, # TODO: Convert to int + conditionName=condition_name, + status=best_candidate.status.name, + statusText=best_candidate.status.name, # Same as status per source code + eligibilityCohorts=audit_eligibility_cohorts, + eligibilityCohortGroups=audit_eligibility_cohort_groups, + filterRules=audit_filter_rule, + suitabilityRules=audit_suitability_rule, + actionRule=audit_redirect_rule, + actions=audit_actions) + + print(">>> audit_condition:", audit_condition) + + # g.audit_log.response.condition.append(audit_condition) diff --git a/src/eligibility_signposting_api/model/eligibility.py b/src/eligibility_signposting_api/model/eligibility.py index 7e3582c3b..ac8261683 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 eligibility_signposting_api.model.rules import RulePriority + NHSNumber = NewType("NHSNumber", str) DateOfBirth = NewType("DateOfBirth", date) Postcode = NewType("Postcode", str) @@ -66,6 +68,7 @@ def best(*statuses: Status) -> Status: class Reason: rule_type: RuleType rule_name: RuleName + rule_priority: RulePriority rule_description: RuleDescription | None matcher_matched: bool @@ -98,6 +101,7 @@ class CohortGroupResult: status: Status reasons: list[Reason] description: str | None + audit_reasons: list[Reason] @dataclass diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index d89885b03..2982600f7 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -7,10 +7,11 @@ from itertools import groupby from typing import TYPE_CHECKING, Any -from flask import g +from eligibility_signposting_api.audit_service import AuditService if TYPE_CHECKING: - from eligibility_signposting_api.model.rules import ActionsMapper, Iteration, IterationCohort + from eligibility_signposting_api.model.rules import ActionsMapper, Iteration, IterationCohort, CampaignID, \ + CampaignVersion, RulePriority, RuleName from wireup import service @@ -89,6 +90,7 @@ def get_the_best_cohort_memberships( status=cc.status, reasons=cc.reasons, description=(cc.description or "").strip() if cc.description else "", + audit_reasons=cc.audit_reasons, #TODO: remove this or fix it! ) for cc in best_cohorts ] @@ -103,8 +105,8 @@ def get_exclusion_rules( ir for ir in filter_rules if ir.cohort_label is None - or cohort.cohort_label == ir.cohort_label - or (isinstance(ir.cohort_label, (list, set, tuple)) and cohort.cohort_label in ir.cohort_label) + or cohort.cohort_label == ir.cohort_label + or (isinstance(ir.cohort_label, (list, set, tuple)) and cohort.cohort_label in ir.cohort_label) ) @staticmethod @@ -132,13 +134,16 @@ def evaluate_eligibility(self, *, include_actions_flag: bool = True) -> eligibil """Iterates over campaign groups, evaluates eligibility, and returns a consolidated status.""" condition_results: dict[ConditionName, IterationResult] = {} actions: SuggestedActions | None = SuggestedActions([]) + redirect_rule_priority, redirect_rule_name = None, None + # g.audit_log.response = ResponseAuditData() + # g.audit_log.response.condition = [] for condition_name, campaign_group in self.campaigns_grouped_by_condition_name: - g.audit_log.response.responseId = condition_name + # audit_condition = AuditCondition() + iteration_results: dict[str, tuple[Iteration, IterationResult, CampaignID, CampaignVersion, dict[str, CohortGroupResult]]] = {} - iteration_results: dict[str, tuple[Iteration, IterationResult]] = {} - - for active_iteration in [cc.current_iteration for cc in campaign_group]: + for cc in campaign_group: + active_iteration = cc.current_iteration cohort_results: dict[str, CohortGroupResult] = self.get_cohort_results(active_iteration) # Determine Result between cohorts - get the best @@ -146,35 +151,61 @@ def evaluate_eligibility(self, *, include_actions_flag: bool = True) -> eligibil iteration_results[active_iteration.name] = ( active_iteration, IterationResult(status, best_cohorts, actions), + cc.id, + cc.version, + cohort_results ) # Determine results between iterations - get the best if iteration_results: - best_iteration_name, (best_active_iteration, best_candidate) = max( + best_iteration_name, (best_active_iteration, best_candidate, best_campaign_id, + best_campaign_version, best_cohort_results) = max( iteration_results.items(), key=lambda item: item[1][1].status.value ) else: best_candidate = IterationResult(eligibility.Status.not_eligible, [], actions) + best_campaign_id = None + best_campaign_version = None best_active_iteration = None + best_cohort_results = None + condition_results[condition_name] = best_candidate if best_candidate.status == Status.actionable and best_active_iteration is not None: - actions = self.handle_redirect_rules(best_active_iteration) if include_actions_flag else None + if include_actions_flag: + actions, matched_r_rule_priority, matched_r_rule_name = self.handle_redirect_rules( + best_active_iteration) + redirect_rule_name = matched_r_rule_name + redirect_rule_priority = matched_r_rule_priority + else: + actions = None + if best_candidate.status in (Status.not_eligible, Status.not_actionable) and not include_actions_flag: actions = None # add actions to condition results condition_results[condition_name].actions = actions + + # add audit data + #TODO: Do we need to use deduplicated cohort results from build_condition_results instead of here? + AuditService.append_audit_condition(actions, best_active_iteration, + best_candidate, best_campaign_id, + best_campaign_version, + condition_name, best_cohort_results, + redirect_rule_priority, redirect_rule_name) + # Consolidate all the results and return 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) -> tuple[ + SuggestedActions | None, RulePriority | None, RuleName | 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) + matched_redirect_rule_priority, matched_redirect_rule_name = None, None for _, rule_group in groupby(sorted_rules_by_priority, key=priority_getter): rule_group_list = list(rule_group) matcher_matched_list = [ @@ -187,9 +218,11 @@ def handle_redirect_rules(self, best_active_iteration: Iteration) -> SuggestedAc rule_actions = self.get_actions_from_comms(action_mapper, comms_routing) if rule_actions and len(rule_actions.actions) > 0: actions = rule_actions + matched_redirect_rule_priority = rule_group_list[0].priority + matched_redirect_rule_name = rule_group_list[0].name break - return actions + return actions, matched_redirect_rule_priority, matched_redirect_rule_name def get_cohort_results(self, active_iteration: rules.Iteration) -> dict[str, CohortGroupResult]: cohort_results: dict[str, CohortGroupResult] = {} @@ -209,6 +242,7 @@ def get_cohort_results(self, active_iteration: rules.Iteration) -> dict[str, Coh Status.not_eligible, [], cohort.negative_description, + [] #TODO: remove this or fix it! ) return cohort_results @@ -232,6 +266,7 @@ def build_condition_results(condition_results: dict[ConditionName, IterationResu reasons=[reason for cohort in group for reason in cohort.reasons], # get the first nonempty description description=next((c.description for c in group if c.description), group[0].description), + audit_reasons=[] #TODO: remove this or fix it! ) for group_cohort_code, group in grouped_cohort_results.items() if group @@ -269,6 +304,7 @@ def is_eligible_by_filter_rules( Status.not_eligible, [], cohort.negative_description, + group_exclusion_reasons, ) is_eligible = False break @@ -304,6 +340,7 @@ def evaluate_suppression_rules( Status.actionable, [], cohort.positive_description, + suppression_reasons ) else: cohort_results[key] = CohortGroupResult( @@ -311,6 +348,7 @@ def evaluate_suppression_rules( Status.not_actionable, suppression_reasons, cohort.positive_description, + suppression_reasons ) def evaluate_rules_priority_group( @@ -344,8 +382,7 @@ def get_actions_from_comms(action_mapper: ActionsMapper, comms: str) -> Suggeste action_type=ActionType(action.action_type), action_code=ActionCode(action.action_code), action_description=ActionDescription(action.action_description) - if action.action_description - else None, + if action.action_description else None, url_link=UrlLink(action.url_link) if action.url_link else None, url_label=UrlLabel(action.url_label) if action.url_label else None, ) diff --git a/src/eligibility_signposting_api/services/calculators/rule_calculator.py b/src/eligibility_signposting_api/services/calculators/rule_calculator.py index 5834c678e..c24fb591b 100644 --- a/src/eligibility_signposting_api/services/calculators/rule_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/rule_calculator.py @@ -24,6 +24,7 @@ def evaluate_exclusion(self) -> tuple[eligibility.Status, eligibility.Reason]: reason = eligibility.Reason( rule_name=eligibility.RuleName(self.rule.name), rule_type=eligibility.RuleType(self.rule.type), + rule_priority=eligibility.RulePriority(self.rule.priority), rule_description=eligibility.RuleDescription(self.rule.description), matcher_matched=matcher_matched, ) diff --git a/src/eligibility_signposting_api/services/eligibility_services.py b/src/eligibility_signposting_api/services/eligibility_services.py index c8d9c5b50..f22a0c991 100644 --- a/src/eligibility_signposting_api/services/eligibility_services.py +++ b/src/eligibility_signposting_api/services/eligibility_services.py @@ -51,6 +51,8 @@ def get_eligibility_status( raise UnknownPersonError from e else: calc: calculator.EligibilityCalculator = self.calculator_factory.get(person_data, campaign_configs) - return calc.evaluate_eligibility(include_actions_flag=include_actions_flag) + evaluate_eligibility = calc.evaluate_eligibility(include_actions_flag=include_actions_flag) + # audit_service.audit(evaluate_eligibility) + return evaluate_eligibility raise UnknownPersonError # pragma: no cover diff --git a/src/eligibility_signposting_api/views/eligibility.py b/src/eligibility_signposting_api/views/eligibility.py index aafe65134..aecda767c 100644 --- a/src/eligibility_signposting_api/views/eligibility.py +++ b/src/eligibility_signposting_api/views/eligibility.py @@ -25,6 +25,10 @@ eligibility_blueprint = Blueprint("eligibility", __name__) +@eligibility_blueprint.before_request +def store_request_id(): + rid = request.headers.get("X-Request-ID", "unknown") + # request_id_var.set(rid) @eligibility_blueprint.get("/", defaults={"nhs_number": ""}) @eligibility_blueprint.get("/") diff --git a/tests/unit/test_audit_logging.py b/tests/unit/test_audit_logging.py index b85ef1f6f..c88f4069f 100644 --- a/tests/unit/test_audit_logging.py +++ b/tests/unit/test_audit_logging.py @@ -1,4 +1,4 @@ -from eligibility_signposting_api.audit.audit_service import RequestAuditData, RequestAuditHeader, \ +from eligibility_signposting_api.audit_service import RequestAuditData, RequestAuditHeader, \ RequestAuditQueryParams, AuditService from mangum.types import LambdaEvent From 45c99e1333ce244b6da14e30ea5161cee69ab938 Mon Sep 17 00:00:00 2001 From: ayeshalshukri1-nhs <112615598+ayeshalshukri1-nhs@users.noreply.github.com> Date: Mon, 30 Jun 2025 17:05:57 +0100 Subject: [PATCH 04/21] Patch - Fix broken tests. --- tests/unit/views/test_eligibility.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/unit/views/test_eligibility.py b/tests/unit/views/test_eligibility.py index 56e1becb2..126dcd56a 100644 --- a/tests/unit/views/test_eligibility.py +++ b/tests/unit/views/test_eligibility.py @@ -21,6 +21,7 @@ RuleType, Status, ) +from eligibility_signposting_api.model.rules import RulePriority from eligibility_signposting_api.services import EligibilityService, UnknownPersonError from eligibility_signposting_api.services.eligibility_services import InvalidQueryParamError from eligibility_signposting_api.views.eligibility import ( @@ -212,18 +213,21 @@ def test_build_suitability_results_with_deduplication(): rule_name=RuleName("Exclude too young less than 75"), rule_description=RuleDescription("your age is greater than 75"), matcher_matched=False, + rule_priority=RulePriority(1) ), Reason( rule_type=RuleType.suppression, rule_name=RuleName("Exclude too young less than 75"), rule_description=RuleDescription("your age is greater than 75"), matcher_matched=False, + rule_priority=RulePriority(1) ), Reason( rule_type=RuleType.suppression, rule_name=RuleName("Exclude more than 100"), rule_description=RuleDescription("your age is greater than 100"), matcher_matched=False, + rule_priority=RulePriority(1) ), ], ), @@ -236,6 +240,7 @@ def test_build_suitability_results_with_deduplication(): rule_name=RuleName("Exclude too young less than 75"), rule_description=RuleDescription("your age is greater than 75"), matcher_matched=False, + rule_priority=RulePriority(1) ) ], ), @@ -248,6 +253,7 @@ def test_build_suitability_results_with_deduplication(): rule_name=RuleName("Exclude is present in sw1"), rule_description=RuleDescription("your a member of sw1"), matcher_matched=False, + rule_priority=RulePriority(1) ) ], ), @@ -261,6 +267,7 @@ def test_build_suitability_results_with_deduplication(): rule_name=RuleName("Already vaccinated"), rule_description=RuleDescription("you have already vaccinated"), matcher_matched=False, + rule_priority=RulePriority(1) ) ], ), @@ -294,18 +301,21 @@ def test_build_suitability_results_when_rule_text_is_empty_or_null(): rule_name=RuleName("Exclude too young less than 75"), rule_description=RuleDescription("your age is greater than 75"), matcher_matched=False, + rule_priority=RulePriority(1) ), Reason( rule_type=RuleType.suppression, rule_name=RuleName("Exclude more than 100"), rule_description=RuleDescription(""), matcher_matched=False, + rule_priority=RulePriority(1) ), Reason( rule_type=RuleType.suppression, rule_name=RuleName("Exclude more than 100"), matcher_matched=False, rule_description=None, + rule_priority=RulePriority(1) ), ], ), @@ -318,6 +328,7 @@ def test_build_suitability_results_when_rule_text_is_empty_or_null(): rule_name=RuleName("Exclude is present in sw1"), rule_description=RuleDescription(""), matcher_matched=False, + rule_priority=RulePriority(1) ) ], ), @@ -330,6 +341,7 @@ def test_build_suitability_results_when_rule_text_is_empty_or_null(): rule_name=RuleName("Exclude is present in sw1"), rule_description=None, matcher_matched=False, + rule_priority=RulePriority(1) ) ], ), From 947393f4c9a2606503eab108ac7b31281238f48b Mon Sep 17 00:00:00 2001 From: Shweta <216860557+shweta-nhs@users.noreply.github.com> Date: Thu, 3 Jul 2025 11:18:44 +0100 Subject: [PATCH 05/21] Adding integration test for audit --- .../audit_models.py | 103 ++++++----- .../audit_service.py | 163 +++++++++++------- .../model/eligibility.py | 3 +- .../services/audit_service.py | 3 +- .../calculators/eligibility_calculator.py | 80 +++++---- .../services/eligibility_services.py | 4 - .../views/eligibility.py | 16 +- tests/conftest.py | 11 +- tests/integration/conftest.py | 65 +++++++ .../lambda/test_app_running_as_lambda.py | 112 +++++++++++- .../services/test_eligibility_services.py | 7 +- tests/unit/test_audit_logging.py | 34 ---- tests/unit/views/test_eligibility.py | 45 +++-- 13 files changed, 436 insertions(+), 210 deletions(-) delete mode 100644 tests/unit/test_audit_logging.py diff --git a/src/eligibility_signposting_api/audit_models.py b/src/eligibility_signposting_api/audit_models.py index c9e646ddf..dd84e4b27 100644 --- a/src/eligibility_signposting_api/audit_models.py +++ b/src/eligibility_signposting_api/audit_models.py @@ -1,82 +1,97 @@ from dataclasses import dataclass, field from datetime import datetime -from typing import List @dataclass class RequestAuditHeader: - xRequestID: str - xCorrelationID: str - nhsdEndUserOrganisationODS: str - nhsdApplicationID: str + x_request_id: str = None + x_correlation_id: str = None + nhsd_end_user_organisation_ods: str = None + nhsd_application_id: str = None + @dataclass class RequestAuditQueryParams: category: str | None = None conditions: str | None = None - includeActions: str | None = None + include_actions: str | None = None + @dataclass class RequestAuditData: - requestTimestamp: datetime - headers: RequestAuditHeader - queryParams: RequestAuditQueryParams - nhsNumber: int + request_timestamp: datetime = field(default_factory=datetime.utcnow) + headers: RequestAuditHeader = field(default_factory=RequestAuditHeader) + query_params: RequestAuditQueryParams = field(default_factory=RequestAuditQueryParams) + nhs_number: int | None = None + @dataclass class AuditEligibilityCohorts: - cohortCode: str | None = None - cohortStatus: str | None = None + cohort_code: str | None = None + cohort_status: str | None = None + @dataclass class AuditEligibilityCohortGroups: - cohortCode: str | None = None - cohortText: str | None = None - cohortStatus: str | None = None + cohort_code: str | None = None + cohort_text: str | None = None + cohort_status: str | None = None + @dataclass class AuditFilterRule: - rulePriority: int | None = None - ruleName: str | None = None + rule_priority: int | None = None + rule_name: str | None = None + @dataclass class AuditSuitabilityRule: - rulePriority: int | None = None - ruleName: str | None = None - ruleMessage: str | None = None + rule_priority: int | None = None + rule_name: str | None = None + rule_message: str | None = None + @dataclass class AuditRedirectRule: - rulePriority: int | None = None - ruleName: str | None = None + rule_priority: int | None = None + rule_name: str | None = None + @dataclass class AuditAction: - internalName: str | None = None - actionType: str | None = None - actionCode: str | None = None - actionDescription: str | None = None - actionUrl: str | None = None - actionUrlLabel: str | None = None + internal_name: str | None = None + action_type: str | None = None + action_code: str | None = None + action_description: str | None = None + action_url: str | None = None + action_url_label: str | None = None + @dataclass class AuditCondition: - campaignID: str - campaignVersion: int - iterationID: str - iterationVersion: int - conditionName: str - status: str - statusText: str - eligibilityCohorts: List[AuditEligibilityCohorts] | None = None - eligibilityCohortGroups: List[AuditEligibilityCohortGroups] | None = None - filterRules: AuditFilterRule | None = None - suitabilityRules: AuditSuitabilityRule | None = None - actionRule: AuditRedirectRule | None = None - actions: List[AuditAction] = field(default_factory=list) + campaign_id: str = None + campaign_version: str = None + iteration_id: str = None + iteration_version: str = None + condition_name: str = None + status: str = None + status_text: str = None + eligibility_cohorts: list[AuditEligibilityCohorts] | None = None + eligibility_cohort_groups: list[AuditEligibilityCohortGroups] | None = None + filter_rules: AuditFilterRule | None = None + suitability_rules: AuditSuitabilityRule | None = None + action_rule: AuditRedirectRule | None = None + actions: list[AuditAction] = field(default_factory=list) + @dataclass class ResponseAuditData: - responseId: str | None = None - lastUpdated: str | None = None - condition: List[AuditCondition] = field(default_factory=list) + response_id: str | None = None + last_updated: str | None = None + condition: list[AuditCondition] = field(default_factory=list) + + +@dataclass +class AuditEvent: + request: RequestAuditData = field(default_factory=RequestAuditData) + response: ResponseAuditData = field(default_factory=ResponseAuditData) diff --git a/src/eligibility_signposting_api/audit_service.py b/src/eligibility_signposting_api/audit_service.py index 6592ecc2a..4825e61c7 100644 --- a/src/eligibility_signposting_api/audit_service.py +++ b/src/eligibility_signposting_api/audit_service.py @@ -1,76 +1,106 @@ import logging -from datetime import datetime -from itertools import groupby +from dataclasses import asdict +from datetime import UTC, datetime from operator import attrgetter -from eligibility_signposting_api.model.eligibility import CohortGroupResult, Status -from eligibility_signposting_api.audit_models import RequestAuditData, RequestAuditHeader, RequestAuditQueryParams, \ - AuditEligibilityCohorts, AuditEligibilityCohortGroups, AuditAction, AuditCondition, AuditRedirectRule, \ - AuditFilterRule, AuditSuitabilityRule +from flask import Request, g + +from eligibility_signposting_api.audit_models import ( + AuditAction, + AuditCondition, + AuditEligibilityCohortGroups, + AuditEligibilityCohorts, + AuditFilterRule, + AuditRedirectRule, + AuditSuitabilityRule, + RequestAuditData, + RequestAuditHeader, + RequestAuditQueryParams, +) +from eligibility_signposting_api.model.eligibility import ( + CohortGroupResult, + ConditionName, + IterationResult, + Status, + SuggestedActions, +) +from eligibility_signposting_api.model.rules import CampaignID, CampaignVersion, Iteration, RuleName, RulePriority +from eligibility_signposting_api.services.audit_service import AuditService logger = logging.getLogger(__name__) -from flask import Request, g, Response - - -class AuditService: +class AuditContext: @staticmethod def add_request_details(request: Request) -> None: resource_id = None - if 'id' in request.view_args: + + if "nhs_number" in request.view_args: try: - resource_id = int(request.view_args['id']) + resource_id = int(request.view_args["nhs_number"]) except (ValueError, TypeError): - logger.warning(f"Could not parse 'id' from path parameters: {request.view_args.get('id')}") + logger.exception("Could not parse 'nhs_number' from path parameters") # TODO: fix log g.audit_log.request = RequestAuditData( - nhsNumber=resource_id, - requestTimestamp=datetime.now(), - headers=(RequestAuditHeader( - xRequestID=request.headers.get("X-Request-ID"), - xCorrelationID=request.headers.get("X-Correlation-ID"), - nhsdEndUserOrganisationODS=request.headers.get("NHSD-End-User-Organisation-ODS"), - nhsdApplicationID=request.headers.get("nhsd-application-id") - )), - queryParams=(RequestAuditQueryParams( - category=request.args.get("category"), - conditions=request.args.get("conditions"), - includeActions=request.args.get("include_actions") - )), + nhs_number=resource_id, + request_timestamp=datetime.now(tz=UTC), + headers=( + RequestAuditHeader( + x_request_id=request.headers.get("X-Request-ID"), + x_correlation_id=request.headers.get("X-Correlation-ID"), + nhsd_end_user_organisation_ods=request.headers.get("NHSD-End-User-Organisation-ODS"), + nhsd_application_id=request.headers.get("nhsd-application-id"), + ) + ), + query_params=( + RequestAuditQueryParams( + category=request.args.get("category"), + conditions=request.args.get("conditions"), + include_actions=request.args.get("includeActions"), + ) + ), ) @staticmethod - def add_response_details(response: Response) -> None: - pass - - @staticmethod - def append_audit_condition(suggested_actions, best_active_iteration, best_candidate, campaign_id, campaign_version, - condition_name, best_cohort_results: dict[str, CohortGroupResult], priority, name): - + def append_audit_condition( + suggested_actions: SuggestedActions | None, + condition_name: ConditionName, + best_results: tuple[Iteration, IterationResult, dict[str, CohortGroupResult]], + campaign_details: tuple[CampaignID | None, CampaignVersion | None], + redirect_rule_details: tuple[RulePriority | None, RuleName | None], + ) -> None: audit_eligibility_cohorts, audit_eligibility_cohort_groups, audit_actions = [], [], [] audit_filter_rule, audit_suitability_rule, audit_redirect_rule = None, None, None + best_active_iteration = best_results[0] + best_candidate = best_results[1] + best_cohort_results = best_results[2] for value in sorted(best_cohort_results.values(), key=attrgetter("cohort_code")): audit_eligibility_cohorts.append( - AuditEligibilityCohorts(cohortCode=value.cohort_code, cohortStatus=value.status.name)) + AuditEligibilityCohorts(cohort_code=value.cohort_code, cohort_status=value.status.name) + ) audit_eligibility_cohort_groups.append( - AuditEligibilityCohortGroups(cohortCode=value.cohort_code, cohortStatus=value.status.name, - cohortText=value.description)) + AuditEligibilityCohortGroups( + cohort_code=value.cohort_code, cohort_status=value.status.name, cohort_text=value.description + ) + ) # TODO: what if value.audit_reasons is empty? if value.audit_reasons: if best_candidate.status.name == Status.not_eligible.name: - audit_filter_rule = AuditFilterRule(rulePriority=value.audit_reasons[0].rule_priority, - ruleName=value.audit_reasons[0].rule_name) + audit_filter_rule = AuditFilterRule( + rule_priority=value.audit_reasons[0].rule_priority, rule_name=value.audit_reasons[0].rule_name + ) if best_candidate.status.name == Status.not_actionable.name: - audit_suitability_rule = AuditSuitabilityRule(rulePriority=value.audit_reasons[0].rule_priority, - ruleName=value.audit_reasons[0].rule_name, - ruleMessage=value.audit_reasons[0].rule_description) + audit_suitability_rule = AuditSuitabilityRule( + rule_priority=value.audit_reasons[0].rule_priority, + rule_name=value.audit_reasons[0].rule_name, + rule_message=value.audit_reasons[0].rule_description, + ) if best_candidate.status.name == Status.actionable.name: - audit_redirect_rule = AuditRedirectRule(priority, name) + audit_redirect_rule = AuditRedirectRule(redirect_rule_details[0], redirect_rule_details[1]) if suggested_actions is None or suggested_actions == []: audit_actions = suggested_actions @@ -78,22 +108,33 @@ def append_audit_condition(suggested_actions, best_active_iteration, best_candid elif len(suggested_actions.actions) > 0: for action in suggested_actions.actions: audit_actions.append( - AuditAction(actionCode=action.action_code, actionDescription=action.action_description)) - - audit_condition = AuditCondition(campaignID=campaign_id, - campaignVersion=campaign_version, # TODO: Convert to int - iterationID=best_active_iteration.id, - iterationVersion=best_active_iteration.version, # TODO: Convert to int - conditionName=condition_name, - status=best_candidate.status.name, - statusText=best_candidate.status.name, # Same as status per source code - eligibilityCohorts=audit_eligibility_cohorts, - eligibilityCohortGroups=audit_eligibility_cohort_groups, - filterRules=audit_filter_rule, - suitabilityRules=audit_suitability_rule, - actionRule=audit_redirect_rule, - actions=audit_actions) - - print(">>> audit_condition:", audit_condition) - - # g.audit_log.response.condition.append(audit_condition) + AuditAction(action_code=action.action_code, action_description=action.action_description) + ) + + audit_condition = AuditCondition( + campaign_id=campaign_details[0], + campaign_version=campaign_details[1], + iteration_id=best_active_iteration.id, + iteration_version=best_active_iteration.version, + condition_name=condition_name, + status=best_candidate.status.name, + status_text=best_candidate.status.name, + eligibility_cohorts=audit_eligibility_cohorts, + eligibility_cohort_groups=audit_eligibility_cohort_groups, + filter_rules=audit_filter_rule, + suitability_rules=audit_suitability_rule, + action_rule=audit_redirect_rule, + actions=audit_actions, + ) + + g.audit_log.response.condition.append(audit_condition) # TODO: check with multiple conditions + + @staticmethod + def add_response_details(response) -> None: + g.audit_log.response.response_id = str(response.response_id) + g.audit_log.response.last_updated = str(response.meta.last_updated) + + @staticmethod + def write_to_firehose(service: AuditService, response) -> None: + AuditContext.add_response_details(response) + service.audit(asdict(g.audit_log)) diff --git a/src/eligibility_signposting_api/model/eligibility.py b/src/eligibility_signposting_api/model/eligibility.py index ac8261683..56f3c1109 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 eligibility_signposting_api.model.rules import RulePriority - NHSNumber = NewType("NHSNumber", str) DateOfBirth = NewType("DateOfBirth", date) Postcode = NewType("Postcode", str) @@ -15,6 +13,7 @@ RuleName = NewType("RuleName", str) RuleDescription = NewType("RuleDescription", str) +RulePriority = NewType("RulePriority", str) ActionType = NewType("ActionType", str) ActionCode = NewType("ActionCode", str) diff --git a/src/eligibility_signposting_api/services/audit_service.py b/src/eligibility_signposting_api/services/audit_service.py index b1d8b411e..7494661cf 100644 --- a/src/eligibility_signposting_api/services/audit_service.py +++ b/src/eligibility_signposting_api/services/audit_service.py @@ -31,8 +31,9 @@ def audit(self, audit_record: dict) -> None: Returns: str: The Firehose record ID. """ + data = json.dumps(audit_record, default=str) response = self.firehose.put_record( DeliveryStreamName=self.audit_delivery_stream, - Record={"Data": (json.dumps(audit_record) + "\n").encode("utf-8")}, + Record={"Data": (data + "\n").encode("utf-8")}, ) logger.info("Successfully sent to the Firehose", extra={"firehose_record_id": response["RecordId"]}) diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index 2982600f7..a024b45f9 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -7,11 +7,18 @@ from itertools import groupby from typing import TYPE_CHECKING, Any -from eligibility_signposting_api.audit_service import AuditService +from eligibility_signposting_api.audit_service import AuditContext if TYPE_CHECKING: - from eligibility_signposting_api.model.rules import ActionsMapper, Iteration, IterationCohort, CampaignID, \ - CampaignVersion, RulePriority, RuleName + from eligibility_signposting_api.model.rules import ( + ActionsMapper, + CampaignID, + CampaignVersion, + Iteration, + IterationCohort, + RuleName, + RulePriority, + ) from wireup import service @@ -90,7 +97,7 @@ def get_the_best_cohort_memberships( status=cc.status, reasons=cc.reasons, description=(cc.description or "").strip() if cc.description else "", - audit_reasons=cc.audit_reasons, #TODO: remove this or fix it! + audit_reasons=cc.audit_reasons, # TODO: remove this or fix it! ) for cc in best_cohorts ] @@ -105,8 +112,8 @@ def get_exclusion_rules( ir for ir in filter_rules if ir.cohort_label is None - or cohort.cohort_label == ir.cohort_label - or (isinstance(ir.cohort_label, (list, set, tuple)) and cohort.cohort_label in ir.cohort_label) + or cohort.cohort_label == ir.cohort_label + or (isinstance(ir.cohort_label, (list, set, tuple)) and cohort.cohort_label in ir.cohort_label) ) @staticmethod @@ -135,12 +142,11 @@ def evaluate_eligibility(self, *, include_actions_flag: bool = True) -> eligibil condition_results: dict[ConditionName, IterationResult] = {} actions: SuggestedActions | None = SuggestedActions([]) redirect_rule_priority, redirect_rule_name = None, None - # g.audit_log.response = ResponseAuditData() - # g.audit_log.response.condition = [] for condition_name, campaign_group in self.campaigns_grouped_by_condition_name: - # audit_condition = AuditCondition() - iteration_results: dict[str, tuple[Iteration, IterationResult, CampaignID, CampaignVersion, dict[str, CohortGroupResult]]] = {} + iteration_results: dict[ + str, tuple[Iteration, IterationResult, CampaignID, CampaignVersion, dict[str, CohortGroupResult]] + ] = {} for cc in campaign_group: active_iteration = cc.current_iteration @@ -153,17 +159,24 @@ def evaluate_eligibility(self, *, include_actions_flag: bool = True) -> eligibil IterationResult(status, best_cohorts, actions), cc.id, cc.version, - cohort_results + cohort_results, ) # Determine results between iterations - get the best if iteration_results: - best_iteration_name, (best_active_iteration, best_candidate, best_campaign_id, - best_campaign_version, best_cohort_results) = max( - iteration_results.items(), key=lambda item: item[1][1].status.value - ) + ( + best_iteration_name, + ( + best_active_iteration, + best_candidate, + best_campaign_id, + best_campaign_version, + best_cohort_results, + ), + ) = max(iteration_results.items(), key=lambda item: item[1][1].status.value) else: best_candidate = IterationResult(eligibility.Status.not_eligible, [], actions) + # TODO: Refactor? best_campaign_id = None best_campaign_version = None best_active_iteration = None @@ -174,7 +187,8 @@ def evaluate_eligibility(self, *, include_actions_flag: bool = True) -> eligibil if best_candidate.status == Status.actionable and best_active_iteration is not None: if include_actions_flag: actions, matched_r_rule_priority, matched_r_rule_name = self.handle_redirect_rules( - best_active_iteration) + best_active_iteration + ) redirect_rule_name = matched_r_rule_name redirect_rule_priority = matched_r_rule_priority else: @@ -187,19 +201,22 @@ def evaluate_eligibility(self, *, include_actions_flag: bool = True) -> eligibil condition_results[condition_name].actions = actions # add audit data - #TODO: Do we need to use deduplicated cohort results from build_condition_results instead of here? - AuditService.append_audit_condition(actions, best_active_iteration, - best_candidate, best_campaign_id, - best_campaign_version, - condition_name, best_cohort_results, - redirect_rule_priority, redirect_rule_name) + # TODO: Do we need to use deduplicated cohort results from build_condition_results instead of here? + AuditContext.append_audit_condition( + actions, + condition_name, + (best_active_iteration, best_candidate, best_cohort_results), + (best_campaign_id, best_campaign_version), + (redirect_rule_priority, redirect_rule_name), + ) # Consolidate all the results and return final_result = self.build_condition_results(condition_results) return eligibility.EligibilityStatus(conditions=final_result) - def handle_redirect_rules(self, best_active_iteration: Iteration) -> tuple[ - SuggestedActions | None, RulePriority | None, RuleName | None]: + def handle_redirect_rules( + self, best_active_iteration: Iteration + ) -> tuple[SuggestedActions | None, RulePriority | None, RuleName | 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) @@ -242,7 +259,7 @@ def get_cohort_results(self, active_iteration: rules.Iteration) -> dict[str, Coh Status.not_eligible, [], cohort.negative_description, - [] #TODO: remove this or fix it! + [], # TODO: remove this or fix it! ) return cohort_results @@ -266,7 +283,7 @@ def build_condition_results(condition_results: dict[ConditionName, IterationResu reasons=[reason for cohort in group for reason in cohort.reasons], # get the first nonempty description description=next((c.description for c in group if c.description), group[0].description), - audit_reasons=[] #TODO: remove this or fix it! + audit_reasons=[], # TODO: remove this or fix it! ) for group_cohort_code, group in grouped_cohort_results.items() if group @@ -336,11 +353,7 @@ def evaluate_suppression_rules( key = cohort.cohort_label if is_actionable: cohort_results[key] = CohortGroupResult( - cohort.cohort_group, - Status.actionable, - [], - cohort.positive_description, - suppression_reasons + cohort.cohort_group, Status.actionable, [], cohort.positive_description, suppression_reasons ) else: cohort_results[key] = CohortGroupResult( @@ -348,7 +361,7 @@ def evaluate_suppression_rules( Status.not_actionable, suppression_reasons, cohort.positive_description, - suppression_reasons + suppression_reasons, ) def evaluate_rules_priority_group( @@ -382,7 +395,8 @@ def get_actions_from_comms(action_mapper: ActionsMapper, comms: str) -> Suggeste action_type=ActionType(action.action_type), action_code=ActionCode(action.action_code), action_description=ActionDescription(action.action_description) - if action.action_description else None, + if action.action_description + else None, url_link=UrlLink(action.url_link) if action.url_link else None, url_label=UrlLabel(action.url_label) if action.url_label else None, ) diff --git a/src/eligibility_signposting_api/services/eligibility_services.py b/src/eligibility_signposting_api/services/eligibility_services.py index 032b68816..c8d9c5b50 100644 --- a/src/eligibility_signposting_api/services/eligibility_services.py +++ b/src/eligibility_signposting_api/services/eligibility_services.py @@ -4,7 +4,6 @@ from eligibility_signposting_api.model import eligibility from eligibility_signposting_api.repos import CampaignRepo, NotFoundError, PersonRepo -from eligibility_signposting_api.services.audit_service import AuditService from eligibility_signposting_api.services.calculators import eligibility_calculator as calculator logger = logging.getLogger(__name__) @@ -24,13 +23,11 @@ def __init__( self, person_repo: PersonRepo, campaign_repo: CampaignRepo, - audit_service: AuditService, calculator_factory: calculator.EligibilityCalculatorFactory, ) -> None: super().__init__() self.person_repo = person_repo self.campaign_repo = campaign_repo - self.audit_service = audit_service self.calculator_factory = calculator_factory def get_eligibility_status( @@ -54,7 +51,6 @@ def get_eligibility_status( raise UnknownPersonError from e else: calc: calculator.EligibilityCalculator = self.calculator_factory.get(person_data, campaign_configs) - self.audit_service.audit({"test_audit": "check if audit works"}) return calc.evaluate_eligibility(include_actions_flag=include_actions_flag) raise UnknownPersonError # pragma: no cover diff --git a/src/eligibility_signposting_api/views/eligibility.py b/src/eligibility_signposting_api/views/eligibility.py index aecda767c..dfafe60de 100644 --- a/src/eligibility_signposting_api/views/eligibility.py +++ b/src/eligibility_signposting_api/views/eligibility.py @@ -9,8 +9,10 @@ from flask.typing import ResponseReturnValue from wireup import Injected +from eligibility_signposting_api.audit_service import AuditContext from eligibility_signposting_api.model.eligibility import Condition, EligibilityStatus, NHSNumber, Status from eligibility_signposting_api.services import EligibilityService, UnknownPersonError +from eligibility_signposting_api.services.audit_service import AuditService from eligibility_signposting_api.services.eligibility_services import InvalidQueryParamError from eligibility_signposting_api.views.response_model import eligibility from eligibility_signposting_api.views.response_model.eligibility import ProcessedSuggestion @@ -25,14 +27,17 @@ eligibility_blueprint = Blueprint("eligibility", __name__) + @eligibility_blueprint.before_request -def store_request_id(): - rid = request.headers.get("X-Request-ID", "unknown") - # request_id_var.set(rid) +def before_request() -> None: + AuditContext.add_request_details(request) + @eligibility_blueprint.get("/", defaults={"nhs_number": ""}) @eligibility_blueprint.get("/") -def check_eligibility(nhs_number: NHSNumber, eligibility_service: Injected[EligibilityService]) -> ResponseReturnValue: +def check_eligibility( + nhs_number: NHSNumber, eligibility_service: Injected[EligibilityService], audit_service: Injected[AuditService] +) -> ResponseReturnValue: logger.info("checking nhs_number %r in %r", nhs_number, eligibility_service, extra={"nhs_number": nhs_number}) try: eligibility_status = eligibility_service.get_eligibility_status( @@ -44,6 +49,9 @@ def check_eligibility(nhs_number: NHSNumber, eligibility_service: Injected[Eligi return handle_unknown_person_error(nhs_number) else: eligibility_response = build_eligibility_response(eligibility_status) + + AuditContext.write_to_firehose(audit_service, eligibility_response) + return make_response( eligibility_response.model_dump(by_alias=True, mode="json", exclude_none=True), HTTPStatus.OK ) diff --git a/tests/conftest.py b/tests/conftest.py index 5d90004aa..4c43c14c6 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4,10 +4,11 @@ import pytest from faker import Faker from faker.providers import BaseProvider -from flask import Flask +from flask import Flask, g from flask.testing import FlaskClient from eligibility_signposting_api.app import create_app +from eligibility_signposting_api.audit_models import AuditEvent @pytest.fixture(scope="session") @@ -20,6 +21,14 @@ def client(app) -> FlaskClient: return app.test_client() +@pytest.fixture(autouse=True) +def audit_log_context(app): + with app.app_context(): + g.audit_log = AuditEvent() + yield + g.pop("audit_log", None) + + @pytest.fixture(scope="session") def faker() -> Faker: faker = Faker("en_UK") diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index d6ebc0f4f..f8178b62a 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -349,6 +349,27 @@ def persisted_77yo_person(person_table: Any, faker: Faker) -> Generator[eligibil for row in rows: person_table.delete_item(Key={"NHS_NUMBER": row["NHS_NUMBER"], "ATTRIBUTE_TYPE": row["ATTRIBUTE_TYPE"]}) +@pytest.fixture +def persisted_person_all_cohorts(person_table: Any, faker: Faker) -> Generator[eligibility.NHSNumber]: + nhs_number = eligibility.NHSNumber(faker.nhs_number()) + date_of_birth = eligibility.DateOfBirth(faker.date_of_birth(minimum_age=74, maximum_age=74)) + + for row in ( + rows := person_rows_builder( + nhs_number, + date_of_birth=date_of_birth, + postcode="hp1", + cohorts=["cohort1", "cohort2", "cohort3"], + icb="QE1" + ) + ): + person_table.put_item(Item=row) + + yield nhs_number + + for row in rows: + person_table.delete_item(Key={"NHS_NUMBER": row["NHS_NUMBER"], "ATTRIBUTE_TYPE": row["ATTRIBUTE_TYPE"]}) + @pytest.fixture def persisted_person_no_cohorts(person_table: Any, faker: Faker) -> Generator[eligibility.NHSNumber]: @@ -441,6 +462,50 @@ def campaign_config(s3_client: BaseClient, rules_bucket: BucketName) -> Generato yield campaign s3_client.delete_object(Bucket=rules_bucket, Key=f"{campaign.name}.json") +@pytest.fixture(scope="class") +def multiple_campaign_configs(s3_client: BaseClient, rules_bucket: BucketName) -> Generator[list, None, None]: + """Create and upload multiple campaign configs to S3, then clean up after tests.""" + campaigns, campaign_data_keys = [], [] + + targets = ["RSV", "COVID", "FLU", "FLU"] + target_rules_map = { + targets[0]: [rule.PersonAgeSuppressionRuleFactory.build(type=rules.RuleType.filter)], + targets[1]: [rule.PersonAgeSuppressionRuleFactory.build()], + targets[2]: [rule.ICBRedirectRuleFactory.build()], + targets[3]: [], + } # TODO: check if targets[3] is working as expected + + for i in range(4): + campaign = rule.CampaignConfigFactory.build( + name=f"campaign_{i}", + target=targets[i], + iterations=[ + rule.IterationFactory.build( + iteration_rules=target_rules_map.get(targets[i]), + iteration_cohorts=[ + rule.IterationCohortFactory.build( + cohort_label=f"cohort1", + cohort_group=f"cohort_group{i+1}", + positive_description=f"positive_desc_{i+1}", + negative_description=f"negative_desc_{i+1}", + ) + ], + ) + ], + ) + campaign_data = {"CampaignConfig": campaign.model_dump(by_alias=True)} + key = f"{campaign.name}.json" + s3_client.put_object( + Bucket=rules_bucket, Key=key, Body=json.dumps(campaign_data), ContentType="application/json" + ) + campaigns.append(campaign) + campaign_data_keys.append(key) + + yield campaigns + + for key in campaign_data_keys: + s3_client.delete_object(Bucket=rules_bucket, Key=key) + @pytest.fixture(scope="class") def campaign_config_with_magic_cohort( diff --git a/tests/integration/lambda/test_app_running_as_lambda.py b/tests/integration/lambda/test_app_running_as_lambda.py index cdd1149d3..8b2e4daa9 100644 --- a/tests/integration/lambda/test_app_running_as_lambda.py +++ b/tests/integration/lambda/test_app_running_as_lambda.py @@ -10,7 +10,16 @@ from brunns.matchers.data import json_matching as is_json_that from brunns.matchers.response import is_response from faker import Faker -from hamcrest import assert_that, contains_exactly, contains_string, has_entries, has_item, has_key +from hamcrest import ( + assert_that, + contains_exactly, + contains_string, + equal_to, + has_entries, + has_item, + has_key, + is_not, +) from yarl import URL from eligibility_signposting_api.model.eligibility import NHSNumber @@ -152,11 +161,10 @@ 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_return_response( # noqa: PLR0913 lambda_client: BaseClient, # noqa:ARG001 persisted_person: NHSNumber, - campaign_config: CampaignConfig, # noqa:ARG001 + campaign_config: CampaignConfig, s3_client: BaseClient, audit_bucket: BucketName, api_gateway_endpoint: URL, @@ -166,7 +174,14 @@ def test_given_nhs_number_in_path_matches_with_nhs_number_in_headers( # noqa: P invoke_url = f"{api_gateway_endpoint}/patient-check/{persisted_person}" response = httpx.get( invoke_url, - headers={"nhs-login-nhs-number": str(persisted_person)}, + headers={ + "nhs-login-nhs-number": str(persisted_person), + "x_request_id": "x_request_id", + "x_correlation_id": "x_correlation_id", + "nhsd_end_user_organisation_ods": "nhsd_end_user_organisation_ods", + "nhsd_application_id": "nhsd_application_id", + }, + params={"includeActions": "Y"}, timeout=10, ) @@ -180,7 +195,51 @@ def test_given_nhs_number_in_path_matches_with_nhs_number_in_headers( # noqa: P object_keys = [obj["Key"] for obj in objects] latest_key = sorted(object_keys)[-1] audit_data = json.loads(s3_client.get_object(Bucket=audit_bucket, Key=latest_key)["Body"].read()) - assert_that(audit_data, has_entries(test_audit="check if audit works")) + + expected_headers = { + "x_request_id": "x_request_id", + "x_correlation_id": "x_correlation_id", + "nhsd_end_user_organisation_ods": "nhsd_end_user_organisation_ods", + "nhsd_application_id": "nhsd_application_id", + } + expected_query_params = {"category": None, "conditions": None, "include_actions": "Y"} + + assert_that(audit_data["request"]["request_timestamp"], is_not(equal_to(""))) + assert_that(audit_data["request"]["headers"], equal_to(expected_headers)) + assert_that(audit_data["request"]["nhs_number"], equal_to(int(persisted_person))) + assert_that(audit_data["request"]["query_params"], equal_to(expected_query_params)) + + expected_conditions = [ + { + "campaign_id": campaign_config.id, + "campaign_version": campaign_config.version, + "iteration_id": campaign_config.iterations[0].id, + "iteration_version": campaign_config.iterations[0].version, + "condition_name": campaign_config.target, + "status": "not_actionable", + "status_text": "not_actionable", + "eligibility_cohorts": [{"cohort_code": "cohort_group1", "cohort_status": "not_actionable"}], + "eligibility_cohort_groups": [ + { + "cohort_code": "cohort_group1", + "cohort_text": "positive_description", + "cohort_status": "not_actionable", + } + ], + "filter_rules": None, + "suitability_rules": { + "rule_priority": 10, + "rule_name": "Exclude too young less than 75", + "rule_message": "Exclude too young less than 75", + }, + "action_rule": None, + "actions": [], + } + ] + + assert_that(audit_data["response"]["response_id"], is_not(equal_to(""))) + assert_that(audit_data["response"]["last_updated"], is_not(equal_to(""))) + assert_that(audit_data["response"]["condition"], equal_to(expected_conditions)) def test_given_nhs_number_in_path_does_not_match_with_nhs_number_in_headers_results_in_error_response( @@ -203,3 +262,44 @@ def test_given_nhs_number_in_path_does_not_match_with_nhs_number_in_headers_resu response, is_response().with_status_code(HTTPStatus.FORBIDDEN).and_body("NHS number mismatch"), ) + +def test_given_person_has_unique_status_for_different_conditions_with_audit( # noqa: PLR0913 + lambda_client: BaseClient, # noqa:ARG001 + persisted_person_all_cohorts: NHSNumber, + multiple_campaign_configs: CampaignConfig, + s3_client: BaseClient, + audit_bucket: BucketName, + api_gateway_endpoint: URL, +): + # Given + # When + invoke_url = f"{api_gateway_endpoint}/patient-check/{persisted_person_all_cohorts}" + response = httpx.get( + invoke_url, + headers={ + "nhs-login-nhs-number": str(persisted_person_all_cohorts), + "x_request_id": "x_request_id", + "x_correlation_id": "x_correlation_id", + "nhsd_end_user_organisation_ods": "nhsd_end_user_organisation_ods", + "nhsd_application_id": "nhsd_application_id", + }, + params={"includeActions": "Y"}, + timeout=10, + ) + + time.sleep(30) + # logger.info("<<<< response.json.processedSuggestions: ", response.json()["processedSuggestions"]) + # Then + assert_that( + response, + is_response().with_status_code(HTTPStatus.OK).and_body(is_json_that(has_key("processedSuggestions"))), + ) + + 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] + audit_data = json.loads(s3_client.get_object(Bucket=audit_bucket, Key=latest_key)["Body"].read()) + + logger.info("<<<< audit_data.request: ", audit_data["request"]) + logger.info("<<<< audit_data.response: ", audit_data["response"]) + diff --git a/tests/unit/services/test_eligibility_services.py b/tests/unit/services/test_eligibility_services.py index a96c8b2bd..c99f3b73a 100644 --- a/tests/unit/services/test_eligibility_services.py +++ b/tests/unit/services/test_eligibility_services.py @@ -6,7 +6,6 @@ from eligibility_signposting_api.model.eligibility import NHSNumber from eligibility_signposting_api.repos import CampaignRepo, NotFoundError, PersonRepo from eligibility_signposting_api.services import EligibilityService, UnknownPersonError -from eligibility_signposting_api.services.audit_service import AuditService from eligibility_signposting_api.services.calculators.eligibility_calculator import EligibilityCalculatorFactory from tests.fixtures.matchers.eligibility import is_eligibility_status @@ -15,9 +14,8 @@ def test_eligibility_service_returns_from_repo(): # Given person_repo = MagicMock(spec=PersonRepo) campaign_repo = MagicMock(spec=CampaignRepo) - audit_service = MagicMock(spec=AuditService) person_repo.get_eligibility = MagicMock(return_value=[]) - service = EligibilityService(person_repo, campaign_repo, audit_service, EligibilityCalculatorFactory()) + service = EligibilityService(person_repo, campaign_repo, EligibilityCalculatorFactory()) # When actual = service.get_eligibility_status(NHSNumber("1234567890")) @@ -30,9 +28,8 @@ def test_eligibility_service_for_nonexistent_nhs_number(): # Given person_repo = MagicMock(spec=PersonRepo) campaign_repo = MagicMock(spec=CampaignRepo) - audit_service = MagicMock(spec=AuditService) person_repo.get_eligibility_data = MagicMock(side_effect=NotFoundError) - service = EligibilityService(person_repo, campaign_repo, audit_service, EligibilityCalculatorFactory()) + service = EligibilityService(person_repo, campaign_repo, EligibilityCalculatorFactory()) # When with pytest.raises(UnknownPersonError): diff --git a/tests/unit/test_audit_logging.py b/tests/unit/test_audit_logging.py deleted file mode 100644 index c88f4069f..000000000 --- a/tests/unit/test_audit_logging.py +++ /dev/null @@ -1,34 +0,0 @@ -from eligibility_signposting_api.audit_service import RequestAuditData, RequestAuditHeader, \ - RequestAuditQueryParams, AuditService - -from mangum.types import LambdaEvent - -def test_audit_data_object_populates_response_summary(): - mock_event: LambdaEvent = { - "pathParameters": {"id": "1112223334"}, - "headers": { - "X-Request-ID": "request-uuid", - "X-Correlation-ID": "correlation-uuid", - "NHSD-End-User-Organisation-ODS": "nhs-ods-code-123" - }, - "queryStringParameters": { - "conditions": "RSV", - "category": "Vaccinations", - "include_actions": "Y" - }, - "requestContext": {"time": "01/Jan/2025:00:00:00 +0000"} - } - - header = RequestAuditHeader(xRequestID="request-uuid", xCorrelationID="correlation-uuid", - nhsdEndUserOrganisationODS="nhs-ods-code-123") - - query_params = RequestAuditQueryParams(category="Vaccinations", conditions="RSV", include_actions="Y") - - expected_audit_entry = RequestAuditData(nhs_number=1112223334, request_timestamp="01/Jan/2025:00:00:00 +0000", - headers=header, query_params=query_params) - - audit_service = AuditService() - actual_audit_entry = audit_service.create_request_audit_data(mock_event) - - assert actual_audit_entry == expected_audit_entry - diff --git a/tests/unit/views/test_eligibility.py b/tests/unit/views/test_eligibility.py index 126dcd56a..2e83ba21c 100644 --- a/tests/unit/views/test_eligibility.py +++ b/tests/unit/views/test_eligibility.py @@ -18,11 +18,12 @@ Reason, RuleDescription, RuleName, + RulePriority, RuleType, Status, ) -from eligibility_signposting_api.model.rules import RulePriority from eligibility_signposting_api.services import EligibilityService, UnknownPersonError +from eligibility_signposting_api.services.audit_service import AuditService from eligibility_signposting_api.services.eligibility_services import InvalidQueryParamError from eligibility_signposting_api.views.eligibility import ( build_eligibility_cohorts, @@ -39,6 +40,11 @@ logger = logging.getLogger(__name__) +class FakeAuditService: + def audit(self, audit_record): + pass + + class FakeEligibilityService(EligibilityService): def __init__(self): pass @@ -80,7 +86,10 @@ def get_eligibility_status( def test_nhs_number_given(app: Flask, client: FlaskClient): # Given - with get_app_container(app).override.service(EligibilityService, new=FakeEligibilityService()): + with ( + get_app_container(app).override.service(EligibilityService, new=FakeEligibilityService()), + get_app_container(app).override.service(AuditService, new=FakeAuditService()), + ): # When response = client.get("/patient-check/12345") @@ -213,21 +222,21 @@ def test_build_suitability_results_with_deduplication(): rule_name=RuleName("Exclude too young less than 75"), rule_description=RuleDescription("your age is greater than 75"), matcher_matched=False, - rule_priority=RulePriority(1) + rule_priority=RulePriority(1), ), Reason( rule_type=RuleType.suppression, rule_name=RuleName("Exclude too young less than 75"), rule_description=RuleDescription("your age is greater than 75"), matcher_matched=False, - rule_priority=RulePriority(1) + rule_priority=RulePriority(1), ), Reason( rule_type=RuleType.suppression, rule_name=RuleName("Exclude more than 100"), rule_description=RuleDescription("your age is greater than 100"), matcher_matched=False, - rule_priority=RulePriority(1) + rule_priority=RulePriority(1), ), ], ), @@ -240,7 +249,7 @@ def test_build_suitability_results_with_deduplication(): rule_name=RuleName("Exclude too young less than 75"), rule_description=RuleDescription("your age is greater than 75"), matcher_matched=False, - rule_priority=RulePriority(1) + rule_priority=RulePriority(1), ) ], ), @@ -253,7 +262,7 @@ def test_build_suitability_results_with_deduplication(): rule_name=RuleName("Exclude is present in sw1"), rule_description=RuleDescription("your a member of sw1"), matcher_matched=False, - rule_priority=RulePriority(1) + rule_priority=RulePriority(1), ) ], ), @@ -267,7 +276,7 @@ def test_build_suitability_results_with_deduplication(): rule_name=RuleName("Already vaccinated"), rule_description=RuleDescription("you have already vaccinated"), matcher_matched=False, - rule_priority=RulePriority(1) + rule_priority=RulePriority(1), ) ], ), @@ -301,21 +310,21 @@ def test_build_suitability_results_when_rule_text_is_empty_or_null(): rule_name=RuleName("Exclude too young less than 75"), rule_description=RuleDescription("your age is greater than 75"), matcher_matched=False, - rule_priority=RulePriority(1) + rule_priority=RulePriority(1), ), Reason( rule_type=RuleType.suppression, rule_name=RuleName("Exclude more than 100"), rule_description=RuleDescription(""), matcher_matched=False, - rule_priority=RulePriority(1) + rule_priority=RulePriority(1), ), Reason( rule_type=RuleType.suppression, rule_name=RuleName("Exclude more than 100"), matcher_matched=False, rule_description=None, - rule_priority=RulePriority(1) + rule_priority=RulePriority(1), ), ], ), @@ -328,7 +337,7 @@ def test_build_suitability_results_when_rule_text_is_empty_or_null(): rule_name=RuleName("Exclude is present in sw1"), rule_description=RuleDescription(""), matcher_matched=False, - rule_priority=RulePriority(1) + rule_priority=RulePriority(1), ) ], ), @@ -341,7 +350,7 @@ def test_build_suitability_results_when_rule_text_is_empty_or_null(): rule_name=RuleName("Exclude is present in sw1"), rule_description=None, matcher_matched=False, - rule_priority=RulePriority(1) + rule_priority=RulePriority(1), ) ], ), @@ -370,7 +379,10 @@ def test_no_suitability_rules_for_actionable(): 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()): + with ( + get_app_container(app).override.service(EligibilityService, new=FakeEligibilityService()), + get_app_container(app).override.service(AuditService, new=FakeAuditService()), + ): # When response = client.get("/patient-check/12345?includeActions=Y") @@ -380,7 +392,10 @@ def test_nhs_number_and_include_actions_param_given_and_is_yes(app: Flask, clien def test_nhs_number_and_include_actions_param_no_given(app: Flask, client: FlaskClient): # Given - with get_app_container(app).override.service(EligibilityService, new=FakeEligibilityService()): + with ( + get_app_container(app).override.service(EligibilityService, new=FakeEligibilityService()), + get_app_container(app).override.service(AuditService, new=FakeAuditService()), + ): # When response = client.get("/patient-check/12345?includeActions=N") From 4a29b3ef7bba01fd4656f59151d73e7923679f80 Mon Sep 17 00:00:00 2001 From: Shweta <216860557+shweta-nhs@users.noreply.github.com> Date: Thu, 3 Jul 2025 15:09:28 +0100 Subject: [PATCH 06/21] Fixed actions part of the audit --- src/eligibility_signposting_api/app.py | 13 -- .../{audit_service.py => audit_context.py} | 29 ++--- .../audit_models.py | 2 +- .../model/eligibility.py | 2 +- .../calculators/eligibility_calculator.py | 9 +- .../views/eligibility.py | 2 +- tests/integration/conftest.py | 9 +- .../lambda/test_app_running_as_lambda.py | 113 +++++++++++++++++- 8 files changed, 134 insertions(+), 45 deletions(-) rename src/eligibility_signposting_api/{audit_service.py => audit_context.py} (84%) diff --git a/src/eligibility_signposting_api/app.py b/src/eligibility_signposting_api/app.py index 1e2a5d6ab..348a06557 100644 --- a/src/eligibility_signposting_api/app.py +++ b/src/eligibility_signposting_api/app.py @@ -16,8 +16,6 @@ init_logging() logger = logging.getLogger(__name__) -# app = Flask(__name__) - def main() -> None: # pragma: no cover """Run the Flask app as a local process.""" @@ -25,17 +23,6 @@ def main() -> None: # pragma: no cover app.run(debug=config()["log_level"] == logging.DEBUG) -# @app.before_request -# def request_audit(): -# AuditService.add_request_details(request) -# -# -# @app.after_request -# def response_audit(response): -# AuditService.audit(asdict(g.audit_log)) -# return response - - @validate_matching_nhs_number() def lambda_handler(event: LambdaEvent, context: LambdaContext) -> dict[str, Any]: # pragma: no cover """Run the Flask app as an AWS Lambda.""" diff --git a/src/eligibility_signposting_api/audit_service.py b/src/eligibility_signposting_api/audit_context.py similarity index 84% rename from src/eligibility_signposting_api/audit_service.py rename to src/eligibility_signposting_api/audit_context.py index 4825e61c7..8cf7e9636 100644 --- a/src/eligibility_signposting_api/audit_service.py +++ b/src/eligibility_signposting_api/audit_context.py @@ -15,7 +15,7 @@ AuditSuitabilityRule, RequestAuditData, RequestAuditHeader, - RequestAuditQueryParams, + RequestAuditQueryParams, AuditEvent, ) from eligibility_signposting_api.model.eligibility import ( CohortGroupResult, @@ -33,13 +33,10 @@ class AuditContext: @staticmethod def add_request_details(request: Request) -> None: + g.audit_log = AuditEvent() resource_id = None - if "nhs_number" in request.view_args: - try: - resource_id = int(request.view_args["nhs_number"]) - except (ValueError, TypeError): - logger.exception("Could not parse 'nhs_number' from path parameters") # TODO: fix log + resource_id = request.view_args["nhs_number"] g.audit_log.request = RequestAuditData( nhs_number=resource_id, request_timestamp=datetime.now(tz=UTC), @@ -86,17 +83,17 @@ def append_audit_condition( ) ) - # TODO: what if value.audit_reasons is empty? - if value.audit_reasons: + if value.audit_rules: if best_candidate.status.name == Status.not_eligible.name: audit_filter_rule = AuditFilterRule( - rule_priority=value.audit_reasons[0].rule_priority, rule_name=value.audit_reasons[0].rule_name + rule_priority=int(value.audit_rules[0].rule_priority), + rule_name=value.audit_rules[0].rule_name ) if best_candidate.status.name == Status.not_actionable.name: audit_suitability_rule = AuditSuitabilityRule( - rule_priority=value.audit_reasons[0].rule_priority, - rule_name=value.audit_reasons[0].rule_name, - rule_message=value.audit_reasons[0].rule_description, + rule_priority=int(value.audit_rules[0].rule_priority), + rule_name=value.audit_rules[0].rule_name, + rule_message=value.audit_rules[0].rule_description, ) if best_candidate.status.name == Status.actionable.name: @@ -108,7 +105,11 @@ def append_audit_condition( elif len(suggested_actions.actions) > 0: for action in suggested_actions.actions: audit_actions.append( - AuditAction(action_code=action.action_code, action_description=action.action_description) + AuditAction(action_code=action.action_code, + action_type=action.action_type, + action_description=action.action_description, + action_url=action.url_link, + action_url_label=action.url_label) ) audit_condition = AuditCondition( @@ -127,7 +128,7 @@ def append_audit_condition( actions=audit_actions, ) - g.audit_log.response.condition.append(audit_condition) # TODO: check with multiple conditions + g.audit_log.response.condition.append(audit_condition) @staticmethod def add_response_details(response) -> None: diff --git a/src/eligibility_signposting_api/audit_models.py b/src/eligibility_signposting_api/audit_models.py index dd84e4b27..36ee1648e 100644 --- a/src/eligibility_signposting_api/audit_models.py +++ b/src/eligibility_signposting_api/audit_models.py @@ -22,7 +22,7 @@ class RequestAuditData: request_timestamp: datetime = field(default_factory=datetime.utcnow) headers: RequestAuditHeader = field(default_factory=RequestAuditHeader) query_params: RequestAuditQueryParams = field(default_factory=RequestAuditQueryParams) - nhs_number: int | None = None + nhs_number: str | None = None @dataclass diff --git a/src/eligibility_signposting_api/model/eligibility.py b/src/eligibility_signposting_api/model/eligibility.py index 56f3c1109..0295e7727 100644 --- a/src/eligibility_signposting_api/model/eligibility.py +++ b/src/eligibility_signposting_api/model/eligibility.py @@ -100,7 +100,7 @@ class CohortGroupResult: status: Status reasons: list[Reason] description: str | None - audit_reasons: list[Reason] + audit_rules: list[Reason] @dataclass diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index a024b45f9..e4989a4e0 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -7,7 +7,7 @@ from itertools import groupby from typing import TYPE_CHECKING, Any -from eligibility_signposting_api.audit_service import AuditContext +from eligibility_signposting_api.audit_context import AuditContext if TYPE_CHECKING: from eligibility_signposting_api.model.rules import ( @@ -97,7 +97,7 @@ def get_the_best_cohort_memberships( status=cc.status, reasons=cc.reasons, description=(cc.description or "").strip() if cc.description else "", - audit_reasons=cc.audit_reasons, # TODO: remove this or fix it! + audit_rules=cc.audit_rules, # TODO: remove this or fix it! ) for cc in best_cohorts ] @@ -199,11 +199,12 @@ def evaluate_eligibility(self, *, include_actions_flag: bool = True) -> eligibil # add actions to condition results condition_results[condition_name].actions = actions + actions: SuggestedActions | None = SuggestedActions([]) # add audit data # TODO: Do we need to use deduplicated cohort results from build_condition_results instead of here? AuditContext.append_audit_condition( - actions, + condition_results[condition_name].actions, condition_name, (best_active_iteration, best_candidate, best_cohort_results), (best_campaign_id, best_campaign_version), @@ -283,7 +284,7 @@ def build_condition_results(condition_results: dict[ConditionName, IterationResu reasons=[reason for cohort in group for reason in cohort.reasons], # get the first nonempty description description=next((c.description for c in group if c.description), group[0].description), - audit_reasons=[], # TODO: remove this or fix it! + audit_rules=[], # TODO: remove this or fix it! ) for group_cohort_code, group in grouped_cohort_results.items() if group diff --git a/src/eligibility_signposting_api/views/eligibility.py b/src/eligibility_signposting_api/views/eligibility.py index dfafe60de..fafc73edf 100644 --- a/src/eligibility_signposting_api/views/eligibility.py +++ b/src/eligibility_signposting_api/views/eligibility.py @@ -9,7 +9,7 @@ from flask.typing import ResponseReturnValue from wireup import Injected -from eligibility_signposting_api.audit_service import AuditContext +from eligibility_signposting_api.audit_context import AuditContext from eligibility_signposting_api.model.eligibility import Condition, EligibilityStatus, NHSNumber, Status from eligibility_signposting_api.services import EligibilityService, UnknownPersonError from eligibility_signposting_api.services.audit_service import AuditService diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index f8178b62a..470a55e4a 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -463,19 +463,18 @@ def campaign_config(s3_client: BaseClient, rules_bucket: BucketName) -> Generato s3_client.delete_object(Bucket=rules_bucket, Key=f"{campaign.name}.json") @pytest.fixture(scope="class") -def multiple_campaign_configs(s3_client: BaseClient, rules_bucket: BucketName) -> Generator[list, None, None]: +def multiple_campaign_configs(s3_client: BaseClient, rules_bucket: BucketName) -> Generator[list[rules.CampaignConfig]]: """Create and upload multiple campaign configs to S3, then clean up after tests.""" campaigns, campaign_data_keys = [], [] - targets = ["RSV", "COVID", "FLU", "FLU"] + targets = ["RSV", "COVID", "FLU"] target_rules_map = { targets[0]: [rule.PersonAgeSuppressionRuleFactory.build(type=rules.RuleType.filter)], targets[1]: [rule.PersonAgeSuppressionRuleFactory.build()], targets[2]: [rule.ICBRedirectRuleFactory.build()], - targets[3]: [], - } # TODO: check if targets[3] is working as expected + } - for i in range(4): + for i in range(3): campaign = rule.CampaignConfigFactory.build( name=f"campaign_{i}", target=targets[i], diff --git a/tests/integration/lambda/test_app_running_as_lambda.py b/tests/integration/lambda/test_app_running_as_lambda.py index 8b2e4daa9..2a43725a8 100644 --- a/tests/integration/lambda/test_app_running_as_lambda.py +++ b/tests/integration/lambda/test_app_running_as_lambda.py @@ -1,6 +1,7 @@ import base64 import json import logging +import time from http import HTTPStatus import httpx @@ -19,6 +20,7 @@ has_item, has_key, is_not, + contains_inanyorder ) from yarl import URL @@ -206,7 +208,7 @@ def test_given_nhs_number_in_path_matches_with_nhs_number_in_headers_and_return_ assert_that(audit_data["request"]["request_timestamp"], is_not(equal_to(""))) assert_that(audit_data["request"]["headers"], equal_to(expected_headers)) - assert_that(audit_data["request"]["nhs_number"], equal_to(int(persisted_person))) + assert_that(audit_data["request"]["nhs_number"], equal_to(persisted_person)) assert_that(audit_data["request"]["query_params"], equal_to(expected_query_params)) expected_conditions = [ @@ -266,7 +268,7 @@ def test_given_nhs_number_in_path_does_not_match_with_nhs_number_in_headers_resu def test_given_person_has_unique_status_for_different_conditions_with_audit( # noqa: PLR0913 lambda_client: BaseClient, # noqa:ARG001 persisted_person_all_cohorts: NHSNumber, - multiple_campaign_configs: CampaignConfig, + multiple_campaign_configs: list[CampaignConfig], s3_client: BaseClient, audit_bucket: BucketName, api_gateway_endpoint: URL, @@ -287,8 +289,6 @@ def test_given_person_has_unique_status_for_different_conditions_with_audit( # timeout=10, ) - time.sleep(30) - # logger.info("<<<< response.json.processedSuggestions: ", response.json()["processedSuggestions"]) # Then assert_that( response, @@ -300,6 +300,107 @@ def test_given_person_has_unique_status_for_different_conditions_with_audit( # latest_key = sorted(object_keys)[-1] audit_data = json.loads(s3_client.get_object(Bucket=audit_bucket, Key=latest_key)["Body"].read()) - logger.info("<<<< audit_data.request: ", audit_data["request"]) - logger.info("<<<< audit_data.response: ", audit_data["response"]) + expected_headers = { + "x_request_id": "x_request_id", + "x_correlation_id": "x_correlation_id", + "nhsd_end_user_organisation_ods": "nhsd_end_user_organisation_ods", + "nhsd_application_id": "nhsd_application_id", + } + expected_query_params = {"category": None, "conditions": None, "include_actions": "Y"} + + rsv_campaign = multiple_campaign_configs[0] + covid_campaign = multiple_campaign_configs[1] + flu_campaign = multiple_campaign_configs[2] + + expected_conditions = [ + { + "campaign_id": rsv_campaign.id, + "campaign_version": rsv_campaign.version, + "iteration_id": rsv_campaign.iterations[0].id, + "iteration_version": rsv_campaign.iterations[0].version, + "condition_name": rsv_campaign.target, + "status": "not_eligible", + "status_text": "not_eligible", + "eligibility_cohorts": [{"cohort_code": "cohort_group1", "cohort_status": "not_eligible"}], + "eligibility_cohort_groups": [ + { + "cohort_code": "cohort_group1", + "cohort_text": "negative_desc_1", + "cohort_status": "not_eligible", + } + ], + "filter_rules": { + "rule_priority": 10, + "rule_name": "Exclude too young less than 75" + }, + "suitability_rules": None, + "action_rule": None, + "actions": [], + }, + { + "campaign_id": covid_campaign.id, + "campaign_version": covid_campaign.version, + "iteration_id": covid_campaign.iterations[0].id, + "iteration_version": covid_campaign.iterations[0].version, + "condition_name": covid_campaign.target, + "status": "not_actionable", + "status_text": "not_actionable", + "eligibility_cohorts": [{"cohort_code": "cohort_group2", "cohort_status": "not_actionable"}], + "eligibility_cohort_groups": [ + { + "cohort_code": "cohort_group2", + "cohort_text": "positive_desc_2", + "cohort_status": "not_actionable", + } + ], + "filter_rules": None, + "suitability_rules": { + "rule_priority": 10, + "rule_name": "Exclude too young less than 75", + "rule_message": "Exclude too young less than 75" + }, + "action_rule": None, + "actions": [], + }, + { + "campaign_id": flu_campaign.id, + "campaign_version": flu_campaign.version, + "iteration_id": flu_campaign.iterations[0].id, + "iteration_version": flu_campaign.iterations[0].version, + "condition_name": flu_campaign.target, + "status": "actionable", + "status_text": "actionable", + "eligibility_cohorts": [{"cohort_code": "cohort_group3", "cohort_status": "actionable"}], + "eligibility_cohort_groups": [ + { + "cohort_code": "cohort_group3", + "cohort_text": "positive_desc_3", + "cohort_status": "actionable", + } + ], + "filter_rules": None, + "suitability_rules": None, + "action_rule": { + "rule_priority": 20, + "rule_name": "In QE1" + }, + "actions": [{ + "internal_name": None, # TODO: FIX! + "action_type": "defaultcomms", + "action_code": "action_code", + "action_description": None, + "action_url": None, + "action_url_label": None + }], + } + ] + + assert_that(audit_data["request"]["request_timestamp"], is_not(equal_to(""))) + assert_that(audit_data["request"]["headers"], equal_to(expected_headers)) + assert_that(audit_data["request"]["nhs_number"], equal_to(persisted_person_all_cohorts)) + assert_that(audit_data["request"]["query_params"], equal_to(expected_query_params)) + assert_that(audit_data["response"]["response_id"], is_not(equal_to(""))) + assert_that(audit_data["response"]["last_updated"], is_not(equal_to(""))) + assert_that(audit_data["response"]["condition"], contains_inanyorder(*expected_conditions)) + From d4a17e9a31a09d08b39f54e02481694225b90765 Mon Sep 17 00:00:00 2001 From: Shweta <216860557+shweta-nhs@users.noreply.github.com> Date: Thu, 3 Jul 2025 17:12:38 +0100 Subject: [PATCH 07/21] Added unit tests for AuditContext --- .../audit_context.py | 4 +- .../audit_models.py | 5 +- .../lambda/test_app_running_as_lambda.py | 1 - tests/unit/test_audit_context.py | 128 ++++++++++++++++++ 4 files changed, 133 insertions(+), 5 deletions(-) create mode 100644 tests/unit/test_audit_context.py diff --git a/src/eligibility_signposting_api/audit_context.py b/src/eligibility_signposting_api/audit_context.py index 8cf7e9636..544a93ad9 100644 --- a/src/eligibility_signposting_api/audit_context.py +++ b/src/eligibility_signposting_api/audit_context.py @@ -132,8 +132,8 @@ def append_audit_condition( @staticmethod def add_response_details(response) -> None: - g.audit_log.response.response_id = str(response.response_id) - g.audit_log.response.last_updated = str(response.meta.last_updated) + g.audit_log.response.response_id = response.response_id + g.audit_log.response.last_updated = response.meta.last_updated @staticmethod def write_to_firehose(service: AuditService, response) -> None: diff --git a/src/eligibility_signposting_api/audit_models.py b/src/eligibility_signposting_api/audit_models.py index 36ee1648e..31c30bff4 100644 --- a/src/eligibility_signposting_api/audit_models.py +++ b/src/eligibility_signposting_api/audit_models.py @@ -1,5 +1,6 @@ from dataclasses import dataclass, field from datetime import datetime +from uuid import UUID @dataclass @@ -19,7 +20,7 @@ class RequestAuditQueryParams: @dataclass class RequestAuditData: - request_timestamp: datetime = field(default_factory=datetime.utcnow) + request_timestamp: datetime = field(default_factory=datetime.utcnow) # TODO: fix use of deprecated datetime.now() headers: RequestAuditHeader = field(default_factory=RequestAuditHeader) query_params: RequestAuditQueryParams = field(default_factory=RequestAuditQueryParams) nhs_number: str | None = None @@ -86,7 +87,7 @@ class AuditCondition: @dataclass class ResponseAuditData: - response_id: str | None = None + response_id: UUID | None = None last_updated: str | None = None condition: list[AuditCondition] = field(default_factory=list) diff --git a/tests/integration/lambda/test_app_running_as_lambda.py b/tests/integration/lambda/test_app_running_as_lambda.py index 2a43725a8..ee55c6c68 100644 --- a/tests/integration/lambda/test_app_running_as_lambda.py +++ b/tests/integration/lambda/test_app_running_as_lambda.py @@ -1,7 +1,6 @@ import base64 import json import logging -import time from http import HTTPStatus import httpx diff --git a/tests/unit/test_audit_context.py b/tests/unit/test_audit_context.py new file mode 100644 index 000000000..ad8d1b717 --- /dev/null +++ b/tests/unit/test_audit_context.py @@ -0,0 +1,128 @@ +import uuid +import pytest +from datetime import datetime + +from flask import g, request, Flask + +from eligibility_signposting_api.audit_context import AuditContext +from eligibility_signposting_api.audit_models import AuditEvent +from eligibility_signposting_api.model.eligibility import SuggestedActions, ConditionName, IterationResult, \ + CohortGroupResult, SuggestedAction, ActionCode, ActionType, ActionDescription, UrlLink, UrlLabel, Reason, RuleName, \ + RuleDescription, RulePriority, Status +from eligibility_signposting_api.model.rules import Iteration, CampaignID, CampaignVersion, \ + RuleType +from fixtures.builders.model.rule import IterationFactory +from fixtures.builders.views.response_model.eligibility import EligibilityResponseFactory + + +@pytest.fixture +def app(): + app = Flask(__name__) + return app + + +def test_add_request_details_sets_audit_log_on_g(app): + headers = { + "X-Request-ID": "test-x-request-id", + "X-Correlation-ID": "test-x-correlation-id", + "NHSD-End-User-Organisation-ODS": "test-org", + "nhsd-application-id": "test-app-id", + } + + nhs_number = "1234567890" + url = "/patient-check?includeActions=Y" + + with app.test_request_context(url, headers=headers, method="GET"): + request.view_args = {'nhs_number': nhs_number} + AuditContext.add_request_details(request) + + assert hasattr(g, "audit_log") + audit_req = g.audit_log.request + assert audit_req.nhs_number == nhs_number + assert audit_req.headers.x_request_id == "test-x-request-id" + assert audit_req.headers.x_correlation_id == "test-x-correlation-id" + assert audit_req.headers.nhsd_end_user_organisation_ods == "test-org" + assert audit_req.headers.nhsd_application_id == "test-app-id" + assert audit_req.query_params.include_actions == "Y" + assert isinstance(audit_req.request_timestamp, datetime) + + +def test_add_request_details_when_headers_are_empty_sets_audit_log_on_g(app): + nhs_number = "1234567890" + url = "/patient-check?includeActions=Y" + + with app.test_request_context(url, method="GET"): + request.view_args = {'nhs_number': nhs_number} + AuditContext.add_request_details(request) + + assert hasattr(g, "audit_log") + audit_req = g.audit_log.request + assert audit_req.nhs_number == nhs_number + assert audit_req.headers.x_request_id is None + assert audit_req.headers.x_correlation_id is None + assert audit_req.headers.nhsd_end_user_organisation_ods is None + assert audit_req.headers.nhsd_application_id is None + assert audit_req.query_params.include_actions == "Y" + assert isinstance(audit_req.request_timestamp, datetime) + + +def test_append_audit_condition_adds_condition_to_audit_log(app): + suggested_actions: SuggestedActions | None + condition_name: ConditionName + best_results: tuple[Iteration, IterationResult, dict[str, CohortGroupResult]] + campaign_details: tuple[CampaignID | None, CampaignVersion | None] + redirect_rule_details: tuple[RulePriority | None, RuleName | None] + + suggested_actions = SuggestedActions(actions=[SuggestedAction( + action_code=ActionCode("ActionCode1"), + action_type=ActionType("ActionType1"), + action_description=ActionDescription("ActionDescription1"), + url_link=UrlLink("https://www.example.com"), + url_label=UrlLabel("ActionLabel1") + )]) + + condition_name = ConditionName("Condition1") + iteration = IterationFactory.build() + audit_rules = [Reason( + rule_type=RuleType.filter, + rule_name=RuleName("FilterRuleName1"), + rule_description=RuleDescription("FilterRuleDescription1"), + matcher_matched=True, + rule_priority=RulePriority("1") + )] + cohort_group_result = CohortGroupResult(status=Status.actionable, cohort_code="CohortCode1", + description="CohortDescription1", audit_rules=audit_rules, + reasons=audit_rules) + iteration_result = IterationResult(status=Status.actionable, cohort_results=[cohort_group_result], + actions=suggested_actions) + best_results = (iteration, iteration_result, {"CohortCode1": cohort_group_result}) + campaign_details = (CampaignID("CampaignID1"), CampaignVersion("CampaignVersion1")) + redirect_rule_details = (RulePriority("1"), RuleName("RedirectRuleName1")) + + with app.app_context(): + g.audit_log = AuditEvent() + + AuditContext.append_audit_condition(suggested_actions, condition_name, best_results, campaign_details, + redirect_rule_details) + + assert g.audit_log.response.condition, condition_name + cond = g.audit_log.response.condition[0] + assert cond.condition_name == condition_name + assert cond.campaign_id == campaign_details[0] + assert cond.status == best_results[1].status.name + assert cond.status_text == best_results[1].status.name + +def test_add_response_details_adds_to_audit_log(app): + eligibility_response = EligibilityResponseFactory.build(response_id=uuid.uuid4(), + meta={"last_updated": datetime(2023, 1, 1, 0, 0)}, + processed_suggestions=[]) + + with app.app_context(): + g.audit_log = AuditEvent() + + AuditContext.add_response_details(eligibility_response) + + assert g.audit_log.response.response_id == eligibility_response.response_id + assert g.audit_log.response.last_updated is eligibility_response.meta.last_updated + + From 05053e12c564019bf6f9ac74f7ba134e58da9af5 Mon Sep 17 00:00:00 2001 From: Shweta <216860557+shweta-nhs@users.noreply.github.com> Date: Fri, 4 Jul 2025 11:08:49 +0100 Subject: [PATCH 08/21] Added unit tests and fixed TODOs --- src/eligibility_signposting_api/app.py | 2 +- .../audit_context.py | 19 +-- .../audit_models.py | 4 +- tests/integration/conftest.py | 12 +- .../lambda/test_app_running_as_lambda.py | 38 +++--- tests/unit/test_audit_context.py | 122 ++++++++++++------ 6 files changed, 121 insertions(+), 76 deletions(-) diff --git a/src/eligibility_signposting_api/app.py b/src/eligibility_signposting_api/app.py index 348a06557..2cf614bea 100644 --- a/src/eligibility_signposting_api/app.py +++ b/src/eligibility_signposting_api/app.py @@ -3,7 +3,7 @@ import wireup.integration.flask from asgiref.wsgi import WsgiToAsgi -from flask import Flask, request +from flask import Flask from mangum import Mangum from mangum.types import LambdaContext, LambdaEvent diff --git a/src/eligibility_signposting_api/audit_context.py b/src/eligibility_signposting_api/audit_context.py index 544a93ad9..756cb197e 100644 --- a/src/eligibility_signposting_api/audit_context.py +++ b/src/eligibility_signposting_api/audit_context.py @@ -10,12 +10,13 @@ AuditCondition, AuditEligibilityCohortGroups, AuditEligibilityCohorts, + AuditEvent, AuditFilterRule, AuditRedirectRule, AuditSuitabilityRule, RequestAuditData, RequestAuditHeader, - RequestAuditQueryParams, AuditEvent, + RequestAuditQueryParams, ) from eligibility_signposting_api.model.eligibility import ( CohortGroupResult, @@ -57,7 +58,6 @@ def add_request_details(request: Request) -> None: ), ) - @staticmethod def append_audit_condition( suggested_actions: SuggestedActions | None, @@ -86,8 +86,7 @@ def append_audit_condition( if value.audit_rules: if best_candidate.status.name == Status.not_eligible.name: audit_filter_rule = AuditFilterRule( - rule_priority=int(value.audit_rules[0].rule_priority), - rule_name=value.audit_rules[0].rule_name + rule_priority=int(value.audit_rules[0].rule_priority), rule_name=value.audit_rules[0].rule_name ) if best_candidate.status.name == Status.not_actionable.name: audit_suitability_rule = AuditSuitabilityRule( @@ -105,11 +104,13 @@ def append_audit_condition( elif len(suggested_actions.actions) > 0: for action in suggested_actions.actions: audit_actions.append( - AuditAction(action_code=action.action_code, - action_type=action.action_type, - action_description=action.action_description, - action_url=action.url_link, - action_url_label=action.url_label) + AuditAction( + action_code=action.action_code, + action_type=action.action_type, + action_description=action.action_description, + action_url=action.url_link, + action_url_label=action.url_label, + ) ) audit_condition = AuditCondition( diff --git a/src/eligibility_signposting_api/audit_models.py b/src/eligibility_signposting_api/audit_models.py index 31c30bff4..d2e1ee095 100644 --- a/src/eligibility_signposting_api/audit_models.py +++ b/src/eligibility_signposting_api/audit_models.py @@ -1,5 +1,5 @@ from dataclasses import dataclass, field -from datetime import datetime +from datetime import datetime, UTC from uuid import UUID @@ -20,7 +20,7 @@ class RequestAuditQueryParams: @dataclass class RequestAuditData: - request_timestamp: datetime = field(default_factory=datetime.utcnow) # TODO: fix use of deprecated datetime.now() + request_timestamp: datetime = field(default_factory=lambda: datetime.now(UTC)) headers: RequestAuditHeader = field(default_factory=RequestAuditHeader) query_params: RequestAuditQueryParams = field(default_factory=RequestAuditQueryParams) nhs_number: str | None = None diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 470a55e4a..2054579b7 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -349,6 +349,7 @@ def persisted_77yo_person(person_table: Any, faker: Faker) -> Generator[eligibil for row in rows: person_table.delete_item(Key={"NHS_NUMBER": row["NHS_NUMBER"], "ATTRIBUTE_TYPE": row["ATTRIBUTE_TYPE"]}) + @pytest.fixture def persisted_person_all_cohorts(person_table: Any, faker: Faker) -> Generator[eligibility.NHSNumber]: nhs_number = eligibility.NHSNumber(faker.nhs_number()) @@ -360,7 +361,7 @@ def persisted_person_all_cohorts(person_table: Any, faker: Faker) -> Generator[e date_of_birth=date_of_birth, postcode="hp1", cohorts=["cohort1", "cohort2", "cohort3"], - icb="QE1" + icb="QE1", ) ): person_table.put_item(Item=row) @@ -462,6 +463,7 @@ def campaign_config(s3_client: BaseClient, rules_bucket: BucketName) -> Generato yield campaign s3_client.delete_object(Bucket=rules_bucket, Key=f"{campaign.name}.json") + @pytest.fixture(scope="class") def multiple_campaign_configs(s3_client: BaseClient, rules_bucket: BucketName) -> Generator[list[rules.CampaignConfig]]: """Create and upload multiple campaign configs to S3, then clean up after tests.""" @@ -483,10 +485,10 @@ def multiple_campaign_configs(s3_client: BaseClient, rules_bucket: BucketName) - iteration_rules=target_rules_map.get(targets[i]), iteration_cohorts=[ rule.IterationCohortFactory.build( - cohort_label=f"cohort1", - cohort_group=f"cohort_group{i+1}", - positive_description=f"positive_desc_{i+1}", - negative_description=f"negative_desc_{i+1}", + cohort_label="cohort1", + cohort_group=f"cohort_group{i + 1}", + positive_description=f"positive_desc_{i + 1}", + negative_description=f"negative_desc_{i + 1}", ) ], ) diff --git a/tests/integration/lambda/test_app_running_as_lambda.py b/tests/integration/lambda/test_app_running_as_lambda.py index ee55c6c68..793234fa7 100644 --- a/tests/integration/lambda/test_app_running_as_lambda.py +++ b/tests/integration/lambda/test_app_running_as_lambda.py @@ -13,13 +13,13 @@ from hamcrest import ( assert_that, contains_exactly, + contains_inanyorder, contains_string, equal_to, has_entries, has_item, has_key, is_not, - contains_inanyorder ) from yarl import URL @@ -162,6 +162,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_and_return_response( # noqa: PLR0913 lambda_client: BaseClient, # noqa:ARG001 persisted_person: NHSNumber, @@ -264,6 +265,7 @@ def test_given_nhs_number_in_path_does_not_match_with_nhs_number_in_headers_resu is_response().with_status_code(HTTPStatus.FORBIDDEN).and_body("NHS number mismatch"), ) + def test_given_person_has_unique_status_for_different_conditions_with_audit( # noqa: PLR0913 lambda_client: BaseClient, # noqa:ARG001 persisted_person_all_cohorts: NHSNumber, @@ -328,10 +330,7 @@ def test_given_person_has_unique_status_for_different_conditions_with_audit( # "cohort_status": "not_eligible", } ], - "filter_rules": { - "rule_priority": 10, - "rule_name": "Exclude too young less than 75" - }, + "filter_rules": {"rule_priority": 10, "rule_name": "Exclude too young less than 75"}, "suitability_rules": None, "action_rule": None, "actions": [], @@ -356,7 +355,7 @@ def test_given_person_has_unique_status_for_different_conditions_with_audit( # "suitability_rules": { "rule_priority": 10, "rule_name": "Exclude too young less than 75", - "rule_message": "Exclude too young less than 75" + "rule_message": "Exclude too young less than 75", }, "action_rule": None, "actions": [], @@ -379,19 +378,18 @@ def test_given_person_has_unique_status_for_different_conditions_with_audit( # ], "filter_rules": None, "suitability_rules": None, - "action_rule": { - "rule_priority": 20, - "rule_name": "In QE1" - }, - "actions": [{ - "internal_name": None, # TODO: FIX! - "action_type": "defaultcomms", - "action_code": "action_code", - "action_description": None, - "action_url": None, - "action_url_label": None - }], - } + "action_rule": {"rule_priority": 20, "rule_name": "In QE1"}, + "actions": [ + { + "internal_name": None, # TODO: FIX! + "action_type": "defaultcomms", + "action_code": "action_code", + "action_description": None, + "action_url": None, + "action_url_label": None, + } + ], + }, ] assert_that(audit_data["request"]["request_timestamp"], is_not(equal_to(""))) @@ -401,5 +399,3 @@ def test_given_person_has_unique_status_for_different_conditions_with_audit( # assert_that(audit_data["response"]["response_id"], is_not(equal_to(""))) assert_that(audit_data["response"]["last_updated"], is_not(equal_to(""))) assert_that(audit_data["response"]["condition"], contains_inanyorder(*expected_conditions)) - - diff --git a/tests/unit/test_audit_context.py b/tests/unit/test_audit_context.py index ad8d1b717..6bc7d2ded 100644 --- a/tests/unit/test_audit_context.py +++ b/tests/unit/test_audit_context.py @@ -1,18 +1,34 @@ import uuid -import pytest -from datetime import datetime +from dataclasses import asdict +from datetime import UTC, datetime +from unittest.mock import Mock -from flask import g, request, Flask +import pytest +from flask import Flask, g, request from eligibility_signposting_api.audit_context import AuditContext from eligibility_signposting_api.audit_models import AuditEvent -from eligibility_signposting_api.model.eligibility import SuggestedActions, ConditionName, IterationResult, \ - CohortGroupResult, SuggestedAction, ActionCode, ActionType, ActionDescription, UrlLink, UrlLabel, Reason, RuleName, \ - RuleDescription, RulePriority, Status -from eligibility_signposting_api.model.rules import Iteration, CampaignID, CampaignVersion, \ - RuleType -from fixtures.builders.model.rule import IterationFactory -from fixtures.builders.views.response_model.eligibility import EligibilityResponseFactory +from eligibility_signposting_api.model.eligibility import ( + ActionCode, + ActionDescription, + ActionType, + CohortGroupResult, + ConditionName, + IterationResult, + Reason, + RuleDescription, + RuleName, + RulePriority, + Status, + SuggestedAction, + SuggestedActions, + UrlLabel, + UrlLink, +) +from eligibility_signposting_api.model.rules import CampaignID, CampaignVersion, Iteration, RuleType +from eligibility_signposting_api.services.audit_service import AuditService +from tests.fixtures.builders.model.rule import IterationFactory +from tests.fixtures.builders.views.response_model.eligibility import EligibilityResponseFactory @pytest.fixture @@ -33,7 +49,7 @@ def test_add_request_details_sets_audit_log_on_g(app): url = "/patient-check?includeActions=Y" with app.test_request_context(url, headers=headers, method="GET"): - request.view_args = {'nhs_number': nhs_number} + request.view_args = {"nhs_number": nhs_number} AuditContext.add_request_details(request) assert hasattr(g, "audit_log") @@ -52,7 +68,7 @@ def test_add_request_details_when_headers_are_empty_sets_audit_log_on_g(app): url = "/patient-check?includeActions=Y" with app.test_request_context(url, method="GET"): - request.view_args = {'nhs_number': nhs_number} + request.view_args = {"nhs_number": nhs_number} AuditContext.add_request_details(request) assert hasattr(g, "audit_log") @@ -66,35 +82,46 @@ def test_add_request_details_when_headers_are_empty_sets_audit_log_on_g(app): assert isinstance(audit_req.request_timestamp, datetime) -def test_append_audit_condition_adds_condition_to_audit_log(app): +def test_append_audit_condition_adds_condition_to_audit_log_on_g(app): suggested_actions: SuggestedActions | None condition_name: ConditionName best_results: tuple[Iteration, IterationResult, dict[str, CohortGroupResult]] campaign_details: tuple[CampaignID | None, CampaignVersion | None] redirect_rule_details: tuple[RulePriority | None, RuleName | None] - suggested_actions = SuggestedActions(actions=[SuggestedAction( - action_code=ActionCode("ActionCode1"), - action_type=ActionType("ActionType1"), - action_description=ActionDescription("ActionDescription1"), - url_link=UrlLink("https://www.example.com"), - url_label=UrlLabel("ActionLabel1") - )]) + suggested_actions = SuggestedActions( + actions=[ + SuggestedAction( + action_code=ActionCode("ActionCode1"), + action_type=ActionType("ActionType1"), + action_description=ActionDescription("ActionDescription1"), + url_link=UrlLink("https://www.example.com"), + url_label=UrlLabel("ActionLabel1"), + ) + ] + ) condition_name = ConditionName("Condition1") iteration = IterationFactory.build() - audit_rules = [Reason( - rule_type=RuleType.filter, - rule_name=RuleName("FilterRuleName1"), - rule_description=RuleDescription("FilterRuleDescription1"), - matcher_matched=True, - rule_priority=RulePriority("1") - )] - cohort_group_result = CohortGroupResult(status=Status.actionable, cohort_code="CohortCode1", - description="CohortDescription1", audit_rules=audit_rules, - reasons=audit_rules) - iteration_result = IterationResult(status=Status.actionable, cohort_results=[cohort_group_result], - actions=suggested_actions) + audit_rules = [ + Reason( + rule_type=RuleType.filter, + rule_name=RuleName("FilterRuleName1"), + rule_description=RuleDescription("FilterRuleDescription1"), + matcher_matched=True, + rule_priority=RulePriority("1"), + ) + ] + cohort_group_result = CohortGroupResult( + status=Status.actionable, + cohort_code="CohortCode1", + description="CohortDescription1", + audit_rules=audit_rules, + reasons=audit_rules, + ) + iteration_result = IterationResult( + status=Status.actionable, cohort_results=[cohort_group_result], actions=suggested_actions + ) best_results = (iteration, iteration_result, {"CohortCode1": cohort_group_result}) campaign_details = (CampaignID("CampaignID1"), CampaignVersion("CampaignVersion1")) redirect_rule_details = (RulePriority("1"), RuleName("RedirectRuleName1")) @@ -102,8 +129,9 @@ def test_append_audit_condition_adds_condition_to_audit_log(app): with app.app_context(): g.audit_log = AuditEvent() - AuditContext.append_audit_condition(suggested_actions, condition_name, best_results, campaign_details, - redirect_rule_details) + AuditContext.append_audit_condition( + suggested_actions, condition_name, best_results, campaign_details, redirect_rule_details + ) assert g.audit_log.response.condition, condition_name cond = g.audit_log.response.condition[0] @@ -112,10 +140,11 @@ def test_append_audit_condition_adds_condition_to_audit_log(app): assert cond.status == best_results[1].status.name assert cond.status_text == best_results[1].status.name -def test_add_response_details_adds_to_audit_log(app): - eligibility_response = EligibilityResponseFactory.build(response_id=uuid.uuid4(), - meta={"last_updated": datetime(2023, 1, 1, 0, 0)}, - processed_suggestions=[]) + +def test_add_response_details_adds_to_audit_log_on_G(app): + eligibility_response = EligibilityResponseFactory.build( + response_id=uuid.uuid4(), meta={"last_updated": datetime(2023, 1, 1, 0, 0)}, processed_suggestions=[] + ) with app.app_context(): g.audit_log = AuditEvent() @@ -126,3 +155,20 @@ def test_add_response_details_adds_to_audit_log(app): assert g.audit_log.response.last_updated is eligibility_response.meta.last_updated +def test_write_to_firehose_calls_audit_service_with_correct_data_from_g(app): + mock_audit_service = Mock(spec=AuditService) + eligibility_response = EligibilityResponseFactory.build( + response_id=(uuid.uuid4()), + meta={"last_updated": (datetime(2023, 1, 1, 0, 0, tzinfo=UTC))}, + processed_suggestions=[], + ) + + with app.app_context(): + g.audit_log = AuditEvent() + + AuditContext.write_to_firehose(mock_audit_service, eligibility_response) + + assert g.audit_log.response.response_id == eligibility_response.response_id + assert g.audit_log.response.last_updated == eligibility_response.meta.last_updated + + mock_audit_service.audit.assert_called_once_with(asdict(g.audit_log)) From 84a57c9ec5a19c8ebb258fec860f7d3b02cff94b Mon Sep 17 00:00:00 2001 From: Shweta <216860557+shweta-nhs@users.noreply.github.com> Date: Fri, 4 Jul 2025 11:30:19 +0100 Subject: [PATCH 09/21] Fixed TODOs --- src/eligibility_signposting_api/audit_models.py | 2 +- .../services/calculators/eligibility_calculator.py | 3 ++- src/eligibility_signposting_api/views/eligibility.py | 2 -- tests/unit/test_audit_context.py | 9 +++++---- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/eligibility_signposting_api/audit_models.py b/src/eligibility_signposting_api/audit_models.py index d2e1ee095..0a4c60e56 100644 --- a/src/eligibility_signposting_api/audit_models.py +++ b/src/eligibility_signposting_api/audit_models.py @@ -1,5 +1,5 @@ from dataclasses import dataclass, field -from datetime import datetime, UTC +from datetime import UTC, datetime from uuid import UUID diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index e4989a4e0..b771193e6 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -199,6 +199,7 @@ def evaluate_eligibility(self, *, include_actions_flag: bool = True) -> eligibil # add actions to condition results condition_results[condition_name].actions = actions + # reset actions for the next condition actions: SuggestedActions | None = SuggestedActions([]) # add audit data @@ -256,7 +257,7 @@ def get_cohort_results(self, active_iteration: rules.Iteration) -> dict[str, Coh # Not base eligible elif cohort.cohort_label is not None: cohort_results[cohort.cohort_label] = CohortGroupResult( - (cohort.cohort_group), + cohort.cohort_group, Status.not_eligible, [], cohort.negative_description, diff --git a/src/eligibility_signposting_api/views/eligibility.py b/src/eligibility_signposting_api/views/eligibility.py index fafc73edf..253850af6 100644 --- a/src/eligibility_signposting_api/views/eligibility.py +++ b/src/eligibility_signposting_api/views/eligibility.py @@ -49,9 +49,7 @@ def check_eligibility( return handle_unknown_person_error(nhs_number) else: eligibility_response = build_eligibility_response(eligibility_status) - AuditContext.write_to_firehose(audit_service, eligibility_response) - return make_response( eligibility_response.model_dump(by_alias=True, mode="json", exclude_none=True), HTTPStatus.OK ) diff --git a/tests/unit/test_audit_context.py b/tests/unit/test_audit_context.py index 6bc7d2ded..97e65e83a 100644 --- a/tests/unit/test_audit_context.py +++ b/tests/unit/test_audit_context.py @@ -33,8 +33,7 @@ @pytest.fixture def app(): - app = Flask(__name__) - return app + return Flask(__name__) def test_add_request_details_sets_audit_log_on_g(app): @@ -141,9 +140,11 @@ def test_append_audit_condition_adds_condition_to_audit_log_on_g(app): assert cond.status_text == best_results[1].status.name -def test_add_response_details_adds_to_audit_log_on_G(app): +def test_add_response_details_adds_to_audit_log_on_g(app): eligibility_response = EligibilityResponseFactory.build( - response_id=uuid.uuid4(), meta={"last_updated": datetime(2023, 1, 1, 0, 0)}, processed_suggestions=[] + response_id=uuid.uuid4(), + meta={"last_updated": datetime(2023, 1, 1, 0, 0, tzinfo=UTC)}, + processed_suggestions=[], ) with app.app_context(): From b9961b5b20a7544ac197ea2206b4800bf4b44104 Mon Sep 17 00:00:00 2001 From: Shweta <216860557+shweta-nhs@users.noreply.github.com> Date: Fri, 4 Jul 2025 13:41:23 +0100 Subject: [PATCH 10/21] Fixed linting --- .../audit_context.py | 70 ++++++++++--------- .../audit_models.py | 24 +++---- .../calculators/eligibility_calculator.py | 6 ++ .../services/calculators/rule_calculator.py | 2 +- .../views/eligibility.py | 11 ++- tests/unit/test_audit_context.py | 28 +++----- 6 files changed, 74 insertions(+), 67 deletions(-) diff --git a/src/eligibility_signposting_api/audit_context.py b/src/eligibility_signposting_api/audit_context.py index 756cb197e..1c9aacf6c 100644 --- a/src/eligibility_signposting_api/audit_context.py +++ b/src/eligibility_signposting_api/audit_context.py @@ -2,6 +2,7 @@ from dataclasses import asdict from datetime import UTC, datetime from operator import attrgetter +from uuid import UUID from flask import Request, g @@ -36,7 +37,7 @@ class AuditContext: def add_request_details(request: Request) -> None: g.audit_log = AuditEvent() resource_id = None - if "nhs_number" in request.view_args: + if request.view_args and request.view_args["nhs_number"]: resource_id = request.view_args["nhs_number"] g.audit_log.request = RequestAuditData( nhs_number=resource_id, @@ -62,7 +63,7 @@ def add_request_details(request: Request) -> None: def append_audit_condition( suggested_actions: SuggestedActions | None, condition_name: ConditionName, - best_results: tuple[Iteration, IterationResult, dict[str, CohortGroupResult]], + best_results: tuple[Iteration | None, IterationResult | None, dict[str, CohortGroupResult] | None], campaign_details: tuple[CampaignID | None, CampaignVersion | None], redirect_rule_details: tuple[RulePriority | None, RuleName | None], ) -> None: @@ -72,35 +73,37 @@ def append_audit_condition( best_candidate = best_results[1] best_cohort_results = best_results[2] - for value in sorted(best_cohort_results.values(), key=attrgetter("cohort_code")): - audit_eligibility_cohorts.append( - AuditEligibilityCohorts(cohort_code=value.cohort_code, cohort_status=value.status.name) - ) - - audit_eligibility_cohort_groups.append( - AuditEligibilityCohortGroups( - cohort_code=value.cohort_code, cohort_status=value.status.name, cohort_text=value.description + if best_cohort_results: + for value in sorted(best_cohort_results.values(), key=attrgetter("cohort_code")): + cohort_status_name = value.status.name if value.status else None + audit_eligibility_cohorts.append( + AuditEligibilityCohorts(cohort_code=value.cohort_code, cohort_status=cohort_status_name) ) - ) - if value.audit_rules: - if best_candidate.status.name == Status.not_eligible.name: - audit_filter_rule = AuditFilterRule( - rule_priority=int(value.audit_rules[0].rule_priority), rule_name=value.audit_rules[0].rule_name - ) - if best_candidate.status.name == Status.not_actionable.name: - audit_suitability_rule = AuditSuitabilityRule( - rule_priority=int(value.audit_rules[0].rule_priority), - rule_name=value.audit_rules[0].rule_name, - rule_message=value.audit_rules[0].rule_description, + audit_eligibility_cohort_groups.append( + AuditEligibilityCohortGroups( + cohort_code=value.cohort_code, cohort_status=cohort_status_name, cohort_text=value.description ) + ) - if best_candidate.status.name == Status.actionable.name: + if value.audit_rules and best_candidate: # Check best_candidate here as well + if best_candidate.status and best_candidate.status.name == Status.not_eligible.name: + audit_filter_rule = AuditFilterRule( + rule_priority=int(value.audit_rules[0].rule_priority), + rule_name=value.audit_rules[0].rule_name, + ) + if best_candidate.status and best_candidate.status.name == Status.not_actionable.name: + audit_suitability_rule = AuditSuitabilityRule( + rule_priority=int(value.audit_rules[0].rule_priority), + rule_name=value.audit_rules[0].rule_name, + rule_message=value.audit_rules[0].rule_description, + ) + + if best_candidate and best_candidate.status and best_candidate.status.name == Status.actionable.name: audit_redirect_rule = AuditRedirectRule(redirect_rule_details[0], redirect_rule_details[1]) - if suggested_actions is None or suggested_actions == []: - audit_actions = suggested_actions - + if suggested_actions is None: + audit_actions = None elif len(suggested_actions.actions) > 0: for action in suggested_actions.actions: audit_actions.append( @@ -116,11 +119,11 @@ def append_audit_condition( audit_condition = AuditCondition( campaign_id=campaign_details[0], campaign_version=campaign_details[1], - iteration_id=best_active_iteration.id, - iteration_version=best_active_iteration.version, + iteration_id=best_active_iteration.id if best_active_iteration else None, + iteration_version=best_active_iteration.version if best_active_iteration else None, condition_name=condition_name, - status=best_candidate.status.name, - status_text=best_candidate.status.name, + status=best_candidate.status.name if best_candidate and best_candidate.status else None, + status_text=best_candidate.status.name if best_candidate and best_candidate.status else None, eligibility_cohorts=audit_eligibility_cohorts, eligibility_cohort_groups=audit_eligibility_cohort_groups, filter_rules=audit_filter_rule, @@ -132,11 +135,10 @@ def append_audit_condition( g.audit_log.response.condition.append(audit_condition) @staticmethod - def add_response_details(response) -> None: - g.audit_log.response.response_id = response.response_id - g.audit_log.response.last_updated = response.meta.last_updated + def add_response_details(response_id: UUID, last_updated: datetime) -> None: + g.audit_log.response.response_id = response_id + g.audit_log.response.last_updated = last_updated @staticmethod - def write_to_firehose(service: AuditService, response) -> None: - AuditContext.add_response_details(response) + def write_to_firehose(service: AuditService) -> None: service.audit(asdict(g.audit_log)) diff --git a/src/eligibility_signposting_api/audit_models.py b/src/eligibility_signposting_api/audit_models.py index 0a4c60e56..b7e2a4f82 100644 --- a/src/eligibility_signposting_api/audit_models.py +++ b/src/eligibility_signposting_api/audit_models.py @@ -5,10 +5,10 @@ @dataclass class RequestAuditHeader: - x_request_id: str = None - x_correlation_id: str = None - nhsd_end_user_organisation_ods: str = None - nhsd_application_id: str = None + x_request_id: str | None = None + x_correlation_id: str | None = None + nhsd_end_user_organisation_ods: str | None = None + nhsd_application_id: str | None = None @dataclass @@ -70,19 +70,19 @@ class AuditAction: @dataclass class AuditCondition: - campaign_id: str = None - campaign_version: str = None - iteration_id: str = None - iteration_version: str = None - condition_name: str = None - status: str = None - status_text: str = None + campaign_id: str | None = None + campaign_version: str | None = None + iteration_id: str | None = None + iteration_version: str | None = None + condition_name: str | None = None + status: str | None = None + status_text: str | None = None eligibility_cohorts: list[AuditEligibilityCohorts] | None = None eligibility_cohort_groups: list[AuditEligibilityCohortGroups] | None = None filter_rules: AuditFilterRule | None = None suitability_rules: AuditSuitabilityRule | None = None action_rule: AuditRedirectRule | None = None - actions: list[AuditAction] = field(default_factory=list) + actions: list[AuditAction] | None = field(default_factory=list) @dataclass diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index b771193e6..9760e4f29 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -144,6 +144,12 @@ def evaluate_eligibility(self, *, include_actions_flag: bool = True) -> eligibil redirect_rule_priority, redirect_rule_name = None, None for condition_name, campaign_group in self.campaigns_grouped_by_condition_name: + best_active_iteration: Iteration | None + best_candidate: IterationResult + best_campaign_id: CampaignID | None + best_campaign_version: CampaignVersion | None + best_cohort_results: dict[str, CohortGroupResult] | None + iteration_results: dict[ str, tuple[Iteration, IterationResult, CampaignID, CampaignVersion, dict[str, CohortGroupResult]] ] = {} diff --git a/src/eligibility_signposting_api/services/calculators/rule_calculator.py b/src/eligibility_signposting_api/services/calculators/rule_calculator.py index c24fb591b..145a1e89f 100644 --- a/src/eligibility_signposting_api/services/calculators/rule_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/rule_calculator.py @@ -24,7 +24,7 @@ def evaluate_exclusion(self) -> tuple[eligibility.Status, eligibility.Reason]: reason = eligibility.Reason( rule_name=eligibility.RuleName(self.rule.name), rule_type=eligibility.RuleType(self.rule.type), - rule_priority=eligibility.RulePriority(self.rule.priority), + rule_priority=eligibility.RulePriority(str(self.rule.priority)), rule_description=eligibility.RuleDescription(self.rule.description), matcher_matched=matcher_matched, ) diff --git a/src/eligibility_signposting_api/views/eligibility.py b/src/eligibility_signposting_api/views/eligibility.py index 253850af6..88b059293 100644 --- a/src/eligibility_signposting_api/views/eligibility.py +++ b/src/eligibility_signposting_api/views/eligibility.py @@ -49,7 +49,7 @@ def check_eligibility( return handle_unknown_person_error(nhs_number) else: eligibility_response = build_eligibility_response(eligibility_status) - AuditContext.write_to_firehose(audit_service, eligibility_response) + AuditContext.write_to_firehose(audit_service) return make_response( eligibility_response.model_dump(by_alias=True, mode="json", exclude_none=True), HTTPStatus.OK ) @@ -122,9 +122,14 @@ def build_eligibility_response(eligibility_status: EligibilityStatus) -> eligibi processed_suggestions.append(suggestions) + response_id = uuid.uuid4() + updated = eligibility.LastUpdated(datetime.now(tz=UTC)) + + AuditContext.add_response_details(response_id, updated) + return eligibility.EligibilityResponse( # pyright: ignore[reportCallIssue] - responseId=uuid.uuid4(), # pyright: ignore[reportCallIssue] - meta=eligibility.Meta(lastUpdated=eligibility.LastUpdated(datetime.now(tz=UTC))), + responseId=response_id, # pyright: ignore[reportCallIssue] + meta=eligibility.Meta(lastUpdated=updated), # pyright: ignore[reportCallIssue] processedSuggestions=processed_suggestions, ) diff --git a/tests/unit/test_audit_context.py b/tests/unit/test_audit_context.py index 97e65e83a..3890ac4dd 100644 --- a/tests/unit/test_audit_context.py +++ b/tests/unit/test_audit_context.py @@ -28,7 +28,6 @@ from eligibility_signposting_api.model.rules import CampaignID, CampaignVersion, Iteration, RuleType from eligibility_signposting_api.services.audit_service import AuditService from tests.fixtures.builders.model.rule import IterationFactory -from tests.fixtures.builders.views.response_model.eligibility import EligibilityResponseFactory @pytest.fixture @@ -141,35 +140,30 @@ def test_append_audit_condition_adds_condition_to_audit_log_on_g(app): def test_add_response_details_adds_to_audit_log_on_g(app): - eligibility_response = EligibilityResponseFactory.build( - response_id=uuid.uuid4(), - meta={"last_updated": datetime(2023, 1, 1, 0, 0, tzinfo=UTC)}, - processed_suggestions=[], - ) + response_id = uuid.uuid4() + last_updated = datetime(2023, 1, 1, 0, 0, tzinfo=UTC) with app.app_context(): g.audit_log = AuditEvent() - AuditContext.add_response_details(eligibility_response) + AuditContext.add_response_details(response_id, last_updated) - assert g.audit_log.response.response_id == eligibility_response.response_id - assert g.audit_log.response.last_updated is eligibility_response.meta.last_updated + assert g.audit_log.response.response_id == response_id + assert g.audit_log.response.last_updated is last_updated def test_write_to_firehose_calls_audit_service_with_correct_data_from_g(app): mock_audit_service = Mock(spec=AuditService) - eligibility_response = EligibilityResponseFactory.build( - response_id=(uuid.uuid4()), - meta={"last_updated": (datetime(2023, 1, 1, 0, 0, tzinfo=UTC))}, - processed_suggestions=[], - ) + response_id = uuid.uuid4() + last_updated = datetime(2023, 1, 1, 0, 0, tzinfo=UTC) with app.app_context(): g.audit_log = AuditEvent() - AuditContext.write_to_firehose(mock_audit_service, eligibility_response) + AuditContext.add_response_details(response_id, last_updated) + AuditContext.write_to_firehose(mock_audit_service) - assert g.audit_log.response.response_id == eligibility_response.response_id - assert g.audit_log.response.last_updated == eligibility_response.meta.last_updated + assert g.audit_log.response.response_id == response_id + assert g.audit_log.response.last_updated == last_updated mock_audit_service.audit.assert_called_once_with(asdict(g.audit_log)) From 5fbab26851989c443e502dc46bd62c68cd34e807 Mon Sep 17 00:00:00 2001 From: Shweta <216860557+shweta-nhs@users.noreply.github.com> Date: Fri, 4 Jul 2025 14:33:39 +0100 Subject: [PATCH 11/21] Uses list[SuggestedAction] instead of SuggestedActions --- .../audit_context.py | 10 ++++---- .../calculators/eligibility_calculator.py | 4 +++- tests/unit/test_audit_context.py | 24 +++++++++---------- tests/unit/views/test_eligibility.py | 8 +++++++ 4 files changed, 27 insertions(+), 19 deletions(-) diff --git a/src/eligibility_signposting_api/audit_context.py b/src/eligibility_signposting_api/audit_context.py index 1c9aacf6c..d282e93bb 100644 --- a/src/eligibility_signposting_api/audit_context.py +++ b/src/eligibility_signposting_api/audit_context.py @@ -24,7 +24,7 @@ ConditionName, IterationResult, Status, - SuggestedActions, + SuggestedAction, ) from eligibility_signposting_api.model.rules import CampaignID, CampaignVersion, Iteration, RuleName, RulePriority from eligibility_signposting_api.services.audit_service import AuditService @@ -61,7 +61,7 @@ def add_request_details(request: Request) -> None: @staticmethod def append_audit_condition( - suggested_actions: SuggestedActions | None, + suggested_actions: list[SuggestedAction] | None, condition_name: ConditionName, best_results: tuple[Iteration | None, IterationResult | None, dict[str, CohortGroupResult] | None], campaign_details: tuple[CampaignID | None, CampaignVersion | None], @@ -104,14 +104,14 @@ def append_audit_condition( if suggested_actions is None: audit_actions = None - elif len(suggested_actions.actions) > 0: - for action in suggested_actions.actions: + elif len(suggested_actions) > 0: + for action in suggested_actions: audit_actions.append( AuditAction( action_code=action.action_code, action_type=action.action_type, action_description=action.action_description, - action_url=action.url_link, + action_url=str(action.url_link), action_url_label=action.url_label, ) ) diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index 548282770..c0d6aea74 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -221,7 +221,9 @@ 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) -> tuple[list[SuggestedAction] | None, RulePriority | None, RuleName | None]: + def handle_redirect_rules( + self, best_active_iteration: Iteration + ) -> tuple[list[SuggestedAction] | None, RulePriority | None, RuleName | 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) diff --git a/tests/unit/test_audit_context.py b/tests/unit/test_audit_context.py index 3890ac4dd..accb55ce3 100644 --- a/tests/unit/test_audit_context.py +++ b/tests/unit/test_audit_context.py @@ -5,6 +5,7 @@ import pytest from flask import Flask, g, request +from pydantic import HttpUrl from eligibility_signposting_api.audit_context import AuditContext from eligibility_signposting_api.audit_models import AuditEvent @@ -21,7 +22,6 @@ RulePriority, Status, SuggestedAction, - SuggestedActions, UrlLabel, UrlLink, ) @@ -81,23 +81,21 @@ def test_add_request_details_when_headers_are_empty_sets_audit_log_on_g(app): def test_append_audit_condition_adds_condition_to_audit_log_on_g(app): - suggested_actions: SuggestedActions | None + suggested_actions: list[SuggestedAction] | None condition_name: ConditionName best_results: tuple[Iteration, IterationResult, dict[str, CohortGroupResult]] campaign_details: tuple[CampaignID | None, CampaignVersion | None] redirect_rule_details: tuple[RulePriority | None, RuleName | None] - suggested_actions = SuggestedActions( - actions=[ - SuggestedAction( - action_code=ActionCode("ActionCode1"), - action_type=ActionType("ActionType1"), - action_description=ActionDescription("ActionDescription1"), - url_link=UrlLink("https://www.example.com"), - url_label=UrlLabel("ActionLabel1"), - ) - ] - ) + suggested_actions = [ + SuggestedAction( + action_code=ActionCode("ActionCode1"), + action_type=ActionType("ActionType1"), + action_description=ActionDescription("ActionDescription1"), + url_link=UrlLink(HttpUrl("https://example.com")), + url_label=UrlLabel("ActionLabel1"), + ) + ] condition_name = ConditionName("Condition1") iteration = IterationFactory.build() diff --git a/tests/unit/views/test_eligibility.py b/tests/unit/views/test_eligibility.py index 6d14f7d82..5b7616776 100644 --- a/tests/unit/views/test_eligibility.py +++ b/tests/unit/views/test_eligibility.py @@ -615,6 +615,10 @@ def test_excludes_nulls_via_build_response(client: FlaskClient): "eligibility_signposting_api.views.eligibility.EligibilityService.get_eligibility_status", return_value=MagicMock(), # No effect ), + patch( + "eligibility_signposting_api.views.eligibility.AuditService.audit", + return_value=MagicMock(), # No effect + ), patch( "eligibility_signposting_api.views.eligibility.build_eligibility_response", return_value=mocked_response, @@ -663,6 +667,10 @@ def test_build_response_include_values_that_are_not_null(client: FlaskClient): "eligibility_signposting_api.views.eligibility.EligibilityService.get_eligibility_status", return_value=MagicMock(), # No effect ), + patch( + "eligibility_signposting_api.views.eligibility.AuditService.audit", + return_value=MagicMock(), # No effect + ), patch( "eligibility_signposting_api.views.eligibility.build_eligibility_response", return_value=mocked_response, From 7a9a2d715882c139295010a4c868946c802c7321 Mon Sep 17 00:00:00 2001 From: Shweta <216860557+shweta-nhs@users.noreply.github.com> Date: Fri, 4 Jul 2025 15:17:35 +0100 Subject: [PATCH 12/21] Adds internal action code to audit actions --- .../audit_context.py | 3 +- .../audit_models.py | 2 +- .../model/eligibility.py | 2 + .../calculators/eligibility_calculator.py | 2 + .../test_eligibility_calculator.py | 149 ++++++++++++++---- tests/unit/test_audit_context.py | 29 +++- 6 files changed, 149 insertions(+), 38 deletions(-) diff --git a/src/eligibility_signposting_api/audit_context.py b/src/eligibility_signposting_api/audit_context.py index d282e93bb..b8a64fa39 100644 --- a/src/eligibility_signposting_api/audit_context.py +++ b/src/eligibility_signposting_api/audit_context.py @@ -100,7 +100,7 @@ def append_audit_condition( ) if best_candidate and best_candidate.status and best_candidate.status.name == Status.actionable.name: - audit_redirect_rule = AuditRedirectRule(redirect_rule_details[0], redirect_rule_details[1]) + audit_redirect_rule = AuditRedirectRule(int(redirect_rule_details[0]), redirect_rule_details[1]) if suggested_actions is None: audit_actions = None @@ -108,6 +108,7 @@ def append_audit_condition( for action in suggested_actions: audit_actions.append( AuditAction( + internal_action_code=action.internal_action_code, action_code=action.action_code, action_type=action.action_type, action_description=action.action_description, diff --git a/src/eligibility_signposting_api/audit_models.py b/src/eligibility_signposting_api/audit_models.py index b7e2a4f82..b3dfa4e0a 100644 --- a/src/eligibility_signposting_api/audit_models.py +++ b/src/eligibility_signposting_api/audit_models.py @@ -60,7 +60,7 @@ class AuditRedirectRule: @dataclass class AuditAction: - internal_name: str | None = None + internal_action_code: str | None = None action_type: str | None = None action_code: str | None = None action_description: str | None = None diff --git a/src/eligibility_signposting_api/model/eligibility.py b/src/eligibility_signposting_api/model/eligibility.py index 6c9e86411..bad948361 100644 --- a/src/eligibility_signposting_api/model/eligibility.py +++ b/src/eligibility_signposting_api/model/eligibility.py @@ -17,6 +17,7 @@ RuleDescription = NewType("RuleDescription", str) RulePriority = NewType("RulePriority", str) +InternalActionCode = NewType("InternalActionCode", str) ActionType = NewType("ActionType", str) ActionCode = NewType("ActionCode", str) ActionDescription = NewType("ActionDescription", str) @@ -81,6 +82,7 @@ class SuggestedAction: action_description: ActionDescription | None url_link: UrlLink | None url_label: UrlLabel | None + internal_action_code: InternalActionCode | None = 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 c0d6aea74..baa7c529f 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -30,6 +30,7 @@ CohortGroupResult, Condition, ConditionName, + InternalActionCode, IterationResult, Status, SuggestedAction, @@ -399,6 +400,7 @@ def get_actions_from_comms(action_mapper: ActionsMapper, comms: str) -> list[Sug if action is not None: suggested_actions.append( SuggestedAction( + internal_action_code=InternalActionCode(comm), action_type=ActionType(action.action_type), action_code=ActionCode(action.action_code), action_description=ActionDescription(action.action_description) diff --git a/tests/unit/services/calculators/test_eligibility_calculator.py b/tests/unit/services/calculators/test_eligibility_calculator.py index 1ac57cbdb..d18e490c2 100644 --- a/tests/unit/services/calculators/test_eligibility_calculator.py +++ b/tests/unit/services/calculators/test_eligibility_calculator.py @@ -21,7 +21,7 @@ Status, SuggestedAction, UrlLabel, - UrlLink, + UrlLink, InternalActionCode, ) from eligibility_signposting_api.model.rules import ActionsMapper, AvailableAction from eligibility_signposting_api.services.calculators.eligibility_calculator import EligibilityCalculator @@ -614,11 +614,25 @@ def test_multiple_conditions_where_both_are_actionable(faker: Faker): is_condition() .with_condition_name(ConditionName("RSV")) .and_status(Status.actionable) - .and_actions([suggested_action_for_default_comms]), + .and_actions([SuggestedAction( + internal_action_code=InternalActionCode("defaultcomms"), + action_type=ActionType("CareCardWithText"), + action_code=ActionCode("BookLocal"), + action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), + url_link=None, + url_label=None, + )]), is_condition() .with_condition_name(ConditionName("COVID")) .and_status(Status.actionable) - .and_actions([suggested_action_for_book_nbs]), + .and_actions([SuggestedAction( + internal_action_code=InternalActionCode("ActionCode1"), + action_type=ActionType("ButtonAuthLink"), + action_code=ActionCode("BookNBS"), + action_description=ActionDescription("Action description"), + url_link=UrlLink(HttpUrl("https://www.nhs.uk/book-rsv")), + url_label=UrlLabel("Continue to booking"), + )]), ) ), ) @@ -1738,7 +1752,7 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( ActionType="ButtonAuthLink", ExternalRoutingCode="BookNBS", ActionDescription="Action description", - UrlLink="http://www.nhs.uk/book-rsv", + UrlLink=HttpUrl("https://www.nhs.uk/book-rsv"), UrlLabel="Continue to booking", ) @@ -1748,23 +1762,6 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( ActionDescription="You can get an RSV vaccination at your GP surgery", ) -suggested_action_for_book_nbs = 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=UrlLink(book_nbs_comms.url_link), - url_label=UrlLabel(book_nbs_comms.url_label), -) - -suggested_action_for_default_comms = SuggestedAction( - action_type=ActionType(default_comms_detail.action_type), - action_code=ActionCode(default_comms_detail.action_code), - action_description=ActionDescription(default_comms_detail.action_description), - url_link=None, - url_label=None, -) - - @pytest.mark.parametrize( ("test_comment", "default_comms_routing", "comms_routing", "actions_mapper", "expected_actions"), [ @@ -1774,7 +1771,14 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcomms", "InternalBookNBS", {"InternalBookNBS": book_nbs_comms, "defaultcomms": default_comms_detail}, - [suggested_action_for_book_nbs], + [SuggestedAction( + internal_action_code=InternalActionCode("InternalBookNBS"), + action_type=ActionType("ButtonAuthLink"), + action_code=ActionCode("BookNBS"), + action_description=ActionDescription("Action description"), + url_link=UrlLink(HttpUrl("https://www.nhs.uk/book-rsv")), + url_label=UrlLabel("Continue to booking"), + )], ), ( """Rule match: default_comms_routing has multiple values, @@ -1782,7 +1786,21 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcomms1|defaultcomms2", None, {"defaultcomms1": default_comms_detail, "defaultcomms2": default_comms_detail}, - [suggested_action_for_default_comms, suggested_action_for_default_comms], + [SuggestedAction( + internal_action_code=InternalActionCode("defaultcomms1"), + action_type=ActionType("CareCardWithText"), + action_code=ActionCode("BookLocal"), + action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), + url_link=None, + url_label=None, + ), SuggestedAction( + internal_action_code=InternalActionCode("defaultcomms2"), + action_type=ActionType("CareCardWithText"), + action_code=ActionCode("BookLocal"), + action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), + url_link=None, + url_label=None, + )], ), ( """Rule match: default_comms_routing has multiple values, @@ -1790,7 +1808,14 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcomms1", "", {"defaultcomms1": default_comms_detail}, - [suggested_action_for_default_comms], + [SuggestedAction( + internal_action_code=InternalActionCode("defaultcomms1"), + action_type=ActionType("CareCardWithText"), + action_code=ActionCode("BookLocal"), + action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), + url_link=None, + url_label=None, + )], ), ( """Rule match: default_comms_routing present, @@ -1798,7 +1823,14 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcomms", "InternalBookNBS", {"defaultcomms": default_comms_detail}, - [suggested_action_for_default_comms], + [SuggestedAction( + internal_action_code=InternalActionCode("defaultcomms"), + action_type=ActionType("CareCardWithText"), + action_code=ActionCode("BookLocal"), + action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), + url_link=None, + url_label=None, + )], ), ( """Rule match: default_comms_routing present, @@ -1806,7 +1838,14 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcomms", "InvalidCode", {"defaultcomms": default_comms_detail}, - [suggested_action_for_default_comms], + [SuggestedAction( + internal_action_code=InternalActionCode("defaultcomms"), + action_type=ActionType("CareCardWithText"), + action_code=ActionCode("BookLocal"), + action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), + url_link=None, + url_label=None, + )], ), ( """Rule match: action_mapper present without url, @@ -1822,6 +1861,7 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( }, [ SuggestedAction( + internal_action_code=InternalActionCode("InternalBookNBS"), action_type=ActionType(book_nbs_comms.action_type), action_code=ActionCode(book_nbs_comms.action_code), action_description=ActionDescription(book_nbs_comms.action_description), @@ -1844,7 +1884,14 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "", "InternalBookNBS", {"InternalBookNBS": book_nbs_comms}, - [suggested_action_for_book_nbs], + [SuggestedAction( + internal_action_code=InternalActionCode("InternalBookNBS"), + action_type=ActionType("ButtonAuthLink"), + action_code=ActionCode("BookNBS"), + action_description=ActionDescription("Action description"), + url_link=UrlLink(HttpUrl("https://www.nhs.uk/book-rsv")), + url_label=UrlLabel("Continue to booking"), + )], ), ( """Rule match: default_comms_routing present, @@ -1860,7 +1907,14 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcomms1|invaliddefault", None, {"defaultcomms1": default_comms_detail}, - [suggested_action_for_default_comms], + [SuggestedAction( + internal_action_code=InternalActionCode("defaultcomms1"), + action_type=ActionType("CareCardWithText"), + action_code=ActionCode("BookLocal"), + action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), + url_link=None, + url_label=None, + )], ), ], ) @@ -1963,7 +2017,14 @@ 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([suggested_action_for_book_nbs])) + .and_actions(equal_to([SuggestedAction( + internal_action_code=InternalActionCode("ActionCode1"), + action_type=ActionType("ButtonAuthLink"), + action_code=ActionCode("BookNBS"), + action_description=ActionDescription("Action description"), + url_link=UrlLink(HttpUrl("https://www.nhs.uk/book-rsv")), + url_label=UrlLabel("Continue to booking"), + )])) ) ), test_comment, @@ -2019,7 +2080,14 @@ 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([suggested_action_for_book_nbs])) + .and_actions(equal_to([SuggestedAction( + internal_action_code=InternalActionCode("rule_1_comms_routing"), + action_type=ActionType("ButtonAuthLink"), + action_code=ActionCode("BookNBS"), + action_description=ActionDescription("Action description"), + url_link=UrlLink(HttpUrl("https://www.nhs.uk/book-rsv")), + url_label=UrlLabel("Continue to booking"), + )])) ) ), ) @@ -2073,7 +2141,14 @@ 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([suggested_action_for_default_comms])) + .and_actions(equal_to([SuggestedAction( + internal_action_code=InternalActionCode("defaultcomms"), + action_type=ActionType("CareCardWithText"), + action_code=ActionCode("BookLocal"), + action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), + url_link=None, + url_label=None, + )])) ) ), ) @@ -2103,7 +2178,7 @@ def test_only_highest_priority_rule_is_applied_and_return_actions_only_for_that_ ActionType="AuthLink", ExternalRoutingCode="BookNBS", ActionDescription="Action description", - UrlLink="http://www.nhs.uk/book-rsv", + UrlLink=HttpUrl("https://www.nhs.uk/book-rsv"), UrlLabel="Continue to booking", ), "defaultcomms": default_comms_detail, @@ -2125,6 +2200,7 @@ def test_only_highest_priority_rule_is_applied_and_return_actions_only_for_that_ actual = calculator.evaluate_eligibility() expected_actions = SuggestedAction( + internal_action_code=InternalActionCode("rule_1_comms_routing"), action_type=ActionType("ButtonAuthLink"), action_code=ActionCode("BookNBS"), action_description=ActionDescription("Action description"), @@ -2187,7 +2263,14 @@ 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([suggested_action_for_book_nbs])) + .and_actions(equal_to([SuggestedAction( + internal_action_code=InternalActionCode("book_nbs"), + action_type=ActionType("ButtonAuthLink"), + action_code=ActionCode("BookNBS"), + action_description=ActionDescription("Action description"), + url_link=UrlLink(HttpUrl("https://www.nhs.uk/book-rsv")), + url_label=UrlLabel("Continue to booking"), + )])) ) ), ) diff --git a/tests/unit/test_audit_context.py b/tests/unit/test_audit_context.py index accb55ce3..a141f0f8d 100644 --- a/tests/unit/test_audit_context.py +++ b/tests/unit/test_audit_context.py @@ -8,7 +8,7 @@ from pydantic import HttpUrl from eligibility_signposting_api.audit_context import AuditContext -from eligibility_signposting_api.audit_models import AuditEvent +from eligibility_signposting_api.audit_models import AuditEvent, AuditAction from eligibility_signposting_api.model.eligibility import ( ActionCode, ActionDescription, @@ -23,7 +23,7 @@ Status, SuggestedAction, UrlLabel, - UrlLink, + UrlLink, InternalActionCode, ) from eligibility_signposting_api.model.rules import CampaignID, CampaignVersion, Iteration, RuleType from eligibility_signposting_api.services.audit_service import AuditService @@ -89,10 +89,11 @@ def test_append_audit_condition_adds_condition_to_audit_log_on_g(app): suggested_actions = [ SuggestedAction( + internal_action_code=InternalActionCode("InternalActionCode1"), action_code=ActionCode("ActionCode1"), action_type=ActionType("ActionType1"), action_description=ActionDescription("ActionDescription1"), - url_link=UrlLink(HttpUrl("https://example.com")), + url_link=UrlLink(HttpUrl("https://example.com/")), url_label=UrlLabel("ActionLabel1"), ) ] @@ -129,12 +130,34 @@ def test_append_audit_condition_adds_condition_to_audit_log_on_g(app): suggested_actions, condition_name, best_results, campaign_details, redirect_rule_details ) + expected_audit_action = [AuditAction( + internal_action_code="InternalActionCode1", + action_code="ActionCode1", + action_type="ActionType1", + action_description="ActionDescription1", + action_url="https://example.com/", + action_url_label="ActionLabel1", + )] + assert g.audit_log.response.condition, condition_name cond = g.audit_log.response.condition[0] assert cond.condition_name == condition_name assert cond.campaign_id == campaign_details[0] + assert cond.campaign_version == campaign_details[1] + assert cond.iteration_id == iteration.id + assert cond.iteration_version == iteration.version assert cond.status == best_results[1].status.name assert cond.status_text == best_results[1].status.name + assert cond.actions == expected_audit_action + assert cond.action_rule.rule_priority == 1 + assert cond.action_rule.rule_name == "RedirectRuleName1" + assert cond.suitability_rules is None + assert cond.filter_rules is None + assert cond.eligibility_cohorts[0].cohort_code is "CohortCode1" + assert cond.eligibility_cohorts[0].cohort_status is "actionable" + assert cond.eligibility_cohort_groups[0].cohort_code is "CohortCode1" + assert cond.eligibility_cohort_groups[0].cohort_status is "actionable" + assert cond.eligibility_cohort_groups[0].cohort_text is "CohortDescription1" def test_add_response_details_adds_to_audit_log_on_g(app): From 92b0c0b42a6fa64c1b60080aac75112acaa24f75 Mon Sep 17 00:00:00 2001 From: Shweta <216860557+shweta-nhs@users.noreply.github.com> Date: Fri, 4 Jul 2025 15:42:45 +0100 Subject: [PATCH 13/21] Fix todo's --- .../audit_context.py | 4 +- .../audit_models.py | 4 +- .../calculators/eligibility_calculator.py | 8 +- .../lambda/test_app_running_as_lambda.py | 2 +- .../test_eligibility_calculator.py | 275 +++++++++++------- tests/unit/test_audit_context.py | 35 ++- 6 files changed, 190 insertions(+), 138 deletions(-) diff --git a/src/eligibility_signposting_api/audit_context.py b/src/eligibility_signposting_api/audit_context.py index b8a64fa39..5ba5fc346 100644 --- a/src/eligibility_signposting_api/audit_context.py +++ b/src/eligibility_signposting_api/audit_context.py @@ -100,7 +100,7 @@ def append_audit_condition( ) if best_candidate and best_candidate.status and best_candidate.status.name == Status.actionable.name: - audit_redirect_rule = AuditRedirectRule(int(redirect_rule_details[0]), redirect_rule_details[1]) + audit_redirect_rule = AuditRedirectRule(redirect_rule_details[0], redirect_rule_details[1]) if suggested_actions is None: audit_actions = None @@ -112,7 +112,7 @@ def append_audit_condition( action_code=action.action_code, action_type=action.action_type, action_description=action.action_description, - action_url=str(action.url_link), + action_url=str(action.url_link) if action.url_link else None, action_url_label=action.url_label, ) ) diff --git a/src/eligibility_signposting_api/audit_models.py b/src/eligibility_signposting_api/audit_models.py index b3dfa4e0a..51b759f4f 100644 --- a/src/eligibility_signposting_api/audit_models.py +++ b/src/eligibility_signposting_api/audit_models.py @@ -47,14 +47,14 @@ class AuditFilterRule: @dataclass class AuditSuitabilityRule: - rule_priority: int | None = None + rule_priority: str | None = None rule_name: str | None = None rule_message: str | None = None @dataclass class AuditRedirectRule: - rule_priority: int | None = None + rule_priority: str | None = None rule_name: str | None = None diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index baa7c529f..125d4a81b 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -97,7 +97,7 @@ def get_the_best_cohort_memberships( status=cc.status, reasons=cc.reasons, description=(cc.description or "").strip() if cc.description else "", - audit_rules=cc.audit_rules, # TODO: remove this or fix it! + audit_rules=cc.audit_rules, ) for cc in best_cohorts ] @@ -182,7 +182,6 @@ def evaluate_eligibility(self, *, include_actions_flag: bool = True) -> eligibil ) = max(iteration_results.items(), key=lambda item: item[1][1].status.value) else: best_candidate = IterationResult(eligibility.Status.not_eligible, [], actions) - # TODO: Refactor? best_campaign_id = None best_campaign_version = None best_active_iteration = None @@ -209,7 +208,6 @@ def evaluate_eligibility(self, *, include_actions_flag: bool = True) -> eligibil actions: list[SuggestedAction] | None = [] # add audit data - # TODO: Do we need to use deduplicated cohort results from build_condition_results instead of here? AuditContext.append_audit_condition( condition_results[condition_name].actions, condition_name, @@ -267,7 +265,7 @@ def get_cohort_results(self, active_iteration: rules.Iteration) -> dict[str, Coh Status.not_eligible, [], cohort.negative_description, - [], # TODO: remove this or fix it! + [], ) return cohort_results @@ -291,7 +289,7 @@ def build_condition_results(condition_results: dict[ConditionName, IterationResu reasons=[reason for cohort in group for reason in cohort.reasons], # get the first nonempty description description=next((c.description for c in group if c.description), group[0].description), - audit_rules=[], # TODO: remove this or fix it! + audit_rules=[], ) for group_cohort_code, group in grouped_cohort_results.items() if group diff --git a/tests/integration/lambda/test_app_running_as_lambda.py b/tests/integration/lambda/test_app_running_as_lambda.py index 918ae3e29..707d1e85b 100644 --- a/tests/integration/lambda/test_app_running_as_lambda.py +++ b/tests/integration/lambda/test_app_running_as_lambda.py @@ -382,7 +382,7 @@ def test_given_person_has_unique_status_for_different_conditions_with_audit( # "action_rule": {"rule_priority": 20, "rule_name": "In QE1"}, "actions": [ { - "internal_name": None, # TODO: FIX! + "internal_action_code": "defaultcomms", "action_type": "defaultcomms", "action_code": "action_code", "action_description": None, diff --git a/tests/unit/services/calculators/test_eligibility_calculator.py b/tests/unit/services/calculators/test_eligibility_calculator.py index d18e490c2..86d7ff487 100644 --- a/tests/unit/services/calculators/test_eligibility_calculator.py +++ b/tests/unit/services/calculators/test_eligibility_calculator.py @@ -15,13 +15,14 @@ ActionType, ConditionName, DateOfBirth, + InternalActionCode, NHSNumber, Postcode, RuleDescription, Status, SuggestedAction, UrlLabel, - UrlLink, InternalActionCode, + UrlLink, ) from eligibility_signposting_api.model.rules import ActionsMapper, AvailableAction from eligibility_signposting_api.services.calculators.eligibility_calculator import EligibilityCalculator @@ -614,25 +615,33 @@ def test_multiple_conditions_where_both_are_actionable(faker: Faker): is_condition() .with_condition_name(ConditionName("RSV")) .and_status(Status.actionable) - .and_actions([SuggestedAction( - internal_action_code=InternalActionCode("defaultcomms"), - action_type=ActionType("CareCardWithText"), - action_code=ActionCode("BookLocal"), - action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), - url_link=None, - url_label=None, - )]), + .and_actions( + [ + SuggestedAction( + internal_action_code=InternalActionCode("defaultcomms"), + action_type=ActionType("CareCardWithText"), + action_code=ActionCode("BookLocal"), + action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), + url_link=None, + url_label=None, + ) + ] + ), is_condition() .with_condition_name(ConditionName("COVID")) .and_status(Status.actionable) - .and_actions([SuggestedAction( - internal_action_code=InternalActionCode("ActionCode1"), - action_type=ActionType("ButtonAuthLink"), - action_code=ActionCode("BookNBS"), - action_description=ActionDescription("Action description"), - url_link=UrlLink(HttpUrl("https://www.nhs.uk/book-rsv")), - url_label=UrlLabel("Continue to booking"), - )]), + .and_actions( + [ + SuggestedAction( + internal_action_code=InternalActionCode("ActionCode1"), + action_type=ActionType("ButtonAuthLink"), + action_code=ActionCode("BookNBS"), + action_description=ActionDescription("Action description"), + url_link=UrlLink(HttpUrl("https://www.nhs.uk/book-rsv")), + url_label=UrlLabel("Continue to booking"), + ) + ] + ), ) ), ) @@ -1762,6 +1771,7 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( ActionDescription="You can get an RSV vaccination at your GP surgery", ) + @pytest.mark.parametrize( ("test_comment", "default_comms_routing", "comms_routing", "actions_mapper", "expected_actions"), [ @@ -1771,14 +1781,16 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcomms", "InternalBookNBS", {"InternalBookNBS": book_nbs_comms, "defaultcomms": default_comms_detail}, - [SuggestedAction( - internal_action_code=InternalActionCode("InternalBookNBS"), - action_type=ActionType("ButtonAuthLink"), - action_code=ActionCode("BookNBS"), - action_description=ActionDescription("Action description"), - url_link=UrlLink(HttpUrl("https://www.nhs.uk/book-rsv")), - url_label=UrlLabel("Continue to booking"), - )], + [ + SuggestedAction( + internal_action_code=InternalActionCode("InternalBookNBS"), + action_type=ActionType("ButtonAuthLink"), + action_code=ActionCode("BookNBS"), + action_description=ActionDescription("Action description"), + url_link=UrlLink(HttpUrl("https://www.nhs.uk/book-rsv")), + url_label=UrlLabel("Continue to booking"), + ) + ], ), ( """Rule match: default_comms_routing has multiple values, @@ -1786,21 +1798,24 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcomms1|defaultcomms2", None, {"defaultcomms1": default_comms_detail, "defaultcomms2": default_comms_detail}, - [SuggestedAction( - internal_action_code=InternalActionCode("defaultcomms1"), - action_type=ActionType("CareCardWithText"), - action_code=ActionCode("BookLocal"), - action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), - url_link=None, - url_label=None, - ), SuggestedAction( - internal_action_code=InternalActionCode("defaultcomms2"), - action_type=ActionType("CareCardWithText"), - action_code=ActionCode("BookLocal"), - action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), - url_link=None, - url_label=None, - )], + [ + SuggestedAction( + internal_action_code=InternalActionCode("defaultcomms1"), + action_type=ActionType("CareCardWithText"), + action_code=ActionCode("BookLocal"), + action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), + url_link=None, + url_label=None, + ), + SuggestedAction( + internal_action_code=InternalActionCode("defaultcomms2"), + action_type=ActionType("CareCardWithText"), + action_code=ActionCode("BookLocal"), + action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), + url_link=None, + url_label=None, + ), + ], ), ( """Rule match: default_comms_routing has multiple values, @@ -1808,14 +1823,16 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcomms1", "", {"defaultcomms1": default_comms_detail}, - [SuggestedAction( - internal_action_code=InternalActionCode("defaultcomms1"), - action_type=ActionType("CareCardWithText"), - action_code=ActionCode("BookLocal"), - action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), - url_link=None, - url_label=None, - )], + [ + SuggestedAction( + internal_action_code=InternalActionCode("defaultcomms1"), + action_type=ActionType("CareCardWithText"), + action_code=ActionCode("BookLocal"), + action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), + url_link=None, + url_label=None, + ) + ], ), ( """Rule match: default_comms_routing present, @@ -1823,14 +1840,16 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcomms", "InternalBookNBS", {"defaultcomms": default_comms_detail}, - [SuggestedAction( - internal_action_code=InternalActionCode("defaultcomms"), - action_type=ActionType("CareCardWithText"), - action_code=ActionCode("BookLocal"), - action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), - url_link=None, - url_label=None, - )], + [ + SuggestedAction( + internal_action_code=InternalActionCode("defaultcomms"), + action_type=ActionType("CareCardWithText"), + action_code=ActionCode("BookLocal"), + action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), + url_link=None, + url_label=None, + ) + ], ), ( """Rule match: default_comms_routing present, @@ -1838,14 +1857,16 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcomms", "InvalidCode", {"defaultcomms": default_comms_detail}, - [SuggestedAction( - internal_action_code=InternalActionCode("defaultcomms"), - action_type=ActionType("CareCardWithText"), - action_code=ActionCode("BookLocal"), - action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), - url_link=None, - url_label=None, - )], + [ + SuggestedAction( + internal_action_code=InternalActionCode("defaultcomms"), + action_type=ActionType("CareCardWithText"), + action_code=ActionCode("BookLocal"), + action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), + url_link=None, + url_label=None, + ) + ], ), ( """Rule match: action_mapper present without url, @@ -1884,14 +1905,16 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "", "InternalBookNBS", {"InternalBookNBS": book_nbs_comms}, - [SuggestedAction( - internal_action_code=InternalActionCode("InternalBookNBS"), - action_type=ActionType("ButtonAuthLink"), - action_code=ActionCode("BookNBS"), - action_description=ActionDescription("Action description"), - url_link=UrlLink(HttpUrl("https://www.nhs.uk/book-rsv")), - url_label=UrlLabel("Continue to booking"), - )], + [ + SuggestedAction( + internal_action_code=InternalActionCode("InternalBookNBS"), + action_type=ActionType("ButtonAuthLink"), + action_code=ActionCode("BookNBS"), + action_description=ActionDescription("Action description"), + url_link=UrlLink(HttpUrl("https://www.nhs.uk/book-rsv")), + url_label=UrlLabel("Continue to booking"), + ) + ], ), ( """Rule match: default_comms_routing present, @@ -1907,14 +1930,16 @@ def test_cohort_group_descriptions_pick_first_non_empty_if_available( "defaultcomms1|invaliddefault", None, {"defaultcomms1": default_comms_detail}, - [SuggestedAction( - internal_action_code=InternalActionCode("defaultcomms1"), - action_type=ActionType("CareCardWithText"), - action_code=ActionCode("BookLocal"), - action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), - url_link=None, - url_label=None, - )], + [ + SuggestedAction( + internal_action_code=InternalActionCode("defaultcomms1"), + action_type=ActionType("CareCardWithText"), + action_code=ActionCode("BookLocal"), + action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), + url_link=None, + url_label=None, + ) + ], ), ], ) @@ -2017,14 +2042,20 @@ 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([SuggestedAction( - internal_action_code=InternalActionCode("ActionCode1"), - action_type=ActionType("ButtonAuthLink"), - action_code=ActionCode("BookNBS"), - action_description=ActionDescription("Action description"), - url_link=UrlLink(HttpUrl("https://www.nhs.uk/book-rsv")), - url_label=UrlLabel("Continue to booking"), - )])) + .and_actions( + equal_to( + [ + SuggestedAction( + internal_action_code=InternalActionCode("ActionCode1"), + action_type=ActionType("ButtonAuthLink"), + action_code=ActionCode("BookNBS"), + action_description=ActionDescription("Action description"), + url_link=UrlLink(HttpUrl("https://www.nhs.uk/book-rsv")), + url_label=UrlLabel("Continue to booking"), + ) + ] + ) + ) ) ), test_comment, @@ -2080,14 +2111,20 @@ 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([SuggestedAction( - internal_action_code=InternalActionCode("rule_1_comms_routing"), - action_type=ActionType("ButtonAuthLink"), - action_code=ActionCode("BookNBS"), - action_description=ActionDescription("Action description"), - url_link=UrlLink(HttpUrl("https://www.nhs.uk/book-rsv")), - url_label=UrlLabel("Continue to booking"), - )])) + .and_actions( + equal_to( + [ + SuggestedAction( + internal_action_code=InternalActionCode("rule_1_comms_routing"), + action_type=ActionType("ButtonAuthLink"), + action_code=ActionCode("BookNBS"), + action_description=ActionDescription("Action description"), + url_link=UrlLink(HttpUrl("https://www.nhs.uk/book-rsv")), + url_label=UrlLabel("Continue to booking"), + ) + ] + ) + ) ) ), ) @@ -2141,14 +2178,22 @@ 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([SuggestedAction( - internal_action_code=InternalActionCode("defaultcomms"), - action_type=ActionType("CareCardWithText"), - action_code=ActionCode("BookLocal"), - action_description=ActionDescription("You can get an RSV vaccination at your GP surgery"), - url_link=None, - url_label=None, - )])) + .and_actions( + equal_to( + [ + SuggestedAction( + internal_action_code=InternalActionCode("defaultcomms"), + action_type=ActionType("CareCardWithText"), + action_code=ActionCode("BookLocal"), + action_description=ActionDescription( + "You can get an RSV vaccination at your GP surgery" + ), + url_link=None, + url_label=None, + ) + ] + ) + ) ) ), ) @@ -2263,14 +2308,20 @@ 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([SuggestedAction( - internal_action_code=InternalActionCode("book_nbs"), - action_type=ActionType("ButtonAuthLink"), - action_code=ActionCode("BookNBS"), - action_description=ActionDescription("Action description"), - url_link=UrlLink(HttpUrl("https://www.nhs.uk/book-rsv")), - url_label=UrlLabel("Continue to booking"), - )])) + .and_actions( + equal_to( + [ + SuggestedAction( + internal_action_code=InternalActionCode("book_nbs"), + action_type=ActionType("ButtonAuthLink"), + action_code=ActionCode("BookNBS"), + action_description=ActionDescription("Action description"), + url_link=UrlLink(HttpUrl("https://www.nhs.uk/book-rsv")), + url_label=UrlLabel("Continue to booking"), + ) + ] + ) + ) ) ), ) diff --git a/tests/unit/test_audit_context.py b/tests/unit/test_audit_context.py index a141f0f8d..32f888c9c 100644 --- a/tests/unit/test_audit_context.py +++ b/tests/unit/test_audit_context.py @@ -8,13 +8,14 @@ from pydantic import HttpUrl from eligibility_signposting_api.audit_context import AuditContext -from eligibility_signposting_api.audit_models import AuditEvent, AuditAction +from eligibility_signposting_api.audit_models import AuditAction, AuditEvent from eligibility_signposting_api.model.eligibility import ( ActionCode, ActionDescription, ActionType, CohortGroupResult, ConditionName, + InternalActionCode, IterationResult, Reason, RuleDescription, @@ -23,7 +24,7 @@ Status, SuggestedAction, UrlLabel, - UrlLink, InternalActionCode, + UrlLink, ) from eligibility_signposting_api.model.rules import CampaignID, CampaignVersion, Iteration, RuleType from eligibility_signposting_api.services.audit_service import AuditService @@ -130,14 +131,16 @@ def test_append_audit_condition_adds_condition_to_audit_log_on_g(app): suggested_actions, condition_name, best_results, campaign_details, redirect_rule_details ) - expected_audit_action = [AuditAction( - internal_action_code="InternalActionCode1", - action_code="ActionCode1", - action_type="ActionType1", - action_description="ActionDescription1", - action_url="https://example.com/", - action_url_label="ActionLabel1", - )] + expected_audit_action = [ + AuditAction( + internal_action_code="InternalActionCode1", + action_code="ActionCode1", + action_type="ActionType1", + action_description="ActionDescription1", + action_url="https://example.com/", + action_url_label="ActionLabel1", + ) + ] assert g.audit_log.response.condition, condition_name cond = g.audit_log.response.condition[0] @@ -149,15 +152,15 @@ def test_append_audit_condition_adds_condition_to_audit_log_on_g(app): assert cond.status == best_results[1].status.name assert cond.status_text == best_results[1].status.name assert cond.actions == expected_audit_action - assert cond.action_rule.rule_priority == 1 + assert cond.action_rule.rule_priority == "1" assert cond.action_rule.rule_name == "RedirectRuleName1" assert cond.suitability_rules is None assert cond.filter_rules is None - assert cond.eligibility_cohorts[0].cohort_code is "CohortCode1" - assert cond.eligibility_cohorts[0].cohort_status is "actionable" - assert cond.eligibility_cohort_groups[0].cohort_code is "CohortCode1" - assert cond.eligibility_cohort_groups[0].cohort_status is "actionable" - assert cond.eligibility_cohort_groups[0].cohort_text is "CohortDescription1" + assert cond.eligibility_cohorts[0].cohort_code == "CohortCode1" + assert cond.eligibility_cohorts[0].cohort_status == "actionable" + assert cond.eligibility_cohort_groups[0].cohort_code == "CohortCode1" + assert cond.eligibility_cohort_groups[0].cohort_status == "actionable" + assert cond.eligibility_cohort_groups[0].cohort_text == "CohortDescription1" def test_add_response_details_adds_to_audit_log_on_g(app): From 00cac6c109bdb264f7e22eddb53abc2c70a63558 Mon Sep 17 00:00:00 2001 From: Shweta <216860557+shweta-nhs@users.noreply.github.com> Date: Fri, 4 Jul 2025 15:46:53 +0100 Subject: [PATCH 14/21] Fix linting again --- src/eligibility_signposting_api/audit_context.py | 6 +++--- src/eligibility_signposting_api/audit_models.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/eligibility_signposting_api/audit_context.py b/src/eligibility_signposting_api/audit_context.py index 5ba5fc346..088d10662 100644 --- a/src/eligibility_signposting_api/audit_context.py +++ b/src/eligibility_signposting_api/audit_context.py @@ -89,18 +89,18 @@ def append_audit_condition( if value.audit_rules and best_candidate: # Check best_candidate here as well if best_candidate.status and best_candidate.status.name == Status.not_eligible.name: audit_filter_rule = AuditFilterRule( - rule_priority=int(value.audit_rules[0].rule_priority), + rule_priority=value.audit_rules[0].rule_priority, rule_name=value.audit_rules[0].rule_name, ) if best_candidate.status and best_candidate.status.name == Status.not_actionable.name: audit_suitability_rule = AuditSuitabilityRule( - rule_priority=int(value.audit_rules[0].rule_priority), + rule_priority=value.audit_rules[0].rule_priority, rule_name=value.audit_rules[0].rule_name, rule_message=value.audit_rules[0].rule_description, ) if best_candidate and best_candidate.status and best_candidate.status.name == Status.actionable.name: - audit_redirect_rule = AuditRedirectRule(redirect_rule_details[0], redirect_rule_details[1]) + audit_redirect_rule = AuditRedirectRule(str(redirect_rule_details[0]), redirect_rule_details[1]) if suggested_actions is None: audit_actions = None diff --git a/src/eligibility_signposting_api/audit_models.py b/src/eligibility_signposting_api/audit_models.py index 51b759f4f..39a03cce6 100644 --- a/src/eligibility_signposting_api/audit_models.py +++ b/src/eligibility_signposting_api/audit_models.py @@ -41,7 +41,7 @@ class AuditEligibilityCohortGroups: @dataclass class AuditFilterRule: - rule_priority: int | None = None + rule_priority: str | None = None rule_name: str | None = None From 6effce2a17ff7941e7666e79fc39ca7147591a15 Mon Sep 17 00:00:00 2001 From: Shweta <216860557+shweta-nhs@users.noreply.github.com> Date: Fri, 4 Jul 2025 16:01:57 +0100 Subject: [PATCH 15/21] Fixing priority type --- tests/integration/lambda/test_app_running_as_lambda.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/integration/lambda/test_app_running_as_lambda.py b/tests/integration/lambda/test_app_running_as_lambda.py index 707d1e85b..51aa5e6e0 100644 --- a/tests/integration/lambda/test_app_running_as_lambda.py +++ b/tests/integration/lambda/test_app_running_as_lambda.py @@ -231,7 +231,7 @@ def test_given_nhs_number_in_path_matches_with_nhs_number_in_headers_and_check_i ], "filter_rules": None, "suitability_rules": { - "rule_priority": 10, + "rule_priority": "10", "rule_name": "Exclude too young less than 75", "rule_message": "Exclude too young less than 75", }, @@ -331,7 +331,7 @@ def test_given_person_has_unique_status_for_different_conditions_with_audit( # "cohort_status": "not_eligible", } ], - "filter_rules": {"rule_priority": 10, "rule_name": "Exclude too young less than 75"}, + "filter_rules": {"rule_priority": "10", "rule_name": "Exclude too young less than 75"}, "suitability_rules": None, "action_rule": None, "actions": [], @@ -354,7 +354,7 @@ def test_given_person_has_unique_status_for_different_conditions_with_audit( # ], "filter_rules": None, "suitability_rules": { - "rule_priority": 10, + "rule_priority": "10", "rule_name": "Exclude too young less than 75", "rule_message": "Exclude too young less than 75", }, @@ -379,7 +379,7 @@ def test_given_person_has_unique_status_for_different_conditions_with_audit( # ], "filter_rules": None, "suitability_rules": None, - "action_rule": {"rule_priority": 20, "rule_name": "In QE1"}, + "action_rule": {"rule_priority": "20", "rule_name": "In QE1"}, "actions": [ { "internal_action_code": "defaultcomms", From 7916d345a048e3868c3e20ecc8884d44f51a6a0f Mon Sep 17 00:00:00 2001 From: Shweta <216860557+shweta-nhs@users.noreply.github.com> Date: Mon, 7 Jul 2025 10:51:22 +0100 Subject: [PATCH 16/21] Cleaning audit bucket after use per every test run --- tests/integration/conftest.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 2054579b7..c5363c103 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -208,6 +208,25 @@ def flask_function(lambda_client: BaseClient, iam_role: str, lambda_zip: Path) - lambda_client.delete_function(FunctionName=function_name) +@pytest.fixture(autouse=True) +def clean_audit_bucket(s3_client: BaseClient, audit_bucket: str): + objects_to_delete = [] + paginator = s3_client.get_paginator('list_objects_v2') + pages = paginator.paginate(Bucket=audit_bucket) + for page in pages: + if "Contents" in page: + for obj in page["Contents"]: + objects_to_delete.append({"Key": obj["Key"]}) + + if objects_to_delete: + s3_client.delete_objects( + Bucket=audit_bucket, + Delete={"Objects": objects_to_delete, "Quiet": True}, + ) + + yield + + @pytest.fixture(scope="session") def flask_function_url(lambda_client: BaseClient, flask_function: str) -> URL: response = lambda_client.create_function_url_config(FunctionName=flask_function, AuthType="NONE") From cc1b58d3c1fe04e5795765d59f62304d7e1ed905 Mon Sep 17 00:00:00 2001 From: Shweta <216860557+shweta-nhs@users.noreply.github.com> Date: Mon, 7 Jul 2025 10:55:47 +0100 Subject: [PATCH 17/21] Fix lint --- tests/integration/conftest.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index c5363c103..de5fcaba5 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -211,7 +211,7 @@ def flask_function(lambda_client: BaseClient, iam_role: str, lambda_zip: Path) - @pytest.fixture(autouse=True) def clean_audit_bucket(s3_client: BaseClient, audit_bucket: str): objects_to_delete = [] - paginator = s3_client.get_paginator('list_objects_v2') + paginator = s3_client.get_paginator("list_objects_v2") pages = paginator.paginate(Bucket=audit_bucket) for page in pages: if "Contents" in page: @@ -224,7 +224,6 @@ def clean_audit_bucket(s3_client: BaseClient, audit_bucket: str): Delete={"Objects": objects_to_delete, "Quiet": True}, ) - yield @pytest.fixture(scope="session") From 2e27d5d2ff5ec75f03d9509dd7be749055f87164 Mon Sep 17 00:00:00 2001 From: Shweta <216860557+shweta-nhs@users.noreply.github.com> Date: Mon, 7 Jul 2025 11:01:14 +0100 Subject: [PATCH 18/21] Fix format and lint --- tests/integration/conftest.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index de5fcaba5..44ffeda08 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -215,8 +215,7 @@ def clean_audit_bucket(s3_client: BaseClient, audit_bucket: str): pages = paginator.paginate(Bucket=audit_bucket) for page in pages: if "Contents" in page: - for obj in page["Contents"]: - objects_to_delete.append({"Key": obj["Key"]}) + objects_to_delete.extend([{"Key": obj["Key"]} for obj in page["Contents"]]) if objects_to_delete: s3_client.delete_objects( @@ -225,7 +224,6 @@ def clean_audit_bucket(s3_client: BaseClient, audit_bucket: str): ) - @pytest.fixture(scope="session") def flask_function_url(lambda_client: BaseClient, flask_function: str) -> URL: response = lambda_client.create_function_url_config(FunctionName=flask_function, AuthType="NONE") From 5de4c4afd8585799f710c4d25d2f9bf77d3e6fcd Mon Sep 17 00:00:00 2001 From: Shweta <216860557+shweta-nhs@users.noreply.github.com> Date: Mon, 7 Jul 2025 11:28:42 +0100 Subject: [PATCH 19/21] Convert audit_log into camelCase when writing record to firehose --- .../audit_context.py | 7 +- .../audit_models.py | 61 +++--- .../lambda/test_app_running_as_lambda.py | 180 +++++++++--------- tests/unit/test_audit_context.py | 3 +- 4 files changed, 124 insertions(+), 127 deletions(-) diff --git a/src/eligibility_signposting_api/audit_context.py b/src/eligibility_signposting_api/audit_context.py index 088d10662..921f2d7b4 100644 --- a/src/eligibility_signposting_api/audit_context.py +++ b/src/eligibility_signposting_api/audit_context.py @@ -1,5 +1,4 @@ import logging -from dataclasses import asdict from datetime import UTC, datetime from operator import attrgetter from uuid import UUID @@ -100,7 +99,9 @@ def append_audit_condition( ) if best_candidate and best_candidate.status and best_candidate.status.name == Status.actionable.name: - audit_redirect_rule = AuditRedirectRule(str(redirect_rule_details[0]), redirect_rule_details[1]) + audit_redirect_rule = AuditRedirectRule( + rule_priority=str(redirect_rule_details[0]), rule_name=redirect_rule_details[1] + ) if suggested_actions is None: audit_actions = None @@ -142,4 +143,4 @@ def add_response_details(response_id: UUID, last_updated: datetime) -> None: @staticmethod def write_to_firehose(service: AuditService) -> None: - service.audit(asdict(g.audit_log)) + service.audit(g.audit_log.model_dump(by_alias=True)) diff --git a/src/eligibility_signposting_api/audit_models.py b/src/eligibility_signposting_api/audit_models.py index 39a03cce6..17467130f 100644 --- a/src/eligibility_signposting_api/audit_models.py +++ b/src/eligibility_signposting_api/audit_models.py @@ -1,65 +1,65 @@ -from dataclasses import dataclass, field from datetime import UTC, datetime from uuid import UUID +from pydantic import BaseModel, ConfigDict, Field +from pydantic.alias_generators import to_camel -@dataclass -class RequestAuditHeader: + +class CamelCaseBaseModel(BaseModel): + model_config = ConfigDict( + alias_generator=to_camel, + populate_by_name=True, + ) + + +class RequestAuditHeader(CamelCaseBaseModel): x_request_id: str | None = None x_correlation_id: str | None = None nhsd_end_user_organisation_ods: str | None = None nhsd_application_id: str | None = None -@dataclass -class RequestAuditQueryParams: +class RequestAuditQueryParams(CamelCaseBaseModel): category: str | None = None conditions: str | None = None include_actions: str | None = None -@dataclass -class RequestAuditData: - request_timestamp: datetime = field(default_factory=lambda: datetime.now(UTC)) - headers: RequestAuditHeader = field(default_factory=RequestAuditHeader) - query_params: RequestAuditQueryParams = field(default_factory=RequestAuditQueryParams) +class RequestAuditData(CamelCaseBaseModel): + request_timestamp: datetime = Field(default_factory=lambda: datetime.now(UTC)) + headers: RequestAuditHeader = Field(default_factory=RequestAuditHeader) + query_params: RequestAuditQueryParams = Field(default_factory=RequestAuditQueryParams) nhs_number: str | None = None -@dataclass -class AuditEligibilityCohorts: +class AuditEligibilityCohorts(CamelCaseBaseModel): cohort_code: str | None = None cohort_status: str | None = None -@dataclass -class AuditEligibilityCohortGroups: +class AuditEligibilityCohortGroups(CamelCaseBaseModel): cohort_code: str | None = None cohort_text: str | None = None cohort_status: str | None = None -@dataclass -class AuditFilterRule: +class AuditFilterRule(CamelCaseBaseModel): rule_priority: str | None = None rule_name: str | None = None -@dataclass -class AuditSuitabilityRule: +class AuditSuitabilityRule(CamelCaseBaseModel): rule_priority: str | None = None rule_name: str | None = None rule_message: str | None = None -@dataclass -class AuditRedirectRule: +class AuditRedirectRule(CamelCaseBaseModel): rule_priority: str | None = None rule_name: str | None = None -@dataclass -class AuditAction: +class AuditAction(CamelCaseBaseModel): internal_action_code: str | None = None action_type: str | None = None action_code: str | None = None @@ -68,8 +68,7 @@ class AuditAction: action_url_label: str | None = None -@dataclass -class AuditCondition: +class AuditCondition(CamelCaseBaseModel): campaign_id: str | None = None campaign_version: str | None = None iteration_id: str | None = None @@ -82,17 +81,15 @@ class AuditCondition: filter_rules: AuditFilterRule | None = None suitability_rules: AuditSuitabilityRule | None = None action_rule: AuditRedirectRule | None = None - actions: list[AuditAction] | None = field(default_factory=list) + actions: list[AuditAction] | None = Field(default_factory=list) -@dataclass -class ResponseAuditData: +class ResponseAuditData(CamelCaseBaseModel): response_id: UUID | None = None last_updated: str | None = None - condition: list[AuditCondition] = field(default_factory=list) + condition: list[AuditCondition] = Field(default_factory=list) -@dataclass -class AuditEvent: - request: RequestAuditData = field(default_factory=RequestAuditData) - response: ResponseAuditData = field(default_factory=ResponseAuditData) +class AuditEvent(CamelCaseBaseModel): + request: RequestAuditData = Field(default_factory=RequestAuditData) + response: ResponseAuditData = Field(default_factory=ResponseAuditData) diff --git a/tests/integration/lambda/test_app_running_as_lambda.py b/tests/integration/lambda/test_app_running_as_lambda.py index 51aa5e6e0..35dcb4353 100644 --- a/tests/integration/lambda/test_app_running_as_lambda.py +++ b/tests/integration/lambda/test_app_running_as_lambda.py @@ -200,48 +200,48 @@ def test_given_nhs_number_in_path_matches_with_nhs_number_in_headers_and_check_i audit_data = json.loads(s3_client.get_object(Bucket=audit_bucket, Key=latest_key)["Body"].read()) expected_headers = { - "x_request_id": "x_request_id", - "x_correlation_id": "x_correlation_id", - "nhsd_end_user_organisation_ods": "nhsd_end_user_organisation_ods", - "nhsd_application_id": "nhsd_application_id", + "xRequestId": "x_request_id", + "xCorrelationId": "x_correlation_id", + "nhsdEndUserOrganisationOds": "nhsd_end_user_organisation_ods", + "nhsdApplicationId": "nhsd_application_id", } - expected_query_params = {"category": None, "conditions": None, "include_actions": "Y"} - - assert_that(audit_data["request"]["request_timestamp"], is_not(equal_to(""))) - assert_that(audit_data["request"]["headers"], equal_to(expected_headers)) - assert_that(audit_data["request"]["nhs_number"], equal_to(persisted_person)) - assert_that(audit_data["request"]["query_params"], equal_to(expected_query_params)) + expected_query_params = {"category": None, "conditions": None, "includeActions": "Y"} expected_conditions = [ { - "campaign_id": campaign_config.id, - "campaign_version": campaign_config.version, - "iteration_id": campaign_config.iterations[0].id, - "iteration_version": campaign_config.iterations[0].version, - "condition_name": campaign_config.target, + "campaignId": campaign_config.id, + "campaignVersion": campaign_config.version, + "iterationId": campaign_config.iterations[0].id, + "iterationVersion": campaign_config.iterations[0].version, + "conditionName": campaign_config.target, "status": "not_actionable", - "status_text": "not_actionable", - "eligibility_cohorts": [{"cohort_code": "cohort_group1", "cohort_status": "not_actionable"}], - "eligibility_cohort_groups": [ + "statusText": "not_actionable", + "eligibilityCohorts": [{"cohortCode": "cohort_group1", "cohortStatus": "not_actionable"}], + "eligibilityCohortGroups": [ { - "cohort_code": "cohort_group1", - "cohort_text": "positive_description", - "cohort_status": "not_actionable", + "cohortCode": "cohort_group1", + "cohortText": "positive_description", + "cohortStatus": "not_actionable", } ], - "filter_rules": None, - "suitability_rules": { - "rule_priority": "10", - "rule_name": "Exclude too young less than 75", - "rule_message": "Exclude too young less than 75", + "filterRules": None, + "suitabilityRules": { + "rulePriority": "10", + "ruleName": "Exclude too young less than 75", + "ruleMessage": "Exclude too young less than 75", }, - "action_rule": None, + "actionRule": None, "actions": [], } ] - assert_that(audit_data["response"]["response_id"], is_not(equal_to(""))) - assert_that(audit_data["response"]["last_updated"], is_not(equal_to(""))) + assert_that(audit_data["request"]["requestTimestamp"], is_not(equal_to(""))) + assert_that(audit_data["request"]["headers"], equal_to(expected_headers)) + assert_that(audit_data["request"]["nhsNumber"], equal_to(persisted_person)) + assert_that(audit_data["request"]["queryParams"], equal_to(expected_query_params)) + + assert_that(audit_data["response"]["responseId"], is_not(equal_to(""))) + assert_that(audit_data["response"]["lastUpdated"], is_not(equal_to(""))) assert_that(audit_data["response"]["condition"], equal_to(expected_conditions)) @@ -303,12 +303,12 @@ def test_given_person_has_unique_status_for_different_conditions_with_audit( # audit_data = json.loads(s3_client.get_object(Bucket=audit_bucket, Key=latest_key)["Body"].read()) expected_headers = { - "x_request_id": "x_request_id", - "x_correlation_id": "x_correlation_id", - "nhsd_end_user_organisation_ods": "nhsd_end_user_organisation_ods", - "nhsd_application_id": "nhsd_application_id", + "xRequestId": "x_request_id", + "xCorrelationId": "x_correlation_id", + "nhsdEndUserOrganisationOds": "nhsd_end_user_organisation_ods", + "nhsdApplicationId": "nhsd_application_id", } - expected_query_params = {"category": None, "conditions": None, "include_actions": "Y"} + expected_query_params = {"category": None, "conditions": None, "includeActions": "Y"} rsv_campaign = multiple_campaign_configs[0] covid_campaign = multiple_campaign_configs[1] @@ -316,87 +316,87 @@ def test_given_person_has_unique_status_for_different_conditions_with_audit( # expected_conditions = [ { - "campaign_id": rsv_campaign.id, - "campaign_version": rsv_campaign.version, - "iteration_id": rsv_campaign.iterations[0].id, - "iteration_version": rsv_campaign.iterations[0].version, - "condition_name": rsv_campaign.target, + "campaignId": rsv_campaign.id, + "campaignVersion": rsv_campaign.version, + "iterationId": rsv_campaign.iterations[0].id, + "iterationVersion": rsv_campaign.iterations[0].version, + "conditionName": rsv_campaign.target, "status": "not_eligible", - "status_text": "not_eligible", - "eligibility_cohorts": [{"cohort_code": "cohort_group1", "cohort_status": "not_eligible"}], - "eligibility_cohort_groups": [ + "statusText": "not_eligible", + "eligibilityCohorts": [{"cohortCode": "cohort_group1", "cohortStatus": "not_eligible"}], + "eligibilityCohortGroups": [ { - "cohort_code": "cohort_group1", - "cohort_text": "negative_desc_1", - "cohort_status": "not_eligible", + "cohortCode": "cohort_group1", + "cohortText": "negative_desc_1", + "cohortStatus": "not_eligible", } ], - "filter_rules": {"rule_priority": "10", "rule_name": "Exclude too young less than 75"}, - "suitability_rules": None, - "action_rule": None, + "filterRules": {"rulePriority": "10", "ruleName": "Exclude too young less than 75"}, + "suitabilityRules": None, + "actionRule": None, "actions": [], }, { - "campaign_id": covid_campaign.id, - "campaign_version": covid_campaign.version, - "iteration_id": covid_campaign.iterations[0].id, - "iteration_version": covid_campaign.iterations[0].version, - "condition_name": covid_campaign.target, + "campaignId": covid_campaign.id, + "campaignVersion": covid_campaign.version, + "iterationId": covid_campaign.iterations[0].id, + "iterationVersion": covid_campaign.iterations[0].version, + "conditionName": covid_campaign.target, "status": "not_actionable", - "status_text": "not_actionable", - "eligibility_cohorts": [{"cohort_code": "cohort_group2", "cohort_status": "not_actionable"}], - "eligibility_cohort_groups": [ + "statusText": "not_actionable", + "eligibilityCohorts": [{"cohortCode": "cohort_group2", "cohortStatus": "not_actionable"}], + "eligibilityCohortGroups": [ { - "cohort_code": "cohort_group2", - "cohort_text": "positive_desc_2", - "cohort_status": "not_actionable", + "cohortCode": "cohort_group2", + "cohortText": "positive_desc_2", + "cohortStatus": "not_actionable", } ], - "filter_rules": None, - "suitability_rules": { - "rule_priority": "10", - "rule_name": "Exclude too young less than 75", - "rule_message": "Exclude too young less than 75", + "filterRules": None, + "suitabilityRules": { + "rulePriority": "10", + "ruleName": "Exclude too young less than 75", + "ruleMessage": "Exclude too young less than 75", }, - "action_rule": None, + "actionRule": None, "actions": [], }, { - "campaign_id": flu_campaign.id, - "campaign_version": flu_campaign.version, - "iteration_id": flu_campaign.iterations[0].id, - "iteration_version": flu_campaign.iterations[0].version, - "condition_name": flu_campaign.target, + "campaignId": flu_campaign.id, + "campaignVersion": flu_campaign.version, + "iterationId": flu_campaign.iterations[0].id, + "iterationVersion": flu_campaign.iterations[0].version, + "conditionName": flu_campaign.target, "status": "actionable", - "status_text": "actionable", - "eligibility_cohorts": [{"cohort_code": "cohort_group3", "cohort_status": "actionable"}], - "eligibility_cohort_groups": [ + "statusText": "actionable", + "eligibilityCohorts": [{"cohortCode": "cohort_group3", "cohortStatus": "actionable"}], + "eligibilityCohortGroups": [ { - "cohort_code": "cohort_group3", - "cohort_text": "positive_desc_3", - "cohort_status": "actionable", + "cohortCode": "cohort_group3", + "cohortText": "positive_desc_3", + "cohortStatus": "actionable", } ], - "filter_rules": None, - "suitability_rules": None, - "action_rule": {"rule_priority": "20", "rule_name": "In QE1"}, + "filterRules": None, + "suitabilityRules": None, + "actionRule": {"rulePriority": "20", "ruleName": "In QE1"}, "actions": [ { - "internal_action_code": "defaultcomms", - "action_type": "defaultcomms", - "action_code": "action_code", - "action_description": None, - "action_url": None, - "action_url_label": None, + "internalActionCode": "defaultcomms", + "actionType": "defaultcomms", + "actionCode": "action_code", + "actionDescription": None, + "actionUrl": None, + "actionUrlLabel": None, } ], }, ] - assert_that(audit_data["request"]["request_timestamp"], is_not(equal_to(""))) + assert_that(audit_data["request"]["requestTimestamp"], is_not(equal_to(""))) assert_that(audit_data["request"]["headers"], equal_to(expected_headers)) - assert_that(audit_data["request"]["nhs_number"], equal_to(persisted_person_all_cohorts)) - assert_that(audit_data["request"]["query_params"], equal_to(expected_query_params)) - assert_that(audit_data["response"]["response_id"], is_not(equal_to(""))) - assert_that(audit_data["response"]["last_updated"], is_not(equal_to(""))) + assert_that(audit_data["request"]["nhsNumber"], equal_to(persisted_person_all_cohorts)) + assert_that(audit_data["request"]["queryParams"], equal_to(expected_query_params)) + assert_that(audit_data["response"]["responseId"], is_not(equal_to(""))) + assert_that(audit_data["response"]["lastUpdated"], is_not(equal_to(""))) assert_that(audit_data["response"]["condition"], contains_inanyorder(*expected_conditions)) diff --git a/tests/unit/test_audit_context.py b/tests/unit/test_audit_context.py index 32f888c9c..6827cc2a6 100644 --- a/tests/unit/test_audit_context.py +++ b/tests/unit/test_audit_context.py @@ -1,5 +1,4 @@ import uuid -from dataclasses import asdict from datetime import UTC, datetime from unittest.mock import Mock @@ -190,4 +189,4 @@ def test_write_to_firehose_calls_audit_service_with_correct_data_from_g(app): assert g.audit_log.response.response_id == response_id assert g.audit_log.response.last_updated == last_updated - mock_audit_service.audit.assert_called_once_with(asdict(g.audit_log)) + mock_audit_service.audit.assert_called_once_with(g.audit_log.model_dump(by_alias=True)) From 46bf9d12eeb391b6b618f2a2aa52004a23872934 Mon Sep 17 00:00:00 2001 From: Karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Mon, 7 Jul 2025 10:59:50 +0100 Subject: [PATCH 20/21] github role permissions - reduced (#217) --- .../iams-developer-roles/github_actions_policies.tf | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/infrastructure/stacks/iams-developer-roles/github_actions_policies.tf b/infrastructure/stacks/iams-developer-roles/github_actions_policies.tf index 2cf94cf87..78e909d86 100644 --- a/infrastructure/stacks/iams-developer-roles/github_actions_policies.tf +++ b/infrastructure/stacks/iams-developer-roles/github_actions_policies.tf @@ -13,7 +13,8 @@ resource "aws_iam_policy" "terraform_state" { "s3:ListBucket", "s3:GetObject", "s3:PutObject", - "s3:DeleteObject" + "s3:DeleteObject", + "s3:GetObject" ], Resource = [ "${local.terraform_state_bucket_arn}", @@ -147,6 +148,7 @@ resource "aws_iam_policy" "s3_management" { "s3:PutBucketLogging", "s3:GetObjectTagging", "s3:PutObjectTagging", + "s3:GetObjectVersion" ], Resource = [ "arn:aws:s3:::*eligibility-signposting-api-${var.environment}-eli-rules", @@ -296,9 +298,13 @@ resource "aws_iam_policy" "kms_creation" { Effect = "Allow", Action = [ "kms:CreateKey", + "kms:DescribeKey", "kms:CreateAlias", "kms:List*", "kms:ListAliases", + "kms:Decrypt", + "kms:Encrypt", + "kms:ReEncrypt*", ], Resource = "*" }, From f8b92b003bf30f59fac0ed39c5c9170545d4b007 Mon Sep 17 00:00:00 2001 From: Shweta <216860557+shweta-nhs@users.noreply.github.com> Date: Mon, 7 Jul 2025 12:37:40 +0100 Subject: [PATCH 21/21] Adds audit package for separation of concerns --- src/eligibility_signposting_api/app.py | 6 ++++-- src/eligibility_signposting_api/audit/__init__.py | 0 .../{ => audit}/audit_context.py | 6 +++--- src/eligibility_signposting_api/{ => audit}/audit_models.py | 0 .../{services => audit}/audit_service.py | 0 .../services/calculators/eligibility_calculator.py | 2 +- src/eligibility_signposting_api/views/eligibility.py | 4 ++-- tests/conftest.py | 2 +- tests/integration/lambda/test_app_running_as_lambda.py | 3 --- tests/unit/audit/__init__.py | 0 tests/unit/{ => audit}/test_audit_context.py | 6 +++--- tests/unit/views/test_eligibility.py | 2 +- 12 files changed, 15 insertions(+), 16 deletions(-) create mode 100644 src/eligibility_signposting_api/audit/__init__.py rename src/eligibility_signposting_api/{ => audit}/audit_context.py (96%) rename src/eligibility_signposting_api/{ => audit}/audit_models.py (100%) rename src/eligibility_signposting_api/{services => audit}/audit_service.py (100%) create mode 100644 tests/unit/audit/__init__.py rename tests/unit/{ => audit}/test_audit_context.py (96%) diff --git a/src/eligibility_signposting_api/app.py b/src/eligibility_signposting_api/app.py index 2cf614bea..69b01e77e 100644 --- a/src/eligibility_signposting_api/app.py +++ b/src/eligibility_signposting_api/app.py @@ -7,7 +7,7 @@ from mangum import Mangum from mangum.types import LambdaContext, LambdaEvent -from eligibility_signposting_api import repos, services +from eligibility_signposting_api import audit, repos, services from eligibility_signposting_api.config.config import config, init_logging from eligibility_signposting_api.error_handler import handle_exception from eligibility_signposting_api.views import eligibility_blueprint @@ -41,7 +41,9 @@ def create_app() -> Flask: app.register_error_handler(Exception, handle_exception) # Set up dependency injection using wireup - container = wireup.create_sync_container(service_modules=[services, repos], parameters={**app.config, **config()}) + container = wireup.create_sync_container( + service_modules=[services, repos, audit], parameters={**app.config, **config()} + ) wireup.integration.flask.setup(container, app) logger.info("app ready", extra={"config": {**app.config, **config()}}) diff --git a/src/eligibility_signposting_api/audit/__init__.py b/src/eligibility_signposting_api/audit/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/src/eligibility_signposting_api/audit_context.py b/src/eligibility_signposting_api/audit/audit_context.py similarity index 96% rename from src/eligibility_signposting_api/audit_context.py rename to src/eligibility_signposting_api/audit/audit_context.py index 921f2d7b4..09094d95b 100644 --- a/src/eligibility_signposting_api/audit_context.py +++ b/src/eligibility_signposting_api/audit/audit_context.py @@ -5,7 +5,7 @@ from flask import Request, g -from eligibility_signposting_api.audit_models import ( +from eligibility_signposting_api.audit.audit_models import ( AuditAction, AuditCondition, AuditEligibilityCohortGroups, @@ -18,6 +18,7 @@ RequestAuditHeader, RequestAuditQueryParams, ) +from eligibility_signposting_api.audit.audit_service import AuditService from eligibility_signposting_api.model.eligibility import ( CohortGroupResult, ConditionName, @@ -26,7 +27,6 @@ SuggestedAction, ) from eligibility_signposting_api.model.rules import CampaignID, CampaignVersion, Iteration, RuleName, RulePriority -from eligibility_signposting_api.services.audit_service import AuditService logger = logging.getLogger(__name__) @@ -85,7 +85,7 @@ def append_audit_condition( ) ) - if value.audit_rules and best_candidate: # Check best_candidate here as well + if value.audit_rules and best_candidate: if best_candidate.status and best_candidate.status.name == Status.not_eligible.name: audit_filter_rule = AuditFilterRule( rule_priority=value.audit_rules[0].rule_priority, diff --git a/src/eligibility_signposting_api/audit_models.py b/src/eligibility_signposting_api/audit/audit_models.py similarity index 100% rename from src/eligibility_signposting_api/audit_models.py rename to src/eligibility_signposting_api/audit/audit_models.py diff --git a/src/eligibility_signposting_api/services/audit_service.py b/src/eligibility_signposting_api/audit/audit_service.py similarity index 100% rename from src/eligibility_signposting_api/services/audit_service.py rename to src/eligibility_signposting_api/audit/audit_service.py diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index 125d4a81b..bc060bbef 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -7,7 +7,7 @@ from itertools import groupby from typing import TYPE_CHECKING, Any -from eligibility_signposting_api.audit_context import AuditContext +from eligibility_signposting_api.audit.audit_context import AuditContext if TYPE_CHECKING: from eligibility_signposting_api.model.rules import ( diff --git a/src/eligibility_signposting_api/views/eligibility.py b/src/eligibility_signposting_api/views/eligibility.py index 256622d44..310c4e1eb 100644 --- a/src/eligibility_signposting_api/views/eligibility.py +++ b/src/eligibility_signposting_api/views/eligibility.py @@ -9,10 +9,10 @@ from flask.typing import ResponseReturnValue from wireup import Injected -from eligibility_signposting_api.audit_context import AuditContext +from eligibility_signposting_api.audit.audit_context import AuditContext +from eligibility_signposting_api.audit.audit_service import AuditService from eligibility_signposting_api.model.eligibility import Condition, EligibilityStatus, NHSNumber, Status from eligibility_signposting_api.services import EligibilityService, UnknownPersonError -from eligibility_signposting_api.services.audit_service import AuditService from eligibility_signposting_api.services.eligibility_services import InvalidQueryParamError from eligibility_signposting_api.views.response_model import eligibility from eligibility_signposting_api.views.response_model.eligibility import ProcessedSuggestion diff --git a/tests/conftest.py b/tests/conftest.py index 4c43c14c6..d3e52d7f2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -8,7 +8,7 @@ from flask.testing import FlaskClient from eligibility_signposting_api.app import create_app -from eligibility_signposting_api.audit_models import AuditEvent +from eligibility_signposting_api.audit.audit_models import AuditEvent @pytest.fixture(scope="session") diff --git a/tests/integration/lambda/test_app_running_as_lambda.py b/tests/integration/lambda/test_app_running_as_lambda.py index 35dcb4353..8253e9a1a 100644 --- a/tests/integration/lambda/test_app_running_as_lambda.py +++ b/tests/integration/lambda/test_app_running_as_lambda.py @@ -275,8 +275,6 @@ def test_given_person_has_unique_status_for_different_conditions_with_audit( # audit_bucket: BucketName, api_gateway_endpoint: URL, ): - # Given - # When invoke_url = f"{api_gateway_endpoint}/patient-check/{persisted_person_all_cohorts}" response = httpx.get( invoke_url, @@ -291,7 +289,6 @@ def test_given_person_has_unique_status_for_different_conditions_with_audit( # timeout=10, ) - # Then assert_that( response, is_response().with_status_code(HTTPStatus.OK).and_body(is_json_that(has_key("processedSuggestions"))), diff --git a/tests/unit/audit/__init__.py b/tests/unit/audit/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/unit/test_audit_context.py b/tests/unit/audit/test_audit_context.py similarity index 96% rename from tests/unit/test_audit_context.py rename to tests/unit/audit/test_audit_context.py index 6827cc2a6..2ee9aafdc 100644 --- a/tests/unit/test_audit_context.py +++ b/tests/unit/audit/test_audit_context.py @@ -6,8 +6,9 @@ from flask import Flask, g, request from pydantic import HttpUrl -from eligibility_signposting_api.audit_context import AuditContext -from eligibility_signposting_api.audit_models import AuditAction, AuditEvent +from eligibility_signposting_api.audit.audit_context import AuditContext +from eligibility_signposting_api.audit.audit_models import AuditAction, AuditEvent +from eligibility_signposting_api.audit.audit_service import AuditService from eligibility_signposting_api.model.eligibility import ( ActionCode, ActionDescription, @@ -26,7 +27,6 @@ UrlLink, ) from eligibility_signposting_api.model.rules import CampaignID, CampaignVersion, Iteration, RuleType -from eligibility_signposting_api.services.audit_service import AuditService from tests.fixtures.builders.model.rule import IterationFactory diff --git a/tests/unit/views/test_eligibility.py b/tests/unit/views/test_eligibility.py index 5b7616776..c0b4c3fa7 100644 --- a/tests/unit/views/test_eligibility.py +++ b/tests/unit/views/test_eligibility.py @@ -14,6 +14,7 @@ from pydantic import HttpUrl from wireup.integration.flask import get_app_container +from eligibility_signposting_api.audit.audit_service import AuditService from eligibility_signposting_api.model.eligibility import ( ActionCode, ActionDescription, @@ -33,7 +34,6 @@ UrlLink, ) from eligibility_signposting_api.services import EligibilityService, UnknownPersonError -from eligibility_signposting_api.services.audit_service import AuditService from eligibility_signposting_api.services.eligibility_services import InvalidQueryParamError from eligibility_signposting_api.views.eligibility import ( build_actions,