Merge pull request #2886 from metabase/new-metric-segment-endpoints

Endpoints for listing all segments/metrics 

.dir-locals.el

@@ -24,7 +24,6 @@
                               (execute-sql! 2)
                               (expect 0)
                               (expect-expansion 0)
-                              (expect-let 1)
                               (expect-when-testing-engine 1)
                               (expect-with-all-engines 0)
                               (expect-with-engine 1)

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,12 @@
   (check-superuser)
   (check-404 (metric/retrieve-metric id)))
 
+(defendpoint GET "/"
+  "Fetch *all* `Metrics`."
+  [id]
+  (-> (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,12 @@
   (check-superuser)
   (check-404 (segment/retrieve-segment id)))
 
+(defendpoint GET "/"
+  "Fetch *all* `Segments`."
+  []
+  (-> (db/select Segment, :is_active true)
+      (hydrate :creator)))
+
 
 (defendpoint PUT "/:id"
   "Update a `Segment` with ID."
@@ -33,7 +40,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 +55,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 +64,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 +73,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)

src/metabase/models/table.clj

@@ -26,7 +26,8 @@
 (defn- pre-cascade-delete [{:keys [id]}]
   (db/cascade-delete! Segment :table_id id)
   (db/cascade-delete! Metric :table_id id)
-  (db/cascade-delete! Field :table_id id))
+  (db/cascade-delete! Field :table_id id)
+  (db/cascade-delete! 'Card :table_id id))
 
 (defn ^:hydrate fields
   "Return the `FIELDS` belonging to TABLE."

test/metabase/api/card_test.clj

@@ -13,7 +13,8 @@
                              [view-log :refer [ViewLog]])
             [metabase.test.data :refer :all]
             [metabase.test.data.users :refer :all]
-            [metabase.test.util :refer [match-$ expect-eval-actual-first random-name with-temp with-temp* obj->json->obj expect-with-temp]]))
+            [metabase.test.util :refer [match-$ expect-eval-actual-first random-name with-temp with-temp* obj->json->obj expect-with-temp]]
+            [metabase.util :as u]))
 
 ;; # CARD LIFECYCLE
 
@@ -75,11 +76,11 @@
                    Card     [{card-3-id :id}]
                    Card     [{card-4-id :id}]
                    ;; 3 was viewed most recently, followed by 4, then 1. Card 2 was viewed by a different user so shouldn't be returned
