VED-834 Refactor delete imms journey (#932)

Author: dlzhry2nhs

README.md

@@ -130,10 +130,17 @@ Steps:
     ```
 
 7. Install poetry
+
     ```
     pip install poetry
     ```
 
+8. Install pre-commit hooks. Ensure you have installed nodejs at the same version or later as per .tool-versions and
+   then, from the repo root, run:
+    ```
+    npm install
+    ```
+
 ### Setting up a virtual environment with poetry
 
 The steps below must be performed in each Lambda function folder and e2e folder to ensure the environment is correctly configured.

backend/src/controller/aws_apig_response_utils.py

@@ -4,7 +4,7 @@
 from typing import Optional
 
 
-def create_response(status_code: int, body: Optional[dict | str] = None, headers: Optional[dict] = None):
+def create_response(status_code: int, body: Optional[dict | str] = None, headers: Optional[dict] = None) -> dict:
     """Creates response body as per Lambda -> API Gateway proxy integration"""
     if body is not None:
         if isinstance(body, dict):

backend/src/controller/fhir_api_exception_handler.py

@@ -9,6 +9,7 @@
 from controller.aws_apig_response_utils import create_response
 from models.errors import (
     Code,
+    InvalidImmunizationId,
     ResourceNotFoundError,
     Severity,
     UnauthorizedError,
@@ -17,6 +18,7 @@
 )
 
 _CUSTOM_EXCEPTION_TO_STATUS_MAP: dict[Type[Exception], int] = {
+    InvalidImmunizationId: 400,
     UnauthorizedError: 403,
     UnauthorizedVaxError: 403,
     ResourceNotFoundError: 404,

backend/src/controller/fhir_controller.py

@@ -19,8 +19,8 @@
 from models.errors import (
     Code,
     IdentifierDuplicationError,
+    InvalidImmunizationId,
     ParameterException,
-    ResourceNotFoundError,
     Severity,
     UnauthorizedError,
     UnauthorizedVaxError,
@@ -96,8 +96,8 @@ def get_immunization_by_identifier(self, aws_event) -> dict:
     def get_immunization_by_id(self, aws_event: APIGatewayProxyEventV1) -> dict:
         imms_id = get_path_parameter(aws_event, "id")
 
-        if id_error := self._validate_id(imms_id):
-            return create_response(400, id_error)
+        if not self._is_valid_immunization_id(imms_id):
+            raise InvalidImmunizationId()
 
         supplier_system = get_supplier_system_header(aws_event)
 
@@ -157,10 +157,20 @@ def update_immunization(self, aws_event):
 
         supplier_system = self._identify_supplier_system(aws_event)
 
-        # Validate the imms id - start
-        if id_error := self._validate_id(imms_id):
-            return create_response(400, json.dumps(id_error))
-        # Validate the imms id - end
+        # Refactor to raise InvalidImmunizationId when working on VED-747
+        if not self._is_valid_immunization_id(imms_id):
+            return create_response(
+                400,
+                json.dumps(
+                    create_operation_outcome(
+                        resource_id=str(uuid.uuid4()),
+                        severity=Severity.error,
+                        code=Code.invalid,
+                        diagnostics="Validation errors: the provided event ID is either missing or not in the expected "
+                        "format.",
+                    )
+                ),
+            )
 
         # Validate the body of the request - start
         try:
@@ -320,31 +330,18 @@ def update_immunization(self, aws_event):
         except UnauthorizedVaxError as unauthorized:
             return create_response(403, unauthorized.to_operation_outcome())
 
-    def delete_immunization(self, aws_event):
-        try:
-            if aws_event.get("headers"):
-                imms_id = aws_event["pathParameters"]["id"]
-            else:
-                raise UnauthorizedError()
-        except UnauthorizedError as unauthorized:
-            return create_response(403, unauthorized.to_operation_outcome())
+    @fhir_api_exception_handler
+    def delete_immunization(self, aws_event: APIGatewayProxyEventV1) -> dict:
+        imms_id = get_path_parameter(aws_event, "id")
 
-        # Validate the imms id
-        if id_error := self._validate_id(imms_id):
-            return create_response(400, json.dumps(id_error))
+        if not self._is_valid_immunization_id(imms_id):
+            raise InvalidImmunizationId()
 
-        supplier_system = self._identify_supplier_system(aws_event)
+        supplier_system = get_supplier_system_header(aws_event)
 
-        try:
-            self.fhir_service.delete_immunization(imms_id, supplier_system)
-            return create_response(204)
+        self.fhir_service.delete_immunization(imms_id, supplier_system)
 
-        except ResourceNotFoundError as not_found:
-            return create_response(404, not_found.to_operation_outcome())
-        except UnhandledResponseError as unhandled_error:
-            return create_response(500, unhandled_error.to_operation_outcome())
-        except UnauthorizedVaxError as unauthorized:
-            return create_response(403, unauthorized.to_operation_outcome())
+        return create_response(204)
 
     def search_immunizations(self, aws_event: APIGatewayProxyEventV1) -> dict:
         try:
@@ -406,17 +403,9 @@ def search_immunizations(self, aws_event: APIGatewayProxyEventV1) -> dict:
             result_json_dict["total"] = 0
         return create_response(200, json.dumps(result_json_dict))
 
-    def _validate_id(self, _id: str) -> Optional[dict]:
-        if not re.match(self.immunization_id_pattern, _id):
-            msg = "Validation errors: the provided event ID is either missing or not in the expected format."
-            return create_operation_outcome(
-                resource_id=str(uuid.uuid4()),
-                severity=Severity.error,
-                code=Code.invalid,
-                diagnostics=msg,
-            )
-
-        return None
+    def _is_valid_immunization_id(self, immunization_id: str) -> bool:
+        """Validates if the given unique Immunization ID is valid."""
+        return False if not re.match(self.immunization_id_pattern, immunization_id) else True
 
     def _validate_identifier_system(self, _id: str, _elements: str) -> Optional[dict]:
         if not _id:

backend/src/delete_imms_handler.py

@@ -1,13 +1,9 @@
 import argparse
 import logging
 import pprint
-import uuid
 
-from constants import GENERIC_SERVER_ERROR_DIAGNOSTICS_MESSAGE
-from controller.aws_apig_response_utils import create_response
 from controller.fhir_controller import FhirController, make_controller
 from log_structure import function_info
-from models.errors import Code, Severity, create_operation_outcome
 
 logging.basicConfig(level="INFO")
 logger = logging.getLogger()
@@ -19,17 +15,7 @@ def delete_imms_handler(event, _context):
 
 
 def delete_immunization(event, controller: FhirController):
-    try:
-        return controller.delete_immunization(event)
-    except Exception:  # pylint: disable = broad-exception-caught
-        logger.exception("Unhandled exception")
-        exp_error = create_operation_outcome(
-            resource_id=str(uuid.uuid4()),
-            severity=Severity.error,
-            code=Code.server_error,
-            diagnostics=GENERIC_SERVER_ERROR_DIAGNOSTICS_MESSAGE,
-        )
-        return create_response(500, exp_error)
+    return controller.delete_immunization(event)
 
 
 if __name__ == "__main__":

backend/src/models/errors.py

@@ -127,6 +127,19 @@ def to_operation_outcome(self) -> dict:
         pass
 
 
+@dataclass
+class InvalidImmunizationId(ValidationError):
+    """Use this when the unique Immunization ID is invalid"""
+
+    def to_operation_outcome(self) -> dict:
+        return create_operation_outcome(
+            resource_id=str(uuid.uuid4()),
+            severity=Severity.error,
+            code=Code.invalid,
+            diagnostics="Validation errors: the provided event ID is either missing or not in the expected format.",
+        )
+
+
 @dataclass
 class InvalidPatientId(ValidationError):
     """Use this when NHS Number is invalid or doesn't exist"""

backend/src/repository/fhir_repository.py

@@ -320,22 +320,24 @@ def _perform_dynamo_update(
                 ReturnValues="ALL_NEW",
                 ConditionExpression=condition_expression,
             )
-            return self._handle_dynamo_response(response), updated_version
         except botocore.exceptions.ClientError as error:
             # Either resource didn't exist or it has already been deleted. See ConditionExpression in the request
             if error.response["Error"]["Code"] == "ConditionalCheckFailedException":
                 raise ResourceNotFoundError(resource_type="Immunization", resource_id=imms_id)
             else:
+                # VED-747: consider refactoring to simply re-raise the exception and handle in controller
                 raise UnhandledResponseError(
                     message=f"Unhandled error from dynamodb: {error.response['Error']['Code']}",
                     response=error.response,
                 )
 
-    def delete_immunization(self, imms_id: str, supplier_system: str) -> dict:
+        return json.loads(response["Attributes"]["Resource"]), updated_version
+
+    def delete_immunization(self, imms_id: str, supplier_system: str) -> None:
         now_timestamp = int(time.time())
 
         try:
-            response = self.table.update_item(
+            self.table.update_item(
                 Key={"PK": _make_immunization_pk(imms_id)},
                 UpdateExpression=(
                     "SET DeletedAt = :timestamp, Operation = :operation, SupplierSystem = :supplier_system"
@@ -345,22 +347,16 @@ def delete_immunization(self, imms_id: str, supplier_system: str) -> dict:
                     ":operation": "DELETE",
                     ":supplier_system": supplier_system,
                 },
-                ReturnValues="ALL_NEW",
                 ConditionExpression=(
                     Attr("PK").eq(_make_immunization_pk(imms_id))
                     & (Attr("DeletedAt").not_exists() | Attr("DeletedAt").eq("reinstated"))
                 ),
             )
-
-            return self._handle_dynamo_response(response)
         except botocore.exceptions.ClientError as error:
             if error.response["Error"]["Code"] == "ConditionalCheckFailedException":
                 raise ResourceNotFoundError(resource_type="Immunization", resource_id=imms_id)
             else:
-                raise UnhandledResponseError(
-                    message=f"Unhandled error from dynamodb: {error.response['Error']['Code']}",
-                    response=error.response,
-                )
+                raise error
 
     def find_immunizations(self, patient_identifier: str, vaccine_types: set):
         """it should find all of the specified patient's Immunization events for all of the specified vaccine_types"""
@@ -414,13 +410,6 @@ def get_all_items(self, condition, is_not_deleted):
 
         return all_items
 
-    @staticmethod
-    def _handle_dynamo_response(response):
-        if response["ResponseMetadata"]["HTTPStatusCode"] == 200:
-            return json.loads(response["Attributes"]["Resource"])
-        else:
-            raise UnhandledResponseError(message="Non-200 response from dynamodb", response=response)
-
     @staticmethod
     def _vaccine_type(patientsk) -> str:
         parsed = [str.strip(str.lower(s)) for s in patientsk.split("#")]

backend/src/service/fhir_service.py

@@ -245,11 +245,10 @@ def update_reinstated_immunization(
 
         return UpdateOutcome.UPDATE, Immunization.parse_obj(imms), updated_version
 
-    def delete_immunization(self, imms_id: str, supplier_system: str) -> Immunization:
+    def delete_immunization(self, imms_id: str, supplier_system: str) -> None:
         """
-        Delete an Immunization if it exits and return the ID back if successful.
-        Exception will be raised if resource does not exist. Multiple calls to this method won't change
-        the record in the database.
+        Delete an Immunization if it exists and return the ID back if successful. An exception will be raised if the
+        resource does not exist.
         """
         existing_immunisation, _ = self.immunization_repo.get_immunization_and_version_by_id(imms_id)
 
@@ -261,8 +260,7 @@ def delete_immunization(self, imms_id: str, supplier_system: str) -> Immunizatio
         if not self.authoriser.authorise(supplier_system, ApiOperationCode.DELETE, {vaccination_type}):
             raise UnauthorizedVaxError()
 
-        imms = self.immunization_repo.delete_immunization(imms_id, supplier_system)
-        return Immunization.parse_obj(imms)
+        self.immunization_repo.delete_immunization(imms_id, supplier_system)
 
     @staticmethod
     def is_valid_date_from(immunization: dict, date_from: Union[datetime.date, None]):