Summary

Fix the `Download Diff` issue in Chrome (v16 or newer) when filename has comma(s).

Review Request #6786 — Created Jan. 16, 2015 and submitted March 17, 2015, 3:34 p.m.

Information

Owner

Chester

Repository

Review Board

Branch

master

Bugs

3704

Depends On

Commit

0dad998...

Reviewers

Groups

reviewboard

People

Description

If a diff file with a comma in the filename was uploaded via the web UI, clicking "Download Diff" could present an error about "Multiple Content-Disposition" when running with Chrome 16 or newer.

The problem is that we use the Content-Disposition header to tell the browser what filename to save as, but commas in this header are interpreted as multiple values.

To fix this, before we assemble the Content-Disposition header, we replace all commas in the diffset filename with underscores.

Testing Done


Wrote a unit test, in the test, I set the diffset.name as 'test, comma'.
Unit test passed.
Unit test failed whitout the fix.

Issues

Description	From	Last Updated
Col: 80 E501 line too long (97 > 79 characters)	reviewbot	Jan. 16, 2015, 11:48 a.m.
Typo - "Chrom" - "Chrome". Perhaps you should just generalize this and put above this line: # Some older browsers …	mike_conley	Jan. 16, 2015, 11:50 a.m.
Col: 73 W291 trailing whitespace	reviewbot	Jan. 16, 2015, 11:51 a.m.
Come to think of it, it might be a good idea to refer to the issue # itself here. So …	mike_conley	Jan. 17, 2015, 3:22 p.m.
Another type-o - "nmae" -> "name".	mike_conley	Jan. 17, 2015, 3:22 p.m.
Blank line between these. "No.3704" doesn't indicate what this is. Better to provide a link to the bug.	chipx86	Jan. 18, 2015, 10:31 a.m.
This doesn't seem related to the fix.	chipx86	Jan. 18, 2015, 10:14 a.m.
Just a reminder, we'd still like to see a unit test added for this.	david	Feb. 27, 2015, 4:32 a.m.
Does this test fail if you comment out the change in views.py? I don't see you setting a filename with …	david	Feb. 25, 2015, 6:13 p.m.
Col: 51 E211 whitespace before '['	reviewbot	Feb. 25, 2015, 6:12 p.m.
Col: 29 E127 continuation line over-indented for visual indent	reviewbot	Feb. 25, 2015, 6:12 p.m.
This still doesn't test the failure case because there's no comma in the filename! Please comment out your change in …	david	March 3, 2015, 10:38 a.m.
Col: 51 E211 whitespace before '['	reviewbot	Feb. 27, 2015, 4:55 a.m.
Col: 29 E127 continuation line over-indented for visual indent	reviewbot	Feb. 27, 2015, 4:55 a.m.
Col: 41 E128 continuation line under-indented for visual indent	reviewbot	March 3, 2015, 10:53 a.m.
Col: 51 E211 whitespace before '['	reviewbot	March 3, 2015, 10:53 a.m.
Col: 29 E127 continuation line over-indented for visual indent	reviewbot	March 3, 2015, 10:53 a.m.
Col: 51 E211 whitespace before '['	reviewbot	March 7, 2015, 12:15 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py

reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (97 > 79 characters)
```

A couple of typos in the description in the URL: "reviewsboard" and "revies".

reviewboard/reviews/views.py (Diff revision 1)

The issue has been resolved. Show all issues

Typo - "Chrom" - "Chrome".

Perhaps you should just generalize this and put above this line:

# Some older browsers don't like commas in filenames,
# so we work around that by replacing them with
# underscores.

Just a suggestion.

Commit:

0223ea2b6e295abf967a9b8554f9e0b85c0cc5bb

53b8a8f74ef590a5e8538b86c5c6aaee4a3c644c

Diff:

Revision 2 (+3)

Show changes

reviewboard/reviews/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py

reviewboard/reviews/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 73
 W291 trailing whitespace
```

Commit:

53b8a8f74ef590a5e8538b86c5c6aaee4a3c644c

