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#4156 Fix failure to update organization_name on employees during merge #25778

Merged
merged 1 commit into from
Mar 10, 2023

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

dev/core#4156 Fix failure to update organization_name on employees during merge

Before

  1. set an employer's relationship to an organization
  2. merge that organization into another organization (you can generally do an organization search that is narrow enough to find your organization & merge it into any other one)
  3. check your individual - their relationship will be updated & employer_id but not organization_name - so it shows incorrectly on the contact summary

After

organization name updated

Technical Details

Passing in organization_name triggers the code to update the employee record. This is kinda black-magicky but the test protects it

Comments

There is some suggestion this regressed at some point but I couldn't see anything in the code to point me to it -I think 5.60 with no backport makes sense

@civibot
Copy link

civibot bot commented Mar 10, 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 Mar 10, 2023

(Standard links)

@totten
Copy link
Member

totten commented Mar 10, 2023

@eileenmcnaughton I did a local r-run on 5.60 with and without the patch. I'm not seeing any difference in behavior. For reference, here was the procedure:

  • Login. Search for some organizations and related contacts in sample data. Pick a few.
    Organization Employee
    California Sports Trust (#87) Herminia Adams (#95)
    Bay Sustainability Partners (#140) Angelika Jacobs (#185)
  • Open the four contacts in different tabs, eg
    http://dmaster.bknix:8001/civicrm/contact/view?reset=1&cid=87
    http://dmaster.bknix:8001/civicrm/contact/view?reset=1&cid=95
    http://dmaster.bknix:8001/civicrm/contact/view?reset=1&cid=140
    http://dmaster.bknix:8001/civicrm/contact/view?reset=1&cid=185
    
  • In the "Find Contacts (type=Organization)", pick those two orgs and merge them. This goes to a URL like:
    http://dmaster.bknix:8001/civicrm/contact/merge?reset=1&cid=140&oid=87   or...
    http://dmaster.bknix:8001/civicrm/contact/merge?reset=1&cid=87&oid=140
    
  • Merge the contacts.
  • Reload the contact tabs.

For both Herminia (#95) and Angelika (#185), the employer link looks the same as before.

If you go to edit the employer, then it does refresh with a newer name. But the persistent value seems unchanged.

These observations were the same with or without the patch.

@eileenmcnaughton
Copy link
Contributor Author

Hmm - that might be what the test fail is picking up - it works differently on the api action with the patch

@eileenmcnaughton eileenmcnaughton force-pushed the 560 branch 4 times, most recently from 40c428b to efaf2ed Compare March 10, 2023 01:32
@totten
Copy link
Member

totten commented Mar 10, 2023

@eileenmcnaughton OK, re-running, it's 50% better.

  • With http://dmaster.bknix:8001/civicrm/contact/merge?reset=1&cid=140&oid=87

    • ✔️ Herminia (#95) switches to "Bay Sustainability Partners"
    • ✔️ Angelika (#185) stays on "Bay Sustainability Partners"
  • With http://dmaster.bknix:8001/civicrm/contact/merge?reset=1&cid=87&oid=140

    • ✔️ Herminia (#95) stays on "California Sports Trust"
    • 🛑 Angelika (#185) should have changed to "California Sports Trust" but didn't.

@eileenmcnaughton
Copy link
Contributor Author

Ug that's horrible - there is no way I can see that being a recent regression either. The only way I can see this line

CRM_Contact_BAO_Contact_Utils::updateCurrentEmployer($contact->id);
not doing both of them is if angelika has no employer_id

There is some crazy magic here

//update current employer field
if ($currentEmloyerId = CRM_Utils_Array::value('current_employer_id', $submitted)) {
if (!CRM_Utils_System::isNull($currentEmloyerId)) {
$submitted['current_employer'] = $submitted['current_employer_id'];
}
else {
$submitted['current_employer'] = '';
}
unset($submitted['current_employer_id']);
}
that does .... something

@totten
Copy link
Member

totten commented Mar 10, 2023

FWIW, it looks like both start out with the expected employer_id. Here's a dump of everything that comes to mind (before merge), though nothing stands out as different.

mysql> select id, contact_type, contact_sub_type, display_name , employer_id , organization_name  from civicrm_contact where id in (95,185);
+-----+--------------+------------------+---------------------+-------------+-----------------------------+
| id  | contact_type | contact_sub_type | display_name        | employer_id | organization_name           |
+-----+--------------+------------------+---------------------+-------------+-----------------------------+
|  95 | Individual   | NULL             | Ms. Herminia Adams  |          87 | California Sports Trust     |
| 185 | Individual   | NULL             | Dr. Angelika Jacobs |         140 | Bay Sustainability Partners |
+-----+--------------+------------------+---------------------+-------------+-----------------------------+

mysql> select * from civicrm_relationship where contact_id_a in (87,95,140,185);
+-----+--------------+--------------+----------------------+------------+----------+-----------+-------------+-------------------+-------------------+---------+---------------------+---------------------+
| id  | contact_id_a | contact_id_b | relationship_type_id | start_date | end_date | is_active | description | is_permission_a_b | is_permission_b_a | case_id | created_date        | modified_date       |
+-----+--------------+--------------+----------------------+------------+----------+-----------+-------------+-------------------+-------------------+---------+---------------------+---------------------+
| 203 |           95 |           87 |                    5 | NULL       | NULL     |         1 | NULL        |                 0 |                 0 |    NULL | 2023-01-24 16:10:18 | 2023-01-24 16:10:18 |
| 208 |          185 |          140 |                    5 | NULL       | NULL     |         1 | NULL        |                 0 |                 0 |    NULL | 2023-01-24 16:10:18 | 2023-01-24 16:10:18 |
+-----+--------------+--------------+----------------------+------------+----------+-----------+-------------+-------------------+-------------------+---------+---------------------+---------------------+
2 rows in set (0.01 sec)

mysql> select * from civicrm_relationship_cache where near_contact_id in (95,185);
+-----+-----------------+----------------------+-------------+-----------------+---------------+----------------+--------------+-----------+------------+----------+---------+
| id  | relationship_id | relationship_type_id | orientation | near_contact_id | near_relation | far_contact_id | far_relation | is_active | start_date | end_date | case_id |
+-----+-----------------+----------------------+-------------+-----------------+---------------+----------------+--------------+-----------+------------+----------+---------+
| 405 |             203 |                    5 | a_b         |              95 | Employee of   |             87 | Employer of  |         1 | NULL       | NULL     |    NULL |
| 415 |             208 |                    5 | a_b         |             185 | Employee of   |            140 | Employer of  |         1 | NULL       | NULL     |    NULL |
+-----+-----------------+----------------------+-------------+-----------------+---------------+----------------+--------------+-----------+------------+----------+---------+

In the errant case, I can also confirm that the employer_id is updated as expected. It's just the organization_name that stays behind.

@eileenmcnaughton
Copy link
Contributor Author

@totten I think we should merge this as is to the rc & consider coming back to it in master - it is better & I'm less & less convinced it is a regression

@totten
Copy link
Member

totten commented Mar 10, 2023

@eileenmcnaughton Yup, it is better, and it has test coverage. So better in than out.

@totten totten merged commit b92a611 into civicrm:5.60 Mar 10, 2023
@eileenmcnaughton eileenmcnaughton deleted the 560 branch March 10, 2023 21:51
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.

2 participants