woocommerce / google-listings-and-ads

Sync your store with Google to list products for free, run paid ads, and track performance straight from your store dashboard.
https://woo.com/products/google-listings-and-ads/
GNU General Public License v3.0
45 stars 21 forks source link

Hides jetpack during onboarding if already connected #2516

Closed dsawardekar closed 3 weeks ago

dsawardekar commented 1 month ago

Changes proposed in this Pull Request:

This PR improves the plugin setup process by conditionally hiding the WP.com account connection card when the store is already connected to WP.com.

Closes: https://github.com/woocommerce/google-listings-and-ads/issues/2487

Screenshots:

Disconnected: image

Connected: image

Settings: image

Detailed test instructions:

  1. Switch to this branch, and rebuild the plugin with npm run build
  2. With Jetpack not connected, go to the onboarding flow.
    • You should see the WP.com / Jetpack Connection Card
  3. Connect Jetpack, and then refresh the page to restart the onboarding steps
    • The WP.com / Jetpack Connection Card should be hidden
  4. Verify that the WP.com card is still shown on the settings tab.

You can use the below code in an mu-plugin to temporarily test this by toggling the active state to 'yes' / 'no'.

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 );

Additional details:

Update - Hides WP.com card if already connected

To run the e2e tests for the onboarding process you can use,

$ npm run test:e2e -- tests/e2e/specs/setup-mc/step-1-accounts.test.js'

This PR also includes some refactoring of the E2E test playwright utilities to avoid relying on nth-child locations for account cards, now that the WP.com account card will not be displayed in some scenarios.

Changelog entry

Update - Hides WordPress.com account connection setting from the onboarding flow if already connected.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 63.8%. Comparing base (f398161) to head (ecd25fc). Report is 149 commits behind head on feature/2458-streamline-onboarding.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2516/graphs/tree.svg?width=650&height=150&src=pr&token=UROWUPF1LX&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce)](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2516?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) ```diff @@ Coverage Diff @@ ## feature/2458-streamline-onboarding #2516 +/- ## ======================================================================= - Coverage 64.5% 63.8% -0.7% ======================================================================= Files 795 326 -469 Lines 22844 5086 -17758 Branches 1220 1229 +9 ======================================================================= - Hits 14739 3247 -11492 + Misses 7938 1672 -6266 Partials 167 167 ``` | [Flag](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2516/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | Coverage Δ | | |---|---|---| | [js-unit-tests](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2516/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | `63.8% <ø> (+0.3%)` | :arrow_up: | | [php-unit-tests](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2516/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#carryforward-flags-in-the-pull-request-comment) to find out more. [see 479 files with indirect coverage changes](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2516/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce)
dsawardekar commented 1 month ago

@joemcgill I've refactored the code per the feedback. Also confirmed that settings still show the card, AC is also updated to reflect this. Back to you for another review.

dsawardekar commented 1 month ago

Thanks, @joemcgill . I've merged the PR, and updated the AC to remove the non-mocking based instructions.

ankitguptaindia commented 4 weeks ago

@joemcgill, I have reviewed the PR and found it to be functioning as described ✅. However, I came across a use case where I would appreciate your feedback:

Current Behavior: When a user completes the onboarding flow, the WordPress.com card is hidden if Jetpack is connected, and the status is displayed as "connected" on the plugin’s Settings tab.

Use Case: The button on the Settings screen labelled "Disconnect from all accounts" suggests that clicking it will disconnect all accounts, including the WordPress.com account. Furthermore, when the user clicks this button, a confirmation message (e.g., "I understand that I am disconnecting any WordPress.com account...") appears, reinforcing that the WordPress.com account will be disconnected.

Given this, a user might expect the WordPress.com card to reappear on the onboarding screen when they restart the plugin setup. However, with the current implementation, the WordPress.com card does not appear on the first screen of the onboarding steps after all accounts have been disconnected.

This discrepancy between the expected behaviour and the label/button details could cause some confusion for users.

https://github.com/user-attachments/assets/cb41c87d-0a6c-4235-9adc-77c488ddcb1a

Disconnection Screens ![image](https://github.com/user-attachments/assets/117a8b22-1742-445d-a96f-10becb0a2dde) ![image](https://github.com/user-attachments/assets/c3973d58-2925-48b5-bb7d-4a12d05d6a6c)

Additional Note: I tested the same flow with the stable build and found that, when the user disconnects all accounts and restarts the onboarding journey, the WordPress.com card appears on the onboarding screen with a "CONNECTED" status.

Screenshot ![image](https://github.com/user-attachments/assets/271bb1f5-af85-48fc-9534-09e911b11ea2)
joemcgill commented 4 weeks ago

Thanks @ankitguptaindia! Re:

Use Case: The button on the Settings screen labelled "Disconnect from all accounts" suggests that clicking it will disconnect all accounts, including the WordPress.com account. Furthermore, when the user clicks this button, a confirmation message (e.g., "I understand that I am disconnecting any WordPress.com account...") appears, reinforcing that the WordPress.com account will be disconnected.

This behavior is something that both @dsawardekar and I noticed as well. This seems to be the current behavior in the develop branch and not something related to the changes made here. I'm going to create a new issue to address this.

joemcgill commented 4 weeks ago

@eason9487 this one is ready for your review.

joemcgill commented 3 weeks ago

Re: https://github.com/woocommerce/google-listings-and-ads/pull/2516#issuecomment-2291417223

Use Case: The button on the Settings screen labelled "Disconnect from all accounts" suggests that clicking it will disconnect all accounts, including the WordPress.com account. Furthermore, when the user clicks this button, a confirmation message (e.g., "I understand that I am disconnecting any WordPress.com account...") appears, reinforcing that the WordPress.com account will be disconnected.

This behavior is something that both @dsawardekar and I noticed as well. This seems to be the current behavior in the develop branch and not something related to the changes made here. I'm going to create a new issue to address this.

I'm no longer seeing this behavior today and the WP.com connection is being disconnected correctly when clicking "Disconnect all accounts" so this may have been due to a local configuration issue.

dsawardekar commented 3 weeks ago

@joemcgill PR updated, back to you for CR.

RE: Disconnection

I'm still seeing the disconnection issue. It appears to disconnect correctly, but shows an error on the FE. But if you refresh the page, everything except the Jetpack connection has disconnected. I'm currently using the jetpack REST override to reset the state. If there is a better way, please let me know.