Repository navigation
[Payment due @FitseTLT] Fix approval limit CSV import conversion - #101422
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
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 --helpTypically, you'd want to translate only what you changed by running |
bc2ef2f to
bcbfdc7
Compare
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.
|
bcbfdc7 to
3cde44e
Compare
|
@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? |
|
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 Separately: Recommended implementationDelete 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 const {getCurrencySymbol, getCurrencyDecimals} = useCurrencyListActions();
const currency = policy?.outputCurrency;
const normalized = normalizeImportedAmount(String(value), getCurrencySymbol(currency ?? ''), currency);
Why this over the alternatives
Also fix: cents conversion is 100× off for zero-decimal currenciesUnrelated 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 The manual UI path agrees: With approvalLimit = String(convertToBackendAmount(Number.parseFloat(normalizedApprovalLimit)));Keep Test to addThere is currently no test covering the approval-limit CSV column. Next Steps: Reply with |
|
@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.
|
Done — conflicts resolved and pushed as a merge commit ( One file conflicted: Both sides were purely additive, so I kept both:
The 11 language files auto-merged; How I verified the resolution
Heads-up: I could not run Unrelated to the conflict, two things in this PR still need you:
|
|
@MelvinBot are the conflicts cause because of me? |
|
No — this wasn't something you did. The conflict came from [Payment due @abdulrahuman5196] Improve column auto-mapping when the file structure is different landing on That PR rewrote the top of The PR is
They should clear once those fixes land on |
|
@MelvinBot is the - TypeScript Checks / typecheck (pull_request)](https://github.com/Expensify/App/actions/runs/35272704781/job/105375696220?pr=101422) |
|
No, neither failure is yours. Both are in test files your PR doesn't touch, and both are already fixed on Your diff only touches EvidenceTypeScript Checks — 2 errors, both in
ESLint — 1 error in That comes from |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
| fieldNotMapped: (fieldName: string) => `Ups! Ein erforderliches Feld („${fieldName}“) wurde nicht zugeordnet. Bitte überprüfe es und versuche es erneut.`, | ||
| singleFieldMultipleColumns: (fieldName: string) => `Ups! Du hast ein einzelnes Feld („${fieldName}“) mehreren Spalten zugeordnet. Bitte überprüfe dies und versuche es erneut.`, | ||
| emptyMappedField: (fieldName: string) => `Ups! Das Feld („${fieldName}“) enthält einen oder mehrere leere Werte. Bitte überprüfe es und versuche es erneut.`, | ||
| invalidApprovalLimit: 'Approval limits must be valid amounts for the workspace currency.', |
There was a problem hiding this comment.
The copies are not translated
| } else { | ||
| errors = {}; | ||
| } |
| /** | ||
| * Normalizes an amount cell from an imported spreadsheet by removing its currency marker, grouping separators, and whitespace. | ||
| */ | ||
| function normalizeImportedAmount(value: string, currencySymbol?: string, currencyCode?: string): string { |
There was a problem hiding this comment.
It would be ideal to add a unit test.
| const customField2 = membersCustomField2Column !== -1 ? (membersCustomField2?.[containsHeader ? index + 1 : index] ?? '') : undefined; | ||
| const approvalLimit = membersApprovalLimitColumn !== -1 ? (membersApprovalLimit?.[containsHeader ? index + 1 : index] ?? '') : undefined; | ||
| const approvalLimitValue = membersApprovalLimit?.[containsHeader ? index + 1 : index] ?? ''; | ||
| const normalizedApprovalLimit = normalizeImportedAmount(approvalLimitValue, currencySymbol, currency); |
There was a problem hiding this comment.
We can take this in the if condition
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2026-09-30.16-52-30.mp4Android: mWeb Chrome2026-09-30.16-21-45.mp4iOS: HybridAppiOS: mWeb Safari2026-09-30.16-19-09.mp4MacOS: Chrome / Safari2026-09-30.16-16-16.mp4 |
melvin-bot
Bot
changed the title
|
🎯 @FitseTLT, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
|
🎀 👀 🎀 C+ reviewed |
|
@rushatgabhane any updates here? 👋 |
|
@brianlee-expensify im waiting for an internal engineer to approve @Gonals assigned you for final review 🙇 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 Gonals has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/Gonals in version: 9.5.6-0 🚀
|
|
No help site update is needed: this bug fix makes CSV-imported approval limits use the amount users already expect (for example, |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.5.6-6 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Payment issue created: #103657 |
7 participants
Explanation of Change
Updated member CSV imports so approval limits are treated as the desired amount in the workspace currency.
For example, an approval limit of
500in a USD workspace is sent as50000cents and displayed as$500instead of$5.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/682002
PROPOSAL:
Tests
Email,Approval limit, andOver limit forward tofields.Approval limitin the CSV to the desired approval limit amount, e.g.500.$500, instead of$5.Offline tests
No offline behavior was changed. Offline testing was not performed.
QA Steps
Email,Approval limit, andOver limit forward tofields.Approval limitin the CSV to the desired approval limit amount, e.g.500.$500, not$5.ImportMembersSpreadsheetsends the approval limit converted to the backend minor-unit value, e.g.approvalLimit: 50000for a$500USD workspace.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionregressions (e.g. long loading states that impact usability).
coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
Avatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)strikethrough, and italic.
Avataris modified, I verifiedthat
Avataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
csv.value.test.mp4