Download Review File Attachments as tarball

Review Request #6402 — Created Oct. 5, 2014 and submitted

Information

Review Board
master
2f19217...

Reviewers

Download File Attachments as tarball:
- only working with files stored on host filesystem and Amazon S3.
- now compresses into .tar.gz file instead.
- properly streams compressed file back to client.
- Download Attachments and Download Diff now under Downloads
menu dropdown in review request editor.

- URL changed from: r/<requestID>/file/zip/ -> r/<requestID>/file/tar/

Tested on:
- Linux Mint 17: Chromium 37.0.2062.120
- Linux Mint 17: Firefox 32.0.3
- Windows 8.1: Chrome 38.0.2125.101
- Windows 8.1: Firefox 32.0.3
- Windows 8.1: Internet Explorer 11
Tested also with curl to see if file is streaming as expected.

Review requests with small files, large files (2mb+), and mix of both, tested and working with host file system and Amazon S3 storage option.

Description From Last Updated

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

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

local variable 'zip_path' is assigned to but never used

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

Col: 19 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

Col: 36 E703 statement ends with a semicolon

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

local variable 'zip_filename' is assigned to but never used

reviewbotreviewbot

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

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 621

reviewbotreviewbot

Col: 49 W291 trailing whitespace

reviewbotreviewbot

Col: 27 W291 trailing whitespace

reviewbotreviewbot

Col: 38 W291 trailing whitespace

reviewbotreviewbot

Col: 9 E113 unexpected indentation

reviewbotreviewbot

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

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 618

reviewbotreviewbot

There should be a single blank line before a block

brenniebrennie

list comprehension redefines 'file_attachment' from line 619

reviewbotreviewbot

This is in the diff in https://reviews.reviewboard.org/r/6432/ You should mark this RR as depends on 6432 and base your RR …

brenniebrennie

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 6 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 6 E113 unexpected indentation

reviewbotreviewbot

Col: 6 E265 block comment should start with '# '

reviewbotreviewbot

Col: 6 E303 too many blank lines (2)

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 629

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

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

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 634

reviewbotreviewbot

Col: 40 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 56 E502 the backslash is redundant between brackets

reviewbotreviewbot

Col: 72 E502 the backslash is redundant between brackets

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 635

reviewbotreviewbot

Col: 41 E502 the backslash is redundant between brackets

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 636

reviewbotreviewbot

undefined name 'eview_request_id'

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 636

reviewbotreviewbot

As mentioned earlier, can you pull the latest master and rebase?

daviddavid

One too many blank lines here.

daviddavid

You should use the 'six' version of StringIO for python 3.x compat: from django.utils.six.moves import cStringIO as StringIO ... s …

daviddavid

Revert this change.

daviddavid

Since you only iterate over file_paths once, how about combining these? Then your loop belowe would look something like: for …

daviddavid

