From b08e03edeaab473f90ba196b61c5798bc8e53bdc Mon Sep 17 00:00:00 2001
From: Kamil Mielnik <kamil@kamilmielnik.com>
Date: Mon, 23 Oct 2023 21:41:36 +0700
Subject: [PATCH] Do not use history.goBack() to trigger navigation (#34956)
- Some tests had a redundant initial route just so that there is
an entry in history we can go back to. This caused NewItemMenu to
be redundantly rendered, which caused race conditions in the entity
framework. We do not need history.goBack(), we can use history.push()
but we need an extra, safe route we can try to navigate to.
- @see https://metaboat.slack.com/archives/C057T1QTB3L/p1697183111294489
- @see https://metaboat.slack.com/archives/C5XHN8GLW/p1697811520881169
- @see https://github.com/metabase/metabase/pull/34661
---
.../containers/QueryBuilder.unit.spec.tsx | 101 +++++++-----------
1 file changed, 37 insertions(+), 64 deletions(-)
diff --git a/frontend/src/metabase/query_builder/containers/QueryBuilder.unit.spec.tsx b/frontend/src/metabase/query_builder/containers/QueryBuilder.unit.spec.tsx
index d8c898562b8..b2f638ab79e 100644
--- a/frontend/src/metabase/query_builder/containers/QueryBuilder.unit.spec.tsx
+++ b/frontend/src/metabase/query_builder/containers/QueryBuilder.unit.spec.tsx
@@ -212,6 +212,8 @@ const TestQueryBuilder = (
const TestHome = () => <NewItemMenu trigger={<button>New</button>} />;
+const TestRedirect = () => <div />;
+
function isSavedCard(card: Card | UnsavedCard | null): card is Card {
return card !== null && "id" in card;
}
@@ -270,6 +272,7 @@ const setup = async ({
<Route path=":slug" component={TestQueryBuilder} />
<Route path=":slug/notebook" component={TestQueryBuilder} />
</Route>
+ <Route path="/redirect" component={TestRedirect} />
</Route>,
{
withRouter: true,
@@ -570,15 +573,12 @@ describe("QueryBuilder", () => {
it("shows custom warning modal when leaving via SPA navigation", async () => {
const { history } = await setup({
card: null,
- initialRoute: "/",
+ initialRoute: "/model/new",
});
- history.push("/model/new");
- await waitForLoaderToBeRemoved();
-
await startNewNotebookModel();
- history.goBack();
+ history.push("/redirect");
expect(screen.getByTestId("leave-confirmation")).toBeInTheDocument();
});
@@ -629,17 +629,14 @@ describe("QueryBuilder", () => {
it("shows custom warning modal when user tries to leave an ad-hoc native query", async () => {
const { history } = await setup({
card: TEST_UNSAVED_NATIVE_CARD,
- initialRoute: "/",
+ initialRoute: `/question#${serializeCardForUrl(
+ TEST_UNSAVED_NATIVE_CARD,
+ )}`,
});
- history.push(
- `/question#${serializeCardForUrl(TEST_UNSAVED_NATIVE_CARD)}`,
- );
- await waitForLoaderToBeRemoved();
-
await triggerNativeQueryChange();
- history.goBack();
+ history.push("/redirect");
expect(screen.getByTestId("leave-confirmation")).toBeInTheDocument();
});
@@ -674,7 +671,7 @@ describe("QueryBuilder", () => {
screen.queryByTestId("leave-confirmation"),
).not.toBeInTheDocument();
- history.goBack();
+ history.push("/redirect");
expect(
screen.queryByTestId("leave-confirmation"),
@@ -686,16 +683,13 @@ describe("QueryBuilder", () => {
it("shows custom warning modal when leaving edited query via SPA navigation", async () => {
const { history } = await setup({
card: TEST_MODEL_CARD,
- initialRoute: "/",
+ initialRoute: `/model/${TEST_MODEL_CARD.id}/query`,
});
- history.push(`/model/${TEST_MODEL_CARD.id}/query`);
- await waitForLoaderToBeRemoved();
-
await triggerNotebookQueryChange();
await waitForSaveModelToBeEnabled();
- history.goBack();
+ history.push("/redirect");
expect(screen.getByTestId("leave-confirmation")).toBeInTheDocument();
});
@@ -703,19 +697,16 @@ describe("QueryBuilder", () => {
it("does not show custom warning modal when leaving unedited query via SPA navigation", async () => {
const { history } = await setup({
card: TEST_MODEL_CARD,
- initialRoute: "/",
+ initialRoute: `/model/${TEST_MODEL_CARD.id}/query`,
});
- history.push(`/model/${TEST_MODEL_CARD.id}/query`);
- await waitForLoaderToBeRemoved();
-
await triggerNotebookQueryChange();
await waitForSaveModelToBeEnabled();
await revertNotebookQueryChange();
await waitForSaveModelToBeDisabled();
- history.goBack();
+ history.push("/redirect");
expect(
screen.queryByTestId("leave-confirmation"),
@@ -776,7 +767,7 @@ describe("QueryBuilder", () => {
screen.queryByTestId("leave-confirmation"),
).not.toBeInTheDocument();
- history.goBack();
+ history.push("/redirect");
expect(
screen.queryByTestId("leave-confirmation"),
@@ -789,16 +780,13 @@ describe("QueryBuilder", () => {
const { history } = await setup({
card: TEST_MODEL_CARD,
dataset: TEST_MODEL_DATASET,
- initialRoute: "/",
+ initialRoute: `/model/${TEST_MODEL_CARD.id}/metadata`,
});
- history.push(`/model/${TEST_MODEL_CARD.id}/metadata`);
- await waitForLoaderToBeRemoved();
-
await triggerMetadataChange();
await waitForSaveModelToBeEnabled();
- history.goBack();
+ history.push("/redirect");
expect(screen.getByTestId("leave-confirmation")).toBeInTheDocument();
});
@@ -807,13 +795,10 @@ describe("QueryBuilder", () => {
const { history } = await setup({
card: TEST_MODEL_CARD,
dataset: TEST_MODEL_DATASET,
- initialRoute: "/",
+ initialRoute: `/model/${TEST_MODEL_CARD.id}/metadata`,
});
- history.push(`/model/${TEST_MODEL_CARD.id}/metadata`);
- await waitForLoaderToBeRemoved();
-
- history.goBack();
+ history.push("/redirect");
expect(
screen.queryByTestId("leave-confirmation"),
@@ -880,7 +865,7 @@ describe("QueryBuilder", () => {
screen.queryByTestId("leave-confirmation"),
).not.toBeInTheDocument();
- history.goBack();
+ history.push("/redirect");
expect(
screen.queryByTestId("leave-confirmation"),
@@ -931,7 +916,7 @@ describe("QueryBuilder", () => {
await triggerNativeQueryChange();
await waitForSaveNewQuestionToBeEnabled();
- history.goBack();
+ history.push("/redirect");
expect(screen.getByTestId("leave-confirmation")).toBeInTheDocument();
});
@@ -948,7 +933,7 @@ describe("QueryBuilder", () => {
);
await waitForLoaderToBeRemoved();
- history.goBack();
+ history.push("/redirect");
expect(
screen.queryByTestId("leave-confirmation"),
@@ -1019,7 +1004,7 @@ describe("QueryBuilder", () => {
screen.queryByTestId("leave-confirmation"),
).not.toBeInTheDocument();
- history.goBack();
+ history.push("/redirect");
expect(
screen.queryByTestId("leave-confirmation"),
@@ -1031,14 +1016,13 @@ describe("QueryBuilder", () => {
it("shows custom warning modal when leaving edited question via SPA navigation", async () => {
const { history } = await setup({
card: TEST_NATIVE_CARD,
- initialRoute: "/",
+ initialRoute: `/question/${TEST_NATIVE_CARD.id}`,
});
- history.push(`/question/${TEST_NATIVE_CARD.id}`);
await triggerNativeQueryChange();
await waitForSaveQuestionToBeEnabled();
- history.goBack();
+ history.push("/redirect");
expect(screen.getByTestId("leave-confirmation")).toBeInTheDocument();
});
@@ -1046,14 +1030,12 @@ describe("QueryBuilder", () => {
it("does not show custom warning modal leaving with no changes via SPA navigation", async () => {
const { history } = await setup({
card: TEST_NATIVE_CARD,
- initialRoute: "/",
+ initialRoute: `/question/${TEST_NATIVE_CARD.id}`,
});
- history.push(`/question/${TEST_NATIVE_CARD.id}`);
- await waitForLoaderToBeRemoved();
await waitForNativeQueryEditoReady();
- history.goBack();
+ history.push("/redirect");
expect(
screen.queryByTestId("leave-confirmation"),
@@ -1108,7 +1090,7 @@ describe("QueryBuilder", () => {
screen.queryByTestId("leave-confirmation"),
).not.toBeInTheDocument();
- history.goBack();
+ history.push("/redirect");
expect(
screen.queryByTestId("leave-confirmation"),
@@ -1151,7 +1133,7 @@ describe("QueryBuilder", () => {
screen.queryByTestId("leave-confirmation"),
).not.toBeInTheDocument();
- history.goBack();
+ history.push("/redirect");
expect(
screen.queryByTestId("leave-confirmation"),
@@ -1163,16 +1145,13 @@ describe("QueryBuilder", () => {
it("shows custom warning modal when leaving notebook-edited question via SPA navigation", async () => {
const { history } = await setup({
card: TEST_STRUCTURED_CARD,
- initialRoute: "/",
+ initialRoute: `/question/${TEST_STRUCTURED_CARD.id}/notebook`,
});
- history.push(`/question/${TEST_STRUCTURED_CARD.id}/notebook`);
- await waitForLoaderToBeRemoved();
-
await triggerNotebookQueryChange();
await waitForSaveQuestionToBeEnabled();
- history.goBack();
+ history.push("/redirect");
expect(screen.getByTestId("leave-confirmation")).toBeInTheDocument();
});
@@ -1180,16 +1159,13 @@ describe("QueryBuilder", () => {
it("does not show custom warning modal when leaving visualization-edited question via SPA navigation", async () => {
const { history } = await setup({
card: TEST_STRUCTURED_CARD,
- initialRoute: "/",
+ initialRoute: `/question/${TEST_STRUCTURED_CARD.id}`,
});
- history.push(`/question/${TEST_STRUCTURED_CARD.id}`);
- await waitForLoaderToBeRemoved();
-
await triggerVisualizationQueryChange();
await waitForSaveQuestionToBeEnabled();
- history.goBack();
+ history.push("/redirect");
expect(
screen.queryByTestId("leave-confirmation"),
@@ -1199,13 +1175,10 @@ describe("QueryBuilder", () => {
it("does not show custom warning modal leaving with no changes via SPA navigation", async () => {
const { history } = await setup({
card: TEST_STRUCTURED_CARD,
- initialRoute: "/",
+ initialRoute: `/question/${TEST_STRUCTURED_CARD.id}/notebook`,
});
- history.push(`/question/${TEST_STRUCTURED_CARD.id}/notebook`);
- await waitForLoaderToBeRemoved();
-
- history.goBack();
+ history.push("/redirect");
expect(
screen.queryByTestId("leave-confirmation"),
@@ -1240,7 +1213,7 @@ describe("QueryBuilder", () => {
screen.queryByTestId("leave-confirmation"),
).not.toBeInTheDocument();
- history.goBack();
+ history.push("/redirect");
expect(
screen.queryByTestId("leave-confirmation"),
@@ -1283,7 +1256,7 @@ describe("QueryBuilder", () => {
screen.queryByTestId("leave-confirmation"),
).not.toBeInTheDocument();
- history.goBack();
+ history.push("/redirect");
expect(
screen.queryByTestId("leave-confirmation"),
--
GitLab