File attachment hotlinking view
Review Request #6217 — Created Aug. 12, 2014 and discarded
Information | |
---|---|
rvaiya | |
Review Board | |
master | |
6220 | |
Reviewers | |
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. |
|
|
'HttpResponse' imported but unused |
![]() |
|
Two blank lines here. |
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
![]() |
|
"file_download" is what I'd prefer. Can you also add a docstring explaining what this does? Make sure it's in the … |
|
|
That should just go in the function arguments. No need to pull it out of kwargs. In fact, you shouldn't … |
|
|
The 404 would be publicly-visible, so if we were showing a message, we'd want to localize, make sure it's in … |
|
|
Use pk= instead of id= when referencing primary keys. |
|
|
Col: 70 W291 trailing whitespace |
![]() |
|
Col: 80 E501 line too long (86 > 79 characters) |
![]() |
|
Should be "files/". Can you also use "file_attachment_id", so it's explicit? |
|
|
'HttpResponseNotFound' imported but unused |
![]() |
|
'ObjectDoesNotExist' imported but unused |
![]() |
|
Col: 50 W291 trailing whitespace |
![]() |
|
Col: 56 W291 trailing whitespace |
![]() |
|
Missing a trailing period. |
|
|
I'd prefer not including the particular URL in here, because if we decide to move it, it's likely that this … |
|
|
Remove this blank line. |
|
|
Should probably use file_attachment.get_absolute_url(). |
|
|
The second field in the tuple should be aligned with the first: (r'^file...', 'reviewboard.attachments...') |
|
-
-
-
-
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. """
-
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):
-
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, raisingHttp404
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)
-
reviewboard/attachments/views.py (Diff revision 1) Use
pk=
instead ofid=
when referencing primary keys. -
reviewboard/urls.py (Diff revision 1) Should be "files/".
Can you also use "file_attachment_id", so it's explicit?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+25) |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/attachments/views.py reviewboard/urls.py Tool: Pyflakes Processed Files: reviewboard/attachments/views.py reviewboard/urls.py
-
-
-
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+23) |

-
Tool: Pyflakes Processed Files: reviewboard/attachments/views.py reviewboard/urls.py Tool: PEP8 Style Checker Processed Files: reviewboard/attachments/views.py reviewboard/urls.py
-
-
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.
-
-
reviewboard/urls.py (Diff revision 3) The second field in the tuple should be aligned with the first:
(r'^file...', 'reviewboard.attachments...')
-
-
-
reviewboard/attachments/views.py (Diff revision 3) Should probably use
file_attachment.get_absolute_url()
.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+22) |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/attachments/views.py reviewboard/urls.py Tool: Pyflakes Processed Files: reviewboard/attachments/views.py reviewboard/urls.py
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+22) |