Add closing/reopening signals

Review Request #7332 — Created May 25, 2015 and discarded

smacleod
Review Board
release-2.0.x
reviewboard
mcote
Fire "review_request_closing" when attempting to close a review request.
If a signal receiver raises a CloseError abort the close operation.
This signal allows extensions to prevent a review request from being
closed. Alert the user in the UI when a close request has failed.

Also introduce a new "review_request_reopening" signal which behaves the
same way as the closing signal but deals with reopening review requests.

These new signals expand upon publishing signals that were previously
added to allow extensions control over publishing - see commit
ab84ccdf3d5bb883f18d34133c30af410c7ff1d2

Wrote a dummy extension to raise the new errors inside of the new
signals - API requests were properly blocked and the error messages
were alerted to the user.

Compared change description behaviour to Review Board without this
change and observed it to be unchanged after the patch.

Description From Last Updated

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Can you add docstrings to these? I kind of wish we had prefixed all the others with "ReviewRequest," to differentiate ...

chipx86chipx86

You shoul use super(CloseError, self).__init__(msg) here. Same in ReopenError

brenniebrennie

Can we do something more graceful than an alert ?

brenniebrennie

Can we have this be "... while closing the review request." ?

chipx86chipx86

Same here with reopening.

chipx86chipx86

I want to make sure this doesn't change the semantics around any piece of draft/changedescription creation, since we're reordering the ...

chipx86chipx86

"An error that occurs while..."

brenniebrennie

"An error that occurs while attempting..."

brenniebrennie

Single quotes.

brenniebrennie

Single quotes.

brenniebrennie

Exceptions take a message string in the arguments by default, so we should work with that and not have our ...

chipx86chipx86

Since this is a human-readable string, I'd prefer something less terse, like 'Error closing the review request: %s' I also ...

chipx86chipx86

While here, can you put these in alphabetical order?

chipx86chipx86

Let's do one keyword argument per line, to help with readability.

chipx86chipx86

Here too.

chipx86chipx86

Should use single quotes. Here and the new one below (or all of them). Also, we're starting to add docs ...

chipx86chipx86

Same here.

chipx86chipx86

With the above changes, this would be: return CLOSE_ERROR.with_message(six.text_type(e))

chipx86chipx86
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/signals.py
        reviewboard/reviews/errors.py
        reviewboard/reviews/models/review_request.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/signals.py
        reviewboard/reviews/errors.py
        reviewboard/reviews/models/review_request.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. reviewboard/reviews/errors.py (Diff revision 1)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. reviewboard/reviews/errors.py (Diff revision 1)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. reviewboard/reviews/signals.py (Diff revision 1)
     
     
    Col: 48
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/reviews/signals.py (Diff revision 1)
     
     
    Col: 48
     E128 continuation line under-indented for visual indent
    
  6. 
      
SM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/signals.py
        reviewboard/reviews/errors.py
        reviewboard/reviews/models/review_request.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/signals.py
        reviewboard/reviews/errors.py
        reviewboard/reviews/models/review_request.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/reviews/errors.py (Diff revision 2)
     
     
     

    You shoul use super(CloseError, self).__init__(msg) here.

    Same in ReopenError

  3. Can we do something more graceful than an alert ?

    1. I'm just keeping with the convention of the other errors like this for publishing. Going to drop this, we can always iterate on better reporting.

  4. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/errors.py (Diff revision 2)
     
     

    Can you add docstrings to these?

    I kind of wish we had prefixed all the others with "ReviewRequest," to differentiate between publishing other sorts of things. Wonder if we should start that precedence here or not...

    1. Currently PublishError is used for both review requests and reviews, so that's probably why we didn't prefix. I'm going to not prefix these either but if you feel strongly going forward we can quickly do it.

    2. That's fine. I kind of think they should have the prefix, but none of the others do, so we'd have to change them all anyway, breaking extension compatibility. Better to be consistent for now, and break things all at once later.

  3. reviewboard/webapi/errors.py (Diff revision 2)
     
     

    Can we have this be "... while closing the review request." ?

  4. reviewboard/webapi/errors.py (Diff revision 2)
     
     

    Same here with reopening.

  5. reviewboard/webapi/resources/review_request.py (Diff revision 2)
     
     
     
     
     

    I want to make sure this doesn't change the semantics around any piece of draft/changedescription creation, since we're reordering the operations here. I went through and I think it's okay, and we might actually have been leaking a ChangeDescription entry in the database before that this would fix. Still, to be sure, can you do some testing around:

    • Discarding an unpublished review request, and reopening it.
    • Discarding a published review request, and reopening it.
    • Closing a published review request as submitted, and reopening it.

    For each of these, make sure there's no change description sitting there on the page after, and then try publishing and make sure you see the expected change description.

    1. Ya, I traced through all the code relavant to this and was pretty sure it wouldn't be an issue. I believe I did test all those cases but I'll make sure.

    2. Before and after this patch Review Board behaves the same way with respect to what change descriptions are shown. That behaviour is however not what you described (change descriptions are left around in some cases). Since I'm not actually modifying the current behaviour I'd really prefer to just land this without fixing it all so that it can be part of the upcoming release.

    3. Yep, totally fine. We should file a bug though.

  6. 
      
