fixing Issue 3300:Add a confirmation when discarding a pending review
Review Request #6341 — Created Sept. 19, 2014 and submitted
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.
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.
| Description | From | Last Updated |
|---|---|---|
|
here it is |
AS asalahli | |
|
.hideAndReload() could be indented one more level |
 dkus
|
|
|
The indentation in this line got unintentionally changed. Please change it back. |
 david
|
|
|
This inline function has gotten quite long, with a lot of really deep indentation. Can you pull this into a … |
david
|
|
|
Is this var being used? |
dkus
|
|
|
Since _onDiscardClicked returns false, you can just do: .click(_.bind(this._onDiscardClicked, this)); |
david
|
|
|
This should be indented only 4 spaces. |
david
|
|
|
You've got a trailing space here. |
david
|
|
|
No space before (). |
 chipx86
|
|
|
Instead of embedding the localized string directly in the HTML, you should do: $('<input type="button"/>') .val(gettext('Cancel')) ... etc. The reason … |
chipx86
|
AS
-
-
reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 1) Did
.hideAndReload()disappear in the new version? Isn't it needed?
Change Summary:
Added back this.close() and .hideAndReload() as part of the confirmed discard.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+22 -9) |
-
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) .hideAndReload() could be indented one more level
Change Summary:
Indented .hideAndReload()
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+22 -9) |
-
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: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+29 -9) |
-
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: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+22 -8) |
-
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: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+21 -8) |
-
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 6) The indentation in this line got unintentionally changed. Please change it back.
-
reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 6) 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))?
AD
-
-
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: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 7 (+34 -9) |
-
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) Since
_onDiscardClickedreturns false, you can just do:.click(_.bind(this._onDiscardClicked, this)); -
reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 7) This should be indented only 4 spaces.
Change Summary:
Made an explicit self variable, and collapsed the inline bind function for discard button.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+34 -11) |
-
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) You've got a trailing space here.
Change Summary:
Added details to the 'testing done' field.
Testing Done: |
|
|---|
Change Summary:
Deleted trailing space
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 9 (+34 -11) |
-
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 9) 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.
Change Summary:
Pushed to release-2.0.x (8faeb03)
Testing Done: |
|
|---|
Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/views/reviewDialogView.js Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/views/reviewDialogView.js