Skip to content

Commit

Permalink
Validate custom column names (#1013)
Browse files Browse the repository at this point in the history
  • Loading branch information
mattrunyon authored Jan 27, 2023
1 parent 8a8d04e commit 1ae6aed
Show file tree
Hide file tree
Showing 6 changed files with 412 additions and 117 deletions.
6 changes: 5 additions & 1 deletion packages/iris-grid/src/IrisGrid.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2979,7 +2979,11 @@ export class IrisGrid extends Component<IrisGridProps, IrisGridState> {
}

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 {
Expand Down
215 changes: 215 additions & 0 deletions packages/iris-grid/src/sidebar/CustomColumnBuilder.test.tsx
Original file line number Diff line number Diff line change
@@ -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<CustomColumnBuilderProps>) {
return (
<CustomColumnBuilder
model={model}
customColumns={customColumns}
onSave={onSave}
onCancel={onCancel}
/>
);
}

test('Renders the default state', async () => {
render(<Builder />);
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(<Builder onSave={mockSave} customColumns={customColumns} />);

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(
<Builder model={model} onSave={mockSave} customColumns={['foo=bar']} />
);

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(<Builder />);

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(<Builder customColumns={customColumns} onSave={mockSave} />);

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(<Builder model={model} onSave={mockSave} />);

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(<Builder customColumns={customColumns} />);

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(<Builder model={model} customColumns={['foo=bar']} />);

// 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(
<Builder customColumns={['abc=bar', 'foo=bar']} />
);

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();
});
49 changes: 33 additions & 16 deletions packages/iris-grid/src/sidebar/CustomColumnBuilder.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -17,7 +18,7 @@ type Input = {
name: string;
formula: string;
};
interface CustomColumnBuilderProps {
export interface CustomColumnBuilderProps {
model: IrisGridModel;
customColumns: string[];
onSave: (columns: string[]) => void;
Expand All @@ -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();
}
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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);
}

Expand All @@ -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 (
<CustomColumnInput
key={eventKey}
Expand All @@ -326,6 +331,7 @@ class CustomColumnBuilder extends Component<
onDeleteColumn={this.handleDeleteColumn}
onTabInEditor={this.handleEditorTabNavigation}
invalid={hasRequestFailed}
isDuplicate={isDuplicate}
/>
);
});
Expand All @@ -334,21 +340,32 @@ 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 (
<Button
kind={isSuccessShowing ? 'success' : 'primary'}
className={classNames('btn-apply', {
'btn-spinner': isCustomColumnApplying,
})}
disabled={isSuccessShowing || isCustomColumnApplying}
disabled={
isSuccessShowing ||
isCustomColumnApplying ||
!areNamesValid ||
!areNamesUnique
}
onClick={this.handleSaveClick}
>
{isCustomColumnApplying && (
<span>
<LoadingSpinner />
<span className="btn-normal-content">Applying</span>
<span className="btn-hover-content">Applying</span>
</span>
)}
{!isSuccessShowing && !isCustomColumnApplying && saveText}
Expand All @@ -361,7 +378,7 @@ class CustomColumnBuilder extends Component<
);
}

render(): ReactElement {
render(): JSX.Element {
const { onCancel } = this.props;
const { errorMessage } = this.state;
return (
Expand Down
Loading

0 comments on commit 1ae6aed

Please sign in to comment.