Fix: Added error handling when fetch users api call fails

[finnar-bin]

Added a catch when api call fails

Closes #2103

[agalin920]

@theofficialnar How does this fix the reported error?

The error that we are seeing is indicating that a promise was rejected, but instead of an Error object, which is the standard practice, an object with the keys _meta, data, error, and status was provided.

Which tells me its not really a unhandled rejection error but a handled one that is reading something unexpected

[finnar-bin]

@theofficialnar How does this fix the reported error?

The error that we are seeing is indicating that a promise was rejected, but instead of an Error object, which is the standard practice, an object with the keys _meta, data, error, and status was provided.

Which tells me its not really a unhandled rejection error but a handled one that is reading something unexpected

@agalin920 right, I realize the promise is throwing the res object instead of an Error. I'll update that. But doesn't it also make sense to add that catch at the end to handle that thrown error?

[agalin920]

@theofficialnar yea I agree. Just pointing out a potentially deeper issue

[finnar-bin]

@theofficialnar yea I agree. Just pointing out a potentially deeper issue

@agalin920 I found out that request.js doesn't throw an error for 4** responses. So, I went ahead and made it throw a new error when res.status is 4**. This makes it so that the promise is properly rejected when res.status is 4** and the redux actions utilizing request will then be able to properly handle the failed api call via catch.

It's probably worth mentioning that I am not 100% sure why request.js was made to not throw an error for 4** responses to begin with, since I can see that there are some commented out code that were supposedly handling those.

If it is by design that 4** responses should not be thrown as errors in request.js then I guess the intended fix for this bug is to just handle that 4** response inside fetchUsers() by simply checking if res.status !== 200 then wrapping that response as a new Error so that it can be handled locally on the function's catch.

[agalin920]

We can't really do this since this would break existing functionality that expects 400s to go into .then

Some actions will check for data int .then and default to an empty [] or {};

This will cause the .then not to go in and default.

[finnar-bin]

Gotcha, I guess that answers my question as to why request.js doesn't throw errors for 400s then. I'll just handle that inside fetchUsers() then.

[agalin920]

@theofficialnar
These changes still didn't convince me this is the issue so I took a look:

Looking at this call trace
https://zestyio.sentry.io/issues/2277454417/?project=5441698&referrer=github_integration

It looks like the culprit is a 403 when fetching the instance

If we take a look at the code for the instance fetch

export function fetchInstance() {
  return (dispatch) => {
    dispatch({
      type: "FETCHING_INSTANCE",
    });

  return request(`${CONFIG.API_ACCOUNTS}/instances/${ZUID}`).then((res) => {
    if (res.status === 403) {
      throw res;
    }
    dispatch({
      type: "FETCHING_INSTANCE_SUCCESS",
      payload: {
        data: res.data,
      },
    });

    return res;
  });
};

We can see that on 403 it is throwing the response but the response is not an Error Object so if there is a subsequent catch I would expect the error mentioned in the sentry ticket

And as predicted there is a catch on its chain

memo(function LoadInstance(props) {
    const [error, setError] = useState("");
    useEffect(() => {
      props
        .dispatch(fetchInstance())
        .then((res) => {
          document.title = `Manager - ${res.data.name} - Zesty`;
          CONFIG.URL_PREVIEW_FULL = `${CONFIG.URL_PREVIEW_PROTOCOL}${res.data.randomHashID}${CONFIG.URL_PREVIEW}`;
        })
        .catch((res) => {
          if (res.status === 403) {
            setError("You do not have permission to access to this instance");
          }
        });

so I expect these type of cases to be the reason for the error

[finnar-bin]

@agalin920 I see, right I'll add some handling for that as well. There is an event under this similar sentry issue that doesn't have a 403 on /instances though, and only 403d on /domains and /users. Since domains isn't throwing the response on 403 but /users does, don't you think it's also worth handling that (which is what I currently have added on the changes)?

https://zestyio.sentry.io/issues/2277454417/events/a32f854cc9cd4e69a4e66ab13b68efed/

[agalin920]

@agalin920 I see, right I'll add some handling for that as well. There is an event under this similar sentry issue that doesn't have a 403 on /instances though, and only 403d on /domains and /users. Since domains isn't throwing the response on 403 but /users does, don't you think it's also worth handling that (which is what I currently have added on the changes)?

https://zestyio.sentry.io/issues/2277454417/events/a32f854cc9cd4e69a4e66ab13b68efed/

Yes it also needs to be handled on /users just keep in mind that the issue is NOT an unhandled catch. It is the throw res.

if (res.status === 200) {
        dispatch({
          type: users.actions.fetchUsersSuccess,
          payload: {
            data: res.data,
          },
        });
      } else {
        throw res;
      }

[agalin920]

That's it. Thanks