Detect the verification status of the phone number to the contact information settings #1717

eason9487 · 2022-10-05T07:33:56Z

Changes proposed in this Pull Request:

Add the verification status of the phone number to the API GET mc/contact-information.
Add a phone verification status check to the API GET mc/setup.
Detect the verification status of the phone number in the onboarding flow.
Add the verification status of the phone number to PhoneNumberCardPreview component.

Screenshots:

💡 The phone numbers shown in the following videos were temporarily changed to mocks in my local env. It still uses a real phone number when communicating with APIs.

📹 Onboarding flow

Kapture.2022-10-05.at.14.17.30.mp4

📹 Settings page

Kapture.2022-10-05.at.14.21.49.mp4

Detailed test instructions:

Go to the admin page of Google Merchant Center and save an Unverified phone number. https://merchants.google.com/mc/merchantprofile/businessinfo
Go to the onboarding flow of GLA.
When entering step 3, the phone number card in the contact info settings should be in editing mode.
Verify a phone number and complete step 3.
After refreshing page, it should start with step 4 since the mc/setup API should consider step 3 was completed.
Back to step 3, the phone number card in the contact info settings should not be in editing mode.
Finish the onboarding flow and go to GLA settings page.
Check if there is a "Verified" status shown under the phone number.
Revisit the admin page of Google Merchant Center and save an Unverified phone number again.
Refresh GLA settings page to see if the verification status is changed to "Unverified".

Changelog entry

Update - Detect the verification status of the phone number in the contact information settings.

…t-information`

…_contact_information_setup

…PhoneNumber` hook

…flow

…view component

… the previous steps

puntope · 2022-10-10T17:23:25Z

js/src/components/contact-information/phone-number-card/phone-number-card.js

@@ -117,14 +117,27 @@ const PhoneNumberCard = ( {
 } ) => {
 	const { loaded, data } = phoneNumber;
 	const [ isEditing, setEditing ] = useState( initEditing );
+	const onPhoneNumberVerifiedRef = useRef();


❓ What is the advantage of useRef vs useCallback?

(I take as granted this is for avoiding error as onPhoneNumberVerified needs to be a dependency in useEffect

What is the advantage of useRef vs useCallback?

I think it's probably not a choice about advantages, but rather what the concern is to be addressed here. useCallback is used when needing a callback with stable reference based on deps array. useRef has plenty of use scenarios and one of them is the need of ignoring the reference changes such as the use case here.

I take as granted this is for avoiding error as onPhoneNumberVerified needs to be a dependency in useEffect

Yes, it avoids the possibility of infinity re-rendering loop.

puntope · 2022-10-10T17:25:22Z

js/src/components/contact-information/index.js

@@ -127,7 +116,7 @@ const ContactInformation = ( { onPhoneNumberVerified } ) => {
 				<PhoneNumberCard
 					view="setup-mc"
 					phoneNumber={ phone }
-					initEditing={ initEditing }
+					initEditing={ null }


💅 We can just omit this prop initEditing. Since the default value is null

Removed in e3f4d79.

puntope · 2022-10-10T17:29:57Z

js/src/components/contact-information/phone-number-card/phone-number-card.js

+	const onPhoneNumberVerifiedRef = useRef();
+	onPhoneNumberVerifiedRef.current = onPhoneNumberVerified;
+
+	const { isVerified } = data;

 	// Handle the initial UI state of `initEditing = null`.
 	// The `isEditing` state is on hold. Determine it after the `phoneNumber` loaded.


❓ don't understand this line tbh. What does it mean "The isEditing state is on hold"?

Rephrased in 0ae56ea.

puntope · 2022-10-10T17:34:21Z

js/src/components/contact-information/phone-number-card/phone-number-card.js

+	// If `initEditing` is true, EditPhoneNumberCard takes care of the call of `onPhoneNumberVerified`
+	// after the phone number is verified. If `initEditing` is false or null, this useEffect handles
+	// the call of `onPhoneNumberVerified` when the loaded phone number has already been verified.


❓ This was a bit confusing for me.

Suggestion:

When initEditing prop is not true we call onPhoneNumberVerified when the loaded phone number has already been verified.

It's intended to illustrate how onPhoneNumberVerified calls are handled under different conditions. I'd like to keep the explicit mention of "If initEditing is false or null" to clarify why the conditional below is a shorter one initEditing ! == true rather than a longer one ( initEditing === false || initEditing === null ), and to avoid the appearance that it could be changed to initEditing === false.

puntope

Working good. Thanks @eason9487

I left tho a few comments regarding code :)

puntope · 2022-10-10T17:35:07Z

js/src/components/contact-information/phone-number-card/phone-number-card.js

 		}
-	}, [ loaded, data.isValid, isEditing ] );
+	}, [ loaded, isVerified, initEditing ] );


