Endpoints for listing all segments/metrics

src/metabase/api/metric.clj

@@ -3,8 +3,9 @@
   (:require [compojure.core :refer [defroutes GET PUT POST DELETE]]
             [metabase.api.common :refer :all]
             [metabase.db :as db]
-            (metabase.models [revision :as revision]
-                             [metric :refer [Metric] :as metric]
+            (metabase.models [hydrate :refer [hydrate]]
+                             [metric :refer [Metric], :as metric]
+                             [revision :as revision]
                              [table :refer [Table]])))
 
 
@@ -25,6 +26,13 @@
   (check-superuser)
   (check-404 (metric/retrieve-metric id)))
 
+(defendpoint GET "/"
+  "Fetch all `Metrics`. You must be a superuser to do this."
+  [id]
+  (check-superuser)
+  (check-404 (-> (db/select Metric, :is_active true)
+                 (hydrate :creator))))
+
 
 (defendpoint PUT "/:id"
   "Update a `Metric` with ID."

src/metabase/api/segment.clj

@@ -3,7 +3,8 @@
   (:require [compojure.core :refer [defroutes GET PUT POST DELETE]]
             [metabase.api.common :refer :all]
             [metabase.db :as db]
-            (metabase.models [revision :as revision]
+            (metabase.models [hydrate :refer [hydrate]]
+                             [revision :as revision]
                              [segment :refer [Segment] :as segment]
                              [table :refer [Table]])))
 
@@ -25,6 +26,13 @@
   (check-superuser)
   (check-404 (segment/retrieve-segment id)))
 
+(defendpoint GET "/"
+  "Fetch *all* `Segments`. You must be a superuser."
+  []
+  (check-superuser)
+  (-> (db/select Segment, :is_active true)
+      (hydrate :creator)))
+
 
 (defendpoint PUT "/:id"
   "Update a `Segment` with ID."
@@ -33,7 +41,7 @@
    revision_message [Required NonEmptyString]
    definition       [Required Dict]}
   (check-superuser)
-  (check-404 (segment/exists-segment? id))
+  (check-404 (segment/exists? id))
   (segment/update-segment!
     {:id               id
      :name             name
@@ -48,7 +56,7 @@
   [id revision_message]
   {revision_message [Required NonEmptyString]}
   (check-superuser)
-  (check-404 (segment/exists-segment? id))
+  (check-404 (segment/exists? id))
   (segment/delete-segment! id *current-user-id* revision_message)
   {:success true})
 
@@ -57,7 +65,7 @@
   "Fetch `Revisions` for `Segment` with ID."
   [id]
   (check-superuser)
-  (check-404 (segment/exists-segment? id))
+  (check-404 (segment/exists? id))
   (revision/revisions+details Segment id))
 
 
@@ -66,7 +74,7 @@
   [id :as {{:keys [revision_id]} :body}]
   {revision_id [Required Integer]}
   (check-superuser)
-  (check-404 (segment/exists-segment? id))
+  (check-404 (segment/exists? id))
   (revision/revert!
     :entity      Segment
     :id          id

src/metabase/models/metric.clj

@@ -85,14 +85,13 @@
         (hydrate :creator))))
 
 (defn exists?
-  "Predicate function which checks for a given `Metric` with ID.
-   Returns true if `Metric` exists and is active, false otherwise."
-  [id]
+  "Does an *active* `Metric` with ID exist?"
+  ^Boolean [id]
   {:pre [(integer? id)]}
   (db/exists? Metric :id id, :is_active true))
 
 (defn retrieve-metric
-  "Fetch a single `Metric` by its ID value."
+  "Fetch a single `Metric` by its ID value. Hydrates its `:creator`."
   [id]
   {:pre [(integer? id)]}
   (-> (Metric id)

src/metabase/models/segment.clj

@@ -70,15 +70,14 @@
     (-> (events/publish-event :segment-create segment)
         (hydrate :creator))))
 
-(defn exists-segment?
-  "Predicate function which checks for a given `Segment` with ID.
-   Returns true if `Segment` exists and is active, false otherwise."
-  [id]
+(defn exists?
+  "Does an *active* `Segment` with ID exist?"
+  ^Boolean [id]
   {:pre [(integer? id)]}
   (db/exists? Segment, :id id, :is_active true))
 
 (defn retrieve-segment
-  "Fetch a single `Segment` by its ID value."
+  "Fetch a single `Segment` by its ID value. Hydrates the Segment's `:creator`."
   [id]
   {:pre [(integer? id)]}
   (-> (Segment id)

test/metabase/api/metric_test.clj

@@ -4,11 +4,12 @@
             (metabase [http-client :as http]
                       [middleware :as middleware])
             (metabase.models [database :refer [Database]]
-                             [revision :refer [Revision]]
+                             [hydrate :refer [hydrate]]
                              [metric :refer [Metric], :as metric]
+                             [revision :refer [Revision]]
                              [table :refer [Table]])
-            [metabase.test.data.users :refer :all]
             [metabase.test.data :refer :all]
+            [metabase.test.data.users :refer :all]
             [metabase.test.util :as tu]))
 
 ;; ## Helper Fns
@@ -319,3 +320,18 @@
     [(dissoc ((user->client :crowberto) :post 200 (format "metric/%d/revert" id) {:revision_id revision-id}) :id :timestamp)
      (doall (for [revision ((user->client :crowberto) :get 200 (format "metric/%d/revisions" id))]
               (dissoc revision :timestamp :id)))]))
