From 3c1366190620de9999fd492174d94fb9ea1562cc Mon Sep 17 00:00:00 2001
From: Emmad Usmani <emmadusmani@berkeley.edu>
Date: Tue, 30 May 2023 10:53:33 -0700
Subject: [PATCH] fix dashboard save failure after deleting tabs (#31039)
* fix dashboard save failure after deleting tabs
* fix undo regression and remove unused code in delete reducer case
---
.../src/metabase/dashboard/actions/tabs.ts | 44 ++++++++-----
.../DashboardTabs/DashboardTabs.unit.spec.tsx | 62 +++++++++++++------
2 files changed, 72 insertions(+), 34 deletions(-)
diff --git a/frontend/src/metabase/dashboard/actions/tabs.ts b/frontend/src/metabase/dashboard/actions/tabs.ts
index 62c3c98d708..8cee600a60a 100644
--- a/frontend/src/metabase/dashboard/actions/tabs.ts
+++ b/frontend/src/metabase/dashboard/actions/tabs.ts
@@ -38,11 +38,16 @@ const INIT_TABS = "metabase/dashboard/INIT_TABS";
const createNewTabAction = createAction<CreateNewTabPayload>(CREATE_NEW_TAB);
-let tempNewTabId = -2;
+let tempTabId = -2;
+// Needed for testing
+export function resetTempTabId() {
+ tempTabId = -2;
+}
+
export function createNewTab() {
// Decrement by 2 to leave space for two new tabs if dash doesn't have tabs already
- const tabId = tempNewTabId;
- tempNewTabId -= 2;
+ const tabId = tempTabId;
+ tempTabId -= 2;
return createNewTabAction({ tabId });
}
@@ -61,10 +66,18 @@ export const saveCardsAndTabs =
export const initTabs = createAction(INIT_TABS);
-function getPrevDashAndTabs(state: Draft<DashboardState>) {
+function getPrevDashAndTabs({
+ state,
+ filterRemovedTabs = false,
+}: {
+ state: Draft<DashboardState>;
+ filterRemovedTabs?: boolean;
+}) {
const dashId = state.dashboardId;
const prevDash = dashId ? state.dashboards[dashId] : null;
- const prevTabs = prevDash?.ordered_tabs ?? [];
+ const prevTabs =
+ prevDash?.ordered_tabs?.filter(t => !filterRemovedTabs || !t.isRemoved) ??
+ [];
return { dashId, prevDash, prevTabs };
}
@@ -94,7 +107,7 @@ export const tabsReducer = createReducer<DashboardState>(
builder.addCase<typeof createNewTabAction>(
createNewTabAction,
(state, { payload: { tabId } }) => {
- const { dashId, prevDash, prevTabs } = getPrevDashAndTabs(state);
+ const { dashId, prevDash, prevTabs } = getPrevDashAndTabs({ state });
if (!dashId || !prevDash) {
throw Error(
`CREATE_NEW_TAB was dispatched but either dashId (${dashId}) or prevDash (${prevDash}) are null`,
@@ -144,7 +157,10 @@ export const tabsReducer = createReducer<DashboardState>(
builder.addCase(
deleteTab,
(state, { payload: { tabId, tabDeletionId } }) => {
- const { dashId, prevDash, prevTabs } = getPrevDashAndTabs(state);
+ const { dashId, prevDash, prevTabs } = getPrevDashAndTabs({
+ state,
+ filterRemovedTabs: true,
+ });
const tabToRemove = prevTabs.find(({ id }) => id === tabId);
if (!dashId || !prevDash || !tabToRemove) {
throw Error(
@@ -153,11 +169,7 @@ export const tabsReducer = createReducer<DashboardState>(
}
// 1. Select a different tab if needed
- const noTabsRemaining = prevTabs.length === 1;
- const deletingSelectedTab = state.selectedTabId === tabToRemove.id;
- if (noTabsRemaining) {
- state.selectedTabId = null;
- } else if (deletingSelectedTab) {
+ if (state.selectedTabId === tabToRemove.id) {
const tabToRemoveIndex = prevTabs.findIndex(
({ id }) => id === tabToRemove.id,
);
@@ -187,7 +199,7 @@ export const tabsReducer = createReducer<DashboardState>(
);
builder.addCase(undoDeleteTab, (state, { payload: { tabDeletionId } }) => {
- const { prevTabs } = getPrevDashAndTabs(state);
+ const { prevTabs } = getPrevDashAndTabs({ state });
const { tabId, removedDashCardIds } = state.tabDeletions[tabDeletionId];
const removedTab = prevTabs.find(({ id }) => id === tabId);
if (!removedTab) {
@@ -207,7 +219,7 @@ export const tabsReducer = createReducer<DashboardState>(
});
builder.addCase(renameTab, (state, { payload: { tabId, name } }) => {
- const { dashId, prevDash, prevTabs } = getPrevDashAndTabs(state);
+ const { dashId, prevDash, prevTabs } = getPrevDashAndTabs({ state });
const tabToRenameIndex = prevTabs.findIndex(({ id }) => id === tabId);
if (!dashId || !prevDash || tabToRenameIndex === -1) {
throw Error(
@@ -227,7 +239,7 @@ export const tabsReducer = createReducer<DashboardState>(
builder.addCase(
saveCardsAndTabs,
(state, { payload: { cards: newCards, ordered_tabs: newTabs } }) => {
- const { prevDash, prevTabs } = getPrevDashAndTabs(state);
+ const { prevDash, prevTabs } = getPrevDashAndTabs({ state });
if (!prevDash) {
throw Error(
`SAVE_CARDS_AND_TABS was dispatched but prevDash (${prevDash}) is null`,
@@ -251,7 +263,7 @@ export const tabsReducer = createReducer<DashboardState>(
);
builder.addCase(initTabs, state => {
- const { prevTabs } = getPrevDashAndTabs(state);
+ const { prevTabs } = getPrevDashAndTabs({ state });
state.selectedTabId = prevTabs[0]?.id ?? null;
});
},
diff --git a/frontend/src/metabase/dashboard/components/DashboardTabs/DashboardTabs.unit.spec.tsx b/frontend/src/metabase/dashboard/components/DashboardTabs/DashboardTabs.unit.spec.tsx
index cae90379660..45794a0cd23 100644
--- a/frontend/src/metabase/dashboard/components/DashboardTabs/DashboardTabs.unit.spec.tsx
+++ b/frontend/src/metabase/dashboard/components/DashboardTabs/DashboardTabs.unit.spec.tsx
@@ -7,10 +7,11 @@ import { DashboardOrderedTab } from "metabase-types/api";
import { createMockCard } from "metabase-types/api/mocks";
import { ORDERS_ID, SAMPLE_DB_ID } from "metabase-types/api/mocks/presets";
import { INITIAL_DASHBOARD_STATE } from "metabase/dashboard/constants";
-import { getDefaultTab } from "metabase/dashboard/actions";
+import { getDefaultTab, resetTempTabId } from "metabase/dashboard/actions";
import { INPUT_WRAPPER_TEST_ID } from "metabase/core/components/TabButton";
import { DashboardTabs } from "./DashboardTabs";
+import { useDashboardTabs } from "./useDashboardTabs";
const TEST_CARD = createMockCard({
dataset_query: {
@@ -97,12 +98,20 @@ function setup({
},
};
- const { store } = renderWithProviders(
- <DashboardTabs isEditing={isEditing} />,
- {
- storeInitialState: { dashboard },
- },
- );
+ const TestComponent = () => {
+ const { selectedTabId } = useDashboardTabs();
+
+ return (
+ <>
+ <DashboardTabs isEditing={isEditing} />
+ <span>Selected tab id is {selectedTabId}</span>
+ </>
+ );
+ };
+
+ const { store } = renderWithProviders(<TestComponent />, {
+ storeInitialState: { dashboard },
+ });
return {
getDashcards: () =>
Object.values((store.getState() as unknown as State).dashboard.dashcards),
@@ -145,6 +154,10 @@ async function renameTab(num: number, name: string) {
}
describe("DashboardTabs", () => {
+ beforeEach(() => {
+ resetTempTabId();
+ });
+
describe("when not editing", () => {
it("should display tabs without menus when there are two or more", () => {
setup({ isEditing: false });
@@ -171,20 +184,22 @@ describe("DashboardTabs", () => {
expect(queryTab(1)).not.toBeInTheDocument();
});
- it("should automatically select the first tab on render", () => {
- setup({ isEditing: false });
+ describe("when selecting tabs", () => {
+ it("should automatically select the first tab on render", () => {
+ setup({ isEditing: false });
- expect(queryTab(1)).toHaveAttribute("aria-selected", "true");
- expect(queryTab(2)).toHaveAttribute("aria-selected", "false");
- expect(queryTab(3)).toHaveAttribute("aria-selected", "false");
- });
+ expect(queryTab(1)).toHaveAttribute("aria-selected", "true");
+ expect(queryTab(2)).toHaveAttribute("aria-selected", "false");
+ expect(queryTab(3)).toHaveAttribute("aria-selected", "false");
+ });
- it("should allow you to click to select tabs", () => {
- setup({ isEditing: false });
+ it("should allow you to click to select tabs", () => {
+ setup({ isEditing: false });
- expect(selectTab(2)).toHaveAttribute("aria-selected", "true");
- expect(queryTab(1)).toHaveAttribute("aria-selected", "false");
- expect(queryTab(3)).toHaveAttribute("aria-selected", "false");
+ expect(selectTab(2)).toHaveAttribute("aria-selected", "true");
+ expect(queryTab(1)).toHaveAttribute("aria-selected", "false");
+ expect(queryTab(3)).toHaveAttribute("aria-selected", "false");
+ });
});
});
@@ -276,6 +291,17 @@ describe("DashboardTabs", () => {
expect(queryTab(2)).toHaveAttribute("aria-selected", "true");
});
+
+ it("should correctly update selected tab id when deleting tabs (#30923)", async () => {
+ setup({ tabs: [] });
+ createNewTab();
+ createNewTab();
+
+ await deleteTab(2);
+ await deleteTab(2);
+
+ expect(screen.getByText("Selected tab id is -1")).toBeInTheDocument();
+ });
});
describe("when renaming tabs", () => {
--
GitLab