-
Notifications
You must be signed in to change notification settings - Fork 21
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
Detect the verification status of the phone number to the contact information settings #1717
Changes from 5 commits
f14afcf
fdfa239
cdbf344
c4e45c7
4cf3f29
789cb42
d484955
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
.gla-contact-info-preview-card { | ||
.gla-account-card__description { | ||
gap: $grid-unit-05; | ||
} | ||
} | ||
|
||
.gla-phone-number-card-preview { | ||
&__verified-status { | ||
color: $alert-green; | ||
} | ||
|
||
&__unverified-status { | ||
color: $alert-red; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ | |
* External dependencies | ||
*/ | ||
import { __ } from '@wordpress/i18n'; | ||
import { useState, useEffect } from '@wordpress/element'; | ||
import { useState, useEffect, useRef } from '@wordpress/element'; | ||
import { CardDivider } from '@wordpress/components'; | ||
import { Spinner } from '@woocommerce/components'; | ||
|
||
|
@@ -101,10 +101,10 @@ function EditPhoneNumberCard( { phoneNumber, onPhoneNumberVerified } ) { | |
* @param {boolean|null} [props.initEditing=null] Specify the inital UI state. | ||
* `true`: initialize with the editing UI. | ||
* `false`: initialize with the viewing UI. | ||
Comment on lines
102
to
103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ I don't understand well this concept of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rephrased in 0ae56ea. |
||
* `null`: determine the initial UI state according to the `data.isValid` after the `phoneNumber` loaded. | ||
* `null`: determine the initial UI state according to the `data.isVerified` after the `phoneNumber` loaded. | ||
* @param {Function} [props.onEditClick] Called when clicking on "Edit" button. | ||
* If this callback is omitted, it will enter edit mode when clicking on "Edit" button. | ||
* @param {Function} [props.onPhoneNumberVerified] Called when the phone number is verified in edit mode. | ||
* @param {Function} [props.onPhoneNumberVerified] Called when the phone number is verified or has been verified. | ||
* | ||
* @fires gla_mc_phone_number_edit_button_click | ||
*/ | ||
|
@@ -117,14 +117,27 @@ const PhoneNumberCard = ( { | |
} ) => { | ||
const { loaded, data } = phoneNumber; | ||
const [ isEditing, setEditing ] = useState( initEditing ); | ||
const onPhoneNumberVerifiedRef = useRef(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ What is the advantage of useRef vs useCallback? (I take as granted this is for avoiding error as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think it's probably not a choice about advantages, but rather what the concern is to be addressed here.
Yes, it avoids the possibility of infinity re-rendering loop. |
||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ don't understand this line tbh. What does it mean "The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rephrased in 0ae56ea. |
||
useEffect( () => { | ||
if ( loaded && isEditing === null ) { | ||
setEditing( ! data.isValid ); | ||
setEditing( ! isVerified ); | ||
} | ||
}, [ loaded, isVerified, isEditing ] ); | ||
|
||
// 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. | ||
Comment on lines
+133
to
+135
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ This was a bit confusing for me. Suggestion: When There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's intended to illustrate how |
||
useEffect( () => { | ||
if ( loaded && initEditing !== true && isVerified ) { | ||
onPhoneNumberVerifiedRef.current(); | ||
} | ||
}, [ loaded, data.isValid, isEditing ] ); | ||
}, [ loaded, isVerified, initEditing ] ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ Could be possible to have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed in 290da41. |
||
|
||
// Return a simple loading AccountCard since the initial edit state is unknown before loaded. | ||
if ( isEditing === null ) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,35 +124,40 @@ protected function get_contact_information_endpoint_edit_callback(): callable { | |
*/ | ||
protected function get_schema_properties(): array { | ||
return [ | ||
'id' => [ | ||
'id' => [ | ||
'type' => 'integer', | ||
'description' => __( 'The Merchant Center account ID.', 'google-listings-and-ads' ), | ||
'context' => [ 'view', 'edit' ], | ||
'validate_callback' => 'rest_validate_request_arg', | ||
], | ||
'phone_number' => [ | ||
'phone_number' => [ | ||
'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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ I guess this is an enum 'verified'|'unverified' ?? Or it could be more things? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in bba9fa3. |
||
'description' => __( 'The verification status of the phone number associated with the Merchant Center account.', 'google-listings-and-ads' ), | ||
'context' => [ 'view' ], | ||
], | ||
'mc_address' => [ | ||
'type' => 'object', | ||
'description' => __( 'The address associated with the Merchant Center account.', 'google-listings-and-ads' ), | ||
'context' => [ 'view' ], | ||
'properties' => $this->get_address_schema(), | ||
], | ||
'wc_address' => [ | ||
'wc_address' => [ | ||
'type' => 'object', | ||
'description' => __( 'The WooCommerce store address.', 'google-listings-and-ads' ), | ||
'context' => [ 'view' ], | ||
'properties' => $this->get_address_schema(), | ||
], | ||
'is_mc_address_different' => [ | ||
'is_mc_address_different' => [ | ||
'type' => 'boolean', | ||
'description' => __( 'Whether the Merchant Center account address is different than the WooCommerce store address.', 'google-listings-and-ads' ), | ||
'context' => [ 'view' ], | ||
], | ||
'wc_address_errors' => [ | ||
'wc_address_errors' => [ | ||
'type' => 'array', | ||
'description' => __( 'The errors associated with the WooCommerce address', 'google-listings-and-ads' ), | ||
'context' => [ 'view' ], | ||
|
@@ -215,10 +220,11 @@ public function get_update_args(): array { | |
* @return Response | ||
*/ | ||
protected function get_contact_information_response( ?AccountBusinessInformation $contact_information, Request $request ): Response { | ||
$phone_number = null; | ||
$mc_address = null; | ||
$wc_address = null; | ||
$is_address_diff = false; | ||
$phone_number = null; | ||
$phone_verification_status = null; | ||
$mc_address = null; | ||
$wc_address = null; | ||
$is_address_diff = false; | ||
|
||
if ( $this->settings->get_store_address() instanceof AccountAddress ) { | ||
$wc_address = $this->settings->get_store_address(); | ||
|
@@ -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() ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💅 Maybe we can have 'VERIFIED' as well in the frontend and get rid of this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the status fields returned by the other GLA APIs are almost always lowercase, the idea here was to keep them consistent. |
||
} catch ( InvalidValue $exception ) { | ||
// log and fail silently | ||
do_action( 'woocommerce_gla_exception', $exception, __METHOD__ ); | ||
|
@@ -249,12 +256,13 @@ protected function get_contact_information_response( ?AccountBusinessInformation | |
|
||
return $this->prepare_item_for_response( | ||
[ | ||
'id' => $this->options->get_merchant_id(), | ||
'phone_number' => $phone_number, | ||
'mc_address' => self::serialize_address( $mc_address ), | ||
'wc_address' => self::serialize_address( $wc_address ), | ||
'is_mc_address_different' => $is_address_diff, | ||
'wc_address_errors' => $wc_address_errors, | ||
'id' => $this->options->get_merchant_id(), | ||
'phone_number' => $phone_number, | ||
'phone_verification_status' => $phone_verification_status, | ||
'mc_address' => self::serialize_address( $mc_address ), | ||
'wc_address' => self::serialize_address( $wc_address ), | ||
'is_mc_address_different' => $is_address_diff, | ||
'wc_address_errors' => $wc_address_errors, | ||
], | ||
$request | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -293,8 +293,9 @@ protected function maybe_add_contact_info_issue( array $issues, DateTime $cache_ | |
*/ | ||
protected function is_mc_contact_information_setup(): bool { | ||
$is_setup = [ | ||
'phone_number' => false, | ||
'address' => false, | ||
'phone_number' => false, | ||
'phone_verification_status' => false, | ||
'address' => false, | ||
]; | ||
|
||
try { | ||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💅 So I would suggest 'phone_number_verified' or 'is_verified' or something like that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed in 32df87f. |
||
|
||
/** @var Settings $settings */ | ||
$settings = $this->container->get( Settings::class ); | ||
|
@@ -323,7 +325,7 @@ protected function is_mc_contact_information_setup(): bool { | |
} | ||
} | ||
|
||
return $is_setup['phone_number'] && $is_setup['address']; | ||
return $is_setup['phone_number'] && $is_setup['phone_verification_status'] && $is_setup['address']; | ||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅 We can just omit this prop
initEditing
. Since the default value isnull
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in e3f4d79.