newsletter: unsubscribe stale subscribers instead of deleting them; sweep: apply the documented fail-safe
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
This commit is contained in:
1 parent
ac0bdb6518
commit
7bb676eb68
1 file changed
+45
-20
+45
-20
@@ -422,8 +422,11 @@ async def listmonk_reconcile() -> dict:
|
|||||||
test accounts, lapsed members) and are removed:
|
test accounts, lapsed members) and are removed:
|
||||||
- emails that exist in the members table but are inactive are taken off
|
- emails that exist in the members table but are inactive are taken off
|
||||||
the list (the subscriber record stays as a suppression marker);
|
the list (the subscriber record stays as a suppression marker);
|
||||||
- emails that are not members at all are deleted outright (stray or
|
- emails that are not members at all are taken off the list too, and the
|
||||||
test accounts should not persist in the mailing system).
|
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.
|
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", {})
|
data = r.json().get("data", {})
|
||||||
results = data.get("results", [])
|
results = data.get("results", [])
|
||||||
subs += 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
|
break
|
||||||
page += 1
|
page += 1
|
||||||
removed, emails = 0, []
|
removed, emails = 0, []
|
||||||
|
by_reason = {"inactive member": 0, "not a member": 0}
|
||||||
for s in subs:
|
for s in subs:
|
||||||
email = (s.get("email") or "").lower()
|
email = (s.get("email") or "").lower()
|
||||||
if not email or email in active:
|
if not email or email in active:
|
||||||
continue
|
continue
|
||||||
sid = s.get("id")
|
sid = s.get("id")
|
||||||
if email in member_emails:
|
# Both classes leave the list the same way: unsubscribe, keep the
|
||||||
# Inactive member: take off the list, keep the record.
|
# record. Nothing is deleted.
|
||||||
r = await client.put(
|
reason = "inactive member" if email in member_emails else "not a member"
|
||||||
f"{LISTMONK_BASE}/api/subscribers/{sid}",
|
r = await client.put(
|
||||||
headers=auth,
|
f"{LISTMONK_BASE}/api/subscribers/{sid}",
|
||||||
json={"lists": []},
|
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,
|
|
||||||
)
|
|
||||||
if r.status_code in (200, 201):
|
if r.status_code in (200, 201):
|
||||||
removed += 1
|
removed += 1
|
||||||
|
by_reason[reason] += 1
|
||||||
emails.append(email)
|
emails.append(email)
|
||||||
else:
|
else:
|
||||||
logger.warning(
|
logger.warning(
|
||||||
"Listmonk reconcile: remove %s failed: %s", email, r.status_code
|
"Listmonk reconcile: unsubscribe %s failed: %s", email, r.status_code
|
||||||
)
|
)
|
||||||
if removed:
|
if removed:
|
||||||
|
# Counts only — subscriber addresses don't belong in the log.
|
||||||
logger.info(
|
logger.info(
|
||||||
"Listmonk reconcile: removed %d stale subscribers: %s",
|
"Listmonk reconcile: removed %d stale subscribers (%s)",
|
||||||
removed,
|
removed,
|
||||||
", ".join(emails),
|
", ".join(f"{k}: {v}" for k, v in by_reason.items() if v),
|
||||||
)
|
)
|
||||||
return {"removed": removed, "emails": emails}
|
return {"removed": removed, "emails": emails}
|
||||||
except Exception as e:
|
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.
|
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_emails = {m["email"] for m in active}
|
||||||
active_by_email = {m["email"]: m for m in active}
|
active_by_email = {m["email"]: m for m in active}
|
||||||
|
|
||||||
|
|||||||
Reference in new issue
Block a user