diff --git a/superset-frontend/.eslintrc.js b/superset-frontend/.eslintrc.js index bf6cb9b6fd6..96933e2f7da 100644 --- a/superset-frontend/.eslintrc.js +++ b/superset-frontend/.eslintrc.js @@ -16,6 +16,10 @@ * specific language governing permissions and limitations * under the License. */ + +// Register TypeScript require hook so ESLint can load .ts plugin files +require('tsx/cjs'); + const packageConfig = require('./package.json'); const importCoreModules = []; @@ -148,7 +152,7 @@ module.exports = { // Custom Superset rules 'theme-colors/no-literal-colors': 'error', 'icons/no-fa-icons-usage': 'error', - 'i18n-strings/no-template-vars': ['error', true], + 'i18n-strings/no-template-vars': 'error', // Core ESLint overrides for Superset 'no-console': 'warn', @@ -195,7 +199,7 @@ module.exports = { '**/jest.setup.js', '**/webpack.config.js', '**/webpack.config.*.js', - '**/.eslintrc.js', + '**/.eslintrc*.js', ], optionalDependencies: false, }, diff --git a/superset-frontend/.eslintrc.minimal.js b/superset-frontend/.eslintrc.minimal.js index 6018fdd0122..b6b796e0ad2 100644 --- a/superset-frontend/.eslintrc.minimal.js +++ b/superset-frontend/.eslintrc.minimal.js @@ -17,6 +17,9 @@ * under the License. */ +// Register TypeScript require hook so ESLint can load .ts plugin files +require('tsx/cjs'); + /** * MINIMAL ESLint config - ONLY for rules OXC doesn't support * This config is designed to be run alongside OXC linter @@ -66,7 +69,7 @@ module.exports = { // Custom Superset plugins 'theme-colors/no-literal-colors': 'error', 'icons/no-fa-icons-usage': 'error', - 'i18n-strings/no-template-vars': ['error', true], + 'i18n-strings/no-template-vars': 'error', 'file-progress/activate': 1, // Explicitly turn off all other rules to avoid conflicts diff --git a/superset-frontend/src/pages/ChartList/ChartList.listview.test.tsx b/superset-frontend/src/pages/ChartList/ChartList.listview.test.tsx index 6d7f1d4cfe4..11220bc709c 100644 --- a/superset-frontend/src/pages/ChartList/ChartList.listview.test.tsx +++ b/superset-frontend/src/pages/ChartList/ChartList.listview.test.tsx @@ -342,10 +342,6 @@ test('displays chart data correctly in table rows', async () => { within(chartRow).getByText(testChart.changed_on_delta_humanized), ).toBeInTheDocument(); - // Check actions column within the specific row - const actionsContainer = chartRow.querySelector('.actions'); - expect(actionsContainer).toBeInTheDocument(); - // Verify action buttons exist within the specific row expect(within(chartRow).getByTestId('delete')).toBeInTheDocument(); expect(within(chartRow).getByTestId('upload')).toBeInTheDocument(); diff --git a/superset-frontend/src/pages/ChartList/index.tsx b/superset-frontend/src/pages/ChartList/index.tsx index d54a6932d48..50573e005cb 100644 --- a/superset-frontend/src/pages/ChartList/index.tsx +++ b/superset-frontend/src/pages/ChartList/index.tsx @@ -222,9 +222,13 @@ function ChartList(props: ChartListProps) { ] = useState([]); // TODO: Fix usage of localStorage keying on the user id - const userSettings = dangerouslyGetItemDoNotUse(userId?.toString(), null) as { - thumbnails: boolean; - }; + const userSettings = useMemo( + () => + dangerouslyGetItemDoNotUse(userId?.toString(), null) as { + thumbnails: boolean; + }, + [userId], + ); const openChartImportModal = () => { showImportModal(true); @@ -245,18 +249,22 @@ function ChartList(props: ChartListProps) { const canDelete = hasPerm('can_write'); const canExport = hasPerm('can_export'); const initialSort = [{ id: 'changed_on_delta_humanized', desc: true }]; - const handleBulkChartExport = async (chartsToExport: Chart[]) => { - const ids = chartsToExport.map(({ id }) => id); - setPreparingExport(true); - try { - await handleResourceExport('chart', ids, () => { + + const handleBulkChartExport = useCallback( + async (chartsToExport: Chart[]) => { + const ids = chartsToExport.map(({ id }) => id); + setPreparingExport(true); + try { + await handleResourceExport('chart', ids, () => { + setPreparingExport(false); + }); + } catch (error) { setPreparingExport(false); - }); - } catch (error) { - setPreparingExport(false); - addDangerToast(t('There was an issue exporting the selected charts')); - } - }; + addDangerToast(t('There was an issue exporting the selected charts')); + } + }, + [addDangerToast], + ); function handleBulkChartDelete(chartsToDelete: Chart[]) { SupersetClient.delete({ @@ -275,54 +283,53 @@ function ChartList(props: ChartListProps) { ), ); } - const fetchDashboards = async ( - filterValue = '', - page: number, - pageSize: number, - ) => { - // add filters if filterValue - const filters = filterValue - ? { - filters: [ - { - col: 'dashboard_title', - opr: FilterOperator.StartsWith, - value: filterValue, - }, - ], - } - : {}; - const queryParams = rison.encode({ - select_columns: ['dashboard_title', 'id'], - keys: ['none'], - order_column: 'dashboard_title', - order_direction: 'asc', - page, - page_size: pageSize, - ...filters, - }); - const response: void | JsonResponse = await SupersetClient.get({ - endpoint: `/api/v1/dashboard/?q=${queryParams}`, - }).catch(() => - addDangerToast(t('An error occurred while fetching dashboards')), - ); - const dashboards = response?.json?.result?.map( - ({ - dashboard_title: dashboardTitle, - id, - }: { - dashboard_title: string; - id: number; - }) => ({ - label: dashboardTitle, - value: id, - }), - ); - return { - data: uniqBy(dashboards, 'value'), - totalCount: response?.json?.count, - }; - }; + const fetchDashboards = useCallback( + async (filterValue = '', page: number, pageSize: number) => { + // add filters if filterValue + const filters = filterValue + ? { + filters: [ + { + col: 'dashboard_title', + opr: FilterOperator.StartsWith, + value: filterValue, + }, + ], + } + : {}; + const queryParams = rison.encode({ + select_columns: ['dashboard_title', 'id'], + keys: ['none'], + order_column: 'dashboard_title', + order_direction: 'asc', + page, + page_size: pageSize, + ...filters, + }); + const response: void | JsonResponse = await SupersetClient.get({ + endpoint: `/api/v1/dashboard/?q=${queryParams}`, + }).catch(() => + addDangerToast(t('An error occurred while fetching dashboards')), + ); + const dashboards = response?.json?.result?.map( + ({ + dashboard_title: dashboardTitle, + id, + }: { + dashboard_title: string; + id: number; + }) => ({ + label: dashboardTitle, + value: id, + }), + ); + return { + data: uniqBy(dashboards, 'value'), + totalCount: response?.json?.count, + }; + }, + [addDangerToast], + ); const columns = useMemo( () => [ @@ -506,6 +513,38 @@ function ChartList(props: ChartListProps) { return ( + {canEdit && ( + + + + + + )} + {canExport && ( + + + + + + )} {canDelete && ( )} - {canExport && ( - - - - - - )} - {canEdit && ( - - - - - - )} ); }, @@ -592,6 +599,8 @@ function ChartList(props: ChartListProps) { refreshData, addSuccessToast, addDangerToast, + handleBulkChartExport, + openChartEditModal, ], ); @@ -613,7 +622,7 @@ function ChartList(props: ChartListProps) { ); const filters: ListViewFilters = useMemo(() => { - const filters_list = [ + const filtersList = [ { Header: t('Name'), key: 'search', @@ -741,8 +750,15 @@ function ChartList(props: ChartListProps) { dropdownStyle: { minWidth: WIDER_DROPDOWN_WIDTH }, }, ] as ListViewFilters; - return filters_list; - }, [addDangerToast, favoritesFilter, props.user]); + return filtersList; + }, [ + addDangerToast, + canReadTag, + favoritesFilter, + fetchDashboards, + props.user, + userId, + ]); const sortTypes = [ { @@ -792,8 +808,14 @@ function ChartList(props: ChartListProps) { addSuccessToast, bulkSelectEnabled, favoriteStatus, + handleBulkChartExport, hasPerm, loading, + openChartEditModal, + refreshData, + saveFavoriteStatus, + userId, + userSettings, ], ); diff --git a/superset-frontend/src/pages/DashboardList/index.tsx b/superset-frontend/src/pages/DashboardList/index.tsx index 6b96d36e13e..8c5a31f6552 100644 --- a/superset-frontend/src/pages/DashboardList/index.tsx +++ b/superset-frontend/src/pages/DashboardList/index.tsx @@ -224,9 +224,9 @@ function DashboardList(props: DashboardListProps) { const initialSort = [{ id: 'changed_on_delta_humanized', desc: true }]; - function openDashboardEditModal(dashboard: Dashboard) { + const openDashboardEditModal = useCallback((dashboard: Dashboard) => { setDashboardToEdit(dashboard); - } + }, []); function handleDashboardEdit(edits: Dashboard) { return SupersetClient.get({ @@ -237,29 +237,29 @@ function DashboardList(props: DashboardListProps) { dashboards.map(dashboard => { if (dashboard.id === json?.result?.id) { const { - changed_by_name, - changed_by, - dashboard_title = '', + changed_by_name: changedByName, + changed_by: changedBy, + dashboard_title: dashboardTitle = '', slug = '', - json_metadata = '', - changed_on_delta_humanized, + json_metadata: jsonMetadata = '', + changed_on_delta_humanized: changedOnDeltaHumanized, url = '', - certified_by = '', - certification_details = '', + certified_by: certifiedBy = '', + certification_details: certificationDetails = '', owners, tags, } = json.result; return { ...dashboard, - changed_by_name, - changed_by, - dashboard_title, + changed_by_name: changedByName, + changed_by: changedBy, + dashboard_title: dashboardTitle, slug, - json_metadata, - changed_on_delta_humanized, + json_metadata: jsonMetadata, + changed_on_delta_humanized: changedOnDeltaHumanized, url, - certified_by, - certification_details, + certified_by: certifiedBy, + certification_details: certificationDetails, owners, tags, }; @@ -276,18 +276,23 @@ function DashboardList(props: DashboardListProps) { ); } - const handleBulkDashboardExport = async (dashboardsToExport: Dashboard[]) => { - const ids = dashboardsToExport.map(({ id }) => id); - setPreparingExport(true); - try { - await handleResourceExport('dashboard', ids, () => { + const handleBulkDashboardExport = useCallback( + async (dashboardsToExport: Dashboard[]) => { + const ids = dashboardsToExport.map(({ id }) => id); + setPreparingExport(true); + try { + await handleResourceExport('dashboard', ids, () => { + setPreparingExport(false); + }); + } catch (error) { setPreparingExport(false); - }); - } catch (error) { - setPreparingExport(false); - addDangerToast(t('There was an issue exporting the selected dashboards')); - } - }; + addDangerToast( + t('There was an issue exporting the selected dashboards'), + ); + } + }, + [addDangerToast], + ); function handleBulkDashboardDelete(dashboardsToDelete: Dashboard[]) { return SupersetClient.delete({ @@ -435,6 +440,38 @@ function DashboardList(props: DashboardListProps) { return ( + {canEdit && ( + + + + + + )} + {canExport && ( + + + + + + )} {canDelete && ( )} - {canExport && ( - - - - - - )} - {canEdit && ( - - - - - - )} ); }, @@ -523,6 +528,8 @@ function DashboardList(props: DashboardListProps) { refreshData, addSuccessToast, addDangerToast, + handleBulkDashboardExport, + openDashboardEditModal, ], ); @@ -544,7 +551,7 @@ function DashboardList(props: DashboardListProps) { ); const filters: ListViewFilters = useMemo(() => { - const filters_list = [ + const filtersList = [ { Header: t('Name'), key: 'search', @@ -594,7 +601,7 @@ function DashboardList(props: DashboardListProps) { ), ), ), - props.user, + user, ), optionFilterProps: OWNER_OPTION_FILTER_PROPS, paginate: true, @@ -636,8 +643,8 @@ function DashboardList(props: DashboardListProps) { dropdownStyle: { minWidth: WIDER_DROPDOWN_WIDTH }, }, ] as ListViewFilters; - return filters_list; - }, [addDangerToast, favoritesFilter, props.user]); + return filtersList; + }, [addDangerToast, canReadTag, favoritesFilter, user]); const sortTypes = [ { @@ -688,6 +695,8 @@ function DashboardList(props: DashboardListProps) { user?.userId, saveFavoriteStatus, userKey, + handleBulkDashboardExport, + openDashboardEditModal, ], ); diff --git a/superset-frontend/src/pages/DatabaseList/index.tsx b/superset-frontend/src/pages/DatabaseList/index.tsx index 84baa209bbe..e8bf7ab29c2 100644 --- a/superset-frontend/src/pages/DatabaseList/index.tsx +++ b/superset-frontend/src/pages/DatabaseList/index.tsx @@ -19,7 +19,7 @@ import { t } from '@apache-superset/core'; import { getExtensionsRegistry, SupersetClient } from '@superset-ui/core'; import { styled } from '@apache-superset/core/ui'; -import { useState, useMemo, useEffect } from 'react'; +import { useState, useMemo, useEffect, useCallback } from 'react'; import rison from 'rison'; import { useSelector } from 'react-redux'; import { useQueryParams, BooleanParam } from 'use-query-params'; @@ -169,26 +169,29 @@ function DatabaseList({ } }, [query, setQuery, refreshData]); - const openDatabaseDeleteModal = (database: DatabaseObject) => - SupersetClient.get({ - endpoint: `/api/v1/database/${database.id}/related_objects/`, - }) - .then(({ json = {} }) => { - setDatabaseCurrentlyDeleting({ - ...database, - charts: json.charts, - dashboards: json.dashboards, - sqllab_tab_count: json.sqllab_tab_states.count, - }); + const openDatabaseDeleteModal = useCallback( + (database: DatabaseObject) => + SupersetClient.get({ + endpoint: `/api/v1/database/${database.id}/related_objects/`, }) - .catch( - createErrorHandler(errMsg => - t( - 'An error occurred while fetching database related data: %s', - errMsg, + .then(({ json = {} }) => { + setDatabaseCurrentlyDeleting({ + ...database, + charts: json.charts, + dashboards: json.dashboards, + sqllab_tab_count: json.sqllab_tab_states.count, + }); + }) + .catch( + createErrorHandler(errMsg => + t( + 'An error occurred while fetching database related data: %s', + errMsg, + ), ), ), - ); + [], + ); function handleDatabaseDelete(database: DatabaseObject) { const { id, database_name: dbName } = database; @@ -216,14 +219,17 @@ function DatabaseList({ ); } - function handleDatabaseEditModal({ - database = null, - modalOpen = false, - }: { database?: DatabaseObject | null; modalOpen?: boolean } = {}) { - // Set database and modal - setCurrentDatabase(database); - setDatabaseModalOpen(modalOpen); - } + const handleDatabaseEditModal = useCallback( + ({ + database = null, + modalOpen = false, + }: { database?: DatabaseObject | null; modalOpen?: boolean } = {}) => { + // Set database and modal + setCurrentDatabase(database); + setDatabaseModalOpen(modalOpen); + }, + [], + ); const canCreate = hasPerm('can_write'); const canEdit = hasPerm('can_write'); @@ -328,59 +334,70 @@ function DatabaseList({ ]; } - async function handleDatabaseExport(database: DatabaseObject) { - if (database.id === undefined) { - return; - } + const handleDatabaseExport = useCallback( + async (database: DatabaseObject) => { + if (database.id === undefined) { + return; + } - setPreparingExport(true); - try { - await handleResourceExport('database', [database.id], () => { + setPreparingExport(true); + try { + await handleResourceExport('database', [database.id], () => { + setPreparingExport(false); + }); + } catch (error) { setPreparingExport(false); - }); - } catch (error) { - setPreparingExport(false); - addDangerToast(t('There was an issue exporting the database')); - } - } + addDangerToast(t('There was an issue exporting the database')); + } + }, + [addDangerToast, setPreparingExport], + ); - function handleDatabasePermSync(database: DatabaseObject) { - if (shouldSyncPermsInAsyncMode) { - addInfoToast(t('Validating connectivity for %s', database.database_name)); - } else { - addInfoToast(t('Syncing permissions for %s', database.database_name)); - } - SupersetClient.post({ - endpoint: `/api/v1/database/${database.id}/sync_permissions/`, - }).then( - ({ response }) => { - // Sync request - if (response.status === 200) { - addSuccessToast( - t('Permissions successfully synced for %s', database.database_name), - ); - } - // Async request - else { - addInfoToast( + const handleDatabasePermSync = useCallback( + (database: DatabaseObject) => { + if (shouldSyncPermsInAsyncMode) { + addInfoToast( + t('Validating connectivity for %s', database.database_name), + ); + } else { + addInfoToast(t('Syncing permissions for %s', database.database_name)); + } + SupersetClient.post({ + endpoint: `/api/v1/database/${database.id}/sync_permissions/`, + }).then( + ({ response }) => { + // Sync request + if (response.status === 200) { + addSuccessToast( + t( + 'Permissions successfully synced for %s', + database.database_name, + ), + ); + } + // Async request + else { + addInfoToast( + t( + 'Syncing permissions for %s in the background', + database.database_name, + ), + ); + } + }, + createErrorHandler(errMsg => + addDangerToast( t( - 'Syncing permissions for %s in the background', + 'An error occurred while syncing permissions for %s: %s', database.database_name, + errMsg, ), - ); - } - }, - createErrorHandler(errMsg => - addDangerToast( - t( - 'An error occurred while syncing permissions for %s: %s', - database.database_name, - errMsg, ), ), - ), - ); - } + ); + }, + [shouldSyncPermsInAsyncMode, addInfoToast, addSuccessToast, addDangerToast], + ); const initialSort = [{ id: 'changed_on_delta_humanized', desc: true }]; @@ -487,39 +504,6 @@ function DatabaseList({ } return ( - {canDelete && ( - - - - - - )} - {canExport && ( - - - - - - )} {canEdit && ( )} + {canExport && ( + + + + + + )} {canEdit && ( )} + {canDelete && ( + + + + + + )} ); }, @@ -568,7 +585,15 @@ function DatabaseList({ id: QueryObjectColumns.ChangedBy, }, ], - [canDelete, canEdit, canExport], + [ + canDelete, + canEdit, + canExport, + handleDatabaseEditModal, + handleDatabaseExport, + handleDatabasePermSync, + openDatabaseDeleteModal, + ], ); const filters: ListViewFilters = useMemo( @@ -634,7 +659,7 @@ function DatabaseList({ dropdownStyle: { minWidth: WIDER_DROPDOWN_WIDTH }, }, ], - [], + [user], ); return ( diff --git a/superset-frontend/src/pages/DatasetList/index.tsx b/superset-frontend/src/pages/DatasetList/index.tsx index d88a0ae8fb3..39684dd9ca7 100644 --- a/superset-frontend/src/pages/DatasetList/index.tsx +++ b/superset-frontend/src/pages/DatasetList/index.tsx @@ -228,17 +228,19 @@ const DatasetList: FunctionComponent = ({ const addCertificationFields = json.result.columns.map( (column: ColumnObject) => { const { - certification: { details = '', certified_by = '' } = {}, + certification: { + details = '', + certified_by: certifiedBy = '', + } = {}, } = JSON.parse(column.extra || '{}') || {}; return { ...column, certification_details: details || '', - certified_by: certified_by || '', - is_certified: details || certified_by, + certified_by: certifiedBy || '', + is_certified: details || certifiedBy, }; }, ); - // eslint-disable-next-line no-param-reassign json.result.columns = [...addCertificationFields]; setDatasetCurrentlyEditing(json.result); }) @@ -251,51 +253,53 @@ const DatasetList: FunctionComponent = ({ [addDangerToast], ); - const openDatasetDeleteModal = (dataset: Dataset) => - SupersetClient.get({ - endpoint: `/api/v1/dataset/${dataset.id}/related_objects`, - }) - .then(({ json = {} }) => { - setDatasetCurrentlyDeleting({ - ...dataset, - charts: json.charts, - dashboards: json.dashboards, - }); + const openDatasetDeleteModal = useCallback( + (dataset: Dataset) => + SupersetClient.get({ + endpoint: `/api/v1/dataset/${dataset.id}/related_objects`, }) - .catch( - createErrorHandler(errMsg => - t( - 'An error occurred while fetching dataset related data: %s', - errMsg, + .then(({ json = {} }) => { + setDatasetCurrentlyDeleting({ + ...dataset, + charts: json.charts, + dashboards: json.dashboards, + }); + }) + .catch( + createErrorHandler(errMsg => + t( + 'An error occurred while fetching dataset related data: %s', + errMsg, + ), ), ), - ); + [], + ); - const openDatasetDuplicateModal = (dataset: VirtualDataset) => { + const openDatasetDuplicateModal = useCallback((dataset: VirtualDataset) => { setDatasetCurrentlyDuplicating(dataset); - }; + }, []); - const handleBulkDatasetExport = async (datasetsToExport: Dataset[]) => { - const ids = datasetsToExport.map(({ id }) => id); - setPreparingExport(true); - try { - await handleResourceExport('dataset', ids, () => { + const handleBulkDatasetExport = useCallback( + async (datasetsToExport: Dataset[]) => { + const ids = datasetsToExport.map(({ id }) => id); + setPreparingExport(true); + try { + await handleResourceExport('dataset', ids, () => { + setPreparingExport(false); + }); + } catch (error) { setPreparingExport(false); - }); - } catch (error) { - setPreparingExport(false); - addDangerToast(t('There was an issue exporting the selected datasets')); - } - }; + addDangerToast(t('There was an issue exporting the selected datasets')); + } + }, + [addDangerToast, setPreparingExport], + ); const columns = useMemo( () => [ { - Cell: ({ - row: { - original: { kind }, - }, - }: any) => null, + Cell: () => null, accessor: 'kind_icon', disableSortBy: true, size: 'xs', @@ -432,38 +436,6 @@ const DatasetList: FunctionComponent = ({ } return ( - {canDelete && ( - - - - - - )} - {canExport && ( - - - - - - )} {canEdit && ( = ({ )} + {canExport && ( + + + + + + )} {canDuplicate && original.kind === 'virtual' && ( = ({ )} + {canDelete && ( + + + + + + )} ); }, @@ -516,7 +520,18 @@ const DatasetList: FunctionComponent = ({ id: QueryObjectColumns.ChangedBy, }, ], - [canEdit, canDelete, canExport, openDatasetEditModal, canDuplicate, user], + [ + canEdit, + canDelete, + canExport, + canDuplicate, + openDatasetEditModal, + openDatasetDeleteModal, + openDatasetDuplicateModal, + handleBulkDatasetExport, + PREVENT_UNSAFE_DEFAULT_URLS_ON_DATASET, + user, + ], ); const filterTypes: ListViewFilters = useMemo( diff --git a/superset-frontend/src/pages/RowLevelSecurityList/index.tsx b/superset-frontend/src/pages/RowLevelSecurityList/index.tsx index e6e86413a38..724cadf2bc7 100644 --- a/superset-frontend/src/pages/RowLevelSecurityList/index.tsx +++ b/superset-frontend/src/pages/RowLevelSecurityList/index.tsx @@ -18,7 +18,7 @@ */ import { t } from '@apache-superset/core'; import { SupersetClient } from '@superset-ui/core'; -import { useMemo, useState } from 'react'; +import { useCallback, useMemo, useState } from 'react'; import { ConfirmStatusChange, Tooltip } from '@superset-ui/core/components'; import { ModifiedInfo, @@ -51,7 +51,7 @@ interface RLSProps { function RowLevelSecurityList(props: RLSProps) { const { addDangerToast, addSuccessToast, user } = props; const [ruleModalOpen, setRuleModalOpen] = useState(false); - const [currentRule, setCurrentRule] = useState(null); + const [currentRule, setCurrentRule] = useState(null); const { state: { @@ -74,29 +74,31 @@ function RowLevelSecurityList(props: RLSProps) { true, ); - function handleRuleEdit(rule: null) { + const handleRuleEdit = useCallback((rule: RLSObject | null) => { setCurrentRule(rule); setRuleModalOpen(true); - } + }, []); - function handleRuleDelete( - { id, name }: RLSObject, - refreshData: (arg0?: FetchDataConfig | null) => void, - addSuccessToast: (arg0: string) => void, - addDangerToast: (arg0: string) => void, - ) { - return SupersetClient.delete({ - endpoint: `/api/v1/rowlevelsecurity/${id}`, - }).then( - () => { - refreshData(); - addSuccessToast(t('Deleted %s', name)); - }, - createErrorHandler(errMsg => - addDangerToast(t('There was an issue deleting %s: %s', name, errMsg)), + const handleRuleDelete = useCallback( + ( + { id, name }: RLSObject, + refreshData: (arg0?: FetchDataConfig | null) => void, + addSuccessToast: (arg0: string) => void, + addDangerToast: (arg0: string) => void, + ) => + SupersetClient.delete({ + endpoint: `/api/v1/rowlevelsecurity/${id}`, + }).then( + () => { + refreshData(); + addSuccessToast(t('Deleted %s', name)); + }, + createErrorHandler(errMsg => + addDangerToast(t('There was an issue deleting %s: %s', name, errMsg)), + ), ), - ); - } + [], + ); function handleBulkRulesDelete(rulesToDelete: RLSObject[]) { const ids = rulesToDelete.map(({ id }) => id); return SupersetClient.delete({ @@ -174,6 +176,22 @@ function RowLevelSecurityList(props: RLSProps) { const handleEdit = () => handleRuleEdit(original); return (
+ {canEdit && ( + + + + + + )} {canWrite && ( )} - {canEdit && ( - - - - - - )}
); }, @@ -238,14 +240,14 @@ function RowLevelSecurityList(props: RLSProps) { }, ], [ - user.userId, canEdit, canWrite, canExport, - hasPerm, refreshData, addDangerToast, addSuccessToast, + handleRuleDelete, + handleRuleEdit, ], ); diff --git a/superset-frontend/src/pages/SavedQueryList/index.tsx b/superset-frontend/src/pages/SavedQueryList/index.tsx index 9b32384ce5b..5101963951c 100644 --- a/superset-frontend/src/pages/SavedQueryList/index.tsx +++ b/superset-frontend/src/pages/SavedQueryList/index.tsx @@ -451,13 +451,6 @@ function SavedQueryList({ const handleDelete = () => setQueryCurrentlyDeleting(original); const actions = [ - { - label: 'preview-action', - tooltip: t('Query preview'), - placement: 'bottom', - icon: 'Binoculars', - onClick: handlePreview, - }, canEdit && { label: 'edit-action', tooltip: t('Edit query'), @@ -465,6 +458,13 @@ function SavedQueryList({ icon: 'EditOutlined', onClick: handleEdit, }, + { + label: 'preview-action', + tooltip: t('Query preview'), + placement: 'bottom', + icon: 'Binoculars', + onClick: handlePreview, + }, { label: 'copy-action', tooltip: t('Copy query URL'), diff --git a/superset-frontend/src/pages/ThemeList/index.tsx b/superset-frontend/src/pages/ThemeList/index.tsx index 3e2db4615de..d0c84adf4be 100644 --- a/superset-frontend/src/pages/ThemeList/index.tsx +++ b/superset-frontend/src/pages/ThemeList/index.tsx @@ -439,15 +439,6 @@ function ThemesList({ const handleExport = () => handleBulkThemeExport([original]); const actions = [ - canApply - ? { - label: 'apply-action', - tooltip: t('Set local theme for testing'), - placement: 'bottom', - icon: 'ThunderboltOutlined', - onClick: handleApply, - } - : null, canEdit ? { label: 'edit-action', @@ -457,6 +448,15 @@ function ThemesList({ onClick: handleEdit, } : null, + canApply + ? { + label: 'apply-action', + tooltip: t('Set local theme for testing'), + placement: 'bottom', + icon: 'ThunderboltOutlined', + onClick: handleApply, + } + : null, canExport ? { label: 'export-action', diff --git a/superset-frontend/src/views/CRUD/hooks.test.tsx b/superset-frontend/src/views/CRUD/hooks.test.tsx index 2f08fc6e871..907a4120603 100644 --- a/superset-frontend/src/views/CRUD/hooks.test.tsx +++ b/superset-frontend/src/views/CRUD/hooks.test.tsx @@ -16,245 +16,724 @@ * specific language governing permissions and limitations * under the License. */ -import { renderHook } from '@testing-library/react-hooks'; +import { renderHook, act } from '@testing-library/react-hooks'; +import { waitFor } from 'spec/helpers/testing-library'; import { JsonResponse, SupersetClient } from '@superset-ui/core'; -import { useListViewResource } from './hooks'; +import { + useListViewResource, + useSingleViewResource, + useFavoriteStatus, + useChartEditModal, +} from './hooks'; +import type Chart from 'src/types/Chart'; -// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks -describe('useListViewResource', () => { - afterEach(() => { - jest.restoreAllMocks(); - }); +/** Find the endpoint string from a spy's mock calls that matches a substring. */ +function findEndpoint(spy: jest.SpyInstance, substring: string): string { + const match = spy.mock.calls.find( + (call: unknown[]) => + typeof (call[0] as Record)?.endpoint === 'string' && + (call[0] as Record).endpoint.includes(substring), + ); - test('should fetch data with correct query parameters', async () => { - const pageIndex = 0; // Declare and initialize the pageIndex variable - const pageSize = 10; // Declare and initialize the pageSize variable - const baseFilters = [{ id: 'status', operator: 'equals', value: 'active' }]; + if (!match) { + throw new Error(`No call found with endpoint containing "${substring}"`); + } - const fetchSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ - json: { - result: { - dashboard_title: 'New Title', - slug: '/new', - json_metadata: '{"something":"foo"}', - owners: [{ id: 1, first_name: 'Al', last_name: 'Pacino' }], - roles: [], - }, - }, - } as unknown as JsonResponse); + return (match[0] as Record).endpoint; +} - const { result } = renderHook(() => - useListViewResource('example', 'Example', jest.fn()), - ); - result.current.fetchData({ - pageIndex, - pageSize, - sortBy: [{ id: 'foo' }], // Change the type of sortBy from string to SortColumn[] - filters: baseFilters, - }); +beforeEach(() => { + jest.restoreAllMocks(); +}); - expect(fetchSpy).toHaveBeenNthCalledWith(2, { - endpoint: - '/api/v1/example/?q=(filters:!((col:status,opr:equals,value:active)),order_column:foo,order_direction:asc,page:0,page_size:10)', +// useListViewResource +test('useListViewResource: initial state has loading true and empty collection', () => { + jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { permissions: ['can_read'] }, + } as unknown as JsonResponse); + + const { result } = renderHook(() => + useListViewResource('chart', 'Charts', jest.fn()), + ); + + expect(result.current.state.loading).toBe(true); + expect(result.current.state.resourceCollection).toEqual([]); + expect(result.current.state.resourceCount).toBe(0); + expect(result.current.state.bulkSelectEnabled).toBe(false); +}); + +test('useListViewResource: fetches permissions on mount', async () => { + const getSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { permissions: ['can_read', 'can_write'] }, + } as unknown as JsonResponse); + + const { result } = renderHook(() => + useListViewResource('chart', 'Charts', jest.fn()), + ); + + await waitFor(() => { + expect(getSpy).toHaveBeenCalledWith({ + endpoint: expect.stringContaining('/api/v1/chart/_info'), }); }); - test('should pass the selectColumns to the fetch call', async () => { - const pageIndex = 0; // Declare and initialize the pageIndex variable - const pageSize = 10; // Declare and initialize the pageSize variable - const baseFilters = [{ id: 'status', operator: 'equals', value: 'active' }]; - const selectColumns = ['id', 'name']; - - const fetchSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ - json: { - result: { - dashboard_title: 'New Title', - slug: '/new', - json_metadata: '{"something":"foo"}', - owners: [{ id: 1, first_name: 'Al', last_name: 'Pacino' }], - roles: [], - }, - }, - } as unknown as JsonResponse); - - const { result } = renderHook(() => - useListViewResource( - 'example', - 'Example', - jest.fn(), - undefined, - undefined, - undefined, - undefined, - selectColumns, - ), - ); - - result.current.fetchData({ - pageIndex, - pageSize, - sortBy: [{ id: 'foo' }], // Change the type of sortBy from string to SortColumn[] - filters: baseFilters, - }); - - expect(fetchSpy).toHaveBeenNthCalledWith(2, { - endpoint: - '/api/v1/example/?q=(filters:!((col:status,opr:equals,value:active)),order_column:foo,order_direction:asc,page:0,page_size:10,select_columns:!(id,name))', - }); - }); - - // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks - describe('ChartList-specific filter scenarios', () => { - afterEach(() => { - jest.restoreAllMocks(); - }); - - test('converts Type filter to correct API call for charts', async () => { - const fetchSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ - json: { result: [], count: 0 }, - } as unknown as JsonResponse); - - const { result } = renderHook(() => - useListViewResource('chart', 'Chart', jest.fn()), - ); - - const typeFilter = [{ id: 'viz_type', operator: 'eq', value: 'table' }]; - - result.current.fetchData({ - pageIndex: 0, - pageSize: 25, - sortBy: [{ id: 'changed_on_delta_humanized', desc: true }], - filters: typeFilter, - }); - - expect(fetchSpy).toHaveBeenNthCalledWith(2, { - endpoint: expect.stringContaining('/api/v1/chart/?q='), - }); - - const call = fetchSpy.mock.calls[1]; - const { endpoint } = call[0]; - - expect(endpoint).toMatch(/col:viz_type/); - expect(endpoint).toMatch(/opr:eq/); - expect(endpoint).toMatch(/value:table/); - expect(endpoint).toMatch(/order_column:changed_on_delta_humanized/); - expect(endpoint).toMatch(/order_direction:desc/); - }); - - test('converts chart search filter with ChartAllText operator', async () => { - const fetchSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ - json: { result: [], count: 0 }, - } as unknown as JsonResponse); - - const { result } = renderHook(() => - useListViewResource('chart', 'Chart', jest.fn()), - ); - - const searchFilter = [ - { - id: 'slice_name', - operator: 'chart_all_text', - value: 'test chart', - }, - ]; - - result.current.fetchData({ - pageIndex: 0, - pageSize: 25, - sortBy: [{ id: 'changed_on_delta_humanized', desc: true }], - filters: searchFilter, - }); - - const call = fetchSpy.mock.calls[1]; - const { endpoint } = call[0]; - - expect(endpoint).toContain('col%3Aslice_name'); - expect(endpoint).toContain('opr%3Achart_all_text'); - expect(endpoint).toContain("value%3A'test+chart'"); - }); - - test('converts chart-specific favorite filter', async () => { - const fetchSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ - json: { result: [], count: 0 }, - } as unknown as JsonResponse); - - const { result } = renderHook(() => - useListViewResource('chart', 'Chart', jest.fn()), - ); - - const favoriteFilter = [ - { id: 'id', operator: 'chart_is_favorite', value: true }, - ]; - - result.current.fetchData({ - pageIndex: 0, - pageSize: 25, - sortBy: [{ id: 'changed_on_delta_humanized', desc: true }], - filters: favoriteFilter, - }); - - const call = fetchSpy.mock.calls[1]; - const { endpoint } = call[0]; - - expect(endpoint).toMatch(/col:id/); - expect(endpoint).toMatch(/opr:chart_is_favorite/); - expect(endpoint).toContain('value:!t'); - }); - - test('handles multiple chart filters correctly', async () => { - const fetchSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ - json: { result: [], count: 0 }, - } as unknown as JsonResponse); - - const { result } = renderHook(() => - useListViewResource('chart', 'Chart', jest.fn()), - ); - - const multipleFilters = [ - { id: 'viz_type', operator: 'eq', value: 'table' }, - { id: 'slice_name', operator: 'chart_all_text', value: 'test' }, - ]; - - result.current.fetchData({ - pageIndex: 0, - pageSize: 25, - sortBy: [{ id: 'changed_on_delta_humanized', desc: true }], - filters: multipleFilters, - }); - - const call = fetchSpy.mock.calls[1]; - const { endpoint } = call[0]; - - // Should contain both filters - expect(endpoint).toMatch(/col:viz_type/); - expect(endpoint).toMatch(/value:table/); - expect(endpoint).toMatch(/col:slice_name/); - expect(endpoint).toMatch(/value:test/); - }); - - test('handles chart sorting scenarios', async () => { - const fetchSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ - json: { result: [], count: 0 }, - } as unknown as JsonResponse); - - const { result } = renderHook(() => - useListViewResource('chart', 'Chart', jest.fn()), - ); - - // Test alphabetical sort (slice_name ASC) - result.current.fetchData({ - pageIndex: 0, - pageSize: 25, - sortBy: [{ id: 'slice_name', desc: false }], - filters: [], - }); - - const call = fetchSpy.mock.calls[1]; - const { endpoint } = call[0]; - - expect(endpoint).toMatch(/order_column:slice_name/); - expect(endpoint).toMatch(/order_direction:asc/); - }); + await waitFor(() => { + expect(result.current.hasPerm('can_read')).toBe(true); + expect(result.current.hasPerm('can_write')).toBe(true); + expect(result.current.hasPerm('can_delete')).toBe(false); }); }); + +test('useListViewResource: skips permissions fetch when infoEnable is false', async () => { + const getSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { permissions: [] }, + } as unknown as JsonResponse); + + renderHook(() => useListViewResource('chart', 'Charts', jest.fn(), false)); + + await act(async () => {}); + + // No API call expected + expect(getSpy).not.toHaveBeenCalled(); +}); + +test('useListViewResource: hasPerm returns false when permissions are empty', async () => { + jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: {}, + } as unknown as JsonResponse); + + const { result } = renderHook(() => + useListViewResource('chart', 'Charts', jest.fn()), + ); + + expect(result.current.hasPerm('can_read')).toBe(false); +}); + +test('useListViewResource: fetchData calls correct endpoint and updates state', async () => { + const mockData = [ + { id: 1, name: 'Item 1' }, + { id: 2, name: 'Item 2' }, + ]; + const getSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { result: mockData, count: 2 }, + } as unknown as JsonResponse); + + const { result } = renderHook(() => + useListViewResource('chart', 'Charts', jest.fn()), + ); + + await act(async () => { + await result.current.fetchData({ + pageIndex: 0, + pageSize: 25, + sortBy: [{ id: 'name', desc: false }], + filters: [], + }); + }); + + // First call is permissions _info, second is the data fetch + const endpoint = findEndpoint(getSpy, '/api/v1/chart/?q='); + expect(endpoint).toContain('order_column:name'); + expect(endpoint).toContain('order_direction:asc'); + expect(endpoint).toContain('page:0'); + expect(endpoint).toContain('page_size:25'); + + await waitFor(() => { + expect(result.current.state.resourceCollection).toEqual(mockData); + expect(result.current.state.resourceCount).toBe(2); + expect(result.current.state.loading).toBe(false); + }); +}); + +test('useListViewResource: fetchData includes selectColumns in query', async () => { + const getSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { result: [], count: 0 }, + } as unknown as JsonResponse); + + const selectColumns = ['id', 'name']; + + const { result } = renderHook(() => + useListViewResource( + 'chart', + 'Charts', + jest.fn(), + undefined, + undefined, + undefined, + undefined, + selectColumns, + ), + ); + + await act(async () => { + await result.current.fetchData({ + pageIndex: 0, + pageSize: 10, + sortBy: [{ id: 'name' }], + filters: [], + }); + }); + + const endpoint = findEndpoint(getSpy, '/api/v1/chart/?q='); + expect(endpoint).toContain('select_columns:!(id,name)'); +}); + +test('useListViewResource: fetchData merges baseFilters with user filters', async () => { + const getSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { result: [], count: 0 }, + } as unknown as JsonResponse); + + const baseFilters = [{ id: 'published', operator: 'eq', value: true }]; + + const { result } = renderHook(() => + useListViewResource( + 'dashboard', + 'Dashboards', + jest.fn(), + true, + [], + baseFilters, + ), + ); + + await act(async () => { + await result.current.fetchData({ + pageIndex: 0, + pageSize: 25, + sortBy: [{ id: 'title' }], + filters: [{ id: 'name', operator: 'ct', value: 'test' }], + }); + }); + + const endpoint = findEndpoint(getSpy, '/api/v1/dashboard/?q='); + + // Both base filter and user filter should be present + expect(endpoint).toContain('col:published'); + expect(endpoint).toContain('col:name'); +}); + +test('useListViewResource: fetchData filters out empty/null/undefined filter values', async () => { + const getSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { result: [], count: 0 }, + } as unknown as JsonResponse); + + const { result } = renderHook(() => + useListViewResource('chart', 'Charts', jest.fn()), + ); + + await act(async () => { + await result.current.fetchData({ + pageIndex: 0, + pageSize: 25, + sortBy: [{ id: 'name' }], + filters: [ + { id: 'name', operator: 'ct', value: 'keep' }, + { id: 'empty', operator: 'eq', value: '' }, + { id: 'nullval', operator: 'eq', value: null }, + { id: 'undef', operator: 'eq', value: undefined }, + ], + }); + }); + + const endpoint = findEndpoint(getSpy, '/api/v1/chart/?q='); + + expect(endpoint).toContain('col:name'); + expect(endpoint).not.toContain('col:empty'); + expect(endpoint).not.toContain('col:nullval'); + expect(endpoint).not.toContain('col:undef'); +}); + +test('useListViewResource: fetchData sets loading to true then false', async () => { + let resolveGet: ((value: unknown) => void) | undefined; + jest.spyOn(SupersetClient, 'get').mockImplementation( + () => + new Promise(resolve => { + resolveGet = resolve; + }) as Promise, + ); + + const { result } = renderHook(() => + useListViewResource('chart', 'Charts', jest.fn(), false), + ); + + // Initial loading + expect(result.current.state.loading).toBe(true); + + act(() => { + result.current.fetchData({ + pageIndex: 0, + pageSize: 10, + sortBy: [{ id: 'name' }], + filters: [], + }); + }); + + // Loading should be true while fetching + expect(result.current.state.loading).toBe(true); + + await act(async () => { + expect(resolveGet).toBeDefined(); + resolveGet?.({ json: { result: [], count: 0 } }); + }); + + await waitFor(() => { + expect(result.current.state.loading).toBe(false); + }); +}); + +test('useListViewResource: refreshData re-fetches with last config', async () => { + const getSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { result: [], count: 0 }, + } as unknown as JsonResponse); + + const { result } = renderHook(() => + useListViewResource('chart', 'Charts', jest.fn()), + ); + + const config = { + pageIndex: 2, + pageSize: 50, + sortBy: [{ id: 'name', desc: true }], + filters: [], + }; + + // FetchData to cache the config + await act(async () => { + await result.current.fetchData(config); + }); + + getSpy.mockClear(); + + // RefreshData should reuse the cached config + await act(async () => { + await result.current.refreshData(); + }); + + expect(getSpy).toHaveBeenCalledWith({ + endpoint: expect.stringContaining('page:2'), + }); + expect(getSpy).toHaveBeenCalledWith({ + endpoint: expect.stringContaining('page_size:50'), + }); +}); + +test('useListViewResource: refreshData returns null when no cached config', () => { + jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: {}, + } as unknown as JsonResponse); + + const { result } = renderHook(() => + useListViewResource('chart', 'Charts', jest.fn()), + ); + + const returnValue = result.current.refreshData(); + expect(returnValue).toBeNull(); +}); + +test('useListViewResource: toggleBulkSelect toggles bulkSelectEnabled', async () => { + jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: {}, + } as unknown as JsonResponse); + + const { result } = renderHook(() => + useListViewResource('chart', 'Charts', jest.fn()), + ); + + expect(result.current.state.bulkSelectEnabled).toBe(false); + + act(() => { + result.current.toggleBulkSelect(); + }); + + expect(result.current.state.bulkSelectEnabled).toBe(true); + + act(() => { + result.current.toggleBulkSelect(); + }); + + expect(result.current.state.bulkSelectEnabled).toBe(false); +}); + +test('useListViewResource: setResourceCollection updates the collection', async () => { + jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: {}, + } as unknown as JsonResponse); + + const { result } = renderHook(() => + useListViewResource('chart', 'Charts', jest.fn()), + ); + + const newCollection = [{ id: 1 }, { id: 2 }]; + + act(() => { + result.current.setResourceCollection(newCollection); + }); + + expect(result.current.state.resourceCollection).toEqual(newCollection); +}); + +test('useListViewResource: uses desc sort direction when desc is true', async () => { + const getSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { result: [], count: 0 }, + } as unknown as JsonResponse); + + const { result } = renderHook(() => + useListViewResource('chart', 'Charts', jest.fn()), + ); + + await act(async () => { + await result.current.fetchData({ + pageIndex: 0, + pageSize: 25, + sortBy: [{ id: 'changed_on', desc: true }], + filters: [], + }); + }); + + const endpoint = findEndpoint(getSpy, '/api/v1/chart/?q='); + expect(endpoint).toContain('order_direction:desc'); +}); + +// useSingleViewResource +test('useSingleViewResource: initial state has loading false and null resource', () => { + const { result } = renderHook(() => + useSingleViewResource('chart', 'Charts', jest.fn()), + ); + + expect(result.current.state.loading).toBe(false); + expect(result.current.state.resource).toBeNull(); + expect(result.current.state.error).toBeNull(); +}); + +test('useSingleViewResource: fetchResource calls correct endpoint', async () => { + const mockResult = { id: 42, name: 'Test Chart' }; + const getSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { result: mockResult }, + } as unknown as JsonResponse); + + const { result } = renderHook(() => + useSingleViewResource('chart', 'Charts', jest.fn()), + ); + + await act(async () => { + await result.current.fetchResource(42); + }); + + expect(getSpy).toHaveBeenCalledWith({ + endpoint: '/api/v1/chart/42', + }); + + await waitFor(() => { + expect(result.current.state.resource).toEqual(mockResult); + expect(result.current.state.loading).toBe(false); + expect(result.current.state.error).toBeNull(); + }); +}); + +test('useSingleViewResource: fetchResource appends pathSuffix', async () => { + const getSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { result: {} }, + } as unknown as JsonResponse); + + const { result } = renderHook(() => + useSingleViewResource('chart', 'Charts', jest.fn(), 'related_objects'), + ); + + await act(async () => { + await result.current.fetchResource(42); + }); + + expect(getSpy).toHaveBeenCalledWith({ + endpoint: '/api/v1/chart/42/related_objects', + }); +}); + +test('useSingleViewResource: createResource posts to correct endpoint', async () => { + const postSpy = jest.spyOn(SupersetClient, 'post').mockResolvedValue({ + json: { id: 99, result: { name: 'New Chart' } }, + } as unknown as JsonResponse); + + const { result } = renderHook(() => + useSingleViewResource('chart', 'Charts', jest.fn()), + ); + + let createdId: number | undefined; + await act(async () => { + createdId = await result.current.createResource({ + name: 'New Chart', + } as any); + }); + + expect(postSpy).toHaveBeenCalledWith({ + endpoint: '/api/v1/chart/', + body: JSON.stringify({ name: 'New Chart' }), + headers: { 'Content-Type': 'application/json' }, + }); + expect(createdId).toBe(99); + + await waitFor(() => { + expect(result.current.state.loading).toBe(false); + }); +}); + +test('useSingleViewResource: updateResource puts to correct endpoint', async () => { + const putSpy = jest.spyOn(SupersetClient, 'put').mockResolvedValue({ + json: { id: 42, result: { name: 'Updated' } }, + } as unknown as JsonResponse); + + const { result } = renderHook(() => + useSingleViewResource('chart', 'Charts', jest.fn()), + ); + + await act(async () => { + await result.current.updateResource(42, { name: 'Updated' } as any); + }); + + expect(putSpy).toHaveBeenCalledWith({ + endpoint: '/api/v1/chart/42', + body: JSON.stringify({ name: 'Updated' }), + headers: { 'Content-Type': 'application/json' }, + }); + + await waitFor(() => { + expect(result.current.state.resource).toEqual({ name: 'Updated', id: 42 }); + expect(result.current.state.loading).toBe(false); + }); +}); + +test('useSingleViewResource: clearError resets error to null', async () => { + // First make a failing request to get an error state + jest.spyOn(SupersetClient, 'get').mockRejectedValue('Network error'); + + const { result } = renderHook(() => + useSingleViewResource('chart', 'Charts', jest.fn()), + ); + + await act(async () => { + try { + await result.current.fetchResource(1); + } catch { + // expected + } + }); + + act(() => { + result.current.clearError(); + }); + + expect(result.current.state.error).toBeNull(); +}); + +test('useSingleViewResource: setResource updates the resource', () => { + jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: {}, + } as unknown as JsonResponse); + + const { result } = renderHook(() => + useSingleViewResource('chart', 'Charts', jest.fn()), + ); + + act(() => { + result.current.setResource({ id: 1, name: 'Manual' } as any); + }); + + expect(result.current.state.resource).toEqual({ id: 1, name: 'Manual' }); +}); + +// useFavoriteStatus +test('useFavoriteStatus: does not fetch when ids array is empty', async () => { + const getSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { result: [] }, + } as unknown as JsonResponse); + + renderHook(() => useFavoriteStatus('chart', [], jest.fn())); + + await act(async () => {}); + + // No API call should have been made + expect(getSpy).not.toHaveBeenCalled(); +}); + +test('useFavoriteStatus: saveFaveStar posts when not starred', async () => { + jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { result: [] }, + } as unknown as JsonResponse); + const postSpy = jest + .spyOn(SupersetClient, 'post') + .mockResolvedValue({} as JsonResponse); + + const { result } = renderHook(() => + useFavoriteStatus('chart', [], jest.fn()), + ); + + await act(async () => { + // isStarred = false --> should POST to add favorite + result.current[0](42, false); + }); + + expect(postSpy).toHaveBeenCalledWith({ + endpoint: '/api/v1/chart/42/favorites/', + }); +}); + +test('useFavoriteStatus: saveFaveStar deletes when already starred', async () => { + jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { result: [] }, + } as unknown as JsonResponse); + const deleteSpy = jest + .spyOn(SupersetClient, 'delete') + .mockResolvedValue({} as JsonResponse); + + const { result } = renderHook(() => + useFavoriteStatus('chart', [], jest.fn()), + ); + + await act(async () => { + // isStarred = true --> should DELETE to remove favorite + result.current[0](42, true); + }); + + expect(deleteSpy).toHaveBeenCalledWith({ + endpoint: '/api/v1/chart/42/favorites/', + }); +}); + +test('useFavoriteStatus: saveFaveStar updates local status on success', async () => { + jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { result: [] }, + } as unknown as JsonResponse); + jest.spyOn(SupersetClient, 'post').mockResolvedValue({} as JsonResponse); + + const { result } = renderHook(() => + useFavoriteStatus('chart', [], jest.fn()), + ); + + // Star a chart + await act(async () => { + result.current[0](42, false); + }); + + await waitFor(() => { + expect(result.current[1]).toEqual(expect.objectContaining({ 42: true })); + }); +}); + +test('useFavoriteStatus: saveFaveStar uses correct endpoint per type', async () => { + jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { result: [] }, + } as unknown as JsonResponse); + const postSpy = jest + .spyOn(SupersetClient, 'post') + .mockResolvedValue({} as JsonResponse); + + const { result } = renderHook(() => + useFavoriteStatus('dashboard', [], jest.fn()), + ); + + await act(async () => { + result.current[0](7, false); + }); + + expect(postSpy).toHaveBeenCalledWith({ + endpoint: '/api/v1/dashboard/7/favorites/', + }); +}); + +// useChartEditModal +test('useChartEditModal: openChartEditModal sets sliceCurrentlyEditing', () => { + const mockChart: Chart = { + id: 1, + slice_name: 'Test Chart', + description: 'A test', + cache_timeout: 300, + certified_by: 'Admin', + certification_details: 'Certified', + is_managed_externally: false, + } as Chart; + + const { result } = renderHook(() => + useChartEditModal(jest.fn(), [mockChart]), + ); + + expect(result.current.sliceCurrentlyEditing).toBeNull(); + + act(() => { + result.current.openChartEditModal(mockChart); + }); + + expect(result.current.sliceCurrentlyEditing).toEqual({ + slice_id: 1, + slice_name: 'Test Chart', + description: 'A test', + cache_timeout: 300, + certified_by: 'Admin', + certification_details: 'Certified', + is_managed_externally: false, + }); +}); + +test('useChartEditModal: closeChartEditModal clears sliceCurrentlyEditing', () => { + const mockChart: Chart = { + id: 1, + slice_name: 'Test Chart', + } as Chart; + + const { result } = renderHook(() => + useChartEditModal(jest.fn(), [mockChart]), + ); + + act(() => { + result.current.openChartEditModal(mockChart); + }); + expect(result.current.sliceCurrentlyEditing).not.toBeNull(); + + act(() => { + result.current.closeChartEditModal(); + }); + expect(result.current.sliceCurrentlyEditing).toBeNull(); +}); + +test('useChartEditModal: handleChartUpdated merges edits into chart list', () => { + const setCharts = jest.fn(); + const charts: Chart[] = [ + { id: 1, slice_name: 'Original' } as Chart, + { id: 2, slice_name: 'Other' } as Chart, + ]; + + const { result } = renderHook(() => useChartEditModal(setCharts, charts)); + + act(() => { + result.current.handleChartUpdated({ + id: 1, + slice_name: 'Updated Name', + } as Chart); + }); + + expect(setCharts).toHaveBeenCalledWith([ + { id: 1, slice_name: 'Updated Name' }, + { id: 2, slice_name: 'Other' }, + ]); +}); + +test('useChartEditModal: handleChartUpdated leaves non-matching charts unchanged', () => { + const setCharts = jest.fn(); + const charts: Chart[] = [ + { id: 1, slice_name: 'A' } as Chart, + { id: 2, slice_name: 'B' } as Chart, + ]; + + const { result } = renderHook(() => useChartEditModal(setCharts, charts)); + + act(() => { + result.current.handleChartUpdated({ + id: 999, + slice_name: 'Nonexistent', + } as Chart); + }); + + expect(setCharts).toHaveBeenCalledWith([ + { id: 1, slice_name: 'A' }, + { id: 2, slice_name: 'B' }, + ]); +}); diff --git a/superset-frontend/src/views/CRUD/hooks.ts b/superset-frontend/src/views/CRUD/hooks.ts index 85f22183485..6d85d3d8e5f 100644 --- a/superset-frontend/src/views/CRUD/hooks.ts +++ b/superset-frontend/src/views/CRUD/hooks.ts @@ -17,7 +17,7 @@ * under the License. */ import rison from 'rison'; -import { useState, useEffect, useCallback } from 'react'; +import { useState, useEffect, useCallback, useRef } from 'react'; import { t } from '@apache-superset/core'; import { makeApi, @@ -91,14 +91,22 @@ export function useListViewResource( bulkSelectEnabled: false, }); - function updateState(update: Partial>) { - setState(currentState => ({ ...currentState, ...update })); - } + const updateState = useCallback( + (update: Partial>) => { + setState(currentState => ({ ...currentState, ...update })); + }, + [], + ); function toggleBulkSelect() { updateState({ bulkSelectEnabled: !state.bulkSelectEnabled }); } + const handleErrorMsgRef = useRef(handleErrorMsg); + useEffect(() => { + handleErrorMsgRef.current = handleErrorMsg; + }); + useEffect(() => { if (!infoEnable) return; SupersetClient.get({ @@ -112,7 +120,7 @@ export function useListViewResource( }); }, createErrorHandler(errMsg => - handleErrorMsg( + handleErrorMsgRef.current( t( 'An error occurred while fetching %s info: %s', resourceLabel, @@ -121,15 +129,20 @@ export function useListViewResource( ), ), ); - }, []); + }, [infoEnable, resource, resourceLabel, updateState]); - function hasPerm(perm: string) { - if (!state.permissions.length) { - return false; - } + const hasPerm = useCallback( + (perm: string) => { + if (!state.permissions.length) { + return false; + } - return Boolean(state.permissions.find(p => p === perm)); - } + return Boolean(state.permissions.find(p => p === perm)); + }, + [state.permissions], + ); + + const lastFetchDataConfigRef = useRef(null); const fetchData = useCallback( ({ @@ -138,14 +151,16 @@ export function useListViewResource( sortBy, filters: filterValues, }: FetchDataConfig) => { + const config: FetchDataConfig = { + filters: filterValues, + pageIndex, + pageSize, + sortBy, + }; + lastFetchDataConfigRef.current = config; // set loading state, cache the last config for refreshing data. updateState({ - lastFetchDataConfig: { - filters: filterValues, - pageIndex, - pageSize, - sortBy, - }, + lastFetchDataConfig: config, loading: true, }); const filterExps = (baseFilters || []) @@ -196,7 +211,27 @@ export function useListViewResource( updateState({ loading: false }); }); }, - [baseFilters], + [ + baseFilters, + handleErrorMsg, + resource, + resourceLabel, + selectColumns, + updateState, + ], + ); + + const refreshData = useCallback( + (provideConfig?: FetchDataConfig) => { + if (lastFetchDataConfigRef.current) { + return fetchData(lastFetchDataConfigRef.current); + } + if (provideConfig) { + return fetchData(provideConfig); + } + return null; + }, + [fetchData], ); return { @@ -214,15 +249,7 @@ export function useListViewResource( hasPerm, fetchData, toggleBulkSelect, - refreshData: (provideConfig?: FetchDataConfig) => { - if (state.lastFetchDataConfig) { - return fetchData(state.lastFetchDataConfig); - } - if (provideConfig) { - return fetchData(provideConfig); - } - return null; - }, + refreshData, }; } @@ -237,7 +264,7 @@ export function useSingleViewResource( resourceName: string, resourceLabel: string, // resourceLabel for translations handleErrorMsg: (errorMsg: string) => void, - path_suffix = '', + pathSuffix = '', ) { const [state, setState] = useState>({ loading: false, @@ -245,9 +272,12 @@ export function useSingleViewResource( error: null, }); - function updateState(update: Partial>) { - setState(currentState => ({ ...currentState, ...update })); - } + const updateState = useCallback( + (update: Partial>) => { + setState(currentState => ({ ...currentState, ...update })); + }, + [], + ); const fetchResource = useCallback( (resourceID: number) => { @@ -258,7 +288,7 @@ export function useSingleViewResource( const baseEndpoint = `/api/v1/${resourceName}/${resourceID}`; const endpoint = - path_suffix !== '' ? `${baseEndpoint}/${path_suffix}` : baseEndpoint; + pathSuffix !== '' ? `${baseEndpoint}/${pathSuffix}` : baseEndpoint; return SupersetClient.get({ endpoint, }) @@ -288,7 +318,7 @@ export function useSingleViewResource( updateState({ loading: false }); }); }, - [handleErrorMsg, resourceName, resourceLabel], + [handleErrorMsg, pathSuffix, resourceName, resourceLabel, updateState], ); const createResource = useCallback( @@ -332,7 +362,7 @@ export function useSingleViewResource( updateState({ loading: false }); }); }, - [handleErrorMsg, resourceName, resourceLabel], + [handleErrorMsg, resourceName, resourceLabel, updateState], ); const updateResource = useCallback( @@ -381,7 +411,7 @@ export function useSingleViewResource( } }); }, - [handleErrorMsg, resourceName, resourceLabel], + [handleErrorMsg, resourceName, resourceLabel, updateState], ); const clearError = () => @@ -427,12 +457,12 @@ export function useImportResource( failed: false, }); - function updateState(update: Partial) { + const updateState = useCallback((update: Partial) => { setState(currentState => ({ ...currentState, ...update })); - } + }, []); const importResource = useCallback( - ( + async ( bundle: File, databasePasswords: Record = {}, sshTunnelPasswords: Record = {}, @@ -553,7 +583,7 @@ export function useImportResource( updateState({ loading: false }); }); }, - [], + [handleErrorMsg, resourceLabel, resourceName, updateState], ); return { state, importResource }; @@ -591,8 +621,11 @@ export function useFavoriteStatus( ) { const [favoriteStatus, setFavoriteStatus] = useState({}); - const updateFavoriteStatus = (update: FavoriteStatus) => - setFavoriteStatus(currentState => ({ ...currentState, ...update })); + const updateFavoriteStatus = useCallback( + (update: FavoriteStatus) => + setFavoriteStatus(currentState => ({ ...currentState, ...update })), + [], + ); useEffect(() => { if (!ids.length) { @@ -615,7 +648,7 @@ export function useFavoriteStatus( ), ), ); - }, [ids, type, handleErrorMsg]); + }, [ids, type, handleErrorMsg, updateFavoriteStatus]); const saveFaveStar = useCallback( (id: number, isStarred: boolean) => { @@ -639,7 +672,7 @@ export function useFavoriteStatus( ), ); }, - [type], + [handleErrorMsg, type, updateFavoriteStatus], ); return [saveFaveStar, favoriteStatus] as const; @@ -652,7 +685,7 @@ export const useChartEditModal = ( const [sliceCurrentlyEditing, setSliceCurrentlyEditing] = useState(null); - function openChartEditModal(chart: Chart) { + const openChartEditModal = useCallback((chart: Chart) => { setSliceCurrentlyEditing({ slice_id: chart.id, slice_name: chart.slice_name, @@ -662,11 +695,11 @@ export const useChartEditModal = ( certification_details: chart.certification_details, is_managed_externally: chart.is_managed_externally, }); - } + }, []); - function closeChartEditModal() { + const closeChartEditModal = useCallback(() => { setSliceCurrentlyEditing(null); - } + }, []); function handleChartUpdated(edits: Chart) { // update the chart in our state with the edited info @@ -796,8 +829,8 @@ export function useDatabaseValidation() { ]; return allowed.includes(err.error_type) || onCreate; }) - .reduce((acc: JsonObject, err_2: any) => { - const { message, extra } = err_2; + .reduce((acc: JsonObject, err2: any) => { + const { message, extra } = err2; if (extra?.catalog) { const { idx } = extra.catalog; @@ -816,8 +849,8 @@ export function useDatabaseValidation() { } if (extra?.missing) { - extra.missing.forEach((field_1: string) => { - acc[field_1] = 'This is a required field'; + extra.missing.forEach((field1: string) => { + acc[field1] = 'This is a required field'; }); }