Skip to content
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

[$1000] Splitting bill shows deleted message for a brief moment of time #19487

Closed
6 tasks done
kavimuru opened this issue May 23, 2023 · 16 comments
Closed
6 tasks done

[$1000] Splitting bill shows deleted message for a brief moment of time #19487

kavimuru opened this issue May 23, 2023 · 16 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@kavimuru
Copy link

kavimuru commented May 23, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Sign into staging New Expensify
  2. Select + icon > click Split Bill > create an amount > Choose person > Next > click Split button
  3. A 1:1 chat should open with the Split Bill Details
  4. Click the Split Amount Box in the chat (money report box)
  5. Notice [Deleted Message] at the top of the page for a moment and then the amount box shows again

Expected Result:

Splitting bill should not show the deleted message for a brief moment of time

Actual Result:

Deleted message appears for some period of time and then it changes to the message

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.17.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:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

delete.mp4
Recording.738.mp4

Expensify/Expensify Issue URL:
Issue reported by: @avi-shek-jha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1684592253606429

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~011ef39d4d0e55c94e
  • Upwork Job ID: 1661459081945231360
  • Last Price Increase: 2023-05-24
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 23, 2023

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot
Copy link

melvin-bot bot commented May 23, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@Christinadobrzyn
Copy link
Contributor

I'm able to reproduce it, I don't think it's related to an existing issue. I think this can be external?

@Christinadobrzyn Christinadobrzyn added the External Added to denote the issue can be worked on by a contributor label May 24, 2023
@melvin-bot melvin-bot bot changed the title Splitting bill shows deleted message for a brief moment of time [$1000] Splitting bill shows deleted message for a brief moment of time May 24, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 24, 2023

Job added to Upwork: https://www.upwork.com/jobs/~011ef39d4d0e55c94e

@melvin-bot
Copy link

melvin-bot bot commented May 24, 2023

Current assignee @Christinadobrzyn is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented May 24, 2023

Triggered auto assignment to Contributor-plus team member for initial proposal review - @s77rt (External)

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 24, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 24, 2023

Triggered auto assignment to @jasperhuangg (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@brianlee1025
Copy link

brianlee1025 commented May 25, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

[Deleted message] appears for few seconds before turning into the correct report name.

What is the root cause of that problem?

This is due to the getReportName function is always returning Localize.translateLocal('parentReportAction.deletedMessage'); as shown below.

App/src/libs/ReportUtils.js

Lines 1115 to 1122 in 0f33e2c

if (isThread(report)) {
const parentReportAction = ReportActionsUtils.getParentReportAction(report);
if (ReportActionsUtils.isTransactionThread(parentReportAction)) {
return getTransactionReportName(parentReportAction);
}
const parentReportActionMessage = lodashGet(parentReportAction, ['message', 0, 'text'], '').replace(/(\r\n|\n|\r)/gm, ' ');
return parentReportActionMessage || Localize.translateLocal('parentReportAction.deletedMessage');
}

The root cause of this issue is because the app has already navigated to the report page (as shown below) before the report has created completely and returned from the back-end service. The insufficient child report data is therefore causing parentReportAction = ReportActionsUtils.getParentReportAction(report); to fail to get the correct report name.

Report.openReport(thread.reportID, thread.participants, thread, props.action.reportActionID);
Navigation.navigate(ROUTES.getReportRoute(thread.reportID));

What changes do you think we should make in order to solve the problem?

To add an additional condition to make sure if the child report data is created and returned from the back-end service.

return parentReportActionMessage ? parentReportActionMessage : !ReportActionsUtils.isSplitMoneyReportAction(parentReportAction) ? '' : Localize.translateLocal('parentReportAction.deletedMessage');

The isSplitMoneyReportAction function would be something like this to determine if the given report action is split money report action by checking for presence of originalMessage object and 'split' type.:
function isSplitMoneyReportAction(reportAction) { return ( reportAction && !_.isEmpty(reportAction) && reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && _.has(reportAction, 'originalMessage') && lodashGet(reportAction, 'originalMessage.type') === CONST.IOU.REPORT_ACTION_TYPE.SPLIT ); }

Lastly, after implementing this checking, the child report name will become ' ' when the creation of the child report has yet to complete.
P/s: The ' ' can be changed to any desired value.

What alternative solutions did you explore? (Optional)

We could also add some checking to make sure the openReport api function has finished calling before navigating to the child report screen if we want.

Report.openReport(thread.reportID, thread.participants, thread, props.action.reportActionID);
Navigation.navigate(ROUTES.getReportRoute(thread.reportID));

@tienifr
Copy link
Contributor

tienifr commented May 25, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

[Deleted message] is shown in a new split bill thread

What is the root cause of that problem?

  1. When we create a new thread for split bill action, we call onIOUPreviewPressed function. But we pass props.requestReportID as parentReportID this would be empty in split bill

props.requestReportID,
);
Report.openReport(thread.reportID, thread.participants, thread, props.action.reportActionID);

