Summary

Drag 'n Drop Inline Images Backend - WebAPI

Review Request #6617 — Created Nov. 21, 2014 and submitted Dec. 4, 2014, 12:23 a.m.

Information

Owner

dkus

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description

Part of a set of changes to add support for dragging and dropping images into the markdown editor.

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.

Testing Done

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 functionality

Added unit tests around the new UserFileAttachment WebAPI Resource.

Issues

Description	From	Last Updated
These two statements can be combined.	david	Nov. 23, 2014, 8:56 a.m.
You can remove the "that is" part.	chipx86	Nov. 23, 2014, 8:56 a.m.
This isn't your fault, but this is no longer true, given that the thumbnail can also be updated. Can you …	chipx86	Nov. 23, 2014, 8:58 a.m.
We use resources everywhere else. We should keep consistent with that.	chipx86	Nov. 23, 2014, 12:05 p.m.
This will appear in the published docs, so we should flesh this out a bit more, make it easier to …	chipx86	Nov. 23, 2014, 9:16 a.m.
This can be one statement.	chipx86	Nov. 23, 2014, 9:17 a.m.
user_file_attachment is local to this file, so no need to access it through something else.	chipx86	Nov. 23, 2014, 12:22 p.m.
We have all the info we need at the time we're building the filter, so let's just do this inside …	chipx86	Nov. 23, 2014, 9:18 a.m.
The summary must fit on a single line. You can try: Returns information on a user's file attachment.	chipx86	Nov. 23, 2014, 9:19 a.m.
Both of these are saying the same thing.	chipx86	Nov. 23, 2014, 9:19 a.m.
This isn't strictly true. It's not expected, but rather, optional. Maybe: "If file data is provided, then it is expected …	chipx86	Nov. 23, 2014, 9:22 a.m.
Some of this can use the has_list_access_permissions().	chipx86	Nov. 28, 2014, 11:35 a.m.
I don't think you need to copy the dictionary.	chipx86	Nov. 23, 2014, 9:33 a.m.
This can be: if 'path' in request.FILES ...:	chipx86	Nov. 23, 2014, 9:44 a.m.
Shouldn't need to copy.	chipx86	Nov. 23, 2014, 9:45 a.m.
""" on the next line. (Note the above comment about how summaries must be on a single line? Unit tests …	chipx86	Nov. 23, 2014, 9:47 a.m.
No period at the end.	chipx86	Nov. 23, 2014, 9:47 a.m.
Why does this one have a _ prefix, but create_user_file_attachment does not? Why is create_base_file_attachment "private"?	mike_conley	Dec. 4, 2014, 12:15 a.m.
Nit - local_site_name is the first item in the rest of these url getters - might as well stick with …	mike_conley	Nov. 28, 2014, 4:47 p.m.

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/errors.py
    reviewboard/webapi/resources/file_attachment.py
    reviewboard/webapi/resources/base_review_request_file_attachment.py
    reviewboard/webapi/tests/mimetypes.py
    reviewboard/webapi/resources/user.py
    reviewboard/webapi/tests/test_user_file_attachment.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/errors.py
    reviewboard/webapi/resources/file_attachment.py
    reviewboard/webapi/resources/base_review_request_file_attachment.py
    reviewboard/webapi/tests/mimetypes.py
    reviewboard/webapi/resources/user.py
    reviewboard/webapi/tests/test_user_file_attachment.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

reviewboard/testing/testcase.py (Diff revision 1)

The issue has been resolved. Show all issues

These two statements can be combined.

Looks good. A few things, mostly having to do with docs, and a few ways to simplify some statements.

reviewboard/webapi/errors.py (Diff revision 1)
The issue has been resolved. Show all issues
```
You can remove the "that is" part.
```

reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revision 1)

The issue has been resolved. Show all issues

This isn't your fault, but this is no longer true, given that the thumbnail can also be updated. Can you rewrite this part of the docstring to explain that both the caption and the file attachment's thumbnail can be updated?

reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)

The issue has been resolved. Show all issues

We use resources everywhere else. We should keep consistent with that.

dkus Nov. 23, 2014, 9:47 a.m.

I think the problem here was that I'm also importing reviewboard.webapi.resources.resources

chipx86 Nov. 23, 2014, 11:31 a.m.

You have code trying to access, say, reviewboard.webapi.resources.user_file_attachment (through the res import), but I can't imagine that works. I would expect it to fail to find the instances you're accessing. That exists on the resources object within that. There's basically no reason you'd need to import reviewboard.webapi.resources itself.

reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)

The issue has been resolved. Show all issues

This will appear in the published docs, so we should flesh this out a bit more, make it easier to read.

How about:

The file attachment is not tied to any particular review
request, and instead is owned by a user for usage in
Markdown-formatted text.

The file contents are optional when first creating a file
attachment. This is to allow a caller to create the attachment
and get the resulting URL for embedding in a text field. The
file's contents can then be added separately (and only once)
in a PUT request.

reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)

The issue has been resolved. Show all issues

This can be one statement.

reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)

The issue has been resolved. Show all issues

We have all the info we need at the time we're building the filter, so let's just do this inside the filter() call:

return self.model.objects.filter(user=user, local_site=local_site)

reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)

The issue has been resolved. Show all issues

The summary must fit on a single line. You can try:

Returns information on a user's file attachment.

reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Both of these are saying the same thing.
```

reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)

The issue has been resolved. Show all issues

This isn't strictly true. It's not expected, but rather, optional. Maybe:

"If file data is provided, then it is expected that the data will be encoded as multipart/form-data. ..."

reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)

The issue has been resolved. Show all issues

Some of this can use the has_list_access_permissions().

dkus Nov. 23, 2014, 9:47 a.m.

I was just looking at this some more. Can has_list_access_permissions() return DOES_NOT_EXIST in the case where the user can not be found? Right now the GET list is just returning 500 when you specify a user that doesn't exists, would be nicer if it returned 404.

chipx86 Nov. 23, 2014, 11:31 a.m.

has_list_access_permissions() can only return True/False.
There should never be a 500. That usually indicates an error with the cod. What does the traceback say?

dkus Nov. 23, 2014, 12:08 p.m.

http://pastie.org/private/mwjbxwngsukqlx5ihcehq

dkus Nov. 23, 2014, 12:09 p.m.

Note: this is when you get the list resource: /api/users/<some_non_existing_user>/file-attachments/

reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
I don't think you need to copy the dictionary.
```

reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)

The issue has been resolved. Show all issues

This can be:

if 'path' in request.FILES ...:

reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Shouldn't need to copy.
```

reviewboard/webapi/tests/test_user_file_attachment.py (Diff revision 1)

The issue has been resolved. Show all issues

""" on the next line.

