From c318f1586ccb1b532c921225526b77af8faf8384 Mon Sep 17 00:00:00 2001
From: Allen Gilliland <agilliland@gmail.com>
Date: Tue, 15 Mar 2016 15:56:17 -0700
Subject: [PATCH] start using :visibility_type instead of :field-type
"sensitive" or :active true/false
---
src/metabase/api/table.clj | 12 +++--
src/metabase/driver/query_processor.clj | 9 ++--
src/metabase/models/field.clj | 3 +-
src/metabase/models/field_values.clj | 6 +--
src/metabase/models/table.clj | 7 +--
test/metabase/api/table_test.clj | 4 +-
test/metabase/models/field_test.clj | 47 +++++++++++--------
test/metabase/models/field_values_test.clj | 18 +++++--
test/metabase/test/data.clj | 5 +-
.../data/dataset_definitions/test-data.edn | 2 +-
test/metabase/test/data/interface.clj | 5 +-
11 files changed, 71 insertions(+), 47 deletions(-)
diff --git a/src/metabase/api/table.clj b/src/metabase/api/table.clj
index 97147a4deec..3f442966a67 100644
--- a/src/metabase/api/table.clj
+++ b/src/metabase/api/table.clj
@@ -55,8 +55,10 @@
(defendpoint GET "/:id/fields"
"Get all `Fields` for `Table` with ID."
[id]
- (read-check Table id)
- (sel :many Field :table_id id, :active true, :field_type [not= "sensitive"], (k/order :name :ASC)))
+ ;; TODO: check that order doesn't change in a problematic way due to sorting by position
+ (let-404 [table (Table id)]
+ (read-check table)
+ (sel :many Field :table_id id, :visibility_type [not-in ["sensitive" "retired"]], (k/order :name :ASC))))
(defendpoint GET "/:id/query_metadata"
"Get metadata about a `Table` useful for running queries.
@@ -73,14 +75,14 @@
;; If someone passes include_sensitive_fields return hydrated :fields as-is
identity
;; Otherwise filter out all :sensitive fields
- (partial filter (fn [{:keys [field_type]}]
- (not= (keyword field_type) :sensitive)))))))
+ (partial filter (fn [{:keys [visibility_type]}]
+ (not= (keyword visibility_type) :sensitive)))))))
(defendpoint GET "/:id/fks"
"Get all `ForeignKeys` whose destination is a `Field` that belongs to this `Table`."
[id]
(read-check Table id)
- (let-404 [field-ids (sel :many :id Field :table_id id :active true)]
+ (let-404 [field-ids (sel :many :id Field :table_id id :visibility_type [not= "retired"])]
(-> (sel :many ForeignKey :destination_id [in field-ids])
;; TODO - it's a little silly to hydrate both of these table objects
(hydrate [:origin [:table :db]] [:destination :table]))))
diff --git a/src/metabase/driver/query_processor.clj b/src/metabase/driver/query_processor.clj
index ea98aecdee1..8dc39c1c768 100644
--- a/src/metabase/driver/query_processor.clj
+++ b/src/metabase/driver/query_processor.clj
@@ -172,11 +172,10 @@
(qp (if-not (should-add-implicit-fields? query)
query
(let [fields (for [field (db/sel :many :fields [Field :name :display_name :base_type :special_type :preview_display :display_name :table_id :id :position :description]
- :table_id source-table-id
- :active true
- :field_type [not= "sensitive"]
- :parent_id nil
- (k/order :position :asc) (k/order :id :desc))]
+ :table_id source-table-id
+ :visibility_type [not-in ["sensitive" "retired"]]
+ :parent_id nil
+ (k/order :position :asc) (k/order :id :desc))]
(let [field (-> (resolve/rename-mb-field-keys field)
map->Field
(resolve/resolve-table {source-table-id source-table}))]
diff --git a/src/metabase/models/field.clj b/src/metabase/models/field.clj
index 925097dc25f..2d536d7fbe0 100644
--- a/src/metabase/models/field.clj
+++ b/src/metabase/models/field.clj
@@ -48,8 +48,7 @@
"Possible values for `Field.field_type`."
#{:metric ; A number that can be added, graphed, etc.
:dimension ; A high or low-cardinality numerical string value that is meant to be used as a grouping
- :info ; Non-numerical value that is not meant to be used
- :sensitive}) ; A Fields that should *never* be shown *anywhere*
+ :info}) ; Non-numerical value that is not meant to be used
(def ^:const visibility-types
"Possible values for `Field.visibility_type`."
diff --git a/src/metabase/models/field_values.clj b/src/metabase/models/field_values.clj
index ec141621ded..561ec64b37b 100644
--- a/src/metabase/models/field_values.clj
+++ b/src/metabase/models/field_values.clj
@@ -28,11 +28,11 @@
(defn field-should-have-field-values?
"Should this `Field` be backed by a corresponding `FieldValues` object?"
{:arglists '([field])}
- [{:keys [base_type special_type field_type] :as field}]
- {:pre [field_type
+ [{:keys [base_type special_type visibility_type] :as field}]
+ {:pre [visibility_type
(contains? field :base_type)
(contains? field :special_type)]}
- (and (not= (keyword field_type) :sensitive)
+ (and (not (contains? #{:retired :sensitive :hidden :details-only} (keyword visibility_type)))
(not (contains? #{:DateField :DateTimeField :TimeField} (keyword base_type)))
(or (contains? #{:category :city :state :country :name} (keyword special_type))
(= (keyword base_type) :BooleanField))))
diff --git a/src/metabase/models/table.clj b/src/metabase/models/table.clj
index 461b3e7ddaa..bf6cf86ebf4 100644
--- a/src/metabase/models/table.clj
+++ b/src/metabase/models/table.clj
@@ -31,7 +31,7 @@
(defn ^:hydrate fields
"Return the `FIELDS` belonging to TABLE."
[{:keys [id]}]
- (db/sel :many Field :table_id id, :active true, (k/order :position :ASC) (k/order :name :ASC)))
+ (db/sel :many Field :table_id id, :visibility_type [not= "retired"], (k/order :position :ASC) (k/order :name :ASC)))
(defn ^:hydrate metrics
"Retrieve the metrics for TABLE."
@@ -47,7 +47,8 @@
"Return the `FieldValues` for all `Fields` belonging to TABLE."
{:hydrate :field_values, :arglists '([table])}
[{:keys [id]}]
- (let [field-ids (db/sel :many :id Field, :table_id id, :active true, :field_type [not= "sensitive"]
+ ;; TODO: any reason to return field-values for other visibility options?
+ (let [field-ids (db/sel :many :id Field, :table_id id, :visibility_type "normal"
(k/order :position :asc)
(k/order :name :asc))]
(db/sel :many :field->field [FieldValues :field_id :values] :field_id [in field-ids])))
@@ -56,7 +57,7 @@
"Return the ID of the primary key `Field` for TABLE."
{:hydrate :pk_field, :arglists '([table])}
[{:keys [id]}]
- (db/sel :one :id Field, :table_id id, :special_type "id"))
+ (db/sel :one :id Field, :table_id id, :special_type "id", :visibility_type [not-in ["sensitive" "retired"]]))
(def ^{:arglists '([table])} database
"Return the `Database` associated with this `Table`."
diff --git a/test/metabase/api/table_test.clj b/test/metabase/api/table_test.clj
index 28f558a6bd1..5d92344729c 100644
--- a/test/metabase/api/table_test.clj
+++ b/test/metabase/api/table_test.clj
@@ -273,13 +273,13 @@
:updated_at $
:active true
:id $
- :field_type "sensitive"
+ :field_type "info"
:position 0
:target nil
:preview_display true
:created_at $
:base_type "TextField"
- :visibility_type "normal"
+ :visibility_type "sensitive"
:parent_id nil})]
:rows 15
:updated_at $
diff --git a/test/metabase/models/field_test.clj b/test/metabase/models/field_test.clj
index 44b08be542b..dcbcb1f7493 100644
--- a/test/metabase/models/field_test.clj
+++ b/test/metabase/models/field_test.clj
@@ -22,55 +22,64 @@
;; field-should-have-field-values?
-;; sensitive fields should always be excluded
-(expect false (field-should-have-field-values? {:base_type :BooleanField
- :special_type :category
- :field_type :sensitive}))
+;; retired/sensitive/hidden/details-only fields should always be excluded
+(expect false (field-should-have-field-values? {:base_type :BooleanField
+ :special_type :category
+ :visibility_type :retired}))
+(expect false (field-should-have-field-values? {:base_type :BooleanField
+ :special_type :category
+ :visibility_type :sensitive}))
+(expect false (field-should-have-field-values? {:base_type :BooleanField
+ :special_type :category
+ :visibility_type :hidden}))
+(expect false (field-should-have-field-values? {:base_type :BooleanField
+ :special_type :category
+ :visibility_type :details-only}))
;; date/time based fields should always be excluded
(expect false (field-should-have-field-values? {:base_type :DateField
:special_type :category
- :field_type :dimension}))
+ :visibility_type :normal}))
(expect false (field-should-have-field-values? {:base_type :DateTimeField
:special_type :category
- :field_type :dimension}))
+ :visibility_type :normal}))
(expect false (field-should-have-field-values? {:base_type :TimeField
:special_type :category
- :field_type :dimension}))
+ :visibility_type :normal}))
;; most special types should be excluded
(expect false (field-should-have-field-values? {:base_type :CharField
:special_type :image
- :field_type :dimension}))
+ :visibility_type :normal}))
(expect false (field-should-have-field-values? {:base_type :CharField
:special_type :id
- :field_type :dimension}))
+ :visibility_type :normal}))
(expect false (field-should-have-field-values? {:base_type :CharField
:special_type :fk
- :field_type :dimension}))
+ :visibility_type :normal}))
(expect false (field-should-have-field-values? {:base_type :CharField
:special_type :latitude
- :field_type :dimension}))
+ :visibility_type :normal}))
(expect false (field-should-have-field-values? {:base_type :CharField
:special_type :number
- :field_type :dimension}))
+ :visibility_type :normal}))
(expect false (field-should-have-field-values? {:base_type :CharField
:special_type :timestamp_milliseconds
- :field_type :dimension}))
+ :visibility_type :normal}))
;; boolean fields + category/city/state/country fields are g2g
(expect true (field-should-have-field-values? {:base_type :BooleanField
:special_type :number
- :field_type :dimension}))
+ :visibility_type :normal}))
(expect true (field-should-have-field-values? {:base_type :CharField
:special_type :category
- :field_type :dimension}))
+ :visibility_type :normal}))
(expect true (field-should-have-field-values? {:base_type :TextField
:special_type :city
- :field_type :dimension}))
+ :visibility_type :normal}))
(expect true (field-should-have-field-values? {:base_type :TextField
:special_type :state
- :field_type :dimension}))
+ :visibility_type :normal}))
(expect true (field-should-have-field-values? {:base_type :TextField
:special_type :country
- :field_type :dimension}))
+ :visibility_type :normal}))
(expect true (field-should-have-field-values? {:base_type :TextField
:special_type :name
- :field_type :dimension}))
+ :visibility_type :normal}))
diff --git a/test/metabase/models/field_values_test.clj b/test/metabase/models/field_values_test.clj
index de5c642daae..5d510d4e29b 100644
--- a/test/metabase/models/field_values_test.clj
+++ b/test/metabase/models/field_values_test.clj
@@ -11,27 +11,35 @@
(expect true
(field-should-have-field-values? {:special_type :category
- :field_type :info
+ :visibility_type :normal
:base_type :TextField}))
(expect false
(field-should-have-field-values? {:special_type :category
- :field_type :sensitive
+ :visibility_type :sensitive
+ :base_type :TextField}))
+(expect false
+ (field-should-have-field-values? {:special_type :category
+ :visibility_type :hidden
+ :base_type :TextField}))
+(expect false
+ (field-should-have-field-values? {:special_type :category
+ :visibility_type :details-only
:base_type :TextField}))
(expect false
(field-should-have-field-values? {:special_type nil
- :field_type :info
+ :visibility_type :normal
:base_type :TextField}))
(expect true
(field-should-have-field-values? {:special_type "country"
- :field_type :info
+ :visibility_type :normal
:base_type :TextField}))
(expect true
(field-should-have-field-values? {:special_type nil
- :field_type :info
+ :visibility_type :normal
:base_type "BooleanField"}))
diff --git a/test/metabase/test/data.clj b/test/metabase/test/data.clj
index abaa7969445..cedc8b66df5 100644
--- a/test/metabase/test/data.clj
+++ b/test/metabase/test/data.clj
@@ -175,7 +175,7 @@
table-name
(u/pprint-to-str (dissoc table-definition :rows))
(u/pprint-to-str (sel :many :fields [Table :schema :name], :db_id (:id db))))))))]
- (doseq [{:keys [field-name field-type special-type], :as field-definition} (:field-definitions table-definition)]
+ (doseq [{:keys [field-name field-type visibility-type special-type], :as field-definition} (:field-definitions table-definition)]
(let [field (delay (or (i/metabase-instance field-definition @table)
(throw (Exception. (format "Field '%s' not loaded from definition:\n"
field-name
@@ -183,6 +183,9 @@
(when field-type
(log/debug (format "SET FIELD TYPE %s.%s -> %s" table-name field-name field-type))
(upd Field (:id @field) :field_type (name field-type)))
+ (when visibility-type
+ (log/debug (format "SET VISIBILITY TYPE %s.%s -> %s" table-name field-name visibility-type))
+ (upd Field (:id @field) :visibility_type (name visibility-type)))
(when special-type
(log/debug (format "SET SPECIAL TYPE %s.%s -> %s" table-name field-name special-type))
(upd Field (:id @field) :special_type (name special-type)))))))
diff --git a/test/metabase/test/data/dataset_definitions/test-data.edn b/test/metabase/test/data/dataset_definitions/test-data.edn
index 32a29284bcc..60ce41280e3 100644
--- a/test/metabase/test/data/dataset_definitions/test-data.edn
+++ b/test/metabase/test/data/dataset_definitions/test-data.edn
@@ -28,7 +28,7 @@
:base-type :DateTimeField}
{:field-name "password"
:base-type :CharField
- :field-type :sensitive}]
+ :visibility-type :sensitive}]
[["Plato Yeshua" #inst "2014-04-01T08:30" "4be68cda-6fd5-4ba7-944e-2b475600bda5"]
["Felipinho Asklepios" #inst "2014-12-05T15:15" "5bb19ad9-f3f8-421f-9750-7d398e38428d"]
["Kaneonuskatew Eiran" #inst "2014-11-06T16:15" "a329ccfe-b99c-42eb-9c93-cb9adc3eb1ab"]
diff --git a/test/metabase/test/data/interface.clj b/test/metabase/test/data/interface.clj
index a2b86e0ce95..22894b9acf0 100644
--- a/test/metabase/test/data/interface.clj
+++ b/test/metabase/test/data/interface.clj
@@ -15,6 +15,7 @@
^Keyword base-type
^Keyword field-type
^Keyword special-type
+ ^Keyword visibility-type
^Keyword fk])
(defrecord TableDefinition [^String table-name
@@ -117,7 +118,7 @@
(defn create-field-definition
"Create a new `FieldDefinition`; verify its values."
- ^FieldDefinition [{:keys [field-name base-type field-type special-type fk], :as field-definition-map}]
+ ^FieldDefinition [{:keys [field-name base-type field-type special-type visibility-type fk], :as field-definition-map}]
(assert (or (contains? field/base-types base-type)
(and (map? base-type)
(string? (:native base-type))))
@@ -125,6 +126,8 @@
"Field base-type should be either a valid base type like :TextField or be some native type wrapped in a map, like {:native \"JSON\"}."))
(when field-type
(assert (contains? field/field-types field-type)))
+ (when visibility-type
+ (assert (contains? field/visibility-types visibility-type)))
(when special-type
(assert (contains? field/special-types special-type)))
(map->FieldDefinition field-definition-map))
--
GitLab