Remapping admin fixes for the backend

Added a route for deleting dimensions, and added field_id to the field
values response.

src/metabase/api/field.clj

 (ns metabase.api.field
-  (:require [compojure.core :refer [GET POST PUT]]
+  (:require [compojure.core :refer [GET POST PUT DELETE]]
             [metabase.api.common :as api]
             [metabase.db.metadata-queries :as metadata]
             [metabase.models
              [dimension :refer [Dimension]]
              [field :as field :refer [Field]]
-             [field-values :refer [create-field-values-if-needed! field-should-have-field-values? FieldValues]]]
+             [field-values :refer [create-field-values-if-needed! field-should-have-field-values? field-values->pairs FieldValues]]]
             [metabase.util :as u]
             [metabase.util.schema :as su]
             [schema.core :as s]
@@ -70,14 +70,18 @@
   "If `Field`'s special type derives from `type/Category`, or its base type is `type/Boolean`, return
    all distinct values of the field, and a map of human-readable values defined by the user."
   [id]
-  (let [field (api/read-check Field id)]
-    (if-not (field-should-have-field-values? field)
-      {:values []}
-      {:values (create-field-values-if-needed! field)})))
+  (try
+    (let [field (api/read-check Field id)]
+      (if-let [field-values (and (field-should-have-field-values? field)
+                                 (create-field-values-if-needed! field))]
+        (-> field-values
+            (assoc :values (field-values->pairs field-values))
+            (dissoc :human_readable_values))
+        {:values []}))
+    (catch Exception e (.printStackTrace e) (throw e))))
 
 (api/defendpoint POST "/:id/dimension"
-  "If `Field`'s special type derives from `type/Category`, or its base type is `type/Boolean`, return
-   all distinct values of the field, and a map of human-readable values defined by the user."
+  "Sets the dimension for the given field at ID"
   [id :as {{dimension-type :type dimension-name :name human_readable_field_id :human_readable_field_id} :body}]
   {dimension-type         (s/enum "internal" "external")
    dimension-name         su/NonBlankString
@@ -95,6 +99,13 @@
                    :human_readable_field_id human_readable_field_id}))
     (Dimension :field_id id)))
 
+(api/defendpoint DELETE "/:id/dimension"
+  "Remove the dimension associated to field at ID"
+  [id]
+  (let [field (api/read-check Field id)]
+    (db/delete! Dimension :field_id id)
+    api/generic-204-no-content))
+
 (defn validate-human-readable-pairs
   "Human readable values are optional, but if present they must be
   present for each field value. Throws if invalid, returns a boolean
@@ -124,18 +135,16 @@
                                (map second value-pairs)))))
 
 (api/defendpoint POST "/:id/values"
-  "Update the human-readable values for a `Field` whose special type is `category`/`city`/`state`/`country`
-   or whose base type is `type/Boolean`."
+  "Update the fields values and human-readable values for a `Field` whose special type is `category`/`city`/`state`/`country`
+   or whose base type is `type/Boolean`. The human-readable values are optional."
   [id :as {{value-pairs :values} :body}]
   {value-pairs [[(s/one s/Num "value") (s/optional su/NonBlankString "human readable value")]]}
-  (try
-    (let [field (api/write-check Field id)]
-      (api/check (field-should-have-field-values? field)
-        [400 "You can only update the human readable values of a mapped values of a Field whose 'special_type' is 'category'/'city'/'state'/'country' or whose 'base_type' is 'type/Boolean'."])
-      (if-let [field-value-id (db/select-one-id FieldValues, :field_id id)]
-        (update-field-values! field-value-id value-pairs)
-        (create-field-values! field value-pairs)))
-    (catch Exception e (println "fail") (.printStackTrace e) (throw e)))
+  (let [field (api/write-check Field id)]
+    (api/check (field-should-have-field-values? field)
+      [400 "You can only update the human readable values of a mapped values of a Field whose 'special_type' is 'category'/'city'/'state'/'country' or whose 'base_type' is 'type/Boolean'."])
+    (if-let [field-value-id (db/select-one-id FieldValues, :field_id id)]
+      (update-field-values! field-value-id value-pairs)
+      (create-field-values! field value-pairs)))
   {:status :success})
 
 (api/define-routes)

