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

CRM-19933 proposed fix for import wiping out preferred comm method (possibly CRM-20035 too) #10731

Merged
merged 1 commit into from
Aug 31, 2017

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jul 24, 2017

@lcdservices just took a quick look at the prefferred_communication_method issue & it appears to me that we need to stop the BAO setting it & then force-set it on the 2 forms that display it for editing (ie QF is trimming it)

No unit test as yet :-(



@eileenmcnaughton eileenmcnaughton changed the title CRM-19933 proposed fix for import wiping out preferred comm method CRM-19933 proposed fix for import wiping out preferred comm method (possibly CRM-20035 too) Aug 29, 2017
@monishdeb
Copy link
Member

monishdeb commented Aug 29, 2017

Did style fix and tested CRM-19933 and CRM-20035, working fine. @eileenmcnaughton are you going to add unit test for it?

@monishdeb
Copy link
Member

Seems like 2 test build failures is related.

@eileenmcnaughton
Copy link
Contributor Author

There is some nasty magic in the deprecated formatArrayKeys that we need to keep to keep behaviour unchanged. Since that is not the main part of the PR I'm reverting that change

@eileenmcnaughton
Copy link
Contributor Author

@monishdeb I have added a test to ensure it is possible to set this to null via the api (which the form layer doesn't use but it does share the relevant code. In doing so I found some duplicate handling in the api which was causing problems with separators being appended at the api layer and in the BAO. I had to remove that to get the null test to pass.

Can you re-review if these tests pass?

@monishdeb
Copy link
Member

Sure

@monishdeb
Copy link
Member

Tested for CRM-19933 and CRM-20035, working fine.

@monishdeb monishdeb merged commit da3249e into civicrm:master Aug 31, 2017
@eileenmcnaughton eileenmcnaughton deleted the pref_comm branch August 31, 2017 21:12
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.

3 participants