Drag 'n Drop Inline Images Backend - WebAPI

Review Request #6617 — Created Nov. 21, 2014 and submitted

Information

Review Board
master

Reviewers

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

  • Creating a Web API for user file attachments, UserFileAttachment resource.
  • UserFileAttachment resource can be accessed at /api/users/<username>/file-attachments/<id>/
  • Users can only get/modify/delete their own UserFileAttachments. Superusers can get/modify any.
  • UserFileAttachments can be created (using POST) without an associated file, in this case the database entry will be created, and the view will return a 404.
  • UserFileAttachments can be modified (using PUT) to include a file. This can only be done to UserFileAttachments that do not already have an asosciated file.
  • When deleting a UserFileAttachment, the associated file is also deleted.

Also did some refactoring of the base_file_attachment Web API resource so that it could be extended by both user_file_attachment and review_request_file_attachment resources.

All unit tests pass after making the changes.

Extensive manual testing of UserFileAttachment Web API resource using Postman REST client:
- Testing each method (GET/POST/PUT/DELETE) for UserFileAttachment as a regular user and superuser.
- Testing local_site functionality

Added unit tests around the new UserFileAttachment WebAPI Resource.

Description From Last Updated

These two statements can be combined.

daviddavid

You can remove the "that is" part.

chipx86chipx86

This isn't your fault, but this is no longer true, given that the thumbnail can also be updated. Can you …

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

This can be one statement.

chipx86chipx86

user_file_attachment is local to this file, so no need to access it through something else.

chipx86chipx86

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

chipx86chipx86

The summary must fit on a single line. You can try: Returns information on a user's file attachment.

chipx86chipx86

Both of these are saying the same thing.

chipx86chipx86

This isn't strictly true. It's not expected, but rather, optional. Maybe: "If file data is provided, then it is expected …

chipx86chipx86

Some of this can use the has_list_access_permissions().

chipx86chipx86

I don't think you need to copy the dictionary.

chipx86chipx86

This can be: if 'path' in request.FILES ...:

chipx86chipx86

Shouldn't need to copy.

chipx86chipx86

""" on the next line. (Note the above comment about how summaries must be on a single line? Unit tests …

chipx86chipx86

No period at the end.

chipx86chipx86

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

mike_conleymike_conley

Nit - local_site_name is the first item in the rest of these url getters - might as well stick with …

mike_conleymike_conley
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/errors.py
        reviewboard/webapi/resources/file_attachment.py
        reviewboard/webapi/resources/base_review_request_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_user_file_attachment.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/resources/user_file_attachment.py
    
    Ignored Files:
        docs/manual/webapi/2.0/errors/229-file-already-exists.rst
        docs/manual/webapi/2.0/errors/index.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/draft_file_attachment.py
        reviewboard/webapi/resources/diff_file_attachment.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/resources/file_attachment.py
        reviewboard/webapi/resources/base_review_request_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_user_file_attachment.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/resources/user_file_attachment.py
    
    Ignored Files:
        docs/manual/webapi/2.0/errors/229-file-already-exists.rst
        docs/manual/webapi/2.0/errors/index.rst
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/testing/testcase.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    These two statements can be combined.

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

  2. reviewboard/webapi/errors.py (Diff revision 1)
     
     
    Show all issues

    You can remove the "that is" part.

  3. Show all issues

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

  4. Show all issues

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

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

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

  5. Show all issues

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

    How about:

    The file attachment is not tied to any particular review
    request, and instead is owned by a user for usage in
    Markdown-formatted text.
    
    The file contents are optional when first creating a file
    attachment. This is to allow a caller to create the attachment
    and get the resulting URL for embedding in a text field. The
    file's contents can then be added separately (and only once)
    in a PUT request.
    
  6. reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This can be one statement.

  7. reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)
     
     
     
     
     
     
     
    Show all issues

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

    return self.model.objects.filter(user=user, local_site=local_site)
    
  8. Show all issues

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

    Returns information on a user's file attachment.
    
  9. Show all issues

    Both of these are saying the same thing.

  10. reviewboard/webapi/resources/user_file_attachment.py (Diff revision 1)
     
     
     
     
     
    Show all issues

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

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

    Some of this can use the has_list_access_permissions().

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

    2. has_list_access_permissions() can only return True/False.

      There should never be a 500. That usually indicates an error with the cod. What does the traceback say?

    3. http://pastie.org/private/mwjbxwngsukqlx5ihcehq

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

  12. Show all issues

    I don't think you need to copy the dictionary.

  13. Show all issues

    This can be:

    if 'path' in request.FILES ...:
    
  14. Show all issues

    Shouldn't need to copy.

  15. Show all issues

    """ on the next line.

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

  16. Show all issues

    No period at the end.

  17. 
      
chipx86
  1. 
      
  2. Show all issues

    user_file_attachment is local to this file, so no need to access it through something else.

  3. 
      
dkus
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/draft_file_attachment.py
        reviewboard/webapi/resources/diff_file_attachment.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/resources/file_attachment.py
        reviewboard/webapi/resources/base_review_request_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_user_file_attachment.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/resources/user_file_attachment.py
    
    Ignored Files:
        docs/manual/webapi/2.0/errors/229-file-already-exists.rst
        docs/manual/webapi/2.0/errors/index.rst
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/draft_file_attachment.py
        reviewboard/webapi/resources/diff_file_attachment.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/resources/file_attachment.py
        reviewboard/webapi/resources/base_review_request_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_user_file_attachment.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/resources/user_file_attachment.py
    
    Ignored Files:
        docs/manual/webapi/2.0/errors/229-file-already-exists.rst
        docs/manual/webapi/2.0/errors/index.rst
    
    
  2. 
      
mike_conley
  1. This looks good to me - but you might want another mentor to kick the tires as well.

  2. reviewboard/testing/testcase.py (Diff revision 2)
     
     
    Show all issues

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

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

  3. reviewboard/webapi/tests/urls.py (Diff revision 2)
     
     
     
    Show all issues

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

  4. 
      
dkus
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/draft_file_attachment.py
        reviewboard/webapi/resources/diff_file_attachment.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/resources/file_attachment.py
        reviewboard/webapi/resources/base_review_request_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_user_file_attachment.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/resources/user_file_attachment.py
    
    Ignored Files:
        docs/manual/webapi/2.0/errors/229-file-already-exists.rst
        docs/manual/webapi/2.0/errors/index.rst
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/draft_file_attachment.py
        reviewboard/webapi/resources/diff_file_attachment.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/resources/file_attachment.py
        reviewboard/webapi/resources/base_review_request_file_attachment.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/tests/test_user_file_attachment.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/resources/user_file_attachment.py
    
    Ignored Files:
        docs/manual/webapi/2.0/errors/229-file-already-exists.rst
        docs/manual/webapi/2.0/errors/index.rst
    
    
  2. 
      
chipx86
  1. Ship It!
  2. 
      
dkus
Review request changed
Status:
Completed
Change Summary:
Pushed to ucosp/dkus/dnd-inline-images (2db233d)