Skip to content
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

Create Web_Tag class for Ads module #8313

Closed
7 tasks
10upsimon opened this issue Feb 26, 2024 · 11 comments
Closed
7 tasks

Create Web_Tag class for Ads module #8313

10upsimon opened this issue Feb 26, 2024 · 11 comments
Labels
Module: Ads Google Ads module related issues P0 High priority PHP Type: Feature New feature

Comments

@10upsimon
Copy link
Collaborator

10upsimon commented Feb 26, 2024

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
  • 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
        $filter_google_gtagjs = function ( $tag, $handle ) {
        if ( GTag::HANDLE !== $handle ) {
        return $tag;
        }
        // Retain this comment for detection of Site Kit placed tag.
        $snippet_comment = sprintf( "\n<!-- %s -->\n", esc_html__( 'Google Analytics snippet added by Site Kit', 'google-site-kit' ) );
        return $snippet_comment . $tag;
        };
        add_filter( 'script_loader_tag', $filter_google_gtagjs, 10, 2 );
  • 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
        public function register_tag() {
        $is_amp = $this->context->is_amp();
        $module_settings = $this->get_settings();
        $settings = $module_settings->get();
        $tag = $is_amp
        ? new AMP_Tag( $settings['ampContainerID'], self::MODULE_SLUG )
        : new Web_Tag( $settings['containerID'], self::MODULE_SLUG );
        if ( ! $tag->is_tag_blocked() ) {
        $tag->use_guard( new Tag_Verify_Guard( $this->context->input() ) );
        $tag->use_guard( new Tag_Guard( $module_settings, $is_amp ) );
        $tag->use_guard( new Tag_Environment_Type_Guard() );
        if ( $tag->can_register() ) {
        $tag->register();
        }
        }
        }

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 10upsimon added Module: Ads Google Ads module related issues P0 High priority PHP Type: Feature New feature labels Feb 26, 2024
@tofumatt tofumatt self-assigned this Feb 26, 2024
@tofumatt
Copy link
Collaborator

ACs 👍🏻

@tofumatt tofumatt removed their assignment Feb 26, 2024
@zutigrm zutigrm assigned zutigrm and unassigned zutigrm Feb 28, 2024
@eugene-manuilov eugene-manuilov self-assigned this Mar 1, 2024
@eugene-manuilov
Copy link
Collaborator

IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment Mar 1, 2024
@nfmohit nfmohit self-assigned this Mar 24, 2024
@nfmohit
Copy link
Collaborator

nfmohit commented Mar 27, 2024

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 nfmohit removed their assignment Mar 29, 2024
@nfmohit
Copy link
Collaborator

nfmohit commented Mar 29, 2024

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

@eugene-manuilov eugene-manuilov self-assigned this Mar 29, 2024
eugene-manuilov added a commit that referenced this issue Mar 29, 2024
@eugene-manuilov eugene-manuilov removed their assignment Mar 29, 2024
@eugene-manuilov
Copy link
Collaborator

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

@wpdarren wpdarren self-assigned this Mar 31, 2024
@wpdarren
Copy link
Collaborator

wpdarren commented Mar 31, 2024

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 wpdarren assigned nfmohit and wpdarren and unassigned wpdarren and nfmohit Mar 31, 2024
@wpdarren
Copy link
Collaborator

wpdarren commented Apr 1, 2024

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 wpdarren removed their assignment Apr 1, 2024
@aaemnnosttv
Copy link
Collaborator

@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.

@nfmohit
Copy link
Collaborator

nfmohit commented Apr 1, 2024

@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!

@wpdarren
Copy link
Collaborator

wpdarren commented Apr 2, 2024

@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
Copy link
Collaborator

wpdarren commented Apr 3, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Ads Google Ads module related issues P0 High priority PHP Type: Feature New feature
Projects
None yet
Development

No branches or pull requests

7 participants