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

Sync MC address during account connection #2602

Open
10 tasks
Tracked by #2509
joemcgill opened this issue Sep 11, 2024 · 8 comments · Fixed by #2653
Open
10 tasks
Tracked by #2509

Sync MC address during account connection #2602

joemcgill opened this issue Sep 11, 2024 · 8 comments · Fixed by #2653
Assignees

Comments

@joemcgill
Copy link
Collaborator

joemcgill commented Sep 11, 2024

Part of #2509

As a follow-up to #2493, the requirement to validate a phone number is possibly going away, in which case, we can consider moving the process of syncing the store's address with the MC account to the GoogleComboAccountCard when the account is first connected, if we see that there is no address associated with the MC account or if it differs from the store's address.

Screenshots:

Warning

Screenshots are out of date with the most recent change to the acceptance criteria

Image

Acceptance Criteria (updated 11/5)

Onboarding

  • During onboarding, the store address information will always be shown at the bottom of the GoogleComboAccountsCard.
  • If all of the address requirements have not been met, the missing data will be shown as validation errors on the address card, and the user will not be able to complete step 1 until this is resolved.
  • Clicking the "Get data from WooCommerce Settings" button (❓ Can we use "Update store address" or something shorter?)
  • If all address requirements are met and the store address does not match what is in MC, the address info will be synced when the user clicks "Continue" to move to step 2.
  • The current Step 3 of the onboarding step is removed (see Onboarding: Remove “Confirm store requirements” step if possible #2493)

Settings screen

  • On the settings screen, the Contact section displays the current StoreAddressCardPreview component with the "Edit" button to open the edit flow.
  • The edit flow will show the same Address Card from onboarding with validation errors visible
  • The "Save" button will be disabled until validation errors have been resolved
  • Clicking the "Save" button will sync address data to MC and return to the settings screen.
  • All unused components related to phone verification is removed

Implementation Brief

  • Create SyncStoreAddress component in js/src/components/google-combo-account-card/sync-store-address/index.js to render the StoreAddressCard component based on the following conditions:
    • The MC account must be connected.
      • Check the MC status via the useGoogleMCAccount hook and checking the status
    • Use the useStoreAddress hook to determine if the address needs to be synced.
      • mc_address should not be null.
      • is_mc_address_different is true which means the address needs to be synced.
    • Update the StoreAddressCard component so that the handleRefreshClick callback uses the updateGoogleMCContactInformation hook, as demonstrated in [this comment] (Sync MC address during account connection #2602 (comment)) and this followup.
  • Update GoogleComboAccountsCard to include SyncStoreAddress as part of the card body.
  • The "Continue" button for GoogleComboAccountsCard should be disabled until the address has been synced, by using the above hooks.

Test Coverage

  • Appropriate E2E tests should be added to tests for the above
  • PHPUnit tests related to the onboarding flow are updated
@joemcgill joemcgill added the needs design The issue requires design input/work from a designer. label Sep 11, 2024
@joemcgill joemcgill added the status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. label Sep 11, 2024
@asvinb
Copy link
Collaborator

asvinb commented Sep 16, 2024

@joemcgill While working on the IB, I tested the "Refresh to sync" function. My understanding is that if the address in WC is different from MC, then the "Refresh to sync" button will update the MC address with the WC address. However when I was testing, this does not seem to work.

Store address in WC:
image

The city is: Rose Hill

Store address in MC:
image

The city is: Curepipe

When I click on the refresh button, an API request is sent to /wp-json/wc/gla/mc/contact-information and there are no errors. When I check in MC dashboard, the address is not updated. You can see a video below:

Sep-16-2024.15-21-43.mp4

When I query the data store, the address are still different (after clicking the refresh button):
image

Let me know if am missing anything. Thanks!

@joemcgill
Copy link
Collaborator Author

Thanks for flagging the concern, @asvinb. If syncing is indeed not working, we could file a bug report and that could be worked on in advance of this feature. For now, I've moved this back to definition while the design approach is being finalized.

@macka61 macka61 added needs design The issue requires design input/work from a designer. status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. and removed needs design The issue requires design input/work from a designer. status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. labels Oct 1, 2024
@joemcgill
Copy link
Collaborator Author

@asvinb rather than having this be part of the Card body added in #2597, the new designs have this being displayed as an additional Card body beneath the expanded cards for Ads and MC. Given that, I think we could do this before #2579 is completed since we're creating an MC account in #2567 and may need to sync the address for the new account. What do you think?

@joemcgill joemcgill removed needs design The issue requires design input/work from a designer. status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. labels Oct 4, 2024
@asvinb
Copy link
Collaborator

asvinb commented Oct 9, 2024

I think that works @joemcgill . I've updated the IB to reflect that.

@joemcgill
Copy link
Collaborator Author

@eason9487 I was able to validate the issue that @asvinb shared here: #2602 (comment)

It looks to me that the "Refresh to sync" button in the StoreAddressCard has not synced the store address with MC for quite some time. Currently, the callback that is triggered when that button is clicked triggers a refresh() from the useStoreAddress() hook, which sends a GET request to the /wc/gla/mc/contact-information endpoint, which will update the store address shown on the card if the store address has changed, but it doesn't sync those changes to MC.

https://github.com/woocommerce/google-listings-and-ads/blob/develop/js/src/components/contact-information/store-address-card.js#L68-L81

To sync the address with MC, it seems that we should be calling the updateGoogleMCContactInformation() action instead.

As a quick experiment, I've applied this diff locally and it seems to work, but can likely be improved:

diff --git a/js/src/components/contact-information/store-address-card.js b/js/src/components/contact-information/store-address-card.js
index 482603b4c..ba2488d26 100644
--- a/js/src/components/contact-information/store-address-card.js
+++ b/js/src/components/contact-information/store-address-card.js
@@ -19,6 +19,7 @@ import AppButton from '.~/components/app-button';
 import ValidationErrors from '.~/components/validation-errors';
 import ContactInformationPreviewCard from './contact-information-preview-card';
 import TrackableLink from '.~/components/trackable-link';
+import { useAppDispatch } from '.~/data';
 import mapStoreAddressErrors from './mapStoreAddressErrors';
 import { recordGlaEvent } from '.~/utils/tracks';
 import './store-address-card.scss';
@@ -54,7 +55,8 @@ import './store-address-card.scss';
  * @return {JSX.Element} Filled AccountCard component.
  */
 const StoreAddressCard = ( { showValidation = false } ) => {
-	const { loaded, data, refetch } = useStoreAddress();
+	const { loaded, data } = useStoreAddress();
+	const { updateGoogleMCContactInformation } = useAppDispatch();
 	const path = getPath();
 	const { subpath } = getQuery();
 
@@ -66,7 +68,7 @@ const StoreAddressCard = ( { showValidation = false } ) => {
 	}
 
 	const handleRefreshClick = () => {
-		refetch();
+		updateGoogleMCContactInformation();
 
 		refetchedCallbackRef.current = ( storeAddress ) => {
 			const eventProps = {

Is there a reason that I'm not aware of why the button wasn't already using the updateGoogleMCContactInformation action? Also, is there a preferred way to handle the loaded state in this scenario, where the POST request causes the data returned by useStoreAddress to update without calling the selector again, e.g., would it be better to move the update function into the useStoreAddress() itself?

@joemcgill joemcgill added the needs feedback The issue/PR needs a response from any of the parties involved in the issue. label Oct 16, 2024
@eason9487
Copy link
Member

Hi @joemcgill

Is there a reason that I'm not aware of why the button wasn't already using the updateGoogleMCContactInformation action?

I don't remember the reason, and I can't find the relevant discussion for a while. But I'm sure it's not a bug. The current implementation was based on the design at that time.

Also, is there a preferred way to handle the loaded state in this scenario, where the POST request causes the data returned by useStoreAddress to update without calling the selector again, e.g., would it be better to move the update function into the useStoreAddress() itself?

It could be handled in the same way as EditStoreAddress:

const { data: address } = useStoreAddress();
const [ isSaving, setSaving ] = useState( false );
const handleSaveClick = () => {
setSaving( true );
updateGoogleMCContactInformation()
.then( () => getHistory().push( getSettingsUrl() ) )
.catch( () => setSaving( false ) );
};

@joemcgill joemcgill removed the needs feedback The issue/PR needs a response from any of the parties involved in the issue. label Oct 17, 2024
@asvinb asvinb assigned asvinb and unassigned joemcgill Oct 25, 2024
@asvinb asvinb assigned joemcgill and unassigned asvinb Oct 28, 2024
@joemcgill joemcgill linked a pull request Oct 28, 2024 that will close this issue
@asvinb asvinb removed their assignment Oct 29, 2024
@joemcgill
Copy link
Collaborator Author

I've just updated the description of this issue to communicate that one aspect of the designs are currently known to be incorrect.

Warning

The screenshot for this design is incorrect. The refresh button and text will be shown when we DO have a valid address, otherwise no button will be shown and the merchant will be directed to update their address in the WooCommerce settings.

The Figma was never updated, but we'll fix it during the implementation by moving the Refresh button and text to the case where we DO have an adddress.

@joemcgill
Copy link
Collaborator Author

I've updated the acceptance criteria for the settings screen after confirming the approach from this discussion.

@joemcgill joemcgill assigned asvinb and ankitguptaindia and unassigned joemcgill Nov 1, 2024
@asvinb asvinb assigned joemcgill and unassigned asvinb Nov 6, 2024
@joemcgill joemcgill assigned eason9487 and unassigned joemcgill Nov 7, 2024
@eason9487 eason9487 removed their assignment Nov 13, 2024
@joemcgill joemcgill assigned eason9487 and mikkamp and unassigned joemcgill Nov 14, 2024
@eason9487 eason9487 removed their assignment Nov 14, 2024
@joemcgill joemcgill assigned fblascogarma and unassigned mikkamp Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants