Summary

Drag 'n Drop Inline Images Backend

Review Request #6454 — Created Oct. 17, 2014 and discarded Nov. 21, 2014, 3:32 p.m.

Information

Owner

dkus

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

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 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 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:
- 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.

Issues

Description	From	Last Updated
Col: 1 E101 indentation contains mixed spaces and tabs	reviewbot	Oct. 17, 2014, 1:15 p.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 17, 2014, 1:15 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Oct. 18, 2014, 12:16 p.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 25, 2014, 3:57 p.m.
Col: 29 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 25, 2014, 3:57 p.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 25, 2014, 3:57 p.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 25, 2014, 3:57 p.m.
Col: 29 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 25, 2014, 3:57 p.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 25, 2014, 3:57 p.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 26, 2014, 2:35 p.m.
Should remove the extra blank line between the """ and the fields.	chipx86	Oct. 27, 2014, 8:35 a.m.
We're doing this in multiple places now. We need to only have this logic in one place.	chipx86	Oct. 27, 2014, 3:46 p.m.
Should remove the extra blank line before the """.	chipx86	Oct. 27, 2014, 8:37 a.m.
And the one after.	chipx86	Oct. 27, 2014, 8:37 a.m.
Remove this blank line.	chipx86	Oct. 27, 2014, 8:37 a.m.
Remove the blank line.	chipx86	Oct. 27, 2014, 8:38 a.m.
No blank line here. Same below right after a function definition.	chipx86	Oct. 27, 2014, 8:38 a.m.
Another case that should be split out.	chipx86	Oct. 27, 2014, 3:46 p.m.
file is a type in Python, so this should be renamed to something like filename.	brennie	Oct. 27, 2014, 8:34 a.m.
Same here.	brennie	Oct. 27, 2014, 8:34 a.m.
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	Oct. 27, 2014, 8:21 a.m.
As Barret, says, we don't use ternary operators.	chipx86	Oct. 27, 2014, 8:40 a.m.
Redo this to not use ternary?	brennie	Oct. 27, 2014, 8:22 a.m.
Same here with regards to ternary and backslash	brennie	Oct. 27, 2014, 8:21 a.m.
No blank line.	chipx86	Oct. 27, 2014, 8:40 a.m.
Unnecessary blank line.	brennie	Oct. 27, 2014, 8:25 a.m.
Docstrings are always in one of the following forms: """Single-line summary.""" or: """Single-line summary. Multi-line description. """ Also, "Used to …	chipx86	Oct. 27, 2014, 8:59 a.m.
Unnecessary blank line.	brennie	Oct. 27, 2014, 8:27 a.m.
Should be combined to one statement. You also need to check user.is_authorized().	chipx86	Nov. 1, 2014, 9:16 a.m.
Unnecessary blank line.	brennie	Oct. 27, 2014, 8:26 a.m.
Should be one statement.	chipx86	Oct. 27, 2014, 8:59 a.m.
No need for parens.	chipx86	Oct. 27, 2014, 9:27 a.m.
No blank line.	chipx86	Oct. 27, 2014, 9:27 a.m.
Indentation issues.	chipx86	Oct. 27, 2014, 9:27 a.m.
Unnecessary blank line.	brennie	Oct. 27, 2014, 8:28 a.m.
Unnecessary blank line	brennie	Oct. 27, 2014, 8:28 a.m.
file is a type in Python.	brennie	Oct. 27, 2014, 8:28 a.m.
Unnecessary blank line	brennie	Oct. 27, 2014, 8:29 a.m.
Unnecessary blank line	brennie	Oct. 27, 2014, 8:29 a.m.
Unnecessary blank line	brennie	Oct. 27, 2014, 8:29 a.m.
I think this is probably base as a URL off of /users/<username>/. The view should then take the username from …	chipx86	Oct. 27, 2014, 4:09 p.m.
We use parens for multi-line conditionals. As per the comment in the urls.py change, this should check the username in …	chipx86	Oct. 27, 2014, 4:09 p.m.
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	Oct. 27, 2014, 10:03 a.m.
Undo this change.	brennie	Oct. 27, 2014, 8:31 a.m.
You probably also want user__isnull=True.	chipx86	Oct. 27, 2014, 10:14 a.m.
These are all third-party modules, from the perspective of this project, so must be in the same group. Also, the …	chipx86	Oct. 27, 2014, 10:17 a.m.
Alphabetical order.	chipx86	Oct. 27, 2014, 10:17 a.m.
No blank line.	chipx86	Oct. 27, 2014, 10:18 a.m.
This must have a docstring. The docstring will be directly used on the API docs, so it must be intended …	chipx86	Oct. 27, 2014, 10:21 a.m.
I would call this UserFileAttachmentResource.	chipx86	Oct. 27, 2014, 10:30 a.m.
You need to define a name. You can't reuse the other name.	chipx86	Oct. 27, 2014, 4:11 p.m.
We should be setting up a link between this and the appropriate user resource, rather than simply serializing the username. …	chipx86	Oct. 27, 2014, 10:23 a.m.
Missing a trailing period.	chipx86	Oct. 27, 2014, 10:24 a.m.
The payloads are different from the file attachment payloads from other resources, so we can't use the same mimetypes. This …	chipx86	Oct. 27, 2014, 10:26 a.m.
This will end up doing a database query per object, since we have to fetch the local_site relation. We already …	chipx86	Oct. 27, 2014, 10:40 a.m.
As per the URL stuff above, this will need to include the username.	chipx86	Oct. 27, 2014, 4:11 p.m.
We need to use `build_server_url() with this, or it won't actually be absolute.	chipx86	Oct. 27, 2014, 10:37 a.m.
No blank line.	chipx86	Oct. 27, 2014, 10:27 a.m.
If local_site is None, then the two statements would be equivalent. You shouldn't need the conditional.	chipx86	Oct. 27, 2014, 10:47 a.m.
Blank line between these.	chipx86	Oct. 27, 2014, 10:47 a.m.
This means an authenticated user will only ever be able to list their own attachments, and an anonymous user will …	chipx86	Nov. 1, 2014, 1:22 p.m.
There's no such thing :)	chipx86	Nov. 1, 2014, 9:14 a.m.
No blank line.	chipx86	Oct. 27, 2014, 11:32 a.m.
Ah, I see where the above comes from. A better option is to instead just check if the user has …	chipx86	Nov. 1, 2014, 9:14 a.m.
These can be combined.	chipx86	Oct. 27, 2014, 11:33 a.m.
You shouldn't need to call serialize_object. Instead, you should just need to pass the object.	chipx86	Oct. 27, 2014, 11:34 a.m.
No blank line.	chipx86	Oct. 27, 2014, 11:34 a.m.
Same as above.	chipx86	Oct. 27, 2014, 11:34 a.m.
Like the other docstrings, these are all going to be publicly shown on the website. Make sure to keep to …	chipx86	Nov. 1, 2014, 9:14 a.m.
No blank line.	chipx86	Oct. 27, 2014, 11:35 a.m.
user_file_attachment	chipx86	Oct. 27, 2014, 11:35 a.m.
I don't think this is the right URL.	chipx86	Nov. 1, 2014, 2:07 p.m.
These blank lines should go. Same with all functions below.	chipx86	Oct. 27, 2014, 11:36 a.m.
Space before \. Same below.	chipx86	Oct. 27, 2014, 11:37 a.m.
This is a lot of verbose and unnecessary work. Instead, just do what we do in test_file_attachment.py. Call the create …	chipx86	Nov. 1, 2014, 2:06 p.m.
See above with the info on docstrings. Also, the URL is wrong.	chipx86	Nov. 1, 2014, 2:05 p.m.
user_file_attachment	chipx86	Oct. 27, 2014, 11:46 a.m.
This file was created after running a test and I accidentally added it. Should this be removed, and should there …	dkus	Nov. 8, 2014, 1:55 p.m.
Blank line between these.	chipx86	Nov. 17, 2014, 3:01 p.m.
Same here.	chipx86	Nov. 17, 2014, 3:02 p.m.
Same here.	chipx86	Nov. 17, 2014, 3:02 p.m.
A top-level urlpatterns indicates that the stuff will live pretty close to the root of the site. However, this is …	chipx86	Nov. 17, 2014, 3:03 p.m.
Should have a docstring.	chipx86	Nov. 17, 2014, 3:03 p.m.
This can go directly in the query above. get_object_or_404( ..., local_site=local_site, file__isnull=False)	chipx86	Nov. 17, 2014, 3:03 p.m.
We define this prefix twice. What we can do to avoid that (and avoid introducing problems if we ever have …	chipx86	Nov. 17, 2014, 3:03 p.m.
The blank line is on the wrong side of that import.	chipx86	Nov. 17, 2014, 3:03 p.m.
This needs to be fleshed out. This docstring is what will be publicly shown in the API docs on our …	chipx86	Nov. 21, 2014, 1:13 p.m.
"Handles" is probably not the best for the public docs. "Uploads a file attachment associated with the user." would be …	chipx86	Nov. 21, 2014, 1:13 p.m.
Similar comment here.	chipx86	Nov. 21, 2014, 1:13 p.m.
Things like FileAttachment are internal and should not be exposed in any public docs. Also, "Including the file."	chipx86	Nov. 21, 2014, 1:13 p.m.
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	Nov. 21, 2014, 1:13 p.m.
"Deletes a file attachment owned by the user."	chipx86	Nov. 21, 2014, 1:13 p.m.
Again on here, we don't want to talk about models or internal classes. Let's also say "administrator" instead of "superuser."	chipx86	Nov. 21, 2014, 1:13 p.m.
This is true of all DELETE methods.	chipx86	Nov. 21, 2014, 1:14 p.m.

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

reviewboard/attachments/evolutions/__init__.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 E101 indentation contains mixed spaces and tabs
```
reviewboard/webapi/resources/file_attachment_user.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```

Change Summary:

Fixing white space.

Commit:

-	15c3ef9724211c96a8c8272b2d75aed3f26659b9
+	b4cb8950525fe32bad8967de0347baf9185ba397

Diff:

Revision 2 (+711 -325)

Show changes

	reviewboard/urls.py
	reviewboard/attachments/forms.py
	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/urls.py
	reviewboard/attachments/views.py
	reviewboard/attachments/evolutions/__init__.py
	reviewboard/attachments/evolutions/file_attachment_ownership.py
	8 more

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:

-	b4cb8950525fe32bad8967de0347baf9185ba397
+	1b06560659e32aae699123bbe2810619faa37305

Diff:

Revision 3 (+843 -326)

Show changes

	reviewboard/urls.py
	reviewboard/attachments/forms.py
	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/attachments/urls.py
	reviewboard/attachments/views.py
	reviewboard/attachments/evolutions/__init__.py
	9 more

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

reviewboard/attachments/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Change Summary:

Fixing line length. Thanks Review Bot!

Commit:

-	1b06560659e32aae699123bbe2810619faa37305
+	20f7fe98ae15af6093b57ab942fa8ebdced7d3d7

Diff:

Revision 4 (+845 -326)

Show changes

	reviewboard/urls.py
	reviewboard/attachments/forms.py
	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/attachments/urls.py
	reviewboard/attachments/views.py
	reviewboard/attachments/evolutions/__init__.py
	9 more

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:

-	20f7fe98ae15af6093b57ab942fa8ebdced7d3d7
+	fd5a9b30f12a854a7b08e9d60ec25499055cc466

Diff:

Revision 5 (+1095 -328)

Show changes

	reviewboard/urls.py
	reviewboard/attachments/forms.py
	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/attachments/urls.py
	reviewboard/attachments/views.py
	reviewboard/attachments/evolutions/__init__.py
	12 more

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

reviewboard/webapi/tests/test_file_attachment_user.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/test_file_attachment_user.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 29
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/test_file_attachment_user.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```

