-
-
reviewboard/webapi/resources/base_file_attachment.py (Diff revision 1) local variable 'result' is assigned to but never used
-
reviewboard/webapi/resources/base_file_attachment.py (Diff revision 1) Col: 80 E501 line too long (84 > 79 characters)
Fix the file attachment aren't delete when removed from review requests
Review Request #7879 — Created Jan. 16, 2016 and submitted
Information | |
---|---|
weijie | |
Review Board | |
master | |
4054 | |
Reviewers | |
reviewboard, students | |
File attachments aren't deleted when removed from review requests
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
Description | From | Last Updated |
---|---|---|
local variable 'result' is assigned to but never used |
![]() |
|
Col: 80 E501 line too long (84 > 79 characters) |
![]() |
|
Add a blank line before this. This comment also doesn't make sense. We should say something along the lines of: … |
|
|
Conditionals in python don't require surrounding parentheses. We should also use .exists() instead of len(... .all()): if not file_attachment.review_request.exists(): |
|
|
We don't want to delete the draft--there may be other modifications to the review request. |
|
|
Add a blank line after this. |
|
|
Blank line between these. |
|
|
Col: 80 E501 line too long (86 > 79 characters) |
![]() |
|
Make sure to add a unit test covering the case of a draft file attachment that was not published, to … |
|
|
Col: 80 E501 line too long (86 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (86 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (80 > 79 characters) |
![]() |
|
Col: 52 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 52 E124 closing bracket does not match visual indentation |
![]() |
|
Col: 1 W293 blank line contains whitespace |
![]() |
|
Col: 1 W293 blank line contains whitespace |
![]() |
|
Col: 9 E265 block comment should start with '# ' |
![]() |
|
Col: 80 E501 line too long (85 > 79 characters) |
![]() |
|
Col: 9 E265 block comment should start with '# ' |
![]() |
|
Col: 80 E501 line too long (80 > 79 characters) |
![]() |
|
Col: 51 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 51 E124 closing bracket does not match visual indentation |
![]() |
|
Col: 80 E501 line too long (85 > 79 characters) |
![]() |
|
Col: 9 E265 block comment should start with '# ' |
![]() |
|
Col: 42 W291 trailing whitespace |
![]() |
|
Col: 51 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 39 W291 trailing whitespace |
![]() |
|
Col: 80 E501 line too long (86 > 79 characters) |
![]() |
|
Can we also check not file_attachment.inactive_review_request.exists()? |
|
|
Can we add a check here that a new query to verify that FileAttachment.get(pk=file_attachment.pk) raises FileAttachment.DoesNotExist? |
|
|
Col: 80 E501 line too long (86 > 79 characters) |
![]() |
|
Can you surround the conditional with parens instead of using the continuation character? |
|
|
This should be a comment, not a multiline string (prefix each line with #). |
|

Summary: |
|
||||||
---|---|---|---|---|---|---|---|
Testing Done: |
|

-
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) 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) Conditionals in python don't require surrounding parentheses.
We should also use
.exists()
instead oflen(... .all())
:if not file_attachment.review_request.exists():
-
reviewboard/webapi/resources/base_file_attachment.py (Diff revision 2) We don't want to delete the draft--there may be other modifications to the review request.
-

-
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) Col: 80 E501 line too long (86 > 79 characters)
-
-
-
reviewboard/webapi/resources/base_file_attachment.py (Diff revision 3) 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.

-
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) Col: 80 E501 line too long (86 > 79 characters)
Testing Done: |
|
---|
Description: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+34) |

-
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) Col: 80 E501 line too long (86 > 79 characters)
-
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 5) Col: 80 E501 line too long (80 > 79 characters)
-
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 5) Col: 52 E128 continuation line under-indented for visual indent
-
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 5) Col: 52 E124 closing bracket does not match visual indentation
-
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 5) Col: 1 W293 blank line contains whitespace
-
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 5) Col: 1 W293 blank line contains whitespace
-
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 5) Col: 9 E265 block comment should start with '# '

-
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) Col: 80 E501 line too long (85 > 79 characters)
-
reviewboard/webapi/resources/base_file_attachment.py (Diff revision 6) Col: 9 E265 block comment should start with '# '
-
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 6) Col: 80 E501 line too long (80 > 79 characters)
-
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 6) Col: 51 E128 continuation line under-indented for visual indent
-
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 6) Col: 51 E124 closing bracket does not match visual indentation

-
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) Col: 80 E501 line too long (85 > 79 characters)
-
reviewboard/webapi/resources/base_file_attachment.py (Diff revision 7) Col: 9 E265 block comment should start with '# '
-
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 7) Col: 42 W291 trailing whitespace
-
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 7) Col: 51 E128 continuation line under-indented for visual indent
-
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 7) Col: 39 W291 trailing whitespace

-
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) Col: 80 E501 line too long (86 > 79 characters)
-
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) Can we also check
not file_attachment.inactive_review_request.exists()
? -
reviewboard/webapi/tests/test_file_attachment_draft.py (Diff revision 8) Can we add a check here that a new query to verify that
FileAttachment.get(pk=file_attachment.pk)
raisesFileAttachment.DoesNotExist
?

-
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) Col: 80 E501 line too long (86 > 79 characters)

-
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) Can you surround the conditional with parens instead of using the continuation character?

-
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: |
|
---|
-
The bug number should go in the "Bugs" field, not in the description.
-
reviewboard/webapi/resources/base_file_attachment.py (Diff revision 11) This should be a comment, not a multiline string (prefix each line with #).
Description: |
|
||||||
---|---|---|---|---|---|---|---|
Bugs: |
|
||||||
Diff: |
Revision 12 (+7) |

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