File attachment hotlinking view

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

rvaiya
Review Board
master
6220
reviewboard

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)
     
     
     'HttpResponse' imported but unused
    
  3. reviewboard/attachments/views.py (Diff revision 1)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. reviewboard/attachments/views.py (Diff revision 1)
     
     
    Col: 70
     W291 trailing whitespace
    
  5. reviewboard/urls.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  6. 
      
chipx86
  1. 
      
  2. reviewboard/attachments/views.py (Diff revision 1)
     
     
     
     

    Should be one import line.

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

    Two blank lines here.

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

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

    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)
     
     
     
     
     
     
     
     

    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)
     
     

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

  8. reviewboard/urls.py (Diff revision 1)
     
     

    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)
     
     
     'HttpResponseNotFound' imported but unused
    
  3. reviewboard/attachments/views.py (Diff revision 2)
     
     
     'ObjectDoesNotExist' imported but unused
    
  4. reviewboard/attachments/views.py (Diff revision 2)
     
     
    Col: 50
     W291 trailing whitespace
    
  5. reviewboard/attachments/views.py (Diff revision 2)
     
     
    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)
     
     

    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)
     
     

    Remove this blank line.

  4. reviewboard/urls.py (Diff revision 3)
     
     
     

    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)
     
     

    Missing a trailing period.

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

    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

Loading...