-
Notifications
You must be signed in to change notification settings - Fork 21
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
Onboarding: Hide WP.com connection card if already connected #2487
Comments
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. |
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. 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. |
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. |
Locally replacing line 163 of the following file with google-listings-and-ads/src/API/Site/Controllers/Jetpack/AccountController.php Lines 161 to 163 in a8e9cac
Another way without code modification can be found in the Steps to reproduce of issue #1972. |
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. google-listings-and-ads/js/src/components/wpcom-account-card/connect-wpcom-account-card.js Lines 37 to 40 in a8e9cac
|
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? |
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! |
@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. |
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? |
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. |
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. |
@dsawardekar reported that the filter I suggested in #2487 (comment) 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 ); |
…e-tests Update e2e tests for #2487
Part of #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
WPComAccountCard
component should be shown in a disconnected state.WPComAccountCard
should be hidden from the "setup your accounts screen".WPComAccountCard
is hidden, theGoogleAccountCard
should be enabled.WPComAccountCard
when connected.Implementation Brief
To test the WP.com card in a disconnected state see #2487 (comment) and #2487 (comment)
The WP.com status is determined in the
SetupAccounts
component defined injs/src/setup-mc/setup-stepper/setup-accounts/index.js
by using theuseJetpackAccount()
hook. Thejetpack
value is passed to theWPComAccountCard
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 theWPComAccountCard
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 theWPComAccountCard
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:WPComAccountCard
is visible and disconnected initially.WPComAccountCard
is hidden in any tests where the WP.com connection is mocked in thetest.beforeAll()
method.The text was updated successfully, but these errors were encountered: