[HOLD] Handle errors on manual requests #22725

neil-marcellini · 2023-07-12T00:08:23Z

Details

Hello reviewer, thanks for taking a look. Here we are handling errors when creating a money request fails early on the backend. Manual requests can't really fail on the backend, but distance requests created while offline certainly can. This PR fixes the error handling in anticipation of that and is easier to test.

The most important change is to add some additional failure data when opening a failed money request report. Since that report doesn't exist we remove it and all data related to it. We also display an error message, and the same clean up runs when that message is dismissed.

Fixed Issues

$ #22126
PROPOSAL: N/A

Tests

Set up
Make all manual money requests fail by throwing at the top of this function. Add this line.

throw new ExpError('12124', 'Test error');

Test request and expense report creation failure t1

Create a new workspace
Green plus > Request money
Select the manual tab, enter an amount, next
Select the newly created workspace
Click request
Verify that there is a red dot on the report preview and an error message is shown below
Verify that there is a red dot shown on the row for the expense report in the chat list
Click on the report preview
Verify that you reach a not found page
Go back to the workspace chat
Verify that the workspace chat is empty other than the welcome message
Verify that the row for the expense report in the chat list was removed

Test request and expense report creation failure t2

Create a new workspace
Green plus > Request money
Select the manual tab, enter an amount, next
Select the newly created workspace
Click request
Verify that there is a red dot on the report preview and an error message is shown below
Verify that there is a red dot shown on the row for the expense report in the chat list
Click the X to dismiss the error message
Verify that the report preview was removed
Verify that the row for the expense report in the chat list was removed

Test only request creation failure

Comment out the line that throws an error in the backend
Green plus > Request money
Select the manual tab, enter an amount, next
Select the newly created workspace
Click request
Uncomment the line to throw an error in the backend
Run the tests above and verify that they pass with a separate, failed, report preview.

Verify that no errors appear in the JS console

Offline tests

Run the tests above, going offline before creating the failed request, then going online after.

QA Steps

No QA since there's no easy way to make the backend error. Later we will QA failure cases for distance requests.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

I only tested on web because the changes are platform independent.

Web

web1080.mov

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

cloudflare-workers-and-pages · 2023-10-06T16:28:46Z

Deploying with Cloudflare Pages

Latest commit:	`8d099a4`
Status:	✅ Deploy successful!
Preview URL:	https://300c7fe7.helpdot.pages.dev
Branch Preview URL:	https://neil-request-errors.helpdot.pages.dev

View logs

neil-marcellini · 2023-10-11T03:02:43Z

I'm going to put this up for review now, and then come back tomorrow to fix the unit tests.

melvin-bot · 2023-10-11T03:02:51Z

@allroundexperts Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

neil-marcellini · 2023-10-11T21:36:53Z

@allroundexperts I think it will be pretty hard for you to test without being able to make a change on the backend so I'm going to remove you as a reviewer.

src/libs/actions/Report.js

src/types/onyx/ReportAction.ts

tgolen · 2023-10-12T16:38:59Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

