[R-1.3] Add Google Ads invite acceptance flow #2270
Conversation
github-actions
bot
added
the
changelog: update
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/setup-google-ads-in-step1 #2270 +/- ##
=====================================================================
- Coverage 62.2% 62.0% -0.2%
- Complexity 4209 4227 +18
=====================================================================
Files 748 772 +24
Lines 21615 21869 +254
Branches 533 548 +15
=====================================================================
+ Hits 13436 13560 +124
- Misses 7727 7846 +119
- Partials 452 463 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ankitrox
changed the title
|
I've merged in the UI changes related to this work from #2275 so we can review and iterate on things in one PR. |
…om/woocommerce/google-listings-and-ads into update/2262-ads-accept-invite-flow
There was a problem hiding this comment.
Hey @asvinb and @ankitrox, I've left a bunch of feedback inline. At a high level, the main issue I'm seeing here is that there seems to be some places where the UI and the API are out of sync causing the account-status endpoint to be called before an Ads account is available, which results in error notices being shown.
I also think that once the GoogleAdsAccountCard has established that the state for having account access has switched (i.e., the invite has been accepeted), that we should immediately kick off the rest of the account setup process by sending another POST request to gla/ads/accounts if the status is incomplete so we can finish the conversion setup and try to link the MC account (if available).
js/src/components/google-ads-account-card/google-ads-account-card.js
Outdated
Show resolved
Hide resolved
…om/woocommerce/google-listings-and-ads into update/2262-ads-accept-invite-flow
…mmerce/google-listings-and-ads into update/2262-ads-accept-invite-flow
There was a problem hiding this comment.
@eason9487 I believe I've addressed all the remaining feedback, just one question about the useEffect hook in the ClaimTermsAndCreateAccountButton component that needs additional discussion.
js/src/components/google-ads-account-card/google-ads-account-card.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for the feedback @eason9487. I'd like to try to align on an approach for completing this Ads account setup process to avoid additional review cycles related to this discussion. Could you provide direction for how you would like that to work?
A possible way would be:
|
There was a problem hiding this comment.
This review is in response to the comment #2270 (comment)
And covers the changes done in bb66dd4
src/Ads/AccountService.php
Outdated
| $this->state->update( $state ); | ||
|
|
||
| // Exit the setup loop. | ||
| break 2; |
There was a problem hiding this comment.
I understand where we are going with this trying to avoid a 428 response. However the initial reason why we started using a different response is because the status wasn't being returned in the response.
For example when we want to create/link a new Ads account we do this with a POST request to the endpoint ads/accounts, which then sets off the multistep process.
A successful response from this request is a 200 response with a response of:
{
"id": 12345678
}If any of the steps failed to complete it would return an error response or the 428 which indicated that another pre-condition must be fulfilled before we can consider all steps complete.
However after the change we have done here break 2 will jump out of the for loop which handles all steps, and it will once again return a 200 response with:
{
"id": 12345678
}Even though we set the internal status that the has_access step is incomplete there is no indication from the response that it must check for additional steps to complete.
Which is why I suggested to still return the 428 from the initial create/link request. It then has the ads/account_access endpoint to fetch the status and current step to know how to handle it (this endpoint is the one that should never return a 428 response).
It is right that eventually we will ideally move away from a 428 in all cases, but that's going to require some additional refactoring which is issue #2345
You are welcome to implement that as well but as I mentioned the status should then be returned within the response from the POST to ads/accounts. And that process must remain consistent between any step which might be unable to complete has_access and billing. Since that's a bit more work to redo I'm considering that as out of scope, and still recommend to go with my original suggestion here #2270 (comment)
Which in simple terms would mean, instead of the break 2 we would throw an ExceptionWithResponseData using 428 as the status code. That's also something you could do within the check_ads_account_has_access similar to how it's done in check_billing_status.
Overriding the status with pending doesn't really do anythin, as throwing the error already prevents it from marking the step as complete.
There was a problem hiding this comment.
I've updated this in 957949a. I was trying to avoid the 428 altogether, but manually setting the step to PENDING and then forcing the loop to stop execution didn't seem elegant either. From what I can tell, the return value from the POST request is not required by the setup process in order to work properly, so this should be ok until #2345 is resolved.
src/Ads/AccountService.php
Outdated
| /** | ||
| * Check if the Ads account has access. | ||
| * | ||
| * @throws Exception If the account doesn't have access. |
There was a problem hiding this comment.
This function is refactored to returning a boolean and not throwing an Exception, which is not shown in the docblock. However as suggested in my previous comment this should throw an ExceptionWithResponseData and doesn't need a return value.
src/Ads/AccountService.php
Outdated
| // If no email, means google account is not connected. | ||
| if ( ! empty( $email ) ) { | ||
| $has_access = $this->container->get( Ads::class )->has_access( $email ); | ||
| $invite_link = $this->options->get( OptionsInterface::ADS_BILLING_URL, '' ); |
There was a problem hiding this comment.
Email is not required to fetch the billing URL, and it will default to empty string if it's not stored so we can save a couple lines if we always return it, regardless of status.
This makes sure HTTP errors are handled correctly when the server returns an error code and returns a callback to avoid loops. The callback name in CreateAccount has also been updated for consistency.
|
@mikkamp I've adjusted the API in 957949a to return a @eason9487 I've moved all of the logic for continuing the setup process after the account is accepted to the |
There was a problem hiding this comment.
Thanks for the adjustments.
I have viewed the new frontend changes but there a few new issues need to be addressed.
Kapture.2024-04-16.at.14.25.23.mp4
Three issues can be found in the video:
- Between 10 to 12 seconds, it shows the "Create Button" button for a moment when creating account.
Ref: - Between 11 to 20 seconds, it shows a blank page is opened in a pop-up window.
Ref: - Between 39 to 45 seconds, it shows the "Create Button" button again for a longer moment when updating account.
Ref:
Thanks for the changes. I tested that on Jurassic Ninja and can confirm I get the 428 with the overwritten "rate limited" body. The next call is then to {
"has_access": false,
"step": "account_access",
"invite_link": "https://ads.google.com/..."
}I'm then able to grant access and continue on with the process. Which eventually updates the card to being fully connected: What I did find odd though is how the second 428 gets handled (which is because I skipped billing). It doesn't have to pause there as that can be handled later, but I did expect the next step to be billing. Note I haven't connected a MC account yet at this stage. {
"has_access": true,
"step": "link_merchant",
"invite_link": "https://ads.google.com/..."
}Looking at the connection test page it showed the status as: I suppose that's not a big deal because completing the MC step is still required, which will also update the ads status as well. So when we do reach the onboarding step where billing details are required it will have the right step as When I did reach the create campaign step and the billing required card is shown it did have the correct deep link, directly to the billing setup, so all good there: So to summarize I don't see any additional changes that are required to the API requests at the moment. Note I did also encounter the same issues with the button showing create at the wrong time, as Eason mentioned, but those points can be addressed in the relevant comments. |
|
Thanks @eason9487 and @mikkamp. I've made some additional updates to try and smooth out the UX of when the buttons are shown to address this feedback. Regarding @mikkamp's feedback about the I think this is now ready for another review. There was quite a bit of refactor to get these loading states right, so hopefully we're getting pretty close. |
There was a problem hiding this comment.
Thanks for all the work on this PR. The frontend side works well and as expected.
There is a further suggestion but it's optional. The following git diff result shows that:
- Simplify the loading state control within
CreateAccountcomponent - Parallel call
fetchGoogleAdsAccountandfetchGoogleAdsAccountStatus - Remove the "Waiting…" state because once the user closes all pop-up windows, there is no button to open the invitation window again from the onboarding page unless they refresh the webpage
- Disable the "Or, use your existing Google Ads account" button while upserting Google Ads account
diff --git a/js/src/components/google-ads-account-card/claim-account-button.js b/js/src/components/google-ads-account-card/claim-account-button.js
index 74f9989ea..a88ed342d 100644
--- a/js/src/components/google-ads-account-card/claim-account-button.js
+++ b/js/src/components/google-ads-account-card/claim-account-button.js
@@ -2,7 +2,6 @@
* External dependencies
*/
import { __ } from '@wordpress/i18n';
-import { noop } from 'lodash';
import { useCallback } from '@wordpress/element';
/**
@@ -10,36 +9,25 @@ import { useCallback } from '@wordpress/element';
*/
import AppButton from '.~/components/app-button';
import getWindowFeatures from '.~/utils/getWindowFeatures';
-import LoadingLabel from '.~/components/loading-label';
import useGoogleAdsAccountStatus from '.~/hooks/useGoogleAdsAccountStatus';
-const ClaimAccountButton = ( { loading = false, onClaimClick = noop } ) => {
+const ClaimAccountButton = () => {
const { inviteLink } = useGoogleAdsAccountStatus();
const handleClaimAccountClick = useCallback(
( event ) => {
- onClaimClick();
-
const { defaultView } = event.target.ownerDocument;
const features = getWindowFeatures( defaultView, 600, 800 );
defaultView.open( inviteLink, '_blank', features );
},
- [ inviteLink, onClaimClick ]
+ [ inviteLink ]
);
return (
- <>
- { loading ? (
- <LoadingLabel
- text={ __( 'Waiting…', 'google-listings-and-ads' ) }
- />
- ) : (
- <AppButton isSecondary onClick={ handleClaimAccountClick }>
- { __( 'Claim Account', 'google-listings-and-ads' ) }
- </AppButton>
- ) }
- </>
+ <AppButton isSecondary onClick={ handleClaimAccountClick }>
+ { __( 'Claim Account', 'google-listings-and-ads' ) }
+ </AppButton>
);
};
diff --git a/js/src/components/google-ads-account-card/claim-account/index.js b/js/src/components/google-ads-account-card/claim-account/index.js
index bb7845d18..641f315c2 100644
--- a/js/src/components/google-ads-account-card/claim-account/index.js
+++ b/js/src/components/google-ads-account-card/claim-account/index.js
@@ -7,11 +7,16 @@ import { Fragment } from '@wordpress/element';
/**
* Internal dependencies
*/
+import { useAppDispatch } from '.~/data';
import Section from '.~/wcdl/section';
+import useWindowFocusCallbackIntervalEffect from '.~/hooks/useWindowFocusCallbackIntervalEffect';
import DisconnectAccount from '../disconnect-account';
import './index.scss';
const ClaimAccount = () => {
+ const { fetchGoogleAdsAccountStatus } = useAppDispatch();
+ useWindowFocusCallbackIntervalEffect( fetchGoogleAdsAccountStatus, 30 );
+
return (
<Fragment>
<p className="gla-ads-claim-account-notice">
diff --git a/js/src/components/google-ads-account-card/create-account-button.js b/js/src/components/google-ads-account-card/create-account-button.js
index a78e4b393..d9334b1f9 100644
--- a/js/src/components/google-ads-account-card/create-account-button.js
+++ b/js/src/components/google-ads-account-card/create-account-button.js
@@ -9,7 +9,6 @@ import { useState } from '@wordpress/element';
*/
import TermsModal from './terms-modal';
import AppButton from '.~/components/app-button';
-import LoadingLabel from '.~/components/loading-label';
/**
* Renders a Google Ads account creaton button.
@@ -18,8 +17,7 @@ import LoadingLabel from '.~/components/loading-label';
* @param {Object} props React props.
* @param {Function} [props.onCreateAccount] Called after the user accept the terms agreement.
*/
-const CreateAccountButton = ( props ) => {
- const { onCreateAccount, ...rest } = props;
+const CreateAccountButton = ( { onCreateAccount } ) => {
const [ isOpen, setOpen ] = useState( false );
const handleButtonClick = () => {
@@ -32,18 +30,11 @@ const CreateAccountButton = ( props ) => {
return (
<>
- { rest.loading ? (
- <LoadingLabel
- text={ __( 'Creating…', 'google-listings-and-ads' ) }
- />
- ) : (
- <AppButton
- isSecondary
- { ...rest }
- text={ __( 'Create account', 'google-listings-and-ads' ) }
- onClick={ handleButtonClick }
- />
- ) }
+ <AppButton
+ isSecondary
+ text={ __( 'Create account', 'google-listings-and-ads' ) }
+ onClick={ handleButtonClick }
+ />
{ isOpen && (
<TermsModal
onCreateAccount={ onCreateAccount }
diff --git a/js/src/components/google-ads-account-card/create-account.js b/js/src/components/google-ads-account-card/create-account.js
index 2084343a8..d84f9a66f 100644
--- a/js/src/components/google-ads-account-card/create-account.js
+++ b/js/src/components/google-ads-account-card/create-account.js
@@ -2,12 +2,11 @@
* External dependencies
*/
import { __ } from '@wordpress/i18n';
-import { useState, useEffect, useCallback, Fragment } from '@wordpress/element';
+import { useState, useEffect, Fragment } from '@wordpress/element';
/**
* Internal dependencies
*/
-import { useAppDispatch } from '.~/data';
import AccountCard, { APPEARANCE } from '.~/components/account-card';
import AppButton from '.~/components/app-button';
import ClaimAccount from './claim-account';
@@ -18,75 +17,49 @@ import Section from '.~/wcdl/section';
import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount';
import useGoogleAdsAccountStatus from '.~/hooks/useGoogleAdsAccountStatus';
import useUpsertAdsAccount from '.~/hooks/useUpsertAdsAccount';
-import useWindowFocusCallbackIntervalEffect from '.~/hooks/useWindowFocusCallbackIntervalEffect';
-import LoadingLabel from '../loading-label/loading-label';
const CreateAccount = ( props ) => {
const { allowShowExisting, onShowExisting } = props;
const [ showClaimModal, setShowClaimModal ] = useState( false );
- const [ isClaiming, setIsClaiming ] = useState( false );
const { googleAdsAccount } = useGoogleAdsAccount();
const { hasAccess, step } = useGoogleAdsAccountStatus();
- const [ upsertAdsAccount, { loading: isUpsertAdsAccountLoading } ] =
- useUpsertAdsAccount();
+ const [ upsertAdsAccount, { loading, creating } ] = useUpsertAdsAccount();
+
const shouldClaimGoogleAdsAccount = Boolean(
googleAdsAccount.id && hasAccess === false
);
const handleOnRequestClose = () => {
setShowClaimModal( false );
- setIsClaiming( false );
};
const handleOnCreateAccount = async () => {
- setIsClaiming( true );
await upsertAdsAccount();
setShowClaimModal( true );
};
- const handleOnClaimClick = () => {
- setIsClaiming( true );
- };
-
- const { fetchGoogleAdsAccountStatus } = useAppDispatch();
-
- const refreshAdsStatus = useCallback( async () => {
- if ( ! shouldClaimGoogleAdsAccount ) {
- return false;
- }
-
- await fetchGoogleAdsAccountStatus();
- }, [ fetchGoogleAdsAccountStatus, shouldClaimGoogleAdsAccount ] );
-
- useWindowFocusCallbackIntervalEffect( refreshAdsStatus, 30 );
-
// Update the Ads account once we have access.
useEffect( () => {
if ( hasAccess === true && step === 'conversion_action' ) {
- setIsClaiming( false );
upsertAdsAccount();
}
}, [ hasAccess, upsertAdsAccount, step ] );
const getIndicator = () => {
+ if ( loading ) {
+ const text = creating
+ ? __( 'Creating…', 'google-listings-and-ads' )
+ : __( 'Updating…', 'google-listings-and-ads' );
+
+ return <AppButton loading text={ text } />;
+ }
+
if ( shouldClaimGoogleAdsAccount ) {
- return (
- <ClaimAccountButton
- loading={ isClaiming || showClaimModal }
- onClaimClick={ handleOnClaimClick }
- />
- );
+ return <ClaimAccountButton />;
}
- return googleAdsAccount.id && isUpsertAdsAccountLoading ? (
- <LoadingLabel
- text={ __( 'Updating…', 'google-listings-and-ads' ) }
- />
- ) : (
- <CreateAccountButton
- onCreateAccount={ handleOnCreateAccount }
- loading={ isUpsertAdsAccountLoading }
- />
+ return (
+ <CreateAccountButton onCreateAccount={ handleOnCreateAccount } />
);
};
@@ -98,7 +71,11 @@ const CreateAccount = ( props ) => {
>
{ allowShowExisting && ! shouldClaimGoogleAdsAccount && (
<Section.Card.Footer>
- <AppButton isLink onClick={ onShowExisting }>
+ <AppButton
+ isLink
+ disabled={ loading }
+ onClick={ onShowExisting }
+ >
{ __(
'Or, use your existing Google Ads account',
'google-listings-and-ads'
diff --git a/js/src/hooks/useUpsertAdsAccount.js b/js/src/hooks/useUpsertAdsAccount.js
index 69eff8aff..aa0cf0349 100644
--- a/js/src/hooks/useUpsertAdsAccount.js
+++ b/js/src/hooks/useUpsertAdsAccount.js
@@ -2,7 +2,7 @@
* External dependencies
*/
import { __ } from '@wordpress/i18n';
-import { useCallback, useState } from '@wordpress/element'; // Add this line
+import { useCallback, useState } from '@wordpress/element';
/**
* Internal dependencies
@@ -18,7 +18,7 @@ import useDispatchCoreNotices from '.~/hooks/useDispatchCoreNotices';
*
* @return {Array} [ upsertAdsAccount, fetchResult ]
* - `upsertAdsAccount` A function to be called to trigger `apiFetch` to create or update a Google Ads account.
- * - `fetchResult` An object containing data about the `apiFetchCallback`
+ * - `fetchResult` An object containing the state of this hook.
*
* @see useApiFetchCallback
*/
@@ -29,18 +29,21 @@ const useUpsertAdsAccount = () => {
const { createNotice } = useDispatchCoreNotices();
const { fetchGoogleAdsAccount, fetchGoogleAdsAccountStatus } =
useAppDispatch();
- const [ isFetchingAdsData, setFetchingAdsData ] = useState( false );
+ const [ currentAction, setCurrentAction ] = useState( null );
- const [ fetchCreateAccount, { loading: isAccountUpdateLoading, ...data } ] =
- useApiFetchCallback( {
- path: `${ API_NAMESPACE }/ads/accounts`,
- method: 'POST',
- data: {
- id: googleAdsAccount?.id || undefined,
- },
- } );
+ const isCreation = ! googleAdsAccount?.id;
+
+ const [ fetchCreateAccount ] = useApiFetchCallback( {
+ path: `${ API_NAMESPACE }/ads/accounts`,
+ method: 'POST',
+ data: {
+ id: googleAdsAccount?.id || undefined,
+ },
+ } );
const upsertAdsAccount = useCallback( async () => {
+ setCurrentAction( isCreation ? 'create' : 'update' );
+
try {
await fetchCreateAccount( { parse: false } );
} catch ( e ) {
@@ -59,11 +62,14 @@ const useUpsertAdsAccount = () => {
}
// Update Google Ads data in the data store after posting an account update.
- setFetchingAdsData( true );
- await fetchGoogleAdsAccount();
- await fetchGoogleAdsAccountStatus();
- setFetchingAdsData( false );
+ await Promise.all( [
+ fetchGoogleAdsAccount(),
+ fetchGoogleAdsAccountStatus(),
+ ] );
+
+ setCurrentAction( null );
}, [
+ isCreation,
createNotice,
fetchCreateAccount,
fetchGoogleAdsAccount,
@@ -73,8 +79,8 @@ const useUpsertAdsAccount = () => {
return [
upsertAdsAccount,
{
- ...data,
- loading: isAccountUpdateLoading || isFetchingAdsData,
+ loading: currentAction !== null,
+ creating: currentAction === 'create',
},
];
};| * External dependencies | ||
| */ | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { useCallback, useState } from '@wordpress/element'; // Add this line |
There was a problem hiding this comment.
The code comment looks irrelevant.
|
After syncing with @MatthiasReinholz today, we're going to go ahead and merge this into the feature branch for the updated set-up flow work and can open new tickets for any additional follow-up work. |
joemcgill
merged commit f4e55be
into
feature/setup-google-ads-in-step1
eason9487
changed the title
Changes proposed in this Pull Request:
Closes #2262.
Replace this with a good description of your changes & reasoning.
Screenshots:
Detailed test instructions:
A. Account access endpoint
Go to edit profile in the dashboard and create an application password.
Open POSTman tool and enter the url
https://localhost:8888/wp-json/wc/gla/ads/account-statusor if you have configured local tunnel using ngrok, then it should behttps://{$replace-with-ngrok-domain}/wp-json/wc/gla/ads/account-statusIn the
Authorizationtab, add username and password by selectingbasic authscheme. Password should be application password created in step 1. Ensure that user has permission tomanage_woocommerce.Go to step-1 of the onboarding journey and ensure that google account is created. Once that is done, create a new ads account.
Once new ad account is created, open POSTman and request the endpoint stated in step-2. It should return the following object.
UI
Additional details:
Changelog entry