[$500] Expense - Expense preview shows incorrect amount briefly when WS default currency is different

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.1.5
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:

Action Performed:

1. Navigate to staging.new.expensify.com.
2. Go to Settings > Workspaces > any workspace.
3. Click Settings > Default currency.
4. Select a different currency.
5. Go to workspace chat > + > Request money.
6. Enter amount (MYR1234) while remaining the local currency > Next.
7. Click Request in confirmation page.
   Note that the amount in the confirmation shows the incorrect amount ($1234) briefly before the conversion.

Expected Result:

The amount in the expense preview will show the correct amount (MYR1234) briefly before the conversion.

Actual Result:

The amount in the expense preview shows the incorrect amount ($1234) briefly before the conversion.

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

Bug6285572_1700575757806.20231121_203317.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~015cbb737be93fbffb
• Upwork Job ID: 1726990587939540992
• Last Price Increase: 2023-11-21
• Automatic offers:
• c3024 | Reviewer | 28042435
• esh-g | Contributor | 28042437

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~015cbb737be93fbffb

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[FitseTLT]

Proposal

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

Expense preview shows incorrect amount briefly when WS default currency is different

What is the root cause of that problem?

The cause of the problem is our optimistic data. We are incrementing the raw value in the default currency to the iouReport.total here:

App/src/libs/actions/IOU.js

Lines 467 to 468 in a722235

	iouReport.total -= amount;
	} else {

and then that iouReport.total value is getting displayed in the report preview with the workspace currency until the real data (value converted to the workspace currency) comes from BE.

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

If it is possible to convert currencies in FE we should set the optimistic amount after converting it to the workspace currency
Or else we should stop displaying optimistic data and wait for the BE in the cases where there is a mismatch between workspace currency and default currency to avoid displaying the wrong amount. 👍

What alternative solutions did you explore? (Optional)

[ZhenjaHorbach]

Proposal

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

Expense - Expense preview shows incorrect amount briefly when WS default currency is different

What is the root cause of that problem?

The main problem is how we get currency
Since we use ONYXKEYS.IOU to get the currency, it always returns the default value

App/src/pages/iou/steps/NewRequestAmountPage.js

Line 188 in 9da929f

iou: {key: ONYXKEYS.IOU},

App/src/pages/iou/steps/NewRequestAmountPage.js

Line 70 in 9da929f

const currency = CurrencyUtils.isValidCurrencyCode(currentCurrency) ? currentCurrency : iou.currency;

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

Since in the workspace, we use an ONYXKEYS.COLLECTION.POLICY when saving currency
We can use ONYXKEYS.COLLECTION.POLICY as a higher priority value for currency

we can add

            policies: {
                key: ONYXKEYS.COLLECTION.POLICY,
            },

App/src/pages/iou/steps/NewRequestAmountPage.js

Lines 187 to 194 in 9da929f

	export default withOnyx({
	iou: {key: ONYXKEYS.IOU},
	report: {
	key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${lodashGet(route, 'params.reportID', '')}`,
	},
	selectedTab: {
	key: `${ONYXKEYS.COLLECTION.SELECTED_TAB}${CONST.TAB.RECEIPT_TAB_ID}`,
	},

And then add

    const itemPolicy = policy[`${ONYXKEYS.COLLECTION.POLICY}${report.policyID}`] || {};

    const onyxCurrency = itemPolicy.currency || iou.currency;

    const currency = CurrencyUtils.isValidCurrencyCode(currentCurrency) ? currentCurrency : onyxCurrency;

App/src/pages/iou/steps/NewRequestAmountPage.js

Line 70 in 9da929f

const currency = CurrencyUtils.isValidCurrencyCode(currentCurrency) ? currentCurrency : iou.currency;

What alternative solutions did you explore? (Optional)

As alternative, we can update the currency value from ONYXKEYS.IOU during choosing the currency in workspace

[esh-g]

Proposal

Please re-state the problem we are trying to solve?

Expense - Expense preview shows incorrect amount briefly when WS default currency is different

What is the root cause?

Two cases:

1. If there is a iouReport already created, we don't consider the currency may be different and just subtract here:
   

   App/src/libs/actions/IOU.js

   Lines 463 to 467 in 6507621

   	if (isPolicyExpenseChat) {
   	iouReport = {...iouReport};
   	// Because of the Expense reports are stored as negative values, we substract the total from the amount
   	iouReport.total -= amount;

2. When creating a new iouReport, we create it in the workspace currency but don't change the amount here:
   

   App/src/libs/ReportUtils.js

   Lines 2489 to 2496 in 6507621

   	function buildOptimisticExpenseReport(chatReportID, policyID, payeeAccountID, total, currency) {
   	// 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);
   	// The expense report is always created with the policy's output currency
   	const outputCurrency = lodashGet(allPolicies, [`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, 'outputCurrency'], CONST.CURRENCY.USD);

What changes should be made to fix this?

Fix for Case 1
We should add a condition to check if the currency is same and subtract only if it is like this:

if (policy.outputCurrency === currency) {
    iouReport.total -= amount;
}

Note: policy can be passed as a parameter from the money request page like this: MoneyRequestConfirmPage(props.policy) -> IOU.requestMoney -> getMoneyRequestInformation

Also, this is what we do for distance requests as well. We don't update iouReport optimistically for distance request, we rely on the backend. Moreover, it wouldn't make sense to update the iouReport optimistically because we don't have the converted amount on front-end anyways.

Fix for Case 2
We should stop using outputCurrent in reportUtils.buildOptimisticExpenseReport() and just use the original currency the request was in because we don't have the ability to change the amount to the desired currency on the front-end, so we can rely on the backend to update that

Additional improvement (Mentioned in this slack convo)

We should also darken the report preview when we send a request offline to make sure the user knows that there is a pending request.
We can do this by adding a pending action to the iouReport here:

App/src/libs/actions/IOU.js

Lines 145 to 150 in 2feaac8

	onyxMethod: isNewIOUReport ? Onyx.METHOD.SET : Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport.reportID}`,
	value: {
	...iouReport,
	lastMessageText: iouAction.message[0].text,
	lastMessageHtml: iouAction.message[0].html,

pendingFields: {
    request: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
}

And this pending action can be cleared here:

App/src/libs/actions/IOU.js

Lines 214 to 217 in 2feaac8

	...(isNewIOUReport
	? [
	{
	onyxMethod: Onyx.METHOD.MERGE,

So, we just need to remove the condition ...(isNewIOUReport and just clear the pending and error fields everytime.

Now, we can use this pendingField in the ReportPreview component by adding:

<OfflineWithFeedback pendingAction={props.iouReport?.pendingFields?.request}>

Now if we also want the header to be greyed out, we can possibly add a new pending action here:

App/src/libs/ReportUtils.ts

Line 3929 in 2feaac8

function getReportOfflinePendingActionAndErrors(report: OnyxEntry<Report>): ReportOfflinePendingActionAndErrors {

like this:

const iouUpdatePendingAction = report?.pendingFields?.request;

and then use this in the ReportScreen.js separately or just club it with the other pending actions as one.

[Tony-MK]

Proposal

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

Expense preview shows incorrect amount briefly when WS default currency is different

What is the root cause of that problem?

App/src/components/ReportActionItem/ReportPreview.js

Line 230 in 85f372c

<Text style={styles.textHeadline}>{getDisplayAmount()}</Text>

App/src/components/ReportActionItem/ReportPreview.js

Lines 148 to 153 in 85f372c

	const getDisplayAmount = () => {
	if (hasPendingWaypoints) {
	return props.translate('common.tbd');
	}
	if (totalDisplaySpend) {
	return CurrencyUtils.convertToDisplayString(totalDisplaySpend, props.iouReport.currency);

The root cause of the problem comes from the frontend not having any way to convert different currencies, and there are no checks to determine and handle whether the currency of the money request's transaction and iouReport are different. The backend, because the backend creates the iouReportand dictates its currency. As for now, this is why we can't change the iouReport's currency after creation link to fix. Also, it's the only place where we can compute the iouReport's total amount in terms of default workspace currency. Due to this reason, the frontend temporarily sets the requested amount in the ReportPreview component using the default workspace currency, even if both currencies are different.

App/src/components/ReportActionItem/ReportPreview.js

Line 153 in 85f372c

return CurrencyUtils.convertToDisplayString(totalDisplaySpend, props.iouReport.currency);

This is the line where props.iouReportID.currency, which is set by the policy, is used to produce a string with the workspace's right currency but the wrong amount value, totalDisplaySpend from the function ReportUtils.getMoneyRequestSpendBreakdown.

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

Since we do not have the correct amount from the server, every possible figure we can show will eventually change. Unless the currencies are the same, therefore, the most appropriate solution in this situation is to display TBD. Then, the correct IOU amount will be displayed once the server pushes it. The reason why I believe this is the most appropriate solution is because, not only will it improve the user's offline experience, we already have implemented this method with distance requests. When a user's distance request changes waypoints and before the server returns the distance request's amount, if not manually set, and distance, the ReportPreview component will display TBD. To gain a further understanding of why TBD is used for distance requests, here is a recent merged pull request and closed issue that might help.

Hence, we create a very strict condition that is only true in this specific case, so we don't create regressions. The condition below is capable of determining whether the amount to be displayed on the ReportPreview is incorrect and is going to be updated soon.

const isWaitingOnAmount = isPolicyExpenseChat && lodashGet(props.iouReport, 'pendingFields.createChat') === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD && props.iouReport !== TransactionUtils.getAllReportTransactions(props.iouReportID)[0].currency

App/src/components/ReportActionItem/ReportPreview.js

Lines 150 to 152 in 85f372c

	return props.translate('common.tbd');
	}
	if (totalDisplaySpend) {

Using isWaitingOnAmount to change the condition above to look something similar to the one below should show TBD for incorrect amount values.

if (hasPendingWaypoints || isWaitingOnAmount) {
    return props.translate('common.tbd');
}

Result Video:

macOS.-.Chrome.mov

What alternative solutions did you explore? (Optional)

This solution is like the previous one, but we use the amount in the very first money request currency, TransactionUtils.getAllReportTransactions(props.iouReportID)[0] instead of the default WS currency.

App/src/components/ReportActionItem/ReportPreview.js

Line 153 in 85f372c

return CurrencyUtils.convertToDisplayString(totalDisplaySpend, props.iouReport.currency);

We will use the same condition as before and change the line above to look something similar to the one below. This will return a formatted string with the amount using the very first money request's currency.

return CurrencyUtils.convertToDisplayString(
   totalDisplaySpend, 
   isWaitingOnAmount ? TransactionUtils.getAllReportTransactions(props.iouReportID)[0].currency : props.iouReport.currency
);

Result Video:

macOS.-.Chrome.Alternative.mov

[DylanDylann]

We decided to do nothing here because we can't convert currency from the FE side
cc @danieldoglas Tag you here because we have this context in the previous issue

[c3024]

@DylanDylann Could you share the link to that conversation?

[esh-g]

I think the current behaviour should be changed because if the user has already requested for example $12 and another request of ₹10 is made, it shows for a moment that the request is $22 which is obviously wrong and should not be done

[DylanDylann]

@c3024 Please check this discussion #26309 (comment)

[c3024]

Thanks @DylanDylann

The existing optimistic update behaviour for the report preview in the workspace chat is weird.

But in the linked discussion you clearly laid out these cases and @danieldoglas and @dylanexpensify decided to do nothing here.

So let's do nothing here.

cc @NicMendonca

[melvin-bot]

@NicMendonca, @c3024 Eep! 4 days overdue now. Issues have feelings too...

[esh-g]

bump @c3024 on the updated proposal

[c3024]

@esh-g Thanks for the thorough proposal with detailed changes.

Proposal from @esh-g looks good and works well.

🎀 👀 🎀
C+ Reviewed

[melvin-bot]

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

[DylanDylann]

@esh-g Could you check this thread, too? Your proposal will fix it, right?

[DylanDylann]

A bit curious, in offline, we only update the amount if the currency is the same. Do you guys think it will make confusion for users because of inconsistency

[esh-g]

@DylanDylann That is why it was decided to grey out the report preview to indicate a pending action. Moreover, we also do the same for offline distance requests. The preview doesn't get updated until you go online for distance requests as well.

[melvin-bot]

@nkuoch @NicMendonca @c3024 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!

[c3024]

Awaiting assignment

[melvin-bot]

@nkuoch, @NicMendonca, @c3024 Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

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

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

[esh-g]

PR is here: #33171

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

This issue has not been updated in over 15 days. @nkuoch, @NicMendonca, @esh-g, @c3024 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!

[c3024]

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:

• [@c3024] The PR that introduced the bug has been identified. Link to the PR: No specific PR caused this. The case of optimistic update for requesting money with a currency different from existing iou report currency was missed.
• [@c3024] 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: NA
• [@c3024] 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: This is a difficult-to-catch case and a discussion is not beneficial.
• [@c3024] Determine if we should create a regression test for this bug. Yes
• [@c3024] 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.

[c3024]

Regression test proposal

Test 1

1. Create a new workspace.
2. Set the workspace default currency to USD
3. Go to the expense chat of the workspace
4. Go offline
5. Make a request in a currency that is not USD
6. Verify that the preview shows the amount and currency used in step 5
7. Go online
8. Verify that the amount and currency in preview are updated to USD

Test 2

1. Create a new workspace
2. Set the workspace default currency to USD
3. Go to the expense chat of the workspace
4. Make a request of any amount in any currency from the 'Request Money' option in the composer
5. Wait till the preview gets updated to amount and currency to USD
6. Go offline
7. Make another request in a currency that is different from USD
8. Verify that the preview is greyed out and the total amount is not updated
9. Go online
10. Verify that the preview is not greyed out and the total amount gets updated

👍 or 👎

[c3024]

@NicMendonca

Automation failed to update the title here and labels for the issue are not updated. PR #33171 referred in this issue was deployed to production more than a week ago.

PS: There are no deploy blocker caused by this issue including this.

Thanks

[c3024]

@NicMendonca Automation failed here and Melvin relegated this to Monthly. It'll take forever to come back into view.

Can you please get this paid?

[NicMendonca]

both have been paid!