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

[REF] Update custom data handing in contact import #16986

Merged
merged 1 commit into from
Apr 7, 2020

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Apr 5, 2020

Overview

Cleanup custom field handling in contact import.

Before

Over-reliance on htlm_type.

After

Uses more standard isSerialized method.

Comments

The other import classes got similar cleanup in #16979, which was merged by @Eileen. This one was a bit more cumbersome. I still consider the large amount of duplicated code across the 5 import classes to be a big FIXME, as it makes maintenance like this far more difficult than it should be.

The diff is not as large as it looks; changing the switch to if/else caused the indentation to change.

@civibot
Copy link

civibot bot commented Apr 5, 2020

(Standard links)

@civibot civibot bot added the master label Apr 5, 2020
@eileenmcnaughton
Copy link
Contributor

The code makes sense & testing reveals bugs that are not made worse by this - ie importing custom fields of the following combos fails with an without this patch

Money->Select
Number->Text
Number->Select

Screen Shot 2020-04-07 at 11 59 20 AM

leads to

Screen Shot 2020-04-07 at 12 02 13 PM

I haven't dug further as my focus is to ensure no regression from this

@eileenmcnaughton eileenmcnaughton merged commit 2bb8b30 into civicrm:master Apr 7, 2020
@eileenmcnaughton eileenmcnaughton deleted the importParser branch April 7, 2020 00:03
@colemanw
Copy link
Member Author

colemanw commented Apr 8, 2020

That bears further investigation. The Import classes are a mess; I'd like to clean up, consolidate and test their handling custom data, esp. in light of this movement to improve the custom field schema.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants