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 17, 2024
1 parent a2cdc13 commit 76329cc
Show file tree
Hide file tree
Showing 7 changed files with 165 additions and 115 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
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
2 changes: 1 addition & 1 deletion src/metabase/models/card.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@

(def ^:private excluded-columns-for-card-revision
[:id :created_at :updated_at :entity_id :creator_id :public_uuid :made_public_by_id :metabase_version
:initially_published_at :cache_invalidated_at])
:initially_published_at :cache_invalidated_at :trashed_from_collection_id])

(defmethod revision/revert-to-revision! :model/Card
[model id user-id serialized-card]
Expand Down
3 changes: 2 additions & 1 deletion src/metabase/models/dashboard.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,8 @@
;; lower-numbered positions appearing before higher numbered ones.
;; TODO: querying on stats we don't have any dashboard that has a position, maybe we could just drop it?
:public_uuid :made_public_by_id
:position :initially_published_at])
:position :initially_published_at
:trashed_from_collection_id])

(def ^:private excluded-columns-for-dashcard-revision
[:entity_id :created_at :updated_at :collection_authority_level])
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 nil})

(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 nil})

(deftest card-create-test
(testing :event/card-create
Expand Down
76 changes: 38 additions & 38 deletions test/metabase/models/dashboard_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,30 +36,30 @@
DashboardCard {dashcard-id :id} {:dashboard_id dashboard-id, :card_id card-id}
DashboardCardSeries _ {:dashboardcard_id dashcard-id, :card_id series-id-1, :position 0}
DashboardCardSeries _ {:dashboardcard_id dashcard-id, :card_id series-id-2, :position 1}]
(is (= {:name "Test Dashboard"
:auto_apply_filters true
:collection_id nil
:description nil
:cache_ttl nil
:cards [{:size_x 4
:size_y 4
:row 0
:col 0
:id true
:card_id true
:series true
:dashboard_tab_id nil
:action_id nil
:parameter_mappings []
:visualization_settings {}
:dashboard_id dashboard-id}]
:tabs []
:archived false
:collection_position nil
:enable_embedding false
:embedding_params nil
:parameters []
:width "fixed"}
(is (= {:name "Test Dashboard"
:auto_apply_filters true
:collection_id nil
:description nil
:cache_ttl nil
:cards [{:size_x 4
:size_y 4
:row 0
:col 0
:id true
:card_id true
:series true
:dashboard_tab_id nil
:action_id nil
:parameter_mappings []
:visualization_settings {}
:dashboard_id dashboard-id}]
:tabs []
:archived false
:collection_position nil
:enable_embedding false
:embedding_params nil
:parameters []
:width "fixed"}
(update (revision/serialize-instance Dashboard (:id dashboard) dashboard)
:cards
(fn [[{:keys [id card_id series], :as card}]]
Expand All @@ -73,20 +73,20 @@
(t2.with-temp/with-temp [Dashboard {dashboard-id :id :as dashboard} {:name "Test Dashboard"}
:model/DashboardTab {tab-id :id} {:dashboard_id dashboard-id :name "Test Tab" :position 0}
DashboardCard {dashcard-id :id} {:dashboard_id dashboard-id :dashboard_tab_id tab-id}]
(is (=? {:name "Test Dashboard"
:auto_apply_filters true
:collection_id nil
:description nil
:cache_ttl nil
:cards [{:size_x 4
:size_y 4
:row 0
:col 0
:id dashcard-id
:dashboard_tab_id tab-id}]
:tabs [{:id tab-id
:name "Test Tab"
:position 0}]}
(is (=? {:name "Test Dashboard"
:auto_apply_filters true
:collection_id nil
:description nil
:cache_ttl nil
:cards [{:size_x 4
:size_y 4
:row 0
:col 0
:id dashcard-id
:dashboard_tab_id tab-id}]
:tabs [{:id tab-id
:name "Test Tab"
:position 0}]}
(revision/serialize-instance Dashboard (:id dashboard) dashboard))))))

(deftest ^:parallel diff-dashboards-str-test
Expand Down

0 comments on commit 76329cc

Please sign in to comment.