Skip to content

Commit

Permalink
Localize the name of the Trash
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@johnswanson
johnswanson committed May 13, 2024
1 parent 0e92c09 commit ff2f1ea
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 28 deletions.
1 change: 1 addition & 0 deletions src/metabase/api/collection.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
7 changes: 6 additions & 1 deletion src/metabase/api/search.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down Expand Up @@ -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
Expand Down
28 changes: 21 additions & 7 deletions src/metabase/models/collection.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Expand All @@ -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?"
Expand All @@ -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)
Expand Down Expand Up @@ -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) %)]
Expand Down
3 changes: 3 additions & 0 deletions src/metabase/search/config.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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]
Expand All @@ -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"
Expand Down
48 changes: 28 additions & 20 deletions test/metabase/db/schema_migrations_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down

0 comments on commit ff2f1ea

Please sign in to comment.