1405404 - Pocket recommendations do not show in Deutsch multi-locale builds

Status: VERIFIED FIXED

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

Description

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

I didn't implement this initially in bug 1404460: "de" is used for multi-locale builds but cannot be selected by Android so I thought it was unnecessary. We should add it to the whitelist.

Comment 1

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

Attachment: Bug 1405404: Add 'de' to list of pocket whitelisted locales.

This is used in multi-locale builds.

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

Comment 2

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

Comment on attachment 8914839 [details]
Bug 1405404: Add 'de' to list of pocket whitelisted locales.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186110/diff/1-2/

Comment 3

[Chenxia Liu [:liuche]]

Comment on attachment 8914839 [details]
Bug 1405404: Add 'de' to list of pocket whitelisted locales.

https://reviewboard.mozilla.org/r/186110/#review191134

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/ActivityStreamConfiguration.java:26
(Diff revision 2)
>   */
>  public class ActivityStreamConfiguration {
>  
>      private static final Set<Locale> pocketEnabledLocales;
>      @VisibleForTesting static final String[] pocketEnabledLocaleTags = new String[] {
>              // Sorted alphabetically to preserve blame for additions/removals.

I wonder if we should document here that new locales being added should be cross-referenced with both Fennec locale and Android locale.

Comment 4

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

Comment on attachment 8914839 [details]
Bug 1405404: Add 'de' to list of pocket whitelisted locales.

https://reviewboard.mozilla.org/r/186110/#review191134

> I wonder if we should document here that new locales being added should be cross-referenced with both Fennec locale and Android locale.

I added a note.

Comment 5

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

Comment on attachment 8914839 [details]
Bug 1405404: Add 'de' to list of pocket whitelisted locales.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186110/diff/2-3/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0bf07287d614
Add 'de' to list of pocket whitelisted locales. r=liuche

Comment 7

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

Comment on attachment 8914839 [details]
Bug 1405404: Add 'de' to list of pocket whitelisted locales.

This must be uplifted in unison with bug 1404460 (and should land after).

Approval Request Comment
[Feature/Bug causing the regression]: bug 1404460.
[User impact if declined]: Users using the Deutsch language override in Fennec settings will not get Pocket stories.
[Is this code covered by automated tests?]: Lightly (we ensure the locale says that Pocket should be enabled, as expected).
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: Ideally, see bug 1404460 comment 11.
[List of other uplifts needed for the feature/fix]: bug 1404460 (before this).
[Is the change risky?]: Trivial.
[Why is the change risky/not risky?]: We add a one locale, "de", to an existing list of whitelisted locales so assuming the current code works, there should be no problems.
[String changes made/needed]: None

Comment 8

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

This was backed out for test failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0bf07287d61448f027f4f8de2e587a634bb70ec8&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable

It seems my test code assumed all locales were specified as "x-y" but I added an "x".

Comment 9

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

Comment on attachment 8914839 [details]
Bug 1405404: Add 'de' to list of pocket whitelisted locales.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186110/diff/3-4/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0db210721aa4
Add 'de' to list of pocket whitelisted locales. r=liuche

Comment 11

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

Note: I didn't have an opportunity to test this test change because multi-locale builds are breaking gradle builds but I'm confident in the change.

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc879542e9ae
Backed out changeset 0bf07287d614 for failing android-test. r=backout

Comment 13

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

^ is the backout I described in comment 8, which occurred prior to my relanding in comment 10.

(In reply to Michael Comella (:mcomella) from comment #11)
> Note: I didn't have an opportunity to test this test change because
> multi-locale builds are breaking gradle builds but I'm confident in the
> change.

I tested locally: it passed. (I blew out my multi-locale build for the gradle build)

Comment 14

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

Comment on attachment 8914839 [details]
Bug 1405404: Add 'de' to list of pocket whitelisted locales.

This is needed for Bug 1404460, Beta57+

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6e3b901aec51

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/0db210721aa4

Comment 17

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

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

Verified this on multiple devices and pocket stories are being displayed when changing to Deutsch.