rusackas commented on code in PR #42142:
URL: https://github.com/apache/superset/pull/42142#discussion_r3706288612
##########
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:
Good catch, thanks. `set_dash_metadata` only special-cased a handful of keys
and never merged anything else, so `show_chart_timestamps`, `stagger_refresh`,
and `timed_refresh_immune_slices` were reverting silently. Merged everything
else through before the special-casing runs, plus a regression test covering
those three.
##########
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:
Good catch, thanks. `handleDashboardData` reads the parsed `metadata`
object, not `json_metadata`, so the seeded JSON had no `refresh_frequency` key
and the assertion passed via the `?? refreshFrequency` fallback regardless of
the sync code. Added `metadata: JSON.parse(mockedJsonMetadata)` to match the
pattern the other test already uses, and confirmed it fails if the sync in
`handleRefreshFrequencyChange` is reverted.
--
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]
