Skip to content

fix/78095: Comma split on locale languages #78246

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dangrous merged 5 commits into from
Jan 8, 2026
Merged

fix/78095: Comma split on locale languages #78246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5e0e9e5
fix: comma split on locale languages
btkcodedev Dec 19, 2025
b82d2f1
update: test cases with both comma and dot
btkcodedev Dec 23, 2025
080caf2
use existing function for replacement
btkcodedev Dec 26, 2025
4ee02d3
fix: alphabetize imports
btkcodedev Dec 27, 2025
3b92690
Merge remote-tracking branch 'origin/main' into btkcodedev/78095Comma…
btkcodedev Jan 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/components/PercentageForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type {ForwardedRef} from 'react';
import React, {useCallback, useMemo, useRef} from 'react';
import useLocalize from '@hooks/useLocalize';
import {replaceAllDigits, stripCommaFromAmount, stripSpacesFromAmount, validatePercentage} from '@libs/MoneyRequestUtils';
import {replaceAllDigits, replaceCommasWithPeriod, stripSpacesFromAmount, validatePercentage} from '@libs/MoneyRequestUtils';
import CONST from '@src/CONST';
import TextInput from './TextInput';
import type {BaseTextInputProps, BaseTextInputRef} from './TextInput/BaseTextInput/types';
Expand Down Expand Up @@ -49,8 +49,9 @@ function PercentageForm({value: amount, errorText, onInputChange, label, allowEx
return;
}

const strippedAmount = stripCommaFromAmount(newAmountWithoutSpaces);
onInputChange?.(strippedAmount);
// Convert comma to period for internal representation (commas are used as decimal separators in some locales like Spanish)
const normalizedAmount = replaceCommasWithPeriod(newAmountWithoutSpaces);
onInputChange?.(normalizedAmount);
},
[allowExceedingHundred, allowDecimal, onInputChange],
);
Expand Down
7 changes: 5 additions & 2 deletions src/libs/MoneyRequestUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,17 @@ function validateAmount(amount: string, decimals: number, amountMaxLength: numbe
* Some callers (e.g. split-by-percentage) may temporarily allow values above 100 while the user edits; they can
* opt into this relaxed behavior via the `allowExceedingHundred` flag.
* The `allowDecimal` flag enables one decimal place (0.1 precision) for more granular percentage splits.
* Accepts both period (.) and comma (,) as decimal separators to support locale-specific input (e.g., Spanish).
*/
function validatePercentage(amount: string, allowExceedingHundred = false, allowDecimal = false): boolean {
if (allowExceedingHundred) {
const regex = allowDecimal ? /^\d*\.?\d?$/u : /^\d*$/u;
// Accept both period and comma as decimal separators for locale support (e.g., Spanish uses comma)
const regex = allowDecimal ? /^\d*[.,]?\d?$/u : /^\d*$/u;
return amount === '' || regex.test(amount);
}

const regexString = allowDecimal ? '^(100(\\.0)?|[0-9]{1,2}(\\.\\d)?)$' : '^(100|[0-9]{1,2})$';
// Accept both period and comma as decimal separators
const regexString = allowDecimal ? '^(100([.,]0)?|[0-9]{1,2}([.,]\\d)?)$' : '^(100|[0-9]{1,2})$';
const percentageRegex = new RegExp(regexString, 'i');
return amount === '' || percentageRegex.test(amount);
}
Expand Down
24 changes: 24 additions & 0 deletions tests/unit/MoneyRequestUtilsTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,30 @@ describe('ReportActionsUtils', () => {
expect(validatePercentage('7.55', true, true)).toBe(false);
expect(validatePercentage('abc', true, true)).toBe(false);
});

it('accepts comma as decimal separator for locale support (e.g., Spanish)', () => {
// With allowDecimal=true and comma separator
expect(validatePercentage('7,5', false, true)).toBe(true);
expect(validatePercentage('0,1', false, true)).toBe(true);
expect(validatePercentage('99,9', false, true)).toBe(true);
expect(validatePercentage('100,0', false, true)).toBe(true);

// With allowExceedingHundred=true and allowDecimal=true
expect(validatePercentage('150,5', true, true)).toBe(true);
expect(validatePercentage('7,5', true, true)).toBe(true);
expect(validatePercentage(',5', true, true)).toBe(true);

// Invalid: more than one decimal place with comma
expect(validatePercentage('7,55', true, true)).toBe(false);
expect(validatePercentage('100,01', false, true)).toBe(false);

// Invalid: mixed comma and period (should not accept both)
expect(validatePercentage('7,5.5', true, true)).toBe(false);
expect(validatePercentage('7.5,5', true, true)).toBe(false);
expect(validatePercentage('1,234.56', true, true)).toBe(false);
expect(validatePercentage('1.234,56', true, true)).toBe(false);
Comment on lines +100 to +101
Copy link
Contributor

@dangrous dangrous Dec 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the only test cases I'm unsure about - should these be accepted (assuming allowExceedingHundred)? Both would be valid percentages, since the first punctuation is being used as a hundreds separator instead of a decimal. What would the first one 1,234.56 return BEFORE we made the changes in this branch?

I realize we may need to do some combo of replaceCommasWithPeriod and stripCommasFromAmount - maybe we swap all periods with commas or something?

Copy link
Contributor Author

@btkcodedev btkcodedev Dec 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarifications:

  1. Before this branch, the regex only allowed one period not comma(which is the whole issue), so 1,234.56 would have been obviously rejected.
    Currently new regex - [.,]? optionally ONE comma or period followed by \d? optionally ONE more digit

  2. Regarding the concern about users entering commas as thousands separators: the input allows only a single decimal digit. This greatly limits the possibility of mixed notation or user confusion.

  3. a) Percentages 0-100 should never need thousands separators as in default
    b) IF allowExceedingHundred is passed, this is regex: /^\d*[.,]?\d?$/u - very permissive, allows any number with optional one decimal

The validation correctly supports either a comma or a period as the decimal separator, but not thousands separators or mixed notation, which is appropriate for percentages.

Copy link
Contributor Author

@btkcodedev btkcodedev Dec 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC @dangrous 🙇

Copy link
Contributor

@dangrous dangrous Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hrm okay - @heyjennahay from a product perspective, what do you think here? Is it okay to disallow someone putting 1,234.56 into the percentage (while allowing 1234.56)? I think it's pretty much an edge case, but since we're in there changing the logic, figured I'd check. Seems like it would make it a little more complicated

Copy link
Contributor

@heyjennahay heyjennahay Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the slow reply. I think this is fine. As you say, it is an edge case and not really worth putting a "complete" solution which handles all cases.

expect(validatePercentage('10,5.0', false, true)).toBe(false);
});
});

describe('handleNegativeAmountFlipping', () => {
Expand Down
Loading