Integrate with contact information API and complete form submissions within MC setup and settings

[eason9487]

Changes proposed in this Pull Request:

Partial of #863 and based on #921

• Integrate data store with the backend contact information API
• Change the data source of useStoreAddress to the contact information API
• Add isValid and isDirty to the callback param of onPhoneNumberChange in the PhoneNumberContent component
• Implement the data submission of MC setup and Settings pages

Screenshots:

When the phone number got from Google MC is empty

Kapture.2021-08-02.at.18.37.16.mp4

When the phone number got from Google MC has already exist

Kapture.2021-08-02.at.18.38.42.mp4

Edit via the Settings page

Kapture.2021-08-02.at.18.41.38.mp4

Detailed test instructions:

To reset GMC phone number as empty data, it could be done by replacing the value of POST request here by

$this->contact_information->update_phone_number( null );

And then call the update contact info API via DevTool console by

wp.data.dispatch('wc/gla').updateGoogleMCContactInformation('123', '456')

MC setup

1. Reset GMC phone number and go through the MC setup flow to see if the phone number and store address are updated to GMC
2. Repeat the 1. with an existing phone number

Settings

1. Reset GMC phone number and go to the Settings page and it should show a warning section
2. Click on the "Add information", fill in data, save with the "Save details" button
3. After updating complete, it should bring back to the main Settings page, and the contact info on UI should be update-to-date
4. Repeat the above flow with an existing phone number

Changelog entry

[ecgan]

I have a bit of mixed feelings about this PR. I left a few comments with ❔ . In short, personally I would use a Form component to keep track of form values and check for validity, and there are a few other things that I'm wondering as well. I think this may all boil down to personal preferences, or maybe it's because I haven't been following this feature project closely.

In addition, I am facing a blocker in Setup MC Step 4, where the contact-information API returns null for wc_address, causing useStoreAddress to throw error "TypeError: Cannot read property 'country' of null". I have already posted this in our project slack channel (p1627923876015900-slack-C01E9LB4X61) for advice. I'm not able to test as per the testing instructions.

Since the comments are not blockers, and given the time constraint that we have, I'd say feel free to go ahead with this PR. 👌

[ecgan]

❔ Why do we want to create another selector, and not just use getGoogleMCContactInformation and its hasFinishedResolution? Are there any issues with that approach? 🤔

[eason9487]

Are there any issues with that approach?

Since the store address and GMC phone number are provided by the same API endpoint, and it needs to re-fetch the address during editing. To achieve all of the following targets, I finally ended up with this solution.

• Only allow the <StoreAddressCard> enter the loading state after clicking the refresh button.
• Don't reset the phone number user entered/changed after clicking the refresh button.
• Don't issue duplicated API requests to load phone number and address separately.

Here is a GIF to show UI issues if we use getGoogleMCContactInformation for getting phone number:

[ecgan]

Wow! Thank you, especially the animated gif! 😀 👍

(I'm thinking it may be better to have separate APIs for phone number and store address - one reason being phone number is stored in Google and store address is in the local WC site - but anyway I guess we can keep that out of this PR 😄 )

[ecgan]

❔ I'm wondering whether we really need to check for dirtiness. Can't we just pass the data without checking for dirtiness?

[eason9487]

Users can set their business information at Google's Merchant Center console as well, and the phone number is able to verify there. If we pass the data to Google end without checking for dirtiness, then the verified phone number would be overwritten to an unverified one.

[eason9487]

And the country calling code at Google end would be lost. 😟

[ecgan]

If we pass the data to Google end without checking for dirtiness, then the verified phone number would be overwritten to an unverified one.

So even if the phone number is still exactly the same (not dirty), when we pass it to Google, the verified phone number would become unverified. Gotcha. 👍

[eason9487]

So even if the phone number is still exactly the same (not dirty), when we pass it to Google, the verified phone number would become unverified.

Just tested this case again, and ... yes. 😂

[ecgan]

❔ I'm wondering why don't we use a Form component to keep track of the form value and check for data validity? 🤔

[eason9487]

Besides considering that the current Form components are not very convenient to use, they often cause the props drilling on formProps, and might not be suitable for use when dealing with multi-layer states. And moreover in this case:

1. The format verification and dirtiness checks of international phone numbers are very specific and consistent. Encapsulated them inside the component can make it easier and simpler to use at the consumer side each time. If compose it to the Form component, the outer Form wrapping is always required. And that looks redundant to me.
2. According to the full version of contact information, there will be an additional state layer to process the phone number verification via Text message or Phone call. Eventually, the data verification of input data is needed before calling the API for asking a text message or phone call. Furthermore, the verification could be finished without propagating to the outer Form then. The outer layer could only need to know whether the overall phone verification is finished.
3. From my perspective, using Form components to manage all form values is just a pattern but sometimes it makes the code more complex or lengthy.

[ecgan]

Okay, gotcha. 👍

[ecgan]

❔ What is the reason to return emptyData object when loaded is false, as opposed to returning null or undefined? 🤔

[eason9487]

It doesn't have to check data before access to data.* properties when whether the data is loaded is not a concern. And also a little bit error-proofing if forget to access property through data?.isAddressFilled.

• google-listings-and-ads/js/src/settings/edit-contact-information/index.js

  Lines 43 to 44 in af9790b

  	address.isAddressFilled &&
  	( phoneNumber.isDirty || address.isMCAddressDifferent );

• google-listings-and-ads/js/src/setup-mc/setup-stepper/store-requirements/index.js

  Line 103 in af9790b

  address.isAddressFilled;

[ecgan]

Okay, gotcha. The way I usually do it is to check for whether the data is loaded first; and when it is, I'd know I'll be able to access the data based on its schema. For example:

const { loaded, data } = useStoreAddress();

// not loaded yet, show a loading icon or message.
if ( ! loaded ) {
	return 'Loading...'
}

// data is loaded, do things with data.
return data.address

Thanks for letting me know.