aminghadersohi commented on code in PR #41136:
URL: https://github.com/apache/superset/pull/41136#discussion_r3476354982
##########
superset-frontend/packages/superset-ui-core/src/components/Select/Select.test.tsx:
##########
@@ -1146,6 +1146,53 @@ test('pasting an non-existent option should not add it
if allowNewOptions is fal
expect(await findAllSelectOptions()).toHaveLength(0);
});
+// Reference for the bug this tests:
https://github.com/apache/superset/issues/32645
+// Dashboard filters with "Dynamically search all filter values" only load a
+// page of options client-side, so a pasted value outside that page used to be
+// silently dropped. allowNewOptionsOnPaste keeps such values so the filter can
+// still apply them.
+test('keeps pasted values outside loaded options when allowNewOptionsOnPaste
is true', async () => {
Review Comment:
MEDIUM: this test pins label display (`.ant-select-selection-item`
textContent) but doesn't assert that `onChange` is called with the correct
value for `OutsideValue` — the value that actually reaches the filter query. A
regression in the value-submission path (e.g. if the early-return value were `{
label: item, value: '' }`) would still pass. The existing pattern at line 1067
(`test('fires onChange when pasting a selection', ...)`) shows how:
```ts
const onChange = jest.fn();
// render with onChange={onChange}
// ...after fireEvent paste + waitFor:
expect(onChange).toHaveBeenCalledWith(
expect.arrayContaining([expect.objectContaining({ value: 'OutsideValue'
})]),
expect.anything(),
);
```
##########
superset-frontend/packages/superset-ui-core/src/components/Select/Select.test.tsx:
##########
@@ -1146,6 +1146,53 @@ test('pasting an non-existent option should not add it
if allowNewOptions is fal
expect(await findAllSelectOptions()).toHaveLength(0);
});
+// Reference for the bug this tests:
https://github.com/apache/superset/issues/32645
+// Dashboard filters with "Dynamically search all filter values" only load a
+// page of options client-side, so a pasted value outside that page used to be
+// silently dropped. allowNewOptionsOnPaste keeps such values so the filter can
+// still apply them.
+test('keeps pasted values outside loaded options when allowNewOptionsOnPaste
is true', async () => {
+ render(
+ <Select
+ {...defaultProps}
+ mode="multiple"
+ allowNewOptions={false}
+ allowNewOptionsOnPaste
+ />,
+ );
+ const input = getElementByClassName('.ant-select-selection-search-input');
+ const paste = createEvent.paste(input, {
+ clipboardData: {
+ // Liam is a loaded option; OutsideValue is not in the loaded page.
+ getData: () => 'Liam,OutsideValue',
+ },
+ });
+ fireEvent(input, paste);
+ await waitFor(() => {
+ const values = [
+ ...getElementsByClassName('.ant-select-selection-item'),
+ ].map(value => value.textContent);
+ expect(values).toEqual(expect.arrayContaining(['Liam', 'OutsideValue']));
Review Comment:
NIT: `arrayContaining` without a length guard passes even if extra items are
present. Consider adding `expect(values).toHaveLength(2)` alongside, or use
`.toEqual(['Liam', 'OutsideValue'])` if insertion order is stable (the paste
handler appends, so 'Liam' — a loaded option resolved via `getPastedTextValue`
— arrives before 'OutsideValue', making order deterministic).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
