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

Simplify creation of a copy of the annotation with editor changes applied

Replace `updateDomainModel()` with a simpler function which returns a
copy of the input annotation with the changes from the editor applied.
parent debca3f9
...@@ -31,23 +31,20 @@ function errorMessage(reason) { ...@@ -31,23 +31,20 @@ function errorMessage(reason) {
return message; return message;
} }
/** Update `annotation` from vm. /**
* * Return a copy of `annotation` with changes made in the editor applied.
* Copy any properties from vm that might have been modified by the user into
* `annotation`, overwriting any existing properties in `annotation`.
*
* @param {object} annotation The object to copy properties to
* @param {object} vm The object to copy properties from
*
*/ */
function updateDomainModel(annotation, vm, permissions) { function updateModel(annotation, changes, permissions) {
annotation.text = vm.state().text; return Object.assign({}, annotation, {
annotation.tags = vm.state().tags; // Explicitly copy across the non-enumerable local tag for the annotation
if (vm.state().isPrivate) { $$tag: annotation.$$tag,
annotation.permissions = permissions.private();
} else { // Apply changes from the draft
annotation.permissions = permissions.shared(annotation.group); tags: changes.tags,
} text: changes.text,
permissions: changes.isPrivate ?
permissions.private() : permissions.shared(annotation.group),
});
} }
/** Update the view model from the domain model changes. */ /** Update the view model from the domain model changes. */
...@@ -65,11 +62,6 @@ function updateViewModel($scope, vm) { ...@@ -65,11 +62,6 @@ function updateViewModel($scope, vm) {
vm.documentMeta = annotationMetadata.domainAndTitle(vm.annotation); vm.documentMeta = annotationMetadata.domainAndTitle(vm.annotation);
} }
/**
* @ngdoc type
* @name annotation.AnnotationController
*
*/
// @ngInject // @ngInject
function AnnotationController( function AnnotationController(
$document, $q, $rootScope, $scope, $timeout, $window, annotationUI, $document, $q, $rootScope, $scope, $timeout, $window, annotationUI,
...@@ -423,11 +415,6 @@ function AnnotationController( ...@@ -423,11 +415,6 @@ function AnnotationController(
} }
}; };
/**
* @ngdoc method
* @name annotation.AnnotationController#save
* @description Saves any edits and returns to the viewer.
*/
vm.save = function() { vm.save = function() {
if (!vm.annotation.user) { if (!vm.annotation.user) {
flash.info('Please sign in to save your annotations.'); flash.info('Please sign in to save your annotations.');
...@@ -438,29 +425,22 @@ function AnnotationController( ...@@ -438,29 +425,22 @@ function AnnotationController(
return Promise.resolve(); return Promise.resolve();
} }
var updatedModel = angular.copy(vm.annotation); var updatedModel = updateModel(vm.annotation, vm.state(), permissions);
// Copy across the non-enumerable local tag for the annotation
updatedModel.$$tag = vm.annotation.$$tag;
updateDomainModel(updatedModel, vm, permissions); // Optimistically switch back to view mode and display the saving
var saved = save(updatedModel).then(function (model) {
var isNew = !vm.annotation.id;
drafts.remove(vm.annotation);
if (isNew) {
$rootScope.$emit(events.ANNOTATION_CREATED, vm.annotation);
} else {
$rootScope.$emit(events.ANNOTATION_UPDATED, vm.annotation);
}
updateView();
});
// optimistically switch back to view mode and display the saving
// indicator // indicator
vm.isSaving = true; vm.isSaving = true;
return saved.then(function () { return save(updatedModel).then(function (model) {
Object.assign(updatedModel, model);
vm.isSaving = false; vm.isSaving = false;
var event = isNew(vm.annotation) ?
events.ANNOTATION_CREATED : events.ANNOTATION_UPDATED;
drafts.remove(vm.annotation);
$rootScope.$emit(event, updatedModel);
}).catch(function (reason) { }).catch(function (reason) {
vm.isSaving = false; vm.isSaving = false;
vm.edit(); vm.edit();
...@@ -590,7 +570,7 @@ module.exports = { ...@@ -590,7 +570,7 @@ module.exports = {
// to be unit tested. // to be unit tested.
// FIXME: The code should be refactored to enable unit testing without having // FIXME: The code should be refactored to enable unit testing without having
// to do this. // to do this.
updateDomainModel: updateDomainModel, updateModel: updateModel,
// These are meant to be the public API of this module. // These are meant to be the public API of this module.
directive: annotation, directive: annotation,
......
...@@ -30,8 +30,8 @@ function annotationDirective() { ...@@ -30,8 +30,8 @@ function annotationDirective() {
} }
describe('annotation', function() { describe('annotation', function() {
describe('updateDomainModel()', function() { describe('updateModel()', function() {
var updateDomainModel = require('../annotation').updateDomainModel; var updateModel = require('../annotation').updateModel;
function fakePermissions() { function fakePermissions() {
return { return {
...@@ -40,79 +40,30 @@ describe('annotation', function() { ...@@ -40,79 +40,30 @@ describe('annotation', function() {
}; };
} }
it('copies text from viewModel into domainModel', function() { it('copies tags and text into the new model', function() {
var domainModel = {}; var changes = {text: 'bar', tags: ['foo', 'bar']};
var viewModel = {state: sinon.stub.returns({text: 'bar', tags: []})}; var newModel = updateModel(fixtures.defaultAnnotation(), changes,
fakePermissions());
updateDomainModel(domainModel, viewModel, fakePermissions()); assert.deepEqual(newModel.tags, changes.tags);
assert.equal(newModel.text, changes.text);
assert.equal(domainModel.text, viewModel.state().text);
});
it('overwrites text in domainModel', function() {
var domainModel = {text: 'foo'};
var viewModel = {state: sinon.stub.returns({text: 'bar', tags: []})};
updateDomainModel(domainModel, viewModel, fakePermissions());
assert.equal(domainModel.text, viewModel.state().text);
});
it('doesn\'t touch other properties in domainModel', function() {
var domainModel = {foo: 'foo', bar: 'bar'};
var viewModel = {state: sinon.stub.returns({foo: 'FOO', tags: []})};
updateDomainModel(domainModel, viewModel, fakePermissions());
assert.equal(
domainModel.bar, 'bar',
'updateDomainModel() should not touch properties of domainModel' +
'that don\'t exist in viewModel');
});
it('copies tag texts from viewModel into domainModel', function() {
var domainModel = {};
var viewModel = {
state: sinon.stub().returns({
tags: ['foo', 'bar'],
})
};
updateDomainModel(domainModel, viewModel, fakePermissions());
assert.deepEqual(domainModel.tags, ['foo', 'bar']);
}); });
it('sets domainModel.permissions to private if vm.isPrivate', function() { it('sets permissions to private if the draft is private', function() {
var domainModel = {}; var changes = {isPrivate: true, text: 'bar', tags: ['foo', 'bar']};
var viewModel = { var annot = fixtures.defaultAnnotation();
state: sinon.stub().returns({
isPrivate: true,
text: 'foo',
}),
};
var permissions = fakePermissions(); var permissions = fakePermissions();
permissions.private = sinon.stub().returns('private permissions'); permissions.private = sinon.stub().returns('private permissions');
var newModel = updateModel(annot, changes, permissions);
updateDomainModel(domainModel, viewModel, permissions); assert.equal(newModel.permissions, 'private permissions');
assert.equal(domainModel.permissions, 'private permissions');
}); });
it('sets domainModel.permissions to shared if !vm.isPrivate', function() { it('sets permissions to shared if the draft is shared', function() {
var domainModel = {}; var changes = {isPrivate: false, text: 'bar', tags: ['foo', 'bar']};
var viewModel = { var annot = fixtures.defaultAnnotation();
state: sinon.stub().returns({
isPrivate: false,
text: 'foo',
}),
};
var permissions = fakePermissions(); var permissions = fakePermissions();
permissions.shared = sinon.stub().returns('shared permissions'); permissions.shared = sinon.stub().returns('shared permissions');
var newModel = updateModel(annot, changes, permissions);
updateDomainModel(domainModel, viewModel, permissions); assert.equal(newModel.permissions, 'shared permissions');
assert.equal(domainModel.permissions, 'shared permissions');
}); });
}); });
...@@ -150,8 +101,6 @@ describe('annotation', function() { ...@@ -150,8 +101,6 @@ describe('annotation', function() {
}; };
} }
before(function() { before(function() {
angular.module('h', []) angular.module('h', [])
.directive('annotation', annotationDirective()); .directive('annotation', annotationDirective());
...@@ -238,11 +187,11 @@ describe('annotation', function() { ...@@ -238,11 +187,11 @@ describe('annotation', function() {
$provide.value('drafts', fakeDrafts); $provide.value('drafts', fakeDrafts);
$provide.value('features', fakeFeatures); $provide.value('features', fakeFeatures);
$provide.value('flash', fakeFlash); $provide.value('flash', fakeFlash);
$provide.value('groups', fakeGroups);
$provide.value('permissions', fakePermissions); $provide.value('permissions', fakePermissions);
$provide.value('session', fakeSession); $provide.value('session', fakeSession);
$provide.value('settings', fakeSettings); $provide.value('settings', fakeSettings);
$provide.value('store', fakeStore); $provide.value('store', fakeStore);
$provide.value('groups', fakeGroups);
})); }));
beforeEach( beforeEach(
...@@ -800,7 +749,7 @@ describe('annotation', function() { ...@@ -800,7 +749,7 @@ describe('annotation', function() {
})); }));
var saved = controller.save(); var saved = controller.save();
assert.equal(controller.isSaving, true); assert.equal(controller.isSaving, true);
create(); create(Object.assign({}, controller.annotation, {id: 'new-id'}));
return saved.then(function () { return saved.then(function () {
assert.equal(controller.isSaving, false); assert.equal(controller.isSaving, false);
}); });
...@@ -808,36 +757,19 @@ describe('annotation', function() { ...@@ -808,36 +757,19 @@ describe('annotation', function() {
it('does not remove the draft if saving fails', function () { it('does not remove the draft if saving fails', function () {
var controller = createController(); var controller = createController();
var failCreation; fakeStore.annotation.create = sinon.stub().returns(Promise.reject({status: -1}));
fakeStore.annotation.create = sinon.stub().returns(new Promise(function (resolve, reject) { return controller.save().then(function () {
failCreation = reject;
}));
var saved = controller.save();
assert.equal(controller.isSaving, true);
failCreation({status: -1});
return saved.then(function () {
assert.equal(controller.isSaving, false);
assert.notCalled(fakeDrafts.remove); assert.notCalled(fakeDrafts.remove);
}); });
}); });
it( it('sets the annotation\'s group to the focused group', function() {
'Passes group:<id> to the server when saving a new annotation', fakeGroups.focused = function () {
function() { return { id: 'test-id' };
fakeGroups.focused = function () { };
return { id: 'test-id' }; var controller = createDirective(fixtures.newAnnotation()).controller;
}; assert.equal(controller.annotation.group, 'test-id');
var annotation = { });
user: 'acct:fred@hypothes.is',
text: 'foo',
};
var controller = createDirective(annotation).controller;
return controller.save().then(function() {
assert.calledWith(fakeStore.annotation.create, sinon.match({}),
sinon.match({group: 'test-id'}));
});
}
);
}); });
describe('saving an edited an annotation', function() { describe('saving an edited an annotation', function() {
...@@ -852,19 +784,16 @@ describe('annotation', function() { ...@@ -852,19 +784,16 @@ describe('annotation', function() {
return createDirective(annotation).controller; return createDirective(annotation).controller;
} }
it( it('flashes a generic error if the server cannot be reached', function () {
'flashes a generic error if the server cannot be reached', var controller = createController();
function() { fakeStore.annotation.update = sinon.stub().returns(Promise.reject({
var controller = createController(); status: -1
fakeStore.annotation.update = sinon.stub().returns(Promise.reject({ }));
status: -1 return controller.save().then(function() {
})); assert.calledWith(fakeFlash.error,
return controller.save().then(function() { 'Service unreachable.', 'Saving annotation failed');
assert.calledWith(fakeFlash.error, });
'Service unreachable.', 'Saving annotation failed'); });
});
}
);
it('flashes an error if saving the annotation fails on the server', function () { it('flashes an error if saving the annotation fails on the server', function () {
var controller = createController(); var controller = createController();
......
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