sadpandajoe commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4162962816
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -564,32 +629,32 @@ export default function PluginFilterSelect(props:
PluginFilterSelectProps) {
updateDataMaskRef.current = updateDataMask;
}, [updateDataMask]);
- const debouncedLikeChange = useMemo(
- () =>
- debounce((text: string) => {
- if (text) {
- updateDataMaskRef.current([text]);
- } else {
- updateDataMaskRef.current(null);
- }
- }, Constants.SLOW_DEBOUNCE),
- [],
- );
-
+ // Leaving a LIKE operator discards the pending edit so returning to one
+ // doesn't replay text typed under the previous mode.
useEffect(() => {
- if (!isLikeOperator || clearAllTrigger) {
- debouncedLikeChange.cancel();
+ if (!isLikeOperator) {
+ settleLikeValue.cancel();
+ setPendingLikeValue(null);
}
- }, [clearAllTrigger, debouncedLikeChange, isLikeOperator]);
+ }, [isLikeOperator, settleLikeValue]);
Review Comment:
Changing a configured Contains default to Starts with clears the form's
default, but both modes leave isLikeOperator true and retain the old pending
text. Pasting the same value into the cleared input then changes none of this
effect's dependencies, so the input shows a value while the default remains
unset; could every match-type change invalidate the pending edit?
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:
##########
@@ -2052,30 +2051,152 @@ test('pending LIKE debounce is canceled when
operatorType switches back to Exact
reduxState,
);
- fireEvent.change(
- screen.getByPlaceholderText('Type to search (contains)...'),
- {
- target: { value: 'Jen' },
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
+ const callsBeforeSwitch = setDataMaskMock.mock.calls.length;
+
+ // Same batching as the sibling test above: the keystroke and the switch
+ // away from the LIKE operator land in the same update, before the
+ // deferred value has a chance to settle on its own.
+ act(() => {
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin
+ {...buildSelectFilterProps({
+ formData: { operatorType: SelectFilterOperatorType.Exact },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ })}
+ />,
+ );
+ });
+
+ await act(async () => {});
+
+ // Rerendering with fresh (non-memoized) props can independently re-run
+ // this plugin's own default-value initialisation, unrelated to the LIKE
+ // debounce this test is pinning -- so assert no *new* call carries the
+ // 'Jen' ILIKE payload, rather than that setDataMask was never called again.
+ const callsSinceSwitch = setDataMaskMock.mock.calls
+ .slice(callsBeforeSwitch)
+ .map(call => call[0]);
+ expect(callsSinceSwitch).not.toContainEqual(
+ expect.objectContaining({
+ extraFormData: expect.objectContaining({
+ filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })],
+ }),
+ }),
+ );
+});
+
+test('switching Exact then back to LIKE does not replay the discarded pending
edit', async () => {
+ jest.useFakeTimers({ advanceTimers: true });
+ const setDataMaskMock = jest.fn();
+ const buildProps = (operatorType: SelectFilterOperatorType) =>
+ buildSelectFilterProps({
+ formData: { operatorType },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ });
+ const reduxState = {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: { 'test-filter': { name: 'Test Filter' } },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: undefined },
+ },
+ },
},
+ };
+
+ const { rerender } = render(
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Contains)} />,
+ reduxState,
);
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
- setDataMaskMock.mockClear();
+ act(() => {
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Exact)} />,
+ );
+ });
+ await act(async () => {});
+ const callsBeforeReturn = setDataMaskMock.mock.calls.length;
rerender(
- <SelectFilterPlugin
- {...buildSelectFilterProps({
- formData: { operatorType: SelectFilterOperatorType.Exact },
- filterState: { value: undefined },
- setDataMask: setDataMaskMock,
- })}
- />,
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Contains)} />,
+ );
+ await act(async () => {});
+
+ expect(
+ setDataMaskMock.mock.calls.slice(callsBeforeReturn).map(call => call[0]),
+ ).not.toContainEqual(
+ expect.objectContaining({
+ extraFormData: expect.objectContaining({
+ filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })],
+ }),
+ }),
+ );
+});
+
+test('clear-all does not restore a LIKE edit still pending in the deferred
value', async () => {
+ jest.useFakeTimers({ advanceTimers: true });
+ const setDataMaskMock = jest.fn();
+ const likeProps = buildSelectFilterProps({
+ formData: { operatorType: SelectFilterOperatorType.Contains },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ });
+ const reduxState = {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: { 'test-filter': { name: 'Test Filter' } },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: undefined },
+ },
+ },
+ },
+ };
+
+ const { rerender } = render(
+ <SelectFilterPlugin {...likeProps} />,
+ reduxState,
);
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
+ // Type and fire clear-all in one update, then release the trigger, so the
+ // deferred value still holds the typed text when the trigger goes away.
act(() => {
- jest.advanceTimersByTime(500);
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin
+ {...likeProps}
+ clearAllTrigger={{ 'test-filter': true }}
+ />,
+ );
});
+ const callsBeforeRelease = setDataMaskMock.mock.calls.length;
+ rerender(<SelectFilterPlugin {...likeProps} clearAllTrigger={{}} />);
Review Comment:
The empty object is still truthy, so this rerender keeps both the reset
effect and the LIKE dispatch guard active rather than exercising Clear All
completion. Could the test release the trigger with a falsy value and assert
that a previously typed predicate is not restored after acknowledgement?
--
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]
