Fixed Improvements to error messaging issue #1091 #1166

devkashan1 · 2021-01-04T20:48:28Z

The details of the implementation for this problem that I have fixed in the project as I briefed above:
I have added a new file where we can manage any error messages at this place src\libs\ErrorMessage\messages.js.
I have also added another file where I created a function to fetch the appropriate message via type and error code. The path is src\libs\ErrorMessage\index.js.

After these, I have modified a file src\libs\actions\Session.js where we call the getErrorMessage function.
Line #11: import {getErrorMessage} from '../ErrorMessage';
Line #162: Onyx.merge(ONYXKEYS.ACCOUNT, {error: getErrorMessage('Login',error.message)});
Line #169: Onyx.merge(ONYXKEYS.ACCOUNT, {error: getErrorMessage('Login',error.message), loading: false});

That's all.

github-actions · 2021-01-04T20:48:43Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

devkashan1 · 2021-01-04T21:00:50Z

I have read the CLA Document and I hereby sign the CLA

devkashan1

I have reviewed everything. It was set via prettify formatter.

deetergp

Please address the linter failures. If you look at the rest of the code base you'll see that we use four-space indentation everywhere. Please do the same for src/libs/ErrorMessage/messages.js

devkashan1 · 2021-01-08T03:27:52Z

Thanks for letting me. I've fixed linter failures. Can you please review it now.

yuwenmemon · 2021-01-12T22:06:49Z

src/libs/ErrorMessage/errorMessages.js