(Note the above comment about how summaries must be on a single line? Unit tests are the one exception.)

reviewboard/webapi/tests/test_user_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No period at the end.
```

reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
user_file_attachment is local to this file, so no need to access it through something else.
```

Change Summary:

Review request changes. Moving the has_list_access_permission() checks into the get_list() method so that we can return a different result depending on what fails.

Diff:

Revision 2 (+845 -281)

Show changes

	docs/manual/webapi/2.0/errors/229-file-already-exists.rst
	docs/manual/webapi/2.0/errors/index.rst
	reviewboard/testing/testcase.py
	reviewboard/webapi/errors.py
	reviewboard/webapi/resources/base_file_attachment.py
	reviewboard/webapi/resources/base_review_request_file_attachment.py
	reviewboard/webapi/resources/diff_file_attachment.py
	reviewboard/webapi/resources/draft_file_attachment.py
	6 more

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/errors.py
    reviewboard/webapi/resources/file_attachment.py
    reviewboard/webapi/resources/base_review_request_file_attachment.py
    reviewboard/webapi/tests/mimetypes.py
    reviewboard/webapi/resources/user.py
    reviewboard/webapi/tests/test_user_file_attachment.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: 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/errors.py
    reviewboard/webapi/resources/file_attachment.py
    reviewboard/webapi/resources/base_review_request_file_attachment.py
    reviewboard/webapi/tests/mimetypes.py
    reviewboard/webapi/resources/user.py
    reviewboard/webapi/tests/test_user_file_attachment.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

Ship it!

This looks good to me - but you might want another mentor to kick the tires as well.

reviewboard/testing/testcase.py (Diff revision 2)

The issue has been dropped. Show all issues

Why does this one have a _ prefix, but create_user_file_attachment does not? Why is create_base_file_attachment "private"?

dkus Nov. 28, 2014, 4:59 p.m.

I made it private because it's not really intended to be called from outside of here, only through create_user_file_attachment and create_file_attachment. It is the result of extracting some common code from both these methods.

reviewboard/webapi/tests/urls.py (Diff revision 2)

The issue has been resolved. Show all issues

Nit - local_site_name is the first item in the rest of these url getters - might as well stick with the pattern. Same below.

Diff:

Revision 3 (+845 -281)

Show changes

	docs/manual/webapi/2.0/errors/229-file-already-exists.rst
	docs/manual/webapi/2.0/errors/index.rst
	reviewboard/testing/testcase.py
	reviewboard/webapi/errors.py
	reviewboard/webapi/resources/base_file_attachment.py
	reviewboard/webapi/resources/base_review_request_file_attachment.py
	reviewboard/webapi/resources/diff_file_attachment.py
	reviewboard/webapi/resources/draft_file_attachment.py
	6 more

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/errors.py
    reviewboard/webapi/resources/file_attachment.py
    reviewboard/webapi/resources/base_review_request_file_attachment.py
    reviewboard/webapi/tests/mimetypes.py
    reviewboard/webapi/resources/user.py
    reviewboard/webapi/tests/test_user_file_attachment.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: 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/errors.py
    reviewboard/webapi/resources/file_attachment.py
    reviewboard/webapi/resources/base_review_request_file_attachment.py
    reviewboard/webapi/tests/mimetypes.py
    reviewboard/webapi/resources/user.py
    reviewboard/webapi/tests/test_user_file_attachment.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

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to ucosp/dkus/dnd-inline-images (2db233d)