[fix bug 1279291] Construct and send attribution code from download buttons #4456

alexgibson · 2016-11-08T15:10:43Z

Description

Constructs attribution data based on utm parameters and referrer information for relay to the Firefox stub installer.
Data is first signed and encoded via an XHR request to the stub_attribution_code service, before being appended to Bouncer download URLs as query parameters.
Data returned from the service is also stored in a session cookie, to save multiple requests when navigating pages.
Stub attribution requests are made only if the following criteria are satisfied:
1. User is on Windows (since only windows users get the stub installer).
2. User's browser does not need sha-1 bouncer.
3. User does not have DNT enabled.
4. User falls within a predefined sample rate limit (set via an env variable). See https://bugzilla.mozilla.org/show_bug.cgi?id=1278981#c6

Bugzilla link

https://bugzilla.mozilla.org/show_bug.cgi?id=1279291

Testing

Demo: https://www-demo4.allizom.org/en-US/
Local testing: to test this locally you'll have to set STUB_ATTRIBUTION_HMAC_KEY='foo' in your .env file (the value for the key can be anything).

Checklist

Requires l10n changes.
Related functional & integration tests passing.

alexgibson · 2016-11-09T10:33:36Z

@pmac before we get into code review and QA, I'd like to get your thoughts here in terms of approach?

pmac · 2016-11-09T13:31:16Z

bedrock/base/templates/base-pebbles.html

