Skip to content
Snippets Groups Projects
Unverified Commit c64ef909 authored by dpsutton's avatar dpsutton Committed by GitHub
Browse files

Dataset recent activity (#18967) 

* Get rid of the n-queries

old logic: select last view logs, and then for each one, select its
associated model (card, table, etc).

new logic: get the last view logs, and do three batch queries, one each
to card, dashboard, table, for the ids from the view logs

* Mark datasets in api/activity/recent_views

* Get rid of `(db/delete! Activity)` in tests

* Handle datasets in activity list
parent 0a8eaab3
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/api/activity.clj
+ 71
− 36
View file @ c64ef909
(ns metabase.api.activity (ns metabase.api.activity
(:require [clojure.set :as set] (:require [clojure.set :as set]
[compojure.core :refer [GET]] [compojure.core :refer [GET]]
[medley.core :as m]
[metabase.api.common :refer [*current-user-id* defendpoint define-routes]] [metabase.api.common :refer [*current-user-id* defendpoint define-routes]]
[metabase.models.activity :refer [Activity]] [metabase.models.activity :refer [Activity]]
[metabase.models.card :refer [Card]] [metabase.models.card :refer [Card]]
...@@ -36,28 +37,47 @@ ...@@ -36,28 +37,47 @@
(referenced-objects->existing-objects {\"dashboard\" #{41 42 43}, \"card\" #{100 101}, ...}) (referenced-objects->existing-objects {\"dashboard\" #{41 42 43}, \"card\" #{100 101}, ...})
;; -> {\"dashboard\" #{41 43}, \"card\" #{101}, ...}" ;; -> {\"dashboard\" #{41 43}, \"card\" #{101}, ...}"
[referenced-objects] [referenced-objects]
(into {} (for [[model ids] referenced-objects (merge
:when (seq ids)] (when-let [card-ids (get referenced-objects "card")]
{model (case model (let [id->dataset? (db/select-id->field :dataset Card
"card" (db/select-ids 'Card, :id [:in ids]) :id [:in card-ids])
"dashboard" (db/select-ids 'Dashboard, :id [:in ids]) {dataset-ids true card-ids' false} (group-by (comp boolean id->dataset?)
"metric" (db/select-ids 'Metric, :id [:in ids], :archived false) ;; only existing ids go back
"pulse" (db/select-ids 'Pulse, :id [:in ids]) (keys id->dataset?))]
"segment" (db/select-ids 'Segment, :id [:in ids], :archived false) (cond-> {}
nil)}))) ; don't care about other models (seq dataset-ids) (assoc "dataset" (set dataset-ids))
(seq card-ids') (assoc "card" (set card-ids')))))
(into {} (for [[model ids] (dissoc referenced-objects "card")
:when (seq ids)]
{model (case model
"dashboard" (db/select-ids 'Dashboard, :id [:in ids])
"metric" (db/select-ids 'Metric, :id [:in ids], :archived false)
"pulse" (db/select-ids 'Pulse, :id [:in ids])
"segment" (db/select-ids 'Segment, :id [:in ids], :archived false)
nil)})))) ; don't care about other models
(defn- add-model-exists-info (defn- add-model-exists-info
"Add `:model_exists` keys to `activities`, and `:exists` keys to nested dashcards where appropriate." "Add `:model_exists` keys to `activities`, and `:exists` keys to nested dashcards where appropriate."
[activities] [activities]
(let [existing-objects (-> activities activities->referenced-objects referenced-objects->existing-objects)] (let [existing-objects (-> activities activities->referenced-objects referenced-objects->existing-objects)
(for [{:keys [model model_id], :as activity} activities] existing-dataset? (fn [card-id]
(let [activity (assoc activity :model_exists (contains? (get existing-objects model) model_id))] (contains? (get existing-objects "dataset") card-id))]
(if-not (dashcard-activity? activity) (for [{:keys [model_id], :as activity} activities]
activity (let [model (if (and (= (:model activity) "card")
(update-in activity [:details :dashcards] (fn [dashcards] (existing-dataset? (:model_id activity)))
(for [dashcard dashcards] "dataset"
(assoc dashcard :exists (contains? (get existing-objects "card") (:model activity))]
(:card_id dashcard))))))))))) (cond-> (assoc activity
:model_exists (contains? (get existing-objects model) model_id)
:model model)
(dashcard-activity? activity)
(update-in [:details :dashcards]
(fn [dashcards]
(for [dashcard dashcards]
(assoc dashcard :exists
(or (existing-dataset? (:card_id dashcard))
(contains? (get existing-objects "card")
(:card_id dashcard))))))))))))
(defendpoint GET "/" (defendpoint GET "/"
"Get recent activity." "Get recent activity."
...@@ -66,14 +86,25 @@ ...@@ -66,14 +86,25 @@
(hydrate :user :table :database) (hydrate :user :table :database)
add-model-exists-info))) add-model-exists-info)))
(defn- view-log-entry->matching-object [{:keys [model model_id]}] (defn- models-for-views
(when (contains? #{"card" "dashboard" "table"} model) "Returns a map of {model {id instance}} for activity views suitable for looking up by model and id to get a model."
(db/select-one [views]
(case model (letfn [(select-items! [model ids]
"card" [Card :id :name :collection_id :description :display :dataset_query] (when (seq ids)
"dashboard" [Dashboard :id :name :collection_id :description] (db/select
"table" [Table :id :name :db_id :display_name]) (case model
:id model_id))) "card" [Card :id :name :collection_id :description :display
:dataset_query :dataset]
"dashboard" [Dashboard :id :name :collection_id :description]
"table" [Table :id :name :db_id :display_name])
{:where [:in :id ids]})))
(by-id [models] (m/index-by :id models))]
(into {} (map (fn [[model models]]
[model (->> models
(map :model_id)
(select-items! model)
(by-id))]))
(group-by :model views))))
(defendpoint GET "/recent_views" (defendpoint GET "/recent_views"
"Get the list of 10 things the current user has been viewing most recently." "Get the list of 10 things the current user has been viewing most recently."
...@@ -81,15 +112,19 @@ ...@@ -81,15 +112,19 @@
;; expected output of the query is a single row per unique model viewed by the current user ;; expected output of the query is a single row per unique model viewed by the current user
;; including a `:max_ts` which has the most recent view timestamp of the item and `:cnt` which has total views ;; including a `:max_ts` which has the most recent view timestamp of the item and `:cnt` which has total views
;; and we order the results by most recently viewed then hydrate the basic details of the model ;; and we order the results by most recently viewed then hydrate the basic details of the model
(for [view-log (db/select [ViewLog :user_id :model :model_id [:%count.* :cnt] [:%max.timestamp :max_ts]] (let [views (db/select [ViewLog :user_id :model :model_id
:user_id *current-user-id* [:%count.* :cnt] [:%max.timestamp :max_ts]]
{:group-by [:user_id :model :model_id] {:group-by [:user_id :model :model_id]
:order-by [[:max_ts :desc]] :where [:and
:limit 5}) [:= :user_id *current-user-id*]
:let [model-object (view-log-entry->matching-object view-log)] [:in :model #{"card" "dashboard" "table"}]]
:when (and model-object :order-by [[:max_ts :desc]]
(mi/can-read? model-object))] :limit 5})
(assoc view-log :model_object (dissoc model-object :dataset_query)))) model->id->items (models-for-views views)]
(for [{:keys [model model_id] :as view-log} views
:let [model-object (get-in model->id->items [model model_id])]
:when (and model-object (mi/can-read? model-object))]
(cond-> (assoc view-log :model_object (dissoc model-object :dataset_query))
(:dataset model-object) (assoc :model "dataset")))))
(define-routes) (define-routes)
test/metabase/api/activity_test.clj
+ 29
− 25
View file @ c64ef909
...@@ -108,11 +108,12 @@ ...@@ -108,11 +108,12 @@
:description "rand-name" :description "rand-name"
:creator_id (mt/user->id :crowberto)}] :creator_id (mt/user->id :crowberto)}]
Table [table1 {:name "rand-name"}] Table [table1 {:name "rand-name"}]
Card [card2 {:name "rand-name" Card [dataset {:name "rand-name"
:creator_id (mt/user->id :crowberto) :dataset true
:display "table" :creator_id (mt/user->id :crowberto)
:visualization_settings {}}]] :display "table"
(create-view! (mt/user->id :crowberto) "card" (:id card2)) :visualization_settings {}}]]
(create-view! (mt/user->id :crowberto) "card" (:id dataset))
(create-view! (mt/user->id :crowberto) "dashboard" (:id dash1)) (create-view! (mt/user->id :crowberto) "dashboard" (:id dash1))
(create-view! (mt/user->id :crowberto) "card" (:id card1)) (create-view! (mt/user->id :crowberto) "card" (:id card1))
(create-view! (mt/user->id :crowberto) "card" 36478) (create-view! (mt/user->id :crowberto) "card" 36478)
...@@ -133,6 +134,7 @@ ...@@ -133,6 +134,7 @@
:model_object {:id (:id card1) :model_object {:id (:id card1)
:name (:name card1) :name (:name card1)
:collection_id nil :collection_id nil
:dataset false
:description (:description card1) :description (:description card1)
:display (name (:display card1))}} :display (name (:display card1))}}
{:cnt 1 {:cnt 1
...@@ -145,13 +147,14 @@ ...@@ -145,13 +147,14 @@
:description (:description dash1)}} :description (:description dash1)}}
{:cnt 1 {:cnt 1
:user_id (mt/user->id :crowberto) :user_id (mt/user->id :crowberto)
:model "card" :model "dataset"
:model_id (:id card2) :model_id (:id dataset)
:model_object {:id (:id card2) :model_object {:id (:id dataset)
:name (:name card2) :name (:name dataset)
:dataset true
:collection_id nil :collection_id nil
:description (:description card2) :description (:description dataset)
:display (name (:display card2))}}] :display (name (:display dataset))}}]
(for [recent-view (mt/user-http-request :crowberto :get 200 "activity/recent_views")] (for [recent-view (mt/user-http-request :crowberto :get 200 "activity/recent_views")]
(dissoc recent-view :max_ts)))))) (dissoc recent-view :max_ts))))))
...@@ -181,47 +184,48 @@ ...@@ -181,47 +184,48 @@
(deftest referenced-objects->existing-objects-test (deftest referenced-objects->existing-objects-test
(mt/with-temp Dashboard [{dashboard-id :id}] (mt/with-temp Dashboard [{dashboard-id :id}]
(is (= {"dashboard" #{dashboard-id}, "card" nil} (is (= {"dashboard" #{dashboard-id}}
(#'activity-api/referenced-objects->existing-objects {"dashboard" #{dashboard-id 0} (#'activity-api/referenced-objects->existing-objects {"dashboard" #{dashboard-id 0}
"card" #{0}}))))) "card" #{0}})))))
(deftest add-model-exists-info-test (deftest add-model-exists-info-test
(mt/with-temp* [Dashboard [{dashboard-id :id}] (mt/with-temp* [Dashboard [{dashboard-id :id}]
Card [{card-id :id}]] Card [{card-id :id}]
Card [{dataset-id :id} {:dataset true}]]
(is (= [{:model "dashboard", :model_id dashboard-id, :model_exists true} (is (= [{:model "dashboard", :model_id dashboard-id, :model_exists true}
{:model "card", :model_id 0, :model_exists false} {:model "card", :model_id 0, :model_exists false}
{:model "dataset", :model_id dataset-id, :model_exists true}
{:model "dashboard" {:model "dashboard"
:model_id 0 :model_id 0
:model_exists false :model_exists false
:topic :dashboard-remove-cards :topic :dashboard-remove-cards
:details {:dashcards [{:card_id card-id, :exists true} :details {:dashcards [{:card_id card-id, :exists true}
{:card_id 0, :exists false}]}}] {:card_id 0, :exists false}
{:card_id dataset-id, :exists true}]}}]
(#'activity-api/add-model-exists-info [{:model "dashboard", :model_id dashboard-id} (#'activity-api/add-model-exists-info [{:model "dashboard", :model_id dashboard-id}
{:model "card", :model_id 0} {:model "card", :model_id 0}
{:model "card", :model_id dataset-id}
{:model "dashboard" {:model "dashboard"
:model_id 0 :model_id 0
:topic :dashboard-remove-cards :topic :dashboard-remove-cards
:details {:dashcards [{:card_id card-id} :details {:dashcards [{:card_id card-id}
{:card_id 0}]}}]))))) {:card_id 0}
{:card_id dataset-id}]}}])))))
(deftest activity-visibility-test (deftest activity-visibility-test
;; clear out all existing Activity entries
;;
;; TODO -- this is a bad pattern -- test shouldn't wipe out a bunch of other stuff like this. Just fetch all the
;; activities and look for the presence of this specific one.
(db/delete! Activity)
(mt/with-temp Activity [activity {:topic "user-joined" (mt/with-temp Activity [activity {:topic "user-joined"
:details {} :details {}
:timestamp #t "2019-02-15T11:55:00.000Z"}] :timestamp (java.time.ZonedDateTime/now)}]
(letfn [(user-can-see-user-joined-activity? [user] (letfn [(activity-topics [user]
(seq (mt/user-http-request user :get 200 "activity")))] (into #{} (map :topic)
(mt/user-http-request user :get 200 "activity")))]
(testing "Only admins should get to see user-joined activities" (testing "Only admins should get to see user-joined activities"
(testing "admin should see `:user-joined` activities" (testing "admin should see `:user-joined` activities"
(testing "Sanity check: admin should be able to read the activity" (testing "Sanity check: admin should be able to read the activity"
(mt/with-test-user :crowberto (mt/with-test-user :crowberto
(is (models/can-read? activity)))) (is (models/can-read? activity))))
(is (user-can-see-user-joined-activity? :crowberto))) (is (contains? (activity-topics :crowberto) "user-joined")))
(testing "non-admin should *not* see `:user-joined` activities" (testing "non-admin should *not* see `:user-joined` activities"
(testing "Sanity check: non-admin should *not* be able to read the activity" (testing "Sanity check: non-admin should *not* be able to read the activity"
(mt/with-test-user :rasta (mt/with-test-user :rasta
(is (not (models/can-read? activity))))) (is (not (models/can-read? activity)))))
(is (not (user-can-see-user-joined-activity? :rasta)))))))) (is (not (contains? (activity-topics :rasta) "user-joined"))))))))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment