From a98331971509f20893c3363505a3c31fc4355b18 Mon Sep 17 00:00:00 2001 From: Diego Pucci Date: Tue, 10 Dec 2024 14:12:34 +0200 Subject: [PATCH] fix(Dashboard): Sync color configuration via dedicated endpoint --- .../src/dashboard/actions/dashboardState.js | 253 ++++++++++-------- .../components/PropertiesModal/index.tsx | 4 +- superset-frontend/src/utils/colorScheme.ts | 62 +++-- superset/commands/dashboard/exceptions.py | 4 + superset/commands/dashboard/update.py | 27 +- superset/constants.py | 1 + superset/daos/dashboard.py | 18 ++ superset/dashboards/api.py | 94 +++++++ superset/dashboards/schemas.py | 9 + .../integration_tests/dashboards/api_tests.py | 111 ++++++++ 10 files changed, 453 insertions(+), 130 deletions(-) diff --git a/superset-frontend/src/dashboard/actions/dashboardState.js b/superset-frontend/src/dashboard/actions/dashboardState.js index 600098526bbe0..7e00baee4f705 100644 --- a/superset-frontend/src/dashboard/actions/dashboardState.js +++ b/superset-frontend/src/dashboard/actions/dashboardState.js @@ -56,7 +56,7 @@ import { getActiveFilters } from 'src/dashboard/util/activeDashboardFilters'; import { safeStringify } from 'src/utils/safeStringify'; import { logEvent } from 'src/logger/actions'; import { LOG_ACTIONS_CONFIRM_OVERWRITE_DASHBOARD_METADATA } from 'src/logger/LogUtils'; -import { isEqual } from 'lodash'; +import { isEqual, sortBy } from 'lodash'; import { UPDATE_COMPONENTS_PARENTS_LIST } from './dashboardLayout'; import { saveChartConfiguration, @@ -73,8 +73,9 @@ import { isLabelsColorMapSynced, getColorSchemeDomain, getColorNamespace, - getLabelsColorMapEntries, + getFreshLabelsColorMapEntries, getFreshSharedLabels, + getDynamicLabelsColors, } from '../../utils/colorScheme'; export const SET_UNSAVED_CHANGES = 'SET_UNSAVED_CHANGES'; @@ -253,15 +254,18 @@ export function setDashboardSharedLabelsColorsSynced() { return { type: SET_DASHBOARD_SHARED_LABELS_COLORS_SYNCED }; } -export const setDashboardMetadata = updatedMetadata => async dispatch => { - dispatch( - dashboardInfoChanged({ - metadata: { - ...updatedMetadata, - }, - }), - ); -}; +export const setDashboardMetadata = + updatedMetadata => async (dispatch, getState) => { + const { dashboardInfo } = getState(); + dispatch( + dashboardInfoChanged({ + metadata: { + ...dashboardInfo.metadata, + ...updatedMetadata, + }, + }), + ); + }; export function saveDashboardRequest(data, id, saveType) { return (dispatch, getState) => { @@ -320,7 +324,7 @@ export function saveDashboardRequest(data, id, saveType) { expanded_slices: data.metadata?.expanded_slices || {}, label_colors: customLabelsColor, shared_label_colors: getFreshSharedLabels(sharedLabelsColor), - map_label_colors: getLabelsColorMapEntries(customLabelsColor), + map_label_colors: getFreshLabelsColorMapEntries(customLabelsColor), refresh_frequency: data.metadata?.refresh_frequency || 0, timed_refresh_immune_slices: data.metadata?.timed_refresh_immune_slices || [], @@ -719,11 +723,18 @@ export function setDatasetsStatus(status) { }; } -const storeDashboardMetadata = async (id, metadata) => +const storeDashboardColorConfig = async (id, metadata) => SupersetClient.put({ - endpoint: `/api/v1/dashboard/${id}`, + endpoint: `/api/v1/dashboard/${id}/colors?mark_updated=false`, headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ json_metadata: JSON.stringify(metadata) }), + body: JSON.stringify({ + color_namespace: metadata.color_namespace, + color_scheme: metadata.color_scheme, + color_scheme_domain: metadata.color_scheme_domain || [], + shared_label_colors: metadata.shared_label_colors || [], + map_label_colors: metadata.map_label_colors || {}, + label_colors: metadata.label_colors || {}, + }), }); /** @@ -742,7 +753,7 @@ export const persistDashboardLabelsColor = () => async (dispatch, getState) => { if (labelsColorMapMustSync || sharedLabelsColorsMustSync) { dispatch(setDashboardLabelsColorMapSynced()); dispatch(setDashboardSharedLabelsColorsSynced()); - storeDashboardMetadata(id, metadata); + storeDashboardColorConfig(id, metadata); } }; @@ -756,7 +767,6 @@ export const persistDashboardLabelsColor = () => async (dispatch, getState) => { */ export const applyDashboardLabelsColorOnLoad = metadata => async dispatch => { try { - const updatedMetadata = { ...metadata }; const customLabelsColor = metadata.label_colors || {}; let hasChanged = false; @@ -764,11 +774,14 @@ export const applyDashboardLabelsColorOnLoad = metadata => async dispatch => { const sharedLabels = metadata.shared_label_colors || []; if (!Array.isArray(sharedLabels) && Object.keys(sharedLabels).length > 0) { hasChanged = true; - updatedMetadata.shared_label_colors = []; + dispatch( + setDashboardMetadata({ + shared_label_colors: [], + }), + ); } // backward compatibility of map_label_colors - const hasMapLabelColors = - Object.keys(metadata.map_label_colors || {}).length > 0; + const hasMapLabelColors = !!metadata.map_label_colors; let updatedScheme = metadata.color_scheme; const categoricalSchemes = getCategoricalSchemeRegistry(); @@ -780,11 +793,14 @@ export const applyDashboardLabelsColorOnLoad = metadata => async dispatch => { const defaultScheme = categoricalSchemes.defaultKey; const fallbackScheme = defaultScheme?.toString() || 'supersetColors'; hasChanged = true; - updatedScheme = fallbackScheme; - updatedMetadata.color_scheme = updatedScheme; dispatch(setColorScheme(updatedScheme)); + dispatch( + setDashboardMetadata({ + color_scheme: updatedScheme, + }), + ); } // the stored color domain registry and fresh might differ at this point @@ -795,24 +811,28 @@ export const applyDashboardLabelsColorOnLoad = metadata => async dispatch => { if (!isEqual(freshColorSchemeDomain, currentColorSchemeDomain)) { hasChanged = true; - updatedMetadata.color_scheme_domain = freshColorSchemeDomain; + dispatch( + setDashboardMetadata({ + color_scheme_domain: freshColorSchemeDomain, + }), + ); } // if color scheme is invalid or map is missing, apply a fresh color map // if valid, apply the stored map to keep consistency across refreshes const shouldGoFresh = !hasMapLabelColors || hasInvalidColorScheme; - applyColors(updatedMetadata, shouldGoFresh); + applyColors(metadata, shouldGoFresh); if (shouldGoFresh) { - // a fresh color map has been applied - // needs to be stored for consistency hasChanged = true; - updatedMetadata.map_label_colors = - getLabelsColorMapEntries(customLabelsColor); + dispatch( + setDashboardMetadata({ + map_label_colors: getFreshLabelsColorMapEntries(customLabelsColor), + }), + ); } if (hasChanged) { - dispatch(setDashboardMetadata(updatedMetadata)); dispatch(setDashboardLabelsColorMapSync()); } } catch (e) { @@ -832,19 +852,28 @@ export const ensureSyncedLabelsColorMap = metadata => (dispatch, getState) => { const { dashboardState: { labelsColorMapMustSync }, } = getState(); - const updatedMetadata = { ...metadata }; const customLabelsColor = metadata.label_colors || {}; - const isMapSynced = isLabelsColorMapSynced(metadata); - const mustSync = !isMapSynced; - - if (mustSync) { - const freshestColorMapEntries = - getLabelsColorMapEntries(customLabelsColor); - updatedMetadata.map_label_colors = freshestColorMapEntries; - dispatch(setDashboardMetadata(updatedMetadata)); + const fullLabelsColors = getDynamicLabelsColors( + metadata.map_label_colors || {}, + customLabelsColor, + ); + const freshColorMapEntries = + getFreshLabelsColorMapEntries(customLabelsColor); + const isMapSynced = isLabelsColorMapSynced( + fullLabelsColors, + freshColorMapEntries, + customLabelsColor, + ); + + if (!isMapSynced) { + dispatch( + setDashboardMetadata({ + map_label_colors: freshColorMapEntries, + }), + ); } - if (mustSync && !labelsColorMapMustSync) { + if (!isMapSynced && !labelsColorMapMustSync) { // prepare to persist the just applied labels color map dispatch(setDashboardLabelsColorMapSync()); } @@ -867,7 +896,6 @@ export const ensureSyncedSharedLabelsColors = const { dashboardState: { sharedLabelsColorsMustSync }, } = getState(); - const updatedMetadata = { ...metadata }; const sharedLabelsColors = enforceSharedLabelsColorsArray( metadata.shared_label_colors, ); @@ -875,15 +903,17 @@ export const ensureSyncedSharedLabelsColors = forceFresh ? [] : sharedLabelsColors, ); const isSharedLabelsColorsSynced = isEqual( - sharedLabelsColors, - freshLabelsColors, + sortBy(sharedLabelsColors), + sortBy(freshLabelsColors), ); - const mustSync = !isSharedLabelsColorsSynced; if (mustSync) { - updatedMetadata.shared_label_colors = freshLabelsColors; - dispatch(setDashboardMetadata(updatedMetadata)); + dispatch( + setDashboardMetadata({ + shared_label_colors: freshLabelsColors, + }), + ); } if (mustSync && !sharedLabelsColorsMustSync) { @@ -901,77 +931,78 @@ export const ensureSyncedSharedLabelsColors = * @param {*} renderedChartIds - the charts that have finished rendering * @returns void */ -export const updateDashboardLabelsColor = - renderedChartIds => (dispatch, getState) => { - try { - const { - dashboardInfo: { metadata }, - charts, - } = getState(); - const colorScheme = metadata.color_scheme; - const labelsColorMapInstance = getLabelsColorMap(); - const fullLabelsColors = metadata.map_label_colors || {}; - const sharedLabelsColors = enforceSharedLabelsColorsArray( - metadata.shared_label_colors, - ); - const customLabelsColors = metadata.label_colors || {}; - - // for dashboards with no color scheme, the charts should always use their individual schemes - // this logic looks for unique labels (not shared across multiple charts) of each rendered chart - // it applies a new color to those unique labels when the applied scheme is not up to date - // while leaving shared label colors and custom label colors intact for color consistency - const shouldReset = []; - if (renderedChartIds.length > 0) { - const sharedLabelsSet = new Set(sharedLabelsColors); - renderedChartIds.forEach(id => { - const chart = charts[id]; - const formData = chart.form_data || chart.latestQueryFormData; - // ensure charts have their original color scheme always available - labelsColorMapInstance.setOwnColorScheme( - formData.slice_id, - formData.color_scheme, - ); +export const updateDashboardLabelsColor = renderedChartIds => (_, getState) => { + try { + const { + dashboardInfo: { metadata }, + charts, + } = getState(); + const colorScheme = metadata.color_scheme; + const labelsColorMapInstance = getLabelsColorMap(); + const sharedLabelsColors = enforceSharedLabelsColorsArray( + metadata.shared_label_colors, + ); + const customLabelsColors = metadata.label_colors || {}; + const fullLabelsColors = getDynamicLabelsColors( + metadata.map_label_colors || {}, + customLabelsColors, + ); - // if dashboard has a scheme, charts should ignore individual schemes - // thus following logic is inapplicable if a dashboard color scheme exists - if (colorScheme) return; + // for dashboards with no color scheme, the charts should always use their individual schemes + // this logic looks for unique labels (not shared across multiple charts) of each rendered chart + // it applies a new color to those unique labels when the applied scheme is not up to date + // while leaving shared label colors and custom label colors intact for color consistency + const shouldReset = []; + if (renderedChartIds.length > 0) { + const sharedLabelsSet = new Set(sharedLabelsColors); + renderedChartIds.forEach(id => { + const chart = charts[id]; + const formData = chart.form_data || chart.latestQueryFormData; + // ensure charts have their original color scheme always available + labelsColorMapInstance.setOwnColorScheme( + formData.slice_id, + formData.color_scheme, + ); - const chartColorScheme = formData.color_scheme; - const currentChartConfig = labelsColorMapInstance.chartsLabelsMap.get( - formData.slice_id, - ); - const currentChartLabels = currentChartConfig?.labels || []; - const uniqueChartLabels = currentChartLabels.filter( - l => - !sharedLabelsSet.has(l) && !customLabelsColors.hasOwnProperty(l), - ); + // if dashboard has a scheme, charts should ignore individual schemes + // thus following logic is inapplicable if a dashboard color scheme exists + if (colorScheme) return; - // Map unique labels to colors - const uniqueChartLabelsColor = new Set( - uniqueChartLabels.map(l => fullLabelsColors[l]).filter(Boolean), - ); + const chartColorScheme = formData.color_scheme; + const currentChartConfig = labelsColorMapInstance.chartsLabelsMap.get( + formData.slice_id, + ); + const currentChartLabels = currentChartConfig?.labels || []; + const uniqueChartLabels = currentChartLabels.filter( + l => !sharedLabelsSet.has(l) && !customLabelsColors.hasOwnProperty(l), + ); - const expectedColorsForChartScheme = new Set( - getColorSchemeDomain(chartColorScheme), - ); + // Map unique labels to colors + const uniqueChartLabelsColor = new Set( + uniqueChartLabels.map(l => fullLabelsColors[l]).filter(Boolean), + ); - // Check if any unique label color is not in the expected colors set - const shouldResetColors = [...uniqueChartLabelsColor].some( - color => !expectedColorsForChartScheme.has(color), - ); + const expectedColorsForChartScheme = new Set( + getColorSchemeDomain(chartColorScheme), + ); - // Only push uniqueChartLabels if they require resetting - if (shouldResetColors) shouldReset.push(...uniqueChartLabels); - }); - } + // Check if any unique label color is not in the expected colors set + const shouldResetColors = [...uniqueChartLabelsColor].some( + color => !expectedColorsForChartScheme.has(color), + ); - // an existing map is available, use mrge option - // to only apply colors to newly found labels - const shouldGoFresh = shouldReset.length > 0 ? shouldReset : false; - const shouldMerge = !shouldGoFresh; - // re-apply the color map first to get fresh maps accordingly - applyColors(metadata, shouldGoFresh, shouldMerge); - } catch (e) { - console.error('Failed to update colors for new charts and labels:', e); + // Only push uniqueChartLabels if they require resetting + if (shouldResetColors) shouldReset.push(...uniqueChartLabels); + }); } - }; + + // an existing map is available, use mrge option + // to only apply colors to newly found labels + const shouldGoFresh = shouldReset.length > 0 ? shouldReset : false; + const shouldMerge = !shouldGoFresh; + // re-apply the color map first to get fresh maps accordingly + applyColors(metadata, shouldGoFresh, shouldMerge); + } catch (e) { + console.error('Failed to update colors for new charts and labels:', e); + } +}; diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx index 998e8540dcef3..eff3116cd5cd9 100644 --- a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx +++ b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx @@ -47,7 +47,7 @@ import { loadTags } from 'src/components/Tags/utils'; import { applyColors, getColorNamespace, - getLabelsColorMapEntries, + getFreshLabelsColorMapEntries, } from 'src/utils/colorScheme'; import getOwnerName from 'src/utils/getOwnerName'; import Owner from 'src/types/Owner'; @@ -369,7 +369,7 @@ const PropertiesModal = ({ dispatch( setDashboardMetadata({ ...updatedDashboardMetadata, - map_label_colors: getLabelsColorMapEntries(customLabelColors), + map_label_colors: getFreshLabelsColorMapEntries(customLabelColors), }), ); diff --git a/superset-frontend/src/utils/colorScheme.ts b/superset-frontend/src/utils/colorScheme.ts index 35f23d29f4d4d..a02956f24a5c1 100644 --- a/superset-frontend/src/utils/colorScheme.ts +++ b/superset-frontend/src/utils/colorScheme.ts @@ -23,6 +23,8 @@ import { getCategoricalSchemeRegistry, getLabelsColorMap, } from '@superset-ui/core'; +import { intersection, pick } from 'lodash'; +import { areObjectsEqual } from 'src/reduxUtils'; const EMPTY_ARRAY: string[] = []; @@ -91,14 +93,13 @@ export const getSharedLabelsColorMapEntries = ( * @param customLabelsColor - the custom label colors in label_colors field * @returns all color entries except custom label colors */ -export const getLabelsColorMapEntries = ( +export const getFreshLabelsColorMapEntries = ( customLabelsColor: Record = {}, ): Record => { const labelsColorMapInstance = getLabelsColorMap(); const allEntries = Object.fromEntries(labelsColorMapInstance.getColorMap()); // custom label colors are applied and stored separetely via label_colors - // removing all instances of custom label colors from the entries Object.keys(customLabelsColor).forEach(label => { delete allEntries[label]; }); @@ -106,6 +107,26 @@ export const getLabelsColorMapEntries = ( return allEntries; }; +/** + * Returns all dynamic labels and colors (excluding custom label colors). + * + * @param labelsColorMap - the labels color map + * @param customLabelsColor - the custom label colors in label_colors field + * @returns all color entries except custom label colors + */ +export const getDynamicLabelsColors = ( + fullLabelsColors: Record, + customLabelsColor: Record = {}, +): Record => { + const labelsColors = { ...fullLabelsColors }; + + Object.keys(customLabelsColor).forEach(label => { + delete labelsColors[label]; + }); + + return labelsColors; +}; + export const getColorSchemeDomain = (colorScheme: string) => getCategoricalSchemeRegistry().get(colorScheme)?.colors || []; @@ -116,20 +137,29 @@ export const getColorSchemeDomain = (colorScheme: string) => * @returns true if the labels color map is the same as fresh */ export const isLabelsColorMapSynced = ( - metadata: Record, + storedLabelsColors: Record, + freshLabelsColors: Record, + customLabelColors: Record, ): boolean => { - const storedLabelsColorMap = metadata.map_label_colors || {}; - const customLabelColors = metadata.label_colors || {}; - const freshColorMap = getLabelsColorMap().getColorMap(); - const fullFreshColorMap = { - ...Object.fromEntries(freshColorMap), - ...customLabelColors, - }; + const freshLabelsCount = Object.keys(freshLabelsColors).length; + + // still updating, pass + if (!freshLabelsCount) return true; + + const commonKeys = intersection( + Object.keys(storedLabelsColors), + Object.keys(freshLabelsColors), + ); + + const comparableStoredLabelsColors = pick(storedLabelsColors, commonKeys); + const comparableFreshLabelsColors = pick(freshLabelsColors, commonKeys); - const isSynced = Object.entries(fullFreshColorMap).every( - ([label, color]) => - storedLabelsColorMap.hasOwnProperty(label) && - storedLabelsColorMap[label] === color, + const isSynced = areObjectsEqual( + comparableStoredLabelsColors, + comparableFreshLabelsColors, + { + ignoreFields: Object.keys(customLabelColors), + }, ); return isSynced; @@ -227,7 +257,7 @@ export const applyColors = ( if (fresh) { // requires a new map all together applicableColorMapEntries = { - ...getLabelsColorMapEntries(customLabelsColor), + ...getFreshLabelsColorMapEntries(customLabelsColor), }; } if (merge) { @@ -235,7 +265,7 @@ export const applyColors = ( // without overriding existing ones applicableColorMapEntries = { ...fullLabelsColor, - ...getLabelsColorMapEntries(customLabelsColor), + ...getFreshLabelsColorMapEntries(customLabelsColor), }; } diff --git a/superset/commands/dashboard/exceptions.py b/superset/commands/dashboard/exceptions.py index a4b7be12611f5..29dd98cc2a65c 100644 --- a/superset/commands/dashboard/exceptions.py +++ b/superset/commands/dashboard/exceptions.py @@ -62,6 +62,10 @@ class DashboardNativeFiltersUpdateFailedError(UpdateFailedError): message = _("Dashboard native filters could not be patched.") +class DashboardColorsConfigUpdateFailedError(UpdateFailedError): + message = _("Dashboard color configuration could not be updated.") + + class DashboardDeleteFailedError(DeleteFailedError): message = _("Dashboard could not be deleted.") diff --git a/superset/commands/dashboard/update.py b/superset/commands/dashboard/update.py index 15f5e5b5841b8..3bc0edbb89b78 100644 --- a/superset/commands/dashboard/update.py +++ b/superset/commands/dashboard/update.py @@ -22,9 +22,10 @@ from flask_appbuilder.models.sqla import Model from marshmallow import ValidationError -from superset import app, security_manager +from superset import app, db, security_manager from superset.commands.base import BaseCommand, UpdateMixin from superset.commands.dashboard.exceptions import ( + DashboardColorsConfigUpdateFailedError, DashboardForbiddenError, DashboardInvalidError, DashboardNativeFiltersUpdateFailedError, @@ -202,3 +203,27 @@ def run(self) -> Model: ) return configuration + + +class UpdateDashboardColorsConfigCommand(UpdateDashboardCommand): + @transaction( + on_error=partial(on_error, reraise=DashboardColorsConfigUpdateFailedError) + ) + def __init__(self, model_id, data, mark_updated=True): + super().__init__(model_id, data) + self._mark_updated = mark_updated + + def run(self) -> Model: + super().validate() + assert self._model + + original_changed_on = self._model.changed_on + + DashboardDAO.update_colors_config(self._model, self._properties) + + if not self._mark_updated: + db.session.commit() + # restore the original changed_on value + self._model.changed_on = original_changed_on + + return self._model diff --git a/superset/constants.py b/superset/constants.py index f6cf2a115754e..fce4be326d42f 100644 --- a/superset/constants.py +++ b/superset/constants.py @@ -174,6 +174,7 @@ class RouteMethod: # pylint: disable=too-few-public-methods "csv_metadata": "csv_upload", "slack_channels": "write", "put_filters": "write", + "put_colors": "write", } EXTRA_FORM_DATA_APPEND_KEYS = { diff --git a/superset/daos/dashboard.py b/superset/daos/dashboard.py index b0d932f614d5e..c9c119c54b98e 100644 --- a/superset/daos/dashboard.py +++ b/superset/daos/dashboard.py @@ -391,6 +391,24 @@ def update_native_filters_config( return updated_configuration + @classmethod + def update_colors_config( + cls, dashboard: Dashboard, attributes: dict[str, Any] + ) -> None: + metadata = json.loads(dashboard.json_metadata or "{}") + + for key in [ + "color_scheme_domain", + "color_scheme", + "shared_label_colors", + "map_label_colors", + "label_colors", + ]: + if key in attributes: + metadata[key] = attributes[key] + + dashboard.json_metadata = json.dumps(metadata) + @staticmethod def add_favorite(dashboard: Dashboard) -> None: ids = DashboardDAO.favorited_ids([dashboard]) diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index a752091cc18e6..e6457a7cf776e 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -42,6 +42,7 @@ ) from superset.commands.dashboard.exceptions import ( DashboardAccessDeniedError, + DashboardColorsConfigUpdateFailedError, DashboardCopyError, DashboardCreateFailedError, DashboardDeleteFailedError, @@ -57,6 +58,7 @@ from superset.commands.dashboard.permalink.create import CreateDashboardPermalinkCommand from superset.commands.dashboard.unfave import DelFavoriteDashboardCommand from superset.commands.dashboard.update import ( + UpdateDashboardColorsConfigCommand, UpdateDashboardCommand, UpdateDashboardNativeFiltersCommand, ) @@ -81,6 +83,7 @@ from superset.dashboards.schemas import ( CacheScreenshotSchema, DashboardCacheScreenshotResponseSchema, + DashboardColorsConfigUpdateSchema, DashboardCopySchema, DashboardDatasetSchema, DashboardGetResponseSchema, @@ -108,6 +111,7 @@ ) from superset.tasks.utils import get_current_user from superset.utils import json +from superset.utils.core import parse_boolean_string from superset.utils.pdf import build_pdf_from_screenshots from superset.utils.screenshots import ( DashboardScreenshot, @@ -187,6 +191,7 @@ def ensure_screenshots_enabled(self) -> Optional[Response]: "cache_dashboard_screenshot", "screenshot", "put_filters", + "put_colors", } resource_name = "dashboard" allow_browser_login = True @@ -275,6 +280,7 @@ def ensure_screenshots_enabled(self) -> Optional[Response]: add_model_schema = DashboardPostSchema() edit_model_schema = DashboardPutSchema() update_filters_model_schema = DashboardNativeFiltersConfigUpdateSchema() + update_colors_model_schema = DashboardColorsConfigUpdateSchema() chart_entity_response_schema = ChartEntityResponseSchema() dashboard_get_response_schema = DashboardGetResponseSchema() dashboard_dataset_schema = DashboardDatasetSchema() @@ -767,6 +773,89 @@ def put_filters(self, pk: int) -> Response: response = self.response_422(message=str(ex)) return response + @expose("//colors", methods=("PUT",)) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.put_colors", + log_to_statsd=False, + ) + @requires_json + def put_colors(self, pk: int) -> Response: + """ + Modify colors configuration for a dashboard. + --- + put: + summary: Update colors configuration for a dashboard. + parameters: + - in: path + schema: + type: integer + name: pk + - in: query + name: mark_updated + schema: + type: boolean + description: Whether to mark the dashboard changed_on as updated (default: true) + default: true + requestBody: + description: Colors configuration + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/DashboardColorsConfigUpdateSchema' + responses: + 200: + description: Dashboard colors updated + content: + application/json: + schema: + type: object + properties: + result: + type: array + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + try: + item = self.update_colors_model_schema.load(request.json, partial=True) + except ValidationError as error: + return self.response_400(message=error.messages) + + try: + mark_updated = parse_boolean_string( + request.args.get("mark_updated", "true") + ) + UpdateDashboardColorsConfigCommand(pk, item, mark_updated).run() + response = self.response(200) + except DashboardNotFoundError: + response = self.response_404() + except DashboardForbiddenError: + response = self.response_403() + except DashboardInvalidError as ex: + return self.response_422(message=ex.normalized_messages()) + except DashboardColorsConfigUpdateFailedError as ex: + logger.error( + "Error changing color configuration for dashboard %s: %s", + self.__class__.__name__, + str(ex), + exc_info=True, + ) + response = self.response_422(message=str(ex)) + return response + @expose("/", methods=("DELETE",)) @protect() @safe @@ -1174,6 +1263,11 @@ def screenshot(self, pk: int, digest: str) -> WerkzeugResponse: schema: type: string name: digest + - in: query + name: download_format + schema: + type: string + enum: [png, pdf] responses: 200: description: Dashboard thumbnail image diff --git a/superset/dashboards/schemas.py b/superset/dashboards/schemas.py index 9f9e2f56ebad0..4426a97e7aa47 100644 --- a/superset/dashboards/schemas.py +++ b/superset/dashboards/schemas.py @@ -428,6 +428,15 @@ class DashboardNativeFiltersConfigUpdateSchema(BaseDashboardSchema): reordered = fields.List(fields.String(), allow_none=False) +class DashboardColorsConfigUpdateSchema(BaseDashboardSchema): + color_namespace = fields.String(allow_none=True) + color_scheme = fields.String(allow_none=True) + map_label_colors = fields.Dict(allow_none=False) + shared_label_colors = SharedLabelsColorsField() + label_colors = fields.Dict(allow_none=False) + color_scheme_domain = fields.List(fields.String(), allow_none=False) + + class DashboardScreenshotPostSchema(Schema): dataMask = fields.Dict( keys=fields.Str(), diff --git a/tests/integration_tests/dashboards/api_tests.py b/tests/integration_tests/dashboards/api_tests.py index 6c6e11c8969c3..d7f147edbce6f 100644 --- a/tests/integration_tests/dashboards/api_tests.py +++ b/tests/integration_tests/dashboards/api_tests.py @@ -3197,3 +3197,114 @@ def test_cache_dashboard_screenshot_feature_disabled(self, is_feature_enabled): response = self._cache_screenshot(dashboard.id) assert response.status_code == 404 + + @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") + def test_put_dashboard_colors(self): + """ + Dashboard API: Test updating dashboard colors + """ + self.login(ADMIN_USERNAME) + dashboard = Dashboard.get("world_health") + + colors = { + "label_colors": {"Sales": "#FF0000", "Profit": "#00FF00"}, + "shared_label_colors": ["#0000FF", "#FFFF00"], + "map_label_colors": {"Revenue": "#FFFFFF"}, + "color_scheme": "d3Category10", + } + + uri = f"api/v1/dashboard/{dashboard.id}/colors" + rv = self.client.put(uri, json=colors) + assert rv.status_code == 200 + + updated_dashboard = db.session.query(Dashboard).get(dashboard.id) + updated_label_colors = json.loads(updated_dashboard.json_metadata).get( + "label_colors" + ) + updated_shared_label_colors = json.loads(updated_dashboard.json_metadata).get( + "shared_label_colors" + ) + updated_map_label_colors = json.loads(updated_dashboard.json_metadata).get( + "map_label_colors" + ) + updated_color_scheme = json.loads(updated_dashboard.json_metadata).get( + "color_scheme" + ) + + assert updated_label_colors == colors["label_colors"] + assert updated_shared_label_colors == colors["shared_label_colors"] + assert updated_map_label_colors == colors["map_label_colors"] + assert updated_color_scheme == colors["color_scheme"] + + @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") + def test_put_dashboard_colors_no_mark_updated(self): + """ + Dashboard API: Test updating dashboard colors without marking the dashboard as updated + """ + self.login(ADMIN_USERNAME) + dashboard = Dashboard.get("world_health") + + colors = {"color_scheme": "d3Category10"} + + previous_changed_on = dashboard.changed_on + uri = f"api/v1/dashboard/{dashboard.id}/colors?mark_updated=false" + rv = self.client.put(uri, json=colors) + assert rv.status_code == 200 + + updated_dashboard = db.session.query(Dashboard).get(dashboard.id) + updated_color_scheme = json.loads(updated_dashboard.json_metadata).get( + "color_scheme" + ) + + assert updated_color_scheme == colors["color_scheme"] + assert updated_dashboard.changed_on == previous_changed_on + + def test_put_dashboard_colors_not_found(self): + """ + Dashboard API: Test updating colors for dashboard that does not exist + """ + self.login(ADMIN_USERNAME) + + colors = {"label_colors": {"Sales": "#FF0000"}} + + invalid_id = self.get_nonexistent_numeric_id(Dashboard) + uri = f"api/v1/dashboard/{invalid_id}/colors" + rv = self.client.put(uri, json=colors) + assert rv.status_code == 404 + + @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") + def test_put_dashboard_colors_invalid(self): + """ + Dashboard API: Test updating dashboard colors with invalid color format + """ + self.login(ADMIN_USERNAME) + dashboard = Dashboard.get("world_health") + + colors = {"test_invalid_prop": {"Sales": "invalid"}} + + uri = f"api/v1/dashboard/{dashboard.id}/colors" + rv = self.client.put(uri, json=colors) + assert rv.status_code == 400 + + def test_put_dashboard_colors_not_authorized(self): + """ + Dashboard API: Test updating colors without authorization + """ + with self.create_app().app_context(): + admin = security_manager.find_user("admin") + dashboard = self.insert_dashboard("title", None, [admin.id]) + + assert dashboard.id is not None + + colors = {"label_colors": {"Sales": "#FF0000"}} + + self.login(GAMMA_USERNAME) + uri = f"api/v1/dashboard/{dashboard.id}/colors" + rv = self.client.put(uri, json=colors) + assert rv.status_code == 403 + + yield dashboard + + # Cleanup + db.session.delete(dashboard) + db.session.commit()