asdf2014 opened a new pull request, #42485:
URL: https://github.com/apache/superset/pull/42485
### SUMMARY
Editing a metric's currency through **Chart Source → Edit dataset** saves
fine, but the chart keeps rendering with the old formatting until the page is
fully reloaded (#42468).
`currency_formats` is a lookup of metric name to `Currency` that charts read
when formatting values. The backend never sends it: it only ships `currency` on
each metric, and the lookup is derived on the client. Until now that derivation
happened in exactly one place, when explore first hydrates. Saving the dataset
hands a fresh `GET /api/v1/dataset/{id}` payload to `changeDatasource`, that
payload has no `currency_formats`, and the derived value is simply overwritten
with nothing. The metric's own `d3format` survives the same round trip, because
charts read it directly off `datasource.metrics`, which is why only currency
appears to be ignored.
One detail worth calling out for review: charts render from
`state.explore.datasource`, which is written by
`UPDATE_FORM_DATA_BY_DATASOURCE`, not by `SET_DATASOURCE`. Deriving inside
`setDatasource` alone therefore does not fix the bug, so `changeDatasource` now
feeds both reducers the same derived object.
Changes:
- Add `getCurrencyFormats` to `@superset-ui/core`, next to
`buildCustomFormatters` and `getValueFormatter`, and route the two existing
inline derivations (`hydrateExplore`, `useDashboardDatasets`) through it.
- Derive in `setDatasource`, so a datasource handed over straight from an
API response carries the lookup by the time it reaches the store.
- `hydrateExplore` now derives onto a copy instead of writing through to the
caller's object. Chart pages hydrate from a module level fallback payload, so
writing through made that constant itself the datasource held in the store.
**Why not add a `currency_formats` property on the backend?** It is a fair
question, since `column_formats` is the exact structural twin (`{m.metric_name:
m.d3format for m in self.metrics if m.d3format}`) and is exposed through
`show_columns`. Three things argue against doing it here: `SemanticView` is a
second datasource implementation where `column_formats` is currently a
hardcoded placeholder, so each implementation would need its own copy; the
field would have to be added to both `explore/schemas.py` and
`dashboards/schemas.py`, which widens the surface and raises mixed version
questions; and the dashboard side consumes a camelCase `currencyFormats`, so a
snake_case server field would not be a drop in replacement. The client side fix
is small and backportable, and a server side property remains a reasonable
follow up.
**Known gaps, deliberately left out of scope.** Two dashboard writers of the
same store slice have the same defect and are not touched here:
`fetchDatasourceMetadata` (`src/dashboard/actions/datasources.ts`) on a store
miss, and `saveDashboardRequest` (`src/dashboard/actions/dashboardState.ts`)
after saving dashboard properties. Both are reachable and both drop the lookup.
I kept this PR to the explore path the issue reports, and will file a follow up
for the dashboard side.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Verified end to end on a locally built image, with a Big Number chart on a
metric configured as USD, changing the currency to EUR through Edit dataset and
**not** reloading the page:
| | before edit | after saving, no reload |
|---|---|---|
| master | `$ 10,032,628.85` | `10,032,628.85` |
| this PR | `$ 10,032,628.85` | `€ 10,032,628.85` |
On master the symbol disappears entirely rather than staying stale, which
matches the root cause: the lookup is gone, so the formatter falls back to
plain numbers.
### TESTING INSTRUCTIONS
1. Pick any dataset and give one metric a currency (Edit dataset → Metrics →
Metric currency), for example `$ (USD)` with a `,.2f` D3 format.
2. Build a Big Number chart on that metric. It renders with the currency
symbol.
3. In explore, open **Chart Source → ⋯ → Edit dataset → Metrics**, expand
the same metric, change **Metric currency** to `€ (EUR)`, and save.
4. Without reloading the page, the value should immediately render with the
new symbol. On master it loses its symbol until a full reload.
Unit tests: `npm run test -- src/explore/actions src/explore/reducers
packages/superset-ui-core/test/currency-format
src/hooks/apiResources/dashboards`.
Each production hunk was mutation checked: reverting the derivation in
`setDatasource`, the object sharing in `changeDatasource`, the copy in
`hydrateExplore`, or the filter in `getCurrencyFormats` turns at least one test
red.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: Fixes #42468
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
