Skip to content

Commit

Permalink
Rework popular_items to use recent_views and view_count columns (#43041)
Browse files Browse the repository at this point in the history
  • Loading branch information
@noahmoss
noahmoss committed May 23, 2024
1 parent 0e157dc commit 5597f0f
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 56 deletions.
75 changes: 32 additions & 43 deletions src/metabase/api/activity.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,14 @@
[clojure.string :as str]
[compojure.core :refer [GET]]
[medley.core :as m]
[metabase.api.common :as api :refer [*current-user-id* define-routes]]
[metabase.api.common :as api :refer [*current-user-id*]]
[metabase.db.query :as mdb.query]
[metabase.models.card :refer [Card]]
[metabase.models.dashboard :refer [Dashboard]]
[metabase.models.interface :as mi]
[metabase.models.query-execution :refer [QueryExecution]]
[metabase.models.recent-views :as recent-views]
[metabase.models.table :refer [Table]]
[metabase.models.view-log :refer [ViewLog]]
[metabase.util.honey-sql-2 :as h2x]
[metabase.util.malli :as mu]
[toucan2.core :as t2]))
Expand Down Expand Up @@ -74,42 +73,36 @@
Viewing a Dashboard will add entries to the view log for all cards on that dashboard so all card views are instead derived
from the query_execution table. The query context is always a `:question`. The results are normalized and concatenated to the
query results for dashboard and table views."
[views-limit card-runs-limit all-users?]
;; TODO update to use RecentViews instead of ViewLog
(let [dashboard-and-table-views (t2/select [ViewLog
[[:min :view_log.user_id] :user_id]
[views-limit card-runs-limit]
(let [dashboard-and-table-views (t2/select [:model/RecentViews
[[:min :recent_views.user_id] :user_id]
:model
:model_id
[:%count.* :cnt]
[[:max [:coalesce :d.view_count :t.view_count]] :cnt]
[:%max.timestamp :max_ts]]
{:group-by [:model :model_id]
:where [:and
(when-not all-users? [:= (mdb.query/qualify ViewLog :user_id) *current-user-id*])
[:in :model #{"dashboard" "table"}]
[:= :bm.id nil]]
[:in :model #{"dashboard" "table"}]]
:order-by [[:max_ts :desc] [:model :desc]]
:limit views-limit
:left-join [[:dashboard_bookmark :bm]
:left-join [[:report_dashboard :d]
[:and
[:= :model "dashboard"]
[:= :bm.user_id *current-user-id*]
[:= :model_id :bm.dashboard_id]]]})
[:= :d.id :model_id]]
[:metabase_table :t]
[:and
[:= :model "table"]
[:= :t.id :model_id]]]})
card-runs (->> (t2/select [QueryExecution
[:%min.executor_id :user_id]
[(mdb.query/qualify QueryExecution :card_id) :model_id]
[:%count.* :cnt]
[:%max.started_at :max_ts]]
{:group-by [(mdb.query/qualify QueryExecution :card_id) :context]
:where [:and
(when-not all-users? [:= :executor_id *current-user-id*])
[:= :context (h2x/literal :question)]
[:= :bm.id nil]]
[:= :context (h2x/literal :question)]]
:order-by [[:max_ts :desc]]
:limit card-runs-limit
:left-join [[:card_bookmark :bm]
[:and
[:= :bm.user_id *current-user-id*]
[:= (mdb.query/qualify QueryExecution :card_id) :bm.card_id]]]})
:limit card-runs-limit})
(map #(dissoc % :row_count))
(map #(assoc % :model "card")))]
(->> (concat card-runs dashboard-and-table-views)
Expand Down Expand Up @@ -193,22 +186,22 @@
;; - total count -> higher = higher score
;; - recently viewed -> more recent = higher score
;; - official/verified -> yes = higher score
(let [views (views-and-runs views-limit card-runs-limit true)
(let [views (views-and-runs views-limit card-runs-limit)
model->id->items (models-for-views views)
filtered-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 [is-dataset? (= (keyword (:type model-object)) :model)
is-metric? (= (keyword (:type model-object)) :metric)]]
(cond-> (assoc view-log :model_object model-object)
is-dataset? (assoc :model "dataset")
is-metric? (assoc :model "metric")))
scored-views (score-items filtered-views)]
filtered-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 [is-dataset? (= (keyword (:type model-object)) :model)
is-metric? (= (keyword (:type model-object)) :metric)]]
(cond-> (assoc view-log :model_object model-object)
is-dataset? (assoc :model "dataset")
is-metric? (assoc :model "metric")))
scored-views (score-items filtered-views)]
(->> scored-views
(sort-by
;; sort by model first, and then score when they are the same model
Expand All @@ -219,13 +212,9 @@
recent-views/fill-recent-view-info)))))

(api/defendpoint GET "/popular_items"
"Get the list of 5 popular things for the current user. Query takes 8 and limits to 5 so that if it
finds anything archived, deleted, etc it can usually still get 5."
"Get the list of 5 popular things on the instance. Query takes 8 and limits to 5 so that if it finds anything
archived, deleted, etc it can usually still get 5. "
[]
;; we can do a weighted score which incorporates:
;; total count -> higher = higher score
;; recently viewed -> more recent = higher score
;; official/verified -> yes = higher score
{:popular_items (get-popular-items-model-and-id)})

(define-routes)
(api/define-routes)
26 changes: 13 additions & 13 deletions test/metabase/api/activity_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@
[metabase.events :as events]
[metabase.models.card :refer [Card]]
[metabase.models.dashboard :refer [Dashboard]]
[metabase.models.query-execution :refer [QueryExecution]]
[metabase.models.table :refer [Table]]
[metabase.models.view-log :refer [ViewLog]]
[metabase.query-processor.util :as qp.util]
[metabase.test :as mt]
[metabase.test.fixtures :as fixtures]
Expand Down Expand Up @@ -69,7 +67,7 @@
(mt/with-temp [:model/Collection coll {:name "Analytics"}
:model/Dashboard dash-1 {:collection_id (t2/select-one-pk :model/Collection :personal_owner_id (mt/user->id :crowberto))}
:model/Dashboard dash-2 {:collection_id (:id coll)}]
(mt/with-model-cleanup [ViewLog]
(mt/with-model-cleanup [:model/RecentViews]
(mt/with-test-user :crowberto
(testing "view a dashboard in a personal collection"
(events/publish-event! :event/dashboard-read {:object dash-1 :user-id (mt/user->id :crowberto)})
Expand Down Expand Up @@ -113,7 +111,7 @@
:display "table"
:visualization_settings {}}]
(testing "recent_views endpoint shows the current user's recently viewed items."
(mt/with-model-cleanup [ViewLog]
(mt/with-model-cleanup [:model/RecentViews]
(mt/with-test-user :crowberto
(doseq [[topic event] [[:event/card-query {:card-id (:id dataset)}]
[:event/card-query {:card-id (:id dataset)}]
Expand Down Expand Up @@ -163,12 +161,12 @@
(reverse views)
(range))
(group-by #(if (:card_id %) :card :other)))]
(t2/insert! ViewLog (:other views))
(t2/insert! QueryExecution (:card views))))
(t2/insert! :model/RecentViews (:other views))
(t2/insert! :model/QueryExecution (:card views))))

(deftest popular-items-test
;; Clear out the view log & query execution log so that test doesn't read stale state
(t2/delete! :model/ViewLog)
;; Clear out recent views & query execution log so that test doesn't read stale state
(t2/delete! :model/RecentViews)
(t2/delete! :model/QueryExecution)
(mt/with-temp [Card card1 {:name "rand-name"
:creator_id (mt/user->id :crowberto)
Expand All @@ -181,10 +179,12 @@
:visualization_settings {}}
Dashboard dash1 {:name "rand-name"
:description "rand-name"
:creator_id (mt/user->id :crowberto)}
:creator_id (mt/user->id :crowberto)
:view_count 10}
Dashboard dash2 {:name "other-dashboard"
:description "just another dashboard"
:creator_id (mt/user->id :crowberto)}
:creator_id (mt/user->id :crowberto)
:view_count 5}
Table table1 {:name "rand-name"}
Table hidden-table {:name "hidden table"
:visibility_type "hidden"}
Expand All @@ -200,7 +200,7 @@
:visualization_settings {}}]
(let [test-ids (set (map :id [card1 archived dash1 dash2 table1 hidden-table dataset metric]))]
(testing "Items viewed by multiple users are never duplicated in the popular items list."
(mt/with-model-cleanup [ViewLog QueryExecution]
(mt/with-model-cleanup [:model/RecentViews :model/QueryExecution]
(create-views! [[(mt/user->id :rasta) "dashboard" (:id dash1)]
[(mt/user->id :crowberto) "dashboard" (:id dash1)]
[(mt/user->id :rasta) "card" (:id card1)]
Expand All @@ -213,7 +213,7 @@
(filter (comp test-ids u/the-id))
(map (juxt :model :id)))))))
(testing "Items viewed by other users can still show up in popular items."
(mt/with-model-cleanup [ViewLog QueryExecution]
(mt/with-model-cleanup [:model/RecentViews :model/QueryExecution]
(create-views! [[(mt/user->id :rasta) "dashboard" (:id dash1)]
[(mt/user->id :rasta) "card" (:id card1)]
[(mt/user->id :rasta) "table" (:id table1)]
Expand All @@ -228,7 +228,7 @@
(filter #(test-ids (:id %)))
(map (juxt :model :id)))))))
(testing "Items with more views show up sooner in popular items."
(mt/with-model-cleanup [ViewLog QueryExecution]
(mt/with-model-cleanup [:model/RecentViews :model/QueryExecution]
(create-views! (concat
;; one item with many views is considered more popular
(repeat 10 [(mt/user->id :rasta) "dashboard" (:id dash1)])
Expand Down

0 comments on commit 5597f0f

Please sign in to comment.