Summary

Allow updating file attachments.

Review Request #6387 — Created Sept. 30, 2014 and submitted Oct. 14, 2014, 11:56 a.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

800

Depends On

Commit

b775166...

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done


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.

Issues

Description	From	Last Updated
Col: 25 E127 continuation line over-indented for visual indent	reviewbot	Sept. 30, 2014, 2:25 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Sept. 30, 2014, 2:26 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Sept. 30, 2014, 2:26 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Sept. 30, 2014, 2:26 p.m.
Might be better as a ModelChoiceField, since that will handle validation for us.	chipx86	Oct. 1, 2014, 3:28 p.m.
What happens if we have two uploading simultaneously? Perhaps we should do something like the local_id calculation for ReviewRequests?	chipx86	Sept. 30, 2014, 5:23 p.m.
pk= If we use a ModelChoiceField above, we'll just have the instance here.	chipx86	Oct. 1, 2014, 3:28 p.m.
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()	chipx86	Oct. 1, 2014, 3:35 p.m.
What's the result if we don't do the first exists()? Would be nice to get this down to one query …	chipx86	Oct. 1, 2014, 3:51 p.m.
Can this be a single create() call?	chipx86	Oct. 1, 2014, 4:08 p.m.
Should save(update_fields['attachment_history'])	chipx86	Oct. 1, 2014, 4:09 p.m.
Should check against attachment_history_id.	chipx86	Oct. 1, 2014, 4:10 p.m.
This could be turned into a list comprehension pretty easily: latest_file_attachments = [ file_attachment for file_attachment in file_attachments if (not …	chipx86	Oct. 1, 2014, 5:43 p.m.
Since we're doing this twice, let's pull it out into a util function.	chipx86	Oct. 1, 2014, 5:43 p.m.
We use ID elsewhere.	chipx86	Oct. 1, 2014, 4:12 p.m.
If we're going to do this form, we should indent the this.get(...). I'd prefer just moving the expression to the …	chipx86	Oct. 1, 2014, 4:12 p.m.
We should do this first, in case there's an exception when creating the view.	chipx86	Oct. 1, 2014, 4:13 p.m.
I don't see where we're actually allowing this to be set.	chipx86	Oct. 1, 2014, 4:25 p.m.
Should check against attachment_history_id.	chipx86	Oct. 1, 2014, 4:26 p.m.
We can save on a query with: attachments = FileAttachment.objects.filter( attachment_history=file_attachment.attachment_history_id)	chipx86	Oct. 1, 2014, 4:26 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Oct. 2, 2014, 11:01 a.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Oct. 2, 2014, 11:02 a.m.
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	Oct. 2, 2014, 11:03 a.m.
Col: 85 E251 unexpected spaces around keyword / parameter equals	reviewbot	Oct. 2, 2014, 11:03 a.m.
Col: 25 E128 continuation line under-indented for visual indent	reviewbot	Oct. 2, 2014, 11:03 a.m.
Col: 5 E124 closing bracket does not match visual indentation	reviewbot	Oct. 2, 2014, 11:04 a.m.
Col: 29 E131 continuation line unaligned for hanging indent	reviewbot	Oct. 2, 2014, 11:06 a.m.
We only care about the ID and latest_revision, and not the other fields or the deserialized object, so let's use …	chipx86	Oct. 2, 2014, 1:21 p.m.
history.pk	chipx86	Oct. 2, 2014, 1:22 p.m.
Col: 29 E131 continuation line unaligned for hanging indent	reviewbot	Oct. 2, 2014, 2:13 p.m.
Sicne this is a ChoiceField now, and we get a FileAttachmentHistory, we should probably remove the _id suffix.	chipx86	Oct. 2, 2014, 4:13 p.m.
I think if I were seeing this error and didn't know the code, I'd be confused. How about: The FileAttachmentHistory …	chipx86	Oct. 2, 2014, 4:11 p.m.
""" on the next line.	chipx86	Oct. 2, 2014, 4:11 p.m.
This is a different condition, so it should have its own test.	chipx86	Oct. 2, 2014, 4:13 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Oct. 2, 2014, 3:51 p.m.
Col: 29 E131 continuation line unaligned for hanging indent	reviewbot	Oct. 2, 2014, 3:51 p.m.
No need for the outer parens in JavaScript.	chipx86	Oct. 2, 2014, 4:13 p.m.
ValidationError is going to trigger a HTTP 500. We need to check for that.	chipx86	Oct. 2, 2014, 4:12 p.m.
Col: 29 E131 continuation line unaligned for hanging indent	reviewbot	Oct. 2, 2014, 5:15 p.m.

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

reviewboard/attachments/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 25
 E127 continuation line over-indented for visual indent
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/webapi/resources/base_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/webapi/resources/base_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Commit:

-	6ad07cfbdc021d429a149ad43521fe5ead999a4d
+	e564e34798ac738a5fb19a0456784611cba10ef7

Diff:

Revision 2 (+163 -32)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
	reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
	reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
	reviewboard/webapi/resources/base_file_attachment.py

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

Change Summary:

Attach this to an old enhancement request.

Bugs:

+	800

reviewboard/attachments/forms.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Might be better as a ModelChoiceField, since that will handle validation for us.
```

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

The issue has been dropped. Show all issues

What happens if we have two uploading simultaneously?

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

david Oct. 1, 2014, 11 p.m.

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

reviewboard/attachments/forms.py (Diff revision 2)
The issue has been resolved. Show all issues
```
pk=
If we use a ModelChoiceField above, we'll just have the instance here.
```

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

The issue has been resolved. 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()

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

The issue has been resolved. 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.

reviewboard/reviews/models/base_review_request_details.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can this be a single create() call?
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should save(update_fields['attachment_history'])
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should check against attachment_history_id.
```

reviewboard/reviews/views.py (Diff revision 2)

The issue has been resolved. 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]

reviewboard/reviews/views.py (Diff revision 2)

The issue has been resolved. Show all issues

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

reviewboard/static/rb/js/resources/models/fileAttachmentModel.js (Diff revision 2)
The issue has been resolved. Show all issues
```
We use ID elsewhere.
```

reviewboard/static/rb/js/resources/models/fileAttachmentModel.js (Diff revision 2)

The issue has been resolved. 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.

reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
We should do this first, in case there's an exception when creating the view.
```

reviewboard/webapi/resources/base_file_attachment.py (Diff revision 2)

The issue has been dropped. Show all issues

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

david Oct. 1, 2014, 11 p.m.

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

reviewboard/webapi/resources/base_file_attachment.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should check against attachment_history_id.
```

reviewboard/webapi/resources/base_file_attachment.py (Diff revision 2)

The issue has been resolved. Show all issues

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

Commit:

-	e564e34798ac738a5fb19a0456784611cba10ef7
+	a12addfc0f022a4e46d3318aeee371c0d60c9768

Diff:

Revision 3 (+184 -38)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
	reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
	reviewboard/static/rb/js/views/uploadAttachmentView.js
	reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
	1 more

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

reviewboard/attachments/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/attachments/models.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (84 > 79 characters)
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 85
 E251 unexpected spaces around keyword / parameter equals
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 25
 E128 continuation line under-indented for visual indent
```
reviewboard/reviews/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 5
 E124 closing bracket does not match visual indentation
```

Commit:

-	a12addfc0f022a4e46d3318aeee371c0d60c9768
+	259d034c3324bcf6c3a84768758d922e3ef2c038

Diff:

Revision 4 (+187 -38)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
	reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
	reviewboard/static/rb/js/views/uploadAttachmentView.js
	reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
	1 more

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

reviewboard/reviews/models/base_review_request_details.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 29
 E131 continuation line unaligned for hanging indent
```

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

reviewboard/reviews/views.py (Diff revision 4)

The issue has been resolved. 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.

david Oct. 2, 2014, 12:53 p.m.

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

chipx86 Oct. 2, 2014, 12:54 p.m.

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

reviewboard/reviews/views.py (Diff revision 4)
The issue has been dropped. Show all issues
```
history.pk
```

Change Summary:

Update to use latest_revision field. More unit testing is coming in a future update.

Commit:

-	259d034c3324bcf6c3a84768758d922e3ef2c038
+	b2a6ec41c92e1cd9d542c6075d2eede66ef915ad

Diff:

Revision 5 (+189 -38)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
	reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
	reviewboard/static/rb/js/views/uploadAttachmentView.js
	reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
	1 more

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

reviewboard/reviews/models/base_review_request_details.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 29
 E131 continuation line unaligned for hanging indent
```

Change Summary:

Add unit testing for revisions and display position, refactor UploadFileForm a bit.

Description:

		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.

Commit:

-	b2a6ec41c92e1cd9d542c6075d2eede66ef915ad
+	e8e3c2aaf1e5fa513c8a024945956283eb418fb0

Diff:

Revision 6 (+311 -57)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/reviews/context.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
	reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
	4 more

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

reviewboard/attachments/tests.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 6)
The issue has been dropped. Show all issues
```
Col: 29
 E131 continuation line unaligned for hanging indent
```

We also need unit tests for the API, to check success and error conditions.

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

The issue has been resolved. Show all issues

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

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

The issue has been resolved. 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?

reviewboard/attachments/tests.py (Diff revision 6)
The issue has been resolved. Show all issues
```
""" on the next line.
```
reviewboard/attachments/tests.py (Diff revision 6)
The issue has been resolved. Show all issues
```
This is a different condition, so it should have its own test.
```
reviewboard/static/rb/js/resources/models/fileAttachmentModel.js (Diff revision 6)
The issue has been resolved. Show all issues
```
No need for the outer parens in JavaScript.
```

reviewboard/webapi/resources/base_file_attachment.py (Diff revision 6)

The issue has been dropped. Show all issues

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

david Oct. 2, 2014, 4:28 p.m.

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

Change Summary:

Make requested changes, add API unit tests.

Commit:

-	e8e3c2aaf1e5fa513c8a024945956283eb418fb0
+	b775166736661d7b4b9ff76a18bd23fffd59b6f8

Diff:

Revision 7 (+402 -59)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/reviews/context.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/static/rb/js/resources/models/fileAttachmentModel.js
	reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
	5 more

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

reviewboard/reviews/models/base_review_request_details.py (Diff revision 7)
The issue has been dropped. Show all issues
```
Col: 29
 E131 continuation line unaligned for hanging indent
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (c866259)