From c29951e0fa551968ef006d838f616964f26b87e1 Mon Sep 17 00:00:00 2001
From: adam-james <21064735+adam-james-v@users.noreply.github.com>
Date: Thu, 24 Mar 2022 13:08:31 -0700
Subject: [PATCH] Hydrate Collections on Timelines for card/collection
endpoints (#21227)
* Hydrate Collections on Timelines for card/collection endpoints
* Add tests to check that collection is hydrated
* Add missing docstring
---
src/metabase/api/timeline.clj | 16 ++--------------
src/metabase/models/timeline.clj | 20 ++++++++++++++++++--
test/metabase/api/card_test.clj | 5 +++++
test/metabase/api/collection_test.clj | 5 +++++
4 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/src/metabase/api/timeline.clj b/src/metabase/api/timeline.clj
index 591a3ac3298..e98d88fb530 100644
--- a/src/metabase/api/timeline.clj
+++ b/src/metabase/api/timeline.clj
@@ -32,25 +32,13 @@
{:icon timeline/DefaultIcon}))]
(db/insert! Timeline tl)))
-;; todo: should this fn move into `metabase.model.collection`?
-;; a nearly identical fn exists in `metabase.api.collection`
-(defn- root-collection
- []
- (-> (collection/root-collection-with-ui-details nil)
- collection/personal-collection-with-ui-details
- (hydrate :parent_id :effective_location [:effective_ancestors :can_write] :can_write)))
-
(api/defendpoint GET "/"
"Fetch a list of [[Timelines]]. Can include `archived=true` to return archived timelines."
[include archived]
{include (s/maybe Include)
archived (s/maybe su/BooleanString)}
(let [archived? (Boolean/parseBoolean archived)
- hydrate-root-collection (fn [tl]
- (if (nil? (:collection_id tl))
- (assoc tl :collection (root-collection))
- tl))
- timelines (map hydrate-root-collection (db/select Timeline [:where [:= :archived archived?]]))]
+ timelines (map timeline/hydrate-root-collection (db/select Timeline [:where [:= :archived archived?]]))]
(cond->> (hydrate timelines :creator :collection)
(= include "events")
(map #(timeline-event/include-events-singular % {:events/all? archived?})))))
@@ -69,7 +57,7 @@
;; `collection_id` `nil` means we need to assoc 'root' collection
;; because hydrate `:collection` needs a proper `:id` to work.
(nil? (:collection_id timeline))
- (assoc :collection (root-collection))
+ timeline/hydrate-root-collection
(= include "events")
(timeline-event/include-events-singular {:events/all? archived?
diff --git a/src/metabase/models/timeline.clj b/src/metabase/models/timeline.clj
index 9b86314c669..57c5a5a41c6 100644
--- a/src/metabase/models/timeline.clj
+++ b/src/metabase/models/timeline.clj
@@ -1,5 +1,6 @@
(ns metabase.models.timeline
- (:require [metabase.models.interface :as i]
+ (:require [metabase.models.collection :as collection]
+ [metabase.models.interface :as i]
[metabase.models.permissions :as perms]
[metabase.models.timeline-event :as timeline-event]
[metabase.util :as u]
@@ -22,6 +23,19 @@
;;;; functions
+(defn- root-collection
+ []
+ (-> (collection/root-collection-with-ui-details nil)
+ collection/personal-collection-with-ui-details
+ (hydrate :parent_id :effective_location [:effective_ancestors :can_write] :can_write)))
+
+(defn hydrate-root-collection
+ "Hydrate `:collection` on [[Timelines]] when the id is `nil`."
+ [{:keys [collection_id] :as timeline}]
+ (if (nil? collection_id)
+ (assoc timeline :collection (root-collection))
+ timeline))
+
(defn timelines-for-collection
"Load timelines based on `collection-id` passed in (nil means the root collection). Hydrates the events on each
timeline at `:events` on the timeline."
@@ -29,7 +43,9 @@
(cond-> (hydrate (db/select Timeline
:collection_id collection-id
:archived (boolean archived?))
- :creator)
+ :creator
+ :collection)
+ (nil? collection-id) (->> (map hydrate-root-collection))
events? (timeline-event/include-events options)))
(u/strict-extend (class Timeline)
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index 57be4235ad6..03347ec4e4a 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -1280,6 +1280,11 @@
(testing "Timelines in the collection of the card are returned"
(is (= #{"Timeline A"}
(timeline-names (timelines-request card-a false)))))
+ (testing "Timelines in the collection have a hydrated `:collection` key"
+ (is (= #{(u/the-id coll-a)}
+ (->> (timelines-request card-a false)
+ (map #(get-in % [:collection :id]))
+ set))))
(testing "Only un-archived timelines in the collection of the card are returned"
(is (= #{"Timeline B"}
(timeline-names (timelines-request card-b false)))))
diff --git a/test/metabase/api/collection_test.clj b/test/metabase/api/collection_test.clj
index 24c4654b52e..a7e570572f1 100644
--- a/test/metabase/api/collection_test.clj
+++ b/test/metabase/api/collection_test.clj
@@ -1481,6 +1481,11 @@
(testing "Timelines in the collection of the card are returned"
(is (= #{"Timeline A"}
(timeline-names (timelines-request coll-a false)))))
+ (testing "Timelines in the collection have a hydrated `:collection` key"
+ (is (= #{(u/the-id coll-a)}
+ (->> (timelines-request coll-a false)
+ (map #(get-in % [:collection :id]))
+ set))))
(testing "Only un-archived timelines in the collection of the card are returned"
(is (= #{"Timeline B"}
(timeline-names (timelines-request coll-b false)))))
--
GitLab