Fix for #6884: Update dashboard save to create only one revision when you add a card (#27708)

* Update dashboard save to save all dashcard data on create, not update

* Remove repro e2e test

* Add e2e test

* Add series to POST request, missing by mistake

* Formatting

* Rename DashboardApi methods for consistent names and cases

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

@@ -54,28 +54,38 @@ export const saveDashboardAndCards = createThunkAction(
         dashboard.ordered_cards
           .filter(dc => dc.isRemoved && !dc.isAdded)
           .map(dc =>
-            DashboardApi.removecard({
+            DashboardApi.removeCard({
               dashId: dashboard.id,
               dashcardId: dc.id,
             }),
           ),
       );
 
-      // add isAdded dashboards
+      // update parameter mappings
+      dashboard.ordered_cards = dashboard.ordered_cards.map(dc => ({
+        ...dc,
+        parameter_mappings: dc.parameter_mappings.filter(
+          mapping =>
+            // filter out mappings for deleted parameters
+            _.findWhere(dashboard.parameters, {
+              id: mapping.parameter_id,
+            }) &&
+            // filter out mappings for deleted series
+            (!dc.card_id ||
+              dc.card_id === mapping.card_id ||
+              _.findWhere(dc.series, { id: mapping.card_id })),
+        ),
+      }));
+
+      // add new cards to dashboard
       const updatedDashcards = await Promise.all(
         dashboard.ordered_cards
           .filter(dc => !dc.isRemoved)
           .map(async dc => {
             if (dc.isAdded) {
-              const result = await DashboardApi.addcard({
+              const result = await DashboardApi.addCard({
                 dashId: dashboard.id,
                 cardId: dc.card_id,
-              });
-              dispatch(updateDashcardId(dc.id, result.id));
-
-              // mark isAdded because addcard doesn't record the position
-              return {
-                ...result,
                 col: dc.col,
                 row: dc.row,
                 size_x: dc.size_x,
@@ -83,8 +93,9 @@ export const saveDashboardAndCards = createThunkAction(
                 series: dc.series,
                 parameter_mappings: dc.parameter_mappings,
                 visualization_settings: dc.visualization_settings,
-                isAdded: true,
-              };
+              });
+              dispatch(updateDashcardId(dc.id, result.id));
+              return result;
             } else {
               return dc;
             }
@@ -106,45 +117,20 @@ export const saveDashboardAndCards = createThunkAction(
         );
       }
 
-      // reposition the cards
-      if (_.some(updatedDashcards, dc => dc.isDirty || dc.isAdded)) {
-        const cards = updatedDashcards.map(
-          ({
-            id,
-            card_id,
-            row,
-            col,
-            size_x,
-            size_y,
-            series,
-            parameter_mappings,
-            visualization_settings,
-          }) => ({
-            id,
-            card_id,
-            row,
-            col,
-            size_x,
-            size_y,
-            series,
-            visualization_settings,
-            parameter_mappings:
-              parameter_mappings &&
-              parameter_mappings.filter(
-                mapping =>
-                  // filter out mappings for deleted parameters
-                  _.findWhere(dashboard.parameters, {
-                    id: mapping.parameter_id,
-                  }) &&
-                  // filter out mappings for deleted series
-                  (!card_id ||
-                    card_id === mapping.card_id ||
-                    _.findWhere(series, { id: mapping.card_id })),
-              ),
-          }),
-        );
-
-        const result = await DashboardApi.reposition_cards({
+      // update the dashboard cards
+      if (_.some(updatedDashcards, dc => dc.isDirty)) {
+        const cards = updatedDashcards.map(dc => ({
+          id: dc.id,
+          card_id: dc.card_id,
+          row: dc.row,
+          col: dc.col,
+          size_x: dc.size_x,
+          size_y: dc.size_y,
+          series: dc.series,
+          visualization_settings: dc.visualization_settings,
+          parameter_mappings: dc.parameter_mappings,
+        }));
+        const result = await DashboardApi.updateCards({
           dashId: dashboard.id,
           cards,
         });

frontend/src/metabase/services.js

@@ -119,9 +119,9 @@ export const DashboardApi = {
   get: GET("/api/dashboard/:dashId"),
   update: PUT("/api/dashboard/:id"),
   delete: DELETE("/api/dashboard/:dashId"),
-  addcard: POST("/api/dashboard/:dashId/cards"),
-  removecard: DELETE("/api/dashboard/:dashId/cards"),
-  reposition_cards: PUT("/api/dashboard/:dashId/cards"),
+  addCard: POST("/api/dashboard/:dashId/cards"),
+  removeCard: DELETE("/api/dashboard/:dashId/cards"),
+  updateCards: PUT("/api/dashboard/:dashId/cards"),
   favorite: POST("/api/dashboard/:dashId/favorite"),
   unfavorite: DELETE("/api/dashboard/:dashId/favorite"),
   parameterValues: GET("/api/dashboard/:dashId/params/:paramId/values"),

frontend/test/metabase/scenarios/collections/revision-history.cy.spec.js

@@ -42,27 +42,6 @@ describe("revision history", () => {
 
       cy.findAllByText("Revert").should("not.exist");
     });
-
-    it.skip("dashboard should update properly on revert (metabase#6884)", () => {
-      visitAndEditDashboard(1);
-      // Add another question without changing its size or moving it afterwards
-      cy.icon("add").last().click();
-      cy.findByText("Orders, Count").click();
-      saveDashboard();
-      // Revert the card to the state when the second card was added
-      cy.icon("ellipsis").click();
-      cy.findByText("Revision history").click();
-      clickRevert("added a card.", 0); // the top-most string or the latest card addition
-      cy.wait("@revert");
-      cy.request("GET", "/api/dashboard/1").then(xhr => {
-        const SECOND_CARD = xhr.body.ordered_cards[1];
-        const { col, size_x, size_y } = SECOND_CARD;
-        // The second card shrunk its size and changed the position completely to the left covering the first one
-        expect(col).not.to.eq(0);
-        expect(size_x).to.eq(4);
-        expect(size_y).to.eq(4);
-      });
-    });
   });
 
   Object.entries(PERMISSIONS).forEach(([permission, userGroup]) => {
@@ -84,6 +63,20 @@ describe("revision history", () => {
               }
             });
 
+            it("shouldn't create a rearrange revision when adding a card (metabase#6884)", () => {
+              cy.createDashboard().then(({ body }) => {
+                visitAndEditDashboard(body.id);
+              });
+              cy.icon("add").last().click();
+              cy.findByText("Orders, Count").click();
+              saveDashboard();
+              openRevisionHistory();
+              cy.findByText(/added a card/)
+                .siblings("button")
+                .should("not.exist");
+              cy.findByText(/rearranged the cards/).should("not.exist");
+            });
+
             it("should be able to revert a dashboard (metabase#15237)", () => {
               visitDashboard(1);
               openRevisionHistory();
@@ -99,7 +92,7 @@ describe("revision history", () => {
 
               // Should be able to revert back again
               cy.findByText("History");
-              clickRevert(/rearranged the cards/);
+              clickRevert(/added a card/);
 
               cy.wait("@revert").then(({ response: { statusCode, body } }) => {
                 expect(statusCode).to.eq(200);