Allow setting Snippet collection_id from API (#12848)

src/metabase/api/native_query_snippet.clj

@@ -41,17 +41,19 @@
 
 (api/defendpoint POST "/"
   "Create a new `NativeQuerySnippet`."
-  [:as {{:keys [content description name]} :body}]
-  {content     s/Str
-   description (s/maybe s/Str)
-   name        snippet/NativeQuerySnippetName}
+  [:as {{:keys [content description name collection_id]} :body}]
+  {content       s/Str
+   description   (s/maybe s/Str)
+   name          snippet/NativeQuerySnippetName
+   collection_id (s/maybe su/IntGreaterThanZero)}
   (check-snippet-name-is-unique name)
   (api/check-500
    (db/insert! NativeQuerySnippet
-     {:content     content
-      :creator_id  api/*current-user-id*
-      :description description
-      :name        name})))
+     {:content       content
+      :creator_id    api/*current-user-id*
+      :description   description
+      :name          name
+      :collection_id collection_id})))
 
 (defn- write-check-and-update-snippet!
   "Check whether current user has write permissions, then update NativeQuerySnippet with values in `body`.  Returns
@@ -59,7 +61,7 @@
   [id body]
   (let [snippet     (api/write-check NativeQuerySnippet id)
         body-fields (u/select-keys-when body
-                      :present #{:description}
+                      :present #{:description :collection_id}
                       :non-nil #{:archived :content :name})
         [changes]   (data/diff body-fields snippet)]
     (when (seq changes)
@@ -70,11 +72,12 @@
 
 (api/defendpoint PUT "/:id"
   "Update an existing `NativeQuerySnippet`."
-  [id :as {{:keys [archived content description name] :as body} :body}]
-  {archived    (s/maybe s/Bool)
-   content     (s/maybe s/Str)
-   description (s/maybe s/Str)
-   name        (s/maybe snippet/NativeQuerySnippetName)}
+  [id :as {{:keys [archived content description name collection_id] :as body} :body}]
+  {archived      (s/maybe s/Bool)
+   content       (s/maybe s/Str)
+   description   (s/maybe s/Str)
+   name          (s/maybe snippet/NativeQuerySnippetName)
+   collection_id (s/maybe su/IntGreaterThanZero)}
   (write-check-and-update-snippet! id body))
 
 

src/metabase/models/card.clj

@@ -119,7 +119,7 @@
                                 (:database query))))))
     ;; make sure this Card doesn't have circular source query references
     (check-for-circular-source-query-references card)
-    (collection/check-collection-namespace card)))
+    (collection/check-collection-namespace Card (:collection_id card))))
 
 (defn- post-insert [card]
   ;; if this Card has any native template tag parameters we need to update FieldValues for any Fields that are
@@ -154,7 +154,7 @@
     ;; 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))
-    (collection/check-collection-namespace card)))
+    (collection/check-collection-namespace Card (:collection_id card))))
 
 ;; Cards don't normally get deleted (they get archived instead) so this mostly affects tests
 (defn- pre-delete [{:keys [id]}]

src/metabase/models/collection.clj

@@ -966,14 +966,18 @@
   `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}]
+    (check-collection-namespace Card new-collection-id)"
+  [model collection-id]
   (when collection-id
-    (let [collection-namespace (keyword (db/select-one-field :namespace 'Collection :id collection-id))
-          allowed-namespaces   (allowed-namespaces object)]
+    (let [collection           (or (db/select-one [Collection :namespace] :id collection-id)
+                                   (let [msg (tru "Collection does not exist.")]
+                                     (throw (ex-info msg {:status-code 404
+                                                          :errors      {:collection_id msg}}))))
+          collection-namespace (keyword (:namespace collection))
+          allowed-namespaces   (allowed-namespaces model)]
       (when-not (contains? allowed-namespaces collection-namespace)
         (let [msg (tru "A {0} can only go in Collections in the {1} namespace."
-                       (name object)
+                       (name model)
                        (str/join (format " %s " (tru "or")) (map #(pr-str (or % (tru "default")))
                                                                  allowed-namespaces)))]
           (throw (ex-info msg {:status-code          400

src/metabase/models/dashboard.clj

@@ -58,11 +58,11 @@
   (let [defaults  {:parameters []}
         dashboard (merge defaults dashboard)]
     (u/prog1 dashboard
-      (collection/check-collection-namespace (map->DashboardInstance dashboard)))))
+      (collection/check-collection-namespace Dashboard (:collection_id dashboard)))))
 
 (defn- pre-update [dashboard]
   (u/prog1 dashboard
-    (collection/check-collection-namespace dashboard)))
+    (collection/check-collection-namespace Dashboard (:collection_id dashboard))))
 
 (u/strict-extend (class Dashboard)
   models/IModel

src/metabase/models/native_query_snippet.clj

@@ -25,7 +25,7 @@
 
 (defn- pre-insert [snippet]
   (u/prog1 snippet
-    (collection/check-collection-namespace snippet)))
+    (collection/check-collection-namespace NativeQuerySnippet (:collection_id snippet))))
 
 (defn- pre-update [{:keys [creator_id id], :as updates}]
   (u/prog1 updates
@@ -33,7 +33,7 @@
     (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.")))))
-    (collection/check-collection-namespace updates)))
+    (collection/check-collection-namespace NativeQuerySnippet (:collection_id updates))))
 
 (u/strict-extend (class NativeQuerySnippet)
   models/IModel

src/metabase/models/pulse.clj

@@ -47,11 +47,11 @@
 
 (defn- pre-insert [notification]
   (u/prog1 notification
-    (collection/check-collection-namespace notification)))
+    (collection/check-collection-namespace Pulse (:collection_id notification))))
 
 (defn- pre-update [updates]
   (u/prog1 updates
-    (collection/check-collection-namespace updates)))
+    (collection/check-collection-namespace Pulse (:collection_id updates))))
 
 (defn- alert->card
   "Return the Card associated with an Alert, fetching it if needed, for permissions-checking purposes."

test/metabase/api/native_query_snippet_test.clj

@@ -3,11 +3,14 @@
   (:require [clojure
              [string :as str]
              [test :refer :all]]
+            [metabase
+             [models :refer [Collection]]
+             [test :as mt]]
             [metabase.models.native-query-snippet :refer [NativeQuerySnippet]]
-            [metabase.test :as mt]
             [metabase.util.schema :as su]
             [schema.core :as s]
-            [toucan.db :as db]))
+            [toucan.db :as db]
+            [toucan.util.test :as tt]))
 
 (def ^:private test-snippet-fields [:content :creator_id :description :name])
 
@@ -104,12 +107,44 @@
       (finally
         (db/delete! NativeQuerySnippet :name "test-snippet")))))
 
+(deftest create-snippet-in-collection-test
+  (testing "POST /api/native-query-snippet"
+    (testing "\nShould be able to create a Snippet in a Collection"
+      (letfn [(create! [expected-status-code collection-id]
+                (try
+                  (let [response ((mt/user->client :rasta) :post expected-status-code (snippet-url)
+                                  {:name "test-snippet", :description "Just null", :content "NULL", :collection_id collection-id})]
+                    {:response response
+                     :db       (some-> (:id response) NativeQuerySnippet)})
+                  (finally
+                    (db/delete! NativeQuerySnippet :name "test-snippet"))))]
+        (mt/with-temp Collection [{collection-id :id} {:namespace "snippets"}]
+          (let [{:keys [response db]} (create! 200 collection-id)]
+            (testing "\nAPI response"
+              (is (= {:name "test-snippet", :collection_id collection-id}
+                     (select-keys response [:name :collection_id]))))
+            (testing "\nobject in application DB"
+              (is (schema= {:collection_id (s/eq collection-id)
+                            s/Keyword      s/Any}
+                           db)))))
+
+        (testing "\nShould throw an error if the Collection isn't in the 'snippets' namespace"
+          (mt/with-temp Collection [{collection-id :id}]
+            (is (= {:errors               {:collection_id "A NativeQuerySnippet can only go in Collections in the :snippets namespace."}
+                    :allowed-namespaces   ["snippets"]
+                    :collection-namespace nil}
+                   (:response (create! 400 collection-id))))))
+
+        (testing "\nShould throw an error if Collection does not exist"
+          (is (= {:errors {:collection_id "Collection does not exist."}}
+                 (:response (create! 404 Integer/MAX_VALUE)))))))))
+
 (deftest update-snippet-api-test
   (testing "PUT /api/native-query-snippet/:id"
     (mt/with-temp NativeQuerySnippet [snippet {:content "-- SQL comment here"
                                                :name    "comment"}]
       (testing "update stores updated snippet"
-        (doseq [[message user] {"admin user should be able to update" :crowberto
+        (doseq [[message user] {"admin user should be able to update"     :crowberto
                                 "non-admin user should be able to update" :rasta}]
           (testing message
             (let [updated-desc    "Updated description."
@@ -136,3 +171,35 @@
           ((mt/user->client :crowberto) :put 200 (snippet-url (:id snippet)) {:creator_id (mt/user->id :rasta)})
           (is (= (mt/user->id :lucky)
                  (db/select-one-field :creator_id NativeQuerySnippet :id (:id snippet)))))))))
+
+(deftest update-snippet-collection-test
+  (testing "PUT /api/native-query-snippet/:id"
+    (testing "\nChange collection_id"
+      (tt/with-temp* [Collection [{c1-id :id, :as collection-1} {:name "a Collection", :namespace "snippets"}]
+                      Collection [{c2-id :id, :as collection-2} {:name "another Collection", :namespace "snippets"}]]
+        (let [no-collection {:name "no Collection"}]
+          (doseq [[source dest] [[no-collection collection-1]
+                                 [collection-1 collection-2]
+                                 [collection-1 no-collection]]]
+            (testing (format "\nShould be able to move a Snippet from %s to %s" (:name source) (:name dest))
+              (tt/with-temp NativeQuerySnippet [{snippet-id :id} {:collection_id (:id source)}]
+                (testing "\nresponse"
+                  (is (= {:collection_id (:id dest)}
+                         (-> ((mt/user->client :rasta) :put 200 (snippet-url snippet-id) {:collection_id (:id dest)})
+                             (select-keys [:collection_id :errors])))))
+                (testing "\nvalue in app DB"
+                  (is (= (:id dest)
+                         (db/select-one-field :collection_id NativeQuerySnippet :id snippet-id)))))))))
+
+      (testing "\nShould throw an error if you try to move it to a Collection not in the 'snippets' namespace"
+        (tt/with-temp* [Collection         [{collection-id :id}]
+                        NativeQuerySnippet [{snippet-id :id}]]
+          (is (= {:errors               {:collection_id "A NativeQuerySnippet can only go in Collections in the :snippets namespace."}
+                  :allowed-namespaces   ["snippets"]
+                  :collection-namespace nil}
+                 ((mt/user->client :rasta) :put 400 (snippet-url snippet-id) {:collection_id collection-id})))))
+
+      (testing "\nShould throw an error if Collection does not exist"
+        (tt/with-temp NativeQuerySnippet [{snippet-id :id}]
+          (is (= {:errors {:collection_id "Collection does not exist."}}
+                 ((mt/user->client :rasta) :put 404 (snippet-url snippet-id) {:collection_id Integer/MAX_VALUE}))))))))

test/metabase/models/collection_test.clj

@@ -1477,3 +1477,26 @@
               :name              "Personal Collection"
               :namespace         "x"
               :personal_owner_id user-id}))))))
+
+(deftest check-collection-namespace-test
+  (testing "check-collection-namespace"
+    (testing "Should succeed if namespace is allowed"
+      (mt/with-temp* [Card       [{card-id :id}]
+                      Collection [{collection-id :id}]]
+        (is (= nil
+               (collection/check-collection-namespace Card collection-id)))))
+
+    (testing "Should throw exception if namespace is not allowed"
+      (mt/with-temp* [Card       [{card-id :id}]
+                      Collection [{collection-id :id} {:namespace "x"}]]
+        (is (thrown-with-msg?
+             clojure.lang.ExceptionInfo
+             #"A Card can only go in Collections in the \"default\" namespace"
+             (collection/check-collection-namespace Card collection-id)))))
+
+    (testing "Should throw exception if Collection does not exist"
+      (mt/with-temp Card [{card-id :id}]
+        (is (thrown-with-msg?
+             clojure.lang.ExceptionInfo
+             #"Collection does not exist"
+             (collection/check-collection-namespace Card Integer/MAX_VALUE)))))))

test/metabase/test/util.clj

@@ -114,123 +114,154 @@
 
 (defn- rasta-id [] (user-id :rasta))
 
-(u/strict-extend (class Card)
-  tt/WithTempDefaults
-  {:with-temp-defaults (fn [_] {:creator_id             (rasta-id)
-                                :dataset_query          {}
-                                :display                :table
-                                :name                   (random-name)
-                                :visualization_settings {}})})
-
-(u/strict-extend (class Collection)
-  tt/WithTempDefaults
-  {:with-temp-defaults (fn [_] {:name  (random-name)
-                                :color "#ABCDEF"})})
-
-(u/strict-extend (class Dashboard)
-  tt/WithTempDefaults
-  {:with-temp-defaults (fn [_] {:creator_id   (rasta-id)
-                                :name         (random-name)})})
-
-(u/strict-extend (class DashboardCardSeries)
-  tt/WithTempDefaults
-  {:with-temp-defaults (constantly {:position 0})})
-
-(u/strict-extend (class Database)
-  tt/WithTempDefaults
-  {:with-temp-defaults (fn [_] {:details   {}
-                                :engine    :h2
-                                :is_sample false
-                                :name      (random-name)})})
-
-(u/strict-extend (class Dimension)
-  tt/WithTempDefaults
-  {:with-temp-defaults (fn [_] {:name (random-name)
-                                :type "internal"})})
-
-(u/strict-extend (class Field)
-  tt/WithTempDefaults
-  {:with-temp-defaults (fn [_] {:database_type "VARCHAR"
-                                :base_type     :type/Text
-                                :name          (random-name)
-                                :position      1
-                                :table_id      (data/id :checkins)})})
-
-(u/strict-extend (class Metric)
-  tt/WithTempDefaults
-  {:with-temp-defaults (fn [_] {:creator_id  (rasta-id)
-                                :definition  {}
-                                :description "Lookin' for a blueberry"
-                                :name        "Toucans in the rainforest"
-                                :table_id    (data/id :checkins)})})
-
-(u/strict-extend (class NativeQuerySnippet)
-  tt/WithTempDefaults
-  {:with-temp-defaults (fn [_] {:creator_id (user-id :crowberto)
-                                :name       (random-name)
-                                :content    "1 = 1"})})
-
-(u/strict-extend (class PermissionsGroup)
-  tt/WithTempDefaults
-  {:with-temp-defaults (fn [_] {:name (random-name)})})
-
-(u/strict-extend (class Pulse)
-  tt/WithTempDefaults
-  {:with-temp-defaults (fn [_] {:creator_id (rasta-id)
-                                :name       (random-name)})})
-
-(u/strict-extend (class PulseCard)
-  tt/WithTempDefaults
-  {:with-temp-defaults (fn [_] {:position    0
-                                :include_csv false
-                                :include_xls false})})
-
-(u/strict-extend (class PulseChannel)
-  tt/WithTempDefaults
-  {:with-temp-defaults (constantly {:channel_type  :email
-                                    :details       {}
-                                    :schedule_type :daily
-                                    :schedule_hour 15})})
-
-(u/strict-extend (class Revision)
-  tt/WithTempDefaults
-  {:with-temp-defaults (fn [_] {:user_id      (rasta-id)
-                                :is_creation  false
-                                :is_reversion false})})
-
-(u/strict-extend (class Segment)
-  tt/WithTempDefaults
-  {:with-temp-defaults (fn [_] {:creator_id (rasta-id)
-                                :definition  {}
-                                :description "Lookin' for a blueberry"
-                                :name        "Toucans in the rainforest"
-                                :table_id    (data/id :checkins)})})
-
-;; TODO - `with-temp` doesn't return `Sessions`, probably because their ID is a string?
-
-(u/strict-extend (class Table)
-  tt/WithTempDefaults
-  {:with-temp-defaults (fn [_] {:db_id  (data/id)
-                                :active true
-                                :name   (random-name)})})
-
-(u/strict-extend (class TaskHistory)
-  tt/WithTempDefaults
-  {:with-temp-defaults (fn [_]
-                         (let [started (t/zoned-date-time)
-                               ended   (t/plus started (t/millis 10))]
-                           {:db_id      (data/id)
-                            :task       (random-name)
-                            :started_at started
-                            :ended_at   ended
-                            :duration   (.toMillis (t/duration started ended))}))})
-
-(u/strict-extend (class User)
-  tt/WithTempDefaults
-  {:with-temp-defaults (fn [_] {:first_name (random-name)
-                                :last_name  (random-name)
-                                :email      (random-email)
-                                :password   (random-name)})})
+(defn- set-with-temp-defaults! []
+  (extend (class Card)
+    tt/WithTempDefaults
+    {:with-temp-defaults (fn [_] {:creator_id             (rasta-id)
+                                  :dataset_query          {}
+                                  :display                :table
+                                  :name                   (random-name)
+                                  :visualization_settings {}})})
+
+  (extend (class Collection)
+    tt/WithTempDefaults
+    {:with-temp-defaults (fn [_] {:name  (random-name)
+                                  :color "#ABCDEF"})})
+
+  (extend (class Dashboard)
+    tt/WithTempDefaults
+    {:with-temp-defaults (fn [_] {:creator_id   (rasta-id)
+                                  :name         (random-name)})})
+
+  (extend (class DashboardCardSeries)
+    tt/WithTempDefaults
+    {:with-temp-defaults (constantly {:position 0})})
+
+  (extend (class Database)
+    tt/WithTempDefaults
+    {:with-temp-defaults (fn [_] {:details   {}
+                                  :engine    :h2
+                                  :is_sample false
+                                  :name      (random-name)})})
+
+  (extend (class Dimension)
+    tt/WithTempDefaults
+    {:with-temp-defaults (fn [_] {:name (random-name)
+                                  :type "internal"})})
+
+  (extend (class Field)
+    tt/WithTempDefaults
+    {:with-temp-defaults (fn [_] {:database_type "VARCHAR"
+                                  :base_type     :type/Text
+                                  :name          (random-name)
+                                  :position      1
+                                  :table_id      (data/id :checkins)})})
+
+  (extend (class Metric)
+    tt/WithTempDefaults
+    {:with-temp-defaults (fn [_] {:creator_id  (rasta-id)
+                                  :definition  {}
+                                  :description "Lookin' for a blueberry"
+                                  :name        "Toucans in the rainforest"
+                                  :table_id    (data/id :checkins)})})
+
+  (extend (class NativeQuerySnippet)
+    tt/WithTempDefaults
+    {:with-temp-defaults (fn [_] {:creator_id (user-id :crowberto)
+                                  :name       (random-name)
+                                  :content    "1 = 1"})})
+
+  (extend (class PermissionsGroup)
+    tt/WithTempDefaults
+    {:with-temp-defaults (fn [_] {:name (random-name)})})
+
+  (extend (class Pulse)
+    tt/WithTempDefaults
+    {:with-temp-defaults (fn [_] {:creator_id (rasta-id)
+                                  :name       (random-name)})})
+
+  (extend (class PulseCard)
+    tt/WithTempDefaults
+    {:with-temp-defaults (fn [_] {:position    0
+                                  :include_csv false
+                                  :include_xls false})})
+
+  (extend (class PulseChannel)
+    tt/WithTempDefaults
+    {:with-temp-defaults (constantly {:channel_type  :email
+                                      :details       {}
+                                      :schedule_type :daily
+                                      :schedule_hour 15})})
+
+  (extend (class Revision)
+    tt/WithTempDefaults
+    {:with-temp-defaults (fn [_] {:user_id      (rasta-id)
+                                  :is_creation  false
+                                  :is_reversion false})})
+
+  (extend (class Segment)
+    tt/WithTempDefaults
+    {:with-temp-defaults (fn [_] {:creator_id (rasta-id)
+                                  :definition  {}
+                                  :description "Lookin' for a blueberry"
+                                  :name        "Toucans in the rainforest"
+                                  :table_id    (data/id :checkins)})})
+
+  ;; TODO - `with-temp` doesn't return `Sessions`, probably because their ID is a string?
+
+  (extend (class Table)
+    tt/WithTempDefaults
+    {:with-temp-defaults (fn [_] {:db_id  (data/id)
+                                  :active true
+                                  :name   (random-name)})})
+
+  (extend (class TaskHistory)
+    tt/WithTempDefaults
+    {:with-temp-defaults (fn [_]
+                           (let [started (t/zoned-date-time)
+                                 ended   (t/plus started (t/millis 10))]
+                             {:db_id      (data/id)
+                              :task       (random-name)
+                              :started_at started
+                              :ended_at   ended
+                              :duration   (.toMillis (t/duration started ended))}))})
+
+  (extend (class User)
+    tt/WithTempDefaults
+    {:with-temp-defaults (fn [_] {:first_name (random-name)
+                                  :last_name  (random-name)
+                                  :email      (random-email)
+                                  :password   (random-name)})}))
+
+(set-with-temp-defaults!)
+
+;; if any of the models get redefined, reload the `with-temp-defaults` so they apply to the new version of the model
+(doseq [model-var [#'Card
+                   #'Collection
+                   #'Dashboard
+                   #'DashboardCardSeries
+                   #'Database
+                   #'Dimension
+                   #'Field
+                   #'Metric
+                   #'NativeQuerySnippet
+                   #'Permissions
+                   #'PermissionsGroup
+                   #'Pulse
+                   #'PulseCard
+                   #'PulseChannel
+                   #'Revision
+                   #'Segment
+                   #'Table
+                   #'TaskHistory
+                   #'User]]
+  (remove-watch model-var ::reload)
+  (add-watch
+   model-var
+   ::reload
+   (fn [_ reference _ _]
+     (println (format "%s changed, reloading with-temp-defaults" model-var))
+     #_(set-with-temp-defaults!))))
 
 
 ;;; ------------------------------------------------- Other Util Fns -------------------------------------------------