Onboarding: Hide WP.com connection card if already connected

joemcgill commented 1 month ago

Part of https://github.com/woocommerce/google-listings-and-ads/issues/2458

When a user is going through the plugin setup process, they are shown a list of accounts that need to be connected, the first of which is the required connection to WP.com. However, whenever the store is already connected to WP.com, this account card is unnecessary and can be hidden.

Acceptance Criteria

[ ] When the store is not connected to a WP.com account, the current WPComAccountCard component should be shown in a disconnected state.
[ ] When the WP.com account is already connected, the WPComAccountCard should be hidden from the "setup your accounts screen".
[ ] When the WPComAccountCard is hidden, the GoogleAccountCard should be enabled.
[ ] On completion of onboarding, the linked accounts on the plugin settings page should still show the connected WPComAccountCard when connected.

Implementation Brief

To test the WP.com card in a disconnected state see https://github.com/woocommerce/google-listings-and-ads/issues/2487#issuecomment-2270527832 and https://github.com/woocommerce/google-listings-and-ads/issues/2487#issuecomment-2276122847

The WP.com status is determined in the SetupAccounts component defined in js/src/setup-mc/setup-stepper/setup-accounts/index.js by using the useJetpackAccount() hook. The jetpack value is passed to the WPComAccountCard component once loaded to determine whether it is shown in a connected or disconnected state.

If the WP.com state is not connected (i.e. jetpack?.active !== 'yes') then the WPComAccountCard should be shown so the user can set up their connection and so they see the WP.com card in the connected state once they've completed the setup.

If the initial WP.com state is already connected (i.e. jetpack?.active === 'yes') then the WPComAccountCard should not be rendered at all.

No changes should be made to the WPComAccountCard component itself, since it needs to be able to show both a connected and disconnected state for other parts of the application.

Test Coverage

The E2E tests in tests/e2e/specs/setup-mc/step-1-accounts.test.js should be updated to show:

The WPComAccountCard is visible and disconnected initially.
The WPComAccountCard is hidden in any tests where the WP.com connection is mocked in the test.beforeAll() method.

joemcgill commented 1 month ago

Hi @mikkamp and @eason9487. I'm assigning this and other issues related to #2460 to you both for review. For this and other tickets, please confirm that the approach described in the Implementation Brief section makes sense and address anything in the Definition Questions section that you are able to. Once you're happy with the issue, you can feel free to reassign to me so we can plan it into one of our team's sprints.

fblascogarma commented 1 month ago

Hi @joemcgill , about "1. The need to show the connected WP.com card is a suggestion in order to provide user feedback when connecting that the connection succeeded. Does this approach make sense to others?": the idea is to only show the WP card if users don't have it connected yet to their Woo account. Most users already did this when onboarding to Woo. In those cases, remove the card entirely. No need to show them that connection. However, if users don't have a WP account connected to Woo yet, then yes let's keep the card so they can do it. Once they successfully connected it, let's keep the card to show users it has been done. Let me know if this clarifies that doubt.

For "2. When testing, I couldn't disable the WP.com connection, even when using the "disconnect all" button from the settings screen. Is there a trick to be able to test this properly?": The WP connection is used for Woo core product, so it's okay that this cannot be disabled on the Google plugin. I'll talk with Matthias tomorrow during my weekly sync just to double check and see if he has another view. If he does, I'll let you know.

joemcgill commented 1 month ago

Thanks for confirming my hunch about showing the WP.com connection in the rare cases that this is the first time someone is making that connection, @fblascogarma. The second question is mostly so we can test this functionality appropriately, I think @mikkamp is probably best suited to answer that one, so we can just wait for his feedback.

eason9487 commented 1 month ago

2. When testing, I couldn't disable the WP.com connection, even when using the "disconnect all" button from the settings screen. Is there a trick to be able to test this properly?

Locally replacing line 163 of the following file with $this->manager->remove_connection( true, true ); should be able to do so.

