Closed asvinb closed 1 week ago
codecov[bot]
commented
3 weeks ago All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 61.5%. Comparing base (
14c080e) to head (c77748c). Report is 57 commits behind head on feature/2509-consolidate-google-account-cards.
joemcgill
commented
2 weeks ago After confirming with @fblascogarma earlier this week, the approach we decided to go with for resolving this concern is to update the UI on the settings screen to make use of the redesigned StoreAddressCard and greatly simplify the UI there to remove the unnecessary phone verification data, and the edit store address flow. Those updates were implemented in #2661 and merged to this PR for finalization.
One additional issue that we've discovered is that currently when the onboarding flow is loaded, a request is made to mc/setup to return the step that should be started if requirements of the setup flow have already been completed. That logic is handled in the MerchantCenterService::get_setup_status() method (ref). This logic doesn't currently account for the need to check for a synced address before moving to the product_listings step so that will need to be resolved. I think we can add $this->is_mc_contact_information_setup() back to that logic, which previously existed as part of the logic to see if we could start on the paid_ads step.
joemcgill
commented
2 weeks ago @asvinb I've updated the rest of this PR after incorporating the changes from #2661 if you want to give it another look.
@mikkamp could you review the PHP changes specific to src/MerchantCenter/MerchantCenterService.php and the related PHPUnit tests?
@eason9487 this will still need QA, but would appreciate an early review since there are so many changes (mostly removal of unused components).
eason9487
commented
2 weeks ago Before looking into the code implementation, I would like to raise concerns about the new UI.
A similar concern about insufficient guidance also occurs on the plugin Settings page.
https://github.com/user-attachments/assets/4adad75b-e337-45e4-ba83-2ad59b3bcf14
For example:
The following compares several scenarios: After a GMC account is connected, the operations required to reach the point where the WC address is synchronized to the GMC and continue to the next step.
| Scenarios \ Comparisons | Before | After |
|---|---|---|
| When the two addresses are already the same | 1. Click "Continue" button | 1. Click "Continue" button |
| When the two addresses are already the same but want to edit it |
1. Click link to open WooCommerce Settings 2. Update and save address 3. Back to Onboarding webpage 4. Click "Refresh to sync" button 5. Click "Continue" button |
1. Open new webpage 2. Navigate to WooCommerce Settings 3. Update and save address 4. Back to Onboarding webpage 5. Refresh webpage 6. Click "Refresh to sync" button 7. Click "Continue" button |
| When the two addresses are different | 1. Click "Continue" button | 1. Click "Refresh to sync" button 2. Click "Continue" button |
| When a required address field is missing | 1. Click "Continue" to be notified of the missing field 2. Click link to open WooCommerce Settings 3. Add and save address 4. Back to Onboarding webpage 5. Click "Refresh to sync" button 6. Click "Continue" button |
1. Click link to open WooCommerce Settings 2. Add and save address 3. Back to Onboarding webpage 4. Refresh webpage 5. Click "Refresh to sync" button 6. Click "Continue" button |
| When the two addresses are already the same but want to edit an address field and later find a typo in the just edited field |
1. Click link to open WooCommerce Settings 2. Update and save address 3. Back to Onboarding webpage 4. Click "Refresh to sync" button 5. Switch to WooCommerce Settings page 6. Correct and save address 7. Back to Onboarding webpage 8. Click "Refresh to sync" button 9. Click "Continue" button |
1. Open new webpage 2. Navigate to WooCommerce Settings 3. Update and save address 4. Back to Onboarding webpage 5. Refresh webpage 6. Click "Refresh to sync" button 7. Switch to WooCommerce Settings 8. Correct and save address 9. Back to Onboarding webpage 10. Refresh webpage 11. Click "Refresh to sync" button 12. Click "Continue" button |
That's why the original process was designed that way. It's not very convenient either, but at least the guidance is relatively clear and the operations are fewer and more consistent..
eason9487
commented
2 weeks ago Without introducing wider changes, whenever the merchant updates WC address, this whole mechanism always requires an action to display the address to be synchronized and maybe also display the errors to be resolved on this plugin page.
Replacing this action with the updateGoogleMCContactInformation API call or hiding the UI that triggers this action under certain conditions probably wouldn't make the whole thing any better. At least the current implementation doesn't seem to make it any easier for merchants to use.
It's recommended to confirm if the concerns in https://github.com/woocommerce/google-listings-and-ads/pull/2653#issuecomment-2454215548 are in the desired direction. I would suggest that the main direction could be to remove the phone number and merge step 3 into step 1, but to leave the original mechanism of address data (re)fetching and synchronization as unchanged as possible.
Adjustments to this mechanism might be enough by rephrasing the text of the "Refresh to sync" button, or perhaps go one step further by automatically triggering a refetch of address data when the page regains focus.
joemcgill
commented
2 weeks ago Thanks for flagging your concerns @eason9487. I've asked @fblascogarma and @MatthiasReinholz to review and help provide clarification.
For what it's worth, I did meet with @fblascogarma last week to confirm these requirements, as mentioned in this comment. The decision when we spoke was to update this component so that clicking the "Refresh to sync" button would sync the address information to MC rather than updating the UI with fresh data from the WC store.
In the interest of time, can I get the current implementation reviewed and we can make further adjustments to the acceptance criteria as part of the UAT review that will happen on this feature before it's merged to develop?
ankitguptaindia
commented
2 weeks ago Testing Environment -
Test Results - Tested PR based on the information and current Acceptance Criteria provided in connected GH issue. Sync flow is working as expected as described in requirement.
Functional Demo / Screencast -
Onboarding Screen Sync Flow: ✅
https://github.com/user-attachments/assets/ec09aeb5-cd60-4595-bbcf-22e03f756167
If the store address matches with MC address: ✅
https://github.com/user-attachments/assets/d5400079-12e0-4729-946e-e2b4e07c817b
Setting Screen Sync Flow: ✅
https://github.com/user-attachments/assets/0e344208-7ab1-4dc8-9f11-d54651dd3099
joemcgill
commented
2 weeks ago Change of plans here. After confirming with @fblascogarma and @MatthiasReinholz today, we're going to take a slightly different approach here. I've updated the AC in the issue, but the key changes are:
- Show always the Store Address component, independently if a merchant already synced their store address to MC.
- Change CTA wording and keep same functionality. Wording: “Update store address”
joemcgill
commented
2 weeks ago After reviewing the PR with @fblascogarma today and confirming that the updates reflect our shared understanding of the new requirements, we've got an additional change to the copy used in the address card:
When the address is valid:
"We’re using your store address for Google verification. This information won’t be public. Edit in WooCommerce settings if needed and update to review the changes."
When the address is NOT valid
"Your store address is required by Google for verification. This information won’t be public. Complete that in WooCommerce settings and update to review the changes."
{ verification errors shown below }
I've applied those changes in d772944.
joemcgill
commented
2 weeks ago @eason9487 I've updated the approach for this one based on the updated requirements. There are still some E2E tests to update tomorrow, but would be happy for you to give it another look given the number of changes.
joemcgill
commented
2 weeks ago E2E tests are updated so this should be ready for another look.
joemcgill
commented
1 week ago @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.
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:
Additional details:
Changelog entry