Skip to content
Snippets Groups Projects
Unverified Commit 5597f0fe authored by Noah Moss's avatar Noah Moss Committed by GitHub
Browse files

Rework popular_items to use recent_views and view_count columns (#43041)

parent 0e157dc8
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
+ 32
− 43
View file @ 5597f0fe
......@@ -3,7 +3,7 @@
[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]]
......@@ -11,7 +11,6 @@
[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]))
......@@ -74,26 +73,26 @@
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]
......@@ -101,15 +100,9 @@
[:%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)
......@@ -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
......@@ -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)
test/metabase/api/activity_test.clj
+ 13
− 13
View file @ 5597f0fe
......@@ -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]
......@@ -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)})
......@@ -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)}]
......@@ -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)
......@@ -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"}
......@@ -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)]
......@@ -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)]
......@@ -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)])
......
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