Fix: Invalidate draw/highlight annotations without properly structured locations (#9)

pramodsum · web-flow · commit 39a94934928c · 2017-10-30T10:51:00.000-07:00
- Fix: Create draw threads for controller setup without validation
- Update: Cleaning up validation methods
diff --git a/src/Annotator.js b/src/Annotator.js
@@ -517,6 +517,39 @@ class Annotator extends EventEmitter {
         service.removeListener(ANNOTATOR_EVENT.error, this.handleServiceEvents);
     }
 
+    /**
+     * Gets thread params for the new annotation thread
+     *
+     * @param {Annotation[]} annotations - Annotations in thread
+     * @param {Object} location - Location object
+     * @param {string} [type] - Optional annotation type
+     * @return {AnnotationThread} Created annotation thread
+     */
+    getThreadParams(annotations, location, type) {
+        const params = {
+            annotatedElement: this.annotatedElement,
+            annotations,
+            annotationService: this.annotationService,
+            container: this.container,
+            fileVersionId: this.fileVersionId,
+            isMobile: this.isMobile,
+            hasTouch: this.hasTouch,
+            locale: this.locale,
+            location,
+            type,
+            permissions: this.permissions,
+            localized: this.localized
+        };
+
+        // Set existing thread ID if created with annotations
+        if (annotations.length > 0) {
+            params.threadID = annotations[0].threadID;
+            params.threadNumber = annotations[0].threadNumber;
+        }
+
+        return params;
+    }
+
     /**
      * Binds custom event listeners for a thread.
      *
diff --git a/src/__tests__/annotatorUtil-test.js b/src/__tests__/annotatorUtil-test.js
@@ -19,7 +19,10 @@ import {
     htmlEscape,
     repositionCaret,
     isPending,
-    validateThreadParams,
+    isPointLocationValid,
+    isHighlightLocationValid,
+    isDrawLocationValid,
+    areThreadParamsValid,
     eventToLocationHandler,
     decodeKeydown,
     getHeaders,
@@ -411,23 +414,64 @@ describe('annotatorUtil', () => {
         });
     });
 
-    describe('validateThreadParams()', () => {
+    describe('isPointLocationValid()', () => {
         it('should return false if the thread is null or missing any expected params', () => {
-            expect(validateThreadParams(null)).to.be.false;
-            expect(validateThreadParams({ fileVersionId: 123 })).to.be.false;
+            expect(isPointLocationValid({})).to.be.false;
+            expect(isPointLocationValid({ x: 1, y: 2 })).to.be.true;
         });
+    });
 
-        it('should return true if the thread is has all expected params', () => {
+    describe('isHighlightLocationValid()', () => {
+        it('should return false if the thread is null or missing any expected params', () => {
+            expect(isHighlightLocationValid({})).to.be.false;
+            expect(isHighlightLocationValid({ quadPoints: {} })).to.be.true;
+        });
+    });
+
+    describe('isDrawLocationValid()', () => {
+        it('should return false if the thread is null or missing any expected params', () => {
+            expect(isDrawLocationValid({})).to.be.false;
+            expect(isDrawLocationValid({ minX: 1, minY: 1, maxX: 2, maxY: 2 })).to.be.true;
+        });
+    });
+
+    describe('areThreadParamsValid()', () => {
+        it('should return false if the thread is null or missing any expected params', () => {
+            expect(areThreadParamsValid(null)).to.be.false;
+            expect(areThreadParamsValid({ fileVersionId: 123 })).to.be.false;
+        });
+
+        it('should return false if thread has invalid location', () => {
             const threadParams = {
                 annotatedElement: {},
                 annotations: [],
                 annotationService: {},
                 fileVersionId: 123,
                 location: {},
+                locale: 'en-US'
+            };
+
+            threadParams.type = TYPES.point;
+            expect(areThreadParamsValid(threadParams)).to.be.false;
+
+            threadParams.type = TYPES.highlight;
+            expect(areThreadParamsValid(threadParams)).to.be.false;
+
+            threadParams.type = TYPES.draw;
+            expect(areThreadParamsValid(threadParams)).to.be.false;
+        });
+
+        it('should return true if the thread is has all expected params', () => {
+            const threadParams = {
+                annotatedElement: {},
+                annotations: [],
+                annotationService: {},
+                fileVersionId: 123,
+                location: { x: 1, y: 2 },
                 locale: 'en-US',
-                type: 'point'
+                type: TYPES.point
             };
-            expect(validateThreadParams(threadParams)).to.be.true;
+            expect(areThreadParamsValid(threadParams)).to.be.true;
         });
     });
 
diff --git a/src/annotatorUtil.js b/src/annotatorUtil.js
@@ -479,15 +479,52 @@ export function isPending(threadState) {
     return PENDING_STATES.indexOf(threadState) > -1;
 }
 
+/**
+ * Checks whether a point annotation thread has the correct location params
+ *
+ * @param {Object} location Point annotation thread location object
+ * @return {boolean} Whether or not the point annotation has the correct location information
+ */
+export function isPointLocationValid(location) {
+    return !!(location && location.x && location.y);
+}
+
+/**
+ * Checks whether a highlight annotation thread has the correct location params
+ *
+ * @param {Object} location Highlight annotation thread location object
+ * @return {boolean} Whether or not the highlight annotation has the correct location information
+ */
+export function isHighlightLocationValid(location) {
+    return !!(location && location.quadPoints);
+}
+
+/**
+ * Checks whether a draw annotation thread has the correct location params
+ *
+ * @param {Object} location Draw annotation thread location object
+ * @return {boolean} Whether or not the draw annotation has the correct location information
+ */
+export function isDrawLocationValid(location) {
+    return !!(location && location.minX && location.minY && location.maxX && location.maxY);
+}
+
 /**
  * Checks whether annotation thread is valid by checking whether each property
  * in THREAD_PARAMS on the specified file object is defined.
  *
  * @param {Object} thread Annotation thread params to check
  * @return {boolean} Whether or not annotation thread has all the required params
  */
-export function validateThreadParams(thread) {
+export function areThreadParamsValid(thread) {
     if (thread) {
+        if (
+            (thread.type === TYPES.point && !isPointLocationValid(thread.location)) ||
+            (isHighlightAnnotation(thread.type) && !isHighlightLocationValid(thread.location)) ||
+            (thread.type === TYPES.draw && !isDrawLocationValid(thread.location))
+        ) {
+            return false;
+        }
         return THREAD_PARAMS.every((param) => typeof thread[param] !== 'undefined');
     }
     return false;
diff --git a/src/doc/DocAnnotator.js b/src/doc/DocAnnotator.js
@@ -322,28 +322,8 @@ class DocAnnotator extends Annotator {
      */
     createAnnotationThread(annotations, location, type) {
         let thread;
-        const threadParams = {
-            annotatedElement: this.annotatedElement,
-            annotations,
-            annotationService: this.annotationService,
-            container: this.container,
-            fileVersionId: this.fileVersionId,
-            isMobile: this.isMobile,
-            hasTouch: this.hasTouch,
-            locale: this.locale,
-            location,
-            type,
-            permissions: this.permissions,
-            localized: this.localized
-        };
-
-        // Set existing thread ID if created with annotations
-        if (annotations.length > 0) {
-            threadParams.threadID = annotations[0].threadID;
-            threadParams.threadNumber = annotations[0].threadNumber;
-        }
-
-        if (!annotatorUtil.validateThreadParams(threadParams)) {
+        const threadParams = this.getThreadParams(annotations, location, type);
+        if (!annotatorUtil.areThreadParamsValid(threadParams)) {
             this.handleValidationError();
             return thread;
         }
@@ -356,8 +336,8 @@ class DocAnnotator extends Annotator {
             thread = new DocPointThread(threadParams);
         }
 
-        if (!thread && this.notification) {
-            this.emit(ANNOTATOR_EVENT.error, this.localized.createError);
+        if (!thread) {
+            this.emit(ANNOTATOR_EVENT.error, this.localized.loadError);
         } else if (thread && (type !== TYPES.draw || location.page)) {
             this.addThreadToMap(thread);
         }
diff --git a/src/doc/__tests__/DocAnnotator-test.js b/src/doc/__tests__/DocAnnotator-test.js
@@ -11,6 +11,7 @@ import { CreateEvents } from '../CreateHighlightDialog';
 import * as annotatorUtil from '../../annotatorUtil';
 import * as docAnnotatorUtil from '../docAnnotatorUtil';
 import {
+    ANNOTATOR_EVENT,
     STATES,
     TYPES,
     CLASS_ANNOTATION_LAYER_HIGHLIGHT,
@@ -52,7 +53,8 @@ describe('doc/DocAnnotator', () => {
                 locale: 'en-US'
             },
             localizedStrings: {
-                anonymousUserName: 'anonymous'
+                anonymousUserName: 'anonymous',
+                loadError: 'loaderror'
             }
         });
         annotator.annotatedElement = document.querySelector('.annotated-element');
@@ -355,7 +357,7 @@ describe('doc/DocAnnotator', () => {
         beforeEach(() => {
             stubs.addThread = sandbox.stub(annotator, 'addThreadToMap');
             stubs.setupFunc = AnnotationThread.prototype.setup;
-            stubs.validateThread = sandbox.stub(annotatorUtil, 'validateThreadParams').returns(true);
+            stubs.validateThread = sandbox.stub(annotatorUtil, 'areThreadParamsValid').returns(true);
             sandbox.stub(annotator, 'handleValidationError');
             annotator.notification = {
                 show: sandbox.stub()
@@ -420,6 +422,13 @@ describe('doc/DocAnnotator', () => {
             expect(thread).to.be.empty;
             expect(annotator.handleValidationError).to.be.called;
         });
+
+        it('should emit error and return undefined if thread fails to create', () => {
+            sandbox.stub(annotator, 'emit');
+            const thread = annotator.createAnnotationThread([], {}, 'random');
+            expect(thread).to.be.undefined;
+            expect(annotator.emit).to.be.calledWith(ANNOTATOR_EVENT.error, annotator.localized.loadError);
+        });
     });
 
     describe('createPlainHighlight()', () => {
diff --git a/src/drawing/DrawingModeController.js b/src/drawing/DrawingModeController.js
@@ -1,6 +1,7 @@
 import rbush from 'rbush';
 import AnnotationModeController from '../AnnotationModeController';
 import annotationsShell from './../annotationsShell.html';
+import DocDrawingThread from '../doc/DocDrawingThread';
 import * as annotatorUtil from '../annotatorUtil';
 import {
     CLASS_ANNOTATION_DRAW,
@@ -153,7 +154,8 @@ class DrawingModeController extends AnnotationModeController {
         /* eslint-enable require-jsdoc */
 
         // Setup
-        this.currentThread = this.annotator.createAnnotationThread([], {}, TYPES.draw);
+        const threadParams = this.annotator.getThreadParams([], {}, TYPES.draw);
+        this.currentThread = new DocDrawingThread(threadParams);
         this.bindCustomListenersOnThread(this.currentThread);
 
         // Get handlers
diff --git a/src/drawing/__tests__/DrawingModeController-test.js b/src/drawing/__tests__/DrawingModeController-test.js
@@ -135,22 +135,15 @@ describe('drawing/DrawingModeController', () => {
     describe('setupHandlers()', () => {
         beforeEach(() => {
             drawingModeController.annotator = {
-                createAnnotationThread: sandbox.stub(),
+                getThreadParams: sandbox.stub().returns({
+                    annotatedElement: document
+                }),
                 getLocationFromEvent: sandbox.stub(),
                 annotatedElement: {}
             };
-            stubs.createThread = drawingModeController.annotator.createAnnotationThread;
+            stubs.getThreadParams = drawingModeController.annotator.getThreadParams;
             stubs.getLocation = drawingModeController.annotator.getLocationFromEvent;
             stubs.bindCustomListenersOnThread = sandbox.stub(drawingModeController, 'bindCustomListenersOnThread');
-
-            stubs.createThread.returns({
-                saveAnnotation: () => {},
-                undo: () => {},
-                redo: () => {},
-                handleMove: () => {},
-                handleStart: () => {},
-                handleStop: () => {}
-            });
         });
 
         it('should successfully contain draw mode handlers if undo and redo buttons do not exist', () => {
@@ -159,7 +152,7 @@ describe('drawing/DrawingModeController', () => {
             drawingModeController.redoButtonEl = undefined;
 
             drawingModeController.setupHandlers();
-            expect(stubs.createThread).to.be.called;
+            expect(stubs.getThreadParams).to.be.called;
             expect(stubs.bindCustomListenersOnThread).to.be.called;
             expect(drawingModeController.handlers.length).to.equal(4);
         });
@@ -172,7 +165,7 @@ describe('drawing/DrawingModeController', () => {
 
 
             drawingModeController.setupHandlers();
-            expect(stubs.createThread).to.be.called;
+            expect(stubs.getThreadParams).to.be.called;
             expect(stubs.bindCustomListenersOnThread).to.be.called;
             expect(drawingModeController.handlers.length).to.equal(7);
         });
diff --git a/src/image/ImageAnnotator.js b/src/image/ImageAnnotator.js
@@ -4,6 +4,7 @@ import ImagePointThread from './ImagePointThread';
 import * as annotatorUtil from '../annotatorUtil';
 import * as imageAnnotatorUtil from './imageAnnotatorUtil';
 import {
+    TYPES,
     CLASS_ANNOTATION_POINT_MARKER,
     SELECTOR_ANNOTATION_BUTTON_POINT,
     ANNOTATOR_EVENT
@@ -111,33 +112,22 @@ class ImageAnnotator extends Annotator {
             fixedLocation.page = 1;
         }
 
-        const threadParams = {
-            annotatedElement: this.annotatedElement,
-            annotations,
-            annotationService: this.annotationService,
-            container: this.container,
-            fileVersionId: this.fileVersionId,
-            isMobile: this.isMobile,
-            locale: this.locale,
-            location: fixedLocation,
-            type,
-            permissions: this.permissions,
-            localized: this.localized
-        };
-
-        if (!annotatorUtil.validateThreadParams(threadParams)) {
+        const threadParams = this.getThreadParams(annotations, location, type);
+        if (!annotatorUtil.areThreadParamsValid(threadParams)) {
             this.handleValidationError();
             return thread;
         }
 
-        // Set existing thread ID if created with annotations
-        if (annotations.length > 0) {
-            threadParams.threadID = annotations[0].threadID;
-            threadParams.threadNumber = annotations[0].threadNumber;
+        if (type === TYPES.point) {
+            thread = new ImagePointThread(threadParams);
+        }
+
+        if (!thread) {
+            this.emit(ANNOTATOR_EVENT.error, this.localized.loadError);
+        } else {
+            this.addThreadToMap(thread);
         }
 
-        thread = new ImagePointThread(threadParams);
-        this.addThreadToMap(thread);
         return thread;
     }
 
diff --git a/src/image/__tests__/ImageAnnotator-test.js b/src/image/__tests__/ImageAnnotator-test.js