SM
SM
SM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/signals.py
        reviewboard/reviews/errors.py
        reviewboard/reviews/models/review_request.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/signals.py
        reviewboard/reviews/errors.py
        reviewboard/reviews/models/review_request.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
brennie
  1. Just a few nits. Looks good to me!

  2. reviewboard/reviews/errors.py (Diff revision 3)
     
     
     

    "An error that occurs while..."

  3. reviewboard/reviews/errors.py (Diff revision 3)
     
     

    "An error that occurs while attempting..."

  4. reviewboard/webapi/errors.py (Diff revision 3)
     
     

    Single quotes.

    1. The entire file uses double quotes here, so I'm going to just keep it consistent.

  5. reviewboard/webapi/errors.py (Diff revision 3)
     
     

    Single quotes.

  6. 
      
SM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/signals.py
        reviewboard/reviews/errors.py
        reviewboard/reviews/models/review_request.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/signals.py
        reviewboard/reviews/errors.py
        reviewboard/reviews/models/review_request.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/errors.py (Diff revision 4)
     
     
     
     
     

    Exceptions take a message string in the arguments by default, so we should work with that and not have our own msg field. (The others probably should be updated to do the right thing here as well.)

  3. reviewboard/reviews/errors.py (Diff revision 4)
     
     
     

    Since this is a human-readable string, I'd prefer something less terse, like 'Error closing the review request: %s'

    I also don't think we should be overriding __str__ on any of these. Instead, we should do what other exceptions do and build a finalized string in __init__, like:

    def __init__(self, msg):
        super(CloseError, self).__init__(
            'Error closing the review request: %s' % msg)
    

    These comments all apply to the other exceptions as well.

  4. reviewboard/reviews/models/review_request.py (Diff revision 4)
     
     
     
     
     
     

    While here, can you put these in alphabetical order?

  5. Let's do one keyword argument per line, to help with readability.

  6. Here too.

  7. reviewboard/reviews/signals.py (Diff revision 4)
     
     
     
     

    Should use single quotes. Here and the new one below (or all of them).

    Also, we're starting to add docs for all these sorts of things, for the new codebase docs. Can you prefix these with things like:

    #: Emitted when a review request is about to be closed.
    #:
    #: <whatever other details>
    #:
    #: Args:
    #:     user (User):
    #:         <description of user>
    #:
    #:     review_request (ReviewRequest):
    #:         <description of review request>
    #:
    #: ...
    

    This will result in a nicely-formatted set of docs. (The above will be parsed by a Sphinx extension we're now using to produce proper argument docs with types.)

    If these can return a value, then add to the end:

    #: Returns:
    #:     <type>: <description>
    
  8. reviewboard/reviews/signals.py (Diff revision 4)
     
     

    Same here.

  9. With the above changes, this would be:

    return CLOSE_ERROR.with_message(six.text_type(e))
    
  10. 
      
SM
Review request changed

Status: Discarded

Loading...