Drag 'n Drop Inline Images Backend
Review Request #6454 — Created Oct. 17, 2014 and discarded
Adding support for dragging and dropping images into the markdown editor. Instead of having upload their image first and then link to that image, users can simply drag-and-drop their image file into the markdown editor. This will upload the image to reviewboard and insert a link to that image in the editor.
These changes focus on the backend work involved. This includes:
- Extending FileAttachment Model to include a User, Local Site and UUID field
- Creating a view that will redirect to a FileAttachments' file given the FileAttachment's UUID. This view can be accessed at /users/<username>/file-attachments/<uuid>/
- Creating a Web API for user file attachments, UserFileAttachment resource.
- UserFileAttachment resource can be accessed at /api/users/<username>/file-attachments/<id>/
- Users can only get/modify/delete their own UserFileAttachments. Superusers can get/modify any.
- UserFileAttachments can be created (using POST) without an associated file, in this case the database entry will be created, and the view will return a 404.
- UserFileAttachments can be modified (using PUT) to include a file. This can only be done to UserFileAttachments that do not already have an asosciated file.
- When deleting a UserFileAttachment, the associated file is also deleted.Also did some refactoring of the base_file_attachment Web API resource so that it could be extended by both user_file_attachment and review_request_file_attachment resources.
See https://reviews.reviewboard.org/r/6220/ for initial prototype.
All unit tests pass after making the changes.
Extensive manual testing of UserFileAttachment Web API resource using Postman REST client:
- Testing each method (GET/POST/PUT/DELETE) for UserFileAttachment as a regular user and superuser.
- Testing local_site functionalityAdded unit tests around the changes to the FileAttachment model:
- Tests around creating a FileAttachment with/without a file
- Tests around the is_accessible_by(user) and is_mutable_by(user) methodsAdded unit tests around the new UserFileAttachment WebAPI Resource.
Description | From | Last Updated |
---|---|---|
Col: 1 E101 indentation contains mixed spaces and tabs |
reviewbot | |
Col: 17 E126 continuation line over-indented for hanging indent |
reviewbot | |
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot | |
Col: 17 E126 continuation line over-indented for hanging indent |
reviewbot | |
Col: 29 E126 continuation line over-indented for hanging indent |
reviewbot | |
Col: 17 E126 continuation line over-indented for hanging indent |
reviewbot | |
Col: 17 E126 continuation line over-indented for hanging indent |
reviewbot | |
Col: 29 E126 continuation line over-indented for hanging indent |
reviewbot | |
Col: 17 E126 continuation line over-indented for hanging indent |
reviewbot | |
Col: 17 E126 continuation line over-indented for hanging indent |
reviewbot | |
Should remove the extra blank line between the """ and the fields. |
chipx86 | |
We're doing this in multiple places now. We need to only have this logic in one place. |
chipx86 | |
Should remove the extra blank line before the """. |
chipx86 | |
And the one after. |
chipx86 | |
Remove this blank line. |
chipx86 | |
Remove the blank line. |
chipx86 | |
No blank line here. Same below right after a function definition. |
chipx86 | |
Another case that should be split out. |
chipx86 | |
file is a type in Python, so this should be renamed to something like filename. |
brennie | |
Same here. |
brennie | |
Why not just if self.mimetype_handler: return self.mimetype_handler.get_thumbnail() return None It gets rid of the ugly ternary and the backslash. |
brennie | |
As Barret, says, we don't use ternary operators. |
chipx86 | |
Redo this to not use ternary? |
brennie | |
Same here with regards to ternary and backslash |
brennie | |
No blank line. |
chipx86 | |
Unnecessary blank line. |
brennie | |
Docstrings are always in one of the following forms: """Single-line summary.""" or: """Single-line summary. Multi-line description. """ Also, "Used to … |
chipx86 | |
Unnecessary blank line. |
brennie | |
Should be combined to one statement. You also need to check user.is_authorized(). |
chipx86 | |
Unnecessary blank line. |
brennie | |
Should be one statement. |
chipx86 | |
No need for parens. |
chipx86 | |
No blank line. |
chipx86 | |
Indentation issues. |
chipx86 | |
Unnecessary blank line. |
brennie | |
Unnecessary blank line |
brennie | |
file is a type in Python. |
brennie | |
Unnecessary blank line |
brennie | |
Unnecessary blank line |
brennie | |
Unnecessary blank line |
brennie | |
I think this is probably base as a URL off of /users/<username>/. The view should then take the username from … |
chipx86 | |
We use parens for multi-line conditionals. As per the comment in the urls.py change, this should check the username in … |
chipx86 | |
There's a lot of duplication between this and create_file_attachment. I'd suggest creating a _create_base_file_attachment that handles the common logic, and … |
chipx86 | |
Undo this change. |
brennie | |
You probably also want user__isnull=True. |
chipx86 | |
These are all third-party modules, from the perspective of this project, so must be in the same group. Also, the … |
chipx86 | |
Alphabetical order. |
chipx86 | |
No blank line. |
chipx86 | |
This must have a docstring. The docstring will be directly used on the API docs, so it must be intended … |
chipx86 | |
I would call this UserFileAttachmentResource. |
chipx86 | |
You need to define a name. You can't reuse the other name. |
chipx86 | |
We should be setting up a link between this and the appropriate user resource, rather than simply serializing the username. … |
chipx86 | |
Missing a trailing period. |
chipx86 | |
The payloads are different from the file attachment payloads from other resources, so we can't use the same mimetypes. This … |
chipx86 | |
This will end up doing a database query per object, since we have to fetch the local_site relation. We already … |
chipx86 | |
As per the URL stuff above, this will need to include the username. |
chipx86 | |
We need to use `build_server_url() with this, or it won't actually be absolute. |
chipx86 | |
No blank line. |
chipx86 | |
If local_site is None, then the two statements would be equivalent. You shouldn't need the conditional. |
chipx86 | |
Blank line between these. |
chipx86 | |
This means an authenticated user will only ever be able to list their own attachments, and an anonymous user will … |
chipx86 | |
There's no such thing :) |
chipx86 | |
No blank line. |
chipx86 | |
Ah, I see where the above comes from. A better option is to instead just check if the user has … |
chipx86 | |
These can be combined. |
chipx86 | |
You shouldn't need to call serialize_object. Instead, you should just need to pass the object. |
chipx86 | |
No blank line. |
chipx86 | |
Same as above. |
chipx86 | |
Like the other docstrings, these are all going to be publicly shown on the website. Make sure to keep to … |
chipx86 | |
No blank line. |
chipx86 | |
user_file_attachment |
chipx86 | |
I don't think this is the right URL. |
chipx86 | |
These blank lines should go. Same with all functions below. |
chipx86 | |
Space before \. Same below. |
chipx86 | |
This is a lot of verbose and unnecessary work. Instead, just do what we do in test_file_attachment.py. Call the create … |
chipx86 | |
See above with the info on docstrings. Also, the URL is wrong. |
chipx86 | |
user_file_attachment |
chipx86 | |
This file was created after running a test and I accidentally added it. Should this be removed, and should there … |
dkus | |
Blank line between these. |
chipx86 | |
Same here. |
chipx86 | |
Same here. |
chipx86 | |
A top-level urlpatterns indicates that the stuff will live pretty close to the root of the site. However, this is … |
chipx86 | |
Should have a docstring. |
chipx86 | |
This can go directly in the query above. get_object_or_404( ..., local_site=local_site, file__isnull=False) |
chipx86 | |
We define this prefix twice. What we can do to avoid that (and avoid introducing problems if we ever have … |
chipx86 | |
The blank line is on the wrong side of that import. |
chipx86 | |
This needs to be fleshed out. This docstring is what will be publicly shown in the API docs on our … |
chipx86 | |
"Handles" is probably not the best for the public docs. "Uploads a file attachment associated with the user." would be … |
chipx86 | |
Similar comment here. |
chipx86 | |
Things like FileAttachment are internal and should not be exposed in any public docs. Also, "Including the file." |
chipx86 | |
Missing period. FILE_ALREADY_EXISTS is an internal detail. We do have a handy thing for referencing errors, though: :ref:`webapi2.0-error-<code>`. Where <code> … |
chipx86 | |
"Deletes a file attachment owned by the user." |
chipx86 | |
Again on here, we don't want to talk about models or internal classes. Let's also say "administrator" instead of "superuser." |
chipx86 | |
This is true of all DELETE methods. |
chipx86 |
- Change Summary:
-
Fixing white space.
- Commit:
-
15c3ef9724211c96a8c8272b2d75aed3f26659b9b4cb8950525fe32bad8967de0347baf9185ba397
- Diff:
-
Revision 2 (+711 -325)
-
Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/root.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.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.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/attachments/urls.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/views.py Tool: Pyflakes Processed Files: reviewboard/webapi/resources/root.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.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.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/attachments/urls.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/views.py
- Change Summary:
-
Adding some unit tests for user file attachments model / form.
- Commit:
-
b4cb8950525fe32bad8967de0347baf9185ba3971b06560659e32aae699123bbe2810619faa37305
- Diff:
-
Revision 3 (+843 -326)
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/root.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.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.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/attachments/urls.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/tests.py reviewboard/attachments/views.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/root.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.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.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/attachments/urls.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/tests.py reviewboard/attachments/views.py
-
- Change Summary:
-
Fixing line length. Thanks Review Bot!
- Commit:
-
1b06560659e32aae699123bbe2810619faa3730520f7fe98ae15af6093b57ab942fa8ebdced7d3d7
- Diff:
-
Revision 4 (+845 -326)
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/root.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.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.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/attachments/urls.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/tests.py reviewboard/attachments/views.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/root.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.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.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/attachments/urls.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/tests.py reviewboard/attachments/views.py
- Change Summary:
-
Added unit test around the WebAPI resource.
- Description:
-
Adding support for dragging and dropping images into the markdown editor. Instead of having upload their image first and then link to that image, users can simply drag-and-drop their image file into the markdown editor. This will upload the image to reviewboard and insert a link to that image in the editor.
These changes focus on the backend work involved. This includes:
- Extending FileAttachment Model to include a User and Local Site field - Creating a Web API for user file attachments - Creating a view that can redirect to a FileAttachments' file given the FileAttachment's ID Also did some refactoring of the base_file_attachment Web API resource so that it could be extended by both user_file_attachment and review_request_file_attachment resources.
See https://reviews.reviewboard.org/r/6220/ for initial prototype.
- - Still a work in progress.
- Testing Done:
-
All unit tests pass after making the changes.
Extensive manual testing of user_file_attachment Web API resource using Postman REST client.
~ Working on adding unit tests
~ Added unit tests around the changes to the FileAttachment model.
+ Added unit tests around the new FileAttachmentUser WebAPI Resource. - Commit:
-
20f7fe98ae15af6093b57ab942fa8ebdced7d3d7fd5a9b30f12a854a7b08e9d60ec25499055cc466
- Diff:
-
Revision 5 (+1095 -328)
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.py reviewboard/attachments/models.py reviewboard/webapi/errors.py reviewboard/attachments/evolutions/__init__.py reviewboard/attachments/mimetypes.py reviewboard/webapi/resources/root.py reviewboard/attachments/evolutions/file_attachment_ownership.py reviewboard/webapi/resources/file_attachment.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/attachments/urls.py reviewboard/webapi/tests/test_file_attachment_user.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/tests.py reviewboard/attachments/views.py reviewboard/webapi/tests/urls.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.py reviewboard/attachments/models.py reviewboard/webapi/errors.py reviewboard/attachments/evolutions/__init__.py reviewboard/attachments/mimetypes.py reviewboard/webapi/resources/root.py reviewboard/attachments/evolutions/file_attachment_ownership.py reviewboard/webapi/resources/file_attachment.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/attachments/urls.py reviewboard/webapi/tests/test_file_attachment_user.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/tests.py reviewboard/attachments/views.py reviewboard/webapi/tests/urls.py
-
-
-
- Change Summary:
-
Updating some comments
- Summary:
-
[WIP] Drag 'n Drop Inline Images BackendDrag 'n Drop Inline Images Backend
- Commit:
-
fd5a9b30f12a854a7b08e9d60ec25499055cc4660f819692d8ae7e01d303995eacba855aa993def0
- Diff:
-
Revision 6 (+1094 -328)
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.py reviewboard/attachments/models.py reviewboard/webapi/errors.py reviewboard/attachments/evolutions/__init__.py reviewboard/attachments/mimetypes.py reviewboard/webapi/resources/root.py reviewboard/attachments/evolutions/file_attachment_ownership.py reviewboard/webapi/resources/file_attachment.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/attachments/urls.py reviewboard/webapi/tests/test_file_attachment_user.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/tests.py reviewboard/attachments/views.py reviewboard/webapi/tests/urls.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.py reviewboard/attachments/models.py reviewboard/webapi/errors.py reviewboard/attachments/evolutions/__init__.py reviewboard/attachments/mimetypes.py reviewboard/webapi/resources/root.py reviewboard/attachments/evolutions/file_attachment_ownership.py reviewboard/webapi/resources/file_attachment.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/attachments/urls.py reviewboard/webapi/tests/test_file_attachment_user.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/tests.py reviewboard/attachments/views.py reviewboard/webapi/tests/urls.py
-
-
-
- Change Summary:
-
Some style changes in an attempt to appease the review bot gods.
- Commit:
-
0f819692d8ae7e01d303995eacba855aa993def01094b94c36a7575685cfe89783bb759160dddf69
- Diff:
-
Revision 7 (+1089 -328)
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.py reviewboard/attachments/models.py reviewboard/webapi/errors.py reviewboard/attachments/evolutions/__init__.py reviewboard/attachments/mimetypes.py reviewboard/webapi/resources/root.py reviewboard/attachments/evolutions/file_attachment_ownership.py reviewboard/webapi/resources/file_attachment.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/attachments/urls.py reviewboard/webapi/tests/test_file_attachment_user.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/tests.py reviewboard/attachments/views.py reviewboard/webapi/tests/urls.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.py reviewboard/attachments/models.py reviewboard/webapi/errors.py reviewboard/attachments/evolutions/__init__.py reviewboard/attachments/mimetypes.py reviewboard/webapi/resources/root.py reviewboard/attachments/evolutions/file_attachment_ownership.py reviewboard/webapi/resources/file_attachment.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/attachments/urls.py reviewboard/webapi/tests/test_file_attachment_user.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/tests.py reviewboard/attachments/views.py reviewboard/webapi/tests/urls.py
-
- Change Summary:
-
Formatting
- Commit:
-
1094b94c36a7575685cfe89783bb759160dddf69e846ed0fd70944ffbed117b66591832ebe808412
- Diff:
-
Revision 8 (+1092 -328)
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.py reviewboard/attachments/models.py reviewboard/webapi/errors.py reviewboard/attachments/evolutions/__init__.py reviewboard/attachments/mimetypes.py reviewboard/webapi/resources/root.py reviewboard/attachments/evolutions/file_attachment_ownership.py reviewboard/webapi/resources/file_attachment.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/attachments/urls.py reviewboard/webapi/tests/test_file_attachment_user.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/tests.py reviewboard/attachments/views.py reviewboard/webapi/tests/urls.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.py reviewboard/attachments/models.py reviewboard/webapi/errors.py reviewboard/attachments/evolutions/__init__.py reviewboard/attachments/mimetypes.py reviewboard/webapi/resources/root.py reviewboard/attachments/evolutions/file_attachment_ownership.py reviewboard/webapi/resources/file_attachment.py reviewboard/webapi/resources/file_attachment_user.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/attachments/urls.py reviewboard/webapi/tests/test_file_attachment_user.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/tests.py reviewboard/attachments/views.py reviewboard/webapi/tests/urls.py
-
-
-
-
-
-
-
-
-
-
-
-
Docstrings are always in one of the following forms:
"""Single-line summary."""
or:
"""Single-line summary. Multi-line description. """
Also, "Used to ..." isn't quite right. Look at our other "is_accessible_by" and similar functions for how we document these functions.
Same below.
-
-
-
-
-
-
I think this is probably base as a URL off of
/users/<username>/
. The view should then take the username from the URL and use that as part of the query for the file attachment. -
We use parens for multi-line conditionals.
As per the comment in the urls.py change, this should check the username in the URL.
-
There's a lot of duplication between this and
create_file_attachment
. I'd suggest creating a_create_base_file_attachment
that handles the common logic, and building off of that for both. -
-
These are all third-party modules, from the perspective of this project, so must be in the same group. Also, the imports must all be in alphabetical order.
-
-
-
This must have a docstring.
The docstring will be directly used on the API docs, so it must be intended for third-party consumers of our API to read. You can use our others as a base.
-
-
-
We should be setting up a link between this and the appropriate user resource, rather than simply serializing the username. Links are a good thing, and the username can be retrieved from that link. Should update the 'type' and 'description' appropriately, and remove the serialize function below.
-
-
The payloads are different from the file attachment payloads from other resources, so we can't use the same mimetypes. This must be
user-file-attachment(s)
. -
This will end up doing a database query per object, since we have to fetch the
local_site
relation.We already know, from our query in
get_queryset()
, that the local site is the same one provided in the URL. So, instead, you should use a variable we have handy:request._local_site_name
. -
-
-
-
If
local_site
isNone
, then the two statements would be equivalent. You shouldn't need the conditional. -
-
This means an authenticated user will only ever be able to list their own attachments, and an anonymous user will be able to access everyone's attachments.
I don't remember if we discussed how permissions should work here. Please let me know if this was already discussed. If not, I see two possibilities:
1) Users can only ever list/access their own attachments (meaning we should check
request.user.is_anonymous()
up-front and returnself.model.objects.none()
otherwise, and should always have theuser=
query even if not a list).2) Anyone can list anyone's attachments, in which case the username should be an optional query parameter on the URL.
-
-
-
Ah, I see where the above comes from.
A better option is to instead just check if the user has access to the local_site.
-
-
-
-
-
Like the other docstrings, these are all going to be publicly shown on the website. Make sure to keep to the styling and format and usefulness of other resources.
-
-
-
-
-
-
This is a lot of verbose and unnecessary work. Instead, just do what we do in
test_file_attachment.py
. Call the create functions inline when defining the lists to return. -
-
- Change Summary:
-
Lots of changes from reviews
- Commit:
-
e846ed0fd70944ffbed117b66591832ebe808412649a9588d354969069d40c48403b508b0cd39cf3
- Diff:
-
Revision 9 (+1142 -339)
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.py reviewboard/webapi/tests/test_user_file_attachment.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.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/attachments/urls.py reviewboard/webapi/tests/mimetypes.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/tests.py reviewboard/webapi/resources/user.py reviewboard/attachments/views.py reviewboard/webapi/tests/urls.py reviewboard/webapi/resources/user_file_attachment.py Ignored Files: reviewboard/scmtools/testdata/hg_repo/.hg/cache/branchheads Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.py reviewboard/webapi/tests/test_user_file_attachment.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.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/attachments/urls.py reviewboard/webapi/tests/mimetypes.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/tests.py reviewboard/webapi/resources/user.py reviewboard/attachments/views.py reviewboard/webapi/tests/urls.py reviewboard/webapi/resources/user_file_attachment.py Ignored Files: reviewboard/scmtools/testdata/hg_repo/.hg/cache/branchheads
- Change Summary:
-
Updating description / testing done to reflect some changes made in that last diff
- Description:
-
Adding support for dragging and dropping images into the markdown editor. Instead of having upload their image first and then link to that image, users can simply drag-and-drop their image file into the markdown editor. This will upload the image to reviewboard and insert a link to that image in the editor.
These changes focus on the backend work involved. This includes:
~ - Extending FileAttachment Model to include a User and Local Site field ~ - Extending FileAttachment Model to include a User, Local Site and UUID field - Creating a Web API for user file attachments ~ - Creating a view that can redirect to a FileAttachments' file given the FileAttachment's ID ~ - Creating a view that can redirect to a FileAttachments' file given the FileAttachment's UUID Also did some refactoring of the base_file_attachment Web API resource so that it could be extended by both user_file_attachment and review_request_file_attachment resources.
See https://reviews.reviewboard.org/r/6220/ for initial prototype.
- Testing Done:
-
All unit tests pass after making the changes.
Extensive manual testing of user_file_attachment Web API resource using Postman REST client.
Added unit tests around the changes to the FileAttachment model.
~ Added unit tests around the new FileAttachmentUser WebAPI Resource. ~ Added unit tests around the new UserFileAttachment WebAPI Resource.
- Change Summary:
-
Making sure the file attachment is deleted instead of just the db entry.
Also getting rid of that testdata file which should not have been included - Description:
-
Adding support for dragging and dropping images into the markdown editor. Instead of having upload their image first and then link to that image, users can simply drag-and-drop their image file into the markdown editor. This will upload the image to reviewboard and insert a link to that image in the editor.
These changes focus on the backend work involved. This includes:
- Extending FileAttachment Model to include a User, Local Site and UUID field ~ - Creating a Web API for user file attachments ~ - Creating a view that can redirect to a FileAttachments' file given the FileAttachment's UUID ~ - Creating a view that will redirect to a FileAttachments' file given the FileAttachment's UUID. This view can be accessed at /users/<username>/file-attachments/<uuid>/ ~ - Creating a Web API for user file attachments, UserFileAttachment resource. + - UserFileAttachment resource can be accessed at /api/users/<username>/file-attachments/<id>/ + - Users can only get/modify/delete their own UserFileAttachments. Superusers can get/modify any. + - UserFileAttachments can be created (using POST) without an associated file, in this case the database entry will be created, and the view will return a 404. + - UserFileAttachments can be modified (using PUT) to include a file. This can only be done to UserFileAttachments that do not already have an asosciated file. + - When deleting a UserFileAttachment, the associated file is also deleted. Also did some refactoring of the base_file_attachment Web API resource so that it could be extended by both user_file_attachment and review_request_file_attachment resources.
See https://reviews.reviewboard.org/r/6220/ for initial prototype.
- Testing Done:
-
All unit tests pass after making the changes.
~ Extensive manual testing of user_file_attachment Web API resource using Postman REST client.
~ Extensive manual testing of UserFileAttachment Web API resource using Postman REST client:
+ - Testing each method (GET/POST/PUT/DELETE) for UserFileAttachment as a regular user and superuser. + - Testing local_site functionality ~ Added unit tests around the changes to the FileAttachment model.
~ Added unit tests around the new UserFileAttachment WebAPI Resource. ~ Added unit tests around the changes to the FileAttachment model:
~ - Tests around creating a FileAttachment with/without a file + - Tests around the is_accessible_by(user) and is_mutable_by(user) methods + + Added unit tests around the new UserFileAttachment WebAPI Resource.
- Commit:
-
649a9588d354969069d40c48403b508b0cd39cf36d1f8f3f7c5716d0d56fda61c12d9c41d719fe2e
- Diff:
-
Revision 10 (+1143 -339)
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.py reviewboard/webapi/tests/test_user_file_attachment.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.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/attachments/urls.py reviewboard/webapi/tests/mimetypes.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/tests.py reviewboard/webapi/resources/user.py reviewboard/attachments/views.py reviewboard/webapi/tests/urls.py reviewboard/webapi/resources/user_file_attachment.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.py reviewboard/webapi/tests/test_user_file_attachment.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.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/attachments/urls.py reviewboard/webapi/tests/mimetypes.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/tests.py reviewboard/webapi/resources/user.py reviewboard/attachments/views.py reviewboard/webapi/tests/urls.py reviewboard/webapi/resources/user_file_attachment.py
-
Great work, David. There's a lot of awesome stuff in this change.
My biggest comment is that the change itself is just doing too much, making it hard to review. Ideally, we'd want more bite-sized changes. It's not too late, though. There's a lot of good pieces of this you could pretty easily split out.
My suggestions for separate commits would be:
- The model changes to FileAttachment + evolution.
- The reorganization of code for the file attachment forms, mimetype stuff, etc.
- The redirect view and URL changes.
- The API changes.
That will let us get this all in piece-by-piece, and provide a more thorough review.
Now your first question will likely be, "how do I do this?" What I recommend you do is create a branch off of master for your first commit and do:
$ git merge --squash --no-commit your-orig-branch-name $ git reset
This will get all the changes into your working directory for the branch, with nothing ready to commit.
You can then do this for each of the files you want to include in that change:
git add -p <filename>
This will walk you through the chunks. You can schedule each for commit. Hit 's' to split a large set of changes into smaller ones. You can even edit them with 'e'. Powerful, powerful stuff, if you haven't worked with it yet.
Then when ready, commit it, and post that for review. Then create a new branch for commit 2, begin the
git add -p
work again, etc., until done.The result will be a series of new commits that will be far easier to land. I'm available in IRC if you have any questions! This will be easier than it sounds, and you won't risk losing your old branch.
-
-
-
-
A top-level
urlpatterns
indicates that the stuff will live pretty close to the root of the site. However, this is intended for usage deep within the URL patterns.I'd suggest nuking this and instead including this directly in the URLs for the user in the root urls.py file. (See my other comment for some organizational ideas.)
-
-
This can go directly in the query above.
get_object_or_404( ..., local_site=local_site, file__isnull=False)
-
We define this prefix twice. What we can do to avoid that (and avoid introducing problems if we ever have to update that regex later) is to do:
user_urlpatterns = patterns( '', url('^infobox/$', ...), url('^file-attachments/$', ...), ) ... url(r'^users/(?P<username>[...])/', include(user_urlpatterns), ...
-
-
This needs to be fleshed out. This docstring is what will be publicly shown in the API docs on our site. We also take the docstrings for get, get_list, create, update, and delete, and use those for the site.
-
"Handles" is probably not the best for the public docs.
"Uploads a file attachment associated with the user." would be better.
-
-
Things like FileAttachment are internal and should not be exposed in any public docs.
Also, "Including the file."
-
Missing period.
FILE_ALREADY_EXISTS
is an internal detail. We do have a handy thing for referencing errors, though::ref:`webapi2.0-error-<code>`.
Where
<code>
is the API error code.Note that you will need to add a doc page for that error you introduced in docs/manual/webapi/2.0/errors/. You can see the other files for examples.
-
-
Again on here, we don't want to talk about models or internal classes.
Let's also say "administrator" instead of "superuser."
-
- Change Summary:
-
Updating the API documentation and cleaning up the urls
- Commit:
-
6d1f8f3f7c5716d0d56fda61c12d9c41d719fe2e40f4f0696fef14dea7936ab4b5b740ae36f5741a
- Diff:
-
Revision 11 (+1183 -341)
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.py reviewboard/webapi/tests/test_user_file_attachment.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.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/webapi/tests/mimetypes.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/tests.py reviewboard/webapi/resources/user.py reviewboard/attachments/views.py reviewboard/webapi/tests/urls.py reviewboard/webapi/resources/user_file_attachment.py Ignored Files: docs/manual/webapi/2.0/errors/229-file-already-exists.rst docs/manual/webapi/2.0/errors/index.rst Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/resources/base_file_attachment.py reviewboard/webapi/resources/draft_file_attachment.py reviewboard/webapi/resources/diff_file_attachment.py reviewboard/webapi/tests/test_user_file_attachment.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.py reviewboard/webapi/resources/base_review_request_file_attachment.py reviewboard/webapi/tests/mimetypes.py reviewboard/attachments/forms.py reviewboard/urls.py reviewboard/attachments/tests.py reviewboard/webapi/resources/user.py reviewboard/attachments/views.py reviewboard/webapi/tests/urls.py reviewboard/webapi/resources/user_file_attachment.py Ignored Files: docs/manual/webapi/2.0/errors/229-file-already-exists.rst docs/manual/webapi/2.0/errors/index.rst
- Status:
- Discarded
- Change Summary:
-
This review request has been split into the following review requests:
https://reviews.reviewboard.org/r/6614/ (FileAttachment model changes)
https://reviews.reviewboard.org/r/6615/ (FileAttachment form changes)
https://reviews.reviewboard.org/r/6616/ (Adding the view)
https://reviews.reviewboard.org/r/6617/ (Adding the WebAPI Resource)