![2023-10-12_09-36-10](https://github.com/Expensify/App/assets/1228807/e65bc0e9-a7c2-4fd2-8e96-f336e188890a)

Desktop

iOS

Android

tgolen · 2023-10-12T16:39:23Z

I only tested on web because the changes are platform independent.

Can you please complete the testing on all platforms? You've checked off all the checkboxes for testing them and there are no notes or explanations about why the tests were not performed.

Here were some bugs I found while testing.

Bug 1: No RBR in the LHN for the workspace chat after creating request.

Bug 2: Create 2 failed requests, then 1 request that didn't fail. The expense report has two failed requests at the top, but you can only dismiss the error on one of them and the other one stays stuck. This happens anytime the first request is a failed one and the error on the first one can never be dismissed.

Bug 3: In the state above (where there is 1 failed and 1 good request on a report), clicking on the failed request doesn't show a NOT FOUND page and you can view the details of it.

Bug 4: Similar to Bug 3, on a slow connection, the first time you access the details of the failed request, you can see the details. If you go back, then select the request again, there is a skeleton loading UI and then the NOT FOUND page appears.

neil-marcellini · 2023-10-13T00:09:27Z

I only tested on web because the changes are platform independent.

Can you please complete the testing on all platforms? You've checked off all the checkboxes for testing them and there are no notes or explanations about why the tests were not performed.

That is the explanation for why I only tested on web. I guess it would a good idea to test on all platforms because some of this might behave differently on native due to styling, and because it's not a super simple flow.

neil-marcellini · 2023-10-13T00:13:16Z

Thanks for the thorough review @tgolen! I addressed some of your feedback but I haven't started on the bugs.

I think this PR is in a bit of a weird place and I would like to chat about the overall plan more in Slack another day. For example I've seen it suggested that the RBR should direct to the lowest level context item (the individual request), vs the top level (workspace chat) as I have implemented now. Also, I would think that a failed money request should be shown on an existing expense report when there is one, but that's not the case. There are many more considerations and I would like to come up with a better plan to fix all of it.

tgolen · 2023-10-13T15:09:05Z

I agree with you that this would be great to have more discussion about it. I think that the intentions were good for the current requirements, but when you actually try them out and see the UX, it becomes very clunky and difficult to navigate and recover from. I'll be on the lookout for that discussion to help out in whatever way I can!

tgolen · 2023-10-17T14:44:52Z

@neil-marcellini what do you think about either closing this, putting it into draft state, or putting HOLD in the title?

tgolen · 2023-10-20T15:45:42Z

@neil-marcellini what do you think about my previous suggestion?

think about either closing this, putting it into draft state, or putting HOLD in the title?

neil-marcellini · 2023-10-27T17:22:02Z

Oh yeah good point. I put it on HOLD (for planning) and marked it as a draft. After I get a chance to do a little pre-design I will either pick it back up or close it, depending on how different the chosen solution is.

neil-marcellini · 2024-01-19T00:15:24Z

Super old and I'm not coming back to this lol 😢

neil-marcellini added 16 commits July 11, 2023 17:07

Add RBR indicator on report preview

b3a44d3

Fix disable pay button and when report has errors

96e14de

Display money request transaction error messages

7fa160b

Add transactionErrors prop type and default

efd0783

WIP cleanUpFailedMoneyRequest

004ffed

Fix selector for transaction errors

e5a112e

Get last message text from actions

2dcd690

WIP Clean up failed request with updated total

e0ed7da

Merge branch 'main' into neil-request-errors

00b5d8f

Remove unused prop after merge

51e5128

Remove duplicate error message

1568548

Clean up money request when clearing action errors

ba72483

Fix crash by getting preview from workspace chat

e3338f9

Merge branch 'main' into neil-request-errors

a6b9ed0

Reset the report total after failed money request

b554a58

Merge branch 'main' into neil-request-errors

2cf82a2

neil-marcellini mentioned this pull request Sep 20, 2023

🔴 Fix error handling when creating a request fails #22126

Closed

neil-marcellini and others added 11 commits September 21, 2023 08:43

Fix settlement button after merge

a535623

WIP debug deleting action

eaeccd2

Fix deleting iou action, remove debugging

593d8e8

Show error message when iouReport creation fails

e43a5c4

Merge branch 'main' into neil-request-errors

c844962

Now that the onyx PR is merged, we can remove this workaround

79a6a9a

Merge branch 'main' into neil-request-errors

1eb4d6e

Bump the onyx version

7ccf623

Update types / delete the reportAction

51e2ad3

prettier

82d0567

Merge branch 'main' into neil-request-errors

cf6fd9e

Style

9f53c77

neil-marcellini added 2 commits October 10, 2023 19:52

Add JSDoc

b435855

Fix lint again

8476ff0

neil-marcellini marked this pull request as ready for review October 11, 2023 03:02

neil-marcellini requested a review from a team as a code owner October 11, 2023 03:02

melvin-bot bot requested review from allroundexperts and removed request for a team October 11, 2023 03:02

neil-marcellini added 3 commits October 11, 2023 10:06

Linked report must exist before deleting actions

1c14141

Merge branch 'main' into neil-request-errors

3e3937c

Fix missing code after merge

525fe27

neil-marcellini requested a review from tgolen October 11, 2023 21:34

neil-marcellini removed the request for review from allroundexperts October 11, 2023 21:36

tgolen requested changes Oct 12, 2023

View reviewed changes

src/libs/actions/Report.js Outdated Show resolved Hide resolved

src/libs/actions/Report.js Outdated Show resolved Hide resolved

src/libs/actions/Report.js Outdated Show resolved Hide resolved

src/types/onyx/ReportAction.ts Outdated Show resolved Hide resolved

neil-marcellini added 2 commits October 12, 2023 17:04

Implement review feedback

64866e7

More descriptive field comment

8d099a4

neil-marcellini changed the title ~~Handle errors on manual requests~~ [HOLD] Handle errors on manual requests Oct 27, 2023

neil-marcellini marked this pull request as draft October 27, 2023 17:19

neil-marcellini closed this Jan 19, 2024

neil-marcellini deleted the neil-request-errors branch January 19, 2024 00:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD] Handle errors on manual requests #22725

[HOLD] Handle errors on manual requests #22725

neil-marcellini commented Jul 12, 2023 •

edited

Loading

cloudflare-workers-and-pages bot commented Oct 6, 2023 •

edited

Loading

neil-marcellini commented Oct 11, 2023

melvin-bot bot commented Oct 11, 2023

neil-marcellini commented Oct 11, 2023

tgolen commented Oct 12, 2023 •

edited

Loading

tgolen commented Oct 12, 2023 •

edited

Loading

neil-marcellini commented Oct 13, 2023

neil-marcellini commented Oct 13, 2023

tgolen commented Oct 13, 2023

tgolen commented Oct 17, 2023

tgolen commented Oct 20, 2023

neil-marcellini commented Oct 27, 2023

neil-marcellini commented Jan 19, 2024

[HOLD] Handle errors on manual requests #22725

[HOLD] Handle errors on manual requests #22725

Conversation

neil-marcellini commented Jul 12, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

cloudflare-workers-and-pages bot commented Oct 6, 2023 • edited Loading

Deploying with Cloudflare Pages

neil-marcellini commented Oct 11, 2023

melvin-bot bot commented Oct 11, 2023

neil-marcellini commented Oct 11, 2023

tgolen commented Oct 12, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

tgolen commented Oct 12, 2023 • edited Loading

neil-marcellini commented Oct 13, 2023

neil-marcellini commented Oct 13, 2023

tgolen commented Oct 13, 2023

tgolen commented Oct 17, 2023

tgolen commented Oct 20, 2023

neil-marcellini commented Oct 27, 2023

neil-marcellini commented Jan 19, 2024

neil-marcellini commented Jul 12, 2023 •

edited

Loading

cloudflare-workers-and-pages bot commented Oct 6, 2023 •

edited

Loading

tgolen commented Oct 12, 2023 •

edited

Loading

tgolen commented Oct 12, 2023 •

edited

Loading