-
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
Remove functionality preventing GA4 from outputting tags when GTM takes over #8196
Comments
@jimmymadon is it possible to add Type and Priority label to this ticket at this moment? |
AC 🌶️ |
IB ✅ |
Unnasigning myself as it will take me more time to go through whole PR, in order to focus on priority issue I need to finalize till EOD before the vacation |
QA Update:
|
I just ran through the QA steps as you did and saw both tags: The QA Brief omits this bit of info, but did you remember to check the website in a private browsing window/another browser where you aren't logged in to WordPress? Otherwise a tag/tags might be missing because they don't appear for signed-in users. But I was able to see both after testing, as long as I viewed the site as an "anonymous" user on the front-end. 🤔 |
QA Update:
|
@wpdarren You do need to select GA4 and set up Analytics on Site Kit as well. Are you connecting Tag Manager AND Analytics on the site? |
@tofumatt No, as I said in my comment, I have not set up Analytics, just Google Tag Manager with a GA4/Google Tag against a container. I think we need to sync about this because I have spent much time trying to get this to work as per the QAB. I will reach out to you when I am online. |
Screenshot.2024-03-05.at.11.29.24.mp4I've attached a screenshot showing the QA steps I took to get both. Seems like the QA Brief was misunderstood, thinking GA shouldn't be connected, but it should 😄 |
Feature Description
Originally, when GA was already connected, and the selected Tag Manager container had a tag that was a valid UA
trackingId
which matched the existingpropertyId
saved in Analytics settings, we would prevent the Analytics module from outputting the snippet. This was mainly to prevent UA from tracking duplicate events. This screen is highlighted in point 2 of this comment. This issue does not arise as having multiple snippets for the same GA4 data stream does not duplicate events in GA4.Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Implementation Brief
Note: Removing code from the old
Analytics
module is unnecessary since this will be removed in a couple of sprints. However, for completeness of this PR/issue and since SAM might not launch at the same time as this issue being merged, we should remove these pieces anyways.Remove the
gaPropertyID
setting and all of its usage from the Tag Manager module:includes/Modules/Tag_Manager/Settings.php
:gaPropertyID
setting.includes/Modules/Tag_Manager.php
:googlesitekit_analytics_can_use_snippet
filter and its implementation incan_analytics_use_snippet
.assets/js/modules/tagmanager/datastore/base.js
:gaPropertyID
setting field.assets/js/modules/tagmanager/hooks/useGAPropertyIDEffect.js
and all of its usage from:assets/js/modules/tagmanager/components/settings/SettingsEdit.js
assets/js/modules/tagmanager/components/setup/SetupMain.js
Remove the
canUseSnippet
setting and all of its usage from theAnalytics
andAnalytics-4
modules:includes/Modules/Analytics/Settings.php
includes/Modules/Analytics_4/Settings.php
includes/Modules/Analytics/Tag_Guard.php
assets/js/modules/analytics/datastore/base.js
:canUseSnippet
setting.assets/js/modules/analytics/datastore/settings.js
:getCanUseSnippet
selector.assets/js/modules/analytics/components/common/AdsConversionIDTextField.js
assets/js/modules/analytics/components/common/UseSnippetSwitch.js
assets/js/modules/analytics/components/settings/OptionalSettingsView.js
assets/js/modules/analytics/components/settings/SettingsUseSnippetSwitch.js
assets/js/modules/analytics/components/setup/SetupUseSnippetSwitch.js
Test Coverage
googlesitekit_analytics_can_use_snippet
fromtests/phpunit/integration/Modules/Tag_ManagerTest.php
andtests/phpunit/integration/Modules/Analytics/SettingsTest.php
.getCanUseSnippet
selector tests fromassets/js/modules/analytics/datastore/settings.test.js
.canUseSnippet
andgaPropertyID
that have been removed above from stories, tests and fixtures.QA Brief
Changelog entry
The text was updated successfully, but these errors were encountered: