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.

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

Improve redirects logic in payments #7654

Open
oaratovskyi opened this issue Nov 7, 2023 · 3 comments · May be fixed by #8590
Open

Improve redirects logic in payments #7654

oaratovskyi opened this issue Nov 7, 2023 · 3 comments · May be fixed by #8590
Assignees
Labels
category: core WC Payments core related issues, where it’s obvious. component: onboarding Onboarding merchant in KYC, dev vs live, etc focus: account lifecycle priority: medium The issue/PR is medium priority—non-critical functionality loss, minimal effect on usability size: medium The issue is sized medium. status: on hold The issue is currently not prioritized. type: technical debt This issue/PR represents/solves the technical debt of the project.

Comments

@oaratovskyi
Copy link
Contributor

Description

At the moment, we have quite complex redirects logic in WC_Payments_Account. First, we have a lot of hooks around the topic:

add_action( 'admin_init', [ $this, 'maybe_redirect_to_onboarding' ], 11 ); // Run this after the WC setup wizard and onboarding redirection logic.
add_action( 'admin_init', [ $this, 'maybe_redirect_to_wcpay_connect' ], 12 ); // Run this after the redirect to onboarding logic.
add_action( 'admin_init', [ $this, 'maybe_redirect_to_capital_offer' ] );
add_action( 'admin_init', [ $this, 'maybe_redirect_to_server_link' ] );
add_action( 'admin_init', [ $this, 'maybe_redirect_settings_to_connect' ] );

Second, there is a big function maybe_handle_onboarding that also contains logic around redirects, see a couple of examples (not all):

if ( ( $from_wc_admin_task || $from_wc_pay_connect_page ) ) {
	// Redirect complete accounts to payments overview page, otherwise to the onboarding flow.
	if ( $this->is_account_complete() ) {
		$this->redirect_to( static::get_overview_page_url() );
	} else {
		$this->redirect_to_onboarding_flow_page();
	}
}
...
if ( isset( $_GET['wcpay-disable-onboarding-test-mode'] ) ) {
    ...
    $this->redirect_to_onboarding_flow_page();
}

After conversation in #7652 (comment), we decided to normalize and reduce the redirect logic, so we don't get to pile on more conditional branches to the logic.
We should strive to have a stable, catch-all logic that would prevent any other such issues in the future (no more ending up on pages with "Sorry you are not allowed").

Dev ideas

  • Centralize logic around redirects in a class
  • Reduce the amount of hooks to one or two that are responsible for all kinds of redirects
  • Have extensive tests covering all possible scenarios
@oaratovskyi oaratovskyi added type: technical debt This issue/PR represents/solves the technical debt of the project. component: onboarding Onboarding merchant in KYC, dev vs live, etc labels Nov 7, 2023
@daquinons daquinons added category: core WC Payments core related issues, where it’s obvious. priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. labels Nov 17, 2023
@daquinons
Copy link
Member

Open to increase it to 5 estimation points if necessary to nail this

@daquinons daquinons added priority: medium The issue/PR is medium priority—non-critical functionality loss, minimal effect on usability and removed priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. labels Dec 7, 2023
@anu-rock anu-rock added the size: medium The issue is sized medium. label Mar 15, 2024
@oaratovskyi oaratovskyi self-assigned this Apr 2, 2024
@oaratovskyi oaratovskyi linked a pull request Apr 7, 2024 that will close this issue
6 tasks
@oaratovskyi oaratovskyi removed their assignment Apr 8, 2024
@oaratovskyi
Copy link
Contributor Author

I started thinking about the best solution but got a bit stuck and then PRD of Tracking v2 appeared which is due in April and I'm the DRI of one. I'm putting this on pause until the next cooldown.

@oaratovskyi oaratovskyi self-assigned this Apr 8, 2024
@anu-rock anu-rock added the status: on hold The issue is currently not prioritized. label Apr 12, 2024
@anu-rock
Copy link
Contributor

Moving it to our on-hold pipeline to keep track.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: core WC Payments core related issues, where it’s obvious. component: onboarding Onboarding merchant in KYC, dev vs live, etc focus: account lifecycle priority: medium The issue/PR is medium priority—non-critical functionality loss, minimal effect on usability size: medium The issue is sized medium. status: on hold The issue is currently not prioritized. type: technical debt This issue/PR represents/solves the technical debt of the project.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants