Summary

Add closing/reopening signals

Review Request #7332 — Created May 25, 2015 and discarded June 16, 2015, 6:16 p.m.

Information

Owner

smacleod

Repository

Review Board

Branch

release-2.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

mcote

Description

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

Testing Done

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.

Issues

Description	From	Last Updated
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	May 25, 2015, 1:34 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	May 25, 2015, 1:34 p.m.
Col: 48 E128 continuation line under-indented for visual indent	reviewbot	May 25, 2015, 1:34 p.m.
Col: 48 E128 continuation line under-indented for visual indent	reviewbot	May 25, 2015, 1:34 p.m.
Can you add docstrings to these? I kind of wish we had prefixed all the others with "ReviewRequest," to differentiate …	chipx86	June 3, 2015, 9:51 a.m.
You shoul use super(CloseError, self).__init__(msg) here. Same in ReopenError	brennie	June 3, 2015, 9:58 a.m.
Can we do something more graceful than an alert ?	brennie	June 3, 2015, 9:38 a.m.
Can we have this be "... while closing the review request." ?	chipx86	June 3, 2015, 9:52 a.m.
Same here with reopening.	chipx86	June 3, 2015, 9:52 a.m.
I want to make sure this doesn't change the semantics around any piece of draft/changedescription creation, since we're reordering the …	chipx86	June 3, 2015, 10:11 a.m.
"An error that occurs while..."	brennie	June 3, 2015, 5:42 p.m.
"An error that occurs while attempting..."	brennie	June 3, 2015, 5:42 p.m.
Single quotes.	brennie	June 3, 2015, 5:42 p.m.
Single quotes.	brennie	June 3, 2015, 5:42 p.m.
Exceptions take a message string in the arguments by default, so we should work with that and not have our …	chipx86	June 3, 2015, 5:56 p.m.
Since this is a human-readable string, I'd prefer something less terse, like 'Error closing the review request: %s' I also …	chipx86	June 3, 2015, 5:56 p.m.
While here, can you put these in alphabetical order?	chipx86	June 3, 2015, 5:56 p.m.
Let's do one keyword argument per line, to help with readability.	chipx86	June 3, 2015, 5:56 p.m.
Here too.	chipx86	June 3, 2015, 5:56 p.m.
Should use single quotes. Here and the new one below (or all of them). Also, we're starting to add docs …	chipx86	June 3, 2015, 5:56 p.m.
Same here.	chipx86	June 3, 2015, 5:56 p.m.
With the above changes, this would be: return CLOSE_ERROR.with_message(six.text_type(e))	chipx86	June 3, 2015, 5:56 p.m.

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

reviewboard/reviews/errors.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/reviews/errors.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/reviews/signals.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 48
 E128 continuation line under-indented for visual indent
```
reviewboard/reviews/signals.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 48
 E128 continuation line under-indented for visual indent
```

Change Summary:

Fix Review Bot's issues.

Commit:

56d6cd39739c3a399181a69bfd0439752e339647

9b765260311be8e8542d37e1092a48b8269678f7

Diff:

Revision 2 (+89 -14)

Show changes

	reviewboard/reviews/errors.py
	reviewboard/reviews/signals.py
	reviewboard/reviews/models/review_request.py
	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/webapi/errors.py
	reviewboard/webapi/resources/review_request.py

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

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

The issue has been resolved. Show all issues

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

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2)

The issue has been dropped. Show all issues

Can we do something more graceful than an alert ?

smacleod June 3, 2015, 9:38 a.m.

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.

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

The issue has been resolved. Show all issues

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...

smacleod June 3, 2015, 9:58 a.m.

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.

chipx86 June 3, 2015, 5:57 p.m.

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.

reviewboard/webapi/errors.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we have this be "... while closing the review request." ?
```
reviewboard/webapi/errors.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Same here with reopening.
```

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

The issue has been dropped. Show all issues

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.

smacleod June 3, 2015, 9:58 a.m.

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.

smacleod June 3, 2015, 10:11 a.m.

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.

chipx86 June 3, 2015, 5:57 p.m.

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

Status:: Completed

Status:: Re-opened

Change Summary:

Updated based on reviews and did more testing.

Testing Done:

		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.

Commit:

9b765260311be8e8542d37e1092a48b8269678f7

5d43e217f797aa4e8ba85cb30f6217528ddb79c2

Diff:

Revision 3 (+93 -14)

Show changes

	reviewboard/reviews/errors.py
	reviewboard/reviews/signals.py
	reviewboard/reviews/models/review_request.py
	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/webapi/errors.py
	reviewboard/webapi/resources/review_request.py

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

Ship it!

```
Just a few nits. Looks good to me!
```
reviewboard/reviews/errors.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"An error that occurs while..."
```
reviewboard/reviews/errors.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"An error that occurs while attempting..."
```
reviewboard/webapi/errors.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Single quotes.
```
1. SM
  
  smacleod June 3, 2015, 5:43 p.m.
  The entire file uses double quotes here, so I'm going to just keep it consistent.
reviewboard/webapi/errors.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Single quotes.
```

Change Summary:

Updated based on Barret's review.

Commit:

5d43e217f797aa4e8ba85cb30f6217528ddb79c2

ce68da631a9752f2c3cbc90edd1ffd2077b1e533

Diff:

Revision 4 (+93 -14)

Show changes

	reviewboard/reviews/errors.py
	reviewboard/reviews/signals.py
	reviewboard/reviews/models/review_request.py
	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/webapi/errors.py
	reviewboard/webapi/resources/review_request.py

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

Fix it!

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

An issue was opened. Show all issues

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.)

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

An issue was opened. Show all issues

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.

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

An issue was opened. Show all issues

While here, can you put these in alphabetical order?

reviewboard/reviews/models/review_request.py (Diff revision 4)
An issue was opened. Show all issues
```
Let's do one keyword argument per line, to help with readability.
```
reviewboard/reviews/models/review_request.py (Diff revision 4)
An issue was opened. Show all issues
```
Here too.
```

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

An issue was opened. Show all issues

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>

reviewboard/reviews/signals.py (Diff revision 4)
An issue was opened. Show all issues
```
Same here.
```

reviewboard/webapi/resources/review_request.py (Diff revision 4)

An issue was opened. Show all issues

With the above changes, this would be:

return CLOSE_ERROR.with_message(six.text_type(e))

Status:: Discarded