Enable deferred intent creation when initialization process encounters cache unavailability #7686
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
timur27
changed the title
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
timur27
commented
Nov 9, 2023
Comment on lines
-62
to
-76
| 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 ) ); | ||
| } | ||
|
|
There was a problem hiding this comment.
No need to check for wcpay_upe_disabled anymore since UPE is always disabled now
timur27
commented
Nov 9, 2023
| @@ -24,78 +24,6 @@ public function tear_down() { | |||
| delete_option( '_wcpay_feature_upe' ); | |||
| } | |||
|
|
|||
| public function test_get_note() { | |||
There was a problem hiding this comment.
All the non-UPE related tests have been removed from this class, since the plugin now executes UPE by default
timur27
commented
Nov 9, 2023
Comment on lines
-58
to
-65
| 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 ); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is irrelevant going forward due to UPE being always enabled
timur27
commented
Nov 9, 2023
| /** | ||
| * @dataProvider maybe_filter_gateway_title_data_provider | ||
| */ | ||
| public function test_maybe_filter_gateway_title_with_no_additional_feature_flags_enabled( $data ) { |
There was a problem hiding this comment.
Filtering occurs for non-deferred UPE only, so this test is not needed anymore as it will fail because of UPE enabled by default.
timur27
commented
Nov 9, 2023
| public function test_payment_fields_outputs_fields() { | ||
| $this->wcpay_gateway->payment_fields(); | ||
|
|
||
| $this->expectOutputRegex( '/<div id="wcpay-card-element"><\/div>/' ); |
There was a problem hiding this comment.
No wcpay-card-element is rendered anymore. Instead, the UPE gateway test tests the correct input element.
|
Size Change: 0 B Total Size: 1.44 MB ℹ️ View Unchanged
|
timur27
commented
Nov 9, 2023
|
|
||
| public function test_update_settings_validation_fails_if_invalid_gateway_id_supplied() { |
There was a problem hiding this comment.
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()
timur27
force-pushed
the
fix/more-use-cases-for-enabled-dupe
branch
from
60b5c5f to
c89c113
Compare
timur27
force-pushed
the
fix/more-use-cases-for-enabled-dupe
branch
8 times, most recently
from
601d9c9 to
e0e85b2
Compare
timur27
force-pushed
the
fix/more-use-cases-for-enabled-dupe
branch
9 times, most recently
from
51bdede to
e0e85b2
Compare
timur27
force-pushed
the
fix/more-use-cases-for-enabled-dupe
branch
from
6f0bc10 to
e95b783
Compare
timur27
commented
Nov 16, 2023
| @@ -98,6 +98,9 @@ function() { | |||
| require_once $_plugin_dir . 'includes/admin/class-wc-rest-payments-payment-intents-controller.php'; | |||
| require_once $_plugin_dir . 'includes/class-woopay-tracker.php'; | |||
| require_once $_plugin_dir . 'includes/admin/class-wc-rest-payments-customer-controller.php'; | |||
|
|
|||
| // Load currency helper class early to ensure its implementation is used over the one resolved during further test initialization. | |||
| require_once __DIR__ . '/helpers/class-wc-helper-site-currency.php'; | |||
There was a problem hiding this comment.
More context on this change in p1700139623363929-slack-CU6SYV31A
timur27
requested review from
a team and
frosso
and removed request for
a team
frosso
approved these changes
Nov 17, 2023
| @@ -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 ); | |||
There was a problem hiding this comment.
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? 😵
There was a problem hiding this comment.
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.
frosso
approved these changes
Nov 17, 2023
|
Performed a quick successful sanity check after abc7347. Merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #7482
Changes proposed in this Pull Request
When checking the state of the feature flag for the deferred intent creation UPE, sometimes the cache isn't available which makes the
WC_Payments_Featuresreturn false. If with this setup, a store has legacy UPE enabled, legacy UPE takes over the initialization (instead of initializing deferred UPE as one would expect), eventually leading to unpredictable state of the registered gateways. This is what the author of #7482 faced.The main change in this PR is to enable deferred UPE when the cache is unavailable for some reason. Cache being unavailable is a rare/edge-case scenario, and the changes in this PR are not negatively impacting the scenario when we disable the dUPE feature flag for a merchant. The flag state is correctly propagated to the plugin if we do so.
While the unit test suite is initialized, it checks UPE flags. Previously, while checking the flag, all the UPEs were disabled, and thus, the legacy gateway was initialized for tests. Now, when we run dUPE in case of a non-available cache, tests start to run deferred intent creation UPE, which led to multiple failures that this PR fixes. I encourage to review the tests in more detail. Some tests were removed, some were replaced, and some context is left in the comments.
Testing instructions
On
developfix/more-use-cases-for-enabled-dupegateway_typefield in the Metadata of payment details on the Stripe dashboard)npm run test:phpon your local to ensure that the changes inbootstrap.phpdon't impact the setup negatively.npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge