Skip to content

VED-747 Refactor update immunisation endpoint#966

Merged
dlzhry2nhs merged 7 commits intomasterfrom
feature/VED-747-refactor-update-imms-journey
Nov 5, 2025
Merged

VED-747 Refactor update immunisation endpoint#966
dlzhry2nhs merged 7 commits intomasterfrom
feature/VED-747-refactor-update-imms-journey

Conversation

@dlzhry2nhs
Copy link
Collaborator

@dlzhry2nhs dlzhry2nhs commented Nov 4, 2025

Summary

  • Routine Change

Refactoring - no functional change with the exception of the following which I will flag to testers and mention as a comment:

  • 404 scenario validation error message subtly changed for consistency with other endpoints
  • header validation tightened. No longer accepts 0 and negative numbers (more details in comment)

General refactoring changes:

Controller:

  • Vastly simplified. Previously it was executing a lot of logic. It is now simply responsible for parsing of the data and light touch validation of the path params and so on.
  • Business logic has been moved down to the service layer.
  • Decorator added to controller method to handle known and unexpected exceptions.

Service:

  • removed get_immunization_by_id_all this was called previously by the update controller, and confusingly masked a lot of business logic.
  • replicated business logic which was in the controller into the service layer. Attempted to abstract any of the checks into validation_utils. Hopefully you agree it's much more readable now

Repository:

  • Refactored get_immunization_and_resource_meta_by_id and removed get_immunization_by_id_all. They were basically doing the same thing albeit a switch can now be passed in to specify whether we include logically deleted resources in our request. Abstracted the horrendous inconsistent dict we were returning with ImmunizationRecordMetadata
  • Simplified update immunization operation and removed superfluous methods for the different flavours of update i.e. plain update, reinstate and update reinstated. The latter is just an update tbh.

Reviews Required

  • Dev

Review Checklist

ℹ️ This section is to be filled in by the reviewer.

  • I have reviewed the changes in this PR and they fill all or part of the acceptance criteria of the ticket, and the code is in a mergeable state.
  • If there were infrastructure, operational, or build changes, I have made sure there is sufficient evidence that the changes will work.
  • I have ensured the changelog has been updated by the submitter, if necessary.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2025

This branch is working on a ticket in the NHS England VED JIRA Project. Here's a handy link to the ticket:

VED-747

def __str__(self):
return f"The provided id:{self.imms_id} doesn't match with the content of the message"
return (
f"Validation errors: The provided immunization id:{self.imms_id} doesn't match with the content of the "
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were previously not using this error. I adjusted the error message to avoid any regression, now that we use it when validating the IDs.

except (ValidationError, ValueError, MandatoryError) as error:
raise CustomValidationError(message=str(error)) from error

patient = self._validate_patient(immunization)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obsolete after removing PDS checks.

imms_id = "an-id"
req_imms = create_covid_immunization_dict(imms_id)
self.fhir_service._validate_patient = MagicMock(return_value={})
self.assertEqual(str(error.exception), "Immunization resource does not exist. ID: non-existent-id-123")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testers: new validation error message will look like this which is consistent for 404 errors in our other endpoints.

Previously we were returning: Validation errors: The requested immunization resource with id:{imms_id} was not found. Please check if you have assertions which check this particular message in the Update endpoint, as they will need changing after this gets merged and deployed.

@dlzhry2nhs dlzhry2nhs temporarily deployed to internal-dev-sandbox November 4, 2025 11:57 — with GitHub Actions Inactive
@dlzhry2nhs dlzhry2nhs marked this pull request as ready for review November 4, 2025 12:01
@dlzhry2nhs dlzhry2nhs temporarily deployed to internal-dev-sandbox November 4, 2025 12:02 — with GitHub Actions Inactive

@staticmethod
def _is_valid_resource_version(resource_version: str) -> bool:
return resource_version.isdigit() and int(resource_version) > 0
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testers - minor change. Previously we permitted 0 and negative numbers. We will now catch such an error earlier on in the flow and get an error stating the the resource version is invalid.

Previously, this would have been accepted but then the update would fail on the old resource vs. new resource version check, as a resource in the database would only have a positive int as the resource version due to the constraints during both create and update operations.

JamesW1-NHS
JamesW1-NHS previously approved these changes Nov 5, 2025
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 5, 2025

@dlzhry2nhs dlzhry2nhs merged commit fae18b2 into master Nov 5, 2025
17 checks passed
@dlzhry2nhs dlzhry2nhs deleted the feature/VED-747-refactor-update-imms-journey branch November 5, 2025 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants