R2 – Enable Enhanced Conversions #2242
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2242 +/- ##
===========================================
- Coverage 63.8% 62.6% -1.2%
- Complexity 4205 4288 +83
===========================================
Files 455 770 +315
Lines 17878 22116 +4238
Branches 0 563 +563
===========================================
+ Hits 11412 13853 +2441
- Misses 6466 7796 +1330
- Partials 0 467 +467
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…woocommerce/google-listings-and-ads into feature/2239-enhanced-conversions-panel
…hub.com/woocommerce/google-listings-and-ads into feature/2239-enhanced-conversions-panel
js/src/components/enhanced-conversion-tracking-settings/index.js
Outdated
Show resolved
Hide resolved
js/src/components/enhanced-conversion-tracking-settings/index.js
Outdated
Show resolved
Hide resolved
js/src/product-feed/submission-success-guide/EnhancedConversion.js
Outdated
Show resolved
Hide resolved
js/src/components/enhanced-conversion-tracking-settings/cta/PendingStatus.js
Outdated
Show resolved
Hide resolved
…woocommerce/google-listings-and-ads into feature/2239-enhanced-conversions-panel
…woocommerce/google-listings-and-ads into feature/2239-enhanced-conversions-panel
…enhanced-conversion (R2.1f) Google tag enhanced conversion
joemcgill
changed the title
This adds another side effect to get the campaigns data needed to show the correct 2nd page and removes several instances of loading UI when a selector is resolving. This also adds a new side effect to the CTA when accepting terms that will turn off polling once we've determined that the user has accepted the terms, so the Enable button will be shown.
|
@eason9487 and @mikkamp, we've finished processing all of the feedback from earlier rounds and have merged in the approved tagging PR from #2258 so that this should now represent the whole feature set for #2216. I've updated the description for this PR to reflect the latest state. The only known issues still to address:
I'd like to get the whole feature reviewed so we can address feedback while waiting for #2205 to be merged, at which point, I would update this branch so that we can test both features as an integrated set of changes before this PR is merged as well. Let me know if you have any questions or concerns. |
There was a problem hiding this comment.
I started going through this PR, but I just came across points from my previous review that weren't addressed. It still has the possibility of breaking with a fatal error.
So I'm not sure what I should be reviewing in regards to the PHP parts. Unless there are some commits missing that are in another PR.
…hub.com/woocommerce/google-listings-and-ads into feature/2239-enhanced-conversions-panel
|
@mikkamp Both of the points mentioned have been addressed. Sorry for inconvenience as earlier the fix was added as part of another PR which got closed.
CC: @joemcgill |
|
@eason9487 while waiting for another review from you, I wanted to clarify a couple of the changes we made since your last review, which affects two main issues you had previously raised:
We have removed the logic that checks for a connected Ads account prior to showing the second screen entirely here because connecting an Ads account is a requirement to complete the setup and trigger this modal in the first place, so a second screen will always be shown.
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. |
There was a problem hiding this comment.
@mikkamp Both of the points mentioned have been addressed.
For fatal error issue, a check has been added that $ads_id exists and if it doesn't, we are returning false
For strtolower, this has been removed and the unit test has been added in AccountsController to ensure that it returns error when unexpected case is passed in parameter value.
Thanks for the additional fixes I can confirm that the fatal error is no longer possible.
Just wanted to mentioned that the following comments also didn't get addressed:
Those aren't a big deal, as generally they shouldn't be called when disconnected. But if that's not respected a user could potentially end up with a setting from the previous connection when they are connecting a new account.
Since they aren't major issues I'll still go ahead and mark the PHP part of this PR as approved.
There was a problem hiding this comment.
I started a new round but after checking the issues in one of my previous reviews, I found the current implementation still doesn't fulfill this requirement.
Option 2a: User skips campaign creation during onboarding, but has an existing spending campaign
- (Modal 1/2) “You have successfully setup Google Listings & Ads”
- (Modal 2/2) Enhanced Conversions Setup Modal
Kapture.2024-03-27.at.21.47.25.mp4
The video shows the connected Google Ads account has a few spending Performance Max campaigns, but after skipping the campaign creation, the modal shows the Option 2b result.
Option 2b: User skips campaign creation during onboarding, and does not have an existing spending campaign (i.e. user has no campaigns, or user has non-GL&A campaigns that never spent)
- (Modal 1/2) “You have successfully setup Google Listings & Ads”
- (Modal 2/2) “Spend $500 get $500 Modal”
Therefore, I'm not pretty sure if this PR is ready for review? Or, is it possible to first double-confirm that all requirements have been satisfied?
| const { acceptedCustomerDataTerms } = useAcceptedCustomerDataTerms(); | ||
| const { allowEnhancedConversions } = useAllowEnhancedConversions(); | ||
|
|
||
| // @todo: Remove condition once R1 PRs are merged since there should always be a connected Ads account. |
There was a problem hiding this comment.
This TODO is false as Google Ads account can be disconnected from this plugin's Settings page.
| if ( ! acceptedCustomerDataTerms ) { | ||
| return <AcceptTerms />; | ||
| } |
There was a problem hiding this comment.
This component is unexpectedly rendered for a moment before data is fetched.
Kapture.2024-03-28.at.11.10.48.mp4
It would be easier to reproduce it by adding debugger here and observing it with browser DevTool.
if ( ! acceptedCustomerDataTerms ) {
debugger;
return <AcceptTerms />;
}| href={ url } | ||
| target="_blank" | ||
| type="external" | ||
| eventName="gla_ads_tos" // @todo: review eventName |
There was a problem hiding this comment.
Referring to #2270 (comment), please help to remove the eventName prop and leave the @todo.
| // @todo: review data-action label | ||
| data-action="view-product-feed" |
There was a problem hiding this comment.
Memo: Refer to #2242 (comment).
There was a problem hiding this comment.
Sounds good. We can change the data-action value to "close" and update the SubmissionSuccessGuide docs.
|
|
||
| const handleAcceptTerms = useCallback( | ||
| ( event ) => { | ||
| event.preventDefault(); |
There was a problem hiding this comment.
Wonder why it needs to call preventDefault?
| const PendingNotice = () => { | ||
| const { updateEnhancedAdsConversionStatus } = useAppDispatch(); | ||
| const { url } = useGoogleAdsEnhancedConversionTermsURL(); | ||
| useTermsPolling(); |
There was a problem hiding this comment.
It's confusing to use useTermsPolling and have its startBackgroundPoll be false because the terms data is not polled but only invalidates the getAcceptedCustomerDataTerms selector of data-store.
Kapture.2024-03-28.at.15.42.26.mp4
The video shows after accepting the terms and back to the plugin, the page stays at this notice and the accepted status will not be updated unless the user manually refreshes the page.
Is this the expected result that the user has to refresh the page to be able to see the toggle of enhanced ads conversion status?
If yes, this component doesn't need to use useTermsPolling() as the data polling is not used and the selector invalidation won't make any difference to this component.
| useEffect( () => { | ||
| if ( | ||
| allowEnhancedConversions === ENHANCED_ADS_CONVERSION_STATUS.PENDING | ||
| ) { | ||
| invalidateResolution( 'getAcceptedCustomerDataTerms', [] ); | ||
| } | ||
| }, [ allowEnhancedConversions, invalidateResolution ] ); |
There was a problem hiding this comment.
I would like to know when it will be necessary to do this processing?
| export const ENHANCED_CONVERSION_TERMS_BASE_URL = | ||
| 'https://ads.google.com/aw/conversions/customersettings'; | ||
|
|
||
| const useGoogleAdsEnhancedConversionTermsURL = () => { |
There was a problem hiding this comment.
For React custom hooks in this codebase, it would be better to name the file with the same name as the hook, and the same goes for imports:
| <CTA | ||
| onEnableClick={ handleEnableOrDisableClick } | ||
| onDisableClick={ handleEnableOrDisableClick } | ||
| acceptTermsLabel={ __( |
There was a problem hiding this comment.
Looks like this prop was missed to be forwarded from the CTA component to AcceptTerms component.
There was a problem hiding this comment.
I think this prop may be unnecessary an can likely be removed. Good catch.
| // Turn off polling when the user has accepted the terms. | ||
| useEffect( () => { | ||
| if ( acceptedCustomerDataTerms && startBackgroundPoll ) { | ||
| setStartBackgroundPoll( false ); | ||
| } | ||
| }, [ acceptedCustomerDataTerms, startBackgroundPoll ] ); |
There was a problem hiding this comment.
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:
- Move the required condition check from
useTermsPollingtouseAutoCheckEnhancedConversionTOS - Move the determinations of starting and stopping the polling to
useAutoCheckEnhancedConversionTOS - Return the
isPollingfromuseAutoCheckEnhancedConversionTOS - Call the
useAutoCheckEnhancedConversionTOShook inEnhancedConversionTrackingSettingsinstead ofAcceptTerms
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)
There was a problem hiding this comment.
Thanks for the suggestion and for confirming that not addressing the edge case is not a blocker.
|
Thanks for the review thus far, @eason9487. While you're still reviewing, I took a deeper look into the bug you reported here, where an account that already has PMax campaigns set up is connected and confirm the behavior. This seems to be related to an existing bug with the I don't know if there is any potential side effect to making sure that choosing "Skip this step for now" triggers the API call to the Curious to get your thoughts on this. |
There was a problem hiding this comment.
I've finished the last part of the third round code review for the frontend changes. Please also refer to the first part and the second part.
| } ); | ||
|
|
||
| test( 'Click on accept TOS button callback', () => { | ||
| window.open = jest.fn(); |
There was a problem hiding this comment.
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();| useGoogleAccount.mockReturnValue( { | ||
| hasFinishedResolution: true, | ||
| isResolving: false, | ||
| scope: { | ||
| adsRequired: true, | ||
| }, | ||
| google: true, | ||
| } ); |
There was a problem hiding this comment.
This mock could be omitted as the mocked useGoogleAdsAccount isolates it.
| key: 'user_data', | ||
| } ); | ||
|
|
||
| expect( dataConfig.allow_enhanced_conversions ).toBeTruthy(); |
There was a problem hiding this comment.
If it only expects a true, it would be a bit better to use .toBe( true ).
|
Hi @joemcgill
The TODO is one of the improvement ideas about letting the frontend side get the value in a different way, and I won't consider it as an existing bug because I don't recall an existing issue related to it. Ads setup was not initially part of the onboarding flow. When it was added to the onboarding flow in 2022, we didn't change its definition after overall consideration of the way it was jointed together, and "Skip this step for now" wasn't considered as completed ads setup then.
Yes, this is how this plugin was designed before this PR. If I remember it right, this is the first PR that needs to know if there is an existing paid campaign before the "Ads setup" is completed.
The definition of Given the impact scope, trying to change the logic of calling to
|
Changes proposed in this Pull Request:
Closes #2216
Replace this with a good description of your changes & reasoning.
Screenshots:
Detailed test instructions:
/wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fproduct-feed&guide=submission-successSubmissionSuccessGuidepanel to enable Enhanced Conversions #2239 for the different screens to be displayed within the modal and also the modal content.Additional details:
Changelog entry