https://github.com/woocommerce/google-listings-and-ads/blob/a8e9cac5cace2cb24efb076427308d1b16ce69cf/src/API/Site/Controllers/Jetpack/AccountController.php#L161-L163

Another way without code modification can be found in the Steps to reproduce of issue #1972.

eason9487 commented 1 month ago

Regarding the Implementation Brief, since the webpage will be redirected to an external page for authorization confirmation, this may not be able to implement the first one of the Definition Questions using only React's runtime states.

https://github.com/woocommerce/google-listings-and-ads/blob/a8e9cac5cace2cb24efb076427308d1b16ce69cf/js/src/components/wpcom-account-card/connect-wpcom-account-card.js#L37-L40

joemcgill commented 1 month ago

Thanks @eason9487! With that hint, I found another way we can disable the WP.com connection for testing that doesn't require any code modification:

/**
 * Mock WordPress.com connection status.
 */
function gla_disable_jetpack( $value, $name ) {
    if ( 'id' == $name ) {
        return false;
    }

    return $value;
}
add_filter( 'jetpack_options', 'gla_disable_jetpack', 10, 2 );

This will show the site as disconnected when the onboarding flow first runs and then you have to disable the filter before trying to connect or else the connection will fail.

Now that I can test the flow, I see what you're saying about the redirect away from the page while doing the WP.com connection. Perhaps we can save the initial connection state to session storage, otherwise I think we should probably drop the second AC requirement entirely since the user will not need instant feedback that the connection was successful. What do you think @fblascogarma?

fblascogarma commented 1 month ago

Hi @joemcgill , why users will not need instant feedback that the connection was successful? Is it because the screen on the redirect already shows them that?

Once that is done, users need to click something to go back to our onboarding or they're redirected?

What happens if users are redirect to connect their WP accounts and they don't authorize? Will they be redirect to our onboarding but not have WP connected?

Do we have a logic implemented to only allow users with WP account connected to move forwards?

And the dev effort to implement the connection status after the redirect is higher than expected? Is it significant?

Thank you!

joemcgill commented 1 month ago

@fblascogarma It's probably easier to demonstrate the current user experience:

You can see in the video that the tab navigates away from the onboarding step to complete the WP.com setup. After it's completed the onboarding setup is reloaded, and will show that WP.com is already connected. We don't have a way of knowing when the account is already connected whether the user just completed the connection or if they previously had a connection.

We could probably come up with a way of trying to show the connected WP.com card if we suspect the onboarding flow is being started for the first time after connecting to WP.com, but I'm proposing that we avoid the added complexity for now and iterate in a follow-up ticket if we think optionally showing the WP.com card in a connected state is important when the user returns.

fblascogarma commented 1 month ago

I'm not a fan but if that lowers complexity significantly, then I'm okay.

To double check, users will not be able to move forwards if they don't have a WP account connected, right?

joemcgill commented 1 month ago

To double check, users will not be able to move forwards if they don't have a WP account connected, right?

Correct. If they don't have a WP account connected they will still see the WP.com connection card with the button to connect, and will not be able to move forward until they've done so.

joemcgill commented 1 month ago

To unblock this ticket, I've gone ahead and removed the requirement for showing the WP.com card in a connected state the first time it's connected during onboarding and will open a follow-up ticket to capture that enhancement if we think it's necessary.

joemcgill commented 1 month ago

@dsawardekar reported that the filter I suggested in https://github.com/woocommerce/google-listings-and-ads/issues/2487#issuecomment-2276122847 didn't work for him (I'm guessing due to it being a new install) but was able to filter the REST API response instead. This may be better to use during testing.

add_filter( 'rest_pre_echo_response', function( $result, $server, $request ) {
    if ( '/wc/gla/jetpack/connected' === $request->get_route() ) {
        $result['active'] = 'no';
    }

    return $result;
}, 10, 3 );

woocommerce / google-listings-and-ads