codeant-ai-for-open-source[bot] commented on code in PR #44848:
URL: https://github.com/apache/superset/pull/44848#discussion_r4150664515
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -1309,3 +1328,297 @@ test('automatic axis title margin adjustment handles
both X and Y axis titles be
jest.restoreAllMocks();
}
});
+
+const getChart = (store: Store) =>
+ (
+ store.getState() as {
+ charts: Record<number, { triggerQuery?: boolean }>;
+ }
+ ).charts[1];
+
+test.each([
+ ['Ctrl', '{Control>}{Enter}{/Control}'],
+ ['Cmd', '{Meta>}{Enter}{/Meta}'],
+])('%s+Enter anywhere on the page queues the chart query', async (_, keys) => {
+ setupTableChartControlPanel();
+ try {
+ const store = createStore(reduxState, reducerIndex);
+ renderWithRouter({ initialState: reduxState, store: store as Store });
+ await screen.findByTestId('control-panels-container');
+
+ // Mounting queues a query; clear it so only the shortcut can set it again.
+ act(() => {
+ store.dispatch(chartActions.triggerQuery(false, 1));
+ });
+ expect(getChart(store as Store).triggerQuery).toBe(false);
+
+ await userEvent.keyboard(keys);
+
+ await waitFor(() =>
+ expect(getChart(store as Store).triggerQuery).toBe(true),
+ );
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test.each([
+ ['Enter without a modifier', '{Enter}'],
+ ['Ctrl with another key', '{Control>}a{/Control}'],
+])('%s does not queue the chart query', async (_, keys) => {
+ setupTableChartControlPanel();
+ try {
+ const store = createStore(reduxState, reducerIndex);
+ renderWithRouter({ initialState: reduxState, store: store as Store });
+ await screen.findByTestId('control-panels-container');
+ act(() => {
+ store.dispatch(chartActions.triggerQuery(false, 1));
+ });
+
+ await userEvent.keyboard(keys);
+
+ expect(getChart(store as Store).triggerQuery).toBe(false);
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+const stateWithControls = (controls: object) => ({
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: { ...reduxState.explore.controls, ...controls },
+ },
+});
+
+test('keeps the Run button enabled when no control has validation errors',
async () => {
+ setupTableChartControlPanel();
+ try {
+ renderWithRouter({
+ initialState: stateWithControls({
+ metric: { value: 'count', label: 'Metric' },
+ }),
+ });
+
+ const panel = await screen.findByTestId('control-panels-container');
+ expect(panel).toHaveAttribute('data-has-button-error', 'false');
+ expect(screen.getByRole('button', { name: 'Update chart' })).toBeEnabled();
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('disables the Run button and reports the error when a control fails
validation', async () => {
+ setupTableChartControlPanel();
+ try {
+ renderWithRouter({
+ initialState: stateWithControls({
+ metric: {
+ value: '',
+ label: 'Metric',
+ validationErrors: ['Metric is required'],
+ },
+ }),
+ });
+
+ const panel = await screen.findByTestId('control-panels-container');
+ expect(panel).toHaveAttribute('data-has-button-error', 'true');
+ expect(panel).toHaveAttribute('data-has-data-tab-error', 'true');
+ expect(screen.getByRole('button', { name: 'Update chart'
})).toBeDisabled();
+ expect(screen.getByRole('tooltip')).toHaveTextContent(
+ 'Control labeled Metric: Metric is required',
+ );
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+test('disables the Run button for a matrixify control error without flagging
the Data tab', async () => {
+ setupTableChartControlPanel();
+ try {
+ renderWithRouter({
+ initialState: stateWithControls({
+ matrixify_rows: {
+ value: '',
+ label: 'Rows',
+ tabOverride: 'matrixify',
+ validationErrors: ['Rows are required'],
+ },
+ }),
+ });
+
+ const panel = await screen.findByTestId('control-panels-container');
+ expect(panel).toHaveAttribute('data-has-button-error', 'true');
+ expect(panel).toHaveAttribute('data-has-data-tab-error', 'false');
+ expect(screen.getByRole('button', { name: 'Update chart'
})).toBeDisabled();
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ }
+});
+
+const tooltipState = (vizType: string, template = '') => ({
+ ...reduxState,
+ charts: {
+ 1: {
+ ...reduxState.charts[1],
+ latestQueryFormData: {
+ ...reduxState.charts[1].latestQueryFormData,
+ viz_type: vizType,
+ },
+ },
+ },
+ explore: {
+ ...reduxState.explore,
+ form_data: { datasource: '1__table', viz_type: vizType, metrics: [] },
+ controls: {
+ ...reduxState.explore.controls,
+ viz_type: { value: vizType },
+ tooltip_contents: { value: [] },
+ tooltip_template: { value: template },
+ },
+ },
+});
+
+// Renders the container and returns a function that selects tooltip contents
+// the way the control would and reports the template the container wrote back.
+const renderTooltipChart = (vizType: string, template = '') => {
+ getChartControlPanelRegistry().registerValue(vizType, {
+ controlPanelSections: [],
+ });
+ const setControlValueSpy = jest.spyOn(exploreActions, 'setControlValue');
+ const initialState = tooltipState(vizType, template);
+ const store = createStore(initialState, reducerIndex);
+ renderWithRouter({ initialState, store: store as Store });
+ setControlValueSpy.mockClear();
+ return {
+ selectTooltipContents: (contents: unknown[]) => {
+ act(() => {
+ store.dispatch(
+ exploreActions.setControlValue('tooltip_contents', contents),
+ );
+ });
+ },
+ templateWrites: () =>
+ setControlValueSpy.mock.calls
+ .filter(([controlName]) => controlName === 'tooltip_template')
+ .map(([, value]) => value),
+ storedTemplate: () =>
+ ((store as Store).getState() as ExplorePageState).explore.controls
+ .tooltip_template?.value,
+ cleanup: () => getChartControlPanelRegistry().remove(vizType),
+ };
+};
+
+test('appends a {{ field }} variable to the tooltip template for a selected
column', () => {
+ const chart = renderTooltipChart('deck_scatter');
+ try {
+ chart.selectTooltipContents([{ item_type: 'column', column_name: 'name'
}]);
+
+ expect(chart.templateWrites()).toEqual(['{{ name }}']);
+ expect(chart.storedTemplate()).toBe('{{ name }}');
+ } finally {
+ chart.cleanup();
+ }
+});
+
+test('appends after the existing tooltip template text', () => {
+ const chart = renderTooltipChart('deck_scatter', 'Details:');
+ try {
+ chart.selectTooltipContents([{ item_type: 'column', column_name: 'name'
}]);
+
+ expect(chart.templateWrites()).toEqual(['Details: {{ name }}']);
+ expect(chart.storedTemplate()).toBe('Details: {{ name }}');
+ } finally {
+ chart.cleanup();
+ }
+});
+
+test('appends a limited variable for a column on an aggregated chart', () => {
+ const chart = renderTooltipChart('heatmap');
+ try {
+ chart.selectTooltipContents([{ item_type: 'column', column_name: 'name'
}]);
+
+ expect(chart.templateWrites()).toEqual(['{{ limit name 10 }}']);
+ expect(chart.storedTemplate()).toBe('{{ limit name 10 }}');
+ } finally {
+ chart.cleanup();
+ }
+});
+
+test('appends a plain variable for a metric even on an aggregated chart', ()
=> {
+ const chart = renderTooltipChart('heatmap');
+ try {
+ chart.selectTooltipContents([
+ { item_type: 'metric', metric_name: 'count' },
+ ]);
+
+ expect(chart.templateWrites()).toEqual(['{{ count }}']);
+ expect(chart.storedTemplate()).toBe('{{ count }}');
+ } finally {
+ chart.cleanup();
+ }
+});
+
+test('leaves the tooltip template alone when it already references the field',
() => {
+ const chart = renderTooltipChart('deck_scatter', 'Name: {{ name }}');
+ try {
+ chart.selectTooltipContents([{ item_type: 'column', column_name: 'name'
}]);
+
+ expect(chart.templateWrites()).toEqual([]);
+ expect(chart.storedTemplate()).toBe('Name: {{ name }}');
+ } finally {
+ chart.cleanup();
+ }
+});
+
+const datasourceState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ form_data: { datasource: '1__table', viz_type: VizType.Table, metrics: []
},
+ controls: { ...reduxState.explore.controls, row_limit: { value: 100 } },
+ },
+};
+
+test('requests the datasource metadata for the new datasource when the
datasource control changes', () => {
+ setupTableChartControlPanel();
+ try {
+ const fetchMetadataSpy = jest.spyOn(
+ datasourceActions,
+ 'fetchDatasourceMetadata',
+ );
+ const store = createStore(datasourceState, reducerIndex);
+ renderWithRouter({ initialState: datasourceState, store: store as Store });
+ fetchMetadataSpy.mockClear();
+
+ act(() => {
+ store.dispatch(exploreActions.setControlValue('datasource', '2__table'));
+ });
+
+ expect(fetchMetadataSpy).toHaveBeenCalledTimes(1);
+ expect(fetchMetadataSpy).toHaveBeenCalledWith('2__table');
Review Comment:
**Suggestion:** The effect calls `fetchDatasourceMetadata` with
`props.form_data.datasource`, which remains `1__table`; this expectation
requires `2__table` and fails against the current caller.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Often` ยท ๐ท๏ธ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e50ab52bb2a24898b34229ad147cbd24&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e50ab52bb2a24898b34229ad147cbd24&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx
**Line:** 1599:1599
**Comment:**
*Api Mismatch: The effect calls `fetchDatasourceMetadata` with
`props.form_data.datasource`, which remains `1__table`; this expectation
requires `2__table` and fails against the current caller.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44848&comment_hash=8f61d34de0eed73b98a75c0e3620f8c518cd685411a936c3e99fde22ed8e014e&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44848&comment_hash=8f61d34de0eed73b98a75c0e3620f8c518cd685411a936c3e99fde22ed8e014e&reaction=dislike'>๐</a>
--
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]
