Refactor payIOU

[youssef-lr]

Details

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/213712

Tests & QA

1. SendMoney

Offline

1. As User B, click on the global create menu +, select Send Money
2. Go through the prompt and select and account you have never chatted before, let's call it User A
3. In the last payment page, add a comment describing what the payment is for
4. Choose I'll settle up elsewhere option in the settlement button dropdown, if you have more options (otherwise you can only see this option)
5. Press the button to pay
6. Make sure you have been navigated to this new chat which is greyed out since it was optimistically created
7. Confirm there is IOU preview showing the correct amount you have sent with.
8. The report message says Sent $X for <comment>
9. Click on the payment details and the message and amount is also correct in the right hand side and greyed out.
10. Now click on the + in the chat report.
11. Select Send Money
12. Follow the prompt and send more money to User A
13. Confirm there has been another Sent $X for <comment> comment left and IOU preview with the correct amount
14. Now, click the global create + again and choose Send Money
15. This time choose account you have already chatted with before (User C)
16. Follow the prompts to sent money and choose I'll settle up elsewhere
17. Confirm you have been navigated to the correct report and the payment details are showing there greyed out

---

Online

1. Go back online
2. Confirm that the IOUPreview and the message are same and no longer greyed out in both of the chats
3. Click on the global create button + again, Send Money and choose either User A or User C
4. Follow the prompt and sent them money using the I'll settle up elsewhere option.
5. Confirm you have been navigated to the correct report and the payment details showed up correct, this time not greyed out.

---

Error

1. Login as User A and go offline
2. In other incognito window, login as User B
3. There is no previous chat between User A and User B
4. As User B, who is online, create chat with User A and send them a message
5. As User A, who is offline, click on the global create + button and select Send Money
6. Send money to User B and choose I'll settle elsewhere
7. Send the moeny and see new chat being created and greyed out.
8. As User A, go back online
9. See an error on the optimistically created chat
10. See the message from User B in a different report.
11. You are able to clear the stale report.

2. PayMoneyRequest

Offline

Pre-requisites:

• Have a chat between User A and User B, such that User B owes money to User A
• Have a chat between User B and User C, such that User B owes money User C

1. As User B, navigate to the chat with User A and you should see an IOU preview with Pay button
2. Press the Pay button
3. In the IOU Details page, choose I'll settle up elsewhere option in the dropdown and click the settlement button
4. Verify a greyed out Settled up elsewhere message displays immediately
5. Verify the IOU Preview at the top of the modal says You paid <name of the receiver> and the preview is greyed out
6. Click the X to close the modal and navigate away to a different chat and back again
7. Verify the IOUPreview and settled message in the chat view are greyed out
8. Click the IOUPreview to access the detailed view again
9. Verify that the message on the report and the IOUPreview in the header are still both greyed out

---

Online

1. Go back online
2. Confirm that the IOUPreview and the message don't change and are no longer greyed out.
3. Still as User B, navigate to the chat with User C
4. Click Pay in the IOU Details page, choose I'll settle up elsewhere option in the dropdown and click the settlement button
5. Verify the "Settled up elsewhere" message has immediately appeared in the IOU details modal and doesn't appear greyed out
6. Verify the IOU Preview reads You paid <name of the receiver> and is not greyed out.

---

Error

1. Login as User A
2. Request $10 from User B
3. Login as User B in another incognito window
4. Navigate to the chat report with User A
5. Go offline
6. Click Pay in the IOU Preview
7. As User A, go to the chat with User B and click on the IOU request details
8. Click Cancel on the $10 request
9. As User B, still offline, click the I'll settle up elsewhere button in the IOU details modal to pay it
10. Verify the greyed out "Settled up elsewhere" message has shown up and the report shows as paid.
11. Go back online
12. Verify you see an error message in-line because the request was cancelled before the payment was made offline.
13. Navigate back to the LHN
14. Verify you see a single red dot on the chat row that contains the failed settlement
15. Navigate into the chat
16. Verify you see an in-line error message on the failed settlement
17. Click/tap the X to dismiss the error message
18. Verify the error message disappears

