Conversation
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.
Looks good –– just have a small concern with the staging environment.
71d6691
to
0ef53f2
Compare
if (window.ENVIRONMENT && | ||
window.ENVIRONMENT.REACT_APP_GOOGLE_ANALYTICS_KEY && | ||
window.ENVIRONMENT.REACT_APP_GOOGLE_ANALYTICS_KEY !== "" && | ||
window.ENVIRONMENT.ENVIRONMENT !== 'development') { |
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.
This is here to demo how to implement this in production, but this line should be commented out to see the logging in development.
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 applied this diff and followed your testing instructions:
diff --git a/src/app/public/index.html b/src/app/public/index.html
index 6a90f32..d146c41 100644
--- a/src/app/public/index.html
+++ b/src/app/public/index.html
@@ -36,8 +36,7 @@
<script>
if (window.ENVIRONMENT &&
window.ENVIRONMENT.REACT_APP_GOOGLE_ANALYTICS_KEY &&
- window.ENVIRONMENT.REACT_APP_GOOGLE_ANALYTICS_KEY !== "" &&
- window.ENVIRONMENT.ENVIRONMENT !== 'development') {
+ window.ENVIRONMENT.REACT_APP_GOOGLE_ANALYTICS_KEY !== "") {
console.log(window.ENVIRONMENT.REACT_APP_GOOGLE_ANALYTICS_KEY);
}
</script>
I was able to get both endpoints to print the GA token in the console. I'm at a loss for how the React development server is reading environment variables from .env.backend
though. The Docker Compose service is only setup to read from .env.frontend
.
I would also apply this diff to keep the task definitions in sync:
diff --git a/deployment/terraform/task-definitions/app_cli.json b/deployment/terraform/task-definitions/app_cli.json
index ea819b1..3c12dc0 100644
--- a/deployment/terraform/task-definitions/app_cli.json
+++ b/deployment/terraform/task-definitions/app_cli.json
@@ -18,6 +18,8 @@
{ "name": "DJANGO_SECRET_KEY", "value": "${django_secret_key}" },
{ "name": "DEFAULT_FROM_EMAIL", "value": "${default_from_email}" },
{ "name": "GOOGLE_SERVER_SIDE_API_KEY", "value": "${google_server_side_api_key}" },
+ { "name": "REACT_APP_GOOGLE_CLIENT_SIDE_API_KEY", "value": "${google_client_side_api_key}" },
+ { "name": "REACT_APP_GOOGLE_ANALYTICS_KEY", "value": "${google_analytics_key}" },
{ "name": "ROLLBAR_SERVER_SIDE_ACCESS_TOKEN", "value": "${rollbar_server_side_access_token}" },
{ "name": "REACT_APP_ROLLBAR_CLIENT_SIDE_ACCESS_TOKEN", "value": "${rollbar_client_side_access_token}" }
],
After applying the suggestion below, I was able to see expected results from the $ terraform plan
output.
These are coming in from a web/environment.js file injected by Django. |
I tested this out and it's working as described! I think you can feel free to include the demo commit -- since I'll use it -- but change the commit name message to indicate that it is in place for subsequent work. |
🤦♂️. I keep getting confused when I see |
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.
👍 Looks good to me. Worth getting a +1 from @rbreslow too!
Shows how the variable will be used in production. To demo this, add REACT_APP_GOOGLE_ANALYTICS_KEY to your `.env.backend` file. Run `cibuild` and `server`, then check in :6543/ and :8081/ that the variable is logged to the console. This content will be replaced with actual Google Tag Manager code.
b91a2bc
to
980ff1f
Compare
Just force pushed all recommendations. Ready for another look. |
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.
👍
This should help log their errors in Rollbar
232132e
to
af2c914
Compare
Thanks for taking a look! Merging now. |
Overview
Wires up Google Analytics code as an environment variable. Also updated
terraform.tfvars
in the production bucket with the real value.I did not add it to the staging bucket because we should not be tracking that.
The final commit is for demoing, and will be dropped before the merge.
Connects #394
Notes
When wiring up Google Tag Manager, we should do something like what's done for Rollbar, where we check if the key is available:
https://github.com/open-apparel-registry/open-apparel-registry/blob/d9978b79474a6b06e4fd18846a9e919199c5b475/src/app/public/index.html#L40-L42
This will ensure that analytics are not captured for dev / test environments.
Testing Instructions
Check out this branch
Add a value to
.env.backend
like this:Run
cibuild
andserver