Simplify and harden our onboarding [connect] links and Connect page state logic #9105
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
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. |
|
Size Change: +526 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
@timmy5685 That is just the disabled primary button component. There is no custom styling that I can see. |
@timmy5685, that does match our button styling for disabled buttons, yes: more info on the button component here: https://github.com/WordPress/gutenberg/tree/trunk/packages/components/src/button |
|
thanks @vladolaru @orcunomattic - for some reason I thought we have a different style (greyed out or something) when a primary button is disabled. Realizing I was probably confusing that with the disabled secondary button style. Appreciate y'all confirming! |
|
@dmallory42 if you have the time for another round of smoke testing... very much appreciated. |
dmallory42
dismissed
oaratovskyi’s stale review
Oleks is currently AFK and changes he requested have been addressed
dmallory42
approved these changes
Aug 7, 2024
There was a problem hiding this comment.
Thanks a lot for the awesome work on this PR! I've smoke tested some more with different scenarios and everything is looking as expected. I think this is ready to go! 
A small suggestion, we should consider checking the critical flows to see if there's any other onboarding scenarios we want to add, just to be extra safe - or, if we have especially specific edge cases to test, we can add them to the release-specific testing instructions.
|
Thank you, @dmallory42 !
Good point! I will add a critical flow for onboarding from the WooCommerce Payments Settings. Will also look to update the testing instructions for the rest of the flows, where there are changes. |
vladolaru
deleted the
fix/9103-payments-settings-originated-onboarding-flow
branch
vladolaru
changed the title
rafaelzaleski
pushed a commit
that referenced
this pull request
Aug 27, 2024
Co-authored-by: Daniel Mallory <[email protected]>
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 #9103
Fixes #9101
Fixes #8990
Fixes #7803
Fixes #183
Fixes #6542
Fixes #9024
Yeah... I know these are quite a few, but that is what you get when you work on something long overdue.
Changes proposed in this Pull Request
The bullet points
fromandsourcein the URLs maintain/determine proper value even when coming from WooCommerce places where we have no control of the URLs usedfromandsourceget sent with Tracks events, when we have them.The story
I would say refactor, but it is more like reengineering our handling of connect links (via the
maybe_handle_onboardingmethod) to make it simpler, predictable, resilient, much better documented, and covered by unit tests (there were none before).Connect links are a catchall tool where we can throw merchants from anywhere into the onboarding flow, trusting the system to look at the current store state, maybe take some action, and point the merchant to the right place. Naturally, as we wanted to do more and provide more tailored experiences, the complexity has become too much to handle. This PR "cleans the room" and establishes code logic driven by business/product logic (and documents it).
At the same time, I tried to impose a clear separation between determining the
fromof a connect link and the onboardingsourceof the current onboarding flow. Conflating the two got it into trouble with the recent onboarding incident (paJDYF-ehK-p2).frommeans where the merchant was in the onboarding flow immediately before. It might mean a certain step, or a certain page, or a certain button on a page. Its goal is to guide us on what to do next.fromis not meant to be sticky and it should change throughout the onboarding flow, generally representing the previous step.sourcemeans the very top place the merchant started the onboarding flow (our TOFU, if you will).sourceis meant to be sticky and not change throughout the onboarding flow as it gets attached to our Tracks events.Another important concept that I tried to apply consistently is: when do we have a GOOD Stripe account to work with? Up until now, we've been dangling between
is_stripe_connectedandis_details_submitted(or a combination of the two). I believe theis_stripe_account_validlogic is the way to go! We are much more strict about the account state and will redirect to the connect page whenever the account is not in working condition (this doesn't mean pending verifications). A GOOD account:card_paymentsNOTunrequested)If either of these is not met, WooPayments can't work, and we should not show the Payments Overview page or any other page. This is building/expanding this PR by Oleks.
For our sanity, I went to our surfaces (in JS) and made sure that whenever some place (like a button) redirects folks to onboarding, it also declares where they are sending from and maintains the source. It is best to avoid automagically determining the from as much as possible (we have logic to fallback on referer and stuff, but it is best to be declarative about it because it has important implications).
Our JS Tracks events got a "splash" of conformity: they will send the
sourceandfromwhenever they can. There were some missing Tracks events around our tasks, modals, etc. Now we have them.Notes to PR reviewers:
maybe_handle_onboardingcode flow and documentation need to make sense to you. If something doesn't, point it out.Onboarding flows testing instructions
I think it is best to use short-lived JN stores (quick link to create with just WooCommerce) without WooPayments on the live-branch and instead upload the branch's built zip (you can find it via the
Prepare live branch for Jetpack Beta Builder / Build and inform the zip file (pull_request)CI job details >Upload the zip file as an artifactstep, but you will need to change the directory name in the zip to justwoocommerce-payments). This way all the logic that automatically attempts to install/activate the plugin (like Payments task, Incentive, or Payments Settings) will work and not error out.Also, try it mostly without the WCPay Dev Tools and dev mode enabled.
At each step in the onboarding flow, please make sure that the
sourceandfromGET params make sense. Thesourceparam should be very sticky, while thefromparam should point to the immediately previous step (not sticky) -fromis not necessarily pointing to a page, but also to a certain tool.As long as you stay in an onboarding flow, the
sourceshould remain the same. If you navigate away, reset the account, or move from test to live, the source will reset.Please stay on the lookout and report back any "You are not allowed to access this page" scenarios. There shouldn't be any.
Here are a couple of ideas:
Working to not working testing instructions
Start each testing scenario with a fully working setup: Jetpack connection working and a fully onboarded account.
/wp-admin/admin.php?page=wc-admin&path=%2Fpayments%2Fconnect&from=WCADMIN_PAYMENT_TASK) and you should be redirected to the Payments Overview page/wp-admin/admin.php?page=wc-admin&path=%2Fpayments%2Foverview) you should be redirected back to the Connect page./wp-admin/admin.php?page=wc-admin&path=%2Fpayments%2Fconnect) you should be redirected to the Payments Overview page.wcpay_account_dataoption) so it hascard_paymentsasunrequested(puts:13:"card_payments";s:11:"unrequested";in the serialized JSON)/wp-admin/admin.php?page=wc-admin&path=%2Fpayments%2Foverview) you should be redirected back to the Connect pagewcpay_account_dataoption) back to a valid state so it hascard_paymentsasactive(puts:13:"card_payments";s:6:"active";in the serialized JSON)/wp-admin/admin.php?page=wc-admin&path=%2Fpayments%2Fconnect), you should be redirected back to the Overview pagewcpay_account_dataoption) so it hascard_paymentsasunrequested(puts:13:"card_payments";s:11:"unrequested";in the serialized JSON) and go to the Connect page. Reset your account, and it should work and redirect you back to the Connect page with no error and the builder flow section visible:Multiple onboarding sessions testing instructions
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