From 9698a82987f91718b64cee89b4be736eb358df5b Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Mon, 12 Jan 2026 15:21:45 +0000 Subject: [PATCH 1/8] [PRMP-1058] Report rejections handling --- .../handlers/report_distribution_handler.py | 6 +- .../handlers/ses_feedback_monitor_handler.py | 29 ++++ lambdas/services/email_service.py | 80 +++++++--- .../reporting/report_distribution_service.py | 45 +++++- .../services/ses_feedback_monitor_service.py | 149 ++++++++++++++++++ 5 files changed, 283 insertions(+), 26 deletions(-) create mode 100644 lambdas/handlers/ses_feedback_monitor_handler.py create mode 100644 lambdas/services/ses_feedback_monitor_service.py diff --git a/lambdas/handlers/report_distribution_handler.py b/lambdas/handlers/report_distribution_handler.py index 30c8547af..664a5028a 100644 --- a/lambdas/handlers/report_distribution_handler.py +++ b/lambdas/handlers/report_distribution_handler.py @@ -20,6 +20,7 @@ "CONTACT_TABLE_NAME", "PRM_MAILBOX_EMAIL", "SES_FROM_ADDRESS", + "SES_CONFIGURATION_SET", ] ) @override_error_check @@ -35,9 +36,12 @@ def lambda_handler(event, context) -> Dict[str, Any]: prm_mailbox = os.environ["PRM_MAILBOX_EMAIL"] from_address = os.environ["SES_FROM_ADDRESS"] + configuration_set = os.environ["SES_CONFIGURATION_SET"] + s3_service = S3Service() contact_repo = ReportContactRepository(contact_table) - email_service = EmailService() + + email_service = EmailService(default_configuration_set=configuration_set) service = ReportDistributionService( s3_service=s3_service, diff --git a/lambdas/handlers/ses_feedback_monitor_handler.py b/lambdas/handlers/ses_feedback_monitor_handler.py new file mode 100644 index 000000000..800f58012 --- /dev/null +++ b/lambdas/handlers/ses_feedback_monitor_handler.py @@ -0,0 +1,29 @@ +import os +import boto3 +from typing import Any, Dict + +from services.email_service import EmailService +from services.ses_feedback_monitor_service import SesFeedbackMonitorConfig, SesFeedbackMonitorService + + +def parse_alert_types(configured: str) -> set[str]: + return {s.strip().upper() for s in configured.split(",") if s.strip()} + + +def lambda_handler(event, context) -> Dict[str, Any]: + config = SesFeedbackMonitorConfig( + feedback_bucket=os.environ["SES_FEEDBACK_BUCKET_NAME"], + feedback_prefix=os.environ["SES_FEEDBACK_PREFIX"], + prm_mailbox=os.environ["PRM_MAILBOX_EMAIL"], + from_address=os.environ["SES_FROM_ADDRESS"], + alert_on_event_types=parse_alert_types( + os.environ["ALERT_ON_EVENT_TYPES"] + ), + ) + + service = SesFeedbackMonitorService( + s3_client=boto3.client("s3"), + email_service=EmailService(), + config=config, + ) + return service.process_ses_feedback_event(event) diff --git a/lambdas/services/email_service.py b/lambdas/services/email_service.py index 8a11a9bd8..6a9e9b083 100644 --- a/lambdas/services/email_service.py +++ b/lambdas/services/email_service.py @@ -2,7 +2,7 @@ from email.mime.multipart import MIMEMultipart from email.mime.text import MIMEText from email.mime.application import MIMEApplication -from typing import Iterable, Optional +from typing import Iterable, Optional, Dict, Any from utils.audit_logging_setup import LoggingService @@ -15,8 +15,9 @@ class EmailService: Higher-level methods prepare inputs and call send_email(). """ - def __init__(self): + def __init__(self, *, default_configuration_set: Optional[str] = None): self.ses = boto3.client("ses") + self.default_configuration_set = default_configuration_set def send_email( self, @@ -26,7 +27,14 @@ def send_email( body_text: str, from_address: str, attachments: Optional[Iterable[str]] = None, - )->MIMEMultipart: + configuration_set: Optional[str] = None, + tags: Optional[Dict[str, str]] = None, + ) -> Dict[str, Any]: + """ + Sends an email using SES SendRawEmail. + + If configuration_set is not provided, self.default_configuration_set is used (if set). + """ msg = MIMEMultipart() msg["Subject"] = subject msg["To"] = to_address @@ -34,7 +42,8 @@ def send_email( msg.attach(MIMEText(body_text, "plain")) - for attachment_path in attachments or []: + attachment_list = list(attachments or []) + for attachment_path in attachment_list: with open(attachment_path, "rb") as f: part = MIMEApplication(f.read()) part.add_header( @@ -43,22 +52,51 @@ def send_email( filename=attachment_path.split("/")[-1], ) msg.attach(part) + + effective_config_set = configuration_set or self.default_configuration_set + logger.info( f"Sending email: from={from_address!r}, to={to_address!r}, subject={subject!r}, " - f"attachments={len(list(attachments or []))}" + f"attachments={len(attachment_list)}, configuration_set={effective_config_set!r}, tags={tags!r}" + ) + + return self._send_raw( + msg=msg, + to_address=to_address, + configuration_set=effective_config_set, + tags=tags, ) - return self._send_raw(msg, to_address) - def _send_raw(self, msg: MIMEMultipart, to_address: str)->MIMEMultipart: + def _send_raw( + self, + *, + msg: MIMEMultipart, + to_address: str, + configuration_set: Optional[str] = None, + tags: Optional[Dict[str, str]] = None, + ) -> Dict[str, Any]: subject = msg.get("Subject", "") from_address = msg.get("From", "") - logger.info(f"Sending SES raw email: subject={subject!r}, from={from_address!r}, to={to_address!r}") - resp = self.ses.send_raw_email( - Source=from_address, - RawMessage={"Data": msg.as_string()}, - Destinations=[to_address], + + logger.info( + f"Sending SES raw email: subject={subject!r}, from={from_address!r}, to={to_address!r}, " + f"configuration_set={configuration_set!r}, tags={tags!r}" ) + kwargs: Dict[str, Any] = { + "Source": from_address, + "RawMessage": {"Data": msg.as_string()}, + "Destinations": [to_address], + } + + if configuration_set: + kwargs["ConfigurationSetName"] = configuration_set + + if tags: + kwargs["Tags"] = [{"Name": k, "Value": v} for k, v in tags.items()] + + resp = self.ses.send_raw_email(**kwargs) + logger.info(f"SES accepted email: subject={subject!r}, message_id={resp.get('MessageId')}") return resp @@ -68,13 +106,15 @@ def send_report_email( to_address: str, from_address: str, attachment_path: str, - ): - self.send_email( + tags: Optional[Dict[str, str]] = None, + ) -> Dict[str, Any]: + return self.send_email( to_address=to_address, from_address=from_address, subject="Daily Upload Report", body_text="Please find your encrypted daily upload report attached.", attachments=[attachment_path], + tags=tags, ) def send_password_email( @@ -83,12 +123,14 @@ def send_password_email( to_address: str, from_address: str, password: str, - ): - self.send_email( + tags: Optional[Dict[str, str]] = None, + ) -> Dict[str, Any]: + return self.send_email( to_address=to_address, from_address=from_address, subject="Daily Upload Report Password", body_text=f"Password for your report:\n\n{password}", + tags=tags, ) def send_prm_missing_contact_email( @@ -99,8 +141,9 @@ def send_prm_missing_contact_email( ods_code: str, attachment_path: str, password: str, - ): - self.send_email( + tags: Optional[Dict[str, str]] = None, + ) -> Dict[str, Any]: + return self.send_email( to_address=prm_mailbox, from_address=from_address, subject=f"Missing contact for ODS {ods_code}", @@ -110,4 +153,5 @@ def send_prm_missing_contact_email( f"Please resolve the contact and forward the report." ), attachments=[attachment_path], + tags=tags, ) diff --git a/lambdas/services/reporting/report_distribution_service.py b/lambdas/services/reporting/report_distribution_service.py index f64c632c6..e448fe1b7 100644 --- a/lambdas/services/reporting/report_distribution_service.py +++ b/lambdas/services/reporting/report_distribution_service.py @@ -1,7 +1,6 @@ -import os import secrets import tempfile -from typing import List +from typing import List, Dict import boto3 @@ -51,9 +50,11 @@ def list_xlsx_keys(self, prefix: str) -> List[str]: return keys def process_one_report(self, *, ods_code: str, key: str) -> None: + base_tags: Dict[str, str] = {"ods_code": ods_code, "report_key": key} + with tempfile.TemporaryDirectory() as tmpdir: - local_xlsx = os.path.join(tmpdir, f"{ods_code}.xlsx") - local_zip = os.path.join(tmpdir, f"{ods_code}.zip") + local_xlsx = f"{tmpdir}/{ods_code}.xlsx" + local_zip = f"{tmpdir}/{ods_code}.zip" self.s3_service.download_file(self.bucket, key, local_xlsx) @@ -68,9 +69,17 @@ def process_one_report(self, *, ods_code: str, key: str) -> None: ods_code=ods_code, attachment_path=local_zip, password=password, + base_tags=base_tags, ) - def send_report_emails(self, *, ods_code: str, attachment_path: str, password: str) -> None: + def send_report_emails( + self, + *, + ods_code: str, + attachment_path: str, + password: str, + base_tags: Dict[str, str], + ) -> None: try: contact_email = self.contact_repo.get_contact_email(ods_code) except Exception as e: @@ -85,6 +94,7 @@ def send_report_emails(self, *, ods_code: str, attachment_path: str, password: s to_address=contact_email, attachment_path=attachment_path, password=password, + base_tags=base_tags, ) return @@ -93,29 +103,50 @@ def send_report_emails(self, *, ods_code: str, attachment_path: str, password: s ods_code=ods_code, attachment_path=attachment_path, password=password, + base_tags=base_tags, ) - def email_contact(self, *, to_address: str, attachment_path: str, password: str) -> None: + def email_contact( + self, + *, + to_address: str, + attachment_path: str, + password: str, + base_tags: Dict[str, str], + ) -> None: + tags = {**base_tags, "email": to_address} + logger.info(f"Sending report email to {to_address}") self.email_service.send_report_email( to_address=to_address, from_address=self.from_address, attachment_path=attachment_path, + tags=tags, ) + logger.info(f"Sending password email to {to_address}") self.email_service.send_password_email( to_address=to_address, from_address=self.from_address, password=password, + tags=tags, ) def email_prm_missing_contact( - self, *, ods_code: str, attachment_path: str, password: str + self, + *, + ods_code: str, + attachment_path: str, + password: str, + base_tags: Dict[str, str], ) -> None: + tags = {**base_tags, "email": self.prm_mailbox} + self.email_service.send_prm_missing_contact_email( prm_mailbox=self.prm_mailbox, from_address=self.from_address, ods_code=ods_code, attachment_path=attachment_path, password=password, + tags=tags, ) diff --git a/lambdas/services/ses_feedback_monitor_service.py b/lambdas/services/ses_feedback_monitor_service.py new file mode 100644 index 000000000..5df4d7855 --- /dev/null +++ b/lambdas/services/ses_feedback_monitor_service.py @@ -0,0 +1,149 @@ +import json +from dataclasses import dataclass +from datetime import datetime, timezone +from typing import Any, Dict, List, Optional, Tuple, Protocol + +from utils.audit_logging_setup import LoggingService +from services.email_service import EmailService + +logger = LoggingService(__name__) + + +class S3Client(Protocol): + def put_object(self, **kwargs): # pragma: no cover (protocol) + ... + + +@dataclass(frozen=True) +class SesFeedbackMonitorConfig: + feedback_bucket: str + feedback_prefix: str + prm_mailbox: str + from_address: str + alert_on_event_types: set[str] # {"BOUNCE","REJECT"} + + +class SesFeedbackMonitorService: + def __init__(self, *, s3_client: S3Client, email_service: EmailService, config: SesFeedbackMonitorConfig): + self.s3 = s3_client + self.email_service = email_service + self.config = config + + @staticmethod + def parse_sns_message(record: Dict[str, Any]) -> Dict[str, Any]: + msg = record["Sns"]["Message"] + return json.loads(msg) if isinstance(msg, str) else msg + + @staticmethod + def event_type(payload: Dict[str, Any]) -> str: + return (payload.get("eventType") or payload.get("notificationType") or "UNKNOWN").upper() + + @staticmethod + def message_id(payload: Dict[str, Any]) -> str: + mail = payload.get("mail") or {} + return mail.get("messageId") or payload.get("mailMessageId") or "unknown-message-id" + + @staticmethod + def extract_tags(payload: Dict[str, Any]) -> Dict[str, List[str]]: + mail = payload.get("mail") or {} + tags = mail.get("tags") or {} + return tags if isinstance(tags, dict) else {} + + @staticmethod + def extract_recipients_and_diagnostic(payload: Dict[str, Any]) -> Tuple[List[str], Optional[str]]: + recipients: List[str] = [] + diagnostic: Optional[str] = None + + if "bounce" in payload: + b = payload.get("bounce") or {} + bounced = b.get("bouncedRecipients") or [] + for r in bounced: + email_addr = r.get("emailAddress") + if email_addr: + recipients.append(email_addr) + if bounced: + diagnostic = (bounced[0] or {}).get("diagnosticCode") + diagnostic = diagnostic or b.get("smtpResponse") + return recipients, diagnostic + + if "complaint" in payload: + c = payload.get("complaint") or {} + complained = c.get("complainedRecipients") or [] + for r in complained: + email_addr = r.get("emailAddress") + if email_addr: + recipients.append(email_addr) + return recipients, diagnostic + + if "reject" in payload: + r = payload.get("reject") or {} + diagnostic = r.get("reason") or r.get("message") + return recipients, diagnostic + + return recipients, diagnostic + + @staticmethod + def build_s3_key(prefix: str, event_type: str, message_id: str) -> str: + now = datetime.now(timezone.utc) + return f"{prefix.rstrip('/')}/{event_type}/{now:%Y/%m/%d}/{message_id}.json" + + def process_ses_feedback_event(self, event: Dict[str, Any]) -> Dict[str, Any]: + stored = 0 + alerted = 0 + + for record in event.get("Records") or []: + payload = self.parse_sns_message(record) + et = self.event_type(payload) + mid = self.message_id(payload) + + s3_key = self.build_s3_key(self.config.feedback_prefix, et, mid) + + self.s3.put_object( + Bucket=self.config.feedback_bucket, + Key=s3_key, + Body=json.dumps(payload).encode("utf-8"), + ContentType="application/json", + ) + stored += 1 + logger.info(f"Stored SES feedback event: type={et}, message_id={mid}, s3=s3://{self.config.feedback_bucket}/{s3_key}") + + if et in self.config.alert_on_event_types: + subject, body = self.build_prm_email(payload, s3_key) + self.email_service.send_email( + to_address=self.config.prm_mailbox, + from_address=self.config.from_address, + subject=subject, + body_text=body, + ) + alerted += 1 + logger.info(f"Emailed PRM for SES feedback event: type={et}, message_id={mid}") + + return {"status": "ok", "stored": stored, "alerted": alerted} + + def build_prm_email(self, payload: Dict[str, Any], s3_key: str) -> Tuple[str, str]: + et = self.event_type(payload) + mid = self.message_id(payload) + tags = self.extract_tags(payload) + recipients, diagnostic = self.extract_recipients_and_diagnostic(payload) + + ods_code = (tags.get("ods_code") or [None])[0] + report_key = (tags.get("report_key") or [None])[0] + email_tag = (tags.get("email") or [None])[0] + + subject = f"SES {et}: messageId={mid}" + body_lines = [ + f"Event type: {et}", + f"Message ID: {mid}", + f"Affected recipients: {', '.join(recipients) if recipients else '(none parsed)'}", + f"Diagnostic: {diagnostic or '(none parsed)'}", + "", + f"ODS code tag: {ods_code or '(none)'}", + f"Email tag: {email_tag or '(none)'}", + f"Report key tag: {report_key or '(none)'}", + "", + f"Stored at: s3://{self.config.feedback_bucket}/{s3_key}", + "", + "Raw event JSON:", + json.dumps(payload, indent=2, sort_keys=True), + ] + return subject, "\n".join(body_lines) From 583a5b58e59c2d3131c95f80c49e360dabe6550f Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Mon, 12 Jan 2026 15:22:52 +0000 Subject: [PATCH 2/8] [PRMP-1058] added lambda layer call --- .../workflows/base-lambdas-reusable-deploy-all.yml | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/.github/workflows/base-lambdas-reusable-deploy-all.yml b/.github/workflows/base-lambdas-reusable-deploy-all.yml index 5128113a4..4055ee7b9 100644 --- a/.github/workflows/base-lambdas-reusable-deploy-all.yml +++ b/.github/workflows/base-lambdas-reusable-deploy-all.yml @@ -824,3 +824,17 @@ jobs: lambda_layer_names: "core_lambda_layer,reports_lambda_layer" secrets: AWS_ASSUME_ROLE: ${{ secrets.AWS_ASSUME_ROLE }} + + deploy_ses_feedback_monitor_lambda: + name: Deploy SES Feedback Monitor + uses: ./.github/workflows/base-lambdas-reusable-deploy.yml + with: + environment: ${{ inputs.environment }} + python_version: ${{ inputs.python_version }} + build_branch: ${{ inputs.build_branch }} + sandbox: ${{ inputs.sandbox }} + lambda_handler_name: ses_feedback_monitor_handler + lambda_aws_name: sesFeedbackMonitor + lambda_layer_names: "core_lambda_layer,reports_lambda_layer" + secrets: + AWS_ASSUME_ROLE: ${{ secrets.AWS_ASSUME_ROLE }} \ No newline at end of file From aef729c475d444abd6ad80c1a99f3342b72bdf38 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Wed, 14 Jan 2026 15:47:53 +0000 Subject: [PATCH 3/8] [PRMP-1058] Sanitised tags --- .../services/reporting/report_distribution_service.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lambdas/services/reporting/report_distribution_service.py b/lambdas/services/reporting/report_distribution_service.py index e448fe1b7..1c86409b6 100644 --- a/lambdas/services/reporting/report_distribution_service.py +++ b/lambdas/services/reporting/report_distribution_service.py @@ -1,3 +1,4 @@ +import re import secrets import tempfile from typing import List, Dict @@ -12,6 +13,9 @@ logger = LoggingService(__name__) +_SES_TAG_VALUE_ALLOWED = re.compile(r"[^A-Za-z0-9_\-\.@]") +def _sanitize_ses_tag_value(value: str) -> str: + return _SES_TAG_VALUE_ALLOWED.sub("_", str(value)) class ReportDistributionService: def __init__( @@ -50,8 +54,10 @@ def list_xlsx_keys(self, prefix: str) -> List[str]: return keys def process_one_report(self, *, ods_code: str, key: str) -> None: - base_tags: Dict[str, str] = {"ods_code": ods_code, "report_key": key} - + base_tags: Dict[str, str] = { + "ods_code": _sanitize_ses_tag_value(ods_code), + "report_key": _sanitize_ses_tag_value(key), + } with tempfile.TemporaryDirectory() as tmpdir: local_xlsx = f"{tmpdir}/{ods_code}.xlsx" local_zip = f"{tmpdir}/{ods_code}.zip" From 462209c8de0268f9d012538bae94c64663208108 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Thu, 15 Jan 2026 09:44:36 +0000 Subject: [PATCH 4/8] [PRMP-1058] Sanitised tags --- .../handlers/report_orchestration_handler.py | 4 +- .../handlers/ses_feedback_monitor_handler.py | 11 +- .../reporting/reporting_dynamo_repository.py | 16 +- lambdas/services/email_service.py | 10 +- .../reporting/report_distribution_service.py | 6 +- .../services/ses_feedback_monitor_service.py | 34 +- .../test_report_distribution_handler.py | 56 +++- .../test_ses_feedback_monitor_handler.py | 164 ++++++++++ .../services/reporting/test_email_service.py | 125 +++++++- .../test_report_distribution_service.py | 147 +++++++-- .../test_ses_feedback_monitor_service.py | 293 ++++++++++++++++++ 11 files changed, 790 insertions(+), 76 deletions(-) create mode 100644 lambdas/tests/unit/handlers/test_ses_feedback_monitor_handler.py create mode 100644 lambdas/tests/unit/services/test_ses_feedback_monitor_service.py diff --git a/lambdas/handlers/report_orchestration_handler.py b/lambdas/handlers/report_orchestration_handler.py index be9ea7777..696ce2e03 100644 --- a/lambdas/handlers/report_orchestration_handler.py +++ b/lambdas/handlers/report_orchestration_handler.py @@ -89,7 +89,9 @@ def lambda_handler(event, context) -> Dict[str, Any]: keys.append(key) logger.info(f"Uploaded report for ODS={ods_code} to s3://{report_bucket}/{key}") - logger.info(f"Generated and uploaded {len(keys)} report(s) for report_date={report_date}") + logger.info( + f"Generated and uploaded {len(keys)} report(s) for report_date={report_date}" + ) return { "report_date": report_date, "bucket": report_bucket, diff --git a/lambdas/handlers/ses_feedback_monitor_handler.py b/lambdas/handlers/ses_feedback_monitor_handler.py index 800f58012..6f1b0003c 100644 --- a/lambdas/handlers/ses_feedback_monitor_handler.py +++ b/lambdas/handlers/ses_feedback_monitor_handler.py @@ -1,9 +1,12 @@ import os -import boto3 from typing import Any, Dict +import boto3 from services.email_service import EmailService -from services.ses_feedback_monitor_service import SesFeedbackMonitorConfig, SesFeedbackMonitorService +from services.ses_feedback_monitor_service import ( + SesFeedbackMonitorConfig, + SesFeedbackMonitorService, +) def parse_alert_types(configured: str) -> set[str]: @@ -16,9 +19,7 @@ def lambda_handler(event, context) -> Dict[str, Any]: feedback_prefix=os.environ["SES_FEEDBACK_PREFIX"], prm_mailbox=os.environ["PRM_MAILBOX_EMAIL"], from_address=os.environ["SES_FROM_ADDRESS"], - alert_on_event_types=parse_alert_types( - os.environ["ALERT_ON_EVENT_TYPES"] - ), + alert_on_event_types=parse_alert_types(os.environ["ALERT_ON_EVENT_TYPES"]), ) service = SesFeedbackMonitorService( diff --git a/lambdas/repositories/reporting/reporting_dynamo_repository.py b/lambdas/repositories/reporting/reporting_dynamo_repository.py index 86d595d87..0afb2300d 100644 --- a/lambdas/repositories/reporting/reporting_dynamo_repository.py +++ b/lambdas/repositories/reporting/reporting_dynamo_repository.py @@ -4,10 +4,15 @@ from boto3.dynamodb.conditions import Key from services.base.dynamo_service import DynamoDBService from utils.audit_logging_setup import LoggingService -from utils.utilities import utc_date_string, utc_date, utc_day_start_timestamp, utc_day_end_timestamp +from utils.utilities import ( + utc_date, + utc_day_end_timestamp, + utc_day_start_timestamp, +) logger = LoggingService(__name__) + class ReportingDynamoRepository: def __init__(self, table_name: str): self.table_name = table_name @@ -37,10 +42,9 @@ def get_records_for_time_window( effective_start_ts = max(start_timestamp, day_start_ts) effective_end_ts = min(end_timestamp, day_end_ts) - key_condition = ( - Key("Date").eq(current_date.isoformat()) - & Key("Timestamp").between(effective_start_ts, effective_end_ts) - ) + key_condition = Key("Date").eq(current_date.isoformat()) & Key( + "Timestamp" + ).between(effective_start_ts, effective_end_ts) records_for_day = self.dynamo_service.query_by_key_condition_expression( table_name=self.table_name, @@ -51,4 +55,4 @@ def get_records_for_time_window( records_for_window.extend(records_for_day) current_date += timedelta(days=1) - return records_for_window \ No newline at end of file + return records_for_window diff --git a/lambdas/services/email_service.py b/lambdas/services/email_service.py index 6a9e9b083..5467d8812 100644 --- a/lambdas/services/email_service.py +++ b/lambdas/services/email_service.py @@ -1,9 +1,9 @@ -import boto3 +from email.mime.application import MIMEApplication from email.mime.multipart import MIMEMultipart from email.mime.text import MIMEText -from email.mime.application import MIMEApplication -from typing import Iterable, Optional, Dict, Any +from typing import Any, Dict, Iterable, Optional +import boto3 from utils.audit_logging_setup import LoggingService logger = LoggingService(__name__) @@ -97,7 +97,9 @@ def _send_raw( resp = self.ses.send_raw_email(**kwargs) - logger.info(f"SES accepted email: subject={subject!r}, message_id={resp.get('MessageId')}") + logger.info( + f"SES accepted email: subject={subject!r}, message_id={resp.get('MessageId')}" + ) return resp def send_report_email( diff --git a/lambdas/services/reporting/report_distribution_service.py b/lambdas/services/reporting/report_distribution_service.py index 1c86409b6..0afca276b 100644 --- a/lambdas/services/reporting/report_distribution_service.py +++ b/lambdas/services/reporting/report_distribution_service.py @@ -1,10 +1,9 @@ import re import secrets import tempfile -from typing import List, Dict +from typing import Dict, List import boto3 - from repositories.reporting.report_contact_repository import ReportContactRepository from services.base.s3_service import S3Service from services.email_service import EmailService @@ -14,9 +13,12 @@ logger = LoggingService(__name__) _SES_TAG_VALUE_ALLOWED = re.compile(r"[^A-Za-z0-9_\-\.@]") + + def _sanitize_ses_tag_value(value: str) -> str: return _SES_TAG_VALUE_ALLOWED.sub("_", str(value)) + class ReportDistributionService: def __init__( self, diff --git a/lambdas/services/ses_feedback_monitor_service.py b/lambdas/services/ses_feedback_monitor_service.py index 5df4d7855..6a924f473 100644 --- a/lambdas/services/ses_feedback_monitor_service.py +++ b/lambdas/services/ses_feedback_monitor_service.py @@ -1,10 +1,10 @@ import json from dataclasses import dataclass from datetime import datetime, timezone -from typing import Any, Dict, List, Optional, Tuple, Protocol +from typing import Any, Dict, List, Optional, Protocol, Tuple -from utils.audit_logging_setup import LoggingService from services.email_service import EmailService +from utils.audit_logging_setup import LoggingService logger = LoggingService(__name__) @@ -24,7 +24,13 @@ class SesFeedbackMonitorConfig: class SesFeedbackMonitorService: - def __init__(self, *, s3_client: S3Client, email_service: EmailService, config: SesFeedbackMonitorConfig): + def __init__( + self, + *, + s3_client: S3Client, + email_service: EmailService, + config: SesFeedbackMonitorConfig, + ): self.s3 = s3_client self.email_service = email_service self.config = config @@ -36,12 +42,18 @@ def parse_sns_message(record: Dict[str, Any]) -> Dict[str, Any]: @staticmethod def event_type(payload: Dict[str, Any]) -> str: - return (payload.get("eventType") or payload.get("notificationType") or "UNKNOWN").upper() + return ( + payload.get("eventType") or payload.get("notificationType") or "UNKNOWN" + ).upper() @staticmethod def message_id(payload: Dict[str, Any]) -> str: mail = payload.get("mail") or {} - return mail.get("messageId") or payload.get("mailMessageId") or "unknown-message-id" + return ( + mail.get("messageId") + or payload.get("mailMessageId") + or "unknown-message-id" + ) @staticmethod def extract_tags(payload: Dict[str, Any]) -> Dict[str, List[str]]: @@ -50,7 +62,9 @@ def extract_tags(payload: Dict[str, Any]) -> Dict[str, List[str]]: return tags if isinstance(tags, dict) else {} @staticmethod - def extract_recipients_and_diagnostic(payload: Dict[str, Any]) -> Tuple[List[str], Optional[str]]: + def extract_recipients_and_diagnostic( + payload: Dict[str, Any] + ) -> Tuple[List[str], Optional[str]]: recipients: List[str] = [] diagnostic: Optional[str] = None @@ -105,7 +119,9 @@ def process_ses_feedback_event(self, event: Dict[str, Any]) -> Dict[str, Any]: ContentType="application/json", ) stored += 1 - logger.info(f"Stored SES feedback event: type={et}, message_id={mid}, s3=s3://{self.config.feedback_bucket}/{s3_key}") + logger.info( + f"Stored SES feedback event: type={et}, message_id={mid}, s3=s3://{self.config.feedback_bucket}/{s3_key}" + ) if et in self.config.alert_on_event_types: subject, body = self.build_prm_email(payload, s3_key) @@ -116,7 +132,9 @@ def process_ses_feedback_event(self, event: Dict[str, Any]) -> Dict[str, Any]: body_text=body, ) alerted += 1 - logger.info(f"Emailed PRM for SES feedback event: type={et}, message_id={mid}") + logger.info( + f"Emailed PRM for SES feedback event: type={et}, message_id={mid}" + ) return {"status": "ok", "stored": stored, "alerted": alerted} diff --git a/lambdas/tests/unit/handlers/test_report_distribution_handler.py b/lambdas/tests/unit/handlers/test_report_distribution_handler.py index d129d2ed3..b0d319ceb 100644 --- a/lambdas/tests/unit/handlers/test_report_distribution_handler.py +++ b/lambdas/tests/unit/handlers/test_report_distribution_handler.py @@ -1,5 +1,6 @@ import importlib import os + import pytest MODULE_UNDER_TEST = "handlers.report_distribution_handler" @@ -19,6 +20,7 @@ def required_env(mocker): "CONTACT_TABLE_NAME": "contact-table", "PRM_MAILBOX_EMAIL": "prm@example.com", "SES_FROM_ADDRESS": "from@example.com", + "SES_CONFIGURATION_SET": "my-config-set", }, clear=False, ) @@ -61,7 +63,7 @@ def test_lambda_handler_wires_dependencies_and_returns_result_list_mode( mocked_s3_cls.assert_called_once_with() mocked_contact_repo_cls.assert_called_once_with("contact-table") - mocked_email_cls.assert_called_once_with() + mocked_email_cls.assert_called_once_with(default_configuration_set="my-config-set") mocked_dist_svc_cls.assert_called_once_with( s3_service=s3_instance, @@ -90,10 +92,24 @@ def test_lambda_handler_uses_bucket_from_event_when_provided_list_mode( svc_instance = mocker.Mock() svc_instance.list_xlsx_keys.return_value = [] - mocker.patch.object(handler_module, "ReportDistributionService", autospec=True, return_value=svc_instance) - mocker.patch.object(handler_module, "S3Service", autospec=True, return_value=mocker.Mock()) - mocker.patch.object(handler_module, "ReportContactRepository", autospec=True, return_value=mocker.Mock()) - mocker.patch.object(handler_module, "EmailService", autospec=True, return_value=mocker.Mock()) + mocker.patch.object( + handler_module, + "ReportDistributionService", + autospec=True, + return_value=svc_instance, + ) + mocker.patch.object( + handler_module, "S3Service", autospec=True, return_value=mocker.Mock() + ) + mocker.patch.object( + handler_module, + "ReportContactRepository", + autospec=True, + return_value=mocker.Mock(), + ) + mocker.patch.object( + handler_module, "EmailService", autospec=True, return_value=mocker.Mock() + ) result = handler_module.lambda_handler(event, context) @@ -112,15 +128,33 @@ def test_lambda_handler_process_one_mode_happy_path( svc_instance.extract_ods_code_from_key.return_value = "ABC" svc_instance.process_one_report.return_value = None - mocker.patch.object(handler_module, "ReportDistributionService", autospec=True, return_value=svc_instance) - mocker.patch.object(handler_module, "S3Service", autospec=True, return_value=mocker.Mock()) - mocker.patch.object(handler_module, "ReportContactRepository", autospec=True, return_value=mocker.Mock()) - mocker.patch.object(handler_module, "EmailService", autospec=True, return_value=mocker.Mock()) + mocker.patch.object( + handler_module, + "ReportDistributionService", + autospec=True, + return_value=svc_instance, + ) + mocker.patch.object( + handler_module, "S3Service", autospec=True, return_value=mocker.Mock() + ) + mocker.patch.object( + handler_module, + "ReportContactRepository", + autospec=True, + return_value=mocker.Mock(), + ) + mocker.patch.object( + handler_module, "EmailService", autospec=True, return_value=mocker.Mock() + ) result = handler_module.lambda_handler(event, context) - svc_instance.extract_ods_code_from_key.assert_called_once_with("reports/ABC/whatever.xlsx") - svc_instance.process_one_report.assert_called_once_with(ods_code="ABC", key="reports/ABC/whatever.xlsx") + svc_instance.extract_ods_code_from_key.assert_called_once_with( + "reports/ABC/whatever.xlsx" + ) + svc_instance.process_one_report.assert_called_once_with( + ods_code="ABC", key="reports/ABC/whatever.xlsx" + ) assert result == { "status": "ok", diff --git a/lambdas/tests/unit/handlers/test_ses_feedback_monitor_handler.py b/lambdas/tests/unit/handlers/test_ses_feedback_monitor_handler.py new file mode 100644 index 000000000..b0d319ceb --- /dev/null +++ b/lambdas/tests/unit/handlers/test_ses_feedback_monitor_handler.py @@ -0,0 +1,164 @@ +import importlib +import os + +import pytest + +MODULE_UNDER_TEST = "handlers.report_distribution_handler" + + +@pytest.fixture +def handler_module(): + return importlib.import_module(MODULE_UNDER_TEST) + + +@pytest.fixture +def required_env(mocker): + mocker.patch.dict( + os.environ, + { + "REPORT_BUCKET_NAME": "my-report-bucket", + "CONTACT_TABLE_NAME": "contact-table", + "PRM_MAILBOX_EMAIL": "prm@example.com", + "SES_FROM_ADDRESS": "from@example.com", + "SES_CONFIGURATION_SET": "my-config-set", + }, + clear=False, + ) + + +def test_lambda_handler_wires_dependencies_and_returns_result_list_mode( + mocker, handler_module, required_env +): + event = {"action": "list", "prefix": "reports/2026-01-01/"} + context = mocker.Mock() + context.aws_request_id = "req-123" # avoid JSON serialization issues in decorators + + s3_instance = mocker.Mock(name="S3ServiceInstance") + contact_repo_instance = mocker.Mock(name="ReportContactRepositoryInstance") + email_instance = mocker.Mock(name="EmailServiceInstance") + + svc_instance = mocker.Mock(name="ReportDistributionServiceInstance") + svc_instance.list_xlsx_keys.return_value = ["a.xlsx", "b.xlsx"] + + mocked_s3_cls = mocker.patch.object( + handler_module, "S3Service", autospec=True, return_value=s3_instance + ) + mocked_contact_repo_cls = mocker.patch.object( + handler_module, + "ReportContactRepository", + autospec=True, + return_value=contact_repo_instance, + ) + mocked_email_cls = mocker.patch.object( + handler_module, "EmailService", autospec=True, return_value=email_instance + ) + mocked_dist_svc_cls = mocker.patch.object( + handler_module, + "ReportDistributionService", + autospec=True, + return_value=svc_instance, + ) + + result = handler_module.lambda_handler(event, context) + + mocked_s3_cls.assert_called_once_with() + mocked_contact_repo_cls.assert_called_once_with("contact-table") + mocked_email_cls.assert_called_once_with(default_configuration_set="my-config-set") + + mocked_dist_svc_cls.assert_called_once_with( + s3_service=s3_instance, + contact_repo=contact_repo_instance, + email_service=email_instance, + bucket="my-report-bucket", + from_address="from@example.com", + prm_mailbox="prm@example.com", + ) + + svc_instance.list_xlsx_keys.assert_called_once_with(prefix="reports/2026-01-01/") + assert result == { + "bucket": "my-report-bucket", + "prefix": "reports/2026-01-01/", + "keys": ["a.xlsx", "b.xlsx"], + } + + +def test_lambda_handler_uses_bucket_from_event_when_provided_list_mode( + mocker, handler_module, required_env +): + event = {"action": "list", "prefix": "p/", "bucket": "override-bucket"} + context = mocker.Mock() + context.aws_request_id = "req-456" + + svc_instance = mocker.Mock() + svc_instance.list_xlsx_keys.return_value = [] + + mocker.patch.object( + handler_module, + "ReportDistributionService", + autospec=True, + return_value=svc_instance, + ) + mocker.patch.object( + handler_module, "S3Service", autospec=True, return_value=mocker.Mock() + ) + mocker.patch.object( + handler_module, + "ReportContactRepository", + autospec=True, + return_value=mocker.Mock(), + ) + mocker.patch.object( + handler_module, "EmailService", autospec=True, return_value=mocker.Mock() + ) + + result = handler_module.lambda_handler(event, context) + + svc_instance.list_xlsx_keys.assert_called_once_with(prefix="p/") + assert result == {"bucket": "override-bucket", "prefix": "p/", "keys": []} + + +def test_lambda_handler_process_one_mode_happy_path( + mocker, handler_module, required_env +): + event = {"action": "process_one", "key": "reports/ABC/whatever.xlsx"} + context = mocker.Mock() + context.aws_request_id = "req-789" + + svc_instance = mocker.Mock() + svc_instance.extract_ods_code_from_key.return_value = "ABC" + svc_instance.process_one_report.return_value = None + + mocker.patch.object( + handler_module, + "ReportDistributionService", + autospec=True, + return_value=svc_instance, + ) + mocker.patch.object( + handler_module, "S3Service", autospec=True, return_value=mocker.Mock() + ) + mocker.patch.object( + handler_module, + "ReportContactRepository", + autospec=True, + return_value=mocker.Mock(), + ) + mocker.patch.object( + handler_module, "EmailService", autospec=True, return_value=mocker.Mock() + ) + + result = handler_module.lambda_handler(event, context) + + svc_instance.extract_ods_code_from_key.assert_called_once_with( + "reports/ABC/whatever.xlsx" + ) + svc_instance.process_one_report.assert_called_once_with( + ods_code="ABC", key="reports/ABC/whatever.xlsx" + ) + + assert result == { + "status": "ok", + "bucket": "my-report-bucket", + "key": "reports/ABC/whatever.xlsx", + "ods_code": "ABC", + } diff --git a/lambdas/tests/unit/services/reporting/test_email_service.py b/lambdas/tests/unit/services/reporting/test_email_service.py index efde7f725..fe5912834 100644 --- a/lambdas/tests/unit/services/reporting/test_email_service.py +++ b/lambdas/tests/unit/services/reporting/test_email_service.py @@ -1,5 +1,4 @@ import pytest - from services.email_service import EmailService @@ -10,6 +9,7 @@ def email_service(mocker): svc.ses = mocker.Mock() return svc + def test_send_email_sends_raw_email_without_attachments(email_service, mocker): mocked_send_raw = mocker.patch.object(email_service, "_send_raw", autospec=True) @@ -22,14 +22,14 @@ def test_send_email_sends_raw_email_without_attachments(email_service, mocker): ) mocked_send_raw.assert_called_once() + call_kwargs = mocked_send_raw.call_args.kwargs - call_args, call_kwargs = mocked_send_raw.call_args - assert call_kwargs == {} + assert set(call_kwargs.keys()) == {"msg", "to_address", "configuration_set", "tags"} + assert call_kwargs["to_address"] == "to@example.com" + assert call_kwargs["configuration_set"] is None + assert call_kwargs["tags"] is None - msg_arg = call_args[0] - to_arg = call_args[1] - - assert to_arg == "to@example.com" + msg_arg = call_kwargs["msg"] assert msg_arg["Subject"] == "Hello" assert msg_arg["To"] == "to@example.com" assert msg_arg["From"] == "from@example.com" @@ -38,6 +38,60 @@ def test_send_email_sends_raw_email_without_attachments(email_service, mocker): assert "Body text" in raw +def test_send_email_uses_default_configuration_set_when_not_provided(mocker): + mocker.patch("services.email_service.boto3.client", autospec=True) + svc = EmailService(default_configuration_set="DEFAULT_CFG") + svc.ses = mocker.Mock() + + mocked_send_raw = mocker.patch.object(svc, "_send_raw", autospec=True) + + svc.send_email( + to_address="to@example.com", + subject="Hello", + body_text="Body text", + from_address="from@example.com", + attachments=None, + configuration_set=None, + ) + + mocked_send_raw.assert_called_once() + assert mocked_send_raw.call_args.kwargs["configuration_set"] == "DEFAULT_CFG" + + +def test_send_email_configuration_set_overrides_default(mocker): + mocker.patch("services.email_service.boto3.client", autospec=True) + svc = EmailService(default_configuration_set="DEFAULT_CFG") + svc.ses = mocker.Mock() + + mocked_send_raw = mocker.patch.object(svc, "_send_raw", autospec=True) + + svc.send_email( + to_address="to@example.com", + subject="Hello", + body_text="Body text", + from_address="from@example.com", + attachments=None, + configuration_set="OVERRIDE_CFG", + ) + + mocked_send_raw.assert_called_once() + assert mocked_send_raw.call_args.kwargs["configuration_set"] == "OVERRIDE_CFG" + + +def test_send_email_passes_tags_through_to_send_raw(email_service, mocker): + mocked_send_raw = mocker.patch.object(email_service, "_send_raw", autospec=True) + + email_service.send_email( + to_address="to@example.com", + subject="Hello", + body_text="Body text", + from_address="from@example.com", + tags={"k1": "v1", "k2": "v2"}, + ) + + mocked_send_raw.assert_called_once() + assert mocked_send_raw.call_args.kwargs["tags"] == {"k1": "v1", "k2": "v2"} + def test_send_email_attaches_files_and_sets_filenames(email_service, mocker): file_bytes_1 = b"zipbytes1" @@ -64,10 +118,7 @@ def test_send_email_attaches_files_and_sets_filenames(email_service, mocker): mocked_open.assert_any_call("/var/tmp/b.zip", "rb") mocked_send_raw.assert_called_once() - - call_args, call_kwargs = mocked_send_raw.call_args - assert call_kwargs == {} - msg = call_args[0] + msg = mocked_send_raw.call_args.kwargs["msg"] raw = msg.as_string() assert 'filename="a.zip"' in raw @@ -75,25 +126,57 @@ def test_send_email_attaches_files_and_sets_filenames(email_service, mocker): assert "See attached" in raw - -def test_send_raw_calls_ses_send_raw_email(email_service, mocker): +def test_send_raw_calls_ses_send_raw_email_minimal(email_service): from email.mime.multipart import MIMEMultipart + msg = MIMEMultipart() msg["Subject"] = "S" msg["To"] = "to@example.com" msg["From"] = "from@example.com" - email_service._send_raw(msg, "to@example.com") + email_service.ses.send_raw_email.return_value = {"MessageId": "abc123"} + + resp = email_service._send_raw(msg=msg, to_address="to@example.com") + assert resp == {"MessageId": "abc123"} email_service.ses.send_raw_email.assert_called_once() call_kwargs = email_service.ses.send_raw_email.call_args.kwargs + assert call_kwargs["Source"] == "from@example.com" assert call_kwargs["Destinations"] == ["to@example.com"] assert "RawMessage" in call_kwargs assert "Data" in call_kwargs["RawMessage"] assert isinstance(call_kwargs["RawMessage"]["Data"], str) assert "Subject: S" in call_kwargs["RawMessage"]["Data"] + assert "ConfigurationSetName" not in call_kwargs + assert "Tags" not in call_kwargs + + +def test_send_raw_includes_configuration_set_and_tags(email_service): + from email.mime.multipart import MIMEMultipart + + msg = MIMEMultipart() + msg["Subject"] = "S" + msg["To"] = "to@example.com" + msg["From"] = "from@example.com" + + email_service.ses.send_raw_email.return_value = {"MessageId": "abc123"} + + email_service._send_raw( + msg=msg, + to_address="to@example.com", + configuration_set="CFG", + tags={"env": "test", "team": "data"}, + ) + + call_kwargs = email_service.ses.send_raw_email.call_args.kwargs + assert call_kwargs["ConfigurationSetName"] == "CFG" + assert call_kwargs["Tags"] == [ + {"Name": "env", "Value": "test"}, + {"Name": "team", "Value": "data"}, + ] + def test_send_report_email_calls_send_email_with_expected_inputs(email_service, mocker): mocked_send_email = mocker.patch.object(email_service, "send_email", autospec=True) @@ -102,6 +185,7 @@ def test_send_report_email_calls_send_email_with_expected_inputs(email_service, to_address="to@example.com", from_address="from@example.com", attachment_path="/tmp/report.zip", + tags={"k": "v"}, ) mocked_send_email.assert_called_once_with( @@ -110,16 +194,20 @@ def test_send_report_email_calls_send_email_with_expected_inputs(email_service, subject="Daily Upload Report", body_text="Please find your encrypted daily upload report attached.", attachments=["/tmp/report.zip"], + tags={"k": "v"}, ) -def test_send_password_email_calls_send_email_with_expected_inputs(email_service, mocker): +def test_send_password_email_calls_send_email_with_expected_inputs( + email_service, mocker +): mocked_send_email = mocker.patch.object(email_service, "send_email", autospec=True) email_service.send_password_email( to_address="to@example.com", from_address="from@example.com", password="pw123", + tags={"k": "v"}, ) mocked_send_email.assert_called_once_with( @@ -127,10 +215,13 @@ def test_send_password_email_calls_send_email_with_expected_inputs(email_service from_address="from@example.com", subject="Daily Upload Report Password", body_text="Password for your report:\n\npw123", + tags={"k": "v"}, ) -def test_send_prm_missing_contact_email_calls_send_email_with_expected_inputs(email_service, mocker): +def test_send_prm_missing_contact_email_calls_send_email_with_expected_inputs( + email_service, mocker +): mocked_send_email = mocker.patch.object(email_service, "send_email", autospec=True) email_service.send_prm_missing_contact_email( @@ -139,6 +230,7 @@ def test_send_prm_missing_contact_email_calls_send_email_with_expected_inputs(em ods_code="Y12345", attachment_path="/tmp/report.zip", password="pw123", + tags={"k": "v"}, ) mocked_send_email.assert_called_once_with( @@ -151,4 +243,5 @@ def test_send_prm_missing_contact_email_calls_send_email_with_expected_inputs(em "Please resolve the contact and forward the report." ), attachments=["/tmp/report.zip"], + tags={"k": "v"}, ) diff --git a/lambdas/tests/unit/services/reporting/test_report_distribution_service.py b/lambdas/tests/unit/services/reporting/test_report_distribution_service.py index 56b5150e0..850982db4 100644 --- a/lambdas/tests/unit/services/reporting/test_report_distribution_service.py +++ b/lambdas/tests/unit/services/reporting/test_report_distribution_service.py @@ -1,7 +1,11 @@ import os + import pytest +from services.reporting.report_distribution_service import ( + ReportDistributionService, + _sanitize_ses_tag_value, +) -from services.reporting.report_distribution_service import ReportDistributionService @pytest.fixture def mock_s3_service(mocker): @@ -22,7 +26,9 @@ def mock_email_service(mocker): @pytest.fixture def service(mocker, mock_s3_service, mock_contact_repo, mock_email_service): - mocker.patch("services.reporting.report_distribution_service.boto3.client", autospec=True) + mocker.patch( + "services.reporting.report_distribution_service.boto3.client", autospec=True + ) return ReportDistributionService( s3_service=mock_s3_service, @@ -33,18 +39,34 @@ def service(mocker, mock_s3_service, mock_contact_repo, mock_email_service): prm_mailbox="prm@example.com", ) + +def test_sanitize_ses_tag_value_replaces_disallowed_chars(): + assert _sanitize_ses_tag_value("A B/C") == "A_B_C" + assert _sanitize_ses_tag_value("x@y.com") == "x@y.com" # @ allowed + assert _sanitize_ses_tag_value("a.b-c_d") == "a.b-c_d" # . - _ allowed + + def test_extract_ods_code_from_key_strips_xlsx_extension(): - assert ReportDistributionService.extract_ods_code_from_key( - "Report-Orchestration/2026-01-01/Y12345.xlsx" - ) == "Y12345" + assert ( + ReportDistributionService.extract_ods_code_from_key( + "Report-Orchestration/2026-01-01/Y12345.xlsx" + ) + == "Y12345" + ) def test_extract_ods_code_from_key_is_case_insensitive(): - assert ReportDistributionService.extract_ods_code_from_key("a/b/C789.XLSX") == "C789" + assert ( + ReportDistributionService.extract_ods_code_from_key("a/b/C789.XLSX") == "C789" + ) def test_extract_ods_code_from_key_keeps_non_xlsx_filename(): - assert ReportDistributionService.extract_ods_code_from_key("a/b/report.csv") == "report.csv" + assert ( + ReportDistributionService.extract_ods_code_from_key("a/b/report.csv") + == "report.csv" + ) + def test_list_xlsx_keys_filters_only_xlsx(service, mocker): paginator = mocker.Mock() @@ -141,14 +163,52 @@ def test_process_one_report_downloads_encrypts_and_delegates_email( password="fixed-password", ) - mocked_send.assert_called_once_with( - ods_code="Y12345", - attachment_path=local_zip, - password="fixed-password", + # Updated: send_report_emails now requires base_tags + mocked_send.assert_called_once() + call_kwargs = mocked_send.call_args.kwargs + assert call_kwargs["ods_code"] == "Y12345" + assert call_kwargs["attachment_path"] == local_zip + assert call_kwargs["password"] == "fixed-password" + assert call_kwargs["base_tags"] == { + "ods_code": "Y12345", + "report_key": "Report-Orchestration_2026-01-01_Y12345.xlsx", + } + + +def test_process_one_report_sanitizes_tags(service, mocker, mock_s3_service): + mocker.patch( + "services.reporting.report_distribution_service.secrets.token_urlsafe", + return_value="pw", + ) + + fake_tmp = "/tmp/fake_tmpdir" + td = mocker.MagicMock() + td.__enter__.return_value = fake_tmp + td.__exit__.return_value = False + mocker.patch( + "services.reporting.report_distribution_service.tempfile.TemporaryDirectory", + return_value=td, ) + mocker.patch( + "services.reporting.report_distribution_service.zip_encrypt_file", autospec=True + ) + mocked_send = mocker.patch.object(service, "send_report_emails", autospec=True) + + service.process_one_report( + ods_code="Y 12/345", + key="prefix/2026-01-01/Y 12/345.xlsx", + ) + + assert mocked_send.call_args.kwargs["base_tags"] == { + "ods_code": "Y_12_345", + "report_key": "prefix_2026-01-01_Y_12_345.xlsx", + } -def test_process_one_report_propagates_download_errors(service, mocker, mock_s3_service): + +def test_process_one_report_propagates_download_errors( + service, mocker, mock_s3_service +): mock_s3_service.download_file.side_effect = RuntimeError("download failed") td = mocker.MagicMock() @@ -159,7 +219,10 @@ def test_process_one_report_propagates_download_errors(service, mocker, mock_s3_ return_value=td, ) - mocked_zip = mocker.patch("services.reporting.report_distribution_service.zip_encrypt_file", autospec=True) + mocked_zip = mocker.patch( + "services.reporting.report_distribution_service.zip_encrypt_file", + autospec=True, + ) mocked_send = mocker.patch.object(service, "send_report_emails", autospec=True) with pytest.raises(RuntimeError, match="download failed"): @@ -169,7 +232,7 @@ def test_process_one_report_propagates_download_errors(service, mocker, mock_s3_ mocked_send.assert_not_called() -def test_process_one_report_does_not_send_email_if_zip_fails(service, mocker, mock_s3_service): +def test_process_one_report_does_not_send_email_if_zip_fails(service, mocker): mocker.patch( "services.reporting.report_distribution_service.secrets.token_urlsafe", return_value="pw", @@ -231,16 +294,22 @@ def test_process_one_report_does_not_zip_or_send_email_if_password_generation_fa mocked_zip.assert_not_called() mocked_send.assert_not_called() -def test_send_report_emails_with_contact_calls_email_contact(service, mock_contact_repo, mocker): + +def test_send_report_emails_with_contact_calls_email_contact( + service, mock_contact_repo, mocker +): mock_contact_repo.get_contact_email.return_value = "contact@example.com" mocked_email_contact = mocker.patch.object(service, "email_contact", autospec=True) - mocked_email_prm = mocker.patch.object(service, "email_prm_missing_contact", autospec=True) + mocked_email_prm = mocker.patch.object( + service, "email_prm_missing_contact", autospec=True + ) service.send_report_emails( ods_code="Y12345", attachment_path="/tmp/Y12345.zip", password="pw", + base_tags={"ods_code": "Y12345", "report_key": "k.xlsx"}, ) mock_contact_repo.get_contact_email.assert_called_once_with("Y12345") @@ -248,20 +317,26 @@ def test_send_report_emails_with_contact_calls_email_contact(service, mock_conta to_address="contact@example.com", attachment_path="/tmp/Y12345.zip", password="pw", + base_tags={"ods_code": "Y12345", "report_key": "k.xlsx"}, ) mocked_email_prm.assert_not_called() -def test_send_report_emails_without_contact_calls_email_prm(service, mock_contact_repo, mocker): +def test_send_report_emails_without_contact_calls_email_prm( + service, mock_contact_repo, mocker +): mock_contact_repo.get_contact_email.return_value = None mocked_email_contact = mocker.patch.object(service, "email_contact", autospec=True) - mocked_email_prm = mocker.patch.object(service, "email_prm_missing_contact", autospec=True) + mocked_email_prm = mocker.patch.object( + service, "email_prm_missing_contact", autospec=True + ) service.send_report_emails( ods_code="A99999", attachment_path="/tmp/A99999.zip", password="pw", + base_tags={"ods_code": "A99999", "report_key": "k.xlsx"}, ) mock_contact_repo.get_contact_email.assert_called_once_with("A99999") @@ -269,20 +344,26 @@ def test_send_report_emails_without_contact_calls_email_prm(service, mock_contac ods_code="A99999", attachment_path="/tmp/A99999.zip", password="pw", + base_tags={"ods_code": "A99999", "report_key": "k.xlsx"}, ) mocked_email_contact.assert_not_called() -def test_send_report_emails_contact_lookup_exception_falls_back_to_prm(service, mock_contact_repo, mocker): +def test_send_report_emails_contact_lookup_exception_falls_back_to_prm( + service, mock_contact_repo, mocker +): mock_contact_repo.get_contact_email.side_effect = RuntimeError("ddb down") mocked_email_contact = mocker.patch.object(service, "email_contact", autospec=True) - mocked_email_prm = mocker.patch.object(service, "email_prm_missing_contact", autospec=True) + mocked_email_prm = mocker.patch.object( + service, "email_prm_missing_contact", autospec=True + ) service.send_report_emails( ods_code="A99999", attachment_path="/tmp/A99999.zip", password="pw", + base_tags={"ods_code": "A99999", "report_key": "k.xlsx"}, ) mocked_email_contact.assert_not_called() @@ -290,29 +371,40 @@ def test_send_report_emails_contact_lookup_exception_falls_back_to_prm(service, ods_code="A99999", attachment_path="/tmp/A99999.zip", password="pw", + base_tags={"ods_code": "A99999", "report_key": "k.xlsx"}, ) -def test_email_contact_sends_report_and_password(service, mock_email_service): +def test_email_contact_sends_report_and_password_with_tags(service, mock_email_service): + base_tags = {"ods_code": "Y12345", "report_key": "k.xlsx"} + service.email_contact( to_address="contact@example.com", attachment_path="/tmp/file.zip", password="pw", + base_tags=base_tags, ) + expected_tags = {**base_tags, "email": "contact@example.com"} + mock_email_service.send_report_email.assert_called_once_with( to_address="contact@example.com", from_address="from@example.com", attachment_path="/tmp/file.zip", + tags=expected_tags, ) mock_email_service.send_password_email.assert_called_once_with( to_address="contact@example.com", from_address="from@example.com", password="pw", + tags=expected_tags, ) -def test_email_contact_sends_password_even_if_report_email_fails(service, mock_email_service): +def test_email_contact_does_not_send_password_if_report_email_fails( + service, mock_email_service +): + base_tags = {"ods_code": "Y12345", "report_key": "k.xlsx"} mock_email_service.send_report_email.side_effect = RuntimeError("SES down") with pytest.raises(RuntimeError, match="SES down"): @@ -320,22 +412,31 @@ def test_email_contact_sends_password_even_if_report_email_fails(service, mock_e to_address="contact@example.com", attachment_path="/tmp/file.zip", password="pw", + base_tags=base_tags, ) mock_email_service.send_password_email.assert_not_called() -def test_email_prm_missing_contact_sends_prm_missing_contact_email(service, mock_email_service): +def test_email_prm_missing_contact_sends_prm_missing_contact_email_with_tags( + service, mock_email_service +): + base_tags = {"ods_code": "X11111", "report_key": "k.xlsx"} + service.email_prm_missing_contact( ods_code="X11111", attachment_path="/tmp/file.zip", password="pw", + base_tags=base_tags, ) + expected_tags = {**base_tags, "email": "prm@example.com"} + mock_email_service.send_prm_missing_contact_email.assert_called_once_with( prm_mailbox="prm@example.com", from_address="from@example.com", ods_code="X11111", attachment_path="/tmp/file.zip", password="pw", + tags=expected_tags, ) diff --git a/lambdas/tests/unit/services/test_ses_feedback_monitor_service.py b/lambdas/tests/unit/services/test_ses_feedback_monitor_service.py new file mode 100644 index 000000000..7cc164f06 --- /dev/null +++ b/lambdas/tests/unit/services/test_ses_feedback_monitor_service.py @@ -0,0 +1,293 @@ +import json +from datetime import datetime, timezone + +import pytest +from services.ses_feedback_monitor_service import ( + SesFeedbackMonitorConfig, + SesFeedbackMonitorService, +) + + +@pytest.fixture +def config(): + return SesFeedbackMonitorConfig( + feedback_bucket="feedback-bucket", + feedback_prefix="ses/feedback", + prm_mailbox="prm@example.com", + from_address="from@example.com", + alert_on_event_types={"BOUNCE", "REJECT"}, + ) + + +@pytest.fixture +def s3_client(mocker): + return mocker.Mock() + + +@pytest.fixture +def email_service(mocker): + return mocker.Mock() + + +@pytest.fixture +def svc(s3_client, email_service, config): + return SesFeedbackMonitorService( + s3_client=s3_client, email_service=email_service, config=config + ) + + +def test_parse_sns_message_parses_json_string(): + record = {"Sns": {"Message": json.dumps({"a": 1, "eventType": "bounce"})}} + payload = SesFeedbackMonitorService.parse_sns_message(record) + assert payload == {"a": 1, "eventType": "bounce"} + + +def test_parse_sns_message_returns_non_string_message_as_is(): + record = {"Sns": {"Message": {"a": 1}}} + payload = SesFeedbackMonitorService.parse_sns_message(record) + assert payload == {"a": 1} + + +@pytest.mark.parametrize( + "payload, expected", + [ + ({"eventType": "bounce"}, "BOUNCE"), + ({"notificationType": "complaint"}, "COMPLAINT"), + ({"eventType": None, "notificationType": None}, "UNKNOWN"), + ({}, "UNKNOWN"), + ], +) +def test_event_type(payload, expected): + assert SesFeedbackMonitorService.event_type(payload) == expected + + +@pytest.mark.parametrize( + "payload, expected", + [ + ({"mail": {"messageId": "m1"}}, "m1"), + ({"mailMessageId": "legacy"}, "legacy"), + ({}, "unknown-message-id"), + ({"mail": {}}, "unknown-message-id"), + ], +) +def test_message_id(payload, expected): + assert SesFeedbackMonitorService.message_id(payload) == expected + + +def test_extract_tags_returns_dict_or_empty(): + assert SesFeedbackMonitorService.extract_tags({"mail": {"tags": {"k": ["v"]}}}) == { + "k": ["v"] + } + assert ( + SesFeedbackMonitorService.extract_tags({"mail": {"tags": ["not-a-dict"]}}) == {} + ) + assert SesFeedbackMonitorService.extract_tags({"mail": {}}) == {} + assert SesFeedbackMonitorService.extract_tags({}) == {} + + +def test_extract_recipients_and_diagnostic_for_bounce_uses_diagnostic_code_first(): + payload = { + "bounce": { + "bouncedRecipients": [ + {"emailAddress": "a@example.com", "diagnosticCode": "550 5.1.1 bad"}, + {"emailAddress": "b@example.com"}, + ], + "smtpResponse": "fallback smtp", + } + } + recipients, diagnostic = ( + SesFeedbackMonitorService.extract_recipients_and_diagnostic(payload) + ) + assert recipients == ["a@example.com", "b@example.com"] + assert diagnostic == "550 5.1.1 bad" + + +def test_extract_recipients_and_diagnostic_for_bounce_falls_back_to_smtp_response(): + payload = { + "bounce": { + "bouncedRecipients": [{"emailAddress": "a@example.com"}], + "smtpResponse": "smtp fallback", + } + } + recipients, diagnostic = ( + SesFeedbackMonitorService.extract_recipients_and_diagnostic(payload) + ) + assert recipients == ["a@example.com"] + assert diagnostic == "smtp fallback" + + +def test_extract_recipients_and_diagnostic_for_complaint(): + payload = { + "complaint": {"complainedRecipients": [{"emailAddress": "x@example.com"}]} + } + recipients, diagnostic = ( + SesFeedbackMonitorService.extract_recipients_and_diagnostic(payload) + ) + assert recipients == ["x@example.com"] + assert diagnostic is None + + +def test_extract_recipients_and_diagnostic_for_reject(): + payload = {"reject": {"reason": "Bad content"}} + recipients, diagnostic = ( + SesFeedbackMonitorService.extract_recipients_and_diagnostic(payload) + ) + assert recipients == [] + assert diagnostic == "Bad content" + + +def test_extract_recipients_and_diagnostic_unknown_payload(): + recipients, diagnostic = ( + SesFeedbackMonitorService.extract_recipients_and_diagnostic({"eventType": "x"}) + ) + assert recipients == [] + assert diagnostic is None + + +def test_build_s3_key_uses_prefix_and_date_and_message_id(mocker): + + fixed_now = datetime(2026, 1, 15, 12, 34, 56, tzinfo=timezone.utc) + + module = __import__(SesFeedbackMonitorService.__module__, fromlist=["datetime"]) + mock_dt = mocker.patch.object(module, "datetime", autospec=True) + mock_dt.now.return_value = fixed_now + + key = SesFeedbackMonitorService.build_s3_key( + prefix="ses/feedback/", + event_type="BOUNCE", + message_id="mid123", + ) + + assert key == "ses/feedback/BOUNCE/2026/01/15/mid123.json" + + +def test_build_s3_key_strips_trailing_slash(mocker): + + fixed_now = datetime(2026, 1, 15, 0, 0, 0, tzinfo=timezone.utc) + + module = __import__(SesFeedbackMonitorService.__module__, fromlist=["datetime"]) + mock_dt = mocker.patch.object(module, "datetime", autospec=True) + mock_dt.now.return_value = fixed_now + + key = SesFeedbackMonitorService.build_s3_key( + prefix="pfx////", event_type="REJECT", message_id="m" + ) + + assert key == "pfx/REJECT/2026/01/15/m.json" + + +def test_build_prm_email_includes_expected_fields(svc): + payload = { + "eventType": "bounce", + "mail": { + "messageId": "m-1", + "tags": { + "ods_code": ["Y12345"], + "report_key": ["Report-Orchestration/2026-01-01/Y12345.xlsx"], + "email": ["contact@example.com"], + }, + }, + "bounce": { + "bouncedRecipients": [ + {"emailAddress": "a@example.com", "diagnosticCode": "550 bad"} + ] + }, + } + + subject, body = svc.build_prm_email( + payload, s3_key="ses/feedback/BOUNCE/2026/01/15/m-1.json" + ) + + assert subject == "SES BOUNCE: messageId=m-1" + assert "Event type: BOUNCE" in body + assert "Message ID: m-1" in body + assert "Affected recipients: a@example.com" in body + assert "Diagnostic: 550 bad" in body + assert "ODS code tag: Y12345" in body + assert "Email tag: contact@example.com" in body + assert "Report key tag: Report-Orchestration/2026-01-01/Y12345.xlsx" in body + assert ( + "Stored at: s3://feedback-bucket/ses/feedback/BOUNCE/2026/01/15/m-1.json" + in body + ) + assert "Raw event JSON:" in body + assert '"eventType": "bounce"' in body + + +def test_process_ses_feedback_event_stores_each_record_and_alerts_only_configured_types( + svc, s3_client, email_service, mocker +): + mocker.patch.object( + SesFeedbackMonitorService, + "build_s3_key", + return_value="ses/feedback/BOUNCE/2026/01/15/m.json", + ) + + bounce_payload = { + "eventType": "bounce", + "mail": {"messageId": "m"}, + "bounce": {"bouncedRecipients": []}, + } + complaint_payload = { + "notificationType": "complaint", + "mail": {"messageId": "c"}, + "complaint": {"complainedRecipients": []}, + } + + event = { + "Records": [ + {"Sns": {"Message": json.dumps(bounce_payload)}}, + {"Sns": {"Message": json.dumps(complaint_payload)}}, + ] + } + + mocker.patch.object(svc, "build_prm_email", return_value=("subj", "body")) + + resp = svc.process_ses_feedback_event(event) + + assert resp == {"status": "ok", "stored": 2, "alerted": 1} + + assert s3_client.put_object.call_count == 2 + put1 = s3_client.put_object.call_args_list[0].kwargs + assert put1["Bucket"] == "feedback-bucket" + assert put1["Key"] == "ses/feedback/BOUNCE/2026/01/15/m.json" + assert put1["ContentType"] == "application/json" + assert json.loads(put1["Body"].decode("utf-8")) == bounce_payload + + email_service.send_email.assert_called_once_with( + to_address="prm@example.com", + from_address="from@example.com", + subject="subj", + body_text="body", + ) + + +def test_process_ses_feedback_event_handles_empty_records( + svc, s3_client, email_service +): + resp = svc.process_ses_feedback_event({"Records": []}) + assert resp == {"status": "ok", "stored": 0, "alerted": 0} + s3_client.put_object.assert_not_called() + email_service.send_email.assert_not_called() + + +def test_process_ses_feedback_event_message_can_be_dict_not_json_string( + svc, s3_client, email_service, mocker +): + mocker.patch.object( + SesFeedbackMonitorService, "build_s3_key", return_value="k.json" + ) + mocker.patch.object(svc, "build_prm_email", return_value=("s", "b")) + + payload = { + "eventType": "reject", + "mail": {"messageId": "m"}, + "reject": {"reason": "nope"}, + } + event = {"Records": [{"Sns": {"Message": payload}}]} + + resp = svc.process_ses_feedback_event(event) + + assert resp == {"status": "ok", "stored": 1, "alerted": 1} + s3_client.put_object.assert_called_once() + email_service.send_email.assert_called_once() From 0ff40e1af828c95e10c23779ab93e387cac34011 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Thu, 15 Jan 2026 15:25:55 +0000 Subject: [PATCH 5/8] [PRMP-1058] updated service logic --- .../handlers/ses_feedback_monitor_handler.py | 6 +- lambdas/services/base/s3_service.py | 18 ++ .../services/ses_feedback_monitor_service.py | 24 ++- .../test_ses_feedback_monitor_handler.py | 165 +++++------------- .../unit/services/base/test_s3_service.py | 106 ++++++++--- .../test_ses_feedback_monitor_service.py | 30 ++-- 6 files changed, 175 insertions(+), 174 deletions(-) diff --git a/lambdas/handlers/ses_feedback_monitor_handler.py b/lambdas/handlers/ses_feedback_monitor_handler.py index 6f1b0003c..0e254688d 100644 --- a/lambdas/handlers/ses_feedback_monitor_handler.py +++ b/lambdas/handlers/ses_feedback_monitor_handler.py @@ -1,7 +1,7 @@ import os from typing import Any, Dict -import boto3 +from services.base.s3_service import S3Service from services.email_service import EmailService from services.ses_feedback_monitor_service import ( SesFeedbackMonitorConfig, @@ -22,8 +22,10 @@ def lambda_handler(event, context) -> Dict[str, Any]: alert_on_event_types=parse_alert_types(os.environ["ALERT_ON_EVENT_TYPES"]), ) + s3_service = S3Service() + service = SesFeedbackMonitorService( - s3_client=boto3.client("s3"), + s3_service=s3_service, email_service=EmailService(), config=config, ) diff --git a/lambdas/services/base/s3_service.py b/lambdas/services/base/s3_service.py index 3ee42b4e2..e0f2e4a7e 100644 --- a/lambdas/services/base/s3_service.py +++ b/lambdas/services/base/s3_service.py @@ -1,4 +1,5 @@ import io +import json from datetime import datetime, timedelta, timezone from io import BytesIO from typing import Any, Mapping @@ -27,6 +28,8 @@ def __new__(cls, *args, **kwargs): def __init__(self, custom_aws_role=None): if not self.initialised: self.config = BotoConfig( + connect_timeout=3, + read_timeout=5, retries={"max_attempts": 3, "mode": "standard"}, s3={"addressing_style": "virtual"}, signature_version="s3v4", @@ -43,6 +46,21 @@ def __init__(self, custom_aws_role=None): self.custom_aws_role, "s3", config=self.config ) + def put_json( + self, + bucket: str, + key: str, + payload: Mapping[str, Any], + *, + content_type: str = "application/json", + ): + return self.client.put_object( + Bucket=bucket, + Key=key, + Body=json.dumps(payload).encode("utf-8"), + ContentType=content_type, + ) + # S3 Location should be a minimum of a s3_object_key but can also be a directory location in the form of # {{directory}}/{{s3_object_key}} def create_upload_presigned_url(self, s3_bucket_name: str, s3_object_location: str): diff --git a/lambdas/services/ses_feedback_monitor_service.py b/lambdas/services/ses_feedback_monitor_service.py index 6a924f473..31e1caedc 100644 --- a/lambdas/services/ses_feedback_monitor_service.py +++ b/lambdas/services/ses_feedback_monitor_service.py @@ -1,19 +1,15 @@ import json from dataclasses import dataclass from datetime import datetime, timezone -from typing import Any, Dict, List, Optional, Protocol, Tuple +from typing import Any, Dict, List, Optional, Tuple +from services.base.s3_service import S3Service from services.email_service import EmailService from utils.audit_logging_setup import LoggingService logger = LoggingService(__name__) -class S3Client(Protocol): - def put_object(self, **kwargs): # pragma: no cover (protocol) - ... - - @dataclass(frozen=True) class SesFeedbackMonitorConfig: feedback_bucket: str @@ -27,11 +23,11 @@ class SesFeedbackMonitorService: def __init__( self, *, - s3_client: S3Client, + s3_service: S3Service, email_service: EmailService, config: SesFeedbackMonitorConfig, ): - self.s3 = s3_client + self.s3 = s3_service self.email_service = email_service self.config = config @@ -112,15 +108,15 @@ def process_ses_feedback_event(self, event: Dict[str, Any]) -> Dict[str, Any]: s3_key = self.build_s3_key(self.config.feedback_prefix, et, mid) - self.s3.put_object( - Bucket=self.config.feedback_bucket, - Key=s3_key, - Body=json.dumps(payload).encode("utf-8"), - ContentType="application/json", + self.s3.put_json( + self.config.feedback_bucket, + s3_key, + payload, ) stored += 1 logger.info( - f"Stored SES feedback event: type={et}, message_id={mid}, s3=s3://{self.config.feedback_bucket}/{s3_key}" + f"Stored SES feedback event: type={et}, message_id={mid}, " + f"s3=s3://{self.config.feedback_bucket}/{s3_key}" ) if et in self.config.alert_on_event_types: diff --git a/lambdas/tests/unit/handlers/test_ses_feedback_monitor_handler.py b/lambdas/tests/unit/handlers/test_ses_feedback_monitor_handler.py index b0d319ceb..6251d0258 100644 --- a/lambdas/tests/unit/handlers/test_ses_feedback_monitor_handler.py +++ b/lambdas/tests/unit/handlers/test_ses_feedback_monitor_handler.py @@ -3,7 +3,7 @@ import pytest -MODULE_UNDER_TEST = "handlers.report_distribution_handler" +MODULE_UNDER_TEST = "handlers.ses_feedback_monitor_handler" @pytest.fixture @@ -16,45 +16,42 @@ def required_env(mocker): mocker.patch.dict( os.environ, { - "REPORT_BUCKET_NAME": "my-report-bucket", - "CONTACT_TABLE_NAME": "contact-table", + "SES_FEEDBACK_BUCKET_NAME": "my-feedback-bucket", + "SES_FEEDBACK_PREFIX": "ses-feedback/", "PRM_MAILBOX_EMAIL": "prm@example.com", "SES_FROM_ADDRESS": "from@example.com", - "SES_CONFIGURATION_SET": "my-config-set", + "ALERT_ON_EVENT_TYPES": "BOUNCE, REJECT", }, clear=False, ) -def test_lambda_handler_wires_dependencies_and_returns_result_list_mode( +def test_lambda_handler_wires_dependencies_and_returns_service_result( mocker, handler_module, required_env ): - event = {"action": "list", "prefix": "reports/2026-01-01/"} + event = {"Records": []} context = mocker.Mock() - context.aws_request_id = "req-123" # avoid JSON serialization issues in decorators + context.aws_request_id = "req-123" s3_instance = mocker.Mock(name="S3ServiceInstance") - contact_repo_instance = mocker.Mock(name="ReportContactRepositoryInstance") email_instance = mocker.Mock(name="EmailServiceInstance") - svc_instance = mocker.Mock(name="ReportDistributionServiceInstance") - svc_instance.list_xlsx_keys.return_value = ["a.xlsx", "b.xlsx"] + svc_instance = mocker.Mock(name="SesFeedbackMonitorServiceInstance") + svc_instance.process_ses_feedback_event.return_value = { + "status": "ok", + "stored": 1, + "alerted": 0, + } mocked_s3_cls = mocker.patch.object( handler_module, "S3Service", autospec=True, return_value=s3_instance ) - mocked_contact_repo_cls = mocker.patch.object( - handler_module, - "ReportContactRepository", - autospec=True, - return_value=contact_repo_instance, - ) mocked_email_cls = mocker.patch.object( handler_module, "EmailService", autospec=True, return_value=email_instance ) - mocked_dist_svc_cls = mocker.patch.object( + mocked_svc_cls = mocker.patch.object( handler_module, - "ReportDistributionService", + "SesFeedbackMonitorService", autospec=True, return_value=svc_instance, ) @@ -62,103 +59,35 @@ def test_lambda_handler_wires_dependencies_and_returns_result_list_mode( result = handler_module.lambda_handler(event, context) mocked_s3_cls.assert_called_once_with() - mocked_contact_repo_cls.assert_called_once_with("contact-table") - mocked_email_cls.assert_called_once_with(default_configuration_set="my-config-set") - - mocked_dist_svc_cls.assert_called_once_with( - s3_service=s3_instance, - contact_repo=contact_repo_instance, - email_service=email_instance, - bucket="my-report-bucket", - from_address="from@example.com", - prm_mailbox="prm@example.com", - ) - - svc_instance.list_xlsx_keys.assert_called_once_with(prefix="reports/2026-01-01/") - assert result == { - "bucket": "my-report-bucket", - "prefix": "reports/2026-01-01/", - "keys": ["a.xlsx", "b.xlsx"], - } - - -def test_lambda_handler_uses_bucket_from_event_when_provided_list_mode( - mocker, handler_module, required_env -): - event = {"action": "list", "prefix": "p/", "bucket": "override-bucket"} - context = mocker.Mock() - context.aws_request_id = "req-456" - - svc_instance = mocker.Mock() - svc_instance.list_xlsx_keys.return_value = [] - - mocker.patch.object( - handler_module, - "ReportDistributionService", - autospec=True, - return_value=svc_instance, - ) - mocker.patch.object( - handler_module, "S3Service", autospec=True, return_value=mocker.Mock() - ) - mocker.patch.object( - handler_module, - "ReportContactRepository", - autospec=True, - return_value=mocker.Mock(), - ) - mocker.patch.object( - handler_module, "EmailService", autospec=True, return_value=mocker.Mock() - ) - - result = handler_module.lambda_handler(event, context) - - svc_instance.list_xlsx_keys.assert_called_once_with(prefix="p/") - assert result == {"bucket": "override-bucket", "prefix": "p/", "keys": []} - - -def test_lambda_handler_process_one_mode_happy_path( - mocker, handler_module, required_env -): - event = {"action": "process_one", "key": "reports/ABC/whatever.xlsx"} - context = mocker.Mock() - context.aws_request_id = "req-789" - - svc_instance = mocker.Mock() - svc_instance.extract_ods_code_from_key.return_value = "ABC" - svc_instance.process_one_report.return_value = None - - mocker.patch.object( - handler_module, - "ReportDistributionService", - autospec=True, - return_value=svc_instance, - ) - mocker.patch.object( - handler_module, "S3Service", autospec=True, return_value=mocker.Mock() - ) - mocker.patch.object( - handler_module, - "ReportContactRepository", - autospec=True, - return_value=mocker.Mock(), - ) - mocker.patch.object( - handler_module, "EmailService", autospec=True, return_value=mocker.Mock() - ) - - result = handler_module.lambda_handler(event, context) - - svc_instance.extract_ods_code_from_key.assert_called_once_with( - "reports/ABC/whatever.xlsx" - ) - svc_instance.process_one_report.assert_called_once_with( - ods_code="ABC", key="reports/ABC/whatever.xlsx" - ) - - assert result == { - "status": "ok", - "bucket": "my-report-bucket", - "key": "reports/ABC/whatever.xlsx", - "ods_code": "ABC", - } + mocked_email_cls.assert_called_once_with() + + mocked_svc_cls.assert_called_once() + _, kwargs = mocked_svc_cls.call_args + + assert kwargs["s3_service"] is s3_instance + assert kwargs["email_service"] is email_instance + + cfg = kwargs["config"] + assert cfg.feedback_bucket == "my-feedback-bucket" + assert cfg.feedback_prefix == "ses-feedback/" + assert cfg.prm_mailbox == "prm@example.com" + assert cfg.from_address == "from@example.com" + assert cfg.alert_on_event_types == {"BOUNCE", "REJECT"} + + svc_instance.process_ses_feedback_event.assert_called_once_with(event) + assert result == {"status": "ok", "stored": 1, "alerted": 0} + + +@pytest.mark.parametrize( + "configured, expected", + [ + ("BOUNCE,REJECT", {"BOUNCE", "REJECT"}), + (" bounce , reject ", {"BOUNCE", "REJECT"}), + ("BOUNCE,, ,REJECT,", {"BOUNCE", "REJECT"}), + ("", set()), + (" ", set()), + ("complaint", {"COMPLAINT"}), + ], +) +def test_parse_alert_types(handler_module, configured, expected): + assert handler_module.parse_alert_types(configured) == expected diff --git a/lambdas/tests/unit/services/base/test_s3_service.py b/lambdas/tests/unit/services/base/test_s3_service.py index b113024b3..e325ee496 100755 --- a/lambdas/tests/unit/services/base/test_s3_service.py +++ b/lambdas/tests/unit/services/base/test_s3_service.py @@ -1,4 +1,5 @@ import datetime +import json from io import BytesIO import pytest @@ -21,12 +22,6 @@ from utils.exceptions import TagNotFoundException TEST_DOWNLOAD_PATH = "test_path" -MOCK_EVENT_BODY = { - "resourceType": "DocumentReference", - "subject": {"identifier": {"value": 111111000}}, - "content": [{"attachment": {"contentType": "application/pdf"}}], - "description": "test_filename.pdf", -} def flatten(list_of_lists): @@ -37,12 +32,16 @@ def flatten(list_of_lists): @freeze_time("2023-10-30T10:25:00") @pytest.fixture def mock_service(mocker, set_env): - mocker.patch("boto3.client") + S3Service._instance = None + + mock_boto_client = mocker.patch("boto3.client") mocker.patch("services.base.iam_service.IAMService") + service = S3Service(custom_aws_role="mock_arn_custom_role") service.expiration_time = datetime.datetime.now( datetime.timezone.utc ) + datetime.timedelta(hours=1) + yield service S3Service._instance = None @@ -65,6 +64,43 @@ def mock_list_objects_paginate(mock_client): return mock_paginator_method +def test_s3_service_constructs_boto_client_with_timeouts(mocker): + S3Service._instance = None + mocked_boto_client = mocker.patch("boto3.client") + + _ = S3Service() + + mocked_boto_client.assert_called_once() + _, kwargs = mocked_boto_client.call_args + assert kwargs["config"].connect_timeout == 3 + assert kwargs["config"].read_timeout == 5 + + S3Service._instance = None + + +def test_put_json_calls_put_object_with_encoded_json(mock_service, mock_client): + payload = {"a": 1, "b": {"c": 2}} + + mock_service.put_json("bucket", "key.json", payload) + + mock_client.put_object.assert_called_once() + _, kwargs = mock_client.put_object.call_args + + assert kwargs["Bucket"] == "bucket" + assert kwargs["Key"] == "key.json" + assert kwargs["ContentType"] == "application/json" + assert json.loads(kwargs["Body"].decode("utf-8")) == payload + + +def test_put_json_allows_custom_content_type(mock_service, mock_client): + payload = {"hello": "world"} + + mock_service.put_json("bucket", "key", payload, content_type="application/x-ndjson") + + _, kwargs = mock_client.put_object.call_args + assert kwargs["ContentType"] == "application/x-ndjson" + + def test_create_upload_presigned_url(mock_service, mocker, mock_custom_client): mock_custom_client.generate_presigned_post.return_value = ( MOCK_PRESIGNED_URL_RESPONSE @@ -89,6 +125,7 @@ def test_create_download_presigned_url(mock_service, mocker, mock_custom_client) mock_custom_client, mock_service.expiration_time, ) + response = mock_service.create_download_presigned_url(MOCK_BUCKET, TEST_FILE_KEY) assert response == MOCK_PRESIGNED_URL_RESPONSE @@ -160,7 +197,7 @@ def test_copy_across_bucket_if_none_match(mock_service, mock_client): def test_delete_object(mock_service, mock_client): mock_service.delete_object(s3_bucket_name=MOCK_BUCKET, file_key=TEST_FILE_NAME) - mock_client.delete_object_assert_called_once_with( + mock_client.delete_object.assert_called_once_with( Bucket=MOCK_BUCKET, Key=TEST_FILE_NAME ) @@ -200,8 +237,7 @@ def test_get_tag_value(mock_service, mock_client): actual = mock_service.get_tag_value( s3_bucket_name=MOCK_BUCKET, file_key=TEST_FILE_NAME, tag_key=test_tag_key ) - expected = test_tag_value - assert actual == expected + assert actual == test_tag_value mock_client.get_object_tagging.assert_called_once_with( Bucket=MOCK_BUCKET, @@ -245,11 +281,12 @@ def test_file_exist_on_s3_return_true_if_object_exists(mock_service, mock_client mock_client.head_object.return_value = mock_response - expected = True - actual = mock_service.file_exist_on_s3( - s3_bucket_name=MOCK_BUCKET, file_key=TEST_FILE_NAME + assert ( + mock_service.file_exist_on_s3( + s3_bucket_name=MOCK_BUCKET, file_key=TEST_FILE_NAME + ) + is True ) - assert actual == expected mock_client.head_object.assert_called_once_with( Bucket=MOCK_BUCKET, @@ -267,13 +304,13 @@ def test_file_exist_on_s3_return_false_if_object_does_not_exist( mock_client.head_object.side_effect = mock_error - expected = False - actual = mock_service.file_exist_on_s3( - s3_bucket_name=MOCK_BUCKET, file_key=TEST_FILE_NAME + assert ( + mock_service.file_exist_on_s3( + s3_bucket_name=MOCK_BUCKET, file_key=TEST_FILE_NAME + ) + is False ) - assert actual == expected - mock_client.head_object.assert_called_with( Bucket=MOCK_BUCKET, Key=TEST_FILE_NAME, @@ -302,6 +339,7 @@ def test_file_exist_on_s3_raises_client_error_if_unexpected_response( def test_s3_service_singleton_instance(mocker): + S3Service._instance = None mocker.patch("boto3.client") instance_1 = S3Service() @@ -309,8 +347,11 @@ def test_s3_service_singleton_instance(mocker): assert instance_1 is instance_2 + S3Service._instance = None + def test_not_created_presigned_url_without_custom_client(mocker): + S3Service._instance = None mocker.patch("boto3.client") mock_service = S3Service() @@ -318,8 +359,11 @@ def test_not_created_presigned_url_without_custom_client(mocker): assert response is None + S3Service._instance = None + def test_not_created_custom_client_without_client_role(mocker): + S3Service._instance = None mocker.patch("boto3.client") iam_service = mocker.patch("services.base.iam_service.IAMService") @@ -328,6 +372,8 @@ def test_not_created_custom_client_without_client_role(mocker): iam_service.assert_not_called() assert mock_service.custom_client is None + S3Service._instance = None + @freeze_time("2023-10-30T10:25:00") def test_created_custom_client_when_client_role_is_passed(mocker): @@ -351,6 +397,8 @@ def test_created_custom_client_when_client_role_is_passed(mocker): assert mock_service.custom_client == custom_client_mock iam_service_instance.assume_role.assert_called() + S3Service._instance = None + def test_list_all_objects_return_a_list_of_file_details( mock_service, mock_client, mock_list_objects_paginate @@ -428,7 +476,7 @@ def test_save_or_create_file(mock_service, mock_client): mock_service.save_or_create_file(MOCK_BUCKET, TEST_FILE_NAME, body) mock_client.put_object.assert_called() - args, kwargs = mock_client.put_object.call_args + _, kwargs = mock_client.put_object.call_args assert kwargs["Bucket"] == MOCK_BUCKET assert kwargs["Key"] == TEST_FILE_NAME @@ -442,6 +490,13 @@ def test_returns_binary_file_content_when_file_exists( "Body": mocker.Mock(read=lambda: b"file-content") } + body = mock_service.get_object_stream(bucket=MOCK_BUCKET, key=TEST_FILE_KEY) + assert body.read() == b"file-content" + + mock_client.get_object.assert_called_once_with( + Bucket=MOCK_BUCKET, Key=TEST_FILE_KEY + ) + def test_raises_exception_when_file_does_not_exist(mock_service, mock_client): mock_client.get_object.side_effect = MOCK_CLIENT_ERROR @@ -570,12 +625,15 @@ def test_copy_across_bucket_retries_on_409_conflict(mock_service, mock_client): mock_client.copy_object.side_effect = [ ClientError( { - "Error": {"Code": "PreconditionFailed", "Message": "Precondition Failed"}, - "ResponseMetadata": {"HTTPStatusCode": 409} + "Error": { + "Code": "PreconditionFailed", + "Message": "Precondition Failed", + }, + "ResponseMetadata": {"HTTPStatusCode": 409}, }, - "CopyObject" + "CopyObject", ), - {"CopyObjectResult": {"ETag": "mock-etag"}} # Success on retry + {"CopyObjectResult": {"ETag": "mock-etag"}}, # Success on retry ] mock_service.copy_across_bucket( diff --git a/lambdas/tests/unit/services/test_ses_feedback_monitor_service.py b/lambdas/tests/unit/services/test_ses_feedback_monitor_service.py index 7cc164f06..0384afde2 100644 --- a/lambdas/tests/unit/services/test_ses_feedback_monitor_service.py +++ b/lambdas/tests/unit/services/test_ses_feedback_monitor_service.py @@ -20,7 +20,7 @@ def config(): @pytest.fixture -def s3_client(mocker): +def s3_service(mocker): return mocker.Mock() @@ -30,9 +30,9 @@ def email_service(mocker): @pytest.fixture -def svc(s3_client, email_service, config): +def svc(s3_service, email_service, config): return SesFeedbackMonitorService( - s3_client=s3_client, email_service=email_service, config=config + s3_service=s3_service, email_service=email_service, config=config ) @@ -145,7 +145,6 @@ def test_extract_recipients_and_diagnostic_unknown_payload(): def test_build_s3_key_uses_prefix_and_date_and_message_id(mocker): - fixed_now = datetime(2026, 1, 15, 12, 34, 56, tzinfo=timezone.utc) module = __import__(SesFeedbackMonitorService.__module__, fromlist=["datetime"]) @@ -162,7 +161,6 @@ def test_build_s3_key_uses_prefix_and_date_and_message_id(mocker): def test_build_s3_key_strips_trailing_slash(mocker): - fixed_now = datetime(2026, 1, 15, 0, 0, 0, tzinfo=timezone.utc) module = __import__(SesFeedbackMonitorService.__module__, fromlist=["datetime"]) @@ -215,7 +213,7 @@ def test_build_prm_email_includes_expected_fields(svc): def test_process_ses_feedback_event_stores_each_record_and_alerts_only_configured_types( - svc, s3_client, email_service, mocker + svc, s3_service, email_service, mocker ): mocker.patch.object( SesFeedbackMonitorService, @@ -247,12 +245,12 @@ def test_process_ses_feedback_event_stores_each_record_and_alerts_only_configure assert resp == {"status": "ok", "stored": 2, "alerted": 1} - assert s3_client.put_object.call_count == 2 - put1 = s3_client.put_object.call_args_list[0].kwargs - assert put1["Bucket"] == "feedback-bucket" - assert put1["Key"] == "ses/feedback/BOUNCE/2026/01/15/m.json" - assert put1["ContentType"] == "application/json" - assert json.loads(put1["Body"].decode("utf-8")) == bounce_payload + assert s3_service.put_json.call_count == 2 + + first_call = s3_service.put_json.call_args_list[0] + assert first_call.args[0] == "feedback-bucket" + assert first_call.args[1] == "ses/feedback/BOUNCE/2026/01/15/m.json" + assert first_call.args[2] == bounce_payload email_service.send_email.assert_called_once_with( to_address="prm@example.com", @@ -263,16 +261,16 @@ def test_process_ses_feedback_event_stores_each_record_and_alerts_only_configure def test_process_ses_feedback_event_handles_empty_records( - svc, s3_client, email_service + svc, s3_service, email_service ): resp = svc.process_ses_feedback_event({"Records": []}) assert resp == {"status": "ok", "stored": 0, "alerted": 0} - s3_client.put_object.assert_not_called() + s3_service.put_json.assert_not_called() email_service.send_email.assert_not_called() def test_process_ses_feedback_event_message_can_be_dict_not_json_string( - svc, s3_client, email_service, mocker + svc, s3_service, email_service, mocker ): mocker.patch.object( SesFeedbackMonitorService, "build_s3_key", return_value="k.json" @@ -289,5 +287,5 @@ def test_process_ses_feedback_event_message_can_be_dict_not_json_string( resp = svc.process_ses_feedback_event(event) assert resp == {"status": "ok", "stored": 1, "alerted": 1} - s3_client.put_object.assert_called_once() + s3_service.put_json.assert_called_once_with("feedback-bucket", "k.json", payload) email_service.send_email.assert_called_once() From 4c2efcfa478918db8eb9a94802b0da4eec3674f5 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Fri, 16 Jan 2026 09:05:24 +0000 Subject: [PATCH 6/8] [PRMP-1058] fixed test --- lambdas/tests/unit/services/base/test_s3_service.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lambdas/tests/unit/services/base/test_s3_service.py b/lambdas/tests/unit/services/base/test_s3_service.py index e325ee496..38fb8c3fc 100755 --- a/lambdas/tests/unit/services/base/test_s3_service.py +++ b/lambdas/tests/unit/services/base/test_s3_service.py @@ -34,7 +34,7 @@ def flatten(list_of_lists): def mock_service(mocker, set_env): S3Service._instance = None - mock_boto_client = mocker.patch("boto3.client") + mocker.patch("boto3.client") mocker.patch("services.base.iam_service.IAMService") service = S3Service(custom_aws_role="mock_arn_custom_role") @@ -483,9 +483,7 @@ def test_save_or_create_file(mock_service, mock_client): assert kwargs["Body"].read() == body -def test_returns_binary_file_content_when_file_exists( - mock_service, mock_client, mocker -): +def test_returns_binary_file_content_when_file_exists(mock_service, mock_client, mocker): mock_client.get_object.return_value = { "Body": mocker.Mock(read=lambda: b"file-content") } From 3370f69295a2f8c232207e302cbadf39f217d510 Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Fri, 16 Jan 2026 11:52:25 +0000 Subject: [PATCH 7/8] [PRMP-1058] minor updates --- lambdas/handlers/ses_feedback_monitor_handler.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lambdas/handlers/ses_feedback_monitor_handler.py b/lambdas/handlers/ses_feedback_monitor_handler.py index 0e254688d..f74b31bc4 100644 --- a/lambdas/handlers/ses_feedback_monitor_handler.py +++ b/lambdas/handlers/ses_feedback_monitor_handler.py @@ -7,12 +7,17 @@ SesFeedbackMonitorConfig, SesFeedbackMonitorService, ) +from utils.decorators.handle_lambda_exceptions import handle_lambda_exceptions +from utils.decorators.override_error_check import override_error_check +from utils.decorators.set_audit_arg import set_request_context_for_logging def parse_alert_types(configured: str) -> set[str]: return {s.strip().upper() for s in configured.split(",") if s.strip()} - +@override_error_check +@handle_lambda_exceptions +@set_request_context_for_logging def lambda_handler(event, context) -> Dict[str, Any]: config = SesFeedbackMonitorConfig( feedback_bucket=os.environ["SES_FEEDBACK_BUCKET_NAME"], From f7d63df04ad43ee740dc5bb872f31e101b8c3f4d Mon Sep 17 00:00:00 2001 From: PedroSoaresNHS Date: Fri, 16 Jan 2026 14:42:18 +0000 Subject: [PATCH 8/8] [PRMP-1058] minor updates --- lambdas/services/ses_feedback_monitor_service.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lambdas/services/ses_feedback_monitor_service.py b/lambdas/services/ses_feedback_monitor_service.py index 31e1caedc..e20ba041a 100644 --- a/lambdas/services/ses_feedback_monitor_service.py +++ b/lambdas/services/ses_feedback_monitor_service.py @@ -103,10 +103,10 @@ def process_ses_feedback_event(self, event: Dict[str, Any]) -> Dict[str, Any]: for record in event.get("Records") or []: payload = self.parse_sns_message(record) - et = self.event_type(payload) - mid = self.message_id(payload) + event_type = self.event_type(payload) + message_id = self.message_id(payload) - s3_key = self.build_s3_key(self.config.feedback_prefix, et, mid) + s3_key = self.build_s3_key(self.config.feedback_prefix, event_type, message_id) self.s3.put_json( self.config.feedback_bucket, @@ -115,11 +115,11 @@ def process_ses_feedback_event(self, event: Dict[str, Any]) -> Dict[str, Any]: ) stored += 1 logger.info( - f"Stored SES feedback event: type={et}, message_id={mid}, " + f"Stored SES feedback event: type={event_type}, message_id={message_id}, " f"s3=s3://{self.config.feedback_bucket}/{s3_key}" ) - if et in self.config.alert_on_event_types: + if event_type in self.config.alert_on_event_types: subject, body = self.build_prm_email(payload, s3_key) self.email_service.send_email( to_address=self.config.prm_mailbox, @@ -129,7 +129,7 @@ def process_ses_feedback_event(self, event: Dict[str, Any]) -> Dict[str, Any]: ) alerted += 1 logger.info( - f"Emailed PRM for SES feedback event: type={et}, message_id={mid}" + f"Emailed PRM for SES feedback event: type={event_type}, message_id={message_id}" ) return {"status": "ok", "stored": stored, "alerted": alerted}