Allow updating file attachments.

Review Request #6387 — Created Sept. 30, 2014 and submitted

Information

Review Board
master
800
b775166...

Reviewers

This change adds a new model, FileAttachmentHistory, which aggregates multiple
FileAttachments together into a single unit. Within a history, each file
attachment has a revision. Any "legacy" attachments will be associated with a
new FileAttachmentHistory when the review request page is shown. The history
also contains a "display_position" field, which may eventually be extended to
allow users to re-order the list of attachments on the page.

This also adds an "Update" button to file attachments in the "Actions"
drop-down menu. When clicking this, users will be presented with an upload
dialog allowing them to upload a new file. When a public attachment is updated,
it adds a new revision. When a draft attachment is updated, it is replaced with
the new file.

While I was adding tests and validation to the UploadFileForm, I did some
refactoring. The review request is now passed in as part of the constructor,
which aids in validation. Additionally, I've made it just use self.files (part
of the django Form class) instead of needing to pass in the file a second time
to the create() method.

  • Loaded a review request with existing file attachments and saw that new
    FileAttachmentHistory entries were created for each one.
  • Updated existing file attachments and saw that the new ones were created,
    visually replaced the ones that they updated, and that publishing the draft
    created the correct change description.
  • Updated a file attachment multiple times without publishing the draft and saw
    that it replaced the new revision rather than creating multiple revisions.
  • Checked a file attachment that had not yet been published and saw that it
    did not have the "Update" action.
  • Ran unit tests.
Description From Last Updated

Col: 25 E127 continuation line over-indented for visual indent

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Might be better as a ModelChoiceField, since that will handle validation for us.

chipx86chipx86

What happens if we have two uploading simultaneously? Perhaps we should do something like the local_id calculation for ReviewRequests?

chipx86chipx86

pk= If we use a ModelChoiceField above, we'll just have the instance here.

chipx86chipx86

Might be good to have a Meta.get_latest_by = 'attachment_revision set for FileAttachment. Then we can just do: attachment_history.file_attachments.latest()

chipx86chipx86

What's the result if we don't do the first exists()? Would be nice to get this down to one query …

chipx86chipx86

Can this be a single create() call?

chipx86chipx86

Should save(update_fields['attachment_history'])

chipx86chipx86

Should check against attachment_history_id.

chipx86chipx86

