Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Increment view_count on card, dashboard, and table views #41882

Merged
merged 12 commits into from
May 8, 2024
Merged

Increment view_count on card, dashboard, and table views #41882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
96cebe4
Add view_count to report_card and report_dashboard
calherries Apr 25, 2024
68a8960
Migrations to add and populate view_count for cards and dashboards
calherries Apr 25, 2024
3ec54c2
tidy
calherries Apr 25, 2024
b7b94ed
Fix tests
calherries Apr 26, 2024
6b3587b
Add view_count separately to view_log
calherries Apr 26, 2024
70cf20d
Reuse view_log namespace instead
calherries Apr 26, 2024
3c8a42f
fix tests
calherries Apr 26, 2024
b84d9da
Merge branch 'master' into update-view-count-columns
calherries Apr 26, 2024
3b37ede
Tidy tests
calherries Apr 26, 2024
f79e028
Increment view counts for dashcards on a dashboard
calherries Apr 26, 2024
c319813
Add table view count column and increment it on views (#41886)
calherries Apr 30, 2024
db0de4f
Merge branch 'master' into update-view-count-columns
calherries Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 43 additions & 1 deletion resources/migrations/001_update_migrations.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6697,6 +6697,49 @@ databaseChangeLog:
);
rollback: # nothing to do, since view_count didn't exist in v49

- changeSet:
id: v50.2024-04-25T16:29:35
author: calherries
comment: Add metabase_table.view_count
changes:
- addColumn:
columns:
- column:
name: view_count
type: integer
defaultValueNumeric: 0
remarks: Keeps a running count of card views
constraints:
nullable: false
tableName: metabase_table

- changeSet:
id: v50.2024-04-25T16:29:36
author: calherries
comment: Populate metabase_table.view_count
changes:
- sql:
dbms: mysql,mariadb
sql: >-
UPDATE metabase_table t
SET t.view_count = (
SELECT count(*)
FROM view_log v
WHERE v.model = 'table'
AND v.model_id = t.id
);
- sql:
dbms: postgresql,h2
sql: >-
UPDATE metabase_table t
SET view_count = (
SELECT count(*)
FROM view_log v
WHERE v.model = 'table'
AND v.model_id = t.id
);
rollback: # nothing to do, since view_count didn't exist in v49

- changeSet:
id: v50.2024-04-26T09:19:00
author: adam-james
Expand Down Expand Up @@ -6748,7 +6791,6 @@ databaseChangeLog:
column:
name: user_id


# >>>>>>>>>> DO NOT ADD NEW MIGRATIONS BELOW THIS LINE! ADD THEM ABOVE <<<<<<<<<<

########################################################################################################################
Expand Down
39 changes: 26 additions & 13 deletions src/metabase/events/view_log.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
(ns metabase.events.view-log
"This namespace is responsible for subscribing to events which should update the view log."
"This namespace is responsible for subscribing to events which should update the view log and view counts."
(:require
[metabase.api.common :as api]
[metabase.events :as events]
Expand All @@ -12,6 +12,15 @@
[steffan-westcott.clj-otel.api.trace.span :as span]
[toucan2.core :as t2]))

(defn increment-view-counts!
"Increments the view_count column for a model given a list of ids.
Assumes the model has one primary key `id`, and the column for the view count is named `view_count`"
[model & ids]
(when (seq ids)
(t2/query {:update (t2/table-name model)
:set {:view_count [:+ :view_count [:inline 1]]}
:where [:in :id ids]})))

(defn- record-views!
"Simple base function for recording a view of a given `model` and `model-id` by a certain `user`."
[view-or-views]
Expand All @@ -34,18 +43,19 @@

(m/defmethod events/publish-event! ::card-read-event
"Handle processing for a generic read event notification"
[topic event]
[topic {:keys [object user-id] :as event}]
(span/with-span!
{:name "view-log-card-read"
:topic topic
:user-id (:user-id event)}
:user-id user-id}
(try
(increment-view-counts! :model/Card (:id object))
(-> event
generate-view
(assoc :context "question")
record-views!)
(catch Throwable e
(log/warnf e "Failed to process view_log event. %s" topic)))))
(log/warnf e "Failed to process view event. %s" topic)))))

(derive ::collection-read-event :metabase/event)
(derive :event/collection-read ::collection-read-event)
Expand All @@ -58,7 +68,7 @@
generate-view
record-views!)
(catch Throwable e
(log/warnf e "Failed to process view_log event. %s" topic))))
(log/warnf e "Failed to process view event. %s" topic))))

(derive ::read-permission-failure :metabase/event)
(derive :event/read-permission-failure ::read-permission-failure)
Expand All @@ -74,7 +84,7 @@
generate-view
record-views!))
(catch Throwable e
(log/warnf e "Failed to process view_log event. %s" topic))))
(log/warnf e "Failed to process view event. %s" topic))))

(derive ::dashboard-read :metabase/event)
(derive :event/dashboard-read ::dashboard-read)
Expand All @@ -98,16 +108,18 @@
(let [dashcards (filter :card_id (:dashcards object)) ;; filter out link/text cards wtih no card_id
user-id (or user-id api/*current-user-id*)
views (map (fn [dashcard]
{:model "card"
:model_id (u/id (:card_id dashcard))
:user_id user-id
:has_access (readable-dashcard? dashcard)
:context "dashboard"})
{:model "card"
:model_id (u/id (:card_id dashcard))
:user_id user-id
:has_access (readable-dashcard? dashcard)
:context "dashboard"})
dashcards)
dash-view (generate-view event)]
(apply increment-view-counts! :model/Card (map :card_id dashcards))
(increment-view-counts! :model/Dashboard (:id object))
(record-views! (cons dash-view views)))
(catch Throwable e
(log/warnf e "Failed to process view_log event. %s" topic)))))
(log/warnf e "Failed to process view event. %s" topic)))))

(derive ::table-read :metabase/event)
(derive :event/table-read ::table-read)
Expand All @@ -121,6 +133,7 @@
:topic topic
:user-id user-id}
(try
(increment-view-counts! :model/Table (:id object))
(let [table-id (u/id object)
database-id (:db_id object)
has-access? (when (= api/*current-user-id* user-id)
Expand All @@ -130,4 +143,4 @@
generate-view
record-views!))
(catch Throwable e
(log/warnf e "Failed to process view_log event. %s" topic)))))
(log/warnf e "Failed to process view event. %s" topic)))))
8 changes: 4 additions & 4 deletions test/metabase/api/activity_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,15 @@
{:topic :event/table-read :event {:object table-1}}]]
(events/publish-event! topic (assoc event :user-id (mt/user->id :crowberto))))
(testing "most_recently_viewed_dashboard endpoint shows the current user's most recently viewed dashboard."
(is (= (assoc dash-3 :collection nil) #_dash-2 ;; TODO: this should be dash-2, because dash-3 is archived
(is (= (assoc dash-3 :collection nil :view_count 1) #_dash-2 ;; TODO: this should be dash-2, because dash-3 is archived
(mt/user-http-request :crowberto :get 200 "activity/most_recently_viewed_dashboard")))))
(mt/with-test-user :rasta
(testing "If nothing has been viewed, return a 204"
(is (nil? (mt/user-http-request :rasta :get 204
"activity/most_recently_viewed_dashboard"))))
(events/publish-event! :event/dashboard-read {:object dash-1 :user-id (mt/user->id :rasta)})
(testing "Only the user's own views are returned."
(is (= (assoc dash-1 :collection nil)
(is (= (assoc dash-1 :collection nil :view_count 2)
(mt/user-http-request :rasta :get 200
"activity/most_recently_viewed_dashboard"))))
(events/publish-event! :event/dashboard-read {:object dash-1 :user-id (mt/user->id :rasta)})
Expand All @@ -74,13 +74,13 @@
(testing "view a dashboard in a personal collection"
(events/publish-event! :event/dashboard-read {:object dash-1 :user-id (mt/user->id :crowberto)})
(let [crowberto-personal-coll (t2/select-one :model/Collection :personal_owner_id (mt/user->id :crowberto))]
(is (= (assoc dash-1 :collection (assoc crowberto-personal-coll :is_personal true))
(is (= (assoc dash-1 :collection (assoc crowberto-personal-coll :is_personal true) :view_count 1)
(mt/user-http-request :crowberto :get 200
"activity/most_recently_viewed_dashboard")))))

(testing "view a dashboard in a public collection"
(events/publish-event! :event/dashboard-read {:object dash-2 :user-id (mt/user->id :crowberto)})
(is (= (assoc dash-2 :collection (assoc coll :is_personal false))
(is (= (assoc dash-2 :collection (assoc coll :is_personal false) :view_count 1)
(mt/user-http-request :crowberto :get 200
"activity/most_recently_viewed_dashboard"))))))))

Expand Down
3 changes: 2 additions & 1 deletion test/metabase/api/dashboard_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -749,7 +749,8 @@
:cache_ttl 1234
:creator_id (mt/user->id :rasta)
:collection false
:collection_id true})
:collection_id true
:view_count 1})
(dashboard-response (t2/select-one Dashboard :id dashboard-id)))))

(testing "No-op PUT: Do not return 500"
Expand Down
2 changes: 1 addition & 1 deletion test/metabase/api/field_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
{:table_id (mt/id :users)
:table (merge
(mt/obj->json->obj (mt/object-defaults Table))
(t2/select-one [Table :created_at :updated_at :initial_sync_status] :id (mt/id :users))
(t2/select-one [Table :created_at :updated_at :initial_sync_status :view_count] :id (mt/id :users))
{:description nil
:entity_type "entity/UserTable"
:visibility_type nil
Expand Down
22 changes: 15 additions & 7 deletions test/metabase/api/table_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
{:db (db-details)
:entity_type "entity/GenericTable"
:field_order "database"
:view_count 0
:metrics []
:segments []}))

Expand Down Expand Up @@ -148,7 +149,9 @@
(testing "GET /api/table/:id"
(is (= (merge
(dissoc (table-defaults) :segments :field_values :metrics)
(t2/hydrate (t2/select-one [Table :id :created_at :updated_at :initial_sync_status] :id (mt/id :venues)) :pk_field)
(t2/hydrate (t2/select-one [Table :id :created_at :updated_at :initial_sync_status :view_count]
:id (mt/id :venues))
:pk_field)
{:schema "PUBLIC"
:name "VENUES"
:display_name "Venues"
Expand All @@ -164,7 +167,9 @@
:schema nil}]
(is (= (merge
(dissoc (table-defaults) :segments :field_values :metrics :db)
(t2/hydrate (t2/select-one [Table :id :created_at :updated_at :initial_sync_status] :id table-id) :pk_field)
(t2/hydrate (t2/select-one [Table :id :created_at :updated_at :initial_sync_status :view_count]
:id table-id)
:pk_field)
{:schema ""
:name "schemaless_table"
:display_name "Schemaless"
Expand Down Expand Up @@ -201,7 +206,7 @@
(testing "Sensitive fields are included"
(is (= (merge
(query-metadata-defaults)
(t2/select-one [Table :created_at :updated_at :initial_sync_status] :id (mt/id :users))
(t2/select-one [Table :created_at :updated_at :initial_sync_status :view_count] :id (mt/id :users))
{:schema "PUBLIC"
:name "USERS"
:display_name "Users"
Expand Down Expand Up @@ -273,7 +278,7 @@
(testing "Sensitive fields should not be included"
(is (= (merge
(query-metadata-defaults)
(t2/select-one [Table :created_at :updated_at :initial_sync_status] :id (mt/id :users))
(t2/select-one [Table :created_at :updated_at :initial_sync_status :view_count] :id (mt/id :users))
{:schema "PUBLIC"
:name "USERS"
:display_name "Users"
Expand Down Expand Up @@ -474,7 +479,9 @@
:database_indexed true
:table (merge
(dissoc (table-defaults) :segments :field_values :metrics)
(t2/select-one [Table :id :created_at :updated_at :initial_sync_status]
(t2/select-one [Table
:id :created_at :updated_at
:initial_sync_status :view_count]
:id (mt/id :checkins))
{:schema "PUBLIC"
:name "CHECKINS"
Expand All @@ -492,14 +499,15 @@
:database_indexed true
:table (merge
(dissoc (table-defaults) :db :segments :field_values :metrics)
(t2/select-one [Table :id :created_at :updated_at :initial_sync_status]
(t2/select-one [Table
:id :created_at :updated_at
:initial_sync_status :view_count]
:id (mt/id :users))
{:schema "PUBLIC"
:name "USERS"
:display_name "Users"
:entity_type "entity/UserTable"})))}]
(mt/user-http-request :rasta :get 200 (format "table/%d/fks" (mt/id :users)))))))

(testing "should just return nothing for 'virtual' tables"
(is (= []
(mt/user-http-request :crowberto :get 200 "table/card__1000/fks"))))))
Expand Down
12 changes: 11 additions & 1 deletion test/metabase/db/schema_migrations_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1927,13 +1927,18 @@

(deftest view-count-test
(testing "report_card.view_count and report_dashboard.view_count should be populated"
(impl/test-migrations ["v50.2024-04-25T16:29:31" "v50.2024-04-25T16:29:34"] [migrate!]
(impl/test-migrations ["v50.2024-04-25T16:29:31" "v50.2024-04-25T16:29:36"] [migrate!]
(let [user-id 13371338 ; use internal user to avoid creating a real user
db-id (t2/insert-returning-pk! :metabase_database {:name "db"
:engine "postgres"
:created_at :%now
:updated_at :%now
:details "{}"})
table-id (t2/insert-returning-pk! :metabase_table {:active true
:db_id db-id
:name "a table"
:created_at :%now
:updated_at :%now})
dash-id (t2/insert-returning-pk! :report_dashboard {:name "A dashboard"
:creator_id user-id
:parameters "[]"
Expand All @@ -1947,6 +1952,10 @@
:name "a card"
:created_at :%now
:updated_at :%now})
_ (t2/insert-returning-pk! :view_log {:user_id user-id
:model "table"
:model_id table-id
:timestamp :%now})
_ (t2/insert-returning-pk! :view_log {:user_id user-id
:model "card"
:model_id card-id
Expand All @@ -1957,5 +1966,6 @@
:model_id dash-id
:timestamp :%now}))]
(migrate!)
(is (= 1 (t2/select-one-fn :view_count :metabase_table table-id)))
(is (= 1 (t2/select-one-fn :view_count :report_card card-id)))
(is (= 2 (t2/select-one-fn :view_count :report_dashboard dash-id)))))))