Bug 1446276 - Separate pref for user set topstories and system set topstories. r=gvn,k88hudson
☠☠ backed out by d6b6d63e601d ☠ ☠
authorScott <scott.downe@gmail.com>
Mon, 25 May 2020 19:11:48 +0000
changeset 532112 dd35edffc6dffb7ae6a4050b955c6e7855cdffcd
parent 532111 da2c7b0ac9a44e44000a6be1cea9d753c33784d4
child 532113 2383139c85c0a60504a871530d099f96eff9e59d
push id117001
push usersdowne@getpocket.com
push dateMon, 25 May 2020 23:15:54 +0000
treeherderautoland@dd35edffc6df [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgvn, k88hudson
bugs1446276
milestone78.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1446276 - Separate pref for user set topstories and system set topstories. r=gvn,k88hudson Differential Revision: https://phabricator.services.mozilla.com/D75217
browser/components/newtab/content-src/components/Base/Base.jsx
browser/components/newtab/content-src/lib/selectLayoutRender.js
browser/components/newtab/data/content/activity-stream.bundle.js
browser/components/newtab/lib/ActivityStream.jsm
browser/components/newtab/test/unit/content-src/components/DiscoveryStreamBase.test.jsx
browser/components/newtab/test/unit/content-src/lib/selectLayoutRender.test.js
browser/components/newtab/test/unit/lib/ActivityStream.test.js
--- a/browser/components/newtab/content-src/components/Base/Base.jsx
+++ b/browser/components/newtab/content-src/components/Base/Base.jsx
@@ -127,17 +127,18 @@ export class BaseContent extends React.P
     const { App } = props;
     const { initialized } = App;
     const prefs = props.Prefs.values;
 
     const isDiscoveryStream =
       props.DiscoveryStream.config && props.DiscoveryStream.config.enabled;
     let filteredSections = props.Sections;
 
-    const pocketEnabled = prefs["feeds.section.topstories"];
+    const pocketEnabled =
+      prefs["feeds.section.topstories"] && prefs["feeds.system.topstories"];
     const noSectionsEnabled =
       !prefs["feeds.topsites"] &&
       filteredSections.filter(section => section.enabled).length === 0;
     const searchHandoffEnabled = prefs["improvesearch.handoffToAwesomebar"];
 
     const outerClassName = [
       "outer-wrapper",
       isDiscoveryStream && pocketEnabled && "ds-outer-wrapper-search-alignment",
--- a/browser/components/newtab/content-src/lib/selectLayoutRender.js
+++ b/browser/components/newtab/content-src/lib/selectLayoutRender.js
@@ -69,17 +69,19 @@ export const selectLayoutRender = ({
   if (!prefs["feeds.topsites"]) {
     filterArray.push("TopSites");
   }
 
   if (!locale.startsWith("en-")) {
     filterArray.push("Navigation");
   }
 
-  if (!prefs["feeds.section.topstories"]) {
+  const pocketEnabled =
+    prefs["feeds.section.topstories"] && prefs["feeds.system.topstories"];
+  if (!pocketEnabled) {
     filterArray.push(...DS_COMPONENTS);
   }
 
   const placeholderComponent = component => {
     if (!component.feed) {
       // TODO we now need a placeholder for topsites and textPromo.
       return {
         ...component,
--- a/browser/components/newtab/data/content/activity-stream.bundle.js
+++ b/browser/components/newtab/data/content/activity-stream.bundle.js
@@ -738,17 +738,17 @@ class BaseContent extends react__WEBPACK
       App
     } = props;
     const {
       initialized
     } = App;
     const prefs = props.Prefs.values;
     const isDiscoveryStream = props.DiscoveryStream.config && props.DiscoveryStream.config.enabled;
     let filteredSections = props.Sections;
-    const pocketEnabled = prefs["feeds.section.topstories"];
+    const pocketEnabled = prefs["feeds.section.topstories"] && prefs["feeds.system.topstories"];
     const noSectionsEnabled = !prefs["feeds.topsites"] && filteredSections.filter(section => section.enabled).length === 0;
     const searchHandoffEnabled = prefs["improvesearch.handoffToAwesomebar"];
     const outerClassName = ["outer-wrapper", isDiscoveryStream && pocketEnabled && "ds-outer-wrapper-search-alignment", isDiscoveryStream && "ds-outer-wrapper-breakpoint-override", prefs.showSearch && this.state.fixedSearch && !noSectionsEnabled && "fixed-search", prefs.showSearch && noSectionsEnabled && "only-search"].filter(v => v).join(" ");
     return react__WEBPACK_IMPORTED_MODULE_7___default.a.createElement("div", null, react__WEBPACK_IMPORTED_MODULE_7___default.a.createElement("div", {
       className: outerClassName
     }, react__WEBPACK_IMPORTED_MODULE_7___default.a.createElement("main", null, prefs.showSearch && react__WEBPACK_IMPORTED_MODULE_7___default.a.createElement("div", {
       className: "non-collapsible-section"
     }, react__WEBPACK_IMPORTED_MODULE_7___default.a.createElement(content_src_components_ErrorBoundary_ErrorBoundary__WEBPACK_IMPORTED_MODULE_6__["ErrorBoundary"], null, react__WEBPACK_IMPORTED_MODULE_7___default.a.createElement(content_src_components_Search_Search__WEBPACK_IMPORTED_MODULE_8__["Search"], _extends({
@@ -10704,17 +10704,19 @@ const selectLayoutRender = ({
   if (!prefs["feeds.topsites"]) {
     filterArray.push("TopSites");
   }
 
   if (!locale.startsWith("en-")) {
     filterArray.push("Navigation");
   }
 
-  if (!prefs["feeds.section.topstories"]) {
+  const pocketEnabled = prefs["feeds.section.topstories"] && prefs["feeds.system.topstories"];
+
+  if (!pocketEnabled) {
     filterArray.push(...DS_COMPONENTS);
   }
 
   const placeholderComponent = component => {
     if (!component.feed) {
       // TODO we now need a placeholder for topsites and textPromo.
       return { ...component,
         data: {
--- a/browser/components/newtab/lib/ActivityStream.jsm
+++ b/browser/components/newtab/lib/ActivityStream.jsm
@@ -168,17 +168,17 @@ const PREFS_CONFIG = new Map([
     "feeds.section.topstories.options",
     {
       title: "Configuration options for top stories feed",
       // This is a dynamic pref as it depends on the feed being shown or not
       getValue: args =>
         JSON.stringify({
           api_key_pref: "extensions.pocket.oAuthConsumerKey",
           // Use the opposite value as what default value the feed would have used
-          hidden: !PREFS_CONFIG.get("feeds.section.topstories").getValue(args),
+          hidden: !PREFS_CONFIG.get("feeds.system.topstories").getValue(args),
           provider_icon: "pocket",
           provider_name: "Pocket",
           read_more_endpoint:
             "https://getpocket.com/explore/trending?src=fx_new_tab",
           stories_endpoint: `https://getpocket.cdn.mozilla.net/v3/firefox/global-recs?version=3&consumer_key=$apiKey&locale_lang=${
             args.locale
           }&feed_variant=${
             showSpocs(args) ? "default_spocs_on" : "default_spocs_off"
@@ -370,16 +370,24 @@ const PREFS_CONFIG = new Map([
   [
     "section.topstories.rows",
     {
       title: "Number of rows of Top Stories to display",
       value: 1,
     },
   ],
   [
+    "feeds.section.topstories",
+    {
+      title:
+        "User pref to show stories on newtab (feeds.system.topstories has to be set to true as well)",
+      value: true,
+    },
+  ],
+  [
     "sectionOrder",
     {
       title: "The rendering order for the sections",
       value: "topsites,topstories,highlights",
     },
   ],
   [
     "improvesearch.noDefaultSearchTile",
@@ -596,21 +604,21 @@ const FEEDS_DATA = [
   },
   {
     name: "section.highlights",
     factory: () => new HighlightsFeed(),
     title: "Fetches content recommendations from places db",
     value: true,
   },
   {
-    name: "section.topstories",
+    name: "system.topstories",
     factory: () =>
       new TopStoriesFeed(PREFS_CONFIG.get("discoverystream.config")),
     title:
-      "Fetches content recommendations from a configurable content provider",
+      "System pref that fetches content recommendations from a configurable content provider",
     // Dynamically determine if Pocket should be shown for a geo / locale
     getValue: ({ geo, locale }) => {
       const preffedRegionsString =
         Services.prefs.getStringPref(REGION_STORIES_CONFIG) || "";
       const preffedRegions = preffedRegionsString.split(",").map(s => s.trim());
       const locales = {
         US: ["en-CA", "en-GB", "en-US"],
         CA: ["en-CA", "en-GB", "en-US"],
--- a/browser/components/newtab/test/unit/content-src/components/DiscoveryStreamBase.test.jsx
+++ b/browser/components/newtab/test/unit/content-src/components/DiscoveryStreamBase.test.jsx
@@ -102,16 +102,17 @@ describe("<DiscoveryStreamBase>", () => 
     };
     return shallow(
       <DiscoveryStreamBase
         locale="en-US"
         DiscoveryStream={defaultProps}
         Prefs={{
           values: {
             "feeds.section.topstories": true,
+            "feeds.system.topstories": true,
             "feeds.topsites": true,
           },
         }}
         document={{
           documentElement: { lang: "en-US" },
         }}
         Sections={[
           {
--- a/browser/components/newtab/test/unit/content-src/lib/selectLayoutRender.test.js
+++ b/browser/components/newtab/test/unit/content-src/lib/selectLayoutRender.test.js
@@ -926,16 +926,17 @@ describe("selectLayoutRender", () => {
       type: at.DISCOVERY_STREAM_LAYOUT_UPDATE,
       data: { layout: fakeLayout },
     });
 
     const { layoutRender } = selectLayoutRender({
       state: store.getState().DiscoveryStream,
       prefs: {
         "feeds.topsites": true,
+        "feeds.system.topstories": true,
         "feeds.section.topstories": true,
       },
     });
 
     assert.equal(layoutRender[0].components[0].type, "Message");
     assert.equal(layoutRender[0].components[1].type, "TopSites");
     assert.equal(layoutRender[0].components[2], undefined);
   });
--- a/browser/components/newtab/test/unit/lib/ActivityStream.test.js
+++ b/browser/components/newtab/test/unit/lib/ActivityStream.test.js
@@ -116,28 +116,23 @@ describe("ActivityStream", () => {
     it("should create a Telemetry feed", () => {
       const feed = as.feeds.get("feeds.telemetry")();
       assert.instanceOf(feed, Fake);
     });
     it("should create a Prefs feed", () => {
       const feed = as.feeds.get("feeds.prefs")();
       assert.instanceOf(feed, Fake);
     });
-    it("should create a section feed for each section in PREFS_CONFIG", () => {
-      // If new sections are added, their feeds will have to be added to the
-      // list of injected feeds above for this test to pass
-      let feedCount = 0;
-      for (const pref of PREFS_CONFIG.keys()) {
-        if (pref.search(/^feeds\.section\.[^.]+$/) === 0) {
-          const feed = as.feeds.get(pref)();
-          assert.instanceOf(feed, Fake);
-          feedCount++;
-        }
-      }
-      assert.isAbove(feedCount, 0);
+    it("should create a HighlightsFeed feed", () => {
+      const feed = as.feeds.get("feeds.section.highlights")();
+      assert.instanceOf(feed, Fake);
+    });
+    it("should create a TopStoriesFeed feed", () => {
+      const feed = as.feeds.get("feeds.system.topstories")();
+      assert.instanceOf(feed, Fake);
     });
     it("should create a AboutPreferences feed", () => {
       const feed = as.feeds.get("feeds.aboutpreferences")();
       assert.instanceOf(feed, Fake);
     });
     it("should create a SectionsFeed", () => {
       const feed = as.feeds.get("feeds.sections")();
       assert.instanceOf(feed, Fake);
@@ -336,61 +331,61 @@ describe("ActivityStream", () => {
     });
     it("should be false with no geo/locale", () => {
       prefHasUserValueStub.returns(false);
       appLocaleAsBCP47Stub.get(() => "");
       getStringPrefStub.withArgs("browser.search.region").returns("");
 
       as._updateDynamicPrefs();
 
-      assert.isFalse(PREFS_CONFIG.get("feeds.section.topstories").value);
+      assert.isFalse(PREFS_CONFIG.get("feeds.system.topstories").value);
     });
     it("should be false with unexpected geo", () => {
       getStringPrefStub.withArgs("browser.search.region").returns("NOGEO");
 
       as._updateDynamicPrefs();
 
-      assert.isFalse(PREFS_CONFIG.get("feeds.section.topstories").value);
+      assert.isFalse(PREFS_CONFIG.get("feeds.system.topstories").value);
     });
     it("should be false with expected geo and unexpected locale", () => {
       appLocaleAsBCP47Stub.get(() => "no-LOCALE");
 
       as._updateDynamicPrefs();
 
-      assert.isFalse(PREFS_CONFIG.get("feeds.section.topstories").value);
+      assert.isFalse(PREFS_CONFIG.get("feeds.system.topstories").value);
     });
     it("should be true with expected geo and locale", () => {
       as._updateDynamicPrefs();
-      assert.isTrue(PREFS_CONFIG.get("feeds.section.topstories").value);
+      assert.isTrue(PREFS_CONFIG.get("feeds.system.topstories").value);
     });
     it("should be false after expected geo and locale then unexpected", () => {
       getStringPrefStub
         .withArgs("browser.search.region")
         .onFirstCall()
         .returns("US")
         .onSecondCall()
         .returns("NOGEO");
 
       as._updateDynamicPrefs();
       as._updateDynamicPrefs();
 
-      assert.isFalse(PREFS_CONFIG.get("feeds.section.topstories").value);
+      assert.isFalse(PREFS_CONFIG.get("feeds.system.topstories").value);
     });
     it("should be true with updated pref change", () => {
       appLocaleAsBCP47Stub.get(() => "en-GB");
       getStringPrefStub.withArgs("browser.search.region").returns("GB");
       getStringPrefStub
         .withArgs(
           "browser.newtabpage.activity-stream.discoverystream.region-stories-config"
         )
         .returns("US,CA,GB");
 
       as._updateDynamicPrefs();
 
-      assert.isTrue(PREFS_CONFIG.get("feeds.section.topstories").value);
+      assert.isTrue(PREFS_CONFIG.get("feeds.system.topstories").value);
     });
   });
   describe("_updateDynamicPrefs topstories delayed default value", () => {
     let clock;
     beforeEach(() => {
       clock = sinon.useFakeTimers();
 
       // Have addObserver cause prefHasUserValue to now return true then observe
@@ -404,40 +399,40 @@ describe("ActivityStream", () => {
     afterEach(() => clock.restore());
 
     it("should set false with unexpected geo", () => {
       sandbox.stub(global.Services.prefs, "getStringPref").returns("NOGEO");
 
       as._updateDynamicPrefs();
       clock.tick(1);
 
-      assert.isFalse(PREFS_CONFIG.get("feeds.section.topstories").value);
+      assert.isFalse(PREFS_CONFIG.get("feeds.system.topstories").value);
     });
     it("should set true with expected geo and locale", () => {
       sandbox.stub(global.Services.prefs, "getStringPref").returns("US");
       sandbox
         .stub(global.Services.locale, "appLocaleAsBCP47")
         .get(() => "en-US");
 
       as._updateDynamicPrefs();
       clock.tick(1);
 
-      assert.isTrue(PREFS_CONFIG.get("feeds.section.topstories").value);
+      assert.isTrue(PREFS_CONFIG.get("feeds.system.topstories").value);
     });
     it("should not change default even with expected geo and locale", () => {
-      as._defaultPrefs.set("feeds.section.topstories", false);
+      as._defaultPrefs.set("feeds.system.topstories", false);
       sandbox.stub(global.Services.prefs, "getStringPref").returns("US");
       sandbox
         .stub(global.Services.locale, "appLocaleAsBCP47")
         .get(() => "en-US");
 
       as._updateDynamicPrefs();
       clock.tick(1);
 
-      assert.isFalse(PREFS_CONFIG.get("feeds.section.topstories").value);
+      assert.isFalse(PREFS_CONFIG.get("feeds.system.topstories").value);
     });
   });
   describe("telemetry reporting on init failure", () => {
     it("should send a ping on init error", () => {
       as = new ActivityStream();
       const telemetry = { handleUndesiredEvent: sandbox.spy() };
       sandbox.stub(as.store, "init").throws();
       sandbox.stub(as.store.feeds, "get").returns(telemetry);