From 1ae6aedf0721f08096002c23a5eb1c2bc447b28e Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Fri, 27 Jan 2023 12:03:39 -0600 Subject: [PATCH] Validate custom column names (#1013) --- packages/iris-grid/src/IrisGrid.tsx | 6 +- .../src/sidebar/CustomColumnBuilder.test.tsx | 215 ++++++++++++++++++ .../src/sidebar/CustomColumnBuilder.tsx | 49 ++-- .../src/sidebar/CustomColumnInput.test.tsx | 72 ++++++ .../src/sidebar/CustomColumnInput.tsx | 170 +++++++------- .../iris-grid/src/sidebar/InputEditor.tsx | 17 +- 6 files changed, 412 insertions(+), 117 deletions(-) create mode 100644 packages/iris-grid/src/sidebar/CustomColumnBuilder.test.tsx create mode 100644 packages/iris-grid/src/sidebar/CustomColumnInput.test.tsx diff --git a/packages/iris-grid/src/IrisGrid.tsx b/packages/iris-grid/src/IrisGrid.tsx index 65a6652dee..34b0f23914 100644 --- a/packages/iris-grid/src/IrisGrid.tsx +++ b/packages/iris-grid/src/IrisGrid.tsx @@ -2979,7 +2979,11 @@ export class IrisGrid extends Component { } this.setState({ customColumns }); - this.startLoading('Applying custom columns...'); + if (customColumns.length > 0) { + // If there are no custom columns, the change handler never fires + // This causes the loader to stay until canceled by the user + this.startLoading('Applying custom columns...'); + } } handleCustomColumnsChanged(): void { diff --git a/packages/iris-grid/src/sidebar/CustomColumnBuilder.test.tsx b/packages/iris-grid/src/sidebar/CustomColumnBuilder.test.tsx new file mode 100644 index 0000000000..1e366f038f --- /dev/null +++ b/packages/iris-grid/src/sidebar/CustomColumnBuilder.test.tsx @@ -0,0 +1,215 @@ +import React from 'react'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { EventShimCustomEvent } from '@deephaven/utils'; +import CustomColumnBuilder, { + CustomColumnBuilderProps, +} from './CustomColumnBuilder'; +import IrisGridTestUtils from '../IrisGridTestUtils'; +import IrisGridModel from '../IrisGridModel'; + +function Builder({ + model = IrisGridTestUtils.makeModel(), + customColumns = [], + onSave = jest.fn(), + onCancel = jest.fn(), +}: Partial) { + return ( + + ); +} + +test('Renders the default state', async () => { + render(); + expect(screen.getByPlaceholderText('Column Name')).toBeInTheDocument(); + expect(screen.getByText('Column Formula')).toBeInTheDocument(); +}); + +test('Calls on save', async () => { + const user = userEvent.setup(); + const customColumns = ['abc=def', 'foo=bar']; + const mockSave = jest.fn(); + render(); + + await user.type(screen.getByDisplayValue('abc'), 'cba'); + await user.click(screen.getByText(/Save/)); + expect(mockSave).toHaveBeenLastCalledWith(['abccba=def', 'foo=bar']); +}); + +test('Switches to loader button while saving', async () => { + jest.useFakeTimers(); + const user = userEvent.setup({ delay: null }); + const model = IrisGridTestUtils.makeModel(); + const mockSave = jest.fn(() => + setTimeout(() => { + model.dispatchEvent( + new EventShimCustomEvent(IrisGridModel.EVENT.COLUMNS_CHANGED) + ); + }, 50) + ); + + render( + + ); + + await user.click(screen.getByText(/Save/)); + expect(screen.getByText('Applying')).toBeInTheDocument(); + jest.advanceTimersByTime(50); + expect(screen.getByText('Success')).toBeInTheDocument(); + jest.advanceTimersByTime(CustomColumnBuilder.SUCCESS_SHOW_DURATION); + expect(screen.getByText(/Save/)).toBeInTheDocument(); + + // Component should ignore this event and not change the save button + model.dispatchEvent( + new EventShimCustomEvent(IrisGridModel.EVENT.COLUMNS_CHANGED) + ); + expect(screen.getByText(/Save/)).toBeInTheDocument(); + jest.useRealTimers(); +}); + +test('Adds a column', async () => { + const user = userEvent.setup(); + render(); + + await user.click(screen.getByText('Add Another Column')); + expect(screen.getAllByPlaceholderText('Column Name').length).toBe(2); + expect(screen.getAllByText('Column Formula').length).toBe(2); +}); + +test('Ignores empty names or formulas on save', async () => { + const user = userEvent.setup(); + const customColumns = ['foo=bar']; + const mockSave = jest.fn(); + render(); + + await user.click(screen.getByText('Add Another Column')); + await user.type(screen.getAllByPlaceholderText('Column Name')[1], 'test'); + await user.click(screen.getByText(/Save/)); + expect(mockSave).toBeCalledWith(customColumns); +}); + +test('Ignores deleted formulas on save', async () => { + // There is an issue with populating the custom columns and then editing the existing column + // RTL/monaco aren't playing nicely and it won't edit the existing text + // This test instead creates the new text, saves, then removes it to test the same behavior + jest.useFakeTimers(); + const user = userEvent.setup({ delay: null }); + const model = IrisGridTestUtils.makeModel(); + const mockSave = jest.fn(() => + setTimeout(() => { + model.dispatchEvent( + new EventShimCustomEvent(IrisGridModel.EVENT.COLUMNS_CHANGED) + ); + }, 50) + ); + + const { container } = render(); + + await user.type(screen.getByPlaceholderText('Column Name'), 'foo'); + await user.click(container.querySelectorAll('.input-editor-wrapper')[0]); + await user.keyboard('bar'); + + await user.click(screen.getByText(/Save/)); + jest.advanceTimersByTime(50); // Applying duration + jest.advanceTimersByTime(CustomColumnBuilder.SUCCESS_SHOW_DURATION); + + expect(mockSave).toBeCalledWith(['foo=bar']); + + mockSave.mockClear(); + + await user.click(container.querySelectorAll('.input-editor-wrapper')[0]); + await user.keyboard('{Control>}a{/Control}{Backspace}'); + await user.click(screen.getByText(/Save/)); + expect(mockSave).toBeCalledWith([]); + + jest.useRealTimers(); +}); + +test('Deletes columns', async () => { + const user = userEvent.setup(); + const customColumns = ['abc=def', 'foo=bar']; + render(); + + await user.click(screen.getAllByLabelText(/Delete/)[0]); + expect(screen.queryByDisplayValue('abc')).toBeNull(); + expect(screen.queryByDisplayValue('def')).toBeNull(); + expect(screen.getByDisplayValue('foo')).toBeInTheDocument(); + expect(screen.getByDisplayValue('bar')).toBeInTheDocument(); + + await user.click(screen.getByLabelText(/Delete/)); + expect(screen.queryByDisplayValue('foo')).toBeNull(); + expect(screen.queryByDisplayValue('bar')).toBeNull(); + expect(screen.getByPlaceholderText('Column Name')).toBeInTheDocument(); + expect(screen.getByText('Column Formula')).toBeInTheDocument(); +}); + +test('Displays request failure message', async () => { + const user = userEvent.setup(); + const model = IrisGridTestUtils.makeModel(); + render(); + + // Should ignore this since not in saving state + model.dispatchEvent( + new EventShimCustomEvent(IrisGridModel.EVENT.REQUEST_FAILED, { + detail: { errorMessage: 'Error message' }, + }) + ); + expect(screen.queryByText(/Error message/)).toBeNull(); + + await user.click(screen.getByText(/Save/)); + model.dispatchEvent( + new EventShimCustomEvent(IrisGridModel.EVENT.REQUEST_FAILED, { + detail: { errorMessage: 'Error message' }, + }) + ); + + expect(screen.getByText(/Error message/)).toBeInTheDocument(); + + const input = screen.getByDisplayValue('foo'); + await user.click(input); + expect(input).not.toHaveClass('is-invalid'); +}); + +test('Handles focus changes via keyboard', async () => { + const user = userEvent.setup(); + const { container } = render( + + ); + + const nameInputs = screen.getAllByPlaceholderText('Column Name'); + const formulaInputs = container.querySelectorAll( + '.input-editor-wrapper textarea' + ); + const deleteButtons = screen.getAllByLabelText(/Delete/); + const dragHandles = screen.getAllByLabelText(/Drag/); + await user.click(nameInputs[0]); + + await user.keyboard('{Tab}'); + expect(deleteButtons[0]).toHaveFocus(); + await user.keyboard('{Tab}'); + expect(dragHandles[0]).toHaveFocus(); + await user.keyboard('{Tab}'); + expect(formulaInputs[0]).toHaveFocus(); + + await user.keyboard('{Tab}'); + expect(nameInputs[1]).toHaveFocus(); + await user.keyboard('{Tab}'); + expect(deleteButtons[1]).toHaveFocus(); + await user.keyboard('{Tab}'); + expect(dragHandles[1]).toHaveFocus(); + await user.keyboard('{Tab}'); + expect(formulaInputs[1]).toHaveFocus(); + + await user.keyboard('{Tab}'); + expect(screen.getByText('Add Another Column')).toHaveFocus(); + + await user.keyboard('{Shift>}{Tab}{/Shift}'); + expect(formulaInputs[1]).toHaveFocus(); + await user.keyboard('{Shift>}{Tab}{/Shift}'); + expect(dragHandles[1]).toHaveFocus(); +}); diff --git a/packages/iris-grid/src/sidebar/CustomColumnBuilder.tsx b/packages/iris-grid/src/sidebar/CustomColumnBuilder.tsx index 5951d54b85..554890b74e 100644 --- a/packages/iris-grid/src/sidebar/CustomColumnBuilder.tsx +++ b/packages/iris-grid/src/sidebar/CustomColumnBuilder.tsx @@ -6,6 +6,7 @@ import { DragDropContext, Droppable, DropResult } from 'react-beautiful-dnd'; import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'; import { Button, DragUtils, LoadingSpinner } from '@deephaven/components'; import { dhNewCircleLargeFilled, vsWarning, vsPass } from '@deephaven/icons'; +import { DbNameValidator } from '@deephaven/utils'; import CustomColumnInput from './CustomColumnInput'; import './CustomColumnBuilder.scss'; import IrisGridModel from '../IrisGridModel'; @@ -17,7 +18,7 @@ type Input = { name: string; formula: string; }; -interface CustomColumnBuilderProps { +export interface CustomColumnBuilderProps { model: IrisGridModel; customColumns: string[]; onSave: (columns: string[]) => void; @@ -37,12 +38,6 @@ class CustomColumnBuilder extends Component< > { static SUCCESS_SHOW_DURATION = 750; - static defaultProps = { - customColumns: [], - onSave: (): void => undefined, - onCancel: (): void => undefined, - }; - static makeCustomColumnInputEventKey(): string { return shortid.generate(); } @@ -265,15 +260,15 @@ class CustomColumnBuilder extends Component< const { inputs } = this.state; // focus on drag handle if (shiftKey) { - (this.container?.querySelectorAll(`.btn-drag-handle`)[ + (this.container?.querySelectorAll('.btn-drag-handle')[ focusEditorIndex ] as HTMLButtonElement).focus(); return; } if (focusEditorIndex === inputs.length - 1) { - (this.container?.querySelectorAll(`.btn-add-column`)[ - focusEditorIndex - ] as HTMLButtonElement).focus(); + (this.container?.querySelector( + '.btn-add-column' + ) as HTMLButtonElement)?.focus(); } else { // focus on next column name input const nextFocusIndex = focusEditorIndex + 1; @@ -284,7 +279,7 @@ class CustomColumnBuilder extends Component< } handleSaveClick(): void { - const { onSave } = this.props; + const { onSave, customColumns: originalCustomColumns } = this.props; const { inputs, isCustomColumnApplying } = this.state; if (isCustomColumnApplying) { return; @@ -297,7 +292,11 @@ class CustomColumnBuilder extends Component< } }); this.resetErrorMessage(); - this.setState({ isCustomColumnApplying: true }); + this.setState({ + // If both are 0, then moving from no custom to no custom. The parent won't re-render to cancel the loading state + isCustomColumnApplying: + customColumns.length > 0 || originalCustomColumns.length > 0, + }); onSave(customColumns); } @@ -313,8 +312,14 @@ class CustomColumnBuilder extends Component< renderInputs(): ReactElement[] { const { inputs, hasRequestFailed } = this.state; + const nameCount = new Map(); + inputs.forEach(({ name }) => + nameCount.set(name, (nameCount.get(name) ?? 0) + 1) + ); + return inputs.map((input, index) => { const { eventKey, name, formula } = input; + const isDuplicate = (nameCount.get(name) ?? 0) > 1; return ( ); }); @@ -334,6 +340,13 @@ class CustomColumnBuilder extends Component< renderSaveButton(): ReactElement { const { inputs, isCustomColumnApplying, isSuccessShowing } = this.state; const saveText = inputs.length > 1 ? 'Save Columns' : 'Save Column'; + const areNamesValid = inputs.every( + ({ name }) => name === '' || DbNameValidator.isValidColumnName(name) + ); + const filteredNames = inputs + .filter(({ name }) => name !== '') + .map(({ name }) => name); + const areNamesUnique = new Set(filteredNames).size === filteredNames.length; return ( + icon={vsGripper} + tooltip="Drag column to re-order" + /> - + {(!isValidName || isDuplicate) && ( +

+ {!isValidName ? 'Invalid name' : 'Duplicate name'} +

+ )} -
+ - )} - - ); - } +
+ + )} + + ); } export default CustomColumnInput; diff --git a/packages/iris-grid/src/sidebar/InputEditor.tsx b/packages/iris-grid/src/sidebar/InputEditor.tsx index 01cc83b882..4d3621e927 100644 --- a/packages/iris-grid/src/sidebar/InputEditor.tsx +++ b/packages/iris-grid/src/sidebar/InputEditor.tsx @@ -103,6 +103,9 @@ export default class InputEditor extends Component< // disable tab to spaces in this editor to improve tab navigation this.editor.getModel()?.updateOptions({ tabSize: 0 }); + // monaco does not propagate tab or enter events + this.editor.onKeyDown(this.handleKeyDown); + this.editor.onDidChangeModelContent(this.handleContentChanged); this.editor.onDidFocusEditorText(this.handleEditorFocus); this.editor.onDidBlurEditorText(this.handleEditorBlur); @@ -116,7 +119,7 @@ export default class InputEditor extends Component< handleContentChanged(): void { const { onContentChanged } = this.props; const value = this.editor?.getModel()?.getValue(); - if (value !== '' && value !== undefined) { + if (value !== undefined) { this.setState({ isEditorEmpty: value.length === 0 }); } onContentChanged(value); @@ -127,10 +130,7 @@ export default class InputEditor extends Component< } handleEditorBlur(): void { - const value = this.editor?.getModel()?.getValue(); - if (value === undefined || value === '') { - throw new Error('value is undefined'); - } + const value = this.editor?.getModel()?.getValue() ?? ''; this.setState({ isEditorEmpty: value.length === 0, isEditorFocused: false, @@ -146,9 +146,11 @@ export default class InputEditor extends Component< this.editor?.focus(); } - handleKeyDown(event: React.KeyboardEvent): void { + handleKeyDown(event: monaco.IKeyboardEvent): void { const { onTab, editorIndex } = this.props; - if (event.key === 'Tab') { + if (event.code === 'Tab') { + event.stopPropagation(); + event.preventDefault(); onTab(editorIndex, event.shiftKey); } } @@ -162,7 +164,6 @@ export default class InputEditor extends Component< focused: isEditorFocused, invalid, })} - onKeyDown={this.handleKeyDown} role="presentation" onClick={this.handleContainerClick} >