Consolidate accounts onboarding: Merge the store address setting in Step 3 into Step 1 and remove Step 3 along with the phone verification

[asvinb]

Changes proposed in this Pull Request:

Closes #2602 .

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

Screenshots (Onboarding):

The store address is invalid (missing info)

The store address is not in sync with MC

The store address is in sync with MC
(nothing is shown during onboarding in this scenario)

Screenshots (Settings):

The store address is not in sync with MC

The store address is invalid (missing info)

The store address is in sync with MC

Detailed test instructions:

1. Go through the onboarding flow.
2. The card to sync the store address should be visible if the WooCommerce street address is different from the MC address.
3. Clicking the "Refresh to sync" button should sync the WooCommerce store street address to the MC account.
4. If the address is/has been synced, the card should not be displayed.
5. The "Continue" button should be disabled if there are connected accounts but the store address has not been synced.
6. If there is no address in WooCommerce settings, the card should display an alternate text to prompt the user to add the address in WC settings.

Additional details:

Changelog entry

Update - Merge the store address setting in Step 3 of the onboarding flow into Step 1 and remove Step 3 along with the contact phone verification.
Update - Remove the contact phone from the Settings page.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.5%. Comparing base (14c080e) to head (c77748c).
Report is 57 commits behind head on feature/2509-consolidate-google-account-cards.

Additional details and impacted files

@@                               Coverage Diff                               @@
##           feature/2509-consolidate-google-account-cards   #2653     +/-   ##
===============================================================================
- Coverage                                           62.7%   61.5%   -1.2%     
===============================================================================
  Files                                                326     320      -6     
  Lines                                               5166    4934    -232     
  Branches                                            1266    1207     -59     
===============================================================================
- Hits                                                3239    3036    -203     
+ Misses                                              1751    1730     -21     
+ Partials                                             176     168      -8     

Flag	Coverage Δ
js-unit-tests	61.5% <ø> (-1.2%)	⬇️

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

Files with missing lines	Coverage Δ
js/src/components/account-card/index.js	89.2% <ø> (-2.7%)	⬇️
js/src/data/action-types.js	100.0% <ø> (ø)
js/src/data/actions.js	8.6% <ø> (+0.7%)	⬆️
js/src/data/resolvers.js	7.1% <ø> (-0.5%)	⬇️
js/src/data/selectors.js	49.5% <ø> (+2.0%)	⬆️
.../src/setup-mc/setup-stepper/saved-setup-stepper.js	87.2% <ø> (-0.6%)	⬇️
js/src/setup-mc/setup-stepper/stepNameKeyMap.js	100.0% <ø> (ø)
js/src/utils/urls.js	52.6% <ø> (+0.1%)	⬆️

... and 1 file with indirect coverage changes

[joemcgill]

@asvinb this is looking pretty good. Only a couple of critical issues to address.

[joemcgill]

This hook cannot be used here until after the MC account is connected. Otherwise it will send an API request to /gla/mc/contact-information which will result in a 401 error.

Either we need to incorporate useGoogleMCAccount() into the useStoreAddressSynced() hook so that it returns false early if there is no MC account, or rely on a useState() value that gets updated after the verification is made.

[asvinb]

Thanks @joemcgill . Completely missed that 🤦 . I've updated the useStoreAddressSynced hook. Let me know what you think.

[joemcgill]

Instead of checking data.address here, which is only part of the address, it's probably better to check the data.isAddressFilled property, which checks for all required address fields. See:

google-listings-and-ads/js/src/hooks/useStoreAddress.js

Line 67 in 99408e1

const isAddressFilled = ! missingRequiredFields.length;

[asvinb]

With the recent changes in the hook, we are now using ! missingRequiredFields.length directly @joemcgill

[joemcgill]

The functional changes look good to me, except one note inline.

However, I noticed that the new design are slightly more streamlined, avoiding the separator between the Card description and the Address:

Old

New (ignore the missing button)

[joemcgill]

