[GPCAPIM-255] Controller module #49

Vox-Ben · 2026-01-20T16:03:13Z

Description

Creates the controller class that orchestrates calls to the other gateway components and to the GP provider

Context

This module is needed so that we have something that actually calls all the components that we need to call and manages their responses.

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.

github-actions · 2026-01-20T16:05:59Z

ALB Target: arn:aws:elasticloadbalancing:eu-west-2:900119715266:targetgroup/gpcapim-255-controller-module/b4084f2f62453ce7
Preview URL: https://feature-gpcapim-255-controller-module.dev.endpoints.clinical-data-gateway.national.nhs.uk

github-actions · 2026-01-20T16:33:13Z

ALB Target: arn:aws:elasticloadbalancing:eu-west-2:900119715266:targetgroup/gpcapim-255-controller-module/b4084f2f62453ce7
Preview URL: https://feature-gpcapim-255-controller-module.dev.endpoints.clinical-data-gateway.national.nhs.uk

gateway-api/src/gateway_api/common/common.py

gateway-api/src/gateway_api/controller.py

DWolfsNHS · 2026-01-20T16:44:10Z

gateway-api/src/gateway_api/controller.py

+        self,
+        trace_id: str,  # NOSONAR S1172 (ignore in stub)
+        body: json_str,  # NOSONAR S1172 (ignore in stub)
+        nhsnumber: str,  # NOSONAR S1172 (ignore in stub)


nhsnumber won't be needed for this method

Suggested change

nhsnumber: str, # NOSONAR S1172 (ignore in stub)

We can get the patient's records without the NHS number? How does the provider know which patient's records to send us?

It's contained as a parameter in the request body, which we just forward on.

github-actions · 2026-01-20T21:14:20Z

ALB Target: arn:aws:elasticloadbalancing:eu-west-2:900119715266:targetgroup/gpcapim-255-controller-module/b4084f2f62453ce7
Preview URL: https://feature-gpcapim-255-controller-module.dev.endpoints.clinical-data-gateway.national.nhs.uk

github-actions · 2026-01-20T22:26:11Z

ALB Target: arn:aws:elasticloadbalancing:eu-west-2:900119715266:targetgroup/gpcapim-255-controller-module/b4084f2f62453ce7
Preview URL: https://feature-gpcapim-255-controller-module.dev.endpoints.clinical-data-gateway.national.nhs.uk

github-actions · 2026-01-21T16:13:02Z

ALB Target: arn:aws:elasticloadbalancing:eu-west-2:900119715266:targetgroup/gpcapim-255-controller-module/b4084f2f62453ce7
Preview URL: https://feature-gpcapim-255-controller-module.dev.endpoints.clinical-data-gateway.national.nhs.uk

sonarqubecloud · 2026-01-21T16:15:08Z

Quality Gate failed

Failed conditions
91.7% Coverage on New Code (required ≥ 95%)

See analysis details on SonarQube Cloud

DWolfsNHS · 2026-01-22T14:41:11Z

gateway-api/src/gateway_api/controller.py

