rusackas commented on code in PR #42142:
URL: https://github.com/apache/superset/pull/42142#discussion_r3666454342
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -343,7 +343,12 @@ const PropertiesModal = ({
? resettableCustomLabels
: false;
const jsonMetadataObj = getJsonMetadata();
- jsonMetadataObj.refresh_frequency = refreshFrequency;
+ // A refresh_frequency edited directly in the Advanced JSON editor takes
+ // precedence over the Refresh dropdown state, mirroring how color_scheme
is
+ // handled above. Nullish coalescing preserves an explicit 0 ("Don't
+ // refresh") rather than falling through to the dropdown value (#42116).
+ jsonMetadataObj.refresh_frequency =
+ jsonMetadataObj.refresh_frequency ?? refreshFrequency;
Review Comment:
Good catch, fixed! The validator now checks the JSON editor's
`refresh_frequency` when it's set there, instead of only the dropdown state.
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -537,8 +542,14 @@ const PropertiesModal = ({
// Section handlers for extracted components
const handleThemeChange = (value: any) => setSelectedThemeId(value || null);
- const handleRefreshFrequencyChange = (value: number) =>
+ const handleRefreshFrequencyChange = (value: number) => {
setRefreshFrequency(value);
+ // Keep the Advanced JSON editor in sync with the dropdown so the two
+ // sources can't diverge, mirroring onColorSchemeChange (#42116).
+ const jsonMetadataObj = getJsonMetadata();
+ jsonMetadataObj.refresh_frequency = value;
+ setJsonMetadata(jsonStringify(jsonMetadataObj));
Review Comment:
Good catch, fixed! Now it only syncs the JSON editor when the current text
is actually parseable, so it won't stomp an in-progress edit.
##########
superset/daos/dashboard.py:
##########
@@ -396,15 +396,28 @@ def set_dash_metadata(
else:
md["color_namespace"] = data.get("color_namespace")
- md["expanded_slices"] = data.get("expanded_slices", {})
- if "refresh_frequency" in data:
- md["refresh_frequency"] = data["refresh_frequency"]
- md["color_scheme"] = data.get("color_scheme", "")
- md["label_colors"] = data.get("label_colors", {})
- md["shared_label_colors"] = data.get("shared_label_colors", [])
- md["map_label_colors"] = data.get("map_label_colors", {})
- md["color_scheme_domain"] = data.get("color_scheme_domain", [])
- md["cross_filters_enabled"] = data.get("cross_filters_enabled", True)
+ # Only overwrite these metadata fields when the caller explicitly sends
+ # them. Previously each used ``data.get(key, default)``, which reset a
+ # value to its default whenever it was absent from the payload -- e.g.
a
+ # ``refresh_frequency`` set directly in the Advanced JSON editor got
+ # wiped on save. ``setdefault`` still seeds a default for brand-new
+ # dashboards that have never had the key, keeping the shape stable
+ # without clobbering existing values (#42116).
+ metadata_defaults: dict[str, Any] = {
+ "expanded_slices": {},
+ "refresh_frequency": 0,
+ "color_scheme": "",
+ "label_colors": {},
+ "shared_label_colors": [],
+ "map_label_colors": {},
+ "color_scheme_domain": [],
+ "cross_filters_enabled": True,
+ }
+ for key, default_value in metadata_defaults.items():
+ if key in data:
+ md[key] = data[key]
+ else:
+ md.setdefault(key, default_value)
Review Comment:
Good catch, fixed! `shared_label_colors`, `map_label_colors`, and
`color_scheme_domain` are frontend-derived and the Properties modal never sends
them, so they keep resetting to default when absent instead of getting
preserved like the rest.
--
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]
