flake8
failed.
JSHint
failed.
Support manual run and retry for tools attached to status updates
Review Request #10311 — Created Nov. 7, 2018 and discarded
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_RUNwhich 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
StatusUpdatesEntrymodel containing the review
request ID to be passed to a the event handler of these new buttons.
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.
| Description | From | Last Updated |
|---|---|---|
|
Your summary shows up (basically) in the first line of your description. That is unnecessary. |
 brennie
|
|
|
Can you rewrite your summary in the imperitive mood, i.e. as a command? If you substitute your summary into the … |
brennie
|
|
|
E501 line too long (80 > 79 characters) |
 reviewbot
|
|
|
E265 block comment should start with '# ' |
reviewbot
|
|
|
Col: 45 Expected '}' to match '{' from line 97 and instead saw ';'. |
reviewbot
|
|
|
Col: 44 Missing semicolon. |
reviewbot
|
|
|
Col: 13 Expected ')' and instead saw 'error'. |
reviewbot
|
|
|
Col: 18 Missing semicolon. |
reviewbot
|
|
|
Col: 18 Expected '}' to match '{' from line 94 and instead saw ':'. |
reviewbot
|
|
|
Col: 20 Expected '}' to match '{' from line 14 and instead saw '('. |
reviewbot
|
|
|
Col: 21 Expected ')' and instead saw 'x'. |
reviewbot
|
|
|
Col: 22 Missing semicolon. |
reviewbot
|
|
|
Col: 22 Expected an identifier and instead saw ')'. |
reviewbot
|
|
|
Col: 24 Expected an operator and instead saw '=>'. |
reviewbot
|
|
|
Col: 24 Expected an assignment or function call and instead saw an expression. |
reviewbot
|
|
|
Col: 26 Missing semicolon. |
reviewbot
|
|
|
Col: 42 Missing semicolon. |
reviewbot
|
|
|
Col: 43 Unnecessary semicolon. |
reviewbot
|
|
|
Col: 10 Expected an assignment or function call and instead saw an expression. |
reviewbot
|
|
|
Col: 11 Unrecoverable syntax error. (84% scanned). |
reviewbot
|
|
|
E501 line too long (83 > 79 characters) |
reviewbot
|
|
|
Col: 44 Missing semicolon. |
reviewbot
|
|
|
Col: 42 Missing semicolon. |
reviewbot
|
|
|
E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
F841 local variable 'review' is assigned to but never used |
reviewbot
|
|
|
F841 local variable 'review' is assigned to but never used |
reviewbot
|
|
|
E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Missing period. |
brennie
|
|
|
Missing docstring. |
brennie
|
|
|
Missing period. |
brennie
|
|
|
You don't need the surrounding parentheses since this is in a function call. |
brennie
|
|
|
You shouldn't need any explicit newlines in this test or the other since assertHTMLEqual ignores whitespace. |
brennie
|
|
|
You don't need the surrounding parentheses since this is in a function call. |
brennie
|
|
|
Remove this blank line. |
brennie
|
|
|
Missing args (e is jQuery.Event) |
brennie
|
|
|
This doesn't do any interpolation or JS. This can just be a string literal. |
brennie
|
|
|
If we make template a string literal, we can replace the right hand side of this statement with $(template).appendTo($testsScratch). |
brennie
|
|
|
Function body should be indented. |
brennie
|
|
|
k comes after d. |
brennie
|
|
|
Reflow this docstring. |
brennie
|
|
|
This too |
brennie
|
|
|
E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
These should be in alphabetical order |
 bolariinwa
|
|
|
Unnecessary. We have this pattern all over the codebase. |
brennie
|
|
|
If you aren't using the xhr, change the args to () |
brennie
|
|
|
Attributes should be aligned with attributes, e.g. <time class="timesince" datetime="..." title="..."> 0 minutes ago </time> |
brennie
|
|
|
Could add some indents for the span and input tags since they are inside the div? |
 Malcolm
|
|
|
Formatting here could be cleaned up too. |
 david
