• 
      

    Fix options override order in RB.apiCall.

    Review Request #12463 — Created July 12, 2022 and submitted

    Information

    Review Board
    release-5.0.x

    Reviewers

    RB.apiCall has had a long-existing bug where built-in callbacks could
    get shadowed by options arguments. The intent of the implementation is
    that the built-in error and complete callbacks always run, and will
    then call any ones which are passed in by the caller. Unfortunately, the
    order of arguments passed to $.extend() prevented this from working as
    intended. For existing versions of Review Board, the only noticeable
    effect was the error shadows within the OAuth UI, which were highly
    unlikely to occur. With the JS refactoring that happened for 5.0, we
    added a complete callback for diff fragments, which override the
    built-in one, causing the activity indicator to never disappear.

    To fix this, I've rearranged the $.extend() call a bit in order to
    ensure that the built-in callbacks always run first. Because the
    built-in error callback has its own display mechanism, I've just removed
    the error display from the OAuth code.

    • Ran js-tests.
    • Verified that the diff fragment loader now correctly hid the activity
      indicator.
    • Checked that the OAuth error handlers worked as expected.
    • Audited all callsites to RB.apiCall to ensure that there wouldn't be
      other issues.
    Summary ID
    Fix options override order in RB.apiCall.
    `RB.apiCall` has had a long-existing bug where built-in callbacks could get shadowed by options arguments. The intent of the implementation is that the built-in `error` and `complete` callbacks always run, and will then call any ones which are passed in by the caller. Unfortunately, the order of arguments passed to `$.extend()` prevented this from working as intended. For existing versions of Review Board, the only noticeable effect was the error shadows within the OAuth UI, which were highly unlikely to occur. With the JS refactoring that happened for 5.0, we added a complete callback for diff fragments, which override the built-in one, causing the activity indicator to never disappear. To fix this, I've rearranged the `$.extend()` call a bit in order to ensure that the built-in callbacks always run first. Because the built-in error callback has its own display mechanism, I've just removed the error display from the OAuth code. Testing Done: - Ran js-tests. - Verified that the diff fragment loader now correctly hid the activity indicator. - Checked that the OAuth error handlers worked as expected.
    df5d3964980184517ad69d53dea88f7320f73c74
    chipx86
    1. Looks good. The one thing I wanted to check was that options.url can still be overridden, but since that's one of the first things we pull out, it looks like we're set there.

      Ship It!

    2. 
        
    david
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-5.0.x (29fa166)