968291 - Add GA Events to X-Ray Goggles

Status: RESOLVED WONTFIX

(Webmaker Graveyard :: X-Ray Goggles, defect)

Description

[Adam Lofting (:adamlofting)]

Add the new webmaker-analytics module to X-Ray Goggles and use this to track the interaction events listed on this Etherpad:

https://etherpad.mozilla.org/webmaker-ga-events-naming-convention

This will give us a rich set of data about how people use the tool, and how this usage relates to other aggregate data collected by GA.

Comment 1

[Alexander [:admix]]

I this bug relevant still? Can I pick it up?

Comment 2

[Scott [:thecount] Downe]

@admix That would be awesome!

You'll want to implement this like the other apps.

You'll add a bower component: https://github.com/mozilla/webmaker-analytics

That makes using google analytics inside the app pretty trivial.

Here is the implementation of analytics on popcorn maker, you'll want to find a similar implementation:
https://github.com/mozilla/popcorn.webmaker.org/commit/c1d30c792014b5a2e62b92b8b40b96955b2af45d

In the ether pad in the first comment, we give a rough layout for the events we'll want in goggles:

analytics.event("Activate X-ray Goggles");
analytics.event("Undo");
analytics.event("Redo");
analytics.event("Help");
analytics.event("Publish");

We need to find the appropriate areas in the goggles code for each event and hook up the analytics module to that area of code, then fire the events at the appropriate time.

The events are not in stone, and there may be other useful events, so we're open to changes to the above list, but it's a good starting place.

Anyway, ping me if you need any help or have questions!

Comment 3

[Alexander [:admix]]

Scott, can you please take a look at my progress so far:
https://github.com/admix/goggles.webmaker.org/commit/b5c6d7a625949ff3d2c4f071f5f97efbf89bbd5d
I added events to ones you specified and to 'Esc' as well.

Everything looks like work ok, only one thing that bugs me, error occurs from time to time, when I load goggles: 
   Uncaught Error: Mismatched anonymous define() module: function (analytics)

it goes to here: https://github.com/admix/goggles.webmaker.org/blob/bug968291/public/js/browser-screen.js#L1-L14

I think there is a problem with my defines and requires. Any thoughts on that?
Thanks.

Comment 4

[Scott [:thecount] Downe]

I added some line comments to the pull request.

Comment 5

[Alexander [:admix]]

(In reply to Scott [:thecount] Downe from comment #4)
> I added some line comments to the pull request.
I tried to use define([]) here: https://github.com/admix/goggles.webmaker.org/blob/b5c6d7a625949ff3d2c4f071f5f97efbf89bbd5d/webxray/src/touch-toolbar.js#L1-L7 , but it doesn't work, I think it tries to find requires in the main.js, but it could't - I added /js/main.js requirejs code to /src/main.js - didn't work for me as well. 
The other thing, I still can not get rid of that $(window).ready/requirejs mismatched error here:  https://github.com/admix/goggles.webmaker.org/blob/b5c6d7a625949ff3d2c4f071f5f97efbf89bbd5d/public/js/browser-screen.js#L1-L10 (the window.ready loads first and there might be a conflict because of that).
Would appreciate any help. Thanks.

Comment 6

[Scott [:thecount] Downe]

Added some more comments :)

Comment 7

[Alexander [:admix]]

Thank you for the comments, I made it work for the undo,redo, publish and escape. But for activate button it keeps looking for the file in the wrong path. https://github.com/admix/goggles.webmaker.org/commit/10a733f9fe345e48b81626be7cd7d7aae99f414e

I think it is somehow related to this require thing: https://github.com/admix/goggles.webmaker.org/blob/bug968291/public/js/main.js#L5

Comment 8

[Alexander [:admix]]

Attachment: https://github.com/mozilla/goggles.webmaker.org/pull/124

Added GA events to Activate X-Ray Button, usdo, redo, publish, esc, help.

One small thing is, I don't like the path I added to the require here: https://github.com/admix/goggles.webmaker.org/blob/bug968291/public/js/browser-screen.js#L8
But it works, maybe there is another way around.

PR -> https://github.com/mozilla/goggles.webmaker.org/pull/124

Comment 9

[Scott [:thecount] Downe]

Comment on attachment 8398724 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/124

One comment, but we're very close now :)

Comment 10

[Alexander [:admix]]

Comment on attachment 8398724 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/124

Fixed the spacing here: https://github.com/admix/goggles.webmaker.org/blob/6509fa639d6048b10952473801b76ccb70ccd7c1/public/js/browser-screen.js#L22-L42
now should be OK.

Comment 11

[Scott [:thecount] Downe]

Comment on attachment 8398724 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/124

This looks good to me.

I think, a sanity check from Pomax if he wants to take a look?

Comment 12

[Scott [:thecount] Downe]

Comment on attachment 8398724 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/124

This is looking good and I think it's just about ready, but going to move this on to Pomax now.

Comment 13

[Alexander [:admix]]

I updated the PR with 2 new commits.
 1) I separated GA events for: 'Undo','Redo','Help','Publish' and 'Quit', so that now they doesn't depend on the language user use -> GA will track English words as events.
 
 2) The second commit is: I added GA event to 'Publish to Internet' button, which appears when user click 'Publish' in the tool bar. This button actually matters for GA, imho.

Comment 14

[Adam Lofting (:adamlofting)]

:admix, I just wanted to say thank you for this! This work you've done is going to be really helpful to us in better understanding how the tools get used :)

Comment 15

[Pomax]

Comment on attachment 8398724 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/124

a few code comments. If you address them and set me r? again, It's probably r+ next time!

Comment 16

[Alexander [:admix]]

Comment on attachment 8398724 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/124

I made the changes and left 2 comments on a PR.
Basically:
 1) preventDefault(); was written to prevent CSP violation (all the functionality remain).
 2) $("body").append - didn't work for me, CSP fires unsafe-eval violation. That is what I tried:
$("body").append($("<script src='/webxray.js' data-lang='"+ localeInfo +"' data-baseuri='"+ hostname + "/" + localeInfo +"'>"));
Maybe there is another way to do it.

Comment 17

[Pomax]

Comment on attachment 8398724 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/124

it cannot be the case that clicking a button violates CSP, but preventing the default behaviour and then manually hacking in the same functionality is CSP-safe. 

As for the actual script injection, we can try jQuery's loadScript, but a script injection has to go in document.head in order to legally execute. Injecting in document.body is not good enough.

Comment 18

[Alexander [:admix]]

As a follow up on our discussion in IRC, as I got the idea: 
 - index page doesn't need CSP (no input fields, no potential injections);
 - once CSP removed, 'click' event can be modified (erase everything except GA);
 - include CSP to only publish page.

I modified 'click' event and now it uses jQuery and no CSP violations were fired and work fine. Here is the commit: https://github.com/admix/goggles.webmaker.org/commit/3f0a43aae68c1e8ab4e93fbd6e2f4924d5f74e29

Q: should I file separate ticket for CSP and make this one as a block?

Comment 19

[Pomax]

yeah let's morph this ticket into the simple fix for the GA script, and then do CSP separately.

Comment 20

[Alexander [:admix]]

Comment on attachment 8398724 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/124

Revered back to:
1)GA for Activate X-Ray, Undo, Redo, Publish, Help, Esc, PUBLISH
2) try/catch for JSON.parse
3) removed jQuery from require

PR -> https://github.com/mozilla/goggles.webmaker.org/pull/124
PS: I will then squash and rebase.

Comment 21

[Pomax]

Comment on attachment 8398724 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/124

few more nits, but also a big questionmark about how you're tracking the events. That code should not be a huge switch with hardcoded labels...

Comment 22

[Alexander [:admix]]

I changed the path in require(['analytics'], function(analytics){...
But a bug appeared, for some reason it tries to find analytics inside the /js/analytics.js ,however it should look for it here: /bower/webmaker-analytics/analytics.js, like it is specified 
in the /js/main.js -> https://github.com/admix/goggles.webmaker.org/blob/bug968291/public/js/main.js#L2-L13
Application finds other bower packed values, but not analytics. The odd part is that, once page is refreshed - application finds analytics in a right (/bower/...) location. I'm confused...
I posted one more commit with update, would appreciate to any help or direction.

(available all friday in IRC :) )
Thanks.

Comment 23

[Pomax]

Is this still relevant to our product?