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

Onboarding: Create new ads account. #2651

Open
wants to merge 39 commits into
base: update/2582-claim-ads-account
Choose a base branch
from

Conversation

ankitrox
Copy link
Collaborator

@ankitrox ankitrox commented Oct 25, 2024

Changes proposed in this Pull Request:

Closes #2603

Replace this with a good description of your changes & reasoning.

Screenshots:

Detailed test instructions.

Note: As this PR also depends on claim card which is being handled in #2582, an integration branch is created to test this feature.

  1. Checkout to the integration branch and build. Make sure you are disconnected from all Google accounts.

  2. Connect to the Google account with existing Google ads accounts so that the existing ads account is shown.

  3. Click on "Or, create a new Google Ads account" link. It should show you the modal.

  4. Ensure that clicking on "Cancel" button does nothing.

  5. Clicking on "Yes, I want a new account" should create a new account. While creating the new account, you shall see the text "Creating new ads account" in place of existing ads account card.

  6. Once the account is created successfully, existing ads account card will disappear and claim card will be shown.

  7. Once the ads account is claimed, you shall see the notice Google Ads conversion measurement has been set up for your store. in green color.

  8. Existing ads account card will again be shown with the Connected label and the newly created ads account ID in dropdown.

Additional details:

Changelog entry

@github-actions github-actions bot added the changelog: update Big changes to something that wasn't broken. label Oct 25, 2024
Copy link

codecov bot commented Oct 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.7%. Comparing base (1eadc09) to head (b89142d).

Additional details and impacted files

Impacted file tree graph

@@                      Coverage Diff                      @@
##           update/2582-claim-ads-account   #2651   +/-   ##
=============================================================
  Coverage                           62.7%   62.7%           
=============================================================
  Files                                325     325           
  Lines                               5163    5163           
  Branches                            1266    1266           
=============================================================
  Hits                                3239    3239           
  Misses                              1747    1747           
  Partials                             177     177           
Flag Coverage Δ
js-unit-tests 62.7% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
js/src/components/account-card/index.js 91.9% <ø> (ø)

@ankitrox ankitrox changed the title Create new ads account. Onboarding: Create new ads account. Oct 28, 2024
@asvinb
Copy link
Collaborator

asvinb commented Oct 30, 2024

@joemcgill Can you kindly review the changes I made to the PR in the following commit please:
3e2dcbe

Thanks!

@asvinb asvinb changed the base branch from update/2596-connect-ads-account to update/2582-claim-ads-account October 30, 2024 16:09
Copy link
Collaborator

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having trouble testing this accurately since I've also started hitting the errors with account creation limits but overall this is looking pretty close. I've left a few comments specific to the account creation piece.

