From c4ca999b2999667afd5db165fbb92f527b0beb85 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Mon, 15 Dec 2025 11:52:34 +0000 Subject: [PATCH 01/21] [PRMP-1079] test out paginator for with dynamodb client --- .../document_upload_review_service.py | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/lambdas/services/document_upload_review_service.py b/lambdas/services/document_upload_review_service.py index 007f212f5..9377e09c2 100644 --- a/lambdas/services/document_upload_review_service.py +++ b/lambdas/services/document_upload_review_service.py @@ -1,5 +1,6 @@ import os +import boto3 from boto3.dynamodb.conditions import Attr, ConditionBase from botocore.exceptions import ClientError from enums.document_review_status import DocumentReviewStatus @@ -74,6 +75,44 @@ def query_docs_pending_review_by_custodian_with_limit( logger.error(e) raise DocumentReviewException("Failed to query document reviews") + + def query_docs_with_paginator( + self, + ods_code: str, + limit: int = DEFAULT_QUERY_LIMIT, + start_key: str | None = None, + nhs_number: str | None = None, + uploader: str | None = None, + ) -> tuple[list[DocumentUploadReviewReference], dict | None]: + logger.info(f"Getting review document references for custodian: {ods_code}") + + filter_expression = self.build_review_query_filter( + nhs_number=nhs_number, uploader=uploader + ) + + key_condition = self.dynamo_service.build_key_condition(search_key="Custodian", search_condition=ods_code) + + client = boto3.client("dynamodb") + paginator = client.get_paginator("query") + + response = paginator.paginate( + TableName=self.table_name, + IndexName="CustodianIndex", + KeyConditionExpression=key_condition, + FilterExpression=filter_expression, + PaginatorConfig={ + "MaxItems": limit, + "PageSize": 1, + "StartingToken": start_key + } + ) + + references = self._validate_review_references(response["Items"]) + last_evaluated_key = response.get("NextToken") + + return references, last_evaluated_key + + def _validate_review_references( self, items: list[dict] ) -> list[DocumentUploadReviewReference]: From 5fb5ec6428eff1ba0d95ff918ae47e79bab8bc0c Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Tue, 16 Dec 2025 09:53:47 +0000 Subject: [PATCH 02/21] [PRMP-1079] key needs to be a string for keycondition --- lambdas/services/document_upload_review_service.py | 8 ++++---- lambdas/services/search_document_review_service.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lambdas/services/document_upload_review_service.py b/lambdas/services/document_upload_review_service.py index 9377e09c2..cdf79dd88 100644 --- a/lambdas/services/document_upload_review_service.py +++ b/lambdas/services/document_upload_review_service.py @@ -90,16 +90,16 @@ def query_docs_with_paginator( nhs_number=nhs_number, uploader=uploader ) - key_condition = self.dynamo_service.build_key_condition(search_key="Custodian", search_condition=ods_code) - client = boto3.client("dynamodb") paginator = client.get_paginator("query") + key_condition = {"S": ods_code} + response = paginator.paginate( TableName=self.table_name, IndexName="CustodianIndex", - KeyConditionExpression=key_condition, - FilterExpression=filter_expression, + KeyConditionExpression='{"ID"}:' + f"{key_condition}" + '}', + # FilterExpression=filter_expression, PaginatorConfig={ "MaxItems": limit, "PageSize": 1, diff --git a/lambdas/services/search_document_review_service.py b/lambdas/services/search_document_review_service.py index 860faa551..efaba30d8 100644 --- a/lambdas/services/search_document_review_service.py +++ b/lambdas/services/search_document_review_service.py @@ -71,7 +71,7 @@ def get_review_document_references( nhs_number: str | None = None, uploader: str | None = None, ): - return self.document_service.query_docs_pending_review_by_custodian_with_limit( + return self.document_service.query_docs_with_paginator( ods_code=ods_code, limit=limit, start_key=start_key, From 842d7289406d321922e3b30ab68ac995b3c2d3f4 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Tue, 16 Dec 2025 10:10:43 +0000 Subject: [PATCH 03/21] [PRMP-1079] change key expression format --- lambdas/services/document_upload_review_service.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lambdas/services/document_upload_review_service.py b/lambdas/services/document_upload_review_service.py index cdf79dd88..c49469a23 100644 --- a/lambdas/services/document_upload_review_service.py +++ b/lambdas/services/document_upload_review_service.py @@ -98,7 +98,7 @@ def query_docs_with_paginator( response = paginator.paginate( TableName=self.table_name, IndexName="CustodianIndex", - KeyConditionExpression='{"ID"}:' + f"{key_condition}" + '}', + KeyConditionExpression=f"Custodian = {key_condition}", # FilterExpression=filter_expression, PaginatorConfig={ "MaxItems": limit, From 9d3ad51ee95ef68da2e5c5327e63996ccfa9f9ba Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Tue, 16 Dec 2025 15:39:46 +0000 Subject: [PATCH 04/21] [PRMP-1079] use build full result --- .../document_upload_review_service.py | 54 ++++++++++++++----- .../search_document_review_service.py | 30 ++--------- lambdas/utils/dynamo_utils.py | 18 +++++++ 3 files changed, 64 insertions(+), 38 deletions(-) diff --git a/lambdas/services/document_upload_review_service.py b/lambdas/services/document_upload_review_service.py index c49469a23..8a7aede0d 100644 --- a/lambdas/services/document_upload_review_service.py +++ b/lambdas/services/document_upload_review_service.py @@ -2,6 +2,7 @@ import boto3 from boto3.dynamodb.conditions import Attr, ConditionBase +from boto3.dynamodb.types import TypeDeserializer from botocore.exceptions import ClientError from enums.document_review_status import DocumentReviewStatus from enums.dynamo_filter import AttributeOperator @@ -12,7 +13,7 @@ from services.document_service import DocumentService from utils.audit_logging_setup import LoggingService from utils.dynamo_query_filter_builder import DynamoQueryFilterBuilder -from utils.dynamo_utils import build_transaction_item +from utils.dynamo_utils import build_transaction_item, deserialize_dynamodb_object, build_mixed_condition_expression from utils.exceptions import DocumentReviewException logger = LoggingService(__name__) @@ -76,7 +77,7 @@ def query_docs_pending_review_by_custodian_with_limit( raise DocumentReviewException("Failed to query document reviews") - def query_docs_with_paginator( + def query_docs_pending_review_with_paginator( self, ods_code: str, limit: int = DEFAULT_QUERY_LIMIT, @@ -86,29 +87,32 @@ def query_docs_with_paginator( ) -> tuple[list[DocumentUploadReviewReference], dict | None]: logger.info(f"Getting review document references for custodian: {ods_code}") - filter_expression = self.build_review_query_filter( - nhs_number=nhs_number, uploader=uploader - ) - client = boto3.client("dynamodb") paginator = client.get_paginator("query") key_condition = {"S": ods_code} + filter_expression, condition_attribute_names, condition_attribute_values = build_mixed_condition_expression(nhs_number, uploader) + condition_attribute_values.update({":c": key_condition}) + response = paginator.paginate( TableName=self.table_name, IndexName="CustodianIndex", - KeyConditionExpression=f"Custodian = {key_condition}", - # FilterExpression=filter_expression, - PaginatorConfig={ + KeyConditionExpression="Custodian=:c", + FilterExpression=filter_expression, + ExpressionAttributeValues=condition_attribute_values, + ExpressionAttributeNames=condition_attribute_names, + PaginationConfig={ "MaxItems": limit, "PageSize": 1, "StartingToken": start_key } - ) + ).build_full_result() - references = self._validate_review_references(response["Items"]) - last_evaluated_key = response.get("NextToken") + deserialized_items = [deserialize_dynamodb_object(item) for item in response.get("Items", [])] + + references = self._validate_review_references(deserialized_items) + last_evaluated_key = response.get("NextToken", None) return references, last_evaluated_key @@ -329,3 +333,29 @@ def build_review_query_filter( filter_builder.add_condition("Author", AttributeOperator.EQUAL, uploader) return filter_builder.build() + + def build_paginator_query_filter( + self, nhs_number: str | None = None, uploader: str | None = None + ): + conditions = [ + { + "field": "ReviewStatus", + "operator": "=", + "value": DocumentReviewStatus.PENDING_REVIEW.value, + } + ] + if nhs_number: + conditions.append({ + "field": "NhsNumber", + "operator": "=", + "value": nhs_number, + }) + + if uploader: + conditions.append({ + "field": "Author", + "operator": "=", + "value": uploader, + }) + + return build_mixed_condition_expression(conditions) diff --git a/lambdas/services/search_document_review_service.py b/lambdas/services/search_document_review_service.py index efaba30d8..c05396fdb 100644 --- a/lambdas/services/search_document_review_service.py +++ b/lambdas/services/search_document_review_service.py @@ -20,14 +20,13 @@ def process_request( self, ods_code: str, params: dict ) -> tuple[list[str], str | None]: try: - - decoded_start_key = self.decode_start_key(params.get("nextPageToken", None)) + start_key = params.get("nextPageToken", None) str_limit = params.get("limit", self.document_service.DEFAULT_QUERY_LIMIT) limit = int(str_limit) references, last_evaluated_key = self.get_review_document_references( - start_key=decoded_start_key, + start_key=start_key, ods_code=ods_code, limit=limit, nhs_number=params.get("nhsNumber", None), @@ -50,9 +49,7 @@ def process_request( for reference in references ] - encoded_exclusive_start_key = self.encode_start_key(last_evaluated_key) - - return output_refs, encoded_exclusive_start_key + return output_refs, last_evaluated_key except ValidationError as e: logger.error(e) @@ -71,29 +68,10 @@ def get_review_document_references( nhs_number: str | None = None, uploader: str | None = None, ): - return self.document_service.query_docs_with_paginator( + return self.document_service.query_docs_pending_review_with_paginator( ods_code=ods_code, limit=limit, start_key=start_key, nhs_number=nhs_number, uploader=uploader, ) - - def decode_start_key(self, encoded_start_key: str | None) -> dict[str, str] | None: - return ( - json.loads( - base64.b64decode(encoded_start_key.encode("ascii")).decode("utf-8") - ) - if encoded_start_key - else None - ) - - def encode_start_key(self, start_key: dict) -> str | None: - if start_key: - for key, value in start_key.items(): - if isinstance(value, decimal.Decimal): - start_key[key] = int(value) - return ( - base64.b64encode(json.dumps(start_key).encode("ascii")).decode("utf-8") - ) - return None diff --git a/lambdas/utils/dynamo_utils.py b/lambdas/utils/dynamo_utils.py index 35b5673fc..b6d3bdae8 100644 --- a/lambdas/utils/dynamo_utils.py +++ b/lambdas/utils/dynamo_utils.py @@ -2,6 +2,8 @@ from typing import Any, Dict import inflection +from boto3.dynamodb.types import TypeSerializer, TypeDeserializer + from enums.dynamo_filter import AttributeOperator from enums.infrastructure import DynamoTables from enums.lambda_error import LambdaError @@ -323,6 +325,22 @@ def build_transaction_item( expression_attribute_values=expression_attribute_values or None, ) +def serialize_dict_to_dynamodb_object(object: dict) -> dict: + serializer = TypeSerializer() + return { + key: serializer.serialize(value) + for key, value in object.items() + } + + +def deserialize_dynamodb_object(object: dict) -> dict: + deserialize = TypeDeserializer().deserialize + parsed_dynamodb_items = { + key: deserialize(value) + for key, value in object.items() + } + return parsed_dynamodb_items + class DocTypeTableRouter: def __init__(self): From 14be2df94f125ef75a3328691195ab470c696908 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Tue, 16 Dec 2025 16:45:13 +0000 Subject: [PATCH 05/21] [PRMP-1079] serialize condition attribute values --- .../document_upload_review_service.py | 64 ++++++++++--------- 1 file changed, 33 insertions(+), 31 deletions(-) diff --git a/lambdas/services/document_upload_review_service.py b/lambdas/services/document_upload_review_service.py index 8a7aede0d..67d15cc07 100644 --- a/lambdas/services/document_upload_review_service.py +++ b/lambdas/services/document_upload_review_service.py @@ -2,7 +2,6 @@ import boto3 from boto3.dynamodb.conditions import Attr, ConditionBase -from boto3.dynamodb.types import TypeDeserializer from botocore.exceptions import ClientError from enums.document_review_status import DocumentReviewStatus from enums.dynamo_filter import AttributeOperator @@ -13,7 +12,8 @@ from services.document_service import DocumentService from utils.audit_logging_setup import LoggingService from utils.dynamo_query_filter_builder import DynamoQueryFilterBuilder -from utils.dynamo_utils import build_transaction_item, deserialize_dynamodb_object, build_mixed_condition_expression +from utils.dynamo_utils import build_transaction_item, deserialize_dynamodb_object, build_mixed_condition_expression, \ + serialize_dict_to_dynamodb_object from utils.exceptions import DocumentReviewException logger = LoggingService(__name__) @@ -92,15 +92,17 @@ def query_docs_pending_review_with_paginator( key_condition = {"S": ods_code} - filter_expression, condition_attribute_names, condition_attribute_values = build_mixed_condition_expression(nhs_number, uploader) - condition_attribute_values.update({":c": key_condition}) + filter_expression, condition_attribute_names, condition_attribute_values = self.build_paginator_query_filter(nhs_number, uploader) + + serialized_condition_attribute_values = serialize_dict_to_dynamodb_object(condition_attribute_values) + serialized_condition_attribute_values.update({":c": key_condition}) response = paginator.paginate( TableName=self.table_name, IndexName="CustodianIndex", KeyConditionExpression="Custodian=:c", FilterExpression=filter_expression, - ExpressionAttributeValues=condition_attribute_values, + ExpressionAttributeValues=serialized_condition_attribute_values, ExpressionAttributeNames=condition_attribute_names, PaginationConfig={ "MaxItems": limit, @@ -132,6 +134,32 @@ def _validate_review_references( "Failed to validate document review references" ) + def build_paginator_query_filter( + self, nhs_number: str | None = None, uploader: str | None = None + ): + conditions = [ + { + "field": "ReviewStatus", + "operator": "=", + "value": DocumentReviewStatus.PENDING_REVIEW.value, + } + ] + if nhs_number: + conditions.append({ + "field": "NhsNumber", + "operator": "=", + "value": nhs_number, + }) + + if uploader: + conditions.append({ + "field": "Author", + "operator": "=", + "value": uploader, + }) + + return build_mixed_condition_expression(conditions) + def get_document( self, document_id: str, version: int | None ) -> DocumentUploadReviewReference | None: @@ -333,29 +361,3 @@ def build_review_query_filter( filter_builder.add_condition("Author", AttributeOperator.EQUAL, uploader) return filter_builder.build() - - def build_paginator_query_filter( - self, nhs_number: str | None = None, uploader: str | None = None - ): - conditions = [ - { - "field": "ReviewStatus", - "operator": "=", - "value": DocumentReviewStatus.PENDING_REVIEW.value, - } - ] - if nhs_number: - conditions.append({ - "field": "NhsNumber", - "operator": "=", - "value": nhs_number, - }) - - if uploader: - conditions.append({ - "field": "Author", - "operator": "=", - "value": uploader, - }) - - return build_mixed_condition_expression(conditions) From dd2a861502db4441874e409812388bd83752136d Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Tue, 16 Dec 2025 17:22:36 +0000 Subject: [PATCH 06/21] [PRMP-1079] add dynamo_utils tests --- lambdas/tests/unit/utils/test_dynamo_utils.py | 41 ++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/lambdas/tests/unit/utils/test_dynamo_utils.py b/lambdas/tests/unit/utils/test_dynamo_utils.py index 0ebe45ce7..6e81ffcb1 100644 --- a/lambdas/tests/unit/utils/test_dynamo_utils.py +++ b/lambdas/tests/unit/utils/test_dynamo_utils.py @@ -1,4 +1,5 @@ import json +from decimal import Decimal import pytest from enums.lambda_error import LambdaError @@ -26,13 +27,32 @@ create_expression_value_placeholder, create_expressions, create_update_expression, - parse_dynamo_record, + parse_dynamo_record, serialize_dict_to_dynamodb_object, deserialize_dynamodb_object, ) from utils.lambda_exceptions import InvalidDocTypeException from lambdas.enums.snomed_codes import SnomedCodes +MOCK_PYTHON_DICT = { + "test_string": "hello", + "test_int": 123, + "test_bool": True, + "test_list": [1, 2, 3], + "test_dict": {"key1": "value1", "key2": 1}, + "test_list_of_dicts": [{"key1": "value1"}, {"key2": 2}], +} + +MOCK_DYNAMO_DB_OBJECT = { + "test_string": {"S": "hello"}, + "test_int": {"N": "123"}, + "test_bool": {"BOOL": True}, + "test_list": {"L": [{"N": "1"}, {"N": "2"}, {"N": "3"}]}, + "test_dict": {"M": {"key1": {"S": "value1"}, "key2": {"N": "1"}}}, + "test_list_of_dicts": {"L": [{"M": {"key1":{"S":"value1"}}},{"M":{"key2": {"N": "2"}}}]}, +} + + def test_create_expressions_correctly_creates_an_expression_of_one_field(): expected_projection = "#VirusScannerResult_attr" expected_expr_attr_names = {"#VirusScannerResult_attr": "VirusScannerResult"} @@ -528,3 +548,22 @@ def test_build_transaction_item_delete_with_conditions(): delete_item["ExpressionAttributeValues"][":Status_condition_val"] == "archived" ) assert delete_item["ExpressionAttributeValues"][":Expired_condition_val"] is True + + +def test_serialize_dict_to_dynamodb_object(): + input = MOCK_PYTHON_DICT + + expected = MOCK_DYNAMO_DB_OBJECT + actual = serialize_dict_to_dynamodb_object(input) + assert actual == expected + +# TODO Test with a float throws error/catches error +# tests for deserialisation and error cases + + +def test_deserialize_dynamodb_object(): + input = MOCK_DYNAMO_DB_OBJECT + expected = MOCK_PYTHON_DICT + + actual = deserialize_dynamodb_object(input) + assert actual == expected From 38e48fdcdb68459dc7bd758f091dcf5565aa22d5 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Wed, 17 Dec 2025 08:50:59 +0000 Subject: [PATCH 07/21] [PRMP-1079] add negative test for dynamo util --- lambdas/tests/unit/utils/test_dynamo_utils.py | 13 ++++++++++--- lambdas/utils/dynamo_utils.py | 1 + 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/lambdas/tests/unit/utils/test_dynamo_utils.py b/lambdas/tests/unit/utils/test_dynamo_utils.py index 6e81ffcb1..db93d2b43 100644 --- a/lambdas/tests/unit/utils/test_dynamo_utils.py +++ b/lambdas/tests/unit/utils/test_dynamo_utils.py @@ -1,4 +1,5 @@ import json +from copy import deepcopy from decimal import Decimal import pytest @@ -557,9 +558,6 @@ def test_serialize_dict_to_dynamodb_object(): actual = serialize_dict_to_dynamodb_object(input) assert actual == expected -# TODO Test with a float throws error/catches error -# tests for deserialisation and error cases - def test_deserialize_dynamodb_object(): input = MOCK_DYNAMO_DB_OBJECT @@ -567,3 +565,12 @@ def test_deserialize_dynamodb_object(): actual = deserialize_dynamodb_object(input) assert actual == expected + + +def test_serialize_dynamodb_object_throws_error_unsupported_data_type(): + unsupported_input = deepcopy(MOCK_PYTHON_DICT) + unsupported_input.update({"float": 1.23}) + + with pytest.raises(TypeError): + serialize_dict_to_dynamodb_object(unsupported_input) + diff --git a/lambdas/utils/dynamo_utils.py b/lambdas/utils/dynamo_utils.py index b6d3bdae8..782f97da2 100644 --- a/lambdas/utils/dynamo_utils.py +++ b/lambdas/utils/dynamo_utils.py @@ -325,6 +325,7 @@ def build_transaction_item( expression_attribute_values=expression_attribute_values or None, ) + def serialize_dict_to_dynamodb_object(object: dict) -> dict: serializer = TypeSerializer() return { From 1b4433ff1c53adc13dbf70aad48dda52ebc1db04 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Wed, 17 Dec 2025 10:07:37 +0000 Subject: [PATCH 08/21] [PRMP-1079] amend service tests to call query docs pending review with paginator --- .../generate_document_manifest_handler.py | 9 +--- .../search_document_review_service.py | 6 +-- .../test_search_document_review_service.py | 48 +++++-------------- lambdas/tests/unit/utils/test_dynamo_utils.py | 1 - 4 files changed, 14 insertions(+), 50 deletions(-) diff --git a/lambdas/handlers/generate_document_manifest_handler.py b/lambdas/handlers/generate_document_manifest_handler.py index d9e1b3778..9aa6a3e12 100644 --- a/lambdas/handlers/generate_document_manifest_handler.py +++ b/lambdas/handlers/generate_document_manifest_handler.py @@ -1,4 +1,3 @@ -from boto3.dynamodb.types import TypeDeserializer from enums.lambda_error import LambdaError from enums.logging_app_interaction import LoggingAppInteraction from models.zip_trace import DocumentManifestZipTrace @@ -9,6 +8,7 @@ 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.dynamo_utils import deserialize_dynamodb_object from utils.lambda_exceptions import GenerateManifestZipException from utils.lambda_response import ApiGatewayResponse from utils.request_context import request_context @@ -71,9 +71,4 @@ def manifest_zip_handler(zip_trace_item): def prepare_zip_trace_data(new_zip_trace: dict) -> dict: - deserialize = TypeDeserializer().deserialize - parsed_dynamodb_items = { - key: deserialize(dynamodb_value) - for key, dynamodb_value in new_zip_trace.items() - } - return parsed_dynamodb_items + return deserialize_dynamodb_object(new_zip_trace) diff --git a/lambdas/services/search_document_review_service.py b/lambdas/services/search_document_review_service.py index c05396fdb..febd775b1 100644 --- a/lambdas/services/search_document_review_service.py +++ b/lambdas/services/search_document_review_service.py @@ -1,7 +1,3 @@ -import base64 -import decimal -import json - from enums.lambda_error import LambdaError from pydantic import ValidationError from services.document_upload_review_service import DocumentUploadReviewService @@ -64,7 +60,7 @@ def get_review_document_references( self, ods_code: str, limit: int | None = None, - start_key: dict | None = None, + start_key: str | None = None, nhs_number: str | None = None, uploader: str | None = None, ): diff --git a/lambdas/tests/unit/services/test_search_document_review_service.py b/lambdas/tests/unit/services/test_search_document_review_service.py index de6fbb80c..e4eb159e3 100644 --- a/lambdas/tests/unit/services/test_search_document_review_service.py +++ b/lambdas/tests/unit/services/test_search_document_review_service.py @@ -40,10 +40,6 @@ def search_document_review_service(set_env, mocker): def test_handle_gateway_api_request_happy_path(search_document_review_service, mocker): - mocker.patch.object( - search_document_review_service, "decode_start_key" - ).return_value = TEST_LAST_EVALUATED_KEY - expected_refs = [ DocumentUploadReviewReference.model_validate(item) for item in MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"] @@ -52,7 +48,7 @@ def test_handle_gateway_api_request_happy_path(search_document_review_service, m search_document_review_service, "get_review_document_references" ).return_value = ( expected_refs, - TEST_LAST_EVALUATED_KEY, + TEST_ENCODED_START_KEY, ) expected = ( @@ -117,12 +113,9 @@ def test_handle_gateway_api_request_happy_path(search_document_review_service, m ods_code=TEST_CURRENT_GP_ODS, ) - search_document_review_service.decode_start_key.assert_called_with( - TEST_ENCODED_START_KEY - ) search_document_review_service.get_review_document_references.assert_called_with( ods_code=TEST_CURRENT_GP_ODS, - start_key=TEST_LAST_EVALUATED_KEY, + start_key=TEST_ENCODED_START_KEY, limit=int(TEST_QUERY_LIMIT), uploader="Z67890", nhs_number=None, @@ -134,10 +127,6 @@ def test_handle_gateway_api_request_happy_path(search_document_review_service, m def test_process_request_handles_invalid_limit_querystring( search_document_review_service, mocker ): - mocker.patch.object( - search_document_review_service, "decode_start_key" - ).return_value = TEST_LAST_EVALUATED_KEY - with pytest.raises(DocumentReviewException) as e: search_document_review_service.process_request( params=MOCK_QUERYSTRING_PARAMS_INVALID_LIMIT, ods_code=TEST_CURRENT_GP_ODS @@ -150,10 +139,6 @@ def test_process_request_handles_invalid_limit_querystring( def test_process_request_handles_validation_error( search_document_review_service, mocker ): - mocker.patch.object( - search_document_review_service, "decode_start_key" - ).return_value = TEST_LAST_EVALUATED_KEY - mocker.patch.object( search_document_review_service, "get_review_document_references" ).side_effect = ValidationError("", []) @@ -172,13 +157,13 @@ def test_service_queries_document_review_table_with_correct_args( search_document_review_service, ): search_document_review_service.get_review_document_references( - TEST_CURRENT_GP_ODS, int(TEST_QUERY_LIMIT), TEST_LAST_EVALUATED_KEY, None, None + TEST_CURRENT_GP_ODS, int(TEST_QUERY_LIMIT), TEST_ENCODED_START_KEY, None, None ) - search_document_review_service.document_service.query_docs_pending_review_by_custodian_with_limit.assert_called_with( + search_document_review_service.document_service.query_docs_pending_review_with_paginator.assert_called_with( ods_code=TEST_CURRENT_GP_ODS, limit=int(TEST_QUERY_LIMIT), - start_key=TEST_LAST_EVALUATED_KEY, + start_key=TEST_ENCODED_START_KEY, nhs_number=None, uploader=None, ) @@ -191,9 +176,9 @@ def test_get_review_document_references_returns_document_references( DocumentUploadReviewReference.model_validate(item) for item in MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"] ] - search_document_review_service.document_service.query_docs_pending_review_by_custodian_with_limit.return_value = ( + search_document_review_service.document_service.query_docs_pending_review_with_paginator.return_value = ( expected_references, - TEST_LAST_EVALUATED_KEY, + TEST_ENCODED_START_KEY, ) actual = search_document_review_service.get_review_document_references( @@ -202,7 +187,7 @@ def test_get_review_document_references_returns_document_references( expected = ( expected_references, - TEST_LAST_EVALUATED_KEY, + TEST_ENCODED_START_KEY, ) assert actual == expected @@ -211,7 +196,7 @@ def test_get_review_document_references_returns_document_references( def test_get_review_document_references_handles_empty_result( search_document_review_service, ): - search_document_review_service.document_service.query_docs_pending_review_by_custodian_with_limit.return_value = ( + search_document_review_service.document_service.query_docs_pending_review_with_paginator.return_value = ( [], None, ) @@ -230,7 +215,7 @@ def test_get_review_document_references_handles_no_limit_passed( DocumentUploadReviewReference.model_validate(item) for item in MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"] ] - search_document_review_service.document_service.query_docs_pending_review_by_custodian_with_limit.return_value = ( + search_document_review_service.document_service.query_docs_pending_review_with_paginator.return_value = ( expected_references, None, ) @@ -251,7 +236,7 @@ def test_get_review_document_references_throws_exception_client_error( search_document_review_service, ): ( - search_document_review_service.document_service.query_docs_pending_review_by_custodian_with_limit + search_document_review_service.document_service.query_docs_pending_review_with_paginator ).side_effect = DocumentReviewException(500, LambdaError.DocumentReviewDB) with pytest.raises(DocumentReviewException) as e: @@ -261,14 +246,3 @@ def test_get_review_document_references_throws_exception_client_error( assert e.value.status_code == 500 assert e.value.error == LambdaError.DocumentReviewDB - -def test_decode_start_key(search_document_review_service): - encoded_start_key = TEST_ENCODED_START_KEY - - actual = search_document_review_service.decode_start_key(encoded_start_key) - assert actual == TEST_LAST_EVALUATED_KEY - - -def test_encode_start_key(search_document_review_service): - actual = search_document_review_service.encode_start_key(TEST_LAST_EVALUATED_KEY) - assert actual == TEST_ENCODED_START_KEY diff --git a/lambdas/tests/unit/utils/test_dynamo_utils.py b/lambdas/tests/unit/utils/test_dynamo_utils.py index db93d2b43..bdbceb773 100644 --- a/lambdas/tests/unit/utils/test_dynamo_utils.py +++ b/lambdas/tests/unit/utils/test_dynamo_utils.py @@ -1,6 +1,5 @@ import json from copy import deepcopy -from decimal import Decimal import pytest from enums.lambda_error import LambdaError From 20cb592551cddc613ee5bfdb1198d33da52eb24d Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Wed, 17 Dec 2025 10:35:32 +0000 Subject: [PATCH 09/21] [PRMP-1079] add paginator query filter test --- .../test_document_upload_review_service.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/lambdas/tests/unit/services/test_document_upload_review_service.py b/lambdas/tests/unit/services/test_document_upload_review_service.py index 8b23f2878..bca01cc86 100644 --- a/lambdas/tests/unit/services/test_document_upload_review_service.py +++ b/lambdas/tests/unit/services/test_document_upload_review_service.py @@ -570,4 +570,21 @@ def test_get_document_by_id_raises_exception_client_error(mock_service): ) with pytest.raises(DocumentReviewException): - mock_service.get_document(TEST_UUID, 1) \ No newline at end of file + mock_service.get_document(TEST_UUID, 1) + + +@pytest.mark.parametrize("nhs_number, uploader, expected", [ + (None, None, + ("#ReviewStatus_attr = :ReviewStatus_condition_val", {"#ReviewStatus_attr": "ReviewStatus"}, {":ReviewStatus_condition_val": DocumentReviewStatus.PENDING_REVIEW}) + ), + (TEST_NHS_NUMBER, None, ( + "#ReviewStatus_attr = :ReviewStatus_condition_val AND #NhsNumber_attr = :NhsNumber_condition_val", {"#ReviewStatus_attr": "ReviewStatus", "#NhsNumber_attr": "NhsNumber"}, {":ReviewStatus_condition_val": DocumentReviewStatus.PENDING_REVIEW, ":NhsNumber_condition_val": TEST_NHS_NUMBER} + )), + (TEST_NHS_NUMBER, TEST_ODS_CODE, ("#ReviewStatus_attr = :ReviewStatus_condition_val AND #NhsNumber_attr = :NhsNumber_condition_val AND #Author_attr = :Author_condition_val", {"#ReviewStatus_attr": "ReviewStatus", "#NhsNumber_attr": "NhsNumber", "#Author_attr": "Author"}, {":ReviewStatus_condition_val": DocumentReviewStatus.PENDING_REVIEW, ":NhsNumber_condition_val": TEST_NHS_NUMBER, ":Author_condition_val": TEST_ODS_CODE} + )), + (None, TEST_ODS_CODE, ("#ReviewStatus_attr = :ReviewStatus_condition_val AND #Author_attr = :Author_condition_val", {"#ReviewStatus_attr": "ReviewStatus", "#Author_attr": "Author"}, {":ReviewStatus_condition_val": DocumentReviewStatus.PENDING_REVIEW, ":Author_condition_val": TEST_ODS_CODE})) +]) +def test_build_paginator_query_filter(mock_service, nhs_number, uploader, expected): + actual = mock_service.build_paginator_query_filter(nhs_number=nhs_number, uploader=uploader) + + assert actual == expected From 5ac066ef84050c2067fe0b7f198b855d2a16f7f0 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Wed, 17 Dec 2025 10:40:10 +0000 Subject: [PATCH 10/21] [PRMP-1079] format --- lambdas/services/base/dynamo_service.py | 4 +- .../document_upload_review_service.py | 50 ++++++++----- .../unit/services/base/test_dynamo_service.py | 2 +- .../test_document_upload_review_service.py | 75 +++++++++++++++---- .../test_search_document_review_service.py | 19 +++-- lambdas/tests/unit/utils/test_dynamo_utils.py | 32 ++++---- lambdas/utils/dynamo_utils.py | 13 +--- 7 files changed, 124 insertions(+), 71 deletions(-) diff --git a/lambdas/services/base/dynamo_service.py b/lambdas/services/base/dynamo_service.py index 38f71f3fc..04c16c87a 100644 --- a/lambdas/services/base/dynamo_service.py +++ b/lambdas/services/base/dynamo_service.py @@ -1,9 +1,9 @@ +import operator import time -from typing import Optional, Sequence from functools import reduce +from typing import Optional, Sequence import boto3 -import operator from boto3.dynamodb.conditions import Attr, ConditionBase, Key from botocore.exceptions import ClientError from utils.audit_logging_setup import LoggingService diff --git a/lambdas/services/document_upload_review_service.py b/lambdas/services/document_upload_review_service.py index 67d15cc07..114901a6b 100644 --- a/lambdas/services/document_upload_review_service.py +++ b/lambdas/services/document_upload_review_service.py @@ -12,8 +12,12 @@ from services.document_service import DocumentService from utils.audit_logging_setup import LoggingService from utils.dynamo_query_filter_builder import DynamoQueryFilterBuilder -from utils.dynamo_utils import build_transaction_item, deserialize_dynamodb_object, build_mixed_condition_expression, \ - serialize_dict_to_dynamodb_object +from utils.dynamo_utils import ( + build_mixed_condition_expression, + build_transaction_item, + deserialize_dynamodb_object, + serialize_dict_to_dynamodb_object, +) from utils.exceptions import DocumentReviewException logger = LoggingService(__name__) @@ -76,7 +80,6 @@ def query_docs_pending_review_by_custodian_with_limit( logger.error(e) raise DocumentReviewException("Failed to query document reviews") - def query_docs_pending_review_with_paginator( self, ods_code: str, @@ -92,9 +95,13 @@ def query_docs_pending_review_with_paginator( key_condition = {"S": ods_code} - filter_expression, condition_attribute_names, condition_attribute_values = self.build_paginator_query_filter(nhs_number, uploader) + filter_expression, condition_attribute_names, condition_attribute_values = ( + self.build_paginator_query_filter(nhs_number, uploader) + ) - serialized_condition_attribute_values = serialize_dict_to_dynamodb_object(condition_attribute_values) + serialized_condition_attribute_values = serialize_dict_to_dynamodb_object( + condition_attribute_values + ) serialized_condition_attribute_values.update({":c": key_condition}) response = paginator.paginate( @@ -107,18 +114,19 @@ def query_docs_pending_review_with_paginator( PaginationConfig={ "MaxItems": limit, "PageSize": 1, - "StartingToken": start_key - } + "StartingToken": start_key, + }, ).build_full_result() - deserialized_items = [deserialize_dynamodb_object(item) for item in response.get("Items", [])] + deserialized_items = [ + deserialize_dynamodb_object(item) for item in response.get("Items", []) + ] references = self._validate_review_references(deserialized_items) last_evaluated_key = response.get("NextToken", None) return references, last_evaluated_key - def _validate_review_references( self, items: list[dict] ) -> list[DocumentUploadReviewReference]: @@ -145,18 +153,22 @@ def build_paginator_query_filter( } ] if nhs_number: - conditions.append({ - "field": "NhsNumber", - "operator": "=", - "value": nhs_number, - }) + conditions.append( + { + "field": "NhsNumber", + "operator": "=", + "value": nhs_number, + } + ) if uploader: - conditions.append({ - "field": "Author", - "operator": "=", - "value": uploader, - }) + conditions.append( + { + "field": "Author", + "operator": "=", + "value": uploader, + } + ) return build_mixed_condition_expression(conditions) diff --git a/lambdas/tests/unit/services/base/test_dynamo_service.py b/lambdas/tests/unit/services/base/test_dynamo_service.py index 80fa1ea91..597d8f568 100755 --- a/lambdas/tests/unit/services/base/test_dynamo_service.py +++ b/lambdas/tests/unit/services/base/test_dynamo_service.py @@ -2,7 +2,7 @@ from unittest.mock import call import pytest -from boto3.dynamodb.conditions import Attr, Equals, Key, And +from boto3.dynamodb.conditions import And, Attr, Equals, Key from botocore.exceptions import ClientError from enums.dynamo_filter import AttributeOperator from enums.metadata_field_names import DocumentReferenceMetadataFields diff --git a/lambdas/tests/unit/services/test_document_upload_review_service.py b/lambdas/tests/unit/services/test_document_upload_review_service.py index bca01cc86..b2839366f 100644 --- a/lambdas/tests/unit/services/test_document_upload_review_service.py +++ b/lambdas/tests/unit/services/test_document_upload_review_service.py @@ -13,7 +13,6 @@ TEST_NHS_NUMBER, TEST_UUID, ) -from tests.unit.helpers.data.dynamo.dynamo_responses import MOCK_SEARCH_RESPONSE from tests.unit.helpers.data.search_document_review.dynamo_response import ( MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE, ) @@ -563,7 +562,6 @@ def test_get_document_returns_none_no_documents_found(mock_service): assert actual == expected - def test_get_document_by_id_raises_exception_client_error(mock_service): mock_service.dynamo_service.get_item.side_effect = ClientError( {"Error": {"Code": "500", "Message": "mocked error"}}, "get_item" @@ -573,18 +571,67 @@ def test_get_document_by_id_raises_exception_client_error(mock_service): mock_service.get_document(TEST_UUID, 1) -@pytest.mark.parametrize("nhs_number, uploader, expected", [ - (None, None, - ("#ReviewStatus_attr = :ReviewStatus_condition_val", {"#ReviewStatus_attr": "ReviewStatus"}, {":ReviewStatus_condition_val": DocumentReviewStatus.PENDING_REVIEW}) - ), - (TEST_NHS_NUMBER, None, ( - "#ReviewStatus_attr = :ReviewStatus_condition_val AND #NhsNumber_attr = :NhsNumber_condition_val", {"#ReviewStatus_attr": "ReviewStatus", "#NhsNumber_attr": "NhsNumber"}, {":ReviewStatus_condition_val": DocumentReviewStatus.PENDING_REVIEW, ":NhsNumber_condition_val": TEST_NHS_NUMBER} - )), - (TEST_NHS_NUMBER, TEST_ODS_CODE, ("#ReviewStatus_attr = :ReviewStatus_condition_val AND #NhsNumber_attr = :NhsNumber_condition_val AND #Author_attr = :Author_condition_val", {"#ReviewStatus_attr": "ReviewStatus", "#NhsNumber_attr": "NhsNumber", "#Author_attr": "Author"}, {":ReviewStatus_condition_val": DocumentReviewStatus.PENDING_REVIEW, ":NhsNumber_condition_val": TEST_NHS_NUMBER, ":Author_condition_val": TEST_ODS_CODE} - )), - (None, TEST_ODS_CODE, ("#ReviewStatus_attr = :ReviewStatus_condition_val AND #Author_attr = :Author_condition_val", {"#ReviewStatus_attr": "ReviewStatus", "#Author_attr": "Author"}, {":ReviewStatus_condition_val": DocumentReviewStatus.PENDING_REVIEW, ":Author_condition_val": TEST_ODS_CODE})) -]) +@pytest.mark.parametrize( + "nhs_number, uploader, expected", + [ + ( + None, + None, + ( + "#ReviewStatus_attr = :ReviewStatus_condition_val", + {"#ReviewStatus_attr": "ReviewStatus"}, + {":ReviewStatus_condition_val": DocumentReviewStatus.PENDING_REVIEW}, + ), + ), + ( + TEST_NHS_NUMBER, + None, + ( + "#ReviewStatus_attr = :ReviewStatus_condition_val AND #NhsNumber_attr = :NhsNumber_condition_val", + {"#ReviewStatus_attr": "ReviewStatus", "#NhsNumber_attr": "NhsNumber"}, + { + ":ReviewStatus_condition_val": DocumentReviewStatus.PENDING_REVIEW, + ":NhsNumber_condition_val": TEST_NHS_NUMBER, + }, + ), + ), + ( + TEST_NHS_NUMBER, + TEST_ODS_CODE, + ( + ( + "#ReviewStatus_attr = :ReviewStatus_condition_val AND #NhsNumber_attr = :NhsNumber_condition_val AND " + "#Author_attr = :Author_condition_val" + ), + { + "#ReviewStatus_attr": "ReviewStatus", + "#NhsNumber_attr": "NhsNumber", + "#Author_attr": "Author", + }, + { + ":ReviewStatus_condition_val": DocumentReviewStatus.PENDING_REVIEW, + ":NhsNumber_condition_val": TEST_NHS_NUMBER, + ":Author_condition_val": TEST_ODS_CODE, + }, + ), + ), + ( + None, + TEST_ODS_CODE, + ( + "#ReviewStatus_attr = :ReviewStatus_condition_val AND #Author_attr = :Author_condition_val", + {"#ReviewStatus_attr": "ReviewStatus", "#Author_attr": "Author"}, + { + ":ReviewStatus_condition_val": DocumentReviewStatus.PENDING_REVIEW, + ":Author_condition_val": TEST_ODS_CODE, + }, + ), + ), + ], +) def test_build_paginator_query_filter(mock_service, nhs_number, uploader, expected): - actual = mock_service.build_paginator_query_filter(nhs_number=nhs_number, uploader=uploader) + actual = mock_service.build_paginator_query_filter( + nhs_number=nhs_number, uploader=uploader + ) assert actual == expected diff --git a/lambdas/tests/unit/services/test_search_document_review_service.py b/lambdas/tests/unit/services/test_search_document_review_service.py index e4eb159e3..add20d28a 100644 --- a/lambdas/tests/unit/services/test_search_document_review_service.py +++ b/lambdas/tests/unit/services/test_search_document_review_service.py @@ -62,9 +62,9 @@ def test_handle_gateway_api_request_happy_path(search_document_review_service, m "nhsNumber": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][0][ "NhsNumber" ], - "documentSnomedCodeType": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE[ - "Items" - ][0]["DocumentSnomedCodeType"], + "documentSnomedCodeType": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][ + 0 + ]["DocumentSnomedCodeType"], "author": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][0]["Author"], "uploadDate": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][0][ "UploadDate" @@ -79,9 +79,9 @@ def test_handle_gateway_api_request_happy_path(search_document_review_service, m "nhsNumber": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][1][ "NhsNumber" ], - "documentSnomedCodeType": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE[ - "Items" - ][1]["DocumentSnomedCodeType"], + "documentSnomedCodeType": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][ + 1 + ]["DocumentSnomedCodeType"], "author": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][1]["Author"], "uploadDate": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][1][ "UploadDate" @@ -96,9 +96,9 @@ def test_handle_gateway_api_request_happy_path(search_document_review_service, m "nhsNumber": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][2][ "NhsNumber" ], - "documentSnomedCodeType": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE[ - "Items" - ][2]["DocumentSnomedCodeType"], + "documentSnomedCodeType": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][ + 2 + ]["DocumentSnomedCodeType"], "author": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][2]["Author"], "uploadDate": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][2][ "UploadDate" @@ -245,4 +245,3 @@ def test_get_review_document_references_throws_exception_client_error( ) assert e.value.status_code == 500 assert e.value.error == LambdaError.DocumentReviewDB - diff --git a/lambdas/tests/unit/utils/test_dynamo_utils.py b/lambdas/tests/unit/utils/test_dynamo_utils.py index bdbceb773..c4bbd9edb 100644 --- a/lambdas/tests/unit/utils/test_dynamo_utils.py +++ b/lambdas/tests/unit/utils/test_dynamo_utils.py @@ -27,29 +27,32 @@ create_expression_value_placeholder, create_expressions, create_update_expression, - parse_dynamo_record, serialize_dict_to_dynamodb_object, deserialize_dynamodb_object, + deserialize_dynamodb_object, + parse_dynamo_record, + serialize_dict_to_dynamodb_object, ) from utils.lambda_exceptions import InvalidDocTypeException from lambdas.enums.snomed_codes import SnomedCodes - MOCK_PYTHON_DICT = { - "test_string": "hello", - "test_int": 123, - "test_bool": True, - "test_list": [1, 2, 3], - "test_dict": {"key1": "value1", "key2": 1}, - "test_list_of_dicts": [{"key1": "value1"}, {"key2": 2}], + "test_string": "hello", + "test_int": 123, + "test_bool": True, + "test_list": [1, 2, 3], + "test_dict": {"key1": "value1", "key2": 1}, + "test_list_of_dicts": [{"key1": "value1"}, {"key2": 2}], } MOCK_DYNAMO_DB_OBJECT = { - "test_string": {"S": "hello"}, - "test_int": {"N": "123"}, - "test_bool": {"BOOL": True}, - "test_list": {"L": [{"N": "1"}, {"N": "2"}, {"N": "3"}]}, - "test_dict": {"M": {"key1": {"S": "value1"}, "key2": {"N": "1"}}}, - "test_list_of_dicts": {"L": [{"M": {"key1":{"S":"value1"}}},{"M":{"key2": {"N": "2"}}}]}, + "test_string": {"S": "hello"}, + "test_int": {"N": "123"}, + "test_bool": {"BOOL": True}, + "test_list": {"L": [{"N": "1"}, {"N": "2"}, {"N": "3"}]}, + "test_dict": {"M": {"key1": {"S": "value1"}, "key2": {"N": "1"}}}, + "test_list_of_dicts": { + "L": [{"M": {"key1": {"S": "value1"}}}, {"M": {"key2": {"N": "2"}}}] + }, } @@ -572,4 +575,3 @@ def test_serialize_dynamodb_object_throws_error_unsupported_data_type(): with pytest.raises(TypeError): serialize_dict_to_dynamodb_object(unsupported_input) - diff --git a/lambdas/utils/dynamo_utils.py b/lambdas/utils/dynamo_utils.py index 782f97da2..5bdd69633 100644 --- a/lambdas/utils/dynamo_utils.py +++ b/lambdas/utils/dynamo_utils.py @@ -2,8 +2,7 @@ from typing import Any, Dict import inflection -from boto3.dynamodb.types import TypeSerializer, TypeDeserializer - +from boto3.dynamodb.types import TypeDeserializer, TypeSerializer from enums.dynamo_filter import AttributeOperator from enums.infrastructure import DynamoTables from enums.lambda_error import LambdaError @@ -328,18 +327,12 @@ def build_transaction_item( def serialize_dict_to_dynamodb_object(object: dict) -> dict: serializer = TypeSerializer() - return { - key: serializer.serialize(value) - for key, value in object.items() - } + return {key: serializer.serialize(value) for key, value in object.items()} def deserialize_dynamodb_object(object: dict) -> dict: deserialize = TypeDeserializer().deserialize - parsed_dynamodb_items = { - key: deserialize(value) - for key, value in object.items() - } + parsed_dynamodb_items = {key: deserialize(value) for key, value in object.items()} return parsed_dynamodb_items From be2a8f2682d1401e0324069d36c8164fe4f77d07 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Wed, 17 Dec 2025 14:39:50 +0000 Subject: [PATCH 11/21] [PRMP-1079] add test for querying with paginator --- .../document_upload_review_service.py | 2 +- .../search_document_review/dynamo_response.py | 57 +++++++++++++++++++ .../test_document_upload_review_service.py | 33 ++++++++++- 3 files changed, 90 insertions(+), 2 deletions(-) diff --git a/lambdas/services/document_upload_review_service.py b/lambdas/services/document_upload_review_service.py index 114901a6b..e8b73c28b 100644 --- a/lambdas/services/document_upload_review_service.py +++ b/lambdas/services/document_upload_review_service.py @@ -96,7 +96,7 @@ def query_docs_pending_review_with_paginator( key_condition = {"S": ods_code} filter_expression, condition_attribute_names, condition_attribute_values = ( - self.build_paginator_query_filter(nhs_number, uploader) + self.build_paginator_query_filter(nhs_number=nhs_number, uploader=uploader) ) serialized_condition_attribute_values = serialize_dict_to_dynamodb_object( diff --git a/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py b/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py index 6e1df3c8d..8aa2bc0c5 100644 --- a/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py +++ b/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py @@ -98,3 +98,60 @@ }, "LastEvaluatedKey": TEST_UUID, } + +MOCK_DOCUMENT_REVIEW_PAGINATOR_RESPONSE = { + "Items": [ + { + "ID": {"S": "3d8683b9-1665-40d2-8499-6e8302d507ff"}, + "Version": {"N": "1"}, + "Files": {"L": [ + {"M": {"FileLocation": {"S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-123"}, "FileName": {"S": "document.csv"}}}, + {"M": {"FileLocation": {"S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-223"}, "FileName": {"S": "results.pdf"}}} + ]}, + "Author": {"S": MOCK_PREVIOUS_ODS_CODE}, + "Custodian": {"S": TEST_CURRENT_GP_ODS}, + "UploadDate": {"N": "1704110400"}, + "NhsNumber": {"S": TEST_NHS_NUMBER}, + "ReviewReason": {"S": "Failure"}, + "ReviewStatus": {"S": DocumentReviewStatus.PENDING_REVIEW.value}, + "LastUpdated": {"N": "1704110400"}, + "DocumentSnomedCodeType": {"S": SnomedCodes.LLOYD_GEORGE.value.code}, + }, + { + "ID": {"S": "4d8683b9-1665-40d2-8499-6e8302d507ff"}, + "Version": {"N": "2"}, + "Files": {"L": [ + {"M": {"FileLocation": {"S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-123"}, "FileName": {"S": "document.csv"}}}, + {"M": {"FileLocation": {"S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-223"}, "FileName": {"S": "results.pdf"}}} + ]}, + "Author": {"S": MOCK_PREVIOUS_ODS_CODE}, + "Custodian": {"S": TEST_CURRENT_GP_ODS}, + "UploadDate": {"N": "1704110400"}, + "NhsNumber": {"S": TEST_NHS_NUMBER}, + "ReviewReason": {"S": "Failure"}, + "ReviewStatus": {"S": DocumentReviewStatus.PENDING_REVIEW.value}, + "LastUpdated": {"N": "1704110400"}, + "DocumentSnomedCodeType": {"S": SnomedCodes.LLOYD_GEORGE.value.code}, + }, + { + "ID": {"S": "5d8683b9-1665-40d2-8499-6e8302d507ff"}, + "Version": {"N": "3"}, + "Files": {"L": [ + {"M": {"FileLocation": {"S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-123"}, + "FileName": {"S": "document.csv"}}}, + {"M": {"FileLocation": {"S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-223"}, + "FileName": {"S": "results.pdf"}}} + ]}, + "Author": {"S": MOCK_PREVIOUS_ODS_CODE}, + "Custodian": {"S": TEST_CURRENT_GP_ODS}, + "UploadDate": {"N": "1704110400"}, + "NhsNumber": {"S": TEST_NHS_NUMBER}, + "ReviewReason": {"S": "Failure"}, + "ReviewStatus": {"S": DocumentReviewStatus.PENDING_REVIEW.value}, + "LastUpdated": {"N": "1704110400"}, + "DocumentSnomedCodeType": {"S": SnomedCodes.LLOYD_GEORGE.value.code}, + }, + + ], + "NextToken": TEST_UUID, +} diff --git a/lambdas/tests/unit/services/test_document_upload_review_service.py b/lambdas/tests/unit/services/test_document_upload_review_service.py index b2839366f..cf0fd5873 100644 --- a/lambdas/tests/unit/services/test_document_upload_review_service.py +++ b/lambdas/tests/unit/services/test_document_upload_review_service.py @@ -14,7 +14,7 @@ TEST_UUID, ) from tests.unit.helpers.data.search_document_review.dynamo_response import ( - MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE, + MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE, MOCK_DOCUMENT_REVIEW_PAGINATOR_RESPONSE, ) from utils.exceptions import DocumentReviewException @@ -58,6 +58,12 @@ def mock_review_update(): return review_update +@pytest.fixture +def mock_paginator(set_env, mocker): + mocker.patch("") + + + def test_table_name(mock_service): """Test that table_name property returns correct environment variable.""" @@ -635,3 +641,28 @@ def test_build_paginator_query_filter(mock_service, nhs_number, uploader, expect ) assert actual == expected + + +def test_query_docs_pending_review_with_paginator(mock_service, mocker): + mock_serialize = mocker.patch("services.document_upload_review_service.serialize_dict_to_dynamodb_object") + mock_query_filter = mocker.patch.object(mock_service, "build_paginator_query_filter") + mock_query_filter.return_value = "#ReviewStatus_attr = :ReviewStatus_condition_val", {"#ReviewStatus_attr": "ReviewStatus"}, {":ReviewStatus_condition_val": DocumentReviewStatus.PENDING_REVIEW}, + + mock_client = MagicMock() + mocker.patch("boto3.client", return_value=mock_client) + mock_paginator = MagicMock() + + mock_client.get_paginator.return_value = mock_paginator + + mock_paginator.paginate.return_value.build_full_result.return_value = MOCK_DOCUMENT_REVIEW_PAGINATOR_RESPONSE + + expected = ([DocumentUploadReviewReference(**item) for item in MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"]], TEST_UUID) + + actual = mock_service.query_docs_pending_review_with_paginator(TEST_ODS_CODE) + + mock_query_filter.assert_called_with(nhs_number=None, uploader=None) + mock_serialize.assert_called() + mock_paginator.paginate.assert_called() + + assert actual == expected + From bad0827359885cc2e579da3516353b981cb2236d Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Wed, 17 Dec 2025 17:38:49 +0000 Subject: [PATCH 12/21] [PRMP-1079] extract query with paginator to dynamo service --- lambdas/services/base/dynamo_service.py | 50 ++++++++++++++++++- .../search_document_review/dynamo_response.py | 8 ++- .../unit/services/base/test_dynamo_service.py | 35 ++++++++++++- lambdas/utils/dynamo_utils.py | 8 ++- 4 files changed, 93 insertions(+), 8 deletions(-) diff --git a/lambdas/services/base/dynamo_service.py b/lambdas/services/base/dynamo_service.py index 04c16c87a..65ed77942 100644 --- a/lambdas/services/base/dynamo_service.py +++ b/lambdas/services/base/dynamo_service.py @@ -10,7 +10,8 @@ from utils.dynamo_utils import ( create_expression_attribute_values, create_expressions, - create_update_expression, + create_update_expression, serialize_dict_to_dynamodb_object, + deserialize_dynamodb_object ) from utils.exceptions import DynamoServiceException @@ -29,6 +30,7 @@ def __new__(cls): def __init__(self): if not self.initialised: self.dynamodb = boto3.resource("dynamodb", region_name="eu-west-2") + self.client = boto3.client("dynamodb") self.initialised = True def get_table(self, table_name: str): @@ -429,3 +431,49 @@ def build_update_transaction_item( }, } } + + + def query_table_with_paginator( + self, + table_name: str, + index_name: str, + key: str, + condition: dict, + filter_expression: str | None = None, + expression_attribute_names: str | None = None, + expression_attribute_values: dict = {}, + limit: int = 20, + page_size: int = 1, + start_key: str | None = None, + ) -> dict: + + query_params = { + "TableName": table_name, + "IndexName": index_name, + "KeyConditionExpression": f"{key}=:i", + "PaginationConfig": { + "MaxItems": limit, + "PageSize": page_size, + "StartingToken": start_key, + } + } + + expression_attribute_values[":i"] = condition + + if filter_expression: + query_params["FilterExpression"] = filter_expression + + if expression_attribute_names: + query_params["ExpressionAttributeNames"] = expression_attribute_names + + if expression_attribute_values: + query_params["ExpressionAttributeValues"] = serialize_dict_to_dynamodb_object( + expression_attribute_values) + + paginator = self.client.get_paginator("query") + response = paginator.paginate( + **query_params + ).build_full_result() + + response["Items"] = [deserialize_dynamodb_object(item) for item in response["Items"]] + return response diff --git a/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py b/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py index 8aa2bc0c5..3ffb2f4b3 100644 --- a/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py +++ b/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py @@ -29,7 +29,7 @@ "UploadDate": 1704110400, "NhsNumber": TEST_NHS_NUMBER, "ReviewReason": "Failure", - "ReviewStatus": DocumentReviewStatus.PENDING_REVIEW, + "ReviewStatus": DocumentReviewStatus.PENDING_REVIEW.value, "LastUpdated": 1704110400, # Timestamp: 2024-01-01T12:00:00 "DocumentSnomedCodeType": SnomedCodes.LLOYD_GEORGE.value.code, }, @@ -51,7 +51,7 @@ "UploadDate": 1704110400, "NhsNumber": TEST_NHS_NUMBER, "ReviewReason": "Failure", - "ReviewStatus": DocumentReviewStatus.PENDING_REVIEW, + "ReviewStatus": DocumentReviewStatus.PENDING_REVIEW.value, "LastUpdated": 1704110400, # Timestamp: 2024-01-01T12:00:00 "DocumentSnomedCodeType": SnomedCodes.LLOYD_GEORGE.value.code, }, @@ -71,11 +71,9 @@ "Author": MOCK_PREVIOUS_ODS_CODE, "Custodian": TEST_CURRENT_GP_ODS, "UploadDate": 1704110400, - "Reviewer": None, "NhsNumber": TEST_NHS_NUMBER, - "ReviewDate": None, "ReviewReason": "Failure", - "ReviewStatus": DocumentReviewStatus.PENDING_REVIEW, + "ReviewStatus": DocumentReviewStatus.PENDING_REVIEW.value, "LastUpdated": 1704110400, # Timestamp: 2024-01-01T12:00:00 "DocumentSnomedCodeType": SnomedCodes.LLOYD_GEORGE.value.code, }, diff --git a/lambdas/tests/unit/services/base/test_dynamo_service.py b/lambdas/tests/unit/services/base/test_dynamo_service.py index 597d8f568..d11e5d32d 100755 --- a/lambdas/tests/unit/services/base/test_dynamo_service.py +++ b/lambdas/tests/unit/services/base/test_dynamo_service.py @@ -1,5 +1,5 @@ import copy -from unittest.mock import call +from unittest.mock import call, MagicMock import pytest from boto3.dynamodb.conditions import And, Attr, Equals, Key @@ -9,6 +9,7 @@ from services.base.dynamo_service import DynamoDBService from tests.unit.conftest import MOCK_CLIENT_ERROR, MOCK_TABLE_NAME, TEST_NHS_NUMBER from tests.unit.helpers.data.dynamo.dynamo_responses import MOCK_SEARCH_RESPONSE +from tests.unit.helpers.data.search_document_review.dynamo_response import MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE, MOCK_DOCUMENT_REVIEW_PAGINATOR_RESPONSE from tests.unit.helpers.data.dynamo.dynamo_scan_response import ( EXPECTED_ITEMS_FOR_PAGINATED_RESULTS, MOCK_PAGINATED_RESPONSE_1, @@ -24,7 +25,9 @@ @pytest.fixture def mock_service(set_env, mocker): mocker.patch("boto3.resource") + mock_client = MagicMock() service = DynamoDBService() + mocker.patch.object(service, "client", return_value=mock_client) yield service DynamoDBService.instance = None @@ -1196,3 +1199,33 @@ def test_build_key_condition_non_matching_list_lengths( mock_service.build_key_condition( search_key=search_key, search_condition=search_condition ) + + +def test_query_table_using_paginator(mock_service): + mock_paginator = mock_service.client.get_paginator.return_value = MagicMock() + + mock_paginator.paginate.return_value.build_full_result.return_value = MOCK_DOCUMENT_REVIEW_PAGINATOR_RESPONSE + + expected = {"Items": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"], "NextToken": MOCK_DOCUMENT_REVIEW_PAGINATOR_RESPONSE["NextToken"]} + + actual = mock_service.query_table_with_paginator( + table_name=MOCK_TABLE_NAME, + index_name="NhsNumberIndex", + key="NhsNumber", + condition=TEST_NHS_NUMBER, + ) + + mock_paginator.paginate.assert_called_with( + TableName=MOCK_TABLE_NAME, + IndexName="NhsNumberIndex", + KeyConditionExpression="NhsNumber=:i", + ExpressionAttributeValues={":i": {"S": TEST_NHS_NUMBER}}, + PaginationConfig={"MaxItems": 20, "PageSize": 1, "StartingToken": None}, + ) + + assert actual == expected + +def test_query_table_using_pagination_with_filter_expression(mock_service): + mock_paginator = mock_service.client.get_paginator.return_value = MagicMock() + + pass diff --git a/lambdas/utils/dynamo_utils.py b/lambdas/utils/dynamo_utils.py index 5bdd69633..0f85231ca 100644 --- a/lambdas/utils/dynamo_utils.py +++ b/lambdas/utils/dynamo_utils.py @@ -3,6 +3,8 @@ import inflection from boto3.dynamodb.types import TypeDeserializer, TypeSerializer +from decimal import Decimal + from enums.dynamo_filter import AttributeOperator from enums.infrastructure import DynamoTables from enums.lambda_error import LambdaError @@ -332,7 +334,11 @@ def serialize_dict_to_dynamodb_object(object: dict) -> dict: def deserialize_dynamodb_object(object: dict) -> dict: deserialize = TypeDeserializer().deserialize - parsed_dynamodb_items = {key: deserialize(value) for key, value in object.items()} + parsed_dynamodb_items = {} + for key, value in object.items(): + parsed_dynamodb_items[key] = deserialize(value) + if type(deserialize(value)) is Decimal: + parsed_dynamodb_items[key] = int(deserialize(value)) return parsed_dynamodb_items From 3caa1b08bd5cebcdd294eeaa0317781a71de5118 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Wed, 17 Dec 2025 18:01:49 +0000 Subject: [PATCH 13/21] [PRMP-1079] document service uses dynamo service to query with paginator --- lambdas/services/base/dynamo_service.py | 51 ++++++++++--------- .../document_upload_review_service.py | 39 ++++---------- .../unit/services/base/test_dynamo_service.py | 41 ++++++++++++++- .../test_document_upload_review_service.py | 28 +++++----- 4 files changed, 93 insertions(+), 66 deletions(-) diff --git a/lambdas/services/base/dynamo_service.py b/lambdas/services/base/dynamo_service.py index 65ed77942..666bc3b94 100644 --- a/lambdas/services/base/dynamo_service.py +++ b/lambdas/services/base/dynamo_service.py @@ -447,33 +447,38 @@ def query_table_with_paginator( start_key: str | None = None, ) -> dict: - query_params = { - "TableName": table_name, - "IndexName": index_name, - "KeyConditionExpression": f"{key}=:i", - "PaginationConfig": { - "MaxItems": limit, - "PageSize": page_size, - "StartingToken": start_key, + try: + query_params = { + "TableName": table_name, + "IndexName": index_name, + "KeyConditionExpression": f"{key}=:i", + "PaginationConfig": { + "MaxItems": limit, + "PageSize": page_size, + "StartingToken": start_key, + } } - } - expression_attribute_values[":i"] = condition + expression_attribute_values[":i"] = condition + + if filter_expression: + query_params["FilterExpression"] = filter_expression - if filter_expression: - query_params["FilterExpression"] = filter_expression + if expression_attribute_names: + query_params["ExpressionAttributeNames"] = expression_attribute_names - if expression_attribute_names: - query_params["ExpressionAttributeNames"] = expression_attribute_names + if expression_attribute_values: + query_params["ExpressionAttributeValues"] = serialize_dict_to_dynamodb_object( + expression_attribute_values) - if expression_attribute_values: - query_params["ExpressionAttributeValues"] = serialize_dict_to_dynamodb_object( - expression_attribute_values) + paginator = self.client.get_paginator("query") + response = paginator.paginate( + **query_params + ).build_full_result() - paginator = self.client.get_paginator("query") - response = paginator.paginate( - **query_params - ).build_full_result() + response["Items"] = [deserialize_dynamodb_object(item) for item in response["Items"]] + return response - response["Items"] = [deserialize_dynamodb_object(item) for item in response["Items"]] - return response + except Exception as e: + logger.error("Failed to query DynamoDB") + raise e diff --git a/lambdas/services/document_upload_review_service.py b/lambdas/services/document_upload_review_service.py index e8b73c28b..e75a9201c 100644 --- a/lambdas/services/document_upload_review_service.py +++ b/lambdas/services/document_upload_review_service.py @@ -90,39 +90,22 @@ def query_docs_pending_review_with_paginator( ) -> tuple[list[DocumentUploadReviewReference], dict | None]: logger.info(f"Getting review document references for custodian: {ods_code}") - client = boto3.client("dynamodb") - paginator = client.get_paginator("query") - - key_condition = {"S": ods_code} - filter_expression, condition_attribute_names, condition_attribute_values = ( self.build_paginator_query_filter(nhs_number=nhs_number, uploader=uploader) ) - - serialized_condition_attribute_values = serialize_dict_to_dynamodb_object( - condition_attribute_values + response = self.dynamo_service.query_table_using_paginator( + table_name=self.table_name, + index_name="CustodianIndex", + key="Custodian", + condition=ods_code, + filter_expression=filter_expression, + expression_attribute_names=condition_attribute_names, + expression_attribute_values=condition_attribute_values, + start_key=start_key, + limit=limit, ) - serialized_condition_attribute_values.update({":c": key_condition}) - - response = paginator.paginate( - TableName=self.table_name, - IndexName="CustodianIndex", - KeyConditionExpression="Custodian=:c", - FilterExpression=filter_expression, - ExpressionAttributeValues=serialized_condition_attribute_values, - ExpressionAttributeNames=condition_attribute_names, - PaginationConfig={ - "MaxItems": limit, - "PageSize": 1, - "StartingToken": start_key, - }, - ).build_full_result() - - deserialized_items = [ - deserialize_dynamodb_object(item) for item in response.get("Items", []) - ] - references = self._validate_review_references(deserialized_items) + references = self._validate_review_references(response["Items"]) last_evaluated_key = response.get("NextToken", None) return references, last_evaluated_key diff --git a/lambdas/tests/unit/services/base/test_dynamo_service.py b/lambdas/tests/unit/services/base/test_dynamo_service.py index d11e5d32d..bb4cf52f8 100755 --- a/lambdas/tests/unit/services/base/test_dynamo_service.py +++ b/lambdas/tests/unit/services/base/test_dynamo_service.py @@ -18,6 +18,7 @@ MOCK_RESPONSE, MOCK_RESPONSE_WITH_LAST_KEY, ) +from utils.dynamo_utils import build_mixed_condition_expression, serialize_dict_to_dynamodb_object from utils.dynamo_query_filter_builder import DynamoQueryFilterBuilder from utils.exceptions import DynamoServiceException @@ -1228,4 +1229,42 @@ def test_query_table_using_paginator(mock_service): def test_query_table_using_pagination_with_filter_expression(mock_service): mock_paginator = mock_service.client.get_paginator.return_value = MagicMock() - pass + conditions=[ + { + "field": "ReviewStatus", + "operator": "=", + "value": "PENDING_REVIEW", + }, + { + "field": "NhsNumber", + "operator": "=", + "value": TEST_NHS_NUMBER, + } + ] + filter_expression, condition_attribute_names, condition_attribute_values = ( + build_mixed_condition_expression(conditions=conditions) + ) + + serialized_condition_attribute_values = serialize_dict_to_dynamodb_object( + condition_attribute_values + ) + + mock_service.query_table_with_paginator( + table_name=MOCK_TABLE_NAME, + index_name="NhsNumberIndex", + key="NhsNumber", + condition=TEST_NHS_NUMBER, + filter_expression=filter_expression, + expression_attribute_names=condition_attribute_names, + expression_attribute_values=condition_attribute_values, + ) + + mock_paginator.paginate.assert_called_with( + TableName=MOCK_TABLE_NAME, + IndexName="NhsNumberIndex", + KeyConditionExpression="NhsNumber=:i", + FilterExpression=filter_expression, + ExpressionAttributeValues={":i": {"S": TEST_NHS_NUMBER}, **serialized_condition_attribute_values}, + ExpressionAttributeNames=condition_attribute_names, + PaginationConfig={"MaxItems": 20, "PageSize": 1, "StartingToken": None}, + ) \ No newline at end of file diff --git a/lambdas/tests/unit/services/test_document_upload_review_service.py b/lambdas/tests/unit/services/test_document_upload_review_service.py index cf0fd5873..6b0a48e60 100644 --- a/lambdas/tests/unit/services/test_document_upload_review_service.py +++ b/lambdas/tests/unit/services/test_document_upload_review_service.py @@ -643,26 +643,26 @@ def test_build_paginator_query_filter(mock_service, nhs_number, uploader, expect assert actual == expected -def test_query_docs_pending_review_with_paginator(mock_service, mocker): - mock_serialize = mocker.patch("services.document_upload_review_service.serialize_dict_to_dynamodb_object") - mock_query_filter = mocker.patch.object(mock_service, "build_paginator_query_filter") - mock_query_filter.return_value = "#ReviewStatus_attr = :ReviewStatus_condition_val", {"#ReviewStatus_attr": "ReviewStatus"}, {":ReviewStatus_condition_val": DocumentReviewStatus.PENDING_REVIEW}, +def test_query_docs_pending_review_with_paginator(mock_service): + filter_expression, condition_attribute_names, condition_attribute_values = mock_service.build_paginator_query_filter() - mock_client = MagicMock() - mocker.patch("boto3.client", return_value=mock_client) - mock_paginator = MagicMock() - - mock_client.get_paginator.return_value = mock_paginator - - mock_paginator.paginate.return_value.build_full_result.return_value = MOCK_DOCUMENT_REVIEW_PAGINATOR_RESPONSE + mock_service.dynamo_service.query_table_using_paginator.return_value = {"Items": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"], "NextToken": TEST_UUID} expected = ([DocumentUploadReviewReference(**item) for item in MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"]], TEST_UUID) actual = mock_service.query_docs_pending_review_with_paginator(TEST_ODS_CODE) - mock_query_filter.assert_called_with(nhs_number=None, uploader=None) - mock_serialize.assert_called() - mock_paginator.paginate.assert_called() + mock_service.dynamo_service.query_table_using_paginator.assert_called_with( + table_name=MOCK_DOCUMENT_REVIEW_TABLE, + index_name="CustodianIndex", + key="Custodian", + condition=TEST_ODS_CODE, + filter_expression=filter_expression, + expression_attribute_names=condition_attribute_names, + expression_attribute_values=condition_attribute_values, + limit=50, + start_key=None, + ) assert actual == expected From c4df7ccf43531118ee7dd074b7efbfbb1d2fe098 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Wed, 17 Dec 2025 18:03:36 +0000 Subject: [PATCH 14/21] [PRMP-1079] format --- lambdas/services/base/dynamo_service.py | 21 +++--- .../document_upload_review_service.py | 3 - .../search_document_review/dynamo_response.py | 75 +++++++++++++++---- .../test_document_upload_review_service.py | 21 ++++-- lambdas/utils/dynamo_utils.py | 3 +- 5 files changed, 87 insertions(+), 36 deletions(-) diff --git a/lambdas/services/base/dynamo_service.py b/lambdas/services/base/dynamo_service.py index 666bc3b94..48d6afff7 100644 --- a/lambdas/services/base/dynamo_service.py +++ b/lambdas/services/base/dynamo_service.py @@ -10,8 +10,9 @@ from utils.dynamo_utils import ( create_expression_attribute_values, create_expressions, - create_update_expression, serialize_dict_to_dynamodb_object, - deserialize_dynamodb_object + create_update_expression, + deserialize_dynamodb_object, + serialize_dict_to_dynamodb_object, ) from utils.exceptions import DynamoServiceException @@ -432,7 +433,6 @@ def build_update_transaction_item( } } - def query_table_with_paginator( self, table_name: str, @@ -456,7 +456,7 @@ def query_table_with_paginator( "MaxItems": limit, "PageSize": page_size, "StartingToken": start_key, - } + }, } expression_attribute_values[":i"] = condition @@ -468,15 +468,16 @@ def query_table_with_paginator( query_params["ExpressionAttributeNames"] = expression_attribute_names if expression_attribute_values: - query_params["ExpressionAttributeValues"] = serialize_dict_to_dynamodb_object( - expression_attribute_values) + query_params["ExpressionAttributeValues"] = ( + serialize_dict_to_dynamodb_object(expression_attribute_values) + ) paginator = self.client.get_paginator("query") - response = paginator.paginate( - **query_params - ).build_full_result() + response = paginator.paginate(**query_params).build_full_result() - response["Items"] = [deserialize_dynamodb_object(item) for item in response["Items"]] + response["Items"] = [ + deserialize_dynamodb_object(item) for item in response["Items"] + ] return response except Exception as e: diff --git a/lambdas/services/document_upload_review_service.py b/lambdas/services/document_upload_review_service.py index e75a9201c..f10b8773c 100644 --- a/lambdas/services/document_upload_review_service.py +++ b/lambdas/services/document_upload_review_service.py @@ -1,6 +1,5 @@ import os -import boto3 from boto3.dynamodb.conditions import Attr, ConditionBase from botocore.exceptions import ClientError from enums.document_review_status import DocumentReviewStatus @@ -15,8 +14,6 @@ from utils.dynamo_utils import ( build_mixed_condition_expression, build_transaction_item, - deserialize_dynamodb_object, - serialize_dict_to_dynamodb_object, ) from utils.exceptions import DocumentReviewException diff --git a/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py b/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py index 3ffb2f4b3..2e83a6506 100644 --- a/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py +++ b/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py @@ -102,10 +102,26 @@ { "ID": {"S": "3d8683b9-1665-40d2-8499-6e8302d507ff"}, "Version": {"N": "1"}, - "Files": {"L": [ - {"M": {"FileLocation": {"S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-123"}, "FileName": {"S": "document.csv"}}}, - {"M": {"FileLocation": {"S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-223"}, "FileName": {"S": "results.pdf"}}} - ]}, + "Files": { + "L": [ + { + "M": { + "FileLocation": { + "S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-123" + }, + "FileName": {"S": "document.csv"}, + } + }, + { + "M": { + "FileLocation": { + "S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-223" + }, + "FileName": {"S": "results.pdf"}, + } + }, + ] + }, "Author": {"S": MOCK_PREVIOUS_ODS_CODE}, "Custodian": {"S": TEST_CURRENT_GP_ODS}, "UploadDate": {"N": "1704110400"}, @@ -118,10 +134,26 @@ { "ID": {"S": "4d8683b9-1665-40d2-8499-6e8302d507ff"}, "Version": {"N": "2"}, - "Files": {"L": [ - {"M": {"FileLocation": {"S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-123"}, "FileName": {"S": "document.csv"}}}, - {"M": {"FileLocation": {"S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-223"}, "FileName": {"S": "results.pdf"}}} - ]}, + "Files": { + "L": [ + { + "M": { + "FileLocation": { + "S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-123" + }, + "FileName": {"S": "document.csv"}, + } + }, + { + "M": { + "FileLocation": { + "S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-223" + }, + "FileName": {"S": "results.pdf"}, + } + }, + ] + }, "Author": {"S": MOCK_PREVIOUS_ODS_CODE}, "Custodian": {"S": TEST_CURRENT_GP_ODS}, "UploadDate": {"N": "1704110400"}, @@ -134,12 +166,26 @@ { "ID": {"S": "5d8683b9-1665-40d2-8499-6e8302d507ff"}, "Version": {"N": "3"}, - "Files": {"L": [ - {"M": {"FileLocation": {"S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-123"}, - "FileName": {"S": "document.csv"}}}, - {"M": {"FileLocation": {"S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-223"}, - "FileName": {"S": "results.pdf"}}} - ]}, + "Files": { + "L": [ + { + "M": { + "FileLocation": { + "S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-123" + }, + "FileName": {"S": "document.csv"}, + } + }, + { + "M": { + "FileLocation": { + "S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-223" + }, + "FileName": {"S": "results.pdf"}, + } + }, + ] + }, "Author": {"S": MOCK_PREVIOUS_ODS_CODE}, "Custodian": {"S": TEST_CURRENT_GP_ODS}, "UploadDate": {"N": "1704110400"}, @@ -149,7 +195,6 @@ "LastUpdated": {"N": "1704110400"}, "DocumentSnomedCodeType": {"S": SnomedCodes.LLOYD_GEORGE.value.code}, }, - ], "NextToken": TEST_UUID, } diff --git a/lambdas/tests/unit/services/test_document_upload_review_service.py b/lambdas/tests/unit/services/test_document_upload_review_service.py index 6b0a48e60..64a40c258 100644 --- a/lambdas/tests/unit/services/test_document_upload_review_service.py +++ b/lambdas/tests/unit/services/test_document_upload_review_service.py @@ -14,7 +14,7 @@ TEST_UUID, ) from tests.unit.helpers.data.search_document_review.dynamo_response import ( - MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE, MOCK_DOCUMENT_REVIEW_PAGINATOR_RESPONSE, + MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE, ) from utils.exceptions import DocumentReviewException @@ -63,7 +63,6 @@ def mock_paginator(set_env, mocker): mocker.patch("") - def test_table_name(mock_service): """Test that table_name property returns correct environment variable.""" @@ -644,11 +643,22 @@ def test_build_paginator_query_filter(mock_service, nhs_number, uploader, expect def test_query_docs_pending_review_with_paginator(mock_service): - filter_expression, condition_attribute_names, condition_attribute_values = mock_service.build_paginator_query_filter() + filter_expression, condition_attribute_names, condition_attribute_values = ( + mock_service.build_paginator_query_filter() + ) - mock_service.dynamo_service.query_table_using_paginator.return_value = {"Items": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"], "NextToken": TEST_UUID} + mock_service.dynamo_service.query_table_using_paginator.return_value = { + "Items": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"], + "NextToken": TEST_UUID, + } - expected = ([DocumentUploadReviewReference(**item) for item in MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"]], TEST_UUID) + expected = ( + [ + DocumentUploadReviewReference(**item) + for item in MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"] + ], + TEST_UUID, + ) actual = mock_service.query_docs_pending_review_with_paginator(TEST_ODS_CODE) @@ -665,4 +675,3 @@ def test_query_docs_pending_review_with_paginator(mock_service): ) assert actual == expected - diff --git a/lambdas/utils/dynamo_utils.py b/lambdas/utils/dynamo_utils.py index 0f85231ca..bb84187e5 100644 --- a/lambdas/utils/dynamo_utils.py +++ b/lambdas/utils/dynamo_utils.py @@ -1,10 +1,9 @@ from datetime import datetime +from decimal import Decimal from typing import Any, Dict import inflection from boto3.dynamodb.types import TypeDeserializer, TypeSerializer -from decimal import Decimal - from enums.dynamo_filter import AttributeOperator from enums.infrastructure import DynamoTables from enums.lambda_error import LambdaError From bec65faad7cf9bbd0b24335d98bfdba48f29f714 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Thu, 18 Dec 2025 09:37:26 +0000 Subject: [PATCH 15/21] [PRMP-1079] update method used to align with actual method definition --- lambdas/services/document_upload_review_service.py | 4 ++-- .../unit/services/test_document_upload_review_service.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lambdas/services/document_upload_review_service.py b/lambdas/services/document_upload_review_service.py index f10b8773c..ea26a5f4b 100644 --- a/lambdas/services/document_upload_review_service.py +++ b/lambdas/services/document_upload_review_service.py @@ -90,7 +90,7 @@ def query_docs_pending_review_with_paginator( filter_expression, condition_attribute_names, condition_attribute_values = ( self.build_paginator_query_filter(nhs_number=nhs_number, uploader=uploader) ) - response = self.dynamo_service.query_table_using_paginator( + response = self.dynamo_service.query_table_with_paginator( table_name=self.table_name, index_name="CustodianIndex", key="Custodian", @@ -103,7 +103,7 @@ def query_docs_pending_review_with_paginator( ) references = self._validate_review_references(response["Items"]) - last_evaluated_key = response.get("NextToken", None) + last_evaluated_key = response.get("NextToken") return references, last_evaluated_key diff --git a/lambdas/tests/unit/services/test_document_upload_review_service.py b/lambdas/tests/unit/services/test_document_upload_review_service.py index 64a40c258..cdc7af2df 100644 --- a/lambdas/tests/unit/services/test_document_upload_review_service.py +++ b/lambdas/tests/unit/services/test_document_upload_review_service.py @@ -647,7 +647,7 @@ def test_query_docs_pending_review_with_paginator(mock_service): mock_service.build_paginator_query_filter() ) - mock_service.dynamo_service.query_table_using_paginator.return_value = { + mock_service.dynamo_service.query_table_with_paginator.return_value = { "Items": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"], "NextToken": TEST_UUID, } @@ -662,7 +662,7 @@ def test_query_docs_pending_review_with_paginator(mock_service): actual = mock_service.query_docs_pending_review_with_paginator(TEST_ODS_CODE) - mock_service.dynamo_service.query_table_using_paginator.assert_called_with( + mock_service.dynamo_service.query_table_with_paginator.assert_called_with( table_name=MOCK_DOCUMENT_REVIEW_TABLE, index_name="CustodianIndex", key="Custodian", From fdc677ca53cb1e576491b60d29f473d798c22f93 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Thu, 18 Dec 2025 15:43:38 +0000 Subject: [PATCH 16/21] [PRMP-1079] update type hint on argument --- lambdas/services/base/dynamo_service.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lambdas/services/base/dynamo_service.py b/lambdas/services/base/dynamo_service.py index 48d6afff7..46bb580db 100644 --- a/lambdas/services/base/dynamo_service.py +++ b/lambdas/services/base/dynamo_service.py @@ -438,7 +438,7 @@ def query_table_with_paginator( table_name: str, index_name: str, key: str, - condition: dict, + condition: str, filter_expression: str | None = None, expression_attribute_names: str | None = None, expression_attribute_values: dict = {}, From 024fc72c31e3bf074a4676b8c91b352e4325be85 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Fri, 19 Dec 2025 11:05:39 +0000 Subject: [PATCH 17/21] [PRMP-1079] test refactoring --- .../document_upload_review_service.py | 60 +++++-------------- .../test_document_upload_review_service.py | 54 ++++++++--------- 2 files changed, 41 insertions(+), 73 deletions(-) diff --git a/lambdas/services/document_upload_review_service.py b/lambdas/services/document_upload_review_service.py index ea26a5f4b..cf588d1ee 100644 --- a/lambdas/services/document_upload_review_service.py +++ b/lambdas/services/document_upload_review_service.py @@ -42,71 +42,41 @@ def model_class(self) -> type: def s3_bucket(self) -> str: return self._s3_bucket - def query_docs_pending_review_by_custodian_with_limit( + def query_docs_pending_review_with_paginator( self, ods_code: str, limit: int = DEFAULT_QUERY_LIMIT, - start_key: dict | None = None, + start_key: str | None = None, nhs_number: str | None = None, uploader: str | None = None, ) -> tuple[list[DocumentUploadReviewReference], dict | None]: - logger.info(f"Getting review document references for custodian: {ods_code}") - - filter_expression = self.build_review_query_filter( - nhs_number=nhs_number, uploader=uploader - ) try: - response = self.dynamo_service.query_table_single( + logger.info(f"Getting review document references for custodian: {ods_code}") + + filter_expression, condition_attribute_names, condition_attribute_values = ( + self.build_paginator_query_filter(nhs_number=nhs_number, uploader=uploader) + ) + response = self.dynamo_service.query_table_with_paginator( table_name=self.table_name, - search_key="Custodian", - search_condition=ods_code, index_name="CustodianIndex", - limit=limit, + key="Custodian", + condition=ods_code, + filter_expression=filter_expression, + expression_attribute_names=condition_attribute_names, + expression_attribute_values=condition_attribute_values, start_key=start_key, - query_filter=filter_expression, + limit=limit, ) references = self._validate_review_references(response["Items"]) - - last_evaluated_key = response.get("LastEvaluatedKey", None) + last_evaluated_key = response.get("NextToken") return references, last_evaluated_key - except ClientError as e: logger.error(e) raise DocumentReviewException("Failed to query document reviews") - def query_docs_pending_review_with_paginator( - self, - ods_code: str, - limit: int = DEFAULT_QUERY_LIMIT, - start_key: str | None = None, - nhs_number: str | None = None, - uploader: str | None = None, - ) -> tuple[list[DocumentUploadReviewReference], dict | None]: - logger.info(f"Getting review document references for custodian: {ods_code}") - - filter_expression, condition_attribute_names, condition_attribute_values = ( - self.build_paginator_query_filter(nhs_number=nhs_number, uploader=uploader) - ) - response = self.dynamo_service.query_table_with_paginator( - table_name=self.table_name, - index_name="CustodianIndex", - key="Custodian", - condition=ods_code, - filter_expression=filter_expression, - expression_attribute_names=condition_attribute_names, - expression_attribute_values=condition_attribute_values, - start_key=start_key, - limit=limit, - ) - - references = self._validate_review_references(response["Items"]) - last_evaluated_key = response.get("NextToken") - - return references, last_evaluated_key - def _validate_review_references( self, items: list[dict] ) -> list[DocumentUploadReviewReference]: diff --git a/lambdas/tests/unit/services/test_document_upload_review_service.py b/lambdas/tests/unit/services/test_document_upload_review_service.py index cdc7af2df..20f22ae91 100644 --- a/lambdas/tests/unit/services/test_document_upload_review_service.py +++ b/lambdas/tests/unit/services/test_document_upload_review_service.py @@ -58,11 +58,6 @@ def mock_review_update(): return review_update -@pytest.fixture -def mock_paginator(set_env, mocker): - mocker.patch("") - - def test_table_name(mock_service): """Test that table_name property returns correct environment variable.""" @@ -465,30 +460,7 @@ def test_build_filter_handles_both_nhs_number_and_uploader(mock_service): assert actual == expected -def test_query_review_documents_queries_dynamodb_with_filter_expression_nhs_number_passed( - mock_service, mocker -): - mock_nhs_number_filter_builder = mocker.patch.object( - mock_service, "build_review_query_filter" - ) - mock_nhs_number_filter_builder.return_value = Attr("NhsNumber").eq(TEST_NHS_NUMBER) - - mock_service.query_docs_pending_review_by_custodian_with_limit( - ods_code=TEST_ODS_CODE, nhs_number=TEST_NHS_NUMBER - ) - mock_nhs_number_filter_builder.assert_called_with( - nhs_number=TEST_NHS_NUMBER, uploader=None - ) - mock_service.dynamo_service.query_table_single.assert_called_with( - table_name=MOCK_DOCUMENT_REVIEW_TABLE, - search_key="Custodian", - search_condition=TEST_ODS_CODE, - index_name="CustodianIndex", - limit=TEST_QUERY_LIMIT, - start_key=None, - query_filter=mock_nhs_number_filter_builder.return_value, - ) def test_query_review_documents_queries_dynamodb_with_filter_expression_uploader_passed( @@ -675,3 +647,29 @@ def test_query_docs_pending_review_with_paginator(mock_service): ) assert actual == expected + + +def test_query_review_documents_queries_dynamodb_with_filter_expression_nhs_number_passed( + mock_service, mocker +): + mock_nhs_number_filter_builder = mocker.patch.object( + mock_service, "build_review_query_filter" + ) + mock_nhs_number_filter_builder.return_value = Attr("NhsNumber").eq(TEST_NHS_NUMBER) + + mock_service.query_docs_pending_review_by_custodian_with_limit( + ods_code=TEST_ODS_CODE, nhs_number=TEST_NHS_NUMBER + ) + + mock_nhs_number_filter_builder.assert_called_with( + nhs_number=TEST_NHS_NUMBER, uploader=None + ) + mock_service.dynamo_service.query_table_single.assert_called_with( + table_name=MOCK_DOCUMENT_REVIEW_TABLE, + search_key="Custodian", + search_condition=TEST_ODS_CODE, + index_name="CustodianIndex", + limit=TEST_QUERY_LIMIT, + start_key=None, + query_filter=mock_nhs_number_filter_builder.return_value, + ) From 1016a68db1c07f3152d18ecdf065178ae788ff89 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Fri, 19 Dec 2025 11:29:41 +0000 Subject: [PATCH 18/21] [PRMP-1079] remove unnecessary method --- .../test_document_upload_review_service.py | 81 +------------------ 1 file changed, 1 insertion(+), 80 deletions(-) diff --git a/lambdas/tests/unit/services/test_document_upload_review_service.py b/lambdas/tests/unit/services/test_document_upload_review_service.py index 20f22ae91..2f8ddd9ff 100644 --- a/lambdas/tests/unit/services/test_document_upload_review_service.py +++ b/lambdas/tests/unit/services/test_document_upload_review_service.py @@ -460,59 +460,6 @@ def test_build_filter_handles_both_nhs_number_and_uploader(mock_service): assert actual == expected - - - -def test_query_review_documents_queries_dynamodb_with_filter_expression_uploader_passed( - mock_service, mocker -): - mock_uploader_filter_builder = mocker.patch.object( - mock_service, "build_review_query_filter" - ) - mock_uploader_filter_builder.return_value = Attr("Author").eq(NEW_ODS_CODE) - mock_service.query_docs_pending_review_by_custodian_with_limit( - ods_code=TEST_ODS_CODE, uploader=NEW_ODS_CODE - ) - mock_uploader_filter_builder.assert_called_with( - nhs_number=None, uploader=NEW_ODS_CODE - ) - mock_service.dynamo_service.query_table_single.assert_called_with( - table_name=MOCK_DOCUMENT_REVIEW_TABLE, - search_key="Custodian", - search_condition=TEST_ODS_CODE, - index_name="CustodianIndex", - limit=TEST_QUERY_LIMIT, - start_key=None, - query_filter=mock_uploader_filter_builder.return_value, - ) - - -def test_query_review_documents_by_custodian_handles_filtering_by_nhs_number_and_uploader( - mock_service, mocker -): - mock_uploader_filter_builder = mocker.patch.object( - mock_service, "build_review_query_filter" - ) - mock_uploader_filter_builder.return_value = Attr("Author").eq(NEW_ODS_CODE) & Attr( - "NhsNumber" - ).eq(TEST_NHS_NUMBER) - mock_service.query_docs_pending_review_by_custodian_with_limit( - ods_code=TEST_ODS_CODE, uploader=NEW_ODS_CODE, nhs_number=TEST_NHS_NUMBER - ) - mock_uploader_filter_builder.assert_called_with( - nhs_number=TEST_NHS_NUMBER, uploader=NEW_ODS_CODE - ) - mock_service.dynamo_service.query_table_single.assert_called_with( - table_name=MOCK_DOCUMENT_REVIEW_TABLE, - search_key="Custodian", - search_condition=TEST_ODS_CODE, - index_name="CustodianIndex", - limit=TEST_QUERY_LIMIT, - start_key=None, - query_filter=mock_uploader_filter_builder.return_value, - ) - - def test_get_document_returns_review_document(mock_service): mock_service.dynamo_service.get_item.return_value = { "Item": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][0] @@ -646,30 +593,4 @@ def test_query_docs_pending_review_with_paginator(mock_service): start_key=None, ) - assert actual == expected - - -def test_query_review_documents_queries_dynamodb_with_filter_expression_nhs_number_passed( - mock_service, mocker -): - mock_nhs_number_filter_builder = mocker.patch.object( - mock_service, "build_review_query_filter" - ) - mock_nhs_number_filter_builder.return_value = Attr("NhsNumber").eq(TEST_NHS_NUMBER) - - mock_service.query_docs_pending_review_by_custodian_with_limit( - ods_code=TEST_ODS_CODE, nhs_number=TEST_NHS_NUMBER - ) - - mock_nhs_number_filter_builder.assert_called_with( - nhs_number=TEST_NHS_NUMBER, uploader=None - ) - mock_service.dynamo_service.query_table_single.assert_called_with( - table_name=MOCK_DOCUMENT_REVIEW_TABLE, - search_key="Custodian", - search_condition=TEST_ODS_CODE, - index_name="CustodianIndex", - limit=TEST_QUERY_LIMIT, - start_key=None, - query_filter=mock_nhs_number_filter_builder.return_value, - ) + assert actual == expected \ No newline at end of file From 0fa9a8c510de4f5df0194d5e68e2b8e7f22c3db2 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Fri, 19 Dec 2025 13:54:48 +0000 Subject: [PATCH 19/21] [PRMP-1079] address SonarCloud issue --- lambdas/services/base/dynamo_service.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lambdas/services/base/dynamo_service.py b/lambdas/services/base/dynamo_service.py index 5e7cd11fc..f0d2c9bf1 100644 --- a/lambdas/services/base/dynamo_service.py +++ b/lambdas/services/base/dynamo_service.py @@ -441,7 +441,7 @@ def query_table_with_paginator( condition: str, filter_expression: str | None = None, expression_attribute_names: str | None = None, - expression_attribute_values: dict = {}, + expression_attribute_values: dict | None = None, limit: int = 20, page_size: int = 1, start_key: str | None = None, @@ -459,6 +459,9 @@ def query_table_with_paginator( }, } + if expression_attribute_values is None: + expression_attribute_values = {} + expression_attribute_values[":i"] = condition if filter_expression: From 41cbceeb51538ba8e57e253569db7df3715d3da5 Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Fri, 19 Dec 2025 15:05:43 +0000 Subject: [PATCH 20/21] [PRMP-1079] move method into parent class --- lambdas/services/document_service.py | 40 +++++++++++++++++++ .../document_upload_review_service.py | 40 ++++++++----------- .../test_document_upload_review_service.py | 1 + 3 files changed, 57 insertions(+), 24 deletions(-) diff --git a/lambdas/services/document_service.py b/lambdas/services/document_service.py index daa701ac1..272f6f44f 100644 --- a/lambdas/services/document_service.py +++ b/lambdas/services/document_service.py @@ -298,3 +298,43 @@ def create_dynamo_entry( except (ValidationError, ClientError) as e: logger.error(e) raise e + + def query_table_with_paginator( + self, + index_name: str, + search_key: str, + search_condition: str, + table_name: str | None = None, + filter_expression: str | None = None, + expression_attribute_names: dict | None = None, + expression_attribute_values: dict | None = None, + limit: int | None = None, + page_size: int = 1, + start_key: str | None = None, + model_class: BaseModel | None = None, + ) -> tuple[list[BaseModel], str | None]: + + try: + table_name = table_name or self.table_name + model_class = model_class or self.model_class + + response = self.dynamo_service.query_table_with_paginator( + table_name=table_name, + index_name=index_name, + key=search_key, + condition=search_condition, + filter_expression=filter_expression, + expression_attribute_names=expression_attribute_names, + expression_attribute_values=expression_attribute_values, + limit=limit, + page_size=page_size, + start_key=start_key + ) + + references=[model_class.model_validate(item) for item in response["Items"]] + + return references, response.get("NextToken") + + except (ValidationError, ClientError) as e: + logger.error(e) + raise e \ No newline at end of file diff --git a/lambdas/services/document_upload_review_service.py b/lambdas/services/document_upload_review_service.py index 8162edfe6..7d9e47492 100644 --- a/lambdas/services/document_upload_review_service.py +++ b/lambdas/services/document_upload_review_service.py @@ -50,33 +50,25 @@ def query_docs_pending_review_with_paginator( start_key: str | None = None, nhs_number: str | None = None, uploader: str | None = None, - ) -> tuple[list[DocumentUploadReviewReference], dict | None]: + ) -> tuple[list[DocumentUploadReviewReference], str | None]: - try: - logger.info(f"Getting review document references for custodian: {ods_code}") - - filter_expression, condition_attribute_names, condition_attribute_values = ( - self.build_paginator_query_filter(nhs_number=nhs_number, uploader=uploader) - ) - response = self.dynamo_service.query_table_with_paginator( - table_name=self.table_name, - index_name="CustodianIndex", - key="Custodian", - condition=ods_code, - filter_expression=filter_expression, - expression_attribute_names=condition_attribute_names, - expression_attribute_values=condition_attribute_values, - start_key=start_key, - limit=limit, - ) + logger.info(f"Getting review document references for custodian: {ods_code}") - references = self._validate_review_references(response["Items"]) - last_evaluated_key = response.get("NextToken") + filter_expression, condition_attribute_names, condition_attribute_values = ( + self.build_paginator_query_filter(nhs_number=nhs_number, uploader=uploader) + ) + references, last_evaluated_key = self.query_table_with_paginator( + index_name="CustodianIndex", + search_key="Custodian", + search_condition=ods_code, + filter_expression=filter_expression, + expression_attribute_names=condition_attribute_names, + expression_attribute_values=condition_attribute_values, + limit=limit, + start_key=start_key, + ) - return references, last_evaluated_key - except ClientError as e: - logger.error(e) - raise DocumentReviewException("Failed to query document reviews") + return references, last_evaluated_key def _validate_review_references( self, items: list[dict] diff --git a/lambdas/tests/unit/services/test_document_upload_review_service.py b/lambdas/tests/unit/services/test_document_upload_review_service.py index 9bd3671f0..f14f7a733 100644 --- a/lambdas/tests/unit/services/test_document_upload_review_service.py +++ b/lambdas/tests/unit/services/test_document_upload_review_service.py @@ -574,6 +574,7 @@ def test_query_docs_pending_review_with_paginator(mock_service): expression_attribute_values=condition_attribute_values, limit=50, start_key=None, + page_size=1 ) assert actual == expected \ No newline at end of file From 3effa0443c7cadc017352b2bf84ff99ce6ec741c Mon Sep 17 00:00:00 2001 From: steph-torres-nhs <173282814+steph-torres-nhs@users.noreply.github.com> Date: Thu, 8 Jan 2026 12:34:03 +0000 Subject: [PATCH 21/21] [PRMP-1079] format --- lambdas/services/base/dynamo_service.py | 2 +- lambdas/services/document_service.py | 8 ++-- .../document_upload_review_service.py | 15 +++---- .../unit/services/base/test_dynamo_service.py | 33 +++++++++++---- .../test_document_upload_review_service.py | 9 ++-- lambdas/utils/dynamo_utils.py | 42 +++++++++++++------ 6 files changed, 69 insertions(+), 40 deletions(-) diff --git a/lambdas/services/base/dynamo_service.py b/lambdas/services/base/dynamo_service.py index f0d2c9bf1..7f8584220 100644 --- a/lambdas/services/base/dynamo_service.py +++ b/lambdas/services/base/dynamo_service.py @@ -199,7 +199,7 @@ def update_item( table_name: str, key_pair: dict[str, str], updated_fields: dict, - condition_expression: str | ConditionBase| None = None, + condition_expression: str | ConditionBase | None = None, expression_attribute_values: dict | None = None, ): table = self.get_table(table_name) diff --git a/lambdas/services/document_service.py b/lambdas/services/document_service.py index 597f7d353..7e5fabd60 100644 --- a/lambdas/services/document_service.py +++ b/lambdas/services/document_service.py @@ -324,13 +324,15 @@ def query_table_with_paginator( expression_attribute_values=expression_attribute_values, limit=limit, page_size=page_size, - start_key=start_key + start_key=start_key, ) - references=[model_class.model_validate(item) for item in response["Items"]] + references = [ + model_class.model_validate(item) for item in response["Items"] + ] return references, response.get("NextToken") except (ValidationError, ClientError) as e: logger.error(e) - raise e \ No newline at end of file + raise e diff --git a/lambdas/services/document_upload_review_service.py b/lambdas/services/document_upload_review_service.py index 2a6ab5995..9615d1258 100644 --- a/lambdas/services/document_upload_review_service.py +++ b/lambdas/services/document_upload_review_service.py @@ -5,7 +5,7 @@ from botocore.exceptions import ClientError from enums.document_review_status import DocumentReviewStatus from enums.dynamo_filter import AttributeOperator -from enums.lambda_error import LambdaError, ErrorMessage +from enums.lambda_error import ErrorMessage from enums.metadata_field_names import DocumentReferenceMetadataFields from models.document_reference import S3_PREFIX from models.document_review import DocumentUploadReviewReference @@ -14,10 +14,7 @@ from utils.audit_logging_setup import LoggingService from utils.aws_transient_error_check import is_transient_error from utils.dynamo_query_filter_builder import DynamoQueryFilterBuilder -from utils.dynamo_utils import ( - build_mixed_condition_expression, - build_transaction_item, -) +from utils.dynamo_utils import build_mixed_condition_expression, build_transaction_item from utils.exceptions import DocumentReviewException logger = LoggingService(__name__) @@ -58,7 +55,9 @@ def query_docs_pending_review_with_paginator( logger.info(f"Getting review document references for custodian: {ods_code}") filter_expression, condition_attribute_names, condition_attribute_values = ( - self.build_paginator_query_filter(nhs_number=nhs_number, uploader=uploader) + self.build_paginator_query_filter( + nhs_number=nhs_number, uploader=uploader + ) ) references, last_evaluated_key = self.query_table_with_paginator( index_name="CustodianIndex", @@ -88,9 +87,7 @@ def _validate_review_references( return review_references except ValidationError as e: logger.error(e) - raise DocumentReviewException( - ErrorMessage.FAILED_TO_VALIDATE.value - ) + raise DocumentReviewException(ErrorMessage.FAILED_TO_VALIDATE.value) def build_paginator_query_filter( self, nhs_number: str | None = None, uploader: str | None = None diff --git a/lambdas/tests/unit/services/base/test_dynamo_service.py b/lambdas/tests/unit/services/base/test_dynamo_service.py index bb4cf52f8..6d66c6673 100755 --- a/lambdas/tests/unit/services/base/test_dynamo_service.py +++ b/lambdas/tests/unit/services/base/test_dynamo_service.py @@ -1,5 +1,5 @@ import copy -from unittest.mock import call, MagicMock +from unittest.mock import MagicMock, call import pytest from boto3.dynamodb.conditions import And, Attr, Equals, Key @@ -9,7 +9,6 @@ from services.base.dynamo_service import DynamoDBService from tests.unit.conftest import MOCK_CLIENT_ERROR, MOCK_TABLE_NAME, TEST_NHS_NUMBER from tests.unit.helpers.data.dynamo.dynamo_responses import MOCK_SEARCH_RESPONSE -from tests.unit.helpers.data.search_document_review.dynamo_response import MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE, MOCK_DOCUMENT_REVIEW_PAGINATOR_RESPONSE from tests.unit.helpers.data.dynamo.dynamo_scan_response import ( EXPECTED_ITEMS_FOR_PAGINATED_RESULTS, MOCK_PAGINATED_RESPONSE_1, @@ -18,8 +17,15 @@ MOCK_RESPONSE, MOCK_RESPONSE_WITH_LAST_KEY, ) -from utils.dynamo_utils import build_mixed_condition_expression, serialize_dict_to_dynamodb_object +from tests.unit.helpers.data.search_document_review.dynamo_response import ( + MOCK_DOCUMENT_REVIEW_PAGINATOR_RESPONSE, + MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE, +) from utils.dynamo_query_filter_builder import DynamoQueryFilterBuilder +from utils.dynamo_utils import ( + build_mixed_condition_expression, + serialize_dict_to_dynamodb_object, +) from utils.exceptions import DynamoServiceException @@ -1205,9 +1211,14 @@ def test_build_key_condition_non_matching_list_lengths( def test_query_table_using_paginator(mock_service): mock_paginator = mock_service.client.get_paginator.return_value = MagicMock() - mock_paginator.paginate.return_value.build_full_result.return_value = MOCK_DOCUMENT_REVIEW_PAGINATOR_RESPONSE + mock_paginator.paginate.return_value.build_full_result.return_value = ( + MOCK_DOCUMENT_REVIEW_PAGINATOR_RESPONSE + ) - expected = {"Items": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"], "NextToken": MOCK_DOCUMENT_REVIEW_PAGINATOR_RESPONSE["NextToken"]} + expected = { + "Items": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"], + "NextToken": MOCK_DOCUMENT_REVIEW_PAGINATOR_RESPONSE["NextToken"], + } actual = mock_service.query_table_with_paginator( table_name=MOCK_TABLE_NAME, @@ -1226,10 +1237,11 @@ def test_query_table_using_paginator(mock_service): assert actual == expected + def test_query_table_using_pagination_with_filter_expression(mock_service): mock_paginator = mock_service.client.get_paginator.return_value = MagicMock() - conditions=[ + conditions = [ { "field": "ReviewStatus", "operator": "=", @@ -1239,7 +1251,7 @@ def test_query_table_using_pagination_with_filter_expression(mock_service): "field": "NhsNumber", "operator": "=", "value": TEST_NHS_NUMBER, - } + }, ] filter_expression, condition_attribute_names, condition_attribute_values = ( build_mixed_condition_expression(conditions=conditions) @@ -1264,7 +1276,10 @@ def test_query_table_using_pagination_with_filter_expression(mock_service): IndexName="NhsNumberIndex", KeyConditionExpression="NhsNumber=:i", FilterExpression=filter_expression, - ExpressionAttributeValues={":i": {"S": TEST_NHS_NUMBER}, **serialized_condition_attribute_values}, + ExpressionAttributeValues={ + ":i": {"S": TEST_NHS_NUMBER}, + **serialized_condition_attribute_values, + }, ExpressionAttributeNames=condition_attribute_names, PaginationConfig={"MaxItems": 20, "PageSize": 1, "StartingToken": None}, - ) \ No newline at end of file + ) diff --git a/lambdas/tests/unit/services/test_document_upload_review_service.py b/lambdas/tests/unit/services/test_document_upload_review_service.py index 1ad54e508..8672c1412 100644 --- a/lambdas/tests/unit/services/test_document_upload_review_service.py +++ b/lambdas/tests/unit/services/test_document_upload_review_service.py @@ -5,10 +5,8 @@ from botocore.exceptions import ClientError from enums.document_review_status import DocumentReviewStatus from enums.metadata_field_names import DocumentReferenceMetadataFields -from models.document_review import ( - DocumentUploadReviewReference, -) from freezegun import freeze_time +from models.document_review import DocumentUploadReviewReference from services.document_upload_review_service import DocumentUploadReviewService from tests.unit.conftest import ( MOCK_DOCUMENT_REVIEW_BUCKET, @@ -182,6 +180,7 @@ def test_handle_standard_custodian_update_updates_document(mock_service, mocker) update_fields_name=update_fields, ) + def test_get_document_review_by_id( mock_service, mock_document_review_references, mocker ): @@ -661,7 +660,7 @@ def test_query_docs_pending_review_with_paginator(mock_service): expression_attribute_values=condition_attribute_values, limit=50, start_key=None, - page_size=1 + page_size=1, ) - assert actual == expected \ No newline at end of file + assert actual == expected diff --git a/lambdas/utils/dynamo_utils.py b/lambdas/utils/dynamo_utils.py index 8732989ef..336de2910 100644 --- a/lambdas/utils/dynamo_utils.py +++ b/lambdas/utils/dynamo_utils.py @@ -89,7 +89,9 @@ def create_expression_attribute_values(attribute_field_values: dict) -> dict: """ expression_attribute_values = {} for field_name, field_value in attribute_field_values.items(): - expression_attribute_values[f"{create_expression_value_placeholder(field_name)}"] = field_value + expression_attribute_values[ + f"{create_expression_value_placeholder(field_name)}" + ] = field_value return expression_attribute_values @@ -133,12 +135,14 @@ def filter_uploaded_docs_and_recently_uploading_docs(): filter_builder.add_condition("Uploaded", AttributeOperator.EQUAL, True) uploaded_filter_expression = filter_builder.build() - filter_builder.add_condition("Uploading", AttributeOperator.EQUAL, True).add_condition( - "LastUpdated", AttributeOperator.GREATER_OR_EQUAL, time_limit - ) + filter_builder.add_condition( + "Uploading", AttributeOperator.EQUAL, True + ).add_condition("LastUpdated", AttributeOperator.GREATER_OR_EQUAL, time_limit) uploading_filter_expression = filter_builder.build() - return delete_filter_expression & (uploaded_filter_expression | uploading_filter_expression) + return delete_filter_expression & ( + uploaded_filter_expression | uploading_filter_expression + ) def parse_dynamo_record(dynamodb_record: Dict[str, Any]) -> Dict[str, Any]: @@ -194,8 +198,12 @@ def build_mixed_condition_expression( if operator in ["attribute_exists", "attribute_not_exists"]: condition_expressions.append(f"{operator}({condition_placeholder})") else: - condition_value_placeholder = create_expression_value_placeholder(field_name, suffix) - condition_expressions.append(f"{condition_placeholder} {operator} {condition_value_placeholder}") + condition_value_placeholder = create_expression_value_placeholder( + field_name, suffix + ) + condition_expressions.append( + f"{condition_placeholder} {operator} {condition_value_placeholder}" + ) condition_attribute_values[condition_value_placeholder] = field_value condition_expression = f" {join_operator} ".join(condition_expressions) @@ -236,7 +244,9 @@ def build_general_transaction_item( action = action.capitalize() if action not in ["Put", "Update", "Delete", "Conditioncheck"]: - raise ValueError(f"Invalid action: {action}. Must be one of: Put, Update, Delete, ConditionCheck") + raise ValueError( + f"Invalid action: {action}. Must be one of: Put, Update, Delete, ConditionCheck" + ) transaction_item: dict[str, dict[str, Any]] = {action: {"TableName": table_name}} @@ -261,10 +271,14 @@ def build_general_transaction_item( transaction_item[action]["ConditionExpression"] = condition_expression if expression_attribute_names: - transaction_item[action]["ExpressionAttributeNames"] = expression_attribute_names + transaction_item[action][ + "ExpressionAttributeNames" + ] = expression_attribute_names if expression_attribute_values: - transaction_item[action]["ExpressionAttributeValues"] = expression_attribute_values + transaction_item[action][ + "ExpressionAttributeValues" + ] = expression_attribute_values return transaction_item @@ -293,8 +307,8 @@ def build_transaction_item( expression_attribute_values.update(update_attr_values) if conditions: - condition_expr, condition_attr_names, condition_attr_values = build_mixed_condition_expression( - conditions, condition_join_operator + condition_expr, condition_attr_names, condition_attr_values = ( + build_mixed_condition_expression(conditions, condition_join_operator) ) condition_expression = condition_expr expression_attribute_names.update(condition_attr_names) @@ -346,5 +360,7 @@ def resolve(self, doc_type: SnomedCode) -> str: table = self.mapping[doc_type.code] return str(table) except KeyError: - logger.error(f"SNOMED code {doc_type.code} - {doc_type.display_name} is not supported") + logger.error( + f"SNOMED code {doc_type.code} - {doc_type.display_name} is not supported" + ) raise InvalidDocTypeException(400, LambdaError.DocTypeDB)