-
Notifications
You must be signed in to change notification settings - Fork 4.1k
feat: add GTM integration #36859
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
base: master
Are you sure you want to change the base?
feat: add GTM integration #36859
Conversation
Thanks for the pull request, @bydawen! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this still a draft, just some early feedback. Before submitting for review, make sure to link this to supporting documentation such as a product proposal, relevant github issues, linked PRs, etc.
lms/envs/production.py
Outdated
##### GOOGLE TAG MANAGER IDS ##### | ||
GOOGLE_TAG_MANAGER_ID = AUTH_TOKENS.get('GOOGLE_TAG_MANAGER_ID') | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
##### GOOGLE TAG MANAGER IDS ##### | |
GOOGLE_TAG_MANAGER_ID = AUTH_TOKENS.get('GOOGLE_TAG_MANAGER_ID') |
Either the default value will come in from common.py, or a custom value will be set in the LMS_CFG YAML file. This line would have no effect and should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you totally correct, this is completely unnecessary now. Thank you for advice!
01bc5df
to
f2ba4cd
Compare
Hi @kdmccormick, thank you for the review and remarks. For this PR, I forgot to mark it as a draft before submitting — my apologies for that. My colleague created a forum post about GTM integration to gather feedback and start a discussion. I've also pinned the link at the top of the PR description: |
IMO you should also add GTM environment configuration. Example:
Copy from my comment on Open edX Forum: https://discuss.openedx.org/t/add-gtm-support-to-open-edx-core/16234/5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple requests. I'm also checking with my team on whether this needs a Product Proposal or whether the forum post is sufficient.
lms/templates/main.html
Outdated
@@ -177,6 +177,15 @@ | |||
</script> | |||
% endif | |||
|
|||
<% gtm_id = static.get_value("GOOGLE_TAG_MANAGER_ID", getattr(settings, "GOOGLE_TAG_MANAGER_ID", None)) %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<% gtm_id = static.get_value("GOOGLE_TAG_MANAGER_ID", getattr(settings, "GOOGLE_TAG_MANAGER_ID", None)) %> | |
<% gtm_id = static.get_value("GOOGLE_TAG_MANAGER_ID", settings.GOOGLE_TAG_MANAGER_ID) %> |
There is no reason to use getattr
when we are certain that settings.GOOGLE_TAG_MANAGER_ID
will at least be defined with a default value of None. In fact, it is harmful to use getattr
, because it would silence errors in the case where "GOOGLE_TAG_MANAGER_ID"
is typo'd, renamed, or removed.
This applies in general when writing Python -- always prefer accessing the attribute directly, only use getattr
when absolutely necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, fixed
lms/envs/common.py
Outdated
################### GOOGLE TAG MANAGER ################## | ||
# .. setting_name: GOOGLE_TAG_MANAGER_ID | ||
# .. setting_default: None | ||
# .. setting_description: The container ID for Google Tag Manager, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain more. Why would I set this? What does it do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a more detailed description of these settings.
Thank you for review!
Is it possible to use the site theming capability to insert this instead? See the comment on your discourse post: https://discuss.openedx.org/t/add-gtm-support-to-open-edx-core/16234/5?u=feanil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See @feanil ' s comment. I can see the community need for an easy way to configure GTM, but this is part of a general problem (adding configurable JS snippets) and we'd rather not continue adding vendor-specific settings into edx-platform unless absolutely necessary.
Hi @feanil, I’ve replied on the forum post about this. In short, the main reason behind our approach is to keep the implementation minimal. |
The minimal implementation is the one which does not add any new code :) Using site theming does not add any new code that needs to be maintained, but adding a new one-off setting and vendor-specific JS does. |
Open edX forum post with a proposal of the feature:
https://discuss.openedx.org/t/add-gtm-support-to-open-edx-core/16234
Added GTM support to enable easier integration of analytics and tracking scripts. Making it more flexible for platform operators to manage tags via the site configuration panel in the admin section of the platform:
If Google Tag Manager ID configured correctly it will appear on the website with corresponding GTM scripts initialisations:
Also we made PR to enable similar support on the MFE parts of edX platform:
frontend-platform