<AccountCard
className="gla-google-combo-service-account-card--ads"
title={ __(
'Creating a new Google Ads account',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When finalizeAdsAccountCreation is true it would be better to show a more precise message, like "Connecting your Google Ads account".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Updated @joemcgill

) }
indicator={
<LoadingLabel
text={ __( 'Creating…', 'google-listings-and-ads' ) }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, if we're finalizing the account, this should probably say 'Connecting...'

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -58,14 +58,15 @@ const ConnectedGoogleComboAccountCard = () => {
}

// @TODO: edit mode implementation in 2605
const editMode = false;
const editMode = true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes the ConnectAds component to remain shown after the new account is connected, which is probably helpful for testing in development, but is confusing for QA purposes. Can this be changed back to false?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Updated!

Copy link
Collaborator

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated indicator states look better, but I think this could be refined a bit more to remove the need to handle the account creation directly in this component and consolidate that logic in the parent component.

* @return {JSX.Element} {@link AccountCard} filled with content.
*/
const ConnectAds = () => {
const ConnectAds = ( { finalizeAdsAccountCreation } ) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to pass a state called isConnecting (or similar) here. Also, we can add two new props: onCreate and isCreating and handle the upsertAdsAccount logic in the parent component. The useUpsertAdsAccount() hook returns an action prop that can be used to distinguish if we are creating a new account, or updating (i.e. connecting) a new account.

Here's a nearly complete idea of how that could work:

diff --git a/js/src/components/google-combo-account-card/connect-ads/connect-ads.js b/js/src/components/google-combo-account-card/connect-ads/connect-ads.js
index f0ec7731a..20821e5e3 100644
--- a/js/src/components/google-combo-account-card/connect-ads/connect-ads.js
+++ b/js/src/components/google-combo-account-card/connect-ads/connect-ads.js
@@ -12,7 +12,6 @@ import ConnectAdsFooter from './connect-ads-footer';
 import ConfirmCreateModal from './confirm-create-modal';
 import LoadingLabel from '.~/components/loading-label';
 import useApiFetchCallback from '.~/hooks/useApiFetchCallback';
-import useUpsertAdsAccount from '.~/hooks/useUpsertAdsAccount';
 import useDispatchCoreNotices from '.~/hooks/useDispatchCoreNotices';
 import useExistingGoogleAdsAccounts from '.~/hooks/useExistingGoogleAdsAccounts';
 import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount';
@@ -26,10 +25,12 @@ import ConnectButton from '.~/components/google-ads-account-card/connect-ads/con
  * ConnectAds component renders an account card to connect to an existing Google Ads account.
  *
  * @param {Object} props Component props.
- * @param {boolean} props.finalizeAdsAccountCreation Whether the user is in the process of finalizing the Ads account creation, i.e after the user has claimed the account and the step is conversion_action.
+ * @param {boolean} props.isConnecting Whether the user is in the process of finalizing the Ads account creation, i.e after the user has claimed the account and the step is conversion_action.
+ * @param {boolean} props.isCreating Whether the user is in the process of creating a new Ads account.
+ * @param {Function} props.onCreate A callback to fire when creating a new account.
  * @return {JSX.Element} {@link AccountCard} filled with content.
  */
-const ConnectAds = ( { finalizeAdsAccountCreation } ) => {
+const ConnectAds = ( { isConnecting, isCreating, onCreate } ) => {
 	const [ value, setValue ] = useState();
 	const [ isLoading, setLoading ] = useState( false );
 	const { createNotice } = useDispatchCoreNotices();
@@ -44,8 +45,6 @@ const ConnectAds = ( { finalizeAdsAccountCreation } ) => {
 		method: 'POST',
 		data: { id: value },
 	} );
-	const [ upsertAdsAccount, { loading: creatingNewAccount } ] =
-		useUpsertAdsAccount();
 
 	const onCreateNew = () => {
 		setShowCreateNewModal( true );
@@ -57,7 +56,7 @@ const ConnectAds = ( { finalizeAdsAccountCreation } ) => {
 
 	const handleOnContinue = async () => {
 		setShowCreateNewModal( false );
-		await upsertAdsAccount();
+		await onCreate();
 	};
 
 	useEffect( () => {
@@ -117,14 +116,14 @@ const ConnectAds = ( { finalizeAdsAccountCreation } ) => {
 
 	// Show a loading state if the Ads account is being updated or if a new Ads account is being created.
 	// If finalizeAdsAccountCreation is true, the processing is done in `ConnectedGoogleComboAccountCard`.
-	if ( creatingNewAccount || finalizeAdsAccountCreation ) {
+	if ( isConnecting || isCreating ) {
 		let title = __(
 			'Creating a new Google Ads account',
 			'google-listings-and-ads'
 		);
 		let indicatorLabel = __( 'Creating…', 'google-listings-and-ads' );
 
-		if ( finalizeAdsAccountCreation ) {
+		if ( isConnecting ) {
 			title = __(
 				'Connecting your Google Ads account',
 				'google-listings-and-ads'
diff --git a/js/src/components/google-combo-account-card/connected-google-combo-account-card.js b/js/src/components/google-combo-account-card/connected-google-combo-account-card.js
index c2866ba68..d6ba96535 100644
--- a/js/src/components/google-combo-account-card/connected-google-combo-account-card.js
+++ b/js/src/components/google-combo-account-card/connected-google-combo-account-card.js
@@ -40,7 +40,7 @@ const ConnectedGoogleComboAccountCard = () => {
 	const { invalidateResolution } = useAppDispatch();
 	const { googleAdsAccount } = useGoogleAdsAccount();
 	const { hasAccess, step } = useGoogleAdsAccountStatus();
-	const [ upsertAdsAccount ] = useUpsertAdsAccount();
+	const [ upsertAdsAccount, { loading, action } ] = useUpsertAdsAccount();
 
 	const finalizeAdsAccountCreation =
 		hasAccess === true && step === 'conversion_action';
@@ -116,7 +116,9 @@ const ConnectedGoogleComboAccountCard = () => {
 
 			{ showConnectAds && (
 				<ConnectAds
-					finalizeAdsAccountCreation={ finalizeAdsAccountCreation }
+					isConnecting={ loading && action === 'update' }
+					isCreating={ loading && action === 'create' }
+					onCreate={ () => upsertAdsAccount() }
 				/>
 			) }
 		</div>

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense @joemcgill . Updated.

Copy link
Collaborator

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updates look good and E2E tests are passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: update Big changes to something that wasn't broken.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Onboarding: Create new Ads account when there are existing accounts in Google Accounts Card
3 participants