-
-
Notifications
You must be signed in to change notification settings - Fork 807
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
dev/core#3939 Fix import mandatory field validation regression #24838
Conversation
(Standard links)
|
f362928
to
7d52d3c
Compare
7d52d3c
to
53809cc
Compare
test this please |
@eileenmcnaughton thank you for this fix! Importing works well now. However, one minor thing from the issue is still open:
If I select email in the field mapping and try to save it (either by updating an existing import field mapping or create a new one) the email field is not saved within the mapping. This is a minor issue which is not necessarily a blocker, but it would be nice to get this one fixed as well. |
@colemanw @seamuslee001 see above - @Detsieber has tested - can someone merge? @Detsieber can you create a new gitlab issue for the outstanding part? Then we can close the main issue without losing that |
Actually I put something up for the second part - so if we merge that don't need a new ticket #24871 |
Thanks a lot @eileenmcnaughton , I have tested this and also works well! |
Overview
Fix import mandatory field validation regression
Before
Despite email being a valid match it does not pass the mandatory field validation rule
After
The mapping above passes validation
Technical Details
We are now using apiv4 under the hood. The api v4 field name is
email_primary.email
- but presenting that field name in the import mapping ui causes a js error due to the.
having another meaning in js. The work around is to replace it at the js layer with__
& swap it back in the form layer to process normally. (The angular layer just uses the 'right' name).However, it seems this handling was missing in the validate function so I have added there
Comments
@seamuslee001 this should be an easy merge - I have UI-tested against 5.55 rc & added unit test
5.54 affect as issue was reported against it - #24839
https://lab.civicrm.org/dev/core/-/issues/3939