[$250] Track expense - System message appears on wrong LHN report after changing merchant or desc

[lanitochka17]

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.77-3
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:

1. Navigate to staging.new.expensify.com
2. Open personal report
3. Create a tracked expense
4. Update either the description or the merchant of the tracked expense

Expected Result:

System message indicating merchant or description being updated gets displayed on LHN for the combined report of the tracked expense only

Actual Result:

System message indicating merchant or description being updated is shown on the LHN of the personal space report

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6495857_1717054028787.bandicam_2024-05-30_10-20-30-005.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e89fac46add8bf9d
• Upwork Job ID: 1797717388975722496
• Last Price Increase: 2024-06-03
• Automatic offers:
• c3024 | Contributor | 102627885

Issue Owner

Current Issue Owner: @allroundexperts

[melvin-bot]

Triggered auto assignment to @johncschuster (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.

[lanitochka17]

We think that this bug might be related to #vip-vsp

[lanitochka17]

@johncschuster 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

[c3024]

Proposal

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

System message (like changing of merchant) appears as subtitle of self DM in LHN. It should only appear as last message in the transaction thread and not in the selfDM in LHN.

What is the root cause of that problem?

This bug happens only when there is only one transaction in the self DM report.
Here in OptionsListUtils

App/src/libs/OptionsListUtils.ts

Lines 291 to 299 in 0cb2dc0

	// If the report is a one-transaction report and has , we need to return the combined reportActions so that the LHN can display modifications
	// to the transaction thread or the report itself
	const transactionThreadReportID = ReportActionUtils.getOneTransactionThreadReportID(reportID, actions[reportActions[0]], true);
	if (transactionThreadReportID) {
	const transactionThreadReportActionsArray = Object.values(actions[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${transactionThreadReportID}`] ?? {});
	sortedReportActions = ReportActionUtils.getCombinedReportActions(reportActionsArray, transactionThreadReportActionsArray, reportID);
	}
	lastReportActions[reportID] = sortedReportActions[0];

we merge the transaction thread report actions into IOU report actions if it is oneTransactionThreadReport so the last transaction of modified merchant is added into the report actions and set as the lastReportAction. In the present case of self DM, the lastMessage of modifying merchant appears on the LHN.

But this merging of transaction thread report actions for single transaction reports is not applicable for self DM because unlike other reports the tracked expenses are shown as separate report actions here instead of a single report preview merging all IOUs as shown in other reports.

Additionally, we don't see this report action of modifying merchant in the self DM report screen though the transaction report actions are added here as well

App/src/pages/home/report/ReportActionsView.tsx

Line 160 in 0cb2dc0

() => ReportActionsUtils.getCombinedReportActions(allReportActions, transactionThreadReportActions),

with transactionThreadReportActions obtained from onyx with the transactionThreadReportID passed from ReportScreen

App/src/pages/home/ReportScreen.tsx

Lines 331 to 334 in 0cb2dc0

	const transactionThreadReportID = useMemo(
	() => ReportActionsUtils.getOneTransactionThreadReportID(report.reportID, reportActions ?? [], false, isOffline),
	[report.reportID, reportActions, isOffline],
	);

This is because there is a check here when getting the getOneTransactionThreadReportID

App/src/libs/ReportActionsUtils.ts

Lines 878 to 887 in 0cb2dc0

	skipReportTypeCheck: boolean | undefined = undefined,
	isOffline: boolean | undefined = undefined,
	): string | null {
	if (!skipReportTypeCheck) {
	// If the report is not an IOU, Expense report, or Invoice, it shouldn't be treated as one-transaction report.
	const report = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];
	if (report?.type !== CONST.REPORT.TYPE.IOU && report?.type !== CONST.REPORT.TYPE.EXPENSE && report?.type !== CONST.REPORT.TYPE.INVOICE) {
	return null;
	}
	}

and it returns null when the skipReportTypeCheck is passed as false as seen here for track expenses because it is not one of the actions being checked there

App/src/pages/home/ReportScreen.tsx

Line 332 in 0cb2dc0

() => ReportActionsUtils.getOneTransactionThreadReportID(report.reportID, reportActions ?? [], false, isOffline),

whereas it is passed as true here

App/src/libs/OptionsListUtils.ts

Line 293 in 0cb2dc0

const transactionThreadReportID = ReportActionUtils.getOneTransactionThreadReportID(reportID, actions[reportActions[0]], true);

which skips the check. If the check is not skipped it returns null and LHN shows correct message.

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

We can check if the report is self DM and pass the boolean here

App/src/libs/OptionsListUtils.ts

Line 293 in 0cb2dc0

const transactionThreadReportID = ReportActionUtils.getOneTransactionThreadReportID(reportID, actions[reportActions[0]], true);

like

const isSelfDM = ReportUtils.isSelfDM(ReportUtils.getReport(reportID));

            // If the report is a one-transaction report and has , we need to return the combined reportActions so that the LHN can display modifications
            // to the transaction thread or the report itself
            const transactionThreadReportID = ReportActionUtils.getOneTransactionThreadReportID(reportID, actions[reportActions[0]], !isSelfDM);
            

What alternative solutions did you explore? (Optional)

I see this skip of check added here in #39472 for showing correct messages in LHN for edge cases. I changed the skipcheck to false in OptionsListUtils directly (irrespective of whether it is a self DM or not) and the tests succeeded fine. So, we might consider changing this

App/src/libs/OptionsListUtils.ts

Line 293 in 0cb2dc0

const transactionThreadReportID = ReportActionUtils.getOneTransactionThreadReportID(reportID, actions[reportActions[0]], true);

simply to

const transactionThreadReportID = ReportActionUtils.getOneTransactionThreadReportID(reportID, actions[reportActions[0]], false);

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01e89fac46add8bf9d

[melvin-bot]

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

[allroundexperts]

Reviewing today.

[allroundexperts]

@c3024's proposal looks good to me. I would suggest to go with the main proposal since its less likely to cause a regression.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[NikkiWines]

I see this skip of check added here in #39472 for showing correct messages in LHN for edge cases. I changed the skipcheck to false in OptionsListUtils directly (irrespective of whether it is a self DM or not) and the tests succeeded fine. So, we might consider changing this

I just double-checked this, and the behavior without the skipReportType check is fine for me as well. Even though I agree with @allroundexperts that it's more likely to cause regressions, I think we should use the alternative proposal, but go one step further and remove the skipReportTypeCheck altogether since it's apparently unnecessary now.

@allroundexperts @c3024 how does that sound to you?

[c3024]

Yes, let's remove it. I don't think anything will be broken.

[melvin-bot]

📣 @c3024 🎉 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 📖

[c3024]

@allroundexperts PR ready for review!

[c3024]

@johncschuster This was deployed to production 4 days ago but automation failed here.

[c3024]

@johncschuster Can you please complete the payment here? This was deployed to production 10 days ago. Thanks!

[mallenexpensify]

Contributor: @c3024 paid $250 via Upwork
Contributor+: @allroundexperts due $250 via NewDot

@allroundexperts can you fill out the BZ checklist plzzzz

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@] The PR that introduced the bug has been identified. Link to the PR:
• [@] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@] Determine if we should create a regression test for this bug.
• [@] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@bz] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:
• [ ]

[melvin-bot]

@johncschuster @NikkiWines @allroundexperts @c3024 this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[allroundexperts]

On to the checklist today.

[allroundexperts]

Checklist

1. Add One Transaction Report View #36934
2. The PR author (@NikkiWines) is aware of the bug already.
3. N/A
4. A regression test would be helpful.

Regression test

1. Login to any account and open the chat with yourself (personal report)
2. Create a tracked expense
3. Update either the description or the merchant of the tracked expense

Verify that the system message indicating merchant or description being updated gets displayed on LHN for the combined report of the tracked expense only.

Do we 👍 or 👎 ?

[mallenexpensify]

Thanks @allroundexperts

• https://github.com/Expensify/Expensify/issues/409882

[JmillsExpensify]

$250 approved for @allroundexperts