Skip to content

Fix approval limit CSV import conversion - #101422

Draft
rushatgabhane wants to merge 3 commits into
Expensify:mainfrom
rushatgabhane:682002-approval-limit-csv
Draft

rushatgabhane wants to merge 3 commits into
Expensify:mainfrom
rushatgabhane:682002-approval-limit-csv

Conversation

@rushatgabhane

@rushatgabhane rushatgabhane commented Sep 17, 2026

Copy link
Copy Markdown
Member

Explanation of Change

Updated member CSV imports so approval limits are converted from major currency units into backend minor units before being stored or sent.

For example, an approval limit of 500 is converted to 50000 cents for USD. The conversion also respects the workspace currency’s decimal places and preserves blank values.

Fixed Issues

$ https://github.com/Expensify/App/issues/682002
PROPOSAL:

Tests

  1. Opened a workspace on the web.
  2. Uploaded a members CSV with an approval limit of 500.
  3. Verified that the approval limit was converted to 50000 cents and displayed as $500.
  4. Verified that the CSV import completed successfully.
  • Verify that no errors appear in the JS console

Offline tests

No offline behavior was changed. Offline testing was not performed.

QA Steps

  1. Upload a members CSV with Approval limit = 500.
  2. Verify that the member receives a $500 approval limit instead of $5.
  3. Verify that blank approval-limit values remain blank.
  4. Verify that invalid values are rejected.
  5. Confirm that the import works correctly on the required platforms.
  • Verify that no errors appear in the JS console

Screenshots/Videos

No visual or layout changes were made. Screenshots are not applicable.

@melvin-bot

melvin-bot Bot commented Sep 17, 2026

Copy link
Copy Markdown

Hey, I noticed you changed src/languages/en.ts in a PR from a fork. For security reasons, translations are not generated automatically for PRs from forks.

If you want to automatically generate translations for other locales, an Expensify employee will have to:

  1. Look at the code and make sure there are no malicious changes.
  2. Run the Generate static translations GitHub workflow. If you have write access and the K2 extension, you can simply click: [this button]

Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running:

npx bun ./scripts/generateTranslations.ts --help

Typically, you'd want to translate only what you changed by running npx bun ./scripts/generateTranslations.ts --compare-ref main

@rushatgabhane
rushatgabhane force-pushed the 682002-approval-limit-csv branch from bc2ef2f to bcbfdc7 Compare September 17, 2026 13:28
@codecov

codecov Bot commented Sep 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.

Files with missing lines Coverage Δ
src/libs/importSpreadsheetUtils.ts 50.71% <100.00%> (+0.95%) ⬆️
...rc/pages/workspace/members/ImportedMembersPage.tsx 81.08% <42.10%> (-5.95%) ⬇️
... and 17 files with indirect coverage changes

@rushatgabhane
rushatgabhane force-pushed the 682002-approval-limit-csv branch from bcbfdc7 to 3cde44e Compare September 17, 2026 13:49
@rushatgabhane

Copy link
Copy Markdown
Member Author

@MelvinBot The current CURRENCY_SYMBOLS regex only supports $, €, £, and ¥, but Expensify supports more currencies. What is the best existing repository or external-library approach to parse and clean currency symbols for every supported currency?
Please recommend the exact helper or implementation we should use in ImportedMembersPage.tsx, while preserving currency-specific decimals, blank values, validation, and storing cents as a string.

@MelvinBot

Copy link
Copy Markdown
Contributor

Don't use a symbol whitelist and don't add a library. The authoritative list of every currency Expensify supports is already on the client: Onyx CURRENCY_LIST carries a symbol per currency (159 distinct symbols), exposed as getCurrencySymbol from useCurrencyListActions() — the same hook this file already uses for getCurrencyDecimals. Strip the workspace currency's symbol/code, with \p{Sc} as a catch-all for any other Unicode currency sign.

Separately: src/pages/workspace/members/ImportedMembersPage.tsx:220 multiplies by 10 ** decimals, which is wrong for the 33 zero-decimal currencies. See below.

Recommended implementation

Delete CONST.REGEX.CURRENCY_SYMBOLS and add this to src/libs/importSpreadsheetUtils.ts (the shared home every Imported*Page already imports):

import StringUtils from './StringUtils';
import {stripCommaFromAmount, stripSpacesFromAmount} from './MoneyRequestUtils';

/**
 * Normalize an amount cell from an imported spreadsheet. Removes the workspace currency's symbol or ISO
 * code (prefix/suffix only, where currency markers actually appear) and any Unicode currency sign, plus
 * grouping separators and whitespace. Everything else is left intact so validateAmount can reject it.
 */
function normalizeImportedAmount(value: string, currencySymbol?: string, currencyCode?: string): string {
    const tokens = [currencySymbol, currencyCode].filter(Boolean).map((token) => StringUtils.escapeRegExp(token as string));
    const withoutCurrencyCode = tokens.length > 0 ? value.trim().replace(new RegExp(`^(?:${tokens.join('|')})\\s*|\\s*(?:${tokens.join('|')})$`, 'i'), '') : value;
    return stripCommaFromAmount(stripSpacesFromAmount(withoutCurrencyCode.replaceAll(/\p{Sc}/gu, '')));
}

