-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[$250] Send invoice - System message for invoice edit is missing "Reply in thread" for invoice sender #43797
Comments
Triggered auto assignment to @dylanexpensify ( |
@dylanexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors |
ProposalPlease re-state the problem that we are trying to solve in this issue.In Step 7, "Reply in thread" is missing for invoice sender but present for invoice receiver What is the root cause of that problem?When we create a send invoice, we don't pass the ID of Line 3545 in 1e0831e
And then Line 6063 in 1e0831e
It doesn't happen in the App/src/pages/home/ReportScreen.tsx Line 261 in 1e0831e
What changes do you think we should make in order to solve the problem?We should pass the Line 3545 in 1e0831e
To do that we should
What alternative solutions did you explore? (Optional)
and then pass it as a parameter of Line 3545 in 1e0831e
OPTIONAL: The created action of the transaction thread report is also duplicated. The RCA is the same with the |
ProposalPlease re-state the problem that we are trying to solve in this issue.Send invoice - System message for invoice edit is missing "Reply in thread" for invoice sender and some visual issues related to transaction details. What is the root cause of that problem?When user send an invoice to the server returns new IOU report action which has different report action id than optimistic report action. Basically because there are two IOU report action after user send invoice (one from optimistic action, one from server, this two IOU have different report action ids). The report view/list success to display correct report actions, this is because the optimistic IOU report action is filtered by this function, so it is not displayed in report actions list: App/src/pages/home/ReportScreen.tsx Line 261 in f618a94
When report actions in view/list only have 1 IOU report actions it will display transaction details (this is already correct). Also, when only 1 IOU report actions is displayed, the child transactions details thread reportactions will be merged into parent report report actions(which is the report which don't has "Reply in thread"). Because of the getContinuousReportActionChain which is success to filter the optimistic IOU report action, but when user right click to show report acitons context menu item it fails to display App/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx Lines 205 to 207 in b3f688b
The This is because: Lines 6066 to 6070 in f618a94
App/src/libs/ReportActionsUtils.ts Lines 920 to 922 in f618a94
Fails to returns correct childreportid / trasaction thread report id, because early return in that if clause. Because of incorrect App/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx Lines 131 to 144 in f618a94
the reportAction here will be undefined, and What changes do you think we should make in order to solve the problem?We could remove onyx optimistic IOU report actions because server returns new/different IOU report actions, in here: Lines 1035 to 1041 in b3f688b
inside successData in We set the
We set it to null, to remove the optimistic iou report actions... For the navigation and header issue, we should use the parent of parent header and back to report link, if the parent of parent report is one trasaction / only has 1 IOU report action. alternative solution:Use |
reproing today! |
Job added to Upwork: https://www.upwork.com/jobs/~0111ac375ebfe2baa1 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @rayane-djouah ( |
added to project |
@truph01, @tsa321
|
Basically because there are two IOU report action after user send invoice (one from optimistic action, one from server, this two IOU have different report action ids). App/src/pages/home/ReportScreen.tsx Line 261 in f618a94
When report actions in view/list only have 1 IOU report actions it will display transaction details (this is already correct). Also, when only 1 IOU report actions is displayed, the child transactions details thread reportactions will be merged into parent report report actions(which is the report which don't has "Reply in thread"). The getContinuousReportActionChain success to filter out the optimistic IOU report action, but when user right click to show report actions context menu item it fails to display App/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx Lines 205 to 207 in b3f688b
The This is because: Lines 6066 to 6070 in f618a94
App/src/libs/ReportActionsUtils.ts Lines 920 to 922 in f618a94
Fails to returns correct childreportid / trasaction thread report id, because early return in that if clause. Because of incorrect App/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx Lines 131 to 144 in f618a94
the reportAction here will be undefined, and I have updated my proposal in RCA section.
Because after user send invoice the report action of type
because the system message of IOU request upate/edit are from child thread transaction report. The system message is displayed/merged into parent report because the parent has exactly one IOU report action (because report screen success filtering the optimistic IOU report action). |
This because the originalReportID is wrong as I mentioned in my proposal and then we can't get the current reportAction in
Because BE generate an iou action with a random
This is not the same chat. The first chat is the transaction thread report that is the current parent report of the system message thread. The second is the invoice room but this report has only one transaction and then this report display as combine report that will display all report actions of invoice room and all report actions of the transaction thread. So this case isn't a bug. |
@truph01 - I think you included the "What alternative solutions did you explore? (Optional)" title in the middle of your solution by mistake? |
@tsa321, @truph01 Thank you for your explanations. I confirmed that we are having duplicate
Line 1828 in 896c14c
Both proposals have a correct RCA, but @truph01 was the first to identify the root cause and provide the correct solution based on what we established as a pattern in IOU flows like Track Expense and Request Money... @tsa321 - Removing optimistic IOU report actions inside
I will let the assigned internal engineer decide whether we should fix the header link so we can navigate directly to the invoice room when we are in one transaction report instead of navigating to the iou report and then to the invoice report. I think we can address this in the PR. @truph01's proposal looks good to me. 🎀👀🎀 C+ reviewed |
Triggered auto assignment to @cristipaval, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@cristipaval to review and confirm |
I think @truph01 can work on new issues once they merge all the ongoing ones. @dylanexpensify, can you confirm? 🙏 |
@cristipaval, @dylanexpensify, @rayane-djouah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Waiting for the PR to get deployed. |
@cristipaval - Reminder that the old reports with 2 CREATED report actions still need to be cleaned up. Are you tracking this internally? |
Bump @cristipaval 🙇♂️ |
I don't think it makes sense to spend time on cleaning that as we don't have active users using the invoice rooms yet. @madmax330, do you think we should run some custom queries to clean the DB, or should we deal with it if it becomes a real problem? |
Given the context above, this issue should be on [HOLD for Payment 2024-09-6] according to today's production deploy from Deploy Checklist: New Expensify 2024-08-28. |
@cristipaval are they real users or test users? |
@madmax330 - It looks like @cristipaval is on parental leave, could you please create an internal issue for this? Thanks! |
@madmax330, @cristipaval, @dylanexpensify, @rayane-djouah, @truph01 Eep! 4 days overdue now. Issues have feelings too... |
@madmax330 shall we get a new engineer on this? Or are you on it? |
@dylanexpensify actually I think we can just close this out since it was fixed already and the 2 report actions don't cause much of a problem |
@dylanexpensify This is due payment - #43797 (comment) Thanks! |
Cool is there a payment summary already? |
@madmax330, @cristipaval, @dylanexpensify, @rayane-djouah, @truph01 Huh... This is 4 days overdue. Who can take care of this? |
Payment summary: Contributor: @truph01 $250 Please apply/request! |
Here is the pull request that resolves this issue: Here are the issues fixed by the PR:
Checklist
Regression Test Proposal Test case 1:
Test case 2:
Test case 3:
Test case 4:
Do we agree 👍 or 👎 |
@madmax330, @cristipaval, @dylanexpensify, @rayane-djouah, @truph01 Whoops! This issue is 2 days overdue. Let's get this updated quick! |
@dylanexpensify Friendly bump |
@madmax330, @cristipaval, @dylanexpensify, @rayane-djouah, @truph01 6 days overdue. This is scarier than being forced to listen to Vogon poetry! |
on it now. |
done! |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 1.4.84-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team
Action Performed:
Expected Result:
In Step 7, "Reply in thread" should be available for both invoice sender and receiver
In Step 10, the invoice thread should have no visual issue
In Step 11, user should be navigated to the main chat
Actual Result:
In Step 7, "Reply in thread" is missing for invoice sender but present for invoice receiver
In Step 10, the invoice details appear broken above existing invoice details
In Step 11, user is navigated to the same invoice thread instead of the main chat
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6514451_1718472036865.send_invoice_reply_in_thread.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @dylanexpensifyThe text was updated successfully, but these errors were encountered: