From 008ea7bf4aee23bfea614793d95beb0ca0f9d203 Mon Sep 17 00:00:00 2001
From: Anton Kulyk <kuliks.anton@gmail.com>
Date: Mon, 22 Nov 2021 11:35:03 +0200
Subject: [PATCH] Use /dataset URLs for datasets (#19032)
* Use /dataset URL for datasets
* Show 404 when opening a question with dataset path
* Add E2E tests
* Fix URL change when changing dataset status
---
frontend/src/metabase/App.jsx | 2 +-
frontend/src/metabase/lib/urls.js | 6 ++-
.../src/metabase/query_builder/actions.js | 47 ++++++++++++-------
frontend/src/metabase/routes.jsx | 7 +++
frontend/test/metabase/lib/urls.unit.spec.js | 14 ++++++
.../scenarios/question/datasets.cy.spec.js | 17 ++++++-
6 files changed, 70 insertions(+), 23 deletions(-)
diff --git a/frontend/src/metabase/App.jsx b/frontend/src/metabase/App.jsx
index 936508fd7db..d20a971c336 100644
--- a/frontend/src/metabase/App.jsx
+++ b/frontend/src/metabase/App.jsx
@@ -25,7 +25,7 @@ const mapStateToProps = (state, props) => ({
const getErrorComponent = ({ status, data, context }) => {
if (status === 403) {
return <Unauthorized />;
- } else if (status === 404) {
+ } else if (status === 404 || data?.error_code === "not-found") {
return <NotFound />;
} else if (
data &&
diff --git a/frontend/src/metabase/lib/urls.js b/frontend/src/metabase/lib/urls.js
index 7f6d7149f02..feca8a8bf70 100644
--- a/frontend/src/metabase/lib/urls.js
+++ b/frontend/src/metabase/lib/urls.js
@@ -49,6 +49,8 @@ export function question(card, hash = "", query = "") {
}
const { card_id, id, name } = card;
+ const basePath =
+ card?.dataset || card?.model === "dataset" ? "dataset" : "question";
/**
* If the question has been added to the dashboard we're reading the dashCard's properties.
@@ -65,10 +67,10 @@ export function question(card, hash = "", query = "") {
* Please see: https://github.com/metabase/metabase/pull/15989#pullrequestreview-656646149
*/
if (!name) {
- return `/question/${questionId}${query}${hash}`;
+ return `/${basePath}/${questionId}${query}${hash}`;
}
- const path = appendSlug(`/question/${questionId}`, slugg(name));
+ const path = appendSlug(`/${basePath}/${questionId}`, slugg(name));
return `${path}${query}${hash}`;
}
diff --git a/frontend/src/metabase/query_builder/actions.js b/frontend/src/metabase/query_builder/actions.js
index 992b014e78d..c5178708653 100644
--- a/frontend/src/metabase/query_builder/actions.js
+++ b/frontend/src/metabase/query_builder/actions.js
@@ -449,6 +449,18 @@ export const initializeQB = (location, params, queryParams) => {
card = null;
}
+ if (!card.dataset && location.pathname.startsWith("/dataset")) {
+ dispatch(
+ setErrorPage({
+ data: {
+ error_code: "not-found",
+ },
+ context: "query-builder",
+ }),
+ );
+ card = null;
+ }
+
preserveParameters = true;
} catch (error) {
console.warn("initializeQb failed because of an error:", error);
@@ -1087,7 +1099,7 @@ export const apiCreateQuestion = question => {
};
export const API_UPDATE_QUESTION = "metabase/qb/API_UPDATE_QUESTION";
-export const apiUpdateQuestion = question => {
+export const apiUpdateQuestion = (question, { rerunQuery = false } = {}) => {
return async (dispatch, getState) => {
const originalQuestion = getOriginalQuestion(getState());
question = question || getQuestion(getState());
@@ -1135,6 +1147,11 @@ export const apiUpdateQuestion = question => {
}
dispatch.action(API_UPDATE_QUESTION, updatedQuestion.card());
+
+ if (rerunQuery) {
+ await dispatch(loadMetadataForCard(question.card()));
+ dispatch(runQuestionQuery());
+ }
};
};
@@ -1482,21 +1499,12 @@ export const turnQuestionIntoDataset = () => async (dispatch, getState) => {
// to enable "simple mode" like features
dataset = dataset.composeDataset();
}
- await dispatch(apiUpdateQuestion(dataset));
- await dispatch(loadMetadataForCard(dataset.card()));
-
- // When a question is turned into a dataset,
- // its visualization is changed to a table
- // In order for that transition not to look like something just broke,
- // we rerun the query
- if (question.display() !== "table") {
- dispatch(runQuestionQuery());
- }
+ await dispatch(apiUpdateQuestion(dataset, { rerunQuery: true }));
dispatch(
addUndo({
message: t`This is a dataset now.`,
- actions: [apiUpdateQuestion(question)],
+ actions: [apiUpdateQuestion(question, { rerunQuery: true })],
}),
);
};
@@ -1512,16 +1520,19 @@ export const turnDatasetIntoQuestion = () => async (dispatch, getState) => {
const originalQuestion = getOriginalQuestion(getState());
question = question.setDatasetQuery(originalQuestion.datasetQuery());
}
- await dispatch(apiUpdateQuestion(question));
+ await dispatch(apiUpdateQuestion(question, { rerunQuery: true }));
+
+ const undoAction = apiUpdateQuestion(
+ dataset.isStructured() ? dataset.composeDataset() : dataset,
+ {
+ rerunQuery: true,
+ },
+ );
dispatch(
addUndo({
message: t`This is a question now.`,
- actions: [
- apiUpdateQuestion(
- dataset.isStructured() ? dataset.composeDataset() : dataset,
- ),
- ],
+ actions: [undoAction],
}),
);
};
diff --git a/frontend/src/metabase/routes.jsx b/frontend/src/metabase/routes.jsx
index c13b85277f2..e7a99304e8a 100644
--- a/frontend/src/metabase/routes.jsx
+++ b/frontend/src/metabase/routes.jsx
@@ -240,6 +240,13 @@ export const getRoutes = store => (
<Route path=":slug/notebook" component={QueryBuilder} />
</Route>
+ <Route path="/dataset">
+ <IndexRoute component={QueryBuilder} />
+ <Route path="notebook" component={QueryBuilder} />
+ <Route path=":slug" component={QueryBuilder} />
+ <Route path=":slug/notebook" component={QueryBuilder} />
+ </Route>
+
<Route path="/ready" component={PostSetupApp} />
<Route path="browse" component={BrowseApp}>
diff --git a/frontend/test/metabase/lib/urls.unit.spec.js b/frontend/test/metabase/lib/urls.unit.spec.js
index 6f540082761..1b489af1a3d 100644
--- a/frontend/test/metabase/lib/urls.unit.spec.js
+++ b/frontend/test/metabase/lib/urls.unit.spec.js
@@ -66,6 +66,20 @@ describe("urls", () => {
);
});
});
+
+ describe("dataset", () => {
+ it("returns /dataset URLS", () => {
+ expect(question({ id: 1, dataset: true, name: "Foo" })).toEqual(
+ "/dataset/1-foo",
+ );
+ expect(
+ question({ id: 1, card_id: 42, dataset: true, name: "Foo" }),
+ ).toEqual("/dataset/42-foo");
+ expect(
+ question({ id: 1, card_id: 42, model: "dataset", name: "Foo" }),
+ ).toEqual("/dataset/42-foo");
+ });
+ });
});
describe("query", () => {
diff --git a/frontend/test/metabase/scenarios/question/datasets.cy.spec.js b/frontend/test/metabase/scenarios/question/datasets.cy.spec.js
index 34bf71085e8..5643c15db5e 100644
--- a/frontend/test/metabase/scenarios/question/datasets.cy.spec.js
+++ b/frontend/test/metabase/scenarios/question/datasets.cy.spec.js
@@ -83,7 +83,7 @@ describe("scenarios > datasets", () => {
it("allows to turn a dataset back into a saved question", () => {
cy.request("PUT", "/api/card/1", { dataset: true });
cy.intercept("PUT", "/api/card/1").as("cardUpdate");
- cy.visit("/question/1");
+ cy.visit("/dataset/1");
openDetailsSidebar();
cy.findByText("Turn back into a saved question").click();
@@ -97,6 +97,19 @@ describe("scenarios > datasets", () => {
assertIsDataset();
});
+ it("shows 404 when opening a question with a /dataset URL", () => {
+ cy.visit("/dataset/1");
+ cy.findByText(/We're a little lost/i);
+ });
+
+ it("redirects to /dataset URL when opening a dataset with /question URL", () => {
+ cy.request("PUT", "/api/card/1", { dataset: true });
+ cy.visit("/question/1");
+ openDetailsSidebar();
+ assertIsDataset();
+ cy.url().should("include", "/dataset");
+ });
+
describe("data picker", () => {
beforeEach(() => {
cy.intercept("GET", "/api/search").as("search");
@@ -508,7 +521,7 @@ function openDetailsSidebar() {
cy.findByTestId("saved-question-header-button").click();
}
-function getDetailsSidebarActions(iconName) {
+function getDetailsSidebarActions() {
return cy.findByTestId("question-action-buttons");
}
--
GitLab