New resource for "owned" and review request independent file attachments
Review Request #6220 — Created Aug. 12, 2014 and discarded
Information | |
---|---|
rvaiya | |
Review Board | |
master | |
|
|
Reviewers | |
reviewboard, students | |
New resource for user file attachments.
New file attachment resource was creating using the frontend backbone/RB.BaseResource model and post/put/get were tested manually.
- Successfully created a file attachment user content is always empty upon creation as expected
- Successfully posted caption and file data
- Successfully modified and saved caption data
- Tried to update file a second time and observed the expected behaviour (it failed)
- Manually tested get_list by making a request to /api/file-attachments/, it returned a list of all file attachments owned by the user making the request as expected.
Description | From | Last Updated |
---|---|---|
'reviewboard' imported but unused |
![]() |
|
Col: 13 E241 multiple spaces after ':' |
![]() |
|
Col: 22 E241 multiple spaces after ':' |
![]() |
|
Col: 22 E241 multiple spaces after ':' |
![]() |
|
Col: 17 E241 multiple spaces after ':' |
![]() |
|
Col: 26 E241 multiple spaces after ':' |
![]() |
|
Col: 28 E241 multiple spaces after ':' |
![]() |
|
Col: 22 E241 multiple spaces after ':' |
![]() |
|
Col: 22 E241 multiple spaces after ':' |
![]() |
|
Col: 17 E241 multiple spaces after ':' |
![]() |
|
Col: 26 E241 multiple spaces after ':' |
![]() |
|
Col: 28 E241 multiple spaces after ':' |
![]() |
|
local variable 'positions_key' is assigned to but never used |
![]() |
|
redefinition of unused 'FileDiffMigrationTests' from line 726 |
![]() |
|
list comprehension redefines 'file_attachment' from line 583 |
![]() |
|
We generally avoid the ternary form because it's annoying. How about rewriting this as: if self.orig_filename: return self.orig_filename elif self.file: … |
|
|
Why did you make this change? |
|
|
Trailing spaces in javascript can break IE. |
|
|
Remove this line. |
|
|
This should probably include everything if it's an admin making the request. |
|
|
Docstrings should generally follow the same format that you might use for a commit message -- the first line should … |
|
|
Same comments as the above docstring. |
|
|
What happens if you just let it assign None to the caption field? |
|
|
PERMISSION_DENIED doesn't seem quite right. How about creating a new error, FILE_ALREADY_EXISTS? |
|
|
'User' imported but unused |
![]() |
|
'User' imported but unused |
![]() |
Testing Done: |
|
---|
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+22) |

-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/file_attachment_user.py reviewboard/attachments/models.py reviewboard/attachments/forms.py reviewboard/webapi/resources/root.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/file_attachment_user.py reviewboard/attachments/models.py reviewboard/attachments/forms.py reviewboard/webapi/resources/root.py
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+126 -2) |

-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/file_attachment_user.py reviewboard/attachments/models.py reviewboard/attachments/forms.py reviewboard/webapi/resources/root.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/file_attachment_user.py reviewboard/attachments/models.py reviewboard/attachments/forms.py reviewboard/webapi/resources/root.py
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+126 -2) |

-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/file_attachment_user.py reviewboard/attachments/models.py reviewboard/attachments/forms.py reviewboard/webapi/resources/root.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/file_attachment_user.py reviewboard/attachments/models.py reviewboard/attachments/forms.py reviewboard/webapi/resources/root.py
Description: |
|
||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||
Diff: |
Revision 6 (+407 -25) |

