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

Add table view count column and increment it on views #41886

Merged
merged 7 commits into from
Apr 30, 2024
Merged

Add table view count column and increment it on views #41886

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
53f77f2
Add table view count
calherries Apr 26, 2024
61089ae
Increment counts on view
calherries Apr 26, 2024
98a240b
Fix test
calherries Apr 26, 2024
d703b7f
fix migration
calherries Apr 26, 2024
400270d
fix tests
calherries Apr 26, 2024
795cf55
fix
calherries Apr 26, 2024
a8dfedc
~ remove comment
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
43 changes: 43 additions & 0 deletions resources/migrations/001_update_migrations.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6665,6 +6665,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

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

########################################################################################################################
Expand Down
1 change: 1 addition & 0 deletions src/metabase/events/view_log.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,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 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 @@ -1918,13 +1918,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 @@ -1938,6 +1943,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 @@ -1948,5 +1957,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)))))))
13 changes: 13 additions & 0 deletions test/metabase/events/view_log_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,19 @@
(is (= 2 (t2/select-one-fn :view_count :model/Card (:id card)))
"view_count for cards on the dashboard be incremented too")))))

(deftest table-read-view-count-test
(mt/with-temp [:model/User user {}
:model/Table table {}]
(testing "A card read events are recorded by a card's view_count"
(is (= 0 (:view_count table))
"view_count should be 0 before the event is published")
(events/publish-event! :event/table-read {:object table :user-id (u/the-id user)})
(is (= 1 (t2/select-one-fn :view_count :model/Table (:id table)))
"view_count should be incremented")
(events/publish-event! :event/table-read {:object table :user-id (u/the-id user)})
(is (= 2 (t2/select-one-fn :view_count :model/Table (:id table)))
"view_count should be incremented"))))


;;; ---------------------------------------- API tests begin -----------------------------------------

Expand Down