Summary

Fixed a bug where "Start Over" in Update Diff creates a new review request.

Review Request #8402 — Created Sept. 17, 2016 and submitted Oct. 25, 2016, 9:57 p.m.

Information

Owner

Connor-Y

Repository

Review Board

Branch

release-2.5.x

Bugs

4404

Depends On

Commit

fa15150...

Reviewers

Groups

reviewboard, students

People

Description

Uploading a bad Diff in the UpdateDiff window causes a Start Over window.
When starting over, a valid diff creates a new review instead of updating.

Fixed by removing code that set the review request number to null.
The request number is now properly retained and no new review is created.

Testing Done

Testing done via reaching Start Over message, then uploading a proper diff.
All results ended with the review being properly updated.

Tested invalid, unparsable and empty files to reach the "Start Over" page.
Tested (via modifying the code) to directly land on the "Start Over" page.
Tested uploading multiple incorrect files then a correct file.
Tested with multiple diff files.
Tested both by using the 'Select a diff' option and clicking and dragging.
Tested on both the Update page menu and the "New Review Request" page.
Tested via Jasmine that StartOver doesn't change reviewRequest.
All tests done on Firefox 48.0, Reviewboard 2.5.x and Python2.7.12

Issues

Description	From	Last Updated
A couple comments I'm making on everyone's changes (this is pretty common for the initial review requests): 1) Your Description …	chipx86	Sept. 23, 2016, 9:41 a.m.
Your testing mentions Review Board 2.6, which isn't a thing. Did you mean 2.5? The branch still lists "master".	chipx86	Sept. 23, 2016, 10:06 a.m.
This model is also used on the "New Review Request" page. Can you verify that "Start Over" still works correctly …	david	Sept. 18, 2016, 1:06 p.m.
I don't think this is the right fix. We want defaults for every attribute in the model. It seems more …	chipx86	Sept. 20, 2016, 7:28 p.m.
One space before function()	mike_conley	Oct. 5, 2016, 9:40 a.m.
It would be nice to use " instead of \'	david	Oct. 5, 2016, 9:41 a.m.
It would be nice to put the .and.callThrough(...) on the next line to keep things close to 80 characters in …	david	Oct. 5, 2016, 9:44 a.m.
Busted indentation in here, I think.	mike_conley	Oct. 5, 2016, 9:41 a.m.
It looks like the first-level things are only indented 3 spaces instead of 4. Please make sure that all indentation …	david	Oct. 5, 2016, 10:27 a.m.
Can you format this as one variable per line? var reviewRequest, reviewRequestEditor;	david	Oct. 5, 2016, 10:28 a.m.
This needs to be specified in the var statement above. Otherwise you're creating a new global variable.	david	Oct. 5, 2016, 10:28 a.m.
When writing ES5, we merge everything into a single var statement, like this: var priorRequest = reviewRequest, model; That said, …	david	Oct. 5, 2016, 10:29 a.m.
There's an extra blank line here.	david	Oct. 5, 2016, 10:29 a.m.
Can you undo this change?	david	Oct. 5, 2016, 10:30 a.m.
This doesn't seem to be testing anything--you assign the priorRequest variable once, and then test it against what you assigned …	david	Oct. 7, 2016, 2:32 p.m.
Switch quotes around here.	brennie	Oct. 11, 2016, 5:03 p.m.

I think the code looks OK, but I'd like some changes to the commit message/review request description.

The bug number should be listed in the "Bugs" field, not the summary or description. The summary should be a single line that describes the change (such as "Fix a bug when clicking "Start Over" in the "Update Diff" dialog." The description should be a longer summary of what the problem was and how you solved it (so that we don't need to open the bug tracker to know what the change was about).

See https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/ for a more complete explanation of what we look for in these, along with examples.

reviewboard/static/rb/js/models/uploadDiffModel.js (Diff revision 1)

The issue has been resolved. Show all issues

This model is also used on the "New Review Request" page. Can you verify that "Start Over" still works correctly there?

Summary:

-	Fixed Error#4404 by removing code that set the reviewquest number to null accidentally
+	Fixed a bug caused clicking 'Start Over' in 'Update Diff' window.

Description:

~		Fixed Error#4404 by removing code that set the reviewrequest number to null accidentally when pressing the "Start Over" button.
	~	Uploading an invalid Diff in the Update Diff window causes a 'Start Over' window to be displayed. Pressing 'Start Over' then uploading any valid diff creates a new review as opposed to properly updating the current one.
	+
	+	This was fixed by removing code that set the review request number to null when pressing the "Start Over" button. The code now properly retains the request number and thus does not create a new review.

Bugs:

+	4404

Testing Done:

		Testing done by reaching the "Start Over" message, clicking it and then uploading a proper diff. All results ended with the review being properly updated.
		Tested invalid, unparsable and empty files to reach the "Start Over" page.
		Tested (via modifying the code) to directly land on the "Start Over" page.
		Tested uploading multiple incorrect files then a correct file.
		Tested with multiple diff files.
		Tested both by using the 'Select a diff' option and clicking and dragging.
	+	Tested on both the Update page menu and the "New Review Request" page.
		All tests done on Firefox 48.0, Reviewboard 2.6 and Python2.7.12

reviewboard/static/rb/js/models/uploadDiffModel.js (Diff revision 1)

The issue has been dropped. Show all issues

I don't think this is the right fix. We want defaults for every attribute in the model. It seems more like the dialog has the proper state initially, but some state is missing when clicking Start Over. That should be fixed.

A unit test will need to be written for the resulting fix, to ensure that the resulting state is correct after clicking Start Over.

chipx86 Sept. 20, 2016, 7:28 p.m.

Sorry! I misread the code at the time. I think this looks correct to me.

For the unit test, you'll need to create an uploadDiffModelTests.js under rb/js/models/tests/, which would set up a test ensuring that a call to startOver does not change reviewRequest. You can look at other tests for examples on structure.

The issue has been resolved. Show all issues

A couple comments I'm making on everyone's changes (this is pretty common for the initial review requests):

1) Your Description and Testing Done should wrap at around 75 characters on each line. This helps when we turn these into commits, since Git commits should usually be no more than around 75 characters per line. This applies to the summary as well (which I know can be hard, and it's okay if this goes up to 79). A good way to ensure this is to make sure your editor is set to wrap lines at 75 characters when editing a git commit message.

2) The Summary should be more explicit. It reads that a bug caused "Start Over" to be clicked in "Update Diff," which isn't what you meant. Something better might be: Fixed a bug where "Start Over" in Update Diff creates a new review request.

Summary:

-	Fixed a bug caused clicking 'Start Over' in 'Update Diff' window.
+	Fixed a bug where "Start Over" in Update Diff creates a new review request.

Description:

~		Uploading an invalid Diff in the Update Diff window causes a 'Start Over' window to be displayed. Pressing 'Start Over' then uploading any valid diff creates a new review as opposed to properly updating the current one.
	~	Uploading a bad Diff in the UpdateDiff window causes a Start Over window.
	+	When starting over, a valid diff creates a new review instead of updating.

~		This was fixed by removing code that set the review request number to null when pressing the "Start Over" button. The code now properly retains the request number and thus does not create a new review.
	~	Fixed by removing code that set the review request number to null.
	+	The request number is now properly retained and no new review is created.

Testing Done:

~		Testing done by reaching the "Start Over" message, clicking it and then uploading a proper diff. All results ended with the review being properly updated.
~		Tested invalid, unparsable and empty files to reach the "Start Over" page.
	~	Testing done via reaching Start Over message, then uploading a proper diff.
	~	All results ended with the review being properly updated.
	+
	+	Tested invalid, unparsable and empty files to reach the "Start Over" page.
		Tested (via modifying the code) to directly land on the "Start Over" page.
		Tested uploading multiple incorrect files then a correct file.
		Tested with multiple diff files.
		Tested both by using the 'Select a diff' option and clicking and dragging.
		Tested on both the Update page menu and the "New Review Request" page.
	+	Tested via Jasmine that StartOver doesn't change reviewRequest.
		All tests done on Firefox 48.0, Reviewboard 2.6 and Python2.7.12

Commit:

-	0c883cf434066150e828f8de9bfb29235104b17c
+	c171092f7641182efb5877f8f76a0427a755c8fa

Diff:

Revision 2 (+31 -1)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/models/uploadDiffModel.js
	reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/uploadDiffModel.js
    reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/uploadDiffModel.js
    reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js

The issue has been resolved. Show all issues

Your testing mentions Review Board 2.6, which isn't a thing. Did you mean 2.5?

The branch still lists "master".

Testing Done:

		Testing done via reaching Start Over message, then uploading a proper diff.
		All results ended with the review being properly updated.

		Tested invalid, unparsable and empty files to reach the "Start Over" page.
		Tested (via modifying the code) to directly land on the "Start Over" page.
		Tested uploading multiple incorrect files then a correct file.
		Tested with multiple diff files.
		Tested both by using the 'Select a diff' option and clicking and dragging.
		Tested on both the Update page menu and the "New Review Request" page.
		Tested via Jasmine that StartOver doesn't change reviewRequest.
~		All tests done on Firefox 48.0, Reviewboard 2.6 and Python2.7.12
	~	All tests done on Firefox 48.0, Reviewboard 3.0 and Python2.7.12

reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js (Diff revision 2)
The issue has been resolved. Show all issues
```
One space before function()
```
reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Busted indentation in here, I think.
```

Can you rebase this change onto release-2.5.x?

Connor-Y Sept. 24, 2016, 8:21 a.m.

Someone told me to leave it on 3.0.x and they would tell me if there were problems. Should I rebase anyways?

david Sept. 24, 2016, 8:22 a.m.
```
Hmm. Who was that?
```
CO

Connor-Y Sept. 24, 2016, 11:42 a.m.
```
I think it was Barret.
```

david Sept. 29, 2016, 11:06 a.m.

Given that the bug was reported against 2.5.x, and that's our current stable release, that's where we should fix it.

david Sept. 29, 2016, 11:07 a.m.

New feature development should happen on release-3.0.x or master.

reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js (Diff revision 2)
The issue has been resolved. Show all issues
```
It would be nice to use " instead of \'
```

reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js (Diff revision 2)

The issue has been resolved. Show all issues

It would be nice to put the .and.callThrough(...) on the next line to keep things close to 80 characters in width.

Change Summary:

Rebased change from release-3.0.x to release-2.5.x. Fixed formatting errors.

Testing Done:

		Testing done via reaching Start Over message, then uploading a proper diff.
		All results ended with the review being properly updated.

		Tested invalid, unparsable and empty files to reach the "Start Over" page.
		Tested (via modifying the code) to directly land on the "Start Over" page.
		Tested uploading multiple incorrect files then a correct file.
		Tested with multiple diff files.
		Tested both by using the 'Select a diff' option and clicking and dragging.
		Tested on both the Update page menu and the "New Review Request" page.
		Tested via Jasmine that StartOver doesn't change reviewRequest.
~		All tests done on Firefox 48.0, Reviewboard 3.0 and Python2.7.12
	~	All tests done on Firefox 48.0, Reviewboard 2.5.x and Python2.7.12

Branch:

-	master
+	release-2.5.x

Commit:

-	c171092f7641182efb5877f8f76a0427a755c8fa
+	368dbee95aac09e5899ce51583081fd572a13024

Diff:

Revision 3 (+33 -1)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/models/uploadDiffModel.js
	reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/uploadDiffModel.js
    reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/uploadDiffModel.js
    reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js

reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js (Diff revision 3)

The issue has been resolved. Show all issues

It looks like the first-level things are only indented 3 spaces instead of 4. Please make sure that all indentation are multiples of 4.

reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js (Diff revision 3)

The issue has been resolved. Show all issues

Can you format this as one variable per line?

var reviewRequest,
    reviewRequestEditor;

reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js (Diff revision 3)
The issue has been resolved. Show all issues
```
This needs to be specified in the var statement above. Otherwise you're creating a new global variable.
```

reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js (Diff revision 3)

The issue has been resolved. Show all issues

When writing ES5, we merge everything into a single var statement, like this:

var priorRequest = reviewRequest,
    model;


That said, it probably makes sense to define model inline in the call-through function (var model = options.model;) rather than here in the containing function.

reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js (Diff revision 3)
The issue has been resolved. Show all issues
```
There's an extra blank line here.
```
reviewboard/staticbundles.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can you undo this change?
```

Change Summary:

Fixed formatting errors.

Commit:

-	368dbee95aac09e5899ce51583081fd572a13024
+	d949da229c782b9087fab6c0e5f37848f44c66e3

Diff:

Revision 4 (+32 -1)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/models/uploadDiffModel.js
	reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/uploadDiffModel.js
    reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/uploadDiffModel.js
    reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js

reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js (Diff revision 4)

The issue has been resolved. Show all issues

This doesn't seem to be testing anything--you assign the priorRequest variable once, and then test it against what you assigned to it. That can never fail.

I think what you ought to be checking is expect(model.get('reviewRequest').toBe(reviewRequest)

Connor-Y Oct. 7, 2016, 2:32 p.m.

Thanks for pointing this out David. It turns out that the entire function wasn't doing what I was supposed to at all and because of that line it was returning true when run. I've properly rewritten the function to work.

Change Summary:

Completely rewrote testing function to properly verify that that review request is not modified by the startOver function.

Commit:

-	d949da229c782b9087fab6c0e5f37848f44c66e3
+	76fba7bf295888278e60cac1c483532077d77ce8

Diff:

Revision 5 (+29 -1)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/models/uploadDiffModel.js
	reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/uploadDiffModel.js
    reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/uploadDiffModel.js
    reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js

Ship it!

reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Switch quotes around here.
```

Change Summary:

Switched quoutes around

Commit:

-	76fba7bf295888278e60cac1c483532077d77ce8
+	fa15150036dd21bdba996da0b66d560853eee0e6

Diff:

Revision 6 (+29 -1)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/models/uploadDiffModel.js
	reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/uploadDiffModel.js
    reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/uploadDiffModel.js
    reviewboard/static/rb/js/models/tests/uploadDiffModelTests.js

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (33ae429)