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

Towards fixing household merge export, extract function, add test, fix prev #12272

Merged
merged 1 commit into from
Jun 7, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Follow up on #12212 - extend testing, further function extraction, fix mistake in that PR

Before

functions

After

City & State province field tested, function extracted

Technical Details

@monishdeb I think I found an error in the previous commit see

|| empty($value[4]))) {

if (array_key_exists($relationshipTypes, $contactRelationshipTypes) && (!empty($value[2]) || empty($value[4]))) {

but reading the code it should be !empty

Comments

This needs to be merged for the 5.3 rc which will be cut tomorrow. Let's see if it can be reviewed in time or we need an rc pr

@civibot
Copy link

civibot bot commented Jun 6, 2018

(Standard links)

@monishdeb
Copy link
Member

Oops, my bad. Let me test it on UI.

@eileenmcnaughton
Copy link
Contributor Author

@monishdeb I just merged this to master - #12274 - you should grab it as you may hit the notices

@eileenmcnaughton
Copy link
Contributor Author

@monishdeb 5.3 is branched - the one part of this that fixes a mistake should go against the rc - will wait on your review & we can organise

@monishdeb
Copy link
Member

I have tested the City & State province field for 'Merge All Contacts with the Same Address' and 'Merge Household Members into their Households' scenarios and it produce expected result w/o notices + the UT ensure the fix.

@monishdeb monishdeb merged commit c898924 into civicrm:master Jun 7, 2018
@eileenmcnaughton eileenmcnaughton deleted the export_house branch June 7, 2018 06:21
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