+
+
+;;; GET /api/metric/
+
+(tu/expect-with-temp [Metric [metric-1]
+                      Metric [metric-2]
+                      Metric [_        {:is_active false}]] ; inactive metrics shouldn't show up
+  (tu/mappify (hydrate [metric-1
+                        metric-2] :creator))
+  ((user->client :crowberto) :get 200 "metric/"))
+
+;; non-admin users shouldn't be allowed to use this endpoint -- should get a 403
+(expect
+  "You don't have permissions to do that."
+  ((user->client :rasta) :get 403 "metric/"))

test/metabase/api/segment_test.clj

@@ -4,6 +4,7 @@
             (metabase [http-client :as http]
                       [middleware :as middleware])
             (metabase.models [database :refer [Database]]
+                             [hydrate :refer [hydrate]]
                              [revision :refer [Revision]]
                              [segment :refer [Segment], :as segment]
                              [table :refer [Table]])
@@ -324,3 +325,18 @@
     [(dissoc ((user->client :crowberto) :post 200 (format "segment/%d/revert" id) {:revision_id revision-id}) :id :timestamp)
      (doall (for [revision ((user->client :crowberto) :get 200 (format "segment/%d/revisions" id))]
               (dissoc revision :timestamp :id)))]))
+
+
+;;; GET /api/segement/
+
+(tu/expect-with-temp [Segment [segment-1]
+                      Segment [segment-2]
+                      Segment [_          {:is_active false}]] ; inactive segments shouldn't show up
+  (tu/mappify (hydrate [segment-1
+                        segment-2] :creator))
+  ((user->client :crowberto) :get 200 "segment/"))
+
+;; non-admin users shouldn't be allowed to use this endpoint -- should get a 403
+(expect
+  "You don't have permissions to do that."
+  ((user->client :rasta) :get 403 "segment/"))

test/metabase/models/segment_test.clj

@@ -2,7 +2,7 @@
   (:require [expectations :refer :all]
             (metabase.models [database :refer [Database]]
                              [hydrate :refer :all]
-                             [segment :refer :all]
+                             [segment :refer :all, :as segment]
                              [table :refer [Table]])
             [metabase.test.data :refer :all]
             [metabase.test.data.users :refer :all]
@@ -41,15 +41,15 @@
     (create-segment-then-select! table-id "I only want *these* things" nil (user->id :rasta) {:clause ["a" "b"]})))
 
 
-;; exists-segment?
+;; exists?
 (expect
   [true
    false]
   (tu/with-temp* [Database [{database-id :id}]
                   Table    [{table-id :id}   {:db_id database-id}]
                   Segment  [{segment-id :id} {:table_id table-id}]]
-    [(exists-segment? segment-id)
-     (exists-segment? 3400)]))
+    [(segment/exists? segment-id)
+     (segment/exists? 3400)]))
 
 
 ;; retrieve-segment

test/metabase/test/util.clj

 (ns metabase.test.util
   "Helper functions and macros for writing unit tests."
-  (:require [cheshire.core :as json]
+  (:require [clojure.walk :as walk]
+            [cheshire.core :as json]
             [expectations :refer :all]
             [metabase.db :as db]
             (metabase.models [card :refer [Card]]
@@ -16,6 +17,7 @@
                              [revision :refer [Revision]]
                              [segment :refer [Segment]]
                              [table :refer [Table]])
+            [metabase.test.data :as data]
             [metabase.util :as u]))
 
 (declare $->prop)
@@ -148,7 +150,8 @@
   {:with-temp-defaults (fn [_] {:creator_id  ((resolve 'metabase.test.data.users/user->id) :rasta)
                                 :definition  {}
                                 :description "Lookin' for a blueberry"
-                                :name        "Toucans in the rainforest"})})
+                                :name        "Toucans in the rainforest"
+                                :table_id    (data/id :venues)})})
 
 (u/strict-extend (class Pulse)
   WithTempDefaults
@@ -183,7 +186,8 @@
   {:with-temp-defaults (fn [_] {:creator_id ((resolve 'metabase.test.data.users/user->id) :rasta)
                                 :definition  {}
                                 :description "Lookin' for a blueberry"
-                                :name        "Toucans in the rainforest"})})
+                                :name        "Toucans in the rainforest"
+                                :table_id    (data/id :venues)})})
 
 (u/strict-extend (class Table)
   WithTempDefaults
@@ -253,8 +257,9 @@
     `(let [~with-temp-form (delay (with-temp* ~with-temp*-form
                                     [~expected ~actual]))]
        (expect
-         (first  @~with-temp-form)
-         (second @~with-temp-form)))))
+         (u/ignore-exceptions
+           (first @~with-temp-form))   ; if dereferencing with-temp-form throws an exception then expect Exception <-> Exception will pass; we don't want that, so make sure the expected
+         (second @~with-temp-form))))) ; case is nil if we encounter an exception so the two don't match and the test doesn't succeed
 
 ;; ## resolve-private-fns
 
@@ -276,9 +281,21 @@
 
 (defn obj->json->obj
   "Convert an object to JSON and back again. This can be done to ensure something will match its serialized + deserialized form,
-   e.g. keywords that aren't map keys:
+   e.g. keywords that aren't map keys, record types vs. plain map types, or timestamps vs ISO-8601 strings:
 
      (obj->json->obj {:type :query}) -> {:type \"query\"}"
   {:style/indent 0}
   [obj]
   (json/parse-string (json/generate-string obj) keyword))
+
+
+(defn mappify
+  "Walk COLL and convert all record types to plain Clojure maps.
+   Useful because expectations will consider an instance of a record type to be different from a plain Clojure map, even if all keys & values are the same."
+  [coll]
+  {:style/indent 0}
+  (walk/postwalk (fn [x]
+                   (if (map? x)
+                     (into {} x)
+                     x))
+                 coll))