CRM-18338 Fix test CRM_Dedupe_MergerTest

John K. as I tried to fix those tests, am really confused on what scenario you would expect $migrationInfo array holds location parameters under 'location' key BUT as per your earlier changes you have changed the key to 'location_blocks' from 'location' as in https://github.com/civicrm/civicrm-core/pull/7220/files#diff-874dc48dd188118ab0ba9cb9bbde9a02R1260 and in other places. Reason why the tests are throwing notice error : undefined key 'location'.

Does this $migrationInfo can hold location information under both 'location' and 'location_blocks' keys or in other words does these two keys holds separate meaning ? Am pretty sure that this two tests
CRM_Dedupe_MergerTest.testBatchMergeSelectedDuplicates
CRM_Dedupe_MergerTest.testBatchMergeAllDuplicates

are failing afterrefactoring changes done in https://github.com/civicrm/civicrm-core/pull/7220

Hi Monish.

Yes, I expect those changes we did caused the test to break. I didn't pick up on it at the time. It was a real nightmare doing this refactor, but I think we did manage to improve the workflow (and certainly fixed a few bugs along the way).

I think more inline documentation about these keys is still required. I was trying to balance the level of refactoring with just getting it working, which is why some of the original odd logic remains. The dedupe process certainly needs a lot of work still.

If my memory serves me correctly, location_blocks holds all the current location blocks for that contact. The 'location' key holds all the changes that are being made to location blocks as part of the merge. But I'd have to take a deep dive into this again to fully understand what was going on.

Ya, should I assign this issue to ya ? However I was thinking will be appropriate restrict the assignment of $migrationInfo['location'] under array_key_exists('location', $migrationInfo) IF loop to kill these two test units and I won't think it would be a hackish fix as we just ensuring that we rely ONLY on $migrationInfo['location'] for retrieving desired information as per the current code. What ya say ?

Can give it a go. I'll understand better if I see it. Feel free to give it a shot and I can run the tests and take a look as part of the QA.

Submitted the PR https://github.com/civicrm/civicrm-core/pull/8083 including 2 test fixes CRM_Dedupe_MergerTest.*

John K. can you please test my patch ?

Monish Deb hmn, not quite doing the trick.
I don't think we can assign the 'operation' like that. I just tested a dedupe which was supposed to add a 'home mobile' to a person without one and it was not added.

Ya technically I have kept the 'operation' assignment intact like it was earlier. Like earlier if operation is null then it simply assign 2. Now !isset($migrationInfo['location']) then assign 2 which holds the same meaning. However I think we need to change the location to location_block key as per https://github.com/civicrm/civicrm-core/pull/7220/files#diff-874dc48dd188118ab0ba9cb9bbde9a02R1260 OR we need to lookup for operation value both in $migration['location'] and $migrationInfo['location_block']. This are the only two workaround I have for now

I'll have another crack at this, diving back into the location / location_blocks jumble. I'll try to get a bit more inline documentation in the code at the same time.

I'm reluctant to do too much in the way of changing the code itself, because it works as it is at the moment.

The proposed patch also breaks 'overwriting' an email address; which was working beforehand.

I'll fix up this first... CRM-18455

Righto, so the tests are passing. And here are the manual tests that I did. It would be good to re-test all of this during QA - along with anything else that I haven't thought of. These tests could also be a good starting point for any further unit/web tests Eileen McNaughton ?

• Merge screen JS displays matching locations on the 'main' contact when 'location' and/or 'type' is changed (all location block entities)
• For each location entity test: overwrite existing, add new, and both of those from different 'original' locations. eg:
• * home address OVER home address
• * work address OVER home address
• * home phone OVER home phone
• * home phone ADD NEW home phone
• * home phone OVR other mobile
• * home phone NEW other fax
• * etc.
• Check safe merge works and catches exceptions
• Check force merge works

The current logic for force merge (before the PR and maintained in the PR) is that all location fields are added, not overwritten. And any conflicts in core details (or the address) are ignored. The details in the 'main' contact supersede those of the 'other' in all cases.

For example:

Other (dupe):
Name: Cat Man
Job title: Villain
Home address: Kittentown
Home phone: cat-123
Work phone: litter-321

Main:
Name: Bat Man
Job title: Hero
Home address: Gotham
Home phone: 123-bat
Work phone: 321-bat

Force merge result:
Name: Bat Man
Job title: Hero
Home address: Gotham
Home phone: 123-bat
Home phone: cat-123
Work phone: 321-bat
Work phone: litter-321

It might be better to force merge based on which contact was most recently updated - and then allow overwriting of matching phones/emails. This would be potentially more destructive, but less messy. It might also result in better information in the core details (ie: the job title from the most recently updated contact would persist). This is something to consider in a future/follow-up issue.

QA NOTES:

I've just attached a visual of the merge screen. This patch https://github.com/civicrm/civicrm-core/pull/8192/commits/bf43eaacbc0a23d40825629c1bd33a992056064a alters (& simplifies) the logic of the code that causes changing the location to load the details for the right hand side contact into the space on the right hand side for that location.

If there is existing data it causes it to say 'overwrite' or 'add' for no existing data. I tested & did not observe a change after applying that commit, either on the original screen or on the screen presented after I clicked flip.

I am working through the patches 'from the top' & feel the first 2 are fine