Updating graph.metrics when column is removed (#29189) (#29302)

* Updating graph.metrics when column is removed

* updating logic and adding unit tests

Co-authored-by: Nick Fitzpatrick <nick@metabase.com>

e2e/test/scenarios/visualizations/reproductions/20548-bar-duplicate-y-axis-after-changing-metrics.cy.spec.js

@@ -18,7 +18,7 @@ const questionDetails = {
   },
 };
 
-describe.skip("issue 20548", () => {
+describe("issue 20548", () => {
   beforeEach(() => {
     cy.intercept("POST", "/api/dataset").as("dataset");
 
@@ -42,7 +42,7 @@ describe.skip("issue 20548", () => {
 
     cy.findByTestId("viz-settings-button").click();
     // Implicit assertion - it would fail if it finds more than one "Count" in the sidebar
-    sidebar().findByDisplayValue("Count");
+    sidebar().findAllByText("Count").should("have.length", 1);
   });
 });
 

frontend/src/metabase-lib/Question.ts

@@ -697,19 +697,31 @@ class QuestionInner {
     );
 
     const graphMetrics = this.setting("graph.metrics");
+
     if (
       graphMetrics &&
-      addedColumnNames.length > 0 &&
-      removedColumnNames.length === 0
+      (addedColumnNames.length > 0 || removedColumnNames.length > 0)
     ) {
       const addedMetricColumnNames = addedColumnNames.filter(
         name =>
           query.columnDimensionWithName(name) instanceof AggregationDimension,
       );
 
-      if (addedMetricColumnNames.length > 0) {
+      const removedMetricColumnNames = removedColumnNames.filter(
+        name =>
+          previousQuery.columnDimensionWithName(name) instanceof
+          AggregationDimension,
+      );
+
+      if (
+        addedMetricColumnNames.length > 0 ||
+        removedMetricColumnNames.length > 0
+      ) {
         return this.updateSettings({
-          "graph.metrics": [...graphMetrics, ...addedMetricColumnNames],
+          "graph.metrics": [
+            ..._.difference(graphMetrics, removedMetricColumnNames),
+            ...addedMetricColumnNames,
+          ],
         });
       }
     }

frontend/test/metabase/lib/dataset.unit.spec.js

@@ -16,6 +16,72 @@ describe("metabase/util/dataset", () => {
     });
   });
 
+  describe("syncColumnsAndSettings", () => {
+    it("should automatically add new metrics when a new aggregrate column is added", () => {
+      const prevQuestion = PRODUCTS.query({
+        aggregation: [["count"]],
+        breakout: [PRODUCTS.CATEGORY.dimension().mbql()],
+      })
+        .question()
+        .setSettings({
+          "graph.metrics": ["count"],
+        });
+
+      const newQuestion = prevQuestion
+        .query()
+        .aggregate(["sum", PRODUCTS.PRICE.dimension().mbql()])
+        .question()
+        .syncColumnsAndSettings(prevQuestion);
+
+      expect(newQuestion.setting("graph.metrics")).toMatchObject([
+        "count",
+        "sum",
+      ]);
+    });
+
+    it("should automatically remove metrics from settings when an aggregrate column is removed", () => {
+      const prevQuestion = PRODUCTS.query({
+        aggregation: [["sum", PRODUCTS.PRICE.dimension().mbql()], ["count"]],
+        breakout: [PRODUCTS.CATEGORY.dimension().mbql()],
+      })
+        .question()
+        .setSettings({
+          "graph.metrics": ["count", "sum"],
+        });
+
+      const newQuestion = prevQuestion
+        .query()
+        .removeAggregation(1)
+        .question()
+        .syncColumnsAndSettings(prevQuestion);
+
+      expect(newQuestion.setting("graph.metrics")).toMatchObject(["sum"]);
+    });
+
+    it("Adding a breakout should not affect graph.metrics", () => {
+      const prevQuestion = PRODUCTS.query({
+        aggregation: [["sum", PRODUCTS.PRICE.dimension().mbql()], ["count"]],
+        breakout: [PRODUCTS.CATEGORY.dimension().mbql()],
+      })
+        .question()
+        .setSettings({
+          "graph.metrics": ["count", "sum"],
+        });
+
+      const newQuestion = prevQuestion
+        .query()
+        .breakout(PRODUCTS.VENDOR.dimension().mbql())
+        .question()
+        .syncColumnsAndSettings(prevQuestion);
+
+      expect(newQuestion.setting("graph.metrics")).toMatchObject([
+        "count",
+        "sum",
+      ]);
+      expect(newQuestion.query().columns()).toHaveLength(4);
+    });
+  });
+
   describe("syncTableColumnsToQuery", () => {
     it("should not modify `fields` if no `table.columns` setting preset", () => {
       const question = syncTableColumnsToQuery(