Change Summary:

Updating some comments

Summary:

-	[WIP] Drag 'n Drop Inline Images Backend
+	Drag 'n Drop Inline Images Backend

Commit:

-	fd5a9b30f12a854a7b08e9d60ec25499055cc466
+	0f819692d8ae7e01d303995eacba855aa993def0

Diff:

Revision 6 (+1094 -328)

Show changes

	reviewboard/urls.py
	reviewboard/attachments/forms.py
	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/attachments/urls.py
	reviewboard/attachments/views.py
	reviewboard/attachments/evolutions/__init__.py
	12 more

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

reviewboard/webapi/tests/test_file_attachment_user.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/test_file_attachment_user.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 29
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/test_file_attachment_user.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```

Change Summary:

Some style changes in an attempt to appease the review bot gods.

Commit:

-	0f819692d8ae7e01d303995eacba855aa993def0
+	1094b94c36a7575685cfe89783bb759160dddf69

Diff:

Revision 7 (+1089 -328)

Show changes

	reviewboard/urls.py
	reviewboard/attachments/forms.py
	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/attachments/urls.py
	reviewboard/attachments/views.py
	reviewboard/attachments/evolutions/__init__.py
	12 more

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

reviewboard/webapi/tests/test_file_attachment_user.py (Diff revision 7)

The issue has been resolved. Show all issues

Col: 17
 E126 continuation line over-indented for hanging indent

david Oct. 25, 2014, 9:58 p.m.

I think that this can be fixed as follows:

return (get_file_attachment_user_list_url(local_site_name),
        file_attachment_item_mimetype,
        {
            'path': open(self._getTrophyFilename(), 'r'),
            'caption': caption,
        },
        [caption])


Note the { on its own line.

Change Summary:

Formatting

Commit:

-	1094b94c36a7575685cfe89783bb759160dddf69
+	e846ed0fd70944ffbed117b66591832ebe808412

Diff:

Revision 8 (+1092 -328)

Show changes

	reviewboard/urls.py
	reviewboard/attachments/forms.py
	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/attachments/urls.py
	reviewboard/attachments/views.py
	reviewboard/attachments/evolutions/__init__.py
	12 more

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

reviewboard/attachments/mimetypes.py (Diff revision 8)
The issue has been resolved. Show all issues
```
file is a type in Python, so this should be renamed to something like filename.
```
reviewboard/attachments/mimetypes.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Same here.
```

reviewboard/attachments/models.py (Diff revision 8)

The issue has been resolved. Show all issues

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.

reviewboard/attachments/models.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Redo this to not use ternary?
```
reviewboard/attachments/models.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Same here with regards to ternary and backslash
```
reviewboard/attachments/models.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Unnecessary blank line.
```
reviewboard/attachments/models.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Unnecessary blank line.
```
reviewboard/attachments/models.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Unnecessary blank line.
```
reviewboard/attachments/tests.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Unnecessary blank line.
```
reviewboard/attachments/tests.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Unnecessary blank line
```
reviewboard/attachments/tests.py (Diff revision 8)
The issue has been resolved. Show all issues
```
file is a type in Python.
```
reviewboard/attachments/tests.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Unnecessary blank line
```
reviewboard/attachments/tests.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Unnecessary blank line
```
reviewboard/attachments/tests.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Unnecessary blank line
```
reviewboard/webapi/resources/base_file_attachment.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Undo this change.
```

chipx86 Oct. 27, 2014, 1:28 a.m.

This looks long, but a large amount of the comments are about excess blank lines you shouldn't have, and docstring issues.

reviewboard/attachments/forms.py (Diff revision 8)

The issue has been resolved. Show all issues

Should remove the extra blank line between the """ and the fields.

reviewboard/attachments/forms.py (Diff revision 8)

The issue has been resolved. Show all issues

We're doing this in multiple places now. We need to only have this logic in one place.

reviewboard/attachments/forms.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Should remove the extra blank line before the """.
```
reviewboard/attachments/forms.py (Diff revision 8)
The issue has been resolved. Show all issues
```
And the one after.
```
reviewboard/attachments/forms.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Remove this blank line.
```
reviewboard/attachments/forms.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Remove the blank line.
```
reviewboard/attachments/forms.py (Diff revision 8)
The issue has been resolved. Show all issues
```
No blank line here.
Same below right after a function definition.
```
reviewboard/attachments/forms.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Another case that should be split out.
```
reviewboard/attachments/models.py (Diff revision 8)
The issue has been resolved. Show all issues
```
As Barret, says, we don't use ternary operators.
```
reviewboard/attachments/models.py (Diff revision 8)
The issue has been resolved. Show all issues
```
No blank line.
```

reviewboard/attachments/models.py (Diff revision 8)

The issue has been resolved. Show all issues

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.

reviewboard/attachments/models.py (Diff revision 8)

The issue has been resolved. Show all issues

Should be combined to one statement.

You also need to check user.is_authorized().

dkus Oct. 27, 2014, 9:26 a.m.

What does user.is_authorized() do? I can't seem to find it being used anywhere.

chipx86 Oct. 27, 2014, 1:09 p.m.

Basically, if a user has a logged in session, is_authorized() will return True. Otherwise, False.
As a convenience, there's an is_anonymous() function that is the inverse of is_authorized().

reviewboard/attachments/models.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Should be one statement.
```
reviewboard/attachments/tests.py (Diff revision 8)
The issue has been resolved. Show all issues
```
No need for parens.
```
reviewboard/attachments/tests.py (Diff revision 8)
The issue has been resolved. Show all issues
```
No blank line.
```
reviewboard/attachments/tests.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Indentation issues.
```

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

The issue has been resolved. Show all issues

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.

dkus Oct. 27, 2014, 9:54 a.m.

So what should the URL look like? /users/<username>/attachments/<id>/ ?

chipx86 Oct. 27, 2014, 1:09 p.m.
```
That looks perfect!
```

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

The issue has been resolved. Show all issues

We use parens for multi-line conditionals.

As per the comment in the urls.py change, this should check the username in the URL.

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

The issue has been resolved. Show all issues

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.

reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revision 8)
The issue has been resolved. Show all issues
```
You probably also want user__isnull=True.
```

reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)

The issue has been resolved. Show all issues

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.

reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Alphabetical order.
```
reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
No blank line.
```

reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)

The issue has been resolved. Show all issues

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.

reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
I would call this UserFileAttachmentResource.
```

reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)

The issue has been resolved. Show all issues

You need to define a name. You can't reuse the other name.

dkus Oct. 27, 2014, 10:40 a.m.
```
Not sure what you mean by this.
```

chipx86 Oct. 27, 2014, 1:09 p.m.

Sorry, bad placement of the line and very unhelpful comment by me.

Each resource has a name that identifies it. It's used in the payload and in URI templates.

"file-attachments" is the name being inherited from the base class. That's currently being used to indicate actual standard file attachments on a review request. Because these are different, are in a different location, and have a different purpose, we need to use something different, like "user_file_attachment".

reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)

The issue has been resolved. Show all issues

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.

reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Missing a trailing period.
```

reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)

The issue has been resolved. Show all issues

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

reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)

The issue has been resolved. Show all issues

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.

reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
As per the URL stuff above, this will need to include the username.
```
reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
We need to use `build_server_url() with this, or it won't actually be absolute.
```
reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
No blank line.
```

reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)

The issue has been resolved. Show all issues

If local_site is None, then the two statements would be equivalent. You shouldn't need the conditional.

reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Blank line between these.
```

reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)

The issue has been resolved. Show all issues

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 return self.model.objects.none() otherwise, and should always have the user= 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.

dkus Oct. 27, 2014, 12:05 p.m.

