site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Update Consent Mode once the Ads Conversion ID setting has been migrated to the Ads module.

Open techanvil opened this issue 1 year ago • 2 comments

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

  • Once the Ads Conversion ID has been migrated to the Ads Module:
    • All references to it in the Consent Mode feature should be updated to use the value from the Ads Module.
    • The confirmation dialog shown when disabling Consent Mode on the Settings screen should be updated to include the Ads module when it's connected.

Implementation Brief

An initial IB has been drafted, to be completed.

  • [ ] Ensure the following code snippets are updated to get the adsConversionID from the ads module:

https://github.com/google/site-kit-wp/blob/26e6a4318b09e47796fa5c02adf263ab756cb383/assets/js/components/consent-mode/ConsentModeSetupCTAWidget.js#L72-L75

https://github.com/google/site-kit-wp/blob/26e6a4318b09e47796fa5c02adf263ab756cb383/assets/js/components/settings/SettingsCardConsentMode.js#L50-L53

https://github.com/google/site-kit-wp/blob/26e6a4318b09e47796fa5c02adf263ab756cb383/assets/js/components/consent-mode/ConfirmDisableConsentModeDialog.js#L45-L47

Test Coverage

  • Update the assets/js/components/consent-mode/ConsentModeSetupCTAWidget.stories.js story to use the ads store.

QA Brief

Changelog entry

techanvil avatar Mar 08 '24 14:03 techanvil

I have reviewed the current consent mode for any use of the adsConversionID and all cases are covered in the IB.

benbowler avatar Mar 18 '24 14:03 benbowler

IB ✔️

eugene-manuilov avatar Mar 18 '24 17:03 eugene-manuilov

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:

  1. Analytics module is not connected, but Ads module is connected with an Ads conversion tracking ID.
  2. Ads module is not connected, but Analytics module is connected and:
    1. It is linked with Google Ads (adsLinked) and/or
    2. The connected Google Tag has an Ads conversion ID as one of its destinations.

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?

nfmohit avatar Apr 16 '24 10:04 nfmohit

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.

techanvil avatar Apr 16 '24 10:04 techanvil

Apologies, I hadn't spotted @zutigrm is off today. @nfmohit has kindly offered to raise a PR to address this.

techanvil avatar Apr 16 '24 11:04 techanvil

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.

nfmohit avatar Apr 16 '24 14:04 nfmohit

Top stuff, thanks @nfmohit. The PR is now merged, back to you @mohitwp to continue the QA for this one.

techanvil avatar Apr 16 '24 15:04 techanvil

QA Update ⚠️

  • Tested on main environment.
  • Hey @nfmohit , according to the QAB Consent mode, the banner only appears if the Ads module is connected, regardless of whether analytics is connected or not. I just want to confirm if this information is correct, especially if the QAB hasn't been updated after recent changes. Could you please review and confirm?

If only Analytics is connected

image

image

Both Analytics and Ads Module connected-

image

Only Ads module is connected -

image

Ads Module connected with Analytics

image

Ads Module connected without Analytics

image

Recommended badge -

image

Recommended badge not showing if analytics is not connected-

image

mohitwp avatar Apr 17 '24 07:04 mohitwp

  • Hey @nfmohit , according to the QAB Consent mode, the banner only appears if the Ads module is connected, regardless of whether analytics is connected or not. I just want to confirm if this information is correct, especially if the QAB hasn't been updated after recent changes. Could you please review and confirm?

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!

nfmohit avatar Apr 17 '24 07:04 nfmohit

QA Update ✅

  • Tested on main environment.
  • Verified according to the QAB Consent mode, the banner only appears if the Ads module is connected, regardless of whether analytics is connected or not.
  • Verified Recommended badge is appearing only if Ads module is connected.
  • Verified Copy under disconnect modal is getting update as per which module is active.

If only Analytics is connected

image

image

Both Analytics and Ads Module connected-

image

Only Ads module is connected -

image

Ads Module connected with Analytics

image

Ads Module connected without Analytics

image

Recommended badge -

image

Recommended badge not showing if analytics is not connected-

image

mohitwp avatar Apr 17 '24 08:04 mohitwp