Allow comments to be deleted from review dialog

Review Request #7744 — Created Oct. 28, 2015 and submitted

Information

Review Board
master

Reviewers

The review dialog now displays a delete icon next to all deletable
comments (diff comments and attachment comments, currently). Previously,
the only way to delete a comment was to delete it from the review UI
where it was created. However, with general comments coming soon, there
was no review UI to manage them and delete them. Hence, it makes sense
for all deletable comments to be deletable from within the review
dialog.

JS Unit tests have been added for deletion of diff comments, attachment
comments, and screenshot comments.

This patch also contains some minor JS cleanups:
- RB.BaseResourceModel.ready now ensures its success and error
options are functions before bdinging them.
- Replaced a binding of self = this and calling a function by binding
said function to the correct context.
- Cleaned up some assignments in RB.BaseResourceModel._finishDestroy
so that it only does one set() call instead of several.

  • Ran JS tests.
  • Deleted attachment comments and diff comments from the review dialog.
    It worked as expected.
  • Manually verified that the deletion confirmation works in both cases.

Description From Last Updated

The order of things is weird here. Can we put the pencil first? We may also want to make the …

daviddavid

I suspect this should not be within the <label> element.

daviddavid

How about: this._commentViews = _.without(this._commentViews, view);

daviddavid

This variable isn't used anymore.

daviddavid
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
    
    
  2. 
      
brennie
david
  1. This should have some confirmation before deleting.

  2. Show all issues

    The order of things is weird here. Can we put the pencil first?

    We may also want to make the trash can slightly less tall, and try to align the baselines better.

    1. I had put the delete before edit becuase the edit icon disappears when clicked (and so the trash can would jump around).

  3. reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 1)
     
     
     
     
     
     
    Show all issues

    I suspect this should not be within the <label> element.

    1. I put the trashcan in the <label> because the pencil icon gets dynamically appended to the <label>. I'll change this around.

  4. reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 1)
     
     
     
     
     
     
     
    Show all issues

    How about:

    this._commentViews = _.without(this._commentViews, view);
    
    1. I did not know about _.without!

  5. 
      
brennie
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

    This variable isn't used anymore.

  3. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to master (870f5c0)