Skip to content

PRMP-1087 Refactor document review status lock handling #963

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

Draft
NogaNHS wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

PRMP-1087 Refactor document review status lock handling #963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b5377a8
Refactor document review status handling: replace APPROVED_PENDING_DO…
NogaNHS Dec 18, 2025
e866afd
Update document review status handling: replace REVIEW_IN_PROGRESS wi…
NogaNHS Dec 18, 2025
5e8a633
[PRMP-1087] resolve conflicts, merge in main
steph-torres-nhs Dec 22, 2025
81bc711
Merge branch 'main' into PRMP-1087
steph-torres-nhs Dec 22, 2025
e9db86e
Merge branch 'main' into PRMP-1087
steph-torres-nhs Dec 24, 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
22 changes: 11 additions & 11 deletions app/src/types/blocks/documentReview.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,15 @@ export type DocumentReviewStatusDto = {
};

export enum DocumentReviewStatus {
PENDING_REVIEW = 'PENDING_REVIEW',
APPROVED = 'APPROVED',
APPROVED_PENDING_DOCUMENTS = 'APPROVED_PENDING_DOCUMENTS',
REJECTED = 'REJECTED',
AWAITING_DOCUMENTS = 'AWAITING_DOCUMENTS',
REJECTED_DUPLICATE = 'REJECTED_DUPLICATE',
REASSIGNED = 'REASSIGNED',
REASSIGNED_PATIENT_UNKNOWN = 'REASSIGNED_PATIENT_UNKNOWN',
NEVER_REVIEWED = 'NEVER_REVIEWED',
REVIEW_PENDING_UPLOAD = 'REVIEW_PENDING_UPLOAD',
VIRUS_SCAN_FAILED = 'VIRUS_SCAN_FAILED',
PENDING_REVIEW = "PENDING_REVIEW",
APPROVED = "APPROVED",
REVIEW_IN_PROGRESS = "REVIEW_IN_PROGRESS",
REJECTED = "REJECTED",
AWAITING_DOCUMENTS = "AWAITING_DOCUMENTS",
REJECTED_DUPLICATE = "REJECTED_DUPLICATE",
REASSIGNED = "REASSIGNED",
REASSIGNED_PATIENT_UNKNOWN = "REASSIGNED_PATIENT_UNKNOWN",
NEVER_REVIEWED = "NEVER_REVIEWED",
REVIEW_PENDING_UPLOAD = "REVIEW_PENDING_UPLOAD",
VIRUS_SCAN_FAILED = "VIRUS_SCAN_FAILED",
}
2 changes: 1 addition & 1 deletion lambdas/enums/document_review_status.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
class DocumentReviewStatus(StrEnum):
PENDING_REVIEW = "PENDING_REVIEW"
APPROVED = "APPROVED"
APPROVED_PENDING_DOCUMENTS = "APPROVED_PENDING_DOCUMENTS"
REVIEW_IN_PROGRESS = "REVIEW_IN_PROGRESS"
REJECTED = "REJECTED"
REJECTED_DUPLICATE = "REJECTED_DUPLICATE"
REASSIGNED = "REASSIGNED"
Expand Down
9 changes: 0 additions & 9 deletions lambdas/services/document_upload_review_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,15 +154,6 @@ def update_pending_review_status(
DocumentReviewStatus.PENDING_REVIEW,
)

def update_approved_pending_review_status(
self, review_update: DocumentUploadReviewReference, field_names: set[str]
) -> None:
self.update_review_document_with_status_filter(
review_update,
field_names,
DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS,
)

def update_review_document_with_status_filter(
self,
review_update: DocumentUploadReviewReference,
Expand Down
34 changes: 3 additions & 31 deletions lambdas/services/update_document_review_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ class UpdateDocumentReviewService:
DocumentReviewStatus.APPROVED,
]

APPROVED_PENDING_REVIEW_STATUSES = [DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS]

UNKNOWN_NHS_NUMBER = "0000000000"

FAILED_LOG_MESSAGE = "Failed to update document review"
Expand Down Expand Up @@ -112,7 +110,6 @@ def _validate_patient_id_match(self, document, patient_id: str):
def _validate_review_status(self, document):
if document.review_status not in [
DocumentReviewStatus.PENDING_REVIEW,
DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS,
]:
logger.error(
f"Invalid review status for document_id: {document.id}. "
Expand All @@ -133,25 +130,13 @@ def _validate_user_match_custodian(self, document, reviewer_ods_code: str):
403, LambdaError.DocumentReferenceUnauthorised
)

