-
Notifications
You must be signed in to change notification settings - Fork 14.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(explore): Update chart save to use API endpoints #20498
Changes from all commits
983e3cf
21602fc
9e1177f
415bddc
5299fe8
af35f22
1459c96
7253863
16da4d4
9c8278d
1aa7d40
5828c4f
57b6838
fcb00d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -16,50 +16,58 @@ | |||||||||||||||||||||||||||
* specific language governing permissions and limitations | ||||||||||||||||||||||||||||
* under the License. | ||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||
import React, { useEffect, useState } from 'react'; | ||||||||||||||||||||||||||||
import React, { useEffect, useRef, useState } from 'react'; | ||||||||||||||||||||||||||||
import { useDispatch } from 'react-redux'; | ||||||||||||||||||||||||||||
import { useLocation } from 'react-router-dom'; | ||||||||||||||||||||||||||||
import { makeApi, t } from '@superset-ui/core'; | ||||||||||||||||||||||||||||
import Loading from 'src/components/Loading'; | ||||||||||||||||||||||||||||
import { addDangerToast } from 'src/components/MessageToasts/actions'; | ||||||||||||||||||||||||||||
import { isNullish } from 'src/utils/common'; | ||||||||||||||||||||||||||||
import { getUrlParam } from 'src/utils/urlUtils'; | ||||||||||||||||||||||||||||
import { URL_PARAMS } from 'src/constants'; | ||||||||||||||||||||||||||||
import { getParsedExploreURLParams } from './exploreUtils/getParsedExploreURLParams'; | ||||||||||||||||||||||||||||
import { hydrateExplore } from './actions/hydrateExplore'; | ||||||||||||||||||||||||||||
import ExploreViewContainer from './components/ExploreViewContainer'; | ||||||||||||||||||||||||||||
import { ExploreResponsePayload } from './types'; | ||||||||||||||||||||||||||||
import { fallbackExploreInitialData } from './fixtures'; | ||||||||||||||||||||||||||||
import { addDangerToast } from '../components/MessageToasts/actions'; | ||||||||||||||||||||||||||||
import { isNullish } from '../utils/common'; | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const loadErrorMessage = t('Failed to load chart data.'); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const fetchExploreData = () => { | ||||||||||||||||||||||||||||
const exploreUrlParams = getParsedExploreURLParams(); | ||||||||||||||||||||||||||||
return makeApi<{}, ExploreResponsePayload>({ | ||||||||||||||||||||||||||||
const fetchExploreData = (exploreUrlParams: URLSearchParams) => | ||||||||||||||||||||||||||||
makeApi<{}, ExploreResponsePayload>({ | ||||||||||||||||||||||||||||
method: 'GET', | ||||||||||||||||||||||||||||
endpoint: 'api/v1/explore/', | ||||||||||||||||||||||||||||
})(exploreUrlParams); | ||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const ExplorePage = () => { | ||||||||||||||||||||||||||||
const [isLoaded, setIsLoaded] = useState(false); | ||||||||||||||||||||||||||||
const isExploreInitialized = useRef(false); | ||||||||||||||||||||||||||||
const dispatch = useDispatch(); | ||||||||||||||||||||||||||||
const location = useLocation(); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
useEffect(() => { | ||||||||||||||||||||||||||||
fetchExploreData() | ||||||||||||||||||||||||||||
.then(({ result }) => { | ||||||||||||||||||||||||||||
if (isNullish(result.dataset?.id) && isNullish(result.dataset?.uid)) { | ||||||||||||||||||||||||||||
const exploreUrlParams = getParsedExploreURLParams(location); | ||||||||||||||||||||||||||||
const isSaveAction = !!getUrlParam(URL_PARAMS.saveAction); | ||||||||||||||||||||||||||||
if (!isExploreInitialized.current || isSaveAction) { | ||||||||||||||||||||||||||||
fetchExploreData(exploreUrlParams) | ||||||||||||||||||||||||||||
.then(({ result }) => { | ||||||||||||||||||||||||||||
if (isNullish(result.dataset?.id) && isNullish(result.dataset?.uid)) { | ||||||||||||||||||||||||||||
dispatch(hydrateExplore(fallbackExploreInitialData)); | ||||||||||||||||||||||||||||
dispatch(addDangerToast(loadErrorMessage)); | ||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||
dispatch(hydrateExplore(result)); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
Comment on lines
+54
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nits, if
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @zhaoyongjie @kgabryje Not sure I totally understand this suggestion – is there an advantage to using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same logic, only optimized for readability. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But doesn't your suggested logic have a different effect? Or were you suggesting the logic should be different? |
||||||||||||||||||||||||||||
.catch(() => { | ||||||||||||||||||||||||||||
dispatch(hydrateExplore(fallbackExploreInitialData)); | ||||||||||||||||||||||||||||
dispatch(addDangerToast(loadErrorMessage)); | ||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||
dispatch(hydrateExplore(result)); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
.catch(() => { | ||||||||||||||||||||||||||||
dispatch(hydrateExplore(fallbackExploreInitialData)); | ||||||||||||||||||||||||||||
dispatch(addDangerToast(loadErrorMessage)); | ||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
.finally(() => { | ||||||||||||||||||||||||||||
setIsLoaded(true); | ||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
}, [dispatch]); | ||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
.finally(() => { | ||||||||||||||||||||||||||||
setIsLoaded(true); | ||||||||||||||||||||||||||||
isExploreInitialized.current = true; | ||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
}, [dispatch, location]); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
if (!isLoaded) { | ||||||||||||||||||||||||||||
return <Loading />; | ||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -16,8 +16,9 @@ | |||||||||||||||||||||||
* specific language governing permissions and limitations | ||||||||||||||||||||||||
* under the License. | ||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||
import { SupersetClient } from '@superset-ui/core'; | ||||||||||||||||||||||||
import { buildV1ChartDataPayload, getExploreUrl } from '../exploreUtils'; | ||||||||||||||||||||||||
import { SupersetClient, t } from '@superset-ui/core'; | ||||||||||||||||||||||||
import { addSuccessToast } from 'src/components/MessageToasts/actions'; | ||||||||||||||||||||||||
import { buildV1ChartDataPayload } from '../exploreUtils'; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
export const FETCH_DASHBOARDS_SUCCEEDED = 'FETCH_DASHBOARDS_SUCCEEDED'; | ||||||||||||||||||||||||
export function fetchDashboardsSucceeded(choices) { | ||||||||||||||||||||||||
|
@@ -60,36 +61,140 @@ export function removeSaveModalAlert() { | |||||||||||||||||||||||
return { type: REMOVE_SAVE_MODAL_ALERT }; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
export function saveSlice(formData, requestParams) { | ||||||||||||||||||||||||
return dispatch => { | ||||||||||||||||||||||||
let url = getExploreUrl({ | ||||||||||||||||||||||||
formData, | ||||||||||||||||||||||||
endpointType: 'base', | ||||||||||||||||||||||||
force: false, | ||||||||||||||||||||||||
curUrl: null, | ||||||||||||||||||||||||
requestParams, | ||||||||||||||||||||||||
export const getSlicePayload = ( | ||||||||||||||||||||||||
sliceName, | ||||||||||||||||||||||||
formDataWithNativeFilters, | ||||||||||||||||||||||||
owners, | ||||||||||||||||||||||||
) => { | ||||||||||||||||||||||||
const formData = { | ||||||||||||||||||||||||
...formDataWithNativeFilters, | ||||||||||||||||||||||||
adhoc_filters: formDataWithNativeFilters.adhoc_filters?.filter( | ||||||||||||||||||||||||
f => !f.isExtra, | ||||||||||||||||||||||||
), | ||||||||||||||||||||||||
}; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const [datasourceId, datasourceType] = formData.datasource.split('__'); | ||||||||||||||||||||||||
const payload = { | ||||||||||||||||||||||||
params: JSON.stringify(formData), | ||||||||||||||||||||||||
slice_name: sliceName, | ||||||||||||||||||||||||
viz_type: formData.viz_type, | ||||||||||||||||||||||||
datasource_id: parseInt(datasourceId, 10), | ||||||||||||||||||||||||
datasource_type: datasourceType, | ||||||||||||||||||||||||
dashboards: formData.dashboards, | ||||||||||||||||||||||||
owners, | ||||||||||||||||||||||||
query_context: JSON.stringify( | ||||||||||||||||||||||||
buildV1ChartDataPayload({ | ||||||||||||||||||||||||
formData, | ||||||||||||||||||||||||
force: false, | ||||||||||||||||||||||||
resultFormat: 'json', | ||||||||||||||||||||||||
resultType: 'full', | ||||||||||||||||||||||||
setDataMask: null, | ||||||||||||||||||||||||
ownState: null, | ||||||||||||||||||||||||
}), | ||||||||||||||||||||||||
), | ||||||||||||||||||||||||
Comment on lines
+85
to
+94
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nits:
Suggested change
One thing is strange, why does every JSON non-primitive value have to be serialized. i.e. We serialize every non-primitive JSON value. This may have some potential deserialization errors. When the backend engineer uses the payload, they must remember which param has been serialized. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure why non-primitives are serialized, I was mostly just keeping what was there before and I confess I don't really understand what's happening with all of these function calls that generate the payload. Are you sure it's safe to leave out all of those params? |
||||||||||||||||||||||||
}; | ||||||||||||||||||||||||
return payload; | ||||||||||||||||||||||||
}; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const addToasts = (isNewSlice, sliceName, addedToDashboard) => { | ||||||||||||||||||||||||
const toasts = []; | ||||||||||||||||||||||||
if (isNewSlice) { | ||||||||||||||||||||||||
toasts.push(addSuccessToast(t('Chart [%s] has been saved', sliceName))); | ||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||
toasts.push( | ||||||||||||||||||||||||
addSuccessToast(t('Chart [%s] has been overwritten', sliceName)), | ||||||||||||||||||||||||
); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
if (addedToDashboard) { | ||||||||||||||||||||||||
if (addedToDashboard.new) { | ||||||||||||||||||||||||
toasts.push( | ||||||||||||||||||||||||
addSuccessToast( | ||||||||||||||||||||||||
t( | ||||||||||||||||||||||||
'Dashboard [%s] just got created and chart [%s] was added to it', | ||||||||||||||||||||||||
addedToDashboard.title, | ||||||||||||||||||||||||
sliceName, | ||||||||||||||||||||||||
), | ||||||||||||||||||||||||
), | ||||||||||||||||||||||||
); | ||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||
toasts.push( | ||||||||||||||||||||||||
addSuccessToast( | ||||||||||||||||||||||||
t( | ||||||||||||||||||||||||
'Chart [%s] was added to dashboard [%s]', | ||||||||||||||||||||||||
sliceName, | ||||||||||||||||||||||||
addedToDashboard.title, | ||||||||||||||||||||||||
), | ||||||||||||||||||||||||
), | ||||||||||||||||||||||||
); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
return toasts; | ||||||||||||||||||||||||
}; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
// Update existing slice | ||||||||||||||||||||||||
export const updateSlice = | ||||||||||||||||||||||||
codyml marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||
({ slice_id: sliceId, owners }, sliceName, formData, addedToDashboard) => | ||||||||||||||||||||||||
async dispatch => { | ||||||||||||||||||||||||
try { | ||||||||||||||||||||||||
const response = await SupersetClient.put({ | ||||||||||||||||||||||||
endpoint: `/api/v1/chart/${sliceId}`, | ||||||||||||||||||||||||
jsonPayload: getSlicePayload(sliceName, formData, owners), | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
dispatch(saveSliceSuccess()); | ||||||||||||||||||||||||
addToasts(false, sliceName, addedToDashboard).map(dispatch); | ||||||||||||||||||||||||
return response.json; | ||||||||||||||||||||||||
} catch (error) { | ||||||||||||||||||||||||
dispatch(saveSliceFailed()); | ||||||||||||||||||||||||
throw error; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
}; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
// Create new slice | ||||||||||||||||||||||||
export const createSlice = | ||||||||||||||||||||||||
(sliceName, formData, addedToDashboard) => async dispatch => { | ||||||||||||||||||||||||
try { | ||||||||||||||||||||||||
const response = await SupersetClient.post({ | ||||||||||||||||||||||||
endpoint: `/api/v1/chart/`, | ||||||||||||||||||||||||
jsonPayload: getSlicePayload(sliceName, formData), | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
dispatch(saveSliceSuccess()); | ||||||||||||||||||||||||
addToasts(true, sliceName, addedToDashboard).map(dispatch); | ||||||||||||||||||||||||
return response.json; | ||||||||||||||||||||||||
} catch (error) { | ||||||||||||||||||||||||
dispatch(saveSliceFailed()); | ||||||||||||||||||||||||
throw error; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
}; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
// Create new dashboard | ||||||||||||||||||||||||
export const createDashboard = dashboardName => async dispatch => { | ||||||||||||||||||||||||
try { | ||||||||||||||||||||||||
const response = await SupersetClient.post({ | ||||||||||||||||||||||||
endpoint: `/api/v1/dashboard/`, | ||||||||||||||||||||||||
jsonPayload: { dashboard_title: dashboardName }, | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
// TODO: This will be removed in the next PR that will change the logic to save a slice | ||||||||||||||||||||||||
url = url.replace('/explore', '/superset/explore'); | ||||||||||||||||||||||||
return response.json; | ||||||||||||||||||||||||
} catch (error) { | ||||||||||||||||||||||||
dispatch(saveSliceFailed()); | ||||||||||||||||||||||||
throw error; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
}; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
// Save the query context so we can re-generate the data from Python | ||||||||||||||||||||||||
// for alerts and reports | ||||||||||||||||||||||||
const queryContext = buildV1ChartDataPayload({ | ||||||||||||||||||||||||
formData, | ||||||||||||||||||||||||
force: false, | ||||||||||||||||||||||||
resultFormat: 'json', | ||||||||||||||||||||||||
resultType: 'full', | ||||||||||||||||||||||||
// Get existing dashboard from ID | ||||||||||||||||||||||||
export const getDashboard = dashboardId => async dispatch => { | ||||||||||||||||||||||||
try { | ||||||||||||||||||||||||
const response = await SupersetClient.get({ | ||||||||||||||||||||||||
endpoint: `/api/v1/dashboard/${dashboardId}`, | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
return SupersetClient.post({ | ||||||||||||||||||||||||
url, | ||||||||||||||||||||||||
postPayload: { form_data: formData, query_context: queryContext }, | ||||||||||||||||||||||||
}) | ||||||||||||||||||||||||
.then(response => { | ||||||||||||||||||||||||
dispatch(saveSliceSuccess(response.json)); | ||||||||||||||||||||||||
return response.json; | ||||||||||||||||||||||||
}) | ||||||||||||||||||||||||
.catch(() => dispatch(saveSliceFailed())); | ||||||||||||||||||||||||
}; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
return response.json; | ||||||||||||||||||||||||
} catch (error) { | ||||||||||||||||||||||||
dispatch(saveSliceFailed()); | ||||||||||||||||||||||||
throw error; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a util is
isDefined
in the@superset-ui/core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kgabryje I think this was your code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I wasn't aware of
isDefined
. Feel free to refactor 🙂