const iouReportID = originalMessage.IOUReportID ? originalMessage.IOUReportID.toString() : '0';
children = (
<MoneyRequestAction
chatReportID={props.report.reportID}
requestReportID={iouReportID}

  1. We update the displayName in

report.displayName = ReportUtils.getReportName(report);

  1. Because the parentReportID is empty so we can't find the parentReportAction, that why we return the displayName as deletedMessage ('[Deleted Message]')

App/src/libs/ReportUtils.js

Lines 1116 to 1121 in 0f33e2c

const parentReportAction = ReportActionsUtils.getParentReportAction(report);
if (ReportActionsUtils.isTransactionThread(parentReportAction)) {
return getTransactionReportName(parentReportAction);
}
const parentReportActionMessage = lodashGet(parentReportAction, ['message', 0, 'text'], '').replace(/(\r\n|\n|\r)/gm, ' ');
return parentReportActionMessage || Localize.translateLocal('parentReportAction.deletedMessage');

What changes do you think we should make in order to solve the problem?

update this line

to

 props.chatReportID 

as what BE returns

Result

Screen.Recording.2023-05-25.at.23.34.24.mov

@s77rt
Copy link
Contributor

s77rt commented May 25, 2023

@brianlee1025 Thanks for the proposal. I don't think your RCA is correct. The App is based on an offline first design and we don't need to wait for the server response (you can reproduce the bug when offline - this should not happen). Apparently the root cause is that the optimistic data that we create is incorrect.

@s77rt
Copy link
Contributor

s77rt commented May 25, 2023

@tienifr Thanks for the proposal. Your RCA makes sense. The suggested fix works but we actually used to pass props.chatReportID. It was changed on purpose by 6a35364. I have asked for a clarification here #18604 (comment).

Maybe we can define parentReportID as chatReportID if SPLIT, otherwise requestReportID.

@mountiny
Copy link
Contributor

hmm I think we have to look more into this. The iou action should be the parent report action and hence the money report where the iou action resides is the parent report.

If you do a Split with just one person, I am not sure how exactly that should be handled, shouldnt we just make one request of the half price? @Julesssss @luacmartins ?

@tienifr
Copy link
Contributor

tienifr commented May 26, 2023

@s77rt @mountiny I have one suggestion: IOUs of type split only exist in group DMs when the participants.length > 1, so I want to add this condition to decide we should delete IOUReportID or not

App/src/libs/ReportUtils.js

Lines 1450 to 1454 in 0925c25

// IOUs of type split only exist in group DMs and those don't have an iouReport so we need to delete the IOUReportID key
if (type === CONST.IOU.REPORT_ACTION_TYPE.SPLIT) {
delete originalMessage.IOUReportID;
originalMessage.participants = [currentUserEmail, ..._.pluck(participants, 'login')];
}

and add IOUReportID as eighth arg (groupChatReport.reportID) in

const groupIOUReportAction = ReportUtils.buildOptimisticIOUReportAction(CONST.IOU.REPORT_ACTION_TYPE.SPLIT, amount, currency, comment, participants, groupTransaction.transactionID);

Result:

Screen.Recording.2023-05-26.at.12.27.03.mov

Please tell me what're your thoughts. Thanks.

@s77rt
Copy link
Contributor

s77rt commented May 26, 2023

I'm not sure how to proceed here but based on @luacmartins statement looks like we may do nothing. Clicking the split preview box is planned to open a details modal (RHN) and not a new report. Ongoing PR: #19390.

@luacmartins
Copy link
Contributor

Yea this behavior shouldn't exist. We don't create a thread when we press the split preview component. I think we can close this issue.

@mountiny
Copy link
Contributor

I agree, lets close this up, sorry for the confusion this is still in development

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

8 participants