Drag 'n Drop Inline Images Backend - WebAPI

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

dkus
Review Board
master
reviewboard, students

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.

  • 0
  • 0
  • 18
  • 1
  • 19
Description From Last Updated
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)
     
     
     
     
     
     
     
     
     
     
     
     

    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)
     
     

    You can remove the "that is" part.

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

    This can be one statement.

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

    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. The summary must fit on a single line. You can try:

    Returns information on a user's file attachment.
    
  9. Both of these are saying the same thing.

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

    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. 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. I don't think you need to copy the dictionary.

  13. This can be:

    if 'path' in request.FILES ...:
    
  14. Shouldn't need to copy.

  15. """ on the next line.

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

  16. No period at the end.

  17. 
      
chipx86
  1. 
      
  2. 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)
     
     

    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)
     
     
     

    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: Closed (submitted)

Change Summary:

Pushed to ucosp/dkus/dnd-inline-images (2db233d)
Loading...