From a544b36960f418710bcdc77db85296dae78bf395 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Mon, 23 Mar 2026 08:31:25 +0000 Subject: [PATCH 1/9] [PRMP-1615] Updated reports --- .../report/bulk_upload_report_output.py | 23 +++++++++++++++---- .../services/bulk_upload_report_service.py | 15 +++++++++++- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/lambdas/models/report/bulk_upload_report_output.py b/lambdas/models/report/bulk_upload_report_output.py index c81107a39..5df01b578 100644 --- a/lambdas/models/report/bulk_upload_report_output.py +++ b/lambdas/models/report/bulk_upload_report_output.py @@ -21,6 +21,7 @@ def __init__( self.total_deceased = set() self.total_restricted = set() self.total_ingested = set() + self.total_in_review = set() def get_total_successful_nhs_numbers(self) -> list: if self.total_successful: @@ -52,6 +53,16 @@ def get_total_restricted_count(self) -> int: def get_total_ingested_count(self) -> int: return len(self.total_ingested) + def get_total_in_review_count(self) -> int: + return len(self.total_in_review) + + def get_total_in_review_percentage(self) -> str: + review_percentage = "0" + if self.total_ingested: + review_rate = (len(self.total_in_review) / len(self.total_ingested)) * 100 + review_percentage = f"{review_rate:.2f}".rstrip("0").rstrip(".") + return f"{review_percentage}%" + @staticmethod def get_sorted(to_sort: set) -> list: return sorted(to_sort, key=lambda x: x[0]) if to_sort else [] @@ -144,9 +155,12 @@ def set_unique_failures(self): for patient in patients_to_remove: self.failures_per_patient.pop(patient) - for patient_data in self.failures_per_patient.values(): - reason = patient_data.get(MetadataReport.Reason) - self.unique_failures[reason] = self.unique_failures.get(reason, 0) + 1 + for nhs_number, patient_data in self.failures_per_patient.items(): + if patient_data.get(MetadataReport.SentToReview): + self.total_in_review.add(nhs_number) + else: + reason = patient_data.get(MetadataReport.Reason) + self.unique_failures[reason] = self.unique_failures.get(reason, 0) + 1 def get_unsuccessful_reasons_data_rows(self): return [ @@ -174,6 +188,7 @@ def populate_report(self): self.total_suspended.update(report.total_suspended) self.total_deceased.update(report.total_deceased) self.total_restricted.update(report.total_restricted) + self.total_in_review.update(report.total_in_review) ods_code_success_total[report.uploader_ods_code] = report.total_successful for reason, count in report.unique_failures.items(): @@ -191,4 +206,4 @@ def populate_report(self): ["Success by ODS", uploader_ods_code, len(nhs_numbers)], ) else: - self.success_summary.append(["Success by ODS", "No ODS codes found", 0]) + self.success_summary.append(["Success by ODS", "No ODS codes found", 0]) \ No newline at end of file diff --git a/lambdas/services/bulk_upload_report_service.py b/lambdas/services/bulk_upload_report_service.py index 51ddebbc2..9b61a5ad0 100644 --- a/lambdas/services/bulk_upload_report_service.py +++ b/lambdas/services/bulk_upload_report_service.py @@ -97,6 +97,8 @@ def generate_individual_ods_report( total_successful_percentage=ods_report.get_total_successful_percentage(), total_registered_elsewhere=ods_report.get_total_registered_elsewhere_count(), total_suspended=ods_report.get_total_suspended_count(), + total_in_review=ods_report.get_total_in_review_count(), + total_in_review_percentage=ods_report.get_total_in_review_percentage(), total_deceased=ods_report.get_total_deceased_count(), extra_rows=ods_report.get_unsuccessful_reasons_data_rows(), ) @@ -127,6 +129,8 @@ def generate_summary_report(self, ods_reports: list[OdsReport]): total_successful_percentage=summary_report.get_total_successful_percentage(), total_registered_elsewhere=summary_report.get_total_registered_elsewhere_count(), total_suspended=summary_report.get_total_suspended_count(), + total_in_review=summary_report.get_total_in_review_count(), + total_in_review_percentage=summary_report.get_total_in_review_percentage(), total_deceased=summary_report.get_total_deceased_count(), total_restricted=summary_report.get_total_restricted_count(), extra_rows=summary_report.success_summary + summary_report.reason_summary, @@ -335,6 +339,8 @@ def write_summary_data_to_csv( total_successful_percentage: str, total_registered_elsewhere: int, total_suspended: int, + total_in_review: int = None, + total_in_review_percentage: str = None, total_deceased: int = None, total_restricted: int = None, extra_rows: list = [], @@ -345,6 +351,13 @@ def write_summary_data_to_csv( writer.writerow(["Type", "Description", "Count"]) writer.writerow(["Total", "Total Ingested", total_ingested]) writer.writerow(["Total", "Total Successful", total_successful]) + + if total_in_review is not None: + writer.writerow(["Total", "Total In Review", total_in_review]) + + if total_in_review_percentage is not None: + writer.writerow(["Total", "Review Percentage", total_in_review_percentage]) + writer.writerow( ["Total", "Successful Percentage", total_successful_percentage], ) @@ -445,4 +458,4 @@ def write_and_upload_additional_reports( s3_bucket_name=self.reports_bucket, file_key=f"{self.s3_key_prefix}/{file_name}", file_name=f"/tmp/{file_name}", - ) + ) \ No newline at end of file From dcac0004d7cf63c51295cfeb3237cb33a5094569 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Mon, 23 Mar 2026 10:54:33 +0000 Subject: [PATCH 2/9] [PRMP-1615] added tests --- .../expected_bulk_upload_summary_report.csv | 2 + .../expected_ods_report_for_uploader_1.csv | 2 + .../expected_ods_report_for_uploader_2.csv | 2 + .../models/test_bulk_upload_report_output.py | 253 ++++++++++++++++++ .../test_bulk_upload_report_service.py | 116 +++++++- 5 files changed, 371 insertions(+), 4 deletions(-) diff --git a/lambdas/tests/unit/helpers/data/bulk_upload/expected_bulk_upload_summary_report.csv b/lambdas/tests/unit/helpers/data/bulk_upload/expected_bulk_upload_summary_report.csv index c70a5b8e0..12669f1f5 100644 --- a/lambdas/tests/unit/helpers/data/bulk_upload/expected_bulk_upload_summary_report.csv +++ b/lambdas/tests/unit/helpers/data/bulk_upload/expected_bulk_upload_summary_report.csv @@ -1,6 +1,8 @@ Type,Description,Count Total,Total Ingested,16 Total,Total Successful,10 +Total,Total In Review,0 +Total,Review Percentage,0% Total,Successful Percentage,62.5% Total,Successful - Registered Elsewhere,2 Total,Successful - Suspended,2 diff --git a/lambdas/tests/unit/helpers/data/bulk_upload/expected_ods_report_for_uploader_1.csv b/lambdas/tests/unit/helpers/data/bulk_upload/expected_ods_report_for_uploader_1.csv index 2d3d659cd..68169f046 100644 --- a/lambdas/tests/unit/helpers/data/bulk_upload/expected_ods_report_for_uploader_1.csv +++ b/lambdas/tests/unit/helpers/data/bulk_upload/expected_ods_report_for_uploader_1.csv @@ -1,6 +1,8 @@ Type,Description,Count Total,Total Ingested,8 Total,Total Successful,5 +Total,Total In Review,0 +Total,Review Percentage,0% Total,Successful Percentage,62.5% Total,Successful - Registered Elsewhere,1 Total,Successful - Suspended,1 diff --git a/lambdas/tests/unit/helpers/data/bulk_upload/expected_ods_report_for_uploader_2.csv b/lambdas/tests/unit/helpers/data/bulk_upload/expected_ods_report_for_uploader_2.csv index 2d3d659cd..68169f046 100644 --- a/lambdas/tests/unit/helpers/data/bulk_upload/expected_ods_report_for_uploader_2.csv +++ b/lambdas/tests/unit/helpers/data/bulk_upload/expected_ods_report_for_uploader_2.csv @@ -1,6 +1,8 @@ Type,Description,Count Total,Total Ingested,8 Total,Total Successful,5 +Total,Total In Review,0 +Total,Review Percentage,0% Total,Successful Percentage,62.5% Total,Successful - Registered Elsewhere,1 Total,Successful - Suspended,1 diff --git a/lambdas/tests/unit/models/test_bulk_upload_report_output.py b/lambdas/tests/unit/models/test_bulk_upload_report_output.py index 08db74d38..4dd728e21 100644 --- a/lambdas/tests/unit/models/test_bulk_upload_report_output.py +++ b/lambdas/tests/unit/models/test_bulk_upload_report_output.py @@ -116,6 +116,76 @@ def test_report_base_total_successful_percentage_returns_correct_single_percenta assert actual == expected +def test_report_base_get_total_in_review_percentage_returns_correct_percentage_to_two_decimal_places(): + base = ReportBase(generated_at=get_timestamp()) + base.total_ingested = { + "9000000000", + "9000000001", + "9000000002", + "9000000003", + "9000000004", + "9000000005", + "9000000006", + "9000000007", + "9000000008", + "9000000009", + "90000000010", + } + base.total_in_review = { + "9000000000", + "9000000003", + "9000000001", + "9000000002", + } + + expected = "36.36%" + actual = base.get_total_in_review_percentage() + assert actual == expected + + +def test_report_base_get_total_in_review_percentage_returns_correct_whole_percentage(): + base = ReportBase(generated_at=get_timestamp()) + base.total_ingested = { + "9000000000", + "9000000001", + "9000000002", + "9000000003", + "9000000004", + "9000000005", + "9000000006", + "9000000007", + "9000000008", + "9000000009", + } + base.total_in_review = { + "9000000000", + } + + expected = "10%" + actual = base.get_total_in_review_percentage() + assert actual == expected + + +def test_report_base_get_total_in_review_percentage_given_empty_input_returns_correctly(): + base = ReportBase(generated_at=get_timestamp()) + base.total_ingested = {} + base.total_in_review = {} + + expected = "0%" + actual = base.get_total_in_review_percentage() + assert actual == expected + + +def test_report_base_total_in_review_percentage_returns_correct_single_percentage(): + base = ReportBase(generated_at=get_timestamp()) + base.total_ingested = {f"{9000000000 + i}" for i in range(100)} + print(len(base.total_ingested)) + base.total_in_review = {"9000000000"} + expected = "1%" + actual = base.get_total_in_review_percentage() + assert actual == expected + + def test_report_base_get_sorted_sorts_successfully(): to_sort = { ("9000000000", "2012-01-13"), @@ -172,6 +242,17 @@ def test_ods_report_populate_report_populates_successfully(): ("9000000001", "2012-01-13", "Patient is deceased - INFORMAL", False), }, "total_restricted": {("9000000002", "2012-01-13", False)}, + "total_ingested": { + "9000000000", + "9000000001", + "9000000002", + "9000000003", + "9000000004", + "9000000005", + "9000000006", + "9000000007", + }, + "total_in_review": set(), "report_items": MOCK_REPORT_ITEMS_UPLOADER_1, "failures_per_patient": { "9000000005": { @@ -310,6 +391,7 @@ def test_ods_report_populate_report_empty_list_populates_successfully(): "total_suspended": set(), "total_deceased": set(), "total_restricted": set(), + "total_in_review": set(), "report_items": [], "failures_per_patient": {}, "unique_failures": {}, @@ -338,6 +420,8 @@ def test_ods_report_populate_report_returns_correct_statistics(): assert actual.get_total_suspended_count() == 1 assert actual.get_total_restricted_count() == 1 assert actual.get_total_registered_elsewhere_count() == 1 + assert actual.get_total_in_review_count() == 0 + assert actual.get_total_in_review_percentage() == "0%" def test_ods_report_populate_report_empty_list_returns_correct_statistics(): @@ -353,6 +437,99 @@ def test_ods_report_populate_report_empty_list_returns_correct_statistics(): assert actual.get_total_suspended_count() == 0 assert actual.get_total_restricted_count() == 0 assert actual.get_total_registered_elsewhere_count() == 0 + assert actual.get_total_in_review_count() == 0 + assert actual.get_total_in_review_percentage() == "0%" + + +def test_ods_report_populate_report_counts_reviewed_failures_and_excludes_them_from_reason_summary(): + test_items = [ + BulkUploadReport( + nhs_number="9000000020", + timestamp=1698661500, + date="2023-10-30", + upload_status=UploadStatus.FAILED, + file_path="/9000000020/1of1_Lloyd_George_Record_[Joe Bloggs]_[9000000020]_[25-12-2019].pdf", + reason="Could not find the given patient on PDS", + pds_ods_code=TEST_UPLOADER_ODS_1, + uploader_ods_code=TEST_UPLOADER_ODS_1, + sent_to_review=True, + ), + BulkUploadReport( + nhs_number="9000000021", + timestamp=1698661501, + date="2023-10-30", + upload_status=UploadStatus.FAILED, + file_path="/9000000021/1of1_Lloyd_George_Record_[Joe Bloggs]_[9000000021]_[25-12-2019].pdf", + reason="Lloyd George file already exists", + pds_ods_code=TEST_UPLOADER_ODS_1, + uploader_ods_code=TEST_UPLOADER_ODS_1, + sent_to_review=False, + ), + BulkUploadReport( + nhs_number="9000000022", + timestamp=1698661502, + date="2023-10-30", + upload_status=UploadStatus.FAILED, + file_path="/9000000022/1of1_Lloyd_George_Record_[Joe Bloggs]_[9000000022]_[25-12-2019].pdf", + reason="Invalid NHS number format", + pds_ods_code=TEST_UPLOADER_ODS_1, + uploader_ods_code=TEST_UPLOADER_ODS_1, + sent_to_review=True, + ), + ] + + actual = OdsReport( + get_timestamp(), + TEST_UPLOADER_ODS_1, + test_items, + ) + + assert actual.total_in_review == {"9000000020", "9000000022"} + assert actual.get_total_in_review_count() == 2 + assert actual.get_total_in_review_percentage() == "66.67%" + assert actual.unique_failures == {"Lloyd George file already exists": 1} + assert actual.get_unsuccessful_reasons_data_rows() == [ + [MetadataReport.Reason, "Lloyd George file already exists", 1], + ] + + +def test_ods_report_populate_report_same_patient_failed_to_review_then_succeeded_removes_from_review_and_reasons(): + test_items = [ + BulkUploadReport( + nhs_number="9000000030", + timestamp=1698661500, + date="2023-10-30", + upload_status=UploadStatus.FAILED, + file_path="/9000000030/1of1_Lloyd_George_Record_[Joe Bloggs]_[9000000030]_[25-12-2019].pdf", + reason="Could not find the given patient on PDS", + pds_ods_code=TEST_UPLOADER_ODS_1, + uploader_ods_code=TEST_UPLOADER_ODS_1, + sent_to_review=True, + ), + BulkUploadReport( + nhs_number="9000000030", + timestamp=1698661501, + date="2023-10-30", + upload_status=UploadStatus.COMPLETE, + file_path="/9000000030/1of1_Lloyd_George_Record_[Joe Bloggs]_[9000000030]_[25-12-2019].pdf", + reason="", + pds_ods_code=TEST_UPLOADER_ODS_1, + uploader_ods_code=TEST_UPLOADER_ODS_1, + sent_to_review=False, + ), + ] + + actual = OdsReport( + get_timestamp(), + TEST_UPLOADER_ODS_1, + test_items, + ) + + assert actual.failures_per_patient == {} + assert actual.total_in_review == set() + assert actual.unique_failures == {} + assert actual.get_total_in_review_count() == 0 + assert actual.get_total_in_review_percentage() == "0%" def test_summary_report_populate_report_populates_successfully(): @@ -414,6 +591,7 @@ def test_summary_report_populate_report_populates_successfully(): ("9000000002", "2012-01-13", False), ("9000000011", "2012-01-13", False), }, + "total_in_review": set(), "ods_reports": test_uploader_reports, "success_summary": [ ["Success by ODS", "Y12345", 5], @@ -457,6 +635,7 @@ def test_summary_report_populate_report_empty_reports_objects_populate_successfu "total_suspended": set(), "total_deceased": set(), "total_restricted": set(), + "total_in_review": set(), "ods_reports": test_uploader_reports, "success_summary": [ ["Success by ODS", "Y12345", 0], @@ -482,6 +661,7 @@ def test_summary_report_populate_report_no_report_objects_populate_successfully( "total_suspended": set(), "total_deceased": set(), "total_restricted": set(), + "total_in_review": set(), "ods_reports": [], "success_summary": [["Success by ODS", "No ODS codes found", 0]], "reason_summary": [], @@ -490,3 +670,76 @@ def test_summary_report_populate_report_no_report_objects_populate_successfully( actual = SummaryReport(generated_at=get_timestamp(), ods_reports=[]).__dict__ assert actual == expected + + +def test_summary_report_populate_report_aggregates_total_in_review_and_only_non_review_reasons(): + uploader_1_report = OdsReport( + get_timestamp(), + TEST_UPLOADER_ODS_1, + [ + BulkUploadReport( + nhs_number="9000000040", + timestamp=1698661500, + date="2023-10-30", + upload_status=UploadStatus.FAILED, + file_path="/9000000040/1of1_Lloyd_George_Record_[Joe Bloggs]_[9000000040]_[25-12-2019].pdf", + reason="Could not find the given patient on PDS", + pds_ods_code=TEST_UPLOADER_ODS_1, + uploader_ods_code=TEST_UPLOADER_ODS_1, + sent_to_review=True, + ), + BulkUploadReport( + nhs_number="9000000041", + timestamp=1698661501, + date="2023-10-30", + upload_status=UploadStatus.FAILED, + file_path="/9000000041/1of1_Lloyd_George_Record_[Joe Bloggs]_[9000000041]_[25-12-2019].pdf", + reason="Lloyd George file already exists", + pds_ods_code=TEST_UPLOADER_ODS_1, + uploader_ods_code=TEST_UPLOADER_ODS_1, + sent_to_review=False, + ), + ], + ) + + uploader_2_report = OdsReport( + get_timestamp(), + TEST_UPLOADER_ODS_2, + [ + BulkUploadReport( + nhs_number="9000000050", + timestamp=1698661502, + date="2023-10-30", + upload_status=UploadStatus.FAILED, + file_path="/9000000050/1of1_Lloyd_George_Record_[Joe Bloggs]_[9000000050]_[25-12-2019].pdf", + reason="Invalid NHS number format", + pds_ods_code=TEST_UPLOADER_ODS_2, + uploader_ods_code=TEST_UPLOADER_ODS_2, + sent_to_review=True, + ), + BulkUploadReport( + nhs_number="9000000051", + timestamp=1698661503, + date="2023-10-30", + upload_status=UploadStatus.FAILED, + file_path="/9000000051/1of1_Lloyd_George_Record_[Joe Bloggs]_[9000000051]_[25-12-2019].pdf", + reason="Fail to parse the patient detail response from PDS API.", + pds_ods_code=TEST_UPLOADER_ODS_2, + uploader_ods_code=TEST_UPLOADER_ODS_2, + sent_to_review=False, + ), + ], + ) + + actual = SummaryReport( + generated_at=get_timestamp(), + ods_reports=[uploader_1_report, uploader_2_report], + ) + + assert actual.total_in_review == {"9000000040", "9000000050"} + assert actual.get_total_in_review_count() == 2 + assert actual.get_total_in_review_percentage() == "50%" + assert actual.reason_summary == [ + ["Reason for Y12345", "Lloyd George file already exists", 1], + ["Reason for Z12345", "Fail to parse the patient detail response from PDS API.", 1], + ] \ No newline at end of file diff --git a/lambdas/tests/unit/services/test_bulk_upload_report_service.py b/lambdas/tests/unit/services/test_bulk_upload_report_service.py index ab0eaff6c..f683c97d5 100644 --- a/lambdas/tests/unit/services/test_bulk_upload_report_service.py +++ b/lambdas/tests/unit/services/test_bulk_upload_report_service.py @@ -466,7 +466,6 @@ def test_write_items_to_csv_with_empty_list_writes_only_header( with open(csv_path, newline="") as f: reader = csv.reader(f) lines = list(reader) - # Only header line should be present assert len(lines) == 1 expected_headers = [ MetadataReport.NhsNumber, @@ -482,6 +481,79 @@ def test_write_items_to_csv_with_empty_list_writes_only_header( assert lines[0] == expected_headers +def test_write_summary_data_to_csv_writes_review_rows_and_reason_rows( + bulk_upload_report_service, +): + mock_file_name = "test_summary_report.csv" + + bulk_upload_report_service.write_summary_data_to_csv( + file_name=mock_file_name, + total_ingested=10, + total_successful=3, + total_successful_percentage="30%", + total_registered_elsewhere=1, + total_suspended=0, + total_in_review=6, + total_in_review_percentage="60%", + total_deceased=0, + total_restricted=0, + extra_rows=[ + ["Reason", "Fail to parse the patient detail response from PDS API.", 1], + ], + ) + + with open(f"/tmp/{mock_file_name}", newline="") as f: + rows = list(csv.reader(f)) + + assert rows == [ + ["Type", "Description", "Count"], + ["Total", "Total Ingested", "10"], + ["Total", "Total Successful", "3"], + ["Total", "Total In Review", "6"], + ["Total", "Review Percentage", "60%"], + ["Total", "Successful Percentage", "30%"], + ["Total", "Successful - Registered Elsewhere", "1"], + ["Total", "Successful - Suspended", "0"], + ["Reason", "Fail to parse the patient detail response from PDS API.", "1"], + ] + + os.remove(f"/tmp/{mock_file_name}") + + +def test_write_summary_data_to_csv_does_not_write_reason_rows_when_extra_rows_empty( + bulk_upload_report_service, +): + mock_file_name = "test_summary_report_no_reasons.csv" + + bulk_upload_report_service.write_summary_data_to_csv( + file_name=mock_file_name, + total_ingested=18, + total_successful=0, + total_successful_percentage="0%", + total_registered_elsewhere=0, + total_suspended=0, + total_in_review=18, + total_in_review_percentage="100%", + extra_rows=[], + ) + + with open(f"/tmp/{mock_file_name}", newline="") as f: + rows = list(csv.reader(f)) + + assert rows == [ + ["Type", "Description", "Count"], + ["Total", "Total Ingested", "18"], + ["Total", "Total Successful", "0"], + ["Total", "Total In Review", "18"], + ["Total", "Review Percentage", "100%"], + ["Total", "Successful Percentage", "0%"], + ["Total", "Successful - Registered Elsewhere", "0"], + ["Total", "Successful - Suspended", "0"], + ] + + os.remove(f"/tmp/{mock_file_name}") + + def test_generate_individual_ods_report_creates_ods_report( bulk_upload_report_service, mock_write_summary_data_to_csv, @@ -507,6 +579,8 @@ def test_generate_individual_ods_report_creates_ods_report( total_successful_percentage="62.5%", total_registered_elsewhere=1, total_suspended=1, + total_in_review=0, + total_in_review_percentage="0%", total_deceased=1, extra_rows=[ ["Reason", "Could not find the given patient on PDS", 2], @@ -586,6 +660,42 @@ def test_generate_summary_report_with_two_ods_reports( os.remove(f"/tmp/{mock_file_name}") +def test_generate_summary_report_passes_review_counts_to_csv_writer( + bulk_upload_report_service, + mock_get_times_for_scan, + mocker, +): + ods_reports = bulk_upload_report_service.generate_ods_reports(MOCK_REPORT_ITEMS_ALL) + + mock_write_summary_data_to_csv = mocker.patch.object( + bulk_upload_report_service, + "write_summary_data_to_csv", + ) + + bulk_upload_report_service.generate_summary_report(ods_reports) + + mock_write_summary_data_to_csv.assert_called_once_with( + file_name=f"daily_statistical_report_bulk_upload_summary_{MOCK_TIMESTAMP}.csv", + total_ingested=16, + total_successful=10, + total_successful_percentage="62.5%", + total_registered_elsewhere=2, + total_suspended=2, + total_in_review=0, + total_in_review_percentage="0%", + total_deceased=2, + total_restricted=2, + extra_rows=[ + ["Success by ODS", "Y12345", 5], + ["Success by ODS", "Z12345", 5], + ["Reason for Y12345", "Could not find the given patient on PDS", 2], + ["Reason for Y12345", "Lloyd George file already exists", 1], + ["Reason for Z12345", "Could not find the given patient on PDS", 2], + ["Reason for Z12345", "Lloyd George file already exists", 1], + ], + ) + + def test_generate_success_report_writes_csv( bulk_upload_report_service, mock_get_times_for_scan, @@ -647,8 +757,6 @@ def test_generate_suspended_report_does_not_write_when_no_data( ): bulk_upload_report_service.write_and_upload_additional_reports = mocker.MagicMock() - # just used to assert this isn't created - blank_ods_reports = bulk_upload_report_service.generate_ods_reports([]) bulk_upload_report_service.generate_suspended_report(blank_ods_reports) @@ -839,4 +947,4 @@ def test_write_and_upload_additional_reports_creates_csv_and_writes_to_s3( s3_bucket_name=MOCK_STATISTICS_REPORT_BUCKET_NAME, file_key=f"bulk-upload-reports/2012-01-13/{mock_file_name}", file_name=f"/tmp/{mock_file_name}", - ) + ) \ No newline at end of file From 076ab5372ffd93ca965b219bcbd7b63f3f39d975 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Mon, 23 Mar 2026 10:56:50 +0000 Subject: [PATCH 3/9] [PRMP-1615] linting --- lambdas/models/report/bulk_upload_report_output.py | 2 +- lambdas/services/bulk_upload_report_service.py | 6 ++++-- .../tests/unit/models/test_bulk_upload_report_output.py | 8 ++++++-- .../unit/services/test_bulk_upload_report_service.py | 2 +- 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/lambdas/models/report/bulk_upload_report_output.py b/lambdas/models/report/bulk_upload_report_output.py index 5df01b578..222e8a968 100644 --- a/lambdas/models/report/bulk_upload_report_output.py +++ b/lambdas/models/report/bulk_upload_report_output.py @@ -206,4 +206,4 @@ def populate_report(self): ["Success by ODS", uploader_ods_code, len(nhs_numbers)], ) else: - self.success_summary.append(["Success by ODS", "No ODS codes found", 0]) \ No newline at end of file + self.success_summary.append(["Success by ODS", "No ODS codes found", 0]) diff --git a/lambdas/services/bulk_upload_report_service.py b/lambdas/services/bulk_upload_report_service.py index 9b61a5ad0..2f557cfeb 100644 --- a/lambdas/services/bulk_upload_report_service.py +++ b/lambdas/services/bulk_upload_report_service.py @@ -356,7 +356,9 @@ def write_summary_data_to_csv( writer.writerow(["Total", "Total In Review", total_in_review]) if total_in_review_percentage is not None: - writer.writerow(["Total", "Review Percentage", total_in_review_percentage]) + writer.writerow( + ["Total", "Review Percentage", total_in_review_percentage], + ) writer.writerow( ["Total", "Successful Percentage", total_successful_percentage], @@ -458,4 +460,4 @@ def write_and_upload_additional_reports( s3_bucket_name=self.reports_bucket, file_key=f"{self.s3_key_prefix}/{file_name}", file_name=f"/tmp/{file_name}", - ) \ No newline at end of file + ) diff --git a/lambdas/tests/unit/models/test_bulk_upload_report_output.py b/lambdas/tests/unit/models/test_bulk_upload_report_output.py index 4dd728e21..f7af68eab 100644 --- a/lambdas/tests/unit/models/test_bulk_upload_report_output.py +++ b/lambdas/tests/unit/models/test_bulk_upload_report_output.py @@ -741,5 +741,9 @@ def test_summary_report_populate_report_aggregates_total_in_review_and_only_non_ assert actual.get_total_in_review_percentage() == "50%" assert actual.reason_summary == [ ["Reason for Y12345", "Lloyd George file already exists", 1], - ["Reason for Z12345", "Fail to parse the patient detail response from PDS API.", 1], - ] \ No newline at end of file + [ + "Reason for Z12345", + "Fail to parse the patient detail response from PDS API.", + 1, + ], + ] diff --git a/lambdas/tests/unit/services/test_bulk_upload_report_service.py b/lambdas/tests/unit/services/test_bulk_upload_report_service.py index f683c97d5..70eb07d90 100644 --- a/lambdas/tests/unit/services/test_bulk_upload_report_service.py +++ b/lambdas/tests/unit/services/test_bulk_upload_report_service.py @@ -947,4 +947,4 @@ def test_write_and_upload_additional_reports_creates_csv_and_writes_to_s3( s3_bucket_name=MOCK_STATISTICS_REPORT_BUCKET_NAME, file_key=f"bulk-upload-reports/2012-01-13/{mock_file_name}", file_name=f"/tmp/{mock_file_name}", - ) \ No newline at end of file + ) From 87297fb3bf04ebb08c86585add36e690cdd92b67 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Mon, 23 Mar 2026 15:55:49 +0000 Subject: [PATCH 4/9] [PRMP-1615] linting --- .../unit/models/test_bulk_upload_report_output.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/lambdas/tests/unit/models/test_bulk_upload_report_output.py b/lambdas/tests/unit/models/test_bulk_upload_report_output.py index f7af68eab..4e3c970e2 100644 --- a/lambdas/tests/unit/models/test_bulk_upload_report_output.py +++ b/lambdas/tests/unit/models/test_bulk_upload_report_output.py @@ -242,16 +242,6 @@ def test_ods_report_populate_report_populates_successfully(): ("9000000001", "2012-01-13", "Patient is deceased - INFORMAL", False), }, "total_restricted": {("9000000002", "2012-01-13", False)}, - "total_ingested": { - "9000000000", - "9000000001", - "9000000002", - "9000000003", - "9000000004", - "9000000005", - "9000000006", - "9000000007", - }, "total_in_review": set(), "report_items": MOCK_REPORT_ITEMS_UPLOADER_1, "failures_per_patient": { From 5995b600b7819b8f1b8210fdf1b4723252c36ace Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Thu, 26 Mar 2026 11:22:22 +0000 Subject: [PATCH 5/9] [PRMP-1635] testing updated report --- .../excel_report_generator_service.py | 63 ++++++++++++++++--- .../reporting/report_orchestration_service.py | 34 +++++++--- 2 files changed, 80 insertions(+), 17 deletions(-) diff --git a/lambdas/services/reporting/excel_report_generator_service.py b/lambdas/services/reporting/excel_report_generator_service.py index 40e4a88ef..1afa4de78 100644 --- a/lambdas/services/reporting/excel_report_generator_service.py +++ b/lambdas/services/reporting/excel_report_generator_service.py @@ -1,6 +1,9 @@ from datetime import datetime, timezone from openpyxl.workbook import Workbook + +from models.report.bulk_upload_report import BulkUploadReport +from models.report.bulk_upload_report_output import OdsReport from utils.audit_logging_setup import LoggingService logger = LoggingService(__name__) @@ -10,18 +13,23 @@ class ExcelReportGenerator: def create_report_orchestration_xlsx( self, ods_code: str, - records: list[dict], + records: list[BulkUploadReport], output_path: str, ) -> str: logger.info( f"Creating Excel report for ODS code {ods_code} and records {len(records)}", ) + ods_report = OdsReport( + generated_at=datetime.now(timezone.utc).strftime("%Y%m%d"), + uploader_ods_code=ods_code, + report_items=records, + ) + wb = Workbook() ws = wb.active ws.title = "Daily Upload Report" - # Report metadata ws.append([f"ODS Code: {ods_code}"]) ws.append([f"Generated at (UTC): {datetime.now(timezone.utc).isoformat()}"]) ws.append([]) @@ -34,6 +42,7 @@ def create_report_orchestration_xlsx( "PDS ODS", "Upload Status", "Reason", + "Sent to Review", "File Path", ], ) @@ -41,16 +50,50 @@ def create_report_orchestration_xlsx( for record in records: ws.append( [ - record.get("NhsNumber"), - record.get("Date"), - record.get("UploaderOdsCode"), - record.get("PdsOdsCode"), - record.get("UploadStatus"), - record.get("Reason"), - record.get("FilePath"), + record.nhs_number, + record.date, + record.uploader_ods_code, + record.pds_ods_code, + record.upload_status, + record.reason, + record.sent_to_review, + record.file_path, ], ) + summary_ws = wb.create_sheet(title="Summary") + + summary_ws.append(["Type", "Description", "Count"]) + summary_ws.append(["Total", "Total Ingested", ods_report.get_total_ingested_count()]) + summary_ws.append(["Total", "Total Successful", ods_report.get_total_successful()]) + summary_ws.append(["Total", "Total In Review", ods_report.get_total_in_review_count()]) + summary_ws.append(["Total", "Review Percentage", ods_report.get_total_in_review_percentage()]) + summary_ws.append(["Total", "Successful Percentage", ods_report.get_total_successful_percentage()]) + summary_ws.append( + [ + "Total", + "Successful - Registered Elsewhere", + ods_report.get_total_registered_elsewhere_count(), + ], + ) + summary_ws.append( + ["Total", "Successful - Suspended", ods_report.get_total_suspended_count()], + ) + + if ods_report.get_total_deceased_count(): + summary_ws.append( + ["Total", "Successful - Deceased", ods_report.get_total_deceased_count()], + ) + + if ods_report.get_total_restricted_count(): + summary_ws.append( + ["Total", "Successful - Restricted", ods_report.get_total_restricted_count()], + ) + + for row in ods_report.get_unsuccessful_reasons_data_rows(): + summary_ws.append(row) + wb.save(output_path) + logger.info(f"Excel report written successfully for ods code {ods_code}") - return output_path + return output_path \ No newline at end of file diff --git a/lambdas/services/reporting/report_orchestration_service.py b/lambdas/services/reporting/report_orchestration_service.py index 45a5274ba..4d05babf0 100644 --- a/lambdas/services/reporting/report_orchestration_service.py +++ b/lambdas/services/reporting/report_orchestration_service.py @@ -1,7 +1,10 @@ import tempfile from collections import defaultdict -from typing import Dict +from typing import Dict, List +from pydantic import ValidationError + +from models.report.bulk_upload_report import BulkUploadReport from repositories.reporting.reporting_dynamo_repository import ReportingDynamoRepository from services.reporting.excel_report_generator_service import ExcelReportGenerator from utils.audit_logging_setup import LoggingService @@ -19,15 +22,26 @@ def process_reporting_window( window_start_ts: int, window_end_ts: int, ) -> Dict[str, str]: - records = self.repository.get_records_for_time_window( + raw_records = self.repository.get_records_for_time_window( window_start_ts, window_end_ts, ) - if not records: + if not raw_records: logger.info("No records found for reporting window") return {} + records: List[BulkUploadReport] = [] + for record in raw_records: + try: + records.append(BulkUploadReport.model_validate(record)) + except ValidationError as e: + logger.error(f"Skipping invalid record in reporting orchestration: {e}") + + if not records: + logger.info("No valid records after validation") + return {} + records_by_ods = self.group_records_by_ods(records) generated_files: Dict[str, str] = {} @@ -42,14 +56,20 @@ def process_reporting_window( return generated_files @staticmethod - def group_records_by_ods(records: list[dict]) -> dict[str, list[dict]]: + def group_records_by_ods( + records: List[BulkUploadReport], + ) -> dict[str, List[BulkUploadReport]]: grouped = defaultdict(list) for record in records: - ods_code = record.get("UploaderOdsCode") or "UNKNOWN" + ods_code = record.uploader_ods_code or "UNKNOWN" grouped[ods_code].append(record) return grouped - def generate_ods_report(self, ods_code: str, records: list[dict]) -> str: + def generate_ods_report( + self, + ods_code: str, + records: List[BulkUploadReport], + ) -> str: with tempfile.NamedTemporaryFile( suffix=f"_{ods_code}.xlsx", delete=False, @@ -59,4 +79,4 @@ def generate_ods_report(self, ods_code: str, records: list[dict]) -> str: records=records, output_path=tmp.name, ) - return tmp.name + return tmp.name \ No newline at end of file From 654b429bef9bcfd94255df9c24f3be83d36eacb5 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Fri, 27 Mar 2026 11:04:24 +0000 Subject: [PATCH 6/9] [PRMP-1635] refactored and updated tests --- .../excel_report_generator_service.py | 144 ++++++++++++--- .../test_excel_report_generator_service.py | 168 ++++++++++++++---- .../test_report_orchestration_service.py | 156 ++++++++++++---- 3 files changed, 370 insertions(+), 98 deletions(-) diff --git a/lambdas/services/reporting/excel_report_generator_service.py b/lambdas/services/reporting/excel_report_generator_service.py index 1afa4de78..afab84519 100644 --- a/lambdas/services/reporting/excel_report_generator_service.py +++ b/lambdas/services/reporting/excel_report_generator_service.py @@ -20,21 +20,57 @@ def create_report_orchestration_xlsx( f"Creating Excel report for ODS code {ods_code} and records {len(records)}", ) - ods_report = OdsReport( - generated_at=datetime.now(timezone.utc).strftime("%Y%m%d"), + generated_at = datetime.now(timezone.utc) + ods_report = self._build_ods_report(ods_code, records, generated_at) + + wb = Workbook() + self._create_detail_sheet(wb, ods_code, records, generated_at) + self._create_summary_sheet(wb, ods_report) + + wb.save(output_path) + + logger.info(f"Excel report written successfully for ods code {ods_code}") + return output_path + + @staticmethod + def _build_ods_report( + ods_code: str, + records: list[BulkUploadReport], + generated_at: datetime, + ) -> OdsReport: + return OdsReport( + generated_at=generated_at.strftime("%Y%m%d"), uploader_ods_code=ods_code, report_items=records, ) - wb = Workbook() - ws = wb.active + def _create_detail_sheet( + self, + workbook: Workbook, + ods_code: str, + records: list[BulkUploadReport], + generated_at: datetime, + ) -> None: + ws = workbook.active ws.title = "Daily Upload Report" - ws.append([f"ODS Code: {ods_code}"]) - ws.append([f"Generated at (UTC): {datetime.now(timezone.utc).isoformat()}"]) - ws.append([]) + self._append_detail_sheet_metadata(ws, ods_code, generated_at) + self._append_detail_sheet_headers(ws) + self._append_detail_sheet_rows(ws, records) - ws.append( + @staticmethod + def _append_detail_sheet_metadata( + worksheet, + ods_code: str, + generated_at: datetime, + ) -> None: + worksheet.append([f"ODS Code: {ods_code}"]) + worksheet.append([f"Generated at (UTC): {generated_at.isoformat()}"]) + worksheet.append([]) + + @staticmethod + def _append_detail_sheet_headers(worksheet) -> None: + worksheet.append( [ "NHS Number", "Date", @@ -47,8 +83,13 @@ def create_report_orchestration_xlsx( ], ) + @staticmethod + def _append_detail_sheet_rows( + worksheet, + records: list[BulkUploadReport], + ) -> None: for record in records: - ws.append( + worksheet.append( [ record.nhs_number, record.date, @@ -61,39 +102,84 @@ def create_report_orchestration_xlsx( ], ) - summary_ws = wb.create_sheet(title="Summary") - - summary_ws.append(["Type", "Description", "Count"]) - summary_ws.append(["Total", "Total Ingested", ods_report.get_total_ingested_count()]) - summary_ws.append(["Total", "Total Successful", ods_report.get_total_successful()]) - summary_ws.append(["Total", "Total In Review", ods_report.get_total_in_review_count()]) - summary_ws.append(["Total", "Review Percentage", ods_report.get_total_in_review_percentage()]) - summary_ws.append(["Total", "Successful Percentage", ods_report.get_total_successful_percentage()]) - summary_ws.append( + def _create_summary_sheet( + self, + workbook: Workbook, + ods_report: OdsReport, + ) -> None: + summary_ws = workbook.create_sheet(title="Summary") + + self._append_summary_headers(summary_ws) + self._append_summary_totals(summary_ws, ods_report) + self._append_summary_optional_totals(summary_ws, ods_report) + self._append_summary_reason_rows(summary_ws, ods_report) + + @staticmethod + def _append_summary_headers(worksheet) -> None: + worksheet.append(["Type", "Description", "Count"]) + + @staticmethod + def _append_summary_totals( + worksheet, + ods_report: OdsReport, + ) -> None: + worksheet.append( + ["Total", "Total Ingested", ods_report.get_total_ingested_count()], + ) + worksheet.append( + ["Total", "Total Successful", ods_report.get_total_successful()], + ) + worksheet.append( + ["Total", "Total In Review", ods_report.get_total_in_review_count()], + ) + worksheet.append( + ["Total", "Review Percentage", ods_report.get_total_in_review_percentage()], + ) + worksheet.append( + [ + "Total", + "Successful Percentage", + ods_report.get_total_successful_percentage(), + ], + ) + worksheet.append( [ "Total", "Successful - Registered Elsewhere", ods_report.get_total_registered_elsewhere_count(), ], ) - summary_ws.append( - ["Total", "Successful - Suspended", ods_report.get_total_suspended_count()], + worksheet.append( + [ + "Total", + "Successful - Suspended", + ods_report.get_total_suspended_count(), + ], ) + @staticmethod + def _append_summary_optional_totals( + worksheet, + ods_report: OdsReport, + ) -> None: if ods_report.get_total_deceased_count(): - summary_ws.append( + worksheet.append( ["Total", "Successful - Deceased", ods_report.get_total_deceased_count()], ) if ods_report.get_total_restricted_count(): - summary_ws.append( - ["Total", "Successful - Restricted", ods_report.get_total_restricted_count()], + worksheet.append( + [ + "Total", + "Successful - Restricted", + ods_report.get_total_restricted_count(), + ], ) + @staticmethod + def _append_summary_reason_rows( + worksheet, + ods_report: OdsReport, + ) -> None: for row in ods_report.get_unsuccessful_reasons_data_rows(): - summary_ws.append(row) - - wb.save(output_path) - - logger.info(f"Excel report written successfully for ods code {ods_code}") - return output_path \ No newline at end of file + worksheet.append(row) \ No newline at end of file diff --git a/lambdas/tests/unit/services/reporting/test_excel_report_generator_service.py b/lambdas/tests/unit/services/reporting/test_excel_report_generator_service.py index 30a46c3ba..7f2f4fe1e 100644 --- a/lambdas/tests/unit/services/reporting/test_excel_report_generator_service.py +++ b/lambdas/tests/unit/services/reporting/test_excel_report_generator_service.py @@ -1,6 +1,9 @@ +from types import SimpleNamespace + import pytest from freezegun import freeze_time from openpyxl import load_workbook + from services.reporting.excel_report_generator_service import ExcelReportGenerator @@ -10,11 +13,62 @@ def excel_report_generator(): @pytest.fixture -def make_report(tmp_path, excel_report_generator): +def mock_ods_report(mocker): + report = mocker.Mock(name="OdsReport") + report.get_total_ingested_count.return_value = 2 + report.get_total_successful.return_value = 1 + report.get_total_in_review_count.return_value = 1 + report.get_total_in_review_percentage.return_value = "50%" + report.get_total_successful_percentage.return_value = "50%" + report.get_total_registered_elsewhere_count.return_value = 0 + report.get_total_suspended_count.return_value = 0 + report.get_total_deceased_count.return_value = 0 + report.get_total_restricted_count.return_value = 0 + report.get_unsuccessful_reasons_data_rows.return_value = [ + ["Reason", "Invalid NHS number", 1], + ] + return report + + +@pytest.fixture +def make_record(): + def _make( + *, + nhs_number=None, + date=None, + uploader_ods_code=None, + pds_ods_code=None, + upload_status=None, + reason=None, + sent_to_review=None, + file_path=None, + ): + return SimpleNamespace( + nhs_number=nhs_number, + date=date, + uploader_ods_code=uploader_ods_code, + pds_ods_code=pds_ods_code, + upload_status=upload_status, + reason=reason, + sent_to_review=sent_to_review, + file_path=file_path, + ) + + return _make + + +@pytest.fixture +def make_report(tmp_path, excel_report_generator, mock_ods_report, mocker): def _make(*, ods_code="Y12345", records=None, filename="report.xlsx"): records = records or [] output_file = tmp_path / filename + mocked_build = mocker.patch.object( + excel_report_generator, + "_build_ods_report", + return_value=mock_ods_report, + ) + result = excel_report_generator.create_report_orchestration_xlsx( ods_code=ods_code, records=records, @@ -25,8 +79,12 @@ def _make(*, ods_code="Y12345", records=None, filename="report.xlsx"): assert output_file.exists() wb = load_workbook(output_file) - ws = wb.active - return output_file, ws + detail_ws = wb["Daily Upload Report"] + summary_ws = wb["Summary"] + + mocked_build.assert_called_once() + + return output_file, detail_ws, summary_ws return _make @@ -40,37 +98,46 @@ def expected_header_row(): "PDS ODS", "Upload Status", "Reason", + "Sent to Review", "File Path", ] @freeze_time("2025-01-01T12:00:00") -def test_create_report_orchestration_xlsx_happy_path(make_report, expected_header_row): +def test_create_report_orchestration_xlsx_happy_path( + make_report, + make_record, + expected_header_row, +): ods_code = "Y12345" records = [ - { - "ID": 1, - "Date": "2025-01-01", - "NhsNumber": "1234567890", - "UploaderOdsCode": "Y12345", - "PdsOdsCode": "A99999", - "UploadStatus": "SUCCESS", - "Reason": "", - "FilePath": "/path/file1.pdf", - }, - { - "ID": 2, - "Date": "2025-01-02", - "NhsNumber": "123456789", - "UploaderOdsCode": "Y12345", - "PdsOdsCode": "B88888", - "UploadStatus": "FAILED", - "Reason": "Invalid NHS number", - "FilePath": "/path/file2.pdf", - }, + make_record( + nhs_number="1234567890", + date="2025-01-01", + uploader_ods_code="Y12345", + pds_ods_code="A99999", + upload_status="SUCCESS", + reason=None, + sent_to_review=False, + file_path="/path/file1.pdf", + ), + make_record( + nhs_number="123456789", + date="2025-01-02", + uploader_ods_code="Y12345", + pds_ods_code="B88888", + upload_status="FAILED", + reason="Invalid NHS number", + sent_to_review=True, + file_path="/path/file2.pdf", + ), ] - _, ws = make_report(ods_code=ods_code, records=records, filename="report.xlsx") + _, ws, summary_ws = make_report( + ods_code=ods_code, + records=records, + filename="report.xlsx", + ) assert ws.title == "Daily Upload Report" assert ws["A1"].value == f"ODS Code: {ods_code}" @@ -86,6 +153,7 @@ def test_create_report_orchestration_xlsx_happy_path(make_report, expected_heade "A99999", "SUCCESS", None, + False, "/path/file1.pdf", ] @@ -96,34 +164,66 @@ def test_create_report_orchestration_xlsx_happy_path(make_report, expected_heade "B88888", "FAILED", "Invalid NHS number", + True, "/path/file2.pdf", ] + assert summary_ws.title == "Summary" + assert [cell.value for cell in summary_ws[1]] == ["Type", "Description", "Count"] + def test_create_report_orchestration_xlsx_with_no_records( make_report, expected_header_row, ): - _, ws = make_report(records=[], filename="empty_report.xlsx") + _, ws, summary_ws = make_report(records=[], filename="empty_report.xlsx") assert ws.max_row == 4 assert [cell.value for cell in ws[4]] == expected_header_row + assert [cell.value for cell in summary_ws[1]] == ["Type", "Description", "Count"] @pytest.mark.parametrize( "records, expected_row", [ ( - [{"ID": 1, "NhsNumber": "1234567890"}], - ["1234567890", None, None, None, None, None, None], + [SimpleNamespace( + nhs_number="1234567890", + date=None, + uploader_ods_code=None, + pds_ods_code=None, + upload_status=None, + reason=None, + sent_to_review=None, + file_path=None, + )], + ["1234567890", None, None, None, None, None, None, None], ), ( - [{"Date": "2025-01-01"}], - [None, "2025-01-01", None, None, None, None, None], + [SimpleNamespace( + nhs_number=None, + date="2025-01-01", + uploader_ods_code=None, + pds_ods_code=None, + upload_status=None, + reason=None, + sent_to_review=None, + file_path=None, + )], + [None, "2025-01-01", None, None, None, None, None, None], ), ( - [{"UploaderOdsCode": "Y12345", "UploadStatus": "SUCCESS"}], - [None, None, "Y12345", None, "SUCCESS", None, None], + [SimpleNamespace( + nhs_number=None, + date=None, + uploader_ods_code="Y12345", + pds_ods_code=None, + upload_status="SUCCESS", + reason=None, + sent_to_review=None, + file_path=None, + )], + [None, None, "Y12345", None, "SUCCESS", None, None, None], ), ], ) @@ -133,7 +233,7 @@ def test_create_report_orchestration_xlsx_handles_missing_fields( records, expected_row, ): - _, ws = make_report(records=records, filename="partial.xlsx") + _, ws, _summary_ws = make_report(records=records, filename="partial.xlsx") assert [cell.value for cell in ws[4]] == expected_header_row - assert [cell.value for cell in ws[5]] == expected_row + assert [cell.value for cell in ws[5]] == expected_row \ No newline at end of file diff --git a/lambdas/tests/unit/services/reporting/test_report_orchestration_service.py b/lambdas/tests/unit/services/reporting/test_report_orchestration_service.py index cabd3c45c..bf9c1898c 100644 --- a/lambdas/tests/unit/services/reporting/test_report_orchestration_service.py +++ b/lambdas/tests/unit/services/reporting/test_report_orchestration_service.py @@ -1,7 +1,19 @@ +import tempfile +from types import SimpleNamespace + import pytest +from pydantic import ValidationError + from services.reporting.report_orchestration_service import ReportOrchestrationService +def make_validated_record(uploader_ods_code, record_id): + return SimpleNamespace( + uploader_ods_code=uploader_ods_code, + id=str(record_id), + ) + + @pytest.fixture def mock_repository(mocker): repo = mocker.Mock(name="ReportingDynamoRepositoryInstance") @@ -40,56 +52,61 @@ def mocked_generate(report_orchestration_service, mocker): @pytest.mark.parametrize( - "records, expected_generate_calls, expected_result", + "raw_records, validated_records, expected_generate_calls, expected_result", [ ( + [], [], [], {}, ), ( - [{"UploaderOdsCode": "X1", "ID": 1}], + [{"UploaderOdsCode": "X1", "ID": "1"}], + [make_validated_record("X1", "1")], [ - ("X1", [{"UploaderOdsCode": "X1", "ID": 1}]), + ("X1", [make_validated_record("X1", "1")]), ], {"X1": "/tmp/X1.xlsx"}, ), ( [ - {"UploaderOdsCode": "Y12345", "ID": 1}, - {"UploaderOdsCode": "Y12345", "ID": 2}, - {"UploaderOdsCode": "A99999", "ID": 3}, + {"UploaderOdsCode": "Y12345", "ID": "1"}, + {"UploaderOdsCode": "Y12345", "ID": "2"}, + {"UploaderOdsCode": "A99999", "ID": "3"}, + ], + [ + make_validated_record("Y12345", "1"), + make_validated_record("Y12345", "2"), + make_validated_record("A99999", "3"), ], [ ( "Y12345", [ - {"UploaderOdsCode": "Y12345", "ID": 1}, - {"UploaderOdsCode": "Y12345", "ID": 2}, + make_validated_record("Y12345", "1"), + make_validated_record("Y12345", "2"), ], ), - ("A99999", [{"UploaderOdsCode": "A99999", "ID": 3}]), + ("A99999", [make_validated_record("A99999", "3")]), ], {"Y12345": "/tmp/Y12345.xlsx", "A99999": "/tmp/A99999.xlsx"}, ), ( [ - {"UploaderOdsCode": "Y12345", "ID": 1}, - {"ID": 2}, - {"UploaderOdsCode": None, "ID": 3}, + {"UploaderOdsCode": "Y12345", "ID": "1"}, + {"UploaderOdsCode": None, "ID": "2"}, + {"UploaderOdsCode": "", "ID": "3"}, ], [ - ("Y12345", [{"UploaderOdsCode": "Y12345", "ID": 1}]), - ("UNKNOWN", [{"ID": 2}, {"UploaderOdsCode": None, "ID": 3}]), + make_validated_record("Y12345", "1"), + make_validated_record(None, "2"), + make_validated_record("", "3"), ], - {"Y12345": "/tmp/Y12345.xlsx", "UNKNOWN": "/tmp/UNKNOWN.xlsx"}, - ), - ( - [{"UploaderOdsCode": "", "ID": 1}], [ - ("UNKNOWN", [{"UploaderOdsCode": "", "ID": 1}]), + ("Y12345", [make_validated_record("Y12345", "1")]), + ("UNKNOWN", [make_validated_record(None, "2"), make_validated_record("", "3")]), ], - {"UNKNOWN": "/tmp/UNKNOWN.xlsx"}, + {"Y12345": "/tmp/Y12345.xlsx", "UNKNOWN": "/tmp/UNKNOWN.xlsx"}, ), ], ) @@ -97,11 +114,19 @@ def test_process_reporting_window_behaviour( report_orchestration_service, mock_repository, mocked_generate, - records, + mocker, + raw_records, + validated_records, expected_generate_calls, expected_result, ): - mock_repository.get_records_for_time_window.return_value = records + mock_repository.get_records_for_time_window.return_value = raw_records + + validated_iter = iter(validated_records) + mocker.patch( + "services.reporting.report_orchestration_service.BulkUploadReport.model_validate", + side_effect=lambda _record: next(validated_iter), + ) result = report_orchestration_service.process_reporting_window(100, 200) @@ -114,35 +139,96 @@ def test_process_reporting_window_behaviour( assert result == expected_result +def test_process_reporting_window_skips_invalid_records( + report_orchestration_service, + mock_repository, + mocked_generate, + mocker, +): + raw_records = [ + {"UploaderOdsCode": "Y12345", "ID": "1"}, + {"UploaderOdsCode": "A99999", "ID": "2"}, + ] + mock_repository.get_records_for_time_window.return_value = raw_records + + valid_record = make_validated_record("Y12345", "1") + + mocker.patch( + "services.reporting.report_orchestration_service.BulkUploadReport.model_validate", + side_effect=[ + valid_record, + ValidationError.from_exception_data( + "BulkUploadReport", + [], + ), + ], + ) + + result = report_orchestration_service.process_reporting_window(100, 200) + + mocked_generate.assert_called_once_with("Y12345", [valid_record]) + assert result == {"Y12345": "/tmp/Y12345.xlsx"} + + +def test_process_reporting_window_returns_empty_when_all_records_invalid( + report_orchestration_service, + mock_repository, + mocked_generate, + mocker, +): + raw_records = [ + {"UploaderOdsCode": "Y12345", "ID": "1"}, + {"UploaderOdsCode": "A99999", "ID": "2"}, + ] + mock_repository.get_records_for_time_window.return_value = raw_records + + mocker.patch( + "services.reporting.report_orchestration_service.BulkUploadReport.model_validate", + side_effect=[ + ValidationError.from_exception_data("BulkUploadReport", []), + ValidationError.from_exception_data("BulkUploadReport", []), + ], + ) + + result = report_orchestration_service.process_reporting_window(100, 200) + + mocked_generate.assert_not_called() + assert result == {} + + @pytest.mark.parametrize( "records, expected", [ ( [ - {"UploaderOdsCode": "Y12345", "ID": 1}, - {"UploaderOdsCode": "Y12345", "ID": 2}, - {"UploaderOdsCode": "A99999", "ID": 3}, - {"ID": 4}, - {"UploaderOdsCode": None, "ID": 5}, + make_validated_record("Y12345", "1"), + make_validated_record("Y12345", "2"), + make_validated_record("A99999", "3"), + make_validated_record(None, "4"), + make_validated_record("", "5"), ], { "Y12345": [ - {"UploaderOdsCode": "Y12345", "ID": 1}, - {"UploaderOdsCode": "Y12345", "ID": 2}, + make_validated_record("Y12345", "1"), + make_validated_record("Y12345", "2"), + ], + "A99999": [make_validated_record("A99999", "3")], + "UNKNOWN": [ + make_validated_record(None, "4"), + make_validated_record("", "5"), ], - "A99999": [{"UploaderOdsCode": "A99999", "ID": 3}], - "UNKNOWN": [{"ID": 4}, {"UploaderOdsCode": None, "ID": 5}], }, ), ([], {}), ( - [{"UploaderOdsCode": "", "ID": 1}], - {"UNKNOWN": [{"UploaderOdsCode": "", "ID": 1}]}, + [make_validated_record("", "1")], + {"UNKNOWN": [make_validated_record("", "1")]}, ), ], ) def test_group_records_by_ods(records, expected): result = ReportOrchestrationService.group_records_by_ods(records) + assert dict(result) == expected @@ -166,7 +252,7 @@ def test_generate_ods_report_creates_excel_report_and_returns_path( fake_named_tmpfile, ): mocked_ntf, fake_tmp = fake_named_tmpfile - records = [{"ID": 1, "UploaderOdsCode": "Y12345"}] + records = [make_validated_record("Y12345", "1")] result_path = report_orchestration_service.generate_ods_report("Y12345", records) @@ -204,4 +290,4 @@ def test_init_constructs_repository_and_excel_generator(mocker): mocked_repo_cls.assert_called_once_with() mocked_excel_cls.assert_called_once_with() assert svc.repository is mock_repo - assert svc.excel_generator is mock_excel + assert svc.excel_generator is mock_excel \ No newline at end of file From 779f7293964ffe3fc87cc57d6d0b5ea86a48cf91 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Fri, 27 Mar 2026 11:04:46 +0000 Subject: [PATCH 7/9] [PRMP-1635] linting --- .../excel_report_generator_service.py | 8 ++- .../reporting/report_orchestration_service.py | 2 +- .../test_excel_report_generator_service.py | 68 ++++++++++--------- .../test_report_orchestration_service.py | 8 ++- 4 files changed, 49 insertions(+), 37 deletions(-) diff --git a/lambdas/services/reporting/excel_report_generator_service.py b/lambdas/services/reporting/excel_report_generator_service.py index afab84519..d02449f48 100644 --- a/lambdas/services/reporting/excel_report_generator_service.py +++ b/lambdas/services/reporting/excel_report_generator_service.py @@ -164,7 +164,11 @@ def _append_summary_optional_totals( ) -> None: if ods_report.get_total_deceased_count(): worksheet.append( - ["Total", "Successful - Deceased", ods_report.get_total_deceased_count()], + [ + "Total", + "Successful - Deceased", + ods_report.get_total_deceased_count(), + ], ) if ods_report.get_total_restricted_count(): @@ -182,4 +186,4 @@ def _append_summary_reason_rows( ods_report: OdsReport, ) -> None: for row in ods_report.get_unsuccessful_reasons_data_rows(): - worksheet.append(row) \ No newline at end of file + worksheet.append(row) diff --git a/lambdas/services/reporting/report_orchestration_service.py b/lambdas/services/reporting/report_orchestration_service.py index 4d05babf0..99e190d18 100644 --- a/lambdas/services/reporting/report_orchestration_service.py +++ b/lambdas/services/reporting/report_orchestration_service.py @@ -79,4 +79,4 @@ def generate_ods_report( records=records, output_path=tmp.name, ) - return tmp.name \ No newline at end of file + return tmp.name diff --git a/lambdas/tests/unit/services/reporting/test_excel_report_generator_service.py b/lambdas/tests/unit/services/reporting/test_excel_report_generator_service.py index 7f2f4fe1e..6b932dc1e 100644 --- a/lambdas/tests/unit/services/reporting/test_excel_report_generator_service.py +++ b/lambdas/tests/unit/services/reporting/test_excel_report_generator_service.py @@ -187,42 +187,48 @@ def test_create_report_orchestration_xlsx_with_no_records( "records, expected_row", [ ( - [SimpleNamespace( - nhs_number="1234567890", - date=None, - uploader_ods_code=None, - pds_ods_code=None, - upload_status=None, - reason=None, - sent_to_review=None, - file_path=None, - )], + [ + SimpleNamespace( + nhs_number="1234567890", + date=None, + uploader_ods_code=None, + pds_ods_code=None, + upload_status=None, + reason=None, + sent_to_review=None, + file_path=None, + ), + ], ["1234567890", None, None, None, None, None, None, None], ), ( - [SimpleNamespace( - nhs_number=None, - date="2025-01-01", - uploader_ods_code=None, - pds_ods_code=None, - upload_status=None, - reason=None, - sent_to_review=None, - file_path=None, - )], + [ + SimpleNamespace( + nhs_number=None, + date="2025-01-01", + uploader_ods_code=None, + pds_ods_code=None, + upload_status=None, + reason=None, + sent_to_review=None, + file_path=None, + ), + ], [None, "2025-01-01", None, None, None, None, None, None], ), ( - [SimpleNamespace( - nhs_number=None, - date=None, - uploader_ods_code="Y12345", - pds_ods_code=None, - upload_status="SUCCESS", - reason=None, - sent_to_review=None, - file_path=None, - )], + [ + SimpleNamespace( + nhs_number=None, + date=None, + uploader_ods_code="Y12345", + pds_ods_code=None, + upload_status="SUCCESS", + reason=None, + sent_to_review=None, + file_path=None, + ), + ], [None, None, "Y12345", None, "SUCCESS", None, None, None], ), ], @@ -236,4 +242,4 @@ def test_create_report_orchestration_xlsx_handles_missing_fields( _, ws, _summary_ws = make_report(records=records, filename="partial.xlsx") assert [cell.value for cell in ws[4]] == expected_header_row - assert [cell.value for cell in ws[5]] == expected_row \ No newline at end of file + assert [cell.value for cell in ws[5]] == expected_row diff --git a/lambdas/tests/unit/services/reporting/test_report_orchestration_service.py b/lambdas/tests/unit/services/reporting/test_report_orchestration_service.py index bf9c1898c..1028f4534 100644 --- a/lambdas/tests/unit/services/reporting/test_report_orchestration_service.py +++ b/lambdas/tests/unit/services/reporting/test_report_orchestration_service.py @@ -1,4 +1,3 @@ -import tempfile from types import SimpleNamespace import pytest @@ -104,7 +103,10 @@ def mocked_generate(report_orchestration_service, mocker): ], [ ("Y12345", [make_validated_record("Y12345", "1")]), - ("UNKNOWN", [make_validated_record(None, "2"), make_validated_record("", "3")]), + ( + "UNKNOWN", + [make_validated_record(None, "2"), make_validated_record("", "3")], + ), ], {"Y12345": "/tmp/Y12345.xlsx", "UNKNOWN": "/tmp/UNKNOWN.xlsx"}, ), @@ -290,4 +292,4 @@ def test_init_constructs_repository_and_excel_generator(mocker): mocked_repo_cls.assert_called_once_with() mocked_excel_cls.assert_called_once_with() assert svc.repository is mock_repo - assert svc.excel_generator is mock_excel \ No newline at end of file + assert svc.excel_generator is mock_excel From 6222a2790718574c552ac944a836a4df4b1ec44e Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Mon, 30 Mar 2026 13:08:12 +0100 Subject: [PATCH 8/9] [PRMP-1635] lint --- .../services/reporting/report_orchestration_service.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lambdas/services/reporting/report_orchestration_service.py b/lambdas/services/reporting/report_orchestration_service.py index b284a6ccc..b1a9a0d04 100644 --- a/lambdas/services/reporting/report_orchestration_service.py +++ b/lambdas/services/reporting/report_orchestration_service.py @@ -18,9 +18,9 @@ def __init__(self): self.excel_generator = ExcelReportGenerator() def process_reporting_window( - self, - window_start_ts: int, - window_end_ts: int, + self, + window_start_ts: int, + window_end_ts: int, ) -> Dict[str, str]: raw_records = self.repository.get_records_for_time_window( window_start_ts, @@ -38,7 +38,9 @@ def process_reporting_window( ] if not filtered_records: - logger.info("No records found for reporting window after excluding expedite files") + logger.info( + "No records found for reporting window after excluding expedite files", + ) return {} records: List[BulkUploadReport] = [] From be8b56a2f89e12308398f8c0adb9fa27fdfca887 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Mon, 30 Mar 2026 15:24:55 +0100 Subject: [PATCH 9/9] [PRMP-1635] fixed comments --- lambdas/services/reporting/report_orchestration_service.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lambdas/services/reporting/report_orchestration_service.py b/lambdas/services/reporting/report_orchestration_service.py index b1a9a0d04..7d9065c83 100644 --- a/lambdas/services/reporting/report_orchestration_service.py +++ b/lambdas/services/reporting/report_orchestration_service.py @@ -22,18 +22,18 @@ def process_reporting_window( window_start_ts: int, window_end_ts: int, ) -> Dict[str, str]: - raw_records = self.repository.get_records_for_time_window( + filtered_records = self.repository.get_records_for_time_window( window_start_ts, window_end_ts, ) - if not raw_records: + if not filtered_records: logger.info("No records found for reporting window") return {} filtered_records = [ record - for record in raw_records + for record in filtered_records if "expedite" not in (record.get("FilePath") or "").lower() ]