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

Extend Consent Mode conditions for determining Ads connection status to include a check for an Ads tag as a destination of a Google Tag

Open techanvil opened this issue 1 year ago • 5 comments

Feature Description

For the Consent Mode feature, we have so far implemented the check for Ads being connected to look for the presence of the Ads Conversion ID or an Ads link.

We should extend these conditions to include the additional check discussed under "when does this apply?" in the one-pager - checking for the presence of Ads tag as a destination of the connected Google Tag.

Note that we are now able to use the Tag Manager API to look up a container by tag ID, so this is now technically feasible. See the tagId parameter for the containers/lookup endpoint and the feature request where it was introduced.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • Within the Consent Mode feature, the conditions for determining whether Ads is connected should include a check for the presence of an Ads tag as a destination of the connected Google Tag.
    • The Google Tag for the connected Analytics property should be checked.
    • When Tag Manager is connected the tags for the connected container should be checked.
    • Note that the case where a variable is used for the tag configuration will be handled in a separate issue: #9556
  • In terms of user-visible behaviour:
    • The Setup CTA Banner should be displayed when an Ads tag is detected (while the Consent Mode enabled setting is false and the banner has not been dismissed).
    • The "Recommended" badge and the notice reading "If you have Google Ads campaigns for this site, it’s highly recommended to enable Consent mode..." on the Consent Mode section of the Settings screen should be displayed when an Ads tag is detected.
    • The Ads users' variant of the confirmation dialog shown when toggling the Consent Mode switch off on the Settings screen should be displayed when an Ads tag is detected.

Implementation Brief

Consent Mode backend updates

  • [ ] Update includes/Core/Consent_Mode/REST_Consent_Mode_Controller.php, creating a new REST endpoint recommend-consent-mode, in this endpoint:
    • [ ] First check if the Ads module is connected using the is_connected method, if this is true, return true on the endpoint immediately.
    • [ ] Next check if Analytics is connected using the is_connected method and if Ads is linked to Analytics by checking the adSenseLinked setting, return true on the endpoint immediately.
    • [ ] Finally, check if Tag Manager is connected using the is_connected method, if so:
      • [ ] Use $this->get_tagmanager_service()->accounts_containers_versions->live() to get the live version of the configured GTM tag using the accountID and internalContainerID stored in the module settings: https://github.com/google/site-kit-wp/blob/165c8bb73003e7861b3dc2eead677d034def431e/includes/Modules/Tag_Manager.php#L328-L330
      • [ ] Use array_search to check if any of the destinations in the tag[] key of the response returned for this version are of the type awct, which is an "Google Ads Conversion Tracking" tag, return true if so.
      • [ ] Use array_filter to find destinations in the tag[] key of the response where the type is googtag.
        • [ ] Get the tagId for this tag by filtering the parameters key for this tag, where key is tagId. This should give a tagId in the form G-XXXXXXX.
        • [ ] Loop through each of these tags, using the new Tag Manager container lookup API, calling $tagmanagerService->accounts_containers->lookup( array( 'tag_id' => $tag_id ) ).
        • [ ] Use the Tag Manager destinations API, passing the accountId and containerId returned above like so: $tagmanagerService->accounts_containers_destinations->get("accounts/{account_id}/containers/{container_id}/destinations", array( 'destinationId' => 'G-XXXXXX') ) to get a list of destination tags for this container.
        • [ ] Loop through the returned destinations, check if any of the returned tags destinationIds start with the chars AW-, return true if so.
    • [ ] If none of the above conditions are met, return false.

Datastore changes

  • [ ] Update assets/js/googlesitekit/datastore/site/consent-mode.js, create a new fetch store called getRecommendConsentMode to retrieve the values from the new REST endpoint above, like so: https://github.com/google/site-kit-wp/blob/165c8bb73003e7861b3dc2eead677d034def431e/assets/js/googlesitekit/datastore/site/consent-mode.js#L80-L90

Setup CTA Updates

  • [ ] Update the render condition for assets/js/components/consent-mode/ConsentModeSetupCTAWidget.js to use the new getRecommendConsentMode selector to render the CTA instead of the isAdsConnected selector: https://github.com/google/site-kit-wp/blob/165c8bb73003e7861b3dc2eead677d034def431e/assets/js/components/consent-mode/ConsentModeSetupCTAWidget.js#L122-L127

Recommended Badge

  • [ ] Update assets/js/components/settings/SettingsCardConsentMode.js to use the new getRecommendConsentMode selector, instead of the isAdsConnected selector to render the Recommended badge: https://github.com/google/site-kit-wp/blob/165c8bb73003e7861b3dc2eead677d034def431e/assets/js/components/settings/SettingsCardConsentMode.js#L111-L116

Test Coverage

  • Create thorough tests for the new endpoint in tests/phpunit/integration/Core/Consent_Mode/REST_Consent_Mode_ControllerTest.php.

QA Brief

Changelog entry

techanvil avatar Aug 06 '24 16:08 techanvil

AC ✔️

eugene-manuilov avatar Sep 20 '24 20:09 eugene-manuilov

Hey @techanvil, I had a deep dive into the Tag Manager docs and API and it appears I can do what is required here using existing APIs. Did I miss a way that GTM can be configured where a tags/containers can be nested in a way that requires us to look up containers to check for Ads linking? If so can you share an example of how to set up tag manager to reach this case and I will update the IB.

benbowler avatar Sep 25 '24 13:09 benbowler

Hey @benbowler, thanks for looking into this. The trickier case that you'll need to update the IB for is when the GTM container points to a Google tag, which in turn points to an Ads tag as one of its destinations.

As a prerequisite you'll need a Google tag which points to an Ads tag. You can configure this via the Ads UI:

image

Or by editing a tag via the entry point on https://tagmanager.google.com/#/home#tags, updating it to point to an Ads tag as a destination:

image

Then, when creating your container in Tag Manager, set up a tag using the Google Tag type:

image

Specify the Google tag ID (it may have multiple IDs, you can choose any of them) in the Tag Configuration (or for a more advanced use case you can specify this indirectly via a variable):

image

When you retrieve the live container version from the API you'll see the tag entry has the type googtag.

image

You'll need to use the tagId parameter to containers/lookup as mentioned in the Feature Description to retrieve the container for the Google tag, noting its account ID and container ID. Note that you can't rely on the list of tagIds here to determine whether the tag points to an Ads tag, as these will not necessarily map to destination IDs.

image

Then, use the destinations endpoint to retrieve the list of destinations for the Google tag. These can be examined to determine whether there's an Ads tag as a destination by checking for a destinationId with the prefix AW-:

image

It's not the most straightforward one and I hope that helps, please drop me a line if you have any further questions!

techanvil avatar Oct 03 '24 16:10 techanvil

Hi @benbowler, and apologies, while reviewing the updated IB here I realised that I'd missed a point from the AC, which I've now added. The IB will need an update to accommodate this change:

  • The Ads users' variant of the confirmation dialog shown when toggling the Consent Mode switch off on the Settings screen should be displayed when an Ads tag is detected.

Additionally:

  • It looks like the intention of the specced recommend-consent-mode endpoint is to provide the same functionality as the existing isAdsConnected() selector, augmented to check the connected Tag Manager tag when present. However, as specced the endpoint is missing the check for an Ads tag as a destination of the connected Analytics property. Assuming we continue with this endpoint, this logic would need to be included.
  • I'm not averse to creating a new endpoint to encapsulate all the logic on the backend. We could alternatively take the approach of adding an endpoint just for the Tag Manager checks, and add a check for this to the existing isAdsConnected() selector.
  • If we do fully migrate the logic to the new endpoint maybe the endpoint (and corresponding datastore functions) should be called is-ads-connected, ads-connection-status, or something along those lines, as we might want to use the same check outside of the context of recommending Consent Mode.
  • All of the existing use of the isAdsConnected value should be replaced by the new version. I'd suggest we simply update the current selector rather than introduce a new one.
  • We are adding potentially quite a few additional Tag Manager API calls to every dashboard page load. To mitigate this we should update the useSelect() in ConsentModeSetupCTAWidget that retrieves the isAdsConnected value (or recommendConsentMode if we still go that route) so that it only calls the selector if the other conditions that would prevent the CTA from displaying are all true:

https://github.com/google/site-kit-wp/blob/53da5b5f2855089e4c74fdde73c020aaa344750e/assets/js/components/consent-mode/ConsentModeSetupCTAWidget.js#L77-L79

