From a0773b290bd081a3dc89054429200253ccce04e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20H=C3=A4ggmark?= Date: Fri, 20 Aug 2021 11:53:48 +0200 Subject: [PATCH] Dashboard: Prevents folder change when navigating to general settings (#38103) * Dashboard: Prevents folder change when navigating to general settings * Tests: fixes broken tests * Chore: changes from PR feedback --- .../components/FolderFilter/FolderFilter.tsx | 4 +- .../components/Select/FolderPicker.test.tsx | 59 ++++++++++++------- .../core/components/Select/FolderPicker.tsx | 54 +++++++++++++---- .../alerting/unified/RuleEditor.test.tsx | 11 ++-- .../GeneralSettings.test.tsx | 19 +----- .../DashboardSettings/GeneralSettings.tsx | 3 +- .../forms/SaveDashboardAsForm.test.tsx | 15 ++--- .../manage-dashboards/state/actions.ts | 19 ++++-- public/app/plugins/panel/dashlist/module.tsx | 3 +- public/app/types/acl.ts | 14 ++++- 10 files changed, 122 insertions(+), 79 deletions(-) diff --git a/public/app/core/components/FolderFilter/FolderFilter.tsx b/public/app/core/components/FolderFilter/FolderFilter.tsx index 015e5c04c55..44af8e06d65 100644 --- a/public/app/core/components/FolderFilter/FolderFilter.tsx +++ b/public/app/core/components/FolderFilter/FolderFilter.tsx @@ -4,7 +4,7 @@ import debounce from 'debounce-promise'; import { AsyncMultiSelect, Icon, resetSelectStyles, useStyles2 } from '@grafana/ui'; import { GrafanaTheme2, SelectableValue } from '@grafana/data'; -import { FolderInfo } from 'app/types'; +import { FolderInfo, PermissionLevelString } from 'app/types'; import { getBackendSrv } from 'app/core/services/backend_srv'; export interface FolderFilterProps { @@ -67,7 +67,7 @@ async function getFoldersAsOptions(searchString: string, setLoading: (loading: b const params = { query: searchString, type: 'dash-folder', - permission: 'View', + permission: PermissionLevelString.View, }; const searchHits = await getBackendSrv().search(params); diff --git a/public/app/core/components/Select/FolderPicker.test.tsx b/public/app/core/components/Select/FolderPicker.test.tsx index 58512a0a5c2..43ad88e7902 100644 --- a/public/app/core/components/Select/FolderPicker.test.tsx +++ b/public/app/core/components/Select/FolderPicker.test.tsx @@ -1,30 +1,49 @@ import React from 'react'; import { shallow } from 'enzyme'; -import { FolderPicker } from './FolderPicker'; -jest.mock('@grafana/runtime', () => ({ - ...((jest.requireActual('@grafana/runtime') as unknown) as object), - getBackendSrv: () => ({ - search: jest.fn(() => [ - { title: 'Dash 1', id: 'A' }, - { title: 'Dash 2', id: 'B' }, - ]), - }), -})); - -jest.mock('../../config', () => ({ - getConfig: () => ({}), -})); - -jest.mock('app/core/services/context_srv', () => ({ - contextSrv: { - user: { orgId: 1 }, - }, -})); +import { FolderPicker, getInitialValues } from './FolderPicker'; +import * as api from 'app/features/manage-dashboards/state/actions'; +import { DashboardSearchHit } from '../../../features/search/types'; describe('FolderPicker', () => { it('should render', () => { + jest + .spyOn(api, 'searchFolders') + .mockResolvedValue([ + { title: 'Dash 1', id: 1 } as DashboardSearchHit, + { title: 'Dash 2', id: 2 } as DashboardSearchHit, + ]); const wrapper = shallow(); expect(wrapper).toMatchSnapshot(); }); }); + +describe('getInitialValues', () => { + describe('when called with folderId and title', () => { + it('then it should return folderId and title', async () => { + const getFolder = jest.fn().mockResolvedValue({}); + const folder = await getInitialValues({ folderId: 0, folderName: 'Some title', getFolder }); + + expect(folder).toEqual({ label: 'Some title', value: 0 }); + expect(getFolder).not.toHaveBeenCalled(); + }); + }); + + describe('when called with just a folderId', () => { + it('then it should call api to retrieve title', async () => { + const getFolder = jest.fn().mockResolvedValue({ id: 0, title: 'Title from api' }); + const folder = await getInitialValues({ folderId: 0, getFolder }); + + expect(folder).toEqual({ label: 'Title from api', value: 0 }); + expect(getFolder).toHaveBeenCalledTimes(1); + expect(getFolder).toHaveBeenCalledWith(0); + }); + }); + + describe('when called without folderId', () => { + it('then it should throw an error', async () => { + const getFolder = jest.fn().mockResolvedValue({}); + await expect(getInitialValues({ getFolder })).rejects.toThrow(); + }); + }); +}); diff --git a/public/app/core/components/Select/FolderPicker.tsx b/public/app/core/components/Select/FolderPicker.tsx index a1653017032..9ee045e4b12 100644 --- a/public/app/core/components/Select/FolderPicker.tsx +++ b/public/app/core/components/Select/FolderPicker.tsx @@ -2,13 +2,12 @@ import React, { PureComponent } from 'react'; import { debounce } from 'lodash'; import { AsyncSelect } from '@grafana/ui'; import { AppEvents, SelectableValue } from '@grafana/data'; -import { getBackendSrv } from '@grafana/runtime'; import { selectors } from '@grafana/e2e-selectors'; import appEvents from '../../app_events'; import { contextSrv } from 'app/core/services/context_srv'; -import { DashboardSearchHit } from 'app/features/search/types'; -import { createFolder } from 'app/features/manage-dashboards/state/actions'; +import { createFolder, getFolderById, searchFolders } from 'app/features/manage-dashboards/state/actions'; +import { PermissionLevelString } from '../../../types'; export interface Props { onChange: ($folder: { title: string; id: number }) => void; @@ -18,9 +17,16 @@ export interface Props { dashboardId?: any; initialTitle?: string; initialFolderId?: number; - permissionLevel?: 'View' | 'Edit'; + permissionLevel?: Exclude; allowEmpty?: boolean; showRoot?: boolean; + /** + * Skips loading all folders in order to find the folder matching + * the folder where the dashboard is stored. + * Instead initialFolderId and initialTitle will be used to display the correct folder. + * initialFolderId needs to have an value > -1 or an error will be thrown. + */ + skipInitialLoad?: boolean; } interface State { @@ -48,26 +54,29 @@ export class FolderPicker extends PureComponent { enableReset: false, initialTitle: '', enableCreateNew: false, - permissionLevel: 'Edit', + permissionLevel: PermissionLevelString.Edit, allowEmpty: false, showRoot: true, }; componentDidMount = async () => { + if (this.props.skipInitialLoad) { + const folder = await getInitialValues({ + getFolder: getFolderById, + folderId: this.props.initialFolderId, + folderName: this.props.initialTitle, + }); + this.setState({ folder }); + return; + } + await this.loadInitialValue(); }; getOptions = async (query: string) => { const { rootName, enableReset, initialTitle, permissionLevel, initialFolderId, showRoot } = this.props; - const params = { - query, - type: 'dash-folder', - permission: permissionLevel, - }; - // TODO: move search to BackendSrv interface - // @ts-ignore - const searchHits = (await getBackendSrv().search(params)) as DashboardSearchHit[]; + const searchHits = await searchFolders(query, permissionLevel); const options: Array> = searchHits.map((hit) => ({ label: hit.title, value: hit.id })); if (contextSrv.isEditor && rootName?.toLowerCase().startsWith(query.toLowerCase()) && showRoot) { @@ -179,3 +188,22 @@ export class FolderPicker extends PureComponent { ); } } + +interface Args { + getFolder: typeof getFolderById; + folderId?: number; + folderName?: string; +} + +export async function getInitialValues({ folderName, folderId, getFolder }: Args): Promise> { + if (folderId === null || folderId === undefined || folderId < 0) { + throw new Error('folderId should to be greater or equal to zero.'); + } + + if (folderName) { + return { label: folderName, value: folderId }; + } + + const folderDto = await getFolder(folderId); + return { label: folderDto.title, value: folderId }; +} diff --git a/public/app/features/alerting/unified/RuleEditor.test.tsx b/public/app/features/alerting/unified/RuleEditor.test.tsx index 2b8a0e8cdcd..a5e24a855f9 100644 --- a/public/app/features/alerting/unified/RuleEditor.test.tsx +++ b/public/app/features/alerting/unified/RuleEditor.test.tsx @@ -1,9 +1,9 @@ import { Matcher, render, waitFor } from '@testing-library/react'; import { Provider } from 'react-redux'; -import { locationService, setDataSourceSrv, setBackendSrv, BackendSrv } from '@grafana/runtime'; +import { locationService, setDataSourceSrv } from '@grafana/runtime'; import { configureStore } from 'app/store/configureStore'; import RuleEditor from './RuleEditor'; -import { Router, Route } from 'react-router-dom'; +import { Route, Router } from 'react-router-dom'; import React from 'react'; import { byLabelText, byRole, byTestId, byText } from 'testing-library-selector'; import { selectOptionInTest } from '@grafana/ui'; @@ -18,6 +18,7 @@ import { DataSourceType, GRAFANA_RULES_SOURCE_NAME } from './utils/datasource'; import { DashboardSearchHit } from 'app/features/search/types'; import { getDefaultQueries } from './utils/rule-form'; import { ExpressionEditorProps } from './components/rule-editor/ExpressionEditor'; +import * as api from 'app/features/manage-dashboards/state/actions'; jest.mock('./components/rule-editor/ExpressionEditor', () => ({ // eslint-disable-next-line react/display-name @@ -163,7 +164,7 @@ describe('RuleEditor', () => { }); it('can create new grafana managed alert', async () => { - const searchFolderMock = jest.fn().mockResolvedValue([ + const searchFolderMock = jest.spyOn(api, 'searchFolders').mockResolvedValue([ { title: 'Folder A', id: 1, @@ -182,10 +183,6 @@ describe('RuleEditor', () => { }), }; - const backendSrv = ({ - search: searchFolderMock, - } as any) as BackendSrv; - setBackendSrv(backendSrv); setDataSourceSrv(new MockDataSourceSrv(dataSources)); mocks.api.setRulerRuleGroup.mockResolvedValue(); mocks.api.fetchRulerRulesNamespace.mockResolvedValue([]); diff --git a/public/app/features/dashboard/components/DashboardSettings/GeneralSettings.test.tsx b/public/app/features/dashboard/components/DashboardSettings/GeneralSettings.test.tsx index 476987f5ce5..38039b2531b 100644 --- a/public/app/features/dashboard/components/DashboardSettings/GeneralSettings.test.tsx +++ b/public/app/features/dashboard/components/DashboardSettings/GeneralSettings.test.tsx @@ -4,25 +4,9 @@ import userEvent from '@testing-library/user-event'; import { selectOptionInTest } from '@grafana/ui'; import { byRole } from 'testing-library-selector'; -import { Props, GeneralSettingsUnconnected as GeneralSettings } from './GeneralSettings'; +import { GeneralSettingsUnconnected as GeneralSettings, Props } from './GeneralSettings'; import { DashboardModel } from '../../state'; -jest.mock('@grafana/runtime', () => ({ - ...((jest.requireActual('@grafana/runtime') as unknown) as object), - getBackendSrv: () => ({ - search: jest.fn(() => [ - { title: 'A', id: 'A' }, - { title: 'B', id: 'B' }, - ]), - }), -})); - -jest.mock('app/core/services/context_srv', () => ({ - contextSrv: { - user: { orgId: 1 }, - }, -})); - const setupTestContext = (options: Partial) => { const defaults: Props = { dashboard: ({ @@ -33,6 +17,7 @@ const setupTestContext = (options: Partial) => { time_options: ['5m', '15m', '1h', '6h', '12h', '24h', '2d', '7d', '30d'], }, meta: { + folderId: 1, folderTitle: 'test', }, timezone: 'utc', diff --git a/public/app/features/dashboard/components/DashboardSettings/GeneralSettings.tsx b/public/app/features/dashboard/components/DashboardSettings/GeneralSettings.tsx index db13642ad6e..2e17b425d6f 100644 --- a/public/app/features/dashboard/components/DashboardSettings/GeneralSettings.tsx +++ b/public/app/features/dashboard/components/DashboardSettings/GeneralSettings.tsx @@ -1,7 +1,7 @@ import React, { useState } from 'react'; import { connect, ConnectedProps } from 'react-redux'; import { TimeZone } from '@grafana/data'; -import { TagsInput, Input, Field, CollapsableSection, RadioButtonGroup } from '@grafana/ui'; +import { CollapsableSection, Field, Input, RadioButtonGroup, TagsInput } from '@grafana/ui'; import { selectors } from '@grafana/e2e-selectors'; import { FolderPicker } from 'app/core/components/Select/FolderPicker'; import { DashboardModel } from '../../state/DashboardModel'; @@ -96,6 +96,7 @@ export function GeneralSettingsUnconnected({ dashboard, updateTimeZone }: Props) onChange={onFolderChange} enableCreateNew={true} dashboardId={dashboard.id} + skipInitialLoad={true} /> diff --git a/public/app/features/dashboard/components/SaveDashboard/forms/SaveDashboardAsForm.test.tsx b/public/app/features/dashboard/components/SaveDashboard/forms/SaveDashboardAsForm.test.tsx index 889cebdf5e9..fd46f534bb0 100644 --- a/public/app/features/dashboard/components/SaveDashboard/forms/SaveDashboardAsForm.test.tsx +++ b/public/app/features/dashboard/components/SaveDashboard/forms/SaveDashboardAsForm.test.tsx @@ -3,17 +3,7 @@ import { mount } from 'enzyme'; import { SaveDashboardAsForm } from './SaveDashboardAsForm'; import { DashboardModel } from 'app/features/dashboard/state'; import { act } from 'react-dom/test-utils'; - -jest.mock('@grafana/runtime', () => ({ - ...((jest.requireActual('@grafana/runtime') as unknown) as object), - getBackendSrv: () => ({ get: jest.fn().mockResolvedValue([]), search: jest.fn().mockResolvedValue([]) }), -})); - -jest.mock('app/core/services/context_srv', () => ({ - contextSrv: { - user: { orgId: 1 }, - }, -})); +import * as api from 'app/features/manage-dashboards/state/actions'; jest.mock('app/features/plugins/datasource_srv', () => ({})); jest.mock('app/features/expressions/ExpressionDatasource', () => ({})); @@ -21,6 +11,8 @@ jest.mock('app/features/manage-dashboards/services/ValidationSrv', () => ({ validateNewDashboardName: () => true, })); +jest.spyOn(api, 'searchFolders').mockResolvedValue([]); + const prepareDashboardMock = (panel: any) => { const json = { title: 'name', @@ -56,6 +48,7 @@ const renderAndSubmitForm = async (dashboard: any, submitSpy: any) => { describe('SaveDashboardAsForm', () => { describe('default values', () => { it('applies default dashboard properties', async () => { + jest.spyOn(api, 'searchFolders').mockResolvedValue([]); const spy = jest.fn(); await renderAndSubmitForm(prepareDashboardMock({}), spy); diff --git a/public/app/features/manage-dashboards/state/actions.ts b/public/app/features/manage-dashboards/state/actions.ts index 476d241d382..240625cd83d 100644 --- a/public/app/features/manage-dashboards/state/actions.ts +++ b/public/app/features/manage-dashboards/state/actions.ts @@ -2,16 +2,17 @@ import { AppEvents, DataSourceInstanceSettings, locationUtil } from '@grafana/da import { getBackendSrv } from 'app/core/services/backend_srv'; import { clearDashboard, - setInputs, - setGcomDashboard, - setJsonDashboard, - InputType, ImportDashboardDTO, + InputType, + setGcomDashboard, + setInputs, + setJsonDashboard, } from './reducers'; -import { ThunkResult, FolderInfo, DashboardDTO, DashboardDataDTO } from 'app/types'; +import { DashboardDataDTO, DashboardDTO, FolderInfo, PermissionLevelString, ThunkResult } from 'app/types'; import { appEvents } from '../../../core/core'; import { dashboardWatcher } from 'app/features/live/dashboard/dashboardWatcher'; import { getDataSourceSrv, locationService } from '@grafana/runtime'; +import { DashboardSearchHit } from '../../search/types'; export function fetchGcomDashboard(id: string): ThunkResult { return async (dispatch) => { @@ -222,6 +223,14 @@ export function createFolder(payload: any) { return getBackendSrv().post('/api/folders', payload); } +export function searchFolders(query: any, permission?: PermissionLevelString): Promise { + return getBackendSrv().search({ query, type: 'dash-folder', permission }); +} + +export function getFolderById(id: number): Promise<{ id: number; title: string }> { + return getBackendSrv().get(`/api/folders/id/${id}`); +} + export function deleteDashboard(uid: string, showSuccessAlert: boolean) { return getBackendSrv().request({ method: 'DELETE', diff --git a/public/app/plugins/panel/dashlist/module.tsx b/public/app/plugins/panel/dashlist/module.tsx index 78f1b293246..e4c233c5468 100644 --- a/public/app/plugins/panel/dashlist/module.tsx +++ b/public/app/plugins/panel/dashlist/module.tsx @@ -4,6 +4,7 @@ import { DashListOptions } from './types'; import { FolderPicker } from 'app/core/components/Select/FolderPicker'; import React from 'react'; import { TagsInput } from '@grafana/ui'; +import { PermissionLevelString } from '../../../types'; export const plugin = new PanelPlugin(DashList) .setPanelOptions((builder) => { @@ -49,7 +50,7 @@ export const plugin = new PanelPlugin(DashList) initialFolderId={props.value} initialTitle="All" enableReset={true} - permissionLevel="View" + permissionLevel={PermissionLevelString.View} onChange={({ id }) => props.onChange(id)} /> ); diff --git a/public/app/types/acl.ts b/public/app/types/acl.ts index b45e866dc18..4a74db51adf 100644 --- a/public/app/types/acl.ts +++ b/public/app/types/acl.ts @@ -63,6 +63,12 @@ export enum PermissionLevel { Admin = 4, } +export enum PermissionLevelString { + View = 'View', + Edit = 'Edit', + Admin = 'Admin', +} + export enum DataSourcePermissionLevel { Query = 1, Admin = 2, @@ -92,8 +98,12 @@ export const dashboardAclTargets: AclTargetInfo[] = [ ]; export const dashboardPermissionLevels: DashboardPermissionInfo[] = [ - { value: PermissionLevel.View, label: 'View', description: 'Can view dashboards.' }, - { value: PermissionLevel.Edit, label: 'Edit', description: 'Can add, edit and delete dashboards.' }, + { value: PermissionLevel.View, label: PermissionLevelString.View, description: 'Can view dashboards.' }, + { + value: PermissionLevel.Edit, + label: PermissionLevelString.Edit, + description: 'Can add, edit and delete dashboards.', + }, { value: PermissionLevel.Admin, label: 'Admin',