Bug 1516041 - Clear cache if pocket newtab endpoint changes (#4632)

Also fixes issue where cache time check was reversed, adds test

lib/DiscoveryStreamFeed.jsm

@@ -84,7 +84,7 @@ this.DiscoveryStreamFeed = class DiscoveryStreamFeed {
     const cachedData = await this.cache.get() || {};
 
     let {layout: layoutResponse} = cachedData;
-    if (!layoutResponse || !(Date.now() - layoutResponse._timestamp > LAYOUT_UPDATE_TIME)) {
+    if (!layoutResponse || !(Date.now() - layoutResponse._timestamp < LAYOUT_UPDATE_TIME)) {
       layoutResponse = await this.fetchLayout();
       if (layoutResponse && layoutResponse.layout) {
         layoutResponse._timestamp = Date.now();
@@ -105,16 +105,21 @@ this.DiscoveryStreamFeed = class DiscoveryStreamFeed {
   }
 
   async disable() {
-    // Clear cache
-    await this.cache.set("layout", {});
+    await this.clearCache();
     // Reset reducer
     this.store.dispatch(ac.BroadcastToContent({type: at.DISCOVERY_STREAM_LAYOUT_UPDATE, data: []}));
     this.loaded = false;
   }
 
+  async clearCache() {
+    await this.cache.set("layout", {});
+  }
+
   async onPrefChange() {
-    // Load data from all endpoints if our config.enabled = true.
     if (this.config.enabled) {
+      // We always want to clear the cache if the pref has changed
+      await this.clearCache();
+      // Load data from all endpoints
       await this.enable();
     }
 

test/unit/lib/DiscoveryStreamFeed.test.js

@@ -4,12 +4,14 @@ import {DiscoveryStreamFeed} from "lib/DiscoveryStreamFeed.jsm";
 import {reducers} from "common/Reducers.jsm";
 
 const CONFIG_PREF_NAME = "browser.newtabpage.activity-stream.discoverystream.config";
+const THIRTY_MINUTES = 30 * 60 * 1000;
 
 describe("DiscoveryStreamFeed", () => {
   let feed;
   let sandbox;
   let configPrefStub;
   let fetchStub;
+  let clock;
 
   beforeEach(() => {
     // Pref
@@ -21,12 +23,16 @@ describe("DiscoveryStreamFeed", () => {
     // Fetch
     fetchStub = sandbox.stub(global, "fetch");
 
+    // Time
+    clock = sinon.useFakeTimers();
+
     // Feed
     feed = new DiscoveryStreamFeed();
     feed.store = createStore(combineReducers(reducers));
   });
 
   afterEach(() => {
+    clock.restore();
     sandbox.restore();
   });
 
@@ -54,6 +60,44 @@ describe("DiscoveryStreamFeed", () => {
       assert.calledOnce(fetchStub);
       assert.calledWith(feed.cache.set, "layout", resp);
     });
+    it("should fetch data and populate the cache if the cached data is older than 30 mins", async () => {
+      const resp = {layout: ["foo", "bar"]};
+      const fakeCache = {layout: {layout: ["hello"], _timestamp: Date.now()}};
+
+      sandbox.stub(feed.cache, "get").returns(Promise.resolve(fakeCache));
+      sandbox.stub(feed.cache, "set").returns(Promise.resolve());
+
+      fetchStub.resolves({ok: true, json: () => Promise.resolve(resp)});
+
+      clock.tick(THIRTY_MINUTES + 1);
+      await feed.loadCachedData();
+
+      assert.calledOnce(fetchStub);
+      assert.calledWith(feed.cache.set, "layout", resp);
+    });
+    it("should use the cached data and not fetch if the cached data is less than 30 mins old", async () => {
+      const fakeCache = {layout: {layout: ["hello"], _timestamp: Date.now()}};
+
+      sandbox.stub(feed.cache, "get").returns(Promise.resolve(fakeCache));
+      sandbox.stub(feed.cache, "set").returns(Promise.resolve());
+
+      clock.tick(THIRTY_MINUTES - 1);
+      await feed.loadCachedData();
+
+      assert.notCalled(fetchStub);
+      assert.notCalled(feed.cache.set);
+    });
+  });
+
+  describe("#clearCache", () => {
+    it("should set .layout to {}", async () => {
+      sandbox.stub(feed.cache, "set").returns(Promise.resolve());
+
+      await feed.clearCache();
+
+      assert.calledOnce(feed.cache.set);
+      assert.calledWith(feed.cache.set, "layout", {});
+    });
   });
 
   describe("#onAction: INIT", () => {
@@ -72,6 +116,7 @@ describe("DiscoveryStreamFeed", () => {
       assert.isTrue(feed.loaded);
     });
   });
+
   describe("#onAction: DISCOVERY_STREAM_CONFIG_CHANGE", () => {
     it("should call this.loadCachedData if config.enabled changes to true ", async () => {
       sandbox.stub(feed.cache, "set").returns(Promise.resolve());
@@ -83,13 +128,25 @@ describe("DiscoveryStreamFeed", () => {
       feed._prefCache = {};
       configPrefStub.returns(JSON.stringify({enabled: true}));
 
+      sandbox.stub(feed, "clearCache").returns(Promise.resolve());
       sandbox.stub(feed, "loadCachedData").returns(Promise.resolve());
       await feed.onAction({type: at.DISCOVERY_STREAM_CONFIG_CHANGE});
 
       assert.calledOnce(feed.loadCachedData);
+      assert.calledOnce(feed.clearCache);
       assert.isTrue(feed.loaded);
     });
-    it("should call this.loadCachedData if config.enabled changes to true ", async () => {
+    it("should clear the cache if a config change happens and config.enabled is true", async () => {
+      // force clear cached pref value
+      feed._prefCache = {};
+      configPrefStub.returns(JSON.stringify({enabled: true}));
+
+      sandbox.stub(feed, "clearCache").returns(Promise.resolve());
+      await feed.onAction({type: at.DISCOVERY_STREAM_CONFIG_CHANGE});
+
+      assert.calledOnce(feed.clearCache);
+    });
+    it("should not call this.loadCachedData if config.enabled changes to false", async () => {
       sandbox.stub(feed.cache, "set").returns(Promise.resolve());
       // force clear cached pref value
       feed._prefCache = {};
@@ -100,10 +157,12 @@ describe("DiscoveryStreamFeed", () => {
 
       feed._prefCache = {};
       configPrefStub.returns(JSON.stringify({enabled: false}));
+      sandbox.stub(feed, "clearCache").returns(Promise.resolve());
       sandbox.stub(feed, "loadCachedData").returns(Promise.resolve());
       await feed.onAction({type: at.DISCOVERY_STREAM_CONFIG_CHANGE});
 
       assert.notCalled(feed.loadCachedData);
+      assert.calledOnce(feed.clearCache);
       assert.isFalse(feed.loaded);
     });
   });