Fix warning for unsaved work on beforeUnload

Review Request #8635 - Created Jan. 19, 2017 and submitted

Kanghee Park
Review Board
master
4084
3df832f...
reviewboard, students

Previously, when editing a review or comments as a user without
edit rights, warning users about losing unsaved work did not get
displayed.

Now the ReviewRequestEditorView's beforeUnload event handler does
not check a user's edit rights and only checks to see if any
fields are being edited at the moment.

New unit tests testing the onBeforeUnload event handler has been
added.

Ran JS unit tests.
Check that ReviewDialogView for both edit and non-edit privileged users are
warned before unloading.

  • 0
  • 0
  • 17
  • 3
  • 20
Description From Last Updated
Review Bot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.es6.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewDialogView.es6.js
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
Barret Rennie
  1. 
      
  2. Did you run the JS unit tests or just the python unit tests?

    1. Ran the js-tests. There were 2 tests that failed though that were already failing on master.

  3. If we leave this and the corresponding decr() we should not need to add/remove the event handler for onBeforeUnload. It should only need to be registered once.

    1. The bug report's steps for replication seemed to imply that inputing text into the header/footer fields should trigger the beforeUnload warning, not just the opening of the dialog view.

    2. See new patch! =)

  4. This whole function should be unnecessary. See my other comments about the editCount stuff.

  5. Single quotes.

    This is an ES6 file (.es6.js instead of .js) so we can use const here instead of var.

  6. All declared variables in a function should be declared in a single var statement at the top of the function.

    1. Not changing this file anymore.

  7. Single quotes for strings.

  8. Why do we have to add/remove the handler if we are checking the edit count?

    1. This was because the binding to the handler gets messed up everytime the dialog was closed, since the object gets destroyed. I think this can be done without replacing the handler everytime since it's a singleton. I'll put in a new review with the refactor.

  9. 
      
Kanghee Park
Kanghee Park
Review Bot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
Kanghee Park
Christian Hammond
  1. 
      
  2. reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     

    Can your unit tests set or assert the editable states for these as well? Make sure that now (and in the future) the code does the right thing when editable/not editable?

    1. Hey Christian. The code doesn't check editable state anymore because the reviewReplyEditorView can change reviewRequestEditor's editCount even when the reviewRequestEditor itself has editable == false. So when the user doesn't have edit rights, they can still open reviewReplyEditorView changing editCount, and editable doesn't cover all cases where beforeUnload should trigger, whereas editCount > 0 seems to cover everything. It seems like editable means that the review itself is not editable by the user, but there may be other things on the page such as replies that are editable. If this behaviour is acceptable, I don't think editable should be part of the tests. What do you think?

    2. Hey Kanghee,

      The reason for the unit test is to ensure regressions don't happen down the road. The old code seemed right at the time, but clearly had problems, and a refactor down the road could inadvertently re-introduce logic tied to the editable state. Having a unit test in place is a way of saying "This state should not impact this behavior," helping to catch such things.

    3. Got it! Thanks.

  3. When you have to escape a quote, you can instead use double quotes for the string.

  4. 
      
Kanghee Park
Review Bot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
Kanghee Park
Review Bot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
Barret Rennie
  1. A few tiny nitpicks. This looks good to me!

  2. This is an internal function, so t should be _onBeforeUnload.

    1. Thanks for the feedback! Done.

  3. Can you move the var declaration outside the if to the top of the function? We can still assign it within the if.

    Also, do you mind changing this to single quotes while you're here?

  4. 
      
Christian Hammond
  1. 
      
  2. These 4 tests deal with a combination of states that could probably better be described with one more nested describe(). I'd have one for the editable state. So:

    describe('editable=true', { ... })
    describe('editable=false', { ... })
    

    Each of those can then set the initial state in a beforeEach(), ensuring each test within has the correct starting state and that the initial state's assertions have passed.

    This ensures that all tests are set up correctly, helps show the real "meat" of the tests and how they differ from each other, and ensures future tests in here are consistent (especially if changes are made to any of this logic down the road).

    1. Thanks for the feedback! Done.

  3. reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js (Diff revision 4)
     
     
     
     
     
     
     
     
     

    To help make unit tests more readable, try breaking these into blocks, where each block makes a change and then tests it. Comments help as well.

    So for instance, I'd put a blank line between the expect(editor.get('editable', true)).toBe(true); and the view.model.incr('editCount'), since we started testing a new thing there.

    Also, I don't know that the unit test should be providing a default for these get() calls. It should be testing state in these, not influencing state. This would cover up a bug where editable gets set as undefined.

    Same comments apply below.

    1. Definitely agreed on the default values, shouldn't be there! Thanks. Done.

  4. 
      
Kanghee Park
Review Bot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
David Trowbridge
  1. 
      
    1. Thanks for the comments, updating with fixed diff!

  2. Start this comment with /** so it gets properly interpreted as documentation. A lot of the other comments in older code don't (yet) do this, but we're moving towards it.

  3. Dedent the description of the return value 4 spaces.

  4. We format multi-line comments like:

    /*
     * The components in ReviewablePageView ...
     * many fields ...
     * ...
     */
    

    Please also capitalize the first letter of "ReviewablePageView" and "ReviewRequestEditorView" herein.

  5. 
      
Kanghee Park
Review Bot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
David Trowbridge
  1. Just a couple trivial things:

  2. Add a period at the end.

  3. Add a period at the end.

  4. 
      
Kanghee Park
Review Bot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
David Trowbridge
  1. Ship It!
  2. 
      
Kanghee Park
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (176916d)
Loading...