Optimistically generate transactions in existing IOU flows #17718

roryabraham · 2023-04-20T15:06:17Z

Details

Optimistically generate transactions for IOU flows and maintain their state under the new transaction Onyx collection.

Fixed Issues

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

Tests

Request Money

Go offline.
Use the global create menu to request money from someone you've never chatted with before. Make sure you include a comment.
Verify that the chat report and IOU request is created correctly, including the comment. Look in Onyx, and verify that a transaction was created for the IOU under the transaction collection key. The comments and transaction should be pending. Verify that the transaction has the pendingAction field in Onyx.
In the chat that was created in the previous step, request more money. Verify that the IOU actions are created, the total is updated, and that a second transaction appears in Onyx under the transaction collection key.
Come online, verify everything finalizes correctly.
Request money a third time, and cancel that money request. Verify that the amounts update correctly.
Repeat the previous step offline, verify it works as expected. Then go back online.
As the other person, settle up the money request. Verify that for both users the IOU shows as having been settled up.
Edit the RequestMoney api endpoint to just immediately throw a 400 error.
Go offline.
Use the global create menu to request money from another user you've never chatted with.
Verify that the optimistic report, reportActions, and transactions appear as expected.
Come back online.
Verify that you see RBR errors for the report + all the reportActions. Look in Onyx and verify that the transaction has an errors field.
Clear all the RBR errors, verify that everything disappears, including the optimistic transaction in Onyx.

Split Bill

For the following steps, let's assume we have 3 fresh accounts, A, B, and C.

As user A, create a 1:1 chat with user B. Request $50 from that user.
As user A, create a 1:1 chat with user C. Send them some messages, but do not request money from them.
As user A, go offline.
As user A, split a bill for $100 with A, B, C, and D, where D is an email not currently associated with an Expensify account.
Verify that a group chat is optimistically created with the four users, and the bill split IOU action appears for the $100 bill.
Verify that the chat with user B now shows a $75 debt (the $50 from before plus their $25 portion of the bill split.
Verify that the chat with C has been updated to show the $25 debt
Verify that a 1:1 chat with user D has been optimistically created and updated with the $25 IOU debt.
Look in Onyx, and there should be four transactions – a $100 for the group, and three $25 (1:1) transactions. They should all be pending.
As user A, come back online.
Verify that the group chat and 1:1 chat with D has been finalized. Reverify all the IOU amounts. Look at the transactions in Onyx and verify that they are no longer pending.
As user C, settle up the IOU. Verify that the 1:1 IOU is reconciled.
As user A, go back offline.
Edit the SplitBill API endpoint to throw an error.
Split another $100 with A, B, C, and D.
Verify everything is created optimistically again.
Now come back online and verify that everything from the previous step now shows an error.
Clear out the errors, and verify that the optimistic entities disappear from Onyx.

Verify that no errors appear in the JS console

Offline tests

Included above.

QA Steps

Maybe controversial, but everything we've changed here should be covered by regression tests already, and we aren't expecting any new or changed behaviors from a user perspective here. So this is No QA, just regressions only.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

AndroidmWeb.mov

Mobile Web - Safari

iOSWeb.mov

Desktop

DesktopOffline.mov

DesktopComesBackOnline.mov

iOS

iOS.mov

Android

Android.mov

roryabraham · 2023-04-21T17:29:17Z

tests/actions/ReportTest.js

-            authEndpoint: `${CONFIG.EXPENSIFY.DEFAULT_API_ROOT}api?command=AuthenticatePusher`,
-        });
-
+        PusherHelper.setup();


Didn't end up needing this PusherHelper change, but I do think it's a potentially useful abstraction, so I left it here. I can remove it if you prefer

roryabraham · 2023-04-24T19:24:55Z

src/libs/ReportUtils.js

 * @returns {Object}
 */
-function buildOptimisticIOUReportAction(type, amount, currency, comment, participants, paymentType = '', iouTransactionID = '', iouReportID = '', isSettlingUp = false) {
-    const IOUTransactionID = iouTransactionID || NumberUtils.rand64();
+function buildOptimisticIOUReportAction(type, amount, currency, comment, participants, iouTransactionID, paymentType = '', iouReportID = '', isSettlingUp = false) {


Review note: iouTransactionID became a required param because it needs to be associated with an actual transaction in Onyx and not an ID generated on the fly here.

roryabraham · 2023-04-24T19:32:02Z

tests/unit/IOUUtilsTest.js

This diff looks worse than it is. Tests were failing, and all I did was:

Wrap all tests in a single describe, which is the most common pattern we have for tests, and makes it easier to run or debug all the tests in a file in VSCode or JetBrains IDEs. (this is why the diff looks crazy)

Changed the method signature of createIOUReportAction and cancelMoneyRequests to account for the fact that IOUTransactionID became a required param in ReportUtils.buildOptimisticIOUReportAction

Changed the method signature of cancelMoneyRequest to account for 2

s77rt · 2023-05-04T21:13:55Z

Cool. We just have some RORY_DEBUG logs 😅

s77rt · 2023-05-04T21:39:27Z

Another bug, seems to be native only

roryabraham · 2023-05-04T21:50:31Z

Not able to reproduce on iOS:

Edit: also tried with Euros, seems to work fine

src/libs/actions/Report.js

mountiny · 2023-05-04T23:37:03Z

@roryabraham the github actions job2 is still failing, have you been able to look into that?

roryabraham · 2023-05-04T23:52:18Z

@mountiny @s77rt I reverted the last two commits (force-pushed so they're just gone), but yeah they were causing tests to fail and were just for an unbenchmarked performance optimization suggested here. I was kind of unsure about the change anyways since it didn't really follow our patterns. I think this is fine

s77rt · 2023-05-05T00:02:03Z

I still think allReportActions is a little too much. Basically the whole onyx is now on memory 😅. We should probably find a better way to get transactions given a report id. I won't block on that though given the high priority.

s77rt

LGTM! 🚀

melvin-bot · 2023-05-05T00:03:05Z

🎯 @s77rt, thanks for reviewing and testing this PR! 🎉

An E/App issue has been created to issue payment here: #18461.

roryabraham · 2023-05-05T00:10:27Z

Basically the whole onyx is now on memory 😅.

Agree. FWIW, the same code already exists in ReportActionsUtils. Plus Onyx itself has an in-memory cache later, so now all the reportActions are in memory 3x 🙃. These are the existing patterns so I think we're ok for now given the priority, but I agree it seems like we should come up with a better way to handle this, such as just implementing Onyx.get (which would first pull from cache and only go into storage if it's a cache miss)

luacmartins

Left a few comments

src/libs/CurrencyUtils.js

src/libs/actions/IOU.js

Co-authored-by: Carlos Martins <[email protected]>

comments addressed

OSBotify · 2023-05-05T04:47:15Z

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

OSBotify · 2023-05-05T20:02:23Z

🚀 Deployed to staging by https://github.com/luacmartins in version: 1.3.11-0 🚀

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

OSBotify · 2023-05-09T04:04:58Z

🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.12-0 🚀

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

s77rt · 2023-06-21T22:01:46Z

src/libs/actions/IOU.js

+        failureData.push({
+            onyxMethod: CONST.ONYX.METHOD.MERGE,
+            key: optimisticChatReportData.key,
+            value: {
+                errorFields: {
+                    createChat: {
+                        [DateUtils.getMicroseconds()]: Localize.translateLocal('report.genericCreateReportFailureMessage'),
+                    },
+                },
+            },
+        });


This caused a regression #18839. We already set an error for the report action (in line 883) and we are setting another error for the report itself. This resulted in redundant error messages.

aldo-expensify · 2023-08-04T22:42:38Z

src/libs/CurrencyUtils.js

+ * Takes an amount as a floating point number and converts it to an integer amount.
+ * For example, given [25, USD], return 2500.
+ * Given [25.50, USD] return 2550.
+ * Given [2500, JPY], return 2500.
+ *
+ * @note we do not currently support any currencies with more than two decimal places. Sorry Tunisia :(
+ *
+ * @param {String} currency
+ * @param {Number} amountAsFloat
+ * @returns {Number}
+ */
+function convertToSmallestUnit(currency, amountAsFloat) {
+    const currencyUnit = getCurrencyUnit(currency);
+    return Math.trunc(amountAsFloat * currencyUnit);
+}


Is this supposed to be used for the amount of a transaction we send to the backend? If that's the case, this is wrong. If in the front end we input 25 JPY, the backend expects 2500. The backend ALWAYS works in "cents", even with currencies that don't have cents.

Investigating this bug: #23235

I'm not sure if this is the cause, but it may be it.

Fixing here: #24175

roryabraham added 9 commits April 20, 2023 08:37

Create TransactionUtils.buildOptimisticTransaction

c94e8f4

Oops, add export

5705e77

Make comment optional in buildOptimisticTransaction

962009f

Setup transaction data w/ RBR in RequestMoney flow

c2ab2fc

Make transactionID required in buildOptimisticIOUAction

8d998d9

Finish RequestMoney flow

b68362d

Implement createSplitsAndOnyxData

a6aec7f

Implement getPayMoneyRequestParams

ae89202

Implement getSendMoneyParams

42ff9e4

roryabraham self-assigned this Apr 20, 2023

Update JSDoc comment to make it clear iouTransactionID is not optional

d997104

luacmartins self-requested a review April 20, 2023 15:10

roryabraham added 7 commits April 20, 2023 11:30

Fix lint

6d8d11d

Create PusherHelper abstraction

d85e4a0

Create test for requestMoney

c858075

Refactor fetch mock

9176a17

Create test for requestMoney with existing chatReport

f622e5c

Create test for requestMoney with existing chatReport and iouReport

4b79c27

Add test for failing scenario

619e5cb

roryabraham commented Apr 21, 2023

View reviewed changes

roryabraham added 2 commits April 24, 2023 11:01

Merge branch 'main' into Rory-OptimisticTransactions

cb9ceed

Remove unused PusherHelper

83b0bc7

roryabraham mentioned this pull request Apr 24, 2023

Clear cache in Onyx.clear() Expensify/react-native-onyx#245

Merged

roryabraham added 5 commits April 24, 2023 13:04

Update TODO link

afda245

Merge branch 'main' into Rory-OptimisticTransactions

0fe4736

Upgrade Onyx to 1.0.41

3ee3eb8

Remove resolved TODOS

0a1a59b

Fix IOUUtilsTest

d7c84c6

roryabraham commented Apr 24, 2023

View reviewed changes

Remove debug logs

7a9fbbc

Fix lint

74bef77

s77rt suggested changes May 4, 2023

View reviewed changes

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

roryabraham force-pushed the Rory-OptimisticTransactions branch from 10a072d to 74bef77 Compare May 4, 2023 23:50

s77rt previously approved these changes May 5, 2023

View reviewed changes

melvin-bot bot mentioned this pull request May 5, 2023

[Pay on 16/5] Payment issue for E/App PR#17718 #18461

Closed

luacmartins requested changes May 5, 2023

View reviewed changes

Update src/libs/CurrencyUtils.js

78b59bd

Co-authored-by: Carlos Martins <[email protected]>

roryabraham dismissed s77rt’s stale review via 78b59bd May 5, 2023 04:18

Update src/libs/CurrencyUtils.js

dbf456d

Co-authored-by: Carlos Martins <[email protected]>

roryabraham requested a review from luacmartins May 5, 2023 04:32

luacmartins approved these changes May 5, 2023

View reviewed changes

luacmartins merged commit 230b7f7 into main May 5, 2023

luacmartins deleted the Rory-OptimisticTransactions branch May 5, 2023 04:39

OSBotify mentioned this pull request May 5, 2023

Deploy Checklist: New Expensify 2023-05-05 #18498

Closed

31 tasks

s77rt reviewed Jun 21, 2023

View reviewed changes

s77rt mentioned this pull request Jun 21, 2023

[BONUS PAYOUT ONLY] [$1000] Chat - Redundant "Unexpected error" below the IOU" message after split from 2 devices #18839

Closed

6 tasks

aldo-expensify reviewed Aug 4, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimistically generate transactions in existing IOU flows #17718

Optimistically generate transactions in existing IOU flows #17718

roryabraham commented Apr 20, 2023 •

edited

Loading

roryabraham Apr 21, 2023

roryabraham Apr 24, 2023

roryabraham Apr 24, 2023

s77rt commented May 4, 2023

s77rt commented May 4, 2023

roryabraham commented May 4, 2023 •

edited

Loading

mountiny commented May 4, 2023

roryabraham commented May 4, 2023

s77rt commented May 5, 2023

s77rt left a comment

melvin-bot bot commented May 5, 2023

roryabraham commented May 5, 2023 •

edited

Loading

luacmartins left a comment

OSBotify commented May 5, 2023

OSBotify commented May 5, 2023

OSBotify commented May 9, 2023

s77rt Jun 21, 2023

aldo-expensify Aug 4, 2023 •

edited

Loading

aldo-expensify Aug 4, 2023

aldo-expensify Aug 4, 2023

Optimistically generate transactions in existing IOU flows #17718

Optimistically generate transactions in existing IOU flows #17718

Conversation

roryabraham commented Apr 20, 2023 • edited Loading

Details

Fixed Issues

Tests

Request Money

Split Bill

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

roryabraham Apr 21, 2023

Choose a reason for hiding this comment

roryabraham Apr 24, 2023

Choose a reason for hiding this comment

roryabraham Apr 24, 2023

Choose a reason for hiding this comment

s77rt commented May 4, 2023

s77rt commented May 4, 2023

roryabraham commented May 4, 2023 • edited Loading

mountiny commented May 4, 2023

roryabraham commented May 4, 2023

s77rt commented May 5, 2023

s77rt left a comment

Choose a reason for hiding this comment

melvin-bot bot commented May 5, 2023

roryabraham commented May 5, 2023 • edited Loading

luacmartins left a comment

Choose a reason for hiding this comment

OSBotify commented May 5, 2023

OSBotify commented May 5, 2023

OSBotify commented May 9, 2023

s77rt Jun 21, 2023

Choose a reason for hiding this comment

aldo-expensify Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

aldo-expensify Aug 4, 2023

Choose a reason for hiding this comment

aldo-expensify Aug 4, 2023

Choose a reason for hiding this comment

roryabraham commented Apr 20, 2023 •

edited

Loading

roryabraham commented May 4, 2023 •

edited

Loading

roryabraham commented May 5, 2023 •

edited

Loading

aldo-expensify Aug 4, 2023 •

edited

Loading