Feature/cdapi 71 jw by nhsd-jack-wainwright · Pull Request #68 · NHSDigital/clinical-data-pathology-api

nhsd-jack-wainwright · 2026-03-17T17:31:35Z

Description

Context

Type of changes

Refactoring (non-breaking change)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would change existing functionality)
Bug fix (non-breaking change which fixes an issue)

Checklist

I have followed the code style of the project
I have added tests to cover my changes
I have updated the documentation accordingly
This PR is a result of pair or mob programming
Exceptions/Exclusions to coding standards (e.g. #noqa or #NOSONAR) are included within this Pull Request.

Sensitive Information Declaration

To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.

I confirm that neither PII/PID nor sensitive data are included in this PR and the codebase changes.

nhsd-jack-wainwright

LGTM 👍. Just a few minor comments / suggestions 🙂

nhsd-jack-wainwright · 2026-04-02T08:16:19Z

infrastructure/images/mocks/Dockerfile


 COPY /resources/build/mocks ${LAMBDA_TASK_ROOT}

+# RUN mkdir -p /.aws


Should this commented out logic be removed?

removed this,I think we will have a proper way of including or grabbing credentials in the future

nhsd-jack-wainwright · 2026-04-02T08:30:32Z

mocks/src/apim_mock/auth_check.py

-        KeyConditionExpression=Key("ddb_index").eq(BRANCH_NAME),
-        FilterExpression=Attr("access_token").eq(token)
-        & Attr("expiresAt").gt(int(time())),
+def check_authenticated(token: str) -> bool:


Rather than returning a boolean here, I wonder if it would be nicer to return some sort of typed exception that's declared within the mock logic (maybe something like AuthenticatorError for example). That way we could add in an exception handler to provide the standard APIM response within the top level handler, rather than each mock endpoint needing to define this response?

This function no longer returns anything but will raise an exception when not authenticated, which gets caught by a handler exception at the lambda handler level

nhsd-jack-wainwright · 2026-04-02T08:32:06Z

mocks/src/aws_helpers/dynamo_helper.py

+    pass
+
+
+class DynamoHelper:


Very minor, but I wonder if its worth renaming this helper just to abstract away that we're using dynamo for storage? Maybe it could just be something like StorageHelper?

this have been renamed now

nhsd-jack-wainwright · 2026-04-02T08:35:15Z

mocks/src/aws_helpers/dynamo_helper.py

+
+        return cast("BaseMockItem", response.get("Item"))
+
+    def query_by_ddb_index(self, filter_expression: Any) -> list[BaseMockItem]:


As above with the class name, is it worth renaming this method to be something that's more generic? Maybe this method could just be called find for example that accepts an expression?

I wonder if it's worth always appending the TTL check here also as I think we'd always want to apply that condition as part of looking something up?

I have renamed the function and put a ttl check inside the function to be appended to any filter expression we use

nhsd-jack-wainwright · 2026-04-02T08:36:01Z

mocks/src/aws_helpers/dynamo_helper.py

+        self.branch_name = branch_name
+        self.table_resource_created = False
+
+    def _set_up_resource(self) -> None:


Could this logic not be included as part of the constructor call above? That way other methods within this class don't need to worry about making sure this method is called?

I have changed the way we create the resource now

nhsd-jack-wainwright · 2026-04-02T08:59:30Z

mocks/src/pdm_mock/handler.py

+    auth_token = pdm_routes.current_event.headers.get("Authorization", "").replace(
+        "Bearer ", ""
+    )


As this logic will be common across all authentication, I think it's worth including this logic as part of the check_authenticated method.

I have moved this logic into check_authenticated

nhsd-jack-wainwright · 2026-04-02T09:00:45Z

mocks/src/pdm_mock/logging.py

I don't think this module should be required, as I think the same logging module should be used across all mocks within the lambda. Could the existing logging module as part of the apim_mock instead be moved up a level so it's at the root of the mock? That way each mock can just depend on the same module.

the logging setup is now part of the common module

nhsd-jack-wainwright · 2026-04-02T09:03:37Z

mocks/src/pdm_mock/test_handler.py

+        }
+
+    # @patch("boto3.resource")
+    # def test_get_request_no_document_found(


I think this test should be included?

I originally thought this test was a bit redundant but its different from the other one I've written from the lambda_handler level. Ive reinstated this test

nhsd-jack-wainwright · 2026-04-02T09:05:44Z

mocks/pyproject.toml

