fix(reports): apply chart number and currency formatting to tables sent as text#41028
fix(reports): apply chart number and currency formatting to tables sent as text#41028massucattoj wants to merge 6 commits into
Conversation
dosubot
Bot
added
alert-reports
Code Review Agent Run #bacc66Actionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41028 +/- ##
==========================================
- Coverage 64.42% 64.37% -0.05%
==========================================
Files 2668 2669 +1
Lines 147183 147335 +152
Branches 33947 33979 +32
==========================================
+ Hits 94822 94848 +26
- Misses 50646 50770 +124
- Partials 1715 1717 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #643aa1Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
EnxDev's Review Agent — #41028 · HEAD ab9ba0fcomment — Solid, well-tested port that's a clear improvement over the broken status quo, but it diverges from the browser (d3) on a common preset with float data, plus a few rounding/scope items worth settling before merge. I generated d3-format v3 output for the full preset set and diffed it against the port; findings below are from that comparison. 🔴 Functional
🟡 Should-fix
🔵 Nits
🙌 Praise
|
…to raw, docs/tests
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #d92676Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| column_formats = form_data.get("columnFormats") or {} | ||
| currency_format = form_data.get("currencyFormat") or {} | ||
| currency_formats = form_data.get("currencyFormats") or {} | ||
| metric_level = -1 if form_data.get("combineMetric") else 0 |
There was a problem hiding this comment.
Will this drop per-metric pivot formats when metricsLayout == "ROWS"?
| """ | ||
| if value is None: | ||
| return "" | ||
| if isinstance(value, bool) or not isinstance(value, (int, float)): |
There was a problem hiding this comment.
Won't this skip Decimal values entirely since int/float are treated as numeric?
| # falls back to SMART_NUMBER when no explicit format is set | ||
| number_format = (d3_format or SMART_NUMBER).replace("$", "") | ||
| formatted = format_numeric(number_format, value) | ||
| if currency["symbol"] == AUTO_CURRENCY: |
There was a problem hiding this comment.
Will this ignore detect_currency and row/cell currency context?
|
|
||
| SMART_NUMBER = "SMART_NUMBER" | ||
| SMART_NUMBER_SIGNED = "SMART_NUMBER_SIGNED" | ||
| AUTO_CURRENCY = "AUTO" | ||
|
|
||
| LOCALE = "en_US" |
There was a problem hiding this comment.
Suggestion: Add explicit type annotations to the newly introduced module-level string constants so they comply with the type-hint requirement. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
These newly introduced module-level string constants are unannotated, and the custom rule requires type hints for new Python variables that can be annotated.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/number_format.py
**Line:** 40:45
**Comment:**
*Custom Rule: Add explicit type annotations to the newly introduced module-level string constants so they comply with the type-hint requirement.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| # SI prefixes keyed by their power-of-1000 exponent, mirroring d3-format. | ||
| SI_PREFIXES = { | ||
| -8: "y", | ||
| -7: "z", | ||
| -6: "a", | ||
| -5: "f", | ||
| -4: "p", | ||
| -3: "n", | ||
| -2: "µ", | ||
| -1: "m", | ||
| 0: "", | ||
| 1: "k", | ||
| 2: "M", | ||
| 3: "G", | ||
| 4: "T", | ||
| 5: "P", | ||
| 6: "E", | ||
| 7: "Z", | ||
| 8: "Y", | ||
| } |
There was a problem hiding this comment.
Suggestion: Add a concrete type annotation to this module-level mapping to satisfy the rule for annotatable variables. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This module-level mapping is newly added without a type annotation, which violates the rule requiring type hints on annotatable Python variables.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/number_format.py
**Line:** 47:66
**Comment:**
*Custom Rule: Add a concrete type annotation to this module-level mapping to satisfy the rule for annotatable variables.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| ) | ||
|
|
||
|
|
||
| def test_table_applies_currency_format(): |
There was a problem hiding this comment.
Suggestion: Add an explicit return type annotation to this new test function (for example, annotate it as returning None). [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This new Python test function omits an explicit return type annotation. The custom rule requires type hints on new or modified Python functions when they can be annotated, so this is a real violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/charts/test_client_processing.py
**Line:** 1856:1856
**Comment:**
*Custom Rule: Add an explicit return type annotation to this new test function (for example, annotate it as returning `None`).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| assert formatted["amount"].tolist() == ["$ 1,234.50"] | ||
|
|
||
|
|
||
| def test_table_applies_si_number_format(): |
There was a problem hiding this comment.
Suggestion: Add an explicit return type annotation to this new test function (for example, annotate it as returning None). [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This newly added test function has no return type annotation. Under the Python type-hint rule, it should be annotated (e.g. -> None).
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/charts/test_client_processing.py
**Line:** 1874:1874
**Comment:**
*Custom Rule: Add an explicit return type annotation to this new test function (for example, annotate it as returning `None`).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| assert formatted["amount"].tolist() == ["1.23k"] | ||
|
|
||
|
|
||
| def test_pivot_table_v2_applies_value_format(): |
There was a problem hiding this comment.
Suggestion: Add an explicit return type annotation to this new test function (for example, annotate it as returning None). [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The function is new and currently unannotated. The rule explicitly flags new Python code that omits type hints on functions, so this matches the violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/charts/test_client_processing.py
**Line:** 1887:1887
**Comment:**
*Custom Rule: Add an explicit return type annotation to this new test function (for example, annotate it as returning `None`).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| assert formatted[("qty",)].tolist() == ["10", "20"] | ||
|
|
||
|
|
||
| def test_pivot_table_v2_applies_per_metric_format_when_metrics_combined(): |
There was a problem hiding this comment.
Suggestion: Add an explicit return type annotation to this new test function (for example, annotate it as returning None). [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This test definition lacks a type hint on the function signature. Since it is newly introduced Python code, the rule requires an explicit annotation such as -> None.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/charts/test_client_processing.py
**Line:** 1909:1909
**Comment:**
*Custom Rule: Add an explicit return type annotation to this new test function (for example, annotate it as returning `None`).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| assert formatted[("x", "sales")].tolist() == ["100.00", "200.00"] | ||
|
|
||
|
|
||
| def test_format_column_applies_d3_and_currency(): |
There was a problem hiding this comment.
Suggestion: Add an explicit return type annotation to this new test function (for example, annotate it as returning None). [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This newly added test function is missing an explicit return type annotation. That is a direct violation of the Python type-hint requirement.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/charts/test_client_processing.py
**Line:** 1938:1938
**Comment:**
*Custom Rule: Add an explicit return type annotation to this new test function (for example, annotate it as returning `None`).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| def test_format_number_with_currency(currency, value, expected): | ||
| assert format_number_with_config(",.2f", currency, value) == expected |
There was a problem hiding this comment.
Suggestion: Add explicit type annotations to the currency, value, and expected parameters and declare the return type. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This function is newly added and lacks annotations for its parameters and return type, so it matches the custom rule requiring type hints in Python code.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/number_format_test.py
**Line:** 50:51
**Comment:**
*Custom Rule: Add explicit type annotations to the `currency`, `value`, and `expected` parameters and declare the return type.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| # One intentional deviation: d3 emits a Unicode minus (U+2212); the Python helper | ||
| # emits an ASCII "-" for email/CSV safety, so the comparison normalizes it. | ||
|
|
||
| VALUES = [12345.432, 0, 4725, 80679663, 1234567890, -1234.5, 0.0123, 999.9] |
There was a problem hiding this comment.
Suggestion: Annotate this module-level variable with an explicit type to comply with required variable typing. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This module-level constant is a relevant variable that can be annotated, but it is left untyped in the new file, so the type-hint rule applies.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/number_format_test.py
**Line:** 94:94
**Comment:**
*Custom Rule: Annotate this module-level variable with an explicit type to comply with required variable typing.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| EXPECTED = { | ||
| "~g": [ | ||
| "12345.4", | ||
| "0", | ||
| "4725", | ||
| "8.06797e+7", | ||
| "1.23457e+9", | ||
| "−1234.5", | ||
| "0.0123", | ||
| "999.9", | ||
| ], # noqa: E501 | ||
| ",d": [ | ||
| "12,345", | ||
| "0", | ||
| "4,725", | ||
| "80,679,663", | ||
| "1,234,567,890", | ||
| "−1,235", | ||
| "0", | ||
| "1,000", | ||
| ], # noqa: E501 | ||
| ".1s": ["10k", "0", "5k", "80M", "1G", "−1k", "10m", "1k"], | ||
| ".3s": ["12.3k", "0.00", "4.73k", "80.7M", "1.23G", "−1.23k", "12.3m", "1.00k"], | ||
| ",.1%": [ | ||
| "1,234,543.2%", | ||
| "0.0%", | ||
| "472,500.0%", | ||
| "8,067,966,300.0%", | ||
| "123,456,789,000.0%", | ||
| "−123,450.0%", | ||
| "1.2%", | ||
| "99,990.0%", | ||
| ], # noqa: E501 | ||
| ".2%": [ | ||
| "1234543.20%", | ||
| "0.00%", | ||
| "472500.00%", | ||
| "8067966300.00%", | ||
| "123456789000.00%", | ||
| "−123450.00%", | ||
| "1.23%", | ||
| "99990.00%", | ||
| ], # noqa: E501 | ||
| ".3%": [ | ||
| "1234543.200%", | ||
| "0.000%", | ||
| "472500.000%", | ||
| "8067966300.000%", | ||
| "123456789000.000%", | ||
| "−123450.000%", | ||
| "1.230%", | ||
| "99990.000%", | ||
| ], # noqa: E501 | ||
| ".4r": [ | ||
| "12350", | ||
| "0.000", | ||
| "4725", | ||
| "80680000", | ||
| "1235000000", | ||
| "−1235", | ||
| "0.01230", | ||
| "999.9", | ||
| ], # noqa: E501 | ||
| ",.1f": [ | ||
| "12,345.4", | ||
| "0.0", | ||
| "4,725.0", | ||
| "80,679,663.0", | ||
| "1,234,567,890.0", | ||
| "−1,234.5", | ||
| "0.0", | ||
| "999.9", | ||
| ], # noqa: E501 | ||
| ",.2f": [ | ||
| "12,345.43", | ||
| "0.00", | ||
| "4,725.00", | ||
| "80,679,663.00", | ||
| "1,234,567,890.00", | ||
| "−1,234.50", | ||
| "0.01", | ||
| "999.90", | ||
| ], # noqa: E501 | ||
| ",.3f": [ | ||
| "12,345.432", | ||
| "0.000", | ||
| "4,725.000", | ||
| "80,679,663.000", | ||
| "1,234,567,890.000", | ||
| "−1,234.500", | ||
| "0.012", | ||
| "999.900", | ||
| ], # noqa: E501 | ||
| "+,": [ | ||
| "+12,345.432", | ||
| "+0", | ||
| "+4,725", | ||
| "+80,679,663", | ||
| "+1,234,567,890", | ||
| "−1,234.5", | ||
| "+0.0123", | ||
| "+999.9", | ||
| ], # noqa: E501 | ||
| "$,.2f": [ | ||
| "$12,345.43", | ||
| "$0.00", | ||
| "$4,725.00", | ||
| "$80,679,663.00", | ||
| "$1,234,567,890.00", | ||
| "−$1,234.50", | ||
| "$0.01", | ||
| "$999.90", | ||
| ], # noqa: E501 | ||
| "(,.2f": [ | ||
| "12,345.43", | ||
| "0.00", | ||
| "4,725.00", | ||
| "80,679,663.00", | ||
| "1,234,567,890.00", | ||
| "(1,234.50)", | ||
| "0.01", | ||
| "999.90", | ||
| ], # noqa: E501 | ||
| "($,.2f": [ | ||
| "$12,345.43", | ||
| "$0.00", | ||
| "$4,725.00", | ||
| "$80,679,663.00", | ||
| "$1,234,567,890.00", | ||
| "($1,234.50)", | ||
| "$0.01", | ||
| "$999.90", | ||
| ], # noqa: E501 | ||
| " ,.2f": [ | ||
| " 12,345.43", | ||
| " 0.00", | ||
| " 4,725.00", | ||
| " 80,679,663.00", | ||
| " 1,234,567,890.00", | ||
| "−1,234.50", | ||
| " 0.01", | ||
| " 999.90", | ||
| ], # noqa: E501 | ||
| } |
There was a problem hiding this comment.
Suggestion: Provide an explicit type annotation for this dictionary constant declaration. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This constant dictionary is introduced without an explicit type annotation, which is covered by the custom rule requiring type hints for relevant variables in new Python code.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/number_format_test.py
**Line:** 96:239
**Comment:**
*Custom Rule: Provide an explicit type annotation for this dictionary constant declaration.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| sign_mode = match.group(3) or "-" | ||
| currency_symbol = match.group(4) == "$" | ||
| comma = "," if match.group(7) else "" | ||
| precision = int(match.group(8)) if match.group(8) is not None else None | ||
| trim = bool(match.group(9)) | ||
| type_ = (match.group(10) or "").lower() |
There was a problem hiding this comment.
Suggestion: The parser reads full d3 specifier grammar, but width/padding/alignment flags are ignored during rendering, so valid user-entered d3 formats (for example zero-padded width formats) will not match frontend output. Either implement these parsed flags or reject unsupported specifiers explicitly to avoid silent mis-formatting. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Width-padded d3 formats lose padding in reports.
- ⚠️ Aligned numeric formats differ between Explore and alerts.Steps of Reproduction ✅
1. In `superset/utils/number_format.py`, inspect the d3 specifier regex and parser:
`D3_FORMAT_RE` at lines 29–33 accepts the full d3-format grammar, including optional fill,
alignment, zero-padding, and width
(`[[fill]align][sign][symbol][0][width][,][.precision][~][type]` as per the comment at
lines 29–30).
2. Still in `format_d3` (lines 29–78 of the same file), after matching the specifier, the
code extracts only `sign_mode` (group 3), `currency_symbol` (group 4), `comma` (group 7),
`precision` (group 8), `trim` (group 9), and `type_` (group 10) at lines 44–49; it
completely ignores the parsed fill character (group 1), alignment (group 2), the
zero-padding flag (group 5), and field width (group 6).
3. Consider a user-configured d3 format such as `"08.2f"` or `"^10.3f"` entered in the
Table or Pivot chart number format control; this string is stored in `form_data` as
`d3NumberFormat`/`valueFormat`, then passed from `superset/charts/client_processing.py`
via `format_column` (lines 62–69) into `format_number_with_config`, which calls
`format_d3` when `d3_format` is not SMART or None.
4. When `format_d3` formats a value with such a specifier, the regex successfully matches
and `type_` is `"f"` with `precision` set, but since width, zero-padding, and alignment
are ignored, the function ultimately formats the magnitude using `format(magnitude,
f"{comma}.{precision}f")` (lines 63–69) producing a plain fixed-point number without the
requested field width or padding. On the frontend, d3-format honours these width and
alignment flags, so the Explore view shows padded/aligned numbers while the server-side
path used for Alerts/Reports silently drops those aspects, causing a formatting mismatch
for users who rely on these d3 options.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/number_format.py
**Line:** 142:147
**Comment:**
*Incomplete Implementation: The parser reads full d3 specifier grammar, but width/padding/alignment flags are ignored during rendering, so valid user-entered d3 formats (for example zero-padded width formats) will not match frontend output. Either implement these parsed flags or reject unsupported specifiers explicitly to avoid silent mis-formatting.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
Looks like you've made some good updates, but there are a ton of review comment threads that should be marked as resolved once they've been properly applied/disputed. |
4 participants
SUMMARY
When an Alert/Report sends a Table or Pivot Table chart as text (embedded in the email body), the number/currency formatting configured on the chart was not applied, so the email showed raw numbers.
Root cause is in superset/charts/client_processing.py, which reproduces the chart's client-side formatting on the server for reports:
work, but the common cases (the default SMART_NUMBER, SI like .1s, currency like $,.2f) raised and silently fell back to the raw value. currencyFormat was never read.
Fix: a new superset/utils/number_format.py that faithfully ports the frontend d3-format/currency formatters (it mirrors createSmartNumberFormatter.ts and CurrencyFormatter.ts), reusing Babel (already a dependency) for currency symbols. It is wired into table() (number + currency) and pivot_table_v2() (valueFormat/columnFormats + currency).
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
/api/v1/chart/<chart_id>/data/?type=post_processed&format=json
→ numeric columns with a configured format now render formatted (e.g. "8k", "$ 1,234.50") instead of raw.
pytest tests/unit_tests/utils/number_format_test.py tests/unit_tests/charts/test_client_processing.py
ADDITIONAL INFORMATION