From b67010ea7549de48d985750a5d6ba751686319cc Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Wed, 25 Feb 2026 11:31:21 +0000 Subject: [PATCH 01/15] [PRMP-1505] added created and last_updated column to ods report --- lambdas/services/ods_report_service.py | 226 ++++++++++---- .../unit/services/test_ods_report_service.py | 284 +++++++++++------- lambdas/tests/unit/utils/test_utilities.py | 90 +++++- lambdas/utils/utilities.py | 88 ++++++ 4 files changed, 532 insertions(+), 156 deletions(-) diff --git a/lambdas/services/ods_report_service.py b/lambdas/services/ods_report_service.py index e892b09004..6507cbe7b3 100644 --- a/lambdas/services/ods_report_service.py +++ b/lambdas/services/ods_report_service.py @@ -1,6 +1,11 @@ import os import tempfile from datetime import datetime +from typing import Any + +from openpyxl.workbook import Workbook +from reportlab.lib.pagesizes import letter +from reportlab.pdfgen import canvas from enums.dynamo_filter import AttributeOperator from enums.file_type import FileType @@ -10,9 +15,6 @@ from enums.report_type import ReportType from enums.repository_role import RepositoryRole from models.document_review import DocumentUploadReviewReference -from openpyxl.workbook import Workbook -from reportlab.lib.pagesizes import letter -from reportlab.pdfgen import canvas from services.base.dynamo_service import DynamoDBService from services.base.s3_service import S3Service from services.search_document_review_service import DocumentUploadReviewService @@ -21,6 +23,11 @@ from utils.dynamo_query_filter_builder import DynamoQueryFilterBuilder from utils.lambda_exceptions import OdsReportException from utils.request_context import request_context +from utils.utilities import ( + datetime_to_utc_iso_string, + epoch_seconds_to_datetime_utc, + iso_utc_string_to_datetime, +) logger = LoggingService(__name__) @@ -42,31 +49,31 @@ def get_nhs_numbers_by_ods( is_upload_to_s3_needed: bool = False, file_type_output: FileType = FileType.CSV, ): - results = self.query_table_by_index(ods_code) - nhs_numbers = { - item.get(DocumentReferenceMetadataFields.NHS_NUMBER.value) - for item in results - if item.get(DocumentReferenceMetadataFields.NHS_NUMBER.value) - } + document_items = self.query_table_by_index(ods_code) + patient_rows = self.build_patient_rows(document_items) + if is_upload_to_s3_needed: self.temp_output_dir = tempfile.mkdtemp() + return self.create_and_save_ods_report( - ods_code, - nhs_numbers, - is_pre_signed_needed, - is_upload_to_s3_needed, - file_type_output, + ods_code=ods_code, + patient_rows=patient_rows, + create_pre_signed_url=is_pre_signed_needed, + upload_to_s3=is_upload_to_s3_needed, + file_type_output=file_type_output, ) def get_documents_for_review( - self, ods_code: str, output_file_type: FileType = FileType.CSV + self, + ods_code: str, + output_file_type: FileType = FileType.CSV, ): if output_file_type != FileType.CSV: raise OdsReportException(400, LambdaError.UnsupportedFileType) query_filter = self.document_upload_review_service.build_review_dynamo_filter() - results = self.document_upload_review_service.fetch_documents_from_table( + review_rows = self.document_upload_review_service.fetch_documents_from_table( search_key="Custodian", search_condition=ods_code, index_name="CustodianIndex", @@ -75,14 +82,40 @@ def get_documents_for_review( self.temp_output_dir = tempfile.mkdtemp() - return self.create_and_save_ods_report( + return self.create_and_save_review_report( ods_code=ods_code, - data=results, + review_rows=review_rows, create_pre_signed_url=True, upload_to_s3=True, - report_type=ReportType.REVIEW, ) + def create_and_save_review_report( + self, + ods_code: str, + review_rows: list[DocumentUploadReviewReference], + create_pre_signed_url: bool = False, + upload_to_s3: bool = False, + ): + file_name = self.get_file_name_for_report_type( + ReportType.REVIEW, + ods_code, + len(review_rows), + FileType.CSV, + ) + + local_file_path = os.path.join(self.temp_output_dir, file_name) + + self.create_review_csv_report(local_file_path, review_rows) + + logger.info( + f"Query completed. {len(review_rows)} items written to {file_name}.", + ) + + if upload_to_s3: + self.save_report_to_s3(ods_code, file_name, local_file_path) + if create_pre_signed_url: + return self.get_pre_signed_url(ods_code, file_name) + def scan_table_with_filter(self, ods_code: str): ods_codes = [ods_code] authorization_user = getattr(request_context, "authorization", {}) @@ -148,14 +181,15 @@ def query_table_by_index(self, ods_code: str): def create_and_save_ods_report( self, ods_code: str, - data: set[str] | list[DocumentUploadReviewReference], + patient_rows: dict[str, dict[str, Any]], create_pre_signed_url: bool = False, upload_to_s3: bool = False, file_type_output: FileType = FileType.CSV, - report_type: ReportType = ReportType.PATIENT, ): file_name, local_file_path = self.create_ods_report( - ods_code, data, file_type_output, report_type + ods_code=ods_code, + patient_rows=patient_rows, + file_type_output=file_type_output, ) if upload_to_s3: @@ -167,28 +201,26 @@ def create_and_save_ods_report( def create_ods_report( self, ods_code: str, - data: set[str] | list[DocumentUploadReviewReference], + patient_rows: dict[str, dict[str, Any]], file_type_output: FileType = FileType.CSV, - report_type: ReportType = ReportType.PATIENT, ): file_name = self.get_file_name_for_report_type( - report_type, ods_code, len(data), file_type_output + ReportType.PATIENT, + ods_code, + len(patient_rows), + file_type_output, ) local_file_path = os.path.join(self.temp_output_dir, file_name) match file_type_output: case FileType.CSV: - if report_type == ReportType.PATIENT: - self.create_csv_report(local_file_path, data, ods_code) - elif report_type == ReportType.REVIEW: - self.create_review_csv_report(local_file_path, data) + self.create_csv_report(local_file_path, patient_rows, ods_code) case FileType.XLSX: - self.create_xlsx_report(local_file_path, data, ods_code) + self.create_xlsx_report(local_file_path, patient_rows, ods_code) case FileType.PDF: - self.create_pdf_report(local_file_path, data, ods_code) + self.create_pdf_report(local_file_path, patient_rows, ods_code) case _: raise OdsReportException(400, LambdaError.UnsupportedFileType) - logger.info(f"Query completed. {len(data)} items written to {file_name}.") return (file_name, local_file_path) @@ -220,16 +252,10 @@ def get_file_name_for_report_type( return file_name - def create_csv_report(self, file_name: str, nhs_numbers: set[str], ods_code: str): - with open(file_name, "w") as f: - f.write( - f"Total number of patients for ODS code {ods_code}: {len(nhs_numbers)}\n" - ) - f.write("NHS Numbers:\n") - f.writelines(f"{nhs_number}\n" for nhs_number in nhs_numbers) - def create_review_csv_report( - self, file_name: str, data: list[DocumentUploadReviewReference] + self, + file_name: str, + data: list[DocumentUploadReviewReference], ): headers = [ "nhs_number", @@ -243,7 +269,7 @@ def create_review_csv_report( full_line = "" for header in headers: full_line += f"{header}," - full_line = full_line[:-1] # remove the trailing comma + full_line = full_line[:-1] f.write(f"{full_line}\n") for line in data: @@ -258,22 +284,101 @@ def create_review_csv_report( + line.author + "," + upload_date - + "\n" + + "\n", ) - def create_xlsx_report(self, file_name: str, nhs_numbers: set[str], ods_code: str): + def build_patient_rows( + self, + document_items: list[dict], + ) -> dict[str, dict[str, Any]]: + rows: dict[str, dict[str, Any]] = {} + + for item in document_items: + nhs = item.get(DocumentReferenceMetadataFields.NHS_NUMBER.value) + if not nhs: + continue + + created_dt = iso_utc_string_to_datetime( + item.get(DocumentReferenceMetadataFields.CREATED.value), + ) + updated_dt = epoch_seconds_to_datetime_utc( + item.get(DocumentReferenceMetadataFields.LAST_UPDATED.value), + ) + + existing = rows.get(nhs) + if existing is None: + rows[nhs] = { + "nhs_number": nhs, + "created": created_dt, + "last_updated": updated_dt, + } + continue + + if created_dt is not None and ( + existing["created"] is None or created_dt < existing["created"] + ): + existing["created"] = created_dt + + if updated_dt is not None and ( + existing["last_updated"] is None + or updated_dt > existing["last_updated"] + ): + existing["last_updated"] = updated_dt + + return rows + + def create_csv_report( + self, + file_name: str, + patient_rows: dict[str, dict[str, Any]], + ods_code: str, + ): + with open(file_name, "w") as f: + f.write( + f"Total number of patients for ODS code {ods_code}: {len(patient_rows)}\n", + ) + f.write("NHS Numbers:\n") + + f.write("nhs_number,created,last_updated\n") + for nhs in sorted(patient_rows.keys()): + row = patient_rows[nhs] + created = datetime_to_utc_iso_string(row.get("created")) + last_updated = datetime_to_utc_iso_string(row.get("last_updated")) + f.write(f"{nhs},{created},{last_updated}\n") + + def create_xlsx_report( + self, + file_name: str, + patient_rows: dict[str, dict[str, Any]], + ods_code: str, + ): wb = Workbook() ws = wb.active ws["A1"] = ( - f"Total number of patients for ODS code {ods_code}: {len(nhs_numbers)}\n" + f"Total number of patients for ODS code {ods_code}: {len(patient_rows)}\n" ) ws["A2"] = "NHS Numbers:\n" - for row in nhs_numbers: - ws.append([row]) + + ws.append(["nhs_number", "created", "last_updated"]) + + for nhs in sorted(patient_rows.keys()): + row = patient_rows[nhs] + ws.append( + [ + row.get("nhs_number", ""), + datetime_to_utc_iso_string(row.get("created")), + datetime_to_utc_iso_string(row.get("last_updated")), + ], + ) wb.save(file_name) - def create_pdf_report(self, file_name: str, nhs_numbers: set[str], ods_code: str): + def create_pdf_report( + self, + file_name: str, + patient_rows: dict[str, dict[str, Any]], + ods_code: str, + ): c = canvas.Canvas(file_name, pagesize=letter) _, height = letter c.setFont("Helvetica-Bold", 16) @@ -281,17 +386,32 @@ def create_pdf_report(self, file_name: str, nhs_numbers: set[str], ods_code: str y = 700 c.drawString(x, height - 50, f"NHS numbers within NDR for ODS code: {ods_code}") c.setFont("Helvetica", 12) - - c.drawString(x, y, f"Total number of patients: {len(nhs_numbers)}") + c.drawString(x, y, f"Total number of patients: {len(patient_rows)}") y -= 20 - c.drawString(x, y, "NHS Numbers:") + c.drawString(x, y, "NHS Number | Created | Last Updated") y -= 20 - for row in nhs_numbers: + for nhs in sorted(patient_rows.keys()): if y < 40: c.showPage() + c.setFont("Helvetica-Bold", 16) + c.drawString( + x, + height - 50, + f"NHS numbers within NDR for ODS code: {ods_code}", + ) + c.setFont("Helvetica", 12) y = height - 50 - c.drawString(100, y, row) + y -= 40 + c.drawString(x, y, "NHS Number | Created | Last Updated") + y -= 20 + + row = patient_rows[nhs] + created = datetime_to_utc_iso_string(row.get("created")) + last_updated = datetime_to_utc_iso_string(row.get("last_updated")) + + line = f"{nhs} | {created} | {last_updated}" + c.drawString(x, y, line[:120]) y -= 20 c.save() diff --git a/lambdas/tests/unit/services/test_ods_report_service.py b/lambdas/tests/unit/services/test_ods_report_service.py index 2740f83278..c0601fbd45 100644 --- a/lambdas/tests/unit/services/test_ods_report_service.py +++ b/lambdas/tests/unit/services/test_ods_report_service.py @@ -4,6 +4,10 @@ from unittest.mock import call import pytest +from freezegun import freeze_time +from openpyxl.reader.excel import load_workbook +from pypdf import PdfReader + from enums.document_review_reason import DocumentReviewReason from enums.document_review_status import DocumentReviewStatus from enums.dynamo_filter import AttributeOperator @@ -11,13 +15,10 @@ from enums.lambda_error import LambdaError from enums.metadata_field_names import DocumentReferenceMetadataFields from enums.patient_ods_inactive_status import PatientOdsInactiveStatus -from freezegun import freeze_time from models.document_review import ( DocumentReviewFileDetails, DocumentUploadReviewReference, ) -from openpyxl.reader.excel import load_workbook -from pypdf import PdfReader from services.ods_report_service import OdsReportService from utils.common_query_filters import NotDeleted from utils.dynamo_query_filter_builder import DynamoQueryFilterBuilder @@ -53,7 +54,8 @@ def mocked_pcse_context(mocker): "repository_role": "PCSE", } yield mocker.patch( - "services.ods_report_service.request_context", mocked_pcse_context + "services.ods_report_service.request_context", + mocked_pcse_context, ) @@ -67,8 +69,9 @@ def mock_review_result(): upload_date=int(datetime.now().timestamp()), files=[ DocumentReviewFileDetails( - file_name="mock_file_name", file_location="mock_file_location" - ) + file_name="mock_file_name", + file_location="mock_file_location", + ), ], custodian="mock_custodian", ) @@ -125,40 +128,76 @@ def mock_create_review_csv_report(mocker, ods_report_service): def test_get_nhs_numbers_by_ods( - ods_report_service, mock_query_table_by_index, mock_create_and_save_ods_report + ods_report_service, + mock_query_table_by_index, + mock_create_and_save_ods_report, + mocker, ): mock_query_table_by_index.return_value = [ {DocumentReferenceMetadataFields.NHS_NUMBER.value: "NHS123"}, {DocumentReferenceMetadataFields.NHS_NUMBER.value: "NHS456"}, ] + patient_rows = { + "NHS123": {"nhs_number": "NHS123", "created": None, "last_updated": None}, + "NHS456": {"nhs_number": "NHS456", "created": None, "last_updated": None}, + } + mocker.patch.object( + ods_report_service, + "build_patient_rows", + return_value=patient_rows, + ) + ods_report_service.get_nhs_numbers_by_ods("ODS123") mock_query_table_by_index.assert_called_once_with("ODS123") mock_create_and_save_ods_report.assert_called_once_with( - "ODS123", {"NHS123", "NHS456"}, False, False, "csv" + ods_code="ODS123", + patient_rows=patient_rows, + create_pre_signed_url=False, + upload_to_s3=False, + file_type_output=FileType.CSV, ) def test_get_nhs_numbers_by_ods_with_temp_folder( - ods_report_service, mock_query_table_by_index, mock_create_and_save_ods_report + ods_report_service, + mock_query_table_by_index, + mock_create_and_save_ods_report, + mocker, ): mock_query_table_by_index.return_value = [ {DocumentReferenceMetadataFields.NHS_NUMBER.value: "NHS123"}, {DocumentReferenceMetadataFields.NHS_NUMBER.value: "NHS456"}, ] + patient_rows = { + "NHS123": {"nhs_number": "NHS123", "created": None, "last_updated": None}, + "NHS456": {"nhs_number": "NHS456", "created": None, "last_updated": None}, + } + mocker.patch.object( + ods_report_service, + "build_patient_rows", + return_value=patient_rows, + ) + ods_report_service.get_nhs_numbers_by_ods("ODS123", is_upload_to_s3_needed=True) mock_query_table_by_index.assert_called_once_with("ODS123") mock_create_and_save_ods_report.assert_called_once_with( - "ODS123", {"NHS123", "NHS456"}, False, True, "csv" + ods_code="ODS123", + patient_rows=patient_rows, + create_pre_signed_url=False, + upload_to_s3=True, + file_type_output=FileType.CSV, ) assert ods_report_service.temp_output_dir != "" def test_scan_table_with_filter( - ods_report_service, mocked_context, mock_dynamo_service_scan_table + ods_report_service, + mocked_context, + mock_dynamo_service_scan_table, ): mock_dynamo_service_scan_table.return_value = [ {DocumentReferenceMetadataFields.NHS_NUMBER.value: "NHS123"}, @@ -173,7 +212,9 @@ def test_scan_table_with_filter( def test_scan_table_with_filter_no_results( - ods_report_service, mocked_context, mock_dynamo_service_scan_table + ods_report_service, + mocked_context, + mock_dynamo_service_scan_table, ): mock_dynamo_service_scan_table.return_value = [] @@ -187,22 +228,31 @@ def test_create_and_save_ods_report_create_csv( mock_create_report_csv, mock_save_report_to_s3, mock_get_pre_signed_url, + mocker, ): ods_code = "ODS123" - nhs_numbers = {"NHS123", "NHS456"} + patient_rows = { + "NHS123": {"nhs_number": "NHS123", "created": None, "last_updated": None}, + "NHS456": {"nhs_number": "NHS456", "created": None, "last_updated": None}, + } file_name = "LloydGeorgeSummary_ODS123_2_2024-01-01_12-00.csv" temp_file_path = os.path.join(ods_report_service.temp_output_dir, file_name) - result = ods_report_service.create_and_save_ods_report( - ods_code, nhs_numbers, upload_to_s3=True, file_type_output=FileType.CSV + mocker.patch.object( + ods_report_service, + "create_ods_report", + return_value=(file_name, temp_file_path), ) - mock_create_report_csv.assert_called_once_with( - temp_file_path, nhs_numbers, ods_code + result = ods_report_service.create_and_save_ods_report( + ods_code, + patient_rows, + upload_to_s3=True, + file_type_output=FileType.CSV, ) + mock_save_report_to_s3.assert_called_once_with(ods_code, file_name, temp_file_path) mock_get_pre_signed_url.assert_not_called() - assert result is None @@ -212,19 +262,29 @@ def test_create_and_save_ods_report_create_pdf( mock_create_report_pdf, mock_save_report_to_s3, mock_get_pre_signed_url, + mocker, ): ods_code = "ODS123" - nhs_numbers = {"NHS123", "NHS456"} + patient_rows = { + "NHS123": {"nhs_number": "NHS123", "created": None, "last_updated": None}, + "NHS456": {"nhs_number": "NHS456", "created": None, "last_updated": None}, + } file_name = "LloydGeorgeSummary_ODS123_2_2024-01-01_12-00.pdf" temp_file_path = os.path.join(ods_report_service.temp_output_dir, file_name) - ods_report_service.create_and_save_ods_report( - ods_code, nhs_numbers, upload_to_s3=True, file_type_output=FileType.PDF + mocker.patch.object( + ods_report_service, + "create_ods_report", + return_value=(file_name, temp_file_path), ) - mock_create_report_pdf.assert_called_once_with( - temp_file_path, nhs_numbers, ods_code + ods_report_service.create_and_save_ods_report( + ods_code, + patient_rows, + upload_to_s3=True, + file_type_output=FileType.PDF, ) + mock_save_report_to_s3.assert_called_once_with(ods_code, file_name, temp_file_path) mock_get_pre_signed_url.assert_not_called() @@ -235,19 +295,29 @@ def test_create_and_save_ods_report_create_xlsx( mock_create_report_xlsx, mock_save_report_to_s3, mock_get_pre_signed_url, + mocker, ): ods_code = "ODS123" - nhs_numbers = {"NHS123", "NHS456"} + patient_rows = { + "NHS123": {"nhs_number": "NHS123", "created": None, "last_updated": None}, + "NHS456": {"nhs_number": "NHS456", "created": None, "last_updated": None}, + } file_name = "LloydGeorgeSummary_ODS123_2_2024-01-01_12-00.xlsx" temp_file_path = os.path.join(ods_report_service.temp_output_dir, file_name) - ods_report_service.create_and_save_ods_report( - ods_code, nhs_numbers, upload_to_s3=True, file_type_output=FileType.XLSX + mocker.patch.object( + ods_report_service, + "create_ods_report", + return_value=(file_name, temp_file_path), ) - mock_create_report_xlsx.assert_called_once_with( - temp_file_path, nhs_numbers, ods_code + ods_report_service.create_and_save_ods_report( + ods_code, + patient_rows, + upload_to_s3=True, + file_type_output=FileType.XLSX, ) + mock_save_report_to_s3.assert_called_once_with(ods_code, file_name, temp_file_path) mock_get_pre_signed_url.assert_not_called() @@ -259,11 +329,15 @@ def test_create_and_save_ods_report_send_invalid_file_type( mock_get_pre_signed_url, ): ods_code = "ODS123" - nhs_numbers = {"NHS123", "NHS456"} + patient_rows = { + "NHS123": {"nhs_number": "NHS123", "created": None, "last_updated": None}, + "NHS456": {"nhs_number": "NHS456", "created": None, "last_updated": None}, + } + with pytest.raises(OdsReportException): ods_report_service.create_and_save_ods_report( ods_code, - nhs_numbers, + patient_rows, upload_to_s3=True, create_pre_signed_url=True, file_type_output="invalid", @@ -280,92 +354,109 @@ def test_create_and_save_ods_report_with_pre_sign_url( mock_create_report_csv, mock_save_report_to_s3, mock_get_pre_signed_url, + mocker, ): ods_code = "ODS123" - nhs_numbers = {"NHS123", "NHS456"} + patient_rows = { + "NHS123": {"nhs_number": "NHS123", "created": None, "last_updated": None}, + "NHS456": {"nhs_number": "NHS456", "created": None, "last_updated": None}, + } file_name = "LloydGeorgeSummary_ODS123_2_2024-01-01_12-00.csv" mock_pre_sign_url = "https://presigned.url" + temp_file_path = os.path.join(ods_report_service.temp_output_dir, file_name) mock_get_pre_signed_url.return_value = mock_pre_sign_url - temp_file_path = os.path.join(ods_report_service.temp_output_dir, file_name) + mocker.patch.object( + ods_report_service, + "create_ods_report", + return_value=(file_name, temp_file_path), + ) result = ods_report_service.create_and_save_ods_report( - ods_code, nhs_numbers, True, True + ods_code, + patient_rows, + True, + True, + FileType.CSV, ) - mock_create_report_csv.assert_called_once_with( - temp_file_path, nhs_numbers, ods_code - ) mock_save_report_to_s3.assert_called_once_with(ods_code, file_name, temp_file_path) mock_get_pre_signed_url.assert_called_once_with(ods_code, file_name) assert result == mock_pre_sign_url def test_create_report_csv(ods_report_service, tmp_path): - nhs_numbers = {"NHS123", "NHS456"} + patient_rows = { + "NHS123": {"nhs_number": "NHS123", "created": None, "last_updated": None}, + "NHS456": {"nhs_number": "NHS456", "created": None, "last_updated": None}, + } file_name = tmp_path / "test_report.csv" ods_code = "ODS123" - ods_report_service.create_csv_report(str(file_name), nhs_numbers, ods_code) + ods_report_service.create_csv_report(str(file_name), patient_rows, ods_code) with open(file_name, "r") as f: content = f.readlines() assert ( - f"Total number of patients for ODS code {ods_code}: {len(nhs_numbers)}\n" + f"Total number of patients for ODS code {ods_code}: {len(patient_rows)}\n" in content ) assert "NHS Numbers:\n" in content - assert "NHS123\n" in content - assert "NHS456\n" in content + assert "nhs_number,created,last_updated\n" in content def test_create_xlsx_report(ods_report_service, tmp_path): - file_name = "test_report.xlsx" - nhs_numbers = {"NHS123456", "NHS654321", "NHS111222"} + file_name = tmp_path / "test_report.xlsx" + patient_rows = { + "NHS123456": {"nhs_number": "NHS123456", "created": None, "last_updated": None}, + "NHS654321": {"nhs_number": "NHS654321", "created": None, "last_updated": None}, + "NHS111222": {"nhs_number": "NHS111222", "created": None, "last_updated": None}, + } ods_code = "ODS123" - ods_report_service.create_xlsx_report(file_name, nhs_numbers, ods_code) + ods_report_service.create_xlsx_report(str(file_name), patient_rows, ods_code) assert os.path.exists(file_name) - wb = load_workbook(file_name) + wb = load_workbook(str(file_name)) ws = wb.active assert ( ws["A1"].value - == f"Total number of patients for ODS code {ods_code}: {len(nhs_numbers)}\n" + == f"Total number of patients for ODS code {ods_code}: {len(patient_rows)}\n" ) assert ws["A2"].value == "NHS Numbers:\n" - for i, nhs_number in enumerate(nhs_numbers, start=3): # Start from row 3 + nhs_sorted = sorted(patient_rows.keys()) + for i, nhs_number in enumerate(nhs_sorted, start=4): assert ws.cell(row=i, column=1).value == nhs_number - os.remove(file_name) - @freeze_time("2024-01-01T12:00:00Z") -def test_create_pdf_report(ods_report_service): - file_name = "test_report.pdf" - nhs_numbers = {"NHS123456", "NHS654321", "NHS111222"} +def test_create_pdf_report(ods_report_service, tmp_path): + file_name = tmp_path / "test_report.pdf" + patient_rows = { + "NHS123456": {"nhs_number": "NHS123456", "created": None, "last_updated": None}, + "NHS654321": {"nhs_number": "NHS654321", "created": None, "last_updated": None}, + "NHS111222": {"nhs_number": "NHS111222", "created": None, "last_updated": None}, + } ods_code = "ODS123" - ods_report_service.create_pdf_report(file_name, nhs_numbers, ods_code) + ods_report_service.create_pdf_report(str(file_name), patient_rows, ods_code) assert os.path.exists(file_name) - reader = PdfReader(file_name) + reader = PdfReader(str(file_name)) assert len(reader.pages) > 0 - first_page = reader.pages[0].extract_text() + first_page = reader.pages[0].extract_text() or "" assert f"NHS numbers within NDR for ODS code: {ods_code}" in first_page - assert f"Total number of patients: {len(nhs_numbers)}" in first_page - for nhs_number in nhs_numbers: + assert f"Total number of patients: {len(patient_rows)}" in first_page + for nhs_number in patient_rows.keys(): assert nhs_number in first_page - os.remove(file_name) - @freeze_time("2024-01-01T12:00:00Z") def test_save_report_to_s3(ods_report_service, mocker): @@ -403,14 +494,15 @@ def test_get_documents_for_review( mocker, ): expected_result = "https://example.com/mocked" - mock_get_pre_signed_url.return_value = "https://example.com/mocked" + mock_get_pre_signed_url.return_value = expected_result mock_ods_code = "ODS123" query_builder = DynamoQueryFilterBuilder() query_builder.add_condition( - "ReviewStatus", AttributeOperator.EQUAL, DocumentReviewStatus.PENDING_REVIEW + "ReviewStatus", + AttributeOperator.EQUAL, + DocumentReviewStatus.PENDING_REVIEW, ) - expected_query_filter = query_builder.build() mocker.patch.object( @@ -425,7 +517,7 @@ def test_get_documents_for_review( return_value=expected_query_filter, ) - result = ods_report_service.get_documents_for_review(mock_ods_code, "csv") + result = ods_report_service.get_documents_for_review(mock_ods_code, FileType.CSV) assert result == expected_result @@ -441,9 +533,8 @@ def test_get_documents_for_review( def test_get_documents_for_review_unsupported_file_type(ods_report_service): - with pytest.raises(OdsReportException) as excinfo: - ods_report_service.get_documents_for_review("mock_ods_code", "pdf") + ods_report_service.get_documents_for_review("mock_ods_code", FileType.PDF) assert excinfo.value.error == LambdaError.UnsupportedFileType assert excinfo.value.status_code == 400 @@ -466,22 +557,6 @@ def test_create_review_csv_report(ods_report_service, mock_review_result, tmp_pa == "nhs_number,review_reason,document_snomed_code_type,author,upload_date\n" ) - for i in range(1, len(content)): - assert content[i] == ( - mock_review_result.nhs_number - + "," - + mock_review_result.review_reason - + "," - + mock_review_result.document_snomed_code_type - + "," - + mock_review_result.author - + "," - + datetime.fromtimestamp(mock_review_result.upload_date).isoformat() - + "\n" - ) - - os.remove(file_name) - def test_query_table_by_index(ods_report_service, mocked_context, set_env): mock_dynamo_results = [{"mock_database_field": "mock_database_value"}] @@ -503,7 +578,9 @@ def test_query_table_by_index(ods_report_service, mocked_context, set_env): def test_query_table_by_index_pcse_user( - ods_report_service, mocked_pcse_context, set_env + ods_report_service, + mocked_pcse_context, + set_env, ): mock_dynamo_results = [{"mock_database_field": "mock_database_value"}] mock_ods_code = "ODS123" @@ -532,7 +609,7 @@ def test_query_table_by_index_pcse_user( search_condition=PatientOdsInactiveStatus.DECEASED, query_filter=NotDeleted, ), - ] + ], ) @@ -552,40 +629,43 @@ def test_create_and_save_ods_report_dont_save_to_s3( ods_report_service, mock_save_report_to_s3, mock_get_pre_signed_url, - mock_create_report_csv, + mocker, ): + mocker.patch.object( + ods_report_service, + "create_ods_report", + return_value=("x.csv", "/tmp/x.csv"), + ) + ods_report_service.create_and_save_ods_report( ods_code="ODS123", - data=["mock_nhs_number"], + patient_rows={ + "mock_nhs_number": { + "nhs_number": "mock_nhs_number", + "created": None, + "last_updated": None, + }, + }, create_pre_signed_url=False, upload_to_s3=False, file_type_output=FileType.CSV, ) - mock_create_report_csv.assert_called_once() - mock_save_report_to_s3.assert_not_called() mock_get_pre_signed_url.assert_not_called() -def test_create_pdf_report_multiple_pages(ods_report_service): - file_name = "test_report.pdf" - nhs_numbers = {"NHS123456", "NHS654321", "NHS111222"} - ods_code = "ODS123" +def test_create_pdf_report_multiple_pages(ods_report_service, tmp_path): + file_name = tmp_path / "test_report.pdf" + patient_rows = {} - for i in range(1, 100): - nhs_numbers.add(f"NHS123456{str(i)}") + for i in range(1, 150): + nhs = f"NHS123456{str(i)}" + patient_rows[nhs] = {"nhs_number": nhs, "created": None, "last_updated": None} - ods_report_service.create_pdf_report(file_name, nhs_numbers, ods_code) + ods_report_service.create_pdf_report(str(file_name), patient_rows, "ODS123") assert os.path.exists(file_name) - reader = PdfReader(file_name) + reader = PdfReader(str(file_name)) assert len(reader.pages) > 1 - - first_page = reader.pages[0].extract_text() - - assert f"NHS numbers within NDR for ODS code: {ods_code}" in first_page - assert f"Total number of patients: {len(nhs_numbers)}" in first_page - - os.remove(file_name) diff --git a/lambdas/tests/unit/utils/test_utilities.py b/lambdas/tests/unit/utils/test_utilities.py index 04c45d4dcd..5d01bfe187 100755 --- a/lambdas/tests/unit/utils/test_utilities.py +++ b/lambdas/tests/unit/utils/test_utilities.py @@ -1,15 +1,19 @@ -from datetime import datetime +from datetime import datetime, timedelta, timezone import pytest + from services.mock_pds_service import MockPdsApiService from services.pds_api_service import PdsApiService from utils.exceptions import InvalidNhsNumberException from utils.utilities import ( camelize_dict, + datetime_to_utc_iso_string, + epoch_seconds_to_datetime_utc, flatten, format_cloudfront_url, get_file_key_from_s3_url, get_pds_service, + iso_utc_string_to_datetime, parse_date, redact_id_to_last_4_chars, utc_date_string, @@ -153,3 +157,87 @@ def test_utc_date_string_returns_correct_utc_date( expected_date_string, ): assert utc_date_string(timestamp_seconds) == expected_date_string + + +@pytest.mark.parametrize( + "value", + [ + None, + "", + " ", + ], +) +def test_iso_utc_string_to_datetime_returns_none_for_empty_or_none(value): + assert iso_utc_string_to_datetime(value) is None + + +def test_iso_utc_string_to_datetime_parses_z_suffix(): + result = iso_utc_string_to_datetime("2025-03-11T16:26:44.520811Z") + + assert result == datetime(2025, 3, 11, 16, 26, 44, 520811, tzinfo=timezone.utc) + + +def test_iso_utc_string_to_datetime_normalises_offset_to_utc(): + result = iso_utc_string_to_datetime("2025-03-11T18:26:44+02:00") + + assert result == datetime(2025, 3, 11, 16, 26, 44, tzinfo=timezone.utc) + + +def test_iso_utc_string_to_datetime_naive_datetime_assumes_utc(): + result = iso_utc_string_to_datetime("2025-03-11T16:26:44") + + assert result == datetime(2025, 3, 11, 16, 26, 44, tzinfo=timezone.utc) + + +def test_iso_utc_string_to_datetime_invalid_string_returns_none(): + assert iso_utc_string_to_datetime("not-a-date") is None + + +@pytest.mark.parametrize( + "value, expected", + [ + (0, datetime(1970, 1, 1, tzinfo=timezone.utc)), + ("1704067200", datetime(2024, 1, 1, tzinfo=timezone.utc)), + ], +) +def test_epoch_seconds_to_datetime_utc_valid(value, expected): + assert epoch_seconds_to_datetime_utc(value) == expected + + +@pytest.mark.parametrize( + "value", + [ + None, + "abc", + "123.45", + {}, + [], + ], +) +def test_epoch_seconds_to_datetime_utc_invalid_returns_none(value): + assert epoch_seconds_to_datetime_utc(value) is None + + +def test_datetime_to_utc_iso_string_none_returns_empty_string(): + assert datetime_to_utc_iso_string(None) == "" + + +def test_datetime_to_utc_iso_string_naive_datetime_assumes_utc(): + dt = datetime(2024, 1, 1, 12, 34, 56, 999999) + + assert datetime_to_utc_iso_string(dt) == "2024-01-01T12:34:56.999999" + + +def test_datetime_to_utc_iso_string_converts_timezone_and_drops_microseconds(): + dt = datetime( + 2024, + 1, + 1, + 12, + 0, + 0, + 123456, + tzinfo=timezone(timedelta(hours=2)), + ) + + assert datetime_to_utc_iso_string(dt) == "2024-01-01T10:00:00.123456" diff --git a/lambdas/utils/utilities.py b/lambdas/utils/utilities.py index 17bc899180..5648cd7b28 100755 --- a/lambdas/utils/utilities.py +++ b/lambdas/utils/utilities.py @@ -1,3 +1,4 @@ +# lambdas/utils/utilities.py import itertools import os import re @@ -6,6 +7,7 @@ from urllib.parse import urlparse from inflection import camelize + from services.base.nhs_oauth_service import NhsOauthService from services.base.ssm_service import SSMService from services.mock_pds_service import MockPdsApiService @@ -145,3 +147,89 @@ def utc_day_start_timestamp(day: date) -> int: def utc_day_end_timestamp(day: date) -> int: return utc_day_start_timestamp(day) + 24 * 60 * 60 - 1 + + +ISO_UTC_SUFFIX = "Z" + + +def iso_utc_string_to_datetime(value: str | None) -> datetime | None: + """ + Convert an ISO-8601 UTC string (ending with 'Z') to a timezone-aware datetime. + + Examples: + "2025-03-11T16:26:44.520811Z" -> datetime(2025, 3, 11, 16, 26, 44, tzinfo=UTC) + None -> None + """ + if value is None: + return None + + value = value.strip() + if not value: + return None + + try: + if value.endswith(ISO_UTC_SUFFIX): + value = value[:-1] + "+00:00" + + dt = datetime.fromisoformat(value) + + if dt.tzinfo is None: + return dt.replace(tzinfo=timezone.utc) + + return dt.astimezone(timezone.utc) + + except ValueError: + return None + + +def epoch_seconds_to_datetime_utc(value: int | str | None) -> datetime | None: + """ + Convert epoch seconds to a UTC datetime. + + Accepts: + - int (epoch seconds) + - str containing digits (epoch seconds) + - None + + Returns: + - timezone-aware UTC datetime, or None + """ + if value is None: + return None + + try: + seconds = int(value) + except (TypeError, ValueError): + return None + + try: + return datetime.fromtimestamp(seconds, tz=timezone.utc) + except (OverflowError, OSError): + return None + + +def datetime_to_utc_iso_string(value: datetime | None) -> str: + """ + Convert a datetime to an ISO-8601 string matching create_review_csv_report output. + + Accepts: + - datetime (naive or timezone-aware) + - None + + Behaviour: + - If timezone-aware, the datetime is converted to UTC and made naive + - If naive, it is used as-is + - Microseconds are preserved + - No timezone suffix is included + + Returns: + - ISO-8601 formatted string (e.g. "2025-03-11T16:26:44.520811") + - Empty string if input is None + """ + if value is None: + return "" + + if value.tzinfo is not None: + value = value.astimezone(timezone.utc).replace(tzinfo=None) + + return value.isoformat() From 410bcb1520be32a4aee1e1625198a8844667fdf5 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Wed, 25 Feb 2026 13:56:09 +0000 Subject: [PATCH 02/15] [PRMP-1505] small fixes --- lambdas/services/ods_report_service.py | 12 +++++++----- .../unit/services/test_ods_report_service.py | 4 +--- lambdas/tests/unit/utils/test_utilities.py | 4 ++-- lambdas/utils/utilities.py | 16 ++++++++-------- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/lambdas/services/ods_report_service.py b/lambdas/services/ods_report_service.py index 6507cbe7b3..89b1e5933b 100644 --- a/lambdas/services/ods_report_service.py +++ b/lambdas/services/ods_report_service.py @@ -308,9 +308,9 @@ def build_patient_rows( existing = rows.get(nhs) if existing is None: rows[nhs] = { - "nhs_number": nhs, - "created": created_dt, - "last_updated": updated_dt, + "NHS number": nhs, + "Created": created_dt, + "Last updated": updated_dt, } continue @@ -337,7 +337,6 @@ def create_csv_report( f.write( f"Total number of patients for ODS code {ods_code}: {len(patient_rows)}\n", ) - f.write("NHS Numbers:\n") f.write("nhs_number,created,last_updated\n") for nhs in sorted(patient_rows.keys()): @@ -357,7 +356,6 @@ def create_xlsx_report( ws["A1"] = ( f"Total number of patients for ODS code {ods_code}: {len(patient_rows)}\n" ) - ws["A2"] = "NHS Numbers:\n" ws.append(["nhs_number", "created", "last_updated"]) @@ -371,6 +369,10 @@ def create_xlsx_report( ], ) + ws.column_dimensions["A"].width = 14 + ws.column_dimensions["B"].width = 20 + ws.column_dimensions["C"].width = 20 + wb.save(file_name) def create_pdf_report( diff --git a/lambdas/tests/unit/services/test_ods_report_service.py b/lambdas/tests/unit/services/test_ods_report_service.py index c0601fbd45..221b2b9167 100644 --- a/lambdas/tests/unit/services/test_ods_report_service.py +++ b/lambdas/tests/unit/services/test_ods_report_service.py @@ -402,7 +402,6 @@ def test_create_report_csv(ods_report_service, tmp_path): f"Total number of patients for ODS code {ods_code}: {len(patient_rows)}\n" in content ) - assert "NHS Numbers:\n" in content assert "nhs_number,created,last_updated\n" in content @@ -426,10 +425,9 @@ def test_create_xlsx_report(ods_report_service, tmp_path): ws["A1"].value == f"Total number of patients for ODS code {ods_code}: {len(patient_rows)}\n" ) - assert ws["A2"].value == "NHS Numbers:\n" nhs_sorted = sorted(patient_rows.keys()) - for i, nhs_number in enumerate(nhs_sorted, start=4): + for i, nhs_number in enumerate(nhs_sorted, start=3): assert ws.cell(row=i, column=1).value == nhs_number diff --git a/lambdas/tests/unit/utils/test_utilities.py b/lambdas/tests/unit/utils/test_utilities.py index 5d01bfe187..777b01c275 100755 --- a/lambdas/tests/unit/utils/test_utilities.py +++ b/lambdas/tests/unit/utils/test_utilities.py @@ -225,7 +225,7 @@ def test_datetime_to_utc_iso_string_none_returns_empty_string(): def test_datetime_to_utc_iso_string_naive_datetime_assumes_utc(): dt = datetime(2024, 1, 1, 12, 34, 56, 999999) - assert datetime_to_utc_iso_string(dt) == "2024-01-01T12:34:56.999999" + assert datetime_to_utc_iso_string(dt) == "2024-01-01T12:34:56" def test_datetime_to_utc_iso_string_converts_timezone_and_drops_microseconds(): @@ -240,4 +240,4 @@ def test_datetime_to_utc_iso_string_converts_timezone_and_drops_microseconds(): tzinfo=timezone(timedelta(hours=2)), ) - assert datetime_to_utc_iso_string(dt) == "2024-01-01T10:00:00.123456" + assert datetime_to_utc_iso_string(dt) == "2024-01-01T10:00:00" diff --git a/lambdas/utils/utilities.py b/lambdas/utils/utilities.py index 5648cd7b28..5ecc864828 100755 --- a/lambdas/utils/utilities.py +++ b/lambdas/utils/utilities.py @@ -210,21 +210,21 @@ def epoch_seconds_to_datetime_utc(value: int | str | None) -> datetime | None: def datetime_to_utc_iso_string(value: datetime | None) -> str: """ - Convert a datetime to an ISO-8601 string matching create_review_csv_report output. + Convert a datetime to an ISO-8601 string with second-level precision. Accepts: - datetime (naive or timezone-aware) - None Behaviour: - - If timezone-aware, the datetime is converted to UTC and made naive - - If naive, it is used as-is - - Microseconds are preserved - - No timezone suffix is included + - If the datetime is timezone-aware, it is converted to UTC and made naive + - If the datetime is naive, it is used as-is + - Microseconds are discarded + - No timezone suffix (e.g. 'Z') is included Returns: - - ISO-8601 formatted string (e.g. "2025-03-11T16:26:44.520811") - - Empty string if input is None + - ISO-8601 formatted string: "YYYY-MM-DDTHH:MM:SS" + - Empty string ("") if input is None """ if value is None: return "" @@ -232,4 +232,4 @@ def datetime_to_utc_iso_string(value: datetime | None) -> str: if value.tzinfo is not None: value = value.astimezone(timezone.utc).replace(tzinfo=None) - return value.isoformat() + return value.replace(microsecond=0).isoformat(timespec="seconds") From a4cb5edb434b89f7a37af84bb3c5597a9fc79365 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Wed, 25 Feb 2026 14:45:45 +0000 Subject: [PATCH 03/15] [PRMP-1505] small fixes --- lambdas/services/ods_report_service.py | 12 ++++++------ .../tests/unit/services/test_ods_report_service.py | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lambdas/services/ods_report_service.py b/lambdas/services/ods_report_service.py index 89b1e5933b..7430e54acc 100644 --- a/lambdas/services/ods_report_service.py +++ b/lambdas/services/ods_report_service.py @@ -308,9 +308,9 @@ def build_patient_rows( existing = rows.get(nhs) if existing is None: rows[nhs] = { - "NHS number": nhs, - "Created": created_dt, - "Last updated": updated_dt, + "nhs_number": nhs, + "created": created_dt, + "last_updated": updated_dt, } continue @@ -338,7 +338,7 @@ def create_csv_report( f"Total number of patients for ODS code {ods_code}: {len(patient_rows)}\n", ) - f.write("nhs_number,created,last_updated\n") + f.write("NHS number,Created,Last updated\n") for nhs in sorted(patient_rows.keys()): row = patient_rows[nhs] created = datetime_to_utc_iso_string(row.get("created")) @@ -357,7 +357,7 @@ def create_xlsx_report( f"Total number of patients for ODS code {ods_code}: {len(patient_rows)}\n" ) - ws.append(["nhs_number", "created", "last_updated"]) + ws.append(["NHS number", "Created", "Last updated"]) for nhs in sorted(patient_rows.keys()): row = patient_rows[nhs] @@ -390,7 +390,7 @@ def create_pdf_report( c.setFont("Helvetica", 12) c.drawString(x, y, f"Total number of patients: {len(patient_rows)}") y -= 20 - c.drawString(x, y, "NHS Number | Created | Last Updated") + c.drawString(x, y, "NHS number | Created | Last updated") y -= 20 for nhs in sorted(patient_rows.keys()): if y < 40: diff --git a/lambdas/tests/unit/services/test_ods_report_service.py b/lambdas/tests/unit/services/test_ods_report_service.py index 221b2b9167..51cacd6487 100644 --- a/lambdas/tests/unit/services/test_ods_report_service.py +++ b/lambdas/tests/unit/services/test_ods_report_service.py @@ -402,7 +402,7 @@ def test_create_report_csv(ods_report_service, tmp_path): f"Total number of patients for ODS code {ods_code}: {len(patient_rows)}\n" in content ) - assert "nhs_number,created,last_updated\n" in content + assert "NHS number,Created,Last updated\n" in content def test_create_xlsx_report(ods_report_service, tmp_path): From 2aad23adec05e5f075ed1f9ed63962c97f34f7ef Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Thu, 26 Feb 2026 10:07:08 +0000 Subject: [PATCH 04/15] [PRMP-1505] added extra unit tests --- .../unit/services/test_ods_report_service.py | 83 ++++++++++++++++++- 1 file changed, 82 insertions(+), 1 deletion(-) diff --git a/lambdas/tests/unit/services/test_ods_report_service.py b/lambdas/tests/unit/services/test_ods_report_service.py index 51cacd6487..a3e5b8c23e 100644 --- a/lambdas/tests/unit/services/test_ods_report_service.py +++ b/lambdas/tests/unit/services/test_ods_report_service.py @@ -1,6 +1,6 @@ import os import tempfile -from datetime import datetime +from datetime import datetime, timezone from unittest.mock import call import pytest @@ -667,3 +667,84 @@ def test_create_pdf_report_multiple_pages(ods_report_service, tmp_path): reader = PdfReader(str(file_name)) assert len(reader.pages) > 1 + + + +def test_build_patient_rows_skips_items_without_nhs_number(ods_report_service): + items = [ + { + DocumentReferenceMetadataFields.CREATED.value: "2026-02-25T12:50:35.000000Z", + DocumentReferenceMetadataFields.LAST_UPDATED.value: 1700000000, + }, + { + DocumentReferenceMetadataFields.NHS_NUMBER.value: "9730786917", + DocumentReferenceMetadataFields.CREATED.value: "2026-02-25T12:50:35.000000Z", + DocumentReferenceMetadataFields.LAST_UPDATED.value: 1700000001, + }, + ] + + rows = ods_report_service.build_patient_rows(items) + + assert list(rows.keys()) == ["9730786917"] + assert rows["9730786917"]["nhs_number"] == "9730786917" + + +def test_build_patient_rows_dedupes_and_picks_earliest_created_and_latest_last_updated( + ods_report_service, +): + nhs = "9730786917" + + items = [ + { + DocumentReferenceMetadataFields.NHS_NUMBER.value: nhs, + DocumentReferenceMetadataFields.CREATED.value: "2026-02-25T12:50:40.000000Z", + DocumentReferenceMetadataFields.LAST_UPDATED.value: 1700000010, + }, + { + DocumentReferenceMetadataFields.NHS_NUMBER.value: nhs, + DocumentReferenceMetadataFields.CREATED.value: "2026-02-25T12:50:35.000000Z", + DocumentReferenceMetadataFields.LAST_UPDATED.value: 1700000020, + }, + { + DocumentReferenceMetadataFields.NHS_NUMBER.value: "9730786933", + DocumentReferenceMetadataFields.CREATED.value: "2026-02-25T12:50:36.000000Z", + DocumentReferenceMetadataFields.LAST_UPDATED.value: 1700000030, + }, + ] + + rows = ods_report_service.build_patient_rows(items) + + assert set(rows.keys()) == {nhs, "9730786933"} + + expected_created = datetime(2026, 2, 25, 12, 50, 35, tzinfo=timezone.utc) + expected_last_updated = datetime.fromtimestamp(1700000020, tz=timezone.utc) + + assert rows[nhs]["created"] == expected_created + assert rows[nhs]["last_updated"] == expected_last_updated + + +def test_build_patient_rows_does_not_overwrite_existing_values_with_none( + ods_report_service, +): + nhs = "9730786917" + + items = [ + { + DocumentReferenceMetadataFields.NHS_NUMBER.value: nhs, + DocumentReferenceMetadataFields.CREATED.value: "2026-02-25T12:50:35.000000Z", + DocumentReferenceMetadataFields.LAST_UPDATED.value: 1700000010, + }, + { + DocumentReferenceMetadataFields.NHS_NUMBER.value: nhs, + DocumentReferenceMetadataFields.CREATED.value: None, + DocumentReferenceMetadataFields.LAST_UPDATED.value: None, + }, + ] + + rows = ods_report_service.build_patient_rows(items) + + expected_created = datetime(2026, 2, 25, 12, 50, 35, tzinfo=timezone.utc) + expected_last_updated = datetime.fromtimestamp(1700000010, tz=timezone.utc) + + assert rows[nhs]["created"] == expected_created + assert rows[nhs]["last_updated"] == expected_last_updated \ No newline at end of file From 4c9474b8f5275c5bf7cf049a350779e7955c7635 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Thu, 26 Feb 2026 10:09:45 +0000 Subject: [PATCH 05/15] [PRMP-1505] format --- lambdas/tests/unit/services/test_ods_report_service.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lambdas/tests/unit/services/test_ods_report_service.py b/lambdas/tests/unit/services/test_ods_report_service.py index a3e5b8c23e..095ffbe568 100644 --- a/lambdas/tests/unit/services/test_ods_report_service.py +++ b/lambdas/tests/unit/services/test_ods_report_service.py @@ -669,7 +669,6 @@ def test_create_pdf_report_multiple_pages(ods_report_service, tmp_path): assert len(reader.pages) > 1 - def test_build_patient_rows_skips_items_without_nhs_number(ods_report_service): items = [ { @@ -747,4 +746,4 @@ def test_build_patient_rows_does_not_overwrite_existing_values_with_none( expected_last_updated = datetime.fromtimestamp(1700000010, tz=timezone.utc) assert rows[nhs]["created"] == expected_created - assert rows[nhs]["last_updated"] == expected_last_updated \ No newline at end of file + assert rows[nhs]["last_updated"] == expected_last_updated From 056466abca6321941a13a1ae9b1d750b58b5a663 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Mon, 2 Mar 2026 13:32:58 +0000 Subject: [PATCH 06/15] [PRMP-1505] fixed comment --- lambdas/handlers/get_report_by_ods_handler.py | 4 +-- lambdas/services/ods_report_service.py | 18 ++++++------- .../test_get_report_by_ods_handler.py | 26 +++++++++---------- .../unit/services/test_ods_report_service.py | 6 ++--- 4 files changed, 27 insertions(+), 27 deletions(-) diff --git a/lambdas/handlers/get_report_by_ods_handler.py b/lambdas/handlers/get_report_by_ods_handler.py index e7867460d6..4e6e15b6df 100644 --- a/lambdas/handlers/get_report_by_ods_handler.py +++ b/lambdas/handlers/get_report_by_ods_handler.py @@ -72,7 +72,7 @@ def create_report(ods_code: str, file_type: FileType, report_type: str): def create_patient_report(ods_code: str, file_type: FileType): service = OdsReportService() - return service.get_nhs_numbers_by_ods( + return service.generate_ods_report( ods_code=ods_code, is_pre_signed_needed=True, is_upload_to_s3_needed=True, @@ -94,7 +94,7 @@ def handle_manual_trigger(event): service = OdsReportService() for ods_code in ods_codes: logger.info(f"Starting process for ods code: {ods_code}") - service.get_nhs_numbers_by_ods( + service.generate_ods_report( ods_code=ods_code, is_upload_to_s3_needed=True, file_type_output=file_type ) return ApiGatewayResponse( diff --git a/lambdas/services/ods_report_service.py b/lambdas/services/ods_report_service.py index 7430e54acc..85dc49af2c 100644 --- a/lambdas/services/ods_report_service.py +++ b/lambdas/services/ods_report_service.py @@ -42,7 +42,7 @@ def __init__(self): self.s3_service = S3Service(custom_aws_role=download_report_aws_role_arn) self.document_upload_review_service = DocumentUploadReviewService() - def get_nhs_numbers_by_ods( + def generate_ods_report( self, ods_code: str, is_pre_signed_needed: bool = False, @@ -294,8 +294,8 @@ def build_patient_rows( rows: dict[str, dict[str, Any]] = {} for item in document_items: - nhs = item.get(DocumentReferenceMetadataFields.NHS_NUMBER.value) - if not nhs: + nhs_number = item.get(DocumentReferenceMetadataFields.NHS_NUMBER.value) + if not nhs_number: continue created_dt = iso_utc_string_to_datetime( @@ -305,10 +305,10 @@ def build_patient_rows( item.get(DocumentReferenceMetadataFields.LAST_UPDATED.value), ) - existing = rows.get(nhs) + existing = rows.get(nhs_number) if existing is None: - rows[nhs] = { - "nhs_number": nhs, + rows[nhs_number] = { + "nhs_number": nhs_number, "created": created_dt, "last_updated": updated_dt, } @@ -338,7 +338,7 @@ def create_csv_report( f"Total number of patients for ODS code {ods_code}: {len(patient_rows)}\n", ) - f.write("NHS number,Created,Last updated\n") + f.write("NHS Number,Created Date,Last Updated Date\n") for nhs in sorted(patient_rows.keys()): row = patient_rows[nhs] created = datetime_to_utc_iso_string(row.get("created")) @@ -390,7 +390,7 @@ def create_pdf_report( c.setFont("Helvetica", 12) c.drawString(x, y, f"Total number of patients: {len(patient_rows)}") y -= 20 - c.drawString(x, y, "NHS number | Created | Last updated") + c.drawString(x, y, "NHS Number | Created Date | Last Updated Date") y -= 20 for nhs in sorted(patient_rows.keys()): if y < 40: @@ -405,7 +405,7 @@ def create_pdf_report( y = height - 50 y -= 40 - c.drawString(x, y, "NHS Number | Created | Last Updated") + c.drawString(x, y, "NHS Number | Created Date| Last Updated Date") y -= 20 row = patient_rows[nhs] diff --git a/lambdas/tests/unit/handlers/test_get_report_by_ods_handler.py b/lambdas/tests/unit/handlers/test_get_report_by_ods_handler.py index 56c95d90d7..c5f81d7c70 100644 --- a/lambdas/tests/unit/handlers/test_get_report_by_ods_handler.py +++ b/lambdas/tests/unit/handlers/test_get_report_by_ods_handler.py @@ -31,7 +31,7 @@ def test_lambda_handler_api_gateway_request( "headers": {"Authorization": "mock_token"}, "queryStringParameters": {"odsReportType": "PATIENT"}, } - mock_service.get_nhs_numbers_by_ods.return_value = "example.com/presigned-url" + mock_service.generate_ods_report.return_value = "example.com/presigned-url" expected = ApiGatewayResponse( 200, json.dumps({"url": "example.com/presigned-url"}), "GET" ).create_api_gateway_response() @@ -39,7 +39,7 @@ def test_lambda_handler_api_gateway_request( result = lambda_handler(event, context) assert result == expected - mock_service.get_nhs_numbers_by_ods.assert_called_once_with( + mock_service.generate_ods_report.assert_called_once_with( ods_code="ODS123", is_pre_signed_needed=True, is_upload_to_s3_needed=True, @@ -68,7 +68,7 @@ def test_lambda_handler_gateway_request_review( def test_lambda_handler_manual_trigger(mock_service, set_env, context): event = {"odsCode": "ODS123,ODS456"} - mock_service.get_nhs_numbers_by_ods.return_value = None + mock_service.generate_ods_report.return_value = None expected = ApiGatewayResponse( 200, "Successfully created report", "GET" ).create_api_gateway_response() @@ -76,11 +76,11 @@ def test_lambda_handler_manual_trigger(mock_service, set_env, context): result = lambda_handler(event, context) assert result == expected - assert mock_service.get_nhs_numbers_by_ods.call_count == 2 - mock_service.get_nhs_numbers_by_ods.assert_any_call( + assert mock_service.generate_ods_report.call_count == 2 + mock_service.generate_ods_report.assert_any_call( ods_code="ODS123", file_type_output="csv", is_upload_to_s3_needed=True ) - mock_service.get_nhs_numbers_by_ods.assert_any_call( + mock_service.generate_ods_report.assert_any_call( ods_code="ODS456", file_type_output="csv", is_upload_to_s3_needed=True ) @@ -133,11 +133,11 @@ def test_handle_api_gateway_request_handles_output_file_format( request_context.authorization = { "selected_organisation": {"org_ods_code": "ODS123"} } - mock_service.get_nhs_numbers_by_ods.return_value = "example.com/presigned-url" + mock_service.generate_ods_report.return_value = "example.com/presigned-url" handle_api_gateway_request(event) - mock_service.get_nhs_numbers_by_ods.assert_called_once_with( + mock_service.generate_ods_report.assert_called_once_with( ods_code="ODS123", is_pre_signed_needed=True, is_upload_to_s3_needed=True, @@ -158,7 +158,7 @@ def test_handle_api_gateway_request_invalid_ods_code_raises_exception(mock_servi request_context.authorization = { "selected_organisation": {"org_ods_code": "ODS123"} } - mock_service.get_nhs_numbers_by_ods.side_effect = OdsErrorException( + mock_service.generate_ods_report.side_effect = OdsErrorException( "Invalid ODS code format" ) @@ -191,7 +191,7 @@ def test_handle_api_gateway_request_incorrect_report_type_raises_exception( def test_handle_manual_trigger_single_ods_code(mock_service): event = {"odsCode": "ODS123", "outputFileFormat": "pdf"} - mock_service.get_nhs_numbers_by_ods.return_value = None + mock_service.generate_ods_report.return_value = None expected = ApiGatewayResponse( 200, "Successfully created report", "GET" ).create_api_gateway_response() @@ -199,15 +199,15 @@ def test_handle_manual_trigger_single_ods_code(mock_service): result = handle_manual_trigger(event) assert result == expected - assert mock_service.get_nhs_numbers_by_ods.call_count == 1 - mock_service.get_nhs_numbers_by_ods.assert_called_once_with( + assert mock_service.generate_ods_report.call_count == 1 + mock_service.generate_ods_report.assert_called_once_with( ods_code="ODS123", file_type_output="pdf", is_upload_to_s3_needed=True ) def test_handle_manual_trigger_invalid_ods_code_format(mock_service): event = {"odsCode": "ODS123,ODS456,ODS789"} - mock_service.get_nhs_numbers_by_ods.side_effect = OdsErrorException( + mock_service.generate_ods_report.side_effect = OdsErrorException( "Invalid ODS code format" ) diff --git a/lambdas/tests/unit/services/test_ods_report_service.py b/lambdas/tests/unit/services/test_ods_report_service.py index 095ffbe568..bb07285f03 100644 --- a/lambdas/tests/unit/services/test_ods_report_service.py +++ b/lambdas/tests/unit/services/test_ods_report_service.py @@ -148,7 +148,7 @@ def test_get_nhs_numbers_by_ods( return_value=patient_rows, ) - ods_report_service.get_nhs_numbers_by_ods("ODS123") + ods_report_service.generate_ods_report("ODS123") mock_query_table_by_index.assert_called_once_with("ODS123") mock_create_and_save_ods_report.assert_called_once_with( @@ -181,7 +181,7 @@ def test_get_nhs_numbers_by_ods_with_temp_folder( return_value=patient_rows, ) - ods_report_service.get_nhs_numbers_by_ods("ODS123", is_upload_to_s3_needed=True) + ods_report_service.generate_ods_report("ODS123", is_upload_to_s3_needed=True) mock_query_table_by_index.assert_called_once_with("ODS123") mock_create_and_save_ods_report.assert_called_once_with( @@ -402,7 +402,7 @@ def test_create_report_csv(ods_report_service, tmp_path): f"Total number of patients for ODS code {ods_code}: {len(patient_rows)}\n" in content ) - assert "NHS number,Created,Last updated\n" in content + assert "NHS Number,Created Date,Last Updated Date\n" in content def test_create_xlsx_report(ods_report_service, tmp_path): From 69ccd5a4c45376cabdc21788fc7d5a01e5c003f3 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Mon, 2 Mar 2026 13:33:39 +0000 Subject: [PATCH 07/15] [PRMP-1505] formatting --- lambdas/handlers/get_report_by_ods_handler.py | 13 +- .../models/fhir/R4/fhir_document_reference.py | 54 +++--- lambdas/services/bulk_upload_service.py | 1 + ...aged_document_review_processing_service.py | 45 ++--- .../tests/e2e/api/test_upload_document_api.py | 1 + .../test_get_report_by_ods_handler.py | 31 ++-- .../fhir/R4/test_fhir_document_reference.py | 3 +- .../unit/services/test_bulk_upload_service.py | 163 +++++++++--------- ...aged_document_review_processing_service.py | 53 +++--- 9 files changed, 185 insertions(+), 179 deletions(-) diff --git a/lambdas/handlers/get_report_by_ods_handler.py b/lambdas/handlers/get_report_by_ods_handler.py index 4e6e15b6df..0cb2ffbb7f 100644 --- a/lambdas/handlers/get_report_by_ods_handler.py +++ b/lambdas/handlers/get_report_by_ods_handler.py @@ -21,7 +21,7 @@ "PRESIGNED_ASSUME_ROLE", "LLOYD_GEORGE_DYNAMODB_NAME", "STATISTICAL_REPORTS_BUCKET", - ] + ], ) @override_error_check @handle_lambda_exceptions @@ -31,13 +31,12 @@ def lambda_handler(event, context): if "httpMethod" in event: return handle_api_gateway_request(event) - else: - return handle_manual_trigger(event) + return handle_manual_trigger(event) def handle_api_gateway_request(event): ods_code = request_context.authorization.get("selected_organisation", {}).get( - "org_ods_code" + "org_ods_code", ) if not ods_code: raise OdsErrorException("No ODS code provided") @@ -58,7 +57,7 @@ def handle_api_gateway_request(event): pre_signed_url = create_report(ods_code, file_type, report_type) logger.info("A report has been successfully created.") return ApiGatewayResponse( - 200, json.dumps({"url": pre_signed_url}), "GET" + 200, json.dumps({"url": pre_signed_url}), "GET", ).create_api_gateway_response() @@ -95,8 +94,8 @@ def handle_manual_trigger(event): for ods_code in ods_codes: logger.info(f"Starting process for ods code: {ods_code}") service.generate_ods_report( - ods_code=ods_code, is_upload_to_s3_needed=True, file_type_output=file_type + ods_code=ods_code, is_upload_to_s3_needed=True, file_type_output=file_type, ) return ApiGatewayResponse( - 200, "Successfully created report", "GET" + 200, "Successfully created report", "GET", ).create_api_gateway_response() diff --git a/lambdas/models/fhir/R4/fhir_document_reference.py b/lambdas/models/fhir/R4/fhir_document_reference.py index cf86f8d700..df60b9f3e4 100644 --- a/lambdas/models/fhir/R4/fhir_document_reference.py +++ b/lambdas/models/fhir/R4/fhir_document_reference.py @@ -1,5 +1,7 @@ from typing import Any, Dict, List, Literal, Optional +from pydantic import BaseModel, Field + from enums.patient_ods_inactive_status import PatientOdsInactiveStatus from enums.snomed_codes import SnomedCode, SnomedCodes from models.document_reference import DocumentReference as NdrDocumentReference @@ -11,7 +13,6 @@ Period, Reference, ) -from pydantic import BaseModel, Field from utils.exceptions import FhirDocumentReferenceException from utils.ods_utils import PCSE_ODS_CODE @@ -64,7 +65,7 @@ class ContentStabilityExtensionValueCodeableConcept(CodeableConcept): """CodeableConcept for content stability.""" coding: List[ContentStabilityExtensionCoding] = Field( - default_factory=lambda: [ContentStabilityExtensionCoding()] + default_factory=lambda: [ContentStabilityExtensionCoding()], ) @@ -73,7 +74,7 @@ class ContentStabilityExtension(Extension): url: Literal[CONTENT_STABILITY_URL] = CONTENT_STABILITY_URL valueCodeableConcept: ContentStabilityExtensionValueCodeableConcept = Field( - default_factory=ContentStabilityExtensionValueCodeableConcept + default_factory=ContentStabilityExtensionValueCodeableConcept, ) @@ -136,8 +137,7 @@ def extract_nhs_number_from_fhir(self) -> str: and self.subject.identifier.system == "https://fhir.nhs.uk/Id/nhs-number" ): return self.subject.identifier.value - else: - raise FhirDocumentReferenceException("NHS number was not found") + raise FhirDocumentReferenceException("NHS number was not found") class DocumentReferenceInfo(BaseModel): @@ -167,7 +167,7 @@ def _create_identifier(self, system_suffix: str, value: str) -> Dict[str, Any]: "identifier": { "system": f"{FHIR_BASE_URL}/{system_suffix}", "value": value, - } + }, } def _create_snomed_coding(self, snomed_code: SnomedCode) -> List[Dict[str, str]]: @@ -184,7 +184,7 @@ def _create_snomed_coding(self, snomed_code: SnomedCode) -> List[Dict[str, str]] "system": SNOMED_URL, "code": snomed_code.code, "display": snomed_code.display_name, - } + }, ] def create_nrl_fhir_document_reference_object(self) -> DocumentReference: @@ -204,25 +204,25 @@ def create_nrl_fhir_document_reference_object(self) -> DocumentReference: subject=Reference(**self._create_identifier("nhs-number", self.nhs_number)), content=[DocumentReferenceContent(attachment=self.attachment)], custodian=Reference( - **self._create_identifier("ods-organization-code", self.custodian) + **self._create_identifier("ods-organization-code", self.custodian), ), type=CodeableConcept( - coding=self._create_snomed_coding(self.snomed_code_doc_type) + coding=self._create_snomed_coding(self.snomed_code_doc_type), ), category=[ CodeableConcept( - coding=self._create_snomed_coding(self.snomed_code_category) - ) + coding=self._create_snomed_coding(self.snomed_code_category), + ), ], author=[ Reference( - **self._create_identifier("ods-organization-code", self.custodian) - ) + **self._create_identifier("ods-organization-code", self.custodian), + ), ], context=DocumentReferenceContext( practiceSetting=CodeableConcept( - coding=self._create_snomed_coding(self.snomed_code_practice_setting) - ) + coding=self._create_snomed_coding(self.snomed_code_practice_setting), + ), ), ) @@ -233,7 +233,7 @@ def create_nrl_fhir_document_reference_object(self) -> DocumentReference: return fhir_document_ref def create_fhir_document_reference_object( - self, document: NdrDocumentReference + self, document: NdrDocumentReference, ) -> DocumentReference: """Create a FHIR DocumentReference . @@ -248,7 +248,7 @@ def create_fhir_document_reference_object( id=f"{self.snomed_code_doc_type.code}~{document.id}", docStatus=document.doc_status, type=CodeableConcept( - coding=self._create_snomed_coding(self.snomed_code_doc_type) + coding=self._create_snomed_coding(self.snomed_code_doc_type), ), subject=Reference(**self._create_identifier("nhs-number", self.nhs_number)), content=[DocumentReferenceContent(attachment=self.attachment)], @@ -256,35 +256,35 @@ def create_fhir_document_reference_object( author=[ Reference( **self._create_identifier( - "ods-organization-code", document.author or self.custodian - ) - ) + "ods-organization-code", document.author or self.custodian, + ), + ), ], custodian=Reference( **self._create_identifier( - "ods-organization-code", document.custodian or self.custodian - ) + "ods-organization-code", document.custodian or self.custodian, + ), ), meta=Meta(versionId=document.version), ) def create_fhir_document_reference_object_basic( - self, original_id: str, original_version + self, original_id: str, original_version, ) -> DocumentReference: return DocumentReference( resourceType="DocumentReference", id=f"{original_id}", type=CodeableConcept( - coding=self._create_snomed_coding(self.snomed_code_doc_type) + coding=self._create_snomed_coding(self.snomed_code_doc_type), ), subject=Reference(**self._create_identifier("nhs-number", self.nhs_number)), content=[DocumentReferenceContent(attachment=self.attachment)], author=[ Reference( **self._create_identifier( - "ods-organization-code", self.author or self.custodian - ) - ) + "ods-organization-code", self.author or self.custodian, + ), + ), ], meta=Meta(versionId=original_version), ) diff --git a/lambdas/services/bulk_upload_service.py b/lambdas/services/bulk_upload_service.py index 9b46673744..58e6790e6d 100644 --- a/lambdas/services/bulk_upload_service.py +++ b/lambdas/services/bulk_upload_service.py @@ -4,6 +4,7 @@ import pydantic from botocore.exceptions import ClientError + from enums.document_review_reason import DocumentReviewReason from enums.patient_ods_inactive_status import PatientOdsInactiveStatus from enums.snomed_codes import SnomedCodes diff --git a/lambdas/services/staged_document_review_processing_service.py b/lambdas/services/staged_document_review_processing_service.py index 9da7cee06e..fd25da1857 100644 --- a/lambdas/services/staged_document_review_processing_service.py +++ b/lambdas/services/staged_document_review_processing_service.py @@ -1,6 +1,7 @@ import os from botocore.exceptions import ClientError + from enums.document_review_status import DocumentReviewStatus from enums.virus_scan_result import VirusScanResult from models.document_reference import S3_PREFIX @@ -24,7 +25,7 @@ def __init__(self): self.destination_bucket_name = os.getenv("PENDING_REVIEW_BUCKET_NAME") def handle_upload_document_reference_request( - self, object_key: str, *args, **kwargs + self, object_key: str, *args, **kwargs, ) -> None: try: upload_id = self._validate_object_key(object_key) @@ -33,15 +34,15 @@ def handle_upload_document_reference_request( document_reference = self._fetch_document_reference_by_id(upload_id) if self._is_review_pending_upload( - document_reference + document_reference, ) and self._is_file_at_expected_location(document_reference, object_key): logger.info( - f"Document {document_reference.id} is in pending upload state, processing" + f"Document {document_reference.id} is in pending upload state, processing", ) self._process_document_if_ready(document_reference, object_key) else: logger.info( - f"Document {object_key} not in expected, deleting from staging" + f"Document {object_key} not in expected, deleting from staging", ) self.delete_file_from_staging_bucket(object_key) @@ -52,12 +53,12 @@ def handle_upload_document_reference_request( except (FileProcessingException, ClientError) as e: if is_transient_error(e): logger.error( - f"Transient error processing document {object_key}: {str(e)}" + f"Transient error processing document {object_key}: {str(e)}", ) raise e else: logger.error( - f"Permanent error processing document {object_key}: {str(e)}" + f"Permanent error processing document {object_key}: {str(e)}", ) return @@ -75,7 +76,7 @@ def _validate_object_key(self, object_key: str) -> str: return object_parts[-2] def _process_document_if_ready( - self, document_reference: DocumentUploadReviewReference, object_key: str + self, document_reference: DocumentUploadReviewReference, object_key: str, ) -> None: virus_scan_result = self._perform_virus_scan(document_reference, object_key) @@ -86,26 +87,26 @@ def _process_document_if_ready( else: document_reference.review_status = DocumentReviewStatus.VIRUS_SCAN_FAILED self.review_document_service.update_document_review_status( - document_reference + document_reference, ) logger.warning( - f"Virus scan failed for document {document_reference.id}, status updated" + f"Virus scan failed for document {document_reference.id}, status updated", ) def _fetch_document_reference_by_id( - self, document_key: str + self, document_key: str, ) -> DocumentUploadReviewReference: documents = self.review_document_service.fetch_documents_from_table( - search_key="ID", search_condition=document_key + search_key="ID", search_condition=document_key, ) if not documents or len(documents) != 1: raise DocumentServiceException( - f"Expected exactly one document reference for upload id, found {len(documents)}" + f"Expected exactly one document reference for upload id, found {len(documents)}", ) return documents[0] def _is_review_pending_upload( - self, document_reference: DocumentUploadReviewReference + self, document_reference: DocumentUploadReviewReference, ) -> bool: return ( document_reference.review_status @@ -113,19 +114,19 @@ def _is_review_pending_upload( ) def _is_file_at_expected_location( - self, document_reference: DocumentUploadReviewReference, object_key: str + self, document_reference: DocumentUploadReviewReference, object_key: str, ) -> bool: expected_file_location = document_reference.files[0].file_location return expected_file_location.endswith(object_key) def _perform_virus_scan( - self, document_reference: DocumentUploadReviewReference, object_key: str + self, document_reference: DocumentUploadReviewReference, object_key: str, ) -> str: result = self.virus_scan_service.scan_file( - object_key, nhs_number=document_reference.nhs_number + object_key, nhs_number=document_reference.nhs_number, ) logger.info( - f"Virus scan result: {result} for document reference: {document_reference.id}" + f"Virus scan result: {result} for document reference: {document_reference.id}", ) return result @@ -136,20 +137,20 @@ def _process_review_document_reference( ) -> None: self.copy_files_from_staging_bucket(document_reference, object_key) update_condition = self.review_document_service.build_review_dynamo_filter( - status=DocumentReviewStatus.REVIEW_PENDING_UPLOAD + status=DocumentReviewStatus.REVIEW_PENDING_UPLOAD, ) self.review_document_service.update_document_review_status( - document_reference, condition_expression=update_condition + document_reference, condition_expression=update_condition, ) self.delete_file_from_staging_bucket(object_key) logger.info(f"Successfully processed clean document: {document_reference.id}") def copy_files_from_staging_bucket( - self, document_reference: DocumentUploadReviewReference, source_file_key: str + self, document_reference: DocumentUploadReviewReference, source_file_key: str, ) -> None: try: logger.info( - f"Copying file {source_file_key} from staging bucket for document {document_reference.id}" + f"Copying file {source_file_key} from staging bucket for document {document_reference.id}", ) self.s3_service.copy_across_bucket( @@ -167,7 +168,7 @@ def copy_files_from_staging_bucket( logger.error("Error copying file from staging bucket") if e.response["ResponseMetadata"]["HTTPStatusCode"] == 412: logger.warning( - f"File already exists in destination bucket: {source_file_key}" + f"File already exists in destination bucket: {source_file_key}", ) return raise e diff --git a/lambdas/tests/e2e/api/test_upload_document_api.py b/lambdas/tests/e2e/api/test_upload_document_api.py index 204eea9368..c2514567cd 100644 --- a/lambdas/tests/e2e/api/test_upload_document_api.py +++ b/lambdas/tests/e2e/api/test_upload_document_api.py @@ -5,6 +5,7 @@ import requests from syrupy.filters import paths + from tests.e2e.conftest import ( API_ENDPOINT, API_KEY, diff --git a/lambdas/tests/unit/handlers/test_get_report_by_ods_handler.py b/lambdas/tests/unit/handlers/test_get_report_by_ods_handler.py index c5f81d7c70..8de9a186e4 100644 --- a/lambdas/tests/unit/handlers/test_get_report_by_ods_handler.py +++ b/lambdas/tests/unit/handlers/test_get_report_by_ods_handler.py @@ -2,6 +2,7 @@ import logging import pytest + from handlers.get_report_by_ods_handler import ( handle_api_gateway_request, handle_manual_trigger, @@ -18,13 +19,13 @@ def mock_service(mocker): mock_service = mocker.Mock() mocker.patch( - "handlers.get_report_by_ods_handler.OdsReportService", return_value=mock_service + "handlers.get_report_by_ods_handler.OdsReportService", return_value=mock_service, ) return mock_service def test_lambda_handler_api_gateway_request( - mock_service, set_env, context, mock_jwt_encode + mock_service, set_env, context, mock_jwt_encode, ): event = { "httpMethod": "GET", @@ -33,7 +34,7 @@ def test_lambda_handler_api_gateway_request( } mock_service.generate_ods_report.return_value = "example.com/presigned-url" expected = ApiGatewayResponse( - 200, json.dumps({"url": "example.com/presigned-url"}), "GET" + 200, json.dumps({"url": "example.com/presigned-url"}), "GET", ).create_api_gateway_response() result = lambda_handler(event, context) @@ -48,7 +49,7 @@ def test_lambda_handler_api_gateway_request( def test_lambda_handler_gateway_request_review( - mock_service, set_env, context, mock_jwt_encode + mock_service, set_env, context, mock_jwt_encode, ): event = { "httpMethod": "GET", @@ -57,7 +58,7 @@ def test_lambda_handler_gateway_request_review( } mock_service.get_documents_for_review.return_value = "example.com/presigned-url" expected = ApiGatewayResponse( - 200, json.dumps({"url": "example.com/presigned-url"}), "GET" + 200, json.dumps({"url": "example.com/presigned-url"}), "GET", ).create_api_gateway_response() result = lambda_handler(event, context) @@ -70,7 +71,7 @@ def test_lambda_handler_manual_trigger(mock_service, set_env, context): event = {"odsCode": "ODS123,ODS456"} mock_service.generate_ods_report.return_value = None expected = ApiGatewayResponse( - 200, "Successfully created report", "GET" + 200, "Successfully created report", "GET", ).create_api_gateway_response() result = lambda_handler(event, context) @@ -78,10 +79,10 @@ def test_lambda_handler_manual_trigger(mock_service, set_env, context): assert result == expected assert mock_service.generate_ods_report.call_count == 2 mock_service.generate_ods_report.assert_any_call( - ods_code="ODS123", file_type_output="csv", is_upload_to_s3_needed=True + ods_code="ODS123", file_type_output="csv", is_upload_to_s3_needed=True, ) mock_service.generate_ods_report.assert_any_call( - ods_code="ODS456", file_type_output="csv", is_upload_to_s3_needed=True + ods_code="ODS456", file_type_output="csv", is_upload_to_s3_needed=True, ) @@ -128,10 +129,10 @@ def test_lambda_handler_manual_trigger(mock_service, set_env, context): ], ) def test_handle_api_gateway_request_handles_output_file_format( - mock_service, event, output + mock_service, event, output, ): request_context.authorization = { - "selected_organisation": {"org_ods_code": "ODS123"} + "selected_organisation": {"org_ods_code": "ODS123"}, } mock_service.generate_ods_report.return_value = "example.com/presigned-url" @@ -156,10 +157,10 @@ def test_handle_api_gateway_request_no_ods_code_raises_exception(mock_service): def test_handle_api_gateway_request_invalid_ods_code_raises_exception(mock_service): event = {"httpMethod": "GET", "queryStringParameters": {"odsReportType": "PATIENT"}} request_context.authorization = { - "selected_organisation": {"org_ods_code": "ODS123"} + "selected_organisation": {"org_ods_code": "ODS123"}, } mock_service.generate_ods_report.side_effect = OdsErrorException( - "Invalid ODS code format" + "Invalid ODS code format", ) with pytest.raises(OdsErrorException, match="Invalid ODS code format"): @@ -193,7 +194,7 @@ def test_handle_manual_trigger_single_ods_code(mock_service): event = {"odsCode": "ODS123", "outputFileFormat": "pdf"} mock_service.generate_ods_report.return_value = None expected = ApiGatewayResponse( - 200, "Successfully created report", "GET" + 200, "Successfully created report", "GET", ).create_api_gateway_response() result = handle_manual_trigger(event) @@ -201,14 +202,14 @@ def test_handle_manual_trigger_single_ods_code(mock_service): assert result == expected assert mock_service.generate_ods_report.call_count == 1 mock_service.generate_ods_report.assert_called_once_with( - ods_code="ODS123", file_type_output="pdf", is_upload_to_s3_needed=True + ods_code="ODS123", file_type_output="pdf", is_upload_to_s3_needed=True, ) def test_handle_manual_trigger_invalid_ods_code_format(mock_service): event = {"odsCode": "ODS123,ODS456,ODS789"} mock_service.generate_ods_report.side_effect = OdsErrorException( - "Invalid ODS code format" + "Invalid ODS code format", ) with pytest.raises(OdsErrorException, match="Invalid ODS code format"): diff --git a/lambdas/tests/unit/models/fhir/R4/test_fhir_document_reference.py b/lambdas/tests/unit/models/fhir/R4/test_fhir_document_reference.py index 5eced85853..3bd55ac953 100644 --- a/lambdas/tests/unit/models/fhir/R4/test_fhir_document_reference.py +++ b/lambdas/tests/unit/models/fhir/R4/test_fhir_document_reference.py @@ -1,4 +1,5 @@ import pytest + from models.fhir.R4.base_models import Reference from models.fhir.R4.fhir_document_reference import ( DocumentReference as FhirDocumentReference, @@ -40,7 +41,7 @@ def test_extract_nhs_number_from_fhir_with_missing_identifier(valid_fhir_doc_obj def test_extract_nhs_number_from_fhir_returns_nhs_number( - valid_fhir_doc_object, valid_nhs_number + valid_fhir_doc_object, valid_nhs_number, ): """Test that extract_nhs_number_from_fhir returns the correct nhs number""" result = valid_fhir_doc_object.extract_nhs_number_from_fhir() diff --git a/lambdas/tests/unit/services/test_bulk_upload_service.py b/lambdas/tests/unit/services/test_bulk_upload_service.py index f7bbfc52e2..0199936976 100644 --- a/lambdas/tests/unit/services/test_bulk_upload_service.py +++ b/lambdas/tests/unit/services/test_bulk_upload_service.py @@ -3,11 +3,12 @@ import pytest from botocore.exceptions import ClientError +from freezegun import freeze_time + from enums.document_review_reason import DocumentReviewReason from enums.patient_ods_inactive_status import PatientOdsInactiveStatus from enums.upload_status import UploadStatus from enums.virus_scan_result import SCAN_RESULT_TAG_KEY, VirusScanResult -from freezegun import freeze_time from models.pds_models import Patient from repositories.bulk_upload.bulk_upload_s3_repository import BulkUploadS3Repository from repositories.bulk_upload.bulk_upload_sqs_repository import BulkUploadSqsRepository @@ -74,10 +75,10 @@ def repo_with_review_enabled(set_env, mocker): mocker.patch("services.bulk_upload_service.BulkUploadSqsRepository") mocker.patch("services.bulk_upload_service.BulkUploadS3Repository") mocker.patch( - "services.bulk_upload_service.allowed_to_ingest_ods_code", return_value=True + "services.bulk_upload_service.allowed_to_ingest_ods_code", return_value=True, ) service = BulkUploadService( - strict_mode=True, bypass_pds=False, send_to_review_enabled=True + strict_mode=True, bypass_pds=False, send_to_review_enabled=True, ) yield service @@ -163,13 +164,13 @@ def mock_back_to_queue(mocker): def build_resolved_file_names_cache( - file_path_in_metadata: list[str], file_path_in_s3: list[str] + file_path_in_metadata: list[str], file_path_in_s3: list[str], ) -> dict: return dict(zip(file_path_in_metadata, file_path_in_s3)) def test_lambda_handler_process_each_sqs_message_one_by_one( - set_env, mock_handle_sqs_message + set_env, mock_handle_sqs_message, ): service = BulkUploadService(True) @@ -181,7 +182,7 @@ def test_lambda_handler_process_each_sqs_message_one_by_one( def test_lambda_handler_continue_process_next_message_after_handled_error( - set_env, mock_handle_sqs_message + set_env, mock_handle_sqs_message, ): # emulate that unexpected error happen at 2nd message mock_handle_sqs_message.side_effect = [ @@ -197,7 +198,7 @@ def test_lambda_handler_continue_process_next_message_after_handled_error( def test_lambda_handler_handle_pds_too_many_requests_exception( - set_env, mock_handle_sqs_message, mock_back_to_queue + set_env, mock_handle_sqs_message, mock_back_to_queue, ): # emulate that unexpected error happen at 7th message mock_handle_sqs_message.side_effect = ( @@ -232,10 +233,10 @@ def test_handle_sqs_message_happy_path( TEST_STAGING_METADATA.retries = 0 mock_create_lg_records_and_copy_files = mocker.patch.object( - BulkUploadService, "create_lg_records_and_copy_files" + BulkUploadService, "create_lg_records_and_copy_files", ) mock_report_upload_complete = mocker.patch.object( - repo_under_test.dynamo_repository, "write_report_upload_to_dynamo" + repo_under_test.dynamo_repository, "write_report_upload_to_dynamo", ) mock_remove_ingested_file_from_source_bucket = mocker.patch.object( repo_under_test.bulk_upload_s3_repository, @@ -245,7 +246,7 @@ def test_handle_sqs_message_happy_path( repo_under_test.handle_sqs_message(message=TEST_SQS_MESSAGE) mock_create_lg_records_and_copy_files.assert_called_with( - TEST_STAGING_METADATA, TEST_CURRENT_GP_ODS + TEST_STAGING_METADATA, TEST_CURRENT_GP_ODS, ) mock_pds_validation_strict.assert_called() mock_report_upload_complete.assert_called() @@ -265,11 +266,11 @@ def test_handle_sqs_message_happy_path_single_file( ): TEST_STAGING_METADATA.retries = 0 mock_create_lg_records_and_copy_files = mocker.patch.object( - BulkUploadService, "create_lg_records_and_copy_files" + BulkUploadService, "create_lg_records_and_copy_files", ) mock_create_lg_records_and_copy_files.return_value = TEST_DOCUMENT_REFERENCE mock_report_upload_complete = mocker.patch.object( - repo_under_test.dynamo_repository, "write_report_upload_to_dynamo" + repo_under_test.dynamo_repository, "write_report_upload_to_dynamo", ) mock_remove_ingested_file_from_source_bucket = mocker.patch.object( repo_under_test.bulk_upload_s3_repository, @@ -280,7 +281,7 @@ def test_handle_sqs_message_happy_path_single_file( repo_under_test.handle_sqs_message(message=TEST_SQS_MESSAGE_SINGLE_FILE) mock_create_lg_records_and_copy_files.assert_called_with( - TEST_STAGING_METADATA_SINGLE_FILE, TEST_CURRENT_GP_ODS + TEST_STAGING_METADATA_SINGLE_FILE, TEST_CURRENT_GP_ODS, ) mock_report_upload_complete.assert_called() mock_remove_ingested_file_from_source_bucket.assert_called() @@ -288,13 +289,13 @@ def test_handle_sqs_message_happy_path_single_file( def set_up_mocks_for_non_ascii_files( - service: BulkUploadService, mocker, patient_name_on_s3: str + service: BulkUploadService, mocker, patient_name_on_s3: str, ): service.s3_service = mocker.MagicMock() service.dynamo_repository = mocker.MagicMock() expected_s3_file_paths = make_s3_file_paths( - make_valid_lg_file_names(total_number=3, patient_name=patient_name_on_s3) + make_valid_lg_file_names(total_number=3, patient_name=patient_name_on_s3), ) def mock_file_exist_on_s3(file_key: str) -> bool: @@ -309,11 +310,11 @@ def mock_get_tag_value(s3_bucket_name: str, file_key: str, tag_key: str) -> str: return VirusScanResult.CLEAN raise RuntimeError( - "Unexpected S3 tag calls during non-ascii file name test case." + "Unexpected S3 tag calls during non-ascii file name test case.", ) def mock_copy_across_bucket( - source_bucket: str, source_file_key: str, dest_bucket: str, **_kwargs + source_bucket: str, source_file_key: str, dest_bucket: str, **_kwargs, ): if ( source_bucket == MOCK_STAGING_STORE_BUCKET @@ -354,7 +355,7 @@ def test_handle_sqs_message_happy_path_with_non_ascii_filenames( repo_under_test.bulk_upload_s3_repository.lg_bucket_name = MOCK_LG_BUCKET set_up_mocks_for_non_ascii_files(repo_under_test, mocker, patient_name_on_s3) test_staging_metadata = build_test_staging_metadata_from_patient_name( - patient_name_in_metadata_file + patient_name_in_metadata_file, ) test_sqs_message = build_test_sqs_message(test_staging_metadata) @@ -377,17 +378,17 @@ def test_handle_sqs_message_calls_report_upload_failure_when_patient_record_alre TEST_STAGING_METADATA.retries = 0 mock_create_lg_records_and_copy_files = mocker.patch.object( - BulkUploadService, "create_lg_records_and_copy_files" + BulkUploadService, "create_lg_records_and_copy_files", ) mock_remove_ingested_file_from_source_bucket = mocker.patch.object( repo_under_test.bulk_upload_s3_repository, "remove_ingested_file_from_source_bucket", ) mock_report_upload_failure = mocker.patch.object( - repo_under_test.dynamo_repository, "write_report_upload_to_dynamo" + repo_under_test.dynamo_repository, "write_report_upload_to_dynamo", ) mocked_error = PatientRecordAlreadyExistException( - "Lloyd George already exists for patient, upload cancelled." + "Lloyd George already exists for patient, upload cancelled.", ) mock_validate_files.side_effect = mocked_error @@ -396,7 +397,7 @@ def test_handle_sqs_message_calls_report_upload_failure_when_patient_record_alre mock_create_lg_records_and_copy_files.assert_not_called() mock_remove_ingested_file_from_source_bucket.assert_not_called() mock_report_upload_failure.assert_called_with( - TEST_STAGING_METADATA, UploadStatus.FAILED, str(mocked_error), "Y12345" + TEST_STAGING_METADATA, UploadStatus.FAILED, str(mocked_error), "Y12345", ) repo_under_test.sqs_repository.send_message_to_pdf_stitching_queue.assert_not_called() @@ -412,17 +413,17 @@ def test_handle_sqs_message_calls_report_upload_failure_when_lg_file_name_invali ): TEST_STAGING_METADATA.retries = 0 mock_create_lg_records_and_copy_files = mocker.patch.object( - BulkUploadService, "create_lg_records_and_copy_files" + BulkUploadService, "create_lg_records_and_copy_files", ) mock_remove_ingested_file_from_source_bucket = mocker.patch.object( repo_under_test.bulk_upload_s3_repository, "remove_ingested_file_from_source_bucket", ) mock_report_upload_failure = mocker.patch.object( - repo_under_test.dynamo_repository, "write_report_upload_to_dynamo" + repo_under_test.dynamo_repository, "write_report_upload_to_dynamo", ) mocked_error = LGInvalidFilesException( - "One or more of the files do not match naming convention" + "One or more of the files do not match naming convention", ) mock_validate_files.side_effect = mocked_error @@ -452,10 +453,10 @@ def test_handle_sqs_message_report_failure_when_document_is_infected( ): TEST_STAGING_METADATA.retries = 0 mock_report_upload_failure = mocker.patch.object( - repo_under_test.dynamo_repository, "write_report_upload_to_dynamo" + repo_under_test.dynamo_repository, "write_report_upload_to_dynamo", ) mock_create_lg_records_and_copy_files = mocker.patch.object( - BulkUploadService, "create_lg_records_and_copy_files" + BulkUploadService, "create_lg_records_and_copy_files", ) mock_remove_ingested_file_from_source_bucket = mocker.patch.object( repo_under_test.bulk_upload_s3_repository, @@ -494,7 +495,7 @@ def test_handle_sqs_message_report_failure_when_document_not_exist( S3FileNotFoundException ) mock_report_upload_failure = mocker.patch.object( - repo_under_test.dynamo_repository, "write_report_upload_to_dynamo" + repo_under_test.dynamo_repository, "write_report_upload_to_dynamo", ) repo_under_test.handle_sqs_message(message=TEST_SQS_MESSAGE) @@ -520,7 +521,7 @@ def test_handle_sqs_message_calls_report_upload_successful_when_patient_is_forma mock_ods_validation, ): mock_create_lg_records_and_copy_files = mocker.patch.object( - BulkUploadService, "create_lg_records_and_copy_files" + BulkUploadService, "create_lg_records_and_copy_files", ) mock_remove_ingested_file_from_source_bucket = ( repo_under_test.bulk_upload_s3_repository.remove_ingested_file_from_source_bucket @@ -557,7 +558,7 @@ def test_handle_sqs_message_calls_report_upload_successful_when_patient_is_infor mock_ods_validation, ): mock_create_lg_records_and_copy_files = mocker.patch.object( - BulkUploadService, "create_lg_records_and_copy_files" + BulkUploadService, "create_lg_records_and_copy_files", ) mock_pds_validation_strict.return_value = True mock_remove_ingested_file_from_source_bucket = ( @@ -594,7 +595,7 @@ def test_handle_sqs_message_calls_report_upload_successful_when_patient_has_hist mock_ods_validation, ): mock_create_lg_records_and_copy_files = mocker.patch.object( - BulkUploadService, "create_lg_records_and_copy_files" + BulkUploadService, "create_lg_records_and_copy_files", ) mock_pds_validation_strict.return_value = True mock_remove_ingested_file_from_source_bucket = ( @@ -631,7 +632,7 @@ def test_handle_sqs_message_calls_report_upload_successful_when_patient_is_infor mock_ods_validation, ): mock_create_lg_records_and_copy_files = mocker.patch.object( - BulkUploadService, "create_lg_records_and_copy_files" + BulkUploadService, "create_lg_records_and_copy_files", ) mock_pds_validation_strict.return_value = False mock_remove_ingested_file_from_source_bucket = ( @@ -672,17 +673,17 @@ def test_handle_sqs_message_put_staging_metadata_back_to_queue_when_virus_scan_r VirusScanNoResultException ) mock_report_upload_failure = mocker.patch.object( - repo_under_test.dynamo_repository, "write_report_upload_to_dynamo" + repo_under_test.dynamo_repository, "write_report_upload_to_dynamo", ) mock_create_lg_records_and_copy_files = mocker.patch.object( - BulkUploadService, "create_lg_records_and_copy_files" + BulkUploadService, "create_lg_records_and_copy_files", ) mock_remove_ingested_file_from_source_bucket = mocker.patch.object( repo_under_test.bulk_upload_s3_repository, "remove_ingested_file_from_source_bucket", ) mock_put_staging_metadata_back_to_queue = mocker.patch.object( - repo_under_test.sqs_repository, "put_staging_metadata_back_to_queue" + repo_under_test.sqs_repository, "put_staging_metadata_back_to_queue", ) repo_under_test.handle_sqs_message(message=TEST_SQS_MESSAGE) @@ -710,13 +711,13 @@ def test_handle_sqs_message_rollback_transaction_when_validation_pass_but_file_t TEST_STAGING_METADATA.retries = 0 mock_rollback_transaction_s3 = mocker.patch.object( - repo_under_test.bulk_upload_s3_repository, "rollback_transaction" + repo_under_test.bulk_upload_s3_repository, "rollback_transaction", ) mock_rollback_transaction_dynamo = mocker.patch.object( - repo_under_test.dynamo_repository, "rollback_transaction" + repo_under_test.dynamo_repository, "rollback_transaction", ) mock_report_upload_failure = mocker.patch.object( - repo_under_test.dynamo_repository, "write_report_upload_to_dynamo" + repo_under_test.dynamo_repository, "write_report_upload_to_dynamo", ) mock_remove_ingested_file_from_source_bucket = mocker.patch.object( repo_under_test.bulk_upload_s3_repository, @@ -752,11 +753,11 @@ def test_handle_sqs_message_rollback_transaction_when_validation_pass_but_file_t def test_handle_sqs_message_raise_InvalidMessageException_when_failed_to_extract_data_from_message( - repo_under_test, set_env, mocker + repo_under_test, set_env, mocker, ): invalid_message = {"body": "invalid content"} mock_create_lg_records_and_copy_files = mocker.patch.object( - BulkUploadService, "create_lg_records_and_copy_files" + BulkUploadService, "create_lg_records_and_copy_files", ) with pytest.raises(InvalidMessageException): @@ -766,11 +767,11 @@ def test_handle_sqs_message_raise_InvalidMessageException_when_failed_to_extract def test_validate_files_raise_LGInvalidFilesException_when_file_names_invalid( - repo_under_test, set_env, mock_validate_files + repo_under_test, set_env, mock_validate_files, ): TEST_STAGING_METADATA.retries = 0 invalid_file_name_metadata_as_json = json.dumps( - TEST_STAGING_METADATA_WITH_INVALID_FILENAME.model_dump() + TEST_STAGING_METADATA_WITH_INVALID_FILENAME.model_dump(), ) mock_validate_files.side_effect = LGInvalidFilesException @@ -781,7 +782,7 @@ def test_validate_files_raise_LGInvalidFilesException_when_file_names_invalid( @freeze_time("2023-10-2 13:00:00") def test_reports_failure_when_max_retries_reached( - set_env, mocker, mock_uuid, repo_under_test, mock_validate_files + set_env, mocker, mock_uuid, repo_under_test, mock_validate_files, ): mocker.patch("uuid.uuid4", return_value="123412342") @@ -795,7 +796,7 @@ def test_reports_failure_when_max_retries_reached( def test_resolve_source_file_path_when_filenames_dont_have_accented_chars( - set_env, repo_under_test + set_env, repo_under_test, ): expected = { file.file_path: file.file_path.lstrip("/") @@ -819,7 +820,7 @@ def test_resolve_source_file_path_when_filenames_dont_have_accented_chars( ids=["NFC --> NFC", "NFC --> NFD", "NFD --> NFC", "NFD --> NFD"], ) def test_resolve_source_file_path_when_filenames_have_accented_chars( - set_env, mocker, patient_name_on_s3, patient_name_in_metadata_file, repo_under_test + set_env, mocker, patient_name_on_s3, patient_name_in_metadata_file, repo_under_test, ): patient_name = "Évèlynêë François Ågāřdñ" expected_cache = {} @@ -833,7 +834,7 @@ def test_resolve_source_file_path_when_filenames_have_accented_chars( set_up_mocks_for_non_ascii_files(repo_under_test, mocker, patient_name_on_s3) test_staging_metadata = build_test_staging_metadata_from_patient_name( - patient_name_in_metadata_file + patient_name_in_metadata_file, ) repo_under_test.resolve_source_file_path(test_staging_metadata) actual = repo_under_test.file_path_cache @@ -842,7 +843,7 @@ def test_resolve_source_file_path_when_filenames_have_accented_chars( def test_resolves_source_file_path_raise_S3FileNotFoundException_if_filename_cant_match( - set_env, mocker, repo_under_test + set_env, mocker, repo_under_test, ): patient_name_on_s3 = "Some Name That Not Matching Metadata File" patient_name_in_metadata_file = NAME_WITH_ACCENT_NFC_FORM @@ -852,7 +853,7 @@ def test_resolves_source_file_path_raise_S3FileNotFoundException_if_filename_can set_up_mocks_for_non_ascii_files(repo_under_test, mocker, patient_name_on_s3) test_staging_metadata = build_test_staging_metadata_from_patient_name( - patient_name_in_metadata_file + patient_name_in_metadata_file, ) with pytest.raises(S3FileNotFoundException): @@ -862,16 +863,16 @@ def test_resolves_source_file_path_raise_S3FileNotFoundException_if_filename_can def test_create_lg_records_and_copy_files(set_env, mocker, mock_uuid, repo_under_test): test_document_reference = copy(TEST_DOCUMENT_REFERENCE) repo_under_test.convert_to_document_reference = mocker.MagicMock( - return_value=test_document_reference + return_value=test_document_reference, ) repo_under_test.bulk_upload_s3_repository.copy_to_lg_bucket = mocker.MagicMock( - return_value=MOCK_COPY_OBJECT_RESPONSE + return_value=MOCK_COPY_OBJECT_RESPONSE, ) TEST_STAGING_METADATA.retries = 0 repo_under_test.resolve_source_file_path(TEST_STAGING_METADATA) repo_under_test.create_lg_records_and_copy_files( - TEST_STAGING_METADATA, TEST_CURRENT_GP_ODS + TEST_STAGING_METADATA, TEST_CURRENT_GP_ODS, ) nhs_number = TEST_STAGING_METADATA.nhs_number @@ -890,7 +891,7 @@ def test_create_lg_records_and_copy_files(set_env, mocker, mock_uuid, repo_under ) assert repo_under_test.bulk_upload_s3_repository.copy_to_lg_bucket.call_count == 3 repo_under_test.dynamo_repository.create_record_in_lg_dynamo_table.assert_any_call( - test_document_reference + test_document_reference, ) assert ( repo_under_test.dynamo_repository.create_record_in_lg_dynamo_table.call_count @@ -916,24 +917,24 @@ def test_convert_to_document_reference(set_env, mock_uuid, repo_under_test): @freeze_time("2024-01-01 12:00:00") def test_reject_document_reference_if_missing_scan_date( - set_env, mock_uuid, repo_under_test, mocker + set_env, mock_uuid, repo_under_test, mocker, ): TEST_STAGING_METADATA.retries = 0 modify_test_sqs_message = json.loads(TEST_SQS_MESSAGE.get("body")) modify_test_sqs_message["files"][0]["scan_date"] = "" TEST_STAGING_METADATA.files[0].scan_date = "" mock_report_upload_failure = mocker.patch.object( - repo_under_test.dynamo_repository, "write_report_upload_to_dynamo" + repo_under_test.dynamo_repository, "write_report_upload_to_dynamo", ) repo_under_test.handle_sqs_message( - message={"body": json.dumps(modify_test_sqs_message)} + message={"body": json.dumps(modify_test_sqs_message)}, ) repo_under_test.dynamo_repository.write_report_upload_to_dynamo.assert_called() mock_report_upload_failure.assert_called_with( - TEST_STAGING_METADATA, UploadStatus.FAILED, "Invalid scan date format", "" + TEST_STAGING_METADATA, UploadStatus.FAILED, "Invalid scan date format", "", ) repo_under_test.sqs_repository.send_message_to_pdf_stitching_queue.assert_not_called() @@ -948,7 +949,7 @@ def test_raise_client_error_from_ssm_with_pds_service( mock_pds_validation_strict, ): mock_client_error = ClientError( - {"Error": {"Code": "500", "Message": "test error"}}, "testing" + {"Error": {"Code": "500", "Message": "test error"}}, "testing", ) mock_ods_validation.side_effect = mock_client_error with pytest.raises(ClientError): @@ -989,10 +990,10 @@ def test_create_lg_records_and_copy_files_client_error( ): TEST_STAGING_METADATA.retries = 0 mock_create_lg_records_and_copy_files = mocker.patch.object( - repo_under_test, "create_lg_records_and_copy_files" + repo_under_test, "create_lg_records_and_copy_files", ) mock_rollback_transaction = mocker.patch.object( - repo_under_test, "rollback_transaction" + repo_under_test, "rollback_transaction", ) mock_client_error = ClientError( {"Error": {"Code": "AccessDenied", "Message": "Access Denied"}}, @@ -1025,10 +1026,10 @@ def test_handle_sqs_message_happy_path_historical_name( ): TEST_STAGING_METADATA.retries = 0 mock_create_lg_records_and_copy_files = mocker.patch.object( - BulkUploadService, "create_lg_records_and_copy_files" + BulkUploadService, "create_lg_records_and_copy_files", ) mock_report_upload_complete = mocker.patch.object( - repo_under_test.dynamo_repository, "write_report_upload_to_dynamo" + repo_under_test.dynamo_repository, "write_report_upload_to_dynamo", ) mock_remove_ingested_file_from_source_bucket = mocker.patch.object( repo_under_test.bulk_upload_s3_repository, @@ -1040,7 +1041,7 @@ def test_handle_sqs_message_happy_path_historical_name( repo_under_test.handle_sqs_message(message=TEST_SQS_MESSAGE) mock_create_lg_records_and_copy_files.assert_called_with( - TEST_STAGING_METADATA, TEST_CURRENT_GP_ODS + TEST_STAGING_METADATA, TEST_CURRENT_GP_ODS, ) mock_report_upload_complete.assert_called() mock_report_upload_complete.assert_called_with( @@ -1068,19 +1069,19 @@ def test_handle_sqs_message_lenient_mode_happy_path( mocker.patch.object(service, "sqs_repository") mocker.patch.object(service, "bulk_upload_s3_repository") mock_create_lg_records_and_copy_files = mocker.patch.object( - BulkUploadService, "create_lg_records_and_copy_files" + BulkUploadService, "create_lg_records_and_copy_files", ) mock_report_upload_complete = mocker.patch.object( - service.dynamo_repository, "write_report_upload_to_dynamo" + service.dynamo_repository, "write_report_upload_to_dynamo", ) mock_remove_ingested_file_from_source_bucket = mocker.patch.object( - service.bulk_upload_s3_repository, "remove_ingested_file_from_source_bucket" + service.bulk_upload_s3_repository, "remove_ingested_file_from_source_bucket", ) mocker.patch.object(service.bulk_upload_s3_repository, "check_virus_result") service.handle_sqs_message(message=TEST_SQS_MESSAGE) mock_create_lg_records_and_copy_files.assert_called_with( - TEST_STAGING_METADATA, TEST_CURRENT_GP_ODS + TEST_STAGING_METADATA, TEST_CURRENT_GP_ODS, ) mock_pds_validation_lenient.assert_called() mock_pds_validation_strict.assert_not_called() @@ -1092,18 +1093,18 @@ def test_concatenate_acceptance_reason(repo_under_test): accepted_reason = None test_reason = "test_reason_1" actual_reason = repo_under_test.concatenate_acceptance_reason( - accepted_reason, test_reason + accepted_reason, test_reason, ) assert actual_reason == test_reason another_test_reason = "test_reason_2" another_actual_reason = repo_under_test.concatenate_acceptance_reason( - actual_reason, another_test_reason + actual_reason, another_test_reason, ) assert another_actual_reason == test_reason + ", " + another_test_reason def test_patient_not_found_is_caught_and_written_to_dynamo( - repo_under_test, mock_validate_files, mocker + repo_under_test, mock_validate_files, mocker, ): expected_error_message = "Could not find the given patient on PDS" mocker.patch( @@ -1167,7 +1168,7 @@ def test_check_file_tag_status_returns_empty_string_when_tag_missing(repo, file_ @pytest.mark.parametrize("fragment", ["AccessDenied", "NoSuchKey"]) def test_wraps_access_errors_as_s3_not_found(repo, file_key, fragment): repo.s3_repository.get_tag_value.side_effect = ClientError( - {"Error": {"Code": "S3Error", "Message": fragment}}, "GetObject" + {"Error": {"Code": "S3Error", "Message": fragment}}, "GetObject", ) with pytest.raises(S3FileNotFoundException): @@ -1197,10 +1198,10 @@ def test_handle_sqs_message_report_failure_when_pdf_is_corrupt( ): TEST_STAGING_METADATA.retries = 0 mock_report_upload_failure = mocker.patch.object( - repo_under_test.dynamo_repository, "write_report_upload_to_dynamo" + repo_under_test.dynamo_repository, "write_report_upload_to_dynamo", ) mock_create_lg_records_and_copy_files = mocker.patch.object( - BulkUploadService, "create_lg_records_and_copy_files" + BulkUploadService, "create_lg_records_and_copy_files", ) mock_remove_ingested_file_from_source_bucket = mocker.patch.object( repo_under_test.bulk_upload_s3_repository, @@ -1236,10 +1237,10 @@ def test_handle_sqs_message_report_failure_when_pdf_integrity_check_file_not_fou ): TEST_STAGING_METADATA.retries = 0 mock_report_upload_failure = mocker.patch.object( - repo_under_test.dynamo_repository, "write_report_upload_to_dynamo" + repo_under_test.dynamo_repository, "write_report_upload_to_dynamo", ) mock_create_lg_records_and_copy_files = mocker.patch.object( - BulkUploadService, "create_lg_records_and_copy_files" + BulkUploadService, "create_lg_records_and_copy_files", ) mock_remove_ingested_file_from_source_bucket = mocker.patch.object( repo_under_test.bulk_upload_s3_repository, @@ -1280,7 +1281,7 @@ def test_sends_demographic_error_when_flag_enabled(set_env, repo_with_review_ena def test_sends_to_review_queue_when_patient_not_found( - repo_with_review_enabled, mock_validate_files, mocker + repo_with_review_enabled, mock_validate_files, mocker, ): expected_error_message = "Could not find the given patient on PDS" mocker.patch( @@ -1304,10 +1305,10 @@ def test_sends_to_review_queue_when_patient_not_found( def test_sends_to_review_queue_when_invalid_files( - repo_with_review_enabled, mock_validate_files, mock_pds_service, mocker + repo_with_review_enabled, mock_validate_files, mock_pds_service, mocker, ): mocked_error = LGInvalidFilesException( - "One or more of the files do not match naming convention" + "One or more of the files do not match naming convention", ) mock_validate_files.side_effect = mocked_error @@ -1321,7 +1322,7 @@ def test_sends_to_review_queue_when_invalid_files( def test_does_not_send_to_review_queue_when_virus_scan_fails( - repo_with_review_enabled, mock_validate_files, mock_pds_service, mocker + repo_with_review_enabled, mock_validate_files, mock_pds_service, mocker, ): mocker.patch.object( repo_with_review_enabled.bulk_upload_s3_repository, @@ -1335,10 +1336,10 @@ def test_does_not_send_to_review_queue_when_virus_scan_fails( def test_does_not_send_to_review_queue_when_file_corrupted( - repo_with_review_enabled, mock_validate_files, mock_pds_service, mocker + repo_with_review_enabled, mock_validate_files, mock_pds_service, mocker, ): mocker.patch.object( - repo_with_review_enabled.bulk_upload_s3_repository, "check_virus_result" + repo_with_review_enabled.bulk_upload_s3_repository, "check_virus_result", ) mocker.patch.object( repo_with_review_enabled.bulk_upload_s3_repository, @@ -1352,7 +1353,7 @@ def test_does_not_send_to_review_queue_when_file_corrupted( def test_does_not_send_to_review_queue_when_s3_file_not_found( - repo_with_review_enabled, mock_validate_files, mock_pds_service, mocker + repo_with_review_enabled, mock_validate_files, mock_pds_service, mocker, ): mocker.patch.object( repo_with_review_enabled.bulk_upload_s3_repository, diff --git a/lambdas/tests/unit/services/test_staged_document_review_processing_service.py b/lambdas/tests/unit/services/test_staged_document_review_processing_service.py index 255aae1ab6..b399e6565a 100644 --- a/lambdas/tests/unit/services/test_staged_document_review_processing_service.py +++ b/lambdas/tests/unit/services/test_staged_document_review_processing_service.py @@ -1,5 +1,6 @@ import pytest from botocore.exceptions import ClientError + from enums.document_review_reason import DocumentReviewReason from enums.document_review_status import DocumentReviewStatus from enums.virus_scan_result import VirusScanResult @@ -21,10 +22,10 @@ @pytest.fixture def mock_service(set_env, mocker): mocker.patch( - "services.staged_document_review_processing_service.DocumentUploadReviewService" + "services.staged_document_review_processing_service.DocumentUploadReviewService", ) mocker.patch( - "services.staged_document_review_processing_service.get_virus_scan_service" + "services.staged_document_review_processing_service.get_virus_scan_service", ) mocker.patch("services.staged_document_review_processing_service.S3Service") service = StagedDocumentReviewProcessingService() @@ -34,13 +35,13 @@ def mock_service(set_env, mocker): @pytest.fixture def mock_review_document_service(mocker, mock_service): mocker.patch.object( - mock_service.review_document_service, "fetch_documents_from_table" + mock_service.review_document_service, "fetch_documents_from_table", ) mocker.patch.object( - mock_service.review_document_service, "update_document_review_status" + mock_service.review_document_service, "update_document_review_status", ) mocker.patch.object( - mock_service.review_document_service, "build_review_dynamo_filter" + mock_service.review_document_service, "build_review_dynamo_filter", ) yield mock_service.review_document_service @@ -71,7 +72,7 @@ def sample_document_reference(): DocumentReviewFileDetails( file_name="test-document.pdf", file_location=f"s3://{MOCK_STAGING_STORE_BUCKET}/test-upload-id/test-document.pdf", - ) + ), ], nhs_number=TEST_NHS_NUMBER, ) @@ -86,7 +87,7 @@ def test_handle_upload_document_reference_request_with_clean_virus_scan( ): object_key = "test-upload-id/test-document.pdf" mock_review_document_service.fetch_documents_from_table.return_value = [ - sample_document_reference + sample_document_reference, ] mock_virus_scan_service.scan_file.return_value = VirusScanResult.CLEAN.value mock_review_document_service.build_review_dynamo_filter.return_value = {} @@ -94,11 +95,11 @@ def test_handle_upload_document_reference_request_with_clean_virus_scan( mock_service.handle_upload_document_reference_request(object_key) mock_virus_scan_service.scan_file.assert_called_once_with( - object_key, nhs_number=TEST_NHS_NUMBER + object_key, nhs_number=TEST_NHS_NUMBER, ) mock_s3_service.copy_across_bucket.assert_called_once() mock_s3_service.delete_object.assert_called_once_with( - MOCK_STAGING_STORE_BUCKET, object_key + MOCK_STAGING_STORE_BUCKET, object_key, ) mock_review_document_service.update_document_review_status.assert_called_once() assert ( @@ -115,7 +116,7 @@ def test_handle_upload_document_reference_request_with_infected_file( ): object_key = "test-upload-id/test-document.pdf" mock_review_document_service.fetch_documents_from_table.return_value = [ - sample_document_reference + sample_document_reference, ] mock_virus_scan_service.scan_file.return_value = VirusScanResult.INFECTED.value @@ -150,7 +151,7 @@ def test_handle_upload_document_reference_request_with_other_status( object_key = "test-upload-id/test-document.pdf" sample_document_reference.review_status = DocumentReviewStatus.PENDING_REVIEW mock_review_document_service.fetch_documents_from_table.return_value = [ - sample_document_reference + sample_document_reference, ] mock_service.handle_upload_document_reference_request(object_key) @@ -158,7 +159,7 @@ def test_handle_upload_document_reference_request_with_other_status( mock_virus_scan_service.scan_file.assert_not_called() mock_s3_service.copy_across_bucket.assert_not_called() mock_s3_service.delete_object.assert_called_once_with( - MOCK_STAGING_STORE_BUCKET, object_key + MOCK_STAGING_STORE_BUCKET, object_key, ) @@ -196,7 +197,7 @@ def test_handle_upload_document_reference_request_with_client_errors( ): object_key = "test-upload-id/test-document.pdf" mock_review_document_service.fetch_documents_from_table.return_value = [ - sample_document_reference + sample_document_reference, ] mock_virus_scan_service.scan_file.return_value = VirusScanResult.CLEAN.value mock_review_document_service.build_review_dynamo_filter.return_value = {} @@ -224,10 +225,10 @@ def test_handle_upload_document_reference_request_with_unexpected_error( ): object_key = "test-upload-id/test-document.pdf" mock_review_document_service.fetch_documents_from_table.return_value = [ - sample_document_reference + sample_document_reference, ] mock_review_document_service.fetch_documents_from_table.side_effect = Exception( - "Unexpected error" + "Unexpected error", ) with pytest.raises(Exception): @@ -241,14 +242,14 @@ def test_fetch_document_reference_by_id( ): document_key = "test-upload-id" mock_review_document_service.fetch_documents_from_table.return_value = [ - sample_document_reference + sample_document_reference, ] result = mock_service._fetch_document_reference_by_id(document_key) assert result == sample_document_reference mock_review_document_service.fetch_documents_from_table.assert_called_once_with( - search_key="ID", search_condition=document_key + search_key="ID", search_condition=document_key, ) @@ -261,7 +262,7 @@ def test_handle_upload_document_reference_request_with_mismatched_object_key_and object_key = "test-upload-id/test-document.pdf" sample_document_reference.files[0].file_location = "s3://other-bucket/other-key.pdf" mock_review_document_service.fetch_documents_from_table.return_value = [ - sample_document_reference + sample_document_reference, ] mock_virus_scan_service.scan_file.return_value = VirusScanResult.CLEAN.value mock_service.handle_upload_document_reference_request(object_key) @@ -306,7 +307,7 @@ def test_perform_virus_scan( assert result == VirusScanResult.CLEAN.value mock_virus_scan_service.scan_file.assert_called_once_with( - object_key, nhs_number=TEST_NHS_NUMBER + object_key, nhs_number=TEST_NHS_NUMBER, ) @@ -320,13 +321,13 @@ def test_process_review_document_reference( mock_review_document_service.build_review_dynamo_filter.return_value = {} mock_service._process_review_document_reference( - sample_document_reference, object_key + sample_document_reference, object_key, ) mock_s3_service.copy_across_bucket.assert_called_once() mock_review_document_service.update_document_review_status.assert_called_once() mock_s3_service.delete_object.assert_called_once_with( - MOCK_STAGING_STORE_BUCKET, object_key + MOCK_STAGING_STORE_BUCKET, object_key, ) @@ -344,7 +345,7 @@ def test_copy_files_from_staging_bucket( ) mock_service.copy_files_from_staging_bucket( - sample_document_reference, source_file_key + sample_document_reference, source_file_key, ) mock_s3_service.copy_across_bucket.assert_called_once_with( @@ -374,7 +375,7 @@ def test_copy_files_from_staging_bucket_with_error( with pytest.raises(ClientError): mock_service.copy_files_from_staging_bucket( - sample_document_reference, source_file_key + sample_document_reference, source_file_key, ) @@ -394,7 +395,7 @@ def test_copy_files_from_staging_bucket_with_412_error( mock_s3_service.copy_across_bucket.side_effect = error try: mock_service.copy_files_from_staging_bucket( - sample_document_reference, source_file_key + sample_document_reference, source_file_key, ) except ClientError: assert False, "Exception should not be raised" @@ -417,7 +418,7 @@ def test_delete_file_from_staging_bucket( mock_service.delete_file_from_staging_bucket(source_file_key) mock_s3_service.delete_object.assert_called_once_with( - MOCK_STAGING_STORE_BUCKET, source_file_key + MOCK_STAGING_STORE_BUCKET, source_file_key, ) @@ -427,7 +428,7 @@ def test_delete_file_from_staging_bucket_with_error( ): source_file_key = "test-upload-id/test-document.pdf" error = ClientError( - {"Error": {"Code": "AccessDenied", "Message": "Access denied"}}, "delete_object" + {"Error": {"Code": "AccessDenied", "Message": "Access denied"}}, "delete_object", ) mock_s3_service.delete_object.side_effect = error From 490875472518afb060b8b6cdccde2dc8cf245394 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Mon, 2 Mar 2026 13:42:45 +0000 Subject: [PATCH 08/15] [PRMP-1505] formatting --- lambdas/handlers/get_report_by_ods_handler.py | 12 +++-- .../models/fhir/R4/fhir_document_reference.py | 4 +- .../test_get_report_by_ods_handler.py | 45 ++++++++++++++----- 3 files changed, 46 insertions(+), 15 deletions(-) diff --git a/lambdas/handlers/get_report_by_ods_handler.py b/lambdas/handlers/get_report_by_ods_handler.py index 0cb2ffbb7f..824bab8f48 100644 --- a/lambdas/handlers/get_report_by_ods_handler.py +++ b/lambdas/handlers/get_report_by_ods_handler.py @@ -57,7 +57,9 @@ def handle_api_gateway_request(event): pre_signed_url = create_report(ods_code, file_type, report_type) logger.info("A report has been successfully created.") return ApiGatewayResponse( - 200, json.dumps({"url": pre_signed_url}), "GET", + 200, + json.dumps({"url": pre_signed_url}), + "GET", ).create_api_gateway_response() @@ -94,8 +96,12 @@ def handle_manual_trigger(event): for ods_code in ods_codes: logger.info(f"Starting process for ods code: {ods_code}") service.generate_ods_report( - ods_code=ods_code, is_upload_to_s3_needed=True, file_type_output=file_type, + ods_code=ods_code, + is_upload_to_s3_needed=True, + file_type_output=file_type, ) return ApiGatewayResponse( - 200, "Successfully created report", "GET", + 200, + "Successfully created report", + "GET", ).create_api_gateway_response() diff --git a/lambdas/models/fhir/R4/fhir_document_reference.py b/lambdas/models/fhir/R4/fhir_document_reference.py index bcbb482b8b..00f5b8c638 100644 --- a/lambdas/models/fhir/R4/fhir_document_reference.py +++ b/lambdas/models/fhir/R4/fhir_document_reference.py @@ -222,7 +222,9 @@ def create_nrl_fhir_document_reference_object(self) -> DocumentReference: ], context=DocumentReferenceContext( practiceSetting=CodeableConcept( - coding=self._create_snomed_coding(self.snomed_code_practice_setting), + coding=self._create_snomed_coding( + self.snomed_code_practice_setting, + ), ), ), ) diff --git a/lambdas/tests/unit/handlers/test_get_report_by_ods_handler.py b/lambdas/tests/unit/handlers/test_get_report_by_ods_handler.py index 8de9a186e4..8f65010734 100644 --- a/lambdas/tests/unit/handlers/test_get_report_by_ods_handler.py +++ b/lambdas/tests/unit/handlers/test_get_report_by_ods_handler.py @@ -19,13 +19,17 @@ def mock_service(mocker): mock_service = mocker.Mock() mocker.patch( - "handlers.get_report_by_ods_handler.OdsReportService", return_value=mock_service, + "handlers.get_report_by_ods_handler.OdsReportService", + return_value=mock_service, ) return mock_service def test_lambda_handler_api_gateway_request( - mock_service, set_env, context, mock_jwt_encode, + mock_service, + set_env, + context, + mock_jwt_encode, ): event = { "httpMethod": "GET", @@ -34,7 +38,9 @@ def test_lambda_handler_api_gateway_request( } mock_service.generate_ods_report.return_value = "example.com/presigned-url" expected = ApiGatewayResponse( - 200, json.dumps({"url": "example.com/presigned-url"}), "GET", + 200, + json.dumps({"url": "example.com/presigned-url"}), + "GET", ).create_api_gateway_response() result = lambda_handler(event, context) @@ -49,7 +55,10 @@ def test_lambda_handler_api_gateway_request( def test_lambda_handler_gateway_request_review( - mock_service, set_env, context, mock_jwt_encode, + mock_service, + set_env, + context, + mock_jwt_encode, ): event = { "httpMethod": "GET", @@ -58,7 +67,9 @@ def test_lambda_handler_gateway_request_review( } mock_service.get_documents_for_review.return_value = "example.com/presigned-url" expected = ApiGatewayResponse( - 200, json.dumps({"url": "example.com/presigned-url"}), "GET", + 200, + json.dumps({"url": "example.com/presigned-url"}), + "GET", ).create_api_gateway_response() result = lambda_handler(event, context) @@ -71,7 +82,9 @@ def test_lambda_handler_manual_trigger(mock_service, set_env, context): event = {"odsCode": "ODS123,ODS456"} mock_service.generate_ods_report.return_value = None expected = ApiGatewayResponse( - 200, "Successfully created report", "GET", + 200, + "Successfully created report", + "GET", ).create_api_gateway_response() result = lambda_handler(event, context) @@ -79,10 +92,14 @@ def test_lambda_handler_manual_trigger(mock_service, set_env, context): assert result == expected assert mock_service.generate_ods_report.call_count == 2 mock_service.generate_ods_report.assert_any_call( - ods_code="ODS123", file_type_output="csv", is_upload_to_s3_needed=True, + ods_code="ODS123", + file_type_output="csv", + is_upload_to_s3_needed=True, ) mock_service.generate_ods_report.assert_any_call( - ods_code="ODS456", file_type_output="csv", is_upload_to_s3_needed=True, + ods_code="ODS456", + file_type_output="csv", + is_upload_to_s3_needed=True, ) @@ -129,7 +146,9 @@ def test_lambda_handler_manual_trigger(mock_service, set_env, context): ], ) def test_handle_api_gateway_request_handles_output_file_format( - mock_service, event, output, + mock_service, + event, + output, ): request_context.authorization = { "selected_organisation": {"org_ods_code": "ODS123"}, @@ -194,7 +213,9 @@ def test_handle_manual_trigger_single_ods_code(mock_service): event = {"odsCode": "ODS123", "outputFileFormat": "pdf"} mock_service.generate_ods_report.return_value = None expected = ApiGatewayResponse( - 200, "Successfully created report", "GET", + 200, + "Successfully created report", + "GET", ).create_api_gateway_response() result = handle_manual_trigger(event) @@ -202,7 +223,9 @@ def test_handle_manual_trigger_single_ods_code(mock_service): assert result == expected assert mock_service.generate_ods_report.call_count == 1 mock_service.generate_ods_report.assert_called_once_with( - ods_code="ODS123", file_type_output="pdf", is_upload_to_s3_needed=True, + ods_code="ODS123", + file_type_output="pdf", + is_upload_to_s3_needed=True, ) From 276b745a6b1e4a08c904c61cb10fae25e22e32d2 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Tue, 3 Mar 2026 13:41:08 +0000 Subject: [PATCH 09/15] [PRMP-1505] updated the way created day is calculated --- lambdas/services/ods_report_service.py | 2 +- lambdas/tests/unit/services/test_ods_report_service.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lambdas/services/ods_report_service.py b/lambdas/services/ods_report_service.py index 85dc49af2c..11d623042c 100644 --- a/lambdas/services/ods_report_service.py +++ b/lambdas/services/ods_report_service.py @@ -315,7 +315,7 @@ def build_patient_rows( continue if created_dt is not None and ( - existing["created"] is None or created_dt < existing["created"] + existing["created"] is None or created_dt > existing["created"] ): existing["created"] = created_dt diff --git a/lambdas/tests/unit/services/test_ods_report_service.py b/lambdas/tests/unit/services/test_ods_report_service.py index bb07285f03..abd0cb8b77 100644 --- a/lambdas/tests/unit/services/test_ods_report_service.py +++ b/lambdas/tests/unit/services/test_ods_report_service.py @@ -715,7 +715,7 @@ def test_build_patient_rows_dedupes_and_picks_earliest_created_and_latest_last_u assert set(rows.keys()) == {nhs, "9730786933"} - expected_created = datetime(2026, 2, 25, 12, 50, 35, tzinfo=timezone.utc) + expected_created = datetime(2026, 2, 25, 12, 50, 40, tzinfo=timezone.utc) expected_last_updated = datetime.fromtimestamp(1700000020, tz=timezone.utc) assert rows[nhs]["created"] == expected_created From 26f996d7914afeb3adc3f21973aea1302e228cc2 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Wed, 4 Mar 2026 08:54:04 +0000 Subject: [PATCH 10/15] [PRMP-1505] updated header row name --- lambdas/services/ods_report_service.py | 12 ++++++++---- .../tests/unit/services/test_ods_report_service.py | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/lambdas/services/ods_report_service.py b/lambdas/services/ods_report_service.py index 11d623042c..aa9667c667 100644 --- a/lambdas/services/ods_report_service.py +++ b/lambdas/services/ods_report_service.py @@ -338,7 +338,7 @@ def create_csv_report( f"Total number of patients for ODS code {ods_code}: {len(patient_rows)}\n", ) - f.write("NHS Number,Created Date,Last Updated Date\n") + f.write("NHS Number,Latest Created Date,Latest Updated Date\n") for nhs in sorted(patient_rows.keys()): row = patient_rows[nhs] created = datetime_to_utc_iso_string(row.get("created")) @@ -357,7 +357,7 @@ def create_xlsx_report( f"Total number of patients for ODS code {ods_code}: {len(patient_rows)}\n" ) - ws.append(["NHS number", "Created", "Last updated"]) + ws.append(["NHS Number", "Latest Created Date", "Latest Updated Date"]) for nhs in sorted(patient_rows.keys()): row = patient_rows[nhs] @@ -390,7 +390,7 @@ def create_pdf_report( c.setFont("Helvetica", 12) c.drawString(x, y, f"Total number of patients: {len(patient_rows)}") y -= 20 - c.drawString(x, y, "NHS Number | Created Date | Last Updated Date") + c.drawString(x, y, "NHS Number | Latest Created Date | Latest Updated Date") y -= 20 for nhs in sorted(patient_rows.keys()): if y < 40: @@ -405,7 +405,11 @@ def create_pdf_report( y = height - 50 y -= 40 - c.drawString(x, y, "NHS Number | Created Date| Last Updated Date") + c.drawString( + x, + y, + "NHS Number | Latest Created Date| Latest Updated Date", + ) y -= 20 row = patient_rows[nhs] diff --git a/lambdas/tests/unit/services/test_ods_report_service.py b/lambdas/tests/unit/services/test_ods_report_service.py index abd0cb8b77..147c60c676 100644 --- a/lambdas/tests/unit/services/test_ods_report_service.py +++ b/lambdas/tests/unit/services/test_ods_report_service.py @@ -402,7 +402,7 @@ def test_create_report_csv(ods_report_service, tmp_path): f"Total number of patients for ODS code {ods_code}: {len(patient_rows)}\n" in content ) - assert "NHS Number,Created Date,Last Updated Date\n" in content + assert "NHS Number,Latest Created Date,Latest Updated Date\n" in content def test_create_xlsx_report(ods_report_service, tmp_path): From 1e5a75fad544c0e8a7f5e3b0446702349a8e9e33 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Wed, 4 Mar 2026 13:38:18 +0000 Subject: [PATCH 11/15] Update lambdas/services/ods_report_service.py Co-authored-by: Mohammad Iqbal <127403145+MohammadIqbalAD-NHS@users.noreply.github.com> --- lambdas/services/ods_report_service.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lambdas/services/ods_report_service.py b/lambdas/services/ods_report_service.py index aa9667c667..84d196d98e 100644 --- a/lambdas/services/ods_report_service.py +++ b/lambdas/services/ods_report_service.py @@ -408,7 +408,7 @@ def create_pdf_report( c.drawString( x, y, - "NHS Number | Latest Created Date| Latest Updated Date", + "NHS Number | Latest Created Date | Latest Updated Date", ) y -= 20 From 839b708f19d85155509cd086ca32d13f2ab021ae Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Thu, 5 Mar 2026 09:31:30 +0000 Subject: [PATCH 12/15] [PRMP-1505] updated comments --- lambdas/services/ods_report_service.py | 34 ++++++++++++++------------ 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/lambdas/services/ods_report_service.py b/lambdas/services/ods_report_service.py index aa9667c667..5b3197ff70 100644 --- a/lambdas/services/ods_report_service.py +++ b/lambdas/services/ods_report_service.py @@ -296,6 +296,7 @@ def build_patient_rows( for item in document_items: nhs_number = item.get(DocumentReferenceMetadataFields.NHS_NUMBER.value) if not nhs_number: + logger.warning(f"No nhs number found in document_item: {item}") continue created_dt = iso_utc_string_to_datetime( @@ -305,25 +306,26 @@ def build_patient_rows( item.get(DocumentReferenceMetadataFields.LAST_UPDATED.value), ) - existing = rows.get(nhs_number) - if existing is None: + current_row_for_patient = rows.get(nhs_number) + if current_row_for_patient is None: rows[nhs_number] = { "nhs_number": nhs_number, - "created": created_dt, - "last_updated": updated_dt, + "latest_created_date": created_dt, + "latest_updated_date": updated_dt, } continue if created_dt is not None and ( - existing["created"] is None or created_dt > existing["created"] + current_row_for_patient["latest_created_date"] is None + or created_dt > current_row_for_patient["latest_created_date"] ): - existing["created"] = created_dt + current_row_for_patient["latest_created_date"] = created_dt if updated_dt is not None and ( - existing["last_updated"] is None - or updated_dt > existing["last_updated"] + current_row_for_patient["latest_updated_date"] is None + or updated_dt > current_row_for_patient["latest_updated_date"] ): - existing["last_updated"] = updated_dt + current_row_for_patient["latest_updated_date"] = updated_dt return rows @@ -341,8 +343,8 @@ def create_csv_report( f.write("NHS Number,Latest Created Date,Latest Updated Date\n") for nhs in sorted(patient_rows.keys()): row = patient_rows[nhs] - created = datetime_to_utc_iso_string(row.get("created")) - last_updated = datetime_to_utc_iso_string(row.get("last_updated")) + created = datetime_to_utc_iso_string(row.get("latest_created_date")) + last_updated = datetime_to_utc_iso_string(row.get("latest_updated_date")) f.write(f"{nhs},{created},{last_updated}\n") def create_xlsx_report( @@ -364,8 +366,8 @@ def create_xlsx_report( ws.append( [ row.get("nhs_number", ""), - datetime_to_utc_iso_string(row.get("created")), - datetime_to_utc_iso_string(row.get("last_updated")), + datetime_to_utc_iso_string(row.get("latest_created_date")), + datetime_to_utc_iso_string(row.get("latest_updated_date")), ], ) @@ -413,8 +415,8 @@ def create_pdf_report( y -= 20 row = patient_rows[nhs] - created = datetime_to_utc_iso_string(row.get("created")) - last_updated = datetime_to_utc_iso_string(row.get("last_updated")) + created = datetime_to_utc_iso_string(row.get("latest_created_date")) + last_updated = datetime_to_utc_iso_string(row.get("latest_updated_date")) line = f"{nhs} | {created} | {last_updated}" c.drawString(x, y, line[:120]) @@ -436,4 +438,4 @@ def get_pre_signed_url(self, ods_code: str, file_name: str): return self.s3_service.create_download_presigned_url( s3_bucket_name=self.reports_bucket, file_key=f"ods-reports/{ods_code}/{today.year}/{today.month}/{today.day}/{file_name}", - ) + ) \ No newline at end of file From 6ed16e0981df4a88f44bc6476a7b737a75611f2f Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Thu, 5 Mar 2026 09:31:54 +0000 Subject: [PATCH 13/15] [PRMP-1505] lint --- lambdas/services/ods_report_service.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lambdas/services/ods_report_service.py b/lambdas/services/ods_report_service.py index 7ff425d743..c8ab29ca49 100644 --- a/lambdas/services/ods_report_service.py +++ b/lambdas/services/ods_report_service.py @@ -344,7 +344,9 @@ def create_csv_report( for nhs in sorted(patient_rows.keys()): row = patient_rows[nhs] created = datetime_to_utc_iso_string(row.get("latest_created_date")) - last_updated = datetime_to_utc_iso_string(row.get("latest_updated_date")) + last_updated = datetime_to_utc_iso_string( + row.get("latest_updated_date"), + ) f.write(f"{nhs},{created},{last_updated}\n") def create_xlsx_report( @@ -438,4 +440,4 @@ def get_pre_signed_url(self, ods_code: str, file_name: str): return self.s3_service.create_download_presigned_url( s3_bucket_name=self.reports_bucket, file_key=f"ods-reports/{ods_code}/{today.year}/{today.month}/{today.day}/{file_name}", - ) \ No newline at end of file + ) From 83170a90a402bade393f68bb187bee353e8ed6d9 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Thu, 5 Mar 2026 09:35:40 +0000 Subject: [PATCH 14/15] [PRMP-1505] fixed tests --- lambdas/tests/unit/services/test_ods_report_service.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lambdas/tests/unit/services/test_ods_report_service.py b/lambdas/tests/unit/services/test_ods_report_service.py index 147c60c676..2fb4d6f954 100644 --- a/lambdas/tests/unit/services/test_ods_report_service.py +++ b/lambdas/tests/unit/services/test_ods_report_service.py @@ -718,8 +718,8 @@ def test_build_patient_rows_dedupes_and_picks_earliest_created_and_latest_last_u expected_created = datetime(2026, 2, 25, 12, 50, 40, tzinfo=timezone.utc) expected_last_updated = datetime.fromtimestamp(1700000020, tz=timezone.utc) - assert rows[nhs]["created"] == expected_created - assert rows[nhs]["last_updated"] == expected_last_updated + assert rows[nhs]["latest_created_date"] == expected_created + assert rows[nhs]["latest_updated_date"] == expected_last_updated def test_build_patient_rows_does_not_overwrite_existing_values_with_none( @@ -745,5 +745,5 @@ def test_build_patient_rows_does_not_overwrite_existing_values_with_none( expected_created = datetime(2026, 2, 25, 12, 50, 35, tzinfo=timezone.utc) expected_last_updated = datetime.fromtimestamp(1700000010, tz=timezone.utc) - assert rows[nhs]["created"] == expected_created - assert rows[nhs]["last_updated"] == expected_last_updated + assert rows[nhs]["latest_created_date"] == expected_created + assert rows[nhs]["latest_updated_date"] == expected_last_updated From 416ed3a80a388a60711334c7c9eee70c4cf9532c Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Thu, 5 Mar 2026 09:54:18 +0000 Subject: [PATCH 15/15] [PRMP-1505] removed a bit of duplicated code --- lambdas/services/ods_report_service.py | 38 ++++++++++++++++++++------ 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/lambdas/services/ods_report_service.py b/lambdas/services/ods_report_service.py index c8ab29ca49..ce4aec2b7d 100644 --- a/lambdas/services/ods_report_service.py +++ b/lambdas/services/ods_report_service.py @@ -89,6 +89,21 @@ def get_documents_for_review( upload_to_s3=True, ) + def _upload_and_presign_if_needed( + self, + *, + ods_code: str, + file_name: str, + local_file_path: str, + upload_to_s3: bool, + create_pre_signed_url: bool, + ): + if upload_to_s3: + self.save_report_to_s3(ods_code, file_name, local_file_path) + if create_pre_signed_url: + return self.get_pre_signed_url(ods_code, file_name) + return None + def create_and_save_review_report( self, ods_code: str, @@ -111,10 +126,13 @@ def create_and_save_review_report( f"Query completed. {len(review_rows)} items written to {file_name}.", ) - if upload_to_s3: - self.save_report_to_s3(ods_code, file_name, local_file_path) - if create_pre_signed_url: - return self.get_pre_signed_url(ods_code, file_name) + return self._upload_and_presign_if_needed( + ods_code=ods_code, + file_name=file_name, + local_file_path=local_file_path, + upload_to_s3=upload_to_s3, + create_pre_signed_url=create_pre_signed_url, + ) def scan_table_with_filter(self, ods_code: str): ods_codes = [ods_code] @@ -192,11 +210,13 @@ def create_and_save_ods_report( file_type_output=file_type_output, ) - if upload_to_s3: - self.save_report_to_s3(ods_code, file_name, local_file_path) - - if create_pre_signed_url: - return self.get_pre_signed_url(ods_code, file_name) + return self._upload_and_presign_if_needed( + ods_code=ods_code, + file_name=file_name, + local_file_path=local_file_path, + upload_to_s3=upload_to_s3, + create_pre_signed_url=create_pre_signed_url, + ) def create_ods_report( self,