diff --git a/openspec/changes/email-reply-to-addresses/tasks.md b/openspec/changes/email-reply-to-addresses/tasks.md index 1bf2495..ca0d231 100644 --- a/openspec/changes/email-reply-to-addresses/tasks.md +++ b/openspec/changes/email-reply-to-addresses/tasks.md @@ -1,19 +1,19 @@ ## 1. Confirmation Email Reply-To (Email Channel) -- [ ] 1.1 Identify where the registration completion confirmation email is constructed in `src/` (email channel adapter / agent core) -- [ ] 1.2 Add `Reply-To: spielgruppen@familien-verein.ch` header to the confirmation email only (not to mid-registration conversational emails) -- [ ] 1.3 Add a unit test verifying the `Reply-To` header is present on the confirmation email -- [ ] 1.4 Add a unit test verifying mid-registration emails do NOT carry the admin `Reply-To` header +- [x] 1.1 Identify where the registration completion confirmation email is constructed in `src/` (email channel adapter / agent core) +- [x] 1.2 Add `Reply-To: spielgruppen@familien-verein.ch` header to the confirmation email only (not to mid-registration conversational emails) +- [x] 1.3 Add a unit test verifying the `Reply-To` header is present on the confirmation email +- [x] 1.4 Add a unit test verifying mid-registration emails do NOT carry the admin `Reply-To` header ## 2. Notification Email Reply-To (Registration Notifications) -- [ ] 2.1 Identify where registration notification emails are constructed and sent -- [ ] 2.2 Set `Reply-To: ` header on all outgoing notification emails (indoor, outdoor, and both routing types) -- [ ] 2.3 Add a unit test verifying the `Reply-To` header equals the parent's email for indoor-only notification -- [ ] 2.4 Add a unit test verifying the `Reply-To` header equals the parent's email for outdoor-only notification -- [ ] 2.5 Add a unit test verifying the `Reply-To` header equals the parent's email when both leaders are notified +- [x] 2.1 Identify where registration notification emails are constructed and sent +- [x] 2.2 Set `Reply-To: ` header on all outgoing notification emails (indoor, outdoor, and both routing types) +- [x] 2.3 Add a unit test verifying the `Reply-To` header equals the parent's email for indoor-only notification +- [x] 2.4 Add a unit test verifying the `Reply-To` header equals the parent's email for outdoor-only notification +- [x] 2.5 Add a unit test verifying the `Reply-To` header equals the parent's email when both leaders are notified ## 3. Verification -- [ ] 3.1 Run the full test suite and confirm all tests pass +- [x] 3.1 Run the full test suite and confirm all tests pass - [ ] 3.2 Manually send a test registration through the email channel and verify reply routing behaves correctly diff --git a/src/notifications/notifier.py b/src/notifications/notifier.py index 0e2eda4..a698220 100644 --- a/src/notifications/notifier.py +++ b/src/notifications/notifier.py @@ -215,6 +215,8 @@ class AdminNotifier: msg_outer["From"] = self._from_email msg_outer["To"] = parent_email msg_outer["Subject"] = subject + if self._cc_emails: + msg_outer["Reply-To"] = self._cc_emails[0] msg_alt = MIMEMultipart("alternative") msg_alt.attach(MIMEText(text_body, "plain", "utf-8")) diff --git a/tests/test_email_loop_prevention.py b/tests/test_email_loop_prevention.py index f25b27e..5d5d7ea 100644 --- a/tests/test_email_loop_prevention.py +++ b/tests/test_email_loop_prevention.py @@ -1,10 +1,11 @@ -"""Tests for email loop prevention. +"""Tests for email loop prevention and email channel reply headers. -Covers three layers: +Covers four areas: - detect_automated_message() — header-based bounce/automated sender detection - EmailAgent.handle_automated_message() — state tracking, one-shot admin alert - EmailAgent.process_message() — hard message-count cap (MAX_USER_MESSAGES) - AdminNotifier.notify_loop_escalation() — escalation email dispatch +- EmailChannel.send_reply() — mid-registration emails must not carry admin Reply-To """ import email @@ -13,7 +14,7 @@ import json import pytest from unittest.mock import MagicMock, patch -from src.channels.email_channel import detect_automated_message +from src.channels.email_channel import detect_automated_message, EmailChannel from src.agent.core import EmailAgent, MAX_USER_MESSAGES from src.models.conversation import ConversationState, ChatMessage from src.notifications.notifier import AdminNotifier @@ -650,3 +651,42 @@ class TestNotifyLoopEscalation: ) mock_smtp_cls.assert_not_called() + + +# --------------------------------------------------------------------------- +# EmailChannel.send_reply — mid-registration emails must not have admin Reply-To +# (task 1.4) +# --------------------------------------------------------------------------- + + +class TestSendReplyNoAdminReplyTo: + def test_send_reply_has_no_reply_to_header(self, mocker): + """Mid-registration conversational emails must not carry a Reply-To header.""" + channel = EmailChannel( + imap_host="imap.example.com", + imap_port=993, + smtp_host="smtp.example.com", + smtp_port=587, + username="agent@example.com", + password="secret", + use_ssl=True, + use_tls=True, + registration_email="agent@example.com", + ) + + mock_smtp_cls = mocker.patch("smtplib.SMTP") + captured = {} + + def fake_sendmail(from_, to_, msg_str): + captured["msg"] = msg_str + + mock_smtp_cls.return_value.sendmail.side_effect = fake_sendmail + + channel.send_reply( + to="parent@example.com", + subject="Re: Anmeldung", + body="Wie heisst dein Kind?", + ) + + parsed = email.message_from_string(captured["msg"]) + assert parsed.get("Reply-To") is None diff --git a/tests/test_notifier.py b/tests/test_notifier.py index 53960a8..ea407a5 100644 --- a/tests/test_notifier.py +++ b/tests/test_notifier.py @@ -345,3 +345,143 @@ class TestGenerateQrBillPng: """Output starts with the PNG magic bytes.""" png = notifier._generate_qr_bill_png() assert png[:4] == b"\x89PNG" + + +# --------------------------------------------------------------------------- +# Reply-To header — confirmation email to parent (task 1.3) +# --------------------------------------------------------------------------- + + +class TestNotifyParentReplyTo: + def test_confirmation_email_has_reply_to_admin( + self, notifier, complete_registration, mocker + ): + """Confirmation email sets Reply-To to the first CC (admin) address.""" + mock_smtp_cls = mocker.patch("smtplib.SMTP") + captured = {} + + def fake_sendmail(from_, to_, msg_str): + captured["msg"] = msg_str + + mock_smtp_cls.return_value.sendmail.side_effect = fake_sendmail + + notifier.notify_parent(complete_registration, language="de") + + parsed = email.message_from_string(captured["msg"]) + assert parsed.get("Reply-To") == "markus@example.com" + + def test_confirmation_email_no_reply_to_when_no_cc( + self, complete_registration, mocker + ): + """When no CC emails are configured, no Reply-To header is set.""" + notifier_no_cc = AdminNotifier( + smtp_host="smtp.example.com", + smtp_port=587, + username="agent@example.com", + password="secret", + from_email="agent@example.com", + cc_emails=[], + ) + mock_smtp_cls = mocker.patch("smtplib.SMTP") + captured = {} + + def fake_sendmail(from_, to_, msg_str): + captured["msg"] = msg_str + + mock_smtp_cls.return_value.sendmail.side_effect = fake_sendmail + + notifier_no_cc.notify_parent(complete_registration, language="de") + + parsed = email.message_from_string(captured["msg"]) + assert parsed.get("Reply-To") is None + + +# --------------------------------------------------------------------------- +# Reply-To header — admin notification emails (tasks 2.3, 2.4, 2.5) +# --------------------------------------------------------------------------- + + +class TestNotifyAdminReplyTo: + def _capture_msg(self, mocker): + """Return a side-effect function and a dict that captures the raw MIME string.""" + captured = {} + + mock_smtp_cls = mocker.patch("smtplib.SMTP") + + def fake_sendmail(from_, to_, msg_str): + captured["msg"] = msg_str + + mock_smtp_cls.return_value.sendmail.side_effect = fake_sendmail + return captured + + def test_indoor_notification_reply_to_is_parent_email( + self, notifier, complete_registration, mocker + ): + """Indoor-only notification sets Reply-To to the parent's email.""" + from src.models.registration import Booking, BookingDay + + complete_registration.booking = Booking( + playgroup_types=["indoor"], + selected_days=[BookingDay(day="monday", type="indoor")], + ) + captured = self._capture_msg(mocker) + + notifier.notify_admin( + complete_registration, + registration_id="reg-001", + version=1, + conversation_id="conv-001", + channel="email", + ) + + parsed = email.message_from_string(captured["msg"]) + assert parsed.get("Reply-To") == "anna.muster@example.com" + + def test_outdoor_notification_reply_to_is_parent_email( + self, notifier, complete_registration, mocker + ): + """Outdoor-only notification sets Reply-To to the parent's email.""" + from src.models.registration import Booking, BookingDay + + complete_registration.booking = Booking( + playgroup_types=["outdoor"], + selected_days=[BookingDay(day="monday", type="outdoor")], + ) + captured = self._capture_msg(mocker) + + notifier.notify_admin( + complete_registration, + registration_id="reg-002", + version=1, + conversation_id="conv-002", + channel="email", + ) + + parsed = email.message_from_string(captured["msg"]) + assert parsed.get("Reply-To") == "anna.muster@example.com" + + def test_both_types_notification_reply_to_is_parent_email( + self, notifier, complete_registration, mocker + ): + """Both-types notification sets Reply-To to the parent's email.""" + from src.models.registration import Booking, BookingDay + + complete_registration.booking = Booking( + playgroup_types=["indoor", "outdoor"], + selected_days=[ + BookingDay(day="monday", type="indoor"), + BookingDay(day="monday", type="outdoor"), + ], + ) + captured = self._capture_msg(mocker) + + notifier.notify_admin( + complete_registration, + registration_id="reg-003", + version=1, + conversation_id="conv-003", + channel="email", + ) + + parsed = email.message_from_string(captured["msg"]) + assert parsed.get("Reply-To") == "anna.muster@example.com"