msyavuz commented on code in PR #34182: URL: https://github.com/apache/superset/pull/34182#discussion_r2229839041
########## superset-frontend/src/features/themes/ThemeModal.tsx: ########## @@ -0,0 +1,394 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { FunctionComponent, useState, useEffect, ChangeEvent } from 'react'; + +import { css, styled, t, useTheme } from '@superset-ui/core'; +import { useSingleViewResource } from 'src/views/CRUD/hooks'; +import { useThemeContext } from 'src/theme/ThemeProvider'; + +import { Icons } from '@superset-ui/core/components/Icons'; +import withToasts from 'src/components/MessageToasts/withToasts'; +import { + Input, + Modal, + JsonEditor, + Button, + Form, + Tooltip, + Alert, +} from '@superset-ui/core/components'; +import { Typography } from '@superset-ui/core/components/Typography'; + +import { OnlyKeyWithType } from 'src/utils/types'; +import { ThemeObject } from './types'; + +interface ThemeModalProps { + addDangerToast: (msg: string) => void; + addSuccessToast?: (msg: string) => void; + theme?: ThemeObject | null; + onThemeAdd?: (theme?: ThemeObject) => void; + onHide: () => void; + show: boolean; + canDevelop?: boolean; +} + +type ThemeStringKeys = keyof Pick< + ThemeObject, + OnlyKeyWithType<ThemeObject, String> +>; + +const StyledJsonEditor = styled.div` + ${({ theme }) => css` + border-radius: ${theme.borderRadius}px; + border: 1px solid ${theme.colorBorder}; + + .ace_editor { + border-radius: ${theme.borderRadius}px; + } + `} +`; + +const ThemeModal: FunctionComponent<ThemeModalProps> = ({ + addDangerToast, + addSuccessToast, + onThemeAdd, + onHide, + show, + theme = null, + canDevelop = false, +}) => { + const supersetTheme = useTheme(); + const { setTemporaryTheme } = useThemeContext(); + const [disableSave, setDisableSave] = useState<boolean>(true); + const [currentTheme, setCurrentTheme] = useState<ThemeObject | null>(null); + const [isHidden, setIsHidden] = useState<boolean>(true); + const isEditMode = theme !== null; + const isSystemTheme = currentTheme?.is_system === true; + const isReadOnly = isSystemTheme; + const canDevelopThemes = canDevelop; + + // theme fetch logic + const { + state: { loading, resource }, + fetchResource, + createResource, + updateResource, + } = useSingleViewResource<ThemeObject>('theme', t('theme'), addDangerToast); + + // Functions + const hide = () => { + onHide(); + setCurrentTheme(null); + }; + + const onSave = () => { + if (isEditMode) { + // Edit + if (currentTheme?.id) { + const update_id = currentTheme.id; + delete currentTheme.id; + delete currentTheme.created_by; + delete currentTheme.changed_by; + delete currentTheme.changed_on_delta_humanized; + + updateResource(update_id, currentTheme).then(response => { + if (!response) { + return; + } + + if (onThemeAdd) { + onThemeAdd(); + } + + hide(); + }); + } + } else if (currentTheme) { + // Create + createResource(currentTheme).then(response => { + if (!response) { + return; + } + + if (onThemeAdd) { + onThemeAdd(); + } + + hide(); + }); + } + }; + + const onApply = () => { + if (currentTheme?.json_data && isValidJson(currentTheme.json_data)) { + try { + const themeConfig = JSON.parse(currentTheme.json_data); + setTemporaryTheme(themeConfig); + if (addSuccessToast) { + addSuccessToast(t('Local theme set for preview')); + } + } catch (error) { + addDangerToast(t('Failed to apply theme: Invalid JSON')); + } + } + }; + + const onThemeNameChange = (event: ChangeEvent<HTMLInputElement>) => { + const { target } = event; + + const data = { + ...currentTheme, + theme_name: currentTheme ? currentTheme.theme_name : '', + json_data: currentTheme ? currentTheme.json_data : '', + }; + + data[target.name as ThemeStringKeys] = target.value; + setCurrentTheme(data); + }; + + const onJsonDataChange = (jsonData: string) => { + const data = { + ...currentTheme, + theme_name: currentTheme ? currentTheme.theme_name : '', + json_data: jsonData, + }; + setCurrentTheme(data); + }; + + const isValidJson = (str?: string) => { + if (!str) return false; + try { + JSON.parse(str); + return true; + } catch (e) { + return false; + } + }; + + const validate = () => { + if (isReadOnly) { + setDisableSave(true); + return; + } + + if ( + currentTheme?.theme_name.length && + currentTheme?.json_data?.length && + isValidJson(currentTheme.json_data) + ) { + setDisableSave(false); + } else { + setDisableSave(true); + } + }; Review Comment: Maybe we can show an error when this fails, i think having incorrect json is common enough. ########## superset-frontend/src/pages/ThemeList/ThemeList.test.tsx: ########## @@ -0,0 +1,243 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { + render, + screen, + userEvent, + waitFor, +} from 'spec/helpers/testing-library'; +import fetchMock from 'fetch-mock'; +import ThemesList from './index'; + +const mockThemes = [ + { + id: 1, + theme_name: 'Test Theme 1', + json_data: '{"colors": {"primary": "#1890ff"}}', + changed_on_delta_humanized: '1 day ago', + changed_by: { + first_name: 'John', + last_name: 'Doe', + }, + }, + { + id: 2, + theme_name: 'Test Theme 2', + json_data: '{"colors": {"primary": "#52c41a"}}', + changed_on_delta_humanized: '2 days ago', + changed_by: { + first_name: 'Jane', + last_name: 'Smith', + }, + }, +]; + +const mockUser = { + userId: 1, + firstName: 'Admin', + lastName: 'User', +}; + +// Mock the theme API endpoints +fetchMock.get('glob:*/api/v1/theme/_info*', { + permissions: ['can_write', 'can_export', 'can_read'], +}); + +fetchMock.get('glob:*/api/v1/theme/*', { + result: mockThemes, + count: 2, +}); + +fetchMock.delete('glob:*/api/v1/theme/*', { + message: 'Theme deleted successfully', +}); + +fetchMock.get('glob:*/api/v1/theme/related/changed_by*', { + result: [ + { text: 'John Doe', value: 1 }, + { text: 'Jane Smith', value: 2 }, + ], +}); + +const defaultProps = { + addDangerToast: jest.fn(), + addSuccessToast: jest.fn(), + user: mockUser, +}; + +const renderThemesList = (props = {}) => + render(<ThemesList {...defaultProps} {...props} />, { + useRedux: true, + useTheme: true, + useQueryParams: true, + useRouter: true, + initialState: { + user: mockUser, + }, + }); + +describe('ThemesList', () => { + beforeEach(() => { + fetchMock.resetHistory(); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + test('renders theme list with themes', async () => { + renderThemesList(); + + await waitFor(() => { + expect(screen.getByText('Test Theme 1')).toBeInTheDocument(); + expect(screen.getByText('Test Theme 2')).toBeInTheDocument(); + }); + }); + + test('shows theme actions for themes', async () => { + renderThemesList(); + + await waitFor(() => { + expect(screen.getByText('Test Theme 1')).toBeInTheDocument(); + }); + + // Should show apply, edit, export, and delete actions + const applyButtons = screen.getAllByTestId('apply-action'); + const editButtons = screen.getAllByTestId('edit-action'); + const exportButtons = screen.getAllByTestId('export-action'); + const deleteButtons = screen.getAllByTestId('delete-action'); + + expect(applyButtons.length).toBeGreaterThan(0); + expect(editButtons.length).toBeGreaterThan(0); + expect(exportButtons.length).toBeGreaterThan(0); + expect(deleteButtons.length).toBeGreaterThan(0); + }); + + test('opens theme modal when clicking add theme button', async () => { + renderThemesList(); + + await waitFor(() => { + expect(screen.getByText('Test Theme 1')).toBeInTheDocument(); + }); + + const addButton = screen.getByText('Theme'); + userEvent.click(addButton); + + await waitFor(() => { + expect(screen.getByText('Add theme')).toBeInTheDocument(); + }); + }); + + test('shows bulk select button when user has delete or export permissions', async () => { + renderThemesList(); + + await waitFor(() => { + expect(screen.getByText('Bulk select')).toBeInTheDocument(); + }); + }); + + test('filters themes by name', async () => { + renderThemesList(); + + await waitFor(() => { + expect(screen.getByText('Test Theme 1')).toBeInTheDocument(); + }); + + // Look for the search input in the Name filter section + const searchInput = screen.getByRole('textbox'); + userEvent.type(searchInput, 'Test Theme 1'); + + // The filtering is handled by the backend, so we just verify the input works + await waitFor(() => { + expect(searchInput).toHaveValue('Test Theme 1'); + }); + }); + + test('shows theme apply success message', async () => { + const mockAddSuccessToast = jest.fn(); + renderThemesList({ addSuccessToast: mockAddSuccessToast }); + + await waitFor(() => { + expect(screen.getByText('Test Theme 1')).toBeInTheDocument(); + }); + + // Mock the theme apply functionality + // This would typically involve clicking the apply button and verifying the toast + // The exact implementation depends on how the actions are rendered + }); + + test('shows delete confirmation modal', async () => { + renderThemesList(); + + await waitFor(() => { + expect(screen.getByText('Test Theme 1')).toBeInTheDocument(); + }); + + // Find and click delete button (implementation depends on how actions are rendered) + // Should show confirmation modal + // This test would be more specific once we know the exact structure + }); + + test('handles theme export', async () => { + renderThemesList(); + + await waitFor(() => { + expect(screen.getByText('Test Theme 1')).toBeInTheDocument(); + }); + + // Test export functionality + // This would involve clicking export and verifying the API call + }); + + test('handles bulk operations', async () => { + renderThemesList(); + + await waitFor(() => { + expect(screen.getByText('Test Theme 1')).toBeInTheDocument(); + }); + + // Click bulk select + const bulkSelectButton = screen.getByText('Bulk select'); + userEvent.click(bulkSelectButton); + + // Verify bulk selection interface appears + // This depends on the exact implementation + }); + + test('respects permissions for actions', async () => { + // Test with limited permissions + fetchMock.get( + 'glob:*/api/v1/theme/_info*', + { + permissions: ['can_read'], // Only read permission + }, + { overwriteRoutes: true }, + ); + + renderThemesList(); + + await waitFor(() => { + expect(screen.getByText('Test Theme 1')).toBeInTheDocument(); + }); + + // Should not show add button or bulk select + expect(screen.queryByText('Theme')).not.toBeInTheDocument(); + expect(screen.queryByText('Bulk select')).not.toBeInTheDocument(); + }); +}); Review Comment: This whole suite is missing the actual testing steps ########## superset-frontend/src/pages/ThemeList/index.tsx: ########## @@ -0,0 +1,515 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { useMemo, useState } from 'react'; +import { t, SupersetClient } from '@superset-ui/core'; +import { + Tag, + DeleteModal, + ConfirmStatusChange, + Loading, + Alert, + Tooltip, +} from '@superset-ui/core/components'; + +import rison from 'rison'; +import { useListViewResource } from 'src/views/CRUD/hooks'; +import { createErrorHandler, createFetchRelated } from 'src/views/CRUD/utils'; +import withToasts from 'src/components/MessageToasts/withToasts'; +import { useThemeContext } from 'src/theme/ThemeProvider'; +import SubMenu, { SubMenuProps } from 'src/features/home/SubMenu'; +import handleResourceExport from 'src/utils/export'; +import { + ModifiedInfo, + ListView, + ListViewActionsBar, + ListViewFilterOperator as FilterOperator, + ImportModal as ImportModelsModal, + type ListViewProps, + type ListViewActionProps, + type ListViewFilters, +} from 'src/components'; + +import ThemeModal from 'src/features/themes/ThemeModal'; +import { ThemeObject } from 'src/features/themes/types'; +import { QueryObjectColumns } from 'src/views/CRUD/types'; +import { Icons } from '@superset-ui/core/components/Icons'; + +const PAGE_SIZE = 25; + +const CONFIRM_OVERWRITE_MESSAGE = t( + 'You are importing one or more themes that already exist. ' + + 'Overwriting might cause you to lose some of your work. Are you ' + + 'sure you want to overwrite?', +); + +interface ThemesListProps { + addDangerToast: (msg: string) => void; + addSuccessToast: (msg: string) => void; + user: { + userId: string | number; + firstName: string; + lastName: string; + }; +} + +function ThemesList({ + addDangerToast, + addSuccessToast, + user, +}: ThemesListProps) { + const { + state: { + loading, + resourceCount: themesCount, + resourceCollection: themes, + bulkSelectEnabled, + }, + hasPerm, + fetchData, + refreshData, + toggleBulkSelect, + } = useListViewResource<ThemeObject>('theme', t('Themes'), addDangerToast); + const { setTemporaryTheme, getCurrentCrudThemeId } = useThemeContext(); + const [themeModalOpen, setThemeModalOpen] = useState<boolean>(false); + const [currentTheme, setCurrentTheme] = useState<ThemeObject | null>(null); + const [preparingExport, setPreparingExport] = useState<boolean>(false); + const [importingTheme, showImportModal] = useState<boolean>(false); + const [appliedThemeId, setAppliedThemeId] = useState<number | null>(null); + + const canCreate = hasPerm('can_write'); + const canEdit = hasPerm('can_write'); + const canDelete = hasPerm('can_write'); + const canExport = hasPerm('can_export'); + const canImport = hasPerm('can_write'); + const canApply = hasPerm('can_write'); // Only users with write permission can apply themes + + const [themeCurrentlyDeleting, setThemeCurrentlyDeleting] = + useState<ThemeObject | null>(null); + + const handleThemeDelete = ({ id, theme_name }: ThemeObject) => { + SupersetClient.delete({ + endpoint: `/api/v1/theme/${id}`, + }).then( + () => { + refreshData(); + setThemeCurrentlyDeleting(null); + addSuccessToast(t('Deleted: %s', theme_name)); + }, + createErrorHandler(errMsg => + addDangerToast( + t('There was an issue deleting %s: %s', theme_name, errMsg), + ), + ), + ); + }; + + const handleBulkThemeDelete = (themesToDelete: ThemeObject[]) => { + // Filter out system themes from deletion + const deletableThemes = themesToDelete.filter(theme => !theme.is_system); + + if (deletableThemes.length === 0) { + addDangerToast(t('Cannot delete system themes')); + return; + } + + if (deletableThemes.length !== themesToDelete.length) { + addDangerToast( + t( + 'Skipped %d system themes that cannot be deleted', + themesToDelete.length - deletableThemes.length, + ), + ); + } + + SupersetClient.delete({ + endpoint: `/api/v1/theme/?q=${rison.encode( + deletableThemes.map(({ id }) => id), + )}`, + }).then( + ({ json = {} }) => { + refreshData(); + addSuccessToast(json.message); + }, + createErrorHandler(errMsg => + addDangerToast( + t('There was an issue deleting the selected themes: %s', errMsg), + ), + ), + ); + }; + + function handleThemeEdit(themeObj: ThemeObject) { + setCurrentTheme(themeObj); + setThemeModalOpen(true); + } + + function handleThemeApply(themeObj: ThemeObject) { + if (themeObj.json_data) { + try { + const themeConfig = JSON.parse(themeObj.json_data); + setTemporaryTheme(themeConfig); + setAppliedThemeId(themeObj.id || null); + addSuccessToast(t('Local theme set to "%s"', themeObj.theme_name)); + } catch (error) { + addDangerToast( + t('Failed to set local theme: Invalid JSON configuration'), + ); + } + } + } + + const handleBulkThemeExport = (themesToExport: ThemeObject[]) => { + const ids = themesToExport + .map(({ id }) => id) + .filter((id): id is number => id !== undefined); + handleResourceExport('theme', ids, () => { + setPreparingExport(false); + }); + setPreparingExport(true); + }; + + const openThemeImportModal = () => { + showImportModal(true); + }; + + const closeThemeImportModal = () => { + showImportModal(false); + }; + + const handleThemeImport = () => { + showImportModal(false); + refreshData(); + addSuccessToast(t('Theme imported')); + }; + + const initialSort = [{ id: 'theme_name', desc: true }]; + const columns = useMemo( + () => [ + { + Cell: ({ row: { original } }: any) => { + const currentCrudThemeId = getCurrentCrudThemeId(); + const isCurrentTheme = + (currentCrudThemeId && + original.id?.toString() === currentCrudThemeId) || + (appliedThemeId && original.id === appliedThemeId); + + return ( + <> + {original.theme_name} + {isCurrentTheme && ( + <Tooltip + title={t('This theme is set locally for your session')} + > + <Tag color="green" style={{ marginLeft: 8 }}> + {t('Local')} + </Tag> + </Tooltip> + )} + {original.is_system && ( + <Tooltip title={t('Defined through system configuration.')}> + <Tag color="blue" style={{ marginLeft: 8 }}> + {t('System')} + </Tag> + </Tooltip> + )} + </> + ); + }, + Header: t('Name'), + accessor: 'theme_name', + id: 'theme_name', + }, + { + Cell: ({ + row: { + original: { + changed_on_delta_humanized: changedOn, + changed_by: changedBy, + }, + }, + }: any) => <ModifiedInfo date={changedOn} user={changedBy} />, + Header: t('Last modified'), + accessor: 'changed_on_delta_humanized', + size: 'xl', + disableSortBy: true, + id: 'changed_on_delta_humanized', + }, + { + Cell: ({ row: { original } }: any) => { + const handleEdit = () => handleThemeEdit(original); + const handleDelete = () => setThemeCurrentlyDeleting(original); + const handleApply = () => handleThemeApply(original); + const handleExport = () => handleBulkThemeExport([original]); + + const actions = [ + canApply + ? { + label: 'apply-action', + tooltip: t( + 'Set local theme. Will be applied to your session until unset.', + ), + placement: 'bottom', + icon: 'ThunderboltOutlined', + onClick: handleApply, + } + : null, + canEdit + ? { + label: 'edit-action', + tooltip: original.is_system + ? t('View theme') + : t('Edit theme'), + placement: 'bottom', + icon: original.is_system ? 'EyeOutlined' : 'EditOutlined', + onClick: handleEdit, + } + : null, + canExport + ? { + label: 'export-action', + tooltip: t('Export theme'), + placement: 'bottom', + icon: 'UploadOutlined', + onClick: handleExport, + } + : null, + canDelete && !original.is_system + ? { + label: 'delete-action', + tooltip: t('Delete theme'), + placement: 'bottom', + icon: 'DeleteOutlined', + onClick: handleDelete, + } + : null, + ].filter(item => !!item); + + return ( + <ListViewActionsBar actions={actions as ListViewActionProps[]} /> + ); + }, + Header: t('Actions'), + id: 'actions', + disableSortBy: true, + hidden: !canEdit && !canDelete && !canApply && !canExport, + size: 'xl', + }, + { + accessor: QueryObjectColumns.ChangedBy, + hidden: true, + id: QueryObjectColumns.ChangedBy, + }, + ], + [canDelete, canCreate, canApply, canExport, appliedThemeId], + ); + + const menuData: SubMenuProps = { + name: t('Themes'), + }; + + const subMenuButtons: SubMenuProps['buttons'] = []; + + if (canDelete || canExport) { + subMenuButtons.push({ + name: t('Bulk select'), + onClick: toggleBulkSelect, + buttonStyle: 'secondary', + }); + } + + if (canCreate) { + subMenuButtons.push({ + name: <>{t('Theme')}</>, + buttonStyle: 'primary', + icon: <Icons.PlusOutlined iconSize="m" />, + onClick: () => { + setCurrentTheme(null); + setThemeModalOpen(true); + }, + }); + } + + if (canImport) { + subMenuButtons.push({ + name: ( + <Tooltip + id="import-tooltip" + title={t('Import themes')} + placement="bottomRight" + > + <Icons.DownloadOutlined iconSize="l" data-test="import-button" /> + </Tooltip> + ), + buttonStyle: 'link', + onClick: openThemeImportModal, + }); + } + + menuData.buttons = subMenuButtons; + + const filters: ListViewFilters = useMemo( + () => [ + { + Header: t('Name'), + key: 'search', + id: 'theme_name', + input: 'search', + operator: FilterOperator.Contains, + }, + { + Header: t('Modified by'), + key: 'changed_by', + id: 'changed_by', + input: 'select', + operator: FilterOperator.RelationOneMany, + unfilteredLabel: t('All'), + fetchSelects: createFetchRelated( + 'theme', + 'changed_by', + createErrorHandler(errMsg => + t( + 'An error occurred while fetching theme datasource values: %s', + errMsg, + ), + ), + user, + ), + paginate: true, + }, + ], + [], + ); + + return ( + <> + <SubMenu {...menuData} /> + <ThemeModal + addDangerToast={addDangerToast} + theme={currentTheme} + onThemeAdd={() => refreshData()} + onHide={() => setThemeModalOpen(false)} + show={themeModalOpen} + canDevelop={canEdit} + /> + <ImportModelsModal + resourceName="theme" + resourceLabel={t('theme')} + passwordsNeededMessage="" + confirmOverwriteMessage={CONFIRM_OVERWRITE_MESSAGE} + addDangerToast={addDangerToast} + addSuccessToast={addSuccessToast} + onModelImport={handleThemeImport} + show={importingTheme} + onHide={closeThemeImportModal} + /> + {themeCurrentlyDeleting && ( + <DeleteModal + description={ + <> + <div style={{ marginBottom: 16 }}> Review Comment: Yeah, this component also has some styling issues -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