Then in both call sites, replace stripCommaFromAmount(stripSpacesFromAmount(...).replaceAll(CONST.REGEX.CURRENCY_SYMBOLS, '')) with:

const {getCurrencySymbol, getCurrencyDecimals} = useCurrencyListActions();
const currency = policy?.outputCurrency;
const normalized = normalizeImportedAmount(String(value), getCurrencySymbol(currency ?? ''), currency);

validateAmount(normalized, getCurrencyDecimals(currency)) stays exactly as-is — it already returns true for '', so blank values keep working, and it still enforces the currency's decimal count.

Why this over the alternatives
Approach Verdict
Onyx CURRENCY_LIST symbol + \p{Sc} (recommended) Covers all currencies. The symbol list is backend-owned data the app already loads, so it never goes stale. Anchoring the token replace to prefix/suffix avoids eating letters mid-value for single-letter symbols (D = GMD, G = HTG, B = PAB).
[^\d.-] — the existing repo precedent in src/libs/actions/CompanyCards.ts:89 and src/libs/actions/ImportTransactions.ts:167 Covers every currency but guts your validation — "12abc" silently becomes 12 and passes. Those importers have no validation gate; yours does, so don't copy it.
/\p{Sc}/gu alone (one-char change from the PR) Better than [$€£¥], but misses every alphabetic symbol: CHF, kr, , , Ft, Dhs, RM, Bs. Most of the 159 symbols contain letters.
getLocalizedCurrencySymbol (src/libs/CurrencyUtils.ts:90) Returns one locale-specific rendering of one currency (US$ vs $). Built for display, not parsing.
External library (currency-symbol-map etc.) Nothing like it is in package.json, and expensify-common has no symbol list or amount parser — only Num formatting helpers. Adding one would duplicate CURRENCY_LIST with a less authoritative copy.

Also fix: cents conversion is 100× off for zero-decimal currencies

Unrelated to the symbol question, but it's in the same lines. The backend stores 1/100 units for every currency, regardless of the currency's decimal count — see the doc comment on convertToBackendAmount, and note that convertToFrontendAmountAsString always divides by 100 while using decimals only for display precision.

The manual UI path agrees: WorkspaceWorkflowsApprovalsApprovalLimitPage.tsx:171 writes convertToBackendAmount(...) and :66 reads it back with convertToFrontendAmountAsString.

With 10 ** getCurrencyDecimals(currency), a JPY workspace importing 5000 stores 5000, which reads back as (5000 / 100).toFixed(0) = ¥50. That hits all 33 zero-decimal currencies in the list (JPY, KRW, VND, CLP, ISK, HUF-adjacent, XOF, XAF, …).

approvalLimit = String(convertToBackendAmount(Number.parseFloat(normalizedApprovalLimit)));

Keep getCurrencyDecimals where it belongs — in validateAmount, deciding how many decimals the user may type. The value stays a string, so the wire shape at src/libs/actions/Policy/Member.ts:1032 is unchanged.

Test to add

There is currently no test covering the approval-limit CSV column. tests/unit/ImportTransactions.test.ts:387-406 ('should handle amounts with currency symbols and commas') is the precedent to mirror — add cases for a non-Latin symbol (₹500), an alphabetic one (CHF 1,200.50 in a CHF workspace), a zero-decimal currency (¥5000500000), a blank cell, and a rejected value (12abc).


Next Steps: Reply with @MelvinBot <your feedback> to refine this.


view run

@rushatgabhane

Copy link
Copy Markdown
Member Author

@MelvinBot can you resolve conflicts

Resolved conflicts in src/libs/importSpreadsheetUtils.ts: main added
findColumnName/getCompanyCardColumnMappings while this branch added
normalizeImportedAmount. Both are additive, so kept both plus their
imports and combined the export list.
@MelvinBot

Copy link
Copy Markdown
Contributor

Done — conflicts resolved and pushed as a merge commit (2c58e14). The PR is MERGEABLE again.

One file conflicted: src/libs/importSpreadsheetUtils.ts

Both sides were purely additive, so I kept both:

The 11 language files auto-merged; ImportedMembersPage.tsx was untouched by main.

How I verified the resolution
  • The merged tree differs from main by exactly the PR's original 13 files and the same 55 insertions, 5 deletions as before the merge — so the merge added nothing and dropped nothing from your change.
  • No duplicate declarations; all 4 imports are used; all 5 exported names resolve for every one of the 11 import sites (including tests/unit/ImportTransactions.test.ts).
  • MoneyRequestUtils and StringUtils don't import importSpreadsheetUtils, so the new import edge doesn't create a cycle.
  • APIs the branch depends on all survived main's edits to CurrencyUtils/MoneyRequestUtils: convertToBackendAmount, validateAmount, stripCommaFromAmount, stripSpacesFromAmount, and useCurrencyListActions().getCurrencyDecimals/getCurrencySymbol.
  • Mobile-Expensify submodule pointer matches main.

Heads-up: I could not run typecheck, lint, or prettier locally — those commands were blocked in my sandbox, so I verified statically instead. Please let CI confirm before merging.

Unrelated to the conflict, two things in this PR still need you:

  1. The PROPOSAL: line in the description is still the <add the approved proposal comment URL here> placeholder.
  2. invalidApprovalLimit is the English string in all 11 locales, including de/fr/ja/zh-hans.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants