From 0218e94d936b6acbf2974154f1f8c5e6027501b5 Mon Sep 17 00:00:00 2001 From: Misi Date: Thu, 29 Feb 2024 17:41:08 +0100 Subject: [PATCH] Auth: Add Save and enable, Disable buttons to SSO UI (#83672) * Add Save and enable and Disable button * Change to use Dropdown, reorder buttons * Improve UI * Update public/app/features/auth-config/ProviderConfigForm.tsx * Apply suggestions from code review * Use Stack instead of separate Fields --------- Co-authored-by: Alex Khomenko --- .../auth-config/ProviderConfigForm.test.tsx | 59 +++++- .../auth-config/ProviderConfigForm.tsx | 198 ++++++++++-------- .../auth-config/ProviderConfigPage.tsx | 16 +- 3 files changed, 182 insertions(+), 91 deletions(-) diff --git a/public/app/features/auth-config/ProviderConfigForm.test.tsx b/public/app/features/auth-config/ProviderConfigForm.test.tsx index 0386ab6e710..efab7d42947 100644 --- a/public/app/features/auth-config/ProviderConfigForm.test.tsx +++ b/public/app/features/auth-config/ProviderConfigForm.test.tsx @@ -62,7 +62,7 @@ const testConfig: SSOProvider = { const emptyConfig = { ...testConfig, - settings: { ...testConfig.settings, clientId: '', clientSecret: '' }, + settings: { ...testConfig.settings, enabled: false, clientId: '', clientSecret: '' }, }; function setup(jsx: JSX.Element) { @@ -79,7 +79,6 @@ describe('ProviderConfigForm', () => { it('renders all fields correctly', async () => { setup(); - expect(screen.getByRole('checkbox', { name: /Enabled/i })).toBeInTheDocument(); expect(screen.getByRole('textbox', { name: /Client ID/i })).toBeInTheDocument(); expect(screen.getByRole('combobox', { name: /Team IDs/i })).toBeInTheDocument(); expect(screen.getByRole('combobox', { name: /Allowed organizations/i })).toBeInTheDocument(); @@ -87,7 +86,7 @@ describe('ProviderConfigForm', () => { expect(screen.getByRole('link', { name: /Discard/i })).toBeInTheDocument(); }); - it('should save correct data on form submit', async () => { + it('should save and enable on form submit', async () => { const { user } = setup(); await user.type(screen.getByRole('textbox', { name: /Client ID/i }), 'test-client-id'); await user.type(screen.getByLabelText(/Client secret/i), 'test-client-secret'); @@ -96,7 +95,7 @@ describe('ProviderConfigForm', () => { // Add two orgs await user.type(screen.getByRole('combobox', { name: /Allowed organizations/i }), 'test-org1{enter}'); await user.type(screen.getByRole('combobox', { name: /Allowed organizations/i }), 'test-org2{enter}'); - await user.click(screen.getByRole('button', { name: /Save/i })); + await user.click(screen.getByRole('button', { name: /Save and enable/i })); await waitFor(() => { expect(putMock).toHaveBeenCalledWith( @@ -123,9 +122,53 @@ describe('ProviderConfigForm', () => { }); }); - it('should validate required fields', async () => { + it('should save on form submit', async () => { const { user } = setup(); - await user.click(screen.getByRole('button', { name: /Save/i })); + await user.type(screen.getByRole('textbox', { name: /Client ID/i }), 'test-client-id'); + await user.type(screen.getByLabelText(/Client secret/i), 'test-client-secret'); + // Type a team name and press enter to select it + await user.type(screen.getByRole('combobox', { name: /Team IDs/i }), '12324{enter}'); + // Add two orgs + await user.type(screen.getByRole('combobox', { name: /Allowed organizations/i }), 'test-org1{enter}'); + await user.type(screen.getByRole('combobox', { name: /Allowed organizations/i }), 'test-org2{enter}'); + await user.click(screen.getByText('Save')); + + await waitFor(() => { + expect(putMock).toHaveBeenCalledWith( + '/api/v1/sso-settings/github', + { + id: '300f9b7c-0488-40db-9763-a22ce8bf6b3e', + provider: 'github', + settings: { + name: 'GitHub', + allowedOrganizations: 'test-org1,test-org2', + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + teamIds: '12324', + enabled: false, + }, + }, + { showErrorAlert: false } + ); + + expect(reportInteractionMock).toHaveBeenCalledWith('grafana_authentication_ssosettings_saved', { + provider: 'github', + enabled: false, + }); + }); + }); + + it('should validate required fields on Save', async () => { + const { user } = setup(); + await user.click(screen.getByText('Save')); + + // Should show an alert for empty client ID + expect(await screen.findAllByRole('alert')).toHaveLength(1); + }); + + it('should validate required fields on Save and enable', async () => { + const { user } = setup(); + await user.click(screen.getByRole('button', { name: /Save and enable/i })); // Should show an alert for empty client ID expect(await screen.findAllByRole('alert')).toHaveLength(1); @@ -133,7 +176,9 @@ describe('ProviderConfigForm', () => { it('should delete the current config', async () => { const { user } = setup(); - await user.click(screen.getByRole('button', { name: /Reset/i })); + await user.click(screen.getByTitle(/More actions/i)); + + await user.click(screen.getByRole('menuitem', { name: /Reset to default values/i })); expect(screen.getByRole('dialog', { name: /Reset/i })).toBeInTheDocument(); diff --git a/public/app/features/auth-config/ProviderConfigForm.tsx b/public/app/features/auth-config/ProviderConfigForm.tsx index 17e0f24a6d5..d8b2d3b6414 100644 --- a/public/app/features/auth-config/ProviderConfigForm.tsx +++ b/public/app/features/auth-config/ProviderConfigForm.tsx @@ -3,7 +3,19 @@ import { useForm } from 'react-hook-form'; import { AppEvents } from '@grafana/data'; import { getAppEvents, getBackendSrv, isFetchError, locationService, reportInteraction } from '@grafana/runtime'; -import { Box, Button, CollapsableSection, ConfirmModal, Field, LinkButton, Stack, Switch } from '@grafana/ui'; +import { + Box, + Button, + CollapsableSection, + ConfirmModal, + Dropdown, + Field, + IconButton, + LinkButton, + Menu, + Stack, + Switch, +} from '@grafana/ui'; import { FormPrompt } from '../../core/components/FormPrompt/FormPrompt'; import { Page } from '../../core/components/Page/Page'; @@ -39,6 +51,18 @@ export const ProviderConfigForm = ({ config, provider, isLoading }: ProviderConf const sections = sectionFields[provider]; const [resetConfig, setResetConfig] = useState(false); + const additionalActionsMenu = ( + + { + setResetConfig(true); + }} + /> + + ); + const onSubmit = async (data: SSOProviderDTO) => { setIsSaving(true); setSubmitError(false); @@ -114,95 +138,103 @@ export const ProviderConfigForm = ({ config, provider, isLoading }: ProviderConf } }; + const isEnabled = config?.settings.enabled; + return (
- <> - { - reportInteraction('grafana_authentication_ssosettings_abandoned', { - provider, - }); - reset(); - }} - /> - - - - {sections ? ( - - {sections - .filter((section) => !section.hidden) - .map((section, index) => { - return ( - - {section.fields - .filter((field) => (typeof field !== 'string' ? !field.hidden : true)) - .map((field) => { - return ( - - ); - })} - - ); - })} - - ) : ( - <> - {providerFields.map((field) => { + { + reportInteraction('grafana_authentication_ssosettings_abandoned', { + provider, + }); + reset(); + }} + /> + + {sections ? ( + + {sections + .filter((section) => !section.hidden) + .map((section, index) => { return ( - + + {section.fields + .filter((field) => (typeof field !== 'string' ? !field.hidden : true)) + .map((field) => { + return ( + + ); + })} + ); })} - - )} - - - - - - - Discard - - - - + + + + Discard + + + + - - + /> + + + {resetConfig && ( + ( + + {title} + + + )} + > );