Summary

Support manual run and retry for tools attached to status updates

Review Request #10311 — Created Nov. 7, 2018 and discarded Aug. 10, 2020, 3:40 p.m.

Information

Owner

alextechcc

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

Blocks

10316

Reviewers

Groups

reviewboard, students

People

Description

Allows Status Updates to have tools that are manually run, and allows
Status Updates with a error/timeout state to be rerun.

Adds a new StatusUpdate state NEEDS_RUN which represents the ability
to be run manually on user interaction in a review request, and is
considered a pending StatusUpdate for the purposes of collapsing /
summaries.

Adds a new option to the PUT API for Status Updates that supports
running a tool attached to a Status Update. This runs a new signal:
status_update_run_tool, which extensions can hook into to run some
sort of tool when the user manually hits the run/retry button in the
review request.

Adds a new button to the front-end to PUT to the new StatusUpdate API,
which checks for updates on newly run tools.

Adds a new field to the StatusUpdatesEntry model containing the review
request ID to be passed to a the event handler of these new buttons.

Testing Done

Tests have been added for all added functionality in front-end and
back-end.

All tests pass, and manual testing with reviewbot tools has been done
and works with the new manual run feature, as well as retrying features.

Issues

Description	From	Last Updated
Your summary shows up (basically) in the first line of your description. That is unnecessary.	brennie	Nov. 29, 2018, 8:22 p.m.
Can you rewrite your summary in the imperitive mood, i.e. as a command? If you substitute your summary into the …	brennie	Dec. 2, 2018, 5:38 p.m.
E501 line too long (80 > 79 characters)	reviewbot	Nov. 8, 2018, 8 p.m.
E265 block comment should start with '# '	reviewbot	Nov. 8, 2018, 8 p.m.
Col: 45 Expected '}' to match '{' from line 97 and instead saw ';'.	reviewbot	Nov. 8, 2018, 8 p.m.
Col: 44 Missing semicolon.	reviewbot	Nov. 8, 2018, 8:01 p.m.
Col: 13 Expected ')' and instead saw 'error'.	reviewbot	Nov. 8, 2018, 8:01 p.m.
Col: 18 Missing semicolon.	reviewbot	Nov. 8, 2018, 8:01 p.m.
Col: 18 Expected '}' to match '{' from line 94 and instead saw ':'.	reviewbot	Nov. 8, 2018, 8:01 p.m.
Col: 20 Expected '}' to match '{' from line 14 and instead saw '('.	reviewbot	Nov. 8, 2018, 8:01 p.m.
Col: 21 Expected ')' and instead saw 'x'.	reviewbot	Nov. 8, 2018, 8:01 p.m.
Col: 22 Missing semicolon.	reviewbot	Nov. 8, 2018, 8:01 p.m.
Col: 22 Expected an identifier and instead saw ')'.	reviewbot	Nov. 8, 2018, 8:01 p.m.
Col: 24 Expected an operator and instead saw '=>'.	reviewbot	Nov. 8, 2018, 8:01 p.m.
Col: 24 Expected an assignment or function call and instead saw an expression.	reviewbot	Nov. 8, 2018, 8:01 p.m.
Col: 26 Missing semicolon.	reviewbot	Nov. 8, 2018, 8:01 p.m.
Col: 42 Missing semicolon.	reviewbot	Nov. 8, 2018, 8:01 p.m.
Col: 43 Unnecessary semicolon.	reviewbot	Nov. 8, 2018, 8:01 p.m.
Col: 10 Expected an assignment or function call and instead saw an expression.	reviewbot	Nov. 8, 2018, 8:01 p.m.
Col: 11 Unrecoverable syntax error. (84% scanned).	reviewbot	Nov. 8, 2018, 8:01 p.m.
E501 line too long (83 > 79 characters)	reviewbot	Nov. 15, 2018, 6:53 p.m.
Col: 44 Missing semicolon.	reviewbot	Nov. 8, 2018, 8:15 p.m.
Col: 42 Missing semicolon.	reviewbot	Nov. 8, 2018, 8:15 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Nov. 26, 2018, 3:44 p.m.
F841 local variable 'review' is assigned to but never used	reviewbot	Nov. 26, 2018, 3:45 p.m.
F841 local variable 'review' is assigned to but never used	reviewbot	Nov. 26, 2018, 3:45 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Nov. 26, 2018, 8:27 p.m.
Missing period.	brennie	Nov. 29, 2018, 8:24 p.m.
Missing docstring.	brennie	Nov. 29, 2018, 8:26 p.m.
Missing period.	brennie	Nov. 29, 2018, 8:26 p.m.
You don't need the surrounding parentheses since this is in a function call.	brennie	Nov. 29, 2018, 8:30 p.m.
You shouldn't need any explicit newlines in this test or the other since assertHTMLEqual ignores whitespace.	brennie	Nov. 29, 2018, 8:31 p.m.
You don't need the surrounding parentheses since this is in a function call.	brennie	Nov. 29, 2018, 8:32 p.m.
Remove this blank line.	brennie	Nov. 29, 2018, 8:33 p.m.
Missing args (e is jQuery.Event)	brennie	Nov. 29, 2018, 8:34 p.m.
This doesn't do any interpolation or JS. This can just be a string literal.	brennie	Nov. 29, 2018, 8:36 p.m.
If we make template a string literal, we can replace the right hand side of this statement with $(template).appendTo($testsScratch).	brennie	Nov. 29, 2018, 8:37 p.m.
Function body should be indented.	brennie	Nov. 29, 2018, 8:38 p.m.
k comes after d.	brennie	Nov. 29, 2018, 8:39 p.m.
Reflow this docstring.	brennie	Nov. 29, 2018, 8:40 p.m.
This too	brennie	Nov. 29, 2018, 8:40 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Nov. 29, 2018, 8:50 p.m.
These should be in alphabetical order	bolariinwa	Dec. 1, 2018, 2:02 p.m.
Unnecessary. We have this pattern all over the codebase.	brennie	Dec. 1, 2018, 2:07 p.m.
If you aren't using the xhr, change the args to ()	brennie	Dec. 1, 2018, 2:07 p.m.
Attributes should be aligned with attributes, e.g. <time class="timesince" datetime="..." title="..."> 0 minutes ago </time>	brennie	Dec. 1, 2018, 2:11 p.m.
Could add some indents for the span and input tags since they are inside the div?	Malcolm	Dec. 6, 2018, 3:55 p.m.
Formatting here could be cleaned up too.	david	Dec. 6, 2018, 3:58 p.m.
Can we use a bit more descriptive class name (status-update-run-tool perhaps)?	david	Dec. 6, 2018, 4:03 p.m.
Might be better to have the closing </div> tag on a new line and unindented so that the <span> section …	Malcolm	Dec. 6, 2018, 3:57 p.m.
Should this be None instead of ''?	brennie	Dec. 19, 2018, 11:57 a.m.
I don't know that we should call this "running" or "tool" since (a) it doesn't actually run the tool -- …	brennie	Dec. 19, 2018, 12:15 p.m.
Does this still pass if you indent status-update-state-needs-run to be aligned with status-update-summary-entry ?	brennie	Dec. 19, 2018, 12:15 p.m.
Should be indented one space.	brennie	Dec. 19, 2018, 12:17 p.m.
nit: multi-line comments should use /* */	brennie	Dec. 19, 2018, 12:17 p.m.
This is way over indented. It should only be indented a single space relative to the <a>	brennie	Dec. 19, 2018, 12:18 p.m.

flake8 failed.

JSHint failed.

flake8

reviewboard/reviews/models/status_update.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/reviews/models/status_update.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E265 block comment should start with '# '
```

JSHint

reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 45
 Expected '}' to match '{' from line 97 and instead saw ';'.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 44
 Missing semicolon.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 13
 Expected ')' and instead saw 'error'.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 18
 Missing semicolon.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 18
 Expected '}' to match '{' from line 94 and instead saw ':'.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 20
 Expected '}' to match '{' from line 14 and instead saw '('.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 21
 Expected ')' and instead saw 'x'.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 22
 Missing semicolon.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 22
 Expected an identifier and instead saw ')'.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 24
 Expected an operator and instead saw '=>'.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 24
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 26
 Missing semicolon.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 42
 Missing semicolon.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 43
 Unnecessary semicolon.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 10
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 11
 Unrecoverable syntax error. (84% scanned).
```

Description:

		Initial addition of run-tool feature
	+
	+	Not looking for feedback yet.

Commit:

-	80f212ec6ac9ad77b598aed801846f9b914fb51f
+	1f719b82d6ef2c875cc1babfd514b91f5cbb8c19

Diff:

Revision 2 (+101 -4)

Show changes

	reviewboard/reviews/detail.py
	reviewboard/reviews/signals.py
	reviewboard/reviews/models/status_update.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/reviewRequestPage/models/statusUpdatesEntryModel.es6.js
	reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js
	reviewboard/templates/reviews/status_update_summary.html
	reviewboard/webapi/resources/status_update.py

