Support manual run and retry for tools attached to status updates

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

alextechcc
Review Board
release-4.0.x
10316
791d5e0...
reviewboard, students

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.

  • 0
  • 0
  • 56
  • 0
  • 56
Description From Last Updated
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. 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.

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

  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
Review request changed
Loading...