Refactor createIOUTransaction to RequestMoney by Gonals · Pull Request #10843 · Expensify/App

Gonals · 2022-09-06T16:23:35Z

Details

Fixed Issues

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

Tests

Let's do this!

In Newdot, open an existing chat with no IOUs and request money:

The request should show immediately with the correct amounts. After a few seconds, nothing should change:

Try adding more requests in the same chat. They should all show up immediately and correctly, and the total should get updated:

Now, try requesting money from the big + button and choose someone with NO chat with you.
A new chat should be created immediately and the money request should show there normally.
Go offline.
Repeat steps 1-5. Everything should work the same, but the created IOUs should show semitransparent:

Go back online. The IOUs should go back to full opacity

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

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

PR Reviewer Checklist

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

QA Steps

Same as tests

Verify that no errors appear in the JS console

melvin-bot · 2022-09-06T16:23:40Z

Looks like you modified deprecatedAPI.js! To be clear, you should not be adding any code to this file.

Instead, all new API commands should use API.js, and follow our guidelines for writing new API commands.

Unsure if your change is okay? Drop a note in #expensify-open-source!

alex-mechler · 2022-09-09T09:36:47Z

Removing my review request as I'll be OOO all next week. Grab someone fresh with pullerbear when this is ready!

luacmartins

@Gonals Actually, I just noticed that the last message is not updated on the LHN when we are offline 😭

web.mov

luacmartins · 2022-10-26T18:15:47Z

@Gonals I noticed the bug in my split bill PR too and updating the report with lastMessageText and lastMessageHtml like I did here solves the issue

Gonals · 2022-10-26T18:38:27Z

@luacmartins, I could not reproduce the issue, but updating the lastMessage won't hurt and will get the optimistic report closer to the actual thing, so I did it anyways. It seems to test well!

luacmartins

