[HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$500] [CRITICAL] [UX Reliability] App Crash with Invalid Currency code

[mountiny]

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:
Reproducible in staging?:
Reproducible in production?:
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: @joekaufmanexpensify
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1717433997016439

Action Performed:

Break down in numbered steps

1. Have a report with invalid currency code
2. You can locally merge in incorrect data to an expense report
3. Open the report

Expected Result:

Describe what you think should've happened

We will handle this issue gracefully

Actual Result:

Describe what actually happened

The app crashes

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: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01365ad3a139cd53cd
• Upwork Job ID: 1797682414465474560
• Last Price Increase: 2024-06-20
• Automatic offers:
• ahmedGaber93 | Reviewer | 102595642
• b4s36t4 | Contributor | 102595648

Issue Owner

Current Issue Owner: @CortneyOfstad

[melvin-bot]

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

[melvin-bot]

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

[mountiny]

Looking for proposals

[melvin-bot]

⚠️ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01365ad3a139cd53cd

[melvin-bot]

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

[Krishna2323]

Proposal

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

App Crash with Invalid Currency code

What is the root cause of that problem?

When the currency is not present, we don't return early.

App/src/libs/CurrencyUtils.ts

Lines 100 to 110 in a015db7

	function convertToDisplayString(amountInCents = 0, currency: string = CONST.CURRENCY.USD): string {
	const convertedAmount = convertToFrontendAmount(amountInCents);
	return NumberFormatUtils.format(BaseLocaleListener.getPreferredLocale(), convertedAmount, {
	style: 'currency',
	currency,
	// We are forcing the number of decimals because we override the default number of decimals in the backend for RSD
	// See: https://github.com/Expensify/PHP-Libs/pull/834
	minimumFractionDigits: currency === 'RSD' ? getCurrencyDecimals(currency) : undefined,
	});
	}

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

If currency is a falsy value we can return early or we can return convertedAmount as string without calling NumberFormatUtils.format.

What alternative solutions did you explore? (Optional)

We can use isValidCurrencyCode to check if we the currency is valid or not and based on that we can perform expected behaviour.

We can pass CONST.CURRENCY.USD to NumberFormatUtils.format if isValidCurrencyCode(currency) is false.

App/src/libs/CurrencyUtils.ts

Lines 146 to 152 in a015db7

	/**
	* Checks if passed currency code is a valid currency based on currency list
	*/
	function isValidCurrencyCode(currencyCode: string): boolean {
	const currency = currencyList?.[currencyCode];
	return Boolean(currency);
	}

[dragnoir]

Proposal

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

Non supported currency turns error

What is the root cause of that problem?

We are passing an empty string when iouReport?.currency is not available

App/src/libs/ReportUtils.ts

Line 6398 in bc3009a

return [CurrencyUtils.convertToDisplayString(unheldTotal, iouReport?.currency ?? ''), CurrencyUtils.convertToDisplayString((iouReport?.total ?? 0) * -1, iouReport?.currency ?? '')];

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

When the currency is not passed from the report details, the function had a default value that cover this issue.

App/src/libs/CurrencyUtils.ts

Line 100 in bc3009a

