Skip to content
Snippets Groups Projects
Unverified Commit 2b8e282e authored by adam-james's avatar adam-james Committed by GitHub
Browse files

recent_views stored in user local setting (#29631) 

* First pass at recent_views stored in user local setting

This modifies the view log event handler to write a user's views to their local settings map (stored as json in the
appdb).

some notes on what causes a view

:card-query
 - occurs when collection with pinned cards/models is opened. maybe shouldn't count as a view

:card-read
 - happens when you view a card, like, load it in the UI and are looking at it

:card-read AND :card-query
 - happens when you view a model, like, load it in the UI and are looking at it

:dashboard-read
 - happens when you view a dashboard

:card-create
 - when you save a card
 - this will probably cause double counts because it will save and then trigger a :card-read right away too

:table-read
 - when viewing a table.
 - when selecting a db/table in dropdowns in the Question editor

I think we need the view log topics to be much smaller: #{:card-read :dashboard-read :table-read}

* WIP test for the recent_views user local setting.

This isn't totally correct yet. I think it doesn't clean up after itself correctly. That's pending.

And, I have to investigate because :table-read isn't working either. Still worth pushing this progress!

* Test that works! All potential view events need :actor_id

* Working test for /api/recent_views

* Order of views in user-local recent-views works now

If a user views something they've recently viewed, the homepage order doesn't change. Now it will, as in, the most
recently viewed item will always be the first item in the vector.

* One time recent_views expensive query done in the user-recent-views getter

The logic of filling the recent-views setting was first written in the activity api namespace. Now it's handled where
the setting is defined, which lets anyone else consuming the setting not worry about filling it if its empty.

The setting can be reset with `nil` which will cause the query to run again. Otherwise, if the list of recent items is
an empty vector, that is considered valid and will not re-run the query.

Updating the setting per view is handled still in the view log handler, implemented in the private function `update-users-recent-views!`.

* Add `most-recently-viewed-dashboard` setting

The most recently viewed dashboard for that user, in the past 24 hours.

This is not meant to be a permanent solution to providing this bit of info for the user, rather, it's a 'stop gap'
while the audit tables are reworked.

* Add test for most recent dashboard setting

* Pass :context to the view log fns

This lets us filter out views triggered by pinned cards.

* Fix tests failing after adding :context to view metadata

* See if card-query can help us filter out pinned item views

* Tests should send :card-query events

* Since we use :card-query, we have to filter out dashboard context too

This should prevent views being recorded if someone just opens a dashboard.