I don't believe this was discussed. Which of these possiblities would be preferred? I don't think there is really a use for retrieving someone else's file attachments (I don't think the client-side stuff I'm writing even needs to call GET)

chipx86 Oct. 27, 2014, 1:09 p.m.

There may not be one right now, but depending on which way we go, we may limit what we can do in the future, since we try hard not to change API.
One thing to consider is that an attachment may be made on a private review request, and as such, it may be bad to allow people to find it. That's probably the most important factor that influences the design. So important, in fact, that we should probably address it now.
Let's have a discussion in IRC about this. Ping me whenever you're free and we'll try to get a consensus.

reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
There's no such thing :)
```
reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
No blank line.
```

reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)

The issue has been resolved. Show all issues

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.

reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
These can be combined.
```
reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
You shouldn't need to call serialize_object. Instead, you should just need to pass the object.
```
reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
No blank line.
```
reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Same as above.
```

reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)

The issue has been resolved. Show all issues

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.

reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
No blank line.
```
reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
user_file_attachment
```

reviewboard/webapi/tests/test_file_attachment_user.py (Diff revision 8)

The issue has been resolved. Show all issues

I don't think this is the right URL.

dkus Oct. 27, 2014, 11:46 a.m.

It was my understanding from the project definition that the API should be /api/file-attachments/<id>/.

chipx86 Oct. 27, 2014, 1:09 p.m.

If we're adding that, then that resource needs to handle all types of file attachments, including ones on a review request and ones associated with a diff, and needs to have a set of query parameters explicit enough to allow narrowing down by those types.

I'm fine doing this, but it does mean all payloads for all types of file attachments need to be identical. 'user', 'review_request', etc. all need to be present in BaseFileAttachmentResource.

reviewboard/webapi/tests/test_file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
These blank lines should go.
Same with all functions below.
```
reviewboard/webapi/tests/test_file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Space before \.
Same below.
```

