-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Set childReportID of the optimistic IOU report action #37232
fix: Set childReportID of the optimistic IOU report action #37232
Conversation
I think we can release without it, but there's a BE fix pending: #33114 (comment) |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / Safariexpense-distance.movexpense-distance-fab.movexpense-manual.movexpense-manual-fab.movexpense-manual-scan.movexpense-manual-scan-fab.movexpense-scan.movexpense-scan-fab.moviou-manual.moviou-manual-fab.moviou-manual-scan.moviou-scan.moviou-scan-fab.movsend-fab.movsplit-user-manual.movsplit-user-manual-fab.movsplit-workspace-distance.movsplit-workspace-manual.movsplit-workspace-manual-fab.movMacOS: Desktop |
@paultsimura this is blocker. Bug not fixed: split manual request with a single user (non-Expensify) from FAB split-user-manual-fab.mov |
This one is BE as well, similar to the @cead22 when creating a Split request and calling the Here's the {
"reportID": "1932415687405820",
"amount": 2200,
"splits": "[{\"email\":\"paultsimura@gmail.com\",\"accountID\":15528187,\"amount\":1100},{\"email\":\"paultsimura+qa@gmail.com\",\"accountID\":15528255,\"amount\":1100,\"iouReportID\":\"2941200419003018\",\"chatReportID\":\"1932415687405820\",\"transactionID\":\"7451263110636286127\",\"reportActionID\":\"4265162090444479673\",\"createdChatReportActionID\":\"3058872218140896575\",\"createdIOUReportActionID\":\"792942737366932173\",\"reportPreviewReportActionID\":\"7347533532549283559\",\"transactionThreadReportID\":\"5196953162708250\",\"createdReportActionIDForThread\":\"8302969548555128822\"}]",
"currency": "PLN",
"merchant": "(none)",
"created": "2024-03-03 19:54:14.852",
"comment": "",
"category": "",
"tag": "",
"billable": false,
"transactionID": "5684861019807192728",
"reportActionID": "4158501737745037324",
"policyID": "_FAKE_"
} |
While it's BE issue, I found interesting thing (possible fix in FE). double.optimistic.chat.behavior.mov |
This looks like a workaround. |
Found a fix, working on it... |
@situchan I think after this PR this code won't be needed anymore 🤔 App/src/components/ReportActionItem/MoneyRequestAction.tsx Lines 94 to 102 in 6c2abde
App/src/pages/home/report/ContextMenu/ContextMenuActions.tsx Lines 213 to 219 in c2e24e1
|
They were added in #18760, #25429.
|
Do you observe any bug with this? If so – could you please share the recording? I didn't notice anything suspicious |
I just shared test case. If you confirm that no bug found on all types of requests, you can remove them as unnecessary codes. We don't wanna be affected by any possible regressions from code cleanup. |
Now we should hold PR for BE fix of split, send cases, right? |
I'll let @cead22 decide here. This PR won't break the flow because the behavior we have with splits & send flow is already happening in Prod. But just to be sure the BE fix works well with this PR, we can hold. |
Tests well, both IOU & expense requests: Screen.Recording.2024-03-03.at.22.02.56-compressed.mp4Screen.Recording.2024-03-03.at.22.03.51-compressed.mp4 |
Fixed ✔️ |
Thanks for letting me know, I'll make an issue for us to update the back end! |
I made the issue, and we can wait for that fix to test this PR, but are you sure the backend replacing the value with a different one, and the issue isn't that we're not setting If the backend isn't returning a bad value, then things should work with the front end fix, because we'll navigate to the right report based on that front end value, right? |
Works now after removing node modules again and clearing caches |
Split scan seems not working. To reproduce, click report preview quickly after scan success split-user-scan-fab.mov |
Test Result: (🟢: pass, 🔴: failed, 🟡: pending test)
(I keep updating fields upon testing) |
@paultsimura Let's split testing. Can you please test all above cases (from FAB) for existing report (if you haven't tested)? |
@situchan I've confirmed that all manual requests work fine. |
@cead22 there is a BE issue with the Screen.Recording.2024-03-07.at.17.02.31-compressed.mp4Could we get this fixed on the BE side? Seems like so far it's the only API endpoint that's blocking us (if we want to hold for the BE change). |
Being many issues are held for this PR and already dragged a lot for past blockers, I suggest to merge without waiting BE fix. |
Tests completed Result: #37232 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All tests pass except split scan.
@cead22 all yours
@@ -91,15 +89,7 @@ function MoneyRequestAction({ | |||
return; | |||
} | |||
|
|||
// If the childReportID is not present, we need to create a new thread |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we removing this because we're always creating transaction threads in the backend, and action?.childReportID
will be set to the ID of that transaction thread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We've discussed it briefly here: #37232 (comment)
src/libs/actions/IOU.ts
Outdated
@@ -926,7 +926,8 @@ function getMoneyRequestInformation( | |||
false, | |||
currentTime, | |||
); | |||
const optimisticTransactionThread = ReportUtils.buildTransactionThread(iouAction, iouReport.reportID); | |||
const optimisticTransactionThread = ReportUtils.buildTransactionThread(iouAction, iouReport); | |||
iouAction.childReportID = optimisticTransactionThread.reportID; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a comment above this line saying why we need to do this? It's not super obvious right away, and it made me wonder if we should be setting this in const iouAction = ReportUtils.buildOptimisticIOUReportAction
, but then realized ReportUtils.buildTransactionThread
takes iouAction
which is created by ReportUtils.buildOptimisticIOUReportAction
.
I don't have a good solution for that right now -- it seems like we could generate an optimistic reportID before the calls to ReportUtils.buildOptimisticIOUReportAction
and ReportUtils.buildTransactionThread
, and pass it to both so we don't have to manually set iouAction.childReportID = optimisticTransactionThread.reportID;
, but I'm not sure if that's a good idea.
I'm curious what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, unfortunately these 2 are co-dependent: the transaction thread has the iou action's ID as the parent action ID, and vice versa. Also, passing the pre-generated report ID will cause a lot of refactoring of the current methods that use the buildOptimisticChatReport
. One possible idea here is to make a separate function that will generate the iouAction
, optimisticTransactionThread
, and the optimisticCreatedActionForTransactionThread
. This way, we'll not be setting the iouAction.childReportID
in 4 different places, but in only one function, and will be calling it from those 4 places. It's not perfect either, but will minimize code duplication.
Smth like:
function buildOptimisticIOUReportActionWithTransactionThread(...) {
const iouAction = ReportUtils.buildOptimisticIOUReportAction(
CONST.IOU.REPORT_ACTION_TYPE.CREATE,
amount,
currency,
comment,
[participant],
optimisticTransaction.transactionID,
undefined,
iouReport.reportID,
false,
false,
receiptObject,
false,
currentTime,
);
const optimisticTransactionThread = ReportUtils.buildTransactionThread(iouAction, iouReport.reportID);
iouAction.childReportID = optimisticTransactionThread.reportID;
const optimisticCreatedActionForTransactionThread = ReportUtils.buildOptimisticCreatedReportAction(payeeEmail);
return { iouAction, optimisticTransactionThread, optimisticCreatedActionForTransactionThread }
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I thought of that but wasn't convinced it would be better. But on second thought, I do think it's better and we can add a comment inside that function above where we set iouAction.childReportID
so people know not to remove it.
The alternative would be to put that comment in 4 places, and we'd have that code duplicated like you said, and nothing prevents people from forgetting to set iouAction.childReportID
I'm good to update to this 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done ✔️
@situchan I've re-tested the 4 different flows that use the 4 different functions that call this new buildOptimisticMoneyRequestEntities
function – everything works as expected (except for the known Complete Split BE issue):
- Send Money
- Request Money
- Split right away (the fixed amount split)
- Split with Complete Split (the scan split)
@cead22 while testing, I've noticed a (relatively unrelated) issue with the Screen.Recording.2024-03-10.at.19.12.42-compressed.mp4But to fix it, we need another BE fix. Should we hold for it, or can we make it a separate issue? |
yes, I noticed that as well but that should not really block this PR to be merged |
): [OptimisticCreatedReportAction, OptimisticIOUReportAction, OptimisticChatReport, OptimisticCreatedReportAction] { | ||
const iouActionCreationTime = DateUtils.getDBTime(); | ||
|
||
// The `CREATED` action must be optimistically generated before the IOU action so that it won't appear after the IOU action in the chat. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💚 great comment
@cead22 looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
🚀 Deployed to staging by https://github.com/cead22 in version: 1.4.51-0 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.51-3 🚀
|
Details
When creating a money request, set the report action's
childReportID
to the optimistically created money requestreportID
.Fixed Issues
$ #33114
PROPOSAL: #33114 (comment)
Tests
Same as QA
Offline tests
Same as QA
QA Steps
Test 1:
manual
,scan
, anddistance
requests;Test 2:
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android-compressed.mp4
Android: mWeb Chrome
chrome-compressed.mp4
iOS: Native
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-02-27.at.00.09.34-compressed.mp4
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-02-27.at.00.16.14-compressed.mp4
MacOS: Chrome / Safari
Screen.Recording.2024-02-26.at.23.41.37-compressed.mp4
MacOS: Desktop
Screen.Recording.2024-02-26.at.23.51.31-compressed.mp4