Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the new /api/links resource #356

Merged
merged 4 commits into from
Apr 18, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/sidebar/annotation-ui.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions src/sidebar/reducers/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -29,6 +30,7 @@ function init(settings) {
{},
annotations.init(),
frames.init(),
links.init(),
selection.init(settings),
session.init(),
viewer.init()
Expand All @@ -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
Expand Down
27 changes: 27 additions & 0 deletions src/sidebar/reducers/links.js
Original file line number Diff line number Diff line change
@@ -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 },
};
31 changes: 31 additions & 0 deletions src/sidebar/reducers/test/links-test.js
Original file line number Diff line number Diff line change
@@ -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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nicer way of describing the module than the other modules use (which don't really have a clear scheme). It would be a good idea to make them consistent with this in future (not as part of this PR!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I think so too

describe('#init()', function() {
it('returns a null links object', function() {
assert.deepEqual(init(), {links: null});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since init() is a pure function with no arguments that returns a trivial value, I think a test is a bit pointless really. It is essentially testing the value of a constant.

});
});

describe('#update.UPDATE_LINKS()', function() {
it('returns the given newLinks as the links object', function() {
assert.deepEqual(
update('CURRENT_STATE', {newLinks: 'NEW_LINKS'}),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since tests partly serve as documentation, I prefer that test data should look like the real data, even if simplified. eg. If a function with a string argument expects a URL, the test should pass a URL, even if not strictly required for the test to pass.

In this case, the easiest way to do that would be to pass init() as the initial state and say a single-entry routes dictionary as the argument.

Copy link
Contributor Author

@seanh seanh Apr 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that tests partially serve as documentation. But here what I'm attempting to document is that what the UPDATE_LINKS() actually does is blindly take whatever value is passed to it (as newLinks: <VALUE>) and return it (as links: <VALUE>). The actual value, 'NEW_LINKS' in this test, is opaque as far as UPDATE_LINKS() is concerned. It could be a boolean, an object, whatever, and UPDATE_LINKS() would work the same. The fact that what serviceUrl actually passes happens to be an object is not UPDATE_LINKS()'s concern.

I could use an object here instead of a string in order to be consistent with what serviceUrl actually passes here. But then if serviceUrl were changed to store a different type of object these links test would not be failing, but would be out of date. In a weak way, the readability of the links tests would have been coupled to the serviceUrl code, whereas I'd like them to be coupled only to the links code.

Similarly the current value which annotationUI (or whatever part of our Redux machinery) passes to UPDATE_LINKS(), 'CURRENT_STATE' in this test, is also an opaque value (UPDATE_LINKS() actually doesn't touch it at all).

I agree that it's unfortunate that, because the tests use strings here, from reading the tests it might look as if the links reducer is only for storing strings when in fact it's for storing any kind of value and in practice is currently only used to store an object. This is case where in Python you would use a sentinel object rather than a string as a clear way to say this to the reader that this is an opaque value. I don't think any such thing exists in JavaScript so I fell back on all caps strings hoping that would be clear enough.

Even though currently it's only used for storing the links templates, this links reducer could easily be generalised into a "store one opaque value in Redux and be able to update that value by completely replacing it with a new value" reducer, like var linksReducer = singleOpaqueValueReducer('links');.

Do you think that makes sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that the code doesn't care about the shape of links now happens to be true. It might not be the case as the code evolves. I would avoid any abstraction at this stage and add it once there is enough obviously duplicate code to make it worthwhile.

You're also assuming that we keep links as its own state module, as opposed to say, grouping it with other API-related state which we might want to do.

{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' });
});
});
});
73 changes: 48 additions & 25 deletions src/sidebar/service-url.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,43 +3,66 @@
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(error) {
console.warn('The links API request was rejected: ' + error.message);
});

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;
};
}

Expand Down
1 change: 1 addition & 0 deletions src/sidebar/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ function store($http, $q, auth, settings) {
read: apiCall('profile.read'),
update: apiCall('profile.update'),
},
links: apiCall('links'),
};
}

Expand Down
166 changes: 156 additions & 10 deletions src/sidebar/test/service-url-test.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,166 @@
'use strict';

var serviceUrl = require('../service-url');
var proxyquire = require('proxyquire');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comment as with service-url.js, the diff is messed up so I recommend just viewing the service-url-test.js file (which is completely new) instead of looking at the diff


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};
},
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This annotationUI fake is interesting. From serviceUrl's point of view, the contract of annotationUI is just:

  • You can set the links value (and it will do the magic re-rendering of everything)
  • You can get the links value, and it will return the same object that was set
  • Initially, before set, get returns null. Or it could be undefined

This is actually the contract of a standard JavaScript object:

> var annotationUI = {}
> annotationUI.links
undefined
> annotationUI.links = 'foo'
> annotationUI.links
"foo"

