sadpandajoe commented on code in PR #40679:
URL: https://github.com/apache/superset/pull/40679#discussion_r4033489038
##########
superset/dashboards/schemas.py:
##########
@@ -280,6 +284,9 @@ class DashboardGetResponseSchema(Schema):
dashboard_title = fields.String(
metadata={"description": dashboard_title_description}
)
+ localized_title = fields.String(
Review Comment:
`dashboard_title` is nullable (the POST/PUT schemas allow `None`, and the
model column is nullable), so this derived property can serialize as `null`.
Declaring it as a non-null string leaves the response/OpenAPI contract and the
new frontend types (`localized_title?: string`) unable to represent a valid
titleless dashboard. Could this field use `allow_none=True` and the TypeScript
declarations use `string | null`, with a titleless response case covered?
##########
superset-frontend/src/dashboard/actions/hydrate.ts:
##########
@@ -403,6 +405,12 @@ export const hydrateDashboard =
// only persistent refreshFrequency will be saved to backend
shouldPersistRefreshFrequency: false,
css: dashboard.css || '',
+ // Display-only localized title; the canonical title lives in the
+ // header layout meta (meta.text) and is what edits/saves operate on.
+ localizedTitle: dashboard.localized_title,
Review Comment:
The version-preview path spreads the live dashboard and overrides only
`dashboard_title` with the snapshot value. This line therefore keeps the live
`localized_title`, while line 413 records the snapshot title as the value it
belongs to; the header guard accepts that mismatched pair and renders the live
title over historical content. This also happens with the feature off because
`localized_title` then mirrors the live canonical title. Could the preview
clear/override `localized_title` (or carry the title it was actually resolved
for) and add a renamed-title preview test?
##########
superset/jinja_context.py:
##########
@@ -1108,6 +1109,20 @@ def set_context(self, **kwargs: Any) -> None:
}
)
+ def i18n_with_cache_key(default_text: str) -> str:
+ # The rendered value varies by locale, so it has to vary the query
+ # cache key too -- otherwise one viewer's translated SQL result is
+ # served to a viewer in another locale within the cache timeout.
+ # Keyed on the resolved text rather than the locale so locales that
+ # resolve alike still share a cache entry.
+ return extra_cache.cache_key_wrapper(i18n_macro(default_text))
+
+ # Registered only when a deployment has not already bound this name
+ # through JINJA_CONTEXT_ADDONS: the macro is new, so an existing addon
+ # called ``i18n`` has to keep working across the upgrade.
+ if "i18n" not in self._context:
Review Comment:
This check covers all entries in `self._context`, not just
`JINJA_CONTEXT_ADDONS`: dataset `template_params` are merged into the same
dictionary first. An existing dataset with an unrelated `i18n` template
parameter will therefore suppress the new macro and `{{ i18n('Sales') }}` fails
because the parameter is not callable. Could the compatibility guard test
`context_addons()` specifically, so addons retain precedence without letting
per-query parameters unbind a core macro?
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx:
##########
@@ -762,6 +762,15 @@ const Chart = (props: ChartProps) => {
props.updateSliceName(props.id, name)
}
sliceName={props.sliceName}
+ localizedName={
Review Comment:
The localized name is passed only as a sibling prop to `SliceHeader`;
`sliceForHeader` still contains only the canonical `slice_name`. As a result
the visible header can say “Ventes” while the kebab menu tooltip says “Click to
edit Sales” and the View as table modal is titled “Chart Data: Sales”, since
both read `slice.slice_name` from `SliceHeaderControls`. Could the display name
also be propagated to those two UI strings while keeping the canonical name for
edits and filenames?
##########
superset/models/dashboard.py:
##########
@@ -329,13 +349,27 @@ def data(self) -> dict[str, Any]:
positions = self.position_json
if positions:
positions = json.loads(positions)
+ # Resolve every chart name in one shot; the per-slice
``localized_name``
+ # lookups below then read from the request memo instead of hitting the
+ # translation hook once per chart. Gated so a disabled deployment does
+ # not pay for the extra pass over the slices.
+ if is_asset_translation_enabled():
+ translate_many(
+ (slc.slice_name for slc in self.slices),
+ model_name="Slice",
+ field_name="slice_name",
+ )
return {
"id": self.id,
"metadata": self.params_dict,
"certified_by": self.certified_by,
"certification_details": self.certification_details,
"css": self.css,
+ # ``dashboard_title`` stays canonical: the layout header seeds
+ # ``meta.text`` from it and persists it on save. The localized
value
+ # is exposed separately for display only.
"dashboard_title": self.dashboard_title,
+ "localized_title": self.localized_title,
Review Comment:
Adding this read-only property to `Dashboard.data` makes the existing
data-to-update round trip fail: `UpdateDashboardCommand` eventually calls
`setattr(dashboard, "localized_title", ...)`, but the property has no setter.
The change to `update_tabs_test.py` works around that by popping this key,
which leaves every other caller to do the same. Since the REST GET path dumps
the model through `DashboardGetResponseSchema` directly, could this key (and
its otherwise-unused prefetch in `data`) be removed instead?
--
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]
