Only save changed dashboards (#29101)

* only save changed dashboards

* test early return from save action

* remove problematic api wait for dashboard save

* deeply compare cards and add better tests

* update failing e2e test

e2e/support/helpers/e2e-dashboard-helpers.js

@@ -20,9 +20,7 @@ export function editDashboard() {
 }
 
 export function saveDashboard() {
-  cy.intercept("GET", "/api/dashboard/*").as("getDashboard");
   cy.findByText("Save").click();
-  cy.wait("@getDashboard");
   cy.findByText("You're editing this dashboard.").should("not.exist");
   cy.wait(1); // this is stupid but necessary to due to the dashboard resizing and detaching elements
 }

e2e/test/scenarios/dashboard-filters/reproductions/22788-flter-cc-dropped-on-second-edit.cy.spec.js

@@ -4,6 +4,7 @@ import {
   filterWidget,
   editDashboard,
   saveDashboard,
+  sidebar,
 } from "e2e/support/helpers";
 import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
 
@@ -80,6 +81,12 @@ describe("issue 22788", () => {
         cy.findByText(ccDisplayName);
       });
 
+    // need to actually change the dashboard to test a real save
+    sidebar().within(() => {
+      cy.findByDisplayValue("Text").clear().type('my filter text');
+      cy.button('Done').click();
+    });
+
     saveDashboard();
 
     addFilterAndAssert();

frontend/src/metabase/dashboard/actions/actions.unit.spec.js

@@ -12,6 +12,7 @@ import {
   openAddQuestionSidebar,
   removeParameter,
   SET_DASHBOARD_ATTRIBUTES,
+  saveDashboardAndCards,
 } from "./index";
 
 DashboardApi.parameterSearch = jest.fn();
@@ -151,4 +152,40 @@ describe("dashboard actions", () => {
       });
     });
   });
+
+  describe("saveDashboardAndCards", () => {
+    it("should not save anything if the dashboard has not changed", async () => {
+      const dashboard = {
+        id: 1,
+        name: "Foo",
+        parameters: [],
+        ordered_cards: [
+          { id: 1, name: "Foo", card_id: 1 },
+          { id: 2, name: "Bar", card_id: 2 },
+        ],
+      };
+
+      const getState = () => ({
+        dashboard: {
+          isEditing: dashboard,
+          dashboardId: 1,
+          dashboards: {
+            1: {
+              ...dashboard,
+              ordered_cards: [1, 2],
+            },
+          },
+          dashcards: {
+            1: dashboard.ordered_cards[0],
+            2: dashboard.ordered_cards[1],
+          },
+        },
+      });
+
+      await saveDashboardAndCards()(dispatch, getState);
+
+      // if this is called only once, it means that the dashboard was not saved
+      expect(dispatch).toHaveBeenCalledTimes(1);
+    });
+  });
 });

frontend/src/metabase/dashboard/actions/save.js

@@ -12,6 +12,7 @@ import { getDashboardBeforeEditing } from "../selectors";
 
 import { updateDashcardId } from "./core";
 import { fetchDashboard } from "./data-fetching";
+import { hasDashboardChanged, haveDashboardCardsChanged } from "./utils";
 
 export const SAVE_DASHBOARD_AND_CARDS =
   "metabase/dashboard/SAVE_DASHBOARD_AND_CARDS";
@@ -29,11 +30,28 @@ export const saveDashboardAndCards = createThunkAction(
         ),
       };
 
+      const dashboardBeforeEditing = getDashboardBeforeEditing(state);
+
+      if (dashboardBeforeEditing) {
+        const dashboardHasChanged = hasDashboardChanged(
+          dashboard,
+          dashboardBeforeEditing,
+        );
+
+        const cardsHaveChanged = haveDashboardCardsChanged(
+          dashboard.ordered_cards,
+          dashboardBeforeEditing.ordered_cards,
+        );
+
+        if (!cardsHaveChanged && !dashboardHasChanged) {
+          return;
+        }
+      }
+
       // clean invalid dashcards
       // We currently only do this for dashcard click behavior.
       // Invalid (partially complete) states are fine during editing,
       // but we should restore the previous value if saved while invalid.
