VED-835 Refactor create immunisation endpoint#926
Merged
dlzhry2nhs merged 7 commits intomasterfrom Oct 24, 2025
Merged
Conversation
Contributor
|
This branch is working on a ticket in the NHS England VED JIRA Project. Here's a handy link to the ticket: VED-835 |
dlzhry2nhs
temporarily deployed
to
internal-dev
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev
GitHub Actions
Inactive
dlzhry2nhs
commented
Oct 22, 2025
| """it should return 400 if json has superseded nhs number.""" | ||
| # Given | ||
| create_result = { | ||
| "diagnostics": "Validation errors: contained[?(@.resourceType=='Patient')].identifier[0].value does not exists" |
Collaborator
Author
There was a problem hiding this comment.
No longer a valid scenario. See 2aea793#diff-3ea35476d2424353d5982240bd60f7d32bda82e49830191d5bf200ca7f21c2ecL367 and ticket for further context.
dlzhry2nhs
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
dlzhry2nhs
commented
Oct 22, 2025
| } | ||
| ) | ||
|
|
||
| def test_add_patient(self): |
Collaborator
Author
There was a problem hiding this comment.
All these tests are obsolete - basically repeated scenarios with weak assertions that are already covered by the first.
dlzhry2nhs
commented
Oct 22, 2025
| def test_decimal_on_create(self): | ||
| """it should create Immunization, and preserve decimal value""" | ||
| imms = create_covid_19_immunization_dict(imms_id="an-id") | ||
| imms["doseQuantity"] = 0.7477 |
Collaborator
Author
There was a problem hiding this comment.
This was invalid FHIR.
dlzhry2nhs
temporarily deployed
to
internal-dev
GitHub Actions
Inactive
JamesW1-NHS
reviewed
Oct 22, 2025
JamesW1-NHS
previously approved these changes
Oct 22, 2025
dlzhry2nhs
force-pushed
the
feature/VED-835-refactor-create-endpoint
branch
from
1b270d0 to
39b4b37
Compare
dlzhry2nhs
temporarily deployed
to
internal-dev
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
|
dlzhry2nhs
temporarily deployed
to
internal-dev
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
dlzhry2nhs
temporarily deployed
to
internal-dev
GitHub Actions
Inactive
mfjarvis
reviewed
Oct 24, 2025
mfjarvis
approved these changes
Oct 24, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Routine Change
No functional changes; purely refactoring into controller -> service -> repository
Added error scenarios for the Create Imms journey into the FHIR API exception decorator
Moved the duplicate checking into the service layer.
Simplified each layer, improved readability and return types.
Additionally added some extra terraform constraints which I think will help with the fresh environment deployment flakiness
FOR DISCUSSION: I have modified the code so that the service passes around the actual FHIR Immunization entity rather than dicts. Dicts are mutable and do not help to document the code. If we do not want to do this, I can remove the commit (number 4 I think). One other thing to consider is that it will change the ordering of the JSON that is dumped to the DB. E.g. previously "id" would be the last key as it was added last, but now it will be at the top, just after resource, because we are using the
.jsonmethod of the FHIR entity. Hopefully that is okay - as the Get endpoint also uses the FHIR resource to dump so it will not affect anything the client sees when retrieving. As long as we don't care about the particular ordering of what is actually stored to Dynamo then should be fine.Reviews Required
Review Checklist
ℹ️ This section is to be filled in by the reviewer.