Fix: thread save incorrectly rejects when no dialogue exists #247
Conversation
|
The line |
|
ImageAnnotator uses -1 to indicate that it is a single paged image. I have a separate PR (#249) to change page -1 to page 1 for images. Having -1 pages was just how it was implemented beforehand to indicate that we didn't care for pages in images, but that definitely changed once we added the multi-page image annotations. |
|
After @pramodsum and @minhhnguyen's discussion, is this still a valid PR? |
|
@JustinHoldstock |
|
LGTM but someone with more annotations expertise should verify |
|
tests incoming |
|
|
||
| if (this.dialog) { | ||
| // Add thread number to associated dialog and thread | ||
| if (this.dialog.element && this.dialog.element.dataset) { | ||
| this.dialog.element.dataset.threadNumber = this.thread; |
There was a problem hiding this comment.
This will be setting the dialog thread number to null
this.dialog.element.dataset.threadNumber = this.threadNumber;
There was a problem hiding this comment.
@pramodsum tests in and variable name updated
| this.saveAnnotationToThread(savedAnnotation); | ||
| } else { | ||
| // Otherwise, replace temporary annotation with annotation saved to server | ||
| this.annotations[tempIdx] = savedAnnotation; |
There was a problem hiding this comment.
Is it possible that this.annotations[-1] would have tempAnnotation in it? Would we need to replace it with the correct one no matter what?
| * @param {Annotation} localAnnotation - The temporary annotation holding place of the server validated annotation | ||
| * @return {void} | ||
| */ | ||
| updateLocalAnnotationFromServer(serverAnnotation, localAnnotation) { |
There was a problem hiding this comment.
Thanks for moving this out and writing tests! 👍
| this.dialog.removeAnnotation(tempAnnotationID); | ||
| } | ||
| }) | ||
| .then((savedAnnotation) => this.updateLocalAnnotationFromServer(savedAnnotation, tempAnnotation)) |
There was a problem hiding this comment.
I'd like to keep the naming consistent. Can we make this method either updateTempAnnotation(...) or change tempAnnotation, tempIdx, and other temp variables to localAnnotation, etc?
There was a problem hiding this comment.
changing all localAnnotation variable names to tempAnnotation
| * | ||
| * @private | ||
| * @param {Annotation} serverAnnotation - The annotation returned by the backend. Use this as the source of truth | ||
| * @param {Annotation} localAnnotation - The temporary annotation holding place of the server validated annotation |
There was a problem hiding this comment.
The temporary annotation stored locally holding place of the server validated annotation
| * @return {void} | ||
| */ | ||
| updateLocalAnnotationFromServer(serverAnnotation, localAnnotation) { | ||
| const tempIdx = this.annotations.indexOf(localAnnotation); |
| this.dialog.element.dataset.threadNumber = this.threadNumber; | ||
| } | ||
|
|
||
| this.dialog.addAnnotation(serverAnnotation); |
There was a problem hiding this comment.
// Removes temporary/local annotation and replaces it with the annotation saved to server
| stubs.saveAnnotationToThread = sandbox.stub(thread, 'saveAnnotationToThread'); | ||
| thread.annotations.push(serverAnnotation) | ||
| thread.updateLocalAnnotationFromServer(replacementAnnotation, serverAnnotation); | ||
| thread.on('annotationcreateerror', () => { |
There was a problem hiding this comment.
Not sure the check on 'annotationcreateerror' needs to be in the unit tests for updateLocalAnnotationFromServer(). Might make sense to move the portion of code called in the catch block into it's own method and move this check there.
There was a problem hiding this comment.
Good point, I've removed this portion from the tests.
|
Just make sure you test this thoroughly with all the different annotation types just to be sure 👍 |
|
I'll go through a quick sanity check prior to merging this |
Promise would reject upon successful creation of annotation on the backend due to attempted dereferences on null/undefined objects.
Might have future issues with
this.thread = this.thread || savedAnnotation.thread;if create(annotationData) returns a null annotation object.
This is not likely, I just haven't read through all of AnnotationService.
For
this.annotations[tempIdx] = savedAnnotation;,let me know if this.annotations[-1] is actually intentional. I saw that ImageAnnotator uses threads.page[-1] as a valid index.