From 901c949c650e820bf83529c61ad0710e7c5df477 Mon Sep 17 00:00:00 2001
From: Mark Bastian <markbastian@gmail.com>
Date: Thu, 11 May 2023 11:13:05 -0600
Subject: [PATCH] Remove x-ray card suffix qualifier (#30646)
Removes display of underlying type of x-ray card (e.g. model, question) in title. Adds tests.
---
src/metabase/automagic_dashboards/core.clj | 6 +-
.../api/automagic_dashboards_test.clj | 45 ++++++++--
.../automagic_dashboards/core_test.clj | 85 +++++++++++++------
3 files changed, 102 insertions(+), 34 deletions(-)
diff --git a/src/metabase/automagic_dashboards/core.clj b/src/metabase/automagic_dashboards/core.clj
index 6a6ff7d064d..8df6221d700 100644
--- a/src/metabase/automagic_dashboards/core.clj
+++ b/src/metabase/automagic_dashboards/core.clj
@@ -257,14 +257,14 @@
(defmethod ->root Card
[card]
- (let [source (source card)]
+ (let [{:keys [dataset] :as source} (source card)]
{:entity card
:source source
:database (:database_id card)
:query-filter (get-in card [:dataset_query :query :filter])
- :full-name (tru "\"{0}\" question" (:name card))
+ :full-name (tru "\"{0}\"" (:name card))
:short-name (source-name {:source source})
- :url (format "%squestion/%s" public-endpoint (u/the-id card))
+ :url (format "%s%s/%s" public-endpoint (if dataset "model" "question") (u/the-id card))
:rules-prefix [(if (table-like? card)
"table"
"question")]}))
diff --git a/test/metabase/api/automagic_dashboards_test.clj b/test/metabase/api/automagic_dashboards_test.clj
index bd9a6eef329..db9bdb26d26 100644
--- a/test/metabase/api/automagic_dashboards_test.clj
+++ b/test/metabase/api/automagic_dashboards_test.clj
@@ -92,7 +92,7 @@
[collection-id]
(perms/revoke-collection-permissions! (perms-group/all-users) collection-id))
-(deftest card-xray-test
+(deftest question-xray-test
(mt/with-non-admin-groups-no-root-collection-perms
(let [cell-query (#'magic/encode-base64-json [:> [:field (mt/id :venues :price) nil] 5])]
(doseq [test-fn
@@ -112,12 +112,43 @@
[card-id cell-query]
#(revoke-collection-permissions! collection-id))))))]]
(tt/with-temp* [Collection [{collection-id :id}]
- Card [{card-id :id} {:table_id (mt/id :venues)
- :collection_id collection-id
- :dataset_query (mt/mbql-query venues
- {:filter [:> $price 10]})}]]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
- (test-fn collection-id card-id))))))
+ Card [{card-id :id} {:table_id (mt/id :venues)
+ :collection_id collection-id
+ :dataset_query (mt/mbql-query venues
+ {:filter [:> $price 10]})}]]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
+ (test-fn collection-id card-id))))))
+
+
+(deftest model-xray-test
+ (testing "The API surface of a model (dataset = true) is very much like that of a question,
+ even though the underlying API will assert that dataset is true and the returned dashboard will be different."
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (let [cell-query (#'magic/encode-base64-json [:> [:field (mt/id :venues :price) nil] 5])]
+ (doseq [test-fn
+ [(fn [collection-id card-id]
+ (testing "GET /api/automagic-dashboards/model/:id"
+ (is (some? (api-call "model/%s" [card-id] #(revoke-collection-permissions! collection-id))))))
+
+ (fn [collection-id card-id]
+ (testing "GET /api/automagic-dashboards/model/:id/cell/:cell-query"
+ (is (some? (api-call "model/%s/cell/%s"
+ [card-id cell-query]
+ #(revoke-collection-permissions! collection-id))))))
+
+ (fn [collection-id card-id]
+ (testing "GET /api/automagic-dashboards/model/:id/cell/:cell-query/rule/example/indepth"
+ (is (some? (api-call "model/%s/cell/%s/rule/example/indepth"
+ [card-id cell-query]
+ #(revoke-collection-permissions! collection-id))))))]]
+ (tt/with-temp* [Collection [{collection-id :id}]
+ Card [{card-id :id} {:table_id (mt/id :venues)
+ :collection_id collection-id
+ :dataset_query (mt/mbql-query venues
+ {:filter [:> $price 10]})
+ :dataset true}]]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection-id)
+ (test-fn collection-id card-id)))))))
(deftest adhoc-query-xray-test
(let [query (#'magic/encode-base64-json
diff --git a/test/metabase/automagic_dashboards/core_test.clj b/test/metabase/automagic_dashboards/core_test.clj
index 1eafdd174f1..cf877c18dc4 100644
--- a/test/metabase/automagic_dashboards/core_test.clj
+++ b/test/metabase/automagic_dashboards/core_test.clj
@@ -1,29 +1,30 @@
(ns ^:mb/once metabase.automagic-dashboards.core-test
(:require
- [cheshire.core :as json]
- [clojure.core.async :as a]
- [clojure.set :as set]
- [clojure.test :refer :all]
- [java-time :as t]
- [metabase.api.common :as api]
- [metabase.automagic-dashboards.core :as magic]
- [metabase.automagic-dashboards.rules :as rules]
- [metabase.mbql.schema :as mbql.s]
- [metabase.models :refer [Card Collection Database Field Metric Table]]
- [metabase.models.interface :as mi]
- [metabase.models.permissions :as perms]
- [metabase.models.permissions-group :as perms-group]
- [metabase.models.query :as query :refer [Query]]
- [metabase.query-processor.async :as qp.async]
- [metabase.sync :as sync]
- [metabase.test :as mt]
- [metabase.test.automagic-dashboards :as automagic-dashboards.test]
- [metabase.util :as u]
- [metabase.util.date-2 :as u.date]
- [metabase.util.i18n :refer [tru]]
- [ring.util.codec :as codec]
- [schema.core :as s]
- [toucan2.core :as t2]))
+ [cheshire.core :as json]
+ [clojure.core.async :as a]
+ [clojure.set :as set]
+ [clojure.string :as str]
+ [clojure.test :refer :all]
+ [java-time :as t]
+ [metabase.api.common :as api]
+ [metabase.automagic-dashboards.core :as magic]
+ [metabase.automagic-dashboards.rules :as rules]
+ [metabase.mbql.schema :as mbql.s]
+ [metabase.models :refer [Card Collection Database Field Metric Table]]
+ [metabase.models.interface :as mi]
+ [metabase.models.permissions :as perms]
+ [metabase.models.permissions-group :as perms-group]
+ [metabase.models.query :as query :refer [Query]]
+ [metabase.query-processor.async :as qp.async]
+ [metabase.sync :as sync]
+ [metabase.test :as mt]
+ [metabase.test.automagic-dashboards :as automagic-dashboards.test]
+ [metabase.util :as u]
+ [metabase.util.date-2 :as u.date]
+ [metabase.util.i18n :refer [tru]]
+ [ring.util.codec :as codec]
+ [schema.core :as s]
+ [toucan2.core :as t2]))
(set! *warn-on-reflection* true)
@@ -569,6 +570,42 @@
:when name]
name)))))))))))
+(deftest model-title-does-not-leak-abstraction-test
+ (testing "The title of a model or question card should not be X model or X question, but just X."
+ (mt/dataset sample-dataset
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (let [source-query {:database (mt/id)
+ :query (mt/$ids
+ {:source-table $$products
+ :fields [$products.category
+ $products.price]})
+ :type :query}]
+ (mt/with-temp* [Collection [{collection-id :id}]
+ Card [model-card {:table_id (mt/id :products)
+ :collection_id collection-id
+ :dataset_query source-query
+ :result_metadata (mt/with-test-user
+ :rasta
+ (result-metadata-for-query
+ source-query))
+ :dataset true}]
+ Card [question-card {:table_id (mt/id :products)
+ :collection_id collection-id
+ :dataset_query source-query
+ :result_metadata (mt/with-test-user
+ :rasta
+ (result-metadata-for-query
+ source-query))
+ :dataset false}]]
+ (let [{model-dashboard-name :name} (mt/with-test-user :rasta (magic/automagic-analysis model-card nil))
+ {question-dashboard-name :name} (mt/with-test-user :rasta (magic/automagic-analysis question-card nil))]
+ (is (false? (str/ends-with? model-dashboard-name "question")))
+ (is (false? (str/ends-with? model-dashboard-name "model")))
+ (is (true? (str/ends-with? model-dashboard-name (format "\"%s\"" (:name model-card)))))
+ (is (false? (str/ends-with? question-dashboard-name "question")))
+ (is (false? (str/ends-with? question-dashboard-name "model")))
+ (is (true? (str/ends-with? question-dashboard-name (format "\"%s\"" (:name question-card))))))))))))
+
(deftest model-with-joins-test
;; This model does a join of 3 tables and aliases columns.
;; The created dashboard should use all the data with the correct labels.
--
GitLab