+        except (TypeError, json.JSONDecodeError):
+            raise RequestError(
+                status_code=400,
+                message='Request body must be valid JSON with an "nhs-number" field',


will this exception throw when there's no nhs-number, or just if it's not valid json?

DWolfsNHS · 2026-01-22T14:42:37Z

gateway-api/src/gateway_api/controller.py

+        ):  # Must be a dict-like object
+            raise RequestError(
+                status_code=400,
+                message='Request body must be a JSON object with an "nhs-number" field',


will this throw for missing nhs-number, or just if the structure is wrong?

DWolfsNHS · 2026-01-22T15:08:28Z

gateway-api/src/gateway_api/controller.py

+
+        return consumer_asid, provider_asid, provider_endpoint
+
+    def call_gp_provider(


I think this does more than call the provider (the provider module does only that): perhaps orchestrate_gp_provider-call?

davidhamill1-nhs

1329 lines of code is not a small PR.

gateway-api/src/gateway_api/common/common.py

davidhamill1-nhs · 2026-01-22T10:54:53Z

gateway-api/src/gateway_api/common/common.py

+    :returns: ``True`` if the number is a valid NHS number, otherwise ``False``.
+    """
+    str_value = str(value)  # Just in case they passed an integer
+    digits = re.sub(r"\D", "", str_value or "")


This will convert "94347NOT_AN_NHS_NUMBER65919" to "9434765919". Do we want that? I don't think I have come across an NHS number written with anything other than a space, hyphen or digits. And PDS FHIR API expects and returns digits only. May need to confirm expectations for this.

>>> str_value = "46HELLO98194180" >>> digits = re.sub(r"\D", "", str_value or "") >>> digits '4698194180'

davidhamill1-nhs · 2026-01-22T11:08:48Z

gateway-api/src/gateway_api/common/test_common.py

+def test_validate_nhs_number_accepts_valid_number_with_separators() -> None:
+    """
+    Validate that separators (spaces, hyphens) are ignored and valid numbers pass.
+    """
+    assert common.validate_nhs_number("943 476 5919") is True
+    assert common.validate_nhs_number("943-476-5919") is True
+    assert common.validate_nhs_number(9434765919) is True


Having a multiple asserts in a unit test means that the first failing assertion prevents any later assertions. You could convert this to a parameterized test averts this.

Suggested change

def test_validate_nhs_number_accepts_valid_number_with_separators() -> None:

"""

Validate that separators (spaces, hyphens) are ignored and valid numbers pass.

"""

assert common.validate_nhs_number("943 476 5919") is True

assert common.validate_nhs_number("943-476-5919") is True

assert common.validate_nhs_number(9434765919) is True

@pytest.mark.parametrize(

"valid_nhs_number",

[

"9434765919",

"9876543210",

9434765919,

],

)

def test_validate_nhs_number_accepts_valid_number_with_separators(

valid_nhs_number: str | int,

) -> None:

"""

Validate that separators (spaces, hyphens) are ignored and valid numbers pass.

"""

assert common.validate_nhs_number(valid_nhs_number) is True

davidhamill1-nhs · 2026-01-22T11:15:12Z

gateway-api/src/gateway_api/common/test_common.py

+def test_validate_nhs_number_returns_false_for_non_ten_digits_and_non_numeric() -> None:
+    """
+    validate_nhs_number should return False when:
+    - The number of digits is not exactly 10.
+    - The input is not numeric.
+
+    Notes:
+    - The implementation strips non-digit characters before validation, so a fully
+        non-numeric input becomes an empty digit string and is rejected.
+    """
+    # Not ten digits after stripping -> False


Your test function name, its doc string and the comments all repeat one another.

Suggested change

def test_validate_nhs_number_returns_false_for_non_ten_digits_and_non_numeric() -> None:

"""

validate_nhs_number should return False when:

- The number of digits is not exactly 10.

- The input is not numeric.

Notes:

- The implementation strips non-digit characters before validation, so a fully

non-numeric input becomes an empty digit string and is rejected.

"""

# Not ten digits after stripping -> False

def test_validate_nhs_number_returns_false_for_non_ten_digits_and_non_numeric() ->

davidhamill1-nhs · 2026-01-22T11:16:09Z

gateway-api/src/gateway_api/common/test_common.py

+    assert common.validate_nhs_number("12345678901") is False
+
+    # Not numeric -> False (becomes 0 digits after stripping)
+    assert common.validate_nhs_number("NOT_A_NUMBER") is False


See previous comment. May wish to add a test for

Suggested change

assert common.validate_nhs_number("NOT_A_NUMBER") is False

assert common.validate_nhs_number("NOT_A_NUMBER") is False

assert common.validate_nhs_number("94347659NOT_A_NUMBER19") is False

davidhamill1-nhs · 2026-01-22T14:58:18Z

gateway-api/src/gateway_api/test_controller.py

+    monkeypatch.setattr(controller_module, "GpProviderClient", FakeGpProviderClient)
+
+
+def _make_controller() -> Controller:


If you make this a fixture, you can just pull it through the test functions parameters. ```suggestion
@pytest.fixture
def controller() -> Controller:

davidhamill1-nhs · 2026-01-22T15:03:47Z

gateway-api/src/gateway_api/test_controller.py

+    return SimpleNamespace(gp_ods_code=gp_ods_code)
+
+
+class FakePdsClient:


Can we not use the stub_pds.PdsFhirApiStub?

davidhamill1-nhs · 2026-01-22T15:04:22Z

gateway-api/src/gateway_api/test_controller.py

+        return self._org_details_by_ods.get(ods_code)
+
+
+class FakeGpProviderClient:


I guess with @DWolfsNHS GP stub yet to go in, we can't yet call on that in the same way we could with the PDS FHIR one.

davidhamill1-nhs · 2026-01-22T15:11:55Z

gateway-api/src/gateway_api/test_controller.py

+    c = _make_controller()
+
+    # PDS returns None by default
+    body = make_request_body("9434765919")
+    headers = make_headers()
+
+    r = c.call_gp_provider(body, headers, "token-abc")
+
+    assert r.status_code == 404
+    assert "No PDS patient found for NHS number" in (r.data or "")


Given how much is mocked, it feels like this is testing the mocks more than the actual controller.

It is not clear from the test setup why this should return a 404/No patient found.

davidhamill1-nhs · 2026-01-22T15:20:21Z

gateway-api/src/gateway_api/test_controller.py

These feel less like unit tests, testing a unit of code, and more like integration tests.

You could break call_gp_prodvier in to multiple private method calls, eg
Put

try: nhs_number = self._get_details_from_body(request_body) except RequestError as err: return FlaskResponse( status_code=err.status_code, data=str(err), ) # Extract consumer ODS from headers consumer_ods = headers.get("Ods-from", "").strip() if not consumer_ods: return FlaskResponse( status_code=400, data='Missing required header "Ods-from"', ) trace_id = headers.get("X-Request-ID") if trace_id is None: return FlaskResponse( status_code=400, data="Missing required header: X-Request-ID" ) try: provider_ods = self._get_pds_details(auth_token, consumer_ods, nhs_number) except RequestError as err: return FlaskResponse(status_code=err.status_code, data=str(err))

in its own method and unit test that. And similarly with the calls to ODS for the provider and the consumer.

DWolfsNHS · 2026-01-22T15:23:52Z

gateway-api/src/gateway_api/controller.py

+        )
+
+
+def _coerce_nhs_number_to_int(value: str | int) -> int:


It feel like this would this sit along side validate_nhs_number - both here, or both in common.

Vox-Ben requested a review from a team as a code owner January 20, 2026 16:03

Vox-Ben added 7 commits January 20, 2026 16:03

Change return type to flask response

9b229de

Refactor things to make ruff happy

56ee98b

Pass the request body & multiple SDS calls

bc4ddab

Mypy happy, tests passing

dd4dea4

Tests passing. Maybe got too many tests.

2601d55

Trim some unnecessary unit tests

18039c2

Sort out docstrings

fc5b194

Vox-Ben force-pushed the feature/GPCAPIM-255_controller_module branch from 1ab29e7 to fc5b194 Compare January 20, 2026 16:04

Vox-Ben added 2 commits January 20, 2026 16:29

Add tests for coverage

a00639e

Add tests for coverage

dff5ef4

DWolfsNHS reviewed Jan 20, 2026

View reviewed changes

gateway-api/src/gateway_api/common/common.py Show resolved Hide resolved

DWolfsNHS reviewed Jan 20, 2026

View reviewed changes

gateway-api/src/gateway_api/controller.py Outdated Show resolved Hide resolved

DWolfsNHS reviewed Jan 20, 2026

View reviewed changes

Change GP Connect to GP provider

66d8bdf

Remove redundant parentheses

05f81e2

Fix expected response

c39e48c

DWolfsNHS reviewed Jan 22, 2026

View reviewed changes

davidhamill1-nhs reviewed Jan 22, 2026

View reviewed changes

DWolfsNHS reviewed Jan 22, 2026

View reviewed changes


		return consumer_asid, provider_asid, provider_endpoint

		def call_gp_provider(

	assert common.validate_nhs_number("NOT_A_NUMBER") is False
	assert common.validate_nhs_number("NOT_A_NUMBER") is False
	assert common.validate_nhs_number("94347659NOT_A_NUMBER19") is False

		monkeypatch.setattr(controller_module, "GpProviderClient", FakeGpProviderClient)


		def _make_controller() -> Controller:

		return SimpleNamespace(gp_ods_code=gp_ods_code)


		class FakePdsClient:

		return self._org_details_by_ods.get(ods_code)


		class FakeGpProviderClient:

[GPCAPIM-255] Controller module #49

Are you sure you want to change the base?

[GPCAPIM-255] Controller module #49

Conversation

Vox-Ben commented Jan 20, 2026

Description

Context

Type of changes

Checklist

Sensitive Information Declaration

Uh oh!

github-actions bot commented Jan 20, 2026

Uh oh!

github-actions bot commented Jan 20, 2026

Uh oh!

Uh oh!

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!

github-actions bot commented Jan 20, 2026

Uh oh!

github-actions bot commented Jan 20, 2026

Uh oh!

github-actions bot commented Jan 21, 2026

Uh oh!

sonarqubecloud bot commented Jan 21, 2026

Quality Gate failed

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!

davidhamill1-nhs left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants