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.

Sign up for GitHub

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
Merged

Fix: Replace {1} with user name on drawing save #179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/doc/DocDrawingDialog.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ class DocDrawingDialog extends AnnotationDialog {
if (!this.element) {
this.setup([annotation]);
}
this.assignDrawingLabel(annotation);
}

/**
Expand Down Expand Up @@ -145,7 +146,7 @@ class DocDrawingDialog extends AnnotationDialog {

const firstAnnotation = util.getFirstAnnotation(annotations);
if (firstAnnotation) {
this.assignDrawingLabel(firstAnnotation);
this.addAnnotation(firstAnnotation);
Copy link

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)

}

this.element.appendChild(this.drawingDialogEl);
Expand Down Expand Up @@ -249,17 +250,16 @@ class DocDrawingDialog extends AnnotationDialog {
* the username does not exist.
*
* @private
* @param {Annotation} savedAnnotation The annotation data to populate the label with.
* @param {Annotation} annotation The annotation data to populate the label with.
* @return {void}
*/
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

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.

https://github.com/box/box-annotations/blob/master/src/AnnotationService.js#L56

return;
}

const drawingLabelEl = this.drawingDialogEl.querySelector(`.${constants.CLASS_ANNOTATION_DRAWING_LABEL}`);
const username = savedAnnotation.user ? savedAnnotation.user.name : constants.USER_ANONYMOUS;
drawingLabelEl.textContent = util.replacePlaceholders(this.localized.whoDrew, [username]);
drawingLabelEl.textContent = util.replacePlaceholders(this.localized.whoDrew, [annotation.user.name]);

util.showElement(drawingLabelEl);
}
Expand Down
38 changes: 30 additions & 8 deletions src/doc/__tests__/DocDrawingDialog-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,23 @@ describe('doc/DocDrawingDialog', () => {
});
});

describe('addAnnotation()', () => {
it('should setup the element if not already done', () => {
dialog.element = undefined;
sandbox.stub(dialog, 'setup');
sandbox.stub(dialog, 'assignDrawingLabel');

dialog.addAnnotation({});
expect(dialog.setup).to.be.calledWith([{}]);
expect(dialog.assignDrawingLabel).to.be.calledWith({});

dialog.element = document.createElement('div');
dialog.addAnnotation({});
expect(dialog.setup).to.be.calledWith([{}]);
expect(dialog.assignDrawingLabel).to.be.calledWith({});
});
});

describe('postDrawing()', () => {
it('should emit annotation create to indicate that the save button was pressed', () => {
const event = {
Expand Down Expand Up @@ -134,7 +151,7 @@ describe('doc/DocDrawingDialog', () => {

sandbox.stub(dialog, 'generateDialogEl').returns(drawingDialogEl);
sandbox.stub(dialog, 'bindDOMListeners');
sandbox.stub(dialog, 'assignDrawingLabel');
sandbox.stub(dialog, 'addAnnotation');
});

it('should setup the dialog element without a commit button when given an annotation', () => {
Expand All @@ -148,7 +165,7 @@ describe('doc/DocDrawingDialog', () => {
dialog.setup([annotation]);
expect(dialog.generateDialogEl).to.be.called;
expect(dialog.bindDOMListeners).to.be.called;
expect(dialog.assignDrawingLabel).to.be.called;
expect(dialog.addAnnotation).to.be.called;
expect(dialog.element.contains(dialog.drawingDialogEl));
expect(dialog.commitButtonEl).to.be.null;
});
Expand All @@ -157,7 +174,7 @@ describe('doc/DocDrawingDialog', () => {
dialog.setup([]);
expect(dialog.generateDialogEl).to.be.called;
expect(dialog.bindDOMListeners).to.be.called;
expect(dialog.assignDrawingLabel).to.not.be.called;
expect(dialog.addAnnotation).to.not.be.called;
expect(dialog.element.contains(dialog.drawingDialogEl));
expect(dialog.commitButtonEl).to.not.be.undefined;
expect(dialog.element.contains(dialog.commitButtonEl));
Expand Down Expand Up @@ -276,19 +293,24 @@ describe('doc/DocDrawingDialog', () => {
sandbox.stub(util, 'replacePlaceholders').returns(notYaoMing);
sandbox.stub(util, 'showElement');

dialog.assignDrawingLabel('non empty');
dialog.assignDrawingLabel({ user: { id: '1' } });
expect(drawingLabelEl.textContent).to.equal(notYaoMing);
expect(dialog.drawingDialogEl.querySelector).to.be.called;
expect(util.replacePlaceholders).to.be.called;
expect(util.showElement).to.be.called;
});

it('should do nothing when given an invalid annotation or does not have a dialog', () => {
expect(dialog.assignDrawingLabel, 'not empty').to.not.throw();
sandbox.stub(util, 'showElement');

dialog.drawingDialogEl = document.createElement('div');
dialog.assignDrawingLabel(null);
dialog.assignDrawingLabel({ user: { id: '0' } });

dialog.drawingDialogEl = undefined;
dialog.assignDrawingLabel({ user: { id: '1' } });

dialog.drawingDialogEl = 'not empty';
expect(dialog.assignDrawingLabel, undefined).to.not.throw();
expect(dialog.assignDrawingLabel, null).to.not.throw();
expect(util.showElement).to.not.be.called;
});
});
});