Localize the name of the Trash (#42514)

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

@@ -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)

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

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) %)]

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"

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"