1132222 - Implement further GA hooks on the standalone UI

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P2)

Comment 1

[Romain Testard [:RT]]

Requirements pulled from bug 1130174 for implementation.

Comment 2

[Romain Testard [:RT]]

Sevaan, is there anything else worth tracking from your standpoint so you get the right feedback on the UX?

Comment 3

[Sevaan Franks [:sevaan]]

No, I think that covers it for the Standalone UI. Thanks!

Comment 4

[Mark Banner (:standard8)]

(Commenting on User Story)
Implementation wise, I would suggest that for items 1,2,3,8,10,11 we create a new store for the standalone (or maybe driver) that doesn't have any of its own data but listens to actions from the dispatcher, and calls the relevant GA events.

For the other items (mainly link-clicks), we can probably dispatch a single specific action with some information about the link click that the GA store/driver then forwards.

Comment 5

[Romain Testard [:RT]]

Gareth, we need an extra event (number 13 in the US field) to track people who click URLs attached to the conversations (see bug 1118833 for feature details - essentially in Fx39 we allow users to attach the URL of a website to a conversation and we want to understand if people reach out to these links from the link clicker UI). Can you please help define the GA event in the US field? Thanks!

Comment 6

[Gareth Cull [:garethc]]

Hey Romain,

It's probably best from a privacy perspective to NOT capture details about the url, but know that people are interacting with that space within the UI.

ga('send', 'event', '/conversation/ interactions', 'link click', 'Shared URL');

Would this work?

Comment 7

[Romain Testard [:RT]]

Thanks Gareth. Agreed we don't need to capture the URL details, we just need to know if people click it and how often for us to work out the usefulness of the feature.
I updated the US under section 13.

Comment 8

[Mark Banner (:standard8)]

Gareth, I've recently noticed a message in analytics:

"""
We've identified problems in your setup. These may cause problems loading In-Page Analytics.

Your site is configured to set X-Frame-Options: headers. In-Page Analytics can only work in Full View mode on your site.
"""

Is this going to cause an issue for this bug? (We're setting X-Frame-Options for security reasons).

Comment 9

[Gareth Cull [:garethc]]

Hi Mark,

In-Page Analytics overlays GA data (clicks) onto an experience, which isn't actually applicable for our GA implementation. This is due to the following:

1) the majority of pages are conversations with their own unique url and therefore doesn't make sense to use this functionality for an individual conversation. Plus, since we are rolling up and anonymizing conversation urls under a common path /conversion/, i don't believe it would work properly anyway. 

2) We'll actually understand how people interact with the ui through our event implementation defined above.

Gareth
Gareth

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Updated the user story to cover verifying that were doing the right thing when Do-Not-Track is set.

Standard 8 and I chatted a bit about this more,and decided that a reasonable way forward  would be to start by writing a single stand-alone only analytics store that knows how to speak to GA.  We thought about the possibility as well of having a single standalone/desktop store with separate drivers, but decided that it would be simpler to approach it this way and refactor for generality later if we come to believe that it's worth the benefit.

One question that came up, for RT/Sevaan.  What's the point of the retry button on the expired/invalid notification view?  I.e. if either of those cases is true, how could retrying every magically work?  Or is this also intended to include cases like DNS failure and server outages too?

Comment 11

[Romain Testard [:RT]]

Fair point, this is probably setting an expectation that it could work if the user re-tries.
If when reaching this state the user has no chance to enter a room by hitting retry we should just change the UI to inform the user more appropriately. Sevaan you agree?

Comment 12

[Sevaan Franks [:sevaan]]

Agreed.

Comment 13

[Romain Testard [:RT]]

Removed GA event on retry button use from the US.

Comment 14

[Mark Banner (:standard8)]

I have a WIP for this.

