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

Onboarding: Create New Google Combo Accounts Card. #2601

Conversation

ankitrox
Copy link
Collaborator

@ankitrox ankitrox commented Sep 11, 2024

Changes proposed in this Pull Request:

Closes #2566 .

Replace this with a good description of your changes & reasoning.

Screenshots:

Screenshot 2024-09-11 at 5 53 28 PM

Detailed test instructions:

  1. Disconnect all Google accounts if already connected in the plugin.

  2. Start the onboarding process again.

  3. There should be Google card visible in the first step.

  4. The connect button would be disabled if Jetpack is not connected or Terms and conditions are not accepted.

  5. Clicking the "Connect" button initiates the current flow to connect a Google account and grant permissions

  6. After connecting the Google account successfully, the connected version of Google card should be visible.

Additional details:

Changelog entry

Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.7%. Comparing base (71fb4c7) to head (1b8940f).
Report is 19 commits behind head on feature/2590-consolidate-google-account-cards.

Additional details and impacted files

Impacted file tree graph

@@                              Coverage Diff                              @@
##           feature/2590-consolidate-google-account-cards   #2601   +/-   ##
=============================================================================
  Coverage                                           62.7%   62.7%           
=============================================================================
  Files                                                319     319           
  Lines                                               5059    5059           
  Branches                                            1229    1229           
=============================================================================
  Hits                                                3171    3171           
  Misses                                              1715    1715           
  Partials                                             173     173           
Flag Coverage Δ
js-unit-tests 62.7% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@ankitrox ankitrox changed the base branch from feature/2458-streamline-onboarding to feature/2458-consolidate-google-account-cards September 11, 2024 12:28
@ankitrox ankitrox changed the base branch from feature/2458-consolidate-google-account-cards to feature/2458-streamline-onboarding September 11, 2024 12:59
@joemcgill joemcgill linked an issue Sep 11, 2024 that may be closed by this pull request
7 tasks
@ankitrox ankitrox changed the base branch from feature/2458-streamline-onboarding to feature/2458-consolidate-google-account-cards September 11, 2024 13:58
Copy link
Collaborator

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is starting to look really good. I've left some feedback inline about the Terms & Conditions links that were previously used.

Copy link
Collaborator

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple more notes, but this is looking right to me besides updating the E2E tests. I think @ankitguptaindia could start some QA testing while the E2E updates are being made.

* @param {boolean} props.disabled
* @fires gla_google_account_connect_button_click with `{ action: 'authorization', context: 'reconnect' }`
* @fires gla_google_account_connect_button_click with `{ action: 'authorization', context: 'setup-mc' }`
* @fires gla_documentation_link_click with `{ context: 'setup-mc-accounts', link_id: 'required-google-permissions', href: 'https://woocommerce.com/document/google-for-woocommerce/get-started/setup-and-configuration/#required-google-permissions' }`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This accounts for the readMoreLink component. Will need to add two additional ones for the two new AppDocumentationLink components.

Copy link
Collaborator

@asvinb asvinb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ankitrox I left a few comments. Can you kindly check them out and let me know what you think please?
Thanks!

@ankitguptaindia
Copy link
Member

ankitguptaindia commented Sep 13, 2024

QA Test Report

I have completed the QA for this PR and confirmed that the requirements align with the details provided in the connected issue (FR ticket).
Below are the test cases I performed, all of which are functioning as expected:

  1. Google Accounts Card Disabled if Store Not Connected to WP.com - If the store is not connected to WP.com, the Google account card should be disabled. ✅

    Screenshot 2024-09-13 at 5 05 07 PM


  1. T&C Acceptance Required - The merchant should not be able to connect their account until the "Accept T&C" checkbox is checked. ✅

    Screenshot 2024-09-13 at 5 21 20 PM


  1. External Links Open in New Tabs - Merchant Center and Google Ads links should be open in new tabs. ✅

  1. Button Behavior After T&C Acceptance - After the merchant accepts the T&C checkbox, the 'Continue' button should be disabled, and only the 'Connect' button should be enabled. ✅

    Screenshot 2024-09-13 at 5 37 04 PM


  1. Access Screen for Existing Google Account - If the merchant uses an existing Google account that already has the required access, the access request screen shpuld not appear. ✅

  1. Access Request Message - If the merchant doesn't provide full access or only grants partial access, the card shows a message requesting full access to proceed. ✅

    Screenshot 2024-09-13 at 5 18 34 PM


  1. Access Screen for New Google Account If the merchant uses a new Google account, the access request screen should be appear. ✅

Quick Demo:

Recording.822.mp4

@ankitguptaindia
Copy link
Member

@ankitrox Can we align 'Ads' in the same line with 'Google'?

image

--

QA Note- Will test this PR again quickly after completion of CR.

@ankitrox
Copy link
Collaborator Author

@ankitrox Can we align 'Ads' in the same line with 'Google'?

image

--

QA Note- Will test this PR again quickly after completion of CR.

@ankitguptaindia This has been fixed with the new label prop for CheckboxControl component.

Copy link
Collaborator

@asvinb asvinb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ankitrox Changes LGTM. Just a one thing to fix here:
https://github.com/woocommerce/google-listings-and-ads/pull/2601/files#r1759081955
and you can send to QA.
Thanks!

@ankitguptaindia
Copy link
Member

ankitguptaindia commented Sep 16, 2024

QA/Test Report-

Testing Environment -

  • WordPress: 6.6.2
  • Theme active on store: Twenty Twenty-Four Version: 1.2
  • WooCommerce - Version 9.3.1
  • PHP: 8.3
  • Web Server: Nginx
  • Browser: Chrome - Version 128
  • OS: macOS Sonoma 14.6.1

Test Results - Verified all cases after completion of CR. PR changes working as described in AC.

Detailed Test cases status: #2601 (comment)

Functional Demo / Screencast -

Google Card workflow when has all permission:

Google.Connection.mp4

Google Card workflow when has limited permission:

G2.mp4

This PR is ready for the next step, we're discussing some minor concerns related to the button color status over Slack, Will update/create this/new PR if the issue is related to current changes.

Copy link
Member

@eason9487 eason9487 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work. Left some comments that may need to be addressed.

Furthermore:

@eason9487 eason9487 added the changelog: none Skip changelog entry for this PR label Sep 17, 2024
@ankitrox
Copy link
Collaborator Author

Thanks for the work. Left some comments that may need to be addressed.

Furthermore:

Thank you @eason9487 for reviewing the PR.

I've removed the redundant checks in e2e and also fixed the failing tests.

@asvinb
Copy link
Collaborator

asvinb commented Sep 18, 2024

Changes LGTM @ankitrox .
Assigning to @eason9487 to take another look.

Copy link
Member

@eason9487 eason9487 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates! LGTM.

There are two unnecessary styles in connect-google-combo-account-card.scss to be removed, and I believe the removal won't need another round of code reviews from Woo. I will be approving this RP in advance.

@ankitrox ankitrox merged commit 52a53f4 into feature/2590-consolidate-google-account-cards Sep 18, 2024
5 checks passed
@ankitrox ankitrox deleted the feature/2566-google-combo-card branch September 18, 2024 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: none Skip changelog entry for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Onboarding: Create New Google Combo Accounts Card
5 participants