Skip to content
Snippets Groups Projects
Commit 4abdbb2e authored by Allen Gilliland's avatar Allen Gilliland
Browse files

few more tweaks to segment revisions to account for case where we are... 

few more tweaks to segment revisions to account for case where we are comparing against nil object for initial revision.
parent ba7c9e83
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/models/segment.clj
+ 12
− 8
View file @ 4abdbb2e
......@@ -143,14 +143,18 @@
(m/filter-vals (complement delay?))))
(defn diff-segments [this segment1 segment2]
(let [base-diff (revision/default-diff-map this
(select-keys segment1 [:name :description :definition])
(select-keys segment2 [:name :description :definition]))]
(cond-> (merge-with merge
(u/update-values (:after base-diff) (fn [v] {:after v}))
(u/update-values (:before base-diff) (fn [v] {:before v})))
(get-in base-diff [:after :definition]) (assoc :definition {:before (get-in segment1 [:definition :filter])
:after (get-in segment2 [:definition :filter])}))))
(if-not segment1
;; this is the first version of the segment
(u/update-values (select-keys segment2 [:name :description :definition]) (fn [v] {:after v}))
;; do our diff logic
(let [base-diff (revision/default-diff-map this
(select-keys segment1 [:name :description :definition])
(select-keys segment2 [:name :description :definition]))]
(cond-> (merge-with merge
(u/update-values (:after base-diff) (fn [v] {:after v}))
(u/update-values (:before base-diff) (fn [v] {:before v})))
(get-in base-diff [:after :definition]) (assoc :definition {:before (get-in segment1 [:definition :filter])
:after (get-in segment2 [:definition :filter])})))))
(extend SegmentEntity
revision/IRevisioned
......
test/metabase/api/segment_test.clj
+ 18
− 12
View file @ 4abdbb2e
......@@ -238,14 +238,15 @@
:message "updated"
:user (-> (user-details (fetch-user :crowberto))
(dissoc :email :date_joined :last_login :is_superuser))
:diff {:before {:a "b"}, :after {:a "c"}}
:description "changed a from \"b\" to \"c\"."}
:diff {:name {:before "b" :after "c"}}
:description "renamed this Segment from \"b\" to \"c\"."}
{:is_reversion false
:is_creation true
:message nil
:user (-> (user-details (fetch-user :rasta))
(dissoc :email :date_joined :last_login :is_superuser))
:diff nil
:diff {:name {:after "b"}
:definition {:after {:filter ["AND" [">" 1 25]]}}}
:description nil}]
(tu/with-temp Database [{database-id :id} {:name "Hillbilly"
:engine :yeehaw
......@@ -263,13 +264,15 @@
(tu/with-temp Revision [_ {:model "Segment"
:model_id id
:user_id (user->id :rasta)
:object {:a "b"}
:object {:name "b"
:definition {:filter ["AND" [">" 1 25]]}}
:is_creation true
:is_reversion false}]
(tu/with-temp Revision [_ {:model "Segment"
:model_id id
:user_id (user->id :crowberto)
:object {:a "c"}
:object {:name "c"
:definition {:filter ["AND" [">" 1 25]]}}
:is_creation false
:is_reversion false
:message "updated"}]
......@@ -298,8 +301,8 @@
:message nil
:user (-> (user-details (fetch-user :crowberto))
(dissoc :email :date_joined :last_login :is_superuser))
:diff {:before {:name "Changed Segment Name"},
:after {:name "One Segment to rule them all, one segment to define them"}}
:diff {:name {:before "Changed Segment Name"
:after "One Segment to rule them all, one segment to define them"}}
:description "renamed this Segment from \"Changed Segment Name\" to \"One Segment to rule them all, one segment to define them\"."}
;; full list of final revisions, first one should be same as the revision returned by the endpoint
[{:is_reversion true
......@@ -307,23 +310,26 @@
:message nil
:user (-> (user-details (fetch-user :crowberto))
(dissoc :email :date_joined :last_login :is_superuser))
:diff {:before {:name "Changed Segment Name"},
:after {:name "One Segment to rule them all, one segment to define them"}}
:diff {:name {:before "Changed Segment Name"
:after "One Segment to rule them all, one segment to define them"}}
:description "renamed this Segment from \"Changed Segment Name\" to \"One Segment to rule them all, one segment to define them\"."}
{:is_reversion false
:is_creation false
:message "updated"
:user (-> (user-details (fetch-user :crowberto))
(dissoc :email :date_joined :last_login :is_superuser))
:diff {:before {:name "One Segment to rule them all, one segment to define them"},
:after {:name "Changed Segment Name"}}
:diff {:name {:after "Changed Segment Name"
:before "One Segment to rule them all, one segment to define them"}}
:description "renamed this Segment from \"One Segment to rule them all, one segment to define them\" to \"Changed Segment Name\"."}
{:is_reversion false
:is_creation true
:message nil
:user (-> (user-details (fetch-user :rasta))
(dissoc :email :date_joined :last_login :is_superuser))
:diff nil
:diff {:name {:after "One Segment to rule them all, one segment to define them"}
:description {:after "One segment to bring them all, and in the DataModel bind them"}
:definition {:after {:database 123
:query {:filter ["In the Land of Metabase where the Datas lie"]}}}}
:description nil}]]
(tu/with-temp Database [{database-id :id} {:name "Hillbilly"
:engine :yeehaw
......
test/metabase/models/segment_test.clj
+ 12
− 0
View file @ 4abdbb2e
......@@ -224,6 +224,7 @@
:description "BBB"
:definition {:filter ["AND",["BETWEEN",4,"2014-07-01","2014-10-19"]]}))))))
;; test case where difinition doesn't change
(expect
{:name {:before "A"
:after "B"}}
......@@ -234,3 +235,14 @@
{:name "B"
:description "Unchanged"
:definition {:filter ["AND",[">",4,"2014-10-19"]]}}))
;; first version, so comparing against nil
(expect
{:name {:after "A"}
:description {:after "Unchanged"}
:definition {:after {:filter ["AND",[">",4,"2014-10-19"]]}}}
(diff-segments Segment
nil
{:name "A"
:description "Unchanged"
:definition {:filter ["AND",[">",4,"2014-10-19"]]}}))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment