fixes x-ray loading order (#33145)

e2e/test/scenarios/dashboard/dashboard.cy.spec.js

@@ -20,6 +20,8 @@ import {
   enableTracking,
   expectGoodSnowplowEvent,
   closeNavigationSidebar,
+  updateDashboardCards,
+  getDashboardCards,
 } from "e2e/support/helpers";
 
 import { SAMPLE_DB_ID } from "e2e/support/cypress_data";
@@ -791,6 +793,23 @@ describe("scenarios > dashboard", () => {
 
     cy.icon("pencil").should("be.visible");
   });
+
+  it("shows sorted cards on mobile screens", () => {
+    cy.viewport(400, 800);
+    cy.createDashboard().then(({ body: { id: dashboard_id } }) => {
+      const cards = [
+        createTextCard("bottom", 1), // the bottom card intentionally goes first to have unsorted cards coming from the BE
+        createTextCard("top", 0),
+      ];
+
+      updateDashboardCards({ dashboard_id, cards });
+
+      visitDashboard(dashboard_id);
+    });
+
+    getDashboardCards().eq(0).contains("top");
+    getDashboardCards().eq(1).contains("bottom");
+  });
 });
 
 describeWithSnowplow("scenarios > dashboard", () => {
@@ -898,3 +917,21 @@ function createDashboardUsingUI(name, description) {
     cy.url().should("contain", `/dashboard/${body.id}`);
   });
 }
+
+function createTextCard(text, row) {
+  return {
+    row,
+    size_x: 24,
+    size_y: 1,
+    visualization_settings: {
+      virtual_card: {
+        name: null,
+        display: "text",
+        visualization_settings: {},
+        dataset_query: {},
+        archived: false,
+      },
+      text,
+    },
+  };
+}

e2e/test/scenarios/dashboard/x-rays.cy.spec.js

@@ -9,6 +9,7 @@ import {
   addOrUpdateDashboardCard,
   visitDashboardAndCreateTab,
   popover,
+  getDashboardCards,
 } from "e2e/support/helpers";
 
 import { SAMPLE_DB_ID } from "e2e/support/cypress_data";
@@ -189,6 +190,33 @@ describe("scenarios > x-rays", () => {
     cy.findByText("How these transactions are distributed");
   });
 
