Support manual run and retry for tools attached to status updates

Review Request #10311 — Created Nov. 7, 2018 and discarded

Information

Review Board
release-4.0.x

Reviewers

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.

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.

brenniebrennie

Can you rewrite your summary in the imperitive mood, i.e. as a command? If you substitute your summary into the …

brenniebrennie

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E265 block comment should start with '# '

reviewbotreviewbot

Col: 45 Expected '}' to match '{' from line 97 and instead saw ';'.

reviewbotreviewbot

Col: 44 Missing semicolon.

reviewbotreviewbot

Col: 13 Expected ')' and instead saw 'error'.

reviewbotreviewbot

Col: 18 Missing semicolon.

reviewbotreviewbot

Col: 18 Expected '}' to match '{' from line 94 and instead saw ':'.

reviewbotreviewbot

Col: 20 Expected '}' to match '{' from line 14 and instead saw '('.

reviewbotreviewbot

Col: 21 Expected ')' and instead saw 'x'.

reviewbotreviewbot

Col: 22 Missing semicolon.

reviewbotreviewbot

Col: 22 Expected an identifier and instead saw ')'.

reviewbotreviewbot

Col: 24 Expected an operator and instead saw '=>'.

reviewbotreviewbot

Col: 24 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 26 Missing semicolon.

reviewbotreviewbot

Col: 42 Missing semicolon.

reviewbotreviewbot

Col: 43 Unnecessary semicolon.

reviewbotreviewbot

Col: 10 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 11 Unrecoverable syntax error. (84% scanned).

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 44 Missing semicolon.

reviewbotreviewbot

Col: 42 Missing semicolon.

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

F841 local variable 'review' is assigned to but never used

reviewbotreviewbot

F841 local variable 'review' is assigned to but never used

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

Missing period.

brenniebrennie

Missing docstring.

brenniebrennie

Missing period.

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

Remove this blank line.

brenniebrennie

Missing args (e is jQuery.Event)

brenniebrennie

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

brenniebrennie

If we make template a string literal, we can replace the right hand side of this statement with $(template).appendTo($testsScratch).

brenniebrennie

Function body should be indented.

brenniebrennie

k comes after d.

brenniebrennie

Reflow this docstring.

brenniebrennie

This too

brenniebrennie

E501 line too long (81 > 79 characters)

reviewbotreviewbot

These should be in alphabetical order

bolariinwabolariinwa

Unnecessary. We have this pattern all over the codebase.

brenniebrennie

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

brenniebrennie

Attributes should be aligned with attributes, e.g. <time class="timesince" datetime="..." title="..."> 0 minutes ago </time>

brenniebrennie

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

MalcolmMalcolm

Formatting here could be cleaned up too.

daviddavid

Can we use a bit more descriptive class name (status-update-run-tool perhaps)?

daviddavid

Might be better to have the closing </div> tag on a new line and unindented so that the <span> section …

MalcolmMalcolm

Should this be None instead of ''?

brenniebrennie

I don't know that we should call this "running" or "tool" since (a) it doesn't actually run the tool -- …

brenniebrennie

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

brenniebrennie

Should be indented one space.

brenniebrennie

nit: multi-line comments should use /* */

brenniebrennie

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

brenniebrennie
Checks run (2 failed)
flake8 failed.
JSHint failed.

flake8

JSHint

alextechcc
alextechcc
Review request changed
alextechcc
alextechcc
alextechcc
alextechcc
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

alextechcc
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

alextechcc
brennie
  1. 
      
  2. Show all issues

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

  3. 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>
    
    1. This issue has been closed twice but hasn't been addressed.

  4. Show all issues

    Missing period.

  5. Show all issues

    Missing docstring.

  6. Show all issues

    Missing period.

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

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

  8. Show all issues

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

  9. reviewboard/reviews/tests/test_entries.py (Diff revision 7)
     
     
     
     
     
     
     
     
    Show all issues

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

  10. Show all issues

    Remove this blank line.

  11. Show all issues

    Missing args (e is jQuery.Event)

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

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

  13. Show all issues

    If we make template a string literal, we can replace the right hand side of this statement with $(template).appendTo($testsScratch).

  14. Show all issues

    Function body should be indented.

  15. reviewboard/webapi/tests/test_status_update.py (Diff revision 7)
     
     
     
     
     
     
    Show all issues

    k comes after d.

  16. reviewboard/webapi/tests/test_status_update.py (Diff revision 7)
     
     
     
     
    Show all issues

    Reflow this docstring.

  17. reviewboard/webapi/tests/test_status_update.py (Diff revision 7)
     
     
     
     
    Show all issues

    This too

  18. 
      
alextechcc
Review request changed
alextechcc
alextechcc
bolariinwa
  1. 
      
  2. reviewboard/reviews/detail.py (Diff revision 9)
     
     
     
     
    Show all issues

    These should be in alphabetical order

  3. 
      
brennie
  1. 
      
  2. Show all issues

    Unnecessary. We have this pattern all over the codebase.

  3. Show all issues

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

  4. Show all issues

    Attributes should be aligned with attributes, e.g.

    <time class="timesince"
          datetime="..."
          title="...">
     0 minutes ago
    </time>
    
  5. 
      
alextechcc
Malcolm
  1. 
      
  2. reviewboard/reviews/tests/test_entries.py (Diff revision 10)
     
     
     
     
     
     
     
     
    Show all issues

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

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

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

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

  4. 
      
david
  1. 
      
  2. reviewboard/reviews/tests/test_entries.py (Diff revision 10)
     
     
     
     
     
     
     
     
    Show all issues

    Formatting here could be cleaned up too.

  3. Show all issues

    Can we use a bit more descriptive class name (status-update-run-tool perhaps)?

  4. 
      
alextechcc
ilaw
  1. 
      
  2. 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?

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

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

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

  4. Is this supposed to be running or needs to run?

  5. 
      
alextechcc
brennie
  1. 
      
  2. Show all issues

    Should this be None instead of ''?

  3. reviewboard/reviews/signals.py (Diff revision 11)
     
     
     
     
     
     
     
    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)?

  4. Show all issues

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

    1. Nope, spaces inside attributes count.

  5. reviewboard/reviews/tests/test_entries.py (Diff revision 11)
     
     
     
     
    Show all issues

    Should be indented one space.

  6. Show all issues

    nit: multi-line comments should use /* */

  7. Show all issues

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

  8. 
      
alextechcc
misery
  1. 
      
  2. Ping... is this still in progress? I really like to see this.

  3. 
      
david
Review request changed
Status:
Discarded
Change Summary:

Supplanted by /r/11129/