Checks run (2 failed)

flake8 failed.

JSHint failed.

flake8

reviewboard/reviews/detail.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (83 > 79 characters)
```

JSHint

reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 44
 Missing semicolon.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 42
 Missing semicolon.
```

Description:

		Initial addition of run-tool feature

~		Not looking for feedback yet.
	~	Not looking for feedback yet, aside from high-level design direction.

Commit:

-	1f719b82d6ef2c875cc1babfd514b91f5cbb8c19
+	809475c1f4e62367f4adeb43b0e429135c4e7bbf

Diff:

Revision 3 (+107 -4)

Show changes

	reviewboard/reviews/detail.py
	reviewboard/reviews/signals.py
	reviewboard/reviews/models/status_update.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/reviewRequestPage/models/statusUpdatesEntryModel.es6.js
	reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js
	reviewboard/templates/reviews/status_update_summary.html
	reviewboard/webapi/resources/status_update.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Fixed broken collapse button by extending parent events, use effective state to determine runnable (timed out)

Commit:

-	809475c1f4e62367f4adeb43b0e429135c4e7bbf
+	7aa2828684684d8bd3c887fb74eb2c3001040757

Diff:

Revision 4 (+110 -4)

Show changes

	reviewboard/reviews/detail.py
	reviewboard/reviews/signals.py
	reviewboard/reviews/models/status_update.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/reviewRequestPage/models/statusUpdatesEntryModel.es6.js
	reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js
	reviewboard/templates/reviews/status_update_summary.html
	reviewboard/webapi/resources/status_update.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:


Fix missing summary class
Add tests, fix broken tests due to rerun button rendering
Small code cleanup

Description:

		Initial addition of run-tool feature
-
-		Not looking for feedback yet, aside from high-level design direction.

Commit:

-	7aa2828684684d8bd3c887fb74eb2c3001040757
+	042c1476cfbcc5ce956fed927c664d070320b347

Diff:

Revision 5 (+275 -12)

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/reviews/detail.py (Diff revision 5)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/webapi/tests/test_status_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
F841 local variable 'review' is assigned to but never used
```
reviewboard/webapi/tests/test_status_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
F841 local variable 'review' is assigned to but never used
```

Change Summary:

Removed unnecessary review object, removed WIP tag, updated description, added front-end tests.

Summary:

-	[WIP] Initial addition of run-tool feature for StatusUpdates
+	Initial addition of run-tool feature for StatusUpdates

Description:

~		Initial addition of run-tool feature
	~	Initial addition of run-tool feature. This feature allows Status Updates
	+	to have tools that are manually run, and allows Status Updates with a
	+	error/timeout state to be rerun.
	+
	+	A new StatusUpdate state `NEEDS_RUN` was added, which is considered a
	+	pending StatusUpdate for the purposes of collapsing / summaries.
	+
	+	`NEEDS_RUN` represents a Status Update that has the ability to be run
	+	manually on user interaction in a review request.
	+
	+	A new option to the PUT API for Status Updates was added to support
	+	running a tool attached to a Status Update. This runs a new signal:
	+	`status_update_run_tool`, which extensions can hook into to run some
	+	sort of tool when the user manually hits the run/retry button in the
	+	review request.
	+
	+	A new button was added to the front-end to PUT to the new StatusUpdate
	+	API, which checks for updates on newly run tools.
	+
	+	The `StatusUpdatesEntry` model now contains the review request ID to be
	+	passed to a the event handler of these new buttons.

Testing Done:

	+	Tests have been added for all added functionality in front-end and
	+	back-end.
	+
	+	All tests pass, and manual testing with reviewbot tools has been done
	+	and works with the new manual run feature, as well as retrying features.

Commit:

-	042c1476cfbcc5ce956fed927c664d070320b347
+	c2b72733640ecfb988f42ba6b34e11358cbececb

Diff:

Revision 6 (+318 -14)

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/reviews/detail.py (Diff revision 6)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```

Change Summary:

Fixed underindent

Commit:

-	c2b72733640ecfb988f42ba6b34e11358cbececb
+	434cb1b3831cce7f13430b2a8292784c35398039

Diff:

Revision 7 (+318 -14)

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.

The issue has been resolved. Show all issues

Your summary shows up (basically) in the first line of your description. That is unnecessary.

The issue has been resolved. Show all issues

Can you rewrite your summary in the imperitive mood, i.e. as a command? If you substitute your summary into the following template, it should be a correct sentence.

This patch will <summary>

brennie Dec. 2, 2018, 12:01 p.m.

This issue has been closed twice but hasn't been addressed.

reviewboard/reviews/models/status_update.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Missing period.
```
reviewboard/reviews/models/status_update.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Missing docstring.
```
reviewboard/reviews/models/status_update.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Missing period.
```

