Adds optimistic data for transaction threads with violations #35449

cdanwards · 2024-01-31T02:37:34Z

Details

Fixed Issues

PROPOSAL:

This PR addresses the need to add the optimistic data necessary for transaction threads to display in the LHN even when offline. We do this by creating transaction threads and created actions for those threads and adding that data to the optimistic data within RequestMoney, CreateDistanceRequest, and SplitBill and subsequently passing that thread and created action data to the API Calls.

There were a few instances where logic needed to be altered in order for transaction threads with violations to correctly display in the LHN.

Some bug fixes that were discovered in the creation of this:

originalMessage needed to be added to the mapped version of reportActions in SidebarLinksData so that we correctly identify threads with violations
adding policy, policyTags, and policyCategories to the splitBill function to correctly create transactionViolations onyx collections for those transactions

Issue 34548 is also resolved within this PR by adding a check to ReportUtils for the status of the report and if it is reimbursed, we don't want to display a transaction thread with violations.

Tests

Verify that no errors appear in the JS console

Pre-testing Instructions

Verify you have a workspace with violations enabled and that have required Tags and Categories and that you have an account with that workspace.
Verify that you have the canUseViolations beta enabled

Testing Money Request

Select the Floating Action Button (FAB) within the Left Hand Navigation (LHN) and then select Request Money
Select Manual as the entry method and enter an amount.
Select the Next button.
Select your workspace from the possible options.
Verify you have been taken to the Money Request Confirmation Screen.
Select the Merchant row and enter a merchant name, then hit the Save button. (It can be anything and this is required to continue)
Select the Request button
Verify that the Money Request you just created appears in the LHN with an RBR indicator.
Select and verify the information.

Testing Distance Requests

Select the Floating Action Button (FAB) within the Left Hand Navigation (LHN) and then select Request Money
Select the Distance tab
Select the Start row and in the input screen type 100 5th Ave New York NY and select the address
Select the End row and in the input screen type 400 5th Ave New York NY and select the address
Select the Next button
Select your Workspace from the available options
Verify that the amount 1.41 miles and that the date is correct.
Select the Request button
Verify that the transaction thread correctly appears in the LHN with the correct information and an RBR indicator.

Splits are included in the functionality of this PR and so transaction threads are created optimistically however because violations aren't created for splits, they will not appear in the LHN. To test that they are created correctly, you can log the optimisticTransactionThread and optimisticCreatedActionForTransactionThread within the createSplitsAndOnyxData in the IOU.js file. Then within the OnyxDB in the Storage browser tab you can verify that those reports and report actions have been created.

Offline tests

Same as Tests

QA Steps

Same as Tests

PR Author Checklist

Screenshots/Videos

Android: Native

Android_480.mov

Android: mWeb Chrome

AndroidChrome_480.mov

iOS: Native

iOS_480.mov

iOS: mWeb Safari

iOS_Safari.mov

MacOS: Chrome / Safari

Web_480.mov

MacOS: Desktop

Desktop_480.mov

…hown

…ards/lhn-optimistic-ids

…tionThread

…litBill functions

cdanwards · 2024-01-31T15:25:06Z

src/libs/ReportUtils.ts

+    const reportIsSettled = report.statusNum === CONST.REPORT.STATUS_NUM.REIMBURSED;
+
+    // Always show IOU reports with violations unless they are reimbursed
+    if (isExpenseRequest(report) && doesReportHaveViolations && !reportIsSettled) {


Moving this check up to this location was discussed here: https://expensify.slack.com/archives/C01GTK53T8Q/p1706026331938069

…itsAndOnyxData function

…ards/lhn-optimistic-ids

…tions/cdanwards/lhn-optimistic-ids

…ve policyTags & categories

…tions/cdanwards/lhn-optimistic-ids

cdanwards · 2024-02-02T16:53:08Z

@cead22 this is ready for review!

melvin-bot · 2024-02-02T16:53:25Z

@grgia 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]

cead22 · 2024-02-06T01:26:31Z

Thanks! There are conflicts on this branch, once they're resolved I'll bring a C+ to review

cdanwards · 2024-02-06T01:54:18Z

@cead22 saw the conflicts at end of day today, will be fixing tomorrow morning!

cead22 · 2024-02-06T02:01:18Z

Sounds good, thank you!

…ards/lhn-optimistic-ids

situchan · 2024-02-20T13:14:06Z

#33114 is not fixed yet. Bug still happening:

Screen.Recording.2024-02-20.at.7.12.37.PM.mov

situchan · 2024-02-20T13:17:32Z

@cdanwards please at least test and upload one platform video which involves all test cases

cead22 · 2024-02-23T00:24:29Z

#33114 is not fixed yet. Bug still happening:

@situchan I believe this diff fixes it, can you give it a try?

diff --git a/src/libs/actions/IOU.ts b/src/libs/actions/IOU.ts
index cda09f99c5..1e3ad06c46 100644
--- a/src/libs/actions/IOU.ts
+++ b/src/libs/actions/IOU.ts
@@ -883,7 +883,7 @@ function getMoneyRequestInformation(
     const currentTime = DateUtils.getDBTime();
     const optimisticCreatedActionForChat = ReportUtils.buildOptimisticCreatedReportAction(payeeEmail);
     const optimisticCreatedActionForIOU = ReportUtils.buildOptimisticCreatedReportAction(payeeEmail, DateUtils.subtractMillisecondsFromDateTime(currentTime, 1));
-    const iouAction = ReportUtils.buildOptimisticIOUReportAction(
+    let iouAction = ReportUtils.buildOptimisticIOUReportAction(
         CONST.IOU.REPORT_ACTION_TYPE.CREATE,
         amount,
         currency,
@@ -899,6 +899,7 @@ function getMoneyRequestInformation(
         currentTime,
     );
     const optimisticTransactionThread = ReportUtils.buildTransactionThread(iouAction, iouReport.reportID);
+    iouAction.childReportID = optimisticTransactionThread.reportID;
     const optimisticCreatedActionForTransactionThread = ReportUtils.buildOptimisticCreatedReportAction(payeeEmail);
 
     let reportPreviewAction = shouldCreateNewMoneyRequestReport ? null : ReportActionsUtils.getReportPreviewAction(chatReport.reportID, iouReport.reportID);

Also, I'm thinking we merge this PR as is even if it doesn't fix #33114, so long as it passes the other tests, and we can unlink that issue from this PR and work on the fix for that issue separately, since we can't update this PR anymore.

Alternatively, I can create a new PR with all the commits from this one, and the fix above -- assuming it works (the thing is that I'm having issue running the app on mweb on android).

What do you think?

situchan · 2024-02-23T00:34:11Z

Above change works! Need to tackle more to make TS check happy

situchan · 2024-02-23T00:34:33Z

Also, I'm thinking we merge this PR as is even if it doesn't fix #33114, so long as it passes the other tests, and we can unlink that issue from this PR and work on the fix for that issue separately, since we can't update this PR anymore.

👍

cead22 · 2024-02-23T00:53:36Z

Cool, I updated the description to remove the link and the tests for 33114. Can you complete the reviewer checklist when you get a chance.

If everything works fine for you (other than 33114), we can merge this PR, and I'll update #33114 with a better problem description, the diff above, and find a contributor to work on it

situchan · 2024-02-23T11:13:33Z

Just to remind again from https://expensify.slack.com/archives/C01GTK53T8Q/p1705432850640919?thread_ts=1705336943.293449&cid=C01GTK53T8Q

For a money request that has a violation:

The money request view show should violations for both payee and payer
The money request preview show should violations for both
The report preview should show a RBR for both
The LHN only for the payee should show the transaction thread with RBR -- clicking this chat takes you to the money request view

Am I missing something? #34548 isn't fixed yet.

RBR still shows in LHN for already paid request on payee side

situchan · 2024-02-23T11:23:00Z

@cead22 RBR doesn't show at all on payer side. As this PR isn't supposed to handle them and didn't touch this logic, I consider them out of scope.

(payer: dark mode, payee: light mode)

ReportPreview

MoneyRequestPreview

MoneyRequestView

situchan · 2024-02-23T11:55:33Z

Looks like there's bug in backend. After creating distance request without category, default category is auto-selected.
Reproducible on both this branch and main so not frontend bug.

Screen.Recording.2024-02-23.at.5.50.55.PM.mov

situchan · 2024-02-23T12:03:23Z

Checklist complete.
Reported a few concerns/bugs above (#35449 (comment) #35449 (comment) #35449 (comment)). Otherwise looks good.

cead22 · 2024-02-23T17:19:43Z

Thanks for the thorough review

Am I missing something? #34548 isn't fixed yet.

I don't think you're missing anything, and I see some code meant to handle this, but let's merge and fix this in a separate PR

@cead22 RBR doesn't show at all on payer side. As this PR isn't supposed to handle them and didn't touch this logic, I consider them out of scope.

Agreed

Looks like there's bug in backend. After creating distance request without category, default category is auto-selected.
Reproducible on both this branch and main so not frontend bug.

Agreed. This may be due to implicit categorization, which is a feature we have where we set the category automatically for a request with merchant X, if you've created requests before with merchant X and have added a category to it. You should be able to reproduce this by creating a manual request with a specific merchant, and add a category to it, and then create a second manual request with the same merchant

situchan · 2024-02-23T17:24:44Z

@cead22 mind assigning me (for C+ role) in follow-up issues since I have context?

OSBotify · 2024-02-23T17:25:17Z

✋ 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 · 2024-02-26T21:05:29Z

🚀 Deployed to staging by https://github.com/cead22 in version: 1.4.44-0 🚀

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

kavimuru · 2024-02-26T21:25:58Z

@cead22 Should the QA team test?

Splits are included in the functionality of this PR and so transaction threads are created optimistically however because violations aren't created for splits, they will not appear in the LHN. To test that they are created correctly, you can log the optimisticTransactionThread and optimisticCreatedActionForTransactionThread within the createSplitsAndOnyxData in the IOU.js file. Then within the OnyxDB in the Storage browser tab you can verify that those reports and report actions have been created.

kavimuru · 2024-02-27T14:30:49Z

@cdanwards or @situchan could you please help with the above? Seems we can't test.

Splits are included in the functionality of this PR and so transaction threads are created optimistically however because violations aren't created for splits, they will not appear in the LHN. To test that they are created correctly, you can log the optimisticTransactionThread and optimisticCreatedActionForTransactionThread within the createSplitsAndOnyxData in the IOU.js file. Then within the OnyxDB in the Storage browser tab you can verify that those reports and report actions have been created.

situchan · 2024-02-27T14:33:00Z

@cdanwards or @situchan could you please help with the above? Seems we can't test.

Splits are included in the functionality of this PR and so transaction threads are created optimistically however because violations aren't created for splits, they will not appear in the LHN. To test that they are created correctly, you can log the optimisticTransactionThread and optimisticCreatedActionForTransactionThread within the createSplitsAndOnyxData in the IOU.js file. Then within the OnyxDB in the Storage browser tab you can verify that those reports and report actions have been created.

yes, it's dev only tests. You can skip that.

OSBotify · 2024-02-28T23:54:05Z

🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.44-13 🚀

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

danieldoglas · 2024-03-05T18:12:02Z

@cead22 can you confirm if #37296 is an intended result of these changes?

cead22 · 2024-03-05T22:39:06Z

Can you be more specific? transaction threads that have violations should show RBR on the LHN for the person that made the request, and only for them

cdanwards added 6 commits January 29, 2024 15:11

Add originalMessage to reportAction in reportActionsSelector for LHN

7315ae8

Add optimistic transaction thread logic for iou functions

33da1f4

Update LHN logic to allow Transaction Threads with Violations to be s…

d1325b3

…hown

Merge branch 'main' of github.com:Expensify/App into violations/cdanw…

076b27a

…ards/lhn-optimistic-ids

Add optimisticTransactionThread and optimisticCreatedActionForTransac…

8779158

…tionThread

Add policyTags and policyCategories to createSplitsAndOnyxData and sp…

6d07a21

…litBill functions

cdanwards commented Jan 31, 2024

View reviewed changes

cdanwards and others added 9 commits January 31, 2024 11:11

Fix undefined values for policyTags and policyCategories in createSpl…

46c3870

…itsAndOnyxData function

Merge branch 'main' of github.com:Expensify/App into violations/cdanw…

0ffaf19

…ards/lhn-optimistic-ids

Remove isNewIOUReport from getMoneyRequestInformation function

d61a4ed

Remove unused parameters and update function signature

e683b8f

Update IOUTest to not fail with transaction threads

0bf7365

Merge branch 'main' of github.com:infinitered/ExpensifyApp into viola…

4319da4

…tions/cdanwards/lhn-optimistic-ids

Remove unneeded jest timeout

11a63e4

feat(Violations): reorder arguments when building onyx data, and remo…

c19372a

…ve policyTags & categories

Merge branch 'main' of github.com:infinitered/ExpensifyApp into viola…

74beda7

…tions/cdanwards/lhn-optimistic-ids

cdanwards marked this pull request as ready for review February 2, 2024 16:52

cdanwards requested a review from a team as a code owner February 2, 2024 16:52

melvin-bot bot requested review from grgia and removed request for a team February 2, 2024 16:53

cead22 requested review from cead22 and removed request for grgia February 5, 2024 21:56

cdanwards added 3 commits February 6, 2024 09:43

Merge branch 'main' of github.com:Expensify/App into violations/cdanw…

dd05a86

…ards/lhn-optimistic-ids

Update optimistic transaction thread logic for typscript conversion

da57979

Refactor buildTransactionThread function calls in IOU.ts

f06d5b0

trjExpensify mentioned this pull request Feb 19, 2024

[Hold #31411] [$500] Scan - None not displayed in merchant while offline & after adding amount & going online #32552

Closed

6 tasks

This comment was marked as off-topic.

Sign in to view

situchan approved these changes Feb 23, 2024

View reviewed changes

cead22 approved these changes Feb 23, 2024

View reviewed changes

cead22 merged commit 690174f into Expensify:main Feb 23, 2024
19 checks passed

cead22 mentioned this pull request Feb 23, 2024

Send optimistic transactionThreadReportID and createdReportActionIDForThread with API commands RequestMoney, CreateDistanceRequest, CompleteSplitBill, SendMoney #31411

Closed

github-actions bot mentioned this pull request Feb 26, 2024

Deploy Checklist: New Expensify 2024-02-26 #37224

Closed

izarutskaya mentioned this pull request Feb 28, 2024

Expense - Transaction thread with violation does not appear in LHN #37405

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds optimistic data for transaction threads with violations #35449

Adds optimistic data for transaction threads with violations #35449

cdanwards commented Jan 31, 2024 •

edited by cead22

Loading

cdanwards Jan 31, 2024

cdanwards commented Feb 2, 2024

melvin-bot bot commented Feb 2, 2024

cead22 commented Feb 6, 2024

cdanwards commented Feb 6, 2024

cead22 commented Feb 6, 2024

This comment was marked as off-topic.

situchan commented Feb 20, 2024

situchan commented Feb 20, 2024

cead22 commented Feb 23, 2024

situchan commented Feb 23, 2024

situchan commented Feb 23, 2024

cead22 commented Feb 23, 2024

situchan commented Feb 23, 2024 •

edited

Loading

situchan commented Feb 23, 2024 •

edited

Loading

situchan commented Feb 23, 2024

situchan commented Feb 23, 2024 •

edited

Loading

cead22 commented Feb 23, 2024

situchan commented Feb 23, 2024 •

edited

Loading

OSBotify commented Feb 23, 2024

OSBotify commented Feb 26, 2024

kavimuru commented Feb 26, 2024

kavimuru commented Feb 27, 2024

situchan commented Feb 27, 2024

OSBotify commented Feb 28, 2024

danieldoglas commented Mar 5, 2024

cead22 commented Mar 5, 2024

Adds optimistic data for transaction threads with violations #35449

Adds optimistic data for transaction threads with violations #35449

Conversation

cdanwards commented Jan 31, 2024 • edited by cead22 Loading

Details

Fixed Issues

Tests

Pre-testing Instructions

Testing Money Request

Testing Distance Requests

Offline tests

Same as Tests

QA Steps

Same as Tests

PR Author Checklist

Screenshots/Videos

cdanwards Jan 31, 2024

Choose a reason for hiding this comment

cdanwards commented Feb 2, 2024

melvin-bot bot commented Feb 2, 2024

cead22 commented Feb 6, 2024

cdanwards commented Feb 6, 2024

cead22 commented Feb 6, 2024

This comment was marked as off-topic.

situchan commented Feb 20, 2024

situchan commented Feb 20, 2024

cead22 commented Feb 23, 2024

situchan commented Feb 23, 2024

situchan commented Feb 23, 2024

cead22 commented Feb 23, 2024

situchan commented Feb 23, 2024 • edited Loading

situchan commented Feb 23, 2024 • edited Loading

situchan commented Feb 23, 2024

situchan commented Feb 23, 2024 • edited Loading

cead22 commented Feb 23, 2024

situchan commented Feb 23, 2024 • edited Loading

OSBotify commented Feb 23, 2024

OSBotify commented Feb 26, 2024

kavimuru commented Feb 26, 2024

kavimuru commented Feb 27, 2024

situchan commented Feb 27, 2024

OSBotify commented Feb 28, 2024

danieldoglas commented Mar 5, 2024

cead22 commented Mar 5, 2024

cdanwards commented Jan 31, 2024 •

edited by cead22

Loading

situchan commented Feb 23, 2024 •

edited

Loading

situchan commented Feb 23, 2024 •

edited

Loading

situchan commented Feb 23, 2024 •

edited

Loading

situchan commented Feb 23, 2024 •

edited

Loading