@@ -3,7 +3,7 @@
 <!doctype html>
 {# Note the "windows" class, without javascript platform-specific
     assets default to windows #}
-<html class="windows x86 no-js" lang="{{ LANG|replace('en-US', 'en') }}" dir="{{ DIR }}" data-latest-firefox="{{ latest_firefox_version }}" data-esr-versions="{{ esr_firefox_versions|join(' ') }}" {% if settings.GTM_CONTAINER_ID %}data-gtm-container-id="{{ settings.GTM_CONTAINER_ID }}"{% endif %} {% block gtm_page_id %}{% endblock %}>
+<html class="windows x86 no-js" lang="{{ LANG|replace('en-US', 'en') }}" dir="{{ DIR }}" data-latest-firefox="{{ latest_firefox_version }}" data-esr-versions="{{ esr_firefox_versions|join(' ') }}" {% if settings.GTM_CONTAINER_ID %}data-gtm-container-id="{{ settings.GTM_CONTAINER_ID }}"{% endif %} {% block gtm_page_id %}{% endblock %} {% if settings.STUB_ATTRIBUTION_HMAC_KEY %}data-stub-attribution-rate="{{ settings.STUB_ATTRIBUTION_RATE }}"{% endif %}>


I think all of the switching off of the JS should happen based on the STUB_ATTRIBUTION_RATE setting, per my comment in the other bug. This would mean that the JS could be included but the ajax endpoint would return a 403 if the rate were > 0 but no HMAC key were set, but that might possibly be useful for testing. The main thing though is that the inclusion of the JS and these values should be based on the RATE setting, not HMAC.

pmac · 2016-11-09T13:31:38Z

bedrock/base/templates/base-pebbles.html

+
+    {# Bug 1279291 #}
+    {% block stub_attribution %}
+      {% if settings.STUB_ATTRIBUTION_HMAC_KEY %}


also STUB_ATTRIBUTION_RATE here.

pmac · 2016-11-09T13:35:49Z

media/js/base/stub-attribution.js

+            return;
+        }
+
+        $('a[href*="https://download.mozilla.org/"]').each(function() {


we only need to update the windows link. none of the other platforms will use these attributes.

Yeah I still need to do this, good catch.

I've added this now. I made the call to append the params to both 32 and 64bit Windows URLs. The 64bit links don't seem to use the stub installer currently, but I'm making the assumption this could change in the future.

pmac · 2016-11-09T13:37:35Z

media/js/base/stub-attribution.js

+        // append stub attribution query params.
+        $.each(data, function(key, val) {
+            if (key === 'attribution_code' || key === 'attribution_sig') {
+                url += (url.indexOf('?') > -1 ? '&' : '?') + key + '=' + val;


don't think you need to look for the ?, if a link to download.m.o has no other query params it's worthless and we've got bigger problems.

Yes I don't think it's strictly necessary, but also it does no real harm leaving this here for completeness imo.

pmac · 2016-11-09T13:43:51Z

I do like the general direction. I was hoping you didn't have to parse the query params and could just grab them all and tack it onto the ajax request along with the referrer. You're welcome to do that still, but it looks like we've already got tools to find and parse UTM params, so that's cool. Like I said in a line comment, let's not use the HMAC setting for the JS at all. For one thing the RATE setting is the proper one in my opinion, and for another I'm not comfortable with a sensitive setting like that being referenced in the template code at all; a small mistake could expose the secret.

alexgibson · 2016-11-09T13:47:15Z

For one thing the RATE setting is the proper one in my opinion, and for another I'm not comfortable with a sensitive setting like that being referenced in the template code at all; a small mistake could expose the secret.

This is a great point, will update.

alexgibson · 2016-11-11T10:27:44Z

@pmac - I added a commit to make the Bouncer URL configurable via an env variable. I figured this was probably the simplest way for testing on demo? We can always remove this prior to merging.

pmac · 2016-11-11T13:28:32Z

@alexgibson seems like a fine change. The bit in the JS is a little unfortunate, but perhaps we can find a way of picking those elements out of the DOM w/o having to match the href? But on the whole it may well come in handy again in the future to be able to set the bouncer URL.

alexgibson · 2016-11-11T14:13:58Z

The bit in the JS is a little unfortunate, but perhaps we can find a way of picking those elements out of the DOM w/o having to match the href?

Yeah, thinking about it we could just drop the href from the selector and look for .download-link. It was this way originally to avoid the sha-1 bouncer URL's, but this should be safe now considering we're also checking the data-attributes for win and win64.

alexgibson · 2016-11-11T15:08:57Z

@pmac - this hopefully feels a little cleaner. I still had to add a check to make sure the link isn't transitional, but this at least avoids needing to depend on the Bouncer end point.

pmac · 2016-11-11T16:55:00Z

Very nice!

alexgibson · 2016-12-16T10:05:48Z

@pmac it occurred to me that using a session cookie for stub attribution might be too long, particularly for those who don't restart their browsers that often. I changed this PR to use a cookie that expires in 7 days, but even that might be too long perhaps. Any thoughts on what a sensible expiry date should be?

pmac · 2016-12-16T14:31:19Z

@alexgibson great catch. I'd think 24 hours would be plenty. After that I'm not sure we can say that the old source is what drove the download if they do finally complete it. Is it written in such a way that coming from a new source, even if there is already a cookie will overwrite or update it? This might be a good question for @chrismore et al.

alexgibson · 2016-12-16T14:33:04Z

Is it written in such a way that coming from a new source, even if there is already a cookie will overwrite or update it?

Not currently, the existing cookie would have to expire. This also saves on the number of requests we make to auth, so I'd prefer not to have to make things overwrite unless really necessary. 24 hours for the cookie should suffice I hope.

chrismore · 2016-12-16T20:26:29Z

24 hours sounds fine to me! Thanks

ejregithub · 2017-01-26T22:32:51Z

@alexgibson - dark launch of the service has been scheduled for the morning of Thursday Feb 2nd. Can you please look at a rebase here / put this into prod, behind switches, in time for that date, when Paul will flip the switches to on?

pmac · 2017-01-27T20:24:21Z

Just FYI @alexgibson, I changed how bedrock encodes the data (oremj and I decided to switch) in PR #4609, which has now merged. So I rebased demo__4 branch on current master and pushed it. The rebase was clean, but I didn't double check anything. But, if you want you could potentially start from the new demo__4 branch which is rebased, or do your own rebase on your branch and we can push the demo again when you're happy.

alexgibson · 2017-01-30T15:07:41Z

Just FYI @alexgibson, I changed how bedrock encodes the data (oremj and I decided to switch) in PR #4609, which has now merged. So I rebased demo__4 branch on current master and pushed it. The rebase was clean, but I didn't double check anything. But, if you want you could potentially start from the new demo__4 branch which is rebased, or do your own rebase on your branch and we can push the demo again when you're happy.

I've rebased and updated this branch, and re-pushed it to demo 4 - will give it some testing. I also added some extra logic to support the modal download links that we're just added to /new.

alexgibson · 2017-01-30T17:22:17Z

@jpetto - I think this is now ready again for review. Appreciate if you could r?

jpetto

Looking really solid. I don't see any blockers, but did pose a few questions.

jpetto · 2017-01-30T21:22:18Z

media/js/base/stub-attribution.js

+                version = $(this).data('downloadVersion');
+                // Currently only Windows 32bit uses the stub installer, but this could
+                // easily change in the future so we'll make that bet now.
+                if (version && (version === 'win' || version === 'win64')) {


Does this logic imply the comment above should be updated to reflect both Win32 and Win64?

Hmm, no - but perhaps my comment didn't make sense. I'm betting that they will want attribution data for both 32 and 64 bit builds eventually (even though it only forks with 32 bit installer currently), so I'm adding that case now. I'll update the comment to make it a little clearer.

jpetto · 2017-01-30T21:47:27Z

tests/unit/spec/base/stub-attribution.js

+
+    describe('getCookie', function() {
+
+        it('should return an object an expected', function() {


"Should return an expected object"?

jpetto · 2017-01-30T22:39:06Z

media/js/base/stub-attribution.js

+        }
+
+        // if there are no utm params and no referrer, do nothing.
+        if (utmCount === 0 && (typeof referrer === 'undefined' || referrer === '')) {


Apologies if this has been covered elsewhere, or is totally out of scope, but do we care about links coming from marketing emails or other non-web page sources? I'm not sure those have a referrer...

Currently the ask is to capture utm params, and failing those are not present, to fall back to referrer. If neither exist we do nothing. As for emails, if those links don't use utm params for tracking then we could add some special casing for those. but until someone asks for it, I think we're fine.

jpetto · 2017-01-30T22:43:53Z

media/js/base/stub-attribution.js

+     */
+    StubAttribution.isFirefoxNewScene2 = function(location) {
+        location = typeof location !== 'undefined' ? location : window.location.href;
+        return location.indexOf('/firefox/new/?scene=2') > -1;


Since we're dealing with external links, are we sure scene=2 will always directly follow the ?? Could we hit a scenario where _utm params are injected before the scene=2 key/val pair?

Good, call - will fix 👍

jpetto · 2017-01-30T22:54:51Z

Assuming this was discussed along the way, but what's the expected flow for this attribution? Guess is:

Windows user lands on external site X, which provides a _utm'd link to any page on mozilla.org except for /firefox/new/?scene=2. User lands on said page, gets the attribution info in a cookie, then clicks a download link on that page/during that visit pointing to /firefox/new/?scene=2, where attribution is pulled from cookie and injected prior to download kicking off? (Alternatively, user lands on a page like /firefox/channel with direct links and has attribution code injected there directly.)

alexgibson · 2017-01-31T09:41:23Z

Windows user lands on external site X, which provides a _utm'd link to any page on mozilla.org except for /firefox/new/?scene=2. User lands on said page, gets the attribution info in a cookie, then clicks a download link on that page/during that visit pointing to /firefox/new/?scene=2, where attribution is pulled from cookie and injected prior to download kicking off? (Alternatively, user lands on a page like /firefox/channel with direct links and has attribution code injected there directly.)

Correct yeah. I should have probably talked you through this up front (sorry). But the fact you managed to work it all out OK just from the code / comments gives me some faith that this isn't too arcane 👍

alexgibson · 2017-01-31T10:09:14Z

Thanks for the review @jpetto - I pushed a commit with some fixes.

jpetto · 2017-01-31T17:37:08Z

Nice! LGTM 👍

…uttons

alexgibson added Do Not Merge ⚠️ WIP labels Nov 8, 2016

alexgibson force-pushed the bug-1279291-stub-attribution-client-js branch 5 times, most recently from d4d1576 to 2143d50 Compare November 9, 2016 10:32

alexgibson force-pushed the bug-1279291-stub-attribution-client-js branch 2 times, most recently from 1fcec08 to 835f3e4 Compare November 9, 2016 11:17

pmac reviewed Nov 9, 2016

View reviewed changes

alexgibson force-pushed the bug-1279291-stub-attribution-client-js branch 2 times, most recently from fc3d31a to b135b99 Compare November 9, 2016 15:02

alexgibson removed the WIP label Nov 10, 2016

alexgibson force-pushed the bug-1279291-stub-attribution-client-js branch from b135b99 to edba3c4 Compare November 10, 2016 09:56

alexgibson force-pushed the bug-1279291-stub-attribution-client-js branch 2 times, most recently from 2904dc3 to 8487457 Compare November 11, 2016 15:06

alexgibson force-pushed the bug-1279291-stub-attribution-client-js branch from 8487457 to 7f9d5db Compare November 11, 2016 15:18

alexgibson force-pushed the bug-1279291-stub-attribution-client-js branch from 7f9d5db to b81468a Compare November 17, 2016 11:22

alexgibson force-pushed the bug-1279291-stub-attribution-client-js branch from 95b00f0 to c5471b1 Compare December 16, 2016 10:03

alexgibson force-pushed the bug-1279291-stub-attribution-client-js branch 3 times, most recently from d23b529 to 4006584 Compare December 16, 2016 17:42

alexgibson force-pushed the bug-1279291-stub-attribution-client-js branch from 4006584 to 1ff161a Compare December 21, 2016 10:20

alexgibson force-pushed the bug-1279291-stub-attribution-client-js branch from 1ff161a to 4e8d5bb Compare January 3, 2017 09:13

alexgibson force-pushed the bug-1279291-stub-attribution-client-js branch from 4e8d5bb to 750caaf Compare January 10, 2017 10:48

alexgibson force-pushed the bug-1279291-stub-attribution-client-js branch from 750caaf to f9b528f Compare January 30, 2017 14:39

alexgibson requested a review from jpetto January 30, 2017 17:21

jpetto suggested changes Jan 30, 2017

View reviewed changes

jpetto approved these changes Jan 31, 2017

View reviewed changes

alexgibson force-pushed the bug-1279291-stub-attribution-client-js branch from 1734cad to 77390ad Compare January 31, 2017 17:38

alexgibson removed the Do Not Merge ⚠️ label Jan 31, 2017

[fix bug 1279291] Construct and send attribution code from download b…

4eb1c87

…uttons

alexgibson force-pushed the bug-1279291-stub-attribution-client-js branch from 77390ad to 4eb1c87 Compare February 1, 2017 11:30

pmac merged commit 04af7ae into mozilla:master Feb 1, 2017

alexgibson deleted the bug-1279291-stub-attribution-client-js branch November 15, 2019 15:12

stephaniehobson mentioned this pull request Apr 15, 2024

Add products to product list page #14407

Merged


		describe('getCookie', function() {

		it('should return an object an expected', function() {

[fix bug 1279291] Construct and send attribution code from download buttons #4456

[fix bug 1279291] Construct and send attribution code from download buttons #4456

Conversation

alexgibson commented Nov 8, 2016 • edited

Description

Bugzilla link

Testing

Checklist

alexgibson commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexgibson Nov 9, 2016 • edited

Choose a reason for hiding this comment

pmac commented Nov 9, 2016

alexgibson commented Nov 9, 2016

alexgibson commented Nov 11, 2016 • edited

pmac commented Nov 11, 2016

alexgibson commented Nov 11, 2016

alexgibson commented Nov 11, 2016

pmac commented Nov 11, 2016

alexgibson commented Dec 16, 2016 • edited

pmac commented Dec 16, 2016

alexgibson commented Dec 16, 2016

chrismore commented Dec 16, 2016

ejregithub commented Jan 26, 2017

pmac commented Jan 27, 2017

alexgibson commented Jan 30, 2017 • edited

alexgibson commented Jan 30, 2017

jpetto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpetto commented Jan 30, 2017

alexgibson commented Jan 31, 2017

alexgibson commented Jan 31, 2017

jpetto commented Jan 31, 2017

alexgibson commented Nov 8, 2016 •

edited

alexgibson Nov 9, 2016 •

edited

alexgibson commented Nov 11, 2016 •

edited

alexgibson commented Dec 16, 2016 •

edited

alexgibson commented Jan 30, 2017 •

edited