From 6ffd3c1b63ea08a961f4eb1640799a2acaba7937 Mon Sep 17 00:00:00 2001
From: Cal Herries <39073188+calherries@users.noreply.github.com>
Date: Tue, 17 Jan 2023 20:47:49 +0200
Subject: [PATCH] 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
---
.../src/metabase/dashboard/actions/save.js | 86 ++++++++-----------
frontend/src/metabase/services.js | 6 +-
.../collections/revision-history.cy.spec.js | 37 ++++----
3 files changed, 54 insertions(+), 75 deletions(-)
diff --git a/frontend/src/metabase/dashboard/actions/save.js b/frontend/src/metabase/dashboard/actions/save.js
index c73c6f3c591..98cd8775b17 100644
--- a/frontend/src/metabase/dashboard/actions/save.js
+++ b/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,
});
diff --git a/frontend/src/metabase/services.js b/frontend/src/metabase/services.js
index 7d20d3788f4..597f6958ebe 100644
--- a/frontend/src/metabase/services.js
+++ b/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"),
diff --git a/frontend/test/metabase/scenarios/collections/revision-history.cy.spec.js b/frontend/test/metabase/scenarios/collections/revision-history.cy.spec.js
index bd6bfc18ad0..7750242aec4 100644
--- a/frontend/test/metabase/scenarios/collections/revision-history.cy.spec.js
+++ b/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);
--
GitLab