e5c45027a971ae4141a7e4127aae677942004d44

Diff:

Revision 3 (+3)

Show changes

reviewboard/reviews/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py

Can you link to the google code issue too? I want some background.

Chester Jan. 16, 2015, 11:55 a.m.

https://code.google.com/p/chromium/issues/detail?id=108188

Description:: ~
change reviewsboard/revies/views.py to be compatible with old version chrome(v16)
~
change reviewsboard/revies/views.py to be compatible with old version chrome(v16). When you Download Diff on some specific page, there might be "Multiple Content-Disposition" error page on Chrome. It's caused by the comma in the filename, so just need to swap it with an underscore
Bugs:: 3704

You still have typos in your summary/description (reviewsboard, revies)

Chester Jan. 16, 2015, 5:05 p.m.
```
changed it, thank you!
```

david Jan. 16, 2015, 5:07 p.m.

You still have typos in the summary, and it still says "reviewsboard"

Summary:: change reviewsboard/revies/views.py to be compatible with old version chrome
Change reviewsboard/reviews/views.py to be compatible with old version chrome

Summary:: Change reviewsboard/reviews/views.py to be compatible with old version chrome
Change reviewboard/reviews/views.py to be compatible with old version chrome

Description:: ~
change reviewsboard/revies/views.py to be compatible with old version chrome(v16). When you Download Diff on some specific page, there might be "Multiple Content-Disposition" error page on Chrome. It's caused by the comma in the filename, so just need to swap it with an underscore
~
change reviewboard/revies/views.py to be compatible with old version chrome(v16). When you Download Diff on some specific page, there might be "Multiple Content-Disposition" error page on Chrome. It's caused by the comma in the filename, so just need to swap it with an underscore

reviewboard/reviews/views.py (Diff revision 3)

The issue has been resolved. Show all issues

Come to think of it, it might be a good idea to refer to the issue # itself here. So maybe your comment can become:

"This next bit fixes issue 3704, where Multiple Content-Disposition error pages were shown on older versions of Chrome when attempting to download a diff with a comma in the filename. We workaround this by replacing commas with underscores."

