Increment view_count on card, dashboard, and table views

src/metabase/events/view_log.clj

@@ -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]
@@ -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]
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -130,4 +142,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)))))

test/metabase/api/activity_test.clj

@@ -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)})
@@ -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"))))))))
 

test/metabase/api/dashboard_test.clj

@@ -716,7 +716,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"

test/metabase/events/view_log_test.clj

@@ -47,7 +47,7 @@
 (deftest collection-read-ee-test
   (when (premium-features/log-enabled?)
     (mt/with-temp [:model/Collection coll {}]
-      (testing "A basic card read event is recorded in EE"
+      (testing "A basic collection read event is recorded in EE"
         (events/publish-event! :event/collection-read {:object coll :user-id (mt/user->id :crowberto)})
         (is (partial=
              {:user_id    (mt/user->id :crowberto)
@@ -106,6 +106,36 @@
                 :context    "dashboard"}
                (latest-view (u/id user) (u/id card)))))))))
 
+(deftest card-read-view-count-test
+  (mt/with-temp [:model/User user {}
+                 :model/Card card {:creator_id (u/id user)}]
+    (testing "A card read events are recorded by a card's view_count"
+      (is (= 0 (:view_count card))
+          "view_count should be 0 before the event is published")
+      (events/publish-event! :event/card-read {:object card :user-id (u/the-id user)})
+      (is (= 1 (t2/select-one-fn :view_count :model/Card (:id card))))
+      (events/publish-event! :event/card-read {:object card :user-id (u/the-id user)})
+      (is (= 2 (t2/select-one-fn :view_count :model/Card (:id card)))))))
+
+(deftest dashboard-read-view-count-test
+  (mt/with-temp [:model/User          user      {}
+                 :model/Dashboard     dashboard {:creator_id (u/id user)}
+                 :model/Card          card      {:name "Dashboard Test Card"}
+                 :model/DashboardCard _dashcard {:dashboard_id (u/id dashboard) :card_id (u/id card)}]
+    (let [dashboard (t2/hydrate dashboard [:dashcards :card])]
+      (testing "A dashboard read events are recorded by a dashboard's view_count"
+        (is (= 0 (:view_count dashboard) (:view_count card))
+            "view_count should be 0 before the event is published")
+        (events/publish-event! :event/dashboard-read {:object dashboard :user-id (u/the-id user)})
+        (is (= 1 (t2/select-one-fn :view_count :model/Dashboard (:id dashboard))))
+        (is (= 1 (t2/select-one-fn :view_count :model/Card (:id card)))
+            "view_count for cards on the dashboard be incremented too")
+        (events/publish-event! :event/dashboard-read {:object dashboard :user-id (u/the-id user)})
+        (is (= 2 (t2/select-one-fn :view_count :model/Dashboard (:id dashboard))))
+        (is (= 2 (t2/select-one-fn :view_count :model/Card (:id card)))
+            "view_count for cards on the dashboard be incremented too")))))
+
+
 ;;; ---------------------------------------- API tests begin -----------------------------------------
 
 (deftest get-collection-view-log-test