Skip to content

VED-850 Redundant code #956

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JamesW1-NHS merged 7 commits into from
Nov 3, 2025
Merged

VED-850 Redundant code #956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e02b345
remove unused CsvImmunisationErrorModel
JamesW1-NHS Oct 30, 2025
66aaaec
remove redundant UpdateOutcome
JamesW1-NHS Oct 30, 2025
5921c1c
remove redundant UpdateOutcome II
JamesW1-NHS Oct 30, 2025
a53f830
Merge branch 'master' into VED-850-redundant-code
JamesW1-NHS Oct 31, 2025
6990adc
Merge branch 'master' into VED-850-redundant-code
JamesW1-NHS Oct 31, 2025
cab1434
dummy commit for github actions
JamesW1-NHS Oct 31, 2025
94a0a8b
Merge branch 'master' into VED-850-redundant-code
JamesW1-NHS Nov 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions backend/src/controller/fhir_controller.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
from models.utils.generic_utils import check_keys_in_sources
from parameter_parser import create_query_string, process_params, process_search_params
from repository.fhir_repository import ImmunizationRepository, create_table
from service.fhir_service import FhirService, UpdateOutcome, get_service_url
from service.fhir_service import FhirService, get_service_url

IMMUNIZATION_ENV = os.getenv("IMMUNIZATION_ENV")

Expand Down Expand Up @@ -297,7 +297,7 @@ def update_immunization(self, aws_event):
diagnostics=resource["diagnostics"],
)
return create_response(400, json.dumps(exp_error))
if outcome == UpdateOutcome.UPDATE:
if outcome:
return create_response(
200, None, {"E-Tag": updated_version}
) # include e-tag here, is it not included in the response resource
Expand Down
7 changes: 0 additions & 7 deletions backend/src/models/failures.py
View file
Open in desktop

This file was deleted.

24 changes: 9 additions & 15 deletions backend/src/service/fhir_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import logging
import os
import uuid
from enum import Enum
from typing import Optional, Union, cast
from uuid import uuid4

Expand Down Expand Up @@ -70,11 +69,6 @@ def get_service_url(
return f"https://{subdomain}api.service.nhs.uk/{service_base_path}"


class UpdateOutcome(Enum):
UPDATE = 0
CREATE = 1


class FhirService:
def __init__(
self,
Expand Down Expand Up @@ -168,13 +162,13 @@ def update_immunization(
existing_resource_version: int,
existing_resource_vacc_type: str,
supplier_system: str,
) -> tuple[Optional[UpdateOutcome], Immunization | dict, Optional[int]]:
) -> tuple[bool, Immunization | dict, Optional[int]]:
# VED-747 - TODO - this and the below 2 methods are duplicated. We should streamline the update journey
immunization["id"] = imms_id

patient = self._validate_patient(immunization)
if "diagnostics" in patient:
return None, patient, None
return False, patient, None
Copy link
Collaborator

@dlzhry2nhs dlzhry2nhs Oct 31, 2025

Choose a reason for hiding this comment

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

We should not be looking for 'diagnostics' in patient. This is all legacy code from when we used to interact with PDS. I have removed it from the create journey and will be doing so for the in-flight ticket where we do the full refactoring of the update service.

Copy link
Contributor Author

@JamesW1-NHS JamesW1-NHS Oct 31, 2025

Choose a reason for hiding this comment

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

Good spot. In fact that means I can remove the 'outcome' element from the method return entirely, and all the tests which check it.

Copy link
Contributor Author

@JamesW1-NHS JamesW1-NHS Oct 31, 2025

Choose a reason for hiding this comment

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

After discussion, I won't remove the check for 'diagnostics' nor the 'outcome' element yet in this ticket. It should be resolved in 747.


vaccination_type = get_vaccine_type(immunization)

Expand All @@ -191,7 +185,7 @@ def update_immunization(
imms_id, immunization, patient, existing_resource_version, supplier_system
)

return UpdateOutcome.UPDATE, Immunization.parse_obj(imms), updated_version
return True, Immunization.parse_obj(imms), updated_version

def reinstate_immunization(
self,
Expand All @@ -200,11 +194,11 @@ def reinstate_immunization(
existing_resource_version: int,
existing_resource_vacc_type: str,
supplier_system: str,
) -> tuple[Optional[UpdateOutcome], Immunization | dict, Optional[int]]:
) -> tuple[bool, Immunization | dict, Optional[int]]:
immunization["id"] = imms_id
patient = self._validate_patient(immunization)
if "diagnostics" in patient:
return None, patient, None
return False, patient, None

vaccination_type = get_vaccine_type(immunization)

Expand All @@ -219,7 +213,7 @@ def reinstate_immunization(
imms_id, immunization, patient, existing_resource_version, supplier_system
)

return UpdateOutcome.UPDATE, Immunization.parse_obj(imms), updated_version
return True, Immunization.parse_obj(imms), updated_version

def update_reinstated_immunization(
self,
Expand All @@ -228,11 +222,11 @@ def update_reinstated_immunization(
existing_resource_version: int,
existing_resource_vacc_type: str,
supplier_system: str,
) -> tuple[Optional[UpdateOutcome], Immunization | dict, Optional[int]]:
) -> tuple[bool, Immunization | dict, Optional[int]]:
immunization["id"] = imms_id
patient = self._validate_patient(immunization)
if "diagnostics" in patient:
return None, patient, None
return False, patient, None

vaccination_type = get_vaccine_type(immunization)

Expand All @@ -251,7 +245,7 @@ def update_reinstated_immunization(
supplier_system,
)

return UpdateOutcome.UPDATE, Immunization.parse_obj(imms), updated_version
return True, Immunization.parse_obj(imms), updated_version

def delete_immunization(self, imms_id: str, supplier_system: str) -> None:
"""
Expand Down
14 changes: 7 additions & 7 deletions backend/tests/controller/test_fhir_controller.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
)
from parameter_parser import patient_identifier_system, process_search_params
from repository.fhir_repository import ImmunizationRepository
from service.fhir_service import FhirService, UpdateOutcome
from service.fhir_service import FhirService
from testing_utils.generic_utils import load_json_data
from testing_utils.immunization_utils import create_covid_immunization

Expand Down Expand Up @@ -951,7 +951,7 @@ def test_update_immunization(self):
"pathParameters": {"id": imms_id},
}
self.service.update_immunization.return_value = (
UpdateOutcome.UPDATE,
True,
"value doesn't matter",
2,
)
Expand Down Expand Up @@ -1055,7 +1055,7 @@ def test_update_immunization_for_batch_existing_record_is_none(self):
"pathParameters": {"id": imms_id},
}
self.service.update_immunization.return_value = (
UpdateOutcome.UPDATE,
True,
"value doesn't matter",
)
self.service.get_immunization_by_id_all.return_value = None
Expand Down Expand Up @@ -1104,7 +1104,7 @@ def test_update_deletedat_immunization_with_version(self):
"body": imms,
"pathParameters": {"id": imms_id},
}
self.service.reinstate_immunization.return_value = UpdateOutcome.UPDATE, {}, 2
self.service.reinstate_immunization.return_value = True, {}, 2
self.service.get_immunization_by_id_all.return_value = {
"resource": "new_value",
"Version": 1,
Expand All @@ -1128,7 +1128,7 @@ def test_update_deletedat_immunization_without_version(self):
"body": imms,
"pathParameters": {"id": imms_id},
}
self.service.reinstate_immunization.return_value = UpdateOutcome.UPDATE, {}, 2
self.service.reinstate_immunization.return_value = True, {}, 2
self.service.get_immunization_by_id_all.return_value = {
"resource": "new_value",
"Version": 1,
Expand Down Expand Up @@ -1311,7 +1311,7 @@ def test_update_immunization_when_reinstated_true(self):
"pathParameters": {"id": imms_id},
}
self.service.update_reinstated_immunization.return_value = (
UpdateOutcome.UPDATE,
True,
{},
3,
)
Expand Down Expand Up @@ -1359,7 +1359,7 @@ def test_update_reinstated_immunization_with_diagnostics_error(self):
"VaccineType": "COVID",
}
self.service.reinstate_immunization.return_value = (
None,
False,
{"diagnostics": "Patient NHS number has been superseded"},
None,
)
Expand Down
12 changes: 6 additions & 6 deletions backend/tests/service/test_fhir_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
from models.fhir_immunization import ImmunizationValidator
from models.utils.generic_utils import get_contained_patient
from repository.fhir_repository import ImmunizationRepository
from service.fhir_service import FhirService, UpdateOutcome, get_service_url
from service.fhir_service import FhirService, get_service_url
from testing_utils.generic_utils import load_json_data
from testing_utils.immunization_utils import (
VALID_NHS_NUMBER,
Expand Down Expand Up @@ -634,7 +634,7 @@ def test_update_immunization(self):
outcome, _, _ = self.fhir_service.update_immunization(imms_id, req_imms, 1, "COVID", "Test")

# Then
self.assertEqual(outcome, UpdateOutcome.UPDATE)
self.assertTrue(outcome)
self.imms_repo.update_immunization.assert_called_once_with(imms_id, req_imms, req_patient, 1, "Test")
self.authoriser.authorise.assert_called_once_with("Test", ApiOperationCode.UPDATE, {"COVID"})

Expand Down Expand Up @@ -696,7 +696,7 @@ def test_reinstate_immunization_returns_updated_version(self):

outcome, resource, version = self.fhir_service.reinstate_immunization(imms_id, req_imms, 1, "COVID", "Test")

self.assertEqual(outcome, UpdateOutcome.UPDATE)
self.assertTrue(outcome)
self.assertEqual(version, 5)

def test_reinstate_immunization_raises_exception_when_missing_authz(self):
Expand Down Expand Up @@ -724,7 +724,7 @@ def test_update_reinstated_immunization_returns_updated_version(self):
imms_id, req_imms, 1, "COVID", "Test"
)

self.assertEqual(outcome, UpdateOutcome.UPDATE)
self.assertTrue(outcome)
self.assertEqual(version, 9)

def test_reinstate_immunization_with_diagnostics(self):
Expand All @@ -735,7 +735,7 @@ def test_reinstate_immunization_with_diagnostics(self):

outcome, resource, version = self.fhir_service.reinstate_immunization(imms_id, req_imms, 1, "COVID", "Test")

self.assertIsNone(outcome)
self.assertFalse(outcome)
self.assertEqual(resource, {"diagnostics": "invalid patient"})
self.assertIsNone(version)
self.imms_repo.reinstate_immunization.assert_not_called()
Expand All @@ -750,7 +750,7 @@ def test_update_reinstated_immunization_with_diagnostics(self):
imms_id, req_imms, 1, "COVID", "Test"
)

self.assertIsNone(outcome)
self.assertFalse(outcome)
self.assertEqual(resource, {"diagnostics": "invalid patient"})
self.assertIsNone(version)
self.imms_repo.update_reinstated_immunization.assert_not_called()
Expand Down