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

Google Tag Manager setup #34554

Merged
merged 10 commits into from
Jul 3, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions corehq/apps/analytics/static/analytix/js/gtm.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
"use strict";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this copied from gtm?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this script does the job of loading the analytics script and passing the desired user info here.
This is same approach used for loading other analytics tooling script.

/**
* Handles communication with the google tag manager API.
*/
hqDefine('analytix/js/gtm', [
'jquery',
'underscore',
'analytix/js/initial',
'analytix/js/logging',
'analytix/js/utils',
], function (
$,
_,
initialAnalytics,
logging,
utils
) {
var _get = initialAnalytics.getFn('gtm'),
_logger = logging.getLoggerForApi('Google Tag Manager'),
_ready = $.Deferred();

window.dataLayer = window.dataLayer || [];

/**
* Helper function to send event to Google Tag Manager.
* @param {string} eventName
* @param {object} eventData
* @param {function|undefined} callbackFn - optional
*/
var gtmSendEvent = function (eventName, eventData, callbackFn) {
_ready.done(function () {
var data = {
event: eventName,
};
if (eventData) {
_.extend(data, eventData);
}
window.dataLayer.push(data);
_logger.verbose.log(eventName, 'window.dataLayer.push');
}).fail(function () {
if (_.isFunction(callbackFn)) {
callbackFn();
}
});
};

$(function () {
var apiId = _get('apiId'),
scriptUrl = '//www.googletagmanager.com/gtm.js?id=' + apiId;

_ready = utils.initApi(_ready, apiId, scriptUrl, _logger, function () {
var userProperties = {
userId: _get('userId', 'none'),
isDimagi: _get('userIsDimagi', 'no', 'yes'),
isCommCare: _get('userIsCommCare', 'no', 'yes'),
domain: _get('domain', 'none'),
hqEnvironment: _get('hqInstance', 'none'),
};
// userProperties are sent first to be available for use as early as possible
gtmSendEvent('userProperties', userProperties);
gtmSendEvent('gtm.js', {'gtm.start': new Date().getTime()});
});
});

return {
sendEvent: gtmSendEvent,
};
});
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@
{% include 'analytics/initial/kissmetrics.html' %}
{% include 'analytics/initial/hubspot.html' %}
{% include 'analytics/initial/appcues.html' %}
{% include 'analytics/initial/gtm.html' %}
13 changes: 13 additions & 0 deletions corehq/apps/analytics/templates/analytics/initial/gtm.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{% load hq_shared_tags %}
{% initial_analytics_data 'gtm.apiId' ANALYTICS_IDS.GTM_ID %}
{% if request.couch_user %}
{% initial_analytics_data 'gtm.userId' request.couch_user.userID %}
{% initial_analytics_data 'gtm.userIsDimagi' request.couch_user.is_dimagi %}
{% initial_analytics_data 'gtm.userIsCommCare' request.couch_user.is_commcare_user %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd suggest saving that as gtm.userIsCommCareUser since that makes it more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in ac89b8a

{% endif %}
{% if domain %}
{% initial_analytics_data 'gtm.domain' domain|escapejs %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we are using escapejs here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out.
I reused this code from google.html. I believe it was added to avoid XSS attack . After looking more into it and don't think it is needed here as we are capturing domain from the url and it should not allow js mixed into it.
Removed it in ac89b8a

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, in that case I say we should retain it.

I assume its possible for someone to put anything in the domain part in the url & HQ will look for it. No actual domain page would load but I assume this part still gets added to the page, hence the escapejs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a spitball suggestion (feel free to ignore):
I agree with Manish's concern about someone manually changing the domain part of the url, so either

  1. keep the escapejs filter
  2. access the domain through a way we know it's right, e.g. request.couch_user.domain (maybe even request.domain?)

Option 2 seems least pretty though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the domain is invalid, the page should have 404ed long before this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the domain is invalid, the page should have 404ed long before this point.

Yes, this was tested and it is not possible to add script from the url.

@Charl1996 Correct me if I am wrong, my understanding is that domain for request or request.couch_user is still obtained from url ? (A user may be associated with multiple domains.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, but as far as I know there's some sanity checks it goes through in middleware such that it will fail if the domain is wrong (Jenny's comment maybe hints at this). That said, I haven't looked at the code.

{% endif %}
{% if ANALYTICS_CONFIG.HQ_INSTANCE %}
{% initial_analytics_data 'gtm.hqInstance' ANALYTICS_CONFIG.HQ_INSTANCE %}
{% endif %}
20 changes: 0 additions & 20 deletions corehq/apps/hqwebapp/templates/hqwebapp/base.html
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,6 @@
<!--[if IE 8]><html lang="{{ LANGUAGE_CODE }}" class="lt-ie9"><![endif]-->
<!--[if gt IE 8]><!--><html lang="{{ LANGUAGE_CODE }}"><!--<![endif]-->
<head>
{% if ANALYTICS_IDS.GTM_ID %}
{# This is fine as an inline script; it's independent of all HQ code and all third-party libraries #}
<!-- Google Tag Manager -->
<script>
(function(w,d,s,l,i){w[l]=w[l]||[];w[l].push({'gtm.start':
new Date().getTime(),event:'gtm.js'});var f=d.getElementsByTagName(s)[0],
j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src=
'https://www.googletagmanager.com/gtm.js?id='+i+dl;f.parentNode.insertBefore(j,f);
})(window,document,'script','dataLayer','{{ ANALYTICS_IDS.GTM_ID }}');
</script>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this happening now? or its not needed anymore?

same for iframe that was removed later?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The loading of gtm.js now happens here hence excluded from html. This was
done to be in consistent with other analytics tooling load mechanism and also to reuse the existing efficient loading technique with logs and validations.

The iframe one was optional and is only for scenario where JavaScript is disabled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we confirm this does not break anything that was being tracked already?

Copy link
Contributor

@orangejenny orangejenny May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check in with SaaS tech about this, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to confirm that Google Tag manager is not used at present by checking below things

  • No option in Cloud PR to add GTM_ID without which above snippet is not executed.
  • Checked through browser tools that gtm.js is not loaded for Prod or India.

I think it is wise to double check with SAAS though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ajeety4
I believe you were able to check with SaaS about it? Are there changes coming from your discussion?

Copy link
Contributor Author

@ajeety4 ajeety4 May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mkangia , I checked with SAAS about this. They do not use it and suggested to check with Marketing.
I talked with Marketing and they do use Google Tag Manager, I am just waiting for more details on it, should be able to get that in next couple of days.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: We reached out to marketing and they do have a Google Tag manager account with a container for CommcareHQ. As expected, the container for CommcareHQ is empty and not currently being used and aligns with the current findings.

(Just for FYI - they have a container for www.dimagi.com which is currently in use)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it is safe to remove this section or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is. It is safe even it was being used as the change just loads the same file from the javascript instead of the html.
I think it is still wise to merge this only after the decision to use GTM is finalised.
(No use to have the code that is not actively used or planned to be used.)

<!-- End Google Tag Manager -->
{% endif %}

{% captureas title_block %}{% block title %}{% endblock title %}{% endcaptureas %}
{% captureas title_context_block %}{% block title_context %}{% endblock title_context %}{% endcaptureas %}
<title>
Expand Down Expand Up @@ -187,13 +174,6 @@
{% endblock %}
</head>
<body>
{% if ANALYTICS_IDS.GTM_ID %}
<!-- Google Tag Manager (noscript) -->
<noscript><iframe src="https://www.googletagmanager.com/ns.html?id={{ ANALYTICS_IDS.GTM_ID }}"
height="0" width="0" style="display:none;visibility:hidden"></iframe></noscript>
<!-- End Google Tag Manager (noscript) -->
{% endif %}

{# for setting up page-wide backgrounds #}
{% block background_content %}{% endblock %}

Expand Down