Instead of hard-coding './reviewboard/htdocs' (which won't work in a deployed server), it should use siteconfig.get('site_media_root')

daviddavid

You should use os.path.join to create file paths.

brenniebrennie

You're leaking the file here. This should be: with open(file_path, 'r') as f: file_contents = f.read()

daviddavid

It's very weird to mix formatting styles here, plus you're doing two format operations where you only need one. How …

daviddavid

Suggestion: why not bool(file_attachments)? Empty lists will evaluate to false and so will None.

brenniebrennie

list comprehension redefines 'file_attachment' from line 636

reviewbotreviewbot

Can we make the URL /file/zip/ ? That way it fits in with the existing /file/N/ URLs.

daviddavid

Is this necessary? Anything on PyPI should automatically be included...

daviddavid

Why not wrap it in () so that you don't need the \\, i.e. response['Content-Disposition'] = ( 'attachment; filename=review-attachments-%s' % …

brenniebrennie

Shouldn't this have a .zip extension?

brenniebrennie

Docstrings should always be in this format: """Single-line summary.""" or: """Single-line summary. Multi-line description. """ with no blank line after. …

chipx86chipx86

I'd just put .all() at the end of this one. No point doing it below.

chipx86chipx86

Blank line between these.

chipx86chipx86

Blank line between these.

chipx86chipx86

The point of the storage backends is that you should never have to do logic like this. Plus, there is …

chipx86chipx86

As above, we're not actually taking advantage of the streaming here. What you're doing is reading everything into memory and …

chipx86chipx86

"review-attachments" is a misleading prefix. This is for attachments on the review request, not reviews. However, we don't want the …

chipx86chipx86

We should probably have both of these within a "Download" menu, like we have with "Update."

chipx86chipx86

I know the other links are hard-coded, but we should be using {% url download-zip-attachments review_request.display_id %} here.

chipx86chipx86

Should have a comma at end of line. That way future dependancies will only be a 1-line change.

brenniebrennie

local variable 'total_size' is assigned to but never used

reviewbotreviewbot

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

reviewbotreviewbot

If you're only compressing as tar.gz you won't need zipstream.

brenniebrennie

'os' imported but unused

reviewbotreviewbot

local variable 'siteconfig' is assigned to but never used

reviewbotreviewbot

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

reviewbotreviewbot

This should go under review_request_urls.

chipx86chipx86

No spaces before/after the """. This should read more like a sentence. In fact, it should really be fleshed out, …

chipx86chipx86

I know we have some old docstrings in here, but this should be of the following format: """One-line summary (< …

chipx86chipx86

This and all the other functions should go in a utility object that wraps tarfile.TarFile. This view should be able …

chipx86chipx86

Docstrings shouldn't have a space after the first triple quote, e.g. """Build TarInfo...""" Same below

brenniebrennie

We should just let a link be a link. I think, given other issues reported during today's meeting, that links …

chipx86chipx86

These don't look like they were meant to be in this change?

chipx86chipx86

undefined name 'time'

reviewbotreviewbot

This must be added alphabetically to the list of imports (after reviewboard.attachments.models.)

chipx86chipx86

How about "Downloads all file attachments on a review request as a tarball." ?

chipx86chipx86

"... are downloaded as a tarball." Not sure we need to talk about what isn't supported.

chipx86chipx86

Can you put the arguments on the first line? If they need to wrap, do: response[...] = ('...' % out_filename)

chipx86chipx86

Not sure we need this. The template can just check the file attachments list. I think we just want to …

chipx86chipx86

I wasn't clear before, but what I meant in the previous review is that the templates can just check file_attachments, …

chipx86chipx86

No space after """.

chipx86chipx86

Too many backticks. Generally, though, we don't add this sort of doc to the docstrings. This should be more higher-level, …

chipx86chipx86

No need to say all the things being outputted. Just say what the function does at a high level, and …

chipx86chipx86

These should be class-level constants.

chipx86chipx86

Will these ever be changed between instances? I can understand that it might be beneficial to keep the BLOCK_SIZE as …

ML mloyzer

This should be written more like how you'd say it to someone. "Build a TarInfo object representing one file in …

chipx86chipx86

This should have a try/except for each thing, since a storage backend may implement one but not the other. It …

chipx86chipx86

We should yield before we close.

chipx86chipx86

Same comment as above. This shouldn't know how it's going to be used (the StreamingHttpResponse part). Just go into the …

chipx86chipx86

Blank line between these.

chipx86chipx86

yield isn't a function, so you shouldn't use parens.

chipx86chipx86

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

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/urls.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (100 > 79 characters)
    
  3. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
     local variable 'zip_path' is assigned to but never used
    
  5. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 9
     E265 block comment should start with '# '
    
  6. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 19
     E702 multiple statements on one line (semicolon)
    
  7. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 36
     E703 statement ends with a semicolon
    
  8. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 9
     E265 block comment should start with '# '
    
  9. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
     local variable 'zip_filename' is assigned to but never used
    
  10. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  11. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
     list comprehension redefines 'file_attachment' from line 621
    
  12. 
      
AD
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/urls.py (Diff revision 2)
     
     
    Show all issues
    Col: 49
     W291 trailing whitespace
    
  3. reviewboard/reviews/urls.py (Diff revision 2)
     
     
    Show all issues
    Col: 27
     W291 trailing whitespace
    
  4. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Show all issues
    Col: 38
     W291 trailing whitespace
    
  5. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Show all issues
    Col: 9
     E113 unexpected indentation
    
  6. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
     list comprehension redefines 'file_attachment' from line 618
    
  4. 
      
AD
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
     list comprehension redefines 'file_attachment' from line 619
    
  3. 
      
ML
  1. What happens when there is one file attachment (so the 'Download File Attachments as ZIP' button is there) then delete that file? Is the button removed/disabled? I'm trying to see if there is a way to execute the code in zip_attachments on a review request without any file attachments.

    1. When you delete and haven't published it, then there is a blank ZIP file download. I'm looking into preventing that from happening.
      The button is removed when that change is published or when there are no file attachments present though.

  2. 
      
brennie
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 4)
     
     
     
    Show all issues

    There should be a single blank line before a block

  3. Other than that, it looks good!

AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/admin/forms.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/admin/forms.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  3. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Show all issues
    Col: 6
     E111 indentation is not a multiple of four
    
  4. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Show all issues
    Col: 6
     E113 unexpected indentation
    
  5. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Show all issues
    Col: 6
     E265 block comment should start with '# '
    
  6. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Show all issues
    Col: 6
     E303 too many blank lines (2)
    
  7. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Show all issues
    Col: 9
     E265 block comment should start with '# '
    
  8. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Show all issues
     list comprehension redefines 'file_attachment' from line 629
    
  9. 
      
mike_conley
  1. 
      
  2. reviewboard/reviews/views.py (Diff revisions 4 - 5)
     
     

    Out of curiosity, why the round-trip through the StringIO?

    1. I was playing around with different implementations to see if it makes any difference.
      There was a function, I forget which, that took a file-like object as a parameter instead of bytes.

  3. 
      
brennie
  1. 
      
  2. reviewboard/admin/forms.py (Diff revision 5)
     
     
     
     
    Show all issues

    This is in the diff in https://reviews.reviewboard.org/r/6432/

    You should mark this RR as depends on 6432 and base your RR off of that diff via:

    rbt diff dev/fix-storage-saving..dev/zip-attachments

    but using the correct branch names.

    1. Would editting it in the review request page achieve the same thing?

    2. You should rebase your development branch on an updated master since 6432 has been submitted. Then you can re-run rbt post and the change should not appear. Alternatively, you can run rbt post -X reviewboard/admin/forms.py

  3. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/admin/forms.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/admin/forms.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  3. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Show all issues
    Col: 21
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Show all issues
    Col: 21
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  6. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (87 > 79 characters)
    
  7. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Show all issues
     list comprehension redefines 'file_attachment' from line 634
    
  8. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/admin/forms.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/admin/forms.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
    Col: 40
     W291 trailing whitespace
    
  3. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
    Col: 9
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
    Col: 56
     E502 the backslash is redundant between brackets
    
  5. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
    Col: 72
     E502 the backslash is redundant between brackets
    
  6. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
    Col: 21
     E128 continuation line under-indented for visual indent
    
  7. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
    Col: 21
     E128 continuation line under-indented for visual indent
    
  8. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
    Col: 13
     E128 continuation line under-indented for visual indent
    
  9. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
     list comprehension redefines 'file_attachment' from line 635
    
  10. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/admin/forms.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/admin/forms.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Show all issues
    Col: 41
     E502 the backslash is redundant between brackets
    
  3. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Show all issues
    Col: 9
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Show all issues
    Col: 13
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Show all issues
     list comprehension redefines 'file_attachment' from line 636
    
  6. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/admin/forms.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/admin/forms.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 9)
     
     
    Show all issues
     undefined name 'eview_request_id'
    
  3. reviewboard/reviews/views.py (Diff revision 9)
     
     
    Show all issues
     list comprehension redefines 'file_attachment' from line 636
    
  4. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/admin/forms.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/admin/forms.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 10)
     
     
    Show all issues
     list comprehension redefines 'file_attachment' from line 636
    
  3. 
      
AD
brennie
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 10)
     
     
    Show all issues

    You should use os.path.join to create file paths.

  3. reviewboard/reviews/views.py (Diff revision 10)
     
     
    Show all issues

    Suggestion: why not bool(file_attachments)?

    Empty lists will evaluate to false and so will None.

  4. 
      
david
  1. 
      
  2. reviewboard/admin/forms.py (Diff revision 10)
     
     
     
    Show all issues

    As mentioned earlier, can you pull the latest master and rebase?

  3. reviewboard/reviews/views.py (Diff revision 10)
     
     
    Show all issues

    One too many blank lines here.

  4. reviewboard/reviews/views.py (Diff revision 10)
     
     
    Show all issues

    You should use the 'six' version of StringIO for python 3.x compat:

    from django.utils.six.moves import cStringIO as StringIO
    
    ...
    
    s = StringIO()
    
  5. reviewboard/reviews/views.py (Diff revision 10)
     
     
    Show all issues

    Revert this change.

  6. reviewboard/reviews/views.py (Diff revision 10)
     
     
    Show all issues

    Since you only iterate over file_paths once, how about combining these? Then your loop belowe would look something like:

    for fa in file_attachments.all():
        file_dir, file_name = os.path.split(fa.file.url)
        ...
    
  7. reviewboard/reviews/views.py (Diff revision 10)
     
     
    Show all issues

    Instead of hard-coding './reviewboard/htdocs' (which won't work in a deployed server), it should use siteconfig.get('site_media_root')

  8. reviewboard/reviews/views.py (Diff revision 10)
     
     
    Show all issues

    You're leaking the file here. This should be:

    with open(file_path, 'r') as f:
        file_contents = f.read()
    
  9. reviewboard/reviews/views.py (Diff revision 10)
     
     
     
    Show all issues

    It's very weird to mix formatting styles here, plus you're doing two format operations where you only need one. How about just:

    response['Content-Disposition'] = (
        'attachment; filename=review-attachments-%s'
        % review_request_id)
    
  10. Show all issues

    Can we make the URL /file/zip/ ? That way it fits in with the existing /file/N/ URLs.

  11. setup.py (Diff revision 10)
     
     
    Show all issues

    Is this necessary? Anything on PyPI should automatically be included...

  12. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 11)
     
     
     
    Show all issues

    Why not wrap it in () so that you don't need the \\, i.e.

    response['Content-Disposition'] = (
        'attachment; filename=review-attachments-%s' % review_request_id)
    
    1. EDIT: removed -- wrong comment.

  3. reviewboard/reviews/views.py (Diff revision 11)
     
     
    Show all issues

    Shouldn't this have a .zip extension?

    1. Good catch! Thanks for pointing that out. The browser automatically knew it was a zip so I assumed it was all good there.

  4. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
    
    
  2. 
      
brennie
  1. 
      
  2. setup.py (Diff revision 12)
     
     
    Show all issues

    Should have a comma at end of line. That way future dependancies will only be a 1-line change.

  3. 
      
dkus
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 12)
     
     

    I am not too familiar with how python handles file IO, but shouldn't the file be read / written in chunks? What happens if the file is really big (many gbs), will this put all of it into memory?

  3. 
      
chipx86
  1. I have some comments about the design below. The summary is that this code doesn't actually stream zip files. Try attaching a large file (hundreds of megs or more), add some printing, open the browser's Network tab, open a process monitor to look at the dev server's memory usage, and then download. See what happens.

    I know we discussed using zipstream before, but it looks like we don't actually need to add yet another dependency. Check this out and see if this solution works (once all the notes are addressed and confirmed to work): http://stackoverflow.com/a/24173992

    1. Okay, I've looked into this further, and now I understand better. There's no way to chunk-write to a ZipFile instance without rewriting part of ZipFile itself.

      Given that, ZipStream does do what we want, and does it how we'd do it, so that's worth using.

  2. reviewboard/reviews/views.py (Diff revision 12)
     
     
     
     
     
     
    Show all issues

    Docstrings should always be in this format:

    """Single-line summary."""
    

    or:

    """Single-line summary.
    
    Multi-line description.
    """
    

    with no blank line after.

    The total line length must not exceed 79 characters.

  3. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Show all issues

    I'd just put .all() at the end of this one. No point doing it below.

  4. reviewboard/reviews/views.py (Diff revision 12)
     
     
     
    Show all issues

    Blank line between these.

  5. reviewboard/reviews/views.py (Diff revision 12)
     
     
     
    Show all issues

    Blank line between these.

  6. reviewboard/reviews/views.py (Diff revision 12)
     
     
     
     
     
     
     
     
     
    Show all issues

    The point of the storage backends is that you should never have to do logic like this. Plus, there is absolutely no guarantee that the path from the storage backend will be a URL that you can even directly read from. Even on S3, there may be an access token required to get the URL, breaking this urlopen.

    You should be able to do fa.file.open and read from that directly. The storage backend will handle the opening and reading.

    You also should not be reading the entire file in one go. The file could be 500MB in size, and you'll end up destroying the Apache process by reading it all in one go. Instead, you need to use the chunks() function, writing to the HTTP stream as you go.

    Look at the File documentation: https://docs.djangoproject.com/en/dev/ref/files/file/#django.core.files.File

  7. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Show all issues

    As above, we're not actually taking advantage of the streaming here. What you're doing is reading everything into memory and then writing it all at once, which will blow up Apache and time out the request.

    Read through the docs on StreamingHttpResponse: https://docs.djangoproject.com/en/1.6/ref/request-response/#streaminghttpresponse-objects

    Note the first bullet point. My understanding is that you just want to pass zf to StreamingHTTPResponse and then begin writing the chunks to it, instead of to a StringIO.

  8. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Show all issues

    "review-attachments" is a misleading prefix. This is for attachments on the review request, not reviews. However, we don't want the filename to be too long.

    How about just r%d-attachments.zip?

  9. reviewboard/templates/reviews/review_detail.html (Diff revision 12)
     
     
     
     
     
     
     
    Show all issues

    We should probably have both of these within a "Download" menu, like we have with "Update."

  10. Show all issues

    I know the other links are hard-coded, but we should be using {% url download-zip-attachments review_request.display_id %} here.

  11. 
      
AD
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. reviewboard/reviews/views.py (Diff revision 13)
     
     
    Show all issues
     local variable 'total_size' is assigned to but never used
    
  3. reviewboard/reviews/views.py (Diff revision 13)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. 
      
AD
brennie
  1. 
      
  2. setup.py (Diff revision 13)
     
     
    Show all issues

    If you're only compressing as tar.gz you won't need zipstream.

  3. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. reviewboard/reviews/views.py (Diff revision 14)
     
     
    Show all issues
     'os' imported but unused
    
  3. reviewboard/reviews/views.py (Diff revision 14)
     
     
    Show all issues
     local variable 'siteconfig' is assigned to but never used
    
  4. reviewboard/reviews/views.py (Diff revision 14)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  5. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        setup.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/urls.py (Diff revision 15)
     
     
     
     
    Show all issues

    This should go under review_request_urls.

  3. reviewboard/reviews/views.py (Diff revision 15)
     
     
    Show all issues

    No spaces before/after the """.

    This should read more like a sentence. In fact, it should really be fleshed out, to full human-readable documentation, covering what this is, what it does, how, and why.

  4. reviewboard/reviews/views.py (Diff revision 15)
     
     
     
     
     
    Show all issues

    I know we have some old docstrings in here, but this should be of the following format:

    """One-line summary (< 79 chars in line length).
    
    Multi-line description.
    """
    
  5. reviewboard/reviews/views.py (Diff revision 15)
     
     
    Show all issues

    This and all the other functions should go in a utility object that wraps tarfile.TarFile. This view should be able to very simply instantiate a thing and return the result.

    I'd recommend that this go in reviewboard/attachments/streaming_tarfile.py.

  6. Show all issues

    We should just let a link be a link.

    I think, given other issues reported during today's meeting, that links on the navbar are failing to just work, because we're blocking them. We should fix that instead of work around it here, since this only solves the one case.

  7. setup.py (Diff revision 15)
     
     
     
     
    Show all issues

    These don't look like they were meant to be in this change?

  8. 
      
brennie
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 15)
     
     
    Show all issues

    Docstrings shouldn't have a space after the first triple quote, e.g.

    """Build TarInfo..."""
    

    Same below

  3. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. reviewboard/reviews/views.py (Diff revision 16)
     
     
    Show all issues
     undefined name 'time'
    
  3. 
      
AD
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 17)
     
     
    Show all issues

    This must be added alphabetically to the list of imports (after reviewboard.attachments.models.)

  3. reviewboard/reviews/views.py (Diff revision 17)
     
     
    Show all issues

    How about "Downloads all file attachments on a review request as a tarball." ?

  4. reviewboard/reviews/views.py (Diff revision 17)
     
     
    Show all issues

    "... are downloaded as a tarball."

    Not sure we need to talk about what isn't supported.

  5. reviewboard/reviews/views.py (Diff revision 17)
     
     
     
    Show all issues

    Can you put the arguments on the first line? If they need to wrap, do:

    response[...] = ('...'
                     % out_filename)
    
  6. reviewboard/reviews/views.py (Diff revision 17)
     
     
    Show all issues

    Not sure we need this. The template can just check the file attachments list.

    I think we just want to check latest_file_attachments though, not file_attachments. The latter contains discarded ones as well.

  7. 
      
