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

Review Request #7879 — Created Jan. 16, 2016 and submitted

weijie
Review Board
master
4054
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

reviewbotreviewbot

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

reviewbotreviewbot

Add a blank line before this. This comment also doesn't make sense. We should say something along the lines of: ...

daviddavid

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

daviddavid

We don't want to delete the draft--there may be other modifications to the review request.

daviddavid

Add a blank line after this.

daviddavid

Blank line between these.

chipx86chipx86

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

reviewbotreviewbot

Make sure to add a unit test covering the case of a draft file attachment that was not published, to ...

chipx86chipx86

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 52 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 52 E124 closing bracket does not match visual indentation

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

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

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

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

reviewbotreviewbot

Col: 51 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 51 E124 closing bracket does not match visual indentation

reviewbotreviewbot

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

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

Col: 42 W291 trailing whitespace

reviewbotreviewbot

Col: 51 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 39 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

daviddavid

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

daviddavid

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

reviewbotreviewbot

Can you surround the conditional with parens instead of using the continuation character?

daviddavid

This should be a comment, not a multiline string (prefix each line with #).

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/base_file_attachment.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/base_file_attachment.py
    
    
  2.  local variable 'result' is assigned to but never used
    
  3. Col: 80
     E501 line too long (84 > 79 characters)
    
  4. 
      
WE
WE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/base_file_attachment.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/base_file_attachment.py
    
    
  2. 
      
david
  1. 
      
  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.
    
  3. Conditionals in python don't require surrounding parentheses.

    We should also use .exists() instead of len(... .all()):

    if not file_attachment.review_request.exists():
    
  4. We don't want to delete the draft--there may be other modifications to the review request.

  5. Add a blank line after this.

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

  2. 
      
WE
WE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/base_file_attachment.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/base_file_attachment.py
    
    
  2. Col: 80
     E501 line too long (86 > 79 characters)
    
  3. 
      
chipx86
  1. 
      
  2. Blank line between these.

  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.

  4. 
      
WE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/base_file_attachment.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/base_file_attachment.py
    
    
  2. Col: 80
     E501 line too long (86 > 79 characters)
    
  3. 
      
brennie
  1. 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).

  2. 
      
WE
WE
reviewbot
  1. 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
    
    
  2. Col: 80
     E501 line too long (86 > 79 characters)
    
  3. Col: 80
     E501 line too long (80 > 79 characters)
    
  4. Col: 52
     E128 continuation line under-indented for visual indent
    
  5. Col: 52
     E124 closing bracket does not match visual indentation
    
  6. Col: 1
     W293 blank line contains whitespace
    
  7. Col: 1
     W293 blank line contains whitespace
    
  8. Col: 9
     E265 block comment should start with '# '
    
  9. 
      
WE
reviewbot
  1. 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
    
    
  2. Col: 80
     E501 line too long (85 > 79 characters)
    
  3. Col: 9
     E265 block comment should start with '# '
    
  4. Col: 80
     E501 line too long (80 > 79 characters)
    
  5. Col: 51
     E128 continuation line under-indented for visual indent
    
  6. Col: 51
     E124 closing bracket does not match visual indentation
    
  7. 
      
WE
reviewbot
  1. 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
    
    
  2. Col: 80
     E501 line too long (85 > 79 characters)
    
  3. Col: 9
     E265 block comment should start with '# '
    
  4. Col: 42
     W291 trailing whitespace
    
  5. Col: 51
     E128 continuation line under-indented for visual indent
    
  6. Col: 39
     W291 trailing whitespace
    
  7. 
      
WE
reviewbot
  1. 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
    
    
  2. Col: 80
     E501 line too long (86 > 79 characters)
    
  3. 
      
WE
david
  1. This is looking pretty good! Just a couple comments to bullet-proof things even further.

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

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

    2. Sorry. You are right.

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

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

    2. It'll be something like:

      with self.assertRaises(FileAttachment.DoesNotExist):
          FileAttachment.get(pk=file_attechment.pk)
      
  4. 
      
WE
reviewbot
  1. 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
    
    
  2. Col: 80
     E501 line too long (86 > 79 characters)
    
    1. 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?

  3. 
      
WE
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. Can you surround the conditional with parens instead of using the continuation character?

  3. 
      
WE
reviewbot
  1. 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
    
    
  2. 
      
WE
david
  1. The bug number should go in the "Bugs" field, not in the description.

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

    This should be a comment, not a multiline string (prefix each line with #).

  3. 
      
WE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/base_file_attachment.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/base_file_attachment.py
    
    
  2. 
      
david
  1. 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!

  2. 
      
WE
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (9d5f2e1)
Loading...