rebenitez1802 commented on code in PR #45506:
URL: https://github.com/apache/superset/pull/45506#discussion_r4349786181
##########
docs/docs/using-superset/creating-your-first-dashboard.mdx:
##########
@@ -158,6 +158,9 @@ You can also certify metrics if you'd like for your team in
this view.
<img src={useBaseUrl('/img/tutorial/tutorial_calculated_column.png')} />
+Use the **Metrics** and **Calculated columns** tabs in **Edit Dataset** to add,
+edit, or delete entries. Select **Save** and then **Confirm** to save your
changes.
Review Comment:
Removed the tutorial paragraph in 09f456271a so this PR stays focused on the
pending-edit fix.
##########
superset-frontend/src/components/Datasource/components/CollectionTable/CollectionTable.debounce.test.tsx:
##########
@@ -0,0 +1,282 @@
+/**
+ * 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 {
+ act,
+ fireEvent,
+ render,
+ screen,
+ within,
+ waitFor,
+} from 'spec/helpers/testing-library';
+import { Constants } from '@superset-ui/core/components';
+import TextControl from 'src/explore/components/controls/TextControl';
+import TextAreaControl from 'src/explore/components/controls/TextAreaControl';
+import Field from '../Field';
+import Fieldset from '../Fieldset';
+import CollectionTable from '.';
+
+const initialRows = [
+ { id: 1, column_name: 'first', description: '', verbose_name: '' },
+ { id: 2, column_name: 'second', description: '', verbose_name: '' },
+];
+
+const tableProps = {
+ collection: initialRows,
+ tableColumns: ['column_name', 'description', 'verbose_name'],
+ sortColumns: ['column_name'],
+ allowAddItem: true,
+ allowDeletes: true,
+ itemGenerator: () => ({
+ id: 3,
+ column_name: 'added',
+ description: '',
+ verbose_name: '',
+ }),
+ expandFieldset: (
+ <Fieldset>
+ <Field
+ fieldKey="description"
+ label="Description"
+ control={<TextControl placeholder="Description" />}
+ />
+ <Field
+ fieldKey="verbose_name"
+ label="Label"
+ control={<TextControl placeholder="Label" />}
+ />
+ </Fieldset>
+ ),
+};
+
+/** Commits pending text edits together, as controls can do within one render.
*/
+function flushEdits() {
+ act(() => jest.advanceTimersByTime(Constants.FAST_DEBOUNCE));
+}
+
+beforeEach(() => jest.useFakeTimers());
+afterEach(() => jest.useRealTimers());
+
+test('a pending expanded-row edit preserves an added row and its order', () =>
{
+ const onChange = jest.fn();
+ render(<CollectionTable {...tableProps} onChange={onChange} />);
+ fireEvent.click(screen.getAllByRole('button', { name: /expand row/i })[0]);
+ fireEvent.change(screen.getByPlaceholderText('Description'), {
+ target: { value: 'edited' },
+ });
+ fireEvent.click(screen.getByRole('button', { name: /add item/i }));
+ flushEdits();
+
+ expect(screen.getByText('added')).toBeVisible();
+ expect(onChange).toHaveBeenLastCalledWith([
+ expect.objectContaining({ id: 3 }),
+ expect.objectContaining({ id: 1, description: 'edited' }),
+ expect.objectContaining({ id: 2 }),
+ ]);
+});
+
+test('a pending expanded-row edit does not restore another deleted row', () =>
{
+ const onChange = jest.fn();
+ render(<CollectionTable {...tableProps} onChange={onChange} />);
+ fireEvent.click(screen.getAllByRole('button', { name: /expand row/i })[0]);
+ fireEvent.change(screen.getByPlaceholderText('Description'), {
+ target: { value: 'edited' },
+ });
+ const deletedRow = screen.getByRole('row', { name: /second/i });
+ fireEvent.click(
+ within(deletedRow).getByRole('button', {
+ name: /delete item/i,
+ }),
+ );
+ flushEdits();
+
+ expect(screen.queryByText('second')).not.toBeInTheDocument();
+ expect(onChange).toHaveBeenLastCalledWith([
+ expect.objectContaining({ id: 1, description: 'edited' }),
+ ]);
+});
+
+test('pending sibling fields and rows all survive one debounce batch', () => {
+ const onChange = jest.fn();
+ render(<CollectionTable {...tableProps} onChange={onChange} />);
+ screen
+ .getAllByRole('button', { name: /expand row/i })
+ .forEach(button => fireEvent.click(button));
+ fireEvent.change(screen.getAllByPlaceholderText('Description')[0], {
+ target: { value: 'first description' },
+ });
+ fireEvent.change(screen.getAllByPlaceholderText('Label')[0], {
+ target: { value: 'first label' },
+ });
+ fireEvent.change(screen.getAllByPlaceholderText('Description')[1], {
+ target: { value: 'second description' },
+ });
+ flushEdits();
+
+ expect(onChange).toHaveBeenLastCalledWith([
+ expect.objectContaining({
+ id: 1,
+ description: 'first description',
+ verbose_name: 'first label',
+ }),
+ expect.objectContaining({ id: 2, description: 'second description' }),
+ ]);
+});
+
+test('a pending edit preserves externally refreshed values, membership and
order', () => {
+ const onChange = jest.fn();
+ const { rerender } = render(
+ <CollectionTable {...tableProps} onChange={onChange} />,
+ );
+ fireEvent.click(screen.getAllByRole('button', { name: /expand row/i })[0]);
+ fireEvent.change(screen.getByPlaceholderText('Description'), {
+ target: { value: 'edited' },
+ });
+ const refreshed = [
+ { id: 4, column_name: 'external', description: '', verbose_name: '' },
+ { ...initialRows[0], verbose_name: 'refreshed label' },
+ ];
+ onChange.mockClear();
+ rerender(
+ <CollectionTable
+ {...tableProps}
+ collection={refreshed}
+ onChange={onChange}
+ />,
+ );
+ expect(onChange).not.toHaveBeenCalled();
+ flushEdits();
+
+ expect(screen.getByText('external')).toBeVisible();
+ expect(screen.queryByText('second')).not.toBeInTheDocument();
+ expect(onChange).toHaveBeenLastCalledWith([
+ expect.objectContaining({ id: 4 }),
+ expect.objectContaining({
+ id: 1,
+ description: 'edited',
+ verbose_name: 'refreshed label',
+ }),
+ ]);
+});
+
+test('an asynchronous field callback cannot resurrect the row it edited', ()
=> {
+ let pendingChange: (() => void) | undefined;
+ /** Models a control whose asynchronous edit can finish after it unmounts. */
+ function AsyncControl({ onChange }: { onChange?: (value: string) => void }) {
+ return (
+ <button
+ type="button"
+ onClick={() => {
+ pendingChange = () => onChange?.('late edit');
+ }}
+ >
+ Queue edit
+ </button>
+ );
+ }
+ const onChange = jest.fn();
+ render(
+ <CollectionTable
+ {...tableProps}
+ onChange={onChange}
+ expandFieldset={
+ <Fieldset>
+ <Field
+ fieldKey="description"
+ label="Description"
+ control={<AsyncControl />}
+ />
+ </Fieldset>
+ }
+ />,
+ );
+ fireEvent.click(screen.getAllByRole('button', { name: /expand row/i })[0]);
+ fireEvent.click(screen.getByRole('button', { name: 'Queue edit' }));
+ const deletedRow = screen.getByRole('row', { name: /first/i });
+ fireEvent.click(
+ within(deletedRow).getByRole('button', {
+ name: /delete item/i,
+ }),
+ );
+ onChange.mockClear();
+ act(() => pendingChange?.());
+
+ expect(screen.queryByText('first')).not.toBeInTheDocument();
+ expect(onChange).not.toHaveBeenCalled();
+});
+
+test.each(['expand another row', 'add a row'] as const)(
Review Comment:
Added the fresh-array parent-rerender scenario as the third case in
09f456271a, keeping onChange and the fieldset stable to isolate collection
identity. Confirmed it fails on the merge base and passes here. I retained the
expansion case because it catches the unstable per-row callback regression
encountered while developing this fix. Both comments are addressed, and the
updated 79-test run and pre-commit checks pass. Could you take another look and
approve if everything looks good?
--
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]
