Conversation
2ad4c06 to
03b981a
Compare
|
Thanks for this @bluewave41 - before I review, would you mind fixing up these failing tests? Let me know if I can help - happy to! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #17121 +/- ##
=======================================
Coverage 81.70% 81.71%
=======================================
Files 174 174
Lines 9326 9330 +4
=======================================
+ Hits 7620 7624 +4
Misses 1706 1706 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Updates Firefox for Android Google Play store links to carry through (or default) UTM campaign parameters via the referrer= query string, so Android “thanks” flows can retain attribution when navigating to the Play Store.
Changes:
- Remove the static, pre-encoded UTM referrer from the
GOOGLE_PLAY_FIREFOX_LINK_UTMSsetting and instead constructreferrer=dynamically. - Thread request UTM values through
download_firefox→android_builds→FirefoxAndroid.get_download_url(...). - Add view-level tests asserting the Android thanks page link contains default/overridden UTM values.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| bedrock/settings/appstores.py | Removes the hard-coded encoded referrer from the Play Store URL constant. |
| bedrock/firefox/firefox_details.py | Extends Android download URL generation to append an encoded referrer= built from UTM params. |
| bedrock/firefox/templatetags/helpers.py | Collects UTM params from the request (with defaults) and passes them into Android download URL generation. |
| bedrock/firefox/tests/test_views.py | Adds tests for UTM presence/defaulting/overriding in the Android thanks page Play Store link. |
| # Link to Firefox for Android on the Google Play store with Google Analytics | ||
| # campaign parameters. | ||
| # To clarify below, 'referrer' key value must be a URL encoded string of utm_* | ||
| # key/values (https://bugzilla.mozilla.org/show_bug.cgi?id=1099429#c0). | ||
| GOOGLE_PLAY_FIREFOX_LINK_UTMS = ( | ||
| GOOGLE_PLAY_FIREFOX_LINK + "&referrer=" + quote("utm_source=www.mozilla.org&utm_medium=referral&utm_campaign=download") | ||
| ) | ||
| GOOGLE_PLAY_FIREFOX_LINK_UTMS = GOOGLE_PLAY_FIREFOX_LINK |
There was a problem hiding this comment.
fix(blocking): The comments above GOOGLE_PLAY_FIREFOX_LINK_UTMS say this constant includes URL-encoded utm_* referrer params, but it’s now just the bare Play Store URL. This is misleading for future changes; either update the comment (and possibly the constant name) to reflect the new behavior, or restore the encoded default referrer at the settings level if that’s still the intended contract.
There was a problem hiding this comment.
Yeah, I saw this too and wondered if we can just now use GOOGLE_PLAY_FIREFOX_LINK wherever we have used GOOGLE_PLAY_FIREFOX_LINK_UTMS in the past?
| if channel != "release": | ||
| product_id = self.store_product_ids.get(channel, "org.mozilla.firefox") | ||
| return self.store_url.replace(self.store_product_ids["release"], product_id) | ||
|
|
||
| return self.store_url | ||
| return ( | ||
| self.store_url | ||
| + "&referrer=" | ||
| + quote(f"utm_source={utm_params['utm_source']}&utm_medium={utm_params['utm_medium']}&utm_campaign={utm_params['utm_campaign']}") | ||
| ) |
There was a problem hiding this comment.
fix(blocking): utm_params are ignored for channel != "release" because the function returns early after swapping the product id. Before this change, non-release links inherited the &referrer=... from store_url; now beta/nightly Play Store links will drop referrer/UTM tracking entirely. Consider appending the encoded referrer for non-release channels too (after replacing the product id), or moving referrer building into a shared path so all store links include it consistently.
There was a problem hiding this comment.
@willdurand Do you want RTAMO for Android to only work for release or is there a need for supporting beta/nightly too, please?
There was a problem hiding this comment.
It wouldn't be a bad idea to support all channels.
There was a problem hiding this comment.
Looking at the entire patch, it does seem like a regression. I think we should probably not early return when the channel isn't release anymore. Instead, we should update self.store_url, and unconditionally return that + the utm params in the referrer query param as shown in the code snippet above.
| @@ -493,8 +501,12 @@ | |||
| if channel != "release": | |||
| product_id = self.store_product_ids.get(channel, "org.mozilla.firefox") | |||
| return self.store_url.replace(self.store_product_ids["release"], product_id) | |||
|
|
|||
| return self.store_url | |||
| return ( | |||
| self.store_url | |||
| + "&referrer=" | |||
| + quote(f"utm_source={utm_params['utm_source']}&utm_medium={utm_params['utm_medium']}&utm_campaign={utm_params['utm_campaign']}") | |||
| ) | |||
There was a problem hiding this comment.
fix(blocking): utm_params defaults are only applied when utm_params is None. If a caller passes a dict/QueryDict missing one of the required keys, the utm_params['utm_source'] / ['utm_medium'] / ['utm_campaign'] indexing will raise a KeyError. To make this API robust, merge defaults with any provided mapping (e.g., use .get(...) for each key or overlay provided values onto a default dict) before constructing the referrer.
| href = link.attr("href") | ||
| assert quote("utm_source=www.mozilla.org") not in href | ||
| assert quote("utm_source=www.test.com") in href | ||
| assert quote("utm_medium=download") not in href |
There was a problem hiding this comment.
fix(blocking): This assertion checks utm_medium=download is not present, but the default medium being overridden in this PR is utm_medium=referral. As written, the test would still pass even if the default utm_medium=referral incorrectly remained in the URL. Update the assertion to ensure the actual default medium value is absent when overridden.
| assert quote("utm_medium=download") not in href | |
| assert quote("utm_medium=referral") not in href |
| @@ -474,6 +481,7 @@ | |||
| instead of Google Play. | |||
| :return: string url | |||
| """ | |||
| utm_params = utm_params or {"utm_source": "www.mozilla.org", "utm_medium": "referral", "utm_campaign": "download"} | |||
| if force_direct: | |||
| # Use a bouncer link | |||
| return "?".join( | |||
| @@ -493,8 +501,12 @@ | |||
| if channel != "release": | |||
| product_id = self.store_product_ids.get(channel, "org.mozilla.firefox") | |||
| return self.store_url.replace(self.store_product_ids["release"], product_id) | |||
|
|
|||
| return self.store_url | |||
| return ( | |||
| self.store_url | |||
| + "&referrer=" | |||
| + quote(f"utm_source={utm_params['utm_source']}&utm_medium={utm_params['utm_medium']}&utm_campaign={utm_params['utm_campaign']}") | |||
| ) | |||
There was a problem hiding this comment.
test(blocking): The new behavior of appending a referrer= parameter (and accepting per-request utm_params) isn’t directly asserted in the existing FirefoxAndroid.get_download_url unit tests (they currently only use startswith(...)). Consider adding focused assertions that (1) release/beta/nightly store URLs include referrer= and the encoded utm_* values, and (2) provided utm_params override defaults as expected, to prevent regressions.
I think it does, right? |
| utm_params = ctx["request"].GET.copy() | ||
| utm_params["utm_source"] = utm_params.get("utm_source", "www.mozilla.org") | ||
| utm_params["utm_medium"] = utm_params.get("utm_medium", "referral") | ||
| utm_params["utm_campaign"] = utm_params.get("utm_campaign", "download") |
There was a problem hiding this comment.
We'd need utm_content here as well.
| version = firefox_android.latest_version(channel) | ||
| builds = android_builds(channel, builds) | ||
|
|
||
| utm_params = ctx["request"].GET.copy() |
There was a problem hiding this comment.
Aren't those query_params and not just the UTM ones? Query params are a bit more than just the UTM params.
|
@bluewave41 This is coming together well - thanks! When testing just now, using the links in the description, I noticed that the @willdurand Can you advise here on what the expected behaviour needs to be, please? EDIT: ah, Will's addressed that in a separate comment |
@willdurand Yes it will, but first we'll get it working on www.m.o before we also support on www.firefox.com because right now RTAMO still goes only to www.m.o. yeah? |
| return ( | ||
| self.store_url | ||
| + "&referrer=" | ||
| + quote(f"utm_source={utm_params['utm_source']}&utm_medium={utm_params['utm_medium']}&utm_campaign={utm_params['utm_campaign']}") |
There was a problem hiding this comment.
I think we shouldn't assume that the utm_params have these keys. We should concatenate all the key/value pairs in utm_params instead.
If this changeset needs to go into the FXC codebase, please add the
WMO and FXClabel.One-line summary
This PR changes auto redirect behavior for Android user agents when redirecting to the app store. UTM parameters are now retained with default values passed if non exist.
Issue / Bugzilla link
https://mozilla-hub.atlassian.net/browse/WT-1010
Testing
utmparameters match in both pages