Add google analytics event on tile clicks

RESOLVED FIXED in Firefox 43

Status

Hello (Loop)
Client
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Mardak, Assigned: Mardak)

Tracking

unspecified
mozilla43
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox43 fixed)

Details

User Story

Acceptance criteria:
1) Fire and GA event for when the link clicker joins a conversation first
2) Fire an event when an ad is clicked on.

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

2 years ago
Split from 1181987 comment 38:
> The success criteria was asking for a Google analytics event to be sent to track clicks on tiles (will allow correlating other GA traffic (JOIN events) to tile clicks.
(Assignee)

Updated

2 years ago
Depends on: 1181987

Comment 1

2 years ago
Updating the US per email exchange with Gareth.
User Story: (updated)
Here are some GA requirements based on how we've been tracking interactions on the link clicker UI. I've added in some additional information to capture impressions of the ad being shown. Is this possible to capture?

1) Fire a GA event when the link clicker joins the conversation first (and capture impression information about the ad that is displayed):

ga('send', 'event', '/conversation/ interactions', 'page load messages', 'You are the first one here.', {
  'dimension1':  adName 
});

where adName tells us what ad is being shown upon being the first one here. Needed to calculate a CTR on the ad.

2) Fire an event when an ad is clicked on
ga('send', 'event', '/conversation/ interactions', 'ad click', adName);

where adName is the ad being shown.
(Assignee)

Comment 3

2 years ago
How important is it to track which ad/adName was shown/clicked vs just that an ad was shown and an ad was clicked?

The tile is in an iframe that acts like a black box to the parent/link-clicker page. In fact, the parent only knows if the user has moused over the tile but not when the user clicks.

The iframe itself will be reporting specific tile impressions and clicks, and content services can report by tiles for each country/locale. So given that there's those details elsewhere, does the link clicker UI need to track it itself?
(Assignee)

Comment 4

2 years ago
Hrmm. I found an interesting hack/approach that can detect if the user clicked on the iframe from the link clicker:

addEventListener("blur", e => setTimeout(_ => console.log(document.activeElement)))

document.activeElement will be the iframe when the window blurs to open the clicked iframe page. It seems to work in Firefox and Chrome.
(Assignee)

Updated

2 years ago
Flags: needinfo?(garethcull.bugs)
It would give us a fuller picture of performance for those ads. Since I can see resulting traffic to mozilla properties from these in-house tiles, the only missing piece is understanding the performance of those tiles from a ctr perspective. What type of ad is driving quality traffic and getting users to convert on the destination page? Plus we could use this information to see the impact on any downstream events in the conversation funnel by types of ads shown. Is this difficult to add in? It would be good to consolidate this data in Google Analytics if possible. Thanks Ed!
Flags: needinfo?(garethcull.bugs)
(Assignee)

Comment 6

2 years ago
(In reply to Gareth Cull [:garethc] from comment #5)
> It would give us a fuller picture of performance for those ads. Since I can
> see resulting traffic to mozilla properties from these in-house tiles, the
> only missing piece is understanding the performance of those tiles from a
> ctr perspective.
That information is available from Content Services, and the tiles being served are dedicated to the "hello" channel, so CTR reports are separate from the desktop tiles.

The main difference of not tracking from Hello side is you wouldn't be able to associate which ads were seen by which users as that data is not tracked by Content Services and reports are aggregated.

I've CCed various people who can provide more information about what can be reported about the tiles.

Comment 7

2 years ago
My 2 cents: we're fine with using the CS reports regarding tile performance initially.
Perhaps we split this in 2 bugs since it sounds like this one could be implemented now (and has a chance to get deployed to production at launch - production deployment still targeted at Aug 23rd)?
(In reply to Romain Testard [:RT] from comment #7)
> My 2 cents: we're fine with using the CS reports regarding tile performance
> initially.
> Perhaps we split this in 2 bugs since it sounds like this one could be
> implemented now (and has a chance to get deployed to production at launch -
> production deployment still targeted at Aug 23rd)?

Just clarified with RT via irc, the split is for the first version to log without the ad name.

If so, then from comment 0, the GA event for the first item would be "Joined room" (I think, worth checking) - we'd just need a separate event for the click.
(Assignee)

Updated

2 years ago
Depends on: 1193090
(Assignee)

Comment 9

2 years ago
Created attachment 8646048 [details] [diff] [review]
v1
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #8646048 - Flags: review?(dmose)
(Assignee)

Comment 10

2 years ago
Created attachment 8646052 [details] [diff] [review]
v1.1
Attachment #8646052 - Flags: review?(dmose)
(Assignee)

Updated

2 years ago
Attachment #8646048 - Attachment is obsolete: true
Attachment #8646048 - Flags: review?(dmose)
Comment on attachment 8646052 [details] [diff] [review]
v1.1

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

Jumping in and stealing review to move this along. Not far off, but there's some optimizations that can be made.

::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
@@ +108,5 @@
> +        return;
> +      }
> +      this._watchedTile = true;
> +
> +      window.addEventListener("message", function(event) {

Please can you move this adding of the listener to a componentDidMount function, you can then add a componentWillUnmount function for removing the listener. These will only be called once per mount/unmount, so you won't need the additional tracking.

Here's an existing example:

http://hg.mozilla.org/mozilla-central/annotate/8cba870a352c/browser/components/loop/content/shared/js/views.js#l996

@@ +109,5 @@
> +      }
> +      this._watchedTile = true;
> +
> +      window.addEventListener("message", function(event) {
> +        if (event.data == "tile-click") {

nit: We use strict equals (===) in content code.

::: browser/components/loop/test/standalone/standaloneRoomViews_test.js
@@ +90,5 @@
> +      loop.config.tilesIframeUrl = "data:text/html,<script>parent.postMessage('tile-click', '*');</script>";
> +
> +      // Render the iframe into a fixture
> +      var fixture = document.querySelector("#fixtures");
> +      if (fixture == null) {

I don't think you need this check and what it adds - if it doesn't exist, the test should fail, because that's an expectation of the test that we have this defined.

@@ +108,5 @@
> +          }), fixture);
> +
> +      // XXX Call the onMouseOver handler because Simluate doesn't work yet
> +      // https://github.com/facebook/react/issues/1297#issuecomment-121622910
> +      view.watchForTileClick();

You won't need this with the componentDidMount changes.

@@ +118,5 @@
> +            linkInfo: "Tiles iframe click"
> +          }));
> +
> +        // Clean up the rendered iframe
> +        fixture.innerHTML = "";