reviewboard/reviews/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Another type-o - "nmae" -> "name".
```

Description:

~		change reviewboard/revies/views.py to be compatible with old version chrome(v16). When you Download Diff on some specific page, there might be "Multiple Content-Disposition" error page on Chrome. It's caused by the comma in the filename, so just need to swap it with an underscore
	~	I modify reviewboard/reviews/views.py to be compatible with old version Chrome(v16 or older). When you click `Download Diff` on some code review page, there might be "Multiple Content-Disposition" error page on Chrome. It's caused by the comma in the filename, so I just need to replace it with an underscore.

Commit:

e5c45027a971ae4141a7e4127aae677942004d44

41b81a934859df2ddce7bbff051cc32f0eb5c6c9

Diff:

Revision 4 (+6 -2)

Show changes

	reviewboard/reviews/views.py
	reviewboard/templates/admin/scmtools/repository/rbtools_setup.html

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py

Ignored Files:
    reviewboard/templates/admin/scmtools/repository/rbtools_setup.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py

Ignored Files:
    reviewboard/templates/admin/scmtools/repository/rbtools_setup.html

Summary:: Change reviewboard/reviews/views.py to be compatible with old version chrome
Modify reviewboard/reviews/views.py to be compatible with old version Chrome

Hi Chester,

views.py is a pretty big file, and most of it is fine with Chrome. Can you update the summary describing more precisely what's being made compatible? Remember, someone unfamiliar with the codebase should be able to know what you're fixing.

The description should also clearly explain the problem you're solving, why it was broken, and how you're solving it, at a high level.

Summary:: Modify reviewboard/reviews/views.py to be compatible with old version Chrome
Modify reviewboard/reviews/views.py `Download Diff` function to be compatible with old version Chrome

Along with this, I'd like to see a unit test that verifies the resulting filename is what we expect. Unit tests are very important to our product, so it's good to start learning with simple ones like this.

reviewboard/reviews/views.py (Diff revision 4)

The issue has been resolved. Show all issues

Blank line between these.

"No.3704" doesn't indicate what this is. Better to provide a link to the bug.

chipx86 Jan. 18, 2015, 10:23 a.m.
```
This wasn't actually fixed.
```

Chester Jan. 18, 2015, 10:32 a.m.

just added a blank line before the comment block

reviewboard/templates/admin/scmtools/repository/rbtools_setup.html (Diff revision 4)

The issue has been dropped. Show all issues

This doesn't seem related to the fix.

Chester Jan. 18, 2015, 10:20 a.m.

sorry, I posted by mistake, this diff belongs to request 6788, which has landed.

chipx86 Jan. 18, 2015, 10:23 a.m.

No problem. Fix the first issue in this review, fix the summary, add a unit test, and this will be ready to get in!

Also, this issue was fixed, since you reverted the change. It shouldn't be dropped.

Summary:

Modify reviewboard/reviews/views.py `Download Diff` function to be compatible with old version Chrome

Modify reviewboard/reviews/views.py to fix Download Diff to be compatible with old versions of Chrome (v16 or older)

Commit:

41b81a934859df2ddce7bbff051cc32f0eb5c6c9

5bd8aab37de65ea84fd5e1d02e62637839a5847b

Diff:

Revision 5 (+5)

Show changes

reviewboard/reviews/views.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py



Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py

Summary:

Modify reviewboard/reviews/views.py to fix Download Diff to be compatible with old versions of Chrome (v16 or older)

Modify reviewboard/reviews/views.py to fix Download Diff to be compatible with some versions of Chrome (v16 or newer)

Commit:

5bd8aab37de65ea84fd5e1d02e62637839a5847b

222adb61f0dbb3d2b23c9ca9170a3a8bb213b02d

Diff:

Revision 6 (+6)

Show changes

reviewboard/reviews/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py

Description:: ~
I modify reviewboard/reviews/views.py to be compatible with old version Chrome(v16 or older). When you click Download Diff on some code review page, there might be "Multiple Content-Disposition" error page on Chrome. It's caused by the comma in the filename, so I just need to replace it with an underscore.
~
I modify reviewboard/reviews/views.py to be compatible with some versions of Chrome(v16 or newer). When you click Download Diff on some code review page, there might be "Multiple Content-Disposition" error page on Chrome. It's caused by the comma in the filename, so I just need to replace it with an underscore.

Summary:: Modify reviewboard/reviews/views.py to fix Download Diff to be compatible with some versions of Chrome (v16 or newer)
[WIP]Modify reviewboard/reviews/views.py to fix Download Diff to be compatible with some versions of Chrome (v16 or newer)

reviewboard/reviews/views.py (Diff revision 6)

The issue has been resolved. Show all issues

Just a reminder, we'd still like to see a unit test added for this.

Chester Feb. 22, 2015, 6:13 p.m.

I will implement it. But now I am working on gitlab project, so please let me finish most part of that project and then come back in about 2 weeks. Thank you for reminding!!!

Commit:

222adb61f0dbb3d2b23c9ca9170a3a8bb213b02d

fa432e8b036f312990800fa1fc834d1288923bee

Diff:

Revision 7 (+19)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/tests.py

reviewboard/reviews/tests.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 51
 E211 whitespace before '['
```
reviewboard/reviews/tests.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 29
 E127 continuation line over-indented for visual indent
