Skip to content
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

Merged
merged 1 commit into from
Apr 23, 2018

Conversation

pramodsum
Copy link
Contributor

No description provided.

@boxcla
Copy link

boxcla commented Apr 23, 2018

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') {
Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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') {
Copy link
Contributor

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);
Copy link
Contributor

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.

Copy link
Contributor Author

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)

@pramodsum pramodsum merged commit 408b495 into box:master Apr 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants