Fix a bug in the RB.UploadAttachmentView refactoring change

Review Request #10937 — Created March 4, 2020 and submitted

hxqlin
Review Board
release-3.0.x
b5ece1b...
reviewboard

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.

  • 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

Description From Last Updated

Looking at this further, we actually need to break this into two steps: Build the version of options we want ...

chipx86chipx86

Since we're already extending this with options, which now contains attachmentHistoryID and presetCaption, we won't need to put them in ...

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

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

    1. Build the version of options we want to pass in to both initialize and this.template up-front
    2. Build body.
    3. 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.)

    1. (Er, "break this into three steps.")

  3. 
      
hxqlin
chipx86
  1. 
      
  2. 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.

  3. 
      
hxqlin
hxqlin
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (29ae747)
Loading...