[HOLD for payment 2024-03-14] [$500] [MEDIUM] Expense Report - In an offline created expense report, a negative request amount is displayed in the Header and Title

[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.39-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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Issue found when executing PR #35913

Action Performed:

Preconditions:
Set up an OldDot administrator account, invite the employee to the policy https://sites.google.com/applausemail.com/applause-expensifyproject/wiki-guides/newdot-categories?authuser=0

1. Open https://staging.new.expensify.com/
2. Log in with the account of the employee added to the policy
3. Navigate to the group policy chat room
4. Turn off the internet
5. Create a manual request and send it to the WS room
6. Go to Report Conversation

Expected Result:

IOU created offline should not display a negative amount in the header and title

Actual Result:

In an offline created IOU, a negative request amount is displayed in the Header and Title

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

Bug6373401_1707479630694.Recording__1319.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013de0165fd934bba3
• Upwork Job ID: 1755994535213568000
• Last Price Increase: 2024-02-09
• Automatic offers:
• paultsimura | Contributor | 0

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~013de0165fd934bba3

[melvin-bot]

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

[melvin-bot]

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

[lanitochka17]

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

[paultsimura]

Proposal

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

Optimistically created Expense report contains a negative value in the title.

What is the root cause of that problem?

When setting the optimistic reportName of the newly created Expense report, we use the incorrect value for the formatted total:

App/src/libs/ReportUtils.ts

Lines 2820 to 2824 in ad3d78c

	// The amount for Expense reports are stored as negative value in the database
	const storedTotal = total * -1;
	const policyName = getPolicyName(allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${chatReportID}`]);
	const formattedTotal = CurrencyUtils.convertToDisplayString(storedTotal, currency);
	const policy = getPolicy(policyID);

The storedTotal is a negated numeric value that should be stored in the DB: we store the Expense reports' total as a negative value. However, this shouldn't affect the user-visible content – report title.

Note: this happens only with the enabled report fields beta. With it, we show the report field value in the title. Otherwise, we calculate the title dynamically and negate this amount correctly.

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

When coming online, the RequestMoney API response changes this report title to the Expense Report #{reportID}:

We should change the optimistic expense report creation to set the reportName in a similar way so that the optimistic data is aligned with the future Server response:

reportName: `Expense Report ${reportID}`,

App/src/libs/ReportUtils.ts

Line 2841 in ad3d78c

reportName: `${policyName} owes ${formattedTotal}`,

What alternative solutions did you explore? (Optional)

If we don't want to change the report title format, but keep the "${policy} owes $amount" format for the optimistic expense reports, we should simply use the total instead of storedTotal when building the formattedTotal, which is used only in the report title:

const formattedTotal = CurrencyUtils.convertToDisplayString(total, currency);

[mallenexpensify]

Checking to see if this fits with #wave6-collect-submitters cuz of "Create a manual request and send it to the WS room"

[mananjadhav]

@mallenexpensify Should this be further reviewed?

[mallenexpensify]

Yes please @mananjadhav , thanks for asking. I've added this to the wave 6 project.

[mananjadhav]

@mallenexpensify @nkuoch The proposal by @paultsimura looks promising but I want to confirm if we want to update the report title to match what comes from the backend or just fix the total?

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[nkuoch]

Hmm I'm not 100% sure, so checking with @marcaaron ?

[marcaaron]

Haven't worked much on this feature, but happy to lend a hand...

this happens only with the enabled report fields beta

@paultsimura are you saying that if we are not on the beta the bug does not exist? I didn't quite follow how this relates to "report fields".

Otherwise, we calculate the title dynamically and negate this amount correctly.

Where does this happen? In the backend? And you are suggesting we need to also do it in the frontend?

The storedTotal is a negated numeric value that should be stored in the DB: we store the Expense reports' total as a negative value. However, this shouldn't affect the user-visible content – report title.

This explanation generally makes sense to me. Gonna pass this to @thienlnam though since I believe they have worked on the report fields stuff.

[marcaaron]

I want to confirm if we want to update the report title to match what comes from the backend or just fix the total?

@mananjadhav I did not understand this question. What does "fix the total" mean to you?

[thienlnam]

Is this still happening? We merged a PR recently that disables the report title for anything other than expense / paid collect policies. So this should no longer show up for IOU reports

[paultsimura]

Let me try to answer to all of the above 🙂

@thienlnam – it's still happening. I'm sure the QA team just mixed the Expense/IOU terminology a bit. Here's the recording from the latest main, it's exactly Expense-related bug:

Screen.Recording.2024-02-15.at.10.27.44-compressed.mp4

[paultsimura]

are you saying that if we are not on the beta the bug does not exist? I didn't quite follow how this relates to "report fields".

Yes, without beta it doesn't happen because, without this beta, we use the report title that's calculated on the fly (on the client). For the Expense report, the title is calculated based on the report's total.
However, when the Beta is enabled, the reportFields.title takes priority and is shown instead of being calculated on the fly – this makes sense as the report title can be changed to a custom one, and we will want to show it instead of trying to re-calculate it.
Moreover, as I explained later on, with the new approach, we no longer use the "{policy} owes {amount}" Expense report title, but instead the "Expense Report #{reportID}".
So IMO, the issue here is just that we've updated the BE part to set the Expense report's name to "Expense Report #{reportID}", but did not update the client to generate the same report name optimistically. And that's what I suggest we should fix as the optimistic data should match the BE response.

cc: @marcaaron

[thienlnam]

Ah I see, thanks for the clarification.

cc @trjExpensify I think we've talked about this, but should the Expense report title be overwritten on an expense report with report fields like this?

[trjExpensify]

Sorry, not sure I'm following the question?

[paultsimura]

Thanks. The PR is ready for review: #37160

[melvin-bot]

@mananjadhav, @paultsimura, @mallenexpensify, @thienlnam Whoops! This issue is 2 days overdue. Let's get this updated quick!

[mallenexpensify]

@mananjadhav commented here
I invited him to the beta, he should be able to review soon (was out sick for a few days)

[melvin-bot]

@mananjadhav @paultsimura @mallenexpensify @thienlnam this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.48-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix optimistic report amount in title #37617

If no regressions arise, payment will be issued on 2024-03-14. 🎊

For reference, here are some details about the assignees on this issue:

• @mananjadhav requires payment through NewDot Manual Requests
• @paultsimura requires payment automatic offer (Contributor)

[melvin-bot]

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:

• [@mananjadhav / @paultsimura] The PR that introduced the bug has been identified. Link to the PR:
• [@mananjadhav / @paultsimura] 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:
• [@mananjadhav / @paultsimura] 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:
• [@mananjadhav / @paultsimura] Determine if we should create a regression test for this bug.
• [@mananjadhav / @paultsimura] 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.
• [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[mallenexpensify]

@mananjadhav plz fill out the BZ checklist from above, thx

[mananjadhav]

While we added the storedTotal logic in this commit, I wouldn't count this as an offending PR as I am not sure when the ReportFields was enabled.

Nevertheless, I think it. makes sense to add a regression test for this one. I think the Test steps from the PR are good. I am not sure how do we mention the preconditions here @mallenexpensify.

@mallenexpensify Can you please add the payout summary so that I can raise the request?

[mallenexpensify]

Contributor: @paultsimura paid $500 via Upwork
Contributor+: @mananjadhav due $500 via NewDot.

Thanks @mananjadhav , I just copy/pasted the preconditions from the PR, along with the steps.

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

[JmillsExpensify]

$500 approved for @mananjadhav based on summary above.