Add action.dataset_query to search results (#29399)

* Add action.dataset_query to search results

* Add action.database_id to search results

* Unskip repro

* better FE error handling for action query display

* update repro

---------

Co-authored-by: Ryan Laurie <iethree@gmail.com>

e2e/test/scenarios/models/reproductions/29378-actions-search-crash.cy.spec.js

@@ -26,7 +26,7 @@ const ACTION_DETAILS = {
   },
 };
 
-describe.skip("issue 29378", () => {
+describe("issue 29378", () => {
   beforeEach(() => {
     restore();
     cy.signInAsAdmin();
@@ -40,6 +40,7 @@ describe.skip("issue 29378", () => {
     cy.visit(`/model/${MODEL_ID}/detail`);
     cy.findByRole("tab", { name: "Actions" }).click();
     cy.findByText(ACTION_DETAILS.name).should("be.visible");
+    cy.findByText(ACTION_DETAILS.dataset_query.native.query).should("be.visible");
 
     cy.findByRole("tab", { name: "Used by" }).click();
     cy.findByPlaceholderText("Search…").type(ACTION_DETAILS.name);
@@ -47,5 +48,6 @@ describe.skip("issue 29378", () => {
 
     cy.findByRole("tab", { name: "Actions" }).click();
     cy.findByText(ACTION_DETAILS.name).should("be.visible");
+    cy.findByText(ACTION_DETAILS.dataset_query.native.query).should("be.visible");
   });
 });

frontend/src/metabase/models/components/ModelDetailPage/ModelActionDetails/ModelActionListItem.tsx

@@ -3,6 +3,7 @@ import { t } from "ttag";
 import Link from "metabase/core/components/Link";
 import EntityMenu from "metabase/components/EntityMenu";
 import ModalWithTrigger from "metabase/components/ModalWithTrigger";
+import Icon from "metabase/components/Icon";
 import { useConfirmation } from "metabase/hooks/use-confirmation";
 import ActionExecuteModal from "metabase/actions/containers/ActionExecuteModal";
 import { WritebackAction, WritebackQueryAction } from "metabase-types/api";
@@ -33,6 +34,14 @@ interface ModalProps {
 }
 
 function QueryActionCardContent({ action }: { action: WritebackQueryAction }) {
+  if (!action.dataset_query?.native?.query) {
+    return (
+      <CodeBlock>
+        <Icon name="warning" size={16} tooltip={t`No query found`} />
+      </CodeBlock>
+    );
+  }
+
   return <CodeBlock>{action.dataset_query.native.query}</CodeBlock>;
 }
 

src/metabase/api/search.clj

@@ -81,19 +81,21 @@
    :updated_at          :timestamp
    ;; returned for Card only
    :dashboardcard_count :integer
-   :dataset_query       :text
    :moderated_status    :text
    ;; returned for Metric and Segment
    :table_id            :integer
-   :database_id         :integer
    :table_schema        :text
    :table_name          :text
    :table_description   :text
+   ;; returned for Metric, Segment, and Action
+   :database_id         :integer
    ;; returned for Database and Table
    :initial_sync_status :text
    ;; returned for Action
    :model_id            :integer
-   :model_name          :text))
+   :model_name          :text
+   ;; returned for Card and Action
+   :dataset_query       :text))
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                               Shared Query Logic                                               |
@@ -283,6 +285,8 @@
   (-> (base-query-for-model model search-ctx)
       (sql.helpers/left-join [:report_card :model]
                              [:= :model.id :action.model_id])
+      (sql.helpers/left-join :query_action
+                             [:= :query_action.action_id :action.id])
       (add-collection-join-and-where-clauses :model.collection_id search-ctx)))
 
 (s/defmethod search-query-for-model "card"

src/metabase/search/config.clj

@@ -132,9 +132,11 @@
 (defmethod columns-for-model "action"
   [_]
   (conj default-columns :model_id
-        [:model.collection_id :collection_id]
-        [:model.id            :model_id]
-        [:model.name          :model_name]))
+        [:model.collection_id        :collection_id]
+        [:model.id                   :model_id]
+        [:model.name                 :model_name]
+        [:query_action.database_id   :database_id]
+        [:query_action.dataset_query :dataset_query]))
 
 (defmethod columns-for-model "card"
   [_]

test/metabase/api/search_test.clj

@@ -18,6 +18,7 @@
             PermissionsGroupMembership
             Pulse
             PulseCard
+            QueryAction
             Segment
             Table]]
    [metabase.models.permissions :as perms]
@@ -82,6 +83,12 @@
   [name & kvs]
   (apply assoc default-search-row :name name kvs))
 
+(defn- query-action
+  [action-id]
+  {:action_id     action-id
+   :database_id   (u/the-id (mt/db))
+   :dataset_query (mt/query venues)})
+
 (def ^:private test-collection (make-result "collection test collection"
                                             :bookmark false
                                             :model "collection"
@@ -96,7 +103,8 @@
     test-collection
     (make-result "card test card", :model "card", :bookmark false, :dataset_query nil, :dashboardcard_count 0)
     (make-result "dataset test dataset", :model "dataset", :bookmark false, :dataset_query nil, :dashboardcard_count 0)
-    (make-result "action test action", :model "action", :model_name (:name action-model-params), :model_id true)
+    (make-result "action test action", :model "action", :model_name (:name action-model-params), :model_id true,
+                 :dataset_query (update (mt/query venues) :type name), :database_id true)
     (merge
      (make-result "metric test metric", :model "metric", :description "Lookin' for a blueberry")
      (table-search-results))
@@ -130,18 +138,20 @@
                         (merge (data-map instance-name)
                                (when-not in-root-collection?
                                  {:collection_id (u/the-id collection)})))]
-    (mt/with-temp* [Collection [coll         (data-map "collection %s collection")]
-                    Card       [action-model (if in-root-collection?
-                                               action-model-params
-                                               (assoc action-model-params :collection_id (u/the-id coll)))]
-                    Action     [action       (merge (data-map "action %s action")
-                                                    {:type :query, :model_id (u/the-id action-model)})]
-                    Card       [card         (coll-data-map "card %s card" coll)]
-                    Card       [dataset      (assoc (coll-data-map "dataset %s dataset" coll)
-                                                    :dataset true)]
-                    Dashboard  [dashboard    (coll-data-map "dashboard %s dashboard" coll)]
-                    Metric     [metric       (data-map "metric %s metric")]
-                    Segment    [segment      (data-map "segment %s segment")]]
+    (mt/with-temp* [Collection  [coll           (data-map "collection %s collection")]
+                    Card        [action-model   (if in-root-collection?
+                                                  action-model-params
+                                                  (assoc action-model-params :collection_id (u/the-id coll)))]
+                    Action      [{action-id :id
+                                  :as action}   (merge (data-map "action %s action")
+                                  {:type :query, :model_id (u/the-id action-model)})]
+                    QueryAction [_qa (query-action action-id)]
+                    Card        [card           (coll-data-map "card %s card" coll)]
+                    Card        [dataset        (assoc (coll-data-map "dataset %s dataset" coll)
+                                                       :dataset true)]
+                    Dashboard   [dashboard      (coll-data-map "dashboard %s dashboard" coll)]
+                    Metric      [metric         (data-map "metric %s metric")]
+                    Segment     [segment        (data-map "segment %s segment")]]
       (f {:action     action
           :collection coll
           :card       card
@@ -220,11 +230,11 @@
              [:like [:lower :display_name]      "%foo%"] [:inline 0]
              [:like [:lower :description]       "%foo%"] [:inline 0]
              [:like [:lower :collection_name]   "%foo%"] [:inline 0]
-             [:like [:lower :dataset_query]     "%foo%"] [:inline 0]
              [:like [:lower :table_schema]      "%foo%"] [:inline 0]
              [:like [:lower :table_name]        "%foo%"] [:inline 0]
              [:like [:lower :table_description] "%foo%"] [:inline 0]
              [:like [:lower :model_name]        "%foo%"] [:inline 0]
+             [:like [:lower :dataset_query]     "%foo%"] [:inline 0]
              :else [:inline 1]]]
            (api.search/order-clause "Foo")))))
 
@@ -468,50 +478,53 @@
 (deftest archived-results-test
   (testing "Should return unarchived results by default"
     (with-search-items-in-root-collection "test"
-      (mt/with-temp* [Card       [action-model {:dataset true}]
-                      Action     [_ (archived {:name     "action test action 2"
-                                               :type     :query
-                                               :model_id (u/the-id action-model)})]
-                      Card       [_ (archived {:name "card test card 2"})]
-                      Card       [_ (archived {:name "dataset test dataset" :dataset true})]
-                      Dashboard  [_ (archived {:name "dashboard test dashboard 2"})]
-                      Collection [_ (archived {:name "collection test collection 2"})]
-                      Metric     [_ (archived {:name "metric test metric 2"})]
-                      Segment    [_ (archived {:name "segment test segment 2"})]]
+      (mt/with-temp* [Card        [action-model {:dataset true}]
+                      Action      [{action-id :id} (archived {:name     "action test action 2"
+                                                              :type     :query
+                                                              :model_id (u/the-id action-model)})]
+                      QueryAction [_ (query-action action-id)]
+                      Card        [_ (archived {:name "card test card 2"})]
+                      Card        [_ (archived {:name "dataset test dataset" :dataset true})]
+                      Dashboard   [_ (archived {:name "dashboard test dashboard 2"})]
+                      Collection  [_ (archived {:name "collection test collection 2"})]
+                      Metric      [_ (archived {:name "metric test metric 2"})]
+                      Segment     [_ (archived {:name "segment test segment 2"})]]
         (is (= (default-search-results)
                (search-request-data :crowberto :q "test"))))))
 
   (testing "Should return archived results when specified"
     (with-search-items-in-root-collection "test2"
-      (mt/with-temp* [Card       [action-model action-model-params]
-                      Action     [_ (archived {:name     "action test action"
-                                               :type     :query
-                                               :model_id (u/the-id action-model)})]
-                      Action     [_ (archived {:name     "action that will not appear in results"
-                                               :type     :query
-                                               :model_id (u/the-id action-model)})]
-                      Card       [_ (archived {:name "card test card"})]
-                      Card       [_ (archived {:name "card that will not appear in results"})]
-                      Card       [_ (archived {:name "dataset test dataset" :dataset true})]
-                      Dashboard  [_ (archived {:name "dashboard test dashboard"})]
-                      Collection [_ (archived {:name "collection test collection"})]
-                      Metric     [_ (archived {:name "metric test metric"})]
-                      Segment    [_ (archived {:name "segment test segment"})]]
+      (mt/with-temp* [Card        [action-model action-model-params]
+                      Action      [{action-id :id} (archived {:name     "action test action"
+                                                              :type     :query
+                                                              :model_id (u/the-id action-model)})]
+                      QueryAction [_ (query-action action-id)]
+                      Action      [_ (archived {:name     "action that will not appear in results"
+                                                :type     :query
+                                                :model_id (u/the-id action-model)})]
+                      Card        [_ (archived {:name "card test card"})]
+                      Card        [_ (archived {:name "card that will not appear in results"})]
+                      Card        [_ (archived {:name "dataset test dataset" :dataset true})]
+                      Dashboard   [_ (archived {:name "dashboard test dashboard"})]
+                      Collection  [_ (archived {:name "collection test collection"})]
+                      Metric      [_ (archived {:name "metric test metric"})]
+                      Segment     [_ (archived {:name "segment test segment"})]]
         (is (= (default-archived-results)
                (search-request-data :crowberto :q "test", :archived "true"))))))
 
   (testing "Should return archived results when specified without a search query"
     (with-search-items-in-root-collection "test2"
-      (mt/with-temp* [Card       [action-model action-model-params]
-                      Action     [_ (archived {:name     "action test action"
-                                               :type     :query
-                                               :model_id (u/the-id action-model)})]
-                      Card       [_ (archived {:name "card test card"})]
-                      Card       [_ (archived {:name "dataset test dataset" :dataset true})]
-                      Dashboard  [_ (archived {:name "dashboard test dashboard"})]
-                      Collection [_ (archived {:name "collection test collection"})]
-                      Metric     [_ (archived {:name "metric test metric"})]
-                      Segment    [_ (archived {:name "segment test segment"})]]
+      (mt/with-temp* [Card        [action-model action-model-params]
+                      Action      [{action-id :id} (archived {:name     "action test action"
+                                                :type     :query
+                                                :model_id (u/the-id action-model)})]
+                      QueryAction [_ (query-action action-id)]
+                      Card        [_ (archived {:name "card test card"})]
+                      Card        [_ (archived {:name "dataset test dataset" :dataset true})]
+                      Dashboard   [_ (archived {:name "dashboard test dashboard"})]
+                      Collection  [_ (archived {:name "collection test collection"})]
+                      Metric      [_ (archived {:name "metric test metric"})]
+                      Segment     [_ (archived {:name "segment test segment"})]]
         (is (ordered-subset? (default-archived-results)
                              (search-request-data :crowberto :archived "true")))))))
 
@@ -547,10 +560,10 @@
 
 (deftest table-test
   (testing "You should see Tables in the search results!\n"
-    (mt/with-temp Table [_ {:name "Round Table"}]
+    (mt/with-temp Table [_ {:name "RoundTable"}]
       (do-test-users [user [:crowberto :rasta]]
-        (is (= [(default-table-search-row "Round Table")]
-               (search-request-data user :q "Round Table"))))))
+        (is (= [(default-table-search-row "RoundTable")]
+               (search-request-data user :q "RoundTable"))))))
   (testing "You should not see hidden tables"
     (mt/with-temp* [Table [_normal {:name "Foo Visible"}]
                     Table [_hidden {:name "Foo Hidden", :visibility_type "hidden"}]]
@@ -559,9 +572,9 @@
                (search-request-data user :q "Foo"))))))
   (testing "You should be able to search by their display name"
     (let [lancelot "Lancelot's Favorite Furniture"]
-      (mt/with-temp Table [_ {:name "Round Table" :display_name lancelot}]
+      (mt/with-temp Table [_ {:name "RoundTable" :display_name lancelot}]
         (do-test-users [user [:crowberto :rasta]]
-          (is (= [(assoc (default-table-search-row "Round Table") :name lancelot)]
+          (is (= [(assoc (default-table-search-row "RoundTable") :name lancelot)]
                  (search-request-data user :q "Lancelot")))))))
   (testing "When searching with ?archived=true, normal Tables should not show up in the results"
     (let [table-name (mt/random-name)]
@@ -587,25 +600,25 @@
   (testing (str "If the All Users group doesn't have perms to view a Table, but the current User is in a group that "
                 "does have perms, they should still be able to see it (#12332)")
     (mt/with-temp* [Database                   [{db-id :id}]
-                    Table                      [table {:name "Round Table", :db_id db-id}]
+                    Table                      [table {:name "RoundTable", :db_id db-id}]
                     PermissionsGroup           [{group-id :id}]
                     PermissionsGroupMembership [_ {:group_id group-id, :user_id (mt/user->id :rasta)}]]
       (perms/revoke-data-perms! (perms-group/all-users) db-id (:schema table) (:id table))
       (perms/grant-permissions! group-id (perms/table-read-path table))
       (do-test-users [user [:crowberto :rasta]]
-        (is (= [(default-table-search-row "Round Table")]
+        (is (= [(default-table-search-row "RoundTable")]
                (binding [*search-request-results-database-id* db-id]
-                 (search-request-data user :q "Round Table"))))))))
+                 (search-request-data user :q "RoundTable"))))))))
 
 (deftest all-users-no-data-perms-table-test
   (testing "If the All Users group doesn't have perms to view a Table they sholdn't see it (#16855)"
     (mt/with-temp* [Database                   [{db-id :id}]
-                    Table                      [table {:name "Round Table", :db_id db-id}]]
+                    Table                      [table {:name "RoundTable", :db_id db-id}]]
       (perms/revoke-data-perms! (perms-group/all-users) db-id (:schema table) (:id table))
       (is (= []
-             (filter #(= (:name %) "Round Table")
+             (filter #(= (:name %) "RoundTable")
                      (binding [*search-request-results-database-id* db-id]
-                       (search-request-data :rasta :q "Round Table"))))))))
+                       (search-request-data :rasta :q "RoundTable"))))))))
 
 (deftest collection-namespaces-test
   (testing "Search should only return Collections in the 'default' namespace"