I wonder if annotationUI could use getters and setters so that it presents the interface of a normal JavaScript object to serviceUrl? That would simplify serviceUrl's code, but also in the tests fakeAnnotationUI would just be: var fakeAnnotationUI = {};.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for simple actions but not for the case where one action updates multiple parts of the state. For example, the process of logging out might: 1) Update the user's session, 2) Clear the set of annotations, 3) Remove any unsaved drafts. We don't actually have any actions that really do this yet, but it is a common pattern.

btw. What you are describing sounds a bit like MobX which is a library that lets you turn ordinary objects into "observable" ones that trigger arbitrary side-effects then properties are modified.

}

beforeEach(function () {
service = serviceUrl({serviceUrl: 'https://test.hypothes.is/'});
function createServiceUrl(linksPromise) {
var replaceURLParams = sinon.stub().returns(
{url: 'EXPANDED_URL', params: {}}
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaceURLParams could be simpler from the point of view of serviceUrl if it just returned the expanded URL (only) and raised an error if there were unused params. That way serviceUrl wouldn't have to check for unused params and format and throw the error itself. (And it wouldn't have to catch the error either, since it wants to throw it.) But also this mock would just be var replaceURLParams = sinon.stub().returns('EXPANDED_URL') or, when testing what happens when an error is thrown, var replaceURLParams = sinon.stub().throws(new Error()). I haven't looked into how replaceURLParams is used elsewhere, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBQH I wouldn't have bothered mocking replaceURLParams. On the one hand, doing so means that a change to replaceURLParams implementation cannot break this test.

On the other hand, a) the mock's outputs here don't even resemble the real one, so the test loses some value as documentation. b) the mock's outputs don't depend on the inputs, which creates additional ways for the code under test to do the wrong thing but yet still pass the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the mock's outputs don't depend on the inputs, which creates additional ways for the code
under test to do the wrong thing but yet still pass the test.

I don't think this is true (I don't think you can actually change serviceUrl so that it calls replaceURLParams incorrectly, or does the wrong thing with the return value from replaceURLParams, and not have one of my serviceUrl tests fail).

I'm not sure that the test here does lose any value as documentation. All serviceUrl really does is call replaceURLParams and return replaceURLParams's return value. The return value from replaceURLParams is an opaque value, as far as serviceUrl is concerned. The tests, which are tests for serviceURL only after all (replaceURLParams has its own tests elsewhere), do document the intended behaviour of serviceURL correctly (that it just blindly returns what replaceURLParams returns).

I suppose I could have used "https://example.com/expanded_url" instead of "EXPANDED_URL" but I'm not sure there's much difference (this is another case where I would rather have used mock.sentinel.expanded_url if we had that in JavaScript).

There is of course a trade-off that, as you point out, while isolating the serviceUrl tests mean that the serviceUrl tests won't fail if there's a bug in replaceURLParams, if the stub replaceURLParams in service-url-test.js is wrong or out of date then the serviceUrl tests could be passing even though the code is wrong and would crash or misbehave in production. As you point out integrating service-url-test.js with the real replaceURLParams would avoid this problem.

I fall down on the side of isolating the tests here, rather than integrating them, for a couple of reasons.

One is that isolated tests provide feedback on the design of your code by revealing how deeply and complexly the module under test is coupled to its dependencies. If code is tightly coupled you will end up having to create complex mocks. I think that a couple of the mocks I had to create here to write isolated tests for serviceUrl are showing us small examples of unnecessarily tight coupling, which the previous integrated tests for serviceUrl didn't reveal.

The second reason is that it scales better. Across a large test suite when you make a lot of small decisions to integrate the tests for a lot of modules with a lot of their dependencies, then you end up in a situation where running the entire test suite is slow, where a bug in one part of the code causes a cascade of test failures all over the test suite making debugging difficult, and where the design of the code in terms of the coupling, interfaces and contracts of modules is bad because you haven't been getting design feedback from isolated tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are sound principles but I'm not convinced they really apply in this case. There is a spectrum of choices when writing tests between injecting absolutely every dependency that your code has and injecting none at all. Both extremes are obviously bad. Injecting a dependency always has a cost in terms of making the code or the tests a bit more complex and less straightforward, as well as differences in behaviour between the injected dependency and the real one.

I usually take the view that pure helper functions which don't involve any substantial computation are not worth injecting most of the time. But in the interests of forward progress, I'm going to ah. disagree and commit.

Let's settle this over a beer in SF 😉


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 () {

beforeEach(function() {
sinon.stub(console, 'warn');
});

afterEach(function () {
console.warn.restore();
});

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(new Error('Oops'));

var serviceUrl = createServiceUrl(linksPromise).serviceUrl;

assert.equal(serviceUrl('second_link'), '');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assert is being executed before the Promise handlers inside serviceUrl have actually been executed, so this isn't testing the behaviour you intended to test. The behaviour you actually want to test is difficult without the kind of refactoring we talked about in Slack though to make it possible to wait on the link fetch completing.

});
});

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');
});
});
});
});