Summary

Ability to update attachments

Review Request #5824 — Created May 24, 2014 and discarded Dec. 10, 2014, 4:53 p.m.

Information

Owner

volodymyr

Repository

Review Board

Branch

Bugs

800

Depends On

~~6120~~, ~~5911~~

Blocks

6119

Reviewers

Groups

reviewboard, students

People

Description

This review allows users to update attachments with new version. See https://reviewboard.hackpad.com/Reviewing-attachments-tbfh4oiwTHB for description or https://www.youtube.com/watch?v=anX_rQKWSp0 for a quick demo.

Review includes: * Change to a schema to track sequences of file attachments * Modified attachment creation code to correctly assign the new parameters * Modified display logic to only show the latest element of each attachment sequence * Modified attachment review pages to include the slider

This review is aimed at project branch (see testing section for the problems description)

Testing Done

Performed various scenarios of adding/updating/reviewing attachments.

Current problems: * There is no beautiful UI * We are reloading the page each time slider is changed - API should be extended to load the necessary data dynamically.

Issues

Description	From	Last Updated
As per last week's meeting, we need to stick to existing terminolgoy as best as possible, instead of inventing new …	chipx86	June 14, 2014, 10:49 a.m.
'Max' imported but unused	reviewbot	June 14, 2014, 10:49 a.m.
Col: 72 E502 the backslash is redundant between brackets	reviewbot	June 14, 2014, 10:49 a.m.
Col: 17 E128 continuation line under-indented for visual indent	reviewbot	June 14, 2014, 10:49 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	June 14, 2014, 10:49 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	June 22, 2014, 11:49 a.m.
Col: 61 E211 whitespace before '['	reviewbot	June 22, 2014, 11:49 a.m.
list comprehension redefines 'file_attachment' from line 574	reviewbot	June 17, 2014, 6:04 p.m.
Col: 17 E122 continuation line missing indentation or outdented	reviewbot	June 29, 2014, 12:41 p.m.
Col: 61 E211 whitespace before '['	reviewbot	June 29, 2014, 12:41 p.m.
list comprehension redefines 'file_attachment' from line 574	reviewbot	June 29, 2014, 12:41 p.m.
Col: 17 E122 continuation line missing indentation or outdented	reviewbot	July 6, 2014, 8:37 a.m.
Col: 61 E211 whitespace before '['	reviewbot	July 6, 2014, 8:37 a.m.
'FileAttachmentHistory' imported but unused	reviewbot	July 6, 2014, 8:37 a.m.
list comprehension redefines 'file_attachment' from line 576	reviewbot	July 6, 2014, 8:37 a.m.
Instead of using FileAttachment.objects.filter(attachment_history=attachment_history), you can follow the reverse field (note that this particular code needs the related_name change I …	david	July 19, 2014, 9:10 p.m.
Why not just use the FileAttachmentHistory's id field as the display position? It's already a unique integer which is auto-incremented …	david	July 19, 2014, 9:10 p.m.
Col: 17 E122 continuation line missing indentation or outdented	reviewbot	July 10, 2014, 11:05 a.m.
We use trailing commas for lists and dicts in python.	david	July 19, 2014, 9:10 p.m.
Care to add related_name='file_attachments' here?	david	July 19, 2014, 9:10 p.m.
If you set default=0, you don't have to set it when creating attachments that don't use an existing FileAttachmentHistory	david	July 19, 2014, 9:10 p.m.
This can follow the backwards relationship too: return self.file_attachments.count() - 1 Why don't you consider the first uploaded file to …	david	July 7, 2014, 3:17 p.m.
I think it's worth creating a method on FileAttachmentHistory to return the latest revision.	david	July 19, 2014, 9:11 p.m.
Col: 61 E211 whitespace before '['	reviewbot	July 10, 2014, 11:05 a.m.
Can you add a docstring?	david	July 19, 2014, 9:11 p.m.
These can be combined and made more efficient using sorted: return sorted(self.get_latest_file_attachments(), key=get_display_position)	david	July 19, 2014, 9:11 p.m.
'FileAttachmentHistory' imported but unused	reviewbot	July 19, 2014, 9:10 p.m.
Care to just put this in the dict definition for context_data? It's not worth defining a variable unless we use …	david	July 19, 2014, 9:11 p.m.
list comprehension redefines 'file_attachment' from line 576	reviewbot	July 10, 2014, 11:05 a.m.
Same here.	david	July 19, 2014, 9:11 p.m.
This can use the backwards relationship.	david	July 19, 2014, 9:11 p.m.
Should have a trailing comma.	david	July 19, 2014, 9:12 p.m.
I think it might be nice to pull out the upload file view into a separate change. You already have …	david	July 19, 2014, 9:12 p.m.
It looks like 'e' isn't used in here, which will make jshint unhappy. Can you just remove it from the …	david	July 19, 2014, 9:13 p.m.
These can be simplified: this.$el.html('<div id="revision_selector" />');	david	July 19, 2014, 9:13 p.m.
Can you make this use /* */, since it's a multi-line comment?	david	July 19, 2014, 9:13 p.m.
Alignment is funky here.	david	July 19, 2014, 9:13 p.m.
This also needs to take into account the local site name and SITE_ROOT.	david	Aug. 2, 2014, 10:16 p.m.
Leftover debug output?	david	July 19, 2014, 9:14 p.m.
If you put the review request into a variable in the containing method, you could just use it here without …	david	July 19, 2014, 9:14 p.m.
We generally put the space at the end of the first line instead of the beginning of the second.	david	July 19, 2014, 9:14 p.m.
This can use the reverse relation.	david	July 19, 2014, 9:14 p.m.
Col: 56 E251 unexpected spaces around keyword / parameter equals	reviewbot	Aug. 2, 2014, 10:18 p.m.
Col: 58 E251 unexpected spaces around keyword / parameter equals	reviewbot	Aug. 2, 2014, 10:18 p.m.
Col: 63 E502 the backslash is redundant between brackets	reviewbot	Aug. 2, 2014, 10:18 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Aug. 2, 2014, 10:18 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Aug. 2, 2014, 10:18 p.m.
Col: 59 E502 the backslash is redundant between brackets	reviewbot	Aug. 2, 2014, 10:18 p.m.
Col: 13 E131 continuation line unaligned for hanging indent	reviewbot	Aug. 2, 2014, 10:19 p.m.
Col: 60 E502 the backslash is redundant between brackets	reviewbot	Aug. 2, 2014, 10:19 p.m.
Col: 17 E131 continuation line unaligned for hanging indent	reviewbot	Aug. 2, 2014, 10:19 p.m.
Col: 56 E251 unexpected spaces around keyword / parameter equals	reviewbot	Aug. 2, 2014, 10:20 p.m.
Col: 58 E251 unexpected spaces around keyword / parameter equals	reviewbot	Aug. 2, 2014, 10:20 p.m.
Col: 63 E502 the backslash is redundant between brackets	reviewbot	Aug. 2, 2014, 10:20 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Aug. 2, 2014, 10:20 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Aug. 2, 2014, 10:23 p.m.
Col: 18 E261 at least two spaces before inline comment	reviewbot	Aug. 2, 2014, 10:23 p.m.
Col: 59 E502 the backslash is redundant between brackets	reviewbot	Aug. 2, 2014, 10:22 p.m.
Col: 13 E131 continuation line unaligned for hanging indent	reviewbot	Aug. 2, 2014, 10:22 p.m.
Col: 60 E502 the backslash is redundant between brackets	reviewbot	Aug. 2, 2014, 10:23 p.m.
Col: 17 E131 continuation line unaligned for hanging indent	reviewbot	Aug. 2, 2014, 10:22 p.m.
Can this fit on one line?	PE PeterTran	Aug. 6, 2014, 12:06 p.m.
Col: 36 E131 continuation line unaligned for hanging indent	reviewbot	Aug. 6, 2014, 12:06 p.m.
Col: 40 E131 continuation line unaligned for hanging indent	reviewbot	Aug. 6, 2014, 12:06 p.m.
Is this a tab instead of white space?	PE PeterTran	Aug. 6, 2014, 12:06 p.m.
Can this be hyphen-separated for consistency with your classes?	PE PeterTran	Aug. 6, 2014, 12:06 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Aug. 11, 2014, 8:07 p.m.
Col: 80 E501 line too long (133 > 79 characters)	reviewbot	Aug. 11, 2014, 8:07 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Aug. 26, 2014, 5:14 a.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Aug. 26, 2014, 5:14 a.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Aug. 12, 2014, 11:51 a.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Aug. 12, 2014, 11:51 a.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
  Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/templates/reviews/parts/file_attachment_thumbnail.html

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
  Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/templates/reviews/parts/file_attachment_thumbnail.html

Change Summary:

Passing and handling of sequence data; uses the WIP version of the file upload backbone view

Depends On:

~~5911 - Backbone view for attachment upload~~

Diff:

Revision 2 (+59 -3)

Show changes

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

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
  Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    reviewboard/templates/reviews/review_request_dlgs.html

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
  Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    reviewboard/templates/reviews/review_request_dlgs.html

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

The issue has been resolved. Show all issues

As per last week's meeting, we need to stick to existing terminolgoy as best as possible, instead of inventing new terminology.
I strongly believe the right model is:

A attachment_revision field for the revision of the file attachment.
Some way of grouping related attachments (a SHA1 or some other unique identifier, rather than an integer, would be needed, or some other table, but a SHA1 is probably fine -- you'll need to make sure this is indexed in the Meta class for FileAttachment.)
A custom "through" table for the M2M field between ReviewRequest and FileAttachment that specifies the visible position the file attachment is shown at in the list of thumbnails (since right now, it's based on timestamp or ID or something, and we don't want things to shift around).

david June 1, 2014, 2:09 p.m.

For 2. here, I think we should use another model (maybe FileAttachmentHistory, to match the DiffSetHistory elsewhere). This way we get consistency for free because we're just using a foreign key.

chipx86 June 1, 2014, 2:17 p.m.

Agreed. This is probably the best way to go.

volodymyr June 7, 2014, 12:17 p.m.

I created FileAttachmentHistory for the purpose of using it for groupping. I can't put anything useful there though, so it serves that same purpose as if I had this variable inside the class.
I will look into part 3 shortly. The behaviour I'm thinking is that once the file attachment was added, it position stays the same (even if it gets updated). If the file collection was deleted, we preserve the relative ordering of the other files. If I will need extra field in the M2M table to achieve this behaviour, I will add it.

Change Summary:

Addressed feedback partially.

Diff:

Revision 3 (+71 -9)

Show changes

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

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
  Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    reviewboard/templates/reviews/review_request_dlgs.html

reviewboard/attachments/forms.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 72
 E502 the backslash is redundant between brackets
```
reviewboard/attachments/forms.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 17
 E128 continuation line under-indented for visual indent
```
reviewboard/attachments/models.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
  Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    reviewboard/templates/reviews/review_request_dlgs.html

reviewboard/attachments/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
 'Max' imported but unused
```

Change Summary:

Now we only display the latest versions of the file; they are ordered vy FileAttachmentHistory id. Using updated view from https://reviews.reviewboard.org/r/5911/

Diff:

Revision 4 (+116 -16)

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/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
	reviewboard/templates/reviews/review_request_box.html
	reviewboard/templates/reviews/review_request_dlgs.html
	2 more

This is a review from Review Bot.
  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/templates/reviews/review_request_dlgs.html
    reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/templates/reviews/review_request_box.html

reviewboard/attachments/models.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
1. VO
  
  volodymyr June 14, 2014, 10:51 a.m.
  I will fix these in the next iteration.
reviewboard/reviews/models/base_review_request_details.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 61
 E211 whitespace before '['
```

This is a review from Review Bot.
  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/templates/reviews/review_request_dlgs.html
    reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/templates/reviews/review_request_box.html

reviewboard/reviews/views.py (Diff revision 4)
The issue has been dropped. Show all issues
```
 list comprehension redefines 'file_attachment' from line 574
```

Diff:

Revision 5 (+134 -18)

Show changes

	reviewboard/staticbundles.py
	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
	reviewboard/templates/reviews/review_request_box.html
	3 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
    reviewboard/staticbundles.py
    reviewboard/reviews/models/base_review_request_details.py

Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/templates/reviews/review_request_dlgs.html
    reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/templates/reviews/review_request_box.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
    reviewboard/staticbundles.py
    reviewboard/reviews/models/base_review_request_details.py

Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/templates/reviews/review_request_dlgs.html
    reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/templates/reviews/review_request_box.html

reviewboard/attachments/forms.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 17
 E122 continuation line missing indentation or outdented
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 61
 E211 whitespace before '['
```
reviewboard/reviews/views.py (Diff revision 5)
The issue has been dropped. Show all issues
```
 list comprehension redefines 'file_attachment' from line 574
```

Change Summary:

Now this diff includes a slider that allows you to select revision of the file attachment.

Testing Done:

~		-
	~	Performed various scenarios of adding/updating attachments.
	+
	+	Current problems:
	+	* Comments are not displayed for a revision chosen with a slider

Diff:

Revision 6 (+451 -20)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/reviews/ui/base.py
	reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js
	reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
	8 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/reviews/urls.py
    reviewboard/attachments/forms.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/ui/base.py

Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js
    reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/templates/reviews/review_request_dlgs.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/reviews/urls.py
    reviewboard/attachments/forms.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/ui/base.py

Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js
    reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/templates/reviews/review_request_dlgs.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/attachments/forms.py (Diff revision 6)
The issue has been dropped. Show all issues
```
Col: 17
 E122 continuation line missing indentation or outdented
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 6)
The issue has been dropped. Show all issues
```
Col: 61
 E211 whitespace before '['
```
reviewboard/reviews/views.py (Diff revision 6)
The issue has been dropped. Show all issues
```
 'FileAttachmentHistory' imported but unused
```
reviewboard/reviews/views.py (Diff revision 6)
The issue has been dropped. Show all issues
```
 list comprehension redefines 'file_attachment' from line 576
```

Diff:

Revision 7 (+511 -131)

Show changes

	reviewboard/staticbundles.py
	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/reviews/ui/base.py
	reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js
	10 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/reviews/urls.py
    reviewboard/attachments/forms.py
    reviewboard/staticbundles.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/ui/base.py

Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
    reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
    reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/templates/reviews/review_request_dlgs.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/reviews/urls.py
    reviewboard/attachments/forms.py
    reviewboard/staticbundles.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/ui/base.py

Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
    reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
    reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/templates/reviews/review_request_dlgs.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/attachments/forms.py (Diff revision 7)
The issue has been dropped. Show all issues
```
Col: 17
 E122 continuation line missing indentation or outdented
```
reviewboard/reviews/models/base_review_request_details.py (Diff revision 7)
The issue has been dropped. Show all issues
```
Col: 61
 E211 whitespace before '['
```
reviewboard/reviews/views.py (Diff revision 7)
The issue has been resolved. Show all issues
```
 'FileAttachmentHistory' imported but unused
```
reviewboard/reviews/views.py (Diff revision 7)
The issue has been dropped. Show all issues
```
 list comprehension redefines 'file_attachment' from line 576
```

Fix it!

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

The issue has been resolved. Show all issues

Instead of using FileAttachment.objects.filter(attachment_history=attachment_history), you can follow the reverse field (note that this particular code needs the related_name change I asked for elsewhere):

attachment_revision = (
    attachment_history.file_attachments
        .aggregate(Max('attachment_revision'))
        .get('attachment_revision__max')
    + 1)

volodymyr July 10, 2014, 8:16 p.m.

Nice, I didn't know about the reverse fields before.

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

The issue has been resolved. Show all issues

Why not just use the FileAttachmentHistory's id field as the display position? It's already a unique integer which is auto-incremented any time a FileAttachmentHistory is created.

volodymyr July 10, 2014, 7:41 a.m.

Yeah, that's what I wanted to do. Per discussion with ChipX86, however, it was decided to have a separate field for position, as someone might implement attachment reordering. In order to do so easily, we would use a separate field for that, and whoever is implementing this change will just modify the behaviour of this field.

david July 10, 2014, 1:30 p.m.

OK. How about rewriting this particular code as such:

if FileAttachmentHistory.objects.exists():
    attachment_history.display_position = 1
else:
    attachment_history.display_position = (
        FileAttachmentHistory.objects
            .aggregate(Max('display_position'))
            .get('display_position__max') + 1)


The changes here are not using the ternary form (which we generally don't like), some reformatting, and using .exists() instead of .count() (which is slightly more efficient use of the database).

VO

volodymyr July 10, 2014, 8:16 p.m.
```
Cool, will do.
```

reviewboard/attachments/forms.py (Diff revision 7)
The issue has been resolved. Show all issues
```
We use trailing commas for lists and dicts in python.
```
reviewboard/attachments/models.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Care to add related_name='file_attachments' here?
```

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

The issue has been resolved. Show all issues

If you set default=0, you don't have to set it when creating attachments that don't use an existing FileAttachmentHistory

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

An issue was opened. Show all issues

This can follow the backwards relationship too:

return self.file_attachments.count() - 1


Why don't you consider the first uploaded file to be a revision? That doesn't make sense to me. It's not like with diffs where the 'orig' version is upstream.

volodymyr July 10, 2014, 8:16 p.m.

Well, I was thinking of mimicing behaviour for diffs (you have an original file, which gets upgraded a coupld of times). When displaying it, I am using the "orig" label, just like the diffs do, so I need total - 1 extra selection points. So I used the same definition of revisions as diffs did.

If you think we should not do that, I can return the count here and then subtract one in the front end part. OK?

reviewboard/reviews/models/base_review_request_details.py (Diff revision 7)

The issue has been resolved. Show all issues

I think it's worth creating a method on FileAttachmentHistory to return the latest revision.

VO

volodymyr July 10, 2014, 8:16 p.m.
```
Will do.
```

reviewboard/reviews/models/base_review_request_details.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Can you add a docstring?
```

reviewboard/reviews/models/base_review_request_details.py (Diff revision 7)

The issue has been resolved. Show all issues

These can be combined and made more efficient using sorted:

return sorted(self.get_latest_file_attachments(),
              key=get_display_position)

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

The issue has been resolved. Show all issues

Care to just put this in the dict definition for context_data? It's not worth defining a variable unless we use it more than once.

reviewboard/reviews/views.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/reviews/views.py (Diff revision 7)
The issue has been resolved. Show all issues
```
This can use the backwards relationship.
```
reviewboard/reviews/views.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Should have a trailing comma.
```

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 7)

The issue has been resolved. Show all issues

I think it might be nice to pull out the upload file view into a separate change. You already have the view itself in a separate review request. That way we could move over to the view and get that code pushed before all the rest of this is finished.

volodymyr July 19, 2014, 9:14 p.m.

Done:
https://reviews.reviewboard.org/r/5911/ (and also https://reviews.reviewboard.org/r/6120/)

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 7)

The issue has been resolved. Show all issues

It looks like 'e' isn't used in here, which will make jshint unhappy. Can you just remove it from the argument list?

reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 7)

The issue has been resolved. Show all issues

These can be simplified:

this.$el.html('<div id="revision_selector" />');

reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Can you make this use /* */, since it's a multi-line comment?
```
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Alignment is funky here.
```
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 7)
The issue has been dropped. Show all issues
```
This also needs to take into account the local site name and SITE_ROOT.
```
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Leftover debug output?
```

reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 7)

The issue has been resolved. Show all issues

If you put the review request into a variable in the containing method, you could just use it here without fetching it again.

reviewboard/webapi/resources/base_file_attachment.py (Diff revision 7)
The issue has been resolved. Show all issues
```
We generally put the space at the end of the first line instead of the beginning of the second.
```
reviewboard/webapi/resources/base_file_attachment.py (Diff revision 7)
The issue has been resolved. Show all issues
```
This can use the reverse relation.
```

Diff:

Revision 8 (+183 -6)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/reviews/ui/base.py
	reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
	reviewboard/static/rb/js/views/fileAttachmentReviewableView.js
	reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
	6 more

Diff:

Revision 9 (+297 -13)

Show changes

	reviewboard/staticbundles.py
	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/reviews/ui/__init__.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/ui/generic.py
	12 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/ui/base.py

Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/static/rb/js/views/fileAttachmentReviewableView.js
    reviewboard/templates/reviews/ui/text.html
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    reviewboard/static/rb/js/views/textBasedReviewableView.js
    reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
    reviewboard/static/rb/js/views/imageReviewableView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/reviews/ui/generic.py
    reviewboard/reviews/ui/__init__.py
    reviewboard/attachments/forms.py
    reviewboard/staticbundles.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/ui/base.py

Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/static/rb/js/views/fileAttachmentReviewableView.js
    reviewboard/templates/reviews/ui/text.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    reviewboard/static/rb/js/views/textBasedReviewableView.js
    reviewboard/static/rb/js/views/genericReviewableView.js
    reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/attachments/models.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 56
 E251 unexpected spaces around keyword / parameter equals
```
reviewboard/attachments/models.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 58
 E251 unexpected spaces around keyword / parameter equals
```
reviewboard/attachments/models.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 63
 E502 the backslash is redundant between brackets
```
reviewboard/attachments/models.py (Diff revision 8)
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 8)
The issue has been resolved. Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```
reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 59
 E502 the backslash is redundant between brackets
```
reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 13
 E131 continuation line unaligned for hanging indent
```
reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 60
 E502 the backslash is redundant between brackets
```
reviewboard/reviews/views.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 17
 E131 continuation line unaligned for hanging indent
```
reviewboard/attachments/models.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 56
 E251 unexpected spaces around keyword / parameter equals
```
reviewboard/attachments/models.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 58
 E251 unexpected spaces around keyword / parameter equals
```
reviewboard/attachments/models.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 63
 E502 the backslash is redundant between brackets
```
reviewboard/attachments/models.py (Diff revision 9)
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 9)
The issue has been resolved. Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```
reviewboard/reviews/ui/generic.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 18
 E261 at least two spaces before inline comment
```
reviewboard/reviews/views.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 59
 E502 the backslash is redundant between brackets
```
reviewboard/reviews/views.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 13
 E131 continuation line unaligned for hanging indent
```
reviewboard/reviews/views.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 60
 E502 the backslash is redundant between brackets
```
reviewboard/reviews/views.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 17
 E131 continuation line unaligned for hanging indent
```

Change Summary:

Addressed feedback from review bot.

Diff:

Revision 10 (+297 -13)

Show changes

	reviewboard/staticbundles.py
	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/reviews/ui/__init__.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/ui/generic.py
	12 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/reviews/ui/generic.py
    reviewboard/reviews/ui/__init__.py
    reviewboard/attachments/forms.py
    reviewboard/staticbundles.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/ui/base.py

Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/static/rb/js/views/fileAttachmentReviewableView.js
    reviewboard/templates/reviews/ui/text.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    reviewboard/static/rb/js/views/textBasedReviewableView.js
    reviewboard/static/rb/js/views/genericReviewableView.js
    reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/reviews/views.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Col: 36
 E131 continuation line unaligned for hanging indent
```
reviewboard/reviews/views.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Col: 40
 E131 continuation line unaligned for hanging indent
```

reviewboard/reviews/models/base_review_request_details.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Can this fit on one line?
```
reviewboard/static/rb/js/views/fileAttachmentReviewableView.js (Diff revision 10)
The issue has been resolved. Show all issues
```
Is this a tab instead of white space?
```
1. VO
  
  volodymyr Aug. 6, 2014, 12:08 p.m.
  Whoooops.

reviewboard/templates/reviews/ui/text.html (Diff revision 10)

The issue has been resolved. Show all issues

Can this be hyphen-separated for consistency with your classes?

volodymyr Aug. 6, 2014, 12:08 p.m.

There are a bunch of these (e.g. diff_revision_selector), but that's an existing code, so I decided not to modify it. Maybe I'll make a separate review that fixes style problems in files that I had to work with.

Change Summary:

Addressed review feedback.

Diff:

Revision 11 (+296 -13)

Show changes

	reviewboard/staticbundles.py
	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/reviews/ui/__init__.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/ui/generic.py
	12 more

Change Summary:: Updated description
Summary:: [WIP] Ability to update attachments
Ability to update attachments
Description:: ~
This is the WIP review for attachments updating. See https://reviewboard.hackpad.com/Reviewing-attachments-tbfh4oiwTHB
~
This review allows users to update attachments with new version. See https://reviewboard.hackpad.com/Reviewing-attachments-tbfh4oiwTHB for description or https://www.youtube.com/watch?v=anX_rQKWSp0 for a quick demo.

~
Right now this is just playing around. My plan is to get the basic working version done with terrible UI, and then keep improving things.
~
Review includes:
+ * Change to a schema to track sequences of file attachments
+ * Modified attachment creation code to correctly assign the new parameters
+ * Modified display logic to only show the latest element of each attachment sequence
+ * Modified attachment review pages to include the slider

~
Desicions made:
~
This review is aimed at project branch (see testing section for the problems description)
- - Right now review has ManyToMany relationship with FileAttachment table. To achive my goals, I could have added a new DB table called FileAttachemntSequence, which would have ManyToMany relationship with FileAttachment, and reviews table would have ManyToMany relationship with it. This will require changing every place where fileAttachments are retrieved, so I decided not do it. Instead, I will add sequence_id and position_in_sequnce to the FileAttachment table. Fresh versions of the same file will have the same sequence_id, but higher position_in_sequnce. Only files with the highest position_in_sequnce among their sequence_id's will be displayed in thumbnails.
- - Ideally, we would have to write a new view for file upload. I need to pass sequence_id in the POST request (so that I can tell which file user wants to update, or whether it's a new file).
Testing Done:: ~
Performed various scenarios of adding/updating attachments.
~
Performed various scenarios of adding/updating/reviewing attachments.

Current problems:
~ * Comments are not displayed for a revision chosen with a slider
~ * There is no beautiful UI
+ * We are reloading the page each time slider is changed - API should be extended to load the necessary data dynamically.

Change Summary:: Updated dependencies.
Depends On:: ~~6120 - Revision selectors~~

Change Summary:

Added database evolution, tested it. Pulled new code. Made sure that stuff still works.

Diff:

Revision 12 (+268 -8)

Show changes

	reviewboard/staticbundles.py
	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/attachments/evolutions/__init__.py
	reviewboard/attachments/evolutions/file_attachment_revision.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/reviews/ui/__init__.py
	11 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/reviews/ui/generic.py
    reviewboard/reviews/ui/__init__.py
    reviewboard/attachments/forms.py
    reviewboard/staticbundles.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/ui/base.py

Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/static/rb/js/views/fileAttachmentReviewableView.js
    reviewboard/templates/reviews/ui/text.html
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    reviewboard/static/rb/js/views/textBasedReviewableView.js
    reviewboard/static/rb/js/views/genericReviewableView.js
    reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
    reviewboard/static/rb/js/views/imageReviewableView.js
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/reviews/ui/generic.py
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/reviews/ui/__init__.py
    reviewboard/attachments/evolutions/file_attachment_revision.py
    reviewboard/attachments/forms.py
    reviewboard/staticbundles.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/ui/base.py

Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/static/rb/js/views/fileAttachmentReviewableView.js
    reviewboard/templates/reviews/ui/text.html
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/js/views/textBasedReviewableView.js
    reviewboard/static/rb/js/views/genericReviewableView.js
    reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
    reviewboard/static/rb/js/views/imageReviewableView.js

reviewboard/attachments/evolutions/file_attachment_revision.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
reviewboard/attachments/evolutions/file_attachment_revision.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (133 > 79 characters)
```

Change Summary:

Fixed long lines in evolution file.

Diff:

Revision 13 (+270 -8)

Show changes

	reviewboard/staticbundles.py
	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/attachments/evolutions/__init__.py
	reviewboard/attachments/evolutions/file_attachment_revision.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/reviews/ui/__init__.py
	11 more

Change Summary:

Removed incorrect comment.

Diff:

Revision 14 (+268 -8)

Show changes

	reviewboard/staticbundles.py
	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/attachments/evolutions/__init__.py
	reviewboard/attachments/evolutions/file_attachment_revision.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_review_request_details.py
	reviewboard/reviews/ui/__init__.py
	11 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/reviews/ui/generic.py
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/reviews/ui/__init__.py
    reviewboard/attachments/evolutions/file_attachment_revision.py
    reviewboard/attachments/forms.py
    reviewboard/staticbundles.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/ui/base.py

Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/static/rb/js/views/fileAttachmentReviewableView.js
    reviewboard/templates/reviews/ui/text.html
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/js/views/textBasedReviewableView.js
    reviewboard/static/rb/js/views/genericReviewableView.js
    reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
    reviewboard/static/rb/js/views/imageReviewableView.js

reviewboard/attachments/evolutions/file_attachment_revision.py (Diff revision 13)
The issue has been dropped. Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```
reviewboard/attachments/evolutions/file_attachment_revision.py (Diff revision 13)
The issue has been dropped. Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/reviews/ui/generic.py
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/reviews/ui/__init__.py
    reviewboard/attachments/evolutions/file_attachment_revision.py
    reviewboard/attachments/forms.py
    reviewboard/staticbundles.py
    reviewboard/reviews/models/base_review_request_details.py
    reviewboard/reviews/ui/base.py

Ignored Files:
    reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
    reviewboard/static/rb/js/views/fileAttachmentReviewableView.js
    reviewboard/templates/reviews/ui/text.html
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/js/views/textBasedReviewableView.js
    reviewboard/static/rb/js/views/genericReviewableView.js
    reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
    reviewboard/static/rb/js/views/imageReviewableView.js

reviewboard/attachments/evolutions/file_attachment_revision.py (Diff revision 14)
The issue has been dropped. Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```
reviewboard/attachments/evolutions/file_attachment_revision.py (Diff revision 14)
The issue has been dropped. Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```

Status:: Discarded

~		This is the WIP review for attachments updating. See https://reviewboard.hackpad.com/Reviewing-attachments-tbfh4oiwTHB
	~	This review allows users to update attachments with new version. See https://reviewboard.hackpad.com/Reviewing-attachments-tbfh4oiwTHB for description or https://www.youtube.com/watch?v=anX_rQKWSp0 for a quick demo.

~		Right now this is just playing around. My plan is to get the basic working version done with terrible UI, and then keep improving things.
	~	Review includes:
	+	* Change to a schema to track sequences of file attachments
	+	* Modified attachment creation code to correctly assign the new parameters
	+	* Modified display logic to only show the latest element of each attachment sequence
	+	* Modified attachment review pages to include the slider

~		Desicions made:
	~	This review is aimed at project branch (see testing section for the problems description)
-		- Right now review has ManyToMany relationship with FileAttachment table. To achive my goals, I could have added a new DB table called FileAttachemntSequence, which would have ManyToMany relationship with FileAttachment, and reviews table would have ManyToMany relationship with it. This will require changing every place where fileAttachments are retrieved, so I decided not do it. Instead, I will add sequence_id and position_in_sequnce to the FileAttachment table. Fresh versions of the same file will have the same sequence_id, but higher position_in_sequnce. Only files with the highest position_in_sequnce among their sequence_id's will be displayed in thumbnails.
-		- Ideally, we would have to write a new view for file upload. I need to pass sequence_id in the POST request (so that I can tell which file user wants to update, or whether it's a new file).