Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Localize the name of the Trash #42514

Merged
merged 1 commit into from
May 13, 2024
Merged

Localize the name of the Trash #42514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/metabase/api/collection.clj
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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.
Comment on lines +125 to +127
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've run into issues like this before...may be a good topic for a backend guild discussion at some point. Feels like the older a migration is, the more likely there will be conflicts like this in the tests, so maybe we can deprecate migration tests after a certain number of versions...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking the same thing - especially for non-custom migrations. It seems pretty unlikely that an old SQL migration will start behaving unexpectedly after many versions (Clojure-based migrations seem more important to keep testing).

(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