From 2e3c294f3a8ddaef5b84304cfaa2801c1d585faa Mon Sep 17 00:00:00 2001 From: Andrei Oprea Date: Wed, 1 Aug 2018 12:16:02 +0000 Subject: [PATCH] Fix Bug 1479413 - Remove preview provider and message after it has been displayed --- content-src/asrouter/README.md | 29 +++++++++++++++++++++++++++++ lib/ASRouter.jsm | 19 ++++++++++++++++--- test/unit/asrouter/ASRouter.test.js | 21 ++++++++------------- 3 files changed, 53 insertions(+), 16 deletions(-) create mode 100644 content-src/asrouter/README.md diff --git a/content-src/asrouter/README.md b/content-src/asrouter/README.md new file mode 100644 index 000000000..1004f7426 --- /dev/null +++ b/content-src/asrouter/README.md @@ -0,0 +1,29 @@ +# Activity Stream Router + +## Preferences `browser.newtab.activity-stream.asrouter.*` + +Name | Used for | Type | Example value +--- | --- | --- | --- +`whitelistHosts` | Whitelist a host in order to fetch messages from its endpoint | `[String]` | `["gist.github.com", "gist.githubusercontent.com", "localhost:8000"]` +`snippetsUrl` | The main remote endpoint that serves all snippet messages | `String` | `https://activity-stream-icons.services.mozilla.com/v1/messages.json.br` + +## Admin Interface + +* Navigate to `about:newtab#asrouter` + * See all available messages and message providers + * Block, unblock or force show a specific message + +## Snippet Preview + +* Whitelist the provider host that will serve the messages + * In `about:config`, `browser.newtab.activity-stream.asrouter.whitelistHosts` can contain a array of hosts + * Example value `["gist.github.com", "gist.githubusercontent.com", "localhost:8000"]` + * Errors are surfaced in the `Console` tab of the `Browser Toolbox` ([read how to enable](https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox)) +* Navigate to `about:newtab?endpoint=` + * Example `https://gist.githubusercontent.com/piatra/70234f08696c0a0509d7ba5568cd830f/raw/68370f34abc134142c64b6f0a9b9258a06de7aa3/messages.json` + * URL should be from an endpoint that was just whitelisted + * The snippet preview should imediately load + * The endpoint must be HTTPS, the host must be whitelisted + * Errors are surfaced in the `Console` tab of the `Browser Toolbox` + +### [Snippet message format documentation](https://github.com/mozilla/activity-stream/blob/master/content-src/asrouter/schemas/message-format.md) diff --git a/lib/ASRouter.jsm b/lib/ASRouter.jsm index 81cc4d182..ee9955b38 100644 --- a/lib/ASRouter.jsm +++ b/lib/ASRouter.jsm @@ -241,7 +241,8 @@ class _ASRouter { ASRouterTriggerListeners.get(triggerID).uninit(); } - await this.setState(newState); + // We don't want to cache preview endpoints, remove them after messages are fetched + await this.setState(this._removePreviewEndpoint(newState)); await this.cleanupImpressions(); } } @@ -466,7 +467,15 @@ class _ASRouter { message = await this._findMessage(msgs, target, trigger); } - await this.setState({lastMessageId: message ? message.id : null}); + if (previewMsgs.length) { + // We don't want to cache preview messages, remove them after we selected the message to show + await this.setState(state => ({ + lastMessageId: message.id, + messages: state.messages.filter(m => m.id !== message.id) + })); + } else { + await this.setState({lastMessageId: message ? message.id : null}); + } await this._sendMessageToTarget(message, target, trigger); } @@ -547,10 +556,14 @@ class _ASRouter { await this.onMessage({target, data: {type: "TRIGGER", trigger}}); } + _removePreviewEndpoint(state) { + state.providers = state.providers.filter(p => p.id !== "preview"); + return state; + } + async _addPreviewEndpoint(url) { const providers = [...this.state.providers]; if (this._validPreviewEndpoint(url) && !providers.find(p => p.url === url)) { - // Set update cycle to 0 to fetch new content on every page refresh providers.push({id: "preview", type: "remote", url, updateCycleInMs: 0}); await this.setState({providers}); } diff --git a/test/unit/asrouter/ASRouter.test.js b/test/unit/asrouter/ASRouter.test.js index 2333f38b1..38e1b5ec9 100644 --- a/test/unit/asrouter/ASRouter.test.js +++ b/test/unit/asrouter/ASRouter.test.js @@ -329,27 +329,21 @@ describe("ASRouter", () => { assert.calledWith(msg.target.sendAsyncMessage, PARENT_TO_CHILD_MESSAGE_NAME, {type: "CLEAR_ALL"}); }); - it("should add the endpoint provided on CONNECT_UI_REQUEST", async () => { + it("should make a request to the provided endpoint on CONNECT_UI_REQUEST", async () => { const url = "https://snippets-admin.mozilla.org/foo"; const msg = fakeAsyncMessage({type: "CONNECT_UI_REQUEST", data: {endpoint: {url}}}); await Router.onMessage(msg); - assert.isDefined(Router.state.providers.find(p => p.url === url)); + assert.calledWith(global.fetch, url); + assert.lengthOf(Router.state.providers.filter(p => p.url === url), 0); }); - it("should add the endpoint provided on ADMIN_CONNECT_STATE", async () => { + it("should make a request to the provided endpoint on ADMIN_CONNECT_STATE and remove the endpoint", async () => { const url = "https://snippets-admin.mozilla.org/foo"; const msg = fakeAsyncMessage({type: "ADMIN_CONNECT_STATE", data: {endpoint: {url}}}); await Router.onMessage(msg); - assert.isDefined(Router.state.providers.find(p => p.url === url)); - }); - it("should not add the same endpoint twice", async () => { - const url = "https://snippets-admin.mozilla.org/foo"; - const msg = fakeAsyncMessage({type: "CONNECT_UI_REQUEST", data: {endpoint: {url}}}); - await Router.onMessage(msg); - await Router.onMessage(msg); - - assert.lengthOf(Router.state.providers.filter(p => p.url === url), 1); + assert.calledWith(global.fetch, url); + assert.lengthOf(Router.state.providers.filter(p => p.url === url), 0); }); it("should not add a url that is not from a whitelisted host", async () => { const url = "https://mozilla.org"; @@ -452,13 +446,14 @@ describe("ASRouter", () => { assert.calledOnce(Router.sendNextMessage); assert.calledWithExactly(Router.sendNextMessage, sinon.match.instanceOf(FakeRemotePageManager), {}); }); - it("should return the preview message if that's available", async () => { + it("should return the preview message if that's available and remove it from Router.state", async () => { const expectedObj = {provider: "preview"}; Router.setState({messages: [expectedObj]}); await Router.sendNextMessage(channel); assert.calledWith(channel.sendAsyncMessage, PARENT_TO_CHILD_MESSAGE_NAME, {type: "SET_MESSAGE", data: expectedObj}); + assert.isUndefined(Router.state.messages.find(m => m.provider === "preview")); }); it("should call _getBundledMessages if we request a message that needs to be bundled", async () => { sandbox.stub(Router, "_getBundledMessages").resolves();