+packages = [
+  {include = "apim_mock", from = "src"},
+  {include = "pdm_mock", from = "src"},
+  {include = "aws_helpers", from = "src"}


Maybe instead of having a aws_helpers package here we could instead have a common package which then includes the following modules:

storage - Includes the current DynamoHelper class

logging - Includes the logger configuration so it can be reused across all mocks

package has been renamed

nhsd-jack-wainwright · 2026-04-02T09:08:31Z

mocks/test_lambda_handler.py

Now that we've separated out the endpoints for individual mocks out of the top level lambda_handler I wonder if these tests can be a bit simpler? I think these tests only really need to verify the behaviour housed within the lambda_handler.py file itself, perhaps also verifying that the routers from the individual mock modules are registered?

the tests in this file have been moved to the relevant handler files

nhsd-jack-wainwright

Looks good to me 👍, just a couple extra minor comments.

nhsd-jack-wainwright · 2026-04-10T12:18:18Z

mocks/src/common/test_storage_helper.py

+
+class TestStorageHelper:
+    # @patch("boto3.resource")
+    # def test_resource_set_up_once(self, mock_boto_resource: MagicMock) -> None:


Should this commented out test be included?

nhsd-jack-wainwright · 2026-04-10T12:20:44Z

mocks/src/common/storage_helper.py

+
+        self.table = self.dynamodb.Table(self.table_name)
+
+    def get_item_by_session_id(self, session_id: str) -> BaseMockItem:


I think this method could still be implemented with a generic?

Suggested change

def get_item_by_session_id(self, session_id: str) -> BaseMockItem:

def get_item_by_session_id[T: BaseMockItem](self, session_id: str, required_type: type[T]) -> T:

...

item = response.get("Item")

if not isinstance(item, required_type):

raise ItemNotFoundException(f"Item found but was not expected type. item={item}")

return item

sonarqubecloud · 2026-04-13T10:11:22Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

github-actions · 2026-04-13T10:11:27Z

Deployment Complete

Preview with mock:
- URL: https://feature-cdapi-71-jw.dev.endpoints.pathology-laboratory-reporting.national.nhs.uk — Status
  - Smoke test result: ✅ Passed (HTTP 200)
- Proxy URL: https://internal-dev.api.service.nhs.uk/pathology-laboratory-reporting-pr-68
- Lambda function: pr-feature-cdapi-71-jw
Mock endpoints:
- URL: https://feature-cdapi-71-jw.m.dev.endpoints.pathology-laboratory-reporting.national.nhs.uk
  - Smoke test result: ✅ Passed (HTTP 200)
- Lambda function: mock-feature-cdapi-71-jw
Preview with integration:
- URL: https://feature-cdapi-71-jw.i.dev.endpoints.pathology-laboratory-reporting.national.nhs.uk — Status
  - Smoke test result: ✅ Passed (HTTP 200)
- Proxy URL: https://internal-dev.api.service.nhs.uk/pathology-laboratory-reporting-pri-68
- Lambda function: int-feature-cdapi-71-jw

nhsd-jack-wainwright force-pushed the feature/CDAPI-85 branch from 730f22c to 4093d6a Compare March 19, 2026 16:48

nhsd-jack-wainwright force-pushed the feature/CDAPI-71-JW branch from db4de43 to a19eef9 Compare March 19, 2026 17:02

nhsd-jack-wainwright force-pushed the feature/CDAPI-85 branch from 4093d6a to 8a8d338 Compare March 19, 2026 18:30

Base automatically changed from feature/CDAPI-85 to main March 19, 2026 18:46

MohammadPatelNHS force-pushed the feature/CDAPI-71-JW branch 4 times, most recently from 89ed95e to 75d0e29 Compare March 27, 2026 09:42

MohammadPatelNHS marked this pull request as ready for review March 27, 2026 09:43

MohammadPatelNHS requested a review from a team as a code owner March 27, 2026 09:43

nhsd-jack-wainwright commented Apr 2, 2026

View reviewed changes

MohammadPatelNHS force-pushed the feature/CDAPI-71-JW branch from 75d0e29 to 8c72967 Compare April 10, 2026 09:24

MohammadPatelNHS requested a review from a team as a code owner April 10, 2026 09:24

nhsd-jack-wainwright commented Apr 10, 2026

View reviewed changes

MohammadPatelNHS force-pushed the feature/CDAPI-71-JW branch from 8c72967 to d2ff798 Compare April 10, 2026 15:11

[CDAPI-71]: Implemented PDM Mock

b0c9d80

MohammadPatelNHS force-pushed the feature/CDAPI-71-JW branch from d2ff798 to b0c9d80 Compare April 13, 2026 10:01


		COPY /resources/build/mocks ${LAMBDA_TASK_ROOT}

		# RUN mkdir -p /.aws


		return cast("BaseMockItem", response.get("Item"))

		def query_by_ddb_index(self, filter_expression: Any) -> list[BaseMockItem]:


		self.table = self.dynamodb.Table(self.table_name)

		def get_item_by_session_id(self, session_id: str) -> BaseMockItem:

-    def get_item_by_session_id(self, session_id: str) -> BaseMockItem:
+    def get_item_by_session_id[T: BaseMockItem](self, session_id: str, required_type: type[T]) -> T:
+        ...
+        item = response.get("Item")
+        if not isinstance(item, required_type):
+            raise ItemNotFoundException(f"Item found but was not expected type. item={item}")
+        return item

Conversation

nhsd-jack-wainwright commented Mar 17, 2026 • edited by MohammadPatelNHS Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Context

Type of changes

Checklist

Sensitive Information Declaration

Uh oh!

nhsd-jack-wainwright left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nhsd-jack-wainwright left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Apr 13, 2026

Quality Gate passed

Uh oh!

github-actions bot commented Apr 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nhsd-jack-wainwright commented Mar 17, 2026 •

edited by MohammadPatelNHS

Loading