---

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover 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 included screenshots or videos for tests on all platforms
• [] I ran the tests on all platforms & verified they passed on:
  • [] iOS / native
  • [] Android / native
  • [] iOS / Safari
  • [] Android / Chrome
  • MacOS / Chrome
  • [] MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (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 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 was added in all src/languages/* files
  • I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team 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 followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• 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 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 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.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

• 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 expected offline behavior are in the Offline steps 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• 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:
  • iOS / native
  • Android / native
  • iOS / Safari
  • Android / Chrome
  • MacOS / Chrome
  • 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 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 was added in all src/languages/* files
  • I verified any copy / text that was added to the app is correct English and 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
• I verified that if a function's arguments changed that all usages have also been updated correctly
• 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 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 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.
• I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots

Web

Offline - SendMoney
https://user-images.githubusercontent.com/9680864/204683072-6bde8506-963a-44fd-af41-585d907f230c.mov

Online - SendMoney
https://user-images.githubusercontent.com/9680864/204683025-253dbb7d-4a28-4ae5-8172-dc16ec40cc43.mov

Error - SendMoney

Error.mov

Error 2 - SendMoney

send.money.-.error.mov

PayMoneyRequest

PayMoneyRequest.mov

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

[mountiny]

@youssef-lr I think we should in a big part just reused the requestMoney here and then add what we need for this specific command.

Essentially the good thing about Send money request is that we dont have to update any of the totals etc since that is untouched, we will add a new report actions, that is requested and paid.

[mountiny]

@youssef-lr on a good track, just couple of comments

[mountiny]

@youssef-lr Love the refactor, lets add videos and make this ready for a review as well as the Web-PR.

[mountiny]

Suggested change

	} = getPayMoneyRequestParams(
	chatReport, iouReport, recipient, CONST.IOU.PAYMENT_TYPE.ELSEWHERE,
	);
	} = getPayMoneyRequestParams(chatReport, iouReport, recipient, CONST.IOU.PAYMENT_TYPE.ELSEWHERE);

[youssef-lr]

I'm trying to fix the spinner issue, it seems like the iouReport is not immediately available in IOUPreview, I tried with request money and I don't see "report is loading" being logged at all, but with sendMoney it does. I'm not sure why as the code is the same for both actions.

[youssef-lr]

@luacmartins for the IOU Preview bug after an error, it's because of the check here which decides if we should show it or it. When we pay a cancelled request, the last IOU action (which is of type 'cancel') satisfies that check and we end up showing the preview. I fixed it in this commit by adding another check. Let me know what you think!

Screen.Recording.2022-12-08.at.03.55.18.mov

[youssef-lr]

I think props.chatReport.hasOutstandingIOU || props.action.originalMessage.type === 'pay' alone might be enough.

[mollfpr]

@youssef-lr That will make the IOUPreview shown on every IOU report.

We should keep props.isMostRecentIOUReportAction to show IOUPreview for recent IOUReport.

[mountiny]

@youssef-lr

I think props.chatReport.hasOutstandingIOU || props.action.originalMessage.type === 'pay' alone might be enough.

this wont be enough will it, you can send money no matter if you have existing pay request there

[youssef-lr]

Yeah @mollfpr @mountiny sorry scratch that.

[mollfpr]

Are we ready for another round?

[youssef-lr]

@mollfpr Correct, but there's one bug/improvement I can't figure out yet.

[mountiny]

@youssef-lr same here, we have paid this up and we included all the data about the iou report in here so we can use set can we not?

Suggested change

	onyxMethod: CONST.ONYX.METHOD.MERGE,
	onyxMethod: CONST.ONYX.METHOD.SET,

[luacmartins]

I think that we should use merge if the IOUReport is already stored in Onyx. The difference between this and sendMoney is that sendMoney creates an IOUReport that doesn't yet exist in Onyx.

[youssef-lr]

I agree, here we already have an iouReport that we're settling up.

[youssef-lr]

This is ready for another round!

[luacmartins]

LGTM and tests well! Awesome job @youssef-lr!

[mountiny]

@youssef-lr great job ❤️

[mollfpr]

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:
  • iOS / native
  • Android / native
  • iOS / Safari
  • Android / Chrome
  • MacOS / Chrome
  • 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 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 was added in all src/languages/* files
  • I verified any copy / text that was added to the app is correct English and 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 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 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.
• 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

Desktop

12739.Desktop.SendMoney.Offline.mov

12739.Desktop.SendMoney.Online.mov

12739.Desktop.SendMoney.Error.mov

12739.Desktop.RequestMoney.Online.mov

12739.Desktop.RequestMoney.Error.mov

12739.Desktop.RequestMoney.Offline.mov

iOS

12739.iOS.SendMoney.Offline.mov

12739.iOS.SendMoney.Online.mov

12739.iOS.SendMoney.Error.mov

12739.iOS.RequestMoney.Offline.mov

12739.iOS.RequestMoney.Online.mov

Android

12739.Android.SendMoney.Offline.mov

12739.Android.SendMoney.Online.mov

12739.Android.SendMoney.Error.mov

12739.Android.RequestMoney.Offline.mov

12739.Android.RequestMoney.Online.mov

12739.Android.RequestMoney.Error.mov

[mollfpr]

LGTM tests well 👍

Checklist and scree records

[luacmartins]

Cool, we got the approvals. Let's merge this baby!

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[luacmartins]

@laurenreidexpensify could you please pay @mollfpr for their review on this PR (once the regression period is over)?

[mountiny]

This has been deployed to staging

[OSBotify]

🚀 Deployed to production by @chiragsalian in version: 1.2.38-6 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[laurenreidexpensify]

@mollfpr Upwork job here https://www.upwork.com/jobs/~01166169da982f6883 payment will be on 20 Dec

[mollfpr]

Accepted, thanks @laurenreidexpensify

[laurenreidexpensify]

Paid 🎉

[MariaHCD]

Just a note here that since we didn't clear the iouReportID in the successData here, it caused this: #16105

[mountiny]

Oh, good find

[rushatgabhane]

Hi, we missed a case CONST.IOU.PAYMENT_TYPE.VBBA causing a bug #21535