-
Notifications
You must be signed in to change notification settings - Fork 918
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Trigger TrackProductDownload for all mobile and desktop downloads #13416
Conversation
8663c90
to
5b44ecb
Compare
5b44ecb
to
106c30f
Compare
b4fe423
to
40f906e
Compare
d09117b
to
66e9d56
Compare
66e9d56
to
0d280c8
Compare
This is ready for review but is toooooo scary to merge before All Hands. |
@alexgibson Do you have time to take another look at this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through and tested a bunch of pages locally and everything seems to work well, and nice job on the tests!
I did have a few questions along the way, and I'm not sure if those are really issues or just things I thought I should flag / question.
The one thing I couldn't do is test with tagassistant, which for some reason refuses to connect on any demo server for me :/
Converting this back to a draft and exploring a different way of doing this. |
0d280c8
to
2b1c77f
Compare
2b1c77f
to
c8ebc7a
Compare
@alexgibson I made ALL THE UPDATES. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #13416 +/- ##
=======================================
Coverage 76.65% 76.66%
=======================================
Files 147 147
Lines 7878 7881 +3
=======================================
+ Hits 6039 6042 +3
Misses 1839 1839
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updates are looking good! I made a couple of comments below. I do wonder if we should apply the query params to the mobile app store links as well? (seems easier to read?)
One other thing that occurred to me is that not all of our adjust links seem to be using the adjust helpers :/
Perhaps if we're to move forward with this approach then we need to make sure we're adding the params to the manual links as well? It's also not 100% clear to me why we stopped using the helpers (perhaps worth asking Dan about?)
productSplit[1] === 'latest' ? 'release' : productSplit[1]; | ||
let releaseChannel = productSplit[1]; | ||
// (except for latest, msi, or sub installer - we update those to say release) | ||
if (['latest', 'stub', 'msi'].includes(releaseChannel)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might throw an error in IE (haven't tested, but checked https://caniuse.com/array-includes)
} else if (appStoreURL.test(downloadURL)) { | ||
} else if (appStoreURL.test(downloadURL) || iTunesURL.test(downloadURL)) { | ||
let iosProduct = 'unrecognized'; | ||
if (downloadURL.indexOf('/id989804926') !== -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree it might be worth applying the same params we use on adjust links to make this a bit clearer to read?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10 minutes of exploration tells me this could be complicated because the store links are often included in the adjust links. Can I add this to my list of future improvements and merge this as is? (once I fix the IE compatibility problem)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+
One-line summary
Add Adjust support to TrackProductDownload and trigger on all remaining desktop downloads (previously this had just been triggered on /all).
Significant changes and points to review
method
andlink_url
parameters to product_download eventga-product-download
on all pagesga-product-download
to adjust links and download buttonsIssue / Bugzilla link
#13238
Testing
Successful integration tests
This branch is up on demo4 for testing with https://tagassistant.google.com/
https://www-demo4.allizom.org/en-US/firefox/new/ (and click download)
https://www-demo4.allizom.org/en-US/firefox/download/thanks/?s=direct
https://www-demo4.allizom.org/en-US/firefox/
https://www-demo4.allizom.org/en-US/firefox/browsers/
https://www-demo4.allizom.org/en-US/firefox/browsers/
https://www-demo4.allizom.org/en-US/firefox/browsers/mobile/
https://www-demo4.allizom.org/en-US/firefox/browsers/mobile/ios
https://www-demo4.allizom.org/en-US/firefox/browsers/mobile/android
https://www-demo4.allizom.org/en-US/firefox/browsers/mobile/focus
https://www-demo4.allizom.org/en-US/firefox/developer/
https://www-demo4.allizom.org/en-US/firefox/channel/desktop/