❓ Could be possible to have isVerified but not loaded phoneNumber?

Removed in 290da41.

puntope · 2022-10-10T17:37:29Z

js/src/components/contact-information/phone-number-card/phone-number-card.js

 *     `true`: initialize with the editing UI.
 *     `false`: initialize with the viewing UI.


❓ I don't understand well this concept of initialize with the editing UI and initialize with the viewing UI.

Rephrased in 0ae56ea.

puntope · 2022-10-10T17:52:16Z

src/API/Site/Controllers/MerchantCenter/ContactInformationController.php

@@ -228,7 +234,8 @@ protected function get_contact_information_response( ?AccountBusinessInformation
 		if ( $contact_information instanceof AccountBusinessInformation ) {
 			if ( ! empty( $contact_information->getPhoneNumber() ) ) {
 				try {
-					$phone_number = PhoneNumber::cast( $contact_information->getPhoneNumber() )->get();
+					$phone_number              = PhoneNumber::cast( $contact_information->getPhoneNumber() )->get();
+					$phone_verification_status = strtolower( $contact_information->getPhoneVerificationStatus() );


💅 Maybe we can have 'VERIFIED' as well in the frontend and get rid of this strtolower function

Since the status fields returned by the other GLA APIs are almost always lowercase, the idea here was to keep them consistent.

puntope · 2022-10-10T17:54:31Z

src/MerchantCenter/MerchantCenterService.php

@@ -310,7 +311,8 @@ protected function is_mc_contact_information_setup(): bool {
 		}

 		if ( $contact_info instanceof AccountBusinessInformation ) {
-			$is_setup['phone_number'] = ! empty( $contact_info->getPhoneNumber() );
+			$is_setup['phone_number']              = ! empty( $contact_info->getPhoneNumber() );
+			$is_setup['phone_verification_status'] = 'VERIFIED' === $contact_info->getPhoneVerificationStatus();


💅 phone_verification_status seems not accurate naming for me. It sounds like we expect a status (like 'verified'. However we just want a booean.

So I would suggest 'phone_number_verified' or 'is_verified' or something like that.

Renamed in 32df87f.

puntope · 2022-10-10T17:55:44Z

src/API/Site/Controllers/MerchantCenter/ContactInformationController.php

 				'type'        => 'string',
 				'description' => __( 'The phone number associated with the Merchant Center account.', 'google-listings-and-ads' ),
 				'context'     => [ 'view' ],
 			],
-			'mc_address'              => [
+			'phone_verification_status' => [
+				'type'        => 'string',


❓ I guess this is an enum 'verified'|'unverified' ?? Or it could be more things?

Added in bba9fa3.

…step Change the Stepper in the onboarding flow to only allow going back to the previous steps

ianlin

Thanks @puntope fo reviewing, I have checked the code and tested the PR again and there is nothing needs to be updated immediately. I created a followup issue #1724 so Eason can take a look when he gets back.

…tInformation component since it's the same as the default value. Address #1717 (comment)

…'s `initEditing` prop and related processing. Address - #1717 (comment) - #1717 (comment)

…onPhoneNumberVerified in PhoneNumberCard. Address #1717 (comment)

…tInformationController. Address #1717 (comment)

…er_verified` in MerchantCenterService. Address #1717 (comment)

eason9487 added 5 commits October 5, 2022 14:30

Add a new field phone_verification_status to the API `GET mc/contac…

f14afcf

…t-information`

Add a phone verification status check to MerchantCenterService->is_mc…

fdfa239

…_contact_information_setup

Add a new property isVerified to the returning data of `useGoogleMC…

cdbf344

…PhoneNumber` hook

Detect the verification status of the phone number in the onboarding …

c4e45c7

…flow

Add the verification status of the phone number to PhoneNumberCardPre…

4cf3f29

…view component

eason9487 requested a review from a team October 5, 2022 07:33

eason9487 self-assigned this Oct 5, 2022

github-actions bot added the changelog: update Big changes to something that wasn't broken. label Oct 5, 2022

eason9487 linked an issue Oct 5, 2022 that may be closed by this pull request

Submit business contact information - after phone verification status is available #1033

Closed

5 tasks

Change the Stepper in the onboarding flow to only allow going back to…

789cb42

… the previous steps

puntope reviewed Oct 10, 2022

View reviewed changes

ianlin mentioned this pull request Oct 12, 2022

Followup of PR reviews comments in #1717 #1724

Closed

Merge pull request #1720 from woocommerce/fix/1684-onboarding-switch-…

d484955

…step Change the Stepper in the onboarding flow to only allow going back to the previous steps

ianlin approved these changes Oct 12, 2022

View reviewed changes

ianlin merged commit b47ad11 into feature/1610-streamlined-onboarding Oct 12, 2022

ianlin deleted the update/1033-phone-number-verification-status branch October 12, 2022 05:07

eason9487 added a commit that referenced this pull request Oct 17, 2022

Remove specifying of PhoneNumberCard's initEditing prop from Contac…

e3f4d79

…tInformation component since it's the same as the default value. Address #1717 (comment)

eason9487 added a commit that referenced this pull request Oct 17, 2022

Rephrase the JSDoc descriptions and code comments for PhoneNumberCard…

0ae56ea

…'s `initEditing` prop and related processing. Address - #1717 (comment) - #1717 (comment)

eason9487 added a commit that referenced this pull request Oct 17, 2022

Omit the loaded checking in the useEffect that handles the call of …

290da41

…onPhoneNumberVerified in PhoneNumberCard. Address #1717 (comment)

eason9487 added a commit that referenced this pull request Oct 17, 2022

Add enum to the phone_verification_status schema property in Contac…

bba9fa3

…tInformationController. Address #1717 (comment)

eason9487 added a commit that referenced this pull request Oct 17, 2022

Renaming an array key from phone_verification_status to `phone_numb…

32df87f

…er_verified` in MerchantCenterService. Address #1717 (comment)

eason9487 mentioned this pull request Oct 17, 2022

Detect the verification status of the phone number: Follow-up adjustments for PR 1717 code reviews #1732

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect the verification status of the phone number to the contact information settings #1717

Detect the verification status of the phone number to the contact information settings #1717

eason9487 commented Oct 5, 2022

puntope Oct 10, 2022

eason9487 Oct 17, 2022

puntope Oct 10, 2022 •

edited

Loading

eason9487 Oct 17, 2022 •

edited

Loading

puntope Oct 10, 2022

eason9487 Oct 17, 2022

puntope Oct 10, 2022

eason9487 Oct 17, 2022

puntope left a comment

puntope Oct 10, 2022

eason9487 Oct 17, 2022

puntope Oct 10, 2022

eason9487 Oct 17, 2022

puntope Oct 10, 2022

eason9487 Oct 17, 2022

puntope Oct 10, 2022

eason9487 Oct 17, 2022

puntope Oct 10, 2022

eason9487 Oct 17, 2022

ianlin left a comment

		* `true`: initialize with the editing UI.
		* `false`: initialize with the viewing UI.

Detect the verification status of the phone number to the contact information settings #1717

Detect the verification status of the phone number to the contact information settings #1717

Conversation

eason9487 commented Oct 5, 2022

Changes proposed in this Pull Request:

Screenshots:

📹 Onboarding flow

📹 Settings page

Detailed test instructions:

Changelog entry

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puntope Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

eason9487 Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puntope left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianlin left a comment

Choose a reason for hiding this comment

puntope Oct 10, 2022 •

edited

Loading

eason9487 Oct 17, 2022 •

edited

Loading