sadpandajoe commented on code in PR #34785:
URL: https://github.com/apache/superset/pull/34785#discussion_r4051126653
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx:
##########
@@ -33,37 +33,46 @@ import { isEmbedded } from 'src/dashboard/util/isEmbedded';
import { Slice } from 'src/types/Chart';
import { RootState } from 'src/dashboard/types';
import { findPermission } from 'src/utils/findPermission';
+import { useToasts } from 'src/components/MessageToasts/withToasts';
+import { getFormDataWithDashboardContext } from
'src/explore/controlUtils/getFormDataWithDashboardContext';
+import { useDashboardFormData } from
'src/dashboard/hooks/useDashboardFormData';
+import { generateExploreUrl } from 'src/explore/exploreUtils/formData';
import { Dataset } from '../types';
import DrillDetailPane from './DrillDetailPane';
interface ModalFooterProps {
canExplore: boolean;
closeModal?: () => void;
- exploreChart: () => void;
+ showEditButton: boolean;
+ onExploreClick?: (event: React.MouseEvent) => void;
+ isGeneratingUrl: boolean;
}
const ModalFooter = ({
canExplore,
closeModal,
- exploreChart,
+ showEditButton,
+ onExploreClick,
+ isGeneratingUrl,
}: ModalFooterProps) => {
const theme = useTheme();
return (
<>
- {!isEmbedded() && (
+ {!isEmbedded() && showEditButton && (
Review Comment:
This hides the existing Explore/Edit action for every dataset without the
new configuration, so the claimed no-op fallback removes the only shortcut back
to the source chart after upgrade. Should the unconfigured path keep the
previous source-chart action?
##########
superset/datasets/schemas.py:
##########
@@ -174,6 +174,7 @@ class DatasetPutSchema(Schema):
columns = fields.List(fields.Nested(DatasetColumnsPutSchema))
metrics = fields.List(fields.Nested(DatasetMetricsPutSchema))
folders = fields.List(fields.Nested(FolderSchema), required=False)
+ drill_through_chart_id = fields.Integer(allow_none=True)
Review Comment:
The runtime dataset API now accepts and returns this field, but the
checked-in OpenAPI document has no `drill_through_chart_id` property. Generated
clients therefore cannot configure the feature and may reject the response;
could the API specification be regenerated with this field?
##########
superset-frontend/src/components/Select/ChartSelect.tsx:
##########
@@ -0,0 +1,105 @@
+/**
+ * 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 { useMemo } from 'react';
+import { t } from '@superset-ui/core';
+import SelectAsyncControl from
'src/explore/components/controls/SelectAsyncControl';
+import type { ComponentProps } from 'react';
+import rison from 'rison';
+
+// Extract the actual props from SelectAsyncControl component
+type SelectAsyncControlProps = ComponentProps<typeof SelectAsyncControl>;
+
+export interface ChartSelectProps
+ extends Omit<
+ SelectAsyncControlProps,
+ 'onChange' | 'dataEndpoint' | 'mutator' | 'addDangerToast' | 'multi'
+ > {
+ // ChartSelect-specific props that override base props
+ value?: number | null;
+ onChange: (value: number | null) => void;
+ datasetId?: number;
+}
+
+/**
+ * A chart selection component built on SelectAsyncControl
+ * @param value - The selected chart ID
+ * @param onChange - Callback when selection changes
+ * @param datasetId - Optional dataset ID to filter charts
+ * @param placeholder - Optional placeholder text
+ * @param ariaLabel - ARIA label for accessibility
+ * @param rest - All other props are passed through to SelectAsyncControl
+ */
+export default function ChartSelectUsingAsync({
+ value,
+ onChange,
+ datasetId,
+ placeholder = t('Select a chart'),
+ ariaLabel = t('Select drill-to-details chart'),
+ ...rest
+}: ChartSelectProps) {
+ // Build query parameters for filtering charts by dataset
+ const queryParams = useMemo(() => {
+ if (!datasetId) return undefined;
+
+ const filters = [
+ {
+ col: 'datasource_id',
+ opr: 'eq',
+ value: datasetId,
+ },
+ {
+ col: 'datasource_type',
+ opr: 'eq',
+ value: 'table',
+ },
+ ];
+
+ return {
+ q: rison.encode({
Review Comment:
This request gets only Flask-AppBuilder’s default 20-row page, and
`SelectAsyncControl` neither paginates nor sends a server-side search term. A
valid chart that sorts after the first page can never be selected; can this use
the paginated async selector instead?
##########
superset/datasets/schemas.py:
##########
@@ -174,6 +174,7 @@ class DatasetPutSchema(Schema):
columns = fields.List(fields.Nested(DatasetColumnsPutSchema))
metrics = fields.List(fields.Nested(DatasetMetricsPutSchema))
folders = fields.List(fields.Nested(FolderSchema), required=False)
+ drill_through_chart_id = fields.Integer(allow_none=True)
Review Comment:
Agreed—the UI restricts this relationship to charts from the dataset, but
the PUT contract accepts any slice ID; the renderer then keeps that chart’s
controls while replacing its datasource, so incompatible columns or metrics
fail the drill query. Should the update command verify that the target chart
uses this dataset?
##########
superset-frontend/src/dashboard/hooks/useDashboardFormData.ts:
##########
@@ -0,0 +1,108 @@
+/**
+ * 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 { useMemo } from 'react';
+import { useSelector } from 'react-redux';
+import { RootState, DashboardContextFormData } from '../types';
+import { getExtraFormData } from '../components/nativeFilters/utils';
+import { getAllActiveFilters } from '../util/activeAllDashboardFilters';
+import { getFilterIdsAppliedOnChart } from
'../util/getFilterIdsAppliedOnChart';
+
+/**
+ * Hook that provides dashboard context as formatted formData for charts.
+ * This encapsulates all the complex logic for determining which dashboard
+ * filters, colors, and other context should be applied to a specific chart.
+ *
+ * @param chartId - The ID of the chart to get dashboard context for
+ * @returns Dashboard context formatted as QueryFormData fields
+ */
+export const useDashboardFormData = (
+ chartId: number | null | undefined,
+): DashboardContextFormData => {
+ // Dashboard state selectors
+ const dashboardId = useSelector<RootState, number>(
+ ({ dashboardInfo }) => dashboardInfo.id,
+ );
+
+ const nativeFilters = useSelector(
+ (state: RootState) => state.nativeFilters?.filters,
+ );
+
+ const dataMask = useSelector((state: RootState) => state.dataMask);
+
+ const chartConfiguration = useSelector(
+ (state: RootState) =>
+ state.dashboardInfo.metadata?.chart_configuration || {},
+ );
+
+ const allSliceIds = useSelector(
+ (state: RootState) => state.dashboardState.sliceIds,
+ );
+
+ // Compute dashboard context for the chart
+ return useMemo((): DashboardContextFormData => {
+ const baseContext: DashboardContextFormData = { dashboardId };
+
+ // Early return if we don't have required data or chartId
+ if (
+ chartId == null ||
+ !nativeFilters ||
+ !dataMask ||
+ !chartConfiguration ||
+ !allSliceIds
+ ) {
+ return baseContext;
+ }
+
+ // Get active filters using the same logic as normal dashboard charts
+ const activeFilters = getAllActiveFilters({
+ chartConfiguration,
+ nativeFilters,
+ dataMask,
+ allSliceIds,
+ });
+
+ // Find which filters apply to this specific chart
+ const filterIdsAppliedOnChart = getFilterIdsAppliedOnChart(
+ activeFilters,
+ chartId,
+ );
+
+ // If no filters apply, return just the base context
+ if (filterIdsAppliedOnChart.length === 0) {
+ return baseContext;
Review Comment:
Agreed—the target chart is normally off-dashboard, so this lookup finds no
native-filter scope and the drill result can ignore filters applied to the
source chart. Should the context be derived from the source chart ID instead?
--
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]
