Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable deferred intent creation when initialization process encounters cache unavailability #7686

Merged
merged 16 commits into from
Nov 17, 2023
Merged

Enable deferred intent creation when initialization process encounters cache unavailability #7686

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
e2a6285
test
Nov 8, 2023
4ed46d9
Replace integration test with unit test of the method
Nov 9, 2023
b623c8f
Merge branch 'develop' into fix/more-use-cases-for-enabled-dupe
Nov 9, 2023
7869532
Fixing tests - part I
Nov 9, 2023
5def517
Fixing tests - part II
Nov 9, 2023
9d8d698
Fixing tests - part III
Nov 9, 2023
f1d2c51
Add changelog and fix linting issues
Nov 9, 2023
5306cd8
Apply yoda condition
Nov 9, 2023
82fd490
Revert test name to a meaningful string
Nov 9, 2023
98d6852
Revert test name to a meaningful string II
Nov 9, 2023
e0e85b2
Fix WooPay tracker test by pre-enabling gateway
Nov 13, 2023
e95b783
Load currency helper class early in the tst initialization to ensure …
Nov 16, 2023
73fb1e2
Merge branch 'develop' into e0e85b25cdbedfac6c16529b86e1d34fa44e8f5c
Nov 16, 2023
52496de
Revert the original test name
Nov 16, 2023
abc7347
Improve logic which returns the state of the dUPE FF
Nov 17, 2023
a540e9d
Merge branch 'develop' into fix/more-use-cases-for-enabled-dupe
timur27 Nov 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions changelog/fix-more-use-cases-for-enabled-dupe
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: fix

