Refactor redirects logic in payments #8590
Conversation
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: 0 B Total Size: 1.25 MB ℹ️ View Unchanged
|
| add_action( 'admin_init', [ $this, 'maybe_redirect_onboarding_flow_to_connect' ] ); | ||
|
|
||
| add_action( 'admin_init', [ $this, 'maybe_redirect_after_plugin_activation' ], 11 ); // Run this after the WC setup wizard and onboarding redirection logic. | ||
| add_action( 'admin_init', [ $this, 'maybe_redirect_by_get_param' ], 12 ); // Run this after the redirect to onboarding logic. |
There was a problem hiding this comment.
reduced next actions that depend on GET param: maybe_redirect_to_wcpay_connect, maybe_redirect_to_capital_offer and maybe_redirect_to_server_link
to one action maybe_redirect_by_get_param
| add_action( 'admin_init', [ $this, 'maybe_redirect_after_plugin_activation' ], 11 ); // Run this after the WC setup wizard and onboarding redirection logic. | ||
| add_action( 'admin_init', [ $this, 'maybe_redirect_by_get_param' ], 12 ); // Run this after the redirect to onboarding logic. | ||
| add_action( 'admin_init', [ $this, 'maybe_redirect_from_settings_page' ] ); | ||
| add_action( 'admin_init', [ $this, 'maybe_redirect_from_onboarding_page' ] ); |
There was a problem hiding this comment.
reduced maybe_redirect_onboarding_flow_to_overview and maybe_redirect_onboarding_flow_to_connect to maybe_redirect_from_onboarding_page. we can add more logic about redirect from onboarding page to this action
| add_action( 'admin_init', [ $this, 'maybe_redirect_onboarding_flow_to_overview' ] ); | ||
| add_action( 'admin_init', [ $this, 'maybe_redirect_onboarding_flow_to_connect' ] ); | ||
|
|
||
| add_action( 'admin_init', [ $this, 'maybe_redirect_after_plugin_activation' ], 11 ); // Run this after the WC setup wizard and onboarding redirection logic. |
There was a problem hiding this comment.
maybe_redirect_to_onboarding -> maybe_redirect_after_plugin_activation to make it more readable
| } | ||
|
|
||
| $this->redirect_to_login(); | ||
| // Clear account transient when generating Stripe dashboard's login link. | ||
| $this->clear_cache(); |
There was a problem hiding this comment.
extracted this call from redirect_to_login function to remove dependency from WC_Payments_Account
| * | ||
| * @param string $location The URL to redirect to. | ||
| */ | ||
| protected function redirect_to( $location ) { |
There was a problem hiding this comment.
moved to Redirect_Service
| /** | ||
| * For the connected account, fetches the login url from the API and redirects to it | ||
| */ | ||
| private function redirect_to_login() { |
There was a problem hiding this comment.
moved to Redirect_Service
| * | ||
| * @see self::add_payments_menu() | ||
| */ | ||
| public function maybe_redirect_overview_to_connect() { |
There was a problem hiding this comment.
this function is not needed because this redirect (from overview to connect) is handled in a different way in maybe_redirect_from_payments_admin_child_pages, confirmed by manual testing
| /** | ||
| * @dataProvider data_maybe_redirect_overview_to_connect | ||
| */ | ||
| public function test_maybe_redirect_overview_to_connect( $expected_times_redirect_called, $is_wc_registered_page, $get_params ) { |
There was a problem hiding this comment.
this test is removed since the function was removed
| } | ||
|
|
||
| public function test_maybe_redirect_to_capital_offer_redirects_to_capital_offer() { | ||
| $request = $this->mock_wcpay_request( Get_Account_Capital_Link::class ); |
There was a problem hiding this comment.
moved to WC_Payments_Redirect_Service_Test
| $this->wcpay_account->maybe_redirect_to_server_link(); | ||
| } | ||
|
|
||
| public function test_maybe_redirect_to_server_link_redirects_to_overview_on_error() { |
There was a problem hiding this comment.
moved to WC_Payments_Redirect_Service_Test
| /** | ||
| * Data provider for test_maybe_redirect_onboarding_flow_to_overview | ||
| */ | ||
| public function data_maybe_redirect_onboarding_flow_to_overview() { |
There was a problem hiding this comment.
unique test cases reduced into data_maybe_redirect_from_onboarding_page
There was a problem hiding this comment.
Thanks for working on this! The logic is very tricky and it looks a lot more organised now after these changes.
I noticed one small error while testing, but everything else worked as expected. Details as follows:
When testing the final scenario, I got to the personal details step of the Stripe KYC then clicked "Return to WooPayments" on the KYC. This took me to a Sorry, you are not allowed to access this page. error. I guess this is because it is redirecting back to the Payments Connect page, but I think that this should redirect to the payments overview page (and I think it did in the past but can't say I'm 100% sure!)
Everything else worked great! Let's make sure this is merged so that GlobalStep can have a couple of rounds of testing before the next release. 🙂
@dmallory42 I couldn't reproduce this one |
|
Thanks for getting back to me! I was testing on a JN branch. As we spoke about on Slack, it looks like an issue which also happens in the current environment, so we agreed to create a follow-up issue to tackle it. |
|
@dmallory42 created the issue as agreed |
|
Addressed conflicts with develop after @vladolaru's changes |
Fixes #7654
Changes proposed in this Pull Request
Refactor logic of redirects in payments admin: centralize it in one place, remove duplications, cover with tests what uncovered.
Testing instructions
Pre-requisites
dev/7654-refactor-redirects-logic-in-paymentsMain flow
from=WCADMIN_PAYMENT_SETTINGSexists in the URL.from=WCPAY_ONBOARDING_FLOWexists in the URL.Builder flow
Redirect to capital offer
Redirect to server link
Redirect to continue Stripe KYC from KYC reminder
Bonus flow - this is also worth testing manually, but it's a bit trickier (since you'll need to recover the Jetpack connection with tube). We have unit tests covering this and I tested it manually, but sharing how to test it, too:
Shows error when no Jetpack connected
Please connect to WordPress.com to start using WooPayments..npm run tube:startand public site will allow you to pass the Jetpack step. After getting that, you can stop the tube withnpm run tube:stopnpm 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