Skip to content

Commit

Permalink
Migration to add trashed_from_*
Browse files Browse the repository at this point in the history 
We want to record where things were trashed *from* for two purposes:

- first, we want to be able to put things back if they're "untrashed".

- second, we want to be able to enforce permissions *as if* something is
still in its previous location. That is, if we trash a card or a
dashboard from Collection A, the permissions of Collection A should
continue to apply to the card or dashboard (e.g. in terms of who can
view it).

To achieve this, collections get a `trashed_from_location` (paralleling
their `location`) and dashboards/cards get a
`trashed_from_collection_id` (paralleling their `collection_id`).
  • Loading branch information
@johnswanson
johnswanson committed Apr 18, 2024
1 parent a2cdc13 commit 09672cb
Show file tree
Hide file tree
Showing 6 changed files with 264 additions and 206 deletions.
45 changes: 45 additions & 0 deletions resources/migrations/001_update_migrations.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6188,6 +6188,51 @@ databaseChangeLog:
- customChange:
class: "metabase.db.custom_migrations.BackfillQueryField"

- changeSet:
id: v50.2024-04-12T13:24:09
author: johnswanson
comment: Add `collection.trashed_from_location`
changes:
- addColumn:
tableName: collection
columns:
- column:
name: trashed_from_location
type: ${text.type}
remarks: "The previous location this collection was trashed from"
constraints:
nullable: true

- changeSet:
id: v50.2024-04-12T13:43:12
author: johnswanson
comment: Add `report_card.trashed_from_collection_id`
changes:
- addColumn:
tableName: report_card
columns:
- column:
name: trashed_from_collection_id
type: int
remarks: "The previous parent collection this card was trashed *from*"
constraints:
nullable: true

- changeSet:
id: v50.2024-04-12T13:43:20
author: johnswanson
comment: Add `report_dashboard.trashed_from_collection_id`
changes:
- addColumn:
tableName: report_dashboard
columns:
- column:
name: trashed_from_collection_id
type: int
remarks: "The previous parent collection this dashboard was trashed *from*"
constraints:
nullable: true

# >>>>>>>>>> DO NOT ADD NEW MIGRATIONS BELOW THIS LINE! ADD THEM ABOVE <<<<<<<<<<

########################################################################################################################
Expand Down
47 changes: 24 additions & 23 deletions test/metabase/api/card_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,29 +63,30 @@

(def card-defaults
"The default card params."
{:archived false
:collection_id nil
:collection_position nil
:collection_preview true
:dataset_query {}
:type "question"
:description nil
:display "scalar"
:enable_embedding false
:initially_published_at nil
:entity_id nil
:embedding_params nil
:made_public_by_id nil
:parameters []
:parameter_mappings []
:moderation_reviews ()
:public_uuid nil
:query_type nil
:cache_ttl nil
:average_query_time nil
:last_query_start nil
:result_metadata nil
:cache_invalidated_at nil})
{:archived false
:collection_id nil
:collection_position nil
:collection_preview true
:dataset_query {}
:type "question"
:description nil
:display "scalar"
:enable_embedding false
:initially_published_at nil
:entity_id nil
:embedding_params nil
:made_public_by_id nil
:parameters []
:parameter_mappings []
:moderation_reviews ()
:public_uuid nil
:query_type nil
:cache_ttl nil
:average_query_time nil
:last_query_start nil
:result_metadata nil
:cache_invalidated_at nil
:trashed_from_collection_id nil})

;; Used in dashboard tests
(def card-defaults-no-hydrate
Expand Down
43 changes: 22 additions & 21 deletions test/metabase/api/dashboard_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,27 +200,28 @@
:parameters "abc"})))))

(def ^:private dashboard-defaults
{:archived false
: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
:enable_embedding false
:initially_published_at nil
:entity_id true
:made_public_by_id nil
:parameters []
:points_of_interest nil
:cache_ttl nil
:position nil
:width "fixed"
:public_uuid nil
:auto_apply_filters true
:show_in_getting_started false
:updated_at true})
{:archived false
: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
:enable_embedding false
:initially_published_at nil
:entity_id true
:made_public_by_id nil
:parameters []
:points_of_interest nil
:cache_ttl nil
:position nil
:width "fixed"
:public_uuid nil
:auto_apply_filters true
:show_in_getting_started false
:updated_at true
:trashed_from_collection_id nil})

(deftest create-dashboard-test
(testing "POST /api/dashboard"
Expand Down
64 changes: 33 additions & 31 deletions test/metabase/events/revision_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,39 +21,41 @@
:creator_id (mt/user->id :crowberto)})

(defn- card->revision-object [card]
{:archived false
:collection_id nil
:collection_position nil
:collection_preview true
:database_id (mt/id)
:dataset_query (:dataset_query card)
:type :question
:description nil
:display :table
:enable_embedding false
:embedding_params nil
:name (:name card)
:parameters []
:parameter_mappings []
:cache_ttl nil
:query_type :query
:table_id (mt/id :categories)
:visualization_settings {}})
{:archived false
:collection_id nil
:collection_position nil
:collection_preview true
:database_id (mt/id)
:dataset_query (:dataset_query card)
:type :question
:description nil
:display :table
:enable_embedding false
:embedding_params nil
:name (:name card)
:parameters []
:parameter_mappings []
:cache_ttl nil
:query_type :query
:table_id (mt/id :categories)
:visualization_settings {}
:trashed_from_collection_id (:trashed_from_collection_id card)})

(defn- dashboard->revision-object [dashboard]
{:collection_id (:collection_id dashboard)
:description nil
:cache_ttl nil
:auto_apply_filters true
:name (:name dashboard)
:width (:width dashboard)
:tabs []
:cards []
:archived false
:collection_position nil
:enable_embedding false
:embedding_params nil
:parameters []})
{:collection_id (:collection_id dashboard)
:description nil
:cache_ttl nil
:auto_apply_filters true
:name (:name dashboard)
:width (:width dashboard)
:tabs []
:cards []
:archived false
:collection_position nil
:enable_embedding false
:embedding_params nil
:parameters []
:trashed_from_collection_id (:trashed_from_collection_id dashboard)})

(deftest card-create-test
(testing :event/card-create
Expand Down
5 changes: 4 additions & 1 deletion test/metabase/models/card_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -839,7 +839,10 @@
:table_id :database_id :query_type
;; we don't need a description for made_public_by_id because whenever this field changes
;; public_uuid will change and we have a description for it.
:made_public_by_id} col)
:made_public_by_id
;; similarly, we don't need a description for `trashed_from_collection_id` because whenever
;; this field changes `archived` will also change and we have a description for that.
:trashed_from_collection_id} col)
(testing (format "we should have a revision description for %s" col)
(is (some? (u/build-sentence
(revision/diff-strings
Expand Down
Loading

0 comments on commit 09672cb

Please sign in to comment.