Skip to content

Resolve orphaned incomplete orders superseded by a newer checkout - #3715

Open
kimcoleman wants to merge 1 commit into
strangerstudios:devfrom
kimcoleman:change-order-status-pending-orders
Open

Resolve orphaned incomplete orders superseded by a newer checkout#3715
kimcoleman wants to merge 1 commit into
strangerstudios:devfrom
kimcoleman:change-order-status-pending-orders

Conversation

@kimcoleman

Copy link
Copy Markdown
Member

Problem

A member can be left with a stale pending order that never resolves:

  1. Member checks out with Pay by Check on a site that filters pmpro_check_status_after_checkout to pending (withholding access until the check arrives). This creates a pending order.
  2. Member changes their mind and checks out again with Stripe, which succeeds and creates a newer success order.
  3. The original check order stays pending forever — the account page shows "Your latest payment for this membership is past due..." even though they have paid.

Root cause

PMPro already cleans up incomplete orders: PMPro_Subscription::save() marks a subscription's token/pending/review orders as error when that subscription is cancelled, and a new checkout for the same level cancels the prior subscription.

But the cleanup keys off a subscription, and the orphaned pending order has none. A pending order receives a subscription_transaction_id at checkout, but the PMPro_Subscription row is only persisted when the membership is activated. While the order is pending, no subscription record exists — so there is nothing to cancel, and the existing cleanup never reaches the orphaned order.

Fix

On pmpro_after_checkout, mark the member's older incomplete (token/pending/review) orders as error when a newer order supersedes them.

  • Same level: re-checking out for the same level resolves the prior incomplete order.
  • Level groups: switching to another level in a group that only allows one level at a time deactivates sibling levels, so their incomplete orders are superseded too. The fix expands the level set to the whole group in that case, mirroring the group-deactivation logic in pmpro_changeMembershipLevel(). Multi-selection groups and ungrouped levels stay same-level-only, so a member who can legitimately hold sibling levels keeps those pending orders.

Guards:

  • Only orders older than the newly completed order are touched (id comparison).
  • Orders tied to a still-active subscription are left alone, so legitimate past-due renewals are untouched.

An order note is added explaining the status change, mirroring the existing cleanup.

Testing notes

  • Reproduce: create a pending check order for a level, then complete a successful Stripe checkout for the same level (or another level in a one-at-a-time group). The older pending order should flip to error with a note.
  • Confirm legitimate past-due renewals (older success + newer pending) are untouched.

Not covered here

This resolves the orphan on the member's next checkout. A pending order that is simply abandoned (never superseded) is out of scope — a cron sweep could be added separately to catch those.

🤖 Generated with Claude Code

A pending/token/review order receives a subscription_transaction_id at
checkout, but its PMPro_Subscription row is only persisted once the
membership is activated. If a member checks out again before that order
resolves, the cleanup in PMPro_Subscription::save() never runs (there is
no subscription to cancel), leaving the original order pending forever.

On pmpro_after_checkout, mark the member's older incomplete orders as
error when a newer order supersedes them. Covers re-checking out for the
same level and switching to another level in a group that only allows one
level at a time. Orders tied to a still-active subscription are left alone
so legitimate past-due renewals are untouched.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@flintfromthebasement flintfromthebasement left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR: #3715 — Resolve orphaned incomplete orders superseded by a newer checkout
kimcoleman → dev | 1 file, +48 -0 lines
#3715


Summary

Solid fix. The root cause analysis is correct and the implementation is tight. pmpro_error_orphaned_incomplete_orders() correctly fires on pmpro_after_checkout, scopes cleanup to the right level set (same level or the whole group for one-at-a-time groups), and guards against touching renewal orders that legitimately have an active subscription. Ready to merge.


Issues

  • Minor includes/checkout.phpMemberOrder::get_orders() defaults to a 100-row limit when no limit arg is passed. The call at the new function doesn't override it. For the described scenario (one or a few stale pending orders) this is a non-issue in practice, but passing 'limit' => 0 would remove the implicit cap and be more correct. From class.memberorder.php:577: $limit = isset( $args['limit'] ) ? (int) $args['limit'] : 100; — and a zero value skips the LIMIT clause entirely.

Looks Good

  • The 'success' !== $order->status early return is the right entry guard; the hook fires from both checkout.php and the IPN handlers, so this prevents the cleanup from running on non-final orders regardless of call site.
  • The subscription check on the incomplete order is correct. For a pending Pay-by-Check order with no subscription record, get_subscription() returns null (the order isn't success, so no subscription is materialized). The guard only preserves orders that have a real, active subscription row — exactly what "legitimate past-due renewal" means.
  • add_order_note() mutates $this->notes in memory; saveOrder() persists both the status change and the updated notes in the same UPDATE query. Sequencing is correct.
  • Group logic mirrors pmpro_changeMembershipLevel() faithfully: only expands $superseded_level_ids to the full group when allow_multiple_selections is falsy, leaving multi-selection groups and ungrouped levels as same-level-only.
  • The id comparison (>= $order->id) is the right anchor. An orphaned order is definitionally older; this guards against accidentally touching the newly completed order or any future orders from a race.

@dparker1005

Copy link
Copy Markdown
Member

Holding this back from 3.8.3. The intention behind this PR makes sense, but a core-wide sweep changing pending orders to error also catches Stripe delayed-notification payments (ACH, SEPA, Bacs), which sit at pending while the bank settles. Changing these orders to error could then silently hide a real payment that was collected if the checkout eventually completes.

I agree that we need a solution around checkouts that are initiated while other checkouts are already in progress, but we may need to consider solutions warning users before they start another checkout or solutions per-gateway since only each gateway knows if it can stop an in-progress pending payment.

@dparker1005 dparker1005 added this to the 3.9 milestone Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants