Merge branch 'saved-questions' of github.com:metabase/metabase into saved-questions

src/metabase/api/card.clj

@@ -31,19 +31,19 @@
 (defn- hydrate-favorites
   "Efficiently add `favorite` status for a large collection of `Cards`."
   [cards]
-  (let [favorite-card-ids (set (sel :many :field [CardFavorite :card_id], :owner_id *current-user-id*, :id [in (map :id cards)]))]
+  (let [favorite-card-ids (set (sel :many :field [CardFavorite :card_id], :owner_id *current-user-id*, :card_id [in (map :id cards)]))]
     (for [card cards]
       (assoc card :favorite (contains? favorite-card-ids (:id card))))))
 
 (defn- cards:all
   "Return all `Cards`."
   []
-  (sel :many Card, (k/order :name :ASC)))
+  (sel :many Card, :archived false, (k/order :name :ASC)))
 
 (defn- cards:mine
   "Return all `Cards` created by current user."
   []
-  (sel :many Card, :creator_id *current-user-id*, (k/order :name :ASC)))
+  (sel :many Card, :creator_id *current-user-id*, :archived false, (k/order :name :ASC)))
 
 (defn- cards:fav
   "Return all `Cards` favorited by the current user."
@@ -51,24 +51,25 @@
   (->> (hydrate (sel :many [CardFavorite :card_id], :owner_id *current-user-id*)
                 :card)
        (map :card)
+       (filter (complement :archived))
        (sort-by :name)))
 
 (defn- cards:database
   "Return all `Cards` belonging to `Database` with DATABASE-ID."
   [database-id]
-  (sel :many Card (k/order :name :ASC), :database_id database-id))
+  (sel :many Card (k/order :name :ASC), :database_id database-id, :archived false))
 
 (defn- cards:table
   "Return all `Cards` belonging to `Table` with TABLE-ID."
   [table-id]
-  (sel :many Card (k/order :name :ASC), :table_id table-id))
+  (sel :many Card (k/order :name :ASC), :table_id table-id, :archived false))
 
 (defn- cards-with-ids
-  "Return `Cards` with CARD-IDS.
+  "Return unarchived `Cards` with CARD-IDS.
    Make sure cards are returned in the same order as CARD-IDS`; `[in card-ids]` won't preserve the order."
   [card-ids]
   {:pre [(every? integer? card-ids)]}
-  (let [card-id->card (sel :many :field->obj [Card :id], :id [in card-ids])]
+  (let [card-id->card (sel :many :field->obj [Card :id], :id [in card-ids], :archived false)]
     (filter identity (map card-id->card card-ids))))
 
 (defn- cards:recent
@@ -112,8 +113,8 @@
    :popular  (u/drop-first-arg cards:popular)
    :archived (u/drop-first-arg cards:archived)})
 
-(defn- card-has-label? [label card]
-  (contains? (set (map :slug (:labels card))) label))
+(defn- card-has-label? [label-slug card]
+  (contains? (set (map :slug (:labels card))) label-slug))
 
 (defn- cards-for-filter-option [filter-option model-id label]
   (let [cards (-> ((filter-option->fn (or filter-option :all)) model-id)
@@ -173,7 +174,7 @@
 
 (defendpoint PUT "/:id"
   "Update a `Card`."
-  [id :as {{:keys [dataset_query description display name public_perms visualization_settings archived]} :body}]
+  [id :as {{:keys [dataset_query description display name public_perms visualization_settings archived], :as body} :body}]
   {name                   NonEmptyString
    public_perms           PublicPerms
    display                NonEmptyString

src/metabase/api/common.clj

@@ -91,8 +91,8 @@
    (when-not tst
      (throw (invalid-param-exception (str field-name) message)))))
 
-(defmacro checkp-with
-  "Check (TEST-FN VALUE), or throw an exception with STATUS-CODE (default is 400).
+(defn checkp-with
+  "Check (F VALUE), or throw an exception with STATUS-CODE (default is 400).
    SYMB is passed in order to give the user a relevant error message about which parameter was bad.
 
    Returns VALUE upon success.
@@ -100,20 +100,18 @@
     (checkp-with (partial? contains? {:all :mine}) f :all)
       -> :all
     (checkp-with (partial? contains {:all :mine}) f :bad)
-      -> ExceptionInfo: Invalid value ':bad' for 'f': test failed: (partial? contains? {:all :mine}
+      -> ExceptionInfo: Invalid value ':bad' for 'f': test failed: (partial? contains?) {:all :mine}
 
    You may optionally pass a MESSAGE to append to the exception upon failure;
    this will be used in place of the \"test failed: ...\" message.
 
    MESSAGE may be either a string or a pair like `[status-code message]`."
-  ([test-fn symb value message]
+  ([f symb value]
+   (checkp-with f symb value (str "test failed: " f)))
+  ([f symb value message]
    {:pre [(symbol? symb)]}
-   `(let [message# ~message
-          value# ~value]
-      (checkp (~test-fn value#) ~symb (format "Invalid value '%s' for '%s': %s" (str value#) ~symb message#))
-      value#))
-  ([test-fn symb value]
-   `(checkp-with ~test-fn ~symb ~value ~(str "test failed: " test-fn))))
+   (checkp (f value) symb (format "Invalid value '%s' for '%s': %s" (str value) symb message))
+   value))
 
 (defn checkp-contains?
   "Check that the VALUE of parameter SYMB is in VALID-VALUES, or throw a 400.
@@ -279,7 +277,7 @@
          (defn ~fn-name ~@(when docstr [docstr]) [~symbol-binding ~value-binding]
            {:pre [(symbol? ~symbol-binding)]}
            ~(if nillable?
-              `(when ~value-binding
+              `(when-not (nil? ~value-binding)
                  ~@body)
               `(do
                  ~@body)))

test/metabase/api/card_test.clj

@@ -5,6 +5,7 @@
             [metabase.http-client :refer :all]
             [metabase.driver.query-processor.expand :as ql]
             (metabase.models [card :refer [Card]]
+                             [card-favorite :refer [CardFavorite]]
                              [card-label :refer [CardLabel]]
                              [common :as common]
                              [database :refer [Database]]
@@ -100,6 +101,16 @@
   #{card-2-id card-3-id}
   (set (map :id ((user->client :rasta) :get 200 "card", :f :archived))))
 
+;;; Filter by `fav`
+(expect-with-temp [Card         [{card-id-1 :id}]
+                   Card         [{card-id-2 :id}]
+                   Card         [{card-id-3 :id}]
+                   CardFavorite [_ {:card_id card-id-1, :owner_id (user->id :rasta)}]
+                   CardFavorite [_ {:card_id card-id-2, :owner_id (user->id :crowberto)}]]
+  [{:id card-id-1, :favorite true}]
+  (for [card ((user->client :rasta) :get 200 "card", :f :fav)]
+    (select-keys card [:id :favorite])))
+
 ;;; Filter by labels
 (expect-with-temp [Card      [{card-1-id :id}]
                    Card      [{card-2-id :id}]
@@ -122,8 +133,8 @@
      :name                   card-name
      :creator_id             (user->id :rasta)
      :dataset_query          {:database database-id
-                              :type     :query
-                              :query    {:source-table table-id, :aggregation {:aggregation-type :count}}}
+                              :type     "query"
+                              :query    {:source-table table-id, :aggregation {:aggregation-type "count"}}}
      :display                "scalar"
      :visualization_settings {:global {:title nil}}
      :public_perms           0
@@ -192,14 +203,22 @@
          (sel :one :field [Card :name] :id card-id))]))
 
 
-;; TODO - can we update a card's `archived` status?
-
-
-
 (defmacro ^:private with-temp-card {:style/indent 1} [binding & body]
   `(with-temp Card ~binding
      ~@body))
 
+;; Can we update a Card's archived status?
+(expect
+  [false true false]
+  (with-temp-card [{:keys [id]}]
+    (let [archived?     (fn [] (:archived (Card id)))
+          set-archived! (fn [archived]
+                          ((user->client :rasta) :put 200 (str "card/" id) {:archived archived})
+                          (archived?))]
+      [(archived?)
+       (set-archived! true)
+       (set-archived! false)])))
+
 
 ;; ## DELETE /api/card/:id
 ;; Check that we can delete a card
@@ -258,7 +277,6 @@
 
 ;;; POST /api/card/:id/labels
 ;; Check that we can update card labels
-
 (expect-with-temp [Card  [{card-id :id}]
                    Label [{label-1-id :id} {:name "Toucan-Friendly"}]
                    Label [{label-2-id :id} {:name "Toucan-Unfriendly"}]]

test/metabase/api/common_test.clj

@@ -118,3 +118,11 @@
     (defendpoint GET "/:id" [id]
       {id Required}
       (->404 (sel :one Card :id id)))))
+
+
+;;; ------------------------------------------------------------ ANNOTATION TESTS ------------------------------------------------------------
+
+(expect true  (annotation:Boolean 'archived true))
+(expect false (annotation:Boolean 'archived false))
+(expect nil   (annotation:Boolean 'archived nil))
+(expect clojure.lang.ExceptionInfo (annotation:Boolean 'archived 1))