-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/root.py reviewboard/diffviewer/management/commands/condensediffs.py reviewboard/attachments/models.py reviewboard/diffviewer/managers.py reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/webapi/resources/review_request.py reviewboard/webapi/tests/test_review_request.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/staticbundles.py reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/scmtools/svn/subvertpy.py Ignored Files: reviewboard/static/rb/js/configForms/models/tests/resourceListItemModelTests.js reviewboard/static/rb/css/ui/boxes.less reviewboard/static/rb/js/resources/models/apiTokenModel.js reviewboard/static/rb/js/resources/models/fileAttachmentUserModel.js reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/root.py reviewboard/diffviewer/management/commands/condensediffs.py reviewboard/attachments/models.py reviewboard/diffviewer/managers.py reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/webapi/resources/review_request.py reviewboard/webapi/tests/test_review_request.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/staticbundles.py reviewboard/webapi/resources/review_request_draft.py reviewboard/webapi/tests/test_review_request_draft.py reviewboard/scmtools/svn/subvertpy.py Ignored Files: reviewboard/static/rb/js/configForms/models/tests/resourceListItemModelTests.js reviewboard/static/rb/css/ui/boxes.less reviewboard/static/rb/js/resources/models/apiTokenModel.js reviewboard/static/rb/js/resources/models/fileAttachmentUserModel.js reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+1382 -216) |

-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/admin/views.py reviewboard/diffviewer/renderers.py reviewboard/webapi/resources/root.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/evolutions/__init__.py reviewboard/webapi/resources/diff.py reviewboard/staticbundles.py reviewboard/diffviewer/evolutions/raw_diff_file_data.py reviewboard/admin/forms.py reviewboard/attachments/models.py reviewboard/webapi/resources/base_original_file.py setup.py reviewboard/diffviewer/managers.py reviewboard/admin/siteconfig.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/admin/checks.py reviewboard/diffviewer/management/commands/condensediffs.py reviewboard/webapi/resources/base_patched_file.py reviewboard/scmtools/hg.py reviewboard/__init__.py reviewboard/webapi/resources/filediff.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/models.py Ignored Files: reviewboard/static/rb/js/resources/models/fileAttachmentUserModel.js reviewboard/static/rb/js/resources/models/baseResourceModel.js docs/releasenotes/index.rst docs/manual/admin/installation/linux.rst docs/manual/admin/configuration/file-storage-settings.rst docs/releasenotes/2.0.6.rst AUTHORS docs/releasenotes/1.7.28.rst reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js reviewboard/static/rb/js/resources/models/apiTokenModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/admin/views.py reviewboard/diffviewer/renderers.py reviewboard/webapi/resources/root.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/evolutions/__init__.py reviewboard/webapi/resources/diff.py reviewboard/staticbundles.py reviewboard/diffviewer/evolutions/raw_diff_file_data.py reviewboard/admin/forms.py reviewboard/attachments/models.py reviewboard/webapi/resources/base_original_file.py setup.py reviewboard/diffviewer/managers.py reviewboard/admin/siteconfig.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/admin/checks.py reviewboard/diffviewer/management/commands/condensediffs.py reviewboard/webapi/resources/base_patched_file.py reviewboard/scmtools/hg.py reviewboard/__init__.py reviewboard/webapi/resources/filediff.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/models.py Ignored Files: reviewboard/static/rb/js/resources/models/fileAttachmentUserModel.js reviewboard/static/rb/js/resources/models/baseResourceModel.js docs/releasenotes/index.rst docs/manual/admin/installation/linux.rst docs/manual/admin/configuration/file-storage-settings.rst docs/releasenotes/2.0.6.rst AUTHORS docs/releasenotes/1.7.28.rst reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js reviewboard/static/rb/js/resources/models/apiTokenModel.js
-
-
-
-
-
-
-
-
-
-
-
-
-
reviewboard/admin/views.py (Diff revision 7) local variable 'positions_key' is assigned to but never used
-
reviewboard/diffviewer/tests.py (Diff revision 7) redefinition of unused 'FileDiffMigrationTests' from line 726
-
reviewboard/reviews/views.py (Diff revision 7) list comprehension redefines 'file_attachment' from line 583
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+212 -4) |