|
|
|
Can we use a bit more descriptive class name (status-update-run-tool perhaps)? |
david
|
|
|
Might be better to have the closing </div> tag on a new line and unindented so that the <span> section … |
Malcolm
|
|
|
Should this be None instead of ''? |
brennie
|
|
|
I don't know that we should call this "running" or "tool" since (a) it doesn't actually run the tool -- … |
brennie
|
|
|
Does this still pass if you indent status-update-state-needs-run to be aligned with status-update-summary-entry ? |
brennie
|
|
|
Should be indented one space. |
brennie
|
|
|
nit: multi-line comments should use /* */ |
brennie
|
|
|
This is way over indented. It should only be indented a single space relative to the <a> |
brennie
|
- Commit:
-
80f212ec6ac9ad77b598aed801846f9b914fb51f1f719b82d6ef2c875cc1babfd514b91f5cbb8c19
- Diff:
-
Revision 2 (+101 -4)
- Commit:
-
1f719b82d6ef2c875cc1babfd514b91f5cbb8c19809475c1f4e62367f4adeb43b0e429135c4e7bbf
- Diff:
-
Revision 3 (+107 -4)
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:
-
809475c1f4e62367f4adeb43b0e429135c4e7bbf7aa2828684684d8bd3c887fb74eb2c3001040757
- Diff:
-
Revision 4 (+110 -4)
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:
-
7aa2828684684d8bd3c887fb74eb2c3001040757042c1476cfbcc5ce956fed927c664d070320b347
- Diff:
-
Revision 5 (+275 -12)
- Change Summary:
-
Removed unnecessary review object, removed WIP tag, updated description, added front-end tests.
- Summary:
-
[WIP] Initial addition of run-tool feature for StatusUpdatesInitial 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_RUNwas added, which is considered a+ pending StatusUpdate for the purposes of collapsing / summaries. + + NEEDS_RUNrepresents 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
StatusUpdatesEntrymodel 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:
-
042c1476cfbcc5ce956fed927c664d070320b347c2b72733640ecfb988f42ba6b34e11358cbececb
- Diff:
-
Revision 6 (+318 -14)
- Change Summary:
-
Fixed underindent
- Commit:
-
c2b72733640ecfb988f42ba6b34e11358cbececb434cb1b3831cce7f13430b2a8292784c35398039
- Diff:
-
Revision 7 (+318 -14)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
-
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> -
-
-
-
-
You shouldn't need any explicit newlines in this test or the other since
assertHTMLEqualignores whitespace. -
-
-
-
-
If we make
templatea string literal, we can replace the right hand side of this statement with$(template).appendTo($testsScratch). -
-
-
-
- Change Summary:
-
Incorperated Barret's suggested changes.
- Commit:
-
434cb1b3831cce7f13430b2a8292784c35398039fa43eedd3de0a58ac715ec8bdc884a109a25bc4a
- Diff:
-
Revision 8 (+323 -18)
- 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_RUNwas added, which is considered a~ pending StatusUpdate for the purposes of collapsing / summaries. ~ Adds a new StatusUpdate state
NEEDS_RUNwhich 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_RUNrepresents 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 somesort 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
StatusUpdatesEntrymodel now contains the review request ID to be~ passed to a the event handler of these new buttons. ~ Adds a new field to the
StatusUpdatesEntrymodel containing the review~ request ID to be passed to a the event handler of these new buttons.
- Commit:
-
fa43eedd3de0a58ac715ec8bdc884a109a25bc4aeb2fc8355c0c4447571539e99a9afcfff232bb08
- Diff:
-
Revision 9 (+323 -18)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Addressed Barret's review
- Commit:
-
eb2fc8355c0c4447571539e99a9afcfff232bb08b688d08ad2492423d1227494de6f9762632fc599
- Diff:
-
Revision 10 (+324 -18)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Format and naming changes. Note that assertHTMLEqual cares about whitespace inside attributes.
- Commit:
-
b688d08ad2492423d1227494de6f9762632fc5999f8b0b41788d411c840ea6666f6b465c69349ae3
- Diff:
-
Revision 11 (+331 -18)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
-
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_runand updating the docs to match (here and elsewhere)? -
Does this still pass if you indent
status-update-state-needs-runto be aligned withstatus-update-summary-entry? -
-
-
- Commit:
-
9f8b0b41788d411c840ea6666f6b465c69349ae3791d5e084c35c81cc69b86bb1a78055182e4ac6b
- Diff:
-
Revision 12 (+336 -18)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
