Allow comment creators to fix/drop

Review Request #3837 — Created Feb. 3, 2013 and discarded

Information

nh2
Review Board
master

Reviewers

Allow comment creators to fix/drop

So far, only the owner of the review request can fix/drop issues.

This commit allows the creator of a comment to fix/close it themselves.
Fixes #2888.

API: Allow comment *creators* to fix/drop

UI: Show Fixed/Drop buttons for issue creators
Tried with local instance:
- review owner can still fix/drop issues
- issue creators can successfully fix/drop issues
- other people can not fix/drop issues as before

All tests pass.


I updated the following tests to work again and also test if the above three properties hold:
- test_update_diff_comment_issue_status
- test_update_screenshot_comment_issue_status
Description From Last Updated

*Should* always succeed, but that doesn't mean it will. Always check and log. Nitpick, but comments should always end with …

chipx86chipx86

This should be using comment.get_review(), which we also call below. We should only fetch that once.

chipx86chipx86

I feel we're putting too much business logic now in the template tag. Ideally, this should all be determined in …

chipx86chipx86

We're determining a bool and then later converting to a string. Just do an if statement here and set interactive …

chipx86chipx86

I think it is better to prepare a variable's value first then return the variable, instead of having a if …

GR gregwym

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

reviewbotreviewbot

You're doing this twice.

chipx86chipx86

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

reviewbotreviewbot

This logic is different from the above logic. Don't check this stuff here, or compute comment_creator, or any of that. …

chipx86chipx86

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

reviewbotreviewbot

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

reviewbotreviewbot

Docstrings must follow this convention: """One-line summary Multi-line description. """ or: """One-line summary""" Line must be < 80 chars.

chipx86chipx86

This information should be explained in the docstring.

chipx86chipx86

A tip that Django won't tell you about but will save a query is to not compare against things like …

chipx86chipx86

The first two checks are free, and the third can have up to 3 queries (for review, review request, and …

chipx86chipx86

Not a fan of this syntax. It's a bit weird to read, and it makes it harder to add any …

chipx86chipx86

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

reviewbotreviewbot

"a user"

chipx86chipx86

This is a new test.

chipx86chipx86

Trailing period.

chipx86chipx86

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

reviewbotreviewbot

Also a new test.

chipx86chipx86

Too much copy/paste. Let's limit the check to where we're putting the boolean. (It's okay for that to exceed 80 …

chipx86chipx86

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

We don't usually have a whole post-condition description, since that makes the test output extremely long. I'd say just do …

chipx86chipx86

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

reviewbotreviewbot

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

reviewbotreviewbot

"... with owner trying to change a published issue"

chipx86chipx86

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

reviewbotreviewbot

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

reviewbotreviewbot

This is a new test.

chipx86chipx86

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

reviewbotreviewbot

"... with changing an issue and no permission"

chipx86chipx86

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Similar comments to above. In fact, at this point, all my previous comments repeat.

chipx86chipx86

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/webapi/resources.py
        reviewboard/reviews/templatetags/reviewtags.py
      Ignored Files:
    
    
  2. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (88 > 79 characters)
    
  3. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  4. 
      
GR
  1. 
      
  2. I think it is better to prepare a variable's value first then return the variable, instead of having a if statement within return. 
  3. reviewboard/webapi/resources.py (Diff revision 1)
     
     
     
    Can a specific user be deleted? I doubt that. 
  4. 
      
chipx86
  1. 
      
  2. *Should* always succeed, but that doesn't mean it will. Always check and log.
    
    Nitpick, but comments should always end with a period. (And be valid sentences.)
    1. Changed to get_review() as suggested below.
  3. This should be using comment.get_review(), which we also call below. We should only fetch that once.
  4. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 1)
     
     
     
     
     
     
     
     
    I feel we're putting too much business logic now in the template tag. Ideally, this should all be determined in a can_change_issue_status() function in the BaseComment model.
  5. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 1)
     
     
     
     
    We're determining a bool and then later converting to a string. Just do an if statement here and set interactive to be 'true' or 'false' from that.
  6. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    You're doing this twice.
  7. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    This logic is different from the above logic.
    
    Don't check this stuff here, or compute comment_creator, or any of that. Instead, use the aforementioned new function.
  8. 
      
