Show payment methods available in the PMC on settings page

[Mayisha]

In the current PMC implementation, we are syncing the enabled/disable payment methods between plugin settings and Stripe dashboard. But the list of available payment methods in plugin settings is not aligned with the payment method configuration. As a result, we show some payment methods in the settings page that are not available in the PMC.

Changes proposed in this Pull Request:

• Retrieve available payment methods from PMC when get_upe_available_payment_methods is called.
• Aligned with the cache implementation.
• If pmc is not enabled, fall back to the previous code.

Testing instructions

• Go to your Stripe settings page and connect to a Stripe account that does not support some payment methods (i.e, I have tested with an Australian Stripe account)
• In develop, go to the payment methods tab and notice that you have OXXO, Boleto, ACSS available in the payment method list.

• Go to the payment methods page in your Stripe dashboard and choose the payment configuration under WooCommerce.
• Notice that, you do not have OXXO/Boleto/ACSS available here.
• Now set your store currency to CAD and enable ACSS (Pre-authorized debit).
• Click Save and refresh your page.
• Notice that, ACSS is still disabled.
• In your store logs, you will find the error Error: acss_debit is not available to this merchant.
• Now checkout to this branch and refresh the Stripe settings page.
• Confirm that you don't see the unsupported payment methods anymore.
• Try to enable/disable a few methods and confirm that the changes persists in your plugin settings page and also synced in the Stripe dashboard.

[diegocurbelo]

The implementation looks good, the only comment is renaming the get_upe_available_payment_methods function in WC_Stripe_Payment_Method_Configurations

[daledupreez]

@Mayisha, could you add more context to the PR so we can test it before @diegocurbelo is available?

[Mayisha]

@daledupreez I have added the description and addressed the feedback.

[daledupreez]

It looks like there are a number of legitimate unit test failures that we need to resolve before we can merge this.

@Mayisha, let me know if you'd like to hand over work on getting the tests working.

[daledupreez]

I am not clear on why this code is not checking/calling self::is_enabled(). We are checking that in get_upe_enabled_payment_method_ids() below, and in the current calling code, so I am not sure why we're skipping the check here.

My initial suspicion is that is_enabled() is checking for write permissions, where we only need to read what functions are available for the account. But then I got confused across the various cases.

[Mayisha]

My initial suspicion is that is_enabled() is checking for write permissions, where we only need to read what functions are available for the account. But then I got confused across the various cases.

is_enabled() returns true if PMC is enabled for this account. PMC is enabled only if the account has been re-authenticated using the connect OAuth flow. If an account still uses manual keys (which we used to support many versions before), we can not retrieve the PMC for those accounts. So for these accounts, we need to fallback to the available and enabled payment methods list stored in the DB.

I am not clear on why this code is not checking/calling self::is_enabled()

I had the other version too when I was checking this in the configuration class. Are you proposing this would be better?

[daledupreez]

Most of my confusion stems from my lack of context and noticing that we're not following the same pattern as get_upe_enabled_payment_method_ids(), which has the following code in develop at the moment:

woocommerce-gateway-stripe/includes/class-wc-stripe-payment-method-configurations.php

Lines 146 to 176 in 0a9c6b9

	/**
	* Get the UPE enabled payment method IDs.
	*
	* @param bool $force_refresh Whether to force a refresh of the payment method configuration from Stripe.
	* @return array
	*/
	public static function get_upe_enabled_payment_method_ids( $force_refresh = false ) {
	// If the payment method configurations API is not enabled, we fallback to the enabled payment methods stored in the DB.
	if ( ! self::is_enabled() ) {
	$stripe_settings = WC_Stripe_Helper::get_stripe_settings();
	return isset( $stripe_settings['upe_checkout_experience_accepted_payments'] ) && ! empty( $stripe_settings['upe_checkout_experience_accepted_payments'] )
	? $stripe_settings['upe_checkout_experience_accepted_payments']
	: [ WC_Stripe_Payment_Methods::CARD ];
	}
	// Migrate payment methods from DB to Stripe PMC if needed
	self::maybe_migrate_payment_methods_from_db_to_pmc();
	$enabled_payment_method_ids = [];
	$merchant_payment_method_configuration = self::get_primary_configuration( $force_refresh );
	if ( $merchant_payment_method_configuration ) {
	foreach ( $merchant_payment_method_configuration as $payment_method_id => $payment_method ) {
	if ( isset( $payment_method->display_preference->value ) && 'on' === $payment_method->display_preference->value ) {
	$enabled_payment_method_ids[] = $payment_method_id;
	}
	}
	}
	return $enabled_payment_method_ids;
	}

