From e13ea190f6dcb437b655958119f753381b90dc00 Mon Sep 17 00:00:00 2001 From: Joaquim d'Souza Date: Thu, 11 Jun 2026 15:57:59 +0200 Subject: [PATCH 1/5] feat: improve robustness from REI examples --- packages/join-block/join.php | 65 ++++ .../src/Services/ActionNetworkService.php | 31 ++ .../join-block/src/Services/JoinService.php | 34 +- .../join-block/src/Services/StripeService.php | 349 +++++++++++++++++- .../tests/FirstInvoiceClassifierTest.php | 70 ++++ 5 files changed, 533 insertions(+), 16 deletions(-) create mode 100644 packages/join-block/tests/FirstInvoiceClassifierTest.php diff --git a/packages/join-block/join.php b/packages/join-block/join.php index cc58662..0e489d7 100644 --- a/packages/join-block/join.php +++ b/packages/join-block/join.php @@ -769,6 +769,71 @@ wp_schedule_event(time(), 'hourly', 'ck_join_block_stripe_cron_hook'); } +// Level-triggered safety net: converge Action Network state with Stripe for +// recent subscriptions, catching webhooks that were missed, suppressed or +// arrived out of order. Alerts (error-level logs, which reach Sentry) +// whenever it finds a divergence it cannot fix itself. +add_action('ck_join_block_reconciliation_cron_hook', function () { + StripeService::initialise(); + StripeService::reconcileRecentMemberships(); +}); + +if (!wp_next_scheduled('ck_join_block_reconciliation_cron_hook')) { + wp_schedule_event(time(), 'daily', 'ck_join_block_reconciliation_cron_hook'); +} + +// Heartbeat for the downstream CRM pipeline (Action Network -> Make -> +// Airtable, etc.), which the join flow cannot observe directly. Completed +// joins are recorded on success and a daily digest is logged and emailed, so +// a silent downstream outage shows up within a day instead of surfacing weeks +// later as missing records. +add_action('ck_join_flow_success', function ($data) { + $joins = get_option('CK_JOIN_FLOW_COMPLETED_JOINS', []); + if (!is_array($joins)) { + $joins = []; + } + $joins[] = [ + 'email' => $data['email'] ?? '(unknown)', + 'time' => gmdate('c'), + ]; + $cutoff = time() - 30 * 86400; + $joins = array_values(array_filter($joins, function ($join) use ($cutoff) { + return strtotime($join['time'] ?? '') >= $cutoff; + })); + update_option('CK_JOIN_FLOW_COMPLETED_JOINS', $joins, false); +}); + +add_action('ck_join_block_heartbeat_cron_hook', function () { + global $joinBlockLog; + + $joins = get_option('CK_JOIN_FLOW_COMPLETED_JOINS', []); + $dayAgo = time() - 86400; + $recent = array_values(array_filter(is_array($joins) ? $joins : [], function ($join) use ($dayAgo) { + return strtotime($join['time'] ?? '') >= $dayAgo; + })); + + $count = count($recent); + $joinBlockLog->info("Join heartbeat: $count completed join(s) in the last 24 hours"); + + $recipient = apply_filters('ck_join_flow_heartbeat_recipient', get_option('admin_email')); + if (!$recipient) { + return; + } + + $lines = array_map(function ($join) { + return ($join['time'] ?? '(unknown time)') . ' ' . ($join['email'] ?? '(unknown)'); + }, $recent); + $body = "Completed joins in the last 24 hours: $count\n\n" + . ($lines ? implode("\n", $lines) . "\n\n" : "") + . "Compare against the CRM pipeline (e.g. Action Network -> Make -> Airtable). " + . "If joins are listed here but missing downstream, the pipeline is broken.\n"; + wp_mail($recipient, "[CK Join Flow] Daily join heartbeat: $count join(s)", $body); +}); + +if (!wp_next_scheduled('ck_join_block_heartbeat_cron_hook')) { + wp_schedule_event(time(), 'daily', 'ck_join_block_heartbeat_cron_hook'); +} + if (defined('WP_CLI') && WP_CLI) { WP_CLI::add_command('join update_an_from_stripe', function () { StripeService::initialise(); diff --git a/packages/join-block/src/Services/ActionNetworkService.php b/packages/join-block/src/Services/ActionNetworkService.php index 2e58a66..aef2bbb 100644 --- a/packages/join-block/src/Services/ActionNetworkService.php +++ b/packages/join-block/src/Services/ActionNetworkService.php @@ -140,6 +140,37 @@ public static function personExists($email) return self::getPerson($email) !== null; } + /** + * Read-only view of a person for reconciliation. Returns null if the + * person does not exist, otherwise: + * 'email_status' — the primary email address status (e.g. 'subscribed', + * 'bouncing'), or null if unavailable; + * 'tags' — the person's tag names, or null when enumeration was + * aborted (treat as unknown, not as zero tags). + */ + public static function getPersonSnapshot($email) + { + $person = self::getPerson($email); + if ($person === null) { + return null; + } + + $emailStatus = null; + foreach ($person['email_addresses'] ?? [] as $address) { + if (!empty($address['primary'])) { + $emailStatus = $address['status'] ?? null; + break; + } + } + + $tagNames = self::getPersonTagNames($email); + + return [ + 'email_status' => $emailStatus, + 'tags' => is_array($tagNames) ? $tagNames : null, + ]; + } + private static function getPerson($email) { global $joinBlockLog; diff --git a/packages/join-block/src/Services/JoinService.php b/packages/join-block/src/Services/JoinService.php index 4912f64..6a0b50d 100644 --- a/packages/join-block/src/Services/JoinService.php +++ b/packages/join-block/src/Services/JoinService.php @@ -272,7 +272,13 @@ private static function tryHandleJoin($data) ActionNetworkService::signup($data); $joinBlockLog->info("Completed Action Network signup request for $email"); } catch (\Exception $exception) { - $joinBlockLog->error("Action Network error for email $email: " . $exception->getMessage()); + // The payment has already been taken by this point, so this is + // money collected with no CRM record. The saved join data lets + // ensureStripeSubscriptionsCreated() retry. + $joinBlockLog->error( + "Action Network error for email $email after successful payment — money taken with no CRM" + . " record yet: " . $exception->getMessage() + ); throw $exception; } } @@ -446,7 +452,31 @@ public static function ensureStripeSubscriptionsCreated($subscriptionId = null) || in_array($subscription->status, ['incomplete_expired', 'canceled']); if ($expired || (time() - $createdAt) > $day) { - $joinBlockLog->info("ensureStripeSubscriptionsCreated: deleting unprocessable {$name}"); + // If money was actually collected (e.g. the subscription was + // cancelled after a successful or still-settling payment), + // discarding the join data silently would strand a paid + // member outside the CRM. + $paidInvoice = false; + if ($subscription && !empty($subscription->latest_invoice)) { + try { + $latestInvoice = \Stripe\Invoice::retrieve($subscription->latest_invoice); + $paidInvoice = $latestInvoice->status === 'paid'; + } catch (\Exception $e) { + $joinBlockLog->warning( + "ensureStripeSubscriptionsCreated: could not inspect latest invoice for {$name}: " + . $e->getMessage() + ); + } + } + if ($paidInvoice) { + $joinBlockLog->error( + "ensureStripeSubscriptionsCreated: discarding {$name}, but subscription" + . " {$data['stripeSubscriptionId']} has a paid invoice — payment collected without a" + . " completed join. Complete the join manually or refund." + ); + } else { + $joinBlockLog->info("ensureStripeSubscriptionsCreated: deleting unprocessable {$name}"); + } delete_option($name); } else { $joinBlockLog->info("ensureStripeSubscriptionsCreated: not yet paid, will retry {$name}"); diff --git a/packages/join-block/src/Services/StripeService.php b/packages/join-block/src/Services/StripeService.php index 7573f99..704405c 100644 --- a/packages/join-block/src/Services/StripeService.php +++ b/packages/join-block/src/Services/StripeService.php @@ -13,11 +13,49 @@ class StripeService { + /** + * How long after a subscription's first invoice is created its webhooks + * keep deferring to the /join endpoint for Action Network state. Within + * this window first-invoice webhooks race the /join request, and + * concurrent Action Network writes mangle the webhook payloads Action + * Network sends to downstream consumers (e.g. Make). Card payments + * resolve in seconds, so anything older is a delayed-settlement payment + * method (e.g. Bacs, which takes days) or a webhook redelivery — /join + * finished long ago and there is nothing left to race. + */ + public const FIRST_INVOICE_RACE_WINDOW = 3600; + public static function initialise() { Stripe::setApiKey(Settings::get('STRIPE_SECRET_KEY')); } + /** + * Decide how to treat a webhook for a subscription_create (first) invoice. + * + * @param bool $hasUnprocessedJoinData The JOIN_FORM_UNPROCESSED_STRIPE_REQUEST + * option still exists, i.e. /join has not run. + * @param int $invoiceCreated Invoice creation timestamp. + * @param int|null $now Current timestamp, injectable for tests. + * @return string 'recover' — the join never completed: run the saved-data + * recovery path (invoice.paid only); + * 'defer' — /join may still be in flight: leave Action + * Network state alone; + * 'settle' — late settlement: /join is long done, act on + * the member's state directly. + */ + public static function classifyFirstInvoiceEvent($hasUnprocessedJoinData, $invoiceCreated, $now = null) + { + $now = $now ?? time(); + if ($hasUnprocessedJoinData) { + return 'recover'; + } + if (($now - (int) $invoiceCreated) < self::FIRST_INVOICE_RACE_WINDOW) { + return 'defer'; + } + return 'settle'; + } + /** * Validates a one-off donation amount. * Returns null if valid, or an error message string if invalid. @@ -776,10 +814,46 @@ public static function handleWebhook($event) $customerId = $subscription['customer'] ?? '(unknown)'; $joinBlockLog->info("Subscription cancelled for Stripe customer $customerId"); - if (!empty($subscription['customer'])) { - $customerLapsed = true; - $lapseTrigger = 'subscription_deleted'; + if (empty($subscription['customer'])) { + break; + } + + // Cancelling a subscription does not stop a first payment + // that is still settling (delayed methods such as Bacs): + // it can land days later against the dead subscription. + $latestInvoiceId = $subscription['latest_invoice'] ?? null; + if ($latestInvoiceId) { + try { + $latestInvoice = \Stripe\Invoice::retrieve($latestInvoiceId); + if ($latestInvoice->status === 'open') { + $joinBlockLog->error( + "Subscription {$subscription['id']} for Stripe customer $customerId was cancelled" + . " with open invoice {$latestInvoice->id}. If its payment is still processing it" + . " will settle against the dead subscription — void the invoice or refund the" + . " payment." + ); + } + } catch (\Exception $e) { + $joinBlockLog->warning( + "Could not inspect latest invoice for cancelled subscription {$subscription['id']}: " + . $e->getMessage() + ); + } + } + + // Cancelling one subscription must not lapse a member who + // still has another live one — e.g. /join cancels the + // previous subscription moments after creating its + // replacement during a re-join or tier change. + if (self::customerHasActiveSubscription($customerId, $subscription['id'] ?? null)) { + $joinBlockLog->info( + "Skipping lapsing for Stripe customer $customerId: another active subscription exists." + ); + break; } + + $customerLapsed = true; + $lapseTrigger = 'subscription_deleted'; break; case 'invoice.payment_failed': @@ -787,8 +861,25 @@ public static function handleWebhook($event) $customerId = $invoice['customer'] ?? '(unknown)'; if (($invoice['billing_reason'] ?? null) === 'subscription_create') { - $joinBlockLog->info("Skipping invoice.payment_failed lapsing for Stripe customer $customerId: subscription_create invoice, /join endpoint will handle Action Network state."); - break; + $subscriptionId = $invoice['subscription'] + ?? $invoice['parent']['subscription_details']['subscription'] + ?? null; + $hasUnprocessedJoinData = $subscriptionId + && get_option("JOIN_FORM_UNPROCESSED_STRIPE_REQUEST_{$subscriptionId}"); + + if (self::classifyFirstInvoiceEvent($hasUnprocessedJoinData, $invoice['created'] ?? 0) !== 'settle') { + $joinBlockLog->info("Skipping invoice.payment_failed lapsing for Stripe customer $customerId: subscription_create invoice, /join endpoint will handle Action Network state."); + break; + } + + // A first invoice failing this long after signup means a + // delayed-settlement payment (e.g. Bacs) bounced after the + // join completed: the member is tagged but never paid. + // Fall through to the normal failed-payment handling. + $joinBlockLog->warning( + "Late first-invoice payment failure for Stripe customer $customerId" + . " (invoice {$invoice['id']}): the join completed but the payment has now failed." + ); } if (empty($invoice['next_payment_attempt'])) { @@ -817,18 +908,31 @@ public static function handleWebhook($event) $customerId = $invoice['customer'] ?? '(unknown)'; $joinBlockLog->info("Invoice paid for Stripe customer $customerId"); if (($invoice['billing_reason'] ?? null) === 'subscription_create') { - $joinBlockLog->info("Skipping invoice.paid un-lapsing for Stripe customer $customerId: subscription_create invoice, /join endpoint will handle Action Network state."); - // If the member paid but never returned to the site, the /join - // endpoint is never hit, so complete the join from the saved - // form data instead. NOTE: this must run without acquiring the - // per-email lock here — handleJoin() acquires it itself, and - // flock blocks on a second handle even within one process. $subscriptionId = $invoice['subscription'] ?? $invoice['parent']['subscription_details']['subscription'] ?? null; - if ($subscriptionId) { + $hasUnprocessedJoinData = $subscriptionId + && get_option("JOIN_FORM_UNPROCESSED_STRIPE_REQUEST_{$subscriptionId}"); + $classification = self::classifyFirstInvoiceEvent($hasUnprocessedJoinData, $invoice['created'] ?? 0); + + if ($classification === 'recover') { + // The member paid but never returned to the site, so the + // /join endpoint was never hit: complete the join from the + // saved form data instead. NOTE: this must run without + // acquiring the per-email lock here — handleJoin() acquires + // it itself, and flock blocks on a second handle even + // within one process. + $joinBlockLog->info("Skipping invoice.paid un-lapsing for Stripe customer $customerId: subscription_create invoice, /join endpoint will handle Action Network state."); JoinService::ensureStripeSubscriptionsCreated($subscriptionId); + break; } + + if ($classification === 'defer') { + $joinBlockLog->info("Skipping invoice.paid un-lapsing for Stripe customer $customerId: subscription_create invoice, /join endpoint will handle Action Network state."); + break; + } + + self::handleLateFirstInvoicePaid($invoice, $subscriptionId, $event); break; } if (!empty($invoice['customer'])) { @@ -891,8 +995,20 @@ public static function handleWebhook($event) $currentStatus = $subscription['status'] ?? null; $email = null; - // Only act on status changes not already covered by invoice events - if ($previousStatus && $previousStatus !== $currentStatus) { + // A new subscription's incomplete -> active flip happens + // seconds before /join writes to Action Network: acting on + // it here recreates exactly the concurrent-write problem + // the subscription_create invoice guard exists to prevent. + $subscriptionAge = time() - (int) ($subscription['created'] ?? 0); + if ( + $previousStatus && $previousStatus !== $currentStatus + && $subscriptionAge < self::FIRST_INVOICE_RACE_WINDOW + ) { + $joinBlockLog->info( + "Skipping status change handling for new subscription {$subscription['id']}" + . " ($previousStatus -> $currentStatus): /join endpoint will handle Action Network state." + ); + } elseif ($previousStatus && $previousStatus !== $currentStatus) { $cid = $subscription['customer']; $email = self::getEmailForCustomer($cid); @@ -1023,6 +1139,211 @@ private static function getEmailForCustomer($customerId) return $customer->email; } + private static function customerHasActiveSubscription($customerId, $excludeSubscriptionId = null) + { + global $joinBlockLog; + + try { + foreach (['active', 'trialing'] as $status) { + $subscriptions = Subscription::all([ + 'customer' => $customerId, + 'status' => $status, + 'limit' => 10, + ]); + foreach ($subscriptions->data as $subscription) { + if ($subscription->id !== $excludeSubscriptionId) { + return true; + } + } + } + } catch (\Exception $e) { + $joinBlockLog->warning( + "Could not list subscriptions for Stripe customer $customerId: " . $e->getMessage() + ); + } + return false; + } + + /** + * A subscription_create invoice was paid long after it was created — a + * delayed-settlement payment method (e.g. Bacs direct debit, ~6 working + * days) or a webhook redelivery. Unlike fresh first invoices this cannot + * race the /join endpoint, so act on the member's state directly: un-lapse + * them if their subscription is live, or raise an alert if money has been + * collected for a subscription that no longer exists. + */ + private static function handleLateFirstInvoicePaid($invoice, $subscriptionId, $event) + { + global $joinBlockLog; + + $customerId = $invoice['customer']; + + $subscription = null; + if ($subscriptionId) { + try { + $subscription = Subscription::retrieve($subscriptionId); + } catch (\Stripe\Exception\InvalidRequestException $e) { + $joinBlockLog->warning( + "Late first-invoice settlement: could not retrieve subscription $subscriptionId: " + . $e->getMessage() + ); + } + } + $status = $subscription->status ?? 'missing'; + + if (!in_array($status, ['active', 'trialing'])) { + $joinBlockLog->error( + "Payment collected for dead subscription: first invoice {$invoice['id']} of Stripe customer" + . " $customerId settled late, but subscription " . ($subscriptionId ?: '(unknown)') . " is $status." + . " Refund the payment or reinstate the membership manually." + ); + return; + } + + $email = self::getEmailForCustomer($customerId); + if (!$email) { + $joinBlockLog->error( + "Late first-invoice settlement for Stripe customer $customerId: could not resolve an email" + . " address, cannot reconcile Action Network state." + ); + return; + } + + $joinBlockLog->info( + "Late first-invoice settlement for $email: subscription $subscriptionId is $status," + . " ensuring the member is not marked lapsed." + ); + + $lockFile = JoinService::acquireLock($email); + try { + $context = ['provider' => 'stripe', 'trigger' => 'late_first_invoice_paid', 'event' => $event]; + if (JoinService::shouldUnlapseMember($email, $context)) { + JoinService::toggleMemberLapsed($email, false, null, $context); + } + } finally { + JoinService::releaseLock($lockFile); + } + } + + /** + * Level-triggered safety net, run daily by cron: converge Action Network + * state with Stripe for customers with recent subscription activity, + * catching webhooks that were missed, suppressed or arrived out of order. + * + * Only the unambiguous divergence is fixed automatically (a lapsed tag on + * a paying member with a live subscription, at most one Action Network + * write per person, under the per-email lock). Everything else — money + * collected with no CRM record, money collected with no live subscription, + * a member whose email is bouncing — is alerted via error/warning-level + * logs, which reach Sentry, because fixing those requires either full join + * data or a human decision (refund vs reinstate). + */ + public static function reconcileRecentMemberships($sinceDays = 7) + { + global $joinBlockLog; + + $joinBlockLog->info("Running reconcileRecentMemberships"); + + if (!Settings::get("USE_ACTION_NETWORK")) { + $joinBlockLog->info("reconcileRecentMemberships: Action Network integration disabled, nothing to do"); + return; + } + + $subscriptions = Subscription::all([ + 'created' => ['gte' => time() - $sinceDays * 86400], + 'status' => 'all', + 'limit' => 100, + ]); + + // Group by customer so a re-join sequence (cancelled subscription + // followed by its live replacement) is judged as one membership. + $customerIds = []; + foreach ($subscriptions->autoPagingIterator() as $subscription) { + $customerIds[$subscription->customer] = true; + } + + foreach (array_keys($customerIds) as $customerId) { + try { + self::reconcileCustomerMembership($customerId); + } catch (\Exception $e) { + $joinBlockLog->error( + "reconcileRecentMemberships: failed for Stripe customer $customerId: " . $e->getMessage() + ); + } + } + } + + private static function reconcileCustomerMembership($customerId) + { + global $joinBlockLog; + + $paidInvoices = \Stripe\Invoice::all([ + 'customer' => $customerId, + 'status' => 'paid', + 'limit' => 1, + ]); + if (count($paidInvoices->data) === 0) { + // Abandoned signup (incomplete payment, no charge): correctly has + // no membership, nothing to reconcile. + return; + } + + $email = self::getEmailForCustomer($customerId); + if (!$email) { + $joinBlockLog->warning( + "reconcileRecentMemberships: could not resolve email for Stripe customer $customerId" + ); + return; + } + + $hasLiveSubscription = self::customerHasActiveSubscription($customerId); + $lapsedTag = Settings::get("LAPSED_TAG"); + + $person = ActionNetworkService::getPersonSnapshot($email); + $isLapsed = $person !== null + && $lapsedTag + && is_array($person['tags']) + && in_array($lapsedTag, $person['tags'], true); + + if ($hasLiveSubscription) { + if ($person === null) { + $joinBlockLog->error( + "reconcileRecentMemberships: $email has a paid, active Stripe subscription but no Action" + . " Network record — the join never completed. Investigate and backfill." + ); + return; + } + + if ($isLapsed) { + $joinBlockLog->error( + "reconcileRecentMemberships: $email has an active Stripe subscription but carries the" + . " '$lapsedTag' tag — removing it." + ); + $lockFile = JoinService::acquireLock($email); + try { + $context = ['provider' => 'stripe', 'trigger' => 'reconciliation', 'event' => null]; + if (JoinService::shouldUnlapseMember($email, $context)) { + JoinService::toggleMemberLapsed($email, false, null, $context); + } + } finally { + JoinService::releaseLock($lockFile); + } + } + + if (($person['email_status'] ?? null) === 'bouncing') { + $joinBlockLog->warning( + "reconcileRecentMemberships: paying member $email has a bouncing email address in Action" + . " Network — they are not receiving membership emails. Possible typo at signup." + ); + } + } elseif ($person !== null && !$isLapsed && is_array($person['tags'])) { + $joinBlockLog->error( + "reconcileRecentMemberships: $email has paid Stripe invoices but no live subscription, and is" + . " not marked lapsed in Action Network. Refund or reinstate manually." + ); + } + } + private static function resolveTierTagChanges(array $newPlan, ?array $oldPlan): array { $parseTags = fn($str) => array_filter(array_map('trim', explode(',', $str ?? '')), fn($t) => $t !== ''); diff --git a/packages/join-block/tests/FirstInvoiceClassifierTest.php b/packages/join-block/tests/FirstInvoiceClassifierTest.php new file mode 100644 index 0000000..c9d7ee6 --- /dev/null +++ b/packages/join-block/tests/FirstInvoiceClassifierTest.php @@ -0,0 +1,70 @@ +assertSame( + 'recover', + StripeService::classifyFirstInvoiceEvent(true, self::NOW - 5, self::NOW) + ); + $this->assertSame( + 'recover', + StripeService::classifyFirstInvoiceEvent(true, self::NOW - 10 * 86400, self::NOW) + ); + } + + public function testDeferWhenInvoiceIsFresh(): void + { + // A card-paid first invoice settles seconds after creation, while + // /join is still in flight: leave Action Network state alone. + $this->assertSame( + 'defer', + StripeService::classifyFirstInvoiceEvent(false, self::NOW - 5, self::NOW) + ); + $this->assertSame( + 'defer', + StripeService::classifyFirstInvoiceEvent( + false, + self::NOW - (StripeService::FIRST_INVOICE_RACE_WINDOW - 1), + self::NOW + ) + ); + } + + public function testSettleWhenInvoiceIsOld(): void + { + // A first invoice settling days later (e.g. Bacs) cannot race /join. + $this->assertSame( + 'settle', + StripeService::classifyFirstInvoiceEvent( + false, + self::NOW - StripeService::FIRST_INVOICE_RACE_WINDOW, + self::NOW + ) + ); + $this->assertSame( + 'settle', + StripeService::classifyFirstInvoiceEvent(false, self::NOW - 8 * 86400, self::NOW) + ); + } + + public function testMissingInvoiceCreatedTimestampSettles(): void + { + // A zero/absent created timestamp must not be mistaken for a fresh + // invoice, or the event would be dropped forever. + $this->assertSame( + 'settle', + StripeService::classifyFirstInvoiceEvent(false, 0, self::NOW) + ); + } +} From e96f4d0c69c5058583e02b19c999b1a0b8050d3d Mon Sep 17 00:00:00 2001 From: Joaquim d'Souza Date: Mon, 20 Jul 2026 11:27:59 +0100 Subject: [PATCH 2/5] fix: make join CRM writes durable and stop double-charging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Investigating a member who paid on 2026-06-19 but has no demographic data in Action Network. Their AN record was created 11 days later by the newsletter form, carrying only a newsletter-signup tag; the join flow never wrote to AN at all. They were also charged twice, 91 seconds apart. ActionNetworkService::signup() is the only path that writes given_name, family_name, postal_addresses, phone and dob. Everything else that touches AN (updateCustomFields, addTag) writes only email, tags and custom fields by design, so once the signup is missed the demographics never arrive. Amount verification cancelPreviousAndGetCurrentSubscription() reported the current subscription's amount by scanning the customer's subscription list and initialising $amount = 0. A missing subscription ID, a wrong-customer subscription, or a Stripe error inside the swallowing catch all left that 0 in place and returned it as though it were real. The caller read 0.0 as a mismatch and threw before ever reaching the CRM write. Split into getSubscriptionAmount(), which retrieves by ID and returns null for unknown while letting API errors propagate, and cancelPreviousSubscriptions(). The caller now distinguishes "cannot verify" (code 9, retryable) from a real mismatch (code 8), compares with a tolerance, and verifies BEFORE cancelling — previously a failed check left the member's old subscription cancelled with nothing to show for it. CRM failure no longer fails the member's request The payment has already succeeded by the time AN is called, so a rethrow showed a paying member an error they could not act on and invited the resubmit that creates a duplicate subscription. Failures now write a JOIN_FORM_PENDING_CRM_* record instead. ensureStripeSubscriptionsCreated() likewise no longer discards join data for a cancelled-but-paid subscription. That record is the only copy of what the member submitted. Recovery snapshot freshness join.php saves the snapshot at create-subscription time, before the form is finished. handleJoin() now refreshes it with the payload it was actually handed, preserving the original createdAt so the expiry check still fires. Duplicate subscriptions createSubscription() now takes a Stripe idempotency key derived from the form session and the member's choices, so resubmitting the same request returns the existing subscription. The payment page skips confirmation when the returned intent has already succeeded. Also fixes a precedence slip where `(float) $x ?? 0` parsed as `((float) $x) ?? 0`, making the fallback dead code. Note: JOIN_FORM_PENDING_CRM_* records have no consumer yet — the retry worker is the next commit. Until then they accumulate, which is still strictly better than the data being destroyed. The Stripe idempotency change alters live payment behaviour and wants test-mode validation before deploy. Co-Authored-By: Claude Opus 4.8 --- packages/join-block/join.php | 3 +- .../join-block/src/Services/JoinService.php | 163 +++++++++++++++-- .../join-block/src/Services/StripeService.php | 97 ++++++++-- .../tests/SubscriptionIdempotencyKeyTest.php | 172 ++++++++++++++++++ .../src/pages/payment-details.page.tsx | 17 +- 5 files changed, 419 insertions(+), 33 deletions(-) create mode 100644 packages/join-block/tests/SubscriptionIdempotencyKeyTest.php diff --git a/packages/join-block/join.php b/packages/join-block/join.php index 0e489d7..f7956ce 100644 --- a/packages/join-block/join.php +++ b/packages/join-block/join.php @@ -509,7 +509,8 @@ $data["customMembershipAmount"] ?? null, $data["donationAmount"] ?? null, $data["recurDonation"] ?? false, - !empty($data["donationSupporterMode"]) + !empty($data["donationSupporterMode"]), + StripeService::buildSubscriptionIdempotencyKey($data, $plan) ); // Save this data in the database so if the user pays but never returns to diff --git a/packages/join-block/src/Services/JoinService.php b/packages/join-block/src/Services/JoinService.php index 6a0b50d..7eb58c5 100644 --- a/packages/join-block/src/Services/JoinService.php +++ b/packages/join-block/src/Services/JoinService.php @@ -10,6 +10,17 @@ class JoinService { + // Membership amounts are money in major units, held as floats. Compare with + // a tolerance rather than == / === so that a value which has been through + // Stripe's minor-unit integers and back cannot fail on representation + // alone. Half a penny is far below the smallest real price difference. + public const AMOUNT_EPSILON = 0.005; + + // Join data whose CRM push failed after the payment succeeded. The member + // has paid and is a member; only the CRM write is outstanding, so this is + // kept for retry rather than surfaced as a failure to them. + public const CRM_RETRY_OPTION_PREFIX = 'JOIN_FORM_PENDING_CRM_'; + // According to error messages from Chargebee, dates should be sent as the format yyyy-MM-dd. // Meaning 2021-12-25 for Christmas Day, 25th of December 2021. private static function formatDob($day, $month, $year) @@ -20,6 +31,87 @@ private static function formatDob($day, $month, $year) return $date->format('Y-m-d'); } + /** + * Option name for a pending CRM push. Keyed on the Stripe subscription + * where there is one, falling back to a hash of the email so a GoCardless + * or Chargebee join still gets exactly one record per member. + */ + private static function crmRetryKey($data) + { + $subscriptionId = $data['stripeSubscriptionId'] ?? ''; + if ($subscriptionId) { + return self::CRM_RETRY_OPTION_PREFIX . $subscriptionId; + } + $email = strtolower(trim((string) ($data['email'] ?? ''))); + return self::CRM_RETRY_OPTION_PREFIX . 'email_' . sha1($email); + } + + /** + * Durably record that a CRM push is still outstanding for a member whose + * payment has already succeeded. + * + * This record is the only copy of what the member submitted — the join + * form data is not otherwise persisted — so losing it means their details + * can only be recovered by asking them again. + */ + public static function queueCrmRetry($data, $service, $reason) + { + global $joinBlockLog; + + $optionName = self::crmRetryKey($data); + + $attempts = 1; + $existing = get_option($optionName); + if ($existing) { + $decoded = json_decode($existing, true); + $attempts = (int) ($decoded['attempts'] ?? 0) + 1; + } + + update_option($optionName, wp_json_encode([ + 'data' => $data, + 'service' => $service, + 'reason' => $reason, + 'attempts' => $attempts, + 'lastAttemptAt' => time(), + ])); + + $email = $data['email'] ?? 'unknown'; + $joinBlockLog->error("Queued $service CRM retry for $email (attempt $attempts): $reason"); + } + + /** + * Refresh the recovery snapshot with the payload we were actually handed. + * + * join.php writes this at create-subscription time, before the member has + * finished the form, so the stored copy can be missing fields that the + * final /join request carries. Any replay should use the freshest version. + */ + private static function saveRecoverySnapshot($data) + { + $subscriptionId = $data['stripeSubscriptionId'] ?? ''; + if (!$subscriptionId) { + return; + } + + $optionName = "JOIN_FORM_UNPROCESSED_STRIPE_REQUEST_{$subscriptionId}"; + + $existing = get_option($optionName); + if ($existing) { + // Keep the original createdAt. ensureStripeSubscriptionsCreated() + // uses it to decide when to stop retrying, so refreshing it on + // every attempt would make the record immortal. + $decoded = json_decode($existing, true); + if (!empty($decoded['createdAt'])) { + $data['createdAt'] = $decoded['createdAt']; + } + } + if (empty($data['createdAt'])) { + $data['createdAt'] = time(); + } + + update_option($optionName, wp_json_encode($data)); + } + public static function handleJoin($data) { global $joinBlockLog; @@ -34,6 +126,7 @@ public static function handleJoin($data) } } $lockFile = self::acquireLock($lockKey); + self::saveRecoverySnapshot($data); $chargebeeCustomer = self::tryHandleJoin($data); // The join is complete, so it no longer needs to be picked up by // ensureStripeSubscriptionsCreated() (the webhook/cron recovery path) @@ -182,10 +275,10 @@ private static function tryHandleJoin($data) } $data['membershipPlan']['remove_tags'] = Settings::computeTagsToRemove($data['membershipPlan']); - $membershipAmount = (float) $data['membershipPlan']['amount'] ?? 0; + $membershipAmount = (float) ($data['membershipPlan']['amount'] ?? 0); if ($data['membershipPlan']['allow_custom_amount']) { $minimumAmount = $membershipAmount; - $membershipAmount = (float) $data['customMembershipAmount'] ?? 0; + $membershipAmount = (float) ($data['customMembershipAmount'] ?? 0); if ($membershipAmount < $minimumAmount || $membershipAmount > 1000) { $error = "Invalid membership amount: $membershipAmount < $minimumAmount or > 1000"; $joinBlockLog->error($error); @@ -220,13 +313,45 @@ private static function tryHandleJoin($data) if (Settings::get("USE_STRIPE") && !$isOneOffSupporterDonation) { StripeService::initialise(); - $subscriptionInfo = StripeService::cancelPreviousAndGetCurrentSubscription($data["email"], $data["stripeCustomerId"] ?? null, $data["stripeSubscriptionId"] ?? null); - if ($subscriptionInfo["amount"] !== $membershipAmount) { - $email = $data['email']; - $subAmount = $subscriptionInfo["amount"]; - $joinBlockLog->error("Found mismatched subscription amounts for $email - claimed: $membershipAmount, found in stripe: $subAmount"); + $email = $data['email']; + $subscriptionId = $data["stripeSubscriptionId"] ?? null; + + // Verify the amount BEFORE cancelling anything. Cancelling first + // means a failed verification leaves the member with their previous + // subscription cancelled and no new membership to show for it. + try { + $actualAmount = StripeService::getSubscriptionAmount($subscriptionId); + } catch (\Exception $e) { + $joinBlockLog->error( + "Could not read subscription amount for $email from Stripe: " . $e->getMessage() + ); + throw new JoinBlockException("Could not verify subscription amount", 9); + } + + // Distinct from a mismatch: we do not know the amount, so we cannot + // say it is wrong. Treated as retryable rather than telling a member + // who has already paid that their amount is invalid. + if ($actualAmount === null) { + $subscriptionLabel = $subscriptionId ?: 'none'; + $joinBlockLog->error( + "Could not determine subscription amount for $email (subscription: $subscriptionLabel)" + ); + throw new JoinBlockException("Could not verify subscription amount", 9); + } + + if (abs($actualAmount - $membershipAmount) > self::AMOUNT_EPSILON) { + $joinBlockLog->error( + "Found mismatched subscription amounts for $email - claimed: $membershipAmount," + . " found in stripe: $actualAmount" + ); throw new JoinBlockException("Invalid subscription amount", 8); } + + $subscriptionInfo = StripeService::cancelPreviousSubscriptions( + $email, + $data["stripeCustomerId"] ?? null, + $subscriptionId + ); $data["stripeFirstSubscriptionDate"] = $subscriptionInfo["firstSubscription"]; $data["stripeFirstPaymentDate"] = $subscriptionInfo["firstPayment"]; $data["stripeLastPaymentDate"] = $subscriptionInfo["lastPayment"]; @@ -272,14 +397,16 @@ private static function tryHandleJoin($data) ActionNetworkService::signup($data); $joinBlockLog->info("Completed Action Network signup request for $email"); } catch (\Exception $exception) { - // The payment has already been taken by this point, so this is - // money collected with no CRM record. The saved join data lets - // ensureStripeSubscriptionsCreated() retry. + // The payment has already succeeded, so the member has joined + // and there is nothing they can do about a CRM failure. Failing + // the request would show them an error they cannot act on, and + // invite a resubmit — which creates a duplicate subscription. + // Record the outstanding push instead, and retry it out of band. $joinBlockLog->error( "Action Network error for email $email after successful payment — money taken with no CRM" . " record yet: " . $exception->getMessage() ); - throw $exception; + self::queueCrmRetry($data, 'action_network', $exception->getMessage()); } } @@ -470,9 +597,17 @@ public static function ensureStripeSubscriptionsCreated($subscriptionId = null) } if ($paidInvoice) { $joinBlockLog->error( - "ensureStripeSubscriptionsCreated: discarding {$name}, but subscription" - . " {$data['stripeSubscriptionId']} has a paid invoice — payment collected without a" - . " completed join. Complete the join manually or refund." + "ensureStripeSubscriptionsCreated: subscription {$data['stripeSubscriptionId']} has a" + . " paid invoice — payment collected without a completed join. Preserving the" + . " submitted data for retry." + ); + // Do not discard. This is the only record of what the + // member submitted, so dropping it makes the join + // unrecoverable without asking them for it again. + self::queueCrmRetry( + $data, + 'action_network', + 'payment collected without a completed join' ); } else { $joinBlockLog->info("ensureStripeSubscriptionsCreated: deleting unprocessable {$name}"); diff --git a/packages/join-block/src/Services/StripeService.php b/packages/join-block/src/Services/StripeService.php index 704405c..8859ccc 100644 --- a/packages/join-block/src/Services/StripeService.php +++ b/packages/join-block/src/Services/StripeService.php @@ -145,7 +145,39 @@ public static function resolveSubscriptionPriceStrategy(array $plan, float $cust return 'default'; } - public static function createSubscription($customer, $plan, $customAmount = null, $donationAmount = null, $recurDonation = false, $isSupporterMode = false) + /** + * Stable key for a subscription-creation request, or null when one cannot + * be derived safely. + * + * Submitting the same choices twice within one form session should return + * the subscription Stripe already created rather than making another one: + * resubmitting after an error is how a member ends up charged twice. + * Changing plan or amount changes the key, as does starting a new session, + * so a deliberate second subscription is still possible. + * + * Without a session token the key would collapse to email + plan, and two + * genuinely separate joins would collide — so we return null and accept the + * old behaviour rather than risk replaying the wrong subscription. + */ + public static function buildSubscriptionIdempotencyKey($data, $plan) + { + $sessionToken = $data['sessionToken'] ?? ''; + if (!$sessionToken) { + return null; + } + + return 'join_sub_' . sha1(implode('|', [ + $sessionToken, + strtolower(trim((string) ($data['email'] ?? ''))), + Settings::getMembershipPlanId($plan), + (string) ($data['customMembershipAmount'] ?? ''), + (string) ($data['donationAmount'] ?? ''), + !empty($data['recurDonation']) ? '1' : '0', + !empty($data['donationSupporterMode']) ? '1' : '0', + ])); + } + + public static function createSubscription($customer, $plan, $customAmount = null, $donationAmount = null, $recurDonation = false, $isSupporterMode = false, $idempotencyKey = null) { $priceId = $plan["stripe_price_id"]; $customAmount = (float) $customAmount; @@ -189,7 +221,8 @@ public static function createSubscription($customer, $plan, $customAmount = null $subscriptionPayload['add_invoice_items'] = $addInvoiceItems; } - $subscription = Subscription::create($subscriptionPayload); + $options = $idempotencyKey ? ['idempotency_key' => $idempotencyKey] : []; + $subscription = Subscription::create($subscriptionPayload, $options); return $subscription; } @@ -562,12 +595,53 @@ public static function getOrCreatePriceForProduct($product, $amount, $currency, } /** - * Cancels the customer's existing subscriptions, except for $subscriptionId, which is the - * subscription that was just created via the browser Stripe client during this join flow. - * That current subscription is left in place, and its amount is returned so the caller can - * verify it against the amount claimed in the join request. + * The recurring amount of one specific subscription, in major units + * (e.g. 3.30 for £3.30), or null when it cannot be determined. + * + * Fetches by ID rather than scanning the customer's subscription list, so + * there is no "wasn't in the page we looked at" failure mode. Callers MUST + * treat null as *unknown* and never as zero: a null that gets coerced to + * 0.0 looks exactly like a genuine amount mismatch, which is how a paying + * member can be rejected at the amount check. + * + * API errors propagate rather than being swallowed, so a transport failure + * (retryable) stays distinguishable from a subscription that really has no + * priced item (not retryable). + * + * @throws ApiErrorException */ - public static function cancelPreviousAndGetCurrentSubscription($email, $customerId, $subscriptionId) + public static function getSubscriptionAmount($subscriptionId) + { + global $joinBlockLog; + + if (!$subscriptionId) { + $joinBlockLog->warning("getSubscriptionAmount: called without a subscription ID"); + return null; + } + + $subscription = Subscription::retrieve($subscriptionId); + $item = $subscription->items->first(); + + if (!$item || !isset($item->price->unit_amount)) { + $joinBlockLog->warning("getSubscriptionAmount: $subscriptionId has no priced item"); + return null; + } + + return round($item->price->unit_amount / 100, 2); + } + + /** + * Cancel every *other* active-ish subscription for the customer, and report + * the member's subscription/payment dates. $subscriptionId is the one just + * created by the browser Stripe client during this join flow, and is left + * in place. + * + * Deliberately does not report the current subscription's amount — see + * getSubscriptionAmount(). Date collection is best-effort: the catch below + * keeps a transient Stripe error from failing an otherwise good join, and + * the dates are only used to populate CRM custom fields. + */ + public static function cancelPreviousSubscriptions($email, $customerId, $subscriptionId) { global $joinBlockLog; @@ -576,7 +650,6 @@ public static function cancelPreviousAndGetCurrentSubscription($email, $customer $firstSubscriptionDate = date('Y-m-d'); $firstPayment = null; $lastPayment = null; - $amount = 0; if (!$customerId) { [$customer,] = self::upsertCustomer($email); @@ -616,13 +689,6 @@ public static function cancelPreviousAndGetCurrentSubscription($email, $customer $invoice->voidInvoice(); } } - - if ($sub->id === $subscriptionId) { - $subItem = $sub->items->first(); - if ($subItem) { - $amount = $subItem->price->unit_amount; - } - } } // Fetch all paid invoices for first/last payment dates @@ -649,7 +715,6 @@ public static function cancelPreviousAndGetCurrentSubscription($email, $customer "firstSubscription" => $firstSubscriptionDate, "firstPayment" => $firstPayment, "lastPayment" => $lastPayment, - "amount" => round($amount / 100, 2), ]; } diff --git a/packages/join-block/tests/SubscriptionIdempotencyKeyTest.php b/packages/join-block/tests/SubscriptionIdempotencyKeyTest.php new file mode 100644 index 0000000..6a56010 --- /dev/null +++ b/packages/join-block/tests/SubscriptionIdempotencyKeyTest.php @@ -0,0 +1,172 @@ + 'Cost of a cup of coffee in your country', + 'frequency' => 'monthly', + 'currency' => 'USD', + ]; + + private array $data = [ + 'sessionToken' => '11111111-2222-3333-4444-555555555555', + 'email' => 'member@example.com', + 'customMembershipAmount' => null, + 'donationAmount' => null, + 'recurDonation' => false, + 'donationSupporterMode' => false, + ]; + + protected function setUp(): void + { + parent::setUp(); + Monkey\setUp(); + + Monkey\Functions\when('sanitize_title')->alias(function ($title) { + $title = strtolower((string) $title); + $title = preg_replace('/[^a-z0-9]+/', '-', $title); + return trim($title, '-'); + }); + } + + protected function tearDown(): void + { + Monkey\tearDown(); + parent::tearDown(); + } + + private function key(array $overrides = []): ?string + { + return StripeService::buildSubscriptionIdempotencyKey( + array_merge($this->data, $overrides), + $this->plan + ); + } + + // ------------------------------------------------------------------------- + // Stability — the double-charge case + // ------------------------------------------------------------------------- + + public function testResubmittingTheSameChoicesProducesTheSameKey(): void + { + $this->assertSame($this->key(), $this->key()); + } + + public function testEmailIsNormalisedSoCasingDoesNotSplitTheKey(): void + { + $this->assertSame( + $this->key(), + $this->key(['email' => ' Member@Example.COM ']) + ); + } + + // ------------------------------------------------------------------------- + // Distinctness — a deliberate second subscription must still be possible + // ------------------------------------------------------------------------- + + public function testANewFormSessionProducesADifferentKey(): void + { + $this->assertNotSame( + $this->key(), + $this->key(['sessionToken' => '99999999-8888-7777-6666-555555555555']) + ); + } + + public function testChangingTheCustomAmountProducesADifferentKey(): void + { + $this->assertNotSame( + $this->key(['customMembershipAmount' => 5]), + $this->key(['customMembershipAmount' => 10]) + ); + } + + public function testChangingTheDonationAmountProducesADifferentKey(): void + { + $this->assertNotSame( + $this->key(['donationAmount' => 5]), + $this->key(['donationAmount' => 20]) + ); + } + + public function testTogglingRecurringDonationProducesADifferentKey(): void + { + $this->assertNotSame( + $this->key(['donationAmount' => 5, 'recurDonation' => false]), + $this->key(['donationAmount' => 5, 'recurDonation' => true]) + ); + } + + public function testTogglingSupporterModeProducesADifferentKey(): void + { + $this->assertNotSame( + $this->key(['donationSupporterMode' => false]), + $this->key(['donationSupporterMode' => true]) + ); + } + + public function testADifferentPlanProducesADifferentKey(): void + { + $usdKey = StripeService::buildSubscriptionIdempotencyKey($this->data, $this->plan); + $gbpKey = StripeService::buildSubscriptionIdempotencyKey( + $this->data, + array_merge($this->plan, ['currency' => 'GBP']) + ); + + $this->assertNotSame($usdKey, $gbpKey); + } + + public function testADifferentMemberProducesADifferentKey(): void + { + $this->assertNotSame( + $this->key(), + $this->key(['email' => 'someone-else@example.com']) + ); + } + + // ------------------------------------------------------------------------- + // Safety valve + // ------------------------------------------------------------------------- + + /** + * Without a session token the key would collapse to email + plan, so two + * genuinely separate joins by the same person would collide and the second + * would silently receive the first subscription. Returning null keeps the + * old (duplicate-prone) behaviour, which is the lesser failure. + */ + public function testMissingSessionTokenYieldsNoKeyRatherThanAWeakOne(): void + { + $this->assertNull($this->key(['sessionToken' => ''])); + + $withoutToken = $this->data; + unset($withoutToken['sessionToken']); + $this->assertNull( + StripeService::buildSubscriptionIdempotencyKey($withoutToken, $this->plan) + ); + } + + public function testKeyFitsWithinStripesIdempotencyKeyLimit(): void + { + $this->assertLessThanOrEqual(255, strlen((string) $this->key())); + } +} diff --git a/packages/join-flow/src/pages/payment-details.page.tsx b/packages/join-flow/src/pages/payment-details.page.tsx index c91a794..1f34081 100644 --- a/packages/join-flow/src/pages/payment-details.page.tsx +++ b/packages/join-flow/src/pages/payment-details.page.tsx @@ -406,7 +406,8 @@ const StripeForm = ({ ); } else { const subscription = await createSubscription({ ...data }); - clientSecret = subscription.latest_invoice.payment_intent.client_secret; + const paymentIntent = subscription.latest_invoice.payment_intent; + clientSecret = paymentIntent.client_secret; sessionStorage.setItem( SAVED_STATE_KEY, @@ -414,9 +415,21 @@ const StripeForm = ({ ...data, stripeCustomerId: subscription.customer, stripeSubscriptionId: subscription.id, - stripePaymentIntentId: subscription.latest_invoice.payment_intent.id + stripePaymentIntentId: paymentIntent.id }) ); + + // Resubmitting within one session returns the subscription Stripe + // already created (see buildSubscriptionIdempotencyKey server-side). + // If that payment has already gone through, confirming it again would + // error — and taking a second payment would be worse. Carry straight + // on to the confirm step instead. + if (paymentIntent.status === "succeeded") { + const succeededUrl = new URL(window.location.href); + succeededUrl.searchParams.set("stripe_success", "true"); + window.location.assign(succeededUrl.toString()); + return; + } } const returnUrl = new URL(window.location.href); From 8b22b77e8dfdb1333f6da6bbb24b0cb0a4e980c7 Mon Sep 17 00:00:00 2001 From: Joaquim d'Souza Date: Mon, 20 Jul 2026 11:44:07 +0100 Subject: [PATCH 3/5] feat: retry outstanding CRM pushes without touching payments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the consumer for the JOIN_FORM_PENDING_CRM_* records written by the previous commit, so a member whose payment succeeded but whose CRM write failed is finished automatically instead of silently missing. ensureCrmPushesCompleted(), hourly: - reads Stripe only to confirm the payment still stands - takes the same per-email lock as /join and the webhooks - retries just the services that failed, with 1h/2h/4h... backoff capped at 24h - after 12 attempts parks the record for a human. It is never deleted: it holds the only copy of what the member submitted. The "don't touch working payments" property is structural, not a convention. The CRM writes are extracted out of tryHandleJoin() into pushToCrms(), which contains no payment code at all, so the retry path cannot reach subscription cancellation even if edited carelessly. Mailchimp and Zetkin failures now queue too, rather than being logged and forgotten. Recovery replays no longer cancel subscriptions ensureStripeSubscriptionsCreated() and the GoCardless cron replay handleJoin() from data saved hours or days earlier. That ran cancelPreviousSubscriptions() against a stale view of which subscription is current — so if the member had joined again since, the replay would cancel their live subscription. Replays now set isRecoveryReplay and only read. To make that possible, cancelPreviousSubscriptions() is split into a read (getSubscriptionDates) and a write (the cancellation), with customer resolution pulled out into resolveCustomerId(). Reconciler no longer accepts mere existence as proof of a join reconcileRecentMemberships() treated "an Action Network person exists" as evidence the join completed. A newsletter signup creates a person from an email alone, so it masked exactly the case being investigated and no alert fired. It now also checks for a name, which the join flow always collects and a newsletter signup never sets. Co-Authored-By: Claude Opus 4.8 --- packages/join-block/join.php | 14 + .../src/Services/ActionNetworkService.php | 7 +- .../join-block/src/Services/JoinService.php | 309 ++++++++++++++---- .../join-block/src/Services/StripeService.php | 175 +++++++--- .../join-block/tests/CrmRetryBackoffTest.php | 95 ++++++ .../tests/JoinServiceMailchimpTest.php | 34 +- 6 files changed, 537 insertions(+), 97 deletions(-) create mode 100644 packages/join-block/tests/CrmRetryBackoffTest.php diff --git a/packages/join-block/join.php b/packages/join-block/join.php index f7956ce..44bec60 100644 --- a/packages/join-block/join.php +++ b/packages/join-block/join.php @@ -748,6 +748,8 @@ continue; } + // Replay: complete the join without touching payment state. + $data['isRecoveryReplay'] = true; JoinService::handleJoin($data); delete_option($result->option_name); $joinBlockLog->info("ensureSubscriptionsCreated: success, deleting option {$result->option_name}"); @@ -770,6 +772,18 @@ wp_schedule_event(time(), 'hourly', 'ck_join_block_stripe_cron_hook'); } +// Finish CRM pushes for members whose payment succeeded but whose CRM record +// was never written. Complements the reconciliation cron below, which can spot +// such a member but holds no copy of their details to fix it with. Reads Stripe +// only to confirm the payment still stands; writes exclusively to CRMs. +add_action('ck_join_block_crm_retry_cron_hook', function () { + JoinService::ensureCrmPushesCompleted(); +}); + +if (!wp_next_scheduled('ck_join_block_crm_retry_cron_hook')) { + wp_schedule_event(time(), 'hourly', 'ck_join_block_crm_retry_cron_hook'); +} + // Level-triggered safety net: converge Action Network state with Stripe for // recent subscriptions, catching webhooks that were missed, suppressed or // arrived out of order. Alerts (error-level logs, which reach Sentry) diff --git a/packages/join-block/src/Services/ActionNetworkService.php b/packages/join-block/src/Services/ActionNetworkService.php index aef2bbb..bd84cf3 100644 --- a/packages/join-block/src/Services/ActionNetworkService.php +++ b/packages/join-block/src/Services/ActionNetworkService.php @@ -146,7 +146,11 @@ public static function personExists($email) * 'email_status' — the primary email address status (e.g. 'subscribed', * 'bouncing'), or null if unavailable; * 'tags' — the person's tag names, or null when enumeration was - * aborted (treat as unknown, not as zero tags). + * aborted (treat as unknown, not as zero tags); + * 'has_name' — whether the person has a given or family name. The + * join flow always collects a name, while newsletter and + * petition signups create a person from an email alone, + * so this distinguishes "joined" from "merely exists". */ public static function getPersonSnapshot($email) { @@ -168,6 +172,7 @@ public static function getPersonSnapshot($email) return [ 'email_status' => $emailStatus, 'tags' => is_array($tagNames) ? $tagNames : null, + 'has_name' => !empty($person['given_name']) || !empty($person['family_name']), ]; } diff --git a/packages/join-block/src/Services/JoinService.php b/packages/join-block/src/Services/JoinService.php index 7eb58c5..9497bc1 100644 --- a/packages/join-block/src/Services/JoinService.php +++ b/packages/join-block/src/Services/JoinService.php @@ -21,6 +21,11 @@ class JoinService // kept for retry rather than surfaced as a failure to them. public const CRM_RETRY_OPTION_PREFIX = 'JOIN_FORM_PENDING_CRM_'; + // After this many failed attempts the record is parked for a human rather + // than retried forever. It is never deleted: parking keeps the member's + // submitted details, which exist nowhere else. + public const CRM_RETRY_MAX_ATTEMPTS = 12; + // According to error messages from Chargebee, dates should be sent as the format yyyy-MM-dd. // Meaning 2021-12-25 for Christmas Day, 25th of December 2021. private static function formatDob($day, $month, $year) @@ -53,8 +58,13 @@ private static function crmRetryKey($data) * This record is the only copy of what the member submitted — the join * form data is not otherwise persisted — so losing it means their details * can only be recovered by asking them again. + * + * @param array $data The join payload. + * @param array|null $services Service keys still outstanding, or null for + * "every configured CRM", used when we do not + * know how far the original join got. */ - public static function queueCrmRetry($data, $service, $reason) + public static function queueCrmRetry($data, $services, $reason) { global $joinBlockLog; @@ -69,14 +79,80 @@ public static function queueCrmRetry($data, $service, $reason) update_option($optionName, wp_json_encode([ 'data' => $data, - 'service' => $service, + 'services' => $services, 'reason' => $reason, 'attempts' => $attempts, 'lastAttemptAt' => time(), ])); $email = $data['email'] ?? 'unknown'; - $joinBlockLog->error("Queued $service CRM retry for $email (attempt $attempts): $reason"); + $label = $services ? implode(', ', $services) : 'all configured CRMs'; + $joinBlockLog->error("Queued CRM retry for $email ($label, attempt $attempts): $reason"); + } + + /** + * Push a member to every configured CRM, returning the service keys that + * failed. + * + * Contains no payment code at all — no subscription creation, cancellation + * or Chargebee/GoCardless/Auth0 calls. The retry worker depends on that + * structurally: replaying a CRM push must never be able to touch a payment + * that is working fine, and from here it cannot reach the code that would. + * + * Each service is independent — one failing must not stop the others. + * + * @param array $data The join payload. + * @param array|null $only Restrict to these service keys, or null for every + * configured service. + * @return string[] Service keys that failed. + */ + public static function pushToCrms($data, $only = null) + { + global $joinBlockLog; + + $email = $data['email'] ?? 'unknown'; + $wanted = function ($service) use ($only) { + return $only === null || in_array($service, $only, true); + }; + $failed = []; + + if (Settings::get("USE_MAILCHIMP") && $wanted('mailchimp')) { + $joinBlockLog->info("Processing Mailchimp signup request for $email"); + try { + MailchimpService::signup($data); + $joinBlockLog->info("Completed Mailchimp signup request for $email"); + } catch (\Exception $exception) { + $joinBlockLog->error("Mailchimp error for email $email: " . $exception->getMessage()); + $failed[] = 'mailchimp'; + } + } + + if (Settings::get("USE_ACTION_NETWORK") && $wanted('action_network')) { + $joinBlockLog->info("Processing Action Network signup request for $email"); + try { + ActionNetworkService::signup($data); + $joinBlockLog->info("Completed Action Network signup request for $email"); + } catch (\Exception $exception) { + $joinBlockLog->error( + "Action Network error for email $email after successful payment — money taken with no CRM" + . " record yet: " . $exception->getMessage() + ); + $failed[] = 'action_network'; + } + } + + if (Settings::get("USE_ZETKIN") && $wanted('zetkin')) { + $joinBlockLog->info("Processing Zetkin signup request for $email"); + try { + ZetkinService::signup($data); + $joinBlockLog->info("Completed Zetkin signup request for $email"); + } catch (\Exception $exception) { + $joinBlockLog->error("Zetkin error for email $email: " . $exception->getMessage()); + $failed[] = 'zetkin'; + } + } + + return $failed; } /** @@ -347,11 +423,20 @@ private static function tryHandleJoin($data) throw new JoinBlockException("Invalid subscription amount", 8); } - $subscriptionInfo = StripeService::cancelPreviousSubscriptions( - $email, - $data["stripeCustomerId"] ?? null, - $subscriptionId - ); + $customerId = StripeService::resolveCustomerId($email, $data["stripeCustomerId"] ?? null); + + // A recovery replay runs on data saved hours or days earlier, by + // which time the member may have joined again. Cancelling + // "previous" subscriptions from that stale view would cancel the + // live one. De-duplication belongs to the live flow, which knows + // the current subscription; a replay only ever reads. + if (empty($data['isRecoveryReplay'])) { + StripeService::cancelPreviousSubscriptions($email, $customerId, $subscriptionId); + } else { + $joinBlockLog->info("Recovery replay for $email: leaving existing subscriptions untouched"); + } + + $subscriptionInfo = StripeService::getSubscriptionDates($email, $customerId); $data["stripeFirstSubscriptionDate"] = $subscriptionInfo["firstSubscription"]; $data["stripeFirstPaymentDate"] = $subscriptionInfo["firstPayment"]; $data["stripeLastPaymentDate"] = $subscriptionInfo["lastPayment"]; @@ -377,53 +462,13 @@ private static function tryHandleJoin($data) } } - if (Settings::get("USE_MAILCHIMP")) { - $email = $data['email']; - $joinBlockLog->info("Processing Mailchimp signup request for $email"); - try { - MailchimpService::signup($data); - $joinBlockLog->info("Completed Mailchimp signup request for $email"); - } catch (\Exception $exception) { - // A Mailchimp failure should not block a successful join. - // The member record can be retro-added to Mailchimp once the underlying issue is resolved. - $joinBlockLog->error("Mailchimp error for email $email: " . $exception->getMessage()); - } - } - - if (Settings::get("USE_ACTION_NETWORK")) { - $email = $data['email']; - $joinBlockLog->info("Processing Action Network signup request for $email"); - try { - ActionNetworkService::signup($data); - $joinBlockLog->info("Completed Action Network signup request for $email"); - } catch (\Exception $exception) { - // The payment has already succeeded, so the member has joined - // and there is nothing they can do about a CRM failure. Failing - // the request would show them an error they cannot act on, and - // invite a resubmit — which creates a duplicate subscription. - // Record the outstanding push instead, and retry it out of band. - $joinBlockLog->error( - "Action Network error for email $email after successful payment — money taken with no CRM" - . " record yet: " . $exception->getMessage() - ); - self::queueCrmRetry($data, 'action_network', $exception->getMessage()); - } - } - - if (Settings::get("USE_ZETKIN")) { - $email = $data['email']; - $joinBlockLog->info("Processing Zetkin signup request for $email"); - try { - ZetkinService::signup($data); - $joinBlockLog->info("Completed Zetkin signup request for $email"); - } catch (\Exception $exception) { - // Non-blocking: Zetkin is a secondary integration. The Stripe payment is - // the essential step and has already completed by this point. A Zetkin - // failure (e.g. expired credentials) should not surface an error to the - // member — they have successfully joined. The member record can be - // retro-added to Zetkin once the underlying issue is resolved. - $joinBlockLog->error("Zetkin error for email $email: " . $exception->getMessage()); - } + // The payment has already succeeded by this point, so a CRM failure is + // not something the member can act on. Record what is outstanding and + // let ensureCrmPushesCompleted() finish it, rather than showing them an + // error and inviting the resubmit that creates a duplicate subscription. + $failedCrms = self::pushToCrms($data); + if ($failedCrms) { + self::queueCrmRetry($data, $failedCrms, 'push failed during join'); } $webhookUuid = $data['webhookUuid'] ?? ''; @@ -566,6 +611,8 @@ public static function ensureStripeSubscriptionsCreated($subscriptionId = null) } if ($subscription && in_array($subscription->status, ['active', 'trialing'])) { + // Replay: complete the join without touching payment state. + $data['isRecoveryReplay'] = true; self::handleJoin($data); delete_option($name); $joinBlockLog->info("ensureStripeSubscriptionsCreated: success, deleting option {$name}"); @@ -604,9 +651,10 @@ public static function ensureStripeSubscriptionsCreated($subscriptionId = null) // Do not discard. This is the only record of what the // member submitted, so dropping it makes the join // unrecoverable without asking them for it again. + // Services unknown: the join never ran, so retry them all. self::queueCrmRetry( $data, - 'action_network', + null, 'payment collected without a completed join' ); } else { @@ -625,6 +673,155 @@ public static function ensureStripeSubscriptionsCreated($subscriptionId = null) } } + /** + * Seconds to wait before the next attempt: 1h, 2h, 4h ... capped at 24h. + * + * A CRM that is down is usually down for everyone, so retrying every queued + * member hourly helps nobody and buries the useful log lines. + */ + public static function crmRetryBackoffSeconds($attempts) + { + $hours = min(2 ** max(0, (int) $attempts - 1), 24); + return $hours * 3600; + } + + public static function crmRetryIsDue($record, $now = null) + { + $now = $now ?? time(); + $lastAttemptAt = (int) ($record['lastAttemptAt'] ?? 0); + $backoff = self::crmRetryBackoffSeconds($record['attempts'] ?? 0); + return ($now - $lastAttemptAt) >= $backoff; + } + + /** + * Read-only check that the member's payment still stands. + * + * Deliberately never mutates. This runs long after the join, by which time + * the member may have re-joined or changed plan, so altering a subscription + * from this stale data would damage a payment that is working fine. + * + * @return bool|null true = paid, false = no payment, null = could not tell. + */ + private static function paymentStillStands($data) + { + $subscriptionId = $data['stripeSubscriptionId'] ?? ''; + if (!$subscriptionId) { + // A GoCardless or Chargebee join: there is no Stripe subscription to + // check, and the record only exists because a payment succeeded. + return true; + } + + StripeService::initialise(); + return StripeService::subscriptionWasPaid($subscriptionId); + } + + /** + * Retry outstanding CRM pushes for members whose payment already succeeded. + * + * Complements reconcileRecentMemberships(), which can detect a member who + * paid but never reached the CRM yet has no copy of their submitted details + * to fix it with. This worker holds that copy. + * + * Touches no payment state: it reads Stripe to confirm the payment stands, + * then only writes to CRMs. + */ + public static function ensureCrmPushesCompleted() + { + global $wpdb; + global $joinBlockLog; + + $joinBlockLog->info("Running ensureCrmPushesCompleted"); + + $results = $wpdb->get_results( + $wpdb->prepare( + "SELECT * FROM {$wpdb->prefix}options WHERE option_name LIKE %s", + self::CRM_RETRY_OPTION_PREFIX . '%' + ) + ); + + foreach ($results as $result) { + $name = $result->option_name; + try { + $record = json_decode($result->option_value, true); + $data = $record['data'] ?? null; + $email = $data['email'] ?? ''; + + if (!$data || !$email) { + $joinBlockLog->error("ensureCrmPushesCompleted: $name holds no usable join data, skipping"); + continue; + } + + if (!empty($record['parked'])) { + continue; + } + + $attempts = (int) ($record['attempts'] ?? 0); + if ($attempts >= self::CRM_RETRY_MAX_ATTEMPTS) { + $record['parked'] = true; + update_option($name, wp_json_encode($record)); + $joinBlockLog->error( + "ensureCrmPushesCompleted: giving up on $email after $attempts attempts. Their submitted" + . " details are preserved in the $name option — complete the CRM record manually." + ); + continue; + } + + if (!self::crmRetryIsDue($record)) { + continue; + } + + $paymentStands = self::paymentStillStands($data); + if ($paymentStands === null) { + // Could not reach Stripe. Leave the attempt count alone so a + // provider outage does not burn through the retry budget. + $joinBlockLog->warning( + "ensureCrmPushesCompleted: could not confirm payment for $email, will try again later" + ); + continue; + } + if ($paymentStands === false) { + $record['parked'] = true; + update_option($name, wp_json_encode($record)); + $joinBlockLog->error( + "ensureCrmPushesCompleted: $email has no surviving payment, so has not been added to the" + . " CRM. Parked in $name — refund or complete manually." + ); + continue; + } + + // Same lock the /join endpoint and the webhooks take, so a retry + // cannot interleave with a live write for the same person. + $lockFile = self::acquireLock($email); + try { + $failed = self::pushToCrms($data, $record['services'] ?? null); + } finally { + self::releaseLock($lockFile); + } + + if (!$failed) { + delete_option($name); + // Deliberately does not fire ck_join_flow_success: handleJoin() + // now completes even when a CRM push fails, so the hook has + // already fired for this member and the heartbeat digest would + // count them twice. + $joinBlockLog->info("ensureCrmPushesCompleted: completed CRM push for $email, cleared $name"); + continue; + } + + $record['services'] = $failed; + $record['attempts'] = $attempts + 1; + $record['lastAttemptAt'] = time(); + update_option($name, wp_json_encode($record)); + $joinBlockLog->warning( + "ensureCrmPushesCompleted: still outstanding for $email (" . implode(', ', $failed) . ")," + . " attempt " . ($attempts + 1) . " of " . self::CRM_RETRY_MAX_ATTEMPTS + ); + } catch (\Exception $e) { + $joinBlockLog->error("ensureCrmPushesCompleted: could not process $name: " . $e->getMessage()); + } + } + } + public static function shouldLapseMember($email, $context = [], $default = true) { return (bool) apply_filters('ck_join_flow_should_lapse_member', $default, $email, $context); diff --git a/packages/join-block/src/Services/StripeService.php b/packages/join-block/src/Services/StripeService.php index 8859ccc..bc1a15c 100644 --- a/packages/join-block/src/Services/StripeService.php +++ b/packages/join-block/src/Services/StripeService.php @@ -631,33 +631,83 @@ public static function getSubscriptionAmount($subscriptionId) } /** - * Cancel every *other* active-ish subscription for the customer, and report - * the member's subscription/payment dates. $subscriptionId is the one just - * created by the browser Stripe client during this join flow, and is left - * in place. + * Whether a subscription represents a payment that actually went through. * - * Deliberately does not report the current subscription's amount — see - * getSubscriptionAmount(). Date collection is best-effort: the catch below - * keeps a transient Stripe error from failing an otherwise good join, and - * the dates are only used to populate CRM custom fields. + * Strictly read-only — used by the CRM retry worker to confirm a member + * really paid before writing them to a CRM, long after the join. Nothing + * here may mutate the subscription: by now it may be a live membership that + * has moved on from the data the retry record was written with. + * + * @return bool|null true = paid, false = not paid, null = could not tell + * (transient failure — the caller should try again later + * rather than conclude anything). */ - public static function cancelPreviousSubscriptions($email, $customerId, $subscriptionId) + public static function subscriptionWasPaid($subscriptionId) { global $joinBlockLog; - $joinBlockLog->info("Removing previous subscriptions for user " . $email . ", customer: " . $customerId); + try { + $subscription = Subscription::retrieve($subscriptionId); + } catch (\Stripe\Exception\InvalidRequestException $e) { + // The subscription does not exist; a definite answer, not an outage. + $joinBlockLog->warning("subscriptionWasPaid($subscriptionId): not found — " . $e->getMessage()); + return false; + } catch (\Exception $e) { + $joinBlockLog->warning("subscriptionWasPaid($subscriptionId): could not check — " . $e->getMessage()); + return null; + } + + if (in_array($subscription->status, ['active', 'trialing'], true)) { + return true; + } + + // A cancelled subscription still counts if money changed hands: the + // member paid, so they belong in the CRM regardless of its state now. + if (empty($subscription->latest_invoice)) { + return false; + } + + try { + $invoice = \Stripe\Invoice::retrieve($subscription->latest_invoice); + } catch (\Exception $e) { + $joinBlockLog->warning( + "subscriptionWasPaid($subscriptionId): could not inspect invoice — " . $e->getMessage() + ); + return null; + } + + return $invoice->status === 'paid'; + } + + /** + * Resolve the member's Stripe customer, creating one if we do not have it. + */ + public static function resolveCustomerId($email, $customerId) + { + if ($customerId) { + return $customerId; + } + [$customer,] = self::upsertCustomer($email); + return $customer->id; + } + + /** + * Read-only: the member's earliest subscription date, and their first and + * last payment dates, for the CRM custom fields. + * + * Best-effort — a transient Stripe error must not fail an otherwise good + * join, so on failure the caller gets today's date and nulls rather than an + * exception. + */ + public static function getSubscriptionDates($email, $customerId) + { + global $joinBlockLog; $firstSubscriptionDate = date('Y-m-d'); $firstPayment = null; $lastPayment = null; - if (!$customerId) { - [$customer,] = self::upsertCustomer($email); - $customerId = $customer->id; - } - try { - // Fetch all subscriptions for date calculation $subscriptions = \Stripe\Subscription::all([ 'customer' => $customerId, 'status' => 'all', @@ -665,33 +715,12 @@ public static function cancelPreviousSubscriptions($email, $customerId, $subscri ]); foreach ($subscriptions->autoPagingIterator() as $sub) { - // Track earliest subscription date $createdDate = date('Y-m-d', $sub->created); - if (is_null($firstSubscriptionDate) || $createdDate < $firstSubscriptionDate) { + if ($createdDate < $firstSubscriptionDate) { $firstSubscriptionDate = $createdDate; } - - // Cancel and void if it's an "active-ish" subscription and not the current one - if ($sub->id !== $subscriptionId && in_array($sub->status, ['active', 'trialing', 'past_due'])) { - $joinBlockLog->info("Canceling subscription " . $sub->id . " for user " . $email); - $sub->cancel(); - - // Find and void open invoices for this subscription - $invoices = \Stripe\Invoice::all([ - 'customer' => $customerId, - 'subscription' => $sub->id, - 'status' => 'open', - 'limit' => 100, - ]); - - foreach ($invoices->autoPagingIterator() as $invoice) { - $joinBlockLog->info("Voiding invoice " . $invoice->id . " for canceled subscription " . $sub->id); - $invoice->voidInvoice(); - } - } } - // Fetch all paid invoices for first/last payment dates $paidInvoices = \Stripe\Invoice::all([ 'customer' => $customerId, 'status' => 'paid', @@ -708,7 +737,7 @@ public static function cancelPreviousSubscriptions($email, $customerId, $subscri } } } catch (\Exception $e) { - $joinBlockLog->error("Error removing subscriptions for user " . $email . ": " . $e->getMessage()); + $joinBlockLog->error("Error reading subscription dates for " . $email . ": " . $e->getMessage()); } return [ @@ -718,6 +747,61 @@ public static function cancelPreviousSubscriptions($email, $customerId, $subscri ]; } + /** + * Cancel every *other* active-ish subscription for the customer, removing + * the duplicates a member creates by submitting the payment step twice. + * $subscriptionId is the one to keep: the subscription the browser Stripe + * client just created. + * + * Only safe to call while $subscriptionId is genuinely the member's current + * subscription. Calling it from stale data — a recovery replay hours later, + * say — would cancel a live subscription the member has since created. See + * ensureStripeSubscriptionsCreated(), which deliberately does not. + */ + public static function cancelPreviousSubscriptions($email, $customerId, $subscriptionId) + { + global $joinBlockLog; + + $joinBlockLog->info("Removing previous subscriptions for user " . $email . ", customer: " . $customerId); + + try { + $subscriptions = \Stripe\Subscription::all([ + 'customer' => $customerId, + 'status' => 'all', + 'limit' => 100, + ]); + + foreach ($subscriptions->autoPagingIterator() as $sub) { + if ($sub->id === $subscriptionId) { + continue; + } + if (!in_array($sub->status, ['active', 'trialing', 'past_due'])) { + continue; + } + + $joinBlockLog->info("Canceling subscription " . $sub->id . " for user " . $email); + $sub->cancel(); + + // Find and void open invoices for this subscription + $invoices = \Stripe\Invoice::all([ + 'customer' => $customerId, + 'subscription' => $sub->id, + 'status' => 'open', + 'limit' => 100, + ]); + + foreach ($invoices->autoPagingIterator() as $invoice) { + $joinBlockLog->info( + "Voiding invoice " . $invoice->id . " for canceled subscription " . $sub->id + ); + $invoice->voidInvoice(); + } + } + } catch (\Exception $e) { + $joinBlockLog->error("Error removing subscriptions for user " . $email . ": " . $e->getMessage()); + } + } + public static function getSubscriptionHistory($customerId) { global $joinBlockLog; @@ -1379,6 +1463,19 @@ private static function reconcileCustomerMembership($customerId) return; } + // A person record on its own is not evidence the join completed: a + // newsletter or petition signup creates one from an email alone, + // and that record then masks the missing join. The join flow always + // collects a name, so its absence means the demographic push never + // ran. (Update-flow joins deliberately send no name, but those are + // existing members who already have one.) + if (empty($person['has_name'])) { + $joinBlockLog->error( + "reconcileRecentMemberships: $email has a paid, active Stripe subscription but no name in" + . " Action Network — the join's demographic push never completed. Investigate and backfill." + ); + } + if ($isLapsed) { $joinBlockLog->error( "reconcileRecentMemberships: $email has an active Stripe subscription but carries the" diff --git a/packages/join-block/tests/CrmRetryBackoffTest.php b/packages/join-block/tests/CrmRetryBackoffTest.php new file mode 100644 index 0000000..577c183 --- /dev/null +++ b/packages/join-block/tests/CrmRetryBackoffTest.php @@ -0,0 +1,95 @@ +assertSame( + $expectedHours * self::HOUR, + JoinService::crmRetryBackoffSeconds($attempts) + ); + } + + public function backoffSchedule(): array + { + return [ + 'never attempted' => [0, 1], + 'after 1 failure' => [1, 1], + 'after 2 failures' => [2, 2], + 'after 3 failures' => [3, 4], + 'after 4 failures' => [4, 8], + 'after 5 failures' => [5, 16], + 'capped at 24h' => [6, 24], + 'still capped' => [12, 24], + ]; + } + + /** + * A negative or nonsense attempt count must not produce a negative or zero + * delay, which would spin the worker. + */ + public function testMalformedAttemptCountStillYieldsAPositiveDelay(): void + { + $this->assertSame(self::HOUR, JoinService::crmRetryBackoffSeconds(-5)); + } + + // ------------------------------------------------------------------------- + // Due-ness + // ------------------------------------------------------------------------- + + public function testRecordIsDueOnceTheBackoffHasElapsed(): void + { + $now = 1_700_000_000; + $record = ['attempts' => 3, 'lastAttemptAt' => $now - (4 * self::HOUR)]; + + $this->assertTrue(JoinService::crmRetryIsDue($record, $now)); + } + + public function testRecordIsNotDueBeforeTheBackoffHasElapsed(): void + { + $now = 1_700_000_000; + $record = ['attempts' => 3, 'lastAttemptAt' => $now - (4 * self::HOUR) + 1]; + + $this->assertFalse(JoinService::crmRetryIsDue($record, $now)); + } + + /** + * A freshly queued record has no lastAttemptAt, and must be picked up on + * the next run rather than waiting out a backoff it never earned. + */ + public function testFreshlyQueuedRecordIsDueImmediately(): void + { + $this->assertTrue(JoinService::crmRetryIsDue([], 1_700_000_000)); + } + + public function testLongStalledRecordIsDue(): void + { + $now = 1_700_000_000; + $record = ['attempts' => 11, 'lastAttemptAt' => $now - (30 * 24 * self::HOUR)]; + + $this->assertTrue(JoinService::crmRetryIsDue($record, $now)); + } +} diff --git a/packages/join-block/tests/JoinServiceMailchimpTest.php b/packages/join-block/tests/JoinServiceMailchimpTest.php index f959b1b..43755bc 100644 --- a/packages/join-block/tests/JoinServiceMailchimpTest.php +++ b/packages/join-block/tests/JoinServiceMailchimpTest.php @@ -55,11 +55,16 @@ class JoinServiceMailchimpTest extends TestCase 'add_tags' => '', ]; + /** Options written during a test, so we can assert on the CRM retry record. */ + private array $savedOptions = []; + protected function setUp(): void { parent::setUp(); Monkey\setUp(); + $this->savedOptions = []; + // WordPress functions used in the join path. Monkey\Functions\when('wp_json_encode')->alias('json_encode'); Monkey\Functions\when('esc_html')->returnArg(); @@ -92,7 +97,19 @@ public function get_results(string $query, $output = null): array if ($key === 'ck_join_flow_membership_plan_standard') { return $this->membershipPlan; } - return false; + return $this->savedOptions[$key] ?? false; + }); + + Monkey\Functions\when('update_option') + ->alias(function (string $key, $value) { + $this->savedOptions[$key] = $value; + return true; + }); + + Monkey\Functions\when('delete_option') + ->alias(function (string $key) { + unset($this->savedOptions[$key]); + return true; }); // All plugin settings default to false/empty — no payment provider, @@ -156,6 +173,21 @@ public function testMailchimpFailureDoesNotBlockJoin(): void global $joinBlockLog; $this->assertNotEmpty($joinBlockLog->errors, 'Expected Mailchimp error to be logged'); $this->assertStringContainsString('Mailchimp', $joinBlockLog->errors[0]); + + // ...and the outstanding push recorded, so the retry worker can finish + // it. Previously the failure was swallowed and the member was simply + // never added to Mailchimp. + $queued = array_filter( + $this->savedOptions, + fn($key) => str_starts_with($key, JoinService::CRM_RETRY_OPTION_PREFIX), + ARRAY_FILTER_USE_KEY + ); + $this->assertCount(1, $queued, 'Expected the failed Mailchimp push to be queued for retry'); + + $record = json_decode((string) reset($queued), true); + $this->assertSame(['mailchimp'], $record['services']); + $this->assertSame($this->joinData['email'], $record['data']['email']); + $this->assertSame(1, $record['attempts']); } /** From 0de9f064075639663f8e3194bea3d03c555675bf Mon Sep 17 00:00:00 2001 From: Joaquim d'Souza Date: Mon, 20 Jul 2026 14:37:07 +0100 Subject: [PATCH 4/5] fix: harden CRM retry record keying, storage and parking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects in the retry queue, all found in review. crmRetryKey() could collide With no stripeSubscriptionId it hashed the email — but handleJoin() can be called without one (it falls back to sessionToken for its lock), so sha1('') gave every such record the same option name and each queued member silently overwrote the last. Since the record is the only copy of what the member submitted, that destroyed data. Now falls through subscription -> email -> sessionToken -> payload hash, treating blank as absent at each step rather than hashing it. Records were autoloaded Each carries the full join payload, and new options default to autoload=yes, so a backlog would be read into memory on every page request. Now written with autoload disabled, matching the existing CK_JOIN_FLOW_COMPLETED_JOINS convention. Applied to the recovery snapshots too, including the pre-existing Stripe, GoCardless and Chargebee unprocessed-request options, which have the same problem. Unusable records were re-logged forever A record with no decodable JSON or no email was logged and skipped, leaving it in place to be re-scanned and re-logged on every run. Such a record can never succeed — without an email there is no CRM to push to and no lock to take — so it is now parked. A decode failure keeps the raw value, which remains the only copy of whatever was written. Parking is consolidated into parkCrmRetry(), which records the reason and timestamp and never deletes. Every path through the worker now either progresses, parks, or skips for a bounded reason. Co-Authored-By: Claude Opus 4.8 --- packages/join-block/join.php | 23 ++- .../join-block/src/Services/JoinService.php | 101 +++++++++---- packages/join-block/tests/CrmRetryKeyTest.php | 133 ++++++++++++++++++ 3 files changed, 229 insertions(+), 28 deletions(-) create mode 100644 packages/join-block/tests/CrmRetryKeyTest.php diff --git a/packages/join-block/join.php b/packages/join-block/join.php index 44bec60..48fca72 100644 --- a/packages/join-block/join.php +++ b/packages/join-block/join.php @@ -396,7 +396,12 @@ // Make stage = "confirm" to match the normal flow // (the user is redirected back from GoCardless and submits their data to the /join endpoint) $data['stage'] = "confirm"; - update_option("JOIN_FORM_UNPROCESSED_GOCARDLESS_REQUEST_{$billingRequest['id']}", wp_json_encode($data)); + // autoload disabled: full join payload, see the Stripe equivalent below. + update_option( + "JOIN_FORM_UNPROCESSED_GOCARDLESS_REQUEST_{$billingRequest['id']}", + wp_json_encode($data), + false + ); return ["href" => $authLink, "gcBillingRequestId" => $billingRequest["id"]]; } @@ -458,7 +463,12 @@ // Make stage = "confirm" to match the normal flow // (the user is redirected back from ChargeBee and submits their data to the /join endpoint) $data['stage'] = "confirm"; - update_option("JOIN_FORM_UNPROCESSED_CHARGEBEE_REQUEST_{$hostedPageId}", wp_json_encode($data)); + // autoload disabled: full join payload, see the Stripe equivalent below. + update_option( + "JOIN_FORM_UNPROCESSED_CHARGEBEE_REQUEST_{$hostedPageId}", + wp_json_encode($data), + false + ); return ["href" => $hostedPageUrl, "cbHostedPageId" => $hostedPageId]; } catch (\Exception $e) { @@ -522,7 +532,14 @@ // Make stage = "confirm" to match the normal flow // (the user returns from the payment step and submits their data to the /join endpoint) $data['stage'] = "confirm"; - update_option("JOIN_FORM_UNPROCESSED_STRIPE_REQUEST_{$subscription->id}", wp_json_encode($data)); + // autoload disabled: this holds the full join payload, and a + // backlog of unprocessed requests would otherwise be read into + // memory on every page request. + update_option( + "JOIN_FORM_UNPROCESSED_STRIPE_REQUEST_{$subscription->id}", + wp_json_encode($data), + false + ); return $subscription; } catch (\Exception $e) { diff --git a/packages/join-block/src/Services/JoinService.php b/packages/join-block/src/Services/JoinService.php index 9497bc1..118dc0c 100644 --- a/packages/join-block/src/Services/JoinService.php +++ b/packages/join-block/src/Services/JoinService.php @@ -38,17 +38,35 @@ private static function formatDob($day, $month, $year) /** * Option name for a pending CRM push. Keyed on the Stripe subscription - * where there is one, falling back to a hash of the email so a GoCardless - * or Chargebee join still gets exactly one record per member. + * where there is one, falling back to the email so a GoCardless or + * Chargebee join still gets exactly one record per member. + * + * Each fallback must be non-empty before it is used. handleJoin() can be + * called without an email — it falls back to sessionToken for its lock — + * and hashing an empty string would give every such record the same option + * name, so each would silently overwrite the last. */ - private static function crmRetryKey($data) + public static function crmRetryKey($data) { - $subscriptionId = $data['stripeSubscriptionId'] ?? ''; - if ($subscriptionId) { + $subscriptionId = trim((string) ($data['stripeSubscriptionId'] ?? '')); + if ($subscriptionId !== '') { return self::CRM_RETRY_OPTION_PREFIX . $subscriptionId; } + $email = strtolower(trim((string) ($data['email'] ?? ''))); - return self::CRM_RETRY_OPTION_PREFIX . 'email_' . sha1($email); + if ($email !== '') { + return self::CRM_RETRY_OPTION_PREFIX . 'email_' . sha1($email); + } + + $sessionToken = trim((string) ($data['sessionToken'] ?? '')); + if ($sessionToken !== '') { + return self::CRM_RETRY_OPTION_PREFIX . 'session_' . sha1($sessionToken); + } + + // Last resort: the payload itself. Two identical payloads collapsing to + // one record is correct de-duplication; two different ones must not + // collide, which hashing a missing identifier would guarantee. + return self::CRM_RETRY_OPTION_PREFIX . 'payload_' . sha1((string) wp_json_encode($data)); } /** @@ -77,13 +95,16 @@ public static function queueCrmRetry($data, $services, $reason) $attempts = (int) ($decoded['attempts'] ?? 0) + 1; } + // autoload disabled: each record carries the full join payload, and a + // backlog of them would otherwise be read into memory on every single + // page request. update_option($optionName, wp_json_encode([ 'data' => $data, 'services' => $services, 'reason' => $reason, 'attempts' => $attempts, 'lastAttemptAt' => time(), - ])); + ]), false); $email = $data['email'] ?? 'unknown'; $label = $services ? implode(', ', $services) : 'all configured CRMs'; @@ -185,7 +206,8 @@ private static function saveRecoverySnapshot($data) $data['createdAt'] = time(); } - update_option($optionName, wp_json_encode($data)); + // autoload disabled — see queueCrmRetry(): full join payload. + update_option($optionName, wp_json_encode($data), false); } public static function handleJoin($data) @@ -715,6 +737,30 @@ private static function paymentStillStands($data) return StripeService::subscriptionWasPaid($subscriptionId); } + /** + * Stop retrying a record, without ever deleting it. + * + * Parking is how the worker makes progress on something it cannot finish. + * The record is the only copy of what the member submitted, so it stays for + * a human to pick up — but it is not re-attempted, and not re-logged on + * every subsequent run. + */ + private static function parkCrmRetry($optionName, $record, $reason) + { + global $joinBlockLog; + + $record['parked'] = true; + $record['parkedReason'] = $reason; + $record['parkedAt'] = time(); + + update_option($optionName, wp_json_encode($record), false); + + $joinBlockLog->error( + "ensureCrmPushesCompleted: parked $optionName — $reason. The submitted details are preserved in" + . " that option; complete the CRM record manually." + ); + } + /** * Retry outstanding CRM pushes for members whose payment already succeeded. * @@ -743,11 +789,15 @@ public static function ensureCrmPushesCompleted() $name = $result->option_name; try { $record = json_decode($result->option_value, true); - $data = $record['data'] ?? null; - $email = $data['email'] ?? ''; - if (!$data || !$email) { - $joinBlockLog->error("ensureCrmPushesCompleted: $name holds no usable join data, skipping"); + if (!is_array($record)) { + // Keep the raw value: it is still the only copy of whatever + // was written here, and may be recoverable by hand. + self::parkCrmRetry( + $name, + ['raw' => $result->option_value], + 'record could not be decoded' + ); continue; } @@ -755,14 +805,20 @@ public static function ensureCrmPushesCompleted() continue; } + $data = $record['data'] ?? null; + $email = $data['email'] ?? ''; + + if (!$data || !$email) { + // Without an email there is no CRM to push to and no lock to + // take, so this can never succeed. Park it rather than + // re-reading and re-logging it on every run forever. + self::parkCrmRetry($name, $record, 'no usable join data (missing payload or email)'); + continue; + } + $attempts = (int) ($record['attempts'] ?? 0); if ($attempts >= self::CRM_RETRY_MAX_ATTEMPTS) { - $record['parked'] = true; - update_option($name, wp_json_encode($record)); - $joinBlockLog->error( - "ensureCrmPushesCompleted: giving up on $email after $attempts attempts. Their submitted" - . " details are preserved in the $name option — complete the CRM record manually." - ); + self::parkCrmRetry($name, $record, "gave up after $attempts attempts"); continue; } @@ -780,12 +836,7 @@ public static function ensureCrmPushesCompleted() continue; } if ($paymentStands === false) { - $record['parked'] = true; - update_option($name, wp_json_encode($record)); - $joinBlockLog->error( - "ensureCrmPushesCompleted: $email has no surviving payment, so has not been added to the" - . " CRM. Parked in $name — refund or complete manually." - ); + self::parkCrmRetry($name, $record, "no surviving payment for $email — refund or complete by hand"); continue; } @@ -811,7 +862,7 @@ public static function ensureCrmPushesCompleted() $record['services'] = $failed; $record['attempts'] = $attempts + 1; $record['lastAttemptAt'] = time(); - update_option($name, wp_json_encode($record)); + update_option($name, wp_json_encode($record), false); $joinBlockLog->warning( "ensureCrmPushesCompleted: still outstanding for $email (" . implode(', ', $failed) . ")," . " attempt " . ($attempts + 1) . " of " . self::CRM_RETRY_MAX_ATTEMPTS diff --git a/packages/join-block/tests/CrmRetryKeyTest.php b/packages/join-block/tests/CrmRetryKeyTest.php new file mode 100644 index 0000000..bcd6198 --- /dev/null +++ b/packages/join-block/tests/CrmRetryKeyTest.php @@ -0,0 +1,133 @@ +alias('json_encode'); + } + + protected function tearDown(): void + { + Monkey\tearDown(); + parent::tearDown(); + } + + // ------------------------------------------------------------------------- + // Preferred identifiers + // ------------------------------------------------------------------------- + + public function testPrefersTheStripeSubscriptionId(): void + { + $key = JoinService::crmRetryKey([ + 'stripeSubscriptionId' => 'sub_123', + 'email' => 'member@example.com', + ]); + + $this->assertSame(JoinService::CRM_RETRY_OPTION_PREFIX . 'sub_123', $key); + } + + public function testFallsBackToEmailWhenThereIsNoSubscription(): void + { + $withEmail = JoinService::crmRetryKey(['email' => 'member@example.com']); + $other = JoinService::crmRetryKey(['email' => 'someone-else@example.com']); + + $this->assertStringStartsWith(JoinService::CRM_RETRY_OPTION_PREFIX . 'email_', $withEmail); + $this->assertNotSame($withEmail, $other); + } + + public function testEmailIsNormalisedSoOneMemberGetsOneRecord(): void + { + $this->assertSame( + JoinService::crmRetryKey(['email' => 'member@example.com']), + JoinService::crmRetryKey(['email' => ' Member@Example.COM ']) + ); + } + + // ------------------------------------------------------------------------- + // Collision resistance — the reason the fallback chain exists + // ------------------------------------------------------------------------- + + /** + * handleJoin() can be called without an email (it falls back to + * sessionToken for its lock), so this is reachable. Hashing an empty string + * would give every such record the same option name, and each queued member + * would overwrite the last. + */ + public function testTwoRecordsWithNoSubscriptionOrEmailDoNotCollide(): void + { + $first = JoinService::crmRetryKey(['sessionToken' => 'session-aaa']); + $second = JoinService::crmRetryKey(['sessionToken' => 'session-bbb']); + + $this->assertNotSame($first, $second); + $this->assertStringStartsWith(JoinService::CRM_RETRY_OPTION_PREFIX . 'session_', $first); + } + + public function testBlankIdentifiersAreTreatedAsAbsentNotHashed(): void + { + $first = JoinService::crmRetryKey([ + 'stripeSubscriptionId' => '', + 'email' => ' ', + 'sessionToken' => 'session-aaa', + ]); + $second = JoinService::crmRetryKey([ + 'stripeSubscriptionId' => '', + 'email' => '', + 'sessionToken' => 'session-bbb', + ]); + + $this->assertNotSame($first, $second); + } + + /** + * With no identifier at all, distinct payloads must still get distinct + * records. Identical payloads collapsing to one is correct de-duplication. + */ + public function testWithNoIdentifiersAtAllDistinctPayloadsStillDiffer(): void + { + $first = JoinService::crmRetryKey(['firstName' => 'Ada']); + $second = JoinService::crmRetryKey(['firstName' => 'Grace']); + + $this->assertNotSame($first, $second); + $this->assertStringStartsWith(JoinService::CRM_RETRY_OPTION_PREFIX . 'payload_', $first); + $this->assertSame($first, JoinService::crmRetryKey(['firstName' => 'Ada'])); + } + + /** + * The degenerate case the fallback chain was added for: two entirely empty + * payloads are indistinguishable, but must not take a key that a real + * member's record could also land on. + */ + public function testEmptyPayloadDoesNotShareAKeyWithAnIdentifiedMember(): void + { + $empty = JoinService::crmRetryKey([]); + $identified = JoinService::crmRetryKey(['email' => 'member@example.com']); + + $this->assertNotSame($empty, $identified); + } + + public function testKeyFitsWithinTheWordPressOptionNameColumn(): void + { + $key = JoinService::crmRetryKey(['sessionToken' => str_repeat('x', 500)]); + + $this->assertLessThanOrEqual(191, strlen($key)); + } +} From a71a7dccd69f142309e77fc81efeb85066d8a911 Mon Sep 17 00:00:00 2001 From: Joaquim d'Souza Date: Mon, 20 Jul 2026 14:44:18 +0100 Subject: [PATCH 5/5] Bump version to 1.4.29 --- packages/join-block/join.php | 2 +- packages/join-block/readme.txt | 4 +++- packages/join-flow/src/index.tsx | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/join-block/join.php b/packages/join-block/join.php index 48fca72..d64e1fb 100644 --- a/packages/join-block/join.php +++ b/packages/join-block/join.php @@ -3,7 +3,7 @@ /** * Plugin Name: Common Knowledge Join Flow * Description: Common Knowledge join flow plugin. - * Version: 1.4.28 + * Version: 1.4.29 * Author: Common Knowledge * Text Domain: common-knowledge-join-flow * License: GPLv2 or later diff --git a/packages/join-block/readme.txt b/packages/join-block/readme.txt index 93bb149..0f41dfa 100644 --- a/packages/join-block/readme.txt +++ b/packages/join-block/readme.txt @@ -4,7 +4,7 @@ Tags: membership, subscription, join Contributors: commonknowledgecoop Requires at least: 5.4 Tested up to: 6.8 -Stable tag: 1.4.28 +Stable tag: 1.4.29 Requires PHP: 8.1 License: GPLv2 or later License URI: https://www.gnu.org/licenses/gpl-2.0.html @@ -107,6 +107,8 @@ Need help? Contact us at [hello@commonknowledge.coop](mailto:hello@commonknowled == Changelog == += 1.4.29 = +* Improve push-to-CRM robustness (add retry queue) = 1.4.28 = * Add incomplete stripe journey handling = 1.4.27 = diff --git a/packages/join-flow/src/index.tsx b/packages/join-flow/src/index.tsx index 2c32106..6e52694 100644 --- a/packages/join-flow/src/index.tsx +++ b/packages/join-flow/src/index.tsx @@ -24,7 +24,7 @@ const init = () => { const sentryDsn = getEnvStr("SENTRY_DSN") Sentry.init({ dsn: sentryDsn, - release: "1.4.28" + release: "1.4.29" }); if (getEnv('USE_CHARGEBEE')) {