1411657 - Put "Recommended by Pocket" feature behind a feature flag

Status: VERIFIED FIXED

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

Description

[Maria Popova]

Activity Stream may be pushed to a later release on desktop.  With that, we want the Pocket stories feature to be put behind a feature flag on Firefox Android,so we can launch the feature across all platforms simultaneously.

No changes are needed to settings, which control what sections to be displayed on New Tab.

Comment 1

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

Maria, from what I understand, I think there are two approaches here (my time estimates preface the lines):
- 1 hour: Disable Pocket by default but leave it in Settings to allow users to re-enable it (very simple)
- 2 hours: Disable Pocket entirely (slightly more complicated but probably still straightforward)

Which do you prefer?

Also, we'll need to update the SUMO page (from bug 1400945) accordingly.

Comment 2

[Maria Popova]

Hi Mike,  please get option 1 ready.  We DON'T need to uplift that change yet, but need to make it available for QA to test.  Thanks!

Comment 3

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

Attachment: Bug 1411657: Use resources when getting default Pocket value.

This is an existing bug that previously had no impact because the values were
duplicated. Now that I'm going to change the value, the bug has to be fixed.

I opted not to fix the bug for the other top sites features in order to
minimize the scope of this change.

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

Comment 4

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

Attachment: Bug 1411657: Disable Pocket by default.

From product (bug 1411657 comment 0):

Activity Stream may be pushed to a later release on desktop.  With that, we want
the Pocket stories feature to be put behind a feature flag on Firefox Android,so
we can launch the feature across all platforms simultaneously.

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

Comment 5

[Chenxia Liu [:liuche]]

Comment on attachment 8922038 [details]
Bug 1411657: Use resources when getting default Pocket value.

https://reviewboard.mozilla.org/r/193036/#review198288

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8922039 [details]
Bug 1411657: Disable Pocket by default.

https://reviewboard.mozilla.org/r/193046/#review198290

I'm glad we're using bool.xml :)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4dc6374aca24
Use resources when getting default Pocket value. r=liuche
https://hg.mozilla.org/integration/autoland/rev/38345cd61a3d
Disable Pocket by default. r=liuche

Comment 8

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

Comment on attachment 8922038 [details]
Bug 1411657: Use resources when getting default Pocket value.

NI :sylvestre to make sure this happens.

Approval Request Comment
[Feature/Bug causing the regression]: See comment 0: product may want to hold back Activity Stream, we're tentatively disabling it here.

[User impact if declined]: Users will get AS even though Product does not want them too.

[Is this code covered by automated tests?]: We have a tangential test: testActivityStreamPocketReferrer, which tests that clicked Pocket items have the proper referrer.
[Has the fix been verified in Nightly?]: No, tested locally.
[Needs manual test from QE? If yes, steps to reproduce]: Bogdan can verify.
1) Launch fennec on a clean build (expected: Pocket is disabled)
2) Go to AS home panel settings (expected: Pocket option is disabled)
3) Enable Pocket, return to homescreen (expected: Pocket is enabled)
4) Returns to settings, disable Pocket, return to homescreen (expected: Pocket is disabled)

[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal.
[Why is the change risky/not risky?]: The primary change is making the constant for whether Pocket appears by default return "false" rather than "true". However, there was an existing bug where we used a hard-coded true, rather than pulling from resources, so the first patch adds the code to pull the constant from resources. It's pretty simple and we do it in other places. The one potential problem it could have is that we're getting a context from the framework in a place we didn't before and generally the context is not always non-null, but views should always have a context and this should never be null.

[String changes made/needed]: None

Comment 9

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

Comment on attachment 8922038 [details]
Bug 1411657: Use resources when getting default Pocket value.

My push is going to fail because I forgot to update testActivityStreamPocketReferrer.

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=38345cd61a3dd70fafc8e8bb3128b0ac8eb6ac41

Comment 10

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

(In reply to Michael Comella (:mcomella) from comment #9)
> My push is going to fail because I forgot to update
> testActivityStreamPocketReferrer.

It only ran rc4 so maybe not. But we need this upcoming fix.

Comment 11

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

Attachment: Bug 1411657: Disable testActivityStreamPocketReferrer when Pocket is disabled by default.

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

Comment 12

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

(In reply to Michael Comella (:mcomella) from comment #10)
> (In reply to Michael Comella (:mcomella) from comment #9)
> > My push is going to fail because I forgot to update
> > testActivityStreamPocketReferrer.
> 
> It only ran rc4 so maybe not.

rc3 in the job after mine failed: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=184c3c90cf23323a822f979d4cff1e10d629e245&selectedJob=139610064

Comment 13

[Chenxia Liu [:liuche]]

Comment on attachment 8922075 [details]
Bug 1411657: Disable testActivityStreamPocketReferrer when Pocket is disabled by default.

https://reviewboard.mozilla.org/r/193058/#review198324

Comment 14

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

Comment on attachment 8922038 [details]
Bug 1411657: Use resources when getting default Pocket value.

Approval Request Comment: see comment 8

Comment 15

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

Comment on attachment 8922039 [details]
Bug 1411657: Disable Pocket by default.

Approval Request Comment: see comment 8

Comment 16

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

Comment on attachment 8922075 [details]
Bug 1411657: Disable testActivityStreamPocketReferrer when Pocket is disabled by default.

Approval Request Comment: see comment 8

Comment 17

[Mozilla Autoland]

We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.

Comment 18

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

(In reply to Mozilla Autoland from comment #17)
> We're sorry - something has gone wrong while rewriting or rebasing your
> commits. The commits being pushed no longer match what was requested. Please
> file a bug.

I tried landing all three commits via autoland but it didn't work; I guess I have to wait for the backout or file a new bug.

Comment 19

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

(In reply to Michael Comella (:mcomella) from comment #18)
> (In reply to Mozilla Autoland from comment #17)
> > We're sorry - something has gone wrong while rewriting or rebasing your
> > commits. The commits being pushed no longer match what was requested. Please
> > file a bug.
> 
> I tried landing all three commits via autoland but it didn't work; I guess I
> have to wait for the backout or file a new bug.

Since no one was around to back out my change, I moved the final commit to bug 1411749.

Comment 20

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

(In reply to Michael Comella (:mcomella) from comment #8)
> [List of other uplifts needed for the feature/fix]: None

Actually, please uplift bug 1411749 after this one! It fixes the test.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

302 to Ritu as she is the master of 57! :)

Comment 22

[Attila Craciun [:arny]]

https://hg.mozilla.org/mozilla-central/rev/4dc6374aca24
https://hg.mozilla.org/mozilla-central/rev/38345cd61a3d

Comment 23

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

Tested this in Nightly following instructions in Comment 8. Everything works as expected, no new issues have been created. Marking this as verified for Nightly.

Comment 24

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

Comment on attachment 8922038 [details]
Bug 1411657: Use resources when getting default Pocket value.

Must fix, was verified on Nightly, Beta57+

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f99471c1c261
https://hg.mozilla.org/releases/mozilla-beta/rev/ae4b19c61dc7

Comment 26

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

Verified in Beta as well, everything works as expected. Marking this as verified.