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.

Sign up for GitHub

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

Chore: Adding console errors for annotation errors #388

Merged
merged 5 commits into from
Sep 13, 2017
Merged

Chore: Adding console errors for annotation errors #388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
efc0467
Chore: Adding console errors for annotation errors
pramodsum Sep 12, 2017
2690ddc
Chore: Disabling eslint around console.error statements
pramodsum Sep 12, 2017
085725f
Chore: pr fixes
pramodsum Sep 12, 2017
c9743f5
Fix: tests
pramodsum Sep 12, 2017
23630ff
Merge branch 'master' into master
pramodsum Sep 12, 2017
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
33 changes: 21 additions & 12 deletions src/lib/annotations/AnnotationService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,17 +110,20 @@ class AnnotationService extends EventEmitter {

resolve(createdAnnotation);
} else {
reject(new Error('Could not create annotation'));
const error = new Error('Could not create annotation');
reject(error);
this.emit('annotationerror', {
reason: 'create'
reason: 'create',
error: error.toString()
});
}
})
/* istanbul ignore next */
.catch(() => {
.catch((error) => {
reject(new Error('Could not create annotation due to invalid or expired token'));
this.emit('annotationerror', {
reason: 'authorization'
reason: 'authorization',
error: error.toString()
});
});
});
Expand Down Expand Up @@ -161,17 +164,20 @@ class AnnotationService extends EventEmitter {
if (response.status === 204) {
resolve();
} else {
reject(new Error(`Could not delete annotation with ID ${annotationID}`));
const error = new Error(`Could not delete annotation with ID ${annotationID}`);
reject(error);
this.emit('annotationerror', {
reason: 'delete'
reason: 'delete',
error: error.toString()
});
}
})
/* istanbul ignore next */
.catch(() => {
.catch((error) => {
reject(new Error('Could not delete annotation due to invalid or expired token'));
this.emit('annotationerror', {
reason: 'authorization'
reason: 'authorization',
error: error.toString()
});
});
});
Expand Down Expand Up @@ -287,9 +293,11 @@ class AnnotationService extends EventEmitter {
.then((response) => response.json())
.then((data) => {
if (data.type === 'error' || !Array.isArray(data.entries)) {
reject(new Error(`Could not read annotations from file version with ID ${fileVersionId}`));
const error = new Error(`Could not read annotations from file version with ID ${fileVersionId}`);
reject(error);
this.emit('annotationerror', {
reason: 'read'
reason: 'read',
error: error.toString()
});
} else {
data.entries.forEach((annotationData) => {
Expand All @@ -303,10 +311,11 @@ class AnnotationService extends EventEmitter {
}
}
})
.catch(() => {
.catch((error) => {
reject(new Error('Could not read annotations from file due to invalid or expired token'));
this.emit('annotationerror', {
reason: 'authorization'
reason: 'authorization',
error: error.toString()
});
});
}
Expand Down
40 changes: 36 additions & 4 deletions src/lib/annotations/AnnotationThread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ class AnnotationThread extends EventEmitter {
return this.annotationService
.create(annotationData)
.then((savedAnnotation) => this.updateTemporaryAnnotation(tempAnnotation, savedAnnotation))
.catch(() => this.handleThreadSaveError(tempAnnotationID));
.catch((error) => this.handleThreadSaveError(error, tempAnnotationID));
}

/**
Expand All @@ -171,9 +171,27 @@ class AnnotationThread extends EventEmitter {
deleteAnnotation(annotationID, useServer = true) {
// Ignore if no corresponding annotation exists in thread or user doesn't have permissions
const annotation = this.annotations.find((annot) => annot.annotationID === annotationID);
if (!annotation || (annotation.permissions && !annotation.permissions.can_delete)) {
if (!annotation) {
// Broadcast error
this.emit(THREAD_EVENT.deleteError);
/* eslint-disable no-console */
console.error(
THREAD_EVENT.deleteError,
`Annotation with ID ${annotation.threadNumber} could not be found.`
);
/* eslint-enable no-console */
return Promise.reject();
}

if (annotation.permissions && !annotation.permissions.can_delete) {
// Broadcast error
this.emit(THREAD_EVENT.deleteError);
/* eslint-disable no-console */
console.error(
THREAD_EVENT.deleteError,
`User does not have the correct permissions to delete annotation with ID ${annotation.threadNumber}.`
);
/* eslint-enable no-console */
return Promise.reject();
}

Expand Down Expand Up @@ -203,6 +221,12 @@ class AnnotationThread extends EventEmitter {
}

if (!useServer) {
/* eslint-disable no-console */
console.error(
THREAD_EVENT.deleteError,
`Annotation with ID ${annotation.threadNumber} not deleted from server`
);
/* eslint-enable no-console */
return Promise.resolve();
}

Expand All @@ -228,9 +252,12 @@ class AnnotationThread extends EventEmitter {
// Broadcast annotation deletion event
this.emit(THREAD_EVENT.delete);
})
.catch(() => {
.catch((error) => {
// Broadcast error
this.emit(THREAD_EVENT.deleteError);
/* eslint-disable no-console */
console.error(THREAD_EVENT.deleteError, error.toString());
/* eslint-enable no-console */
});
}

Expand Down Expand Up @@ -588,15 +615,20 @@ class AnnotationThread extends EventEmitter {
* Deletes the temporary annotation if the annotation failed to save on the server
*
* @private
* @param {error} error - error thrown while saving the annotation
* @param {string} tempAnnotationID - ID of temporary annotation to be updated with annotation from server
* @return {void}
*/
handleThreadSaveError(tempAnnotationID) {
handleThreadSaveError(error, tempAnnotationID) {
// Remove temporary annotation
this.deleteAnnotation(tempAnnotationID, /* useServer */ false);

// Broadcast error
this.emit(THREAD_EVENT.createError);

/* eslint-disable no-console */
console.error(THREAD_EVENT.createError, error.toString());
/* eslint-enable no-console */
}

/**
Expand Down
9 changes: 9 additions & 0 deletions src/lib/annotations/Annotator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,12 @@ class Annotator extends EventEmitter {
default:
}

if (data.error) {
/* eslint-disable no-console */
console.error(ANNOTATOR_EVENT.error, data.error);
/* eslint-enable no-console */
}

if (errorMessage) {
this.emit(ANNOTATOR_EVENT.error, errorMessage);
}
Expand Down Expand Up @@ -935,6 +941,9 @@ class Annotator extends EventEmitter {
}

this.emit(ANNOTATOR_EVENT.error, __('annotations_load_error'));
/* eslint-disable no-console */
console.error('Annotation could not be created due to invalid params');
/* eslint-enable no-console */
this.validationErrorEmitted = true;
}

Expand Down
12 changes: 8 additions & 4 deletions src/lib/annotations/__tests__/AnnotationService-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ describe('lib/annotations/AnnotationService', () => {
(error) => {
expect(error.message).to.equal('Could not create annotation');
expect(emitStub).to.be.calledWith('annotationerror', {
reason: 'create'
reason: 'create',
error: sinon.match.string
});
}
);
Expand Down Expand Up @@ -214,7 +215,8 @@ describe('lib/annotations/AnnotationService', () => {
(error) => {
expect(error.message).to.equal('Could not delete annotation with ID 3');
expect(emitStub).to.be.calledWith('annotationerror', {
reason: 'delete'
reason: 'delete',
error: sinon.match.string
});
}
);
Expand Down Expand Up @@ -415,7 +417,8 @@ describe('lib/annotations/AnnotationService', () => {
(error) => {
expect(error.message).to.equal('Could not read annotations from file version with ID 2');
expect(emitStub).to.be.calledWith('annotationerror', {
reason: 'read'
reason: 'read',
error: sinon.match.string
});
}
);
Expand All @@ -439,7 +442,8 @@ describe('lib/annotations/AnnotationService', () => {
return promise.catch((error) => {
expect(error.message).to.equal('Could not read annotations from file due to invalid or expired token');
expect(emitStub).to.be.calledWith('annotationerror', {
reason: 'authorization'
reason: 'authorization',
error: sinon.match.string
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion src/lib/annotations/__tests__/AnnotationThread-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -773,7 +773,7 @@ describe('lib/annotations/AnnotationThread', () => {
describe('handleThreadSaveError()', () => {
it('should delete temp annotation and emit event', () => {
sandbox.stub(thread, 'deleteAnnotation');
thread.handleThreadSaveError(1);
thread.handleThreadSaveError(new Error(), 1);
expect(thread.deleteAnnotation).to.be.calledWith(1, false);
expect(thread.emit).to.be.calledWith(THREAD_EVENT.createError);
});
Expand Down
6 changes: 3 additions & 3 deletions src/lib/annotations/doc/DocAnnotator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,9 @@ class DocAnnotator extends Annotator {
const highlightThreads = this.getHighlightThreadsOnPage(pageNum);
highlightThreads.forEach((thread) => {
if (annotatorUtil.isPending(thread.state)) {
/* eslint-disable no-console */
console.error('Pending annotation thread destroyed', thread.threadNumber);
/* eslint-enable no-console */
thread.destroy();
}
});
Expand Down Expand Up @@ -861,20 +864,17 @@ class DocAnnotator extends Annotator {

// Select page of first node selected
const { pageEl } = annotatorUtil.getPageInfo(selection.anchorNode);

if (!pageEl) {
return;
}

const lastRange = selection.getRangeAt(selection.rangeCount - 1);
const rects = lastRange.getClientRects();

if (rects.length === 0) {
return;
}

const { right, bottom } = rects[rects.length - 1];

const pageDimensions = pageEl.getBoundingClientRect();
const pageLeft = pageDimensions.left;
const pageTop = pageDimensions.top + PAGE_PADDING_TOP;
Expand Down
4 changes: 3 additions & 1 deletion src/lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,9 @@ export function get(url, ...rest) {
break;
}

return fetch(url, { headers }).then(checkStatus).then(parser);
return fetch(url, { headers })
.then(checkStatus)
.then(parser);
}

/**
Expand Down
6 changes: 5 additions & 1 deletion src/lib/viewers/BaseViewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,11 @@ class BaseViewer extends EventEmitter {
}

if (this.annotationsPromise) {
this.annotationsPromise.then(this.loadAnnotator);
this.annotationsPromise.then(this.loadAnnotator).catch((error) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

newline the catch()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our linter puts it back in one line 😛

/* eslint-disable no-console */
console.error('Annotation assets failed to load', error.toString());
/* eslint-enable no-console */
});
}
});
}
Expand Down