rusackas commented on code in PR #42142:
URL: https://github.com/apache/superset/pull/42142#discussion_r3693892507
##########
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:
Good catch. That's a real bug, the generic attribute update in
`DashboardDAO.update` was setting `dashboard.json_metadata` to the incoming
payload before `set_dash_metadata` ran, so `params_dict` had nothing but the
new data to merge against by the time the merge logic saw it. The DAO test
passed because it calls `set_dash_metadata` directly on an untouched dashboard,
but the real PUT path was resetting anything omitted. I excluded
`json_metadata` from the generic update in `UpdateDashboardCommand.run` so
`set_dash_metadata` is the only writer, plus a regression test through the
actual API endpoint that reproduces it (fails without the fix, passes with it).
##########
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:
Added it, selecting '1 minute' (the 60-second option) from the dropdown
without touching the JSON editor, then asserting the submitted json_metadata
carries refresh_frequency: 60.
--
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]
