From d621c3f5f7a51cb7f3e42b8a28a1c74f60c07dd1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cam=20Sa=C3=BCl?= <cammsaul@gmail.com>
Date: Mon, 24 Apr 2017 17:04:37 -0700
Subject: [PATCH] Simplify API logic with new select-non-nil-keys and
select-keys-when fns
---
resources/migrations/000_migrations.yaml | 4 +-
src/metabase/api/card.clj | 21 ++--
src/metabase/api/common.clj | 3 +-
src/metabase/api/dashboard.clj | 33 ++++--
src/metabase/api/field.clj | 38 +++---
src/metabase/api/metric.clj | 22 ++--
src/metabase/api/segment.clj | 14 +--
src/metabase/models/dashboard.clj | 20 ----
src/metabase/models/metric.clj | 24 ++--
src/metabase/models/segment.clj | 15 +--
src/metabase/util.clj | 25 ++++
test/metabase/api/card_test.clj | 2 +-
test/metabase/api/metric_test.clj | 108 ++++++++---------
test/metabase/models/metric_test.clj | 111 ++++++++----------
test/metabase/permissions_collection_test.clj | 4 +-
test/metabase/util_test.clj | 12 ++
16 files changed, 214 insertions(+), 242 deletions(-)
diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index 6204cdf0f5b..36834b9eca2 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/resources/migrations/000_migrations.yaml
@@ -3570,14 +3570,16 @@ databaseChangeLog:
- changeSet:
id: 54
author: tlrobinson
+ validCheckSum: '7:695b12a78e897c62c21d41bfb04ab44b'
+ validCheckSum: '7:0857800db71a4757e7202aad4eaed48d'
changes:
- addColumn:
tableName: pulse
- remarks: 'Skip a scheduled Pulse if none of its questions have any results'
columns:
- column:
name: skip_if_empty
type: boolean
+ remarks: 'Skip a scheduled Pulse if none of its questions have any results'
defaultValueBoolean: false
constraints:
nullable: false
diff --git a/src/metabase/api/card.clj b/src/metabase/api/card.clj
index e27c8c855ce..1fb8cc4ad40 100644
--- a/src/metabase/api/card.clj
+++ b/src/metabase/api/card.clj
@@ -49,7 +49,7 @@
"Efficiently add `favorite` status for a large collection of `Cards`."
[cards]
(when (seq cards)
- (let [favorite-card-ids (set (db/select-field :card_id CardFavorite, :owner_id *current-user-id*, :card_id [:in (map :id cards)]))]
+ (let [favorite-card-ids (db/select-field :card_id CardFavorite, :owner_id *current-user-id*, :card_id [:in (map :id cards)])]
(for [card cards]
(assoc card :favorite (contains? favorite-card-ids (:id card)))))))
@@ -145,7 +145,6 @@
;; TODO - do we need to hydrate the cards' collections as well?
(defn- cards-for-filter-option [filter-option model-id label collection-slug]
- (println "collection-slug:" collection-slug) ; NOCOMMIT
(let [cards (-> ((filter-option->fn (or filter-option :all)) model-id)
(hydrate :creator :collection)
hydrate-labels
@@ -269,17 +268,10 @@
(check-superuser))
;; ok, now save the Card
(db/update! Card id
- (merge
- ;; `collection_id` and `description` can be `nil` (in order to unset them)
- (when (contains? body :collection_id)
- {:collection_id collection_id})
- (when (contains? body :description)
- {:description description})
- ;; other values should only be modified if they're passed in as non-nil
- (into {} (for [k [:dataset_query :display :name :visualization_settings :archived :enable_embedding :embedding_params]
- :let [v (k body)]
- :when (not (nil? v))]
- {k v}))))
+ ;; `collection_id` and `description` can be `nil` (in order to unset them). Other values should only be modified if they're passed in as non-nil
+ (u/select-keys-when body
+ :present #{:collection_id :description}
+ :non-nil #{:dataset_query :display :name :visualization_settings :archived :enable_embedding :embedding_params}))
(let [event (cond
;; card was archived
(and archived
@@ -294,9 +286,12 @@
(hydrate card :creator :dashboard_count :labels :can_write :collection))))
+;; TODO - Pretty sure this endpoint is not actually used any more, since Cards are supposed to get archived (via PUT /api/card/:id) instead of deleted.
+;; Should we remove this?
(defendpoint DELETE "/:id"
"Delete a `Card`."
[id]
+ (log/warn "DELETE /api/card/:id is deprecated. Instead of deleting a Card, you should change its `archived` value via PUT /api/card/:id.")
(let [card (write-check Card id)]
(db/delete! Card :id id)
(events/publish-event! :card-delete (assoc card :actor_id *current-user-id*)))
diff --git a/src/metabase/api/common.clj b/src/metabase/api/common.clj
index f1e2b8f9ddf..7c9b94d7a11 100644
--- a/src/metabase/api/common.clj
+++ b/src/metabase/api/common.clj
@@ -317,4 +317,5 @@
"Check that the OBJECT is not `:archived`, or throw a `404`. Returns OBJECT as-is if check passes."
[object]
(u/prog1 object
- (check-404 (not (:archived object)))))
+ (check (not (:archived object))
+ [404 "The object has been archived."])))
diff --git a/src/metabase/api/dashboard.clj b/src/metabase/api/dashboard.clj
index a80203a4e15..9f26f9a638e 100644
--- a/src/metabase/api/dashboard.clj
+++ b/src/metabase/api/dashboard.clj
@@ -18,11 +18,12 @@
(defn- dashboards-list [filter-option]
- (filter mi/can-read? (-> (db/select Dashboard {:where (case (or (keyword filter-option) :all)
- :all true
- :mine [:= :creator_id *current-user-id*])
- :order-by [:%lower.name]})
- (hydrate :creator))))
+ (as-> (db/select Dashboard {:where (case (or (keyword filter-option) :all)
+ :all true
+ :mine [:= :creator_id *current-user-id*])
+ :order-by [:%lower.name]}) <>
+ (hydrate <> :creator)
+ (filter mi/can-read? <>)))
(defendpoint GET "/"
"Get `Dashboards`. With filter option `f` (default `all`), restrict results as follows:
@@ -88,8 +89,15 @@
(not= embedding_params (:embedding_params dash))))
(check-embedding-enabled)
(check-superuser)))
- (check-500 (-> (assoc dashboard :id id)
- (dashboard/update-dashboard! *current-user-id*))))
+ (check-500
+ (db/update! Dashboard id
+ ;; description is allowed to be `nil`. Everything else must be non-nil
+ (u/select-keys-when dashboard
+ :present #{:description}
+ :non-nil #{:name :parameters :caveats :points_of_interest :show_in_getting_started :enable_embedding :embedding_params})))
+ ;; now publish an event and return the updated Dashboard
+ (u/prog1 (Dashboard id)
+ (events/publish-event! :dashboard-update (assoc <> :actor_id *current-user-id*))))
(defendpoint DELETE "/:id"
@@ -101,6 +109,7 @@
generic-204-no-content)
+;; TODO - param should be `card_id`, not `cardId` (fix here + on frontend at the same time)
(defendpoint POST "/:id/cards"
"Add a `Card` to a `Dashboard`."
[id :as {{:keys [cardId parameter_mappings series] :as dashboard-card} :body}]
@@ -123,11 +132,11 @@
(defendpoint PUT "/:id/cards"
"Update `Cards` on a `Dashboard`. Request body should have the form:
- {:cards [{:id ...
- :sizeX ...
- :sizeY ...
- :row ...
- :col ...
+ {:cards [{:id ...
+ :sizeX ...
+ :sizeY ...
+ :row ...
+ :col ...
:series [{:id 123
...}]} ...]}"
[id :as {{:keys [cards]} :body}]
diff --git a/src/metabase/api/field.clj b/src/metabase/api/field.clj
index 4729dd7fb1d..2f57419393c 100644
--- a/src/metabase/api/field.clj
+++ b/src/metabase/api/field.clj
@@ -38,27 +38,23 @@
points_of_interest (s/maybe su/NonBlankString)
special_type (s/maybe FieldType)
visibility_type (s/maybe FieldVisibilityType)}
- (let [field (write-check Field id)]
- (let [special_type (keyword (get body :special_type (:special_type field)))
- visibility_type (or visibility_type (:visibility_type field))
- ;; only let target field be set for :type/FK type fields, and if it's not in the payload then leave the current value
- fk-target-field-id (when (isa? special_type :type/FK)
- (get body :fk_target_field_id (:fk_target_field_id field)))]
- ;; validate that fk_target_field_id is a valid Field
- ;; TODO - we should also check that the Field is within the same database as our field
- (when fk-target-field-id
- (checkp (db/exists? Field :id fk-target-field-id)
- :fk_target_field_id "Invalid target field"))
- ;; everything checks out, now update the field
- (check-500 (db/update! Field id (merge {:caveats caveats
- :description description
- :fk_target_field_id fk_target_field_id
- :points_of_interest points_of_interest
- :special_type special_type
- :visibility_type visibility_type}
- (when display_name {:display_name display_name}))))
- ;; return updated field
- (Field id))))
+ (let [field (write-check Field id)
+ special_type (keyword (or special_type (:special_type field)))
+ ;; only let target field be set for :type/FK type fields, and if it's not in the payload then leave the current value
+ fk_target_field_id (when (isa? special_type :type/FK)
+ (or fk_target_field_id (:fk_target_field_id field)))]
+ ;; validate that fk_target_field_id is a valid Field
+ ;; TODO - we should also check that the Field is within the same database as our field
+ (when fk_target_field_id
+ (checkp (db/exists? Field :id fk_target_field_id)
+ :fk_target_field_id "Invalid target field"))
+ ;; everything checks out, now update the field
+ (check-500 (db/update! Field id
+ (u/select-keys-when (assoc body :fk_target_field_id fk_target_field_id)
+ :present #{:caveats :description :fk_target_field_id :points_of_interest :special_type :visibility_type}
+ :non-nil #{:display_name})))
+ ;; return updated field
+ (Field id)))
(defendpoint GET "/:id/summary"
"Get the count and distinct count of `Field` with ID."
diff --git a/src/metabase/api/metric.clj b/src/metabase/api/metric.clj
index fcee3655552..409ce273870 100644
--- a/src/metabase/api/metric.clj
+++ b/src/metabase/api/metric.clj
@@ -42,30 +42,24 @@
(defendpoint GET "/"
"Fetch *all* `Metrics`."
[id]
- (filter mi/can-read? (-> (db/select Metric, :is_active true, {:order-by [:%lower.name]})
- (hydrate :creator)
- add-db-ids)))
+ (as-> (db/select Metric, :is_active true, {:order-by [:%lower.name]}) <>
+ (hydrate <> :creator)
+ (add-db-ids <>)
+ (filter mi/can-read? <>)))
(defendpoint PUT "/:id"
"Update a `Metric` with ID."
- [id :as {{:keys [name description caveats points_of_interest how_is_this_calculated show_in_getting_started definition revision_message]} :body}]
+ [id :as {{:keys [definition name revision_message], :as body} :body}]
{name su/NonBlankString
revision_message su/NonBlankString
definition su/Map}
(check-superuser)
(write-check Metric id)
(metric/update-metric!
- {:id id
- :name name
- :description description
- :caveats caveats
- :points_of_interest points_of_interest
- :how_is_this_calculated how_is_this_calculated
- :show_in_getting_started show_in_getting_started
- :definition definition
- :revision_message revision_message}
- *current-user-id*))
+ (assoc (select-keys body #{:caveats :definition :description :how_is_this_calculated :name :points_of_interest :revision_message :show_in_getting_started})
+ :id id)
+ *current-user-id*))
(defendpoint PUT "/:id/important_fields"
"Update the important `Fields` for a `Metric` with ID.
diff --git a/src/metabase/api/segment.clj b/src/metabase/api/segment.clj
index f45f98007ae..aa60b0f9edf 100644
--- a/src/metabase/api/segment.clj
+++ b/src/metabase/api/segment.clj
@@ -39,22 +39,16 @@
(defendpoint PUT "/:id"
"Update a `Segment` with ID."
- [id :as {{:keys [name description caveats points_of_interest show_in_getting_started definition revision_message]} :body}]
+ [id :as {{:keys [name definition revision_message], :as body} :body}]
{name su/NonBlankString
revision_message su/NonBlankString
definition su/Map}
(check-superuser)
(write-check Segment id)
(segment/update-segment!
- {:id id
- :name name
- :description description
- :caveats caveats
- :points_of_interest points_of_interest
- :show_in_getting_started show_in_getting_started
- :definition definition
- :revision_message revision_message}
- *current-user-id*))
+ (assoc (select-keys body #{:name :description :caveats :points_of_interest :show_in_getting_started :definition :revision_message})
+ :id id)
+ *current-user-id*))
(defendpoint DELETE "/:id"
diff --git a/src/metabase/models/dashboard.clj b/src/metabase/models/dashboard.clj
index 660819acfda..26dbf0b8180 100644
--- a/src/metabase/models/dashboard.clj
+++ b/src/metabase/models/dashboard.clj
@@ -89,26 +89,6 @@
(events/publish-event! :dashboard-create)))
-
-(defn update-dashboard!
- "Update a `Dashboard`"
- [dashboard user-id]
- {:pre [(map? dashboard)
- (u/maybe? u/sequence-of-maps? (:parameters dashboard))
- (integer? user-id)]}
- (db/update! Dashboard (u/get-id dashboard)
- (merge
- ;; description is allowed to be `nil`
- (when (contains? dashboard :description)
- {:description (:description dashboard)})
- ;; only set everything else if its non-nil
- (into {} (for [k [:name :parameters :caveats :points_of_interest :show_in_getting_started :enable_embedding :embedding_params]
- :when (k dashboard)]
- {k (k dashboard)}))))
- (u/prog1 (Dashboard (u/get-id dashboard))
- (events/publish-event! :dashboard-update (assoc <> :actor_id user-id))))
-
-
;;; ## ---------------------------------------- REVISIONS ----------------------------------------
(defn serialize-dashboard
diff --git a/src/metabase/models/metric.clj b/src/metabase/models/metric.clj
index 5f91e2e7fc9..751157b8673 100644
--- a/src/metabase/models/metric.clj
+++ b/src/metabase/models/metric.clj
@@ -49,8 +49,8 @@
(select-keys metric1 [:name :description :definition])
(select-keys metric2 [:name :description :definition]))]
(cond-> (merge-with merge
- (m/map-vals (fn [v] {:after v}) (:after base-diff))
- (m/map-vals (fn [v] {:before v}) (:before base-diff)))
+ (m/map-vals (fn [v] {:after v}) (:after base-diff))
+ (m/map-vals (fn [v] {:before v}) (:before base-diff)))
(or (get-in base-diff [:after :definition])
(get-in base-diff [:before :definition])) (assoc :definition {:before (get-in metric1 [:definition])
:after (get-in metric2 [:definition])})))))
@@ -117,16 +117,20 @@
(retrieve-metrics table-id :active))
([table-id state]
{:pre [(integer? table-id) (keyword? state)]}
- (-> (if (= :all state)
- (db/select Metric, :table_id table-id, {:order-by [[:name :asc]]})
- (db/select Metric, :table_id table-id, :is_active (= :active state), {:order-by [[:name :asc]]}))
+ (-> (db/select Metric
+ {:where [:and [:= :table_id table-id]
+ (case state
+ :all true
+ :active [:= :is_active true]
+ :deleted [:= :is_active false])]
+ :order-by [[:name :asc]]})
(hydrate :creator))))
(defn update-metric!
"Update an existing `Metric`.
Returns the updated `Metric` or throws an Exception."
- [{:keys [id name description caveats points_of_interest how_is_this_calculated show_in_getting_started definition revision_message]} user-id]
+ [{:keys [id name definition revision_message], :as body} user-id]
{:pre [(integer? id)
(string? name)
(map? definition)
@@ -134,13 +138,7 @@
(string? revision_message)]}
;; update the metric itself
(db/update! Metric id
- :name name
- :description description
- :caveats caveats
- :points_of_interest points_of_interest
- :how_is_this_calculated how_is_this_calculated
- :show_in_getting_started show_in_getting_started
- :definition definition)
+ (select-keys body #{:caveats :definition :description :how_is_this_calculated :name :points_of_interest :show_in_getting_started}))
(u/prog1 (retrieve-metric id)
(events/publish-event! :metric-update (assoc <> :actor_id user-id, :revision_message revision_message))))
diff --git a/src/metabase/models/segment.clj b/src/metabase/models/segment.clj
index 1236d306228..ac7b91d4c72 100644
--- a/src/metabase/models/segment.clj
+++ b/src/metabase/models/segment.clj
@@ -107,8 +107,7 @@
(defn update-segment!
"Update an existing `Segment`.
Returns the updated `Segment` or throws an Exception."
- {:style/indent 0}
- [{:keys [id name description caveats points_of_interest show_in_getting_started definition revision_message]} user-id]
+ [{:keys [id name description caveats points_of_interest show_in_getting_started definition revision_message], :as body} user-id]
{:pre [(integer? id)
(string? name)
(map? definition)
@@ -116,15 +115,9 @@
(string? revision_message)]}
;; update the segment itself
(db/update! Segment id
- (merge
- {:name name
- :description description
- :caveats caveats
- :definition definition}
- (when (seq points_of_interest)
- {:points_of_interest points_of_interest})
- (when (not (nil? show_in_getting_started))
- {:show_in_getting_started show_in_getting_started})))
+ (u/select-keys-when body
+ :present #{:name :description :caveats :definition}
+ :non-nil #{:points_of_interest :show_in_getting_started}))
(u/prog1 (retrieve-segment id)
(events/publish-event! :segment-update (assoc <> :actor_id user-id, :revision_message revision_message))))
diff --git a/src/metabase/util.clj b/src/metabase/util.clj
index d73dc84cf24..fff2ab723b0 100644
--- a/src/metabase/util.clj
+++ b/src/metabase/util.clj
@@ -831,3 +831,28 @@
(if-let [new-index (s/index-of s substr index)]
(recur (inc new-index) (inc cnt))
cnt))))
+
+(defn select-non-nil-keys
+ "Like `select-keys`, but returns a map only containing keys in KS that are present *and non-nil* in M.
+
+ (select-non-nil-keys {:a 100, :b nil} #{:a :b :c})
+ ;; -> {:a 100}"
+ [m ks]
+ (into {} (for [k ks
+ :when (not (nil? (get m k)))]
+ {k (get m k)})))
+
+(defn select-keys-when
+ "Returns a map that only contains keys that are either `:present` or `:non-nil`.
+ Combines behavior of `select-keys` and `select-non-nil-keys`.
+ This is useful for API endpoints that update a model, which often have complex rules about what gets updated
+ (some keys are updated if `nil`, others only if non-nil).
+
+ (select-keys-when {:a 100, :b nil, :d 200, :e nil}
+ :present #{:a :b :c}
+ :non-nil #{:d :e :f})
+ ;; -> {:a 100, :b nil, :d 200}"
+ {:style/indent 1}
+ [m & {:keys [present non-nil]}]
+ (merge (select-keys m present)
+ (select-non-nil-keys m non-nil)))
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index a42630ea44f..a60512e787d 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -649,7 +649,7 @@
;; Test that we *cannot* share a Card if the Card has been archived
(expect
- "Not found."
+ "The object has been archived."
(tu/with-temporary-setting-values [enable-public-sharing true]
(tt/with-temp Card [card {:archived true}]
((user->client :crowberto) :post 404 (format "card/%d/public_link" (u/get-id card))))))
diff --git a/test/metabase/api/metric_test.clj b/test/metabase/api/metric_test.clj
index 925031a4d49..3cb39bbee62 100644
--- a/test/metabase/api/metric_test.clj
+++ b/test/metabase/api/metric_test.clj
@@ -15,6 +15,17 @@
;; ## Helper Fns
+(def ^:private ^:const metric-defaults
+ {:description nil
+ :show_in_getting_started false
+ :caveats nil
+ :points_of_interest nil
+ :how_is_this_calculated nil
+ :created_at true
+ :updated_at true
+ :is_active true
+ :definition {}})
+
(defn- user-details [user]
(tu/match-$ user
{:id $
@@ -72,19 +83,13 @@
:definition "foobar"}))
(expect
- {:name "A Metric"
- :description "I did it!"
- :show_in_getting_started false
- :caveats nil
- :points_of_interest nil
- :how_is_this_calculated nil
- :creator_id (user->id :crowberto)
- :creator (user-details (fetch-user :crowberto))
- :created_at true
- :updated_at true
- :is_active true
- :definition {:database 21
- :query {:filter ["abc"]}}}
+ (merge metric-defaults
+ {:name "A Metric"
+ :description "I did it!"
+ :creator_id (user->id :crowberto)
+ :creator (user-details (fetch-user :crowberto))
+ :definition {:database 21
+ :query {:filter ["abc"]}}})
(tt/with-temp* [Database [{database-id :id}]
Table [{:keys [id]} {:db_id database-id}]]
(metric-response ((user->client :crowberto) :post 200 "metric" {:name "A Metric"
@@ -107,39 +112,37 @@
:revision_message "something different"}))
;; test validations
-(expect {:errors {:name "value must be a non-blank string."}}
+(expect
+ {:errors {:name "value must be a non-blank string."}}
((user->client :crowberto) :put 400 "metric/1" {}))
-(expect {:errors {:revision_message "value must be a non-blank string."}}
+(expect
+ {:errors {:revision_message "value must be a non-blank string."}}
((user->client :crowberto) :put 400 "metric/1" {:name "abc"}))
-(expect {:errors {:revision_message "value must be a non-blank string."}}
+(expect
+ {:errors {:revision_message "value must be a non-blank string."}}
((user->client :crowberto) :put 400 "metric/1" {:name "abc"
- :revision_message ""}))
+ :revision_message ""}))
-(expect {:errors {:definition "value must be a map."}}
+(expect
+ {:errors {:definition "value must be a map."}}
((user->client :crowberto) :put 400 "metric/1" {:name "abc"
- :revision_message "123"}))
+ :revision_message "123"}))
-(expect {:errors {:definition "value must be a map."}}
+(expect
+ {:errors {:definition "value must be a map."}}
((user->client :crowberto) :put 400 "metric/1" {:name "abc"
- :revision_message "123"
- :definition "foobar"}))
+ :revision_message "123"
+ :definition "foobar"}))
(expect
- {:name "Costa Rica"
- :description nil
- :show_in_getting_started false
- :caveats nil
- :points_of_interest nil
- :how_is_this_calculated nil
- :creator_id (user->id :rasta)
- :creator (user-details (fetch-user :rasta))
- :created_at true
- :updated_at true
- :is_active true
- :definition {:database 2
- :query {:filter ["not" "the toucans you're looking for"]}}}
+ (merge metric-defaults
+ {:name "Costa Rica"
+ :creator_id (user->id :rasta)
+ :creator (user-details (fetch-user :rasta))
+ :definition {:database 2
+ :query {:filter ["not" "the toucans you're looking for"]}}})
(tt/with-temp* [Database [{database-id :id}]
Table [{table-id :id} {:db_id database-id}]
Metric [{:keys [id]} {:table_id table-id}]]
@@ -172,18 +175,12 @@
(expect
[{:success true}
- {:name "Toucans in the rainforest"
- :description "Lookin' for a blueberry"
- :show_in_getting_started false
- :caveats nil
- :points_of_interest nil
- :how_is_this_calculated nil
- :creator_id (user->id :rasta)
- :creator (user-details (fetch-user :rasta))
- :created_at true
- :updated_at true
- :is_active false
- :definition {}}]
+ (merge metric-defaults
+ {:name "Toucans in the rainforest"
+ :description "Lookin' for a blueberry"
+ :creator_id (user->id :rasta)
+ :creator (user-details (fetch-user :rasta))
+ :is_active false})]
(tt/with-temp* [Database [{database-id :id}]
Table [{table-id :id} {:db_id database-id}]
Metric [{:keys [id]} {:table_id table-id}]]
@@ -199,18 +196,11 @@
(expect
- {:name "Toucans in the rainforest"
- :description "Lookin' for a blueberry"
- :show_in_getting_started false
- :caveats nil
- :points_of_interest nil
- :how_is_this_calculated nil
- :creator_id (user->id :crowberto)
- :creator (user-details (fetch-user :crowberto))
- :created_at true
- :updated_at true
- :is_active true
- :definition {}}
+ (merge metric-defaults
+ {:name "Toucans in the rainforest"
+ :description "Lookin' for a blueberry"
+ :creator_id (user->id :crowberto)
+ :creator (user-details (fetch-user :crowberto))})
(tt/with-temp* [Database [{database-id :id}]
Table [{table-id :id} {:db_id database-id}]
Metric [{:keys [id]} {:creator_id (user->id :crowberto)
diff --git a/test/metabase/models/metric_test.clj b/test/metabase/models/metric_test.clj
index e0ec8b92ad3..4ac4e9a7e4b 100644
--- a/test/metabase/models/metric_test.clj
+++ b/test/metabase/models/metric_test.clj
@@ -10,6 +10,15 @@
[metabase.test.util :as tu]
[metabase.util :as u]))
+(def ^:private ^:const metric-defaults
+ {:description nil
+ :how_is_this_calculated nil
+ :show_in_getting_started false
+ :caveats nil
+ :points_of_interest nil
+ :is_active true
+ :definition {}})
+
(defn- user-details
[username]
(dissoc (fetch-user username) :date_joined :last_login))
@@ -30,16 +39,11 @@
;; create-metric!
(expect
- {:creator_id (user->id :rasta)
- :creator (user-details :rasta)
- :name "I only want *these* things"
- :description nil
- :how_is_this_calculated nil
- :show_in_getting_started false
- :caveats nil
- :points_of_interest nil
- :is_active true
- :definition {:clause ["a" "b"]}}
+ (merge metric-defaults
+ {:creator_id (user->id :rasta)
+ :creator (user-details :rasta)
+ :name "I only want *these* things"
+ :definition {:clause ["a" "b"]}})
(tt/with-temp* [Database [{database-id :id}]
Table [{:keys [id]} {:db_id database-id}]]
(create-metric-then-select! id "I only want *these* things" nil (user->id :rasta) {:clause ["a" "b"]})))
@@ -60,17 +64,13 @@
;; retrieve-metric
(expect
- {:creator_id (user->id :rasta)
- :creator (user-details :rasta)
- :name "Toucans in the rainforest"
- :description "Lookin' for a blueberry"
- :how_is_this_calculated nil
- :show_in_getting_started false
- :caveats nil
- :points_of_interest nil
- :is_active true
- :definition {:database 45
- :query {:filter ["yay"]}}}
+ (merge metric-defaults
+ {:creator_id (user->id :rasta)
+ :creator (user-details :rasta)
+ :name "Toucans in the rainforest"
+ :description "Lookin' for a blueberry"
+ :definition {:database 45
+ :query {:filter ["yay"]}}})
(tt/with-temp* [Database [{database-id :id}]
Table [{table-id :id} {:db_id database-id}]
Metric [{metric-id :id} {:table_id table-id
@@ -81,18 +81,12 @@
:creator (u/rpartial dissoc :date_joined :last_login)))))
-;; retrieve-segements
+;; retrieve-metrics
(expect
- [{:creator_id (user->id :rasta)
- :creator (user-details :rasta)
- :name "Metric 1"
- :description nil
- :how_is_this_calculated nil
- :show_in_getting_started false
- :caveats nil
- :points_of_interest nil
- :is_active true
- :definition {}}]
+ [(merge metric-defaults
+ {:creator_id (user->id :rasta)
+ :creator (user-details :rasta)
+ :name "Metric 1"})]
(tt/with-temp* [Database [{database-id :id}]
Table [{table-id-1 :id} {:db_id database-id}]
Table [{table-id-2 :id} {:db_id database-id}]
@@ -113,17 +107,12 @@
;; 4. ability to modify the definition json
;; 5. revision is captured along with our commit message
(expect
- {:creator_id (user->id :rasta)
- :creator (user-details :rasta)
- :name "Costa Rica"
- :description nil
- :how_is_this_calculated nil
- :show_in_getting_started false
- :caveats nil
- :points_of_interest nil
- :is_active true
- :definition {:database 2
- :query {:filter ["not" "the toucans you're looking for"]}}}
+ (merge metric-defaults
+ {:creator_id (user->id :rasta)
+ :creator (user-details :rasta)
+ :name "Costa Rica"
+ :definition {:database 2
+ :query {:filter ["not" "the toucans you're looking for"]}}})
(tt/with-temp* [Database [{database-id :id}]
Table [{table-id :id} {:db_id database-id}]
Metric [{metric-id :id} {:table_id table-id}]]
@@ -142,16 +131,12 @@
;; delete-metric!
(expect
- {:creator_id (user->id :rasta)
- :creator (user-details :rasta)
- :name "Toucans in the rainforest"
- :description "Lookin' for a blueberry"
- :how_is_this_calculated nil
- :show_in_getting_started false
- :caveats nil
- :points_of_interest nil
- :is_active false
- :definition {}}
+ (merge metric-defaults
+ {:creator_id (user->id :rasta)
+ :creator (user-details :rasta)
+ :name "Toucans in the rainforest"
+ :description "Lookin' for a blueberry"
+ :is_active false})
(tt/with-temp* [Database [{database-id :id}]
Table [{table-id :id} {:db_id database-id}]
Metric [{metric-id :id} {:table_id table-id}]]
@@ -165,18 +150,14 @@
;; serialize-metric
(expect
- {:id true
- :table_id true
- :creator_id (user->id :rasta)
- :name "Toucans in the rainforest"
- :description "Lookin' for a blueberry"
- :how_is_this_calculated nil
- :show_in_getting_started false
- :caveats nil
- :points_of_interest nil
- :definition {:aggregation ["count"]
- :filter ["AND" [">" 4 "2014-10-19"]]}
- :is_active true}
+ (merge metric-defaults
+ {:id true
+ :table_id true
+ :creator_id (user->id :rasta)
+ :name "Toucans in the rainforest"
+ :description "Lookin' for a blueberry"
+ :definition {:aggregation ["count"]
+ :filter ["AND" [">" 4 "2014-10-19"]]}})
(tt/with-temp* [Database [{database-id :id}]
Table [{table-id :id} {:db_id database-id}]
Metric [metric {:table_id table-id
diff --git a/test/metabase/permissions_collection_test.clj b/test/metabase/permissions_collection_test.clj
index d6e82ec3712..7e5c19c7abd 100644
--- a/test/metabase/permissions_collection_test.clj
+++ b/test/metabase/permissions_collection_test.clj
@@ -55,8 +55,10 @@
(println "[In the occasionally failing test]") ; DEBUG
(set-card-collection! collection)
(permissions/grant-collection-read-permissions! (group/all-users) collection)
- ;; try it twice because sometimes it randomly fails :unamused:
+ ;; try it a few times because sometimes it randomly fails :unamused:
(or (can-run-query? :rasta)
+ (can-run-query? :rasta)
+ (Thread/sleep 1000)
(can-run-query? :rasta))))
;; Make sure a User isn't allowed to save a Card they have collections readwrite permissions for
diff --git a/test/metabase/util_test.clj b/test/metabase/util_test.clj
index 8520c9866e9..8232181092d 100644
--- a/test/metabase/util_test.clj
+++ b/test/metabase/util_test.clj
@@ -230,3 +230,15 @@
(expect 1 (occurances-of-substring "WHERE ID = {{id}}" "{{id}}"))
(expect 2 (occurances-of-substring "WHERE ID = {{id}} OR USER_ID = {{id}}" "{{id}}"))
(expect 3 (occurances-of-substring "WHERE ID = {{id}} OR USER_ID = {{id}} OR TOUCAN_ID = {{id}} OR BIRD_ID = {{bird}}" "{{id}}"))
+
+
+;;; tests for `select-non-nil-keys` and `select-keys-when`
+(expect
+ {:a 100}
+ (select-non-nil-keys {:a 100, :b nil} #{:a :b :c}))
+
+(expect
+ {:a 100, :b nil, :d 200}
+ (select-keys-when {:a 100, :b nil, :d 200, :e nil}
+ :present #{:a :b :c}
+ :non-nil #{:d :e :f}))
--
GitLab