Commit 682088d7 authored by Sean Hammond's avatar Sean Hammond Committed by GitHub

Merge pull request #4 from hypothesis/remove-ngresource-for-api-calls

Remove use of ngResource for search and annotation queries
parents 8a5ab37d 0d9217ec
......@@ -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
......
......@@ -124,12 +124,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.
*
......@@ -301,8 +322,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 {
......@@ -513,31 +535,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