Summary

Fix a bug in the RB.UploadAttachmentView refactoring change

Review Request #10937 — Created March 4, 2020 and submitted March 22, 2020, 4:20 p.m.

Information

Owner

hxqlin

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

b5ece1b...

Reviewers

Groups

reviewboard

People

Description

The change e94bac071aa42fa45ad30d92f975d6d7c114e16a was landed prematurely and introduced

a some regressions. Some of them were fixed in 3a74bdfaee929403eb7ada0c38d286e6c27d0d20 but

a couple remained.
In RB.UploadAttachmentView, the options in initialize needs to be defaulted to an

empty object to prevent reference errors. In addition, the function was referencing 

this.presetCaption and this.attachmentHistory, which were undefined. This caused the 

dialog to have a blank caption when file attachments were updated.

Testing Done

dialogView unit tests still pass

Manual testing:

Adding a file attachment:
- opening the dialog, uploading a file, and submitting it (without editing the caption)
- opening the dialog, uploading a file, editing the caption, and submitting it
- opening the dialog, editing the caption, uploading a file, and submitting it
- opening the dialog, uploading a file, then cancelling the dialog
- opening the dialog, editing the caption, then cancelling the dialog
- opening the dialog, then cancelling the dialog without uploading anything
- verified the 'Upload' button is disabled unless a file is actually uploaded

Updating a file attachment:
- editing the caption for a file attachment inline and then opening the update dialog
to check that the caption is consistent
- opening the update dialog and cancelling the dialog without uploading anything new
- updating a file attachment without changing the caption
- updating a file attachment and changing the caption
- verified the 'Upload' button is disabled unless a new file is actually uploaded

Issues

Description	From	Last Updated
Looking at this further, we actually need to break this into two steps: Build the version of options we want …	chipx86	March 4, 2020, 8:31 p.m.
Since we're already extending this with options, which now contains attachmentHistoryID and presetCaption, we won't need to put them in …	chipx86	March 12, 2020, 10:08 p.m.

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/views/uploadAttachmentView.es6.js (Diff revision 1)

The issue has been resolved. Show all issues

Looking at this further, we actually need to break this into two steps:

Build the version of options we want to pass in to both initialize and this.template up-front
Build body.
Call RB.DialogView.prototype.initialize().

The reason is that this is trying to do two things at once, which it cannot do: Set defaults for options, and pass the options (provided or defaults) to the template. The template depends on the defaults set right above, and it's not getting a chance to use them.

So if you split this into three statements, that should solve it.

(Wish we had unit tests for this view, because we'd have caught this earlier.)

chipx86 March 4, 2020, 3:44 p.m.
```
(Er, "break this into three steps.")
```

Change Summary:

Fix issue from review

Commit:

1939d08ae3959a809eaa68500477d20453393e29

943e872b537200b126555f8ef0cf077f0922e44a

Diff:

Revision 2 (+15 -8)

Show changes

reviewboard/static/rb/js/views/uploadAttachmentView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/static/rb/js/views/uploadAttachmentView.es6.js (Diff revision 2)

The issue has been resolved. Show all issues

Since we're already extending this with options, which now contains attachmentHistoryID and presetCaption, we won't need to put them in the initial payload. We can just include body exclusively.

Commit:

943e872b537200b126555f8ef0cf077f0922e44a

b5ece1bbd38d7b7d20bd8f106d931ae81494bab4

Diff:

Revision 3 (+13 -8)

Show changes

reviewboard/static/rb/js/views/uploadAttachmentView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Status:: Completed
Change Summary:: Pushed to release-3.0.x (29ae747)