Commit 0d9217ec authored by Robert Knight's avatar Robert Knight

Remove use of ngResource for search and annotation queries

Several aspects of ngResource make it sub-optimal for our needs:

 1. It mutates the model object directly after making an API call, which
    does not fit well with usage in a Redux app where the UI state should
    be an immutable object.

 2. The ngResource classes can only be constructed once the API
    description has been retrieved. At least one place in our code,
    which handled newly created annotations arriving from the page,
    failed to account for this.

This commit therefore replaces use of ngResource for making API calls
to the search and annotation endpoints with a simple wrapper around HTTP
requests which makes a call and then returns the resulting object.

The new wrapper will wait until the API description has been received
before attempting to make an API call, avoiding problem (2) above.

As a bonus, this brings us a step towards decoupling the JS API client
from Angular so we could re-use it outside the app in future.

 * Replace ngResource with simple $http wrapper in store.js

 * Add tests for annotation update and deletion API calls

 * Change tests in annotation-test.js from
   `assert(predicate(actual, expected))` form to
   `assert.predicate(actual, expected))` form as this results in errors
   that are _much_ easier to debug when they fail.
parent d87490f3
......@@ -24,7 +24,7 @@ function annotationMapper($rootScope, annotationUI, store) {
$rootScope.$emit(events.ANNOTATION_UPDATED, existing);
return;
}
loaded.push(new store.AnnotationResource(annotation));
loaded.push(annotation);
});
$rootScope.$emit(events.ANNOTATIONS_LOADED, loaded);
......@@ -42,13 +42,12 @@ function annotationMapper($rootScope, annotationUI, store) {
}
function createAnnotation(annotation) {
annotation = new store.AnnotationResource(annotation);
$rootScope.$emit(events.BEFORE_ANNOTATION_CREATED, annotation);
return annotation;
}
function deleteAnnotation(annotation) {
return annotation.$delete({
return store.annotation.delete({
id: annotation.id
}).then(function () {
$rootScope.$emit(events.ANNOTATION_DELETED, annotation);
......
......@@ -9,16 +9,16 @@ var angular = require('angular');
*/
function fetchThread(store, id) {
var annot;
return store.AnnotationResource.get({id: id}).$promise.then(function (annot) {
return store.annotation.get({id: id}).then(function (annot) {
if (annot.references && annot.references.length) {
// This is a reply, fetch the top-level annotation
return store.AnnotationResource.get({id: annot.references[0]}).$promise;
return store.annotation.get({id: annot.references[0]});
} else {
return annot;
}
}).then(function (annot_) {
annot = annot_;
return store.SearchResource.get({references: annot.id}).$promise;
return store.search({references: annot.id});
}).then(function (searchResult) {
return [annot].concat(searchResult.rows);
});
......
......@@ -34,10 +34,6 @@ var resolve = {
sessionState: function (session) {
return session.load();
},
// @ngInject
store: function (store) {
return store.$promise;
},
streamer: streamer.connect,
};
......
......@@ -26,7 +26,7 @@ module.exports = class CrossFrame
formatted[k] = v
formatted
parser: (annotation) ->
parsed = new store.AnnotationResource()
parsed = {}
for k, v of annotation when k in whitelist
parsed[k] = v
parsed
......
......@@ -129,12 +129,33 @@ function updateViewModel($scope, domainModel,
function AnnotationController(
$document, $q, $rootScope, $scope, $timeout, $window, annotationUI,
annotationMapper, drafts, flash, features, groups, permissions, session,
settings) {
settings, store) {
var vm = this;
var domainModel;
var newlyCreatedByHighlightButton;
/** Save an annotation to the server. */
function save(annot) {
var saved;
if (annot.id) {
saved = store.annotation.update({id: annot.id}, annot);
} else {
saved = store.annotation.create({}, annot);
}
return saved.then(function (savedAnnot) {
// Copy across internal properties which are not part of the annotation
// model saved on the server
savedAnnot.$$tag = annot.$$tag;
Object.keys(annot).forEach(function (k) {
if (k[0] === '$') {
savedAnnot[k] = annot[k];
}
});
return savedAnnot;
});
}
/**
* Initialize this AnnotationController instance.
*
......@@ -306,8 +327,9 @@ function AnnotationController(
// User is logged in, save to server.
// Highlights are always private.
domainModel.permissions = permissions.private();
domainModel.$create().then(function() {
$rootScope.$emit(events.ANNOTATION_CREATED, domainModel);
save(domainModel).then(function(model) {
domainModel = model;
$rootScope.$emit(events.ANNOTATION_CREATED, model);
updateView(domainModel);
});
} else {
......@@ -518,31 +540,23 @@ function AnnotationController(
return Promise.resolve();
}
var saved;
switch (vm.action) {
case 'create':
updateDomainModel(domainModel, vm, permissions);
saved = domainModel.$create().then(function () {
$rootScope.$emit(events.ANNOTATION_CREATED, domainModel);
updateView(domainModel);
drafts.remove(domainModel);
});
break;
case 'edit':
var updatedModel = angular.copy(domainModel);
updateDomainModel(updatedModel, vm, permissions);
saved = updatedModel.$update({
id: updatedModel.id
}).then(function () {
drafts.remove(domainModel);
$rootScope.$emit(events.ANNOTATION_UPDATED, updatedModel);
});
break;
var updatedModel = angular.copy(domainModel);
default:
throw new Error('Tried to save an annotation that is not being edited');
}
// Copy across the non-enumerable local tag for the annotation
updatedModel.$$tag = domainModel.$$tag;
updateDomainModel(updatedModel, vm, permissions);
var saved = save(updatedModel).then(function (model) {
var isNew = !domainModel.id;
drafts.remove(domainModel);
domainModel = model;
if (isNew) {
$rootScope.$emit(events.ANNOTATION_CREATED, domainModel);
} else {
$rootScope.$emit(events.ANNOTATION_UPDATED, domainModel);
}
updateView(domainModel);
});
// optimistically switch back to view mode and display the saving
// indicator
......
......@@ -8,15 +8,15 @@ var inherits = require('inherits');
*
* SearchClient handles paging through results, canceling search etc.
*
* @param {Object} resource - ngResource class instance for the /search API
* @param {Object} searchFn - Function for querying the search API
* @param {Object} opts - Search options
* @constructor
*/
function SearchClient(resource, opts) {
function SearchClient(searchFn, opts) {
opts = opts || {};
var DEFAULT_CHUNK_SIZE = 200;
this._resource = resource;
this._searchFn = searchFn;
this._chunkSize = opts.chunkSize || DEFAULT_CHUNK_SIZE;
if (typeof opts.incremental !== 'undefined') {
this._incremental = opts.incremental;
......@@ -37,7 +37,7 @@ SearchClient.prototype._getBatch = function (query, offset) {
}, query);
var self = this;
this._resource.get(searchQuery).$promise.then(function (results) {
this._searchFn(searchQuery).then(function (results) {
if (self._canceled) {
return;
}
......
'use strict';
var angular = require('angular');
var get = require('lodash.get');
var retryUtil = require('./retry-util');
var urlUtil = require('./util/url-util');
function prependTransform(defaults, transform) {
// We can't guarantee that the default transformation is an array
......@@ -79,55 +81,60 @@ function serializeParams(params) {
return parts.join('&');
}
/**
* @ngdoc factory
* @name store
* @description The `store` service handles the backend calls for the restful
* API. This is created dynamically from the document returned at
* API index so as to ensure that URL paths/methods do not go out
* of date.
*
* The service currently exposes two resources:
* Creates a function that will make an API call to a named route.
*
* store.SearchResource, for searching, and
* store.AnnotationResource, for CRUD operations on annotations.
* @param $http - The Angular HTTP service
* @param links - Object or promise for an object mapping named API routes to
* URL templates and methods
* @param route - The dotted path of the named API route (eg. `annotation.create`)
*/
// @ngInject
function store($http, $resource, settings) {
var instance = {};
var defaultOptions = {
paramSerializer: serializeParams,
transformRequest: prependTransform(
$http.defaults.transformRequest,
stripInternalProperties
)
function createAPICall($http, links, route) {
return function (params, data) {
return links.then(function (links) {
var descriptor = get(links, route);
var url = urlUtil.replaceURLParams(descriptor.url, params);
var req = {
data: data,
method: descriptor.method,
params: url.params,
paramSerializer: serializeParams,
url: url.url,
transformRequest: prependTransform(
$http.defaults.transformRequest,
stripInternalProperties
),
};
return $http(req);
}).then(function (result) {
return result.data;
});
};
}
// We call the API root and it gives back the actions it provides.
instance.$promise = retryUtil.retryPromiseOperation(function () {
/**
* API client for the Hypothesis REST API.
*
* Returns an object that with keys that match the routes in
* the Hypothesis API (see http://h.readthedocs.io/en/latest/api/).
*/
// @ngInject
function store($http, settings) {
var links = retryUtil.retryPromiseOperation(function () {
return $http.get(settings.apiUrl);
}).then(function (response) {
var links = response.data.links;
// N.B. in both cases below we explicitly override the default `get`
// action because there is no way to provide defaultOptions to the default
// action.
instance.SearchResource = $resource(links.search.url, {}, {
get: angular.extend({url: links.search.url}, defaultOptions),
});
instance.AnnotationResource = $resource(links.annotation.read.url, {}, {
get: angular.extend(links.annotation.read, defaultOptions),
create: angular.extend(links.annotation.create, defaultOptions),
update: angular.extend(links.annotation.update, defaultOptions),
delete: angular.extend(links.annotation.delete, defaultOptions),
});
return instance;
return response.data.links;
});
return instance;
return {
search: createAPICall($http, links, 'search'),
annotation: {
create: createAPICall($http, links, 'annotation.create'),
delete: createAPICall($http, links, 'annotation.delete'),
get: createAPICall($http, links, 'annotation.read'),
update: createAPICall($http, links, 'annotation.update'),
},
};
}
module.exports = store;
......@@ -20,11 +20,13 @@ module.exports = class StreamController
searchParams = searchFilter.toObject($routeParams.q)
query = angular.extend(options, searchParams)
query._separate_replies = true
store.SearchResource.get(query, load)
store.search(query)
.then(load)
.catch((err) -> console.error err)
load = ({rows, replies}) ->
offset += rows.length
annotationMapper.loadAnnotations(rows, replies)
offset += rows.length
annotationMapper.loadAnnotations(rows, replies)
# Reload on query change (ignore hash change)
lastQuery = $routeParams.q
......
......@@ -13,7 +13,9 @@ describe('annotationMapper', function() {
beforeEach(function () {
fakeStore = {
AnnotationResource: sandbox.stub().returns({}),
annotation: {
delete: sinon.stub().returns(Promise.resolve({})),
},
};
angular.module('app', [])
.service('annotationMapper', require('../annotation-mapper'))
......@@ -40,7 +42,7 @@ describe('annotationMapper', function() {
annotationMapper.loadAnnotations(annotations);
assert.called($rootScope.$emit);
assert.calledWith($rootScope.$emit, events.ANNOTATIONS_LOADED,
[{}, {}, {}]);
[{id: 1}, {id: 2}, {id: 3}]);
});
it('also includes replies in the annotationLoaded event', function () {
......@@ -50,7 +52,7 @@ describe('annotationMapper', function() {
annotationMapper.loadAnnotations(annotations, replies);
assert.called($rootScope.$emit);
assert.calledWith($rootScope.$emit, events.ANNOTATIONS_LOADED,
[{}, {}, {}]);
[{id: 1}, {id: 2}, {id: 3}]);
});
it('triggers the annotationUpdated event for each loaded annotation', function () {
......@@ -122,25 +124,16 @@ describe('annotationMapper', function() {
});
describe('#createAnnotation()', function () {
it('creates a new annotaton resource', function () {
it('creates a new annotation resource', function () {
var ann = {};
fakeStore.AnnotationResource.returns(ann);
var ret = annotationMapper.createAnnotation(ann);
assert.equal(ret, ann);
});
it('creates a new resource with the new keyword', function () {
var ann = {};
fakeStore.AnnotationResource.returns(ann);
annotationMapper.createAnnotation();
assert.calledWithNew(fakeStore.AnnotationResource);
});
it('emits the "beforeAnnotationCreated" event', function () {
sandbox.stub($rootScope, '$emit');
var ann = {};
fakeStore.AnnotationResource.returns(ann);
annotationMapper.createAnnotation();
annotationMapper.createAnnotation(ann);
assert.calledWith($rootScope.$emit,
events.BEFORE_ANNOTATION_CREATED, ann);
});
......@@ -148,16 +141,14 @@ describe('annotationMapper', function() {
describe('#deleteAnnotation()', function () {
it('deletes the annotation on the server', function () {
var p = Promise.resolve();
var ann = {$delete: sandbox.stub().returns(p)};
var ann = {id: 'test-id'};
annotationMapper.deleteAnnotation(ann);
assert.called(ann.$delete);
assert.calledWith(fakeStore.annotation.delete, {id: 'test-id'});
});
it('triggers the "annotationDeleted" event on success', function (done) {
sandbox.stub($rootScope, '$emit');
var p = Promise.resolve();
var ann = {$delete: sandbox.stub().returns(p)};
var ann = {};
annotationMapper.deleteAnnotation(ann).then(function () {
assert.calledWith($rootScope.$emit,
events.ANNOTATION_DELETED, ann);
......@@ -165,23 +156,14 @@ describe('annotationMapper', function() {
$rootScope.$apply();
});
it('does nothing on error', function (done) {
it('does not emit an event on error', function (done) {
sandbox.stub($rootScope, '$emit');
var p = Promise.reject();
var ann = {$delete: sandbox.stub().returns(p)};
fakeStore.annotation.delete.returns(Promise.reject());
var ann = {id: 'test-id'};
annotationMapper.deleteAnnotation(ann).catch(function () {
assert.notCalled($rootScope.$emit);
}).then(done, done);
$rootScope.$apply();
});
it('return a promise that resolves to the deleted annotation', function (done) {
var p = Promise.resolve();
var ann = {$delete: sandbox.stub().returns(p)};
annotationMapper.deleteAnnotation(ann).then(function (value) {
assert.equal(value, ann);
}).then(done, done);
$rootScope.$apply();
});
});
});
......@@ -7,7 +7,7 @@ var angular = require('angular');
function FakeStore(annots) {
this.annots = annots;
this.AnnotationResource = {
this.annotation = {
get: function (query) {
var result;
if (query.id) {
......@@ -15,20 +15,18 @@ function FakeStore(annots) {
return a.id === query.id;
});
}
return {$promise: Promise.resolve(result)};
return Promise.resolve(result);
}
};
this.SearchResource = {
get: function (query) {
var result;
if (query.references) {
result = annots.filter(function (a) {
return a.references && a.references.indexOf(query.references) !== -1;
});
}
return {$promise: Promise.resolve({rows: result})};
this.search = function (query) {
var result;
if (query.references) {
result = annots.filter(function (a) {
return a.references && a.references.indexOf(query.references) !== -1;
});
}
return Promise.resolve({rows: result});
};
}
......
......@@ -16,24 +16,20 @@ describe('SearchClient', function () {
{id: 'four'},
];
var fakeResource;
var fakeSearchFn;
beforeEach(function () {
fakeResource = {
get: sinon.spy(function (params) {
return {
$promise: Promise.resolve({
rows: RESULTS.slice(params.offset,
params.offset + params.limit),
total: RESULTS.length,
}),
};
}),
};
fakeSearchFn = sinon.spy(function (params) {
return Promise.resolve({
rows: RESULTS.slice(params.offset,
params.offset + params.limit),
total: RESULTS.length,
});
});
});
it('emits "results"', function () {
var client = new SearchClient(fakeResource);
var client = new SearchClient(fakeSearchFn);
var onResults = sinon.stub();
client.on('results', onResults);
client.get({uri: 'http://example.com'});
......@@ -43,7 +39,7 @@ describe('SearchClient', function () {
});
it('emits "results" with chunks in incremental mode', function () {
var client = new SearchClient(fakeResource, {chunkSize: 2});
var client = new SearchClient(fakeSearchFn, {chunkSize: 2});
var onResults = sinon.stub();
client.on('results', onResults);
client.get({uri: 'http://example.com'});
......@@ -54,7 +50,7 @@ describe('SearchClient', function () {
});
it('emits "results" once in non-incremental mode', function () {
var client = new SearchClient(fakeResource,
var client = new SearchClient(fakeSearchFn,
{chunkSize: 2, incremental: false});
var onResults = sinon.stub();
client.on('results', onResults);
......@@ -66,7 +62,7 @@ describe('SearchClient', function () {
});
it('does not emit "results" if canceled', function () {
var client = new SearchClient(fakeResource);
var client = new SearchClient(fakeSearchFn);
var onResults = sinon.stub();
var onEnd = sinon.stub();
client.on('results', onResults);
......@@ -81,12 +77,10 @@ describe('SearchClient', function () {
it('emits "error" event if search fails', function () {
var err = new Error('search failed');
fakeResource.get = function () {
return {
$promise: Promise.reject(err),
};
fakeSearchFn = function () {
return Promise.reject(err);
};
var client = new SearchClient(fakeResource);
var client = new SearchClient(fakeSearchFn);
var onError = sinon.stub();
client.on('error', onError);
client.get({uri: 'http://example.com'});
......
......@@ -11,7 +11,7 @@ describe('store', function () {
var store = null;
before(function () {
angular.module('h', ['ngResource'])
angular.module('h')
.service('store', proxyquire('../store', util.noCallThru({
angular: angular,
'./retry-util': {
......@@ -35,7 +35,7 @@ describe('store', function () {
sandbox.restore();
});
beforeEach(angular.mock.inject(function ($q, _$httpBackend_, _store_) {
beforeEach(angular.mock.inject(function (_$httpBackend_, _store_) {
$httpBackend = _$httpBackend_;
store = _store_;
......@@ -46,11 +46,18 @@ describe('store', function () {
method: 'POST',
url: 'http://example.com/api/annotations',
},
delete: {},
delete: {
method: 'DELETE',
url: 'http://example.com/api/annotations/:id',
},
read: {},
update: {},
update: {
method: 'PUT',
url: 'http://example.com/api/annotations/:id',
},
},
search: {
method: 'GET',
url: 'http://example.com/api/search',
},
},
......@@ -58,34 +65,42 @@ describe('store', function () {
$httpBackend.flush();
}));
it('reads the operations from the backend', function () {
assert.isFunction(store.AnnotationResource);
assert.isFunction(store.SearchResource);
});
it('saves a new annotation', function () {
var annotation = new store.AnnotationResource({id: 'test'});
var saved = {};
annotation.$create().then(function () {
store.annotation.create({}, {}).then(function (saved) {
assert.isNotNull(saved.id);
});
$httpBackend.expectPOST('http://example.com/api/annotations')
.respond(function () {
return [201, {id: 'new-id'}, {}];
});
$httpBackend.flush();
});
$httpBackend.expectPOST('http://example.com/api/annotations', {id: 'test'})
it('updates an annotation', function () {
store.annotation.update({id: 'an-id'}, {text: 'updated'});
$httpBackend.expectPUT('http://example.com/api/annotations/an-id')
.respond(function () {
saved.id = annotation.id;
return [201, {}, {}];
return [200, {}, {}];
});
$httpBackend.flush();
});
it('deletes an annotation', function () {
store.annotation.delete({id: 'an-id'}, {});
$httpBackend.expectDELETE('http://example.com/api/annotations/an-id')
.respond(function () {
return [200, {}, {}];
});
$httpBackend.flush();
});
it('removes internal properties before sending data to the server', function () {
var annotation = new store.AnnotationResource({
var annotation = {
$highlight: true,
$notme: 'nooooo!',
allowed: 123
});
annotation.$create();
};
store.annotation.create({}, annotation);
$httpBackend.expectPOST('http://example.com/api/annotations', {
allowed: 123
})
......@@ -96,7 +111,7 @@ describe('store', function () {
// Our backend service interprets semicolons as query param delimiters, so we
// must ensure to encode them in the query string.
it('encodes semicolons in query parameters', function () {
store.SearchResource.get({'uri': 'http://example.com/?foo=bar;baz=qux'});
store.search({'uri': 'http://example.com/?foo=bar;baz=qux'});
$httpBackend.expectGET('http://example.com/api/search?uri=http%3A%2F%2Fexample.com%2F%3Ffoo%3Dbar%3Bbaz%3Dqux')
.respond(function () { return [200, {}, {}]; });
$httpBackend.flush();
......
......@@ -62,9 +62,7 @@ describe 'StreamController', ->
}
fakeStore = {
SearchResource: {
get: sandbox.spy()
}
search: sandbox.spy(-> Promise.resolve({rows: [], total: 0}))
}
fakeStreamer = {
......@@ -103,22 +101,21 @@ describe 'StreamController', ->
it 'calls the search API with _separate_replies: true', ->
createController()
assert.equal(
fakeStore.SearchResource.get.firstCall.args[0]._separate_replies, true)
assert.equal(fakeStore.search.firstCall.args[0]._separate_replies, true)
it 'passes the annotations and replies from search to loadAnnotations()', ->
fakeStore.SearchResource.get = (query, func) ->
func({
fakeStore.search = (query) ->
Promise.resolve({
'rows': ['annotation_1', 'annotation_2']
'replies': ['reply_1', 'reply_2', 'reply_3']
})
createController()
assert fakeAnnotationMapper.loadAnnotations.calledOnce
assert fakeAnnotationMapper.loadAnnotations.calledWith(
['annotation_1', 'annotation_2'], ['reply_1', 'reply_2', 'reply_3']
)
Promise.resolve().then ->
assert.calledOnce fakeAnnotationMapper.loadAnnotations
assert.calledWith fakeAnnotationMapper.loadAnnotations,
['annotation_1', 'annotation_2'], ['reply_1', 'reply_2', 'reply_3']
describe 'on $routeUpdate', ->
......
......@@ -9,8 +9,8 @@ var events = require('../events');
var noCallThru = require('./util').noCallThru;
var searchClients;
function FakeSearchClient(resource, opts) {
assert.ok(resource);
function FakeSearchClient(searchFn, opts) {
assert.ok(searchFn);
searchClients.push(this);
this.cancel = sinon.stub();
this.incremental = !!opts.incremental;
......@@ -118,7 +118,7 @@ describe('WidgetController', function () {
fakeSettings = {};
fakeStore = {
SearchResource: {},
search: sinon.stub(),
};
$provide.value('VirtualThreadList', FakeVirtualThreadList);
......
'use strict';
var urlUtil = require('../url-util');
describe('url-util', function () {
describe('replaceURLParams()', function () {
it('should replace params in URLs', function () {
var replaced = urlUtil.replaceURLParams('http://foo.com/things/:id',
{id: 'test'});
assert.equal(replaced.url, 'http://foo.com/things/test');
});
it('should return unused params', function () {
var replaced = urlUtil.replaceURLParams('http://foo.com/:id',
{id: 'test', 'q': 'unused'});
assert.deepEqual(replaced.params, {q: 'unused'});
});
});
});
'use strict';
/**
* Replace parameters in a URL template with values from a `params` object.
*
* Returns an object containing the expanded URL and a dictionary of unused
* parameters.
*
* replaceURLParams('/things/:id', {id: 'foo', q: 'bar'}) =>
* {url: '/things/foo', params: {q: 'bar'}}
*/
function replaceURLParams(url, params) {
var unusedParams = {};
for (var param in params) {
if (params.hasOwnProperty(param)) {
var value = params[param];
var urlParam = ':' + param;
if (url.indexOf(urlParam) !== -1) {
url = url.replace(urlParam, value);
} else {
unusedParams[param] = value;
}
}
}
return {url: url, params: unusedParams};
}
module.exports = {
replaceURLParams: replaceURLParams,
};
......@@ -189,7 +189,7 @@ module.exports = function WidgetController(
}
function _loadAnnotationsFor(uris, group) {
var searchClient = new SearchClient(store.SearchResource, {
var searchClient = new SearchClient(store.search, {
// If no group is specified, we are fetching annotations from
// all groups in order to find out which group contains the selected
// annotation, therefore we need to load all chunks before processing
......
......@@ -61,6 +61,7 @@
"karma-phantomjs-launcher": "^0.2.3",
"karma-sinon": "^1.0.4",
"lodash.debounce": "^4.0.3",
"lodash.get": "^4.3.0",
"mkdirp": "^0.5.1",
"mocha": "^2.4.5",
"ng-tags-input": "^3.1.1",
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment