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

R2 – Enable Enhanced Conversions #2242

Open
wants to merge 125 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
125 commits
Select commit Hold shift + click to select a range
f32b67f
Save WIP.
asvinb Feb 2, 2024
f225bdd
Get correct value from API for accepted terms.
asvinb Feb 2, 2024
41754d0
Add comments to the selectors.
asvinb Feb 6, 2024
6302778
Try to filter out the difference screens.
asvinb Feb 6, 2024
bfb368f
Fix comment.
asvinb Feb 6, 2024
dd662d9
Fix failing tests.
asvinb Feb 9, 2024
748a9d6
Fix failing tests.
asvinb Feb 9, 2024
864d74b
Hopefully last of PHP fixes.
asvinb Feb 9, 2024
43595c3
Add SettingsPanel.
asvinb Feb 9, 2024
97f0449
Common CTA component for Enhanced Conversion.
asvinb Feb 9, 2024
bb6e289
Fix polling.
asvinb Feb 12, 2024
2ee6242
API endpoint to read/write enhanced conversion status
ankitrox Feb 12, 2024
0eab3d7
Conflict resolution
ankitrox Feb 12, 2024
e29cbc9
Update docblock
ankitrox Feb 12, 2024
af8eaf8
Add tests for CTA component.
asvinb Feb 12, 2024
0b00e2a
Merge branch 'feature/2239-enhanced-conversions-panel' of github.com:…
asvinb Feb 12, 2024
4b6461d
API to read/write EC
ankitrox Feb 12, 2024
904d485
Merge branch 'feature/2239-enhanced-conversions-panel' of https://git…
ankitrox Feb 12, 2024
c6c0b25
Fix: tests
ankitrox Feb 12, 2024
717ad0a
Tests: update and get EC status
ankitrox Feb 12, 2024
3ee62e4
Tests: ads controller
ankitrox Feb 12, 2024
15e6a95
Add EC status to valid options
ankitrox Feb 12, 2024
022200c
Merge branch 'develop' into feature/2239-enhanced-conversions-panel
asvinb Feb 13, 2024
e807c0c
Merge branch 'feature/2239-enhanced-conversions-panel' of github.com:…
asvinb Feb 13, 2024
636f52b
Fix: phpcs
ankitrox Feb 13, 2024
13e7ee8
Fix linting errors.
asvinb Feb 13, 2024
55ba363
Merge branch 'feature/2239-enhanced-conversions-panel' of https://git…
ankitrox Feb 13, 2024
71bfadb
Remove non-required test
ankitrox Feb 13, 2024
dbd8a18
Use new hook instead of usePolling hook for pending status.
asvinb Feb 13, 2024
a9161bd
Merge branch 'feature/2239-enhanced-conversions-panel' of github.com:…
asvinb Feb 13, 2024
679253d
Fix linting issues.
asvinb Feb 13, 2024
16b6767
Tests: Ads controller
ankitrox Feb 13, 2024
cff4675
Add config for enhanced conversion
ankitrox Feb 13, 2024
c063f68
Review condition to display EC screen.
asvinb Feb 14, 2024
78b97fd
Fix race condition.
asvinb Feb 15, 2024
b85f7de
Add more tests.
asvinb Feb 15, 2024
5af67a9
Tests to enable/disable CTA.
asvinb Feb 15, 2024
07f16ff
Add tests for submission guide.
asvinb Feb 15, 2024
3302f7e
Add user data on purchase page for enhanced conversion
ankitrox Feb 15, 2024
9225fa4
Merge branch 'feature/2239-enhanced-conversions-panel' of https://git…
ankitrox Feb 15, 2024
26c0a65
Fix: phpcs issues
ankitrox Feb 15, 2024
d613825
revert tag changes
ankitrox Feb 15, 2024
783229f
Option to store data terms in options table
ankitrox Feb 16, 2024
e180b48
Fix: test for deleting option
ankitrox Feb 16, 2024
672800e
Update src/Google/GlobalSiteTag.php
ankitrox Feb 16, 2024
e3cf005
E2E: Enhanced conversion user data test
ankitrox Feb 16, 2024
7523ea1
Merge branch 'update/52310672-google-tag-enhanced-conversion' of http…
ankitrox Feb 16, 2024
4d53889
Rename folders to be consistent with codebase.
asvinb Feb 20, 2024
120d10c
Add adsConnected property to glaData.
asvinb Feb 20, 2024
327da7d
Merge branch 'feature/2239-enhanced-conversions-panel-qa' into featur…
asvinb Feb 20, 2024
2390178
Have a single endpoint.
asvinb Feb 20, 2024
23eff1b
Add PendingNotice component.
asvinb Feb 20, 2024
14f926b
Make use of adsConnected property og glaData.
asvinb Feb 20, 2024
d6c0039
Add adsConnected to global properties.
asvinb Feb 20, 2024
f49f341
Update conditions.
asvinb Feb 20, 2024
d43057a
Add filter for TOS value.
asvinb Feb 20, 2024
5b0731d
Fix condition to display pending state.
asvinb Feb 21, 2024
93f9c4b
Conflict resolve: develop
ankitrox Feb 26, 2024
4eae5f4
Resolve conflicts: merge develop
ankitrox Feb 26, 2024
9ccbba8
Resolve conflict: merge feature/2239-enhanced-conversions-panel
ankitrox Feb 26, 2024
7549b7a
Update src/API/Google/Ads.php
ankitrox Feb 28, 2024
b61d133
Update options name with ADS_ prefix
ankitrox Feb 28, 2024
4b7def0
Fix: phpcs tests
ankitrox Feb 28, 2024
eff7785
Merge branch 'develop' into feature/2239-enhanced-conversions-panel
ankitrox Feb 28, 2024
62fb97e
Merge branch 'feature/2239-enhanced-conversions-panel' into update/52…
ankitrox Feb 28, 2024
78f6774
Address code review feedback.
asvinb Mar 1, 2024
01e67fd
Merge branch 'feature/2239-enhanced-conversions-panel' of github.com:…
asvinb Mar 1, 2024
6d1c4b2
Add test for disabled button.
asvinb Mar 4, 2024
85e005a
Conditionally add enhanced conversion event
ankitrox Mar 4, 2024
1920f47
Add adsId property.
asvinb Mar 4, 2024
b43997b
Normalize and hash data
ankitrox Mar 4, 2024
a4e289a
Merge branch 'feature/2239-enhanced-conversions-panel' into update/52…
ankitrox Mar 4, 2024
365139c
Fix: algo name
ankitrox Mar 4, 2024
803ddc0
Merge branch 'develop' into feature/2239-enhanced-conversions-panel
asvinb Mar 6, 2024
db349d2
Revert to adsConnected property.
asvinb Mar 6, 2024
060aaac
Rename prop from onClose to onModalClose.
asvinb Mar 6, 2024
8a4d44e
Simplify the useAutoCheckEnhancedConversionTOS hook.
asvinb Mar 6, 2024
42a772e
Remove check to display settings.
asvinb Mar 6, 2024
1c57792
Fix: key name for hashed data
ankitrox Mar 7, 2024
4440b27
Merge branch 'develop' into feature/2239-enhanced-conversions-panel
joemcgill Mar 8, 2024
e369351
CR feedback implementation.
asvinb Mar 12, 2024
d6e6dba
Merge branch 'feature/2239-enhanced-conversions-panel' of github.com:…
asvinb Mar 12, 2024
e12a768
Remove unused properties from mocks.
asvinb Mar 12, 2024
094b7fb
Fix linting errors.
asvinb Mar 12, 2024
dd21b22
Use Toggle instead of Button for Enhanced Conversion Settings panel.
asvinb Mar 12, 2024
d94a2d2
Show Spinner while resolving Enhanced Conversion status.
asvinb Mar 12, 2024
d896b88
Merge branch 'develop' into feature/2239-enhanced-conversions-panel
ankitrox Mar 13, 2024
ea8cd23
Resolve conflicts: merge develop
ankitrox Mar 13, 2024
ed233e3
Update tests.
asvinb Mar 14, 2024
2738535
Merge branch 'feature/2239-enhanced-conversions-panel' of github.com:…
asvinb Mar 14, 2024
3fb3910
Update tests to remove isResolving.
asvinb Mar 14, 2024
8293c5a
Check for valid Google Ads account and ID.
asvinb Mar 14, 2024
e93b854
Add EC data prior to firing events
joemcgill Mar 15, 2024
c0da595
Update e2e tests
joemcgill Mar 15, 2024
1afe1d5
Remove E2E tests for now
joemcgill Mar 15, 2024
ce4441e
Add e2e tests
joemcgill Mar 15, 2024
84343ae
Fix JSDoc errors
joemcgill Mar 16, 2024
8ee59b6
Fix tests
joemcgill Mar 16, 2024
72a2f2a
Enable EC for e2e test
joemcgill Mar 17, 2024
aaa28be
Make disabled test more robust
joemcgill Mar 17, 2024
8a87321
Merge branch 'develop' into feature/2239-enhanced-conversions-panel
asvinb Mar 19, 2024
40a0e8e
Remove unused adsConnected property.
asvinb Mar 19, 2024
35ab06f
Add useGoogleAdsTermsURL hook.
asvinb Mar 19, 2024
5d6d8c1
Show button when EC is disabled.
asvinb Mar 20, 2024
a36070b
Fix js tests.
asvinb Mar 20, 2024
512f985
Remove unused prop.
asvinb Mar 20, 2024
7e60441
Add todo note for R1 PR.
asvinb Mar 20, 2024
7d60a3f
Add normalization for email address
ankitrox Mar 20, 2024
447f12a
Resolve conflicts during pull
ankitrox Mar 20, 2024
41189eb
Add ocid in connection response
ankitrox Mar 20, 2024
e82a9a9
Merge branch 'feature/2239-enhanced-conversions-panel-deeplink' into …
asvinb Mar 20, 2024
95dc435
Rename constant.
asvinb Mar 20, 2024
b568996
Remove unused code for grabbing the ocid via endpoint.
asvinb Mar 20, 2024
996286f
Remove redundant code
ankitrox Mar 20, 2024
37495fa
Merge branch 'feature/2239-enhanced-conversions-panel' of https://git…
ankitrox Mar 20, 2024
c2d73c2
Remove hashing from non PII fields and fix merge
joemcgill Mar 21, 2024
02a9303
Add local state for pending status.
asvinb Mar 21, 2024
727b0c0
Merge branch 'feature/2239-enhanced-conversions-panel' of github.com:…
asvinb Mar 21, 2024
ed9b948
Merge pull request #2258 from woocommerce/update/52310672-google-tag-…
joemcgill Mar 21, 2024
f824205
Global Site Tag: Move EC data to private method
joemcgill Mar 21, 2024
5ac23ec
Add docblock
joemcgill Mar 21, 2024
cde898f
Improve loading of EC success modal
joemcgill Mar 21, 2024
33f835a
Fix tests and linting
joemcgill Mar 21, 2024
e08cbf0
Fix: fatal error when ads account disconnected
ankitrox Mar 26, 2024
a3fb3e6
Merge branch 'feature/2239-enhanced-conversions-panel' of https://git…
ankitrox Mar 26, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/**
* External dependencies
*/
import { __ } from '@wordpress/i18n';
import { noop } from 'lodash';
import { useCallback } from '@wordpress/element';

/**
* Internal dependencies
*/
import { ENHANCED_ADS_CONVERSION_STATUS } from '.~/constants';
import { useAppDispatch } from '.~/data';
import AppButton from '.~/components/app-button';
import useGoogleAdsEnhancedConversionTermsURL from '.~/hooks/useGoogleAdsTermsURL';

const AcceptTerms = ( {
acceptTermsLabel = __(
'Accept Terms & Conditions',
'google-listings-and-ads'
),
onAcceptTerms = noop,
} ) => {
const { url } = useGoogleAdsEnhancedConversionTermsURL();
const { updateEnhancedAdsConversionStatus } = useAppDispatch();

const handleAcceptTerms = useCallback(
( event ) => {
event.preventDefault();
Copy link
Member

Choose a reason for hiding this comment

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

Wonder why it needs to call preventDefault?


window.open( url, '_blank' );
updateEnhancedAdsConversionStatus(
ENHANCED_ADS_CONVERSION_STATUS.PENDING
);
onAcceptTerms();
},
[ updateEnhancedAdsConversionStatus, url, onAcceptTerms ]
);

return (
<AppButton isPrimary onClick={ handleAcceptTerms }>
{ acceptTermsLabel }
</AppButton>
);
};

export default AcceptTerms;
98 changes: 98 additions & 0 deletions js/src/components/enhanced-conversion-tracking-settings/cta.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/**
* External dependencies
*/
import { noop } from 'lodash';
import { __ } from '@wordpress/i18n';
import { useCallback, useState, useEffect } from '@wordpress/element';

