sadpandajoe commented on code in PR #42142:
URL: https://github.com/apache/superset/pull/42142#discussion_r3697444885
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -537,8 +544,19 @@ 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). Skip
+ // this while the editor holds invalid/in-progress JSON so we don't
+ // clobber the user's unfinished edit with a one-field object.
+ const jsonMetadataObj = parseJsonMetadata();
+ if (!jsonMetadataObj) {
+ return;
+ }
+ jsonMetadataObj.refresh_frequency = value;
Review Comment:
Agreed—a truthy primitive parses successfully, then this assignment throws
in the dropdown handler instead of preserving the edit. Could we require a
non-array object before mutating it?
##########
superset/commands/dashboard/update.py:
##########
@@ -83,11 +83,23 @@ def run(self) -> Model:
json.loads(position_json)
)
- dashboard = DashboardDAO.update(self._model, self._properties)
- if self._properties.get("json_metadata"):
+ # ``set_dash_metadata`` merges the incoming metadata against
+ # ``dashboard.params_dict`` (the *stored* ``json_metadata``) to
+ # preserve fields the caller omitted. Routing ``json_metadata``
+ # through the generic attribute update below would overwrite
+ # that stored value before the merge ever sees it, silently
+ # collapsing the merge into a no-op and resetting any omitted
+ # field to its default -- so it is excluded here and applied
+ # exclusively via ``set_dash_metadata``.
+ json_metadata = self._properties.get("json_metadata")
+ dashboard = DashboardDAO.update(
+ self._model,
+ {k: v for k, v in self._properties.items() if k !=
"json_metadata"},
+ )
+ if json_metadata:
DashboardDAO.set_dash_metadata(
dashboard,
- data=json.loads(self._properties.get("json_metadata",
"{}")),
+ data=json.loads(json_metadata),
)
Review Comment:
Agreed—an explicit `null` or empty string is accepted by the PUT schema but
now returns success without changing the stored metadata. Could we distinguish
an omitted key from an explicit clearing value and preserve the endpoint’s
prior semantics?
--
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]
