Summary

Per-user file storage model changes

Review Request #6022 — Created June 23, 2014 and submitted Aug. 12, 2014, 7:12 p.m.

Information

Owner

rvaiya

Repository

Review Board

Branch

master

Bugs

Depends On

Commit

1f7043c...

Reviewers

Groups

reviewboard, students

People

Description

Model changes for enabling ownership of FileAttachments

Testing Done


Evolutions manually run successfully
Test suite run

Issues

Description	From	Last Updated
Col: 5 E265 block comment should start with '# '	reviewbot	July 21, 2014, 8:16 a.m.
Col: 43 E251 unexpected spaces around keyword / parameter equals	reviewbot	July 21, 2014, 8:16 a.m.
Col: 45 E251 unexpected spaces around keyword / parameter equals	reviewbot	July 21, 2014, 8:16 a.m.
Col: 56 E251 unexpected spaces around keyword / parameter equals	reviewbot	July 21, 2014, 8:16 a.m.
Col: 58 E251 unexpected spaces around keyword / parameter equals	reviewbot	July 21, 2014, 8:17 a.m.
Col: 5 E265 block comment should start with '# '	reviewbot	July 21, 2014, 8:17 a.m.
Col: 56 W291 trailing whitespace	reviewbot	Aug. 12, 2014, 3:05 p.m.
Col: 48 E251 unexpected spaces around keyword / parameter equals	reviewbot	July 21, 2014, 8:17 a.m.
Col: 50 E251 unexpected spaces around keyword / parameter equals	reviewbot	July 21, 2014, 8:17 a.m.
Col: 61 E251 unexpected spaces around keyword / parameter equals	reviewbot	July 21, 2014, 8:17 a.m.
Col: 63 E251 unexpected spaces around keyword / parameter equals	reviewbot	July 21, 2014, 8:17 a.m.
undefined name 'self'	reviewbot	July 21, 2014, 8:17 a.m.
Col: 80 E501 line too long (95 > 79 characters)	reviewbot	July 21, 2014, 8:22 a.m.
The first line of this file should be from __future__ import unicode_literals	david	Aug. 12, 2014, 2:56 p.m.
Col: 80 E501 line too long (96 > 79 characters)	reviewbot	Aug. 12, 2014, 3:05 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Aug. 12, 2014, 3:05 p.m.
Col: 80 E501 line too long (96 > 79 characters)	reviewbot	Aug. 12, 2014, 3:05 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Aug. 12, 2014, 3:05 p.m.
You'll need to prepend this line to the file, followed by a blank line: from __future__ import unicode_literals This ensures …	chipx86	Aug. 12, 2014, 4:38 p.m.
Imports should be alphabetical.	chipx86	Aug. 12, 2014, 4:38 p.m.
Since file and review_request are now optional, they should default to None.	chipx86	Aug. 12, 2014, 4:39 p.m.
Remove this blank line.	chipx86	Aug. 12, 2014, 4:39 p.m.
This is referencing file, since it's None. Doesn't look like this form was tested?	chipx86	Aug. 12, 2014, 4:39 p.m.
You need 2 blank lines between classes. You removed one of them.	chipx86	Aug. 12, 2014, 4:40 p.m.
Alphabetical.	chipx86	Aug. 12, 2014, 4:40 p.m.
Alphabetical.	chipx86	Aug. 12, 2014, 4:40 p.m.
Should name this 'file_attachments'.	chipx86	Aug. 12, 2014, 4:40 p.m.
No blank line here.	chipx86	Aug. 12, 2014, 4:40 p.m.
Since we're dealing with optional keyword arguments now, can you do: form.create(file=..., review_request=...)	chipx86	Aug. 12, 2014, 4:41 p.m.
'guess_mimetype' imported but unused	reviewbot	Aug. 12, 2014, 5:33 p.m.
undefined name 'is_exe_in_path'	reviewbot	Aug. 12, 2014, 5:33 p.m.
undefined name 'subprocess'	reviewbot	Aug. 12, 2014, 5:33 p.m.
undefined name 'subprocess'	reviewbot	Aug. 12, 2014, 5:33 p.m.
undefined name 'subprocess'	reviewbot	Aug. 12, 2014, 5:34 p.m.
undefined name 'subprocess'	reviewbot	Aug. 12, 2014, 5:34 p.m.
'guess_mimetype' imported but unused	reviewbot	Aug. 12, 2014, 5:34 p.m.
undefined name 'is_exe_in_path'	reviewbot	Aug. 12, 2014, 5:34 p.m.
undefined name 'subprocess'	reviewbot	Aug. 12, 2014, 5:34 p.m.
undefined name 'subprocess'	reviewbot	Aug. 12, 2014, 5:34 p.m.
undefined name 'subprocess'	reviewbot	Aug. 12, 2014, 5:34 p.m.
undefined name 'subprocess'	reviewbot	Aug. 12, 2014, 5:34 p.m.
Too many blank lines. Should only be one at this level.	chipx86	Aug. 12, 2014, 7:11 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/accounts/models.py
    reviewboard/attachments/models.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/accounts/models.py
    reviewboard/attachments/models.py

