Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bubble API errors up. #588

Merged
merged 12 commits into from
Oct 14, 2020
76 changes: 40 additions & 36 deletions src/lib/API.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,13 @@ function createLogin(login, password) {
* @returns {Promise}
*/
function queueRequest(command, data) {
return new Promise((resolve) => {
return new Promise((resolve, reject) => {
// Add the write request to a queue of actions to perform
networkRequestQueue.push({
command,
data,
callback: resolve,
resolve,
reject,
});

// Try to fire off the request as soon as it's queued so we don't add a delay to every queued command
Expand Down Expand Up @@ -154,34 +155,6 @@ function request(command, parameters, type = 'post') {
return queueRequest(command, parameters);
}

// We treat Authenticate in a special way because unlike other commands, this one can't fail
// with 407 authToken expired. When other api commands fail with this error we call Authenticate
// to get a new authToken and then fire the original api command again
if (command === 'Authenticate') {
return xhr(command, parameters, type)
.then((response) => {
// If we didn't get a 200 response from authenticate we either failed to authenticate with
// an expensify login or the login credentials we created after the initial authentication.
// In both cases, we need the user to sign in again with their expensify credentials
if (response.jsonCode !== 200) {
throw new Error(response.message);
}

// Update the authToken so it's used in the call to createLogin below
authToken = response.authToken;
return response;
})
.then((response) => {
// If Expensify login, it's the users first time signing in and we need to
// create a login for the user
if (parameters.useExpensifyLogin) {
return createLogin(Str.generateDeviceLoginID(), Guid())
.then(() => setSuccessfulSignInData(response, parameters.exitTo));
}
})
.catch(error => Ion.merge(IONKEYS.SESSION, {error: error.message}));
}

// If we end up here with no authToken it means we are trying to make
// an API request before we are signed in. In this case, we should just
// cancel this and all other requests and set reauthenticating to false.
Expand Down Expand Up @@ -244,12 +217,20 @@ function request(command, parameters, type = 'post') {
}
return responseData;
})
.catch(() => {
.catch((error) => {
// If the request failed, we need to put the request object back into the queue as long as there is no
// doNotRetry option set in the parametersWithAuthToken
if (parametersWithAuthToken.doNotRetry !== true) {
queueRequest(command, parametersWithAuthToken);
}

// If we already have an error, throw that so we do not swallow it
if (error instanceof Error) {
throw error;
}

// Throw a generic error so we can pass the error up the chain
throw new Error(`API Command ${command} failed`);
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
});
}

Expand All @@ -276,7 +257,8 @@ function processNetworkRequestQueue() {

_.each(networkRequestQueue, (queuedRequest) => {
request(queuedRequest.command, queuedRequest.data)
.then(queuedRequest.callback);
.then(queuedRequest.resolve)
.catch(queuedRequest.reject);
});

networkRequestQueue = [];
Expand Down Expand Up @@ -368,7 +350,11 @@ function getAuthToken() {
*/
function authenticate(parameters) {
Ion.merge(IONKEYS.SESSION, {loading: true, error: ''});
return queueRequest('Authenticate', {

// We treat Authenticate in a special way because unlike other commands, this one can't fail
// with 407 authToken expired. When other api commands fail with this error we call Authenticate
// to get a new authToken and then fire the original api command again
return xhr('Authenticate', {
// When authenticating for the first time, we pass useExpensifyLogin as true so we check for credentials for
// the expensify partnerID to let users authenticate with their expensify user and password.
useExpensifyLogin: true,
Expand All @@ -379,13 +365,31 @@ function authenticate(parameters) {
twoFactorAuthCode: parameters.twoFactorAuthCode,
exitTo: parameters.exitTo,
})
.then((response) => {
// If we didn't get a 200 response from authenticate we either failed to authenticate with
// an expensify login or the login credentials we created after the initial authentication.
// In both cases, we need the user to sign in again with their expensify credentials
if (response.jsonCode !== 200) {
throw new Error(response.message);
}

// Update the authToken so it's used in the call to createLogin below
authToken = response.authToken;
return response;
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linter wanted a blank line here 🙃

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you move the comment to be outside of the .then() then the blank line before the comment will look nicer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to update the linter to not complain about this when the comment in one level more indented than the previous line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't find a rule in the linter that would make this pass. Ended up removing the comment, since (imo) it doesn't add a ton to this function overall

// After the user authenticates, create a new login for the user so that we can reauthenticate when the
// authtoken expires
.then(response => (
createLogin(Str.generateDeviceLoginID(), Guid())
.then(() => setSuccessfulSignInData(response, parameters.exitTo))
))
.catch((error) => {
console.error(error);
console.debug('[SIGNIN] Request error');
Ion.merge(IONKEYS.SESSION, {error: error.message});
}).finally(() => {
Ion.merge(IONKEYS.SESSION, {loading: false});
});
})
.finally(() => Ion.merge(IONKEYS.SESSION, {loading: false}));
}

/**
Expand Down