From 0bd20d0b3c6a5121c0124ff8bb3424facd1ddf35 Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Fri, 3 Jun 2022 14:50:59 +0700
Subject: [PATCH] Add `parameters` to report_card (#22976)
* add parameters to report_card and card APIs
* make sure migration can work with existing cards and add migration tests
* remove debug code
* When parameters is empty, return template tag parameters
* update schema message
* update migraiton test name
* minor changes in test
* fix failing tests
* Always use card.parameters in PublicQuestion
* Add defaults and readd isEmpty check
* Default to undefined/use template tags in PublicQuestion...
* parameters should be in the writableProperties
* Native query card: construct parameters from template tags
* Separate the generation of parameter id
* Add parameter_mappings to report_card (#23003)
* add parameter_mappings to report_card
* fix from Noah's comments
* fix from Noah's comments
* Update a parameter from an updated template tag
* Correct the parameters construction
* Also add `parameter_mappings` to writableProperties
* CI test: bust the npm module cache
* Revert "CI test: bust the npm module cache"
This reverts commit 5a327b616f0220f43a90f7f871e0bd877ffa6f47.
Co-authored-by: Dalton Johnson <daltojohnso@users.noreply.github.com>
Co-authored-by: Ariya Hidayat <ariya@metabase.com>
---
.../serialization/cmd_test.clj | 2 +
frontend/src/metabase/entities/questions.js | 2 +
.../src/metabase/parameters/utils/cards.ts | 41 ++++++-----
.../metabase/parameters/utils/dashboards.ts | 3 +-
.../metabase/parameters/utils/parameter-id.js | 4 ++
.../utils/parameter-id.unit.spec.js | 7 ++
.../public/containers/PublicQuestion.jsx | 14 +++-
.../metabase/query_builder/actions/native.js | 30 +++++++-
resources/migrations/000_migrations.yaml | 55 +++++++++++++++
src/metabase/api/card.clj | 19 +++--
src/metabase/api/dashboard.clj | 4 +-
src/metabase/models/card.clj | 25 +++++--
src/metabase/models/dashboard.clj | 9 +--
src/metabase/models/params.clj | 14 ++++
src/metabase/util/schema.clj | 12 ++++
test/metabase/api/card_test.clj | 65 ++++++++++++++++-
test/metabase/api/dashboard_test.clj | 3 +-
test/metabase/db/schema_migrations_test.clj | 51 +++++++++++++-
test/metabase/events/revision_test.clj | 2 +
test/metabase/models/card_test.clj | 69 +++++++++++++++++++
test/metabase/models/dashboard_test.clj | 4 +-
21 files changed, 383 insertions(+), 52 deletions(-)
create mode 100644 frontend/src/metabase/parameters/utils/parameter-id.js
create mode 100644 frontend/src/metabase/parameters/utils/parameter-id.unit.spec.js
diff --git a/enterprise/backend/test/metabase_enterprise/serialization/cmd_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/cmd_test.clj
index 4eacbc75f4e..6089512e1ba 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/cmd_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/cmd_test.clj
@@ -69,6 +69,8 @@
:dataset_query "{}"
:creator_id (u/the-id user)
:visualization_settings "{}"
+ :parameters "[]"
+ :parameter_mappings "[]"
:created_at :%now
:updated_at :%now)
;; serialize "everything" (which should just be the card and user), which should succeed if #16931 is fixed
diff --git a/frontend/src/metabase/entities/questions.js b/frontend/src/metabase/entities/questions.js
index dc9de872d16..df6c909604c 100644
--- a/frontend/src/metabase/entities/questions.js
+++ b/frontend/src/metabase/entities/questions.js
@@ -79,6 +79,8 @@ const Questions = createEntity({
"display",
"description",
"visualization_settings",
+ "parameters",
+ "parameter_mappings",
"archived",
"enable_embedding",
"embedding_params",
diff --git a/frontend/src/metabase/parameters/utils/cards.ts b/frontend/src/metabase/parameters/utils/cards.ts
index 265bed63523..2bfbf5acc27 100644
--- a/frontend/src/metabase/parameters/utils/cards.ts
+++ b/frontend/src/metabase/parameters/utils/cards.ts
@@ -30,7 +30,24 @@ function getTemplateTagType(tag: TemplateTag) {
}
}
+export function getTemplateTagParameter(tag: TemplateTag): ParameterWithTarget {
+ const target: ParameterTarget =
+ tag.type === "dimension"
+ ? ["dimension", ["template-tag", tag.name]]
+ : ["variable", ["template-tag", tag.name]];
+
+ return {
+ id: tag.id,
+ type: tag["widget-type"] || getTemplateTagType(tag),
+ target,
+ name: tag["display-name"],
+ slug: tag.name,
+ default: tag.default,
+ };
+}
+
// NOTE: this should mirror `template-tag-parameters` in src/metabase/api/embed.clj
+
export function getTemplateTagParameters(
tags: TemplateTag[],
): ParameterWithTarget[] {
@@ -39,21 +56,7 @@ export function getTemplateTagParameters(
tag =>
tag.type != null && (tag["widget-type"] || tag.type !== "dimension"),
)
- .map(tag => {
- const target: ParameterTarget =
- tag.type === "dimension"
- ? ["dimension", ["template-tag", tag.name]]
- : ["variable", ["template-tag", tag.name]];
-
- return {
- id: tag.id,
- type: tag["widget-type"] || getTemplateTagType(tag),
- target,
- name: tag["display-name"],
- slug: tag.name,
- default: tag.default,
- };
- });
+ .map(getTemplateTagParameter);
}
export function getTemplateTagsForParameters(card: Card) {
@@ -74,7 +77,11 @@ export function getTemplateTagsForParameters(card: Card) {
export function getParametersFromCard(
card: Card,
): Parameter[] | ParameterWithTarget[] {
- if (card && card.parameters) {
+ if (!card) {
+ return [];
+ }
+
+ if (card.parameters && !_.isEmpty(card.parameters)) {
return card.parameters;
}
@@ -86,12 +93,12 @@ export function getValueAndFieldIdPopulatedParametersFromCard(
card: Card,
metadata: Metadata,
parameterValues: { [key: string]: any },
+ parameters = getParametersFromCard(card),
) {
if (!card) {
return [];
}
- const parameters = getParametersFromCard(card);
const valuePopulatedParameters: (Parameter[] | ParameterWithTarget[]) & {
value?: any;
} = getValuePopulatedParameters(parameters, parameterValues);
diff --git a/frontend/src/metabase/parameters/utils/dashboards.ts b/frontend/src/metabase/parameters/utils/dashboards.ts
index 1667118647f..2f8b4ab783e 100644
--- a/frontend/src/metabase/parameters/utils/dashboards.ts
+++ b/frontend/src/metabase/parameters/utils/dashboards.ts
@@ -2,6 +2,7 @@ import _ from "underscore";
import { setIn } from "icepick";
import Question from "metabase-lib/lib/Question";
+import { generateParameterId } from "metabase/parameters/utils/parameter-id";
import { getParameterTargetField } from "metabase/parameters/utils/targets";
import { slugify } from "metabase/lib/formatting";
import {
@@ -59,7 +60,7 @@ export function createParameter(
const parameter = {
name: "",
slug: "",
- id: Math.floor(Math.random() * Math.pow(2, 32)).toString(16),
+ id: generateParameterId(),
type: option.type,
sectionId: option.sectionId,
};
diff --git a/frontend/src/metabase/parameters/utils/parameter-id.js b/frontend/src/metabase/parameters/utils/parameter-id.js
new file mode 100644
index 00000000000..2071d30a566
--- /dev/null
+++ b/frontend/src/metabase/parameters/utils/parameter-id.js
@@ -0,0 +1,4 @@
+export function generateParameterId() {
+ const num = Math.floor(Math.random() * Math.pow(2, 32));
+ return num.toString(16);
+}
diff --git a/frontend/src/metabase/parameters/utils/parameter-id.unit.spec.js b/frontend/src/metabase/parameters/utils/parameter-id.unit.spec.js
new file mode 100644
index 00000000000..1298d4aadff
--- /dev/null
+++ b/frontend/src/metabase/parameters/utils/parameter-id.unit.spec.js
@@ -0,0 +1,7 @@
+import { generateParameterId } from "./parameter-id";
+
+describe("parameters/utils/parameter-id", () => {
+ it("should generate a random parameter id", () => {
+ expect(generateParameterId().length).toBeGreaterThan(1);
+ });
+});
diff --git a/frontend/src/metabase/public/containers/PublicQuestion.jsx b/frontend/src/metabase/public/containers/PublicQuestion.jsx
index 77a6ef252a4..129bdaac12e 100644
--- a/frontend/src/metabase/public/containers/PublicQuestion.jsx
+++ b/frontend/src/metabase/public/containers/PublicQuestion.jsx
@@ -16,8 +16,8 @@ import {
} from "metabase/parameters/utils/parameter-values";
import { applyParameters } from "metabase/meta/Card";
import {
- getParametersFromCard,
getValueAndFieldIdPopulatedParametersFromCard,
+ getParametersFromCard,
} from "metabase/parameters/utils/cards";
import {
@@ -91,6 +91,8 @@ class PublicQuestion extends Component {
const parameters = getValueAndFieldIdPopulatedParametersFromCard(
card,
metadata,
+ {},
+ card.parameters || undefined,
);
const parameterValuesById = getParameterValuesByIdFromQueryParams(
parameters,
@@ -134,7 +136,7 @@ class PublicQuestion extends Component {
return;
}
- const parameters = getParametersFromCard(card);
+ const parameters = card.parameters || getParametersFromCard(card);
try {
this.setState({ result: null });
@@ -187,7 +189,13 @@ class PublicQuestion extends Component {
);
const parameters =
- card && getValueAndFieldIdPopulatedParametersFromCard(card, metadata);
+ card &&
+ getValueAndFieldIdPopulatedParametersFromCard(
+ card,
+ metadata,
+ {},
+ card.parameters || undefined,
+ );
return (
<EmbedFrame
diff --git a/frontend/src/metabase/query_builder/actions/native.js b/frontend/src/metabase/query_builder/actions/native.js
index 823eedf1ad2..ab456bdd6ee 100644
--- a/frontend/src/metabase/query_builder/actions/native.js
+++ b/frontend/src/metabase/query_builder/actions/native.js
@@ -1,5 +1,5 @@
import _ from "underscore";
-import { updateIn } from "icepick";
+import { assoc, updateIn } from "icepick";
import { createAction } from "redux-actions";
@@ -17,6 +17,12 @@ import {
import { updateQuestion } from "./core";
import { SET_UI_CONTROLS } from "./ui";
+import {
+ getTemplateTagsForParameters,
+ getTemplateTagParameters,
+ getTemplateTagParameter,
+} from "metabase/parameters/utils/cards";
+
export const TOGGLE_DATA_REFERENCE = "metabase/qb/TOGGLE_DATA_REFERENCE";
export const toggleDataReference = createAction(TOGGLE_DATA_REFERENCE, () => {
MetabaseAnalytics.trackStructEvent("QueryBuilder", "Toggle Data Reference");
@@ -124,7 +130,7 @@ export const setTemplateTag = createThunkAction(
}
// we need to preserve the order of the keys to avoid UI jumps
- return updateIn(
+ const updatedTagsCard = updateIn(
updatedCard,
["dataset_query", "native", "template-tags"],
tags => {
@@ -137,6 +143,26 @@ export const setTemplateTag = createThunkAction(
return { ...tags, [name]: newTag };
},
);
+
+ const { parameters } = updatedTagsCard;
+ if (parameters && Array.isArray(parameters)) {
+ if (parameters.length === 0) {
+ // reconstruct from the existing template tags
+ const tags = getTemplateTagsForParameters(updatedTagsCard);
+ const newParameters = getTemplateTagParameters(tags);
+ return assoc(updatedTagsCard, "parameters", newParameters);
+ } else {
+ // update an existing parameter
+ const index = parameters.findIndex(p => p.id === templateTag.id);
+ if (index < 0) {
+ console.warn(`Can't find parameter with id=${templateTag.id}!`);
+ } else {
+ parameters[index] = getTemplateTagParameter(templateTag);
+ }
+ }
+ }
+
+ return updatedTagsCard;
};
},
);
diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index d25efa4df8c..d16360ece2b 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/resources/migrations/000_migrations.yaml
@@ -11791,6 +11791,61 @@ databaseChangeLog:
nullable: true
unique: true
tableName: report_dashboardcard
+ - changeSet:
+ id: v44.00-023
+ author: qnkhuat
+ comment: Added 0.44.0 - Add parameters to report_card
+ changes:
+ - addColumn:
+ tableName: report_card
+ columns:
+ - column:
+ name: parameters
+ type: ${text.type}
+ remarks: List of parameter associated to a card
+ constraints:
+ nullable: true
+ deferrable: false
+ initiallyDeferred: false
+ - changeSet:
+ id: v44.00-024
+ author: qnkhuat
+ comment: Added 0.44.0 - Add parameters to report_card
+ changes:
+ - addNotNullConstraint:
+ columnDataType: ${text.type}
+ columnName: parameters
+ defaultNullValue: '[]'
+ tableName: report_card
+
+ - changeSet:
+ id: v44.00-025
+ author: qnkhuat
+ comment: Added 0.44.0 - Add parameter_mappings to report_card
+ changes:
+ - addColumn:
+ tableName: report_card
+ columns:
+ - column:
+ name: parameter_mappings
+ type: ${text.type}
+ remarks: List of parameter associated to a card
+ constraints:
+ nullable: true
+ deferrable: false
+ initiallyDeferred: false
+ - changeSet:
+ id: v44.00-026
+ author: qnkhuat
+ comment: Added 0.44.0 - Add parameter_mappings to report_card
+ changes:
+ - addNotNullConstraint:
+ columnDataType: ${text.type}
+ columnName: parameter_mappings
+ defaultNullValue: '[]'
+ tableName: report_card
+
+
# >>>>>>>>>> DO NOT ADD NEW MIGRATIONS BELOW THIS LINE! ADD THEM ABOVE <<<<<<<<<<
diff --git a/src/metabase/api/card.clj b/src/metabase/api/card.clj
index d8a78843748..608ec898ccd 100644
--- a/src/metabase/api/card.clj
+++ b/src/metabase/api/card.clj
@@ -270,14 +270,16 @@
(defn- create-card-async!
"Create a new Card asynchronously. Returns a channel for fetching the newly created Card, or an Exception if one was
thrown. Closing this channel before it finishes will cancel the Card creation."
- [{:keys [dataset_query result_metadata dataset], :as card-data}]
+ [{:keys [dataset_query result_metadata dataset parameters parameter_mappings], :as card-data}]
;; `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!`
- (let [data-keys [:dataset_query :description :display :name
- :visualization_settings :collection_id :collection_position :cache_ttl]
+ (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 api/*current-user-id*
- :dataset (boolean (:dataset card-data)))
+ :dataset (boolean (:dataset card-data))
+ :parameters (or parameters [])
+ :parameter_mappings (or parameter_mappings []))
result-metadata-chan (result-metadata-async {:query dataset_query
:metadata result_metadata
:dataset? dataset})
@@ -298,8 +300,10 @@
(api/defendpoint ^:returns-chan POST "/"
"Create a new `Card`."
[:as {{:keys [collection_id collection_position dataset_query description display name
- result_metadata visualization_settings cache_ttl], :as body} :body}]
+ parameters parameter_mappings result_metadata visualization_settings cache_ttl], :as body} :body}]
{name su/NonBlankString
+ parameters (s/maybe [su/Parameter])
+ parameter_mappings (s/maybe [su/ParameterMapping])
description (s/maybe su/NonBlankString)
display su/NonBlankString
visualization_settings su/Map
@@ -502,7 +506,7 @@
(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
- :embedding_params :result_metadata})))
+ :parameters :parameter_mappings :embedding_params :result_metadata})))
;; Fetch the updated Card from the DB
(let [card (Card id)]
(delete-alerts-if-needed! card-before-update card)
@@ -521,10 +525,11 @@
(api/defendpoint ^:returns-chan PUT "/:id"
"Update a `Card`."
[id :as {{:keys [dataset_query description display name visualization_settings archived collection_id
- collection_position enable_embedding embedding_params result_metadata
+ collection_position enable_embedding embedding_params result_metadata parameters
cache_ttl dataset]
:as card-updates} :body}]
{name (s/maybe su/NonBlankString)
+ parameters (s/maybe [su/Parameter])
dataset_query (s/maybe su/Map)
dataset (s/maybe s/Bool)
display (s/maybe su/NonBlankString)
diff --git a/src/metabase/api/dashboard.clj b/src/metabase/api/dashboard.clj
index 4497e465e4f..d557b3c93af 100644
--- a/src/metabase/api/dashboard.clj
+++ b/src/metabase/api/dashboard.clj
@@ -69,7 +69,7 @@
"Create a new Dashboard."
[:as {{:keys [name description parameters cache_ttl collection_id collection_position], :as _dashboard} :body}]
{name su/NonBlankString
- parameters [su/Map]
+ parameters (s/maybe [su/Parameter])
description (s/maybe s/Str)
cache_ttl (s/maybe su/IntGreaterThanZero)
collection_id (s/maybe su/IntGreaterThanZero)
@@ -275,7 +275,7 @@
show_in_getting_started (s/maybe s/Bool)
enable_embedding (s/maybe s/Bool)
embedding_params (s/maybe su/EmbeddingParams)
- parameters (s/maybe [su/Map])
+ parameters (s/maybe [su/Parameter])
position (s/maybe su/IntGreaterThanZero)
archived (s/maybe s/Bool)
collection_id (s/maybe su/IntGreaterThanZero)
diff --git a/src/metabase/models/card.clj b/src/metabase/models/card.clj
index 2a9b7924668..40c57d6a88a 100644
--- a/src/metabase/models/card.clj
+++ b/src/metabase/models/card.clj
@@ -210,11 +210,17 @@
;; TODO -- consider whether we should validate the Card query when you save/update it??
(defn- pre-insert [card]
- (u/prog1 card
- ;; make sure this Card doesn't have circular source query references
- (check-for-circular-source-query-references card)
- (check-field-filter-fields-are-from-correct-database card)
- (collection/check-collection-namespace Card (:collection_id card))))
+ (let [defaults {:parameters []
+ :parameter_mappings []}
+ card (merge defaults card)]
+ (u/prog1 card
+ ;; make sure this Card doesn't have circular source query references
+ (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
+ (params/assert-valid-parameters card)
+ (params/assert-valid-parameter-mappings card)
+ (collection/check-collection-namespace Card (:collection_id card)))))
(defn- post-insert [card]
;; if this Card has any native template tag parameters we need to update FieldValues for any Fields that are
@@ -231,7 +237,8 @@
For the OSS edition, there is no implementation for this function -- it is a no-op. For Metabase Enterprise Edition,
the implementation of this function is
[[metabase-enterprise.sandbox.models.group-table-access-policy/update-card-check-gtaps]] and is installed by that
- namespace."} pre-update-check-sandbox-constraints
+ namespace."}
+ pre-update-check-sandbox-constraints
(atom identity))
(defn- pre-update [{archived? :archived, id :id, :as changes}]
@@ -261,6 +268,8 @@
(check-field-filter-fields-are-from-correct-database changes)
;; Make sure the Collection is in the default Collection namespace (e.g. as opposed to the Snippets Collection namespace)
(collection/check-collection-namespace Card (:collection_id changes))
+ (params/assert-valid-parameters changes)
+ (params/assert-valid-parameter-mappings changes)
;; additional checks (Enterprise Edition only)
(@pre-update-check-sandbox-constraints changes)))
@@ -289,7 +298,9 @@
:embedding_params :json
:query_type :keyword
:result_metadata ::result-metadata
- :visualization_settings :visualization-settings})
+ :visualization_settings :visualization-settings
+ :parameters :parameters-list
+ :parameter_mappings :parameters-list})
:properties (constantly {:timestamped? true
:entity_id true})
;; Make sure we normalize the query before calling `pre-update` or `pre-insert` because some of the
diff --git a/src/metabase/models/dashboard.clj b/src/metabase/models/dashboard.clj
index 0580b3c7a3b..20cbfd7537f 100644
--- a/src/metabase/models/dashboard.clj
+++ b/src/metabase/models/dashboard.clj
@@ -64,11 +64,6 @@
(models/defmodel Dashboard :report_dashboard)
;;; ----------------------------------------------- Entity & Lifecycle -----------------------------------------------
-(defn- assert-valid-parameters [{:keys [parameters]}]
- (when (s/check (s/maybe [{:id su/NonBlankString, s/Keyword s/Any}]) parameters)
- (throw (ex-info (tru ":parameters must be a sequence of maps with String :id keys")
- {:parameters parameters}))))
-
(defn- pre-delete [dashboard]
(db/delete! 'Revision :model "Dashboard" :model_id (u/the-id dashboard)))
@@ -76,12 +71,12 @@
(let [defaults {:parameters []}
dashboard (merge defaults dashboard)]
(u/prog1 dashboard
- (assert-valid-parameters dashboard)
+ (params/assert-valid-parameters dashboard)
(collection/check-collection-namespace Dashboard (:collection_id dashboard)))))
(defn- pre-update [dashboard]
(u/prog1 dashboard
- (assert-valid-parameters dashboard)
+ (params/assert-valid-parameters dashboard)
(collection/check-collection-namespace Dashboard (:collection_id dashboard))))
(defn- update-dashboard-subscription-pulses!
diff --git a/src/metabase/models/params.clj b/src/metabase/models/params.clj
index fb303a8ab20..a23c504852c 100644
--- a/src/metabase/models/params.clj
+++ b/src/metabase/models/params.clj
@@ -17,6 +17,20 @@
;;; | SHARED |
;;; +----------------------------------------------------------------------------------------------------------------+
+(defn assert-valid-parameters
+ "Receive a Paremeterized Object and check if its parameters is valid."
+ [{:keys [parameters]}]
+ (when (s/check (s/maybe [su/Parameter]) parameters)
+ (throw (ex-info (tru ":parameters must be a sequence of maps with String :id key")
+ {:parameters parameters}))))
+
+(defn assert-valid-parameter-mappings
+ "Receive a Paremeterized Object and check if its parameters is valid."
+ [{:keys [parameter_mappings]}]
+ (when (s/check (s/maybe [su/ParameterMapping]) parameter_mappings)
+ (throw (ex-info (tru ":parameter_mappings must be a sequence of maps with String :parameter_id key")
+ {:parameter_mappings parameter_mappings}))))
+
(s/defn unwrap-field-clause :- mbql.s/field
"Unwrap something that contains a `:field` clause, such as a template tag, Also handles unwrapped integers for
legacy compatibility.
diff --git a/src/metabase/util/schema.clj b/src/metabase/util/schema.clj
index 0b2d709b947..6ad80d196ab 100644
--- a/src/metabase/util/schema.clj
+++ b/src/metabase/util/schema.clj
@@ -327,6 +327,18 @@
false)))
(deferred-tru "value must be a valid JSON string.")))
+(def Parameter
+ "Schema for a valid Parameter."
+ (with-api-error-message {:id NonBlankString
+ s/Keyword s/Any}
+ (deferred-tru "parameter must be a map with String :id key")))
+
+(def ParameterMapping
+ "Schema for a valid Parameter Mapping"
+ (with-api-error-message {:parameter_id NonBlankString
+ s/Keyword s/Any}
+ (deferred-tru "parameter mapping must be a String :parameter_id key")))
+
(def EmbeddingParams
"Schema for a valid map of embedding params."
(with-api-error-message (s/maybe {s/Keyword (s/enum "disabled" "enabled" "locked")})
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index 2bb567d2e7e..2f515fb4e21 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -43,6 +43,7 @@
(-> (mt/run-mbql-query venues {:aggregation [[:count]]}) :data :cols first :base_type))
(def card-defaults
+ "The default card params."
{:archived false
:collection_id nil
:collection_position nil
@@ -54,6 +55,8 @@
:entity_id nil
:embedding_params nil
:made_public_by_id nil
+ :parameters []
+ :parameter_mappings []
:moderation_reviews ()
:public_uuid nil
:query_type nil
@@ -307,13 +310,19 @@
(mt/with-model-cleanup [Card]
(let [card (assoc (card-with-name-and-query (mt/random-name)
(mbql-count-query (mt/id) (mt/id :venues)))
- :collection_id (u/the-id collection))]
+ :collection_id (u/the-id collection)
+ :parameters [{:id "abc123", :name "test", :type "date"}]
+ :parameter_mappings [{:parameter_id "abc123", :card_id "10",
+ :target [:dimension [:template-tags "category"]]}])]
(is (= (merge
card-defaults
{:name (:name card)
:collection_id true
:collection true
:creator_id (mt/user->id :rasta)
+ :parameters [{:id "abc123", :name "test", :type "date"}]
+ :parameter_mappings [{:parameter_id "abc123", :card_id "10",
+ :target ["dimension" ["template-tags" "category"]]}]
:dataset_query true
:query_type "query"
:visualization_settings {:global {:title nil}}
@@ -347,6 +356,16 @@
(update :id boolean)
(update :timestamp boolean))))))))))))))
+(deftest create-card-validation-test
+ (testing "POST /api/card"
+ (is (= {:errors {:visualization_settings "value must be a map."}}
+ (mt/user-http-request :crowberto :post 400 "card" {:visualization_settings "ABC"})))
+
+ (is (= {:errors {:parameters (str "value may be nil, or if non-nil, value must be an array. "
+ "Each parameter must be a map with String :id key")}}
+ (mt/user-http-request :crowberto :post 400 "card" {:visualization_settings {:global {:title nil}}
+ :parameters "abc"})))))
+
(deftest save-empty-card-test
(testing "POST /api/card"
(testing "Should be able to save an empty Card"
@@ -757,6 +776,50 @@
(is (= ""
(db/select-one-field :description Card :id (u/the-id card)))))))
+(deftest update-card-parameters-test
+ (testing "PUT /api/card/:id"
+ (mt/with-temp Card [card]
+ (testing "successfully update with valid parameters"
+ (is (partial= {:parameters [{:id "random-id"
+ :type "number"}]}
+ (mt/user-http-request :rasta :put 202 (str "card/" (u/the-id card))
+ {:parameters [{:id "random-id"
+ :type "number"}]})))))
+
+ (mt/with-temp Card [card {:parameters [{:id "random-id"
+ :type "number"}]}]
+ (testing "nil parameters will no-op"
+ (is (partial= {:parameters [{:id "random-id"
+ :type "number"}]}
+ (mt/user-http-request :rasta :put 202 (str "card/" (u/the-id card))
+ {:parameters nil}))))
+ (testing "an empty list will remove parameters"
+ (is (partial= {:parameters []}
+ (mt/user-http-request :rasta :put 202 (str "card/" (u/the-id card))
+ {:parameters []})))))))
+
+(deftest update-card-parameter-mappings-test
+ (testing "PUT /api/card/:id"
+ (mt/with-temp Card [card]
+ (testing "successfully update with valid parameter_mappings"
+ (is (partial= {:parameter_mappings [{:parameter_id "abc123", :card_id "10",
+ :target ["dimension" ["template-tags" "category"]]}]}
+ (mt/user-http-request :rasta :put 202 (str "card/" (u/the-id card))
+ {:parameter_mappings [{:parameter_id "abc123", :card_id "10",
+ :target ["dimension" ["template-tags" "category"]]}]})))))
+
+ (mt/with-temp Card [card {:parameter_mappings [{:parameter_id "abc123", :card_id "10",
+ :target ["dimension" ["template-tags" "category"]]}]}]
+ (testing "nil parameters will no-op"
+ (is (partial= {:parameter_mappings [{:parameter_id "abc123", :card_id "10",
+ :target ["dimension" ["template-tags" "category"]]}]}
+ (mt/user-http-request :rasta :put 202 (str "card/" (u/the-id card))
+ {:parameters nil}))))
+ (testing "an empty list will remove parameter_mappings"
+ (is (partial= {:parameter_mappings []}
+ (mt/user-http-request :rasta :put 202 (str "card/" (u/the-id card))
+ {:parameter_mappings []})))))))
+
(deftest update-embedding-params-test
(testing "PUT /api/card/:id"
(mt/with-temp Card [card]
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index 282068be20e..b3f1e362d6a 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -113,7 +113,8 @@
(is (= {:errors {:name "value must be a non-blank string."}}
(mt/user-http-request :rasta :post 400 "dashboard" {})))
- (is (= {:errors {:parameters "value must be an array. Each value must be a map."}}
+ (is (= {:errors {:parameters (str "value may be nil, or if non-nil, value must be an array. "
+ "Each parameter must be a map with String :id key")}}
(mt/user-http-request :crowberto :post 400 "dashboard" {:name "Test"
:parameters "abc"})))))
diff --git a/test/metabase/db/schema_migrations_test.clj b/test/metabase/db/schema_migrations_test.clj
index 8d977584ca8..159c7b59c3a 100644
--- a/test/metabase/db/schema_migrations_test.clj
+++ b/test/metabase/db/schema_migrations_test.clj
@@ -555,7 +555,7 @@
:left-join [[PermissionsGroup :pg] [:= :p.group_id :pg.id]]
:where [:= :pg.name perms-group/all-users-group-name]}))))))
-(deftest grant-subscription-permission-tests
+(deftest grant-subscription-permission-test
(testing "Migration v43.00-047: Grant the 'All Users' Group permissions to create/edit subscriptions and alerts"
(impl/test-migrations ["v43.00-047" "v43.00-048"] [migrate!]
(migrate!)
@@ -565,7 +565,7 @@
:left-join [[PermissionsGroup :pg] [:= :p.group_id :pg.id]]
:where [:= :p.object "/general/subscription/"]}))))))))
-(deftest rename-general-permissions-to-application-tests
+(deftest rename-general-permissions-to-application-test
(testing "Migration v43.00-057: Rename general permissions to application permissions"
(impl/test-migrations ["v43.00-057" "v43.00-058"] [migrate!]
(letfn [(get-perms [object] (set (map :name (db/query {:select [:pg.name]
@@ -575,3 +575,50 @@
(is (= #{"All Users"} (get-perms "/general/subscription/")))
(migrate!)
(is (= #{"All Users"} (get-perms "/application/subscription/")))))))
+
+(deftest add-parameter-to-cards-test
+ (testing "Migration v44.00-022: Add parameters to report_card"
+ (impl/test-migrations ["v44.00-022" "v44.00-024"] [migrate!]
+ (let [user-id
+ (db/simple-insert! User {:first_name "Howard"
+ :last_name "Hughes"
+ :email "howard@aircraft.com"
+ :password "superstrong"
+ :date_joined :%now})
+ database-id (db/simple-insert! Database {:name "DB", :engine "h2", :created_at :%now, :updated_at :%now})
+ card-id (db/simple-insert! Card {:name "My Saved Question"
+ :created_at :%now
+ :updated_at :%now
+ :creator_id user-id
+ :display "table"
+ :dataset_query "{}"
+ :visualization_settings "{}"
+ :database_id database-id
+ :collection_id nil})]
+ (migrate!)
+ (is (= [] (:parameters (first (db/simple-select Card {:where [:= :id card-id]})))))))))
+
+(deftest add-parameter-mappings-to-cards-test
+ (testing "Migration v44.00-024: Add parameter_mappings to cards"
+ (impl/test-migrations ["v44.00-024" "v44.00-026"] [migrate!]
+ (let [user-id
+ (db/simple-insert! User {:first_name "Howard"
+ :last_name "Hughes"
+ :email "howard@aircraft.com"
+ :password "superstrong"
+ :date_joined :%now})
+ database-id
+ (db/simple-insert! Database {:name "DB", :engine "h2", :created_at :%now, :updated_at :%now})
+ card-id
+ (db/simple-insert! Card {:name "My Saved Question"
+ :created_at :%now
+ :updated_at :%now
+ :creator_id user-id
+ :display "table"
+ :dataset_query "{}"
+ :visualization_settings "{}"
+ :database_id database-id
+ :collection_id nil})]
+ (migrate!)
+ (is (= []
+ (:parameter_mappings (first (db/simple-select Card {:where [:= :id card-id]})))))))))
diff --git a/test/metabase/events/revision_test.clj b/test/metabase/events/revision_test.clj
index 21c16fdb23e..40085e50e2b 100644
--- a/test/metabase/events/revision_test.clj
+++ b/test/metabase/events/revision_test.clj
@@ -32,6 +32,8 @@
:id (u/the-id card)
:made_public_by_id nil
:name (:name card)
+ :parameters []
+ :parameter_mappings []
:public_uuid nil
:cache_ttl nil
:query_type :query
diff --git a/test/metabase/models/card_test.clj b/test/metabase/models/card_test.clj
index c61c192a440..6bd68d3484a 100644
--- a/test/metabase/models/card_test.clj
+++ b/test/metabase/models/card_test.clj
@@ -291,3 +291,72 @@
clojure.lang.ExceptionInfo
#"Invalid Field Filter: Field \d+ \"VENUES\"\.\"NAME\" belongs to Database \d+ \"test-data\", but the query is against Database \d+ \"sample-dataset\""
(db/update! Card card-id bad-card-data))))))))
+
+
+;;; ------------------------------------------ Parameters tests ------------------------------------------
+
+(deftest validate-parameters-test
+ (testing "Should validate Card :parameters when"
+ (testing "creating"
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #":parameters must be a sequence of maps with String :id key"
+ (mt/with-temp Card [_ {:parameters {:a :b}}])))
+
+ (mt/with-temp Card [card {:parameters [{:id "valid-id"}]}]
+ (is (some? card))))
+
+ (testing "updating"
+ (mt/with-temp Card [{:keys [id]} {:parameters []}]
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #":parameters must be a sequence of maps with String :id key"
+ (db/update! Card id :parameters [{:id 100}])))
+ (is (some? (db/update! Card id :parameters [{:id "new-valid-id"}])))))))
+
+(deftest normalize-parameters-test
+ (testing ":parameters should get normalized when coming out of the DB"
+ (doseq [[target expected] {[:dimension [:field-id 1000]] [:dimension [:field 1000 nil]]
+ [:field-id 1000] [:field 1000 nil]}]
+ (testing (format "target = %s" (pr-str target))
+ (mt/with-temp Card [{card-id :id} {:parameters [{:name "Category Name"
+ :slug "category_name"
+ :id "_CATEGORY_NAME_"
+ :type "category"
+ :target target}]}]
+ (is (= [{:name "Category Name"
+ :slug "category_name"
+ :id "_CATEGORY_NAME_"
+ :type :category
+ :target expected}]
+ (db/select-one-field :parameters Card :id card-id))))))))
+
+(deftest validate-parameter-mappings-test
+ (testing "Should validate Card :parameter_mappings when"
+ (testing "creating"
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #":parameter_mappings must be a sequence of maps with String :parameter_id key"
+ (mt/with-temp Card [_ {:parameter_mappings {:a :b}}])))
+
+ (mt/with-temp Card [card {:parameter_mappings [{:parameter_id "valid-id"}]}]
+ (is (some? card))))
+
+ (testing "updating"
+ (mt/with-temp Card [{:keys [id]} {:parameter_mappings []}]
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #":parameter_mappings must be a sequence of maps with String :parameter_id key"
+ (db/update! Card id :parameter_mappings [{:parameter_id 100}])))
+
+ (is (some? (db/update! Card id :parameter_mappings [{:parameter_id "new-valid-id"}])))))))
+
+(deftest normalize-parameter-mappings-test
+ (testing ":parameter_mappings should get normalized when coming out of the DB"
+ (mt/with-temp Card [{card-id :id} {:parameter_mappings [{:parameter_id "22486e00"
+ :card_id 1
+ :target [:dimension [:field-id 1]]}]}]
+ (is (= [{:parameter_id "22486e00",
+ :card_id 1,
+ :target [:dimension [:field 1 nil]]}]
+ (db/select-one-field :parameter_mappings Card :id card-id))))))
diff --git a/test/metabase/models/dashboard_test.clj b/test/metabase/models/dashboard_test.clj
index 5599ee5ee99..73495dede6d 100644
--- a/test/metabase/models/dashboard_test.clj
+++ b/test/metabase/models/dashboard_test.clj
@@ -282,13 +282,13 @@
(testing "creating"
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
- #":parameters must be a sequence of maps with String :id keys"
+ #":parameters must be a sequence of maps with String :id key"
(mt/with-temp Dashboard [_ {:parameters {:a :b}}]))))
(testing "updating"
(mt/with-temp Dashboard [{:keys [id]} {:parameters []}]
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
- #":parameters must be a sequence of maps with String :id keys"
+ #":parameters must be a sequence of maps with String :id key"
(db/update! Dashboard id :parameters [{:id 100}])))))))
(deftest normalize-parameters-test
--
GitLab