-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Replace {1} with user name on drawing save #179
Conversation
Verified that @pramodsum has signed the CLA. Thanks for the pull request! |
assignDrawingLabel(savedAnnotation) { | ||
if (!savedAnnotation || !this.drawingDialogEl) { | ||
assignDrawingLabel(annotation) { | ||
if (!annotation || !this.drawingDialogEl || annotation.user.id === '0') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to ensure that the user id is not '0' so that we know the API has returned a valid user name and successfully saved the new annotation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure that the annotation will have a user? user.id could throw an error otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The temporary annotation contains a blank user w/ an id of 0. Once the API successfully saves an annotation, it returns a user with a valid id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assignDrawingLabel(savedAnnotation) { | ||
if (!savedAnnotation || !this.drawingDialogEl) { | ||
assignDrawingLabel(annotation) { | ||
if (!annotation || !this.drawingDialogEl || annotation.user.id === '0') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure that the annotation will have a user? user.id could throw an error otherwise.
@@ -145,7 +146,7 @@ class DocDrawingDialog extends AnnotationDialog { | |||
|
|||
const firstAnnotation = util.getFirstAnnotation(annotations); | |||
if (firstAnnotation) { | |||
this.assignDrawingLabel(firstAnnotation); | |||
this.addAnnotation(firstAnnotation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does addAnnotation get called when creating an annotation OR rendering an existing one?
My understanding of this code is:
We're setting up a dialog, so if there's an annotation already in that dialog, add it to the dialog. Draw dialogs can only have one annotation (no comments), so we're not really adding an annotation here, but loading the previously saved annotation into this dialog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both. addAnnotation just adds the element to the dialog so it occurs either when the dialog is initially being set up or when a new annotation is saved to the thread (which only occurs once for drawing annotations)
No description provided.