From 0e2f490960df9f94e81cec5575d53a154759289d Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Mon, 5 Feb 2024 22:53:36 +0700
Subject: [PATCH] Add `report_card.type` (#38074)
---
.clj-kondo/hooks/clojure/core.clj | 2 +
e2e/snapshot-creators/default.cy.snap.js | 2 +-
e2e/support/commands/api/question.js | 40 ++--
.../helpers/e2e-qa-databases-helpers.js | 2 +-
e2e/support/integration/visit-dashboard.js | 2 +-
...parameter-via-url-does-not-work.cy.spec.js | 2 +-
.../default-sample-database.cy.spec.js | 2 +-
.../collection-pinned-overview.cy.spec.js | 2 +-
.../collections/collections.cy.spec.js | 2 +-
.../collections/permissions.cy.spec.js | 2 +-
.../dashboard-back-navigation.cy.spec.js | 4 +-
.../embedding/embedding-smoketests.cy.spec.js | 2 +-
...lti-nested-joins-wrong-aliasing.cy.spec.js | 2 +-
.../scenarios/models/model-indexes.cy.spec.js | 4 +-
.../models/models-metadata.cy.spec.js | 14 +-
.../models/models-query-editor.cy.spec.js | 8 +-
.../models/models-revision-history.cy.spec.js | 2 +-
e2e/test/scenarios/models/models.cy.spec.js | 14 +-
...-native-model-results-disappear.cy.spec.js | 2 +-
.../20042-nodata-user-blank-screen.cy.spec.js | 2 +-
.../20045-rerun-model-adds-hash.cy.spec.js | 2 +-
...edit-metadata-empty-description.cy.spec.js | 2 +-
...should-override-column-settings.cy.spec.js | 2 +-
...dd-remove-column-drops-metadata.cy.spec.js | 2 +-
.../models/reproductions/22518.cy.spec.js | 2 +-
...lues-override-column-identifier.cy.spec.js | 2 +-
...-apply-dash-filter-native-model.cy.spec.js | 2 +-
...visualization-settins-breaks-ui.cy.spec.js | 2 +-
.../25537-model-picker-locale.cy.spec.js | 2 +-
.../26091-new-models-picker.cy.spec.js | 2 +-
.../28193-cannot-use-custom-column.cy.spec.js | 2 +-
.../29378-actions-search-crash.cy.spec.js | 2 +-
...el-drill-through-click-behavior.cy.spec.js | 2 +-
...1-model-editor-results-metadata.cy.spec.js | 2 +-
...a-model-must-duplicate-question.cy.spec.js | 3 +-
.../31663-no-model-fks.cy.spec.js | 2 +-
.../31905-duplicated-api-request.cy.spec.js | 2 +-
...963-model-question-display-lock.cy.spec.js | 2 +-
e2e/test/scenarios/native/data_ref.cy.spec.js | 2 +-
.../native/native_subquery.cy.spec.js | 4 +-
.../scenarios/onboarding/metabot.cy.spec.js | 2 +-
.../28788-search-results-overflow.cy.spec.js | 2 +-
.../onboarding/search/search.cy.spec.js | 4 +-
.../bookmarks-collection.cy.spec.js | 2 +-
.../content-verification.cy.spec.js | 2 +-
e2e/test/scenarios/question/new.cy.spec.js | 2 +-
.../scenarios/question/notebook.cy.spec.js | 6 +-
.../34414-populate-field-values.cy.spec.js | 2 +-
.../sharing/public-sharing.cy.spec.js | 2 +-
frontend/src/metabase-lib/Question.ts | 25 ++-
.../metabase-lib/queries/structured/Join.ts | 4 +-
frontend/src/metabase-types/api/card.ts | 6 +
frontend/src/metabase-types/api/mocks/card.ts | 1 +
.../QueryActionContextProvider.tsx | 1 +
frontend/src/metabase/entities/questions.js | 1 +
.../ModelDetailLink/ModelDetailLink.tsx | 13 +-
.../utils/mapping-options.unit.spec.js | 2 +-
.../query_builder/actions/core/core.js | 8 +-
.../actions/core/updateQuestion.ts | 6 +-
.../metabase/query_builder/actions/models.js | 4 +-
.../query_builder/actions/querying.js | 2 +-
.../DatasetEditor/DatasetQueryEditor.jsx | 2 +-
.../StructuredQuery-nesting.unit.spec.js | 2 +-
.../migrations/001_update_migrations.yaml | 36 ++++
src/metabase/api/card.clj | 31 +--
src/metabase/db/custom_migrations.clj | 75 ++++++++
src/metabase/models/card.clj | 178 +++++++++++++-----
src/metabase/models/revision.clj | 2 +-
src/metabase/models/revision/diff.clj | 9 +-
test/metabase/api/card_test.clj | 66 ++++++-
test/metabase/api/revision_test.clj | 4 +-
test/metabase/api/search_test.clj | 16 +-
test/metabase/db/custom_migrations_test.clj | 72 ++++++-
test/metabase/events/revision_test.clj | 2 +-
test/metabase/models/card_test.clj | 1 +
test/metabase/models/revision_test.clj | 16 +-
76 files changed, 569 insertions(+), 195 deletions(-)
diff --git a/.clj-kondo/hooks/clojure/core.clj b/.clj-kondo/hooks/clojure/core.clj
index beea24347e9..c9cd4c404cf 100644
--- a/.clj-kondo/hooks/clojure/core.clj
+++ b/.clj-kondo/hooks/clojure/core.clj
@@ -21,7 +21,9 @@
toucan2.core/delete!
toucan2.core/update!
toucan2.core/insert!
+ toucan2.core/insert-returning-instance!
toucan2.core/insert-returning-instances!
+ toucan2.core/insert-returning-pk!
toucan2.core/insert-returning-pks!
clojure.core.async/<!!
clojure.core.async/>!!
diff --git a/e2e/snapshot-creators/default.cy.snap.js b/e2e/snapshot-creators/default.cy.snap.js
index 34d4e72f4f9..33b5a9743f7 100644
--- a/e2e/snapshot-creators/default.cy.snap.js
+++ b/e2e/snapshot-creators/default.cy.snap.js
@@ -214,7 +214,7 @@ describe("snapshots", () => {
cy.createQuestion({
name: "Orders Model",
query: { "source-table": ORDERS_ID },
- dataset: true,
+ type: "model",
});
}
diff --git a/e2e/support/commands/api/question.js b/e2e/support/commands/api/question.js
index f9aa5609c40..fc84604e115 100644
--- a/e2e/support/commands/api/question.js
+++ b/e2e/support/commands/api/question.js
@@ -30,12 +30,12 @@ Cypress.Commands.add(
/**
*
- * @param {("query"|"native")} type
+ * @param {("query"|"native")} queryType
*
* @param {object} questionDetails
* @param {string} [questionDetails.name="test question"]
* @param {string} questionDetails.description
- * @param {boolean} questionDetails.dataset - Is this a Model or no? (model = dataset)
+ * @param {("question"|"model")} questionDetails.type Entity type
* @param {object} questionDetails.native
* @param {object} questionDetails.query
* @param {number} [questionDetails.database=1]
@@ -52,11 +52,11 @@ Cypress.Commands.add(
* @param {string} customOptions.interceptAlias - We need distinctive endpoint aliases for cases where we have multiple questions or nested questions.
*/
function question(
- type,
+ queryType,
{
name = "test question",
description,
- dataset = false,
+ type = "question",
native,
query,
database = SAMPLE_DB_ID,
@@ -81,8 +81,8 @@ function question(
name,
description,
dataset_query: {
- type,
- [type]: type === "native" ? native : query,
+ type: queryType,
+ [queryType]: queryType === "native" ? native : query,
database,
},
display,
@@ -104,27 +104,27 @@ function question(
cy.wrap(body.id).as(idAlias);
}
- if (dataset || enable_embedding) {
+ if (type === "model" || enable_embedding) {
cy.request("PUT", `/api/card/${body.id}`, {
- dataset,
+ type,
enable_embedding,
embedding_params,
});
}
if (loadMetadata || visitQuestion) {
- dataset
- ? cy.intercept("POST", `/api/dataset`).as("dataset")
- : // We need to use the wildcard because endpoint for pivot tables has the following format: `/api/card/pivot/${id}/query`
- cy
- .intercept("POST", `/api/card/**/${body.id}/query`)
- .as(interceptAlias);
-
- const url = dataset ? `/model/${body.id}` : `/question/${body.id}`;
- cy.visit(url);
-
- // Wait for `result_metadata` to load
- dataset ? cy.wait("@dataset") : cy.wait("@" + interceptAlias);
+ if (type === "model") {
+ cy.intercept("POST", `/api/dataset`).as("dataset");
+ cy.visit(`/model/${body.id}`);
+ cy.wait("@dataset"); // Wait for `result_metadata` to load
+ } else {
+ // We need to use the wildcard because endpoint for pivot tables has the following format: `/api/card/pivot/${id}/query`
+ cy.intercept("POST", `/api/card/**/${body.id}/query`).as(
+ interceptAlias,
+ );
+ cy.visit(`/question/${body.id}`);
+ cy.wait("@" + interceptAlias); // Wait for `result_metadata` to load
+ }
}
});
}
diff --git a/e2e/support/helpers/e2e-qa-databases-helpers.js b/e2e/support/helpers/e2e-qa-databases-helpers.js
index b08d848c6ff..cf428a413c7 100644
--- a/e2e/support/helpers/e2e-qa-databases-helpers.js
+++ b/e2e/support/helpers/e2e-qa-databases-helpers.js
@@ -220,7 +220,7 @@ export const createModelFromTableName = ({
query: {
"source-table": tableId,
},
- dataset: true,
+ type: "model",
},
{
wrapId: true,
diff --git a/e2e/support/integration/visit-dashboard.js b/e2e/support/integration/visit-dashboard.js
index 84f32731f18..e73c136fb2e 100644
--- a/e2e/support/integration/visit-dashboard.js
+++ b/e2e/support/integration/visit-dashboard.js
@@ -34,7 +34,7 @@ const questionDetails = {
const modelDetails = {
name: "GUI Model",
query: { "source-table": PRODUCTS_ID },
- dataset: true,
+ type: "model",
};
const pivotTable = {
diff --git a/e2e/test/scenarios/actions/reproductions/32974-linked-parameter-via-url-does-not-work.cy.spec.js b/e2e/test/scenarios/actions/reproductions/32974-linked-parameter-via-url-does-not-work.cy.spec.js
index b9254709f88..50487c2de28 100644
--- a/e2e/test/scenarios/actions/reproductions/32974-linked-parameter-via-url-does-not-work.cy.spec.js
+++ b/e2e/test/scenarios/actions/reproductions/32974-linked-parameter-via-url-does-not-work.cy.spec.js
@@ -45,7 +45,7 @@ const MODEL_DETAILS = {
name: "Products model",
query: { "source-table": PRODUCTS_ID },
database: SAMPLE_DB_ID,
- dataset: true,
+ type: "model",
};
const EXPECTED_UPDATED_VALUE = 999;
diff --git a/e2e/test/scenarios/admin/databases/default-sample-database.cy.spec.js b/e2e/test/scenarios/admin/databases/default-sample-database.cy.spec.js
index 65a2a904313..222b6cbc0c9 100644
--- a/e2e/test/scenarios/admin/databases/default-sample-database.cy.spec.js
+++ b/e2e/test/scenarios/admin/databases/default-sample-database.cy.spec.js
@@ -124,7 +124,7 @@ describe("scenarios > admin > databases > sample database", () => {
);
cy.intercept("DELETE", `/api/database/${SAMPLE_DB_ID}`).as("delete");
// model
- cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { dataset: true });
+ cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { type: "model" });
// Create a segment through API
createSegment({
name: "Small orders",
diff --git a/e2e/test/scenarios/collections/collection-pinned-overview.cy.spec.js b/e2e/test/scenarios/collections/collection-pinned-overview.cy.spec.js
index a3e8dd82015..1fb189fa823 100644
--- a/e2e/test/scenarios/collections/collection-pinned-overview.cy.spec.js
+++ b/e2e/test/scenarios/collections/collection-pinned-overview.cy.spec.js
@@ -107,7 +107,7 @@ describe("scenarios > collection pinned items overview", () => {
});
it("should be able to pin a model", () => {
- cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { dataset: true });
+ cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { type: "model" });
openRootCollection();
openUnpinnedItemMenu(MODEL_NAME);
diff --git a/e2e/test/scenarios/collections/collections.cy.spec.js b/e2e/test/scenarios/collections/collections.cy.spec.js
index f4f905a7c11..f868a17f184 100644
--- a/e2e/test/scenarios/collections/collections.cy.spec.js
+++ b/e2e/test/scenarios/collections/collections.cy.spec.js
@@ -583,7 +583,7 @@ describe("scenarios > collection defaults", () => {
});
it("should allow to x-ray models from collection views", () => {
- cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { dataset: true });
+ cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { type: "model" });
cy.visit("/collection/root");
openEllipsisMenuFor("Orders");
diff --git a/e2e/test/scenarios/collections/permissions.cy.spec.js b/e2e/test/scenarios/collections/permissions.cy.spec.js
index acd796df0e3..faf8086123e 100644
--- a/e2e/test/scenarios/collections/permissions.cy.spec.js
+++ b/e2e/test/scenarios/collections/permissions.cy.spec.js
@@ -170,7 +170,7 @@ describe("collection permissions", () => {
cy.skipOn(user === "nodata");
cy.createNativeQuestion({
name: "Model",
- dataset: true,
+ type: "model",
native: {
query: "SELECT * FROM ORDERS",
},
diff --git a/e2e/test/scenarios/dashboard/dashboard-back-navigation.cy.spec.js b/e2e/test/scenarios/dashboard/dashboard-back-navigation.cy.spec.js
index e86b2ab28b9..4ca49af1253 100644
--- a/e2e/test/scenarios/dashboard/dashboard-back-navigation.cy.spec.js
+++ b/e2e/test/scenarios/dashboard/dashboard-back-navigation.cy.spec.js
@@ -128,7 +128,7 @@ describe("scenarios > dashboard > dashboard back navigation", () => {
{ tags: "@slow" },
() => {
const cardTitle = "Orders by Subtotal";
- cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { dataset: true });
+ cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { type: "model" });
cy.visit(
`/auto/dashboard/model/${ORDERS_QUESTION_ID}?#show=${MAX_CARDS}`,
);
@@ -369,7 +369,7 @@ const createDashboardWithCards = () => {
const modelDetails = {
name: "Orders model",
query: { "source-table": ORDERS_ID },
- dataset: true,
+ type: "model",
};
const actionDetails = {
diff --git a/e2e/test/scenarios/embedding/embedding-smoketests.cy.spec.js b/e2e/test/scenarios/embedding/embedding-smoketests.cy.spec.js
index a4565d522eb..dd039426a8c 100644
--- a/e2e/test/scenarios/embedding/embedding-smoketests.cy.spec.js
+++ b/e2e/test/scenarios/embedding/embedding-smoketests.cy.spec.js
@@ -29,7 +29,7 @@ describe("scenarios > embedding > smoke tests", { tags: "@OSS" }, () => {
it("should not offer to share or embed models (metabase#20815)", () => {
cy.intercept("POST", "/api/dataset").as("dataset");
- cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { dataset: true });
+ cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { type: "model" });
cy.visit(`/model/${ORDERS_QUESTION_ID}`);
cy.wait("@dataset");
diff --git a/e2e/test/scenarios/joins/reproductions/22859-multi-nested-joins-wrong-aliasing.cy.spec.js b/e2e/test/scenarios/joins/reproductions/22859-multi-nested-joins-wrong-aliasing.cy.spec.js
index 252631009f0..24203e0f5e0 100644
--- a/e2e/test/scenarios/joins/reproductions/22859-multi-nested-joins-wrong-aliasing.cy.spec.js
+++ b/e2e/test/scenarios/joins/reproductions/22859-multi-nested-joins-wrong-aliasing.cy.spec.js
@@ -75,7 +75,7 @@ describe("issue 22859 - multiple levels of nesting", () => {
it("model based on multi-level nested saved question should work (metabase#22859-1)", () => {
cy.get("@q2Id").then(id => {
// Convert the second question to a model
- cy.request("PUT", `/api/card/${id}`, { dataset: true });
+ cy.request("PUT", `/api/card/${id}`, { type: "model" });
cy.intercept("POST", "/api/dataset").as("dataset");
cy.visit(`/model/${id}`);
diff --git a/e2e/test/scenarios/models/model-indexes.cy.spec.js b/e2e/test/scenarios/models/model-indexes.cy.spec.js
index 65f78d6d8fd..50ba792d302 100644
--- a/e2e/test/scenarios/models/model-indexes.cy.spec.js
+++ b/e2e/test/scenarios/models/model-indexes.cy.spec.js
@@ -28,7 +28,7 @@ describe("scenarios > model indexes", () => {
{
name: "Products Model",
query: { "source-table": PRODUCTS_ID },
- dataset: true,
+ type: "model",
},
{ wrapId: true, idAlias: "modelId" },
);
@@ -161,7 +161,7 @@ describe("scenarios > model indexes", () => {
{
name: "People Model",
query: { "source-table": PEOPLE_ID },
- dataset: true,
+ type: "model",
},
{
wrapId: true,
diff --git a/e2e/test/scenarios/models/models-metadata.cy.spec.js b/e2e/test/scenarios/models/models-metadata.cy.spec.js
index 788e2274efa..c4a5cf320d6 100644
--- a/e2e/test/scenarios/models/models-metadata.cy.spec.js
+++ b/e2e/test/scenarios/models/models-metadata.cy.spec.js
@@ -41,7 +41,7 @@ describe("scenarios > models metadata", () => {
"source-table": ORDERS_ID,
limit: 5,
},
- dataset: true,
+ type: "model",
};
cy.createQuestion(modelDetails).then(({ body: { id } }) => {
@@ -126,7 +126,7 @@ describe("scenarios > models metadata", () => {
cy.createNativeQuestion(
{
name: "Native Model",
- dataset: true,
+ type: "model",
native: {
query: "SELECT * FROM ORDERS LIMIT 5",
},
@@ -176,7 +176,7 @@ describe("scenarios > models metadata", () => {
cy.createNativeQuestion(
{
name: "Native Model",
- dataset: true,
+ type: "model",
native: {
query: "SELECT * FROM ORDERS LIMIT 5",
},
@@ -198,7 +198,7 @@ describe("scenarios > models metadata", () => {
cy.createNativeQuestion(
{
name: "Native Model",
- dataset: true,
+ type: "model",
native: {
query: "SELECT * FROM ORDERS LIMIT 5",
},
@@ -234,7 +234,7 @@ describe("scenarios > models metadata", () => {
cy.createNativeQuestion({
name: "Native Model",
- dataset: true,
+ type: "model",
native: {
query: "SELECT * FROM ORDERS LIMIT 5",
},
@@ -289,7 +289,7 @@ describe("scenarios > models metadata", () => {
cy.createNativeQuestion(
{
name: "Native Model",
- dataset: true,
+ type: "model",
native: {
query: "select * from orders limit 100",
},
@@ -427,7 +427,7 @@ describe("scenarios > models metadata", () => {
const questionDetails = {
name: "22521",
- dataset: true,
+ type: "model",
query: {
"source-table": PRODUCTS_ID,
limit: 5,
diff --git a/e2e/test/scenarios/models/models-query-editor.cy.spec.js b/e2e/test/scenarios/models/models-query-editor.cy.spec.js
index 07a7bd00b3d..f800b6c9ff2 100644
--- a/e2e/test/scenarios/models/models-query-editor.cy.spec.js
+++ b/e2e/test/scenarios/models/models-query-editor.cy.spec.js
@@ -23,7 +23,7 @@ describe("scenarios > models query editor", () => {
beforeEach(() => {
cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, {
name: "Orders Model",
- dataset: true,
+ type: "model",
});
});
@@ -122,7 +122,7 @@ describe("scenarios > models query editor", () => {
cy.createNativeQuestion(
{
name: "Native Model",
- dataset: true,
+ type: "model",
native: {
query: "SELECT * FROM orders limit 5",
},
@@ -161,7 +161,7 @@ describe("scenarios > models query editor", () => {
cy.createNativeQuestion(
{
name: "Native Model",
- dataset: true,
+ type: "model",
native: {
query: "SELECT * FROM orders limit 5",
},
@@ -199,7 +199,7 @@ describe("scenarios > models query editor", () => {
cy.createNativeQuestion(
{
name: "Erroring Model",
- dataset: true,
+ type: "model",
native: {
// Let's use API to type the most of the query, but stil make it invalid
query: "SELECT 1 FROM",
diff --git a/e2e/test/scenarios/models/models-revision-history.cy.spec.js b/e2e/test/scenarios/models/models-revision-history.cy.spec.js
index daebf109ba4..ab9fc1aab7e 100644
--- a/e2e/test/scenarios/models/models-revision-history.cy.spec.js
+++ b/e2e/test/scenarios/models/models-revision-history.cy.spec.js
@@ -7,7 +7,7 @@ describe("scenarios > models > revision history", () => {
cy.signInAsAdmin();
cy.request("PUT", `/api/card/${ORDERS_BY_YEAR_QUESTION_ID}`, {
name: "Orders Model",
- dataset: true,
+ type: "model",
});
});
diff --git a/e2e/test/scenarios/models/models.cy.spec.js b/e2e/test/scenarios/models/models.cy.spec.js
index 38e1a7ef285..436aa257e31 100644
--- a/e2e/test/scenarios/models/models.cy.spec.js
+++ b/e2e/test/scenarios/models/models.cy.spec.js
@@ -192,7 +192,7 @@ describe("scenarios > models", () => {
});
it("allows to turn a model back into a saved question", () => {
- cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { dataset: true });
+ cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { type: "model" });
cy.intercept("PUT", `/api/card/${ORDERS_QUESTION_ID}`).as("cardUpdate");
cy.visit(`/model/${ORDERS_QUESTION_ID}`);
@@ -222,7 +222,7 @@ describe("scenarios > models", () => {
});
it("redirects to /model URL when opening a model with /question URL", () => {
- cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { dataset: true });
+ cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { type: "model" });
// Important - do not use visitQuestion(ORDERS_QUESTION_ID) here!
cy.visit("/question/" + ORDERS_QUESTION_ID);
cy.wait("@dataset");
@@ -234,7 +234,7 @@ describe("scenarios > models", () => {
describe("data picker", () => {
beforeEach(() => {
cy.intercept("GET", "/api/search*").as("search");
- cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { dataset: true });
+ cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { type: "model" });
});
it("transforms the data picker", () => {
@@ -348,7 +348,7 @@ describe("scenarios > models", () => {
beforeEach(() => {
cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, {
name: "Orders Model",
- dataset: true,
+ type: "model",
});
});
@@ -493,7 +493,7 @@ describe("scenarios > models", () => {
cy.createNativeQuestion({
native: { query: "SELECT * FROM products" },
}).then(({ body: { id: modelId } }) => {
- cy.request("PUT", `/api/card/${modelId}`, { dataset: true }).then(() => {
+ cy.request("PUT", `/api/card/${modelId}`, { type: "model" }).then(() => {
cy.visit(`/model/${modelId}/query`);
cy.get(".ace_editor:not(.ace_autocomplete)")
.should("be.visible")
@@ -510,7 +510,7 @@ describe("scenarios > models", () => {
cy.intercept("POST", "/api/card/*/query").as("cardQuery");
cy.createNativeQuestion({
name: "TEST MODEL",
- dataset: true,
+ type: "model",
native: {
query: "select * from orders",
},
@@ -555,7 +555,7 @@ describe("scenarios > models", () => {
"source-table": ORDERS_ID,
limit: 5,
},
- dataset: true,
+ type: "model",
};
beforeEach(() => {
diff --git a/e2e/test/scenarios/models/reproductions/19180-native-model-results-disappear.cy.spec.js b/e2e/test/scenarios/models/reproductions/19180-native-model-results-disappear.cy.spec.js
index 3fb0e8e509a..6c0f873175e 100644
--- a/e2e/test/scenarios/models/reproductions/19180-native-model-results-disappear.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/19180-native-model-results-disappear.cy.spec.js
@@ -13,7 +13,7 @@ describe("issue 19180", () => {
it("shouldn't drop native model query results after leaving the query editor", () => {
cy.createNativeQuestion(QUESTION).then(({ body: { id: QUESTION_ID } }) => {
- cy.request("PUT", `/api/card/${QUESTION_ID}`, { dataset: true }).then(
+ cy.request("PUT", `/api/card/${QUESTION_ID}`, { type: "model" }).then(
() => {
cy.visit(`/model/${QUESTION_ID}/query`);
cy.wait("@cardQuery");
diff --git a/e2e/test/scenarios/models/reproductions/20042-nodata-user-blank-screen.cy.spec.js b/e2e/test/scenarios/models/reproductions/20042-nodata-user-blank-screen.cy.spec.js
index 7cb793bd81e..8c45c760b73 100644
--- a/e2e/test/scenarios/models/reproductions/20042-nodata-user-blank-screen.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/20042-nodata-user-blank-screen.cy.spec.js
@@ -10,7 +10,7 @@ describe("issue 20042", () => {
cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, {
name: "Orders Model",
- dataset: true,
+ type: "model",
});
cy.signIn("nodata");
diff --git a/e2e/test/scenarios/models/reproductions/20045-rerun-model-adds-hash.cy.spec.js b/e2e/test/scenarios/models/reproductions/20045-rerun-model-adds-hash.cy.spec.js
index 154ad5d66b1..495a2a93ff3 100644
--- a/e2e/test/scenarios/models/reproductions/20045-rerun-model-adds-hash.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/20045-rerun-model-adds-hash.cy.spec.js
@@ -10,7 +10,7 @@ describe("issue 20045", () => {
cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, {
name: "Orders Model",
- dataset: true,
+ type: "model",
});
});
diff --git a/e2e/test/scenarios/models/reproductions/20517-edit-metadata-empty-description.cy.spec.js b/e2e/test/scenarios/models/reproductions/20517-edit-metadata-empty-description.cy.spec.js
index 3f4cf01bf79..506eb7f0366 100644
--- a/e2e/test/scenarios/models/reproductions/20517-edit-metadata-empty-description.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/20517-edit-metadata-empty-description.cy.spec.js
@@ -9,7 +9,7 @@ const modelDetails = {
"source-table": ORDERS_ID,
limit: 5,
},
- dataset: true,
+ type: "model",
};
describe("issue 20517", () => {
diff --git a/e2e/test/scenarios/models/reproductions/20624-model-metadata-should-override-column-settings.cy.spec.js b/e2e/test/scenarios/models/reproductions/20624-model-metadata-should-override-column-settings.cy.spec.js
index 0f0c39fe570..d866641ab99 100644
--- a/e2e/test/scenarios/models/reproductions/20624-model-metadata-should-override-column-settings.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/20624-model-metadata-should-override-column-settings.cy.spec.js
@@ -5,7 +5,7 @@ const renamedColumn = "TITLE renamed";
const questionDetails = {
name: "20624",
- dataset: true,
+ type: "model",
native: { query: "select * from PRODUCTS limit 2" },
visualization_settings: {
column_settings: { '["name","TITLE"]': { column_title: renamedColumn } },
diff --git a/e2e/test/scenarios/models/reproductions/22517-add-remove-column-drops-metadata.cy.spec.js b/e2e/test/scenarios/models/reproductions/22517-add-remove-column-drops-metadata.cy.spec.js
index aa1704b711c..b9be4dc903a 100644
--- a/e2e/test/scenarios/models/reproductions/22517-add-remove-column-drops-metadata.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/22517-add-remove-column-drops-metadata.cy.spec.js
@@ -12,7 +12,7 @@ describe("issue 22517", () => {
{
name: "22517",
native: { query: `select * from orders` },
- dataset: true,
+ type: "model",
},
{ visitQuestion: true },
);
diff --git a/e2e/test/scenarios/models/reproductions/22518.cy.spec.js b/e2e/test/scenarios/models/reproductions/22518.cy.spec.js
index a2cb50788fa..f2dcbb7317a 100644
--- a/e2e/test/scenarios/models/reproductions/22518.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/22518.cy.spec.js
@@ -15,7 +15,7 @@ describe("issue 22518", () => {
native: {
query: "select 1 id, 'a' foo",
},
- dataset: true,
+ type: "model",
},
{ visitQuestion: true },
);
diff --git a/e2e/test/scenarios/models/reproductions/22715-remapped-values-override-column-identifier.cy.spec.js b/e2e/test/scenarios/models/reproductions/22715-remapped-values-override-column-identifier.cy.spec.js
index 8cb99b97623..126822e28f7 100644
--- a/e2e/test/scenarios/models/reproductions/22715-remapped-values-override-column-identifier.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/22715-remapped-values-override-column-identifier.cy.spec.js
@@ -23,7 +23,7 @@ describe("filtering based on the remapped column name should result in a correct
visitQuestion(id);
// Turn the question into a model
- cy.request("PUT", `/api/card/${id}`, { dataset: true });
+ cy.request("PUT", `/api/card/${id}`, { type: "model" });
// Let's go straight to the model metadata editor
cy.visit(`/model/${id}/metadata`);
diff --git a/e2e/test/scenarios/models/reproductions/23024-cannot-apply-dash-filter-native-model.cy.spec.js b/e2e/test/scenarios/models/reproductions/23024-cannot-apply-dash-filter-native-model.cy.spec.js
index d2e9bcda2cd..1f5d4402f6a 100644
--- a/e2e/test/scenarios/models/reproductions/23024-cannot-apply-dash-filter-native-model.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/23024-cannot-apply-dash-filter-native-model.cy.spec.js
@@ -25,7 +25,7 @@ describe("issue 23024", () => {
query: `select *
from products limit 5`,
},
- dataset: true,
+ type: "model",
},
{ wrapId: true, idAlias: "modelId" },
);
diff --git a/e2e/test/scenarios/models/reproductions/23421-visualization-settins-breaks-ui.cy.spec.js b/e2e/test/scenarios/models/reproductions/23421-visualization-settins-breaks-ui.cy.spec.js
index bbabf815789..7a0b9638311 100644
--- a/e2e/test/scenarios/models/reproductions/23421-visualization-settins-breaks-ui.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/23421-visualization-settins-breaks-ui.cy.spec.js
@@ -12,7 +12,7 @@ const questionDetails = {
"table.pivot_column": "orphaned1",
"table.cell_column": "orphaned2",
},
- dataset: true,
+ type: "model",
};
describe("issue 23421", () => {
diff --git a/e2e/test/scenarios/models/reproductions/25537-model-picker-locale.cy.spec.js b/e2e/test/scenarios/models/reproductions/25537-model-picker-locale.cy.spec.js
index 336bd94d88d..f3285fa55db 100644
--- a/e2e/test/scenarios/models/reproductions/25537-model-picker-locale.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/25537-model-picker-locale.cy.spec.js
@@ -6,7 +6,7 @@ const { ORDERS_ID } = SAMPLE_DATABASE;
const questionDetails = {
name: "Orders model",
query: { "source-table": ORDERS_ID },
- dataset: true,
+ type: "model",
};
describe("issue 25537", () => {
diff --git a/e2e/test/scenarios/models/reproductions/26091-new-models-picker.cy.spec.js b/e2e/test/scenarios/models/reproductions/26091-new-models-picker.cy.spec.js
index d08653ecdd3..50aaf75d773 100644
--- a/e2e/test/scenarios/models/reproductions/26091-new-models-picker.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/26091-new-models-picker.cy.spec.js
@@ -7,7 +7,7 @@ const { PRODUCTS_ID } = SAMPLE_DATABASE;
const modelDetails = {
name: "Old model",
query: { "source-table": PRODUCTS_ID },
- dataset: true,
+ type: "model",
};
describe("issue 26091", () => {
diff --git a/e2e/test/scenarios/models/reproductions/28193-cannot-use-custom-column.cy.spec.js b/e2e/test/scenarios/models/reproductions/28193-cannot-use-custom-column.cy.spec.js
index c5334e92f83..4ab761b7091 100644
--- a/e2e/test/scenarios/models/reproductions/28193-cannot-use-custom-column.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/28193-cannot-use-custom-column.cy.spec.js
@@ -11,7 +11,7 @@ describe("issue 28193", () => {
cy.signInAsAdmin();
// Turn the question into a model
- cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { dataset: true });
+ cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { type: "model" });
});
it("should be able to use custom column in a model query (metabase#28193)", () => {
diff --git a/e2e/test/scenarios/models/reproductions/29378-actions-search-crash.cy.spec.js b/e2e/test/scenarios/models/reproductions/29378-actions-search-crash.cy.spec.js
index 5b017ba712c..88400abb1ff 100644
--- a/e2e/test/scenarios/models/reproductions/29378-actions-search-crash.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/29378-actions-search-crash.cy.spec.js
@@ -33,7 +33,7 @@ describe("issue 29378", () => {
});
it("should not crash the model detail page after searching for an action (metabase#29378)", () => {
- cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { dataset: true });
+ cy.request("PUT", `/api/card/${ORDERS_QUESTION_ID}`, { type: "model" });
createAction(ACTION_DETAILS);
cy.visit(`/model/${ORDERS_QUESTION_ID}/detail`);
diff --git a/e2e/test/scenarios/models/reproductions/29517-native-remapped-model-drill-through-click-behavior.cy.spec.js b/e2e/test/scenarios/models/reproductions/29517-native-remapped-model-drill-through-click-behavior.cy.spec.js
index 17986ea8dc0..5099bf186d6 100644
--- a/e2e/test/scenarios/models/reproductions/29517-native-remapped-model-drill-through-click-behavior.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/29517-native-remapped-model-drill-through-click-behavior.cy.spec.js
@@ -10,7 +10,7 @@ import { ORDERS_DASHBOARD_ID } from "e2e/support/cypress_sample_instance_data";
const questionDetails = {
name: "29517",
- dataset: true,
+ type: "model",
native: {
query:
'Select Orders."ID" AS "ID",\nOrders."CREATED_AT" AS "CREATED_AT"\nFrom Orders',
diff --git a/e2e/test/scenarios/models/reproductions/29951-model-editor-results-metadata.cy.spec.js b/e2e/test/scenarios/models/reproductions/29951-model-editor-results-metadata.cy.spec.js
index 185065b7eaa..c789411d71c 100644
--- a/e2e/test/scenarios/models/reproductions/29951-model-editor-results-metadata.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/29951-model-editor-results-metadata.cy.spec.js
@@ -18,7 +18,7 @@ const questionDetails = {
},
limit: 2,
},
- dataset: true,
+ type: "model",
};
describe("issue 29951", { requestTimeout: 10000, viewportWidth: 1600 }, () => {
diff --git a/e2e/test/scenarios/models/reproductions/31309-duplicating-a-model-must-duplicate-question.cy.spec.js b/e2e/test/scenarios/models/reproductions/31309-duplicating-a-model-must-duplicate-question.cy.spec.js
index 8b3de914b33..400ddddc932 100644
--- a/e2e/test/scenarios/models/reproductions/31309-duplicating-a-model-must-duplicate-question.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/31309-duplicating-a-model-must-duplicate-question.cy.spec.js
@@ -39,9 +39,8 @@ describe("issue 31309", () => {
{
name: "model",
query: TEST_QUERY,
- type: "query",
database: SAMPLE_DB_ID,
- dataset: true,
+ type: "model",
},
{
visitQuestion: true,
diff --git a/e2e/test/scenarios/models/reproductions/31663-no-model-fks.cy.spec.js b/e2e/test/scenarios/models/reproductions/31663-no-model-fks.cy.spec.js
index 8ce0dc17832..0bd5fb2dda5 100644
--- a/e2e/test/scenarios/models/reproductions/31663-no-model-fks.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/31663-no-model-fks.cy.spec.js
@@ -18,7 +18,7 @@ describe("issue 31663", () => {
cy.createQuestion(
{
name: "Products Model",
- dataset: true,
+ type: "model",
query: { "source-table": PRODUCTS_ID },
},
{ visitQuestion: true },
diff --git a/e2e/test/scenarios/models/reproductions/31905-duplicated-api-request.cy.spec.js b/e2e/test/scenarios/models/reproductions/31905-duplicated-api-request.cy.spec.js
index 907c3075895..351b32da9c9 100644
--- a/e2e/test/scenarios/models/reproductions/31905-duplicated-api-request.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/31905-duplicated-api-request.cy.spec.js
@@ -13,7 +13,7 @@ describe("issue 31905", () => {
cy.createQuestion(
{
name: "Orders Model",
- dataset: true,
+ type: "model",
query: { "source-table": ORDERS_ID, limit: 2 },
},
{ visitQuestion: true },
diff --git a/e2e/test/scenarios/models/reproductions/32963-model-question-display-lock.cy.spec.js b/e2e/test/scenarios/models/reproductions/32963-model-question-display-lock.cy.spec.js
index d20f04636d8..35668fab3bb 100644
--- a/e2e/test/scenarios/models/reproductions/32963-model-question-display-lock.cy.spec.js
+++ b/e2e/test/scenarios/models/reproductions/32963-model-question-display-lock.cy.spec.js
@@ -18,7 +18,7 @@ describe("issue 32963", () => {
cy.createQuestion(
{
name: "Orders Model",
- dataset: true,
+ type: "model",
query: { "source-table": ORDERS_ID },
},
{ visitQuestion: true },
diff --git a/e2e/test/scenarios/native/data_ref.cy.spec.js b/e2e/test/scenarios/native/data_ref.cy.spec.js
index e0373c20ad0..8ca4dcf0a13 100644
--- a/e2e/test/scenarios/native/data_ref.cy.spec.js
+++ b/e2e/test/scenarios/native/data_ref.cy.spec.js
@@ -46,7 +46,7 @@ describe("scenarios > native question > data reference sidebar", () => {
name: "Native Products Model",
description: "A model of the Products table",
native: { query: "select id as renamed_id from products" },
- dataset: true,
+ type: "model",
},
{ visitQuestion: true },
);
diff --git a/e2e/test/scenarios/native/native_subquery.cy.spec.js b/e2e/test/scenarios/native/native_subquery.cy.spec.js
index b4d722e35e2..01aacbb0435 100644
--- a/e2e/test/scenarios/native/native_subquery.cy.spec.js
+++ b/e2e/test/scenarios/native/native_subquery.cy.spec.js
@@ -26,7 +26,7 @@ describe("scenarios > question > native subquery", () => {
native: {
query: "SELECT id AS another_unique_column_name FROM PEOPLE",
},
- dataset: true,
+ type: "model",
}).then(({ body: { id: questionId2 } }) => {
const tagName1 = `#${questionId1}-a-people-question`;
const queryText = `{{${tagName1}}}`;
@@ -70,7 +70,7 @@ describe("scenarios > question > native subquery", () => {
native: {
query: "SELECT id FROM PEOPLE",
},
- dataset: true,
+ type: "model",
}).then(({ body: { id: questionId2 } }) => {
// Move question 2 to personal collection
cy.visit(`/question/${questionId2}`);
diff --git a/e2e/test/scenarios/onboarding/metabot.cy.spec.js b/e2e/test/scenarios/onboarding/metabot.cy.spec.js
index d3005839e7a..938def5e3a8 100644
--- a/e2e/test/scenarios/onboarding/metabot.cy.spec.js
+++ b/e2e/test/scenarios/onboarding/metabot.cy.spec.js
@@ -27,7 +27,7 @@ const MODEL_DETAILS = {
query: {
"source-table": PRODUCTS_ID,
},
- dataset: true,
+ type: "model",
};
const PROMPT_RESPONSE = {
diff --git a/e2e/test/scenarios/onboarding/search/reproductions/28788-search-results-overflow.cy.spec.js b/e2e/test/scenarios/onboarding/search/reproductions/28788-search-results-overflow.cy.spec.js
index b9cc3bca67c..097ed9f2022 100644
--- a/e2e/test/scenarios/onboarding/search/reproductions/28788-search-results-overflow.cy.spec.js
+++ b/e2e/test/scenarios/onboarding/search/reproductions/28788-search-results-overflow.cy.spec.js
@@ -14,7 +14,7 @@ describe("issue 28788", () => {
it("search results container should not be scrollable horizontally (metabase#28788)", () => {
const questionDetails = {
name: `28788-${LONG_STRING}`,
- dataset: true,
+ type: "model",
description: LONG_STRING,
query: {
"source-table": PEOPLE_ID,
diff --git a/e2e/test/scenarios/onboarding/search/search.cy.spec.js b/e2e/test/scenarios/onboarding/search/search.cy.spec.js
index a019815fcea..8925e176e93 100644
--- a/e2e/test/scenarios/onboarding/search/search.cy.spec.js
+++ b/e2e/test/scenarios/onboarding/search/search.cy.spec.js
@@ -314,7 +314,7 @@ describe("scenarios > search", () => {
cy.createQuestion({
name: "Orders Model",
query: { "source-table": ORDERS_ID },
- dataset: true,
+ type: "model",
}).then(({ body: { id } }) => {
createAction({
name: "Update orders quantity",
@@ -340,7 +340,7 @@ describe("scenarios > search", () => {
{
name: "Products Model",
query: { "source-table": PRODUCTS_ID },
- dataset: true,
+ type: "model",
},
{ wrapId: true, idAlias: "modelId" },
);
diff --git a/e2e/test/scenarios/organization/bookmarks-collection.cy.spec.js b/e2e/test/scenarios/organization/bookmarks-collection.cy.spec.js
index 864f39247d3..0eaeca0d717 100644
--- a/e2e/test/scenarios/organization/bookmarks-collection.cy.spec.js
+++ b/e2e/test/scenarios/organization/bookmarks-collection.cy.spec.js
@@ -100,7 +100,7 @@ describe("scenarios > organization > bookmarks > collection", () => {
cy.createQuestion({
name: "Orders Model",
query: { "source-table": STATIC_ORDERS_ID, aggregation: [["count"]] },
- dataset: true,
+ type: "model",
});
addThenRemoveBookmarkTo("Orders Model");
diff --git a/e2e/test/scenarios/organization/content-verification.cy.spec.js b/e2e/test/scenarios/organization/content-verification.cy.spec.js
index 139d6b1f186..4df687a2645 100644
--- a/e2e/test/scenarios/organization/content-verification.cy.spec.js
+++ b/e2e/test/scenarios/organization/content-verification.cy.spec.js
@@ -45,7 +45,7 @@ describeEE("scenarios > premium > content verification", () => {
cy.log("Turn the question into a model and try again");
cy.request("PUT", `/api/card/${ORDERS_COUNT_QUESTION_ID}`, {
- dataset: true,
+ type: "model",
});
cy.intercept("POST", "/api/dataset").as("dataset");
diff --git a/e2e/test/scenarios/question/new.cy.spec.js b/e2e/test/scenarios/question/new.cy.spec.js
index fbe6efc6b1b..c19802c9089 100644
--- a/e2e/test/scenarios/question/new.cy.spec.js
+++ b/e2e/test/scenarios/question/new.cy.spec.js
@@ -424,7 +424,7 @@ describeOSS(
cy.createQuestion({
name: "Orders Model",
query: { "source-table": ORDERS_ID },
- dataset: true,
+ type: "model",
});
cy.visit("/question/new");
diff --git a/e2e/test/scenarios/question/notebook.cy.spec.js b/e2e/test/scenarios/question/notebook.cy.spec.js
index d04e602c193..0e807372927 100644
--- a/e2e/test/scenarios/question/notebook.cy.spec.js
+++ b/e2e/test/scenarios/question/notebook.cy.spec.js
@@ -465,7 +465,7 @@ describe("scenarios > question > notebook", { tags: "@slow" }, () => {
cy.createQuestion({
name: "Products model",
query: { "source-table": PRODUCTS_ID },
- dataset: true,
+ type: "model",
display: "table",
});
@@ -473,7 +473,7 @@ describe("scenarios > question > notebook", { tags: "@slow" }, () => {
{
name: "Orders model",
query: { "source-table": ORDERS_ID },
- dataset: true,
+ type: "model",
display: "table",
},
{ visitQuestion: true },
@@ -533,7 +533,7 @@ describe("scenarios > question > notebook", { tags: "@slow" }, () => {
() => {
cy.createNativeQuestion({
name: "Orders, Model",
- dataset: true,
+ type: "model",
native: { query: "SELECT * FROM ORDERS" },
});
diff --git a/e2e/test/scenarios/question/reproductions/34414-populate-field-values.cy.spec.js b/e2e/test/scenarios/question/reproductions/34414-populate-field-values.cy.spec.js
index 6bce944a12e..2036125a34f 100644
--- a/e2e/test/scenarios/question/reproductions/34414-populate-field-values.cy.spec.js
+++ b/e2e/test/scenarios/question/reproductions/34414-populate-field-values.cy.spec.js
@@ -13,7 +13,7 @@ const { INVOICES_ID } = SAMPLE_DATABASE;
const INVOICE_MODEL_DETAILS = {
name: "Invoices Model",
query: { "source-table": INVOICES_ID },
- dataset: true,
+ type: "model",
};
describe("issue 34414", () => {
diff --git a/e2e/test/scenarios/sharing/public-sharing.cy.spec.js b/e2e/test/scenarios/sharing/public-sharing.cy.spec.js
index b406a7dde51..9e32c8dfbfb 100644
--- a/e2e/test/scenarios/sharing/public-sharing.cy.spec.js
+++ b/e2e/test/scenarios/sharing/public-sharing.cy.spec.js
@@ -205,7 +205,7 @@ describe("scenarios > admin > settings > public sharing", () => {
query: {
"source-table": ORDERS_ID,
},
- dataset: true,
+ type: "model",
}).then(({ body }) => {
const modelId = body.id;
cy.wrap(modelId).as("modelId");
diff --git a/frontend/src/metabase-lib/Question.ts b/frontend/src/metabase-lib/Question.ts
index bfbe2dbb88d..a80a54af623 100644
--- a/frontend/src/metabase-lib/Question.ts
+++ b/frontend/src/metabase-lib/Question.ts
@@ -24,6 +24,7 @@ import { sortObject } from "metabase-lib/utils";
import type {
Card as CardObject,
+ CardType,
CollectionId,
DatabaseId,
DatasetQuery,
@@ -259,16 +260,30 @@ class Question {
/**
* returns whether this question is a model
- * @returns boolean
+ * @deprecated Use Question.prototype.type instead
*/
- isDataset() {
+ isDataset(): boolean {
return this._card && this._card.dataset;
}
- setDataset(dataset) {
+ type(): CardType | undefined {
+ return this._card && this._card.type;
+ }
+
+ /**
+ * @deprecated Use Question.prototype.setType instead
+ */
+ private _setDataset(dataset: boolean) {
return this.setCard(assoc(this.card(), "dataset", dataset));
}
+ setType(type: CardType) {
+ const dataset = type === "model";
+ // _setDataset is still called for backwards compatibility
+ // as we're migrating "dataset" -> "type" incrementally
+ return this.setCard(assoc(this.card(), "type", type))._setDataset(dataset);
+ }
+
isPersisted() {
return this._card && this._card.persisted;
}
@@ -436,10 +451,6 @@ class Question {
return this.setSettings({ ...this.settings(), ...settings });
}
- type(): string {
- return this.datasetQuery().type;
- }
-
creationType(): string {
return this.card().creationType;
}
diff --git a/frontend/src/metabase-lib/queries/structured/Join.ts b/frontend/src/metabase-lib/queries/structured/Join.ts
index 737df55be7f..38d776afbbf 100644
--- a/frontend/src/metabase-lib/queries/structured/Join.ts
+++ b/frontend/src/metabase-lib/queries/structured/Join.ts
@@ -100,7 +100,7 @@ class Join extends MBQLObjectClause {
? new StructuredQuery(
this.legacyQuery({ useStructuredQuery: true })
.question()
- .setDataset(false),
+ .setType("question"),
{
type: "query",
query: {
@@ -112,7 +112,7 @@ class Join extends MBQLObjectClause {
? new StructuredQuery(
this.legacyQuery({ useStructuredQuery: true })
.question()
- .setDataset(false),
+ .setType("question"),
{
type: "query",
query: sourceQuery,
diff --git a/frontend/src/metabase-types/api/card.ts b/frontend/src/metabase-types/api/card.ts
index de7d302121c..8dafa3052a5 100644
--- a/frontend/src/metabase-types/api/card.ts
+++ b/frontend/src/metabase-types/api/card.ts
@@ -7,12 +7,18 @@ import type { UserInfo } from "./user";
import type { Collection } from "./collection";
import type { SmartScalarComparison } from "./visualization-settings";
+export type CardType = "model" | "question";
+
export interface Card<Q extends DatasetQuery = DatasetQuery>
extends UnsavedCard<Q> {
id: CardId;
name: string;
description: string | null;
+ /**
+ * @deprecated Use "type" instead
+ */
dataset: boolean;
+ type: CardType;
public_uuid: string | null;
/* Indicates whether static embedding for this card has been published */
diff --git a/frontend/src/metabase-types/api/mocks/card.ts b/frontend/src/metabase-types/api/mocks/card.ts
index 1518a5ca290..60edf49f58f 100644
--- a/frontend/src/metabase-types/api/mocks/card.ts
+++ b/frontend/src/metabase-types/api/mocks/card.ts
@@ -25,6 +25,7 @@ export const createMockCard = (opts?: Partial<Card>): Card => ({
visualization_settings: createMockVisualizationSettings(),
result_metadata: [],
dataset: false,
+ type: "question",
can_write: true,
cache_ttl: null,
collection: null,
diff --git a/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/QueryActionContextProvider/QueryActionContextProvider.tsx b/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/QueryActionContextProvider/QueryActionContextProvider.tsx
index d5761cf228d..6ff0d604cac 100644
--- a/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/QueryActionContextProvider/QueryActionContextProvider.tsx
+++ b/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/QueryActionContextProvider/QueryActionContextProvider.tsx
@@ -67,6 +67,7 @@ function convertActionToQuestionCard(
action.visualization_settings as VisualizationSettings,
dataset: false,
+ type: "question",
can_write: true,
public_uuid: null,
collection_id: null,
diff --git a/frontend/src/metabase/entities/questions.js b/frontend/src/metabase/entities/questions.js
index 01eb1b394e8..2f704040132 100644
--- a/frontend/src/metabase/entities/questions.js
+++ b/frontend/src/metabase/entities/questions.js
@@ -128,6 +128,7 @@ const Questions = createEntity({
"name",
"cache_ttl",
"dataset",
+ "type",
"dataset_query",
"display",
"description",
diff --git a/frontend/src/metabase/models/components/ModelDetailLink/ModelDetailLink.tsx b/frontend/src/metabase/models/components/ModelDetailLink/ModelDetailLink.tsx
index 85c79e261e8..bd91080f6f6 100644
--- a/frontend/src/metabase/models/components/ModelDetailLink/ModelDetailLink.tsx
+++ b/frontend/src/metabase/models/components/ModelDetailLink/ModelDetailLink.tsx
@@ -8,10 +8,19 @@ import * as Urls from "metabase/lib/urls";
import type { Card, CollectionItem } from "metabase-types/api";
-type ModelCard = Card & { dataset: true };
+type ModelCard = Card & { dataset: true; type: "model" };
+
+/**
+ * Omitting the "type" attribute is hopefully a temporary workaround
+ * until Metrics v2 are supported in Collections and the ambiguity between
+ * CollectionItem["type"] and Card["type"] disappears.
+ *
+ * @see https://github.com/metabase/metabase/issues/37350#issuecomment-1910284020
+ */
+type ModelCollectionItem = Omit<CollectionItem, "type">;
interface Props extends ButtonProps {
- model: ModelCard | CollectionItem;
+ model: ModelCard | ModelCollectionItem;
}
function ModelDetailLink({ model, ...props }: Props) {
diff --git a/frontend/src/metabase/parameters/utils/mapping-options.unit.spec.js b/frontend/src/metabase/parameters/utils/mapping-options.unit.spec.js
index 7ae4333ad4c..7cc09393c2a 100644
--- a/frontend/src/metabase/parameters/utils/mapping-options.unit.spec.js
+++ b/frontend/src/metabase/parameters/utils/mapping-options.unit.spec.js
@@ -50,7 +50,7 @@ describe("parameters/utils/mapping-options", () => {
const question = ordersTable.question();
dataset = question
.setCard({ ...question.card(), id: 123 })
- .setDataset(true);
+ .setType("model");
// create a virtual table for the card
// that contains fields with custom, model-specific metadata
diff --git a/frontend/src/metabase/query_builder/actions/core/core.js b/frontend/src/metabase/query_builder/actions/core/core.js
index 4461c352314..7ba01e4e09e 100644
--- a/frontend/src/metabase/query_builder/actions/core/core.js
+++ b/frontend/src/metabase/query_builder/actions/core/core.js
@@ -175,7 +175,9 @@ export const navigateToNewCardInsideQB = createThunkAction(
}
// When the dataset query changes, we should loose the dataset flag,
// to start building a new ad-hoc question based on a dataset
- dispatch(setCardAndRun({ ...card, dataset: false }));
+ dispatch(
+ setCardAndRun({ ...card, dataset: false, type: "question" }),
+ );
}
if (objectId !== undefined) {
dispatch(zoomInRow({ objectId }));
@@ -224,7 +226,7 @@ export const apiCreateQuestion = question => {
MetabaseAnalytics.trackStructEvent(
"QueryBuilder",
"Create Card",
- createdQuestion.type(),
+ createdQuestion.datasetQuery().type,
);
trackNewQuestionSaved(
question,
@@ -295,7 +297,7 @@ export const apiUpdateQuestion = (question, { rerunQuery } = {}) => {
MetabaseAnalytics.trackStructEvent(
"QueryBuilder",
"Update Card",
- updatedQuestion.type(),
+ updatedQuestion.datasetQuery().type,
);
await dispatch({
diff --git a/frontend/src/metabase/query_builder/actions/core/updateQuestion.ts b/frontend/src/metabase/query_builder/actions/core/updateQuestion.ts
index 819fc693bf3..dcca58e8684 100644
--- a/frontend/src/metabase/query_builder/actions/core/updateQuestion.ts
+++ b/frontend/src/metabase/query_builder/actions/core/updateQuestion.ts
@@ -127,10 +127,10 @@ export const updateQuestion = (
if (shouldTurnIntoAdHoc) {
newQuestion = newQuestion.withoutNameAndId();
- // When the dataset query changes, we should loose the dataset flag,
+ // When the dataset query changes, we should change the question type,
// to start building a new ad-hoc question based on a dataset
if (newQuestion.isDataset()) {
- newQuestion = newQuestion.setDataset(false);
+ newQuestion = newQuestion.setType("question");
dispatch(onCloseQuestionInfo());
}
}
@@ -139,7 +139,7 @@ export const updateQuestion = (
// so that its query is shown properly in the notebook editor. Various child components of the notebook editor have access to
// this `updateQuestion` action, so they end up triggering the action with the altered question.
if (queryBuilderMode === "dataset" && !newQuestion.isDataset()) {
- newQuestion = newQuestion.setDataset(true);
+ newQuestion = newQuestion.setType("model");
}
const queryResult = getFirstQueryResult(getState());
diff --git a/frontend/src/metabase/query_builder/actions/models.js b/frontend/src/metabase/query_builder/actions/models.js
index c219c1b0847..8020703a163 100644
--- a/frontend/src/metabase/query_builder/actions/models.js
+++ b/frontend/src/metabase/query_builder/actions/models.js
@@ -42,7 +42,7 @@ export const turnQuestionIntoDataset = () => async (dispatch, getState) => {
{
id: question.id(),
},
- question.setDataset(true).setPinned(true).setDisplay("table").card(),
+ question.setType("model").setPinned(true).setDisplay("table").card(),
),
);
@@ -69,7 +69,7 @@ export const turnQuestionIntoDataset = () => async (dispatch, getState) => {
export const turnDatasetIntoQuestion = () => async (dispatch, getState) => {
const dataset = getQuestion(getState());
- const question = dataset.setDataset(false);
+ const question = dataset.setType("question");
await dispatch(apiUpdateQuestion(question, { rerunQuery: true }));
dispatch(
diff --git a/frontend/src/metabase/query_builder/actions/querying.js b/frontend/src/metabase/query_builder/actions/querying.js
index de1a04e218d..2d4b4a507ca 100644
--- a/frontend/src/metabase/query_builder/actions/querying.js
+++ b/frontend/src/metabase/query_builder/actions/querying.js
@@ -132,7 +132,7 @@ export const runQuestionQuery = ({
MetabaseAnalytics.trackStructEvent(
"QueryBuilder",
"Run Query",
- question.type(),
+ question.datasetQuery().type,
duration,
),
);
diff --git a/frontend/src/metabase/query_builder/components/DatasetEditor/DatasetQueryEditor.jsx b/frontend/src/metabase/query_builder/components/DatasetEditor/DatasetQueryEditor.jsx
index cd4bd74619f..180d8c247f3 100644
--- a/frontend/src/metabase/query_builder/components/DatasetEditor/DatasetQueryEditor.jsx
+++ b/frontend/src/metabase/query_builder/components/DatasetEditor/DatasetQueryEditor.jsx
@@ -28,7 +28,7 @@ function DatasetQueryEditor({
}) {
// Datasets/models by default behave like they are already nested,
// so we need to edit the dataset/model question like it is a normal question
- const question = dataset.setDataset(false);
+ const question = dataset.setType("question");
const { isNative } = Lib.queryDisplayInfo(question.query());
const [isResizing, setResizing] = useState(false);
diff --git a/frontend/test/metabase-lib/lib/queries/StructuredQuery-nesting.unit.spec.js b/frontend/test/metabase-lib/lib/queries/StructuredQuery-nesting.unit.spec.js
index 45a51b5a945..38c7cd4cdd2 100644
--- a/frontend/test/metabase-lib/lib/queries/StructuredQuery-nesting.unit.spec.js
+++ b/frontend/test/metabase-lib/lib/queries/StructuredQuery-nesting.unit.spec.js
@@ -139,7 +139,7 @@ describe("StructuredQuery nesting", () => {
const metadata = ordersTable.metadata;
const question = ordersTable.question();
- const dataset = question.setId(1).setDataset(true);
+ const dataset = question.setId(1).setType("model");
const nestedDatasetQuery = dataset
.composeDataset()
.legacyQuery({ useStructuredQuery: true });
diff --git a/resources/migrations/001_update_migrations.yaml b/resources/migrations/001_update_migrations.yaml
index d3e3be7fed6..5a9ed7b612d 100644
--- a/resources/migrations/001_update_migrations.yaml
+++ b/resources/migrations/001_update_migrations.yaml
@@ -5228,6 +5228,42 @@ databaseChangeLog:
tableName: metabase_field
indexName: idx_field_name_lower
+ - changeSet:
+ id: v49.2024-01-22T11:50:00
+ author: qnkhuat
+ comment: Add `report_card.type`
+ changes:
+ - addColumn:
+ tableName: report_card
+ columns:
+ - column:
+ remarks: The type of card, could be 'question', 'model', 'metric'
+ name: type
+ type: varchar(16)
+ defaultValue: "question"
+ constraints:
+ nullable: false
+
+ - changeSet:
+ id: v49.2024-01-22T11:51:00
+ author: qnkhuat
+ comment: Backfill `report_card.type`
+ changes:
+ - sql:
+ sql: >-
+ UPDATE report_card
+ SET type = 'model'
+ WHERE dataset is true
+ rollback: # no need, the column will be dropped
+
+ - changeSet:
+ id: v49.2024-01-22T11:52:00
+ author: qnkhuat
+ comment: Backfill `report_card.type`
+ changes:
+ - customChange:
+ class: "metabase.db.custom_migrations.CardRevisionAddType"
+
- changeSet:
id: v49.2024-01-29T19:26:40
author: adam-james
diff --git a/src/metabase/api/card.clj b/src/metabase/api/card.clj
index 7ae1b5e0466..2b4e9af917e 100644
--- a/src/metabase/api/card.clj
+++ b/src/metabase/api/card.clj
@@ -155,8 +155,8 @@
:parameter_usage_count
[:collection :is_personal]
[:moderation_reviews :moderator_details])
- (cond->
- (:dataset card) (t2/hydrate :persisted)))))
+ (cond-> ; card
+ (card/model? card) (t2/hydrate :persisted)))))
(api/defendpoint GET "/:id"
"Get `Card` with ID."
@@ -384,12 +384,14 @@
;;; ------------------------------------------------- Creating Cards -------------------------------------------------
+
(api/defendpoint POST "/"
"Create a new `Card`."
[:as {{:keys [collection_id collection_position dataset dataset_query description display name
- parameters parameter_mappings result_metadata visualization_settings cache_ttl], :as body} :body}]
+ parameters parameter_mappings result_metadata visualization_settings cache_ttl type], :as body} :body}]
{name ms/NonBlankString
dataset [:maybe :boolean]
+ type [:maybe card/CardTypes]
dataset_query ms/Map
parameters [:maybe [:sequential ms/Parameter]]
parameter_mappings [:maybe [:sequential ms/ParameterMapping]]
@@ -441,13 +443,14 @@
"Update a `Card`."
[id :as {{:keys [dataset_query description display name visualization_settings archived collection_id
collection_position enable_embedding embedding_params result_metadata parameters
- cache_ttl dataset collection_preview]
+ cache_ttl dataset collection_preview type]
:as card-updates} :body}]
{id ms/PositiveInt
name [:maybe ms/NonBlankString]
parameters [:maybe [:sequential ms/Parameter]]
dataset_query [:maybe ms/Map]
dataset [:maybe :boolean]
+ type [:maybe card/CardTypes]
display [:maybe ms/NonBlankString]
description [:maybe :string]
visualization_settings [:maybe ms/Map]
@@ -459,8 +462,11 @@
result_metadata [:maybe qr/ResultsMetadata]
cache_ttl [:maybe ms/PositiveInt]
collection_preview [:maybe :boolean]}
- (let [card-before-update (t2/hydrate (api/write-check Card id)
- [:moderation_reviews :moderator_details])]
+ (let [card-before-update (t2/hydrate (api/write-check Card id)
+ [:moderation_reviews :moderator_details])
+ is-model-after-update? (if (and (nil? type) (nil? dataset))
+ (card/model? card-before-update)
+ (card/model? (card/ensure-type-and-dataset-are-consistent card-updates)))]
;; Do various permissions checks
(doseq [f [collection/check-allowed-to-change-collection
check-allowed-to-modify-query
@@ -471,11 +477,10 @@
:query dataset_query
:metadata result_metadata
:original-metadata (:result_metadata card-before-update)
- :dataset? (if (some? dataset)
- dataset
- (:dataset card-before-update))})
+ :dataset? is-model-after-update?})
card-updates (merge card-updates
- (when dataset
+ (when (and (or (some? type) (some? dataset))
+ is-model-after-update?)
{:display :table}))
metadata-timeout (a/timeout card/metadata-sync-wait-ms)
[fresh-metadata port] (a/alts!! [result-metadata-chan metadata-timeout])
@@ -703,7 +708,7 @@
[card-id]
{card-id ms/PositiveInt}
(premium-features/assert-has-feature :cache-granular-controls (tru "Granular cache controls"))
- (api/let-404 [{:keys [dataset database_id] :as card} (t2/select-one Card :id card-id)]
+ (api/let-404 [{:keys [database_id] :as card} (t2/select-one Card :id card-id)]
(let [database (t2/select-one Database :id database_id)]
(api/write-check database)
(when-not (driver/database-supports? (:engine database)
@@ -716,7 +721,7 @@
(throw (ex-info (tru "Persisting models not enabled for database")
{:status-code 400
:database (:name database)})))
- (when-not dataset
+ (when-not (card/model? card)
(throw (ex-info (tru "Card is not a model") {:status-code 400})))
(when-let [persisted-info (persisted-info/turn-on-model! api/*current-user-id* card)]
(task.persist-refresh/schedule-refresh-for-individual! persisted-info))
@@ -728,7 +733,7 @@
{card-id ms/PositiveInt}
(api/let-404 [card (t2/select-one Card :id card-id)
persisted-info (t2/select-one PersistedInfo :card_id card-id)]
- (when (not (:dataset card))
+ (when (not (card/model? card))
(throw (ex-info (trs "Cannot refresh a non-model question") {:status-code 400})))
(when (:archived card)
(throw (ex-info (trs "Cannot refresh an archived model") {:status-code 400})))
diff --git a/src/metabase/db/custom_migrations.clj b/src/metabase/db/custom_migrations.clj
index 84952d1b0a6..68878dd4bd3 100644
--- a/src/metabase/db/custom_migrations.clj
+++ b/src/metabase/db/custom_migrations.clj
@@ -1033,3 +1033,78 @@
(define-reversible-migration UnifyTimeColumnsType
(unify-time-column-type! :up)
(unify-time-column-type! :down))
+
+(define-reversible-migration CardRevisionAddType
+ (case (mdb.connection/db-type)
+ :postgres
+ (t2/query ["UPDATE revision
+ SET object = jsonb_set(
+ object::jsonb, '{type}',
+ to_jsonb(CASE
+ WHEN (object::jsonb->>'dataset')::boolean THEN 'model'
+ ELSE 'question'
+ END)::jsonb, true)
+ WHERE model = 'Card' AND (object::jsonb->>'dataset') IS NOT NULL;"])
+
+ :mysql
+ (t2/query ["UPDATE revision
+ SET object = JSON_SET(
+ object,
+ '$.type',
+ CASE
+ WHEN JSON_UNQUOTE(JSON_EXTRACT(object, '$.dataset')) = 'true' THEN 'model'
+ ELSE 'question'
+ END)
+ WHERE model = 'Card' AND JSON_UNQUOTE(JSON_EXTRACT(object, '$.dataset')) IS NOT NULL;;"])
+ :h2
+ (let [migrate! (fn [revision]
+ (let [object (json/parse-string (:object revision) keyword)
+ new-object (assoc object :type (if (:dataset object)
+ "model"
+ "question"))]
+ (t2/query {:update :revision
+ :set {:object (json/generate-string new-object)}
+ :where [:= :id (:id revision)]})))]
+ (run! migrate! (t2/reducible-query {:select [:*]
+ :from [:revision]
+ :where [:= :model "Card"]}))))
+ (case (mdb.connection/db-type)
+ :postgres
+ (t2/query ["UPDATE revision
+ SET object = jsonb_set(
+ object::jsonb - 'type',
+ '{dataset}',
+ to_jsonb(CASE
+ WHEN (object::jsonb->>'type') = 'model'
+ THEN true ELSE false
+ END)
+ )
+ WHERE model = 'Card' AND (object::jsonb->>'type') IS NOT NULL;"])
+
+ :mysql
+ (do
+ (t2/query ["UPDATE revision
+ SET object = JSON_SET(
+ object,
+ '$.dataset',
+ CASE
+ WHEN JSON_UNQUOTE(JSON_EXTRACT(object, '$.type')) = 'model'
+ THEN true ELSE false
+ END)
+ WHERE model = 'Card' AND JSON_UNQUOTE(JSON_EXTRACT(object, '$.type')) IS NOT NULL;"])
+ (t2/query ["UPDATE revision
+ SET object = JSON_REMOVE(object, '$.type')
+ WHERE model = 'Card' AND JSON_UNQUOTE(JSON_EXTRACT(object, '$.type')) IS NOT NULL;"]))
+
+ :h2
+ (let [rollback! (fn [revision]
+ (let [object (json/parse-string (:object revision) keyword)
+ new-object (-> object
+ (assoc :dataset (= (:type object) "model"))
+ (dissoc :type))]
+ (t2/query {:update :revision
+ :set {:object (json/generate-string new-object)}
+ :where [:= :id (:id revision)]})))]
+ (run! rollback! (t2/reducible-query {:select [:*]
+ :from [:revision]
+ :where [:= :model "Card"]})))))
diff --git a/src/metabase/models/card.clj b/src/metabase/models/card.clj
index fcdc7a316b0..580a3c1ee6a 100644
--- a/src/metabase/models/card.clj
+++ b/src/metabase/models/card.clj
@@ -44,6 +44,7 @@
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
[metabase.util.log :as log]
+ [metabase.util.malli :as mu]
[methodical.core :as methodical]
[schema.core :as s]
[toucan2.core :as t2]
@@ -101,6 +102,38 @@
([_ pk]
(mi/can-read? (t2/select-one :model/Card :id pk))))
+(def ^{:private true
+ :doc "All acceptable card types.
+ Previously (< 49), we only had 2 card types: question and model, which were differentiated using the
+ boolean `dataset` column. Soon we'll have more card types (e.g: metric) and we will longer be able to use a boolean
+ column to differentiate between all types. So we've added a new `type` column for this purpose.
+
+ Migrating all the code to use `report_card.type` will be quite an effort, we decided that we'll migrate it gradually.
+ In the mean time we'll have both `type` and `dataset` columns."} card-types
+ #{"model" "question"
+ ;; metric will be added as part of epic #37335
+ #_"metric"})
+
+(def CardTypes
+ "Malli schema for acceptable card types."
+ (into [:enum] card-types))
+
+(mu/defn ^:private is-type? :- :boolean
+ "Returns true if card is of type `target-type`"
+ [target-type :- CardTypes
+ {:keys [type] :as _card} :- [:map [:type CardTypes]]]
+ (= target-type type))
+
+(defn question?
+ "Returns true if `card` is a question."
+ [card]
+ (is-type? "question" card))
+
+(defn model?
+ "Returns true if `card` is a model."
+ [card]
+ (is-type? "model" card))
+
;;; -------------------------------------------------- Hydration --------------------------------------------------
(mi/define-simple-hydration-method dashboard-count
@@ -148,7 +181,9 @@
;;; --------------------------------------------------- Revisions ----------------------------------------------------
(def ^:private excluded-columns-for-card-revision
- [:id :created_at :updated_at :entity_id :creator_id :public_uuid :made_public_by_id :metabase_version])
+ [:id :created_at :updated_at :entity_id :creator_id :public_uuid :made_public_by_id :metabase_version
+ ;; we'll use type now
+ :dataset])
(defmethod revision/serialize-instance :model/Card
([instance]
@@ -156,7 +191,8 @@
([_model _id instance]
(cond-> (apply dissoc instance excluded-columns-for-card-revision)
;; datasets should preserve edits to metadata
- (not (:dataset instance))
+ ;; the type check only needed in tests because most test object does not include `type` key
+ (and (some? (:type instance)) (not (model? instance)))
(dissoc :result_metadata))))
;;; --------------------------------------------------- Lifecycle ----------------------------------------------------
@@ -306,15 +342,58 @@
:query-database query-db-id
:field-filter-database field-db-id})))))))
-(defn- assert-valid-model
+(defn- assert-card-type-and-dataset
+ "Throw an exception if card type and dataset contradicts, return the card if it's not."
+ [{:keys [type dataset] :as card}]
+ (if (and (some? type) (some? dataset)
+ (if (true? dataset)
+ (not= "model" type)
+ (= "model" type)))
+ (throw (ex-info (tru ":dataset is inconsistent with :type")
+ {:status-code 400}))
+ card))
+
+(defn ensure-type-and-dataset-are-consistent
+ "We're in the process of migrating from using `report_card.dataset` to `report_card.type`.
+ In the future we'll drop `dataset` and only use `type`. But for now we need to make sure that both keys are aligned
+ when dealing with cards.
+ - If both keys are present, throw an exception if type and dataset is inconsistent.
+ If not we make sure `dataset` is true if `type` is `model` else false.
+ This will make a different when we have `metric` type since a boolean can't represent tri-state
+ - If only one key is present, we'll assoc the correct value for the other key."
+ [{:keys [type dataset] :as card}]
+ (cond
+ ;; if none of the 2 keys is present, do nothing
+ (and (nil? type) (nil? dataset))
+ card
+
+ ;; if both type and dataset is present, makes sure they don't contradict
+ (and (some? type) (some? dataset))
+ (assert-card-type-and-dataset card)
+
+ ;; if only type is present, make sure dataset follows
+ (some? type)
+ (assoc card :dataset (= type "model"))
+
+ ;; if only dataset is present, make sure type follows
+ (some? dataset)
+ (let [inferred-type (if dataset
+ "model"
+ "question")]
+ (log/warnf "Card type not found, defaulting to '%s'" inferred-type)
+ (assoc card :type inferred-type))))
+
+(defn- assert-valid-type
"Check that the card is a valid model if being saved as one. Throw an exception if not."
- [{:keys [dataset dataset_query]}]
- (when dataset
- (let [template-tag-types (->> (vals (get-in dataset_query [:native :template-tags]))
- (map (comp keyword :type)))]
- (when (some (complement #{:card :snippet}) template-tag-types)
- (throw (ex-info (tru "A model made from a native SQL question cannot have a variable or field filter.")
- {:status-code 400}))))))
+ [{:keys [type dataset_query]}]
+ (case type
+ "model" (let [template-tag-types (->> (get-in dataset_query [:native :template-tags])
+ vals
+ (map (comp keyword :type)))]
+ (when (some (complement #{:card :snippet}) template-tag-types)
+ (throw (ex-info (tru "A model made from a native SQL question cannot have a variable or field filter.")
+ {:status-code 400}))))
+ nil))
;; TODO -- consider whether we should validate the Card query when you save/update it??
(defn- pre-insert [card]
@@ -326,7 +405,7 @@
(check-for-circular-source-query-references card)
(check-field-filter-fields-are-from-correct-database card)
;; TODO: add a check to see if all id in :parameter_mappings are in :parameters
- (assert-valid-model card)
+ (assert-valid-type card)
(params/assert-valid-parameters card)
(params/assert-valid-parameter-mappings card)
(collection/check-collection-namespace Card (:collection_id card)))))
@@ -444,7 +523,7 @@
(parameter-card/upsert-or-delete-from-parameters! "card" id (:parameters changes))
;; additional checks (Enterprise Edition only)
(pre-update-check-sandbox-constraints changes)
- (assert-valid-model (merge old-card-info changes)))))
+ (assert-valid-type (merge old-card-info changes)))))
(t2/define-after-select :model/Card
[card]
@@ -453,6 +532,7 @@
(t2/define-before-insert :model/Card
[card]
(-> card
+ ensure-type-and-dataset-are-consistent
(assoc :metabase_version config/mb-version-string)
maybe-normalize-query
populate-result-metadata
@@ -475,6 +555,7 @@
;; We have to convert this to a plain map rather than a Toucan 2 instance at this point to work around upstream bug
;; https://github.com/camsaul/toucan2/issues/145 .
(-> (into {:id (:id card)} (t2/changes card))
+ ensure-type-and-dataset-are-consistent
maybe-normalize-query
populate-result-metadata
pre-update
@@ -594,19 +675,23 @@ saved later when it is ready."
the transaction yet. If you pass true here it is important to call the event after the cards are successfully
created."
([card creator] (create-card! card creator false))
- ([{:keys [dataset_query result_metadata dataset parameters parameter_mappings], :as card-data} creator delay-event?]
- ;; `zipmap` instead of `select-keys` because we want to get `nil` values for keys that aren't present. Required by
- ;; `api/maybe-reconcile-collection-position!`
+ ([{:keys [dataset_query result_metadata dataset parameters parameter_mappings type] :as card-data} creator delay-event?]
+ (assert-card-type-and-dataset card-data)
(let [data-keys [:dataset_query :description :display :name :visualization_settings
- :parameters :parameter_mappings :collection_id :collection_position :cache_ttl]
- card-data (assoc (zipmap data-keys (map card-data data-keys))
- :creator_id (:id creator)
- :dataset (boolean (:dataset card-data))
- :parameters (or parameters [])
- :parameter_mappings (or parameter_mappings []))
+ :parameters :parameter_mappings :collection_id :collection_position :cache_ttl :type :dataset]
+ ;; `zipmap` instead of `select-keys` because we want to get `nil` values for keys that aren't present. Required by
+ ;; `api/maybe-reconcile-collection-position!`
+ card-data (-> (zipmap data-keys (map card-data data-keys))
+ (assoc
+ :creator_id (:id creator)
+ :parameters (or parameters [])
+ :parameter_mappings (or parameter_mappings []))
+ (cond-> (and (nil? type) (nil? dataset))
+ (assoc :type "question"))
+ ensure-type-and-dataset-are-consistent)
result-metadata-chan (result-metadata-async {:query dataset_query
:metadata result_metadata
- :dataset? dataset})
+ :dataset? (model? card-data)})
metadata-timeout (a/timeout metadata-sync-wait-ms)
[metadata port] (a/alts!! [result-metadata-chan metadata-timeout])
timed-out? (= port metadata-timeout)
@@ -614,9 +699,9 @@ saved later when it is ready."
;; Adding a new card at `collection_position` could cause other cards in this
;; collection to change position, check that and fix it if needed
(api/maybe-reconcile-collection-position! card-data)
- (first (t2/insert-returning-instances! Card (cond-> card-data
- (and metadata (not timed-out?))
- (assoc :result_metadata metadata)))))]
+ (t2/insert-returning-instance! Card (cond-> card-data
+ (and metadata (not timed-out?))
+ (assoc :result_metadata metadata))))]
(when-not delay-event?
(events/publish-event! :event/card-create {:object card :user-id (:id creator)}))
(when timed-out?
@@ -757,26 +842,27 @@ saved later when it is ready."
included, otherwise the metadata will be saved to the database asynchronously."
[{:keys [card-before-update card-updates actor]}]
;; don't block our precious core.async thread, run the actual DB updates on a separate thread
- (t2/with-transaction [_conn]
- (api/maybe-reconcile-collection-position! card-before-update card-updates)
-
- (when (and (card-is-verified? card-before-update)
- (changed? card-compare-keys card-before-update card-updates))
- ;; this is an enterprise feature but we don't care if enterprise is enabled here. If there is a review we need
- ;; to remove it regardless if enterprise edition is present at the moment.
- (moderation-review/create-review! {:moderated_item_id (:id card-before-update)
- :moderated_item_type "card"
- :moderator_id (:id actor)
- :status nil
- :text (tru "Unverified due to edit")}))
- ;; ok, now save the Card
- (t2/update! Card (:id card-before-update)
- ;; `collection_id` and `description` can be `nil` (in order to unset them). Other values should only be
- ;; modified if they're passed in as non-nil
- (u/select-keys-when card-updates
- :present #{:collection_id :collection_position :description :cache_ttl :dataset}
- :non-nil #{:dataset_query :display :name :visualization_settings :archived :enable_embedding
- :parameters :parameter_mappings :embedding_params :result_metadata :collection_preview})))
+ (let [card-updates (ensure-type-and-dataset-are-consistent card-updates)]
+ (t2/with-transaction [_conn]
+ (api/maybe-reconcile-collection-position! card-before-update card-updates)
+
+ (when (and (card-is-verified? card-before-update)
+ (changed? card-compare-keys card-before-update card-updates))
+ ;; this is an enterprise feature but we don't care if enterprise is enabled here. If there is a review we need
+ ;; to remove it regardless if enterprise edition is present at the moment.
+ (moderation-review/create-review! {:moderated_item_id (:id card-before-update)
+ :moderated_item_type "card"
+ :moderator_id (:id actor)
+ :status nil
+ :text (tru "Unverified due to edit")}))
+ ;; ok, now save the Card
+ (t2/update! Card (:id card-before-update)
+ ;; `collection_id` and `description` can be `nil` (in order to unset them). Other values should only be
+ ;; modified if they're passed in as non-nil
+ (u/select-keys-when card-updates
+ :present #{:collection_id :collection_position :description :cache_ttl :dataset :type}
+ :non-nil #{:dataset_query :display :name :visualization_settings :archived :enable_embedding
+ :parameters :parameter_mappings :embedding_params :result_metadata :collection_preview}))))
;; Fetch the updated Card from the DB
(let [card (t2/select-one Card :id (:id card-before-update))]
(delete-alerts-if-needed! :old-card card-before-update, :new-card card, :actor actor)
@@ -792,7 +878,7 @@ saved later when it is ready."
(serdes/extract-query-collections Card opts))
(defn- export-result-metadata [card metadata]
- (when (and metadata (:dataset card))
+ (when (and metadata (model? card))
(for [m metadata]
(-> (dissoc m :fingerprint)
(m/update-existing :table_id serdes/*export-table-fk*)
diff --git a/src/metabase/models/revision.clj b/src/metabase/models/revision.clj
index 389727c31ed..95f79810424 100644
--- a/src/metabase/models/revision.clj
+++ b/src/metabase/models/revision.clj
@@ -32,7 +32,7 @@
(defmethod revert-to-revision! :default
[model id _user-id serialized-instance]
- (t2/update! model id, serialized-instance))
+ (t2/update! model id serialized-instance))
(defmulti diff-map
"Return a map describing the difference between `object-1` and `object-2`."
diff --git a/src/metabase/models/revision/diff.clj b/src/metabase/models/revision/diff.clj
index 4d908fa0dce..d7c3043be40 100644
--- a/src/metabase/models/revision/diff.clj
+++ b/src/metabase/models/revision/diff.clj
@@ -76,11 +76,12 @@
[:dataset_query _ _]
(deferred-tru "modified the query")
- [:dataset false true]
- (deferred-tru "turned this into a model")
+ ;; report_card.type
+ [:type "question" "model"]
+ (deferred-tru "turned this to a model")
- [:dataset true false]
- (deferred-tru "changed this from a model to a saved question")
+ [:type old new]
+ (deferred-tru "type changed from {0} to {1}" old new)
[:display _ _]
(deferred-tru "changed the display from {0} to {1}" (name v1) (name v2))
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index 0d88058b1bf..d6859b45084 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -72,6 +72,7 @@
:collection_preview true
:dataset_query {}
:dataset false
+ :type "question"
:description nil
:display "scalar"
:enable_embedding false
@@ -978,6 +979,59 @@
[:trace [:sequential :any]]]
(create-card! :rasta 403)))))))))
+(deftest create-card-with-type-and-dataset-test
+ (mt/with-model-cleanup [:model/Card]
+ (testing "type and dataset must match"
+ (is (= ":dataset is inconsistent with :type"
+ (mt/user-http-request :crowberto :post 400 "card" (assoc (card-with-name-and-query (mt/random-name))
+ :dataset true
+ :type :question))))
+ (is (= ":dataset is inconsistent with :type"
+ (mt/user-http-request :crowberto :post 400 "card" (assoc (card-with-name-and-query (mt/random-name))
+ :dataset false
+ :type "model")))))
+ (testing "can create a model using dataset"
+ (is (=? {:dataset true
+ :type "model"}
+ (mt/user-http-request :crowberto :post 200 "card" (assoc (card-with-name-and-query (mt/random-name))
+ :dataset true)))))
+
+ (testing "can create a model using type"
+ (is (=? {:dataset true
+ :type "model"}
+ (mt/user-http-request :crowberto :post 200 "card" (assoc (card-with-name-and-query (mt/random-name))
+ :type :model)))))
+
+ (testing "default is a question"
+ (is (=? {:dataset false
+ :type "question"}
+ (mt/user-http-request :crowberto :post 200 "card" (card-with-name-and-query (mt/random-name))))))))
+
+(deftest update-card-with-type-and-dataset-test
+ (testing "can toggle model using only type"
+ (mt/with-temp [:model/Card card {:dataset_query {}}]
+ (is (=? {:dataset true
+ :type "model"}
+ (mt/user-http-request :crowberto :put 200 (str "card/" (:id card)) {:type "model"})))
+
+ (is (=? {:dataset false
+ :type "question"}
+ (mt/user-http-request :crowberto :put 200 (str "card/" (:id card)) {:type "question"})))))
+
+ (testing "can toggle model using both type and dataset"
+ (mt/with-temp [:model/Card card {:dataset_query {}}]
+ (is (=? {:dataset true
+ :type "model"}
+ (mt/user-http-request :crowberto :put 200 (str "card/" (:id card)) {:type "model" :dataset true})))
+
+ (is (=? {:dataset false
+ :type "question"}
+ (mt/user-http-request :crowberto :put 200 (str "card/" (:id card)) {:type "question" :dataset false})))
+
+ (testing "but error if type and dataset doesn't match"
+ (is (= ":dataset is inconsistent with :type"
+ (mt/user-http-request :crowberto :put 400 (str "card/" (:id card)) {:type "question" :dataset true})))))))
+
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | COPYING A CARD (POST /api/card/:id/copy) |
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -1250,6 +1304,16 @@
(is (= "A model made from a native SQL question cannot have a variable or field filter."
(mt/user-http-request :rasta :put 400 (format "card/%d" (:id card)) {:dataset true})))))))
+(deftest ^:parallel turn-card-to-model-change-display-test
+ (mt/with-temp [:model/Card card {:display :line}]
+ (is (=? {:display "table"}
+ (mt/user-http-request :crowberto :put 200 (str "card/" (:id card))
+ {:dataset true}))))
+
+ (mt/with-temp [:model/Card card {:display :line}]
+ (is (=? {:display "table"}
+ (mt/user-http-request :crowberto :put 200 (str "card/" (:id card))
+ {:type "model"})))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Updating the positions of stuff |
@@ -2647,7 +2711,7 @@
:dataset_query (mbql-count-query)}]
(is (=? {:display "table" :dataset true}
(mt/user-http-request :crowberto :put 200 (str "card/" (u/the-id card))
- (assoc card :dataset true)))))))
+ (assoc card :dataset true :type "model")))))))
(deftest dataset-card-2
(testing "Cards preserve their edited metadata"
diff --git a/test/metabase/api/revision_test.clj b/test/metabase/api/revision_test.clj
index c87f8aa25ad..54cc8ce5e8e 100644
--- a/test/metabase/api/revision_test.clj
+++ b/test/metabase/api/revision_test.clj
@@ -326,7 +326,7 @@
(create-card-revision! card-id false :crowberto)
;; 2. turn to a model
- (t2/update! Card :id card-id {:dataset true})
+ (t2/update! Card :id card-id {:type "model"})
(create-card-revision! card-id false :crowberto)
;; 3. edit query and metadata
@@ -355,7 +355,7 @@
:has_multiple_changes false}
{:description "changed the display from table to scalar, modified the query and edited the metadata."
:has_multiple_changes true}
- {:description "turned this into a model and edited the metadata."
+ {:description "turned this to a model and edited the metadata."
:has_multiple_changes true}
{:description "renamed this Card from \"A card\" to \"New name\"."
:has_multiple_changes false}
diff --git a/test/metabase/api/search_test.clj b/test/metabase/api/search_test.clj
index 5fe0061607e..824b5931769 100644
--- a/test/metabase/api/search_test.clj
+++ b/test/metabase/api/search_test.clj
@@ -1096,7 +1096,9 @@
:id id
:user-id user-id
:is-creation? true
- :object {:id id}}))
+ :object (merge {:id id}
+ (when (= model :model/Card)
+ {:type "question"}))}))
(testing "Able to filter by last editor"
(let [resp (mt/user-http-request :crowberto :get 200 "search" :q search-term :last_edited_by rasta-user-id)]
@@ -1233,7 +1235,9 @@
:id id
:user-id (mt/user->id :rasta)
:is-creation? true
- :object {:id id}}))
+ :object (merge {:id id}
+ (when (= model :model/Card)
+ {:type "question"}))}))
(testing "returns only applicable models"
(let [resp (mt/user-http-request :crowberto :get 200 "search" :q search-term :last_edited_at "today")]
(is (= #{[action-id "action"]
@@ -1258,7 +1262,9 @@
:id id
:user-id (mt/user->id :rasta)
:is-creation? true
- :object {:id id}}))
+ :object (merge {:id id}
+ (when (= model :model/Card)
+ {:type "question"}))}))
(is (= #{"dashboard" "dataset" "metric" "card"}
(-> (mt/user-http-request :crowberto :get 200 "search" :q search-term :last_edited_at "today" :last_edited_by (mt/user->id :rasta))
:available_models
@@ -1513,14 +1519,14 @@
:id card-id-1
:user-id user-id-1
:is-creation? true
- :object {:id card-id-1}})
+ :object {:id card-id-1 :type "question"}})
(revision/push-revision!
{:entity :model/Card
:id card-id-2
:user-id user-id-2
:is-creation? true
- :object {:id card-id-2}})
+ :object {:id card-id-2 :type "question"}})
(testing "search result should returns creator_common_name and last_editor_common_name"
(is (= #{["card" card-id-1 "Ngoc Khuat" "Ngoc Khuat"]
diff --git a/test/metabase/db/custom_migrations_test.clj b/test/metabase/db/custom_migrations_test.clj
index b56ff1905bb..555fbb7ae04 100644
--- a/test/metabase/db/custom_migrations_test.clj
+++ b/test/metabase/db/custom_migrations_test.clj
@@ -35,6 +35,33 @@
(jobs/defjob AbandonmentEmail [_] :default)
+(defn- table-default [table]
+ (case table
+ :core_user {:first_name (mt/random-name)
+ :last_name (mt/random-name)
+ :email (mt/random-email)
+ :password "superstrong"
+ :date_joined :%now}
+ :metabase_database {:name (mt/random-name)
+ :engine "h2"
+ :details "{}"
+ :created_at :%now
+ :updated_at :%now}
+ :report_card {:name (mt/random-name)
+ :dataset_query "{}"
+ :display "table"
+ :visualization_settings "{}"
+ :created_at :%now
+ :updated_at :%now}
+ :revision {:timestamp :%now}
+ {}))
+
+(defn- new-instance-with-default
+ ([table]
+ (new-instance-with-default table {}))
+ ([table properties]
+ (t2/insert-returning-instance! table (merge (table-default table) properties))))
+
(deftest delete-abandonment-email-task-test
(testing "Migration v46.00-086: Delete the abandonment email task"
(impl/test-migrations ["v46.00-086"] [migrate!]
@@ -1326,13 +1353,15 @@
:updated_at :%now
:details "{}"})
[card-id] (t2/insert-returning-pks!
- :model/Card
+ :report_card
{:visualization_settings card-vis
:display "table"
:dataset_query "{}"
:creator_id user-id
:database_id database-id
- :name "My Card"})
+ :name "My Card"
+ :created_at :%now
+ :updated_at :%now})
[dashboard-id] (t2/insert-returning-pks! :model/Dashboard {:name "My Dashboard"
:creator_id user-id
:parameters []})
@@ -1511,3 +1540,42 @@
(testing "created_at shouldn't change if there is an update"
(is (= (:created_at session)
(t2/select-one-fn :created_at :core_session :id (:id session))))))))))
+
+(deftest card-revision-add-type-test
+ (impl/test-migrations "v49.2024-01-22T11:52:00" [migrate!]
+ (let [user-id (:id (new-instance-with-default :core_user))
+ db-id (:id (new-instance-with-default :metabase_database))
+ card (new-instance-with-default :report_card {:dataset false :creator_id user-id :database_id db-id})
+ model (new-instance-with-default :report_card {:dataset true :creator_id user-id :database_id db-id})
+ card-revision-id (:id (new-instance-with-default :revision
+ {:object (json/generate-string (dissoc card :type))
+ :model "Card"
+ :model_id (:id card)
+ :user_id user-id}))
+ model-revision-id (:id (new-instance-with-default :revision
+ {:object (json/generate-string (dissoc model :type))
+ :model "Card"
+ :model_id (:id card)
+ :user_id user-id}))]
+ (testing "sanity check revision object"
+ (let [card-revision-object (t2/select-one-fn (comp json/parse-string :object) :revision card-revision-id)]
+ (testing "doesn't have type"
+ (is (not (contains? card-revision-object "type"))))
+ (testing "has dataset"
+ (is (contains? card-revision-object "dataset")))))
+
+ (testing "after migration card revisions should have type"
+ (migrate!)
+ (let [card-revision-object (t2/select-one-fn (comp json/parse-string :object) :revision card-revision-id)
+ model-revision-object (t2/select-one-fn (comp json/parse-string :object) :revision model-revision-id)]
+ (is (= "question" (get card-revision-object "type")))
+ (is (= "model" (get model-revision-object "type")))))
+
+ (testing "rollback should remove type and keep dataset"
+ (migrate! :down 48)
+ (let [card-revision-object (t2/select-one-fn (comp json/parse-string :object) :revision card-revision-id)
+ model-revision-object (t2/select-one-fn (comp json/parse-string :object) :revision model-revision-id)]
+ (is (contains? card-revision-object "dataset"))
+ (is (contains? model-revision-object "dataset"))
+ (is (not (contains? card-revision-object "type")))
+ (is (not (contains? model-revision-object "type"))))))))
diff --git a/test/metabase/events/revision_test.clj b/test/metabase/events/revision_test.clj
index a271acf6455..35623717afb 100644
--- a/test/metabase/events/revision_test.clj
+++ b/test/metabase/events/revision_test.clj
@@ -27,7 +27,7 @@
:collection_preview true
:database_id (mt/id)
:dataset_query (:dataset_query card)
- :dataset false
+ :type "question"
:description nil
:display :table
:enable_embedding false
diff --git a/test/metabase/models/card_test.clj b/test/metabase/models/card_test.clj
index ce7ef83dc15..b6d5da9e13b 100644
--- a/test/metabase/models/card_test.clj
+++ b/test/metabase/models/card_test.clj
@@ -760,6 +760,7 @@
(= col :table_id) (mt/id :venues)
(= col :database_id) (:id db)
(= col :query_type) :native
+ (= col :type) "model"
(= col :dataset_query) (mt/mbql-query users)
(= col :visualization_settings) {:text "now it's a text card"}
(int? value) (inc value)
diff --git a/test/metabase/models/revision_test.clj b/test/metabase/models/revision_test.clj
index 998b5b068b5..7c3915b909e 100644
--- a/test/metabase/models/revision_test.clj
+++ b/test/metabase/models/revision_test.clj
@@ -92,12 +92,12 @@
{:name "Spots by State", :private true})))))
(testing "Check that several changes are handled nicely"
- (is (= (str "turned this into a model, made it private and renamed it from \"Tips by State\" to \"Spots by State\".")
+ (is (= "turned this to a model, made it private and renamed it from \"Tips by State\" to \"Spots by State\"."
(u/build-sentence
((get-method revision/diff-strings :default)
Card
- {:name "Tips by State", :private false, :dataset false}
- {:name "Spots by State", :private true, :dataset true}))))))
+ {:name "Tips by State", :private false, :type "question"}
+ {:name "Spots by State", :private true, :type "model"}))))))
(deftest ^:parallel revision-contains-changes-that-has-havent-been-specced-test
(testing "When revision object contains key that we don't know how to generate diff-string
@@ -416,24 +416,24 @@
{:description "changed the display from table to bar and turned this into a model."
:has_multiple_changes true}
(#'revision/revision-description-info model
- {:object {:dataset false
+ {:object {:type "question"
:display :table}
:is_reversion false
:is_creation false}
- {:object {:dataset true
+ {:object {:type "model"
:display :bar}
:is_reversion false
:is_creation false}))
(testing "changes contains unspecified keys will not be mentioned"
- (is (= {:description "turned this into a model."
+ (is (= {:description "turned this to a model."
:has_multiple_changes false}
(#'revision/revision-description-info model
- {:object {:dataset false
+ {:object {:type "question"
:unknown_key false}
:is_reversion false
:is_creation false}
- {:object {:dataset true
+ {:object {:type "model"
:unknown_key false}
:is_reversion false
:is_creation false})))))))))
--
GitLab