Summary

Fix the file attachment aren't delete when removed from review requests

Review Request #7879 — Created Jan. 16, 2016 and submitted March 1, 2016, 11:01 a.m.

Information

Owner

weijie

Repository

Review Board

Branch

master

Bugs

4054

Depends On

Reviewers

Groups

reviewboard, students

People

Description

File attachments aren't deleted when removed from review requests

Testing Done

I tested delete files manually.

 1. I add s single file, then I delete it before publishing. The url r/xxxx/file/yyyy return 404.

 2. I add a single file, then I delete it after publishing. The file exists. 

 3. I add 2 files, one is add before publishing and another is add after publishing. The result is same as above.

Currently I am adding the unit test.

./reviewboard/manage.py test -- reviewboard.webapi.tests.test_file_attachment_draft

Issues

Description	From	Last Updated
local variable 'result' is assigned to but never used	reviewbot	Jan. 16, 2016, 7:37 a.m.
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	Jan. 16, 2016, 7:38 a.m.
Add a blank line before this. This comment also doesn't make sense. We should say something along the lines of: …	david	Jan. 17, 2016, 6:04 a.m.
Conditionals in python don't require surrounding parentheses. We should also use .exists() instead of len(... .all()): if not file_attachment.review_request.exists():	david	Jan. 17, 2016, 6:04 a.m.
We don't want to delete the draft--there may be other modifications to the review request.	david	Jan. 17, 2016, 6:04 a.m.
Add a blank line after this.	david	Jan. 17, 2016, 6:04 a.m.
Blank line between these.	chipx86	Jan. 17, 2016, 6:05 a.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Jan. 17, 2016, 6:05 a.m.
Make sure to add a unit test covering the case of a draft file attachment that was not published, to …	chipx86	Jan. 23, 2016, 10:57 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Jan. 23, 2016, 10:39 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Jan. 23, 2016, 10:38 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Jan. 23, 2016, 10:38 p.m.
Col: 52 E128 continuation line under-indented for visual indent	reviewbot	Jan. 23, 2016, 10:38 p.m.
Col: 52 E124 closing bracket does not match visual indentation	reviewbot	Jan. 23, 2016, 10:38 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Jan. 23, 2016, 10:38 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Jan. 23, 2016, 10:38 p.m.
Col: 9 E265 block comment should start with '# '	reviewbot	Jan. 23, 2016, 10:38 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Jan. 23, 2016, 10:42 p.m.
Col: 9 E265 block comment should start with '# '	reviewbot	Jan. 23, 2016, 10:44 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Jan. 23, 2016, 10:43 p.m.
Col: 51 E128 continuation line under-indented for visual indent	reviewbot	Jan. 23, 2016, 10:43 p.m.
Col: 51 E124 closing bracket does not match visual indentation	reviewbot	Jan. 23, 2016, 10:43 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Jan. 23, 2016, 10:53 p.m.
Col: 9 E265 block comment should start with '# '	reviewbot	Jan. 23, 2016, 10:54 p.m.
Col: 42 W291 trailing whitespace	reviewbot	Jan. 23, 2016, 10:53 p.m.
Col: 51 E128 continuation line under-indented for visual indent	reviewbot	Jan. 23, 2016, 10:57 p.m.
Col: 39 W291 trailing whitespace	reviewbot	Jan. 23, 2016, 10:54 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Jan. 23, 2016, 10:57 p.m.
Can we also check not file_attachment.inactive_review_request.exists()?	david	Feb. 3, 2016, 8:50 p.m.
Can we add a check here that a new query to verify that FileAttachment.get(pk=file_attachment.pk) raises FileAttachment.DoesNotExist?	david	Feb. 3, 2016, 8:50 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Jan. 30, 2016, 12:03 p.m.
Can you surround the conditional with parens instead of using the continuation character?	david	Jan. 30, 2016, 12:04 p.m.
This should be a comment, not a multiline string (prefix each line with #).	david	Feb. 23, 2016, 1:23 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py

reviewboard/webapi/resources/base_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 local variable 'result' is assigned to but never used
```
reviewboard/webapi/resources/base_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (84 > 79 characters)
```

Summary:

-	4054: File attachments aren't deleted when removed from review requests
+	Fix the file attachment aren't delete when removed from review requests

Testing Done:

	+	I testd the single and multiple files delete before and after publish.
	+	Only the file delete before publishing is deleted.

Diff:

Revision 2 (+5 -1)

Show changes

reviewboard/webapi/resources/base_file_attachment.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py

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

The issue has been resolved. Show all issues

Add a blank line before this.

This comment also doesn't make sense. We should say something along the lines of:

# If this file attachment has never been made public, delete the model itself.

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

The issue has been resolved. Show all issues

Conditionals in python don't require surrounding parentheses.
We should also use .exists() instead of len(... .all()):

if not file_attachment.review_request.exists():

reviewboard/webapi/resources/base_file_attachment.py (Diff revision 2)
The issue has been resolved. Show all issues
```
We don't want to delete the draft--there may be other modifications to the review request.
```
reviewboard/webapi/resources/base_file_attachment.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Add a blank line after this.
```

Also, please put the bug number into the "bugs" field.

Groups:

+	students

Diff:

Revision 3 (+4)

Show changes

reviewboard/webapi/resources/base_file_attachment.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py

reviewboard/webapi/resources/base_file_attachment.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```

reviewboard/webapi/resources/base_file_attachment.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line between these.
```

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

The issue has been resolved. Show all issues

Make sure to add a unit test covering the case of a draft file attachment that was not published, to prove that this worked and to make sure it never regresses.

Diff:

Revision 4 (+5)

Show changes

reviewboard/webapi/resources/base_file_attachment.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py

reviewboard/webapi/resources/base_file_attachment.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```

Could you give this a read and update the description?

Also, make sure spelling is correct (you have 'testd' instead of 'tested' in your testing done).

Testing Done:

~		I testd the single and multiple files delete before and after publish.
~		Only the file delete before publishing is deleted.
	~	I tested delete files manually.
	~	1. I add s single file, then I delete it before publishing. The url r/xxxx/file/yyyy return 404.
	+	2. I add a single file, then I delete it after publishing. The file exists.
	+	3. I add 2 files, one is add before publishing and another is add after publishing. The result is same as above.
	+
	+	Currently I am adding the unit test.

Description:

~		Fix the issue 4054: File attachments aren't deleted when removed from review requests
	~	[WIP]Fix the issue 4054: File attachments aren't deleted when removed from review requests

Diff:

Revision 5 (+34)

Show changes

	reviewboard/webapi/resources/base_file_attachment.py
	reviewboard/webapi/tests/test_file_attachment_draft.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/webapi/tests/test_file_attachment_draft.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/webapi/tests/test_file_attachment_draft.py

reviewboard/webapi/resources/base_file_attachment.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 52
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 52
 E124 closing bracket does not match visual indentation
```
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 9
 E265 block comment should start with '# '
```

Diff:

Revision 6 (+24 -1)

Show changes

	reviewboard/webapi/resources/base_file_attachment.py
	reviewboard/webapi/tests/test_file_attachment_draft.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/webapi/tests/test_file_attachment_draft.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/webapi/tests/test_file_attachment_draft.py

reviewboard/webapi/resources/base_file_attachment.py (Diff revision 6)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/webapi/resources/base_file_attachment.py (Diff revision 6)
The issue has been dropped. Show all issues
```
Col: 9
 E265 block comment should start with '# '
```
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 51
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 51
 E124 closing bracket does not match visual indentation
```

Diff:

Revision 7 (+23 -1)

Show changes

	reviewboard/webapi/resources/base_file_attachment.py
	reviewboard/webapi/tests/test_file_attachment_draft.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/webapi/tests/test_file_attachment_draft.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/webapi/tests/test_file_attachment_draft.py

reviewboard/webapi/resources/base_file_attachment.py (Diff revision 7)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/webapi/resources/base_file_attachment.py (Diff revision 7)
The issue has been dropped. Show all issues
```
Col: 9
 E265 block comment should start with '# '
```
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 42
 W291 trailing whitespace
```
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 51
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 39
 W291 trailing whitespace
```

Diff:

Revision 8 (+23 -1)

Show changes

	reviewboard/webapi/resources/base_file_attachment.py
	reviewboard/webapi/tests/test_file_attachment_draft.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/webapi/tests/test_file_attachment_draft.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/webapi/tests/test_file_attachment_draft.py

reviewboard/webapi/resources/base_file_attachment.py (Diff revision 8)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```

Description:

~		[WIP]Fix the issue 4054: File attachments aren't deleted when removed from review requests
	~	Fix the issue 4054: File attachments aren't deleted when removed from review requests

This is looking pretty good! Just a couple comments to bullet-proof things even further.

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

The issue has been resolved. Show all issues

Can we also check not file_attachment.inactive_review_request.exists()?

mike_conley Jan. 29, 2016, 8:51 p.m.

I don't actually see this fix in your latest patch. Is there a reason why we don't need it?

WE

weijie Jan. 29, 2016, 10:55 p.m.
```
Sorry. You are right.
```

reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 8)

