Fix pie rows settings include filtered out values (#47879)

* dedupe a bit

* remove pie row settings for values not from dataset

* remove unnecessary filtering

* series -> rows

* extend spec

* feedback

e2e/test/scenarios/visualizations-charts/pie_chart.cy.spec.js

@@ -172,12 +172,25 @@ describe("scenarios > visualizations > pie chart", () => {
     changeRowLimit(4, 2);
     ensurePieChartRendered(["Gadget", "Doohickey"]);
 
+    // Ensure row settings should show only two rows
+    cy.findByTestId("viz-settings-button").click();
+    getDraggableElements().should("have.length", 2);
+    getDraggableElements().contains("Woooget").should("not.exist");
+    getDraggableElements().contains("Gizmo").should("not.exist");
+
+    cy.findByTestId("Gadget-settings-button").click();
+    cy.findByDisplayValue("Gadget").type("{selectall}Katget").realPress("Tab");
+    moveDnDKitElement(getDraggableElements().contains("Katget"), {
+      vertical: 30,
+    });
+
     changeRowLimit(2, 4);
-    ensurePieChartRendered(["Woooget", "Gadget", "Gizmo", "Doohickey"]);
+    ensurePieChartRendered(["Doohickey", "Katget", "Gizmo", "Woooget"]);
     chartPathWithFillColor("#509EE3").should("be.visible");
 
     cy.findByTestId("chart-legend").within(() => {
-      cy.get("li").eq(2).contains("Woooget");
+      cy.get("li").eq(1).contains("Katget");
+      cy.get("li").eq(3).contains("Woooget");
     });
   });
 });

frontend/src/metabase/visualizations/components/settings/ChartSettingSeriesOrder.tsx