NH
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/webapi/resources.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/models.py
      Ignored Files:
    
    
  2. reviewboard/reviews/models.py (Diff revision 2)
     
     
    Col: 16
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/reviews/models.py (Diff revision 2)
     
     
    Col: 16
     E128 continuation line under-indented for visual indent
    
  4. 
      
NH
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/webapi/resources.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/models.py
      Ignored Files:
    
    
  2. 
      
NH
NH
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/webapi/tests.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/models.py
      Ignored Files:
    
    
    1. lol, I got these as well. 
      SublimeLinter disable the `E501` check by default, you can enable it in package specified user preferences. 
      
      	{
      	    /*
      	        A list of pep8 error numbers to ignore. By default "line too long" errors are ignored.
      	        The list of error codes is in this file: https://github.com/jcrocholl/pep8/blob/master/pep8.py.
      	        Search for "Ennn:", where nnn is a 3-digit number.
      	    */
      	    "pep8_ignore": []
      	}
      
      
  2. reviewboard/webapi/tests.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/webapi/tests.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/models.py (Diff revision 4)
     
     
     
     
     
    Docstrings must follow this convention:
    
        """One-line summary
    
        Multi-line description.
        """
    
    or:
    
        """One-line summary"""
    
    Line must be < 80 chars.
    1. Few of the other docstrings in this file follow this convention.
      
      For example, just above:
      
          def get_pending_reply(self, user):
              """
              Returns the pending reply to this review owned by the specified
              user, if any.
              """
    2. This is probably the oldest file in the codebase. A lot of these docstrings predate the standards we have today.
    3. Done.
  3. reviewboard/reviews/models.py (Diff revision 4)
     
     
     
     
    This information should be explained in the docstring.
    1. I would disagree: Docstrings usually describe what the functionality of a method is, e.g. "says if the given user can change it or not". Our choices on who exactly can change them are not part of the API, they are an implementation choice.
      
      Docstrings are part of the API; putting this in the docstring would mean the API changes when we decide to let more/less people change the issue, although the API didn't change at all.
    2. I disagree. This is exactly the kind of thing that should be explained by the docstring. If we're saying "if you use can change it or not," it helps to know what those conditions are without digging into the code. That's important information.
  4. reviewboard/reviews/models.py (Diff revision 4)
     
     
     
     
     
    A tip that Django won't tell you about but will save a query is to not compare against things like review_request.submitter. Instead, compare the IDs ('pk' variable). So:
    
        if user.pk == self.get_review().user_id:
    
    
    On top of this, the users_allowed_change trick is cute, but inefficient. We'll fetch both objects up-front, meaning we introduce 2 SQL queries, always. Instead, we should check each one individually, so that optimistically, we only have one query.
    
    We should start the check with the user that is most likely going to be making the change. For most uses out there, this is going to be the review request's submitter, so check that first.
    1. Fixed by change for comment below.
  5. reviewboard/reviews/models.py (Diff revision 4)
     
     
     
     
    The first two checks are free, and the third can have up to 3 queries (for review, review request, and submitter). We should do the first two checks first, and if they fail, bail.
    1. Done. It is much cleaner now.
  6. Not a fan of this syntax. It's a bit weird to read, and it makes it harder to add any logic to that without awkward wrapping. IN general, I prefer a standard if/else instead.
    
    Now in this case, the fact that this was computing "true" and "false" was wrong. That means this code knows exactly how it's going to be used in the template (as a JavaScript value). Instead, this should pass the bool, and the template should be converting to the right value.
  7. reviewboard/webapi/tests.py (Diff revision 4)
     
     
    "a user"
  8. reviewboard/webapi/tests.py (Diff revision 4)
     
     
     
     
     
     
    This is a new test.
    1. Are you sure?
      
      test_update_diff_comment_issue_status() until now tested who can update the issue status and who not. This is still the case.
      
      If we don't put this here the test only tests diff-comments partially; we'd have to change
      
          """Testing the PUT review-requests/<id>/reviews/<id>/diff-comments/<id> API with an issue"""
      
      to
      
          """Testing some assertions about the PUT review-requests/<id>/reviews/<id>/diff-comments/<id> API with an issue"""
      
      and make another test with the docstring
      
          """Testing some other assertions about the PUT review-requests/<id>/reviews/<id>/diff-comments/<id> API with an issue"""
      
      and write somewhere that these two *together* fully specify what the diff-comments should do / not do.
    2. We're testing separate conditions for the API. There's actually three tests in this function that should have been split up.
      
      1) We're testing PUT with issue_status cannot be set when the comment is unpublished. ("Testing ... with an issue and unpublished comment")
      2) We're testing PUT with issue_status can be set by the issue creator. ("Testing ... with an issue and by issue creator")
      3) We're testing PUT with issue_status cannot be set by users who are neither the issue creator nor review request owner. ("Testing ... with an issue and user without permissions")
      
      If a unit test has to say "Now let's do...", then there's a big red flag. Unit tests should be testing one concrete thing (things like setting up the initial state doesn't count, but rather the real call we're hoping to test). With our API code, that means we're testing, in the end, one concrete GET/POST/PUT/DELETE with a set of preconditions (parameters, logged in user, etc.).
    3. Done. I actually did split it up into 3 for this change, and also pulled out quite some code duplication from the tests around it.
      
      Please tell me if you like the naming/docstrings.
  9. reviewboard/webapi/tests.py (Diff revision 4)
     
     
    Trailing period.
  10. reviewboard/webapi/tests.py (Diff revision 4)
     
     
     
     
     
     
    Also a new test.
    1. See question in issue above.
  11. 
      
