OBPIH-5218 Add gtag for google analytics #3927

awalkowiak · 2023-03-16T18:01:29Z

removed old google-analytics:1.0 dependency
added gsp layout for react (copy of main) to not overwrite the main one
added tag lib injecting gtag tracking code

- removed old google-analytics:1.0 dependency - added gsp layout for react (copy of main) to not overwrite the main one - added tag lib injecting gtag tracking code

mdpearson · 2023-03-16T18:16:13Z

grails-app/taglib/org/pih/warehouse/GoogleAnalyticsTagLib.groovy

+ * */
+ def gtag = {attrs ->
+ if (!getTrackingIds()) {
+ out << ""


is there a reason we're pushing an empty string here?

No reason, we can push nothing.

mdpearson · 2023-03-16T18:18:34Z

grails-app/taglib/org/pih/warehouse/GoogleAnalyticsTagLib.groovy

+
+ gtag('config', TRACKING_ID);
+ </script>
+ * */


Not sure this comment is necessary

It's not not necessary. I would maybe include a link to the google tag docs to help explain the what and why.

mdpearson · 2023-03-16T18:21:51Z

src/assets/grails-template.html

@@ -1,7 +1,7 @@
 <html>
 <head>
 <meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
- <meta name="layout" content="main" />
+ <meta name="layout" content="react" />


I'm confused about this change. Why not keep content="main" here and add the <g:gtag /> line to main.gsp? Is there a reason we need both main.gsp and react.gsp?

I suggested adding this to main.gsp on the tech huddle, and I remember @jmiranda had some concerns regarding that since it is the main layout. If he is fine with adding gtag to the main.gsp, then I'm fine with it either.

Nah, we don't use main anywhere else as far as I know. I don't remember having any reservations about that so my apologies for the confusion.

I would be more concerned with adding react.gsp since we already have a partial template _react.gsp so it might get confusing to distinguish between the two.

Ok, I'll change it back to main.gsp

mdpearson

Apart from one question my comments are very minor, take 'em or leave 'em. The thing I'm most curious about is why we have a react.gsp and a main.gsp now that appear identical except for one line. Can the code duplication be avoided?

mdpearson · 2023-03-16T18:24:19Z

grails-app/conf/BuildConfig.groovy

@@ -218,7 +218,6 @@ grails.project.dependency.resolution = {
 // Not critical to application (might require code changes)
 //build(":codenarc:0.17")
 //compile(":dynamic-controller:0.3")
- compile(":google-analytics:1.0")


I love it when we lose a dependency! Yay!

jmiranda · 2023-03-17T04:55:00Z

grails-app/views/layouts/custom.gsp

@@ -62,7 +62,7 @@
 <g:render template="/common/fullstory"/>
 <g:render template="/common/hotjar"/>

- <ga:trackPageview/>
+ <g:gtag />


should we not just call it the same as before?

if we do change it I'd like it to be more explicit. gtag could mean anything so something like

<g:googleAnalytics />

or

<g:googleTagManager />

or

<g:globalSiteTag />

would be much better

<g:globalSiteTag /> will be most suiting

jmiranda · 2023-03-17T04:56:17Z

grails-app/taglib/org/pih/warehouse/GoogleAnalyticsTagLib.groovy

+ * tracking ids as list of strings
+ * */
+ private getTrackingIds() {
+ return ConfigurationHolder.config.google.gtag.trackingIds


Can you explain the multiple tracking IDs? Or provide some documentation for when this might be used?

You can have multiple google analytics properties. And each has to be configured separately within the gtag tracking code as:

gtag('config', TRACKING_ID_1); gtag('config', TRACKING_ID_2); gtag('config', TRACKING_ID_3); ...

https://developers.google.com/analytics/devguides/collection/gtagjs#configure_additional_google_analytics_properties

jmiranda · 2023-03-17T05:00:57Z

src/assets/grails-template.html

@@ -1,7 +1,7 @@
 <html>
 <head>
 <meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
- <meta name="layout" content="main" />
+ <meta name="layout" content="react" />


Nah, we don't use main anywhere else as far as I know. I don't remember having any reservations about that so my apologies for the confusion.

I would be more concerned with adding react.gsp since we already have a partial template _react.gsp so it might get confusing to distinguish between the two.

jmiranda · 2023-03-17T05:02:25Z

grails-app/taglib/org/pih/warehouse/GoogleAnalyticsTagLib.groovy

+
+ gtag('config', TRACKING_ID);
+ </script>
+ * */


It's not not necessary. I would maybe include a link to the google tag docs to help explain the what and why.

jmiranda · 2023-03-17T05:05:21Z

grails-app/views/layouts/custom.gsp

@@ -62,7 +62,7 @@
 <g:render template="/common/fullstory"/>
 <g:render template="/common/hotjar"/>

- <ga:trackPageview/>
+ <g:gtag />


if we do change it I'd like it to be more explicit. gtag could mean anything so something like

<g:googleAnalytics />

or

<g:googleTagManager />

or

<g:globalSiteTag />

would be much better

mdpearson

Looks good to me! Thanks!

OBPIH-5218 Add gtag for google analytics

2eff04e

- removed old google-analytics:1.0 dependency - added gsp layout for react (copy of main) to not overwrite the main one - added tag lib injecting gtag tracking code

awalkowiak requested review from jmiranda and mdpearson March 16, 2023 18:01

mdpearson reviewed Mar 16, 2023

View reviewed changes

mdpearson requested changes Mar 16, 2023

View reviewed changes

mdpearson reviewed Mar 16, 2023

View reviewed changes

jmiranda requested changes Mar 17, 2023

View reviewed changes

OBPIH-5218 Improvements after review

eaa3af3

mdpearson approved these changes Mar 17, 2023

View reviewed changes

OBPIH-5218 Change tracking id config property name

487231f

awalkowiak merged commit edd16a0 into release/0.8.21-hotfix1 Mar 20, 2023

awalkowiak deleted the OBPIH-5218 branch March 20, 2023 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OBPIH-5218 Add gtag for google analytics #3927

OBPIH-5218 Add gtag for google analytics #3927

awalkowiak commented Mar 16, 2023

mdpearson Mar 16, 2023

awalkowiak Mar 16, 2023

mdpearson Mar 16, 2023

jmiranda Mar 17, 2023

mdpearson Mar 16, 2023

awalkowiak Mar 16, 2023

jmiranda Mar 17, 2023

awalkowiak Mar 17, 2023

mdpearson left a comment

mdpearson Mar 16, 2023

jmiranda Mar 17, 2023

jmiranda Mar 17, 2023

awalkowiak Mar 17, 2023 •

edited

Loading

jmiranda Mar 17, 2023

awalkowiak Mar 17, 2023

jmiranda Mar 17, 2023

jmiranda Mar 17, 2023

jmiranda Mar 17, 2023

mdpearson left a comment

OBPIH-5218 Add gtag for google analytics #3927

OBPIH-5218 Add gtag for google analytics #3927

Conversation

awalkowiak commented Mar 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdpearson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awalkowiak Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdpearson left a comment

Choose a reason for hiding this comment

awalkowiak Mar 17, 2023 •

edited

Loading