EnxDev commented on code in PR #43988:
URL: https://github.com/apache/superset/pull/43988#discussion_r3959490605
##########
superset-frontend/src/explore/components/SaveModal.tsx:
##########
@@ -532,8 +532,13 @@ const SaveModal = ({
}
}
- // Sets the form data
- actions.setFormData({ ...formData });
+ // Saving a Query as a dataset synchronously updates form_data through
+ // changeDatasource. Re-applying this render's Query-backed form_data
+ // would overwrite that conversion just before createSlice reads the
+ // store, causing the chart API to receive datasource_type="query".
+ if (datasource?.type !== DatasourceType.Query) {
Review Comment:
I verified that the dashboard concern doesn’t apply here. `createSlice` and
`updateSlice` receive `sliceDashboards` as an explicit argument, and
`saveModalActions.ts:155–190` builds both `payload.dashboards` and
`params.dashboards` from that argument, never from `form_data`.
This means that `formData.dashboards = sliceDashboards` was dead code: the
value was assigned but never read anywhere. `selectedTabId` is also a local
variable, not part of the form data.
However, skipping that block did prevent `delete formData.url_params` from
running. That deletion modified the stale `form_data` object from the current
render, while `changeDatasource` had already replaced the object in the store
with a new one that still contained `url_params`. As a result, saving a
Query-backed chart could persist them.
I fixed this by changing the order of operations instead of adding a special
case: `setFormData` now runs before the dataset conversion. This makes the
rewrite performed by `changeDatasource` the final store update, ensuring that
`url_params` is removed on both paths. I also removed the unused
`formData.dashboards` assignment and the unnecessary type check
(`SaveModal.tsx:482–493`).
The test now verifies the expected behavior directly:
- `createSlice` receives `[dashboardId]` on the Query-backed path.
- `setFormData` and `changeDatasource` run in the correct order.
- `url_params` is not included in the final form data.
I also fixed the test fixture: the `GET /api/v1/dashboard/<id>` mock
returned `result` as a list, which meant that `dashboardResult.id` was always
`undefined`. Because of that, the previous assertion could not have detected a
missing dashboard.
All 27 tests pass, pre-commit is clean, and the changes were pushed in
`95d9734834`.
--
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]