LGTM and tests well (couldn't test offline on iOS simulator). Known issues:

LHN last message is wiped when reconnecting. @Julesssss seems to be aware of this and working on a fix in #12082
Unread message indicator is inconsistent. I think this has the same cause as https://github.com/Expensify/Expensify/issues/229696, since the ReconnectApp READ replaces the lastReadSequenceNumber before we send the WRITE request.

Web

web.mov

Desktop

desktop.mov

iOS

ios.mov

Safari

safari.mov

Android

android.mov

Chrome

chrome.mov

luacmartins · 2022-10-26T20:15:59Z

Gonna merge this since @mountiny and @Julesssss had approved it already. At this point, any issues that come up (aside from the known issues I mentioned above) should be minor and can be addressed as a polish follow up. Thanks for hanging on to this @Gonals! Awesome work!

OSBotify · 2022-10-26T20:20:06Z

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

Santhosh-Sellavel · 2022-10-26T20:20:17Z

Oops I should've left a comment here, @mountiny had asked for C+ review here

I was reviewing this now.

luacmartins · 2022-10-26T20:41:17Z

@Santhosh-Sellavel feel free to leave any post merge reviews. If you catch anything, we can open up issues to address any them.

OSBotify · 2022-10-26T23:19:34Z

🚀 Deployed to staging by @luacmartins in version: 1.2.20-0 🚀

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

Julesssss · 2022-10-27T08:02:09Z

Great work @Gonals, I think you should take the rest of the day off to celebrate

OSBotify · 2022-10-28T02:24:16Z

🚀 Deployed to production by @chiragsalian in version: 1.2.20-3 🚀

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

Santhosh-Sellavel · 2022-10-30T20:32:55Z

Sorry for the delay, since this is merged, focused on other issues.

Since this is deployed. Tested on staging but fails, and ran into something went wrong.

Steps:

Open a chat without an IOU
Create a couple of IOU Requests
Go offline
Create one more IOU
Now creating an IOU with User Never chated
Added on more to same.
Go Online

A Red dot appeared on those chats. Shows an error, refer video for more.

Desktop.mov

Santhosh-Sellavel

Code Looks good, I feel unit tests are missed for independent Utils.

Santhosh-Sellavel · 2022-10-26T19:41:24Z

+function calculateAmount(participants, total, isDefaultUser = false) {
+    // Convert to cents before working with iouAmount to avoid
+    // javascript subtraction with decimal problem -- when dealing with decimals,
+    // because they are encoded as IEEE 754 floating point numbers, some of the decimal
+    // numbers cannot be represented with perfect accuracy.
+    // Cents is temporary and there must be support for other currencies in the future
+    const iouAmount = Math.round(parseFloat(total * 100));
+    const totalParticipants = participants.length + 1;
+    const amountPerPerson = Math.round(iouAmount / totalParticipants);
+
+    if (!isDefaultUser) {
+        return amountPerPerson;
+    }
+
+    const sumAmount = amountPerPerson * totalParticipants;
+    const difference = iouAmount - sumAmount;
+
+    return iouAmount !== sumAmount ? (amountPerPerson + difference) : amountPerPerson;
+}


Can we add unit tests for this file?

Those are quite good shouts, I think such issues could be external, what do you think @Julesssss @luacmartins @Gonals

Santhosh-Sellavel · 2022-10-26T19:50:56Z

+function buildOptimisticIOUReportAction(sequenceNumber, type, amount, currency, comment, paymentType = '', iouTransactionID = '', iouReportID = '', debtorEmail = '', locale = 'en') {
+    const IOUTransactionID = iouTransactionID || NumberUtils.rand64();


Not sure why unit tests for this missed out.

Santhosh-Sellavel · 2022-10-31T16:56:33Z

Sorry for the delay, since this is merged, focused on other issues.

Since this is deployed. Tested on staging but fails, and ran into something went wrong.

Steps:

Open a chat without an IOU

Create a couple of IOU Requests

Go offline

Create one more IOU

Now creating an IOU with User Never chated

Added on more to same.

Go Online

A Red dot appeared on those chats. Shows an error, refer video for more.

Desktop.mov

what about this one
@mountiny ?

mountiny · 2022-10-31T17:03:27Z

@Santhosh-Sellavel Can you report this in the Slack channel, that might be the best way forward. Thanks!

Gonals · 2022-12-15T09:54:30Z

Hey! @Gonals (that's me!) @mountiny @Julesssss @luacmartins, this introduced a bug here: #13511

amyevans · 2022-12-15T17:00:08Z

👋 Hey y'all! Some of you are aware since I tagged you when initially working on the bug, but for everyone's benefit and to complete the Bug Zero checklist: this PR resulted in a bug here #12432

Gonals added 8 commits September 5, 2022 11:50

remove old API call and create skeleton of new one

1b4218d

add fake data

a5912bf

Merge branch 'main' into alberto-requestMoney

8de1ab4

error message

da2704b

use existing chat when available

8490938

remove unneeded code

ccd0e51

add missing params

cfd5408

style

5e489a2

Gonals requested a review from a team as a code owner September 6, 2022 16:23

Gonals self-assigned this Sep 6, 2022

melvin-bot Bot requested review from alex-mechler and removed request for a team September 6, 2022 16:24

Gonals added 5 commits September 7, 2022 16:11

resolve conflicts

76115dc

move to actual params

4da1f9d

pass parameters

41f975d

remove extra function

86ff05b

Merge branch 'main' into alberto-requestMoney

417226a

alex-mechler removed their request for review September 9, 2022 09:36

Gonals added 5 commits September 12, 2022 11:40

Use existing IOU report when available

4b0979c

load ious early

706ab51

build better optimistic action

d1d0464

handle total IOU owed

9373c1d

Do not disable button when offline

567595d

Gonals changed the title ~~[WIP] [HOLD 34756] Refactor createIOUTransaction to RequestMoney~~ [WIP] Refactor createIOUTransaction to RequestMoney Sep 13, 2022

Gonals added 3 commits September 13, 2022 19:55

update comments

1adea53

add translation

464f429

add JSDOc

e253231

Gonals requested a review from Julesssss September 14, 2022 10:24

luacmartins reviewed Oct 26, 2022

View reviewed changes

luacmartins self-requested a review October 26, 2022 16:19

Gonals added 2 commits October 26, 2022 20:22

Merge branch 'main' into alberto-requestMoney

587575a

update lastMessage

1031f7a

Gonals dismissed stale reviews from luacmartins, Julesssss, and mountiny via 1031f7a October 26, 2022 18:37

luacmartins requested changes Oct 26, 2022

View reviewed changes

Comment thread src/libs/actions/IOU.js Outdated

treat message as an array

d1183ad

luacmartins approved these changes Oct 26, 2022

View reviewed changes

luacmartins merged commit a341945 into main Oct 26, 2022

luacmartins deleted the alberto-requestMoney branch October 26, 2022 20:16

OSBotify mentioned this pull request Oct 26, 2022

Deploy Checklist: New Expensify 2022-10-26 #12165

Closed

28 tasks

Santhosh-Sellavel reviewed Oct 30, 2022

View reviewed changes

Gonals mentioned this pull request Dec 15, 2022

[HOLD for payment 2022-12-22] [HOLD for payment 2022-12-20] If user tries to Request money after settling, error message is shown #13511

Closed

amyevans mentioned this pull request Dec 15, 2022

[HOLD for payment 2022-12-20] Web- Chat - Onyx error is displayed in console when requesting money #12432

Closed

		function buildOptimisticIOUReportAction(sequenceNumber, type, amount, currency, comment, paymentType = '', iouTransactionID = '', iouReportID = '', debtorEmail = '', locale = 'en') {
		const IOUTransactionID = iouTransactionID \|\| NumberUtils.rand64();

Uh oh!

Conversation

Gonals commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

PR Reviewer Checklist

QA Steps

Uh oh!

melvin-bot Bot commented Sep 6, 2022

Uh oh!

alex-mechler commented Sep 9, 2022

Uh oh!

luacmartins left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luacmartins commented Oct 26, 2022

Uh oh!

Gonals commented Oct 26, 2022

Uh oh!

Uh oh!

luacmartins left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luacmartins commented Oct 26, 2022

Uh oh!

OSBotify commented Oct 26, 2022

Uh oh!

Santhosh-Sellavel commented Oct 26, 2022

Uh oh!

luacmartins commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OSBotify commented Oct 26, 2022

Uh oh!

Julesssss commented Oct 27, 2022

Uh oh!

OSBotify commented Oct 28, 2022

Uh oh!

Santhosh-Sellavel commented Oct 30, 2022

Uh oh!

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

Uh oh!

Santhosh-Sellavel Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

mountiny Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

Santhosh-Sellavel Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

Santhosh-Sellavel commented Oct 31, 2022

Uh oh!

mountiny commented Oct 31, 2022

Uh oh!

Gonals commented Dec 15, 2022

Uh oh!

amyevans commented Dec 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Gonals commented Sep 6, 2022 •

edited

Loading

luacmartins left a comment •

edited

Loading

luacmartins left a comment •

edited

Loading

luacmartins commented Oct 26, 2022 •

edited

Loading