Skip to content
Snippets Groups Projects
Unverified Commit cef52044 authored by Kamil Mielnik's avatar Kamil Mielnik Committed by GitHub
Browse files

Fix - Unsaved changes warning shown when going back to notebook after... 

Fix - Unsaved changes warning shown when going back to notebook after visualizing the question (#35003)

* Fix running and then editing the question

* Fix running and then editing the structured model

* Add a test for #35000

* Add a test for models in #35000

* Consolidate waiting for save to be enabled/disabled
- Rename waitForSaveQuestionToBeEnabled to waitForSaveToBeEnabled
- Rename waitForSaveModelToBeEnabled to waitForSaveChangesToBeEnabled
- Rename waitForSaveModelToBeDisabled to waitForSaveChangesToBeDisabled
- Remove waitForSaveNewQuestionToBeEnabled in favor of waitForSaveToBeEnabled
parent 0404a350
Branches
Tags
No related merge requests found
Hide whitespace changes
Inline Side-by-side
frontend/src/metabase/query_builder/containers/QueryBuilder.unit.spec.tsx
+ 77
− 34
View file @ cef52044
......@@ -262,6 +262,7 @@ const setup = async ({
<Route path="new" component={NewModelOptions} />
<Route path="query" component={TestQueryBuilder} />
<Route path="metadata" component={TestQueryBuilder} />
<Route path="notebook" component={TestQueryBuilder} />
<Route path=":slug/query" component={TestQueryBuilder} />
<Route path=":slug/metadata" component={TestQueryBuilder} />
<Route path=":slug/notebook" component={TestQueryBuilder} />
......@@ -400,7 +401,7 @@ describe("QueryBuilder", () => {
});
await triggerNotebookQueryChange();
await waitForSaveModelToBeEnabled();
await waitForSaveChangesToBeEnabled();
const mockEvent = callMockEvent(mockEventListener, "beforeunload");
expect(mockEvent.preventDefault).toHaveBeenCalled();
......@@ -428,7 +429,7 @@ describe("QueryBuilder", () => {
});
await triggerMetadataChange();
await waitForSaveModelToBeEnabled();
await waitForSaveChangesToBeEnabled();
const mockEvent = callMockEvent(mockEventListener, "beforeunload");
expect(mockEvent.preventDefault).toHaveBeenCalled();
......@@ -463,7 +464,7 @@ describe("QueryBuilder", () => {
await waitForLoaderToBeRemoved();
await triggerNativeQueryChange();
await waitForSaveNewQuestionToBeEnabled();
await waitForSaveToBeEnabled();
const mockEvent = callMockEvent(mockEventListener, "beforeunload");
expect(mockEvent.preventDefault).toHaveBeenCalled();
......@@ -605,7 +606,7 @@ describe("QueryBuilder", () => {
setupCardQueryMetadataEndpoint(TEST_NATIVE_CARD);
await startNewNotebookModel();
await waitForSaveQuestionToBeEnabled();
await waitForSaveToBeEnabled();
userEvent.click(screen.getByRole("button", { name: "Save" }));
userEvent.click(
......@@ -651,7 +652,7 @@ describe("QueryBuilder", () => {
});
await triggerNotebookQueryChange();
await waitForSaveQuestionToBeEnabled();
await waitForSaveToBeEnabled();
userEvent.click(screen.getByText("Save"));
userEvent.click(
......@@ -687,7 +688,7 @@ describe("QueryBuilder", () => {
});
await triggerNotebookQueryChange();
await waitForSaveModelToBeEnabled();
await waitForSaveChangesToBeEnabled();
history.push("/redirect");
......@@ -701,10 +702,10 @@ describe("QueryBuilder", () => {
});
await triggerNotebookQueryChange();
await waitForSaveModelToBeEnabled();
await waitForSaveChangesToBeEnabled();
await revertNotebookQueryChange();
await waitForSaveModelToBeDisabled();
await waitForSaveChangesToBeDisabled();
history.push("/redirect");
......@@ -720,7 +721,7 @@ describe("QueryBuilder", () => {
});
await triggerNotebookQueryChange();
await waitForSaveModelToBeEnabled();
await waitForSaveChangesToBeEnabled();
userEvent.click(screen.getByRole("button", { name: "Cancel" }));
......@@ -734,10 +735,10 @@ describe("QueryBuilder", () => {
});
await triggerNotebookQueryChange();
await waitForSaveModelToBeEnabled();
await waitForSaveChangesToBeEnabled();
await revertNotebookQueryChange();
await waitForSaveModelToBeDisabled();
await waitForSaveChangesToBeDisabled();
userEvent.click(screen.getByRole("button", { name: "Cancel" }));
......@@ -753,7 +754,7 @@ describe("QueryBuilder", () => {
});
await triggerNotebookQueryChange();
await waitForSaveModelToBeEnabled();
await waitForSaveChangesToBeEnabled();
userEvent.click(screen.getByRole("button", { name: "Save changes" }));
......@@ -784,7 +785,7 @@ describe("QueryBuilder", () => {
});
await triggerMetadataChange();
await waitForSaveModelToBeEnabled();
await waitForSaveChangesToBeEnabled();
history.push("/redirect");
......@@ -829,7 +830,7 @@ describe("QueryBuilder", () => {
});
await triggerMetadataChange();
await waitForSaveModelToBeEnabled();
await waitForSaveChangesToBeEnabled();
userEvent.click(screen.getByRole("button", { name: "Cancel" }));
......@@ -851,7 +852,7 @@ describe("QueryBuilder", () => {
userEvent.click(screen.getByText("Metadata"));
await triggerMetadataChange();
await waitForSaveModelToBeEnabled();
await waitForSaveChangesToBeEnabled();
userEvent.click(screen.getByRole("button", { name: "Save changes" }));
......@@ -881,7 +882,7 @@ describe("QueryBuilder", () => {
});
await triggerNotebookQueryChange();
await waitForSaveModelToBeEnabled();
await waitForSaveChangesToBeEnabled();
userEvent.click(screen.getByTestId("editor-tabs-metadata-name"));
......@@ -890,7 +891,7 @@ describe("QueryBuilder", () => {
).not.toBeInTheDocument();
await triggerMetadataChange();
await waitForSaveModelToBeEnabled();
await waitForSaveChangesToBeEnabled();
userEvent.click(screen.getByTestId("editor-tabs-query-name"));
......@@ -898,6 +899,29 @@ describe("QueryBuilder", () => {
screen.queryByTestId("leave-confirmation"),
).not.toBeInTheDocument();
});
it("does not show custom warning modal when editing & visualizing the model back and forth (metabase#35000)", async () => {
await setup({
card: TEST_MODEL_CARD,
initialRoute: `/model/${TEST_MODEL_CARD.id}/notebook`,
});
await triggerNotebookQueryChange();
await waitForSaveToBeEnabled();
userEvent.click(screen.getByText("Visualize"));
await waitForLoaderToBeRemoved();
userEvent.click(screen.getByLabelText("notebook icon"));
await waitFor(() => {
expect(screen.getByText("Visualize")).toBeInTheDocument();
});
expect(
screen.queryByTestId("leave-confirmation"),
).not.toBeInTheDocument();
});
});
describe("creating native questions", () => {
......@@ -914,7 +938,7 @@ describe("QueryBuilder", () => {
await waitForLoaderToBeRemoved();
await triggerNativeQueryChange();
await waitForSaveNewQuestionToBeEnabled();
await waitForSaveToBeEnabled();
history.push("/redirect");
......@@ -978,7 +1002,7 @@ describe("QueryBuilder", () => {
await waitForLoaderToBeRemoved();
await triggerNativeQueryChange();
await waitForSaveNewQuestionToBeEnabled();
await waitForSaveToBeEnabled();
userEvent.click(screen.getByText("Save"));
......@@ -1020,7 +1044,7 @@ describe("QueryBuilder", () => {
});
await triggerNativeQueryChange();
await waitForSaveQuestionToBeEnabled();
await waitForSaveToBeEnabled();
history.push("/redirect");
......@@ -1049,7 +1073,7 @@ describe("QueryBuilder", () => {
});
await triggerNativeQueryChange();
await waitForSaveQuestionToBeEnabled();
await waitForSaveToBeEnabled();
userEvent.click(
within(screen.getByTestId("query-builder-main")).getByRole("button", {
......@@ -1069,7 +1093,7 @@ describe("QueryBuilder", () => {
});
await triggerNativeQueryChange();
await waitForSaveQuestionToBeEnabled();
await waitForSaveToBeEnabled();
userEvent.click(screen.getByText("Save"));
......@@ -1104,7 +1128,7 @@ describe("QueryBuilder", () => {
});
await triggerNativeQueryChange();
await waitForSaveQuestionToBeEnabled();
await waitForSaveToBeEnabled();
userEvent.click(screen.getByText("Save"));
......@@ -1149,7 +1173,7 @@ describe("QueryBuilder", () => {
});
await triggerNotebookQueryChange();
await waitForSaveQuestionToBeEnabled();
await waitForSaveToBeEnabled();
history.push("/redirect");
......@@ -1163,7 +1187,7 @@ describe("QueryBuilder", () => {
});
await triggerVisualizationQueryChange();
await waitForSaveQuestionToBeEnabled();
await waitForSaveToBeEnabled();
history.push("/redirect");
......@@ -1185,6 +1209,29 @@ describe("QueryBuilder", () => {
).not.toBeInTheDocument();
});
it("does not show custom warning modal when editing & visualizing the question back and forth (metabase#35000)", async () => {
await setup({
card: TEST_STRUCTURED_CARD,
initialRoute: `/question/${TEST_STRUCTURED_CARD.id}/notebook`,
});
await triggerNotebookQueryChange();
await waitForSaveToBeEnabled();
userEvent.click(screen.getByText("Visualize"));
await waitForLoaderToBeRemoved();
userEvent.click(screen.getByLabelText("notebook icon"));
await waitFor(() => {
expect(screen.getByText("Visualize")).toBeInTheDocument();
});
expect(
screen.queryByTestId("leave-confirmation"),
).not.toBeInTheDocument();
});
it("does not show custom warning modal when saving edited question", async () => {
const { history } = await setup({
card: TEST_STRUCTURED_CARD,
......@@ -1192,7 +1239,7 @@ describe("QueryBuilder", () => {
});
await triggerNotebookQueryChange();
await waitForSaveQuestionToBeEnabled();
await waitForSaveToBeEnabled();
userEvent.click(screen.getByText("Save"));
......@@ -1227,7 +1274,7 @@ describe("QueryBuilder", () => {
});
await triggerNotebookQueryChange();
await waitForSaveQuestionToBeEnabled();
await waitForSaveToBeEnabled();
userEvent.click(screen.getByText("Save"));
......@@ -1409,28 +1456,24 @@ const revertNotebookQueryChange = async () => {
userEvent.tab();
};
const waitForSaveModelToBeEnabled = async () => {
const waitForSaveChangesToBeEnabled = async () => {
await waitFor(() => {
expect(screen.getByRole("button", { name: "Save changes" })).toBeEnabled();
});
};
const waitForSaveModelToBeDisabled = async () => {
const waitForSaveChangesToBeDisabled = async () => {
await waitFor(() => {
expect(screen.getByRole("button", { name: "Save changes" })).toBeDisabled();
});
};
const waitForSaveQuestionToBeEnabled = async () => {
const waitForSaveToBeEnabled = async () => {
await waitFor(() => {
expect(screen.getByText("Save")).toBeEnabled();
});
};
const waitForSaveNewQuestionToBeEnabled = async () => {
await waitForSaveQuestionToBeEnabled();
};
const waitForNativeQueryEditoReady = async () => {
await waitFor(() => {
expect(screen.getByTestId("mock-native-query-editor")).toBeInTheDocument();
......
frontend/src/metabase/query_builder/utils.ts
+ 9
− 2
View file @ cef52044
......@@ -79,8 +79,12 @@ export const isNavigationAllowed = ({
}
const { hash, pathname } = destination;
const isRunningModel = pathname === "/model" && hash.length > 0;
const isRunningQuestion = pathname === "/question" && hash.length > 0;
const runModelPathnames = question.isStructured()
? ["/model", "/model/notebook"]
: ["/model"];
const isRunningModel =
runModelPathnames.includes(pathname) && hash.length > 0;
const validSlugs = [question.id(), question.slug()]
.filter(Boolean)
.map(String);
......@@ -101,6 +105,7 @@ export const isNavigationAllowed = ({
}
if (question.isNative()) {
const isRunningQuestion = pathname === "/question" && hash.length > 0;
return isRunningQuestion;
}
......@@ -109,6 +114,8 @@ export const isNavigationAllowed = ({
* https://github.com/metabase/metabase/issues/34686
*/
if (!isNewQuestion && question.isStructured()) {
const isRunningQuestion =
["/question", "/question/notebook"].includes(pathname) && hash.length > 0;
const allowedPathnames = validSlugs.flatMap(slug => [
`/question/${slug}`,
`/question/${slug}/notebook`,
......
frontend/src/metabase/query_builder/utils.unit.spec.ts
+ 46
− 0
View file @ cef52044
......@@ -99,11 +99,21 @@ const runModelLocation = createMockLocation({
hash: `#${serializeCardForUrl(nativeModelCard)}`,
});
const runModelEditNotebookLocation = createMockLocation({
pathname: "/model/notebook",
hash: `#${serializeCardForUrl(nativeModelCard)}`,
});
const runQuestionLocation = createMockLocation({
pathname: "/question",
hash: `#${serializeCardForUrl(nativeCard)}`,
});
const runQuestionEditNotebookLocation = createMockLocation({
pathname: "/question/notebook",
hash: `#${serializeCardForUrl(nativeCard)}`,
});
describe("isNavigationAllowed", () => {
describe("when there is no destination (i.e. it's a beforeunload event)", () => {
const destination = undefined;
......@@ -143,7 +153,9 @@ describe("isNavigationAllowed", () => {
newModelQueryTabLocation,
newModelMetadataTabLocation,
runModelLocation,
runModelEditNotebookLocation,
runQuestionLocation,
runQuestionEditNotebookLocation,
])("allows navigating away to `$pathname`", destination => {
expect(
isNavigationAllowed({ destination, question, isNewQuestion: true }),
......@@ -168,7 +180,9 @@ describe("isNavigationAllowed", () => {
newModelQueryTabLocation,
newModelMetadataTabLocation,
runModelLocation,
runModelEditNotebookLocation,
runQuestionLocation,
runQuestionEditNotebookLocation,
])("to `$pathname`", destination => {
expect(
isNavigationAllowed({ destination, question, isNewQuestion }),
......@@ -199,6 +213,8 @@ describe("isNavigationAllowed", () => {
newModelQueryTabLocation,
newModelMetadataTabLocation,
runModelLocation,
runModelEditNotebookLocation,
runQuestionEditNotebookLocation,
])("to `$pathname`", destination => {
expect(
isNavigationAllowed({ destination, question, isNewQuestion }),
......@@ -219,6 +235,14 @@ describe("isNavigationAllowed", () => {
).toBe(true);
});
it("allows to run the question and then edit it again", () => {
const destination = runQuestionEditNotebookLocation;
expect(
isNavigationAllowed({ destination, question, isNewQuestion }),
).toBe(true);
});
it("allows to run a model", () => {
const destination = runModelLocation;
......@@ -276,6 +300,8 @@ describe("isNavigationAllowed", () => {
newModelQueryTabLocation,
newModelMetadataTabLocation,
runModelLocation,
runModelEditNotebookLocation,
runQuestionEditNotebookLocation,
])("to `$pathname`", destination => {
expect(
isNavigationAllowed({ destination, question, isNewQuestion }),
......@@ -307,6 +333,14 @@ describe("isNavigationAllowed", () => {
).toBe(true);
});
it("allows to run the model and then edit it again", () => {
const destination = runModelEditNotebookLocation;
expect(
isNavigationAllowed({ destination, question, isNewQuestion }),
).toBe(true);
});
describe("disallows all other navigation", () => {
it.each([
anyLocation,
......@@ -315,6 +349,7 @@ describe("isNavigationAllowed", () => {
...getStructuredQuestionLocations(structuredQuestion),
...getNativeQuestionLocations(nativeQuestion),
runQuestionLocation,
runQuestionEditNotebookLocation,
])("to `$pathname`", destination => {
expect(
isNavigationAllowed({ destination, question, isNewQuestion }),
......@@ -343,6 +378,14 @@ describe("isNavigationAllowed", () => {
).toBe(true);
});
it("allows to run the model and then edit it again", () => {
const destination = runModelEditNotebookLocation;
expect(
isNavigationAllowed({ destination, question, isNewQuestion }),
).toBe(true);
});
describe("disallows all other navigation", () => {
it.each([
anyLocation,
......@@ -351,6 +394,7 @@ describe("isNavigationAllowed", () => {
...getNativeQuestionLocations(nativeQuestion),
newModelMetadataTabLocation,
newModelQueryTabLocation,
runQuestionEditNotebookLocation,
])("to `$pathname`", destination => {
expect(
isNavigationAllowed({ destination, question, isNewQuestion }),
......@@ -387,6 +431,8 @@ describe("isNavigationAllowed", () => {
...getNativeQuestionLocations(nativeQuestion),
newModelMetadataTabLocation,
newModelQueryTabLocation,
runModelEditNotebookLocation,
runQuestionEditNotebookLocation,
])("to `$pathname`", destination => {
expect(
isNavigationAllowed({ destination, question, isNewQuestion }),
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment