Skip to content

C
coopwire-hypothesis
Unverified Commit 8142037e authored by Lyza Gardner's avatar Lyza Gardner Committed by GitHub
Browse files
Options

Merge pull request #1869 from hypothesis/create-annotation-service 

Move `createAnnotation` to a service
parents ec5d1b01 5ee1e30d
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 321 additions and 426 deletions
src/sidebar/components/annotation-omega.js
View file @ 8142037e
import classnames from 'classnames';
import { createElement } from 'preact';
import { useEffect, useState } from 'preact/hooks';
import { useState } from 'preact/hooks';
import propTypes from 'prop-types';
import useStore from '../store/use-store';
import {
isHighlight,
isNew,
isReply,
quote,
} from '../util/annotation-metadata';
import { isNew, isReply, quote } from '../util/annotation-metadata';
import { isShared } from '../util/permissions';
import { withServices } from '../util/service-context';
......@@ -56,18 +51,6 @@ function AnnotationOmega({
const toggleAction = threadIsCollapsed ? 'Show replies' : 'Hide replies';
const toggleText = `${toggleAction} (${replyCount})`;
useEffect(() => {
// TEMPORARY. Create a new draft for new (non-highlight) annotations
// to put the component in "edit mode."
if (!isSaving && !draft && isNew(annotation) && !isHighlight(annotation)) {
createDraft(annotation, {
tags: annotation.tags,
text: annotation.text,
isPrivate: !isShared(annotation.permissions),
});
}
}, [annotation, draft, createDraft, isSaving]);
const shouldShowActions = !isSaving && !isEditing && !isNew(annotation);
const shouldShowLicense = isEditing && !isPrivate && group.type !== 'private';
const shouldShowReplyToggle = replyCount > 0 && !isReply(annotation);
......@@ -80,14 +63,7 @@ function AnnotationOmega({
createDraft(annotation, { ...draft, text });
};
const onReply = () => {
// TODO: Re-evaluate error handling here
if (!userid) {
flash.error('Please log in to reply to annotations');
} else {
annotationsService.reply(annotation, userid);
}
};
const onReply = () => annotationsService.reply(annotation, userid);
const onSave = async () => {
setIsSaving(true);
......
src/sidebar/components/annotation.js
View file @ 8142037e
......@@ -82,15 +82,6 @@ function AnnotationController(
* new client-side).
*/
newlyCreatedByHighlightButton = self.annotation.$highlight || false;
// If this annotation is not a highlight and if it's new (has just been
// created by the annotate button) or it has edits not yet saved to the
// server - then open the editor on AnnotationController instantiation.
if (!newlyCreatedByHighlightButton) {
if (isNew(self.annotation) || store.getDraft(self.annotation)) {
self.edit();
}
}
};
/**
......
src/sidebar/components/new-note-btn.js
View file @ 8142037e
......@@ -8,11 +8,10 @@ import { applyTheme } from '../util/theme';
import Button from './button';
function NewNoteButton({ settings }) {
function NewNoteButton({ annotationsService, settings }) {
const topLevelFrame = useStore(store => store.mainFrame());
const isLoggedIn = useStore(store => store.isLoggedIn());
const createAnnotation = useStore(store => store.createAnnotation);
const openSidebarPanel = useStore(store => store.openSidebarPanel);
const onNewNoteBtnClick = function() {
......@@ -24,7 +23,7 @@ function NewNoteButton({ settings }) {
target: [],
uri: topLevelFrame.uri,
};
createAnnotation(annot);
annotationsService.create(annot);
};
return (
......@@ -42,9 +41,10 @@ function NewNoteButton({ settings }) {
}
NewNoteButton.propTypes = {
// Injected services.
annotationsService: propTypes.object.isRequired,
settings: propTypes.object.isRequired,
};
NewNoteButton.injectedProps = ['settings'];
NewNoteButton.injectedProps = ['annotationsService', 'settings'];
export default withServices(NewNoteButton);
src/sidebar/components/test/annotation-omega-test.js
View file @ 8142037e
......@@ -445,23 +445,6 @@ describe('AnnotationOmega', () => {
describe('annotation actions', () => {
describe('replying to an annotation', () => {
// nb: There's no reason this logic needs to stay within `AnnotationOmega`
// once we've migrated to it; it could happily move to `AnnotationActionBar`
it('should show a flash alert if user not logged in', () => {
// No logged-in user...
fakeStore.profile.returns({});
const wrapper = createComponent();
wrapper
.find('AnnotationActionBar')
.props()
.onReply();
assert.calledOnce(fakeFlash.error);
assert.notCalled(fakeAnnotationsService.reply);
});
it('should create a reply', () => {
const theAnnot = fixtures.defaultAnnotation();
const wrapper = createComponent({ annotation: theAnnot });
......
src/sidebar/components/test/annotation-test.js
View file @ 8142037e
......@@ -235,28 +235,6 @@ describe('annotation', function() {
sandbox.restore();
});
describe('initialization', function() {
it('creates drafts for new annotations on initialization', function() {
const annotation = fixtures.newAnnotation();
createDirective(annotation);
assert.calledWith(fakeStore.createDraft, annotation, {
isPrivate: false,
tags: annotation.tags,
text: annotation.text,
});
});
it('edits annotations with drafts on initialization', function() {
const annotation = fixtures.oldAnnotation();
// The drafts store has some draft changes for this annotation.
fakeStore.getDraft.returns({ text: 'foo', tags: [] });
const controller = createDirective(annotation).controller;
assert.isTrue(controller.editing());
});
});
describe('#editing()', function() {
it('returns false if the annotation does not have a draft', function() {
const controller = createDirective().controller;
......
src/sidebar/components/test/new-note-btn-test.js
View file @ 8142037e
......@@ -11,13 +11,22 @@ import mockImportedComponents from '../../../test-util/mock-imported-components'
describe('NewNoteButton', function() {
let fakeStore;
let fakeAnnotationsService;
let fakeSettings;
function createComponent() {
return mount(<NewNoteButton settings={fakeSettings} />);
return mount(
<NewNoteButton
annotationsService={fakeAnnotationsService}
settings={fakeSettings}
/>
);
}
beforeEach(function() {
fakeAnnotationsService = {
create: sinon.stub(),
};
fakeSettings = {
branding: {
ctaBackgroundColor: '#00f',
......@@ -77,7 +86,7 @@ describe('NewNoteButton', function() {
.onClick();
});
assert.calledWith(fakeStore.createAnnotation, {
assert.calledWith(fakeAnnotationsService.create, {
target: [],
uri: 'thisFrame',
});
......
src/sidebar/services/annotations.js
View file @ 8142037e
import SearchClient from '../search-client';
import { isNew, isPublic } from '../util/annotation-metadata';
import { privatePermissions, sharedPermissions } from '../util/permissions';
import * as metadata from '../util/annotation-metadata';
import {
defaultPermissions,
privatePermissions,
sharedPermissions,
} from '../util/permissions';
import { generateHexString } from '../util/random';
import uiConstants from '../ui-constants';
// @ngInject
export default function annotationsService(
......@@ -12,6 +18,88 @@ export default function annotationsService(
) {
let searchClient = null;
/**
* Extend new annotation objects with defaults and permissions.
*/
function initialize(annotationData, now = new Date()) {
const defaultPrivacy = store.getDefault('annotationPrivacy');
const groupid = store.focusedGroupId();
const profile = store.profile();
const userid = profile.userid;
const userInfo = profile.user_info;
// We need a unique local/app identifier for this new annotation such
// that we might look it up later in the store. It won't have an ID yet,
// as it has not been persisted to the service.
const $tag = generateHexString(8);
let permissions = defaultPermissions(userid, groupid, defaultPrivacy);
// Highlights are peculiar in that they always have private permissions
if (metadata.isHighlight(annotationData)) {
permissions = privatePermissions(userid);
}
return Object.assign(
{
created: now.toISOString(),
group: groupid,
permissions,
tags: [],
text: '',
updated: now.toISOString(),
user: userid,
user_info: userInfo,
$tag: $tag,
},
annotationData
);
}
/**
* Populate a new annotation object from `annotation` and add it to the store.
* Create a draft for it unless it's a highlight and clear other empty
* drafts out of the way.
*
* @param {Object} annotationData
* @param {Date} now
*/
function create(annotationData, now = new Date()) {
const annotation = initialize(annotationData, now);
store.addAnnotations([annotation]);
// Remove other drafts that are in the way, and their annotations (if new)
store.deleteNewAndEmptyDrafts();
// Create a draft unless it's a highlight
if (!metadata.isHighlight(annotation)) {
store.createDraft(annotation, {
tags: annotation.tags,
text: annotation.text,
isPrivate: !metadata.isPublic(annotation),
});
}
// NB: It may make sense to move the following code at some point to
// the UI layer
// Select the correct tab
// If the annotation is of type note or annotation, make sure
// the appropriate tab is selected. If it is of type reply, user
// stays in the selected tab.
if (metadata.isPageNote(annotation)) {
store.selectTab(uiConstants.TAB_NOTES);
} else if (metadata.isAnnotation(annotation)) {
store.selectTab(uiConstants.TAB_ANNOTATIONS);
}
(annotation.references || []).forEach(parent => {
// Expand any parents of this annotation.
store.setCollapsed(parent, false);
});
}
/**
* Load annotations for all URIs and groupId.
*
......@@ -90,14 +178,14 @@ export default function annotationsService(
function reply(annotation, userid) {
const replyAnnotation = {
group: annotation.group,
permissions: isPublic(annotation)
permissions: metadata.isPublic(annotation)
? sharedPermissions(userid, annotation.group)
: privatePermissions(userid),
references: (annotation.references || []).concat(annotation.id),
target: [{ source: annotation.target[0].source }],
uri: annotation.uri,
};
store.createAnnotation(replyAnnotation);
create(replyAnnotation);
}
/**
......@@ -110,7 +198,7 @@ export default function annotationsService(
const annotationWithChanges = applyDraftChanges(annotation);
if (isNew(annotation)) {
if (metadata.isNew(annotation)) {
saved = api.annotation.create({}, annotationWithChanges);
} else {
saved = api.annotation.update(
......@@ -136,6 +224,7 @@ export default function annotationsService(
}
return {
create,
load,
reply,
save,
......
src/sidebar/services/root-thread.js
View file @ 8142037e
......@@ -39,6 +39,7 @@ const sortFns = {
// @ngInject
export default function RootThread(
$rootScope,
annotationsService,
store,
searchFilter,
viewFilter
......@@ -114,7 +115,7 @@ export default function RootThread(
});
$rootScope.$on(events.BEFORE_ANNOTATION_CREATED, function(event, ann) {
store.createAnnotation(ann);
annotationsService.create(ann);
});
// Remove any annotations that are deleted or unloaded
......
src/sidebar/services/test/root-thread-test.js
View file @ 8142037e
......@@ -20,6 +20,7 @@ const fixtures = immutable({
});
describe('rootThread', function() {
let fakeAnnotationsService;
let fakeStore;
let fakeBuildThread;
let fakeSearchFilter;
......@@ -31,6 +32,9 @@ describe('rootThread', function() {
let rootThread;
beforeEach(function() {
fakeAnnotationsService = {
create: sinon.stub(),
};
fakeStore = {
state: {
annotations: {
......@@ -83,6 +87,7 @@ describe('rootThread', function() {
angular
.module('app', [])
.value('annotationsService', fakeAnnotationsService)
.value('store', fakeStore)
.value('searchFilter', fakeSearchFilter)
.value('settings', fakeSettings)
......@@ -351,10 +356,10 @@ describe('rootThread', function() {
context('when annotation events occur', function() {
const annot = annotationFixtures.defaultAnnotation();
it('creates a new annotation in the store when BEFORE_ANNOTATION_CREATED event occurs', function() {
it('creates a new annotation when BEFORE_ANNOTATION_CREATED event occurs', function() {
$rootScope.$broadcast(events.BEFORE_ANNOTATION_CREATED, annot);
assert.notCalled(fakeStore.removeAnnotations);
assert.calledWith(fakeStore.createAnnotation, sinon.match(annot));
assert.calledWith(fakeAnnotationsService.create, sinon.match(annot));
});
[
......
src/sidebar/store/modules/annotations.js
View file @ 8142037e
......@@ -5,15 +5,10 @@
import { createSelector } from 'reselect';
import uiConstants from '../../ui-constants';
import * as metadata from '../../util/annotation-metadata';
import * as arrayUtil from '../../util/array';
import { defaultPermissions, privatePermissions } from '../../util/permissions';
import * as util from '../util';
import drafts from './drafts';
import selection from './selection';
/**
* Return a copy of `current` with all matching annotations in `annotations`
* removed.
......@@ -241,6 +236,8 @@ function addAnnotations(annotations) {
});
// If we're not in the sidebar, we're done here.
// FIXME Split the annotation-adding from the anchoring code; possibly
// move into service
if (!getState().viewer.isSidebar) {
return;
}
......@@ -320,71 +317,6 @@ function hideAnnotation(id) {
};
}
/**
* Create a new annotation (as-yet unpersisted)
*
* This method has several responsibilities:
* 1. Set some default data attributes on the annotation
* 2. Remove any existing, empty drafts
* 3. Add the annotation to the current collection of annotations
* 4. Change focused tab to the applicable one for the new annotation's meta-type
* 5. Expand all of the new annotation's parents
*
*/
function createAnnotation(ann, now = new Date()) {
return (dispatch, getState) => {
/**
* Extend the new, unsaved annotation object with defaults for some
* required data fields and default permissions.
*
* Note: the `created` and `updated` values will be ignored and superseded
* by the service when the annotation is persisted, but they are used
* app-side for annotation card sorting until then.
*/
const groupid = getState().groups.focusedGroupId;
const userid = getState().session.userid;
ann = Object.assign(
{
created: now.toISOString(),
group: groupid,
permissions: defaultPermissions(
userid,
groupid,
getState().defaults.annotationPrivacy
),
tags: [],
text: '',
updated: now.toISOString(),
user: userid,
user_info: getState().session.user_info,
},
ann
);
// Highlights are peculiar in that they always have private permissions
if (metadata.isHighlight(ann)) {
ann.permissions = privatePermissions(userid);
}
// When a new annotation is created, remove any existing annotations
// that are empty.
dispatch(drafts.actions.deleteNewAndEmptyDrafts([ann]));
dispatch(addAnnotations([ann]));
// If the annotation is of type note or annotation, make sure
// the appropriate tab is selected. If it is of type reply, user
// stays in the selected tab.
if (metadata.isPageNote(ann)) {
dispatch(selection.actions.selectTab(uiConstants.TAB_NOTES));
} else if (metadata.isAnnotation(ann)) {
dispatch(selection.actions.selectTab(uiConstants.TAB_ANNOTATIONS));
}
(ann.references || []).forEach(parent => {
// Expand any parents of this annotation.
dispatch(selection.actions.setCollapsed(parent, false));
});
};
}
/**
* Update the local hidden state of an annotation.
*
......@@ -491,7 +423,6 @@ export default {
actions: {
addAnnotations,
clearAnnotations,
createAnnotation,
hideAnnotation,
removeAnnotations,
updateAnchorStatus,
......
src/sidebar/store/modules/test/annotations-test.js
View file @ 8142037e
import * as fixtures from '../../../test/annotation-fixtures';
import uiConstants from '../../../ui-constants';
import * as metadata from '../../../util/annotation-metadata';
import createStore from '../../create-store';
import annotations from '../annotations';
import { $imports } from '../annotations';
import defaults from '../defaults';
import drafts from '../drafts';
import groups from '../groups';
import selection from '../selection';
import session from '../session';
import viewer from '../viewer';
const { actions, selectors } = annotations;
function createTestStore() {
return createStore(
[annotations, selection, defaults, drafts, groups, session, viewer],
[{}]
);
return createStore([annotations, viewer], [{}]);
}
// Tests for most of the functionality in reducers/annotations.js are currently
// in the tests for the whole Redux store
describe('sidebar/store/modules/annotations', function() {
let fakeDefaultPermissions;
let fakePrivatePermissions;
beforeEach(() => {
fakeDefaultPermissions = sinon.stub();
fakePrivatePermissions = sinon.stub();
$imports.$mock({
'../../util/permissions': {
defaultPermissions: fakeDefaultPermissions,
privatePermissions: fakePrivatePermissions,
},
});
});
afterEach(() => {
$imports.$restore();
});
describe('#addAnnotations()', function() {
const ANCHOR_TIME_LIMIT = 1000;
let clock;
......@@ -72,36 +44,6 @@ describe('sidebar/store/modules/annotations', function() {
]);
});
it('does not change `selectedTab` state if annotations are already loaded', function() {
const annot = fixtures.defaultAnnotation();
store.addAnnotations([annot]);
const page = fixtures.oldPageNote();
store.addAnnotations([page]);
assert.equal(
store.getState().selection.selectedTab,
uiConstants.TAB_ANNOTATIONS
);
});
it('sets `selectedTab` to "note" if only page notes are present', function() {
const page = fixtures.oldPageNote();
store.addAnnotations([page]);
assert.equal(
store.getState().selection.selectedTab,
uiConstants.TAB_NOTES
);
});
it('leaves `selectedTab` as "annotation" if annotations and/or page notes are present', function() {
const page = fixtures.oldPageNote();
const annot = fixtures.defaultAnnotation();
store.addAnnotations([annot, page]);
assert.equal(
store.getState().selection.selectedTab,
uiConstants.TAB_ANNOTATIONS
);
});
it('assigns a local tag to annotations', function() {
const annotA = Object.assign(fixtures.defaultAnnotation(), { id: 'a1' });
const annotB = Object.assign(fixtures.defaultAnnotation(), { id: 'a2' });
......@@ -444,187 +386,4 @@ describe('sidebar/store/modules/annotations', function() {
});
});
});
describe('#createAnnotation', function() {
let clock;
let now;
let store;
beforeEach(() => {
// Stop the clock to keep the current date from advancing
clock = sinon.useFakeTimers();
now = new Date();
store = createTestStore();
});
afterEach(() => {
clock.restore();
});
it('should create an annotation', function() {
const ann = fixtures.oldAnnotation();
store.dispatch(actions.createAnnotation(ann));
assert.equal(
selectors.findAnnotationByID(store.getState(), ann.id).id,
ann.id
);
});
it('should set basic default properties on a new/empty annotation', () => {
store.dispatch(actions.createAnnotation({ id: 'myID' }, now));
const createdAnnotation = selectors.findAnnotationByID(
store.getState(),
'myID'
);
assert.include(createdAnnotation, {
created: now.toISOString(),
updated: now.toISOString(),
text: '',
});
assert.isArray(createdAnnotation.tags);
});
it('should set user properties on a new/empty annotation', () => {
store.dispatch(actions.createAnnotation({ id: 'myID' }, now));
const createdAnnotation = selectors.findAnnotationByID(
store.getState(),
'myID'
);
assert.equal(createdAnnotation.user, store.getState().session.userid);
assert.equal(
createdAnnotation.user_info,
store.getState().session.user_info
);
});
it('should set default permissions on a new annotation', () => {
fakeDefaultPermissions.returns('somePermissions');
store.dispatch(actions.createAnnotation({ id: 'myID' }, now));
const createdAnnotation = selectors.findAnnotationByID(
store.getState(),
'myID'
);
assert.equal(createdAnnotation.permissions, 'somePermissions');
});
it('should always assign private permissions to highlights', () => {
fakePrivatePermissions.returns('private');
store.dispatch(
actions.createAnnotation({ id: 'myID', $highlight: true }, now)
);
const createdAnnotation = selectors.findAnnotationByID(
store.getState(),
'myID'
);
assert.equal(createdAnnotation.permissions, 'private');
});
it('should set group to currently-focused group if not set on annotation', () => {
store.dispatch(actions.createAnnotation({ id: 'myID' }, now));
const createdAnnotation = selectors.findAnnotationByID(
store.getState(),
'myID'
);
assert.equal(
createdAnnotation.group,
store.getState().groups.focusedGroupId
);
});
it('should set not overwrite properties if present', () => {
store.dispatch(
actions.createAnnotation(
{
id: 'myID',
created: 'when',
updated: 'then',
text: 'my annotation',
tags: ['foo', 'bar'],
group: 'fzzy',
permissions: ['whatever'],
user: 'acct:foo@bar.com',
user_info: {
display_name: 'Herbivore Fandango',
},
},
now
)
);
const createdAnnotation = selectors.findAnnotationByID(
store.getState(),
'myID'
);
assert.include(createdAnnotation, {
created: 'when',
updated: 'then',
text: 'my annotation',
group: 'fzzy',
user: 'acct:foo@bar.com',
});
assert.include(createdAnnotation.tags, 'foo', 'bar');
assert.include(createdAnnotation.permissions, 'whatever');
assert.equal(
createdAnnotation.user_info.display_name,
'Herbivore Fandango'
);
});
it('should change tab focus to TAB_ANNOTATIONS when a new annotation is created', function() {
store.dispatch(actions.createAnnotation(fixtures.oldAnnotation()));
assert.equal(
store.getState().selection.selectedTab,
uiConstants.TAB_ANNOTATIONS
);
});
it('should change tab focus to TAB_NOTES when a new note annotation is created', function() {
store.dispatch(actions.createAnnotation(fixtures.oldPageNote()));
assert.equal(
store.getState().selection.selectedTab,
uiConstants.TAB_NOTES
);
});
it('should expand parent of created annotation', function() {
const store = createTestStore();
store.dispatch(
actions.addAnnotations([
{
id: 'annotation_id',
$highlight: undefined,
target: [{ source: 'source', selector: [] }],
references: [],
text: 'This is my annotation',
tags: ['tag_1', 'tag_2'],
},
])
);
// Collapse the parent.
store.dispatch(selection.actions.setCollapsed('annotation_id', true));
// Creating a new child annotation should expand its parent.
store.dispatch(
actions.createAnnotation({
highlight: undefined,
target: [{ source: 'http://example.org' }],
references: ['annotation_id'],
text: '',
tags: [],
})
);
assert.isTrue(store.getState().selection.expanded.annotation_id);
});
});
});
src/sidebar/test/integration/threading-test.js
View file @ 8142037e
......@@ -57,6 +57,7 @@ describe('annotation threading', function() {
.service('store', storeFactory)
.service('rootThread', rootThreadFactory)
.service('searchFilter', searchFilterFactory)
.service('annotationsService', () => {})
.service('viewFilter', viewFilterFactory)
.value('features', fakeFeatures)
.value('settings', {})
......
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