feat(email): set Reply-To headers on confirmation and notification emails
- notify_parent: adds Reply-To set to the first CC (admin) address so parent replies to the completion confirmation reach the admin rather than re-entering the agent pipeline - notify_admin / notify_registration_update: Reply-To = parent email was already wired through _send(); confirmed with new tests - Mid-registration send_reply emails carry no Reply-To (verified by test) - 5 new tests across TestNotifyParentReplyTo and TestNotifyAdminReplyTo; 1 new test in TestSendReplyNoAdminReplyTo (174 passing, 0 failing) https://claude.ai/code/session_014NynTjALA5TeP85mbr5ZA8
This commit is contained in:
@@ -1,19 +1,19 @@
|
|||||||
## 1. Confirmation Email Reply-To (Email Channel)
|
## 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)
|
- [x] 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)
|
- [x] 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
|
- [x] 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.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. Notification Email Reply-To (Registration Notifications)
|
||||||
|
|
||||||
- [ ] 2.1 Identify where registration notification emails are constructed and sent
|
- [x] 2.1 Identify where registration notification emails are constructed and sent
|
||||||
- [ ] 2.2 Set `Reply-To: <parent email>` header on all outgoing notification emails (indoor, outdoor, and both routing types)
|
- [x] 2.2 Set `Reply-To: <parent email>` 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
|
- [x] 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
|
- [x] 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.5 Add a unit test verifying the `Reply-To` header equals the parent's email when both leaders are notified
|
||||||
|
|
||||||
## 3. Verification
|
## 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
|
- [ ] 3.2 Manually send a test registration through the email channel and verify reply routing behaves correctly
|
||||||
|
|||||||
@@ -215,6 +215,8 @@ class AdminNotifier:
|
|||||||
msg_outer["From"] = self._from_email
|
msg_outer["From"] = self._from_email
|
||||||
msg_outer["To"] = parent_email
|
msg_outer["To"] = parent_email
|
||||||
msg_outer["Subject"] = subject
|
msg_outer["Subject"] = subject
|
||||||
|
if self._cc_emails:
|
||||||
|
msg_outer["Reply-To"] = self._cc_emails[0]
|
||||||
|
|
||||||
msg_alt = MIMEMultipart("alternative")
|
msg_alt = MIMEMultipart("alternative")
|
||||||
msg_alt.attach(MIMEText(text_body, "plain", "utf-8"))
|
msg_alt.attach(MIMEText(text_body, "plain", "utf-8"))
|
||||||
|
|||||||
@@ -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
|
- detect_automated_message() — header-based bounce/automated sender detection
|
||||||
- EmailAgent.handle_automated_message() — state tracking, one-shot admin alert
|
- EmailAgent.handle_automated_message() — state tracking, one-shot admin alert
|
||||||
- EmailAgent.process_message() — hard message-count cap (MAX_USER_MESSAGES)
|
- EmailAgent.process_message() — hard message-count cap (MAX_USER_MESSAGES)
|
||||||
- AdminNotifier.notify_loop_escalation() — escalation email dispatch
|
- AdminNotifier.notify_loop_escalation() — escalation email dispatch
|
||||||
|
- EmailChannel.send_reply() — mid-registration emails must not carry admin Reply-To
|
||||||
"""
|
"""
|
||||||
|
|
||||||
import email
|
import email
|
||||||
@@ -13,7 +14,7 @@ import json
|
|||||||
import pytest
|
import pytest
|
||||||
from unittest.mock import MagicMock, patch
|
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.agent.core import EmailAgent, MAX_USER_MESSAGES
|
||||||
from src.models.conversation import ConversationState, ChatMessage
|
from src.models.conversation import ConversationState, ChatMessage
|
||||||
from src.notifications.notifier import AdminNotifier
|
from src.notifications.notifier import AdminNotifier
|
||||||
@@ -650,3 +651,42 @@ class TestNotifyLoopEscalation:
|
|||||||
)
|
)
|
||||||
|
|
||||||
mock_smtp_cls.assert_not_called()
|
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
|
||||||
|
|||||||
@@ -345,3 +345,143 @@ class TestGenerateQrBillPng:
|
|||||||
"""Output starts with the PNG magic bytes."""
|
"""Output starts with the PNG magic bytes."""
|
||||||
png = notifier._generate_qr_bill_png()
|
png = notifier._generate_qr_bill_png()
|
||||||
assert png[:4] == b"\x89PNG"
|
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"
|
||||||
|
|||||||
Reference in New Issue
Block a user