-
Notifications
You must be signed in to change notification settings - Fork 286
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
Update Consent Mode once the Ads Conversion ID setting has been migrated to the Ads module. #8365
Comments
I have reviewed the current consent mode for any use of the |
IB ✔️ |
Hi @zutigrm & @techanvil. It looks like this change in the PR has stopped the consent mode banner from appearing if the Ads module is not connected. The above change implies that the consent mode banner should only show up if both the Analytics & Ads modules are connected. However, it is possible for Google Ads to be connected even without one of the modules disconnected. Examples:
However, with the above linked change, the banner will only appear if both Ads and Analytics modules are connected, which I think is not right. Thoughts? |
Thanks @nfmohit, great spot. That one totally slipped by me. @zutigrm, please can you file a followup PR to update the condition that Nahid's highlighted? It should be returning false if neither of the modules are connected, e.g. along these lines: if (
! (
isModuleConnected( 'analytics-4' ) || isModuleConnected( 'ads' )
)
) {
return false;
} With this being a release issue we should address this as a priority. Thanks! @mohitwp, I've moved this back from QA to Execution and assigned it back to Aleksej to address the above. |
Thank you @techanvil . This is in CR now with a follow-up PR. |
Fix consent mode Ads connection status logic
QA Update
|
Hi @mohitwp. That is correct. If the Ads module is connected, we don't need the Analytics module to also be connected in this context. Thanks! |
QA Update ✅
|
Feature Description
Ensure that the relevant parts of Consent Mode are updated once the Ads Conversion ID setting has been migrated to the Ads module.
Some of this will almost certainly be updated as part of the Ads Module work, this issue is to ensure nothing is missed.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Implementation Brief
An initial IB has been drafted, to be completed.
adsConversionID
from theads
module:site-kit-wp/assets/js/components/consent-mode/ConsentModeSetupCTAWidget.js
Lines 72 to 75 in 26e6a43
site-kit-wp/assets/js/components/settings/SettingsCardConsentMode.js
Lines 50 to 53 in 26e6a43
site-kit-wp/assets/js/components/consent-mode/ConfirmDisableConsentModeDialog.js
Lines 45 to 47 in 26e6a43
Test Coverage
assets/js/components/consent-mode/ConsentModeSetupCTAWidget.stories.js
story to use the ads store.QA Brief
adsModule
feature flag, and connect Analytics and Ads modulesConsent Mode
. Then try to disable it, the popup should mention Ads module in the Note section, together with Analytics"Note: these active modules depend on consent mode and will be affected: Analytics and Ads"
Ads
module, and repeat the step above, note section should mention onlyAnalytics
as affected module (Ads should be in the list only when active)recommended
badge is showing in admin settings when Ads module is connectedChangelog entry
The text was updated successfully, but these errors were encountered: