Summary

Add manual run and retry for status updates.

Review Request #11129 — Created Aug. 10, 2020 and submitted Aug. 13, 2020, 10:16 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

baa7313...

Reviewers

Groups

reviewboard

People

Description

One common request for the status updates feature has been to allow
manual or deferred running of tools. This is desirable for a few
different situations, such as infrequently-run or expensive checkers,
test builds, etc. This change adds the framework for that.

The StatusUpdate model has a new state, NOT_YET_RUN, which will show
a button that will trigger the action. This requires that the caller (an
extension) listen to the new status_update_request_run signal to
actually run the tool.

Because it is very similar in its implementation, this additionally adds
an ability for the creator of the status update to indicate that it can
be retried. The Retry action will be shown if the status update is
marked as retryable (via extra_data), and the result of the status
update is in either error or timeout states. Like the "Run" action,
this requires handling the new signal to actually run the tool when the
user clicks the button.

The StatusUpdate API can now accept request-run in the state field,
which will trigger the new signal. This allows the frontend (or
third-party callers) to trigger the run/retry action.

Based on work by Alex Klemenchuk at /r/10311/

Testing Done


Ran python and JS unit tests.
Used in conjunction with a Review Bot change to use these features.

Issues

Description	From	Last Updated
Col: 15 'localSitePrefix' is defined but never used.	reviewbot	Aug. 10, 2020, 2:49 p.m.
Col: 49 Missing semicolon.	reviewbot	Aug. 10, 2020, 2:50 p.m.
E703 statement ends with a semicolon	reviewbot	Aug. 10, 2020, 2:49 p.m.
E703 statement ends with a semicolon	reviewbot	Aug. 10, 2020, 2:49 p.m.
Can you add: ... Type: bool	chipx86	Aug. 10, 2020, 3:44 p.m.
Can you add: ... Type: unicode	chipx86	Aug. 10, 2020, 3:44 p.m.
"ID"	chipx86	Aug. 10, 2020, 3:44 p.m.
Can you swap these so they'll be in alphabetical order?	chipx86	Aug. 10, 2020, 3:44 p.m.
Instead of checking if this is in kwargs and then fetching this in two spots, how about pulling it out …	chipx86	Aug. 10, 2020, 3:45 p.m.
Hmm, we should localize this, though I noticed we don't elsewhere. In standard forms, they're typically localized and have trailing …	chipx86	Aug. 10, 2020, 3:46 p.m.
Should use assertSpyCalledWith().	chipx86	Aug. 10, 2020, 4:04 p.m.
Should use assertSpyCalledWith().	chipx86	Aug. 10, 2020, 4:04 p.m.
Should use assertIn(). Though, can we check the explicit value, instead of just the presence?	chipx86	Aug. 10, 2020, 4:06 p.m.
Same as above.	chipx86	Aug. 10, 2020, 4:06 p.m.
E703 statement ends with a semicolon	reviewbot	Aug. 10, 2020, 4:16 p.m.
E703 statement ends with a semicolon	reviewbot	Aug. 10, 2020, 4:16 p.m.

flake8 failed.

JSHint failed.

flake8

reviewboard/webapi/tests/test_status_update.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E703 statement ends with a semicolon
```
reviewboard/webapi/tests/test_status_update.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E703 statement ends with a semicolon
```

JSHint

reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 15
 'localSitePrefix' is defined but never used.
```
reviewboard/static/rb/js/reviewRequestPage/views/tests/baseStatusUpdatesEntryViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 49
 Missing semicolon.
```

Commit:

-	f49e00cbb7d15236c5f720d41a05c10b64424628
+	d0ff77c3bdbd970248e380669b47c865b62fb6a5

Diff:

Revision 2 (+419 -16)

Show changes

	reviewboard/reviews/detail.py
	reviewboard/reviews/signals.py
	reviewboard/reviews/models/status_update.py
	reviewboard/reviews/tests/test_entries.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/reviewRequestPage/models/statusUpdatesEntryModel.es6.js
	reviewboard/static/rb/js/reviewRequestPage/models/tests/statusUpdatesEntryModelTests.es6.js
	reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/models/status_update.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you add:

...

Type:
    bool
```
reviewboard/reviews/models/status_update.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you add:

...

Type:
    unicode
```
reviewboard/static/rb/js/reviewRequestPage/models/statusUpdatesEntryModel.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
"ID"
```
reviewboard/static/rb/js/reviewRequestPage/models/statusUpdatesEntryModel.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you swap these so they'll be in alphabetical order?
```

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

The issue has been resolved. Show all issues

Instead of checking if this is in kwargs and then fetching this in two spots, how about pulling it out first and then checking if it's None before operating on it?

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

The issue has been dropped. Show all issues

Hmm, we should localize this, though I noticed we don't elsewhere. In standard forms, they're typically localized and have trailing periods.

Dunno if it's worth doing that in this change, since it's not an isolated case here.

david Aug. 10, 2020, 4:06 p.m.

None of the field errors in the API are localized.

reviewboard/webapi/tests/test_status_update.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should use assertSpyCalledWith().
```
reviewboard/webapi/tests/test_status_update.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should use assertSpyCalledWith().
```
reviewboard/webapi/tests/test_status_update.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should use assertIn().
Though, can we check the explicit value, instead of just the presence?
```
reviewboard/webapi/tests/test_status_update.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Same as above.
```

Commit:

-	d0ff77c3bdbd970248e380669b47c865b62fb6a5
+	c60f95da7ce8fd84ad97297e247ef2c3f1fe1d8c

Diff:

Revision 3 (+431 -17)

Show changes

	reviewboard/reviews/detail.py
	reviewboard/reviews/signals.py
	reviewboard/reviews/models/status_update.py
	reviewboard/reviews/tests/test_entries.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/reviewRequestPage/models/statusUpdatesEntryModel.es6.js
	reviewboard/static/rb/js/reviewRequestPage/models/tests/statusUpdatesEntryModelTests.es6.js
	reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js
	4 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/tests/test_status_update.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E703 statement ends with a semicolon
```
reviewboard/webapi/tests/test_status_update.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E703 statement ends with a semicolon
```

Change Summary:

Derp.

Commit:

-	c60f95da7ce8fd84ad97297e247ef2c3f1fe1d8c
+	baa7313dbd52f9aef4c68ca2af8e05df8ac1096e

Diff:

Revision 4 (+431 -17)

Show changes

	reviewboard/reviews/detail.py
	reviewboard/reviews/signals.py
	reviewboard/reviews/models/status_update.py
	reviewboard/reviews/tests/test_entries.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/reviewRequestPage/models/statusUpdatesEntryModel.es6.js
	reviewboard/static/rb/js/reviewRequestPage/models/tests/statusUpdatesEntryModelTests.es6.js
	reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (bf6dcee)