Conversation
github-actions
bot
added
the
CODE RUB
There was a problem hiding this comment.
Pull request overview
Adds an integration test path for retrieving patient “NHS login user info” by introducing a DTO, wiring an NHS login service mock into the test host, and adding broker/test methods to call and validate the endpoint.
Changes:
- Added
NhsLoginUserInfomodel DTO for integration tests. - Overrode
INhsLoginServicewithin the integration test web host to return fixed user info. - Added an API broker method and a new integration test to fetch/verify
/patients/patientInfo.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| LondonDataServices.IDecide.Portal.Server.Tests.Integration/Models/NhsLoginUserInfo/NhsLoginUserInfo.cs | Introduces DTO used by tests to deserialize patient info response. |
| LondonDataServices.IDecide.Portal.Server.Tests.Integration/Brokers/TestWebApplicationFactory.cs | Adds an INhsLoginService override to return deterministic user info during tests. |
| LondonDataServices.IDecide.Portal.Server.Tests.Integration/Brokers/ApiBroker.cs | Adds shared JSON deserialization helper for raw HttpContent. |
| LondonDataServices.IDecide.Portal.Server.Tests.Integration/Brokers/ApiBroker.Patients.cs | Adds GetPatientInfoAsync broker call to the new endpoint. |
| LondonDataServices.IDecide.Portal.Server.Tests.Integration/Apis/Patients/PatientTests.GetPatientInfo.cs | Adds an integration test validating the returned patient info. |
| using LondonDataServices.IDecide.Core.Models.Foundations.NhsLogins; | ||
| using LondonDataServices.IDecide.Core.Services.Foundations.NhsLogins; |
There was a problem hiding this comment.
This PR introduces a test DTO named NhsLoginUserInfo under ...Tests.Integration.Models..., but the test host override constructs LondonDataServices.IDecide.Core.Models.Foundations.NhsLogins.NhsLoginUserInfo. Having two different NhsLoginUserInfo types in play is easy to confuse and can lead to subtle mismatches (attributes, casing, data types). Consider using a single type consistently (either the Core model everywhere, or the test DTO everywhere), or add a namespace alias/fully-qualify usages to make the intended type explicit.
| var testNhsLoginUserInfo = new NhsLoginUserInfo | ||
| { | ||
| Birthdate = new DateTime(1990, 1, 15), | ||
| FamilyName = "TestFamilyName", | ||
| GivenName = "TestGivenName", | ||
| Email = "test@example.com", | ||
| PhoneNumber = "+447887510886" | ||
| }; |
There was a problem hiding this comment.
This PR introduces a test DTO named NhsLoginUserInfo under ...Tests.Integration.Models..., but the test host override constructs LondonDataServices.IDecide.Core.Models.Foundations.NhsLogins.NhsLoginUserInfo. Having two different NhsLoginUserInfo types in play is easy to confuse and can lead to subtle mismatches (attributes, casing, data types). Consider using a single type consistently (either the Core model everywhere, or the test DTO everywhere), or add a namespace alias/fully-qualify usages to make the intended type explicit.
|
|
||
| return JsonSerializer.Deserialize<T>(jsonContent, jsonSerializerOptions); |
There was a problem hiding this comment.
JsonSerializer.Deserialize<T>(...) can return null (e.g., empty body, null literal, or incompatible JSON). Returning that as ValueTask<T> can push a null into callers unexpectedly. Consider changing the return type to ValueTask<T?> and handling null at call sites, or throw a clear exception when deserialization returns null.
| return JsonSerializer.Deserialize<T>(jsonContent, jsonSerializerOptions); | |
| T deserialized = JsonSerializer.Deserialize<T>(jsonContent, jsonSerializerOptions); | |
| if (deserialized is null) | |
| { | |
| throw new JsonException( | |
| $"Unable to deserialize HTTP content to type '{typeof(T).FullName}'."); | |
| } | |
| return deserialized; |
| public async ValueTask<NhsLoginUserInfo> GetPatientInfoAsync() | ||
| { | ||
| HttpResponseMessage response = | ||
| await this.authenticatedHttpClient.GetAsync($"{patientsRelativeUrl}/patientInfo"); |
There was a problem hiding this comment.
This method deserializes the response body without checking the HTTP status code. If the endpoint returns a non-2xx response (or an error payload), deserialization may produce misleading results or null. Consider calling response.EnsureSuccessStatusCode() (or otherwise asserting success) before deserializing.
| await this.authenticatedHttpClient.GetAsync($"{patientsRelativeUrl}/patientInfo"); | |
| await this.authenticatedHttpClient.GetAsync($"{patientsRelativeUrl}/patientInfo"); | |
| response.EnsureSuccessStatusCode(); |
| // Debug: Print the raw JSON to see what we're getting | ||
| var jsonOptions = new JsonSerializerOptions | ||
| { | ||
| PropertyNamingPolicy = JsonNamingPolicy.CamelCase, | ||
| PropertyNameCaseInsensitive = true, | ||
| WriteIndented = true | ||
| }; | ||
|
|
||
| var debugJson = JsonSerializer.Serialize(actualNhsLoginUserInfo, jsonOptions); | ||
| System.Diagnostics.Debug.WriteLine($"Received JSON: {debugJson}"); |
There was a problem hiding this comment.
This looks like temporary debugging code and adds noise to test output/maintenance. Consider removing it, or if you need diagnostics, route it through the test framework output (e.g., ITestOutputHelper) and/or guard it behind a conditional compilation symbol so it doesn’t run in normal CI.
| // --------------------------------------------------------- | ||
|
|
||
| using System; | ||
| using System.Net.Http; |
There was a problem hiding this comment.
This using is unused in this file and can be removed to keep the test file tidy.
| using System.Net.Http; |
...Services.IDecide.Portal.Server.Tests.Integration/Models/NhsLoginUserInfo/NhsLoginUserInfo.cs
Show resolved
Hide resolved
1 participant
CLOSES AB#27298