msyavuz commented on code in PR #33831:
URL: https://github.com/apache/superset/pull/33831#discussion_r2336957848
##########
pyproject.toml:
##########
@@ -35,8 +35,7 @@ classifiers = [
"Programming Language :: Python :: 3.12",
]
dependencies = [
- # no bounds for apache-superset-core until we have a stable version
Review Comment:
Do we have a stable version?
##########
superset-frontend/src/dashboard/actions/dashboardInfo.ts:
##########
@@ -26,8 +27,66 @@ import {
GlobalChartCrossFilterConfig,
RootState,
} from 'src/dashboard/types';
+import {
+ ChartCustomizationItem,
+ FilterOption,
+} from 'src/dashboard/components/nativeFilters/ChartCustomization/types';
+import { triggerQuery } from 'src/components/Chart/chartAction';
+import { removeDataMask, updateDataMask } from 'src/dataMask/actions';
import { onSave } from './dashboardState';
+const createUpdateDashboardApi = (id: number | string | undefined) => {
Review Comment:
Can id be anything else than number here?
##########
superset-frontend/src/dashboard/actions/dashboardInfo.ts:
##########
@@ -142,43 +244,351 @@ export function saveFilterBarOrientation(orientation:
FilterBarOrientation) {
dispatch(onSave(lastModifiedTime));
}
} catch (errorObject) {
- const errorText = await getErrorText(errorObject, 'dashboard');
- dispatch(addDangerToast(errorText));
+ const { error } = await getClientErrorObject(errorObject);
+ dispatch(
+ addDangerToast(
+ t(
+ 'Sorry, there was an error saving this dashboard: %s',
+ error || 'Bad Request',
+ ),
+ ),
+ );
throw errorObject;
}
};
}
export function saveCrossFiltersSetting(crossFiltersEnabled: boolean) {
- return async (dispatch: Dispatch, getState: () => RootState) => {
+ return async function saveCrossFiltersSettingThunk(
+ dispatch: Dispatch,
+ getState: () => RootState,
+ ) {
const { id, metadata } = getState().dashboardInfo;
- const updateDashboard = makeApi<
- Partial<DashboardInfo>,
- { result: Partial<DashboardInfo>; last_modified_time: number }
- >({
- method: 'PUT',
- endpoint: `/api/v1/dashboard/${id}`,
- });
+ dispatch(setCrossFiltersEnabled(crossFiltersEnabled));
+ const updateDashboard = createUpdateDashboardApi(id);
+
try {
const response = await updateDashboard({
json_metadata: JSON.stringify({
...metadata,
cross_filters_enabled: crossFiltersEnabled,
}),
});
- const updatedDashboard = response.result;
- const lastModifiedTime = response.last_modified_time;
Review Comment:
Don't we need this anymore?
##########
superset-frontend/src/dashboard/actions/dashboardInfo.ts:
##########
@@ -142,43 +244,351 @@ export function saveFilterBarOrientation(orientation:
FilterBarOrientation) {
dispatch(onSave(lastModifiedTime));
}
} catch (errorObject) {
- const errorText = await getErrorText(errorObject, 'dashboard');
- dispatch(addDangerToast(errorText));
+ const { error } = await getClientErrorObject(errorObject);
+ dispatch(
+ addDangerToast(
+ t(
+ 'Sorry, there was an error saving this dashboard: %s',
+ error || 'Bad Request',
+ ),
+ ),
+ );
throw errorObject;
}
};
}
export function saveCrossFiltersSetting(crossFiltersEnabled: boolean) {
- return async (dispatch: Dispatch, getState: () => RootState) => {
+ return async function saveCrossFiltersSettingThunk(
+ dispatch: Dispatch,
+ getState: () => RootState,
+ ) {
const { id, metadata } = getState().dashboardInfo;
- const updateDashboard = makeApi<
- Partial<DashboardInfo>,
- { result: Partial<DashboardInfo>; last_modified_time: number }
- >({
- method: 'PUT',
- endpoint: `/api/v1/dashboard/${id}`,
- });
+ dispatch(setCrossFiltersEnabled(crossFiltersEnabled));
+ const updateDashboard = createUpdateDashboardApi(id);
+
try {
const response = await updateDashboard({
json_metadata: JSON.stringify({
...metadata,
cross_filters_enabled: crossFiltersEnabled,
}),
});
- const updatedDashboard = response.result;
- const lastModifiedTime = response.last_modified_time;
- if (updatedDashboard.json_metadata) {
- const metadata = JSON.parse(updatedDashboard.json_metadata);
- dispatch(setCrossFiltersEnabled(metadata.cross_filters_enabled));
+ dispatch(
+ dashboardInfoChanged({
+ metadata: JSON.parse(response.result.json_metadata || '{}'),
+ }),
+ );
+ return response;
+ } catch (err) {
+ dispatch(addDangerToast(t('Failed to save cross-filters setting')));
+ throw err;
+ }
+ };
+}
+
+export function saveChartCustomization(
+ chartCustomizationItems: ChartCustomizationSavePayload[],
+): ThunkAction<
+ Promise<{ result: Partial<DashboardInfo>; last_modified_time: number }>,
+ RootState,
+ null,
+ AnyAction
+> {
+ return async function (
+ dispatch: ThunkDispatch<RootState, null, AnyAction>,
+ getState: () => RootState,
+ ) {
+ const { id, metadata, json_metadata } = getState().dashboardInfo;
+
+ const currentState = getState();
+ const currentChartCustomizationItems =
+ currentState.dashboardInfo.metadata?.chart_customization_config || [];
+
+ const existingItemsMap = new Map(
+ currentChartCustomizationItems.map(item => [item.id, item]),
+ );
+
+ const updatedItemsMap = new Map(existingItemsMap);
+
+ chartCustomizationItems.forEach(newItem => {
+ if (newItem.removed) {
+ updatedItemsMap.delete(newItem.id);
+ } else {
+ const chartCustomizationItem: ChartCustomizationItem = {
+ id: newItem.id,
+ title: newItem.title,
+ removed: newItem.removed,
+ chartId: newItem.chartId,
+ customization: newItem.customization,
+ };
+ updatedItemsMap.set(newItem.id, chartCustomizationItem);
+ }
+ });
+
+ const simpleItems = Array.from(updatedItemsMap.values());
+
+ dispatch(setChartCustomization(simpleItems));
+
+ const removedItems = currentChartCustomizationItems.filter(
+ existingItem => !updatedItemsMap.has(existingItem.id),
+ );
+
+ removedItems.forEach(removedItem => {
+ const customizationFilterId = `chart_customization_${removedItem.id}`;
+ dispatch(removeDataMask(customizationFilterId));
+ });
+
+ simpleItems.forEach(item => {
+ const customizationFilterId = `chart_customization_${item.id}`;
+
+ if (item.customization?.column) {
+ const existingDataMask = getState().dataMask[customizationFilterId];
+
+ const existingFilterState = existingDataMask?.filterState;
+
+ dispatch(removeDataMask(customizationFilterId));
+
+ const dataMask = {
+ extraFormData: {},
+ filterState: {
+ value:
+ existingFilterState?.value ||
+ item.customization?.defaultDataMask?.filterState?.value ||
+ [],
+ },
+ ownState: {
+ column: item.customization.column,
+ },
+ };
+
+ dispatch(updateDataMask(customizationFilterId, dataMask));
+ } else {
+ dispatch(removeDataMask(customizationFilterId));
}
+ });
+
+ const updateDashboard = createUpdateDashboardApi(id);
+
+ try {
+ let parsedMetadata: any = {};
+ try {
+ parsedMetadata = json_metadata ? JSON.parse(json_metadata) : metadata;
+ } catch (e) {
+ console.error('Error parsing json_metadata:', e);
+ parsedMetadata = metadata || {};
+ }
+
+ const updatedMetadata = {
+ ...parsedMetadata,
+ native_filter_configuration: (
+ parsedMetadata.native_filter_configuration || []
+ ).filter(
+ (item: any) =>
+ !(
+ item.type === 'CHART_CUSTOMIZATION' &&
+ item.id === 'chart_customization_groupby'
+ ),
+ ),
+ chart_customization_config: simpleItems,
+ };
+
+ const response = await updateDashboard({
+ json_metadata: JSON.stringify(updatedMetadata),
+ });
+
+ const lastModifiedTime = response.last_modified_time;
+
if (lastModifiedTime) {
dispatch(onSave(lastModifiedTime));
}
+
+ const { dashboardState } = getState();
+ const chartIds = dashboardState.sliceIds || [];
+ if (chartIds.length > 0) {
+ chartIds.forEach(chartId => {
+ dispatch(triggerQuery(true, chartId));
+ });
+ }
+
+ return response;
} catch (errorObject) {
- const errorText = await getErrorText(errorObject, 'dashboard');
- dispatch(addDangerToast(errorText));
+ const { error } = await getClientErrorObject(errorObject);
+ dispatch(
+ addDangerToast(error || t('Failed to save chart customization')),
+ );
throw errorObject;
}
};
}
+
+export const INITIALIZE_CHART_CUSTOMIZATION = 'INITIALIZE_CHART_CUSTOMIZATION';
+export interface InitializeChartCustomization {
+ type: typeof INITIALIZE_CHART_CUSTOMIZATION;
+ chartCustomizationItems: ChartCustomizationItem[];
+}
+
+export function initializeChartCustomization(
+ chartCustomizationItems: ChartCustomizationItem[],
+): ThunkAction<void, RootState, null, AnyAction> {
+ return (dispatch: ThunkDispatch<RootState, null, AnyAction>) => {
+ dispatch({
+ type: INITIALIZE_CHART_CUSTOMIZATION,
+ chartCustomizationItems,
+ });
+
+ chartCustomizationItems.forEach(item => {
+ const customizationFilterId = `chart_customization_${item.id}`;
+
+ if (item.customization?.column) {
+ dispatch(removeDataMask(customizationFilterId));
+
+ const dataMask = {
+ extraFormData: {},
+ filterState: {
+ value:
+ item.customization?.defaultDataMask?.filterState?.value || [],
+ },
+ ownState: {
+ column: item.customization.column,
+ },
+ };
+ dispatch(updateDataMask(customizationFilterId, dataMask));
+ } else {
+ dispatch(removeDataMask(customizationFilterId));
+ }
+ });
+ };
+}
+
+export const SET_CHART_CUSTOMIZATION_DATA_LOADING =
+ 'SET_CHART_CUSTOMIZATION_DATA_LOADING';
+export interface SetChartCustomizationDataLoading {
+ type: typeof SET_CHART_CUSTOMIZATION_DATA_LOADING;
+ itemId: string;
+ isLoading: boolean;
+}
+
+export function setChartCustomizationDataLoading(
+ itemId: string,
+ isLoading: boolean,
+): SetChartCustomizationDataLoading {
+ return {
+ type: SET_CHART_CUSTOMIZATION_DATA_LOADING,
+ itemId,
+ isLoading,
+ };
+}
+
+export const SET_CHART_CUSTOMIZATION_DATA = 'SET_CHART_CUSTOMIZATION_DATA';
+export interface SetChartCustomizationData {
+ type: typeof SET_CHART_CUSTOMIZATION_DATA;
+ itemId: string;
+ data: FilterOption[];
+}
+
+export function setChartCustomizationData(
+ itemId: string,
+ data: FilterOption[],
+): SetChartCustomizationData {
+ return {
+ type: SET_CHART_CUSTOMIZATION_DATA,
+ itemId,
+ data,
+ };
+}
+
+export function loadChartCustomizationData(
+ itemId: string,
+ datasetId: string,
+ columnName: string | string[],
+): ThunkAction<Promise<void>, RootState, null, AnyAction> {
+ return async (dispatch: ThunkDispatch<RootState, null, AnyAction>) => {
+ if (!datasetId || !columnName) {
+ return;
+ }
+
+ const actualColumnName = Array.isArray(columnName)
+ ? columnName[0]
+ : columnName;
+
+ if (!actualColumnName) {
+ return;
+ }
+
+ dispatch(setChartCustomizationDataLoading(itemId, false));
+ };
+}
+
+export const SET_PENDING_CHART_CUSTOMIZATION =
+ 'SET_PENDING_CHART_CUSTOMIZATION';
+export interface SetPendingChartCustomization {
+ type: typeof SET_PENDING_CHART_CUSTOMIZATION;
+ pendingCustomization: ChartCustomizationSavePayload;
+}
+
+export function setPendingChartCustomization(
+ pendingCustomization: ChartCustomizationSavePayload,
+): SetPendingChartCustomization {
+ return {
+ type: SET_PENDING_CHART_CUSTOMIZATION,
+ pendingCustomization,
+ };
+}
+
+export const CLEAR_PENDING_CHART_CUSTOMIZATION =
+ 'CLEAR_PENDING_CHART_CUSTOMIZATION';
+export interface ClearPendingChartCustomization {
+ type: typeof CLEAR_PENDING_CHART_CUSTOMIZATION;
+ itemId: string;
+}
+
+export function clearPendingChartCustomization(
+ itemId: string,
+): ClearPendingChartCustomization {
+ return {
+ type: CLEAR_PENDING_CHART_CUSTOMIZATION,
+ itemId,
+ };
+}
+
+export const CLEAR_ALL_PENDING_CHART_CUSTOMIZATIONS =
+ 'CLEAR_ALL_PENDING_CHART_CUSTOMIZATIONS';
+export interface ClearAllPendingChartCustomizations {
+ type: typeof CLEAR_ALL_PENDING_CHART_CUSTOMIZATIONS;
+}
+
+export function clearAllPendingChartCustomizations():
ClearAllPendingChartCustomizations {
+ return {
+ type: CLEAR_ALL_PENDING_CHART_CUSTOMIZATIONS,
+ };
+}
+
+export const CLEAR_ALL_CHART_CUSTOMIZATIONS = 'CLEAR_ALL_CHART_CUSTOMIZATIONS';
+export interface ClearAllChartCustomizations {
+ type: typeof CLEAR_ALL_CHART_CUSTOMIZATIONS;
+}
+
+export function clearAllChartCustomizations(): ClearAllChartCustomizations {
+ return {
+ type: CLEAR_ALL_CHART_CUSTOMIZATIONS,
+ };
+}
+
+export function clearAllChartCustomizationsFromMetadata() {
+ return clearAllChartCustomizations();
+}
+
+export type AnyDashboardInfoAction =
+ | ReturnType<typeof dashboardInfoChanged>
+ | ReturnType<typeof nativeFiltersConfigChanged>
+ | ReturnType<typeof setFilterBarOrientation>
+ | ReturnType<typeof setCrossFiltersEnabled>
+ | ReturnType<typeof setChartCustomization>
+ | InitializeChartCustomization
+ | SetChartCustomizationDataLoading
+ | SetChartCustomizationData
+ | SetPendingChartCustomization
+ | ClearPendingChartCustomization
+ | ClearAllPendingChartCustomizations
+ | ClearAllChartCustomizations;
Review Comment:
This file might need splitting up into filterActions etc.
##########
superset-frontend/src/dashboard/actions/dashboardInfo.ts:
##########
@@ -142,43 +244,351 @@ export function saveFilterBarOrientation(orientation:
FilterBarOrientation) {
dispatch(onSave(lastModifiedTime));
}
} catch (errorObject) {
- const errorText = await getErrorText(errorObject, 'dashboard');
- dispatch(addDangerToast(errorText));
+ const { error } = await getClientErrorObject(errorObject);
+ dispatch(
+ addDangerToast(
+ t(
+ 'Sorry, there was an error saving this dashboard: %s',
+ error || 'Bad Request',
+ ),
+ ),
+ );
throw errorObject;
}
};
}
export function saveCrossFiltersSetting(crossFiltersEnabled: boolean) {
- return async (dispatch: Dispatch, getState: () => RootState) => {
+ return async function saveCrossFiltersSettingThunk(
Review Comment:
Nice! I like the named function here
##########
superset-frontend/src/dashboard/actions/dashboardInfo.ts:
##########
@@ -113,16 +165,66 @@ export function
setCrossFiltersEnabled(crossFiltersEnabled: boolean) {
return { type: SET_CROSS_FILTERS_ENABLED, crossFiltersEnabled };
}
+export const SAVE_CHART_CUSTOMIZATION_COMPLETE =
+ 'SAVE_CHART_CUSTOMIZATION_COMPLETE';
+
+export function setChartCustomization(
+ chartCustomization: ChartCustomizationItem[],
+) {
+ return { type: SAVE_CHART_CUSTOMIZATION_COMPLETE, chartCustomization };
+}
+
+export const SET_DASHBOARD_THEME = 'SET_DASHBOARD_THEME';
+
+export function setDashboardTheme(theme: { id: number; name: string } | null) {
+ return { type: SET_DASHBOARD_THEME, theme };
+}
+
+export function updateDashboardTheme(themeId: number | null) {
+ return async (dispatch: Dispatch, getState: () => RootState) => {
+ const { id } = getState().dashboardInfo;
+ const updateDashboard = createUpdateDashboardApi(id);
+
+ try {
+ const response = await updateDashboard({
+ theme_id: themeId,
+ });
+
+ // Update the dashboard info with the new theme
+ if (themeId === null) {
+ // Clearing the theme
+ dispatch(setDashboardTheme(null));
+ } else if (response.result.theme) {
+ // API returned the theme object
+ dispatch(setDashboardTheme(response.result.theme));
+ } else {
+ // API didn't return theme object, create it from the themeId
+ dispatch(setDashboardTheme({ id: themeId, name: `Theme ${themeId}` }));
+ }
+
+ const lastModifiedTime = response.last_modified_time;
+ if (lastModifiedTime) {
+ dispatch(onSave(lastModifiedTime));
+ }
+ } catch (errorObject) {
+ const { error } = await getClientErrorObject(errorObject);
+ dispatch(
+ addDangerToast(
+ t(
+ 'Sorry, there was an error saving this dashboard: %s',
Review Comment:
I am a bit lost on what this is and where it's used. Also seems out of scope
for the pr?
##########
superset-frontend/src/dashboard/actions/dashboardInfo.ts:
##########
@@ -26,8 +27,66 @@ import {
GlobalChartCrossFilterConfig,
RootState,
} from 'src/dashboard/types';
+import {
+ ChartCustomizationItem,
+ FilterOption,
+} from 'src/dashboard/components/nativeFilters/ChartCustomization/types';
+import { triggerQuery } from 'src/components/Chart/chartAction';
+import { removeDataMask, updateDataMask } from 'src/dataMask/actions';
import { onSave } from './dashboardState';
+const createUpdateDashboardApi = (id: number | string | undefined) => {
+ if (!id) {
+ throw new Error('Dashboard ID is required');
+ }
+ return makeApi<
+ Partial<DashboardInfo>,
+ { result: Partial<DashboardInfo>; last_modified_time: number }
+ >({
+ method: 'PUT',
+ endpoint: `/api/v1/dashboard/${id}`,
+ });
+};
+
+export interface ChartCustomizationSavePayload {
+ id: string;
+ title?: string;
+ description?: string;
+ removed?: boolean;
+ chartId?: number;
+ customization: {
+ name: string;
+ dataset:
+ | string
+ | number
+ | {
+ value: string | number;
+ label?: string;
+ table_name?: string;
+ schema?: string;
+ }
+ | null;
+ datasetInfo?: {
+ label: string;
+ value: number;
+ table_name: string;
+ };
+ column: string | string[] | null;
+ description?: string;
+ sortFilter?: boolean;
+ sortAscending?: boolean;
+ sortMetric?: string;
+ hasDefaultValue?: boolean;
+ defaultValue?: string;
+ isRequired?: boolean;
+ selectFirst?: boolean;
+ defaultDataMask?: any;
+ defaultValueQueriesData?: any;
Review Comment:
Don't we have better types for these?
##########
superset-frontend/src/dashboard/actions/dashboardInfo.ts:
##########
@@ -26,8 +27,66 @@ import {
GlobalChartCrossFilterConfig,
RootState,
} from 'src/dashboard/types';
+import {
+ ChartCustomizationItem,
+ FilterOption,
+} from 'src/dashboard/components/nativeFilters/ChartCustomization/types';
+import { triggerQuery } from 'src/components/Chart/chartAction';
+import { removeDataMask, updateDataMask } from 'src/dataMask/actions';
import { onSave } from './dashboardState';
+const createUpdateDashboardApi = (id: number | string | undefined) => {
+ if (!id) {
+ throw new Error('Dashboard ID is required');
+ }
Review Comment:
Related to the previous point can the id be nullish in runtime?
##########
superset-frontend/src/dashboard/actions/dashboardInfo.ts:
##########
@@ -142,43 +244,351 @@ export function saveFilterBarOrientation(orientation:
FilterBarOrientation) {
dispatch(onSave(lastModifiedTime));
}
} catch (errorObject) {
- const errorText = await getErrorText(errorObject, 'dashboard');
- dispatch(addDangerToast(errorText));
+ const { error } = await getClientErrorObject(errorObject);
+ dispatch(
+ addDangerToast(
+ t(
+ 'Sorry, there was an error saving this dashboard: %s',
+ error || 'Bad Request',
+ ),
+ ),
+ );
throw errorObject;
}
};
}
export function saveCrossFiltersSetting(crossFiltersEnabled: boolean) {
- return async (dispatch: Dispatch, getState: () => RootState) => {
+ return async function saveCrossFiltersSettingThunk(
+ dispatch: Dispatch,
+ getState: () => RootState,
+ ) {
const { id, metadata } = getState().dashboardInfo;
- const updateDashboard = makeApi<
- Partial<DashboardInfo>,
- { result: Partial<DashboardInfo>; last_modified_time: number }
- >({
- method: 'PUT',
- endpoint: `/api/v1/dashboard/${id}`,
- });
+ dispatch(setCrossFiltersEnabled(crossFiltersEnabled));
Review Comment:
So we are doing optimistic update here but there is no rollback if the api
fails.
--
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]
