Fix edit permissions for indian subscriptions

[wjrosa]

Fixes STRIPE-884

Changes proposed in this Pull Request:

In this PR, I am changing the main logic used to block editing of Indian subscriptions. Instead of just checking for any existing mandate ID meta in the parent order, I am now retrieving the mandate object from the Stripe API and comparing the specific fields that flag an Indian subscription as uneditable.

Testing instructions

• Checkout and build this branch on your test environment (fix/edit-permissions-for-indian-subscriptions)
• Install the subscription extension and add a subscription product
• Connect your Stripe account
• As a customer, purchase the subscription using Stripe Link (or another mandate-based payment method: SEPA, Bancontact, iDEAL, BACS, ACH, etc.)
• Complete the purchase successfully
• As the customer, go to "My Account → Subscriptions" and change the payment method to a regular Stripe credit card
• As a merchant/admin, navigate to WooCommerce → Orders and open the subscription
• Attempt to edit a line item (e.g., change shipping method from "DHL" to "Local Pickup")
• Confirm the pencil icon now appears next to line items, allowing edits

If you want, connect an Indian Stripe account, set your store currency to INR, purchase the subscription again, and this time confirm you are not able to edit it as a merchant.

---

• Covered with tests (or have a good reason not to test in description ☝️)
• Tested on mobile (or does not apply)

Changelog entry

• This Pull Request does not require a changelog entry. (Comment required below)

Changelog Entry Comment

Comment

Post merge

• Added testing instructions to the Release Testing Instructions wiki page (or does not apply)
• Added the needs docs label (or does not apply)
• Included this PR in the Release Thread scope (or does not apply)

[coderabbitai]

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

includes/compat/trait-wc-stripe-subscriptions.php (1)

1060-1117: ⚠️ Potential issue | 🟡 Minor

Add null checks for type-specific payment method properties.

