Fix: Validating annotation thread params #17

pramodsum · 2017-03-24T22:12:28Z

No description provided.

tonyjin · 2017-03-27T20:59:21Z

src/lib/Notification.js

@@ -62,11 +62,14 @@ class Notification {

    /**
     * Hides the notification message.
+     * Does nothing if the notification is already hidden.


Nit-pick, but I wouldn't make this a second line - Continue the first line and wrap it at a reasonable breaking point.

tonyjin · 2017-03-27T21:06:43Z

src/lib/annotations/Annotator.js

@@ -432,6 +451,13 @@ class Annotator extends EventEmitter {
     * @return {void}
     */
    addThreadToMap(thread) {
+        if (!annotatorUtil.checkThreadValid(thread)) {


I think you're adding double validation here - move the check to the individual createAnnotationThread() implementations. It makes sense there since each individual annotator knows best about what sort of thread it considers valid.

Yea I just realized this was extra validation and removed the check here

tonyjin · 2017-03-27T21:07:21Z

src/lib/annotations/Annotator.js

+        if (!annotatorUtil.checkThreadValid(thread)) {
+            this.emit('annotationerror', {
+                reason: 'validation'
+            });


I'm still iffy on this implementation since if there are multiple validation errors we'd essentially be showing multiple error messages "on top of each other" which doesn't feel great.

Maybe have a private function in the annotator called handleLoadError() that then keeps track of whether we've already shown a notification or not for validation, and if not, show a notification error. We'd then call that function directly whenever we think there's an error (so call checkThreadValid() and handleLoadError() in the false case within the same function). I think it's okay to keep broadcasting the annotationerror event per validation failure, but let's not bind to an event within the same class and repeatedly show the error notification

Would it make sense to move a check like this into Notification.js to be something like is there a notification already showing which is for the same type of error?

@tonyjin added the check to Notification.js. Thoughts?

No I don't think adding that to Notification.js is a good idea. It should ideally be as dumb and maintain as little state as possible

Added a handleValidationError() method which maintains whether a notification was already displayed once and displays the notification accordingly

tonyjin · 2017-03-28T22:30:18Z

I still don't think this approach is the right way, here are the main points:

Emitting the error is fine in case anyone else wants to bind to it in the future, but since we are already within the Annotator class, just call this.handleValidationError() directly and move emitting the error to within that function. The reason we add listeners to the service for 'annotationerror' is because we should not call a Annotator function directly from within the service, but we can pass a message to the Annotator. In this case, we are already within the Annotator.
Don't double validate - you have validation when reading and validating when creating - just keep the validation you have using the params map inside createAnnotationThread() and remove checkThreadValid()
With the above changes, return null instead of thread when the validateThreadParams() check fails within createAnnotationThread

We can discuss more in person tomorrow if needed.

pramodsum · 2017-03-29T22:27:52Z

@tjin fixed it according to your comments above. Sorry I think I misunderstood what you meant earlier.

…information

…ents

- Also includes fix for thread locale to be correctly set for image point annotations - Prevents multiple validation notifications from being triggered while another validation notification is already being displayed

… in Annotator - Returns null when validateThreadParams() fails

JustinHoldstock · 2017-04-04T22:52:21Z

src/lib/Notification.js

     *
     * @return {void}
     */
    hide() {
-        this.notificationEl.classList.add(CLASS_HIDDEN);
+        if (this.notificationEl) {


Won't this always fire, since this.notificationElexists here? You could check to see if the class list contains the hidden class, and do nothing if it does

classList already has built in functionality to do nothing if there is already a CLASS_HIDDEN class. I think this is to guard against a situation where notification.hide() is called in a timeout or something after the notification is destroyed so notificationEl could not exist

^ exactly why i added it!

tonyjin reviewed Mar 27, 2017

View reviewed changes

pramodsum added 7 commits April 4, 2017 13:22

Validating if document and image annotations contain proper location …

04e26b9

…information

Fix unit tests

ffc4500

Updating Annotator unit tests to match new standard

af2c9cf

Ensure blank notifications are never created for 'annotationerror' ev…

d92934b

…ents

Fixes from comments

c1747e7

- Also includes fix for thread locale to be correctly set for image point annotations - Prevents multiple validation notifications from being triggered while another validation notification is already being displayed

Removing redundant checks

a47fde6

Removing checkValidThread() and emission of 'annotationerror' message…

d7f3181

… in Annotator - Returns null when validateThreadParams() fails

JustinHoldstock approved these changes Apr 4, 2017

View reviewed changes

Merge branch 'master' into annotations

78bd4ea

pramodsum merged commit 6a357f7 into box:master Apr 5, 2017

tonyjin added the legacy-ignore label Apr 7, 2017

This was referenced Nov 8, 2017

Update: box-annotations to v0.2.0 #477

Merged

Update: box-annotations to v0.2.1 #479

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Validating annotation thread params #17

Fix: Validating annotation thread params #17

pramodsum commented Mar 24, 2017

tonyjin Mar 27, 2017

tonyjin Mar 27, 2017

pramodsum Mar 27, 2017

tonyjin Mar 27, 2017

tonyjin Mar 27, 2017

pramodsum Mar 27, 2017

pramodsum Mar 28, 2017

tonyjin Mar 28, 2017

pramodsum Mar 28, 2017

tonyjin commented Mar 28, 2017

pramodsum commented Mar 29, 2017

JustinHoldstock Apr 4, 2017

tonyjin Apr 4, 2017

pramodsum Apr 4, 2017

Fix: Validating annotation thread params #17

Fix: Validating annotation thread params #17

Conversation

pramodsum commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyjin commented Mar 28, 2017

pramodsum commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment