Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate old pre-1.37 "Custom Drill-through" settings to x.37+ "Click Behavior" #15014

Closed
camsaul opened this issue Mar 1, 2021 · 0 comments · Fixed by #15050
Closed

Migrate old pre-1.37 "Custom Drill-through" settings to x.37+ "Click Behavior" #15014

camsaul opened this issue Mar 1, 2021 · 0 comments · Fixed by #15050

Comments

@camsaul
Copy link
Member

camsaul commented Mar 1, 2021

We need to update Card and DashboardCard visualization_settings and convert them from the old format to the new format where appropriate. @flamber has written SQL to do this for Postgres specifically:

-- MAKE SURE YOU HAVE BACKUPS BEFORE RUNNING. RUN THE "getter-select" TO SEE WHICH COLUMNS MIGHT BE UPDATED.
-- WARNING - THIS WILL REPLACE ANY 37+ "CLICK BEHAVIOR" WITH DATA FROM THE OLD <=36 "CUSTOM DRILL-THROUGH"



-- Function credit to https://stackoverflow.com/a/49688529
create or replace function jsonb_merge(CurrentData jsonb,newData jsonb)
 returns jsonb
 language sql
 immutable
as $jsonb_merge_func$
 select case jsonb_typeof(CurrentData)
   when 'object' then case jsonb_typeof(newData)
     when 'object' then (
       select    jsonb_object_agg(k, case
                   when e2.v is null then e1.v
                   when e1.v is null then e2.v
                   when e1.v = e2.v then e1.v 
                   else jsonb_merge(e1.v, e2.v)
                 end)
       from      jsonb_each(CurrentData) e1(k, v)
       full join jsonb_each(newData) e2(k, v) using (k)
     )
     else newData
   end
   when 'array' then CurrentData || newData
   else newData
 end
$jsonb_merge_func$;



-- MAKE SURE YOU HAVE BACKUPS BEFORE RUNNING. RUN THE "getter-select" TO SEE WHICH COLUMNS MIGHT BE UPDATED.
-- WARNING - THIS WILL REPLACE ANY 37+ "CLICK BEHAVIOR" WITH DATA FROM THE OLD <=36 "CUSTOM DRILL-THROUGH"
update report_dashboardcard
set visualization_settings=flamberfix.custom_drill_to_click_behavior_converter
from (
    -- START of getter
	select dashcard.id, jsonb_strip_nulls(
		  jsonb_merge(
			  jsonb_merge(
				  dashcard.visualization_settings::jsonb
			      ,
				  jsonb_strip_nulls(jsonb_build_object('click_behavior',
				    (SELECT
				       jsonb_strip_nulls(jsonb_build_object(
				         'type', clickType,
				         'linkType', 'url',
				         'linkTemplate', linkTemplate
				       ))
				     FROM (
				     	  SELECT
						    jsonb_merge(card.visualization_settings::jsonb, dashcard.visualization_settings::jsonb)::jsonb->>'click' as clickType,
						    jsonb_merge(card.visualization_settings::jsonb, dashcard.visualization_settings::jsonb)::jsonb->>'click_link_template' as linkTemplate
				     ) as json_viz
				     WHERE clickType = 'link' AND linkTemplate IS NOT NULL
				    )
				  ))::jsonb
			  )
			  ,
			  jsonb_strip_nulls(jsonb_build_object('column_settings', (
			    SELECT
			        jsonb_strip_nulls(jsonb_object_agg(field,
			          jsonb_strip_nulls(jsonb_build_object('click_behavior',
			            jsonb_strip_nulls(jsonb_build_object(
			              'type', clickType,
			              'linkType', 'url',
			              'linkTemplate', linkTemplate,
			              'linkTextTemplate', linkTextTemplate
			            ))
			          ))
			        ))::jsonb AS fields
			    FROM (
				  SELECT
				    key as field,
				    value::jsonb->'view_as' as clickType,
				    value::jsonb->'link_template' as linkTemplate,
				    value::jsonb->'link_text' as linkTextTemplate
				  FROM jsonb_each_text(jsonb_merge(card.visualization_settings::jsonb, dashcard.visualization_settings::jsonb)::jsonb->'column_settings')
				) as json_table
			    WHERE jsonb_typeof(clickType)!='null' AND jsonb_typeof(linkTemplate)!='null'
			  )
			  ))::jsonb
			)
		)::jsonb as custom_drill_to_click_behavior_converter
	from report_dashboardcard dashcard
	left join report_card card on card.id = dashcard.card_id
	where dashcard.card_id is not null and (
	  card.visualization_settings like '%"link_template":%' or card.visualization_settings like '%"click_link_template":%'
	  or
	  dashcard.visualization_settings like '%"link_template":%' or dashcard.visualization_settings like '%"click_link_template":%'
	)
	-- END of getter
) as flamberfix
where report_dashboardcard.id=flamberfix.id and report_dashboardcard.visualization_settings::jsonb!=flamberfix.custom_drill_to_click_behavior_converter;



DROP FUNCTION IF exists jsonb_merge(CurrentData jsonb,newData jsonb);

To get this working for MySQL & H2 as well we can leverage the data migrations code we have in metabase.db.data-migrations. An example data migration for viz settings would look something like

