diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index db651b4e310dcc7f1f04377a87d5b7474347a203..20a23d4d346fb82cc4846e382203db11e37f2aae 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/resources/migrations/000_migrations.yaml
@@ -6380,9 +6380,8 @@ databaseChangeLog:
columnName: started_at
newDataType: ${timestamp_type}
-# Remove `Table.rows`, which hasn't been used for years now. Older versions of
-# Metabase used to store the row count in this column but we disabled it a long
-# time ago for performance reasons. Now it's time to remove it entirely.
+# Remove `Table.rows`, which hasn't been used for years now. Older versions of Metabase used to store the row count in
+# this column but we disabled it a long time ago for performance reasons. Now it's time to remove it entirely.
- changeSet:
id: 169
@@ -6392,3 +6391,45 @@ databaseChangeLog:
- dropColumn:
tableName: metabase_table
columnName: rows
+
+# In EE, NativeQuerySnippets have a permissions system based on "snippet folders" which are Collections under the
+# hood. However, these Collections live in a separate "namespace" -- a completely separate hierarchy of Collections.
+
+ - changeSet:
+ id: 171
+ author: camsaul
+ comment: Added 1.36.0
+ changes:
+ - addColumn:
+ tableName: native_query_snippet
+ columns:
+ - column:
+ name: collection_id
+ type: int
+ remarks: 'ID of the Snippet Folder (Collection) this Snippet is in, if any'
+ constraints:
+ nullable: true
+ references: collection(id)
+ foreignKeyName: fk_snippet_collection_id
+ deleteCascade: true
+ - createIndex:
+ tableName: native_query_snippet
+ indexName: idx_snippet_collection_id
+ columns:
+ - column:
+ name: collection_id
+
+ - changeSet:
+ id: 172
+ author: camsaul
+ comment: Added 1.36.0
+ changes:
+ - addColumn:
+ tableName: collection
+ columns:
+ - column:
+ name: namespace
+ type: varchar(254)
+ remarks: 'The namespace (hierachy) this Collection belongs to. NULL means the Collection is in the default namespace.'
+ constraints:
+ nullable: true
diff --git a/src/metabase/api/collection.clj b/src/metabase/api/collection.clj
index 672d053d997a8b2292f1434eb44cb4a577332de8..cda01049a3fdecdcb7575cdedfa227662e75008a 100644
--- a/src/metabase/api/collection.clj
+++ b/src/metabase/api/collection.clj
@@ -1,5 +1,9 @@
(ns metabase.api.collection
- "/api/collection endpoints."
+ "`/api/collection` endpoints. By default, these endpoints operate on Collections in the 'default' namespace, which is
+ the one that has things like Dashboards and Cards. Other namespaces of Collections exist as well, such as the
+ `:snippet` namespace, (called 'Snippet folders' in the UI). These namespaces are completely independent hierarchies.
+ To use these endpoints for other Collections namespaces, you can pass the `?namespace=` parameter (e.g.
+ `?namespace=snippet`)."
(:require [clojure.string :as str]
[compojure.core :refer [GET POST PUT]]
[metabase.api
@@ -10,8 +14,12 @@
[collection :as collection :refer [Collection]]
[dashboard :refer [Dashboard]]
[interface :as mi]
+ [native-query-snippet :refer [NativeQuerySnippet]]
[permissions :as perms]
[pulse :as pulse :refer [Pulse]]]
+ [metabase.models.collection
+ [graph :as collection.graph]
+ [root :as collection.root]]
[metabase.util :as u]
[metabase.util.schema :as su]
[schema.core :as s]
@@ -27,28 +35,35 @@
By default, this returns non-archived Collections, but instead you can show archived ones by passing
`?archived=true`."
- [archived]
- {archived (s/maybe su/BooleanString)}
+ [archived namespace]
+ {archived (s/maybe su/BooleanString)
+ namespace (s/maybe su/NonBlankString)}
(let [archived? (Boolean/parseBoolean archived)]
- (as-> (db/select Collection :archived archived?
- {:order-by [[:%lower.name :asc]]}) collections
+ (as-> (db/select Collection
+ :archived archived?
+ :namespace namespace
+ {:order-by [[:%lower.name :asc]]}) collections
(filter mi/can-read? collections)
;; include Root Collection at beginning or results if archived isn't `true`
(if archived?
collections
(cons (root-collection) collections))
(hydrate collections :can_write)
- ;; remove the :metabase.models.collection/is-root? tag since FE doesn't need it
+ ;; remove the :metabase.models.collection.root/is-root? tag since FE doesn't need it
(for [collection collections]
- (dissoc collection ::collection/is-root?)))))
+ (dissoc collection ::collection.root/is-root?)))))
;;; --------------------------------- Fetching a single Collection & its 'children' ----------------------------------
+(def ^:private valid-model-param-values
+ "Valid values for the `?model=` param accepted by endpoints in this namespace."
+ #{"card" "collection" "dashboard" "pulse" "snippet"})
+
(def ^:private CollectionChildrenOptions
{:archived? s/Bool
;; when specified, only return results of this type.
- :model (s/maybe (s/enum :card :dashboard :pulse :collection))})
+ :model (s/maybe (apply s/enum (map keyword valid-model-param-values)))})
(defmulti ^:private fetch-collection-children
"Functions for fetching the 'children' of a `collection`, for different types of objects. Possible options are listed
@@ -63,38 +78,59 @@
(defmethod fetch-collection-children :card
[_ collection {:keys [archived?]}]
(-> (db/select [Card :id :name :description :collection_position :display]
+ ;; use `:id` here so if it's the root collection we get `id = nil` (no ID)
:collection_id (:id collection)
- :archived archived?)
+ :archived (boolean archived?))
(hydrate :favorite)))
(defmethod fetch-collection-children :dashboard
[_ collection {:keys [archived?]}]
(db/select [Dashboard :id :name :description :collection_position]
:collection_id (:id collection)
- :archived archived?))
+ :archived (boolean archived?)))
(defmethod fetch-collection-children :pulse
[_ collection {:keys [archived?]}]
(db/select [Pulse :id :name :collection_position]
:collection_id (:id collection)
- :archived archived?
+ :archived (boolean archived?)
;; exclude Alerts
:alert_condition nil))
-(defmethod fetch-collection-children :collection
+(defmethod fetch-collection-children :snippet
[_ collection {:keys [archived?]}]
- (-> (for [child-collection (collection/effective-children collection [:= :archived archived?])]
+ (db/select [NativeQuerySnippet :id :name]
+ :collection_id (:id collection)
+ :archived (boolean archived?)))
+
+(defmethod fetch-collection-children :collection
+ [_ collection {:keys [archived? collection-namespace]}]
+ (-> (for [child-collection (collection/effective-children collection
+ [:= :archived archived?]
+ [:= :namespace (u/qualified-name collection-namespace)])]
(assoc child-collection :model "collection"))
(hydrate :can_write)))
+(defn- model-name->toucan-model [model-name]
+ (case (keyword model-name)
+ :collection Collection
+ :card Card
+ :dashboard Dashboard
+ :pulse Pulse
+ :snippet NativeQuerySnippet))
+
(s/defn ^:private collection-children
"Fetch a sequence of 'child' objects belonging to a Collection, filtered using `options`."
- [collection :- collection/CollectionWithLocationAndIDOrRoot
- {:keys [model collections-only?], :as options} :- CollectionChildrenOptions]
- (->> (for [model-kw [:collection :card :dashboard :pulse]
- ;; only fetch models that are specified by the `model` param; or everything if it's `nil`
- :when (or (not model) (= model model-kw))
- item (fetch-collection-children model-kw collection options)]
+ [{collection-namespace :namespace, :as collection} :- collection/CollectionWithLocationAndIDOrRoot
+ {:keys [model collections-only?], :as options} :- CollectionChildrenOptions]
+ (->> (for [model-kw [:collection :card :dashboard :pulse :snippet]
+ ;; only fetch models that are specified by the `model` param; or everything if it's `nil`
+ :when (or (not model) (= model model-kw))
+ :let [toucan-model (model-name->toucan-model model-kw)
+ allowed-namespaces (collection/allowed-namespaces toucan-model)]
+ :when (or (= model-kw :collection)
+ (contains? allowed-namespaces (keyword collection-namespace)))
+ item (fetch-collection-children model-kw collection (assoc options :collection-namespace collection-namespace))]
(assoc item :model model-kw))
;; sorting by name should be fine for now.
(sort-by (comp str/lower-case :name))))
@@ -106,12 +142,6 @@
(-> collection
(hydrate :parent_id :effective_location [:effective_ancestors :can_write] :can_write)))
-(s/defn ^:private collection-items
- "Return items in the Collection, restricted by `children-options`.
- Works for either a normal Collection or the Root Collection."
- [collection :- collection/CollectionWithLocationAndIDOrRoot, children-options :- CollectionChildrenOptions]
- (collection-children collection children-options))
-
(api/defendpoint GET "/:id"
"Fetch a specific Collection with standard details added"
[id]
@@ -123,9 +153,9 @@
* `model` - only include objects of a specific `model`. If unspecified, returns objects of all models
* `archived` - when `true`, return archived objects *instead* of unarchived ones. Defaults to `false`."
[id model archived]
- {model (s/maybe (s/enum "card" "dashboard" "pulse" "collection"))
+ {model (s/maybe (apply s/enum valid-model-param-values))
archived (s/maybe su/BooleanString)}
- (collection-items
+ (collection-children
(api/read-check Collection id)
{:model (keyword model)
:archived? (Boolean/parseBoolean archived)}))
@@ -139,7 +169,7 @@
(api/defendpoint GET "/root"
"Return the 'Root' Collection object with standard details added"
[]
- (dissoc (root-collection) ::collection/is-root?))
+ (dissoc (root-collection) ::collection.root/is-root?))
(api/defendpoint GET "/root/items"
"Fetch objects that the current user should see at their root level. As mentioned elsewhere, the 'Root' Collection
@@ -151,18 +181,22 @@
location of `/`.
This endpoint is intended to power a 'Root Folder View' for the Current User, so regardless you'll see all the
- top-level objects you're allowed to access."
- [model archived]
- {model (s/maybe (s/enum "card" "dashboard" "pulse" "collection"))
- archived (s/maybe su/BooleanString)}
+ top-level objects you're allowed to access.
+
+ By default, this will show the 'normal' Collections namespace; to view a different Collections namespace, such as
+ `snippets`, you can pass the `?namespace=` parameter."
+ [model archived namespace]
+ {model (s/maybe (apply s/enum valid-model-param-values))
+ archived (s/maybe su/BooleanString)
+ namespace (s/maybe su/NonBlankString)}
;; Return collection contents, including Collections that have an effective location of being in the Root
;; Collection for the Current User.
- (collection-items
- collection/root-collection
- {:model (if (mi/can-read? collection/root-collection)
- (keyword model)
- :collection)
- :archived? (Boolean/parseBoolean archived)}))
+ (collection-children
+ (assoc collection/root-collection :namespace namespace)
+ {:model (if (mi/can-read? collection/root-collection)
+ (keyword model)
+ :collection)
+ :archived? (Boolean/parseBoolean archived)}))
;;; ----------------------------------------- Creating/Editing a Collection ------------------------------------------
@@ -177,11 +211,12 @@
(api/defendpoint POST "/"
"Create a new Collection."
- [:as {{:keys [name color description parent_id]} :body}]
+ [:as {{:keys [name color description parent_id namespace]} :body}]
{name su/NonBlankString
color collection/hex-color-regex
description (s/maybe su/NonBlankString)
- parent_id (s/maybe su/IntGreaterThanZero)}
+ parent_id (s/maybe su/IntGreaterThanZero)
+ namespace (s/maybe su/NonBlankString)}
;; To create a new collection, you need write perms for the location you are going to be putting it in...
(write-check-collection-or-root-collection parent_id)
;; Now create the new Collection :)
@@ -189,7 +224,8 @@
(merge
{:name name
:color color
- :description description}
+ :description description
+ :namespace namespace}
(when parent_id
{:location (collection/children-location (db/select-one [Collection :location :id] :id parent_id))}))))
@@ -270,10 +306,10 @@
(api/defendpoint GET "/graph"
"Fetch a graph of all Collection Permissions."
- []
+ [namespace]
+ {namespace (s/maybe su/NonBlankString)}
(api/check-superuser)
- (collection/graph))
-
+ (collection.graph/graph namespace))
(defn- ->int [id] (Integer/parseInt (name id)))
@@ -296,11 +332,12 @@
(api/defendpoint PUT "/graph"
"Do a batch update of Collections Permissions by passing in a modified graph."
- [:as {body :body}]
- {body su/Map}
+ [namespace :as {body :body}]
+ {body su/Map
+ namespace (s/maybe su/NonBlankString)}
(api/check-superuser)
- (collection/update-graph! (dejsonify-graph body))
- (collection/graph))
+ (collection.graph/update-graph! namespace (dejsonify-graph body))
+ (collection.graph/graph namespace))
(api/define-routes)
diff --git a/src/metabase/api/permissions.clj b/src/metabase/api/permissions.clj
index 130fcd3aa28aef74cd95316de0091975444e6604..e152c7da1f7bd2f28ecced9e07e7b508369e2034 100644
--- a/src/metabase/api/permissions.clj
+++ b/src/metabase/api/permissions.clj
@@ -28,7 +28,6 @@
(api/check-superuser)
(perms/graph))
-
(api/defendpoint PUT "/graph"
"Do a batch update of Permissions by passing in a modified graph. This should return the same graph, in the same
format, that you got from `GET /api/permissions/graph`, with any changes made in the wherever necessary. This
diff --git a/src/metabase/api/search.clj b/src/metabase/api/search.clj
index d17d943cd7bc8617326d5e36578a833d996661ee..a11f6ab3cdead049c08e0b3a33e9eefda01608ff 100644
--- a/src/metabase/api/search.clj
+++ b/src/metabase/api/search.clj
@@ -260,7 +260,9 @@
collection-filter-clause (coll/visible-collection-ids->honeysql-filter-clause
collection-id-column
visible-collections)
- honeysql-query (h/merge-where honeysql-query collection-filter-clause)]
+ honeysql-query (-> honeysql-query
+ (h/merge-where collection-filter-clause)
+ (h/merge-where [:= :collection.namespace nil]))]
;; add a JOIN against Collection *unless* the source table is already Collection
(cond-> honeysql-query
(not= collection-id-column :collection.id)
diff --git a/src/metabase/models/card.clj b/src/metabase/models/card.clj
index 014b12eb09cc6f0830682b55e5da2d084e0ab6c6..5024026ec1421b1b31c2c67e330bb0ea784e440a 100644
--- a/src/metabase/models/card.clj
+++ b/src/metabase/models/card.clj
@@ -11,6 +11,7 @@
[normalize :as normalize]
[util :as mbql.u]]
[metabase.models
+ [collection :as collection]
[dependency :as dependency]
[field-values :as field-values]
[interface :as i]
@@ -117,7 +118,8 @@
(throw (Exception. (tru "You do not have permissions to run ad-hoc native queries against Database {0}."
(:database query))))))
;; make sure this Card doesn't have circular source query references
- (check-for-circular-source-query-references card)))
+ (check-for-circular-source-query-references card)
+ (collection/check-collection-namespace card)))
(defn- post-insert [card]
;; if this Card has any native template tag parameters we need to update FieldValues for any Fields that are
@@ -151,7 +153,8 @@
(field-values/update-field-values-for-on-demand-dbs! newly-added-param-field-ids)))))
;; make sure this Card doesn't have circular source query references if we're updating the query
(when (:dataset_query card)
- (check-for-circular-source-query-references card))))
+ (check-for-circular-source-query-references card))
+ (collection/check-collection-namespace card)))
;; Cards don't normally get deleted (they get archived instead) so this mostly affects tests
(defn- pre-delete [{:keys [id]}]
@@ -161,7 +164,6 @@
(db/delete! 'DashboardCard :card_id id)
(db/delete! 'CardFavorite :card_id id))
-
(u/strict-extend (class Card)
models/IModel
(merge models/IModelDefaults
diff --git a/src/metabase/models/collection.clj b/src/metabase/models/collection.clj
index d16d8c2590ec72326d77b2b29d5b87bf57c22f7d..37a4359225c38220e50adef70dcf854283e9ecb0 100644
--- a/src/metabase/models/collection.clj
+++ b/src/metabase/models/collection.clj
@@ -1,30 +1,31 @@
(ns metabase.models.collection
"Collections are used to organize Cards, Dashboards, and Pulses; as of v0.30, they are the primary way we determine
permissions for these objects.
-
- TODO - I think this namespace is too big now! Maybe move the graph stuff into somewhere like
- `metabase.models.collection.graph`"
+ `metabase.models.collection.graph`. `metabase.models.collection.graph`"
(:refer-clojure :exclude [ancestors descendants])
- (:require [clojure
- [data :as data]
- [string :as str]]
- [clojure.core.memoize :as memoize]
+ (:require [clojure.core.memoize :as memoize]
+ [clojure.string :as str]
[clojure.tools.logging :as log]
[honeysql.core :as hsql]
[metabase.api.common :as api :refer [*current-user-id* *current-user-permissions-set*]]
[metabase.models
- [collection-revision :as collection-revision :refer [CollectionRevision]]
[interface :as i]
[permissions :as perms :refer [Permissions]]]
+ [metabase.models.collection.root :as collection.root]
[metabase.util :as u]
[metabase.util
[i18n :as ui18n :refer [trs tru]]
[schema :as su]]
- [potemkin.types :as p.types]
+ [potemkin :as p]
[schema.core :as s]
[toucan
[db :as db]
- [models :as models]]))
+ [models :as models]])
+ (:import metabase.models.collection.root.RootCollection))
+
+(comment collection.root/keep-me)
+
+(p/import-vars [collection.root root-collection])
(def ^:private ^:const collection-slug-max-length
"Maximum number of characters allowed in a Collection `slug`."
@@ -45,7 +46,7 @@
(when (or (not (string? hex-color))
(not (re-matches hex-color-regex hex-color)))
(throw (ex-info (tru "Invalid color")
- {:status-code 400, :errors {:color (tru "must be a valid 6-character hex color code")}}))))
+ {:status-code 400, :errors {:color (tru "must be a valid 6-character hex color code")}}))))
(defn- slugify [collection-name]
;; double-check that someone isn't trying to use a blank string as the collection name
@@ -83,17 +84,17 @@
(Integer/parseInt id-str)))
(defn- valid-location-path? [s]
- (and (string? s)
- (seq s)
- (or (= s "/")
- (and (re-matches #"/(\d+/)*" s)
- (apply distinct? (unchecked-location-path->ids s))))))
+ (boolean
+ (and (string? s)
+ (re-matches #"^/(\d+/)*$" s)
+ (let [ids (unchecked-location-path->ids s)]
+ (or (empty? ids)
+ (apply distinct? ids))))))
(def LocationPath
"Schema for a directory-style 'path' to the location of a Collection."
(s/pred valid-location-path?))
-
(s/defn location-path :- LocationPath
"Build a 'location path' from a sequence of `collections-or-ids`.
@@ -142,59 +143,40 @@
;; if setting/updating the `location` of this Collection make sure it matches the schema for valid location paths
(when (contains? collection :location)
(when-not (valid-location-path? location)
- (throw
- (ex-info (tru "Invalid Collection location: path is invalid.")
- {:status-code 400
- :errors {:location (tru "Invalid Collection location: path is invalid.")}})))
+ (let [msg (tru "Invalid Collection location: path is invalid.")]
+ (throw (ex-info msg {:status-code 400, :errors {:location msg}}))))
;; if this is a Personal Collection it's only allowed to go in the Root Collection: you can't put it anywhere else!
(when (contains? collection :personal_owner_id)
(when-not (= location "/")
- (throw
- (ex-info (tru "You cannot move a Personal Collection.")
- {:status-code 400
- :errors {:location (tru "You cannot move a Personal Collection.")}}))))
+ (let [msg (tru "You cannot move a Personal Collection.")]
+ (throw (ex-info msg {:status-code 400, :errors {:location msg}})))))
;; Also make sure that all the IDs referenced in the Location path actually correspond to real Collections
(when-not (all-ids-in-location-path-are-valid? location)
- (throw
- (ex-info (tru "Invalid Collection location: some or all ancestors do not exist.")
- {:status-code 404
- :errors {:location (tru "Invalid Collection location: some or all ancestors do not exist.")}})))))
-
-
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | Root Collection Special Placeholder Object |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
-;; The Root Collection special placeholder object is used to represent the fact that we're working with the 'Root'
-;; Collection in many of the functions in this namespace. The Root Collection is not a true Collection, but instead
-;; represents things that have no collection_id, or are otherwise to be seen at the top-level by the current user.
-
-(p.types/defrecord+ ^:private RootCollection [])
-
-(u/strict-extend RootCollection
- i/IObjectPermissions
- (merge
- i/IObjectPermissionsDefaults
- {:perms-objects-set (fn [this read-or-write]
- #{((case read-or-write
- :read perms/collection-read-path
- :write perms/collection-readwrite-path) this)})
- :can-read? (partial i/current-user-has-full-permissions? :read)
- :can-write? (partial i/current-user-has-full-permissions? :write)}))
-
-(def ^RootCollection root-collection
- "Special placeholder object representing the Root Collection, which isn't really a real Collection."
- (map->RootCollection {::is-root? true}))
+ (let [msg (tru "Invalid Collection location: some or all ancestors do not exist.")]
+ (throw (ex-info msg {:status-code 404, :errors {:location msg}}))))))
+
+(defn- assert-valid-namespace
+ "Check that the namespace of this Collection is valid -- it must belong to the same namespace as its parent
+ Collection."
+ [{:keys [location], owner-id :personal_owner_id, collection-namespace :namespace, :as collection}]
+ {:pre [(contains? collection :namespace)]}
+ (when location
+ (when-let [parent-id (location-path->parent-id location)]
+ (let [parent-namespace (db/select-one-field :namespace Collection :id parent-id)]
+ (when-not (= (keyword collection-namespace) (keyword parent-namespace))
+ (let [msg (tru "Collection must be in the same namespace as its parent")]
+ (throw (ex-info msg {:status-code 400, :errors {:location msg}})))))))
+ ;; non-default namespace Collections cannot be personal Collections
+ (when (and owner-id collection-namespace)
+ (let [msg (tru "Personal Collections must be in the default namespace")]
+ (throw (ex-info msg {:status-code 400, :errors {:personal_owner_id msg}})))))
(defn root-collection-with-ui-details
"The special Root Collection placeholder object with some extra details to facilitate displaying it on the FE."
[]
(assoc root-collection
- :name (tru "Our analytics")
- :id "root"))
-
-(defn- is-root-collection? [x]
- (instance? RootCollection x))
+ :name (tru "Our analytics")
+ :id "root"))
(def ^:private CollectionWithLocationOrRoot
(s/cond-pre
@@ -309,7 +291,7 @@
`:effective_location`."
{:hydrate :effective_location}
([collection :- CollectionWithLocationOrRoot]
- (if (is-root-collection? collection)
+ (if (collection.root/is-root-collection? collection)
nil
(effective-location-path (:location collection)
(permissions-set->visible-collection-ids @*current-user-permissions-set*))))
@@ -352,7 +334,7 @@
can effectively treat A as the parent of C."
{:hydrate :effective_ancestors}
[collection :- CollectionWithLocationAndIDOrRoot]
- (if (is-root-collection? collection)
+ (if (collection.root/is-root-collection? collection)
[]
(filter i/can-read? (cons (root-collection-with-ui-details) (ancestors collection)))))
@@ -371,7 +353,7 @@
;; To get children of this collection:
(db/select Collection :location \"/10/20/30/\")"
[{:keys [location], :as collection} :- CollectionWithLocationAndIDOrRoot]
- (if (is-root-collection? collection)
+ (if (collection.root/is-root-collection? collection)
"/"
(str location (u/get-id collection) "/")))
@@ -487,7 +469,7 @@
* C, because by archiving its parent, you are archiving it as well"
[collection :- CollectionWithLocationAndIDOrRoot]
;; Make sure we're not trying to archive the Root Collection...
- (when (is-root-collection? collection)
+ (when (collection.root/is-root-collection? collection)
(throw (Exception. (tru "You cannot archive the Root Collection."))))
;; also make sure we're not trying to archive a PERSONAL Collection
(when (db/exists? Collection :id (u/get-id collection), :personal_owner_id [:not= nil])
@@ -512,7 +494,6 @@
===>
D D > B > C
-
To move or archive B, you would need write permissions for A, B, C, and D:
* A, because we're moving something out of it
@@ -521,7 +502,7 @@
* D, because it's the new parent Collection, and moving something into it requires write perms."
[collection :- CollectionWithLocationAndIDOrRoot, new-parent :- CollectionWithLocationAndIDOrRoot]
;; Make sure we're not trying to move the Root Collection...
- (when (is-root-collection? collection)
+ (when (collection.root/is-root-collection? collection)
(throw (Exception. (tru "You cannot move the Root Collection."))))
;; Needless to say, it makes no sense to move a Collection into itself or into one of its descendants. So let's make
;; sure we're not doing that...
@@ -563,7 +544,7 @@
(db/update-where! Collection {:id [:in affected-collection-ids]
:archived false}
:archived true)
- (doseq [model '[Card Dashboard Pulse]]
+ (doseq [model '[Card Dashboard NativeQuerySnippet Pulse]]
(db/update-where! model {:collection_id [:in affected-collection-ids]
:archived false}
:archived true)))))
@@ -577,7 +558,7 @@
(db/update-where! Collection {:id [:in affected-collection-ids]
:archived true}
:archived false)
- (doseq [model '[Card Dashboard Pulse]]
+ (doseq [model '[Card Dashboard NativeQuerySnippet Pulse]]
(db/update-where! model {:collection_id [:in affected-collection-ids]
:archived true}
:archived false)))))
@@ -613,6 +594,7 @@
(defn- pre-insert [{collection-name :name, color :color, :as collection}]
(assert-valid-location collection)
+ (assert-valid-namespace (merge {:namespace nil} collection))
(assert-valid-hex-color color)
(assoc collection :slug (slugify collection-name)))
@@ -641,8 +623,8 @@
(defn- copy-parent-permissions!
"When creating a new Collection, we shall copy the Permissions entries for its parent. That way, Groups who can see
- its parent can see it; and Groups who can 'curate' its parent can 'curate' it, as a default state. (Of course,
- admins can change these permissions after the fact.)
+ its parent can see it; and Groups who can 'curate' (write) its parent can 'curate' it, as a default state. (Of
+ course, admins can change these permissions after the fact.)
This does *not* apply to Collections that are created inside a Personal Collection or one of its descendants.
Descendants of Personal Collections, like Personal Collections themselves, cannot have permissions entries in the
@@ -777,11 +759,17 @@
(check-changes-allowed-for-personal-collection collection-before-updates collection-updates))
;; (2) make sure the location is valid if we're changing it
(assert-valid-location collection-updates)
- ;; (3) If we're moving a Collection from a location on a Personal Collection hierarchy to a location not on one,
+ ;; (3) make sure Collection namespace is valid
+ (when (contains? collection-updates :namespace)
+ (when (not= (:namespace collection-before-updates) (:namespace collection-updates))
+ (let [msg (tru "You cannot move a Collection to a different namespace once it has been created.")]
+ (throw (ex-info msg {:status-code 400, :errors {:namespace msg}})))))
+ (assert-valid-namespace (merge (select-keys collection-before-updates [:namespace]) collection-updates))
+ ;; (4) If we're moving a Collection from a location on a Personal Collection hierarchy to a location not on one,
;; or vice versa, we need to grant/revoke permissions as appropriate (see above for more details)
(when (api/column-will-change? :location collection-before-updates collection-updates)
(update-perms-when-moving-across-personal-boundry! collection-before-updates collection-updates))
- ;; (4) make sure hex color is valid
+ ;; (5) make sure hex color is valid
(when (api/column-will-change? :color collection-before-updates collection-updates)
(assert-valid-hex-color color))
;; OK, AT THIS POINT THE CHANGES ARE VALIDATED. NOW START ISSUING UPDATES
@@ -837,6 +825,7 @@
models/IModel
(merge models/IModelDefaults
{:hydration-keys (constantly [:collection])
+ :types (constantly {:namespace :keyword})
:pre-insert pre-insert
:post-insert post-insert
:pre-update pre-update
@@ -848,130 +837,6 @@
:perms-objects-set perms-objects-set}))
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | PERMISSIONS GRAPH |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
-;;; ---------------------------------------------------- Schemas -----------------------------------------------------
-
-(def ^:private CollectionPermissions
- (s/enum :write :read :none))
-
-(def ^:private GroupPermissionsGraph
- "collection-id -> status"
- {(s/optional-key :root) CollectionPermissions ; when doing a delta between old graph and new graph root won't always
- su/IntGreaterThanZero CollectionPermissions}) ; be present, which is why it's *optional*
-
-(def ^:private PermissionsGraph
- {:revision s/Int
- :groups {su/IntGreaterThanZero GroupPermissionsGraph}})
-
-
-;;; -------------------------------------------------- Fetch Graph ---------------------------------------------------
-
-(defn- group-id->permissions-set []
- (into {} (for [[group-id perms] (group-by :group_id (db/select 'Permissions))]
- {group-id (set (map :object perms))})))
-
-(s/defn ^:private perms-type-for-collection :- CollectionPermissions
- [permissions-set collection-or-id]
- (cond
- (perms/set-has-full-permissions? permissions-set (perms/collection-readwrite-path collection-or-id)) :write
- (perms/set-has-full-permissions? permissions-set (perms/collection-read-path collection-or-id)) :read
- :else :none))
-
-(s/defn ^:private group-permissions-graph :- GroupPermissionsGraph
- "Return the permissions graph for a single group having PERMISSIONS-SET."
- [permissions-set collection-ids]
- (into
- {:root (perms-type-for-collection permissions-set root-collection)}
- (for [collection-id collection-ids]
- {collection-id (perms-type-for-collection permissions-set collection-id)})))
-
-(s/defn ^:private non-personal-collection-ids :- #{su/IntGreaterThanZero}
- "Return a set of IDs of all Collections that are neither Personal Collections nor descendants of Personal
- Collections (i.e., things that you can set Permissions for, and that should go in the graph.)"
- []
- (->> (db/query
- (merge
- {:select [[:id :id]]
- :from [Collection]}
- (when-let [personal-collection-ids (seq (db/select-ids Collection :personal_owner_id [:not= nil]))]
- {:where (apply
- vector
- :and
- [:not-in :id personal-collection-ids]
- (for [id personal-collection-ids]
- [:not [:like :location (format "/%d/%%" id)]]))})))
- (map :id)
- set))
-
-(s/defn graph :- PermissionsGraph
- "Fetch a graph representing the current permissions status for every group and all permissioned collections. This
- works just like the function of the same name in `metabase.models.permissions`; see also the documentation for that
- function."
- []
- (let [group-id->perms (group-id->permissions-set)
- collection-ids (non-personal-collection-ids)]
- {:revision (collection-revision/latest-id)
- :groups (into {} (for [group-id (db/select-ids 'PermissionsGroup)]
- {group-id (group-permissions-graph (group-id->perms group-id) collection-ids)}))}))
-
-
-;;; -------------------------------------------------- Update Graph --------------------------------------------------
-
-(s/defn ^:private update-collection-permissions!
- [group-id :- su/IntGreaterThanZero
- collection-id :- (s/cond-pre (s/eq :root) su/IntGreaterThanZero)
- new-collection-perms :- CollectionPermissions]
- (let [collection-id (if (= collection-id :root)
- root-collection
- collection-id)]
- ;; remove whatever entry is already there (if any) and add a new entry if applicable
- (perms/revoke-collection-permissions! group-id collection-id)
- (case new-collection-perms
- :write (perms/grant-collection-readwrite-permissions! group-id collection-id)
- :read (perms/grant-collection-read-permissions! group-id collection-id)
- :none nil)))
-
-(s/defn ^:private update-group-permissions!
- [group-id :- su/IntGreaterThanZero, new-group-perms :- GroupPermissionsGraph]
- (doseq [[collection-id new-perms] new-group-perms]
- (update-collection-permissions! group-id collection-id new-perms)))
-
-(defn- save-perms-revision!
- "Save changes made to the collection permissions graph for logging/auditing purposes.
- This doesn't do anything if `*current-user-id*` is unset (e.g. for testing or REPL usage)."
- [current-revision old new]
- (when *current-user-id*
- ;; manually specify ID here so if one was somehow inserted in the meantime in the fraction of a second since we
- ;; called `check-revision-numbers` the PK constraint will fail and the transaction will abort
- (db/insert! CollectionRevision
- :id (inc current-revision)
- :before old
- :after new
- :user_id *current-user-id*)))
-
-(s/defn update-graph!
- "Update the collections permissions graph. This works just like the function of the same name in
- `metabase.models.permissions`, but for `Collections`; refer to that function's extensive documentation to get a
- sense for how this works."
- ([new-graph :- PermissionsGraph]
- (let [old-graph (graph)
- [old new] (data/diff (:groups old-graph) (:groups new-graph))]
- (perms/log-permissions-changes old new)
- (perms/check-revision-numbers old-graph new-graph)
- (when (seq new)
- (db/transaction
- (doseq [[group-id changes] new]
- (update-group-permissions! group-id changes))
- (save-perms-revision! (:revision old-graph) old new)))))
- ;; The following arity is provided soley for convenience for tests/REPL usage
- ([ks new-value]
- {:pre [(sequential? ks)]}
- (update-graph! (assoc-in (graph) (cons :groups ks) new-value))))
-
-
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Perms Checking Helper Fns |
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -1085,3 +950,33 @@
(for [user users]
(assoc user :personal_collection_id (or (user-id->collection-id (u/get-id user))
(user->personal-collection-id (u/get-id user))))))))
+
+(defmulti allowed-namespaces
+ "Set of Collection namespaces instances of this model are allowed to go in. By default, only the default
+ namespace (namespace = `nil`)."
+ {:arglists '([model])}
+ class)
+
+(defmethod allowed-namespaces :default
+ [_]
+ #{nil})
+
+(defn check-collection-namespace
+ "Check that object's `:collection_id` refers to a Collection in an allowed namespace (see
+ `allowed-namespaces`), or throw an Exception.
+
+ ;; Cards can only go in Collections in the default namespace (namespace = nil)
+ (check-collection-namespace card)"
+ [{collection-id :collection_id, :as object}]
+ (when collection-id
+ (let [collection-namespace (keyword (db/select-one-field :namespace 'Collection :id collection-id))
+ allowed-namespaces (allowed-namespaces object)]
+ (when-not (contains? allowed-namespaces collection-namespace)
+ (let [msg (tru "A {0} can only go in Collections in the {1} namespace."
+ (name object)
+ (str/join (format " %s " (tru "or")) (map #(pr-str (or % (tru "default")))
+ allowed-namespaces)))]
+ (throw (ex-info msg {:status-code 400
+ :errors {:collection_id msg}
+ :allowed-namespaces allowed-namespaces
+ :collection-namespace collection-namespace})))))))
diff --git a/src/metabase/models/collection/graph.clj b/src/metabase/models/collection/graph.clj
new file mode 100644
index 0000000000000000000000000000000000000000..f98d0b97a4d78b580100f75c98df6713f9d4bd17
--- /dev/null
+++ b/src/metabase/models/collection/graph.clj
@@ -0,0 +1,161 @@
+(ns metabase.models.collection.graph
+ (:require [clojure.data :as data]
+ [honeysql.helpers :as h]
+ [metabase.api.common :as api :refer [*current-user-id*]]
+ [metabase.models
+ [collection :as collection :refer [Collection]]
+ [collection-revision :as collection-revision :refer [CollectionRevision]]
+ [permissions :as perms :refer [Permissions]]
+ [permissions-group :refer [PermissionsGroup]]]
+ [metabase.util :as u]
+ [metabase.util.schema :as su]
+ [schema.core :as s]
+ [toucan.db :as db]))
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | PERMISSIONS GRAPH |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+;;; ---------------------------------------------------- Schemas -----------------------------------------------------
+
+(def ^:private CollectionPermissions
+ (s/enum :write :read :none))
+
+(def ^:private GroupPermissionsGraph
+ "collection-id -> status"
+ {(s/optional-key :root) CollectionPermissions ; when doing a delta between old graph and new graph root won't always
+ su/IntGreaterThanZero CollectionPermissions}) ; be present, which is why it's *optional*
+
+(def ^:private PermissionsGraph
+ {:revision s/Int
+ :groups {su/IntGreaterThanZero GroupPermissionsGraph}})
+
+
+;;; -------------------------------------------------- Fetch Graph ---------------------------------------------------
+
+(defn- group-id->permissions-set []
+ (into {} (for [[group-id perms] (group-by :group_id (db/select 'Permissions))]
+ {group-id (set (map :object perms))})))
+
+(s/defn ^:private perms-type-for-collection :- CollectionPermissions
+ [permissions-set collection-or-id]
+ (cond
+ (perms/set-has-full-permissions? permissions-set (perms/collection-readwrite-path collection-or-id)) :write
+ (perms/set-has-full-permissions? permissions-set (perms/collection-read-path collection-or-id)) :read
+ :else :none))
+
+(s/defn ^:private group-permissions-graph :- GroupPermissionsGraph
+ "Return the permissions graph for a single group having `permissions-set`."
+ [permissions-set collection-ids]
+ (into
+ {:root (perms-type-for-collection permissions-set collection/root-collection)}
+ (for [collection-id collection-ids]
+ {collection-id (perms-type-for-collection permissions-set collection-id)})))
+
+(s/defn ^:private non-personal-collection-ids :- #{su/IntGreaterThanZero}
+ "Return a set of IDs of all Collections that are neither Personal Collections nor descendants of Personal
+ Collections (i.e., things that you can set Permissions for, and that should go in the graph.)"
+ [collection-namespace :- (s/maybe su/KeywordOrString)]
+ (let [personal-collection-ids (db/select-ids Collection :personal_owner_id [:not= nil])
+ honeysql-form (cond-> {:select [[:id :id]]
+ :from [Collection]
+ :where (if (= collection-namespace ::all)
+ [:= 1 1]
+ [:= :namespace (u/qualified-name collection-namespace)])}
+ (seq personal-collection-ids)
+ (h/merge-where [:not-in :id (set personal-collection-ids)]))
+ honeysql-form (reduce
+ (fn [honeysql-form collection-id]
+ (h/merge-where honeysql-form [:not [:like :location (format "/%d/%%" collection-id)]]))
+ honeysql-form
+ personal-collection-ids)]
+ (set (map :id (db/query honeysql-form)))))
+
+(s/defn graph :- PermissionsGraph
+ "Fetch a graph representing the current permissions status for every group and all permissioned collections. This
+ works just like the function of the same name in `metabase.models.permissions`; see also the documentation for that
+ function.
+
+ The graph is restricted to a given namespace by the optional `collection-namespace` param; by default, `nil`, which
+ restricts it to the 'default' namespace containing normal Card/Dashboard/Pulse Collections. Pass `::all` to get a
+ combined graph of all namespaces, for the purposes of recording graph revisions.
+
+ Note: All Collections are returned at the same level of the 'graph', regardless of how the Collection hierarchy is
+ structured. Collections do not inherit permissions from ancestor Collections in the same way data permissions are
+ inherited (e.g. full `:read` perms for a Database implies `:read` perms for all its schemas); a 'child' object (e.g.
+ schema) *cannot* have more restrictive permissions than its parent (e.g. Database). Child Collections *can* have
+ more restrictive permissions than their parent."
+ ([]
+ (graph nil))
+
+ ([collection-namespace :- (s/maybe su/KeywordOrString)]
+ (let [group-id->perms (group-id->permissions-set)
+ collection-ids (non-personal-collection-ids collection-namespace)]
+ {:revision (collection-revision/latest-id)
+ :groups (into {} (for [group-id (db/select-ids PermissionsGroup)]
+ {group-id (group-permissions-graph (group-id->perms group-id) collection-ids)}))})))
+
+
+;;; -------------------------------------------------- Update Graph --------------------------------------------------
+
+(s/defn ^:private update-collection-permissions!
+ [group-id :- su/IntGreaterThanZero
+ collection-id :- (s/cond-pre (s/eq :root) su/IntGreaterThanZero)
+ new-collection-perms :- CollectionPermissions]
+ (let [collection-id (if (= collection-id :root)
+ collection/root-collection
+ collection-id)]
+ ;; remove whatever entry is already there (if any) and add a new entry if applicable
+ (perms/revoke-collection-permissions! group-id collection-id)
+ (case new-collection-perms
+ :write (perms/grant-collection-readwrite-permissions! group-id collection-id)
+ :read (perms/grant-collection-read-permissions! group-id collection-id)
+ :none nil)))
+
+(s/defn ^:private update-group-permissions!
+ [group-id :- su/IntGreaterThanZero, new-group-perms :- GroupPermissionsGraph]
+ (doseq [[collection-id new-perms] new-group-perms]
+ (update-collection-permissions! group-id collection-id new-perms)))
+
+(defn- save-perms-revision!
+ "Save changes made to the collection permissions graph for logging/auditing purposes. This doesn't do anything if
+ `*current-user-id*` is unset (e.g. for testing or REPL usage).
+
+ * `before-graph`-- the entire graph as it existed before the revision.
+ * `changes` -- set of changes applied in this revision."
+ [current-revision before-graph changes]
+ (when *current-user-id*
+ ;; manually specify ID here so if one was somehow inserted in the meantime in the fraction of a second since we
+ ;; called `check-revision-numbers` the PK constraint will fail and the transaction will abort
+ (db/insert! CollectionRevision
+ :id (inc current-revision)
+ :before before-graph
+ :after changes
+ :user_id *current-user-id*)))
+
+(s/defn update-graph!
+ "Update the Collections permissions graph for Collections of `collection-namespace` (default `nil`, the 'default'
+ namespace). This works just like the function of the same name in `metabase.models.permissions`, but for
+ Collections; refer to that function's extensive documentation to get a sense for how this works."
+ ([new-graph]
+ (update-graph! nil new-graph))
+
+ ([collection-namespace :- (s/maybe su/KeywordOrString), new-graph :- PermissionsGraph]
+ (let [old-graph (graph collection-namespace)
+ ;; fetch the *entire* graph before making any updates. We'll record this in the perms revision.
+ entire-graph (graph ::all)
+ old-perms (:groups old-graph)
+ new-perms (:groups new-graph)
+ ;; filter out any groups not in the old graph
+ new-perms (select-keys new-perms (keys old-perms))
+ ;; filter out any collections not in the old graph
+ new-perms (into {} (for [[group-id collection-id->perms] new-perms]
+ [group-id (select-keys collection-id->perms (keys (get old-perms group-id)))]))
+ [diff-old changes] (data/diff old-perms new-perms)]
+ (perms/log-permissions-changes diff-old changes)
+ (perms/check-revision-numbers old-graph new-graph)
+ (when (seq changes)
+ (db/transaction
+ (doseq [[group-id changes] changes]
+ (update-group-permissions! group-id changes))
+ (save-perms-revision! (:revision old-graph) entire-graph changes))))))
diff --git a/src/metabase/models/collection/root.clj b/src/metabase/models/collection/root.clj
new file mode 100644
index 0000000000000000000000000000000000000000..1c03d1f34b32dfbe2e77f82d7cac29d3c7c7061c
--- /dev/null
+++ b/src/metabase/models/collection/root.clj
@@ -0,0 +1,42 @@
+(ns metabase.models.collection.root
+ (:require [metabase.models
+ [interface :as i]
+ [permissions :as perms]]
+ [metabase.util :as u]
+ [potemkin.types :as p.types]
+ [toucan.models :as models]))
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | Root Collection Special Placeholder Object |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+;; The Root Collection special placeholder object is used to represent the fact that we're working with the 'Root'
+;; Collection in many of the functions in this namespace. The Root Collection is not a true Collection, but instead
+;; represents things that have no collection_id, or are otherwise to be seen at the top-level by the current user.
+
+(p.types/defrecord+ RootCollection [])
+
+(u/strict-extend RootCollection
+ models/IModel
+ (merge
+ models/IModelDefaults
+ {:types {:type :keyword}})
+
+ i/IObjectPermissions
+ (merge
+ i/IObjectPermissionsDefaults
+ {:perms-objects-set (fn [this read-or-write]
+ #{((case read-or-write
+ :read perms/collection-read-path
+ :write perms/collection-readwrite-path) this)})
+ :can-read? (partial i/current-user-has-full-permissions? :read)
+ :can-write? (partial i/current-user-has-full-permissions? :write)}))
+
+(def ^RootCollection root-collection
+ "Special placeholder object representing the Root Collection, which isn't really a real Collection."
+ (map->RootCollection {::is-root? true}))
+
+(defn is-root-collection?
+ "Is `x` the special placeholder object representing the Root Collection?"
+ [x]
+ (instance? RootCollection x))
diff --git a/src/metabase/models/collection_revision.clj b/src/metabase/models/collection_revision.clj
index c8ca745b3c4398226f8cd6479544691b2f4b757e..1c77f2607b69fc8f37a2c746098813b7d74df6d6 100644
--- a/src/metabase/models/collection_revision.clj
+++ b/src/metabase/models/collection_revision.clj
@@ -23,5 +23,5 @@
"Return the ID of the newest `CollectionRevision`, or zero if none have been made yet.
(This is used by the collection graph update logic that checks for changes since the original graph was fetched)."
[]
- (or (db/select-one-id CollectionRevision {:order-by [[:id :desc]]})
+ (or (:id (db/select-one [CollectionRevision [:%max.id :id]]))
0))
diff --git a/src/metabase/models/dashboard.clj b/src/metabase/models/dashboard.clj
index 1cf324587f970b0b1d0adab5b6ba1076fe7a483a..05e1da017018c55c9a883804004e86c6b83702c2 100644
--- a/src/metabase/models/dashboard.clj
+++ b/src/metabase/models/dashboard.clj
@@ -55,9 +55,14 @@
(db/delete! DashboardCard :dashboard_id (u/get-id dashboard)))
(defn- pre-insert [dashboard]
- (let [defaults {:parameters []}]
- (merge defaults dashboard)))
+ (let [defaults {:parameters []}
+ dashboard (merge defaults dashboard)]
+ (u/prog1 dashboard
+ (collection/check-collection-namespace (map->DashboardInstance dashboard)))))
+(defn- pre-update [dashboard]
+ (u/prog1 dashboard
+ (collection/check-collection-namespace dashboard)))
(u/strict-extend (class Dashboard)
models/IModel
@@ -66,6 +71,7 @@
:types (constantly {:parameters :json, :embedding_params :json})
:pre-delete pre-delete
:pre-insert pre-insert
+ :pre-update pre-update
:post-select public-settings/remove-public-uuid-if-public-sharing-is-disabled})
;; You can read/write a Dashboard if you can read/write its parent Collection
diff --git a/src/metabase/models/native_query_snippet.clj b/src/metabase/models/native_query_snippet.clj
index b86414274c2cf5e4365720d048344c4815cbd639..bc4f9a1006e275e26cd1a0e779db8b7693cb901f 100644
--- a/src/metabase/models/native_query_snippet.clj
+++ b/src/metabase/models/native_query_snippet.clj
@@ -1,5 +1,9 @@
(ns metabase.models.native-query-snippet
- (:require [metabase.models.interface :as i]
+ (:require [metabase.models
+ [collection :as collection]
+ [interface :as i]]
+ [metabase.models.native-query-snippet.permissions :as snippet.perms]
+ [metabase.plugins.classloader :as classloader]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
[schema.core :as s]
@@ -7,32 +11,44 @@
[db :as db]
[models :as models]]))
+;; Load the EE implementation of snippet permissions, if they exist (if we're running with EE code available).
+(u/ignore-exceptions
+ (classloader/require 'metabase-enterprise.enhancements.models.native-query-snippet.permissions))
+
;;; ----------------------------------------------- Entity & Lifecycle -----------------------------------------------
(models/defmodel NativeQuerySnippet :native_query_snippet)
+(defmethod collection/allowed-namespaces (class NativeQuerySnippet)
+ [_]
+ #{:snippets})
+
+(defn- pre-insert [snippet]
+ (u/prog1 snippet
+ (collection/check-collection-namespace snippet)))
+
(defn- pre-update [{:keys [creator_id id], :as updates}]
(u/prog1 updates
;; throw an Exception if someone tries to update creator_id
(when (contains? updates :creator_id)
(when (not= creator_id (db/select-one-field :creator_id NativeQuerySnippet :id id))
- (throw (UnsupportedOperationException. (tru "You cannot update the creator_id of a NativeQuerySnippet.")))))))
+ (throw (UnsupportedOperationException. (tru "You cannot update the creator_id of a NativeQuerySnippet.")))))
+ (collection/check-collection-namespace updates)))
(u/strict-extend (class NativeQuerySnippet)
models/IModel
(merge
models/IModelDefaults
{:properties (constantly {:timestamped? true})
+ :pre-insert pre-insert
:pre-update pre-update})
i/IObjectPermissions
(merge
i/IObjectPermissionsDefaults
- {;; In Metabase CE, anyone can read/edit/create NativeQuerySnippets. In EE permissions are dictated by a 'folder'
- ;; system similar to Collections (not yet implemented).
- :can-read? (constantly true)
- :can-write? (constantly true)
- :can-create? (constantly true)}))
+ {:can-read? snippet.perms/can-read?
+ :can-write? snippet.perms/can-write?
+ :can-create? snippet.perms/can-create?}))
;;; ---------------------------------------------------- Schemas -----------------------------------------------------
diff --git a/src/metabase/models/native_query_snippet/permissions.clj b/src/metabase/models/native_query_snippet/permissions.clj
new file mode 100644
index 0000000000000000000000000000000000000000..8d29ecb562b78c227b9a5df8b4ef24e0fe3af7d2
--- /dev/null
+++ b/src/metabase/models/native_query_snippet/permissions.clj
@@ -0,0 +1,64 @@
+(ns metabase.models.native-query-snippet.permissions
+ "NativeQuerySnippets have different permissions implementations. In Metabase CE, anyone can read/edit/create all
+ NativeQuerySnippets. EE has a more advanced implementation.
+
+ The code in this namespace provides sort of a strategy pattern interface to the underlying permissions operations.
+ The default implementation is defined below and can be swapped out at runtime with the more advanced EE
+ implementation."
+ (:require [potemkin.types :as p.types]
+ [pretty.core :refer [PrettyPrintable]]))
+
+(p.types/defprotocol+ PermissionsImpl
+ "Protocol for implementing the permissions logic for NativeQuerySnippets."
+ (can-read?* [this snippet] [this model id]
+ "Can the current User read this `snippet`?")
+ (can-write?* [this snippet] [this model id]
+ "Can the current User edit this `snippet`?")
+ (can-create?* [this snippet] [this model id]
+ "Can the current User save a new `snippet` with these values?"))
+
+(defonce ^:private impl (atom nil))
+
+(defn set-impl!
+ "Change the implementation used for NativeQuerySnippet permissions. `new-impl` must satisfy the `PermissionsImpl`
+ protocol defined above."
+ [new-impl]
+ (reset! impl new-impl))
+
+(def default-impl
+ "Default 'simple' permissions implementation for NativeQuerySnippets for Metabase CE."
+ (reify
+ PrettyPrintable
+ (pretty [_]
+ `default-impl)
+ PermissionsImpl
+ (can-read?* [_ _] true)
+ (can-read?* [_ _ _] true)
+ (can-write?* [_ _] true)
+ (can-write?* [_ _ _] true)
+ (can-create?* [_ _] true)
+ (can-create?* [_ _ _] true)))
+
+(when-not @impl
+ (set-impl! default-impl))
+
+(defn can-read?
+ "Can the current User read this `snippet`?"
+ ([snippet]
+ (can-read?* @impl snippet))
+ ([model id]
+ (can-read?* @impl model id)))
+
+(defn can-write?
+ "Can the current User edit this `snippet`?"
+ ([snippet]
+ (can-write?* @impl snippet))
+ ([model id]
+ (can-write?* @impl model id)))
+
+(defn can-create?
+ "Can the current User save a new `snippet` with these values?"
+ ([snippet]
+ (can-create?* @impl snippet))
+ ([model id]
+ (can-create?* @impl model id)))
diff --git a/src/metabase/models/permissions.clj b/src/metabase/models/permissions.clj
index 75b0b98c74498430129aec681f339650db4bfd32..862e1832ffde139da37f31d9cf6c81d92819fb87 100644
--- a/src/metabase/models/permissions.clj
+++ b/src/metabase/models/permissions.clj
@@ -148,7 +148,7 @@
"Return the permissions path for *readwrite* access for a `collection-or-id`."
[collection-or-id :- MapOrID]
(str "/collection/"
- (if (get collection-or-id :metabase.models.collection/is-root?)
+ (if (get collection-or-id :metabase.models.collection.root/is-root?)
"root"
(u/get-id collection-or-id))
"/"))
@@ -245,7 +245,7 @@
;; now pass that function our collection_id if we have one, or if not, pass it an object representing the Root
;; Collection
#{(path-fn (or (:collection_id this)
- {:metabase.models.collection/is-root? true}))}))
+ {:metabase.models.collection.root/is-root? true}))}))
(def IObjectPermissionsForParentCollection
"Implementation of `IObjectPermissions` for objects that have a `collection_id`, and thus, a parent Collection.
@@ -500,7 +500,7 @@
be given some sort of access."
[collection-or-id :- MapOrID]
;; don't apply this check to the Root Collection, because it's never personal
- (when-not (:metabase.models.collection/is-root? collection-or-id)
+ (when-not (:metabase.models.collection.root/is-root? collection-or-id)
;; ok, once we've confirmed this isn't the Root Collection, see if it's in the DB with a personal_owner_id
(let [collection (if (map? collection-or-id)
collection-or-id
diff --git a/src/metabase/models/pulse.clj b/src/metabase/models/pulse.clj
index 7ac77342cb4ab9036fbf3f98185966e5cde0a5e6..a4452c9535db0fb2e3b352ef6be7aab5163c2300 100644
--- a/src/metabase/models/pulse.clj
+++ b/src/metabase/models/pulse.clj
@@ -22,6 +22,7 @@
[util :as u]]
[metabase.models
[card :refer [Card]]
+ [collection :as collection]
[interface :as i]
[permissions :as perms]
[pulse-card :refer [PulseCard]]
@@ -44,6 +45,14 @@
(doseq [model [PulseCard PulseChannel]]
(db/delete! model :pulse_id (u/get-id notification))))
+(defn- pre-insert [notification]
+ (u/prog1 notification
+ (collection/check-collection-namespace notification)))
+
+(defn- pre-update [updates]
+ (u/prog1 updates
+ (collection/check-collection-namespace updates)))
+
(defn- alert->card
"Return the Card associated with an Alert, fetching it if needed, for permissions-checking purposes."
[alert]
@@ -73,7 +82,9 @@
models/IModelDefaults
{:hydration-keys (constantly [:pulse])
:properties (constantly {:timestamped? true})
- :pre-delete pre-delete})
+ :pre-delete pre-delete
+ :pre-insert pre-insert
+ :pre-update pre-update})
i/IObjectPermissions
(merge
i/IObjectPermissionsDefaults
@@ -413,7 +424,7 @@
;; TODO - why do we make sure to strictly validate everything when we create a PULSE but not when we create an ALERT?
(defn update-alert!
- "Updates the given `ALERT` and returns it"
+ "Updates the given `alert` and returns it"
[alert]
(update-notification! (alert->notification alert))
;; fetch the fully updated pulse and return it (and fire off an event)
@@ -439,5 +450,4 @@
[:= :pcr.user_id user-id]]} "r"]]}]})]
(when (zero? result)
(log/warnf "Failed to remove user-id '%s' from alert-id '%s'" user-id alert-id))
-
result))
diff --git a/test/expectations.clj b/test/expectations.clj
index 3e7f112aa92e3ca890d4bd194e2c6368a3fc4684..67fbe27f95a12f5a68811ef1853ffc02874155f3 100644
--- a/test/expectations.clj
+++ b/test/expectations.clj
@@ -137,8 +137,8 @@
;; expecting it.
(when-not (env/env :drivers)
(t/testing "Don't write any new tests using expect!"
- (t/is (<= total-expect-forms 1716))
- (t/is (<= total-namespaces-using-expect 108))))))
+ (t/is (<= total-expect-forms 1555))
+ (t/is (<= total-namespaces-using-expect 106))))))
(defmacro ^:deprecated expect
"Simple macro that simulates converts an Expectations-style `expect` form into a `clojure.test` `deftest` form."
diff --git a/test/metabase/api/collection_test.clj b/test/metabase/api/collection_test.clj
index ea29433f2a4a3d38bc0984dcaac1a225437947d0..b511d2c61cab9bdb49d432916c0ce03c90e454e1 100644
--- a/test/metabase/api/collection_test.clj
+++ b/test/metabase/api/collection_test.clj
@@ -3,28 +3,21 @@
(:require [clojure
[string :as str]
[test :refer :all]]
- [expectations :refer [expect]]
[metabase
- [email-test :as et]
+ [models :refer [Card Collection Dashboard NativeQuerySnippet PermissionsGroup PermissionsGroupMembership Pulse PulseCard PulseChannel PulseChannelRecipient]]
+ [test :as mt]
[util :as u]]
[metabase.models
- [card :refer [Card]]
- [collection :as collection :refer [Collection]]
+ [collection :as collection]
[collection-test :as collection-test]
- [dashboard :refer [Dashboard]]
[permissions :as perms]
- [permissions-group :as group :refer [PermissionsGroup]]
- [permissions-group-membership :refer [PermissionsGroupMembership]]
- [pulse :refer [Pulse]]
- [pulse-card :refer [PulseCard]]
- [pulse-channel :refer [PulseChannel]]
- [pulse-channel-recipient :refer [PulseChannelRecipient]]]
- [metabase.test
- [fixtures :as fixtures]
- [util :as tu]]
- [metabase.test.data.users :refer [user->client user->id]]
- [toucan.db :as db]
- [toucan.util.test :as tt]))
+ [permissions-group :as group]]
+ [metabase.models.collection
+ [graph :as graph]
+ [graph-test :as graph.test]]
+ [metabase.test.fixtures :as fixtures]
+ [schema.core :as s]
+ [toucan.db :as db]))
(use-fixtures :once (fixtures/initialize :test-users-personal-collections))
@@ -32,108 +25,130 @@
;;; | GET /collection |
;;; +----------------------------------------------------------------------------------------------------------------+
-;; check that we can get a basic list of collections
-;; (for the purposes of test purposes remove the personal collections)
-(tt/expect-with-temp [Collection [collection]]
- [{:parent_id nil, :effective_location nil, :effective_ancestors (), :can_write true, :name "Our analytics", :id "root"}
- (assoc (into {} collection) :can_write true)]
- (for [collection ((user->client :crowberto) :get 200 "collection")
- :when (not (:personal_owner_id collection))]
- collection))
-
-;; We should only see our own Personal Collections!
-(expect
- ["Our analytics"
- "Lucky Pigeon's Personal Collection"]
- (map :name ((user->client :lucky) :get 200 "collection")))
-
-;; ...unless we are *admins*
-(expect
- ["Our analytics"
- "Crowberto Corv's Personal Collection"
- "Lucky Pigeon's Personal Collection"
- "Rasta Toucan's Personal Collection"
- "Trash Bird's Personal Collection"]
- (map :name ((user->client :crowberto) :get 200 "collection")))
-
-;; check that we don't see collections if we don't have permissions for them
-(expect
- ["Our analytics"
- "Collection 1"
- "Rasta Toucan's Personal Collection"]
- (tu/with-non-admin-groups-no-root-collection-perms
- (tt/with-temp* [Collection [collection-1 {:name "Collection 1"}]
- Collection [collection-2 {:name "Collection 2"}]]
- (perms/grant-collection-read-permissions! (group/all-users) collection-1)
- (map :name ((user->client :rasta) :get 200 "collection")))))
-
-;; check that we don't see collections if they're archived
-(expect
- ["Our analytics"
- "Rasta Toucan's Personal Collection"
- "Regular Collection"]
- (tt/with-temp* [Collection [collection-1 {:name "Archived Collection", :archived true}]
- Collection [collection-2 {:name "Regular Collection"}]]
- (perms/grant-collection-read-permissions! (group/all-users) collection-1)
- (perms/grant-collection-read-permissions! (group/all-users) collection-2)
- (map :name ((user->client :rasta) :get 200 "collection"))))
-
-;; Check that if we pass `?archived=true` we instead see archived cards
-(expect
- ["Archived Collection"]
- (tt/with-temp* [Collection [collection-1 {:name "Archived Collection", :archived true}]
- Collection [collection-2 {:name "Regular Collection"}]]
- (perms/grant-collection-read-permissions! (group/all-users) collection-1)
- (perms/grant-collection-read-permissions! (group/all-users) collection-2)
- (map :name ((user->client :rasta) :get 200 "collection" :archived :true))))
+(deftest list-collections-test
+ (testing "GET /api/collection"
+ (testing "check that we can get a basic list of collections"
+ ;; (for the purposes of test purposes remove the personal collections)
+ (mt/with-temp Collection [collection]
+ (is (= [{:parent_id nil
+ :effective_location nil
+ :effective_ancestors []
+ :can_write true
+ :name "Our analytics"
+ :id "root"}
+ (assoc (into {} collection) :can_write true)]
+ (for [collection ((mt/user->client :crowberto) :get 200 "collection")
+ :when (not (:personal_owner_id collection))]
+ collection)))))
+
+ (testing "We should only see our own Personal Collections!"
+ (is (= ["Lucky Pigeon's Personal Collection"]
+ (->> ((mt/user->client :lucky) :get 200 "collection")
+ (filter :personal_owner_id)
+ (map :name))))
+
+ (testing "...unless we are *admins*"
+ (is (= ["Crowberto Corv's Personal Collection"
+ "Lucky Pigeon's Personal Collection"
+ "Rasta Toucan's Personal Collection"
+ "Trash Bird's Personal Collection"]
+ (->> ((mt/user->client :crowberto) :get 200 "collection")
+ (filter #((set (map mt/user->id [:crowberto :lucky :rasta :trashbird])) (:personal_owner_id %)))
+ (map :name)
+ sort)))))
+
+ (testing "check that we don't see collections if we don't have permissions for them"
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp* [Collection [collection-1 {:name "Collection 1"}]
+ Collection [collection-2 {:name "Collection 2"}]]
+ (perms/grant-collection-read-permissions! (group/all-users) collection-1)
+ (is (= ["Our analytics"
+ "Collection 1"
+ "Rasta Toucan's Personal Collection"]
+ (map :name ((mt/user->client :rasta) :get 200 "collection")))))))
+
+ (testing "check that we don't see collections if they're archived"
+ (mt/with-temp* [Collection [collection-1 {:name "Archived Collection", :archived true}]
+ Collection [collection-2 {:name "Regular Collection"}]]
+ (is (= ["Our analytics"
+ "Rasta Toucan's Personal Collection"
+ "Regular Collection"]
+ (map :name ((mt/user->client :rasta) :get 200 "collection"))))))
+
+ (testing "Check that if we pass `?archived=true` we instead see archived Collections"
+ (mt/with-temp* [Collection [collection-1 {:name "Archived Collection", :archived true}]
+ Collection [collection-2 {:name "Regular Collection"}]]
+ (is (= ["Archived Collection"]
+ (map :name ((mt/user->client :rasta) :get 200 "collection" :archived :true))))))
+
+ (testing "?namespace= parameter"
+ (mt/with-temp* [Collection [{normal-id :id} {:name "Normal Collection"}]
+ Collection [{coins-id :id} {:name "Coin Collection", :namespace "currency"}]]
+ (letfn [(collection-names [collections]
+ (->> collections
+ (filter #(#{normal-id coins-id} (:id %)))
+ (map :name)))]
+ (testing "shouldn't show Collections of a different `:namespace` by default"
+ (is (= ["Normal Collection"]
+ (collection-names ((mt/user->client :rasta) :get 200 "collection")))))
+
+ (testing "By passing `:namespace` we should be able to see Collections of that `:namespace`"
+ (testing "?namespace=currency"
+ (is (= ["Coin Collection"]
+ (collection-names ((mt/user->client :rasta) :get 200 "collection?namespace=currency")))))
+
+ (testing "?namespace=stamps"
+ (is (= []
+ (collection-names ((mt/user->client :rasta) :get 200 "collection?namespace=stamps")))))))))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | GET /collection/:id |
;;; +----------------------------------------------------------------------------------------------------------------+
-;; check that we can see collection details (GET /api/collection/:id)
-(expect
- "Coin Collection"
- (tt/with-temp Collection [collection {:name "Coin Collection"}]
- (perms/grant-collection-read-permissions! (group/all-users) collection)
- (:name ((user->client :rasta) :get 200 (str "collection/" (u/get-id collection))))))
+(deftest fetch-collection-test
+ (testing "GET /api/collection/:id"
+ (testing "check that we can see collection details"
+ (mt/with-temp Collection [collection {:name "Coin Collection"}]
+ (perms/grant-collection-read-permissions! (group/all-users) collection)
+ (is (= "Coin Collection"
+ (:name ((mt/user->client :rasta) :get 200 (str "collection/" (u/get-id collection))))))))
-;; check that collections detail properly checks permissions
-(expect
- "You don't have permissions to do that."
- (tu/with-non-admin-groups-no-root-collection-perms
- (tt/with-temp Collection [collection]
- ((user->client :rasta) :get 403 (str "collection/" (u/get-id collection))))))
+ (testing "check that collections detail properly checks permissions"
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp Collection [collection]
+ (is (= "You don't have permissions to do that."
+ ((mt/user->client :rasta) :get 403 (str "collection/" (u/get-id collection))))))))))
-;;; ----------------------------------------- Cards, Dashboards, and Pulses ------------------------------------------
-
-;; check that cards are returned with the collection/items endpoint
-(tt/expect-with-temp [Collection [collection]
- Card [card {:collection_id (u/get-id collection)}]]
- (tu/obj->json->obj
- [{:id (u/get-id card)
- :name (:name card)
- :collection_position nil
- :display "table"
- :description nil
- :favorite false
- :model "card"}])
- (tu/obj->json->obj
- ((user->client :crowberto) :get 200 (str "collection/" (u/get-id collection) "/items"))))
+;;; ------------------------------------------------ Collection Items ------------------------------------------------
(defn- do-with-some-children-of-collection [collection-or-id-or-nil f]
- (tu/with-non-admin-groups-no-root-collection-perms
+ (mt/with-non-admin-groups-no-root-collection-perms
(let [collection-id-or-nil (when collection-or-id-or-nil
(u/get-id collection-or-id-or-nil))]
- (tt/with-temp* [Card [_ {:name "Birthday Card", :collection_id collection-id-or-nil}]
- Dashboard [_ {:name "Dine & Dashboard", :collection_id collection-id-or-nil}]
- Pulse [_ {:name "Electro-Magnetic Pulse", :collection_id collection-id-or-nil}]]
- (f)))))
+ (mt/with-temp* [Card [{card-id :id} {:name "Birthday Card", :collection_id collection-id-or-nil}]
+ Dashboard [{dashboard-id :id} {:name "Dine & Dashboard", :collection_id collection-id-or-nil}]
+ Pulse [{pulse-id :id} {:name "Electro-Magnetic Pulse", :collection_id collection-id-or-nil}]]
+ (f {:card-id card-id, :dashboard-id dashboard-id, :pulse-id pulse-id})))))
(defmacro ^:private with-some-children-of-collection {:style/indent 1} [collection-or-id-or-nil & body]
- `(do-with-some-children-of-collection ~collection-or-id-or-nil (fn [] ~@body)))
+ `(do-with-some-children-of-collection
+ ~collection-or-id-or-nil
+ (fn [~'&ids]
+ ~@body)))
+
+(defn- remove-non-test-items
+ "Remove Cards, Dashboards, and Pulses that aren't the 'Birthday Card'/'Dine & Dashboard'/'Electro-Magnetic Pulse'
+ created by `with-some-children-of-collection`."
+ [items {:keys [card-id dashboard-id pulse-id]}]
+ (filter (fn [{:keys [id model]}]
+ (case model
+ "card" (= id card-id)
+ "dashboard" (= id dashboard-id)
+ "pulse" (= id pulse-id)
+ true))
+ items))
(defn- default-item [item-map]
(merge {:id true, :collection_position nil} item-map))
@@ -146,95 +161,131 @@
:name collection-name}
extra-keypairs))
-;; check that you get to see the children as appropriate
-(expect
- (map default-item [{:name "Birthday Card", :description nil, :favorite false, :model "card", :display "table"}
- {:name "Dine & Dashboard", :description nil, :model "dashboard"}
- {:name "Electro-Magnetic Pulse", :model "pulse"}])
- (tt/with-temp Collection [collection {:name "Debt Collection"}]
- (perms/grant-collection-read-permissions! (group/all-users) collection)
- (with-some-children-of-collection collection
- (tu/boolean-ids-and-timestamps
- ((user->client :rasta) :get 200 (str "collection/" (u/get-id collection) "/items"))))))
-
-;; ...and that you can also filter so that you only see the children you want to see
-(expect
- [(default-item {:name "Dine & Dashboard", :description nil, :model "dashboard"})]
- (tt/with-temp Collection [collection {:name "Art Collection"}]
- (perms/grant-collection-read-permissions! (group/all-users) collection)
- (with-some-children-of-collection collection
- (tu/boolean-ids-and-timestamps
- ((user->client :rasta) :get 200 (str "collection/" (u/get-id collection) "/items?model=dashboard"))))))
-
-;; Let's make sure the `archived` option works.
-(expect
- [(default-item {:name "Dine & Dashboard", :description nil, :model "dashboard"})]
- (tt/with-temp Collection [collection {:name "Art Collection"}]
- (perms/grant-collection-read-permissions! (group/all-users) collection)
- (with-some-children-of-collection collection
- (db/update-where! Dashboard {:collection_id (u/get-id collection)} :archived true)
- (tu/boolean-ids-and-timestamps
- ((user->client :rasta) :get 200 (str "collection/" (u/get-id collection) "/items?archived=true"))))))
+(deftest collection-items-test
+ (testing "GET /api/collection/:id/items"
+ (testing "check that cards are returned with the collection/items endpoint"
+ (mt/with-temp* [Collection [collection]
+ Card [card {:collection_id (u/get-id collection)}]]
+ (is (= (mt/obj->json->obj
+ [{:id (u/get-id card)
+ :name (:name card)
+ :collection_position nil
+ :display "table"
+ :description nil
+ :favorite false
+ :model "card"}])
+ (mt/obj->json->obj
+ ((mt/user->client :crowberto) :get 200 (str "collection/" (u/get-id collection) "/items")))))))
+
+ (testing "check that you get to see the children as appropriate"
+ (mt/with-temp Collection [collection {:name "Debt Collection"}]
+ (perms/grant-collection-read-permissions! (group/all-users) collection)
+ (with-some-children-of-collection collection
+ (is (= (map default-item [{:name "Birthday Card", :description nil, :favorite false, :model "card", :display "table"}
+ {:name "Dine & Dashboard", :description nil, :model "dashboard"}
+ {:name "Electro-Magnetic Pulse", :model "pulse"}])
+ (mt/boolean-ids-and-timestamps
+ ((mt/user->client :rasta) :get 200 (str "collection/" (u/get-id collection) "/items")))))))
+
+ (testing "...and that you can also filter so that you only see the children you want to see"
+ (mt/with-temp Collection [collection {:name "Art Collection"}]
+ (perms/grant-collection-read-permissions! (group/all-users) collection)
+ (with-some-children-of-collection collection
+ (is (= [(default-item {:name "Dine & Dashboard", :description nil, :model "dashboard"})]
+ (mt/boolean-ids-and-timestamps
+ ((mt/user->client :rasta) :get 200 (str "collection/" (u/get-id collection) "/items?model=dashboard")))))))))
+
+ (testing "Let's make sure the `archived` option works."
+ (mt/with-temp Collection [collection {:name "Art Collection"}]
+ (perms/grant-collection-read-permissions! (group/all-users) collection)
+ (with-some-children-of-collection collection
+ (db/update-where! Dashboard {:collection_id (u/get-id collection)} :archived true)
+ (is (= [(default-item {:name "Dine & Dashboard", :description nil, :model "dashboard"})]
+ (mt/boolean-ids-and-timestamps
+ ((mt/user->client :rasta) :get 200 (str "collection/" (u/get-id collection) "/items?archived=true"))))))))))
+
+(deftest snippet-collection-items-test
+ (testing "GET /api/collection/:id/items"
+ (testing "Native query snippets should come back when fetching the items in a Collection in the `:snippets` namespace"
+ (mt/with-temp* [Collection [{collection-id :id} {:namespace "snippets", :name "My Snippet Collection"}]
+ NativeQuerySnippet [{snippet-id :id} {:collection_id collection-id, :name "My Snippet"}]
+ NativeQuerySnippet [{archived-id :id} {:collection_id collection-id, :name "Archived Snippet", :archived true}]]
+ (is (= [{:id snippet-id
+ :name "My Snippet"
+ :model "snippet"}]
+ ((mt/user->client :rasta) :get 200 (format "collection/%d/items" collection-id))))
+
+ (testing "\nShould be able to fetch archived Snippets"
+ (is (= [{:id archived-id
+ :name "Archived Snippet"
+ :model "snippet"}]
+ ((mt/user->client :rasta) :get 200 (format "collection/%d/items?archived=true" collection-id)))))
+
+ (testing "\nShould be able to pass ?model=snippet, even though it makes no difference in this case"
+ (is (= [{:id snippet-id
+ :name "My Snippet"
+ :model "snippet"}]
+ ((mt/user->client :rasta) :get 200 (format "collection/%d/items?model=snippet" collection-id)))))))))
+
;;; --------------------------------- Fetching Personal Collections (Ours & Others') ---------------------------------
(defn- lucky-personal-collection []
- {:description nil
- :archived false
- :slug "lucky_pigeon_s_personal_collection"
- :color "#31698A"
- :can_write true
- :name "Lucky Pigeon's Personal Collection"
- :personal_owner_id (user->id :lucky)
- :effective_ancestors [{:metabase.models.collection/is-root? true, :name "Our analytics", :id "root", :can_write true}]
- :effective_location "/"
- :parent_id nil
- :id (u/get-id (collection/user->personal-collection (user->id :lucky)))
- :location "/"})
+ (merge
+ (mt/object-defaults Collection)
+ {:slug "lucky_pigeon_s_personal_collection"
+ :color "#31698A"
+ :can_write true
+ :name "Lucky Pigeon's Personal Collection"
+ :personal_owner_id (mt/user->id :lucky)
+ :effective_ancestors [{:metabase.models.collection.root/is-root? true, :name "Our analytics", :id "root", :can_write true}]
+ :effective_location "/"
+ :parent_id nil
+ :id (u/get-id (collection/user->personal-collection (mt/user->id :lucky)))
+ :location "/"}))
(defn- lucky-personal-collection-id
[]
- (u/get-id (collection/user->personal-collection (user->id :lucky))))
+ (u/get-id (collection/user->personal-collection (mt/user->id :lucky))))
(defn- api-get-lucky-personal-collection [user-kw & {:keys [expected-status-code], :or {expected-status-code 200}}]
- ((user->client user-kw) :get expected-status-code (str "collection/" (lucky-personal-collection-id))))
+ ((mt/user->client user-kw) :get expected-status-code (str "collection/" (lucky-personal-collection-id))))
(defn- api-get-lucky-personal-collection-items [user-kw & {:keys [expected-status-code], :or {expected-status-code 200}}]
- ((user->client user-kw) :get expected-status-code (str "collection/" (lucky-personal-collection-id) "/items")))
+ ((mt/user->client user-kw) :get expected-status-code (str "collection/" (lucky-personal-collection-id) "/items")))
-;; Can we use this endpoint to fetch our own Personal Collection?
-(expect
- (lucky-personal-collection)
- (api-get-lucky-personal-collection :lucky))
+(deftest fetch-personal-collection-test
+ (testing "GET /api/collection/:id"
+ (testing "Can we use this endpoint to fetch our own Personal Collection?"
+ (is (= (lucky-personal-collection)
+ (api-get-lucky-personal-collection :lucky))))
-;; Can and admin use this endpoint to fetch someone else's Personal Collection?
-(expect
- (lucky-personal-collection)
- (api-get-lucky-personal-collection :crowberto))
+ (testing "Can and admin use this endpoint to fetch someone else's Personal Collection?"
+ (is (= (lucky-personal-collection)
+ (api-get-lucky-personal-collection :crowberto))))
-;; Other, non-admin Users should not be allowed to fetch others' Personal Collections!
-(expect
- "You don't have permissions to do that."
- (api-get-lucky-personal-collection :rasta, :expected-status-code 403))
+ (testing "Other, non-admin Users should not be allowed to fetch others' Personal Collections!"
+ (is (= "You don't have permissions to do that."
+ (api-get-lucky-personal-collection :rasta, :expected-status-code 403))))))
(def ^:private lucky-personal-subcollection-item
[(collection-item "Lucky's Personal Sub-Collection" :can_write true)])
(defn- api-get-lucky-personal-collection-with-subcollection [user-kw]
- (tt/with-temp Collection [_ {:name "Lucky's Personal Sub-Collection"
+ (mt/with-temp Collection [_ {:name "Lucky's Personal Sub-Collection"
:location (collection/children-location
- (collection/user->personal-collection (user->id :lucky)))}]
- (tu/boolean-ids-and-timestamps (api-get-lucky-personal-collection-items user-kw))))
+ (collection/user->personal-collection (mt/user->id :lucky)))}]
+ (mt/boolean-ids-and-timestamps (api-get-lucky-personal-collection-items user-kw))))
-;; If we have a sub-Collection of our Personal Collection, that should show up
-(expect
- lucky-personal-subcollection-item
- (api-get-lucky-personal-collection-with-subcollection :lucky))
+(deftest fetch-personal-collection-items-test
+ (testing "GET /api/collection/:id/items"
+ (testing "If we have a sub-Collection of our Personal Collection, that should show up"
+ (is (= lucky-personal-subcollection-item
+ (api-get-lucky-personal-collection-with-subcollection :lucky))))
-;; sub-Collections of other's Personal Collections should show up for admins as well
-(expect
- lucky-personal-subcollection-item
- (api-get-lucky-personal-collection-with-subcollection :crowberto))
+ (testing "sub-Collections of other's Personal Collections should show up for admins as well"
+ (is (= lucky-personal-subcollection-item
+ (api-get-lucky-personal-collection-with-subcollection :crowberto))))))
;;; ------------------------------------ Effective Ancestors & Effective Children ------------------------------------
@@ -251,7 +302,7 @@
`(perms/grant-collection-read-permissions! (group/all-users) ~collection-symb))
~@body))
-(defn- format-ancestors-and-children
+(defn- format-ancestors
"Nicely format the `:effective_` results from an API call."
[results]
(-> results
@@ -259,420 +310,563 @@
(update :effective_ancestors (partial map #(update % :id integer?)))
(update :effective_location collection-test/location-path-ids->names)))
-(defn- api-get-collection-ancestors-and-children
+(defn- api-get-collection-ancestors
"Call the API with Rasta to fetch `collection-or-id` and put the `:effective_` results in a nice format for the tests
below."
[collection-or-id & additional-get-params]
- [(format-ancestors-and-children ((user->client :rasta) :get 200 (str "collection/" (u/get-id collection-or-id))))
- (tu/boolean-ids-and-timestamps (apply (user->client :rasta) :get 200 (str "collection/" (u/get-id collection-or-id) "/items")
- additional-get-params))])
-
-;; does a top-level Collection like A have the correct Children?
-(expect
- [{:effective_ancestors []
- :effective_location "/"}
- (map collection-item ["B" "C"])]
- (with-collection-hierarchy [a b c d g]
- (api-get-collection-ancestors-and-children a)))
-
-;; ok, does a second-level Collection have its parent and its children?
-(expect
- [{:effective_ancestors [{:name "A", :id true, :can_write false}]
- :effective_location "/A/"}
- (map collection-item ["D" "G"])]
- (with-collection-hierarchy [a b c d g]
- (api-get-collection-ancestors-and-children c)))
-
-;; what about a third-level Collection?
-(expect
- [{:effective_ancestors [{:name "A", :id true, :can_write false}
- {:name "C", :id true, :can_write false}]
- :effective_location "/A/C/"}
- []]
- (with-collection-hierarchy [a b c d g]
- (api-get-collection-ancestors-and-children d)))
-
-;; for D: if we remove perms for C we should only have A as an ancestor; effective_location should lie and say we are
-;; a child of A
-(expect
- [{:effective_ancestors [{:name "A", :id true, :can_write false}]
- :effective_location "/A/"}
- []]
- (with-collection-hierarchy [a b d g]
- (api-get-collection-ancestors-and-children d)))
-
-;; for D: If, on the other hand, we remove A, we should see C as the only ancestor and as a root-level Collection.
-(expect
- [{:effective_ancestors [{:name "C", :id true, :can_write false}]
- :effective_location "/C/"}
- []]
- (with-collection-hierarchy [b c d g]
- (api-get-collection-ancestors-and-children d)))
-
-;; for C: if we remove D we should get E and F as effective children
-(expect
- [{:effective_ancestors [{:name "A", :id true, :can_write false}]
- :effective_location "/A/"}
- (map collection-item ["E" "F"])]
- (with-collection-hierarchy [a b c e f g]
- (api-get-collection-ancestors-and-children c)))
-
-;; Make sure we can collapse multiple generations. For A: removing C and D should move up E and F
-(expect
- [{:effective_ancestors []
- :effective_location "/"}
- (map collection-item ["B" "E" "F"])]
- (with-collection-hierarchy [a b e f g]
- (api-get-collection-ancestors-and-children a)))
-
-;; Let's make sure the 'archived` option works on Collections, nested or not
-(expect
- [{:effective_ancestors []
- :effective_location "/"}
- [(collection-item "B")]]
- (with-collection-hierarchy [a b c]
- (db/update! Collection (u/get-id b) :archived true)
- (api-get-collection-ancestors-and-children a :archived true)))
+ (format-ancestors ((mt/user->client :rasta) :get 200 (str "collection/" (u/get-id collection-or-id)))))
+
+(defn- api-get-collection-children
+ [collection-or-id & additional-get-params]
+ (mt/boolean-ids-and-timestamps (apply (mt/user->client :rasta) :get 200 (str "collection/" (u/get-id collection-or-id) "/items")
+ additional-get-params)))
+
+(deftest effective-ancestors-and-children-test
+ (testing "does a top-level Collection like A have the correct Children?"
+ (with-collection-hierarchy [a b c d g]
+ (testing "ancestors"
+ (is(= {:effective_ancestors []
+ :effective_location "/"}
+ (api-get-collection-ancestors a))))
+ (testing "children"
+ (is (= (map collection-item ["B" "C"])
+ (api-get-collection-children a))))))
+
+ (testing "ok, does a second-level Collection have its parent and its children?"
+ (with-collection-hierarchy [a b c d g]
+ (testing "ancestors"
+ (is (= {:effective_ancestors [{:name "A", :id true, :can_write false}]
+ :effective_location "/A/"}
+ (api-get-collection-ancestors c))))
+ (testing "children"
+ (is (= (map collection-item ["D" "G"])
+ (api-get-collection-children c))))))
+
+ (testing "what about a third-level Collection?"
+ (with-collection-hierarchy [a b c d g]
+ (testing "ancestors"
+ (is (= {:effective_ancestors [{:name "A", :id true, :can_write false}
+ {:name "C", :id true, :can_write false}]
+ :effective_location "/A/C/"}
+ (api-get-collection-ancestors d))))
+ (testing "children"
+ (is (= []
+ (api-get-collection-children d))))))
+
+ (testing (str "for D: if we remove perms for C we should only have A as an ancestor; effective_location should lie "
+ "and say we are a child of A")
+ (with-collection-hierarchy [a b d g]
+ (testing "ancestors"
+ (is (= {:effective_ancestors [{:name "A", :id true, :can_write false}]
+ :effective_location "/A/"}
+ (api-get-collection-ancestors d))))
+ (testing "children"
+ (is (= []
+ (api-get-collection-children d))))))
+
+ (testing "for D: If, on the other hand, we remove A, we should see C as the only ancestor and as a root-level Collection."
+ (with-collection-hierarchy [b c d g]
+ (testing "ancestors"
+ (is (= {:effective_ancestors [{:name "C", :id true, :can_write false}]
+ :effective_location "/C/"}
+ (api-get-collection-ancestors d))))
+ (testing "children"
+ (is (= []
+ (api-get-collection-children d)))))
+
+ (testing "for C: if we remove D we should get E and F as effective children"
+ (with-collection-hierarchy [a b c e f g]
+ (testing "ancestors"
+ (is (= {:effective_ancestors [{:name "A", :id true, :can_write false}]
+ :effective_location "/A/"}
+ (api-get-collection-ancestors c))))
+ (testing "children"
+ (is (= (map collection-item ["E" "F"])
+ (api-get-collection-children c)))))))
+
+ (testing "Make sure we can collapse multiple generations. For A: removing C and D should move up E and F"
+ (with-collection-hierarchy [a b e f g]
+ (testing "ancestors"
+ (is (= {:effective_ancestors []
+ :effective_location "/"}
+ (api-get-collection-ancestors a))))
+ (testing "children"
+ (is (= (map collection-item ["B" "E" "F"])
+ (api-get-collection-children a))))))
+
+ (testing "Let's make sure the 'archived` option works on Collections, nested or not"
+ (with-collection-hierarchy [a b c]
+ (db/update! Collection (u/get-id b) :archived true)
+ (testing "ancestors"
+ (is (= {:effective_ancestors []
+ :effective_location "/"}
+ (api-get-collection-ancestors a :archived true))))
+ (testing "children"
+ (is (= [(collection-item "B")]
+ (api-get-collection-children a :archived true)))))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | GET /collection/root |
;;; +----------------------------------------------------------------------------------------------------------------+
-;; Check that we can see stuff that isn't in any Collection -- meaning they're in the so-called "Root" Collection
-(expect
- {:name "Our analytics"
- :id "root"
- :can_write true
- :effective_location nil
- :effective_ancestors []
- :parent_id nil}
- (with-some-children-of-collection nil
- ((user->client :crowberto) :get 200 "collection/root")))
-
-;; Make sure you can see everything for Users that can see everything
-(expect
- [(default-item {:name "Birthday Card", :description nil, :favorite false, :model "card", :display "table"})
- (collection-item "Crowberto Corv's Personal Collection")
- (default-item {:name "Dine & Dashboard", :description nil, :model "dashboard"})
- (default-item {:name "Electro-Magnetic Pulse", :model "pulse"})]
- (with-some-children-of-collection nil
- (tu/boolean-ids-and-timestamps ((user->client :crowberto) :get 200 "collection/root/items"))))
-
-;; ...but we don't let you see stuff you wouldn't otherwise be allowed to see
-(expect
- [(collection-item "Rasta Toucan's Personal Collection")]
- ;; if a User doesn't have perms for the Root Collection then they don't get to see things with no collection_id
- (with-some-children-of-collection nil
- (tu/boolean-ids-and-timestamps ((user->client :rasta) :get 200 "collection/root/items"))))
-
-;; ...but if they have read perms for the Root Collection they should get to see them
-(expect
- [(default-item {:name "Birthday Card", :description nil, :favorite false, :model "card", :display "table"})
- (default-item {:name "Dine & Dashboard", :description nil, :model "dashboard"})
- (default-item {:name "Electro-Magnetic Pulse", :model "pulse"})
- (collection-item "Rasta Toucan's Personal Collection")]
- (with-some-children-of-collection nil
- (tt/with-temp* [PermissionsGroup [group]
- PermissionsGroupMembership [_ {:user_id (user->id :rasta), :group_id (u/get-id group)}]]
- (perms/grant-permissions! group (perms/collection-read-path {:metabase.models.collection/is-root? true}))
- (tu/boolean-ids-and-timestamps ((user->client :rasta) :get 200 "collection/root/items")))))
-
-;; So I suppose my Personal Collection should show up when I fetch the Root Collection, shouldn't it...
-(expect
- [{:name "Rasta Toucan's Personal Collection"
- :id (u/get-id (collection/user->personal-collection (user->id :rasta)))
- :description nil
- :model "collection"
- :can_write true}]
- ((user->client :rasta) :get 200 "collection/root/items"))
-
-;; And for admins, only return our own Personal Collection (!)
-(expect
- [{:name "Crowberto Corv's Personal Collection"
- :id (u/get-id (collection/user->personal-collection (user->id :crowberto)))
- :description nil
- :model "collection"
- :can_write true}]
- ((user->client :crowberto) :get 200 "collection/root/items"))
-
-;; That includes sub-collections of Personal Collections! I shouldn't see them!
-(expect
- [{:name "Crowberto Corv's Personal Collection"
- :id (u/get-id (collection/user->personal-collection (user->id :crowberto)))
- :description nil
- :model "collection"
- :can_write true}]
- (tt/with-temp Collection [_ {:name "Lucky's Sub-Collection"
- :location (collection/children-location
- (collection/user->personal-collection (user->id :lucky)))}]
- ((user->client :crowberto) :get 200 "collection/root/items")))
-
-;; Can we look for `archived` stuff with this endpoint?
-(expect
- [{:name "Business Card"
- :description nil
- :collection_position nil
- :display "table"
- :favorite false
- :model "card"}]
- (tt/with-temp Card [card {:name "Business Card", :archived true}]
- (for [item ((user->client :crowberto) :get 200 "collection/root/items?archived=true")]
- (dissoc item :id))))
+(deftest fetch-root-collection-test
+ (testing "GET /api/collection/root"
+ (testing "Check that we can see stuff that isn't in any Collection -- meaning they're in the so-called \"Root\" Collection"
+ (is (= {:name "Our analytics"
+ :id "root"
+ :can_write true
+ :effective_location nil
+ :effective_ancestors []
+ :parent_id nil}
+ (with-some-children-of-collection nil
+ ((mt/user->client :crowberto) :get 200 "collection/root")))))
+
+ (testing "Make sure you can see everything for Users that can see everything"
+ (is (= [(default-item {:name "Birthday Card", :description nil, :favorite false, :model "card", :display "table"})
+ (collection-item "Crowberto Corv's Personal Collection")
+ (default-item {:name "Dine & Dashboard", :description nil, :model "dashboard"})
+ (default-item {:name "Electro-Magnetic Pulse", :model "pulse"})]
+ (with-some-children-of-collection nil
+ (-> ((mt/user->client :crowberto) :get 200 "collection/root/items")
+ (remove-non-test-items &ids)
+ mt/boolean-ids-and-timestamps))))
+
+ (testing "...but we don't let you see stuff you wouldn't otherwise be allowed to see"
+ (is (= [(collection-item "Rasta Toucan's Personal Collection")]
+ ;; if a User doesn't have perms for the Root Collection then they don't get to see things with no collection_id
+ (with-some-children-of-collection nil
+ (mt/boolean-ids-and-timestamps ((mt/user->client :rasta) :get 200 "collection/root/items")))))
+
+ (testing "...but if they have read perms for the Root Collection they should get to see them"
+ (with-some-children-of-collection nil
+ (mt/with-temp* [PermissionsGroup [group]
+ PermissionsGroupMembership [_ {:user_id (mt/user->id :rasta), :group_id (u/get-id group)}]]
+ (perms/grant-permissions! group (perms/collection-read-path {:metabase.models.collection.root/is-root? true}))
+ (is (= [(default-item {:name "Birthday Card", :description nil, :favorite false, :model "card", :display "table"})
+ (default-item {:name "Dine & Dashboard", :description nil, :model "dashboard"})
+ (default-item {:name "Electro-Magnetic Pulse", :model "pulse"})
+ (collection-item "Rasta Toucan's Personal Collection")]
+ (-> ((mt/user->client :rasta) :get 200 "collection/root/items")
+ (remove-non-test-items &ids)
+ mt/boolean-ids-and-timestamps ))))))))
+
+ (testing "So I suppose my Personal Collection should show up when I fetch the Root Collection, shouldn't it..."
+ (is (= [{:name "Rasta Toucan's Personal Collection"
+ :id (u/get-id (collection/user->personal-collection (mt/user->id :rasta)))
+ :description nil
+ :model "collection"
+ :can_write true}]
+ (->> ((mt/user->client :rasta) :get 200 "collection/root/items")
+ (filter #(str/includes? (:name %) "Personal Collection"))))))
+
+ (testing "For admins, only return our own Personal Collection (!)"
+ (is (= [{:name "Crowberto Corv's Personal Collection"
+ :id (u/get-id (collection/user->personal-collection (mt/user->id :crowberto)))
+ :description nil
+ :model "collection"
+ :can_write true}]
+ (->> ((mt/user->client :crowberto) :get 200 "collection/root/items")
+ (filter #(str/includes? (:name %) "Personal Collection")))))
+
+ (testing "That includes sub-collections of Personal Collections! I shouldn't see them!"
+ (mt/with-temp Collection [_ {:name "Lucky's Sub-Collection"
+ :location (collection/children-location
+ (collection/user->personal-collection (mt/user->id :lucky)))}]
+ (is (= [{:name "Crowberto Corv's Personal Collection"
+ :id (u/get-id (collection/user->personal-collection (mt/user->id :crowberto)))
+ :description nil
+ :model "collection"
+ :can_write true}]
+ (->> ((mt/user->client :crowberto) :get 200 "collection/root/items")
+ (filter #(str/includes? (:name %) "Personal Collection"))))))))
+
+ (testing "Can we look for `archived` stuff with this endpoint?"
+ (mt/with-temp Card [card {:name "Business Card", :archived true}]
+ (is (= [{:name "Business Card"
+ :description nil
+ :collection_position nil
+ :display "table"
+ :favorite false
+ :model "card"}]
+ (for [item ((mt/user->client :crowberto) :get 200 "collection/root/items?archived=true")]
+ (dissoc item :id))))))))
;;; ----------------------------------- Effective Children, Ancestors, & Location ------------------------------------
-(defn- api-get-root-collection-ancestors-and-children
+(defn- api-get-root-collection-ancestors
"Call the API with Rasta to fetch the 'Root' Collection and put the `:effective_` results in a nice format for the
tests below."
[& additional-get-params]
- [(format-ancestors-and-children ((user->client :rasta) :get 200 "collection/root"))
- (tu/boolean-ids-and-timestamps (apply (user->client :rasta) :get 200 "collection/root/items" additional-get-params))])
-
-;; Do top-level collections show up as children of the Root Collection?
-(expect
- [{:effective_ancestors []
- :effective_location nil}
- (map collection-item ["A" "Rasta Toucan's Personal Collection"])]
- (with-collection-hierarchy [a b c d e f g]
- (api-get-root-collection-ancestors-and-children)))
-
-;; ...and collapsing children should work for the Root Collection as well
-(expect
- [{:effective_ancestors []
- :effective_location nil}
- (map collection-item ["B" "D" "F" "Rasta Toucan's Personal Collection"])]
- (with-collection-hierarchy [b d e f g]
- (api-get-root-collection-ancestors-and-children)))
-
-;; does `archived` work on Collections as well?
-(expect
- [{:effective_ancestors []
- :effective_location nil}
- [(collection-item "A")]]
- (with-collection-hierarchy [a b d e f g]
- (db/update! Collection (u/get-id a) :archived true)
- (api-get-root-collection-ancestors-and-children :archived true)))
+ (format-ancestors ((mt/user->client :rasta) :get 200 "collection/root")))
+
+(defn- api-get-root-collection-children
+ [& additional-get-params]
+ (mt/boolean-ids-and-timestamps (apply (mt/user->client :rasta) :get 200 "collection/root/items" additional-get-params)))
+
+(deftest fetch-root-collection-items-test
+ (testing "GET /api/collection/root/items"
+ (testing "Do top-level collections show up as children of the Root Collection?"
+ (with-collection-hierarchy [a b c d e f g]
+ (testing "ancestors"
+ (is (= {:effective_ancestors []
+ :effective_location nil}
+ (api-get-root-collection-ancestors))))
+ (testing "children"
+ (is (= (map collection-item ["A" "Rasta Toucan's Personal Collection"])
+ (api-get-root-collection-children))))))
+
+ (testing "...and collapsing children should work for the Root Collection as well"
+ (with-collection-hierarchy [b d e f g]
+ (testing "ancestors"
+ (is (= {:effective_ancestors []
+ :effective_location nil}
+ (api-get-root-collection-ancestors))))
+ (testing "children"
+ (is (= (map collection-item ["B" "D" "F" "Rasta Toucan's Personal Collection"])
+ (api-get-root-collection-children))))))
+
+ (testing "does `archived` work on Collections as well?"
+ (with-collection-hierarchy [a b d e f g]
+ (db/update! Collection (u/get-id a) :archived true)
+ (testing "ancestors"
+ (is (= {:effective_ancestors []
+ :effective_location nil}
+ (api-get-root-collection-ancestors :archived true))))
+ (testing "children"
+ (is (= [(collection-item "A")]
+ (api-get-root-collection-children :archived true))))))
+
+ (testing "\n?namespace= parameter"
+ (mt/with-temp* [Collection [{normal-id :id} {:name "Normal Collection"}]
+ Collection [{coins-id :id} {:name "Coin Collection", :namespace "currency"}]]
+ (letfn [(collection-names [items]
+ (->> items
+ (filter #(and (= (:model %) "collection")
+ (#{normal-id coins-id} (:id %))))
+ (map :name)))]
+ (testing "should only show Collections in the 'default' namespace by default"
+ (is (= ["Normal Collection"]
+ (collection-names ((mt/user->client :rasta) :get 200 "collection/root/items")))))
+
+ (testing "By passing `:namespace` we should be able to see Collections in that `:namespace`"
+ (testing "?namespace=currency"
+ (is (= ["Coin Collection"]
+ (collection-names ((mt/user->client :rasta) :get 200 "collection/root/items?namespace=currency")))))
+ (testing "?namespace=stamps"
+ (is (= []
+ (collection-names ((mt/user->client :rasta) :get 200 "collection/root/items?namespace=stamps")))))))))))
+
+(deftest root-collection-snippets-test
+ (testing "GET /api/collection/root/items?namespace=snippets"
+ (testing "\nNative query snippets should come back when fetching the items in the root Collection of the `:snippets` namespace"
+ (mt/with-temp* [NativeQuerySnippet [{snippet-id :id} {:name "My Snippet"}]
+ NativeQuerySnippet [{archived-id :id} {:name "Archived Snippet", :archived true}]
+ Dashboard [{dashboard-id :id} {:name "My Dashboard"}]]
+ (letfn [(only-test-items [results]
+ (if (sequential? results)
+ (filter #(#{snippet-id archived-id dashboard-id} (:id %)) results)
+ results))
+ (only-test-item-names [results]
+ (let [items (only-test-items results)]
+ (if (sequential? items)
+ (map :name items)
+ items)))]
+ (is (= [{:id snippet-id
+ :name "My Snippet"
+ :model "snippet"}]
+ (only-test-items ((mt/user->client :rasta) :get 200 "collection/root/items?namespace=snippets"))))
+
+ (testing "\nSnippets should not come back for the default namespace"
+ (is (= ["My Dashboard"]
+ (only-test-item-names ((mt/user->client :rasta) :get 200 "collection/root/items")))))
+
+ (testing "\nShould be able to fetch archived Snippets"
+ (is (= ["Archived Snippet"]
+ (only-test-item-names ((mt/user->client :rasta) :get 200
+ "collection/root/items?namespace=snippets&archived=true")))))
+
+ (testing "\nShould be able to pass ?model=snippet, even though it makes no difference in this case"
+ (is (= ["My Snippet"]
+ (only-test-item-names ((mt/user->client :rasta) :get 200
+ "collection/root/items?namespace=snippets&model=snippet"))))))))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | POST /api/collection |
;;; +----------------------------------------------------------------------------------------------------------------+
-;; test that we can create a new collection (POST /api/collection)
-(expect
- {:name "Stamp Collection"
- :slug "stamp_collection"
- :description nil
- :color "#123456"
- :archived false
- :location "/"
- :personal_owner_id nil}
- (tu/with-model-cleanup [Collection]
- (-> ((user->client :crowberto) :post 200 "collection"
- {:name "Stamp Collection", :color "#123456"})
- (dissoc :id))))
-
-;; test that non-admins aren't allowed to create a collection in the root collection
-(expect
- "You don't have permissions to do that."
- (tu/with-non-admin-groups-no-root-collection-perms
- ((user->client :rasta) :post 403 "collection"
- {:name "Stamp Collection", :color "#123456"})))
-
-;; Can a non-admin user with Root Collection perms add a new collection to the Root Collection? (#8949)
-(expect
- {:name "Stamp Collection"
- :description nil
- :color "#123456"
- :archived false
- :location "/"
- :personal_owner_id nil
- :slug "stamp_collection"}
- (tu/with-model-cleanup [Collection]
- (tu/with-non-admin-groups-no-root-collection-perms
- (-> (tt/with-temp* [PermissionsGroup [group]
- PermissionsGroupMembership [_ {:user_id (user->id :rasta), :group_id (u/get-id group)}]]
+(deftest create-collection-test
+ (testing "POST /api/collection"
+ (testing "\ntest that we can create a new collection"
+ (mt/with-model-cleanup [Collection]
+ (is (= (merge
+ (mt/object-defaults Collection)
+ {:name "Stamp Collection"
+ :slug "stamp_collection"
+ :color "#123456"
+ :archived false
+ :location "/"
+ :personal_owner_id nil})
+ (-> ((mt/user->client :crowberto) :post 200 "collection"
+ {:name "Stamp Collection", :color "#123456"})
+ (dissoc :id))))))
+
+ (testing "\ntest that non-admins aren't allowed to create a collection in the root collection"
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (is (= "You don't have permissions to do that."
+ ((mt/user->client :rasta) :post 403 "collection"
+ {:name "Stamp Collection", :color "#123456"})))))
+
+ (testing "\nCan a non-admin user with Root Collection perms add a new collection to the Root Collection? (#8949)"
+ (mt/with-model-cleanup [Collection]
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp* [PermissionsGroup [group]
+ PermissionsGroupMembership [_ {:user_id (mt/user->id :rasta), :group_id (u/get-id group)}]]
(perms/grant-collection-readwrite-permissions! group collection/root-collection)
- ((user->client :rasta) :post 200 "collection"
- {:name "Stamp Collection", :color "#123456"}))
- (dissoc :id)))))
-
-;; Can I create a Collection as a child of an existing collection?
-(expect
- {:id true
- :name "Trading Card Collection"
- :slug "trading_card_collection"
- :description "Collection of basketball cards including limited-edition holographic Draymond Green"
- :color "#ABCDEF"
- :archived false
- :location "/A/C/D/"
- :personal_owner_id nil}
- (tu/with-model-cleanup [Collection]
- (with-collection-hierarchy [a c d]
- (-> ((user->client :crowberto) :post 200 "collection"
- {:name "Trading Card Collection"
- :color "#ABCDEF"
- :description "Collection of basketball cards including limited-edition holographic Draymond Green"
- :parent_id (u/get-id d)})
- (update :location collection-test/location-path-ids->names)
- (update :id integer?)))))
-
+ (is (= (merge
+ (mt/object-defaults Collection)
+ {:name "Stamp Collection"
+ :color "#123456"
+ :location "/"
+ :slug "stamp_collection"})
+ (dissoc ((mt/user->client :rasta) :post 200 "collection"
+ {:name "Stamp Collection", :color "#123456"})
+ :id)))))))
+
+ (testing "\nCan I create a Collection as a child of an existing collection?"
+ (mt/with-model-cleanup [Collection]
+ (with-collection-hierarchy [a c d]
+ (is (= (merge
+ (mt/object-defaults Collection)
+ {:id true
+ :name "Trading Card Collection"
+ :slug "trading_card_collection"
+ :description "Collection of basketball cards including limited-edition holographic Draymond Green"
+ :color "#ABCDEF"
+ :location "/A/C/D/"})
+ (-> ((mt/user->client :crowberto) :post 200 "collection"
+ {:name "Trading Card Collection"
+ :color "#ABCDEF"
+ :description "Collection of basketball cards including limited-edition holographic Draymond Green"
+ :parent_id (u/get-id d)})
+ (update :location collection-test/location-path-ids->names)
+ (update :id integer?)))))))
+
+ (testing "\nShould be able to create a Collection in a different namespace"
+ (let [collection-name (mt/random-name)]
+ (try
+ (is (schema= {:name (s/eq collection-name)
+ :namespace (s/eq "snippets")
+ s/Keyword s/Any}
+ ((mt/user->client :crowberto) :post 200 "collection"
+ {:name collection-name
+ :color "#f38630"
+ :descrption "My SQL Snippets"
+ :namespace "snippets"})))
+ (finally
+ (db/delete! Collection :name collection-name)))))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | PUT /api/collection/:id |
;;; +----------------------------------------------------------------------------------------------------------------+
-;; test that we can update a collection (PUT /api/collection/:id)
-(tt/expect-with-temp [Collection [collection]]
- {:id (u/get-id collection)
- :name "My Beautiful Collection"
- :slug "my_beautiful_collection"
- :description nil
- :color "#ABCDEF"
- :archived false
- :location "/"
- :personal_owner_id nil}
- ((user->client :crowberto) :put 200 (str "collection/" (u/get-id collection))
- {:name "My Beautiful Collection", :color "#ABCDEF"}))
-
-;; check that users without write perms aren't allowed to update a Collection
-(expect
- "You don't have permissions to do that."
- (tu/with-non-admin-groups-no-root-collection-perms
- (tt/with-temp Collection [collection]
- ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection))
- {:name "My Beautiful Collection", :color "#ABCDEF"}))))
-
-;; Archiving a collection should delete any alerts associated with questions in the collection
-(expect
- {:emails (merge (et/email-to :crowberto {:subject "One of your alerts has stopped working",
- :body {"the question was archived by Crowberto Corv" true}})
- (et/email-to :rasta {:subject "One of your alerts has stopped working",
- :body {"the question was archived by Crowberto Corv" true}}))
- :pulse nil}
- (tt/with-temp* [Collection [{collection-id :id}]
- Card [{card-id :id :as card} {:collection_id collection-id}]
- Pulse [{pulse-id :id} {:alert_condition "rows"
- :alert_first_only false
- :creator_id (user->id :rasta)
- :name "Original Alert Name"}]
-
- PulseCard [_ {:pulse_id pulse-id
- :card_id card-id
- :position 0}]
- PulseChannel [{pc-id :id} {:pulse_id pulse-id}]
- PulseChannelRecipient [{pcr-id-1 :id} {:user_id (user->id :crowberto)
- :pulse_channel_id pc-id}]
- PulseChannelRecipient [{pcr-id-2 :id} {:user_id (user->id :rasta)
- :pulse_channel_id pc-id}]]
- (et/with-fake-inbox
- (et/with-expected-messages 2
- ((user->client :crowberto) :put 200 (str "collection/" collection-id)
- {:name "My Beautiful Collection", :color "#ABCDEF", :archived true}))
- (array-map
- :emails (et/regex-email-bodies #"the question was archived by Crowberto Corv")
- :pulse (Pulse pulse-id)))))
-
-;; I shouldn't be allowed to archive a Collection without proper perms
-(expect
- "You don't have permissions to do that."
- (tu/with-non-admin-groups-no-root-collection-perms
- (tt/with-temp Collection [collection]
- ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection))
- {:archived true}))))
-
-;; Perms checking should be recursive as well...
-;;
-;; Create Collections A > B, and grant permissions for A. You should not be allowed to archive A because you would
-;; also need perms for B
-(expect
- "You don't have permissions to do that."
- (tu/with-non-admin-groups-no-root-collection-perms
- (tt/with-temp* [Collection [collection-a]
- Collection [collection-b {:location (collection/children-location collection-a)}]]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection-a)
- ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
- {:archived true}))))
-
-;; Can I *change* the `location` of a Collection? (i.e. move it into a different parent Colleciton)
-(expect
- {:id true
- :name "E"
- :slug "e"
- :description nil
- :color "#ABCDEF"
- :archived false
- :location "/A/B/"
- :personal_owner_id nil}
- (with-collection-hierarchy [a b e]
- (-> ((user->client :crowberto) :put 200 (str "collection/" (u/get-id e))
- {:parent_id (u/get-id b)})
- (update :location collection-test/location-path-ids->names)
- (update :id integer?))))
-
-;; I shouldn't be allowed to move the Collection without proper perms.
-;; If I want to move A into B, I should need permissions for both A and B
-(expect
- "You don't have permissions to do that."
- (tu/with-non-admin-groups-no-root-collection-perms
- (tt/with-temp* [Collection [collection-a]
- Collection [collection-b]]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection-a)
- ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
- {:parent_id (u/get-id collection-b)}))))
-
-;; Perms checking should be recursive as well...
-;;
-;; Create A, B, and C; B is a child of A. Grant perms for A and B. Moving A into C should fail because we need perms
-;; for C:
-;; (collections with readwrite perms marked below with a `*`)
-;;
-;; A* -> B* ==> C -> A -> B
-;; C
-(expect
- "You don't have permissions to do that."
- (tu/with-non-admin-groups-no-root-collection-perms
- (tt/with-temp* [Collection [collection-a]
- Collection [collection-b {:location (collection/children-location collection-a)}]
- Collection [collection-c]]
- (doseq [collection [collection-a collection-b]]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection))
- ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
- {:parent_id (u/get-id collection-c)}))))
-
-
-;; Create A, B, and C; B is a child of A. Grant perms for A and C. Moving A into C should fail because we need perms
-;; for B:
-;; (collections with readwrite perms marked below with a `*`)
-;;
-;; A* -> B ==> C -> A -> B
-;; C*
-(expect
- "You don't have permissions to do that."
- (tu/with-non-admin-groups-no-root-collection-perms
- (tt/with-temp* [Collection [collection-a]
- Collection [collection-b {:location (collection/children-location collection-a)}]
- Collection [collection-c]]
- (doseq [collection [collection-a collection-c]]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection))
- ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
- {:parent_id (u/get-id collection-c)}))))
-
-;; Create A, B, and C; B is a child of A. Grant perms for B and C. Moving A into C should fail because we need perms
-;; for A:
-;; (collections with readwrite perms marked below with a `*`)
-;;
-;; A -> B* ==> C -> A -> B
-;; C*
-(expect
- "You don't have permissions to do that."
- (tu/with-non-admin-groups-no-root-collection-perms
- (tt/with-temp* [Collection [collection-a]
- Collection [collection-b {:location (collection/children-location collection-a)}]
- Collection [collection-c]]
- (doseq [collection [collection-b collection-c]]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection))
- ((user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
- {:parent_id (u/get-id collection-c)}))))
+(deftest update-collection-test
+ (testing "PUT /api/collection/:id"
+ (testing "test that we can update a collection"
+ (mt/with-temp Collection [collection]
+ (is (= (merge
+ (mt/object-defaults Collection)
+ {:id (u/get-id collection)
+ :name "My Beautiful Collection"
+ :slug "my_beautiful_collection"
+ :color "#ABCDEF"
+ :location "/"})
+ ((mt/user->client :crowberto) :put 200 (str "collection/" (u/get-id collection))
+ {:name "My Beautiful Collection", :color "#ABCDEF"})))))
+
+ (testing "check that users without write perms aren't allowed to update a Collection"
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp Collection [collection]
+ (is (= "You don't have permissions to do that."
+ ((mt/user->client :rasta) :put 403 (str "collection/" (u/get-id collection))
+ {:name "My Beautiful Collection", :color "#ABCDEF"}))))))))
+
+(deftest archive-collection-test
+ (testing "PUT /api/collection/:id"
+ (testing "Archiving a collection should delete any alerts associated with questions in the collection"
+ (mt/with-temp* [Collection [{collection-id :id}]
+ Card [{card-id :id :as card} {:collection_id collection-id}]
+ Pulse [{pulse-id :id} {:alert_condition "rows"
+ :alert_first_only false
+ :creator_id (mt/user->id :rasta)
+ :name "Original Alert Name"}]
+
+ PulseCard [_ {:pulse_id pulse-id
+ :card_id card-id
+ :position 0}]
+ PulseChannel [{pc-id :id} {:pulse_id pulse-id}]
+ PulseChannelRecipient [{pcr-id-1 :id} {:user_id (mt/user->id :crowberto)
+ :pulse_channel_id pc-id}]
+ PulseChannelRecipient [{pcr-id-2 :id} {:user_id (mt/user->id :rasta)
+ :pulse_channel_id pc-id}]]
+ (mt/with-fake-inbox
+ (mt/with-expected-messages 2
+ ((mt/user->client :crowberto) :put 200 (str "collection/" collection-id)
+ {:name "My Beautiful Collection", :color "#ABCDEF", :archived true}))
+ (testing "emails"
+ (is (= (merge (mt/email-to :crowberto {:subject "One of your alerts has stopped working",
+ :body {"the question was archived by Crowberto Corv" true}})
+ (mt/email-to :rasta {:subject "One of your alerts has stopped working",
+ :body {"the question was archived by Crowberto Corv" true}}))
+ (mt/regex-email-bodies #"the question was archived by Crowberto Corv"))))
+ (testing "Pulse"
+ (is (= nil
+ (Pulse pulse-id)))))))
+
+ (testing "I shouldn't be allowed to archive a Collection without proper perms"
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp Collection [collection]
+ (is (= "You don't have permissions to do that."
+ ((mt/user->client :rasta) :put 403 (str "collection/" (u/get-id collection))
+ {:archived true})))))
+
+ (testing "Perms checking should be recursive as well..."
+ ;; Create Collections A > B, and grant permissions for A. You should not be allowed to archive A because you
+ ;; would also need perms for B
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp* [Collection [collection-a]
+ Collection [collection-b {:location (collection/children-location collection-a)}]]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection-a)
+ (is (= "You don't have permissions to do that."
+ ((mt/user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
+ {:archived true})))))))))
+
+(deftest move-collection-test
+ (testing "PUT /api/collection/:id"
+ (testing "Can I *change* the `location` of a Collection? (i.e. move it into a different parent Collection)"
+ (with-collection-hierarchy [a b e]
+ (is (= (merge
+ (mt/object-defaults Collection)
+ {:id true
+ :name "E"
+ :slug "e"
+ :color "#ABCDEF"
+ :location "/A/B/"})
+ (-> ((mt/user->client :crowberto) :put 200 (str "collection/" (u/get-id e))
+ {:parent_id (u/get-id b)})
+ (update :location collection-test/location-path-ids->names)
+ (update :id integer?))))))
+
+ (testing "I shouldn't be allowed to move the Collection without proper perms."
+ (testing "If I want to move A into B, I should need permissions for both A and B"
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp* [Collection [collection-a]
+ Collection [collection-b]]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection-a)
+ (is (= "You don't have permissions to do that."
+ ((mt/user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
+ {:parent_id (u/get-id collection-b)}))))))
+
+ (testing "Perms checking should be recursive as well..."
+ (testing "Create A, B, and C; B is a child of A."
+ (testing "Grant perms for A and B. Moving A into C should fail because we need perms for C"
+ ;; (collections with readwrite perms marked below with a `*`)
+ ;; A* -> B* ==> C -> A -> B
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp* [Collection [collection-a]
+ Collection [collection-b {:location (collection/children-location collection-a)}]
+ Collection [collection-c]]
+ (doseq [collection [collection-a collection-b]]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection))
+ (is (= "You don't have permissions to do that."
+ ((mt/user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
+ {:parent_id (u/get-id collection-c)}))))))
+
+ (testing "Grant perms for A and C. Moving A into C should fail because we need perms for B."
+ ;; A* -> B ==> C -> A -> B
+ ;; C*
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp* [Collection [collection-a]
+ Collection [collection-b {:location (collection/children-location collection-a)}]
+ Collection [collection-c]]
+ (doseq [collection [collection-a collection-c]]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection))
+ (is (= "You don't have permissions to do that."
+ ((mt/user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
+ {:parent_id (u/get-id collection-c)}))))))
+
+ (testing "Grant perms for B and C. Moving A into C should fail because we need perms for A"
+ ;; A -> B* ==> C -> A -> B
+ ;; C*
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp* [Collection [collection-a]
+ Collection [collection-b {:location (collection/children-location collection-a)}]
+ Collection [collection-c]]
+ (doseq [collection [collection-b collection-c]]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection))
+ (is (= "You don't have permissions to do that."
+ ((mt/user->client :rasta) :put 403 (str "collection/" (u/get-id collection-a))
+ {:parent_id (u/get-id collection-c)})))))))))))
+
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | GET /api/collection/graph and PUT /api/collection/graph |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(deftest graph-test
+ (mt/with-temp* [Collection [{default-a :id} {:location "/"}]
+ Collection [{default-ab :id} {:location (format "/%d/" default-a)}]
+ Collection [{currency-a :id} {:namespace "currency", :location "/"}]
+ Collection [{currency-ab :id} {:namespace "currency", :location (format "/%d/" currency-a)}]
+ PermissionsGroup [{group-id :id}]]
+ (letfn [(nice-graph [graph]
+ (let [id->alias {default-a "Default A"
+ default-ab "Default A -> B"
+ currency-a "Currency A"
+ currency-ab "Currency A -> B"}]
+ (transduce
+ identity
+ (fn
+ ([graph]
+ (-> (get-in graph [:groups (keyword (str group-id))])
+ (select-keys (vals id->alias))))
+ ([graph [collection-id k]]
+ (graph.test/replace-collection-ids collection-id graph k)))
+ graph
+ id->alias)))]
+ (doseq [collection [default-a default-ab currency-a currency-ab]]
+ (perms/grant-collection-read-permissions! group-id collection))
+ (testing "GET /api/collection/graph\n"
+ (testing "Should be able to fetch the permissions graph for the default namespace"
+ (is (= {"Default A" "read", "Default A -> B" "read"}
+ (nice-graph ((mt/user->client :crowberto) :get 200 "collection/graph")))))
+
+ (testing "Should be able to fetch the permissions graph for a non-default namespace"
+ (is (= {"Currency A" "read", "Currency A -> B" "read"}
+ (nice-graph ((mt/user->client :crowberto) :get 200 "collection/graph?namespace=currency")))))
+
+ (testing "have to be a superuser"
+ (is (= "You don't have permissions to do that."
+ ((mt/user->client :rasta) :get 403 "collection/graph")))))
+
+ (testing "PUT /api/collection/graph\n"
+ (testing "Should be able to update the graph for the default namespace.\n"
+ (testing "Should ignore updates to Collections outside of the namespace"
+ (let [response ((mt/user->client :crowberto) :put 200 "collection/graph"
+ (assoc (graph/graph) :groups {group-id {default-ab :write, currency-ab :write}}))]
+ (is (= {"Default A" "read", "Default A -> B" "write"}
+ (nice-graph response))))))
+
+ (testing "Should be able to update the graph for a non-default namespace.\n"
+ (testing "Should ignore updates to Collections outside of the namespace"
+ (let [response ((mt/user->client :crowberto) :put 200 "collection/graph?namespace=currency"
+ (assoc (graph/graph) :groups {group-id {default-a :write, currency-a :write}}))]
+ (is (= {"Currency A" "write", "Currency A -> B" "read"}
+ (nice-graph response))))))
+
+ (testing "have to be a superuser"
+ (is (= "You don't have permissions to do that."
+ ((mt/user->client :rasta) :put 403 "collection/graph?namespace=currency"
+ (assoc (graph/graph) :groups {group-id {default-a :write, currency-a :write}})))))))))
diff --git a/test/metabase/api/search_test.clj b/test/metabase/api/search_test.clj
index 24cb63b3c920211cfd316a49bcfe2a012a083c2a..eada0b6f1f9ff53eb735d207999dc43f103e3da3 100644
--- a/test/metabase/api/search_test.clj
+++ b/test/metabase/api/search_test.clj
@@ -154,7 +154,7 @@
(with-search-items-in-root-collection "test"
(mt/with-temp* [PermissionsGroup [group]
PermissionsGroupMembership [_ {:user_id (test-users/user->id :rasta), :group_id (u/get-id group)}]]
- (perms/grant-permissions! group (perms/collection-read-path {:metabase.models.collection/is-root? true}))
+ (perms/grant-permissions! group (perms/collection-read-path {:metabase.models.collection.root/is-root? true}))
(is (= (remove (comp #{"collection"} :model) (default-search-results))
(search-request :rasta :q "test")))))))
@@ -180,7 +180,7 @@
(with-search-items-in-root-collection "test2"
(mt/with-temp* [PermissionsGroup [group]
PermissionsGroupMembership [_ {:user_id (test-users/user->id :rasta), :group_id (u/get-id group)}]]
- (perms/grant-permissions! group (perms/collection-read-path {:metabase.models.collection/is-root? true}))
+ (perms/grant-permissions! group (perms/collection-read-path {:metabase.models.collection.root/is-root? true}))
(perms/grant-collection-read-permissions! group collection)
(is (= (sorted-results
(into
@@ -312,3 +312,13 @@
(perms/revoke-permissions! (group/all-users) db-id)
(is (= []
(search-request :rasta :q (:name table))))))))
+
+(deftest collection-namespaces-test
+ (testing "Search should only return Collections in the 'default' namespace"
+ (mt/with-temp* [Collection [c1 {:name "Normal Collection"}]
+ Collection [c2 {:name "Coin Collection", :namespace "currency"}]]
+ (is (= ["Normal Collection"]
+ (->> (search-request :crowberto :q "Collection")
+ (filter #(and (= (:model %) "collection")
+ (#{"Normal Collection" "Coin Collection"} (:name %))))
+ (map :name)))))))
diff --git a/test/metabase/models/card_test.clj b/test/metabase/models/card_test.clj
index 6f99eae4ed5974518824b41d471e4fef8e7a3b43..3f3198a06350282300ddd27fb717e57fd145340d 100644
--- a/test/metabase/models/card_test.clj
+++ b/test/metabase/models/card_test.clj
@@ -1,103 +1,105 @@
(ns metabase.models.card-test
(:require [cheshire.core :as json]
- [expectations :refer :all]
- [metabase.models
- [card :as card :refer :all]
- [dashboard :refer [Dashboard]]
- [dashboard-card :refer [DashboardCard]]]
- [metabase.test
- [data :as data]
- [util :as tu]]
- [metabase.util :as u]
+ [clojure.test :refer :all]
+ [metabase
+ [models :refer [Card Collection Dashboard DashboardCard]]
+ [test :as mt]
+ [util :as u]]
+ [metabase.models.card :as card]
+ [metabase.test.util :as tu]
[toucan.db :as db]
[toucan.util.test :as tt]))
-;; Check that the :dashboard_count delay returns the correct count of Dashboards a Card is in
-(expect
- [0 1 2]
- (tt/with-temp* [Card [{card-id :id}]
- Dashboard [dash-1]
- Dashboard [dash-2]]
- (let [add-card-to-dash! (fn [dash] (db/insert! DashboardCard :card_id card-id, :dashboard_id (u/get-id dash)))
- get-dashboard-count (fn [] (dashboard-count (Card card-id)))]
-
- [(get-dashboard-count)
- (do (add-card-to-dash! dash-1) (get-dashboard-count))
- (do (add-card-to-dash! dash-2) (get-dashboard-count))])))
-
-
-;; card-dependencies
-
-(expect
- {:Segment #{2 3}
- :Metric #{}}
- (card-dependencies
- {:dataset_query {:type :query
- :query {:filter [:and
- [:> [:field-id 4] "2014-10-19"]
- [:= [:field-id 5] "yes"]
- [:segment 2]
- [:segment 3]]}}}))
-
-(expect
- {:Segment #{1}
- :Metric #{7}}
- (card-dependencies
- {:dataset_query {:type :query
- :query {:aggregation [:metric 7]
- :filter [:and
- [:> [:field-id 4] "2014-10-19"]
- [:= [:field-id 5] "yes"]
- [:or [:segment 1] [:!= [:field-id 5] "5"]]]}}}))
-
-(expect
- {:Segment #{}
- :Metric #{}}
- (card-dependencies
- {:dataset_query {:type :query
- :query {:aggregation nil
- :filter nil}}}))
-
-
-;; Test that when somebody archives a Card, it is removed from any Dashboards it belongs to
-(expect
- 0
- (tt/with-temp* [Dashboard [dashboard]
- Card [card]
- DashboardCard [dashcard {:dashboard_id (u/get-id dashboard), :card_id (u/get-id card)}]]
- (db/update! Card (u/get-id card) :archived true)
- (db/count DashboardCard :dashboard_id (u/get-id dashboard))))
-
-
-;; test that a Card's :public_uuid comes back if public sharing is enabled...
-(expect
- (tu/with-temporary-setting-values [enable-public-sharing true]
- (tt/with-temp Card [card {:public_uuid (str (java.util.UUID/randomUUID))}]
- (boolean (:public_uuid card)))))
-
-;; ...but if public sharing is *disabled* it should come back as `nil`
-(expect
- nil
- (tu/with-temporary-setting-values [enable-public-sharing false]
- (tt/with-temp Card [card {:public_uuid (str (java.util.UUID/randomUUID))}]
- (:public_uuid card))))
+(deftest dashboard-count-test
+ (testing "Check that the :dashboard_count delay returns the correct count of Dashboards a Card is in"
+ (tt/with-temp* [Card [{card-id :id}]
+ Dashboard [dash-1]
+ Dashboard [dash-2]]
+ (letfn [(add-card-to-dash! [dash]
+ (db/insert! DashboardCard :card_id card-id, :dashboard_id (u/get-id dash)))
+ (get-dashboard-count []
+ (card/dashboard-count (Card card-id)))]
+ (is (= 0
+ (get-dashboard-count)))
+ (testing "add to a Dashboard"
+ (add-card-to-dash! dash-1)
+ (is (= 1
+ (get-dashboard-count))))
+ (testing "add to a second Dashboard"
+ (add-card-to-dash! dash-2)
+ (is (= 2
+ (get-dashboard-count))))))))
+
+(deftest card-dependencies-test
+ (testing "Segment dependencies"
+ (is (= {:Segment #{2 3}
+ :Metric #{}}
+ (card/card-dependencies
+ {:dataset_query {:type :query
+ :query {:filter [:and
+ [:> [:field-id 4] "2014-10-19"]
+ [:= [:field-id 5] "yes"]
+ [:segment 2]
+ [:segment 3]]}}}))))
+
+ (testing "Segment and Metric dependencies"
+ (is (= {:Segment #{1}
+ :Metric #{7}}
+ (card/card-dependencies
+ {:dataset_query {:type :query
+ :query {:aggregation [:metric 7]
+ :filter [:and
+ [:> [:field-id 4] "2014-10-19"]
+ [:= [:field-id 5] "yes"]
+ [:or [:segment 1] [:!= [:field-id 5] "5"]]]}}}))))
+
+ (testing "no dependencies"
+ (is (= {:Segment #{}
+ :Metric #{}}
+ (card/card-dependencies
+ {:dataset_query {:type :query
+ :query {:aggregation nil
+ :filter nil}}})))))
+
+(deftest remove-from-dashboards-when-archiving-test
+ (testing "Test that when somebody archives a Card, it is removed from any Dashboards it belongs to"
+ (tt/with-temp* [Dashboard [dashboard]
+ Card [card]
+ DashboardCard [dashcard {:dashboard_id (u/get-id dashboard), :card_id (u/get-id card)}]]
+ (db/update! Card (u/get-id card) :archived true)
+ (is (= 0
+ (db/count DashboardCard :dashboard_id (u/get-id dashboard)))))))
+
+(deftest public-sharing-test
+ (testing "test that a Card's :public_uuid comes back if public sharing is enabled..."
+ (tu/with-temporary-setting-values [enable-public-sharing true]
+ (tt/with-temp Card [card {:public_uuid (str (java.util.UUID/randomUUID))}]
+ (is (schema= u/uuid-regex
+ (:public_uuid card)))))
+
+ (testing "...but if public sharing is *disabled* it should come back as `nil`"
+ (tu/with-temporary-setting-values [enable-public-sharing false]
+ (tt/with-temp Card [card {:public_uuid (str (java.util.UUID/randomUUID))}]
+ (is (= nil
+ (:public_uuid card))))))))
(defn- dummy-dataset-query [database-id]
- {:database (data/id)
+ {:database (mt/id)
:type :native
:native {:query "SELECT count(*) FROM toucan_sightings;"}})
-(expect
- [{:name "some name" :database_id (data/id)}
- {:name "another name" :database_id (data/id)}]
+(deftest database-id-test
(tt/with-temp Card [{:keys [id] :as card} {:name "some name"
- :dataset_query (dummy-dataset-query (data/id))
- :database_id (data/id)}]
- [(into {} (db/select-one [Card :name :database_id] :id id))
- (do
- (db/update! Card id {:name "another name"
- :dataset_query (dummy-dataset-query (data/id))})
- (into {} (db/select-one [Card :name :database_id] :id id)))]))
+ :dataset_query (dummy-dataset-query (mt/id))
+ :database_id (mt/id)}]
+ (testing "before update"
+ (is (= {:name "some name", :database_id (mt/id)}
+ (into {} (db/select-one [Card :name :database_id] :id id)))))
+ (db/update! Card id {:name "another name"
+ :dataset_query (dummy-dataset-query (mt/id))})
+ (testing "after update"
+ (is (= {:name "another name" :database_id (mt/id)}
+ (into {} (db/select-one [Card :name :database_id] :id id)))))))
;;; ------------------------------------------ Circular Reference Detection ------------------------------------------
@@ -106,7 +108,7 @@
"Generate values for a Card with `source-table` for use with `with-temp`."
{:style/indent 1}
[source-table & {:as kvs}]
- (merge {:dataset_query {:database (data/id)
+ (merge {:dataset_query {:database (mt/id)
:type :query
:query {:source-table source-table}}}
kvs))
@@ -119,39 +121,55 @@
;; we have to manually JSON-encode since we're skipping normal pre-update stuff
(update :dataset_query json/generate-string))}))
-;; If a Card uses itself as a source, perms calculations should fallback to the 'only admins can see it' perms of
-;; #{"/db/0"} (DB 0 will never exist, so regular users will never get to see it, but because admins have root perms,
-;; they will still get to see it and perhaps fix it.)
-(expect
- Exception
- (tt/with-temp Card [card (card-with-source-table (data/id :venues))]
- ;; now try to make the Card reference itself. Should throw Exception
- (db/update! Card (u/get-id card)
- (card-with-source-table (str "card__" (u/get-id card))))))
-
-;; Do the same stuff with circular reference between two Cards... (A -> B -> A)
-(expect
- Exception
- (tt/with-temp* [Card [card-a (card-with-source-table (data/id :venues))]
- Card [card-b (card-with-source-table (str "card__" (u/get-id card-a)))]]
- (db/update! Card (u/get-id card-a)
- (card-with-source-table (str "card__" (u/get-id card-b))))))
-
-;; ok now try it with A -> C -> B -> A
-(expect
- Exception
- (tt/with-temp* [Card [card-a (card-with-source-table (data/id :venues))]
- Card [card-b (card-with-source-table (str "card__" (u/get-id card-a)))]
- Card [card-c (card-with-source-table (str "card__" (u/get-id card-b)))]]
- (db/update! Card (u/get-id card-a)
- (card-with-source-table (str "card__" (u/get-id card-c))))))
-
-(expect
- #{1}
- (#'card/extract-ids :segment {:query {:fields [[:segment 1]
- [:metric 2]]}}))
-
-(expect
- #{2}
- (#'card/extract-ids :metric {:query {:fields [[:segment 1]
- [:metric 2]]}}))
+(deftest circular-reference-test
+ (testing "Should throw an Exception if saving a Card that references itself"
+ (tt/with-temp Card [card (card-with-source-table (mt/id :venues))]
+ ;; now try to make the Card reference itself. Should throw Exception
+ (is (thrown?
+ Exception
+ (db/update! Card (u/get-id card)
+ (card-with-source-table (str "card__" (u/get-id card))))))))
+
+ (testing "Do the same stuff with circular reference between two Cards... (A -> B -> A)"
+ (tt/with-temp* [Card [card-a (card-with-source-table (mt/id :venues))]
+ Card [card-b (card-with-source-table (str "card__" (u/get-id card-a)))]]
+ (is (thrown?
+ Exception
+ (db/update! Card (u/get-id card-a)
+ (card-with-source-table (str "card__" (u/get-id card-b))))))))
+
+ (testing "ok now try it with A -> C -> B -> A"
+ (tt/with-temp* [Card [card-a (card-with-source-table (mt/id :venues))]
+ Card [card-b (card-with-source-table (str "card__" (u/get-id card-a)))]
+ Card [card-c (card-with-source-table (str "card__" (u/get-id card-b)))]]
+ (is (thrown?
+ Exception
+ (db/update! Card (u/get-id card-a)
+ (card-with-source-table (str "card__" (u/get-id card-c)))))))))
+
+(deftest extract-ids-test
+ (doseq [[ids-type expected] {:segment #{1}
+ :metric #{2}}]
+ (testing (pr-str (list 'extract-ids ids-type 'card))
+ (is (= expected
+ (#'card/extract-ids ids-type {:query {:fields [[:segment 1]
+ [:metric 2]]}}))))))
+
+(deftest validate-collection-namespace-test
+ (mt/with-temp Collection [{collection-id :id} {:namespace "currency"}]
+ (testing "Shouldn't be able to create a Card in a non-normal Collection"
+ (let [card-name (mt/random-name)]
+ (try
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"A Card can only go in Collections in the \"default\" namespace"
+ (db/insert! Card (assoc (tt/with-temp-defaults Card) :collection_id collection-id, :name card-name))))
+ (finally
+ (db/delete! Card :name card-name)))))
+
+ (testing "Shouldn't be able to move a Card to a non-normal Collection"
+ (mt/with-temp Card [{card-id :id}]
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"A Card can only go in Collections in the \"default\" namespace"
+ (db/update! Card card-id {:collection_id collection-id})))))))
diff --git a/test/metabase/models/collection/graph_test.clj b/test/metabase/models/collection/graph_test.clj
new file mode 100644
index 0000000000000000000000000000000000000000..3f3fe161ed76d4d16d8dcd6cc5ecc673150185cd
--- /dev/null
+++ b/test/metabase/models/collection/graph_test.clj
@@ -0,0 +1,383 @@
+(ns metabase.models.collection.graph-test
+ (:require [clojure.test :refer :all]
+ [medley.core :as m]
+ [metabase
+ [test :as mt]
+ [util :as u]]
+ [metabase.api.common :refer [*current-user-id*]]
+ [metabase.models
+ [collection :as collection :refer [Collection]]
+ [collection-revision :as collection-revision :refer [CollectionRevision]]
+ [permissions :as perms]
+ [permissions-group :as group :refer [PermissionsGroup]]]
+ [metabase.models.collection.graph :as graph]
+ [metabase.test.fixtures :as fixtures]
+ [metabase.util.schema :as su]
+ [schema.core :as s]
+ [toucan.db :as db]))
+
+(use-fixtures :once (fixtures/initialize :db :test-users :test-users-personal-collections))
+
+(defn- lucky-collection-children-location []
+ (collection/children-location (collection/user->personal-collection (mt/user->id :lucky))))
+
+(defn replace-collection-ids
+ "In Collection perms `graph`, replace instances of the ID of `collection-or-id` with `:COLLECTION`, making it possible
+ to write tests that don't need to know its actual numeric ID."
+ ([collection-or-id graph]
+ (replace-collection-ids collection-or-id graph :COLLECTION))
+
+ ([collection-or-id graph replacement-key]
+ (let [id (u/get-id collection-or-id)
+ ;; match variations that pop up depending on whether the map was serialized to JSON. 100, :100, or "100"
+ id-keys #{id (str id) (keyword (str id))}]
+ (update graph :groups (partial m/map-vals (partial m/map-keys (fn [collection-id]
+ (if (id-keys collection-id)
+ replacement-key
+ collection-id))))))))
+
+(defn- clear-graph-revisions! []
+ (db/delete! CollectionRevision))
+
+(defn- only-groups
+ "Remove entries for non-'magic' groups from a fetched perms `graph`."
+ [graph groups-or-ids]
+ (update graph :groups select-keys (map u/get-id groups-or-ids)))
+
+(defn- only-collections
+ "Remove entries for Collections whose ID is not in `collection-ids` from a fetched perms `graph`."
+ [graph collections-or-ids]
+ (let [ids (for [collection-or-id collections-or-ids]
+ (if (= :root collection-or-id)
+ collection-or-id
+ (u/get-id collection-or-id)))]
+ (update graph :groups (fn [groups]
+ (m/map-vals #(select-keys % ids) groups)))))
+
+(defn- graph
+ "Fetch collection graph.
+
+ * `:clear-revisions?` = delete any previously existing collection revision entries so we get revision = 0
+ * `:collections` = IDs of Collections to keep. `:root` is always kept.
+ * `:groups` = IDs of Groups to keep. 'Magic' groups are always kept."
+ [& {:keys [clear-revisions? collections groups]}]
+ (when clear-revisions?
+ (clear-graph-revisions!))
+ ;; force lazy creation of the three magic groups as needed
+ (group/all-users)
+ (group/admin)
+ (group/metabot)
+ ;; now fetch the graph
+ (cond-> (-> (graph/graph)
+ (only-groups (concat [(group/all-users) (group/metabot) (group/admin)] groups))
+ (only-collections (cons :root collections)))))
+
+(deftest basic-test
+ (testing "Check that the basic graph works"
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (is (= {:revision 0
+ :groups {(u/get-id (group/all-users)) {:root :none}
+ (u/get-id (group/metabot)) {:root :none}
+ (u/get-id (group/admin)) {:root :write}}}
+ (graph :clear-revisions? true))))))
+
+(deftest new-collection-perms-test
+ (testing "Creating a new Collection shouldn't give perms to anyone but admins"
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp Collection [collection]
+ (is (= {:revision 0
+ :groups {(u/get-id (group/all-users)) {:root :none, :COLLECTION :none}
+ (u/get-id (group/metabot)) {:root :none, :COLLECTION :none}
+ (u/get-id (group/admin)) {:root :write, :COLLECTION :write}}}
+ (replace-collection-ids collection (graph :clear-revisions? true, :collections [collection]))))))))
+
+(deftest read-perms-test
+ (testing "make sure read perms show up correctly"
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp Collection [collection]
+ (perms/grant-collection-read-permissions! (group/all-users) collection)
+ (is (= {:revision 0
+ :groups {(u/get-id (group/all-users)) {:root :none, :COLLECTION :read}
+ (u/get-id (group/metabot)) {:root :none, :COLLECTION :none}
+ (u/get-id (group/admin)) {:root :write, :COLLECTION :write}}}
+ (replace-collection-ids collection (graph :clear-revisions? true, :collections [collection]))))))))
+
+(deftest grant-write-perms-for-new-collections-test
+ (testing "make sure we can grant write perms for new collections (!)"
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp Collection [collection]
+ (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
+ (is (= {:revision 0
+ :groups {(u/get-id (group/all-users)) {:root :none, :COLLECTION :write}
+ (u/get-id (group/metabot)) {:root :none, :COLLECTION :none}
+ (u/get-id (group/admin)) {:root :write, :COLLECTION :write}}}
+ (replace-collection-ids collection (graph :clear-revisions? true, :collections [collection]))))))))
+
+(deftest non-magical-groups-test
+ (testing "make sure a non-magical group will show up"
+ (mt/with-temp PermissionsGroup [new-group]
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (is (= {:revision 0
+ :groups {(u/get-id (group/all-users)) {:root :none}
+ (u/get-id (group/metabot)) {:root :none}
+ (u/get-id (group/admin)) {:root :write}
+ (u/get-id new-group) {:root :none}}}
+ (graph :clear-revisions? true, :groups [new-group])))))))
+
+(deftest root-collection-read-perms-test
+ (testing "How abut *read* permissions for the Root Collection?"
+ (mt/with-temp PermissionsGroup [new-group]
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (perms/grant-collection-read-permissions! new-group collection/root-collection)
+ (is (= {:revision 0
+ :groups {(u/get-id (group/all-users)) {:root :none}
+ (u/get-id (group/metabot)) {:root :none}
+ (u/get-id (group/admin)) {:root :write}
+ (u/get-id new-group) {:root :read}}}
+ (graph :clear-revisions? true, :groups [new-group])))))))
+
+(deftest root-collection-write-perms-test
+ (testing "How about granting *write* permissions for the Root Collection?"
+ (mt/with-temp PermissionsGroup [new-group]
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (perms/grant-collection-readwrite-permissions! new-group collection/root-collection)
+ (is (= {:revision 0
+ :groups {(u/get-id (group/all-users)) {:root :none}
+ (u/get-id (group/metabot)) {:root :none}
+ (u/get-id (group/admin)) {:root :write}
+ (u/get-id new-group) {:root :write}}}
+ (graph :clear-revisions? true, :groups [new-group])))))))
+
+(deftest no-op-test
+ (testing "Can we do a no-op update?"
+ ;; need to bind *current-user-id* or the Revision won't get updated
+ (clear-graph-revisions!)
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (binding [*current-user-id* (mt/user->id :crowberto)]
+ (graph/update-graph! (graph :clear-revisions? true))
+ (is (= {:revision 0
+ :groups {(u/get-id (group/all-users)) {:root :none}
+ (u/get-id (group/metabot)) {:root :none}
+ (u/get-id (group/admin)) {:root :write}}}
+ (graph))
+ "revision should not have changed, because there was nothing to do...")))))
+
+(deftest grant-perms-test
+ (testing "Can we give someone read perms via the graph?"
+ (clear-graph-revisions!)
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp Collection [collection]
+ (binding [*current-user-id* (mt/user->id :crowberto)]
+ (graph/update-graph! (assoc-in (graph :clear-revisions? true)
+ [:groups (u/get-id (group/all-users)) (u/get-id collection)]
+ :read))
+ (is (= {:revision 1
+ :groups {(u/get-id (group/all-users)) {:root :none, :COLLECTION :read}
+ (u/get-id (group/metabot)) {:root :none, :COLLECTION :none}
+ (u/get-id (group/admin)) {:root :write, :COLLECTION :write}}}
+ (replace-collection-ids collection (graph :collections [collection]))))))))
+
+ (testing "can we give them *write* perms?"
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp Collection [collection]
+ (binding [*current-user-id* (mt/user->id :crowberto)]
+ (graph/update-graph! (assoc-in (graph :clear-revisions? true)
+ [:groups (u/get-id (group/all-users)) (u/get-id collection)]
+ :write))
+ (is (= {:revision 1
+ :groups {(u/get-id (group/all-users)) {:root :none, :COLLECTION :write}
+ (u/get-id (group/metabot)) {:root :none, :COLLECTION :none}
+ (u/get-id (group/admin)) {:root :write, :COLLECTION :write}}}
+ (replace-collection-ids collection (graph :collections [collection])))))))))
+
+(deftest revoke-perms-test
+ (testing "can we *revoke* perms?"
+ (clear-graph-revisions!)
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp Collection [collection]
+ (binding [*current-user-id* (mt/user->id :crowberto)]
+ (perms/grant-collection-read-permissions! (group/all-users) collection)
+ (graph/update-graph! (assoc-in (graph :clear-revisions? true)
+ [:groups (u/get-id (group/all-users)) (u/get-id collection)]
+ :none))
+ (is (= {:revision 1
+ :groups {(u/get-id (group/all-users)) {:root :none, :COLLECTION :none}
+ (u/get-id (group/metabot)) {:root :none, :COLLECTION :none}
+ (u/get-id (group/admin)) {:root :write, :COLLECTION :write}}}
+ (replace-collection-ids collection (graph :collections [collection])))))))))
+
+(deftest grant-root-permissions-test
+ (testing "Can we grant *read* permissions for the Root Collection?"
+ (mt/with-temp PermissionsGroup [new-group]
+ (clear-graph-revisions!)
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (binding [*current-user-id* (mt/user->id :crowberto)]
+ (graph/update-graph! (assoc-in (graph :clear-revisions? true)
+ [:groups (u/get-id new-group) :root]
+ :read))
+ (is (= {:revision 1
+ :groups {(u/get-id (group/all-users)) {:root :none}
+ (u/get-id (group/metabot)) {:root :none}
+ (u/get-id (group/admin)) {:root :write}
+ (u/get-id new-group) {:root :read}}}
+ (graph :groups [new-group])))))))
+
+ (testing "How about granting *write* permissions for the Root Collection?"
+ (mt/with-temp PermissionsGroup [new-group]
+ (clear-graph-revisions!)
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (binding [*current-user-id* (mt/user->id :crowberto)]
+ (graph/update-graph! (assoc-in (graph :clear-revisions? true)
+ [:groups (u/get-id new-group) :root]
+ :write))
+ (is (= {:revision 1
+ :groups {(u/get-id (group/all-users)) {:root :none}
+ (u/get-id (group/metabot)) {:root :none}
+ (u/get-id (group/admin)) {:root :write}
+ (u/get-id new-group) {:root :write}}}
+ (graph :groups [new-group]))))))))
+
+(deftest revoke-root-permissions-test
+ (testing "can we *revoke* RootCollection perms?"
+ (mt/with-temp PermissionsGroup [new-group]
+ (clear-graph-revisions!)
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (binding [*current-user-id* (mt/user->id :crowberto)]
+ (perms/grant-collection-readwrite-permissions! new-group collection/root-collection)
+ (graph/update-graph! (assoc-in (graph :clear-revisions? true)
+ [:groups (u/get-id new-group) :root]
+ :none))
+ (is (= {:revision 1
+ :groups {(u/get-id (group/all-users)) {:root :none}
+ (u/get-id (group/metabot)) {:root :none}
+ (u/get-id (group/admin)) {:root :write}
+ (u/get-id new-group) {:root :none}}}
+ (graph :groups [new-group]))))))))
+
+(deftest personal-collections-should-not-appear-test
+ (testing "Make sure that personal Collections *do not* appear in the Collections graph"
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (is (= {:revision 0
+ :groups {(u/get-id (group/all-users)) {:root :none}
+ (u/get-id (group/metabot)) {:root :none}
+ (u/get-id (group/admin)) {:root :write}}}
+ (graph :clear-revisions? true)))))
+
+ (testing "Make sure descendants of Personal Collections do not come back as part of the graph either..."
+ (clear-graph-revisions!)
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp Collection [_ {:location (lucky-collection-children-location)}]
+ (is (= {:revision 0
+ :groups {(u/get-id (group/all-users)) {:root :none}
+ (u/get-id (group/metabot)) {:root :none}
+ (u/get-id (group/admin)) {:root :write}}}
+ (graph)))))))
+
+(deftest disallow-editing-personal-collections-test
+ (testing "Make sure that if we try to be sneaky and edit a Personal Collection via the graph, changes are ignored"
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (let [lucky-personal-collection-id (u/get-id (collection/user->personal-collection (mt/user->id :lucky)))
+ path [:groups (u/get-id (group/all-users)) lucky-personal-collection-id]]
+ (mt/throw-if-called graph/update-group-permissions!
+ (graph/update-graph! (assoc-in (graph :clear-revisions? true) path :read)))
+
+ (testing "double-check that the graph is unchanged"
+ (is (= {:revision 0
+ :groups {(u/get-id (group/all-users)) {:root :none}
+ (u/get-id (group/metabot)) {:root :none}
+ (u/get-id (group/admin)) {:root :write}}}
+ (graph))))
+
+ (testing "No revision should have been saved"
+ (is (= 0
+ (collection-revision/latest-id)))))))
+
+ (testing "Make sure you can't be sneaky and edit descendants of Personal Collections either."
+ (mt/with-temp Collection [collection {:location (lucky-collection-children-location)}]
+ (let [lucky-personal-collection-id (u/get-id (collection/user->personal-collection (mt/user->id :lucky)))]
+ (is (thrown?
+ Exception
+ (graph/update-graph! (assoc-in (graph :clear-revisions? true)
+ [:groups
+ (u/get-id (group/all-users))
+ lucky-personal-collection-id
+ (u/get-id collection)]
+ :read))))))))
+
+(deftest collection-namespace-test
+ (testing "The permissions graph should be namespace-aware.\n"
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp* [Collection [{default-a :id} {:location "/"}]
+ Collection [{default-ab :id} {:location (format "/%d/" default-a)}]
+ Collection [{currency-a :id} {:namespace "currency", :location "/"}]
+ Collection [{currency-ab :id} {:namespace "currency", :location (format "/%d/" currency-a)}]
+ PermissionsGroup [{group-id :id}]]
+ (letfn [(nice-graph [graph]
+ (let [id->alias {default-a "Default A"
+ default-ab "Default A -> B"
+ currency-a "Currency A"
+ currency-ab "Currency A -> B"}]
+ (transduce
+ identity
+ (fn
+ ([graph]
+ (-> (get-in graph [:groups group-id])
+ (select-keys (vals id->alias))))
+ ([graph [collection-id k]]
+ (replace-collection-ids collection-id graph k)))
+ graph
+ id->alias)))]
+ (doseq [collection [default-a default-ab currency-a currency-ab]]
+ (perms/grant-collection-read-permissions! group-id collection))
+ (testing "Calling (graph) with no args should only show Collections in the default namespace"
+ (is (= {"Default A" :read, "Default A -> B" :read}
+ (nice-graph (graph/graph))
+ (nice-graph (graph/graph nil)))))
+
+ (testing "You should be able to pass an different namespace to (graph) to see Collections in that namespace"
+ (is (= {"Currency A" :read, "Currency A -> B" :read}
+ (nice-graph (graph/graph :currency)))))
+
+ (testing "Should be able to pass `::graph/all` to get a combined graph of all namespaces (used for saving revisions)"
+ (is (= {"Default A" :read, "Default A -> B" :read "Currency A" :read, "Currency A -> B" :read}
+ (nice-graph (graph/graph ::graph/all)))))
+
+ ;; bind a current user so CollectionRevisions get saved.
+ (mt/with-test-user :crowberto
+ (testing "Should be able to update the graph for the default namespace.\n"
+ (let [before (graph/graph ::graph/all)]
+ (graph/update-graph! (assoc (graph/graph) :groups {group-id {default-ab :write, currency-ab :write}}))
+ (is (= {"Default A" :read, "Default A -> B" :write}
+ (nice-graph (graph/graph))))
+
+ (testing "Updates to Collections in other namespaces should be ignored"
+ (is (= {"Currency A" :read, "Currency A -> B" :read}
+ (nice-graph (graph/graph :currency)))))
+
+ (testing "A CollectionRevision recording the *changes* to the perms graph should be saved."
+ (is (schema= {:id su/IntGreaterThanZero
+ :before (s/eq (mt/obj->json->obj before))
+ :after (s/eq {(keyword (str group-id)) {(keyword (str default-ab)) "write"}})
+ :user_id (s/eq (mt/user->id :crowberto))
+ :created_at java.time.temporal.Temporal
+ s/Keyword s/Any}
+ (db/select-one CollectionRevision {:order-by [[:id :desc]]}))))))
+
+ (testing "Should be able to update the graph for a non-default namespace.\n"
+ (let [before (graph/graph ::graph/all)]
+ (graph/update-graph! :currency (assoc (graph/graph) :groups {group-id {default-a :write, currency-a :write}}))
+ (is (= {"Currency A" :write, "Currency A -> B" :read}
+ (nice-graph (graph/graph :currency))))
+
+ (testing "Updates to Collections in other namespaces should be ignored"
+ (is (= {"Default A" :read, "Default A -> B" :write}
+ (nice-graph (graph/graph)))))
+
+ (testing "A CollectionRevision recording the *changes* to the perms graph should be saved."
+ (is (schema= {:id su/IntGreaterThanZero
+ :before (s/eq (mt/obj->json->obj before))
+ :after (s/eq {(keyword (str group-id)) {(keyword (str currency-a)) "write"}})
+ :user_id (s/eq (mt/user->id :crowberto))
+ :created_at java.time.temporal.Temporal
+ s/Keyword s/Any}
+ (db/select-one CollectionRevision {:order-by [[:id :desc]]}))))))))))))
diff --git a/test/metabase/models/collection_test.clj b/test/metabase/models/collection_test.clj
index dc3a140bc5e876d95493328a59b0f3bebc42010b..6e356b7f19267afbf315dd0bbb766bc65fc0a188 100644
--- a/test/metabase/models/collection_test.clj
+++ b/test/metabase/models/collection_test.clj
@@ -3,400 +3,125 @@
(:require [clojure
[string :as str]
[test :refer :all]]
- [expectations :refer :all]
- [medley.core :as m]
- [metabase.api.common :refer [*current-user-id* *current-user-permissions-set*]]
+ [expectations :refer [expect]]
+ [metabase
+ [models :refer [Card Collection Dashboard NativeQuerySnippet Permissions PermissionsGroup Pulse User]]
+ [test :as mt]
+ [util :as u]]
+ [metabase.api.common :refer [*current-user-permissions-set*]]
[metabase.models
- [card :refer [Card]]
- [collection :as collection :refer [Collection]]
- [collection-revision :refer [CollectionRevision]]
- [dashboard :refer [Dashboard]]
- [permissions :as perms :refer [Permissions]]
- [permissions-group :as group :refer [PermissionsGroup]]
- [pulse :refer [Pulse]]
- [user :refer [User]]]
- [metabase.test
- [fixtures :as fixtures]
- [util :as tu]]
- [metabase.test.data.users :as test-users]
- [metabase.util :as u]
+ [collection :as collection]
+ [permissions :as perms]]
+ [metabase.test.fixtures :as fixtures]
+ [metabase.util.schema :as su]
+ [schema.core :as s]
[toucan
[db :as db]
- [hydrate :refer [hydrate]]]
- [toucan.util.test :as tt]))
+ [hydrate :refer [hydrate]]]))
(use-fixtures :once (fixtures/initialize :db :test-users :test-users-personal-collections))
(defn- lucky-collection-children-location []
- (collection/children-location (collection/user->personal-collection (test-users/user->id :lucky))))
-
-(defn- replace-collection-ids
- "In Collection perms `graph`, replace instances of the ID of `collection-or-id` with `:COLLECTION`, making it possible
- to write tests that don't need to know its actual numeric ID."
- [collection-or-id graph]
- (update graph :groups (partial m/map-vals (partial m/map-keys (fn [collection-id]
- (if (= collection-id (u/get-id collection-or-id))
- :COLLECTION
- collection-id))))))
-
-;; test that we can create a new Collection with valid inputs
-(expect
- {:name "My Favorite Cards"
- :slug "my_favorite_cards"
- :description nil
- :color "#ABCDEF"
- :archived false
- :location "/"
- :personal_owner_id nil}
- (tt/with-temp Collection [collection {:name "My Favorite Cards", :color "#ABCDEF"}]
- (dissoc collection :id)))
+ (collection/children-location (collection/user->personal-collection (mt/user->id :lucky))))
+
+(deftest create-collection-test
+ (testing "test that we can create a new Collection with valid inputs"
+ (mt/with-temp Collection [collection {:name "My Favorite Cards", :color "#ABCDEF"}]
+ (is (= (merge
+ (mt/object-defaults Collection)
+ {:name "My Favorite Cards"
+ :slug "my_favorite_cards"
+ :description nil
+ :color "#ABCDEF"
+ :archived false
+ :location "/"
+ :personal_owner_id nil})
+ (mt/derecordize (dissoc collection :id)))))))
(deftest color-validation-test
(testing "Collection colors should be validated when inserted into the DB"
- (are [test-msg input] (is (thrown? Exception
- (db/insert! Collection {:name "My Favorite Cards", :color input}))
- test-msg)
- "Missing color" nil
- "Too short" #"ABC"
- "Invalid chars" "#BCDEFG"
- "Too long" #"ABCDEFF"
- "Missing hash prefix" "ABCDEF")))
-
-;; double-check that `with-temp-defaults` are working correctly for Collection
-(expect
- :ok
- (tt/with-temp* [Collection [_]]
- :ok))
-
-;; test that duplicate names ARE allowed
-(expect
- :ok
- (tt/with-temp* [Collection [_ {:name "My Favorite Cards"}]
- Collection [_ {:name "My Favorite Cards"}]]
- :ok))
-
-;; Duplicate names should result in duplicate slugs...
-(expect
- ["my_favorite_cards"
- "my_favorite_cards"]
- (tt/with-temp* [Collection [collection-1 {:name "My Favorite Cards"}]
- Collection [collection-2 {:name "My Favorite Cards"}]]
- (map :slug [collection-1 collection-2])))
-
-
-;; things with different names that would cause the same slug SHOULD be allowed
-(expect
- :ok
- (tt/with-temp* [Collection [_ {:name "My Favorite Cards"}]
- Collection [_ {:name "my_favorite Cards"}]]
- :ok))
-
-;; check that archiving a collection archives its cards as well
-(expect
- true
- (tt/with-temp* [Collection [collection]
- Card [card {:collection_id (u/get-id collection)}]]
- (db/update! Collection (u/get-id collection)
- :archived true)
- (db/select-one-field :archived Card :id (u/get-id card))))
-
-;; check that unarchiving a collection unarchives its cards as well
-(expect
- false
- (tt/with-temp* [Collection [collection {:archived true}]
- Card [card {:collection_id (u/get-id collection), :archived true}]]
- (db/update! Collection (u/get-id collection)
- :archived false)
- (db/select-one-field :archived Card :id (u/get-id card))))
-
-;; check that collections' names cannot be blank
-(expect
- Exception
- (tt/with-temp Collection [collection {:name ""}]
- collection))
-
-;; check we can't change the name of a Collection to a blank string
-(expect
- Exception
- (tt/with-temp Collection [collection]
- (db/update! Collection (u/get-id collection)
- :name "")))
-
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | Graph Tests |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
-(defn- clear-graph-revisions! []
- (db/delete! CollectionRevision))
-
-(defn- graph
- "Fetch collection graph. `:clear-revisions?` = delete any previously existing collection revision entries so we get
- revision = 0."
- [& {:keys [clear-revisions?]}]
- (when clear-revisions?
- (clear-graph-revisions!))
- ;; force lazy creation of the three magic groups as needed
- (group/all-users)
- (group/admin)
- (group/metabot)
- ;; now fetch the graph
- (collection/graph))
-
-;; Check that the basic graph works
-(expect
- {:revision 0
- :groups {(u/get-id (group/all-users)) {:root :none}
- (u/get-id (group/metabot)) {:root :none}
- (u/get-id (group/admin)) {:root :write}}}
- (tu/with-non-admin-groups-no-root-collection-perms
- (graph :clear-revisions? true)))
-
-;; Creating a new Collection shouldn't give perms to anyone but admins
-(expect
- {:revision 0
- :groups {(u/get-id (group/all-users)) {:root :none, :COLLECTION :none}
- (u/get-id (group/metabot)) {:root :none, :COLLECTION :none}
- (u/get-id (group/admin)) {:root :write, :COLLECTION :write}}}
- (tu/with-non-admin-groups-no-root-collection-perms
- (tt/with-temp Collection [collection]
- (replace-collection-ids collection (graph :clear-revisions? true)))))
-
-;; make sure read perms show up correctly
-(expect
- {:revision 0
- :groups {(u/get-id (group/all-users)) {:root :none, :COLLECTION :read}
- (u/get-id (group/metabot)) {:root :none, :COLLECTION :none}
- (u/get-id (group/admin)) {:root :write, :COLLECTION :write}}}
- (tu/with-non-admin-groups-no-root-collection-perms
- (tt/with-temp Collection [collection]
- (perms/grant-collection-read-permissions! (group/all-users) collection)
- (replace-collection-ids collection (graph :clear-revisions? true)))))
-
-;; make sure we can grant write perms for new collections (!)
-(expect
- {:revision 0
- :groups {(u/get-id (group/all-users)) {:root :none, :COLLECTION :write}
- (u/get-id (group/metabot)) {:root :none, :COLLECTION :none}
- (u/get-id (group/admin)) {:root :write, :COLLECTION :write}}}
- (tu/with-non-admin-groups-no-root-collection-perms
- (tt/with-temp Collection [collection]
- (perms/grant-collection-readwrite-permissions! (group/all-users) collection)
- (replace-collection-ids collection (graph :clear-revisions? true)))))
-
-;; make sure a non-magical group will show up
-(tt/expect-with-temp [PermissionsGroup [new-group]]
- {:revision 0
- :groups {(u/get-id (group/all-users)) {:root :none}
- (u/get-id (group/metabot)) {:root :none}
- (u/get-id (group/admin)) {:root :write}
- (u/get-id new-group) {:root :none}}}
- (tu/with-non-admin-groups-no-root-collection-perms
- (graph :clear-revisions? true)))
-
-;; How abut *read* permissions for the Root Collection?
-(tt/expect-with-temp [PermissionsGroup [new-group]]
- {:revision 0
- :groups {(u/get-id (group/all-users)) {:root :none}
- (u/get-id (group/metabot)) {:root :none}
- (u/get-id (group/admin)) {:root :write}
- (u/get-id new-group) {:root :read}}}
- (tu/with-non-admin-groups-no-root-collection-perms
- (perms/grant-collection-read-permissions! new-group collection/root-collection)
- (graph :clear-revisions? true)))
-
-;; How about granting *write* permissions for the Root Collection?
-(tt/expect-with-temp [PermissionsGroup [new-group]]
- {:revision 0
- :groups {(u/get-id (group/all-users)) {:root :none}
- (u/get-id (group/metabot)) {:root :none}
- (u/get-id (group/admin)) {:root :write}
- (u/get-id new-group) {:root :write}}}
- (tu/with-non-admin-groups-no-root-collection-perms
- (perms/grant-collection-readwrite-permissions! new-group collection/root-collection)
- (graph :clear-revisions? true)))
-
-;; Can we do a no-op update?
-(expect
- ;; revision should not have changed, because there was nothing to do...
- {:revision 0
- :groups {(u/get-id (group/all-users)) {:root :none}
- (u/get-id (group/metabot)) {:root :none}
- (u/get-id (group/admin)) {:root :write}}}
- ;; need to bind *current-user-id* or the Revision won't get updated
- (do
- (clear-graph-revisions!)
- (tu/with-non-admin-groups-no-root-collection-perms
- (binding [*current-user-id* (test-users/user->id :crowberto)]
- (collection/update-graph! (graph :clear-revisions? true))
- (graph)))))
-
-;; Can we give someone read perms via the graph?
-(expect
- {:revision 1
- :groups {(u/get-id (group/all-users)) {:root :none, :COLLECTION :read}
- (u/get-id (group/metabot)) {:root :none, :COLLECTION :none}
- (u/get-id (group/admin)) {:root :write, :COLLECTION :write}}}
- (do
- (clear-graph-revisions!)
- (tu/with-non-admin-groups-no-root-collection-perms
- (tt/with-temp Collection [collection]
- (binding [*current-user-id* (test-users/user->id :crowberto)]
- (collection/update-graph! (assoc-in (graph :clear-revisions? true)
- [:groups (u/get-id (group/all-users)) (u/get-id collection)]
- :read))
- (replace-collection-ids collection (graph)))))))
-
-;; can we give them *write* perms?
-(expect
- {:revision 1
- :groups {(u/get-id (group/all-users)) {:root :none, :COLLECTION :write}
- (u/get-id (group/metabot)) {:root :none, :COLLECTION :none}
- (u/get-id (group/admin)) {:root :write, :COLLECTION :write}}}
- (tu/with-non-admin-groups-no-root-collection-perms
- (tt/with-temp Collection [collection]
- (binding [*current-user-id* (test-users/user->id :crowberto)]
- (collection/update-graph! (assoc-in (graph :clear-revisions? true)
- [:groups (u/get-id (group/all-users)) (u/get-id collection)]
- :write))
- (replace-collection-ids collection (graph))))))
-
-;; can we *revoke* perms?
-(expect
- {:revision 1
- :groups {(u/get-id (group/all-users)) {:root :none, :COLLECTION :none}
- (u/get-id (group/metabot)) {:root :none, :COLLECTION :none}
- (u/get-id (group/admin)) {:root :write, :COLLECTION :write}}}
- (do
- (clear-graph-revisions!)
- (tu/with-non-admin-groups-no-root-collection-perms
- (tt/with-temp Collection [collection]
- (binding [*current-user-id* (test-users/user->id :crowberto)]
- (perms/grant-collection-read-permissions! (group/all-users) collection)
- (collection/update-graph! (assoc-in (graph :clear-revisions? true)
- [:groups (u/get-id (group/all-users)) (u/get-id collection)]
- :none))
- (replace-collection-ids collection (graph)))))))
-
-;; How abut *read* permissions for the Root Collection?
-(tt/expect-with-temp [PermissionsGroup [new-group]]
- {:revision 1
- :groups {(u/get-id (group/all-users)) {:root :none}
- (u/get-id (group/metabot)) {:root :none}
- (u/get-id (group/admin)) {:root :write}
- (u/get-id new-group) {:root :read}}}
- (do
- (clear-graph-revisions!)
- (tu/with-non-admin-groups-no-root-collection-perms
- (binding [*current-user-id* (test-users/user->id :crowberto)]
- (collection/update-graph! (assoc-in (graph :clear-revisions? true)
- [:groups (u/get-id new-group) :root]
- :read))
- (graph)))))
-
-;; How about granting *write* permissions for the Root Collection?
-(tt/expect-with-temp [PermissionsGroup [new-group]]
- {:revision 1
- :groups {(u/get-id (group/all-users)) {:root :none}
- (u/get-id (group/metabot)) {:root :none}
- (u/get-id (group/admin)) {:root :write}
- (u/get-id new-group) {:root :write}}}
- (do
- (clear-graph-revisions!)
- (tu/with-non-admin-groups-no-root-collection-perms
- (binding [*current-user-id* (test-users/user->id :crowberto)]
- (collection/update-graph! (assoc-in (graph :clear-revisions? true)
- [:groups (u/get-id new-group) :root]
- :write))
- (graph)))))
-
-;; can we *revoke* RootCollection perms?
-(tt/expect-with-temp [PermissionsGroup [new-group]]
- {:revision 1
- :groups {(u/get-id (group/all-users)) {:root :none}
- (u/get-id (group/metabot)) {:root :none}
- (u/get-id (group/admin)) {:root :write}
- (u/get-id new-group) {:root :none}}}
- (do
- (clear-graph-revisions!)
- (tu/with-non-admin-groups-no-root-collection-perms
- (binding [*current-user-id* (test-users/user->id :crowberto)]
- (perms/grant-collection-readwrite-permissions! new-group collection/root-collection)
- (collection/update-graph! (assoc-in (graph :clear-revisions? true)
- [:groups (u/get-id new-group) :root]
- :none))
- (graph)))))
-
-;; Make sure that personal Collections *do not* appear in the Collections graph
-(expect
- {:revision 0
- :groups {(u/get-id (group/all-users)) {:root :none}
- (u/get-id (group/metabot)) {:root :none}
- (u/get-id (group/admin)) {:root :write}}}
- (tu/with-non-admin-groups-no-root-collection-perms
- (graph :clear-revisions? true)))
-
-;; Make sure that if we try to be sneaky and edit a Personal Collection via the graph, an Exception is thrown
-(expect
- Exception
- (let [lucky-personal-collection-id (u/get-id (collection/user->personal-collection (test-users/user->id :lucky)))]
- (collection/update-graph! (assoc-in (graph :clear-revisions? true)
- [:groups (u/get-id (group/all-users)) lucky-personal-collection-id]
- :read))))
-
-;; double-check that the graph is unchanged
-(expect
- {:revision 0
- :groups {(u/get-id (group/all-users)) {:root :none}
- (u/get-id (group/metabot)) {:root :none}
- (u/get-id (group/admin)) {:root :write}}}
- (do
- (clear-graph-revisions!)
- (tu/with-non-admin-groups-no-root-collection-perms
- (u/ignore-exceptions
- (let [lucky-personal-collection-id (u/get-id (collection/user->personal-collection (test-users/user->id :lucky)))]
- (collection/update-graph! (assoc-in (graph :clear-revisions? true)
- [:groups (u/get-id (group/all-users)) lucky-personal-collection-id]
- :read))))
- (graph))))
-
-;; Make sure descendants of Personal Collections do not come back as part of the graph either...
-(expect
- {:revision 0
- :groups {(u/get-id (group/all-users)) {:root :none}
- (u/get-id (group/metabot)) {:root :none}
- (u/get-id (group/admin)) {:root :write}}}
- (do
- (clear-graph-revisions!)
- (tu/with-non-admin-groups-no-root-collection-perms
- (tt/with-temp Collection [_ {:location (lucky-collection-children-location)}]
- (graph)))))
-
-;; ...and that you can't be sneaky and try to edit them either...
-(expect
- Exception
- (tt/with-temp Collection [collection {:location (lucky-collection-children-location)}]
- (let [lucky-personal-collection-id (u/get-id (collection/user->personal-collection (test-users/user->id :lucky)))]
- (collection/update-graph! (assoc-in (graph :clear-revisions? true)
- [:groups
- (u/get-id (group/all-users))
- lucky-personal-collection-id
- (u/get-id collection)]
- :read)))))
+ (doseq [[input msg] {nil "Missing color"
+ "#ABC" "Too short"
+ "#BCDEFG" "Invalid chars"
+ "#ABCDEFF" "Too long"
+ "ABCDEF" "Missing hash prefix"}]
+ (testing msg
+ (is (thrown?
+ Exception
+ (db/insert! Collection {:name "My Favorite Cards", :color input})))))))
+
+(deftest with-temp-defaults-test
+ (testing "double-check that `with-temp-defaults` are working correctly for Collection"
+ (mt/with-temp Collection [collection]
+ (is (some? collection)))))
+
+(deftest duplicate-names-test
+ (testing "test that duplicate names ARE allowed"
+ (mt/with-temp* [Collection [c1 {:name "My Favorite Cards"}]
+ Collection [c2 {:name "My Favorite Cards"}]]
+ (is (some? c1))
+ (is (some? c2))
+
+ (testing "Duplicate names should result in duplicate slugs..."
+ (testing "Collection 1"
+ (is (= "my_favorite_cards"
+ (:slug c1))))
+ (testing "Collection 2"
+ (is (= "my_favorite_cards"
+ (:slug c2)))))))
+
+ (testing "things with different names that would cause the same slug SHOULD be allowed"
+ (mt/with-temp* [Collection [c1 {:name "My Favorite Cards"}]
+ Collection [c2 {:name "my_favorite Cards"}]]
+ (is (some? c1))
+ (is (some? c2))
+ (is (= (:slug c1) (:slug c2))))))
+
+(deftest archive-cards-test
+ (testing "check that archiving a Collection archives its Cards as well"
+ (mt/with-temp* [Collection [collection]
+ Card [card {:collection_id (u/get-id collection)}]]
+ (db/update! Collection (u/get-id collection)
+ :archived true)
+ (is (= true
+ (db/select-one-field :archived Card :id (u/get-id card))))))
+
+ (testing "check that unarchiving a Collection unarchives its Cards as well"
+ (mt/with-temp* [Collection [collection {:archived true}]
+ Card [card {:collection_id (u/get-id collection), :archived true}]]
+ (db/update! Collection (u/get-id collection)
+ :archived false)
+ (is (= false
+ (db/select-one-field :archived Card :id (u/get-id card)))))))
+
+(deftest validate-name-test
+ (testing "check that collections' names cannot be blank"
+ (is (thrown?
+ Exception
+ (mt/with-temp Collection [collection {:name ""}]
+ collection))))
+
+ (testing "check we can't change the name of a Collection to a blank string"
+ (mt/with-temp Collection [collection]
+ (is (thrown?
+ Exception
+ (db/update! Collection (u/get-id collection)
+ :name ""))))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Nested Collections Helper Fns & Macros |
;;; +----------------------------------------------------------------------------------------------------------------+
-(defn do-with-collection-hierarchy [a-fn]
- (tu/with-non-admin-groups-no-root-collection-perms
- (tt/with-temp* [Collection [a {:name "A"}]
- Collection [b {:name "B", :location (collection/location-path a)}]
- Collection [c {:name "C", :location (collection/location-path a)}]
- Collection [d {:name "D", :location (collection/location-path a c)}]
- Collection [e {:name "E", :location (collection/location-path a c d)}]
- Collection [f {:name "F", :location (collection/location-path a c)}]
- Collection [g {:name "G", :location (collection/location-path a c f)}]]
+(defn do-with-collection-hierarchy [options a-fn]
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (mt/with-temp* [Collection [a (merge options {:name "A"})]
+ Collection [b (merge options {:name "B", :location (collection/location-path a)})]
+ Collection [c (merge options {:name "C", :location (collection/location-path a)})]
+ Collection [d (merge options {:name "D", :location (collection/location-path a c)})]
+ Collection [e (merge options {:name "E", :location (collection/location-path a c d)})]
+ Collection [f (merge options {:name "F", :location (collection/location-path a c)})]
+ Collection [g (merge options {:name "G", :location (collection/location-path a c f)})]]
(a-fn {:a a, :b b, :c c, :d d, :e e, :f f, :g g}))))
(defmacro with-collection-hierarchy
@@ -413,8 +138,8 @@
(with-collection-hierarchy [{:keys [a b c]}]
...)"
{:style/indent 1}
- [[collections-binding] & body]
- `(do-with-collection-hierarchy (fn [~collections-binding] ~@body)))
+ [[collections-binding options] & body]
+ `(do-with-collection-hierarchy ~options (fn [~collections-binding] ~@body)))
(defmacro with-current-user-perms-for-collections
"Run `body` with the current User permissions for `collections-or-ids`.
@@ -453,120 +178,153 @@
(deftest location-path-test
(testing "Does our handy utility function for working with `location` paths work as expected?"
(testing "valid input"
- (are [expected args] (is (= expected
- (apply collection/location-path args)))
- "/1/2/3/" [1 2 3]
- "/" nil
- "/1/" [{:id 1}]
- "/1/2/3/" [{:id 1} {:id 2} {:id 3}]
- "/1/337/" [1 {:id 337}]))
- (testing "invalid input"
- (are [args] (thrown? Exception (apply collection/location-path args))
- ["1"]
- [nil]
- [-1]
- ;; shouldn't allow duplicates
- [1 2 1]))))
-
-(expect [1 2 3] (collection/location-path->ids "/1/2/3/"))
-(expect [] (collection/location-path->ids "/"))
-(expect [1] (collection/location-path->ids "/1/"))
-(expect [1 337] (collection/location-path->ids "/1/337/"))
-(expect Exception (collection/location-path->ids "/a/"))
-(expect Exception (collection/location-path->ids nil))
-(expect Exception (collection/location-path->ids "/-1/"))
-(expect Exception (collection/location-path->ids "/1/2/1/"))
-
-(expect 3 (collection/location-path->parent-id "/1/2/3/"))
-(expect nil (collection/location-path->parent-id "/"))
-(expect 1 (collection/location-path->parent-id "/1/"))
-(expect 337 (collection/location-path->parent-id "/1/337/"))
-(expect Exception (collection/location-path->parent-id "/a/"))
-(expect Exception (collection/location-path->parent-id nil))
-(expect Exception (collection/location-path->parent-id "/-1/"))
-(expect Exception (collection/location-path->parent-id "/1/2/1/"))
-
-(expect "/1/2/3/1000/" (collection/children-location {:id 1000, :location "/1/2/3/"}))
-(expect "/1000/" (collection/children-location {:id 1000, :location "/"}))
-(expect "/1/1000/" (collection/children-location {:id 1000, :location "/1/"}))
-(expect "/1/337/1000/" (collection/children-location {:id 1000, :location "/1/337/"}))
-(expect Exception (collection/children-location {:id 1000, :location "/a/"}))
-(expect Exception (collection/children-location {:id 1000, :location nil}))
-(expect Exception (collection/children-location {:id 1000, :location "/-1/"}))
-(expect Exception (collection/children-location {:id nil, :location "/1/"}))
-(expect Exception (collection/children-location {:id "a", :location "/1/"}))
-(expect Exception (collection/children-location {:id 1, :location "/1/2/"}))
-
-;; Make sure we can look at the current user's permissions set and figure out which Collections they're allowed to see
-(expect
- #{8 9}
- (collection/permissions-set->visible-collection-ids
- #{"/db/1/"
- "/db/2/native/"
- "/db/4/schema/"
- "/db/5/schema/PUBLIC/"
- "/db/6/schema/PUBLIC/table/7/"
- "/collection/8/"
- "/collection/9/read/"}))
-
-;; If the current user has root permissions then make sure the function returns `:all`, which signifies that they are
-;; able to see all Collections
-(expect
- :all
- (collection/permissions-set->visible-collection-ids
- #{"/"
- "/db/2/native/"
- "/collection/9/read/"}))
-
-;; for the Root Collection we should return `"root"`
-(expect
- #{8 9 "root"}
- (collection/permissions-set->visible-collection-ids
- #{"/collection/8/"
- "/collection/9/read/"
- "/collection/root/"}))
-
-(expect
- #{"root"}
- (collection/permissions-set->visible-collection-ids
- #{"/collection/root/read/"}))
-
-;; Can we calculate `effective-location-path`?
-(expect "/10/20/" (collection/effective-location-path "/10/20/30/" #{10 20}))
-(expect "/10/30/" (collection/effective-location-path "/10/20/30/" #{10 30}))
-(expect "/" (collection/effective-location-path "/10/20/30/" #{}))
-(expect "/10/20/30/" (collection/effective-location-path "/10/20/30/" #{10 20 30}))
-(expect "/10/20/30/" (collection/effective-location-path "/10/20/30/" :all))
-(expect Exception (collection/effective-location-path "/10/20/30/" nil))
-(expect Exception (collection/effective-location-path "/10/20/30/" [20]))
-(expect Exception (collection/effective-location-path nil #{}))
-(expect Exception (collection/effective-location-path [10 20] #{}))
-
-;; Does the function also work if we call the single-arity version that powers hydration?
-(expect
- "/10/20/"
- (binding [*current-user-permissions-set* (atom #{"/collection/10/" "/collection/20/read/"})]
- (collection/effective-location-path {:location "/10/20/30/"})))
-
-(expect
- "/10/30/"
- (binding [*current-user-permissions-set* (atom #{"/collection/10/read/" "/collection/30/read/"})]
- (collection/effective-location-path {:location "/10/20/30/"})))
-
-(expect
- "/"
- (binding [*current-user-permissions-set* (atom #{})]
- (collection/effective-location-path {:location "/10/20/30/"})))
-
-(expect
- "/10/20/30/"
- (binding [*current-user-permissions-set* (atom #{"/collection/10/" "/collection/20/read/" "/collection/30/read/"})]
- (collection/effective-location-path {:location "/10/20/30/"})))
+ (doseq [[args expected] {[1 2 3] "/1/2/3/"
+ nil "/"
+ [{:id 1}] "/1/"
+ [{:id 1} {:id 2} {:id 3}] "/1/2/3/"
+ [1 {:id 337}] "/1/337/"}]
+ (testing (pr-str (cons 'location-path args))
+ (is (= expected
+ (apply collection/location-path args))))))
-(expect
- "/10/20/30/"
- (binding [*current-user-permissions-set* (atom #{"/"})]
- (collection/effective-location-path {:location "/10/20/30/"})))
+ (testing "invalid input"
+ (doseq [args [["1"]
+ [nil]
+ [-1]
+ ;; shouldn't allow duplicates
+ [1 2 1]]]
+ (testing (pr-str (cons 'location-path args))
+ (is (thrown?
+ Exception
+ (apply collection/location-path args))))))))
+
+(deftest location-path-ids-test
+ (testing "valid input"
+ (doseq [[path expected] {"/1/2/3/" [1 2 3]
+ "/" []
+ "/1/" [1]
+ "/1/337/" [1 337]}]
+ (testing (pr-str (list 'location-path->ids path))
+ (is (= expected
+ (collection/location-path->ids path))))
+
+ (testing (pr-str (list 'location-path->parent-id path))
+ (is (= (last expected)
+ (collection/location-path->parent-id path))))))
+
+ (testing "invalid input"
+ (doseq [path ["/a/"
+ nil
+ "/-1/"
+ "/1/2/1/"]]
+ (testing (pr-str (list 'location-path->ids path))
+ (is (thrown?
+ Exception
+ (collection/location-path->ids path))))
+
+ (testing (pr-str (list 'location-path->parent-id path))
+ (is (thrown?
+ Exception
+ (collection/location-path->parent-id path)))))))
+
+(deftest children-location-test
+ (testing "valid input"
+ (doseq [[collection expected] {{:id 1000, :location "/1/2/3/"} "/1/2/3/1000/"
+ {:id 1000, :location "/"} "/1000/"
+ {:id 1000, :location "/1/"} "/1/1000/"
+ {:id 1000, :location "/1/337/"} "/1/337/1000/"}]
+ (testing (pr-str (list 'children-location collection))
+ (is (= expected
+ (collection/children-location collection))))))
+
+ (testing "invalid input"
+ (doseq [collection [{:id 1000, :location "/a/"}
+ {:id 1000, :location nil}
+ {:id 1000, :location "/-1/"}
+ {:id nil, :location "/1/"}
+ {:id "a", :location "/1/"}
+ {:id 1, :location "/1/2/"}]]
+ (testing (pr-str (list 'children-location collection))
+ (is (thrown?
+ Exception
+ (collection/children-location collection)))))))
+
+(deftest permissions-set->visible-collection-ids-test
+ (testing "Make sure we can look at the current user's permissions set and figure out which Collections they're allowed to see"
+ (is (= #{8 9}
+ (collection/permissions-set->visible-collection-ids
+ #{"/db/1/"
+ "/db/2/native/"
+ "/db/4/schema/"
+ "/db/5/schema/PUBLIC/"
+ "/db/6/schema/PUBLIC/table/7/"
+ "/collection/8/"
+ "/collection/9/read/"}))))
+
+ (testing "If the current user has root permissions then make sure the function returns `:all`, which signifies that they are able to see all Collections"
+ (is (= :all
+ (collection/permissions-set->visible-collection-ids
+ #{"/"
+ "/db/2/native/"
+ "/collection/9/read/"}))))
+
+ (testing "for the Root Collection we should return `root`"
+ (is (= #{8 9 "root"}
+ (collection/permissions-set->visible-collection-ids
+ #{"/collection/8/"
+ "/collection/9/read/"
+ "/collection/root/"})))
+
+ (is (= #{"root"}
+ (collection/permissions-set->visible-collection-ids
+ #{"/collection/root/read/"})))))
+
+(deftest effective-location-path-test
+ (testing "valid input"
+ (doseq [[args expected] {["/10/20/30/" #{10 20}] "/10/20/"
+ ["/10/20/30/" #{10 30}] "/10/30/"
+ ["/10/20/30/" #{}] "/"
+ ["/10/20/30/" #{10 20 30}] "/10/20/30/"
+ ["/10/20/30/" :all] "/10/20/30/"}]
+ (testing (pr-str (cons 'effective-location-path args))
+ (is (= expected
+ (apply collection/effective-location-path args))))))
+
+ (testing "invalid input"
+ (doseq [args [["/10/20/30/" nil]
+ ["/10/20/30/" [20]]
+ [nil #{}]
+ [[10 20] #{}]]]
+ (testing (pr-str (cons 'effective-location-path args))
+ (is (thrown?
+ Exception
+ (apply collection/effective-location-path args))))))
+
+ (testing "Does the function also work if we call the single-arity version that powers hydration?"
+ (testing "mix of full and read perms"
+ (binding [*current-user-permissions-set* (atom #{"/collection/10/" "/collection/20/read/"})]
+ (is (= "/10/20/"
+ (collection/effective-location-path {:location "/10/20/30/"})))))
+
+ (testing "missing some perms"
+ (binding [*current-user-permissions-set* (atom #{"/collection/10/read/" "/collection/30/read/"})]
+ (is (= "/10/30/"
+ (collection/effective-location-path {:location "/10/20/30/"})))))
+
+ (testing "no perms"
+ (binding [*current-user-permissions-set* (atom #{})]
+ (is (= "/"
+ (collection/effective-location-path {:location "/10/20/30/"})))))
+
+ (testing "read perms for all"
+ (binding [*current-user-permissions-set* (atom #{"/collection/10/" "/collection/20/read/" "/collection/30/read/"})]
+ (is (= "/10/20/30/"
+ (collection/effective-location-path {:location "/10/20/30/"})))))
+
+ (testing "root perms"
+ (binding [*current-user-permissions-set* (atom #{"/"})]
+ (is (= "/10/20/30/"
+ (collection/effective-location-path {:location "/10/20/30/"})))))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Nested Collections: CRUD Constraints & Behavior |
@@ -574,13 +332,13 @@
;; Can we INSERT a Collection with a valid path?
(defn- insert-collection-with-location! [location]
- (tu/with-model-cleanup [Collection]
- (-> (db/insert! Collection :name (tu/random-name), :color "#ABCDEF", :location location)
+ (mt/with-model-cleanup [Collection]
+ (-> (db/insert! Collection :name (mt/random-name), :color "#ABCDEF", :location location)
:location
(= location))))
(expect
- (tt/with-temp Collection [parent]
+ (mt/with-temp Collection [parent]
(insert-collection-with-location! (collection/location-path parent))))
;; Make sure we can't INSERT a Collection with an invalid path
@@ -599,20 +357,20 @@
;; MAae sure we can UPDATE a Collection and give it a new, *valid* location
(expect
- (tt/with-temp* [Collection [collection-1]
+ (mt/with-temp* [Collection [collection-1]
Collection [collection-2]]
(db/update! Collection (u/get-id collection-1) :location (collection/location-path collection-2))))
;; Make sure we can't UPDATE a Collection to give it an valid path
(expect
Exception
- (tt/with-temp Collection [collection]
+ (mt/with-temp Collection [collection]
(db/update! Collection (u/get-id collection) :location "/a/")))
;; Make sure we can't UPDATE a Collection to give it a non-existent ancestors
(expect
Exception
- (tt/with-temp Collection [collection]
+ (mt/with-temp Collection [collection]
(db/update! Collection (u/get-id collection) :location (collection/location-path (nonexistent-collection-id)))))
@@ -804,12 +562,13 @@
(with-collection-hierarchy [{:keys [a b c d e f g]}]
(descendants collection/root-collection)))
-;; double-check that descendant-ids is working right too
-(tt/expect-with-temp [Collection [a]
- Collection [b {:location (collection/children-location a)}]
- Collection [c {:location (collection/children-location b)}]]
- #{(u/get-id b) (u/get-id c)}
- (#'collection/descendant-ids a))
+(deftest descendant-ids-test
+ (testing "double-check that descendant-ids is working right too"
+ (mt/with-temp* [Collection [a]
+ Collection [b {:location (collection/children-location a)}]
+ Collection [c {:location (collection/children-location b)}]]
+ (is (= #{(u/get-id b) (u/get-id c)}
+ (#'collection/descendant-ids a))))))
;;; ----------------------------------------------- Effective Children -----------------------------------------------
@@ -944,173 +703,166 @@
;;; ---------------------------------------------- Perms for Archiving -----------------------------------------------
-;; To Archive A, you should need *write* perms for A and all of its descendants, and also the Root Collection...
-(expect
- #{"/collection/root/"
- "/collection/A/"
- "/collection/B/"
- "/collection/C/"
- "/collection/D/"
- "/collection/E/"
- "/collection/F/"
- "/collection/G/"}
- (with-collection-hierarchy [{:keys [a], :as collections}]
- (->> (collection/perms-for-archiving a)
- (perms-path-ids->names collections))))
-
-;; Now let's move down a level. To archive B, you should need permissions for A and B, since B doesn't
-;; have any descendants
-(expect
- #{"/collection/A/"
- "/collection/B/"}
- (with-collection-hierarchy [{:keys [b], :as collections}]
- (->> (collection/perms-for-archiving b)
- (perms-path-ids->names collections))))
-
-;; but for C, you should need perms for A (parent); C; and D, E, F, and G (descendants)
-(expect
- #{"/collection/A/"
- "/collection/C/"
- "/collection/D/"
- "/collection/E/"
- "/collection/F/"
- "/collection/G/"}
- (with-collection-hierarchy [{:keys [c], :as collections}]
- (->> (collection/perms-for-archiving c)
- (perms-path-ids->names collections))))
-
-;; For D you should need C (parent), D, and E (descendant)
-(expect
- #{"/collection/C/"
- "/collection/D/"
- "/collection/E/"}
- (with-collection-hierarchy [{:keys [d], :as collections}]
- (->> (collection/perms-for-archiving d)
- (perms-path-ids->names collections))))
-
-;; If you try to calculate permissions to archive the Root Collection, throw an Exception! Because you can't do
-;; that...
-(expect
- Exception
- (collection/perms-for-archiving collection/root-collection))
-
-;; Let's make sure we get an Exception when we try to archive a Personal Collection
-(expect
- Exception
- (collection/perms-for-archiving (collection/user->personal-collection (test-users/fetch-user :lucky))))
-
-;; also you should get an Exception if you try to pull a fast one on use and pass in some sort of invalid input...
-(expect Exception (collection/perms-for-archiving nil))
-(expect Exception (collection/perms-for-archiving {}))
-(expect Exception (collection/perms-for-archiving 1))
+(deftest perms-for-archiving-test
+ (with-collection-hierarchy [{:keys [a b c d], :as collections}]
+ (testing "To Archive A, you should need *write* perms for A and all of its descendants, and also the Root Collection..."
+ (is (= #{"/collection/root/"
+ "/collection/A/"
+ "/collection/B/"
+ "/collection/C/"
+ "/collection/D/"
+ "/collection/E/"
+ "/collection/F/"
+ "/collection/G/"}
+ (->> (collection/perms-for-archiving a)
+ (perms-path-ids->names collections)))))
+
+ (testing (str "Now let's move down a level. To archive B, you should need permissions for A and B, since B doesn't "
+ "have any descendants")
+ (is (= #{"/collection/A/"
+ "/collection/B/"}
+ (->> (collection/perms-for-archiving b)
+ (perms-path-ids->names collections)))))
+
+ (testing "but for C, you should need perms for A (parent); C; and D, E, F, and G (descendants)"
+ (is (= #{"/collection/A/"
+ "/collection/C/"
+ "/collection/D/"
+ "/collection/E/"
+ "/collection/F/"
+ "/collection/G/"}
+ (->> (collection/perms-for-archiving c)
+ (perms-path-ids->names collections)))))
+
+ (testing "For D you should need C (parent), D, and E (descendant)"
+ (is (= #{"/collection/C/"
+ "/collection/D/"
+ "/collection/E/"}
+ (->> (collection/perms-for-archiving d)
+ (perms-path-ids->names collections)))))))
+
+(deftest perms-for-archiving-exceptions-test
+ (testing "If you try to calculate permissions to archive the Root Collection, throw an Exception!"
+ (is (thrown?
+ Exception
+ (collection/perms-for-archiving collection/root-collection))))
+
+ (testing "Let's make sure we get an Exception when we try to archive a Personal Collection"
+ (is (thrown?
+ Exception
+ (collection/perms-for-archiving (collection/user->personal-collection (mt/fetch-user :lucky))))))
+
+ (testing "invalid input"
+ (doseq [input [nil {} 1]]
+ (testing (format "input = %s" (pr-str input))
+ (is (thrown?
+ Exception
+ (collection/perms-for-archiving input)))))))
;;; ------------------------------------------------ Perms for Moving ------------------------------------------------
;; `*` marks the things that require permissions in charts below!
-;; If we want to move B into C, we should need perms for A, B, and C. B because it is being moved; C we are moving
-;; something into it, A because we are moving something out of it
-;;
-;; +-> B +-> B*
-;; | |
-;; A -+-> C -+-> D -> E ===> A* -> C* -+-> D -> E
-;; | |
-;; +-> F -> G +-> F -> G
-
-(expect
- #{"/collection/A/"
- "/collection/B/"
- "/collection/C/"}
- (with-collection-hierarchy [{:keys [b c], :as collections}]
- (->> (collection/perms-for-moving b c)
- (perms-path-ids->names collections))))
-
-;; Ok, now let's try moving something with descendants. If we move C into B, we need perms for C and all its
-;; descendants, and B, since it's the new parent; and A, the old parent
-;;
-;; +-> B
-;; |
-;; A -+-> C -+-> D -> E ===> A* -> B* -> C* -+-> D* -> E*
-;; | |
-;; +-> F -> G +-> F* -> G*
-(expect
- #{"/collection/A/"
- "/collection/B/"
- "/collection/C/"
- "/collection/D/"
- "/collection/E/"
- "/collection/F/"
- "/collection/G/"}
+(deftest perms-for-moving-test
(with-collection-hierarchy [{:keys [b c], :as collections}]
- (->> (collection/perms-for-moving c b)
- (perms-path-ids->names collections))))
-
-;; Ok, now how about moving B into the Root Collection?
-;;
-;; +-> B B* [and Root*]
-;; |
-;; A -+-> C -+-> D -> E ===> A* -> C -+-> D -> E
-;; | |
-;; +-> F -> G +-> F -> G
-(expect
- #{"/collection/root/"
- "/collection/A/"
- "/collection/B/"}
- (with-collection-hierarchy [{:keys [b], :as collections}]
- (->> (collection/perms-for-moving b collection/root-collection)
- (perms-path-ids->names collections))))
-
-;; How about moving C into the Root Collection?
-;;
-;; +-> B A* -> B
-;; |
-;; A -+-> C -+-> D -> E ===> C* -+-> D* -> E* [and Root*]
-;; | |
-;; +-> F -> G +-> F* -> G*
-(expect
- #{"/collection/root/"
- "/collection/A/"
- "/collection/C/"
- "/collection/D/"
- "/collection/E/"
- "/collection/F/"
- "/collection/G/"}
- (with-collection-hierarchy [{:keys [c], :as collections}]
- (->> (collection/perms-for-moving c collection/root-collection)
- (perms-path-ids->names collections))))
-
-;; If you try to calculate permissions to move or archive the Root Collection, throw an Exception! Because you can't
-;; do that...
-(expect
- Exception
- (with-collection-hierarchy [{:keys [a]}]
- (collection/perms-for-moving collection/root-collection a)))
-
-;; You should also see an Exception if you try to move a Collection into itself or into one its descendants...
-(expect
- Exception
- (with-collection-hierarchy [{:keys [b]}]
- (collection/perms-for-moving b b)))
-
-(expect
- Exception
- (with-collection-hierarchy [{:keys [a b]}]
- (collection/perms-for-moving a b)))
-
-;; Let's make sure we get an Exception when we try to *move* a Collection
-(expect
- Exception
- (with-collection-hierarchy [{:keys [a]}]
- (collection/perms-for-moving (collection/user->personal-collection (test-users/fetch-user :lucky)) a)))
-
-;; also you should get an Exception if you try to pull a fast one on use and pass in some sort of invalid input...
-(expect Exception (collection/perms-for-moving {:location "/"} nil))
-(expect Exception (collection/perms-for-moving {:location "/"} {}))
-(expect Exception (collection/perms-for-moving {:location "/"} 1))
-(expect Exception (collection/perms-for-moving nil {:location "/"}))
-(expect Exception (collection/perms-for-moving {} {:location "/"}))
-(expect Exception (collection/perms-for-moving 1 {:location "/"}))
+ (testing "If we want to move B into C, we should need perms for A, B, and C."
+ ;; B because it is being moved; C we are moving
+ ;; something into it, A because we are moving something out of it
+ ;;
+ ;; +-> B +-> B*
+ ;; | |
+ ;; A -+-> C -+-> D -> E ===> A* -> C* -+-> D -> E
+ ;; | |
+ ;; +-> F -> G +-> F -> G
+
+ (is (= #{"/collection/A/"
+ "/collection/B/"
+ "/collection/C/"}
+ (->> (collection/perms-for-moving b c)
+ (perms-path-ids->names collections)))))
+
+ (testing "Ok, now let's try moving something with descendants."
+ ;; If we move C into B, we need perms for C and all its
+ ;; descendants, and B, since it's the new parent; and A, the old parent
+ ;;
+ ;; +-> B
+ ;; |
+ ;; A -+-> C -+-> D -> E ===> A* -> B* -> C* -+-> D* -> E*
+ ;; | |
+ ;; +-> F -> G +-> F* -> G*
+ (is (= #{"/collection/A/"
+ "/collection/B/"
+ "/collection/C/"
+ "/collection/D/"
+ "/collection/E/"
+ "/collection/F/"
+ "/collection/G/"}
+ (->> (collection/perms-for-moving c b)
+ (perms-path-ids->names collections)))))
+
+ (testing "Ok, now how about moving B into the Root Collection?"
+ ;; +-> B B* [and Root*]
+ ;; |
+ ;; A -+-> C -+-> D -> E ===> A* -> C -+-> D -> E
+ ;; | |
+ ;; +-> F -> G +-> F -> G
+ (is (= #{"/collection/root/"
+ "/collection/A/"
+ "/collection/B/"}
+ (->> (collection/perms-for-moving b collection/root-collection)
+ (perms-path-ids->names collections)))))
+
+ (testing "How about moving C into the Root Collection?"
+ ;; +-> B A* -> B
+ ;; |
+ ;; A -+-> C -+-> D -> E ===> C* -+-> D* -> E* [and Root*]
+ ;; | |
+ ;; +-> F -> G +-> F* -> G*
+ (is (= #{"/collection/root/"
+ "/collection/A/"
+ "/collection/C/"
+ "/collection/D/"
+ "/collection/E/"
+ "/collection/F/"
+ "/collection/G/"}
+ (->> (collection/perms-for-moving c collection/root-collection)
+ (perms-path-ids->names collections)))))))
+
+(deftest perms-for-moving-exceptions-test
+ (with-collection-hierarchy [{:keys [a b], :as collections}]
+ (testing "If you try to calculate permissions to move or archive the Root Collection, throw an Exception!"
+ (is (thrown?
+ Exception
+ (collection/perms-for-moving collection/root-collection a))))
+
+ (testing "You should also see an Exception if you try to move a Collection into itself or into one its descendants..."
+ (testing "B -> B"
+ (is (thrown?
+ Exception
+ (collection/perms-for-moving b b))))
+
+ (testing "A -> B"
+ (is (thrown?
+ Exception
+ (collection/perms-for-moving a b)))))
+
+ (testing "Let's make sure we get an Exception when we try to *move* a Personal Collection"
+ (is (thrown?
+ Exception
+ (collection/perms-for-moving (collection/user->personal-collection (mt/fetch-user :lucky)) a)))))
+
+ (testing "invalid input"
+ (doseq [[collection new-parent] [[{:location "/"} nil]
+ [{:location "/"} {}]
+ [{:location "/"} 1]
+ [nil {:location "/"}]
+ [{} {:location "/"}]
+ [1 {:location "/"}]]]
+ (testing (pr-str (list 'perms-for-moving collection new-parent))
+ (is (thrown?
+ Exception
+ (collection/perms-for-moving collection new-parent)))))))
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -1226,179 +978,129 @@
(with-collection-hierarchy [collections]
(collection-locations (vals collections) :archived false)))
-;; Test that we can archive a Collection with no descendants!
-;;
-;; +-> B +-> B
-;; | |
-;; A -+-> C -+-> D -> E ===> A -+-> C -+-> D
-;; | |
-;; +-> F -> G +-> F -> G
-(expect
- {"A" {"B" {}
- "C" {"D" {}
- "F" {"G" {}}}}}
- (with-collection-hierarchy [{:keys [e], :as collections}]
- (db/update! Collection (u/get-id e) :archived true)
- (collection-locations (vals collections) :archived false)))
-
-;; Test that we can archive a Collection *with* descendants
-;;
-;; +-> B +-> B
-;; | |
-;; A -+-> C -+-> D -> E ===> A -+
-;; |
-;; +-> F -> G
-(expect
- {"A" {"B" {}}}
- (with-collection-hierarchy [{:keys [c], :as collections}]
- (db/update! Collection (u/get-id c) :archived true)
- (collection-locations (vals collections) :archived false)))
-
-;; Test that we can unarchive a Collection with no descendants
-;;
-;; +-> B +-> B
-;; | |
-;; A -+-> C -+-> D ===> A -+-> C -+-> D -> E
-;; | |
-;; +-> F -> G +-> F -> G
-(expect
- {"A" {"B" {}
- "C" {"D" {"E" {}}
- "F" {"G" {}}}}}
- (with-collection-hierarchy [{:keys [e], :as collections}]
- (db/update! Collection (u/get-id e) :archived true)
- (db/update! Collection (u/get-id e) :archived false)
- (collection-locations (vals collections) :archived false)))
-
-
-;; Test that we can unarchive a Collection *with* descendants
-;;
-;; +-> B +-> B
-;; | |
-;; A -+ ===> A -+-> C -+-> D -> E
-;; |
-;; +-> F -> G
-(expect
- {"A" {"B" {}
- "C" {"D" {"E" {}}
- "F" {"G" {}}}}}
- (with-collection-hierarchy [{:keys [c], :as collections}]
- (db/update! Collection (u/get-id c) :archived true)
- (db/update! Collection (u/get-id c) :archived false)
- (collection-locations (vals collections) :archived false)))
-
-;; Test that archiving applies to Cards
-;; Card is in E; archiving E should cause Card to be archived
-(expect
- (with-collection-hierarchy [{:keys [e], :as collections}]
- (tt/with-temp Card [card {:collection_id (u/get-id e)}]
+(deftest nested-collections-archiving-test
+ (testing "Test that we can archive a Collection with no descendants!"
+ ;; +-> B +-> B
+ ;; | |
+ ;; A -+-> C -+-> D -> E ===> A -+-> C -+-> D
+ ;; | |
+ ;; +-> F -> G +-> F -> G
+ (with-collection-hierarchy [{:keys [e], :as collections}]
(db/update! Collection (u/get-id e) :archived true)
- (db/select-one-field :archived Card :id (u/get-id card)))))
-
-;; Test that archiving applies to Cards belonging to descendant Collections
-;; Card is in E, a descendant of C; archiving C should cause Card to be archived
-(expect
- (with-collection-hierarchy [{:keys [c e], :as collections}]
- (tt/with-temp Card [card {:collection_id (u/get-id e)}]
+ (is (= {"A" {"B" {}
+ "C" {"D" {}
+ "F" {"G" {}}}}}
+ (collection-locations (vals collections) :archived false)))))
+
+ (testing "Test that we can archive a Collection *with* descendants"
+ ;; +-> B +-> B
+ ;; | |
+ ;; A -+-> C -+-> D -> E ===> A -+
+ ;; |
+ ;; +-> F -> G
+ (with-collection-hierarchy [{:keys [c], :as collections}]
(db/update! Collection (u/get-id c) :archived true)
- (db/select-one-field :archived Card :id (u/get-id card)))))
-
-;; Test that archiving applies to Dashboards
-;; Dashboard is in E; archiving E should cause Dashboard to be archived
-(expect
- (with-collection-hierarchy [{:keys [e], :as collections}]
- (tt/with-temp Dashboard [dashboard {:collection_id (u/get-id e)}]
+ (is (= {"A" {"B" {}}}
+ (collection-locations (vals collections) :archived false))))))
+
+(deftest nested-collection-unarchiving-test
+ (testing "Test that we can unarchive a Collection with no descendants"
+ ;; +-> B +-> B
+ ;; | |
+ ;; A -+-> C -+-> D ===> A -+-> C -+-> D -> E
+ ;; | |
+ ;; +-> F -> G +-> F -> G
+ (with-collection-hierarchy [{:keys [e], :as collections}]
(db/update! Collection (u/get-id e) :archived true)
- (db/select-one-field :archived Dashboard :id (u/get-id dashboard)))))
-
-;; Test that archiving applies to Dashboards belonging to descendant Collections
-;; Dashboard is in E, a descendant of C; archiving C should cause Dashboard to be archived
-(expect
- (with-collection-hierarchy [{:keys [c e], :as collections}]
- (tt/with-temp Dashboard [dashboard {:collection_id (u/get-id e)}]
- (db/update! Collection (u/get-id c) :archived true)
- (db/select-one-field :archived Dashboard :id (u/get-id dashboard)))))
-
-;; Test that archiving Collections applies to Pulses
-;; Pulse is in E; archiving E should cause Pulse to be archived
-(expect
- (with-collection-hierarchy [{:keys [e], :as collections}]
- (tt/with-temp Pulse [pulse {:collection_id (u/get-id e)}]
- (db/update! Collection (u/get-id e) :archived true)
- (db/select-one-field :archived Pulse :id (u/get-id pulse)))))
-
-;; Test that archiving works on Pulses belonging to descendant Collections
-;; Pulse is in E, a descendant of C; archiving C should cause Pulse to be archived
-(expect
- (with-collection-hierarchy [{:keys [c e], :as collections}]
- (tt/with-temp Pulse [pulse {:collection_id (u/get-id e)}]
- (db/update! Collection (u/get-id c) :archived true)
- (db/select-one-field :archived Pulse :id (u/get-id pulse)))))
-
-;; Test that unarchiving applies to Cards
-;; Card is in E; unarchiving E should cause Card to be unarchived
-(expect
- false
- (with-collection-hierarchy [{:keys [e], :as collections}]
- (db/update! Collection (u/get-id e) :archived true)
- (tt/with-temp Card [card {:collection_id (u/get-id e), :archived true}]
(db/update! Collection (u/get-id e) :archived false)
- (db/select-one-field :archived Card :id (u/get-id card)))))
-
-;; Test that unarchiving applies to Cards belonging to descendant Collections
-;; Card is in E, a descendant of C; unarchiving C should cause Card to be unarchived
-(expect
- false
- (with-collection-hierarchy [{:keys [c e], :as collections}]
- (db/update! Collection (u/get-id c) :archived true)
- (tt/with-temp Card [card {:collection_id (u/get-id e), :archived true}]
- (db/update! Collection (u/get-id c) :archived false)
- (db/select-one-field :archived Card :id (u/get-id card)))))
-
-;; Test that unarchiving applies to Dashboards
-;; Dashboard is in E; unarchiving E should cause Dashboard to be unarchived
-(expect
- false
- (with-collection-hierarchy [{:keys [e], :as collections}]
- (db/update! Collection (u/get-id e) :archived true)
- (tt/with-temp Dashboard [dashboard {:collection_id (u/get-id e), :archived true}]
- (db/update! Collection (u/get-id e) :archived false)
- (db/select-one-field :archived Dashboard :id (u/get-id dashboard)))))
-
-;; Test that unarchiving applies to Dashboards belonging to descendant Collections
-;; Dashboard is in E, a descendant of C; unarchiving C should cause Dashboard to be unarchived
-(expect
- false
- (with-collection-hierarchy [{:keys [c e], :as collections}]
- (db/update! Collection (u/get-id c) :archived true)
- (tt/with-temp Dashboard [dashboard {:collection_id (u/get-id e), :archived true}]
+ (is (= {"A" {"B" {}
+ "C" {"D" {"E" {}}
+ "F" {"G" {}}}}}
+ (collection-locations (vals collections) :archived false)))))
+
+ (testing "Test that we can unarchive a Collection *with* descendants"
+ ;; +-> B +-> B
+ ;; | |
+ ;; A -+ ===> A -+-> C -+-> D -> E
+ ;; |
+ ;; +-> F -> G
+ (with-collection-hierarchy [{:keys [c], :as collections}]
+ (db/update! Collection (u/get-id c) :archived true)
(db/update! Collection (u/get-id c) :archived false)
- (db/select-one-field :archived Dashboard :id (u/get-id dashboard)))))
-
-;; Test that we cannot archive a Collection at the same time we are moving it
-(expect
- Exception
- (with-collection-hierarchy [{:keys [c], :as collections}]
- (db/update! Collection (u/get-id c), :archived true, :location "/")))
-
-;; Test that we cannot unarchive a Collection at the same time we are moving it
-(expect
- Exception
- (with-collection-hierarchy [{:keys [c], :as collections}]
- (db/update! Collection (u/get-id c), :archived true)
- (db/update! Collection (u/get-id c), :archived false, :location "/")))
-
-;; Passing in a value of archived that is the same as the value in the DB shouldn't affect anything however!
-(expect
- (with-collection-hierarchy [{:keys [c], :as collections}]
- (db/update! Collection (u/get-id c), :archived false, :location "/")))
-
-;; Check that attempting to unarchive a Card that's not archived doesn't affect arcived descendants
-(expect
- (with-collection-hierarchy [{:keys [c e], :as collections}]
- (db/update! Collection (u/get-id e), :archived true)
- (db/update! Collection (u/get-id c), :archived false)
- (db/select-one-field :archived Collection :id (u/get-id e))))
+ (is (= {"A" {"B" {}
+ "C" {"D" {"E" {}}
+ "F" {"G" {}}}}}
+ (collection-locations (vals collections) :archived false))))))
+
+(deftest nested-collection-archiving-objects-test
+ (doseq [model [Card Dashboard NativeQuerySnippet Pulse]]
+ (testing (format "Test that archiving applies to %ss" (name model))
+ ;; object is in E; archiving E should cause object to be archived
+ (with-collection-hierarchy [{:keys [e], :as collections} (when (= model NativeQuerySnippet)
+ {:namespace "snippets"})]
+ (mt/with-temp model [object {:collection_id (u/get-id e)}]
+ (db/update! Collection (u/get-id e) :archived true)
+ (is (= true
+ (db/select-one-field :archived model :id (u/get-id object)))))))
+
+ (testing (format "Test that archiving applies to %ss belonging to descendant Collections" (name model))
+ ;; object is in E, a descendant of C; archiving C should cause object to be archived
+ (with-collection-hierarchy [{:keys [c e], :as collections} (when (= model NativeQuerySnippet)
+ {:namespace "snippets"})]
+ (mt/with-temp model [object {:collection_id (u/get-id e)}]
+ (db/update! Collection (u/get-id c) :archived true)
+ (is (= true
+ (db/select-one-field :archived model :id (u/get-id object)))))))))
+
+(deftest nested-collection-unarchiving-objects-test
+ (doseq [model [Card Dashboard NativeQuerySnippet Pulse]]
+ (testing (format "Test that unarchiving applies to %ss" (name model))
+ ;; object is in E; unarchiving E should cause object to be unarchived
+ (with-collection-hierarchy [{:keys [e], :as collections} (when (= model NativeQuerySnippet)
+ {:namespace "snippets"})]
+ (db/update! Collection (u/get-id e) :archived true)
+ (mt/with-temp model [object {:collection_id (u/get-id e), :archived true}]
+ (db/update! Collection (u/get-id e) :archived false)
+ (is (= false
+ (db/select-one-field :archived model :id (u/get-id object)))))))
+
+ (testing (format "Test that unarchiving applies to %ss belonging to descendant Collections" (name model))
+ ;; object is in E, a descendant of C; unarchiving C should cause object to be unarchived
+ (with-collection-hierarchy [{:keys [c e], :as collections} (when (= model NativeQuerySnippet)
+ {:namespace "snippets"})]
+ (db/update! Collection (u/get-id c) :archived true)
+ (mt/with-temp model [object {:collection_id (u/get-id e), :archived true}]
+ (db/update! Collection (u/get-id c) :archived false)
+ (is (= false
+ (db/select-one-field :archived model :id (u/get-id object)))))))))
+
+(deftest archive-while-moving-test
+ (testing "Test that we cannot archive a Collection at the same time we are moving it"
+ (with-collection-hierarchy [{:keys [c], :as collections}]
+ (is (thrown?
+ Exception
+ (db/update! Collection (u/get-id c), :archived true, :location "/")))))
+
+ (testing "Test that we cannot unarchive a Collection at the same time we are moving it"
+ (with-collection-hierarchy [{:keys [c], :as collections}]
+ (db/update! Collection (u/get-id c), :archived true)
+ (is (thrown?
+ Exception
+ (db/update! Collection (u/get-id c), :archived false, :location "/")))))
+
+ (testing "Passing in a value of archived that is the same as the value in the DB shouldn't affect anything however!"
+ (with-collection-hierarchy [{:keys [c], :as collections}]
+ (db/update! Collection (u/get-id c), :archived false, :location "/")
+ (is (= "/"
+ (db/select-one-field :location Collection :id (u/get-id c)))))))
+
+(deftest archive-noop-shouldnt-affect-descendants-test
+ (testing "Check that attempting to unarchive a Card that's not archived doesn't affect archived descendants"
+ (with-collection-hierarchy [{:keys [c e], :as collections}]
+ (db/update! Collection (u/get-id e), :archived true)
+ (db/update! Collection (u/get-id c), :archived false)
+ (is (= true
+ (db/select-one-field :archived Collection :id (u/get-id e)))))))
;; TODO - can you unarchive a Card that is inside an archived Collection??
@@ -1422,30 +1124,30 @@
(expect
#{"/collection/{new}/"
"/collection/root/"}
- (tt/with-temp PermissionsGroup [group]
+ (mt/with-temp PermissionsGroup [group]
(perms/grant-collection-readwrite-permissions! group collection/root-collection)
- (tt/with-temp Collection [collection {:name "{new}"}]
+ (mt/with-temp Collection [collection {:name "{new}"}]
(group->perms [collection] group))))
(expect
#{"/collection/{new}/read/"
"/collection/root/read/"}
- (tt/with-temp PermissionsGroup [group]
+ (mt/with-temp PermissionsGroup [group]
(perms/grant-collection-read-permissions! group collection/root-collection)
- (tt/with-temp Collection [collection {:name "{new}"}]
+ (mt/with-temp Collection [collection {:name "{new}"}]
(group->perms [collection] group))))
;; Needless to say, no perms before hand = no perms after
(expect
#{}
- (tt/with-temp PermissionsGroup [group]
- (tt/with-temp Collection [collection {:name "{new}"}]
+ (mt/with-temp PermissionsGroup [group]
+ (mt/with-temp Collection [collection {:name "{new}"}]
(group->perms [collection] group))))
;; ...and granting perms after shouldn't affect Collections already created
(expect
#{"/collection/root/read/"}
- (tt/with-temp* [PermissionsGroup [group]
+ (mt/with-temp* [PermissionsGroup [group]
Collection [collection {:name "{new}"}]]
(perms/grant-collection-read-permissions! group collection/root-collection)
(group->perms [collection] group)))
@@ -1454,31 +1156,31 @@
(expect
#{"/collection/{parent}/"
"/collection/{child}/"}
- (tt/with-temp* [PermissionsGroup [group]
+ (mt/with-temp* [PermissionsGroup [group]
Collection [parent {:name "{parent}"}]]
(perms/grant-collection-readwrite-permissions! group parent)
- (tt/with-temp Collection [child {:name "{child}", :location (collection/children-location parent)}]
+ (mt/with-temp Collection [child {:name "{child}", :location (collection/children-location parent)}]
(group->perms [parent child] group))))
(expect
#{"/collection/{parent}/read/"
"/collection/{child}/read/"}
- (tt/with-temp* [PermissionsGroup [group]
+ (mt/with-temp* [PermissionsGroup [group]
Collection [parent {:name "{parent}"}]]
(perms/grant-collection-read-permissions! group parent)
- (tt/with-temp Collection [child {:name "{child}", :location (collection/children-location parent)}]
+ (mt/with-temp Collection [child {:name "{child}", :location (collection/children-location parent)}]
(group->perms [parent child] group))))
(expect
#{}
- (tt/with-temp* [PermissionsGroup [group]
+ (mt/with-temp* [PermissionsGroup [group]
Collection [parent {:name "{parent}"}]
Collection [child {:name "{child}", :location (collection/children-location parent)}]]
(group->perms [parent child] group)))
(expect
#{"/collection/{parent}/read/"}
- (tt/with-temp* [PermissionsGroup [group]
+ (mt/with-temp* [PermissionsGroup [group]
Collection [parent {:name "{parent}"}]
Collection [child {:name "{child}", :location (collection/children-location parent)}]]
(perms/grant-collection-read-permissions! group parent)
@@ -1487,23 +1189,23 @@
;; If we have Root Collection perms they shouldn't be copied for a Child
(expect
#{"/collection/root/read/"}
- (tt/with-temp* [PermissionsGroup [group]
+ (mt/with-temp* [PermissionsGroup [group]
Collection [parent {:name "{parent}"}]]
(perms/grant-collection-read-permissions! group collection/root-collection)
- (tt/with-temp Collection [child {:name "{child}", :location (collection/children-location parent)}]
+ (mt/with-temp Collection [child {:name "{child}", :location (collection/children-location parent)}]
(group->perms [parent child] group))))
;; Make sure that when creating a new Collection as child of a Personal Collection, no group permissions are created
(expect
false
- (tt/with-temp Collection [child {:name "{child}", :location (lucky-collection-children-location)}]
+ (mt/with-temp Collection [child {:name "{child}", :location (lucky-collection-children-location)}]
(db/exists? Permissions :object [:like (format "/collection/%d/%%" (u/get-id child))])))
;; Make sure that when creating a new Collection as grandchild of a Personal Collection, no group permissions are
;; created
(expect
false
- (tt/with-temp* [Collection [child {:location (lucky-collection-children-location)}]
+ (mt/with-temp* [Collection [child {:location (lucky-collection-children-location)}]
Collection [grandchild {:location (collection/children-location child)}]]
(or (db/exists? Permissions :object [:like (format "/collection/%d/%%" (u/get-id child))])
(db/exists? Permissions :object [:like (format "/collection/%d/%%" (u/get-id grandchild))]))))
@@ -1513,34 +1215,35 @@
;;; | Personal Collections |
;;; +----------------------------------------------------------------------------------------------------------------+
-;; Make sure we're not allowed to *unarchive* a Personal Collection
-(expect
- Exception
- (tt/with-temp User [my-cool-user]
- (let [personal-collection (collection/user->personal-collection my-cool-user)]
- (db/update! Collection (u/get-id personal-collection) :archived true))))
-
-;; Make sure we're not allowed to *move* a Personal Collection
-(expect
- Exception
- (tt/with-temp* [User [my-cool-user]
- Collection [some-other-collection]]
- (let [personal-collection (collection/user->personal-collection my-cool-user)]
- (db/update! Collection (u/get-id personal-collection) :location (collection/location-path some-other-collection)))))
-
-;; Make sure we're not allowed to change the owner of a Personal Collection
-(expect
- Exception
- (tt/with-temp User [my-cool-user]
- (let [personal-collection (collection/user->personal-collection my-cool-user)]
- (db/update! Collection (u/get-id personal-collection) :personal_owner_id (test-users/user->id :crowberto)))))
-
-;; Does hydrating `:personal_collection_id` force creation of Personal Collections?
-(expect
- (tt/with-temp User [temp-user]
- (-> (hydrate temp-user :personal_collection_id)
- :personal_collection_id
- integer?)))
+(deftest personal-collections-restrictions-test
+ (testing "Make sure we're not allowed to *unarchive* a Personal Collection"
+ (mt/with-temp User [my-cool-user]
+ (let [personal-collection (collection/user->personal-collection my-cool-user)]
+ (is (thrown?
+ Exception
+ (db/update! Collection (u/get-id personal-collection) :archived true))))))
+
+ (testing "Make sure we're not allowed to *move* a Personal Collection"
+ (mt/with-temp* [User [my-cool-user]
+ Collection [some-other-collection]]
+ (let [personal-collection (collection/user->personal-collection my-cool-user)]
+ (is (thrown?
+ Exception
+ (db/update! Collection (u/get-id personal-collection)
+ :location (collection/location-path some-other-collection)))))))
+
+ (testing "Make sure we're not allowed to change the owner of a Personal Collection"
+ (mt/with-temp User [my-cool-user]
+ (let [personal-collection (collection/user->personal-collection my-cool-user)]
+ (is (thrown?
+ Exception
+ (db/update! Collection (u/get-id personal-collection) :personal_owner_id (mt/user->id :crowberto)))))))
+
+ (testing "Does hydrating `:personal_collection_id` force creation of Personal Collections?"
+ (mt/with-temp User [temp-user]
+ (is (schema= {:personal_collection_id su/IntGreaterThanZero
+ s/Keyword s/Any}
+ (hydrate temp-user :personal_collection_id))))))
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -1559,13 +1262,13 @@
;; ===>
;; Root Collection Root Collection > A
(expect
- #{"/collection/root/read/"
- "/collection/A/read/"}
- (tt/with-temp* [PermissionsGroup [group]
- Collection [a {:name "A", :location (lucky-collection-children-location)}]]
- (perms/grant-collection-read-permissions! group collection/root-collection)
- (db/update! Collection (u/get-id a) :location (collection/children-location collection/root-collection))
- (group->perms [a] group)))
+ #{"/collection/root/read/"
+ "/collection/A/read/"}
+ (mt/with-temp* [PermissionsGroup [group]
+ Collection [a {:name "A", :location (lucky-collection-children-location)}]]
+ (perms/grant-collection-read-permissions! group collection/root-collection)
+ (db/update! Collection (u/get-id a) :location (collection/children-location collection/root-collection))
+ (group->perms [a] group)))
;; When moving a Collection from a *descendant* of a Personal Collection to the Root Collection, we should create
;; perms entries that match the Root Collection's entries for any groups that have Root Collection perms.
@@ -1576,7 +1279,7 @@
(expect
#{"/collection/root/"
"/collection/B/"}
- (tt/with-temp* [PermissionsGroup [group]
+ (mt/with-temp* [PermissionsGroup [group]
Collection [a {:name "A", :location (lucky-collection-children-location)}]
Collection [b {:name "B", :location (collection/children-location a)}]]
(perms/grant-collection-readwrite-permissions! group collection/root-collection)
@@ -1592,7 +1295,7 @@
(expect
#{"/collection/A/read/"
"/collection/B/read/"}
- (tt/with-temp* [PermissionsGroup [group]
+ (mt/with-temp* [PermissionsGroup [group]
Collection [a {:name "A", :location (lucky-collection-children-location)}]
Collection [b {:name "B", :location (collection/children-location collection/root-collection)}]]
(perms/grant-collection-read-permissions! group b)
@@ -1608,7 +1311,7 @@
(expect
#{"/collection/B/"
"/collection/C/"}
- (tt/with-temp* [PermissionsGroup [group]
+ (mt/with-temp* [PermissionsGroup [group]
Collection [a {:name "A", :location (lucky-collection-children-location)}]
Collection [b {:name "B", :location (collection/children-location a)}]
Collection [c {:name "C", :location (collection/children-location collection/root-collection)}]]
@@ -1625,7 +1328,7 @@
#{"/collection/A/"
"/collection/B/"
"/collection/C/"}
- (tt/with-temp* [PermissionsGroup [group]
+ (mt/with-temp* [PermissionsGroup [group]
Collection [a {:name "A", :location (lucky-collection-children-location)}]
Collection [b {:name "B", :location (collection/children-location a)}]
Collection [c {:name "C", :location (collection/children-location collection/root-collection)}]]
@@ -1643,7 +1346,7 @@
;; Root Collection > A Root Collection
(expect
#{}
- (tt/with-temp* [PermissionsGroup [group]
+ (mt/with-temp* [PermissionsGroup [group]
Collection [a {:name "A", :location (collection/children-location collection/root-collection)}]]
(perms/grant-collection-readwrite-permissions! group a)
(db/update! Collection (u/get-id a) :location (lucky-collection-children-location))
@@ -1657,7 +1360,7 @@
;; Root Collection > A > B Root Collection > A
(expect
#{"/collection/A/"}
- (tt/with-temp* [PermissionsGroup [group]
+ (mt/with-temp* [PermissionsGroup [group]
Collection [a {:name "A", :location (collection/children-location collection/root-collection)}]
Collection [b {:name "B", :location (collection/children-location a)}]]
(perms/grant-collection-readwrite-permissions! group a)
@@ -1672,7 +1375,7 @@
;; Root Collection > B Root Collection
(expect
#{}
- (tt/with-temp* [PermissionsGroup [group]
+ (mt/with-temp* [PermissionsGroup [group]
Collection [a {:name "A", :location (lucky-collection-children-location)}]
Collection [b {:name "B", :location (collection/children-location collection/root-collection)}]]
(perms/grant-collection-readwrite-permissions! group b)
@@ -1687,7 +1390,7 @@
;; Root Collection > B > C Root Collection > B
(expect
#{"/collection/B/"}
- (tt/with-temp* [PermissionsGroup [group]
+ (mt/with-temp* [PermissionsGroup [group]
Collection [a {:name "A", :location (lucky-collection-children-location)}]
Collection [b {:name "B", :location (collection/children-location collection/root-collection)}]
Collection [c {:name "C", :location (collection/children-location b)}]]
@@ -1703,7 +1406,7 @@
;; Root Collection > B > C Root Collection
(expect
#{}
- (tt/with-temp* [PermissionsGroup [group]
+ (mt/with-temp* [PermissionsGroup [group]
Collection [a {:name "A", :location (lucky-collection-children-location)}]
Collection [b {:name "B", :location (collection/children-location collection/root-collection)}]
Collection [c {:name "C", :location (collection/children-location b)}]]
@@ -1711,3 +1414,66 @@
(perms/grant-collection-readwrite-permissions! group c)
(db/update! Collection (u/get-id b) :location (collection/children-location a))
(group->perms [a b c] group)))
+
+(deftest valid-location-path?-test
+ (doseq [[path expected] {nil false
+ "" false
+ "/" true
+ "/1" false
+ "/1/" true
+ "/1/2/" true
+ "/1/1/" false
+ "/1/2/1/" false
+ "/1/2/3/" true
+ "/abc/" false
+ "1" false
+ "/1.0/" false
+ "/-1/" false
+ 1 false
+ 1.0 false}]
+ (testing (pr-str path)
+ (is (= expected
+ (#'collection/valid-location-path? path))))))
+
+(deftest check-parent-collection-namespace-matches-test
+ (doseq [[parent-namespace child-namespace] [[nil "x"]
+ ["x" nil]
+ ["x" "y"]]]
+ (mt/with-temp Collection [parent-collection {:namespace parent-namespace}]
+ (testing (format "You should not be able to create a Collection in namespace %s inside a Collection in namespace %s"
+ (pr-str child-namespace) (pr-str parent-namespace))
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"Collection must be in the same namespace as its parent"
+ (db/insert! Collection
+ {:location (format "/%d/" (:id parent-collection))
+ :color "#F38630"
+ :name "Child Collection"
+ :namespace child-namespace}))))
+
+ (testing (format "You should not be able to move a Collection of namespace %s into a Collection of namespace %s"
+ (pr-str child-namespace) (pr-str parent-namespace))
+ (mt/with-temp Collection [collection-2 {:namespace child-namespace}]
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"Collection must be in the same namespace as its parent"
+ (collection/move-collection! collection-2 (format "/%d/" (:id parent-collection)))))))
+
+ (testing (format "You should not be able to change the namespace of a Collection from %s to %s"
+ (pr-str parent-namespace) (pr-str child-namespace))
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"You cannot move a Collection to a different namespace once it has been created"
+ (db/update! Collection (:id parent-collection) :namespace child-namespace)))))))
+
+(deftest check-special-collection-namespace-cannot-be-personal-collection
+ (testing "You should not be able to create a Personal Collection with a non-nil `:namespace`."
+ (mt/with-temp User [{user-id :id}]
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"Personal Collections must be in the default namespace"
+ (db/insert! Collection
+ {:color "#F38630"
+ :name "Personal Collection"
+ :namespace "x"
+ :personal_owner_id user-id}))))))
diff --git a/test/metabase/models/dashboard_test.clj b/test/metabase/models/dashboard_test.clj
index 661e27e6fa45245cc1f9be61ef7b43017123fc1f..8aa9ce628d67f6398d4678b548fa85fb0d5debcd 100644
--- a/test/metabase/models/dashboard_test.clj
+++ b/test/metabase/models/dashboard_test.clj
@@ -1,5 +1,9 @@
(ns metabase.models.dashboard-test
- (:require [expectations :refer [expect]]
+ (:require [clojure.test :refer :all]
+ [expectations :refer [expect]]
+ [metabase
+ [test :as mt]
+ [util :as u]]
[metabase.api.common :as api]
[metabase.automagic-dashboards.core :as magic]
[metabase.models
@@ -17,7 +21,6 @@
[data :refer :all]
[util :as tu]]
[metabase.test.data.users :as users]
- [metabase.util :as u]
[toucan.db :as db]
[toucan.util.test :as tt]))
@@ -167,18 +170,18 @@
(update (serialize-dashboard (Dashboard dashboard-id)) :cards check-ids)]))))
-;; test that a Dashboard's :public_uuid comes back if public sharing is enabled...
-(expect
- (tu/with-temporary-setting-values [enable-public-sharing true]
- (tt/with-temp Dashboard [dashboard {:public_uuid (str (java.util.UUID/randomUUID))}]
- (boolean (:public_uuid dashboard)))))
+(deftest public-sharing-test
+ (testing "test that a Dashboard's :public_uuid comes back if public sharing is enabled..."
+ (tu/with-temporary-setting-values [enable-public-sharing true]
+ (tt/with-temp Dashboard [dashboard {:public_uuid (str (java.util.UUID/randomUUID))}]
+ (is (schema= u/uuid-regex
+ (:public_uuid dashboard)))))
-;; ...but if public sharing is *disabled* it should come back as `nil`
-(expect
- nil
- (tu/with-temporary-setting-values [enable-public-sharing false]
- (tt/with-temp Dashboard [dashboard {:public_uuid (str (java.util.UUID/randomUUID))}]
- (:public_uuid dashboard))))
+ (testing "...but if public sharing is *disabled* it should come back as `nil`"
+ (tu/with-temporary-setting-values [enable-public-sharing false]
+ (tt/with-temp Dashboard [dashboard {:public_uuid (str (java.util.UUID/randomUUID))}]
+ (is (= nil
+ (:public_uuid dashboard))))))))
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -226,23 +229,36 @@
(binding [api/*current-user-permissions-set* (atom #{(perms/collection-readwrite-path collection)})]
(mi/can-write? dash))))
+(deftest transient-dashboards-test
+ (testing "test that we save a transient dashboard"
+ (tu/with-model-cleanup ['Card 'Dashboard 'DashboardCard 'Collection]
+ (binding [api/*current-user-id* (users/user->id :rasta)
+ api/*current-user-permissions-set* (-> :rasta
+ users/user->id
+ user/permissions-set
+ atom)]
+ (let [dashboard (magic/automagic-analysis (Table (id :venues)) {})
+ rastas-personal-collection (db/select-one-field :id 'Collection
+ :personal_owner_id api/*current-user-id*)
+ saved-dashboard (save-transient-dashboard! dashboard rastas-personal-collection)]
+ (is (= (db/count 'DashboardCard :dashboard_id (:id saved-dashboard))
+ (-> dashboard :ordered_cards count))))))))
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | Transient Dashboard Tests |
-;;; +----------------------------------------------------------------------------------------------------------------+
+(deftest validate-collection-namespace-test
+ (mt/with-temp Collection [{collection-id :id} {:namespace "currency"}]
+ (testing "Shouldn't be able to create a Dashboard in a non-normal Collection"
+ (let [dashboard-name (mt/random-name)]
+ (try
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"A Dashboard can only go in Collections in the \"default\" namespace"
+ (db/insert! Dashboard (assoc (tt/with-temp-defaults Dashboard) :collection_id collection-id, :name dashboard-name))))
+ (finally
+ (db/delete! Dashboard :name dashboard-name)))))
-;; test that we save a transient dashboard
-(expect
- (tu/with-model-cleanup ['Card 'Dashboard 'DashboardCard 'Collection]
- (binding [api/*current-user-id* (users/user->id :rasta)
- api/*current-user-permissions-set* (-> :rasta
- users/user->id
- user/permissions-set
- atom)]
- (let [dashboard (magic/automagic-analysis (Table (id :venues)) {})
- rastas-personal-collection (db/select-one-field :id 'Collection
- :personal_owner_id api/*current-user-id*)]
- (->> (save-transient-dashboard! dashboard rastas-personal-collection)
- :id
- (db/count 'DashboardCard :dashboard_id)
- (= (-> dashboard :ordered_cards count)))))))
+ (testing "Shouldn't be able to move a Dashboard to a non-normal Collection"
+ (mt/with-temp Dashboard [{card-id :id}]
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"A Dashboard can only go in Collections in the \"default\" namespace"
+ (db/update! Dashboard card-id {:collection_id collection-id})))))))
diff --git a/test/metabase/models/native_query_snippet_test.clj b/test/metabase/models/native_query_snippet_test.clj
index f06ae358926a1bf7078263a9b9e96f922915295a..ee8fa7c7fa586d779a2ae9da687dc3fbcf1c3ec4 100644
--- a/test/metabase/models/native_query_snippet_test.clj
+++ b/test/metabase/models/native_query_snippet_test.clj
@@ -1,7 +1,8 @@
(ns metabase.models.native-query-snippet-test
(:require [clojure.test :refer :all]
- [metabase.models.native-query-snippet :refer [NativeQuerySnippet]]
- [metabase.test :as mt]
+ [metabase
+ [models :refer [Collection NativeQuerySnippet]]
+ [test :as mt]]
[toucan.db :as db]))
(deftest disallow-updating-creator-id-test
@@ -13,3 +14,46 @@
(db/update! NativeQuerySnippet snippet-id :creator_id (mt/user->id :rasta))))
(is (= (mt/user->id :lucky)
(db/select-one-field :creator_id NativeQuerySnippet :id snippet-id))))))
+
+(deftest snippet-collection-test
+ (testing "Should be allowed to create snippets in a Collection in the :snippets namespace"
+ (mt/with-temp* [Collection [{collection-id :id} {:namespace "snippets"}]
+ NativeQuerySnippet [{snippet-id :id} {:collection_id collection-id}]]
+ (is (= collection-id
+ (db/select-one-field :collection_id NativeQuerySnippet :id snippet-id)))))
+
+ (doseq [[source dest] [[nil "snippets"]
+ ["snippets" "snippets"]
+ ["snippets" nil]]]
+ (testing (format "Should be allowed to move snippets from %s to %s"
+ (if source "a :snippets Collection" "no Collection")
+ (if dest "a :snippets Collection" "no Collection"))
+ (mt/with-temp* [Collection [{source-collection-id :id} {:namespace source}]
+ Collection [{dest-collection-id :id} {:namespace dest}]
+ NativeQuerySnippet [{snippet-id :id} (when source
+ {:collection_id source-collection-id})]]
+ (db/update! NativeQuerySnippet snippet-id :collection_id (when dest dest-collection-id))
+ (is (= (when dest dest-collection-id)
+ (db/select-one-field :collection_id NativeQuerySnippet :id snippet-id))))))
+
+ (doseq [collection-namespace [nil "x"]]
+ (testing (format "Should *not* be allowed to create snippets in a Collection in the %s namespace"
+ (pr-str collection-namespace))
+ (mt/with-temp Collection [{collection-id :id} {:namespace collection-namespace}]
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"A NativeQuerySnippet can only go in Collections in the :snippets namespace"
+ (db/insert! NativeQuerySnippet
+ {:name (mt/random-name)
+ :content "1 = 1"
+ :creator_id (mt/user->id :rasta)
+ :collection_id collection-id})))))
+
+ (testing (format "Should *not* be allowed to move snippets into a Collection in the namespace %s" (pr-str collection-namespace))
+ (mt/with-temp* [Collection [{source-collection-id :id} {:namespace "snippets"}]
+ NativeQuerySnippet [{snippet-id :id} {:collection_id source-collection-id}]
+ Collection [{dest-collection-id :id} {:namespace collection-namespace}]]
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"A NativeQuerySnippet can only go in Collections in the :snippets namespace"
+ (db/update! NativeQuerySnippet snippet-id :collection_id dest-collection-id)))))))
diff --git a/test/metabase/models/pulse_test.clj b/test/metabase/models/pulse_test.clj
index b8f85f280ef169f1848e2c731e03bc34ef518ad8..6303b15e1f3b1a7b063025bc8f35f0c7468fcdf3 100644
--- a/test/metabase/models/pulse_test.clj
+++ b/test/metabase/models/pulse_test.clj
@@ -1,6 +1,6 @@
(ns metabase.models.pulse-test
(:require [clojure.test :refer :all]
- [expectations :refer :all]
+ [expectations :refer [expect]]
[medley.core :as m]
[metabase
[test :as mt]
@@ -128,10 +128,10 @@
(dissoc (user-details :rasta) :is_superuser :is_qbnewb)]})
(mt/with-temp Pulse [{:keys [id]}]
(update-notification-channels! {:id id} [{:enabled true
- :channel_type :email
- :schedule_type :daily
- :schedule_hour 4
- :recipients [{:email "foo@bar.com"} {:id (user->id :rasta)}]}])
+ :channel_type :email
+ :schedule_type :daily
+ :schedule_hour 4
+ :recipients [{:email "foo@bar.com"} {:id (user->id :rasta)}]}])
(-> (PulseChannel :pulse_id id)
(hydrate :recipients)
(dissoc :id :pulse_id :created_at :updated_at)
@@ -258,3 +258,22 @@
(with-pulse-in-collection [db _ pulse]
(binding [api/*current-user-permissions-set* (atom #{(perms/object-path (u/get-id db))})]
(mi/can-read? pulse))))
+
+(deftest validate-collection-namespace-test
+ (mt/with-temp Collection [{collection-id :id} {:namespace "currency"}]
+ (testing "Shouldn't be able to create a Pulse in a non-normal Collection"
+ (let [pulse-name (mt/random-name)]
+ (try
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"A Pulse can only go in Collections in the \"default\" namespace"
+ (db/insert! Pulse (assoc (tt/with-temp-defaults Pulse) :collection_id collection-id, :name pulse-name))))
+ (finally
+ (db/delete! Pulse :name pulse-name)))))
+
+ (testing "Shouldn't be able to move a Pulse to a non-normal Collection"
+ (mt/with-temp Pulse [{card-id :id}]
+ (is (thrown-with-msg?
+ clojure.lang.ExceptionInfo
+ #"A Pulse can only go in Collections in the \"default\" namespace"
+ (db/update! Pulse card-id {:collection_id collection-id})))))))
diff --git a/test/metabase/sync_database/analyze_test.clj b/test/metabase/sync_database/analyze_test.clj
index ea4529cdbc7f2db777534e156645e9c71af9a9cc..faa7a4873f4c1a194d43ec0b143bbf08a524ddbe 100644
--- a/test/metabase/sync_database/analyze_test.clj
+++ b/test/metabase/sync_database/analyze_test.clj
@@ -17,9 +17,6 @@
(defn- classified-special-type [values]
(let [field (field/map->FieldInstance {:base_type :type/Text})]
- (println "RESULT =" (classify-text-fingerprint/infer-special-type
- field
- (transduce identity (fingerprinters/fingerprinter field) values)))
(:special_type (classify-text-fingerprint/infer-special-type
field
(transduce identity (fingerprinters/fingerprinter field) values)))))
diff --git a/test/metabase/test/util.clj b/test/metabase/test/util.clj
index 4146c5b6d4df04daf5c023d62a47b4e8d487f638..b3e03dcf6454609752e23f90b69f9ba9db2f3880 100644
--- a/test/metabase/test/util.clj
+++ b/test/metabase/test/util.clj
@@ -166,7 +166,9 @@
(u/strict-extend (class NativeQuerySnippet)
tt/WithTempDefaults
- {:with-temp-defaults (fn [_] {:creator_id (user-id :crowberto)})})
+ {:with-temp-defaults (fn [_] {:creator_id (user-id :crowberto)
+ :name (random-name)
+ :content "1 = 1"})})
(u/strict-extend (class PermissionsGroup)
tt/WithTempDefaults
@@ -575,7 +577,7 @@
(defn do-with-model-cleanup [model-seq f]
(try
- (testing (str (pr-str (cons 'with-model-cleanup model-seq)) "\n")
+ (testing (str "\n" (pr-str (cons 'with-model-cleanup (map name model-seq))) "\n")
(f))
(finally
(doseq [model model-seq]