Summary:

Fix warning for unsaved work on beforeUnload

Review Request #8635 — Created Jan. 19, 2017 and submitted March 16, 2017, 12:52 p.m.

Information
Owner:	khp
Repository:	Review Board
Branch:	master
Bugs:	4084
Depends On:
Commit:	3df832f...
Reviewers
Groups:	reviewboard, students
People:

Description

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.

Testing Done

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

Issues

Description	From	Last Updated
Did you run the JS unit tests or just the python unit tests?	brennie	Jan. 20, 2017, 9:29 a.m.
If we leave this and the corresponding decr() we should not need to add/remove the event handler for onBeforeUnload. It …	brennie	Jan. 20, 2017, 9:29 a.m.
This whole function should be unnecessary. See my other comments about the editCount stuff.	brennie	Jan. 20, 2017, 9:29 a.m.
Single quotes. This is an ES6 file (.es6.js instead of .js) so we can use const here instead of var.	brennie	Jan. 20, 2017, 9:30 a.m.
All declared variables in a function should be declared in a single var statement at the top of the function.	brennie	Jan. 20, 2017, 9:31 a.m.
Single quotes for strings.	brennie	Jan. 20, 2017, 9:31 a.m.
Why do we have to add/remove the handler if we are checking the edit count?	brennie	Jan. 20, 2017, 9:31 a.m.
Single quotes.	brennie	Jan. 20, 2017, 9:31 a.m.
Can your unit tests set or assert the editable states for these as well? Make sure that now (and in …	chipx86	Jan. 28, 2017, 10:38 p.m.
When you have to escape a quote, you can instead use double quotes for the string.	chipx86	Jan. 21, 2017, 10:37 a.m.
This is an internal function, so t should be _onBeforeUnload.	brennie	Feb. 9, 2017, 11:29 a.m.
Can you move the var declaration outside the if to the top of the function? We can still assign it …	brennie	Feb. 9, 2017, 11:31 a.m.
These 4 tests deal with a combination of states that could probably better be described with one more nested describe(). …	chipx86	Feb. 9, 2017, 11:36 a.m.
To help make unit tests more readable, try breaking these into blocks, where each block makes a change and then …	chipx86	Feb. 9, 2017, 1:08 p.m.
Start this comment with /** so it gets properly interpreted as documentation. A lot of the other comments in older …	david	Feb. 18, 2017, 8:56 p.m.
Dedent the description of the return value 4 spaces.	david	Feb. 18, 2017, 9:01 p.m.
We format multi-line comments like: /* * The components in ReviewablePageView ... * many fields ... * ... */ Please …	david	Feb. 18, 2017, 9 p.m.
type: editabe	david	Feb. 18, 2017, 9 p.m.
Add a period at the end.	david	Feb. 20, 2017, 3:04 p.m.
Add a period at the end.	david	Feb. 20, 2017, 3:04 p.m.

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

Show all issues

Did you run the JS unit tests or just the python unit tests?

khp Jan. 19, 2017, 1:42 p.m.

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

reviewboard/static/rb/js/views/reviewDialogView.es6.js (Diff revision 1)

Show all issues

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.

khp Jan. 19, 2017, 11:19 p.m.

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.

KH

khp Jan. 20, 2017, 9:31 a.m.
```
See new patch! =)
```

reviewboard/static/rb/js/views/reviewDialogView.es6.js (Diff revision 1)

Show all issues

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

reviewboard/static/rb/js/views/reviewDialogView.es6.js (Diff revision 1)

Show all issues

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

reviewboard/static/rb/js/views/reviewDialogView.es6.js (Diff revision 1)

Show all issues

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

KH

khp Jan. 20, 2017, 9:31 a.m.
```
Not changing this file anymore.
```

reviewboard/static/rb/js/views/reviewDialogView.es6.js (Diff revision 1)
Show all issues
```
Single quotes for strings.
```

reviewboard/static/rb/js/views/reviewDialogView.es6.js (Diff revision 1)

Show all issues

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

khp Jan. 19, 2017, 1:44 p.m.

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.

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1)
Show all issues
```
Single quotes.
```

