aminghadersohi commented on code in PR #42142:
URL: https://github.com/apache/superset/pull/42142#discussion_r3681328113
##########
tests/integration_tests/dashboards/dao_tests.py:
##########
@@ -73,6 +73,46 @@ def test_get_dashboard_changed_on(self, mock_sm_g, mock_g):
DashboardDAO.set_dash_metadata(dashboard, original_data)
db.session.commit()
+ @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
+ @patch("superset.utils.core.g")
+ @patch("superset.security.manager.g")
+ def test_set_dash_metadata_preserves_unsent_fields(self, mock_sm_g,
mock_g):
+ """
+ set_dash_metadata must not reset metadata fields that are absent from
the
+ incoming payload, such as a ``refresh_frequency`` edited directly in
the
+ Advanced JSON editor (#42116). Fields that are present still override.
+ """
+ mock_g.user = mock_sm_g.user = security_manager.find_user("admin")
+ with self.client.application.test_request_context():
+ dashboard = (
+
db.session.query(Dashboard).filter_by(slug="world_health").first()
+ )
+ original_json_metadata = dashboard.json_metadata
+ try:
+ # Seed an existing refresh_frequency in the stored metadata.
+ metadata = json.loads(dashboard.json_metadata or "{}")
+ metadata["refresh_frequency"] = 60
+ dashboard.json_metadata = json.dumps(metadata)
+ db.session.commit()
+
+ # Payload omits refresh_frequency: it must be preserved, not
+ # reset to 0.
+ DashboardDAO.set_dash_metadata(
+ dashboard, {"color_scheme": "d3Category10"}
+ )
+ db.session.commit()
+ saved = json.loads(dashboard.json_metadata)
+ assert saved["refresh_frequency"] == 60
+ assert saved["color_scheme"] == "d3Category10"
Review Comment:
nit: this block passes on the pre-PR backend too — the
`refresh_frequency`-when-absent guard already landed in #42354 (the `if
"refresh_frequency" in data` branch), so reverting this PR's
`set_dash_metadata` change would not fail this test. The genuinely new behavior
here is preserving
`cross_filters_enabled`/`color_scheme`/`label_colors`/`expanded_slices` when
absent. Consider seeding e.g. `cross_filters_enabled=False`, then asserting it
survives a payload that omits it, so the test guards this diff rather than
#42354's.
##########
superset-frontend/src/dashboard/components/PropertiesModal/PropertiesModal.test.tsx:
##########
@@ -259,6 +284,56 @@ 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);
Review Comment:
nit: this proves the JSON editor wins over a stale dropdown, but with a
truthy `30` it would not catch a regression from `??` back to `||` on
`index.tsx:353` — the falsy `0` case is the whole point of the nullish
coalescing. Consider a second assertion where the JSON holds
`refresh_frequency: 0` while the dropdown state is non-zero, asserting `0`
survives the 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]