reviewboard/webapi/tests/test_file_attachment_user.py (Diff revision 8)

The issue has been resolved. Show all issues

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.

dkus Oct. 27, 2014, 11:46 a.m.

Don't we want there to be some data in the database that won't be returned, to make sure it's only returning the data that we want? If we define these in the item list, then the only items in the database will be the ones we want.

chipx86 Oct. 27, 2014, 1:09 p.m.

Sure, that's totally fine. We don't need to store them as variables in that case, though, and we should just inline the ones we do want to return. That's what we do in other tests.

reviewboard/webapi/tests/test_file_attachment_user.py (Diff revision 8)
The issue has been resolved. Show all issues
```
See above with the info on docstrings.
Also, the URL is wrong.
```
reviewboard/webapi/tests/urls.py (Diff revision 8)
The issue has been resolved. Show all issues
```
user_file_attachment
```

Change Summary:

Lots of changes from reviews

Commit:

-	e846ed0fd70944ffbed117b66591832ebe808412
+	649a9588d354969069d40c48403b508b0cd39cf3

Diff:

Revision 9 (+1142 -339)

Show changes

	reviewboard/urls.py
	reviewboard/attachments/forms.py
	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/attachments/urls.py
	reviewboard/attachments/views.py
	reviewboard/attachments/evolutions/__init__.py
	14 more

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.

reviewboard/scmtools/testdata/hg_repo/.hg/cache/branchheads (Diff revision 9)

The issue has been resolved. Show all issues

This file was created after running a test and I accidentally added it. Should this be removed, and should there be a .gitignore for this?

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:

-	649a9588d354969069d40c48403b508b0cd39cf3
+	6d1f8f3f7c5716d0d56fda61c12d9c41d719fe2e

Diff:

Revision 10 (+1143 -339)

Show changes

	reviewboard/urls.py
	reviewboard/attachments/forms.py
	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/attachments/urls.py
	reviewboard/attachments/views.py
	reviewboard/attachments/evolutions/__init__.py
	13 more

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

Ship it!

```
Ship It!
```

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.

reviewboard/attachments/models.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Blank line between these.
```
reviewboard/attachments/models.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/attachments/models.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Same here.
```

reviewboard/attachments/urls.py (Diff revision 10)

The issue has been resolved. Show all issues

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

reviewboard/attachments/views.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Should have a docstring.
```

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

The issue has been resolved. Show all issues

This can go directly in the query above.
get_object_or_404(
    ...,
    local_site=local_site,
    file__isnull=False)

reviewboard/urls.py (Diff revision 10)

The issue has been resolved. Show all issues

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

...

reviewboard/webapi/resources/user_file_attachment.py (Diff revision 10)
The issue has been resolved. Show all issues
```
The blank line is on the wrong side of that import.
```

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

The issue has been resolved. Show all issues

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.

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

The issue has been resolved. Show all issues

"Handles" is probably not the best for the public docs.
"Uploads a file attachment associated with the user." would be better.

reviewboard/webapi/resources/user_file_attachment.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Similar comment here.
```

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

The issue has been resolved. Show all issues

Things like FileAttachment are internal and should not be exposed in any public docs.
Also, "Including the file."

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

The issue has been resolved. Show all issues

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.

reviewboard/webapi/resources/user_file_attachment.py (Diff revision 10)
The issue has been resolved. Show all issues
```
"Deletes a file attachment owned by the user."
```

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

The issue has been resolved. Show all issues

Again on here, we don't want to talk about models or internal classes.

Let's also say "administrator" instead of "superuser."

reviewboard/webapi/resources/user_file_attachment.py (Diff revision 10)
The issue has been resolved. Show all issues
```
This is true of all DELETE methods.
```

Change Summary:

Updating the API documentation and cleaning up the urls

Commit:

-	6d1f8f3f7c5716d0d56fda61c12d9c41d719fe2e
+	40f4f0696fef14dea7936ab4b5b740ae36f5741a

Diff:

Revision 11 (+1183 -341)

Show changes

	docs/manual/webapi/2.0/errors/229-file-already-exists.rst
	docs/manual/webapi/2.0/errors/index.rst
	reviewboard/urls.py
	reviewboard/attachments/forms.py
	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/attachments/views.py
	14 more

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)