sadpandajoe commented on code in PR #42142:
URL: https://github.com/apache/superset/pull/42142#discussion_r3685925951
##########
superset/daos/dashboard.py:
##########
@@ -396,15 +396,36 @@ 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", {})
+ # 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": {},
+ "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:
This still resets omitted fields in the normal PUT path:
`UpdateDashboardCommand` assigns the incoming `json_metadata` before calling
this helper, so `md` already contains only that incoming object. For example, a
dashboard stored with `cross_filters_enabled: false` and a payload containing
only `color_scheme` reaches this line with no old value and gets reseeded to
`true`; could we merge against the stored metadata before that assignment and
cover the command/API boundary?
##########
superset-frontend/src/dashboard/components/PropertiesModal/PropertiesModal.test.tsx:
##########
@@ -259,6 +284,126 @@ describe('PropertiesModal', () => {
});
});
+ test('preserves a refresh_frequency edited in the JSON editor on save
(#42116)', async () => {
+ // Save (onlyApply: false) PUTs to the API before calling onSubmit, so the
+ // request must be mocked or onSubmit is never reached.
+ const put = jest.spyOn(SupersetCore.SupersetClient, 'put');
+ put.mockResolvedValue({
+ json: {
+ result: {
+ dashboard_title: 'dashboard_title',
+ slug: 'slug',
+ json_metadata: 'json_metadata',
+ editors: 'editors',
+ },
+ },
+ } as any);
+ mockedIsFeatureEnabled.mockReturnValue(false);
+ const props = createProps();
+ const propsWithDashboardInfo = {
+ ...props,
+ dashboardInfo: {
+ ...dashboardInfo,
+ json_metadata: mockedJsonMetadata,
+ },
+ };
+ render(<PropertiesModal {...propsWithDashboardInfo} />, {
+ useRedux: true,
+ });
+ await screen.findByTestId('dashboard-edit-properties-form');
+
+ // Expand the Advanced settings panel so the (mocked) JSON editor mounts.
+ const advancedHeader = screen
+ .getByText('Advanced settings')
+ .closest('.ant-collapse-header');
+ await userEvent.click(advancedHeader!);
+
+ // Edit refresh_frequency directly in the JSON editor without touching the
+ // Refresh dropdown (the reproduction of #42116).
+ const editor = await screen.findByTestId('mock-json-editor');
+ fireEvent.change(editor, {
+ target: { value: JSON.stringify({ refresh_frequency: 30 }) },
+ });
+
+ await userEvent.click(screen.getByRole('button', { name: 'Save' }));
+
+ await waitFor(() => {
+ expect(props.onSubmit).toHaveBeenCalledTimes(1);
+ });
+ const submitted = JSON.parse(props.onSubmit.mock.calls[0][0].jsonMetadata);
+ expect(submitted.refresh_frequency).toBe(30);
+ });
+
+ test('preserves an explicit 0 from the JSON editor over a non-zero dropdown
value (#42116)', async () => {
+ // A truthy value like 30 can't tell `??` and `||` apart. Only a falsy-but-
+ // explicit `refresh_frequency: 0` in the JSON, combined with a non-zero
+ // Refresh dropdown, catches a regression from `??` back to `||` on
+ // index.tsx, which would let the dropdown's non-zero value win over an
+ // explicit "Don't refresh".
+ const put = jest.spyOn(SupersetCore.SupersetClient, 'put');
+ put.mockResolvedValue({
+ json: {
+ result: {
+ dashboard_title: 'dashboard_title',
+ slug: 'slug',
+ json_metadata: 'json_metadata',
+ editors: 'editors',
+ },
+ },
+ } as any);
+ mockedIsFeatureEnabled.mockReturnValue(false);
+ const props = createProps();
+ // A non-zero refresh_frequency in dashboardInfo so the Refresh dropdown
+ // initializes to a truthy value (dashboardInfo, when passed, is used
+ // directly instead of triggering a fetch -- see the
`!currentDashboardInfo`
+ // check in the data-loading effect). handleDashboardData reads the parsed
+ // `metadata` object, not the `json_metadata` string.
+ const nonZeroMetadata = mockedJsonMetadata.replace(
+ '"refresh_frequency": 0',
+ '"refresh_frequency": 30',
+ );
+ const propsWithDashboardInfo = {
+ ...props,
+ dashboardInfo: {
+ ...dashboardInfo,
+ json_metadata: nonZeroMetadata,
+ metadata: JSON.parse(nonZeroMetadata),
+ },
+ };
+ render(<PropertiesModal {...propsWithDashboardInfo} />, {
+ useRedux: true,
+ });
+ await screen.findByTestId('dashboard-edit-properties-form');
+
+ // Confirm the Refresh dropdown actually picked up the non-zero value.
+ const refreshHeader = screen
+ .getByText('Refresh settings')
+ .closest('.ant-collapse-header');
+ await userEvent.click(refreshHeader!);
+ expect(
+ await screen.findByRole('radio', { name: '30 seconds' }),
+ ).toBeChecked();
+
+ // Edit the JSON editor to explicitly set refresh_frequency to 0 ("Don't
+ // refresh") without touching the Refresh dropdown.
+ const advancedHeader = screen
+ .getByText('Advanced settings')
+ .closest('.ant-collapse-header');
+ await userEvent.click(advancedHeader!);
+ const editor = await screen.findByTestId('mock-json-editor');
+ fireEvent.change(editor, {
+ target: { value: JSON.stringify({ refresh_frequency: 0 }) },
+ });
+
+ await userEvent.click(screen.getByRole('button', { name: 'Save' }));
+
+ await waitFor(() => {
+ expect(props.onSubmit).toHaveBeenCalledTimes(1);
+ });
+ const submitted = JSON.parse(props.onSubmit.mock.calls[0][0].jsonMetadata);
+ expect(submitted.refresh_frequency).toBe(0);
Review Comment:
Could we add a component regression that starts with `refresh_frequency:
30`, selects `60 seconds`, saves without editing Advanced JSON, and asserts the
submitted value is `60`? These tests only edit JSON; without the new
dropdown-to-JSON synchronization, the now-authoritative JSON stays at `30` and
silently overrides the user's dropdown selection on save.
--
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]
