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

Diff:

Revision 5 (+275 -12)

Show changes

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

Diff:

Revision 6 (+318 -14)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

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

  5. Missing docstring.

  6. Missing period.

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

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

  8. 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)
     
     
     
     
     
     
     
     

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

  10. Remove this blank line.

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

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

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

  13. reviewboard/webapi/tests/test_status_update.py (Diff revision 7)
     
     
     
     
     
     

    k comes after d.

  14. reviewboard/webapi/tests/test_status_update.py (Diff revision 7)
     
     
     
     

    Reflow this docstring.

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

    This too

  16. 
      
alextechcc
Review request changed
alextechcc
alextechcc
bolariinwa
  1. 
      
  2. reviewboard/reviews/detail.py (Diff revision 9)
     
     
     
     

    These should be in alphabetical order

  3. 
      
brennie
  1. 
      
  2. Unnecessary. We have this pattern all over the codebase.

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

  4. 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)
     
     
     
     
     
     
     
     

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

    Formatting here could be cleaned up too.

  3. 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. Should this be None instead of ''?

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

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

    Should be indented one space.

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

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

Loading...