Support attached comment for reviews submitions

Review Request #2338 — Created May 5, 2011 and submitted

Information

Review Board

Reviewers

* Added supported for close comment. After a review request is marked "submitted" or "discarded", submitter can add text to describe the submission.
* Fixed a bug that submitting review request will trash unpublished draft. Now, a confirm windows will popup to remind users.
* Showed submission date for submitted review request under summary.

TODO:
* Maybe rbtools support is needed.
Manually tested and unit tested.

The unit test output attached. Some failed and errors happened, but it looks like caused by windows environment. In fact, the output is the same before and after this commit.

Description From Last Updated

If you build this data dynamically, based on whether options.comment is defined, then we can simplify all other call sites …

chipx86chipx86

If the above is done, this could go away.

chipx86chipx86

Can you change this to: "You have an unpublished draft. If you close this review request, the draft will be …

chipx86chipx86

This too.

chipx86chipx86

This can be simplified: changedesc = ChangeDescription(public=True, text=comment) The timestamp will already be datetime.now() by default, so you're good there. …

chipx86chipx86

Not needed. That's the default. Also, blank line after this.

chipx86chipx86

Shouldn't need the class. Any CSS/JavaScript can just reference the ID.

chipx86chipx86

Trailing whitespace.

chipx86chipx86

Trailing comma.

chipx86chipx86

No spaces around the = when dealing with parameters to a function.

chipx86chipx86

Sentence casing, with a period.

chipx86chipx86

The wrapping is weird. Rather than passing the field to latest(), I'd just modify changedescs/models.py:ChangeDescription.Meta to have: get_latest_by = 'timestamp'

chipx86chipx86

Sentence casing, with a period.

chipx86chipx86

Can we change this to "description"? I don't want to really mix definitions of "comment", since it usually means a …

chipx86chipx86
chipx86
  1. 
      
  2. Which Description is this? The submit message? I think we need to rename it if so, or somehow change how we show this.
    
    For now, maybe just "Close Description" ?
  3. It would be awesome if we showed the message below this. Sort of like the review request draft banner, but not editable. Or actually... Rather than the edit box introduced in this change, this might be a better workflow:
    
    1) Click Close -> Submitted
    2) Page changes to this (no big edit box first).
    3) The submit banner has an editable field for the description (much like review request draft banners).
    
    The field would be different than the standard fields. It'd be more of an instant apply. No draft. Change it and everyone sees it. For the ChangeDescription model, we could just do:
    
    
    changedesc = latest change description;
    
    if changedesc has field 'status':
        set changedesc.close_description;
    else:
        create new changedesc with close message.
    
    
    That means if you were to set a new close description, and there's already a change description for it, any modifications to that close description would just overwrite what's already in there. That would happen until some other change descrption has been created (new diff, field changes), in which case we just make a new one.
    
    I actually think this is the way to go. No extra work for those who just want to simply close a review request, and yet it's very straight-forward to update the description once submitted (even for old review requests).
  4. 
      
HO
chipx86
  1. Thanks for the changes! Good progress. There's a few more things with the new code, but nothing big.
    
    As for the change to tests.py, make sure your branch is fully synced with master. I just made a change to tests.py and I think you're not synced correctly.
  2. Show all issues
    If you build this data dynamically, based on whether options.comment is defined, then we can simplify all other call sites by not passing 'comment: ""'
    
    So:
    
        data = {
            status: statusType
        };
    
        if (options.comment !== undefined) {
            data.comment = options.comment;
        }
    
        self._apiCall({
            ...
            data: data,
            ...
         });
  3. Show all issues
    If the above is done, this could go away.
  4. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 2)
     
     
     
    Show all issues
    Can you change this to:
    
    "You have an unpublished draft. If you close this review request, the draft will be discarded. Are you sure you want to close the review request?"
  5. Show all issues
    This too.
  6. reviewboard/reviews/models.py (Diff revision 2)
     
     
     
     
     
     
     
    Show all issues
    This can be simplified:
    
        changedesc = ChangeDescription(public=True,
                                       text=comment)
    
    The timestamp will already be datetime.now() by default, so you're good there.
    
    I think comment will work if None is passed, but if not, that could be:
    
        text=comment or ''
  7. reviewboard/reviews/models.py (Diff revision 2)
     
     
    Show all issues
    Not needed. That's the default.
    
    Also, blank line after this.
  8. Show all issues
    Shouldn't need the class. Any CSS/JavaScript can just reference the ID.
  9. Show all issues
    Trailing whitespace.
  10. reviewboard/webapi/resources.py (Diff revision 2)
     
     
     
    You know... I'm curious. How hard would it be to implement this feature for discarded review requests too? Could be useful. Like, "This is discarded because it's implemented a different way in /r/123" If it's not a ton of work, I think it'd be really handy.
    1. Absolutely. It should be easy like a free ride. I will include that as well.
  11. 
      
HO
chipx86
  1. 
      
  2. Show all issues
    Trailing comma.
  3. reviewboard/reviews/models.py (Diff revision 3)
     
     
    Show all issues
    No spaces around the = when dealing with parameters to a function.
  4. reviewboard/reviews/models.py (Diff revision 3)
     
     
    Show all issues
    Sentence casing, with a period.
  5. reviewboard/reviews/models.py (Diff revision 3)
     
     
     
    Show all issues
    The wrapping is weird.
    
    Rather than passing the field to latest(), I'd just modify changedescs/models.py:ChangeDescription.Meta to have:
    
        get_latest_by = 'timestamp'
  6. reviewboard/reviews/models.py (Diff revision 3)
     
     
    Show all issues
    Sentence casing, with a period.
  7. 
      
chipx86
  1. 
      
  2. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Show all issues
    Can we change this to "description"? I don't want to really mix definitions of "comment", since it usually means a comment on a diff/screenshot/file in RB.
  3. 
      
HO
Review request changed
david
  1. This is looking pretty great. I have just one request: can we also allow adding comments when discarding a review? It would be really nice to add a comment explaining why it was dropped.
    1. Hi David.
      Discard comment is already included. You can see the diff of r2 and r3 for details.
    2. I tested it out with your r4 patch and it wasn't there...
  2. 
      
david
  1. Sorry about the confusion regarding "Discard". Pushed to master as 0a7c2f8. Thanks so much!
  2.