Adds optimistic data for transaction threads with violations #35449
Conversation
…ards/lhn-optimistic-ids
…litBill functions
| const reportIsSettled = report.statusNum === CONST.REPORT.STATUS_NUM.REIMBURSED; | ||
|
|
||
| // Always show IOU reports with violations unless they are reimbursed | ||
| if (isExpenseRequest(report) && doesReportHaveViolations && !reportIsSettled) { |
There was a problem hiding this comment.
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
|
@cead22 this is ready for review! |
|
Thanks! There are conflicts on this branch, once they're resolved I'll bring a C+ to review |
|
@cead22 saw the conflicts at end of day today, will be fixing tomorrow morning! |
|
Sounds good, thank you! |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
#33114 is not fixed yet. Bug still happening: Screen.Recording.2024-02-20.at.7.12.37.PM.mov |
|
@cdanwards please at least test and upload one platform video which involves all test cases |
@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? |
|
Above change works! Need to tackle more to make TS check happy 
|
👍 |
|
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 |
|
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:
Am I missing something? #34548 isn't fixed yet. RBR still shows in LHN for already paid request on payee side 
|
|
@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)
|
|
Looks like there's bug in backend. After creating distance request without category, default category is auto-selected. Screen.Recording.2024-02-23.at.5.50.55.PM.mov |
|
Checklist complete. |
|
Thanks for the thorough review
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
Agreed
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 |
|
@cead22 mind assigning me (for C+ role) in follow-up issues since I have context? |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/cead22 in version: 1.4.44-0 🚀
|
|
@cead22 Should the QA team test?
|
|
@cdanwards or @situchan could you please help with the above? Seems we can't test.
|
yes, it's dev only tests. You can skip that. |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.44-13 🚀
|
|
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 |
Details
Fixed Issues
$ #31411
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, andSplitBilland 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:
originalMessageneeded to be added to the mapped version ofreportActionsinSidebarLinksDataso that we correctly identify threads with violationspolicy,policyTags, andpolicyCategoriesto thesplitBillfunction to correctly createtransactionViolationsonyx collections for those transactionsIssue 34548 is also resolved within this PR by adding a check to
ReportUtilsfor the status of the report and if it is reimbursed, we don't want to display a transaction thread with violations.Tests
Pre-testing Instructions
canUseViolationsbeta enabledTesting Money Request
Manualas the entry method and enter an amount.Nextbutton.Merchantrow and enter a merchant name, then hit theSavebutton. (It can be anything and this is required to continue)RequestbuttonTesting Distance Requests
100 5th Ave New York NYand select the address400 5th Ave New York NYand select the addressSplits 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
optimisticTransactionThreadandoptimisticCreatedActionForTransactionThreadwithin thecreateSplitsAndOnyxDatain theIOU.jsfile. 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
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.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