Skip to content

Commit

Permalink
Fix: Reset current annotation thread immediately on save (#313)
Browse files Browse the repository at this point in the history
- Prevents new annotations (drawings) that are created before the create API call has successfully returned from being appended onto previous annotations (drawings)
  • Loading branch information
pramodsum authored Dec 10, 2018
1 parent bcd6ae5 commit 19c89cc
Show file tree
Hide file tree
Showing 6 changed files with 38 additions and 10 deletions.
12 changes: 4 additions & 8 deletions src/AnnotationThread.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,16 +56,8 @@ class AnnotationThread extends EventEmitter {
this.annotatedElement = data.annotatedElement;
this.api = data.api;
this.container = data.container;
this.fileVersionId = data.fileVersionId;
this.location = data.location;
this.threadID = data.threadID || AnnotationAPI.generateID();
this.threadNumber = data.threadNumber || '';
this.type = data.type;
this.locale = data.locale;
this.hasTouch = data.hasTouch || false;
this.permissions = data.permissions;
this.state = STATES.inactive;
this.canComment = true;
this.headerHeight = data.headerHeight;

this.id = data.id;
Expand All @@ -77,6 +69,8 @@ class AnnotationThread extends EventEmitter {
this.modifiedAt = data.modifiedAt;
this.fileVersionId = data.fileVersionId;
this.threadID = data.threadID || AnnotationAPI.generateID();
this.state = STATES.inactive;
this.permissions = data.permissions;
this.canDelete = data.canDelete;
this.canAnnotate = data.canAnnotate;
this.canComment = true;
Expand Down Expand Up @@ -236,6 +230,8 @@ class AnnotationThread extends EventEmitter {
* @return {Promise} - Annotation create promise
*/
save(type: AnnotationType, message: string): Promise<any> {
this.emit(THREAD_EVENT.create);

const annotationData = this.createAnnotationData(type, message);

// Save annotation on client
Expand Down
1 change: 1 addition & 0 deletions src/__tests__/AnnotationThread-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ describe('AnnotationThread', () => {
done();
});
expect(thread.api.create).toBeCalled();
expect(thread.emit).toBeCalledWith(THREAD_EVENT.create);
});

it('should delete the temporary annotation and broadcast an error if there was an error saving', (done) => {
Expand Down
1 change: 1 addition & 0 deletions src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ export const ANNOTATOR_EVENT = {
};

export const THREAD_EVENT = {
create: 'annotationcreate',
pending: 'annotationpending',
render: 'annotationrender',
save: 'annotationsaved',
Expand Down
16 changes: 16 additions & 0 deletions src/controllers/AnnotationModeController.js
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,19 @@ class AnnotationModeController extends EventEmitter {
*/
setupHandlers(): void {}

/**
* Resets current thread when annotation is no longer pending
*
* @param {AnnotationThread} thread - Annotation thread being saved
* @return {void}
*/
resetCurrentThread(thread: AnnotationThread): void {
this.selectedThread = thread;
this.currentThread = undefined;
this.hadPendingThreads = false;
this.pendingThreadID = null;
}

/**
* Handles annotation thread events and emits them to the viewer
*
Expand All @@ -505,6 +518,9 @@ class AnnotationModeController extends EventEmitter {
const { event, data: threadData, eventData } = data;

switch (event) {
case THREAD_EVENT.create:
this.resetCurrentThread(thread);
break;
case THREAD_EVENT.save:
case THREAD_EVENT.cancel:
this.hadPendingThreads = false;
Expand Down
3 changes: 1 addition & 2 deletions src/controllers/DrawingModeController.js
Original file line number Diff line number Diff line change
Expand Up @@ -267,8 +267,7 @@ class DrawingModeController extends AnnotationModeController {
thread.removeListener('threadevent', this.handleThreadEvents);
thread.unbindDrawingListeners();

this.currentThread = undefined;
this.selectedThread = thread;
this.resetCurrentThread(thread);
this.unbindListeners();

// Clear existing canvases
Expand Down
15 changes: 15 additions & 0 deletions src/controllers/__tests__/AnnotationModeController-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -438,19 +438,34 @@ describe('controllers/AnnotationModeController', () => {
});
});

describe('resetCurrentThread()', () => {
it('should reset the current thread when an annotaiton is no longer pending', () => {
controller.currentThread = thread;
controller.resetCurrentThread(thread);
expect(controller.currentThread).toBeUndefined();
expect(controller.selectedThread).toEqual(thread);
});
});

describe('handleThreadEvents()', () => {
beforeEach(() => {
controller.emit = jest.fn();
controller.renderPage = jest.fn();
controller.render = jest.fn();
controller.registerThread = jest.fn();
controller.unregisterThread = jest.fn();
controller.resetCurrentThread = jest.fn();
controller.localized = {
deleteError: 'delete error',
createError: 'create error'
};
});

it('should reset the current thread when a new annotation is created', () => {
controller.handleThreadEvents(thread, { event: THREAD_EVENT.create });
expect(controller.resetCurrentThread).toBeCalledWith(thread);
});

it('should mark hadPendingThreads as false and emit event on thread save or cancel', () => {
controller.handleThreadEvents(thread, { event: THREAD_EVENT.save, data: {} });
expect(controller.emit).toBeCalledWith(THREAD_EVENT.save, expect.any(Object));
Expand Down

0 comments on commit 19c89cc

Please sign in to comment.