Fix bug and now warning will pop up if try to move to a new comment while editing some unsaved comment.

Review Request #8649 — Created Jan. 21, 2017 and discarded

Information

Review Board
master

Reviewers

Fix the bug of unsaved comment being discarded when creating/opening another comment.
For creating a new comment, the code creates a CommentBox(the little green box next to line number) first and then creates a new CommentDialog for editing. For opening another comment, the code only creates a new CommentDialog.
When creating the new CommentDialog, the "dirtiness" of existing CommentDialog is not checked, and thus the unsaved comments would be discarded.
After the fix, a check was done when creating the new CommentDialog. If the old CommentDialog is dirty, pop a warning message. If user chooses not to discard the unsaved comments, the new CommentDialog will not be created and the newly created CommentBox will be deleted in case of creating new comment. If user chooses to discard the unsaved comments, the code works the same way as before.

Test Preperation: create a comment (comment1), write something, and save it.

Test1: Create a comment (comment2), write something, and click on another line number trying to create another comment (comment3).
Test1 Result: Warning pops up.
Test1.1: Click "OK" on the warning popped up in Test1.
Test1.1 Result: comment2 is deleted, and dialog for comment3 pops up.
Test1.2: Click "cancel" on the warning popped up in Test1.
Test1.2 Result: comment3 is deleted, and dialog for comment2 stays in the screen.

Test2: Create a comment (comment4), write something, and click on comment1 trying to open it.
Test2 Result: Warning pops up.
Test2.1: Click "OK" on the warning popped up in Test2.
Test2.1 Result: comment4 is deleted, and dialog for comment1 pops up.
Test2.2: Click "cancel" on the warning popped up in Test2.
Test2.2 Result: the dialog for comment4 stays in the screen, nothing happens to comment1.

Description From Last Updated

The summary and description don't need to include the bug number (since it's listed in the bugs field). It would …

daviddavid

This is a .es6.js file, so we should use const instead of var.

daviddavid

This comment should explain in what situation RB.CommentDialogView.create would refuse to create a new dialog. There should also be a …

daviddavid

This can be simplified quite a bit. We should probably also be returning an explicit null instead of just letting …

daviddavid
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/abstractReviewableView.es6.js
        reviewboard/static/rb/js/views/commentDialogView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/abstractReviewableView.es6.js
        reviewboard/static/rb/js/views/commentDialogView.js
    
    
  2. 
      
JO
JO
david
  1. 
      
  2. Show all issues

    The summary and description don't need to include the bug number (since it's listed in the bugs field).

    It would be nice to separate the description a bit into a few paragraphs--one explaining what the bug was (from a user standpoint), one or more explaining a bit more about what the problem was in the code, and then one about what changes you made. Please make sure it wraps to about 70 columns, too.

    Having some blank lines in the testing done section would also make it easier to read.

  3. Show all issues

    This is a .es6.js file, so we should use const instead of var.

  4. Show all issues

    This comment should explain in what situation RB.CommentDialogView.create would refuse to create a new dialog. There should also be a space between "//" and the comment text, and the sentence should start with a capital letter.

    If the comment gets long enough to wrap onto multiple lines, use a block comment:

    /*
     * This is a very long
     * comment that has to wrap.
     */
    
  5. reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This can be simplified quite a bit. We should probably also be returning an explicit null instead of just letting it be undefined.

    if (instance &&
        instance.model.get('dirty') &&
        !confirm(gettext('You have unsaved changes. Are you sure you want to exit?'))) {
        return null;
    }
    
  6. 
      
JO
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/abstractReviewableView.es6.js
        reviewboard/static/rb/js/views/commentDialogView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/abstractReviewableView.es6.js
        reviewboard/static/rb/js/views/commentDialogView.js
    
    
  2. 
      
JO
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/abstractReviewableView.es6.js
        reviewboard/static/rb/js/views/commentDialogView.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/abstractReviewableView.es6.js
        reviewboard/static/rb/js/views/commentDialogView.js
    
    
  2. 
      
chipx86
  1. Just want to call attention to the issues David opened. Have these been addressed? We'd like to get this in.

  2. 
      
david
Review request changed
Status:
Discarded