diff --git a/docs/operations-guide/changing-session-expiration.md b/docs/operations-guide/changing-session-expiration.md
index d19fb8034a63d66ab83708037ea89c48bcaeca8e..18cadd07486234ee5c029b60aedd566e99679550 100644
--- a/docs/operations-guide/changing-session-expiration.md
+++ b/docs/operations-guide/changing-session-expiration.md
@@ -23,18 +23,22 @@ java -DMAX_SESSION_AGE=1440 -jar metabase.jar
### Using Session cookies
Metabase also supports using [session
-cookies](https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#Session_cookies), which mean users will stay
-authenticated until they close their browser window. Once they close their browser window, next time they visit
-Metabase they'll have to log in again. Session expiration still applies, so even if you leave your browser window open
-forever, you'll still be required to re-authenticate after two weeks or whatever session expiration you've configured.
+cookies](https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#Session_cookies), which mean users will only stay
+authenticated until they close their browser. This can be enabled on a per-user basis by unchecking the "Remember me"
+box when logging in. Once the user closes their browser, the next time they visit Metabase they'll have to log in
+again. Session expiration still applies, so even if you leave your browser open forever, you'll still be
+required to re-authenticate after two weeks or whatever session expiration you've configured.
-You can tell Metabase to use session cookies with the environment variable or Java system property
+You can tell Metabase to always use session cookies with the environment variable or Java system property
`MB_SESSION_COOKIES`:
```
MB_SESSION_COOKIES=true java -jar metabase.jar
```
+Setting this environment variable will override the behavior of the "Remember me" checkbox and enforce the use of
+session cookies for all users.
+
Note that browsers may use "session restoring", which means they automatically restore their previous session when
reopened. In this case, the browser effectively acts as if it was never closed; session cookies will act
the same as permanent cookies. For browsers that support this feature, this behavior is usually configurable.
diff --git a/docs/operations-guide/environment-variables.md b/docs/operations-guide/environment-variables.md
index ca79da0caef2d380a5356cca563ba0503bcd8bdc..be242ceba1c3e0dc95bc8f29e6f8c2ccab030f84 100644
--- a/docs/operations-guide/environment-variables.md
+++ b/docs/operations-guide/environment-variables.md
@@ -946,7 +946,8 @@ Send email notifications to users in Admin group, when a new SSO users is create
Type: boolean<br>
Default: `null`
-When set to `true`, the user login session will expire, when the browser is closed. The user login session will always expire after the amount of time defined in [MAX_SESSION_AGE](#max_session_age) (by default 2 weeks).
+When set to `true`, the user login session will expire when the browser is closed. The user login session will always expire after the amount of time defined in [MAX_SESSION_AGE](#max_session_age) (by default 2 weeks).
+This overrides the "Remember me" checkbox when logging in.
Also see the [Changing session expiration](changing-session-expiration.md) documentation page.
diff --git a/enterprise/backend/src/metabase_enterprise/sandbox/models/permissions/delete_sandboxes.clj b/enterprise/backend/src/metabase_enterprise/sandbox/models/permissions/delete_sandboxes.clj
new file mode 100644
index 0000000000000000000000000000000000000000..9aa41048fe413bbcf40657800acfa9a7321c308b
--- /dev/null
+++ b/enterprise/backend/src/metabase_enterprise/sandbox/models/permissions/delete_sandboxes.clj
@@ -0,0 +1,140 @@
+(ns metabase-enterprise.sandbox.models.permissions.delete-sandboxes
+ (:require [clojure.tools.logging :as log]
+ [metabase-enterprise.enhancements.ee-strategy-impl :as ee-strategy-impl]
+ [metabase-enterprise.sandbox.models.group-table-access-policy :refer [GroupTableAccessPolicy]]
+ [metabase.models.permissions.delete-sandboxes :as delete-sandboxes]
+ [metabase.models.table :refer [Table]]
+ [metabase.public-settings.metastore :as settings.metastore]
+ [metabase.util :as u]
+ [metabase.util.i18n :refer [tru]]
+ [pretty.core :as pretty]
+ [toucan.db :as db]))
+
+(defn- delete-gtaps-with-condition! [group-or-id condition]
+ (when (seq condition)
+ (let [conditions (into
+ [:and
+ [:= :gtap.group_id (u/the-id group-or-id)]]
+ [condition])]
+ (log/debugf "Deleting GTAPs for Group %d with conditions %s" (u/the-id group-or-id) (pr-str conditions))
+ (try
+ (if-let [gtap-ids (not-empty (set (map :id (db/query
+ {:select [[:gtap.id :id]]
+ :from [[GroupTableAccessPolicy :gtap]]
+ :left-join [[Table :table]
+ [:= :gtap.table_id :table.id]]
+ :where conditions}))))]
+ (do
+ (log/debugf "Deleting %d matching GTAPs: %s" (count gtap-ids) (pr-str gtap-ids))
+ (db/delete! GroupTableAccessPolicy :id [:in gtap-ids]))
+ (log/debug "No matching GTAPs need to be deleted."))
+ (catch Throwable e
+ (throw (ex-info (tru "Error deleting Sandboxes: {0}" (ex-message e))
+ {:group (u/the-id group-or-id), :conditions conditions}
+ e)))))))
+
+(defn- delete-gtaps-for-group-table! [{:keys [group-id table-id], :as context} changes]
+ (log/debugf "Deleting unneeded GTAPs for Group %d for Table %d. Graph changes: %s"
+ group-id table-id (pr-str changes))
+ (cond
+ (= changes :none)
+ (do
+ (log/debugf "Group %d no longer has any permissions for Table %d, deleting GTAP for this Table if one exists"
+ group-id table-id)
+ (delete-gtaps-with-condition! group-id [:= :table.id table-id]))
+
+ (= changes :all)
+ (do
+ (log/debugf "Group %d now has full data perms for Table %d, deleting GTAP for this Table if one exists"
+ group-id table-id)
+ (delete-gtaps-with-condition! group-id [:= :table.id table-id]))
+
+ :else
+ (let [new-query-perms (get changes :query :none)]
+ (case new-query-perms
+ :none
+ (do
+ (log/debugf "Group %d no longer has any query perms for Table %d; deleting GTAP for this Table if one exists"
+ group-id table-id)
+ (delete-gtaps-with-condition! group-id [:= :table.id table-id]))
+
+ :all
+ (do
+ (log/debugf "Group %d now has full non-sandboxed query perms for Table %d; deleting GTAP for this Table if one exists"
+ group-id table-id)
+ (delete-gtaps-with-condition! group-id [:= :table.id table-id]))
+
+ :segmented
+ (log/debugf "Group %d now has full segmented query perms for Table %d. Do not need to delete GTAPs."
+ group-id table-id)))))
+
+(defn- delete-gtaps-for-group-schema! [{:keys [group-id database-id schema-name], :as context} changes]
+ (log/debugf "Deleting unneeded GTAPs for Group %d for Database %d, schema %s. Graph changes: %s"
+ group-id database-id (pr-str schema-name) (pr-str changes))
+ (cond
+ (= changes :none)
+ (do
+ (log/debugf "Group %d no longer has any permissions for Database %d schema %s, deleting all GTAPs for this schema"
+ group-id database-id (pr-str schema-name))
+ (delete-gtaps-with-condition! group-id [:and [:= :table.db_id database-id] [:= :table.schema schema-name]]))
+
+ (= changes :all)
+ (do
+ (log/debugf "Group %d changes has full data perms for Database %d schema %s, deleting all GTAPs for this schema"
+ group-id database-id (pr-str schema-name))
+ (delete-gtaps-with-condition! group-id [:and [:= :table.db_id database-id] [:= :table.schema schema-name]]))
+
+ :else
+ (doseq [table-id (set (keys changes))]
+ (delete-gtaps-for-group-table! (assoc context :table-id table-id) (get changes table-id)))))
+
+(defn- delete-gtaps-for-group-database! [{:keys [group-id database-id], :as context} changes]
+ (log/debugf "Deleting unneeded GTAPs for Group %d for Database %d. Graph changes: %s"
+ group-id database-id (pr-str changes))
+ (cond
+ (= changes :none)
+ (do
+ (log/debugf "Group %d no longer has any perms for Database %d, deleting all GTAPs for this DB" group-id database-id)
+ (delete-gtaps-with-condition! group-id [:= :table.db_id database-id]))
+
+ (= changes :all)
+ (do
+ (log/debugf "Group %d now has full data perms for Database %d, deleting all GTAPs for this DB" group-id database-id)
+ (delete-gtaps-with-condition! group-id [:= :table.db_id database-id]))
+
+ :else
+ (doseq [schema-name (set (keys changes))]
+ (delete-gtaps-for-group-schema!
+ (assoc context :schema-name schema-name)
+ (get changes schema-name)))))
+
+(defn- delete-gtaps-for-group! [{:keys [group-id]} changes]
+ (log/debugf "Deleting unneeded GTAPs for Group %d. Graph changes: %s" group-id (pr-str changes))
+ (doseq [database-id (set (keys changes))]
+ (delete-gtaps-for-group-database!
+ {:group-id group-id, :database-id database-id}
+ (get-in changes [database-id :schemas]))))
+
+(defn- delete-gtaps-if-needed-after-permissions-change! [changes]
+ (log/debug "Permissions updated, deleting unneeded GTAPs...")
+ (doseq [group-id (set (keys changes))]
+ (delete-gtaps-for-group! {:group-id group-id} (get changes group-id)))
+ (log/debug "Done deleting unneeded GTAPs."))
+
+(def ^:private impl
+ (reify
+ delete-sandboxes/DeleteSandboxes
+ (delete-gtaps-if-needed-after-permissions-change!* [_ changes]
+ (delete-gtaps-if-needed-after-permissions-change! changes))
+
+ pretty/PrettyPrintable
+ (pretty [_]
+ `impl)))
+
+(def ee-strategy-impl
+ "EE impl for Sandbox (GTAP) deletion behavior. Don't use this directly."
+ (ee-strategy-impl/reify-ee-strategy-impl
+ #'settings.metastore/enable-sandboxes?
+ impl
+ delete-sandboxes/oss-default-impl
+ delete-sandboxes/DeleteSandboxes))
diff --git a/enterprise/backend/src/metabase_enterprise/serialization/cmd.clj b/enterprise/backend/src/metabase_enterprise/serialization/cmd.clj
index f2ec554677d88bbbeb08e45a72e8b95d614baa0a..c613e4db752bf157cd5508774474e55f4403c156 100644
--- a/enterprise/backend/src/metabase_enterprise/serialization/cmd.clj
+++ b/enterprise/backend/src/metabase_enterprise/serialization/cmd.clj
@@ -10,6 +10,7 @@
[metabase.models.database :refer [Database]]
[metabase.models.field :as field :refer [Field]]
[metabase.models.metric :refer [Metric]]
+ [metabase.models.native-query-snippet :refer [NativeQuerySnippet]]
[metabase.models.pulse :refer [Pulse]]
[metabase.models.segment :refer [Segment]]
[metabase.models.table :refer [Table]]
@@ -47,6 +48,7 @@
context)]
(try
(do
+ (log/info (trs "BEGIN LOAD from {0} with context {1}" path context))
(let [all-res [(load/load (str path "/users") context)
(load/load (str path "/databases") context)
(load/load (str path "/collections") context)
@@ -56,9 +58,10 @@
(if-not (empty? reload-fns)
(do (log/info (trs "Finished first pass of load; now performing second pass"))
(doseq [reload-fn reload-fns]
- (reload-fn))))))
+ (reload-fn))))
+ (log/info (trs "END LOAD from {0} with context {1}" path context))))
(catch Throwable e
- (log/error e (trs "Error loading dump: {0}" (.getMessage e)))))))
+ (log/error e (trs "ERROR LOAD from {0}: {1}" path (.getMessage e)))))))
(defn- select-entities-in-collections
([model collections]
@@ -87,25 +90,38 @@
(mapcat #(db/select Segment :table_id (u/the-id %)) tables)))))
(defn- select-collections
+ "Selects the collections for a given user-id, or all collections without a personal ID if the passed user-id is nil.
+ If `state` is passed (by default, `:active`), then that will be used to filter for collections that are archived (if
+ the value is passed as `:all`)."
([users]
(select-collections users :active))
([users state]
- (let [state-filter (case state
- :all nil
- :active [:= :archived false])]
- (db/select Collection
- {:where [:and
- [:or
- [:= :personal_owner_id nil]
- [:= :personal_owner_id (some-> users first u/the-id)]]
- state-filter]}))))
+ (let [state-filter (case state
+ :all nil
+ :active [:= :archived false])
+ base-collections (db/select Collection {:where [:and [:= :location "/"]
+ [:or [:= :personal_owner_id nil]
+ [:= :personal_owner_id
+ (some-> users first u/the-id)]]
+ state-filter]})]
+ (-> (db/select Collection
+ {:where [:and
+ (reduce (fn [acc coll]
+ (conj acc [:like :location (format "/%d/%%" (:id coll))]))
+ [:or] base-collections)
+ state-filter]})
+ (into base-collections)))))
+
(defn dump
"Serialized metabase instance into directory `path`."
([path user]
- (dump path user :active))
- ([path user state]
+ (dump path user :active {}))
+ ([path user opts]
+ (dump path user :active opts))
+ ([path user state opts]
(mdb/setup-db!)
+ (log/info (trs "BEGIN DUMP to {0} via user {1}" path user))
(let [users (if user
(let [user (db/select-one User
:email user
@@ -113,19 +129,32 @@
(assert user (trs "{0} is not a valid user" user))
[user])
[])
- tables (Table)
- collections (Collection)] ;(select-collections users state)]
+ databases (if (contains? opts :only-db-ids)
+ (db/select Database :id [:in (:only-db-ids opts)] {:order-by [[:id :asc]]})
+ (Database))
+ tables (if (contains? opts :only-db-ids)
+ (db/select Table :db_id [:in (:only-db-ids opts)] {:order-by [[:id :asc]]})
+ (Table))
+ fields (if (contains? opts :only-db-ids)
+ (db/select Field :table_id [:in (map :id tables)] {:order-by [[:id :asc]]})
+ (Field))
+ metrics (if (contains? opts :only-db-ids)
+ (db/select Metric :table_id [:in (map :id tables)] {:order-by [[:id :asc]]})
+ (Metric))
+ collections (select-collections users state)]
(dump/dump path
- (Database)
+ databases
tables
- (field/with-values (Field))
- (Metric)
+ (field/with-values fields)
+ metrics
(select-segments-in-tables tables state)
collections
+ (select-entities-in-collections NativeQuerySnippet collections state)
(select-entities-in-collections Card collections state)
(select-entities-in-collections Dashboard collections state)
(select-entities-in-collections Pulse collections state)
users))
(dump/dump-settings path)
(dump/dump-dependencies path)
- (dump/dump-dimensions path)))
+ (dump/dump-dimensions path)
+ (log/info (trs "END DUMP to {0} via user {1}" path user))))
diff --git a/enterprise/backend/src/metabase_enterprise/serialization/dump.clj b/enterprise/backend/src/metabase_enterprise/serialization/dump.clj
index 359b55758cdec494cb8dbf571fd73b2447061a24..2e1fd0d15e00b1ff3adbf432cbd63702f281f15e 100644
--- a/enterprise/backend/src/metabase_enterprise/serialization/dump.clj
+++ b/enterprise/backend/src/metabase_enterprise/serialization/dump.clj
@@ -51,8 +51,8 @@
(doseq [entity (flatten entities)]
(try
(spit-entity path entity)
- (catch Throwable _
- (log/error (trs "Error dumping {0}" (name-for-logging entity))))))
+ (catch Throwable e
+ (log/error e (trs "Error dumping {0}" (name-for-logging entity))))))
(spit-yaml (str path "/manifest.yaml")
{:serialization-version serialize/serialization-protocol-version
:metabase-version config/mb-version-info}))
diff --git a/enterprise/backend/src/metabase_enterprise/serialization/load.clj b/enterprise/backend/src/metabase_enterprise/serialization/load.clj
index f2ed582e55eb8095b7de4e251056d370acca2d40..1fa4d005cd4ed33565f33627231c40292a4fa686 100644
--- a/enterprise/backend/src/metabase_enterprise/serialization/load.clj
+++ b/enterprise/backend/src/metabase_enterprise/serialization/load.clj
@@ -5,7 +5,7 @@
[clojure.string :as str]
[clojure.tools.logging :as log]
[medley.core :as m]
- [metabase-enterprise.serialization.names :refer [fully-qualified-name->context]]
+ [metabase-enterprise.serialization.names :as names :refer [fully-qualified-name->context]]
[metabase-enterprise.serialization.upsert :refer [maybe-fixup-card-template-ids! maybe-upsert-many!]]
[metabase.config :as config]
[metabase.mbql.normalize :as mbql.normalize]
@@ -21,6 +21,7 @@
[metabase.models.field :refer [Field]]
[metabase.models.field-values :refer [FieldValues]]
[metabase.models.metric :refer [Metric]]
+ [metabase.models.native-query-snippet :refer [NativeQuerySnippet]]
[metabase.models.pulse :refer [Pulse]]
[metabase.models.pulse-card :refer [PulseCard]]
[metabase.models.pulse-channel :refer [PulseChannel]]
@@ -28,6 +29,7 @@
[metabase.models.setting :as setting]
[metabase.models.table :refer [Table]]
[metabase.models.user :refer [User]]
+ [metabase.shared.models.visualization-settings :as mb.viz]
[metabase.util :as u]
[metabase.util.date-2 :as u.date]
[metabase.util.i18n :refer [trs]]
@@ -57,21 +59,116 @@
:when (.isDirectory file)]
(.getPath file)))
-(defn- mbql-fully-qualified-names->ids
+(defn- source-table
+ [source-table]
+ (if (and (string? source-table) (str/starts-with? source-table "card__"))
+ source-table
+ (let [{:keys [card table]} (fully-qualified-name->context source-table)]
+ (if card
+ (str "card__" card)
+ table))))
+
+(defn- fq-table-or-card?
+ "Returns true if the given `nm` is either a fully qualified table name OR fully
+ qualified card name."
+ [nm]
+ (or (names/fully-qualified-table-name? nm) (names/fully-qualified-card-name? nm)))
+
+(defn- update-capture-missing*
+ [m ks resolve-fn get-fn update-fn]
+ (let [orig-v (get-fn m ks)
+ res (update-fn m ks resolve-fn)
+ new-v (get-fn res ks)]
+ (if (and (some? orig-v) (nil? new-v))
+ (update res ::unresolved-names #(assoc % orig-v ks))
+ res)))
+
+(defn- update-in-capture-missing
+ [m ks resolve-fn]
+ (update-capture-missing* m ks resolve-fn get-in update-in))
+
+(defn- update-existing-in-capture-missing
+ [m ks resolve-fn]
+ (update-capture-missing* m ks resolve-fn get-in m/update-existing-in))
+
+(defn- update-existing-capture-missing
+ [m k resolve-fn]
+ (update-capture-missing* m [k] resolve-fn get-in m/update-existing-in))
+
+(defn- pull-unresolved-names-up
+ "Assocs the given value `v` to the given key sequence `ks` in the given map `m`. If the given `v` contains any
+ ::unresolved-names, these are \"pulled into\" `m` directly by prepending `ks` to their existing paths and dissocing
+ them from `v`."
+ ([m ks]
+ (pull-unresolved-names-up m ks (get-in m ks)))
+ ([m ks v]
+ (if-let [unresolved-names (::unresolved-names v)]
+ (-> (update m ::unresolved-names (fn [nms] (merge nms (m/map-vals #(vec (concat ks %)) unresolved-names))))
+ (assoc-in ks (dissoc v ::unresolved-names)))
+ (assoc-in m ks v))))
+
+(defn- paths-to-key-in
+ "Finds all paths to a particular key anywhere in the structure `m` (recursively). `m` must be a map, but values
+ inside can also be vectors (in which case, the index will be used as the key).
+
+ Adapted from: https://dnaeon.github.io/clojure-map-ks-paths/"
+ [m match-key]
+ (letfn [(children [node]
+ (let [v (get-in m node)]
+ (cond
+ (map? v)
+ (map (fn [x] (conj node x)) (keys v))
+
+ (vector? v)
+ (map (fn [x] (conj node x)) (range (count v)))
+
+ :default
+ [])))
+ (branch? [node] (-> (children node) seq boolean))]
+ (->> (keys m)
+ (map vector)
+ (mapcat #(tree-seq branch? children %))
+ (filter #(= match-key (last %))))))
+
+(defn- gather-all-unresolved-names
+ "This is less efficient than calling `pull-unresolved-names-up` because it walks the entire tree, but is
+ necessary when dealing with the full MBQL query tree (which can have arbitrary nesting of maps and
+ vectors)."
+ [m]
+ (reduce (fn [acc ks]
+ (pull-unresolved-names-up acc (drop-last ks))) m (paths-to-key-in m ::unresolved-names)))
+
+(defn- mbql-fully-qualified-names->ids*
[entity]
- (mbql.util/replace (mbql.normalize/normalize-tokens entity)
+ (mbql.util/replace entity
;; handle legacy `:field-id` forms encoded prior to 0.39.0
+ ;; and also *current* expresion forms used in parameter mapping dimensions
+ ;; example relevant clause - [:dimension [:fk-> [:field-id 1] [:field-id 2]]]
[:field-id (fully-qualified-name :guard string?)]
- (mbql-fully-qualified-names->ids [:field fully-qualified-name nil])
+ (mbql-fully-qualified-names->ids* [:field fully-qualified-name nil])
- [:field (fully-qualified-name :guard string?) opts]
- [:field (:field (fully-qualified-name->context fully-qualified-name)) opts]
+ [:field (fully-qualified-name :guard names/fully-qualified-field-name?) opts]
+ [:field (:field (fully-qualified-name->context fully-qualified-name)) (mbql-fully-qualified-names->ids* opts)]
+
+ ;; source-field is also used within parameter mapping dimensions
+ ;; example relevant clause - [:field 2 {:source-field 1}]
+ {:source-field (fully-qualified-name :guard string?)}
+ (assoc &match :source-field (:field (fully-qualified-name->context fully-qualified-name)))
[:metric (fully-qualified-name :guard string?)]
[:metric (:metric (fully-qualified-name->context fully-qualified-name))]
[:segment (fully-qualified-name :guard string?)]
- [:segment (:segment (fully-qualified-name->context fully-qualified-name))]))
+ [:segment (:segment (fully-qualified-name->context fully-qualified-name))]
+
+ (_ :guard (every-pred map? #(fq-table-or-card? (:source-table %))))
+ (-> (mbql-fully-qualified-names->ids* (dissoc &match :source-table)) ;; process other keys
+ (assoc :source-table (:source-table &match)) ;; add :source-table back in for lookup
+ (update-existing-capture-missing :source-table source-table)))) ;; look up :source-table and capture missing
+
+(defn- mbql-fully-qualified-names->ids
+ [entity]
+ (mbql-fully-qualified-names->ids* (mbql.normalize/normalize-tokens entity)))
(def ^:private default-user (delay
(let [user (db/select-one-id User :is_superuser true)]
@@ -83,6 +180,21 @@
[path]
(.getName (clojure.java.io/file path)))
+(defn- unresolved-names->string
+ ([entity]
+ (unresolved-names->string entity nil))
+ ([entity insert-id]
+ (str
+ (if-let [nm (:name entity)] (str "\"" nm "\""))
+ (if insert-id (format " (inserted as ID %d) " insert-id))
+ "missing:\n "
+ (str/join
+ "\n "
+ (map
+ (fn [[k v]]
+ (format "at %s -> %s" (str/join "/" v) k))
+ (::unresolved-names entity))))))
+
(defmulti load
"Load an entity of type `model` stored at `path` in the context `context`.
@@ -178,12 +290,107 @@
(assoc-in [:definition :source-table] (:table context))
(update :definition mbql-fully-qualified-names->ids)))))
-(defn- update-parameter-mappings
+(defn- update-card-parameter-mappings
[parameter-mappings]
(for [parameter-mapping parameter-mappings]
(-> parameter-mapping
- (update :card_id fully-qualified-name->card-id)
- (update :target mbql-fully-qualified-names->ids))))
+ (update-existing-capture-missing :card_id fully-qualified-name->card-id)
+ (update-existing-capture-missing :target mbql-fully-qualified-names->ids))))
+
+(defn- resolve-column-settings-key
+ [col-key]
+ (if-let [field-name (::mb.viz/field-str col-key)]
+ (let [field-id ((comp :field fully-qualified-name->context) field-name)]
+ (if (nil? field-id)
+ {::unresolved-names {field-name [::column-settings-key]}}
+ {::mb.viz/field-id field-id}))
+ col-key))
+
+(defn- resolve-param-mapping-key [k]
+ (mbql-fully-qualified-names->ids k))
+
+(defn- resolve-dimension [dimension]
+ (mbql-fully-qualified-names->ids dimension))
+
+(defn- resolve-param-ref [param-ref]
+ (cond-> param-ref
+ (= "dimension" (::mb.viz/param-ref-type param-ref))
+ (-> ; from outer cond->
+ (m/update-existing ::mb.viz/param-ref-id mbql-fully-qualified-names->ids)
+ (m/update-existing ::mb.viz/param-dimension resolve-dimension))))
+
+(defn- resolve-param-mapping-val [v]
+ (-> v
+ (m/update-existing ::mb.viz/param-mapping-id mbql-fully-qualified-names->ids)
+ (m/update-existing ::mb.viz/param-mapping-source resolve-param-ref)
+ (m/update-existing ::mb.viz/param-mapping-target resolve-param-ref)))
+
+(defn- resolve-click-behavior-parameter-mapping [parameter-mapping]
+ (->> parameter-mapping
+ mb.viz/db->norm-param-mapping
+ (reduce-kv (fn [acc k v]
+ (assoc acc (resolve-param-mapping-key k)
+ (resolve-param-mapping-val v))) {})
+ mb.viz/norm->db-param-mapping))
+
+(defn- resolve-click-behavior
+ [click-behavior]
+ (-> (if-let [link-type (::mb.viz/link-type click-behavior)]
+ (case link-type
+ ::mb.viz/card (let [card-id (::mb.viz/link-target-id click-behavior)]
+ (if (string? card-id)
+ (update-existing-in-capture-missing
+ click-behavior
+ [::mb.viz/link-target-id]
+ (comp :card fully-qualified-name->context))))
+ ::mb.viz/dashboard (let [dashboard-id (::mb.viz/link-target-id click-behavior)]
+ (if (string? dashboard-id)
+ (update-existing-in-capture-missing
+ click-behavior
+ [::mb.viz/link-target-id]
+ (comp :dashboard fully-qualified-name->context))))
+ click-behavior)
+ click-behavior)
+ (m/update-existing ::mb.viz/parameter-mapping resolve-click-behavior-parameter-mapping)))
+
+(defn- update-col-settings-click-behavior [col-settings-value]
+ (let [new-cb (resolve-click-behavior (::mb.viz/click-behavior col-settings-value))]
+ (pull-unresolved-names-up col-settings-value [::mb.viz/click-behavior] new-cb)))
+
+(defn- resolve-column-settings-value
+ [col-value]
+ (cond-> col-value
+ (::mb.viz/click-behavior col-value) update-col-settings-click-behavior))
+
+(defn- accumulate-converted-column-settings
+ [acc col-key v]
+ (let [new-key (resolve-column-settings-key col-key)
+ new-val (resolve-column-settings-value v)]
+ (-> (pull-unresolved-names-up acc [::column-settings-key] new-key)
+ (dissoc ::column-settings-key)
+ (pull-unresolved-names-up [new-key] new-val))))
+
+(defn- resolve-top-level-click-behavior [vs-norm]
+ (if-let [click-behavior (::mb.viz/click-behavior vs-norm)]
+ (let [resolved-cb (resolve-click-behavior click-behavior)]
+ (pull-unresolved-names-up vs-norm [::mb.viz/click-behavior] resolved-cb))
+ vs-norm))
+
+(defn- resolve-column-settings [vs-norm]
+ (if-let [col-settings (::mb.viz/column-settings vs-norm)]
+ (let [resolved-cs (reduce-kv accumulate-converted-column-settings {} col-settings)]
+ (pull-unresolved-names-up vs-norm [::mb.viz/column-settings] resolved-cs))
+ vs-norm))
+
+(defn- resolve-visualization-settings
+ [entity]
+ (if-let [viz-settings (:visualization_settings entity)]
+ (let [resolved-vs (-> (mb.viz/db->norm viz-settings)
+ resolve-top-level-click-behavior
+ resolve-column-settings
+ mb.viz/norm->db)]
+ (pull-unresolved-names-up entity [:visualization_settings] resolved-vs))
+ entity))
(defn load-dashboards
"Loads `dashboards` (which is a sequence of maps parsed from a YAML dump of dashboards) in a given `context`."
@@ -198,104 +405,154 @@
dashboard-cards (map :dashboard_cards dashboards)
;; a function that prepares a dash card for insertion, while also validating to ensure the underlying
;; card_id could be resolved from the fully qualified name
- prepare-card-fn (fn [idx dashboard-id card]
- (let [final-c (-> (assoc card :fully-qualified-card-name (:card_id card))
- (update :card_id fully-qualified-name->card-id)
- (assoc :dashboard_id dashboard-id)
- (update :parameter_mappings update-parameter-mappings))]
- (if (and (nil? (:card_id final-c))
- (some? (:fully-qualified-card-name final-c)))
- ;; the lookup of some fully qualified card name failed
- ;; we will need to revisit this dashboard in the next pass
- {:revisit idx}
- ;; card was looked up OK, we can process it
- {:process final-c})))
+ prepare-card-fn (fn [dash-idx dashboard-id acc card-idx card]
+ (let [proc-card (-> card
+ (update-existing-capture-missing :card_id fully-qualified-name->card-id)
+ (assoc :dashboard_id dashboard-id))
+ new-pm (update-card-parameter-mappings (:parameter_mappings proc-card))
+ with-pm (pull-unresolved-names-up proc-card [:parameter_mappings] new-pm)
+ with-viz (resolve-visualization-settings with-pm)]
+ (if-let [unresolved (::unresolved-names with-viz)]
+ ;; prepend the dashboard card index and :visualization_settings to each unresolved
+ ;; name path for better debugging
+ (let [add-keys [:dashboard_cards card-idx :visualization_settings]
+ fixed-names (m/map-vals #(concat add-keys %) unresolved)
+ with-fixed-names (assoc with-viz ::unresolved-names fixed-names)]
+ (-> acc
+ (update ::revisit (fn [revisit-map]
+ (update revisit-map dash-idx #(cons with-fixed-names %))))
+ ;; index means something different here than in the Card case (it's actually the index
+ ;; of the dashboard)
+ (update ::revisit-index #(conj % dash-idx))))
+ (update acc ::process #(conj % with-viz)))))
+ prep-init-acc {::process [] ::revisit-index #{} ::revisit {}}
filtered-cards (reduce-kv
(fn [acc idx [cards dash-id]]
(if dash-id
- (let [{:keys [process revisit]} (apply
- merge-with
- conj
- {:process [] :revisit []}
- (map (partial prepare-card-fn idx dash-id) cards))]
- (-> acc
- (update :process #(concat % process))
- (update :revisit #(concat % revisit))))
+ (let [res (reduce-kv (partial prepare-card-fn idx dash-id) prep-init-acc (vec cards))]
+ (merge-with concat acc res))
acc))
- {:process [] :revisit []}
+ prep-init-acc
(mapv vector dashboard-cards dashboard-ids))
- revisit-indexes (:revisit filtered-cards)
- proceed-cards (->> (:process filtered-cards)
- (map #(dissoc % :fully-qualified-card-name)))
- dashcard-ids (maybe-upsert-many! context DashboardCard (map #(dissoc % :series) proceed-cards))]
+ revisit-indexes (vec (::revisit-index filtered-cards))
+ proceed-cards (vec (::process filtered-cards))
+ dashcard-ids (maybe-upsert-many! context DashboardCard (map #(dissoc % :series) proceed-cards))
+ series-pairs (map vector (map :series proceed-cards) dashcard-ids)]
(maybe-upsert-many! context DashboardCardSeries
- (for [[series dashboard-card-id] (map vector (mapcat (partial map :series) proceed-cards)
- dashcard-ids)
- dashboard-card-series series
+ (for [[series dashboard-card-id] series-pairs
+ dashboard-card-series series
:when (and dashboard-card-series dashboard-card-id)]
(-> dashboard-card-series
(assoc :dashboardcard_id dashboard-card-id)
(update :card_id fully-qualified-name->card-id))))
(let [revisit-dashboards (map (partial nth dashboards) revisit-indexes)]
(if-not (empty? revisit-dashboards)
- (fn []
+ (let [revisit-map (::revisit filtered-cards)
+ revisit-inf-fn (fn [[dash-idx dashcards]]
+ (format
+ "For dashboard %s:%n%s"
+ (->> dash-idx (nth dashboards) :name)
+ (str/join "\n" (map unresolved-names->string dashcards))))]
(log/infof
- "Retrying dashboards for collection %s: %s"
- (or (:collection context) "root")
- (str/join ", " (map :name revisit-dashboards)))
- (load-dashboards context revisit-dashboards))))))
+ "Unresolved references found for dashboard cards in collection %d; will reload after first pass%n%s%n"
+ (:collection context)
+ (str/join "\n" (map revisit-inf-fn revisit-map)))
+ (fn []
+ (log/infof
+ "Retrying dashboards for collection %s: %s"
+ (or (:collection context) "root")
+ (str/join ", " (map :name revisit-dashboards)))
+ (load-dashboards context revisit-dashboards)))))))
(defmethod load "dashboards"
[path context]
- (load-dashboards context (slurp-dir path)))
-
-(defmethod load "pulses"
- [path context]
- (let [pulses (slurp-dir path)
- cards (map :cards pulses)
- channels (map :channels pulses)
- pulse-ids (maybe-upsert-many! context Pulse
- (for [pulse pulses]
- (-> pulse
- (assoc :collection_id (:collection context)
- :creator_id @default-user)
- (dissoc :channels :cards))))]
- (maybe-upsert-many! context PulseCard
- (for [[cards pulse-id] (map vector cards pulse-ids)
- card cards
- :when pulse-id]
- (-> card
- (assoc :pulse_id pulse-id)
- (update :card_id fully-qualified-name->card-id))))
+ (binding [names/*suppress-log-name-lookup-exception* true]
+ (load-dashboards context (slurp-dir path))))
+
+(defn- load-pulses [pulses context]
+ (let [cards (map :cards pulses)
+ channels (map :channels pulses)
+ pulse-ids (maybe-upsert-many! context Pulse
+ (for [pulse pulses]
+ (-> pulse
+ (assoc :collection_id (:collection context)
+ :creator_id @default-user)
+ (dissoc :channels :cards))))
+ pulse-cards (for [[cards pulse-id pulse-idx] (map vector cards pulse-ids (range 0 (count pulse-ids)))
+ card cards
+ :when pulse-id]
+ (-> card
+ (assoc :pulse_id pulse-id)
+ ;; gather the pulse's name and index for easier bookkeeping later
+ (assoc ::pulse-index pulse-idx)
+ (assoc ::pulse-name (:name (nth pulses pulse-idx)))
+ (update-in-capture-missing [:card_id] fully-qualified-name->card-id)))
+ grouped (group-by #(empty? (::unresolved-names %)) pulse-cards)
+ process (get grouped true)
+ revisit (get grouped false)]
+ (maybe-upsert-many! context PulseCard (map #(dissoc % ::pulse-index ::pulse-name) process))
(maybe-upsert-many! context PulseChannel
(for [[channels pulse-id] (map vector channels pulse-ids)
channel channels
:when pulse-id]
- (assoc channel :pulse_id pulse-id)))))
+ (assoc channel :pulse_id pulse-id)))
+ (if-not (empty? revisit)
+ (let [revisit-info-map (group-by ::pulse-name revisit)]
+ (log/infof "Unresolved references for pulses in collection %s; will reload after first pass complete:%n%s%n"
+ (or (:collection context) "root")
+ (str/join "\n" (map
+ (fn [[pulse-name revisit-cards]]
+ (format " for %s:%n%s"
+ pulse-name
+ (str/join "\n" (map (comp unresolved-names->string #(into {} %)) revisit-cards))))
+ revisit-info-map)))
+ (fn []
+ (log/infof "Reloading pulses from collection %d" (:collection context))
+ (let [pulse-indexes (map ::pulse-index revisit)]
+ (load-pulses (map (partial nth pulses) pulse-indexes) context)))))))
-(defn- source-table
- [source-table]
- (let [{:keys [card table]} (fully-qualified-name->context source-table)]
- (if card
- (str "card__" card)
- table)))
+(defmethod load "pulses"
+ [path context]
+ (binding [names/*suppress-log-name-lookup-exception* true]
+ (load-pulses (slurp-dir path) context)))
-(defn- fully-qualified-name->id-rec [query]
- (cond
- (:source-table query) (update-in query [:source-table] source-table)
- (:source-query query) (update-in query [:source-query] fully-qualified-name->id-rec)
- true query))
+(defn- resolve-source-query [query]
+ (if (:source-query query)
+ (update-in-capture-missing query [:source-query] resolve-source-query)
+ query))
(defn- source-card
[fully-qualified-name]
(try
(-> (fully-qualified-name->context fully-qualified-name) :card)
(catch Throwable e
- (log/warn e))))
+ (log/warn e (trs "Could not find context for fully qualified card name {0}" fully-qualified-name)))))
+
+(defn- resolve-snippet
+ [fully-qualified-name]
+ (try
+ (-> (fully-qualified-name->context fully-qualified-name) :snippet)
+ (catch Throwable e
+ (log/debug e (trs "Could not find context for fully qualified snippet name {0}" fully-qualified-name)))))
(defn- resolve-native
- [template-tags]
- (m/map-vals #(m/update-existing % :card-id source-card) template-tags))
+ [card]
+ (let [ks [:dataset_query :native :template-tags]
+ template-tags (get-in card ks)
+ new-template-tags (reduce-kv
+ (fn [m k v]
+ (let [new-v (-> (update-existing-capture-missing v :card-id source-card)
+ (update-existing-capture-missing :snippet-id resolve-snippet))]
+ (pull-unresolved-names-up m [k] new-v)))
+ {}
+ template-tags)]
+ (pull-unresolved-names-up card ks new-template-tags)))
+
+(defn- resolve-card-dataset-query [card]
+ (let [ks [:dataset_query :query]
+ new-q (update-in-capture-missing card ks resolve-source-query)]
+ (-> (pull-unresolved-names-up card ks (get-in new-q ks))
+ (gather-all-unresolved-names))))
(defn- resolve-card [card context]
(-> card
@@ -305,42 +562,85 @@
(assoc :creator_id @default-user
:collection_id (:collection context))
(update-in [:dataset_query :database] (comp :database fully-qualified-name->context))
+ resolve-visualization-settings
(cond->
(-> card
:dataset_query
:type
mbql.util/normalize-token
- (= :query)) (update-in [:dataset_query :query] fully-qualified-name->id-rec)
+ (= :query)) resolve-card-dataset-query
(-> card
:dataset_query
:native
- :template-tags)
- (m/update-existing-in [:dataset_query :native :template-tags] resolve-native))))
-
-(defmethod load "cards"
- [path context]
- (let [paths (list-dirs path)
- touched-card-ids (maybe-upsert-many!
- context Card
- (for [card (slurp-many paths)]
- (resolve-card card context)))]
+ :template-tags
+ not-empty) (resolve-native))))
+(defn- make-dummy-card
+ "Make a dummy card for first pass insertion"
+ [card]
+ (-> card
+ (assoc :dataset_query {:type :native
+ :native {:query "-- DUMMY QUERY FOR SERIALIZATION FIRST PASS INSERT"}
+ :database (:database_id card)})
+ (dissoc ::unresolved-names)))
+
+(defn load-cards
+ "Loads cards in a given `context`, from a given sequence of `paths` (strings). If specified, then `only-cards` (maps
+ having the structure of cards loaded from YAML dumps) will be used instead of loading data from `paths` (to serve as
+ a retry mechanism)."
+ {:added "0.40.0"}
+ [context paths only-cards]
+ (let [cards (or only-cards (slurp-many paths))
+ resolved-cards (for [card cards]
+ (resolve-card card context))
+ grouped-cards (reduce-kv
+ (fn [acc idx card]
+ (if (::unresolved-names card)
+ (-> acc
+ (update ::revisit #(conj % card))
+ (update ::revisit-index #(conj % idx)))
+ (update acc ::process #(conj % card))))
+ {::revisit [] ::revisit-index #{} ::process []}
+ (vec resolved-cards))
+ dummy-insert-cards (not-empty (::revisit grouped-cards))
+ process-cards (::process grouped-cards)
+ touched-card-ids (maybe-upsert-many!
+ context Card
+ process-cards)]
(maybe-fixup-card-template-ids!
(assoc context :mode :update)
Card
(for [card (slurp-many paths)] (resolve-card card (assoc context :mode :update)))
touched-card-ids)
- ;; Nested cards
- (doseq [path paths]
- (load (str path "/cards") context))))
+ (if dummy-insert-cards
+ (let [dummy-inserted-ids (maybe-upsert-many!
+ context
+ Card
+ (map make-dummy-card dummy-insert-cards))
+ id-and-cards (map vector dummy-insert-cards dummy-inserted-ids)
+ retry-info-fn (fn [[card card-id]]
+ (unresolved-names->string card card-id))]
+ (log/infof
+ "Unresolved references found for cards in collection %d; will reload after first pass%n%s%n"
+ (:collection context)
+ (str/join "\n" (map retry-info-fn id-and-cards)))
+ (fn []
+ (log/infof "Attempting to reload cards in collection %d" (:collection context))
+ (let [revisit-indexes (::revisit-index grouped-cards)]
+ (load-cards context paths (mapv (partial nth cards) revisit-indexes))))))))
+
+(defmethod load "cards"
+ [path context]
+ (binding [names/*suppress-log-name-lookup-exception* true]
+ (load-cards context (list-dirs path) nil)))
(defmethod load "users"
[path context]
;; Currently we only serialize the new owner user, so it's fine to ignore mode setting
(maybe-upsert-many! context User
(for [user (slurp-dir path)]
- (assoc user :password "changeme"))))
+ (dissoc user :password))))
(defn- derive-location
[context]
@@ -348,24 +648,58 @@
(str (-> parent-id Collection :location) parent-id "/")
"/"))
+(defn- make-reload-fn [all-results]
+ (let [all-fns (filter fn? all-results)]
+ (if-not (empty? all-fns)
+ (let [new-fns (doall all-fns)]
+ (fn []
+ (make-reload-fn (for [reload-fn new-fns]
+ (reload-fn))))))))
+
+(defn- load-collections
+ [path context]
+ (let [subdirs (list-dirs path)
+ by-ns (group-by #(let [[_ coll-ns] (re-matches #".*/:([^:/]+)" %)]
+ coll-ns)
+ subdirs)
+ grouped (group-by (comp nil? first) by-ns)
+ ns-paths (get grouped false)
+ entity-paths (->> (get grouped true)
+ (map last)
+ first)
+ results (for [path entity-paths]
+ (let [context (assoc context
+ :collection (->> (slurp-dir path)
+ (map #(assoc % :location (derive-location context)
+ :namespace (-> context
+ :collection-namespace)))
+ (maybe-upsert-many! context Collection)
+ first))]
+ (log/infof "Processing collection at path %s" path)
+ [(load (str path "/collections") context)
+ (load (str path "/cards") context)
+ (load (str path "/pulses") context)
+ (load (str path "/dashboards") context)
+ (load (str path "/snippets") context)]))
+ load-ns-fns (for [[coll-ns [coll-ns-path]] ns-paths]
+ (do (log/infof "Loading %s namespace for collection at path %s" coll-ns coll-ns-path)
+ (load-collections coll-ns-path (assoc context :collection-namespace coll-ns))))]
+ (make-reload-fn (concat (apply concat results) ; these are each sequences, so need to flatten those first
+ load-ns-fns))))
+
(defmethod load "collections"
[path context]
- (let [res-fns (for [path (list-dirs path)]
- (let [context (assoc context
- :collection (->> (slurp-dir path)
- (map (fn [collection]
- (assoc collection :location (derive-location context))))
- (maybe-upsert-many! context Collection)
- first))]
- (filter fn? [(load (str path "/collections") context)
- (load (str path "/cards") context)
- (load (str path "/pulses") context)
- (load (str path "/dashboards") context)])))
- all-fns (apply concat res-fns)]
- (if-not (empty? all-fns)
- (fn []
- (doseq [reload-fn all-fns]
- (reload-fn))))))
+ (load-collections path context))
+
+(defn- prepare-snippet [context snippet]
+ (assoc snippet :creator_id @default-user
+ :collection_id (:collection context)))
+
+(defmethod load "snippets"
+ [path context]
+ (let [paths (list-dirs path)
+ snippets (map (partial prepare-snippet context) (slurp-many paths))]
+ (maybe-upsert-many! context NativeQuerySnippet snippets)))
(defn load-settings
"Load a dump of settings."
diff --git a/enterprise/backend/src/metabase_enterprise/serialization/names.clj b/enterprise/backend/src/metabase_enterprise/serialization/names.clj
index 4c3440c5d4e44671ff11722fcd5ca432acdef0f4..0cb368e869cfbc6af1ed8f40a9b0c2056d3b3de0 100644
--- a/enterprise/backend/src/metabase_enterprise/serialization/names.clj
+++ b/enterprise/backend/src/metabase_enterprise/serialization/names.clj
@@ -9,17 +9,19 @@
[metabase.models.database :as database :refer [Database]]
[metabase.models.field :refer [Field]]
[metabase.models.metric :refer [Metric]]
+ [metabase.models.native-query-snippet :refer [NativeQuerySnippet]]
[metabase.models.pulse :refer [Pulse]]
[metabase.models.segment :refer [Segment]]
[metabase.models.table :refer [Table]]
[metabase.models.user :refer [User]]
- [metabase.query-processor.util :as qp.util]
[metabase.util.i18n :as i18n :refer [trs]]
[metabase.util.schema :as su]
[ring.util.codec :as codec]
[schema.core :as s]
[toucan.db :as db]))
+(def ^:private root-collection-path "/collections/root")
+
(defn safe-name
"Return entity name URL encoded except that spaces are retained."
[entity]
@@ -69,47 +71,54 @@
[segment]
(str (->> segment :table_id (fully-qualified-name Table)) "/segments/" (safe-name segment)))
+(defn- local-collection-name [collection]
+ (let [ns-part (if-let [coll-ns (:namespace collection)]
+ (str ":" (if (keyword? coll-ns) (name coll-ns) coll-ns) "/"))]
+ (str "/collections/" ns-part (safe-name collection))))
+
(defmethod fully-qualified-name* (type Collection)
[collection]
(let [parents (some->> (str/split (:location collection) #"/")
rest
not-empty
- (map #(-> % Integer/parseInt Collection safe-name (str "/collections")))
- (str/join "/")
- (format "%s/"))]
- (str "/collections/root/collections/" parents (safe-name collection))))
+ (map #(-> % Integer/parseInt Collection local-collection-name))
+ (apply str))]
+ (str root-collection-path parents (local-collection-name collection))))
(defmethod fully-qualified-name* (type Dashboard)
[dashboard]
(format "%s/dashboards/%s"
(or (some->> dashboard :collection_id (fully-qualified-name Collection))
- "/collections/root")
+ root-collection-path)
(safe-name dashboard)))
(defmethod fully-qualified-name* (type Pulse)
[pulse]
(format "%s/pulses/%s"
(or (some->> pulse :collection_id (fully-qualified-name Collection))
- "/collections/root")
+ root-collection-path)
(safe-name pulse)))
(defmethod fully-qualified-name* (type Card)
[card]
(format "%s/cards/%s"
(or (some->> card
- :dataset_query
- qp.util/query->source-card-id
- (fully-qualified-name Card))
- (some->> card
:collection_id
(fully-qualified-name Collection))
- "/collections/root")
+ root-collection-path)
(safe-name card)))
(defmethod fully-qualified-name* (type User)
[user]
(str "/users/" (:email user)))
+(defmethod fully-qualified-name* (type NativeQuerySnippet)
+ [snippet]
+ (format "%s/snippets/%s"
+ (or (some->> snippet :collection_id (fully-qualified-name Collection))
+ root-collection-path)
+ (safe-name snippet)))
+
(defmethod fully-qualified-name* nil
[_]
nil)
@@ -126,114 +135,200 @@
(s/optional-key :dashboard) su/IntGreaterThanZero
(s/optional-key :collection) (s/maybe su/IntGreaterThanZero) ; root collection
(s/optional-key :pulse) su/IntGreaterThanZero
- (s/optional-key :user) su/IntGreaterThanZero})
+ (s/optional-key :user) su/IntGreaterThanZero
+ (s/optional-key :snippet) (s/maybe su/IntGreaterThanZero)})
-(defmulti ^:private path->context* (fn [_ model _]
+(defmulti ^:private path->context* (fn [_ model _ _]
model))
-(def ^:private ^{:arglists '([context model entity-name])} path->context
+(def ^:private ^{:arglists '([context model model-attrs entity-name])} path->context
"Extract entities from a logical path."
;(memoize path->context*)
- path->context*
- )
+ path->context*)
+
(defmethod path->context* "databases"
- [context _ db-name]
+ [context _ _ db-name]
(assoc context :database (if (= db-name "__virtual")
mbql.s/saved-questions-virtual-database-id
(db/select-one-id Database :name db-name))))
(defmethod path->context* "schemas"
- [context _ schema]
+ [context _ _ schema]
(assoc context :schema schema))
(defmethod path->context* "tables"
- [context _ table-name]
+ [context _ _ table-name]
(assoc context :table (db/select-one-id Table
:db_id (:database context)
:schema (:schema context)
:name table-name)))
(defmethod path->context* "fields"
- [context _ field-name]
+ [context _ _ field-name]
(assoc context :field (db/select-one-id Field
:table_id (:table context)
:name field-name)))
(defmethod path->context* "fks"
- [context _ field-name]
- (path->context* context "fields" field-name))
+ [context _ _ field-name]
+ (path->context* context "fields" nil field-name))
(defmethod path->context* "metrics"
- [context _ metric-name]
+ [context _ _ metric-name]
(assoc context :metric (db/select-one-id Metric
:table_id (:table context)
:name metric-name)))
(defmethod path->context* "segments"
- [context _ segment-name]
+ [context _ _ segment-name]
(assoc context :segment (db/select-one-id Segment
:table_id (:table context)
:name segment-name)))
(defmethod path->context* "collections"
- [context _ collection-name]
+ [context _ model-attrs collection-name]
(if (= collection-name "root")
(assoc context :collection nil)
(assoc context :collection (db/select-one-id Collection
- :name collection-name
- :location (or (some-> context
- :collection
- Collection
- :location
- (str (:collection context) "/"))
- "/")))))
+ :name collection-name
+ :namespace (:namespace model-attrs)
+ :location (or (some-> context
+ :collection
+ Collection
+ :location
+ (str (:collection context) "/"))
+ "/")))))
(defmethod path->context* "dashboards"
- [context _ dashboard-name]
+ [context _ _ dashboard-name]
(assoc context :dashboard (db/select-one-id Dashboard
:collection_id (:collection context)
:name dashboard-name)))
(defmethod path->context* "pulses"
- [context _ pulse-name]
+ [context _ _ pulse-name]
(assoc context :dashboard (db/select-one-id Pulse
:collection_id (:collection context)
:name pulse-name)))
(defmethod path->context* "cards"
- [context _ dashboard-name]
+ [context _ _ dashboard-name]
(assoc context :card (db/select-one-id Card
:collection_id (:collection context)
:name dashboard-name)))
(defmethod path->context* "users"
- [context _ email]
+ [context _ _ email]
(assoc context :user (db/select-one-id User
:email email)))
+(defmethod path->context* "snippets"
+ [context _ _ snippet-name]
+ (assoc context :snippet (db/select-one-id NativeQuerySnippet
+ :collection_id (:collection context)
+ :name snippet-name)))
+
(def ^:private separator-pattern #"\/")
+(def ^:dynamic *suppress-log-name-lookup-exception*
+ "Dynamic boolean var that controls whether warning messages will NOT be logged on a failed name lookup (from within
+ `fully-qualified-name->context`). Intended to be bound differently in first pass (i.e. set to true), where we expect
+ some name lookups to fail, in order to avoid polluting the log. On subsequent rounds (i.e. reload fns) then it should
+ be left off because we wouldn't expect to have failed lookups then."
+ false)
+
+(defn fully-qualified-field-name?
+ "Returns true if the given `field-name` is a fully-qualified field name for serialization purposes (as opposed to a
+ reference to an in-query alias or some other form)."
+ [field-name]
+ (and (some? field-name)
+ (str/starts-with? field-name "/databases/")
+ (or (str/includes? field-name "/fks/") (str/includes? field-name "/fields/"))))
+
+(defn fully-qualified-table-name?
+ "Returns true if the given `table-name` is a fully-qualified table name for serialization purposes (as opposed to a
+ reference to a card)."
+ [table-name]
+ (and (some? table-name)
+ (string? table-name)
+ (str/starts-with? table-name "/databases/")
+ (not (str/starts-with? table-name "card__"))))
+
+(defn fully-qualified-card-name?
+ "Returns true if the given `card-name` is a fully-qualified card name for serialization purposes."
+ [card-name]
+ (and (some? card-name)
+ (string? card-name)
+ (str/starts-with? card-name "/collections/root/")
+ (str/includes? card-name "/cards/")))
+
+;; WARNING: THIS MUST APPEAR AFTER ALL path->context* IMPLEMENTATIONS
+(def ^:private all-entities (-> path->context*
+ methods
+ keys
+ set))
+
+(defn- partition-name-components
+ "This is more complicated than it needs to be due to potential clashes between an entity name (ex: a table called
+ \"users\" and a model name (ex: \"users\"). Could fix in a number of ways, including special prefix of model names,
+ but that would require changing the format and updating all the `defmethod` calls."
+ ([name-comps]
+ (partition-name-components {::name-components [] ::current-component []} name-comps))
+ ([acc [c & more-comps]]
+ (cond
+ (nil? more-comps)
+ (conj (::name-components acc) (conj (::current-component acc) c))
+
+ (::prev-model-name? acc)
+ (if (= \: (first c))
+ (partition-name-components (update acc ::current-component conj c) more-comps)
+ (partition-name-components (-> (assoc acc ::prev-model-name? false)
+ (update ::current-component
+ conj
+ c))
+ more-comps))
+
+ (contains? all-entities c)
+ (partition-name-components (cond-> (assoc acc ::prev-model-name? true
+ ::current-component [c])
+ (not-empty (::current-component acc))
+ (update ::name-components conj (::current-component acc)))
+ more-comps))))
+
(defn fully-qualified-name->context
- "Parse a logcial path into a context map."
+ "Parse a logical path into a context map."
[fully-qualified-name]
(when fully-qualified-name
- (let [context (->> (str/split fully-qualified-name separator-pattern)
- rest ; we start with a /
- (partition 2)
- (reduce (fn [context [model entity-name]]
- (path->context context model (unescape-name entity-name)))
- {}))]
+ (let [components (->> (str/split fully-qualified-name separator-pattern)
+ rest ; we start with a /
+ partition-name-components
+ (map (fn [[model-name & entity-parts]]
+ (cond-> {::model-name model-name ::entity-name (last entity-parts)}
+ (and (= "collections" model-name) (> (count entity-parts) 1))
+ (assoc :namespace (->> entity-parts
+ first ; ns is first/only item after "collections"
+ rest ; strip the starting :
+ (apply str)))))))
+ context (loop [acc-context {}
+ [{:keys [::model-name ::entity-name] :as model-map} & more] components]
+ (let [model-attrs (dissoc model-map ::model-name ::entity-name)
+ new-context (path->context acc-context model-name model-attrs (unescape-name entity-name))]
+ (if (empty? more)
+ new-context
+ (recur new-context more))))]
(try
(s/validate (s/maybe Context) context)
(catch Exception e
- (log/warn
- (ex-info (trs "Can''t resolve {0} in fully qualified name {1}"
- (str/join ", " (map name (keys (:value (ex-data e)))))
- fully-qualified-name)
- {:fully-qualified-name fully-qualified-name
- :resolve-name-failed? true
- :context context})))))))
+ (when-not *suppress-log-name-lookup-exception*
+ (log/warn
+ (ex-info (trs "Can''t resolve {0} in fully qualified name {1}"
+ (str/join ", " (map name (keys (:value (ex-data e)))))
+ fully-qualified-name)
+ {:fully-qualified-name fully-qualified-name
+ :resolve-name-failed? true
+ :context context}
+ e))))))))
(defn name-for-logging
"Return a string representation of entity suitable for logs"
diff --git a/enterprise/backend/src/metabase_enterprise/serialization/serialize.clj b/enterprise/backend/src/metabase_enterprise/serialization/serialize.clj
index c5bc4055e494788df50e1e6997fccc3e8295b8bd..d02916dec508112843ffa54efb46f1f0244ca4b6 100644
--- a/enterprise/backend/src/metabase_enterprise/serialization/serialize.clj
+++ b/enterprise/backend/src/metabase_enterprise/serialization/serialize.clj
@@ -15,12 +15,14 @@
[metabase.models.dimension :refer [Dimension]]
[metabase.models.field :as field :refer [Field]]
[metabase.models.metric :refer [Metric]]
+ [metabase.models.native-query-snippet :refer [NativeQuerySnippet]]
[metabase.models.pulse :refer [Pulse]]
[metabase.models.pulse-card :refer [PulseCard]]
[metabase.models.pulse-channel :refer [PulseChannel]]
[metabase.models.segment :refer [Segment]]
[metabase.models.table :refer [Table]]
[metabase.models.user :refer [User]]
+ [metabase.shared.models.visualization-settings :as mb.viz]
[metabase.util :as u]
[toucan.db :as db]))
@@ -28,11 +30,12 @@
"Current serialization protocol version.
This gets stored with each dump, so we can correctly recover old dumps."
- 1)
+ ;; version 2 - start adding namespace portion to /collections/ paths
+ 2)
(def ^:private ^{:arglists '([form])} mbql-entity-reference?
"Is given form an MBQL entity reference?"
- (partial mbql.normalize/is-clause? #{:field-id :fk-> :metric :segment}))
+ (partial mbql.normalize/is-clause? #{:field :field-id :fk-> :metric :segment}))
(defn- mbql-id->fully-qualified-name
[mbql]
@@ -41,7 +44,17 @@
(mbql.util/replace
;; `integer?` guard is here to make the operation idempotent
[:field (id :guard integer?) opts]
- [:field (fully-qualified-name Field id) opts]
+ [:field (fully-qualified-name Field id) (mbql-id->fully-qualified-name opts)]
+
+ ;; field-id is still used within parameter mapping dimensions
+ ;; example relevant clause - [:dimension [:fk-> [:field-id 1] [:field-id 2]]]
+ [:field-id (id :guard integer?)]
+ [:field-id (fully-qualified-name Field id)]
+
+ ;; source-field is also used within parameter mapping dimensions
+ ;; example relevant clause - [:field 2 {:source-field 1}]
+ {:source-field (id :guard integer?)}
+ (assoc &match :source-field (fully-qualified-name Field id))
[:metric (id :guard integer?)]
[:metric (fully-qualified-name Metric id)]
@@ -71,6 +84,14 @@
second
Integer/parseInt))
(fully-qualified-name Table source-table))))
+ (m/update-existing entity :breakout (fn [breakout]
+ (map mbql-id->fully-qualified-name breakout)))
+ (m/update-existing entity :aggregation (fn [aggregation]
+ (m/map-vals mbql-id->fully-qualified-name aggregation)))
+ (m/update-existing entity :filter (fn [filter]
+ (m/map-vals mbql-id->fully-qualified-name filter)))
+ (m/update-existing entity ::mb.viz/param-mapping-source (partial fully-qualified-name Field))
+ (m/update-existing entity :snippet-id (partial fully-qualified-name NativeQuerySnippet))
(m/map-vals ids->fully-qualified-names entity))))
(defn- strip-crud
@@ -108,12 +129,67 @@
field/values
(u/select-non-nil-keys [:values :human_readable_values]))))))
+(defn- convert-column-settings-key [k]
+ (if-let [field-id (::mb.viz/field-id k)]
+ (-> (db/select-one Field :id field-id)
+ fully-qualified-name
+ mb.viz/field-str->column-ref)
+ k))
+
+(defn- convert-param-mapping-key
+ "The `k` is something like [:dimension [:fk-> [:field-id <id1>] [:field-id <id2]]]"
+ [k]
+ (mbql-id->fully-qualified-name k))
+
+(defn- convert-param-ref [new-id param-ref]
+ (cond-> param-ref
+ (= "dimension" (::mb.viz/param-ref-type param-ref)) ids->fully-qualified-names
+ (some? new-id) (update ::mb.viz/param-ref-id new-id)))
+
+(defn- convert-param-mapping-val [new-id v]
+ (-> v
+ (m/update-existing ::mb.viz/param-mapping-source (partial convert-param-ref new-id))
+ (m/update-existing ::mb.viz/param-mapping-target (partial convert-param-ref new-id))
+ (m/assoc-some ::mb.viz/param-mapping-id (or new-id (::mb.viz/param-mapping-id v)))))
+
+(defn- convert-parameter-mapping [param-mapping]
+ (if (nil? param-mapping)
+ nil
+ (reduce-kv (fn [acc k v]
+ (assoc acc (convert-param-mapping-key k)
+ (convert-param-mapping-val nil v))) {} param-mapping)))
+
+(defn- convert-click-behavior [{:keys [::mb.viz/link-type ::mb.viz/link-target-id] :as click}]
+ (-> (if-let [new-target-id (case link-type
+ ::mb.viz/card (-> (Card link-target-id)
+ fully-qualified-name)
+ ::mb.viz/dashboard (-> (Dashboard link-target-id)
+ fully-qualified-name)
+ nil)]
+ (assoc click ::mb.viz/link-target-id new-target-id)
+ click)
+ (m/update-existing ::mb.viz/parameter-mapping convert-parameter-mapping)))
+
+(defn- convert-column-settings-value [{:keys [::mb.viz/click-behavior] :as v}]
+ (cond (not-empty click-behavior) (assoc v ::mb.viz/click-behavior (convert-click-behavior click-behavior))
+ :else v))
+
+(defn- convert-column-settings [acc k v]
+ (assoc acc (convert-column-settings-key k) (convert-column-settings-value v)))
+
+(defn- convert-viz-settings [viz-settings]
+ (-> (mb.viz/db->norm viz-settings)
+ (m/update-existing ::mb.viz/column-settings (fn [col-settings]
+ (reduce-kv convert-column-settings {} col-settings)))
+ (m/update-existing ::mb.viz/click-behavior convert-click-behavior)
+ mb.viz/norm->db))
+
(defn- dashboard-cards-for-dashboard
[dashboard]
- (let [dashboard-cards (db/select DashboardCard :dashboard_id (u/the-id dashboard))
- series (when (not-empty dashboard-cards)
- (db/select DashboardCardSeries
- :dashboardcard_id [:in (map u/the-id dashboard-cards)]))]
+ (let [dashboard-cards (db/select DashboardCard :dashboard_id (u/the-id dashboard))
+ series (when (not-empty dashboard-cards)
+ (db/select DashboardCardSeries
+ :dashboardcard_id [:in (map u/the-id dashboard-cards)]))]
(for [dashboard-card dashboard-cards]
(-> dashboard-card
(assoc :series (for [series series
@@ -121,6 +197,7 @@
(-> series
(update :card_id (partial fully-qualified-name Card))
(dissoc :id :dashboardcard_id))))
+ (assoc :visualization_settings (convert-viz-settings (:visualization_settings dashboard-card)))
strip-crud))))
(defmethod serialize-one (type Dashboard)
@@ -159,3 +236,7 @@
(select-keys [:dependent_on_id :model_id])
(update :dependent_on_id (partial fully-qualified-name (-> dependency :dependent_on_model symbol)))
(update :model_id (partial fully-qualified-name (-> dependency :model symbol)))))
+
+(defmethod serialize-one (type NativeQuerySnippet)
+ [snippet]
+ (select-keys snippet [:name :description :content]))
diff --git a/enterprise/backend/src/metabase_enterprise/serialization/specs.clj b/enterprise/backend/src/metabase_enterprise/serialization/specs.clj
deleted file mode 100644
index 2d08b79d09b18725069347cb5d025e1ba162d563..0000000000000000000000000000000000000000
--- a/enterprise/backend/src/metabase_enterprise/serialization/specs.clj
+++ /dev/null
@@ -1,9 +0,0 @@
-(ns metabase-enterprise.serialization.specs
- "Shared specs for serialization related code."
- (:require [clojure.spec.alpha :as s]))
-
-;; a structure that represents items that can be loaded
-;; (s/def ::load-items (s/cat ::context map? ::model some? ::entity-dir string? ::entity-names (s/+ string?)))
-
-;; a structure that represents some items having failed to load; items are fully qualified entity names
-(s/def ::failed-name-resolution (s/map-of string? (s/+ string?)))
diff --git a/enterprise/backend/src/metabase_enterprise/serialization/upsert.clj b/enterprise/backend/src/metabase_enterprise/serialization/upsert.clj
index dc7a3ea121eaa414f3d0eae46069735a3229af88..061c53a3f7b4a7b80801c6fc284140f53eb05e98 100644
--- a/enterprise/backend/src/metabase_enterprise/serialization/upsert.clj
+++ b/enterprise/backend/src/metabase_enterprise/serialization/upsert.clj
@@ -16,6 +16,7 @@
[metabase.models.field :refer [Field]]
[metabase.models.field-values :refer [FieldValues]]
[metabase.models.metric :refer [Metric]]
+ [metabase.models.native-query-snippet :refer [NativeQuerySnippet]]
[metabase.models.pulse :refer [Pulse]]
[metabase.models.pulse-card :refer [PulseCard]]
[metabase.models.pulse-channel :refer [PulseChannel]]
@@ -29,14 +30,14 @@
[toucan.models :as models]))
(def ^:private identity-condition
- {Database [:name]
+ {Database [:name :engine]
Table [:schema :name :db_id]
Field [:name :table_id]
Metric [:name :table_id]
Segment [:name :table_id]
- Collection [:name :location]
+ Collection [:name :location :namespace]
Dashboard [:name :collection_id]
- DashboardCard [:card_id :dashboard_id :visualiation_settings]
+ DashboardCard [:card_id :dashboard_id :visualization_settings]
DashboardCardSeries [:dashboardcard_id :card_id]
FieldValues [:field_id]
Dimension [:field_id :human_readable_field_id]
@@ -46,7 +47,8 @@
PulseCard [:pulse_id :card_id]
PulseChannel [:pulse_id :channel_type :details]
Card [:name :collection_id]
- User [:email]})
+ User [:email]
+ NativeQuerySnippet [:name :collection_id]})
;; This could potentially be unrolled into one giant select
(defn- select-identical
@@ -70,7 +72,7 @@
`(try
(do ~@body)
(catch Throwable e#
- (log/error (u/format-color 'red "%s: %s" ~message (.getMessage e#)))
+ (log/error e# (u/format-color 'red "%s: %s" ~message (.getMessage e#)))
nil)))
(defn- insert-many-individually!
diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/api/permissions_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/api/permissions_test.clj
new file mode 100644
index 0000000000000000000000000000000000000000..571bce23c0e4cb63daf8f586912dc83315e98b4e
--- /dev/null
+++ b/enterprise/backend/test/metabase_enterprise/sandbox/api/permissions_test.clj
@@ -0,0 +1,132 @@
+(ns metabase-enterprise.sandbox.api.permissions-test
+ (:require [clojure.test :refer :all]
+ [metabase-enterprise.sandbox.models.group-table-access-policy :refer [GroupTableAccessPolicy]]
+ [metabase.models :refer [Database PermissionsGroup Table]]
+ [metabase.models.permissions-group :as group]
+ [metabase.test :as mt]
+ [metabase.util :as u]
+ [metabase.util.schema :as su]
+ [schema.core :as s]
+ [toucan.db :as db]))
+
+(defn- id->keyword [id]
+ (keyword (str (u/the-id id))))
+
+(defn- db-graph-keypath [group]
+ [:groups (id->keyword group) (id->keyword (mt/id))])
+
+(defn- venues-perms-graph-keypath [group]
+ (concat
+ (db-graph-keypath group)
+ [:schemas :PUBLIC (id->keyword (mt/id :venues))]))
+
+(deftest revoke-perms-delete-gtaps-test
+ (testing "PUT /api/permissions/graph"
+ (testing "removing sandboxed permissions for a group should delete the associated GTAP (#16190)"
+ (doseq [[message {:keys [updated-db-perms expected-perms]}]
+ {"when revoking all permissions for DB"
+ {:updated-db-perms (constantly {:native :none, :schemas :none})
+ :expected-perms (constantly nil)}
+
+ "when revoking all permissions for schema"
+ {:updated-db-perms (constantly {:native :none, :schemas {:PUBLIC :none}})
+ :expected-perms (constantly nil)}
+
+ "when revoking all permissions for Table"
+ {:updated-db-perms (fn []
+ {:native :none, :schemas {:PUBLIC {(id->keyword (mt/id :venues)) :none
+ (id->keyword (mt/id :users)) :all}}})
+ :expected-perms (fn []
+ {:schemas {:PUBLIC {(id->keyword (mt/id :users)) "all"}}})}
+
+ "when revoking segmented query permissions for Table"
+ {:updated-db-perms (fn []
+ {:native :none, :schemas {:PUBLIC {(id->keyword (mt/id :venues)) {:read :all}}}})
+ :expected-perms (fn []
+ {:schemas {:PUBLIC {(id->keyword (mt/id :venues)) {:read "all"}}}})}
+
+ "when changing permissions for DB to unrestricted access"
+ {:updated-db-perms (constantly {:native :none, :schemas :all})
+ :expected-perms (constantly {:schemas "all"})}
+
+ "when changing permissions for schema to unrestricted access"
+ {:updated-db-perms (constantly {:native :none, :schemas {:PUBLIC :all}})
+ :expected-perms (constantly {:schemas {:PUBLIC "all"}})}
+
+ "when changing permissions for Table to :query [grant full query perms]"
+ {:updated-db-perms (fn []
+ {:native :none, :schemas {:PUBLIC {(id->keyword (mt/id :venues)) {:query :all}}}})
+ :expected-perms (fn []
+ {:schemas {:PUBLIC {(id->keyword (mt/id :venues)) {:query "all"}}}})}}]
+ (mt/with-gtaps {:gtaps {:venues {}}}
+ (testing message
+ (testing "sanity check"
+ (testing "perms graph endpoint should return segmented perms for Venues table"
+ (is (= {:query "segmented"}
+ (get-in (mt/user-http-request :crowberto :get 200 "permissions/graph")
+ (venues-perms-graph-keypath &group)))))
+ (testing "GTAP should exist in application DB"
+ (is (schema= [(s/one {:id su/IntGreaterThanZero
+ :group_id (s/eq (u/the-id &group))
+ :table_id (s/eq (mt/id :venues))
+ :card_id (s/eq nil)
+ :attribute_remappings (s/eq nil)
+ s/Keyword s/Any}
+ "GTAP")]
+ (db/select GroupTableAccessPolicy :group_id (u/the-id &group))))))
+ (let [graph (mt/user-http-request :crowberto :get 200 "permissions/graph")
+ graph' (assoc-in graph (db-graph-keypath &group) (updated-db-perms))
+ response (mt/user-http-request :crowberto :put 200 "permissions/graph" graph')]
+ (mt/with-temp* [Database [db-2]
+ Table [db-2-table {:db_id (u/the-id db-2)}]
+ GroupTableAccessPolicy [_ {:group_id (u/the-id &group)
+ :table_id (u/the-id db-2-table)}]
+ PermissionsGroup [other-group]
+ GroupTableAccessPolicy [_ {:group_id (u/the-id other-group)
+ :table_id (mt/id :venues)}]]
+ (testing "perms graph should be updated"
+ (testing "in API request response"
+ (is (= (expected-perms)
+ (get-in response (db-graph-keypath &group)))))
+ (testing "on subsequent fetch of the graph"
+ (is (= (expected-perms)
+ (get-in (mt/user-http-request :crowberto :get 200 "permissions/graph")
+ (db-graph-keypath &group))))))
+ (testing "GTAP should be deleted from application DB"
+ (is (= []
+ (db/select GroupTableAccessPolicy
+ :group_id (u/the-id &group)
+ :table_id (mt/id :venues)))))
+ (testing "GTAP for same group, other database should not be affected"
+ (is (schema= [(s/one {:id su/IntGreaterThanZero
+ :group_id (s/eq (u/the-id &group))
+ :table_id (s/eq (u/the-id db-2-table))
+ :card_id (s/eq nil)
+ :attribute_remappings (s/eq nil)}
+ "GTAP")]
+ (db/select GroupTableAccessPolicy
+ :group_id (u/the-id &group)
+ :table_id (u/the-id db-2-table)))))
+ (testing "GTAP for same table, other group should not be affected"
+ (is (schema= [(s/one {:id su/IntGreaterThanZero
+ :group_id (s/eq (u/the-id other-group))
+ :table_id (s/eq (mt/id :venues))
+ :card_id (s/eq nil)
+ :attribute_remappings (s/eq nil)}
+ "GTAP")]
+ (db/select GroupTableAccessPolicy :group_id (u/the-id other-group)))))))))))))
+
+(deftest grant-sandbox-perms-dont-delete-gtaps-test
+ (testing "PUT /api/permissions/graph"
+ (testing "granting sandboxed permissions for a group should *not* delete an associated GTAP (#16190)"
+ (mt/with-temp-copy-of-db
+ (mt/with-temp GroupTableAccessPolicy [_ {:group_id (u/the-id (group/all-users))
+ :table_id (mt/id :venues)}]
+ (let [graph (mt/user-http-request :crowberto :get 200 "permissions/graph")
+ graph' (assoc-in graph (db-graph-keypath (group/all-users)) {:schemas
+ {"PUBLIC"
+ {(id->keyword (mt/id :venues))
+ {:read :all, :query :segmented}}}})]
+ (mt/user-http-request :crowberto :put 200 "permissions/graph" graph')
+ (testing "GTAP should not have been deleted"
+ (is (db/exists? GroupTableAccessPolicy :group_id (u/the-id (group/all-users)), :table_id (mt/id :venues))))))))))
diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/test_util.clj b/enterprise/backend/test/metabase_enterprise/sandbox/test_util.clj
index 9c5c52fb840fdd1abf66d8dbce2d64d2488715a3..f95485ea8effce63a1ca2a62ad9c3a17b467747f 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/test_util.clj
+++ b/enterprise/backend/test/metabase_enterprise/sandbox/test_util.clj
@@ -109,7 +109,9 @@
(mt/with-gtaps {:gtaps {:checkins {:query {:database (data/id), ...}
:remappings {:user_category [\"variable\" ...]}}}
:attributes {\"user_category\" 1}}
- (mt/run-mbql-query checkins {:limit 2}))"
+ (mt/run-mbql-query checkins {:limit 2}))
+
+ Introduces `&group` anaphor, bound to the PermissionsGroup associated with this GTAP."
{:style/indent 1}
[gtaps-and-attributes-map & body]
`(do-with-gtaps-for-user (fn [] ~gtaps-and-attributes-map) :rasta (fn [~'&group] ~@body)))
diff --git a/enterprise/backend/test/metabase_enterprise/serialization/load_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/load_test.clj
index 525839475b075719ad3f721731427792682d4bf1..098b318f0f8df2b0b28fd8535edf1e48f58cf6c4 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/load_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/load_test.clj
@@ -2,16 +2,30 @@
(:refer-clojure :exclude [load])
(:require [clojure.data :as diff]
[clojure.java.io :as io]
- [clojure.test :refer [deftest is]]
+ [clojure.test :refer [deftest is testing use-fixtures]]
[metabase-enterprise.serialization.cmd :refer [dump load]]
[metabase-enterprise.serialization.test-util :as ts]
[metabase.models :refer [Card Collection Dashboard DashboardCard DashboardCardSeries Database Dependency
- Dimension Field FieldValues Metric Pulse PulseCard PulseChannel Segment Table User]]
+ Dimension Field FieldValues Metric NativeQuerySnippet Pulse PulseCard PulseChannel
+ Segment Table User]]
[metabase.test.data.users :as test-users]
[metabase.util :as u]
- [toucan.db :as db])
+ [toucan.db :as db]
+ [metabase.query-processor :as qp]
+ [metabase.query-processor.middleware.permissions :as qp.perms]
+ [metabase.query-processor.store :as qp.store]
+ [metabase.shared.models.visualization-settings :as mb.viz]
+ [metabase.shared.models.visualization-settings-test :as mb.viz-test]
+ [metabase.shared.util.log :as log]
+ [metabase.test :as mt]
+ [metabase.test.fixtures :as fixtures]
+ [metabase.util.i18n :refer [deferred-trs trs]])
(:import org.apache.commons.io.FileUtils))
+(use-fixtures :once
+ mb.viz-test/with-spec-instrumentation-fixture
+ (fixtures/initialize :test-users-personal-collections))
+
(defn- delete-directory!
[file-or-filename]
(FileUtils/deleteDirectory (io/file file-or-filename)))
@@ -21,7 +35,8 @@
(defn- world-snapshot
[]
(into {} (for [model [Database Table Field Metric Segment Collection Dashboard DashboardCard Pulse
- Card DashboardCardSeries FieldValues Dimension Dependency PulseCard PulseChannel User]]
+ Card DashboardCardSeries FieldValues Dimension Dependency PulseCard PulseChannel User
+ NativeQuerySnippet]]
[model (db/select-field :id model)])))
(defmacro with-world-cleanup
@@ -36,39 +51,312 @@
(vector :in)
(db/delete! model# :id)))))))
+(defn- card-query-results [card]
+ (let [query (:dataset_query card)]
+ (binding [qp.perms/*card-id* nil]
+ (qp/process-query (assoc query :async? false) {:card-id (:id card)}))))
+
+(defn- query-res-match
+ "Checks that the queries for a card match between original (pre-dump) and new (after load). For now, just checks the
+ native form, not actual data, since some of the statements don't have an ordering (thus the rows won't be stable)."
+ [orig-results loaded-card]
+ (let [card-name (:name loaded-card)
+ orig-result (get orig-results card-name)]
+ (try
+ (let [new-result (card-query-results loaded-card)]
+ (is (some? orig-result) (format "No original query result found for card: %s" card-name))
+ (is (some? new-result) (format "No results produced for loaded card: %s" card-name))
+ (is
+ (apply = (map #(get-in % [:data :native_form :query]) [orig-result new-result]))
+ "Native query form did not match"))
+ (catch Exception e
+ (let [msg (trs "Failed to execute query for loaded card \"{0}\": {1}" card-name (.getMessage e))]
+ (log/error e msg)
+ ;; TODO: figure out a better way to simply fail with an explicit message
+ (is (nil? e) msg))))))
+
+(defn- collection-name-for-card [card]
+ (let [collection-id (:collection_id card)]
+ (if (nil? collection-id)
+ "root"
+ (db/select-one-field :name Collection :id (:collection_id card)))))
+
+(defn- collection-names-match
+ "Checks that the collection name for a card matches between original (pre-dump) and new (after load)."
+ [collection-names loaded-card]
+ (let [card-name (:name loaded-card)]
+ (is (= (get collection-names card-name) (collection-name-for-card loaded-card)) "Collection name did not match")))
+
+(defn- gather-orig-results
+ "Create a map from card names to their query results"
+ [card-ids]
+ (reduce (fn [acc card-id]
+ (let [card (Card card-id)]
+ (assoc acc (:name card) (card-query-results card))))
+ {}
+ card-ids))
+
+(defn- gather-collections
+ "Create a map from card names to their collection names"
+ [card-ids]
+ (reduce (fn [acc card-id]
+ (let [card (Card card-id)]
+ (assoc acc (:name card) (collection-name-for-card card))))
+ {}
+ card-ids))
+
+(defmulti ^:private assert-loaded-entity
+ (fn [entity fingerprint]
+ (type entity)))
+
+(defmethod assert-loaded-entity (type Card)
+ [card {:keys [query-results collections]}]
+ (query-res-match query-results card)
+ (collection-names-match collections card))
+
+(defn- collection-parent-name [collection]
+ (let [[_ parent-id] (re-matches #".*/(\d+)/$" (:location collection))]
+ (db/select-one-field :name Collection :id (Integer. parent-id))))
+
+(defmethod assert-loaded-entity (type Collection)
+ [collection _]
+ (case (:name collection)
+ "My Nested Collection" (is (= "My Collection" (collection-parent-name collection)))
+ "My Collection" (is (= "/" (:location collection)))
+ "Snippet Collection" (is (= "/" (:location collection)))
+ "Nested Snippet Collection" (is (= "Snippet Collection" (collection-parent-name collection)))
+ "Crowberto's Personal Collection" (is (= "/" (:location collection)))
+ "Nested Personal Collection" (is (= "Crowberto Corv's Personal Collection"
+ (collection-parent-name collection)))
+ "Deeply Nested Personal Collection" (is (= "Nested Personal Collection"
+ (collection-parent-name collection)))
+ "Felicia's Personal Collection" (is false "Should not have loaded different user's PC")
+ "Felicia's Nested Collection" (is false "Should not have loaded different user's PC"))
+ collection)
+
+(defmethod assert-loaded-entity (type NativeQuerySnippet)
+ [snippet {:keys [entities]}]
+ (when-let [orig-snippet (first (filter (every-pred #(= (type NativeQuerySnippet) (type %))
+ #(= (:name snippet) (:name %))) (map last entities)))]
+ (is (some? orig-snippet))
+ (is (= (select-keys orig-snippet [:name :description :content])
+ (select-keys snippet [:name :description :content])))
+ snippet))
+
+(defn- id->name [model id]
+ (db/select-one-field :name model :id id))
+
+(defn- assert-price-click [click target-id]
+ ;; first, it should be pointing to "My Card"
+ (is (= "My Card" (id->name Card target-id)))
+ (let [param-mapping (:parameterMapping click)]
+ ;; also, the parameter mappings should have been preserved
+ (is (not-empty param-mapping))
+ (let [mapping-key (-> param-mapping
+ keys
+ first)
+ mapping-keyname (#'mb.viz/keyname mapping-key)
+ [_ f1-id f2-id] (re-matches #".*\[\"field(?:-id)?\",(\d+),.*\[\"field(?:-id)?\",(\d+),.*" mapping-keyname)
+ f1 (db/select-one Field :id (Integer/parseInt f1-id))
+ f2 (db/select-one Field :id (Integer/parseInt f2-id))
+ dimension (get-in param-mapping [mapping-key :target :dimension])]
+ ;; the source and target fields should be category_id and price, respectively
+ ;; for an explanation of why both cases are allowed, see `case field-nm` below
+ (is (contains? #{"category_id" "CATEGORY_ID"} (:name f1)))
+ (is (contains? #{"price" "PRICE"} (:name f2)))
+ (is (= {:dimension [:field (u/the-id f2) {:source-field (u/the-id f1)}]} dimension)))))
+
+(defmethod assert-loaded-entity (type Dashboard)
+ [dashboard _]
+ (testing "The dashboard card series were loaded correctly"
+ (when (= "My Dashboard" (:name dashboard))
+ (doseq [dashcard (db/select DashboardCard :dashboard_id (u/the-id dashboard))]
+ (doseq [series (db/select DashboardCardSeries :dashboardcard_id (u/the-id dashcard))]
+ ;; check that the linked :card_id matches the expected name for each in the series
+ ;; based on the entities declared in test_util.clj
+ (let [series-pos (:position series)
+ expected-name (case series-pos
+ 0 "My Card"
+ 1 "My Nested Card"
+ 2 ts/root-card-name)]
+ (is (= expected-name (db/select-one-field :name Card :id (:card_id series))))
+ (case series-pos
+ 1
+ (testing "Top level click action was preserved for dashboard card"
+ (let [viz-settings (:visualization_settings dashcard)
+ cb (:click_behavior viz-settings)]
+ (is (not-empty cb))
+ (is (int? (:targetId cb)))
+ (is (= "My Nested Query Card" (db/select-one-field :name Card :id (:targetId cb))))))
+ 2
+ (testing "Column level click actions were preserved for dashboard card"
+ (let [viz-settings (:visualization_settings dashcard)
+ check-click-fn (fn [[col-key col-value]]
+ (let [col-ref (mb.viz/parse-db-column-ref col-key)
+ {:keys [::mb.viz/field-id ::mb.viz/column-name]} col-ref
+ click-bhv (get col-value :click_behavior)
+ target-id (get click-bhv :targetId)]
+ (cond
+ field-id (let [field-nm (id->name Field field-id)]
+ (case field-nm
+ ;; consider either case for these fields, since they are all
+ ;; caps in some (ex: H2) and lowercase in others (ex: Postgres)
+ ;; this is simpler than the alternative (dynamically loading
+ ;; the expected name based on driver)
+ ("price" "PRICE") (assert-price-click click-bhv target-id)
+ ("name" "NAME") (is (=
+ "Root Dashboard"
+ (id->name Dashboard target-id)))
+ ("latitude" "LATITUDE") (is (= {:parameterMapping {}
+ :type "crossfilter"}
+ click-bhv))))
+ column-name
+ (case column-name
+ "Price Known" (is (= {:type "link"
+ :linkType "url"
+ :linkTemplate "/price-info"} click-bhv))))))]
+ (is (not-empty viz-settings))
+ (doall (map check-click-fn (:column_settings viz-settings)))))
+ true))) ; nothing to assert for other series numberst
+ (when-let [virt-card (get-in dashcard [:visualization_settings :virtual_card])]
+ (is (= ts/virtual-card virt-card))
+ (is (= "Textbox Card" (get-in dashcard [:visualization_settings :text])))))))
+ dashboard)
+
+(defmethod assert-loaded-entity (type Pulse)
+ [pulse _]
+ (is (some? pulse))
+ (let [pulse-cards (db/select PulseCard :pulse_id (u/the-id pulse))]
+ (is (= 2 (count pulse-cards)))
+ (is (= #{ts/root-card-name "My Card"}
+ (into #{} (map (partial db/select-one-field :name Card :id) (map :card_id pulse-cards)))))))
+
+(defmethod assert-loaded-entity :default
+ [entity _]
+ entity)
(deftest dump-load-entities-test
(try
;; in case it already exists
(u/ignore-exceptions
(delete-directory! dump-dir))
- (let [fingerprint (ts/with-world
- (dump dump-dir (:email (test-users/fetch-user :crowberto)))
- [[Database (Database db-id)]
- [Table (Table table-id)]
- [Field (Field numeric-field-id)]
- [Field (Field category-field-id)]
- [Collection (Collection collection-id)]
- [Collection (Collection collection-id-nested)]
- [Metric (Metric metric-id)]
- [Segment (Segment segment-id)]
- [Dashboard (Dashboard dashboard-id)]
- [Card (Card card-id)]
- [Card (Card card-arch-id)]
- [Card (Card card-id-root)]
- [Card (Card card-id-nested)]
- [Card (Card card-id-nested-query)]
- [Card (Card card-id-native-query)]
- [DashboardCard (DashboardCard dashcard-id)]
- [DashboardCard (DashboardCard dashcard-with-click-actions)]])]
- (with-world-cleanup
- (load dump-dir {:on-error :abort :mode :skip})
- (doseq [[model entity] fingerprint]
- (is (or (-> entity :name nil?)
- (db/select-one model :name (:name entity))
- (and (-> entity :archived) ; archived card hasn't been dump-loaded
- (= (:name entity) "My Arch Card")))
- (str " failed " (pr-str entity))))
- fingerprint))
+ (mt/test-drivers (-> (mt/normal-drivers-with-feature :basic-aggregations :binning :expressions)
+ ;; We will run this roundtrip test against any database supporting these features ^ except
+ ;; certain ones for specific reasons, outlined below.
+ ;;
+ ;; Being able to support all of these would require some significant amount of effort (either
+ ;; to come up with "generic" native queries that will work on all of them, or else possibly
+ ;; using Metabase itself to spit out the native queries, but even then we would need to splice
+ ;; different strings together (ex: the SELECT clause, but replace the WHERE clause with the
+ ;; subquery from the linked card parameter).
+ ;;
+ ;; Because this feature is just about testing serialization itself, and not yet another test of
+ ;; query processor, this seems like an acceptable tradeoff (ex: if they dump and load to the
+ ;; same native form on one database, then it's likely they would on any, since that is
+ ;; orthogonal to the issues that serialization has when performing this roundtrip).
+ (disj :oracle ; no bare table names allowed
+ :presto ; no bare table names allowed
+ :redshift ; bare table name doesn't work; it's test_data_venues instead of venues
+ :snowflake ; bare table name doesn't work; it's test_data_venues instead of venues
+ :sqlserver ; ORDER BY not allowed not allowed in derived tables (subselects)
+ :vertica ; bare table name doesn't work; it's test_data_venues instead of venues
+ :sqlite ; foreign-keys is not supported by this driver
+ :sparksql)) ; foreign-keys is not supported by this driver
+
+ (let [fingerprint (ts/with-world
+ (qp.store/fetch-and-store-database! db-id)
+ (qp.store/fetch-and-store-tables! [table-id
+ table-id-categories
+ table-id-users
+ table-id-checkins])
+ (dump dump-dir (:email (test-users/fetch-user :crowberto)) {:only-db-ids #{db-id}})
+ {:query-results (gather-orig-results [card-id
+ card-arch-id
+ card-id-root
+ card-id-nested
+ card-id-nested-query
+ card-id-native-query
+ card-id-root-to-collection
+ card-id-collection-to-root
+ card-id-template-tags
+ card-id-filter-agg
+ card-id-temporal-unit
+ card-id-with-native-snippet
+ card-id-temporal-unit
+ card-join-card-id])
+ :collections (gather-collections [card-id
+ card-arch-id
+ card-id-root
+ card-id-nested
+ card-id-nested-query
+ card-id-native-query
+ card-id-root-to-collection
+ card-id-collection-to-root
+ card-id-template-tags
+ card-id-filter-agg
+ card-id-temporal-unit
+ card-id-with-native-snippet
+ card-id-temporal-unit
+ card-join-card-id])
+ :entities [[Database (Database db-id)]
+ [Table (Table table-id)]
+ [Table (Table table-id-categories)]
+ [Table (Table table-id-users)]
+ [Table (Table table-id-checkins)]
+ [Field (Field numeric-field-id)]
+ [Field (Field name-field-id)]
+ [Field (Field category-field-id)]
+ [Field (Field latitude-field-id)]
+ [Field (Field longitude-field-id)]
+ [Field (Field category-pk-field-id)]
+ [Field (Field date-field-id)]
+ [Field (Field user-id-field-id)]
+ [Field (Field users-pk-field-id)]
+ [Field (Field last-login-field-id)]
+ [Collection (Collection collection-id)]
+ [Collection (Collection collection-id-nested)]
+ [Collection (Collection personal-collection-id)]
+ [Collection (Collection pc-nested-id)]
+ [Collection (Collection pc-deeply-nested-id)]
+ [Metric (Metric metric-id)]
+ [Segment (Segment segment-id)]
+ [Dashboard (Dashboard dashboard-id)]
+ [Dashboard (Dashboard root-dashboard-id)]
+ [Card (Card card-id)]
+ [Card (Card card-arch-id)]
+ [Card (Card card-id-root)]
+ [Card (Card card-id-nested)]
+ [Card (Card card-id-nested-query)]
+ [Card (Card card-id-native-query)]
+ [DashboardCard (DashboardCard dashcard-id)]
+ [DashboardCard (DashboardCard dashcard-top-level-click-id)]
+ [DashboardCard (DashboardCard dashcard-with-click-actions)]
+ [Card (Card card-id-root-to-collection)]
+ [Card (Card card-id-collection-to-root)]
+ [Card (Card card-id-template-tags)]
+ [Card (Card card-id-filter-agg)]
+ [Card (Card card-id-temporal-unit)]
+ [Pulse (Pulse pulse-id)]
+ [DashboardCard (DashboardCard dashcard-with-textbox-id)]
+ [NativeQuerySnippet (NativeQuerySnippet snippet-id)]
+ [Collection (Collection snippet-collection-id)]
+ [Collection (Collection snippet-nested-collection-id)]
+ [NativeQuerySnippet (NativeQuerySnippet nested-snippet-id)]
+ [Card (Card card-id-with-native-snippet)]
+ [Card (Card card-join-card-id)]]})]
+ (with-world-cleanup
+ (load dump-dir {:on-error :continue :mode :update})
+ (mt/with-db (db/select-one Database :name ts/temp-db-name)
+ (doseq [[model entity] (:entities fingerprint)]
+ (testing (format "%s \"%s\"" (type model) (:name entity))
+ (is (or (-> entity :name nil?)
+ (if-let [loaded (db/select-one model :name (:name entity))]
+ (assert-loaded-entity loaded fingerprint))
+ (and (-> entity :archived) ; archived card hasn't been dump-loaded
+ (= (:name entity) "My Arch Card"))
+ ;; Rasta's Personal Collection was not loaded
+ (= "Felicia's Personal Collection" (:name entity)))
+ (str " failed " (pr-str entity)))))
+ fingerprint))))
(finally
- (delete-directory! dump-dir))))
+ #_(delete-directory! dump-dir))))
diff --git a/enterprise/backend/test/metabase_enterprise/serialization/names_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/names_test.clj
index 08890716e931ad6eb0d6b5e53f5f1512a760a767..bb9afe31a78544f208e4e17b719b91db558d885e 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/names_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/names_test.clj
@@ -2,8 +2,9 @@
(:require [clojure.test :refer :all]
[metabase-enterprise.serialization.names :as names]
[metabase-enterprise.serialization.test-util :as ts]
- [metabase.models :refer [Card Collection Dashboard Database Field Metric Segment Table]]
- [metabase.util :as u]))
+ [metabase.models :refer [Card Collection Dashboard Database Field Metric NativeQuerySnippet Segment Table]]
+ [metabase.util :as u]
+ [metabase.test :as mt]))
(deftest safe-name-test
(are [s expected] (= (names/safe-name {:name s}) expected)
@@ -32,8 +33,64 @@
(Collection collection-id)
(Collection collection-id-nested)
(Dashboard dashboard-id)
- (Database db-id)]]
+ (Database db-id)
+ (NativeQuerySnippet snippet-id)]]
(testing (class object)
- (let [context (names/fully-qualified-name->context (names/fully-qualified-name object))]
+ (let [context (names/fully-qualified-name->context (names/fully-qualified-name object))
+ id-fn (some-fn :snippet :field :metric :segment :card :dashboard :collection :table :database)]
(is (= (u/the-id object)
- ((some-fn :field :metric :segment :card :dashboard :collection :table :database) context))))))))
+ (id-fn context))))))))
+
+(deftest fully-qualified-name->context-test
+ (testing "fully-qualified-name->context works as expected"
+ (testing " with cards in root and in a collection"
+ (mt/with-temp* [Collection [{collection-id :id :as coll} {:name "A Collection" :location "/"}]
+ Card [root-card {:name "Root Card"}]
+ Card [collection-card {:name "Collection Card"
+ :collection_id collection-id}]
+ Collection [{sub-collection-id :id :as coll2} {:name "Sub Collection"
+ :location (format "/%d/" collection-id)}]
+ Collection [{deep-collection-id :id :as coll3} {:name "Deep Collection"
+ :location (format "/%d/%d/"
+ collection-id
+ sub-collection-id)}]]
+ (let [card1-name "/collections/root/cards/Root Card"
+ card2-name "/collections/root/collections/A Collection/cards/Collection Card"
+ coll-name "/collections/root/collections/A Collection"
+ coll2-name "/collections/root/collections/A Collection/collections/Sub Collection"
+ coll3-name (str "/collections/root/collections/A Collection/collections/Sub Collection/collections"
+ "/Deep Collection")]
+ (is (= card1-name (names/fully-qualified-name root-card)))
+ (is (= card2-name (names/fully-qualified-name collection-card)))
+ (is (= coll-name (names/fully-qualified-name coll)))
+ (is (= coll2-name (names/fully-qualified-name coll2)))
+ (is (= coll3-name (names/fully-qualified-name coll3))))))
+ (testing " with snippets in a collection"
+ (mt/with-temp* [Collection [{base-collection-id :id} {:name "Base Collection"
+ :namespace "snippets"}]
+ Collection [{collection-id :id} {:name "Nested Collection"
+ :location (format "/%s/" base-collection-id)
+ :namespace "snippets"}]
+ NativeQuerySnippet [snippet {:content "price > 2"
+ :name "Price > 2"
+ :description "Price more than 2"
+ :collection_id collection-id}]]
+ (let [fully-qualified-name (str "/collections/root/collections/:snippets/Base Collection/collections"
+ "/:snippets/Nested Collection/snippets/Price %3E 2")]
+ (is (= fully-qualified-name
+ (names/fully-qualified-name snippet)))
+ (is (= {:collection collection-id
+ :snippet (u/the-id snippet)}
+ (names/fully-qualified-name->context fully-qualified-name))))))
+ (testing " with path elements matching one of our entity names"
+ ; these drivers keep table names lowercased, causing "users" table to clash with our entity name "users"
+ (mt/test-drivers #{:postgres :mysql}
+ (ts/with-world
+ (let [users-pk-field (Field users-pk-field-id)
+ fq-name (names/fully-qualified-name users-pk-field)
+ ctx (names/fully-qualified-name->context fq-name)]
+ ;; MySQL doesn't have schemas, so either one of these could be acceptable
+ (is (contains? #{"/databases/Fingerprint test-data copy/tables/users/fields/id"
+ "/databases/Fingerprint test-data copy/schemas/public/tables/users/fields/id"} fq-name))
+ (is (map? ctx))
+ (is (some? (:table ctx)))))))))
diff --git a/enterprise/backend/test/metabase_enterprise/serialization/serialize_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/serialize_test.clj
index 83660babbce211f657bc5f3cfc9feddd3b656f9e..d79f62834f83e1ebd6bbb03512c12e0c4fe0780a 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/serialize_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/serialize_test.clj
@@ -3,7 +3,8 @@
[clojure.test :refer :all]
[metabase-enterprise.serialization.serialize :as serialize]
[metabase-enterprise.serialization.test-util :as ts]
- [metabase.models :refer [Card Collection Dashboard Database Field Metric Segment Table]]))
+ [metabase.models :refer [Card Collection Dashboard Database Field Metric NativeQuerySnippet Segment
+ Table]]))
(defn- all-ids-are-fully-qualified-names?
[m]
@@ -17,7 +18,8 @@
[[_ & ids]]
(testing (format "\nids = %s" (pr-str ids))
(doseq [id ids]
- (is (string? id)))))
+ (cond (map? id) (all-ids-are-fully-qualified-names? id)
+ (some? id) (is (string? id))))))
(deftest serialization-test
(ts/with-world
@@ -28,7 +30,8 @@
[Dashboard dashboard-id]
[Table table-id]
[Field numeric-field-id]
- [Database db-id]]]
+ [Database db-id]
+ [NativeQuerySnippet snippet-id]]]
(testing (name model)
(let [serialization (serialize/serialize (model id))]
(testing (format "\nserialization = %s" (pr-str serialization))
diff --git a/enterprise/backend/test/metabase_enterprise/serialization/test_util.clj b/enterprise/backend/test/metabase_enterprise/serialization/test_util.clj
index 993807bf97b94b8840f45d9c94a26e835d05ecdc..901b44dea159668d62008c65bc543291bd7ca7ea 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/test_util.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/test_util.clj
@@ -1,43 +1,109 @@
(ns metabase-enterprise.serialization.test-util
(:require [metabase-enterprise.serialization.names :as names]
[metabase.models :refer [Card Collection Dashboard DashboardCard DashboardCardSeries Database Field Metric
- Segment Table]]
+ NativeQuerySnippet Pulse PulseCard Segment Table User]]
+ [metabase.models.collection :as collection]
+ [metabase.query-processor.store :as qp.store]
+ [metabase.shared.models.visualization-settings :as mb.viz]
+ [metabase.test :as mt]
[metabase.test.data :as data]
+ [toucan.db :as db]
[toucan.util.test :as tt]
[metabase-enterprise.serialization.names :refer [fully-qualified-name]]))
+(def root-card-name "My Root Card \\ with a/nasty: (*) //n`me ' * ? \" < > | Å Äž")
+(def temp-db-name "Fingerprint test-data copy")
+
+(defn temp-field [from-field-id table-id]
+ (-> from-field-id
+ Field
+ (dissoc :id)
+ (assoc :table_id table-id)))
+
+(defn temp-table [from-tbl-id db-id]
+ (-> from-tbl-id
+ Table
+ (dissoc :id)
+ (update :display_name #(str "Temp " %))
+ (assoc :db_id db-id)))
+
+(def virtual-card {:name nil
+ :display "text"
+ :visualization_settings {}
+ :dataset_query {}
+ :archived false})
+
+(defn crowberto-pc-id
+ "Gets the personal collection ID for :crowberto (needed for tests). Must be public because the `with-world` macro
+ is public."
+ []
+ (db/select-one-field :id Collection :personal_owner_id (mt/user->id :crowberto)))
+
+(defmacro with-temp-dpc
+ "Wraps with-temp*, but binding `*allow-deleting-personal-collections*` to true so that temporary personal collections
+ can still be deleted."
+ [model-bindings & body]
+ `(binding [collection/*allow-deleting-personal-collections* true]
+ (tt/with-temp* ~model-bindings ~@body)))
+
(defmacro with-world
"Run test in the context of a minimal Metabase instance connected to our test database."
[& body]
- `(tt/with-temp* [Database [{~'db-id :id} (into {} (-> (data/id)
- Database
- (dissoc :id :features :name)))]
- Table [{~'table-id :id :as ~'table} (-> (data/id :venues)
- Table
- (dissoc :id)
- (assoc :db_id ~'db-id))]
- Field [{~'numeric-field-id :id} (-> (data/id :venues :price)
- Field
- (dissoc :id)
- (assoc :table_id ~'table-id))]
- Field [{~'category-field-id :id} (-> (data/id :venues :category_id)
- Field
- (dissoc :id)
- (assoc :table_id ~'table-id))]
+ `(with-temp-dpc [Database [{~'db-id :id} (into {:name temp-db-name} (-> (data/id)
+ Database
+ (dissoc :id :features :name)))]
+ Table [{~'table-id :id :as ~'table} (temp-table (data/id :venues) ~'db-id)]
+ Table [{~'table-id-categories :id} (temp-table (data/id :categories) ~'db-id)]
+ Table [{~'table-id-users :id} (temp-table (data/id :users) ~'db-id)]
+ Table [{~'table-id-checkins :id} (temp-table (data/id :checkins) ~'db-id)]
+ Field [{~'venues-pk-field-id :id} (temp-field (data/id :venues :id) ~'table-id)]
+ Field [{~'numeric-field-id :id} (temp-field (data/id :venues :price) ~'table-id)]
+ Field [{~'name-field-id :id} (temp-field (data/id :venues :name) ~'table-id)]
+ Field [{~'latitude-field-id :id} (temp-field (data/id :venues :latitude) ~'table-id)]
+ Field [{~'longitude-field-id :id} (temp-field (data/id :venues :longitude) ~'table-id)]
+ Field [{~'category-field-id :id} (temp-field (data/id :venues :category_id) ~'table-id)]
+ Field [{~'category-pk-field-id :id} (temp-field
+ (data/id :categories :id)
+ ~'table-id-categories)]
+ Field [{~'date-field-id :id} (temp-field (data/id :checkins :date) ~'table-id-checkins)]
+ Field [{~'users-pk-field-id :id} (temp-field (data/id :users :id)
+ ~'table-id-users)]
+ Field [{~'user-id-field-id :id} (-> (temp-field (data/id :checkins :user_id)
+ ~'table-id-checkins)
+ (assoc :fk_target_field_id ~'users-pk-field-id))]
+ Field [{~'checkins->venues-field-id :id} (-> (temp-field (data/id :checkins :venue_id)
+ ~'table-id-checkins)
+ (assoc :fk_target_field_id ~'venues-pk-field-id))]
+ Field [{~'last-login-field-id :id} (temp-field (data/id :users :last_login)
+ ~'table-id-users)]
Collection [{~'collection-id :id} {:name "My Collection"}]
Collection [{~'collection-id-nested :id} {:name "My Nested Collection"
:location (format "/%s/" ~'collection-id)}]
+ User [{~'user-id-temp :id} {:email "felicia@metabase.com"
+ :first_name "Felicia"
+ :last_name "Temp"
+ :password "fiddlesticks"}]
+ Collection [{~'personal-collection-id :id} {:name "Felicia's Personal Collection"
+ :personal_owner_id ~'user-id-temp}]
+ Collection [{~'pc-felicia-nested-id :id} {:name "Felicia's Nested Collection"
+ :location (format "/%d/" ~'personal-collection-id)}]
+ Collection [{~'pc-nested-id :id} {:name "Nested Personal Collection"
+ :location (format "/%d/" (crowberto-pc-id))}]
+ Collection [{~'pc-deeply-nested-id :id} {:name
+ "Deeply Nested Personal Collection"
+ :location
+ (format "/%d/%d/" (crowberto-pc-id) ~'pc-nested-id)}]
Metric [{~'metric-id :id} {:name "My Metric"
:table_id ~'table-id
:definition {:source-table ~'table-id
:aggregation [:sum [:field ~'numeric-field-id nil]]}}]
-
Segment [{~'segment-id :id} {:name "My Segment"
:table_id ~'table-id
:definition {:source-table ~'table-id
:filter [:!= [:field ~'category-field-id nil] nil]}}]
Dashboard [{~'dashboard-id :id} {:name "My Dashboard"
:collection_id ~'collection-id}]
+ Dashboard [{~'root-dashboard-id :id} {:name "Root Dashboard"}]
Card [{~'card-id :id}
{:table_id ~'table-id
:name "My Card"
@@ -45,8 +111,17 @@
:dataset_query {:type :query
:database ~'db-id
:query {:source-table ~'table-id
- :aggregation [:sum [:field-id ~'numeric-field-id]]
- :breakout [[:field-id ~'category-field-id]]}}}]
+ :filter [:= [:field ~'category-field-id nil] 2]
+ :aggregation [:sum [:field ~'numeric-field-id nil]]
+ :breakout [[:field ~'category-field-id nil]]
+ :joins [{:source-table ~'table-id-categories
+ :alias "cat"
+ :fields "all"
+ :condition [:=
+ [:field ~'category-field-id nil]
+ [:field
+ ~'category-pk-field-id
+ {:join-alias "cat"}]]}]}}}]
Card [{~'card-arch-id :id}
{;:archived true
:table_id ~'table-id
@@ -60,10 +135,11 @@
Card [{~'card-id-root :id}
{:table_id ~'table-id
;; https://en.wikipedia.org/wiki/Filename#Reserved_characters_and_words
- :name "My Root Card \\ with a/nasty: (*) //n`me ' * ? \" < > | Å Äž"
+ :name root-card-name
:dataset_query {:type :query
:database ~'db-id
- :query {:source-table ~'table-id}}}]
+ :query {:source-table ~'table-id}
+ :expressions {"Price Known" [:> [:field ~'numeric-field-id nil] 0]}}}]
Card [{~'card-id-nested :id}
{:table_id ~'table-id
:name "My Nested Card"
@@ -82,7 +158,6 @@
{:source-query
{:source-query
{:source-table ~'table-id}}}}}]
-
Card [{~'card-id-native-query :id}
{:query_type :native
:name "My Native Nested Query Card"
@@ -91,7 +166,7 @@
{:type :native
:database ~'db-id
:native
- {:query "{{#1}}"
+ {:query "SELECT * FROM {{#1}} AS subquery"
:template-tags
{"#1"{:id "72461b3b-3877-4538-a5a3-7a3041924517"
:name "#1"
@@ -101,19 +176,176 @@
DashboardCard [{~'dashcard-id :id}
{:dashboard_id ~'dashboard-id
:card_id ~'card-id}]
+ DashboardCard [{~'dashcard-top-level-click-id :id}
+ {:dashboard_id ~'dashboard-id
+ :card_id ~'card-id-nested
+ ;; this is how click actions on a non-table card work (ex: a chart)
+ :visualization_settings {:click_behavior {:targetId ~'card-id-nested-query
+ :linkType :question
+ :type :link}}}]
DashboardCardSeries [~'_ {:dashboardcard_id ~'dashcard-id
:card_id ~'card-id
:position 0}]
- DashboardCardSeries [~'_ {:dashboardcard_id ~'dashcard-id
+ DashboardCardSeries [~'_ {:dashboardcard_id ~'dashcard-top-level-click-id
:card_id ~'card-id-nested
:position 1}]
DashboardCard [{~'dashcard-with-click-actions :id}
- {:dashboard_id ~'dashboard-id
- :card_id ~'card-id-root}]
- DashboardCardSeries [~'_ {:dashboardcard_id ~'dashcard-with-click-actions
- :card_id ~'card-id-root
- :position 2}]]
- ~@body))
+ {:dashboard_id ~'dashboard-id
+ :card_id ~'card-id-root
+ :visualization_settings (-> (mb.viz/visualization-settings)
+ (mb.viz/with-entity-click-action
+ ~'numeric-field-id
+ ::mb.viz/card
+ ~'card-id
+ (mb.viz/fk-parameter-mapping
+ "Category"
+ ~'category-field-id
+ ~'numeric-field-id))
+ (mb.viz/with-entity-click-action
+ ~'name-field-id
+ ::mb.viz/dashboard
+ ~'root-dashboard-id)
+ (mb.viz/with-click-action
+ (mb.viz/column-name->column-ref "Price Known")
+ (mb.viz/url-click-action "/price-info"))
+ (mb.viz/with-click-action
+ (mb.viz/field-id->column-ref ~'latitude-field-id)
+ (mb.viz/crossfilter-click-action {}))
+ mb.viz/norm->db)}]
+ DashboardCardSeries [~'_ {:dashboardcard_id ~'dashcard-with-click-actions
+ :card_id ~'card-id-root
+ :position 2}]
+ DashboardCard [{~'dashcard-with-textbox-id :id}
+ {:dashboard_id ~'dashboard-id
+ :card_id nil
+ :visualization_settings {:virtual_card virtual-card
+ :text "Textbox Card"}}]
+ Card [{~'card-id-root-to-collection :id}
+ {:table_id ~'table-id
+ :name "Root card based on one in collection"
+ :dataset_query {:type :query
+ :database ~'db-id
+ :query {:source-table (str "card__" ~'card-id)}}}]
+ Card [{~'card-id-collection-to-root :id}
+ {:table_id ~'table-id
+ :name "Card in collection based on root one"
+ :collection_id ~'collection-id
+ :dataset_query {:type :query
+ :database ~'db-id
+ :query {:source-table (str "card__" ~'card-id-root)}}}]
+ Pulse [{~'pulse-id :id} {:name "Serialization Pulse"
+ :collection_id ~'collection-id}]
+ PulseCard [{~'pulsecard-root-id :id} {:pulse_id ~'pulse-id
+ :card_id ~'card-id-root}]
+ PulseCard [{~'pulsecard-collection-id :id} {:pulse_id ~'pulse-id
+ :card_id ~'card-id}
+ :query {:source-table (str "card__" ~'card-id-root)}]
+ Card [{~'card-id-template-tags :id}
+ {:query_type :native
+ :name "My Native Card With Template Tags"
+ :collection_id ~'collection-id
+ :dataset_query
+ {:type :native
+ :database ~'db-id
+ :native {:query "SELECT * FROM venues WHERE {{category-id}}"
+ :template-tags
+ {"category-id" {:id "751880ce-ad1a-11eb-8529-0242ac130003"
+ :name "category-id"
+ :display-name "Category ID"
+ :type "dimension"
+ :dimension [:field ~'category-field-id nil]
+ :widget-type "id"
+ :required true
+ :default 40}}}}}]
+ Card [{~'card-id-filter-agg :id}
+ {:table_id ~'table-id
+ :name "Card With Filter After Aggregation"
+ :collection_id ~'collection-id
+ :dataset_query {:type :query
+ :database ~'db-id
+ :query {:source-query {:source-table
+ ~'table-id
+ :aggregation
+ [[:aggregation-options
+ [:count]
+ {:name "num_per_type"}]]
+ :breakout
+ [[:field ~'category-field-id nil]]}
+ :filter [:>
+ [:field-literal "num_per_type" :type/Integer]
+ 4]}}}]
+ Card [{~'card-id-temporal-unit :id}
+ {:table_id ~'table-id
+ :name "Card With Temporal Unit in Field Clause"
+ :collection_id ~'collection-id
+ :dataset_query {:type :query
+ :database ~'db-id
+ :query {:source-query {:source-table
+ ~'table-id-checkins
+ :aggregation
+ [[:count]]
+ :breakout
+ [[:field ~'last-login-field-id {:source-field
+ ~'user-id-field-id
+ :temporal-unit
+ :month}]]}}}}]
+ NativeQuerySnippet [{~'snippet-id :id}
+ {:content "price > 2"
+ :description "Predicate on venues table for price > 2"
+ :name "Pricey Venues"}]
+ Collection [{~'snippet-collection-id :id}
+ {:name "Snippet Collection"
+ :namespace "snippets"}]
+ Collection [{~'snippet-nested-collection-id :id}
+ {:name "Nested Snippet Collection"
+ :location (format "/%d/" ~'snippet-collection-id)
+ :namespace "snippets"}]
+ NativeQuerySnippet [{~'nested-snippet-id :id}
+ {:content "name LIKE 'A%'"
+ :description "Predicate on venues table for name starting with A"
+ :name "A Venues"
+ :collection_id ~'snippet-nested-collection-id}]
+ Card [{~'card-id-with-native-snippet :id}
+ {:query_type :native
+ :name "Card with Native Query Snippet"
+ :collection_id ~'collection-id
+ :dataset_query
+ {:type :native
+ :database ~'db-id
+ :native {:query (str "SELECT * FROM venues WHERE {{snippet: Pricey Venues}}"
+ " AND {{snippet: A Venues}}")
+ :template-tags {"snippet: Pricey Venues"
+ {:id "d34baf40-b35a-11eb-8529-0242ac130003"
+ :name "Snippet: Pricey Venues"
+ :display-name "Snippet: Pricey Venues"
+ :type "snippet"
+ :snippet-name "Pricey Venues"
+ :snippet-id ~'snippet-id}
+ "snippet: A Venues"
+ {:id "c0775274-b45a-11eb-8529-0242ac130003"
+ :name "Snippet: A Venues"
+ :display-name "Snippet: A Venues"
+ :type "snippet"
+ :snippet-name "A Venues"
+ :snippet-id ~'nested-snippet-id}}}}}]
+ Card [{~'card-join-card-id :id}
+ {:table_id ~'table-id-checkins
+ :name "Card Joining to Another Card"
+ :collection_id ~'collection-id
+ :dataset_query {:type :query
+ :database ~'db-id
+ :query {:source-table ~'table-id-checkins
+ :joins [{:source-table (str "card__" ~'card-id-root)
+ :alias "v"
+ :fields "all"
+ :condition [:=
+ [:field
+ ~'checkins->venues-field-id
+ nil]
+ [:field
+ ~'venues-pk-field-id
+ {:join-alias "v"}]]}]}}}]]
+ (qp.store/with-store ~@body)))
;; Don't memoize as IDs change in each `with-world` context
(alter-var-root #'names/path->context (fn [_] #'names/path->context*))
diff --git a/enterprise/backend/test/metabase_enterprise/serialization/upsert_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/upsert_test.clj
index e528f4ba3c7ec4eafe16de48a6501eb16e94b6fb..39de5aea32e0cf9e31c31cfa6f7f3c531d8ec617 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/upsert_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/upsert_test.clj
@@ -2,7 +2,8 @@
(:require [clojure.data :as diff]
[clojure.test :refer :all]
[metabase-enterprise.serialization.upsert :as upsert]
- [metabase.models :refer [Card Collection Dashboard Database Field Metric Pulse Segment Table User]]
+ [metabase.models :refer [Card Collection Dashboard DashboardCard Database Field Metric NativeQuerySnippet
+ Pulse Segment Table User]]
[metabase.test :as mt]
[metabase.util :as u]
[toucan.db :as db]))
@@ -68,23 +69,49 @@
(testing "Card 2"
(is (same? (Card id2) e2))))))
+(defn- dummy-entity [dummy-dashboard model entity instance-num]
+ (cond
+ (= (type DashboardCard) (type model))
+ ;; hack to make sure that :visualization_settings are slightly different between the two dummy instances
+ ;; this is necessary because DashboardCards have that as part of their identity-condition
+ (assoc entity :dashboard_id (u/the-id dummy-dashboard)
+ :visualization_settings (if (= 1 instance-num) {:column_settings {}}
+ {:click_behavior {}}))
+
+ :else
+ entity))
+
(deftest identical-test
(letfn [(test-select-identical [model]
(testing (name model)
- (let [[e1 e2] (if (contains? (set (#'upsert/identity-condition model)) :name)
+ (let [id-cond (#'upsert/identity-condition model)
+ [e1 e2] (if (contains? (set id-cond) :name)
[{:name "a"} {:name "b"}]
[{} {}])]
- (mt/with-temp* [model [_ e1] ; create an additional entity so we're sure whe get the right one
- model [{id :id} e2]]
+ (mt/with-temp* [Dashboard [dashboard {:name "Dummy Dashboard"}]
+ ;; create an additional entity so we're sure whe get the right one
+ model [_ (dummy-entity dashboard model e1 1)]
+ model [{id :id} (dummy-entity dashboard model e2 2)]]
(let [e (model id)]
- (is (= (#'upsert/select-identical model e) e)))))))]
+ ;; make sure that all columns in identity-condition actually exist in the model
+ (is (= (set id-cond) (-> e
+ (select-keys id-cond)
+ keys
+ set)))
+ (is (= (#'upsert/select-identical model (cond-> e
+ ;; engine is a keyword but has to be a string for
+ ;; HoneySQL to not interpret it as a col name
+ (= (class e) (class Database)) (update :engine name)))
+ e)))))))]
(doseq [model [Collection
Card
Table
Field
Metric
+ NativeQuerySnippet
Segment
Dashboard
+ DashboardCard
Database
Pulse
User]]
diff --git a/frontend/src/metabase-lib/lib/Dimension.js b/frontend/src/metabase-lib/lib/Dimension.js
index 3b656dfc22a913b8d1f0f35acdca7804180fd022..00dad5621b939abbbb30520647cb02f5703bb695 100644
--- a/frontend/src/metabase-lib/lib/Dimension.js
+++ b/frontend/src/metabase-lib/lib/Dimension.js
@@ -780,6 +780,11 @@ export class FieldDimension extends Dimension {
dimension = dimension.withJoinAlias(joinAlias);
}
+ const baseType = this.getOption("base-type");
+ if (baseType) {
+ dimension = dimension.withOption("base-type", baseType);
+ }
+
return dimension;
}
diff --git a/frontend/src/metabase/admin/settings/components/SettingsBatchForm.jsx b/frontend/src/metabase/admin/settings/components/SettingsBatchForm.jsx
index 06e8823e0cc6488097bea1a52cb15c1111059471..d32b991502a39b292608ddd8ad0cc53ca1c0a7c0 100644
--- a/frontend/src/metabase/admin/settings/components/SettingsBatchForm.jsx
+++ b/frontend/src/metabase/admin/settings/components/SettingsBatchForm.jsx
@@ -12,8 +12,6 @@ import DisclosureTriangle from "metabase/components/DisclosureTriangle";
import MetabaseUtils from "metabase/lib/utils";
import SettingsSetting from "./SettingsSetting";
-import { updateSettings } from "../settings";
-
const VALIDATIONS = {
email: {
validate: value => MetabaseUtils.validEmail(value),
@@ -33,7 +31,10 @@ const SAVE_SETTINGS_BUTTONS_STATES = {
@connect(
null,
- { updateSettings },
+ (dispatch, { updateSettings }) => ({
+ updateSettings:
+ updateSettings || (settings => dispatch(updateSettings(settings))),
+ }),
null,
{ withRef: true }, // HACK: needed so consuming components can call methods on the component :-/
)
diff --git a/frontend/src/metabase/auth/components/LdapAndEmailForm.jsx b/frontend/src/metabase/auth/components/LdapAndEmailForm.jsx
index 383056d00abe8489eb9ea2bd6347c2da4d87cc0e..d9eba604a8f06aca87a14fd5a9002903c2eb96ee 100644
--- a/frontend/src/metabase/auth/components/LdapAndEmailForm.jsx
+++ b/frontend/src/metabase/auth/components/LdapAndEmailForm.jsx
@@ -25,6 +25,7 @@ export default class LdapAndEmailForm extends Component {
render() {
const ldapEnabled = Settings.ldapEnabled();
+ const rememberMeDisabled = Settings.get("session-cookies");
return (
<Form onSubmit={this.onSubmit}>
{({ values, Form, FormField, FormSubmit, FormMessage }) => (
@@ -51,6 +52,7 @@ export default class LdapAndEmailForm extends Component {
type="checkbox"
title={t`Remember me`}
initial={true}
+ hidden={rememberMeDisabled}
horizontal
/>
<FormMessage />
diff --git a/frontend/src/metabase/components/TokenField.jsx b/frontend/src/metabase/components/TokenField.jsx
index f321ea4b104470fbac5c467f0f2ca31c762508c4..8748cc7fac61a270b137637d87902f7c065d7a1b 100644
--- a/frontend/src/metabase/components/TokenField.jsx
+++ b/frontend/src/metabase/components/TokenField.jsx
@@ -174,7 +174,7 @@ export default class TokenField extends Component {
}
_updateFilteredValues = (props: Props) => {
- let { options, value, removeSelected, filterOption } = props;
+ let { options = [], value, removeSelected, filterOption } = props;
let { searchValue, selectedOptionValue } = this.state;
const selectedValues = new Set(value.map(v => JSON.stringify(v)));
diff --git a/frontend/src/metabase/meta/Parameter.js b/frontend/src/metabase/meta/Parameter.js
index 348c55e8f2d0206c86ae1f7dd122d3f8618d5875..4a1619e7dae573e3ddd9a821422396679f0e842a 100644
--- a/frontend/src/metabase/meta/Parameter.js
+++ b/frontend/src/metabase/meta/Parameter.js
@@ -325,28 +325,37 @@ function tagFilterForParameter(parameter: Parameter): TemplateTagFilter {
// NOTE: this should mirror `template-tag-parameters` in src/metabase/api/embed.clj
export function getTemplateTagParameters(tags: TemplateTag[]): Parameter[] {
+ function getTemplateTagType(tag) {
+ const { type } = tag;
+ if (type === "date") {
+ return "date/single";
+ } else if (areFieldFilterOperatorsEnabled() && type === "string") {
+ return "string/=";
+ } else if (areFieldFilterOperatorsEnabled() && type === "number") {
+ return "number/=";
+ } else {
+ return "category";
+ }
+ }
+
return tags
.filter(
tag =>
tag.type != null && (tag["widget-type"] || tag.type !== "dimension"),
)
- .map(tag => ({
- id: tag.id,
- type:
- tag["widget-type"] ||
- (tag.type === "date"
- ? "date/single"
- : areFieldFilterOperatorsEnabled()
- ? "string/="
- : "category"),
- target:
- tag.type === "dimension"
- ? ["dimension", ["template-tag", tag.name]]
- : ["variable", ["template-tag", tag.name]],
- name: tag["display-name"],
- slug: tag.name,
- default: tag.default,
- }));
+ .map(tag => {
+ return {
+ id: tag.id,
+ type: tag["widget-type"] || getTemplateTagType(tag),
+ target:
+ tag.type === "dimension"
+ ? ["dimension", ["template-tag", tag.name]]
+ : ["variable", ["template-tag", tag.name]],
+ name: tag["display-name"],
+ slug: tag.name,
+ default: tag.default,
+ };
+ });
}
export const getParametersBySlug = (
diff --git a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.jsx b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.jsx
index fa826ceeaa716f6672cda96bcb56823322ddf717..40f691148e1dc53e1fd34bcfa8483af39266fa73 100644
--- a/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.jsx
+++ b/frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.jsx
@@ -2,6 +2,8 @@ import React, { Component } from "react";
import PropTypes from "prop-types";
import cx from "classnames";
import { t } from "ttag";
+import _ from "underscore";
+
import ExpressionEditorTextfield from "./ExpressionEditorTextfield";
import { isExpression } from "metabase/lib/expressions";
import MetabaseSettings from "metabase/lib/settings";
@@ -16,7 +18,7 @@ export default class ExpressionWidget extends Component {
query: PropTypes.object.isRequired,
onChangeExpression: PropTypes.func.isRequired,
onRemoveExpression: PropTypes.func,
- onClose: PropTypes.func.isRequired,
+ onClose: PropTypes.func,
};
static defaultProps = {
@@ -29,10 +31,12 @@ export default class ExpressionWidget extends Component {
}
UNSAFE_componentWillReceiveProps(newProps) {
- this.setState({
- name: newProps.name,
- expression: newProps.expression,
- });
+ if (!this.state || !_.isEqual(this.props.expression, newProps.expression)) {
+ this.setState({
+ name: newProps.name,
+ expression: newProps.expression,
+ });
+ }
}
isValid() {
diff --git a/frontend/src/metabase/query_builder/selectors.js b/frontend/src/metabase/query_builder/selectors.js
index 3dc0af025354890df6c082c90100c994e706bd8c..10703acbb476c977a4d91fdfb835b393039f8041 100644
--- a/frontend/src/metabase/query_builder/selectors.js
+++ b/frontend/src/metabase/query_builder/selectors.js
@@ -14,6 +14,7 @@ import {
} from "metabase/visualizations";
import { getComputedSettingsForSeries } from "metabase/visualizations/lib/settings/visualization";
import { getParametersWithExtras } from "metabase/meta/Card";
+import { normalizeParameterValue } from "metabase/meta/Parameter";
import Utils from "metabase/lib/utils";
@@ -144,7 +145,14 @@ const getLastRunParameterValues = createSelector(
const getNextRunParameterValues = createSelector(
[getParameters],
parameters =>
- parameters.map(parameter => parameter.value).filter(p => p !== undefined),
+ parameters
+ .map(parameter =>
+ // parameters are "normalized" immediately before a query run, so in order
+ // to compare current parameters to previously-used parameters we need
+ // to run parameters through this normalization function
+ normalizeParameterValue(parameter.type, parameter.value),
+ )
+ .filter(p => p !== undefined),
);
// Certain differences in a query should be ignored. `normalizeQuery`
diff --git a/frontend/src/metabase/visualizations/components/ChartSettings.jsx b/frontend/src/metabase/visualizations/components/ChartSettings.jsx
index 0e32b70f9b2386e674fbffc4830f591ba004f18d..bf764f5b24271584bcd45c9ec1b27e479401dbbb 100644
--- a/frontend/src/metabase/visualizations/components/ChartSettings.jsx
+++ b/frontend/src/metabase/visualizations/components/ChartSettings.jsx
@@ -285,7 +285,10 @@ class ChartSettings extends Component {
</div>
) : (
<div className="Grid flex-full">
- <div className="Grid-cell Cell--1of3 scroll-y scroll-show border-right py4">
+ <div
+ className="Grid-cell Cell--1of3 scroll-y scroll-show border-right py4"
+ data-testid={"chartsettings-sidebar"}
+ >
{widgetList}
</div>
<div className="Grid-cell flex flex-column pt2">
diff --git a/frontend/src/metabase/visualizations/components/ChartTooltip.jsx b/frontend/src/metabase/visualizations/components/ChartTooltip.jsx
index bad6446199380281b92049d9f1fd6c04b57f6831..19fed2b924e2f7832c51cbc1ced32df68b42853e 100644
--- a/frontend/src/metabase/visualizations/components/ChartTooltip.jsx
+++ b/frontend/src/metabase/visualizations/components/ChartTooltip.jsx
@@ -22,7 +22,7 @@ export default class ChartTooltip extends Component {
// Array of key, value, col: { data: [{ key, value, col }], element, event }
return hovered.data.map(d => ({
...d,
- key: d.key || getFriendlyName(d.col),
+ key: d.key || (d.col && getFriendlyName(d.col)),
}));
} else if (hovered.value !== undefined || hovered.dimensions) {
// ClickObject: { value, column, dimensions: [{ value, column }], element, event }
diff --git a/frontend/src/metabase/visualizations/lib/renderer_utils.js b/frontend/src/metabase/visualizations/lib/renderer_utils.js
index 43acb004e8b0e3afc382f761bef4a69324b5efbc..766e8dcadfbeced19d40ab350878fa2d8e6471af 100644
--- a/frontend/src/metabase/visualizations/lib/renderer_utils.js
+++ b/frontend/src/metabase/visualizations/lib/renderer_utils.js
@@ -153,10 +153,12 @@ export function getDatas({ settings, series }, warn) {
if (isNotOrdinal) {
rows = data.rows.filter(([x]) => x !== null);
} else if (parseOptions.isNumeric) {
- rows = data.rows.map(([x, ...rest]) => [
- replaceNullValuesForOrdinal(x),
- ...rest,
- ]);
+ rows = data.rows.map(row => {
+ const [x, ...rest] = row;
+ const newRow = [replaceNullValuesForOrdinal(x), ...rest];
+ newRow._origin = row._origin;
+ return newRow;
+ });
}
if (rows.length < data.rows.length) {
diff --git a/frontend/test/__support__/e2e/cypress.js b/frontend/test/__support__/e2e/cypress.js
index fe927ab00b8211d2d3b2f332e88182cf3815237a..de09d8b248d3eeb07024ba3f02db6206e2c5eee6 100644
--- a/frontend/test/__support__/e2e/cypress.js
+++ b/frontend/test/__support__/e2e/cypress.js
@@ -102,28 +102,6 @@ export function typeAndBlurUsingLabel(label, value) {
.blur();
}
-// Unfortunately, cypress `.type()` is currently broken and requires an ugly "hack"
-// it is documented here: https://github.com/cypress-io/cypress/issues/5480
-// `_typeUsingGet()` and `_typeUsingPlacehodler()` are temporary solution
-// please refrain from using them, unless absolutely neccessary!
-export function _typeUsingGet(selector, value, delay = 100) {
- cy.get(selector)
- .click()
- .type(value, { delay })
- .clear()
- .click()
- .type(value, { delay });
-}
-
-export function _typeUsingPlaceholder(selector, value, delay = 100) {
- cy.findByPlaceholderText(selector)
- .click()
- .type(value, { delay })
- .clear()
- .click()
- .type(value, { delay });
-}
-
Cypress.on("uncaught:exception", (err, runnable) => false);
export function withDatabase(databaseId, f) {
@@ -310,8 +288,8 @@ export function getIframeBody(selector = "iframe") {
.then(cy.wrap);
}
-export function mockSessionProperty(propertyOrObject, value) {
- cy.intercept("GET", "/api/session/properties", req => {
+function mockProperty(propertyOrObject, value, url) {
+ cy.intercept("GET", url, req => {
req.reply(res => {
if (typeof propertyOrObject === "object") {
Object.assign(res.body, propertyOrObject);
@@ -323,6 +301,14 @@ export function mockSessionProperty(propertyOrObject, value) {
});
}
+export function mockSessionProperty(propertyOrObject, value) {
+ mockProperty(propertyOrObject, value, "/api/session/properties");
+}
+
+export function mockCurrentUserProperty(propertyOrObject, value) {
+ mockProperty(propertyOrObject, value, "/api/user/current");
+}
+
export function showDashboardCardActions(index = 0) {
cy.get(".DashCard")
.eq(index)
diff --git a/frontend/test/metabase-db/mongo/line.cy.spec.js b/frontend/test/metabase-db/mongo/line.cy.spec.js
new file mode 100644
index 0000000000000000000000000000000000000000..97112667db55b888d5f3afbf843e453b38c4bcd5
--- /dev/null
+++ b/frontend/test/metabase-db/mongo/line.cy.spec.js
@@ -0,0 +1,45 @@
+import { restore, addMongoDatabase, popover } from "__support__/e2e/cypress";
+
+const MONGO_DB_NAME = "QA Mongo4";
+
+// Skipping the whole describe block because it contains only one skipped test so far!
+// We don't want to run the whole beforeEeach block in CI only to skip the test afterwards.
+// IMPORTANT: when #16170 gets fixed, unskip both describe block and the test itself!
+describe.skip("mongodb > visualization > line chart", () => {
+ before(() => {
+ restore();
+ cy.signInAsAdmin();
+ addMongoDatabase(MONGO_DB_NAME);
+ });
+
+ it.skip("should correctly replace only the missing values with zero (metabase#16170)", () => {
+ cy.visit("/question/new");
+ cy.findByText("Simple question").click();
+ cy.findByText(MONGO_DB_NAME).click();
+ cy.findByText("Orders").click();
+ cy.findAllByRole("button")
+ .contains("Summarize")
+ .click();
+ cy.findByTestId("sidebar-right")
+ .findByText("Created At")
+ .click();
+ assertOnTheYAxis();
+ cy.findAllByRole("button")
+ .contains("Settings")
+ .click();
+ cy.findByTestId("sidebar-left")
+ .findByText("Linear Interpolated")
+ .click();
+ popover()
+ .findByText("Zero")
+ .click();
+ assertOnTheYAxis();
+
+ function assertOnTheYAxis() {
+ cy.get(".y-axis-label").findByText("Count");
+ cy.get(".axis.y .tick")
+ .should("have.length.gt", 10)
+ .and("contain", "200");
+ }
+ });
+});
diff --git a/frontend/test/metabase-db/mongo/native.cy.spec.js b/frontend/test/metabase-db/mongo/native.cy.spec.js
index bb7a8f76dcf6dd427a45e95eac5f7658faa9b31f..a9cc7f081d2c9611953899fcad3714e700317df0 100644
--- a/frontend/test/metabase-db/mongo/native.cy.spec.js
+++ b/frontend/test/metabase-db/mongo/native.cy.spec.js
@@ -2,7 +2,10 @@ import { restore, addMongoDatabase, modal } from "__support__/e2e/cypress";
const MONGO_DB_NAME = "QA Mongo4";
-describe("mongodb > native query", () => {
+// Skipping the whole describe block because it contains only one skipped test so far!
+// We don't want to run the whole beforeEeach block in CI only to skip the test afterwards.
+// IMPORTANT: when #15946 gets fixed, unskip both describe block and the test itself!
+describe.skip("mongodb > native query", () => {
before(() => {
restore();
cy.signInAsAdmin();
diff --git a/frontend/test/metabase-db/postgres/native.cy.spec.js b/frontend/test/metabase-db/postgres/native.cy.spec.js
index ad6ff5dadf143144be8f2ddbb40cba9215e3eb62..4e2dd415ce6400656af00dce9ed8c1171c3b16dc 100644
--- a/frontend/test/metabase-db/postgres/native.cy.spec.js
+++ b/frontend/test/metabase-db/postgres/native.cy.spec.js
@@ -2,7 +2,10 @@ import { restore, addPostgresDatabase, modal } from "__support__/e2e/cypress";
const PG_DB_NAME = "QA Postgres12";
-describe("postgres > question > native", () => {
+// Skipping the whole describe block because it contains only one skipped test so far!
+// We don't want to run the whole beforeEeach block in CI only to skip the test afterwards.
+// IMPORTANT: when #14957 gets fixed, unskip both describe block and the test itself!
+describe.skip("postgres > question > native", () => {
beforeEach(() => {
restore();
cy.signInAsAdmin();
diff --git a/frontend/test/metabase-lib/lib/Dimension.unit.spec.js b/frontend/test/metabase-lib/lib/Dimension.unit.spec.js
index 5fb62f8b3b04d60b9528ac4f45e09265909f0412..5da228661c671bc20b662697946c13f2b9e513d3 100644
--- a/frontend/test/metabase-lib/lib/Dimension.unit.spec.js
+++ b/frontend/test/metabase-lib/lib/Dimension.unit.spec.js
@@ -266,7 +266,7 @@ describe("Dimension", () => {
// TODO -- there are some tests against fields that can be binned above -- we should merge them in with these ones
describe("Numeric Field that can be binned", () => {
- const mbql = ["field", ORDERS.TOTAL.id, null];
+ const mbql = ["field", ORDERS.TOTAL.id, { "base-type": "type/Float" }];
const dimension = Dimension.parseMBQL(mbql, metadata);
describe("INSTANCE METHODS", () => {
@@ -284,6 +284,7 @@ describe("Dimension", () => {
"field",
ORDERS.TOTAL.id,
{
+ "base-type": "type/Float",
binning: {
strategy: "default",
},
diff --git a/frontend/test/metabase/meta/Parameter.unit.spec.js b/frontend/test/metabase/meta/Parameter.unit.spec.js
index c84f64d6762368246347a25685c4345f9894a284..57a4307305524aa479ff14ca8bb6f8d96ecde162 100644
--- a/frontend/test/metabase/meta/Parameter.unit.spec.js
+++ b/frontend/test/metabase/meta/Parameter.unit.spec.js
@@ -1,3 +1,4 @@
+import MetabaseSettings from "metabase/lib/settings";
import {
dateParameterValueToMBQL,
stringParameterValueToMBQL,
@@ -6,9 +7,24 @@ import {
getParametersBySlug,
normalizeParameterValue,
deriveFieldOperatorFromParameter,
+ getTemplateTagParameters,
} from "metabase/meta/Parameter";
+MetabaseSettings.get = jest.fn();
+
+function mockFieldFilterOperatorsFlag(value) {
+ MetabaseSettings.get.mockImplementation(flag => {
+ if (flag === "field-filter-operators-enabled?") {
+ return value;
+ }
+ });
+}
+
describe("metabase/meta/Parameter", () => {
+ beforeEach(() => {
+ MetabaseSettings.get.mockReturnValue(false);
+ });
+
describe("dateParameterValueToMBQL", () => {
it("should parse past30days", () => {
expect(dateParameterValueToMBQL("past30days", null)).toEqual([
@@ -276,4 +292,158 @@ describe("metabase/meta/Parameter", () => {
});
});
});
+
+ describe("getTemplateTagParameters", () => {
+ let tags;
+ beforeEach(() => {
+ tags = [
+ {
+ "widget-type": "foo",
+ type: "string",
+ id: 1,
+ name: "a",
+ "display-name": "A",
+ default: "abc",
+ },
+ {
+ type: "string",
+ id: 2,
+ name: "b",
+ "display-name": "B",
+ },
+ {
+ type: "number",
+ id: 3,
+ name: "c",
+ "display-name": "C",
+ },
+ {
+ type: "date",
+ id: 4,
+ name: "d",
+ "display-name": "D",
+ },
+ {
+ "widget-type": "foo",
+ type: "dimension",
+ id: 5,
+ name: "e",
+ "display-name": "E",
+ },
+ {
+ type: null,
+ id: 6,
+ },
+ {
+ type: "dimension",
+ id: 7,
+ name: "f",
+ "display-name": "F",
+ },
+ ];
+ });
+
+ describe("field filter operators enabled", () => {
+ beforeEach(() => {
+ mockFieldFilterOperatorsFlag(true);
+ });
+
+ it("should convert tags into tag parameters with field filter operator types", () => {
+ const parametersWithFieldFilterOperatorTypes = [
+ {
+ default: "abc",
+ id: 1,
+ name: "A",
+ slug: "a",
+ target: ["variable", ["template-tag", "a"]],
+ type: "foo",
+ },
+ {
+ default: undefined,
+ id: 2,
+ name: "B",
+ slug: "b",
+ target: ["variable", ["template-tag", "b"]],
+ type: "string/=",
+ },
+ {
+ default: undefined,
+ id: 3,
+ name: "C",
+ slug: "c",
+ target: ["variable", ["template-tag", "c"]],
+ type: "number/=",
+ },
+ {
+ default: undefined,
+ id: 4,
+ name: "D",
+ slug: "d",
+ target: ["variable", ["template-tag", "d"]],
+ type: "date/single",
+ },
+ {
+ default: undefined,
+ id: 5,
+ name: "E",
+ slug: "e",
+ target: ["dimension", ["template-tag", "e"]],
+ type: "foo",
+ },
+ ];
+
+ expect(getTemplateTagParameters(tags)).toEqual(
+ parametersWithFieldFilterOperatorTypes,
+ );
+ });
+ });
+
+ describe("field filter operators disabled", () => {
+ it("should convert tags into tag parameters", () => {
+ const parameters = [
+ {
+ default: "abc",
+ id: 1,
+ name: "A",
+ slug: "a",
+ target: ["variable", ["template-tag", "a"]],
+ type: "foo",
+ },
+ {
+ default: undefined,
+ id: 2,
+ name: "B",
+ slug: "b",
+ target: ["variable", ["template-tag", "b"]],
+ type: "category",
+ },
+ {
+ default: undefined,
+ id: 3,
+ name: "C",
+ slug: "c",
+ target: ["variable", ["template-tag", "c"]],
+ type: "category",
+ },
+ {
+ default: undefined,
+ id: 4,
+ name: "D",
+ slug: "d",
+ target: ["variable", ["template-tag", "d"]],
+ type: "date/single",
+ },
+ {
+ default: undefined,
+ id: 5,
+ name: "E",
+ slug: "e",
+ target: ["dimension", ["template-tag", "e"]],
+ type: "foo",
+ },
+ ];
+ expect(getTemplateTagParameters(tags)).toEqual(parameters);
+ });
+ });
+ });
});
diff --git a/frontend/test/metabase/scenarios/admin/settings/settings.cy.spec.js b/frontend/test/metabase/scenarios/admin/settings/settings.cy.spec.js
index 696ca13e31c9e60a5bf9d24c4b5807550a5bcb11..85d321f3afd64b0158cc1a718f97486c9e6d9329 100644
--- a/frontend/test/metabase/scenarios/admin/settings/settings.cy.spec.js
+++ b/frontend/test/metabase/scenarios/admin/settings/settings.cy.spec.js
@@ -343,10 +343,16 @@ describe("scenarios > admin > settings", () => {
.type("localhost")
.blur();
cy.findByPlaceholderText("587")
- .type("1234")
+ .type("25")
+ .blur();
+ cy.findByPlaceholderText("youlooknicetoday")
+ .type("admin")
+ .blur();
+ cy.findByPlaceholderText("Shhh...")
+ .type("admin")
.blur();
cy.findByPlaceholderText("metabase@yourcompany.com")
- .type("admin@metabase.com")
+ .type("mailer@metabase.test")
.blur();
cy.findByText("Save changes").click();
diff --git a/frontend/test/metabase/scenarios/admin/settings/sso.cy.spec.js b/frontend/test/metabase/scenarios/admin/settings/sso.cy.spec.js
new file mode 100644
index 0000000000000000000000000000000000000000..68a64aba68aab6257ba1c29a2fc07bc32383c09d
--- /dev/null
+++ b/frontend/test/metabase/scenarios/admin/settings/sso.cy.spec.js
@@ -0,0 +1,73 @@
+import { restore } from "__support__/e2e/cypress";
+
+describe("scenarios > admin > settings > SSO", () => {
+ beforeEach(() => {
+ restore();
+ cy.signInAsAdmin();
+ });
+
+ it.skip("Google sign-in client ID should save on subsequent tries (metabase#15974)", () => {
+ cy.visit("/admin/settings/authentication/google");
+ cy.findByPlaceholderText("Your Google client ID").type("123");
+ saveSettings();
+
+ cy.findByDisplayValue("123").type("456");
+ saveSettings();
+ });
+
+ describe("LDAP", () => {
+ /**
+ * IMPORTANT:
+ * These repros currently rely on the broken behavior. It is possible to save settings without authentication turned on.
+ * See: https://github.com/metabase/metabase/issues/16225
+ * It is possible related repros will have to be rewritten/adjusted once #16255 gets fixed!
+ */
+
+ beforeEach(() => {
+ cy.visit("/admin/settings/authentication/ldap");
+ });
+
+ // Space after "123 " is crucial for #13313
+ const INVALID_PORTS = ["21.3", "asd", "123 "];
+
+ INVALID_PORTS.forEach(port => {
+ it("shouldn't be possible to save non-numeric port (#13313)", () => {
+ // The real endpoint is `/api/ldap/settings` but I had to use this ambiguous one for this repro because of #16173
+ // Although the endpoint was fixed, we want to always be able to test these issues separately and independently of each other.
+ cy.intercept("PUT", "/api/**").as("update");
+
+ cy.findByPlaceholderText("ldap.yourdomain.org").type("foobar");
+ cy.findByPlaceholderText("389").type(port);
+ cy.button("Save changes").click();
+ cy.wait("@update").then(interception => {
+ expect(interception.response.statusCode).to.eq(500);
+ expect(interception.response.body.cause).to.include(port);
+ });
+ });
+ });
+
+ it("should use the correct endpoint (metabase#16173)", () => {
+ cy.intercept("PUT", "/api/ldap/settings").as("ldapUpdate");
+ cy.findByPlaceholderText("ldap.yourdomain.org").type("foobar");
+ cy.findByPlaceholderText("389").type("888");
+ cy.findByText(/Username or DN/i)
+ .closest("li")
+ .find("input")
+ .type("John");
+ cy.findByText("The password to bind with for the lookup user.")
+ .closest("li")
+ .find("input")
+ .type("Smith");
+ cy.button("Save changes").click();
+ cy.wait("@ldapUpdate").then(interception => {
+ expect(interception.response.statusCode).to.eq(200);
+ });
+ cy.button("Changes saved!");
+ });
+ });
+});
+
+function saveSettings() {
+ cy.button("Save Changes").click();
+ cy.findByText("Saved");
+}
diff --git a/frontend/test/metabase/scenarios/dashboard/dashboard.cy.spec.js b/frontend/test/metabase/scenarios/dashboard/dashboard.cy.spec.js
index df59b59992fbf847e17a4e4e8ec9736a6433028c..f9e37329d5e0968a1fc95c894bb1200f6c88bdd6 100644
--- a/frontend/test/metabase/scenarios/dashboard/dashboard.cy.spec.js
+++ b/frontend/test/metabase/scenarios/dashboard/dashboard.cy.spec.js
@@ -434,7 +434,7 @@ describe("scenarios > dashboard", () => {
expectedRouteCalls({ route_alias: "fetchFieldValues", calls: 0 });
});
- it.skip("should cache filter results after the first DB call (metabase#13832)", () => {
+ it.skip("filter dropdown should not send request for values every time the widget is opened (metabase#16103)", () => {
// In this test we're using already present dashboard ("Orders in a dashboard")
const FILTER_ID = "d7988e02";
diff --git a/frontend/test/metabase/scenarios/dashboard/visualizaiton-options.cy.spec.js b/frontend/test/metabase/scenarios/dashboard/visualizaiton-options.cy.spec.js
new file mode 100644
index 0000000000000000000000000000000000000000..52878650b522b2b0dc94937f0d8fa8f3a4ccc19e
--- /dev/null
+++ b/frontend/test/metabase/scenarios/dashboard/visualizaiton-options.cy.spec.js
@@ -0,0 +1,38 @@
+import { restore } from "__support__/e2e/cypress";
+
+describe("scenarios > dashboard > visualization options", () => {
+ beforeEach(() => {
+ restore();
+ cy.signInAsAdmin();
+ });
+
+ it.skip("column reordering should work (metabase#16229)", () => {
+ cy.visit("/dashboard/1");
+ cy.icon("pencil").click();
+ cy.get(".Card").realHover();
+ cy.icon("palette").click();
+ cy.findByTestId("chartsettings-sidebar").within(() => {
+ cy.findByText("ID")
+ .closest(".cursor-grab")
+ .trigger("mousedown", 0, 0, { force: true })
+ .trigger("mousemove", 5, 5, { force: true })
+ .trigger("mousemove", 0, 100, { force: true })
+ .trigger("mouseup", 0, 100, { force: true });
+
+ /**
+ * When this issue gets fixed, it should be safe to uncomment the following assertion.
+ * It currently doesn't work in UI at all, but Cypress somehow manages to move the "ID" column.
+ * However, it leaves an empty column in its place (thus, making it impossible to use this assertion).
+ */
+ // cy.get(".cursor-grab")
+ // .as("sidebarColumns") // Out of all the columns in the sidebar...
+ // .first() // ...pick the fist one and make sure it's not "ID" anymore
+ // .should("contain", "User ID");
+ });
+
+ // The table preview should get updated immediately, reflecting the changes in columns ordering.
+ cy.get(".Modal .cellData")
+ .first()
+ .contains("User ID");
+ });
+});
diff --git a/frontend/test/metabase/scenarios/native/native.cy.spec.js b/frontend/test/metabase/scenarios/native/native.cy.spec.js
index adc8a3106284a7ddc5576ffb6af64b90e77bd208..7ac205e106025d8513083c5358319fce7e9db22d 100644
--- a/frontend/test/metabase/scenarios/native/native.cy.spec.js
+++ b/frontend/test/metabase/scenarios/native/native.cy.spec.js
@@ -667,7 +667,7 @@ describe("scenarios > question > native", () => {
["off", "on"].forEach(testCase => {
const isFeatureFlagTurnedOn = testCase === "off" ? false : true;
- describe.skip("Feature flag causes problems with Text and Number filters in Native query (metabase#15981)", () => {
+ describe("Feature flag causes problems with Text and Number filters in Native query (metabase#15981)", () => {
beforeEach(() => {
mockSessionProperty(
"field-filter-operators-enabled?",
@@ -694,7 +694,7 @@ describe("scenarios > question > native", () => {
cy.findByText("Rustic Paper Wallet");
cy.icon("contract").click();
cy.findByText("Showing 51 rows");
- cy.get(".RunButton").should("not.exist");
+ cy.icon("play").should("not.exist");
});
it(`number filter should work with the feature flag turned ${testCase}`, () => {
diff --git a/frontend/test/metabase/scenarios/onboarding/auth/sso.cy.spec.js b/frontend/test/metabase/scenarios/onboarding/auth/sso.cy.spec.js
index 001ee92f7c7f9f3ae5948e31ba0cb44262f55ecd..bca23522152d8926696054a53d4d2c93a8dfa33e 100644
--- a/frontend/test/metabase/scenarios/onboarding/auth/sso.cy.spec.js
+++ b/frontend/test/metabase/scenarios/onboarding/auth/sso.cy.spec.js
@@ -1,4 +1,8 @@
-import { describeWithToken, restore } from "__support__/e2e/cypress";
+import {
+ describeWithToken,
+ restore,
+ mockCurrentUserProperty,
+} from "__support__/e2e/cypress";
describe("scenarios > auth > signin > SSO", () => {
beforeEach(() => {
@@ -10,27 +14,40 @@ describe("scenarios > auth > signin > SSO", () => {
});
});
+ ["ldap_auth", "google_auth"].forEach(auth => {
+ it.skip(`login history tab should be available with ${auth} enabled (metabase#15558)`, () => {
+ mockCurrentUserProperty(auth, true);
+ cy.visit("/user/edit_current");
+ cy.findByText("Login History");
+ });
+ });
+
describe("OSS", () => {
beforeEach(() => {
cy.signOut();
+ cy.visit("/");
});
it("should show SSO button", () => {
- cy.visit("/");
cy.findByText("Sign in with Google");
cy.findByText("Sign in with email");
});
it("should show login form when directed to sign in with email", () => {
- cy.visit("/");
cy.findByText("Sign in with email").click();
cy.findByLabelText("Email address");
cy.findByLabelText("Password");
- cy.findByText("Sign in")
- .closest("button")
- .should("be.disabled");
+ cy.button("Sign in").should("be.disabled");
cy.findByText("Sign in with Google").should("not.exist");
});
+
+ it.skip("should surface login errors with Google sign in enabled (metabase#16122)", () => {
+ cy.findByText("Sign in with email").click();
+ cy.findByLabelText("Email address").type("foo@bar.test");
+ cy.findByLabelText("Password").type("123");
+ cy.button("Sign in").click();
+ cy.contains("Password: did not match stored password");
+ });
});
describeWithToken("EE", () => {
diff --git a/frontend/test/metabase/scenarios/question/custom_column.cy.spec.js b/frontend/test/metabase/scenarios/question/custom_column.cy.spec.js
index 622a158605919bba7fc3c20560c12c1866f4b5bf..a7d6828a4410ead555933e85ffb8396bf7442623 100644
--- a/frontend/test/metabase/scenarios/question/custom_column.cy.spec.js
+++ b/frontend/test/metabase/scenarios/question/custom_column.cy.spec.js
@@ -1,8 +1,6 @@
import {
restore,
popover,
- _typeUsingGet,
- _typeUsingPlaceholder,
openOrdersTable,
openProductsTable,
openPeopleTable,
@@ -13,14 +11,6 @@ import { SAMPLE_DATASET } from "__support__/e2e/cypress_sample_dataset";
const { ORDERS, ORDERS_ID, PRODUCTS, PRODUCTS_ID } = SAMPLE_DATASET;
-const customFormulas = [
- {
- customFormula: "[Quantity] * 2",
- columnName: "Double Qt",
- },
- { customFormula: "[Quantity] * [Product.Price]", columnName: "Sum Total" },
-];
-
describe("scenarios > question > custom columns", () => {
beforeEach(() => {
restore();
@@ -28,16 +18,11 @@ describe("scenarios > question > custom columns", () => {
});
it("can create a custom column (metabase#13241)", () => {
- const columnName = "Simple Math";
openOrdersTable({ mode: "notebook" });
cy.icon("add_data").click();
- popover().within(() => {
- _typeUsingGet("[contenteditable='true']", "1 + 1", 400);
- _typeUsingPlaceholder("Something nice and descriptive", columnName);
-
- cy.findByText("Done").click();
- });
+ enterCustomColumnDetails({ formula: "1 + 1", name: "Math" });
+ cy.button("Done").click();
cy.server();
cy.route("POST", "/api/dataset").as("dataset");
@@ -45,27 +30,34 @@ describe("scenarios > question > custom columns", () => {
cy.button("Visualize").click();
cy.wait("@dataset");
cy.findByText("There was a problem with your question").should("not.exist");
- cy.get(".Visualization").contains(columnName);
+ cy.get(".Visualization").contains("Math");
});
it("can create a custom column with an existing column name", () => {
- customFormulas.forEach(({ customFormula, columnName }) => {
+ const customFormulas = [
+ {
+ formula: "[Quantity] * 2",
+ name: "Double Qt",
+ },
+ {
+ formula: "[Quantity] * [Product.Price]",
+ name: "Sum Total",
+ },
+ ];
+
+ customFormulas.forEach(({ formula, name }) => {
openOrdersTable({ mode: "notebook" });
cy.icon("add_data").click();
- popover().within(() => {
- _typeUsingGet("[contenteditable='true']", customFormula, 400);
- _typeUsingPlaceholder("Something nice and descriptive", columnName);
-
- cy.findByText("Done").click();
- });
+ enterCustomColumnDetails({ formula, name });
+ cy.button("Done").click();
cy.server();
cy.route("POST", "/api/dataset").as("dataset");
cy.button("Visualize").click();
cy.wait("@dataset");
- cy.get(".Visualization").contains(columnName);
+ cy.get(".Visualization").contains(name);
});
});
@@ -96,15 +88,12 @@ describe("scenarios > question > custom columns", () => {
// Add custom column based on previous aggregates
const columnName = "MegaTotal";
cy.findByText("Custom column").click();
- popover().within(() => {
- cy.get("[contenteditable='true']")
- .click()
- .type("[Sum of Subtotal] + [Sum of Total]");
- cy.findByPlaceholderText("Something nice and descriptive")
- .click()
- .type(columnName);
- cy.findByText("Done").click();
+
+ enterCustomColumnDetails({
+ formula: "[Sum of Subtotal] + [Sum of Total]",
+ name: columnName,
});
+ cy.button("Done").click();
cy.server();
cy.route("POST", "/api/dataset").as("dataset");
@@ -118,27 +107,19 @@ describe("scenarios > question > custom columns", () => {
});
it.skip("should allow 'zoom in' drill-through when grouped by custom column (metabase#13289)", () => {
- const columnName = "TestColumn";
openOrdersTable({ mode: "notebook" });
// Add custom column that will be used later in summarize (group by)
cy.findByText("Custom column").click();
- popover().within(() => {
- _typeUsingGet("[contenteditable='true']", "1 + 1", 400);
- _typeUsingPlaceholder("Something nice and descriptive", columnName);
-
- cy.findByText("Done").click();
- });
+ enterCustomColumnDetails({ formula: "1 + 1", name: "Math" });
+ cy.button("Done").click();
cy.findByText("Summarize").click();
- popover().within(() => {
- cy.findByText("Count of rows").click();
- });
-
+ cy.findByText("Count of rows").click();
cy.findByText("Pick a column to group by").click();
- popover().within(() => {
- cy.findByText(columnName).click();
- });
+ popover()
+ .findByText("Math")
+ .click();
cy.icon("add")
.last()
@@ -177,17 +158,8 @@ describe("scenarios > question > custom columns", () => {
// add custom column
cy.findByText("Custom column").click();
- popover().within(() => {
- // Double click at the end of this command is just an ugly hack that seems to reduce the flakiness of this test a lot!
- // TODO: investigate contenteditable element - it is losing input value and I could reproduce it even locally (outside of Cypress)
- cy.get("[contenteditable='true']")
- .type("1+1")
- .click()
- .click();
- cy.findByPlaceholderText("Something nice and descriptive").type("X");
-
- cy.findByText("Done").click();
- });
+ enterCustomColumnDetails({ formula: "1 + 1", name: "x" });
+ cy.button("Done").click();
cy.button("Visualize").click();
@@ -202,9 +174,7 @@ describe("scenarios > question > custom columns", () => {
// ID should be "1" but it is picking the product ID and is showing "14"
cy.get(".TableInteractive-cellWrapper--firstColumn")
.eq(1) // the second cell from the top in the first column (the first one is a header cell)
- .within(() => {
- cy.findByText("1");
- });
+ .findByText("1");
});
it("should be able to use custom expression after aggregation (metabase#13857)", () => {
@@ -444,11 +414,10 @@ describe("scenarios > question > custom columns", () => {
it("should relay the type of a date field", () => {
openPeopleTable({ mode: "notebook" });
cy.findByText("Custom column").click();
- popover().within(() => {
- _typeUsingGet("[contenteditable='true']", "[Birth Date]", 400);
- _typeUsingPlaceholder("Something nice and descriptive", "DoB");
- cy.findByText("Done").click();
- });
+
+ enterCustomColumnDetails({ formula: "[Birth Date]", name: "DoB" });
+ cy.findByText("Done").click();
+
cy.findByText("Filter").click();
popover()
.findByText("DoB")
@@ -572,30 +541,27 @@ describe("scenarios > question > custom columns", () => {
it.skip("should handle floating point numbers with '0' omitted (metabase#15741)", () => {
openOrdersTable({ mode: "notebook" });
cy.findByText("Custom column").click();
- cy.get("[contenteditable='true']").type(".5 * [Discount]");
- cy.findByPlaceholderText("Something nice and descriptive").type("Foo");
+ enterCustomColumnDetails({ formula: ".5 * [Discount]", name: "Foo" });
cy.findByText("Unknown Field: .5").should("not.exist");
cy.button("Done").should("not.be.disabled");
});
- describe.skip("contentedtable field (metabase#15734)", () => {
+ describe("ExpressionEditorTextfield", () => {
beforeEach(() => {
// This is the default screen size but we need it explicitly set for this test because of the resize later on
cy.viewport(1280, 800);
openOrdersTable({ mode: "notebook" });
cy.findByText("Custom column").click();
- popover().within(() => {
- cy.get("[contenteditable='true']")
- .as("formula")
- .type("1+1")
- .blur();
+
+ enterCustomColumnDetails({
+ formula: "1+1", // Formula was intentionally written without spaces (important for this repro)!
+ name: "Math",
});
- cy.findByPlaceholderText("Something nice and descriptive").type("Math");
cy.button("Done").should("not.be.disabled");
});
- it("should not accidentally delete CC formula value and/or CC name (metabase#15734-1)", () => {
+ it("should not accidentally delete Custom Column formula value and/or Custom Column name (metabase#15734)", () => {
cy.get("@formula")
.click()
.type("{movetoend}{leftarrow}{movetostart}{rightarrow}{rightarrow}")
@@ -606,23 +572,23 @@ describe("scenarios > question > custom columns", () => {
/**
* 1. Explanation for `cy.get("@formula").click();`
- * - Without it, test runner is too fast and the test resutls in false positive.
+ * - Without it, test runner is too fast and the test results in false positive.
* - This gives it enough time to update the DOM. The same result can be achieved with `cy.wait(1)`
*/
- it("should not erase CC formula and CC name when expression is incomplete (metabase#15734-2)", () => {
+ it("should not erase Custom column formula and Custom column name when expression is incomplete (metabase#16126)", () => {
cy.get("@formula")
.click()
.type("{movetoend}{backspace}")
.blur();
cy.findByText("Expected expression");
cy.button("Done").should("be.disabled");
- cy.get("@formula").click(); /* [1] */
+ cy.get("@formula").click(); /* See comment (1) above */
cy.findByDisplayValue("Math");
});
- it("should not erase CC formula and CC name on window resize (metabase#15734-3)", () => {
+ it("should not erase Custom Column formula and Custom Column name on window resize (metabase#16127)", () => {
cy.viewport(1260, 800);
- cy.get("@formula").click(); /* [1] */
+ cy.get("@formula").click(); /* See comment (1) above */
cy.findByDisplayValue("Math");
cy.button("Done").should("not.be.disabled");
});
@@ -631,13 +597,11 @@ describe("scenarios > question > custom columns", () => {
it("should maintain data type (metabase#13122)", () => {
openOrdersTable({ mode: "notebook" });
cy.findByText("Custom column").click();
- popover().within(() => {
- cy.get("[contenteditable='true']")
- .type("case([Discount] > 0, [Created At], [Product → Created At])")
- .blur();
- cy.findByPlaceholderText("Something nice and descriptive").type("13112");
- cy.button("Done").click();
+ enterCustomColumnDetails({
+ formula: "case([Discount] > 0, [Created At], [Product → Created At])",
+ name: "13112",
});
+ cy.button("Done").click();
cy.findByText("Filter").click();
popover()
.findByText("13112")
@@ -648,13 +612,11 @@ describe("scenarios > question > custom columns", () => {
it("filter based on `concat` function should not offer numeric options (metabase#13217)", () => {
openPeopleTable({ mode: "notebook" });
cy.findByText("Custom column").click();
- popover().within(() => {
- cy.get("[contenteditable='true']")
- .type(`concat("State: ", [State])`)
- .blur();
- cy.findByPlaceholderText("Something nice and descriptive").type("13217");
- cy.button("Done").click();
+ enterCustomColumnDetails({
+ formula: `concat("State: ", [State])`,
+ name: "13217",
});
+ cy.button("Done").click();
cy.findByText("Filter").click();
popover()
.findByText("13217")
@@ -680,15 +642,11 @@ describe("scenarios > question > custom columns", () => {
openOrdersTable({ mode: "notebook" });
cy.findByText("Custom column").click();
- popover().within(() => {
- cy.get("[contenteditable='true']").type(`isnull([Discount])`, {
- delay: 100,
- });
- cy.findByPlaceholderText("Something nice and descriptive").type(
- "No discount",
- );
- cy.button("Done").click();
+ enterCustomColumnDetails({
+ formula: `isnull([Discount])`,
+ name: "No discount",
});
+ cy.button("Done").click();
cy.button("Visualize").click();
cy.wait("@dataset").then(xhr => {
expect(xhr.response.body.error).to.not.exist;
@@ -727,3 +685,10 @@ describe("scenarios > question > custom columns", () => {
cy.findByText("MiscDate");
});
});
+
+function enterCustomColumnDetails({ formula, name } = {}) {
+ cy.get("[contenteditable='true']")
+ .as("formula")
+ .type(formula);
+ cy.findByPlaceholderText("Something nice and descriptive").type(name);
+}
diff --git a/frontend/test/metabase/scenarios/question/filter.cy.spec.js b/frontend/test/metabase/scenarios/question/filter.cy.spec.js
index bd7fb81688290d2ff495dc00e98c767474698a16..cf7b47ed08b56bcb363324d4ad6f4270d6736274 100644
--- a/frontend/test/metabase/scenarios/question/filter.cy.spec.js
+++ b/frontend/test/metabase/scenarios/question/filter.cy.spec.js
@@ -1011,6 +1011,129 @@ describe("scenarios > question > filter", () => {
cy.findByText("AL").click();
cy.button("Add filter").isVisibleInPopover();
});
+
+ it.skip("shoud retain all data series after saving a question where custom expression formula is the first metric (metabase#15882)", () => {
+ visitQuestionAdhoc({
+ dataset_query: {
+ database: 1,
+ query: {
+ "source-table": ORDERS_ID,
+ aggregation: [
+ [
+ "aggregation-options",
+ [
+ "/",
+ ["sum", ["field", ORDERS.DISCOUNT, null]],
+ ["sum", ["field", ORDERS.SUBTOTAL, null]],
+ ],
+ { "display-name": "Discount %" },
+ ],
+ ["count"],
+ ["avg", ["field", ORDERS.TOTAL, null]],
+ ],
+ breakout: [
+ ["field", ORDERS.CREATED_AT, { "temporal-unit": "month" }],
+ ],
+ },
+ type: "query",
+ },
+ display: "line",
+ });
+ assertOnLegendLabels();
+ cy.get(".line").should("have.length", 3);
+ cy.findByText("Save").click();
+ cy.button("Save").click();
+ cy.button("Not now").click();
+ assertOnLegendLabels();
+ cy.get(".line").should("have.length", 3);
+
+ function assertOnLegendLabels() {
+ cy.get(".Card-title")
+ .should("contain", "Discount %")
+ .and("contain", "Count")
+ .and("contain", "Average of Total");
+ }
+ });
+
+ describe.skip("specific combination of filters can cause frontend reload or blank screen (metabase#16198)", () => {
+ it("shouldn't display chosen category in a breadcrumb (metabase#16198-1)", () => {
+ visitQuestionAdhoc({
+ dataset_query: {
+ database: 1,
+ query: {
+ "source-table": PRODUCTS_ID,
+ filter: [
+ "and",
+ ["=", ["field", PRODUCTS.CATEGORY, null], "Gizmo"],
+ ["=", ["field", PRODUCTS.ID, null], 1],
+ ],
+ },
+ type: "query",
+ },
+ });
+
+ cy.findByRole("link", { name: "Sample Dataset" })
+ .parent()
+ .within(() => {
+ cy.findByText("Gizmo").should("not.exist");
+ });
+ });
+
+ it("adding an ID filter shouldn't cause page error and page reload (metabase#16198-2)", () => {
+ openOrdersTable({ mode: "notebook" });
+ cy.findByText("Filter").click();
+ cy.findByText("Custom Expression").click();
+ cy.get("[contenteditable=true]")
+ .type("[Total] < [Product → Price]")
+ .blur();
+ cy.button("Done").click();
+ // Filter currently says "Total is less than..." but it can change in https://github.com/metabase/metabase/pull/16174 to "Total < Price"
+ // See: https://github.com/metabase/metabase/pull/16209#discussion_r638129099
+ cy.findByText(/^Total/);
+ cy.icon("add")
+ .last()
+ .click();
+ popover()
+ .findByText(/^ID$/i)
+ .click();
+ cy.findByPlaceholderText("Enter an ID").type("1");
+ cy.button("Add filter").click();
+ cy.findByText(/^Total/);
+ cy.findByText("Something went wrong").should("not.exist");
+ });
+
+ it("removing first filter in a sequence shouldn't result in an empty page (metabase#16198-3)", () => {
+ openOrdersTable({ mode: "notebook" });
+ cy.findByText("Filter").click();
+ popover()
+ .findByText("Total")
+ .click();
+ cy.findByPlaceholderText("Enter a number").type("123");
+ cy.button("Add filter").click();
+ cy.icon("add")
+ .last()
+ .click();
+ cy.findByText("Custom Expression").click();
+ cy.get("[contenteditable=true]")
+ .type("[Total] < [Product → Price]")
+ .blur();
+ cy.button("Done").click();
+ // cy.findByText(/^Total/);
+ cy.icon("add")
+ .last()
+ .click();
+ popover()
+ .findByText(/^ID$/i)
+ .click();
+ cy.findByPlaceholderText("Enter an ID").type("1");
+ cy.button("Add filter").click();
+ cy.findByText("Total is equal to 123")
+ .parent()
+ .find(".Icon-close")
+ .click();
+ cy.button("Visualize");
+ });
+ });
});
function openExpressionEditorFromFreshlyLoadedPage() {
diff --git a/frontend/test/metabase/scenarios/question/nested.cy.spec.js b/frontend/test/metabase/scenarios/question/nested.cy.spec.js
index 5b1aefae8484ab6aa3df8f17017f2350982d80d7..c9a6d2c3d00c4134b3667613d3c3c34ed38e4c7f 100644
--- a/frontend/test/metabase/scenarios/question/nested.cy.spec.js
+++ b/frontend/test/metabase/scenarios/question/nested.cy.spec.js
@@ -5,6 +5,7 @@ import {
openOrdersTable,
remapDisplayValueToFK,
sidebar,
+ visitQuestionAdhoc,
} from "__support__/e2e/cypress";
import { SAMPLE_DATASET } from "__support__/e2e/cypress_sample_dataset";
@@ -223,68 +224,54 @@ describe("scenarios > question > nested", () => {
});
});
- it.skip("should apply metrics including filter to the nested question (metabase#12507)", () => {
- const METRIC_NAME = "Discount Applied";
+ it("should apply metrics including filter to the nested question (metabase#12507)", () => {
+ const METRIC_NAME = "Sum of discounts";
cy.log("Create a metric with a filter");
-
cy.request("POST", "/api/metric", {
name: METRIC_NAME,
description: "Discounted orders.",
+ table_id: ORDERS_ID,
definition: {
- aggregation: [["count"]],
- filter: ["not-null", ["field", ORDERS.DISCOUNT, null]],
"source-table": ORDERS_ID,
+ aggregation: [["count"]],
+ filter: ["!=", ["field", ORDERS.DISCOUNT, null], 0],
},
- table_id: ORDERS_ID,
- }).then(({ body: { id: METRIC_ID } }) => {
+ }).then(({ body: { id: metricId } }) => {
// "capture" the original query because we will need to re-use it later in a nested question as "source-query"
const ORIGINAL_QUERY = {
- aggregation: ["metric", METRIC_ID],
+ "source-table": ORDERS_ID,
+ aggregation: [["metric", metricId]],
breakout: [
["field", ORDERS.TOTAL, { binning: { strategy: "default" } }],
],
- "source-table": ORDERS_ID,
};
// Create new question which uses previously defined metric
cy.createQuestion({
- name: "Orders with discount applied",
+ name: "12507",
query: ORIGINAL_QUERY,
- display: "bar",
- visualization_settings: {
- "graph.dimension": ["TOTAL"],
- "graph.metrics": [METRIC_NAME],
- },
- }).then(({ body: { id: QUESTION_ID } }) => {
- cy.server();
- cy.route("POST", `/api/card/${QUESTION_ID}/query`).as("cardQuery");
+ }).then(({ body: { id: questionId } }) => {
+ cy.intercept("POST", `/api/card/${questionId}/query`).as("cardQuery");
- cy.log("Create a nested question based on the previous one");
-
- // we're adding filter and saving/overwriting the original question, keeping the same ID
- cy.request("PUT", `/api/card/${QUESTION_ID}`, {
+ cy.log("Create and visit a nested question based on the previous one");
+ visitQuestionAdhoc({
dataset_query: {
- database: 1,
+ type: "query",
query: {
- filter: [
- ">",
- ["field", "TOTAL", { "base-type": "type/Float" }],
- 50,
- ],
- "source-query": ORIGINAL_QUERY,
+ "source-table": `card__${questionId}`,
+ filter: [">", ["field", ORDERS.TOTAL, null], 50],
},
- type: "query",
+ database: 1,
},
});
cy.log("Reported failing since v0.35.2");
- cy.visit(`/question/${QUESTION_ID}`);
+ cy.visit(`/question/${questionId}`);
cy.wait("@cardQuery").then(xhr => {
expect(xhr.response.body.error).not.to.exist;
});
- cy.findByText(METRIC_NAME);
- cy.get(".bar");
+ cy.get(".cellData").contains(METRIC_NAME);
});
});
});
@@ -528,7 +515,7 @@ describe("scenarios > question > nested", () => {
}
});
- describe.skip("should not remove user defined metric when summarizing based on saved question (metabase#15725)", () => {
+ describe("should not remove user defined metric when summarizing based on saved question (metabase#15725)", () => {
beforeEach(() => {
cy.intercept("POST", "/api/dataset").as("dataset");
cy.createNativeQuestion({
diff --git a/frontend/test/metabase/scenarios/question/new.cy.spec.js b/frontend/test/metabase/scenarios/question/new.cy.spec.js
index 6fbf9974302309a10a420b886d8430c26a1bbd07..14bc5532de1ec291e70c0b4f70ee520b964e63ef 100644
--- a/frontend/test/metabase/scenarios/question/new.cy.spec.js
+++ b/frontend/test/metabase/scenarios/question/new.cy.spec.js
@@ -75,6 +75,16 @@ describe("scenarios > question > new", () => {
cy.get(".Visualization .bar").should("have.length", 6);
});
+ it.skip("should display a tooltip for CTA icons on an individual question (metabase#16108)", () => {
+ openOrdersTable();
+ cy.icon("download").realHover();
+ cy.findByText("Download full results");
+ cy.icon("bell").realHover();
+ cy.findByText("Get alerts");
+ cy.icon("share").realHover();
+ cy.findByText("Sharing");
+ });
+
describe("browse data", () => {
it("should load orders table and summarize", () => {
cy.visit("/");
diff --git a/frontend/test/metabase/scenarios/visualizations/gauge.cy.spec.js b/frontend/test/metabase/scenarios/visualizations/gauge.cy.spec.js
index 39b5f39c4501b6f35fa341554fba49e27e78db87..febacf24c51b265ad2727760b75307970d8a17ac 100644
--- a/frontend/test/metabase/scenarios/visualizations/gauge.cy.spec.js
+++ b/frontend/test/metabase/scenarios/visualizations/gauge.cy.spec.js
@@ -10,7 +10,7 @@ describe("scenarios > visualizations > gauge chart", () => {
cy.intercept("POST", "/api/dataset").as("dataset");
});
- it.skip("should not rerender on gauge arc hover (metabase#15980)", () => {
+ it("should not rerender on gauge arc hover (metabase#15980)", () => {
cy.createQuestion({
name: "15980",
query: { "source-table": ORDERS_ID, aggregation: [["count"]] },
diff --git a/frontend/test/metabase/scenarios/visualizations/line_chart.cy.spec.js b/frontend/test/metabase/scenarios/visualizations/line_chart.cy.spec.js
index a4f7cc1433500e67ea803530c0c0e29bcb793dd9..3415b251f43b6c28d716d5630db2b9997697a9c7 100644
--- a/frontend/test/metabase/scenarios/visualizations/line_chart.cy.spec.js
+++ b/frontend/test/metabase/scenarios/visualizations/line_chart.cy.spec.js
@@ -1,7 +1,7 @@
import { restore, visitQuestionAdhoc, popover } from "__support__/e2e/cypress";
import { SAMPLE_DATASET } from "__support__/e2e/cypress_sample_dataset";
-const { ORDERS, ORDERS_ID, PRODUCTS } = SAMPLE_DATASET;
+const { ORDERS, ORDERS_ID, PRODUCTS, PRODUCTS_ID } = SAMPLE_DATASET;
const Y_AXIS_RIGHT_SELECTOR = ".axis.yr";
@@ -68,7 +68,7 @@ describe("scenarios > visualizations > line chart", () => {
cy.get(".value-labels").contains("30%");
});
- it.skip("should correctly display tooltip values when X-axis is numeric and style is 'Ordinal' (metabase#15998)", () => {
+ it("should correctly display tooltip values when X-axis is numeric and style is 'Ordinal' (metabase#15998)", () => {
visitQuestionAdhoc({
dataset_query: {
database: 1,
@@ -104,6 +104,198 @@ describe("scenarios > visualizations > line chart", () => {
testPairedTooltipValues("Average of Quantity", "4");
});
});
+
+ describe.skip("tooltip of combined dashboard cards (multi-series) should show the correct column title (metabase#16249", () => {
+ const RENAMED_FIRST_SERIES = "Foo";
+ const RENAMED_SECOND_SERIES = "Bar";
+
+ it("custom expression names (metabase#16249-1)", () => {
+ createOrdersQuestionWithAggregation({
+ name: "16249_Q1",
+ aggregation: [
+ [
+ "aggregation-options",
+ ["sum", ["field", ORDERS.TOTAL, null]],
+ { "display-name": "CE" },
+ ],
+ ],
+ }).then(({ body: { id: question1Id } }) => {
+ createOrdersQuestionWithAggregation({
+ name: "16249_Q2",
+ aggregation: [
+ [
+ "aggregation-options",
+ ["avg", ["field", ORDERS.SUBTOTAL, null]],
+ { "display-name": "CE" },
+ ],
+ ],
+ }).then(({ body: { id: question2Id } }) => {
+ cy.createDashboard("16249D").then(({ body: { id: dashboardId } }) => {
+ addBothSeriesToDashboard({
+ dashboardId,
+ firstCardId: question1Id,
+ secondCardId: question2Id,
+ });
+ cy.visit(`/dashboard/${dashboardId}`);
+
+ // Rename both series
+ renameSeries([
+ ["16249_Q1", RENAMED_FIRST_SERIES],
+ ["16249_Q2", RENAMED_SECOND_SERIES],
+ ]);
+
+ assertOnLegendItemsValues();
+ assertOnYAxisValues();
+
+ showTooltipForFirstCircleInSeries(0);
+ popover().within(() => {
+ testPairedTooltipValues("Created At", "2016");
+ testPairedTooltipValues(RENAMED_FIRST_SERIES, "42,156.87");
+ });
+
+ showTooltipForFirstCircleInSeries(1);
+ popover().within(() => {
+ testPairedTooltipValues("Created At", "2016");
+ testPairedTooltipValues(RENAMED_SECOND_SERIES, "54.44");
+ });
+ });
+ });
+ });
+ });
+
+ it("regular column names (metabase#16249-2)", () => {
+ createOrdersQuestionWithAggregation({
+ name: "16249_Q3",
+ aggregation: [["sum", ["field", ORDERS.TOTAL, null]]],
+ }).then(({ body: { id: question1Id } }) => {
+ cy.createQuestion({
+ name: "16249_Q4",
+ query: {
+ "source-table": PRODUCTS_ID,
+ aggregation: [["sum", ["field", PRODUCTS.PRICE, null]]],
+ breakout: [
+ ["field", PRODUCTS.CREATED_AT, { "temporal-unit": "year" }],
+ ],
+ },
+ display: "line",
+ }).then(({ body: { id: question2Id } }) => {
+ cy.createDashboard("16249D").then(({ body: { id: dashboardId } }) => {
+ addBothSeriesToDashboard({
+ dashboardId,
+ firstCardId: question1Id,
+ secondCardId: question2Id,
+ });
+
+ cy.visit(`/dashboard/${dashboardId}`);
+
+ renameSeries([
+ ["16249_Q3", RENAMED_FIRST_SERIES],
+ ["16249_Q4", RENAMED_SECOND_SERIES],
+ ]);
+
+ assertOnLegendItemsValues();
+ assertOnYAxisValues();
+
+ showTooltipForFirstCircleInSeries(0);
+ popover().within(() => {
+ testPairedTooltipValues("Created At", "2016");
+ testPairedTooltipValues(RENAMED_FIRST_SERIES, "42,156.87");
+ });
+
+ showTooltipForFirstCircleInSeries(1);
+ popover().within(() => {
+ testPairedTooltipValues("Created At", "2016");
+ testPairedTooltipValues(RENAMED_SECOND_SERIES, "2,829.03");
+ });
+ });
+ });
+ });
+ });
+
+ /**
+ * Helper functions related to repros around 16249 only!
+ * Note:
+ * - This might be too abstract and highly specific.
+ * - That's true in general sense, but that's the reason we're not using them anywhere else than here.
+ * - Without these abstractions, both tests would be MUCH longer and harder to review.
+ */
+
+ function addBothSeriesToDashboard({
+ dashboardId,
+ firstCardId,
+ secondCardId,
+ } = {}) {
+ // Add the first question to the dashboard
+ cy.request("POST", `/api/dashboard/${dashboardId}/cards`, {
+ cardId: firstCardId,
+ }).then(({ body: { id: dashCardId } }) => {
+ // Combine the second question with the first one as the second series
+ cy.request("PUT", `/api/dashboard/${dashboardId}/cards`, {
+ cards: [
+ {
+ id: dashCardId,
+ card_id: firstCardId,
+ row: 0,
+ col: 0,
+ sizeX: 18,
+ sizeY: 12,
+ series: [
+ {
+ id: secondCardId,
+ },
+ ],
+ parameter_mappings: [],
+ },
+ ],
+ });
+ });
+ }
+
+ function createOrdersQuestionWithAggregation({ name, aggregation } = {}) {
+ return cy.createQuestion({
+ name,
+ query: {
+ "source-table": ORDERS_ID,
+ aggregation,
+ breakout: [["field", ORDERS.CREATED_AT, { "temporal-unit": "year" }]],
+ },
+ display: "line",
+ });
+ }
+
+ function renameSeries(series) {
+ cy.icon("pencil").click();
+ cy.get(".Card").realHover();
+ cy.icon("palette").click();
+ series.forEach(serie => {
+ const [old_name, new_name] = serie;
+
+ cy.findByDisplayValue(old_name)
+ .clear()
+ .type(new_name);
+ });
+
+ cy.get(".Modal")
+ .as("modal")
+ .within(() => {
+ cy.button("Done").click();
+ });
+ cy.button("Save").click();
+ cy.findByText("You're editing this dashboard.").should("not.exist");
+ }
+
+ function assertOnLegendItemsValues() {
+ cy.get(".LegendItem")
+ .should("contain", RENAMED_FIRST_SERIES)
+ .and("contain", RENAMED_SECOND_SERIES);
+ }
+
+ function assertOnYAxisValues() {
+ cy.get(".y-axis-label")
+ .should("contain", RENAMED_FIRST_SERIES)
+ .and("contain", RENAMED_SECOND_SERIES);
+ }
+ });
});
function testPairedTooltipValues(val1, val2) {
@@ -112,3 +304,11 @@ function testPairedTooltipValues(val1, val2) {
.siblings("td")
.findByText(val2);
}
+
+function showTooltipForFirstCircleInSeries(series_index) {
+ cy.get(`.sub._${series_index}`)
+ .as("firstSeries")
+ .find("circle")
+ .first()
+ .realHover();
+}
diff --git a/frontend/test/metabase/scenarios/visualizations/pivot_tables.cy.spec.js b/frontend/test/metabase/scenarios/visualizations/pivot_tables.cy.spec.js
index 462b7dd26d4d06fb8d567b87184a8c9e6690f597..37cb88441becc4b52dca238a4100f531de741ce1 100644
--- a/frontend/test/metabase/scenarios/visualizations/pivot_tables.cy.spec.js
+++ b/frontend/test/metabase/scenarios/visualizations/pivot_tables.cy.spec.js
@@ -750,6 +750,66 @@ describe("scenarios > visualizations > pivot tables", () => {
cy.findByText("Grand totals");
});
});
+
+ it.skip("should show stand-alone row values in grouping when rows are collapsed (metabase#15211)", () => {
+ visitQuestionAdhoc({
+ dataset_query: {
+ type: "query",
+ query: {
+ "source-table": ORDERS_ID,
+ aggregation: [["sum", ["field", ORDERS.DISCOUNT, null]], ["count"]],
+ breakout: [
+ ["field", ORDERS.CREATED_AT, { "temporal-unit": "day" }],
+ ["field", ORDERS.PRODUCT_ID, null],
+ ],
+ filter: [
+ "and",
+ [
+ "between",
+ ["field", ORDERS.CREATED_AT, null],
+ "2016-11-09",
+ "2016-11-11",
+ ],
+ ["!=", ["field", ORDERS.PRODUCT_ID, null], 146],
+ ],
+ },
+ database: 1,
+ },
+ display: "pivot",
+ visualization_settings: {
+ "pivot_table.column_split": {
+ rows: [
+ ["field", ORDERS.CREATED_AT, { "temporal-unit": "day" }],
+ ["field", ORDERS.PRODUCT_ID, null],
+ ],
+ columns: [],
+ values: [["aggregation", 0], ["aggregation", 1]],
+ },
+ "pivot_table.collapsed_rows": {
+ value: [],
+ rows: [
+ ["field", ORDERS.CREATED_AT, { "temporal-unit": "day" }],
+ ["field", ORDERS.PRODUCT_ID, null],
+ ],
+ },
+ },
+ });
+
+ cy.findByText("November 9, 2016");
+ cy.findByText("November 10, 2016");
+ cy.findByText("November 11, 2016");
+ collapseRowsFor("Created At: Day");
+ cy.findByText("Totals for November 9, 2016");
+ cy.findByText("Totals for November 10, 2016");
+ cy.findByText("Totals for November 11, 2016");
+
+ function collapseRowsFor(column_name) {
+ cy.findByText(column_name)
+ .parent()
+ .find(".Icon-dash")
+ .click();
+ }
+ });
});
const testQuery = {
diff --git a/frontend/test/metabase/scenarios/visualizations/waterfall.cy.spec.js b/frontend/test/metabase/scenarios/visualizations/waterfall.cy.spec.js
index 3a79e63b72bc0be398afa7a1884fb9b4ce019485..ad8d3eb73faeb75a13541c943b7d84e77c0c5c0d 100644
--- a/frontend/test/metabase/scenarios/visualizations/waterfall.cy.spec.js
+++ b/frontend/test/metabase/scenarios/visualizations/waterfall.cy.spec.js
@@ -152,6 +152,35 @@ describe("scenarios > visualizations > waterfall", () => {
cy.get(".Visualization .bar");
});
+ it.skip("should display correct values when one of them is 0 (metabase#16246)", () => {
+ visitQuestionAdhoc({
+ dataset_query: {
+ type: "native",
+ native: {
+ query:
+ "SELECT * FROM (\nVALUES \n('a',2),\n('b',1),\n('c',-0.5),\n('d',-0.5),\n('e',0.1),\n('f',0),\n('g', -2)\n)\n",
+ "template-tags": {},
+ },
+ database: 1,
+ },
+ display: "waterfall",
+ visualization_settings: {
+ "graph.show_values": true,
+ },
+ });
+
+ cy.get(".value-label")
+ .as("labels")
+ .eq(-3)
+ .invoke("text")
+ .should("eq", "0");
+
+ cy.get("@labels")
+ .last()
+ .invoke("text")
+ .should("eq", "0.1");
+ });
+
describe("scenarios > visualizations > waterfall settings", () => {
beforeEach(() => {
restore();
diff --git a/shared/src/metabase/mbql/schema.cljc b/shared/src/metabase/mbql/schema.cljc
index 190b281313ea330d0f81b278288425b1fa75011a..5f57515789f1056a571d71ac5fc2b1a397c67ddc 100644
--- a/shared/src/metabase/mbql/schema.cljc
+++ b/shared/src/metabase/mbql/schema.cljc
@@ -990,7 +990,7 @@
(every-pred
(some-fn :source-table :source-query)
(complement (every-pred :source-table :source-query)))
- "Joins can must have either a `source-table` or `source-query`, but not both.")))
+ "Joins must have either a `source-table` or `source-query`, but not both.")))
(def Joins
"Schema for a valid sequence of `Join`s. Must be a non-empty sequence, and `:alias`, if specified, must be unique."
diff --git a/shared/src/metabase/mbql/util.cljc b/shared/src/metabase/mbql/util.cljc
index f2ae37e8fa1f80cf77e6bd4eb4f28de869c093ff..b919af2eb3dea762345494e341fc23615b804931 100644
--- a/shared/src/metabase/mbql/util.cljc
+++ b/shared/src/metabase/mbql/util.cljc
@@ -120,12 +120,16 @@
[filter-clause & more-filter-clauses]
(simplify-compound-filter (cons :and (cons filter-clause more-filter-clauses))))
+(s/defn add-filter-clause-to-inner-query :- mbql.s/MBQLQuery
+ [inner-query :- mbql.s/MBQLQuery new-clause :- (s/maybe mbql.s/Filter)]
+ (if-not new-clause
+ inner-query
+ (update inner-query :filter combine-filter-clauses new-clause)))
+
(s/defn add-filter-clause :- mbql.s/Query
"Add an additional filter clause to an `outer-query`. If `new-clause` is `nil` this is a no-op."
- [outer-query :- mbql.s/Query, new-clause :- (s/maybe mbql.s/Filter)]
- (if-not new-clause
- outer-query
- (update-in outer-query [:query :filter] combine-filter-clauses new-clause)))
+ [outer-query :- mbql.s/Query new-clause :- (s/maybe mbql.s/Filter)]
+ (update outer-query :query add-filter-clause-to-inner-query new-clause))
(defn desugar-inside
"Rewrite `:inside` filter clauses as a pair of `:between` clauses."
diff --git a/shared/src/metabase/shared/models/visualization_settings.cljc b/shared/src/metabase/shared/models/visualization_settings.cljc
new file mode 100644
index 0000000000000000000000000000000000000000..07341bc223178f1b8bd7eba0073d8d9b704c9be4
--- /dev/null
+++ b/shared/src/metabase/shared/models/visualization_settings.cljc
@@ -0,0 +1,590 @@
+(ns metabase.shared.models.visualization-settings
+ "Utility code for dealing with visualization settings, from cards, dashboard cards, etc.
+
+ There are two ways of representing the same data, DB form and normalized form. DB form is the \"legacy\" form, which
+ uses unqualified keywords, which map directly to column names via Toucan. Normalized form, on the other hand, uses
+ namespaced keywords and generally \"unwraps\" the semantic structures as much as possible.
+
+ In general, operations/manipulations should happen on the normalized form, and when the DB form is needed again (ex:
+ for updating the database), the map can be converted back. This can be done fairly easily with the threading macro,
+ ex:
+
+ ```
+ (-> (mb.viz/from-db-form (:visualization_settings my-card))
+ tweak-viz-settings
+ tweak-more-viz-settings
+ mb.viz/db-form)
+ ```
+
+ In general, conversion functions in this namespace (i.e. those that convert various pieces from one form to the other)
+ will be prefixed with either `db->norm` or `norm->db`, depending on which direction they implement.
+ "
+ #?@
+ (:clj
+ [(:require [cheshire.core :as json]
+ [clojure.set :as set]
+ [clojure.spec.alpha :as s]
+ [medley.core :as m]
+ [metabase.mbql.normalize :as mbql.normalize])]
+ :cljs
+ [(:require [cljs.js :as js]
+ [clojure.set :as set]
+ [clojure.spec.alpha :as s]
+ [medley.core :as m]
+ [metabase.mbql.normalize :as mbql.normalize])]))
+
+;;; -------------------------------------------------- Main API --------------------------------------------------
+
+
+;;; -------------------------------------------------- Specs --------------------------------------------------
+
+(s/def ::field-id integer?)
+(s/def ::column-name string?)
+
+;; a field reference that is a string, which could be a reference to some named field (ex: output of an aggregation)
+;; or to a fully qualified field name (in the context of serialization); we won't attempt to interpret it here, only
+;; report that it's a string and set it in the ref map appropriately
+(s/def ::field-str string?)
+
+(s/def ::field-metadata (s/or :nil? nil? :map? map?))
+(s/def ::column-ref (s/keys :opt [::field-id ::column-name ::field-str ::field-metadata]))
+
+(s/def ::column-settings (s/keys))
+(s/def ::click-behavior (s/keys))
+(s/def ::visualization-settings (s/keys :opt [::column-settings ::click-behavior]))
+
+(s/def ::db-column-ref-vec (s/or :field (s/tuple (partial = "ref") (s/tuple (partial = "field")
+ (s/or :field-id int? :field-str string?)
+ (s/or :field-metadata map? :nil nil?)))
+ :column-name (s/tuple (partial = "name") string?)))
+
+(s/def ::click-behavior-type keyword? #_(s/or :cross-filter ::cross-filter
+ :link ::link))
+
+(s/def ::click-behavior (s/keys :req [::click-behavior-type]
+ :opt [::link-type ::parameter-mapping ::link-template ::link-text ::link-target-id]))
+
+;; TODO: add more specific shape for this one
+(s/def ::parameter-mapping (s/or :nil? nil? :map? map?))
+
+;; target ID can be the auto generated ID or fully qualified name for serialization
+(s/def ::link-target-id (s/or :int int? :fully-qualified-name string?))
+(s/def ::link-template string?)
+(s/def ::link-text-template string?)
+
+(s/def ::column-title string?)
+(s/def ::date-style #{"M/D/YYYY" "D/M/YYYY" "YYYY/M/D" "MMMM D, YYYY" "D MMMM, YYYY" "dddd, MMMM D, YYYY"})
+(s/def ::date-abbreviate boolean?)
+(s/def ::time-style #{"h:mm A" "k:mm" "h A"})
+(s/def ::time-enabled #{nil "minutes" "seconds" "milliseconds"})
+(s/def ::decimals pos-int?)
+(s/def ::number-separators #(or nil? (and string? (= 2 (count %)))))
+(s/def ::number-style #{"decimal" "percent" "scientific" "currency"})
+(s/def ::prefix string?)
+(s/def ::suffix string?)
+(s/def ::view-as string?)
+(s/def ::link-text string?)
+
+(s/def ::param-mapping-id string?)
+
+(s/def ::param-ref-type #{"column" "dimension" "variable" "parameter"})
+(s/def ::param-ref-id string?)
+(s/def ::param-ref-name string?)
+
+(s/def ::param-mapping-source (s/keys :req [::param-ref-id ::param-ref-type] :opt [::param-ref-name]))
+(s/def ::param-mapping-target ::param-mapping-source)
+
+(s/def ::db-column-ref (s/or :string? string? :vector? vector? :keyword? keyword?))
+
+(s/def ::entity-type #{::card ::dashboard})
+
+;;; ----------------------------------------------- Parsing fns -----------------------------------------------
+
+(defn field-id->column-ref
+ "Creates a normalized column ref map for the given field ID. This becomes a key in the `::column-settings` map.
+
+ If passed, `field-metadata` is also included in the map (but not interpreted)."
+ {:added "0.40.0"}
+ [field-id & [field-metadata]]
+ (cond-> {::field-id field-id}
+ (some? field-metadata) (assoc ::field-metadata field-metadata)))
+
+(s/fdef field-id->column-ref
+ :args (s/cat :field-id int? :field-metadata (s/? ::field-metadata))
+ :ret ::column-ref)
+
+(defn column-name->column-ref
+ "Creates a normalized column ref map for the given `col-name`. This becomes a key in the `::column-settings` map."
+ {:added "0.40.0"}
+ [col-name]
+ {::column-name col-name})
+
+(s/fdef column-name->column-ref
+ :args (s/cat :col-name string?)
+ :ret ::column-ref)
+
+(defn field-str->column-ref
+ "Creates a normalized column ref map for the given field string (which could be the name of a \"synthetic\" field,
+ such as the output of an aggregation, or a fully qualified field name in the context of serialization. The
+ visualization settings code will not make any attempt to interpret this string. It becomes the
+ key in the `::column-settings` map.
+
+ If passed, `field-metadata` is also included in the map (but not interpreted)."
+ {:added "0.40.0"}
+ [field-qualified-name & [field-metadata]]
+ (cond-> {::field-str field-qualified-name}
+ (some? field-metadata) (assoc ::field-metadata field-metadata)))
+
+(s/fdef field-str->column-ref
+ :args (s/cat :field-qualified-name string? :field-metadata (s/? ::field-metadata))
+ :ret ::column-ref)
+
+(defn- keyname
+ "Returns the full string name of the keyword `kw`, including any \"namespace\" portion followed by forward slash.
+
+ From https://clojuredocs.org/clojure.core/name#example-58264f85e4b0782b632278bf
+ Clojure interprets slashes as keyword/name separators, so we need to do something hacky to get the \"full\" name here
+ because our \"keyword value\" (as parsed from JSON/YAML/etc.) might actually look like the string version of a
+ Clojure vector, which itself can contain a fully qualified name for serialization"
+ {:added "0.40.0"}
+ [kw]
+ (str (if-let [kw-ns (namespace kw)] (str kw-ns "/")) (name kw)))
+
+(s/fdef keyname
+ :args (s/cat :kw keyword?)
+ :ret string?)
+
+(defn- parse-json-string
+ "Parse the given `json-str` to a map. In Clojure, this uses Cheshire. In Clojurescript, it calls `.parse` with
+ `js/JSON` and threads that to `js->clj`."
+ [json-str]
+ #?(:clj (json/parse-string json-str)
+ :cljs (-> (.parse js/JSON json-str)
+ js->clj)))
+
+(s/fdef parse-json-string
+ :args (s/cat :json-str string?)
+ :ret (s/or :map map? :seq seqable?))
+
+(defn- encode-json-string
+ "Encode the given `obj` map as a JSON string. In Clojure, this uses Cheshire. In Clojurescript, it uses
+ `cljs.core.clj->js` in conjunction with `cljs.js`."
+ [obj]
+ #?(:clj (json/encode obj)
+ :cljs (.stringify js/JSON (clj->js obj))))
+
+(s/fdef encode-json-string
+ :args (s/cat :obj (s/or :map map? :seq seqable?))
+ :ret string?)
+
+(defn db->norm-column-ref
+ "Converts a (parsed, vectorized) DB-form column ref to the equivalent normal form.
+
+ Does the opposite of `norm->db-column-ref`"
+ [column-ref-vec]
+ (let [parsed (s/conform ::db-column-ref-vec column-ref-vec)]
+ (if (s/invalid? parsed)
+ (throw (ex-info "Invalid input" (s/explain-data ::db-column-ref-vec column-ref-vec)))
+ (let [[m parts] parsed]
+ (case m
+ :field
+ (let [[_ [_ [_ [id-or-str v] [_ field-md]]]] parsed]
+ (cond-> (case id-or-str
+ :field-id {::field-id v}
+ :field-str {::field-str v})
+ (some? field-md) (assoc ::field-metadata field-md)))
+ :column-name
+ {::column-name (nth parts 1)})))))
+
+(s/fdef db->norm-column-ref
+ :args (s/cat :column-ref ::db-column-ref-vec)
+ :ret ::column-ref)
+
+(defn parse-db-column-ref
+ "Parses the DB representation of a column reference, and returns the equivalent normal form.
+
+ The `column-ref` parameter can be a string, a vector, or keyword.
+
+ If a string, it is parsed as JSON, and the value is passed to `db->norm-column-ref` for conversion.
+
+ If a keyword (which is produced by YAML parsing, for instance), it will first be converted to its full name. \"Full\"
+ means that the portions before and after any slashes will be included verbatim (via the `keyname` helper fn). This is
+ necessary because our serialization code considers a forward slash to be a legitimate portion of a fully qualified
+ name, whereas Clojure considers it to be a namespace/local name separator. Once converted thusly, that resulting
+ string value will be passed to `db->norm-column-ref` for conversion, just as in the case above.
+
+ If a vector, it is assumed that vector is already in DB normalized form, so it is passed directly to
+ `db->norm-column-ref` for conversion.
+
+ Returns a map representing the column reference (conforming to the normal form `::column-ref` spec), by delegating
+ to `db->norm-column-ref`."
+ {:added "0.40.0"}
+ [column-ref]
+ (let [parsed (s/conform ::db-column-ref column-ref)]
+ (if (s/invalid? parsed)
+ (throw (ex-info "Invalid input" (s/explain-data ::db-column-ref column-ref)))
+ (let [[k v] parsed
+ ref->vec (case k
+ :string? (comp vec parse-json-string)
+ :keyword? (comp vec parse-json-string keyname)
+ :vector? identity)]
+ (db->norm-column-ref (ref->vec v))))))
+
+(s/fdef parse-db-column-ref
+ :args (s/cat :column-ref ::db-column-ref)
+ :ret ::column-ref)
+
+;;; ------------------------------------------------ Builder fns ------------------------------------------------
+
+(defn visualization-settings
+ "Creates an empty visualization settings map. Intended for use in the context of a threading macro (ex: with
+ `click-action` or a similar function following as the next form)."
+ {:added "0.40.0"}
+ []
+ {})
+
+(defn- with-col-settings [settings]
+ (if (contains? settings ::column-settings)
+ settings
+ (assoc settings ::column-settings {})))
+
+(s/fdef with-col-settings
+ :args (s/cat :settings ::visualization-settings)
+ :ret ::visualization-settings)
+
+(defn crossfilter-click-action
+ "Creates a crossfilter click action with the given `param-mapping`, in the normalized form."
+ {:added "0.40.0"}
+ [param-mapping]
+ {::click-behavior-type ::cross-filter
+ ::parameter-mapping param-mapping})
+
+(s/fdef crossfilter-click-action
+ :args (s/cat :param-mapping ::parameter-mapping)
+ :ret ::click-behavior)
+
+(defn url-click-action
+ "Creates a URL click action linking to a `url-template`, in the normalized form."
+ {:added "0.40.0"}
+ [url-template]
+ {::click-behavior-type ::link
+ ::link-type ::url
+ ::link-template url-template})
+
+(s/fdef url-click-action
+ :args (s/cat :url-template string?)
+ :ret ::click-behavior)
+
+(defn entity-click-action
+ "Creates a click action linking to an entity having `entity-type` with ID `entity-id`, in the normalized form.
+ `parameter-mapping` is an optional argument."
+ {:added "0.40.0"}
+ [entity-type entity-id & [parameter-mapping]]
+ (cond-> {::click-behavior-type ::link
+ ::link-type entity-type
+ ::link-target-id entity-id}
+ (some? parameter-mapping) (assoc ::parameter-mapping parameter-mapping)))
+
+
+(s/fdef entity-click-action
+ :args (s/cat :entity-type ::entity-type :entity-id int? :parameter-mapping ::parameter-mapping)
+ :ret ::click-behavior)
+
+(defn with-click-action
+ "Creates a click action from a given `from-field-id` Field identifier to the given `to-entity-type` having ID
+ `to-entity-id`, and adds it to the given `settings`. This happens in the normalized form, and hence this should be
+ passed the output of another fn (including, currently, `visualization-settings`). If the given `from-field-id`
+ already has a click action, it will be replaced."
+ {:added "0.40.0"}
+ [settings col-key action]
+ (-> settings
+ with-col-settings
+ (update-in [::column-settings] assoc col-key {::click-behavior action})))
+
+(s/fdef with-click-action
+ :args (s/cat :settings map? :col-key ::column-ref :action ::click-behavior)
+ :ret ::click-behavior)
+
+(defn with-entity-click-action
+ "Creates a click action from a given `from-field-id` Field identifier to the given `to-entity-type` having ID
+ `to-entity-id`. This happens in the normalized form, and hence this should be passed the output of another fn
+ (including, currently, `visualization-settings`). If the given `from-field-id` already has a click action, it will
+ be replaced."
+ {:added "0.40.0"}
+ [settings from-field-id to-entity-type to-entity-id & [parameter-mapping]]
+ (with-click-action settings (field-id->column-ref from-field-id) (entity-click-action
+ to-entity-type
+ to-entity-id
+ parameter-mapping)))
+
+(s/fdef with-entity-click-action
+ :args (s/cat :settings map?
+ :from-field-id int?
+ :to-entity-type ::entity-type
+ :to-entity-id int?
+ :parameter-mapping (s/? ::parameter-mapping) )
+ :ret ::click-behavior)
+
+(defn fk-parameter-mapping
+ "Creates a parameter mapping for `source-col-name` (`source-field-id`) to `target-field-id` in normalized form."
+ {:added "0.40.0"}
+ [source-col-name source-field-id target-field-id]
+ (let [id [:dimension [:fk-> [:field source-field-id nil] [:field target-field-id nil]]]
+ dimension {:dimension [:field target-field-id {:source-field source-field-id}]}]
+ {id #::{:param-mapping-id id
+ :param-mapping-source #::{:param-ref-type "column"
+ :param-ref-id source-col-name
+ :param-ref-name source-col-name}
+ :param-mapping-target #::{:param-ref-type "dimension"
+ :param-ref-id id
+ :param-dimension dimension}}}))
+
+(s/fdef fk-parameter-mapping
+ :args (s/cat :source-col-name string? :source-field-id int? :target-field-id int?)
+ :ret map?)
+
+;;; ---------------------------------------------- Conversion fns ----------------------------------------------
+
+(def ^:private db->norm-click-action-type
+ {"link" ::link
+ "crossfilter" ::cross-filter})
+
+(def ^:private norm->db-click-action-type
+ (set/map-invert db->norm-click-action-type))
+
+(def ^:private db->norm-link-type
+ {"question" ::card
+ "dashboard" ::dashboard
+ "url" ::url})
+
+(def ^:private norm->db-link-type
+ (set/map-invert db->norm-link-type))
+
+(def ^:private db->norm-click-behavior-keys
+ {:targetId ::link-target-id
+ :linkTemplate ::link-template
+ :linkTextTemplate ::link-text-template
+ :type ::click-behavior-type
+ :linkType ::link-type})
+
+(def ^:private norm->db-click-behavior-keys
+ (set/map-invert db->norm-click-behavior-keys))
+
+(def ^:private db->norm-column-settings-keys
+ {:column_title ::column-title
+ :date_style ::date-style
+ :date_abbreviate ::date-abbreviate
+ :time_style ::time-style
+ :time_enabled ::time-enabled
+ :decimals ::decimals
+ :number_separators ::number-separators
+ :number_style ::number-style
+ :prefix ::prefix
+ :suffix ::suffix
+ :view_as ::view-as
+ :link_text ::link-text})
+
+(def ^:private norm->db-column-settings-keys
+ (set/map-invert db->norm-column-settings-keys))
+
+(def ^:private db->norm-param-mapping-val-keys
+ {:id ::param-mapping-id
+ :source ::param-mapping-source
+ :target ::param-mapping-target})
+
+(def ^:private norm->db-param-mapping-val-keys
+ (set/map-invert db->norm-param-mapping-val-keys))
+
+(def ^:private db->norm-param-ref-keys
+ {:type ::param-ref-type
+ :id ::param-ref-id
+ :name ::param-ref-name
+ :dimension ::param-dimension})
+
+(def ^:private norm->db-param-ref-keys
+ (set/map-invert db->norm-param-ref-keys))
+
+(defn- db->norm-param-ref [parsed-id param-ref]
+ (cond-> (set/rename-keys param-ref db->norm-param-ref-keys)
+ (= "dimension" (:type param-ref)) (assoc ::param-ref-id parsed-id)))
+
+(defn- norm->db-param-ref [id-str param-ref]
+ (cond-> (set/rename-keys param-ref norm->db-param-ref-keys)
+ (= "dimension" (::param-ref-type param-ref)) (assoc :id id-str)))
+
+(defn db->norm-param-mapping
+ "Converts a `parameter-mapping` (i.e. value of `:parameterMapping`) from DB to normalized form"
+ {:added "0.40.0"}
+ [parameter-mapping]
+ (if (nil? parameter-mapping)
+ nil
+ ;; k is "[\"dimension\",[\"fk->\",[\"field-id\",%d],[\"field-id\",%d]]]"
+ ;; v is {:id <same long string> :source <param-ref> :target <param-ref>}
+ (reduce-kv (fn [acc k v]
+ (let [[new-k new-v]
+ (if (= "dimension" (get-in v [:target :type]))
+ (let [parsed-id (-> (if (keyword? k) (keyname k) k)
+ parse-json-string
+ mbql.normalize/normalize-tokens)]
+ [parsed-id (cond-> v
+ (:source v) (assoc ::param-mapping-source
+ (db->norm-param-ref parsed-id (:source v)))
+ (:target v) (assoc ::param-mapping-target
+ (db->norm-param-ref parsed-id (:target v)))
+ :always (-> ; from outer cond->
+ (assoc ::param-mapping-id parsed-id)
+ (dissoc :source :target :id)))])
+ [k (-> v
+ (m/update-existing :source (partial db->norm-param-ref nil))
+ (m/update-existing :target (partial db->norm-param-ref nil))
+ (set/rename-keys db->norm-param-mapping-val-keys))])]
+ (assoc acc new-k new-v))) {} parameter-mapping)))
+
+(defn- norm->db-dimension-param-mapping [k v]
+ (let [str-id (encode-json-string k)]
+ [str-id (cond-> v
+ (::param-mapping-source v) (assoc :source
+ (norm->db-param-ref
+ str-id
+ (::param-mapping-source v)))
+ (::param-mapping-target v) (assoc :target
+ (norm->db-param-ref
+ str-id
+ (::param-mapping-target v)))
+ :always (->
+ (assoc :id str-id)
+ (dissoc ::param-mapping-id
+ ::param-mapping-source
+ ::param-mapping-target)))]))
+
+(defn- norm->db-generic-param-mapping [pm-k pm-v]
+ (let [new-v (into {} (remove (fn [[k v]]
+ ;; don't keep source or target unless not nil
+ (and (nil? v)
+ (contains? #{::param-mapping-source ::param-mapping-target} k)))) pm-v)]
+ [pm-k (-> new-v
+ (m/update-existing ::param-mapping-source (partial norm->db-param-ref nil))
+ (m/update-existing ::param-mapping-target (partial norm->db-param-ref nil))
+ (set/rename-keys norm->db-param-mapping-val-keys))]))
+
+(defn norm->db-param-mapping
+ "Converts a `parameter-mapping` (i.e. value of `::parameter-mapping`) from normalized to DB form."
+ {:added "0.40.0"}
+ [parameter-mapping]
+ (if (nil? parameter-mapping)
+ nil
+ (reduce-kv (fn [acc k v]
+ (let [[new-k new-v]
+ (if (= "dimension" (get-in v [::param-mapping-target ::param-ref-type]))
+ (norm->db-dimension-param-mapping k v)
+ (norm->db-generic-param-mapping k v))]
+ (assoc acc new-k new-v))) {} parameter-mapping)))
+
+(defn- db->norm-click-behavior [v]
+ (-> v
+ (assoc
+ ::click-behavior-type
+ (db->norm-click-action-type (:type v)))
+ (dissoc :type)
+ (assoc ::link-type (db->norm-link-type (:linkType v)))
+ (dissoc :linkType)
+ (cond-> ; from outer ->
+ (some? (:parameterMapping v)) (assoc ::parameter-mapping (db->norm-param-mapping (:parameterMapping v))))
+ (dissoc :parameterMapping)
+ (set/rename-keys db->norm-click-behavior-keys)))
+
+(defn- db->norm-column-settings-entry
+ "Converts a :column_settings DB form to qualified form. Does the opposite of
+ `norm->db-column-settings-entry`."
+ [m k v]
+ (case k
+ :click_behavior (assoc m ::click-behavior (db->norm-click-behavior v))
+ (assoc m (db->norm-column-settings-keys k) v)))
+
+(defn db->norm
+ "Converts a DB form of visualization settings (i.e. map with key `:visualization_settings`) into the equivalent
+ normalized form (i.e. map with keys `::column-settings`, `::click-behavior`, etc.).
+
+ Does the opposite of `norm->db`."
+ {:added "0.40.0"}
+ [vs]
+ (cond-> vs
+ ;; column_settings at top level; ex: table card
+ (:column_settings vs)
+ (assoc ::column-settings (->> (:column_settings vs)
+ (m/map-kv (fn [k v]
+ (let [k1 (parse-db-column-ref k)
+ v1 (reduce-kv db->norm-column-settings-entry {} v)]
+ [k1 v1])))))
+
+ ;; click behavior key at top level; ex: non-table card
+ (:click_behavior vs)
+ (assoc ::click-behavior (db->norm-click-behavior (:click_behavior vs)))
+
+ :always
+ (dissoc :column_settings :click_behavior)))
+
+(defn- norm->db-click-behavior-value [v]
+ (-> v
+ (assoc
+ :type
+ (norm->db-click-action-type (::click-behavior-type v)))
+ (dissoc ::click-behavior-type)
+ (cond-> ; from outer ->
+ (some? (::parameter-mapping v)) (assoc :parameterMapping (norm->db-param-mapping (::parameter-mapping v))))
+ (dissoc ::parameter-mapping)
+ (assoc :linkType (norm->db-link-type (::link-type v)))
+ (dissoc ::link-type)
+ (set/rename-keys norm->db-click-behavior-keys)))
+
+(defn- norm->db-click-behavior [click-behavior]
+ (cond-> click-behavior
+ (some? (::parameter-mapping click-behavior))
+ (-> (assoc :parameterMapping (norm->db-param-mapping (::parameter-mapping click-behavior)))
+ (dissoc ::parameter-mapping))
+
+ :always (-> (assoc :type (norm->db-click-action-type (::click-behavior-type click-behavior)))
+ (m/assoc-some :linkType (norm->db-link-type (::link-type click-behavior)))
+ (dissoc ::link-type ::click-behavior-type ::parameter-mapping)
+ (set/rename-keys norm->db-click-behavior-keys))))
+
+(defn- norm->db-column-settings-entry
+ "Converts a ::column-settings entry from qualified form to DB form. Does the opposite of
+ `db->norm-column-settings-entry`."
+ [m k v]
+ (case k
+ ::click-behavior (assoc m :click_behavior (norm->db-click-behavior v))
+ (assoc m (norm->db-column-settings-keys k) v)))
+
+(defn norm->db-column-ref
+ "Creates the DB form of a column ref (i.e. the key in the column settings map) for the given normalized args. Either
+ `::field-id` or `::field-str` keys will be checked in the arg map to build the corresponding column ref map."
+ {:added "0.40.0"}
+ [{:keys [::field-id ::field-str ::column-name ::field-metadata]}]
+ (-> (cond
+ (some? field-id) ["ref" ["field" field-id field-metadata]]
+ (some? field-str) ["ref" ["field" field-str field-metadata]]
+ (some? column-name) ["name" column-name])
+ encode-json-string))
+
+(defn- norm->db-column-settings
+ "Converts an entire column settings map from normalized to DB form."
+ [col-settings]
+ (->> col-settings
+ (m/map-kv (fn [k v]
+ [(norm->db-column-ref k) (reduce-kv norm->db-column-settings-entry {} v)]))))
+
+(defn norm->db
+ "Converts the normalized form of visualization settings (i.e. a map having
+ `::column-settings` into the equivalent DB form (i.e. a map having `:column_settings`).
+
+ Does The opposite of `db->norm`."
+ {:added "0.40.0"}
+ [settings]
+ (cond-> settings
+ (::column-settings settings) (-> ; from cond->
+ (assoc :column_settings (norm->db-column-settings (::column-settings settings)))
+ (dissoc ::column-settings))
+ (::click-behavior settings) (-> ; from cond->
+ (assoc :click_behavior (norm->db-click-behavior-value (::click-behavior settings)))
+ (dissoc ::click-behavior))))
diff --git a/shared/test/metabase/shared/models/visualization_settings_test.cljc b/shared/test/metabase/shared/models/visualization_settings_test.cljc
new file mode 100644
index 0000000000000000000000000000000000000000..f736c87362684c29f0e5005e36e7e17179ebfe72
--- /dev/null
+++ b/shared/test/metabase/shared/models/visualization_settings_test.cljc
@@ -0,0 +1,181 @@
+(ns metabase.shared.models.visualization-settings-test
+ "Tests for the shared visualization-settings namespace functions"
+ #?@
+ (:clj
+ [(:require [clojure.spec.test.alpha :as stest]
+ [clojure.test :as t]
+ [clojure.walk :as walk]
+ [metabase.shared.models.visualization-settings :as mb.viz])]
+ :cljs
+ [(:require [clojure.spec.test.alpha :as stest]
+ [clojure.test :as t]
+ [clojure.walk :as walk]
+ [goog.string :as gstring]
+ [metabase.shared.models.visualization-settings :as mb.viz])]))
+
+(def ^:private all-instrument-fns [`mb.viz/field-id->column-ref
+ `mb.viz/column-name->column-ref
+ `mb.viz/field-str->column-ref
+ `mb.viz/keyname
+ `mb.viz/parse-json-string
+ `mb.viz/encode-json-string
+ `mb.viz/parse-db-column-ref
+ `mb.viz/with-col-settings
+ `mb.viz/crossfilter-click-action
+ `mb.viz/url-click-action
+ `mb.viz/entity-click-action
+ `mb.viz/with-click-action
+ `mb.viz/with-entity-click-action
+ `mb.viz/fk-parameter-mapping])
+
+(defn with-spec-instrumentation-fixture
+ "`clojure.test` fixture that turns on instrumentation of all specs in the viz settings namespace, then turns it off."
+ [f]
+ (stest/instrument `all-instrument-fns)
+ (f)
+ (stest/unstrument `all-instrument-fns))
+
+(def fmt #?(:clj format :cljs gstring/format))
+
+(t/use-fixtures :once with-spec-instrumentation-fixture)
+
+(t/deftest parse-column-ref-strings-test
+ (t/testing "Column ref strings are parsed correctly"
+ (let [f-qual-nm "/databases/MY_DB/tables/MY_TBL/fields/COL1"
+ f-id 42
+ col-nm "Year"]
+ (doseq [[input-str expected] [[(fmt "[\"ref\",[\"field\",%d,null]]" f-id) {::mb.viz/field-id f-id}]
+ [(fmt "[\"ref\",[\"field\",\"%s\",null]]" f-qual-nm)
+ {::mb.viz/field-str f-qual-nm}]
+ [(fmt "[\"name\",\"Year\"]" col-nm)
+ {::mb.viz/column-name col-nm}]]]
+ (t/is (= expected (mb.viz/parse-db-column-ref input-str)))))))
+
+(t/deftest form-conversion-test
+ (t/testing ":visualization_settings are correctly converted from DB to qualified form and back"
+ (let [f-id 42
+ target-id 19
+ col-name "My Column"
+ db-click-behavior {:type "link"
+ :linkType "question"
+ :parameterMapping {}
+ :targetId target-id}
+ db-click-bhv-map {:click_behavior db-click-behavior}
+ db-col-settings {(fmt "[\"ref\",[\"field\",%d,{\"base-type\":\"type/Integer\"}]]" f-id) db-click-bhv-map
+ (fmt "[\"name\",\"%s\"]" col-name) db-click-bhv-map}
+ db-viz-settings {:column_settings db-col-settings}
+ norm-click-behavior {::mb.viz/click-behavior-type ::mb.viz/link
+ ::mb.viz/link-type ::mb.viz/card
+ ::mb.viz/parameter-mapping {}
+ ::mb.viz/link-target-id target-id}
+ norm-click-bhvr-map {::mb.viz/click-behavior norm-click-behavior}
+ norm-col-settings {(mb.viz/field-id->column-ref f-id {"base-type" "type/Integer"}) norm-click-bhvr-map
+ (mb.viz/column-name->column-ref col-name) norm-click-bhvr-map}
+ norm-viz-settings {::mb.viz/column-settings norm-col-settings}]
+ (doseq [[db-form norm-form] [[db-viz-settings norm-viz-settings]]]
+ (let [to-norm (mb.viz/db->norm db-form)]
+ (t/is (= norm-form to-norm))
+ (let [to-db (mb.viz/norm->db to-norm)]
+ (t/is (= db-form to-db)))))
+ ;; for a non-table card, the :click_behavior map is directly underneath :visualization_settings
+ (t/is (= norm-click-bhvr-map (mb.viz/db->norm db-click-bhv-map))))))
+
+(t/deftest virtual-card-test
+ (t/testing "Virtual card in visualization settings is preserved through normalization roundtrip"
+ ;; virtual cards have the form of a regular card, mostly
+ (let [db-form {:virtual_card {:archived false
+ ;; the name is nil
+ :name nil
+ ;; there is no dataset_query
+ :dataset_query {}
+ ;; display is text
+ :display "text"
+ ;; visualization settings also exist here (being a card), but are unused
+ :visualization_settings {}}
+ ;; this is where the actual text is stored
+ :text "Stuff in Textbox"}]
+ ;; the current viz setting code does not interpret textbox type cards, hence this should be a passthrough
+ (t/is (= db-form (-> db-form
+ mb.viz/db->norm
+ mb.viz/norm->db))))))
+
+(t/deftest parameter-mapping-test
+ (t/testing "parameterMappings are handled correctly"
+ (let [from-id 101
+ to-id 294
+ card-id 19852
+ mapping-id (fmt "[\"dimension\",[\"fk->\",[\"field\",%d,null],[\"field\",%d,null]]]" from-id to-id)
+ norm-id [:dimension [:fk-> [:field from-id nil] [:field to-id nil]]]
+ col-key "[\"name\",\"Some Column\"]"
+ norm-key {::mb.viz/column-name "Some Column"}
+ dimension [:dimension [:field to-id {:source-field from-id}]]
+ param-map {mapping-id {:id mapping-id
+ :source {:type "column"
+ :id "Category_ID"
+ :name "Category ID"}
+ :target {:type "dimension"
+ :id mapping-id
+ :dimension dimension}}}
+ vs-db {:column_settings {col-key {:click_behavior {:linkType "question"
+ :type "link"
+ :linkTextTemplate "Link Text Template"
+ :targetId card-id
+ :parameterMapping param-map}}}}
+ norm-pm {norm-id #::mb.viz{:param-mapping-id norm-id
+ :param-mapping-source #::mb.viz{:param-ref-id "Category_ID"
+ :param-ref-type "column"
+ :param-ref-name "Category ID"}
+ :param-mapping-target #::mb.viz{:param-ref-id norm-id
+ :param-ref-type "dimension"
+ :param-dimension dimension}}}
+ exp-norm {::mb.viz/column-settings {norm-key {::mb.viz/click-behavior
+ #::mb.viz{:click-behavior-type ::mb.viz/link
+ :link-type ::mb.viz/card
+ :link-text-template "Link Text Template"
+ :link-target-id card-id
+ :parameter-mapping norm-pm}}}}
+ vs-norm (mb.viz/db->norm vs-db)]
+ (t/is (= exp-norm vs-norm))
+ (t/is (= vs-db (mb.viz/norm->db vs-norm))))))
+
+(defn- all-keywords [m]
+ (let [all-kws (atom #{})]
+ (walk/postwalk (fn [v] (if (keyword? v) (swap! all-kws #(conj % v)))) m)
+ @all-kws))
+
+(t/deftest comprehensive-click-actions-test
+ (t/testing "Visualization settings for card in 'EE14566 - Click Behavior visualization_settings' should be handled"
+ (let [db-form {:column_settings
+ {"[\"name\",\"Year\"]"
+ {:click_behavior {:type "crossfilter",
+ :parameterMapping {:447496ef {:source {:type "column",
+ :id "Year",
+ :name "Year"},
+ :target {:type "parameter",
+ :id "447496ef"},
+ :id "447496ef"}}}},
+
+ "[\"name\",\"Question: Plain QB - Orders\"]"
+ {:click_behavior {:type "link",
+ :linkType "question",
+ :parameterMapping {},
+ :targetId 1}},
+
+ "[\"name\",\"Dashboard: EE532\"]"
+ {:click_behavior {:type "link",
+ :linkType "dashboard",
+ :parameterMapping {},
+ :targetId 2}},
+
+ "[\"name\",\"Custom Destination\"]"
+ {:click_behavior {:type "link",
+ :linkType "url",
+ :linkTemplate "/dashboard/1?year={{column:Year}}"}}}}
+ norm-form (mb.viz/db->norm db-form)
+ to-db (mb.viz/norm->db norm-form)]
+ ;; make sure all keywords have the right namespace in normalized form (except the one param mapping key)
+ (t/is (= [:447496ef]
+ (filter #(not= (namespace %) (namespace ::mb.viz/column-settings)) (all-keywords norm-form))))
+ ;; make sure all keywords have the right namespace in normalized form
+ (t/is (empty? (filter #(some? (namespace %)) (all-keywords to-db))))
+ (t/is (= db-form to-db)))))
diff --git a/src/metabase/api/embed.clj b/src/metabase/api/embed.clj
index 8340697927b206f7ec3c7c64f31a48f99587e459..0b9899e0e640bdc42863832705a00bcafcb2ef48 100644
--- a/src/metabase/api/embed.clj
+++ b/src/metabase/api/embed.clj
@@ -135,9 +135,10 @@
:when (and tag-type
(or widget-type (not= tag-type :dimension)))]
{:id (:id tag)
- :type (or widget-type (cond (= tag-type :date) :date/single
- (params/field-filter-operators-enabled?) :string/=
- :else :category))
+ :type (or widget-type (cond (= tag-type :date) :date/single
+ (and (params/field-filter-operators-enabled?) (= tag-type :string)) :string/=
+ (and (params/field-filter-operators-enabled?) (= tag-type :number)) :number/=
+ :else :category))
:target (if (= tag-type :dimension)
[:dimension [:template-tag (:name tag)]]
[:variable [:template-tag (:name tag)]])
diff --git a/src/metabase/driver.clj b/src/metabase/driver.clj
index ec9e6ae9caee0e05bb15ba78e4c15217ee3d2966..0e8d5e3ad3cbe1ad52dcd38c58971db4d39166d3 100644
--- a/src/metabase/driver.clj
+++ b/src/metabase/driver.clj
@@ -192,14 +192,14 @@
If you do need to implement this method yourself, you do not need to call parent implementations. We'll take care of
that for you."
{:arglists '([driver])}
- dispatch-on-uninitialized-driver
+ dispatch-on-uninitialized-driver)
;; VERY IMPORTANT: Unlike all other driver multimethods, we DO NOT use the driver hierarchy for dispatch here. Why?
;; We do not want a driver to inherit parent drivers' implementations and have those implementations end up getting
;; called multiple times. If a driver does not implement `initialize!`, *always* fall back to the default no-op
;; implementation.
;;
;; `initialize-if-needed!` takes care to make sure a driver's parent(s) are initialized before initializing a driver.
- )
+
(defmethod initialize! :default [_]) ; no-op
diff --git a/src/metabase/driver/common/parameters/values.clj b/src/metabase/driver/common/parameters/values.clj
index f75e4b565c83741406d02b4379da38ff9f4cb183..19b2c0fcf9f61a78036069801d6cc3e30365d394 100644
--- a/src/metabase/driver/common/parameters/values.clj
+++ b/src/metabase/driver/common/parameters/values.clj
@@ -235,6 +235,13 @@
(number? value) value
;; same goes for an instance of CommaSeperated values
(instance? CommaSeparatedNumbers value) value
+
+ ;; newer operators use vectors as their arguments even if there's only one
+ (vector? value)
+ (let [values (map parse-number value)]
+ (if (next values)
+ (i/map->CommaSeparatedNumbers {:numbers values})
+ (first values)))
;; if the value is a string, then split it by commas in the string. Usually there should be none.
;; Parse each part as a number.
(string? value)
diff --git a/src/metabase/driver/sql/query_processor.clj b/src/metabase/driver/sql/query_processor.clj
index df5035a0ada187414fd0634647406bb943f76454..3158b65d649c6c0fd7c1ac1f5362c8e199840651 100644
--- a/src/metabase/driver/sql/query_processor.clj
+++ b/src/metabase/driver/sql/query_processor.clj
@@ -260,15 +260,17 @@
(defn cast-field-if-needed
"Wrap a `field-identifier` in appropriate HoneySQL expressions if it refers to a UNIX timestamp Field."
[driver field field-identifier]
- (match [(:base_type field) (:coercion_strategy field)]
- [(:isa? :type/Number) (:isa? :Coercion/UNIXTime->Temporal)]
+ (match [(:base_type field) (:coercion_strategy field) (::outer-select field)]
+ [_ _ true] field-identifier ;; casting happens inside the inner query
+
+ [(:isa? :type/Number) (:isa? :Coercion/UNIXTime->Temporal) _]
(unix-timestamp->honeysql driver
(semantic-type->unix-timestamp-unit (:coercion_strategy field))
field-identifier)
- [:type/Text (:isa? :Coercion/String->Temporal) ]
+ [:type/Text (:isa? :Coercion/String->Temporal) _]
(cast-temporal-string driver (:coercion_strategy field) field-identifier)
- [(:isa? :type/*) (:isa? :Coercion/Bytes->Temporal)]
+ [(:isa? :type/*) (:isa? :Coercion/Bytes->Temporal) _]
(cast-temporal-byte driver (:coercion_strategy field) field-identifier)
:else field-identifier))
@@ -382,8 +384,15 @@
(binding [*field-options* options]
(if (:join-alias options)
(compile-field-with-join-aliases driver field-clause)
- (let [honeysql-form (if (integer? field-id-or-name)
+ (let [honeysql-form (cond
+ ;; selects from an inner select should not
+ (and (integer? field-id-or-name) (contains? options ::outer-select))
+ (->honeysql driver (assoc (qp.store/field field-id-or-name) ::outer-select true))
+
+ (integer? field-id-or-name)
(->honeysql driver (qp.store/field field-id-or-name))
+
+ :else
(->honeysql driver (hx/identifier :field *table-alias* field-id-or-name)))]
(cond->> honeysql-form
(:temporal-unit options) (apply-temporal-bucketing driver options)
@@ -968,7 +977,10 @@
distinct)))]
(-> (mbql.u/replace query
[:expression expression-name]
- [:field expression-name {:base-type (:base_type (annotate/infer-expression-type &match))}])
+ [:field expression-name {:base-type (:base_type (annotate/infer-expression-type &match))}]
+ ;; the outer select should not cast as the cast happens in the inner select
+ [:field (field-id :guard int?) field-info]
+ [:field field-id (assoc field-info ::outer-select true)])
(dissoc :source-table :joins :expressions :source-metadata)
(assoc :source-query subselect))))
diff --git a/src/metabase/models/collection.clj b/src/metabase/models/collection.clj
index 7a3425b3ff958080c6448f0a7b80cdd751f28c71..d4eb2e380a181bdf8813bfec35ffdf5ff8321573 100644
--- a/src/metabase/models/collection.clj
+++ b/src/metabase/models/collection.clj
@@ -785,6 +785,18 @@
;; PUTTING IT ALL TOGETHER <3
+(defn- namespace-equals?
+ "Returns true if the :namespace values (for a collection) are equal between multiple instances. Either one can be a
+ string or keyword.
+
+ This is necessary because on select, the :namespace value becomes a keyword (and hence, is a keyword in `pre-update`,
+ but when passing an entity to update, it must be given as a string, not a keyword, because otherwise HoneySQL will
+ attempt to quote it as a column name instead of a string value (and the update statement will fail)."
+ [& namespaces]
+ (let [std-fn (fn [v]
+ (if (keyword? v) (name v) (str v)))]
+ (apply = (map std-fn namespaces))))
+
(defn- pre-update [{collection-name :name, id :id, color :color, :as collection-updates}]
(let [collection-before-updates (Collection id)]
;; VARIOUS CHECKS BEFORE DOING ANYTHING:
@@ -795,7 +807,7 @@
(assert-valid-location collection-updates)
;; (3) make sure Collection namespace is valid
(when (contains? collection-updates :namespace)
- (when (not= (:namespace collection-before-updates) (:namespace collection-updates))
+ (when-not (namespace-equals? (:namespace collection-before-updates) (:namespace collection-updates))
(let [msg (tru "You cannot move a Collection to a different namespace once it has been created.")]
(throw (ex-info msg {:status-code 400, :errors {:namespace msg}})))))
(assert-valid-namespace (merge (select-keys collection-before-updates [:namespace]) collection-updates))
diff --git a/src/metabase/models/permissions.clj b/src/metabase/models/permissions.clj
index d8abeb21f0681dcbd69a18a0aa08be7b15a52f3c..04ecbc8a3fafab7ba2df8b5d5c0a060448e741ba 100644
--- a/src/metabase/models/permissions.clj
+++ b/src/metabase/models/permissions.clj
@@ -9,6 +9,7 @@
[metabase.models.interface :as i]
[metabase.models.permissions-group :as group]
[metabase.models.permissions-revision :as perms-revision :refer [PermissionsRevision]]
+ [metabase.models.permissions.delete-sandboxes :as delete-sandboxes]
[metabase.models.permissions.parse :as perms-parse]
[metabase.plugins.classloader :as classloader]
[metabase.util :as u]
@@ -714,7 +715,8 @@
(db/transaction
(doseq [[group-id changes] new]
(update-group-permissions! group-id changes))
- (save-perms-revision! (:revision old-graph) old new)))))
+ (save-perms-revision! (:revision old-graph) old new)
+ (delete-sandboxes/delete-gtaps-if-needed-after-permissions-change! new)))))
;; The following arity is provided soley for convenience for tests/REPL usage
([ks :- [s/Any], new-value]
diff --git a/src/metabase/models/permissions/delete_sandboxes.clj b/src/metabase/models/permissions/delete_sandboxes.clj
new file mode 100644
index 0000000000000000000000000000000000000000..67d0abe7614652ebf1a7baa413e24e9a3ea4f53b
--- /dev/null
+++ b/src/metabase/models/permissions/delete_sandboxes.clj
@@ -0,0 +1,37 @@
+(ns metabase.models.permissions.delete-sandboxes
+ (:require [metabase.plugins.classloader :as classloader]
+ [metabase.util :as u]
+ [potemkin :as p]
+ [pretty.core :as pretty]))
+
+(p/defprotocol+ DeleteSandboxes
+ "Protocol for Sandbox deletion behavior when permissions are granted or revoked."
+ (delete-gtaps-if-needed-after-permissions-change!* [this changes]
+ "For use only inside `metabase.models.permissions`; don't call this elsewhere. Delete GTAPs that are no longer
+needed after the permissions graph is updated. See docstring for `delete-gtaps-if-needed-after-permissions-change!` for
+ more information."))
+
+(def oss-default-impl
+ "OSS no-op impl for Sandbox (GTAP) deletion behavior. Don't use this directly."
+ (reify
+ DeleteSandboxes
+ (delete-gtaps-if-needed-after-permissions-change!* [_ _] nil)
+
+ pretty/PrettyPrintable
+ (pretty [_]
+ `oss-default-impl)))
+
+(def ^:private impl
+ (delay
+ (or (u/ignore-exceptions
+ (classloader/require 'metabase-enterprise.sandbox.models.permissions.delete-sandboxes)
+ (var-get (resolve 'metabase-enterprise.sandbox.models.permissions.delete-sandboxes/ee-strategy-impl)))
+ oss-default-impl)))
+
+(defn delete-gtaps-if-needed-after-permissions-change!
+ "For use only inside `metabase.models.permissions`; don't call this elsewhere. Delete GTAPs (sandboxes) that are no
+ longer needed after the permissions graph is updated. This is EE-specific -- OSS impl is a no-op, since sandboxes
+ are an EE-only feature. `changes` are the parts of the graph that have changed, i.e. the `things-only-in-new`
+ returned by `clojure.data/diff`."
+ [changes]
+ (delete-gtaps-if-needed-after-permissions-change!* @impl changes))
diff --git a/src/metabase/public_settings.clj b/src/metabase/public_settings.clj
index b193cdc217ab26d4112846f99b72bfba76a930bb..c960606c5b6d06ae05b9970fcf46f719eb04093c 100644
--- a/src/metabase/public_settings.clj
+++ b/src/metabase/public_settings.clj
@@ -343,6 +343,12 @@
:setter :none
:getter password/active-password-complexity)
+(defsetting session-cookies
+ (deferred-tru "When set, enforces the use of session cookies for all users which expire when the browser is closed.")
+ :type :boolean
+ :visibility :public
+ :default nil)
+
(defsetting report-timezone-short
"Current report timezone abbreviation"
:visibility :public
diff --git a/src/metabase/query_processor/middleware/expand_macros.clj b/src/metabase/query_processor/middleware/expand_macros.clj
index 9eb5e3e5dd1e81f02a38cf920359f9f0bc5f21d1..caedcaf292d8960f1344940d059388523bf39335 100644
--- a/src/metabase/query_processor/middleware/expand_macros.clj
+++ b/src/metabase/query_processor/middleware/expand_macros.clj
@@ -67,11 +67,12 @@
:when (not errors)]
metric))))
-(defn- add-metrics-filters [query metric-id->info]
- (let [filters (for [{{filter-clause :filter} :definition} (vals metric-id->info)
+(s/defn ^:private add-metrics-filters-this-level :- mbql.s/MBQLQuery
+ [inner-query :- mbql.s/MBQLQuery this-level-metric-id->info :- {su/IntGreaterThanZero MetricInfo}]
+ (let [filters (for [{{filter-clause :filter} :definition} (vals this-level-metric-id->info)
:when filter-clause]
filter-clause)]
- (reduce mbql.u/add-filter-clause query filters)))
+ (reduce mbql.u/add-filter-clause-to-inner-query inner-query filters)))
(s/defn ^:private metric-info->ag-clause :- mbql.s/Aggregation
"Return an appropriate aggregation clause from `metric-info`."
@@ -91,12 +92,15 @@
_
[:aggregation-options &match {:display-name metric-name}])))
-(defn- replace-metrics-aggregations [query metric-id->info]
- (let [metric (fn [metric-id]
- (or (get metric-id->info metric-id)
- (throw (ex-info (tru "Metric {0} does not exist, or is invalid." metric-id)
- {:type :invalid-query, :metric metric-id}))))]
- (mbql.u/replace-in query [:query]
+(s/defn ^:private replace-metrics-aggregations-this-level :- mbql.s/MBQLQuery
+ [inner-query :- mbql.s/MBQLQuery this-level-metric-id->info :- {su/IntGreaterThanZero MetricInfo}]
+ (letfn [(metric [metric-id]
+ (or (get this-level-metric-id->info metric-id)
+ (throw (ex-info (tru "Metric {0} does not exist, or is invalid." metric-id)
+ {:type :invalid-query
+ :metric metric-id
+ :query inner-query}))))]
+ (mbql.u/replace-in inner-query [:aggregation]
;; if metric is wrapped in aggregation options that give it a display name, expand the metric but do not name it
[:aggregation-options [:metric (metric-id :guard (complement mbql.u/ga-id?))] (options :guard :display-name)]
[:aggregation-options
@@ -113,20 +117,45 @@
[:metric (metric-id :guard (complement mbql.u/ga-id?))]
(metric-info->ag-clause (metric metric-id) {:use-metric-name-as-display-name? true}))))
-(defn- add-metrics-clauses
+(s/defn ^:private metric-ids-this-level :- (s/maybe #{su/IntGreaterThanZero})
+ [inner-query]
+ (when (map? inner-query)
+ (when-let [aggregations (:aggregation inner-query)]
+ (not-empty
+ (set
+ (mbql.u/match aggregations
+ [:metric (metric-id :guard (complement mbql.u/ga-id?))]
+ metric-id))))))
+
+(s/defn ^:private expand-metrics-clauses-this-level :- (s/constrained
+ mbql.s/MBQLQuery
+ (complement metric-ids-this-level)
+ "Inner MBQL query with no :metric clauses at this level")
+ [inner-query :- mbql.s/MBQLQuery metric-id->info :- {su/IntGreaterThanZero MetricInfo}]
+ (let [this-level-metric-ids (metric-ids-this-level inner-query)
+ this-level-metric-id->info (select-keys metric-id->info this-level-metric-ids)]
+ (-> inner-query
+ (add-metrics-filters-this-level this-level-metric-id->info)
+ (replace-metrics-aggregations-this-level this-level-metric-id->info))))
+
+(s/defn ^:private expand-metrics-clauses :- su/Map
"Add appropriate `filter` and `aggregation` clauses for a sequence of Metrics.
- (add-metrics-clauses {:query {}} [[:metric 10]])
+ (expand-metrics-clauses {:query {}} [[:metric 10]])
;; -> {:query {:aggregation [[:count]], :filter [:= [:field-id 10] 20]}}"
- [query metric-id->info]
- (-> query
- (add-metrics-filters metric-id->info)
- (replace-metrics-aggregations metric-id->info)))
+ [query :- su/Map metric-id->info :- (su/non-empty {su/IntGreaterThanZero MetricInfo})]
+ (mbql.u/replace query
+ (m :guard metric-ids-this-level)
+ (-> m
+ ;; expand this this level...
+ (expand-metrics-clauses-this-level metric-id->info)
+ ;; then recursively expand things at any other levels.
+ (expand-metrics-clauses metric-id->info))))
(s/defn ^:private expand-metrics :- mbql.s/Query
[query :- mbql.s/Query]
(if-let [metrics (metrics query)]
- (add-metrics-clauses query (metric-clauses->id->info metrics))
+ (expand-metrics-clauses query (metric-clauses->id->info metrics))
query))
diff --git a/src/metabase/server/middleware/session.clj b/src/metabase/server/middleware/session.clj
index 09c99c2919959171c2139ce62caed8377fa2a279..ec156bd78f211fb07fb814060f754f3b49fbf59d 100644
--- a/src/metabase/server/middleware/session.clj
+++ b/src/metabase/server/middleware/session.clj
@@ -10,6 +10,7 @@
[metabase.driver.sql.query-processor :as sql.qp]
[metabase.models.session :refer [Session]]
[metabase.models.user :as user :refer [User]]
+ [metabase.public-settings :as public-settings]
[metabase.server.request.util :as request.u]
[metabase.util :as u]
[metabase.util.i18n :as i18n :refer [deferred-trs tru]]
@@ -51,6 +52,15 @@
[response]
(reduce clear-cookie (wrap-body-if-needed response) [metabase-session-cookie metabase-embedded-session-cookie]))
+(defn- use-permanent-cookies?
+ "Check if we should use permanent cookies for a given request, which are not cleared when a browser sesion ends."
+ [request]
+ (if (public-settings/session-cookies)
+ ;; Disallow permanent cookies if MB_SESSION_COOKIES is set
+ false
+ ;; Otherwise check whether the user selected "remember me" during login
+ (get-in request [:body :remember])))
+
(defmulti set-session-cookie
"Add an appropriate cookie to persist a newly created Session to `response`."
{:arglists '([request response session])}
@@ -70,12 +80,11 @@
;; TODO - we should set `site-path` as well. Don't want to enable this yet so we don't end
;; up breaking things
:path "/" #_ (site-path)}
- ;; If the env var `MB_SESSION_COOKIES=true`, do not set the `Max-Age` directive; cookies
- ;; with no `Max-Age` and no `Expires` directives are session cookies, and are deleted when
- ;; the browser is closed
- ;;
- ;; See https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#Session_cookies
- (when-not (config/config-bool :mb-session-cookies)
+ ;; If permanent cookies should be used, set the `Max-Age` directive; cookies with no
+ ;; `Max-Age` and no `Expires` directives are session cookies, and are deleted when the
+ ;; browser is closed.
+ ;; See https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#define_the_lifetime_of_a_cookie
+ (when (use-permanent-cookies? request)
;; max-session age-is in minutes; Max-Age= directive should be in seconds
{:max-age (* 60 (config/config-int :max-session-age))})
;; If the authentication request request was made over HTTPS (hopefully always except for
diff --git a/test/metabase/api/session_test.clj b/test/metabase/api/session_test.clj
index 2eae6f32018120b5c228ebc764be49673d1fe0e3..3d6760b1fa49de953de07b796762d2e9ec4ba6bf 100644
--- a/test/metabase/api/session_test.clj
+++ b/test/metabase/api/session_test.clj
@@ -12,6 +12,7 @@
[metabase.models.setting :as setting]
[metabase.models.user :refer [User]]
[metabase.public-settings :as public-settings]
+ [metabase.server.middleware.session :as mw.session]
[metabase.test :as mt]
[metabase.test.data.users :as test-users]
[metabase.test.fixtures :as fixtures]
@@ -43,6 +44,8 @@
(def ^:private SessionResponse
{:id (s/pred mt/is-uuid-string? "session")})
+(def ^:private session-cookie @#'mw.session/metabase-session-cookie)
+
(deftest login-test
(testing "POST /api/session"
(testing "Test that we can login"
@@ -63,6 +66,14 @@
:active (s/eq true)
s/Keyword s/Any}
(db/select-one LoginHistory :user_id (mt/user->id :rasta), :session_id (:id response)))))))
+ (testing "Test that 'remember me' checkbox sets Max-Age attribute on session cookie"
+ (let [body (assoc (mt/user->credentials :rasta) :remember true)
+ response (mt/client-full-response :post 200 "session" body)]
+ ;; clj-http sets :expires key in response when Max-Age attribute is set
+ (is (get-in response [:cookies session-cookie :expires])))
+ (let [body (assoc (mt/user->credentials :rasta) :remember false)
+ response (mt/client-full-response :post 200 "session" body)]
+ (is (nil? (get-in response [:cookies session-cookie :expires]))))))
(testing "failure should log an error(#14317)"
(mt/with-temp User [user]
(is (schema= [(s/one (s/eq :error)
@@ -72,7 +83,7 @@
(s/one (s/eq "Authentication endpoint error")
"log message")]
(first (mt/with-log-messages-for-level :error
- (mt/client :post 400 "session" {:email (:email user), :password "wooo"})))))))))
+ (mt/client :post 400 "session" {:email (:email user), :password "wooo"}))))))))
(deftest login-validation-test
(testing "POST /api/session"
diff --git a/test/metabase/driver/common/parameters/values_test.clj b/test/metabase/driver/common/parameters/values_test.clj
index e727163e37395a63a44e4d552536c5cde32763cf..e2a2fe654fcde9aafac959f5f4f05fbca9a7d7a4 100644
--- a/test/metabase/driver/common/parameters/values_test.clj
+++ b/test/metabase/driver/common/parameters/values_test.clj
@@ -4,7 +4,6 @@
[metabase.driver.common.parameters :as i]
[metabase.driver.common.parameters.values :as values]
[metabase.models :refer [Card Collection NativeQuerySnippet]]
- [metabase.models.field :refer [map->FieldInstance]]
[metabase.models.permissions :as perms]
[metabase.models.permissions-group :as group]
[metabase.query-processor :as qp]
@@ -19,6 +18,15 @@
(#'values/value-for-tag
{:name "id", :display-name "ID", :type :text, :required true, :default "100"}
[{:type :category, :target [:variable [:template-tag "id"]], :value "2"}]))))
+ (testing "Multiple values with new operators"
+ (is (= 20
+ (#'values/value-for-tag
+ {:name "number_filter", :display-name "ID", :type :number, :required true, :default "100"}
+ [{:type :number/=, :value ["20"], :target [:variable [:template-tag "number_filter"]]}])))
+ (is (= (i/map->CommaSeparatedNumbers {:numbers [20 40]})
+ (#'values/value-for-tag
+ {:name "number_filter", :display-name "ID", :type :number, :required true, :default "100"}
+ [{:type :number/=, :value ["20" "40"], :target [:variable [:template-tag "number_filter"]]}]))))
(testing "Unspecified value"
(is (= i/no-value
diff --git a/test/metabase/driver/sql/query_processor_test.clj b/test/metabase/driver/sql/query_processor_test.clj
index 1fa7c48c4ef3536219ef1b8128e17d7c19744e62..4d022be84ceec4381f84e2363c3a54121c24f12e 100644
--- a/test/metabase/driver/sql/query_processor_test.clj
+++ b/test/metabase/driver/sql/query_processor_test.clj
@@ -3,12 +3,16 @@
[clojure.test :refer :all]
[honeysql.core :as hsql]
[metabase.driver :as driver]
+ [metabase.driver.sql-jdbc.test-util :as sql-jdbc.tu]
[metabase.driver.sql.query-processor :as sql.qp]
+ [metabase.models.field :refer [Field]]
[metabase.models.setting :as setting]
[metabase.query-processor :as qp]
[metabase.test :as mt]
[metabase.util.honeysql-extensions :as hx]
- [pretty.core :refer [PrettyPrintable]])
+ [pretty.core :refer [PrettyPrintable]]
+ [schema.core :as s]
+ [toucan.db :as db])
(:import metabase.util.honeysql_extensions.Identifier))
(deftest process-mbql-query-keys-test
@@ -465,3 +469,19 @@
:alias "CategoriesStats"
:fields :all}]
:limit 3}))))))
+
+(deftest expressions-and-coercions-test
+ (mt/test-drivers (conj (sql-jdbc.tu/sql-jdbc-drivers) :bigquery)
+ (testing "Don't cast in both inner select and outer select when expression (#12430)"
+ (let [price-field-id (mt/id :venues :price)]
+ (mt/with-temp-vals-in-db Field price-field-id {:coercion_strategy :Coercion/UNIXSeconds->DateTime
+ :effective_type :type/DateTime}
+ (let [results (qp/process-query {:database (mt/id)
+ :query {:source-table (mt/id :venues)
+ :expressions {:test ["*" 1 1]}
+ :fields [[:field price-field-id nil]
+ [:expression "test"]]}
+ :type "query"})]
+ (is (schema= [(s/one s/Str "date")
+ (s/one s/Num "expression")]
+ (-> results mt/rows first)))))))))
diff --git a/test/metabase/query_processor/middleware/expand_macros_test.clj b/test/metabase/query_processor/middleware/expand_macros_test.clj
index 026236cb800a9c97b314ec93462163dd925d584e..cf1d152f16453db08a825afa7322df906dcd81f1 100644
--- a/test/metabase/query_processor/middleware/expand_macros_test.clj
+++ b/test/metabase/query_processor/middleware/expand_macros_test.clj
@@ -245,3 +245,72 @@
[:or
[:segment segment-2-id]
[:> [:field 4 nil] 1]]]]]})))))))
+
+(defn- expand-macros [query]
+ (:pre (mt/test-qp-middleware expand-macros/expand-macros query)))
+
+(deftest expand-macros-in-nested-queries-test
+ (testing "expand-macros should expand things in the correct nested level (#12507)"
+ (mt/with-temp* [Metric [metric (mt/$ids checkins
+ {:table_id $$checkins
+ :definition {:source-table $$checkins
+ :aggregation [[:count]]
+ :filter [:not-null $id]}})]
+ Segment [segment (mt/$ids checkins
+ {:table_id $$checkins
+ :definition {:filter [:not-null $id]}})]]
+ (doseq [[macro-type {:keys [before after]}]
+ (mt/$ids checkins
+ {"Metrics"
+ {:before {:source-table $$checkins
+ :aggregation [[:metric (u/the-id metric)]]}
+ :after {:source-table $$checkins
+ :aggregation [[:aggregation-options [:count] {:display-name "Toucans in the rainforest"}]]
+ :filter [:not-null $id]}}
+
+ "Segments"
+ {:before {:source-table $$checkins
+ :filter [:segment (u/the-id segment)]}
+ :after {:source-table $$checkins
+ :filter [:not-null $id]}}})]
+ (testing macro-type
+ (testing "nested 1 level"
+ (is (= (mt/mbql-query nil {:source-query after})
+ (expand-macros
+ (mt/mbql-query nil {:source-query before})))))
+ (testing "nested 2 levels"
+ (is (= (mt/mbql-query nil {:source-query {:source-query after}})
+ (expand-macros
+ (mt/mbql-query nil {:source-query {:source-query before}})))))
+ (testing "nested 3 levels"
+ (is (= (mt/mbql-query nil {:source-query {:source-query {:source-query after}}})
+ (expand-macros
+ (mt/mbql-query nil {:source-query {:source-query {:source-query before}}})))))
+ (testing "nested at different levels"
+ (is (= (mt/mbql-query nil {:source-query (-> after
+ (dissoc :source-table)
+ (assoc :source-query after))})
+ (expand-macros
+ (mt/mbql-query nil {:source-query (-> before
+ (dissoc :source-table)
+ (assoc :source-query before))})))))
+ (testing "inside :source-query inside :joins"
+ (is (= (mt/mbql-query checkins {:joins [{:condition [:= 1 2]
+ :source-query after}]})
+ (expand-macros
+ (mt/mbql-query checkins {:joins [{:condition [:= 1 2]
+ :source-query before}]})))))
+ (when (= macro-type "Segments")
+ (testing "inside join condition"
+ (is (= (mt/mbql-query checkins {:joins [{:source-table $$checkins
+ :condition (:filter after)}]})
+ (expand-macros
+ (mt/mbql-query checkins {:joins [{:source-table $$checkins
+ :condition (:filter before)}]}))))))
+ (testing "inside :joins inside :source-query"
+ (is (= (mt/mbql-query nil {:source-query {:source-table $$checkins
+ :joins [{:condition [:= 1 2]
+ :source-query after}]}})
+ (expand-macros (mt/mbql-query nil {:source-query {:source-table $$checkins
+ :joins [{:condition [:= 1 2]
+ :source-query before}]}}))))))))))
diff --git a/test/metabase/query_processor_test/nested_queries_test.clj b/test/metabase/query_processor_test/nested_queries_test.clj
index 05e2eceb0d35fca84d7b7576b4bdf456e5b93938..d95636bb1bd9fa47c54c451c18389a4609b46ffa 100644
--- a/test/metabase/query_processor_test/nested_queries_test.clj
+++ b/test/metabase/query_processor_test/nested_queries_test.clj
@@ -5,7 +5,7 @@
[java-time :as t]
[metabase.driver :as driver]
[metabase.mbql.schema :as mbql.s]
- [metabase.models :refer [Dimension Field Segment Table]]
+ [metabase.models :refer [Dimension Field Metric Segment Table]]
[metabase.models.card :as card :refer [Card]]
[metabase.models.collection :as collection :refer [Collection]]
[metabase.models.interface :as models]
@@ -479,16 +479,17 @@
(deftest macroexpansion-test
(testing "Make sure that macro expansion works inside of a neested query, when using a compound filter clause (#5974)"
- (mt/with-temp* [Segment [segment (mt/$ids {:table_id $$venues
+ (mt/test-drivers (mt/normal-drivers-with-feature :nested-queries)
+ (mt/with-temp* [Segment [segment (mt/$ids {:table_id $$venues
:definition {:filter [:= $venues.price 1]}})]
- Card [card (mbql-card-def
- :source-table (mt/id :venues)
- :filter [:and [:segment (u/the-id segment)]])]]
- (is (= [[22]]
- (mt/rows
- (qp/process-query
- (query-with-source-card card
- {:aggregation [:count]}))))))))
+ Card [card (mbql-card-def
+ :source-table (mt/id :venues)
+ :filter [:and [:segment (u/the-id segment)]])]]
+ (is (= [[22]]
+ (mt/formatted-rows [int]
+ (qp/process-query
+ (query-with-source-card card
+ {:aggregation [:count]})))))))))
(deftest card-perms-test
(testing "perms for a Card with a SQL source query\n"
@@ -1162,3 +1163,19 @@
(qp/query->native q2))))
(is (= [[543]]
(mt/formatted-rows [int] (qp/process-query q2)))))))))))
+
+(deftest nested-query-with-metric-test
+ (mt/test-drivers (mt/normal-drivers-with-feature :nested-queries)
+ (testing "A nested query with a Metric should work as expected (#12507)"
+ (mt/with-temp Metric [metric (mt/$ids checkins
+ {:table_id $$checkins
+ :definition {:source-table $$checkins
+ :aggregation [[:count]]
+ :filter [:not-null $id]}})]
+ (is (= [[100]]
+ (mt/formatted-rows [int]
+ (mt/run-mbql-query checkins
+ {:source-query {:source-table $$checkins
+ :aggregation [[:metric (u/the-id metric)]]
+ :breakout [$venue_id]}
+ :aggregation [[:count]]}))))))))
diff --git a/test/metabase/server/middleware/session_test.clj b/test/metabase/server/middleware/session_test.clj
index 552cf0ef30d4d81b99d31c2ff3c6cc189cd79cc3..f8b041c66e196e8b710e66f71359c40fa4cc3b6b 100644
--- a/test/metabase/server/middleware/session_test.clj
+++ b/test/metabase/server/middleware/session_test.clj
@@ -8,6 +8,7 @@
[metabase.db :as mdb]
[metabase.driver.sql.query-processor :as sql.qp]
[metabase.models :refer [Session User]]
+ [metabase.public-settings :as public-settings]
[metabase.server.middleware.session :as mw.session]
[metabase.test :as mt]
[metabase.util.i18n :as i18n]
@@ -49,18 +50,25 @@
{:value (str uuid)
:same-site :lax
:http-only true
- :path "/"
- :max-age 1209600}}
+ :path "/"}}
(-> (mw.session/set-session-cookie {} {} {:id uuid, :type :normal})
:cookies))))
- (testing "if `MB_SESSION_COOKIES=true` we shouldn't set a `Max-Age`"
+ (testing "should set `Max-Age` if `remember` is true in request"
+ (is (= {:value (str uuid)
+ :same-site :lax
+ :http-only true
+ :path "/"
+ :max-age 1209600}
+ (-> (mw.session/set-session-cookie {:body {:remember true}} {} {:id uuid, :type :normal})
+ (get-in [:cookies "metabase.SESSION"])))))
+ (testing "if `MB_SESSION_COOKIES=true` we shouldn't set a `Max-Age`, even if `remember` is true"
(is (= {:value (str uuid)
:same-site :lax
:http-only true
:path "/"}
(let [env env/env]
- (with-redefs [env/env (assoc env :mb-session-cookies "true")]
- (-> (mw.session/set-session-cookie {} {} {:id uuid, :type :normal})
+ (mt/with-temporary-setting-values [session-cookies true]
+ (-> (mw.session/set-session-cookie {:body {:remember true}} {} {:id uuid, :type :normal})
(get-in [:cookies "metabase.SESSION"])))))))))
;; if request is an HTTPS request then we should set `:secure true`. There are several different headers we check for