Fix: free trial lost on abandoned WooCommerce checkout#360
Fix: free trial lost on abandoned WooCommerce checkout#360superdav42 wants to merge 6 commits intomainfrom
Conversation
has_trialed() was counting pending memberships (created before payment) as consumed trials. Any abandoned checkout permanently blocked future trials for the customer with no error message. Also prevent ghost "pending payment" popups from surfacing for pending/cancelled memberships in check_pending_payments() and render_pending_payments(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBackend membership checks now ignore memberships with status Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
inc/managers/class-payment-manager.php (1)
157-165: LGTM - Correctly skips abandoned checkouts when checking for pending payments popup.The guard clause prevents misleading popups for pending/cancelled memberships that represent abandoned checkouts. The comment clearly explains the rationale.
Consider using
Membership_Status::PENDINGandMembership_Status::CANCELLEDconstants for consistency with the enum pattern defined inclass-membership-status.php.,
♻️ Suggested change using constants
+use WP_Ultimo\Database\Memberships\Membership_Status; + // ... in check_pending_payments(): - if (in_array($membership->get_status(), ['pending', 'cancelled'], true)) { + if (in_array($membership->get_status(), [Membership_Status::PENDING, Membership_Status::CANCELLED], true)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inc/managers/class-payment-manager.php` around lines 157 - 165, Replace the string literals 'pending' and 'cancelled' in the guard inside class-payment-manager.php with the enum constants defined in class-membership-status.php; specifically update the membership status check that calls $membership->get_status() to compare against Membership_Status::PENDING and Membership_Status::CANCELLED so the code uses the canonical constants rather than raw strings for consistency.inc/models/class-customer.php (1)
417-417: Consider using theMembership_Statusconstant for consistency.The codebase defines status constants in
inc/database/memberships/class-membership-status.php. Using the constant improves maintainability and catches typos at static analysis time.♻️ Suggested change
+use WP_Ultimo\Database\Memberships\Membership_Status; + // ... in the query array: - 'status__not_in' => ['pending'], + 'status__not_in' => [Membership_Status::PENDING],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inc/models/class-customer.php` at line 417, Replace the hard-coded 'pending' string in the status exclusion array with the defined membership status constant to ensure consistency and static checking; specifically, update the array entry where 'status__not_in' => ['pending'] (in the class-customer.php diff) to use the Membership_Status constant (e.g., Membership_Status::PENDING) instead of the literal string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@inc/managers/class-payment-manager.php`:
- Around line 157-165: Replace the string literals 'pending' and 'cancelled' in
the guard inside class-payment-manager.php with the enum constants defined in
class-membership-status.php; specifically update the membership status check
that calls $membership->get_status() to compare against
Membership_Status::PENDING and Membership_Status::CANCELLED so the code uses the
canonical constants rather than raw strings for consistency.
In `@inc/models/class-customer.php`:
- Line 417: Replace the hard-coded 'pending' string in the status exclusion
array with the defined membership status constant to ensure consistency and
static checking; specifically, update the array entry where 'status__not_in' =>
['pending'] (in the class-customer.php diff) to use the Membership_Status
constant (e.g., Membership_Status::PENDING) instead of the literal string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5ac384c-20c5-423f-a178-278c4e956717
📒 Files selected for processing (2)
inc/managers/class-payment-manager.phpinc/models/class-customer.php
After 3 consecutive 'stopped' responses (9s), auto-reload the page. The site was already created but the JS never reloaded because the 'stopped' state fell through to an infinite poll loop with no exit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
assets/js/thank-you.js (1)
128-150: Add error handling to prevent polling from stopping on network failures.If the fetch request fails (network error, server timeout), the unhandled rejection will halt polling entirely, leaving users stuck on the page with no recovery path. Consider wrapping the fetch in a try/catch to continue polling on transient failures.
🛡️ Proposed fix to add error handling
async check_site_created() { const url = new URL(wu_thank_you.ajaxurl); url.searchParams.set("action", "wu_check_pending_site_created"); url.searchParams.set("membership_hash", wu_thank_you.membership_hash); - const response = await fetch(url).then((request) => request.json()); - if (response.publish_status === "completed") { + let response; + try { + response = await fetch(url).then((request) => request.json()); + } catch (error) { + // Network error - retry polling after delay + setTimeout(this.check_site_created, 3e3); + return; + } + if (response?.publish_status === "completed") { window.location.reload(); - } else if (response.publish_status === "running") { + } else if (response?.publish_status === "running") { this.creating = true; this.stopped_count = 0; setTimeout(this.check_site_created, 3e3); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/thank-you.js` around lines 128 - 150, Wrap the fetch+json call in check_site_created in a try/catch so network/server errors don't stop polling: inside async check_site_created() try to await fetch(url).then(r=>r.json()), and on catch set this.creating = false, increment this.stopped_count (or reset/log as appropriate), call setTimeout(this.check_site_created, 3000) to continue polling, and optionally console.error the exception; ensure you still handle the normal publish_status branches when the try succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@assets/js/thank-you.js`:
- Around line 128-150: Wrap the fetch+json call in check_site_created in a
try/catch so network/server errors don't stop polling: inside async
check_site_created() try to await fetch(url).then(r=>r.json()), and on catch set
this.creating = false, increment this.stopped_count (or reset/log as
appropriate), call setTimeout(this.check_site_created, 3000) to continue
polling, and optionally console.error the exception; ensure you still handle the
normal publish_status branches when the try succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06a92133-19ae-425b-9aa0-dd8dbfcf5066
⛔ Files ignored due to path filters (1)
assets/js/thank-you.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (1)
assets/js/thank-you.js
Adds 8 tests covering the two bugs fixed in the prior commit:
Customer_Has_Trialed_Test (5 tests):
- Pending membership with trial must NOT block future trial (the bug)
- Active membership counts as trialed (happy path)
- Trialing membership counts as trialed
- Cancelled membership after genuine trial still blocks a second trial
(ensures fix is scoped to pending only, not cancelled)
- Cleaned-up orphan (cancelled + date_trial_end cleared) does not block
Payment_Manager_Pending_Popup_Test (3 tests):
- Pending membership must not trigger the payment popup
- Cancelled membership must not trigger the payment popup
- Active membership with pending payment must trigger the popup
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/WP_Ultimo/Managers/Payment_Manager_Pending_Popup_Test.php (1)
18-18: Consider adding tests for remaining membership statuses (optional).The
Membership_Statusenum defines six statuses:pending,active,trialing,expired,on-hold,cancelled. Currently tests cover three (pending,cancelled,active). For completeness,trialing,expired, andon-holdcould also be tested to confirm they trigger the popup (same asactive).This is low priority since the implementation is a simple
in_array()check and the boundary cases are covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/WP_Ultimo/Managers/Payment_Manager_Pending_Popup_Test.php` at line 18, Add tests to Payment_Manager_Pending_Popup_Test to cover the remaining Membership_Status values (`trialing`, `expired`, `on-hold`) by mirroring the existing test patterns used for `pending`, `cancelled`, and `active`; for each new status create a test method that sets the membership status to Membership_Status::TRIALING / EXPIRED / ON_HOLD (matching the enum names) and asserts the popup behavior (same assertions used for the `active` case), ensuring the class Payment_Manager_Pending_Popup_Test exercises the same code path as the other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/WP_Ultimo/Managers/Payment_Manager_Pending_Popup_Test.php`:
- Line 18: Add tests to Payment_Manager_Pending_Popup_Test to cover the
remaining Membership_Status values (`trialing`, `expired`, `on-hold`) by
mirroring the existing test patterns used for `pending`, `cancelled`, and
`active`; for each new status create a test method that sets the membership
status to Membership_Status::TRIALING / EXPIRED / ON_HOLD (matching the enum
names) and asserts the popup behavior (same assertions used for the `active`
case), ensuring the class Payment_Manager_Pending_Popup_Test exercises the same
code path as the other tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16218678-0afc-40d0-818d-f5ee4fb7da34
📒 Files selected for processing (2)
tests/WP_Ultimo/Managers/Payment_Manager_Pending_Popup_Test.phptests/WP_Ultimo/Models/Customer_Has_Trialed_Test.php
- Add file-level doc comments to both new test files (fixes PHPCS errors) - Add doc comments to lifecycle methods set_up_before_class, setUp, tearDown, tear_down_after_class (fixes PHPCS errors) - Add pricing_type, duration, duration_unit to wu_create_product() calls in Payment_Manager_Pending_Popup_Test so product creation succeeds instead of returning WP_Error (fixes PHPUnit failures) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/WP_Ultimo/Models/Customer_Has_Trialed_Test.php (1)
23-24: Add per-test cleanup for the shared customer.This class reuses one static customer across all methods, but
setUp()only clears the trial meta. If any assertion fails before the inline$membership->delete(), leftover memberships can leak into the next case and changehas_trialed(). AtearDown()that purges the test customer's memberships/payments would keep the cases isolated.Also applies to: 61-66, 246-251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/WP_Ultimo/Models/Customer_Has_Trialed_Test.php` around lines 23 - 24, The test class Customer_Has_Trialed_Test reuses a static Customer ($customer) but only clears trial meta in setUp(), so leftover memberships/payments from failed assertions can leak between tests; add a tearDown() method that, for the shared Customer, purges all memberships/subscriptions and test payments and resets trial-related meta (the same cleanup currently done inline where membership->delete() is used) to guarantee isolation, and apply the same per-test cleanup to the other test blocks in this file that reuse the static customer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/WP_Ultimo/Managers/Payment_Manager_Pending_Popup_Test.php`:
- Around line 85-93: Capture and assert the result of wu_create_payment() before
proceeding: assign its return to a variable (e.g., $payment =
wu_create_payment(...)) and add an assertion that the fixture was created (e.g.,
$this->assertNotNull($payment) or assertInstanceOf(WU_Payment::class, $payment))
prior to calling check_pending_payments() and asserting the popup flag; repeat
the same guard for the other similar fixtures referenced in the file (the blocks
around the other wu_create_payment() calls).
- Around line 14-21: Add a unit test in
tests/WP_Ultimo/Managers/Payment_Manager_Pending_Popup_Test.php that exercises
render_pending_payments() (not just check_pending_payments()) using the same
fixtures for pending and cancelled memberships; call
Payment_Manager::render_pending_payments() and assert that the popup markup is
NOT rendered (or that the specific pending-payment indicator is absent) for
those statuses so the status guard added in
class-payment-manager.php::render_pending_payments is covered and cannot regress
independently of check_pending_payments().
In `@tests/WP_Ultimo/Models/Customer_Has_Trialed_Test.php`:
- Around line 96-100: The $wpdb->update() call in
test_pending_membership_with_trial_does_not_block_future_trial() (and the
similar calls around the other mentioned ranges) may silently fail and
invalidate the test; modify the test to assert the update succeeded by checking
the return value from $wpdb->update() equals 1 immediately after each update
call (e.g. capture the result of the $wpdb->update(...) call and assertSame(1,
$result) or assertEquals(1, $result)), so the fixture mutation is verified
before proceeding with assertions that depend on that mutated state.
---
Nitpick comments:
In `@tests/WP_Ultimo/Models/Customer_Has_Trialed_Test.php`:
- Around line 23-24: The test class Customer_Has_Trialed_Test reuses a static
Customer ($customer) but only clears trial meta in setUp(), so leftover
memberships/payments from failed assertions can leak between tests; add a
tearDown() method that, for the shared Customer, purges all
memberships/subscriptions and test payments and resets trial-related meta (the
same cleanup currently done inline where membership->delete() is used) to
guarantee isolation, and apply the same per-test cleanup to the other test
blocks in this file that reuse the static customer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 321d887e-4322-4e0c-a4b4-939ed4954654
📒 Files selected for processing (2)
tests/WP_Ultimo/Managers/Payment_Manager_Pending_Popup_Test.phptests/WP_Ultimo/Models/Customer_Has_Trialed_Test.php
| * Regression tests for Payment_Manager::check_pending_payments() and | ||
| * render_pending_payments(). | ||
| * | ||
| * Ensures that pending and cancelled memberships (from abandoned checkouts) | ||
| * do not trigger the "pending payment" popup on user login, which previously | ||
| * pointed users at WC orders that may no longer exist. | ||
| * | ||
| * @see https://github.com/Ultimate-Multisite/ultimate-multisite/pull/360 |
There was a problem hiding this comment.
render_pending_payments() is still untested.
All three methods only exercise check_pending_payments(), but the production fix also added the same status guard in inc/managers/class-payment-manager.php:232-309. Without a render assertion, that second half of the fix can regress independently.
Also applies to: 60-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/WP_Ultimo/Managers/Payment_Manager_Pending_Popup_Test.php` around lines
14 - 21, Add a unit test in
tests/WP_Ultimo/Managers/Payment_Manager_Pending_Popup_Test.php that exercises
render_pending_payments() (not just check_pending_payments()) using the same
fixtures for pending and cancelled memberships; call
Payment_Manager::render_pending_payments() and assert that the popup markup is
NOT rendered (or that the specific pending-payment indicator is absent) for
those statuses so the status guard added in
class-payment-manager.php::render_pending_payments is covered and cannot regress
independently of check_pending_payments().
| wu_create_payment( | ||
| [ | ||
| 'customer_id' => $this->customer->get_id(), | ||
| 'membership_id' => $membership->get_id(), | ||
| 'status' => 'pending', | ||
| 'total' => 50.00, | ||
| 'gateway' => 'woocommerce', | ||
| ] | ||
| ); |
There was a problem hiding this comment.
Assert the pending payment fixture was created.
The pending/cancelled cases pass trivially if wu_create_payment() fails, because check_pending_payments() then sees no pending payment at all. Capture the return value and assert the fixture exists before checking the popup flag.
Suggested guard
- wu_create_payment(
+ $payment = wu_create_payment(
[
'customer_id' => $this->customer->get_id(),
'membership_id' => $membership->get_id(),
'status' => 'pending',
'total' => 50.00,
'gateway' => 'woocommerce',
]
);
+ $this->assertNotEmpty($payment, 'Failed to create the pending payment fixture.');Also applies to: 134-142, 185-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/WP_Ultimo/Managers/Payment_Manager_Pending_Popup_Test.php` around lines
85 - 93, Capture and assert the result of wu_create_payment() before proceeding:
assign its return to a variable (e.g., $payment = wu_create_payment(...)) and
add an assertion that the fixture was created (e.g.,
$this->assertNotNull($payment) or assertInstanceOf(WU_Payment::class, $payment))
prior to calling check_pending_payments() and asserting the popup flag; repeat
the same guard for the other similar fixtures referenced in the file (the blocks
around the other wu_create_payment() calls).
| $wpdb->update( | ||
| $wpdb->prefix . 'wu_memberships', | ||
| ['date_trial_end' => gmdate('Y-m-d H:i:s', strtotime('+14 days'))], | ||
| ['id' => $membership->get_id()] | ||
| ); |
There was a problem hiding this comment.
Assert the fixture mutation succeeds.
test_pending_membership_with_trial_does_not_block_future_trial() can pass even if this write fails, because a plain pending membership already makes has_trialed() return false. The same silent setup failure would also make the positive cases fail for the wrong reason. Please assert that each $wpdb->update() returns 1 so the regression is exercising the intended state.
Suggested guard
- $wpdb->update(
+ $updated = $wpdb->update(
$wpdb->prefix . 'wu_memberships',
['date_trial_end' => gmdate('Y-m-d H:i:s', strtotime('+14 days'))],
['id' => $membership->get_id()]
);
+ $this->assertSame(1, $updated, 'Failed to seed date_trial_end for this scenario.');Also applies to: 129-133, 161-165, 198-202
🧰 Tools
🪛 GitHub Check: Code Quality Checks
[warning] 96-96:
Direct database call without caching detected. Consider using wp_cache_get() / wp_cache_set() or wp_cache_delete().
[warning] 96-96:
Use of a direct database call is discouraged.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/WP_Ultimo/Models/Customer_Has_Trialed_Test.php` around lines 96 - 100,
The $wpdb->update() call in
test_pending_membership_with_trial_does_not_block_future_trial() (and the
similar calls around the other mentioned ranges) may silently fail and
invalidate the test; modify the test to assert the update succeeded by checking
the return value from $wpdb->update() equals 1 immediately after each update
call (e.g. capture the result of the $wpdb->update(...) call and assertSame(1,
$result) or assertEquals(1, $result)), so the fixture mutation is verified
before proceeding with assertions that depend on that mutated state.
Add WordPress.DB.DirectDatabaseQuery exclusion for /tests/ in .phpcs.xml.dist so that legitimate direct DB access in test setup/teardown does not cause the Code Quality CI check to fail. Same pattern as the existing WordPress.Files.FileName exclusion for tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
- Add excludePaths: [./tests] to phpstan.neon.dist so PHPStan does not attempt to analyse test files (which require WP test env stubs not available during CI static analysis) - Add a shell fallback in code-quality.yml: if phpstan-report.xml is empty (all changed files were excluded), write a valid empty checkstyle XML so the annotation action can parse it without failing Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
1 participant
Problem
When a user starts checkout for a plan with a free trial and then abandons the page (closes tab, navigates away), they permanently lose their trial eligibility with no error message. On the next checkout attempt the full price is shown instead of $0 for today.
Root Causes Fixed
1.
has_trialed()counts pending memberships (class-customer.php)WP Ultimo creates the membership in
pendingstatus withdate_trial_endalready set before payment is collected. Without a status filter,has_trialed()finds this abandoned pending membership and permanently marks the customer as having trialed — even though they never paid.Fix: Added
'status__not_in' => ['pending']to thewu_get_memberships()query.cancelledis intentionally kept in scope — a user who genuinely started a trial and then cancelled their active membership should not receive a second trial.2. Ghost "pending payment" popup (
class-payment-manager.php)The abandoned checkout also leaves a pending membership with a pending WU payment. This triggers the "pending payment" popup on every subsequent login, pointing to a WC order that may no longer exist.
Fix: Both
check_pending_payments()andrender_pending_payments()now skippendingandcancelledmemberships.Related
The WooCommerce addon has a companion PR that fixes the remaining three root causes in
class-woocommerce-gateway.php(cancel all pending orders on new checkout, and hourly cleanup cancelling orphaned WU memberships + restoring trial meta).Testing
All 2883 unit tests pass. Specifically validated:
has_trialed()now only counts memberships with status outsidependingSummary by CodeRabbit
Bug Fixes
Improvements
Tests
Style