https://github.com/google/site-kit-wp/blob/53da5b5f2855089e4c74fdde73c020aaa344750e/assets/js/components/consent-mode/ConsentModeSetupCTAWidget.js#L122-L127

  • We might want to iterate on this in a future issue to cache the value and only update it say once per hour.
  • There is a less common case we should handle where the tag ID is specified via a variable in the Tag Manager container configuration, which I alluded to this in my comment above. We can tackle this in a separate issue - I've created an issue for it and added a mention of this to the AC as well to make it clear.

techanvil avatar Oct 24 '24 15:10 techanvil

Thanks @techanvil, I'll review and update the IB.

benbowler avatar Oct 24 '24 15:10 benbowler

Thanks @benbowler! It's looking good. A few last points:

  • On reflection I think ads-measurement-status would be preferable for a REST endpoint name as it's more indicative of a "resource", we can return an object with a connected boolean property. This would be along similar lines to the existing tracking endpoint. The name is also a nod to the CoMo one-pager, see "when does this apply?".
  • We should check the connected Analytics property before checking Tag Manager as this can allow us to return early without any additional API requests.
  • We still need a simple isAdsConnected selector to go with the resolver.

In order to help move this along I've updated the IB accordingly myself. Please take a look at my changes, if you're happy with them feel free to move this to the EB.

techanvil avatar Oct 30 '24 16:10 techanvil

Thanks Tom, I have reviewed this and moving it to EB.

benbowler avatar Oct 31 '24 08:10 benbowler

The current apiclient library we are using includes google/apiclient-services v0.355.0 which does not have updated Tag manager API, so 'lookup' => ['path' => 'tagmanager/v2/accounts/containers:lookup', 'httpMethod' => 'GET', 'parameters' => ['destinationId' => ['location' => 'query', 'type' => 'string']]] definition from our current package. And lookup from this IB point:

Loop through each of these tags, using the new Tag Manager container lookup API, calling $tagmanagerService->accounts_containers->lookup( array( 'tag_id' => $tag_id ) )

Does not work, as tag ID is not supported as parameter in the package we have, hence throwing fatal error.

Updating dependencies under google/apiclient is at that limit of services version, and we cannot update client version due to the minimum php version constrains (any version over current one requires php 8).

Some of the options we have:

  • Apply all logic up to this point - meaning all the checks switched to the backend, except for additional lookup inside the googtag Tag, and file a follow up to extend that part once we switch to the generated service dependencies (@aaemnnosttv is experimenting on this) instead of require from external source
  • Move issue back to backlog until it is unblocked, switching to generated services dependencies, etc, and finish everything afterwards

WDYT? @techanvil @aaemnnosttv

zutigrm avatar Nov 20 '24 17:11 zutigrm

Hey @zutigrm! Thanks for the update, and sorry that we missed the lack of support for the tag_id parameter during definition.

I think your first suggestion makes the most sense - do as much as we can now, and file a followup issue to update the service library and implement the remaining requirements. It will still be an incremental improvement that would be better to deliver sooner rather than later, plus it will result in two smaller issues/PRs rather than one larger one, so it will be easier to review.

techanvil avatar Nov 20 '24 18:11 techanvil

@zutigrm did you create the follow up issue you mentioned, otherwise I can create it so it gets tracked.

benbowler avatar Nov 26 '24 06:11 benbowler

Added a follow up issue

zutigrm avatar Nov 26 '24 23:11 zutigrm

QA Update: ✅

Verified:

  • I setup Site Kit, and on each setup test following scenarios:

    • Connect Ads module - it returns true
    • Connect Analytics module that is connected with Ads - it returns true
    • Connect Tag manager module, and connect tag with Ads, - it returns true
  • Having no Analytics, or Ads, or Tag manager active - it returns false

  • Having Analytics module active, but it is not connected with Ads, and Ads module is not active - it returns false

  • Having Tag Manger active with Tags that have no containers pointing to the Ads - it returns false

  • Consent mode notifications shows as before in settings and in dashboard as setup CTA

Screenshots

Image Image Image Image Image Image Image Image

wpdarren avatar Dec 11 '24 18:12 wpdarren