From 9441692fe9953ca2ddb279029f77c710a4d69a2d Mon Sep 17 00:00:00 2001 From: Matias Chomicki Date: Mon, 15 May 2023 12:48:50 +0200 Subject: [PATCH] Loki Derived Fields: Refactor legacy form components and add validation (#68015) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Derived fields: validate duplicated names * Derived fields: rename prop * Update tests * Derived fields: do not validate empty names as repeated * Derived Field: use non-legacy Field and Input * Derived Field: integrate name validation * Derived field: align delete button * Derived Field: add tooltips * Derived Field: migrate and style internal link field * Update tests * Derived Field: ask user to select data source Otherwise it's bugged * Remove unnecessary onchange handler * Initialize all derived fields attributes Otherwise we trigger controlled-to-uncontrolled React errors * Update public/app/plugins/datasource/loki/configuration/DerivedField.tsx Co-authored-by: Gábor Farkas --------- Co-authored-by: Gábor Farkas --- .../loki/configuration/ConfigEditor.tsx | 2 +- .../loki/configuration/DerivedField.test.tsx | 47 ++++- .../loki/configuration/DerivedField.tsx | 181 ++++++++++-------- .../loki/configuration/DerivedFields.test.tsx | 51 ++++- .../loki/configuration/DerivedFields.tsx | 26 ++- 5 files changed, 202 insertions(+), 105 deletions(-) diff --git a/public/app/plugins/datasource/loki/configuration/ConfigEditor.tsx b/public/app/plugins/datasource/loki/configuration/ConfigEditor.tsx index b518ffc06ee..405b5814a6e 100644 --- a/public/app/plugins/datasource/loki/configuration/ConfigEditor.tsx +++ b/public/app/plugins/datasource/loki/configuration/ConfigEditor.tsx @@ -47,7 +47,7 @@ export const ConfigEditor = (props: Props) => { /> onOptionsChange(setDerivedFields(options, value))} /> diff --git a/public/app/plugins/datasource/loki/configuration/DerivedField.test.tsx b/public/app/plugins/datasource/loki/configuration/DerivedField.test.tsx index fb596e2f082..7b5b58fa119 100644 --- a/public/app/plugins/datasource/loki/configuration/DerivedField.test.tsx +++ b/public/app/plugins/datasource/loki/configuration/DerivedField.test.tsx @@ -1,4 +1,5 @@ import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import React from 'react'; import { DataSourceInstanceSettings, DataSourcePluginMeta } from '@grafana/data'; @@ -8,6 +9,7 @@ import { setDataSourceSrv } from '@grafana/runtime'; import { DerivedField } from './DerivedField'; const mockList = jest.fn(); +const validateMock = jest.fn(); describe('DerivedField', () => { beforeEach(() => { @@ -54,7 +56,15 @@ describe('DerivedField', () => { }; // Render and wait for the Name field to be visible // using findBy to wait for asynchronous operations to complete - render( {}} onDelete={() => {}} suggestions={[]} />); + render( + {}} + onDelete={() => {}} + suggestions={[]} + /> + ); expect(await screen.findByText('Name')).toBeInTheDocument(); expect(screen.getByLabelText(selectors.components.DataSourcePicker.inputV2)).toBeInTheDocument(); @@ -68,7 +78,15 @@ describe('DerivedField', () => { }; // Render and wait for the Name field to be visible // using findBy to wait for asynchronous operations to complete - render( {}} onDelete={() => {}} suggestions={[]} />); + render( + {}} + onDelete={() => {}} + suggestions={[]} + /> + ); expect(await screen.findByText('Name')).toBeInTheDocument(); expect(screen.queryByLabelText(selectors.components.DataSourcePicker.inputV2)).not.toBeInTheDocument(); @@ -82,7 +100,15 @@ describe('DerivedField', () => { }; // Render and wait for the Name field to be visible // using findBy to wait for asynchronous operations to complete - render( {}} onDelete={() => {}} suggestions={[]} />); + render( + {}} + onDelete={() => {}} + suggestions={[]} + /> + ); expect(await screen.findByText('Name')).toBeInTheDocument(); expect(mockList).toHaveBeenCalledWith( expect.objectContaining({ @@ -90,4 +116,19 @@ describe('DerivedField', () => { }) ); }); + + it('validates the field name', async () => { + const value = { + matcherRegex: '', + name: 'field-name', + datasourceUid: 'test', + }; + const validate = jest.fn().mockReturnValue(false); + render( + {}} onDelete={() => {}} suggestions={[]} /> + ); + userEvent.click(await screen.findByDisplayValue(value.name)); + + expect(await screen.findByText('The name is already in use')).toBeInTheDocument(); + }); }); diff --git a/public/app/plugins/datasource/loki/configuration/DerivedField.tsx b/public/app/plugins/datasource/loki/configuration/DerivedField.tsx index 81d2ae8c007..356fa4a3223 100644 --- a/public/app/plugins/datasource/loki/configuration/DerivedField.tsx +++ b/public/app/plugins/datasource/loki/configuration/DerivedField.tsx @@ -1,15 +1,13 @@ import { css } from '@emotion/css'; -import React, { useEffect, useState } from 'react'; +import React, { ChangeEvent, useEffect, useState } from 'react'; import { usePrevious } from 'react-use'; import { GrafanaTheme2, VariableSuggestion } from '@grafana/data'; import { DataSourcePicker } from '@grafana/runtime'; -import { Button, DataLinkInput, LegacyForms, useStyles2 } from '@grafana/ui'; +import { Button, DataLinkInput, Field, Icon, Input, Label, Tooltip, useStyles2, Switch } from '@grafana/ui'; import { DerivedFieldConfig } from '../types'; -const { Switch, FormField } = LegacyForms; - const getStyles = (theme: GrafanaTheme2) => ({ row: css` display: flex; @@ -17,9 +15,11 @@ const getStyles = (theme: GrafanaTheme2) => ({ `, nameField: css` flex: 2; + margin-right: ${theme.spacing(0.5)}; `, regexField: css` flex: 3; + margin-right: ${theme.spacing(0.5)}; `, urlField: css` flex: 1; @@ -28,6 +28,10 @@ const getStyles = (theme: GrafanaTheme2) => ({ urlDisplayLabelField: css` flex: 1; `, + internalLink: css` + margin-right: ${theme.spacing(1)}; + `, + dataSource: css``, }); type Props = { @@ -36,9 +40,10 @@ type Props = { onDelete: () => void; suggestions: VariableSuggestion[]; className?: string; + validateName: (name: string) => boolean; }; export const DerivedField = (props: Props) => { - const { value, onChange, onDelete, suggestions, className } = props; + const { value, onChange, onDelete, suggestions, className, validateName } = props; const styles = useStyles2(getStyles); const [showInternalLink, setShowInternalLink] = useState(!!value.datasourceUid); const previousUid = usePrevious(value.datasourceUid); @@ -60,101 +65,107 @@ export const DerivedField = (props: Props) => { }); }; + const invalidName = !validateName(value.name); + return (
- - + + + } - /> -
- - onChange({ - ...value, - url: newValue, - }) - } - suggestions={suggestions} - /> - } - className={styles.urlField} - /> - -
- -
- { - if (showInternalLink) { + + onChange({ ...value, - datasourceUid: undefined, - }); - } - setShowInternalLink(!showInternalLink); - }} - /> - - {showInternalLink && ( - - onChange({ - ...value, - datasourceUid: ds.uid, + url: newValue, }) } - current={value.datasourceUid} + suggestions={suggestions} /> + + + } + > + + +
+ +
+ + ) => { + const { checked } = e.currentTarget; + if (!checked) { + onChange({ + ...value, + datasourceUid: undefined, + }); + } + setShowInternalLink(checked); + }} + /> + + + {showInternalLink && ( + + + onChange({ + ...value, + datasourceUid: ds.uid, + }) + } + current={value.datasourceUid} + noDefault + /> + )}
); }; + +const TooltipLabel = ({ content, label }: { content: string; label: string }) => ( + +); diff --git a/public/app/plugins/datasource/loki/configuration/DerivedFields.test.tsx b/public/app/plugins/datasource/loki/configuration/DerivedFields.test.tsx index 3d6259a99b4..dc387e26f35 100644 --- a/public/app/plugins/datasource/loki/configuration/DerivedFields.test.tsx +++ b/public/app/plugins/datasource/loki/configuration/DerivedFields.test.tsx @@ -1,4 +1,5 @@ -import { render, screen, waitFor, fireEvent } from '@testing-library/react'; +import { render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import React from 'react'; import { DerivedFields } from './DerivedFields'; @@ -23,7 +24,7 @@ describe('DerivedFields', () => { }); it('renders correctly when there are fields', async () => { - render( {}} />); + render( {}} />); await waitFor(() => expect(screen.getAllByTestId('derived-field')).toHaveLength(2)); expect(screen.getByText('Add')).toBeInTheDocument(); @@ -34,22 +35,58 @@ describe('DerivedFields', () => { const onChange = jest.fn(); render(); - fireEvent.click(screen.getByText('Add')); + userEvent.click(screen.getByText('Add')); await waitFor(() => expect(onChange).toHaveBeenCalledTimes(1)); }); it('removes a field', async () => { const onChange = jest.fn(); - render(); + render(); - fireEvent.click((await screen.findAllByTitle('Remove field'))[0]); + userEvent.click((await screen.findAllByTitle('Remove field'))[0]); - await waitFor(() => expect(onChange).toHaveBeenCalledWith([testValue[1]])); + await waitFor(() => expect(onChange).toHaveBeenCalledWith([testFields[1]])); + }); + + it('validates duplicated field names', async () => { + const repeatedFields = [ + { + matcherRegex: '', + name: 'repeated', + }, + { + matcherRegex: '', + name: 'repeated', + }, + ]; + render(); + + userEvent.click(screen.getAllByPlaceholderText('Field name')[0]); + + expect(await screen.findAllByText('The name is already in use')).toHaveLength(2); + }); + + it('does not validate empty names as repeated', () => { + const repeatedFields = [ + { + matcherRegex: '', + name: '', + }, + { + matcherRegex: '', + name: '', + }, + ]; + render(); + + userEvent.click(screen.getAllByPlaceholderText('Field name')[0]); + + expect(screen.queryByText('The name is already in use')).not.toBeInTheDocument(); }); }); -const testValue = [ +const testFields = [ { matcherRegex: 'regex1', name: 'test1', diff --git a/public/app/plugins/datasource/loki/configuration/DerivedFields.tsx b/public/app/plugins/datasource/loki/configuration/DerivedFields.tsx index d513177b1de..e4370eb4a65 100644 --- a/public/app/plugins/datasource/loki/configuration/DerivedFields.tsx +++ b/public/app/plugins/datasource/loki/configuration/DerivedFields.tsx @@ -1,5 +1,5 @@ import { css } from '@emotion/css'; -import React, { useState } from 'react'; +import React, { useCallback, useState } from 'react'; import { GrafanaTheme2, VariableOrigin, DataLinkBuiltInVars } from '@grafana/data'; import { Button, useTheme2 } from '@grafana/ui'; @@ -20,16 +20,23 @@ const getStyles = (theme: GrafanaTheme2) => ({ }); type Props = { - value?: DerivedFieldConfig[]; + fields?: DerivedFieldConfig[]; onChange: (value: DerivedFieldConfig[]) => void; }; -export const DerivedFields = ({ value = [], onChange }: Props) => { +export const DerivedFields = ({ fields = [], onChange }: Props) => { const theme = useTheme2(); const styles = getStyles(theme); const [showDebug, setShowDebug] = useState(false); + const validateName = useCallback( + (name: string) => { + return fields.filter((field) => field.name && field.name === name).length <= 1; + }, + [fields] + ); + return ( <>

Derived fields

@@ -39,22 +46,23 @@ export const DerivedFields = ({ value = [], onChange }: Props) => {
- {value.map((field, index) => { + {fields.map((field, index) => { return ( { - const newDerivedFields = [...value]; + const newDerivedFields = [...fields]; newDerivedFields.splice(index, 1, newField); onChange(newDerivedFields); }} onDelete={() => { - const newDerivedFields = [...value]; + const newDerivedFields = [...fields]; newDerivedFields.splice(index, 1); onChange(newDerivedFields); }} + validateName={validateName} suggestions={[ { value: DataLinkBuiltInVars.valueRaw, @@ -75,14 +83,14 @@ export const DerivedFields = ({ value = [], onChange }: Props) => { icon="plus" onClick={(event) => { event.preventDefault(); - const newDerivedFields = [...value, { name: '', matcherRegex: '' }]; + const newDerivedFields = [...fields, { name: '', matcherRegex: '', urlDisplayLabel: '', url: '' }]; onChange(newDerivedFields); }} > Add - {value.length > 0 && ( + {fields.length > 0 && ( @@ -96,7 +104,7 @@ export const DerivedFields = ({ value = [], onChange }: Props) => { className={css` margin-bottom: 10px; `} - derivedFields={value} + derivedFields={fields} />
)}