Put "Recommended by Pocket" feature behind a feature flag

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
major
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: mpopova, Assigned: mcomella)

Tracking

Firefox 57
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified, firefox58 verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

2 years ago
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.
Assignee: nobody → michael.l.comella
Priority: -- → P1
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.
Blocks: 1400945
Flags: needinfo?(mpopova)

Updated

2 years ago
QA Contact: bogdan.surd
Reporter

Comment 2

2 years ago
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!
Flags: needinfo?(mpopova)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8922038 [details]
Bug 1411657: Use resources when getting default Pocket value.

https://reviewboard.mozilla.org/r/193036/#review198288
Attachment #8922038 - Flags: review?(liuche) → review+

Comment 6

2 years ago
mozreview-review
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 :)
Attachment #8922039 - Flags: review?(liuche) → review+

Comment 7

2 years ago
Pushed by michael.l.comella@gmail.com:
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 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
Flags: needinfo?(sledru)
Attachment #8922038 - Flags: approval-mozilla-beta?
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
Attachment #8922038 - Flags: approval-mozilla-beta?
Flags: needinfo?(sledru)
(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.
(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

2 years ago
mozreview-review
Comment on attachment 8922075 [details]
Bug 1411657: Disable testActivityStreamPocketReferrer when Pocket is disabled by default.

https://reviewboard.mozilla.org/r/193058/#review198324
Attachment #8922075 - Flags: review?(liuche) → review+
Comment on attachment 8922038 [details]
Bug 1411657: Use resources when getting default Pocket value.

Approval Request Comment: see comment 8
Attachment #8922038 - Flags: approval-mozilla-beta?
Comment on attachment 8922039 [details]
Bug 1411657: Disable Pocket by default.

Approval Request Comment: see comment 8
Attachment #8922039 - Flags: approval-mozilla-beta?
Comment on attachment 8922075 [details]
Bug 1411657: Disable testActivityStreamPocketReferrer when Pocket is disabled by default.

Approval Request Comment: see comment 8
Attachment #8922075 - Flags: approval-mozilla-beta?
Flags: needinfo?(sledru)

Comment 17

2 years ago
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.
(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.
Attachment #8922075 - Attachment is obsolete: true
Attachment #8922075 - Flags: approval-mozilla-beta?
(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.
(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.
302 to Ritu as she is the master of 57! :)
Flags: needinfo?(sledru) → needinfo?(rkothari)

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4dc6374aca24
https://hg.mozilla.org/mozilla-central/rev/38345cd61a3d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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.
Flags: needinfo?(rkothari)
Comment on attachment 8922038 [details]
Bug 1411657: Use resources when getting default Pocket value.

Must fix, was verified on Nightly, Beta57+
Attachment #8922038 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8922039 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified in Beta as well, everything works as expected. Marking this as verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.