Adding this refetch is unnecessary, because updateGoogleMCContactInformation() returns the new data and updates the data store.

[asvinb]

👍

[joemcgill]

Thanks @asvinb.

Overall, this is working as expected for the onboarding flow. However, the need to add an additional compactStyles prop to render a style variation on the StoreAddressCard component made me curious about where else this component was used, and whether we could avoid the need to the prop entirely.

Turns out, the other place this component is used is in the EditStoreAddress flow that you can get to by visiting wp-admin/admin.php?page=wc-admin&subpath=%2Fedit-store-address&path=%2Fgoogle%2Fsettings or by clicking the Edit button on the StoreAddressCardPreview component shown in the settings screen after finishing onboarding.

In the develop branch, when you visit the EditStoreAddress flow, the Refresh button is used to update the component with fresh address data from the WC store that is shown in the lower Section.Card.Body, and the the "Save details" CTA button for that flow is what syncs the address to MC. After these changes, the refresh button does the syncing but then stays continually stuck in an updating state, see this video:

Screen.Recording.2024-10-29.at.3.39.10.PM.mov

Currently in develop the Refresh button similarly is used to update changes to the address that have been made in the WC store that aren't yet reflected in the component state and the address is synced to Google MC when that step of the onboarding flow is completed (see this code ref).

Suggestions

Here are my suggestions for how to address these concern. I can verify which option we want to go with, but would appreciate any ideas/feedback you have as well.

Updating the onboard flow

First, we should apply the new compact design everywhere to simplify the implementation and remove the new compactStyles prop.

Next, we revert the changes to the Refresh button so they are used to update the component with local data rather than sync the data to MC for a consistent UI in all places it's used. We can sync the changes to MC when the first onboarding step is completed, as part of the callback passed to the onClick prop of the AppButton here.

During onboarding, we should still only show this component if we either don't have valid address info or the address info doesn't match MC. However, since we'll now sync the data automatically whenever someone finishes this step, we should only disable the Continue button if there are any validation errors for the address (e.g., ! address.isAddressFilled) and show the validation errors in the StoreAddresCard similar to how it's being done in the EditStoreAddress flow:

<StoreAddressCard
  showValidation={ ! address.isAddressFilled }
/>

Alternate: Update the settings screen

An alternative suggestion would be to move the updated StoreAddressCard from this PR directly to the Settings screen—replacing the current ContactInformationPreview component here. With this approach, we would always show the address info (not just when it's different from MC), and the Refresh button would be for the purpose of updating the MC data.

The EditStoreAddress flow will no longer be needed and could be removed. The PhoneNumberCardPreview and StoreAddressCardPreview components could likely be removed entirely as well, because I don't think they're used anywhere else.

[joemcgill]

@eason9487 Thanks for the detailed review! I think I've addressed all your concerns.

@mikkamp Thanks for reviewing as well! re:

Not a big deal, but I noticed that MerchantTrait::get_valid_business_info still sets a valid phone number and marks it as verified. This is also still checked in ContactInformationTest::test_get_valid_contact_information. Since we no longer validate a phone number we don't need to keep this around.

I've addressed this in 866e7b6.

This part isn't really related to the PHP, however I noticed that loading the account information step is rather slow (in particular for me because I have a large amount of linked test accounts). On one of the requests to ads/accounts the request timed out. This timed out request is not visible but it leaves the user in a state where they can not reconnect to a new ads account.

Good call out. Given that the calls to ads/accounts isn't related to this specific PR, I'll look into handling it in a follow-up PR before #2509 is wrapped up.

[eason9487]

Thanks for the work.

I pushed a commit for updating the package-lock.json file (ref: #2653 (comment)), and there seems to be a typo in JSDoc.

I believe the frontend part won't need another round. I will be approving in advance.

[mikkamp]

Approving the PHP changes in this PR. All code for removing the phone number verification has been removed and the address validation is confirmed for an earlier step.

Note: unit tests in the PR are failing, but these are related to WP 6.7 changes which have been fixed in the main branch.