* ViewLog Cleanup to see if it'll let other tests pass
parent 72ac5858
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 266 additions and 132 deletions
src/metabase/api/activity.clj
+ 11
− 74
View file @ 2b8e282e
......@@ -3,10 +3,9 @@
[clojure.set :as set]
[clojure.string :as str]
[compojure.core :refer [GET]]
[java-time :as t]
[medley.core :as m]
[metabase.api.common :as api :refer [*current-user-id* define-routes]]
[metabase.db.connection :as mdb.connection]
[metabase.events.view-log :as view-log]
[metabase.models.activity :refer [Activity]]
[metabase.models.card :refer [Card]]
[metabase.models.dashboard :refer [Dashboard]]
......@@ -191,83 +190,21 @@
(def ^:private views-limit 8)
(def ^:private card-runs-limit 8)
(defn- bookmarks-query
[user-id]
(let [as-null (when (= (mdb.connection/db-type) :postgres) (h2x/->integer nil))]
{:select [[:type :model] [:item_id :model_id]]
:from [[{:union-all [{:select [:card_id
[as-null :dashboard_id]
[as-null :collection_id]
[:card_id :item_id]
[(h2x/literal "card") :type]
:created_at]
:from [:card_bookmark]
:where [:= :user_id [:inline user-id]]}
{:select [[as-null :card_id]
:dashboard_id
[as-null :collection_id]
[:dashboard_id :item_id]
[(h2x/literal "dashboard") :type]
:created_at]
:from [:dashboard_bookmark]
:where [:= :user_id [:inline user-id]]}]}
:bookmarks]]}))
(defn- recent-views-for-user
[user-id]
(let [bookmarks (bookmarks-query user-id)
qe {:select [[(h2x/literal "qe") :source]
[:executor_id :user_id]
:context
[:started_at :timestamp]
[(h2x/literal "card") :model]
[:card_id :model_id]
[false :dataset]]
:from :query_execution}
vl {:select [[(h2x/literal "vl") :source]
:user_id
[(h2x/literal "question") :context]
:timestamp
:model
:model_id
[:report_card.dataset :dataset]]
:from [:view_log]
:left-join [:report_card
[:and
[:= :view_log.model (h2x/literal "card")]
[:= :view_log.model_id :report_card.id]]]}
views {:union-all [qe vl]}]
(t2/query
{:select [[[:max :timestamp] :timestamp]
:model
:model_id]
:from [[views :views]]
:where [[:and
[:= :user_id [:inline user-id]]
[:>= :timestamp (t/minus (t/offset-date-time) (t/days 30))]
[:not= :context (h2x/literal "pulse")]
[:not= :context (h2x/literal "collection")]
[:not= :context (h2x/literal "ad-hoc")]
[:not= [:composite :context :model] [:composite (h2x/literal "dashboard") (h2x/literal "card")]]
[:not= [:composite :source :model :dataset] [:composite (h2x/literal "vl") (h2x/literal "card") [:inline false]]]
[:not-in [:composite :model :model_id] bookmarks]]]
:group-by [:model :model_id]
:order-by [[:timestamp :desc]]
:limit [:inline 8]})))
(api/defendpoint GET "/recent_views"
"Get a list of 5 things the current user has been viewing most recently."
[]
(let [views (recent-views-for-user *current-user-id*)
(let [views (view-log/user-recent-views)
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])
(dissoc :dataset_query))]
:when (and model-object
(mi/can-read? model-object)
;; hidden tables, archived cards/dashboards
(not (or (:archived model-object)
(= (:visibility_type model-object) :hidden))))]
:let
[model-object (-> (get-in model->id->items [model model_id])
(dissoc :dataset_query))]
:when
(and model-object
(mi/can-read? model-object)
;; hidden tables, archived cards/dashboards
(not (or (:archived model-object)
(= (:visibility_type model-object) :hidden))))]
(cond-> (assoc view-log :model_object model-object)
(:dataset model-object) (assoc :model "dataset")))
(take 5))))
......
src/metabase/events.clj
+ 6
− 1
View file @ 2b8e282e
......@@ -133,4 +133,9 @@
Expand the object when we need more metadata."
[object]
{:cached (:cached object)
:ignore_cache (:ignore_cache object)})
:ignore_cache (:ignore_cache object)
;; the :context key comes from qp middleware:
;; `metabase.query-processor.middleware.process-userland-query/add-and-save-execution-info-xform!`
;; and is important for distinguishing view events triggered when pinned cards are 'viewed'
;; when a user opens a collection.
:context (:context object)})
src/metabase/events/view_log.clj
+ 121
− 6
View file @ 2b8e282e
(ns metabase.events.view-log
(:require
[clojure.core.async :as a]
[java-time :as t]
[metabase.api.common :as api]
[metabase.db.connection :as mdb.connection]
[metabase.events :as events]
[metabase.models.setting :as setting :refer [defsetting]]
[metabase.models.view-log :refer [ViewLog]]
[metabase.server.middleware.session :as mw.session]
[metabase.util.honey-sql-2 :as h2x]
[metabase.util.i18n :as i18n :refer [deferred-tru]]
[metabase.util.log :as log]
[toucan2.core :as t2]))
......@@ -19,6 +26,101 @@
(a/chan))
;;; ## ---------------------------------------- PER-USER VIEWS ----------------------------------------
(defn- bookmarks-query
[user-id]
(let [as-null (when (= (mdb.connection/db-type) :postgres) (h2x/->integer nil))]
{:select [[:type :model] [:item_id :model_id]]
:from [[{:union-all [{:select [:card_id
[as-null :dashboard_id]
[as-null :collection_id]
[:card_id :item_id]
[(h2x/literal "card") :type]
:created_at]
:from [:card_bookmark]
:where [:= :user_id [:inline user-id]]}
{:select [[as-null :card_id]
:dashboard_id
[as-null :collection_id]
[:dashboard_id :item_id]
[(h2x/literal "dashboard") :type]
:created_at]
:from [:dashboard_bookmark]
:where [:= :user_id [:inline user-id]]}]}
:bookmarks]]}))
(defn- recent-views-from-view-log
[user-id]
(let [bookmarks (bookmarks-query user-id)
qe {:select [[(h2x/literal "qe") :source]
[:executor_id :user_id]
:context
[:started_at :timestamp]
[(h2x/literal "card") :model]
[:card_id :model_id]
[false :dataset]]
:from :query_execution}
vl {:select [[(h2x/literal "vl") :source]
:user_id
[(h2x/literal "question") :context]
:timestamp
:model
:model_id
[:report_card.dataset :dataset]]
:from [:view_log]
:left-join [:report_card
[:and
[:= :view_log.model (h2x/literal "card")]
[:= :view_log.model_id :report_card.id]]]}
views {:union-all [qe vl]}]
(t2/query
{:select [[[:max :timestamp] :timestamp]
:model
:model_id]
:from [[views :views]]
:where [[:and
[:= :user_id [:inline user-id]]
[:>= :timestamp (t/minus (t/offset-date-time) (t/days 30))]
[:not= :context (h2x/literal "pulse")]
[:not= :context (h2x/literal "collection")]
[:not= :context (h2x/literal "ad-hoc")]
[:not= [:composite :context :model] [:composite (h2x/literal "dashboard") (h2x/literal "card")]]
[:not= [:composite :source :model :dataset] [:composite (h2x/literal "vl") (h2x/literal "card") [:inline false]]]
[:not-in [:composite :model :model_id] bookmarks]]]
:group-by [:model :model_id]
:order-by [[:timestamp :desc]]
:limit [:inline 8]})))
(defsetting user-recent-views
(deferred-tru "List of the 10 most recently viewed items for the user.")
:user-local :only
:type :json
:getter (fn []
(let [value (setting/get-value-of-type :json :user-recent-views)]
(if value
(vec value)
(let [views (mapv #(select-keys % [:model :model_id])
(recent-views-from-view-log api/*current-user-id*))]
(setting/set-value-of-type! :json :user-recent-views views)
views)))))
;; TODO: remove this setting as part of Audit V2 project.
(defsetting most-recently-viewed-dashboard
(deferred-tru "The Dashboard that the user has most recently viewed within the last 24 hours.")
:user-local :only
:type :json
:getter (fn []
(let [{:keys [id timestamp] :as value} (setting/get-value-of-type :json :most-recently-viewed-dashboard)
yesterday (t/minus (t/zoned-date-time) (t/hours 24))]
;; If the latest view is older than 24 hours, return 'nil'
(when (and value (t/after? (t/zoned-date-time timestamp) yesterday))
id)))
:setter (fn [id]
(when id
;; given a dashboard's ID, save it with a timestamp of 'now', for comparing later in the getter
(setting/set-value-of-type! :json :most-recently-viewed-dashboard {:id id :timestamp (t/zoned-date-time)}))))
;;; ## ---------------------------------------- EVENT PROCESSING ----------------------------------------
(defn- record-view!
......@@ -31,21 +133,34 @@
:model_id model-id
:metadata metadata))
(defn- update-users-recent-views!
[user-id model model-id]
(mw.session/with-current-user user-id
(let [view {:model (name model)
:model_id model-id}
prior-views (remove #{view} (user-recent-views))]
(when (= model "dashboard") (most-recently-viewed-dashboard! model-id))
(when-not ((set prior-views) view)
(let [new-views (vec (take 10 (conj prior-views view)))]
(user-recent-views! new-views))))))
(defn handle-view-event!
"Handle processing for a single event notification received on the view-log-channel"
[event]
;; try/catch here to prevent individual topic processing exceptions from bubbling up. better to handle them here.
(try
(when-let [{topic :topic object :item} event]
(record-view!
(events/topic->model topic)
(events/object->model-id topic object)
(events/object->user-id object)
(events/object->metadata object)))
(let [model (events/topic->model topic)
model-id (events/object->model-id topic object)
user-id (events/object->user-id object)
{:keys [context] :as metadata} (events/object->metadata object)]
(when (and (#{:card-query :dashboard-read :table-read} topic)
((complement #{:collection :dashboard}) context)) ;; we don't want to count pinned card views
(update-users-recent-views! user-id model model-id))
(record-view! model model-id user-id metadata)))
(catch Throwable e
(log/warn (format "Failed to process activity event. %s" (:topic event)) e))))
;;; ## ---------------------------------------- LIFECYLE ----------------------------------------
(defmethod events/init! ::ViewLog
......
src/metabase/query_processor/middleware/process_userland_query.clj
+ 1
− 0
View file @ 2b8e282e
......@@ -94,6 +94,7 @@
(events/publish-event! :card-query {:card_id (:card_id execution-info)
:actor_id (:executor_id execution-info)
:cached (:cached acc)
:context (:context execution-info)
:ignore_cache (get-in execution-info [:json_query :middleware :ignore-cached-results?])}))
(save-successful-query-execution! (:cached acc) execution-info @row-count)
(rf (if (map? acc)
......
src/metabase/server/handler.clj
+ 1
− 5
View file @ 2b8e282e
......@@ -23,11 +23,7 @@
(extend-protocol ring.protocols/StreamableResponseBody
;; java.lang.Double, java.lang.Long, and java.lang.Boolean will be given a Content-Type of "application/json; charset=utf-8"
;; so they should be strings, and will be parsed into their respective values.
java.lang.Double
(write-body-to-stream [num response output-stream]
(ring.protocols/write-body-to-stream (str num) response output-stream))
java.lang.Long
java.lang.Number
(write-body-to-stream [num response output-stream]
(ring.protocols/write-body-to-stream (str num) response output-stream))
......
test/metabase/api/activity_test.clj
+ 57
− 45
View file @ 2b8e282e
......@@ -4,6 +4,7 @@
[clojure.test :refer :all]
[java-time :as t]
[metabase.api.activity :as api.activity]
[metabase.events.view-log :as view-log]
[metabase.models.activity :refer [Activity]]
[metabase.models.card :refer [Card]]
[metabase.models.dashboard :refer [Dashboard]]
......@@ -83,35 +84,6 @@
:when (contains? these-activity-ids (u/the-id activity))]
(dissoc activity :timestamp)))))))))
;;; GET /recent_views
;; Things we are testing for:
;; 1. ordering is sorted by most recent
;; 2. results are filtered to current user
;; 3. `:model_object` is hydrated in each result
;; 4. we filter out entries where `:model_object` is nil (object doesn't exist)
(defn- create-views!
"Insert views [user-id model model-id]. Views are entered a second apart with last view as most recent."
[views]
(let [start-time (t/offset-date-time)
views (->> (map (fn [[user model model-id] seconds-ago]
(case model
"card" {:executor_id user :card_id model-id
:context :question
:hash (qp.util/query-hash {})
:running_time 1
:result_rows 1
:native false
:started_at (t/plus start-time (t/seconds (- seconds-ago)))}
{:user_id user, :model model, :model_id model-id
:timestamp (t/plus start-time (t/seconds (- seconds-ago)))}))
(reverse views)
(range))
(group-by #(if (:card_id %) :card :other)))]
(t2/insert! ViewLog (:other views))
(t2/insert! QueryExecution (:card views))))
(deftest recent-views-test
(mt/with-temp* [Card [card1 {:name "rand-name"
:creator_id (mt/user->id :crowberto)
......@@ -133,22 +105,62 @@
:creator_id (mt/user->id :crowberto)
:display "table"
:visualization_settings {}}]]
(mt/with-model-cleanup [ViewLog QueryExecution]
(create-views! [[(mt/user->id :crowberto) "card" (:id dataset)]
[(mt/user->id :crowberto) "card" (:id card1)]
[(mt/user->id :crowberto) "card" 36478]
[(mt/user->id :crowberto) "dashboard" (:id dash)]
[(mt/user->id :crowberto) "table" (:id table1)]
;; most recent for crowberto are archived card and hidden table
[(mt/user->id :crowberto) "card" (:id archived)]
[(mt/user->id :crowberto) "table" (:id hidden-table)]
[(mt/user->id :rasta) "card" (:id card1)]])
(let [recent-views (mt/user-http-request :crowberto :get 200 "activity/recent_views")]
(is (partial= [{:model "table" :model_id (:id table1)}
{:model "dashboard" :model_id (:id dash)}
{:model "card" :model_id (:id card1)}
{:model "dataset" :model_id (:id dataset)}]
recent-views))))))
(testing "recent_views endpoint shows the current user's recently viewed items."
(mt/with-model-cleanup [ViewLog]
(mt/with-test-user :crowberto
(mt/with-temporary-setting-values [user-recent-views []]
(doseq [event [{:topic :card-query :item dataset}
{:topic :card-query :item dataset}
{:topic :card-query :item card1}
{:topic :card-query :item card1}
{:topic :card-query :item card1}
{:topic :dashboard-read :item dash}
{:topic :card-query :item card1}
{:topic :dashboard-read :item dash}
{:topic :table-read :item table1}
{:topic :card-query :item archived}
{:topic :table-read :item hidden-table}]]
(view-log/handle-view-event!
;; view log entries look for the `:actor_id` in the item being viewed to set that view's :user_id
(assoc-in event [:item :actor_id] (mt/user->id :crowberto))))
(testing "No duplicates or archived items are returned."
(let [recent-views (mt/user-http-request :crowberto :get 200 "activity/recent_views")]
(is (partial=
[{:model "table" :model_id (u/the-id table1)}
{:model "dashboard" :model_id (u/the-id dash)}
{:model "card" :model_id (u/the-id card1)}
{:model "dataset" :model_id (u/the-id dataset)}]
recent-views))))))
(mt/with-test-user :rasta
(mt/with-temporary-setting-values [user-recent-views []]
(view-log/handle-view-event! {:topic :card-query :item (assoc dataset :actor_id (mt/user->id :rasta))})
(view-log/handle-view-event! {:topic :card-query :item (assoc card1 :actor_id (mt/user->id :crowberto))})
(testing "Only the user's own views are returned."
(let [recent-views (mt/user-http-request :rasta :get 200 "activity/recent_views")]
(is (partial=
[{:model "dataset" :model_id (u/the-id dataset)}]
(reverse recent-views)))))))))))
(defn- create-views!
"Insert views [user-id model model-id]. Views are entered a second apart with last view as most recent."
[views]
(let [start-time (t/offset-date-time)
views (->> (map (fn [[user model model-id] seconds-ago]
(case model
"card" {:executor_id user :card_id model-id
:context :question
:hash (qp.util/query-hash {})
:running_time 1
:result_rows 1
:native false
:started_at (t/plus start-time (t/seconds (- seconds-ago)))}
{:user_id user, :model model, :model_id model-id
:timestamp (t/plus start-time (t/seconds (- seconds-ago)))}))
(reverse views)
(range))
(group-by #(if (:card_id %) :card :other)))]
(t2/insert! ViewLog (:other views))
(t2/insert! QueryExecution (:card views))))
(deftest popular-items-test
(mt/with-temp* [Card [card1 {:name "rand-name"
......
test/metabase/events/view_log_test.clj
+ 69
− 1
View file @ 2b8e282e
(ns metabase.events.view-log-test
(:require
[clojure.test :refer :all]
[java-time :as t]
[metabase.events.view-log :as view-log]
[metabase.models :refer [Card Dashboard Table User ViewLog]]
[metabase.models.setting :as setting]
[metabase.test :as mt]
[metabase.util :as u]
[toucan2.core :as t2]))
(deftest card-create-test
......@@ -38,7 +41,7 @@
(is (= {:user_id (:id user)
:model "card"
:model_id (:id card)
:metadata {:cached false :ignore_cache true}}
:metadata {:cached false :ignore_cache true :context nil}}
(mt/derecordize
(t2/select-one [ViewLog :user_id :model :model_id :metadata], :user_id (:id user)))))))
......@@ -64,3 +67,68 @@
:model_id (:id dashboard)}
(mt/derecordize
(t2/select-one [ViewLog :user_id :model :model_id], :user_id (:id user)))))))
(deftest user-recent-views-test
(mt/with-temp* [Card [card1 {:name "rand-name"
:creator_id (mt/user->id :crowberto)
:display "table"
:visualization_settings {}}]
Card [archived {:name "archived-card"
:creator_id (mt/user->id :crowberto)
:display "table"
:archived true
:visualization_settings {}}]
Dashboard [dash {:name "rand-name2"
:description "rand-name2"
:creator_id (mt/user->id :crowberto)}]
Table [table1 {:name "rand-name"}]
Table [hidden-table {:name "hidden table"
:visibility_type "hidden"}]
Card [dataset {:name "rand-name"
:dataset true
:creator_id (mt/user->id :crowberto)
:display "table"
:visualization_settings {}}]]
(mt/with-model-cleanup [ViewLog]
(testing "User's recent views are updated when card/dashboard/table-read events occur."
(mt/with-test-user :crowberto
(view-log/user-recent-views! []) ;; ensure no views from any other tests/temp items exist
(doseq [event [{:topic :card-query :item dataset} ;; oldest view
{:topic :card-query :item dataset}
{:topic :card-query :item card1}
{:topic :card-query :item card1}
{:topic :card-query :item card1}
{:topic :dashboard-read :item dash}
{:topic :card-query :item card1}
{:topic :dashboard-read :item dash}
{:topic :table-read :item table1}
{:topic :card-query :item archived}
{:topic :table-read :item hidden-table}]]
(view-log/handle-view-event!
;; view log entries look for the `:actor_id` in the item being viewed to set that view's :user_id
(assoc-in event [:item :actor_id] (mt/user->id :crowberto))))
(let [recent-views (mt/with-test-user :crowberto (view-log/user-recent-views))]
(is (=
[{:model "table" :model_id (u/the-id hidden-table)}
{:model "card" :model_id (u/the-id archived)}
{:model "table" :model_id (u/the-id table1)}
{:model "dashboard" :model_id (u/the-id dash)}
{:model "card" :model_id (u/the-id card1)}
{:model "card" :model_id (u/the-id dataset)}]
recent-views))))))))
(deftest most-recently-viewed-dashboard-test
(mt/with-temp Dashboard [dash {:name "Look at this Distinguished Dashboard!"}]
(mt/with-model-cleanup [ViewLog]
(testing "When a user views a dashboard, most-recently-viewed-dashboard is updated with that id."
(mt/with-test-user :crowberto (setting/set-value-of-type! :json :most-recently-viewed-dashboard nil))
(view-log/handle-view-event! {:topic :dashboard-read
:metadata {:context "question"}
:item (assoc dash :actor_id (mt/user->id :crowberto))})
(is (= (u/the-id dash) (mt/with-test-user :crowberto (view-log/most-recently-viewed-dashboard)))))
(testing "When the user's most recent dashboard view is older than 24 hours, return `nil`."
(mt/with-test-user :crowberto
(setting/set-value-of-type! :json :most-recently-viewed-dashboard
{:id (u/the-id dash)
:timestamp (t/minus (t/zoned-date-time) (t/hours 25))}))
(is (= nil (mt/with-test-user :crowberto (view-log/most-recently-viewed-dashboard))))))))
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