Allow updating file attachments.
Review Request #6387 — Created Sept. 30, 2014 and submitted
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 |
reviewbot | |
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot | |
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot | |
Might be better as a ModelChoiceField, since that will handle validation for us. |
chipx86 | |
What happens if we have two uploading simultaneously? Perhaps we should do something like the local_id calculation for ReviewRequests? |
chipx86 | |
pk= If we use a ModelChoiceField above, we'll just have the instance here. |
chipx86 | |
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 | |
What's the result if we don't do the first exists()? Would be nice to get this down to one query … |
chipx86 | |
Can this be a single create() call? |
chipx86 | |
Should save(update_fields['attachment_history']) |
chipx86 | |
Should check against attachment_history_id. |
chipx86 | |
This could be turned into a list comprehension pretty easily: latest_file_attachments = [ file_attachment for file_attachment in file_attachments if (not … |
chipx86 | |
Since we're doing this twice, let's pull it out into a util function. |
chipx86 | |
We use ID elsewhere. |
chipx86 | |
If we're going to do this form, we should indent the this.get(...). I'd prefer just moving the expression to the … |
chipx86 | |
We should do this first, in case there's an exception when creating the view. |
chipx86 | |
I don't see where we're actually allowing this to be set. |
chipx86 | |
Should check against attachment_history_id. |
chipx86 | |
We can save on a query with: attachments = FileAttachment.objects.filter( attachment_history=file_attachment.attachment_history_id) |
chipx86 | |
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot | |
Col: 13 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 80 E501 line too long (84 > 79 characters) |
reviewbot | |
Col: 85 E251 unexpected spaces around keyword / parameter equals |
reviewbot | |
Col: 25 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 5 E124 closing bracket does not match visual indentation |
reviewbot | |
Col: 29 E131 continuation line unaligned for hanging indent |
reviewbot | |
We only care about the ID and latest_revision, and not the other fields or the deserialized object, so let's use … |
chipx86 | |
history.pk |
chipx86 | |
Col: 29 E131 continuation line unaligned for hanging indent |
reviewbot | |
Sicne this is a ChoiceField now, and we get a FileAttachmentHistory, we should probably remove the _id suffix. |
chipx86 | |
I think if I were seeing this error and didn't know the code, I'd be confused. How about: The FileAttachmentHistory … |
chipx86 | |
""" on the next line. |
chipx86 | |
This is a different condition, so it should have its own test. |
chipx86 | |
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot | |
Col: 29 E131 continuation line unaligned for hanging indent |
reviewbot | |
No need for the outer parens in JavaScript. |
chipx86 | |
ValidationError is going to trigger a HTTP 500. We need to check for that. |
chipx86 | |
Col: 29 E131 continuation line unaligned for hanging indent |
reviewbot |
- Commit:
-
6ad07cfbdc021d429a149ad43521fe5ead999a4de564e34798ac738a5fb19a0456784611cba10ef7
- Diff:
-
Revision 2 (+163 -32)
-
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:
-
-
-
What happens if we have two uploading simultaneously?
Perhaps we should do something like the local_id calculation for ReviewRequests?
-
-
Might be good to have a
Meta.get_latest_by = 'attachment_revision
set forFileAttachment
. Then we can just do:attachment_history.file_attachments.latest()
-
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.
-
-
-
-
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 perFileAttachment
: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]
-
-
-
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. -
-
-
-
We can save on a query with:
attachments = FileAttachment.objects.filter( attachment_history=file_attachment.attachment_history_id)
- Commit:
-
e564e34798ac738a5fb19a0456784611cba10ef7a12addfc0f022a4e46d3318aeee371c0d60c9768
- Diff:
-
Revision 3 (+184 -38)
-
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
-
-
-
-
-
-
- Commit:
-
a12addfc0f022a4e46d3318aeee371c0d60c9768259d034c3324bcf6c3a84768758d922e3ef2c038
- Diff:
-
Revision 4 (+187 -38)
-
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
-
-
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).
-
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. -
- Change Summary:
-
Update to use latest_revision field. More unit testing is coming in a future update.
- Commit:
-
259d034c3324bcf6c3a84768758d922e3ef2c038b2a6ec41c92e1cd9d542c6075d2eede66ef915ad
- Diff:
-
Revision 5 (+189 -38)
-
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
-
- 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:
-
b2a6ec41c92e1cd9d542c6075d2eede66ef915ade8e3c2aaf1e5fa513c8a024945956283eb418fb0
- Diff:
-
Revision 6 (+311 -57)
-
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
-
-
-
We also need unit tests for the API, to check success and error conditions.
-
Sicne this is a
ChoiceField
now, and we get aFileAttachmentHistory
, we should probably remove the_id
suffix. -
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?
-
-
-
-
- Change Summary:
-
Make requested changes, add API unit tests.
- Commit:
-
e8e3c2aaf1e5fa513c8a024945956283eb418fb0b775166736661d7b4b9ff76a18bd23fffd59b6f8
- Diff:
-
Revision 7 (+402 -59)
-
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
-