sadpandajoe commented on code in PR #44668: URL: https://github.com/apache/superset/pull/44668#discussion_r4172485741
########## superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlForm.reset.test.tsx: ########## @@ -0,0 +1,115 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { render, screen, userEvent } from 'spec/helpers/testing-library'; +import ControlForm, { + ControlFormItem, + ControlFormRow, +} from 'src/explore/components/controls/ColumnConfigControl/ControlForm'; + +const CHECKBOX = ( + <ControlFormItem + name="showCellBars" + controlType="Checkbox" + label="Show cell bars" + description="Whether to display a bar chart background in table columns" + defaultValue + debounceDelay={0} + resettable + /> +); + +const setup = (props = {}) => ( + <ControlForm + onChange={jest.fn()} + onReset={jest.fn()} + value={{ showCellBars: false }} + {...props} + > + <ControlFormRow>{CHECKBOX}</ControlFormRow> + </ControlForm> +); + +// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks +describe('ColumnConfigControl reset to chart-level setting', () => { + test('shows the reset action only for an explicit per-column override', () => { + const { rerender } = render(setup()); + expect( + screen.getByRole('button', { name: /use the chart-level setting/i }), + ).toBeInTheDocument(); + + // no explicit value in the column config: the column already follows + // the chart-level option, so no reset action is offered + rerender(setup({ value: {} })); + expect( + screen.queryByRole('button', { name: /use the chart-level setting/i }), + ).not.toBeInTheDocument(); + }); + + test('reset deletes the key instead of writing an explicit value', async () => { + const onChange = jest.fn(); + const onReset = jest.fn(); + render( + <ControlForm + onChange={onChange} + onReset={onReset} + value={{ showCellBars: false }} + > + <ControlFormRow>{CHECKBOX}</ControlFormRow> + </ControlForm>, + ); + + await userEvent.click( + screen.getByRole('button', { name: /use the chart-level setting/i }), + ); + expect(onReset).toHaveBeenCalledWith('showCellBars'); + // the key must be gone from the propagated value, not set to true/false Review Comment: This deletion test passes even if the popover never removes the override: `onReset` is a no-op mock and `onChange` is never called. Could it exercise `ColumnConfigPopover` with `{ showCellBars: false, d3NumberFormat: ".2f" }` and assert that resetting emits `{ d3NumberFormat: ".2f" }`, preserving the unrelated setting while removing the override? -- 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]