def _ensure_approved_status_follows_pending_review(self, document, update_data):
is_approved_pending_review = document.review_status == DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS
is_update_status_approved = update_data.review_status == DocumentReviewStatus.APPROVED

is_invalid_approved_pending_transition = is_approved_pending_review and not is_update_status_approved
is_approved_status_not_follows_pending_review = is_update_status_approved and not is_approved_pending_review
if is_approved_status_not_follows_pending_review or is_invalid_approved_pending_transition:
raise UpdateDocumentReviewException(
400, LambdaError.UpdateDocStatusUnavailable
)

def _process_review_status_update(
self,
document,
update_data: PatchDocumentReviewRequest,
document_id: str,
reviewer_ods_code: str,
):
self._ensure_approved_status_follows_pending_review(document, update_data)
self._set_review_metadata(document, update_data, reviewer_ods_code)
self._execute_status_action(document, update_data, document_id)

Expand All @@ -169,11 +154,9 @@ def _execute_status_action(self, document, update_data, document_id):
elif document.review_status in self.APPROVED_REVIEW_STATUSES:
document.document_reference_id = update_data.document_reference_id
update_fields.add("document_reference_id")
self._handle_approval(document, update_fields)
self._handle_rejection_or_approval(document, update_fields)
elif document.review_status in self.REJECTED_REVIEW_STATUSES:
self._handle_rejection(document, update_fields)
elif document.review_status in self.APPROVED_PENDING_REVIEW_STATUSES:
self._handle_approved_pending(document, update_fields)
self._handle_rejection_or_approval(document, update_fields)
else:
logger.error(
f"Invalid status update attempted: {document.review_status}",
Expand All @@ -183,23 +166,12 @@ def _execute_status_action(self, document, update_data, document_id):
400, LambdaError.DocumentReviewGeneralError
)

def _handle_rejection(self, document, update_fields):
def _handle_rejection_or_approval(self, document, update_fields):
self.document_review_service.update_pending_review_status(
review_update=document, field_names=update_fields
)
self._handle_soft_delete(document)

def _handle_approval(self, document, update_fields):
self.document_review_service.update_approved_pending_review_status(
review_update=document, field_names=update_fields
)
self._handle_soft_delete(document)

def _handle_approved_pending(self, document, update_fields):
self.document_review_service.update_pending_review_status(
review_update=document, field_names=update_fields
)

