From 6f8375b516082d8ce7ca8b97a04a7bd314fc8152 Mon Sep 17 00:00:00 2001
From: metamben <103100869+metamben@users.noreply.github.com>
Date: Fri, 2 Sep 2022 00:30:53 +0300
Subject: [PATCH] Return app collections as apps, page dashboards as pages
(#25194)
* Return app collections as apps, page dashboards as pages
* Fix bug: descriptions of datasets should also be searchable
---
src/metabase/api/search.clj | 34 ++++++++++++++++++++-----
src/metabase/search/config.clj | 12 ++++++++-
test/metabase/api/search_test.clj | 41 ++++++++++++++++++++++---------
3 files changed, 69 insertions(+), 18 deletions(-)
diff --git a/src/metabase/api/search.clj b/src/metabase/api/search.clj
index 007abbd9d7a..2b591f568c5 100644
--- a/src/metabase/api/search.clj
+++ b/src/metabase/api/search.clj
@@ -283,9 +283,10 @@
(update :select (fn [columns]
(cons [(hx/literal "dataset") :model] (rest columns))))))
-(s/defmethod search-query-for-model "collection"
- [model search-ctx :- SearchContext]
- (-> (base-query-for-model model search-ctx)
+(defn- shared-collection-impl
+ [model search-ctx]
+ (-> (base-query-for-model "collection" search-ctx)
+ (update :where (fn [where] [:and [(if (= model "app") :<> :=) :app.id nil] where]))
(hh/left-join [CollectionBookmark :bookmark]
[:and
[:= :bookmark.collection_id :collection.id]
@@ -294,19 +295,40 @@
[:= :app.collection_id :collection.id])
(add-collection-join-and-where-clauses :collection.id search-ctx)))
+(s/defmethod search-query-for-model "collection"
+ [model search-ctx :- SearchContext]
+ (shared-collection-impl model search-ctx))
+
+(s/defmethod search-query-for-model "app"
+ [model search-ctx :- SearchContext]
+ (-> (shared-collection-impl model search-ctx)
+ (update :select (fn [columns]
+ (cons [(hx/literal model) :model] (rest columns))))))
+
(s/defmethod search-query-for-model "database"
[model search-ctx :- SearchContext]
(base-query-for-model model search-ctx))
-(s/defmethod search-query-for-model "dashboard"
- [model search-ctx :- SearchContext]
- (-> (base-query-for-model model search-ctx)
+(defn- shared-dashboard-impl
+ [model search-ctx]
+ (-> (base-query-for-model "dashboard" search-ctx)
+ (update :where (fn [where] [:and [:= :dashboard.is_app_page (= model "page")] where]))
(hh/left-join [DashboardBookmark :bookmark]
[:and
[:= :bookmark.dashboard_id :dashboard.id ]
[:= :bookmark.user_id api/*current-user-id*]])
(add-collection-join-and-where-clauses :dashboard.collection_id search-ctx)))
+(s/defmethod search-query-for-model "dashboard"
+ [model search-ctx :- SearchContext]
+ (shared-dashboard-impl model search-ctx))
+
+(s/defmethod search-query-for-model "page"
+ [model search-ctx :- SearchContext]
+ (-> (shared-dashboard-impl model search-ctx)
+ (update :select (fn [columns]
+ (cons [(hx/literal model) :model] (rest columns))))))
+
(s/defmethod search-query-for-model "pulse"
[model search-ctx :- SearchContext]
;; Pulses don't currently support being archived, omit if archived is true
diff --git a/src/metabase/search/config.clj b/src/metabase/search/config.clj
index 259b21eb19e..65a32fd0e0a 100644
--- a/src/metabase/search/config.clj
+++ b/src/metabase/search/config.clj
@@ -43,11 +43,13 @@
(def model-to-db-model
"Mapping from string model to the Toucan model backing it."
{"dashboard" Dashboard
+ "page" Dashboard
"metric" Metric
"segment" Segment
"card" Card
"dataset" Card
"collection" Collection
+ "app" Collection
"table" Table
"pulse" Pulse
"database" Database})
@@ -55,7 +57,7 @@
(def all-models
"All valid models to search for. The order of this list also influences the order of the results: items earlier in the
list will be ranked higher."
- ["dashboard" "metric" "segment" "card" "dataset" "collection" "table" "pulse" "database"])
+ ["dashboard" "page" "metric" "segment" "card" "dataset" "collection" "app" "table" "pulse" "database"])
(def ^:const displayed-columns
"All of the result components that by default are displayed by the frontend."
@@ -76,11 +78,19 @@
:dataset_query
:description])
+(defmethod searchable-columns-for-model "dataset"
+ [_]
+ (searchable-columns-for-model "card"))
+
(defmethod searchable-columns-for-model "dashboard"
[_]
[:name
:description])
+(defmethod searchable-columns-for-model "page"
+ [_]
+ (searchable-columns-for-model "dashboard"))
+
(defmethod searchable-columns-for-model "database"
[_]
[:name
diff --git a/test/metabase/api/search_test.clj b/test/metabase/api/search_test.clj
index 5048af2757b..c8a52990d51 100644
--- a/test/metabase/api/search_test.clj
+++ b/test/metabase/api/search_test.clj
@@ -600,15 +600,17 @@
(deftest card-dataset-query-test
(testing "Search results should match a native query's dataset_query column, but not an MBQL query's one."
;; https://github.com/metabase/metabase/issues/24132
- (mt/with-temp* [Card [_mbql-card {:name "Venues Count"
- :query_type "query"
- :dataset_query (mt/mbql-query venues {:aggregation [[:count]]})}]
- Card [_native-card {:name "Another SQL query"
- :query_type "native"
- :dataset_query (mt/native-query {:query "SELECT COUNT(1) AS aggregation FROM venues"})}]]
- (is (= ["Another SQL query"]
- (->> (search-request-data :rasta :q "aggregation")
- (map :name)))))))
+ (let [native-card {:name "Another SQL query"
+ :query_type "native"
+ :dataset_query (mt/native-query {:query "SELECT COUNT(1) AS aggregation FROM venues"})}]
+ (mt/with-temp* [Card [_mbql-card {:name "Venues Count"
+ :query_type "query"
+ :dataset_query (mt/mbql-query venues {:aggregation [[:count]]})}]
+ Card [_native-card native-card]
+ Card [_dataset (assoc native-card :name "Dataset" :dataset true)]]
+ (is (= ["Another SQL query" "Dataset"]
+ (->> (search-request-data :rasta :q "aggregation")
+ (map :name))))))))
(deftest app-test
(testing "App collections should come with app_id set"
@@ -618,6 +620,23 @@
(fn [result]
(cond-> result
(not (#{"metric" "segment"} (:model result))) (assoc-in [:collection :app_id] true)
- (= (:model result) "collection") (assoc :app_id true)))
+ (= (:model result) "collection") (assoc :model "app" :app_id true)))
(default-results-with-collection))
- (search-request-data :rasta :q "test")))))))
+ (search-request-data :rasta :q "test"))))))
+ (testing "App collections should filterable as \"app\""
+ (mt/with-temp* [Collection [collection {:name "App collection to find"}]
+ App [_ {:collection_id (:id collection)}]
+ Collection [_ {:name "Another collection to find"}]]
+ (is (partial= [(assoc (select-keys collection [:name])
+ :model "app")]
+ (search-request-data :rasta :q "find" :models "app"))))))
+
+(deftest page-test
+ (testing "Search results should pages with model \"page\""
+ (mt/with-temp* [Dashboard [_ {:name "Not a page but contains important text!"}]
+ Dashboard [page {:name "Page"
+ :description "Contains important text!"
+ :is_app_page true}]]
+ (is (partial= [(assoc (select-keys page [:name :description])
+ :model "page")]
+ (search-request-data :rasta :q "important text" :models "page"))))))
--
GitLab