reviewboard/reviews/tests/test_entries.py (Diff revision 7)

The issue has been resolved. Show all issues

You don't need the surrounding parentheses since this is in a function call.

reviewboard/reviews/tests/test_entries.py (Diff revision 7)

The issue has been resolved. Show all issues

You shouldn't need any explicit newlines in this test or the other since assertHTMLEqual ignores whitespace.

reviewboard/reviews/tests/test_entries.py (Diff revision 7)

The issue has been resolved. Show all issues

You don't need the surrounding parentheses since this is in a function call.

reviewboard/reviews/tests/test_entries.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Remove this blank line.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Missing args (e is jQuery.Event)
```

reviewboard/static/rb/js/reviewRequestPage/views/tests/baseStatusUpdatesEntryViewTests.es6.js (Diff revision 7)

The issue has been resolved. Show all issues

This doesn't do any interpolation or JS. This can just be a string literal.

reviewboard/static/rb/js/reviewRequestPage/views/tests/baseStatusUpdatesEntryViewTests.es6.js (Diff revision 7)
The issue has been resolved. Show all issues
```
If we make template a string literal, we can replace the right hand side of this statement with $(template).appendTo($testsScratch).
```

reviewboard/static/rb/js/reviewRequestPage/views/tests/baseStatusUpdatesEntryViewTests.es6.js (Diff revision 7)

The issue has been resolved. Show all issues

Function body should be indented.

reviewboard/webapi/tests/test_status_update.py (Diff revision 7)
The issue has been resolved. Show all issues
```
k comes after d.
```
reviewboard/webapi/tests/test_status_update.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Reflow this docstring.
```
reviewboard/webapi/tests/test_status_update.py (Diff revision 7)
The issue has been resolved. Show all issues
```
This too
```

Change Summary:

Incorperated Barret's suggested changes.

Commit:

-	434cb1b3831cce7f13430b2a8292784c35398039
+	fa43eedd3de0a58ac715ec8bdc884a109a25bc4a

Diff:

Revision 8 (+323 -18)

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/reviews/models/status_update.py (Diff revision 8)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```

Change Summary:

Added missing description update.

Description:

~		Initial addition of run-tool feature. This feature allows Status Updates
~		to have tools that are manually run, and allows Status Updates with a
	~	Allows Status Updates to have tools that are manually run, and allows
	~	Status Updates with a error/timeout state to be rerun.
-		error/timeout state to be rerun.

~		A new StatusUpdate state `NEEDS_RUN` was added, which is considered a
~		pending StatusUpdate for the purposes of collapsing / summaries.
	~	Adds a new StatusUpdate state `NEEDS_RUN` which represents the ability
	~	to be run manually on user interaction in a review request, and is
	+	considered a pending StatusUpdate for the purposes of collapsing /
	+	summaries.

~		`NEEDS_RUN` represents a Status Update that has the ability to be run
	~	Adds a new option to the PUT API for Status Updates that supports
-		manually on user interaction in a review request.
-
-		A new option to the PUT API for Status Updates was added to support
		running a tool attached to a Status Update. This runs a new signal:
		`status_update_run_tool`, which extensions can hook into to run some
		sort of tool when the user manually hits the run/retry button in the
		review request.

~		A new button was added to the front-end to PUT to the new StatusUpdate
~		API, which checks for updates on newly run tools.
	~	Adds a new button to the front-end to PUT to the new StatusUpdate API,
	~	which checks for updates on newly run tools.

~		The `StatusUpdatesEntry` model now contains the review request ID to be
~		passed to a the event handler of these new buttons.
	~	Adds a new field to the `StatusUpdatesEntry` model containing the review
	~	request ID to be passed to a the event handler of these new buttons.

Commit:

-	fa43eedd3de0a58ac715ec8bdc884a109a25bc4a
+	eb2fc8355c0c4447571539e99a9afcfff232bb08

Diff:

Revision 9 (+323 -18)

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/detail.py (Diff revision 9)
The issue has been resolved. Show all issues
```
These should be in alphabetical order
```

reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Unnecessary. We have this pattern all over the codebase.
```

reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 9)

The issue has been resolved. Show all issues

If you aren't using the xhr, change the args to ()

