• 
      

    Add manual run and retry for status updates.

    Review Request #11129 — Created Aug. 10, 2020 and submitted

    Information

    Review Board
    release-3.0.x
    baa7313...

    Reviewers

    One common request for the status updates feature has been to allow
    manual or deferred running of tools. This is desirable for a few
    different situations, such as infrequently-run or expensive checkers,
    test builds, etc. This change adds the framework for that.

    The StatusUpdate model has a new state, NOT_YET_RUN, which will show
    a button that will trigger the action. This requires that the caller (an
    extension) listen to the new status_update_request_run signal to
    actually run the tool.

    Because it is very similar in its implementation, this additionally adds
    an ability for the creator of the status update to indicate that it can
    be retried. The Retry action will be shown if the status update is
    marked as retryable (via extra_data), and the result of the status
    update is in either error or timeout states. Like the "Run" action,
    this requires handling the new signal to actually run the tool when the
    user clicks the button.

    The StatusUpdate API can now accept request-run in the state field,
    which will trigger the new signal. This allows the frontend (or
    third-party callers) to trigger the run/retry action.

    Based on work by Alex Klemenchuk at /r/10311/

    • Ran python and JS unit tests.
    • Used in conjunction with a Review Bot change to use these features.
    Description From Last Updated

    Col: 15 'localSitePrefix' is defined but never used.

    reviewbotreviewbot

    Col: 49 Missing semicolon.

    reviewbotreviewbot

    E703 statement ends with a semicolon

    reviewbotreviewbot

    E703 statement ends with a semicolon

    reviewbotreviewbot

    Can you add: ... Type: bool

    chipx86chipx86

    Can you add: ... Type: unicode

    chipx86chipx86

    "ID"

    chipx86chipx86

    Can you swap these so they'll be in alphabetical order?

    chipx86chipx86

    Instead of checking if this is in kwargs and then fetching this in two spots, how about pulling it out …

    chipx86chipx86

    Hmm, we should localize this, though I noticed we don't elsewhere. In standard forms, they're typically localized and have trailing …

    chipx86chipx86

    Should use assertSpyCalledWith().

    chipx86chipx86

    Should use assertSpyCalledWith().

    chipx86chipx86

    Should use assertIn(). Though, can we check the explicit value, instead of just the presence?

    chipx86chipx86

    Same as above.

    chipx86chipx86

    E703 statement ends with a semicolon

    reviewbotreviewbot

    E703 statement ends with a semicolon

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

    flake8

    JSHint

    david
    chipx86
    1. 
        
    2. Show all issues

      Can you add:

      ...
      
      Type:
          bool
      
    3. reviewboard/reviews/models/status_update.py (Diff revision 2)
       
       
       
      Show all issues

      Can you add:

      ...
      
      Type:
          unicode
      
    4. Show all issues

      Can you swap these so they'll be in alphabetical order?

    5. reviewboard/webapi/resources/status_update.py (Diff revision 2)
       
       
       
       
       
       
      Show all issues

      Instead of checking if this is in kwargs and then fetching this in two spots, how about pulling it out first and then checking if it's None before operating on it?

    6. Show all issues

      Hmm, we should localize this, though I noticed we don't elsewhere. In standard forms, they're typically localized and have trailing periods.

      Dunno if it's worth doing that in this change, since it's not an isolated case here.

      1. None of the field errors in the API are localized.

    7. Show all issues

      Should use assertSpyCalledWith().

    8. Show all issues

      Should use assertSpyCalledWith().

    9. Show all issues

      Should use assertIn().

      Though, can we check the explicit value, instead of just the presence?

    10. Show all issues

      Same as above.

    11. 
        
    david
    Review request changed
    david
    chipx86
    1. Ship It!
    2. 
        
    david
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-3.0.x (bf6dcee)