From 3932cf36d47b2ba5b76f59718c57223285ba2355 Mon Sep 17 00:00:00 2001 From: NogaNHS <127490765+NogaNHS@users.noreply.github.com> Date: Wed, 4 Mar 2026 16:14:05 +0000 Subject: [PATCH 01/17] [PRMP-1446] Implement search user restriction functionality with associated tests and error handling --- .../base-lambdas-reusable-deploy-all.yml | 16 + ..._review_accepted_querystring_parameters.py | 2 +- lambdas/enums/feature_flags.py | 1 + lambdas/enums/lambda_error.py | 21 ++ ...riction_accepted_querystring_parameters.py | 8 + .../handlers/user_restrictions/__init__.py | 0 .../search_user_restriction_handler.py | 97 +++++ .../user_restriction_response.py | 10 + .../user_restrictions/user_restrictions.py | 2 +- lambdas/services/authoriser_service.py | 2 + lambdas/services/base/dynamo_service.py | 7 +- .../search_user_restriction_service.py | 183 ++++++++++ .../user_restriction_dynamo_service.py | 99 ++++++ .../services/user_restrictions/utilites.py | 2 +- ...does_not_return_inactive_restrictions.json | 18 + ...ser_restriction_filters_by_nhs_number.json | 43 +++ ...r_restriction_filters_by_smartcard_id.json | 30 ++ ...ates_with_limit_and_next_page_token.1.json | 28 ++ ...inates_with_limit_and_next_page_token.json | 29 ++ ...riction_returns_200_with_restrictions.json | 53 +++ ..._restriction_returns_401_without_auth.json | 3 + ...on_returns_empty_list_when_no_matches.json | 4 + .../files/user_restrictions_seed_data.json | 70 ++++ .../api/test_search_user_restriction_api.py | 220 ++++++++++++ lambdas/tests/e2e/helpers/data_helper.py | 38 ++ lambdas/tests/unit/conftest.py | 2 + .../test_search_user_restriction_handler.py | 331 ++++++++++++++++++ .../test_search_user_restriction_service.py | 283 +++++++++++++++ .../test_user_restriction_dynamo_service.py | 130 +++++++ lambdas/utils/exceptions.py | 8 + lambdas/utils/lambda_exceptions.py | 4 + lambdas/utils/ods_utils.py | 4 +- lambdas/utils/pagination_utils.py | 19 + 33 files changed, 1759 insertions(+), 8 deletions(-) create mode 100644 lambdas/enums/user_restriction_accepted_querystring_parameters.py create mode 100644 lambdas/handlers/user_restrictions/__init__.py create mode 100644 lambdas/handlers/user_restrictions/search_user_restriction_handler.py create mode 100644 lambdas/models/user_restrictions/user_restriction_response.py create mode 100644 lambdas/services/user_restrictions/search_user_restriction_service.py create mode 100644 lambdas/services/user_restrictions/user_restriction_dynamo_service.py create mode 100644 lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_does_not_return_inactive_restrictions.json create mode 100644 lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_filters_by_nhs_number.json create mode 100644 lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_filters_by_smartcard_id.json create mode 100644 lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_paginates_with_limit_and_next_page_token.1.json create mode 100644 lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_paginates_with_limit_and_next_page_token.json create mode 100644 lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_returns_200_with_restrictions.json create mode 100644 lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_returns_401_without_auth.json create mode 100644 lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_returns_empty_list_when_no_matches.json create mode 100644 lambdas/tests/e2e/api/files/user_restrictions_seed_data.json create mode 100644 lambdas/tests/e2e/api/test_search_user_restriction_api.py create mode 100644 lambdas/tests/unit/handlers/test_search_user_restriction_handler.py create mode 100644 lambdas/tests/unit/services/user_restriction/test_search_user_restriction_service.py create mode 100644 lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py create mode 100644 lambdas/utils/pagination_utils.py diff --git a/.github/workflows/base-lambdas-reusable-deploy-all.yml b/.github/workflows/base-lambdas-reusable-deploy-all.yml index a8fe7960ce..2c17343884 100644 --- a/.github/workflows/base-lambdas-reusable-deploy-all.yml +++ b/.github/workflows/base-lambdas-reusable-deploy-all.yml @@ -880,3 +880,19 @@ jobs: lambda_layer_names: "core_lambda_layer" secrets: AWS_ASSUME_ROLE: ${{ secrets.AWS_ASSUME_ROLE }} + + deploy_search_user_restriction_lambda: + name: Deploy Search User Restriction Lambda + uses: ./.github/workflows/base-lambdas-reusable-deploy.yml + with: + environment: ${{ inputs.environment }} + python_version: ${{ inputs.python_version }} + build_branch: ${{ inputs.build_branch }} + sandbox: ${{ inputs.sandbox }} + lambda_handler_name: search_user_restriction_handler + lambda_handler_path: user_restrictions + lambda_aws_name: SearchUserRestriction + lambda_layer_names: "core_lambda_layer" + secrets: + AWS_ASSUME_ROLE: ${{ secrets.AWS_ASSUME_ROLE }} + diff --git a/lambdas/enums/document_review_accepted_querystring_parameters.py b/lambdas/enums/document_review_accepted_querystring_parameters.py index 95c59e8f26..ec0b7d16b4 100644 --- a/lambdas/enums/document_review_accepted_querystring_parameters.py +++ b/lambdas/enums/document_review_accepted_querystring_parameters.py @@ -3,6 +3,6 @@ class DocumentReviewQuerystringParameters(StrEnum): LIMIT = "limit" + NHS_NUMBER = "nhsNumber" NEXT_PAGE_TOKEN = "nextPageToken" UPLOADER = "uploader" - NHS_NUMBER = "nhsNumber" diff --git a/lambdas/enums/feature_flags.py b/lambdas/enums/feature_flags.py index e22ec70ece..9c87d362f3 100755 --- a/lambdas/enums/feature_flags.py +++ b/lambdas/enums/feature_flags.py @@ -12,3 +12,4 @@ class FeatureFlags(StrEnum): UPLOAD_DOCUMENT_ITERATION_2_ENABLED = "uploadDocumentIteration2Enabled" UPLOAD_DOCUMENT_ITERATION_3_ENABLED = "uploadDocumentIteration3Enabled" BULK_UPLOAD_SEND_TO_REVIEW_ENABLED = "bulkUploadSendToReviewEnabled" + USER_RESTRICTION_ENABLED = "userRestrictionEnabled" diff --git a/lambdas/enums/lambda_error.py b/lambdas/enums/lambda_error.py index 098e63050b..3fd5355eae 100644 --- a/lambdas/enums/lambda_error.py +++ b/lambdas/enums/lambda_error.py @@ -746,3 +746,24 @@ def create_error_body( "err_code": "RD_IA", "message": "Invalid action. Expected 'list' or 'process_one'.", } + + """ + Errors for SearchUserRestriction lambda + """ + SearchUserRestrictionMissingODS = { + "err_code": "SUR_4001", + "message": "No ODS code provided in request context", + } + SearchUserRestrictionInvalidQueryString = { + "err_code": "SUR_4002", + "message": "Invalid query string parameter", + } + SearchUserRestrictionDB = { + "err_code": "SUR_5001", + "message": "Failed to query user restrictions from DynamoDB", + } + + SearchUserRestrictionsInvalidQueryParameter = { + "err_code": "SUR_4003", + "message": "Invalid query parameter value: %(details)s", + } diff --git a/lambdas/enums/user_restriction_accepted_querystring_parameters.py b/lambdas/enums/user_restriction_accepted_querystring_parameters.py new file mode 100644 index 0000000000..06222d3f2b --- /dev/null +++ b/lambdas/enums/user_restriction_accepted_querystring_parameters.py @@ -0,0 +1,8 @@ +from enum import StrEnum + + +class UserRestrictionQuerystringParameters(StrEnum): + LIMIT = "limit" + NHS_NUMBER = "nhsNumber" + SMARTCARD_ID = "smartcardId" + NEXT_PAGE_TOKEN = "nextPageToken" diff --git a/lambdas/handlers/user_restrictions/__init__.py b/lambdas/handlers/user_restrictions/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/lambdas/handlers/user_restrictions/search_user_restriction_handler.py b/lambdas/handlers/user_restrictions/search_user_restriction_handler.py new file mode 100644 index 0000000000..14053672c1 --- /dev/null +++ b/lambdas/handlers/user_restrictions/search_user_restriction_handler.py @@ -0,0 +1,97 @@ +import json + +from enums.feature_flags import FeatureFlags +from enums.lambda_error import LambdaError +from enums.user_restriction_accepted_querystring_parameters import ( + UserRestrictionQuerystringParameters, +) +from services.feature_flags_service import FeatureFlagService +from services.user_restrictions.search_user_restriction_service import ( + DEFAULT_LIMIT, + SearchUserRestrictionService, +) +from utils.audit_logging_setup import LoggingService +from utils.decorators.ensure_env_var import ensure_environment_variables +from utils.decorators.handle_lambda_exceptions import handle_lambda_exceptions +from utils.decorators.override_error_check import override_error_check +from utils.decorators.set_audit_arg import set_request_context_for_logging +from utils.exceptions import OdsErrorException, UserRestrictionException +from utils.lambda_response import ApiGatewayResponse +from utils.ods_utils import extract_ods_code_from_request_context + +logger = LoggingService(__name__) + + +@set_request_context_for_logging +@ensure_environment_variables( + names=[ + "RESTRICTIONS_TABLE_NAME", + "APPCONFIG_APPLICATION", + "APPCONFIG_CONFIGURATION", + "APPCONFIG_ENVIRONMENT", + ], +) +@override_error_check +@handle_lambda_exceptions +def lambda_handler(event, _context): + try: + ods_code = extract_ods_code_from_request_context() + feature_flag_service = FeatureFlagService() + feature_flag_service.validate_feature_flag( + FeatureFlags.USER_RESTRICTION_ENABLED.value, + ) + + params = parse_querystring_parameters(event) + + service = SearchUserRestrictionService() + + restrictions, next_token = service.process_request( + ods_code=ods_code, + smart_card_id=params.get(UserRestrictionQuerystringParameters.SMARTCARD_ID), + nhs_number=params.get(UserRestrictionQuerystringParameters.NHS_NUMBER), + next_page_token=params.get( + UserRestrictionQuerystringParameters.NEXT_PAGE_TOKEN, + ), + limit=params.get(UserRestrictionQuerystringParameters.LIMIT, DEFAULT_LIMIT), + ) + + response_body: dict = { + "restrictions": restrictions, + "count": len(restrictions), + } + + if next_token: + response_body["nextPageToken"] = next_token + + return ApiGatewayResponse( + status_code=200, + body=json.dumps(response_body), + methods="GET", + ).create_api_gateway_response() + + except OdsErrorException as e: + logger.error(e) + return ApiGatewayResponse( + status_code=401, + body=LambdaError.SearchUserRestrictionMissingODS.create_error_body(), + methods="GET", + ).create_api_gateway_response() + + except UserRestrictionException as e: + logger.error(f"Invalid query parameter value: {e}") + return ApiGatewayResponse( + status_code=400, + body=LambdaError.SearchUserRestrictionsInvalidQueryParameter.create_error_body(), + methods="GET", + ).create_api_gateway_response() + + +def parse_querystring_parameters(event: dict) -> dict: + logger.info("Parsing query string parameters.") + params = event.get("queryStringParameters") or {} + + return { + param.value: params[param.value] + for param in UserRestrictionQuerystringParameters + if param.value in params + } diff --git a/lambdas/models/user_restrictions/user_restriction_response.py b/lambdas/models/user_restrictions/user_restriction_response.py new file mode 100644 index 0000000000..f913a26ef4 --- /dev/null +++ b/lambdas/models/user_restrictions/user_restriction_response.py @@ -0,0 +1,10 @@ +from typing import Optional + +from models.user_restrictions.user_restrictions import UserRestriction + + +class UserRestrictionResponse(UserRestriction): + patient_given_name: Optional[list[str]] = None + patient_family_name: Optional[str] = None + restricted_user_first_name: Optional[str] = None + restricted_user_last_name: Optional[str] = None diff --git a/lambdas/models/user_restrictions/user_restrictions.py b/lambdas/models/user_restrictions/user_restrictions.py index 375a1af8fc..c9e36e0058 100644 --- a/lambdas/models/user_restrictions/user_restrictions.py +++ b/lambdas/models/user_restrictions/user_restrictions.py @@ -10,7 +10,7 @@ class UserRestrictionsFields(StrEnum): ID = "ID" CREATOR = "CreatorSmartcard" RESTRICTED_USER = "RestrictedSmartcard" - REMOVED_BY = "RemoverSmartCard" + REMOVED_BY = "RemoverSmartcard" class UserRestriction(BaseModel): diff --git a/lambdas/services/authoriser_service.py b/lambdas/services/authoriser_service.py index 2f86cdea3f..b3c8a25177 100644 --- a/lambdas/services/authoriser_service.py +++ b/lambdas/services/authoriser_service.py @@ -149,6 +149,8 @@ def deny_access_policy(self, path, http_verb, user_role, nhs_number: str = None) deny_resource = ( not patient_access_is_allowed or is_user_gp_clinical or is_user_pcse ) + case "/UserRestriction": + deny_resource = False case "/VirusScan": deny_resource = ( diff --git a/lambdas/services/base/dynamo_service.py b/lambdas/services/base/dynamo_service.py index 3ce672d780..be38d2e156 100644 --- a/lambdas/services/base/dynamo_service.py +++ b/lambdas/services/base/dynamo_service.py @@ -6,6 +6,7 @@ import boto3 from boto3.dynamodb.conditions import Attr, ConditionBase, Key from botocore.exceptions import ClientError + from utils.audit_logging_setup import LoggingService from utils.dynamo_utils import ( create_expression_attribute_values, @@ -184,7 +185,7 @@ def create_item(self, table_name, item, key_name: str | None = None): Args: table_name: Name of the DynamoDB table item: The item to be inserted (as a dictionary) - key_name: The name of the key field to check existance for conditional put + key_name: The name of the key field to check existence for conditional put Returns: Response from the DynamoDB put_item operation Raises: @@ -370,7 +371,7 @@ def transact_write_items(self, transact_items: Sequence[dict]): transact_items: List of transaction items (Put, Update, Delete, ConditionCheck) Raises: - ClientError: If the transaction fails (e.g., TransactionCanceledException) + ClientError: If the transaction fails (e.g. TransactionCanceledException) """ try: logger.info(f"Executing transaction with {len(transact_items)} items") @@ -497,7 +498,7 @@ def query_table_with_paginator( key: str, condition: str, filter_expression: str | None = None, - expression_attribute_names: str | None = None, + expression_attribute_names: dict | None = None, expression_attribute_values: dict | None = None, limit: int = 20, page_size: int = 1, diff --git a/lambdas/services/user_restrictions/search_user_restriction_service.py b/lambdas/services/user_restrictions/search_user_restriction_service.py new file mode 100644 index 0000000000..2b9d9ddd7e --- /dev/null +++ b/lambdas/services/user_restrictions/search_user_restriction_service.py @@ -0,0 +1,183 @@ +from botocore.exceptions import ClientError + +from enums.lambda_error import LambdaError +from models.user_restrictions.user_restriction_response import UserRestrictionResponse +from models.user_restrictions.user_restrictions import UserRestriction +from services.user_restrictions.user_restriction_dynamo_service import ( + DEFAULT_LIMIT, + MAX_LIMIT, + UserRestrictionDynamoService, +) +from services.user_restrictions.utilites import get_healthcare_worker_api_service +from utils.audit_logging_setup import LoggingService +from utils.exceptions import ( + HealthcareWorkerAPIException, + HealthcareWorkerPractitionerModelException, + InvalidParamException, + InvalidResourceIdException, + PatientNotFoundException, + PdsErrorException, + UserRestrictionException, + UserRestrictionValidationException, +) +from utils.lambda_exceptions import SearchUserRestrictionException +from utils.pagination_utils import validate_next_page_token +from utils.utilities import get_pds_service + +logger = LoggingService(__name__) + + +class SearchUserRestrictionService: + def __init__(self): + self.dynamo_service = UserRestrictionDynamoService() + self.pds_service = get_pds_service() + self.hwc_service = get_healthcare_worker_api_service() + self._patient_cache: dict = {} + self._practitioner_cache: dict = {} + + def process_request( + self, + ods_code: str, + smart_card_id: str | None = None, + nhs_number: str | None = None, + next_page_token: str | None = None, + limit: int | str = DEFAULT_LIMIT, + ) -> tuple[list[dict], str | None]: + try: + limit = self.validate_limit(limit) + validate_next_page_token(next_page_token) + + logger.info(f"Querying user restrictions for ODS code {ods_code}") + restrictions, next_token = self.dynamo_service.query_restrictions( + ods_code=ods_code, + smart_card_id=smart_card_id, + nhs_number=nhs_number, + limit=limit, + start_key=next_page_token, + ) + logger.info(f"Found {len(restrictions)} restriction(s)") + + enriched = [ + self._enrich_restriction(restriction) for restriction in restrictions + ] + + serialised = [ + item.model_dump_camel_case( + exclude_none=True, + exclude={"last_updated", "is_active", "creator", "custodian"}, + mode="json", + ) + for item in enriched + ] + + return serialised, next_token + + except UserRestrictionValidationException as e: + logger.error(e) + raise SearchUserRestrictionException( + 500, + LambdaError.SearchUserRestrictionDB, + ) from e + except (UserRestrictionException, InvalidParamException) as e: + logger.error(e) + raise SearchUserRestrictionException( + 400, + LambdaError.SearchUserRestrictionInvalidQueryString, + ) from e + except ClientError as e: + logger.error(e) + raise SearchUserRestrictionException( + 500, + LambdaError.SearchUserRestrictionDB, + ) + + def _enrich_restriction( + self, + restriction: UserRestriction, + ) -> UserRestrictionResponse: + response = UserRestrictionResponse.model_validate( + restriction.model_dump(by_alias=True), + ) + + response = self._enrich_with_patient_name(response) + response = self._enrich_with_restricted_user_name(response) + return response + + def _enrich_with_patient_name( + self, + response: UserRestrictionResponse, + ) -> UserRestrictionResponse: + nhs_number = response.nhs_number + if nhs_number not in self._patient_cache: + logger.info( + f"Fetching patient details for NHS number ending {nhs_number[-4:]}", + ) + try: + patient_details = self.pds_service.fetch_patient_details(nhs_number) + self._patient_cache[nhs_number] = patient_details + except ( + PatientNotFoundException, + PdsErrorException, + InvalidResourceIdException, + ) as e: + logger.warning( + f"Could not fetch patient name for NHS number ending " + f"{nhs_number[-4:]}: {e}", + ) + self._patient_cache[nhs_number] = None + else: + logger.info( + f"Using cached patient details for NHS number ending {nhs_number[-4:]}", + ) + + patient_details = self._patient_cache[nhs_number] + if patient_details: + response.patient_given_name = patient_details.given_name + response.patient_family_name = patient_details.family_name + return response + + def _enrich_with_restricted_user_name( + self, + response: UserRestrictionResponse, + ) -> UserRestrictionResponse: + smartcard_id = response.restricted_user + if smartcard_id not in self._practitioner_cache: + logger.info(f"Fetching practitioner details for smartcard {smartcard_id}") + try: + practitioner = self.hwc_service.get_practitioner(smartcard_id) + self._practitioner_cache[smartcard_id] = practitioner + except ( + HealthcareWorkerAPIException, + HealthcareWorkerPractitionerModelException, + ) as e: + logger.warning( + f"Could not fetch practitioner name for smartcard " + f"{smartcard_id}: {e}", + ) + self._practitioner_cache[smartcard_id] = None + else: + logger.info( + f"Using cached practitioner details for smartcard {smartcard_id}", + ) + + practitioner = self._practitioner_cache[smartcard_id] + if practitioner: + response.restricted_user_first_name = practitioner.first_name + response.restricted_user_last_name = practitioner.last_name + return response + + @staticmethod + def validate_limit(limit: int | str) -> int: + try: + limit = int(limit) + except (TypeError, ValueError) as e: + logger.error(f"Invalid limit value: {limit}") + raise UserRestrictionException(f"Invalid limit value: {limit}") from e + + if limit < 1 or limit > MAX_LIMIT: + logger.error(f"Limit {limit} is out of range (1–{MAX_LIMIT})") + raise UserRestrictionException( + f"Limit {limit} is out of range (1–{MAX_LIMIT})", + ) + + return limit diff --git a/lambdas/services/user_restrictions/user_restriction_dynamo_service.py b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py new file mode 100644 index 0000000000..834ecf35d9 --- /dev/null +++ b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py @@ -0,0 +1,99 @@ +import os + +from pydantic import ValidationError + +from enums.dynamo_filter import ConditionOperator +from models.user_restrictions.user_restrictions import UserRestriction +from services.base.dynamo_service import DynamoDBService +from utils.audit_logging_setup import LoggingService +from utils.dynamo_utils import build_mixed_condition_expression +from utils.exceptions import UserRestrictionValidationException + +logger = LoggingService(__name__) + +DEFAULT_LIMIT = 10 +MAX_LIMIT = 100 + +SMARTCARD_KEY = "RestrictedSmartcard" +NHS_NUMBER_KEY = "NhsNumber" +ODS_CODE_GSI = "CustodianIndex" +ODS_CODE_KEY = "Custodian" + + +class UserRestrictionDynamoService: + def __init__(self): + self.dynamo_service = DynamoDBService() + self.table_name = os.environ["RESTRICTIONS_TABLE_NAME"] + + def query_restrictions( + self, + ods_code: str, + smart_card_id: str | None = None, + nhs_number: str | None = None, + limit: int = DEFAULT_LIMIT, + start_key: str | None = None, + ) -> tuple[list[UserRestriction], str | None]: + filter_expression, expression_attribute_names, expression_attribute_values = ( + self._build_query_filter( + smart_card_id=smart_card_id, + nhs_number=nhs_number, + ) + ) + + response = self.dynamo_service.query_table_with_paginator( + table_name=self.table_name, + index_name=ODS_CODE_GSI, + key=ODS_CODE_KEY, + condition=ods_code, + filter_expression=filter_expression, + expression_attribute_names=expression_attribute_names, + expression_attribute_values=expression_attribute_values, + limit=limit, + start_key=start_key, + ) + + items = response.get("Items", []) + restrictions = self._validate_restrictions(items) + next_token = response.get("NextToken") + + return restrictions, next_token + + @staticmethod + def _build_query_filter( + smart_card_id: str | None, + nhs_number: str | None, + ) -> tuple[str, dict, dict]: + conditions = [ + { + "field": "IsActive", + "operator": ConditionOperator.EQUAL.value, + "value": True, + }, + ] + if smart_card_id: + conditions.append( + { + "field": SMARTCARD_KEY, + "operator": ConditionOperator.EQUAL.value, + "value": smart_card_id, + }, + ) + if nhs_number: + conditions.append( + { + "field": NHS_NUMBER_KEY, + "operator": ConditionOperator.EQUAL.value, + "value": nhs_number, + }, + ) + return build_mixed_condition_expression(conditions) + + @staticmethod + def _validate_restrictions(items: list[dict]) -> list[UserRestriction]: + try: + return [UserRestriction.model_validate(item) for item in items] + except ValidationError as e: + logger.error(e) + raise UserRestrictionValidationException( + f"Failed to validate user restrictions: {e}", + ) from e diff --git a/lambdas/services/user_restrictions/utilites.py b/lambdas/services/user_restrictions/utilites.py index d99676601b..64719f29d7 100644 --- a/lambdas/services/user_restrictions/utilites.py +++ b/lambdas/services/user_restrictions/utilites.py @@ -9,6 +9,6 @@ def get_healthcare_worker_api_service() -> HealthCareWorkerApiService: - if os.getenv("USE_MOCK_HEALTHCARE_SERVICE") in ["False", "false"]: + if os.getenv("USE_MOCK_HEALTHCARE_SERVICE", "true") in ["False", "false"]: return HealthCareWorkerApiService() return MockHealthcareWorkerApiService() diff --git a/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_does_not_return_inactive_restrictions.json b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_does_not_return_inactive_restrictions.json new file mode 100644 index 0000000000..13605469c0 --- /dev/null +++ b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_does_not_return_inactive_restrictions.json @@ -0,0 +1,18 @@ +{ + "count": 1, + "restrictions": [ + { + "created": 1741392000, + "id": "a1b2c3d4-0003-4000-8000-000000000003", + "nhsNumber": "9449305943", + "patientFamilyName": "schauffer", + "patientGivenName": [ + "goodwin", + "harley" + ], + "restrictedUser": "123456789012", + "restrictedUserFirstName": "John", + "restrictedUserLastName": "Doe" + } + ] +} diff --git a/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_filters_by_nhs_number.json b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_filters_by_nhs_number.json new file mode 100644 index 0000000000..f21a1e7112 --- /dev/null +++ b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_filters_by_nhs_number.json @@ -0,0 +1,43 @@ +{ + "count": 3, + "restrictions": [ + { + "created": 1700000000, + "nhsNumber": "9449305943", + "patientFamilyName": "schauffer", + "patientGivenName": [ + "goodwin", + "harley" + ], + "restrictedUser": "123456789012", + "restrictedUserFirstName": "John", + "restrictedUserLastName": "Doe" + }, + { + "created": 1740787200, + "id": "a1b2c3d4-0001-4000-8000-000000000001", + "nhsNumber": "9449305943", + "patientFamilyName": "schauffer", + "patientGivenName": [ + "goodwin", + "harley" + ], + "restrictedUser": "777777777777", + "restrictedUserFirstName": "Jane Amy Sheila Claire", + "restrictedUserLastName": "Doe" + }, + { + "created": 1741392000, + "id": "a1b2c3d4-0003-4000-8000-000000000003", + "nhsNumber": "9449305943", + "patientFamilyName": "schauffer", + "patientGivenName": [ + "goodwin", + "harley" + ], + "restrictedUser": "123456789012", + "restrictedUserFirstName": "John", + "restrictedUserLastName": "Doe" + } + ] +} diff --git a/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_filters_by_smartcard_id.json b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_filters_by_smartcard_id.json new file mode 100644 index 0000000000..d2cfaa82bc --- /dev/null +++ b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_filters_by_smartcard_id.json @@ -0,0 +1,30 @@ +{ + "count": 2, + "restrictions": [ + { + "created": 1700000000, + "nhsNumber": "9449305943", + "patientFamilyName": "schauffer", + "patientGivenName": [ + "goodwin", + "harley" + ], + "restrictedUser": "123456789012", + "restrictedUserFirstName": "John", + "restrictedUserLastName": "Doe" + }, + { + "created": 1741392000, + "id": "a1b2c3d4-0003-4000-8000-000000000003", + "nhsNumber": "9449305943", + "patientFamilyName": "schauffer", + "patientGivenName": [ + "goodwin", + "harley" + ], + "restrictedUser": "123456789012", + "restrictedUserFirstName": "John", + "restrictedUserLastName": "Doe" + } + ] +} diff --git a/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_paginates_with_limit_and_next_page_token.1.json b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_paginates_with_limit_and_next_page_token.1.json new file mode 100644 index 0000000000..9acd644a69 --- /dev/null +++ b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_paginates_with_limit_and_next_page_token.1.json @@ -0,0 +1,28 @@ +{ + "count": 2, + "nextPageToken": "eyJFeGNsdXNpdmVTdGFydEtleSI6IHsiQ3JlYXRlZCI6IHsiTiI6ICIxNzQwNzg3MjAwIn0sICJJRCI6IHsiUyI6ICJhMWIyYzNkNC0wMDAyLTQwMDAtODAwMC0wMDAwMDAwMDAwMDIifSwgIkN1c3RvZGlhbiI6IHsiUyI6ICJIODExMDkifX19", + "restrictions": [ + { + "created": 1700000000, + "nhsNumber": "9449305943", + "patientFamilyName": "schauffer", + "patientGivenName": [ + "goodwin", + "harley" + ], + "restrictedUser": "123456789012", + "restrictedUserFirstName": "John", + "restrictedUserLastName": "Doe" + }, + { + "created": 1740787200, + "id": "a1b2c3d4-0002-4000-8000-000000000002", + "nhsNumber": "9730136912", + "patientFamilyName": "BOLTON", + "patientGivenName": [ + "Leah" + ], + "restrictedUser": "323456789033" + } + ] +} diff --git a/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_paginates_with_limit_and_next_page_token.json b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_paginates_with_limit_and_next_page_token.json new file mode 100644 index 0000000000..42e775b4c2 --- /dev/null +++ b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_paginates_with_limit_and_next_page_token.json @@ -0,0 +1,29 @@ +{ + "count": 2, + "restrictions": [ + { + "created": 1700000000, + "nhsNumber": "9449305943", + "patientFamilyName": "schauffer", + "patientGivenName": [ + "goodwin", + "harley" + ], + "restrictedUser": "123456789012", + "restrictedUserFirstName": "John", + "restrictedUserLastName": "Doe" + }, + { + "created": 1700000000, + "nhsNumber": "9449305943", + "patientFamilyName": "schauffer", + "patientGivenName": [ + "goodwin", + "harley" + ], + "restrictedUser": "123456789012", + "restrictedUserFirstName": "John", + "restrictedUserLastName": "Doe" + } + ] +} diff --git a/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_returns_200_with_restrictions.json b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_returns_200_with_restrictions.json new file mode 100644 index 0000000000..26bee5e953 --- /dev/null +++ b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_returns_200_with_restrictions.json @@ -0,0 +1,53 @@ +{ + "count": 4, + "restrictions": [ + { + "created": 1700000000, + "nhsNumber": "9449305943", + "patientFamilyName": "schauffer", + "patientGivenName": [ + "goodwin", + "harley" + ], + "restrictedUser": "123456789012", + "restrictedUserFirstName": "John", + "restrictedUserLastName": "Doe" + }, + { + "created": 1740787200, + "id": "a1b2c3d4-0002-4000-8000-000000000002", + "nhsNumber": "9730136912", + "patientFamilyName": "BOLTON", + "patientGivenName": [ + "Leah" + ], + "restrictedUser": "323456789033" + }, + { + "created": 1740787200, + "id": "a1b2c3d4-0001-4000-8000-000000000001", + "nhsNumber": "9449305943", + "patientFamilyName": "schauffer", + "patientGivenName": [ + "goodwin", + "harley" + ], + "restrictedUser": "777777777777", + "restrictedUserFirstName": "Jane Amy Sheila Claire", + "restrictedUserLastName": "Doe" + }, + { + "created": 1741392000, + "id": "a1b2c3d4-0003-4000-8000-000000000003", + "nhsNumber": "9449305943", + "patientFamilyName": "schauffer", + "patientGivenName": [ + "goodwin", + "harley" + ], + "restrictedUser": "123456789012", + "restrictedUserFirstName": "John", + "restrictedUserLastName": "Doe" + } + ] +} diff --git a/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_returns_401_without_auth.json b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_returns_401_without_auth.json new file mode 100644 index 0000000000..f03a4f27a0 --- /dev/null +++ b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_returns_401_without_auth.json @@ -0,0 +1,3 @@ +{ + "message": "Unauthorized" +} diff --git a/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_returns_empty_list_when_no_matches.json b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_returns_empty_list_when_no_matches.json new file mode 100644 index 0000000000..1d0a16b21b --- /dev/null +++ b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_returns_empty_list_when_no_matches.json @@ -0,0 +1,4 @@ +{ + "count": 0, + "restrictions": [] +} diff --git a/lambdas/tests/e2e/api/files/user_restrictions_seed_data.json b/lambdas/tests/e2e/api/files/user_restrictions_seed_data.json new file mode 100644 index 0000000000..6f81448267 --- /dev/null +++ b/lambdas/tests/e2e/api/files/user_restrictions_seed_data.json @@ -0,0 +1,70 @@ +[ + { + "ID": "a1b2c3d4-0001-4000-8000-000000000001", + "RestrictedSmartcard": "SC001", + "NhsNumber": "9449305943", + "Custodian": "H81109", + "CreatorSmartcard": "SC099", + "RemoverSmartcard": null, + "IsActive": true, + "Created": 1740787200, + "LastUpdated": 1740787200 + }, + { + "ID": "a1b2c3d4-0002-4000-8000-000000000002", + "RestrictedSmartcard": "SC001", + "NhsNumber": "9730136912", + "Custodian": "H81109", + "CreatorSmartcard": "SC099", + "RemoverSmartcard": null, + "IsActive": true, + "Created": 1740787200, + "LastUpdated": 1740787200 + }, + { + "ID": "a1b2c3d4-0003-4000-8000-000000000003", + "RestrictedSmartcard": "SC002", + "NhsNumber": "9449305943", + "Custodian": "H81109", + "CreatorSmartcard": "SC099", + "RemoverSmartcard": null, + "IsActive": true, + "Created": 1741392000, + "LastUpdated": 1741392000 + }, + { + "ID": "a1b2c3d4-0004-4000-8000-000000000004", + "RestrictedSmartcard": "SC002", + "NhsNumber": "9730153973", + "Custodian": "H81109", + "CreatorSmartcard": "SC099", + "RemoverSmartcard": null, + "IsActive": true, + "Created": 1741392000, + "LastUpdated": 1741392000 + }, + { + "ID": "a1b2c3d4-0005-4000-8000-000000000005", + "RestrictedSmartcard": "SC003", + "NhsNumber": "9449305943", + "Custodian": "H81109", + "CreatorSmartcard": "SC099", + "RemoverSmartcard": "SC099", + "IsActive": false, + "Created": 1738368000, + "LastUpdated": 1740873600 + }, + { + "ID": "a1b2c3d4-0006-4000-8000-000000000006", + "RestrictedSmartcard": "SC001", + "NhsNumber": "9449305943", + "Custodian": "Y12345", + "CreatorSmartcard": "SC088", + "RemoverSmartcard": null, + "IsActive": true, + "Created": 1740960000, + "LastUpdated": 1740960000 + } +] + + diff --git a/lambdas/tests/e2e/api/test_search_user_restriction_api.py b/lambdas/tests/e2e/api/test_search_user_restriction_api.py new file mode 100644 index 0000000000..c84247c7c3 --- /dev/null +++ b/lambdas/tests/e2e/api/test_search_user_restriction_api.py @@ -0,0 +1,220 @@ +import logging +import uuid + +import pytest +import requests +from syrupy.filters import paths + +from tests.e2e.helpers.data_helper import UserRestrictionDataHelper +from tests.e2e.helpers.mockcis2_helper import MockCis2Helper + +data_helper = UserRestrictionDataHelper() + +TEST_ODS_CODE = "H81109" +TEST_NHS_NUMBER = "9449305943" +TEST_SMARTCARD_ID = "123456789012" +ANOTHER_SMARTCARD_ID = "323456789033" + + +@pytest.fixture(scope="module") +def auth_token(): + login_helper = MockCis2Helper(ods=TEST_ODS_CODE, repository_role="gp_admin") + login_helper.generate_mockcis2_token() + return login_helper.user_token + + +def _headers(token: str) -> dict: + return {"Authorization": f"{token}"} + + +def _build_restriction( + restriction_id: str | None = None, + ods_code: str = TEST_ODS_CODE, + nhs_number: str = TEST_NHS_NUMBER, + smartcard_id: str = TEST_SMARTCARD_ID, + creator: str = ANOTHER_SMARTCARD_ID, + is_active: bool = True, +) -> dict: + return { + "ID": restriction_id or str(uuid.uuid4()), + "Custodian": ods_code, + "NhsNumber": nhs_number, + "RestrictedSmartcard": smartcard_id, + "CreatorSmartcard": creator, + "IsActive": is_active, + "Created": 1700000000, + "LastUpdated": 1700000001, + } + + +@pytest.fixture +def test_restrictions(): + created = [] + yield created + for record in created: + data_helper.tidyup(record) + + +def test_search_user_restriction_returns_200_with_restrictions( + test_restrictions, + auth_token, + snapshot_json, +): + restriction = _build_restriction() + data_helper.create_restriction(restriction) + test_restrictions.append(restriction) + + url = f"https://{data_helper.api_endpoint}/UserRestriction" + response = requests.get(url, headers=_headers(auth_token)) + + logging.info(response.json()) + assert response.status_code == 200 + + body = response.json() + assert body == snapshot_json(exclude=paths("restrictions.0.id")) + + +def test_search_user_restriction_filters_by_smartcard_id( + test_restrictions, + auth_token, + snapshot_json, +): + target_restriction = _build_restriction(smartcard_id=TEST_SMARTCARD_ID) + other_restriction = _build_restriction(smartcard_id=ANOTHER_SMARTCARD_ID) + + data_helper.create_restriction(target_restriction) + data_helper.create_restriction(other_restriction) + test_restrictions.append(target_restriction) + test_restrictions.append(other_restriction) + + url = f"https://{data_helper.api_endpoint}/UserRestriction" + response = requests.get( + url, + headers=_headers(auth_token), + params={"smartcardId": TEST_SMARTCARD_ID}, + ) + + logging.info(response.json()) + assert response.status_code == 200 + + body = response.json() + assert body == snapshot_json(exclude=paths("restrictions.0.id")) + + +def test_search_user_restriction_filters_by_nhs_number( + test_restrictions, + auth_token, + snapshot_json, +): + target_nhs = "9449305943" + other_nhs = "9730136912" + + target_restriction = _build_restriction(nhs_number=target_nhs) + other_restriction = _build_restriction(nhs_number=other_nhs) + + data_helper.create_restriction(target_restriction) + data_helper.create_restriction(other_restriction) + test_restrictions.append(target_restriction) + test_restrictions.append(other_restriction) + + url = f"https://{data_helper.api_endpoint}/UserRestriction" + response = requests.get( + url, + headers=_headers(auth_token), + params={"nhsNumber": target_nhs}, + ) + + logging.info(response.json()) + assert response.status_code == 200 + + body = response.json() + assert body == snapshot_json(exclude=paths("restrictions.0.id")) + + +def test_search_user_restriction_returns_empty_list_when_no_matches( + test_restrictions, + auth_token, + snapshot_json, +): + non_existent_smartcard = f"NONEXISTENT_{uuid.uuid4().hex[:8]}" + + url = f"https://{data_helper.api_endpoint}/UserRestriction" + response = requests.get( + url, + headers=_headers(auth_token), + params={"smartcardId": non_existent_smartcard}, + ) + + logging.info(response.json()) + assert response.status_code == 200 + + body = response.json() + assert body == snapshot_json + + +def test_search_user_restriction_does_not_return_inactive_restrictions( + test_restrictions, + auth_token, + snapshot_json, +): + inactive_restriction = _build_restriction(is_active=False) + data_helper.create_restriction(inactive_restriction) + test_restrictions.append(inactive_restriction) + + url = f"https://{data_helper.api_endpoint}/UserRestriction" + response = requests.get( + url, + headers=_headers(auth_token), + params={"smartcardId": TEST_SMARTCARD_ID}, + ) + + logging.info(response.json()) + assert response.status_code == 200 + + body = response.json() + assert body == snapshot_json + + +def test_search_user_restriction_paginates_with_limit_and_next_page_token( + test_restrictions, + auth_token, + snapshot_json, +): + restrictions = [_build_restriction() for _ in range(3)] + for r in restrictions: + data_helper.create_restriction(r) + test_restrictions.append(r) + + url = f"https://{data_helper.api_endpoint}/UserRestriction" + + response = requests.get(url, headers=_headers(auth_token), params={"limit": "2"}) + logging.info(response.json()) + assert response.status_code == 200 + + body = response.json() + assert body == snapshot_json( + exclude=paths( + "restrictions.0.id", + "restrictions.1.id", + "nextPageToken", + ), + ) + + if "nextPageToken" in body: + next_response = requests.get( + url, + headers=_headers(auth_token), + params={"limit": "2", "nextPageToken": body["nextPageToken"]}, + ) + assert next_response.status_code == 200 + next_body = next_response.json() + assert next_body == snapshot_json(exclude=paths("restrictions.0.id")) + + +def test_search_user_restriction_returns_401_without_auth(snapshot_json): + url = f"https://{data_helper.api_endpoint}/UserRestriction" + response = requests.get(url) + + logging.info(response.json()) + assert response.status_code == 401 + assert response.json() == snapshot_json diff --git a/lambdas/tests/e2e/helpers/data_helper.py b/lambdas/tests/e2e/helpers/data_helper.py index 8116209005..da831d7bbb 100644 --- a/lambdas/tests/e2e/helpers/data_helper.py +++ b/lambdas/tests/e2e/helpers/data_helper.py @@ -232,6 +232,44 @@ def tidyup(self, record): ) +class UserRestrictionDataHelper: + def __init__(self): + self.workspace = os.environ.get("AWS_WORKSPACE", "") + self.dynamo_service = DynamoDBService() + self.dynamo_table = None + self.api_endpoint = None + + self._build_env() + + def _build_env(self): + if not self.workspace: + raise ValueError("AWS_WORKSPACE environment variable is missing or empty.") + self.dynamo_table = f"{self.workspace}_UserRestrictions" + + domain = ( + "national-document-repository.nhs.uk" + if self.workspace == "pre-prod" + else "access-request-fulfilment.patient-deductions.nhs.uk" + ) + self.api_endpoint = ( + f"api.{self.workspace}.{domain}" + if self.workspace in {"pre-prod", "ndr-test"} + else f"api-{self.workspace}.{domain}" + ) + + def create_restriction(self, restriction: dict) -> None: + self.dynamo_service.create_item(self.dynamo_table, restriction) + + def delete_restriction(self, restriction_id: str) -> None: + self.dynamo_service.delete_item( + table_name=self.dynamo_table, + key={"ID": restriction_id}, + ) + + def tidyup(self, record: dict) -> None: + self.delete_restriction(record["ID"]) + + class LloydGeorgeDataHelper(DataHelper): def __init__(self): self.bulk_upload_table_name = "BulkUploadReport" diff --git a/lambdas/tests/unit/conftest.py b/lambdas/tests/unit/conftest.py index 75fd49077a..153dd3f8bb 100644 --- a/lambdas/tests/unit/conftest.py +++ b/lambdas/tests/unit/conftest.py @@ -147,6 +147,7 @@ MOCK_DOCUMENT_REVIEW_TABLE = "test_document_review" MOCK_DOCUMENT_REVIEW_BUCKET = "test_document_review_bucket" MOCK_EDGE_REFERENCE_TABLE = "test_edge_reference_table" +MOCK_USER_RESTRICTION_TABLE = "test_user_restriction_table" @pytest.fixture @@ -255,6 +256,7 @@ def set_env(monkeypatch): monkeypatch.setenv("EDGE_REFERENCE_TABLE", MOCK_EDGE_REFERENCE_TABLE) monkeypatch.setenv("REVIEW_SQS_QUEUE_URL", REVIEW_SQS_QUEUE_URL) monkeypatch.setenv("HEALTHCARE_WORKER_API_URL", HEALTHCARE_WORKER_API_URL) + monkeypatch.setenv("RESTRICTIONS_TABLE_NAME", MOCK_USER_RESTRICTION_TABLE) EXPECTED_PARSED_PATIENT_BASE_CASE = PatientDetails( diff --git a/lambdas/tests/unit/handlers/test_search_user_restriction_handler.py b/lambdas/tests/unit/handlers/test_search_user_restriction_handler.py new file mode 100644 index 0000000000..b18210195e --- /dev/null +++ b/lambdas/tests/unit/handlers/test_search_user_restriction_handler.py @@ -0,0 +1,331 @@ +import base64 +import json + +import pytest + +from enums.feature_flags import FeatureFlags +from enums.lambda_error import LambdaError +from handlers.user_restrictions.search_user_restriction_handler import ( + lambda_handler, + parse_querystring_parameters, +) +from tests.unit.conftest import MOCK_INTERACTION_ID, TEST_NHS_NUMBER, TEST_UUID +from utils.exceptions import OdsErrorException +from utils.lambda_exceptions import ( + FeatureFlagsException, + SearchUserRestrictionException, +) +from utils.lambda_response import ApiGatewayResponse + +TEST_ODS_CODE = "Y12345" +TEST_SMART_CARD_ID = "SC001" + +TEST_LAST_EVALUATED_KEY = {"ID": TEST_UUID} +TEST_ENCODED_START_KEY = base64.b64encode( + json.dumps(TEST_LAST_EVALUATED_KEY).encode("utf-8"), +).decode("utf-8") + +MOCK_RESTRICTION = { + "id": TEST_UUID, + "restrictedSmartcard": TEST_SMART_CARD_ID, + "nhsNumber": TEST_NHS_NUMBER, + "custodian": TEST_ODS_CODE, + "isActive": True, +} + +MOCK_USER_RESTRICTION_TABLE = "test_user_restriction_table" + + +@pytest.fixture +def mock_feature_flag_service(mocker): + mocked_class = mocker.patch( + "handlers.user_restrictions.search_user_restriction_handler.FeatureFlagService", + ) + mocked_instance = mocked_class.return_value + mocked_instance.validate_feature_flag.return_value = None + yield mocked_instance + + +@pytest.fixture +def mock_service(mocker, mock_feature_flag_service): + mocked_class = mocker.patch( + "handlers.user_restrictions.search_user_restriction_handler.SearchUserRestrictionService", + ) + mocked_instance = mocked_class.return_value + mocked_instance.process_request.return_value = ([MOCK_RESTRICTION], None) + yield mocked_instance + + +@pytest.fixture +def mocked_extract_ods(mocker): + mock = mocker.patch( + "handlers.user_restrictions.search_user_restriction_handler.extract_ods_code_from_request_context", + ) + mock.return_value = TEST_ODS_CODE + yield mock + + +@pytest.fixture +def mocked_extract_ods_error(mocker): + mock = mocker.patch( + "handlers.user_restrictions.search_user_restriction_handler.extract_ods_code_from_request_context", + ) + mock.side_effect = OdsErrorException() + yield mock + + +@pytest.fixture +def base_event(set_env): + return { + "httpMethod": "GET", + "queryStringParameters": None, + "headers": {"Authorization": "test_token"}, + } + + +@pytest.fixture +def event_with_smart_card(base_event): + event = base_event.copy() + event["queryStringParameters"] = {"smartcardId": TEST_SMART_CARD_ID} + return event + + +@pytest.fixture +def event_with_nhs_number(base_event): + event = base_event.copy() + event["queryStringParameters"] = {"nhsNumber": TEST_NHS_NUMBER} + return event + + +@pytest.fixture +def event_with_limit_and_token(base_event): + event = base_event.copy() + event["queryStringParameters"] = { + "limit": "5", + "nextPageToken": TEST_ENCODED_START_KEY, + } + return event + + +@pytest.fixture +def event_with_unknown_params(base_event): + event = base_event.copy() + event["queryStringParameters"] = {"unknown_param": "value"} + return event + + +def test_parse_querystring_parameters_returns_empty_dict_for_none(base_event): + assert parse_querystring_parameters(base_event) == {} + + +def test_parse_querystring_parameters_extracts_smart_card_id(event_with_smart_card): + result = parse_querystring_parameters(event_with_smart_card) + assert result == {"smartcardId": TEST_SMART_CARD_ID} + + +def test_parse_querystring_parameters_extracts_nhs_number(event_with_nhs_number): + result = parse_querystring_parameters(event_with_nhs_number) + assert result == {"nhsNumber": TEST_NHS_NUMBER} + + +def test_parse_querystring_parameters_extracts_limit_and_token( + event_with_limit_and_token, +): + result = parse_querystring_parameters(event_with_limit_and_token) + assert result == { + "limit": "5", + "nextPageToken": TEST_ENCODED_START_KEY, + } + + +def test_parse_querystring_parameters_ignores_unknown_params( + event_with_unknown_params, +): + result = parse_querystring_parameters(event_with_unknown_params) + assert result == {} + + +def test_handler_returns_200_with_restrictions_list( + base_event, + mock_service, + mocked_extract_ods, + context, +): + expected_body = json.dumps({"restrictions": [MOCK_RESTRICTION], "count": 1}) + expected = ApiGatewayResponse( + status_code=200, + body=expected_body, + methods="GET", + ).create_api_gateway_response() + + actual = lambda_handler(base_event, context) + + assert actual == expected + mock_service.process_request.assert_called_once_with( + ods_code=TEST_ODS_CODE, + smart_card_id=None, + nhs_number=None, + next_page_token=None, + limit=10, + ) + + +def test_handler_includes_next_token_when_present( + base_event, + mock_service, + mocked_extract_ods, + context, +): + mock_service.process_request.return_value = ( + [MOCK_RESTRICTION], + TEST_ENCODED_START_KEY, + ) + + actual = lambda_handler(base_event, context) + + body = json.loads(actual["body"]) + assert body["nextPageToken"] == TEST_ENCODED_START_KEY + + +def test_handler_returns_200_with_empty_list_when_no_results( + base_event, + mock_service, + mocked_extract_ods, + context, +): + mock_service.process_request.return_value = ([], None) + + actual = lambda_handler(base_event, context) + + assert actual["statusCode"] == 200 + body = json.loads(actual["body"]) + assert body["restrictions"] == [] + assert body["count"] == 0 + + +def test_handler_calls_service_with_smart_card_id( + event_with_smart_card, + mock_service, + mocked_extract_ods, + context, +): + lambda_handler(event_with_smart_card, context) + + mock_service.process_request.assert_called_once_with( + ods_code=TEST_ODS_CODE, + smart_card_id=TEST_SMART_CARD_ID, + nhs_number=None, + next_page_token=None, + limit=10, + ) + + +def test_handler_calls_service_with_nhs_number( + event_with_nhs_number, + mock_service, + mocked_extract_ods, + context, +): + lambda_handler(event_with_nhs_number, context) + + mock_service.process_request.assert_called_once_with( + ods_code=TEST_ODS_CODE, + smart_card_id=None, + nhs_number=TEST_NHS_NUMBER, + next_page_token=None, + limit=10, + ) + + +def test_handler_passes_limit_and_token_to_service( + event_with_limit_and_token, + mock_service, + mocked_extract_ods, + context, +): + lambda_handler(event_with_limit_and_token, context) + + mock_service.process_request.assert_called_once_with( + ods_code=TEST_ODS_CODE, + smart_card_id=None, + nhs_number=None, + next_page_token=TEST_ENCODED_START_KEY, + limit="5", + ) + + +def test_handler_returns_401_when_no_ods_code_in_request_context( + base_event, + mock_service, + mocked_extract_ods_error, + context, +): + expected_body = json.dumps( + { + "message": LambdaError.SearchUserRestrictionMissingODS.value["message"], + "err_code": LambdaError.SearchUserRestrictionMissingODS.value["err_code"], + "interaction_id": MOCK_INTERACTION_ID, + }, + ) + expected = ApiGatewayResponse( + status_code=401, + body=expected_body, + methods="GET", + ).create_api_gateway_response() + + actual = lambda_handler(base_event, context) + + assert actual == expected + + +def test_handler_returns_400_when_service_raises_invalid_query_string( + base_event, + mock_service, + mocked_extract_ods, + context, +): + mock_service.process_request.side_effect = SearchUserRestrictionException( + 400, + LambdaError.SearchUserRestrictionInvalidQueryString, + ) + + actual = lambda_handler(base_event, context) + + assert actual["statusCode"] == 400 + + +def test_handler_returns_500_when_service_raises_db_error( + base_event, + mock_service, + mocked_extract_ods, + context, +): + mock_service.process_request.side_effect = SearchUserRestrictionException( + 500, + LambdaError.SearchUserRestrictionDB, + ) + + actual = lambda_handler(base_event, context) + + assert actual["statusCode"] == 500 + + +def test_handler_returns_404_when_feature_flag_disabled( + base_event, + mock_service, + mock_feature_flag_service, + mocked_extract_ods, + context, +): + mock_feature_flag_service.validate_feature_flag.side_effect = FeatureFlagsException( + 404, + LambdaError.FeatureFlagDisabled, + ) + + actual = lambda_handler(base_event, context) + + assert actual["statusCode"] == 404 + mock_feature_flag_service.validate_feature_flag.assert_called_once_with( + FeatureFlags.USER_RESTRICTION_ENABLED.value, + ) + mock_service.process_request.assert_not_called() diff --git a/lambdas/tests/unit/services/user_restriction/test_search_user_restriction_service.py b/lambdas/tests/unit/services/user_restriction/test_search_user_restriction_service.py new file mode 100644 index 0000000000..3c20b5f15e --- /dev/null +++ b/lambdas/tests/unit/services/user_restriction/test_search_user_restriction_service.py @@ -0,0 +1,283 @@ +import pytest +from botocore.exceptions import ClientError + +from enums.lambda_error import LambdaError +from models.user_restrictions.practitioner import Practitioner +from models.user_restrictions.user_restriction_response import UserRestrictionResponse +from models.user_restrictions.user_restrictions import UserRestriction +from services.user_restrictions.search_user_restriction_service import ( + MAX_LIMIT, + SearchUserRestrictionService, +) +from services.user_restrictions.user_restriction_dynamo_service import DEFAULT_LIMIT +from tests.unit.conftest import TEST_NHS_NUMBER, TEST_UUID +from utils.exceptions import ( + HealthcareWorkerAPIException, + PatientNotFoundException, + PdsErrorException, + UserRestrictionException, + UserRestrictionValidationException, +) +from utils.lambda_exceptions import SearchUserRestrictionException + +TEST_ODS_CODE = "Y12345" +TEST_SMART_CARD_ID = "SC001" +TEST_NEXT_TOKEN = "some-opaque-next-token" + +MOCK_RESTRICTION_DICT = { + "ID": TEST_UUID, + "RestrictedSmartcard": TEST_SMART_CARD_ID, + "NhsNumber": TEST_NHS_NUMBER, + "Custodian": TEST_ODS_CODE, + "Created": 1700000000, + "CreatorSmartcard": "SC002", + "IsActive": True, + "LastUpdated": 1700000001, +} +MOCK_RESTRICTION = UserRestriction.model_validate(MOCK_RESTRICTION_DICT) +MOCK_RESPONSE = UserRestrictionResponse.model_validate(MOCK_RESTRICTION_DICT) + +MOCK_PRACTITIONER = Practitioner( + smartcard_id=TEST_SMART_CARD_ID, + first_name="Jane", + last_name="Doe", +) + + +@pytest.fixture +def mock_service(set_env, mocker): + mocker.patch( + "services.user_restrictions.search_user_restriction_service.UserRestrictionDynamoService", + ) + mocker.patch( + "services.user_restrictions.search_user_restriction_service.get_pds_service", + ) + mocker.patch( + "services.user_restrictions.search_user_restriction_service.get_healthcare_worker_api_service", + ) + service = SearchUserRestrictionService() + yield service + + +def test_process_request_calls_query_restrictions_and_enriches(mock_service, mocker): + mock_query = mocker.patch.object( + mock_service.dynamo_service, + "query_restrictions", + return_value=([MOCK_RESTRICTION], None), + ) + mocker.patch.object(mock_service, "_enrich_restriction", return_value=MOCK_RESPONSE) + + results, next_token = mock_service.process_request(ods_code=TEST_ODS_CODE) + + mock_query.assert_called_once_with( + ods_code=TEST_ODS_CODE, + smart_card_id=None, + nhs_number=None, + limit=DEFAULT_LIMIT, + start_key=None, + ) + assert len(results) == 1 + assert next_token is None + + +def test_process_request_passes_next_page_token_as_start_key(mock_service, mocker): + mock_query = mocker.patch.object( + mock_service.dynamo_service, + "query_restrictions", + return_value=([MOCK_RESTRICTION], None), + ) + mocker.patch.object(mock_service, "_enrich_restriction", return_value=MOCK_RESPONSE) + + mock_service.process_request( + ods_code=TEST_ODS_CODE, + next_page_token=TEST_NEXT_TOKEN, + ) + + mock_query.assert_called_once_with( + ods_code=TEST_ODS_CODE, + smart_card_id=None, + nhs_number=None, + limit=DEFAULT_LIMIT, + start_key=TEST_NEXT_TOKEN, + ) + + +def test_process_request_returns_next_token_from_dynamo(mock_service, mocker): + mocker.patch.object( + mock_service.dynamo_service, + "query_restrictions", + return_value=([MOCK_RESTRICTION], TEST_NEXT_TOKEN), + ) + mocker.patch.object(mock_service, "_enrich_restriction", return_value=MOCK_RESPONSE) + + _, next_token = mock_service.process_request(ods_code=TEST_ODS_CODE) + + assert next_token == TEST_NEXT_TOKEN + + +def test_process_request_raises_on_client_error(mock_service, mocker): + mocker.patch.object( + mock_service.dynamo_service, + "query_restrictions", + side_effect=ClientError( + {"Error": {"Code": "500", "Message": "DynamoDB error"}}, + "query", + ), + ) + + with pytest.raises(SearchUserRestrictionException) as exc_info: + mock_service.process_request(ods_code=TEST_ODS_CODE) + + assert exc_info.value.status_code == 500 + assert ( + exc_info.value.err_code == LambdaError.SearchUserRestrictionDB.value["err_code"] + ) + + +def test_process_request_raises_500_on_validation_exception(mock_service, mocker): + mocker.patch.object( + mock_service.dynamo_service, + "query_restrictions", + side_effect=UserRestrictionValidationException("bad data"), + ) + + with pytest.raises(SearchUserRestrictionException) as exc_info: + mock_service.process_request(ods_code=TEST_ODS_CODE) + + assert exc_info.value.status_code == 500 + assert ( + exc_info.value.err_code == LambdaError.SearchUserRestrictionDB.value["err_code"] + ) + + +def test_process_request_raises_400_on_invalid_limit(mock_service): + with pytest.raises(SearchUserRestrictionException) as exc_info: + mock_service.process_request(ods_code=TEST_ODS_CODE, limit="not-a-number") + + assert exc_info.value.status_code == 400 + assert ( + exc_info.value.err_code + == LambdaError.SearchUserRestrictionInvalidQueryString.value["err_code"] + ) + + +def test_process_request_raises_400_on_invalid_next_page_token(mock_service): + with pytest.raises(SearchUserRestrictionException) as exc_info: + mock_service.process_request( + ods_code=TEST_ODS_CODE, + next_page_token="!!!not-valid-base64!!!", + ) + + assert exc_info.value.status_code == 400 + assert ( + exc_info.value.err_code + == LambdaError.SearchUserRestrictionInvalidQueryString.value["err_code"] + ) + + +def test_validate_limit_raises_for_non_digit(mock_service): + with pytest.raises(UserRestrictionException): + mock_service.validate_limit("abc") + + +def test_validate_limit_raises_for_zero(mock_service): + with pytest.raises(UserRestrictionException): + mock_service.validate_limit(0) + + +def test_validate_limit_raises_for_over_max(mock_service): + with pytest.raises(UserRestrictionException): + mock_service.validate_limit(MAX_LIMIT + 1) + + +def test_validate_limit_accepts_max_limit(mock_service): + assert mock_service.validate_limit(MAX_LIMIT) == MAX_LIMIT + + +def test_validate_limit_accepts_string_digit(mock_service): + assert mock_service.validate_limit("5") == 5 + + +def test_validate_limit_accepts_one(mock_service): + assert mock_service.validate_limit(1) == 1 + + +def test_enrich_with_patient_name_sets_name_on_success(mock_service): + mock_service.pds_service.fetch_patient_details.return_value.given_name = ["Jane"] + mock_service.pds_service.fetch_patient_details.return_value.family_name = "Doe" + + result = mock_service._enrich_with_patient_name(MOCK_RESPONSE.model_copy()) + + assert result.patient_given_name == ["Jane"] + assert result.patient_family_name == "Doe" + + +def test_enrich_with_patient_name_uses_cache_on_second_call(mock_service): + mock_service.pds_service.fetch_patient_details.return_value.given_name = ["Jane"] + mock_service.pds_service.fetch_patient_details.return_value.family_name = "Doe" + + mock_service._enrich_with_patient_name(MOCK_RESPONSE.model_copy()) + mock_service._enrich_with_patient_name(MOCK_RESPONSE.model_copy()) + + mock_service.pds_service.fetch_patient_details.assert_called_once() + + +@pytest.mark.parametrize("exception", [PatientNotFoundException, PdsErrorException]) +def test_enrich_with_patient_name_logs_warning_on_pds_failure(mock_service, exception): + mock_service.pds_service.fetch_patient_details.side_effect = exception("error") + + result = mock_service._enrich_with_patient_name(MOCK_RESPONSE.model_copy()) + + assert result.patient_given_name is None + assert result.patient_family_name is None + + +def test_enrich_with_patient_name_caches_failed_lookup(mock_service): + mock_service.pds_service.fetch_patient_details.side_effect = ( + PatientNotFoundException("error") + ) + + mock_service._enrich_with_patient_name(MOCK_RESPONSE.model_copy()) + mock_service._enrich_with_patient_name(MOCK_RESPONSE.model_copy()) + + mock_service.pds_service.fetch_patient_details.assert_called_once() + + +def test_enrich_with_restricted_user_name_sets_name_on_success(mock_service): + mock_service.hwc_service.get_practitioner.return_value = MOCK_PRACTITIONER + + result = mock_service._enrich_with_restricted_user_name(MOCK_RESPONSE.model_copy()) + + assert result.restricted_user_first_name == "Jane" + assert result.restricted_user_last_name == "Doe" + + +def test_enrich_with_restricted_user_name_uses_cache_on_second_call(mock_service): + mock_service.hwc_service.get_practitioner.return_value = MOCK_PRACTITIONER + + mock_service._enrich_with_restricted_user_name(MOCK_RESPONSE.model_copy()) + mock_service._enrich_with_restricted_user_name(MOCK_RESPONSE.model_copy()) + + mock_service.hwc_service.get_practitioner.assert_called_once() + + +def test_enrich_with_restricted_user_name_logs_warning_on_hwc_failure(mock_service): + mock_service.hwc_service.get_practitioner.side_effect = ( + HealthcareWorkerAPIException(status_code=404) + ) + + result = mock_service._enrich_with_restricted_user_name(MOCK_RESPONSE.model_copy()) + + assert result.restricted_user_first_name is None + assert result.restricted_user_last_name is None + + +def test_enrich_with_restricted_user_name_caches_failed_lookup(mock_service): + mock_service.hwc_service.get_practitioner.side_effect = ( + HealthcareWorkerAPIException(status_code=404) + ) + + mock_service._enrich_with_restricted_user_name(MOCK_RESPONSE.model_copy()) + mock_service._enrich_with_restricted_user_name(MOCK_RESPONSE.model_copy()) + + mock_service.hwc_service.get_practitioner.assert_called_once() diff --git a/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py b/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py new file mode 100644 index 0000000000..1db6cd17ed --- /dev/null +++ b/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py @@ -0,0 +1,130 @@ +import pytest + +from services.user_restrictions.user_restriction_dynamo_service import ( + NHS_NUMBER_KEY, + ODS_CODE_GSI, + ODS_CODE_KEY, + SMARTCARD_KEY, + UserRestrictionDynamoService, +) +from tests.unit.conftest import TEST_NHS_NUMBER, TEST_UUID +from utils.exceptions import UserRestrictionValidationException + +TEST_ODS_CODE = "Y12345" +TEST_SMART_CARD_ID = "SC001" +MOCK_USER_RESTRICTION_TABLE = "test_user_restriction_table" +TEST_NEXT_TOKEN = "some-opaque-next-token" + +MOCK_RESTRICTION = { + "ID": TEST_UUID, + "RestrictedSmartcard": TEST_SMART_CARD_ID, + "NhsNumber": TEST_NHS_NUMBER, + "Custodian": TEST_ODS_CODE, + "Created": 1700000000, + "CreatorSmartcard": "SC002", + "RemoverSmartCard": None, + "IsActive": True, + "LastUpdated": 1700000001, +} + + +@pytest.fixture +def mock_service(set_env, mocker): + mocker.patch( + "services.user_restrictions.user_restriction_dynamo_service.DynamoDBService", + ) + service = UserRestrictionDynamoService() + service.dynamo_service.query_table_with_paginator.return_value = {"Items": []} + yield service + + +def test_query_restrictions_calls_paginator_with_correct_key_and_index(mock_service): + mock_service.query_restrictions(ods_code=TEST_ODS_CODE) + + call_kwargs = ( + mock_service.dynamo_service.query_table_with_paginator.call_args.kwargs + ) + assert call_kwargs["key"] == ODS_CODE_KEY + assert call_kwargs["condition"] == TEST_ODS_CODE + assert call_kwargs["index_name"] == ODS_CODE_GSI + + +def test_query_restrictions_by_ods_code_uses_active_filter(mock_service): + mock_service.query_restrictions(ods_code=TEST_ODS_CODE) + + call_kwargs = ( + mock_service.dynamo_service.query_table_with_paginator.call_args.kwargs + ) + assert "IsActive" in call_kwargs["filter_expression"] + assert SMARTCARD_KEY not in call_kwargs["filter_expression"] + assert NHS_NUMBER_KEY not in call_kwargs["filter_expression"] + + +def test_query_restrictions_by_smart_card_id_applies_smartcard_filter(mock_service): + mock_service.query_restrictions( + ods_code=TEST_ODS_CODE, + smart_card_id=TEST_SMART_CARD_ID, + ) + + call_kwargs = ( + mock_service.dynamo_service.query_table_with_paginator.call_args.kwargs + ) + assert "IsActive" in call_kwargs["filter_expression"] + assert SMARTCARD_KEY in call_kwargs["filter_expression"] + assert ( + call_kwargs["expression_attribute_values"][":RestrictedSmartcard_condition_val"] + == TEST_SMART_CARD_ID + ) + + +def test_query_restrictions_by_nhs_number_applies_nhs_number_filter(mock_service): + mock_service.query_restrictions(ods_code=TEST_ODS_CODE, nhs_number=TEST_NHS_NUMBER) + + call_kwargs = ( + mock_service.dynamo_service.query_table_with_paginator.call_args.kwargs + ) + assert "IsActive" in call_kwargs["filter_expression"] + assert NHS_NUMBER_KEY in call_kwargs["filter_expression"] + assert ( + call_kwargs["expression_attribute_values"][":NhsNumber_condition_val"] + == TEST_NHS_NUMBER + ) + + +def test_query_restrictions_passes_limit_and_start_key(mock_service): + mock_service.query_restrictions( + ods_code=TEST_ODS_CODE, + limit=5, + start_key=TEST_NEXT_TOKEN, + ) + + call_kwargs = ( + mock_service.dynamo_service.query_table_with_paginator.call_args.kwargs + ) + assert call_kwargs["limit"] == 5 + assert call_kwargs["start_key"] == TEST_NEXT_TOKEN + + +def test_query_restrictions_returns_next_token(mock_service): + mock_service.dynamo_service.query_table_with_paginator.return_value = { + "Items": [MOCK_RESTRICTION], + "NextToken": TEST_NEXT_TOKEN, + } + + _, next_token = mock_service.query_restrictions(ods_code=TEST_ODS_CODE) + + assert next_token == TEST_NEXT_TOKEN + + +def test_query_restrictions_returns_empty_list_when_no_items(mock_service): + mock_service.dynamo_service.query_table_with_paginator.return_value = {"Items": []} + + results, next_token = mock_service.query_restrictions(ods_code=TEST_ODS_CODE) + + assert results == [] + assert next_token is None + + +def test_validate_restrictions_raises_for_invalid_items(): + with pytest.raises(UserRestrictionValidationException): + UserRestrictionDynamoService._validate_restrictions([{"invalid": "data"}]) diff --git a/lambdas/utils/exceptions.py b/lambdas/utils/exceptions.py index ec2eabe48a..abdb9c8eaf 100644 --- a/lambdas/utils/exceptions.py +++ b/lambdas/utils/exceptions.py @@ -233,3 +233,11 @@ def __init__(self, message: str, segment_id: str): def to_dict(self): return {"segmentId": self.segment_id, "message": self.message} + + +class UserRestrictionException(Exception): + pass + + +class UserRestrictionValidationException(Exception): + pass diff --git a/lambdas/utils/lambda_exceptions.py b/lambdas/utils/lambda_exceptions.py index 2399eb640a..aaadadcf8c 100644 --- a/lambdas/utils/lambda_exceptions.py +++ b/lambdas/utils/lambda_exceptions.py @@ -132,3 +132,7 @@ class UpdateDocumentReviewException(LambdaException): class ReportDistributionException(LambdaException): pass + + +class SearchUserRestrictionException(LambdaException): + pass diff --git a/lambdas/utils/ods_utils.py b/lambdas/utils/ods_utils.py index 4d99cf4377..ae1fa13f9b 100644 --- a/lambdas/utils/ods_utils.py +++ b/lambdas/utils/ods_utils.py @@ -28,7 +28,7 @@ def extract_ods_role_code_with_r_prefix_from_role_codes_string(role_codes) -> st def extract_ods_code_from_request_context() -> str: try: ods_code = request_context.authorization.get("selected_organisation", {}).get( - "org_ods_code" + "org_ods_code", ) if not ods_code: raise OdsErrorException() @@ -36,4 +36,4 @@ def extract_ods_code_from_request_context() -> str: return ods_code except AttributeError: - raise OdsErrorException() + raise OdsErrorException("No ODS code found in request context") diff --git a/lambdas/utils/pagination_utils.py b/lambdas/utils/pagination_utils.py new file mode 100644 index 0000000000..71c527ca2c --- /dev/null +++ b/lambdas/utils/pagination_utils.py @@ -0,0 +1,19 @@ +import base64 +import json + +from utils.audit_logging_setup import LoggingService +from utils.exceptions import InvalidParamException + +logger = LoggingService(__name__) + + +def validate_next_page_token(next_page_token: str | None) -> None: + """Raises InvalidParamException if the token is not valid base64-encoded JSON.""" + if not next_page_token: + return + try: + decoded = base64.b64decode(next_page_token.encode("utf-8")).decode("utf-8") + json.loads(decoded) + except Exception as e: + logger.error(f"Invalid next_page_token: {e}") + raise InvalidParamException(f"Invalid next_page_token: {e}") from e From ece638062a4a7e7a6b564f2e918308e47b4a1eaa Mon Sep 17 00:00:00 2001 From: NogaNHS <127490765+NogaNHS@users.noreply.github.com> Date: Wed, 4 Mar 2026 16:55:37 +0000 Subject: [PATCH 02/17] Refactor search user restriction tests to use props for exclusion and update expected results --- ...does_not_return_inactive_restrictions.json | 18 +-------- ...ser_restriction_filters_by_nhs_number.json | 28 +------------- ...r_restriction_filters_by_smartcard_id.json | 15 +------- ...ates_with_limit_and_next_page_token.1.json | 13 +------ ...riction_returns_200_with_restrictions.json | 38 +------------------ .../api/test_search_user_restriction_api.py | 20 ++++------ 6 files changed, 14 insertions(+), 118 deletions(-) diff --git a/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_does_not_return_inactive_restrictions.json b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_does_not_return_inactive_restrictions.json index 13605469c0..1d0a16b21b 100644 --- a/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_does_not_return_inactive_restrictions.json +++ b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_does_not_return_inactive_restrictions.json @@ -1,18 +1,4 @@ { - "count": 1, - "restrictions": [ - { - "created": 1741392000, - "id": "a1b2c3d4-0003-4000-8000-000000000003", - "nhsNumber": "9449305943", - "patientFamilyName": "schauffer", - "patientGivenName": [ - "goodwin", - "harley" - ], - "restrictedUser": "123456789012", - "restrictedUserFirstName": "John", - "restrictedUserLastName": "Doe" - } - ] + "count": 0, + "restrictions": [] } diff --git a/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_filters_by_nhs_number.json b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_filters_by_nhs_number.json index f21a1e7112..1f6bc812b6 100644 --- a/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_filters_by_nhs_number.json +++ b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_filters_by_nhs_number.json @@ -1,5 +1,5 @@ { - "count": 3, + "count": 1, "restrictions": [ { "created": 1700000000, @@ -12,32 +12,6 @@ "restrictedUser": "123456789012", "restrictedUserFirstName": "John", "restrictedUserLastName": "Doe" - }, - { - "created": 1740787200, - "id": "a1b2c3d4-0001-4000-8000-000000000001", - "nhsNumber": "9449305943", - "patientFamilyName": "schauffer", - "patientGivenName": [ - "goodwin", - "harley" - ], - "restrictedUser": "777777777777", - "restrictedUserFirstName": "Jane Amy Sheila Claire", - "restrictedUserLastName": "Doe" - }, - { - "created": 1741392000, - "id": "a1b2c3d4-0003-4000-8000-000000000003", - "nhsNumber": "9449305943", - "patientFamilyName": "schauffer", - "patientGivenName": [ - "goodwin", - "harley" - ], - "restrictedUser": "123456789012", - "restrictedUserFirstName": "John", - "restrictedUserLastName": "Doe" } ] } diff --git a/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_filters_by_smartcard_id.json b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_filters_by_smartcard_id.json index d2cfaa82bc..1f6bc812b6 100644 --- a/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_filters_by_smartcard_id.json +++ b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_filters_by_smartcard_id.json @@ -1,5 +1,5 @@ { - "count": 2, + "count": 1, "restrictions": [ { "created": 1700000000, @@ -12,19 +12,6 @@ "restrictedUser": "123456789012", "restrictedUserFirstName": "John", "restrictedUserLastName": "Doe" - }, - { - "created": 1741392000, - "id": "a1b2c3d4-0003-4000-8000-000000000003", - "nhsNumber": "9449305943", - "patientFamilyName": "schauffer", - "patientGivenName": [ - "goodwin", - "harley" - ], - "restrictedUser": "123456789012", - "restrictedUserFirstName": "John", - "restrictedUserLastName": "Doe" } ] } diff --git a/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_paginates_with_limit_and_next_page_token.1.json b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_paginates_with_limit_and_next_page_token.1.json index 9acd644a69..1f6bc812b6 100644 --- a/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_paginates_with_limit_and_next_page_token.1.json +++ b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_paginates_with_limit_and_next_page_token.1.json @@ -1,6 +1,5 @@ { - "count": 2, - "nextPageToken": "eyJFeGNsdXNpdmVTdGFydEtleSI6IHsiQ3JlYXRlZCI6IHsiTiI6ICIxNzQwNzg3MjAwIn0sICJJRCI6IHsiUyI6ICJhMWIyYzNkNC0wMDAyLTQwMDAtODAwMC0wMDAwMDAwMDAwMDIifSwgIkN1c3RvZGlhbiI6IHsiUyI6ICJIODExMDkifX19", + "count": 1, "restrictions": [ { "created": 1700000000, @@ -13,16 +12,6 @@ "restrictedUser": "123456789012", "restrictedUserFirstName": "John", "restrictedUserLastName": "Doe" - }, - { - "created": 1740787200, - "id": "a1b2c3d4-0002-4000-8000-000000000002", - "nhsNumber": "9730136912", - "patientFamilyName": "BOLTON", - "patientGivenName": [ - "Leah" - ], - "restrictedUser": "323456789033" } ] } diff --git a/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_returns_200_with_restrictions.json b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_returns_200_with_restrictions.json index 26bee5e953..1f6bc812b6 100644 --- a/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_returns_200_with_restrictions.json +++ b/lambdas/tests/e2e/api/__snapshots__/test_search_user_restriction_api/test_search_user_restriction_returns_200_with_restrictions.json @@ -1,5 +1,5 @@ { - "count": 4, + "count": 1, "restrictions": [ { "created": 1700000000, @@ -12,42 +12,6 @@ "restrictedUser": "123456789012", "restrictedUserFirstName": "John", "restrictedUserLastName": "Doe" - }, - { - "created": 1740787200, - "id": "a1b2c3d4-0002-4000-8000-000000000002", - "nhsNumber": "9730136912", - "patientFamilyName": "BOLTON", - "patientGivenName": [ - "Leah" - ], - "restrictedUser": "323456789033" - }, - { - "created": 1740787200, - "id": "a1b2c3d4-0001-4000-8000-000000000001", - "nhsNumber": "9449305943", - "patientFamilyName": "schauffer", - "patientGivenName": [ - "goodwin", - "harley" - ], - "restrictedUser": "777777777777", - "restrictedUserFirstName": "Jane Amy Sheila Claire", - "restrictedUserLastName": "Doe" - }, - { - "created": 1741392000, - "id": "a1b2c3d4-0003-4000-8000-000000000003", - "nhsNumber": "9449305943", - "patientFamilyName": "schauffer", - "patientGivenName": [ - "goodwin", - "harley" - ], - "restrictedUser": "123456789012", - "restrictedUserFirstName": "John", - "restrictedUserLastName": "Doe" } ] } diff --git a/lambdas/tests/e2e/api/test_search_user_restriction_api.py b/lambdas/tests/e2e/api/test_search_user_restriction_api.py index c84247c7c3..67f7c83408 100644 --- a/lambdas/tests/e2e/api/test_search_user_restriction_api.py +++ b/lambdas/tests/e2e/api/test_search_user_restriction_api.py @@ -3,7 +3,7 @@ import pytest import requests -from syrupy.filters import paths +from syrupy.filters import props from tests.e2e.helpers.data_helper import UserRestrictionDataHelper from tests.e2e.helpers.mockcis2_helper import MockCis2Helper @@ -15,6 +15,8 @@ TEST_SMARTCARD_ID = "123456789012" ANOTHER_SMARTCARD_ID = "323456789033" +EXCLUDE_IDS = props("id") + @pytest.fixture(scope="module") def auth_token(): @@ -71,7 +73,7 @@ def test_search_user_restriction_returns_200_with_restrictions( assert response.status_code == 200 body = response.json() - assert body == snapshot_json(exclude=paths("restrictions.0.id")) + assert body == snapshot_json(exclude=EXCLUDE_IDS) def test_search_user_restriction_filters_by_smartcard_id( @@ -98,7 +100,7 @@ def test_search_user_restriction_filters_by_smartcard_id( assert response.status_code == 200 body = response.json() - assert body == snapshot_json(exclude=paths("restrictions.0.id")) + assert body == snapshot_json(exclude=EXCLUDE_IDS) def test_search_user_restriction_filters_by_nhs_number( @@ -128,7 +130,7 @@ def test_search_user_restriction_filters_by_nhs_number( assert response.status_code == 200 body = response.json() - assert body == snapshot_json(exclude=paths("restrictions.0.id")) + assert body == snapshot_json(exclude=EXCLUDE_IDS) def test_search_user_restriction_returns_empty_list_when_no_matches( @@ -192,13 +194,7 @@ def test_search_user_restriction_paginates_with_limit_and_next_page_token( assert response.status_code == 200 body = response.json() - assert body == snapshot_json( - exclude=paths( - "restrictions.0.id", - "restrictions.1.id", - "nextPageToken", - ), - ) + assert body == snapshot_json(exclude=props("id", "nextPageToken")) if "nextPageToken" in body: next_response = requests.get( @@ -208,7 +204,7 @@ def test_search_user_restriction_paginates_with_limit_and_next_page_token( ) assert next_response.status_code == 200 next_body = next_response.json() - assert next_body == snapshot_json(exclude=paths("restrictions.0.id")) + assert next_body == snapshot_json(exclude=EXCLUDE_IDS) def test_search_user_restriction_returns_401_without_auth(snapshot_json): From 31ac7eeb136184930ddf19f21ed9946ab682a4ea Mon Sep 17 00:00:00 2001 From: NogaNHS <127490765+NogaNHS@users.noreply.github.com> Date: Wed, 4 Mar 2026 17:00:14 +0000 Subject: [PATCH 03/17] Refactor search user restriction tests to include mock validation for next page token --- .../test_search_user_restriction_service.py | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/lambdas/tests/unit/services/user_restriction/test_search_user_restriction_service.py b/lambdas/tests/unit/services/user_restriction/test_search_user_restriction_service.py index 3c20b5f15e..dd77ab44ec 100644 --- a/lambdas/tests/unit/services/user_restriction/test_search_user_restriction_service.py +++ b/lambdas/tests/unit/services/user_restriction/test_search_user_restriction_service.py @@ -55,6 +55,9 @@ def mock_service(set_env, mocker): mocker.patch( "services.user_restrictions.search_user_restriction_service.get_healthcare_worker_api_service", ) + mocker.patch( + "services.user_restrictions.search_user_restriction_service.validate_next_page_token", + ) service = SearchUserRestrictionService() yield service @@ -161,20 +164,6 @@ def test_process_request_raises_400_on_invalid_limit(mock_service): ) -def test_process_request_raises_400_on_invalid_next_page_token(mock_service): - with pytest.raises(SearchUserRestrictionException) as exc_info: - mock_service.process_request( - ods_code=TEST_ODS_CODE, - next_page_token="!!!not-valid-base64!!!", - ) - - assert exc_info.value.status_code == 400 - assert ( - exc_info.value.err_code - == LambdaError.SearchUserRestrictionInvalidQueryString.value["err_code"] - ) - - def test_validate_limit_raises_for_non_digit(mock_service): with pytest.raises(UserRestrictionException): mock_service.validate_limit("abc") From 9c1f8e7f4f3a5717e27c287599431a13771bc914 Mon Sep 17 00:00:00 2001 From: NogaNHS <127490765+NogaNHS@users.noreply.github.com> Date: Thu, 5 Mar 2026 11:58:00 +0000 Subject: [PATCH 04/17] [PRMP-1446] addressing Pr comments --- lambdas/enums/lambda_error.py | 11 ++- .../search_user_restriction_handler.py | 27 +++++-- .../user_restriction_response.py | 10 +-- .../user_restrictions/user_restrictions.py | 9 +++ lambdas/services/authoriser_service.py | 7 +- .../search_user_restriction_service.py | 76 ++++++------------- .../user_restriction_dynamo_service.py | 56 ++++++++------ .../files/user_restrictions_seed_data.json | 70 ----------------- .../test_search_user_restriction_handler.py | 63 +++++++-------- .../test_search_user_restriction_service.py | 63 +++++---------- .../test_user_restriction_dynamo_service.py | 51 ++++++++----- 11 files changed, 187 insertions(+), 256 deletions(-) delete mode 100644 lambdas/tests/e2e/api/files/user_restrictions_seed_data.json diff --git a/lambdas/enums/lambda_error.py b/lambdas/enums/lambda_error.py index 3fd5355eae..1ae1addaf5 100644 --- a/lambdas/enums/lambda_error.py +++ b/lambdas/enums/lambda_error.py @@ -748,22 +748,21 @@ def create_error_body( } """ - Errors for SearchUserRestriction lambda + Errors for User Restriction lambdas """ - SearchUserRestrictionMissingODS = { + UserRestrictionMissingODS = { "err_code": "SUR_4001", "message": "No ODS code provided in request context", } - SearchUserRestrictionInvalidQueryString = { + UserRestrictionInvalidQueryString = { "err_code": "SUR_4002", "message": "Invalid query string parameter", } - SearchUserRestrictionDB = { + UserRestrictionDB = { "err_code": "SUR_5001", "message": "Failed to query user restrictions from DynamoDB", } - - SearchUserRestrictionsInvalidQueryParameter = { + UserRestrictionInvalidQueryParameter = { "err_code": "SUR_4003", "message": "Invalid query parameter value: %(details)s", } diff --git a/lambdas/handlers/user_restrictions/search_user_restriction_handler.py b/lambdas/handlers/user_restrictions/search_user_restriction_handler.py index 14053672c1..d9bacc8b99 100644 --- a/lambdas/handlers/user_restrictions/search_user_restriction_handler.py +++ b/lambdas/handlers/user_restrictions/search_user_restriction_handler.py @@ -15,7 +15,12 @@ from utils.decorators.handle_lambda_exceptions import handle_lambda_exceptions from utils.decorators.override_error_check import override_error_check from utils.decorators.set_audit_arg import set_request_context_for_logging -from utils.exceptions import OdsErrorException, UserRestrictionException +from utils.exceptions import ( + InvalidParamException, + OdsErrorException, + UserRestrictionException, + UserRestrictionValidationException, +) from utils.lambda_response import ApiGatewayResponse from utils.ods_utils import extract_ods_code_from_request_context @@ -35,11 +40,11 @@ @handle_lambda_exceptions def lambda_handler(event, _context): try: - ods_code = extract_ods_code_from_request_context() feature_flag_service = FeatureFlagService() feature_flag_service.validate_feature_flag( FeatureFlags.USER_RESTRICTION_ENABLED.value, ) + ods_code = extract_ods_code_from_request_context() params = parse_querystring_parameters(event) @@ -47,7 +52,7 @@ def lambda_handler(event, _context): restrictions, next_token = service.process_request( ods_code=ods_code, - smart_card_id=params.get(UserRestrictionQuerystringParameters.SMARTCARD_ID), + smartcard_id=params.get(UserRestrictionQuerystringParameters.SMARTCARD_ID), nhs_number=params.get(UserRestrictionQuerystringParameters.NHS_NUMBER), next_page_token=params.get( UserRestrictionQuerystringParameters.NEXT_PAGE_TOKEN, @@ -73,15 +78,23 @@ def lambda_handler(event, _context): logger.error(e) return ApiGatewayResponse( status_code=401, - body=LambdaError.SearchUserRestrictionMissingODS.create_error_body(), + body=LambdaError.UserRestrictionMissingODS.create_error_body(), methods="GET", ).create_api_gateway_response() - except UserRestrictionException as e: - logger.error(f"Invalid query parameter value: {e}") + except (UserRestrictionException, InvalidParamException) as e: + logger.error(f"Invalid query parameter: {e}") return ApiGatewayResponse( status_code=400, - body=LambdaError.SearchUserRestrictionsInvalidQueryParameter.create_error_body(), + body=LambdaError.UserRestrictionInvalidQueryParameter.create_error_body(), + methods="GET", + ).create_api_gateway_response() + + except UserRestrictionValidationException as e: + logger.error(f"Restriction model validation error: {e}") + return ApiGatewayResponse( + status_code=500, + body=LambdaError.UserRestrictionDB.create_error_body(), methods="GET", ).create_api_gateway_response() diff --git a/lambdas/models/user_restrictions/user_restriction_response.py b/lambdas/models/user_restrictions/user_restriction_response.py index f913a26ef4..404e20e795 100644 --- a/lambdas/models/user_restrictions/user_restriction_response.py +++ b/lambdas/models/user_restrictions/user_restriction_response.py @@ -1,10 +1,8 @@ -from typing import Optional - from models.user_restrictions.user_restrictions import UserRestriction class UserRestrictionResponse(UserRestriction): - patient_given_name: Optional[list[str]] = None - patient_family_name: Optional[str] = None - restricted_user_first_name: Optional[str] = None - restricted_user_last_name: Optional[str] = None + patient_given_name: list[str] | None = None + patient_family_name: str | None = None + restricted_user_first_name: str | None = None + restricted_user_last_name: str | None = None diff --git a/lambdas/models/user_restrictions/user_restrictions.py b/lambdas/models/user_restrictions/user_restrictions.py index c9e36e0058..521eedddce 100644 --- a/lambdas/models/user_restrictions/user_restrictions.py +++ b/lambdas/models/user_restrictions/user_restrictions.py @@ -11,6 +11,15 @@ class UserRestrictionsFields(StrEnum): CREATOR = "CreatorSmartcard" RESTRICTED_USER = "RestrictedSmartcard" REMOVED_BY = "RemoverSmartcard" + NHS_NUMBER = "NhsNumber" + CUSTODIAN = "Custodian" + IS_ACTIVE = "IsActive" + LAST_UPDATED = "LastUpdated" + CREATED = "Created" + + +class UserRestrictionIndexes(StrEnum): + CUSTODIAN_INDEX = "CustodianIndex" class UserRestriction(BaseModel): diff --git a/lambdas/services/authoriser_service.py b/lambdas/services/authoriser_service.py index b3c8a25177..8f89ec39e8 100644 --- a/lambdas/services/authoriser_service.py +++ b/lambdas/services/authoriser_service.py @@ -150,7 +150,12 @@ def deny_access_policy(self, path, http_verb, user_role, nhs_number: str = None) not patient_access_is_allowed or is_user_gp_clinical or is_user_pcse ) case "/UserRestriction": - deny_resource = False + if http_verb == HttpVerb.POST: + deny_resource = not patient_access_is_allowed + elif http_verb == HttpVerb.GET: + deny_resource = False + else: + deny_resource = True case "/VirusScan": deny_resource = ( diff --git a/lambdas/services/user_restrictions/search_user_restriction_service.py b/lambdas/services/user_restrictions/search_user_restriction_service.py index 2b9d9ddd7e..9cebb4c120 100644 --- a/lambdas/services/user_restrictions/search_user_restriction_service.py +++ b/lambdas/services/user_restrictions/search_user_restriction_service.py @@ -1,6 +1,3 @@ -from botocore.exceptions import ClientError - -from enums.lambda_error import LambdaError from models.user_restrictions.user_restriction_response import UserRestrictionResponse from models.user_restrictions.user_restrictions import UserRestriction from services.user_restrictions.user_restriction_dynamo_service import ( @@ -13,14 +10,11 @@ from utils.exceptions import ( HealthcareWorkerAPIException, HealthcareWorkerPractitionerModelException, - InvalidParamException, InvalidResourceIdException, PatientNotFoundException, PdsErrorException, UserRestrictionException, - UserRestrictionValidationException, ) -from utils.lambda_exceptions import SearchUserRestrictionException from utils.pagination_utils import validate_next_page_token from utils.utilities import get_pds_service @@ -38,58 +32,38 @@ def __init__(self): def process_request( self, ods_code: str, - smart_card_id: str | None = None, + smartcard_id: str | None = None, nhs_number: str | None = None, next_page_token: str | None = None, limit: int | str = DEFAULT_LIMIT, ) -> tuple[list[dict], str | None]: - try: - limit = self.validate_limit(limit) - validate_next_page_token(next_page_token) - - logger.info(f"Querying user restrictions for ODS code {ods_code}") - restrictions, next_token = self.dynamo_service.query_restrictions( - ods_code=ods_code, - smart_card_id=smart_card_id, - nhs_number=nhs_number, - limit=limit, - start_key=next_page_token, - ) - logger.info(f"Found {len(restrictions)} restriction(s)") + limit = self.validate_limit(limit) + validate_next_page_token(next_page_token) + + logger.info(f"Querying user restrictions for ODS code {ods_code}") + restrictions, next_token = self.dynamo_service.query_restrictions( + ods_code=ods_code, + smartcard_id=smartcard_id, + nhs_number=nhs_number, + limit=limit, + start_key=next_page_token, + ) + logger.info(f"Found {len(restrictions)} restriction(s)") - enriched = [ - self._enrich_restriction(restriction) for restriction in restrictions - ] + enriched_response = [ + self._enrich_restriction(restriction) for restriction in restrictions + ] - serialised = [ - item.model_dump_camel_case( - exclude_none=True, - exclude={"last_updated", "is_active", "creator", "custodian"}, - mode="json", - ) - for item in enriched - ] - - return serialised, next_token - - except UserRestrictionValidationException as e: - logger.error(e) - raise SearchUserRestrictionException( - 500, - LambdaError.SearchUserRestrictionDB, - ) from e - except (UserRestrictionException, InvalidParamException) as e: - logger.error(e) - raise SearchUserRestrictionException( - 400, - LambdaError.SearchUserRestrictionInvalidQueryString, - ) from e - except ClientError as e: - logger.error(e) - raise SearchUserRestrictionException( - 500, - LambdaError.SearchUserRestrictionDB, + serialised_response = [ + item.model_dump_camel_case( + exclude_none=True, + exclude={"last_updated", "is_active", "creator", "custodian"}, + mode="json", ) + for item in enriched_response + ] + + return serialised_response, next_token def _enrich_restriction( self, diff --git a/lambdas/services/user_restrictions/user_restriction_dynamo_service.py b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py index 834ecf35d9..d33bc7eb2f 100644 --- a/lambdas/services/user_restrictions/user_restriction_dynamo_service.py +++ b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py @@ -1,9 +1,14 @@ import os +from botocore.exceptions import ClientError from pydantic import ValidationError from enums.dynamo_filter import ConditionOperator -from models.user_restrictions.user_restrictions import UserRestriction +from models.user_restrictions.user_restrictions import ( + UserRestriction, + UserRestrictionIndexes, + UserRestrictionsFields, +) from services.base.dynamo_service import DynamoDBService from utils.audit_logging_setup import LoggingService from utils.dynamo_utils import build_mixed_condition_expression @@ -14,11 +19,6 @@ DEFAULT_LIMIT = 10 MAX_LIMIT = 100 -SMARTCARD_KEY = "RestrictedSmartcard" -NHS_NUMBER_KEY = "NhsNumber" -ODS_CODE_GSI = "CustodianIndex" -ODS_CODE_KEY = "Custodian" - class UserRestrictionDynamoService: def __init__(self): @@ -28,29 +28,35 @@ def __init__(self): def query_restrictions( self, ods_code: str, - smart_card_id: str | None = None, + smartcard_id: str | None = None, nhs_number: str | None = None, limit: int = DEFAULT_LIMIT, start_key: str | None = None, ) -> tuple[list[UserRestriction], str | None]: filter_expression, expression_attribute_names, expression_attribute_values = ( self._build_query_filter( - smart_card_id=smart_card_id, + smartcard_id=smartcard_id, nhs_number=nhs_number, ) ) - response = self.dynamo_service.query_table_with_paginator( - table_name=self.table_name, - index_name=ODS_CODE_GSI, - key=ODS_CODE_KEY, - condition=ods_code, - filter_expression=filter_expression, - expression_attribute_names=expression_attribute_names, - expression_attribute_values=expression_attribute_values, - limit=limit, - start_key=start_key, - ) + try: + response = self.dynamo_service.query_table_with_paginator( + table_name=self.table_name, + index_name=UserRestrictionIndexes.CUSTODIAN_INDEX, + key=UserRestrictionsFields.CUSTODIAN, + condition=ods_code, + filter_expression=filter_expression, + expression_attribute_names=expression_attribute_names, + expression_attribute_values=expression_attribute_values, + limit=limit, + start_key=start_key, + ) + except ClientError as e: + logger.error(f"DynamoDB ClientError when querying restrictions: {e}") + raise UserRestrictionValidationException( + f"Failed to query user restrictions from DynamoDB: {e}", + ) from e items = response.get("Items", []) restrictions = self._validate_restrictions(items) @@ -60,28 +66,28 @@ def query_restrictions( @staticmethod def _build_query_filter( - smart_card_id: str | None, + smartcard_id: str | None, nhs_number: str | None, ) -> tuple[str, dict, dict]: conditions = [ { - "field": "IsActive", + "field": UserRestrictionsFields.IS_ACTIVE, "operator": ConditionOperator.EQUAL.value, "value": True, }, ] - if smart_card_id: + if smartcard_id: conditions.append( { - "field": SMARTCARD_KEY, + "field": UserRestrictionsFields.RESTRICTED_USER, "operator": ConditionOperator.EQUAL.value, - "value": smart_card_id, + "value": smartcard_id, }, ) if nhs_number: conditions.append( { - "field": NHS_NUMBER_KEY, + "field": UserRestrictionsFields.NHS_NUMBER, "operator": ConditionOperator.EQUAL.value, "value": nhs_number, }, diff --git a/lambdas/tests/e2e/api/files/user_restrictions_seed_data.json b/lambdas/tests/e2e/api/files/user_restrictions_seed_data.json deleted file mode 100644 index 6f81448267..0000000000 --- a/lambdas/tests/e2e/api/files/user_restrictions_seed_data.json +++ /dev/null @@ -1,70 +0,0 @@ -[ - { - "ID": "a1b2c3d4-0001-4000-8000-000000000001", - "RestrictedSmartcard": "SC001", - "NhsNumber": "9449305943", - "Custodian": "H81109", - "CreatorSmartcard": "SC099", - "RemoverSmartcard": null, - "IsActive": true, - "Created": 1740787200, - "LastUpdated": 1740787200 - }, - { - "ID": "a1b2c3d4-0002-4000-8000-000000000002", - "RestrictedSmartcard": "SC001", - "NhsNumber": "9730136912", - "Custodian": "H81109", - "CreatorSmartcard": "SC099", - "RemoverSmartcard": null, - "IsActive": true, - "Created": 1740787200, - "LastUpdated": 1740787200 - }, - { - "ID": "a1b2c3d4-0003-4000-8000-000000000003", - "RestrictedSmartcard": "SC002", - "NhsNumber": "9449305943", - "Custodian": "H81109", - "CreatorSmartcard": "SC099", - "RemoverSmartcard": null, - "IsActive": true, - "Created": 1741392000, - "LastUpdated": 1741392000 - }, - { - "ID": "a1b2c3d4-0004-4000-8000-000000000004", - "RestrictedSmartcard": "SC002", - "NhsNumber": "9730153973", - "Custodian": "H81109", - "CreatorSmartcard": "SC099", - "RemoverSmartcard": null, - "IsActive": true, - "Created": 1741392000, - "LastUpdated": 1741392000 - }, - { - "ID": "a1b2c3d4-0005-4000-8000-000000000005", - "RestrictedSmartcard": "SC003", - "NhsNumber": "9449305943", - "Custodian": "H81109", - "CreatorSmartcard": "SC099", - "RemoverSmartcard": "SC099", - "IsActive": false, - "Created": 1738368000, - "LastUpdated": 1740873600 - }, - { - "ID": "a1b2c3d4-0006-4000-8000-000000000006", - "RestrictedSmartcard": "SC001", - "NhsNumber": "9449305943", - "Custodian": "Y12345", - "CreatorSmartcard": "SC088", - "RemoverSmartcard": null, - "IsActive": true, - "Created": 1740960000, - "LastUpdated": 1740960000 - } -] - - diff --git a/lambdas/tests/unit/handlers/test_search_user_restriction_handler.py b/lambdas/tests/unit/handlers/test_search_user_restriction_handler.py index b18210195e..f03d2a057c 100644 --- a/lambdas/tests/unit/handlers/test_search_user_restriction_handler.py +++ b/lambdas/tests/unit/handlers/test_search_user_restriction_handler.py @@ -9,16 +9,21 @@ lambda_handler, parse_querystring_parameters, ) -from tests.unit.conftest import MOCK_INTERACTION_ID, TEST_NHS_NUMBER, TEST_UUID -from utils.exceptions import OdsErrorException -from utils.lambda_exceptions import ( - FeatureFlagsException, - SearchUserRestrictionException, +from tests.unit.conftest import ( + MOCK_INTERACTION_ID, + TEST_CURRENT_GP_ODS, + TEST_NHS_NUMBER, + TEST_UUID, ) +from utils.exceptions import ( + OdsErrorException, + UserRestrictionException, + UserRestrictionValidationException, +) +from utils.lambda_exceptions import FeatureFlagsException from utils.lambda_response import ApiGatewayResponse -TEST_ODS_CODE = "Y12345" -TEST_SMART_CARD_ID = "SC001" +TEST_SMARTCARD_ID = "123456789012" TEST_LAST_EVALUATED_KEY = {"ID": TEST_UUID} TEST_ENCODED_START_KEY = base64.b64encode( @@ -27,9 +32,9 @@ MOCK_RESTRICTION = { "id": TEST_UUID, - "restrictedSmartcard": TEST_SMART_CARD_ID, + "restrictedSmartcard": TEST_SMARTCARD_ID, "nhsNumber": TEST_NHS_NUMBER, - "custodian": TEST_ODS_CODE, + "custodian": TEST_CURRENT_GP_ODS, "isActive": True, } @@ -61,7 +66,7 @@ def mocked_extract_ods(mocker): mock = mocker.patch( "handlers.user_restrictions.search_user_restriction_handler.extract_ods_code_from_request_context", ) - mock.return_value = TEST_ODS_CODE + mock.return_value = TEST_CURRENT_GP_ODS yield mock @@ -86,7 +91,7 @@ def base_event(set_env): @pytest.fixture def event_with_smart_card(base_event): event = base_event.copy() - event["queryStringParameters"] = {"smartcardId": TEST_SMART_CARD_ID} + event["queryStringParameters"] = {"smartcardId": TEST_SMARTCARD_ID} return event @@ -118,9 +123,9 @@ def test_parse_querystring_parameters_returns_empty_dict_for_none(base_event): assert parse_querystring_parameters(base_event) == {} -def test_parse_querystring_parameters_extracts_smart_card_id(event_with_smart_card): +def test_parse_querystring_parameters_extracts_smartcard_id(event_with_smart_card): result = parse_querystring_parameters(event_with_smart_card) - assert result == {"smartcardId": TEST_SMART_CARD_ID} + assert result == {"smartcardId": TEST_SMARTCARD_ID} def test_parse_querystring_parameters_extracts_nhs_number(event_with_nhs_number): @@ -162,8 +167,8 @@ def test_handler_returns_200_with_restrictions_list( assert actual == expected mock_service.process_request.assert_called_once_with( - ods_code=TEST_ODS_CODE, - smart_card_id=None, + ods_code=TEST_CURRENT_GP_ODS, + smartcard_id=None, nhs_number=None, next_page_token=None, limit=10, @@ -203,7 +208,7 @@ def test_handler_returns_200_with_empty_list_when_no_results( assert body["count"] == 0 -def test_handler_calls_service_with_smart_card_id( +def test_handler_calls_service_with_smartcard_id( event_with_smart_card, mock_service, mocked_extract_ods, @@ -212,8 +217,8 @@ def test_handler_calls_service_with_smart_card_id( lambda_handler(event_with_smart_card, context) mock_service.process_request.assert_called_once_with( - ods_code=TEST_ODS_CODE, - smart_card_id=TEST_SMART_CARD_ID, + ods_code=TEST_CURRENT_GP_ODS, + smartcard_id=TEST_SMARTCARD_ID, nhs_number=None, next_page_token=None, limit=10, @@ -229,8 +234,8 @@ def test_handler_calls_service_with_nhs_number( lambda_handler(event_with_nhs_number, context) mock_service.process_request.assert_called_once_with( - ods_code=TEST_ODS_CODE, - smart_card_id=None, + ods_code=TEST_CURRENT_GP_ODS, + smartcard_id=None, nhs_number=TEST_NHS_NUMBER, next_page_token=None, limit=10, @@ -246,8 +251,8 @@ def test_handler_passes_limit_and_token_to_service( lambda_handler(event_with_limit_and_token, context) mock_service.process_request.assert_called_once_with( - ods_code=TEST_ODS_CODE, - smart_card_id=None, + ods_code=TEST_CURRENT_GP_ODS, + smartcard_id=None, nhs_number=None, next_page_token=TEST_ENCODED_START_KEY, limit="5", @@ -262,8 +267,8 @@ def test_handler_returns_401_when_no_ods_code_in_request_context( ): expected_body = json.dumps( { - "message": LambdaError.SearchUserRestrictionMissingODS.value["message"], - "err_code": LambdaError.SearchUserRestrictionMissingODS.value["err_code"], + "message": LambdaError.UserRestrictionMissingODS.value["message"], + "err_code": LambdaError.UserRestrictionMissingODS.value["err_code"], "interaction_id": MOCK_INTERACTION_ID, }, ) @@ -284,10 +289,7 @@ def test_handler_returns_400_when_service_raises_invalid_query_string( mocked_extract_ods, context, ): - mock_service.process_request.side_effect = SearchUserRestrictionException( - 400, - LambdaError.SearchUserRestrictionInvalidQueryString, - ) + mock_service.process_request.side_effect = UserRestrictionException("bad param") actual = lambda_handler(base_event, context) @@ -300,9 +302,8 @@ def test_handler_returns_500_when_service_raises_db_error( mocked_extract_ods, context, ): - mock_service.process_request.side_effect = SearchUserRestrictionException( - 500, - LambdaError.SearchUserRestrictionDB, + mock_service.process_request.side_effect = UserRestrictionValidationException( + "DynamoDB error", ) actual = lambda_handler(base_event, context) diff --git a/lambdas/tests/unit/services/user_restriction/test_search_user_restriction_service.py b/lambdas/tests/unit/services/user_restriction/test_search_user_restriction_service.py index dd77ab44ec..14e98d6049 100644 --- a/lambdas/tests/unit/services/user_restriction/test_search_user_restriction_service.py +++ b/lambdas/tests/unit/services/user_restriction/test_search_user_restriction_service.py @@ -1,7 +1,5 @@ import pytest -from botocore.exceptions import ClientError -from enums.lambda_error import LambdaError from models.user_restrictions.practitioner import Practitioner from models.user_restrictions.user_restriction_response import UserRestrictionResponse from models.user_restrictions.user_restrictions import UserRestriction @@ -10,7 +8,7 @@ SearchUserRestrictionService, ) from services.user_restrictions.user_restriction_dynamo_service import DEFAULT_LIMIT -from tests.unit.conftest import TEST_NHS_NUMBER, TEST_UUID +from tests.unit.conftest import TEST_CURRENT_GP_ODS, TEST_NHS_NUMBER, TEST_UUID from utils.exceptions import ( HealthcareWorkerAPIException, PatientNotFoundException, @@ -18,17 +16,15 @@ UserRestrictionException, UserRestrictionValidationException, ) -from utils.lambda_exceptions import SearchUserRestrictionException -TEST_ODS_CODE = "Y12345" -TEST_SMART_CARD_ID = "SC001" -TEST_NEXT_TOKEN = "some-opaque-next-token" +TEST_SMARTCARD_ID = "555021541101" +TEST_NEXT_TOKEN = "some-next-token" MOCK_RESTRICTION_DICT = { "ID": TEST_UUID, - "RestrictedSmartcard": TEST_SMART_CARD_ID, + "RestrictedSmartcard": TEST_SMARTCARD_ID, "NhsNumber": TEST_NHS_NUMBER, - "Custodian": TEST_ODS_CODE, + "Custodian": TEST_CURRENT_GP_ODS, "Created": 1700000000, "CreatorSmartcard": "SC002", "IsActive": True, @@ -38,7 +34,7 @@ MOCK_RESPONSE = UserRestrictionResponse.model_validate(MOCK_RESTRICTION_DICT) MOCK_PRACTITIONER = Practitioner( - smartcard_id=TEST_SMART_CARD_ID, + smartcard_id=TEST_SMARTCARD_ID, first_name="Jane", last_name="Doe", ) @@ -70,11 +66,11 @@ def test_process_request_calls_query_restrictions_and_enriches(mock_service, moc ) mocker.patch.object(mock_service, "_enrich_restriction", return_value=MOCK_RESPONSE) - results, next_token = mock_service.process_request(ods_code=TEST_ODS_CODE) + results, next_token = mock_service.process_request(ods_code=TEST_CURRENT_GP_ODS) mock_query.assert_called_once_with( - ods_code=TEST_ODS_CODE, - smart_card_id=None, + ods_code=TEST_CURRENT_GP_ODS, + smartcard_id=None, nhs_number=None, limit=DEFAULT_LIMIT, start_key=None, @@ -92,13 +88,13 @@ def test_process_request_passes_next_page_token_as_start_key(mock_service, mocke mocker.patch.object(mock_service, "_enrich_restriction", return_value=MOCK_RESPONSE) mock_service.process_request( - ods_code=TEST_ODS_CODE, + ods_code=TEST_CURRENT_GP_ODS, next_page_token=TEST_NEXT_TOKEN, ) mock_query.assert_called_once_with( - ods_code=TEST_ODS_CODE, - smart_card_id=None, + ods_code=TEST_CURRENT_GP_ODS, + smartcard_id=None, nhs_number=None, limit=DEFAULT_LIMIT, start_key=TEST_NEXT_TOKEN, @@ -113,7 +109,7 @@ def test_process_request_returns_next_token_from_dynamo(mock_service, mocker): ) mocker.patch.object(mock_service, "_enrich_restriction", return_value=MOCK_RESPONSE) - _, next_token = mock_service.process_request(ods_code=TEST_ODS_CODE) + _, next_token = mock_service.process_request(ods_code=TEST_CURRENT_GP_ODS) assert next_token == TEST_NEXT_TOKEN @@ -122,19 +118,11 @@ def test_process_request_raises_on_client_error(mock_service, mocker): mocker.patch.object( mock_service.dynamo_service, "query_restrictions", - side_effect=ClientError( - {"Error": {"Code": "500", "Message": "DynamoDB error"}}, - "query", - ), + side_effect=UserRestrictionValidationException("DynamoDB error"), ) - with pytest.raises(SearchUserRestrictionException) as exc_info: - mock_service.process_request(ods_code=TEST_ODS_CODE) - - assert exc_info.value.status_code == 500 - assert ( - exc_info.value.err_code == LambdaError.SearchUserRestrictionDB.value["err_code"] - ) + with pytest.raises(UserRestrictionValidationException): + mock_service.process_request(ods_code=TEST_CURRENT_GP_ODS) def test_process_request_raises_500_on_validation_exception(mock_service, mocker): @@ -144,24 +132,15 @@ def test_process_request_raises_500_on_validation_exception(mock_service, mocker side_effect=UserRestrictionValidationException("bad data"), ) - with pytest.raises(SearchUserRestrictionException) as exc_info: - mock_service.process_request(ods_code=TEST_ODS_CODE) - - assert exc_info.value.status_code == 500 - assert ( - exc_info.value.err_code == LambdaError.SearchUserRestrictionDB.value["err_code"] - ) + with pytest.raises(UserRestrictionValidationException): + mock_service.process_request(ods_code=TEST_CURRENT_GP_ODS) def test_process_request_raises_400_on_invalid_limit(mock_service): - with pytest.raises(SearchUserRestrictionException) as exc_info: - mock_service.process_request(ods_code=TEST_ODS_CODE, limit="not-a-number") + with pytest.raises(UserRestrictionException): + mock_service.process_request(ods_code=TEST_CURRENT_GP_ODS, limit="not-a-number") - assert exc_info.value.status_code == 400 - assert ( - exc_info.value.err_code - == LambdaError.SearchUserRestrictionInvalidQueryString.value["err_code"] - ) + assert True # no status_code to check — handler owns that mapping def test_validate_limit_raises_for_non_digit(mock_service): diff --git a/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py b/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py index 1db6cd17ed..bf04362375 100644 --- a/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py +++ b/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py @@ -1,10 +1,11 @@ import pytest +from botocore.exceptions import ClientError +from models.user_restrictions.user_restrictions import ( + UserRestrictionIndexes, + UserRestrictionsFields, +) from services.user_restrictions.user_restriction_dynamo_service import ( - NHS_NUMBER_KEY, - ODS_CODE_GSI, - ODS_CODE_KEY, - SMARTCARD_KEY, UserRestrictionDynamoService, ) from tests.unit.conftest import TEST_NHS_NUMBER, TEST_UUID @@ -44,9 +45,9 @@ def test_query_restrictions_calls_paginator_with_correct_key_and_index(mock_serv call_kwargs = ( mock_service.dynamo_service.query_table_with_paginator.call_args.kwargs ) - assert call_kwargs["key"] == ODS_CODE_KEY + assert call_kwargs["key"] == UserRestrictionsFields.CUSTODIAN assert call_kwargs["condition"] == TEST_ODS_CODE - assert call_kwargs["index_name"] == ODS_CODE_GSI + assert call_kwargs["index_name"] == UserRestrictionIndexes.CUSTODIAN_INDEX def test_query_restrictions_by_ods_code_uses_active_filter(mock_service): @@ -55,24 +56,28 @@ def test_query_restrictions_by_ods_code_uses_active_filter(mock_service): call_kwargs = ( mock_service.dynamo_service.query_table_with_paginator.call_args.kwargs ) - assert "IsActive" in call_kwargs["filter_expression"] - assert SMARTCARD_KEY not in call_kwargs["filter_expression"] - assert NHS_NUMBER_KEY not in call_kwargs["filter_expression"] + assert UserRestrictionsFields.IS_ACTIVE in call_kwargs["filter_expression"] + assert ( + UserRestrictionsFields.RESTRICTED_USER not in call_kwargs["filter_expression"] + ) + assert UserRestrictionsFields.NHS_NUMBER not in call_kwargs["filter_expression"] -def test_query_restrictions_by_smart_card_id_applies_smartcard_filter(mock_service): +def test_query_restrictions_by_smartcard_id_applies_smartcard_filter(mock_service): mock_service.query_restrictions( ods_code=TEST_ODS_CODE, - smart_card_id=TEST_SMART_CARD_ID, + smartcard_id=TEST_SMART_CARD_ID, ) call_kwargs = ( mock_service.dynamo_service.query_table_with_paginator.call_args.kwargs ) - assert "IsActive" in call_kwargs["filter_expression"] - assert SMARTCARD_KEY in call_kwargs["filter_expression"] + assert UserRestrictionsFields.IS_ACTIVE in call_kwargs["filter_expression"] + assert UserRestrictionsFields.RESTRICTED_USER in call_kwargs["filter_expression"] assert ( - call_kwargs["expression_attribute_values"][":RestrictedSmartcard_condition_val"] + call_kwargs["expression_attribute_values"][ + f":{UserRestrictionsFields.RESTRICTED_USER}_condition_val" + ] == TEST_SMART_CARD_ID ) @@ -83,10 +88,12 @@ def test_query_restrictions_by_nhs_number_applies_nhs_number_filter(mock_service call_kwargs = ( mock_service.dynamo_service.query_table_with_paginator.call_args.kwargs ) - assert "IsActive" in call_kwargs["filter_expression"] - assert NHS_NUMBER_KEY in call_kwargs["filter_expression"] + assert UserRestrictionsFields.IS_ACTIVE in call_kwargs["filter_expression"] + assert UserRestrictionsFields.NHS_NUMBER in call_kwargs["filter_expression"] assert ( - call_kwargs["expression_attribute_values"][":NhsNumber_condition_val"] + call_kwargs["expression_attribute_values"][ + f":{UserRestrictionsFields.NHS_NUMBER}_condition_val" + ] == TEST_NHS_NUMBER ) @@ -128,3 +135,13 @@ def test_query_restrictions_returns_empty_list_when_no_items(mock_service): def test_validate_restrictions_raises_for_invalid_items(): with pytest.raises(UserRestrictionValidationException): UserRestrictionDynamoService._validate_restrictions([{"invalid": "data"}]) + + +def test_query_restrictions_raises_validation_exception_on_client_error(mock_service): + mock_service.dynamo_service.query_table_with_paginator.side_effect = ClientError( + {"Error": {"Code": "500", "Message": "DynamoDB error"}}, + "query", + ) + + with pytest.raises(UserRestrictionValidationException): + mock_service.query_restrictions(ods_code=TEST_ODS_CODE) From 1d1056ead7686232455f3f662599546c0319e234 Mon Sep 17 00:00:00 2001 From: NogaNHS <127490765+NogaNHS@users.noreply.github.com> Date: Mon, 9 Mar 2026 10:41:15 +0000 Subject: [PATCH 05/17] Add test fixtures and update event handling for user restriction queries --- .../search_user_restriction_handler.py | 1 + lambdas/tests/unit/conftest.py | 2 + .../test_search_user_restriction_handler.py | 84 +++++++++---------- .../test_user_restriction_dynamo_service.py | 38 +++++---- 4 files changed, 66 insertions(+), 59 deletions(-) diff --git a/lambdas/handlers/user_restrictions/search_user_restriction_handler.py b/lambdas/handlers/user_restrictions/search_user_restriction_handler.py index d9bacc8b99..c8f1479d08 100644 --- a/lambdas/handlers/user_restrictions/search_user_restriction_handler.py +++ b/lambdas/handlers/user_restrictions/search_user_restriction_handler.py @@ -34,6 +34,7 @@ "APPCONFIG_APPLICATION", "APPCONFIG_CONFIGURATION", "APPCONFIG_ENVIRONMENT", + "HEALTHCARE_WORKER_API_URL", ], ) @override_error_check diff --git a/lambdas/tests/unit/conftest.py b/lambdas/tests/unit/conftest.py index 153dd3f8bb..6b2a737055 100644 --- a/lambdas/tests/unit/conftest.py +++ b/lambdas/tests/unit/conftest.py @@ -85,6 +85,8 @@ REVIEW_SQS_QUEUE_URL = "test_review_queue" TEST_NHS_NUMBER = "9000000009" TEST_UUID = "1234-4567-8912-HSDF-TEST" +TEST_SMART_CARD_ID = "123456789120" +TEST_NEXT_PAGE_TOKEN = "some-next-token" TEST_FILE_KEY = "test_file_key" TEST_FILE_NAME = "test.pdf" TEST_FILE_SIZE = 24000 diff --git a/lambdas/tests/unit/handlers/test_search_user_restriction_handler.py b/lambdas/tests/unit/handlers/test_search_user_restriction_handler.py index f03d2a057c..eb111eabea 100644 --- a/lambdas/tests/unit/handlers/test_search_user_restriction_handler.py +++ b/lambdas/tests/unit/handlers/test_search_user_restriction_handler.py @@ -38,8 +38,6 @@ "isActive": True, } -MOCK_USER_RESTRICTION_TABLE = "test_user_restriction_table" - @pytest.fixture def mock_feature_flag_service(mocker): @@ -80,47 +78,38 @@ def mocked_extract_ods_error(mocker): @pytest.fixture -def base_event(set_env): - return { - "httpMethod": "GET", - "queryStringParameters": None, - "headers": {"Authorization": "test_token"}, - } - - -@pytest.fixture -def event_with_smart_card(base_event): - event = base_event.copy() - event["queryStringParameters"] = {"smartcardId": TEST_SMARTCARD_ID} - return event +def event_with_smart_card(event): + updated = event.copy() + updated["queryStringParameters"] = {"smartcardId": TEST_SMARTCARD_ID} + return updated @pytest.fixture -def event_with_nhs_number(base_event): - event = base_event.copy() - event["queryStringParameters"] = {"nhsNumber": TEST_NHS_NUMBER} - return event +def event_with_nhs_number(event): + updated = event.copy() + updated["queryStringParameters"] = {"nhsNumber": TEST_NHS_NUMBER} + return updated @pytest.fixture -def event_with_limit_and_token(base_event): - event = base_event.copy() - event["queryStringParameters"] = { +def event_with_limit_and_token(event): + updated = event.copy() + updated["queryStringParameters"] = { "limit": "5", "nextPageToken": TEST_ENCODED_START_KEY, } - return event + return updated @pytest.fixture -def event_with_unknown_params(base_event): - event = base_event.copy() - event["queryStringParameters"] = {"unknown_param": "value"} - return event +def event_with_unknown_params(event): + updated = event.copy() + updated["queryStringParameters"] = {"unknown_param": "value"} + return updated -def test_parse_querystring_parameters_returns_empty_dict_for_none(base_event): - assert parse_querystring_parameters(base_event) == {} +def test_parse_querystring_parameters_returns_empty_dict_for_none(event): + assert parse_querystring_parameters(event) == {} def test_parse_querystring_parameters_extracts_smartcard_id(event_with_smart_card): @@ -151,7 +140,8 @@ def test_parse_querystring_parameters_ignores_unknown_params( def test_handler_returns_200_with_restrictions_list( - base_event, + event, + set_env, mock_service, mocked_extract_ods, context, @@ -163,7 +153,7 @@ def test_handler_returns_200_with_restrictions_list( methods="GET", ).create_api_gateway_response() - actual = lambda_handler(base_event, context) + actual = lambda_handler(event, context) assert actual == expected mock_service.process_request.assert_called_once_with( @@ -176,7 +166,8 @@ def test_handler_returns_200_with_restrictions_list( def test_handler_includes_next_token_when_present( - base_event, + event, + set_env, mock_service, mocked_extract_ods, context, @@ -186,21 +177,22 @@ def test_handler_includes_next_token_when_present( TEST_ENCODED_START_KEY, ) - actual = lambda_handler(base_event, context) + actual = lambda_handler(event, context) body = json.loads(actual["body"]) assert body["nextPageToken"] == TEST_ENCODED_START_KEY def test_handler_returns_200_with_empty_list_when_no_results( - base_event, + event, + set_env, mock_service, mocked_extract_ods, context, ): mock_service.process_request.return_value = ([], None) - actual = lambda_handler(base_event, context) + actual = lambda_handler(event, context) assert actual["statusCode"] == 200 body = json.loads(actual["body"]) @@ -210,6 +202,7 @@ def test_handler_returns_200_with_empty_list_when_no_results( def test_handler_calls_service_with_smartcard_id( event_with_smart_card, + set_env, mock_service, mocked_extract_ods, context, @@ -227,6 +220,7 @@ def test_handler_calls_service_with_smartcard_id( def test_handler_calls_service_with_nhs_number( event_with_nhs_number, + set_env, mock_service, mocked_extract_ods, context, @@ -244,6 +238,7 @@ def test_handler_calls_service_with_nhs_number( def test_handler_passes_limit_and_token_to_service( event_with_limit_and_token, + set_env, mock_service, mocked_extract_ods, context, @@ -260,7 +255,8 @@ def test_handler_passes_limit_and_token_to_service( def test_handler_returns_401_when_no_ods_code_in_request_context( - base_event, + event, + set_env, mock_service, mocked_extract_ods_error, context, @@ -278,26 +274,27 @@ def test_handler_returns_401_when_no_ods_code_in_request_context( methods="GET", ).create_api_gateway_response() - actual = lambda_handler(base_event, context) + actual = lambda_handler(event, context) assert actual == expected def test_handler_returns_400_when_service_raises_invalid_query_string( - base_event, + event, + set_env, mock_service, mocked_extract_ods, context, ): mock_service.process_request.side_effect = UserRestrictionException("bad param") - actual = lambda_handler(base_event, context) + actual = lambda_handler(event, context) assert actual["statusCode"] == 400 def test_handler_returns_500_when_service_raises_db_error( - base_event, + event, mock_service, mocked_extract_ods, context, @@ -306,13 +303,14 @@ def test_handler_returns_500_when_service_raises_db_error( "DynamoDB error", ) - actual = lambda_handler(base_event, context) + actual = lambda_handler(event, context) assert actual["statusCode"] == 500 def test_handler_returns_404_when_feature_flag_disabled( - base_event, + event, + set_env, mock_service, mock_feature_flag_service, mocked_extract_ods, @@ -323,7 +321,7 @@ def test_handler_returns_404_when_feature_flag_disabled( LambdaError.FeatureFlagDisabled, ) - actual = lambda_handler(base_event, context) + actual = lambda_handler(event, context) assert actual["statusCode"] == 404 mock_feature_flag_service.validate_feature_flag.assert_called_once_with( diff --git a/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py b/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py index bf04362375..24db16917c 100644 --- a/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py +++ b/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py @@ -1,5 +1,8 @@ import pytest from botocore.exceptions import ClientError +from unit.services.user_restriction.test_search_user_restriction_service import ( + TEST_NEXT_TOKEN, +) from models.user_restrictions.user_restrictions import ( UserRestrictionIndexes, @@ -8,19 +11,19 @@ from services.user_restrictions.user_restriction_dynamo_service import ( UserRestrictionDynamoService, ) -from tests.unit.conftest import TEST_NHS_NUMBER, TEST_UUID +from tests.unit.conftest import ( + TEST_CURRENT_GP_ODS, + TEST_NHS_NUMBER, + TEST_SMART_CARD_ID, + TEST_UUID, +) from utils.exceptions import UserRestrictionValidationException -TEST_ODS_CODE = "Y12345" -TEST_SMART_CARD_ID = "SC001" -MOCK_USER_RESTRICTION_TABLE = "test_user_restriction_table" -TEST_NEXT_TOKEN = "some-opaque-next-token" - MOCK_RESTRICTION = { "ID": TEST_UUID, "RestrictedSmartcard": TEST_SMART_CARD_ID, "NhsNumber": TEST_NHS_NUMBER, - "Custodian": TEST_ODS_CODE, + "Custodian": TEST_CURRENT_GP_ODS, "Created": 1700000000, "CreatorSmartcard": "SC002", "RemoverSmartCard": None, @@ -40,18 +43,18 @@ def mock_service(set_env, mocker): def test_query_restrictions_calls_paginator_with_correct_key_and_index(mock_service): - mock_service.query_restrictions(ods_code=TEST_ODS_CODE) + mock_service.query_restrictions(ods_code=TEST_CURRENT_GP_ODS) call_kwargs = ( mock_service.dynamo_service.query_table_with_paginator.call_args.kwargs ) assert call_kwargs["key"] == UserRestrictionsFields.CUSTODIAN - assert call_kwargs["condition"] == TEST_ODS_CODE + assert call_kwargs["condition"] == TEST_CURRENT_GP_ODS assert call_kwargs["index_name"] == UserRestrictionIndexes.CUSTODIAN_INDEX def test_query_restrictions_by_ods_code_uses_active_filter(mock_service): - mock_service.query_restrictions(ods_code=TEST_ODS_CODE) + mock_service.query_restrictions(ods_code=TEST_CURRENT_GP_ODS) call_kwargs = ( mock_service.dynamo_service.query_table_with_paginator.call_args.kwargs @@ -65,7 +68,7 @@ def test_query_restrictions_by_ods_code_uses_active_filter(mock_service): def test_query_restrictions_by_smartcard_id_applies_smartcard_filter(mock_service): mock_service.query_restrictions( - ods_code=TEST_ODS_CODE, + ods_code=TEST_CURRENT_GP_ODS, smartcard_id=TEST_SMART_CARD_ID, ) @@ -83,7 +86,10 @@ def test_query_restrictions_by_smartcard_id_applies_smartcard_filter(mock_servic def test_query_restrictions_by_nhs_number_applies_nhs_number_filter(mock_service): - mock_service.query_restrictions(ods_code=TEST_ODS_CODE, nhs_number=TEST_NHS_NUMBER) + mock_service.query_restrictions( + ods_code=TEST_CURRENT_GP_ODS, + nhs_number=TEST_NHS_NUMBER, + ) call_kwargs = ( mock_service.dynamo_service.query_table_with_paginator.call_args.kwargs @@ -100,7 +106,7 @@ def test_query_restrictions_by_nhs_number_applies_nhs_number_filter(mock_service def test_query_restrictions_passes_limit_and_start_key(mock_service): mock_service.query_restrictions( - ods_code=TEST_ODS_CODE, + ods_code=TEST_CURRENT_GP_ODS, limit=5, start_key=TEST_NEXT_TOKEN, ) @@ -118,7 +124,7 @@ def test_query_restrictions_returns_next_token(mock_service): "NextToken": TEST_NEXT_TOKEN, } - _, next_token = mock_service.query_restrictions(ods_code=TEST_ODS_CODE) + _, next_token = mock_service.query_restrictions(ods_code=TEST_CURRENT_GP_ODS) assert next_token == TEST_NEXT_TOKEN @@ -126,7 +132,7 @@ def test_query_restrictions_returns_next_token(mock_service): def test_query_restrictions_returns_empty_list_when_no_items(mock_service): mock_service.dynamo_service.query_table_with_paginator.return_value = {"Items": []} - results, next_token = mock_service.query_restrictions(ods_code=TEST_ODS_CODE) + results, next_token = mock_service.query_restrictions(ods_code=TEST_CURRENT_GP_ODS) assert results == [] assert next_token is None @@ -144,4 +150,4 @@ def test_query_restrictions_raises_validation_exception_on_client_error(mock_ser ) with pytest.raises(UserRestrictionValidationException): - mock_service.query_restrictions(ods_code=TEST_ODS_CODE) + mock_service.query_restrictions(ods_code=TEST_CURRENT_GP_ODS) From eabee0626cea75ae78cdda04ada785a55c1860c2 Mon Sep 17 00:00:00 2001 From: NogaNHS <127490765+NogaNHS@users.noreply.github.com> Date: Mon, 9 Mar 2026 10:54:14 +0000 Subject: [PATCH 06/17] [PRMP-1446] Refactor user restriction tests to use consistent next page token variable --- .../test_search_user_restriction_service.py | 23 +++++++++++-------- .../test_user_restriction_dynamo_service.py | 12 ++++------ 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/lambdas/tests/unit/services/user_restriction/test_search_user_restriction_service.py b/lambdas/tests/unit/services/user_restriction/test_search_user_restriction_service.py index 14e98d6049..8a72909258 100644 --- a/lambdas/tests/unit/services/user_restriction/test_search_user_restriction_service.py +++ b/lambdas/tests/unit/services/user_restriction/test_search_user_restriction_service.py @@ -8,7 +8,13 @@ SearchUserRestrictionService, ) from services.user_restrictions.user_restriction_dynamo_service import DEFAULT_LIMIT -from tests.unit.conftest import TEST_CURRENT_GP_ODS, TEST_NHS_NUMBER, TEST_UUID +from tests.unit.conftest import ( + TEST_CURRENT_GP_ODS, + TEST_NEXT_PAGE_TOKEN, + TEST_NHS_NUMBER, + TEST_SMART_CARD_ID, + TEST_UUID, +) from utils.exceptions import ( HealthcareWorkerAPIException, PatientNotFoundException, @@ -17,12 +23,9 @@ UserRestrictionValidationException, ) -TEST_SMARTCARD_ID = "555021541101" -TEST_NEXT_TOKEN = "some-next-token" - MOCK_RESTRICTION_DICT = { "ID": TEST_UUID, - "RestrictedSmartcard": TEST_SMARTCARD_ID, + "RestrictedSmartcard": TEST_SMART_CARD_ID, "NhsNumber": TEST_NHS_NUMBER, "Custodian": TEST_CURRENT_GP_ODS, "Created": 1700000000, @@ -34,7 +37,7 @@ MOCK_RESPONSE = UserRestrictionResponse.model_validate(MOCK_RESTRICTION_DICT) MOCK_PRACTITIONER = Practitioner( - smartcard_id=TEST_SMARTCARD_ID, + smartcard_id=TEST_SMART_CARD_ID, first_name="Jane", last_name="Doe", ) @@ -89,7 +92,7 @@ def test_process_request_passes_next_page_token_as_start_key(mock_service, mocke mock_service.process_request( ods_code=TEST_CURRENT_GP_ODS, - next_page_token=TEST_NEXT_TOKEN, + next_page_token=TEST_NEXT_PAGE_TOKEN, ) mock_query.assert_called_once_with( @@ -97,7 +100,7 @@ def test_process_request_passes_next_page_token_as_start_key(mock_service, mocke smartcard_id=None, nhs_number=None, limit=DEFAULT_LIMIT, - start_key=TEST_NEXT_TOKEN, + start_key=TEST_NEXT_PAGE_TOKEN, ) @@ -105,13 +108,13 @@ def test_process_request_returns_next_token_from_dynamo(mock_service, mocker): mocker.patch.object( mock_service.dynamo_service, "query_restrictions", - return_value=([MOCK_RESTRICTION], TEST_NEXT_TOKEN), + return_value=([MOCK_RESTRICTION], TEST_NEXT_PAGE_TOKEN), ) mocker.patch.object(mock_service, "_enrich_restriction", return_value=MOCK_RESPONSE) _, next_token = mock_service.process_request(ods_code=TEST_CURRENT_GP_ODS) - assert next_token == TEST_NEXT_TOKEN + assert next_token == TEST_NEXT_PAGE_TOKEN def test_process_request_raises_on_client_error(mock_service, mocker): diff --git a/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py b/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py index 24db16917c..7308df12bb 100644 --- a/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py +++ b/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py @@ -1,8 +1,5 @@ import pytest from botocore.exceptions import ClientError -from unit.services.user_restriction.test_search_user_restriction_service import ( - TEST_NEXT_TOKEN, -) from models.user_restrictions.user_restrictions import ( UserRestrictionIndexes, @@ -13,6 +10,7 @@ ) from tests.unit.conftest import ( TEST_CURRENT_GP_ODS, + TEST_NEXT_PAGE_TOKEN, TEST_NHS_NUMBER, TEST_SMART_CARD_ID, TEST_UUID, @@ -108,25 +106,25 @@ def test_query_restrictions_passes_limit_and_start_key(mock_service): mock_service.query_restrictions( ods_code=TEST_CURRENT_GP_ODS, limit=5, - start_key=TEST_NEXT_TOKEN, + start_key=TEST_NEXT_PAGE_TOKEN, ) call_kwargs = ( mock_service.dynamo_service.query_table_with_paginator.call_args.kwargs ) assert call_kwargs["limit"] == 5 - assert call_kwargs["start_key"] == TEST_NEXT_TOKEN + assert call_kwargs["start_key"] == TEST_NEXT_PAGE_TOKEN def test_query_restrictions_returns_next_token(mock_service): mock_service.dynamo_service.query_table_with_paginator.return_value = { "Items": [MOCK_RESTRICTION], - "NextToken": TEST_NEXT_TOKEN, + "NextToken": TEST_NEXT_PAGE_TOKEN, } _, next_token = mock_service.query_restrictions(ods_code=TEST_CURRENT_GP_ODS) - assert next_token == TEST_NEXT_TOKEN + assert next_token == TEST_NEXT_PAGE_TOKEN def test_query_restrictions_returns_empty_list_when_no_items(mock_service): From e5b6226dd5a16a36c553ebca7f065e125401c779 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Mon, 9 Mar 2026 16:13:51 +0000 Subject: [PATCH 07/17] [PRMP-1476] update restrictions table on MNS notification --- lambdas/handlers/mns_notification_handler.py | 3 +- .../user_restrictions/user_restrictions.py | 4 + .../services/process_mns_message_service.py | 21 +- .../user_restriction_dynamo_service.py | 206 ++++++++++++++++++ lambdas/tests/unit/conftest.py | 3 +- .../handlers/user_restrictions/conftest.py | 24 ++ .../test_process_mns_message_service.py | 56 ++++- lambdas/utils/exceptions.py | 7 + 8 files changed, 320 insertions(+), 4 deletions(-) create mode 100644 lambdas/services/user_restrictions/user_restriction_dynamo_service.py create mode 100644 lambdas/tests/unit/handlers/user_restrictions/conftest.py diff --git a/lambdas/handlers/mns_notification_handler.py b/lambdas/handlers/mns_notification_handler.py index ffc8df2a83..3b2aec16d0 100644 --- a/lambdas/handlers/mns_notification_handler.py +++ b/lambdas/handlers/mns_notification_handler.py @@ -21,7 +21,8 @@ "LLOYD_GEORGE_DYNAMODB_NAME", "DOCUMENT_REVIEW_DYNAMODB_NAME", "MNS_NOTIFICATION_QUEUE_URL", - ] + "RESTRICTIONS_TABLE_NAME", + ], ) @override_error_check def lambda_handler(event, context): diff --git a/lambdas/models/user_restrictions/user_restrictions.py b/lambdas/models/user_restrictions/user_restrictions.py index 375a1af8fc..edd69c3d3a 100644 --- a/lambdas/models/user_restrictions/user_restrictions.py +++ b/lambdas/models/user_restrictions/user_restrictions.py @@ -11,6 +11,10 @@ class UserRestrictionsFields(StrEnum): CREATOR = "CreatorSmartcard" RESTRICTED_USER = "RestrictedSmartcard" REMOVED_BY = "RemoverSmartCard" + IS_ACTIVE = "IsActive" + LAST_UPDATED = "LastUpdated" + NHS_NUMBER = "NhsNumber" + CUSTODIAN = "Custodian" class UserRestriction(BaseModel): diff --git a/lambdas/services/process_mns_message_service.py b/lambdas/services/process_mns_message_service.py index 96e647195e..66fc845a74 100644 --- a/lambdas/services/process_mns_message_service.py +++ b/lambdas/services/process_mns_message_service.py @@ -11,6 +11,7 @@ from services.document_reference_service import DocumentReferenceService from services.document_upload_review_service import DocumentUploadReviewService from services.feature_flags_service import FeatureFlagService +from services.user_restrictions.user_restriction_dynamo_service import UserRestrictionDynamoService from utils.audit_logging_setup import LoggingService from utils.exceptions import PdsErrorException from utils.utilities import get_pds_service @@ -26,6 +27,7 @@ def __init__(self): self.sqs_service = SQSService() self.queue = os.getenv("MNS_NOTIFICATION_QUEUE_URL") self.feature_flag_service = FeatureFlagService() + self.restrictions_dynamo_service = UserRestrictionDynamoService() def handle_mns_notification(self, message: MNSSQSMessage): try: @@ -62,10 +64,12 @@ def handle_gp_change_notification(self, message: MNSSQSMessage) -> None: updated_ods_code = self.get_updated_gp_ods(message.subject.nhs_number) self.update_all_patient_documents( - lg_documents, review_documents, updated_ods_code + lg_documents, review_documents, updated_ods_code, ) logger.info("Update complete for change of GP") + self.update_restrictions(nhs_number=message.subject.nhs_number, custodian=updated_ods_code) + def handle_death_notification(self, message: MNSSQSMessage) -> None: death_notification_type = message.data["deathNotificationStatus"] nhs_number = message.subject.nhs_number @@ -88,6 +92,8 @@ def handle_death_notification(self, message: MNSSQSMessage) -> None: ) logger.info("Update complete for death notification change.") + self.update_restrictions(nhs_number=nhs_number, custodian=updated_ods_code) + case DeathNotificationStatus.FORMAL: lg_documents, review_documents = self.get_all_patient_documents( nhs_number @@ -103,6 +109,8 @@ def handle_death_notification(self, message: MNSSQSMessage) -> None: f"Update complete, patient marked {PatientOdsInactiveStatus.DECEASED}." ) + self.update_restrictions(nhs_number=nhs_number, custodian=PatientOdsInactiveStatus.DECEASED) + def get_updated_gp_ods(self, nhs_number: str) -> str: patient_details = self.pds_service.fetch_patient_details(nhs_number) return patient_details.general_practice_ods @@ -139,3 +147,14 @@ def update_all_patient_documents( self.document_review_service.update_document_review_custodian( review_documents, updated_ods_code ) + + def update_restrictions(self, nhs_number: str, custodian: str) -> None: + + restrictions = self.restrictions_dynamo_service.query_restrictions_by_nhs_number(nhs_number=nhs_number) + + for restriction in restrictions: + self.restrictions_dynamo_service.update_restriction_custodian( + restriction_id=restriction.id, updated_custodian=custodian, + ) + + logger.info(f"All restrictions for patient {nhs_number} updated.") diff --git a/lambdas/services/user_restrictions/user_restriction_dynamo_service.py b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py new file mode 100644 index 0000000000..c1d9053a28 --- /dev/null +++ b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py @@ -0,0 +1,206 @@ +import os +from datetime import datetime, timezone +from enum import StrEnum + +from botocore.exceptions import ClientError +from pydantic import ValidationError + +from enums.dynamo_filter import ConditionOperator, AttributeOperator +from models.user_restrictions.user_restrictions import ( + UserRestriction, + UserRestrictionsFields, +) +from services.base.dynamo_service import DynamoDBService +from utils.audit_logging_setup import LoggingService +from utils.dynamo_query_filter_builder import DynamoQueryFilterBuilder +from utils.dynamo_utils import build_mixed_condition_expression +from utils.exceptions import ( + UserRestrictionConditionCheckFailedException, + UserRestrictionValidationException, +) + +logger = LoggingService(__name__) + +DEFAULT_LIMIT = 10 +MAX_LIMIT = 100 + +SMARTCARD_KEY = "RestrictedSmartcard" +NHS_NUMBER_KEY = "NhsNumber" +ODS_CODE_GSI = "CustodianIndex" +ODS_CODE_KEY = "Custodian" +NHS_NUMBER_GSI = "NhsNumberIndex" + + +class DynamoClientErrors(StrEnum): + NOT_FOUND = "ResourceNotFoundException" + CONDITION_CHECK_FAILURE = "ConditionalCheckFailedException" + + +class UserRestrictionDynamoService: + def __init__(self): + self.dynamo_service = DynamoDBService() + self.table_name = os.environ["RESTRICTIONS_TABLE_NAME"] + + def query_restrictions( + self, + ods_code: str, + smart_card_id: str | None = None, + nhs_number: str | None = None, + limit: int = DEFAULT_LIMIT, + start_key: str | None = None, + ) -> tuple[list[UserRestriction], str | None]: + filter_expression, expression_attribute_names, expression_attribute_values = ( + self._build_query_filter( + smart_card_id=smart_card_id, + nhs_number=nhs_number, + ) + ) + + response = self.dynamo_service.query_table_with_paginator( + table_name=self.table_name, + index_name=ODS_CODE_GSI, + key=ODS_CODE_KEY, + condition=ods_code, + filter_expression=filter_expression, + expression_attribute_names=expression_attribute_names, + expression_attribute_values=expression_attribute_values, + limit=limit, + start_key=start_key, + ) + + items = response.get("Items", []) + restrictions = self._validate_restrictions(items) + next_token = response.get("NextToken") + + return restrictions, next_token + + def get_restriction_by_smartcard_id( + self, + restriction_id: str | None = None, + ) -> UserRestriction | None: + response = self.dynamo_service.get_item( + table_name=self.table_name, + key={UserRestrictionsFields.ID.value: restriction_id}, + ) + + if "Item" not in response: + return None + + return UserRestriction.model_validate(response["Item"]) + + def update_restriction_inactive( + self, + restriction_id: str, + removed_by: str, + patient_id: str, + ): + try: + logger.info("Updating user restriction inactive.") + current_time = int(datetime.now(timezone.utc).timestamp()) + + updated_fields = { + UserRestrictionsFields.REMOVED_BY.value: removed_by, + UserRestrictionsFields.LAST_UPDATED.value: current_time, + UserRestrictionsFields.IS_ACTIVE.value: False, + } + + self.dynamo_service.update_item( + table_name=self.table_name, + key_pair={UserRestrictionsFields.ID.value: restriction_id}, + updated_fields=updated_fields, + condition_expression=f"{UserRestrictionsFields.IS_ACTIVE.value} = :true " + f"AND {UserRestrictionsFields.RESTRICTED_USER.value} <> :user_id " + f"AND {UserRestrictionsFields.NHS_NUMBER.value} = :patient_id", + expression_attribute_values={ + ":true": True, + ":user_id": removed_by, + ":patient_id": patient_id, + }, + ) + + except ClientError as e: + logger.error(e) + if ( + e.response["Error"]["Code"] + == DynamoClientErrors.CONDITION_CHECK_FAILURE + ): + raise UserRestrictionConditionCheckFailedException() + else: + raise e + + def query_restrictions_by_nhs_number( + self, + nhs_number: str, + ): + filter_builder = DynamoQueryFilterBuilder() + filter_builder.add_condition(UserRestrictionsFields.IS_ACTIVE.value, AttributeOperator.EQUAL, True) + active_filter_expression = filter_builder.build() + + items = self.dynamo_service.query_table( + table_name=self.table_name, + index_name=NHS_NUMBER_GSI, + search_key=nhs_number, + query_filter=active_filter_expression, + ) + + return self._validate_restrictions(items) + + def update_restriction_custodian(self, restriction_id: str, updated_custodian: str): + try: + logger.info(f"Updating custodian for restriction: {restriction_id}") + current_time = int(datetime.now(timezone.utc).timestamp()) + + updated_fields = { + UserRestrictionsFields.LAST_UPDATED.value: current_time, + UserRestrictionsFields.CUSTODIAN.value: updated_custodian, + } + + self.dynamo_service.update_item( + table_name=self.table_name, + key_pair={UserRestrictionsFields.ID.value: restriction_id}, + updated_fields=updated_fields, + ) + + except ClientError as e: + logger.error(e) + raise e + + @staticmethod + def _build_query_filter( + smart_card_id: str | None, + nhs_number: str | None, + ) -> tuple[str, dict, dict]: + conditions = [ + { + "field": "IsActive", + "operator": ConditionOperator.EQUAL.value, + "value": True, + }, + ] + if smart_card_id: + conditions.append( + { + "field": SMARTCARD_KEY, + "operator": ConditionOperator.EQUAL.value, + "value": smart_card_id, + }, + ) + if nhs_number: + conditions.append( + { + "field": NHS_NUMBER_KEY, + "operator": ConditionOperator.EQUAL.value, + "value": nhs_number, + }, + ) + return build_mixed_condition_expression(conditions) + + @staticmethod + def _validate_restrictions(items: list[dict]) -> list[UserRestriction]: + try: + return [UserRestriction.model_validate(item) for item in items] + except ValidationError as e: + logger.error(e) + raise UserRestrictionValidationException( + f"Failed to validate user restrictions: {e}", + ) from e \ No newline at end of file diff --git a/lambdas/tests/unit/conftest.py b/lambdas/tests/unit/conftest.py index 75fd49077a..b71ab00e01 100644 --- a/lambdas/tests/unit/conftest.py +++ b/lambdas/tests/unit/conftest.py @@ -147,6 +147,7 @@ MOCK_DOCUMENT_REVIEW_TABLE = "test_document_review" MOCK_DOCUMENT_REVIEW_BUCKET = "test_document_review_bucket" MOCK_EDGE_REFERENCE_TABLE = "test_edge_reference_table" +MOCK_USER_RESTRICTION_TABLE = "test_user_restriction_table" @pytest.fixture @@ -255,7 +256,7 @@ def set_env(monkeypatch): monkeypatch.setenv("EDGE_REFERENCE_TABLE", MOCK_EDGE_REFERENCE_TABLE) monkeypatch.setenv("REVIEW_SQS_QUEUE_URL", REVIEW_SQS_QUEUE_URL) monkeypatch.setenv("HEALTHCARE_WORKER_API_URL", HEALTHCARE_WORKER_API_URL) - + monkeypatch.setenv("RESTRICTIONS_TABLE_NAME", MOCK_USER_RESTRICTION_TABLE) EXPECTED_PARSED_PATIENT_BASE_CASE = PatientDetails( givenName=["Jane"], diff --git a/lambdas/tests/unit/handlers/user_restrictions/conftest.py b/lambdas/tests/unit/handlers/user_restrictions/conftest.py new file mode 100644 index 0000000000..5b0bde1966 --- /dev/null +++ b/lambdas/tests/unit/handlers/user_restrictions/conftest.py @@ -0,0 +1,24 @@ +import pytest + +from enums.feature_flags import FeatureFlags +from services.feature_flags_service import FeatureFlagService + +MOCK_SMARTCARD_ID = "123456789012" + + +@pytest.fixture +def mock_user_restriction_enabled(mocker): + mock_function = mocker.patch.object(FeatureFlagService, "get_feature_flags_by_flag") + mock_feature_flag = mock_function.return_value = { + FeatureFlags.USER_RESTRICTION_ENABLED: True, + } + yield mock_feature_flag + + +@pytest.fixture +def mock_user_restriction_disabled(mocker): + mock_function = mocker.patch.object(FeatureFlagService, "get_feature_flags_by_flag") + mock_feature_flag = mock_function.return_value = { + FeatureFlags.USER_RESTRICTION_ENABLED: False, + } + yield mock_feature_flag \ No newline at end of file diff --git a/lambdas/tests/unit/services/test_process_mns_message_service.py b/lambdas/tests/unit/services/test_process_mns_message_service.py index 8bcd0945cd..e6a9bff9a7 100644 --- a/lambdas/tests/unit/services/test_process_mns_message_service.py +++ b/lambdas/tests/unit/services/test_process_mns_message_service.py @@ -1,13 +1,15 @@ from unittest.mock import MagicMock +import freezegun import pytest from botocore.exceptions import ClientError from enums.patient_ods_inactive_status import PatientOdsInactiveStatus from models.document_reference import DocumentReference from models.document_review import DocumentUploadReviewReference from models.sqs.mns_sqs_message import MNSSQSMessage +from models.user_restrictions.user_restrictions import UserRestriction from services.process_mns_message_service import MNSNotificationService -from tests.unit.conftest import TEST_CURRENT_GP_ODS, TEST_NHS_NUMBER +from tests.unit.conftest import TEST_CURRENT_GP_ODS, TEST_NHS_NUMBER, TEST_UUID from tests.unit.handlers.test_mns_notification_handler import ( MOCK_DEATH_MESSAGE_BODY, MOCK_GP_CHANGE_MESSAGE_BODY, @@ -25,6 +27,7 @@ def mns_service(mocker, set_env, monkeypatch): mocker.patch.object(service, "document_review_service") mocker.patch.object(service, "lg_document_service") mocker.patch.object(service, "sqs_service") + mocker.patch.object(service, "restrictions_dynamo_service") yield service @@ -36,6 +39,7 @@ def mns_service_feature_disabled(mocker, set_env, monkeypatch): mocker.patch.object(service, "document_review_service") mocker.patch.object(service, "lg_document_service") mocker.patch.object(service, "sqs_service") + mocker.patch.object(service, "update_restrictions") yield service @@ -498,3 +502,53 @@ def test_handle_death_notification_removed_with_only_review_documents( mns_service.update_all_patient_documents.assert_called_once_with( [], mock_document_review_references, NEW_ODS_CODE ) + + +@pytest.mark.parametrize("mns_event", [removed_death_notification_message, gp_change_message]) +def test_handle_mns_notification_calls_update_restrictions_called_on_notification(mns_service, mns_event, mocker): + mocker.patch.object(mns_service, "update_restrictions") + updated_ods = mocker.patch.object(mns_service, "get_updated_gp_ods", return_value=TEST_CURRENT_GP_ODS) + + mns_service.handle_mns_notification(mns_event) + + mns_service.update_restrictions.assert_called_with(nhs_number=TEST_NHS_NUMBER, custodian=updated_ods.return_value) + + +def test_handle_mns_notification_calls_update_restriction_called_on_death_notification(mns_service, mocker): + mocker.patch.object(mns_service, "update_restrictions") + + mns_service.handle_mns_notification(death_notification_message) + + mns_service.update_restrictions.assert_called_once_with( + nhs_number=TEST_NHS_NUMBER, custodian=PatientOdsInactiveStatus.DECEASED, + ) + +@freezegun.freeze_time("2021-04-01") +def test_update_restrictions_uses_restriction_dynamo_service(mns_service, mocker): + MOCK_RESTRICTION = { + "ID": TEST_UUID, + "RestrictedSmartcard": "123456789012", + "NhsNumber": TEST_NHS_NUMBER, + "Custodian": TEST_CURRENT_GP_ODS, + "Created": 1700000000, + "CreatorSmartcard": "223456789022", + "RemoverSmartCard": None, + "IsActive": True, + "LastUpdated": 1700000001, + } + mocker.patch.object(mns_service, "get_updated_gp_ods", return_value=NEW_ODS_CODE) + + mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.return_value = [ + UserRestriction.model_validate(MOCK_RESTRICTION), + ] + + mns_service.update_restrictions(nhs_number=TEST_NHS_NUMBER, custodian=NEW_ODS_CODE) + + mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.assert_called_with( + nhs_number=TEST_NHS_NUMBER, + ) + + mns_service.restrictions_dynamo_service.update_restriction_custodian.assert_called_once_with( + restriction_id=MOCK_RESTRICTION["ID"], updated_custodian=NEW_ODS_CODE, + ) + diff --git a/lambdas/utils/exceptions.py b/lambdas/utils/exceptions.py index ec2eabe48a..a8dda2b796 100644 --- a/lambdas/utils/exceptions.py +++ b/lambdas/utils/exceptions.py @@ -233,3 +233,10 @@ def __init__(self, message: str, segment_id: str): def to_dict(self): return {"segmentId": self.segment_id, "message": self.message} + + +class UserRestrictionConditionCheckFailedException(Exception): + pass + +class UserRestrictionValidationException(Exception): + pass \ No newline at end of file From ee54a2904bf1f2177df0c6f4e2b30fc01b5636af Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Mon, 9 Mar 2026 16:14:42 +0000 Subject: [PATCH 08/17] [PRMP-1476] format --- lambdas/handlers/mns_notification_handler.py | 5 +- .../services/process_mns_message_service.py | 64 ++++-- .../user_restriction_dynamo_service.py | 14 +- lambdas/tests/unit/conftest.py | 1 + .../handlers/user_restrictions/conftest.py | 2 +- .../test_process_mns_message_service.py | 194 ++++++++++++------ lambdas/utils/exceptions.py | 3 +- 7 files changed, 196 insertions(+), 87 deletions(-) diff --git a/lambdas/handlers/mns_notification_handler.py b/lambdas/handlers/mns_notification_handler.py index 3b2aec16d0..9e4e797ad2 100644 --- a/lambdas/handlers/mns_notification_handler.py +++ b/lambdas/handlers/mns_notification_handler.py @@ -1,8 +1,9 @@ import json +from pydantic import ValidationError + from enums.mns_notification_types import MNSNotificationTypes from models.sqs.mns_sqs_message import MNSSQSMessage -from pydantic import ValidationError from services.process_mns_message_service import MNSNotificationService from utils.audit_logging_setup import LoggingService from utils.decorators.ensure_env_var import ensure_environment_variables @@ -39,7 +40,7 @@ def lambda_handler(event, context): request_context.patient_nhs_no = mns_message.subject.nhs_number logger.info( - f"Processing SQS message for nhs number: {mns_message.subject.nhs_number}" + f"Processing SQS message for nhs number: {mns_message.subject.nhs_number}", ) if mns_message.type in MNSNotificationTypes.__members__.values(): diff --git a/lambdas/services/process_mns_message_service.py b/lambdas/services/process_mns_message_service.py index 66fc845a74..d82dbf98b2 100644 --- a/lambdas/services/process_mns_message_service.py +++ b/lambdas/services/process_mns_message_service.py @@ -1,6 +1,7 @@ import os from botocore.exceptions import ClientError + from enums.death_notification_status import DeathNotificationStatus from enums.mns_notification_types import MNSNotificationTypes from enums.patient_ods_inactive_status import PatientOdsInactiveStatus @@ -11,7 +12,9 @@ from services.document_reference_service import DocumentReferenceService from services.document_upload_review_service import DocumentUploadReviewService from services.feature_flags_service import FeatureFlagService -from services.user_restrictions.user_restriction_dynamo_service import UserRestrictionDynamoService +from services.user_restrictions.user_restriction_dynamo_service import ( + UserRestrictionDynamoService, +) from utils.audit_logging_setup import LoggingService from utils.exceptions import PdsErrorException from utils.utilities import get_pds_service @@ -42,21 +45,21 @@ def handle_mns_notification(self, message: MNSSQSMessage): except PdsErrorException as e: logger.info("An error occurred when calling PDS") logger.info( - f"Unable to process message: {message.id}, of type: {message.type}" + f"Unable to process message: {message.id}, of type: {message.type}", ) logger.info(f"{e}") raise e except ClientError as e: logger.info( - f"Unable to process message: {message.id}, of type: {message.type}" + f"Unable to process message: {message.id}, of type: {message.type}", ) logger.info(f"{e}") raise e def handle_gp_change_notification(self, message: MNSSQSMessage) -> None: lg_documents, review_documents = self.get_all_patient_documents( - message.subject.nhs_number + message.subject.nhs_number, ) if not lg_documents and not review_documents: @@ -64,11 +67,16 @@ def handle_gp_change_notification(self, message: MNSSQSMessage) -> None: updated_ods_code = self.get_updated_gp_ods(message.subject.nhs_number) self.update_all_patient_documents( - lg_documents, review_documents, updated_ods_code, + lg_documents, + review_documents, + updated_ods_code, ) logger.info("Update complete for change of GP") - self.update_restrictions(nhs_number=message.subject.nhs_number, custodian=updated_ods_code) + self.update_restrictions( + nhs_number=message.subject.nhs_number, + custodian=updated_ods_code, + ) def handle_death_notification(self, message: MNSSQSMessage) -> None: death_notification_type = message.data["deathNotificationStatus"] @@ -77,26 +85,31 @@ def handle_death_notification(self, message: MNSSQSMessage) -> None: match death_notification_type: case DeathNotificationStatus.INFORMAL: logger.info( - "Patient is deceased - INFORMAL, moving on to the next message." + "Patient is deceased - INFORMAL, moving on to the next message.", ) case DeathNotificationStatus.REMOVED: lg_documents, review_documents = self.get_all_patient_documents( - nhs_number + nhs_number, ) if lg_documents or review_documents: updated_ods_code = self.get_updated_gp_ods(nhs_number) self.update_all_patient_documents( - lg_documents, review_documents, updated_ods_code + lg_documents, + review_documents, + updated_ods_code, ) logger.info("Update complete for death notification change.") - self.update_restrictions(nhs_number=nhs_number, custodian=updated_ods_code) + self.update_restrictions( + nhs_number=nhs_number, + custodian=updated_ods_code, + ) case DeathNotificationStatus.FORMAL: lg_documents, review_documents = self.get_all_patient_documents( - nhs_number + nhs_number, ) if lg_documents or review_documents: @@ -106,27 +119,31 @@ def handle_death_notification(self, message: MNSSQSMessage) -> None: PatientOdsInactiveStatus.DECEASED, ) logger.info( - f"Update complete, patient marked {PatientOdsInactiveStatus.DECEASED}." + f"Update complete, patient marked {PatientOdsInactiveStatus.DECEASED}.", ) - self.update_restrictions(nhs_number=nhs_number, custodian=PatientOdsInactiveStatus.DECEASED) + self.update_restrictions( + nhs_number=nhs_number, + custodian=PatientOdsInactiveStatus.DECEASED, + ) def get_updated_gp_ods(self, nhs_number: str) -> str: patient_details = self.pds_service.fetch_patient_details(nhs_number) return patient_details.general_practice_ods def get_all_patient_documents( - self, nhs_number: str + self, + nhs_number: str, ) -> tuple[list[DocumentReference], list[DocumentUploadReviewReference]]: """Fetch patient documents from both LG and document review tables.""" lg_documents = ( self.lg_document_service.fetch_documents_from_table_with_nhs_number( - nhs_number + nhs_number, ) ) review_documents = ( self.document_review_service.fetch_documents_from_table_with_nhs_number( - nhs_number + nhs_number, ) ) @@ -141,20 +158,27 @@ def update_all_patient_documents( """Update documents in both tables if they exist.""" if lg_documents: self.lg_document_service.update_patient_ods_code( - lg_documents, updated_ods_code + lg_documents, + updated_ods_code, ) if review_documents: self.document_review_service.update_document_review_custodian( - review_documents, updated_ods_code + review_documents, + updated_ods_code, ) def update_restrictions(self, nhs_number: str, custodian: str) -> None: - restrictions = self.restrictions_dynamo_service.query_restrictions_by_nhs_number(nhs_number=nhs_number) + restrictions = ( + self.restrictions_dynamo_service.query_restrictions_by_nhs_number( + nhs_number=nhs_number, + ) + ) for restriction in restrictions: self.restrictions_dynamo_service.update_restriction_custodian( - restriction_id=restriction.id, updated_custodian=custodian, + restriction_id=restriction.id, + updated_custodian=custodian, ) logger.info(f"All restrictions for patient {nhs_number} updated.") diff --git a/lambdas/services/user_restrictions/user_restriction_dynamo_service.py b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py index c1d9053a28..ab68facb55 100644 --- a/lambdas/services/user_restrictions/user_restriction_dynamo_service.py +++ b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py @@ -5,7 +5,7 @@ from botocore.exceptions import ClientError from pydantic import ValidationError -from enums.dynamo_filter import ConditionOperator, AttributeOperator +from enums.dynamo_filter import AttributeOperator, ConditionOperator from models.user_restrictions.user_restrictions import ( UserRestriction, UserRestrictionsFields, @@ -129,11 +129,15 @@ def update_restriction_inactive( raise e def query_restrictions_by_nhs_number( - self, - nhs_number: str, + self, + nhs_number: str, ): filter_builder = DynamoQueryFilterBuilder() - filter_builder.add_condition(UserRestrictionsFields.IS_ACTIVE.value, AttributeOperator.EQUAL, True) + filter_builder.add_condition( + UserRestrictionsFields.IS_ACTIVE.value, + AttributeOperator.EQUAL, + True, + ) active_filter_expression = filter_builder.build() items = self.dynamo_service.query_table( @@ -203,4 +207,4 @@ def _validate_restrictions(items: list[dict]) -> list[UserRestriction]: logger.error(e) raise UserRestrictionValidationException( f"Failed to validate user restrictions: {e}", - ) from e \ No newline at end of file + ) from e diff --git a/lambdas/tests/unit/conftest.py b/lambdas/tests/unit/conftest.py index b71ab00e01..153dd3f8bb 100644 --- a/lambdas/tests/unit/conftest.py +++ b/lambdas/tests/unit/conftest.py @@ -258,6 +258,7 @@ def set_env(monkeypatch): monkeypatch.setenv("HEALTHCARE_WORKER_API_URL", HEALTHCARE_WORKER_API_URL) monkeypatch.setenv("RESTRICTIONS_TABLE_NAME", MOCK_USER_RESTRICTION_TABLE) + EXPECTED_PARSED_PATIENT_BASE_CASE = PatientDetails( givenName=["Jane"], familyName="Smith", diff --git a/lambdas/tests/unit/handlers/user_restrictions/conftest.py b/lambdas/tests/unit/handlers/user_restrictions/conftest.py index 5b0bde1966..06f4128b76 100644 --- a/lambdas/tests/unit/handlers/user_restrictions/conftest.py +++ b/lambdas/tests/unit/handlers/user_restrictions/conftest.py @@ -21,4 +21,4 @@ def mock_user_restriction_disabled(mocker): mock_feature_flag = mock_function.return_value = { FeatureFlags.USER_RESTRICTION_ENABLED: False, } - yield mock_feature_flag \ No newline at end of file + yield mock_feature_flag diff --git a/lambdas/tests/unit/services/test_process_mns_message_service.py b/lambdas/tests/unit/services/test_process_mns_message_service.py index e6a9bff9a7..d8b48a7b4c 100644 --- a/lambdas/tests/unit/services/test_process_mns_message_service.py +++ b/lambdas/tests/unit/services/test_process_mns_message_service.py @@ -3,6 +3,7 @@ import freezegun import pytest from botocore.exceptions import ClientError + from enums.patient_ods_inactive_status import PatientOdsInactiveStatus from models.document_reference import DocumentReference from models.document_review import DocumentUploadReviewReference @@ -93,7 +94,9 @@ def mock_document_review_references(mocker): def test_handle_gp_change_message_called_message_type_gp_change( - mns_service, mock_handle_gp_change, mock_handle_death_notification + mns_service, + mock_handle_gp_change, + mock_handle_death_notification, ): mns_service.handle_mns_notification(gp_change_message) @@ -102,7 +105,9 @@ def test_handle_gp_change_message_called_message_type_gp_change( def test_handle_gp_change_message_not_called_message_death_message( - mns_service, mock_handle_death_notification, mock_handle_gp_change + mns_service, + mock_handle_death_notification, + mock_handle_gp_change, ): mns_service.handle_mns_notification(death_notification_message) @@ -123,10 +128,13 @@ def test_handle_mns_notification_error_handling_pds_error(mns_service, mocker): def test_handle_mns_notification_error_handling_client_error(mns_service, mocker): client_error = ClientError( - {"Error": {"Code": "TestException", "Message": "Test exception"}}, "operation" + {"Error": {"Code": "TestException", "Message": "Test exception"}}, + "operation", ) mocker.patch.object( - mns_service, "handle_gp_change_notification", side_effect=client_error + mns_service, + "handle_gp_change_notification", + side_effect=client_error, ) with pytest.raises(ClientError): @@ -134,7 +142,10 @@ def test_handle_mns_notification_error_handling_client_error(mns_service, mocker def test_handle_gp_change_notification_with_patient_documents( - mns_service, mock_document_references, mock_document_review_references, mocker + mns_service, + mock_document_references, + mock_document_review_references, + mocker, ): mocker.patch.object(mns_service, "get_all_patient_documents") mns_service.get_all_patient_documents.return_value = ( @@ -148,13 +159,15 @@ def test_handle_gp_change_notification_with_patient_documents( mns_service.handle_gp_change_notification(gp_change_message) mns_service.get_all_patient_documents.assert_called_once_with( - gp_change_message.subject.nhs_number + gp_change_message.subject.nhs_number, ) mns_service.get_updated_gp_ods.assert_called_once_with( - gp_change_message.subject.nhs_number + gp_change_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_called_once_with( - mock_document_references, mock_document_review_references, NEW_ODS_CODE + mock_document_references, + mock_document_review_references, + NEW_ODS_CODE, ) @@ -167,7 +180,7 @@ def test_handle_gp_change_notification_no_patient_documents(mns_service, mocker) mns_service.handle_gp_change_notification(gp_change_message) mns_service.get_all_patient_documents.assert_called_once_with( - gp_change_message.subject.nhs_number + gp_change_message.subject.nhs_number, ) mns_service.get_updated_gp_ods.assert_not_called() mns_service.update_all_patient_documents.assert_not_called() @@ -186,7 +199,10 @@ def test_handle_death_notification_informal(mns_service, mocker): def test_handle_death_notification_removed_with_documents( - mns_service, mock_document_references, mock_document_review_references, mocker + mns_service, + mock_document_references, + mock_document_review_references, + mocker, ): mocker.patch.object(mns_service, "get_all_patient_documents") mocker.patch.object(mns_service, "get_updated_gp_ods") @@ -200,13 +216,15 @@ def test_handle_death_notification_removed_with_documents( mns_service.handle_death_notification(removed_death_notification_message) mns_service.get_all_patient_documents.assert_called_once_with( - removed_death_notification_message.subject.nhs_number + removed_death_notification_message.subject.nhs_number, ) mns_service.get_updated_gp_ods.assert_called_once_with( - removed_death_notification_message.subject.nhs_number + removed_death_notification_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_called_once_with( - mock_document_references, mock_document_review_references, NEW_ODS_CODE + mock_document_references, + mock_document_review_references, + NEW_ODS_CODE, ) @@ -219,14 +237,17 @@ def test_handle_death_notification_removed_no_documents(mns_service, mocker): mns_service.handle_death_notification(removed_death_notification_message) mns_service.get_all_patient_documents.assert_called_once_with( - removed_death_notification_message.subject.nhs_number + removed_death_notification_message.subject.nhs_number, ) mns_service.get_updated_gp_ods.assert_not_called() mns_service.update_all_patient_documents.assert_not_called() def test_handle_death_notification_formal_with_documents( - mns_service, mock_document_references, mock_document_review_references, mocker + mns_service, + mock_document_references, + mock_document_review_references, + mocker, ): mocker.patch.object(mns_service, "get_all_patient_documents") mocker.patch.object(mns_service, "get_updated_gp_ods") @@ -239,7 +260,7 @@ def test_handle_death_notification_formal_with_documents( mns_service.handle_death_notification(death_notification_message) mns_service.get_all_patient_documents.assert_called_once_with( - death_notification_message.subject.nhs_number + death_notification_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_called_once_with( mock_document_references, @@ -258,7 +279,7 @@ def test_handle_death_notification_formal_no_documents(mns_service, mocker): mns_service.handle_death_notification(death_notification_message) mns_service.get_all_patient_documents.assert_called_once_with( - death_notification_message.subject.nhs_number + death_notification_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_not_called() @@ -273,12 +294,15 @@ def test_get_updated_gp_ods(mns_service): assert result == expected_ods mns_service.pds_service.fetch_patient_details.assert_called_once_with( - TEST_NHS_NUMBER + TEST_NHS_NUMBER, ) def test_pds_is_called_death_notification_removed( - mns_service, mocker, mock_document_references, mock_document_review_references + mns_service, + mocker, + mock_document_references, + mock_document_review_references, ): mocker.patch.object(mns_service, "get_updated_gp_ods") mocker.patch.object(mns_service, "update_all_patient_documents") @@ -310,54 +334,71 @@ def test_get_all_patient_documents(mns_service, mocker): assert lg_docs == expected_lg_docs assert review_docs == expected_review_docs mns_service.lg_document_service.fetch_documents_from_table_with_nhs_number.assert_called_once_with( - TEST_NHS_NUMBER + TEST_NHS_NUMBER, ) mns_service.document_review_service.fetch_documents_from_table_with_nhs_number.assert_called_once_with( - TEST_NHS_NUMBER + TEST_NHS_NUMBER, ) def test_update_all_patient_documents_with_both_types( - mns_service, mock_document_references, mock_document_review_references, mocker + mns_service, + mock_document_references, + mock_document_review_references, + mocker, ): mns_service.update_all_patient_documents( - mock_document_references, mock_document_review_references, NEW_ODS_CODE + mock_document_references, + mock_document_review_references, + NEW_ODS_CODE, ) mns_service.lg_document_service.update_patient_ods_code.assert_called_once_with( - mock_document_references, NEW_ODS_CODE + mock_document_references, + NEW_ODS_CODE, ) mns_service.document_review_service.update_document_review_custodian.assert_called_once_with( - mock_document_review_references, NEW_ODS_CODE + mock_document_review_references, + NEW_ODS_CODE, ) def test_update_all_patient_documents_with_only_lg_documents( - mns_service, mock_document_references, mocker + mns_service, + mock_document_references, + mocker, ): mns_service.update_all_patient_documents(mock_document_references, [], NEW_ODS_CODE) mns_service.lg_document_service.update_patient_ods_code.assert_called_once_with( - mock_document_references, NEW_ODS_CODE + mock_document_references, + NEW_ODS_CODE, ) mns_service.document_review_service.update_document_review_custodian.assert_not_called() def test_update_all_patient_documents_with_only_review_documents( - mns_service, mock_document_review_references, mocker + mns_service, + mock_document_review_references, + mocker, ): mns_service.update_all_patient_documents( - [], mock_document_review_references, NEW_ODS_CODE + [], + mock_document_review_references, + NEW_ODS_CODE, ) mns_service.lg_document_service.update_patient_ods_code.assert_not_called() mns_service.document_review_service.update_document_review_custodian.assert_called_once_with( - mock_document_review_references, NEW_ODS_CODE + mock_document_review_references, + NEW_ODS_CODE, ) def test_handle_gp_change_notification_with_only_lg_documents( - mns_service, mock_document_references, mocker + mns_service, + mock_document_references, + mocker, ): mocker.patch.object(mns_service, "get_all_patient_documents") mns_service.get_all_patient_documents.return_value = ( @@ -371,18 +412,22 @@ def test_handle_gp_change_notification_with_only_lg_documents( mns_service.handle_gp_change_notification(gp_change_message) mns_service.get_all_patient_documents.assert_called_once_with( - gp_change_message.subject.nhs_number + gp_change_message.subject.nhs_number, ) mns_service.get_updated_gp_ods.assert_called_once_with( - gp_change_message.subject.nhs_number + gp_change_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_called_once_with( - mock_document_references, [], NEW_ODS_CODE + mock_document_references, + [], + NEW_ODS_CODE, ) def test_handle_gp_change_notification_with_only_review_documents( - mns_service, mock_document_review_references, mocker + mns_service, + mock_document_review_references, + mocker, ): mocker.patch.object(mns_service, "get_all_patient_documents") mns_service.get_all_patient_documents.return_value = ( @@ -396,18 +441,22 @@ def test_handle_gp_change_notification_with_only_review_documents( mns_service.handle_gp_change_notification(gp_change_message) mns_service.get_all_patient_documents.assert_called_once_with( - gp_change_message.subject.nhs_number + gp_change_message.subject.nhs_number, ) mns_service.get_updated_gp_ods.assert_called_once_with( - gp_change_message.subject.nhs_number + gp_change_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_called_once_with( - [], mock_document_review_references, NEW_ODS_CODE + [], + mock_document_review_references, + NEW_ODS_CODE, ) def test_handle_death_notification_formal_with_only_lg_documents( - mns_service, mock_document_references, mocker + mns_service, + mock_document_references, + mocker, ): mocker.patch.object(mns_service, "get_all_patient_documents") mocker.patch.object(mns_service, "get_updated_gp_ods") @@ -420,7 +469,7 @@ def test_handle_death_notification_formal_with_only_lg_documents( mns_service.handle_death_notification(death_notification_message) mns_service.get_all_patient_documents.assert_called_once_with( - death_notification_message.subject.nhs_number + death_notification_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_called_once_with( mock_document_references, @@ -431,7 +480,9 @@ def test_handle_death_notification_formal_with_only_lg_documents( def test_handle_death_notification_formal_with_only_review_documents( - mns_service, mock_document_review_references, mocker + mns_service, + mock_document_review_references, + mocker, ): mocker.patch.object(mns_service, "get_all_patient_documents") mocker.patch.object(mns_service, "get_updated_gp_ods") @@ -444,7 +495,7 @@ def test_handle_death_notification_formal_with_only_review_documents( mns_service.handle_death_notification(death_notification_message) mns_service.get_all_patient_documents.assert_called_once_with( - death_notification_message.subject.nhs_number + death_notification_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_called_once_with( [], @@ -455,7 +506,9 @@ def test_handle_death_notification_formal_with_only_review_documents( def test_handle_death_notification_removed_with_only_lg_documents( - mns_service, mock_document_references, mocker + mns_service, + mock_document_references, + mocker, ): mocker.patch.object(mns_service, "get_all_patient_documents") mocker.patch.object(mns_service, "get_updated_gp_ods") @@ -469,18 +522,22 @@ def test_handle_death_notification_removed_with_only_lg_documents( mns_service.handle_death_notification(removed_death_notification_message) mns_service.get_all_patient_documents.assert_called_once_with( - removed_death_notification_message.subject.nhs_number + removed_death_notification_message.subject.nhs_number, ) mns_service.get_updated_gp_ods.assert_called_once_with( - removed_death_notification_message.subject.nhs_number + removed_death_notification_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_called_once_with( - mock_document_references, [], NEW_ODS_CODE + mock_document_references, + [], + NEW_ODS_CODE, ) def test_handle_death_notification_removed_with_only_review_documents( - mns_service, mock_document_review_references, mocker + mns_service, + mock_document_review_references, + mocker, ): mocker.patch.object(mns_service, "get_all_patient_documents") mocker.patch.object(mns_service, "get_updated_gp_ods") @@ -494,35 +551,56 @@ def test_handle_death_notification_removed_with_only_review_documents( mns_service.handle_death_notification(removed_death_notification_message) mns_service.get_all_patient_documents.assert_called_once_with( - removed_death_notification_message.subject.nhs_number + removed_death_notification_message.subject.nhs_number, ) mns_service.get_updated_gp_ods.assert_called_once_with( - removed_death_notification_message.subject.nhs_number + removed_death_notification_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_called_once_with( - [], mock_document_review_references, NEW_ODS_CODE + [], + mock_document_review_references, + NEW_ODS_CODE, ) -@pytest.mark.parametrize("mns_event", [removed_death_notification_message, gp_change_message]) -def test_handle_mns_notification_calls_update_restrictions_called_on_notification(mns_service, mns_event, mocker): +@pytest.mark.parametrize( + "mns_event", + [removed_death_notification_message, gp_change_message], +) +def test_handle_mns_notification_calls_update_restrictions_called_on_notification( + mns_service, + mns_event, + mocker, +): mocker.patch.object(mns_service, "update_restrictions") - updated_ods = mocker.patch.object(mns_service, "get_updated_gp_ods", return_value=TEST_CURRENT_GP_ODS) + updated_ods = mocker.patch.object( + mns_service, + "get_updated_gp_ods", + return_value=TEST_CURRENT_GP_ODS, + ) mns_service.handle_mns_notification(mns_event) - mns_service.update_restrictions.assert_called_with(nhs_number=TEST_NHS_NUMBER, custodian=updated_ods.return_value) + mns_service.update_restrictions.assert_called_with( + nhs_number=TEST_NHS_NUMBER, + custodian=updated_ods.return_value, + ) -def test_handle_mns_notification_calls_update_restriction_called_on_death_notification(mns_service, mocker): +def test_handle_mns_notification_calls_update_restriction_called_on_death_notification( + mns_service, + mocker, +): mocker.patch.object(mns_service, "update_restrictions") mns_service.handle_mns_notification(death_notification_message) mns_service.update_restrictions.assert_called_once_with( - nhs_number=TEST_NHS_NUMBER, custodian=PatientOdsInactiveStatus.DECEASED, + nhs_number=TEST_NHS_NUMBER, + custodian=PatientOdsInactiveStatus.DECEASED, ) + @freezegun.freeze_time("2021-04-01") def test_update_restrictions_uses_restriction_dynamo_service(mns_service, mocker): MOCK_RESTRICTION = { @@ -549,6 +627,6 @@ def test_update_restrictions_uses_restriction_dynamo_service(mns_service, mocker ) mns_service.restrictions_dynamo_service.update_restriction_custodian.assert_called_once_with( - restriction_id=MOCK_RESTRICTION["ID"], updated_custodian=NEW_ODS_CODE, + restriction_id=MOCK_RESTRICTION["ID"], + updated_custodian=NEW_ODS_CODE, ) - diff --git a/lambdas/utils/exceptions.py b/lambdas/utils/exceptions.py index a8dda2b796..464e8bb96d 100644 --- a/lambdas/utils/exceptions.py +++ b/lambdas/utils/exceptions.py @@ -238,5 +238,6 @@ def to_dict(self): class UserRestrictionConditionCheckFailedException(Exception): pass + class UserRestrictionValidationException(Exception): - pass \ No newline at end of file + pass From 439cc022a2e0baba7f932d9f951d7678d8a8f3c2 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Tue, 10 Mar 2026 13:22:31 +0000 Subject: [PATCH 09/17] [PRMP-1476] add logging and tests --- .../user_restrictions/user_restrictions.py | 1 + .../user_restriction_dynamo_service.py | 39 ++++++++++-------- .../test_user_restriction_dynamo_service.py | 41 ++++++++++++++++++- lambdas/utils/exceptions.py | 3 ++ 4 files changed, 66 insertions(+), 18 deletions(-) diff --git a/lambdas/models/user_restrictions/user_restrictions.py b/lambdas/models/user_restrictions/user_restrictions.py index 521eedddce..68b5a5370f 100644 --- a/lambdas/models/user_restrictions/user_restrictions.py +++ b/lambdas/models/user_restrictions/user_restrictions.py @@ -20,6 +20,7 @@ class UserRestrictionsFields(StrEnum): class UserRestrictionIndexes(StrEnum): CUSTODIAN_INDEX = "CustodianIndex" + NHS_NUMBER_INDEX = "NhsNumberIndex" class UserRestriction(BaseModel): diff --git a/lambdas/services/user_restrictions/user_restriction_dynamo_service.py b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py index e9b4a22e47..18c5a0015d 100644 --- a/lambdas/services/user_restrictions/user_restriction_dynamo_service.py +++ b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py @@ -18,7 +18,7 @@ from utils.exceptions import ( UserRestrictionConditionCheckFailedException, - UserRestrictionValidationException, + UserRestrictionValidationException, UserRestrictionDynamoDBException, ) logger = LoggingService(__name__) @@ -133,23 +133,30 @@ def update_restriction_inactive( def query_restrictions_by_nhs_number( self, nhs_number: str, - ): - filter_builder = DynamoQueryFilterBuilder() - filter_builder.add_condition( - UserRestrictionsFields.IS_ACTIVE.value, - AttributeOperator.EQUAL, - True, - ) - active_filter_expression = filter_builder.build() + ) -> list[UserRestriction]: + try: + logger.info("Building IsActive filter for DynamoDB query.") + filter_builder = DynamoQueryFilterBuilder() + filter_builder.add_condition( + UserRestrictionsFields.IS_ACTIVE.value, + AttributeOperator.EQUAL, + True, + ) + active_filter_expression = filter_builder.build() - items = self.dynamo_service.query_table( - table_name=self.table_name, - index_name=NHS_NUMBER_GSI, - search_key=nhs_number, - query_filter=active_filter_expression, - ) + logger.info("Querying Restrictions by NHS Number.") + items = self.dynamo_service.query_table( + table_name=self.table_name, + index_name=UserRestrictionIndexes.NHS_NUMBER_INDEX.value, + search_key=UserRestrictionsFields.NHS_NUMBER.value, + search_condition=nhs_number, + query_filter=active_filter_expression, + ) - return self._validate_restrictions(items) + return self._validate_restrictions(items) + except ClientError as e: + logger.error(e) + raise UserRestrictionDynamoDBException("An issue occurred while querying restrictions") def update_restriction_custodian(self, restriction_id: str, updated_custodian: str): try: diff --git a/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py b/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py index 7308df12bb..d60d60849a 100644 --- a/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py +++ b/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py @@ -1,4 +1,5 @@ import pytest +from boto3.dynamodb.conditions import Attr from botocore.exceptions import ClientError from models.user_restrictions.user_restrictions import ( @@ -13,9 +14,9 @@ TEST_NEXT_PAGE_TOKEN, TEST_NHS_NUMBER, TEST_SMART_CARD_ID, - TEST_UUID, + TEST_UUID, MOCK_USER_RESTRICTION_TABLE, ) -from utils.exceptions import UserRestrictionValidationException +from utils.exceptions import UserRestrictionValidationException, UserRestrictionDynamoDBException MOCK_RESTRICTION = { "ID": TEST_UUID, @@ -136,6 +137,42 @@ def test_query_restrictions_returns_empty_list_when_no_items(mock_service): assert next_token is None +def test_query_restrictions_by_nhs_number(mock_service, mocker): + mock_validate = mocker.patch.object(mock_service, "_validate_restrictions") + mock_service.dynamo_service.query_table.return_value = [MOCK_RESTRICTION] + + expected_filter = Attr("IsActive").eq(True) + + mock_service.query_restrictions_by_nhs_number(nhs_number=TEST_NHS_NUMBER) + + mock_service.dynamo_service.query_table.assert_called_with( + table_name=MOCK_USER_RESTRICTION_TABLE, + index_name=UserRestrictionIndexes.NHS_NUMBER_INDEX.value, + search_key=UserRestrictionsFields.NHS_NUMBER.value, + search_condition=TEST_NHS_NUMBER, + query_filter=expected_filter, + ) + + mock_validate.assert_called_with([MOCK_RESTRICTION]) + +def test_query_restrictions_by_nhs_number_handles_client_error(mock_service): + mock_service.dynamo_service.query_table.side_effect = ClientError( + {"Error": {"Code": "500", "Message": "DynamoDB error"}}, + "query", + ) + + with pytest.raises(UserRestrictionDynamoDBException): + mock_service.query_restrictions_by_nhs_number(nhs_number=TEST_NHS_NUMBER) + + +def test_query_restrictions_by_nhs_number_handles_validation_error(mock_service): + mock_service.dynamo_service.query_table.return_value = [{"invalid": "object"}] + + with pytest.raises(UserRestrictionValidationException): + mock_service.query_restrictions_by_nhs_number(nhs_number=TEST_NHS_NUMBER) + + + def test_validate_restrictions_raises_for_invalid_items(): with pytest.raises(UserRestrictionValidationException): UserRestrictionDynamoService._validate_restrictions([{"invalid": "data"}]) diff --git a/lambdas/utils/exceptions.py b/lambdas/utils/exceptions.py index e767065958..6d5810ebcb 100644 --- a/lambdas/utils/exceptions.py +++ b/lambdas/utils/exceptions.py @@ -246,3 +246,6 @@ class UserRestrictionValidationException(Exception): class UserRestrictionConditionCheckFailedException(Exception): pass +class UserRestrictionDynamoDBException(Exception): + pass + From fb0351ef0daa2c561674270edcf236949864afee Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Tue, 10 Mar 2026 13:23:11 +0000 Subject: [PATCH 10/17] [PRMP-1476] format --- .../user_restriction_dynamo_service.py | 8 +++++--- .../test_user_restriction_dynamo_service.py | 10 +++++++--- lambdas/utils/exceptions.py | 2 +- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/lambdas/services/user_restrictions/user_restriction_dynamo_service.py b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py index 18c5a0015d..7fc57f7bf6 100644 --- a/lambdas/services/user_restrictions/user_restriction_dynamo_service.py +++ b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py @@ -15,10 +15,10 @@ from utils.audit_logging_setup import LoggingService from utils.dynamo_query_filter_builder import DynamoQueryFilterBuilder from utils.dynamo_utils import build_mixed_condition_expression - from utils.exceptions import ( UserRestrictionConditionCheckFailedException, - UserRestrictionValidationException, UserRestrictionDynamoDBException, + UserRestrictionDynamoDBException, + UserRestrictionValidationException, ) logger = LoggingService(__name__) @@ -156,7 +156,9 @@ def query_restrictions_by_nhs_number( return self._validate_restrictions(items) except ClientError as e: logger.error(e) - raise UserRestrictionDynamoDBException("An issue occurred while querying restrictions") + raise UserRestrictionDynamoDBException( + "An issue occurred while querying restrictions", + ) def update_restriction_custodian(self, restriction_id: str, updated_custodian: str): try: diff --git a/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py b/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py index d60d60849a..04350d7083 100644 --- a/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py +++ b/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py @@ -10,13 +10,17 @@ UserRestrictionDynamoService, ) from tests.unit.conftest import ( + MOCK_USER_RESTRICTION_TABLE, TEST_CURRENT_GP_ODS, TEST_NEXT_PAGE_TOKEN, TEST_NHS_NUMBER, TEST_SMART_CARD_ID, - TEST_UUID, MOCK_USER_RESTRICTION_TABLE, + TEST_UUID, +) +from utils.exceptions import ( + UserRestrictionDynamoDBException, + UserRestrictionValidationException, ) -from utils.exceptions import UserRestrictionValidationException, UserRestrictionDynamoDBException MOCK_RESTRICTION = { "ID": TEST_UUID, @@ -155,6 +159,7 @@ def test_query_restrictions_by_nhs_number(mock_service, mocker): mock_validate.assert_called_with([MOCK_RESTRICTION]) + def test_query_restrictions_by_nhs_number_handles_client_error(mock_service): mock_service.dynamo_service.query_table.side_effect = ClientError( {"Error": {"Code": "500", "Message": "DynamoDB error"}}, @@ -172,7 +177,6 @@ def test_query_restrictions_by_nhs_number_handles_validation_error(mock_service) mock_service.query_restrictions_by_nhs_number(nhs_number=TEST_NHS_NUMBER) - def test_validate_restrictions_raises_for_invalid_items(): with pytest.raises(UserRestrictionValidationException): UserRestrictionDynamoService._validate_restrictions([{"invalid": "data"}]) diff --git a/lambdas/utils/exceptions.py b/lambdas/utils/exceptions.py index 6d5810ebcb..50f8c4bda2 100644 --- a/lambdas/utils/exceptions.py +++ b/lambdas/utils/exceptions.py @@ -246,6 +246,6 @@ class UserRestrictionValidationException(Exception): class UserRestrictionConditionCheckFailedException(Exception): pass + class UserRestrictionDynamoDBException(Exception): pass - From 225be51a702f154b9eaca5003f077eb1aa713ce3 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Tue, 10 Mar 2026 13:44:09 +0000 Subject: [PATCH 11/17] [PRMP-1476] add feature flag to mns service --- .../services/process_mns_message_service.py | 11 ++++ .../test_process_mns_message_service.py | 58 ++++++++++++++++++- 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/lambdas/services/process_mns_message_service.py b/lambdas/services/process_mns_message_service.py index d82dbf98b2..4a453152f3 100644 --- a/lambdas/services/process_mns_message_service.py +++ b/lambdas/services/process_mns_message_service.py @@ -3,6 +3,7 @@ from botocore.exceptions import ClientError from enums.death_notification_status import DeathNotificationStatus +from enums.feature_flags import FeatureFlags from enums.mns_notification_types import MNSNotificationTypes from enums.patient_ods_inactive_status import PatientOdsInactiveStatus from models.document_reference import DocumentReference @@ -169,6 +170,16 @@ def update_all_patient_documents( def update_restrictions(self, nhs_number: str, custodian: str) -> None: + restrictions_feature_flag = self.feature_flag_service.get_feature_flags_by_flag( + FeatureFlags.USER_RESTRICTION_ENABLED, + ) + + if not restrictions_feature_flag.get( + FeatureFlags.USER_RESTRICTION_ENABLED, + False, + ): + return + restrictions = ( self.restrictions_dynamo_service.query_restrictions_by_nhs_number( nhs_number=nhs_number, diff --git a/lambdas/tests/unit/services/test_process_mns_message_service.py b/lambdas/tests/unit/services/test_process_mns_message_service.py index d8b48a7b4c..5edb6bb808 100644 --- a/lambdas/tests/unit/services/test_process_mns_message_service.py +++ b/lambdas/tests/unit/services/test_process_mns_message_service.py @@ -4,11 +4,13 @@ import pytest from botocore.exceptions import ClientError +from enums.feature_flags import FeatureFlags from enums.patient_ods_inactive_status import PatientOdsInactiveStatus from models.document_reference import DocumentReference from models.document_review import DocumentUploadReviewReference from models.sqs.mns_sqs_message import MNSSQSMessage from models.user_restrictions.user_restrictions import UserRestriction +from services.feature_flags_service import FeatureFlagService from services.process_mns_message_service import MNSNotificationService from tests.unit.conftest import TEST_CURRENT_GP_ODS, TEST_NHS_NUMBER, TEST_UUID from tests.unit.handlers.test_mns_notification_handler import ( @@ -21,7 +23,25 @@ @pytest.fixture -def mns_service(mocker, set_env, monkeypatch): +def mock_user_restriction_disabled(mocker): + mock_function = mocker.patch.object(FeatureFlagService, "get_feature_flags_by_flag") + mock_feature_flag = mock_function.return_value = { + FeatureFlags.USER_RESTRICTION_ENABLED: False, + } + yield mock_feature_flag + + +@pytest.fixture +def mock_user_restriction_enabled(mocker): + mock_function = mocker.patch.object(FeatureFlagService, "get_feature_flags_by_flag") + mock_feature_flag = mock_function.return_value = { + FeatureFlags.USER_RESTRICTION_ENABLED: True, + } + yield mock_feature_flag + + +@pytest.fixture +def mns_service(mocker, set_env, monkeypatch, mock_user_restriction_enabled): monkeypatch.setenv("PDS_FHIR_IS_STUBBED", "False") service = MNSNotificationService() mocker.patch.object(service, "pds_service") @@ -33,14 +53,19 @@ def mns_service(mocker, set_env, monkeypatch): @pytest.fixture -def mns_service_feature_disabled(mocker, set_env, monkeypatch): +def mns_service_restrictions_feature_disabled( + mocker, + set_env, + monkeypatch, + mock_user_restriction_disabled, +): monkeypatch.setenv("PDS_FHIR_IS_STUBBED", "False") service = MNSNotificationService() mocker.patch.object(service, "pds_service") mocker.patch.object(service, "document_review_service") mocker.patch.object(service, "lg_document_service") mocker.patch.object(service, "sqs_service") - mocker.patch.object(service, "update_restrictions") + mocker.patch.object(service, "restrictions_dynamo_service") yield service @@ -630,3 +655,30 @@ def test_update_restrictions_uses_restriction_dynamo_service(mns_service, mocker restriction_id=MOCK_RESTRICTION["ID"], updated_custodian=NEW_ODS_CODE, ) + + +@pytest.mark.parametrize( + "mns_event", + [death_notification_message, gp_change_message, removed_death_notification_message], +) +def test_update_restrictions_does_not_call_dynamodb_feature_flag_disabled( + mns_service_restrictions_feature_disabled, + mns_event, +): + mns_service_restrictions_feature_disabled.handle_mns_notification( + mns_event, + ) + + mns_service_restrictions_feature_disabled.restrictions_dynamo_service.query_restrictions_by_nhs_number.assert_not_called() + mns_service_restrictions_feature_disabled.restrictions_dynamo_service.update_restriction_custodian.assert_not_called() + + +def test_update_restrictions_not_called_on_informal_death_notification( + mns_service, + mocker, +): + mock_update_restriction = mocker.patch.object(mns_service, "update_restrictions") + + mns_service.handle_mns_notification(informal_death_notification_message) + + mock_update_restriction.assert_not_called() From 792f642bdbbb6b4b357b269083c09c70bd7b8dc8 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Tue, 10 Mar 2026 16:53:28 +0000 Subject: [PATCH 12/17] [PRMP-1476] ensure restrictions are updated if even if not documents present --- .../services/process_mns_message_service.py | 120 ++++++++++++------ .../test_process_mns_message_service.py | 76 ++++++++--- 2 files changed, 139 insertions(+), 57 deletions(-) diff --git a/lambdas/services/process_mns_message_service.py b/lambdas/services/process_mns_message_service.py index 4a453152f3..6ed5be41aa 100644 --- a/lambdas/services/process_mns_message_service.py +++ b/lambdas/services/process_mns_message_service.py @@ -9,6 +9,7 @@ from models.document_reference import DocumentReference from models.document_review import DocumentUploadReviewReference from models.sqs.mns_sqs_message import MNSSQSMessage +from models.user_restrictions.user_restrictions import UserRestriction from services.base.sqs_service import SQSService from services.document_reference_service import DocumentReferenceService from services.document_upload_review_service import DocumentUploadReviewService @@ -63,21 +64,33 @@ def handle_gp_change_notification(self, message: MNSSQSMessage) -> None: message.subject.nhs_number, ) - if not lg_documents and not review_documents: - return - - updated_ods_code = self.get_updated_gp_ods(message.subject.nhs_number) - self.update_all_patient_documents( - lg_documents, - review_documents, - updated_ods_code, + restrictions = ( + ( + self.restrictions_dynamo_service.query_restrictions_by_nhs_number( + nhs_number=message.subject.nhs_number, + ) + ) + if self.is_restrictions_enabled() + else None ) - logger.info("Update complete for change of GP") - self.update_restrictions( - nhs_number=message.subject.nhs_number, - custodian=updated_ods_code, - ) + if lg_documents or review_documents or restrictions: + updated_ods_code = self.get_updated_gp_ods(message.subject.nhs_number) + + if lg_documents or review_documents: + self.update_all_patient_documents( + lg_documents, + review_documents, + updated_ods_code, + ) + logger.info("Update complete for change of GP") + + if restrictions: + self.update_restrictions( + nhs_number=message.subject.nhs_number, + custodian=updated_ods_code, + restrictions=restrictions, + ) def handle_death_notification(self, message: MNSSQSMessage) -> None: death_notification_type = message.data["deathNotificationStatus"] @@ -94,25 +107,49 @@ def handle_death_notification(self, message: MNSSQSMessage) -> None: nhs_number, ) - if lg_documents or review_documents: - updated_ods_code = self.get_updated_gp_ods(nhs_number) - self.update_all_patient_documents( - lg_documents, - review_documents, - updated_ods_code, + restrictions = ( + ( + self.restrictions_dynamo_service.query_restrictions_by_nhs_number( + nhs_number=nhs_number, + ) ) - logger.info("Update complete for death notification change.") + if self.is_restrictions_enabled() + else None + ) - self.update_restrictions( - nhs_number=nhs_number, - custodian=updated_ods_code, - ) + if lg_documents or review_documents or restrictions: + updated_ods_code = self.get_updated_gp_ods(nhs_number) + + if lg_documents or review_documents: + self.update_all_patient_documents( + lg_documents, + review_documents, + updated_ods_code, + ) + logger.info("Update complete for death notification change.") + + if restrictions: + self.update_restrictions( + nhs_number=nhs_number, + custodian=updated_ods_code, + restrictions=restrictions, + ) case DeathNotificationStatus.FORMAL: lg_documents, review_documents = self.get_all_patient_documents( nhs_number, ) + restrictions = ( + ( + self.restrictions_dynamo_service.query_restrictions_by_nhs_number( + nhs_number=nhs_number, + ) + ) + if self.is_restrictions_enabled() + else None + ) + if lg_documents or review_documents: self.update_all_patient_documents( lg_documents, @@ -123,9 +160,11 @@ def handle_death_notification(self, message: MNSSQSMessage) -> None: f"Update complete, patient marked {PatientOdsInactiveStatus.DECEASED}.", ) + if restrictions: self.update_restrictions( nhs_number=nhs_number, custodian=PatientOdsInactiveStatus.DECEASED, + restrictions=restrictions, ) def get_updated_gp_ods(self, nhs_number: str) -> str: @@ -168,28 +207,27 @@ def update_all_patient_documents( updated_ods_code, ) - def update_restrictions(self, nhs_number: str, custodian: str) -> None: + def update_restrictions( + self, + nhs_number: str, + custodian: str, + restrictions: list[UserRestriction], + ) -> None: + for restriction in restrictions: + logger.info(f"Updating restriction {restriction.id}") + self.restrictions_dynamo_service.update_restriction_custodian( + restriction_id=restriction.id, + updated_custodian=custodian, + ) + + logger.info(f"All restrictions for patient {nhs_number} updated.") + def is_restrictions_enabled(self) -> bool: restrictions_feature_flag = self.feature_flag_service.get_feature_flags_by_flag( FeatureFlags.USER_RESTRICTION_ENABLED, ) - if not restrictions_feature_flag.get( + return restrictions_feature_flag.get( FeatureFlags.USER_RESTRICTION_ENABLED, False, - ): - return - - restrictions = ( - self.restrictions_dynamo_service.query_restrictions_by_nhs_number( - nhs_number=nhs_number, - ) ) - - for restriction in restrictions: - self.restrictions_dynamo_service.update_restriction_custodian( - restriction_id=restriction.id, - updated_custodian=custodian, - ) - - logger.info(f"All restrictions for patient {nhs_number} updated.") diff --git a/lambdas/tests/unit/services/test_process_mns_message_service.py b/lambdas/tests/unit/services/test_process_mns_message_service.py index 5edb6bb808..1fec31f47e 100644 --- a/lambdas/tests/unit/services/test_process_mns_message_service.py +++ b/lambdas/tests/unit/services/test_process_mns_message_service.py @@ -40,6 +40,21 @@ def mock_user_restriction_enabled(mocker): yield mock_feature_flag +MOCK_RESTRICTION_DICT = { + "ID": TEST_UUID, + "RestrictedSmartcard": "123456789012", + "NhsNumber": TEST_NHS_NUMBER, + "Custodian": TEST_CURRENT_GP_ODS, + "Created": 1700000000, + "CreatorSmartcard": "223456789022", + "RemoverSmartCard": None, + "IsActive": True, + "LastUpdated": 1700000001, +} + +MOCK_RESTRICTION = UserRestriction.model_validate(MOCK_RESTRICTION_DICT) + + @pytest.fixture def mns_service(mocker, set_env, monkeypatch, mock_user_restriction_enabled): monkeypatch.setenv("PDS_FHIR_IS_STUBBED", "False") @@ -199,6 +214,9 @@ def test_handle_gp_change_notification_with_patient_documents( def test_handle_gp_change_notification_no_patient_documents(mns_service, mocker): mocker.patch.object(mns_service, "get_all_patient_documents") mns_service.get_all_patient_documents.return_value = ([], []) + mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.return_value = ( + [] + ) mocker.patch.object(mns_service, "get_updated_gp_ods") mocker.patch.object(mns_service, "update_all_patient_documents") @@ -258,6 +276,9 @@ def test_handle_death_notification_removed_no_documents(mns_service, mocker): mocker.patch.object(mns_service, "get_updated_gp_ods") mocker.patch.object(mns_service, "update_all_patient_documents") mns_service.get_all_patient_documents.return_value = ([], []) + mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.return_value = ( + [] + ) mns_service.handle_death_notification(removed_death_notification_message) @@ -343,6 +364,28 @@ def test_pds_is_called_death_notification_removed( mns_service.update_all_patient_documents.assert_called() +@pytest.mark.parametrize( + "mns_event", + [removed_death_notification_message, gp_change_message], +) +def test_pds_called_only_restrictions_present( + mns_service, + mocker, + mns_event, +): + mocker.patch.object(mns_service, "get_updated_gp_ods") + mocker.patch.object(mns_service, "get_all_patient_documents") + mns_service.get_all_patient_documents.return_value = ([], []) + + mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.return_value = [ + MOCK_RESTRICTION, + ] + + mns_service.handle_mns_notification(mns_event) + + mns_service.get_updated_gp_ods.assert_called() + + def test_get_all_patient_documents(mns_service, mocker): expected_lg_docs = [MagicMock(spec=DocumentReference)] expected_review_docs = [MagicMock(spec=DocumentUploadReviewReference)] @@ -597,6 +640,9 @@ def test_handle_mns_notification_calls_update_restrictions_called_on_notificatio mns_event, mocker, ): + mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.return_value = [ + MOCK_RESTRICTION, + ] mocker.patch.object(mns_service, "update_restrictions") updated_ods = mocker.patch.object( mns_service, @@ -609,6 +655,7 @@ def test_handle_mns_notification_calls_update_restrictions_called_on_notificatio mns_service.update_restrictions.assert_called_with( nhs_number=TEST_NHS_NUMBER, custodian=updated_ods.return_value, + restrictions=[MOCK_RESTRICTION], ) @@ -616,43 +663,40 @@ def test_handle_mns_notification_calls_update_restriction_called_on_death_notifi mns_service, mocker, ): + mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.return_value = [ + MOCK_RESTRICTION, + ] mocker.patch.object(mns_service, "update_restrictions") mns_service.handle_mns_notification(death_notification_message) + mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.assert_called_with( + nhs_number=TEST_NHS_NUMBER, + ) + mns_service.update_restrictions.assert_called_once_with( nhs_number=TEST_NHS_NUMBER, custodian=PatientOdsInactiveStatus.DECEASED, + restrictions=[MOCK_RESTRICTION], ) @freezegun.freeze_time("2021-04-01") def test_update_restrictions_uses_restriction_dynamo_service(mns_service, mocker): - MOCK_RESTRICTION = { - "ID": TEST_UUID, - "RestrictedSmartcard": "123456789012", - "NhsNumber": TEST_NHS_NUMBER, - "Custodian": TEST_CURRENT_GP_ODS, - "Created": 1700000000, - "CreatorSmartcard": "223456789022", - "RemoverSmartCard": None, - "IsActive": True, - "LastUpdated": 1700000001, - } mocker.patch.object(mns_service, "get_updated_gp_ods", return_value=NEW_ODS_CODE) mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.return_value = [ - UserRestriction.model_validate(MOCK_RESTRICTION), + MOCK_RESTRICTION, ] - mns_service.update_restrictions(nhs_number=TEST_NHS_NUMBER, custodian=NEW_ODS_CODE) - - mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.assert_called_with( + mns_service.update_restrictions( nhs_number=TEST_NHS_NUMBER, + custodian=NEW_ODS_CODE, + restrictions=[MOCK_RESTRICTION], ) mns_service.restrictions_dynamo_service.update_restriction_custodian.assert_called_once_with( - restriction_id=MOCK_RESTRICTION["ID"], + restriction_id=MOCK_RESTRICTION.id, updated_custodian=NEW_ODS_CODE, ) From 6ca93a051931e0369bf0a9de0209dd7d5383caef Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Mon, 9 Mar 2026 16:13:51 +0000 Subject: [PATCH 13/17] [PRMP-1476] update restrictions table on MNS notification --- lambdas/handlers/mns_notification_handler.py | 3 +- .../services/process_mns_message_service.py | 21 ++++++- .../user_restriction_dynamo_service.py | 38 +++++++++++++ .../test_process_mns_message_service.py | 56 ++++++++++++++++++- lambdas/utils/exceptions.py | 2 +- 5 files changed, 116 insertions(+), 4 deletions(-) diff --git a/lambdas/handlers/mns_notification_handler.py b/lambdas/handlers/mns_notification_handler.py index ffc8df2a83..3b2aec16d0 100644 --- a/lambdas/handlers/mns_notification_handler.py +++ b/lambdas/handlers/mns_notification_handler.py @@ -21,7 +21,8 @@ "LLOYD_GEORGE_DYNAMODB_NAME", "DOCUMENT_REVIEW_DYNAMODB_NAME", "MNS_NOTIFICATION_QUEUE_URL", - ] + "RESTRICTIONS_TABLE_NAME", + ], ) @override_error_check def lambda_handler(event, context): diff --git a/lambdas/services/process_mns_message_service.py b/lambdas/services/process_mns_message_service.py index 96e647195e..66fc845a74 100644 --- a/lambdas/services/process_mns_message_service.py +++ b/lambdas/services/process_mns_message_service.py @@ -11,6 +11,7 @@ from services.document_reference_service import DocumentReferenceService from services.document_upload_review_service import DocumentUploadReviewService from services.feature_flags_service import FeatureFlagService +from services.user_restrictions.user_restriction_dynamo_service import UserRestrictionDynamoService from utils.audit_logging_setup import LoggingService from utils.exceptions import PdsErrorException from utils.utilities import get_pds_service @@ -26,6 +27,7 @@ def __init__(self): self.sqs_service = SQSService() self.queue = os.getenv("MNS_NOTIFICATION_QUEUE_URL") self.feature_flag_service = FeatureFlagService() + self.restrictions_dynamo_service = UserRestrictionDynamoService() def handle_mns_notification(self, message: MNSSQSMessage): try: @@ -62,10 +64,12 @@ def handle_gp_change_notification(self, message: MNSSQSMessage) -> None: updated_ods_code = self.get_updated_gp_ods(message.subject.nhs_number) self.update_all_patient_documents( - lg_documents, review_documents, updated_ods_code + lg_documents, review_documents, updated_ods_code, ) logger.info("Update complete for change of GP") + self.update_restrictions(nhs_number=message.subject.nhs_number, custodian=updated_ods_code) + def handle_death_notification(self, message: MNSSQSMessage) -> None: death_notification_type = message.data["deathNotificationStatus"] nhs_number = message.subject.nhs_number @@ -88,6 +92,8 @@ def handle_death_notification(self, message: MNSSQSMessage) -> None: ) logger.info("Update complete for death notification change.") + self.update_restrictions(nhs_number=nhs_number, custodian=updated_ods_code) + case DeathNotificationStatus.FORMAL: lg_documents, review_documents = self.get_all_patient_documents( nhs_number @@ -103,6 +109,8 @@ def handle_death_notification(self, message: MNSSQSMessage) -> None: f"Update complete, patient marked {PatientOdsInactiveStatus.DECEASED}." ) + self.update_restrictions(nhs_number=nhs_number, custodian=PatientOdsInactiveStatus.DECEASED) + def get_updated_gp_ods(self, nhs_number: str) -> str: patient_details = self.pds_service.fetch_patient_details(nhs_number) return patient_details.general_practice_ods @@ -139,3 +147,14 @@ def update_all_patient_documents( self.document_review_service.update_document_review_custodian( review_documents, updated_ods_code ) + + def update_restrictions(self, nhs_number: str, custodian: str) -> None: + + restrictions = self.restrictions_dynamo_service.query_restrictions_by_nhs_number(nhs_number=nhs_number) + + for restriction in restrictions: + self.restrictions_dynamo_service.update_restriction_custodian( + restriction_id=restriction.id, updated_custodian=custodian, + ) + + logger.info(f"All restrictions for patient {nhs_number} updated.") diff --git a/lambdas/services/user_restrictions/user_restriction_dynamo_service.py b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py index 47ebf7a8ba..ee29e1f060 100644 --- a/lambdas/services/user_restrictions/user_restriction_dynamo_service.py +++ b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py @@ -102,6 +102,7 @@ def update_restriction_inactive( ) except ClientError as e: + logger.error(e) if ( e.response["Error"]["Code"] == DynamoClientErrors.CONDITION_CHECK_FAILURE @@ -113,6 +114,43 @@ def update_restriction_inactive( ) raise e + def query_restrictions_by_nhs_number( + self, + nhs_number: str, + ): + filter_builder = DynamoQueryFilterBuilder() + filter_builder.add_condition(UserRestrictionsFields.IS_ACTIVE.value, AttributeOperator.EQUAL, True) + active_filter_expression = filter_builder.build() + + items = self.dynamo_service.query_table( + table_name=self.table_name, + index_name=UserRestrictionIndexes.NHS_NUMBER_INDEX, + search_key=nhs_number, + query_filter=active_filter_expression, + ) + + return self._validate_restrictions(items) + + def update_restriction_custodian(self, restriction_id: str, updated_custodian: str): + try: + logger.info(f"Updating custodian for restriction: {restriction_id}") + current_time = int(datetime.now(timezone.utc).timestamp()) + + updated_fields = { + UserRestrictionsFields.LAST_UPDATED.value: current_time, + UserRestrictionsFields.CUSTODIAN.value: updated_custodian, + } + + self.dynamo_service.update_item( + table_name=self.table_name, + key_pair={UserRestrictionsFields.ID.value: restriction_id}, + updated_fields=updated_fields, + ) + + except ClientError as e: + logger.error(e) + raise e + @staticmethod def _build_query_filter( smart_card_id: str | None, diff --git a/lambdas/tests/unit/services/test_process_mns_message_service.py b/lambdas/tests/unit/services/test_process_mns_message_service.py index 8bcd0945cd..e6a9bff9a7 100644 --- a/lambdas/tests/unit/services/test_process_mns_message_service.py +++ b/lambdas/tests/unit/services/test_process_mns_message_service.py @@ -1,13 +1,15 @@ from unittest.mock import MagicMock +import freezegun import pytest from botocore.exceptions import ClientError from enums.patient_ods_inactive_status import PatientOdsInactiveStatus from models.document_reference import DocumentReference from models.document_review import DocumentUploadReviewReference from models.sqs.mns_sqs_message import MNSSQSMessage +from models.user_restrictions.user_restrictions import UserRestriction from services.process_mns_message_service import MNSNotificationService -from tests.unit.conftest import TEST_CURRENT_GP_ODS, TEST_NHS_NUMBER +from tests.unit.conftest import TEST_CURRENT_GP_ODS, TEST_NHS_NUMBER, TEST_UUID from tests.unit.handlers.test_mns_notification_handler import ( MOCK_DEATH_MESSAGE_BODY, MOCK_GP_CHANGE_MESSAGE_BODY, @@ -25,6 +27,7 @@ def mns_service(mocker, set_env, monkeypatch): mocker.patch.object(service, "document_review_service") mocker.patch.object(service, "lg_document_service") mocker.patch.object(service, "sqs_service") + mocker.patch.object(service, "restrictions_dynamo_service") yield service @@ -36,6 +39,7 @@ def mns_service_feature_disabled(mocker, set_env, monkeypatch): mocker.patch.object(service, "document_review_service") mocker.patch.object(service, "lg_document_service") mocker.patch.object(service, "sqs_service") + mocker.patch.object(service, "update_restrictions") yield service @@ -498,3 +502,53 @@ def test_handle_death_notification_removed_with_only_review_documents( mns_service.update_all_patient_documents.assert_called_once_with( [], mock_document_review_references, NEW_ODS_CODE ) + + +@pytest.mark.parametrize("mns_event", [removed_death_notification_message, gp_change_message]) +def test_handle_mns_notification_calls_update_restrictions_called_on_notification(mns_service, mns_event, mocker): + mocker.patch.object(mns_service, "update_restrictions") + updated_ods = mocker.patch.object(mns_service, "get_updated_gp_ods", return_value=TEST_CURRENT_GP_ODS) + + mns_service.handle_mns_notification(mns_event) + + mns_service.update_restrictions.assert_called_with(nhs_number=TEST_NHS_NUMBER, custodian=updated_ods.return_value) + + +def test_handle_mns_notification_calls_update_restriction_called_on_death_notification(mns_service, mocker): + mocker.patch.object(mns_service, "update_restrictions") + + mns_service.handle_mns_notification(death_notification_message) + + mns_service.update_restrictions.assert_called_once_with( + nhs_number=TEST_NHS_NUMBER, custodian=PatientOdsInactiveStatus.DECEASED, + ) + +@freezegun.freeze_time("2021-04-01") +def test_update_restrictions_uses_restriction_dynamo_service(mns_service, mocker): + MOCK_RESTRICTION = { + "ID": TEST_UUID, + "RestrictedSmartcard": "123456789012", + "NhsNumber": TEST_NHS_NUMBER, + "Custodian": TEST_CURRENT_GP_ODS, + "Created": 1700000000, + "CreatorSmartcard": "223456789022", + "RemoverSmartCard": None, + "IsActive": True, + "LastUpdated": 1700000001, + } + mocker.patch.object(mns_service, "get_updated_gp_ods", return_value=NEW_ODS_CODE) + + mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.return_value = [ + UserRestriction.model_validate(MOCK_RESTRICTION), + ] + + mns_service.update_restrictions(nhs_number=TEST_NHS_NUMBER, custodian=NEW_ODS_CODE) + + mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.assert_called_with( + nhs_number=TEST_NHS_NUMBER, + ) + + mns_service.restrictions_dynamo_service.update_restriction_custodian.assert_called_once_with( + restriction_id=MOCK_RESTRICTION["ID"], updated_custodian=NEW_ODS_CODE, + ) + diff --git a/lambdas/utils/exceptions.py b/lambdas/utils/exceptions.py index 160228ef8f..a359789a63 100644 --- a/lambdas/utils/exceptions.py +++ b/lambdas/utils/exceptions.py @@ -256,4 +256,4 @@ class UserRestrictionException(Exception): class UserRestrictionConditionCheckFailedException(Exception): - pass + pass \ No newline at end of file From f8cb9f38a6f4cabb036741537a202ba06f5cf787 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Mon, 9 Mar 2026 16:14:42 +0000 Subject: [PATCH 14/17] [PRMP-1476] format --- lambdas/handlers/mns_notification_handler.py | 5 +- .../services/process_mns_message_service.py | 64 ++++-- .../user_restriction_dynamo_service.py | 16 +- lambdas/tests/unit/conftest.py | 1 + .../test_process_mns_message_service.py | 194 ++++++++++++------ lambdas/utils/exceptions.py | 2 +- 6 files changed, 198 insertions(+), 84 deletions(-) diff --git a/lambdas/handlers/mns_notification_handler.py b/lambdas/handlers/mns_notification_handler.py index 3b2aec16d0..9e4e797ad2 100644 --- a/lambdas/handlers/mns_notification_handler.py +++ b/lambdas/handlers/mns_notification_handler.py @@ -1,8 +1,9 @@ import json +from pydantic import ValidationError + from enums.mns_notification_types import MNSNotificationTypes from models.sqs.mns_sqs_message import MNSSQSMessage -from pydantic import ValidationError from services.process_mns_message_service import MNSNotificationService from utils.audit_logging_setup import LoggingService from utils.decorators.ensure_env_var import ensure_environment_variables @@ -39,7 +40,7 @@ def lambda_handler(event, context): request_context.patient_nhs_no = mns_message.subject.nhs_number logger.info( - f"Processing SQS message for nhs number: {mns_message.subject.nhs_number}" + f"Processing SQS message for nhs number: {mns_message.subject.nhs_number}", ) if mns_message.type in MNSNotificationTypes.__members__.values(): diff --git a/lambdas/services/process_mns_message_service.py b/lambdas/services/process_mns_message_service.py index 66fc845a74..d82dbf98b2 100644 --- a/lambdas/services/process_mns_message_service.py +++ b/lambdas/services/process_mns_message_service.py @@ -1,6 +1,7 @@ import os from botocore.exceptions import ClientError + from enums.death_notification_status import DeathNotificationStatus from enums.mns_notification_types import MNSNotificationTypes from enums.patient_ods_inactive_status import PatientOdsInactiveStatus @@ -11,7 +12,9 @@ from services.document_reference_service import DocumentReferenceService from services.document_upload_review_service import DocumentUploadReviewService from services.feature_flags_service import FeatureFlagService -from services.user_restrictions.user_restriction_dynamo_service import UserRestrictionDynamoService +from services.user_restrictions.user_restriction_dynamo_service import ( + UserRestrictionDynamoService, +) from utils.audit_logging_setup import LoggingService from utils.exceptions import PdsErrorException from utils.utilities import get_pds_service @@ -42,21 +45,21 @@ def handle_mns_notification(self, message: MNSSQSMessage): except PdsErrorException as e: logger.info("An error occurred when calling PDS") logger.info( - f"Unable to process message: {message.id}, of type: {message.type}" + f"Unable to process message: {message.id}, of type: {message.type}", ) logger.info(f"{e}") raise e except ClientError as e: logger.info( - f"Unable to process message: {message.id}, of type: {message.type}" + f"Unable to process message: {message.id}, of type: {message.type}", ) logger.info(f"{e}") raise e def handle_gp_change_notification(self, message: MNSSQSMessage) -> None: lg_documents, review_documents = self.get_all_patient_documents( - message.subject.nhs_number + message.subject.nhs_number, ) if not lg_documents and not review_documents: @@ -64,11 +67,16 @@ def handle_gp_change_notification(self, message: MNSSQSMessage) -> None: updated_ods_code = self.get_updated_gp_ods(message.subject.nhs_number) self.update_all_patient_documents( - lg_documents, review_documents, updated_ods_code, + lg_documents, + review_documents, + updated_ods_code, ) logger.info("Update complete for change of GP") - self.update_restrictions(nhs_number=message.subject.nhs_number, custodian=updated_ods_code) + self.update_restrictions( + nhs_number=message.subject.nhs_number, + custodian=updated_ods_code, + ) def handle_death_notification(self, message: MNSSQSMessage) -> None: death_notification_type = message.data["deathNotificationStatus"] @@ -77,26 +85,31 @@ def handle_death_notification(self, message: MNSSQSMessage) -> None: match death_notification_type: case DeathNotificationStatus.INFORMAL: logger.info( - "Patient is deceased - INFORMAL, moving on to the next message." + "Patient is deceased - INFORMAL, moving on to the next message.", ) case DeathNotificationStatus.REMOVED: lg_documents, review_documents = self.get_all_patient_documents( - nhs_number + nhs_number, ) if lg_documents or review_documents: updated_ods_code = self.get_updated_gp_ods(nhs_number) self.update_all_patient_documents( - lg_documents, review_documents, updated_ods_code + lg_documents, + review_documents, + updated_ods_code, ) logger.info("Update complete for death notification change.") - self.update_restrictions(nhs_number=nhs_number, custodian=updated_ods_code) + self.update_restrictions( + nhs_number=nhs_number, + custodian=updated_ods_code, + ) case DeathNotificationStatus.FORMAL: lg_documents, review_documents = self.get_all_patient_documents( - nhs_number + nhs_number, ) if lg_documents or review_documents: @@ -106,27 +119,31 @@ def handle_death_notification(self, message: MNSSQSMessage) -> None: PatientOdsInactiveStatus.DECEASED, ) logger.info( - f"Update complete, patient marked {PatientOdsInactiveStatus.DECEASED}." + f"Update complete, patient marked {PatientOdsInactiveStatus.DECEASED}.", ) - self.update_restrictions(nhs_number=nhs_number, custodian=PatientOdsInactiveStatus.DECEASED) + self.update_restrictions( + nhs_number=nhs_number, + custodian=PatientOdsInactiveStatus.DECEASED, + ) def get_updated_gp_ods(self, nhs_number: str) -> str: patient_details = self.pds_service.fetch_patient_details(nhs_number) return patient_details.general_practice_ods def get_all_patient_documents( - self, nhs_number: str + self, + nhs_number: str, ) -> tuple[list[DocumentReference], list[DocumentUploadReviewReference]]: """Fetch patient documents from both LG and document review tables.""" lg_documents = ( self.lg_document_service.fetch_documents_from_table_with_nhs_number( - nhs_number + nhs_number, ) ) review_documents = ( self.document_review_service.fetch_documents_from_table_with_nhs_number( - nhs_number + nhs_number, ) ) @@ -141,20 +158,27 @@ def update_all_patient_documents( """Update documents in both tables if they exist.""" if lg_documents: self.lg_document_service.update_patient_ods_code( - lg_documents, updated_ods_code + lg_documents, + updated_ods_code, ) if review_documents: self.document_review_service.update_document_review_custodian( - review_documents, updated_ods_code + review_documents, + updated_ods_code, ) def update_restrictions(self, nhs_number: str, custodian: str) -> None: - restrictions = self.restrictions_dynamo_service.query_restrictions_by_nhs_number(nhs_number=nhs_number) + restrictions = ( + self.restrictions_dynamo_service.query_restrictions_by_nhs_number( + nhs_number=nhs_number, + ) + ) for restriction in restrictions: self.restrictions_dynamo_service.update_restriction_custodian( - restriction_id=restriction.id, updated_custodian=custodian, + restriction_id=restriction.id, + updated_custodian=custodian, ) logger.info(f"All restrictions for patient {nhs_number} updated.") diff --git a/lambdas/services/user_restrictions/user_restriction_dynamo_service.py b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py index ee29e1f060..d8210204c0 100644 --- a/lambdas/services/user_restrictions/user_restriction_dynamo_service.py +++ b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py @@ -25,6 +25,12 @@ DEFAULT_LIMIT = 10 MAX_LIMIT = 100 +SMARTCARD_KEY = "RestrictedSmartcard" +NHS_NUMBER_KEY = "NhsNumber" +ODS_CODE_GSI = "CustodianIndex" +ODS_CODE_KEY = "Custodian" +NHS_NUMBER_GSI = "NhsNumberIndex" + class DynamoClientErrors(StrEnum): NOT_FOUND = "ResourceNotFoundException" @@ -115,11 +121,15 @@ def update_restriction_inactive( raise e def query_restrictions_by_nhs_number( - self, - nhs_number: str, + self, + nhs_number: str, ): filter_builder = DynamoQueryFilterBuilder() - filter_builder.add_condition(UserRestrictionsFields.IS_ACTIVE.value, AttributeOperator.EQUAL, True) + filter_builder.add_condition( + UserRestrictionsFields.IS_ACTIVE.value, + AttributeOperator.EQUAL, + True, + ) active_filter_expression = filter_builder.build() items = self.dynamo_service.query_table( diff --git a/lambdas/tests/unit/conftest.py b/lambdas/tests/unit/conftest.py index 91e60d4e6c..3f23a7be7b 100644 --- a/lambdas/tests/unit/conftest.py +++ b/lambdas/tests/unit/conftest.py @@ -262,6 +262,7 @@ def set_env(monkeypatch): monkeypatch.setenv("USE_MOCK_HEALTHCARE_SERVICE", "true") + EXPECTED_PARSED_PATIENT_BASE_CASE = PatientDetails( givenName=["Jane"], familyName="Smith", diff --git a/lambdas/tests/unit/services/test_process_mns_message_service.py b/lambdas/tests/unit/services/test_process_mns_message_service.py index e6a9bff9a7..d8b48a7b4c 100644 --- a/lambdas/tests/unit/services/test_process_mns_message_service.py +++ b/lambdas/tests/unit/services/test_process_mns_message_service.py @@ -3,6 +3,7 @@ import freezegun import pytest from botocore.exceptions import ClientError + from enums.patient_ods_inactive_status import PatientOdsInactiveStatus from models.document_reference import DocumentReference from models.document_review import DocumentUploadReviewReference @@ -93,7 +94,9 @@ def mock_document_review_references(mocker): def test_handle_gp_change_message_called_message_type_gp_change( - mns_service, mock_handle_gp_change, mock_handle_death_notification + mns_service, + mock_handle_gp_change, + mock_handle_death_notification, ): mns_service.handle_mns_notification(gp_change_message) @@ -102,7 +105,9 @@ def test_handle_gp_change_message_called_message_type_gp_change( def test_handle_gp_change_message_not_called_message_death_message( - mns_service, mock_handle_death_notification, mock_handle_gp_change + mns_service, + mock_handle_death_notification, + mock_handle_gp_change, ): mns_service.handle_mns_notification(death_notification_message) @@ -123,10 +128,13 @@ def test_handle_mns_notification_error_handling_pds_error(mns_service, mocker): def test_handle_mns_notification_error_handling_client_error(mns_service, mocker): client_error = ClientError( - {"Error": {"Code": "TestException", "Message": "Test exception"}}, "operation" + {"Error": {"Code": "TestException", "Message": "Test exception"}}, + "operation", ) mocker.patch.object( - mns_service, "handle_gp_change_notification", side_effect=client_error + mns_service, + "handle_gp_change_notification", + side_effect=client_error, ) with pytest.raises(ClientError): @@ -134,7 +142,10 @@ def test_handle_mns_notification_error_handling_client_error(mns_service, mocker def test_handle_gp_change_notification_with_patient_documents( - mns_service, mock_document_references, mock_document_review_references, mocker + mns_service, + mock_document_references, + mock_document_review_references, + mocker, ): mocker.patch.object(mns_service, "get_all_patient_documents") mns_service.get_all_patient_documents.return_value = ( @@ -148,13 +159,15 @@ def test_handle_gp_change_notification_with_patient_documents( mns_service.handle_gp_change_notification(gp_change_message) mns_service.get_all_patient_documents.assert_called_once_with( - gp_change_message.subject.nhs_number + gp_change_message.subject.nhs_number, ) mns_service.get_updated_gp_ods.assert_called_once_with( - gp_change_message.subject.nhs_number + gp_change_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_called_once_with( - mock_document_references, mock_document_review_references, NEW_ODS_CODE + mock_document_references, + mock_document_review_references, + NEW_ODS_CODE, ) @@ -167,7 +180,7 @@ def test_handle_gp_change_notification_no_patient_documents(mns_service, mocker) mns_service.handle_gp_change_notification(gp_change_message) mns_service.get_all_patient_documents.assert_called_once_with( - gp_change_message.subject.nhs_number + gp_change_message.subject.nhs_number, ) mns_service.get_updated_gp_ods.assert_not_called() mns_service.update_all_patient_documents.assert_not_called() @@ -186,7 +199,10 @@ def test_handle_death_notification_informal(mns_service, mocker): def test_handle_death_notification_removed_with_documents( - mns_service, mock_document_references, mock_document_review_references, mocker + mns_service, + mock_document_references, + mock_document_review_references, + mocker, ): mocker.patch.object(mns_service, "get_all_patient_documents") mocker.patch.object(mns_service, "get_updated_gp_ods") @@ -200,13 +216,15 @@ def test_handle_death_notification_removed_with_documents( mns_service.handle_death_notification(removed_death_notification_message) mns_service.get_all_patient_documents.assert_called_once_with( - removed_death_notification_message.subject.nhs_number + removed_death_notification_message.subject.nhs_number, ) mns_service.get_updated_gp_ods.assert_called_once_with( - removed_death_notification_message.subject.nhs_number + removed_death_notification_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_called_once_with( - mock_document_references, mock_document_review_references, NEW_ODS_CODE + mock_document_references, + mock_document_review_references, + NEW_ODS_CODE, ) @@ -219,14 +237,17 @@ def test_handle_death_notification_removed_no_documents(mns_service, mocker): mns_service.handle_death_notification(removed_death_notification_message) mns_service.get_all_patient_documents.assert_called_once_with( - removed_death_notification_message.subject.nhs_number + removed_death_notification_message.subject.nhs_number, ) mns_service.get_updated_gp_ods.assert_not_called() mns_service.update_all_patient_documents.assert_not_called() def test_handle_death_notification_formal_with_documents( - mns_service, mock_document_references, mock_document_review_references, mocker + mns_service, + mock_document_references, + mock_document_review_references, + mocker, ): mocker.patch.object(mns_service, "get_all_patient_documents") mocker.patch.object(mns_service, "get_updated_gp_ods") @@ -239,7 +260,7 @@ def test_handle_death_notification_formal_with_documents( mns_service.handle_death_notification(death_notification_message) mns_service.get_all_patient_documents.assert_called_once_with( - death_notification_message.subject.nhs_number + death_notification_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_called_once_with( mock_document_references, @@ -258,7 +279,7 @@ def test_handle_death_notification_formal_no_documents(mns_service, mocker): mns_service.handle_death_notification(death_notification_message) mns_service.get_all_patient_documents.assert_called_once_with( - death_notification_message.subject.nhs_number + death_notification_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_not_called() @@ -273,12 +294,15 @@ def test_get_updated_gp_ods(mns_service): assert result == expected_ods mns_service.pds_service.fetch_patient_details.assert_called_once_with( - TEST_NHS_NUMBER + TEST_NHS_NUMBER, ) def test_pds_is_called_death_notification_removed( - mns_service, mocker, mock_document_references, mock_document_review_references + mns_service, + mocker, + mock_document_references, + mock_document_review_references, ): mocker.patch.object(mns_service, "get_updated_gp_ods") mocker.patch.object(mns_service, "update_all_patient_documents") @@ -310,54 +334,71 @@ def test_get_all_patient_documents(mns_service, mocker): assert lg_docs == expected_lg_docs assert review_docs == expected_review_docs mns_service.lg_document_service.fetch_documents_from_table_with_nhs_number.assert_called_once_with( - TEST_NHS_NUMBER + TEST_NHS_NUMBER, ) mns_service.document_review_service.fetch_documents_from_table_with_nhs_number.assert_called_once_with( - TEST_NHS_NUMBER + TEST_NHS_NUMBER, ) def test_update_all_patient_documents_with_both_types( - mns_service, mock_document_references, mock_document_review_references, mocker + mns_service, + mock_document_references, + mock_document_review_references, + mocker, ): mns_service.update_all_patient_documents( - mock_document_references, mock_document_review_references, NEW_ODS_CODE + mock_document_references, + mock_document_review_references, + NEW_ODS_CODE, ) mns_service.lg_document_service.update_patient_ods_code.assert_called_once_with( - mock_document_references, NEW_ODS_CODE + mock_document_references, + NEW_ODS_CODE, ) mns_service.document_review_service.update_document_review_custodian.assert_called_once_with( - mock_document_review_references, NEW_ODS_CODE + mock_document_review_references, + NEW_ODS_CODE, ) def test_update_all_patient_documents_with_only_lg_documents( - mns_service, mock_document_references, mocker + mns_service, + mock_document_references, + mocker, ): mns_service.update_all_patient_documents(mock_document_references, [], NEW_ODS_CODE) mns_service.lg_document_service.update_patient_ods_code.assert_called_once_with( - mock_document_references, NEW_ODS_CODE + mock_document_references, + NEW_ODS_CODE, ) mns_service.document_review_service.update_document_review_custodian.assert_not_called() def test_update_all_patient_documents_with_only_review_documents( - mns_service, mock_document_review_references, mocker + mns_service, + mock_document_review_references, + mocker, ): mns_service.update_all_patient_documents( - [], mock_document_review_references, NEW_ODS_CODE + [], + mock_document_review_references, + NEW_ODS_CODE, ) mns_service.lg_document_service.update_patient_ods_code.assert_not_called() mns_service.document_review_service.update_document_review_custodian.assert_called_once_with( - mock_document_review_references, NEW_ODS_CODE + mock_document_review_references, + NEW_ODS_CODE, ) def test_handle_gp_change_notification_with_only_lg_documents( - mns_service, mock_document_references, mocker + mns_service, + mock_document_references, + mocker, ): mocker.patch.object(mns_service, "get_all_patient_documents") mns_service.get_all_patient_documents.return_value = ( @@ -371,18 +412,22 @@ def test_handle_gp_change_notification_with_only_lg_documents( mns_service.handle_gp_change_notification(gp_change_message) mns_service.get_all_patient_documents.assert_called_once_with( - gp_change_message.subject.nhs_number + gp_change_message.subject.nhs_number, ) mns_service.get_updated_gp_ods.assert_called_once_with( - gp_change_message.subject.nhs_number + gp_change_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_called_once_with( - mock_document_references, [], NEW_ODS_CODE + mock_document_references, + [], + NEW_ODS_CODE, ) def test_handle_gp_change_notification_with_only_review_documents( - mns_service, mock_document_review_references, mocker + mns_service, + mock_document_review_references, + mocker, ): mocker.patch.object(mns_service, "get_all_patient_documents") mns_service.get_all_patient_documents.return_value = ( @@ -396,18 +441,22 @@ def test_handle_gp_change_notification_with_only_review_documents( mns_service.handle_gp_change_notification(gp_change_message) mns_service.get_all_patient_documents.assert_called_once_with( - gp_change_message.subject.nhs_number + gp_change_message.subject.nhs_number, ) mns_service.get_updated_gp_ods.assert_called_once_with( - gp_change_message.subject.nhs_number + gp_change_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_called_once_with( - [], mock_document_review_references, NEW_ODS_CODE + [], + mock_document_review_references, + NEW_ODS_CODE, ) def test_handle_death_notification_formal_with_only_lg_documents( - mns_service, mock_document_references, mocker + mns_service, + mock_document_references, + mocker, ): mocker.patch.object(mns_service, "get_all_patient_documents") mocker.patch.object(mns_service, "get_updated_gp_ods") @@ -420,7 +469,7 @@ def test_handle_death_notification_formal_with_only_lg_documents( mns_service.handle_death_notification(death_notification_message) mns_service.get_all_patient_documents.assert_called_once_with( - death_notification_message.subject.nhs_number + death_notification_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_called_once_with( mock_document_references, @@ -431,7 +480,9 @@ def test_handle_death_notification_formal_with_only_lg_documents( def test_handle_death_notification_formal_with_only_review_documents( - mns_service, mock_document_review_references, mocker + mns_service, + mock_document_review_references, + mocker, ): mocker.patch.object(mns_service, "get_all_patient_documents") mocker.patch.object(mns_service, "get_updated_gp_ods") @@ -444,7 +495,7 @@ def test_handle_death_notification_formal_with_only_review_documents( mns_service.handle_death_notification(death_notification_message) mns_service.get_all_patient_documents.assert_called_once_with( - death_notification_message.subject.nhs_number + death_notification_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_called_once_with( [], @@ -455,7 +506,9 @@ def test_handle_death_notification_formal_with_only_review_documents( def test_handle_death_notification_removed_with_only_lg_documents( - mns_service, mock_document_references, mocker + mns_service, + mock_document_references, + mocker, ): mocker.patch.object(mns_service, "get_all_patient_documents") mocker.patch.object(mns_service, "get_updated_gp_ods") @@ -469,18 +522,22 @@ def test_handle_death_notification_removed_with_only_lg_documents( mns_service.handle_death_notification(removed_death_notification_message) mns_service.get_all_patient_documents.assert_called_once_with( - removed_death_notification_message.subject.nhs_number + removed_death_notification_message.subject.nhs_number, ) mns_service.get_updated_gp_ods.assert_called_once_with( - removed_death_notification_message.subject.nhs_number + removed_death_notification_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_called_once_with( - mock_document_references, [], NEW_ODS_CODE + mock_document_references, + [], + NEW_ODS_CODE, ) def test_handle_death_notification_removed_with_only_review_documents( - mns_service, mock_document_review_references, mocker + mns_service, + mock_document_review_references, + mocker, ): mocker.patch.object(mns_service, "get_all_patient_documents") mocker.patch.object(mns_service, "get_updated_gp_ods") @@ -494,35 +551,56 @@ def test_handle_death_notification_removed_with_only_review_documents( mns_service.handle_death_notification(removed_death_notification_message) mns_service.get_all_patient_documents.assert_called_once_with( - removed_death_notification_message.subject.nhs_number + removed_death_notification_message.subject.nhs_number, ) mns_service.get_updated_gp_ods.assert_called_once_with( - removed_death_notification_message.subject.nhs_number + removed_death_notification_message.subject.nhs_number, ) mns_service.update_all_patient_documents.assert_called_once_with( - [], mock_document_review_references, NEW_ODS_CODE + [], + mock_document_review_references, + NEW_ODS_CODE, ) -@pytest.mark.parametrize("mns_event", [removed_death_notification_message, gp_change_message]) -def test_handle_mns_notification_calls_update_restrictions_called_on_notification(mns_service, mns_event, mocker): +@pytest.mark.parametrize( + "mns_event", + [removed_death_notification_message, gp_change_message], +) +def test_handle_mns_notification_calls_update_restrictions_called_on_notification( + mns_service, + mns_event, + mocker, +): mocker.patch.object(mns_service, "update_restrictions") - updated_ods = mocker.patch.object(mns_service, "get_updated_gp_ods", return_value=TEST_CURRENT_GP_ODS) + updated_ods = mocker.patch.object( + mns_service, + "get_updated_gp_ods", + return_value=TEST_CURRENT_GP_ODS, + ) mns_service.handle_mns_notification(mns_event) - mns_service.update_restrictions.assert_called_with(nhs_number=TEST_NHS_NUMBER, custodian=updated_ods.return_value) + mns_service.update_restrictions.assert_called_with( + nhs_number=TEST_NHS_NUMBER, + custodian=updated_ods.return_value, + ) -def test_handle_mns_notification_calls_update_restriction_called_on_death_notification(mns_service, mocker): +def test_handle_mns_notification_calls_update_restriction_called_on_death_notification( + mns_service, + mocker, +): mocker.patch.object(mns_service, "update_restrictions") mns_service.handle_mns_notification(death_notification_message) mns_service.update_restrictions.assert_called_once_with( - nhs_number=TEST_NHS_NUMBER, custodian=PatientOdsInactiveStatus.DECEASED, + nhs_number=TEST_NHS_NUMBER, + custodian=PatientOdsInactiveStatus.DECEASED, ) + @freezegun.freeze_time("2021-04-01") def test_update_restrictions_uses_restriction_dynamo_service(mns_service, mocker): MOCK_RESTRICTION = { @@ -549,6 +627,6 @@ def test_update_restrictions_uses_restriction_dynamo_service(mns_service, mocker ) mns_service.restrictions_dynamo_service.update_restriction_custodian.assert_called_once_with( - restriction_id=MOCK_RESTRICTION["ID"], updated_custodian=NEW_ODS_CODE, + restriction_id=MOCK_RESTRICTION["ID"], + updated_custodian=NEW_ODS_CODE, ) - diff --git a/lambdas/utils/exceptions.py b/lambdas/utils/exceptions.py index a359789a63..160228ef8f 100644 --- a/lambdas/utils/exceptions.py +++ b/lambdas/utils/exceptions.py @@ -256,4 +256,4 @@ class UserRestrictionException(Exception): class UserRestrictionConditionCheckFailedException(Exception): - pass \ No newline at end of file + pass From e3b264159fe877ed6861d6ba4216f9623818cdd2 Mon Sep 17 00:00:00 2001 From: NogaNHS <127490765+NogaNHS@users.noreply.github.com> Date: Fri, 20 Mar 2026 10:35:29 +0000 Subject: [PATCH 15/17] [PRMP-1476] Implement user restrictions handling in MNS message processing Co-authored-by: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> --- .../user_restrictions/user_restrictions.py | 5 +- .../services/process_mns_message_service.py | 113 ++++++++++----- .../user_restriction_dynamo_service.py | 46 +++--- lambdas/tests/unit/conftest.py | 1 - .../test_process_mns_message_service.py | 134 +++++++++++++++--- .../test_user_restriction_dynamo_service.py | 46 +++++- lambdas/utils/exceptions.py | 4 + 7 files changed, 272 insertions(+), 77 deletions(-) diff --git a/lambdas/models/user_restrictions/user_restrictions.py b/lambdas/models/user_restrictions/user_restrictions.py index c38e80b1b4..210fb7cea6 100644 --- a/lambdas/models/user_restrictions/user_restrictions.py +++ b/lambdas/models/user_restrictions/user_restrictions.py @@ -10,11 +10,12 @@ class UserRestrictionsFields(StrEnum): ID = "ID" CREATOR = "CreatorSmartcard" RESTRICTED_USER = "RestrictedSmartcard" - REMOVED_BY = "RemoverSmartCard" - CUSTODIAN = "Custodian" + REMOVED_BY = "RemoverSmartcard" NHS_NUMBER = "NhsNumber" + CUSTODIAN = "Custodian" IS_ACTIVE = "IsActive" LAST_UPDATED = "LastUpdated" + CREATED = "Created" class UserRestrictionIndexes(StrEnum): diff --git a/lambdas/services/process_mns_message_service.py b/lambdas/services/process_mns_message_service.py index d82dbf98b2..6ed5be41aa 100644 --- a/lambdas/services/process_mns_message_service.py +++ b/lambdas/services/process_mns_message_service.py @@ -3,11 +3,13 @@ from botocore.exceptions import ClientError from enums.death_notification_status import DeathNotificationStatus +from enums.feature_flags import FeatureFlags from enums.mns_notification_types import MNSNotificationTypes from enums.patient_ods_inactive_status import PatientOdsInactiveStatus from models.document_reference import DocumentReference from models.document_review import DocumentUploadReviewReference from models.sqs.mns_sqs_message import MNSSQSMessage +from models.user_restrictions.user_restrictions import UserRestriction from services.base.sqs_service import SQSService from services.document_reference_service import DocumentReferenceService from services.document_upload_review_service import DocumentUploadReviewService @@ -62,21 +64,33 @@ def handle_gp_change_notification(self, message: MNSSQSMessage) -> None: message.subject.nhs_number, ) - if not lg_documents and not review_documents: - return - - updated_ods_code = self.get_updated_gp_ods(message.subject.nhs_number) - self.update_all_patient_documents( - lg_documents, - review_documents, - updated_ods_code, + restrictions = ( + ( + self.restrictions_dynamo_service.query_restrictions_by_nhs_number( + nhs_number=message.subject.nhs_number, + ) + ) + if self.is_restrictions_enabled() + else None ) - logger.info("Update complete for change of GP") - self.update_restrictions( - nhs_number=message.subject.nhs_number, - custodian=updated_ods_code, - ) + if lg_documents or review_documents or restrictions: + updated_ods_code = self.get_updated_gp_ods(message.subject.nhs_number) + + if lg_documents or review_documents: + self.update_all_patient_documents( + lg_documents, + review_documents, + updated_ods_code, + ) + logger.info("Update complete for change of GP") + + if restrictions: + self.update_restrictions( + nhs_number=message.subject.nhs_number, + custodian=updated_ods_code, + restrictions=restrictions, + ) def handle_death_notification(self, message: MNSSQSMessage) -> None: death_notification_type = message.data["deathNotificationStatus"] @@ -93,25 +107,49 @@ def handle_death_notification(self, message: MNSSQSMessage) -> None: nhs_number, ) - if lg_documents or review_documents: - updated_ods_code = self.get_updated_gp_ods(nhs_number) - self.update_all_patient_documents( - lg_documents, - review_documents, - updated_ods_code, + restrictions = ( + ( + self.restrictions_dynamo_service.query_restrictions_by_nhs_number( + nhs_number=nhs_number, + ) ) - logger.info("Update complete for death notification change.") + if self.is_restrictions_enabled() + else None + ) - self.update_restrictions( - nhs_number=nhs_number, - custodian=updated_ods_code, - ) + if lg_documents or review_documents or restrictions: + updated_ods_code = self.get_updated_gp_ods(nhs_number) + + if lg_documents or review_documents: + self.update_all_patient_documents( + lg_documents, + review_documents, + updated_ods_code, + ) + logger.info("Update complete for death notification change.") + + if restrictions: + self.update_restrictions( + nhs_number=nhs_number, + custodian=updated_ods_code, + restrictions=restrictions, + ) case DeathNotificationStatus.FORMAL: lg_documents, review_documents = self.get_all_patient_documents( nhs_number, ) + restrictions = ( + ( + self.restrictions_dynamo_service.query_restrictions_by_nhs_number( + nhs_number=nhs_number, + ) + ) + if self.is_restrictions_enabled() + else None + ) + if lg_documents or review_documents: self.update_all_patient_documents( lg_documents, @@ -122,9 +160,11 @@ def handle_death_notification(self, message: MNSSQSMessage) -> None: f"Update complete, patient marked {PatientOdsInactiveStatus.DECEASED}.", ) + if restrictions: self.update_restrictions( nhs_number=nhs_number, custodian=PatientOdsInactiveStatus.DECEASED, + restrictions=restrictions, ) def get_updated_gp_ods(self, nhs_number: str) -> str: @@ -167,18 +207,27 @@ def update_all_patient_documents( updated_ods_code, ) - def update_restrictions(self, nhs_number: str, custodian: str) -> None: - - restrictions = ( - self.restrictions_dynamo_service.query_restrictions_by_nhs_number( - nhs_number=nhs_number, - ) - ) - + def update_restrictions( + self, + nhs_number: str, + custodian: str, + restrictions: list[UserRestriction], + ) -> None: for restriction in restrictions: + logger.info(f"Updating restriction {restriction.id}") self.restrictions_dynamo_service.update_restriction_custodian( restriction_id=restriction.id, updated_custodian=custodian, ) logger.info(f"All restrictions for patient {nhs_number} updated.") + + def is_restrictions_enabled(self) -> bool: + restrictions_feature_flag = self.feature_flag_service.get_feature_flags_by_flag( + FeatureFlags.USER_RESTRICTION_ENABLED, + ) + + return restrictions_feature_flag.get( + FeatureFlags.USER_RESTRICTION_ENABLED, + False, + ) diff --git a/lambdas/services/user_restrictions/user_restriction_dynamo_service.py b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py index d8210204c0..60a04c3ad2 100644 --- a/lambdas/services/user_restrictions/user_restriction_dynamo_service.py +++ b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py @@ -17,6 +17,7 @@ from utils.dynamo_utils import build_mixed_condition_expression from utils.exceptions import ( UserRestrictionConditionCheckFailedException, + UserRestrictionDynamoDBException, UserRestrictionValidationException, ) @@ -25,12 +26,6 @@ DEFAULT_LIMIT = 10 MAX_LIMIT = 100 -SMARTCARD_KEY = "RestrictedSmartcard" -NHS_NUMBER_KEY = "NhsNumber" -ODS_CODE_GSI = "CustodianIndex" -ODS_CODE_KEY = "Custodian" -NHS_NUMBER_GSI = "NhsNumberIndex" - class DynamoClientErrors(StrEnum): NOT_FOUND = "ResourceNotFoundException" @@ -123,23 +118,32 @@ def update_restriction_inactive( def query_restrictions_by_nhs_number( self, nhs_number: str, - ): - filter_builder = DynamoQueryFilterBuilder() - filter_builder.add_condition( - UserRestrictionsFields.IS_ACTIVE.value, - AttributeOperator.EQUAL, - True, - ) - active_filter_expression = filter_builder.build() + ) -> list[UserRestriction]: + try: + logger.info("Building IsActive filter for DynamoDB query.") + filter_builder = DynamoQueryFilterBuilder() + filter_builder.add_condition( + UserRestrictionsFields.IS_ACTIVE.value, + AttributeOperator.EQUAL, + True, + ) + active_filter_expression = filter_builder.build() - items = self.dynamo_service.query_table( - table_name=self.table_name, - index_name=UserRestrictionIndexes.NHS_NUMBER_INDEX, - search_key=nhs_number, - query_filter=active_filter_expression, - ) + logger.info("Querying Restrictions by NHS Number.") + items = self.dynamo_service.query_table( + table_name=self.table_name, + index_name=UserRestrictionIndexes.NHS_NUMBER_INDEX.value, + search_key=UserRestrictionsFields.NHS_NUMBER.value, + search_condition=nhs_number, + query_filter=active_filter_expression, + ) - return self._validate_restrictions(items) + return self._validate_restrictions(items) + except ClientError as e: + logger.error(e) + raise UserRestrictionDynamoDBException( + "An issue occurred while querying restrictions", + ) def update_restriction_custodian(self, restriction_id: str, updated_custodian: str): try: diff --git a/lambdas/tests/unit/conftest.py b/lambdas/tests/unit/conftest.py index 3f23a7be7b..91e60d4e6c 100644 --- a/lambdas/tests/unit/conftest.py +++ b/lambdas/tests/unit/conftest.py @@ -262,7 +262,6 @@ def set_env(monkeypatch): monkeypatch.setenv("USE_MOCK_HEALTHCARE_SERVICE", "true") - EXPECTED_PARSED_PATIENT_BASE_CASE = PatientDetails( givenName=["Jane"], familyName="Smith", diff --git a/lambdas/tests/unit/services/test_process_mns_message_service.py b/lambdas/tests/unit/services/test_process_mns_message_service.py index d8b48a7b4c..1fec31f47e 100644 --- a/lambdas/tests/unit/services/test_process_mns_message_service.py +++ b/lambdas/tests/unit/services/test_process_mns_message_service.py @@ -4,11 +4,13 @@ import pytest from botocore.exceptions import ClientError +from enums.feature_flags import FeatureFlags from enums.patient_ods_inactive_status import PatientOdsInactiveStatus from models.document_reference import DocumentReference from models.document_review import DocumentUploadReviewReference from models.sqs.mns_sqs_message import MNSSQSMessage from models.user_restrictions.user_restrictions import UserRestriction +from services.feature_flags_service import FeatureFlagService from services.process_mns_message_service import MNSNotificationService from tests.unit.conftest import TEST_CURRENT_GP_ODS, TEST_NHS_NUMBER, TEST_UUID from tests.unit.handlers.test_mns_notification_handler import ( @@ -21,7 +23,40 @@ @pytest.fixture -def mns_service(mocker, set_env, monkeypatch): +def mock_user_restriction_disabled(mocker): + mock_function = mocker.patch.object(FeatureFlagService, "get_feature_flags_by_flag") + mock_feature_flag = mock_function.return_value = { + FeatureFlags.USER_RESTRICTION_ENABLED: False, + } + yield mock_feature_flag + + +@pytest.fixture +def mock_user_restriction_enabled(mocker): + mock_function = mocker.patch.object(FeatureFlagService, "get_feature_flags_by_flag") + mock_feature_flag = mock_function.return_value = { + FeatureFlags.USER_RESTRICTION_ENABLED: True, + } + yield mock_feature_flag + + +MOCK_RESTRICTION_DICT = { + "ID": TEST_UUID, + "RestrictedSmartcard": "123456789012", + "NhsNumber": TEST_NHS_NUMBER, + "Custodian": TEST_CURRENT_GP_ODS, + "Created": 1700000000, + "CreatorSmartcard": "223456789022", + "RemoverSmartCard": None, + "IsActive": True, + "LastUpdated": 1700000001, +} + +MOCK_RESTRICTION = UserRestriction.model_validate(MOCK_RESTRICTION_DICT) + + +@pytest.fixture +def mns_service(mocker, set_env, monkeypatch, mock_user_restriction_enabled): monkeypatch.setenv("PDS_FHIR_IS_STUBBED", "False") service = MNSNotificationService() mocker.patch.object(service, "pds_service") @@ -33,14 +68,19 @@ def mns_service(mocker, set_env, monkeypatch): @pytest.fixture -def mns_service_feature_disabled(mocker, set_env, monkeypatch): +def mns_service_restrictions_feature_disabled( + mocker, + set_env, + monkeypatch, + mock_user_restriction_disabled, +): monkeypatch.setenv("PDS_FHIR_IS_STUBBED", "False") service = MNSNotificationService() mocker.patch.object(service, "pds_service") mocker.patch.object(service, "document_review_service") mocker.patch.object(service, "lg_document_service") mocker.patch.object(service, "sqs_service") - mocker.patch.object(service, "update_restrictions") + mocker.patch.object(service, "restrictions_dynamo_service") yield service @@ -174,6 +214,9 @@ def test_handle_gp_change_notification_with_patient_documents( def test_handle_gp_change_notification_no_patient_documents(mns_service, mocker): mocker.patch.object(mns_service, "get_all_patient_documents") mns_service.get_all_patient_documents.return_value = ([], []) + mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.return_value = ( + [] + ) mocker.patch.object(mns_service, "get_updated_gp_ods") mocker.patch.object(mns_service, "update_all_patient_documents") @@ -233,6 +276,9 @@ def test_handle_death_notification_removed_no_documents(mns_service, mocker): mocker.patch.object(mns_service, "get_updated_gp_ods") mocker.patch.object(mns_service, "update_all_patient_documents") mns_service.get_all_patient_documents.return_value = ([], []) + mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.return_value = ( + [] + ) mns_service.handle_death_notification(removed_death_notification_message) @@ -318,6 +364,28 @@ def test_pds_is_called_death_notification_removed( mns_service.update_all_patient_documents.assert_called() +@pytest.mark.parametrize( + "mns_event", + [removed_death_notification_message, gp_change_message], +) +def test_pds_called_only_restrictions_present( + mns_service, + mocker, + mns_event, +): + mocker.patch.object(mns_service, "get_updated_gp_ods") + mocker.patch.object(mns_service, "get_all_patient_documents") + mns_service.get_all_patient_documents.return_value = ([], []) + + mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.return_value = [ + MOCK_RESTRICTION, + ] + + mns_service.handle_mns_notification(mns_event) + + mns_service.get_updated_gp_ods.assert_called() + + def test_get_all_patient_documents(mns_service, mocker): expected_lg_docs = [MagicMock(spec=DocumentReference)] expected_review_docs = [MagicMock(spec=DocumentUploadReviewReference)] @@ -572,6 +640,9 @@ def test_handle_mns_notification_calls_update_restrictions_called_on_notificatio mns_event, mocker, ): + mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.return_value = [ + MOCK_RESTRICTION, + ] mocker.patch.object(mns_service, "update_restrictions") updated_ods = mocker.patch.object( mns_service, @@ -584,6 +655,7 @@ def test_handle_mns_notification_calls_update_restrictions_called_on_notificatio mns_service.update_restrictions.assert_called_with( nhs_number=TEST_NHS_NUMBER, custodian=updated_ods.return_value, + restrictions=[MOCK_RESTRICTION], ) @@ -591,42 +663,66 @@ def test_handle_mns_notification_calls_update_restriction_called_on_death_notifi mns_service, mocker, ): + mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.return_value = [ + MOCK_RESTRICTION, + ] mocker.patch.object(mns_service, "update_restrictions") mns_service.handle_mns_notification(death_notification_message) + mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.assert_called_with( + nhs_number=TEST_NHS_NUMBER, + ) + mns_service.update_restrictions.assert_called_once_with( nhs_number=TEST_NHS_NUMBER, custodian=PatientOdsInactiveStatus.DECEASED, + restrictions=[MOCK_RESTRICTION], ) @freezegun.freeze_time("2021-04-01") def test_update_restrictions_uses_restriction_dynamo_service(mns_service, mocker): - MOCK_RESTRICTION = { - "ID": TEST_UUID, - "RestrictedSmartcard": "123456789012", - "NhsNumber": TEST_NHS_NUMBER, - "Custodian": TEST_CURRENT_GP_ODS, - "Created": 1700000000, - "CreatorSmartcard": "223456789022", - "RemoverSmartCard": None, - "IsActive": True, - "LastUpdated": 1700000001, - } mocker.patch.object(mns_service, "get_updated_gp_ods", return_value=NEW_ODS_CODE) mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.return_value = [ - UserRestriction.model_validate(MOCK_RESTRICTION), + MOCK_RESTRICTION, ] - mns_service.update_restrictions(nhs_number=TEST_NHS_NUMBER, custodian=NEW_ODS_CODE) - - mns_service.restrictions_dynamo_service.query_restrictions_by_nhs_number.assert_called_with( + mns_service.update_restrictions( nhs_number=TEST_NHS_NUMBER, + custodian=NEW_ODS_CODE, + restrictions=[MOCK_RESTRICTION], ) mns_service.restrictions_dynamo_service.update_restriction_custodian.assert_called_once_with( - restriction_id=MOCK_RESTRICTION["ID"], + restriction_id=MOCK_RESTRICTION.id, updated_custodian=NEW_ODS_CODE, ) + + +@pytest.mark.parametrize( + "mns_event", + [death_notification_message, gp_change_message, removed_death_notification_message], +) +def test_update_restrictions_does_not_call_dynamodb_feature_flag_disabled( + mns_service_restrictions_feature_disabled, + mns_event, +): + mns_service_restrictions_feature_disabled.handle_mns_notification( + mns_event, + ) + + mns_service_restrictions_feature_disabled.restrictions_dynamo_service.query_restrictions_by_nhs_number.assert_not_called() + mns_service_restrictions_feature_disabled.restrictions_dynamo_service.update_restriction_custodian.assert_not_called() + + +def test_update_restrictions_not_called_on_informal_death_notification( + mns_service, + mocker, +): + mock_update_restriction = mocker.patch.object(mns_service, "update_restrictions") + + mns_service.handle_mns_notification(informal_death_notification_message) + + mock_update_restriction.assert_not_called() diff --git a/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py b/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py index 8fb8499887..3ea664a018 100644 --- a/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py +++ b/lambdas/tests/unit/services/user_restriction/test_user_restriction_dynamo_service.py @@ -1,4 +1,5 @@ import pytest +from boto3.dynamodb.conditions import Attr from botocore.exceptions import ClientError from freezegun import freeze_time from pydantic import ValidationError @@ -11,16 +12,21 @@ from services.user_restrictions.user_restriction_dynamo_service import ( UserRestrictionDynamoService, ) -from tests.unit.conftest import TEST_CURRENT_GP_ODS, TEST_NHS_NUMBER, TEST_UUID +from tests.unit.conftest import ( + MOCK_USER_RESTRICTION_TABLE, + TEST_CURRENT_GP_ODS, + TEST_NHS_NUMBER, + TEST_UUID, +) from tests.unit.services.user_restriction.conftest import MOCK_IDENTIFIER from utils.exceptions import ( UserRestrictionConditionCheckFailedException, + UserRestrictionDynamoDBException, UserRestrictionValidationException, ) TEST_ODS_CODE = "Y12345" TEST_SMART_CARD_ID = "SC001" -MOCK_USER_RESTRICTION_TABLE = "test_user_restriction_table" TEST_NEXT_TOKEN = "some-opaque-next-token" MOCK_TIME_STAMP = 1704110400 @@ -145,6 +151,42 @@ def test_query_restrictions_returns_empty_list_when_no_items(mock_service): assert next_token is None +def test_query_restrictions_by_nhs_number(mock_service, mocker): + mock_validate = mocker.patch.object(mock_service, "_validate_restrictions") + mock_service.dynamo_service.query_table.return_value = [MOCK_RESTRICTION_ITEM] + + expected_filter = Attr("IsActive").eq(True) + + mock_service.query_restrictions_by_nhs_number(nhs_number=TEST_NHS_NUMBER) + + mock_service.dynamo_service.query_table.assert_called_with( + table_name=MOCK_USER_RESTRICTION_TABLE, + index_name=UserRestrictionIndexes.NHS_NUMBER_INDEX.value, + search_key=UserRestrictionsFields.NHS_NUMBER.value, + search_condition=TEST_NHS_NUMBER, + query_filter=expected_filter, + ) + + mock_validate.assert_called_with([MOCK_RESTRICTION_ITEM]) + + +def test_query_restrictions_by_nhs_number_handles_client_error(mock_service): + mock_service.dynamo_service.query_table.side_effect = ClientError( + {"Error": {"Code": "500", "Message": "DynamoDB error"}}, + "query", + ) + + with pytest.raises(UserRestrictionDynamoDBException): + mock_service.query_restrictions_by_nhs_number(nhs_number=TEST_NHS_NUMBER) + + +def test_query_restrictions_by_nhs_number_handles_validation_error(mock_service): + mock_service.dynamo_service.query_table.return_value = [{"invalid": "object"}] + + with pytest.raises(UserRestrictionValidationException): + mock_service.query_restrictions_by_nhs_number(nhs_number=TEST_NHS_NUMBER) + + def test_validate_restrictions_raises_for_invalid_items(): with pytest.raises(UserRestrictionValidationException): UserRestrictionDynamoService._validate_restrictions([{"invalid": "data"}]) diff --git a/lambdas/utils/exceptions.py b/lambdas/utils/exceptions.py index 160228ef8f..e73c9e0805 100644 --- a/lambdas/utils/exceptions.py +++ b/lambdas/utils/exceptions.py @@ -257,3 +257,7 @@ class UserRestrictionException(Exception): class UserRestrictionConditionCheckFailedException(Exception): pass + + +class UserRestrictionDynamoDBException(Exception): + pass From fdc1f1ffcd5620c0fd579f1efac47613fd13bf67 Mon Sep 17 00:00:00 2001 From: NogaNHS <127490765+NogaNHS@users.noreply.github.com> Date: Fri, 20 Mar 2026 14:03:56 +0000 Subject: [PATCH 16/17] Refactor MNS message processing --- .../services/process_mns_message_service.py | 180 +++++++----------- .../user_restriction_dynamo_service.py | 58 +++--- .../test_process_mns_message_service.py | 16 -- 3 files changed, 104 insertions(+), 150 deletions(-) diff --git a/lambdas/services/process_mns_message_service.py b/lambdas/services/process_mns_message_service.py index 6ed5be41aa..ff88df0052 100644 --- a/lambdas/services/process_mns_message_service.py +++ b/lambdas/services/process_mns_message_service.py @@ -3,7 +3,6 @@ from botocore.exceptions import ClientError from enums.death_notification_status import DeathNotificationStatus -from enums.feature_flags import FeatureFlags from enums.mns_notification_types import MNSNotificationTypes from enums.patient_ods_inactive_status import PatientOdsInactiveStatus from models.document_reference import DocumentReference @@ -13,7 +12,6 @@ from services.base.sqs_service import SQSService from services.document_reference_service import DocumentReferenceService from services.document_upload_review_service import DocumentUploadReviewService -from services.feature_flags_service import FeatureFlagService from services.user_restrictions.user_restriction_dynamo_service import ( UserRestrictionDynamoService, ) @@ -31,7 +29,6 @@ def __init__(self): self.pds_service = get_pds_service() self.sqs_service = SQSService() self.queue = os.getenv("MNS_NOTIFICATION_QUEUE_URL") - self.feature_flag_service = FeatureFlagService() self.restrictions_dynamo_service = UserRestrictionDynamoService() def handle_mns_notification(self, message: MNSSQSMessage): @@ -43,54 +40,31 @@ def handle_mns_notification(self, message: MNSSQSMessage): case MNSNotificationTypes.DEATH_NOTIFICATION: logger.info("Handling death status notification.") self.handle_death_notification(message) - - except PdsErrorException as e: - logger.info("An error occurred when calling PDS") - logger.info( - f"Unable to process message: {message.id}, of type: {message.type}", - ) - logger.info(f"{e}") - raise e - - except ClientError as e: - logger.info( + except (PdsErrorException, ClientError) as e: + logger.error( f"Unable to process message: {message.id}, of type: {message.type}", ) - logger.info(f"{e}") - raise e + logger.error(str(e)) + raise def handle_gp_change_notification(self, message: MNSSQSMessage) -> None: - lg_documents, review_documents = self.get_all_patient_documents( - message.subject.nhs_number, - ) - - restrictions = ( - ( - self.restrictions_dynamo_service.query_restrictions_by_nhs_number( - nhs_number=message.subject.nhs_number, - ) - ) - if self.is_restrictions_enabled() - else None + nhs_number = message.subject.nhs_number + lg_documents, review_documents, restrictions = self._fetch_patient_data( + nhs_number, ) - if lg_documents or review_documents or restrictions: - updated_ods_code = self.get_updated_gp_ods(message.subject.nhs_number) - - if lg_documents or review_documents: - self.update_all_patient_documents( - lg_documents, - review_documents, - updated_ods_code, - ) - logger.info("Update complete for change of GP") + if not (lg_documents or review_documents or restrictions): + return - if restrictions: - self.update_restrictions( - nhs_number=message.subject.nhs_number, - custodian=updated_ods_code, - restrictions=restrictions, - ) + updated_ods_code = self.get_updated_gp_ods(nhs_number) + self._apply_ods_update( + nhs_number, + lg_documents, + review_documents, + restrictions, + updated_ods_code, + ) + logger.info("Update complete for change of GP.") def handle_death_notification(self, message: MNSSQSMessage) -> None: death_notification_type = message.data["deathNotificationStatus"] @@ -103,69 +77,70 @@ def handle_death_notification(self, message: MNSSQSMessage) -> None: ) case DeathNotificationStatus.REMOVED: - lg_documents, review_documents = self.get_all_patient_documents( + lg_documents, review_documents, restrictions = self._fetch_patient_data( nhs_number, ) - restrictions = ( - ( - self.restrictions_dynamo_service.query_restrictions_by_nhs_number( - nhs_number=nhs_number, - ) - ) - if self.is_restrictions_enabled() - else None - ) - - if lg_documents or review_documents or restrictions: - updated_ods_code = self.get_updated_gp_ods(nhs_number) - - if lg_documents or review_documents: - self.update_all_patient_documents( - lg_documents, - review_documents, - updated_ods_code, - ) - logger.info("Update complete for death notification change.") + if not (lg_documents or review_documents or restrictions): + return - if restrictions: - self.update_restrictions( - nhs_number=nhs_number, - custodian=updated_ods_code, - restrictions=restrictions, - ) + updated_ods_code = self.get_updated_gp_ods(nhs_number) + self._apply_ods_update( + nhs_number, + lg_documents, + review_documents, + restrictions, + updated_ods_code, + ) + logger.info("Update complete for death notification change.") case DeathNotificationStatus.FORMAL: - lg_documents, review_documents = self.get_all_patient_documents( + lg_documents, review_documents, restrictions = self._fetch_patient_data( nhs_number, ) - - restrictions = ( - ( - self.restrictions_dynamo_service.query_restrictions_by_nhs_number( - nhs_number=nhs_number, - ) - ) - if self.is_restrictions_enabled() - else None + self._apply_ods_update( + nhs_number, + lg_documents, + review_documents, + restrictions, + PatientOdsInactiveStatus.DECEASED, + ) + logger.info( + f"Update complete, patient marked {PatientOdsInactiveStatus.DECEASED}.", ) - if lg_documents or review_documents: - self.update_all_patient_documents( - lg_documents, - review_documents, - PatientOdsInactiveStatus.DECEASED, - ) - logger.info( - f"Update complete, patient marked {PatientOdsInactiveStatus.DECEASED}.", - ) - - if restrictions: - self.update_restrictions( - nhs_number=nhs_number, - custodian=PatientOdsInactiveStatus.DECEASED, - restrictions=restrictions, - ) + def _fetch_patient_data( + self, + nhs_number: str, + ) -> tuple[ + list[DocumentReference], + list[DocumentUploadReviewReference], + list[UserRestriction], + ]: + lg_documents, review_documents = self.get_all_patient_documents(nhs_number) + restrictions = ( + self.restrictions_dynamo_service.query_restrictions_by_nhs_number( + nhs_number=nhs_number, + ) + ) + return lg_documents, review_documents, restrictions + + def _apply_ods_update( + self, + nhs_number: str, + lg_documents: list[DocumentReference], + review_documents: list[DocumentUploadReviewReference], + restrictions: list[UserRestriction], + ods_code: str, + ) -> None: + if lg_documents or review_documents: + self.update_all_patient_documents(lg_documents, review_documents, ods_code) + if restrictions: + self.update_restrictions( + nhs_number=nhs_number, + custodian=ods_code, + restrictions=restrictions, + ) def get_updated_gp_ods(self, nhs_number: str) -> str: patient_details = self.pds_service.fetch_patient_details(nhs_number) @@ -175,7 +150,6 @@ def get_all_patient_documents( self, nhs_number: str, ) -> tuple[list[DocumentReference], list[DocumentUploadReviewReference]]: - """Fetch patient documents from both LG and document review tables.""" lg_documents = ( self.lg_document_service.fetch_documents_from_table_with_nhs_number( nhs_number, @@ -221,13 +195,3 @@ def update_restrictions( ) logger.info(f"All restrictions for patient {nhs_number} updated.") - - def is_restrictions_enabled(self) -> bool: - restrictions_feature_flag = self.feature_flag_service.get_feature_flags_by_flag( - FeatureFlags.USER_RESTRICTION_ENABLED, - ) - - return restrictions_feature_flag.get( - FeatureFlags.USER_RESTRICTION_ENABLED, - False, - ) diff --git a/lambdas/services/user_restrictions/user_restriction_dynamo_service.py b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py index 60a04c3ad2..269af1d563 100644 --- a/lambdas/services/user_restrictions/user_restriction_dynamo_service.py +++ b/lambdas/services/user_restrictions/user_restriction_dynamo_service.py @@ -78,30 +78,31 @@ def update_restriction_inactive( removed_by: str, patient_id: str, ): - try: - logger.info("Updating user restriction inactive.") - current_time = int(datetime.now(timezone.utc).timestamp()) + logger.info("Updating user restriction inactive.") + current_time = int(datetime.now(timezone.utc).timestamp()) - updated_fields = { - UserRestrictionsFields.REMOVED_BY.value: removed_by, - UserRestrictionsFields.LAST_UPDATED.value: current_time, - UserRestrictionsFields.IS_ACTIVE.value: False, - } + updated_fields = { + UserRestrictionsFields.REMOVED_BY.value: removed_by, + UserRestrictionsFields.LAST_UPDATED.value: current_time, + UserRestrictionsFields.IS_ACTIVE.value: False, + } + try: self.dynamo_service.update_item( table_name=self.table_name, key_pair={UserRestrictionsFields.ID.value: restriction_id}, updated_fields=updated_fields, - condition_expression=f"{UserRestrictionsFields.IS_ACTIVE.value} = :true " - f"AND {UserRestrictionsFields.RESTRICTED_USER.value} <> :user_id " - f"AND {UserRestrictionsFields.NHS_NUMBER.value} = :patient_id", + condition_expression=( + f"{UserRestrictionsFields.IS_ACTIVE} = :true" + f" AND {UserRestrictionsFields.RESTRICTED_USER} <> :user_id" + f" AND {UserRestrictionsFields.NHS_NUMBER} = :patient_id" + ), expression_attribute_values={ ":true": True, ":user_id": removed_by, ":patient_id": patient_id, }, ) - except ClientError as e: logger.error(e) if ( @@ -113,7 +114,9 @@ def update_restriction_inactive( f"Unexpected DynamoDB error in update_restriction_inactive: " f"{e.response['Error']['Code']} - {e}", ) - raise e + raise UserRestrictionDynamoDBException( + "An issue occurred while updating user restriction inactive", + ) def query_restrictions_by_nhs_number( self, @@ -123,7 +126,7 @@ def query_restrictions_by_nhs_number( logger.info("Building IsActive filter for DynamoDB query.") filter_builder = DynamoQueryFilterBuilder() filter_builder.add_condition( - UserRestrictionsFields.IS_ACTIVE.value, + UserRestrictionsFields.IS_ACTIVE, AttributeOperator.EQUAL, True, ) @@ -132,8 +135,8 @@ def query_restrictions_by_nhs_number( logger.info("Querying Restrictions by NHS Number.") items = self.dynamo_service.query_table( table_name=self.table_name, - index_name=UserRestrictionIndexes.NHS_NUMBER_INDEX.value, - search_key=UserRestrictionsFields.NHS_NUMBER.value, + index_name=UserRestrictionIndexes.NHS_NUMBER_INDEX, + search_key=UserRestrictionsFields.NHS_NUMBER, search_condition=nhs_number, query_filter=active_filter_expression, ) @@ -146,24 +149,27 @@ def query_restrictions_by_nhs_number( ) def update_restriction_custodian(self, restriction_id: str, updated_custodian: str): - try: - logger.info(f"Updating custodian for restriction: {restriction_id}") - current_time = int(datetime.now(timezone.utc).timestamp()) + logger.info(f"Updating custodian for restriction: {restriction_id}") + current_time = int(datetime.now(timezone.utc).timestamp()) - updated_fields = { - UserRestrictionsFields.LAST_UPDATED.value: current_time, - UserRestrictionsFields.CUSTODIAN.value: updated_custodian, - } + updated_fields = { + UserRestrictionsFields.LAST_UPDATED.value: current_time, + UserRestrictionsFields.CUSTODIAN.value: updated_custodian, + } + try: self.dynamo_service.update_item( table_name=self.table_name, key_pair={UserRestrictionsFields.ID.value: restriction_id}, updated_fields=updated_fields, ) - except ClientError as e: - logger.error(e) - raise e + logger.error( + f"DynamoDB ClientError when updating custodian for restriction {restriction_id}: {e}", + ) + raise UserRestrictionDynamoDBException( + f"An issue occurred while updating restriction custodian for restriction {restriction_id}", + ) from e @staticmethod def _build_query_filter( diff --git a/lambdas/tests/unit/services/test_process_mns_message_service.py b/lambdas/tests/unit/services/test_process_mns_message_service.py index 1fec31f47e..f9bd0a63f2 100644 --- a/lambdas/tests/unit/services/test_process_mns_message_service.py +++ b/lambdas/tests/unit/services/test_process_mns_message_service.py @@ -701,22 +701,6 @@ def test_update_restrictions_uses_restriction_dynamo_service(mns_service, mocker ) -@pytest.mark.parametrize( - "mns_event", - [death_notification_message, gp_change_message, removed_death_notification_message], -) -def test_update_restrictions_does_not_call_dynamodb_feature_flag_disabled( - mns_service_restrictions_feature_disabled, - mns_event, -): - mns_service_restrictions_feature_disabled.handle_mns_notification( - mns_event, - ) - - mns_service_restrictions_feature_disabled.restrictions_dynamo_service.query_restrictions_by_nhs_number.assert_not_called() - mns_service_restrictions_feature_disabled.restrictions_dynamo_service.update_restriction_custodian.assert_not_called() - - def test_update_restrictions_not_called_on_informal_death_notification( mns_service, mocker, From c9ea1346d19a8175dba8e3441b2f1dd351f0a395 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Mon, 23 Mar 2026 14:32:32 +0000 Subject: [PATCH 17/17] [PRMP-1476] format --- lambdas/tests/unit/services/test_process_mns_message_service.py | 1 - lambdas/tests/unit/utils/test_ods_utils.py | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/lambdas/tests/unit/services/test_process_mns_message_service.py b/lambdas/tests/unit/services/test_process_mns_message_service.py index 8bdd783183..5e5be10e22 100644 --- a/lambdas/tests/unit/services/test_process_mns_message_service.py +++ b/lambdas/tests/unit/services/test_process_mns_message_service.py @@ -19,7 +19,6 @@ ) from utils.exceptions import PdsErrorException - MOCK_RESTRICTION_DICT = { "ID": TEST_UUID, "RestrictedSmartcard": "123456789012", diff --git a/lambdas/tests/unit/utils/test_ods_utils.py b/lambdas/tests/unit/utils/test_ods_utils.py index 3a36ff2ce8..e41a99cd02 100644 --- a/lambdas/tests/unit/utils/test_ods_utils.py +++ b/lambdas/tests/unit/utils/test_ods_utils.py @@ -1,4 +1,5 @@ import pytest + from enums.patient_ods_inactive_status import PatientOdsInactiveStatus from tests.unit.conftest import TEST_CURRENT_GP_ODS from utils.exceptions import OdsErrorException