This could be turned into a list comprehension pretty easily: latest_file_attachments = [ file_attachment for file_attachment in file_attachments if (not …

chipx86chipx86

Since we're doing this twice, let's pull it out into a util function.

chipx86chipx86

We use ID elsewhere.

chipx86chipx86

If we're going to do this form, we should indent the this.get(...). I'd prefer just moving the expression to the …

chipx86chipx86

We should do this first, in case there's an exception when creating the view.

chipx86chipx86

I don't see where we're actually allowing this to be set.

chipx86chipx86

Should check against attachment_history_id.

chipx86chipx86

We can save on a query with: attachments = FileAttachment.objects.filter( attachment_history=file_attachment.attachment_history_id)

chipx86chipx86

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

reviewbotreviewbot

Col: 13 E128 continuation line under-indented for visual indent

reviewbotreviewbot

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

reviewbotreviewbot

Col: 85 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 25 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 5 E124 closing bracket does not match visual indentation

reviewbotreviewbot

Col: 29 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

We only care about the ID and latest_revision, and not the other fields or the deserialized object, so let's use …

chipx86chipx86

history.pk

chipx86chipx86

Col: 29 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

Sicne this is a ChoiceField now, and we get a FileAttachmentHistory, we should probably remove the _id suffix.

chipx86chipx86

I think if I were seeing this error and didn't know the code, I'd be confused. How about: The FileAttachmentHistory …

chipx86chipx86

""" on the next line.

chipx86chipx86

This is a different condition, so it should have its own test.

chipx86chipx86

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

reviewbotreviewbot

Col: 29 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

No need for the outer parens in JavaScript.

chipx86chipx86

ValidationError is going to trigger a HTTP 500. We need to check for that.

chipx86chipx86

Col: 29 E131 continuation line unaligned for hanging indent

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
        reviewboard/webapi/resources/base_file_attachment.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
        reviewboard/webapi/resources/base_file_attachment.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    
    
  2. reviewboard/attachments/models.py (Diff revision 1)
     
     
    Show all issues
    Col: 25
     E127 continuation line over-indented for visual indent
    
  3. Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  5. Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  6. 
      
david
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
        reviewboard/webapi/resources/base_file_attachment.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
        reviewboard/webapi/resources/base_file_attachment.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    
    
  2. 
      
david
chipx86
  1. 
      
  2. reviewboard/attachments/forms.py (Diff revision 2)
     
     
    Show all issues

    Might be better as a ModelChoiceField, since that will handle validation for us.

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

    What happens if we have two uploading simultaneously?

    Perhaps we should do something like the local_id calculation for ReviewRequests?

    1. I don't think it's crucial to jump through hoops here. This is just used as the sort key, and the sorting algorithm should be stable against the list of returned results, so we won't see anything jumping around (plus it's highly unlikely that two file attachments will be simultaneously uploaded against the same review request).

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

    pk=

    If we use a ModelChoiceField above, we'll just have the instance here.

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

    Might be good to have a Meta.get_latest_by = 'attachment_revision set for FileAttachment. Then we can just do:

    attachment_history.file_attachments.latest()
    
  6. reviewboard/attachments/models.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    What's the result if we don't do the first exists()? Would be nice to get this down to one query to help prevent race conditions and query counts.

    Also, why isn't the display position tied to a review request? Having it be global doesn't seem right.

  7. Show all issues

    Can this be a single create() call?

  8. Show all issues

    Should save(update_fields['attachment_history'])

  9. Show all issues

    Should check against attachment_history_id.

  10. reviewboard/reviews/views.py (Diff revision 2)
     
     
     
     
     
     
     
    Show all issues

    This could be turned into a list comprehension pretty easily:

    latest_file_attachments = [
        file_attachment
        for file_attachment in file_attachments
        if (not file_attachment.is_from_diff and
            file_attachment.attachment_revision ==
                file_attachment.attachment_history.latest_revision)
    ]
    

    I want to go a step further, though, and first pull out all the FileAttachmentHistory instances, map those from ID -> latest revision, and use those. Otherwise, we have a query per FileAttachment:

    file_attachment_histories = FileAttachmentHistory.objects.filter(
        file_attachment__in=file_attachments)
    latest_attachment_revisions = dict([
        (data['id'], data['latest_revision'])
        for data in file_attachment_histories.values('id', 'latest_revision')
    ])
    

    Then the above check just becomes:

    file_attachment.attachment_revision ==
        latest_attachment_revisions[file_attachment.attachment_history_id]
    
  11. reviewboard/reviews/views.py (Diff revision 2)
     
     
     
     
     
     
     
    Show all issues

    Since we're doing this twice, let's pull it out into a util function.

  12. Show all issues

    We use ID elsewhere.

  13. Show all issues

    If we're going to do this form, we should indent the this.get(...). I'd prefer just moving the expression to the next line, though.

  14. Show all issues

    We should do this first, in case there's an exception when creating the view.

  15. reviewboard/webapi/resources/base_file_attachment.py (Diff revision 2)
     
     
     
     
     
     
     
    Show all issues

    I don't see where we're actually allowing this to be set.

    1. It's part of the form data (for the UploadFileForm).

  16. Show all issues

    Should check against attachment_history_id.

  17. Show all issues

    We can save on a query with:

    attachments = FileAttachment.objects.filter(
        attachment_history=file_attachment.attachment_history_id)
    
  18. 
      
david
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
        reviewboard/webapi/resources/base_file_attachment.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
        reviewboard/webapi/resources/base_file_attachment.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    
    
  2. reviewboard/attachments/forms.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/attachments/models.py (Diff revision 3)
     
     
    Show all issues
    Col: 13
     E128 continuation line under-indented for visual indent
    
  4. Show all issues
    Col: 80
     E501 line too long (84 > 79 characters)
    
  5. Show all issues
    Col: 85
     E251 unexpected spaces around keyword / parameter equals
    
  6. Show all issues
    Col: 25
     E128 continuation line under-indented for visual indent
    
  7. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
    Col: 5
     E124 closing bracket does not match visual indentation
    
  8. 
      
david
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
        reviewboard/webapi/resources/base_file_attachment.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
        reviewboard/webapi/resources/base_file_attachment.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    
    
  2. Show all issues
    Col: 29
     E131 continuation line unaligned for hanging indent
    
  3. 
      
chipx86
  1. So the code looks fine, but it seems that this doesn't do any validation of the user-provided history ID, meaning a user could easily do a bunch of POSTs with history IDs, sending malicious content, and it would appear in different review requests.

    I think all we need is to have the form validate an existing history ID against the provided review request, and return an appropriate error in the API when an invalid ID is provided.

    We should also have unit tests covering that, and probably other aspects of this change (getting lists of attachments, calculating positions, passing valid/invalid/no history IDs to the form).

  2. reviewboard/reviews/views.py (Diff revision 4)
     
     
     
     
    Show all issues

    We only care about the ID and latest_revision, and not the other fields or the deserialized object, so let's use values() (like in my earlier review) to keep the work to a minimum here.

    1. latest_revision is not a value that comes from the database, so we can't do that.

    2. Maybe it should be. That'll speed things up a bit. We can probably use a RelationCounterField for this.

  3. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues

    history.pk

  4. 
      
david
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
        reviewboard/webapi/resources/base_file_attachment.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
        reviewboard/webapi/resources/base_file_attachment.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    
    
  2. Show all issues
    Col: 29
     E131 continuation line unaligned for hanging indent
    
  3. 
      
david
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/draft_filediff.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
        reviewboard/reviews/context.py
        reviewboard/attachments/tests.py
        reviewboard/reviews/models/base_review_request_details.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/draft_filediff.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
        reviewboard/reviews/context.py
        reviewboard/attachments/tests.py
        reviewboard/reviews/models/base_review_request_details.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    
    
  2. reviewboard/attachments/tests.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. Show all issues
    Col: 29
     E131 continuation line unaligned for hanging indent
    
  4. 
      
chipx86
  1. We also need unit tests for the API, to check success and error conditions.

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

    Sicne this is a ChoiceField now, and we get a FileAttachmentHistory, we should probably remove the _id suffix.

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

    I think if I were seeing this error and didn't know the code, I'd be confused. How about:

    The FileAttachmentHistory provided is not part of this review request.
    

    Or something?

  4. reviewboard/attachments/tests.py (Diff revision 6)
     
     
    Show all issues

    """ on the next line.

  5. reviewboard/attachments/tests.py (Diff revision 6)
     
     
    Show all issues

    This is a different condition, so it should have its own test.

  6. Show all issues

    No need for the outer parens in JavaScript.

  7. Show all issues

    ValidationError is going to trigger a HTTP 500. We need to check for that.

    1. That case is handled above in the form.is_valid() check.

  8. 
      
david
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/draft_filediff.py
        reviewboard/attachments/models.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/attachments/forms.py
        reviewboard/reviews/context.py
        reviewboard/attachments/tests.py
        reviewboard/reviews/models/base_review_request_details.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/webapi/resources/draft_filediff.py
        reviewboard/attachments/models.py
        reviewboard/webapi/tests/test_file_attachment.py
        reviewboard/attachments/forms.py
        reviewboard/reviews/context.py
        reviewboard/attachments/tests.py
        reviewboard/reviews/models/base_review_request_details.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
        reviewboard/static/rb/js/views/uploadAttachmentView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    
    
  2. Show all issues
    Col: 29
     E131 continuation line unaligned for hanging indent
    
  3. 
      
chipx86
  1. Ship It!

  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to master (c866259)