core/issues/80 - Current Employer is not reset after relationship is … #12032

jitendrapurohit · 2018-04-26T12:28:19Z

…updated

Overview

Current Employer is not reset after relationship is updated.

Before

On editing a relationship type from Employer of to some other value, current employer value is still shown for the contact.

After

Current Employer is deleted when relationship type is updated.

Technical Details

Moved the logic to BAO layer as commented on CRM_Contact_Form_Relationship-> clearCurrentEmployer function.

Comments

Added unit test.

Gitlab issue - https://lab.civicrm.org/dev/core/issues/80

eileenmcnaughton · 2018-04-26T20:01:11Z

CRM/Contact/Form/Relationship.php

-    // @todo this belongs in the BAO.
-    if ($this->_isCurrentEmployer) {
-      $relChanged = $params['relationship_type_id'] != $this->_values['relationship_type_id'];
-      if (!$params['is_active'] || !$params['is_current_employer'] || $relChanged) {


the $params['is_current_employer'] functionality does seem to be form-relevant - ie there could be 2 employers & one (or possibly neither?) could be form-selected - I do like the move to bao for rel type change tho

Tested on assigning relationship through search results and we don't offer current employer checkbox on that form. I found another issue(before this PR) which failed to remove current employer when the checkbox is unchecked while editing the relationship. It has been fixed in the changes made here.

@jitendrapurohit just spotted one more thing - looks like the original code would also clear current employer if toggled to inactive (presumably date changes could trigger that too but I think that could stay out of scope since it was not part of the previous behaviour)

Thanks, this brings another related issue on dmaster which fails to reset current employer after is_active checkbox is unchecked and the form is saved. Added fix for the same to this PR.

eileenmcnaughton · 2018-04-30T11:22:39Z

CRM/Contact/BAO/Relationship.php

+    if (!empty($relationshipId)) {
+      $existingTypeID = CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_Relationship', $relationshipId, 'relationship_type_id');
+      $existingTypeName = CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_RelationshipType', $existingTypeID, 'name_b_a');
+      if ($existingTypeName == 'Employer of' && (empty($params['is_active']) || empty($params['is_current_employer']) || $existingTypeID != $type)) {


I think this needs to be

if (empty($params['is_current_employer']) && $existingTypeName == 'Employer of' && (empty($params['is_active']) || $existingTypeID != $type))

Or else it gets hit when you do an api update. 'is_current_employer' is basically an override that can be passed in & which will skip that clear behaviour.

There is a weirdness with the form that I think you could assign ANY relationship with current_employer in the form but then it might be wiped by an api update of some sort - but I don't know how valid that path is - seems pretty obscure really

In the above format - it fails on the form layer and doesn't remove employer_id when is_current_employer is unchecked.

The change is now modified to look for 'is_current_employer' variable on the form itself instead of BAO and clear the employer if it is not enabled. Tested various scenarios and looks fine to me.

eileenmcnaughton · 2018-05-01T21:06:24Z

CRM/Contact/BAO/Relationship.php

@@ -304,7 +304,7 @@ public static function add(&$params, $ids = array(), $contactId = NULL) {
    if (!empty($relationshipId)) {
      $existingTypeID = CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_Relationship', $relationshipId, 'relationship_type_id');
      $existingTypeName = CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_RelationshipType', $existingTypeID, 'name_b_a');
-      if (empty($params['is_current_employer']) && $existingTypeName == 'Employer of' && (empty($params['is_active'])  || $existingTypeID != $type)) {
+      if ($existingTypeName == 'Employer of' && (empty($params['is_active']) || $existingTypeID != $type)) {


Don't we still need to have !$params['is_current_employer'] here to respect any flag from the form?

Updated the PR which should respect both API and form params. A new function is added which aims to decide if we need to set the current employer for the relationship or not. Tagging it as wip until I do a manual testing for some good number of scenarios. As a quick testing on the contact summary form, it worked fine for me.

jitendrapurohit · 2018-05-10T09:46:52Z

Done initial testing from my side and looks good on API as well as on form layer. removing wip from the title.

eileenmcnaughton · 2018-05-10T23:12:13Z

CRM/Contact/BAO/Relationship.php

+
+    $isExpired = FALSE;
+    if (!empty($params['end_date']) && strtotime($params['end_date']) < time()) {
+      $isExpired = TRUE;


Stylistically it might be nicer to return FALSE here

eileenmcnaughton · 2018-05-10T23:14:58Z

CRM/Contact/BAO/Relationship.php

+   * @return bool
+   *   TRUE if current employer needs to be enabled.
+   */
+  public static function isCurrentEmployerSetInParams($params, $relationshipId, $updatedRelTypeID = NULL) {


I feel like the function name is 'isCurrentEmployerNeedingToBeCleared'

eileenmcnaughton · 2018-05-10T23:24:03Z

@jitendrapurohit I took a look at this & tested it & it seems good. I had some readability concerns - ie. the function name seems wrong. Also, I think you could make that IF more readable if you used early returns instead.

ie

if ($existingTypeName !==  'Employer of') {
  return FALSE;
}

if (!empty($params['is_current_employer']))) {
  return FALSE;
}

I guess that doesn't quite work with your efforts to share with the form function? But I feel the readability is a bit tricky.

Also I note that calling

self::clearCurrentEmployer($relationshipId, CRM_Core_Action::DELETE);

really just does some unnecessary work & then wraps

CRM_Contact_BAO_Contact_Utils::clearCurrentEmployer($relationship->contact_id_a);

jitendrapurohit · 2018-05-11T10:06:45Z

Updated the PR as per above comments. Seems returning early for !empty($params['is_current_employer']) would not unset the current employer if checkbox is enabled but the relationship is expired. I've tested the latest update for relationship create/update and it seems to be working fine 👍

eileenmcnaughton · 2018-05-11T11:21:07Z

OK - I think this is good to merge - maybe squash the commits first?

…updated Additional fixes Additional fixes

jitendrapurohit · 2018-05-11T11:24:06Z

yep - done now.

colemanw · 2018-05-11T14:10:22Z

@civicrm-builder retest this please

eileenmcnaughton · 2018-05-11T21:03:52Z

test this please

eileenmcnaughton reviewed Apr 26, 2018

View reviewed changes

jitendrapurohit force-pushed the core-80 branch 2 times, most recently from 76bb1ef to 2b8f8bd Compare April 30, 2018 11:03

eileenmcnaughton reviewed Apr 30, 2018

View reviewed changes

jitendrapurohit force-pushed the core-80 branch from 2b8f8bd to 90b1ad8 Compare May 1, 2018 11:08

eileenmcnaughton reviewed May 1, 2018

View reviewed changes

jitendrapurohit force-pushed the core-80 branch 2 times, most recently from b2c60b0 to 7aedab9 Compare May 3, 2018 13:21

jitendrapurohit changed the title ~~core/issues/80 - Current Employer is not reset after relationship is …~~ wip - core/issues/80 - Current Employer is not reset after relationship is … May 3, 2018

jitendrapurohit changed the title ~~wip - core/issues/80 - Current Employer is not reset after relationship is …~~ core/issues/80 - Current Employer is not reset after relationship is … May 10, 2018

eileenmcnaughton reviewed May 10, 2018

View reviewed changes

jitendrapurohit force-pushed the core-80 branch from 15b00cb to 6aa9b51 Compare May 11, 2018 10:09

core/issues/80 - Current Employer is not reset after relationship is …

df0c42c

…updated Additional fixes Additional fixes

jitendrapurohit force-pushed the core-80 branch from 6aa9b51 to df0c42c Compare May 11, 2018 11:23

eileenmcnaughton added the merge on pass label May 11, 2018

colemanw merged commit 17e1efd into civicrm:master May 14, 2018

jitendrapurohit deleted the core-80 branch May 15, 2018 03:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/issues/80 - Current Employer is not reset after relationship is … #12032

core/issues/80 - Current Employer is not reset after relationship is … #12032

jitendrapurohit commented Apr 26, 2018 •

edited

Loading

eileenmcnaughton Apr 26, 2018

jitendrapurohit Apr 30, 2018

eileenmcnaughton Apr 30, 2018

jitendrapurohit Apr 30, 2018

eileenmcnaughton Apr 30, 2018

jitendrapurohit May 1, 2018

eileenmcnaughton May 1, 2018

jitendrapurohit May 3, 2018

jitendrapurohit commented May 10, 2018 •

edited

Loading

eileenmcnaughton May 10, 2018

eileenmcnaughton May 10, 2018

eileenmcnaughton commented May 10, 2018

jitendrapurohit commented May 11, 2018 •

edited

Loading

eileenmcnaughton commented May 11, 2018

jitendrapurohit commented May 11, 2018

colemanw commented May 11, 2018

eileenmcnaughton commented May 11, 2018

core/issues/80 - Current Employer is not reset after relationship is … #12032

core/issues/80 - Current Employer is not reset after relationship is … #12032

Conversation

jitendrapurohit commented Apr 26, 2018 • edited Loading

Overview

Before

After

Technical Details

Comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jitendrapurohit commented May 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented May 10, 2018

jitendrapurohit commented May 11, 2018 • edited Loading

eileenmcnaughton commented May 11, 2018

jitendrapurohit commented May 11, 2018

colemanw commented May 11, 2018

eileenmcnaughton commented May 11, 2018

jitendrapurohit commented Apr 26, 2018 •

edited

Loading

jitendrapurohit commented May 10, 2018 •

edited

Loading

jitendrapurohit commented May 11, 2018 •

edited

Loading