Instrument the restart-required flows (closes #1375).#1381
Instrument the restart-required flows (closes #1375).#1381chuckharmston merged 1 commit intomozilla:masterfrom chuckharmston:1375-restart-ga
Conversation
|
Note: the UTMs were added in #1379, where I added the restart-tab-opening. |
frontend/src/app/lib/addon.js
Outdated
| if (useMozAddonManager) { | ||
| sendToGA('event', gaEvent); | ||
| mozAddonManagerInstall(downloadUrl).then(() => { | ||
| gaEvent.eventValue = RESTART_REQUIRED ? 1 : 0; |
There was a problem hiding this comment.
...yeah. I don't know why this worked when I tested it out, I'm going to backtrack to make sure it's doing what's intended.
There was a problem hiding this comment.
At least the tests failed.
|
Needs a rebasin', but not sure if we want to fix the "Insturment" typo in commit message while we're at it. |
| if (useMozAddonManager) { | ||
| sendToGA('event', gaEvent); | ||
| mozAddonManagerInstall(downloadUrl).then(() => { | ||
| gaEvent.eventValue = RESTART_NEEDED ? 1 : 0; |
There was a problem hiding this comment.
Is this just casting a Boolean to an int? And if so, couldn't you just do gaEvent.eventValue = Number(RESTART_NEEDED);?
$ node -e console.log(Number(true)); # 1
$ node -e console.log(Number(false)); # 0There was a problem hiding this comment.
Yeah, but when you're giving numbers magic values, I'd rather be explicit about what the range of values are. Clear > cute.
chuckharmston
changed the title
|
I pulled the branch down, and there were no errors, but I wasn't able to fully test the small measurement pieces (eventValue and utm values). I ventured down #1330 (comment) to enable |
|
Sure. Merging. |
4 participants
r? @ckprice