(defmigration ^{:added "0.39.0"} migrate-map-regions
  (reduce
   (fn [_ {card-id :id, {map-region :map.region, :as viz-settings} :visualization_settings}]
     ;; find viz settings meeting some condition
     (when (= map-region "us_states")
       ;; apply some changes to viz settings
       (let [new-settings (update viz-settings :map.region str/upper-case)]
         ;; save updated viz settings
         (db/update! Card card-id :visualization_settings new-settings))))
   nil
   (db/select-reducible [Card :id :visualization_settings])))

@flamber or @paulrosenzweig might be able to help out with questions around how the format has changed between old and new versions

@camsaul camsaul added this to the 0.38.1 milestone Mar 1, 2021
@flamber flamber changed the title Migrate old < x.37.x "click behavior" viz settings to > x.37.x "custom drill-thru" Migrate old < x.37.x "Custom Drill-through" viz settings to > x.37.x "Click Behavior" Mar 2, 2021
dpsutton added a commit that referenced this issue Mar 3, 2021
@camsaul camsaul modified the milestones: 0.38.1, 0.38.2 Mar 3, 2021
@rlotun rlotun added this to Key Bug fixes (target: 38.2) in 39 and 38.x Planning Board Mar 4, 2021
@flamber flamber changed the title Migrate old < x.37.x "Custom Drill-through" viz settings to > x.37.x "Click Behavior" Migrate old pre-1.37 "Custom Drill-through" settings to x.37+ "Click Behavior" Mar 5, 2021
dpsutton added a commit that referenced this issue Mar 10, 2021
* Fix click through behavior (#15014)

* formatting

* Safer get's, author metadata and correct added

* Don't return anything if we didn't modify it

* Make private and add a docstring

* Generate json strings rather than long in-line versions

* Docstrings and prevent migrating ones that may already be migrated

* Noisier as it now merges click_behavior on top of existing

* update docstring with new nesting

* always keep column settings

previously the script was only looking for column settings which
included view_as or link_template. Now we always keep those columns
and merge in the new click_behavior when those keys are present

* linkTemplateText -> linkTextTemplate

* Ensure dashcard goes on top of card viz settings

* Update expected now that dash merges on top of card

* drop columns that don't have view_as

so we don't overwrite the top level stuff if there is already a
click_behavior. That signifies that either this migration has run, or
perhaps already has had manual intervention. An open question is if we
extend this to the columns: if there's manual intervention do we
really want to be mucking about in here?

Also, go back to the dropping columns style.

```clojure
(if (and ...)
    (assoc m col (merge field-settings click-behavior))
    m) ;; here we omit the column again in our reduce-kv
```

* Fundamentally: scour card for click and merge them into dashcard

reshape all click behaviors in both and merge them in.

Still some broken tests at the moment

* Remove old style click behavior no longer expected

* fixed

* Some more comments and remove some silly select keys and when-lets

just compute both keys and then u/select-non-nil-keys and be done with
it

* Docstrinc additions about nil

* Remove niles, test scenario #5 from flamber

* Handle empties and nil _AFTER_ merging

previously had taken care to punt things down to nil, collapse empty
maps, etc. but this is a value that will merge on top of a map rather
than with the map

```clojure
(m/deep-merge {:a {:b :c}} {:a nil})
{:a nil}
;; vs
(m/deep-merge {:a {:b :c}} {:a {}})
{:a {:b :c}}
```

so let the postwalk after the merge clean up empty collections and nil
values and let them blend as normal

* Deep merge and prioritize new shape in card and dashcard

```clojure
(m/deep-merge fixed-card
              fixed-dashcard
              (existing-fixed card)
              (existing-fixed dashcard))
```

This is the change here essentially. Fix the card and dashcard, look
for existing new shapes on the card and dashcard as well

* Don't merge existing fix from card. can't happen

this change though did help out. we want to resolve when there are old
style and new style on the same dashcard. In this case, the new style
should persist:

```clojure
(testing "If there is migration eligible on dash but also new style on dash, new style wins"
  (let [dash {"column_settings"
              {"[\"ref\",[\"field-id\",4]]"
               {"view_as"       "link"
                "link_template" "http://old" ;; this stuff could be migrated
                "link_text"     "old"
                "column_title"  "column title"
                "click_behavior"
                {"type" "link",
                 "linkType" "url", ;; but there is already a new style and it wins
                 "linkTemplate" "http://new",
                 "linkTextTemplate" "new"}}}}]
    (is (= {"column_settings"
            {"[\"ref\",[\"field-id\",4]]"
             {"click_behavior"
              {"type" "link",
               "linkType" "url",
               "linkTemplate" "http://new",
               "linkTextTemplate" "new"},
              "column_title" "column title"}}}
           (migrate nil dash)))))
```

note the column settings for field-id 4 have the old style and the new
style but the result is that the new style survives. This happens from
the `(m/deep-merge fixed-card fixed-dashcard (existing-fixed
dashcard))` as the existing is put on top

* Don't strip original values

* propagate old info in migration test
@rlotun rlotun moved this from Key Bug fixes (target: 38.2) to Closed Issues and PRs in 39 and 38.x Planning Board Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
39 and 38.x Planning Board
Closed Issues and PRs
Development

Successfully merging a pull request may close this issue.

3 participants