1404460 - Disable Pocket/Top Stories for all but locales from 3 regions (US, Canada, Germany)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Activity Stream, defect, P1)

Description

[Chenxia Liu [:liuche]]

Due to some misunderstanding, we never tracked the work for Pocket being enabled by region. Pocket should only be enabled (top sites section, feed, settings, etc) for 3 regions (US, Canada, Germany), and a specific set of locales from those regions.

US:
en-US
en-GB
en-ZA

Canada:
en-US
en-GB
en-ZA

Germany:
de-DE
de-AT
de-CH
de (this might be a desktop Firefox-specific problem, where some Firefoxes only return "de")

NB: I do not know if we can detect region on Android (though I'd guess it should be possible, based on the Play Store you use because Play Store allows you to release per region I believe).

(Attached in the URL section is additional notes in a Googledoc)

Ideally this is a small change and we can get it done quickly and uplifted into 57. If we need to break up the work, the prioritization is:
- By locale
- By region

NB: Desktop Firefox uses browser.search.region, and can also calculate this (somehow).

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1404460: Only show Pocket stories in specified locales.

Ideally, we'd centralize all queries as to which options are user specified.
However, I wanted to do the smallest change so we can uplift so I filed
bug 1405161 for this centralization.

I opted not to include the "de" locale that is included on desktop because it
does not appear we ever get the "de" locale on Firefox for Android [1].

I tested this patch by changing the system locale between locales with Pocket
on my device (en-US, en-GB, de-DE) and locales without Pocket (ko-KR). The
locale switching system makes this refresh automatically without extra code.

I also intend to test via the in-app locale switcher but that will take time
because I can't do artifact builds with multi-locale so I'm pushing this for
review before I'm finished.

Follow-up changes:
- Add to telemetry
- Hiding the preference in the undesired locales.
- A test for isPocketEnabledByLocaleInner (useful to document how this is
intended to work for locales with variants, different scripts, etc.)

[1]: https://sql.telemetry.mozilla.org/queries/4613#table

Review commit: https://reviewboard.mozilla.org/r/185866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185866/

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1404460: Add POCKET_ENABLED_TO_LOCALE to asUserPrefs telemetry.

After speaking with liuche, we decided it'd be better to add a bit to determine
this rather than combining it with the isPocketEnabled field (which would be
loss of data) or cross-referencing the locale of the submitted event when
checking the Pocket value during telemetry analysis (which is hard to get right
and likely to get out of date).

Review commit: https://reviewboard.mozilla.org/r/185876/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185876/

Comment 3

[Chenxia Liu [:liuche]]

Comment on attachment 8914549 [details]
Bug 1404460: Only show Pocket stories in specified locales.

https://reviewboard.mozilla.org/r/185866/#review190842

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/ActivityStreamPanel.java:115
(Diff revision 1)
>          if (sharedPreferences.getBoolean(PREF_BOOKMARKS_ENABLED, true) || sharedPreferences.getBoolean(PREF_VISITED_ENABLED, true)) {
>              lm.initLoader(LOADER_ID_HIGHLIGHTS, null, new HighlightsCallbacks());
>          }
>  
> -        if (sharedPreferences.getBoolean(PREF_POCKET_ENABLED, true)) {
> +        if (ActivityStreamConfiguration.isPocketEnabledByLocale(getContext()) &&
> +                sharedPreferences.getBoolean(PREF_POCKET_ENABLED, true)) {

Hm, I realize this should fetch the defaults from the Resources bool.xml file, rather than default to true, but that should be done in a follow-up, not here.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8914549 [details]
Bug 1404460: Only show Pocket stories in specified locales.

https://reviewboard.mozilla.org/r/185866/#review190842

> Hm, I realize this should fetch the defaults from the Resources bool.xml file, rather than default to true, but that should be done in a follow-up, not here.

That'd be addressed in the follow-up I filed to centralize all of these calls (and avoid mistakes like this! :): bug 1405161.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1404460: Hide Pocket preference if Pocket not available in locale.

When changing locales, an open dialog will not refresh but clicking on the
"Top sites" preference again (to display the dialog) will show the correct
dialog for the current locale.

Review commit: https://reviewboard.mozilla.org/r/185886/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185886/

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8914558 [details]
Bug 1404460: Add POCKET_ENABLED_TO_LOCALE to asUserPrefs telemetry.

https://reviewboard.mozilla.org/r/185876/#review190850

::: mobile/android/base/java/org/mozilla/gecko/activitystream/ActivityStreamTelemetry.java:86
(Diff revision 1)
>       */
> -    public static int getASUserPreferencesValue(final SharedPreferences sharedPreferences, final Resources res) {
> +    public static int getASUserPreferencesValue(final Context context, final SharedPreferences sharedPreferences) {
> +        final Resources res = context.getResources();
>          int bitPackedPrefValue = 0;
>  
> -        // todo: Add bit for isPocketAllowedByLocale.
> +        if (ActivityStreamConfiguration.isPocketEnabledByLocale(context)) {

Nit: add this line to the bottom of the list of checks so it matches the declaration order.

Comment 7

[Chenxia Liu [:liuche]]

Comment on attachment 8914561 [details]
Bug 1404460: Hide Pocket preference if Pocket not available in locale.

https://reviewboard.mozilla.org/r/185886/#review190854

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1404460: Add test for pocket locale enabling.

Review commit: https://reviewboard.mozilla.org/r/185912/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185912/

Comment 9

[Chenxia Liu [:liuche]]

Comment on attachment 8914576 [details]
Bug 1404460: Add test for pocket locale enabling.

https://reviewboard.mozilla.org/r/185912/#review190860

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/141096343631
Only show Pocket stories in specified locales. r=liuche
https://hg.mozilla.org/integration/autoland/rev/bc59c8db7297
Add POCKET_ENABLED_TO_LOCALE to asUserPrefs telemetry. r=liuche
https://hg.mozilla.org/integration/autoland/rev/bb2d7ca31f69
Hide Pocket preference if Pocket not available in locale. r=liuche
https://hg.mozilla.org/integration/autoland/rev/747862bc5edb
Add test for pocket locale enabling. r=liuche

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8914549 [details]
Bug 1404460: Only show Pocket stories in specified locales.

Approval Request Comment
[Feature/Bug causing the regression]: Activity stream & its "Recommended by Pocket" section: there was a miscommunication about who would receive this section.
[User impact if declined]: Users in all locales will receive "Recommended by Pocket" stories, even though it was only intended for a limited audience. If there are no recommended articles for a given locale (which is most of them), it will default to en-US so these locales will be subjected to english articles about the US (e.g. NFL!) which is unreadable (due to language barriers), annoyingly irrelevant, and possibly offensive. :)

[Is this code covered by automated tests?]: A small portion: the actual locale matching.
[Has the fix been verified in Nightly?]: No - local. Note that I've only tested system locales at the moment and will create a multi-locale build tomorrow to verify fennec locales, but I expect for them to work similarly.
[Needs manual test from QE? If yes, steps to reproduce]: Ideally - Bogdan could verify. There are two STRs:
0) Go to Top Sites on en-US, verify Pocket recommendations appear
1a) Go to the browser settings and change the browser language to a non-whitelisted locale (see below)
1b) Go to Android system settings and change the system language to a non-whitelisted locale (see below)
2) Hit back to return to Top Sites, verify Pocket recommendations do not appear

Similarly, we should verify the whitelisted locales all have Pocket recommendations appear. The whitelisted locales are available in comment 0. Note that we chose to ignore region for simplicity and we don't support "de" on Android (reasoning in one of the commit summaries).

[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: The majority of the code we add is the function to calculate whether or not Pocket is enabled for the given locale. It's fairly simple, covered by tests, and its mostly initialization code so should fail during start-up or compile-time. Then we branch to existing code in a few places on this method to 1) hide or show pocket, 2) send this value in telemetry, and 3) hide the preference – these changes are simple and generally follow a pattern from a similar condition we branch on (has user enabled pocket?).

[String changes made/needed]: No

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8914558 [details]
Bug 1404460: Add POCKET_ENABLED_TO_LOCALE to asUserPrefs telemetry.

Approval Request Comment: see comment 11.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8914561 [details]
Bug 1404460: Hide Pocket preference if Pocket not available in locale.

Approval Request Comment: see comment 11.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8914576 [details]
Bug 1404460: Add test for pocket locale enabling.

Approval Request Comment: see comment 11.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/141096343631
https://hg.mozilla.org/mozilla-central/rev/bc59c8db7297
https://hg.mozilla.org/mozilla-central/rev/bb2d7ca31f69
https://hg.mozilla.org/mozilla-central/rev/747862bc5edb

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Multi-locale builds provide the locale, "de" for deutsch so Pocket incorrectly does not show for them. I filed bug 1405404.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

IMPORTANT FOR UPLIFT: bug 1405404 should be uplifted with this.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #1)
> I also intend to test via the in-app locale switcher but that will take time
> because I can't do artifact builds with multi-locale so I'm pushing this for
> review before I'm finished.

To be explicit, I did this and the only issue I discovered was the one for the "de" locale in comment 16.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8914549 [details]
Bug 1404460: Only show Pocket stories in specified locales.

Must fix for 57, Beta57+

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/416989ade5b5
https://hg.mozilla.org/releases/mozilla-beta/rev/bc71eb47ec4e
https://hg.mozilla.org/releases/mozilla-beta/rev/5f3019ce2cfa
https://hg.mozilla.org/releases/mozilla-beta/rev/ab96b1b8fe40

Comment 21

[Bogdan Surd, QA [:bsurd, NI]]

Devices:
 - Nexus 5 (Android 6.0.1);
 - Samsung Galaxy Tab 3	(Android 7);
 - Google Pixel (Android 8).

Verified this by following instructions in comment 11 and Pocket stories are not displayed for non-whitelisted locales and are available for all of the whitelisted ones. Will verify de (multi locale build) in bug 1405404.