-
Notifications
You must be signed in to change notification settings - Fork 216
Making OCS enabled by default for new installs #4811
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.
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
base: develop
Are you sure you want to change the base?
Making OCS enabled by default for new installs #4811
Conversation
| $options['enabled'] = 'yes'; | ||
| $options['testmode'] = $is_test ? 'yes' : 'no'; | ||
| $options['upe_checkout_experience_enabled'] = $this->get_upe_checkout_experience_enabled(); | ||
| $options['optimized_checkout_element'] = $this->get_optimized_checkout_element_default_value(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will keep thinking about this tomorrow, but it feels like it might be really good to add some unit tests so we can correctly track what is going on here and what edge cases might exist.
I am concerned that we might have edge cases where merchants upgrade, and then reconnect, and we auto-enable Optimized Checkout just because we hadn't saved any data in that sub-option from before that reconnection attempt.
Might it be better to create some code in WC_Stripe::install() and/or hook into the woocommerce_stripe_updated action to set/pick the default value for optimized checkout based on whether there is data in the wc_stripe_version option from before? That way OCS would be default-on for fresh new installs only, whereas we'd leave it off for any other install.
Mayisha
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good and is working well as per my testing in JN sites. 👍
-
New installations
- Directly installed this branch build in a fresh JN site.
- Connected to a test Stripe account.
- OCS is enabled after account connection.
-
Existing account
- Installed Stripe
10.1.0in a fresh site. - Connected to a test Stripe account.
- Changed a few settings keeping OCS disabled.
- Updated the plugin with this branch's build.
- Refreshed settings page -> OCS disabled.
- Reconnected to the Stripe account -> OCS disabled.
- Disconnected the Stripe account, connected again -> OCS disabled.
- Installed Stripe
On the topic of tracking possible edge cases, can we add some logs when we auto enable OCS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just confirmed that the current logic will not work as expected for upgrades from versions where Optimized Checkout was not initialised by default, and then the merchant reconnects.
I specifically tested the following:
- Install Stripe 9.5.2 on a fresh site
- Connect to Stripe
- Verify that the
optimized_checkout_elementsub-option is empty --wp option pluck woocommerce_stripe_settings optimized_checkout_element- I also confirm that sync is enabled, but that may not be relevant --
wp option pluck woocommerce_stripe_settings pmc_enabledshould showyes
- I also confirm that sync is enabled, but that may not be relevant --
- Upgrade to a Stripe build from this branch
- Verify that the
optimized_checkout_elementsub-option is still empty --wp option pluck woocommerce_stripe_settings optimized_checkout_element - Reconnect to Stripe
- Verify that the
optimized_checkout_elementsub-option is now set toyes--wp option pluck woocommerce_stripe_settings optimized_checkout_element
|
Good catch, Dale! Thanks. I have updated the main implementation in 3ee6a77. I have also included unit tests specific for this behavior. |
includes/class-wc-stripe.php
Outdated
| unset( $stripe_settings['pmc_enabled'] ); | ||
| WC_Stripe_Helper::update_main_stripe_settings( $stripe_settings ); | ||
| WC_Stripe_Logger::warning( 'Settings synchronization eligibility will be re-checked after upgrade' ); | ||
| } else if ( 'yes' === $stripe_settings['pmc_enabled'] ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach also has issues. If we have a merchant who has settings sync enabled but hasn't had the optimized_checkout_element sub-option set, upgrading will now result in Optimized Checkout being enabled. AFAICT, we don't have any logic that's only going to run for a new/fresh install.
I had a fairly different thought about how to approach this much earlier in install():
- Get current value of
wc_stripe_version(which we're already doing for the comparison againstWC_STRIPE_VERSION) - If the value is empty, we have a new install, so set a new option (or maybe sub-option) like
wc_stripe_optimized_checkout_default_ontoyes - Leave all other logic in this code as-is -- we only want to take action when we (re)enable settings sync
- Then in
WC_Stripe_Payment_Method_Configurations::maybe_migrate_payment_methods_from_db_to_pmc(), if we execute the code path where we setpmc_enabledtoyes, also setoptimized_checkout_elementtoyesif it wasn't previously set.
I am planning to do something very similar for enabling Amazon Pay on fresh installs as well. Let me know if it would help for me to put together an initial PR. I am now thinking that it could be worth creating a helper method like init_on_new_install() that handles initialising some options for managing default behaviours like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I'd appreciate it if you could open another PR then. We can move forward from there 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is that if we run the code in install just for new installations (wc_stripe_version is false), pmc_enabled will be undefined, and we will never set wc_stripe_optimized_checkout_default_on there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we can do is set a flag/constant when we identify a fresh install, and proceed with the settings update in maybe_migrate_payment_methods_from_db_to_pmc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That wouldn't work either, because the install and the connection method are called different in different flows 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a refactor using a new flag in options to identify new installs. See 7f2b87d. Let me know what you think. It worked for me
Fixes STRIPE-818
Changes proposed in this Pull Request:
As originally envisioned, with this PR, we are making the Optimized Checkout Suite feature enabled by default for all new installations. This is to make sure merchants are using the latest integration recommended by Stripe.
Testing instructions
update/making-ocs-enabled-by-default-for-new-installs)Changelog entry
Changelog Entry Comment
Comment
Post merge