fixing Issue 3300:Add a confirmation when discarding a pending review

Review Request #6341 — Created Sept. 19, 2014 and submitted

justy777
Review Board
master
3300
08a9a70...
reviewboard, students

When a user has a draft review, and opens up the "Edit Review" box to finalize their comments and publish, the "Discard Review" command did not confirm at all. Now it gives a prompt to discard or cancel.

Visually Tested (In Chome, and Firefox).

Wrote a review; both pressing and not pressing 'ok' under the comment.

Case 1: Clicking Discard Review, then clicking cancel. It takes away the prompt.
Case 2: Clicking Discard Review, then clicking discard. Discards the review and goes back to the review request.

Description From Last Updated

here it is

AS asalahli

.hideAndReload() could be indented one more level

dkusdkus

The indentation in this line got unintentionally changed. Please change it back.

daviddavid

This inline function has gotten quite long, with a lot of really deep indentation. Can you pull this into a ...

daviddavid

Is this var being used?

dkusdkus

Since _onDiscardClicked returns false, you can just do: .click(_.bind(this._onDiscardClicked, this));

daviddavid

This should be indented only 4 spaces.

daviddavid

You've got a trailing space here.

daviddavid

No space before ().

chipx86chipx86

Instead of embedding the localized string directly in the HTML, you should do: $('<input type="button"/>') .val(gettext('Cancel')) ... etc. The reason ...

chipx86chipx86
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
    
    
  2. 
      
AS
  1. 
      
  2. Did .hideAndReload() disappear in the new version? Isn't it needed?

    1. I believe your right, good eye.

    2. please open an issue.

  3. 
      
AS
justy777
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/static/rb/js/views/draftReviewBannerView.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/static/rb/js/views/draftReviewBannerView.js
    
    
  2. 
      
dkus
  1. 
      
  2. .hideAndReload() could be indented one more level

  3. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/static/rb/js/views/draftReviewBannerView.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/static/rb/js/views/draftReviewBannerView.js
    
    
  2. 
      
justy777
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/static/rb/js/views/draftReviewBannerView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/static/rb/js/views/draftReviewBannerView.js
    
    
  2. 
      
justy777
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
    
    
  2. 
      
justy777
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
    
    
  2. 
      
NI
  1. Ship It!

  2. 
      
david
  1. 
      
  2. The indentation in this line got unintentionally changed. Please change it back.

  3. reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    This inline function has gotten quite long, with a lot of really deep indentation. Can you pull this into a named function in the object, so that the click handler looks more like .click(_.bind(this._onDiscardClicked, this)) ?

  4. 
      
dkus
  1. 
      
  2. Is this var being used?

    1. it is now being used for model.destroy() on line 777.

  3. 
      
AD
  1. 
      
  2. What happens when you click the Cancel button instead of discarding it?

  3. 
      
justy777
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
    
    
  2. 
      
david
  1. Pretty close, just a couple more things:

  2. reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     

    Since _onDiscardClicked returns false, you can just do:

    .click(_.bind(this._onDiscardClicked, this));
    
  3. This should be indented only 4 spaces.

  4. 
      
justy777
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
    
    
  2. 
      
david
  1. Can you list all of the different test cases you've executed?

  2. You've got a trailing space here.

  3. 
      
justy777
justy777
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.js
    
    
  2. 
      
david
  1. Ship It!

  2. 
      
justy777
chipx86
  1. 
      
  2. No space before ().

  3. Instead of embedding the localized string directly in the HTML, you should do:

    $('<input type="button"/>')
        .val(gettext('Cancel'))
        ...
    

    etc.

    The reason being that localized text may potentially not be valid HTML, and would need to be escaped. val() will take care of this.

    1. fixed in this review request: https://reviews.reviewboard.org/r/6346/

  4. 
      
justy777
Review request changed

Change Summary:

Pushed to release-2.0.x (8faeb03)

Testing Done:

~  

Tested visually, by publishing a review; both pressing and not pressing 'ok' under the comment.

  ~

Visually Tested (In Chome, and Firefox).

  +
  +

Wrote a review; both pressing and not pressing 'ok' under the comment.

   
   

Case 1: Clicking Discard Review, then clicking cancel. It takes away the prompt.

    Case 2: Clicking Discard Review, then clicking discard. Discards the review and goes back to the review request.

Loading...