Resolve PR comments

Author: dlzhry2nhs

backend/src/controller/aws_apig_event_utils.py

@@ -10,8 +10,7 @@
 
 def get_path_parameter(event: APIGatewayProxyEventV1, param_name: str) -> str:
     return dict_utils.get_field(
-        dict(event),
-        "pathParameters",
+        event["pathParameters"],
         param_name,
         default=""
     )

backend/src/controller/fhir_controller.py

@@ -94,7 +94,7 @@ def get_immunization_by_id(self, aws_event: APIGatewayProxyEventV1) -> dict:
 
         supplier_system = get_supplier_system_header(aws_event)
 
-        resource, version = self.fhir_service.get_immunization_by_id(imms_id, supplier_system)
+        resource, version = self.fhir_service.get_immunization_and_version_by_id(imms_id, supplier_system)
 
         return create_response(200, resource.json(), {E_TAG_HEADER_NAME: version})
 

backend/src/repository/fhir_repository.py

@@ -102,7 +102,7 @@ def get_immunization_by_identifier(self, identifier_pk: str) -> tuple[Optional[d
         else:
             return None, None
 
-    def get_immunization_by_id(self, imms_id: str) -> tuple[Optional[dict], Optional[str]]:
+    def get_immunization_and_version_by_id(self, imms_id: str) -> tuple[Optional[dict], Optional[str]]:
         response = self.table.get_item(Key={"PK": _make_immunization_pk(imms_id)})
         item = response.get("Item")
 

backend/src/service/fhir_service.py

@@ -83,11 +83,11 @@ def get_immunization_by_identifier(
         imms_resp['resource'] = filtered_resource
         return form_json(imms_resp, element, identifier, base_url)
 
-    def get_immunization_by_id(self, imms_id: str, supplier_system: str) -> tuple[Immunization, str]:
+    def get_immunization_and_version_by_id(self, imms_id: str, supplier_system: str) -> tuple[Immunization, str]:
         """
         Get an Immunization by its ID. Returns the immunization entity and version number.
         """
-        resource, version = self.immunization_repo.get_immunization_by_id(imms_id)
+        resource, version = self.immunization_repo.get_immunization_and_version_by_id(imms_id)
 
         if resource is None:
             raise ResourceNotFoundError(resource_type="Immunization", resource_id=imms_id)
@@ -230,7 +230,7 @@ def delete_immunization(self, imms_id: str, supplier_system: str) -> Immunizatio
         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)
+        existing_immunisation, _ = self.immunization_repo.get_immunization_and_version_by_id(imms_id)
 
         if not existing_immunisation:
             raise ResourceNotFoundError(resource_type="Immunization", resource_id=imms_id)

backend/tests/controller/test_fhir_api_exception_handler.py

@@ -25,23 +25,19 @@ def dummy_func():
 
     def test_exception_handler_handles_custom_exception_and_returns_fhir_response(self):
         """Test that custom exceptions are handled by the wrapper and a valid response is returned to the client"""
-        test_cases = {
-            ("Test UnauthorisedError", UnauthorizedError, 403, "forbidden", "Unauthorized request"),
-            ("Test UnauthorizedVaxError", UnauthorizedVaxError, 403, "forbidden",
-             "Unauthorized request for vaccine type"),
-            ("Test ResourceNotFoundError", ResourceNotFoundError, 404, "not-found", "Immunization resource does not "
-                                                                                    "exist. ID: 123")
-        }
+        test_cases = [
+            (UnauthorizedError(), 403, "forbidden", "Unauthorized request"),
+            (UnauthorizedVaxError(), 403, "forbidden", "Unauthorized request for vaccine type"),
+            (ResourceNotFoundError(resource_type="Immunization", resource_id="123"), 404, "not-found",
+             "Immunization resource does not exist. ID: 123")
+        ]
 
-        for msg, error_type, expected_status, expected_code, expected_message in test_cases:
-            with self.subTest(msg=msg):
+        for error, expected_status, expected_code, expected_message in test_cases:
+            with self.subTest(msg=f"Test {error.__class__.__name__}"):
 
                 @fhir_api_exception_handler
                 def dummy_func():
-                    if msg == "Test ResourceNotFoundError":
-                        raise error_type(resource_type="Immunization", resource_id="123")
-
-                    raise error_type()
+                    raise error
 
                 response = dummy_func()
 

backend/tests/controller/test_fhir_controller.py

@@ -673,7 +673,7 @@ def test_get_imms_by_id(self):
         """it should return Immunization resource if it exists"""
         # Given
         imms_id = "a-id"
-        self.service.get_immunization_by_id.return_value = (Immunization.construct(), "1")
+        self.service.get_immunization_and_version_by_id.return_value = (Immunization.construct(), "1")
         lambda_event = {
             "headers": {"SupplierSystem": "test"},
             "pathParameters": {"id": imms_id},
@@ -682,7 +682,7 @@ def test_get_imms_by_id(self):
         # When
         response = self.controller.get_immunization_by_id(lambda_event)
         # Then
-        self.service.get_immunization_by_id.assert_called_once_with(imms_id, "test")
+        self.service.get_immunization_and_version_by_id.assert_called_once_with(imms_id, "test")
 
         self.assertEqual(response["statusCode"], 200)
         body = json.loads(response["body"])
@@ -701,7 +701,7 @@ def test_get_imms_by_id_returns_unauthorized_when_supplier_header_missing(self):
         # When
         response = self.controller.get_immunization_by_id(lambda_event)
         # Then
-        self.service.get_immunization_by_id.assert_not_called()
+        self.service.get_immunization_and_version_by_id.assert_not_called()
 
         self.assertEqual(response["statusCode"], 403)
         body = json.loads(response["body"])
@@ -712,7 +712,7 @@ def test_get_imms_by_id_unauthorised_vax_error(self):
         """it should return a 403 error is the service layer throws an UnauthorizedVaxError"""
         # Given
         imms_id = "a-id"
-        self.service.get_immunization_by_id.side_effect = UnauthorizedVaxError
+        self.service.get_immunization_and_version_by_id.side_effect = UnauthorizedVaxError
         lambda_event = {
             "headers": {"SupplierSystem": "test"},
             "pathParameters": {"id": imms_id},
@@ -730,7 +730,7 @@ def test_not_found(self):
         """it should return not-found OperationOutcome if it doesn't exist"""
         # Given
         imms_id = "a-non-existing-id"
-        self.service.get_immunization_by_id.side_effect = ResourceNotFoundError(
+        self.service.get_immunization_and_version_by_id.side_effect = ResourceNotFoundError(
             resource_type="Immunization",
             resource_id=imms_id
         )
@@ -743,7 +743,7 @@ def test_not_found(self):
         response = self.controller.get_immunization_by_id(lambda_event)
 
         # Then
-        self.service.get_immunization_by_id.assert_called_once_with(imms_id, "test")
+        self.service.get_immunization_and_version_by_id.assert_called_once_with(imms_id, "test")
 
         self.assertEqual(response["statusCode"], 404)
         body = json.loads(response["body"])
@@ -756,7 +756,7 @@ def test_validate_imms_id(self):
 
         response = self.controller.get_immunization_by_id(invalid_id)
 
-        self.assertEqual(self.service.get_immunization_by_id.call_count, 0)
+        self.assertEqual(self.service.get_immunization_and_version_by_id.call_count, 0)
         self.assertEqual(response["statusCode"], 400)
         outcome = json.loads(response["body"])
         self.assertEqual(outcome["resourceType"], "OperationOutcome")
@@ -827,7 +827,7 @@ def test_malformed_resource(self):
         }
 
         response = self.controller.create_immunization(aws_event)
-        self.assertEqual(self.service.get_immunization_by_id.call_count, 0)
+        self.assertEqual(self.service.get_immunization_and_version_by_id.call_count, 0)
         self.assertEqual(response["statusCode"], 400)
         outcome = json.loads(response["body"])
         self.assertEqual(outcome["resourceType"], "OperationOutcome")
@@ -1333,7 +1333,7 @@ def test_validate_imms_id(self):
 
         response = self.controller.delete_immunization(invalid_id)
 
-        self.assertEqual(self.service.get_immunization_by_id.call_count, 0)
+        self.assertEqual(self.service.get_immunization_and_version_by_id.call_count, 0)
         self.assertEqual(response["statusCode"], 400)
         outcome = json.loads(response["body"])
         self.assertEqual(outcome["resourceType"], "OperationOutcome")

backend/tests/repository/test_fhir_repository.py

@@ -111,7 +111,7 @@ def test_get_immunization_by_id(self):
                 }
             }
         )
-        immunisation, version = self.repository.get_immunization_by_id(imms_id)
+        immunisation, version = self.repository.get_immunization_and_version_by_id(imms_id)
 
         # Validate the results
         self.assertDictEqual(expected_resource, immunisation)
@@ -123,7 +123,7 @@ def test_immunization_not_found(self):
         imms_id = "non-existent-id"
         self.table.get_item = MagicMock(return_value={})
 
-        imms, version = self.repository.get_immunization_by_id(imms_id)
+        imms, version = self.repository.get_immunization_and_version_by_id(imms_id)
         self.assertIsNone(imms)
         self.assertIsNone(version)
 
@@ -466,7 +466,7 @@ def test_get_deleted_immunization(self):
         imms_id = "a-deleted-id"
         self.table.get_item = MagicMock(return_value={"Item": {"Resource": "{}", "DeletedAt": time.time()}})
 
-        imms, version = self.repository.get_immunization_by_id(imms_id)
+        imms, version = self.repository.get_immunization_and_version_by_id(imms_id)
         self.assertIsNone(imms)
         self.assertIsNone(version)
 

backend/tests/service/test_fhir_service.py

@@ -205,29 +205,29 @@ def test_get_immunization_by_id(self):
         imms_id = "an-id"
         self.mock_redis_client.hget.return_value = "COVID-19"
         self.authoriser.authorise.return_value = True
-        self.imms_repo.get_immunization_by_id.return_value = (create_covid_19_immunization(imms_id).dict(), "")
+        self.imms_repo.get_immunization_and_version_by_id.return_value = (create_covid_19_immunization(imms_id).dict(), "")
 
         # When
-        immunisation, version = self.fhir_service.get_immunization_by_id(imms_id, "Test Supplier")
+        immunisation, version = self.fhir_service.get_immunization_and_version_by_id(imms_id, "Test Supplier")
 
         # Then
         self.authoriser.authorise.assert_called_once_with("Test Supplier", ApiOperationCode.READ, {"COVID-19"})
-        self.imms_repo.get_immunization_by_id.assert_called_once_with(imms_id)
+        self.imms_repo.get_immunization_and_version_by_id.assert_called_once_with(imms_id)
 
         self.assertEqual(immunisation.id, imms_id)
         self.assertEqual(version, "")
 
     def test_immunization_not_found(self):
         """it should return None if Immunization doesn't exist"""
         imms_id = "non-existent-id"
-        self.imms_repo.get_immunization_by_id.return_value = None, None
+        self.imms_repo.get_immunization_and_version_by_id.return_value = None, None
 
         # When
         with self.assertRaises(ResourceNotFoundError) as error:
-            self.fhir_service.get_immunization_by_id(imms_id, "Test Supplier")
+            self.fhir_service.get_immunization_and_version_by_id(imms_id, "Test Supplier")
 
         # Then
-        self.imms_repo.get_immunization_by_id.assert_called_once_with(imms_id)
+        self.imms_repo.get_immunization_and_version_by_id.assert_called_once_with(imms_id)
         self.assertEqual("Immunization resource does not exist. ID: non-existent-id", str(error.exception))
 
     def test_get_immunization_by_id_patient_not_restricted(self):
@@ -240,17 +240,17 @@ def test_get_immunization_by_id_patient_not_restricted(self):
         immunization_data = load_json_data("completed_covid19_immunization_event.json")
         self.mock_redis_client.hget.return_value = "COVID-19"
         self.authoriser.authorise.return_value = True
-        self.imms_repo.get_immunization_by_id.return_value = (immunization_data, "2")
+        self.imms_repo.get_immunization_and_version_by_id.return_value = (immunization_data, "2")
 
         expected_imms = load_json_data("completed_covid19_immunization_event_for_read.json")
         expected_output = Immunization.parse_obj(expected_imms)
 
         # When
-        actual_output, version = self.fhir_service.get_immunization_by_id(imms_id, "Test Supplier")
+        actual_output, version = self.fhir_service.get_immunization_and_version_by_id(imms_id, "Test Supplier")
 
         # Then
         self.authoriser.authorise.assert_called_once_with("Test Supplier", ApiOperationCode.READ, {"COVID-19"})
-        self.imms_repo.get_immunization_by_id.assert_called_once_with(imms_id)
+        self.imms_repo.get_immunization_and_version_by_id.assert_called_once_with(imms_id)
         self.assertEqual(actual_output, expected_output)
         self.assertEqual(version, "2")
 
@@ -284,15 +284,15 @@ def test_unauthorised_error_raised_when_user_lacks_permissions(self):
         imms_id = "an-id"
         self.mock_redis_client.hget.return_value = "COVID-19"
         self.authoriser.authorise.return_value = False
-        self.imms_repo.get_immunization_by_id.return_value = (create_covid_19_immunization(imms_id).dict(), 1)
+        self.imms_repo.get_immunization_and_version_by_id.return_value = (create_covid_19_immunization(imms_id).dict(), 1)
 
         with self.assertRaises(UnauthorizedVaxError):
             # When
-            self.fhir_service.get_immunization_by_id(imms_id, "Test Supplier")
+            self.fhir_service.get_immunization_and_version_by_id(imms_id, "Test Supplier")
 
         # Then
         self.authoriser.authorise.assert_called_once_with("Test Supplier", ApiOperationCode.READ, {"COVID-19"})
-        self.imms_repo.get_immunization_by_id.assert_called_once_with(imms_id)
+        self.imms_repo.get_immunization_and_version_by_id.assert_called_once_with(imms_id)
 
 
 def test_post_validation_failed_get_invalid_target_disease(self):
@@ -729,43 +729,43 @@ def test_delete_immunization(self):
         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 = (imms, "1")
+        self.imms_repo.get_immunization_and_version_by_id.return_value = (imms, "1")
         self.imms_repo.delete_immunization.return_value = imms
 
         # When
         act_imms = 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.get_immunization_and_version_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, self.TEST_IMMUNISATION_ID)
 
     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, None)
+        self.imms_repo.get_immunization_and_version_by_id.return_value = (None, None)
 
         # When
         with self.assertRaises(ResourceNotFoundError):
             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.get_immunization_and_version_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 = (imms, "1")
+        self.imms_repo.get_immunization_and_version_by_id.return_value = (imms, "1")
 
         # 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.get_immunization_and_version_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"})