Several switch branches access nested properties without validation (e.g., $payment_method->card->last4, $payment_method->sepa_debit->last4, $payment_method->link->email). While the parent object is validated before this method is called, Stripe API objects may have incomplete or missing nested properties. The inconsistent use of defensive patterns within the method (line 1113 uses ?? but others don't) suggests adding guards to all accesses.

Consider using null-safe operators or isset() checks for each sub-property, matching existing patterns in includes/payment-tokens/class-wc-stripe-payment-tokens.php (lines 594–596, 609–612).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@includes/compat/trait-wc-stripe-subscriptions.php` around lines 1060 - 1117,
The get_payment_method_to_display_for_payment_method method accesses nested
properties (e.g., $payment_method->card->brand/last4, ->sepa_debit->last4,
->cashapp->cashtag, ->link->email, ->us_bank_account, ->au_becs_debit,
->acss_debit, ->bacs_debit, ->billing_details->email) without null checks;
update the switch branches in get_payment_method_to_display_for_payment_method
to defensively verify each nested property (using isset() or PHP null-safe
operator) before reading sub-properties and provide safe fallbacks (e.g., 'N/A'
or empty string) when missing, mirroring the defensive patterns used in
class-wc-stripe-payment-tokens.php so no undefined property notices occur.

🤖 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/compat/class-wc-stripe-subscriptions-helper.php`:
- Around line 333-342: The is_subscription_edit_page() function uses raw
$_REQUEST via $query_params['post'] when calling get_post_type(); cast or
validate $query_params['post'] to an int (e.g., (int) $query_params['post'] or
use absint()) before passing it to get_post_type() to ensure the value is
sanitized and treated as untrusted input; update the check stored in
$is_shop_subscription_post_type to use the sanitized variable and keep existing
logic for action and post type checks.

In `@includes/compat/trait-wc-stripe-subscriptions.php`:
- Around line 1310-1313: The code assumes
WC_Stripe_API::retrieve()/stripe_request() returned a valid object and directly
accesses $payment_method->type; add an is_wp_error() check after calling
WC_Stripe_API::retrieve() (or wherever $payment_method is populated) and handle
WP_Error by skipping the card-country logic (e.g., do not return false) and/or
log the error (use existing logger) instead of caching the WP_Error;
specifically, before checking WC_Stripe_Payment_Methods::CARD ===
$payment_method->type validate ! is_wp_error( $payment_method ) and
bail/continue appropriately to avoid PHP notices and accidental caching of error
objects.

In `@tests/phpunit/Compat/WC_Stripe_Subscriptions_Helper_Test.php`:
- Around line 372-383: In test_is_subscription_edit_page, don't remove the
$_REQUEST superglobal with unset($_REQUEST) as that can trigger undefined
variable notices later; instead reset it to an empty array by assigning
$_REQUEST = [] after the call to
WC_Stripe_Subscriptions_Helper::is_subscription_edit_page() so the superglobal
remains defined for other tests and teardown.
- Around line 390-429: The data provider provide_test_is_subscription_edit_page
is passing WC_Order/WC_Subscription objects as the 'post' query param but real
requests use post IDs; update the two HPOS disabled cases to pass the post IDs
(e.g., use $order->get_id() and $subscription->get_id() or cast to int) instead
of the objects so that get_post_type($query_params['post']) receives an int/ID
and the is_subscription_edit_page logic (which calls get_post_type) behaves
correctly.

In `@tests/phpunit/WC_Stripe_Payment_Gateway_Test.php`:
- Around line 861-910: The test test_disable_subscription_edit_for_india is
failing because WC_Stripe_Subscriptions_Helper::is_subscription_edit_page()
returns false in the test context (so disable_subscription_edit_for_india exits
early) — fix by setting the appropriate $_REQUEST keys to simulate the
subscription edit page (e.g. set $_REQUEST['page'] and $_REQUEST['id'] for HPOS
or $_REQUEST['action'] and $_REQUEST['post'] for post-based edits) before
calling $this->gateway->disable_subscription_edit_for_india and ensure you clean
up/reset those $_REQUEST entries in tearDown(); this targets the
is_subscription_edit_page check used by the trait-wc-stripe-subscriptions.php
logic so the India card branch is actually exercised.

---

Outside diff comments:
In `@includes/compat/trait-wc-stripe-subscriptions.php`:
- Around line 1060-1117: The get_payment_method_to_display_for_payment_method
method accesses nested properties (e.g., $payment_method->card->brand/last4,
->sepa_debit->last4, ->cashapp->cashtag, ->link->email, ->us_bank_account,
->au_becs_debit, ->acss_debit, ->bacs_debit, ->billing_details->email) without
null checks; update the switch branches in
get_payment_method_to_display_for_payment_method to defensively verify each
nested property (using isset() or PHP null-safe operator) before reading
sub-properties and provide safe fallbacks (e.g., 'N/A' or empty string) when
missing, mirroring the defensive patterns used in
class-wc-stripe-payment-tokens.php so no undefined property notices occur.

---

Duplicate comments:
In `@includes/compat/trait-wc-stripe-subscriptions.php`:
- Around line 1303-1308: The code caches any response from stripe_request into
WC_Stripe_Database_Cache which may store WP_Error or Stripe error objects and
suppress retries; update the logic around $payment_method (from
stripe_request('payment_methods/' . $source_id)) so that you only call
WC_Stripe_Database_Cache::set($cache_key, ...) when the response is a successful
payment method (i.e., not is_wp_error($payment_method) and does not contain an
'error' key/property), leaving errors uncached so retries can occur and
subsequent code doesn't receive a cached WP_Error or error object.

In `@tests/phpunit/WC_Stripe_Payment_Gateway_Test.php`:
- Around line 891-895: The mock HTTP response in the return array uses
'response' => 200 which doesn't match WP HTTP API shape and breaks
wp_remote_retrieve_response_code(); update the mock in the failing test to use
the proper WP format by replacing the plain integer with an associative response
array (e.g., 'response' => ['code' => 200, 'message' => 'OK']) while keeping
'headers' and 'body' as-is so wp_remote_retrieve_response_code() and related
helpers read the status correctly; locate and fix this in the test that returns
the $payment_method mock (the return block containing 'response', 'headers',
'body').
- Around line 947-960: The test data in WC_Stripe_Payment_Gateway_Test uses
stale mandate-era fields (`amount_type`, `supported_types`) inside the `payment
method` fixture for cases keyed 'payment method is not card' and 'method is
card, but not indian'; update those fixtures to use realistic Stripe
PaymentMethod properties (keep `id` and `type`, remove `sepa_debit.amount_type`
and `sepa_debit.supported_types`) and if needed replace with valid fields for
that type (e.g., leave an appropriate `sepa_debit` object with only real fields
or remove it entirely), ensuring the test still asserts based on `type` and
`card.country` where applicable in the WC_Stripe_Payment_Gateway_Test class.

[wjrosa]

I am working on fixing the unit tests 👀. Edit: fixed

[coderabbitai]

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_Payment_Gateway_Test.php`:
- Around line 849-860: The docblock for the test of
disable_subscription_edit_for_india is missing the `@param` entry for the second
parameter $is_subscriptions_edit_page; update the PHPDoc for the test method in
WC_Stripe_Payment_Gateway_Test (the test for
disable_subscription_edit_for_india) to include a line "@param bool
$is_subscriptions_edit_page" (placed as the second `@param` to match the method
signature) so the docblock accurately documents all parameters provided by
provide_test_disable_subscription_edit_for_india and aligns with the method
signature used in the test.

---

Duplicate comments:
In `@tests/phpunit/WC_Stripe_Payment_Gateway_Test.php`:
- Around line 968-994: Remove stale mandate-era fields from the data provider
entries by deleting the non-existent keys 'amount_type' and 'supported_types'
inside the 'sepa_debit' and 'card' fixtures for the cases (e.g., the 'payment
method is not card' and 'method is card, but not indian' entries), and make the
non-India intent explicit by adding a 'country' => 'US' (or other non-IN) key to
the 'card' fixture in the 'method is card, but not indian' case so the test
intent is clear; additionally add a new data-provider case that returns a falsy
$payment_method (simulating a Stripe API error/empty response) to assert the
code path that falls back to returning $editable.
- Around line 899-903: The mocked HTTP response uses the wrong WordPress HTTP
API shape ('response' => 200); update the mock returned array so 'response' is
an associative array with 'code' => 200 and 'message' => 'OK' (e.g. change
'response' => 200 to 'response' => [ 'code' => 200, 'message' => 'OK' ]) where
the $payment_method JSON body is returned in WC_Stripe_Payment_Gateway_Test so
WC_Stripe_API's response parser receives the correct shape.
- Around line 886-891: The test incorrectly does a full assignment ($_REQUEST =
[...]) which creates a local variable rather than mutating the PHP superglobal,
so is_subscription_edit_page() still sees no request keys; replace the
full-assignment with element-wise assignments (e.g. $_REQUEST['post'] =
$subscription; $_REQUEST['action'] = 'edit';) when $is_subscriptions_edit_page
is true, and ensure you unset those specific keys (unset($_REQUEST['post'],
$_REQUEST['action'])) after the case to avoid cross-test pollution; adjust the
code paths that check $is_subscriptions_edit_page and the cleanup so the
superglobal state matches expectations for each test case.

[coderabbitai]

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_Payment_Gateway_Test.php`:
- Around line 916-917: The test currently uses unset($_REQUEST) which is
unreliable for clearing the superglobal; replace that call in
WC_Stripe_Payment_Gateway_Test (the teardown/cleanup block that also sets
WC_Subscriptions_Helpers::$wcs_is_subscription = null) with an explicit
assignment $_REQUEST = [] to ensure no cross-test contamination, or
alternatively save the original $_REQUEST to a temp variable and restore it
after the test for full isolation.

---

Duplicate comments:
In `@tests/phpunit/WC_Stripe_Payment_Gateway_Test.php`:
- Around line 984-998: The fixture "'method is card, but not indian'" contains
stale mandate-era fields (amount_type and supported_types) and is missing an
explicit non-India country; update the fixture used in
WC_Stripe_Payment_Gateway_Test so the payment method.card object does not
include mandate-era fields (remove amount_type and supported_types) and add a
concrete country value that is not 'IN' (e.g., 'US') so the test truly
represents "card explicitly from a non-India country" for the payment method
with id 'pm_456'.
- Around line 900-904: The mocked HTTP response uses an incorrect shape
('response' => 200) which breaks wp_remote_retrieve_response_code and causes
WC_Stripe_API/stripe_request to treat the call as an error; update the mock to
use the proper WP HTTP API shape (e.g. 'response' => ['code' => 200, 'message'
=> 'OK']) while preserving 'headers' and 'body' so $payment_method is populated
and the "method is indian card" path is exercised correctly.

[Mayisha]

Looks good to me. I have tested and verified that subscriptions paid with a normal card is editable but subscriptions paid with Indian 3DS card is not editable in this branch. 👍

[daledupreez]

The changes look good to me and test well, but I do have some questions and minor nits that may drive some minor changes (mostly in the tests).