feat: configurable thousands/decimal separators for number formatting#7749
Open
eran132 wants to merge 2 commits into
Open
feat: configurable thousands/decimal separators for number formatting#7749eran132 wants to merge 2 commits into
eran132 wants to merge 2 commits into
Conversation
numeral.js treats `,` and `.` in format strings as locale markers, not literal characters, so the rendered separators always came from the active `en` locale's delimiters. There was no Redash setting to override them, so a space (or NBSP/apostrophe) thousands separator was unreachable — formats like "0 0" silently dropped the space and produced no grouping at all. Add `REDASH_THOUSANDS_SEPARATOR` / `REDASH_DECIMAL_SEPARATOR` (org settings, defaults `,` / `.` so existing installs are unchanged), exposed via `number_format_config()` and applied once to `numeral.localeData().delimiters` when visualization settings update. Also surface both as editable fields in the organization General settings page. Fixes getredash#7689 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds organization-configurable thousands and decimal separators so numerals can be rendered with non-default delimiters (e.g., spaces for grouping and commas for decimals).
Changes:
- Introduces
thousands_separator/decimal_separatororg settings in the backend and exposes them to the frontend format config. - Updates frontend visualization settings to override numeral.js locale delimiters based on configured separators.
- Adds unit, backend, and Cypress coverage for separator customization and persistence.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| viz-lib/src/visualizations/visualizationsSettings.tsx | Adds separator settings and applies them by mutating numeral.js locale delimiters during settings updates |
| viz-lib/src/lib/value-format.test.ts | Adds unit tests validating default and custom delimiter rendering |
| tests/handlers/test_settings.py | Adds API tests for updating and defaulting separator settings |
| redash/settings/organization.py | Adds env-backed defaults and includes separators in organization settings defaults |
| redash/handlers/authentication.py | Exposes separator settings to frontend number format config |
| client/cypress/integration/settings/organization_settings_spec.js | Adds E2E flow verifying separator setting persistence and table rendering |
| client/app/pages/settings/components/GeneralSettings/FormatSettings.jsx | Adds UI inputs for thousands/decimal separator settings |
| client/app/components/visualizations/visualizationComponents.jsx | Includes new visualization settings keys for propagation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
56
to
69
| export function updateVisualizationsSettings(options: any) { | ||
| extend(visualizationsSettings, options); | ||
|
|
||
| // `,` and `.` in numeral format strings are locale markers, not literal characters — | ||
| // the rendered separators come from the active locale's delimiters. Override them so | ||
| // settings like a space thousands separator (e.g. "1 234 567") are reachable. | ||
| const { thousandsSeparator, decimalSeparator } = visualizationsSettings; | ||
| if (isString(thousandsSeparator) && isString(decimalSeparator)) { | ||
| extend(numeral.localeData().delimiters, { | ||
| thousands: thousandsSeparator, | ||
| decimal: decimalSeparator, | ||
| }); | ||
| } | ||
| } |
Comment on lines
+4
to
+8
| // numeral keeps a single global locale, so restore the defaults after each test | ||
| // to avoid leaking separator overrides into unrelated specs. | ||
| afterEach(() => { | ||
| updateVisualizationsSettings({ thousandsSeparator: ",", decimalSeparator: "." }); | ||
| }); |
Comment on lines
+51
to
+56
| <Input | ||
| style={{ width: 300 }} | ||
| value={values.thousands_separator} | ||
| onChange={e => onChange({ thousands_separator: e.target.value })} | ||
| data-test="ThousandsSeparatorInput" | ||
| /> |
Comment on lines
+50
to
+54
| cy.getByTestId("OrganizationSettingsSaveButton").click(); | ||
|
|
||
| // round-trips through the backend | ||
| cy.reload(); | ||
| cy.getByTestId("ThousandsSeparatorInput").should("have.value", " "); |
Comment on lines
+48
to
+49
| // capture the new separator fields for the PR description | ||
| cy.getByTestId("OrganizationSettings").screenshot("format-settings-separators"); |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- apply each numeral delimiter independently with a default fallback, so a non-string override never leaves numeral stuck on stale delimiters - reset separators in beforeEach too, making the unit suite order-independent - cap the separator inputs at maxLength=1 to match the "Character" copy - wait on the save request before reloading in the e2e spec (avoids a race) and drop the one-off screenshot capture Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Fixes #7689. Setting
REDASH_INTEGER_FORMAT="0 0"(or a space separator via the orgsettings) doesn't produce space-grouped numbers — and on closer inspection Redash isn't stripping the space at all.The root cause is numeral.js semantics:
,and.in numeral format strings are locale markers, not literal characters. The rendered separators come from the active locale'sdelimiters(en→,/.), and Redash never overrode them — so a space/NBSP/apostrophe thousands separator was simply unreachable, and an invalid pattern like"0 0"silently dropped the space (no grouping).Per the discussion in #7689 with @arikfr, this implements the explicit-separator approach (the locale option can follow as a thin layer on top):
thousands_separator/decimal_separator, env-defaulted viaREDASH_THOUSANDS_SEPARATOR/REDASH_DECIMAL_SEPARATOR. Defaults,/.— zero behavior change for existing installs.number_format_config()→ clientConfig and applied once tonumeral.localeData().delimiterswhenever visualization settings update.This makes full continental-European formatting reachable, e.g.
1 234 567,89.Screenshot
New fields on the General settings page:
Testing
All verified locally by execution (Docker, real Postgres + Redis):
tests/handlers/test_settings.py) — round-trip + defaults:5 passed.value-format.test.ts) — delimiter override incl. the reported space case:4 passed.organization_settings_spec.js) — set separator in the UI → reload (backend round-trip) → table renders1 234 567:3 passingon a clean run.