The issue has been resolved. Show all issues

Can we add a check here that a new query to verify that FileAttachment.get(pk=file_attachment.pk) raises FileAttachment.DoesNotExist?

weijie Jan. 29, 2016, 10:55 p.m.

Can you help me to check? I am not sure I am fully understanding that.

david Jan. 30, 2016, 7:24 a.m.

It'll be something like:

with self.assertRaises(FileAttachment.DoesNotExist):
    FileAttachment.get(pk=file_attechment.pk)

Diff:

Revision 9 (+27 -1)

Show changes

	reviewboard/webapi/resources/base_file_attachment.py
	reviewboard/webapi/tests/test_file_attachment_draft.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/webapi/tests/test_file_attachment_draft.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/webapi/tests/test_file_attachment_draft.py

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

The issue has been resolved. Show all issues

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

mike_conley Jan. 29, 2016, 8:49 p.m.

Why did you drop this issue? I think we should try to abide by pep8 where we can. Can you break this up over two lines?

Diff:

Revision 10 (+31 -1)

Show changes

	reviewboard/webapi/resources/base_file_attachment.py
	reviewboard/webapi/tests/test_file_attachment_draft.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/webapi/tests/test_file_attachment_draft.py



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/webapi/tests/test_file_attachment_draft.py

reviewboard/webapi/resources/base_file_attachment.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Can you surround the conditional with parens instead of using the  continuation character?
```

Diff:

Revision 11 (+33 -1)

Show changes

	reviewboard/webapi/resources/base_file_attachment.py
	reviewboard/webapi/tests/test_file_attachment_draft.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/webapi/tests/test_file_attachment_draft.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py
    reviewboard/webapi/tests/test_file_attachment_draft.py

Testing Done:

		I tested delete files manually.
		1. I add s single file, then I delete it before publishing. The url r/xxxx/file/yyyy return 404.
		2. I add a single file, then I delete it after publishing. The file exists.
		3. I add 2 files, one is add before publishing and another is add after publishing. The result is same as above.

~		Currently I am adding the unit test.
	~
	+
	+	Currently I am adding the unit test.
	+	./reviewboard/manage.py test -- reviewboard.webapi.tests.test_file_attachment_draft

The bug number should go in the "Bugs" field, not in the description.

reviewboard/webapi/resources/base_file_attachment.py (Diff revision 11)
The issue has been resolved. Show all issues
```
This should be a comment, not a multiline string (prefix each line with #).
```

Description:

~		Fix the issue 4054: File attachments aren't deleted when removed from review requests
	~	File attachments aren't deleted when removed from review requests

Bugs:

+	4054

Diff:

Revision 12 (+7)

Show changes

reviewboard/webapi/resources/base_file_attachment.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_file_attachment.py

Ship it!

Revision 12 of the diff seems to have posted an old version of the code, but revision 11 looks good. I'm going to land this. Thanks!

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (9d5f2e1)