From 7bb676eb6827f1e564e0640db1eccceb85f51a4f Mon Sep 17 00:00:00 2001 From: Ubuntu Date: Mon, 26 Oct 2026 16:38:52 +0000 Subject: [PATCH] newsletter: unsubscribe stale subscribers instead of deleting them; sweep: apply the documented fail-safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the retrospective review of 394cccfb..e24c272e (#34). listmonk_reconcile() deleted subscribers with no members row outright. That destroyed the subscriber's consent/unsubscribe history, and a stray test account is indistinguishable from a real person who subscribed outside the portal. Both classes now leave the list the same way — PUT {"lists": []} — and the record is kept as a suppression marker; nothing is purged automatically. The summary log line reports counts by reason instead of subscriber addresses. Subscriber pagination also gains a short-page fallback so a missing `total` cannot truncate the reconcile at page 1. reconcile_memberships() called fetch_members() unguarded, so the documented fail-safe — skip reconciliation rather than deactivate everyone missing from a partial list — was unreachable: since fetch_members() now raises, the sweep returned a 500 instead of {"status": "skipped"}. It now catches the failure and returns reason "member_fetch_failed". The two webhook call sites are left unguarded on purpose: a payment webhook should fail loudly so OC retries it, and it self-heals. Refs #34 --- app/main.py | 65 ++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 45 insertions(+), 20 deletions(-) diff --git a/app/main.py b/app/main.py index b397cfd..9adb527 100644 --- a/app/main.py +++ b/app/main.py @@ -422,8 +422,11 @@ async def listmonk_reconcile() -> dict: test accounts, lapsed members) and are removed: - emails that exist in the members table but are inactive are taken off the list (the subscriber record stays as a suppression marker); - - emails that are not members at all are deleted outright (stray or - test accounts should not persist in the mailing system). + - emails that are not members at all are taken off the list too, and the + record is KEPT. A DELETE would destroy the subscriber's consent + history, and a stray test account is indistinguishable from a real + person who subscribed outside the portal (or whose row was removed and + who may come back). Nobody is purged automatically. Best-effort: never raises, so a newsletter outage can't break the sweep. """ @@ -458,40 +461,46 @@ async def listmonk_reconcile() -> dict: data = r.json().get("data", {}) results = data.get("results", []) subs += results - if len(subs) >= (data.get("total") or 0) or not results: + total = data.get("total") + # Stop on an empty or short page, or once the reported count is + # reached. The short-page test means a missing `total` cannot + # truncate the reconcile at page 1. + if ( + not results + or len(results) < 100 + or (total is not None and len(subs) >= total) + ): break page += 1 removed, emails = 0, [] + by_reason = {"inactive member": 0, "not a member": 0} for s in subs: email = (s.get("email") or "").lower() if not email or email in active: continue sid = s.get("id") - if email in member_emails: - # Inactive member: take off the list, keep the record. - r = await client.put( - f"{LISTMONK_BASE}/api/subscribers/{sid}", - headers=auth, - json={"lists": []}, - ) - else: - # Not a member at all (deleted/test account): remove it. - r = await client.delete( - f"{LISTMONK_BASE}/api/subscribers/{sid}", - headers=auth, - ) + # Both classes leave the list the same way: unsubscribe, keep the + # record. Nothing is deleted. + reason = "inactive member" if email in member_emails else "not a member" + r = await client.put( + f"{LISTMONK_BASE}/api/subscribers/{sid}", + headers=auth, + json={"lists": []}, + ) if r.status_code in (200, 201): removed += 1 + by_reason[reason] += 1 emails.append(email) else: logger.warning( - "Listmonk reconcile: remove %s failed: %s", email, r.status_code + "Listmonk reconcile: unsubscribe %s failed: %s", email, r.status_code ) if removed: + # Counts only — subscriber addresses don't belong in the log. logger.info( - "Listmonk reconcile: removed %d stale subscribers: %s", + "Listmonk reconcile: removed %d stale subscribers (%s)", removed, - ", ".join(emails), + ", ".join(f"{k}: {v}" for k, v in by_reason.items() if v), ) return {"removed": removed, "emails": emails} except Exception as e: @@ -3254,7 +3263,23 @@ async def reconcile_memberships() -> dict: Idempotent and safe to run repeatedly. Returns a summary of actions taken. """ - active = await fetch_members() + # fetch_members() raises when OC is unreachable or the query fails; treat + # that like an empty result so the documented fail-safe below actually + # applies, instead of letting the exception escape and 500 the sweep. + try: + active = await fetch_members() + except Exception as e: + logger.warning( + "Sweep: member fetch failed; skipping reconciliation (fail-safe): %s", e + ) + return { + "status": "skipped", + "reason": "member_fetch_failed", + "provisioned": 0, + "deactivated": 0, + "reinvited": 0, + "newsletter_removed": 0, + } active_emails = {m["email"] for m in active} active_by_email = {m["email"]: m for m in active}