@@ -0,0 +1,31 @@
+const erroMessages = [


Got it. Thanks

Done, You can verify please.

yuwenmemon · 2021-01-12T22:08:02Z

src/libs/ErrorMessage/index.js

+import errorMessages from './errorMessages';
+
+/**
+ * It's generic function to print any messages.


Let's remove the generic aspect of this. It's over-engineering. Right now we just need to handle error messages better in the context of Authenticate.

Done, You can verify please.

devkashan1 · 2021-01-12T22:10:26Z

Here is the details of the changes.
Replace a file name src\libs\ErrorMessage\messages.js. to src\libs\ErrorMessage\errorMessages.js. due to avoid misunderstanding of naming.
I had removed the type parameter that was considering as generic but now we can pass only parameter getErrorMessage(error.message)

Also, I have modified a file src\libs\actions\Session.js where we call the getErrorMessage function.
Line #162: Onyx.merge(ONYXKEYS.ACCOUNT, {error: getErrorMessage(error.message)});
Line #169: Onyx.merge(ONYXKEYS.ACCOUNT, {error: getErrorMessage(error.message), loading: false});

After that, I have changed this code for calling the error with one parameter and changed the message obj to errorMessage in src\libs\ErrorMessage\index.js

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;

Please let me know if you have any questions.

deetergp · 2021-01-13T00:46:08Z

src/libs/ErrorMessage/index.js

@@ -2,7 +2,7 @@ import _ from 'lodash';
 import errorMessages from './errorMessages';

 /**
- * It's generic function to print any messages.
+ * It's function to print handle error messages in the context of Authenticate.


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

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

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.

@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

@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.

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

deetergp · 2021-01-13T18:04:31Z

src/libs/ErrorMessage/index.js

@@ -2,7 +2,7 @@ import _ from 'lodash';
 import errorMessages from './errorMessages';

 /**
- * It's generic function to print any messages.
+ * It's function to print handle error messages in the context of Authenticate.


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.

yuwenmemon · 2021-01-14T00:43:51Z

src/libs/ErrorMessage/index.js

@@ -2,7 +2,7 @@ import _ from 'lodash';
 import errorMessages from './errorMessages';

 /**
- * It's generic function to print any messages.
+ * It's function to print handle error messages in the context of Authenticate.


@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.

yuwenmemon · 2021-01-14T06:33:48Z

src/libs/API.js

+ * @param {String} error
+ * @returns {String}
+ */
+function GetSignInErrorMessage(error) {


Ah what I meant was that you should probably place this in Authenticate. I also think you don't need a separate authenticationErrorMessages file either. It's overkill for what we're trying to achieve IMO

What I understood where you mentioned place in Authenticate. This means in src\libs\actions\Session.js file. So, This function GetSignInErrorMessage should be in this src\libs\actions\Session.js file, and no need separate file for authenticationErrorMessages. If you confirm me so I can proceed the changes.

@yuwenmemon Could you please confirm so I can push and close it?

yuwenmemon · 2021-01-19T21:13:27Z

Closing this PR as a solution was already merged

Fixed Improvements to error messaging issue Expensify#1091

6057930

devkashan1 requested a review from a team as a code owner January 4, 2021 20:48

botify requested review from deetergp and removed request for a team January 4, 2021 20:48

devkashan1 commented Jan 4, 2021

View reviewed changes

deetergp requested changes Jan 8, 2021

View reviewed changes

devkashan1 added 6 commits January 8, 2021 07:44

Linter fixes, added 2 spaces.

e97b93c

Fixed a couple of spaces

30ad015

Fixed a few move lines

687f758

Fixed spaces issues.

f6c220e

property shorthand fixed

9c4a69e

object-shorthand fixed

c03e278

devkashan1 added 2 commits January 13, 2021 01:51

fixed the error messages

3f15c26

removed type

8a0b114

devkashan1 mentioned this pull request Jan 12, 2021

Improvements to error messaging #1091

Closed

yuwenmemon requested changes Jan 12, 2021

View reviewed changes

typo fixed and changed the description of function

d5a16a0

deetergp approved these changes Jan 13, 2021

View reviewed changes

deetergp requested changes Jan 13, 2021

View reviewed changes

yuwenmemon requested changes Jan 14, 2021

View reviewed changes

devkashan1 added 3 commits January 14, 2021 08:26

Moved & changed the function in Session.js

0496c93

moved the function to API.js

677448f

Linter fixes

5212bc6

yuwenmemon requested changes Jan 14, 2021

View reviewed changes

yuwenmemon closed this Jan 19, 2021

github-actions bot locked and limited conversation to collaborators Jan 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed Improvements to error messaging issue #1091 #1166

Fixed Improvements to error messaging issue #1091 #1166

devkashan1 commented Jan 4, 2021

github-actions bot commented Jan 4, 2021 •

edited

Loading

devkashan1 commented Jan 4, 2021

devkashan1 left a comment

deetergp left a comment

devkashan1 commented Jan 8, 2021 •

edited

Loading

yuwenmemon Jan 12, 2021

devkashan1 Jan 12, 2021

devkashan1 Jan 12, 2021

yuwenmemon Jan 12, 2021

devkashan1 Jan 12, 2021

devkashan1 Jan 12, 2021

devkashan1 commented Jan 12, 2021

deetergp Jan 13, 2021

devkashan1 Jan 13, 2021

deetergp Jan 13, 2021 •

edited

Loading

devkashan1 Jan 13, 2021

yuwenmemon Jan 14, 2021

devkashan1 Jan 14, 2021

deetergp Jan 13, 2021 •

edited

Loading

yuwenmemon Jan 14, 2021

yuwenmemon Jan 14, 2021

devkashan1 Jan 14, 2021

devkashan1 Jan 15, 2021

yuwenmemon commented Jan 19, 2021

Fixed Improvements to error messaging issue #1091 #1166

Fixed Improvements to error messaging issue #1091 #1166

Conversation

devkashan1 commented Jan 4, 2021

github-actions bot commented Jan 4, 2021 • edited Loading

devkashan1 commented Jan 4, 2021

devkashan1 left a comment

Choose a reason for hiding this comment

deetergp left a comment

Choose a reason for hiding this comment

devkashan1 commented Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devkashan1 commented Jan 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deetergp Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deetergp Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuwenmemon commented Jan 19, 2021

github-actions bot commented Jan 4, 2021 •

edited

Loading

devkashan1 commented Jan 8, 2021 •

edited

Loading

deetergp Jan 13, 2021 •

edited

Loading

deetergp Jan 13, 2021 •

edited

Loading