From ad8cbf1cdc5582f7a68ac2eb24de9b088441545d Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Mon, 10 Apr 2017 18:42:17 +0100 Subject: [PATCH 1/4] Add links reducer Add a Redux reducer for storing the links (URL templates) from the /api/links HTTP resource in the state store. --- src/sidebar/annotation-ui.js | 2 ++ src/sidebar/reducers/index.js | 3 +++ src/sidebar/reducers/links.js | 27 +++++++++++++++++++++ src/sidebar/reducers/test/links-test.js | 31 +++++++++++++++++++++++++ 4 files changed, 63 insertions(+) create mode 100644 src/sidebar/reducers/links.js create mode 100644 src/sidebar/reducers/test/links-test.js diff --git a/src/sidebar/annotation-ui.js b/src/sidebar/annotation-ui.js index 310563de73e..47dfaed2974 100644 --- a/src/sidebar/annotation-ui.js +++ b/src/sidebar/annotation-ui.js @@ -39,6 +39,7 @@ var thunk = require('redux-thunk').default; var reducers = require('./reducers'); var annotationsReducer = require('./reducers/annotations'); var framesReducer = require('./reducers/frames'); +var linksReducer = require('./reducers/links'); var selectionReducer = require('./reducers/selection'); var sessionReducer = require('./reducers/session'); var viewerReducer = require('./reducers/viewer'); @@ -95,6 +96,7 @@ module.exports = function ($rootScope, settings) { var actionCreators = redux.bindActionCreators(Object.assign({}, annotationsReducer.actions, framesReducer.actions, + linksReducer.actions, selectionReducer.actions, sessionReducer.actions, viewerReducer.actions diff --git a/src/sidebar/reducers/index.js b/src/sidebar/reducers/index.js index 6959bcabb56..7e54c5e45ba 100644 --- a/src/sidebar/reducers/index.js +++ b/src/sidebar/reducers/index.js @@ -19,6 +19,7 @@ var annotations = require('./annotations'); var frames = require('./frames'); +var links = require('./links'); var selection = require('./selection'); var session = require('./session'); var viewer = require('./viewer'); @@ -29,6 +30,7 @@ function init(settings) { {}, annotations.init(), frames.init(), + links.init(), selection.init(settings), session.init(), viewer.init() @@ -38,6 +40,7 @@ function init(settings) { var update = util.createReducer(Object.assign( annotations.update, frames.update, + links.update, selection.update, session.update, viewer.update diff --git a/src/sidebar/reducers/links.js b/src/sidebar/reducers/links.js new file mode 100644 index 00000000000..d1cf17f8c3f --- /dev/null +++ b/src/sidebar/reducers/links.js @@ -0,0 +1,27 @@ +/** + * Reducer for storing a "links" object in the Redux state store. + * + * The links object is initially null, and can only be updated by completely + * replacing it with a new links object. + * + * Used by serviceUrl. + */ + +'use strict'; + +/** Return the initial links. */ +function init() { return {links: null}; } + +/** Return updated links based on the given current state and action object. */ +function updateLinks(state, action) { return {links: action.newLinks}; } + +/** Return an action object for updating the links to the given newLinks. */ +function updateLinksAction(newLinks) { + return { type: 'UPDATE_LINKS', newLinks: newLinks }; +} + +module.exports = { + init: init, + update: { UPDATE_LINKS: updateLinks }, + actions: { updateLinks: updateLinksAction }, +}; diff --git a/src/sidebar/reducers/test/links-test.js b/src/sidebar/reducers/test/links-test.js new file mode 100644 index 00000000000..81ec3d462f4 --- /dev/null +++ b/src/sidebar/reducers/test/links-test.js @@ -0,0 +1,31 @@ +'use strict'; + +var links = require('../links'); + +var init = links.init; +var update = links.update.UPDATE_LINKS; +var action = links.actions.updateLinks; + +describe('sidebar.reducers.links', function() { + describe('#init()', function() { + it('returns a null links object', function() { + assert.deepEqual(init(), {links: null}); + }); + }); + + describe('#update.UPDATE_LINKS()', function() { + it('returns the given newLinks as the links object', function() { + assert.deepEqual( + update('CURRENT_STATE', {newLinks: 'NEW_LINKS'}), + {links: 'NEW_LINKS'}); + }); + }); + + describe('#actions.updateLinks()', function() { + it('returns an UPDATE_LINKS action object for the given newLinks', function() { + assert.deepEqual( + action('NEW_LINKS'), + { type: 'UPDATE_LINKS', newLinks: 'NEW_LINKS' }); + }); + }); +}); From 50aeb3b5c4ffb44740498188e5226706bfc796e6 Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Wed, 12 Apr 2017 14:35:19 +0100 Subject: [PATCH 2/4] Add store.links() API wrapper Add a new function to `store`, the https://hypothes.is/api wrapper, that provides access to the new https://hypothes.is/api/links resource. --- src/sidebar/store.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sidebar/store.js b/src/sidebar/store.js index b7bc74a068c..23384f424be 100644 --- a/src/sidebar/store.js +++ b/src/sidebar/store.js @@ -178,6 +178,7 @@ function store($http, $q, auth, settings) { read: apiCall('profile.read'), update: apiCall('profile.update'), }, + links: apiCall('links'), }; } From ea9069a7048c0f2c62790f0247ccfeb5305c46b7 Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Wed, 12 Apr 2017 15:39:36 +0100 Subject: [PATCH 3/4] Change serviceUrl to use the /api/links Change serviceUrl to return URLs by expanding URL templates received from h's /api/links resource, rather than using hardcoded URL templates as it previously did. The interface and contract that serviceUrl presents to its users remains the same - the fact it's now sending an API request to get the URL templates, and waiting for the response, is completely hidden from its users. This hiding is achieved by two tricks: 1. Always returning "" for an expanded URL if the API response hasn't been received yet, rather than, for example, changing the interface to return Promises instead of strings (which would then require changes to every user of the interface, and possibly their users...) 2. Updating annotationUI, the Redux state store, with the new URL templates when the API response is received. This causes any AngularJS components that might have used an (empty string) URL to be re-rendered, they will call sericeUrl URL again and this time get the actual URLs. This is completely transparent to the components - annotationUI causes AngularJS to call them again and this time they will get a different result from serviceUrl. The code of the components themselves doesn't need to change. Additionally, serviceUrl will never throw errors if it hasn't received the API response yet. For example there is no error if an unknown link name is requested, or if unused template params are given. It just always returns "". Once it _does_ have the API response then it will start throwing errors for bad requests, the same as it did previously. --- src/sidebar/service-url.js | 75 ++++++++----- src/sidebar/test/service-url-test.js | 157 +++++++++++++++++++++++++-- 2 files changed, 197 insertions(+), 35 deletions(-) diff --git a/src/sidebar/service-url.js b/src/sidebar/service-url.js index 4523089c79b..a39990c42ac 100644 --- a/src/sidebar/service-url.js +++ b/src/sidebar/service-url.js @@ -3,43 +3,68 @@ var urlUtil = require('./util/url-util'); /** - * A map of all route names to relative URLs on the Hypothesis service that - * the client links to. + * A function that returns an absolute URL given a link name and params, by + * expanding named URL templates received from the annotation service's API. * - * The URLs are relative to the `serviceUrl` URL in the app's settings. + * The links object from the API is a map of link names to URL templates: + * + * { + * signup: "http://localhost:5000/signup", + * user: "http://localhost:5000/u/:user", + * ... + * } + * + * Given a link name (e.g. 'user') and params (e.g. {user: 'bob'}) return + * an absolute URL by expanding the named URL template from the API with the + * given params (e.g. "http://localhost:5000/u/bob"). + * + * Before the links object has been received from the API this function + * always returns empty strings as the URLs. After the links object has been + * received from the API this function starts returning the real URLs. + * + * @param {string} linkName - The name of the link to expand + * @param {object} params - The params with which to expand the link + * @returns {string} The expanded absolute URL, or an empty string if the + * links haven't been received from the API yet + * @throws {Error} If the links have been received from the API but the given + * linkName is unknown + * @throws {Error} If one or more of the params given isn't used in the URL + * template + * + * @ngInject */ -var ROUTES = { - 'account.settings': 'account/settings', - 'forgot-password': 'forgot-password', - 'groups.leave': 'groups/:id/leave', - 'groups.new': 'groups/new', - 'help': 'docs/help', - 'signup': 'signup', - 'search.tag': 'search?q=tag:":tag"', - 'user': 'u/:user', -}; +function serviceUrl(annotationUI, store) { -/** - * A service which maps route names to URLs on the Hypothesis annotation - * service. - */ -// @ngInject -function serviceUrl(settings) { - return function (route, params) { - params = params || {}; + store.links() + .then(annotationUI.updateLinks) + .catch(function() { + // We catch rejected promises here in order to silence + // 'Unhandled promise rejection' warnings, but we don't do anything if + // the promise is rejected. + }); + + return function(linkName, params) { + var links = annotationUI.getState().links; + + if (links === null) { + return ''; + } + + var path = links[linkName]; - var path = ROUTES[route]; if (!path) { - throw new Error('Unknown route ' + route); + throw new Error('Unknown link ' + linkName); } + + params = params || {}; var url = urlUtil.replaceURLParams(path, params); var unused = Object.keys(url.params); if (unused.length > 0) { - throw new Error('Unknown route parameters: ' + unused.join(', ')); + throw new Error('Unknown link parameters: ' + unused.join(', ')); } - return settings.serviceUrl + url.url; + return url.url; }; } diff --git a/src/sidebar/test/service-url-test.js b/src/sidebar/test/service-url-test.js index 864eee9a567..8fddb659d4c 100644 --- a/src/sidebar/test/service-url-test.js +++ b/src/sidebar/test/service-url-test.js @@ -1,20 +1,157 @@ 'use strict'; -var serviceUrl = require('../service-url'); +var proxyquire = require('proxyquire'); -describe('serviceUrl', function () { - var service; +/** Return a fake annotationUI object. */ +function fakeAnnotationUI() { + var links = null; + return { + updateLinks: function(newLinks) { + links = newLinks; + }, + getState: function() { + return {links: links}; + }, + }; +} - beforeEach(function () { - service = serviceUrl({serviceUrl: 'https://test.hypothes.is/'}); +function createServiceUrl(linksPromise) { + var replaceURLParams = sinon.stub().returns( + {url: 'EXPANDED_URL', params: {}} + ); + + var serviceUrlFactory = proxyquire('../service-url', { + './util/url-util': { replaceURLParams: replaceURLParams }, + }); + + var annotationUI = fakeAnnotationUI(); + + var store = { + links: sinon.stub().returns(linksPromise), + }; + + return { + annotationUI: annotationUI, + store: store, + serviceUrl: serviceUrlFactory(annotationUI, store), + replaceURLParams: replaceURLParams, + }; +} + +describe('links', function () { + context('before the API response has been received', function() { + var serviceUrl; + var store; + + beforeEach(function() { + // Create a serviceUrl function with an unresolved Promise that will + // never be resolved - it never receives the links from store.links(). + var parts = createServiceUrl(new Promise(function() {})); + + serviceUrl = parts.serviceUrl; + store = parts.store; + }); + + it('sends one API request for the links at boot time', function() { + assert.calledOnce(store.links); + assert.isTrue(store.links.calledWithExactly()); + }); + + it('returns an empty string for any link', function() { + assert.equal(serviceUrl('foo'), ''); + }); + + it('returns an empty string even if link params are given', function() { + assert.equal(serviceUrl('foo', {bar: 'bar'}), ''); + }); }); - it('returns route URLs', function () { - assert.equal(service('help'), 'https://test.hypothes.is/docs/help'); + context('if the API request fails', function() { + it('just keeps returning empty strings for URLs', function() { + var linksPromise = Promise.reject(); + + var serviceUrl = createServiceUrl(linksPromise).serviceUrl; + + assert.equal(serviceUrl('second_link'), ''); + }); }); - it('expands route parameters', function () { - assert.equal(service('user', {user: 'jim'}), - 'https://test.hypothes.is/u/jim'); + context('after the API response has been received', function() { + var annotationUI; + var linksPromise; + var replaceURLParams; + var serviceUrl; + + beforeEach(function() { + // The links Promise that store.links() will return. + linksPromise = Promise.resolve({ + first_link: 'http://example.com/first_page/:foo', + second_link: 'http://example.com/second_page', + }); + + var parts = createServiceUrl(linksPromise); + + annotationUI = parts.annotationUI; + serviceUrl = parts.serviceUrl; + replaceURLParams = parts.replaceURLParams; + }); + + it('updates annotationUI with the real links', function() { + return linksPromise.then(function(links) { + assert.deepEqual(annotationUI.getState(), {links: links}); + }); + }); + + it('calls replaceURLParams with the path and given params', function() { + return linksPromise.then(function() { + var params = {foo: 'bar'}; + + serviceUrl('first_link', params); + + assert.calledOnce(replaceURLParams); + assert.deepEqual( + replaceURLParams.args[0], + ['http://example.com/first_page/:foo', params]); + }); + }); + + it('passes an empty params object to replaceURLParams if no params are given', function() { + return linksPromise.then(function() { + serviceUrl('first_link'); + + assert.calledOnce(replaceURLParams); + assert.deepEqual(replaceURLParams.args[0][1], {}); + }); + }); + + it('returns the expanded URL from replaceURLParams', function() { + return linksPromise.then(function() { + var renderedUrl = serviceUrl('first_link'); + + assert.equal(renderedUrl, 'EXPANDED_URL'); + }); + }); + + it("throws an error if it doesn't have the requested link", function() { + return linksPromise.then(function() { + assert.throws( + function() { serviceUrl('madeUpLinkName'); }, + Error, 'Unknown link madeUpLinkName'); + }); + }); + + it('throws an error if replaceURLParams returns unused params', function() { + var params = {'unused_param_1': 'foo', 'unused_param_2': 'bar'}; + replaceURLParams.returns({ + url: 'EXPANDED_URL', + params: params, + }); + + return linksPromise.then(function() { + assert.throws( + function() { serviceUrl('first_link', params); }, + Error, 'Unknown link parameters: unused_param_1, unused_param_2'); + }); + }); }); }); From b40af32eca13ea097cd0fe4f440c218973f8d612 Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Tue, 18 Apr 2017 15:31:42 +0100 Subject: [PATCH 4/4] Log a warning if the links API request is rejected There's no unit test for this. Because of Promises, it cannot be tested without refactoring serviceUrl. --- src/sidebar/service-url.js | 6 ++---- src/sidebar/test/service-url-test.js | 11 ++++++++++- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/sidebar/service-url.js b/src/sidebar/service-url.js index a39990c42ac..d07e522a792 100644 --- a/src/sidebar/service-url.js +++ b/src/sidebar/service-url.js @@ -37,10 +37,8 @@ function serviceUrl(annotationUI, store) { store.links() .then(annotationUI.updateLinks) - .catch(function() { - // We catch rejected promises here in order to silence - // 'Unhandled promise rejection' warnings, but we don't do anything if - // the promise is rejected. + .catch(function(error) { + console.warn('The links API request was rejected: ' + error.message); }); return function(linkName, params) { diff --git a/src/sidebar/test/service-url-test.js b/src/sidebar/test/service-url-test.js index 8fddb659d4c..16dccbeecaa 100644 --- a/src/sidebar/test/service-url-test.js +++ b/src/sidebar/test/service-url-test.js @@ -39,6 +39,15 @@ function createServiceUrl(linksPromise) { } describe('links', function () { + + beforeEach(function() { + sinon.stub(console, 'warn'); + }); + + afterEach(function () { + console.warn.restore(); + }); + context('before the API response has been received', function() { var serviceUrl; var store; @@ -68,7 +77,7 @@ describe('links', function () { context('if the API request fails', function() { it('just keeps returning empty strings for URLs', function() { - var linksPromise = Promise.reject(); + var linksPromise = Promise.reject(new Error('Oops')); var serviceUrl = createServiceUrl(linksPromise).serviceUrl;