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

Fixed Improvements to error messaging issue #1091 #1166

Closed
wants to merge 13 commits into from
31 changes: 31 additions & 0 deletions src/libs/ErrorMessage/errorMessages.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
const errorMessages = [
{
errorCode: 401,
errorMessage: 'Incorrect login or password. Please try again.',
},
{
errorCode: 402,
errorMessage:
'You have 2FA enabled on this account. Please sign in using your email or phone number.',
},
{
errorCode: 403,
errorMessage:
'Invalid login or password. Please try again or reset your password.',
},
{
errorCode: 404,
errorMessage:
'We were unable to change your password. This is likely due to an expired password reset link in an old password reset email. We have emailed you a new link so you can try again. Check your Inbox and your Spam folder; it should arrive in just a few minutes.',
},
{
errorCode: 405,
errorMessage:
'You do not have access to this application. Please add your GitHub username for access.',
},
{
errorCode: 413,
errorMessage: 'Your account has been locked.',
},
];
export default errorMessages;
20 changes: 20 additions & 0 deletions src/libs/ErrorMessage/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import _ from 'lodash';
import errorMessages from './errorMessages';

/**
* It's function to print handle error messages in the context of Authenticate.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo? print handle error messages is awkward. Probably should be either/or.

Copy link
Author

Choose a reason for hiding this comment

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

@deetergp Okay, I think, It will be better. This function is handling error messages for the Authentication. else can you advise on it?

Copy link
Contributor

@deetergp deetergp Jan 13, 2021

Choose a reason for hiding this comment

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

At the end of the day, shouldn't your naming explain the "what" and your docs and comments explain the "why"? Like shouldn't function be called getFriendlyErrorMessage and the doc comment should briefly summarize the reason we need the function like Display a user-friendly message in the UI when the API returns an authentication error.

Copy link
Author

Choose a reason for hiding this comment

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

@deetergp Okay, Should we change with this Display a error message in the UI when the API returns an authentication error.?
If it works, please let me know. I'll change it. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

@devkashan1 I meant to move this logic to the context of Authenticate (in src/libs/Api.js) since that's the only spot where it's relevant.

Copy link
Author

Choose a reason for hiding this comment

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

@yuwenmemon Thanks for the clarification. I just have pushed the changes. You can verify now.

*
* @param {String} error
* @returns {String}
*/
function getErrorMessage(error) {
const code = error.split(' ')[0];
if (!_.isEmpty(code)) {
const errorMessage = _.filter(errorMessages, {errorCode: +code.trim()});
if (!_.isEmpty(errorMessage)) {
return errorMessage[0].errorMessage;
}
}
return error;
}
export default getErrorMessage;
5 changes: 3 additions & 2 deletions src/libs/actions/Session.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import CONFIG from '../../CONFIG';
import PushNotification from '../Notification/PushNotification';
import ROUTES from '../../ROUTES';
import Timing from './Timing';
import getErrorMessage from '../ErrorMessage';

let credentials = {};
Onyx.connect({
Expand Down Expand Up @@ -158,14 +159,14 @@ function signIn(password, exitTo, twoFactorAuthCode) {
Onyx.merge(ONYXKEYS.CREDENTIALS, {autoGeneratedLogin, autoGeneratedPassword});
})
.catch((error) => {
Onyx.merge(ONYXKEYS.ACCOUNT, {error: error.message});
Onyx.merge(ONYXKEYS.ACCOUNT, {error: getErrorMessage(error.message)});
})
.finally(() => {
Onyx.merge(ONYXKEYS.ACCOUNT, {loading: false});
});
})
.catch((error) => {
Onyx.merge(ONYXKEYS.ACCOUNT, {error: error.message, loading: false});
Onyx.merge(ONYXKEYS.ACCOUNT, {error: getErrorMessage(error.message), loading: false});
});
}

Expand Down