From 5b64eafbea3caa44dc100c86a622c8c5988110e2 Mon Sep 17 00:00:00 2001
From: bryan <bryan.maass@gmail.com>
Date: Tue, 30 May 2023 15:29:34 -0600
Subject: [PATCH] Add type column to collections + hydrate dashboards and cards
with it (#30994)
* add migration for typed collections
* add hydration of collections to card+dashboard api
- adds it for:
- GET /api/question/:id
- GET /api/dashboard/:id
* fix test that checks for collection shape
* get tests working with hydrated collection
* typo fixes
* fix more tests
- PUT requests don't hydrate :collection
* update tests to fix param_values {} vs nil inconsistencies
---
.../src/metabase_enterprise/audit_db.clj | 2 +-
resources/migrations/000_migrations.yaml | 14 +++++++++++
src/metabase/api/card.clj | 3 ++-
src/metabase/api/dashboard.clj | 13 +++++++++--
src/metabase/models/collection.clj | 2 +-
test/metabase/api/collection_test.clj | 3 ++-
test/metabase/api/dashboard_test.clj | 23 +++++++++++++++----
test/metabase/models/collection_test.clj | 2 +-
8 files changed, 51 insertions(+), 11 deletions(-)
diff --git a/enterprise/backend/src/metabase_enterprise/audit_db.clj b/enterprise/backend/src/metabase_enterprise/audit_db.clj
index 9a56c282eb7..2a2a5d6d575 100644
--- a/enterprise/backend/src/metabase_enterprise/audit_db.clj
+++ b/enterprise/backend/src/metabase_enterprise/audit_db.clj
@@ -21,7 +21,7 @@
(if (t2/select-one Database :id id)
(install-database! engine (inc id))
(do
- ;; guard against someone manually deletihng the audit-db entry, but not removing the audit-db permissions.
+ ;; guard against someone manually deleting the audit-db entry, but not removing the audit-db permissions.
(t2/delete! :permissions {:where [:like :object (str "%/db/" id "/%")]})
(t2/insert! Database {:is_audit true
:id default-audit-db-id
diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index a82d5c413da..28b79cb3e84 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/resources/migrations/000_migrations.yaml
@@ -14798,6 +14798,20 @@ databaseChangeLog:
- customChange:
class: "metabase.db.custom_migrations.DowngradeDashboardTab"
+ - changeSet:
+ id: v47.00-030
+ author: escherize
+ comment: Added 0.47.0 - Type column for collections for instance-analytics
+ changes:
+ - addColumn:
+ tableName: collection
+ columns:
+ - column:
+ name: type
+ type: varchar(256)
+ defaultValue: null
+ remarks: 'This is used to differentiate instance-analytics collections from all other collections.'
+
# >>>>>>>>>> DO NOT ADD NEW MIGRATIONS BELOW THIS LINE! ADD THEM ABOVE <<<<<<<<<<
########################################################################################################################
diff --git a/src/metabase/api/card.clj b/src/metabase/api/card.clj
index f8e24279052..805a6b4f24b 100644
--- a/src/metabase/api/card.clj
+++ b/src/metabase/api/card.clj
@@ -192,7 +192,8 @@
:can_write
:average_query_time
:last_query_start
- :collection [:moderation_reviews :moderator_details])
+ :collection
+ [:moderation_reviews :moderator_details])
(cond-> ;; card
(:dataset raw-card) (hydrate :persisted))
api/read-check
diff --git a/src/metabase/api/dashboard.clj b/src/metabase/api/dashboard.clj
index bc5dca62cda..d8a4d7ac974 100644
--- a/src/metabase/api/dashboard.clj
+++ b/src/metabase/api/dashboard.clj
@@ -217,8 +217,17 @@
;; i'm a bit worried that this is an n+1 situation here. The cards can be batch hydrated i think because they
;; have a hydration key and an id. moderation_reviews currently aren't batch hydrated but i'm worried they
;; cannot be in this situation
- (hydrate [:ordered_cards [:card [:moderation_reviews :moderator_details]] :series :dashcard/action :dashcard/linkcard-info]
- :ordered_tabs :collection_authority_level :can_write :param_fields :param_values)
+ (hydrate [:ordered_cards
+ [:card [:moderation_reviews :moderator_details]]
+ :series
+ :dashcard/action
+ :dashcard/linkcard-info]
+ :ordered_tabs
+ :collection_authority_level
+ :can_write
+ :param_fields
+ :param_values
+ :collection)
api/read-check
api/check-not-archived
hide-unreadable-cards
diff --git a/src/metabase/models/collection.clj b/src/metabase/models/collection.clj
index ed9f1b0face..c556c48b8ab 100644
--- a/src/metabase/models/collection.clj
+++ b/src/metabase/models/collection.clj
@@ -1181,7 +1181,7 @@
:allowed-namespaces allowed-namespaces
:collection-namespace collection-namespace})))))))
-(defn annotate-collections
+(defn- annotate-collections
"Annotate collections with `:below` and `:here` keys to indicate which types are in their subtree and which types are
in the collection at that level."
[{:keys [dataset card] :as _coll-type-ids} collections]
diff --git a/test/metabase/api/collection_test.clj b/test/metabase/api/collection_test.clj
index 1aad6e168bc..8e6467cb610 100644
--- a/test/metabase/api/collection_test.clj
+++ b/test/metabase/api/collection_test.clj
@@ -1450,7 +1450,8 @@
:location (s/eq "/")
:entity_id (s/maybe su/NanoIdString)
:namespace (s/eq nil)
- :created_at java.time.temporal.Temporal}
+ :created_at java.time.temporal.Temporal
+ :type (s/maybe s/Str)}
(mt/user-http-request :crowberto :post 200 "collection"
{:name "foo", :color "#f38630", :authority_level "official"})))
(testing "But they have to be valid types"
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index 712d647e02f..58c7e9324d1 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -89,7 +89,8 @@
:updated_at (boolean updated_at)
:card (-> (into {} card)
(dissoc :id :database_id :table_id :created_at :updated_at :query_average_duration)
- (update :collection_id boolean)))))
+ (update :collection_id boolean)
+ (update :collection boolean)))))
(defn- dashboard-response [{:keys [creator ordered_cards created_at updated_at] :as dashboard}]
;; todo: should be udpated to use mt/boolean-ids-and-timestamps
@@ -98,7 +99,11 @@
(assoc :created_at (boolean created_at)
:updated_at (boolean updated_at))
(update :entity_id boolean)
- (update :collection_id boolean))]
+ (update :collection_id boolean)
+ (update :collection boolean)
+ (cond->
+ (:param_values dashboard)
+ (update :param_values not-empty)))]
(cond-> dash
(contains? dash :last-edit-info)
(update :last-edit-info (fn [info]
@@ -155,6 +160,7 @@
:caveats nil
:collection_id nil
:collection_position nil
+ :collection true
:created_at true ; assuming you call dashboard-response on the results
:description nil
:embedding_params nil
@@ -185,6 +191,7 @@
:updated_at true
:created_at true
:collection_id true
+ :collection false
:cache_ttl 1234
:last-edit-info {:timestamp true :id true :first_name "Rasta"
:last_name "Toucan" :email "rasta@metabase.com"}})
@@ -302,6 +309,7 @@
dashboard-defaults
{:name "Test Dashboard"
:creator_id (mt/user->id :rasta)
+ :collection true
:collection_id true
:collection_authority_level nil
:can_write false
@@ -389,7 +397,7 @@
:collection_id true
:collection_authority_level nil
:can_write false
- :param_values nil
+ :param_values nil
:param_fields {field-id {:id field-id
:table_id table-id
:display_name display-name
@@ -416,6 +424,7 @@
{:name "Dashboard Test Card"
:creator_id (mt/user->id :rasta)
:collection_id true
+ :collection false
:entity_id (:entity_id card)
:display "table"
:query_type nil
@@ -495,6 +504,7 @@
(testing "GET before update"
(is (= (merge dashboard-defaults {:name "Test Dashboard"
:creator_id (mt/user->id :rasta)
+ :collection false
:collection_id true})
(dashboard-response (t2/select-one Dashboard :id dashboard-id)))))
@@ -505,6 +515,7 @@
:cache_ttl 1234
:last-edit-info {:timestamp true :id true :first_name "Rasta"
:last_name "Toucan" :email "rasta@metabase.com"}
+ :collection false
:collection_id true})
(dashboard-response
(mt/user-http-request :rasta :put 200 (str "dashboard/" dashboard-id)
@@ -519,6 +530,7 @@
:description "Some awesome description"
:cache_ttl 1234
:creator_id (mt/user->id :rasta)
+ :collection false
:collection_id true})
(dashboard-response (t2/select-one Dashboard :id dashboard-id)))))
@@ -556,6 +568,7 @@
(with-dashboards-in-writeable-collection [dashboard-id]
(is (= (merge dashboard-defaults {:name "Test Dashboard"
:creator_id (mt/user->id :rasta)
+ :collection false
:collection_id true
:caveats ""
:points_of_interest ""
@@ -826,6 +839,7 @@
{:name "Test Dashboard"
:description "A description"
:creator_id (mt/user->id :rasta)
+ :collection false
:collection_id false})
(dashboard-response response)))
(is (some? (:entity_id response)))
@@ -846,7 +860,8 @@
{:name "Test Dashboard - Duplicate"
:description "A new description"
:creator_id (mt/user->id :crowberto)
- :collection_id false})
+ :collection_id false
+ :collection false})
(dashboard-response response)))
(is (some? (:entity_id response)))
(is (not= (:entity_id dashboard) (:entity_id response))
diff --git a/test/metabase/models/collection_test.clj b/test/metabase/models/collection_test.clj
index 82d393fa643..5b749d9257e 100644
--- a/test/metabase/models/collection_test.clj
+++ b/test/metabase/models/collection_test.clj
@@ -1617,7 +1617,7 @@
{:id 3 :name "c" :location "/1/2/" :here #{:dataset} :below #{:dataset}}
{:id 4 :name "d" :location "/1/2/3/" :here #{:dataset}}
{:id 5 :name "e" :location "/1/" :here #{:card}}]
- (collection/annotate-collections {:card #{1 5} :dataset #{3 4}} collections)))
+ (#'collection/annotate-collections {:card #{1 5} :dataset #{3 4}} collections)))
(is (= [{:id 1 :here #{:card} :below #{:card :dataset}
:children
[{:id 2 :below #{:dataset}
--
GitLab