search

railslove / rack-tracker

Tracking made easy: Don’t fool around with adding tracking and analytics partials to your app and concentrate on the things that matter.
https://www.railslove.com/open-source
MIT License
647 stars 121 forks source link

Avoid showing the JS code when GA tracker is not defined #149

Closed chalofa closed 4 years ago

chalofa commented 4 years ago

with Rails 6, in the system tests, I'm seeing all these Google Analytics extra events as page content: image

Not exactly sure what Rails change caused this issue, but before Rails v6, the tests rendered those GA events as:

Rails 5.2:

<head>
...
<script type="text/javascript">
  ga("ec:setAction","checkout",{"step":"1"});
  ga("ec:addImpression",...
  ga(...)
</script>
</head>
<body>
...
</body>

(note the lack of the GA snippet, but at least, everything is wrapped in a <script> tag

in Rails 6:

<head>
...
ga("ec:setAction","checkout",{"step":"1"});
ga("ec:addImpression",...
ga(...)
</head>
<body>
...
</body>

there's no <script> that wraps that JS code, and therefore, the browser just shows it as visible text...

the change in this PR ensures that if there's no GA tracker, nothing will be rendered (which is the workaround that I'm using right now to avoid this: ensuring rack-tracker is disabled for the test environment)

chalofa commented 4 years ago

@DonSchado looks like you are pretty active with this Google Analytics tracker... if you can take a look at this PR, I would appreciate it...

probably way easier to see without all those whitespace changes: https://github.com/railslove/rack-tracker/pull/149/files?w=1

DonSchado commented 4 years ago

I don't get the diff... is this just a change of indentation? Or also a scope change?

chalofa commented 4 years ago

I don't get the diff... is this just a change of indentation? Or also a scope change?

@DonSchado basically, wrap the whole <script> tag inside the top if tracker...

DonSchado commented 4 years ago

hm wasn't that already fixed here: https://github.com/railslove/rack-tracker/commit/f3d4e65dee9527136c7613940f5cee16fba91f88 ? Sorry I'm confused

chalofa commented 4 years ago

hm wasn't that already fixed here: f3d4e65 ? Sorry I'm confused

@DonSchado nope, the indentation is totally messed up, see this image:

image

I added some indentation so it's easier to see the nested ifs, and move the whole template inside the if tracker conditional, so if no tracker is defined, it won't render those events that are outside that conditional

DonSchado commented 4 years ago

Ah I see. Thanks for taking the time! Very appreciated :)