deptofdefense / move.mil

The entry point website to the Defense Personal Property System (DPS).
https://www.move.mil
Other
15 stars 15 forks source link

adding the ga tag in the header on production env. fixes #337 #383

Closed rich-allen-gov closed 6 years ago

rich-allen-gov commented 6 years ago

Summary of Changes

Adds a second analitics tag so that non-government access to data is easier, and more filterable than with DAP

Testing

To verify the changes proposed in this pull request…

Check your browsers network console and verify that the GA javascript files have been added.

The tag only gets put on production so I'm not sure of the testing scenario. We could add additional dev/staging accounts and use an env variable to configure if we feel that sort of testing would add value.

rich-allen-gov commented 6 years ago

Note: I messed up branch naming here, the branch references 337 however this actually fixes #377.

rich-allen-gov commented 6 years ago

@jgarber623-gov updated, my thought was google was specific to have it in the head, I thought that may have to do with some of the more advanced data they collect and load order. I moved it back to the bottom with the dap script and it doesn't seem like there are any noticeable changes in how it loads so from an code organization perspective this makes sense.

jgarber623-gov commented 6 years ago

@rich-allen-gov Actually, one additional thing to do.

The site uses a Content Security Policy (configured on line 339 in .ebextensions/nginx.conf). Can you add https://www.googletagmanager.com to the script-src directive?

If there are additional domains that GTM calls out to from that script, they'll likely need to be added to the CSP, too.

rich-allen-gov commented 6 years ago

updated, I actually have a question on this now, would it make sense for me to remove this script out of that loop and something more like

<%- if Rails.env.ga_key? -%>

and then use the env? I can update that pretty quickly if you think it's worth the effort and won't be trouble to coordinate.

rich-allen-gov commented 6 years ago

I would add the appropriate bin/setup modifications as well

jgarber623-gov commented 6 years ago

@rich-allen-gov

Re: your comment

I don't think we need to go the environment variable route for a few reasons:

  1. The code is already wrapped in a "is this production?" conditional, so GA/GTM won't load in any other environment.
  2. We're not currently collecting metrics on any environment other than production. If we were, then using an environment variable like you suggest would be A Good Idea.
  3. There may be a time that we pursue Number Two, but we're not doing that now, so I think it'd be best to keep things simple for the time being.

Yours is a good suggestion though and worth keeping in mind in the future.

rich-allen-gov commented 6 years ago

ok, that makes sense. Are there any special steps to flag the nginx change for testing in stage? If not this is good to go, I think.

jgarber623-gov commented 6 years ago

@rich-allen-gov Nope, nothing extra to do. Elastic Beanstalk updates the Nginx config during the deploy process based on the configuration files in the repo.

I'll rebase/merge this now and it'll head on out to staging.