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

Fix civiimport crash on unmapped fields, remove overzealous cleanup, add api to help debug & test #24603

Merged
merged 4 commits into from
Sep 26, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Sep 24, 2022

Overview

Fix civiimport crash on unmapped fields

Before

When I production tested the civivimport extension I hit a crash because on unmapped fields the name is unset. I think I hit this in production but not in prior testing because I was using an existing mapping - which I wasn't doing in my last tests in my dev tests.

After

Crash fixed

In prod testing I quickly felt that adding the validate and import api to help diagnose problems was really helpful. Since that part only affects the exensions, which is still alpha, I added them into this too.

Technical Details

Unmapped fields coming in via qf have the name property set, but equal to ''. In earlier cuts it was do_not_import. This still checks for do_not_import but uses an empty check rather than a comparison on '' - I could ensure it is always passed at the js layer - but that feels more fragile & I can't easily add a test

Comments

I also removed a table clean up that was part of the old form layer - but the form layer is not doing it's own thing so it's just plain risky now & may be why I have seen some odd behaviours around table delete

@civibot
Copy link

civibot bot commented Sep 24, 2022

(Standard links)

@civibot civibot bot added the 5.54 label Sep 24, 2022
@eileenmcnaughton eileenmcnaughton changed the title Fix civiimport crash on unmapped fields Fix civiimport crash on unmapped fields, remove overzealous cleanup, add api to help debug & test Sep 26, 2022
@eileenmcnaughton eileenmcnaughton force-pushed the import branch 2 times, most recently from dbf0f91 to 9f168b3 Compare September 26, 2022 03:59
@eileenmcnaughton
Copy link
Contributor Author

@colemanw can you merge this -still focussed on the extension - which is alpha / disabled in the rc but trying to keep stuff together

@eileenmcnaughton
Copy link
Contributor Author

oh & also - it fixes a bug & a dodgey-ness

@colemanw colemanw merged commit f10342c into civicrm:5.54 Sep 26, 2022
@colemanw colemanw deleted the import branch September 26, 2022 23:35
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