src/metabase/models/field_values.clj

@@ -74,9 +74,8 @@
   [{field-id :id :as field} & [human-readable-values]]
   {:pre [(integer? field-id)]}
   (when (field-should-have-field-values? field)
-    (field-values->pairs
-     (or (db/select-one [FieldValues :values :human_readable_values] :field_id field-id)
-         (create-field-values! field human-readable-values)))))
+    (or (FieldValues :field_id field-id)
+        (create-field-values! field human-readable-values))))
 
 (defn save-field-values!
   "Save the `FieldValues` for FIELD-ID, creating them if needed, otherwise updating them."

test/metabase/api/field_test.clj

@@ -16,6 +16,9 @@
 
 ;; Helper Fns
 
+(def ^:private default-field-values
+  {:id true, :created_at true, :updated_at true, :field_id true})
+
 (defn- db-details []
   (tu/match-$ (db)
     {:created_at         $
@@ -151,13 +154,13 @@
 ;; ## GET /api/field/:id/values
 ;; Should return something useful for a field that has special_type :type/Category
 (expect
-  {:values (mapv vector [1 2 3 4])}
+  (merge default-field-values {:values (mapv vector [1 2 3 4])})
   (do
     ;; clear out existing human_readable_values in case they're set
     (db/update! FieldValues (field-values-id :venues :price)
       :human_readable_values nil)
     ;; now update the values via the API
-    ((user->client :rasta) :get 200 (format "field/%d/values" (id :venues :price)))))
+    (tu/boolean-ids-and-timestamps ((user->client :rasta) :get 200 (format "field/%d/values" (id :venues :price))))))
 
 ;; Should return nothing for a field whose special_type is *not* :type/Category
 (expect
@@ -175,63 +178,68 @@
 
 ;; Human readable values are optional
 (expect
-  [{:values (map vector (range 5 10))}
+  [(merge default-field-values {:values (map vector (range 5 10))})
    {:status "success"}
-   {:values (map vector (range 1 5))}]
+   (merge default-field-values {:values (map vector (range 1 5))})]
   (tt/with-temp* [Field [{field-id :id} category-field]
                   FieldValues [{field-value-id :id} {:values (range 5 10), :field_id field-id}]]
-    [((user->client :crowberto) :get 200 (format "field/%d/values" field-id))
-     ((user->client :crowberto) :post 200 (format "field/%d/values" field-id)
-      {:values (map vector (range 1 5))})
-     ((user->client :crowberto) :get 200 (format "field/%d/values" field-id))]))
+    (mapv tu/boolean-ids-and-timestamps
+          [((user->client :crowberto) :get 200 (format "field/%d/values" field-id))
+           ((user->client :crowberto) :post 200 (format "field/%d/values" field-id)
+            {:values (map vector (range 1 5))})
+           ((user->client :crowberto) :get 200 (format "field/%d/values" field-id))])))
 
 ;; Existing field values can be updated (with their human readable values)
 (expect
-  [{:values (map vector (range 1 5))}
+  [(merge default-field-values {:values (map vector (range 1 5))})
    {:status "success"}
-   {:values (num->$ (range 1 5))}]
+   (merge default-field-values {:values (num->$ (range 1 5))})]
   (tt/with-temp* [Field [{field-id :id} category-field]
                   FieldValues [{field-value-id :id} {:values (range 1 5), :field_id field-id}]]
-    [((user->client :crowberto) :get 200 (format "field/%d/values" field-id))
-     ((user->client :crowberto) :post 200 (format "field/%d/values" field-id)
-      {:values (num->$ (range 1 5))})
-     ((user->client :crowberto) :get 200 (format "field/%d/values" field-id))]))
+    (mapv tu/boolean-ids-and-timestamps
+          [((user->client :crowberto) :get 200 (format "field/%d/values" field-id))
+           ((user->client :crowberto) :post 200 (format "field/%d/values" field-id)
+            {:values (num->$ (range 1 5))})
+           ((user->client :crowberto) :get 200 (format "field/%d/values" field-id))])))
 
 ;; Field values are created when not present
 (expect
-  [{:values []}
+  [(merge default-field-values {:values []})
    {:status "success"}
-   {:values (num->$ (range 1 5))}]
+   (merge default-field-values {:values (num->$ (range 1 5))})]
   (tt/with-temp* [Field [{field-id :id} category-field]]
-    [((user->client :crowberto) :get 200 (format "field/%d/values" field-id))
-     ((user->client :crowberto) :post 200 (format "field/%d/values" field-id)
-      {:values (num->$ (range 1 5))})
-     ((user->client :crowberto) :get 200 (format "field/%d/values" field-id))]))
+    (mapv tu/boolean-ids-and-timestamps
+          [((user->client :crowberto) :get 200 (format "field/%d/values" field-id))
+           ((user->client :crowberto) :post 200 (format "field/%d/values" field-id)
+            {:values (num->$ (range 1 5))})
+           ((user->client :crowberto) :get 200 (format "field/%d/values" field-id))])))
 
 ;; Can unset values
 (expect
-  [{:values (mapv vector (range 1 5))}
+  [(merge default-field-values {:values (mapv vector (range 1 5))})
    {:status "success"}
-   {:values []}]
+   (merge default-field-values {:values []})]
   (tt/with-temp* [Field [{field-id :id} category-field]
                   FieldValues [{field-value-id :id} {:values (range 1 5), :field_id field-id}]]
-    [((user->client :crowberto) :get 200 (format "field/%d/values" field-id))
-     ((user->client :crowberto) :post 200 (format "field/%d/values" field-id)
-      {:values []})
-     ((user->client :crowberto) :get 200 (format "field/%d/values" field-id))]))
+    (mapv tu/boolean-ids-and-timestamps
+          [((user->client :crowberto) :get 200 (format "field/%d/values" field-id))
+           ((user->client :crowberto) :post 200 (format "field/%d/values" field-id)
+            {:values []})
+           ((user->client :crowberto) :get 200 (format "field/%d/values" field-id))])))
 
 ;; Can unset just human readable values
 (expect
-  [{:values (num->$ (range 1 5))}
+  [(merge default-field-values {:values (num->$ (range 1 5))})
    {:status "success"}
-   {:values (mapv vector (range 1 5))}]
+   (merge default-field-values {:values (mapv vector (range 1 5))})]
   (tt/with-temp* [Field [{field-id :id} category-field]
                   FieldValues [{field-value-id :id} {:values (range 1 5), :field_id field-id
                                                      :human_readable_values ["$" "$$" "$$$" "$$$$"]}]]
-    [((user->client :crowberto) :get 200 (format "field/%d/values" field-id))
-     ((user->client :crowberto) :post 200 (format "field/%d/values" field-id)
-      {:values (mapv vector (range 1 5))})
-     ((user->client :crowberto) :get 200 (format "field/%d/values" field-id))]))
+    (mapv tu/boolean-ids-and-timestamps
+          [((user->client :crowberto) :get 200 (format "field/%d/values" field-id))
+           ((user->client :crowberto) :post 200 (format "field/%d/values" field-id)
+            {:values (mapv vector (range 1 5))})
+           ((user->client :crowberto) :get 200 (format "field/%d/values" field-id))])))
 
 ;; Should throw when human readable values are present but not for every value
 (expect
@@ -296,3 +304,21 @@
           new-dim         (dimension-for-field field-id-1)]
       [before-creation
        (tu/boolean-ids-and-timestamps new-dim)])))
+
+(expect
+  [{:id true
+    :created_at true
+    :updated_at true
+    :type :internal
+    :name "some dimension name"
+    :human_readable_field_id false
+    :field_id true}
+   []]
+  (tt/with-temp* [Field [{field-id :id} {:name "Field Test"}]]
+
+    (dimension-post field-id {:name "some dimension name", :type "internal"})
+
+    (let [new-dim (dimension-for-field field-id)]
+      ((user->client :crowberto) :delete 204 (format "field/%d/dimension" field-id))
+      [(tu/boolean-ids-and-timestamps new-dim)
+       (dimension-for-field field-id)])))