In particular, we have local code that returns valid data in the code block starting with if ( ! self::is_enabled() ) {. That is likely for backwards compatibility, but I am wondering why we're not using the same defensive approach in this new method. I am OK with us doing so, but I think we should then document that difference very clearly, as that's a fairly subtle difference between the two methods that is not obvious.

[github-actions]

📈 PHP Unit Code Coverage Report

Package	Line Rate	Health
includes/abstracts/abstract-wc-stripe-payment-gateway.php	48%	❌
includes/class-wc-stripe-payment-method-configurations.php	96%	✔
includes/payment-methods/class-wc-stripe-upe-payment-gateway.php	48%	❌
Summary	45% (7265 / 16309)	❌

[Mayisha]

@daledupreez I have addressed the feedback. Can I have another 👀 from you?

[daledupreez]

I haven't tested yet, but I have some feedback up from inspection. It may be a while before I can fully test, so want to get this feedback to you sooner.

[daledupreez]

Most of my confusion stems from my lack of context and noticing that we're not following the same pattern as get_upe_enabled_payment_method_ids(), which has the following code in develop at the moment:

woocommerce-gateway-stripe/includes/class-wc-stripe-payment-method-configurations.php

Lines 146 to 176 in 0a9c6b9

	/**
	* Get the UPE enabled payment method IDs.
	*
	* @param bool $force_refresh Whether to force a refresh of the payment method configuration from Stripe.
	* @return array
	*/
	public static function get_upe_enabled_payment_method_ids( $force_refresh = false ) {
	// If the payment method configurations API is not enabled, we fallback to the enabled payment methods stored in the DB.
	if ( ! self::is_enabled() ) {
	$stripe_settings = WC_Stripe_Helper::get_stripe_settings();
	return isset( $stripe_settings['upe_checkout_experience_accepted_payments'] ) && ! empty( $stripe_settings['upe_checkout_experience_accepted_payments'] )
	? $stripe_settings['upe_checkout_experience_accepted_payments']
	: [ WC_Stripe_Payment_Methods::CARD ];
	}
	// Migrate payment methods from DB to Stripe PMC if needed
	self::maybe_migrate_payment_methods_from_db_to_pmc();
	$enabled_payment_method_ids = [];
	$merchant_payment_method_configuration = self::get_primary_configuration( $force_refresh );
	if ( $merchant_payment_method_configuration ) {
	foreach ( $merchant_payment_method_configuration as $payment_method_id => $payment_method ) {
	if ( isset( $payment_method->display_preference->value ) && 'on' === $payment_method->display_preference->value ) {
	$enabled_payment_method_ids[] = $payment_method_id;
	}
	}
	}
	return $enabled_payment_method_ids;
	}

In particular, we have local code that returns valid data in the code block starting with if ( ! self::is_enabled() ) {. That is likely for backwards compatibility, but I am wondering why we're not using the same defensive approach in this new method. I am OK with us doing so, but I think we should then document that difference very clearly, as that's a fairly subtle difference between the two methods that is not obvious.

[daledupreez]

Is this change intentionally part of this PR? I just want to make sure, because it's a minor thing that could have unexpected consequences!

[daledupreez]

Could you add a comment explaining why this condition is needed?

I am especially confused because the old behaviour was to filter the local list, while we're now only filtering the list when we're not fetching data from Stripe.

[daledupreez]

It may be worth adding a comment around the choice to trigger these calls in this order, just to make it clear why we're making the calls with the true argument in only this first call.

Suggested change

	$enabled_payment_method_ids = $is_upe_enabled ? $this->gateway->get_upe_enabled_payment_method_ids( true ) : WC_Stripe_Helper::get_legacy_enabled_payment_method_ids();
	// Note that we want to refresh the Stripe Payment Method Configuration cache exactly once
	// when UPE is enabled, so we force a refresh on this first call only, and not when calling
	// get_upe_available_payment_methods() below.
	$enabled_payment_method_ids = $is_upe_enabled ? $this->gateway->get_upe_enabled_payment_method_ids( true ) : WC_Stripe_Helper::get_legacy_enabled_payment_method_ids();