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] Import - extract duplicate code to function #17080

Merged
merged 1 commit into from
Apr 19, 2020

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Apr 15, 2020

Overview

Straightforward function extraction to reduce duplicate code.
This is a followup to #16979

Before

Same code in 4 places

After

Now just in one place

Comments

FIXME: Extracting this was a first step, but there's also still lots of inconsistency
and duplication with how the various import classes handle custom data.

FIXME: Extracting this was a first step, but there's also still lots of inconsistency
and duplication with how the various import classes handle custom data.
@civibot civibot bot added the master label Apr 15, 2020
@civibot
Copy link

civibot bot commented Apr 15, 2020

(Standard links)

@demeritcowboy
Copy link
Contributor

This looks ok to me. There's a trim() difference for one of them but it seems incorrect before.

For r-run I'm not even sure multivalued checkboxes are working on import even before the patch, but I got the same non-result both before and after the patch so it's consistent.

@colemanw
Copy link
Member Author

@demeritcowboy for your "before" test were you using master or a release? We should ensure it wasn't a recent regression in master, as there have been other changes to this code recently such as #16979.

@demeritcowboy
Copy link
Contributor

It was master. I looked on stackexchange just to see if maybe there were some pointers if I was doing it wrong and the most recent I found was from 2018 and it didn't seem to be resolved. And then I went off to something else. I can take another look with a release.

@colemanw colemanw merged commit 82ef5c9 into civicrm:master Apr 19, 2020
@colemanw colemanw deleted the importExtract branch April 19, 2020 01:33
@demeritcowboy
Copy link
Contributor

Same in 5.24.4.

@colemanw
Copy link
Member Author

I suspect that reconciling these classes with the way contact import does it would fix a lot.

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