chipx86
  1. Looks like this is missing the implementation of the streaming tar class.

  2. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/attachments/streaming_tarfile.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/attachments/streaming_tarfile.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. reviewboard/reviews/views.py (Diff revision 18)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/views.py (Diff revisions 17 - 18)
     
     
    Show all issues

    I wasn't clear before, but what I meant in the previous review is that the templates can just check file_attachments, since that will evaluate to a boolean. There's no need to have a separate boolean.

    We have this for has_diffs because it's a much more complex expression not easily represented in templates.

  3. Show all issues

    No space after """.

  4. Show all issues

    Too many backticks.

    Generally, though, we don't add this sort of doc to the docstrings. This should be more higher-level, as in what the class does, as if you were explaining to someone who hasn't seen the source code but just wants a conceptual overview.

    You don't need to document the arguments.

    Actually, given that this is for streaming, why do we even care about the out filename? Is that even used for anything at all? I thought we weren't writing to disk.

  5. Show all issues

    No need to say all the things being outputted. Just say what the function does at a high level, and what the parameters are like. We don't need to go into the internal details.

  6. reviewboard/attachments/streaming_tarfile.py (Diff revision 18)
     
     
     
    Show all issues

    These should be class-level constants.

  7. Show all issues

    This should be written more like how you'd say it to someone. "Build a TarInfo object representing one file in a tarball."

  8. reviewboard/attachments/streaming_tarfile.py (Diff revision 18)
     
     
     
     
     
     
    Show all issues

    This should have a try/except for each thing, since a storage backend may implement one but not the other.

    It should also explicitly check for a NotImplementedError. We want to know about other exception types.

    No blank line before the except.

  9. reviewboard/attachments/streaming_tarfile.py (Diff revision 18)
     
     
     
    Show all issues

    We should yield before we close.

  10. Show all issues

    Same comment as above.

    This shouldn't know how it's going to be used (the StreamingHttpResponse part). Just go into the higher-level bits, like:

    """Generates the tarball containing the file, in chunks.
    
    This will build the contents of the tarball, yielding chunks
    as they're generated.
    """
    
  11. reviewboard/attachments/streaming_tarfile.py (Diff revision 18)
     
     
     
    Show all issues

    Blank line between these.

  12. Show all issues

    yield isn't a function, so you shouldn't use parens.

  13. 
      
ML
  1. 
      
  2. reviewboard/attachments/streaming_tarfile.py (Diff revision 18)
     
     
     
    Show all issues

    Will these ever be changed between instances? I can understand that it might be beneficial to keep the BLOCK_SIZE as an instance variable but would it make sense to put MODE as a class variable?

    I guess part of the reason is because they are assigned constants, can't be given inputs in the init function and there are no setters for them either.

    Secondly, can you maybe describe what MODE = 0644 means?

  3. 
      
AD
Review request changed
Status:
Completed
Change Summary:
Pushed to ucosp/adrw.hong/download-attachments (b133167)