• 
      

    Fix a regression in RB.apiCall and add crucial docs and testing.

    Review Request #12473 — Created July 14, 2022 and submitted — Latest diff uploaded

    Information

    Review Board
    release-5.0.x

    Reviewers

    A recent change (29fa166) to RB.apiCall() altered the order in which
    we registered built-in and caller-supplied callbacks, in an effort to
    fix a bug where a complete handler was getting overridden. The error
    and complete handlers` were changed to always take precedence over any
    caller-supplied ones.

    This code is old and complicated, and most API calls go through it. To
    aim for a lack of regressions, our code was audited to ensure we knew
    exactly when and how we were providing these handlers.

    This change ultimately ended up being the right move for the complete
    handler, but not for error. The reason is that our error handler
    tries to be "smart".

    For instance, an HTTP 500 or a 404 would be an error if the caller was
    expecting a 200. Normally, an error is anything unexpected that leads to
    a failure. However, in a REST API, error codes don't always indicate
    failures. They sometimes indicate status.

    An example is when we request a review request draft. If missing, we get
    a 404. This tells us a draft needs to be created. It's not, however, an
    "error" in the way that jQuery, Backbone, etc. think of it. It's an
    expected result.

    RB.apiCall() was built around this idea. It tried to determine what's
    an HTTP error and what's an API error. If it saw an API error payload,
    it was considered not to be an API error in the same way, and would hand
    the payload to the caller via success() in order to further analyze
    it.

    This default error handler also took care of showing the "Server Error"
    banner (which, notably, does not appear if overriding error).

    This was all the default behavior for all AJAX calls before we moved
    to Backbone, and still applies to any place where we manually call
    RB.apiCall().

    With Backbone, calls go through Backbone.sync -> Backbone.ajax ->
    RB.apiCall. As part of any call, it will always pass in success and
    error handlers. Models (such as resource classes) operating this way
    handled success and error results separately, instead of API errors
    going into the success handler.

    That brings us to the regression.

    The recent fix made our error handler run unconditionally. It would
    see an API error payload and turn it into a success call. While fine
    for manual callers of RB.apiCall(), any Backbone models would find
    their success handlers being called with API error payloads, breaking
    all sorts of state.

    This is all due for a major rethink (for instance, Backbone models can
    never trigger the useful "Server Error" banner).

    However, for now, the fix is pretty clear: Undo the part of the change
    where the built-in error handler was promoted to being the main one.
    We can still keep complete as the primary, since it doesn't try to
    interpret and alter state.

    Since this is complicated, and we've actually had attempts at fixing
    this in the past, a big part of this change is documenting all the
    current behavior and adding comprehensive unit tests to ensure we don't
    inadvertently regress it with future fixes or cleanups.

    Ran through the UI and attempted to trigger API calls. Didn't hit any
    issues anywhere.

    All unit tests pass.

    Commits

    Files