fix(reports): resolve edit-mode report by creationMethod scope

The ReportModal selected the existing report to edit by keying off `dashboardId` first, while the save payload already keyed off `creationMethod`. In Explore, a chart opened from a dashboard carries both a `dashboardId` context prop and a chart-scoped `creationMethod`, so the modal could load an unrelated dashboard-scoped report into edit mode; saving it then sent a chart-only payload to the dashboard report's id, targeting the wrong resource.

Resolve the existing report using the same `creationMethod`-based scope and id the save payload uses, so a chart Explore context never edits a dashboard-scoped report, and vice versa. Add a regression test covering the chart-opened-from-dashboard case.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

.github/labeler.yml

@@ -29,7 +29,7 @@
 "dependencies:python":
 - changed-files:
   - any-glob-to-any-file:
-    - 'requirements/**'
+    - 'superset/requirements/**'
     - 'superset/translations/requirements.txt'
     - 'RELEASING/requirements.txt'
 

superset-frontend/src/components/Chart/useDrillDetailMenuItems/useDrillDetailMenuItems.test.tsx

@@ -17,7 +17,6 @@
  * under the License.
  */
 import { useState } from 'react';
-import fetchMock from 'fetch-mock';
 import {
   cleanup,
   render,
@@ -36,10 +35,6 @@ import { useDrillDetailMenuItems, DrillDetailMenuItemsProps } from './index';
 
 /* eslint jest/expect-expect: ["warn", { "assertFunctionNames": ["expect*"] }] */
 
-// Opening the context menu logs an event, and an unmatched request makes
-// fetch-mock throw inside the component.
-fetchMock.post('glob:*/log/?*', {});
-
 jest.mock(
   '../DrillDetail/DrillDetailPane',
   () =>

superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/AdhocFilter.test.ts

@@ -207,38 +207,6 @@ describe('AdhocFilter', () => {
     expect(adhocFilter10.isValid()).toBe(true);
   });
 
-  test('is invalid when a comparator-taking operator has no comparator', () => {
-    // A comparator that was never set, or that was cleared through the value
-    // Select's clear affordance, is `undefined` rather than `null` or `[]`.
-    const adhocFilter1 = new AdhocFilter({
-      expressionType: ExpressionTypes.Simple,
-      subject: 'is_intro',
-      operator: 'IN',
-      comparator: undefined,
-      clause: Clauses.Where,
-    });
-    expect(adhocFilter1.isValid()).toBe(false);
-
-    const adhocFilter2 = new AdhocFilter({
-      expressionType: ExpressionTypes.Simple,
-      subject: 'is_intro',
-      operator: '==',
-      comparator: undefined,
-      clause: Clauses.Where,
-    });
-    expect(adhocFilter2.isValid()).toBe(false);
-
-    // `false` is a legitimate boolean comparator, not a missing value
-    const adhocFilter3 = new AdhocFilter({
-      expressionType: ExpressionTypes.Simple,
-      subject: 'is_intro',
-      operator: '==',
-      comparator: false,
-      clause: Clauses.Where,
-    });
-    expect(adhocFilter3.isValid()).toBe(true);
-  });
-
   test('can translate from simple expressions to sql expressions', () => {
     const adhocFilter1 = new AdhocFilter({
       expressionType: ExpressionTypes.Simple,

superset-frontend/src/explore/components/controls/FilterControl/AdhocFilter/index.ts

@@ -163,10 +163,8 @@ export default class AdhocFilter {
           // A non-empty array of values ('IN' or 'NOT IN' clauses)
           return this.comparator.length > 0;
         }
-        // A value has been selected or typed. An unset comparator is
-        // `undefined` rather than `null`: picking a new subject resets it, and
-        // the value Select's clear affordance emits `undefined` too.
-        return this.comparator != null;
+        // A value has been selected or typed
+        return this.comparator !== null;
       }
     }
 

superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/AdhocFilterEditPopover.test.tsx

@@ -181,29 +181,6 @@ describe('AdhocFilterEditPopover', () => {
     expect(saveButton).toBeDisabled();
   });
 
-  test('disables save button when a boolean column has no value selected', async () => {
-    const booleanColumn = { type: 'BOOL', column_name: 'is_intro' };
-    renderPopover({
-      adhocFilter: new AdhocFilter({
-        expressionType: ExpressionTypes.Simple,
-        clause: Clauses.Where,
-      }),
-      options: [booleanColumn],
-      datasource: { columns: [booleanColumn], filter_select: false },
-    });
-
-    // Picking the subject resets the comparator to `undefined`; the value
-    // control is then left untouched, mirroring the reported repro.
-    await userEvent.click(screen.getByTestId('select-element'));
-    await userEvent.click(
-      await screen.findByRole('option', { name: /is_intro/ }),
-    );
-
-    expect(
-      screen.getByTestId('adhoc-filter-edit-popover-save-button'),
-    ).toBeDisabled();
-  });
-
   test('initiates resize when resize handle is dragged', async () => {
     const onResize = jest.fn();
     renderPopover({ onResize });

superset-frontend/src/features/reports/ReportModal/ReportModal.test.tsx

@@ -170,10 +170,54 @@ test('creates a new email report via modal Add button', async () => {
   // creation_method, editors, and recipients are set server-side; not in the client payload
   expect(body.creation_method).toBeUndefined();
   expect(body.recipients).toBeUndefined();
+  // Dashboard-scoped report (creationMethod='dashboards'): send `dashboard`,
+  // never `chart`, so the backend does not reject "both".
+  expect(body.dashboard).toBe(1);
+  expect(body.chart).toBeUndefined();
 
   fetchMock.removeRoute('post-subscribe');
 });
 
+test('sends only chart, not dashboard, when creating a chart report from a dashboard context', async () => {
+  // Regression: opening a chart in Explore *from a dashboard* passes the modal a
+  // `dashboardId` context prop while the report stays chart-scoped
+  // (creationMethod='charts'). The payload must carry only `chart`; sending both
+  // `chart` and `dashboard` makes the backend reject with a 422
+  // "Choose a chart or dashboard not both".
+  fetchMock.post(
+    'glob:*/api/v1/report/subscribe',
+    { id: 1, result: {} },
+    { name: 'post-subscribe-chart' },
+  );
+
+  // Routes persist across tests in this file, so always remove this one — even if
+  // an assertion below throws — to avoid shadowing later tests' report routes.
+  try {
+    const chartFromDashboardProps = {
+      ...defaultProps,
+      creationMethod: 'charts' as const,
+      dashboardId: 7,
+      chart: { id: 119, sliceFormData: { viz_type: VizType.Line } },
+    };
+    render(<ReportModal {...chartFromDashboardProps} />, { useRedux: true });
+
+    const addButton = screen.getByRole('button', { name: /add/i });
+    await waitFor(() => userEvent.click(addButton));
+
+    await waitFor(() => {
+      const postCalls = fetchMock.callHistory.calls('post-subscribe-chart');
+      expect(postCalls).toHaveLength(1);
+    });
+
+    const postCalls = fetchMock.callHistory.calls('post-subscribe-chart');
+    const body = JSON.parse(postCalls[0].options.body as string);
+    expect(body.chart).toBe(119);
+    expect(body.dashboard).toBeUndefined();
+  } finally {
+    fetchMock.removeRoute('post-subscribe-chart');
+  }
+});
+
 test('text-based chart hides screenshot width and shows message content', () => {
   // Table is text-based: should show message content but hide custom width
   const textChartProps = {
@@ -288,6 +332,51 @@ test('renders edit mode when report exists in store', () => {
   expect(screen.getByRole('button', { name: /save/i })).toBeInTheDocument();
 });
 
+test('resolves the chart-scoped report, not the dashboard-scoped one, in a dashboard context', () => {
+  // Regression: opening a chart in Explore *from a dashboard* passes the modal
+  // both a `dashboardId` context prop and a chart-scoped `creationMethod`.
+  // Edit-mode resolution must follow the same scope as the save payload
+  // (`creationMethod`); keying off `dashboardId` first loads the unrelated
+  // dashboard-scoped report, so a save then targets the wrong report id.
+  const dashboardReport = {
+    id: 42,
+    name: 'Existing Dashboard Report',
+    creation_method: 'dashboards',
+    dashboard: 7,
+  };
+  const chartReport = {
+    id: 77,
+    name: 'Existing Chart Report',
+    creation_method: 'charts',
+    chart: 119,
+  };
+  const store = createStore(
+    {
+      reports: {
+        dashboards: { 7: dashboardReport },
+        charts: { 119: chartReport },
+      },
+    },
+    reducerIndex,
+  );
+
+  const chartFromDashboardProps = {
+    ...defaultProps,
+    creationMethod: 'charts' as const,
+    dashboardId: 7,
+    chart: { id: 119, sliceFormData: { viz_type: VizType.Line } },
+  };
+  render(<ReportModal {...chartFromDashboardProps} />, {
+    useRedux: true,
+    store,
+  });
+
+  // The modal must load the chart's own report, not the dashboard's.
+  const reportNameTextbox = screen.getByTestId('report-name-test');
+  expect(reportNameTextbox).toHaveDisplayValue('Existing Chart Report');
+  expect(reportNameTextbox).not.toHaveDisplayValue('Existing Dashboard Report');
+});
+
 test('edit mode dispatches editReport via PUT on save', async () => {
   const existingReport = {
     id: 42,

superset-frontend/src/features/reports/ReportModal/index.tsx

@@ -165,13 +165,16 @@ function ReportModal({
   const dispatch = useDispatch();
   // Report fetch logic
   const report = useSelector<any, ReportObject>(state => {
-    const resourceType = dashboardId
-      ? CreationMethod.Dashboards
-      : CreationMethod.Charts;
-    return (
-      reportSelector(state, resourceType, dashboardId || chart?.id) ||
-      EMPTY_OBJECT
-    );
+    // Resolve the existing report with the same scope the save payload uses
+    // (`creationMethod`). Explore can carry a `dashboardId` context prop even for
+    // a chart-scoped report, so keying off `dashboardId` first would load an
+    // unrelated dashboard report and a later save would target the wrong id.
+    const isChartReport = creationMethod === CreationMethod.Charts;
+    const resourceType = isChartReport
+      ? CreationMethod.Charts
+      : CreationMethod.Dashboards;
+    const resourceId = isChartReport ? chart?.id : dashboardId;
+    return reportSelector(state, resourceType, resourceId) || EMPTY_OBJECT;
   });
   const isEditMode = report && Object.keys(report).length;
 
@@ -189,8 +192,13 @@ function ReportModal({
       active: true,
       force_screenshot: false,
       custom_width: currentReport.custom_width,
-      dashboard: dashboardId,
-      chart: chart?.id,
+      // A report belongs to either a chart or a dashboard, never both. Explore can
+      // carry dashboard context even for a chart-scoped report, so send only the
+      // entity that matches the creation method; a payload with both `chart` and
+      // `dashboard` is rejected by the backend with a 422 error.
+      ...(creationMethod === CreationMethod.Charts
+        ? { chart: chart?.id }
+        : { dashboard: dashboardId }),
       name: currentReport.name,
       description: currentReport.description,
       crontab: currentReport.crontab,

superset/db_engine_specs/README.md

@@ -83,7 +83,7 @@ The tables below (generated via `python superset/db_engine_specs/lib.py`) summar
 | Databricks (legacy) | 70 | Supported | Partial | Supported | Partial | Partial | Not supported |
 | StarRocks | 69 | Supported | Partial | Supported | Partial | Partial | Partial |
 | SingleStore | 68 | Supported | Partial | Supported | Not supported | Partial | Not supported |
-| ClickHouse Connect (Superset) | 62 | Supported | Partial | Supported | Partial | Partial | Not supported |
+| ClickHouse Connect (Superset) | 61 | Supported | Partial | Partial | Partial | Partial | Not supported |
 | Google Sheets | 61 | Supported | Partial | Supported | Supported | Partial | Partial |
 | Aurora MySQL (Data API) | 59 | Supported | Partial | Supported | Partial | Partial | Not supported |
 | MariaDB | 59 | Supported | Partial | Supported | Partial | Partial | Not supported |
@@ -91,7 +91,7 @@ The tables below (generated via `python superset/db_engine_specs/lib.py`) summar
 | OceanBase | 59 | Supported | Partial | Supported | Partial | Partial | Not supported |
 | MotherDuck | 58 | Supported | Partial | Supported | Not supported | Partial | Not supported |
 | KustoSQL | 54 | Supported | Partial | Supported | Partial | Partial | Not supported |
-| ClickHouse | 52 | Supported | Partial | Supported | Partial | Partial | Not supported |
+| ClickHouse | 51 | Supported | Partial | Partial | Partial | Partial | Not supported |
 | Databend | 51 | Supported | Partial | Supported | Partial | Partial | Not supported |
 | Apache Drill | 50 | Supported | Partial | Supported | Partial | Partial | Partial |
 | Apache Druid | 47 | Partial | Partial | Supported | Partial | Partial | Not supported |
@@ -293,8 +293,8 @@ The tables below (generated via `python superset/db_engine_specs/lib.py`) summar
 | Aurora MySQL (Data API) | True | True | True | True | True | True | True | True |
 | Aurora PostgreSQL (Data API) | True | True | True | True | True | True | True | True |
 | Azure Synapse | True | True | True | True | True | True | True | True |
-| ClickHouse | True | True | True | True | True | True | True | True |
-| ClickHouse Connect (Superset) | True | True | True | True | True | True | True | True |
+| ClickHouse | False | True | True | True | True | True | True | True |
+| ClickHouse Connect (Superset) | False | True | True | True | True | True | True | True |
 | CockroachDB | True | True | True | True | True | True | True | True |
 | Couchbase | True | True | True | True | False | True | True | True |
 | CrateDB | True | True | True | True | True | True | True | True |

superset/db_engine_specs/clickhouse.py

@@ -112,7 +112,6 @@ class ClickHouseBaseEngineSpec(BaseEngineSpec):
 
     _time_grain_expressions = {
         None: "{col}",
-        "PT1S": "toStartOfSecond(toDateTime64({col}, 3))",
         "PT1M": "toStartOfMinute(toDateTime({col}))",
         "PT5M": "toDateTime(intDiv(toUInt32(toDateTime({col})), 300)*300)",
         "PT10M": "toDateTime(intDiv(toUInt32(toDateTime({col})), 600)*600)",

tests/unit_tests/db_engine_specs/test_clickhouse.py

@@ -62,20 +62,6 @@ def test_convert_dttm(
     assert_convert_dttm(spec, target_type, expected_result, dttm)
 
 
-@pytest.mark.parametrize(
-    "time_grain,expected",
-    [
-        (None, "{col}"),
-        ("PT1S", "toStartOfSecond(toDateTime64({col}, 3))"),
-        ("PT1M", "toStartOfMinute(toDateTime({col}))"),
-    ],
-)
-def test_time_grain_expressions(time_grain: Optional[str], expected: str) -> None:
-    from superset.db_engine_specs.clickhouse import ClickHouseBaseEngineSpec
-
-    assert ClickHouseBaseEngineSpec._time_grain_expressions[time_grain] == expected
-
-
 def test_convert_dttm_normalizes_aware_datetime_to_utc() -> None:
     from superset.db_engine_specs.clickhouse import (
         ClickHouseEngineSpec as spec,  # noqa: N813