File attachment hotlinking view

Review Request #6217 — Created Aug. 12, 2014 and discarded

Information

Review Board
master

Reviewers

A view for file hotlinking based on a FileAttachment id. If the FileAttachment in question has a null file field, a 404 is returned.

  • Manually requested existent and non-existent file attachments and observed expected behaviour.
Description From Last Updated

Should be one import line.

chipx86chipx86

'HttpResponse' imported but unused

reviewbotreviewbot

Two blank lines here.

chipx86chipx86

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

"file_download" is what I'd prefer. Can you also add a docstring explaining what this does? Make sure it's in the …

chipx86chipx86

That should just go in the function arguments. No need to pull it out of kwargs. In fact, you shouldn't …

chipx86chipx86

The 404 would be publicly-visible, so if we were showing a message, we'd want to localize, make sure it's in …

chipx86chipx86

Use pk= instead of id= when referencing primary keys.

chipx86chipx86

Col: 70 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Should be "files/". Can you also use "file_attachment_id", so it's explicit?

chipx86chipx86

'HttpResponseNotFound' imported but unused

reviewbotreviewbot

'ObjectDoesNotExist' imported but unused

reviewbotreviewbot

Col: 50 W291 trailing whitespace

reviewbotreviewbot

Col: 56 W291 trailing whitespace

reviewbotreviewbot

Missing a trailing period.

chipx86chipx86

I'd prefer not including the particular URL in here, because if we decide to move it, it's likely that this …

daviddavid

Remove this blank line.

daviddavid

Should probably use file_attachment.get_absolute_url().

chipx86chipx86

The second field in the tuple should be aligned with the first: (r'^file...', 'reviewboard.attachments...')

daviddavid
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/attachments/views.py
        reviewboard/urls.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/attachments/views.py
        reviewboard/urls.py
    
    
  2. reviewboard/attachments/views.py (Diff revision 1)
     
     
    Show all issues
     'HttpResponse' imported but unused
    
  3. reviewboard/attachments/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. reviewboard/attachments/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 70
     W291 trailing whitespace
    
  5. reviewboard/urls.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  6. 
      
chipx86
  1. 
      
  2. reviewboard/attachments/views.py (Diff revision 1)
     
     
     
     
    Show all issues

    Should be one import line.

  3. reviewboard/attachments/views.py (Diff revision 1)
     
     
     
     
    Show all issues

    Two blank lines here.

  4. reviewboard/attachments/views.py (Diff revision 1)
     
     
    Show all issues

    "file_download" is what I'd prefer.

    Can you also add a docstring explaining what this does? Make sure it's in the following form:

    """One-line summary.
    
    Multi-line description.
    """
    
  5. reviewboard/attachments/views.py (Diff revision 1)
     
     
    Show all issues

    That should just go in the function arguments. No need to pull it out of kwargs.

    In fact, you shouldn't have a kwargs here.

    From the above changes, this would become:

    def file_download(request, file_attachment_id):
    
  6. reviewboard/attachments/views.py (Diff revision 1)
     
     
     
     
     
     
     
     
    Show all issues

    The 404 would be publicly-visible, so if we were showing a message, we'd want to localize, make sure it's in sentence-casing, and be a bit more friendly with the error text.

    Having custom text means that the default Review Board error page wouldn't be shown. So all around, we don't want to provide any of this. In fact, we don't want to return HttpResponseNotFound. Instead, raising Http404 will do the right thing.

    There's a shortcut for all this. It will query, and if not found, raise Http404.

    We can even simplify this further to ensure that we can only look up file attachments with file content. So, change all this to:

    file_attachment = get_object_or_404(FileAttachment,
                                        pk=file_attachment_id,
                                        file__isnull=FalsE)
    
  7. reviewboard/attachments/views.py (Diff revision 1)
     
     
    Show all issues

    Use pk= instead of id= when referencing primary keys.

  8. reviewboard/urls.py (Diff revision 1)
     
     
    Show all issues

    Should be "files/".

    Can you also use "file_attachment_id", so it's explicit?

  9. 
      
RV
RV
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/attachments/views.py
        reviewboard/urls.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/attachments/views.py
        reviewboard/urls.py
    
    
  2. reviewboard/attachments/views.py (Diff revision 2)
     
     
    Show all issues
     'HttpResponseNotFound' imported but unused
    
  3. reviewboard/attachments/views.py (Diff revision 2)
     
     
    Show all issues
     'ObjectDoesNotExist' imported but unused
    
  4. reviewboard/attachments/views.py (Diff revision 2)
     
     
    Show all issues
    Col: 50
     W291 trailing whitespace
    
  5. reviewboard/attachments/views.py (Diff revision 2)
     
     
    Show all issues
    Col: 56
     W291 trailing whitespace
    
  6. 
      
RV
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/attachments/views.py
        reviewboard/urls.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/attachments/views.py
        reviewboard/urls.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/attachments/views.py (Diff revision 3)
     
     
    Show all issues

    I'd prefer not including the particular URL in here, because if we decide to move it, it's likely that this would get out of date.

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

    Remove this blank line.

  4. reviewboard/urls.py (Diff revision 3)
     
     
     
    Show all issues

    The second field in the tuple should be aligned with the first:

    (r'^file...',
     'reviewboard.attachments...')
    
  5. 
      
chipx86
  1. 
      
  2. reviewboard/attachments/views.py (Diff revision 3)
     
     
    Show all issues

    Missing a trailing period.

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

    Should probably use file_attachment.get_absolute_url().

  4. 
      
RV
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/attachments/views.py
        reviewboard/urls.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/attachments/views.py
        reviewboard/urls.py
    
    
  2. 
      
RV
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/attachments/views.py
        reviewboard/urls.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/attachments/views.py
        reviewboard/urls.py
    
    
  2. 
      
RV
Review request changed
Status:
Discarded