Open lanitochka17 opened 2 months ago
Triggered auto assignment to @dylanexpensify (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
@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
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
When we create a send invoice, we don't pass the ID of iouAction
as a parameter in the SendInvoice
API. It leads after the API is complete, the iou report has two IOU
action
And then getOriginalReportID
returns the wrong reportID
for the system message because we get all reportActions of the iou report in Onyx which makes getOneTransactionThreadReportID
function returns undefined
.
It doesn't happen in the ReportScreen
because when we get the report actions here, it doesn't include the optimistic iou action
We should pass the reportActionID
of iouAction
as a parameter of the SendInvoice
API and BE will use this reportActionID
to create the iouAction
and return data for the user.
To do that we should
In getSendInvoiceInformation
function, we will return iouReportActionID: iouAction.reportID
here
https://github.com/Expensify/App/blob/1e0831eabdadc9380b7bc1d8a194ae05102c9736/src/libs/actions/IOU.ts#L1844
Get the iouReportActionID
when we call getSendInvoiceInformation
function here
https://github.com/Expensify/App/blob/1e0831eabdadc9380b7bc1d8a194ae05102c9736/src/libs/actions/IOU.ts#L3542
and then pass it as a parameter of SendInvoice
API here
https://github.com/Expensify/App/blob/1e0831eabdadc9380b7bc1d8a194ae05102c9736/src/libs/actions/IOU.ts#L3545
iouReportActionID
param to generate the the iouAction.OPTIONAL: The created action of the transaction thread report is also duplicated. The RCA is the same with the iouAction
, we can add the same fix for this action as well.
Send invoice - System message for invoice edit is missing "Reply in thread" for invoice sender and some visual issues related to transaction details.
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:
When report actions in view/list only have 1 IOU report actions it will display transaction details (this is already correct). If it is more than one it will displayed as REPORTPREVIEW for each IOU report actions.
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"). So basically the report actions of report which doesn't have a "Reply in thread" consist of report actions from that report and child transaction thread report.
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 Reply in Thread
. This is because:
The getOriginalReportID
fails to get the correct child (original) report id / the transaction thread report which is the report that show the system message about money request update / the report which is the report actions are merged into parent if parent only has 1 IOU report action.
This is because:
getOneTransactionThreadReportID
:
Fails to returns correct childreportid / trasaction thread report id, because early return in that if clause. This is because there are two IOU report actions (one from optimistic update and one from backend, these two report actions have different reportActionID).
Because of incorrect originalReportID
, the reportActions here:
the reportAction here will be undefined, and ContextMenuActions.filter
will filter Reply In Thread
context menu because the context menu give false result in contextAction.shouldShow
.
We could remove onyx optimistic IOU report actions because server returns new/different IOU report actions, in here:
inside successData in buildOnyxDataForInvoice
,
We set the
[transactionThreadCreatedReportAction.reportActionID]: null
....
[iouCreatedAction.reportActionID]: null,
[iouAction.reportActionID]: null,
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.
Use getContinuousReportActionChain
inside getOneTrasactionReportID
to filter out the optmistic IOU report action.
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 (External
)
added to project
@truph01, @tsa321 Thank you, everyone, for the proposals. Both proposals do not have a clear RCA. Can you please clearly explain in detail:
@rayane-djouah
Why is the "Reply in thread" option disabled for invoice sender?
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 optimistic IOU is filtered by this function, so it is not displayed in report actions list:
When report actions in view/list only have 1 IOU report actions it will display transaction details (this is already correct). If it is more than one it will displayed as REPORTPREVIEW for each IOU report actions.
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"). So basically the report actions of report which doesn't have a "Reply in thread" consist of report actions from that report and child transaction thread report.
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 Reply in Thread
. This is because:
The getOriginalReportID
fails to get the correct child (original) report id / the transaction thread report which is the report that show the system message about money request update / the report which is the report actions are merged into parent if parent only has 1 IOU report action.
This is because:
getOneTransactionThreadReportID
:
Fails to returns correct childreportid / trasaction thread report id, because early return in that if clause. This is because there are two IOU report actions (one from optimistic update and one from backend, these two report actions have different reportActionID).
Because of incorrect originalReportID
, the reportActions here:
the reportAction here will be undefined, and ContextMenuActions.filter
will filter Reply In Thread
context menu because the context menu give false result in contextAction.shouldShow
.
I have updated my proposal in RCA section.
Why is there a duplicate report action for the Why is there a duplicate report action for the invoice details??
Because after user send invoice the report action of type CREATE
and IOU
from optimistic update and from back end are preserved/stored. Both have different report action ids. The details is representation of CREATE report action, when there are two create report action, there is a glitch when the APP try to display two invoice details.
Why is the user navigated to the same invoice thread instead of the main chat?
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). basically the system message is from child transaction thread report.
@rayane-djouah
Why is the "Reply in thread" option disabled for invoice sender?
This because the originalReportID is wrong as I mentioned in my proposal and then we can't get the current reportAction in BaseReportActionContextMenu
Why is there a duplicate report action for the invoice details?
Because BE generate an iou action with a random reportActionID
instead of using the reportActionID
from front-end, after SendInvoice
API is complete we have two iou action, one from server, one is optimistic iou action.
Why is the user navigated to the same invoice thread instead of the main chat?
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 iouAction
and createdActionForIOUReport
for the invoice sender; below my investigation:
I sent an invoice
Result from :
The "Reply in thread" option is missing for the sender.
When I deleted site data in the invoice sender session and logged in again, the "Reply in thread" option was available.
I retried the scenario and sent the reply in a thread from the receiver; the invoice details appear duplicated in the sender session.
When I deleted site data in the invoice sender session and logged in again, the invoice details appeared correct.
reportActionID
, createdIOUReportActionID
, and createdReportActionIDForThread
. Therefore, similar to RequestMoneyParams
and other IOU API endpoints, we should includereportActionID
, createdIOUReportActionID
, and createdReportActionIDForThread
in SendInvoiceParams
, and BE should use this IDs to create the report actions and return data with correct IDs.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 successData
in buildOnyxDataForInvoice
could work, but I think it's not the optimal solution and does not follow the established pattern in other IOU flows.
Why is the user navigated to the same invoice thread instead of the main chat? 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.
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.
:ribbon::eyes::ribbon: 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!
@dylanexpensify ^^
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@cristipaval, @dylanexpensify, @rayane-djouah Whoops! This issue is 2 days overdue. Let's get this updated quick!
@cristipaval — How do you think we should approach this issue? Should we start by making frontend changes and then make necessary backend changes?
@cristipaval - we can assign @truph01 per https://expensify.slack.com/archives/C02NK2DQWUX/p1719615492425099?thread_ts=1718830065.116209&cid=C02NK2DQWUX
@cristipaval @dylanexpensify @rayane-djouah this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@cristipaval, @dylanexpensify, @rayane-djouah Whoops! This issue is 2 days overdue. Let's get this updated quick!
Friendly bump @cristipaval
Thanks for pushing this one, @rayane-djouah! 🙏
I think we better do the backend changes first. The thing is that this is not a high priority issue and I won't be able to work on it this week.
@cristipaval, @dylanexpensify, @rayane-djouah Huh... This is 4 days overdue. Who can take care of this?
I might be able to work on the backend changes for this one at the end of the week.
Thank you for the update
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
thanks @cristipaval!
@cristipaval, @dylanexpensify, @rayane-djouah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@cristipaval - Any update? Can we assign @truph01 to prepare the draft PR and hold it for BE changes?
@cristipaval @dylanexpensify @rayane-djouah this issue is now 4 weeks old, please consider:
Thanks!
@rayane-djouah If we want to handle the similar errors in Front End based on this message, we could remove the optimistic created report actions (and optimistic IOU report action) after API request finished.
I'm back 100% this week. I hope I'll be able to take care of the BE changes when I find some free cycles between the higher-priority issues I have on my plate.
Not overdue, Melv.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@cristipaval, @dylanexpensify, @rayane-djouah Whoops! This issue is 2 days overdue. Let's get this updated quick!
The backend PRs are in review
Both backend PRs are merged, they just need to hit staging.
📣 @rayane-djouah 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!
📣 @truph01 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑💻 Keep in mind: Code of Conduct | Contributing 📖
Assigned @truph01 in the meantime. Just keep in mind that you can fully test when the backend PRs are in staging. I'll post an update here when they are
Daily updates: We are still waiting for the backend PR to hit staging.
Daily updates: We are still waiting for the backend PR to hit staging.
They are in staging. Feel free to test and let me know if it works as expected.
@cristipaval I try to send the param named iouReportActionID
to BE when calling API SendInvoice
but there is no proper action returned by BE. Can you help check?
@truph01, are you saying that the IOU action is not added in the onyxData
of the SendInvoice
command response? I'm trying to understand what you mean by
but there is no proper action returned by BE
are you saying that the IOU action is not added in the onyxData of the SendInvoice command response? I'm trying to understand what you mean by
Yes
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com 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
https://github.com/Expensify/App/assets/78819774/650bc6bb-7e6a-401d-91ae-12d5bfbb759c
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @dylanexpensify