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

Create `Web_Tag` class for `Ads` module

Open 10upsimon opened this issue 1 year ago • 6 comments

Feature Description

As part of the new Ads module, comes the need to render an Ads gtag (either as a main gtag or inline config) if the user is making use of the Ads Conversion ID field within the module.

At present, Ads Conversion ID logic is container within the GA4 Web_Tag module. See the code at https://github.com/google/site-kit-wp/blob/develop/includes/Modules/Analytics_4/Web_Tag.php

The Ads module now requires it's own Web_Tag class in order to render the necessary gtag, or add the necessary inline config to an already enqueued gtag.

See the applicable area of the Design Doc here: https://docs.google.com/document/d/1APuSv95bf62uhzlaFlW6jPrzPKy1avpRYd9W1MSAAJo/edit?resourcekey=0-UuynlcUz9CoubgldR6Z5sg#heading=h.miy2mfwgn6h8


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

Acceptance criteria

  • A new Web_Tag class should be created within the Google\Site_Kit\Modules\Ads; namespace
  • This Web_Tag class should utilise the Ads Conversion ID field to either:
    • Render a main gtag script referencing the applicable tag ID from the above field, or
    • Add to any existing queued gtag script
  • Utilise the new decoupled GTag infrastructure to achieve the above
    • See #8269
  • Ensure that necessary inline comments are retained in order for Site Health checks to continue to function correctly

Implementation Brief

  • [ ] Add includes/Modules/Ads/Web_Tag.php
    • [ ] Extend Module_Web_Tag
    • [ ] Add register method
      • Hook into googlesitekit_setup_gtag action, and invoke callback method, say setup_gtag
    • [ ] Add setup_gtag method
      • It should accept one parameter $gtag
      • If $this->tag_id is not empty, invoke add_tag method of $gtag, and pass tag_id class property as only parameter. Example $gtag->add_tag( $this->tag_id )
      • Hook into script_loader_tag filter, and if handle is GTag::HANDLE include snippet comment Google Ads snippet added by Site Kit before the tag. You can see an example here https://github.com/google/site-kit-wp/blob/c593141ba439b1713eb4ce8304f74052603220a0/includes/Modules/Analytics_4/Web_Tag.php#L143-L154
  • [ ] Update Google\Site_Kit\Modules\Ads class
    • [ ] Edit register method
      • Hook into template_redirect action and invoke callback method, say register_tag
    • [ ] Add register_tag method
      • Get the module settings, instantiate Web_Tag class, and pass it the adsConversionID setting.
      • If tag is not blocked (is_tag_blocked) include Tag_Environment_Type_Guard in use_guard method. And if tag can_register, invoke register method on it.
      • You can see an example here https://github.com/google/site-kit-wp/blob/d8a3974d224de98d06316e3f8a876f090733851a/includes/Modules/Tag_Manager.php#L554-L572

Test Coverage

  • No tests are needed

QA Brief

  • Set up Site Kit.
  • Enable the adsModule feature flag, and connect the Ads module with a conversion tracking ID, e.g. AW-1234.
  • In the site front-end, view its source, and verify that it outputs the conversion tracking ID snippet, similar to the snippet below:
<!-- Google tag (gtag.js) snippet added by Site Kit -->

<!-- Google Ads snippet added by Site Kit -->
<script src="https://www.googletagmanager.com/gtag/js?id=AW-1234" id="google_gtagjs-js" async></script>
<script id="google_gtagjs-js-after">
window.dataLayer = window.dataLayer || [];function gtag(){dataLayer.push(arguments);}
gtag("js", new Date());
gtag("set", "developer_id.dZTNiMT", true);
gtag("config", "AW-1234");
</script>

<!-- End Google tag (gtag.js) snippet added by Site Kit -->
  • Now, also connect the Analytics module and choose to output GA snippet.
  • View the front-end page source again and verify that both the Analytics and Ads snippet show at the same time, similar to the following:
<!-- Google tag (gtag.js) snippet added by Site Kit -->

<!-- Google Ads snippet added by Site Kit -->

<!-- Google Analytics snippet added by Site Kit -->
<script src="https://www.googletagmanager.com/gtag/js?id=GT-ABCD123" id="google_gtagjs-js" async></script>
<script id="google_gtagjs-js-after">
window.dataLayer = window.dataLayer || [];function gtag(){dataLayer.push(arguments);}
gtag("set","linker",{"domains":["sitekit.10uplabs.com"]});
gtag("js", new Date());
gtag("set", "developer_id.dZTNiMT", true);
gtag("config", "GT-ABCD123");
gtag("config", "AW-1234");
</script>

<!-- End Google tag (gtag.js) snippet added by Site Kit -->

Changelog entry

  • Add the web tag for the Ads module.

10upsimon avatar Feb 26 '24 09:02 10upsimon

ACs 👍🏻

tofumatt avatar Feb 26 '24 13:02 tofumatt

IB ✔️

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

Update: This is technically blocked by #8273 as using the GTag infrastructure for the Ads module creates duplicates alongside Analytics that still doesn't use the GTag infrastructure. The PR for this issue is otherwise ready.

image

nfmohit avatar Mar 27 '24 03:03 nfmohit

The PR aiming to address the ACs for this issue is ready for CR. However, I have one concern. While this issue implements an independent ability for the Ads module to output the Ads conversion tracking ID, it doesn't prevent the Analytics module from still outputting this tag if still set.

I understand that we have #8248 that is supposed to migrate the Ads conversion tracking ID from the Analytics to the Ads module thus effectively removing it from the Analytics module, however, keep in mind that this migration is conditional. This migration will only happen if the user goes to Site Kit Settings and opens the Analytics settings edit/view screens. See this AC from #8248:

For users who were previously using the value of the GA4 Ads Conversion ID label:

  • Upon visiting the legacy settings field for the first time following plugin update, the value should be migrated to the new location applicable to the settings defined in the Ads module

Let's take the following scenario into consideration:

  1. A pre-existing Site Kit installation has the Analytics module set up with an Ads conversion tracking ID entered.
  2. Now, the new "Ads" module is launched.
  3. The user does not feel any necessity to open the Analytics settings edit/view screens, as a result, the migration of the existing Ads conversion tracking ID does not happen from the Analytics to the Ads module.
  4. They see the new "Ads" module and set it up with the same or different Ads conversion tracking ID.

The above steps will make Site Kit output two tags for the Ads conversion tracking ID, one from the Analytics module, and the other from the Ads module. Here's a screenshot of an experiment I did to replicate the above scenario: image

I think the above scenario can potentially cause duplicate/erroneous tracking, and should be addressed. I haven't been able to locate an issue that addresses this.

CC: @10upsimon @tofumatt @zutigrm @aaemnnosttv @bethanylang

nfmohit avatar Mar 29 '24 19:03 nfmohit

@nfmohit could you please create a new ticket from your comment? We will address it there.

eugene-manuilov avatar Mar 29 '24 22:03 eugene-manuilov

QA Update: ❌

@nfmohit I started to work through testing this ticket and noticed that when I entered a Conversion Tracking ID, e.g., AW-1234 and click on the Complete Setup CTA, an error appears on the screen Error: You are probably offline. I also see a console error: POST https://resolvepies.s4-tastewp.com/wp-json/google-site-kit/v1/modules/ads/data/settings?_locale=user net::ERR_BLOCKED_BY_CLIENT googlesitekit-api-48…1a630b81cb15ca.js:3 Google Site Kit API Error method:POST datapoint:settings type:modules identifier:ads error:"You are probably offline." I wasn't offline.

image

Please note that Analytics was not set up. When Analytics was set up then I am able to set up the module.

wpdarren avatar Mar 31 '24 11:03 wpdarren

QA Update: ✅

Verified:

  • As per QAB, the two snippets are in the source code when Analytics is connected and disconnected.
  • Tested on a post with Key Metrics, to ensure the custom dimensions are included in the Analytics code.

Note: the issue above is not re-creatable after refreshing develop this morning, but I plan to do some more testing with a new site. This issue is outside the scope of this ticket anyway, so it will be addressed separately.

Screenshots

image image image

wpdarren avatar Apr 01 '24 11:04 wpdarren

@wpdarren the net::ERR_BLOCKED_BY_CLIENT error is from the browser and means your browser (the client) blocked the request. This could be from an adblocker or something like that which is interfering with the request. This could be something that may interfere with the function of the module which we should test more but shouldn't be specific to this issue.

aaemnnosttv avatar Apr 01 '24 15:04 aaemnnosttv

@nfmohit could you please create a new ticket from your comment? We will address it there.

I've opened #8455 to address this. Thank you!

nfmohit avatar Apr 01 '24 19:04 nfmohit

@wpdarren, the net::ERR_BLOCKED_BY_CLIENT error is from the browser and means your browser (the client) blocked the request. This could be from an adblocker or something similar that is interfering with the request. It could also be something that may interfere with the module's function, which we should test more but shouldn't be specific to this issue.

@aaemnnosttv Ahh, yes, that would make sense. I have an AdBlocker enabled, so I will do some additional testing around this later today and report back.

wpdarren avatar Apr 02 '24 10:04 wpdarren

This is just a note, @aaemnnosttv, that the issue was with the Adblock Chrome extension. When I disabled it, I could set up the Ads module without issues. I created this ticket for it here so we can fix it for launch.

wpdarren avatar Apr 03 '24 08:04 wpdarren