def _handle_soft_delete(self, review_document: DocumentUploadReviewReference):
logger.info(
f"Deleting document review files for document_id: {review_document.id}"
Expand Down
6 changes: 3 additions & 3 deletions lambdas/tests/unit/services/test_document_upload_review_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ def test_update_document_review_for_patient_conditional_check_failed(
field_names = {"review_status"}

with pytest.raises(DocumentReviewException):
mock_service.update_approved_pending_review_status(
mock_service.update_pending_review_status(
review_update=mock_review_update,
field_names=field_names,
)
Expand All @@ -262,7 +262,7 @@ def test_update_document_review_for_patient_conditional_check_failed(
expected_condition = (
Attr(DocumentReferenceMetadataFields.ID.value).exists()
& Attr("NhsNumber").eq(mock_review_update.nhs_number)
& Attr("ReviewStatus").eq(DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS)
& Attr("ReviewStatus").eq(DocumentReviewStatus.PENDING_REVIEW)
)

assert condition_expr == expected_condition
Expand All @@ -287,7 +287,7 @@ def test_update_document_review_for_patient_other_client_error(
field_names = {"review_status"}

with pytest.raises(DocumentReviewException):
mock_service.update_approved_pending_review_status(
mock_service.update_pending_review_status(
review_update=mock_review_update,
field_names=field_names,
)
Expand Down
116 changes: 9 additions & 107 deletions lambdas/tests/unit/services/test_update_document_review_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ def mock_pds_service(mocker):
def test_update_document_review_successfully_approves_document(
mock_service, mock_document_review
):
mock_document_review.review_status = DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS
mock_document_review.review_status = DocumentReviewStatus.PENDING_REVIEW
update_data = PatchDocumentReviewRequest(
review_status=DocumentReviewStatus.APPROVED,
document_reference_id=TEST_DOCUMENT_REFERENCE_ID,
Expand All @@ -108,10 +108,10 @@ def test_update_document_review_successfully_approves_document(
mock_service.document_review_service.get_document_review_by_id.assert_called_once_with(
document_id=TEST_DOCUMENT_ID, document_version=TEST_VERSION
)
mock_service.document_review_service.update_approved_pending_review_status.assert_called_once()
mock_service.document_review_service.update_pending_review_status.assert_called_once()

call_args = (
mock_service.document_review_service.update_approved_pending_review_status.call_args
mock_service.document_review_service.update_pending_review_status.call_args
)
updated_doc = call_args.kwargs["review_update"]
assert updated_doc.review_status == DocumentReviewStatus.APPROVED
Expand Down Expand Up @@ -251,38 +251,6 @@ def test_update_document_review_successfully_reassigns_document_when_patient_unk
assert existing_review.nhs_number == TEST_NHS_NUMBER


@freeze_time(TEST_FROZEN_TIME)
def test_update_document_review_successfully_sets_approved_pending_documents_status(
mock_service, mock_document_review
):
update_data = PatchDocumentReviewRequest(
review_status=DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS,
)
mock_service.document_review_service.get_document_review_by_id.return_value = (
mock_document_review
)

mock_service.update_document_review(
patient_id=TEST_NHS_NUMBER,
document_id=TEST_DOCUMENT_ID,
document_version=TEST_VERSION,
update_data=update_data,
reviewer_ods_code=TEST_REVIEWER_ODS_CODE,
)

mock_service.document_review_service.update_pending_review_status.assert_called_once()
call_args = (
mock_service.document_review_service.update_pending_review_status.call_args
)
updated_doc = call_args.kwargs["review_update"]
assert updated_doc.review_status == DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS
assert updated_doc.reviewer == TEST_REVIEWER_ODS_CODE
assert updated_doc.review_date == TEST_FROZEN_TIME_TIMESTAMP
assert "document_reference_id" not in call_args.kwargs["field_names"]
assert "reviewer" in call_args.kwargs["field_names"]
assert "review_date" in call_args.kwargs["field_names"]


def test_update_document_review_raises_exception_when_document_not_found(mock_service):
update_data = PatchDocumentReviewRequest(
review_status=DocumentReviewStatus.APPROVED,
Expand All @@ -307,31 +275,6 @@ def test_update_document_review_raises_exception_when_document_not_found(mock_se
mock_service.document_review_service.update_pending_review_status.assert_not_called()


def test_update_document_review_raises_exception_when_approving_from_pending_review(
mock_service, mock_document_review
):
update_data = PatchDocumentReviewRequest(
review_status=DocumentReviewStatus.APPROVED,
document_reference_id=TEST_DOCUMENT_REFERENCE_ID,
)
mock_service.document_review_service.get_document_review_by_id.return_value = (
mock_document_review
)

with pytest.raises(UpdateDocumentReviewException) as exc_info:
mock_service.update_document_review(
patient_id=TEST_NHS_NUMBER,
document_id=TEST_DOCUMENT_ID,
document_version=TEST_VERSION,
update_data=update_data,
reviewer_ods_code=TEST_REVIEWER_ODS_CODE,
)

assert exc_info.value.status_code == 400
assert exc_info.value.error == LambdaError.UpdateDocStatusUnavailable
mock_service.document_review_service.update_pending_review_status.assert_not_called()


def test_fetch_document_returns_document_when_exists(
mock_service, mock_document_review
):
Expand Down Expand Up @@ -382,7 +325,6 @@ def test_validate_patient_id_match_raises_exception_when_nhs_numbers_dont_match(
"valid_status",
[
DocumentReviewStatus.PENDING_REVIEW,
DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS,
],
)
def test_validate_review_status_passes_when_status_is_valid(
Expand Down Expand Up @@ -524,31 +466,11 @@ def test_create_reassigned_document_preserves_document_id(
assert result.id == TEST_DOCUMENT_ID


@freeze_time(TEST_FROZEN_TIME)
def test_process_review_status_update_sets_review_date_and_reviewer(
mock_service, mock_document_review
):
update_data = PatchDocumentReviewRequest(
review_status=DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS,
)

mock_service._process_review_status_update(
mock_document_review, update_data, TEST_DOCUMENT_ID, TEST_REVIEWER_ODS_CODE
)

assert mock_document_review.review_date == TEST_FROZEN_TIME_TIMESTAMP
assert mock_document_review.reviewer == TEST_REVIEWER_ODS_CODE
assert (
mock_document_review.review_status
== DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS
)


@freeze_time(TEST_FROZEN_TIME)
def test_process_review_status_update_calls_approved_service_for_approval(
mock_service, mock_document_review
):
mock_document_review.review_status = DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS
mock_document_review.review_status = DocumentReviewStatus.PENDING_REVIEW
update_data = PatchDocumentReviewRequest(
review_status=DocumentReviewStatus.APPROVED,
document_reference_id=TEST_DOCUMENT_REFERENCE_ID,
Expand All @@ -558,9 +480,9 @@ def test_process_review_status_update_calls_approved_service_for_approval(
mock_document_review, update_data, TEST_DOCUMENT_ID, TEST_REVIEWER_ODS_CODE
)

mock_service.document_review_service.update_approved_pending_review_status.assert_called_once()
mock_service.document_review_service.update_pending_review_status.assert_called_once()
call_args = (
mock_service.document_review_service.update_approved_pending_review_status.call_args
mock_service.document_review_service.update_pending_review_status.call_args
)
assert (
call_args.kwargs["review_update"].document_reference_id
Expand Down Expand Up @@ -623,30 +545,12 @@ def test_process_review_status_update_calls_handle_reassignment_for_reassignment
)


@freeze_time(TEST_FROZEN_TIME)
def test_process_review_status_update_raises_exception_when_approving_from_wrong_status(
mock_service, mock_document_review
):
mock_document_review.review_status = DocumentReviewStatus.PENDING_REVIEW
update_data = PatchDocumentReviewRequest(
review_status=DocumentReviewStatus.APPROVED,
document_reference_id=TEST_DOCUMENT_REFERENCE_ID,
)

with pytest.raises(UpdateDocumentReviewException) as exc_info:
mock_service._process_review_status_update(
mock_document_review, update_data, TEST_DOCUMENT_ID, TEST_REVIEWER_ODS_CODE
)

assert exc_info.value.status_code == 400
assert exc_info.value.error == LambdaError.UpdateDocStatusUnavailable


@freeze_time(TEST_FROZEN_TIME)
def test_process_review_status_update_calls_soft_delete_for_approval(
mock_service, mock_document_review, mocker
):
mock_document_review.review_status = DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS
mock_document_review.review_status = DocumentReviewStatus.PENDING_REVIEW
update_data = PatchDocumentReviewRequest(
review_status=DocumentReviewStatus.APPROVED,
document_reference_id=TEST_DOCUMENT_REFERENCE_ID,
Expand Down Expand Up @@ -725,11 +629,10 @@ def test_handle_reassignment_status_creates_new_document_and_updates_with_transa
DocumentReviewStatus.REASSIGNED_PATIENT_UNKNOWN,
],
)
def test_update_document_review_raises_exception_when_updating_approved_pending_to_invalid_status(
def test_update_document_review_raises_exception_when_updating_approved_to_invalid_status(
mock_service, mock_document_review, invalid_target_status
):
"""Test that APPROVED_PENDING_DOCUMENTS can only transition to APPROVED"""
mock_document_review.review_status = DocumentReviewStatus.APPROVED_PENDING_DOCUMENTS
mock_document_review.review_status = DocumentReviewStatus.APPROVED
update_data = PatchDocumentReviewRequest(
review_status=invalid_target_status,
nhs_number=TEST_REASSIGNED_NHS_NUMBER if "REASSIGNED" in invalid_target_status.value else None,
Expand Down Expand Up @@ -764,7 +667,6 @@ def test_update_document_review_raises_exception_when_updating_approved_pending_
def test_update_document_review_raises_exception_when_approving_from_non_approved_pending_status(
mock_service, mock_document_review, from_status
):
"""Test that only APPROVED_PENDING_DOCUMENTS can transition to APPROVED"""
mock_document_review.review_status = from_status
update_data = PatchDocumentReviewRequest(
review_status=DocumentReviewStatus.APPROVED,
Expand Down
Loading