function convertToDisplayString(amountInCents = 0, currency: string = CONST.CURRENCY.USD): string {

but the issue is from the parent function

App/src/libs/ReportUtils.ts

Line 6398 in bc3009a

return [CurrencyUtils.convertToDisplayString(unheldTotal, iouReport?.currency ?? ''), CurrencyUtils.convertToDisplayString((iouReport?.total ?? 0) * -1, iouReport?.currency ?? '')];

When iouReport?.currency does not exist, it uses another value ''. Passing the empty string is causing this issue.

We should pass iouReport?.currency without the empty string or replace the empty string with USD in case iouReport?.currency is not available

- CurrencyUtils.convertToDisplayString(unheldTotal, iouReport?.currency ??  '')
+ CurrencyUtils.convertToDisplayString(unheldTotal, iouReport?.currency)

What alternative solutions did you explore?

We ned to add a logic to prevent using a non supported currency here

App/src/libs/CurrencyUtils.ts

Line 102 in a015db7

return NumberFormatUtils.format(BaseLocaleListener.getPreferredLocale(), convertedAmount, {

To handle invalid currency codes gracefully, we can use a try-catch block within your convertToDisplayString function. This will allow us to catch the RangeError and provide a default or fallback behavior. Additionally, we can validate the currency code before passing it to the NumberFormat constructor by checking against a list of supported currencies.

Here's an updated version of your function:

/**
 * Given an amount in the "cents", convert it to a string for display in the UI.
 * The backend always handle things in "cents" (subunit equal to 1/100)
 *
 * @param amountInCents – should be an integer. Anything after a decimal place will be dropped.
 * @param currency - IOU currency
 */
function convertToDisplayString(amountInCents = 0, currency: string = CONST.CURRENCY.USD): string {
    const convertedAmount = convertToFrontendAmount(amountInCents);

    try {
        return NumberFormatUtils.format(BaseLocaleListener.getPreferredLocale(), convertedAmount, {
            style: 'currency',
            currency,

            // We are forcing the number of decimals because we override the default number of decimals in the backend for RSD
            // See: https://github.com/Expensify/PHP-Libs/pull/834
            minimumFractionDigits: currency === 'RSD' ? getCurrencyDecimals(currency) : undefined,
        });
    } catch (error) {
        if (error instanceof RangeError) {
            console.error(`Invalid currency code: ${currency}`, error);
            // Provide a fallback display string or handle the error as needed
            return `${convertedAmount} (Invalid currency)`;
        } else {
            throw error; // Re-throw unexpected errors
        }
    }
}

[Krishna2323]

Proposal Updated

• Added alternative

[Krishna2323]

@mountiny, whats the expected behaviour? What should we do when invalid currency is detected?

[b4s36t4]

Proposal

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

[UX Reliability] App Crash with Invalid Currency code

What is the root cause of that problem?

Intl.NumberFormat does have an issue when we either pass empty currency code or currency code which is greater than 4 characters.

If we pass an invalid key say OOO it still works, if I pass oooo or empty valye the application breaks.

So the real issue relies on sending empty currency code which is happening here

App/src/libs/ReportUtils.ts

Lines 6362 to 6376 in 926d7af

	function getNonHeldAndFullAmount(iouReport: OnyxEntry<Report>, policy: OnyxEntry<Policy>): string[] {
	const transactions = TransactionUtils.getAllReportTransactions(iouReport?.reportID ?? '');
	const hasPendingTransaction = transactions.some((transaction) => !!transaction.pendingAction);
	if (hasUpdatedTotal(iouReport, policy) && hasPendingTransaction) {
	const unheldTotal = transactions.reduce((currentVal, transaction) => currentVal - (!TransactionUtils.isOnHold(transaction) ? transaction.amount : 0), 0);
	return [CurrencyUtils.convertToDisplayString(unheldTotal, iouReport?.currency ?? ''), CurrencyUtils.convertToDisplayString((iouReport?.total ?? 0) * -1, iouReport?.currency ?? '')];
	}
	return [
	CurrencyUtils.convertToDisplayString((iouReport?.unheldTotal ?? 0) * -1, iouReport?.currency ?? ''),
	CurrencyUtils.convertToDisplayString((iouReport?.total ?? 0) * -1, iouReport?.currency ?? ''),
	];
	}

If we observe here we're passing empty string here which can cause application break.

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

So, to solve this we have to ensure we never pass empty currency i.e empty string code to the function here

App/src/libs/ReportUtils.ts

Line 6373 in 926d7af

CurrencyUtils.convertToDisplayString((iouReport?.unheldTotal ?? 0) * -1, iouReport?.currency ?? ''),

.

we can make

App/src/libs/CurrencyUtils.ts

Line 100 in 1332a1c

function convertToDisplayString(amountInCents = 0, currency: string = CONST.CURRENCY.USD): string {

this function here to make currency optional and leave as it is without sending any empty value this way we're ensured we never send any empty currency code to the function.

And then in the function instead of directly using the passed code we can use the Intl.supportedValuesOf('currency') to get the list of browser supported currencies and if it's not found we can reveret back to USD which will solve the issue.

What alternative solutions did you explore? (Optional)

Sometimes we might get empty string (maybe from backend) so at that time we cannot do anything to face it, as proposed we can make the currency paramters optional here and instead of empty string we can pass undefined instead of empty string which will get replaced by default value proposed by the function definition.

[mountiny]

I think we should be ok to default to USD in this case as it should be very rare and its probably some malformed data

[mountiny]

Btw, I think the main problem here is that the currency is missing as a prop in the report object so its probably not correctly defaulting to USD

this is example of the report that crashes for me, no currency

[twisterdotcom]

Damn this is killing me today. Would love to get it fixed. Having to right click > Mark as read a lot

[dragnoir]

Proposal updated after this comment

[ShridharGoel]

Proposal

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

Invalid/missing currency code is not handled.

What is the root cause of that problem?

We don't have any mechanism to fallback to USD when the currency is missing or it is invalid.

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

Updated logic in NumberFormatUtils to handle this issue (it can be polished):

import type { ValueOf } from 'type-fest';
import type CONST from '@src/CONST';

function isValidCurrency(locale: string, currency: string): boolean {
    try {
        new Intl.NumberFormat(locale, { style: 'currency', currency }).format(1);
        return true;
    } catch {
        return false;
    }
}

function format(locale: ValueOf<typeof CONST.LOCALES>, number: number, options?: Intl.NumberFormatOptions): string {
    let newOptions = options
    if (options?.style === 'currency' && (!options?.currency || !isValidCurrency(locale, options.currency))) {
        newOptions = { ...options, currency: 'USD' };
    }
    return new Intl.NumberFormat(locale, newOptions).format(number);
}

function formatToParts(locale: ValueOf<typeof CONST.LOCALES>, number: number, options?: Intl.NumberFormatOptions): Intl.NumberFormatPart[] {
    let newOptions = options
    if (options?.style === 'currency' && (!options?.currency || !isValidCurrency(locale, options.currency))) {
        newOptions = { ...options, currency: 'USD' };
    }
    return new Intl.NumberFormat(locale, newOptions).formatToParts(number);
}

export { format, formatToParts };

If style is currency, then we will check for missing/invalid value.

If we don't want to have the check directly in this file, then we can move it to the calling functions.

[ahmedGaber93]

Reviewing now

[AndrewGable]

I have this problem too

[CortneyOfstad]

PR has been created and QA is on-going 👍

[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.81-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix currency breaking issue with currency conversion #43133

If no regressions arise, payment will be issued on 2024-06-18. 🎊

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

• @ahmedGaber93 requires payment automatic offer (Reviewer)
• @b4s36t4 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:

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

[melvin-bot]

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

• fix currency breaking issue with currency conversion #43133

If no regressions arise, payment will be issued on 2024-06-20. 🎊

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

• @ahmedGaber93 requires payment automatic offer (Reviewer)
• @b4s36t4 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:

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

[CortneyOfstad]

@ahmedGaber93 can you complete the checklist before EOD today so we're not delayed with payment tomorrow? Thanks!

[b4s36t4]

@CortneyOfstad doesn't the payment due date is yesterday which is the first that the resolution is deployed on?

[b4s36t4]

Also about the amount, shouldn't this come under critical issues which are eligible for 500$? I'm sorry if it's not!

[ahmedGaber93]

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:

• [@ahmedGaber93] The PR that introduced the bug has been identified. Link to the PR: N/A. the main issue is in backend.
• [@ahmedGaber93] 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: N/A
• [@ahmedGaber93] 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: N/A
• [@ahmedGaber93] Determine if we should create a regression test for this bug. Yes
• [@ahmedGaber93] 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.

Regression Test Proposal

1. Open any report
2. Click (+) icon > Submit expense then submit an expense
3. Open the expense report that you created it in step 2 and Verify the app is not crashed

extra steps for Web and Desktop only

4. Open your browser console and type Onyx.merge("report_<your_report_id>",{currency: null}) then press Enter. You can get your_report_id from browser link.
5. Verify the app is not crashed

Do we agree 👍 or 👎

[CortneyOfstad]

Payment Summary

@ahmedGaber93 — paid $500 (Critical) via Upwork
@b4s36t4 — paid $500 (Critical) via Upwork

Regression

https://github.com/Expensify/Expensify/issues/406381

[melvin-bot]

Upwork job price has been updated to $500

[mallenexpensify]

Heads up, in case anyone uses this as a reference for prices increases for 'critical' type issues. From #expensify-open-source

There have been questions about what constitutes a job being auto-increased to $500. These jobs are only the ones that have the High Priority label added to them (ie. issue with HIGH or CRITICAL in the title or project status/priority are not auto-increased to $500).