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

Review Request #6786 — Created Jan. 16, 2015 and submitted

Information

Review Board
master
0dad998...

Reviewers

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.

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

Col: 80 E501 line too long (97 > 79 characters)

reviewbotreviewbot

Typo - "Chrom" - "Chrome". Perhaps you should just generalize this and put above this line: # Some older browsers …

mike_conleymike_conley

Col: 73 W291 trailing whitespace

reviewbotreviewbot

Come to think of it, it might be a good idea to refer to the issue # itself here. So …

mike_conleymike_conley

Another type-o - "nmae" -> "name".

mike_conleymike_conley

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

chipx86chipx86

This doesn't seem related to the fix.

chipx86chipx86

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

daviddavid

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

daviddavid

Col: 51 E211 whitespace before '['

reviewbotreviewbot

Col: 29 E127 continuation line over-indented for visual indent

reviewbotreviewbot

This still doesn't test the failure case because there's no comma in the filename! Please comment out your change in …

daviddavid

Col: 51 E211 whitespace before '['

reviewbotreviewbot

Col: 29 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 41 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 51 E211 whitespace before '['

reviewbotreviewbot

Col: 29 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 51 E211 whitespace before '['

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    
  2. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (97 > 79 characters)
    
  3. 
      
CR
  1. A couple of typos in the description in the URL: "reviewsboard" and "revies".

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

    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.

  3. 
      
Chester
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    
  2. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Col: 73
     W291 trailing whitespace
    
  3. 
      
Chester
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    
  2. 
      
TB
  1. Can you link to the google code issue too? I want some background.

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

  2. 
      
Chester
david
  1. You still have typos in your summary/description (reviewsboard, revies)

    1. changed it, thank you!

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

  2. 
      
Chester
Chester
Chester
mike_conley
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 3)
     
     
     

    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."

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

    Another type-o - "nmae" -> "name".

  4. 
      
Chester
reviewbot
  1. 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
    
    
  2. 
      
Chester
chipx86
  1. 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.

  2. 
      
Chester
chipx86
  1. 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.

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

    Blank line between these.

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

    1. This wasn't actually fixed.

    2. just added a blank line before the comment block

  3. This doesn't seem related to the fix.

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

    2. 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.

  4. 
      
Chester
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    
  2. 
      
Chester
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    
  2. 
      
Chester
Chester
david
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 6)
     
     

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

    1. 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!!!

  3. 
      
Chester
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/tests.py (Diff revision 7)
     
     
    Col: 51
     E211 whitespace before '['
    
  3. reviewboard/reviews/tests.py (Diff revision 7)
     
     
    Col: 29
     E127 continuation line over-indented for visual indent
    
  4. 
      
Chester
david
  1. 
      
  2. reviewboard/reviews/tests.py (Diff revision 7)
     
     
     
     
     
     
     

    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.

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

  3. 
      
Chester
chipx86
  1. Still expecting to see the summary changed, as per my prior comments.

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

  2. 
      
Chester
Chester
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/tests.py (Diff revision 9)
     
     
    Col: 51
     E211 whitespace before '['
    
  3. reviewboard/reviews/tests.py (Diff revision 9)
     
     
    Col: 29
     E127 continuation line over-indented for visual indent
    
  4. 
      
Chester
david
  1. 
      
  2. reviewboard/reviews/tests.py (Diff revision 9)
     
     

    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.

    1. 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()?

    2. 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.

    3. 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?

    4. 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?

  3. 
      
Chester
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/tests.py (Diff revision 10)
     
     
    Col: 41
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/reviews/tests.py (Diff revision 10)
     
     
    Col: 51
     E211 whitespace before '['
    
  4. reviewboard/reviews/tests.py (Diff revision 10)
     
     
    Col: 29
     E127 continuation line over-indented for visual indent
    
  5. 
      
Chester
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/tests.py (Diff revision 11)
     
     
    Col: 51
     E211 whitespace before '['
    
    1. I didn't see a white space, anything wrong?

  3. 
      
brennie
  1. 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.)

    1. 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.

    2. 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.

    3. I will be careful, thank you!!!

  2. 
      
Chester
Chester
david
  1. Ship It!
  2. 
      
Chester
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (f52c7c6)
Loading...