Skip to content
This repository has been archived by the owner on Feb 29, 2020. It is now read-only.

Commit

Permalink
Bug 1544578 - TopStoriesFeed early return for discovery stream (#4992)
Browse files Browse the repository at this point in the history 
* Bug 1544578 - TopStories Feed early return for discovery stream

fix

* Review feedback, initialize properties once fix

* move storiesLoaded check and rename variable to propertiesInitialized
  • Loading branch information
@punamdahiya
punamdahiya committed May 14, 2019
1 parent 84543b0 commit ea405a2
Show file tree
Hide file tree
Showing 3 changed files with 172 additions and 9 deletions.
2 changes: 1 addition & 1 deletion lib/ActivityStream.jsm
View file
Expand Up @@ -302,7 +302,7 @@ const FEEDS_DATA = [
},
{
name: "section.topstories",
factory: () => new TopStoriesFeed(),
factory: () => new TopStoriesFeed(PREFS_CONFIG.get("discoverystream.config")),
title: "Fetches content recommendations from a configurable content provider",
// Dynamically determine if Pocket should be shown for a geo / locale
getValue: ({geo, locale}) => {
Expand Down
76 changes: 71 additions & 5 deletions lib/TopStoriesFeed.jsm
View file
Expand Up @@ -30,17 +30,33 @@ const SPOC_IMPRESSION_TRACKING_PREF = "feeds.section.topstories.spoc.impressions
const REC_IMPRESSION_TRACKING_PREF = "feeds.section.topstories.rec.impressions";
const OPTIONS_PREF = "feeds.section.topstories.options";
const MAX_LIFETIME_CAP = 500; // Guard against misconfiguration on the server
const DISCOVERY_STREAM_PREF = "discoverystream.config";

this.TopStoriesFeed = class TopStoriesFeed {
constructor() {
this.spocCampaignMap = new Map();
constructor(ds) {
// Use discoverystream config pref default values for fast path and
// if needed lazy load activity stream top stories feed based on
// actual user preference when PREFS_INITIAL_VALUES and PREF_CHANGED is invoked
this.discoveryStreamEnabled = ds && ds.value && JSON.parse(ds.value).enabled;
if (!this.discoveryStreamEnabled) {
this.initializeProperties();
}
}

initializeProperties() {
this.contentUpdateQueue = [];
this.spocCampaignMap = new Map();
this.cache = new PersistentCache(SECTION_ID, true);
this._prefs = new Prefs();
this.propertiesInitialized = true;
}

async onInit() {
SectionsManager.enableSection(SECTION_ID);
if (this.discoveryStreamEnabled) {
return;
}

try {
const {options} = SectionsManager.sections.get(SECTION_ID);
const apiKey = this.getApiKeyFromPref(options.api_key_pref);
Expand Down Expand Up @@ -99,7 +115,12 @@ this.TopStoriesFeed = class TopStoriesFeed {

uninit() {
this.storiesLoaded = false;
Services.obs.removeObserver(this, "idle-daily");
try {
Services.obs.removeObserver(this, "idle-daily");
} catch (e) {
// Attempt to remove unassociated observer which is possible when discovery stream
// is enabled and user never used activity stream experience
}
SectionsManager.disableSection(SECTION_ID);
}

Expand Down Expand Up @@ -643,10 +664,52 @@ this.TopStoriesFeed = class TopStoriesFeed {
return false;
}

lazyLoadTopStories(dsPref) {
try {
this.discoveryStreamEnabled = JSON.parse(dsPref).enabled;
} catch (e) {
// Load activity stream top stories if fail to determine discovery stream state
this.discoveryStreamEnabled = false;
}

// Return without invoking initialization if top stories are loaded
if (this.storiesLoaded) {
return;
}

if (!this.discoveryStreamEnabled && !this.propertiesInitialized) {
this.initializeProperties();
}
this.init();
}

handleDisabled(action) {
switch (action.type) {
case at.PREFS_INITIAL_VALUES:
this.lazyLoadTopStories(action.data[DISCOVERY_STREAM_PREF]);
break;
case at.PREF_CHANGED:
if (action.data.name === DISCOVERY_STREAM_PREF) {
this.lazyLoadTopStories(action.data.value);
}
break;
case at.UNINIT:
this.uninit();
break;
}
}

async onAction(action) {
if (this.discoveryStreamEnabled) {
this.handleDisabled(action);
return;
}
switch (action.type) {
case at.INIT:
this.init();
// Check for pref initial values to lazy load activity stream top stories
// Here we are not using usual INIT and relying on PREFS_INITIAL_VALUES
// to check discoverystream pref and load activity stream top stories only if needed.
case at.PREFS_INITIAL_VALUES:
this.lazyLoadTopStories(action.data[DISCOVERY_STREAM_PREF]);
break;
case at.SYSTEM_TICK:
let stories;
Expand Down Expand Up @@ -710,6 +773,9 @@ this.TopStoriesFeed = class TopStoriesFeed {
break;
}
case at.PREF_CHANGED:
if (action.data.name === DISCOVERY_STREAM_PREF) {
this.lazyLoadTopStories(action.data.value);
}
// Check if spocs was disabled. Remove them if they were.
if (action.data.name === "showSponsored" && !action.data.value) {
await this.removeSpocs();
Expand Down
103 changes: 100 additions & 3 deletions test/unit/lib/TopStoriesFeed.test.js
View file
Expand Up @@ -91,12 +91,109 @@ describe("Top Stories Feed", () => {
globals.restore();
clock.restore();
});

describe("#lazyloading TopStories", () => {
beforeEach(() => {
instance.discoveryStreamEnabled = true;
});
it("should bind parseOptions to SectionsManager.onceInitialized when discovery stream is true", () => {
instance.discoveryStreamEnabled = false;
instance.onAction({type: at.PREFS_INITIAL_VALUES, data: {"discoverystream.config": JSON.stringify({enabled: true})}});
assert.calledOnce(sectionsManagerStub.onceInitialized);
});
it("should bind parseOptions to SectionsManager.onceInitialized when discovery stream is false", () => {
instance.onAction({type: at.PREFS_INITIAL_VALUES, data: {"discoverystream.config": JSON.stringify({enabled: false})}});
assert.calledOnce(sectionsManagerStub.onceInitialized);
});
it("Should initialize properties once while lazy loading if not initialized earlier", () => {
instance.discoveryStreamEnabled = false;
instance.propertiesInitialized = false;
sinon.stub(instance, "initializeProperties");
instance.lazyLoadTopStories();
assert.calledOnce(instance.initializeProperties);
});
it("should not re-initialize properties", () => {
// For discovery stream experience disabled TopStoriesFeed properties
// are initialized in constructor and should not be called again while lazy loading topstories
sinon.stub(instance, "initializeProperties");
instance.discoveryStreamEnabled = false;
instance.propertiesInitialized = true;
instance.lazyLoadTopStories();
assert.notCalled(instance.initializeProperties);
});
it("should have early exit onInit when discovery is true", async () => {
sinon.stub(instance, "doContentUpdate");
await instance.onInit();
assert.notCalled(instance.doContentUpdate);
assert.isUndefined(instance.storiesLoaded);
});
it("should complete onInit when discovery is false", async () => {
instance.discoveryStreamEnabled = false;
sinon.stub(instance, "doContentUpdate");
await instance.onInit();
assert.calledOnce(instance.doContentUpdate);
assert.isTrue(instance.storiesLoaded);
});
it("should handle limited actions when discoverystream is enabled", async () => {
sinon.spy(instance, "handleDisabled");
sinon.stub(instance, "getPocketState");
instance.onAction({type: at.PREFS_INITIAL_VALUES, data: {"discoverystream.config": JSON.stringify({enabled: true})}});
assert.calledOnce(instance.handleDisabled);

instance.onAction({type: at.NEW_TAB_REHYDRATED, meta: {fromTarget: {}}});
assert.notCalled(instance.getPocketState);
});
it("should handle NEW_TAB_REHYDRATED when discoverystream is disabled", async () => {
instance.discoveryStreamEnabled = false;
sinon.spy(instance, "handleDisabled");
sinon.stub(instance, "getPocketState");
instance.onAction({type: at.PREFS_INITIAL_VALUES, data: {"discoverystream.config": JSON.stringify({enabled: false})}});
assert.notCalled(instance.handleDisabled);

instance.onAction({type: at.NEW_TAB_REHYDRATED, meta: {fromTarget: {}}});
assert.calledOnce(instance.getPocketState);
});
it("should handle UNINIT when discoverystream is enabled", async () => {
sinon.stub(instance, "uninit");
instance.onAction({type: at.UNINIT});
assert.calledOnce(instance.uninit);
});
it("should fire init on PREF_CHANGED", () => {
sinon.stub(instance, "onInit");
instance.onAction({type: at.PREF_CHANGED, data: {name: "discoverystream.config", value: {}}});
assert.calledOnce(instance.onInit);
});
it("should not fire init on PREF_CHANGED if stories are loaded", () => {
sinon.stub(instance, "onInit");
sinon.spy(instance, "lazyLoadTopStories");
instance.storiesLoaded = true;
instance.onAction({type: at.PREF_CHANGED, data: {name: "discoverystream.config", value: {}}});
assert.calledOnce(instance.lazyLoadTopStories);
assert.notCalled(instance.onInit);
});
it("should fire init on PREF_CHANGED when discoverystream is disabled", () => {
instance.discoveryStreamEnabled = false;
sinon.stub(instance, "onInit");
instance.onAction({type: at.PREF_CHANGED, data: {name: "discoverystream.config", value: {}}});
assert.calledOnce(instance.onInit);
});
it("should not fire init on PREF_CHANGED when discoverystream is disabled and stories are loaded", () => {
instance.discoveryStreamEnabled = false;
sinon.stub(instance, "onInit");
sinon.spy(instance, "lazyLoadTopStories");
instance.storiesLoaded = true;
instance.onAction({type: at.PREF_CHANGED, data: {name: "discoverystream.config", value: {}}});
assert.calledOnce(instance.lazyLoadTopStories);
assert.notCalled(instance.onInit);
});
});

describe("#init", () => {
it("should create a TopStoriesFeed", () => {
assert.instanceOf(instance, TopStoriesFeed);
});
it("should bind parseOptions to SectionsManager.onceInitialized", () => {
instance.onAction({type: at.INIT});
instance.onAction({type: at.PREFS_INITIAL_VALUES, data: {}});
assert.calledOnce(sectionsManagerStub.onceInitialized);
});
it("should initialize endpoints based on options", async () => {
Expand All @@ -106,13 +203,13 @@ describe("Top Stories Feed", () => {
assert.equal("https://somedomain.org/topics?key=test-api-key", instance.topics_endpoint);
});
it("should enable its section", () => {
instance.onAction({type: at.INIT});
instance.onAction({type: at.PREFS_INITIAL_VALUES, data: {}});
assert.calledOnce(sectionsManagerStub.enableSection);
assert.calledWith(sectionsManagerStub.enableSection, SECTION_ID);
});
it("init should fire onInit", () => {
instance.onInit = sinon.spy();
instance.onAction({type: at.INIT});
instance.onAction({type: at.PREFS_INITIAL_VALUES, data: {}});
assert.calledOnce(instance.onInit);
});
it("should fetch stories on init", async () => {
Expand Down

0 comments on commit ea405a2

Please sign in to comment.