```

Summary:: [WIP]Modify reviewboard/reviews/views.py to fix Download Diff to be compatible with some versions of Chrome (v16 or newer)
Modify reviewboard/reviews/views.py to fix Download Diff to be compatible with some versions of Chrome (v16 or newer)

reviewboard/reviews/tests.py (Diff revision 7)

The issue has been resolved. Show all issues

Does this test fail if you comment out the change in views.py? I don't see you setting a filename with a comma in it.

Chester Feb. 16, 2015, 8:14 p.m.

sorry, never thought about it, I will try to fix it. Thank you!!!

Commit:

fa432e8b036f312990800fa1fc834d1288923bee

b3a57e8eabf07938ad0150f5ede3519e61b7b633

Diff:

Revision 8 (+20)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py

Still expecting to see the summary changed, as per my prior comments.

Chester Feb. 27, 2015, 4:41 a.m.

just changed it, please tell me if I need to improve the summary. Thank you!!!

Summary:: Modify reviewboard/reviews/views.py to fix Download Diff to be compatible with some versions of Chrome (v16 or newer)
Fix the `Download Diff` issue in Chrome (v16 or newer) when filename has comma

Description:

~		I modify reviewboard/reviews/views.py to be compatible with some versions of Chrome(v16 or newer). When you click `Download Diff` on some code review page, there might be "Multiple Content-Disposition" error page on Chrome. It's caused by the comma in the filename, so I just need to replace it with an underscore.
	~	I modify reviewboard/reviews/views.py to be compatible with some versions of Chrome(v16 or newer). When you click `Download Diff` on some code review page, there might be "Multiple Content-Disposition" error page on Chrome. It's caused by the comma in the filename, when `diffset.name != 'diff'`, so I just need to replace it with an underscore.

Testing Done:

~		Only ran reviews/tests module, passed.
	~	Wrote a unit test, in the test, I set the diffset.name as 'test'.
	+	Unit test passed.

Commit:

b3a57e8eabf07938ad0150f5ede3519e61b7b633

bde64b37f2e317fc39ef5d30de606e35bd152c01

Diff:

Revision 9 (+20)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/tests.py

reviewboard/reviews/tests.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 51
 E211 whitespace before '['
```
reviewboard/reviews/tests.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 29
 E127 continuation line over-indented for visual indent
```

Summary:: Fix the `Download Diff` issue in Chrome (v16 or newer) when filename has comma
Fix the `Download Diff` issue in Chrome (v16 or newer) when filename has comma(s).

reviewboard/reviews/tests.py (Diff revision 9)

The issue has been resolved. Show all issues

This still doesn't test the failure case because there's no comma in the filename! Please comment out your change in views.py and make sure that without your fix, this unit test fails, and with your fix, it succeeds.

Chester March 2, 2015, 5:04 p.m.

sorry, I don't quite understand "the unit test fails without my fix, but succeeds with it" part, do I need to create two diffset objects in the unit test function, or to add an assertTrue()?

david March 2, 2015, 5:08 p.m.

Your implementation of this test does not actually test that the bug is fixed, because the filename you're specifying (name='test') does not have a comma in it. Therefore if you were to comment out your added line in views.py (the one that actually fixes the bug), this test would still pass.

When a unit test purports to verify a bug fix, that test should fail before the bug fix is applied, and succeed afterwards.

Chester March 2, 2015, 5:41 p.m.

The comma doesn't seem to appear always when diffset.name != 'diff'. If so, could you please give me some hints how to reproduce the problem (with the comma in the filename)?

I can't reproduce this problem on my computer and the chrome, does it matter?

Chester March 2, 2015, 6:55 p.m.

I tried the diffset.name as default ('diffset'), 'test' and 'diff', but all of them did not have a comma in the filename. Can you reproduce the problem from the unit test?

Commit:

bde64b37f2e317fc39ef5d30de606e35bd152c01

c2d762fd2a4e6a4052ec0597bf3dafc267f49e50

Diff:

Revision 10 (+20)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/tests.py

reviewboard/reviews/tests.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Col: 41
 E128 continuation line under-indented for visual indent
```
reviewboard/reviews/tests.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Col: 51
 E211 whitespace before '['
```
reviewboard/reviews/tests.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Col: 29
 E127 continuation line over-indented for visual indent
```

Testing Done:

~		Wrote a unit test, in the test, I set the diffset.name as 'test'.
	~	Wrote a unit test, in the test, I set the diffset.name as 'test, comma'.
		Unit test passed.
	+	Unit test failed whitout the fix.

Commit:

c2d762fd2a4e6a4052ec0597bf3dafc267f49e50

0dad998601897a8eea24100fbbe419876ee3b4f2

Diff:

Revision 11 (+20)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/tests.py

reviewboard/reviews/tests.py (Diff revision 11)
The issue has been dropped. Show all issues
```
Col: 51
 E211 whitespace before '['
```
1. Chester March 3, 2015, 10:54 a.m.
  I didn't see a white space, anything wrong?

The patch looks good! However, I've got a few nitpicks regarding your description. We generally don't use the first person (I) when writing descriptions. They usually don't reference the author at all. You also don't usually need to mention the files that changed (or code examples); just provide a high level overview of the changes. (e.g., "Previously when X occured, Y would happen because Z. This has been fixed to do the correct behaviour: ABC"..., etc.)

Chester March 16, 2015, 10:57 a.m.

I think the path should be mentioned here because diffset appears in many places and I would like to specify where the problem comes from.

david March 16, 2015, 5:26 p.m.

While it's true that diffset appears in many places, I can just look at the code to see where you've changed things. The purpose of the description is instead to explain more of the why and how. How about something like this:

If a diff file with a comma in the filename was uploaded via the web UI, clicking "Download Diff" could present an error about "Multiple Content-Disposition" when running with Chrome 16 or newer. The problem is that we use the Content-Disposition header to tell the browser what filename to save as, but commas in this header are interpreted as multiple values. To fix this, before we assemble the Content-Disposition header, we replace all commas in the diffset filename with underscores.

Please also take care when writing to spell and grammar check your text.

Chester March 17, 2015, 7:52 a.m.
```
I will be careful, thank you!!!
```

Description:: ~
I modify reviewboard/reviews/views.py to be compatible with some versions of Chrome(v16 or newer). When you click Download Diff on some code review page, there might be "Multiple Content-Disposition" error page on Chrome. It's caused by the comma in the filename, when diffset.name != 'diff', so I just need to replace it with an underscore.
~
When you click Download Diff on some code review page, there might be "Multiple Content-Disposition" error page on some versions of Chrome(v16 or newer).
+
+
This iss caused by the comma in the filename from reviewboard/reviews/views.py's diffset, when diffset.name != 'diff' and diffset.name has at least one comma.
+
+
This change replaces the comman with the underscore.

Description:: ~
When you click Download Diff on some code review page, there might be "Multiple Content-Disposition" error page on some versions of Chrome(v16 or newer).
~
If a diff file with a comma in the filename was uploaded via the web UI, clicking "Download Diff" could present an error about "Multiple Content-Disposition" when running with Chrome 16 or newer.

~
This iss caused by the comma in the filename from reviewboard/reviews/views.py's diffset, when diffset.name != 'diff' and diffset.name has at least one comma.
~
The problem is that we use the Content-Disposition header to tell the browser what filename to save as, but commas in this header are interpreted as multiple values.

~
This change replaces the comman with the underscore.
~
To fix this, before we assemble the Content-Disposition header, we replace all commas in the diffset filename with underscores.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.0.x (f52c7c6)

~		change reviewsboard/revies/views.py to be compatible with old version chrome(v16)
	~	change reviewsboard/revies/views.py to be compatible with old version chrome(v16). When you Download Diff on some specific page, there might be "Multiple Content-Disposition" error page on Chrome. It's caused by the comma in the filename, so just need to swap it with an underscore

~		I modify reviewboard/reviews/views.py to be compatible with some versions of Chrome(v16 or newer). When you click `Download Diff` on some code review page, there might be "Multiple Content-Disposition" error page on Chrome. It's caused by the comma in the filename, when `diffset.name != 'diff'`, so I just need to replace it with an underscore.
	~	When you click `Download Diff` on some code review page, there might be "Multiple Content-Disposition" error page on some versions of Chrome(v16 or newer).
	+
	+	This iss caused by the comma in the filename from `reviewboard/reviews/views.py`'s `diffset`, when `diffset.name != 'diff'` and `diffset.name` has at least one comma.
	+
	+	This change replaces the comman with the underscore.