Adaptive pricing support check for products in the cart#5017
Adaptive pricing support check for products in the cart#5017
Conversation
|
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:
📝 WalkthroughWalkthroughAdds WC_Stripe_Helper::is_adaptive_pricing_supported(), exposes its result to the UPE gateway JS, and expands unit-test helpers (pre-orders, deposits, subscriptions), updates tests/bootstrap to load helpers, and adjusts checkout-context test filters; duplicate test blocks were introduced in one test file. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway as UPE_Gateway
participant Helper as WC_Stripe_Helper
participant Cart
participant Subs as WC_Subscriptions_Product
participant PreOrders as WC_Pre_Orders_Product
participant Deposits as WC_Deposits_Product_Manager
Client->>Gateway: request javascript params
Gateway->>Helper: is_adaptive_pricing_supported()
Helper->>Helper: check feature flag & adaptive_pricing setting
Helper->>Gateway: check woocommerce_is_checkout filter
Helper->>Cart: inspect cart contents
alt cart empty
Helper-->>Gateway: return true
else cart has items
Cart->>Subs: is_subscription(item)?
Cart->>PreOrders: product_is_charged_upon_release(item)?
Cart->>Deposits: deposits_enabled(item)?
alt any subscription / pre-order charged on release / deposit enabled
Helper-->>Gateway: return false
else
Helper-->>Gateway: return true
end
end
Gateway-->>Client: include isAdaptivePricingSupported in JS params
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@includes/class-wc-stripe-helper.php`:
- Around line 1108-1157: The test helper class WC_Subscriptions_Product defines
is_subscription() and get_trial_length() without parameters but production code
calls WC_Subscriptions_Product::is_subscription($product) and
::get_trial_length($product); update the test helper method signatures to accept
a $product parameter (e.g., function is_subscription( $product ) and function
get_trial_length( $product )) and forward or ignore the parameter in their
implementations so PHPUnit on PHP 8+ won’t raise ArgumentCountError; keep
behavior unchanged and update any docblocks/tests that reference the old
signatures.
In `@includes/payment-methods/class-wc-stripe-upe-payment-gateway.php`:
- Around line 532-534: The new stripe_params['isAdaptivePricingSupported'] flag
(set via WC_Stripe_Helper::is_adaptive_pricing_supported() in
class-wc-stripe-upe-payment-gateway.php) affects frontend gating and must be
validated across all checkout flows: classic checkout, Blocks checkout,
optimized checkout, and express checkout (including carts containing
subscriptions and pre-orders). Manually verify in each flow that payment method
availability and rendering match expected behavior when the flag is true/false;
if any mismatch is found, adjust
WC_Stripe_Helper::is_adaptive_pricing_supported() logic or the consumer code
that reads stripe_params['isAdaptivePricingSupported'] so availability/rendering
rules are consistent, add automated integration tests covering each checkout
variant + subscription/pre-order scenarios, and document test results and any
code changes.
In `@tests/phpunit/WC_Stripe_Helper_Test.php`:
- Around line 1145-1153: Test cleanup is restoring the already-modified
$stripe_settings instead of the original settings and is not resetting product
mock states; before the test mutates settings store the original settings (e.g.
$original_stripe_settings) and at the end call
WC_Stripe_Helper::update_main_stripe_settings($original_stripe_settings) to
restore them, also reset the feature flag via
update_option(\WC_Stripe_Feature_Flags::CHECKOUT_SESSIONS_FEATURE_FLAG_NAME,
'no') (or restore its original value if you saved it), and explicitly clear
mocks by setting WC_Subscriptions_Product::is_subscription(…) and
WC_Pre_Orders_Product::is_pre_order(…) back to false (or call their provided
reset methods) in the cleanup block alongside WC()->cart->empty_cart() and
remove_filter('woocommerce_is_checkout', $is_checkout_filter).
- Around line 1163-1215: Update the data provider
provide_is_adaptive_pricing_supported to add cases covering the Blocks checkout
path (is_checkout = false but has_block('woocommerce/checkout') = true) and
mixed-cart scenarios (e.g., cart_product_types like ['simple','subscription']
and ['simple','pre-order']) so the loop that rejects subscriptions/pre-orders is
exercised; adjust the test setup to allow per-product subscription/pre-order
mocking instead of the global WC_Subscriptions_Product::set_is_subscription
(e.g., mock product instances or stub WC_Subscriptions_Product::is_subscription
to check product IDs) and ensure the test toggles
has_block('woocommerce/checkout') when needed to validate both checkout paths.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/phpunit/WC_Stripe_Helper_Test.php`:
- Around line 1125-1135: The global static mocks used here cause all products to
be treated uniformly based on cart_product_type, so add a concise inline comment
above the WC_Subscriptions_Product::set_is_subscription and
WC_Pre_Orders_Product::set_is_pre_order calls noting that the mock applies
globally (e.g., "// Note: mock applies globally to all products — mixed cart
types not supported by this mock"), reference cart_product_type to explain why
tests set a single type, and keep the comment near those lines so future
maintainers understand the limitation.
---
Duplicate comments:
In `@tests/phpunit/Helpers/WC_Subscriptions_Product.php`:
- Around line 32-34: The mock get_trial_length currently ignores the $product
parameter and always returns the global self::$get_trial_length_result,
preventing per-product behavior; change get_trial_length (and the similar mocks
like is_subscription) to accept/interpret self::$get_trial_length_result as
either a scalar fallback or an associative array/list keyed by product ID (or
SKU) and, when $product is provided, look up the product's id (e.g.,
$product->get_id()) and return the matching value (or a default when not
listed); also preserve backward compatibility by returning the scalar fallback
when the static is not an array and handle null $product by returning the
fallback.
In `@tests/phpunit/WC_Stripe_Helper_Test.php`:
- Around line 1166-1218: The data provider lacks cases for the Blocks checkout
branch and mixed-cart contents; add test rows exercising
is_adaptive_pricing_supported when is_checkout() is false but
has_block('woocommerce/checkout') is true, and add mixed-cart scenarios where
cart_product_type represents multiple item types (e.g.,
['simple','subscription'] and ['simple','pre-order']) to ensure the method
short-circuits and returns false for carts containing disallowed types; update
WC_Stripe_Helper_Test::provide_is_adaptive_pricing_supported to include these
entries and ensure the test harness/mocks emulate
has_block('woocommerce/checkout') so the has_block branch in
is_adaptive_pricing_supported is executed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/phpunit/WC_Stripe_Helper_Test.php`:
- Around line 1098-1174: The test test_is_adaptive_pricing_supported uses
$saved_post only inside the second if ($has_block) restore block but assigns it
inside the first if ($has_block), which is fragile; initialize $saved_post =
null before the first if or move the restore into a guaranteed cleanup path
(e.g., a finally-style pattern) so
WC_Stripe_Helper_Test::test_is_adaptive_pricing_supported always has $saved_post
defined when restoring the global $post.
---
Duplicate comments:
In `@tests/phpunit/WC_Stripe_Helper_Test.php`:
- Around line 1181-1248: The data provider provide_is_adaptive_pricing_supported
is missing two edge cases; add an entry where both is_checkout=true and
has_block=true (mixed checkout context) with adaptive_pricing='yes' and
expected=true to ensure no double-evaluation, and add an entry where
adaptive_pricing is an unexpected value (e.g., adaptive_pricing='' with
feature_flag=true and is_checkout=true or has_block=false) with expected=false
to assert the 'yes' strict check rejects non-'yes' values; update the array in
provide_is_adaptive_pricing_supported to include these two cases.
daledupreez
left a comment
There was a problem hiding this comment.
This tests as described, but I do have some feedback and suggestions that may be worth incorporating. My biggest concern is around support for Deposits -- I think we should not support deposits, TBH, even if that's mostly a requirements concern.
includes/class-wc-stripe-helper.php
Outdated
| * When on the checkout page, adaptive pricing is not supported if the cart contains | ||
| * any of: subscription products or pre-order (charge upon release) products. |
There was a problem hiding this comment.
Nit: I think it would help if the specific items were listed on their own lines.
| * When on the checkout page, adaptive pricing is not supported if the cart contains | |
| * any of: subscription products or pre-order (charge upon release) products. | |
| * When on the checkout page, adaptive pricing is not supported if the cart contains | |
| * any of the following: | |
| * - A subscription product. | |
| * - A pre-order product that will be charged upon release. |
Also, is it OK to handle products using a deposit? I don't think we save the payment method in that case, but I worry that Deposits sets the expectation that it is $X today and $Y later, when the actual amounts in local currency may not be $Y on any given future date.
There was a problem hiding this comment.
Good call. I have added deposits in the unsupported list in my latest commits.
There was a problem hiding this comment.
It would also be good to make sure that decision/exclusion is pushed up to the requirements discussion/doc as well.
| WC()->cart->add_to_cart( $product->get_id(), 1 ); | ||
| } | ||
|
|
||
| $actual = WC_Stripe_Helper::is_adaptive_pricing_supported(); |
There was a problem hiding this comment.
As a thought, it may be worth adding tests that check for additional cases that would exercise more logic, including the following:
- multiple standard products in the cart
- multiple standard products plus one subscription in the cart
TIL that pre-orders can't be in a cart with other products.
includes/class-wc-stripe-helper.php
Outdated
| if ( class_exists( 'WC_Subscriptions_Product' ) && WC_Subscriptions_Product::is_subscription( $product ) ) { | ||
| return false; | ||
| } | ||
|
|
||
| // Pre-order (charge upon release) is not supported with adaptive pricing. | ||
| if ( class_exists( 'WC_Pre_Orders_Product' ) && WC_Pre_Orders_Product::product_is_charged_upon_release( $product ) ) { |
There was a problem hiding this comment.
Two non-blocking thoughts on this front:
- Might it be worth adding some defensive checks to ensure the methods exist?
- Might it be worth running the existence checks outside the loop so they only get run once? This is something of a micro-optimization, as class lookups should be quick, but it was something that struck me as unnecessary work.
There was a problem hiding this comment.
Might it be worth adding some defensive checks to ensure the methods exist?
Skipping this as we are checking for class exist which should be sufficient here.
Might it be worth running the existence checks outside the loop so they only get run once?
Sounds good to me. Addressed this feedback.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@includes/class-wc-stripe-helper.php`:
- Around line 1108-1118: The docblock for the method
is_adaptive_pricing_supported (the method that checks subscriptions, pre-orders,
and deposits) is missing mention of products with deposits enabled; update the
method's docblock summary and description to explicitly state that adaptive
pricing is also not supported when the cart contains products with deposits
enabled (the same condition handled in the method where deposits are checked),
so the docblock accurately reflects subscriptions, pre-orders charged on
release, and deposits as reasons adaptive pricing is disabled.
- Line 1127: Replace the external static call WC_Stripe_Helper::get_settings
with an internal static call using self::get_settings to maintain consistency
with other internal calls in the class (e.g., self::get_stripe_settings) —
update the conditional that checks 'adaptive_pricing' to call self::get_settings
instead of WC_Stripe_Helper::get_settings.
In `@tests/phpunit/Helpers/WC_Subscriptions_Product.php`:
- Around line 67-72: The is_subscription() mock currently falls back to
self::$is_subscription_result whenever $product is not a WC_Product even if
self::$subscription_product_ids is populated; update is_subscription() to accept
int|WC_Product|null like the real implementation: if $product is an integer (or
a WC_Product, use $product->get_id()) and self::$subscription_product_ids is
non-empty, return in_array(the resolved product ID,
self::$subscription_product_ids, true); only when $product is null and no
per-product ids apply should it return self::$is_subscription_result. Reference
the function is_subscription and the static properties
self::$subscription_product_ids and self::$is_subscription_result when making
the change.
- Around line 31-41: Add a centralized static reset method to
WC_Subscriptions_Product that clears all static helper state (at minimum reset
self::$subscription_product_ids and any other static flags used by
set_is_subscription() and set_trial_length()) so tests cannot leak state across
iterations; implement a public static function reset(): void that reinitializes
those static properties to their defaults and update tests
(WC_Stripe_Express_Checkout_Helper_Test and WC_Stripe_Helper_Test) to call
WC_Subscriptions_Product::reset() in their tearDown() methods to ensure per-test
cleanup.
In `@tests/phpunit/WC_Stripe_Helper_Test.php`:
- Around line 1157-1248: Add two mixed-cart scenarios to the
provide_is_adaptive_pricing_supported data provider: one with
'cart_product_types' => [ 'simple', 'pre-order' ] and one with
'cart_product_types' => [ 'simple', 'deposits' ]; both should mirror the
behavior of the pure pre-order and deposits cases (set 'feature_flag' => true,
'is_checkout' => true, 'has_block' => false, 'adaptive_pricing' => 'yes', and
'expected' => false). Update the provide_is_adaptive_pricing_supported function
in WC_Stripe_Helper_Test to include these entries so mixed simple+pre-order and
simple+deposits scenarios are explicitly covered.
Changes proposed in this Pull Request:
Added 'is_adaptive_pricing_supported' method to check if adaptive pricing is supported for the store setting and the products in the cart.
Note:
Updated some ECE test files irrelevant to the changes of this PR to ensure test cleanups, so that test setups do not conflict with the tests added for this PR.
Testing instructions
add_filter('wc_stripe_is_checkout_sessions_available', '__return_true');wc_stripe_upe_params.isAdaptivePricingSupported. The value should be1.wc_stripe_upe_params.isAdaptivePricingSupported. The value should be1.wc_stripe_upe_params.isAdaptivePricingSupported. The value should be empty.wc_stripe_upe_params.isAdaptivePricingSupported. The value should be empty.Changelog entry
Changelog Entry Comment
Comment
Post merge