From efc0467c85a4c40b718a3eba8538a925b7ea629e Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Tue, 12 Sep 2017 13:27:07 -0700 Subject: [PATCH 1/4] Chore: Adding console errors for annotation errors --- src/lib/annotations/AnnotationService.js | 15 +++++++----- src/lib/annotations/AnnotationThread.js | 29 ++++++++++++++++++++---- src/lib/annotations/Annotator.js | 5 ++++ src/lib/annotations/doc/DocAnnotator.js | 4 +--- src/lib/util.js | 4 +++- src/lib/viewers/BaseViewer.js | 4 +++- 6 files changed, 46 insertions(+), 15 deletions(-) diff --git a/src/lib/annotations/AnnotationService.js b/src/lib/annotations/AnnotationService.js index 74091bf04..f0704e9a0 100644 --- a/src/lib/annotations/AnnotationService.js +++ b/src/lib/annotations/AnnotationService.js @@ -117,10 +117,11 @@ class AnnotationService extends EventEmitter { } }) /* 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 }); }); }); @@ -168,10 +169,11 @@ class AnnotationService extends EventEmitter { } }) /* 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 }); }); }); @@ -303,10 +305,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 }); }); } diff --git a/src/lib/annotations/AnnotationThread.js b/src/lib/annotations/AnnotationThread.js index 497a674d4..7ee7fe2b8 100644 --- a/src/lib/annotations/AnnotationThread.js +++ b/src/lib/annotations/AnnotationThread.js @@ -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)); } /** @@ -171,9 +171,23 @@ 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); + console.error( + THREAD_EVENT.deleteError, + `Annotation with ID ${annotation.threadNumber} could not be found.` + ); + return Promise.reject(); + } + + if (annotation.permissions && !annotation.permissions.can_delete) { + // Broadcast error + this.emit(THREAD_EVENT.deleteError); + console.error( + THREAD_EVENT.deleteError, + `User does not have the correct permissions to delete annotation with ID ${annotation.threadNumber}.` + ); return Promise.reject(); } @@ -203,6 +217,10 @@ class AnnotationThread extends EventEmitter { } if (!useServer) { + console.error( + THREAD_EVENT.deleteError, + `Annotation with ID ${annotation.threadNumber} not deleted from server` + ); return Promise.resolve(); } @@ -228,9 +246,10 @@ class AnnotationThread extends EventEmitter { // Broadcast annotation deletion event this.emit(THREAD_EVENT.delete); }) - .catch(() => { + .catch((error) => { // Broadcast error this.emit(THREAD_EVENT.deleteError); + console.error(THREAD_EVENT.deleteError, error.toString()); }); } @@ -588,15 +607,17 @@ 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); + console.error(THREAD_EVENT.createError, error.toString()); } /** diff --git a/src/lib/annotations/Annotator.js b/src/lib/annotations/Annotator.js index 4e2408dfe..86045d4bc 100644 --- a/src/lib/annotations/Annotator.js +++ b/src/lib/annotations/Annotator.js @@ -652,6 +652,10 @@ class Annotator extends EventEmitter { default: } + if (data.error) { + console.error(ANNOTATOR_EVENT.error, data.error.toString()); + } + if (errorMessage) { this.emit(ANNOTATOR_EVENT.error, errorMessage); } @@ -935,6 +939,7 @@ class Annotator extends EventEmitter { } this.emit(ANNOTATOR_EVENT.error, __('annotations_load_error')); + console.error('Annotation could not be created due to invalid params'); this.validationErrorEmitted = true; } diff --git a/src/lib/annotations/doc/DocAnnotator.js b/src/lib/annotations/doc/DocAnnotator.js index d631e7798..8885d433e 100644 --- a/src/lib/annotations/doc/DocAnnotator.js +++ b/src/lib/annotations/doc/DocAnnotator.js @@ -463,6 +463,7 @@ class DocAnnotator extends Annotator { const highlightThreads = this.getHighlightThreadsOnPage(pageNum); highlightThreads.forEach((thread) => { if (annotatorUtil.isPending(thread.state)) { + console.error('Pending annotation thread destroyed', thread.threadNumber); thread.destroy(); } }); @@ -861,20 +862,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; diff --git a/src/lib/util.js b/src/lib/util.js index 64dc3e492..4e3207702 100644 --- a/src/lib/util.js +++ b/src/lib/util.js @@ -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); } /** diff --git a/src/lib/viewers/BaseViewer.js b/src/lib/viewers/BaseViewer.js index 8a565b740..a48715441 100644 --- a/src/lib/viewers/BaseViewer.js +++ b/src/lib/viewers/BaseViewer.js @@ -346,7 +346,9 @@ class BaseViewer extends EventEmitter { } if (this.annotationsPromise) { - this.annotationsPromise.then(this.loadAnnotator); + this.annotationsPromise.then(this.loadAnnotator).catch((error) => { + console.error('Annotation assets failed to load', error.toString()); + }); } }); } From 2690ddcaf15c39c189be3b2ec54992b0662ba9a9 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Tue, 12 Sep 2017 16:29:13 -0700 Subject: [PATCH 2/4] Chore: Disabling eslint around console.error statements --- src/lib/annotations/AnnotationThread.js | 10 ++++++++++ src/lib/annotations/Annotator.js | 4 ++++ src/lib/annotations/doc/DocAnnotator.js | 2 ++ src/lib/viewers/BaseViewer.js | 2 ++ 4 files changed, 18 insertions(+) diff --git a/src/lib/annotations/AnnotationThread.js b/src/lib/annotations/AnnotationThread.js index 7ee7fe2b8..58af322f8 100644 --- a/src/lib/annotations/AnnotationThread.js +++ b/src/lib/annotations/AnnotationThread.js @@ -174,20 +174,24 @@ class AnnotationThread extends EventEmitter { 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(); } @@ -217,10 +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(); } @@ -249,7 +255,9 @@ class AnnotationThread extends EventEmitter { .catch((error) => { // Broadcast error this.emit(THREAD_EVENT.deleteError); + /* eslint-disable no-console */ console.error(THREAD_EVENT.deleteError, error.toString()); + /* eslint-enable no-console */ }); } @@ -617,7 +625,9 @@ class AnnotationThread extends EventEmitter { // Broadcast error this.emit(THREAD_EVENT.createError); + /* eslint-disable no-console */ console.error(THREAD_EVENT.createError, error.toString()); + /* eslint-enable no-console */ } /** diff --git a/src/lib/annotations/Annotator.js b/src/lib/annotations/Annotator.js index 86045d4bc..f32bbe7c2 100644 --- a/src/lib/annotations/Annotator.js +++ b/src/lib/annotations/Annotator.js @@ -653,7 +653,9 @@ class Annotator extends EventEmitter { } if (data.error) { + /* eslint-disable no-console */ console.error(ANNOTATOR_EVENT.error, data.error.toString()); + /* eslint-enable no-console */ } if (errorMessage) { @@ -939,7 +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; } diff --git a/src/lib/annotations/doc/DocAnnotator.js b/src/lib/annotations/doc/DocAnnotator.js index 8885d433e..f5cf659de 100644 --- a/src/lib/annotations/doc/DocAnnotator.js +++ b/src/lib/annotations/doc/DocAnnotator.js @@ -463,7 +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(); } }); diff --git a/src/lib/viewers/BaseViewer.js b/src/lib/viewers/BaseViewer.js index a48715441..76b5d4f12 100644 --- a/src/lib/viewers/BaseViewer.js +++ b/src/lib/viewers/BaseViewer.js @@ -347,7 +347,9 @@ class BaseViewer extends EventEmitter { if (this.annotationsPromise) { this.annotationsPromise.then(this.loadAnnotator).catch((error) => { + /* eslint-disable no-console */ console.error('Annotation assets failed to load', error.toString()); + /* eslint-enable no-console */ }); } }); From 085725fc1f1a02c767c7311489eab34feb88fd54 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Tue, 12 Sep 2017 16:39:34 -0700 Subject: [PATCH 3/4] Chore: pr fixes From c9743f55525cb05299d66e41bb8612bdf10175b1 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Tue, 12 Sep 2017 16:54:32 -0700 Subject: [PATCH 4/4] Fix: tests --- src/lib/annotations/AnnotationService.js | 24 ++++++++++++------- src/lib/annotations/AnnotationThread.js | 1 + src/lib/annotations/Annotator.js | 2 +- .../__tests__/AnnotationService-test.js | 12 ++++++---- .../__tests__/AnnotationThread-test.js | 2 +- 5 files changed, 26 insertions(+), 15 deletions(-) diff --git a/src/lib/annotations/AnnotationService.js b/src/lib/annotations/AnnotationService.js index f0704e9a0..f3be99709 100644 --- a/src/lib/annotations/AnnotationService.js +++ b/src/lib/annotations/AnnotationService.js @@ -110,9 +110,11 @@ 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() }); } }) @@ -121,7 +123,7 @@ class AnnotationService extends EventEmitter { reject(new Error('Could not create annotation due to invalid or expired token')); this.emit('annotationerror', { reason: 'authorization', - error + error: error.toString() }); }); }); @@ -162,9 +164,11 @@ 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() }); } }) @@ -173,7 +177,7 @@ class AnnotationService extends EventEmitter { reject(new Error('Could not delete annotation due to invalid or expired token')); this.emit('annotationerror', { reason: 'authorization', - error + error: error.toString() }); }); }); @@ -289,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) => { @@ -309,7 +315,7 @@ class AnnotationService extends EventEmitter { reject(new Error('Could not read annotations from file due to invalid or expired token')); this.emit('annotationerror', { reason: 'authorization', - error + error: error.toString() }); }); } diff --git a/src/lib/annotations/AnnotationThread.js b/src/lib/annotations/AnnotationThread.js index 58af322f8..83a4b33d2 100644 --- a/src/lib/annotations/AnnotationThread.js +++ b/src/lib/annotations/AnnotationThread.js @@ -625,6 +625,7 @@ class AnnotationThread extends EventEmitter { // Broadcast error this.emit(THREAD_EVENT.createError); + /* eslint-disable no-console */ console.error(THREAD_EVENT.createError, error.toString()); /* eslint-enable no-console */ diff --git a/src/lib/annotations/Annotator.js b/src/lib/annotations/Annotator.js index f32bbe7c2..19adada8c 100644 --- a/src/lib/annotations/Annotator.js +++ b/src/lib/annotations/Annotator.js @@ -654,7 +654,7 @@ class Annotator extends EventEmitter { if (data.error) { /* eslint-disable no-console */ - console.error(ANNOTATOR_EVENT.error, data.error.toString()); + console.error(ANNOTATOR_EVENT.error, data.error); /* eslint-enable no-console */ } diff --git a/src/lib/annotations/__tests__/AnnotationService-test.js b/src/lib/annotations/__tests__/AnnotationService-test.js index 4bab59f92..e7e7199fa 100644 --- a/src/lib/annotations/__tests__/AnnotationService-test.js +++ b/src/lib/annotations/__tests__/AnnotationService-test.js @@ -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 }); } ); @@ -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 }); } ); @@ -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 }); } ); @@ -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 }); }); }); diff --git a/src/lib/annotations/__tests__/AnnotationThread-test.js b/src/lib/annotations/__tests__/AnnotationThread-test.js index 805ae171b..86f4c5098 100644 --- a/src/lib/annotations/__tests__/AnnotationThread-test.js +++ b/src/lib/annotations/__tests__/AnnotationThread-test.js @@ -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); });