Refactored delete journey

Author: dlzhry2nhs

backend/src/fhir_controller.py

@@ -374,7 +374,7 @@ def delete_immunization(self, aws_event):
             return response
 
         try:
-            self.fhir_service.delete_immunization(imms_id, imms_vax_type_perms, supplier_system)
+            self.fhir_service.delete_immunization(imms_id, supplier_system)
             return self.create_response(204)
 
         except ResourceNotFoundError as not_found:

backend/src/fhir_repository.py

@@ -332,21 +332,10 @@ def _perform_dynamo_update(
                     response=error.response,
                 )
 
-    def delete_immunization(
-            self, imms_id: str, imms_vax_type_perms: str, supplier_system: str) -> dict:
+    def delete_immunization(self, imms_id: str, supplier_system: str) -> dict:
         now_timestamp = int(time.time())
 
         try:
-            item = self.table.get_item(Key={"PK": _make_immunization_pk(imms_id)}).get("Item")
-            if not item:
-                raise ResourceNotFoundError(resource_type="Immunization", resource_id=imms_id)
-
-            if not item.get("DeletedAt") or item.get("DeletedAt") == "reinstated":
-                vaccine_type = self._vaccine_type(item["PatientSK"])
-                if not validate_permissions(imms_vax_type_perms, ApiOperationCode.DELETE, [vaccine_type]):
-                    raise UnauthorizedVaxError()
-
-            # Proceed with delete update
             response = self.table.update_item(
                 Key={"PK": _make_immunization_pk(imms_id)},
                 UpdateExpression=(

backend/src/fhir_service.py

@@ -18,7 +18,7 @@
 from authorisation.api_operation_code import ApiOperationCode
 from authorisation.authoriser import Authoriser
 from fhir_repository import ImmunizationRepository
-from models.errors import InvalidPatientId, CustomValidationError, UnauthorizedVaxError
+from models.errors import InvalidPatientId, CustomValidationError, UnauthorizedVaxError, ResourceNotFoundError
 from models.fhir_immunization import ImmunizationValidator
 from models.utils.generic_utils import nhs_number_mod11_check, get_occurrence_datetime, create_diagnostics, form_json, get_contained_patient
 from models.errors import MandatoryError
@@ -214,14 +214,24 @@ def update_reinstated_immunization(
 
         return UpdateOutcome.UPDATE, Immunization.parse_obj(imms), updated_version
 
-    def delete_immunization(self, imms_id: str, imms_vax_type_perms, supplier_system: str) -> Immunization:
+    def delete_immunization(self, imms_id: str, supplier_system: str) -> Immunization:
         """
         Delete an Immunization if it exits and return the ID back if successful.
-        Exception will be raised if resource didn't exit. Multiple calls to this method won't change
+        Exception will be raised if resource does not exist. Multiple calls to this method won't change
         the record in the database.
         """
+        existing_immunisation = self.immunization_repo.get_immunization_by_id(imms_id)
+
+        if not existing_immunisation:
+            raise ResourceNotFoundError(resource_type="Immunization", resource_id=imms_id)
+
+        vaccination_type = get_vaccine_type(existing_immunisation.get("Resource", {}))
+
+        if not self.authoriser.authorise(supplier_system, ApiOperationCode.DELETE, {vaccination_type}):
+            raise UnauthorizedVaxError()
+
         imms = self.immunization_repo.delete_immunization(
-            imms_id, imms_vax_type_perms, supplier_system
+            imms_id, supplier_system
         )
         return Immunization.parse_obj(imms)
 

backend/tests/test_fhir_controller.py

@@ -1614,7 +1614,7 @@ def test_delete_immunization(self, mock_get_supplier_permissions):
         response = self.controller.delete_immunization(lambda_event)
 
         # Then
-        self.service.delete_immunization.assert_called_once_with(imms_id, ["COVID19.CRUDS"], "Test")
+        self.service.delete_immunization.assert_called_once_with(imms_id, "Test")
 
         self.assertEqual(response["statusCode"], 204)
         self.assertTrue("body" not in response)

backend/tests/test_fhir_repository.py

@@ -481,7 +481,7 @@ def test_delete_immunization(self):
         with patch("time.time") as mock_time:
             mock_time.return_value = now_epoch
             # When
-            _id = self.repository.delete_immunization(imms_id, "COVID:delete", "Test")
+            _id = self.repository.delete_immunization(imms_id, "Test")
 
         # Then
         self.table.update_item.assert_called_once_with(
@@ -492,89 +492,17 @@ def test_delete_immunization(self):
             ConditionExpression=ANY,
         )
 
-    def test_delete_returns_old_resource(self):
-        """it should return existing Immunization when delete is successful"""
-
-        imms_id = "an-id"
-        resource = {"foo": "bar"}
-        dynamo_response = {
-            "ResponseMetadata": {"HTTPStatusCode": 200},
-            "Attributes": {"Resource": json.dumps(resource)},
-        }
-        self.table.update_item = MagicMock(return_value=dynamo_response)
-        self.table.get_item = MagicMock(
-            return_value={
-                "Item": {
-                    "Resource": json.dumps({"foo": "bar"}),
-                    "Version": 1,
-                    "PatientSK": "COVID19#2516525251",
-                }
-            }
-        )
-
-        now_epoch = 123456
-        with patch("time.time") as mock_time:
-            mock_time.return_value = now_epoch
-            # When
-
-            act_resource = self.repository.delete_immunization(imms_id, ["COVID19.CRUD"], "Test")
-
-        # Then
-        self.table.update_item.assert_called_once_with(
-            Key=ANY,
-            UpdateExpression=ANY,
-            ExpressionAttributeValues=ANY,
-            ConditionExpression=ANY,
-            ReturnValues="ALL_NEW",
-        )
-        self.assertDictEqual(act_resource, resource)
-
-    def test_unauthorised_vax_delete(self):
-        """when delete is called for a resource without proper vax permission"""
-        imms_id = "an-id"
-        self.table.get_item = MagicMock(
-            return_value={
-                "Item": {
-                    "Resource": json.dumps({"foo": "bar"}),
-                    "Version": 1,
-                    "PatientSK": "FLU#2516525251",
-                    "DeletedAt": "reinstated"
-                }
-            }
-        )
-
-        self.repository.table.update_item.return_value = {
-        "ResponseMetadata": {
-            "HTTPStatusCode": 200
-        },
-        "Attributes": {
-            "Resource": json.dumps({"id": "valid-id", "status": "deleted"})
-        }
-    }
-
-        with self.assertRaises(UnauthorizedVaxError) as e:
-            self.repository.delete_immunization(imms_id, ["COVID19.CRUD"], "Test")
-
     def test_multiple_delete_should_not_update_timestamp(self):
         """when delete is called multiple times, or when it doesn't exist, it should not update DeletedAt,
         and it should return Error"""
         imms_id = "an-id"
         error_res = {"Error": {"Code": "ConditionalCheckFailedException"}}
-        self.table.get_item = MagicMock(
-            return_value={
-                "Item": {
-                    "Resource": json.dumps({"foo": "bar"}),
-                    "Version": 1,
-                    "PatientSK": "COVID19#2516525251",
-                }
-            }
-        )
         self.table.update_item.side_effect = botocore.exceptions.ClientError(
             error_response=error_res, operation_name="an-op"
         )
 
         with self.assertRaises(ResourceNotFoundError) as e:
-            self.repository.delete_immunization(imms_id, ["COVID19.CRUD"], "Test")
+            self.repository.delete_immunization(imms_id, "Test")
 
         # Then
         self.table.update_item.assert_called_once_with(
@@ -595,20 +523,11 @@ def test_delete_throws_error_when_response_can_not_be_handled(self):
         imms_id = "an-id"
         bad_request = 400
         response = {"ResponseMetadata": {"HTTPStatusCode": bad_request}}
-        self.table.get_item = MagicMock(
-            return_value={
-                "Item": {
-                    "Resource": json.dumps({"foo": "bar"}),
-                    "Version": 1,
-                    "PatientSK": "COVID19#2516525251",
-                }
-            }
-        )
         self.table.update_item = MagicMock(return_value=response)
 
         with self.assertRaises(UnhandledResponseError) as e:
             # When
-            self.repository.delete_immunization(imms_id, ["COVID19.CRUD"], "Test")
+            self.repository.delete_immunization(imms_id, "Test")
 
         # Then
         self.assertDictEqual(e.exception.response, response)
@@ -806,7 +725,7 @@ def test_decimal_on_update(self):
         self.run_update_immunization_test(imms_id, imms, resource, updated_dose_quantity)
 
     def test_decimal_on_update_patient(self):
-        """it should update record by replacing both Immunization and Patient and dosequantity"""
+        """it should update record by replacing both Immunization and Patient and dose quantity"""
         imms_id = "an-imms-id"
         imms = create_covid_19_immunization_dict(imms_id)
         imms["doseQuantity"] = 1.590

backend/tests/test_fhir_service.py

@@ -15,7 +15,7 @@
 from authorisation.authoriser import Authoriser
 from fhir_repository import ImmunizationRepository
 from fhir_service import FhirService, UpdateOutcome, get_service_url
-from models.errors import InvalidPatientId, CustomValidationError, UnauthorizedVaxError
+from models.errors import InvalidPatientId, CustomValidationError, UnauthorizedVaxError, ResourceNotFoundError
 from models.fhir_immunization import ImmunizationValidator
 from models.utils.generic_utils import get_contained_patient
 from pydantic import ValidationError
@@ -693,10 +693,12 @@ def test_update_reinstated_immunization_with_diagnostics(self):
         self.imms_repo.update_reinstated_immunization.assert_not_called()
 
 
-class TestDeleteImmunization(unittest.TestCase):
+class TestDeleteImmunization(TestFhirServiceBase):
     """Tests for FhirService.delete_immunization"""
+    TEST_IMMUNISATION_ID = "an-id"
 
     def setUp(self):
+        super().setUp()
         self.authoriser = create_autospec(Authoriser)
         self.imms_repo = create_autospec(ImmunizationRepository)
         self.validator = create_autospec(ImmunizationValidator)
@@ -706,33 +708,48 @@ def setUp(self):
 
     def test_delete_immunization(self):
         """it should delete Immunization record"""
-        imms_id = "an-id"
-        imms = json.loads(create_covid_19_immunization(imms_id).json())
+        imms = json.loads(create_covid_19_immunization(self.TEST_IMMUNISATION_ID).json())
+        self.mock_redis_client.hget.return_value = "COVID19"
+        self.authoriser.authorise.return_value = True
+        self.imms_repo.get_immunization_by_id.return_value = {"Resource": imms}
         self.imms_repo.delete_immunization.return_value = imms
 
         # When
-        act_imms = self.fhir_service.delete_immunization(imms_id, "COVID:delete", "Test")
+        act_imms = self.fhir_service.delete_immunization(self.TEST_IMMUNISATION_ID, "Test")
 
         # Then
-        self.imms_repo.delete_immunization.assert_called_once_with(imms_id, "COVID:delete", "Test")
+        self.imms_repo.get_immunization_by_id.assert_called_once_with(self.TEST_IMMUNISATION_ID)
+        self.imms_repo.delete_immunization.assert_called_once_with(self.TEST_IMMUNISATION_ID, "Test")
+        self.authoriser.authorise.assert_called_once_with("Test", ApiOperationCode.DELETE, {"COVID19"})
         self.assertIsInstance(act_imms, Immunization)
-        self.assertEqual(act_imms.id, imms_id)
+        self.assertEqual(act_imms.id, self.TEST_IMMUNISATION_ID)
 
-    def test_delete_immunization_for_batch(self):
-        """it should delete Immunization record"""
-        imms_id = "an-id"
-        imms = json.loads(create_covid_19_immunization(imms_id).json())
-        self.imms_repo.delete_immunization.return_value = imms
+    def test_delete_immunization_throws_not_found_exception_if_does_not_exist(self):
+        """it should raise a ResourceNotFound exception if the immunisation does not exist"""
+        self.imms_repo.get_immunization_by_id.return_value = None
 
         # When
-        act_imms = self.fhir_service.delete_immunization(imms_id, None, "Test")
+        with self.assertRaises(ResourceNotFoundError):
+            self.fhir_service.delete_immunization(self.TEST_IMMUNISATION_ID, "Test")
 
         # Then
-        self.imms_repo.delete_immunization.assert_called_once_with(
-            imms_id, None, "Test"
-        )
-        self.assertIsInstance(act_imms, Immunization)
-        self.assertEqual(act_imms.id, imms_id)
+        self.imms_repo.get_immunization_by_id.assert_called_once_with(self.TEST_IMMUNISATION_ID)
+        self.imms_repo.delete_immunization.assert_not_called()
+
+    def test_delete_immunization_throws_authorisation_exception_if_does_not_have_required_permissions(self):
+        imms = json.loads(create_covid_19_immunization(self.TEST_IMMUNISATION_ID).json())
+        self.mock_redis_client.hget.return_value = "FLU"
+        self.authoriser.authorise.return_value = False
+        self.imms_repo.get_immunization_by_id.return_value = {"Resource": imms}
+
+        # When
+        with self.assertRaises(UnauthorizedVaxError):
+            self.fhir_service.delete_immunization(self.TEST_IMMUNISATION_ID, "Test")
+
+        # Then
+        self.imms_repo.get_immunization_by_id.assert_called_once_with(self.TEST_IMMUNISATION_ID)
+        self.imms_repo.delete_immunization.assert_not_called()
+        self.authoriser.authorise.assert_called_once_with("Test", ApiOperationCode.DELETE, {"FLU"})
 
 
 class TestSearchImmunizations(unittest.TestCase):