/**
* Internal dependencies
*/
import { ENHANCED_ADS_CONVERSION_STATUS } from '.~/constants';
import { useAppDispatch } from '.~/data';
import AppButton from '.~/components/app-button';
import AcceptTerms from './accept-terms';
import useAcceptedCustomerDataTerms from '.~/hooks/useAcceptedCustomerDataTerms';
import useAllowEnhancedConversions from '.~/hooks/useAllowEnhancedConversions';
import useTermsPolling from './useTermsPolling';

const CTA = ( {
disableLabel = __( 'Disable', 'google-listings-and-ads' ),
enableLabel = __( 'Enable', 'google-listings-and-ads' ),
onEnableClick = noop,
onDisableClick = noop,
} ) => {
const [ startBackgroundPoll, setStartBackgroundPoll ] = useState( false );
const { updateEnhancedAdsConversionStatus } = useAppDispatch();
const { acceptedCustomerDataTerms } = useAcceptedCustomerDataTerms();
const { allowEnhancedConversions } = useAllowEnhancedConversions();
useTermsPolling( startBackgroundPoll );

const handleDisable = useCallback( () => {
if ( ! acceptedCustomerDataTerms ) {
return;

Check warning on line 33 in js/src/components/enhanced-conversion-tracking-settings/cta.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/enhanced-conversion-tracking-settings/cta.js#L33

Added line #L33 was not covered by tests
}

updateEnhancedAdsConversionStatus(
ENHANCED_ADS_CONVERSION_STATUS.DISABLED
);

onDisableClick();
}, [
updateEnhancedAdsConversionStatus,
acceptedCustomerDataTerms,
onDisableClick,
] );

// Turn off polling when the user has accepted the terms.
useEffect( () => {
if ( acceptedCustomerDataTerms && startBackgroundPoll ) {
setStartBackgroundPoll( false );

Check warning on line 50 in js/src/components/enhanced-conversion-tracking-settings/cta.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/enhanced-conversion-tracking-settings/cta.js#L50

Added line #L50 was not covered by tests
}
}, [ acceptedCustomerDataTerms, startBackgroundPoll ] );
Comment on lines +47 to +52
Copy link
Member

@eason9487 eason9487 Mar 28, 2024

Choose a reason for hiding this comment

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

Using another state startBackgroundPoll and creating another layer hook useTermsPolling to serve all the data polling processing for the accepted status of customer data terms seems too overly complex.

I think the same results could be achieved in a more simple way:

  1. Move the required condition check from useTermsPolling to useAutoCheckEnhancedConversionTOS
  2. Move the determinations of starting and stopping the polling to useAutoCheckEnhancedConversionTOS
  3. Return the isPolling from useAutoCheckEnhancedConversionTOS
  4. Call the useAutoCheckEnhancedConversionTOS hook in EnhancedConversionTrackingSettings instead of AcceptTerms

It might also solve the issue in #2242 (comment).

Combining the consideration in #2242 (comment). I think such an edge and unexpected case can be ignored, even though the scenario is technically arrivable.

2. It is possible for the EC modal to be displayed in an enabled sate (comment)

This is still technically true, but we are now making sure the previous options are removed when an Ads account is disconnected (here). This fixes the situation you mentioned where disconnecting accounts and going back through the setup process could result in a user seeing this state.

Now, the only way that I think a user could end in this state is if the options failed to delete for some reason, or if they got set manually outside of the expected onboarding process. This all seems pretty edge case to me, so we've not added an additional check to disable the second page of the modal if paid ads are already set up AND EC has already been enabled.

The relevant code could be simpler, for example, I prepared a git diff result based on the revision a3fb3e6:

diff --git a/js/src/components/enhanced-conversion-tracking-settings/accept-terms.js b/js/src/components/enhanced-conversion-tracking-settings/accept-terms.js
index 832274b94..9c5a1799f 100644
--- a/js/src/components/enhanced-conversion-tracking-settings/accept-terms.js
+++ b/js/src/components/enhanced-conversion-tracking-settings/accept-terms.js
@@ -2,7 +2,6 @@
  * External dependencies
  */
 import { __ } from '@wordpress/i18n';
-import { noop } from 'lodash';
 import { useCallback } from '@wordpress/element';
 
 /**
@@ -18,7 +17,6 @@ const AcceptTerms = ( {
 		'Accept Terms & Conditions',
 		'google-listings-and-ads'
 	),
-	onAcceptTerms = noop,
 } ) => {
 	const { url } = useGoogleAdsEnhancedConversionTermsURL();
 	const { updateEnhancedAdsConversionStatus } = useAppDispatch();
@@ -31,9 +29,8 @@ const AcceptTerms = ( {
 			updateEnhancedAdsConversionStatus(
 				ENHANCED_ADS_CONVERSION_STATUS.PENDING
 			);
-			onAcceptTerms();
 		},
-		[ updateEnhancedAdsConversionStatus, url, onAcceptTerms ]
+		[ updateEnhancedAdsConversionStatus, url ]
 	);
 
 	return (
diff --git a/js/src/components/enhanced-conversion-tracking-settings/cta.js b/js/src/components/enhanced-conversion-tracking-settings/cta.js
index dc2ff94ad..897316d72 100644
--- a/js/src/components/enhanced-conversion-tracking-settings/cta.js
+++ b/js/src/components/enhanced-conversion-tracking-settings/cta.js
@@ -3,7 +3,7 @@
  */
 import { noop } from 'lodash';
 import { __ } from '@wordpress/i18n';
-import { useCallback, useState, useEffect } from '@wordpress/element';
+import { useCallback } from '@wordpress/element';
 
 /**
  * Internal dependencies
@@ -13,43 +13,16 @@ import { useAppDispatch } from '.~/data';
 import AppButton from '.~/components/app-button';
 import AcceptTerms from './accept-terms';
 import useAcceptedCustomerDataTerms from '.~/hooks/useAcceptedCustomerDataTerms';
-import useAllowEnhancedConversions from '.~/hooks/useAllowEnhancedConversions';
-import useTermsPolling from './useTermsPolling';
+import useAutoCheckEnhancedConversionTOS from '.~/hooks/useAutoCheckEnhancedConversionTOS';
 
 const CTA = ( {
-	disableLabel = __( 'Disable', 'google-listings-and-ads' ),
 	enableLabel = __( 'Enable', 'google-listings-and-ads' ),
 	onEnableClick = noop,
-	onDisableClick = noop,
 } ) => {
-	const [ startBackgroundPoll, setStartBackgroundPoll ] = useState( false );
 	const { updateEnhancedAdsConversionStatus } = useAppDispatch();
 	const { acceptedCustomerDataTerms } = useAcceptedCustomerDataTerms();
-	const { allowEnhancedConversions } = useAllowEnhancedConversions();
-	useTermsPolling( startBackgroundPoll );
 
-	const handleDisable = useCallback( () => {
-		if ( ! acceptedCustomerDataTerms ) {
-			return;
-		}
-
-		updateEnhancedAdsConversionStatus(
-			ENHANCED_ADS_CONVERSION_STATUS.DISABLED
-		);
-
-		onDisableClick();
-	}, [
-		updateEnhancedAdsConversionStatus,
-		acceptedCustomerDataTerms,
-		onDisableClick,
-	] );
-
-	// Turn off polling when the user has accepted the terms.
-	useEffect( () => {
-		if ( acceptedCustomerDataTerms && startBackgroundPoll ) {
-			setStartBackgroundPoll( false );
-		}
-	}, [ acceptedCustomerDataTerms, startBackgroundPoll ] );
+	const isPolling = useAutoCheckEnhancedConversionTOS();
 
 	const handleEnable = useCallback( () => {
 		if ( ! acceptedCustomerDataTerms ) {
@@ -67,23 +40,18 @@ const CTA = ( {
 		onEnableClick,
 	] );
 
-	const handleOnAcceptTerms = () => {
-		setStartBackgroundPoll( true );
-	};
-
-	if ( startBackgroundPoll ) {
-		return <AppButton isSecondary disabled loading />;
+	if ( isPolling ) {
+		return <AppButton isSecondary loading />;
 	}
 
 	if ( ! acceptedCustomerDataTerms ) {
-		return <AcceptTerms onAcceptTerms={ handleOnAcceptTerms } />;
-	}
-
-	if ( allowEnhancedConversions === ENHANCED_ADS_CONVERSION_STATUS.ENABLED ) {
 		return (
-			<AppButton isPrimary isDestructive onClick={ handleDisable }>
-				{ disableLabel }
-			</AppButton>
+			<AcceptTerms
+				acceptTermsLabel={ __(
+					'Sign terms of service on Google Ads',
+					'google-listings-and-ads'
+				) }
+			/>
 		);
 	}
 
diff --git a/js/src/components/enhanced-conversion-tracking-settings/index.js b/js/src/components/enhanced-conversion-tracking-settings/index.js
index f9f95bcfc..5886eba87 100644
--- a/js/src/components/enhanced-conversion-tracking-settings/index.js
+++ b/js/src/components/enhanced-conversion-tracking-settings/index.js
@@ -12,6 +12,7 @@ import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount';
 import Section from '.~/wcdl/section';
 import PendingNotice from '.~/components/enhanced-conversion-tracking-settings/pending-notice';
 import useAllowEnhancedConversions from '.~/hooks/useAllowEnhancedConversions';
+import useAutoCheckEnhancedConversionTOS from '.~/hooks/useAutoCheckEnhancedConversionTOS';
 import Toggle from './toggle';
 import AcceptTerms from './accept-terms';
 
@@ -34,6 +35,8 @@ const EnhancedConversionTrackingSettings = () => {
 	const { acceptedCustomerDataTerms } = useAcceptedCustomerDataTerms();
 	const { allowEnhancedConversions } = useAllowEnhancedConversions();
 
+	useAutoCheckEnhancedConversionTOS();
+
 	// @todo: Remove condition once R1 PRs are merged since there should always be a connected Ads account.
 	if ( ! googleAdsAccount || ! googleAdsAccount.id ) {
 		return null;
diff --git a/js/src/components/enhanced-conversion-tracking-settings/pending-notice.js b/js/src/components/enhanced-conversion-tracking-settings/pending-notice.js
index 1fb49e8b8..b2839e4f9 100644
--- a/js/src/components/enhanced-conversion-tracking-settings/pending-notice.js
+++ b/js/src/components/enhanced-conversion-tracking-settings/pending-notice.js
@@ -10,13 +10,11 @@ import { createInterpolateElement, useCallback } from '@wordpress/element';
 import { ENHANCED_ADS_CONVERSION_STATUS } from '.~/constants';
 import { useAppDispatch } from '.~/data';
 import TrackableLink from '.~/components/trackable-link';
-import useTermsPolling from './useTermsPolling';
 import useGoogleAdsEnhancedConversionTermsURL from '.~/hooks/useGoogleAdsTermsURL';
 
 const PendingNotice = () => {
 	const { updateEnhancedAdsConversionStatus } = useAppDispatch();
 	const { url } = useGoogleAdsEnhancedConversionTermsURL();
-	useTermsPolling();
 
 	const handleOnClick = useCallback(
 		( event ) => {
diff --git a/js/src/hooks/useAutoCheckEnhancedConversionTOS.js b/js/src/hooks/useAutoCheckEnhancedConversionTOS.js
index 1a576165c..00d2a5881 100644
--- a/js/src/hooks/useAutoCheckEnhancedConversionTOS.js
+++ b/js/src/hooks/useAutoCheckEnhancedConversionTOS.js
@@ -1,25 +1,25 @@
 /**
  * External dependencies
  */
-import { useCallback, useState } from '@wordpress/element';
+import { useCallback } from '@wordpress/element';
 
 /**
  * Internal dependencies
  */
+import { ENHANCED_ADS_CONVERSION_STATUS } from '.~/constants';
 import { useAppDispatch } from '.~/data';
 import useWindowFocusCallbackIntervalEffect from '.~/hooks/useWindowFocusCallbackIntervalEffect';
+import useAcceptedCustomerDataTerms from '.~/hooks/useAcceptedCustomerDataTerms';
+import useAllowEnhancedConversions from '.~/hooks/useAllowEnhancedConversions';
 
 const useAutoCheckEnhancedConversionTOS = () => {
-	const [ polling, setPolling ] = useState( false );
+	const { acceptedCustomerDataTerms } = useAcceptedCustomerDataTerms();
+	const { allowEnhancedConversions } = useAllowEnhancedConversions();
 	const { fetchAcceptedCustomerDataTerms } = useAppDispatch();
 
-	const startEnhancedConversionTOSPolling = useCallback( () => {
-		setPolling( true );
-	}, [] );
-
-	const stopEnhancedConversionTOSPolling = useCallback( () => {
-		setPolling( false );
-	}, [] );
+	const polling =
+		! acceptedCustomerDataTerms &&
+		allowEnhancedConversions === ENHANCED_ADS_CONVERSION_STATUS.PENDING;
 
 	const checkStatus = useCallback( async () => {
 		if ( ! polling ) {
@@ -31,10 +31,7 @@ const useAutoCheckEnhancedConversionTOS = () => {
 
 	useWindowFocusCallbackIntervalEffect( checkStatus, 30 );
 
-	return {
-		startEnhancedConversionTOSPolling,
-		stopEnhancedConversionTOSPolling,
-	};
+	return polling;
 };
 
 export default useAutoCheckEnhancedConversionTOS;
diff --git a/js/src/product-feed/submission-success-guide/enhanced-conversion/actions.js b/js/src/product-feed/submission-success-guide/enhanced-conversion/actions.js
index fca858d63..d1e2e23ff 100644
--- a/js/src/product-feed/submission-success-guide/enhanced-conversion/actions.js
+++ b/js/src/product-feed/submission-success-guide/enhanced-conversion/actions.js
@@ -15,7 +15,7 @@ import CTA from '.~/components/enhanced-conversion-tracking-settings/cta';
 const Actions = ( { onModalClose = noop } ) => {
 	const { createNotice } = useDispatchCoreNotices();
 
-	const handleEnableOrDisableClick = useCallback( () => {
+	const handleEnableClick = useCallback( () => {
 		createNotice(
 			'info',
 			__( 'Status successfully set', 'google-listings-and-ads' )
@@ -38,12 +38,7 @@ const Actions = ( { onModalClose = noop } ) => {
 			</AppButton>
 
 			<CTA
-				onEnableClick={ handleEnableOrDisableClick }
-				onDisableClick={ handleEnableOrDisableClick }
-				acceptTermsLabel={ __(
-					'Sign terms of service on Google Ads',
-					'google-listings-and-ads'
-				) }
+				onEnableClick={ handleEnableClick }
 				enableLabel={ __( 'Confirm', 'google-listings-and-ads' ) }
 			/>
 		</Fragment>

(Please note that the diff result only contains the minimal adjustments to present the idea)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the suggestion and for confirming that not addressing the edge case is not a blocker.


const handleEnable = useCallback( () => {
if ( ! acceptedCustomerDataTerms ) {
return;

Check warning on line 56 in js/src/components/enhanced-conversion-tracking-settings/cta.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/enhanced-conversion-tracking-settings/cta.js#L56

Added line #L56 was not covered by tests
}

updateEnhancedAdsConversionStatus(
ENHANCED_ADS_CONVERSION_STATUS.ENABLED
);

onEnableClick();
}, [
updateEnhancedAdsConversionStatus,
acceptedCustomerDataTerms,
onEnableClick,
] );

const handleOnAcceptTerms = () => {
setStartBackgroundPoll( true );
};

if ( startBackgroundPoll ) {
return <AppButton isSecondary disabled loading />;
}

if ( ! acceptedCustomerDataTerms ) {
return <AcceptTerms onAcceptTerms={ handleOnAcceptTerms } />;
}

if ( allowEnhancedConversions === ENHANCED_ADS_CONVERSION_STATUS.ENABLED ) {
return (
<AppButton isPrimary isDestructive onClick={ handleDisable }>
{ disableLabel }
</AppButton>
);
}

// User has accepted TOS or tracking is disabled.
return (
<AppButton isPrimary onClick={ handleEnable }>
{ enableLabel }
</AppButton>
);
};

export default CTA;
221 changes: 221 additions & 0 deletions js/src/components/enhanced-conversion-tracking-settings/cta.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,221 @@
jest.mock( '@woocommerce/components', () => ( {
...jest.requireActual( '@woocommerce/components' ),
Spinner: jest
.fn( () => <div role="status" aria-label="spinner" /> )
.mockName( 'Spinner' ),
} ) );

jest.mock( '.~/hooks/useAcceptedCustomerDataTerms', () => ( {
__esModule: true,
default: jest.fn().mockName( 'useAcceptedCustomerDataTerms' ),
} ) );

jest.mock( '.~/hooks/useAllowEnhancedConversions', () => ( {
__esModule: true,
default: jest.fn().mockName( 'useAllowEnhancedConversions' ),
} ) );

jest.mock( '.~/hooks/useAutoCheckEnhancedConversionTOS', () => ( {
__esModule: true,
default: jest
.fn()
.mockName( 'useAutoCheckEnhancedConversionTOS' )
.mockImplementation( () => {
return {
startEnhancedConversionTOSPolling: jest.fn(),
stopEnhancedConversionTOSPolling: jest.fn(),
};
} ),
} ) );

jest.mock( '.~/data/actions', () => ( {
...jest.requireActual( '.~/data/actions' ),
updateEnhancedAdsConversionStatus: jest
.fn()
.mockName( 'updateEnhancedAdsConversionStatus' )
.mockImplementation( () => {
return { type: 'test', response: 'enabled' };
} ),
} ) );

/**
* External dependencies
*/
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import '@testing-library/jest-dom';

/**
* Internal dependencies
*/
import useAcceptedCustomerDataTerms from '.~/hooks/useAcceptedCustomerDataTerms';
import useAllowEnhancedConversions from '.~/hooks/useAllowEnhancedConversions';
import { ENHANCED_ADS_CONVERSION_STATUS } from '.~/constants';
import CTA from './cta';

describe( 'Enhanced Conversion CTA', () => {
beforeEach( () => {
jest.clearAllMocks();
} );

test( 'Prompt the user to sign the TOS', () => {
useAcceptedCustomerDataTerms.mockReturnValue( {
acceptedCustomerDataTerms: false,
hasFinishedResolution: true,
} );

useAllowEnhancedConversions.mockReturnValue( {
allowEnhancedConversions: null,
} );

render( <CTA /> );
expect(
screen.getByText( 'Accept Terms & Conditions' )
).toBeInTheDocument();
} );

test( 'Prompt the user to enable enhanced conversion tracking if the TOS has been accepted', () => {
useAcceptedCustomerDataTerms.mockReturnValue( {
acceptedCustomerDataTerms: true,
hasFinishedResolution: true,
} );

useAllowEnhancedConversions.mockReturnValue( {
allowEnhancedConversions: ENHANCED_ADS_CONVERSION_STATUS.DISABLED,
} );

render( <CTA enableLabel="Confirm" /> );
expect( screen.getByText( 'Confirm' ) ).toBeInTheDocument();
} );

test( 'Prompt the user to disable enhanced conversion tracking if enabled', () => {
useAcceptedCustomerDataTerms.mockReturnValue( {
acceptedCustomerDataTerms: true,
hasFinishedResolution: true,
} );

useAllowEnhancedConversions.mockReturnValue( {
allowEnhancedConversions: ENHANCED_ADS_CONVERSION_STATUS.ENABLED,
} );

render( <CTA disableLabel="Disable tracking" /> );
expect( screen.getByText( 'Disable tracking' ) ).toBeInTheDocument();
} );

test( 'Click on accept TOS button callback', () => {
window.open = jest.fn();
Copy link
Member

Choose a reason for hiding this comment

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

Consider window is a global object across jest environment, it could be mocked by

const spy = jest.spyOn( global.window, 'open' ).mockImplementation();

expect( spy ).toHaveBeenCalledTimes( 1 );

// Restore it in the end of this test case.
spy.mockRestore();


useAcceptedCustomerDataTerms.mockReturnValue( {
acceptedCustomerDataTerms: false,
hasFinishedResolution: true,
} );

useAllowEnhancedConversions.mockReturnValue( {
allowEnhancedConversions: null,
} );

render( <CTA /> );

const button = screen.getByRole( 'button' );
userEvent.click( button );

expect( window.open ).toHaveBeenCalledTimes( 1 );
} );

test( 'Click on enable/confirm button callback', () => {
const handleOnEnable = jest.fn().mockName( 'On Enable click' );

useAcceptedCustomerDataTerms.mockReturnValue( {
acceptedCustomerDataTerms: true,
hasFinishedResolution: true,
} );

useAllowEnhancedConversions.mockReturnValue( {
allowEnhancedConversions: null,
} );

render( <CTA onEnableClick={ handleOnEnable } /> );

const button = screen.getByRole( 'button' );
userEvent.click( button );

expect( handleOnEnable ).toHaveBeenCalledTimes( 1 );
} );

test( 'Confirm/enable button callback should not be called if TOS has not been accepted', () => {
const handleOnEnable = jest.fn().mockName( 'On Enable click' );

useAcceptedCustomerDataTerms.mockReturnValue( {
acceptedCustomerDataTerms: false,
hasFinishedResolution: true,
} );

useAllowEnhancedConversions.mockReturnValue( {
allowEnhancedConversions: ENHANCED_ADS_CONVERSION_STATUS.ENABLED,
} );

render( <CTA onEnableClick={ handleOnEnable } /> );

const button = screen.getByRole( 'button' );
userEvent.click( button );

expect( handleOnEnable ).not.toHaveBeenCalled();
} );

test( 'Click on disable button callback', () => {
const handleOnDisable = jest.fn().mockName( 'On Disable click' );

useAcceptedCustomerDataTerms.mockReturnValue( {
acceptedCustomerDataTerms: true,
hasFinishedResolution: true,
} );

useAllowEnhancedConversions.mockReturnValue( {
allowEnhancedConversions: ENHANCED_ADS_CONVERSION_STATUS.ENABLED,
} );

render( <CTA onDisableClick={ handleOnDisable } /> );

const button = screen.getByRole( 'button' );
userEvent.click( button );

expect( handleOnDisable ).toHaveBeenCalledTimes( 1 );
} );

test( 'Disable button callback should not be called if TOS has not been accepted', () => {
const handleOnDisable = jest.fn().mockName( 'On Disable click' );

useAcceptedCustomerDataTerms.mockReturnValue( {
acceptedCustomerDataTerms: false,
hasFinishedResolution: true,
} );

useAllowEnhancedConversions.mockReturnValue( {
allowEnhancedConversions: ENHANCED_ADS_CONVERSION_STATUS.ENABLED,
} );

render( <CTA onDisableClick={ handleOnDisable } /> );

const button = screen.getByRole( 'button' );
userEvent.click( button );

expect( handleOnDisable ).not.toHaveBeenCalled();
} );

test( 'Should render the enable button if TOS has been accepted and the status is not enabled', () => {
useAcceptedCustomerDataTerms.mockReturnValue( {
acceptedCustomerDataTerms: true,
hasFinishedResolution: true,
} );

useAllowEnhancedConversions.mockReturnValue( {
allowEnhancedConversions: null,
} );

render( <CTA /> );

expect(
screen.getByRole( 'button', { name: 'Enable' } )
).toBeInTheDocument();
} );
} );
Loading
Loading