From 5fc6d7aec9e9801a423d1dda7981fda664104925 Mon Sep 17 00:00:00 2001 From: Marcel Peterkau Date: Thu, 20 Aug 2026 12:22:37 +0200 Subject: [PATCH] Stop rolling back reminders/SEPA mails after a successful send The previous fix reverted the reminder to "draft" on any failure after mark_reminder_sent, including failures that happened after the mail had already been handed to send_via_smtp or appended to an IMAP folder. That made a successful send followed by a Sent-folder-copy or archive-move failure look like nothing was sent, inviting a duplicate send/fee booking on retry -- the same "sent" flag needs to be preserved once delivery is no longer reversible, per follow-up review. reminder_mail.generate_and_send_reminder_mail and sepa_mail.generate_debit_mails now track whether the mail actually left the building (SMTP accepted / IMAP append succeeded / local file written) separately from the later archiving step: - Failure before that point: reminder_mail reverts to draft (unchanged); sepa_mail now records a warning and continues with the remaining debits instead of aborting the whole batch. - Failure after that point (Sent-copy append, moving the archive file into place): the reminder stays "sent" / the debit stays in the batch's results, an event is still logged for traceability (with an archive_error note and no document reference), and the caller gets a clear error to follow up on manually -- no rollback, no silent loss of the fact that the mail already went out. --- src/ccma/services/reminder_mail.py | 129 +++++++++++-------- src/ccma/services/sepa_mail.py | 79 +++++++----- tests/test_reminder_mail.py | 69 +++++++++++ tests/test_sepa.py | 191 +++++++++++++++++++++++++++++ 4 files changed, 386 insertions(+), 82 deletions(-) diff --git a/src/ccma/services/reminder_mail.py b/src/ccma/services/reminder_mail.py index 00b6ec3..81d6ce9 100644 --- a/src/ccma/services/reminder_mail.py +++ b/src/ccma/services/reminder_mail.py @@ -166,69 +166,90 @@ def generate_and_send_reminder_mail( sender_email=sender_email, signature=signature, ) - archive_dir = repository.members_root / member_id / "files" / "documents" / "Mahnungen" - archive_dir.mkdir(parents=True, exist_ok=True) - filename = ( - f"{datetime.now().date().isoformat()}-" - f"{_safe_filename(str(sent_reminder.get('name', 'Mahnung')))}.eml" - ) - archive_path = _available_path(archive_dir, filename) - archive_temp = archive_path.with_name(f".{archive_path.name}.tmp") - export_path: Path | None = None - try: - archive_temp.write_bytes(content) - if delivery_mode == "local": - export_temp = target.with_name(f".{target.name}.tmp") - try: - export_temp.write_bytes(content) - os.replace(export_temp, target) - finally: - export_temp.unlink(missing_ok=True) - export_path = target - elif delivery_mode == "send": - settings = repository.get_email_settings() - sent_copy = imap_session(settings) if settings["imap_sent_enabled"] else nullcontext(None) - with smtp_session(settings) as smtp_client, sent_copy as imap_client: - send_via_smtp(smtp_client, content) - if imap_client is not None: - ensure_imap_folder(imap_client, settings["imap_sent_folder"]) - append_message( - imap_client, content, folder=settings["imap_sent_folder"], flags=r"(\Seen)" - ) - else: - settings = repository.get_email_settings() - with imap_session(settings) as client: - ensure_imap_folder(client, settings["imap_drafts_folder"]) - append_message(client, content, folder=settings["imap_drafts_folder"], flags=r"(\Draft)") - os.replace(archive_temp, archive_path) - finally: - archive_temp.unlink(missing_ok=True) except Exception: - # The mail was not (verifiably) delivered/filed -- roll the reminder back to - # "draft" so it does not sit in the "sent" state without ever having gone out, - # and so it can be corrected and sent again. repository.revert_reminder_sent(member_id, claim_id, reminder_id) raise - relative_path = archive_path.relative_to( - repository.members_root / member_id / "files" - ).as_posix() + archive_dir = repository.members_root / member_id / "files" / "documents" / "Mahnungen" + archive_dir.mkdir(parents=True, exist_ok=True) + filename = ( + f"{datetime.now().date().isoformat()}-" + f"{_safe_filename(str(sent_reminder.get('name', 'Mahnung')))}.eml" + ) + archive_path = _available_path(archive_dir, filename) + archive_temp = archive_path.with_name(f".{archive_path.name}.tmp") + export_path: Path | None = None + # Once the mail has actually left the building -- landed on the SMTP server, been + # filed into an IMAP folder, or been written to the local export file -- it must + # not be un-sent again: a later failure (Sent-folder copy, moving the archive file + # into place) can no longer roll the reminder back to "draft", or a retry could + # send/file the same Mahnung a second time and double-book its fee. + delivered = False + archive_failure: Exception | None = None + try: + archive_temp.write_bytes(content) + if delivery_mode == "local": + export_temp = target.with_name(f".{target.name}.tmp") + try: + export_temp.write_bytes(content) + os.replace(export_temp, target) + finally: + export_temp.unlink(missing_ok=True) + export_path = target + delivered = True + elif delivery_mode == "send": + settings = repository.get_email_settings() + sent_copy = imap_session(settings) if settings["imap_sent_enabled"] else nullcontext(None) + with smtp_session(settings) as smtp_client, sent_copy as imap_client: + send_via_smtp(smtp_client, content) + delivered = True + if imap_client is not None: + ensure_imap_folder(imap_client, settings["imap_sent_folder"]) + append_message( + imap_client, content, folder=settings["imap_sent_folder"], flags=r"(\Seen)" + ) + else: + settings = repository.get_email_settings() + with imap_session(settings) as client: + ensure_imap_folder(client, settings["imap_drafts_folder"]) + append_message(client, content, folder=settings["imap_drafts_folder"], flags=r"(\Draft)") + delivered = True + os.replace(archive_temp, archive_path) + except Exception as exc: + archive_temp.unlink(missing_ok=True) + if not delivered: + repository.revert_reminder_sent(member_id, claim_id, reminder_id) + raise + archive_failure = exc + digest = hashlib.sha256(content).hexdigest() + references = {"claim_id": claim_id, "reminder_id": reminder_id} + if archive_failure is None: + references["document"] = archive_path.relative_to( + repository.members_root / member_id / "files" + ).as_posix() + data = { + "recipient": member.email, + "sha256": digest, + "payment_deadline": str(sent_reminder.get("payment_deadline", "")), + "delivery_mode": delivery_mode, + } + if archive_failure is not None: + data["archive_error"] = str(archive_failure) + # Log the event even when archiving failed -- the mail was already delivered/filed, + # so this must stay a traceable "sent" record instead of silently vanishing. repository.append_event( member_id, event_type="reminder_email_sent", summary=f"Mahnung per E-Mail versandt: {sent_reminder.get('name', '')}", actor_type="user", actor_name="Vorstand", - references={ - "claim_id": claim_id, - "reminder_id": reminder_id, - "document": relative_path, - }, - data={ - "recipient": member.email, - "sha256": digest, - "payment_deadline": str(sent_reminder.get("payment_deadline", "")), - "delivery_mode": delivery_mode, - }, + references=references, + data=data, ) + if archive_failure is not None: + raise RepositoryError( + "Die Mahnung wurde zugestellt bzw. abgelegt, konnte aber nicht vollständig " + f"archiviert werden: {archive_failure}. Die Mahnung bleibt als versandt gebucht " + "-- bitte die Mitgliedsakte manuell prüfen." + ) from archive_failure return GeneratedReminderMail(export_path, archive_path, digest) diff --git a/src/ccma/services/sepa_mail.py b/src/ccma/services/sepa_mail.py index 728da29..c7a8c00 100644 --- a/src/ccma/services/sepa_mail.py +++ b/src/ccma/services/sepa_mail.py @@ -166,43 +166,66 @@ def generate_debit_mails( archive_dir.mkdir(parents=True, exist_ok=True) archive_path = _available_path(archive_dir, filename) export_path: Path | None = None - if delivery_mode == "local": - export_path = _available_path(output, filename) - export_path.write_bytes(content) - elif delivery_mode == "send": - send_via_smtp(smtp_client, content) - if imap_client is not None: - append_message( - imap_client, content, folder=email_settings["imap_sent_folder"], flags=r"(\Seen)" - ) - else: - append_message( - imap_client, content, folder=email_settings["imap_drafts_folder"], flags=r"(\Draft)" - ) + # Once the mail has actually left the building for this debit -- SMTP + # accepted it, it's filed in the IMAP folder, or the local export file was + # written -- a later archiving failure must not abort the whole batch and + # lose track of the fact that this one was already delivered; nor may it + # abort earlier/later debits that have nothing to do with this failure. + delivered = False + archive_failure: Exception | None = None try: + if delivery_mode == "local": + export_path = _available_path(output, filename) + export_path.write_bytes(content) + delivered = True + elif delivery_mode == "send": + send_via_smtp(smtp_client, content) + delivered = True + if imap_client is not None: + append_message( + imap_client, content, folder=email_settings["imap_sent_folder"], flags=r"(\Seen)" + ) + else: + append_message( + imap_client, content, folder=email_settings["imap_drafts_folder"], flags=r"(\Draft)" + ) + delivered = True archive_path.write_bytes(content) - except OSError: - if export_path is not None: - export_path.unlink(missing_ok=True) - raise - relative_path = archive_path.relative_to( - repository.members_root / member.member_id / "files" - ).as_posix() + except (OSError, RepositoryError) as exc: + if not delivered: + if export_path is not None: + export_path.unlink(missing_ok=True) + warnings.append(f"{member.member_number or member.display_name}: {exc}") + continue + warnings.append( + f"{member.member_number or member.display_name}: E-Mail wurde versandt/abgelegt, " + f"konnte aber nicht archiviert werden ({exc}); bitte manuell prüfen." + ) + archive_failure = exc + digest = hashlib.sha256(content).hexdigest() + references = {} + if archive_failure is None: + references["document"] = archive_path.relative_to( + repository.members_root / member.member_id / "files" + ).as_posix() + data = { + "sha256": digest, + "recipient": member.email, + "collection_date": collection_date.isoformat(), + "amount": f"{debit.amount:.2f}", + "delivery_mode": delivery_mode, + } + if archive_failure is not None: + data["archive_error"] = str(archive_failure) repository.append_event( member.member_id, event_type="sepa_notification_generated", summary=f"SEPA-Info-Mail erzeugt: {archive_path.name}", actor_type="user", actor_name="Vorstand", - references={"document": relative_path}, - data={ - "sha256": digest, - "recipient": member.email, - "collection_date": collection_date.isoformat(), - "amount": f"{debit.amount:.2f}", - "delivery_mode": delivery_mode, - }, + references=references, + data=data, ) generated.append( GeneratedDebitMail(member.member_id, member.email, export_path, archive_path) diff --git a/tests/test_reminder_mail.py b/tests/test_reminder_mail.py index 2a2fb97..c1beb8f 100644 --- a/tests/test_reminder_mail.py +++ b/tests/test_reminder_mail.py @@ -301,3 +301,72 @@ def test_reminder_mail_local_write_failure_reverts_sent_status(tmp_path, monkeyp data = repository.get_contributions(member.member_id) assert data.reminders[0]["status"] == "draft" + + +def test_reminder_mail_archiving_failure_after_smtp_success_does_not_revert(tmp_path, monkeypatch): + import ccma.services.reminder_mail as reminder_mail_module + + repository, member = _overdue_claim_repository(tmp_path / "store") + member.email = "reminder@example.org" + repository.save_member(member) + repository.save_email_settings( + delivery_mode="send", + smtp_host="mail.example.org", + smtp_port=587, + smtp_security="none", + smtp_username="", + smtp_password="", + imap_host="", + imap_port=993, + imap_security="ssl", + imap_username="", + imap_password="", + imap_drafts_folder="", + ) + reminder = repository.create_reminder_draft( + member.member_id, + "claim-1", + level=1, + name="Zahlungserinnerung", + payment_deadline_days=14, + items=[{"description": "Mahngebühr", "amount": "5.00"}], + ) + + @contextmanager + def _fake_smtp_session(settings): + yield "smtp-client" + + monkeypatch.setattr(reminder_mail_module, "smtp_session", _fake_smtp_session) + monkeypatch.setattr(reminder_mail_module, "send_via_smtp", lambda client, content: None) + + real_replace = reminder_mail_module.os.replace + + def _flaky_replace(src, dst): + # Only the final archive move (into the "Mahnungen" folder) should fail -- + # the mail has already been handed to send_via_smtp by that point. + if str(dst).endswith(".eml") and "Mahnungen" in str(dst): + raise OSError("disk full") + return real_replace(src, dst) + + monkeypatch.setattr(reminder_mail_module.os, "replace", _flaky_replace) + + with pytest.raises(RepositoryError, match="zugestellt"): + generate_and_send_reminder_mail( + repository, + member.member_id, + "claim-1", + reminder["reminder_id"], + delivery_mode="send", + sender_name="Verwaltung C3MA", + sender_email="verwaltung@example.org", + signature="Der Vorstand", + ) + + data = repository.get_contributions(member.member_id) + stored_reminder = data.reminders[0] + assert stored_reminder["status"] == "sent", "mail was already sent -- must not be un-sent" + assert stored_reminder.get("fee_item_ids") + events = repository.get_events(member.member_id) + assert events[-1].event_type == "reminder_email_sent" + assert events[-1].data["archive_error"] + assert "document" not in events[-1].references diff --git a/tests/test_sepa.py b/tests/test_sepa.py index bded358..d057d4e 100644 --- a/tests/test_sepa.py +++ b/tests/test_sepa.py @@ -388,3 +388,194 @@ def test_generated_mail_send_mode_copies_to_sent_folder_reusing_one_imap_connect assert len(imap_connections) == 1, "one IMAP connection should be reused for the whole batch" assert ensured == ["INBOX.Gesendet"] assert appended == [("imap-client", "INBOX.Gesendet", r"(\Seen)")] * 2 + + +def test_generated_mail_archiving_failure_after_smtp_success_keeps_batch_going(tmp_path, monkeypatch): + import ccma.services.sepa_mail as sepa_mail_module + + repository, ada = _repository(tmp_path) + second = repository.create_member(first_name="Grace", last_name="Hopper", member_number="C3-43") + second.status = "active" + second.email = "grace@example.org" + second.account_holder = "Grace Hopper" + second.iban = "DE89370400440532013000" + second.bic = "COBADEFFXXX" + second.mandate_reference = "MANDAT-43" + second.mandate_signed_at = "2025-01-10" + second.mandate_active = True + repository.save_member(second) + repository.save_contributions( + second.member_id, + ContributionData( + claims=[ + { + "claim_id": "due", + "title": "Mitgliedsbeitrag 2026", + "amount": "150.00", + "due_date": "2026-01-31", + "status": "open", + } + ] + ), + ) + organization = repository.get_configuration()["organization"] + organization.update( + { + "name": "Chaos Computer Club Mannheim e.V.", + "email": "verwaltung@example.org", + "iban": "DE89370400440532013000", + "bic": "COBADEFFXXX", + "creditor_id": "DE98ZZZ09999999999", + } + ) + repository.save_organization(organization) + repository.save_email_settings( + delivery_mode="send", + smtp_host="mail.example.org", + smtp_port=587, + smtp_security="none", + smtp_username="", + smtp_password="", + imap_host="", + imap_port=993, + imap_security="ssl", + imap_username="", + imap_password="", + imap_drafts_folder="", + ) + debits, _warnings = pending_direct_debits(repository, due_until=date(2026, 12, 31)) + assert len(debits) == 2 + + monkeypatch.setattr( + sepa_mail_module, "smtp_session", contextmanager(lambda settings: iter(["smtp-client"])) + ) + sent = [] + monkeypatch.setattr(sepa_mail_module, "send_via_smtp", lambda client, content: sent.append(content)) + + from pathlib import Path + + original_write_bytes = Path.write_bytes + + def _flaky_write_bytes(self, data): + if "documents" in str(self) and "C3-43" in self.name: + raise OSError("disk full") + return original_write_bytes(self, data) + + monkeypatch.setattr(Path, "write_bytes", _flaky_write_bytes) + + generated, warnings = generate_debit_mails( + repository, + debits, + collection_date=date(2026, 8, 3), + delivery_mode="send", + sender_name="Verwaltung C3MA", + sender_email="verwaltung@example.org", + signature="Der Vorstand", + ) + + # Both mails were handed to SMTP -- a broken archive write for one of them must + # not swallow the other, nor pretend the failing one was never sent. + assert len(sent) == 2 + assert len(generated) == 2 + assert len(warnings) == 1 + assert "C3-43" in warnings[0] or "Grace" in warnings[0] + + failing = next(g for g in generated if g.member_id == second.member_id) + assert not failing.archive_path.is_file() + succeeding = next(g for g in generated if g.member_id == ada.member_id) + assert succeeding.archive_path.is_file() + + grace_event = repository.get_events(second.member_id)[-1] + assert grace_event.event_type == "sepa_notification_generated" + assert grace_event.data["archive_error"] + assert "document" not in grace_event.references + ada_event = repository.get_events(ada.member_id)[-1] + assert "document" in ada_event.references + + +def test_generated_mail_smtp_failure_for_one_debit_does_not_abort_the_batch(tmp_path, monkeypatch): + import ccma.services.sepa_mail as sepa_mail_module + from ccma.storage.repository import RepositoryError + + repository, ada = _repository(tmp_path) + second = repository.create_member(first_name="Grace", last_name="Hopper", member_number="C3-43") + second.status = "active" + second.email = "grace@example.org" + second.account_holder = "Grace Hopper" + second.iban = "DE89370400440532013000" + second.bic = "COBADEFFXXX" + second.mandate_reference = "MANDAT-43" + second.mandate_signed_at = "2025-01-10" + second.mandate_active = True + repository.save_member(second) + repository.save_contributions( + second.member_id, + ContributionData( + claims=[ + { + "claim_id": "due", + "title": "Mitgliedsbeitrag 2026", + "amount": "150.00", + "due_date": "2026-01-31", + "status": "open", + } + ] + ), + ) + organization = repository.get_configuration()["organization"] + organization.update( + { + "name": "Chaos Computer Club Mannheim e.V.", + "email": "verwaltung@example.org", + "iban": "DE89370400440532013000", + "bic": "COBADEFFXXX", + "creditor_id": "DE98ZZZ09999999999", + } + ) + repository.save_organization(organization) + repository.save_email_settings( + delivery_mode="send", + smtp_host="mail.example.org", + smtp_port=587, + smtp_security="none", + smtp_username="", + smtp_password="", + imap_host="", + imap_port=993, + imap_security="ssl", + imap_username="", + imap_password="", + imap_drafts_folder="", + ) + debits, _warnings = pending_direct_debits(repository, due_until=date(2026, 12, 31)) + assert len(debits) == 2 + + monkeypatch.setattr( + sepa_mail_module, "smtp_session", contextmanager(lambda settings: iter(["smtp-client"])) + ) + sent = [] + + def _flaky_send(client, content): + if b"ada@example.org" in content: + raise RepositoryError("E-Mail konnte nicht versandt werden: connection reset") + sent.append(content) + + monkeypatch.setattr(sepa_mail_module, "send_via_smtp", _flaky_send) + + generated, warnings = generate_debit_mails( + repository, + debits, + collection_date=date(2026, 8, 3), + delivery_mode="send", + sender_name="Verwaltung C3MA", + sender_email="verwaltung@example.org", + signature="Der Vorstand", + ) + + assert len(sent) == 1 + assert len(generated) == 1 + assert generated[0].member_id == second.member_id + assert len(warnings) == 1 + assert "connection reset" in warnings[0] + ada_events = repository.get_events(ada.member_id) + assert all(event.event_type != "sepa_notification_generated" for event in ada_events)