+  it("should start loading cards from top to bottom", () => {
+    // to check the order of loaded cards this test lets the first intercepted
+    // request to be resolved successfully and then it fails all others
+
+    const totalRequests = 8;
+    const successfullyLoadedCards = 1;
+    const failedCards = totalRequests - successfullyLoadedCards;
+
+    cy.intercept({
+      method: "POST",
+      url: "/api/dataset",
+      times: successfullyLoadedCards,
+    }).as("dataset");
+
+    cy.intercept(
+      { method: "POST", url: "/api/dataset", times: failedCards },
+      { statusCode: 500 },
+    ).as("datasetFailed");
+
+    cy.visit(`/auto/dashboard/table/${ORDERS_ID}`);
+
+    cy.wait("@dataset");
+    cy.wait("@datasetFailed");
+
+    getDashboardCards().eq(1).contains("Total transactions");
+  });
+
   it("should be able to click the title of an x-ray dashcard to see it in the query builder (metabase#19405)", () => {
     const timeout = { timeout: 10000 };
 

frontend/src/metabase/dashboard/components/grid/utils.js

@@ -5,24 +5,13 @@ function sumVerticalSpace(layout) {
   return layout.reduce((sum, current) => sum + current.h, 0);
 }
 
-export function sortCardsForMobile(a, b) {
-  const yDiff = a.y - b.y;
-
-  // sort by y position first
-  if (yDiff !== 0) {
-    return yDiff;
-  }
-
-  // for items on the same row, sort by x position
-  return a.x - b.x;
-}
 export function generateMobileLayout({
   desktopLayout,
   defaultCardHeight,
   heightByDisplayType = {},
 }) {
   const mobile = [];
-  desktopLayout.sort(sortCardsForMobile).forEach(item => {
+  desktopLayout.forEach(item => {
     const card = item.dashcard.card;
     const height = heightByDisplayType[card.display] || defaultCardHeight;
     mobile.push({

frontend/src/metabase/dashboard/components/grid/utils.unit.spec.js

-import { sortCardsForMobile } from "./utils";
-
-describe("Dashcard > grid > utils", () => {
-  describe("sortCardsForMobile", () => {
-    it("should sort cards by y position first", () => {
-      const a = { id: "top", y: 1, x: 3 };
-      const b = { id: "middle", y: 2, x: 2 };
-      const c = { id: "bottom", y: 3, x: 1 };
-
-      const result = [b, a, c].sort(sortCardsForMobile);
-      expect(result).toEqual([a, b, c]);
-    });
-
-    it("should sort cards by x position if y position is the same", () => {
-      const a = { id: "left", y: 5, x: 1 };
-      const b = { id: "middle", y: 5, x: 2 };
-      const c = { id: "right", y: 5, x: 3 };
-
-      const result = [c, a, b].sort(sortCardsForMobile);
-      expect(result).toEqual([a, b, c]);
-    });
-
-    it("should sort cards by x and y positions", () => {
-      const a = { id: "top", y: 1, x: 3 };
-      const b = { id: "middle", y: 2, x: 2 };
-      const c = { id: "bottom", y: 3, x: 1 };
-
-      const d = { id: "left", y: 5, x: 1 };
-      const e = { id: "middle", y: 5, x: 2 };
-      const f = { id: "right", y: 5, x: 3 };
-
-      const result = [f, d, c, a, b, e].sort(sortCardsForMobile);
-      expect(result).toEqual([a, b, c, d, e, f]);
-    });
-  });
-});

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

@@ -7,6 +7,7 @@ import {
   getEditingParameterId,
   getIsEditingParameter,
   getClickBehaviorSidebarDashcard,
+  getDashboardComplete,
 } from "metabase/dashboard/selectors";
 import { createMockSettingsState } from "metabase-types/store/mocks";
 import Field from "metabase-lib/metadata/Field";
@@ -340,4 +341,65 @@ describe("dashboard/selectors", () => {
       );
     });
   });
+
+  describe("getDashboardComplete", () => {
+    const multiCardState = chain(STATE)
+      .assocIn(["dashboard", "dashboards", 0, "ordered_cards"], [0, 1, 2])
+      .assocIn(["dashboard", "dashcards", 2], {
+        card: { id: 2, dataset_query: { type: "query", query: {} } },
+        parameter_mappings: [],
+      })
+      .value();
+
+    const setup = positions => {
+      const newStateChain = chain(multiCardState);
+
+      positions.forEach((position, index) => {
+        newStateChain
+          .assocIn(["dashboard", "dashcards", index, "row"], position.row)
+          .assocIn(["dashboard", "dashcards", index, "col"], position.col);
+      });
+
+      return newStateChain.value();
+    };
+
+    it("should filter out removed dashcards", () => {
+      const state = chain(multiCardState)
+        .assocIn(["dashboard", "dashcards", 0, "isRemoved"], true)
+        .assocIn(["dashboard", "dashcards", 2, "isRemoved"], true)
+        .value();
+
+      expect(getDashboardComplete(state).ordered_cards).toEqual([
+        multiCardState.dashboard.dashcards[1],
+      ]);
+    });
+
+    it("should sort cards based on their positions top to bottom", () => {
+      const state = setup([
+        { row: 2, col: 0 },
+        { row: 1, col: 1 },
+        { row: 0, col: 2 },
+      ]);
+
+      const cards = getDashboardComplete(state).ordered_cards;
+
+      expect(cards[2].card.id).toBe(0);
+      expect(cards[1].card.id).toBe(1);
+      expect(cards[0].card.id).toBe(2);
+    });
+
+    it("should sort cards based on their positions left to right when on the same row", () => {
+      const state = setup([
+        { row: 0, col: 2 },
+        { row: 0, col: 1 },
+        { row: 0, col: 0 },
+      ]);
+
+      const cards = getDashboardComplete(state).ordered_cards;
+
+      expect(cards[2].card.id).toBe(0);
+      expect(cards[1].card.id).toBe(1);
+      expect(cards[0].card.id).toBe(2);
+    });
+  });
 });

frontend/src/metabase/dashboard/selectors/selectors.js

@@ -115,13 +115,33 @@ export const getDashCardTable = (state, dashcardId) => {
 
 export const getDashboardComplete = createSelector(
   [getDashboard, getDashcards],
-  (dashboard, dashcards) =>
-    dashboard && {
-      ...dashboard,
-      ordered_cards: dashboard.ordered_cards
-        .map(id => dashcards[id])
-        .filter(dc => !dc.isRemoved),
-    },
+  (dashboard, dashcards) => {
+    if (!dashboard) {
+      return null;
+    }
+
+    const ordered_cards = dashboard.ordered_cards
+      .map(id => dashcards[id])
+      .filter(dc => !dc.isRemoved)
+      .sort((a, b) => {
+        const rowDiff = a.row - b.row;
+
+        // sort by y position first
+        if (rowDiff !== 0) {
+          return rowDiff;
+        }
+
+        // for items on the same row, sort by x position
+        return a.col - b.col;
+      });
+
+    return (
+      dashboard && {
+        ...dashboard,
+        ordered_cards,
+      }
+    );
+  },
 );
 
 export const getAutoApplyFiltersToastId = state =>