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

Popular Items should no longer have duplicates in the returned list (#21602) 

* Popular Items should no longer have duplicates in the returned list

* Introduce concept of model precedence for popular_items

This might also be handled inside the score function, to allow for recency or viewcount to push lower-precedence items
higher in the results, but for now, grouping fetched items by their model and sorting by precedence (dashboard, then
cards, then models, then tables) should be ok.
parent 776cb81b
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/api/activity.clj
+ 14
− 5
View file @ d1ae9740
......@@ -138,23 +138,23 @@
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?]
(let [dashboard-and-table-views (db/select [ViewLog :user_id :model :model_id
(let [dashboard-and-table-views (db/select [ViewLog :%min.view_log.user_id :model :model_id
[:%count.* :cnt] [:%max.timestamp :max_ts]]
{:group-by [(db/qualify ViewLog :user_id) :model :model_id]
{:group-by [:model :model_id]
:where [:and
(when-not all-users? [:= (db/qualify ViewLog :user_id) *current-user-id*])
[:in :model #{"dashboard" "table"}]
[:= :bm.id nil]]
:order-by [[:max_ts :desc]]
:order-by [[:model :desc] [:max_ts :desc]]
:limit views-limit
:left-join [[DashboardBookmark :bm]
[:and
[:not [:= :model "table"]]
[:= :bm.user_id *current-user-id*]
[:= :model_id :bm.dashboard_id]]]})
card-runs (->> (db/select [QueryExecution [:executor_id :user_id] [(db/qualify QueryExecution :card_id) :model_id]
card-runs (->> (db/select [QueryExecution [:%min.executor_id :user_id] [(db/qualify QueryExecution :card_id) :model_id]
[:%count.* :cnt] [:%max.started_at :max_ts]]
{:group-by [:executor_id (db/qualify QueryExecution :card_id) :context]
{:group-by [(db/qualify QueryExecution :card_id) :context]
:where [:and
(when-not all-users? [:= :executor_id *current-user-id*])
[:= :context (hx/literal :question)]
......@@ -232,6 +232,14 @@
(* (/ cnt max-count) views-wt)]]
(assoc item :score (double (reduce + scores))))))))
(def ^:private model-precedence ["dashboard" "card" "dataset" "table"])
(defn- order-items
[items]
(when (seq items)
(let [groups (group-by :model items)]
(mapcat #(get groups %) model-precedence))))
(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 hopefully still get 5."
......@@ -256,6 +264,7 @@
(->> scored-views
(sort-by :score)
reverse
order-items
(take 5)
(map #(dissoc % :score)))))
......
test/metabase/api/activity_test.clj
+ 44
− 15
View file @ d1ae9740
......@@ -160,6 +160,9 @@
Dashboard [dash1 {:name "rand-name"
:description "rand-name"
:creator_id (mt/user->id :crowberto)}]
Dashboard [dash2 {:name "other-dashboard"
:description "just another dashboard"
:creator_id (mt/user->id :crowberto)}]
Table [table1 {:name "rand-name"}]
Table [hidden-table {:name "hidden table"
:visibility_type "hidden"}]
......@@ -168,21 +171,47 @@
:creator_id (mt/user->id :crowberto)
:display "table"
:visualization_settings {}}]]
(mt/with-model-cleanup [ViewLog QueryExecution]
(create-views! (concat
;; one item with many views is considered more popular
(repeat 10 [(mt/user->id :rasta) "card" (:id dataset)])
[[(mt/user->id :rasta) "dashboard" (:id dash1)]
[(mt/user->id :rasta) "card" (:id card1)]
[(mt/user->id :rasta) "table" (:id table1)]
[(mt/user->id :rasta) "card" (:id card1)]]))
(is (= [["dataset" (:id dataset)]
["card" (:id card1)]
["table" (:id table1)]
["dashboard" (:id dash1)]]
;; all views are from :rasta, but :crowberto can still see popular items
(for [popular-item (mt/user-http-request :crowberto :get 200 "activity/popular_items")]
((juxt :model :model_id) popular-item)))))))
(testing "Items viewed by multiple users are not duplicated in the popular items list."
(mt/with-model-cleanup [ViewLog QueryExecution]
(create-views! [[(mt/user->id :rasta) "dashboard" (:id dash1)]
[(mt/user->id :crowberto) "dashboard" (:id dash1)]
[(mt/user->id :rasta) "card" (:id card1)]
[(mt/user->id :crowberto) "card" (:id card1)]])
(is (= [["dashboard" (:id dash1)]
["card" (:id card1)]]
;; all views are from :rasta, but :crowberto can still see popular items
(for [popular-item (mt/user-http-request :crowberto :get 200 "activity/popular_items")]
((juxt :model :model_id) popular-item))))))
(testing "Items viewed by other users can still show up in popular items."
(mt/with-model-cleanup [ViewLog QueryExecution]
(create-views! [[(mt/user->id :rasta) "dashboard" (:id dash1)]
[(mt/user->id :rasta) "card" (:id card1)]
[(mt/user->id :rasta) "table" (:id table1)]
[(mt/user->id :rasta) "card" (:id dataset)]])
(is (= [["dashboard" (:id dash1)]
["card" (:id card1)]
["dataset" (:id dataset)]
["table" (:id table1)]]
;; all views are from :rasta, but :crowberto can still see popular items
(for [popular-item (mt/user-http-request :crowberto :get 200 "activity/popular_items")]
((juxt :model :model_id) popular-item))))))
(testing "Items with more views show up sooner in popular items."
(mt/with-model-cleanup [ViewLog QueryExecution]
(create-views! (concat
;; one item with many views is considered more popular
(repeat 10 [(mt/user->id :rasta) "dashboard" (:id dash1)])
[[(mt/user->id :rasta) "dashboard" (:id dash2)]
[(mt/user->id :rasta) "card" (:id dataset)]
[(mt/user->id :rasta) "table" (:id table1)]
[(mt/user->id :rasta) "card" (:id card1)]]))
(is (= [["dashboard" (:id dash1)]
["dashboard" (:id dash2)]
["card" (:id card1)]
["dataset" (:id dataset)]
["table" (:id table1)]]
;; all views are from :rasta, but :crowberto can still see popular items
(for [popular-item (mt/user-http-request :crowberto :get 200 "activity/popular_items")]
((juxt :model :model_id) popular-item))))))))
;;; activities->referenced-objects, referenced-objects->existing-objects, add-model-exists-info
......
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