From 059f37d0a76098f046ddff8d7474219b5bff364e Mon Sep 17 00:00:00 2001 From: Steve Laing Date: Mon, 17 Nov 2025 10:10:38 +0000 Subject: [PATCH 1/4] Use positional args to simplify config Refactor how we determine how to send email. --- .../management/commands/create_reports.py | 38 +++++-------------- .../commands/test_create_reports.py | 14 +------ 2 files changed, 12 insertions(+), 40 deletions(-) diff --git a/manage_breast_screening/notifications/management/commands/create_reports.py b/manage_breast_screening/notifications/management/commands/create_reports.py index 1819655ee..bf96cd37c 100644 --- a/manage_breast_screening/notifications/management/commands/create_reports.py +++ b/manage_breast_screening/notifications/management/commands/create_reports.py @@ -25,24 +25,19 @@ def __init__( self, query_filename: str, params: list, + send_email: bool = False, report_filename: str | None = None, - should_send_email: bool = False, ): self.query_filename = query_filename self.params = params + self.send_email = send_email self.report_filename = report_filename or query_filename - self.should_send_email = should_send_email class Command(BaseCommand): """ Django Admin command which generates and stores CSV report data based on - common reporting queries: - 'aggregate' covers all notifications sent, failures and deliveries counts - grouped by appointment date, clinic code and bso code. This report covers - a 3 month time period and can be resource intensive. - 'failures' covers all failed status updates from NHS Notify and contains - NHS numbers, Clinic and BSO code and failure dates and reasons for one day. + common reporting queries. Reports are generated sequentially. Reports are stored in Azure Blob storage. """ @@ -51,24 +46,11 @@ class Command(BaseCommand): BSO_CODES = ["MBD"] REPORTS = [ + ReportConfig("aggregate", ["3 months"], True), ReportConfig( - query_filename="aggregate", - params=["3 months"], - report_filename="aggregate", - should_send_email=True, - ), - ReportConfig( - query_filename="failures", - params=[datetime.now(tz=ZONE_INFO).date()], - report_filename="invites_not_sent", - should_send_email=True, - ), - ReportConfig( - query_filename="reconciliation", - params=[datetime.now(tz=ZONE_INFO).date()], - report_filename="reconciliation", - should_send_email=True, + "failures", [datetime.now(tz=ZONE_INFO).date()], True, "invites_not_sent" ), + ReportConfig("reconciliation", [datetime.now(tz=ZONE_INFO).date()], True), ] def add_arguments(self, parser): @@ -96,10 +78,7 @@ def handle(self, *args, **options): content_type="text/csv", container_name=os.getenv("REPORTS_CONTAINER_NAME"), ) - if ( - not self.is_smoke_test(options) - and report_config.should_send_email - ): + if self.should_send_email(options, report_config): NhsMail().send_report_email( attachment_data=csv, attachment_filename=self.filename( @@ -129,3 +108,6 @@ def filename(self, bso_code: str, report_type: str) -> str: def is_smoke_test(self, options): return options.get("smoke_test", False) + + def should_send_email(self, options, report_config) -> bool: + return report_config.send_email and not self.is_smoke_test(options) diff --git a/manage_breast_screening/notifications/tests/management/commands/test_create_reports.py b/manage_breast_screening/notifications/tests/management/commands/test_create_reports.py index 8ef191283..f5fa3c3fa 100644 --- a/manage_breast_screening/notifications/tests/management/commands/test_create_reports.py +++ b/manage_breast_screening/notifications/tests/management/commands/test_create_reports.py @@ -168,18 +168,8 @@ def test_handle_does_not_email_reports_with_should_email_false( Test that reports with should_email=False are not emailed but still stored. """ test_reports = [ - ReportConfig( - query_filename="external_report", - params=[now.date()], - report_filename="external_report", - should_send_email=True, - ), - ReportConfig( - query_filename="internal_report", - params=[now.date()], - report_filename="internal_report", - should_send_email=False, - ), + ReportConfig("external_report", [now.date()], True), + ReportConfig("internal_report", [now.date()], False), ] monkeypatch.setattr(Command, "REPORTS", test_reports) From 328e0a0585c28db92f304304647468456025203f Mon Sep 17 00:00:00 2001 From: Steve Laing Date: Mon, 17 Nov 2025 11:52:43 +0000 Subject: [PATCH 2/4] Amend service to send all reports in one email --- .../notifications/services/nhs_mail.py | 35 ++++----- .../report_emails/invites_not_sent.html | 12 ---- .../report_emails/reconciliation.html | 11 --- .../{aggregate.html => reports.html} | 2 +- .../tests/services/test_nhs_mail.py | 71 ++++++------------- 5 files changed, 36 insertions(+), 95 deletions(-) delete mode 100644 manage_breast_screening/notifications/templates/report_emails/invites_not_sent.html delete mode 100644 manage_breast_screening/notifications/templates/report_emails/reconciliation.html rename manage_breast_screening/notifications/templates/report_emails/{aggregate.html => reports.html} (70%) diff --git a/manage_breast_screening/notifications/services/nhs_mail.py b/manage_breast_screening/notifications/services/nhs_mail.py index 3c87a1e19..d1e64b154 100644 --- a/manage_breast_screening/notifications/services/nhs_mail.py +++ b/manage_breast_screening/notifications/services/nhs_mail.py @@ -24,18 +24,14 @@ def __init__(self) -> None: self._sender_email = os.getenv("NOTIFICATIONS_SMTP_USERNAME", "") self._sender_password = os.getenv("NOTIFICATIONS_SMTP_PASSWORD", "") - def send_report_email( + def send_reports_email( self, - attachment_data: str, - attachment_filename: str, - report_type="invites_not_sent", + attachments_data: dict[str, str], ): - email = self._get_email_content( - attachment_data, attachment_filename, report_type - ) + email = self._get_email_content(attachments_data) logger.info( - f"Email for report type {report_type} created with attachment {attachment_filename}" + f"Email for reports {', '.join(attachments_data.keys())} created and sent" ) if boolean_env("NOTIFICATIONS_SMTP_IS_ENABLED", False): @@ -66,31 +62,30 @@ def _send_via_smtp(self, email): else: logger.info("Email sent") - def _get_email_content(self, attachment_data, attachment_filename, report_type): + def _get_email_content(self, attachments): todays_date = datetime.today().strftime("%d-%m-%Y") message = MIMEMultipart() - message.attach(MIMEText(self._body(report_type), "html")) + message.attach(MIMEText(self._body(), "html")) - message["Subject"] = self._subject(report_type, todays_date) + message["Subject"] = self._subject(todays_date) message["From"] = self._sender_email message["To"] = ",".join(self._recipient_emails) - attachment = MIMEApplication(attachment_data, Name=attachment_filename) - attachment["Content-Disposition"] = ( - f'attachment; filename="{attachment_filename}"' - ) - message.attach(attachment) + for filename, data in attachments.items(): + attachment = MIMEApplication(data, Name=filename) + attachment["Content-Disposition"] = f'attachment; filename="{filename}"' + message.attach(attachment) return message - def _subject(self, report_type, date, bso="Birmingham (MCR)") -> str: - default_subject = f"Breast screening digital comms {report_type.replace('_', ' ')} report – {date} – {bso}" + def _subject(self, date, bso="Birmingham (MCR)") -> str: + default_subject = f"Breast screening digital comms reports – {date} – {bso}" environment = os.getenv("DJANGO_ENV", "local") if environment != "prod": return f"[{environment.upper()}] {default_subject}" else: return default_subject - def _body(self, report_type) -> str: - return render_to_string(f"report_emails/{report_type}.html") + def _body(self) -> str: + return render_to_string("report_emails/reports.html") diff --git a/manage_breast_screening/notifications/templates/report_emails/invites_not_sent.html b/manage_breast_screening/notifications/templates/report_emails/invites_not_sent.html deleted file mode 100644 index b1c0a3b9a..000000000 --- a/manage_breast_screening/notifications/templates/report_emails/invites_not_sent.html +++ /dev/null @@ -1,12 +0,0 @@ - - -

Hello.

-

- It's important that you take action when you receive this email.This is a - list of clients who we haven't been able to invite to their breast - screening appointment. -

-

Please contact them using your standard operating procedure

-

Thank you.

- - diff --git a/manage_breast_screening/notifications/templates/report_emails/reconciliation.html b/manage_breast_screening/notifications/templates/report_emails/reconciliation.html deleted file mode 100644 index a86912a86..000000000 --- a/manage_breast_screening/notifications/templates/report_emails/reconciliation.html +++ /dev/null @@ -1,11 +0,0 @@ - - -

Hello.

-

- Attached to the email is the daily report for breast screening digital - comms. For more information on the data in the report, please check the - guidance here. -

-

Thank you.

- - diff --git a/manage_breast_screening/notifications/templates/report_emails/aggregate.html b/manage_breast_screening/notifications/templates/report_emails/reports.html similarity index 70% rename from manage_breast_screening/notifications/templates/report_emails/aggregate.html rename to manage_breast_screening/notifications/templates/report_emails/reports.html index a86912a86..64c0a4c08 100644 --- a/manage_breast_screening/notifications/templates/report_emails/aggregate.html +++ b/manage_breast_screening/notifications/templates/report_emails/reports.html @@ -2,7 +2,7 @@

Hello.

- Attached to the email is the daily report for breast screening digital + Attached to the email are the daily reports for breast screening digital comms. For more information on the data in the report, please check the guidance here.

diff --git a/manage_breast_screening/notifications/tests/services/test_nhs_mail.py b/manage_breast_screening/notifications/tests/services/test_nhs_mail.py index 8e560391a..aef0d0c0c 100644 --- a/manage_breast_screening/notifications/tests/services/test_nhs_mail.py +++ b/manage_breast_screening/notifications/tests/services/test_nhs_mail.py @@ -38,16 +38,18 @@ def test_sends_to_multiple_emails(self, mock_smtp_server, csv_data, monkeypatch) "NOTIFICATIONS_SMTP_RECIPIENTS", "recipient@nhsmail.net,another@nhs.net" ) subject = NhsMail() - subject.send_report_email(csv_data, "filename.csv", "invites_not_sent") + subject.send_reports_email({"filename.csv": csv_data}) mock_smtp_server.login.assert_called_once_with("sender@nhsmail.net", "password") mock_smtp_server.sendmail.assert_called_once_with( "sender@nhsmail.net", ["recipient@nhsmail.net", "another@nhs.net"], ANY ) - def test_sends_a_failure_report_email(self, mock_smtp_server, csv_data): + def test_sends_a_reports_email(self, mock_smtp_server, csv_data): subject = NhsMail() - subject.send_report_email(csv_data, "filename.csv", "invites_not_sent") + subject.send_reports_email( + {"filename1.csv": csv_data, "filename2.csv": csv_data} + ) mock_smtp_server.login.assert_called_once_with("sender@nhsmail.net", "password") mock_smtp_server.sendmail.assert_called_once_with( @@ -59,62 +61,29 @@ def test_sends_a_failure_report_email(self, mock_smtp_server, csv_data): decoded_subject_line = str(make_header(decode_header(mime_message["Subject"]))) assert ( - "[TEST] Breast screening digital comms invites not sent report – 11-10-2025 – Birmingham (MCR)" + "[TEST] Breast screening digital comms reports – 11-10-2025 – Birmingham (MCR)" in decoded_subject_line ) assert "Content-Type: text/html" in email_content - assert ( - "It's important that you take action when you receive this email" - in email_content - ) - assert "filename.csv" in email_content - decoded_attachment_data = ( - mime_message.get_payload()[1] # type: ignore - .get_payload( # type: ignore - None, True + assert "filename1.csv" in email_content + assert "filename2.csv" in email_content + for index in [1, 2]: + decoded_attachment_data = ( + mime_message.get_payload()[index] # type: ignore + .get_payload( # type: ignore + None, True + ) + .decode("utf-8") # type: ignore ) - .decode("utf-8") # type: ignore - ) - assert csv_data in decoded_attachment_data - - def test_sends_an_aggregate_report_email(self, mock_smtp_server, csv_data): - subject = NhsMail() - subject.send_report_email(csv_data, "filename.csv", "aggregate") - - mock_smtp_server.login.assert_called_once_with("sender@nhsmail.net", "password") - mock_smtp_server.sendmail.assert_called_once_with( - "sender@nhsmail.net", ["recipient@nhsmail.net"], ANY - ) - email_content = mock_smtp_server.sendmail.call_args[0][2] - mime_message = email.message_from_string(email_content) - - decoded_subject_line = str(make_header(decode_header(mime_message["Subject"]))) - - assert ( - "[TEST] Breast screening digital comms aggregate report – 11-10-2025 – Birmingham (MCR)" - in decoded_subject_line - ) - assert ( - "Attached to the email is the daily report for breast screening digital" - in email_content - ) - assert "filename.csv" in email_content - decoded_attachment_data = ( - mime_message.get_payload()[1] # type: ignore - .get_payload( # type: ignore - None, True - ) - .decode("utf-8") # type: ignore - ) - assert csv_data in decoded_attachment_data + assert csv_data in decoded_attachment_data def test_raises_exception_if_email_fails(self, mock_smtp_server, csv_data): mock_smtp_server.sendmail.side_effect = smtplib.SMTPException with pytest.raises(smtplib.SMTPException): subject = NhsMail() - subject.send_report_email(csv_data, "filename.csv") + subject.send_reports_email({"filename.csv": csv_data}) def test_does_not_send_email_if_disabled( self, mock_smtp_server, csv_data, monkeypatch @@ -122,7 +91,7 @@ def test_does_not_send_email_if_disabled( monkeypatch.setenv("NOTIFICATIONS_SMTP_IS_ENABLED", "False") subject = NhsMail() - subject.send_report_email(csv_data, "filename.csv", "invites_not_sent") + subject.send_reports_email({"filename.csv": csv_data}) mock_smtp_server.ehlo.assert_not_called() mock_smtp_server.starttls.assert_not_called() @@ -135,13 +104,13 @@ def test_does_not_add_environment_name_in_production( monkeypatch.setenv("DJANGO_ENV", "prod") subject = NhsMail() - subject.send_report_email(csv_data, "filename.csv", "invites_not_sent") + subject.send_reports_email({"filename.csv": csv_data}) email_content = mock_smtp_server.sendmail.call_args[0][2] mime_message = email.message_from_string(email_content) decoded_subject_line = str(make_header(decode_header(mime_message["Subject"]))) assert ( - "Breast screening digital comms invites not sent report – 11-10-2025 – Birmingham (MCR)" + "Breast screening digital comms reports – 11-10-2025 – Birmingham (MCR)" in decoded_subject_line ) From f41ea86e138f78848d652cdd6cad579cdacedd05 Mon Sep 17 00:00:00 2001 From: Steve Laing Date: Mon, 17 Nov 2025 11:55:38 +0000 Subject: [PATCH 3/4] Amend create_reports command to send all reports in one email Only send one email containing all external reports. --- .../management/commands/create_reports.py | 60 ++++++++----------- .../commands/test_create_reports.py | 53 +++++++--------- 2 files changed, 49 insertions(+), 64 deletions(-) diff --git a/manage_breast_screening/notifications/management/commands/create_reports.py b/manage_breast_screening/notifications/management/commands/create_reports.py index bf96cd37c..73ea45f46 100644 --- a/manage_breast_screening/notifications/management/commands/create_reports.py +++ b/manage_breast_screening/notifications/management/commands/create_reports.py @@ -1,4 +1,3 @@ -import os from datetime import datetime from logging import getLogger @@ -23,15 +22,15 @@ class ReportConfig: def __init__( self, - query_filename: str, + name: str, params: list, send_email: bool = False, - report_filename: str | None = None, + query: str | None = None, ): - self.query_filename = query_filename + self.name = name self.params = params self.send_email = send_email - self.report_filename = report_filename or query_filename + self.query = query or name class Command(BaseCommand): @@ -42,16 +41,18 @@ class Command(BaseCommand): Reports are stored in Azure Blob storage. """ - SMOKE_TEST_BSO_CODE = "SM0K3" BSO_CODES = ["MBD"] - REPORTS = [ ReportConfig("aggregate", ["3 months"], True), ReportConfig( - "failures", [datetime.now(tz=ZONE_INFO).date()], True, "invites_not_sent" + "invites-not-sent", [datetime.now(tz=ZONE_INFO).date()], True, "failures" ), ReportConfig("reconciliation", [datetime.now(tz=ZONE_INFO).date()], True), ] + SMOKE_TEST_BSO_CODE = "SM0K3" + SMOKE_TEST_CONFIG = ReportConfig( + "reconciliation", [datetime.now(tz=ZONE_INFO).date()], False + ) def add_arguments(self, parser): parser.add_argument("--smoke-test", action="store_true") @@ -61,39 +62,36 @@ def handle(self, *args, **options): logger.info("Create Report Command started") bso_codes, report_configs = self.configuration(options) + external_reports = {} for bso_code in bso_codes: for report_config in report_configs: dataframe = pandas.read_sql( - Helper.sql(report_config.query_filename), + Helper.sql(report_config.query), connection, params=(report_config.params + [bso_code]), ) csv = dataframe.to_csv(index=False) + filename = self.filename(bso_code, report_config.name) - BlobStorage().add( - self.filename(bso_code, report_config.report_filename), - csv, - content_type="text/csv", - container_name=os.getenv("REPORTS_CONTAINER_NAME"), - ) - if self.should_send_email(options, report_config): - NhsMail().send_report_email( - attachment_data=csv, - attachment_filename=self.filename( - bso_code, report_config.report_filename - ), - report_type=report_config.report_filename, - ) + BlobStorage().add(filename, csv, content_type="text/csv") + + if report_config.send_email: + external_reports[filename] = csv - logger.info("Report %s created", report_config.report_filename) + logger.info("Report %s created", report_config.name) + + if not options.get("smoke_test", False): + NhsMail().send_reports_email(external_reports) + logger.info(f"Reports sent: {', '.join(external_reports.keys())}") + + logger.info("Create Report Command finished") def configuration(self, options: dict) -> tuple[list[str], list[ReportConfig]]: - if self.is_smoke_test(options): - reconciliation_report_config = self.REPORTS[2] + if options.get("smoke_test", False): bso_codes = [self.SMOKE_TEST_BSO_CODE] - report_configs = [reconciliation_report_config] + report_configs = [self.SMOKE_TEST_CONFIG] else: bso_codes = self.BSO_CODES report_configs = self.REPORTS @@ -101,13 +99,7 @@ def configuration(self, options: dict) -> tuple[list[str], list[ReportConfig]]: return bso_codes, report_configs def filename(self, bso_code: str, report_type: str) -> str: - name = f"{bso_code}-{report_type.replace('_', '-')}-report.csv" + name = f"{bso_code}-{report_type}-report.csv" if bso_code != self.SMOKE_TEST_BSO_CODE: name = f"{datetime.today().strftime('%Y-%m-%dT%H:%M:%S')}-{name}" return name - - def is_smoke_test(self, options): - return options.get("smoke_test", False) - - def should_send_email(self, options, report_config) -> bool: - return report_config.send_email and not self.is_smoke_test(options) diff --git a/manage_breast_screening/notifications/tests/management/commands/test_create_reports.py b/manage_breast_screening/notifications/tests/management/commands/test_create_reports.py index f5fa3c3fa..1f12399a6 100644 --- a/manage_breast_screening/notifications/tests/management/commands/test_create_reports.py +++ b/manage_breast_screening/notifications/tests/management/commands/test_create_reports.py @@ -62,7 +62,7 @@ def test_handle_creates_all_reports(self, dataframe, csv_data, now): assert mock_read_sql.call_count == 3 assert mock_blob_storage.add.call_count == 3 - assert mock_email_service.send_report_email.call_count == 3 + assert mock_email_service.send_reports_email.call_count == 1 for bso_code in Command.BSO_CODES: mock_read_sql.assert_any_call( @@ -85,35 +85,24 @@ def test_handle_creates_all_reports(self, dataframe, csv_data, now): aggregate_filename, csv_data, content_type="text/csv", - container_name="reports", ) mock_blob_storage.add.assert_any_call( failures_filename, csv_data, content_type="text/csv", - container_name="reports", ) mock_blob_storage.add.assert_any_call( reconciliation_filename, csv_data, content_type="text/csv", - container_name="reports", ) - mock_email_service.send_report_email.assert_any_call( - attachment_data=csv_data, - attachment_filename=aggregate_filename, - report_type="aggregate", - ) - mock_email_service.send_report_email.assert_any_call( - attachment_data=csv_data, - attachment_filename=failures_filename, - report_type="invites_not_sent", - ) - mock_email_service.send_report_email.assert_any_call( - attachment_data=csv_data, - attachment_filename=reconciliation_filename, - report_type="reconciliation", + mock_email_service.send_reports_email.assert_called_once_with( + { + aggregate_filename: csv_data, + failures_filename: csv_data, + reconciliation_filename: csv_data, + } ) def test_handle_raises_command_error(self, mock_insights_logger): @@ -157,19 +146,18 @@ def test_smoke_test_argument_uses_correct_configuration( "SM0K3-reconciliation-report.csv", csv_data, content_type="text/csv", - container_name="reports", ) mock_email_service.assert_not_called() - def test_handle_does_not_email_reports_with_should_email_false( + def test_handle_does_not_email_reports_with_send_email_false( self, dataframe, csv_data, now, monkeypatch ): """ - Test that reports with should_email=False are not emailed but still stored. + Test that internal reports are not emailed but still stored. """ test_reports = [ - ReportConfig("external_report", [now.date()], True), - ReportConfig("internal_report", [now.date()], False), + ReportConfig("external-report", [now.date()], True), + ReportConfig("internal-report", [now.date()], False), ] monkeypatch.setattr(Command, "REPORTS", test_reports) @@ -185,20 +173,25 @@ def test_handle_does_not_email_reports_with_should_email_false( assert mock_read_sql.call_count == 2 assert mock_blob_storage.add.call_count == 2 - assert mock_email_service.send_report_email.call_count == 1 + assert mock_email_service.send_reports_email.call_count == 1 internal_filename = ( f"{now.strftime('%Y-%m-%dT%H:%M:%S')}-MBD-internal-report-report.csv" ) + external_filename = ( + f"{now.strftime('%Y-%m-%dT%H:%M:%S')}-MBD-external-report-report.csv" + ) mock_blob_storage.add.assert_any_call( internal_filename, csv_data, content_type="text/csv", - container_name="reports", + ) + mock_blob_storage.add.assert_any_call( + external_filename, + csv_data, + content_type="text/csv", ) - email_calls = [ - call[1]["report_type"] - for call in mock_email_service.send_report_email.call_args_list - ] - assert "internal_report" not in email_calls + mock_email_service.send_reports_email.assert_called_once_with( + {external_filename: csv_data} + ) From 0a41866ddde9db875f5476e81897a1829e5bee75 Mon Sep 17 00:00:00 2001 From: Steve Laing Date: Tue, 18 Nov 2025 14:51:28 +0000 Subject: [PATCH 4/4] Check for presence of external reports as a basis for calling email service --- .../management/commands/create_reports.py | 2 +- .../commands/test_create_reports.py | 50 +++++++++++-------- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/manage_breast_screening/notifications/management/commands/create_reports.py b/manage_breast_screening/notifications/management/commands/create_reports.py index 73ea45f46..0c550005f 100644 --- a/manage_breast_screening/notifications/management/commands/create_reports.py +++ b/manage_breast_screening/notifications/management/commands/create_reports.py @@ -82,7 +82,7 @@ def handle(self, *args, **options): logger.info("Report %s created", report_config.name) - if not options.get("smoke_test", False): + if bool(external_reports): NhsMail().send_reports_email(external_reports) logger.info(f"Reports sent: {', '.join(external_reports.keys())}") diff --git a/manage_breast_screening/notifications/tests/management/commands/test_create_reports.py b/manage_breast_screening/notifications/tests/management/commands/test_create_reports.py index 1f12399a6..ac1f78e24 100644 --- a/manage_breast_screening/notifications/tests/management/commands/test_create_reports.py +++ b/manage_breast_screening/notifications/tests/management/commands/test_create_reports.py @@ -28,21 +28,17 @@ def now(self): @contextmanager def mocked_dependencies(self, dataframe, csv_data, now): - module = ( - "manage_breast_screening.notifications.management.commands.create_reports" - ) - - with patch(f"{module}.BlobStorage") as mock_storage: + with patch(f"{Command.__module__}.BlobStorage") as mock_storage: mock_blob_storage = MagicMock() mock_storage.return_value = mock_blob_storage - with patch(f"{module}.pandas.read_sql") as mock_read_sql: + with patch(f"{Command.__module__}.pandas.read_sql") as mock_read_sql: mock_read_sql.return_value = dataframe - with patch(f"{module}.datetime") as mock_datetime: + with patch(f"{Command.__module__}.datetime") as mock_datetime: mock_datetime.today.return_value = now - with patch(f"{module}.NhsMail") as mock_email: + with patch(f"{Command.__module__}.NhsMail") as mock_email: mock_email_service = MagicMock() mock_email.return_value = mock_email_service @@ -106,22 +102,16 @@ def test_handle_creates_all_reports(self, dataframe, csv_data, now): ) def test_handle_raises_command_error(self, mock_insights_logger): - with patch( - "manage_breast_screening.notifications.queries.helper.Helper" - ) as mock_query: + with patch(f"{Helper.__module__}.Helper") as mock_query: mock_query.sql.side_effect = Exception("err") with pytest.raises(CommandError): Command().handle() @pytest.mark.django_db - def test_calls_insights_logger_if_exception_raised( - self, mock_insights_logger, commands_module_str - ): + def test_calls_insights_logger_if_exception_raised(self, mock_insights_logger): an_exception = Exception("'BlobStorage' object has no attribute 'client'") - with patch( - f"{commands_module_str}.create_reports.BlobStorage" - ) as mock_blob_storage: + with patch(f"{Command.__module__}.BlobStorage") as mock_blob_storage: mock_blob_storage.side_effect = an_exception with pytest.raises(CommandError): Command().handle() @@ -161,9 +151,7 @@ def test_handle_does_not_email_reports_with_send_email_false( ] monkeypatch.setattr(Command, "REPORTS", test_reports) - with patch( - "manage_breast_screening.notifications.queries.helper.Helper.sql" - ) as mock_helper_sql: + with patch(f"{Helper.__module__}.Helper.sql") as mock_helper_sql: mock_helper_sql.return_value = "SELECT 1" with self.mocked_dependencies(dataframe, csv_data, now) as md: @@ -195,3 +183,25 @@ def test_handle_does_not_email_reports_with_send_email_false( mock_email_service.send_reports_email.assert_called_once_with( {external_filename: csv_data} ) + + def test_handle_with_internal_reports_does_not_call_email_service( + self, dataframe, csv_data, now, monkeypatch + ): + """ + Test that internal reports are not emailed but still stored. + """ + monkeypatch.setattr( + Command, "REPORTS", [ReportConfig("internal-report", [1], False)] + ) + + with patch(f"{Helper.__module__}.Helper.sql") as mock_helper_sql: + mock_helper_sql.return_value = "SELECT 1" + + with self.mocked_dependencies(dataframe, csv_data, now) as md: + Command().handle() + + mock_read_sql, mock_blob_storage, mock_email_service = md + + assert mock_read_sql.call_count == 1 + assert mock_blob_storage.add.call_count == 1 + assert mock_email_service.send_reports_email.call_count == 0