-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/root.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/attachments/models.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/resources/models/apiTokenModel.js reviewboard/static/rb/js/resources/models/fileAttachmentUserModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/root.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/attachments/models.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/resources/models/apiTokenModel.js reviewboard/static/rb/js/resources/models/fileAttachmentUserModel.js
-
This also needs unit tests for the new API.
-
reviewboard/attachments/models.py (Diff revision 8) We generally avoid the ternary form because it's annoying. How about rewriting this as:
if self.orig_filename: return self.orig_filename elif self.file: return os.path.basename(self.file.name) else: return None
-
reviewboard/static/rb/js/resources/models/apiTokenModel.js (Diff revision 8) Why did you make this change?
-
reviewboard/static/rb/js/resources/models/fileAttachmentUserModel.js (Diff revision 8) Trailing spaces in javascript can break IE.
-
-
reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8) This should probably include everything if it's an admin making the request.
-
reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8) Docstrings should generally follow the same format that you might use for a commit message -- the first line should be a summary, and then additional paragraphs can go into more explanation.
Indentation should also be:
def create(self, request, *args, **kwargs): """Create a FileAttachment Here's some more explanation. """
I'm also not 100% sure we should completely ignore the POST parameters. If a file or caption is present, we should use it (the web UI isn't the only consumer of the API).
-
reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8) Same comments as the above docstring.
-
reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8) What happens if you just let it assign None to the caption field?
-
reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8) PERMISSION_DENIED doesn't seem quite right. How about creating a new error, FILE_ALREADY_EXISTS?
Change Summary:
Made changes according to David's remarks
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+297 -49) |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/root.py reviewboard/attachments/models.py reviewboard/webapi/errors.py reviewboard/attachments/evolutions/__init__.py reviewboard/attachments/mimetypes.py reviewboard/attachments/evolutions/file_attachment_ownership.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/attachments/forms.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/resources/models/apiTokenModel.js reviewboard/static/rb/js/resources/models/fileAttachmentUserModel.js Tool: Pyflakes Processed Files: reviewboard/webapi/resources/root.py reviewboard/attachments/models.py reviewboard/webapi/errors.py reviewboard/attachments/evolutions/__init__.py reviewboard/attachments/mimetypes.py reviewboard/attachments/evolutions/file_attachment_ownership.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/attachments/forms.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/resources/models/apiTokenModel.js reviewboard/static/rb/js/resources/models/fileAttachmentUserModel.js
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+301 -49) |

-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/root.py reviewboard/attachments/models.py reviewboard/webapi/errors.py reviewboard/attachments/evolutions/__init__.py reviewboard/attachments/mimetypes.py reviewboard/attachments/evolutions/file_attachment_ownership.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/attachments/forms.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/resources/models/apiTokenModel.js reviewboard/static/rb/js/resources/models/fileAttachmentUserModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/root.py reviewboard/attachments/models.py reviewboard/webapi/errors.py reviewboard/attachments/evolutions/__init__.py reviewboard/attachments/mimetypes.py reviewboard/attachments/evolutions/file_attachment_ownership.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/attachments/forms.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/resources/models/apiTokenModel.js reviewboard/static/rb/js/resources/models/fileAttachmentUserModel.js
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+300 -49) |

-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/root.py reviewboard/attachments/models.py reviewboard/webapi/errors.py reviewboard/attachments/evolutions/__init__.py reviewboard/attachments/mimetypes.py reviewboard/attachments/evolutions/file_attachment_ownership.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/attachments/forms.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/resources/models/apiTokenModel.js reviewboard/static/rb/js/resources/models/fileAttachmentUserModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/root.py reviewboard/attachments/models.py reviewboard/webapi/errors.py reviewboard/attachments/evolutions/__init__.py reviewboard/attachments/mimetypes.py reviewboard/attachments/evolutions/file_attachment_ownership.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/attachments/forms.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/resources/models/apiTokenModel.js reviewboard/static/rb/js/resources/models/fileAttachmentUserModel.js