NH
NH
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/webapi/tests.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/templates/reviews/comment_issue.html
    
    
  2. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (95 > 79 characters)
    
  3. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (105 > 79 characters)
    
  4. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (95 > 79 characters)
    
  5. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (166 > 79 characters)
    
  6. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (95 > 79 characters)
    
  7. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (206 > 79 characters)
    
  8. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (94 > 79 characters)
    
  9. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  10. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (161 > 79 characters)
    
  11. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (94 > 79 characters)
    
  12. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  13. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (101 > 79 characters)
    
  14. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (106 > 79 characters)
    
  15. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (101 > 79 characters)
    
  16. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (172 > 79 characters)
    
  17. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (101 > 79 characters)
    
  18. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (212 > 79 characters)
    
  19. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (100 > 79 characters)
    
  20. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  21. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (167 > 79 characters)
    
  22. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (100 > 79 characters)
    
  23. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (92 > 79 characters)
    
  24. 
      
chipx86
  1. Looking good, but I want to go for more consistency with the new tests and our existing ones. While your new ones are very descriptive, we don't really need that level of descriptiveness in the test strings.
    
    What I'd recommend is having all the extra precondition/postcondition explanation be comments at the top of the function. That keeps the test output more scannable, and yet makes it more clear what's expected in the test functions when they're read (if something fails, for instance).
    1. Also, go through ReviewBot's output. The docstring lines can be ignored, but *all* Python code should be < 80 chars per line.
  2. reviewboard/templates/reviews/comment_issue.html (Diff revision 5)
     
     
     
     
     
     
    Too much copy/paste. Let's limit the check to where we're putting the boolean. (It's okay for that to exceed 80 chars.)
  3. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    We don't usually have a whole post-condition description, since that makes the test output extremely long. I'd say just do "... with trying to change an unpublished issue"
  4. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    "... with owner trying to change a published issue"
  5. reviewboard/webapi/tests.py (Diff revision 5)
     
     
     
     
     
     
    This is a new test.
  6. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    "... with changing an issue and no permission"
  7. reviewboard/webapi/tests.py (Diff revision 5)
     
     
    Similar comments to above.
    
    In fact, at this point, all my previous comments repeat.
  8. 
      
david
  1. I've gone through and created an updated version of this change at https://reviews.reviewboard.org/r/4380/
  2. 
      
NH
Review request changed

Status: Discarded

Loading...