Summary

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

Review Request #12473 — Created July 14, 2022 and submitted July 19, 2022, 9:19 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-5.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done

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

All unit tests pass.

Commits

Summary	ID
Fix a regression in RB.apiCall and add crucial docs and testing. 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.	7b4b18e0383850fade206e16bc071f34e732ec35

Summary

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

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.

7b4b18e0383850fade206e16bc071f34e732ec35

Issues

Description	From	Last Updated
Missing semicolon. Column: 6 Error code: W033	reviewbot	July 14, 2022, 9:46 p.m.
'data' was used before it was defined. Column: 19 Error code: W003	reviewbot	July 14, 2022, 9:48 p.m.
Missing semicolon. Column: 23 Error code: W033	reviewbot	July 14, 2022, 9:49 p.m.
Missing semicolon. Column: 23 Error code: W033	reviewbot	July 14, 2022, 9:49 p.m.
Typo "default" -> "defaults"	maubin	July 18, 2022, 1:36 p.m.
We don't need the first {} anymore.	david	July 18, 2022, 2:28 p.m.
"a HTTP" -> "an HTTP"	david	July 18, 2022, 2:29 p.m.
Can we move this to be before the { so it's at the same level as the comment for options?	david	July 18, 2022, 2:35 p.m.

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/utils/tests/apiUtilsTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing semicolon.

Column: 6
Error code: W033
```
reviewboard/static/rb/js/utils/tests/apiUtilsTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
'data' was used before it was defined.

Column: 19
Error code: W003
```
reviewboard/static/rb/js/utils/tests/apiUtilsTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing semicolon.

Column: 23
Error code: W033
```
reviewboard/static/rb/js/utils/tests/apiUtilsTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing semicolon.

Column: 23
Error code: W033
```

Change Summary:


Pre-defined a variable to avoid a JSHint warning.
Added missing semicolons.

Commits:

	Summary	ID
	Fix a regression in RB.apiCall and add crucial docs and testing. 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.	cb06e187843ab7314d96d7da0fced74310b02410
	Fix a regression in RB.apiCall and add crucial docs and testing. 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.	775f1a3386a3200068352911dafaa332035ebd70

Diff:

Revision 2 (+1074 -12)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/utils/apiUtils.es6.js
	reviewboard/static/rb/js/utils/tests/apiUtilsTests.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/static/rb/js/utils/tests/apiUtilsTests.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Typo "default" -> "defaults"
```

Change Summary:

Fixed a typo in a docstring: default -> defaults

Commits:

	Summary	ID
	Fix a regression in RB.apiCall and add crucial docs and testing. 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.	775f1a3386a3200068352911dafaa332035ebd70
	Fix a regression in RB.apiCall and add crucial docs and testing. 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.	46ef2ea71f23ae04b7a61055ebcbcea353e19da4

Diff:

Revision 3 (+1074 -12)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/utils/apiUtils.es6.js
	reviewboard/static/rb/js/utils/tests/apiUtilsTests.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/utils/apiUtils.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
We don't need the first {} anymore.
```
reviewboard/static/rb/js/utils/apiUtils.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
"a HTTP" -> "an HTTP"
```

reviewboard/static/rb/js/utils/apiUtils.es6.js (Diff revision 3)

The issue has been resolved. Show all issues

Can we move this to be before the { so it's at the same level as the comment for options?

chipx86 July 18, 2022, 2:37 p.m.

So I wasn't sure what to do here as far as where best to put the comments, given how this statement is being built correctly (assuming I don't want to touch each line). The problem is that the whole statement really looks like this:


data = $.extend({
    // ...
},
options,
{
    // ...
});


Which I don't love at all.
Without comments, the options bit would be:

data = $.extend({
    // ...
}, options, {
    // ...
});


But then I'd have to move the comment for options inside the first block. And:

data = $.extend({
    // ...
},
    options,
{
    // ...
});


Wouldn't be correct.
Ideally, I'd rewrite it all to be:

data = $.extend(
    {
        // ...
    },
    options,
    {
        // ...
    }
);


But I wanted to avoid touching every line.
So I could move the comment down under options, but that indentation level feels weird as it is. It's no worse to move it I guess. Basically, sorta hijacking this comment to ask, what's your preference on this code?

david July 18, 2022, 2:49 p.m.

So I'd say either:

data = $.extend(
    // ...
    {
    },
    // ...
    options,
    // ...
    {
    });


or:

// ...
const overridible_options = { ... }

// ...
const required_options = { ... }

data = $.extend(overridible_options, options, required_options)


(not sure about those names though)

chipx86 July 18, 2022, 3:36 p.m.

Okay, I'll go with something along the lines of the second set. I think that'll help keep this more maintainable.

Change Summary:

Updated the option-building code to be a lot more readable and maintainable, and to avoid some weirdness with indentation levels and comment placement.

Unit tests and original repro case have been re-tested.

Commits:

	Summary	ID
	Fix a regression in RB.apiCall and add crucial docs and testing. 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.	46ef2ea71f23ae04b7a61055ebcbcea353e19da4
	Fix a regression in RB.apiCall and add crucial docs and testing. 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.	7b4b18e0383850fade206e16bc071f34e732ec35

Diff:

Revision 4 (+1078 -14)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/utils/apiUtils.es6.js
	reviewboard/static/rb/js/utils/tests/apiUtilsTests.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-5.0.x (0d46b22)