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

dev/core#4227 Fix import to not blank birth_date etc on update #26172

Merged
merged 1 commit into from May 12, 2023

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

dev/core#4227 Fix import to not blank birth_date etc on update

Before

per
https://lab.civicrm.org/dev/core/-/issues/4227

After

Fixed according to the parameters of https://lab.civicrm.org/dev/core/-/issues/4227

Technical Details

This is some weird dark code. However, @bjendres has pin pointed that there was a behaviour change & the code is way clearer with this removed.

I initially worried that there might be an expectation at some point that the fields be blanked - but the fact an allowList is specifically configured makes it seem unlikely that this is anything other than a regression from the intended behaviour

Interestingly the test had that value set in the test.

Comments

@civibot
Copy link

civibot bot commented May 8, 2023

No issue was found matching the number given in the pull request title. Please check the issue number.

@civibot
Copy link

civibot bot commented May 8, 2023

(Standard links)

@civibot civibot bot added the master label May 8, 2023
@eileenmcnaughton eileenmcnaughton changed the base branch from master to 5.62 May 8, 2023 05:41
@civibot civibot bot added 5.62 and removed master labels May 8, 2023
if (in_array($key, ['nick_name', 'job_title', 'middle_name', 'birth_date', 'gender_id', 'current_employer', 'prefix_id', 'suffix_id'])
&& ($value == '' || !isset($value)) &&
($session->get('authSrc') & (CRM_Core_Permission::AUTH_SRC_CHECKSUM + CRM_Core_Permission::AUTH_SRC_LOGIN)) == 0 ||
($key === 'current_employer' && empty($params['current_employer']))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current_employer should not show up anymore actually - so could probably go from above too but maybe in master

@eileenmcnaughton
Copy link
Contributor Author

@MegaphoneJon you might also have an interest?

@demeritcowboy demeritcowboy added the merge ready PR will be merged after a few days if there are no objections label May 10, 2023
@demeritcowboy
Copy link
Contributor

I've put merge-ready.
Aside: Not from this PR but the help text for update/fill has gone missing. I'll see if I can see what's going on there.

@yashodha
Copy link
Contributor

@eileenmcnaughton looks good, merging this PR.

@yashodha yashodha merged commit 625954e into civicrm:5.62 May 12, 2023
3 checks passed
@eileenmcnaughton eileenmcnaughton deleted the 562_import branch May 12, 2023 22:45
@eileenmcnaughton
Copy link
Contributor Author

thanks @yashodha

@demeritcowboy
Copy link
Contributor

PR for the missing help text: #26252

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.62 merge ready PR will be merged after a few days if there are no objections
Projects
None yet
3 participants