Enable deferred intent creation when initialization process encounters cache unavailability
3 changes: 3 additions & 0 deletions includes/class-wc-payments-features.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ public static function is_upe_split_enabled() {
*/
public static function is_upe_deferred_intent_enabled() {
$account = WC_Payments::get_database_cache()->get( WCPay\Database_Cache::ACCOUNT_KEY, true );
if ( null === $account ) {
return true;
}
return is_array( $account ) && ( $account[ self::DEFERRED_UPE_SERVER_FLAG_NAME ] ?? false );
Copy link
Contributor

Choose a reason for hiding this comment

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

Side question - would it also make sense to return true in case $account[ self::DEFERRED_UPE_SERVER_FLAG_NAME ] is not set?

i.e.:

Suggested change
return is_array( $account ) && ( $account[ self::DEFERRED_UPE_SERVER_FLAG_NAME ] ?? false );
return is_array( $account ) && ( $account[ self::DEFERRED_UPE_SERVER_FLAG_NAME ] ?? true );

Asking because we're basically saying: "if there's no account data - force dUPE". But if $account data is an array without the is_deferred_intent_creation_upe_enabled key, we're saying that dUPE is disabled. Am I getting my neurons too twisted? 😵

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right and abc7347 improves this together with removing one redundant test which came up after the change, thank you!

The scenario with returning disabled dUPE state wouldn't happen because server always returns a value, but this doesn't mean client can rely on this so I agree with you.

}

Expand Down
28 changes: 10 additions & 18 deletions tests/unit/admin/test-class-wc-rest-payments-settings-controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,16 @@ class WC_REST_Payments_Settings_Controller_Test extends WCPAY_UnitTestCase {
/**
* An array of mocked split UPE payment gateways mapped to payment method ID.
*
* @var array
* @var UPE_Payment_Gateway
*/
private $mock_upe_payment_gateway;

/**
* An array of mocked split UPE payment gateways mapped to payment method ID.
*
* @var array
* @var UPE_Split_Payment_Gateway
*/
private $mock_split_upe_payment_gateways;
private $mock_split_upe_payment_gateway;

/**
* UPE system under test.
Expand Down Expand Up @@ -201,7 +201,7 @@ public function set_up() {

$this->upe_controller = new WC_REST_Payments_Settings_Controller( $this->mock_api_client, $this->mock_upe_payment_gateway, $this->mock_wcpay_account );

$this->mock_upe_split_payment_gateway = new UPE_Split_Payment_Gateway(
$this->mock_split_upe_payment_gateway = new UPE_Split_Payment_Gateway(
$this->mock_api_client,
$this->mock_wcpay_account,
$customer_service,
Expand All @@ -216,7 +216,7 @@ public function set_up() {
$this->mock_fraud_service
);

$this->upe_split_controller = new WC_REST_Payments_Settings_Controller( $this->mock_api_client, $this->mock_upe_split_payment_gateway, $this->mock_wcpay_account );
$this->upe_split_controller = new WC_REST_Payments_Settings_Controller( $this->mock_api_client, $this->mock_split_upe_payment_gateway, $this->mock_wcpay_account );

$this->mock_api_client
->method( 'is_server_connected' )
Expand Down Expand Up @@ -451,22 +451,14 @@ public function test_upe_update_settings_saves_enabled_payment_methods() {
}

public function test_upe_split_update_settings_saves_enabled_payment_methods() {
$this->mock_upe_split_payment_gateway->update_option( 'upe_enabled_payment_method_ids', [ Payment_Method::CARD ] );
$this->mock_split_upe_payment_gateway->update_option( 'upe_enabled_payment_method_ids', [ Payment_Method::CARD ] );

$request = new WP_REST_Request();
$request->set_param( 'enabled_payment_method_ids', [ Payment_Method::CARD, Payment_Method::GIROPAY ] );

$this->upe_split_controller->update_settings( $request );

$this->assertEquals( [ Payment_Method::CARD, Payment_Method::GIROPAY ], $this->mock_upe_split_payment_gateway->get_option( 'upe_enabled_payment_method_ids' ) );
}
$request = new WP_REST_Request();
$request->set_param( 'enabled_payment_method_ids', [ Payment_Method::CARD, Payment_Method::GIROPAY ] );

public function test_update_settings_validation_fails_if_invalid_gateway_id_supplied() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was removed because it's an integration test that works only with the legacy gateway. It's not easy nor elegant to mock gateway in WC_Payments because methods/objects are static.

Since the tested method relies solely on get_upe_available_payment_methods, get_upe_available_payment_methods is now fully tested by test-class-upe-payment-gateway.php::test_get_upe_available_payment_methods()

$request = new WP_REST_Request( 'POST', self::$settings_route );
$request->set_param( 'enabled_payment_method_ids', [ 'foo', 'baz' ] );
$this->upe_split_controller->update_settings( $request );

$response = rest_do_request( $request );
$this->assertEquals( 400, $response->get_status() );
$this->assertEquals( [ Payment_Method::CARD, Payment_Method::GIROPAY ], $this->mock_split_upe_payment_gateway->get_option( 'upe_enabled_payment_method_ids' ) );
}

public function test_update_settings_fails_if_user_cannot_manage_woocommerce() {
Expand Down
15 changes: 0 additions & 15 deletions tests/unit/migrations/test-class-track-upe-status.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,21 +59,6 @@ public function test_track_enabled_on_upgrade() {
$this->assertSame( '1', get_option( Track_Upe_Status::IS_TRACKED_OPTION ) );
}

public function test_track_disabled_on_upgrade() {
update_option( WC_Payments_Features::UPE_FLAG_NAME, 'disabled' );

Track_Upe_Status::maybe_track();

$this->assertEquals(
[
'wcpay_upe_disabled' => [],
],
Tracker::get_admin_events()
);

$this->assertSame( '1', get_option( Track_Upe_Status::IS_TRACKED_OPTION ) );
}

Comment on lines -62 to -76
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to check for wcpay_upe_disabled anymore since UPE is always disabled now

public function test_do_nothing_default_on_upgrade() {
Track_Upe_Status::maybe_track();

Expand Down
72 changes: 0 additions & 72 deletions tests/unit/notes/test-class-wc-payments-notes-additional-payment-methods.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,78 +24,6 @@ public function tear_down() {
delete_option( '_wcpay_feature_upe' );
}

public function test_get_note() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the non-UPE related tests have been removed from this class, since the plugin now executes UPE by default

$note = WC_Payments_Notes_Additional_Payment_Methods::get_note();

$this->assertSame( 'Boost your sales by accepting new payment methods', $note->get_title() );
$this->assertSame( 'Get early access to additional payment methods and an improved checkout experience, coming soon to WooPayments. <a href="https://woo.com/document/woopayments/payment-methods/additional-payment-methods/" target="wcpay_upe_learn_more">Learn more</a>', $note->get_content() );
$this->assertSame( 'info', $note->get_type() );
$this->assertSame( 'wc-payments-notes-additional-payment-methods', $note->get_name() );
$this->assertSame( 'woocommerce-payments', $note->get_source() );

list( $enable_upe_action ) = $note->get_actions();
$this->assertSame( 'wc-payments-notes-additional-payment-methods', $enable_upe_action->name );
$this->assertSame( 'Enable on your store', $enable_upe_action->label );
$this->assertStringStartsWith( 'http://example.org/wp-admin/admin.php?page=wc-settings&tab=checkout&section=woocommerce_payments&action=enable-upe', $enable_upe_action->query );

/**
* The $primary property was deprecated from WooCommerce core. Keeping this to maintain the compatibility with old WooCommerce versions.
* @see https://github.com/woocommerce/woocommerce/blob/ff2d7d704a8f72aeb4990811b6972097aa167bea/plugins/woocommerce/src/Admin/Notes/Note.php#L623-L623.
* @see https://github.com/woocommerce/woocommerce-admin/pull/8474
*/
if ( isset( $enable_upe_action->primary ) ) {
$this->assertSame( true, $enable_upe_action->primary );
}
}

public function test_get_note_does_not_return_note_when_account_is_not_connected() {
$account_mock = $this->getMockBuilder( \WC_Payments_Account::class )->disableOriginalConstructor()->setMethods( [ 'is_stripe_connected' ] )->getMock();
$account_mock->expects( $this->atLeastOnce() )->method( 'is_stripe_connected' )->will( $this->returnValue( false ) );
WC_Payments_Notes_Additional_Payment_Methods::set_account( $account_mock );

$note = WC_Payments_Notes_Additional_Payment_Methods::get_note();

$this->assertNull( $note );
}

public function test_get_note_returns_note_when_account_is_connected() {
$account_mock = $this->getMockBuilder( \WC_Payments_Account::class )->disableOriginalConstructor()->setMethods( [ 'is_stripe_connected', 'is_account_partially_onboarded', 'is_progressive_onboarding_in_progress' ] )->getMock();
$account_mock->expects( $this->once() )->method( 'is_stripe_connected' )->willReturn( true );
$account_mock->expects( $this->once() )->method( 'is_account_partially_onboarded' )->willReturn( false );
$account_mock->expects( $this->once() )->method( 'is_progressive_onboarding_in_progress' )->willReturn( false );

WC_Payments_Notes_Additional_Payment_Methods::set_account( $account_mock );

$note = WC_Payments_Notes_Additional_Payment_Methods::get_note();

$this->assertSame( 'Boost your sales by accepting new payment methods', $note->get_title() );
}

public function test_get_note_returns_note_when_account_is_partially_onboarded() {
$account_mock = $this->getMockBuilder( \WC_Payments_Account::class )->disableOriginalConstructor()->setMethods( [ 'is_stripe_connected', 'is_account_partially_onboarded', 'is_progressive_onboarding_in_progress' ] )->getMock();
$account_mock->expects( $this->once() )->method( 'is_stripe_connected' )->willReturn( true );
$account_mock->expects( $this->once() )->method( 'is_account_partially_onboarded' )->willReturn( true );

WC_Payments_Notes_Additional_Payment_Methods::set_account( $account_mock );

$note = WC_Payments_Notes_Additional_Payment_Methods::get_note();

$this->assertNull( $note );
}

public function test_get_note_returns_note_when_account_is_progressive_in_progress() {
$account_mock = $this->getMockBuilder( \WC_Payments_Account::class )->disableOriginalConstructor()->setMethods( [ 'is_stripe_connected', 'is_account_partially_onboarded', 'is_progressive_onboarding_in_progress' ] )->getMock();
$account_mock->expects( $this->once() )->method( 'is_stripe_connected' )->willReturn( true );
$account_mock->expects( $this->once() )->method( 'is_account_partially_onboarded' )->willReturn( false );
$account_mock->expects( $this->once() )->method( 'is_progressive_onboarding_in_progress' )->willReturn( true );

WC_Payments_Notes_Additional_Payment_Methods::set_account( $account_mock );

$note = WC_Payments_Notes_Additional_Payment_Methods::get_note();

$this->assertNull( $note );
}

public function test_maybe_enable_feature_flag_redirects_to_onboarding_when_account_not_connected() {
$account_mock = $this->getMockBuilder( \WC_Payments_Account::class )->disableOriginalConstructor()->setMethods( [ 'is_stripe_connected', 'redirect_to_onboarding_welcome_page' ] )->getMock();
$account_mock->expects( $this->atLeastOnce() )->method( 'is_stripe_connected' )->will( $this->returnValue( false ) );
Expand Down
8 changes: 0 additions & 8 deletions tests/unit/notes/test-class-wc-payments-notes-set-up-stripelink.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,6 @@ public function test_stripelink_setup_get_note() {
$this->assertStringStartsWith( 'https://woo.com/document/woopayments/payment-methods/link-by-stripe/', $set_up_action->query );
}

public function test_stripelink_setup_note_null_when_upe_disabled() {
$this->mock_gateway_data( '0', [ 'card', 'link' ], [ 'card' ] );

$note = \WC_Payments_Notes_Set_Up_StripeLink::get_note();

$this->assertNull( $note );
}

Comment on lines -58 to -65
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is irrelevant going forward due to UPE being always enabled

public function test_stripelink_setup_note_null_when_link_not_available() {
$this->mock_gateway_data( '1', [ 'card' ], [ 'card' ] );

Expand Down
Loading
Loading