Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Duplicate items when pasting into Select #25447

Merged
merged 3 commits into from
Sep 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions superset-frontend/src/components/Select/AsyncSelect.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -858,6 +858,45 @@ test('does not duplicate options when using numeric values', async () => {
await waitFor(() => expect(getAllSelectOptions().length).toBe(1));
});

test('pasting an existing option does not duplicate it', async () => {
const options = jest.fn(async () => ({
data: [OPTIONS[0]],
totalCount: 1,
}));
render(<AsyncSelect {...defaultProps} options={options} />);
await open();
const input = getElementByClassName('.ant-select-selection-search-input');
const paste = createEvent.paste(input, {
clipboardData: {
getData: () => OPTIONS[0].label,
},
});
fireEvent(input, paste);
expect(await findAllSelectOptions()).toHaveLength(1);
});

test('pasting an existing option does not duplicate it in multiple mode', async () => {
const options = jest.fn(async () => ({
data: [
{ label: 'John', value: 1 },
{ label: 'Liam', value: 2 },
{ label: 'Olivia', value: 3 },
],
totalCount: 3,
}));
render(<AsyncSelect {...defaultProps} options={options} mode="multiple" />);
await open();
const input = getElementByClassName('.ant-select-selection-search-input');
const paste = createEvent.paste(input, {
clipboardData: {
getData: () => 'John,Liam,Peter',
},
});
fireEvent(input, paste);
// Only Peter should be added
expect(await findAllSelectOptions()).toHaveLength(4);
});

/*
TODO: Add tests that require scroll interaction. Needs further investigation.
- Fetches more data when scrolling and more data is available
Expand Down
26 changes: 21 additions & 5 deletions superset-frontend/src/components/Select/AsyncSelect.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ import {
dropDownRenderHelper,
handleFilterOptionHelper,
mapOptions,
getOption,
isObject,
} from './utils';
import {
AsyncSelectProps,
Expand Down Expand Up @@ -523,19 +525,33 @@ const AsyncSelect = forwardRef(
[ref],
);

const getPastedTextValue = useCallback(
(text: string) => {
const option = getOption(text, fullSelectOptions, true);
const value: AntdLabeledValue = {
label: text,
value: text,
};
if (option) {
value.label = isObject(option) ? option.label : option;
value.value = isObject(option) ? option.value! : option;
}
return value;
},
[fullSelectOptions],
);

const onPaste = (e: ClipboardEvent<HTMLInputElement>) => {
const pastedText = e.clipboardData.getData('text');
if (isSingleMode) {
setSelectValue({ label: pastedText, value: pastedText });
setSelectValue(getPastedTextValue(pastedText));
} else {
const token = tokenSeparators.find(token => pastedText.includes(token));
const array = token ? uniq(pastedText.split(token)) : [pastedText];
const values = array.map(item => getPastedTextValue(item));
setSelectValue(previous => [
...((previous || []) as AntdLabeledValue[]),
...array.map<AntdLabeledValue>(value => ({
label: value,
value,
})),
...values,
]);
}
};
Expand Down
39 changes: 39 additions & 0 deletions superset-frontend/src/components/Select/Select.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -972,6 +972,45 @@ test('does not duplicate options when using numeric values', async () => {
await waitFor(() => expect(getAllSelectOptions().length).toBe(1));
});

test('pasting an existing option does not duplicate it', async () => {
render(<Select {...defaultProps} options={[OPTIONS[0]]} />);
await open();
const input = getElementByClassName('.ant-select-selection-search-input');
const paste = createEvent.paste(input, {
clipboardData: {
getData: () => OPTIONS[0].label,
},
});
fireEvent(input, paste);
expect(await findAllSelectOptions()).toHaveLength(1);
});

test('pasting an existing option does not duplicate it in multiple mode', async () => {
const options = [
{ label: 'John', value: 1 },
{ label: 'Liam', value: 2 },
{ label: 'Olivia', value: 3 },
];
render(
<Select
{...defaultProps}
options={options}
mode="multiple"
allowSelectAll={false}
/>,
);
await open();
const input = getElementByClassName('.ant-select-selection-search-input');
const paste = createEvent.paste(input, {
clipboardData: {
getData: () => 'John,Liam,Peter',
},
});
fireEvent(input, paste);
// Only Peter should be added
expect(await findAllSelectOptions()).toHaveLength(4);
});

/*
TODO: Add tests that require scroll interaction. Needs further investigation.
- Fetches more data when scrolling and more data is available
Expand Down
33 changes: 25 additions & 8 deletions superset-frontend/src/components/Select/Select.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ import {
mapValues,
mapOptions,
hasCustomLabels,
getOption,
isObject,
} from './utils';
import { RawValue, SelectOptionsType, SelectProps } from './types';
import {
Expand Down Expand Up @@ -530,27 +532,42 @@ const Select = forwardRef(
actualMaxTagCount -= 1;
}

const getPastedTextValue = useCallback(
(text: string) => {
const option = getOption(text, fullSelectOptions, true);
if (labelInValue) {
const value: AntdLabeledValue = {
label: text,
value: text,
};
if (option) {
value.label = isObject(option) ? option.label : option;
value.value = isObject(option) ? option.value! : option;
}
return value;
}
return option ? (isObject(option) ? option.value! : option) : text;
},
[fullSelectOptions, labelInValue],
);

const onPaste = (e: ClipboardEvent<HTMLInputElement>) => {
const pastedText = e.clipboardData.getData('text');
if (isSingleMode) {
setSelectValue(
labelInValue ? { label: pastedText, value: pastedText } : pastedText,
);
setSelectValue(getPastedTextValue(pastedText));
} else {
const token = tokenSeparators.find(token => pastedText.includes(token));
const array = token ? uniq(pastedText.split(token)) : [pastedText];
const values = array.map(item => getPastedTextValue(item));
if (labelInValue) {
setSelectValue(previous => [
...((previous || []) as AntdLabeledValue[]),
...array.map<AntdLabeledValue>(value => ({
label: value,
value,
})),
...(values as AntdLabeledValue[]),
]);
} else {
setSelectValue(previous => [
...((previous || []) as string[]),
...array,
...(values as string[]),
]);
}
}
Expand Down
28 changes: 17 additions & 11 deletions superset-frontend/src/components/Select/utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,27 +49,33 @@ export function getValue(
return isLabeledValue(option) ? option.value : option;
}

export function hasOption(
export function getOption(
value: V,
options?: V | LabeledValue | (V | LabeledValue)[],
checkLabel = false,
): boolean {
): V | LabeledValue {
const optionsArray = ensureIsArray(options);
// When comparing the values we use the equality
// operator to automatically convert different types
return (
optionsArray.find(
x =>
return optionsArray.find(
x =>
// eslint-disable-next-line eqeqeq
x == value ||
(isObject(x) &&
// eslint-disable-next-line eqeqeq
x == value ||
(isObject(x) &&
// eslint-disable-next-line eqeqeq
(('value' in x && x.value == value) ||
(checkLabel && 'label' in x && x.label === value))),
) !== undefined
(('value' in x && x.value == value) ||
(checkLabel && 'label' in x && x.label === value))),
);
}

export function hasOption(
value: V,
options?: V | LabeledValue | (V | LabeledValue)[],
checkLabel = false,
): boolean {
return getOption(value, options, checkLabel) !== undefined;
}

/**
* It creates a comparator to check for a specific property.
* Can be used with string and number property values.
Expand Down
Loading