@@ -21,11 +21,14 @@ interface SortableItem {
   enabled: boolean;
   name: string;
   color?: string;
+  hidden?: boolean;
 }
 
 interface ChartSettingSeriesOrderProps {
   onChange: (rows: SortableItem[]) => void;
   value: SortableItem[];
+  addButtonLabel: string;
+  searchPickerPlaceholder: string;
   onShowWidget: (
     widget: { props: { seriesKey: string } },
     ref: HTMLElement | undefined,
@@ -39,6 +42,8 @@ interface ChartSettingSeriesOrderProps {
 export const ChartSettingSeriesOrder = ({
   onChange,
   value: orderedItems,
+  addButtonLabel = t`Add another series`,
+  searchPickerPlaceholder = t`Select a series`,
   onShowWidget,
   hasEditSettings = true,
   onChangeSeriesColor,
@@ -47,7 +52,11 @@ export const ChartSettingSeriesOrder = ({
   const [isSeriesPickerVisible, setSeriesPickerVisible] = useState(false);
 
   const [visibleItems, hiddenItems] = useMemo(
-    () => _.partition(orderedItems, item => item.enabled),
+    () =>
+      _.partition(
+        orderedItems.filter(item => !item.hidden),
+        item => item.enabled,
+      ),
     [orderedItems],
   );
 
@@ -133,13 +142,15 @@ export const ChartSettingSeriesOrder = ({
             <Button
               variant="subtle"
               onClick={() => setSeriesPickerVisible(true)}
-            >{t`Add another series`}</Button>
+            >
+              {addButtonLabel}
+            </Button>
           )}
           {isSeriesPickerVisible && (
             <Select
               initiallyOpened
               searchable
-              placeholder={t`Select a series`}
+              placeholder={searchPickerPlaceholder}
               data={hiddenItems.map(item => ({
                 value: item.key,
                 label: getItemTitle(item),

frontend/src/metabase/visualizations/echarts/pie/model/index.ts

@@ -25,7 +25,7 @@ import type {
   PieSliceData,
 } from "./types";
 
-function getColDescs(
+export function getPieColumns(
   rawSeries: RawSeries,
   settings: ComputedVisualizationSettings,
 ): PieColumnDescriptors {
@@ -71,7 +71,7 @@ export function getPieChartModel(
       data: { rows: dataRows },
     },
   ] = rawSeries;
-  const colDescs = getColDescs(rawSeries, settings);
+  const colDescs = getPieColumns(rawSeries, settings);
 
   const rowIndiciesByKey = new Map<string | number, number>();
   dataRows.forEach((row, index) => {

frontend/src/metabase/visualizations/shared/settings/pie.ts

@@ -5,6 +5,7 @@ import { getColorsForValues } from "metabase/lib/colors/charts";
 import { NULL_DISPLAY_VALUE } from "metabase/lib/constants";
 import { checkNotNull, checkNumber, isNumber } from "metabase/lib/types";
 import { SLICE_THRESHOLD } from "metabase/visualizations/echarts/pie/constants";
+import { getPieColumns } from "metabase/visualizations/echarts/pie/model";
 import type { PieRow } from "metabase/visualizations/echarts/pie/model/types";
 import type { ShowWarning } from "metabase/visualizations/echarts/types";
 import { getNumberOr } from "metabase/visualizations/lib/settings/row-values";
@@ -148,23 +149,26 @@ export function getPieRows(
 ) {
   const [
     {
-      data: { cols, rows: dataRows },
+      data: { rows: dataRows },
     },
   ] = rawSeries;
 
+  if (!settings["pie.metric"] || !settings["pie.dimension"]) {
+    return [];
+  }
+
+  const hasSortDimensionChanged =
+    settings["pie.sort_rows_dimension"] != null &&
+    settings["pie.sort_rows_dimension"] !== settings["pie.dimension"];
+
+  const { metricDesc, dimensionDesc } = getPieColumns(rawSeries, settings);
+
   const getColumnSettings = settings["column"];
   if (!getColumnSettings) {
     throw Error("`settings.column` is undefined");
   }
 
-  const dimensionCol = cols.find(c => c.name === settings["pie.dimension"]);
-  if (dimensionCol == null) {
-    throw Error(
-      `Could not find column based on "pie.dimension setting with value ${settings["pie.dimension"]}`,
-    );
-  }
-
-  const dimensionColSettings = getColumnSettings(dimensionCol);
+  const dimensionColSettings = getColumnSettings(dimensionDesc.column);
 
   const formatDimensionValue = (value: RowValue) => {
     if (value == null) {
@@ -174,13 +178,6 @@ export function getPieRows(
     return formatter(value, dimensionColSettings);
   };
 
-  const dimensionIndex = cols.findIndex(
-    col => col.name === settings["pie.dimension"],
-  );
-  const metricIndex = cols.findIndex(
-    col => col.name === settings["pie.metric"],
-  );
-
   let colors = getColors(rawSeries, settings);
   // `pie.colors` is a legacy setting used by old questions for their
   // colors. We'll still read it to preserve those color selections, but
@@ -189,36 +186,41 @@ export function getPieRows(
   if (settings["pie.colors"] != null) {
     colors = { ...colors, ...settings["pie.colors"] };
   }
-  const savedPieRows = settings["pie.rows"] ?? [];
-
-  const savedPieKeys = savedPieRows.map(pieRow => pieRow.key);
-
-  const keyToSavedPieRow = new Map<PieRow["key"], PieRow>();
-  savedPieRows.map(pieRow => keyToSavedPieRow.set(pieRow.key, pieRow));
 
   const currentDataRows = getAggregatedRows(
     dataRows,
-    dimensionIndex,
-    metricIndex,
+    dimensionDesc.index,
+    metricDesc.index,
+  );
+  const keyToCurrentDataRow = new Map<PieRow["key"], RowValues>(
+    currentDataRows.map(dataRow => [
+      getKeyFromDimensionValue(dataRow[dimensionDesc.index]),
+      dataRow,
+    ]),
   );
+  const currentDataKeys = Array.from(keyToCurrentDataRow.keys());
 
-  const keyToCurrentDataRow = new Map<PieRow["key"], RowValues>();
-  const currentDataKeys = currentDataRows.map(dataRow => {
-    const key = getKeyFromDimensionValue(dataRow[dimensionIndex]);
-    keyToCurrentDataRow.set(key, dataRow);
+  const savedPieRows = hasSortDimensionChanged
+    ? []
+    : (settings["pie.rows"] ?? []);
 
-    return key;
-  });
+  const savedPieKeys = savedPieRows.map(pieRow => pieRow.key);
 
+  const keyToSavedPieRow = new Map<PieRow["key"], PieRow>(
+    savedPieRows.map(pieRow => [pieRow.key, pieRow]),
+  );
   const removed = _.difference(savedPieKeys, currentDataKeys);
 
   let newPieRows: PieRow[] = [];
   // Case 1: Auto sorted, sort existing and new rows together
   if (settings["pie.sort_rows"]) {
-    const sortedCurrentDataRows = getSortedRows(currentDataRows, metricIndex);
+    const sortedCurrentDataRows = getSortedRows(
+      currentDataRows,
+      metricDesc.index,
+    );
 
     newPieRows = sortedCurrentDataRows.map(dataRow => {
-      const dimensionValue = dataRow[dimensionIndex];
+      const dimensionValue = dataRow[dimensionDesc.index];
       const key = getKeyFromDimensionValue(dimensionValue);
       // Historically we have used the dimension value in the `pie.colors`
       // setting instead of the key computed above. For compatibility with
@@ -278,11 +280,11 @@ export function getPieRows(
 
       return dataRow;
     });
-    const sortedAddedRows = getSortedRows(addedRows, metricIndex);
+    const sortedAddedRows = getSortedRows(addedRows, metricDesc.index);
 
     newPieRows.push(
       ...sortedAddedRows.map(addedDataRow => {
-        const dimensionValue = addedDataRow[dimensionIndex];
+        const dimensionValue = addedDataRow[dimensionDesc.index];
 
         const color = Color(colors[String(dimensionValue)]).hex();
         const key = getKeyFromDimensionValue(dimensionValue);
@@ -323,7 +325,7 @@ export function getPieRows(
     return (
       currTotal +
       checkNumber(
-        checkNotNull(keyToCurrentDataRow.get(pieRow.key))[metricIndex],
+        checkNotNull(keyToCurrentDataRow.get(pieRow.key))[metricDesc.index],
       )
     );
   }, 0);
@@ -335,7 +337,7 @@ export function getPieRows(
     }
 
     const metricValue = checkNumber(
-      checkNotNull(keyToCurrentDataRow.get(pieRow.key))[metricIndex],
+      checkNotNull(keyToCurrentDataRow.get(pieRow.key))[metricDesc.index],
     );
     const normalizedPercentage = metricValue / total;
 
@@ -359,4 +361,10 @@ export function getPieRows(
   return newPieRows;
 }
 
+export const getPieSortRowsDimensionSetting = (
+  settings: ComputedVisualizationSettings,
+) => {
+  return settings["pie.dimension"];
+};
+
 export const getDefaultSortRows = () => true;

frontend/src/metabase/visualizations/visualizations/PieChart/chart-definition.ts

@@ -20,6 +20,7 @@ import {
   getDefaultSliceThreshold,
   getDefaultSortRows,
   getPieRows,
+  getPieSortRowsDimensionSetting,
 } from "metabase/visualizations/shared/settings/pie";
 import { SERIES_SETTING_KEY } from "metabase/visualizations/shared/settings/series";
 import { getDefaultShowTotal } from "metabase/visualizations/shared/settings/waterfall";
@@ -107,6 +108,8 @@ export const PIE_CHART_DEFINITION: VisualizationDefinition = {
         onChangeSettings,
       ) => {
         return {
+          addButtonLabel: t`Add another row`,
+          searchPickerPlaceholder: t`Select a row`,
           onChangeSeriesColor: (sliceKey: string, color: string) => {
             const pieRows = vizSettings["pie.rows"];
             if (pieRows == null) {
@@ -126,6 +129,8 @@ export const PIE_CHART_DEFINITION: VisualizationDefinition = {
             onChangeSettings({
               "pie.sort_rows": false,
               "pie.rows": newPieRows,
+              "pie.sort_rows_dimension":
+                getPieSortRowsDimensionSetting(vizSettings),
             }),
         };
       },
@@ -136,6 +141,13 @@ export const PIE_CHART_DEFINITION: VisualizationDefinition = {
         "pie.sort_rows",
         "pie.slice_threshold",
       ],
+      writeDependencies: ["pie.sort_rows_dimension"],
+    },
+    "pie.sort_rows_dimension": {
+      getValue: (_series, settings) => getPieSortRowsDimensionSetting(settings),
+      // This read dependency is set so that "pie.sort_rows" is computed *before* this value, ensuring that
+      // that it uses the stored value if one exists. This is needed to check if the dimension has actually changed
+      readDependencies: ["pie.sort_rows", "pie.dimension"],
     },
     "pie.sort_rows": {
       hidden: true,