From ff2f1ea1afacb4bc20ad8da98b2b31140b70c84b Mon Sep 17 00:00:00 2001 From: John Swanson Date: Fri, 10 May 2024 06:47:08 -0700 Subject: [PATCH] Localize the name of the Trash There are at least two spots where we can't just rely on the after-select hook, and select the collection name directly from the database: the Search and Collection API. In these cases we need to call `collection/maybe-localize-trash-name`, which will localize the name if the passed Collection is the Trash collection. --- src/metabase/api/collection.clj | 1 + src/metabase/api/search.clj | 7 ++- src/metabase/models/collection.clj | 28 +++++++++--- src/metabase/search/config.clj | 3 ++ test/metabase/db/schema_migrations_test.clj | 48 ++++++++++++--------- 5 files changed, 59 insertions(+), 28 deletions(-) diff --git a/src/metabase/api/collection.clj b/src/metabase/api/collection.clj index 5616f5f49c279..e7b6305068ad2 100644 --- a/src/metabase/api/collection.clj +++ b/src/metabase/api/collection.clj @@ -626,6 +626,7 @@ (dissoc row :personal_owner_id)))] (for [row (annotate-collections parent-collection rows)] (-> (t2/hydrate (t2/instance :model/Collection row) :can_write :effective_location) + collection/maybe-localize-trash-name (dissoc :collection_position :display :moderated_status :icon :collection_preview :dataset_query :table_id :query_type :is_upload) (update :archived api/bit->boolean) diff --git a/src/metabase/api/search.clj b/src/metabase/api/search.clj index 33c399dfae113..675aa443034e8 100644 --- a/src/metabase/api/search.clj +++ b/src/metabase/api/search.clj @@ -480,7 +480,11 @@ (for [item search-results :when (= (:model item) "dataset")] [(:collection_id item) - {:name (:collection_name item) + {:name (-> {:name (:collection_name item) + :id (:collection_id item) + :type (:collection_type item)} + collection/maybe-localize-trash-name + :name) :effective_location (result->loc item)}])) ;; the set of all collection IDs where we *don't* know the collection name. For example, if `col-id->name&loc` ;; contained `{1 {:effective_location "/2/" :name "Foo"}}`, we need to look up the name of collection `2`. @@ -529,6 +533,7 @@ (map #(if (t2/instance-of? :model/Collection %) (t2/hydrate % :effective_location) (assoc % :effective_location nil))) + (map #(cond-> % (t2/instance-of? :model/Collection %) collection/maybe-localize-trash-name)) (filter (partial check-permissions-for-model (:archived? search-ctx))) ;; MySQL returns `:bookmark` and `:archived` as `1` or `0` so convert those to boolean as ;; needed diff --git a/src/metabase/models/collection.clj b/src/metabase/models/collection.clj index 9f3ee5fa8baba..e44d279f84091 100644 --- a/src/metabase/models/collection.clj +++ b/src/metabase/models/collection.clj @@ -45,8 +45,12 @@ "Maximum number of characters allowed in a Collection `slug`." 510) +(def ^:constant trash-collection-type + "The value of the `:type` field for the Trash collection that holds archived items." + "trash") + (defn- trash-collection* [] - (t2/select-one :model/Collection :type "trash")) + (t2/select-one :model/Collection :type trash-collection-type)) (def ^{:arglists '([])} trash-collection "Memoized copy of the Trash collection from the DB." @@ -68,8 +72,11 @@ (defn is-trash? "Is this the trash collection?" [collection] - (and (not (collection.root/is-root-collection? collection)) - (= (u/the-id collection) (trash-collection-id)))) + ;; in some circumstances we don't have a `:type` on the collection (e.g. search or collection items lists, where we + ;; select a subset of columns). Use the type if it's there, but fall back to the ID to be sure. + ;; We can't *only* use the id because getting that requires selecting a collection :sweat-smile: + (or (= (:type collection) trash-collection-type) + (= (:id collection) (trash-collection-id)))) (defn is-trash-or-descendant? "Is this the trash collection, or a descendant of it?" @@ -91,6 +98,17 @@ {:namespace mi/transform-keyword :authority_level mi/transform-keyword}) +(defn maybe-localize-trash-name + "If the collection is the Trash, translate the `name`. This is a public function because we can't rely on + `define-after-select` in all circumstances, e.g. when searching or listing collection items (where we do a direct DB + query without `:model/Collection`)." + [collection] + (cond-> collection + (is-trash? collection) (assoc :name (tru "Trash")))) + +(t2/define-after-select :model/Collection [collection] + (maybe-localize-trash-name collection)) + (doto Collection (derive :metabase/model) (derive :hook/entity-id) @@ -1027,10 +1045,6 @@ "The value of the `:type` field for the `instance-analytics` Collection created in [[metabase-enterprise.audit-db]]" "instance-analytics") -(def trash-collection-type - "The value of the `:type` field for the Trash collection that holds archived items." - "trash") - (defmethod mi/exclude-internal-content-hsql :model/Collection [_model & {:keys [table-alias]}] (let [maybe-alias #(h2x/identifier :field (some-> table-alias name) %)] diff --git a/src/metabase/search/config.clj b/src/metabase/search/config.clj index 8e61caf8cfa4a..e225831a49f64 100644 --- a/src/metabase/search/config.clj +++ b/src/metabase/search/config.clj @@ -273,6 +273,7 @@ [_] (conj default-columns :collection_id :trashed_from_collection_id :collection_position :dataset_query :display :creator_id [:collection.name :collection_name] + [:collection.type :collection_type] [:collection.location :collection_location] [:collection.authority_level :collection_authority_level] bookmark-col dashboardcard-count-col)) @@ -283,6 +284,7 @@ [:model-index.pk_ref :pk_ref] [:model-index.id :model_index_id] [:collection.name :collection_name] + [:collection.type :collection_type] [:model.collection_id :collection_id] [:model.id :model_id] [:model.name :model_name] @@ -292,6 +294,7 @@ [_] (conj default-columns :trashed_from_collection_id :collection_id :collection_position :creator_id bookmark-col [:collection.name :collection_name] + [:collection.type :collection_type] [:collection.authority_level :collection_authority_level])) (defmethod columns-for-model "database" diff --git a/test/metabase/db/schema_migrations_test.clj b/test/metabase/db/schema_migrations_test.clj index 05019b8c26199..734014312f9c7 100644 --- a/test/metabase/db/schema_migrations_test.clj +++ b/test/metabase/db/schema_migrations_test.clj @@ -122,18 +122,22 @@ :created_at #t "2021-10-20T02:09Z" :updated_at #t "2022-10-20T02:09Z"})] (migrate!) - (testing "A personal Collection should get created_at set by to the date_joined from its owner" - (is (= (t/offset-date-time #t "2022-10-20T02:09Z") - (t/offset-date-time (t2/select-one-fn :created_at Collection :id personal-collection-id))))) - (testing "A non-personal Collection should get created_at set to its oldest object" - (is (= (t/offset-date-time #t "2021-10-20T02:09Z") - (t/offset-date-time (t2/select-one-fn :created_at Collection :id impersonal-collection-id))))) - (testing "Empty Collection should not have been updated" - (let [empty-collection-created-at (t/offset-date-time (t2/select-one-fn :created_at Collection :id empty-collection-id))] - (is (not= (t/offset-date-time #t "2021-10-20T02:09Z") - empty-collection-created-at)) - (is (not= (t/offset-date-time #t "2022-10-20T02:09Z") - empty-collection-created-at)))))))) + ;; Urgh. `collection/is-trash?` will select the Trash collection (cached) based on its `type`. But as of this + ;; migration, this `type` does not exist yet. Neither does the Trash collection though, so let's just ... make + ;; that so. + (with-redefs [collection/is-trash? (constantly false)] + (testing "A personal Collection should get created_at set by to the date_joined from its owner" + (is (= (t/offset-date-time #t "2022-10-20T02:09Z") + (t/offset-date-time (t2/select-one-fn :created_at [:model/Collection :created_at] :id personal-collection-id))))) + (testing "A non-personal Collection should get created_at set to its oldest object" + (is (= (t/offset-date-time #t "2021-10-20T02:09Z") + (t/offset-date-time (t2/select-one-fn :created_at [:model/Collection :created_at] :id impersonal-collection-id))))) + (testing "Empty Collection should not have been updated" + (let [empty-collection-created-at (t/offset-date-time (t2/select-one-fn :created_at Collection :id empty-collection-id))] + (is (not= (t/offset-date-time #t "2021-10-20T02:09Z") + empty-collection-created-at)) + (is (not= (t/offset-date-time #t "2022-10-20T02:09Z") + empty-collection-created-at))))))))) (deftest deduplicate-dimensions-test (testing "Migrations v46.00-029 thru v46.00-031: make Dimension field_id unique instead of field_id + name" @@ -498,16 +502,20 @@ (deftest remove-collection-color-test (testing "Migration v48.00-019" (impl/test-migrations ["v48.00-019"] [migrate!] - (let [collection-id (first (t2/insert-returning-pks! (t2/table-name Collection) {:name "Amazing collection" - :slug "amazing_collection" - :color "#509EE3"}))] + ;; Urgh. `collection/is-trash?` will select the Trash collection (cached) based on its `type`. But as of this + ;; migration, this `type` does not exist yet. Neither does the Trash collection though, so let's just ... make + ;; that so. + (with-redefs [collection/is-trash? (constantly false)] + (let [collection-id (first (t2/insert-returning-pks! (t2/table-name Collection) {:name "Amazing collection" + :slug "amazing_collection" + :color "#509EE3"}))] - (testing "Collection should exist and have the color set by the user prior to migration" - (is (= "#509EE3" (:color (t2/select-one :model/Collection :id collection-id))))) + (testing "Collection should exist and have the color set by the user prior to migration" + (is (= "#509EE3" (:color (t2/select-one :model/Collection :id collection-id))))) - (migrate!) - (testing "should drop the existing color column" - (is (not (contains? (t2/select-one :model/Collection :id collection-id) :color)))))))) + (migrate!) + (testing "should drop the existing color column" + (is (not (contains? (t2/select-one :model/Collection :id collection-id) :color))))))))) (deftest audit-v2-views-test (testing "Migrations v48.00-029 - end"