sadpandajoe commented on code in PR #44668:
URL: https://github.com/apache/superset/pull/44668#discussion_r4409429007
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigPopover.tsx:
##########
@@ -79,7 +79,17 @@ export default function ColumnConfigPopover({
key: i.toString(),
label: item.tab,
children: (
- <ControlForm onChange={onChange} value={column.config}>
+ <ControlForm
+ onChange={onChange}
+ onReset={name =>
Review Comment:
Resetting an override rebuilds the config from the committed
`column.config`, so it ignores edits ControlForm is still holding in its
debounce buffer. If someone changes Min width from 100 to 120 and clicks "Use
the chart-level setting" within the debounce window, this emits width 100, the
resulting prop update cancels the pending write, and the input can still show
120 while 100 is what gets saved (same closure again at line 106). Should the
key deletion live in ControlForm, starting from its latest pending value, so
unrelated pending edits survive and the two call sites share one implementation?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlForm.reset.test.tsx:
##########
@@ -0,0 +1,148 @@
+/**
+ * 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 { GenericDataType } from '@apache-superset/core/common';
+import { render, screen, userEvent } from 'spec/helpers/testing-library';
+import ColumnConfigPopover from
'src/explore/components/controls/ColumnConfigControl/ColumnConfigPopover';
+import ControlForm, {
+ ControlFormItem,
+ ControlFormRow,
+} from 'src/explore/components/controls/ColumnConfigControl/ControlForm';
+import { ColumnConfigFormLayout } from
'src/explore/components/controls/ColumnConfigControl/types';
+
+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
+ expect(onChange).not.toHaveBeenCalledWith(
+ expect.objectContaining({ showCellBars: expect.anything() }),
+ );
+ });
+
+ test('reset through the popover removes only the reset override', async ()
=> {
+ // ControlForm alone only proves the callback fires. The deletion happens
in the
+ // popover, so exercising ControlForm with a mocked onReset stays green
even if that
+ // filter never runs. Render the popover and assert on the value it emits.
+ const onChange = jest.fn();
+ render(
+ <ColumnConfigPopover
+ column={{
+ isChildColumn: false,
+ isTimeComparisonColumn: false,
+ name: 'my_column',
+ type: GenericDataType.String,
+ config: { showCellBars: false, d3NumberFormat: '.2f' },
+ }}
+ configFormLayout={
+ {
+ [GenericDataType.String]: [['showCellBars', 'd3NumberFormat']],
+ } as ColumnConfigFormLayout
+ }
+ onChange={onChange}
+ />,
+ );
+
+ await userEvent.click(
+ screen.getByRole('button', { name: /use the chart-level setting/i }),
+ );
+ // the override is gone, the unrelated formatting override survives
+ expect(onChange).toHaveBeenCalledWith({ d3NumberFormat: '.2f' });
Review Comment:
`toHaveBeenCalledWith` treats an `undefined`-valued property as equal to a
missing one, so this still passes if the popover emits `{ d3NumberFormat:
'.2f', showCellBars: undefined }` instead of deleting the key, and the point of
the PR is that the key is gone. Could this inspect `onChange.mock.calls[0][0]`
and assert `not.toHaveProperty('showCellBars')` (or use `toStrictEqual`)?
##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlForm.reset.test.tsx:
##########
@@ -0,0 +1,148 @@
+/**
+ * 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 { GenericDataType } from '@apache-superset/core/common';
+import { render, screen, userEvent } from 'spec/helpers/testing-library';
+import ColumnConfigPopover from
'src/explore/components/controls/ColumnConfigControl/ColumnConfigPopover';
+import ControlForm, {
+ ControlFormItem,
+ ControlFormRow,
+} from 'src/explore/components/controls/ColumnConfigControl/ControlForm';
+import { ColumnConfigFormLayout } from
'src/explore/components/controls/ColumnConfigControl/types';
+
+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
+ expect(onChange).not.toHaveBeenCalledWith(
+ expect.objectContaining({ showCellBars: expect.anything() }),
+ );
+ });
+
+ test('reset through the popover removes only the reset override', async ()
=> {
+ // ControlForm alone only proves the callback fires. The deletion happens
in the
+ // popover, so exercising ControlForm with a mocked onReset stays green
even if that
+ // filter never runs. Render the popover and assert on the value it emits.
+ const onChange = jest.fn();
+ render(
+ <ColumnConfigPopover
+ column={{
Review Comment:
`ColumnConfigInfo` requires a `key` (types.ts), and this `column` fixture
omits it, so the frontend type-check will report a missing property for this
test file. Should this add `key: 'my_column'`?
--
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]