-                   ViewLog  [_               {:model "card", :model_id card-1-id, :user_id (user->id :rasta),     :timestamp #inst "2015-12-01"}]
-                   ViewLog  [_               {:model "card", :model_id card-2-id, :user_id (user->id :trashbird), :timestamp #inst "2016-01-01"}]
-                   ViewLog  [_               {:model "card", :model_id card-3-id, :user_id (user->id :rasta),     :timestamp #inst "2016-02-01"}]
-                   ViewLog  [_               {:model "card", :model_id card-4-id, :user_id (user->id :rasta),     :timestamp #inst "2016-03-01"}]
-                   ViewLog  [_               {:model "card", :model_id card-3-id, :user_id (user->id :rasta),     :timestamp #inst "2016-04-01"}]]
+                   ViewLog  [_               {:model "card", :model_id card-1-id, :user_id (user->id :rasta),     :timestamp (u/->Timestamp #inst "2015-12-01")}]
+                   ViewLog  [_               {:model "card", :model_id card-2-id, :user_id (user->id :trashbird), :timestamp (u/->Timestamp #inst "2016-01-01")}]
+                   ViewLog  [_               {:model "card", :model_id card-3-id, :user_id (user->id :rasta),     :timestamp (u/->Timestamp #inst "2016-02-01")}]
+                   ViewLog  [_               {:model "card", :model_id card-4-id, :user_id (user->id :rasta),     :timestamp (u/->Timestamp #inst "2016-03-01")}]
+                   ViewLog  [_               {:model "card", :model_id card-3-id, :user_id (user->id :rasta),     :timestamp (u/->Timestamp #inst "2016-04-01")}]]
   [card-3-id card-4-id card-1-id]
   (mapv :id ((user->client :rasta) :get 200 "card", :f :recent)))
 

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
@@ -46,8 +47,8 @@
 ;; test security.  requires superuser perms
 (expect "You don't have permissions to do that."
   ((user->client :rasta) :post 403 "metric" {:name       "abc"
-                                              :table_id   123
-                                              :definition {}}))
+                                             :table_id   123
+                                             :definition {}}))
 
 ;; test validations
 (expect {:errors {:name "field is a required param."}}
@@ -58,16 +59,16 @@
 
 (expect {:errors {:table_id "Invalid value 'foobar' for 'table_id': value must be an integer."}}
   ((user->client :crowberto) :post 400 "metric" {:name     "abc"
-                                                  :table_id "foobar"}))
+                                                 :table_id "foobar"}))
 
 (expect {:errors {:definition "field is a required param."}}
   ((user->client :crowberto) :post 400 "metric" {:name     "abc"
-                                                  :table_id 123}))
+                                                 :table_id 123}))
 
 (expect {:errors {:definition "Invalid value 'foobar' for 'definition': value must be a dictionary."}}
   ((user->client :crowberto) :post 400 "metric" {:name       "abc"
-                                                  :table_id   123
-                                                  :definition "foobar"}))
+                                                 :table_id   123
+                                                 :definition "foobar"}))
 
 (expect
   {:name         "A Metric"
@@ -93,8 +94,8 @@
 ;; test security.  requires superuser perms
 (expect "You don't have permissions to do that."
   ((user->client :rasta) :put 403 "metric/1" {:name             "abc"
-                                               :definition       {}
-                                               :revision_message "something different"}))
+                                              :definition       {}
+                                              :revision_message "something different"}))
 
 ;; test validations
 (expect {:errors {:name "field is a required param."}}
@@ -319,3 +320,13 @@
     [(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 :rasta) :get 200 "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,13 @@
     [(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 :rasta) :get 200 "segment/"))

test/metabase/events/view_log_test.clj

@@ -2,57 +2,44 @@
   (:require [expectations :refer :all]
             [metabase.db :as db]
             [metabase.events.view-log :refer :all]
-            (metabase.models [user :refer [User]]
+            (metabase.models [card :refer [Card]]
+                             [dashboard :refer [Dashboard]]
+                             [user :refer [User]]
                              [view-log :refer [ViewLog]])
             [metabase.test.data :refer :all]
-            [metabase.test.util :refer [expect-eval-actual-first random-name]]
+            [metabase.test.util :as tu]
             [metabase.test-setup :refer :all]))
 
 
-(defn- create-test-user []
-  (let [rand-name (random-name)]
-    (db/insert! User
-      :email      (str rand-name "@metabase.com")
-      :first_name rand-name
-      :last_name  rand-name
-      :password   rand-name)))
-
-
 ;; `:card-create` event
-(expect-let [{user-id :id} (create-test-user)
-             card          {:id 1234
-                            :creator_id user-id}]
-  {:user_id     user-id
-   :model       "card"
-   :model_id    (:id card)}
+(tu/expect-with-temp [User [user]
+                      Card [card {:creator_id (:id user)}]]
+  {:user_id  (:id user)
+   :model    "card"
+   :model_id (:id card)}
   (do
     (process-view-count-event {:topic :card-create
                                :item  card})
-    (-> (ViewLog :user_id user-id)
-        (select-keys [:user_id :model :model_id]))))
+    (db/select-one [ViewLog :user_id :model :model_id], :user_id (:id user))))
 
 ;; `:card-read` event
-(expect-let [{user-id :id} (create-test-user)
-             card          {:id 1234
-                            :actor_id user-id}]
-  {:user_id     user-id
+(tu/expect-with-temp [User [user]
+                      Card [card {:creator_id (:id user)}]]
+  {:user_id     (:id user)
    :model       "card"
    :model_id    (:id card)}
   (do
     (process-view-count-event {:topic :card-read
                                :item  card})
-    (-> (ViewLog :user_id user-id)
-        (select-keys [:user_id :model :model_id]))))
+    (db/select-one [ViewLog :user_id :model :model_id], :user_id (:id user))))
 
 ;; `:dashboard-read` event
-(expect-let [{user-id :id} (create-test-user)
-             dashboard     {:id 1234
-                            :actor_id user-id}]
-  {:user_id     user-id
+(tu/expect-with-temp [User      [user]
+                      Dashboard [dashboard {:creator_id (:id user)}]]
+  {:user_id     (:id user)
    :model       "dashboard"
    :model_id    (:id dashboard)}
   (do
     (process-view-count-event {:topic :dashboard-read
                                :item  dashboard})
-    (-> (ViewLog :user_id user-id)
-        (select-keys [:user_id :model :model_id]))))
+    (db/select-one [ViewLog :user_id :model :model_id], :user_id (:id user))))

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]]
@@ -15,7 +16,9 @@
                              [raw-table :refer [RawTable]]
                              [revision :refer [Revision]]
                              [segment :refer [Segment]]
-                             [table :refer [Table]])
+                             [table :refer [Table]]
+                             [user :refer [User]])
+            [metabase.test.data :as data]
             [metabase.util :as u]))
 
 (declare $->prop)
@@ -110,11 +113,15 @@
 
 (u/strict-extend Object
   WithTempDefaults
-  {:with-temp-defaults (constantly nil)})
+  {:with-temp-defaults (constantly {})})
+
+(defn- rasta-id []
+  (require 'metabase.test.data.users)
+  ((resolve 'metabase.test.data.users/user->id) :rasta))
 
 (u/strict-extend (class Card)
   WithTempDefaults
-  {:with-temp-defaults (fn [_] {:creator_id             ((resolve 'metabase.test.data.users/user->id) :rasta)
+  {:with-temp-defaults (fn [_] {:creator_id             (rasta-id)
                                 :dataset_query          {}
                                 :display                :table
                                 :name                   (random-name)
@@ -123,7 +130,7 @@
 
 (u/strict-extend (class Dashboard)
   WithTempDefaults
-  {:with-temp-defaults (fn [_] {:creator_id   ((resolve 'metabase.test.data.users/user->id) :rasta)
+  {:with-temp-defaults (fn [_] {:creator_id   (rasta-id)
                                 :name         (random-name)
                                 :public_perms 0})})
 
@@ -145,14 +152,15 @@
 
 (u/strict-extend (class Metric)
   WithTempDefaults
-  {:with-temp-defaults (fn [_] {:creator_id  ((resolve 'metabase.test.data.users/user->id) :rasta)
+  {:with-temp-defaults (fn [_] {:creator_id  (rasta-id)
                                 :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
-  {:with-temp-defaults (fn [_] {:creator_id ((resolve 'metabase.test.data.users/user->id) :rasta)
+  {:with-temp-defaults (fn [_] {:creator_id (rasta-id)
                                 :name       (random-name)})})
 
 (u/strict-extend (class PulseChannel)
@@ -174,22 +182,32 @@
 
 (u/strict-extend (class Revision)
   WithTempDefaults
-  {:with-temp-defaults (fn [_] {:user_id      ((resolve 'metabase.test.data.users/user->id) :rasta)
+  {:with-temp-defaults (fn [_] {:user_id      (rasta-id)
                                 :is_creation  false
                                 :is_reversion false})})
 
 (u/strict-extend (class Segment)
   WithTempDefaults
-  {:with-temp-defaults (fn [_] {:creator_id ((resolve 'metabase.test.data.users/user->id) :rasta)
+  {:with-temp-defaults (fn [_] {:creator_id (rasta-id)
                                 :definition  {}
                                 :description "Lookin' for a blueberry"
-                                :name        "Toucans in the rainforest"})})
+                                :name        "Toucans in the rainforest"
+                                :table_id    (data/id :venues)})})
+
+;; TODO - `with-temp` doesn't return `Sessions`, probably because their ID is a string?
 
 (u/strict-extend (class Table)
   WithTempDefaults
   {:with-temp-defaults (fn [_] {:active true
                                 :name   (random-name)})})
 
+(u/strict-extend (class User)
+  WithTempDefaults
+  {:with-temp-defaults (fn [_] {:email      (str (random-name) "@metabase.com")
+                                :password   (random-name)
+                                :first_name (random-name)
+                                :last_name  (random-name)})})
+
 
 (defn do-with-temp
   "Internal implementation of `with-temp` (don't call this directly)."
@@ -253,8 +271,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 +295,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))