R2: Updated Enhanced Conversion flow #2361

asvinb · 2024-04-10T08:32:08Z

Changes proposed in this Pull Request:

Closes #2364 .

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

Screenshots:

Detailed test instructions:

Additional details:

Changelog entry

joemcgill

@asvinb this is already starting off great. I'm getting the requirements finalized and the biggest change is that we don't want to show the second "Enable" screen from the modal once the user returns from enabling EC in the GAFE. I've left some thoughts inline.

joemcgill · 2024-04-11T14:33:17Z

js/src/hooks/useAutoCheckEnhancedConversionTOS.js

-		startEnhancedConversionTOSPolling,
-		stopEnhancedConversionTOSPolling,
-	};
+	return polling;


We're no longer using any return value from this hook, so this could be removed. Alternately, we could return an object containing the polling status and the return value of acceptedCustomerDataTerms() as a convenience. So components wouldn't need to use this alongside the acceptedCustomerDataTerms() hook, ex:

Suggested change

return polling;

return {

isPolling,

acceptedCustomerDataTerms,

hasFinishedResolution

};

Sounds good! Updated @joemcgill

js/src/components/enhanced-conversion-tracking-settings/enable.js

joemcgill · 2024-04-11T15:11:52Z

js/src/product-feed/submission-success-guide/enhanced-conversions/cta.js

@@ -0,0 +1,52 @@
+/**


I'd suggest that this component would be better located with the settings folder since the success modal may go away once promoting EC is no longer a priority, but the settings panel will remain.

joemcgill · 2024-04-11T15:22:23Z

js/src/product-feed/submission-success-guide/enhanced-conversions/cta.js

+	// We could have used the return value from useAutoCheckEnhancedConversionTOS to know
+	// if there's polling in progress. However from a UI point of view, it'll be confusing
+	// for the user when refreshing the page to see a loading spinner while polling is in progress.
+	// Hence we are showing the spinner only when the user clicks on the Enable button.


Given that we want to be able to optionally enable polling, I think it might be clearer if we moved all of this state into the useAutoCheckEnhancedConversionTOS() hook, and return a function that allows this consuming component to enable polling when needed. We would probably need to pass an initial polling state to the hook.

Nice improvement. Updated!

joemcgill · 2024-04-11T16:07:36Z

js/src/product-feed/submission-success-guide/enhanced-conversions/index.js

+	let title = __(
+		'Optimize your conversion tracking with Enhanced Conversions',
+		'google-listings-and-ads'
+	);
+
+	if ( acceptedCustomerDataTerms ) {
+		title = __(
+			'Your Enhanced Conversions are almost ready',
+			'google-listings-and-ads'
+		);
+	}


This could be written as a const:

Suggested change

let title = __(

'Optimize your conversion tracking with Enhanced Conversions',

'google-listings-and-ads'

);

if ( acceptedCustomerDataTerms ) {

title = __(

'Your Enhanced Conversions are almost ready',

'google-listings-and-ads'

);

}

const title = acceptedCustomerDataTerms

? __(

'Your Enhanced Conversions are almost ready',

'google-listings-and-ads'

)

: __(

'Optimize your conversion tracking with Enhanced Conversions',

'google-listings-and-ads'

);

joemcgill · 2024-04-11T16:19:27Z

js/src/product-feed/submission-success-guide/enhanced-conversions/index.js

+			return <AppSpinner />;
+		}
+
+		if ( ! acceptedCustomerDataTerms ) {


The copy for this has been updated so that the panel is not as long. See: https://docs.google.com/document/d/1U1Wiymz51aRsUZQKEHit-y9o1kiSdkEcBV-8e3BXJiM/edit#heading=h.27ah3dar1ze0.

Updated @joemcgill

joemcgill · 2024-04-11T16:43:26Z

js/src/product-feed/submission-success-guide/enhanced-conversions/confirm-button.js

+const ConfirmButton = ( { onConfirm = noop } ) => {
+	const { updateEnhancedAdsConversionStatus } = useAppDispatch();
+
+	const handleConfirm = () => {


We're going to need to move this out to the CTA component and pass it in as part of the onConfirm callback that is passed to this component because the requirements have changed slightly, and we want to automatically confirm that this is enabled once the user has clicked from the T&C modal and returned, without needing to take a second step of also confirming again. This should also work in the settings panel if the user clicks the EnableButton and then returns to the settings page.

asvinb · 2024-04-16T10:29:17Z

@joemcgill PR is ready for another round of review. To know whether we should skip the confirmation screen (because we are using the same components), I added a new property in the data store since we need to use that value in cta.js and index.js. I tried to have it as a state variable but unfortunately the components are rendered differently. Below are screenshots of the different screens within the modal:

Prompt user to enable EC (user has not accepted terms):

Confirmation (user has accepted terms but has not enabled EC in the store):

Success (after enabling EC):

Let me know if you have any questions

joemcgill

Thanks @asvinb. I started doing a new review but realized that the new success flow is not quite right. When I test the EC modal with T&C set to false, I would expect the following flow:

Show the accept-terms page
Click the accept terms CTA
Visit GAFE to enable EC and accept terms
Return to the modal (now with terms accepted)
Show the success page

Instead, when I returned to the modal from the GAFE, I am shown the confirm page. The confirm page should only be shown if the modal is opened with the terms already accepted. I can see that you're setting the skip confirmation state in the EnableButton, but it doesn't look like that state is ever reaching the data store for some reason.

I left some notes inline, but would be great to double check that this flow is working as expected.

joemcgill · 2024-04-12T15:28:27Z

js/src/components/enhanced-conversion-tracking-settings/enable-button.js

-			{ __( 'Enable Enhanced Conversions', 'google-listings-and-ads' ) }
-		</AppButton>
+		<AppButton
+			icon={ externalIcon }


✨ perfect icon choice

joemcgill · 2024-04-17T01:59:47Z

js/src/product-feed/submission-success-guide/enhanced-conversions/index.js

+		if (
+			acceptedCustomerDataTerms &&
+			allowEnhancedConversions !==
+				ENHANCED_ADS_CONVERSION_STATUS.ENABLED &&
+			! skipConfirmation
+		) {
+			return 'confirm';
+		}


The logic here seems incorrect to me. Seems like there could be a case where you have accepted terms, but not enabled EC, and if the skipConfirmation setting isn't set, you would end up with the accept-terms page. Is that expected?

It may be more clear to do these in the order of loading, accept-terms, confirm, and finally success. That way a simple if ( ! acceptedCustomerDataTerms ) { ... would result in the accept-terms page, and you would only reach success if all other cases get passed.

I changed the order as you suggested @joemcgill . Let me know what you think.

joemcgill · 2024-04-17T02:55:42Z

js/src/components/enhanced-conversion-tracking-settings/enable-button.js

+		updateEnhancedAdsConversionStatus(
+			ENHANCED_ADS_CONVERSION_STATUS.PENDING
+		);
+		updateEnhancedConversionsSkipConfirmation( true );


This is only needed for the modal, so probably better to pass this in as part of the onEnable handler from the modal, rather than explicitly setting this here.

👍 The updateEnhancedConversionsSkipConfirmation action has been moved. I've kept updateEnhancedAdsConversionStatus since it's needed when the CTA is rendered within the Settings panel.

js/src/components/enhanced-conversion-tracking-settings/enable.js

asvinb added 5 commits April 9, 2024 16:55

Add minor CR feedback.

5e72c9e

Review enhanced conversions panel wordings and CTAs.

5033e0d

Refactor EC pannel settings.

2582e18

Changes to useTermsPolling hook.

0a35f0c

Delete unused useGoogleAdsEnhancedConversionTermsURL.js.

891676a

asvinb requested a review from joemcgill April 10, 2024 08:32

asvinb added 3 commits April 10, 2024 12:36

Simplify useGoogleAdsEnhancedConversionSettingsURL.js hook.

ec33c2a

Removed unused imports.

6a95b1f

Remove copy to minimize modal height.

95cc03e

eason9487 assigned asvinb Apr 10, 2024

joemcgill requested changes Apr 11, 2024

View reviewed changes

joemcgill linked an issue Apr 11, 2024 that may be closed by this pull request

Update Enhanced Conversions onboarding flow #2364

Open

asvinb added 5 commits April 12, 2024 15:16

Address CR feedback.

489a3f4

Address CR feedback.

9141797

Remove unused import.

5f4648f

Add Success screen.

e9dcd38

Fix failing tests.

1671f7c

joemcgill requested changes Apr 17, 2024

View reviewed changes

Address CR feedback.

62b19a2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R2: Updated Enhanced Conversion flow #2361

R2: Updated Enhanced Conversion flow #2361

asvinb commented Apr 10, 2024 •

edited by joemcgill

joemcgill left a comment

joemcgill Apr 11, 2024

asvinb Apr 12, 2024

joemcgill Apr 11, 2024

asvinb Apr 12, 2024

joemcgill Apr 11, 2024

asvinb Apr 12, 2024

joemcgill Apr 11, 2024

asvinb Apr 12, 2024

joemcgill Apr 11, 2024

asvinb Apr 12, 2024

joemcgill Apr 11, 2024

asvinb Apr 12, 2024

asvinb commented Apr 16, 2024

joemcgill left a comment

joemcgill Apr 12, 2024

joemcgill Apr 17, 2024

asvinb Apr 17, 2024

joemcgill Apr 17, 2024

asvinb Apr 17, 2024

-	return polling;
+	return {
+		isPolling,
+		acceptedCustomerDataTerms,
+		hasFinishedResolution
+	};

R2: Updated Enhanced Conversion flow #2361

Are you sure you want to change the base?

R2: Updated Enhanced Conversion flow #2361

Conversation

asvinb commented Apr 10, 2024 • edited by joemcgill

Changes proposed in this Pull Request:

Screenshots:

Detailed test instructions:

Additional details:

Changelog entry

joemcgill 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvinb commented Apr 16, 2024

joemcgill 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

asvinb commented Apr 10, 2024 •

edited by joemcgill