Please can you move the cleanup bit to an afterEach function, I think it'll be clearer for later on if we're tidying up/moving the tests.
Attachment #8646052 - Flags: review?(dmose) → review-
(In reply to Mark Banner (:standard8) from comment #11)
> @@ +118,5 @@
> > +            linkInfo: "Tiles iframe click"
> > +          }));
> > +
> > +        // Clean up the rendered iframe
> > +        fixture.innerHTML = "";
> 
> Please can you move the cleanup bit to an afterEach function, I think it'll
> be clearer for later on if we're tidying up/moving the tests.

I've just found out, you need to make this:

React.unmountComponentAtNode(fixture);

Otherwise react may dump warnings on the console.
(In reply to Mark Banner (:standard8) from comment #11)
> ::: browser/components/loop/test/standalone/standaloneRoomViews_test.js
> @@ +90,5 @@
> > +      loop.config.tilesIframeUrl = "data:text/html,<script>parent.postMessage('tile-click', '*');</script>";
> > +
> > +      // Render the iframe into a fixture
> > +      var fixture = document.querySelector("#fixtures");
> > +      if (fixture == null) {
> 
> I don't think you need this check and what it adds - if it doesn't exist,
> the test should fail, because that's an expectation of the test that we have
> this defined.

Bah, I see this is for karma now. Please add a comment that this is why we're checking for null. I think we can also change the check to if (!fixture) {
(Assignee)

Comment 14

2 years ago
Created attachment 8646570 [details] [diff] [review]
v2

(In reply to Mark Banner (:standard8) from comment #11)
> > +      this._watchedTile = true;
> > +      window.addEventListener("message", function(event) {
> Please can you move this adding of the listener to a componentDidMount
> function, you can then add a componentWillUnmount function for removing the
> listener.
Neat. This cleaned up quite a bit.

> Here's an existing example:
> http://hg.mozilla.org/mozilla-central/annotate/8cba870a352c/browser/
> components/loop/content/shared/js/views.js#l996
For a moment I thought there was a bug with |this| but turns out React autoBinds methods to the instance from createClass.

> > +        if (event.data == "tile-click") {
> nit: We use strict equals (===) in content code.
Saw this when I rebased to fx-team too.

> > +      var fixture = document.querySelector("#fixtures");
> > +      if (fixture == null) {
> I don't think you need this check
I removed the check and renamed karma's stubs.js to head.js for any karma-specific pre-test js, so in this case, adding a div#fixtures.

> > +        // Clean up the rendered iframe
> > +        fixture.innerHTML = "";
> Please can you move the cleanup bit to an afterEach function, I think it'll
> be clearer for later on if we're tidying up/moving the tests.
Added React.unmountComponentAtNode(fixtures); to the top afterEach
Attachment #8646052 - Attachment is obsolete: true
Attachment #8646570 - Flags: review?(dmose)
(Assignee)

Comment 15

2 years ago
Created attachment 8646592 [details] [diff] [review]
v2.1
Attachment #8646570 - Attachment is obsolete: true
Attachment #8646570 - Flags: review?(dmose)
Attachment #8646592 - Flags: review?(dmose)
(Assignee)

Comment 16

2 years ago
Created attachment 8646599 [details] [diff] [review]
v2.2

This also updates the tests from bug 1171925 to use the global fixtures without special casing for karma within each test.
Comment on attachment 8646599 [details] [diff] [review]
v2.2

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

Looks good, r=dmose with nits addresed.

::: browser/components/loop/test/standalone/standaloneRoomViews_test.js
@@ +100,5 @@
> +            isFirefox: true,
> +            joinRoom: sandbox.stub(),
> +            roomState: ROOM_STATES.JOINED,
> +            roomUsed: false
> +          }), fixtures);

No need to assign this to anything since we're only using it for a side-effect.

@@ +102,5 @@
> +            roomState: ROOM_STATES.JOINED,
> +            roomUsed: false
> +          }), fixtures);
> +
> +      window.addEventListener("message", function onMessage() {

Add a comment here explaining that this is waiting on the iframe data URL postmessage above.
Attachment #8646599 - Flags: review+

Updated

2 years ago
Attachment #8646592 - Flags: review?(dmose)
(Assignee)

Updated

2 years ago
Attachment #8646592 - Attachment is obsolete: true

Comment 18

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/09feeb70edac
https://hg.mozilla.org/mozilla-central/rev/09feeb70edac
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43

Updated

2 years ago
Iteration: --- → 43.1 - Aug 24
You need to log in before you can comment on or make changes to this bug.