Drag 'n Drop Inline Images Backend

Review Request #6454 — Created Oct. 17, 2014 and discarded

Information

Review Board
master

Reviewers

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

Description From Last Updated

Col: 1 E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

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

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

chipx86chipx86

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

chipx86chipx86

Should remove the extra blank line before the """.

chipx86chipx86

And the one after.

chipx86chipx86

Remove this blank line.

chipx86chipx86

Remove the blank line.

chipx86chipx86

No blank line here. Same below right after a function definition.

chipx86chipx86

Another case that should be split out.

chipx86chipx86

file is a type in Python, so this should be renamed to something like filename.

brenniebrennie

Same here.

brenniebrennie

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.

brenniebrennie

As Barret, says, we don't use ternary operators.

chipx86chipx86

Redo this to not use ternary?

brenniebrennie

Same here with regards to ternary and backslash

brenniebrennie

No blank line.

chipx86chipx86

Unnecessary blank line.

brenniebrennie

Docstrings are always in one of the following forms: """Single-line summary.""" or: """Single-line summary. Multi-line description. """ Also, "Used to …

chipx86chipx86

Unnecessary blank line.

brenniebrennie

Should be combined to one statement. You also need to check user.is_authorized().

chipx86chipx86

Unnecessary blank line.

brenniebrennie

Should be one statement.

chipx86chipx86

No need for parens.

chipx86chipx86

No blank line.

chipx86chipx86

Indentation issues.

chipx86chipx86

Unnecessary blank line.

brenniebrennie

Unnecessary blank line

brenniebrennie

file is a type in Python.

brenniebrennie

Unnecessary blank line

brenniebrennie

Unnecessary blank line

brenniebrennie

Unnecessary blank line

brenniebrennie

I think this is probably base as a URL off of /users/<username>/. The view should then take the username from …

chipx86chipx86

We use parens for multi-line conditionals. As per the comment in the urls.py change, this should check the username in …

chipx86chipx86

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 …

chipx86chipx86

Undo this change.

brenniebrennie

You probably also want user__isnull=True.

chipx86chipx86

These are all third-party modules, from the perspective of this project, so must be in the same group. Also, the …

chipx86chipx86

Alphabetical order.

chipx86chipx86

No blank line.

chipx86chipx86

This must have a docstring. The docstring will be directly used on the API docs, so it must be intended …

chipx86chipx86

I would call this UserFileAttachmentResource.

chipx86chipx86

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

chipx86chipx86

We should be setting up a link between this and the appropriate user resource, rather than simply serializing the username. …

chipx86chipx86

Missing a trailing period.

chipx86chipx86

The payloads are different from the file attachment payloads from other resources, so we can't use the same mimetypes. This …

chipx86chipx86

This will end up doing a database query per object, since we have to fetch the local_site relation. We already …

chipx86chipx86

As per the URL stuff above, this will need to include the username.

chipx86chipx86

We need to use `build_server_url() with this, or it won't actually be absolute.

chipx86chipx86

No blank line.

chipx86chipx86

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

chipx86chipx86

Blank line between these.

chipx86chipx86

This means an authenticated user will only ever be able to list their own attachments, and an anonymous user will …

chipx86chipx86

There's no such thing :)

chipx86chipx86

No blank line.

chipx86chipx86

Ah, I see where the above comes from. A better option is to instead just check if the user has …

chipx86chipx86

These can be combined.

chipx86chipx86

You shouldn't need to call serialize_object. Instead, you should just need to pass the object.

chipx86chipx86

No blank line.

chipx86chipx86

Same as above.

chipx86chipx86

Like the other docstrings, these are all going to be publicly shown on the website. Make sure to keep to …

chipx86chipx86

No blank line.

chipx86chipx86

user_file_attachment

chipx86chipx86

I don't think this is the right URL.

chipx86chipx86

These blank lines should go. Same with all functions below.

chipx86chipx86

Space before \. Same below.

chipx86chipx86

This is a lot of verbose and unnecessary work. Instead, just do what we do in test_file_attachment.py. Call the create …

chipx86chipx86

See above with the info on docstrings. Also, the URL is wrong.

chipx86chipx86

user_file_attachment

chipx86chipx86

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

dkusdkus

Blank line between these.

chipx86chipx86

Same here.

chipx86chipx86

Same here.

chipx86chipx86

A top-level urlpatterns indicates that the stuff will live pretty close to the root of the site. However, this is …

chipx86chipx86

Should have a docstring.

chipx86chipx86

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

chipx86chipx86

We define this prefix twice. What we can do to avoid that (and avoid introducing problems if we ever have …

chipx86chipx86

The blank line is on the wrong side of that import.

chipx86chipx86

This needs to be fleshed out. This docstring is what will be publicly shown in the API docs on our …

chipx86chipx86

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

chipx86chipx86

Similar comment here.

chipx86chipx86

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

chipx86chipx86

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> …

chipx86chipx86

"Deletes a file attachment owned by the user."

chipx86chipx86

Again on here, we don't want to talk about models or internal classes. Let's also say "administrator" instead of "superuser."

chipx86chipx86

This is true of all DELETE methods.

chipx86chipx86
reviewbot
  1. 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
    
    
  2. Show all issues
    Col: 1
     E101 indentation contains mixed spaces and tabs
    
  3. Show all issues
    Col: 17
     E126 continuation line over-indented for hanging indent
    
  4. 
      
dkus
reviewbot
  1. 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
    
    
  2. 
      
dkus
reviewbot
  1. 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
    
    
  2. reviewboard/attachments/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
dkus
reviewbot
  1. 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
    
    
  2. 
      
dkus
reviewbot
  1. 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
    
    
  2. Show all issues
    Col: 17
     E126 continuation line over-indented for hanging indent
    
  3. Show all issues
    Col: 29
     E126 continuation line over-indented for hanging indent
    
  4. Show all issues
    Col: 17
     E126 continuation line over-indented for hanging indent
    
  5. 
      
dkus
reviewbot
  1. 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
    
    
  2. Show all issues
    Col: 17
     E126 continuation line over-indented for hanging indent
    
  3. Show all issues
    Col: 29
     E126 continuation line over-indented for hanging indent
    
  4. Show all issues
    Col: 17
     E126 continuation line over-indented for hanging indent
    
  5. 
      
dkus
reviewbot
  1. 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
    
    
  2. Show all issues
    Col: 17
     E126 continuation line over-indented for hanging indent
    
    1. 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.

  3. 
      
dkus
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/attachments/mimetypes.py (Diff revision 8)
     
     
    Show all issues

    file is a type in Python, so this should be renamed to something like filename.

  3. reviewboard/attachments/mimetypes.py (Diff revision 8)
     
     
    Show all issues

    Same here.

  4. reviewboard/attachments/models.py (Diff revision 8)
     
     
     
    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.

  5. reviewboard/attachments/models.py (Diff revision 8)
     
     
    Show all issues

    Redo this to not use ternary?

  6. reviewboard/attachments/models.py (Diff revision 8)
     
     
     
    Show all issues

    Same here with regards to ternary and backslash

  7. reviewboard/attachments/models.py (Diff revision 8)
     
     
    Show all issues

    Unnecessary blank line.

  8. reviewboard/attachments/models.py (Diff revision 8)
     
     
    Show all issues

    Unnecessary blank line.

  9. reviewboard/attachments/models.py (Diff revision 8)
     
     
    Show all issues

    Unnecessary blank line.

  10. reviewboard/attachments/tests.py (Diff revision 8)
     
     
    Show all issues

    Unnecessary blank line.

  11. reviewboard/attachments/tests.py (Diff revision 8)
     
     
    Show all issues

    Unnecessary blank line

  12. reviewboard/attachments/tests.py (Diff revision 8)
     
     
    Show all issues

    file is a type in Python.

  13. reviewboard/attachments/tests.py (Diff revision 8)
     
     
    Show all issues

    Unnecessary blank line

  14. reviewboard/attachments/tests.py (Diff revision 8)
     
     
    Show all issues

    Unnecessary blank line

  15. reviewboard/attachments/tests.py (Diff revision 8)
     
     
    Show all issues

    Unnecessary blank line

  16. Show all issues

    Undo this change.

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

  2. reviewboard/attachments/forms.py (Diff revision 8)
     
     
     
     
     
     
    Show all issues

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

  3. reviewboard/attachments/forms.py (Diff revision 8)
     
     
    Show all issues

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

  4. reviewboard/attachments/forms.py (Diff revision 8)
     
     
     
     
     
     
    Show all issues

    Should remove the extra blank line before the """.

  5. reviewboard/attachments/forms.py (Diff revision 8)
     
     
     
     
     
     
    Show all issues

    And the one after.

  6. reviewboard/attachments/forms.py (Diff revision 8)
     
     
     
     
    Show all issues

    Remove this blank line.

  7. reviewboard/attachments/forms.py (Diff revision 8)
     
     
     
     
    Show all issues

    Remove the blank line.

  8. reviewboard/attachments/forms.py (Diff revision 8)
     
     
     
     
     
    Show all issues

    No blank line here.

    Same below right after a function definition.

  9. reviewboard/attachments/forms.py (Diff revision 8)
     
     
    Show all issues

    Another case that should be split out.

  10. reviewboard/attachments/models.py (Diff revision 8)
     
     
     
    Show all issues

    As Barret, says, we don't use ternary operators.

  11. reviewboard/attachments/models.py (Diff revision 8)
     
     
     
     
    Show all issues

    No blank line.

  12. reviewboard/attachments/models.py (Diff revision 8)
     
     
     
    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.

  13. reviewboard/attachments/models.py (Diff revision 8)
     
     
     
     
     
     
    Show all issues

    Should be combined to one statement.

    You also need to check user.is_authorized().

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

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

  14. reviewboard/attachments/models.py (Diff revision 8)
     
     
     
     
     
     
     
     
     
    Show all issues

    Should be one statement.

  15. reviewboard/attachments/tests.py (Diff revision 8)
     
     
    Show all issues

    No need for parens.

  16. reviewboard/attachments/tests.py (Diff revision 8)
     
     
     
     
    Show all issues

    No blank line.

  17. reviewboard/attachments/tests.py (Diff revision 8)
     
     
     
     
    Show all issues

    Indentation issues.

  18. reviewboard/attachments/urls.py (Diff revision 8)
     
     
     
     
    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.

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

    2. That looks perfect!

  19. reviewboard/attachments/views.py (Diff revision 8)
     
     
     
     
    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.

  20. reviewboard/testing/testcase.py (Diff revision 8)
     
     
    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.

  21. Show all issues

    You probably also want user__isnull=True.

  22. reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
     
     
     
     
     
     
     
     
     
     
    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.

  23. Show all issues

    Alphabetical order.

  24. Show all issues

    No blank line.

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

  26. Show all issues

    I would call this UserFileAttachmentResource.

  27. Show all issues

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

    1. Not sure what you mean by this.

    2. 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".

  28. reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
     
     
     
     
     
    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.

  29. Show all issues

    Missing a trailing period.

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

  31. reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
     
     
     
     
     
    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.

  32. Show all issues

    As per the URL stuff above, this will need to include the username.

  33. Show all issues

    We need to use `build_server_url() with this, or it won't actually be absolute.

  34. Show all issues

    No blank line.

  35. reviewboard/webapi/resources/file_attachment_user.py (Diff revision 8)
     
     
     
     
     
     
    Show all issues

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

  36. Show all issues

    Blank line between these.

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

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

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

  38. Show all issues

    There's no such thing :)

  39. Show all issues

    No blank line.

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

  41. Show all issues

    These can be combined.

  42. Show all issues

    You shouldn't need to call serialize_object. Instead, you should just need to pass the object.

  43. Show all issues

    No blank line.

  44. Show all issues

    Same as above.

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

  46. Show all issues

    No blank line.

  47. Show all issues

    user_file_attachment

  48. Show all issues

    I don't think this is the right URL.

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

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

  49. reviewboard/webapi/tests/test_file_attachment_user.py (Diff revision 8)
     
     
     
     
     
     
    Show all issues

    These blank lines should go.

    Same with all functions below.

  50. Show all issues

    Space before \.

    Same below.

  51. reviewboard/webapi/tests/test_file_attachment_user.py (Diff revision 8)
     
     
     
     
     
     
     
     
     
    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.

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

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

  52. Show all issues

    See above with the info on docstrings.

    Also, the URL is wrong.

  53. reviewboard/webapi/tests/urls.py (Diff revision 8)
     
     
     
     
     
     
     
    Show all issues

    user_file_attachment

  54. 
      
dkus
reviewbot
  1. 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
    
    
  2. 
      
dkus
dkus
  1. 
      
  2. 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?

  3. 
      
dkus
reviewbot
  1. 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
    
    
  2. 
      
RM
  1. Ship It!
  2. 
      
chipx86
  1. 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:

    1. The model changes to FileAttachment + evolution.
    2. The reorganization of code for the file attachment forms, mimetype stuff, etc.
    3. The redirect view and URL changes.
    4. 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.

  2. reviewboard/attachments/models.py (Diff revision 10)
     
     
     
    Show all issues

    Blank line between these.

  3. reviewboard/attachments/models.py (Diff revision 10)
     
     
     
    Show all issues

    Same here.

  4. reviewboard/attachments/models.py (Diff revision 10)
     
     
     
    Show all issues

    Same here.

  5. reviewboard/attachments/urls.py (Diff revision 10)
     
     
     
     
     
     
     
     
    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.)

  6. reviewboard/attachments/views.py (Diff revision 10)
     
     
     
     
     
    Show all issues

    Should have a docstring.

  7. reviewboard/attachments/views.py (Diff revision 10)
     
     
     
    Show all issues

    This can go directly in the query above.

    get_object_or_404(
        ...,
        local_site=local_site,
        file__isnull=False)
    
  8. reviewboard/urls.py (Diff revision 10)
     
     
     
     
     
     
     
    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),
    
    ...
    
  9. reviewboard/webapi/resources/user_file_attachment.py (Diff revision 10)
     
     
     
     
     
    Show all issues

    The blank line is on the wrong side of that import.

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

  11. Show all issues

    "Handles" is probably not the best for the public docs.

    "Uploads a file attachment associated with the user." would be better.

  12. Show all issues

    Similar comment here.

  13. Show all issues

    Things like FileAttachment are internal and should not be exposed in any public docs.

    Also, "Including the file."

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

  15. Show all issues

    "Deletes a file attachment owned by the user."

  16. 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."

  17. Show all issues

    This is true of all DELETE methods.

  18. 
      
dkus
reviewbot
  1. 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
    
    
  2. 
      
dkus
Review request changed
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)