Merge branch 'master' of github.com:metabase/metabase into chart-settings

frontend/src/metabase/admin/settings/selectors.js

@@ -41,6 +41,11 @@ const SECTIONS = [
                 key: "anon-tracking-enabled",
                 display_name: "Anonymous Tracking",
                 type: "boolean"
+            },
+            {
+                key: "enable-advanced-humanization",
+                display_name: "Friendly Table and Field Names",
+                type: "boolean"
             }
         ]
     },

frontend/src/metabase/components/HeaderModal.jsx

@@ -22,7 +22,7 @@ export default class HeaderModal extends Component {
         return (
             <div
                 className={cx(className, "absolute top left right bg-brand flex flex-column layout-centered")}
-                style={{ zIndex: 2, height: height, minHeight: 50, transform: `translateY(${isOpen ? initialTop : "-100%"})`, transition: "transform 400ms ease-in-out" }}
+                style={{ zIndex: 2, height: height, minHeight: 50, transform: `translateY(${isOpen ? initialTop : "-100%"})`, transition: "transform 400ms ease-in-out", overflow: 'hidden' }}
             >
                     <h2 className="text-white pb2">{title}</h2>
                     <div className="flex layout-centered">

frontend/src/metabase/query_builder/VisualizationError.jsx

@@ -38,7 +38,7 @@ class VisualizationError extends Component {
           if (duration > 15*1000) {
               return <VisualizationErrorMessage
                         type="timeout"
-                        title="Your question took to long"
+                        title="Your question took too long"
                         message="We didn't get an answer back from your database in time, so we had to stop. You can try again in a minute, or if the problem persists, you can email an admin to let them know."
                         action={<EmailAdmin />}
                     />

src/metabase/db.clj

@@ -653,6 +653,7 @@
   (let [entity (resolve-entity entity)]
     (apply select-one-field :id entity options)))
 
+;; TODO - maybe rename this `count`? e.g. `db/count` instead of `db/select-one-count`
 (defn select-one-count
   "Select the count of objects matching some condition.
 

src/metabase/models/common.clj

-(ns metabase.models.common
-  (:require [clojure.string :as s]
-            [metabase.util.infer-spaces :refer [infer-spaces]]))
+(ns metabase.models.common)
 
 (def ^:const timezones
   "The different timezones supported by Metabase.
@@ -100,20 +98,3 @@
   [{:id perms-none,      :name "None"},
    {:id perms-read,      :name "Read Only"},
    {:id perms-readwrite, :name "Read & Write"}])
-
-
-
-(defn name->human-readable-name
-  "Convert a string NAME of some object like a `Table` or `Field` to one more friendly to humans.
-
-    (name->human-readable-name \"admin_users\") -> \"Admin Users\""
-  [^String n]
-  (when (seq n)
-    (->> (s/split n #"[-_\s]+|(?<=[a-z])(?=[A-Z])") ; explode string on spaces, underscores, hyphens, and camelCase
-         (filter (complement s/blank?))             ; remove blanks
-         (map infer-spaces)                         ; infer spaces
-         (flatten)                                  ; flatten arrays returned by infer-spaces
-         (map s/capitalize)                         ; capitalize
-         (dedupe)                                   ; remove adjacent duplicates
-         (map #(if (= % "Id") "ID" %))              ; special case "ID"
-         (s/join " "))))                            ; convert back to a single string

src/metabase/models/field.clj

@@ -3,8 +3,8 @@
                      [string :as s])
             [medley.core :as m]
             [metabase.db :as db]
-            (metabase.models [common :as common]
-                             [field-values :refer [FieldValues]]
+            (metabase.models [field-values :refer [FieldValues]]
+                             [humanization :as humanization]
                              [interface :as i])
             [metabase.util :as u]))
 
@@ -82,7 +82,7 @@
                   :field_type      :info
                   :visibility_type :normal
                   :position        0
-                  :display_name    (common/name->human-readable-name (:name field))}]
+                  :display_name    (humanization/name->human-readable-name (:name field))}]
     (merge defaults field)))
 
 (defn- pre-cascade-delete [{:keys [id]}]
@@ -210,7 +210,7 @@
   (let [updated-field (assoc existing-field
                         :base_type    base-type
                         :display_name (or (:display_name existing-field)
-                                          (common/name->human-readable-name field-name))
+                                          (humanization/name->human-readable-name field-name))
                         :special_type (or (:special_type existing-field)
                                           special-type
                                           (when pk? :id)
@@ -239,7 +239,7 @@
     :table_id      table-id
     :raw_column_id raw-column-id
     :name          field-name
-    :display_name  (common/name->human-readable-name field-name)
+    :display_name  (humanization/name->human-readable-name field-name)
     :base_type     base-type
     :special_type  (or special-type
                        (when pk? :id)

src/metabase/models/humanization.clj

+(ns metabase.models.humanization
+  "Logic related to humanization of table names and other identifiers,
+   e.g. taking an identifier like `my_table` and returning a human-friendly one like `My Table`.
+
+   There are two implementations of humanization logic; advanced, cost-based logic is the default;
+   which implementation is used is determined by the Setting `enable-advanced-humanization`.
+
+   The actual algorithm for advanced humanization is in `metabase.util.infer-spaces`."
+  (:require [clojure.string :as s]
+            [clojure.tools.logging :as log]
+            [metabase.db :as db]
+            [metabase.models.setting :refer [defsetting], :as setting]
+            [metabase.util.infer-spaces :refer [infer-spaces]]))
+
+
+(defn- capitalize-word [word]
+  (if (= word "id")
+    "ID"
+    (s/capitalize word)))
+
+(defn- name->human-readable-name:advanced
+  "Implementation of `name->human-readable-name` used if the Setting `enable-advanced-humanization` is `false`."
+  ^String [^String s]
+  (when (seq s)
+    ;; explode string on spaces, underscores, hyphens, and camelCase
+    (s/join " " (for [part  (s/split s #"[-_\s]+|(?<=[a-z])(?=[A-Z])")
+                      :when (not (s/blank? part))
+                      word  (dedupe (flatten (infer-spaces part)))]
+                  (capitalize-word word)))))
+
+(defn- name->human-readable-name:simple
+  "Implementation of `name->human-readable-name` used if the Setting `enable-advanced-humanization` is `true`."
+  ^String [^String s]
+  ;; explode on hypens, underscores, and spaces
+  (when (seq s)
+    (s/join " " (for [part  (s/split s #"[-_\s]+")
+                      :when (not (s/blank? part))]
+                  (capitalize-word part)))))
+
+
+(declare enable-advanced-humanization)
+
+(defn name->human-readable-name
+  "Convert a string NAME of some object like a `Table` or `Field` to one more friendly to humans.
+
+    (name->human-readable-name \"admin_users\") -> \"Admin Users\"
+
+   (The actual implementation of this function depends on the value of `enable-advanced-humanization`; by default,
+   `name->human-readable-name:advanced` is used)."
+  ^String [^String s]
+  ((if (enable-advanced-humanization)
+      name->human-readable-name:advanced
+      name->human-readable-name:simple) s))
+
+
+(defn- custom-display-name?
+  "Is DISPLAY-NAME a custom name that was set manually by the user in the metadata edit screen?"
+  [internal-name display-name]
+  (and (not= display-name (name->human-readable-name:simple   internal-name))
+       (not= display-name (name->human-readable-name:advanced internal-name))))
+
+(defn- re-humanize-names! [model]
+  (doseq [{id :id, internal-name :name, display-name :display_name} (db/select [model :id :name :display_name])
+          :let                                                      [new-display-name (name->human-readable-name internal-name)]
+          :when                                                     (and (not= display-name new-display-name)
+                                                                         (not (custom-display-name? internal-name display-name)))]
+    (log/info (format "Updating display name for %s '%s': '%s' -> '%s'" (name model) internal-name display-name new-display-name))
+    (db/update! model id
+      :display_name new-display-name)))
+
+(defn- re-humanize-table-and-field-names!
+  "Update the display names of all tables in the database using new values obtained from the (obstensibly toggled implementation of) `name->human-readable-name`."
+  []
+  (re-humanize-names! 'Table)
+  (re-humanize-names! 'Field))
+
+
+(defn- set-enable-advanced-humanization! [^Boolean new-value]
+  (setting/set-boolean! :enable-advanced-humanization new-value)
+  (log/info (format "Now using %s table name humanization." (if (enable-advanced-humanization) "ADVANCED" "SIMPLE")))
+  (re-humanize-table-and-field-names!))
+
+(defsetting enable-advanced-humanization
+  "Metabase can attempt to transform your table and field names into more sensible human readable versions, e.g. \"somehorriblename\" becomes \"Some Horrible Name\".
+   This doesn’t work all that well if the names are in a language other than English, however. Do you want us to take a guess?"
+  :type    :boolean
+  :default true
+  :setter  set-enable-advanced-humanization!)

src/metabase/models/interface.clj

@@ -244,6 +244,10 @@
         map->instance         (symbol (str "map->" instance))]
     `(do
        (defrecord ~instance []
+         clojure.lang.Named
+         (~'getName [~'_]
+          ~(name entity))
+
          clojure.lang.IFn
          (~'invoke [this#]
           (invoke-entity-or-instance this#))

src/metabase/models/setting.clj

@@ -55,13 +55,13 @@
   (s/enum :string :boolean :json))
 
 (def ^:private SettingDefinition
-  {:setting-name s/Keyword
-   :description  s/Str            ; used for docstring and is user-facing in the admin panel
-   :default      s/Any            ; this is a string because in the DB all settings are stored as strings; different getters can handle type conversion *from* string
-   :setting-type Type
-   :getter       clojure.lang.IFn
-   :setter       clojure.lang.IFn
-   :internal?    s/Bool})         ; should the API never return this setting? (default: false)
+  {:name        s/Keyword
+   :description s/Str            ; used for docstring and is user-facing in the admin panel
+   :default     s/Any            ; this is a string because in the DB all settings are stored as strings; different getters can handle type conversion *from* string
+   :type        Type
+   :getter      clojure.lang.IFn
+   :setter      clojure.lang.IFn
+   :internal?   s/Bool})         ; should the API never return this setting? (default: false)
 
 
 (defonce ^:private registered-settings
@@ -92,7 +92,7 @@
 ;;; ------------------------------------------------------------ get ------------------------------------------------------------
 
 (defn- setting-name ^String [setting-or-name]
-  (name (:setting-name (resolve-setting setting-or-name))))
+  (name (:name (resolve-setting setting-or-name))))
 
 (defn- env-var-name
   "Get the env var corresponding to SETTING-OR-NAME.
@@ -234,13 +234,13 @@
    This is used internally be `defsetting`; you shouldn't need to use it yourself."
   [{setting-name :name, setting-type :type, default :default, :as setting}]
   (u/prog1 (let [setting-type (s/validate Type (or setting-type :string))]
-             (merge {:setting-name setting-name
-                     :description  nil
-                     :setting-type setting-type
-                     :default      default
-                     :getter       (partial (default-getter-for-type setting-type) setting-name)
-                     :setter       (partial (default-setter-for-type setting-type) setting-name)
-                     :internal?    false}
+             (merge {:name        setting-name
+                     :description nil
+                     :type        setting-type
+                     :default     default
+                     :getter      (partial (default-getter-for-type setting-type) setting-name)
+                     :setter      (partial (default-setter-for-type setting-type) setting-name)
+                     :internal?   false}
                     (dissoc setting :name :type :default)))
     (s/validate SettingDefinition <>)
     (swap! registered-settings assoc setting-name <>)))
@@ -251,18 +251,28 @@
 
 (defn metadata-for-setting-fn
   "Create metadata for the function automatically generated by `defsetting`."
-  [{:keys [default setting-type], :as setting}]
+  [{:keys [default description], setting-type :type, :as setting}]
   {:arglists '([] [new-value])
-   :doc      (str (format "`%s` is a %s `Setting`. You can get its value by calling\n\n" (setting-name setting) (name setting-type))
-                  (format  "    (%s)\n\n" (setting-name setting))
-                  "and set its value by calling\n\n"
-                  (format "    (%s <new-value>)\n\n" (setting-name setting))
-                  (format "You can also set its value with the env var `%s`.\n" (env-var-name setting))
-                  "Clear its value by calling\n\n"
-                  (format "    (%s nil)\n\n" (setting-name setting))
-                  (format "Its default value is `%s`." (if (nil? default)
-                                                         "nil"
-                                                         default)))})
+   ;; indentation below is intentional to make it clearer what shape the generated documentation is going to take. Turn on auto-complete-mode in Emacs and see for yourself!
+   :doc (str/join "\n" [        description
+                                ""
+                        (format "`%s` is a %s `Setting`. You can get its value by calling:" (setting-name setting) (name setting-type))
+                                ""
+                        (format "    (%s)"                                                  (setting-name setting))
+                                ""
+                                "and set its value by calling:"
+                                ""
+                        (format "    (%s <new-value>)"                                      (setting-name setting))
+                                ""
+                        (format "You can also set its value with the env var `%s`."         (env-var-name setting))
+                                ""
+                                "Clear its value by calling:"
+                                ""
+                        (format "    (%s nil)"                                              (setting-name setting))
+                                ""
+                        (format "Its default value is `%s`."                                (if (nil? default) "nil" default))])})
+
+
 
 (defn setting-fn
   "Create the automatically defined getter/setter function for settings defined by `defsetting`."
@@ -321,16 +331,22 @@
   ;; irrelevant changes (to other settings) are made.
   (events/publish-event :settings-update settings))
 
-(defn all
-  "Return a sequence of Settings maps, including value and description.
-   (For security purposes, this doesn't return the value of a setting if it was set via env var)."
-  []
-  (for [[k setting] (sort-by first @registered-settings)
-        :let        [v (get k)]]
+
+(defn- user-facing-info [setting]
+  (let [k (:name setting)
+        v (get k)]
     {:key         k
-     :value       (when (not= v (env-var-value setting))
+     :value       (when (and (not= v (env-var-value setting))
+                             (not= v (:default setting)))
                     v)
      :description (:description setting)
      :default     (or (when (env-var-value setting)
                         (format "Using $%s" (env-var-name setting)))
                       (:default setting))}))
+
+(defn all
+  "Return a sequence of Settings maps in a format suitable for consumption by the frontend.
+   (For security purposes, this doesn't return the value of a setting if it was set via env var)."
+  []
+  (for [setting (sort-by :name (vals @registered-settings))]
+    (user-facing-info setting)))

src/metabase/models/table.clj

 (ns metabase.models.table
   (:require [metabase.db :as db]
-            (metabase.models [common :as common]
-                             [database :refer [Database]]
+            (metabase.models [database :refer [Database]]
                              [field :refer [Field]]
                              [field-values :refer [FieldValues]]
+                             [humanization :as humanization]
                              [interface :as i]
                              [metric :refer [Metric retrieve-metrics]]
                              [segment :refer [Segment retrieve-segments]])
@@ -20,7 +20,7 @@
 (i/defentity Table :metabase_table)
 
 (defn- pre-insert [table]
-  (let [defaults {:display_name (common/name->human-readable-name (:name table))}]
+  (let [defaults {:display_name (humanization/name->human-readable-name (:name table))}]
     (merge defaults table)))
 
 (defn- pre-cascade-delete [{:keys [id]}]
@@ -110,7 +110,7 @@
   [{:keys [id display_name], :as existing-table} {table-name :name}]
   {:pre [(integer? id)]}
   (let [updated-table (assoc existing-table
-                        :display_name (or display_name (common/name->human-readable-name table-name)))]
+                        :display_name (or display_name (humanization/name->human-readable-name table-name)))]
     ;; the only thing we need to update on a table is the :display_name, if it never got set
     (when (nil? display_name)
       (db/update! Table id
@@ -127,5 +127,5 @@
     :schema          schema-name
     :name            table-name
     :visibility_type visibility-type
-    :display_name    (common/name->human-readable-name table-name)
+    :display_name    (humanization/name->human-readable-name table-name)
     :active          true))

src/metabase/util/infer_spaces.clj

 (ns metabase.util.infer-spaces
-    (:require [clojure.java.io :as io]
-              [clojure.string :as s])
-    (:import java.lang.Math))
+  "Logic for automatically inferring where spaces should go in table names. Ported from ported from https://stackoverflow.com/questions/8870261/how-to-split-text-without-spaces-into-list-of-words/11642687#11642687."
+  (:require [clojure.java.io :as io]
+            [clojure.string :as s])
+  (:import java.lang.Math))
 
-;; ported from https://stackoverflow.com/questions/8870261/how-to-split-text-without-spaces-into-list-of-words/11642687#11642687
 
 (def ^:const ^:private special-words ["checkins"])
 
@@ -12,8 +12,9 @@
 
 ;; wordcost = dict((k, log((i+1)*log(len(words)))) for i,k in enumerate(words))
 (def ^:private word-cost
-  (apply hash-map (flatten (map-indexed
-    (fn [idx word] [word (Math/log (* (inc idx) (Math/log (count words))))]) words))))
+  (into {} (map-indexed (fn [idx word]
+                          [word (Math/log (* (inc idx) (Math/log (count words))))])
+                        words)))
 
 ;; maxword = max(len(x) for x in words)
 (def ^:private max-word (apply max (map count words)))

test/metabase/driver/bigquery_test.clj

@@ -61,8 +61,8 @@
   ["birds_50" [{:field-name "name", :base-type :TextField}] [["Rasta"] ["Lucky"]]]
   ["birds_51" [{:field-name "name", :base-type :TextField}] [["Rasta"] ["Lucky"]]])
 
-;; only run this test 1 out of every 4 times since it takes like 5-10 minutes just to sync the DB and we don't have all day
-(when (> (rand) 0.75)
+;; only run this test 1 out of every 5 times since it takes like 5-10 minutes just to sync the DB and we don't have all day
+(when (> (rand) 0.80)
   (expect-with-engine :bigquery
     51
     (data/with-temp-db [db fifty-one-different-tables]

test/metabase/models/common_test.clj

-(ns metabase.models.common-test
-  (:require [expectations :refer :all]
-            [metabase.models.common :refer [name->human-readable-name]]
-            [metabase.test.data :refer :all]))
-
-
-;; testing on `(name->human-readable-name)`
-(expect nil (name->human-readable-name nil))
-(expect nil (name->human-readable-name ""))
-(expect "" (name->human-readable-name "_"))
-(expect "" (name->human-readable-name "-"))
-(expect "ID" (name->human-readable-name "_id"))
-(expect "Agent Invite Migration" (name->human-readable-name "_agent_invite_migration"))
-(expect "Agent Invite Migration" (name->human-readable-name "-agent-invite-migration"))
-(expect "Foo Bar" (name->human-readable-name "fooBar"))
-(expect "Foo Bar" (name->human-readable-name "foo-bar"))
-(expect "Foo Bar" (name->human-readable-name "foo_bar"))
-(expect "Foo Bar" (name->human-readable-name "foo bar"))
-(expect "Dashboard Card Subscription" (name->human-readable-name "dashboardcardsubscription"))
-(expect "Foo ID" (name->human-readable-name "foo_id"))
-(expect "Receiver ID" (name->human-readable-name "receiver_id"))

test/metabase/models/humanization_test.clj

+(ns metabase.models.humanization-test
+  (:require [expectations :refer :all]
+            (metabase.models [field :refer [Field]]
+                             [humanization :refer :all]
+                             [table :refer [Table]])
+            [metabase.test.util :as tu]
+            [metabase.db :as db]))
+
+(tu/resolve-private-fns metabase.models.humanization
+  name->human-readable-name:simple name->human-readable-name:advanced)
+
+;;; name->human-readable-name:advanced
+(expect nil                           (name->human-readable-name:advanced nil))
+(expect nil                           (name->human-readable-name:advanced ""))
+(expect ""                            (name->human-readable-name:advanced "_"))
+(expect ""                            (name->human-readable-name:advanced "-"))
+(expect "ID"                          (name->human-readable-name:advanced "_id"))
+(expect "Agent Invite Migration"      (name->human-readable-name:advanced "_agent_invite_migration"))
+(expect "Agent Invite Migration"      (name->human-readable-name:advanced "-agent-invite-migration"))
+(expect "Foo Bar"                     (name->human-readable-name:advanced "fooBar"))
+(expect "Foo Bar"                     (name->human-readable-name:advanced "foo-bar"))
+(expect "Foo Bar"                     (name->human-readable-name:advanced "foo_bar"))
+(expect "Foo Bar"                     (name->human-readable-name:advanced "foo bar"))
+(expect "Dashboard Card Subscription" (name->human-readable-name:advanced "dashboardcardsubscription"))
+(expect "Foo ID"                      (name->human-readable-name:advanced "foo_id"))
+(expect "Receiver ID"                 (name->human-readable-name:advanced "receiver_id"))
+
+
+;;; name->human-readable-name:simple
+(expect nil                         (name->human-readable-name:simple nil))
+(expect nil                         (name->human-readable-name:simple ""))
+(expect ""                          (name->human-readable-name:simple "_"))
+(expect ""                          (name->human-readable-name:simple "-"))
+(expect "ID"                        (name->human-readable-name:simple "_id"))
+(expect "Agent Invite Migration"    (name->human-readable-name:simple "_agent_invite_migration"))
+(expect "Agent Invite Migration"    (name->human-readable-name:simple "-agent-invite-migration"))
+(expect "Foobar"                    (name->human-readable-name:simple "fooBar"))
+(expect "Foo Bar"                   (name->human-readable-name:simple "foo-bar"))
+(expect "Foo Bar"                   (name->human-readable-name:simple "foo_bar"))
+(expect "Foo Bar"                   (name->human-readable-name:simple "foo bar"))
+(expect "Dashboardcardsubscription" (name->human-readable-name:simple "dashboardcardsubscription"))
+(expect "Foo ID"                    (name->human-readable-name:simple "foo_id"))
+(expect "Receiver ID"               (name->human-readable-name:simple "receiver_id"))
+
+
+;;; Re-humanization
+
+;; check that we get the expected :display_name with advanced humanization *enabled*
+(expect
+  "Toucans Are Cool"
+  (tu/with-temporary-setting-values [enable-advanced-humanization true]
+    (tu/with-temp Table [{table-id :id} {:name "toucansare_cool"}]
+      (db/select-one-field :display_name Table, :id table-id))))
+
+(expect
+  "Fussy Bird Sightings"
+  (tu/with-temporary-setting-values [enable-advanced-humanization true]
+    (tu/with-temp Field [{field-id :id} {:name "fussybird_sightings"}]
+      (db/select-one-field :display_name Field, :id field-id))))
+
+;; check that we get the expected :display_name with advanced humanization *disabled*
+(expect
+  "Toucansare Cool"
+  (tu/with-temporary-setting-values [enable-advanced-humanization false]
+    (tu/with-temp Table [{table-id :id} {:name "toucansare_cool"}]
+      (db/select-one-field :display_name Table, :id table-id))))
+
+(expect
+  "Fussybird Sightings"
+  (tu/with-temporary-setting-values [enable-advanced-humanization false]
+    (tu/with-temp Field [{field-id :id} {:name "fussybird_sightings"}]
+      (db/select-one-field :display_name Field, :id field-id))))
+
+;; now check that existing tables have their :display_names updated appropriately when the setting `enabled-advanced-humanization` is toggled
+(expect
+  ["Toucans Are Cool"
+   "Toucansare Cool"
+   "Toucans Are Cool"]
+  (tu/with-temporary-setting-values [enable-advanced-humanization true]
+    (tu/with-temp Table [{table-id :id} {:name "toucansare_cool"}]
+      (let [display-name #(db/select-one-field :display_name Table, :id table-id)]
+        [(display-name)
+         (do (enable-advanced-humanization false)
+             (display-name))
+         (do (enable-advanced-humanization true)
+             (display-name))]))))
+
+(expect
+  ["Fussy Bird Sightings"
+   "Fussybird Sightings"
+   "Fussy Bird Sightings"]
+  (tu/with-temporary-setting-values [enable-advanced-humanization true]
+    (tu/with-temp Field [{field-id :id} {:name "fussybird_sightings"}]
+      (let [display-name #(db/select-one-field :display_name Field, :id field-id)]
+        [(display-name)
+         (do (enable-advanced-humanization false)
+             (display-name))
+         (do (enable-advanced-humanization true)
+             (display-name))]))))
+
+;; check that if we give a field a custom display_name that re-humanization doesn't overwrite it
+(expect
+  "My Favorite Table"
+  (tu/with-temporary-setting-values [enable-advanced-humanization true]
+    (tu/with-temp Table [{table-id :id} {:name "toucansare_cool"}]
+      (db/update! Table table-id
+        :display_name "My Favorite Table")
+      (enable-advanced-humanization false)
+      (db/select-one-field :display_name Table, :id table-id))))
+
+(expect
+  "My Favorite Field"
+  (tu/with-temporary-setting-values [enable-advanced-humanization true]
+    (tu/with-temp Field [{field-id :id} {:name "fussybird_sightings"}]
+      (db/update! Field field-id
+        :display_name "My Favorite Field")
+      (enable-advanced-humanization false)
+      (db/select-one-field :display_name Field, :id field-id))))
+
+;; make sure switching in the other direction doesn't stomp all over custom names either
+(expect
+  "My Favorite Table"
+  (tu/with-temporary-setting-values [enable-advanced-humanization false]
+    (tu/with-temp Table [{table-id :id} {:name "toucansare_cool"}]
+      (db/update! Table table-id
+        :display_name "My Favorite Table")
+      (enable-advanced-humanization true)
+      (db/select-one-field :display_name Table, :id table-id))))
+
+(expect
+  "My Favorite Field"
+  (tu/with-temporary-setting-values [enable-advanced-humanization false]
+    (tu/with-temp Field [{field-id :id} {:name "fussybird_sightings"}]
+      (db/update! Field field-id
+        :display_name "My Favorite Field")
+      (enable-advanced-humanization true)
+      (db/select-one-field :display_name Field, :id field-id))))

test/metabase/models/setting_test.clj

@@ -18,6 +18,14 @@
   "Test setting - this only shows up in dev (2)"
   :default "[Default Value]")
 
+(defsetting test-boolean-setting
+  "Test setting - this only shows up in dev (3)"
+  :type :boolean)
+
+(defsetting test-json-setting
+  "Test setting - this only shows up in dev (4)"
+  :type :json)
+
 
 ;; ## HELPER FUNCTIONS
 
@@ -104,7 +112,58 @@
    (setting-exists-in-db? :test-setting-2)])
 
 
-;; ## ALL SETTINGS FUNCTIONS
+;;; ------------------------------------------------------------ all & user-facing-info ------------------------------------------------------------
+
+(resolve-private-fns metabase.models.setting resolve-setting user-facing-info)
+
+;; these tests are to check that settings get returned with the correct information; these functions are what feed into the API
+
+(defn- user-facing-info-with-db-and-env-var-values [setting db-value env-var-value]
+  (do-with-temporary-setting-value setting db-value
+    (fn []
+      (with-redefs [environ.core/env {(keyword (str "mb-" (name setting))) env-var-value}]
+        (dissoc (user-facing-info (resolve-setting setting))
+                :key :description)))))
+
+;; user-facing-info w/ no db value, no env var value, no default value
+(expect
+  {:value nil, :default nil}
+  (user-facing-info-with-db-and-env-var-values :test-setting-1 nil nil))
+
+;; user-facing-info w/ no db value, no env var value, default value
+(expect
+  {:value nil, :default "[Default Value]"}
+  (user-facing-info-with-db-and-env-var-values :test-setting-2 nil nil))
+
+;; user-facing-info w/ no db value, env var value, no default value -- shouldn't leak env var value
+(expect
+  {:value nil, :default "Using $MB_TEST_SETTING_1"}
+  (user-facing-info-with-db-and-env-var-values :test-setting-1 nil "TOUCANS"))
+
+;; user-facing-info w/ no db value, env var value, default value
+(expect
+  {:value nil, :default "Using $MB_TEST_SETTING_2"}
+  (user-facing-info-with-db-and-env-var-values :test-setting-2 nil "TOUCANS"))
+
+;; user-facing-info w/ db value, no env var value, no default value
+(expect
+  {:value "WOW", :default nil}
+  (user-facing-info-with-db-and-env-var-values :test-setting-1 "WOW" nil))
+
+;; user-facing-info w/ db value, no env var value, default value
+(expect
+  {:value "WOW", :default "[Default Value]"}
+  (user-facing-info-with-db-and-env-var-values :test-setting-2 "WOW" nil))
+
+;; user-facing-info w/ db value, env var value, no default value -- the DB value should take precedence over the env var
+(expect
+  {:value "WOW", :default "Using $MB_TEST_SETTING_1"}
+  (user-facing-info-with-db-and-env-var-values :test-setting-1 "WOW" "ENV VAR"))
+
+;; user-facing-info w/ db value, env var value, default value -- env var should take precedence over default value
+(expect
+  {:value "WOW", :default "Using $MB_TEST_SETTING_2"}
+  (user-facing-info-with-db-and-env-var-values :test-setting-2 "WOW" "ENV VAR"))
 
 ;; all
 (expect
@@ -123,3 +182,59 @@
       (for [setting (setting/all)
             :when   (re-find #"^test-setting-\d$" (name (:key setting)))]
         setting)))
+
+
+;;; ------------------------------------------------------------ BOOLEAN SETTINGS ------------------------------------------------------------
+
+(expect
+  {:value nil, :default nil}
+  (user-facing-info-with-db-and-env-var-values :test-boolean-setting nil nil))
+
+;; boolean settings shouldn't be obfuscated when set by env var
+(expect
+  {:value true, :default "Using $MB_TEST_BOOLEAN_SETTING"}
+  (user-facing-info-with-db-and-env-var-values :test-boolean-setting nil "true"))
+
+;; env var values should be case-insensitive
+(expect
+  (user-facing-info-with-db-and-env-var-values :test-boolean-setting nil "TRUE"))
+
+;; should throw exception if value isn't true / false
+(expect
+  Exception
+  (test-boolean-setting "X"))
+
+(expect
+  Exception
+  (user-facing-info-with-db-and-env-var-values :test-boolean-setting nil "X"))
+
+;; should be able to set value with a string...
+(expect
+  "false"
+  (test-boolean-setting "FALSE"))
+
+(expect
+  false
+  (do (test-boolean-setting "FALSE")
+      (test-boolean-setting)))
+
+;; ... or a boolean
+(expect
+  "false"
+  (test-boolean-setting false))
+
+(expect
+  false
+  (do (test-boolean-setting false)
+      (test-boolean-setting)))
+
+;;; ------------------------------------------------------------ JSON SETTINGS ------------------------------------------------------------
+
+(expect
+  "{\"a\":100,\"b\":200}"
+  (test-json-setting {:a 100, :b 200}))
+
+(expect
+  {:a 100, :b 200}
+  (do (test-json-setting {:a 100, :b 200})
+      (test-json-setting)))

test/metabase/test/util.clj

@@ -150,7 +150,8 @@
                                 :field_type      :info
                                 :name            (random-name)
                                 :position        1
-                                :preview_display true})})
+                                :preview_display true
+                                :table_id        (data/id :venues)})})
 
 (u/strict-extend (class Metric)
   WithTempDefaults
@@ -200,7 +201,8 @@
 
 (u/strict-extend (class Table)
   WithTempDefaults
-  {:with-temp-defaults (fn [_] {:active true
+  {:with-temp-defaults (fn [_] {:db_id  (data/id)
+                                :active true
                                 :name   (random-name)})})
 
 (u/strict-extend (class User)
@@ -328,6 +330,7 @@
    This works much the same way as `binding`.
 
    Prefer the macro `with-temporary-setting-values` over using this function directly."
+  {:style/indent 2}
   [setting-k value f]
   (let [original-value (setting/get setting-k)]
     (try

test_resources/log4j.properties

@@ -15,6 +15,7 @@ log4j.appender.file.layout=org.apache.log4j.PatternLayout
 log4j.appender.file.layout.ConversionPattern=%d [%t] %-5p%c - %m%n
 
 # customizations to logging by package
+log4j.logger.com.mchange=ERROR
 log4j.logger.metabase=INFO
+log4j.logger.metabase.models.humanization=WARN
 log4j.logger.metabase.sync-database=WARN
-log4j.logger.com.mchange=ERROR