From c64ef9096fe960f64dc1ea40c44dddccdbc4d181 Mon Sep 17 00:00:00 2001
From: dpsutton <dan@dpsutton.com>
Date: Mon, 15 Nov 2021 10:42:43 -0600
Subject: [PATCH] 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
---
src/metabase/api/activity.clj | 107 ++++++++++++++++++----------
test/metabase/api/activity_test.clj | 54 +++++++-------
2 files changed, 100 insertions(+), 61 deletions(-)
diff --git a/src/metabase/api/activity.clj b/src/metabase/api/activity.clj
index 3b15658caab..de9aafae95c 100644
--- a/src/metabase/api/activity.clj
+++ b/src/metabase/api/activity.clj
@@ -1,6 +1,7 @@
(ns metabase.api.activity
(:require [clojure.set :as set]
[compojure.core :refer [GET]]
+ [medley.core :as m]
[metabase.api.common :refer [*current-user-id* defendpoint define-routes]]
[metabase.models.activity :refer [Activity]]
[metabase.models.card :refer [Card]]
@@ -36,28 +37,47 @@
(referenced-objects->existing-objects {\"dashboard\" #{41 42 43}, \"card\" #{100 101}, ...})
;; -> {\"dashboard\" #{41 43}, \"card\" #{101}, ...}"
[referenced-objects]
- (into {} (for [[model ids] referenced-objects
- :when (seq ids)]
- {model (case model
- "card" (db/select-ids 'Card, :id [:in ids])
- "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
+ (merge
+ (when-let [card-ids (get referenced-objects "card")]
+ (let [id->dataset? (db/select-id->field :dataset Card
+ :id [:in card-ids])
+ {dataset-ids true card-ids' false} (group-by (comp boolean id->dataset?)
+ ;; only existing ids go back
+ (keys id->dataset?))]
+ (cond-> {}
+ (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
"Add `:model_exists` keys to `activities`, and `:exists` keys to nested dashcards where appropriate."
[activities]
- (let [existing-objects (-> activities activities->referenced-objects referenced-objects->existing-objects)]
- (for [{:keys [model model_id], :as activity} activities]
- (let [activity (assoc activity :model_exists (contains? (get existing-objects model) model_id))]
- (if-not (dashcard-activity? activity)
- activity
- (update-in activity [:details :dashcards] (fn [dashcards]
- (for [dashcard dashcards]
- (assoc dashcard :exists (contains? (get existing-objects "card")
- (:card_id dashcard)))))))))))
+ (let [existing-objects (-> activities activities->referenced-objects referenced-objects->existing-objects)
+ existing-dataset? (fn [card-id]
+ (contains? (get existing-objects "dataset") card-id))]
+ (for [{:keys [model_id], :as activity} activities]
+ (let [model (if (and (= (:model activity) "card")
+ (existing-dataset? (:model_id activity)))
+ "dataset"
+ (:model activity))]
+ (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 "/"
"Get recent activity."
@@ -66,14 +86,25 @@
(hydrate :user :table :database)
add-model-exists-info)))
-(defn- view-log-entry->matching-object [{:keys [model model_id]}]
- (when (contains? #{"card" "dashboard" "table"} model)
- (db/select-one
- (case model
- "card" [Card :id :name :collection_id :description :display :dataset_query]
- "dashboard" [Dashboard :id :name :collection_id :description]
- "table" [Table :id :name :db_id :display_name])
- :id model_id)))
+(defn- models-for-views
+ "Returns a map of {model {id instance}} for activity views suitable for looking up by model and id to get a model."
+ [views]
+ (letfn [(select-items! [model ids]
+ (when (seq ids)
+ (db/select
+ (case model
+ "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"
"Get the list of 10 things the current user has been viewing most recently."
@@ -81,15 +112,19 @@
;; 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
;; 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]]
- :user_id *current-user-id*
- {:group-by [:user_id :model :model_id]
- :order-by [[:max_ts :desc]]
- :limit 5})
- :let [model-object (view-log-entry->matching-object view-log)]
- :when (and model-object
- (mi/can-read? model-object))]
- (assoc view-log :model_object (dissoc model-object :dataset_query))))
-
+ (let [views (db/select [ViewLog :user_id :model :model_id
+ [:%count.* :cnt] [:%max.timestamp :max_ts]]
+ {:group-by [:user_id :model :model_id]
+ :where [:and
+ [:= :user_id *current-user-id*]
+ [:in :model #{"card" "dashboard" "table"}]]
+ :order-by [[:max_ts :desc]]
+ :limit 5})
+ 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)
diff --git a/test/metabase/api/activity_test.clj b/test/metabase/api/activity_test.clj
index 87e3a41cfb0..b7bb26f358b 100644
--- a/test/metabase/api/activity_test.clj
+++ b/test/metabase/api/activity_test.clj
@@ -108,11 +108,12 @@
:description "rand-name"
:creator_id (mt/user->id :crowberto)}]
Table [table1 {:name "rand-name"}]
- Card [card2 {:name "rand-name"
- :creator_id (mt/user->id :crowberto)
- :display "table"
- :visualization_settings {}}]]
- (create-view! (mt/user->id :crowberto) "card" (:id card2))
+ Card [dataset {:name "rand-name"
+ :dataset true
+ :creator_id (mt/user->id :crowberto)
+ :display "table"
+ :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) "card" (:id card1))
(create-view! (mt/user->id :crowberto) "card" 36478)
@@ -133,6 +134,7 @@
:model_object {:id (:id card1)
:name (:name card1)
:collection_id nil
+ :dataset false
:description (:description card1)
:display (name (:display card1))}}
{:cnt 1
@@ -145,13 +147,14 @@
:description (:description dash1)}}
{:cnt 1
:user_id (mt/user->id :crowberto)
- :model "card"
- :model_id (:id card2)
- :model_object {:id (:id card2)
- :name (:name card2)
+ :model "dataset"
+ :model_id (:id dataset)
+ :model_object {:id (:id dataset)
+ :name (:name dataset)
+ :dataset true
:collection_id nil
- :description (:description card2)
- :display (name (:display card2))}}]
+ :description (:description dataset)
+ :display (name (:display dataset))}}]
(for [recent-view (mt/user-http-request :crowberto :get 200 "activity/recent_views")]
(dissoc recent-view :max_ts))))))
@@ -181,47 +184,48 @@
(deftest referenced-objects->existing-objects-test
(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}
"card" #{0}})))))
(deftest add-model-exists-info-test
(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}
{:model "card", :model_id 0, :model_exists false}
+ {:model "dataset", :model_id dataset-id, :model_exists true}
{:model "dashboard"
:model_id 0
:model_exists false
:topic :dashboard-remove-cards
: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}
{:model "card", :model_id 0}
+ {:model "card", :model_id dataset-id}
{:model "dashboard"
:model_id 0
:topic :dashboard-remove-cards
:details {:dashcards [{:card_id card-id}
- {:card_id 0}]}}])))))
+ {:card_id 0}
+ {:card_id dataset-id}]}}])))))
(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"
:details {}
- :timestamp #t "2019-02-15T11:55:00.000Z"}]
- (letfn [(user-can-see-user-joined-activity? [user]
- (seq (mt/user-http-request user :get 200 "activity")))]
+ :timestamp (java.time.ZonedDateTime/now)}]
+ (letfn [(activity-topics [user]
+ (into #{} (map :topic)
+ (mt/user-http-request user :get 200 "activity")))]
(testing "Only admins should get to see user-joined activities"
(testing "admin should see `:user-joined` activities"
(testing "Sanity check: admin should be able to read the activity"
(mt/with-test-user :crowberto
(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 "Sanity check: non-admin should *not* be able to read the activity"
(mt/with-test-user :rasta
(is (not (models/can-read? activity)))))
- (is (not (user-can-see-user-joined-activity? :rasta))))))))
+ (is (not (contains? (activity-topics :rasta) "user-joined"))))))))
--
GitLab