Testing Done:

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

Commit:

-	abae729ccdc136533e43336d6511b0c2d4b27c19
+	596efa49010c5f5adbf6132e385668cce966fd59

Diff:

Revision 2 (+15 -3)

Show changes

	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

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

Change Summary:

Update Description and Testing Done

Description:

		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.

~		The ReviewDialogView now adds its own beforeUnload event handler
	~	New unit tests testing the onBeforeUnload event handler has been
-		that checks the Top and Bottom comment views to see if either are
-		dirty, and if so, issue a warning when a beforeUnload event is
-		fired. This handler is added and removed when the dialog is opened
-		and closed, respectively.
-
-		New unit tests testing the onBeforeUnload event handlers have been
		added.

Testing Done:

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

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

Show all issues

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?

khp Jan. 21, 2017, 10:56 a.m.

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?

chipx86 Jan. 25, 2017, 12:52 p.m.

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.

KH

khp Jan. 28, 2017, 10:38 p.m.
```
Got it! Thanks.
```

reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js (Diff revision 2)
Show all issues
```
When you have to escape a quote, you can instead use double quotes for the string.
```
1. KH
  
  khp Jan. 21, 2017, 10:56 a.m.
  Got it

Change Summary:

Moved the handler to be an instance method to be better testable.

Commit:

-	596efa49010c5f5adbf6132e385668cce966fd59
+	011c1974ac8ee74b7e3af3cbcb9ed64b1561da4b

Diff:

Revision 3 (+44 -22)

Show changes

	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

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

Change Summary:

Updated according to comments; formatting + unit tests

Commit:

-	011c1974ac8ee74b7e3af3cbcb9ed64b1561da4b
+	13c030e70766208727f5bb7fc9758694b33a8e7d

Diff:

Revision 4 (+73 -22)

Show changes

	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

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

Ship it!

A few tiny nitpicks. This looks good to me!

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 4)
Show all issues
```
This is an internal function, so t should be _onBeforeUnload.
```
1. KH
  
  khp Feb. 10, 2017, 10:50 a.m.
  Thanks for the feedback! Done.

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 4)

Show all issues

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?

KH

khp Feb. 10, 2017, 10:50 a.m.
```
Done.
```

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

Show all issues

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).

KH

khp Feb. 10, 2017, 10:50 a.m.
```
Thanks for the feedback! Done.
```

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

Show all issues

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.

khp Feb. 10, 2017, 10:50 a.m.

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

Change Summary:

Updated with responses to comments.

Commit:

-	13c030e70766208727f5bb7fc9758694b33a8e7d
+	386cbbd64f67fd3e3808a48bcd498d82cf4a85b6

Diff:

Revision 5 (+84 -22)

Show changes

	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

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

khp Feb. 18, 2017, 9:01 p.m.

Thanks for the comments, updating with fixed diff!

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 5)

Show all issues

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.

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 5)
Show all issues
```
Dedent the description of the return value 4 spaces.
```

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

Show all issues

We format multi-line comments like:

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

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

reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js (Diff revision 5)
Show all issues
```
type: editabe
```

Change Summary:

Fixed formatting on comments

Commit:

-	386cbbd64f67fd3e3808a48bcd498d82cf4a85b6
+	570acad1862521745fa1f8861abae4f1e40f755f

Diff:

Revision 6 (+86 -22)

Show changes

	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

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

```
Just a couple trivial things:
```
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 6)
Show all issues
```
Add a period at the end.
```
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 6)
Show all issues
```
Add a period at the end.
```

Change Summary:

Added a couple of periods in to comments

Commit:

-	570acad1862521745fa1f8861abae4f1e40f755f
+	3df832f7c117ba71e93437f8e3841c8f9cef21c6

Diff:

Revision 7 (+86 -22)

Show changes

	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

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

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (176916d)