reviewboard/accounts/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 5
 E265 block comment should start with '# '
```
reviewboard/accounts/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 43
 E251 unexpected spaces around keyword / parameter equals
```
reviewboard/accounts/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 45
 E251 unexpected spaces around keyword / parameter equals
```
reviewboard/accounts/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 56
 E251 unexpected spaces around keyword / parameter equals
```
reviewboard/accounts/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 58
 E251 unexpected spaces around keyword / parameter equals
```
reviewboard/accounts/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 5
 E265 block comment should start with '# '
```
reviewboard/accounts/models.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 56
 W291 trailing whitespace
```
reviewboard/accounts/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 48
 E251 unexpected spaces around keyword / parameter equals
```
reviewboard/accounts/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 50
 E251 unexpected spaces around keyword / parameter equals
```
reviewboard/accounts/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 61
 E251 unexpected spaces around keyword / parameter equals
```
reviewboard/accounts/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 63
 E251 unexpected spaces around keyword / parameter equals
```
reviewboard/accounts/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 undefined name 'self'
```

Change Summary:

Abandoned usage statistics as discussed at the last UCOSP meeting. Added test/evolution for simple change to the FileAttachment model.

Description:

~		Preliminary changes for user file storage
	~	Preliminary back-end changes for user file storage

Testing Done:

~		Standard unit tests run, nothing seems to break. Not ready for a full review yet.
	~	Added test_file_ownership to FileAttachments
	+	Old tests pass

Commit:

c967b15849a705fe80b6caaa7d9220c106a072f2

b9fe14ff99e039f88d17b8c1e63ee7358aa6e819

Diff:

Revision 2 (+24 -1)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/attachments/evolutions/__init__.py
	reviewboard/attachments/evolutions/file_attachment_ownership.py

Tool: Pyflakes
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py

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

Commit:

b9fe14ff99e039f88d17b8c1e63ee7358aa6e819

4286bde44f2d2c74cc519ee612d2eee43b97a710

Diff:

Revision 3 (+25 -1)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/attachments/evolutions/__init__.py
	reviewboard/attachments/evolutions/file_attachment_ownership.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py



Tool: Pyflakes
Processed Files:
    reviewboard/attachments/tests.py
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py

reviewboard/attachments/evolutions/file_attachment_ownership.py (Diff revision 3)
The issue has been resolved. Show all issues
```
The first line of this file should be from __future__ import unicode_literals
```

Summary:

[WIP] Per-user file storage

Per-user file storage model changes

Description:

~		Preliminary back-end changes for user file storage
	~	Model changes for enabling ownership of FileAttachments

Testing Done:

~		Added test_file_ownership to FileAttachments
~		Old tests pass
	~	Evolutions manually run successfully
	~	Test suite run

Commit:

4286bde44f2d2c74cc519ee612d2eee43b97a710

0a1b794936322ee6267f878ddbaf763b5fab5bd5

Diff:

Revision 4 (+84 -58)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/evolutions/__init__.py
	reviewboard/attachments/evolutions/file_attachment_ownership.py

Tool: Pyflakes
Processed Files:
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/mimetypes.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/mimetypes.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py

reviewboard/attachments/evolutions/file_attachment_ownership.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (96 > 79 characters)
```
reviewboard/attachments/mimetypes.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Change Summary:

Updated base_file_attachment resource to allow for empty path

Commit:

0a1b794936322ee6267f878ddbaf763b5fab5bd5

4facd8a7564dd026a519541f1fe408efe31ad83d

Diff:

Revision 5 (+86 -62)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/evolutions/__init__.py
	reviewboard/attachments/evolutions/file_attachment_ownership.py
	reviewboard/webapi/resources/base_file_attachment.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/mimetypes.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py
    reviewboard/attachments/forms.py



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/mimetypes.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py
    reviewboard/attachments/forms.py

reviewboard/attachments/evolutions/file_attachment_ownership.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (96 > 79 characters)
```
reviewboard/attachments/mimetypes.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Change Summary:

Style changes

Commit:

4facd8a7564dd026a519541f1fe408efe31ad83d

bd07aa5cf76e7fb44d0e7d48ea22ea1b2bb470cc

Diff:

Revision 6 (+88 -62)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/evolutions/__init__.py
	reviewboard/attachments/evolutions/file_attachment_ownership.py
	reviewboard/webapi/resources/base_file_attachment.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/mimetypes.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py
    reviewboard/attachments/forms.py



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/mimetypes.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py
    reviewboard/attachments/forms.py

I thought we decided that UploadFileForm was going to be refactored to have a base class for the common bits, and then subclasses for general file uploading an review request-related uploading?

rvaiya Aug. 12, 2014, 4:28 p.m.

I decided to simplify my create method so it always creates an empty FileAttachment associated with the user that made the request. I moved guess_mimetype because I intend on performing the check when the file is ultimately provided in the PUT request (directly). I have realized that in this case my changes to UploadFileForm.create are unncessary since my create method does not use it to construct the FileAttachment (it ignores all POST parameters as mentioned).

reviewboard/attachments/evolutions/file_attachment_ownership.py (Diff revision 6)

The issue has been resolved. Show all issues

You'll need to prepend this line to the file, followed by a blank line:
from __future__ import unicode_literals

This ensures all strings in our codebase are in Unicode format.

reviewboard/attachments/forms.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Imports should be alphabetical.
```

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

The issue has been dropped. Show all issues

Since file and review_request are now optional, they should default to None.

rvaiya Aug. 12, 2014, 4:41 p.m.

I decided to preserve UploadFileForm.create since my resource will not use it.

reviewboard/attachments/forms.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Remove this blank line.
```
reviewboard/attachments/forms.py (Diff revision 6)
The issue has been resolved. Show all issues
```
This is referencing file, since it's None. Doesn't look like this form was tested?
```
reviewboard/attachments/forms.py (Diff revision 6)
The issue has been resolved. Show all issues
```
You need 2 blank lines between classes. You removed one of them.
```
reviewboard/attachments/mimetypes.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Alphabetical.
```
reviewboard/attachments/models.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Alphabetical.
```
reviewboard/attachments/models.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Should name this 'file_attachments'.
```
reviewboard/attachments/models.py (Diff revision 6)
The issue has been resolved. Show all issues
```
No blank line here.
```

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

The issue has been dropped. Show all issues

Since we're dealing with optional keyword arguments now, can you do:
form.create(file=..., review_request=...)

rvaiya Aug. 12, 2014, 4:41 p.m.

Given that I have decided to preserve UploadFileForm.create I have made path mandatory in base_file_attachment. Since my resource will operate independently and this can preserve the existing DnD functionality for review-request associated FileAttachments.

Commit:

bd07aa5cf76e7fb44d0e7d48ea22ea1b2bb470cc

61eabedf4c5cd1138981a3447268091be643c253

Diff:

Revision 7 (+71 -5)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/evolutions/__init__.py
	reviewboard/attachments/evolutions/file_attachment_ownership.py
	reviewboard/webapi/resources/base_file_attachment.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/attachments/models.py
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/mimetypes.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py
    reviewboard/attachments/forms.py

Commit:

61eabedf4c5cd1138981a3447268091be643c253

af372ae22624e529b991ca14284a7fc76a8697f2

Diff:

Revision 8 (+70 -4)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/evolutions/__init__.py
	reviewboard/attachments/evolutions/file_attachment_ownership.py

Tool: Pyflakes
Processed Files:
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/mimetypes.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/mimetypes.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py

reviewboard/attachments/forms.py (Diff revision 8)
The issue has been dropped. Show all issues
```
 'guess_mimetype' imported but unused
```
reviewboard/attachments/forms.py (Diff revision 8)
The issue has been dropped. Show all issues
```
 undefined name 'is_exe_in_path'
```
reviewboard/attachments/forms.py (Diff revision 8)
The issue has been dropped. Show all issues
```
 undefined name 'subprocess'
```
reviewboard/attachments/forms.py (Diff revision 8)
The issue has been dropped. Show all issues
```
 undefined name 'subprocess'
```
reviewboard/attachments/forms.py (Diff revision 8)
The issue has been dropped. Show all issues
```
 undefined name 'subprocess'
```
reviewboard/attachments/forms.py (Diff revision 8)
The issue has been dropped. Show all issues
```
 undefined name 'subprocess'
```

Diff:

Revision 9 (+70 -4)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/evolutions/__init__.py
	reviewboard/attachments/evolutions/file_attachment_ownership.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/mimetypes.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py



Tool: Pyflakes
Processed Files:
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/mimetypes.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py

reviewboard/attachments/forms.py (Diff revision 9)
The issue has been dropped. Show all issues
```
 'guess_mimetype' imported but unused
```
reviewboard/attachments/forms.py (Diff revision 9)
The issue has been dropped. Show all issues
```
 undefined name 'is_exe_in_path'
```
reviewboard/attachments/forms.py (Diff revision 9)
The issue has been dropped. Show all issues
```
 undefined name 'subprocess'
```
reviewboard/attachments/forms.py (Diff revision 9)
The issue has been dropped. Show all issues
```
 undefined name 'subprocess'
```
reviewboard/attachments/forms.py (Diff revision 9)
The issue has been dropped. Show all issues
```
 undefined name 'subprocess'
```
reviewboard/attachments/forms.py (Diff revision 9)
The issue has been dropped. Show all issues
```
 undefined name 'subprocess'
```

Commit:

af372ae22624e529b991ca14284a7fc76a8697f2

b6ee9da579128a32f35bbe6da8ddf60dbe1f9ed0

Diff:

Revision 10 (+71 -45)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/evolutions/__init__.py
	reviewboard/attachments/evolutions/file_attachment_ownership.py

Tool: Pyflakes
Processed Files:
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/mimetypes.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/mimetypes.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py

Ship it!

This looks good! One small thing from me. I want to make sure everyone else is happy too, but then this should be able to go in.

reviewboard/attachments/forms.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Too many blank lines. Should only be one at this level.
```

Commit:

b6ee9da579128a32f35bbe6da8ddf60dbe1f9ed0

1f7043cb33693446a11637c80e39b2307001ca29

Diff:

Revision 11 (+70 -45)

Show changes

	reviewboard/attachments/forms.py
	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/models.py
	reviewboard/attachments/evolutions/__init__.py
	reviewboard/attachments/evolutions/file_attachment_ownership.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/mimetypes.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py



Tool: Pyflakes
Processed Files:
    reviewboard/attachments/evolutions/__init__.py
    reviewboard/attachments/mimetypes.py
    reviewboard/attachments/models.py
    reviewboard/attachments/forms.py
    reviewboard/attachments/evolutions/file_attachment_ownership.py

Ship it!

```
Ship It!
```

Status:: Completed