From 58c7b0ffa56c53f153a1fc4ecc896f60fb899a4e Mon Sep 17 00:00:00 2001
From: Emmad Usmani <emmadusmani@berkeley.edu>
Date: Mon, 26 Jun 2023 15:17:42 -0700
Subject: [PATCH] optimize loading questions only for active tab (#31578)
* only load cards and metadata for current dashboard tab
* add e2e test
* refactor
* more refactoring
* fix filters not working
* update e2e test
* fix bug with x-rays
* add dashboard_tab_id to automagic-dashboard response
* use schema check instead
* fix on public dashboards
* remove FE x-ray fix now that BE is fixed
* add test for public dashboards
* fix duplicate request for public dashboards
* fix notification bug
* fix failing unit tests in `reducers.unit.spec.js`
---------
Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com>
---
e2e/support/helpers/e2e-misc-helpers.js | 8 ++-
e2e/test/scenarios/dashboard/tabs.cy.spec.js | 66 +++++++++++++++++++
.../dashboard/actions/data-fetching.js | 48 ++++++++++++--
.../components/Dashboard/Dashboard.jsx | 13 +++-
frontend/src/metabase/dashboard/reducers.js | 35 +++++-----
.../metabase/dashboard/reducers.unit.spec.js | 2 +-
.../public/containers/PublicDashboard.jsx | 10 ++-
.../automagic_dashboards/comparison.clj | 57 ++++++++--------
.../automagic_dashboards/populate.clj | 53 ++++++++-------
.../api/automagic_dashboards_test.clj | 19 +++++-
10 files changed, 234 insertions(+), 77 deletions(-)
diff --git a/e2e/support/helpers/e2e-misc-helpers.js b/e2e/support/helpers/e2e-misc-helpers.js
index ab4a3a0f74d..c67ed1f7d60 100644
--- a/e2e/support/helpers/e2e-misc-helpers.js
+++ b/e2e/support/helpers/e2e-misc-helpers.js
@@ -169,7 +169,13 @@ export function visitDashboard(dashboard_id, { params = {} } = {}) {
cy.intercept("GET", `/api/dashboard/${dashboard_id}`).as(dashboardAlias);
const canViewDashboard = hasAccess(status);
- const validQuestions = dashboardHasQuestions(ordered_cards);
+
+ let validQuestions = dashboardHasQuestions(ordered_cards);
+ if (params.tab != null) {
+ validQuestions = validQuestions.filter(
+ card => card.dashboard_tab_id === params.tab,
+ );
+ }
if (canViewDashboard && validQuestions) {
// If dashboard has valid questions (GUI or native),
diff --git a/e2e/test/scenarios/dashboard/tabs.cy.spec.js b/e2e/test/scenarios/dashboard/tabs.cy.spec.js
index a20d96f4b6b..fab38cf23d2 100644
--- a/e2e/test/scenarios/dashboard/tabs.cy.spec.js
+++ b/e2e/test/scenarios/dashboard/tabs.cy.spec.js
@@ -78,6 +78,72 @@ describe("scenarios > dashboard > tabs", () => {
cy.findByText("Our analytics").should("be.visible");
});
});
+
+ it("should only fetch cards on the current tab", () => {
+ visitDashboardAndCreateTab({ dashboardId: 1, save: false });
+
+ // Add card to second tab
+ cy.icon("pencil").click();
+ openQuestionsSidebar();
+ sidebar().within(() => {
+ cy.findByText("Orders, Count").click();
+ });
+ saveDashboard();
+
+ cy.intercept(
+ "POST",
+ `/api/dashboard/1/dashcard/1/card/1/query`,
+ cy.spy().as("firstTabQuery"),
+ );
+ cy.intercept(
+ "POST",
+ `/api/dashboard/1/dashcard/2/card/2/query`,
+ cy.spy().as("secondTabQuery"),
+ );
+
+ // Visit first tab and confirm only first card was queried
+ visitDashboard(1, { params: { tab: 1 } });
+ cy.get("@firstTabQuery").should("have.been.calledOnce");
+ cy.get("@secondTabQuery").should("not.have.been.called");
+
+ // Visit second tab and confirm only second card was queried
+ cy.findByRole("tab", { name: "Tab 2" }).click();
+ cy.get("@firstTabQuery").should("have.been.calledOnce");
+ cy.get("@secondTabQuery").should("have.been.calledOnce");
+
+ // Go back to first tab, expect no additional queries
+ cy.findByRole("tab", { name: "Tab 1" }).click();
+ cy.get("@firstTabQuery").should("have.been.calledOnce");
+ cy.get("@secondTabQuery").should("have.been.calledOnce");
+
+ // Go to public dashboard
+ cy.request("PUT", "/api/setting/enable-public-sharing", { value: true });
+ cy.request("POST", `/api/dashboard/1/public_link`).then(
+ ({ body: { uuid } }) => {
+ cy.intercept(
+ "GET",
+ `/api/public/dashboard/${uuid}/dashcard/1/card/1?parameters=%5B%5D`,
+ cy.spy().as("publicFirstTabQuery"),
+ );
+ cy.intercept(
+ "GET",
+ `/api/public/dashboard/${uuid}/dashcard/2/card/2?parameters=%5B%5D`,
+ cy.spy().as("publicSecondTabQuery"),
+ );
+
+ cy.visit(`public/dashboard/${uuid}`);
+ },
+ );
+
+ // Check first tab requests
+ cy.get("@publicFirstTabQuery").should("have.been.calledOnce");
+ cy.get("@publicSecondTabQuery").should("not.have.been.called");
+
+ // Visit second tab and confirm only second card was queried
+ cy.findByRole("tab", { name: "Tab 2" }).click();
+ cy.get("@publicFirstTabQuery").should("have.been.calledOnce");
+ cy.get("@publicSecondTabQuery").should("have.been.calledOnce");
+ });
});
describeWithSnowplow("scenarios > dashboard > tabs", () => {
diff --git a/frontend/src/metabase/dashboard/actions/data-fetching.js b/frontend/src/metabase/dashboard/actions/data-fetching.js
index d92b46d37e8..fefbd091287 100644
--- a/frontend/src/metabase/dashboard/actions/data-fetching.js
+++ b/frontend/src/metabase/dashboard/actions/data-fetching.js
@@ -34,6 +34,7 @@ import {
getCanShowAutoApplyFiltersToast,
getDashboardById,
getDashCardById,
+ getSelectedTabId,
} from "../selectors";
import {
@@ -60,6 +61,9 @@ export const FETCH_DASHBOARD_CARD_DATA =
export const CANCEL_FETCH_DASHBOARD_CARD_DATA =
"metabase/dashboard/CANCEL_FETCH_DASHBOARD_CARD_DATA";
+export const FETCH_DASHBOARD_CARD_METADATA =
+ "metabase/dashboard/FETCH_DASHBOARD_CARD_METADATA";
+
export const FETCH_CARD_DATA = "metabase/dashboard/FETCH_CARD_DATA";
export const CANCEL_FETCH_CARD_DATA =
"metabase/dashboard/CANCEL_FETCH_CARD_DATA";
@@ -197,7 +201,16 @@ export const fetchDashboard = createThunkAction(
}
if (dashboardType === "normal" || dashboardType === "transient") {
- await dispatch(loadMetadataForDashboard(result.ordered_cards));
+ const selectedTabId = getSelectedTabId(getState());
+
+ const cards =
+ selectedTabId === undefined
+ ? result.ordered_cards
+ : result.ordered_cards.filter(
+ c => c.dashboard_tab_id === selectedTabId,
+ );
+
+ await dispatch(loadMetadataForDashboard(cards));
}
const isUsingCachedResults = entities != null;
@@ -409,14 +422,23 @@ export const fetchDashboardCardData = createThunkAction(
FETCH_DASHBOARD_CARD_DATA,
options => (dispatch, getState) => {
const dashboard = getDashboardComplete(getState());
+ const selectedTabId = getSelectedTabId(getState());
+ const dashcardIds = [];
const promises = getAllDashboardCards(dashboard)
.map(({ card, dashcard }) => {
- if (!isVirtualDashCard(dashcard)) {
- return dispatch(fetchCardData(card, dashcard, options)).then(() => {
- return dispatch(updateLoadingTitle());
- });
+ if (
+ isVirtualDashCard(dashcard) ||
+ (selectedTabId !== undefined &&
+ dashcard.dashboard_tab_id !== selectedTabId)
+ ) {
+ return;
}
+
+ dashcardIds.push(dashcard.id);
+ return dispatch(fetchCardData(card, dashcard, options)).then(() => {
+ return dispatch(updateLoadingTitle());
+ });
})
.filter(p => !!p);
@@ -428,7 +450,21 @@ export const fetchDashboardCardData = createThunkAction(
dispatch(loadingComplete());
});
- return { currentTime: performance.now() };
+ return { currentTime: performance.now(), dashcardIds };
+ },
+);
+
+export const fetchDashboardCardMetadata = createThunkAction(
+ FETCH_DASHBOARD_CARD_METADATA,
+ () => async (dispatch, getState) => {
+ const allDashCards = getDashboardComplete(getState()).ordered_cards;
+ const selectedTabId = getSelectedTabId(getState());
+
+ const cards = allDashCards.filter(
+ dc =>
+ selectedTabId !== undefined && dc.dashboard_tab_id === selectedTabId,
+ );
+ await dispatch(loadMetadataForDashboard(cards));
},
);
diff --git a/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.jsx b/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.jsx
index 2de013a9daf..5e1bbd32ada 100644
--- a/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.jsx
+++ b/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.jsx
@@ -59,6 +59,7 @@ class Dashboard extends Component {
dashboard: PropTypes.object,
dashboardId: PropTypes.number,
+ dashcardData: PropTypes.object,
selectedTabId: PropTypes.number,
parameters: PropTypes.array,
parameterValues: PropTypes.object,
@@ -73,6 +74,7 @@ class Dashboard extends Component {
cancelFetchDashboardCardData: PropTypes.func.isRequired,
fetchDashboard: PropTypes.func.isRequired,
fetchDashboardCardData: PropTypes.func.isRequired,
+ fetchDashboardCardMetadata: PropTypes.func.isRequired,
initialize: PropTypes.func.isRequired,
onRefreshPeriodChange: PropTypes.func,
saveDashboardAndCards: PropTypes.func.isRequired,
@@ -140,7 +142,16 @@ class Dashboard extends Component {
if (prevProps.dashboardId !== this.props.dashboardId) {
await this.loadDashboard(this.props.dashboardId);
this.throttleParameterWidgetStickiness();
- } else if (
+ return;
+ }
+
+ if (!_.isEqual(prevProps.selectedTabId, this.props.selectedTabId)) {
+ this.props.fetchDashboardCardData();
+ this.props.fetchDashboardCardMetadata();
+ return;
+ }
+
+ if (
!_.isEqual(prevProps.parameterValues, this.props.parameterValues) ||
(!prevProps.dashboard && this.props.dashboard)
) {
diff --git a/frontend/src/metabase/dashboard/reducers.js b/frontend/src/metabase/dashboard/reducers.js
index 59ba8eeeecb..edebfb786ec 100644
--- a/frontend/src/metabase/dashboard/reducers.js
+++ b/frontend/src/metabase/dashboard/reducers.js
@@ -43,8 +43,9 @@ import {
UNDO_REMOVE_CARD_FROM_DASH,
SHOW_AUTO_APPLY_FILTERS_TOAST,
tabsReducer,
+ SET_LOADING_DASHCARDS_COMPLETE,
} from "./actions";
-import { isVirtualDashCard, syncParametersAndEmbeddingParams } from "./utils";
+import { syncParametersAndEmbeddingParams } from "./utils";
import { INITIAL_DASHBOARD_STATE } from "./constants";
const dashboardId = handleActions(
@@ -367,26 +368,19 @@ const loadingDashCards = handleActions(
loadingStatus: "idle",
}),
},
- [FETCH_DASHBOARD]: {
- next: (state, { payload }) => {
- const cardIds = Object.values(payload.entities.dashcard || {})
- .filter(dc => !isVirtualDashCard(dc))
- .map(dc => dc.id);
+ [FETCH_DASHBOARD_CARD_DATA]: {
+ next: (state, { payload: { currentTime, dashcardIds } }) => {
+ const loadingIds = Array.isArray(dashcardIds) ? dashcardIds : [];
+
return {
...state,
- dashcardIds: cardIds,
- loadingIds: cardIds,
- loadingStatus: "idle",
+ dashcardIds: loadingIds,
+ loadingIds,
+ loadingStatus: loadingIds.length > 0 ? "running" : "idle",
+ startTime: loadingIds.length > 0 ? currentTime : null,
};
},
},
- [FETCH_DASHBOARD_CARD_DATA]: {
- next: (state, { payload: { currentTime } }) => ({
- ...state,
- loadingStatus: state.loadingIds.length > 0 ? "running" : "idle",
- startTime: state.loadingIds.length > 0 ? currentTime : null,
- }),
- },
[FETCH_CARD_DATA]: {
next: (state, { payload: { dashcard_id, currentTime } }) => {
const loadingIds = state.loadingIds.filter(id => id !== dashcard_id);
@@ -409,6 +403,15 @@ const loadingDashCards = handleActions(
};
},
},
+ [SET_LOADING_DASHCARDS_COMPLETE]: {
+ next: state => {
+ return {
+ ...state,
+ loadingIds: [],
+ loadingStatus: "complete",
+ };
+ },
+ },
[RESET]: {
next: state => ({
...state,
diff --git a/frontend/src/metabase/dashboard/reducers.unit.spec.js b/frontend/src/metabase/dashboard/reducers.unit.spec.js
index 9b3b019b01a..39b100b07b1 100644
--- a/frontend/src/metabase/dashboard/reducers.unit.spec.js
+++ b/frontend/src/metabase/dashboard/reducers.unit.spec.js
@@ -251,7 +251,7 @@ describe("dashboard reducers", () => {
},
{
type: FETCH_DASHBOARD_CARD_DATA,
- payload: { currentTime: 100 },
+ payload: { currentTime: 100, dashcardIds },
},
),
).toMatchObject({
diff --git a/frontend/src/metabase/public/containers/PublicDashboard.jsx b/frontend/src/metabase/public/containers/PublicDashboard.jsx
index a84931a76ec..a89fff7d343 100644
--- a/frontend/src/metabase/public/containers/PublicDashboard.jsx
+++ b/frontend/src/metabase/public/containers/PublicDashboard.jsx
@@ -86,7 +86,9 @@ class PublicDashboard extends Component {
initialize();
try {
await fetchDashboard(uuid || token, location.query);
- await fetchDashboardCardData({ reload: false, clearCache: true });
+ if (this.props.dashboard.ordered_tabs.length === 0) {
+ await fetchDashboardCardData({ reload: false, clearCache: true });
+ }
} catch (error) {
console.error(error);
setErrorPage(error);
@@ -106,6 +108,12 @@ class PublicDashboard extends Component {
return this._initialize();
}
+ if (!_.isEqual(prevProps.selectedTabId, this.props.selectedTabId)) {
+ this.props.fetchDashboardCardData();
+ this.props.fetchDashboardCardMetadata();
+ return;
+ }
+
if (!_.isEqual(this.props.parameterValues, prevProps.parameterValues)) {
this.props.fetchDashboardCardData({ reload: false, clearCache: true });
}
diff --git a/src/metabase/automagic_dashboards/comparison.clj b/src/metabase/automagic_dashboards/comparison.clj
index 49c086f55c3..f7d26dfdca9 100644
--- a/src/metabase/automagic_dashboards/comparison.clj
+++ b/src/metabase/automagic_dashboards/comparison.clj
@@ -94,16 +94,16 @@
series (-> card-right
(update :name #(format "%s (%s)" % (comparison-name right)))
vector)]
- (update dashboard :ordered_cards conj {:col 0
- :row row
- :size_x populate/grid-width
- :size_y height
- :card card
- :card_id (:id card)
- :series series
- :visualization_settings {:graph.y_axis.auto_split false
- :graph.series_labels [(:name card) (:name (first series))]}
- :id (gensym)}))
+ (update dashboard :ordered_cards conj (merge (populate/card-defaults)
+ {:col 0
+ :row row
+ :size_x populate/grid-width
+ :size_y height
+ :card card
+ :card_id (:id card)
+ :series series
+ :visualization_settings {:graph.y_axis.auto_split false
+ :graph.series_labels [(:name card) (:name (first series))]}})))
(let [width (/ populate/grid-width 2)
series-left (map clone-card (:series card-left))
series-right (map clone-card (:series card-right))
@@ -114,24 +114,25 @@
(not (multiseries? card-right))
(assoc-in [:visualization_settings :graph.colors] [color-right]))]
(-> dashboard
- (update :ordered_cards conj {:col 0
- :row row
- :size_x width
- :size_y height
- :card card-left
- :card_id (:id card-left)
- :series series-left
- :visualization_settings {}
- :id (gensym)})
- (update :ordered_cards conj {:col width
- :row row
- :size_x width
- :size_y height
- :card card-right
- :card_id (:id card-right)
- :series series-right
- :visualization_settings {}
- :id (gensym)})))))
+ (update :ordered_cards conj (merge (populate/card-defaults)
+ {:col 0
+ :row row
+ :size_x width
+ :size_y height
+ :card card-left
+ :card_id (:id card-left)
+ :series series-left
+ :visualization_settings {}}))
+ (update :ordered_cards conj (merge (populate/card-defaults)
+ {:col width
+ :row row
+ :size_x width
+ :size_y height
+ :card card-right
+ :card_id (:id card-right)
+ :series series-right
+ :visualization_settings {}}))))))
+
(populate/add-text-card dashboard {:text (:text card)
:width (/ populate/grid-width 2)
:height (:height card)
diff --git a/src/metabase/automagic_dashboards/populate.clj b/src/metabase/automagic_dashboards/populate.clj
index a513487aee2..01068810423 100644
--- a/src/metabase/automagic_dashboards/populate.clj
+++ b/src/metabase/automagic_dashboards/populate.clj
@@ -126,6 +126,14 @@
y_label (assoc :graph.y_axis.title_text y_label)))}))
+
+(defn card-defaults
+ "Default properties for a dashcard on magic dashboard."
+ []
+ {:id (gensym)
+ :dashboard_tab_id nil
+ :visualization_settings {}})
+
(defn- add-card
"Add a card to dashboard `dashboard` at position [`x`, `y`]."
[dashboard {:keys [title description dataset_query width height id] :as card} [x y]]
@@ -137,33 +145,34 @@
:id (or id (gensym))}
(merge (visualization-settings card))
card/populate-query-fields)]
- (update dashboard :ordered_cards conj {:col y
- :row x
- :size_x width
- :size_y height
- :card card
- :card_id (:id card)
- :visualization_settings {}
- :id (gensym)})))
+ (update dashboard :ordered_cards conj
+ (merge (card-defaults)
+ {:col y
+ :row x
+ :size_x width
+ :size_y height
+ :card card
+ :card_id (:id card)
+ :visualization_settings {}}))))
(defn add-text-card
"Add a text card to dashboard `dashboard` at position [`x`, `y`]."
[dashboard {:keys [text width height visualization-settings]} [x y]]
(update dashboard :ordered_cards conj
- {:creator_id api/*current-user-id*
- :visualization_settings (merge
- {:text text
- :virtual_card {:name nil
- :display :text
- :dataset_query {}
- :visualization_settings {}}}
- visualization-settings)
- :col y
- :row x
- :size_x width
- :size_y height
- :card nil
- :id (gensym)}))
+ (merge (card-defaults)
+ {:creator_id api/*current-user-id*
+ :visualization_settings (merge
+ {:text text
+ :virtual_card {:name nil
+ :display :text
+ :dataset_query {}
+ :visualization_settings {}}}
+ visualization-settings)
+ :col y
+ :row x
+ :size_x width
+ :size_y height
+ :card nil})))
(defn- make-grid
[width height]
diff --git a/test/metabase/api/automagic_dashboards_test.clj b/test/metabase/api/automagic_dashboards_test.clj
index 799596f2b5d..8cac240a5c6 100644
--- a/test/metabase/api/automagic_dashboards_test.clj
+++ b/test/metabase/api/automagic_dashboards_test.clj
@@ -15,10 +15,25 @@
[metabase.transforms.materialize :as tf.materialize]
[metabase.transforms.specs :as tf.specs]
[metabase.util :as u]
+ [schema.core :as s]
[toucan2.tools.with-temp :as t2.with-temp]))
(use-fixtures :once (fixtures/initialize :db :web-server :test-users :test-users-personal-collections))
+(defn- ordered-cards-schema-check
+ [ordered_cards]
+ (testing "check if all cards in ordered_cards contain the required fields"
+ (doseq [card ordered_cards]
+ (is (schema= {:id (s/cond-pre s/Str s/Int)
+ :dashboard_tab_id (s/maybe s/Int)
+ :row s/Int
+ :col s/Int
+ :size_x s/Int
+ :size_y s/Int
+ :visualization_settings (s/maybe (s/named clojure.lang.IPersistentMap "valid map"))
+ s/Any s/Any}
+ card)))))
+
(defn- api-call
([template args]
(api-call template args (constantly true)))
@@ -30,7 +45,9 @@
(mt/with-test-user :rasta
(with-dashboard-cleanup
(let [api-endpoint (apply format (str "automagic-dashboards/" template) args)
- result (validation-fn (mt/user-http-request :rasta :get 200 api-endpoint))]
+ resp (mt/user-http-request :rasta :get 200 api-endpoint)
+ _ (ordered-cards-schema-check (:ordered_cards resp))
+ result (validation-fn resp)]
(when (and result
(try
(testing "Endpoint should return 403 if user does not have permissions"
--
GitLab