Fix: annotation mode exits on wrong button and fetch annotations retu…

…rns non-promise (#363) * Fix: return promise * Fix: disable modes except
box · Sep 3, 2017 · aed6288 · aed6288
1 parent 5d39f1f
commit aed6288
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 9 deletions.
diff --git a/src/lib/annotations/Annotator.js b/src/lib/annotations/Annotator.js
@@ -369,7 +369,7 @@ class Annotator extends EventEmitter {
         const buttonEl = event.target || this.getAnnotateButton(buttonSelector);
 
         // Exit any other annotation mode
-        this.exitAnnotationModes(mode, buttonEl);
+        this.exitAnnotationModesExcept(mode);
 
         // If in annotation mode, turn it off
         if (this.isInAnnotationMode(mode)) {
@@ -458,18 +458,20 @@ class Annotator extends EventEmitter {
      * Exits all annotation modes except the specified mode
      *
      * @param {string} mode - Current annotation mode
-     * @param {HTMLElement} buttonEl - Annotation mode button DOM element
      * @return {void}
      */
-    exitAnnotationModes(mode, buttonEl) {
+    exitAnnotationModesExcept(mode) {
         Object.keys(this.modeButtons).forEach((type) => {
             if (mode === type) {
                 return;
             }
 
             const buttonSelector = this.modeButtons[type].selector;
-            const modeButtonEl = buttonEl || this.getAnnotateButton(buttonSelector);
-            this.disableAnnotationMode(type, modeButtonEl);
+            if (!this.modeButtons[type].button) {
+                this.modeButtons[type].button = this.getAnnotateButton(buttonSelector);
+            }
+
+            this.disableAnnotationMode(type, this.modeButtons[type].button);
         });
     }
 
@@ -543,7 +545,7 @@ class Annotator extends EventEmitter {
         // Do not load any pre-existing annotations if the user does not have
         // the correct permissions
         if (!this.permissions.canViewAllAnnotations || !this.permissions.canViewOwnAnnotations) {
-            return this.threads;
+            return Promise.resolve(this.threads);
         }
 
         return this.annotationService.getThreadMap(this.fileVersionId).then((threadMap) => {

diff --git a/src/lib/annotations/__tests__/Annotator-test.js b/src/lib/annotations/__tests__/Annotator-test.js
@@ -288,11 +288,31 @@ describe('lib/annotations/Annotator', () => {
             });
         });
 
+        describe('exitAnnotationModesExcept()', () => {
+            it('should call disableAnnotationMode on all modes except the specified one', () => {
+                annotator.modeButtons = {
+                    'type1': {
+                        selector: 'bogus',
+                        button: 'button1'
+                    },
+                    'type2': {
+                        selector: 'test',
+                        button: 'button2'
+                    }
+                };
+
+                sandbox.stub(annotator, 'disableAnnotationMode');
+                annotator.exitAnnotationModesExcept('type2');
+                expect(annotator.disableAnnotationMode).to.be.calledWith('type1', 'button1');
+                expect(annotator.disableAnnotationMode).to.not.be.calledWith('type2', 'button2');
+            });
+        });
+
         describe('toggleAnnotationHandler()', () => {
             beforeEach(() => {
                 stubs.destroyStub = sandbox.stub(annotator, 'destroyPendingThreads');
                 stubs.annotationMode = sandbox.stub(annotator, 'isInAnnotationMode');
-                stubs.exitModes = sandbox.stub(annotator, 'exitAnnotationModes');
+                stubs.exitModes = sandbox.stub(annotator, 'exitAnnotationModesExcept');
                 stubs.disable = sandbox.stub(annotator, 'disableAnnotationMode');
                 stubs.enable = sandbox.stub(annotator, 'enableAnnotationMode');
                 sandbox.stub(annotator, 'getAnnotateButton');
@@ -434,13 +454,22 @@ describe('lib/annotations/Annotator', () => {
                     canViewAllAnnotations: false,
                     canViewOwnAnnotations: true
                 };
-                annotator.fetchAnnotations();
+                let result = annotator.fetchAnnotations();
+                expect(result instanceof Promise).to.be.truthy;
 
                 annotator.permissions = {
                     canViewAllAnnotations: true,
                     canViewOwnAnnotations: false
                 };
-                annotator.fetchAnnotations();
+                result = annotator.fetchAnnotations();
+                expect(result instanceof Promise).to.be.truthy;
+
+                annotator.permissions = {
+                    canViewAllAnnotations: false,
+                    canViewOwnAnnotations: false
+                };
+                result = annotator.fetchAnnotations();
+                expect(result instanceof Promise).to.be.truthy;
             });
 
             it('should reset and create a new thread map by fetching annotation data from the server', () => {