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 1/9] 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 2/9] 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 3/9] 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 4/9] 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 5/9] 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 6/9] 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 7/9] 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 8/9] 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 9/9] 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():