Return app_id for dashboard bookmarks (#25419)

src/metabase/models/bookmark.clj

@@ -39,7 +39,11 @@
   "Normalizes bookmark results. Bookmarks are left joined against the card, collection, and dashboard tables, but only
   points to one of them. Normalizes it so it has just the desired fields."
   [result]
-  (let [result            (into {} (remove (comp nil? second) result))
+  (let [result            (cond-> (into {} (remove (comp nil? second) result))
+                            ;; If not a collection then remove collection properties
+                            ;; to avoid shadowing the "real" properties.
+                            (not= (:type result) "collection")
+                            (dissoc :collection.description :collection.name))
         normalized-result (zipmap (map unqualify-key (keys result)) (vals result))
         id-str            (str (:type normalized-result) "-" (:item_id normalized-result))]
     (-> normalized-result
@@ -100,7 +104,8 @@
          :from      [[(bookmarks-union-query user-id) :bookmark]]
          :left-join [[Card :card] [:= :bookmark.card_id :card.id]
                      [Dashboard :dashboard] [:= :bookmark.dashboard_id :dashboard.id]
-                     [Collection :collection] [:= :bookmark.collection_id :collection.id]
+                     [Collection :collection] [:in :collection.id [:bookmark.collection_id
+                                                                   :dashboard.collection_id]]
                      [App :app] [:= :app.collection_id :collection.id]
                      [BookmarkOrdering :bookmark_ordering] [:and
                                                             [:= :bookmark_ordering.user_id user-id]

test/metabase/api/bookmark_test.clj

@@ -15,10 +15,10 @@
 
 (deftest bookmarks-test
   (testing "POST /api/bookmark/:model/:model-id"
-    (mt/with-temp* [Collection [collection {:name "Test Collection"}]
-                    Card       [card {:name "Test Card" :display "area"}]
-                    Dashboard  [dashboard {:name "Test Dashboard" :is_app_page true}]
-                    App        [app {:collection_id (:id collection), :dashboard_id (:id dashboard)}]]
+    (mt/with-temp* [Collection [{coll-id :id :as collection} {:name "Test Collection"}]
+                    Card       [card {:name "Test Card", :display "area", :collection_id coll-id}]
+                    Dashboard  [dashboard {:name "Test Dashboard", :is_app_page true, :collection_id coll-id}]
+                    App        [app {:collection_id coll-id, :dashboard_id (:id dashboard)}]]
       (testing "check that we can bookmark a Collection"
         (is (= (u/the-id collection)
                (->> (mt/user-http-request :rasta :post 200 (str "bookmark/collection/" (u/the-id collection)))
@@ -43,10 +43,10 @@
           (is (= #{"card" "collection" "dashboard"}
                  (into #{} (map :type) result)))
           (testing "that app_id is hydrated on app collections"
-            (is (partial= [{:item_id (:id collection), :app_id (:id app)}]
+            (is (partial= [{:item_id (:id collection), :name "Test Collection", :app_id (:id app)}]
                           (filter #(= (:type %) "collection") result))))
           (testing "that is_app_page is returned for dashboards"
-            (is (partial= [{:item_id (:id dashboard), :is_app_page true}]
+            (is (partial= [{:item_id (:id dashboard), :name "Test Dashboard", :is_app_page true, :app_id (:id app)}]
                           (filter #(= (:type %) "dashboard") result))))))
       (testing "check that we can delete bookmarks"
         (mt/user-http-request :rasta :delete 204 (str "bookmark/card/" (u/the-id card)))

test/metabase/models/bookmark_test.clj

+(ns metabase.models.bookmark-test
+  (:require [clojure.test :refer :all]
+            [metabase.models.bookmark :as bookmark]))
+
+(deftest normalize-bookmark-result-test
+  (testing "collection properties don't shadow other properties"
+    (let [row {:report_card.archived         nil
+               :report_dashboard.description "Dashboard description"
+               :item_id                      853
+               :report_dashboard.name        "Test Dashboard"
+               :report_card.description      nil
+               :report_card.display          nil
+               :type                         "dashboard"
+               :report_card.name             nil
+               :report_dashboard.archived    false
+               :collection.description       "Collection description"
+               :report_dashboard.is_app_page true
+               :collection.app_id            178
+               :collection.archived          true
+               :report_card.dataset          nil
+               :created_at                   #t "2022-09-14T17:45:13.444716Z"
+               :collection.name              "Test Collection"}]
+      (is (= {:item_id     853
+              :name        "Test Dashboard"
+              :type        "dashboard"
+              :description "Dashboard description"
+              :is_app_page true
+              :app_id      178,
+              :id          "dashboard-853"}
+             (#'bookmark/normalize-bookmark-result row))))))