Summary:

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

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

Information
Owner:	justy777
Repository:	Review Board
Branch:	master
Bugs:	3300
Depends On:
Commit:	08a9a70...
Reviewers
Groups:	reviewboard, students
People:

Description

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.

Testing Done

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.

Issues

Description	From	Last Updated
here it is	AS asalahli	Sept. 19, 2014, 11:41 a.m.
.hideAndReload() could be indented one more level	dkus	Sept. 19, 2014, 12:58 p.m.
The indentation in this line got unintentionally changed. Please change it back.	david	Sept. 20, 2014, 7:36 a.m.
This inline function has gotten quite long, with a lot of really deep indentation. Can you pull this into a …	david	Sept. 20, 2014, 7:36 a.m.
Is this var being used?	dkus	Sept. 20, 2014, 7:37 a.m.
Since _onDiscardClicked returns false, you can just do: .click(_.bind(this._onDiscardClicked, this));	david	Sept. 20, 2014, 8:13 a.m.
This should be indented only 4 spaces.	david	Sept. 20, 2014, 8:13 a.m.
You've got a trailing space here.	david	Sept. 20, 2014, 8:33 a.m.
No space before ().	chipx86	Sept. 20, 2014, 9:28 a.m.
Instead of embedding the localized string directly in the HTML, you should do: $('<input type="button"/>') .val(gettext('Cancel')) ... etc. The reason …	chipx86	Sept. 20, 2014, 9:26 a.m.

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewDialogView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewDialogView.js

reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 1)
```
Did .hideAndReload() disappear in the new version? Isn't it needed?
```
1. justy777 Sept. 19, 2014, 11:23 a.m.
  I believe your right, good eye.
2. justy777 Sept. 19, 2014, 11:25 a.m.
  please open an issue.

reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 1)
Show all issues
```
here it is
```

Change Summary:

Added back this.close() and .hideAndReload() as part of the confirmed discard.

Commit:

-	2a207300a16288610c622294043407e59d416a34
+	43efb4efcabaf93a6760580557a342579420007d

Diff:

Revision 2 (+22 -9)

Show changes

	reviewboard/static/rb/js/views/draftReviewBannerView.js
	reviewboard/static/rb/js/views/reviewDialogView.js

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

reviewboard/static/rb/js/views/draftReviewBannerView.js (Diff revision 2)
Show all issues
```
.hideAndReload() could be indented one more level
```

Change Summary:

Indented .hideAndReload()

Commit:

-	43efb4efcabaf93a6760580557a342579420007d
+	5903d00d37cd8d0d6647b7b73f08a1148574b573

Diff:

Revision 3 (+22 -9)

Show changes

	reviewboard/static/rb/js/views/draftReviewBannerView.js
	reviewboard/static/rb/js/views/reviewDialogView.js

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

Change Summary:

Changed the wrong file, file reverted and the right file is changed.

Commit:

-	5903d00d37cd8d0d6647b7b73f08a1148574b573
+	0f7ec9c29e3aa5d18300d7b60538921424fbb8ec

Diff:

Revision 4 (+29 -9)

Show changes

	reviewboard/static/rb/js/views/draftReviewBannerView.js
	reviewboard/static/rb/js/views/reviewDialogView.js

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

Change Summary:

Changed back the wrong file for real this time

Commit:

-	0f7ec9c29e3aa5d18300d7b60538921424fbb8ec
+	a9d123cf79d2158098fe774f039d62df3b8ac3ec

Diff:

Revision 5 (+22 -8)

Show changes

reviewboard/static/rb/js/views/reviewDialogView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewDialogView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewDialogView.js

Change Summary:

forgot the "this" in front of model

Commit:

-	a9d123cf79d2158098fe774f039d62df3b8ac3ec
+	db9a096a76a72694effd17c29885429fe73f4868

Diff:

Revision 6 (+21 -8)

Show changes

reviewboard/static/rb/js/views/reviewDialogView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewDialogView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewDialogView.js

Ship it!

```
Ship It!
```

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

Show all issues

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

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

Show all issues

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

reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 6)
Show all issues
```
Is this var being used?
```
1. justy777 Sept. 20, 2014, 7:37 a.m.
  it is now being used for model.destroy() on line 777.

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

What happens when you click the Cancel button instead of discarding it?

Change Summary:

Broke out the inline method that handles clicking on the discard button

Commit:

-	db9a096a76a72694effd17c29885429fe73f4868
+	e1e0c47044bccffb7ed7c0ce60f9ec7bd7bb8c0a

Diff:

Revision 7 (+34 -9)

Show changes

reviewboard/static/rb/js/views/reviewDialogView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewDialogView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewDialogView.js

Pretty close, just a couple more things:

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

Show all issues

Since _onDiscardClicked returns false, you can just do:

.click(_.bind(this._onDiscardClicked, this));

reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 7)
Show all issues
```
This should be indented only 4 spaces.
```

Change Summary:

Made an explicit self variable, and collapsed the inline bind function for discard button.

Commit:

-	e1e0c47044bccffb7ed7c0ce60f9ec7bd7bb8c0a
+	6dd4701f2d1634b1df440a6acdeaec90b8d0c683

Diff:

Revision 8 (+34 -11)

Show changes

reviewboard/static/rb/js/views/reviewDialogView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewDialogView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewDialogView.js

Can you list all of the different test cases you've executed?

reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 8)
Show all issues
```
You've got a trailing space here.
```

Change Summary:

Added details to the 'testing done' field.

Testing Done:

~		Tested visually, by publishing a review. Going to review it, add a comment and clicking the discard review button.
	~	Tested visually, by publishing 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.

Change Summary:

Deleted trailing space

Commit:

-	6dd4701f2d1634b1df440a6acdeaec90b8d0c683
+	08a9a707e3a4be7c45613836a5965bb507e87ab9

Diff:

Revision 9 (+34 -11)

Show changes

reviewboard/static/rb/js/views/reviewDialogView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewDialogView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewDialogView.js

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (8faeb03)

reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 9)
Show all issues
```
No space before ().
```

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

Show all issues

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.

justy777 Sept. 20, 2014, 9:28 a.m.

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

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.