core#1364 : 'Merge All Contacts with the Same Address' doesn't consider Household replace individuals that share same address. #15725
Conversation
|
(Standard links)
|
|
@monishdeb test fails relate I like the fact there is a new test for this! |
|
Jenkins test this please |
| INNER JOIN $tableName r2 ON adr.contact_id = r2.civicrm_primary_id | ||
| INNER JOIN civicrm_address adr1 ON r1.civicrm_primary_id = adr1.contact_id AND r1.master_id IS NOT NULL | ||
| INNER JOIN civicrm_address adr2 ON adr2.id = adr1.master_id | ||
| INNER JOIN $tableName r2 ON adr2.contact_id = r2.civicrm_primary_id |
There was a problem hiding this comment.
Due to recent refactoring changes master_id in the exported temp table holds the display name of contact whose address is shared. This is because of formating change in L978 above. Thus this particular query fails, the reason why we used additional INNER JOIN to fetch and JOIN on correct master_id.
I started digging into civicrm#15725 and found that a deprecated methodology was used for the test. I got as far as re-writing the test - although the key lines are commented out as the fix from the above PR is not yet merged
|
@monishdeb I started digging into this but in the first 2 lines you bring back a test-hack we got rid of - I've done an alternate version of the test for you #15948 which doesn't use the hack & instead tests the outputted csv. When we were in NY Tim pointed out that by throwing an exception when CiviExit was called when running from a Unit test we could avoid the 'exit' that has been causing us such problems & led to hacks like the suppress_csv_for_testing & instead we can actually test the csv! I also realised (& got side tracked on) noticing you were fixing an old regression. I'm going to dig a little more. My thinking is that instead of master_id being swapped out when it is swapped out we should do that much later on. The irony is all that hard-to-read code around address merging got left as it was because we didn't understand what it was doing but didn't want to break it .... and it turned out to be broken! |
I'm trying to review civicrm#15725 but through no fault of that PR it hits on one of the parts of the Export code that I still find unreadable :-( This is a further readability fix & it starts to point me to how to get past the underlying WTF about this code. Note that I removed the IF around sharedAddress. From Monish's PR I found out that we have a fairly long-standing regression where the sharedAddress code doesn't actually work :-( so I'm comfortable this won't break anything :-). We also have good test cover on this chunk from probably around when the shared address part got broken.... Looking in the UI there is no implication that the greeting for a shared address would display differently - which is the only explanation I can think of for different handling here. Of course until we remove the later IF the shared address would do things differently - erm if it worked. ALl of which is a long way of saying the removal of the IF won't change anything
I'm trying to review civicrm#15725 but through no fault of that PR it hits on one of the parts of the Export code that I still find unreadable :-( This is a further readability fix & it starts to point me to how to get past the underlying WTF about this code. Note that I removed the IF around sharedAddress. From Monish's PR I found out that we have a fairly long-standing regression where the sharedAddress code doesn't actually work :-( so I'm comfortable this won't break anything :-). We also have good test cover on this chunk from probably around when the shared address part got broken.... Looking in the UI there is no implication that the greeting for a shared address would display differently - which is the only explanation I can think of for different handling here. Of course until we remove the later IF the shared address would do things differently - erm if it worked. ALl of which is a long way of saying the removal of the IF won't change anything
|
@monishdeb I'm still struggling with the merge code - mostly because it was the part of the code that wasn't really touched that much in the code cleanup & when I look at all that wrangling I remain far too confused :-( None of this is your fault, or the fault of this PR but I think you are going to have to bear with me some more as I work through this. I did put up one simplification PR & I think I can see my way to getting that function to something easy (hopefully) to read #15949 However, I think I have a train of thoughts that relates to this PR rather than just the general. When we merge to household I understood the goal was simply to add the household name into the merged names ie Dear Hudson Household and Eileen McNaughton but this looks like it replaces the merged contact with the household. Which got me thinking. Are we saying that 'merge household members' is like a subset of 'merge same address' and when we choose 'merge same address' we should effectively ALSO set isMergeSameHousehold to true - so everything that happens on mergeSameHousehold takes place & then a bit extra fro the address stuff?
|
I'm trying to review civicrm#15725 but through no fault of that PR it hits on one of the parts of the Export code that I still find unreadable :-( This is a further readability fix & it starts to point me to how to get past the underlying WTF about this code. Note that I removed the IF around sharedAddress. From Monish's PR I found out that we have a fairly long-standing regression where the sharedAddress code doesn't actually work :-( so I'm comfortable this won't break anything :-). We also have good test cover on this chunk from probably around when the shared address part got broken.... Looking in the UI there is no implication that the greeting for a shared address would display differently - which is the only explanation I can think of for different handling here. Of course until we remove the later IF the shared address would do things differently - erm if it worked. ALl of which is a long way of saying the removal of the IF won't change anything
|
@eileenmcnaughton yes exactly, the 'Merge Contacts with the Same address' is the superset, means it does two things:
The 'Merge Households members into their Households' only fulfill the #2 criteria. If you check the helpicon text of 'Merge options' it tells us the same story, Use one of the merge contact options when exporting records to reduce your mailing size by sending a single piece of mail to each address. Merge Contacts with the Same Address will combine any records having the same address (street address, city, postal code, country) into a single record. If a household record already exists in which multiple individuals share an address, the household will be exported as the combined record. If no household record exists, the records will be combined and the Addressee field will list the contact names, comma-separated. This is as per how the 'Merge option' feature was introduced in the core. @lcdservices am I right? |
|
@monishdeb if that IS the case then I think we should try setting isMergeSameHousehold = TRUE when isMergeSameAddress is TRUE in the ExportProcessor constructor |
|
@eileenmcnaughton I did that earlier, but it gives me some unexpected results in the output. Let me give it another try. |
|
Jenkins test this please |
…er Household replace individuals that share same address.
Revised version of test from #15725
|
I want to clarify the expected behavior outlined in #15725 (comment) If merge all contacts with same address is checked, the expected behavior is:
That's slightly different than what is described in that earlier comment -- though I don't know which the code currently matches. The intent was to always allow an "explicit" merge -- namely through household relationships. But if no explicit merge has been defined, we force merge remaining contacts based on the address values. |
…useholds This is an alternate to civicrm#15725 that still achieves the goal in civicrm#15725 (comment) of making is so that the mergeSharedAddress option mergesHouseholds first & then merges sharedAddresses. In civicrm#15725 @monishdeb was fixing the code that differentiated between how master_id was treated versus otherwise merged addresses. However that code 'broke' a long time ago and the arguably broken behaviour is locked in by unit tests. Fixing it has not been requested - in this issue or any other in the past year or 2 when it has been broken so I opted to remove the broken code entirely & to open up the next query such that it does not exclude household addresses. This didn't break any tests (woot) and also allowed me to enable the test rows @monishdeb wrote to test this issue
|
We can close this now as your work was absorbed into #16369 @monishdeb |
Overview
Steps to replicate:
Before
3 records i.e. combined record of (A,B,C) , D and E (Incorrect)
After
2 records i.e. D and E. Because D has a same address as of A,B and C so it would replace these 3. E is a household though but has different address so it should not be merged
Technical Details
This PR solves an old issue which was not fixed and previously the old PR #11901 was dropped because the export code itself needs improvements. So I have made the changes on top of refactored code which utilizes ExportProcessor to do the essential job to merge and export contacts on basis of address or household.
Comments
ping @lcdservices @eileenmcnaughton