reviewboard/static/rb/js/reviewRequestPage/views/tests/baseStatusUpdatesEntryViewTests.es6.js (Diff revision 9)

The issue has been resolved. Show all issues

Attributes should be aligned with attributes, e.g.

<time class="timesince"
      datetime="..."
      title="...">
 0 minutes ago
</time>

Change Summary:

Addressed Barret's review

Commit:

-	eb2fc8355c0c4447571539e99a9afcfff232bb08
+	b688d08ad2492423d1227494de6f9762632fc599

Diff:

Revision 10 (+324 -18)

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/tests/test_entries.py (Diff revision 10)

The issue has been resolved. Show all issues

Could add some indents for the span and input tags since they are inside the div?

alextechcc Dec. 2, 2018, 8:45 a.m.

Not sure what style here is preferred, but technically they are indented, they just match the attributes linebreak above.
I believe the attributes should be moved to line up with "class".

david Dec. 2, 2018, 9:54 a.m.

I think we'd usually do this:

'<div class="status-update-summary-entry'
'            status-update-states-failure">'
' <span class="summary">My summary.</span>
' timed out.'
' <input class="runtool" data-status-update-id="1234"'
'        type="button" value="Retry" />'
'</div>'


Not sure if that works with assertHTMLEqual, but worth a try.

reviewboard/static/rb/js/reviewRequestPage/views/tests/baseStatusUpdatesEntryViewTests.es6.js (Diff revision 10)

The issue has been resolved. Show all issues

Might be better to have the closing </div> tag on a new line and unindented so that the <span> section can be more clearly seen as inside the scope of the <div>

reviewboard/reviews/tests/test_entries.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Formatting here could be cleaned up too.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 10)
The issue has been resolved. Show all issues
```
Can we use a bit more descriptive class name (status-update-run-tool perhaps)?
```

Summary:

-	Initial addition of run-tool feature for StatusUpdates
+	Support manual run and retry for tools attached to status updates

reviewboard/reviews/detail.py (Diff revision 10)

Can self.state_summary_count be set to needs_run instead of pending for the needs_run case?

alextechcc Dec. 6, 2018, 4:10 p.m.

I think in this case it'd be better to not introduce a new state summary type. For example, we group all failure states as "failure".

reviewboard/reviews/models/status_update.py (Diff revision 10)

nit: maybe "Check whether" instead of just "Whether"

alextechcc Dec. 6, 2018, 4:10 p.m.

Since this is a @property I believe the wording makes sense.

reviewboard/webapi/resources/status_update.py (Diff revision 10)
```
Is this supposed to be running or needs to run?
```

Change Summary:

Format and naming changes. Note that assertHTMLEqual cares about whitespace inside attributes.

Commit:

-	b688d08ad2492423d1227494de6f9762632fc599
+	9f8b0b41788d411c840ea6666f6b465c69349ae3

Diff:

Revision 11 (+331 -18)

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 11)
The issue has been resolved. Show all issues
```
Should this be None instead of ''?
```

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

The issue has been resolved. Show all issues

I don't know that we should call this "running" or "tool" since (a) it doesn't actually run the tool -- it requests a run and something has to be set up to listen and (b) tools are a very review bot centric idea, whereas status updates can theoretically be used by anything, including non-review bot clients.

How about renaming this to status_update_request_run and updating the docs to match (here and elsewhere)?

reviewboard/reviews/tests/test_entries.py (Diff revision 11)

The issue has been resolved. Show all issues

Does this still pass if you indent status-update-state-needs-run to be aligned with status-update-summary-entry ?

alextechcc Dec. 19, 2018, 12:22 p.m.
```
Nope, spaces inside attributes count.
```

reviewboard/reviews/tests/test_entries.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Should be indented one space.
```
reviewboard/static/rb/js/reviewRequestPage/views/baseStatusUpdatesEntryView.es6.js (Diff revision 11)
The issue has been resolved. Show all issues
```
nit: multi-line comments should use /* */
```

reviewboard/static/rb/js/reviewRequestPage/views/tests/baseStatusUpdatesEntryViewTests.es6.js (Diff revision 11)

The issue has been resolved. Show all issues

This is way over indented. It should only be indented a single space relative to the <a>

Commit:

-	9f8b0b41788d411c840ea6666f6b465c69349ae3
+	791d5e084c35c81cc69b86bb1a78055182e4ac6b

Diff:

Revision 12 (+336 -18)

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.

Ping... is this still in progress? I really like to see this.

Status: Discarded

Change Summary:

Supplanted by /r/11129/