Summary

Fix new comment discarding existing unsaved draft comments.

Review Request #9201 — Created Sept. 22, 2017 and submitted Sept. 24, 2017, 2:29 p.m.

Information

Owner

giuliacm

Repository

Review Board

Branch

release-3.0.x

Bugs

4434

Depends On

Commit

9d1163e...

Reviewers

Groups

reviewboard, students

People

Description

When a comment box was open, clicking a new line to add another comment would
discard the current comment without warning or prompting the user to save
changes. The old comment box was silently closed, and a new one was opened.

This change warns the user before the old comment box is closed. Pressing
cancel keeps the old comment box open. Pressing OK closes the old comment
box without saving it, and creates a new one.

Testing Done

Unit tests pass.

Manually tested clicking a new line to add a comment, when a current comment
box was open. Manually tested clicking a new line to add a comment, when a
previously saved comment box was open. Pressing OK continues to create a
new comment, discarding the unsaved comment. Pressing cancel returns to
the current open comment box.

Issues

Description	From	Last Updated
Can you add a unit test for this change? Mentors can help with what's required here.	chipx86	Sept. 23, 2017, 12:42 p.m.
Col: 40 Expected '!==' and instead saw '!='.	reviewbot	Sept. 22, 2017, 10:12 a.m.
A few easy comments on this line: There should be no space between the ! and the confirm Can you …	david	Sept. 22, 2017, 10:27 a.m.
Please remove this newly-added line (we only want one blank line between class members)	david	Sept. 22, 2017, 10:23 a.m.
There's now an extra blank line here.	david	Sept. 22, 2017, 10:48 a.m.
This could be a little more compact by combining these into one conditional: if (this._activeCommentBlock !== null && !confirm(gettext('...'))) {	david	Sept. 22, 2017, 10:48 a.m.
One last thing: can we wrap this onto two lines to keep it less wide? Split it after the && …	david	Sept. 22, 2017, 12:08 p.m.
Can I suggest a wording change? How about: "You are currently editing another comment. Would you like to discard it …	chipx86	Sept. 23, 2017, 12:41 p.m.

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/abstractReviewableView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 40
 Expected '!==' and instead saw '!='.
```

Commit:

beecc1182f93950607fa75b0595837c3383fcdd5

532742f8bf9f94e5c96e70af2743119fbc7718b0

Diff:

Revision 2 (+8)

Show changes

reviewboard/static/rb/js/views/abstractReviewableView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/views/abstractReviewableView.es6.js (Diff revision 2)

The issue has been resolved. Show all issues

A few easy comments on this line:

There should be no space between the ! and the confirm
Can you use single-quotes instead of double quotes?
Because this is user-visible text, we should mark it as being localizable by wrapping it in gettext(). Thus: confirm(gettext('Do you ... '))

reviewboard/static/rb/js/views/abstractReviewableView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Please remove this newly-added line (we only want one blank line between class members)
```

Commit:

532742f8bf9f94e5c96e70af2743119fbc7718b0

f34e9329bc3c5cbe08234a6d1751c93d6cc31c8d

Diff:

Revision 3 (+7)

Show changes

reviewboard/static/rb/js/views/abstractReviewableView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

f34e9329bc3c5cbe08234a6d1751c93d6cc31c8d

afec64b40193d0cbd6c8c22e8ee21881ce4085ad

Diff:

Revision 4 (+7)

Show changes

reviewboard/static/rb/js/views/abstractReviewableView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/views/abstractReviewableView.es6.js (Diff revision 4)
The issue has been resolved. Show all issues
```
There's now an extra blank line here.
```

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

The issue has been resolved. Show all issues

This could be a little more compact by combining these into one conditional:

if (this._activeCommentBlock !== null &&
    !confirm(gettext('...'))) {

Commit:

afec64b40193d0cbd6c8c22e8ee21881ce4085ad

169d09698f3e7ef8cc6a5f630d9e88aa7c4b664e

Diff:

Revision 5 (+4)

Show changes

reviewboard/static/rb/js/views/abstractReviewableView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

The issue has been resolved. Show all issues

One last thing: can we wrap this onto two lines to keep it less wide? Split it after the &&

if (this._activeCommentBlock !== null &&
    !confirm('...')) {

Commit:

169d09698f3e7ef8cc6a5f630d9e88aa7c4b664e

db5b8165b5b7cbe1decfca20cb40c6d2b50e04ff

Diff:

Revision 6 (+5)

Show changes

reviewboard/static/rb/js/views/abstractReviewableView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been dropped. Show all issues

Can you add a unit test for this change? Mentors can help with what's required here.

david Sept. 23, 2017, 7:55 a.m.

This is one case where I had suggested that creating a unit test was probably not enough benefit for the work. We don't have existing tests for any of this code, so there's no good place to start, and this one requires a huge amount of state set-up to get to the point where writing a test is even possible.

reviewboard/static/rb/js/views/abstractReviewableView.es6.js (Diff revision 6)

The issue has been resolved. Show all issues

Can I suggest a wording change? How about: "You are currently editing another comment. Would you like to discard it and create a new one?"

This should also probably check that the other comment's state is "dirty." If they're just viewing another one, I don't think it's worth warning them.

Commit:

db5b8165b5b7cbe1decfca20cb40c6d2b50e04ff

9d1163ed1c77454c2f8b2f352b7e40e002f244b0

Diff:

Revision 7 (+6)

Show changes

reviewboard/static/rb/js/views/abstractReviewableView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Testing Done:: Unit tests pass.

Manually tested clicking a new line to add a comment, when a current comment
~ box was open. Pressing OK continues to create a new comment, discarding the
~ unsaved comment. Pressing cancel returns to the current open comment box.
~ box was open. Manually tested clicking a new line to add a comment, when a
~ previously saved comment box was open. Pressing OK continues to create a
+ new comment, discarding the unsaved comment. Pressing cancel returns to
+ the current open comment box.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-3.0.x (555beef)

		Unit tests pass.

		Manually tested clicking a new line to add a comment, when a current comment
~		box was open. Pressing OK continues to create a new comment, discarding the
~		unsaved comment. Pressing cancel returns to the current open comment box.
	~	box was open. Manually tested clicking a new line to add a comment, when a
	~	previously saved comment box was open. Pressing OK continues to create a
	+	new comment, discarding the unsaved comment. Pressing cancel returns to
	+	the current open comment box.