-      const dashboardBeforeEditing = getDashboardBeforeEditing(state);
       const clickBehaviorPath = ["visualization_settings", "click_behavior"];
       dashboard.ordered_cards = dashboard.ordered_cards.map((card, index) => {
         if (!clickBehaviorIsValid(getIn(card, clickBehaviorPath))) {

frontend/src/metabase/dashboard/actions/utils.ts

+import _ from "underscore";
+
+import type { Dashboard, DashboardOrderedCard } from "metabase-types/api";
+
+export function hasDashboardChanged(
+  dashboard: Dashboard,
+  dashboardBeforeEditing: Dashboard,
+) {
+  return !_.isEqual(
+    { ...dashboard, ordered_cards: dashboard.ordered_cards.length },
+    {
+      ...dashboardBeforeEditing,
+      ordered_cards: dashboardBeforeEditing.ordered_cards.length,
+    },
+  );
+}
+
+// sometimes the cards objects change order but all the cards themselves are the same
+// this should not trigger a save
+export function haveDashboardCardsChanged(
+  newCards: DashboardOrderedCard[],
+  oldCards: DashboardOrderedCard[],
+) {
+  return (
+    !newCards.every(newCard =>
+      oldCards.some(oldCard => _.isEqual(oldCard, newCard)),
+    ) ||
+    !oldCards.every(oldCard =>
+      newCards.some(newCard => _.isEqual(oldCard, newCard)),
+    )
+  );
+}

frontend/src/metabase/dashboard/actions/utils.unit.spec.ts

+import {
+  createMockDashboard,
+  createMockDashboardOrderedCard,
+} from "metabase-types/api/mocks";
+import { hasDashboardChanged, haveDashboardCardsChanged } from "./utils";
+
+describe("dashboard > actions > utils", () => {
+  describe("hasDashboardChanged", () => {
+    it("should return false if the dashboard has not changed", () => {
+      const oldDash = createMockDashboard({
+        id: 1,
+        name: "old name",
+        parameters: [
+          { id: "1", name: "old name", type: "type/Text", slug: "my_param" },
+        ],
+      });
+      const newDash = createMockDashboard({
+        id: 1,
+        name: "old name",
+        parameters: [
+          { id: "1", name: "old name", type: "type/Text", slug: "my_param" },
+        ],
+      });
+
+      expect(hasDashboardChanged(newDash, oldDash)).toBe(false);
+    });
+
+    it("should return true if the dashboard has changed", () => {
+      const oldDash = createMockDashboard({ id: 1, name: "old name" });
+      const newDash = createMockDashboard({ id: 1, name: "new name" });
+
+      expect(hasDashboardChanged(newDash, oldDash)).toBe(true);
+    });
+
+    it("should return true if the deeply nested properties change", () => {
+      const oldDash = createMockDashboard({
+        id: 1,
+        name: "old name",
+        parameters: [
+          { id: "1", name: "old name", type: "type/Text", slug: "my_param" },
+        ],
+      });
+      const newDash = createMockDashboard({
+        id: 1,
+        name: "old name",
+        parameters: [
+          { id: "1", name: "new name", type: "type/Text", slug: "my_param" },
+        ],
+      });
+
+      expect(hasDashboardChanged(newDash, oldDash)).toBe(true);
+    });
+
+    it("should return true if the number of cards has changed", () => {
+      const oldDash = createMockDashboard({
+        id: 1,
+        name: "old name",
+        ordered_cards: [createMockDashboardOrderedCard()],
+      });
+      const newDash = createMockDashboard({ id: 1, name: "old name" });
+
+      expect(hasDashboardChanged(newDash, oldDash)).toBe(true);
+    });
+
+    it("should ignore card changes", () => {
+      const oldDash = createMockDashboard({
+        id: 1,
+        name: "old name",
+        ordered_cards: [createMockDashboardOrderedCard({ id: 1 })],
+      });
+      const newDash = createMockDashboard({
+        id: 1,
+        name: "old name",
+        ordered_cards: [createMockDashboardOrderedCard({ id: 2 })],
+      });
+
+      expect(hasDashboardChanged(newDash, oldDash)).toBe(false);
+    });
+  });
+
+  describe("haveDashboardCardsChanged", () => {
+    it("should return true if a card changed", () => {
+      const oldCards = [
+        createMockDashboardOrderedCard({ id: 1 }),
+        createMockDashboardOrderedCard({ id: 2 }),
+      ];
+      const newCards = [
+        createMockDashboardOrderedCard({ id: 2 }),
+        createMockDashboardOrderedCard({ id: 3 }),
+      ];
+
+      expect(haveDashboardCardsChanged(newCards, oldCards)).toBe(true);
+    });
+
+    it("should return true if a card was added", () => {
+      const oldCards = [
+        createMockDashboardOrderedCard({ id: 1 }),
+        createMockDashboardOrderedCard({ id: 2 }),
+      ];
+      const newCards = [
+        createMockDashboardOrderedCard({ id: 1 }),
+        createMockDashboardOrderedCard({ id: 2 }),
+        createMockDashboardOrderedCard({ id: 3 }),
+      ];
+
+      expect(haveDashboardCardsChanged(newCards, oldCards)).toBe(true);
+    });
+
+    it("should return true if a card was added and deleted", () => {
+      const oldCards = [
+        createMockDashboardOrderedCard({ id: 1 }),
+        createMockDashboardOrderedCard({ id: 2 }),
+        createMockDashboardOrderedCard({ id: 3 }),
+      ];
+      const newCards = [
+        createMockDashboardOrderedCard({ id: 1 }),
+        createMockDashboardOrderedCard({ id: 2 }),
+        createMockDashboardOrderedCard({ id: 4 }),
+      ];
+
+      expect(haveDashboardCardsChanged(newCards, oldCards)).toBe(true);
+    });
+
+    it("should return true if a card was removed", () => {
+      const oldCards = [
+        createMockDashboardOrderedCard({ id: 1 }),
+        createMockDashboardOrderedCard({ id: 2 }),
+      ];
+      const newCards = [createMockDashboardOrderedCard({ id: 1 })];
+
+      expect(haveDashboardCardsChanged(newCards, oldCards)).toBe(true);
+    });
+
+    it("should return true if deeply nested properties change", () => {
+      const oldCards = [
+        createMockDashboardOrderedCard({
+          id: 1,
+          visualization_settings: { foo: { bar: { baz: 21 } } },
+        }),
+        createMockDashboardOrderedCard({ id: 2 }),
+      ];
+      const newCards = [
+        createMockDashboardOrderedCard({
+          id: 1,
+          visualization_settings: { foo: { bar: { baz: 22 } } },
+        }),
+        createMockDashboardOrderedCard({ id: 2 }),
+      ];
+
+      expect(haveDashboardCardsChanged(newCards, oldCards)).toBe(true);
+    });
+
+    it("should return false if the dashboard cards have not changed", () => {
+      const oldCards = [
+        createMockDashboardOrderedCard({ id: 1 }),
+        createMockDashboardOrderedCard({ id: 2 }),
+      ];
+      const newCards = [
+        createMockDashboardOrderedCard({ id: 1 }),
+        createMockDashboardOrderedCard({ id: 2 }),
+      ];
+
+      expect(haveDashboardCardsChanged(newCards, oldCards)).toBe(false);
+    });
+
+    it("should return false if the dashboard cards have only changed order", () => {
+      const oldCards = [
+        createMockDashboardOrderedCard({ id: 1 }),
+        createMockDashboardOrderedCard({ id: 2 }),
+      ];
+      const newCards = [
+        createMockDashboardOrderedCard({ id: 2 }),
+        createMockDashboardOrderedCard({ id: 1 }),
+      ];
+
+      expect(haveDashboardCardsChanged(newCards, oldCards)).toBe(false);
+    });
+
+    it("should perform reasonably well for 1000 cards", () => {
+      const oldCards = Array(1000)
+        .fill("")
+        .map(index =>
+          createMockDashboardOrderedCard({
+            id: index,
+            visualization_settings: { foo: { bar: { baz: index * 10 } } },
+          }),
+        );
+      const newCards = Array(1000)
+        .fill("")
+        .map(index =>
+          createMockDashboardOrderedCard({
+            id: index,
+            visualization_settings: { foo: { bar: { baz: index * 10 } } },
+          }),
+        );
+
+      const startTime = performance.now();
+      expect(haveDashboardCardsChanged(newCards, oldCards)).toBe(false);
+      const endTime = performance.now();
+
+      expect(endTime - startTime).toBeLessThan(100); // 100 ms (locally this was 6 ms)
+    });
+  });
+});