bito-code-review[bot] commented on code in PR #33002:
URL: https://github.com/apache/superset/pull/33002#discussion_r2845505844
##########
superset-frontend/cypress-base/cypress/e2e/dashboard_list/list.test.ts:
##########
@@ -53,12 +52,22 @@ function confirmDelete(bulk = false) {
cy.wrap($input).type('DELETE');
});
cy.getBySel('modal-confirm-button').should('be.visible').click();
+ cy.wait('@delete');
+}
+
+function confirmBulkDelete() {
+ interceptBulkDelete();
- if (bulk) {
- cy.wait('@bulkDelete');
- } else {
- cy.wait('@delete');
- }
+ // Wait for modal dialog to be present and visible
+ cy.get('[role="dialog")[aria-modal="true"]').should('be.visible');
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Invalid CSS selector syntax</b></div>
<div id="fix">
The CSS selector in confirmBulkDelete has invalid syntax with a misplaced
closing parenthesis, which will cause the Cypress test to fail at runtime when
trying to locate the modal dialog. This breaks the bulk delete test
functionality.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
cy.get('[role="dialog"][aria-modal="true"]').should('be.visible');
````
</div>
</details>
</div>
<small><i>Code Review Run #b3d4a2</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/features/bulkUpdate/BulkCertifyModal.tsx:
##########
@@ -0,0 +1,163 @@
+/**
+ * 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 { useState, FC } from 'react';
+
+import { t } from '@apache-superset/core';
+import { SupersetClient } from '@superset-ui/core';
+import { Chart } from 'src/types/Chart';
+import { Dashboard } from 'src/pages/DashboardList';
+import {
+ Input,
+ Modal,
+ Button,
+ FormLabel,
+ Col,
+ Row,
+} from '@superset-ui/core/components';
+
+interface BulkCertifyModalProps {
+ onHide: () => void;
+ refreshData: () => void;
+ addSuccessToast: (msg: string) => void;
+ addDangerToast: (msg: string) => void;
+ show: boolean;
+ resourceName: 'chart' | 'dashboard';
+ resourceLabel: string;
+ selected: Chart[] | Dashboard[];
+}
+
+const BulkCertifyModal: FC<BulkCertifyModalProps> = ({
+ show,
+ selected = [],
+ resourceName,
+ resourceLabel,
+ onHide,
+ refreshData,
+ addSuccessToast,
+ addDangerToast,
+}) => {
+ const [certifiedBy, setCertifiedBy] = useState<string>('');
+ const [certificationDetails, setCertificationDetails] = useState<string>('');
+
+ const resourceLabelPlural = t(
+ '%s',
+ selected.length > 1 ? `${resourceLabel}s` : resourceLabel,
+ );
+
+ const onSave = async () => {
+ if (!certifiedBy) {
+ addDangerToast(t('Please enter who certified these items'));
+ return;
+ }
+
+ try {
+ await Promise.all(
+ selected.map(item => {
+ const url = `/api/v1/${resourceName}/${item.id}`;
+ const payload = {
+ certified_by: certifiedBy,
+ certification_details: certificationDetails,
+ };
+
+ return SupersetClient.put({
+ url,
+ headers: { 'Content-Type': 'application/json' },
+ jsonPayload: payload,
+ });
+ }),
+ );
+
+ addSuccessToast(t('Successfully certified %s', resourceLabelPlural));
+ refreshData();
+ onHide();
+ setCertifiedBy('');
+ setCertificationDetails('');
+ } catch (error) {
+ addDangerToast(t('Failed to certify %s', resourceLabelPlural));
+ }
+ };
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Improve bulk operation error handling</b></div>
<div id="fix">
The bulk certification uses Promise.all, which fails the entire operation if
any single item fails, potentially masking partial successes and giving
misleading feedback. Switching to Promise.allSettled would allow counting
successful vs failed certifications and showing appropriate toasts for each.
</div>
</div>
<small><i>Code Review Run #b3d4a2</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/components/ListView/ListView.test.jsx:
##########
@@ -0,0 +1,390 @@
+/**
+ * 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, within, waitFor } from 'spec/helpers/testing-library';
+import userEvent from '@testing-library/user-event';
+import { QueryParamProvider } from 'use-query-params';
+import thunk from 'redux-thunk';
+import configureStore from 'redux-mock-store';
+import fetchMock from 'fetch-mock';
+
+// Only import components that are directly referenced in tests
+import { ListView } from './ListView';
+
+jest.mock('@superset-ui/core/components', () => ({
+ Alert: ({ children, message, ...props }) => (
+ <div {...props}>{message || children}</div>
+ ),
+ DropdownButton: ({ children, onClick, 'data-test': dataTest, ...props }) => (
+ <button type="button" onClick={onClick} data-test={dataTest} {...props}>
+ {children}
+ </button>
+ ),
+ Icons: {
+ AppstoreOutlined: props => (
+ <span {...props} role="img" aria-label="appstore" />
+ ),
+ UnorderedListOutlined: props => (
+ <span {...props} role="img" aria-label="unordered-list" />
+ ),
+ DeleteOutlined: props => <span {...props} role="img" aria-label="delete"
/>,
+ },
+ EmptyState: ({ children, ...props }) => <div {...props}>{children}</div>,
+ Checkbox: props => <input type="checkbox" {...props} />,
+ Loading: () => <div>Loading...</div>,
+ Flex: ({ children, ...props }) => <div {...props}>{children}</div>,
+ Menu: {
+ Item: ({ children, ...props }) => <div {...props}>{children}</div>,
+ },
+ Modal: ({ children, ...props }) => <div {...props}>{children}</div>,
+ Select: ({ children, onChange, onSelect, value, ...props }) => (
+ <select
+ role="combobox"
+ value={value}
+ onChange={e => {
+ if (onChange) onChange(e.target.value);
+ if (onSelect) onSelect(e.target.value);
+ }}
+ {...props}
+ >
+ {children}
+ </select>
+ ),
+ Pagination: props => <div {...props}>Pagination</div>,
+ TableCollection: props => <div {...props}>TableCollection</div>,
+ AsyncEsmComponent:
+ () =>
+ ({ children, ...props }) => <div {...props}>{children}</div>,
+}));
+
+jest.mock('./Filters', () => {
+ // eslint-disable-next-line global-require
+ const React = require('react');
+ return React.forwardRef((props, ref) =>
+ React.createElement(
+ 'div',
+ { ref, ...props },
+ React.createElement(
+ 'select',
+ {
+ role: 'combobox',
+ onChange: () =>
+ props.updateFilterValue?.(0, { label: 'foo', value: 'bar' }),
+ },
+ React.createElement(
+ 'option',
+ {
+ value: 'bar',
+ onClick: () =>
+ props.updateFilterValue?.(0, { label: 'foo', value: 'bar' }),
+ },
+ 'foo',
+ ),
+ ),
+ React.createElement(
+ 'select',
+ {
+ role: 'combobox',
+ onChange: () =>
+ props.updateFilterValue?.(2, {
+ label: 'age_option',
+ value: 'age_value',
+ }),
+ },
+ React.createElement('option', { value: 'age_value' }, 'age_option'),
+ ),
+ React.createElement('input', {
+ placeholder: 'Type a value',
+ onBlur: e => {
+ if (e.target.value) props.updateFilterValue?.(1, e.target.value);
+ },
+ }),
+ ),
+ );
+});
+
+jest.mock('./CardCollection', () => props => <div {...props} />);
+
+jest.mock('./CardSortSelect', () => ({
+ CardSortSelect: props => (
+ <select
+ data-test="card-sort-select"
+ role="combobox"
+ onChange={props.onChange}
+ {...props}
+ >
+ <option value="alphabetical">Alphabetical</option>
+ </select>
+ ),
+}));
+
+jest.mock('src/features/tags/BulkTagModal', () => props => <div {...props} />);
+
+const middlewares = [thunk];
+const mockStore = configureStore(middlewares);
+
+function makeMockLocation(query) {
+ const queryStr = encodeURIComponent(query);
+ return {
+ protocol: 'http:',
+ host: 'localhost',
+ pathname: '/',
+ search: queryStr.length ? `?${queryStr}` : '',
+ };
+}
+
+const fetchSelectsMock = jest.fn(() => []);
+const mockedProps = {
+ title: 'Data Table',
+ columns: [
+ {
+ accessor: 'id',
+ Header: 'ID',
+ sortable: true,
+ id: 'id',
+ },
+ {
+ accessor: 'age',
+ Header: 'Age',
+ id: 'age',
+ },
+ {
+ accessor: 'name',
+ Header: 'Name',
+ id: 'name',
+ },
+ {
+ accessor: 'time',
+ Header: 'Time',
+ id: 'time',
+ },
+ ],
+ filters: [
+ {
+ Header: 'ID',
+ id: 'id',
+ input: 'select',
+ selects: [{ label: 'foo', value: 'bar' }],
+ operator: 'eq',
+ },
+ {
+ Header: 'Name',
+ id: 'name',
+ input: 'search',
+ operator: 'ct',
+ },
+ {
+ Header: 'Age',
+ id: 'age',
+ input: 'select',
+ fetchSelects: fetchSelectsMock,
+ paginate: true,
+ operator: 'eq',
+ },
+ {
+ Header: 'Time',
+ id: 'time',
+ input: 'datetime_range',
+ operator: 'between',
+ },
+ ],
+ data: [
+ { id: 1, name: 'data 1', age: 10, time: '2020-11-18T07:53:45.354Z' },
+ { id: 2, name: 'data 2', age: 1, time: '2020-11-18T07:53:45.354Z' },
+ ],
+ count: 2,
+ pageSize: 1,
+ fetchData: jest.fn(() => []),
+ loading: false,
+ bulkSelectEnabled: true,
+ disableBulkSelect: jest.fn(),
+ bulkActions: [
+ {
+ key: 'something',
+ name: 'do something',
+ style: 'danger',
+ onSelect: jest.fn(),
+ },
+ ],
+ cardSortSelectOptions: [
+ {
+ desc: false,
+ id: 'something',
+ label: 'Alphabetical',
+ value: 'alphabetical',
+ },
+ ],
+};
+
+const factory = (props = mockedProps) =>
+ render(
+ <QueryParamProvider location={makeMockLocation()}>
+ <ListView {...props} />
+ </QueryParamProvider>,
+ { store: mockStore() },
+ );
+
+describe('ListView', () => {
+ beforeEach(() => {
+ fetchMock.removeRoutes();
+ fetchMock.clearHistory();
+ jest.clearAllMocks();
+ factory();
+ });
+
+ afterEach(() => {
+ fetchMock.removeRoutes();
+ fetchMock.clearHistory();
+ mockedProps.fetchData.mockClear();
+ mockedProps.bulkActions.forEach(ba => {
+ ba.onSelect.mockClear();
+ });
+ });
+
+ // Example of converted test:
+ test('calls fetchData on mount', () => {
+ expect(mockedProps.fetchData).toHaveBeenCalledWith({
+ filters: [],
+ pageIndex: 0,
+ pageSize: 1,
+ sortBy: [],
+ });
+ });
+
+ test('calls fetchData on sort', async () => {
+ const sortHeader = screen.getAllByTestId('sort-header')[1];
+ await userEvent.click(sortHeader);
+
+ expect(mockedProps.fetchData).toHaveBeenCalledWith({
+ filters: [],
+ pageIndex: 0,
+ pageSize: 1,
+ sortBy: [
+ {
+ desc: false,
+ id: 'id',
+ },
+ ],
+ });
+ });
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect sort test assertion</b></div>
<div id="fix">
The sort test clicks the second header (age column) but expects sorting by
'id'. This incorrect assertion could mask bugs; verify and fix to expect 'age'
based on the clicked element.
</div>
</div>
<small><i>Code Review Run #b3d4a2</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/pages/ChartList/ChartList.test.tsx:
##########
@@ -238,50 +284,124 @@ describe('ChartList', () => {
});
});
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
describe('ChartList - Global Filter Interactions', () => {
beforeEach(() => {
- fetchMock.removeRoutes();
setupMocks();
});
afterEach(() => {
- fetchMock.clearHistory();
+ fetchMock.resetHistory();
+ fetchMock.restore();
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Deprecated fetch-mock API usage</b></div>
<div id="fix">
The afterEach cleanup uses fetchMock.resetHistory() and fetchMock.restore(),
but these methods were removed in fetch-mock v12. The correct methods are
fetchMock.clearHistory() and fetchMock.removeRoutes(), which match the cleanup
pattern used in setupMocks.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
fetchMock.clearHistory();
fetchMock.removeRoutes();
````
</div>
</details>
</div>
<small><i>Code Review Run #b3d4a2</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/pages/ChartList/ChartList.listview.test.tsx:
##########
@@ -61,791 +64,878 @@ const mockUser = {
},
};
-beforeEach(() => {
- mockHandleResourceExport.mockClear();
- setupMocks();
-});
+describe('ChartList - List View Tests', () => {
+ beforeEach(() => {
+ mockHandleResourceExport.mockClear();
+ setupMocks();
+ });
+
+ afterEach(() => {
+ fetchMock.removeRoutes();
+ fetchMock.clearHistory();
+ });
+
+ test('renders ChartList in list view', async () => {
+ renderChartList(mockUser);
+
+ // Wait for component to load
+ await waitFor(() => {
+ expect(screen.getByTestId('chart-list-view')).toBeInTheDocument();
+ });
+
+ // Wait for table to be rendered
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
+
+ // Verify cards are not rendered in list view
+ await waitFor(() => {
+ expect(screen.queryByTestId('styled-card')).not.toBeInTheDocument();
+ });
+ });
+
+ test('correctly displays dataset names with and without schema', async () =>
{
+ // Create custom mock data with different datasource_name_text formats
+ const customMockCharts = [
+ {
+ ...mockCharts[0],
+ id: 100,
+ slice_name: 'Chart with Schema',
+ datasource_name_text: 'public.test_dataset',
+ },
+ {
+ ...mockCharts[0],
+ id: 101,
+ slice_name: 'Chart without Schema',
+ datasource_name_text: 'Jinja 5', // Virtual dataset without schema
+ },
+ {
+ ...mockCharts[0],
+ id: 102,
+ slice_name: 'Chart with Dots in Name',
+ datasource_name_text: 'schema.table.with.dots', // Name contains dots
+ },
+ ];
+
+ // Setup mock with custom charts
+ fetchMock.removeRoutes();
+ fetchMock.clearHistory();
+ setupMocks();
+ fetchMock.get(
+ 'glob:*/api/v1/chart/?*',
+ {
+ result: customMockCharts,
+ chart_count: customMockCharts.length,
+ },
+ { overwriteRoutes: true },
+ );
-afterEach(() => {
- fetchMock.clearHistory().removeRoutes();
- fetchMock.removeRoutes();
- mockIsFeatureEnabled.mockReset();
-});
+ renderChartList(mockUser);
+
+ // Wait for table to be rendered
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
+
+ // Wait for data to load
+ await waitFor(() => {
+ expect(screen.getByText('Chart with Schema')).toBeInTheDocument();
+ });
+
+ // Find the specific dataset links by their parent row context
+ const schemaRow = screen.getByText('Chart with Schema').closest('tr');
+ const noSchemaRow = screen.getByText('Chart without Schema').closest('tr');
+ const dotsRow = screen.getByText('Chart with Dots in Name').closest('tr');
+
+ // Check dataset name displays correctly for each case
+ // For chart with schema (public.test_dataset)
+ expect(schemaRow).toBeInTheDocument();
+ const schemaLink = within(schemaRow!).getByRole('link', {
+ name: /test_dataset/i,
+ });
+ expect(schemaLink).toBeInTheDocument();
+ expect(schemaLink).toHaveTextContent('test_dataset');
+
+ // For chart without schema (Jinja 5)
+ expect(noSchemaRow).toBeInTheDocument();
+ const noSchemaLink = within(noSchemaRow!).getByRole('link', {
+ name: /Jinja 5/i,
+ });
+ expect(noSchemaLink).toBeInTheDocument();
+ expect(noSchemaLink).toHaveTextContent('Jinja 5');
+
+ // For chart with dots in name (schema.table.with.dots)
+ expect(dotsRow).toBeInTheDocument();
+ const dotsLink = within(dotsRow!).getByRole('link', {
+ name: /table\.with\.dots/i,
+ });
+ expect(dotsLink).toBeInTheDocument();
+ expect(dotsLink).toHaveTextContent('table.with.dots');
+ });
+
+ test('switches from list view to card view', async () => {
+ renderChartList(mockUser);
+
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
+
+ // Switch to card view
+ const cardViewToggle = screen.getByRole('img', { name: 'appstore' });
+ fireEvent.click(cardViewToggle);
+
+ // Verify table is no longer rendered
+ await waitFor(() => {
+ expect(screen.queryByTestId('listview-table')).not.toBeInTheDocument();
+ });
+
+ // Verify cards are rendered
+ const cards = screen.getAllByTestId('styled-card');
+ expect(cards).toHaveLength(mockCharts.length);
+ });
+
+ test('renders all required column headers', async () => {
+ renderChartList(mockUser);
+
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
+
+ const table = screen.getByTestId('listview-table');
+ const columnHeaders = table.querySelectorAll('[role="columnheader"]');
+
+ // All the table headers with default feature flags on
+ const expectedHeaders = [
+ 'Name',
+ 'Type',
+ 'Dataset',
+ 'On dashboards',
+ 'Owners',
+ 'Last modified',
+ 'Actions',
+ ];
+
+ // Add one extra column header for favorite stars
+ expect(columnHeaders).toHaveLength(expectedHeaders.length + 1);
+
+ // Verify all expected headers are present
+ expectedHeaders.forEach(headerText => {
+ expect(within(table).getByText(headerText)).toBeInTheDocument();
+ });
+ });
+
+ test('sorts table when clicking column headers', async () => {
+ renderChartList(mockUser);
+
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
+
+ const table = screen.getByTestId('listview-table');
+ const sortableHeaders =
table.querySelectorAll('.ant-table-column-sorters');
+
+ expect(sortableHeaders).toHaveLength(3);
+
+ const nameHeader = within(table).getByText('Name');
+ fireEvent.click(nameHeader);
+
+ await waitFor(() => {
+ const sortCalls = fetchMock.callHistory
+ .calls(/chart\/\?q/)
+ .filter(
+ call =>
+ call.url.includes('order_column') &&
call.url.includes('slice_name'),
+ );
+ expect(sortCalls).toHaveLength(1);
+ });
+
+ const typeHeader = within(table).getByText('Type');
+ fireEvent.click(typeHeader);
+
+ await waitFor(() => {
+ const typeSortCalls = fetchMock.callHistory
+ .calls(/chart\/\?q/)
+ .filter(
+ call =>
+ call.url.includes('order_column') && call.url.includes('viz_type'),
+ );
+ expect(typeSortCalls).toHaveLength(1);
+ });
+
+ const lastModifiedHeader = within(table).getByText('Last modified');
+ fireEvent.click(lastModifiedHeader);
+
+ await waitFor(() => {
+ const lastModifiedSortCalls = fetchMock.callHistory
+ .calls(/chart\/\?q/)
+ .filter(
+ call =>
+ call.url.includes('order_column') &&
+ call.url.includes('last_saved_at'),
+ );
+ expect(lastModifiedSortCalls).toHaveLength(1);
+ });
+ });
+
+ test('displays chart data correctly', async () => {
+ /**
+ * @todo Implement test logic for tagging.
+ * If TAGGING_SYSTEM is ever deprecated to always be on,
+ * will need to combine this with the tagging column test.
+ */
+ renderChartList(mockUser);
+
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
+
+ const table = screen.getByTestId('listview-table');
+ const testChart = mockCharts[0];
+
+ await waitFor(() => {
+
expect(within(table).getByText(testChart.slice_name)).toBeInTheDocument();
+ });
+
+ // Find the specific row for our test chart
+ const chartNameElement = within(table).getByText(testChart.slice_name);
+ const chartRow = chartNameElement.closest(
+ '[data-test="table-row"]',
+ ) as HTMLElement;
+ expect(chartRow).toBeInTheDocument();
+
+ // Check for favorite star column within the specific row
+ const favoriteButton = within(chartRow).getByTestId('fave-unfave-icon');
+ expect(favoriteButton).toBeInTheDocument();
+ expect(favoriteButton).toHaveAttribute('role', 'button');
+
+ // Check chart name link within the specific row
+ const chartLink = within(chartRow).getByTestId(
+ `${testChart.slice_name}-list-chart-title`,
+ );
+ expect(chartLink).toBeInTheDocument();
+ expect(chartLink).toHaveAttribute('href', testChart.url);
-test('renders table in list view', async () => {
- renderChartList(mockUser);
+ // Check viz type within the specific row
+ expect(within(chartRow).getByText(testChart.viz_type)).toBeInTheDocument();
- // Wait for component to load
- await waitFor(() => {
- expect(screen.getByTestId('chart-list-view')).toBeInTheDocument();
- });
+ // Check dataset name and link within the specific row
+ const datasetName = testChart.datasource_name_text?.split('.').pop() || '';
+ expect(within(chartRow).getByText(datasetName)).toBeInTheDocument();
- // Wait for table to be rendered
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
- });
+ const datasetLink = within(chartRow).getByTestId('internal-link');
+ expect(datasetLink).toBeInTheDocument();
+ expect(datasetLink).toHaveAttribute('href', testChart.datasource_url);
- // Verify cards are not rendered in list view
- expect(screen.queryByTestId('styled-card')).not.toBeInTheDocument();
-});
+ // Check dashboard display within the specific row
+ expect(
+ within(chartRow).getByText(testChart.dashboards[0].dashboard_title),
+ ).toBeInTheDocument();
-test('displays dataset names with and without schema prefix', async () => {
- // Create custom mock data with different datasource_name_text formats
- const customMockCharts = [
- {
- ...mockCharts[0],
- id: 100,
- slice_name: 'Chart with Schema',
- datasource_name_text: 'public.test_dataset',
- },
- {
- ...mockCharts[0],
- id: 101,
- slice_name: 'Chart without Schema',
- datasource_name_text: 'Jinja 5', // Virtual dataset without schema
- },
- {
- ...mockCharts[0],
- id: 102,
- slice_name: 'Chart with Dots in Name',
- datasource_name_text: 'schema.table.with.dots', // Name contains dots
- },
- ];
-
- // Setup mock with custom charts
- fetchMock.removeRoutes();
- setupMocks();
- fetchMock.modifyRoute(API_ENDPOINTS.CHARTS, {
- response: {
- result: customMockCharts,
- chart_count: customMockCharts.length,
- },
- });
+ // Check owners display - find avatar group within the row
+ const avatarGroup = chartRow.querySelector(
+ '.ant-avatar-group',
+ ) as HTMLElement;
+ expect(avatarGroup).toBeInTheDocument();
- renderChartList(mockUser);
+ // Test owner initials for mockCharts[0] (we know it has owners)
+ const ownerInitials =
`${testChart.owners[0].first_name[0]}${testChart.owners[0].last_name[0]}`;
+ expect(within(avatarGroup).getByText(ownerInitials)).toBeInTheDocument();
- // Wait for table to be rendered
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ // Check last modified time within the specific row
+ expect(
+ within(chartRow).getByText(testChart.changed_on_delta_humanized),
+ ).toBeInTheDocument();
+
+ // Check actions column within the specific row
+ const actionsContainer = chartRow.querySelector('.actions');
+ expect(actionsContainer).toBeInTheDocument();
+
+ // Verify action buttons exist within the specific row
+ expect(within(chartRow).getByTestId('delete')).toBeInTheDocument();
+ expect(within(chartRow).getByTestId('upload')).toBeInTheDocument();
+ expect(within(chartRow).getByTestId('edit-alt')).toBeInTheDocument();
+ });
+
+ test('export chart api called when export button is clicked', async () => {
+ renderChartList(mockUser);
+
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
+
+ await waitFor(() => {
+ expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
+ expect(screen.getByText(mockCharts[1].slice_name)).toBeInTheDocument();
+ });
+
+ // Click first export button
+ const table = screen.getByTestId('listview-table');
+ const exportButtons = within(table).getAllByTestId('upload');
+ fireEvent.click(exportButtons[0]);
+
+ // Verify export functionality is triggered - check if
handleResourceExport was called
+ await waitFor(() => {
+ expect(mockHandleResourceExport).toHaveBeenCalledWith(
+ 'chart',
+ [mockCharts[0].id],
+ expect.any(Function),
+ );
+ });
});
- // Wait for data to load
- await waitFor(() => {
- expect(screen.getByText('Chart with Schema')).toBeInTheDocument();
- });
+ test('opens edit properties modal when edit button is clicked', async () => {
+ renderChartList(mockUser);
- // Find the specific dataset links by their parent row context
- const schemaRow = screen.getByText('Chart with Schema').closest('tr');
- const noSchemaRow = screen.getByText('Chart without Schema').closest('tr');
- const dotsRow = screen.getByText('Chart with Dots in Name').closest('tr');
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
- // Check dataset name displays correctly for each case
- // For chart with schema (public.test_dataset)
- expect(schemaRow).toBeInTheDocument();
- const schemaLink = within(schemaRow!).getByRole('link', {
- name: /test_dataset/i,
- });
- expect(schemaLink).toBeInTheDocument();
- expect(schemaLink).toHaveTextContent('test_dataset');
+ await waitFor(() => {
+ expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
+ expect(screen.getByText(mockCharts[1].slice_name)).toBeInTheDocument();
+ });
- // For chart without schema (Jinja 5)
- expect(noSchemaRow).toBeInTheDocument();
- const noSchemaLink = within(noSchemaRow!).getByRole('link', {
- name: /Jinja 5/i,
- });
- expect(noSchemaLink).toBeInTheDocument();
- expect(noSchemaLink).toHaveTextContent('Jinja 5');
+ const table = screen.getByTestId('listview-table');
+ const editButtons = within(table).getAllByTestId('edit-alt');
+ fireEvent.click(editButtons[0]);
- // For chart with dots in name (schema.table.with.dots)
- expect(dotsRow).toBeInTheDocument();
- const dotsLink = within(dotsRow!).getByRole('link', {
- name: /table\.with\.dots/i,
+ // Verify edit modal opens
+ await waitFor(() => {
+ const editModal = screen.getByRole('dialog');
+ expect(editModal).toBeInTheDocument();
+ expect(editModal).toHaveTextContent(/properties/i);
+ });
});
- expect(dotsLink).toBeInTheDocument();
- expect(dotsLink).toHaveTextContent('table.with.dots');
-});
-test('switches from list view to card view', async () => {
- renderChartList(mockUser);
+ test('opens delete confirmation when delete button is clicked', async () => {
+ renderChartList(mockUser);
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
- });
-
- // Switch to card view
- const cardViewToggle = screen.getByRole('img', { name: 'appstore' });
- await userEvent.click(cardViewToggle);
-
- // Verify table is no longer rendered
- await waitFor(() => {
- expect(screen.queryByTestId('listview-table')).not.toBeInTheDocument();
- });
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
- // Verify cards are rendered
- const cards = screen.getAllByTestId('styled-card');
- expect(cards).toHaveLength(mockCharts.length);
-});
+ await waitFor(() => {
+ expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
+ expect(screen.getByText(mockCharts[1].slice_name)).toBeInTheDocument();
+ });
-test('renders all required column headers', async () => {
- renderChartList(mockUser);
+ const table = screen.getByTestId('listview-table');
+ const deleteButtons = within(table).getAllByTestId('delete');
+ fireEvent.click(deleteButtons[0]);
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ // Verify delete confirmation modal opens
+ await waitFor(() => {
+ const deleteModal = screen.getByRole('dialog');
+ expect(deleteModal).toBeInTheDocument();
+ expect(deleteModal).toHaveTextContent(/delete/i);
+ });
});
- const table = screen.getByTestId('listview-table');
- const columnHeaders = table.querySelectorAll('[role="columnheader"]');
-
- // All the table headers with default feature flags on
- const expectedHeaders = [
- 'Name',
- 'Type',
- 'Dataset',
- 'On dashboards',
- 'Owners',
- 'Last modified',
- 'Actions',
- ];
-
- // Add one extra column header for favorite stars
- expect(columnHeaders).toHaveLength(expectedHeaders.length + 1);
-
- // Verify all expected headers are present
- expectedHeaders.forEach(headerText => {
- expect(within(table).getByTitle(headerText)).toBeInTheDocument();
- });
-});
+ test('displays certified badge only for certified charts', async () => {
+ // Test certified chart (mockCharts[1] has certification)
+ const certifiedChart = mockCharts[1];
+ // Test uncertified chart (mockCharts[0] has no certification)
+ const uncertifiedChart = mockCharts[0];
-test('sorts table when clicking column headers', async () => {
- renderChartList(mockUser);
+ renderChartList(mockUser);
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
- });
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
- const table = screen.getByTestId('listview-table');
+ await waitFor(() => {
+ expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
+ expect(screen.getByText(mockCharts[1].slice_name)).toBeInTheDocument();
+ });
- const allHeaders = table.querySelectorAll('.ant-table-column-sorters');
+ const table = screen.getByTestId('listview-table');
- const sortableHeaders = Array.from(allHeaders).filter(
- header => !header.closest('.ant-table-measure-cell-content'),
- );
- expect(sortableHeaders).toHaveLength(3);
-
- const nameHeader = within(table).getByTitle('Name');
- await userEvent.click(nameHeader);
-
- await waitFor(() => {
- const sortCalls = fetchMock.callHistory
- .calls(/chart\/\?q/)
- .filter(
- call =>
- call.url.includes('order_column') && call.url.includes('slice_name'),
- );
- expect(sortCalls).toHaveLength(1);
+ const certifiedChartElement = within(table).getByText(
+ certifiedChart.slice_name,
+ );
+ const certifiedChartRow = certifiedChartElement.closest(
+ '[data-test="table-row"]',
+ ) as HTMLElement;
+ const certifiedBadge =
+ within(certifiedChartRow).getByLabelText('certified');
+ expect(certifiedBadge).toBeInTheDocument();
+
+ const uncertifiedChartElement = within(table).getByText(
+ uncertifiedChart.slice_name,
+ );
+ const uncertifiedChartRow = uncertifiedChartElement.closest(
+ '[data-test="table-row"]',
+ ) as HTMLElement;
+ expect(
+ within(uncertifiedChartRow).queryByLabelText('certified'),
+ ).not.toBeInTheDocument();
});
- const typeHeader = within(table).getByTitle('Type');
- await userEvent.click(typeHeader);
+ test('displays info icon only for charts with descriptions', async () => {
+ // Test chart with description (mockCharts[0] has description)
+ const chartWithDesc = mockCharts[0];
+ // Test chart without description (mockCharts[2] has description: null)
+ const chartNoDesc = mockCharts[2];
- await waitFor(() => {
- const typeSortCalls = fetchMock.callHistory
- .calls(/chart\/\?q/)
- .filter(
- call =>
- call.url.includes('order_column') && call.url.includes('viz_type'),
- );
- expect(typeSortCalls).toHaveLength(1);
- });
+ renderChartList(mockUser);
- const lastModifiedHeader = within(table).getByTitle('Last modified');
- await userEvent.click(lastModifiedHeader);
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
- await waitFor(() => {
- const lastModifiedSortCalls = fetchMock.callHistory
- .calls(/chart\/\?q/)
- .filter(
- call =>
- call.url.includes('order_column') &&
- call.url.includes('last_saved_at'),
- );
- expect(lastModifiedSortCalls).toHaveLength(1);
- });
-});
+ await waitFor(() => {
+ expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
+ expect(screen.getByText(mockCharts[2].slice_name)).toBeInTheDocument();
+ });
-test('displays chart data correctly in table rows', async () => {
- /**
- * @todo Implement test logic for tagging.
- * If TAGGING_SYSTEM is ever deprecated to always be on,
- * will need to combine this with the tagging column test.
- */
- renderChartList(mockUser);
+ const table = screen.getByTestId('listview-table');
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ const chartWithDescElement = within(table).getByText(
+ chartWithDesc.slice_name,
+ );
+ const chartWithDescRow = chartWithDescElement.closest(
+ '[data-test="table-row"]',
+ ) as HTMLElement;
+ const infoTooltip =
+ within(chartWithDescRow).getByLabelText('Show info tooltip');
+ expect(infoTooltip).toBeInTheDocument();
+
+ const chartNoDescElement = within(table).getByText(chartNoDesc.slice_name);
+ const chartNoDescRow = chartNoDescElement.closest(
+ '[data-test="table-row"]',
+ ) as HTMLElement;
+ expect(
+ within(chartNoDescRow).queryByLabelText('Show info tooltip'),
+ ).not.toBeInTheDocument();
});
- const table = screen.getByTestId('listview-table');
- const testChart = mockCharts[0];
-
- await waitFor(() => {
- expect(within(table).getByText(testChart.slice_name)).toBeInTheDocument();
- });
+ test('displays chart with empty dataset column', async () => {
+ renderChartList(mockUser);
- // Find the specific row for our test chart
- const chartNameElement = within(table).getByText(testChart.slice_name);
- const chartRow = chartNameElement.closest(
- '[data-test="table-row"]',
- ) as HTMLElement;
- expect(chartRow).toBeInTheDocument();
-
- // Check for favorite star column within the specific row
- const favoriteButton = within(chartRow).getByTestId('fave-unfave-icon');
- expect(favoriteButton).toBeInTheDocument();
- expect(favoriteButton).toHaveAttribute('role', 'button');
-
- // Check chart name link within the specific row
- const chartLink = within(chartRow).getByTestId(
- `${testChart.slice_name}-list-chart-title`,
- );
- expect(chartLink).toBeInTheDocument();
- expect(chartLink).toHaveAttribute('href', testChart.url);
-
- // Check viz type within the specific row
- expect(within(chartRow).getByText(testChart.viz_type)).toBeInTheDocument();
-
- // Check dataset name and link within the specific row
- const datasetName = testChart.datasource_name_text?.split('.').pop() || '';
- expect(within(chartRow).getByText(datasetName)).toBeInTheDocument();
-
- const datasetLink = within(chartRow).getByTestId('internal-link');
- expect(datasetLink).toBeInTheDocument();
- expect(datasetLink).toHaveAttribute('href', testChart.datasource_url);
-
- // Check dashboard display within the specific row
- expect(
- within(chartRow).getByText(testChart.dashboards[0].dashboard_title),
- ).toBeInTheDocument();
-
- // Check owners display - find avatar group within the row
- const avatarGroup = chartRow.querySelector(
- '.ant-avatar-group',
- ) as HTMLElement;
- expect(avatarGroup).toBeInTheDocument();
-
- // Test owner initials for mockCharts[0] (we know it has owners)
- const ownerInitials =
`${testChart.owners[0].first_name[0]}${testChart.owners[0].last_name[0]}`;
- expect(within(avatarGroup).getByText(ownerInitials)).toBeInTheDocument();
-
- // Check last modified time within the specific row
- expect(
- within(chartRow).getByText(testChart.changed_on_delta_humanized),
- ).toBeInTheDocument();
-
- // Verify action buttons exist within the specific row
- expect(within(chartRow).getByTestId('delete')).toBeInTheDocument();
- expect(within(chartRow).getByTestId('upload')).toBeInTheDocument();
- expect(within(chartRow).getByTestId('edit-alt')).toBeInTheDocument();
-});
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
-test('calls export API when export button is clicked', async () => {
- renderChartList(mockUser);
+ await waitFor(() => {
+ expect(screen.getByText(mockCharts[2].slice_name)).toBeInTheDocument();
+ });
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
- });
+ const table = screen.getByTestId('listview-table');
+ const chartNameElement = within(table).getByText(mockCharts[2].slice_name);
+ const chartRow = chartNameElement.closest(
+ '[data-test="table-row"]',
+ ) as HTMLElement;
- await waitFor(() => {
- expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
- expect(screen.getByText(mockCharts[1].slice_name)).toBeInTheDocument();
- });
+ // Chart name should be visible
+ expect(
+ within(chartRow).getByText(mockCharts[2].slice_name),
+ ).toBeInTheDocument();
- // Click first export button
- const table = screen.getByTestId('listview-table');
- const exportButtons = within(table).getAllByTestId('upload');
- await userEvent.click(exportButtons[0]);
-
- // Verify export functionality is triggered - check if handleResourceExport
was called
- await waitFor(() => {
- expect(mockHandleResourceExport).toHaveBeenCalledWith(
- 'chart',
- [mockCharts[0].id],
- expect.any(Function),
+ // Find dataset column index by header
+ const headers = within(table).getAllByRole('columnheader');
+ const datasetHeaderIndex = headers.findIndex(header =>
+ header.textContent?.includes('Dataset'),
);
- });
-});
+ expect(datasetHeaderIndex).toBeGreaterThan(-1); // Ensure column exists
-test('opens edit properties modal on edit button click', async () => {
- renderChartList(mockUser);
+ // Since mockCharts[2] has datasource_name_text: null, verify dataset cell
is empty
+ const datasetCell =
+ within(chartRow).getAllByRole('cell')[datasetHeaderIndex];
+ expect(datasetCell).toBeInTheDocument();
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ // Verify dataset cell is empty for charts with no dataset
+ expect(datasetCell).toHaveTextContent('');
+ // There's a link element but with empty href
+ const datasetLink = within(datasetCell).getByRole('link');
+ expect(datasetLink).toHaveAttribute('href', '');
});
- await waitFor(() => {
- expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
- expect(screen.getByText(mockCharts[1].slice_name)).toBeInTheDocument();
- });
+ test('displays chart with empty on dashboards column', async () => {
+ renderChartList(mockUser);
- const table = screen.getByTestId('listview-table');
- const editButtons = within(table).getAllByTestId('edit-alt');
- await userEvent.click(editButtons[0]);
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
- // Verify edit modal opens
- await waitFor(() => {
- const editModal = screen.getByRole('dialog');
- expect(editModal).toBeInTheDocument();
- expect(editModal).toHaveTextContent(/properties/i);
- });
-});
+ await waitFor(() => {
+ expect(screen.getByText(mockCharts[2].slice_name)).toBeInTheDocument();
+ });
-test('opens delete confirmation on delete button click', async () => {
- renderChartList(mockUser);
+ // Test mockCharts[2] which has dashboards: []
+ const table = screen.getByTestId('listview-table');
+ const chartNameElement = within(table).getByText(mockCharts[2].slice_name);
+ const chartRow = chartNameElement.closest(
+ '[data-test="table-row"]',
+ ) as HTMLElement;
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
- });
+ // Chart should still render - chart name should be visible
+ expect(
+ within(chartRow).getByText(mockCharts[2].slice_name),
+ ).toBeInTheDocument();
- await waitFor(() => {
- expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
- expect(screen.getByText(mockCharts[1].slice_name)).toBeInTheDocument();
- });
+ // Find dashboard column index by header
+ const headers = within(table).getAllByRole('columnheader');
+ const dashboardHeaderIndex = headers.findIndex(header =>
+ header.textContent?.includes('On dashboards'),
+ );
+ expect(dashboardHeaderIndex).toBeGreaterThan(-1); // Ensure column exists
- const table = screen.getByTestId('listview-table');
- const deleteButtons = within(table).getAllByTestId('delete');
- await userEvent.click(deleteButtons[0]);
+ // Since mockCharts[2] has dashboards: [], verify dashboard cell is empty
+ const dashboardCell =
+ within(chartRow).getAllByRole('cell')[dashboardHeaderIndex];
+ expect(dashboardCell).toBeInTheDocument();
- // Verify delete confirmation modal opens
- await waitFor(() => {
- const deleteModal = screen.getByRole('dialog');
- expect(deleteModal).toBeInTheDocument();
- expect(deleteModal).toHaveTextContent(/delete/i);
+ // Verify no dashboard links are present in this cell
+ expect(within(dashboardCell).queryByRole('link')).not.toBeInTheDocument();
});
-});
-test('displays certified badge only for certified charts', async () => {
- // Test certified chart (mockCharts[1] has certification)
- const certifiedChart = mockCharts[1];
- // Test uncertified chart (mockCharts[0] has no certification)
- const uncertifiedChart = mockCharts[0];
+ test('shows tag info when TAGGING_SYSTEM is enabled', async () => {
+ // Enable tagging system feature flag
+ mockIsFeatureEnabled.mockImplementation(
+ feature => feature === 'TAGGING_SYSTEM',
+ );
- renderChartList(mockUser);
+ renderChartList(mockUser);
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
- });
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
- await waitFor(() => {
- expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
- expect(screen.getByText(mockCharts[1].slice_name)).toBeInTheDocument();
- });
+ const testChart = mockCharts[0];
+ const table = screen.getByTestId('listview-table');
+ expect(within(table).getByText('Tags')).toBeInTheDocument();
- const table = screen.getByTestId('listview-table');
-
- const certifiedChartElement = within(table).getByText(
- certifiedChart.slice_name,
- );
- const certifiedChartRow = certifiedChartElement.closest(
- '[data-test="table-row"]',
- ) as HTMLElement;
- const certifiedBadge = within(certifiedChartRow).getByLabelText('certified');
- expect(certifiedBadge).toBeInTheDocument();
-
- const uncertifiedChartElement = within(table).getByText(
- uncertifiedChart.slice_name,
- );
- const uncertifiedChartRow = uncertifiedChartElement.closest(
- '[data-test="table-row"]',
- ) as HTMLElement;
- expect(
- within(uncertifiedChartRow).queryByLabelText('certified'),
- ).not.toBeInTheDocument();
-});
+ await waitFor(() => {
+
expect(within(table).getByText(testChart.slice_name)).toBeInTheDocument();
+ });
-test('displays info icon only for charts with descriptions', async () => {
- // Test chart with description (mockCharts[0] has description)
- const chartWithDesc = mockCharts[0];
- // Test chart without description (mockCharts[2] has description: null)
- const chartNoDesc = mockCharts[2];
+ const chartNameElement = within(table).getByText(testChart.slice_name);
+ const chartRow = chartNameElement.closest(
+ '[data-test="table-row"]',
+ ) as HTMLElement;
+ expect(chartRow).toBeInTheDocument();
- renderChartList(mockUser);
+ const tagList = chartRow.querySelector('.tag-list') as HTMLElement;
+ expect(tagList).toBeInTheDocument();
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
- });
+ // Find the tag in the row
+ const tag = within(tagList).getByTestId('tag');
+ expect(tag).toBeInTheDocument();
+ expect(tag).toHaveTextContent('basic');
- await waitFor(() => {
- expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
- expect(screen.getByText(mockCharts[2].slice_name)).toBeInTheDocument();
+ // Tag should be a link to all_entities page
+ const tagLink = within(tag).getByRole('link');
+ expect(tagLink).toHaveAttribute('href', '/superset/all_entities/?id=1');
+ expect(tagLink).toHaveAttribute('target', '_blank');
});
- const table = screen.getByTestId('listview-table');
-
- const chartWithDescElement = within(table).getByText(
- chartWithDesc.slice_name,
- );
- const chartWithDescRow = chartWithDescElement.closest(
- '[data-test="table-row"]',
- ) as HTMLElement;
- const infoTooltip =
- within(chartWithDescRow).getByLabelText('Show info tooltip');
- expect(infoTooltip).toBeInTheDocument();
-
- const chartNoDescElement = within(table).getByText(chartNoDesc.slice_name);
- const chartNoDescRow = chartNoDescElement.closest(
- '[data-test="table-row"]',
- ) as HTMLElement;
- expect(
- within(chartNoDescRow).queryByLabelText('Show info tooltip'),
- ).not.toBeInTheDocument();
-});
+ test('can bulk select and deselect all charts', async () => {
+ renderChartList(mockUser);
-test('renders empty dashboard column for charts without dashboards', async ()
=> {
- renderChartList(mockUser);
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
- });
+ await waitFor(() => {
+ expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
+ expect(screen.getByText(mockCharts[1].slice_name)).toBeInTheDocument();
+ });
- await waitFor(() => {
- expect(screen.getByText(mockCharts[2].slice_name)).toBeInTheDocument();
- });
+ const bulkSelectButton = screen.getByTestId('bulk-select');
+ fireEvent.click(bulkSelectButton);
- // Test mockCharts[2] which has dashboards: []
- const table = screen.getByTestId('listview-table');
- const chartNameElement = within(table).getByText(mockCharts[2].slice_name);
- const chartRow = chartNameElement.closest(
- '[data-test="table-row"]',
- ) as HTMLElement;
-
- // Chart should still render - chart name should be visible
- expect(
- within(chartRow).getByText(mockCharts[2].slice_name),
- ).toBeInTheDocument();
-
- // Find dashboard column index by header
- const headers = within(table).getAllByRole('columnheader');
- const dashboardHeaderIndex = headers.findIndex(header =>
- header.textContent?.includes('On dashboards'),
- );
- expect(dashboardHeaderIndex).toBeGreaterThan(-1); // Ensure column exists
-
- // Since mockCharts[2] has dashboards: [], verify dashboard cell is empty
- const dashboardCell =
- within(chartRow).getAllByRole('cell')[dashboardHeaderIndex];
- expect(dashboardCell).toBeInTheDocument();
-
- // Verify no dashboard links are present in this cell
- expect(within(dashboardCell).queryByRole('link')).not.toBeInTheDocument();
-});
-
-test('renders dashboard crosslinks as navigable links', async () => {
- renderChartList(mockUser);
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
- });
-
- const table = screen.getByTestId('listview-table');
-
- // mockCharts[1] has multiple dashboards - verify all render with correct
hrefs
- const chartRow = within(table)
- .getByText(mockCharts[1].slice_name)
- .closest('[data-test="table-row"]') as HTMLElement;
- const crosslinks = within(chartRow).getByTestId('crosslinks');
- const dashboards = mockCharts[1].dashboards as {
- dashboard_title: string;
- id: number;
- }[];
- const links = within(crosslinks).getAllByRole('link');
- expect(links).toHaveLength(dashboards.length);
- dashboards.forEach(dashboard => {
- expect(
- within(crosslinks).getByRole('link', {
- name: new RegExp(dashboard.dashboard_title),
- }),
- ).toHaveAttribute('href', `/superset/dashboard/${dashboard.id}`);
- });
-});
+ await waitFor(() => {
+ // Expect header checkbox + one checkbox per chart
+ expect(screen.getAllByRole('checkbox')).toHaveLength(
+ mockCharts.length + 1,
+ );
+ });
-test('shows tag column when TAGGING_SYSTEM is enabled', async () => {
- // Enable tagging system feature flag
- mockIsFeatureEnabled.mockImplementation(
- feature => feature === 'TAGGING_SYSTEM',
- );
+ // Use the header checkbox to select all
+ const selectAllCheckbox = screen.getByLabelText('Select all');
+ expect(selectAllCheckbox).not.toBeChecked();
- renderChartList(mockUser);
+ fireEvent.click(selectAllCheckbox);
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
- });
+ await waitFor(() => {
+ // All checkboxes should be checked
+ const checkboxes = screen.getAllByRole('checkbox');
+ checkboxes.forEach(checkbox => {
+ expect(checkbox).toBeChecked();
+ });
- const testChart = mockCharts[0];
- const table = screen.getByTestId('listview-table');
- expect(within(table).getByTitle('Tags')).toBeInTheDocument();
+ // Should show all charts selected
+ expect(screen.getByTestId('bulk-select-copy')).toHaveTextContent(
+ `${mockCharts.length} Selected`,
+ );
+ });
+
+ // Use the deselect all link to deselect all
+ const deselectAllButton = screen.getByTestId('bulk-select-deselect-all');
+ fireEvent.click(deselectAllButton);
+
+ await waitFor(() => {
+ // All checkboxes should be unchecked
+ const checkboxes = screen.getAllByRole('checkbox');
+ checkboxes.forEach(checkbox => {
+ expect(checkbox).not.toBeChecked();
+ });
+
+ // Should show 0 selected
+ expect(screen.getByTestId('bulk-select-copy')).toHaveTextContent(
+ '0 Selected',
+ );
- await waitFor(() => {
- expect(within(table).getByText(testChart.slice_name)).toBeInTheDocument();
+ // Bulk action buttons should disappear
+ expect(
+ screen.queryByTestId('bulk-select-action'),
+ ).not.toBeInTheDocument();
+ });
});
- const chartNameElement = within(table).getByText(testChart.slice_name);
- const chartRow = chartNameElement.closest(
- '[data-test="table-row"]',
- ) as HTMLElement;
- expect(chartRow).toBeInTheDocument();
-
- const tagList = chartRow.querySelector('.tag-list') as HTMLElement;
- expect(tagList).toBeInTheDocument();
-
- // Find the tag in the row
- const tag = within(tagList).getByTestId('tag');
- expect(tag).toBeInTheDocument();
- expect(tag).toHaveTextContent('basic');
+ test('can bulk export selected charts', async () => {
+ renderChartList(mockUser);
- // Tag should be a link to all_entities page
- const tagLink = within(tag).getByRole('link');
- expect(tagLink).toHaveAttribute('href', '/superset/all_entities/?id=1');
- expect(tagLink).toHaveAttribute('target', '_blank');
-});
-
-test('supports bulk select and deselect all', async () => {
- renderChartList(mockUser);
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
- });
+ await waitFor(() => {
+ expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
+ });
- await waitFor(() => {
- expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
- expect(screen.getByText(mockCharts[1].slice_name)).toBeInTheDocument();
- });
+ const bulkSelectButton = screen.getByTestId('bulk-select');
+ fireEvent.click(bulkSelectButton);
- const bulkSelectButton = screen.getByTestId('bulk-select');
- await userEvent.click(bulkSelectButton);
+ await waitFor(() => {
+ // Expect header checkbox + one checkbox per chart
+ expect(screen.getAllByRole('checkbox')).toHaveLength(
+ mockCharts.length + 1,
+ );
+ });
- await waitFor(() => {
- // Expect header checkbox + one checkbox per chart
- expect(screen.getAllByRole('checkbox')).toHaveLength(mockCharts.length +
1);
- });
+ // Use select all to select multiple charts
+ const selectAllCheckbox = screen.getByLabelText('Select all');
+ fireEvent.click(selectAllCheckbox);
- // Use the header checkbox to select all
- const selectAllCheckbox = screen.getAllByLabelText('Select all')[0];
- expect(selectAllCheckbox).not.toBeChecked();
+ await waitFor(() => {
+ expect(screen.getByTestId('bulk-select-copy')).toHaveTextContent(
+ `${mockCharts.length} Selected`,
+ );
+ });
- await userEvent.click(selectAllCheckbox);
+ const exportButton = screen.getByText('Export');
+ expect(exportButton).toBeInTheDocument();
- await waitFor(() => {
- // Should show all charts selected
- expect(screen.getByTestId('bulk-select-copy')).toHaveTextContent(
- `${mockCharts.length} Selected`,
- );
- });
+ fireEvent.click(exportButton);
- // All checkboxes should be checked
- let checkboxes = screen.getAllByRole('checkbox');
- checkboxes.forEach(checkbox => {
- expect(checkbox).toBeChecked();
+ await waitFor(() => {
+ expect(mockHandleResourceExport).toHaveBeenCalledWith(
+ 'chart',
+ mockCharts.map(chart => chart.id),
+ expect.any(Function),
+ );
+ });
});
- // Use the deselect all link to deselect all
- const deselectAllButton = screen.getByTestId('bulk-select-deselect-all');
- await userEvent.click(deselectAllButton);
+ test('can bulk delete selected charts', async () => {
+ renderChartList(mockUser);
- await waitFor(() => {
- // Should show 0 selected
- expect(screen.getByTestId('bulk-select-copy')).toHaveTextContent(
- '0 Selected',
- );
- });
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
- // All checkboxes should be unchecked
- checkboxes = screen.getAllByRole('checkbox');
- checkboxes.forEach(checkbox => {
- expect(checkbox).not.toBeChecked();
- });
-
- // Bulk action buttons should disappear
- expect(screen.queryByTestId('bulk-select-action')).not.toBeInTheDocument();
-});
+ await waitFor(() => {
+ expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
+ expect(screen.getByText(mockCharts[1].slice_name)).toBeInTheDocument();
+ });
-test('supports bulk export of selected charts', async () => {
- renderChartList(mockUser);
+ const bulkSelectButton = screen.getByTestId('bulk-select');
+ fireEvent.click(bulkSelectButton);
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
- });
+ await waitFor(() => {
+ // Expect header checkbox + one checkbox per chart
+ expect(screen.getAllByRole('checkbox')).toHaveLength(
+ mockCharts.length + 1,
+ );
+ });
- await waitFor(() => {
- expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
- });
+ // Use select all to select multiple charts
+ const selectAllCheckbox = screen.getByLabelText('Select all');
+ fireEvent.click(selectAllCheckbox);
- const bulkSelectButton = screen.getByTestId('bulk-select');
- await userEvent.click(bulkSelectButton);
+ await waitFor(() => {
+ expect(screen.getByTestId('bulk-select-copy')).toHaveTextContent(
+ `${mockCharts.length} Selected`,
+ );
+ });
- await waitFor(() => {
- // Expect header checkbox + one checkbox per chart
- expect(screen.getAllByRole('checkbox')).toHaveLength(mockCharts.length +
1);
+ const bulkActionButton = screen.getByTestId('bulk-select-action');
+ expect(bulkActionButton).toBeInTheDocument();
});
- // Use select all to select multiple charts
- const selectAllCheckbox = screen.getAllByLabelText('Select all')[0];
- await userEvent.click(selectAllCheckbox);
-
- await waitFor(() => {
- expect(screen.getByTestId('bulk-select-copy')).toHaveTextContent(
- `${mockCharts.length} Selected`,
+ test('can bulk add tags to selected charts', async () => {
+ // Enable tagging system feature flag
+ mockIsFeatureEnabled.mockImplementation(
+ feature => feature === 'TAGGING_SYSTEM',
);
- });
- // Click bulk export button
- const bulkActions = screen.getAllByTestId('bulk-select-action');
- const exportButton = bulkActions.find(btn => btn.textContent === 'Export');
- expect(exportButton).toBeInTheDocument();
+ renderChartList(mockUser);
- await userEvent.click(exportButton!);
-
- // Verify export function was called with all chart IDs
- await waitFor(() => {
- expect(mockHandleResourceExport).toHaveBeenCalledWith(
- 'chart',
- mockCharts.map(chart => chart.id),
- expect.any(Function),
- );
- });
-});
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
-test('supports bulk delete of selected charts', async () => {
- renderChartList(mockUser);
+ // Wait for chart data to load
+ await waitFor(() => {
+ expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
+ expect(screen.getByText(mockCharts[1].slice_name)).toBeInTheDocument();
+ });
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
- });
+ // Activate bulk select and select charts
+ const bulkSelectButton = screen.getByTestId('bulk-select');
+ fireEvent.click(bulkSelectButton);
- await waitFor(() => {
- expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
- expect(screen.getByText(mockCharts[1].slice_name)).toBeInTheDocument();
- });
-
- const bulkSelectButton = screen.getByTestId('bulk-select');
- await userEvent.click(bulkSelectButton);
+ await waitFor(() => {
+ // Expect header checkbox + one checkbox per chart
+ expect(screen.getAllByRole('checkbox')).toHaveLength(
+ mockCharts.length + 1,
+ );
+ });
+
+ // Select first chart
+ const table = screen.getByTestId('listview-table');
+ // Target first data row specifically (not header row)
+ const dataRows = within(table).getAllByTestId('table-row');
+ const firstRowCheckbox = within(dataRows[0]).getByRole('checkbox');
+ fireEvent.click(firstRowCheckbox);
+
+ await waitFor(() => {
+ expect(screen.getByTestId('bulk-select-copy')).toHaveTextContent(
+ '1 Selected',
+ );
+ });
- await waitFor(() => {
- // Expect header checkbox + one checkbox per chart
- expect(screen.getAllByRole('checkbox')).toHaveLength(mockCharts.length +
1);
+ const bulkActionButton = screen.getByTestId('bulk-select-action');
+ expect(bulkActionButton).toBeInTheDocument();
});
- // Use select all to select multiple charts
- const selectAllCheckbox = screen.getAllByLabelText('Select all')[0];
- await userEvent.click(selectAllCheckbox);
+ test('exit bulk select by hitting x on bulk select bar', async () => {
+ renderChartList(mockUser);
- await waitFor(() => {
- expect(screen.getByTestId('bulk-select-copy')).toHaveTextContent(
- `${mockCharts.length} Selected`,
- );
- });
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
- // Click bulk delete button
- const bulkActions = screen.getAllByTestId('bulk-select-action');
- const deleteButton = bulkActions.find(btn => btn.textContent === 'Delete');
- expect(deleteButton).toBeInTheDocument();
+ await waitFor(() => {
+ expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
+ expect(screen.getByText(mockCharts[1].slice_name)).toBeInTheDocument();
+ });
- await userEvent.click(deleteButton!);
+ const bulkSelectButton = screen.getByTestId('bulk-select');
+ fireEvent.click(bulkSelectButton);
- // Should open delete confirmation modal
- await waitFor(() => {
- const deleteModal = screen.getByRole('dialog');
- expect(deleteModal).toBeInTheDocument();
- expect(deleteModal).toHaveTextContent(/delete/i);
- expect(deleteModal).toHaveTextContent(/selected charts/i);
- });
-});
+ await waitFor(() => {
+ // Expect header checkbox + one checkbox per chart
+ expect(screen.getAllByRole('checkbox')).toHaveLength(
+ mockCharts.length + 1,
+ );
+ });
-test('supports bulk add tags to selected charts', async () => {
- // Enable tagging system feature flag
- mockIsFeatureEnabled.mockImplementation(
- feature => feature === 'TAGGING_SYSTEM',
- );
+ const table = screen.getByTestId('listview-table');
+ // Target first data row specifically (not header row)
+ const dataRows = within(table).getAllByTestId('table-row');
+ const firstRowCheckbox = within(dataRows[0]).getByRole('checkbox');
+ fireEvent.click(firstRowCheckbox);
- renderChartList(mockUser);
+ await waitFor(() => {
+ expect(screen.getByTestId('bulk-select-copy')).toHaveTextContent(
+ '1 Selected',
+ );
+ });
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
- });
+ // Find and click the close button (x) on the bulk select bar
+ const closeIcon = document.querySelector(
+ '.ant-alert-close-icon',
+ ) as HTMLButtonElement;
+ fireEvent.click(closeIcon);
- // Wait for chart data to load
- await waitFor(() => {
- expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
- expect(screen.getByText(mockCharts[1].slice_name)).toBeInTheDocument();
+ await waitFor(() => {
+ expect(screen.queryAllByRole('checkbox')).toHaveLength(0);
+ expect(screen.queryByTestId('bulk-select-copy')).not.toBeInTheDocument();
+ });
});
- // Activate bulk select and select charts
- const bulkSelectButton = screen.getByTestId('bulk-select');
- await userEvent.click(bulkSelectButton);
+ test('exit bulk select by clicking bulk select button again', async () => {
+ renderChartList(mockUser);
- await waitFor(() => {
- // Expect header checkbox + one checkbox per chart
- expect(screen.getAllByRole('checkbox')).toHaveLength(mockCharts.length +
1);
- });
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
- // Select first chart
- const table = screen.getByTestId('listview-table');
- // Target first data row specifically (not header row)
- const dataRows = within(table).getAllByTestId('table-row');
- const firstRowCheckbox = within(dataRows[0]).getByRole('checkbox');
- await userEvent.click(firstRowCheckbox);
+ await waitFor(() => {
+ expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
+ expect(screen.getByText(mockCharts[1].slice_name)).toBeInTheDocument();
+ });
- await waitFor(() => {
- expect(screen.getByTestId('bulk-select-copy')).toHaveTextContent(
- '1 Selected',
- );
- });
+ const bulkSelectButton = screen.getByTestId('bulk-select');
+ fireEvent.click(bulkSelectButton);
- const addTagButton = screen.queryByText('Add Tag') as HTMLButtonElement;
- expect(addTagButton).toBeInTheDocument();
- await userEvent.click(addTagButton);
+ await waitFor(() => {
+ // Expect header checkbox + one checkbox per chart
+ expect(screen.getAllByRole('checkbox')).toHaveLength(
+ mockCharts.length + 1,
+ );
+ });
- await waitFor(() => {
- const tagModal = screen.getByRole('dialog');
- expect(tagModal).toBeInTheDocument();
- expect(tagModal).toHaveTextContent(/tag/i);
- });
-});
+ const table = screen.getByTestId('listview-table');
+ // Target first data row specifically (not header row)
+ const dataRows = within(table).getAllByTestId('table-row');
+ const firstRowCheckbox = within(dataRows[0]).getByRole('checkbox');
+ fireEvent.click(firstRowCheckbox);
-test('exits bulk select on button toggle', async () => {
- renderChartList(mockUser);
+ await waitFor(() => {
+ expect(screen.getByTestId('bulk-select-copy')).toHaveTextContent(
+ '1 Selected',
+ );
+ });
- await waitFor(() => {
- expect(screen.getByTestId('listview-table')).toBeInTheDocument();
- });
+ fireEvent.click(bulkSelectButton);
- await waitFor(() => {
- expect(screen.getByText(mockCharts[0].slice_name)).toBeInTheDocument();
- expect(screen.getByText(mockCharts[1].slice_name)).toBeInTheDocument();
+ await waitFor(() => {
+ expect(screen.queryAllByRole('checkbox')).toHaveLength(0);
+ expect(screen.queryByTestId('bulk-select-copy')).not.toBeInTheDocument();
+ });
});
- const bulkSelectButton = screen.getByTestId('bulk-select');
- await userEvent.click(bulkSelectButton);
+ test('displays dataset name without schema prefix correctly', async () => {
+ // Test just name case - should display the full name when no schema prefix
+ renderChartList(mockUser);
- await waitFor(() => {
- // Expect header checkbox + one checkbox per chart
- expect(screen.getAllByRole('checkbox')).toHaveLength(mockCharts.length +
1);
- });
+ await waitFor(() => {
+ expect(screen.getByTestId('listview-table')).toBeInTheDocument();
+ });
- const table = screen.getByTestId('listview-table');
- // Target first data row specifically (not header row)
- const dataRows = within(table).getAllByTestId('table-row');
- const firstRowCheckbox = within(dataRows[0]).getByRole('checkbox');
- await userEvent.click(firstRowCheckbox);
+ const table = screen.getByTestId('listview-table');
- await waitFor(() => {
- expect(screen.getByTestId('bulk-select-copy')).toHaveTextContent(
- '1 Selected',
- );
- });
+ // Wait for chart with simple dataset name to load
+ await waitFor(() => {
+ expect(
+ within(table).getByText(mockCharts[1].slice_name),
+ ).toBeInTheDocument();
+ });
- await userEvent.click(bulkSelectButton);
+ // Test mockCharts[1] which has 'sales_data' (no schema prefix)
+ const chart1Row = within(table)
+ .getByText(mockCharts[1].slice_name)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Fix incorrect test selector</b></div>
<div id="fix">
Change the CSS selector from '[data-test="table-row"]' to
'[data-testid="table-row"]' to match the attribute used by the component.
</div>
</div>
<small><i>Code Review Run #b3d4a2</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
