Summary

Add old status/public states to the review request reopened signal.

Review Request #8724 — Created Feb. 8, 2017 and submitted Feb. 15, 2017, 12:44 a.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-2.5.x

Bugs

Depends On

Commit

0fef63a...

Reviewers

Groups

reviewboard

People

Description

This fleshes out the review request reopened signals to include

information on what the status and public states were prior to

reopening. Consumers can use this to more intelligently determine

whether to take action on the reopen. For instance, it could decide not

to send a message to a service for a review request that was previously

unpublished and is now being reopened, or it could send a different

message when reopened from a discarded state vs. a submitted state.
Unit tests were added to test reopen functionality and resulting states,

including the new signal changes.

Testing Done

Unit tests pass.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/signals.py
    reviewboard/reviews/models/review_request.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/signals.py
    reviewboard/reviews/models/review_request.py

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

For reopening, status and public haven't changed yet, so they're the same as the old values.

Would it perhaps make more sense to provide the new values in the signal so listeners could determine what will happen?

chipx86 Feb. 8, 2017, 2:14 p.m.

I considered this, but intentionally left the values in for consistency and to make it easier to share code between handlers. For instance:

def on_reopened(self, **kwargs):
    self.send_some_message('reopened', **kwargs)

def on_reopening(self, **kwargs):
    self.send_some_message('reopening', **kwargs)

def send_some_message(self, description, user, review_request, old_status, old_public):
    ...


I originally included the new values as well. I got rid of them because the status is always going to be PENDING_REVIEW. However, maybe the public state makes sense (it only changes if coming from a discarded state, which can be inferred from the old status, but that requires knowledge of internals). And maybe if we're including that, the status would also make sense to include, even if it's always going to be a fixed value... Not sure yet.

Ship it!

```
Ship It!
```

Change Summary:

Removed the new parameters from the reopening signal.

Summary:

-	Add old status/public states to the review request reopen signals.
+	Add old status/public states to the review request reopened signal.

Description:

~		This fleshes out the review request reopen signals to include
	~	This fleshes out the review request reopened signals to include
		information on what the status and public states were prior to
		reopening. Consumers can use this to more intelligently determine
		whether to take action on the reopen. For instance, it could decide not
		to send a message to a service for a review request that was previously
		unpublished and is now being reopened, or it could send a different
~		message when reopening from a discarded state vs. a submitted state.
	~	message when reopened from a discarded state vs. a submitted state.

		Unit tests were added to test reopen functionality and resulting states,
		including the new signal changes.

Commit:

-	5dd3881c3c0d2479d5a16fc3b049afe380a3d6e0
+	0fef63a640f428b09c58ec3d407744fdae11c096

Diff:

Revision 2 (+88 -6)

Show changes

	reviewboard/reviews/signals.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/tests/test_review_request.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/signals.py
    reviewboard/reviews/models/review_request.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/tests/test_review_request.py
    reviewboard/reviews/signals.py
    reviewboard/reviews/models/review_request.py

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (afa77f2)