sadpandajoe commented on code in PR #42142:
URL: https://github.com/apache/superset/pull/42142#discussion_r3697443820
##########
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"},
Review Comment:
Excluding `json_metadata` here makes `set_dash_metadata` the only writer,
but that helper copies only a fixed subset of incoming keys. The Properties
modal's `show_chart_timestamps` toggle and Advanced JSON edits to fields such
as `stagger_refresh` or `timed_refresh_immune_slices` now return 200 while
reverting to the stored value; should the merge carry all incoming metadata
keys before applying its special cases?
##########
superset-frontend/src/dashboard/components/PropertiesModal/PropertiesModal.test.tsx:
##########
@@ -259,6 +284,175 @@ 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);
+ });
+
+ test('propagates a Refresh dropdown-only change into the submitted JSON
metadata', async () => {
+ // Selecting a value from the Refresh dropdown without touching the
+ // Advanced JSON editor must still make it into the submitted payload --
+ // handleRefreshFrequencyChange writes the new value into the JSON
+ // metadata object on change (#42116, requested in review).
+ 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,
Review Comment:
This test still passes if the new dropdown-to-JSON synchronization is
removed: inherited `dashboardInfo.metadata` is the raw JSON string, so the
modal seeds an object with no `refresh_frequency`, and save falls back to the
selected `60` anyway. Could we pass parsed metadata here so the test actually
starts at `0` and fails without the sync?
```suggestion
json_metadata: mockedJsonMetadata,
metadata: JSON.parse(mockedJsonMetadata),
```
--
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]
