[$250] Scan - LHN preview for expense report shows 0.00 when there is only one scan expense

IuliiaHerets commented 1 month ago

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: v9.0.23-0 Reproducible in staging?: Y Reproducible in production?: Y Issue reported by: Applause Internal Team

Action Performed:

Go to staging.new.expensify.com
Go to workspace chat that has no unsettled expense (or create a new workspace).
Submit a scan expense.
After the scanning is complete, click on the expense preview in the main chat.

Expected Result:

LHN preview for the expense report will show the correct scanned amount.

Actual Result:

LHN preview for the expense report shows 0.00. This issue only happens when there is only one scan expense in the report.

Workaround:

Unknown

Platforms:

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/aa964514-eb12-434d-8933-8537968c8591

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01676bf97c74d0768c
Upwork Job ID: 1826760340462476516
Last Price Increase: 2024-08-22
Automatic offers:

DylanDylann | Reviewer | 103712163
nkdengineer | Contributor | 103712164

Issue Owner

Current Issue Owner: @DylanDylann

melvin-bot[bot] commented 1 month ago

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

IuliiaHerets commented 1 month ago

We think that this bug might be related to #wave-collect - Release 1

IuliiaHerets commented 1 month ago

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

bernhardoj commented 1 month ago

Proposal

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

LHN last message shows 0 amount in one-expense report if the expense is a scan request.

What is the root cause of that problem?

The one-expense report "lastMessageText" itself is "$0.00 expense". Looks like the last message when we do a scan request is always like that. But if we open an empty transaction thread, the last message shown in LHN is "No activity yet". I believe we want the last message for one-expense report to be the same as transaction thread, so I'll explain why the last message shown in LHN is different.

When we get the last message of a report, we take its last visible action and we create it based on the action type. https://github.com/Expensify/App/blob/50de692ff915d7998bff3fb0d024446b59912b52/src/libs/OptionsListUtils.ts#L593-L595

If the last message is still empty, we fallback to the report lastMessageText property. https://github.com/Expensify/App/blob/50de692ff915d7998bff3fb0d024446b59912b52/src/libs/OptionsListUtils.ts#L671-L677

In one-expense report, the last report action is null because the last report action is the CREATED action, but we filter it out, so, it fallback to the lastMessageText which is "$0.00 expense".

This is the same case as one-transaction thread too, but the lastMessageText is empty, so, it gets the last message from the welcome message, which is also empty for a chat thread (including transaction thread), so we finally fallback to "No activity yet". https://github.com/Expensify/App/blob/50de692ff915d7998bff3fb0d024446b59912b52/src/libs/SidebarUtils.ts#L448-L449 https://github.com/Expensify/App/blob/50de692ff915d7998bff3fb0d024446b59912b52/src/libs/SidebarUtils.ts#L488-L492

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

To show "No activity yet" for one-expense report too,

If it's a one-transaction report and the last report action is empty (null), then return empty string as the last message from OptionsListUtils.getLastMessageTextForReport.
```
if (!lastReportAction && transactionThreadReportIDs[reportID]) {
return "";
} else if (ReportUtils.isArchivedRoom(report, reportNameValuePairs)) {
```

transactionThreadReportIDs is a new variable that stores the one-transaction thread report ID for each one-expense report.

const transactionThreadReportIDs = {};

https://github.com/Expensify/App/blob/50de692ff915d7998bff3fb0d024446b59912b52/src/libs/OptionsListUtils.ts#L309-L313

transactionThreadReportIDs[reportID] = transactionThreadReportID;

Update getWelcomeMessage to return empty welcome message for one-expense report too. https://github.com/Expensify/App/blob/50de692ff915d7998bff3fb0d024446b59912b52/src/libs/SidebarUtils.ts#L488-L492

 || !!ReportActionsUtils.getOneTransactionThreadReportID(report?.reportID ?? '-1', reportActions)

We will pass the reportActions as the param of getWelcomeMessage.

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~01676bf97c74d0768c

melvin-bot[bot] commented 1 month ago

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

nkdengineer commented 1 month ago

Proposal

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

LHN preview for the expense report shows 0.00. This issue only happens when there is only one scan expense in the report.

What is the root cause of that problem?

When the expense report is a combined report, we filtered out the IOU action of expense report via getCombinedReportActions function here

https://github.com/Expensify/App/blob/156f0a2bd0e94558ad621caa701bd749dd124bfe/src/libs/OptionsListUtils.ts#L310-L313

That makes lastReportAction here null then we fallback to report.lastMessageText which is 0.00 expense in this case

https://github.com/Expensify/App/blob/156f0a2bd0e94558ad621caa701bd749dd124bfe/src/libs/OptionsListUtils.ts#L595

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

I think we should not filter out the IOU action in OptionsListUtils because we will use this action to get the correct last message text. For example, if the receipt is scanning, LHN should display Receipt scanning... that is the behavior when the expense report is not a combined report.

https://github.com/Expensify/App/blob/156f0a2bd0e94558ad621caa701bd749dd124bfe/src/libs/OptionsListUtils.ts#L310-L313

To do that we can create a new param in getCombinedReportActions like shouldFilterIOUAction with default value is true then if it's false we will return early here

if (!isMoneyRequestAction(action) || !shouldFilterIOUAction) {
    return true;
}

What alternative solutions did you explore? (Optional)

NA

melvin-bot[bot] commented 1 month ago

@twisterdotcom, @DylanDylann Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

DylanDylann commented 1 month ago

Two proposals have different output

With @bernhardoj's proposal, the subtitle of one transaction expense in LHN displays No activity yet when the receipt is being scanned and after the scan is successful
With @nkdengineer's proposal, the subtitle of one-transaction expense in LHN relies on the last reportActions, It will display Receipt scanning... if the receipt is being scanned and display the correct amount (getting from transaction) after the scanning is successful

Note that the amount from both report.lastMessagetext and reportAction.message/originalMessage is returned wrong from BE

The decision to assign this task to someone depends on our expectation. Let's wait for the final decision from the internal engineer

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 month ago

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

trjExpensify commented 1 month ago

It will display Receipt scanning... if the receipt is being scanned and display the correct amount (getting from transaction) after the scanning is successful

That seems correct to me. Receipt scanning... (during scanning), $x.xx expense (after scanning, if successful). The latter being consistent with "manual" one expense reports on prod:

francoisl commented 1 month ago

Yeah I would tend to agree too.

One question about the approach though (and maybe this can be discussed in the PR) is why do we need a new param shouldFilterIOUAction? Or asked differently, when would we use true and when would we use false? If we only set it to false for one-transaction reports, could we count how many items are in filteredParentReportActions instead? Basically, I'm thinking if we can avoid introducing a new param, it's one less chance that someone will use it wrong in the future and cause a bug.

melvin-bot[bot] commented 1 month ago

📣 @DylanDylann 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 week ago

This issue has not been updated in over 15 days. @francoisl, @twisterdotcom, @DylanDylann, @nkdengineer eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

DylanDylann commented 1 week ago

@twisterdotcom @francoisl Please help to process payment here

twisterdotcom commented 1 week ago

Payment Summary:

@DylanDylann paid $250 via Upwork here (C+)
@nkdengineer paid $250 via Upwork here (Contributor)

Expensify / App