(Commenting on User Story)
> 3) On successful join of the conversation room (ie. after they accept the
> dialogue prompting him to share mic and cam)
> ga('send', 'event', '/conversation/ interactions', 'success', 'Joined
> Conversation');

Romain, I'm splitting this into:

ga('send', 'event', '/conversation/ interactions', 'success', 'Media granted');
ga('send', 'event', '/conversation/ interactions', 'success', 'Media connected');

"Media granted" = user has granted permissions to use at least one device.
"Media connected" = the user has joined the conversation and streams are being sent and received.

Is that ok? The only other possible spot to log in place of this would be once all three of these are satisfied:

- the user has accepted permission
- the join request was accepted by the loop server
- the sdk has connected to the tokbox servers

This would be somewhere in-between the media granted & media connected events that I'm proposing (obviously media connected relies on the room generator being in the room).

Comment 15

[Romain Testard [:RT]]

(In reply to Mark Banner (:standard8) from comment #14)
> I have a WIP for this.
> 
> (Commenting on User Story)
> > 3) On successful join of the conversation room (ie. after they accept the
> > dialogue prompting him to share mic and cam)
> > ga('send', 'event', '/conversation/ interactions', 'success', 'Joined
> > Conversation');
> 
> Romain, I'm splitting this into:
> 
> ga('send', 'event', '/conversation/ interactions', 'success', 'Media
> granted');
> ga('send', 'event', '/conversation/ interactions', 'success', 'Media
> connected');
> 
> "Media granted" = user has granted permissions to use at least one device.
> "Media connected" = the user has joined the conversation and streams are
> being sent and received.
> 
> Is that ok? The only other possible spot to log in place of this would be
> once all three of these are satisfied:
> 
> - the user has accepted permission
> - the join request was accepted by the loop server
> - the sdk has connected to the tokbox servers
> 
> This would be somewhere in-between the media granted & media connected
> events that I'm proposing (obviously media connected relies on the room
> generator being in the room).

Great idea, that's an area we need more visibility on. Changing the US field to reflect this now.

Comment 16

[Mark Banner (:standard8)]

Attachment: Add more metrics reporting to the Loop standalone UI.

Simplish patch to add a new store that handles the GA metrics. We default to listening to actions where we can, but revert to listening to the activeRoomStore if we need information on state changes that the store already knows about.

I tried writing a test for StandaloneRoomFooter but I couldn't get React to simulate the link clicks. I think it is something to do with the use of the dangerouslySetInnerHtml. This doesn't feel worth the battle at the moment as it isn't a major issue if that bit doesn't work.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8594537 [details] [diff] [review]
Add more metrics reporting to the Loop standalone UI.

Review of attachment 8594537 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with a few changes.  Use your judgment on the ones you want.  r=dmose

::: browser/components/loop/content/shared/js/actions.js
@@ +472,5 @@
> +    RecordClick: Action.define("recordClick", {
> +      // Note: for ToS and Privacy links, this should be the link, for
> +      // other links this should be a generic description so that we don't
> +      // record what users are clicking, just the information about the fact
> +      // they clicked the link in that spot.

Might be worth have a couple of examples here, but not a showstopper.

::: browser/components/loop/standalone/content/js/standaloneMetricsStore.js
@@ +5,5 @@
> +var loop = loop || {};
> +loop.store = loop.store || {};
> +
> +/**
> + * The standalone metrics store is used to activities to

Missing a verb here.

@@ +24,5 @@
> +
> +  var METRICS_INTERACTIONS = loop.store.METRICS_INTERACTIONS = {
> +    general: "/conversation/ interactions",
> +    download: "Firefox Downloads"
> +  };

Let's call this METRICS_CATEGORY, and include a comment that makes it clear that this is to match up with the GA "category" concept.

@@ +26,5 @@
> +    general: "/conversation/ interactions",
> +    download: "Firefox Downloads"
> +  };
> +
> +  var METRICS_ACTIONS = loop.store.METRICS_ACTIONS = {

Maybe rename this METRICS_GA_ACTIONS and comment about how they (don't) relate to flux actions.

@@ +71,5 @@
> +      }
> +    },
> +
> +    /**
> +     * Returns initial state data for this store. These are mainly relections

reflections

@@ +90,5 @@
> +     * Saves an event to ga.
> +     *
> +     * @param  {String} category The category for the event.
> +     * @param  {String} action   The type of action.
> +     * @param  {String} label    The label for the event.

Would be nice to clarify that the label is an action-specific detail of some sort.

@@ +201,5 @@
> +
> +      this._storeEvent(METRICS_INTERACTIONS.general,
> +        type === "audio" ? METRICS_ACTIONS.audioMute : METRICS_ACTIONS.faceMute,
> +        muted ? "mute" : "unmute");
> +    }

Please split this up so that understanding this one statement doesn't require parsing three levels of abstraction simultaneously.

::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
@@ +193,5 @@
>        return mozL10n.get("legal_text_and_links", {
>          "clientShortname": mozL10n.get("clientShortname2"),
>          "terms_of_use_url": React.renderToStaticMarkup(
> +          <a href={loop.config.legalWebsiteUrl}
> +             target="_blank">

We decided this isn't necessary.

@@ +206,5 @@
>        });
>      },
>  
> +    recordClick: function(event) {
> +      if (event.target && event.target.href) {

A comment here about why this is necessary would be nice too.

@@ +217,5 @@
>      render: function() {
>        return (
>          <footer>
> +          <p dangerouslySetInnerHTML={{__html: this._getContent()}}
> +             onClick={this.recordClick}></p>

Probably worth a comment here about why we're using this static markup technique at all.

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +1093,4 @@
>  
> +    loop.store.StoreMixin.register({
> +      feedbackStore: feedbackStore,
> +      standaloneMetricsStore: standaloneMetricsStore

This probably wants to have a different keepalive mechanism, but if this really is the best one, I think it needs a comment so that it's clear that this shouldn't just be removed.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b07d738cfd6d

Comment 19

[Mark Banner (:standard8)]

I chatted with Dan a bit about using a driver rather than a store. We came to the conclusion that a store is probably fine for now, though the driver concept might make more sense. I'm adding a note to bug 1148381 to point out we might want to consider moving to a driver when we implement that.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b07d738cfd6d

Comment 21

[Gareth Cull [:garethc]]

When do these GA events get pushed to production? I'm not seeing anything show up yet in the hello GA profile?

Comment 22

[Gareth Cull [:garethc]]

Thanks for implementing the GA events. Here's a link to Link Clicker UI Dashboard: 
https://docs.google.com/a/mozilla.com/spreadsheets/d/12noPqQ2fJXH_5cJbaY8J1eAKqDBN0PHti4saykmIZRs/edit#gid=1980035091