From 2058bf545dd527f93bf685a9ac11c9bc4bd2a49a Mon Sep 17 00:00:00 2001 From: DemeritCowboy Date: Wed, 16 Oct 2019 22:57:59 -0400 Subject: [PATCH] fix name vs label for case roles --- CRM/Case/ManagedEntities.php | 8 +- CRM/Case/XMLProcessor.php | 23 +- CRM/Case/XMLProcessor/Process.php | 97 +++++--- CRM/Utils/Check/Component/Case.php | 235 ++++++++++++++++++ ang/crmCaseType.js | 211 +++++++++++----- ang/crmCaseType/rolesTable.html | 2 +- ang/crmCaseType/timelineTable.html | 2 +- tests/karma/unit/crmCaseTypeSpec.js | 174 +++++++++---- tests/phpunit/CRM/Case/BAO/CaseTest.php | 62 +++++ .../CRM/Case/XMLProcessor/ProcessTest.php | 190 ++++++++++++-- tests/phpunit/CRM/Case/XMLProcessorTest.php | 51 ++++ 11 files changed, 880 insertions(+), 175 deletions(-) create mode 100644 tests/phpunit/CRM/Case/XMLProcessorTest.php diff --git a/CRM/Case/ManagedEntities.php b/CRM/Case/ManagedEntities.php index 15bc97354f..be0d846cb1 100644 --- a/CRM/Case/ManagedEntities.php +++ b/CRM/Case/ManagedEntities.php @@ -111,11 +111,11 @@ class CRM_Case_ManagedEntities { $result = []; if (!isset(Civi::$statics[__CLASS__]['reltypes'])) { - $relationshipInfo = CRM_Core_PseudoConstant::relationshipType('label', TRUE, NULL); + $relationshipInfo = CRM_Core_PseudoConstant::relationshipType('name', TRUE, NULL); foreach ($relationshipInfo as $id => $relTypeDetails) { - Civi::$statics[__CLASS__]['reltypes']["{$id}_a_b"] = $relTypeDetails['label_a_b']; - if ($relTypeDetails['label_a_b'] != $relTypeDetails['label_b_a']) { - Civi::$statics[__CLASS__]['reltypes']["{$id}_b_a"] = $relTypeDetails['label_b_a']; + Civi::$statics[__CLASS__]['reltypes']["{$id}_a_b"] = $relTypeDetails['name_a_b']; + if ($relTypeDetails['name_a_b'] != $relTypeDetails['name_b_a']) { + Civi::$statics[__CLASS__]['reltypes']["{$id}_b_a"] = $relTypeDetails['name_b_a']; } } } diff --git a/CRM/Case/XMLProcessor.php b/CRM/Case/XMLProcessor.php index 245e9ace1c..577b8610c4 100644 --- a/CRM/Case/XMLProcessor.php +++ b/CRM/Case/XMLProcessor.php @@ -91,12 +91,10 @@ class CRM_Case_XMLProcessor { } /** - * Get all relationship type labels - * - * TODO: These should probably be names, but under legacy behavior this has - * been labels. + * Get all relationship type display labels (not machine names) * * @param bool $fromXML + * TODO: This parameter is always FALSE now so no longer needed. * Is this to be used for lookup of values from XML? * Relationships are recorded in XML from the perspective of the non-client * while relationships in the UI and everywhere else are from the @@ -106,11 +104,24 @@ class CRM_Case_XMLProcessor { */ public function &allRelationshipTypes($fromXML = FALSE) { if (!isset(Civi::$statics[__CLASS__]['reltypes'][$fromXML])) { - $relationshipInfo = CRM_Core_PseudoConstant::relationshipType('label', TRUE); + // Note this now includes disabled types too. The only place this + // function is being used is for comparison against a list, not + // displaying a dropdown list or something like that, so we need + // to include disabled. + $relationshipInfo = civicrm_api3('RelationshipType', 'get', [ + 'options' => ['limit' => 0], + ]); Civi::$statics[__CLASS__]['reltypes'][$fromXML] = []; - foreach ($relationshipInfo as $id => $info) { + foreach ($relationshipInfo['values'] as $id => $info) { Civi::$statics[__CLASS__]['reltypes'][$fromXML][$id . '_b_a'] = ($fromXML) ? $info['label_a_b'] : $info['label_b_a']; + /** + * Exclude if bidirectional + * (Why? I'm thinking this was for consistency with the dropdown + * in ang/crmCaseType.js where it would be needed to avoid seeing + * duplicates in the dropdown. Not sure if needed here but keeping + * as-is.) + */ if ($info['label_b_a'] !== $info['label_a_b']) { Civi::$statics[__CLASS__]['reltypes'][$fromXML][$id . '_a_b'] = ($fromXML) ? $info['label_b_a'] : $info['label_a_b']; } diff --git a/CRM/Case/XMLProcessor/Process.php b/CRM/Case/XMLProcessor/Process.php index 1b96b2e152..841d630a13 100644 --- a/CRM/Case/XMLProcessor/Process.php +++ b/CRM/Case/XMLProcessor/Process.php @@ -105,7 +105,7 @@ class CRM_Case_XMLProcessor_Process extends CRM_Case_XMLProcessor { foreach ($xml->CaseRoles as $caseRoleXML) { foreach ($caseRoleXML->RelationshipType as $relationshipTypeXML) { if ((int ) $relationshipTypeXML->creator == 1) { - if (!$this->createRelationships($this->locateNameOrLabel($relationshipTypeXML), + if (!$this->createRelationships($relationshipTypeXML, $params ) ) { @@ -184,16 +184,12 @@ class CRM_Case_XMLProcessor_Process extends CRM_Case_XMLProcessor { // Look up relationship types according to the XML convention (described // from perspective of non-client) but return the labels according to the UI // convention (described from perspective of client) - $relationshipTypes = &$this->allRelationshipTypes(TRUE); $relationshipTypesToReturn = &$this->allRelationshipTypes(FALSE); $result = []; foreach ($caseRolesXML as $caseRoleXML) { foreach ($caseRoleXML->RelationshipType as $relationshipTypeXML) { - $relationshipTypeName = (string ) $relationshipTypeXML->name; - $relationshipTypeID = array_search($relationshipTypeName, - $relationshipTypes - ); + list($relationshipTypeID,) = $this->locateNameOrLabel($relationshipTypeXML); if ($relationshipTypeID === FALSE) { continue; } @@ -210,19 +206,16 @@ class CRM_Case_XMLProcessor_Process extends CRM_Case_XMLProcessor { } /** - * @param string $relationshipTypeName + * @param SimpleXMLElement $relationshipTypeXML * @param array $params * * @return bool * @throws Exception */ - public function createRelationships($relationshipTypeName, &$params) { - // The relationshipTypeName is coming from XML, so the argument should be - // `TRUE` - $relationshipTypes = &$this->allRelationshipTypes(TRUE); - // get the relationship - $relationshipType = array_search($relationshipTypeName, $relationshipTypes); + public function createRelationships($relationshipTypeXML, &$params) { + // get the relationship + list($relationshipType, $relationshipTypeName) = $this->locateNameOrLabel($relationshipTypeXML); if ($relationshipType === FALSE) { $docLink = CRM_Utils_System::docURL2("user/case-management/set-up"); CRM_Core_Error::fatal(ts('Relationship type %1, found in case configuration file, is not present in the database %2', @@ -367,7 +360,8 @@ class CRM_Case_XMLProcessor_Process extends CRM_Case_XMLProcessor { if (!empty($caseTypeXML->CaseRoles) && $caseTypeXML->CaseRoles->RelationshipType) { foreach ($caseTypeXML->CaseRoles->RelationshipType as $relTypeXML) { - $result[] = (string) $relTypeXML->name; + list(, $relationshipTypeMachineName) = $this->locateNameOrLabel($relTypeXML); + $result[] = $relationshipTypeMachineName; } } @@ -848,32 +842,67 @@ AND a.is_deleted = 0 /** * At some point name and label got mixed up for case roles. - * Check for higher priority tag first which represents name, then fall back to the tag which somehow became label. - * We do this to avoid requiring people to update their xml files which can be stored in external files. - * - * Note this is different than doing something like comparing the tag against name in the database and then falling back to comparing label in the database, which is subject to an edge case where you would get the wrong one (where the label of one relationship type is the same as the name of another). Here there are two tags with explicit single meanings. + * Check against known machine name values, and then if no match check + * against labels. + * This is subject to some edge cases, but we catch those with a system + * status check. + * We do this to avoid requiring people to update their xml files which can + * be stored in external files we can't/don't want to edit. * * @param SimpleXMLElement $xml * - * @return string + * @return array[bool|string,string] */ public function locateNameOrLabel($xml) { - /* While it's unlikely, it's possible somebody is using '0' as their machineName, so we should let them. - * Specifically if machineName is: - * missing - use name - * null - use name - * blank - use name - * the string '0' - use machineName - * the number 0 - use machineName (but can't really have number 0 in simplexml unless cast to number) - * the word 'null' - use machineName and best not to think about it - */ - if (isset($xml->machineName)) { - $machineName = (string) $xml->machineName; - if ($machineName !== '') { - return $machineName; - } + $lookupString = (string) $xml->name; + + // Don't use pseudoconstant because we need everything both name and + // label and disabled types. + $relationshipTypes = civicrm_api3('RelationshipType', 'get', [ + 'options' => ['limit' => 0], + ])['values']; + + // First look and see if it matches a machine name in the system. + // There are some edge cases here where we've actually been passed in a + // display label and it happens to match the machine name for a different + // db entry, but we have a system status check. + // But, we do want to check against the a_b version first, because of the + // way direction matters and that for bidirectional only one is present in + // the list where this eventually gets used, so return that first. + $relationshipTypeMachineNames = array_column($relationshipTypes, 'id', 'name_a_b'); + if (isset($relationshipTypeMachineNames[$lookupString])) { + return ["{$relationshipTypeMachineNames[$lookupString]}_b_a", $lookupString]; + } + $relationshipTypeMachineNames = array_column($relationshipTypes, 'id', 'name_b_a'); + if (isset($relationshipTypeMachineNames[$lookupString])) { + return ["{$relationshipTypeMachineNames[$lookupString]}_a_b", $lookupString]; + } + + // Now at this point assume we've been passed a display label, so find + // what it matches and return the associated machine name. This is a bit + // trickier because suppose somebody has changed the display labels so + // that they are now the same, but the machine names are different. We + // don't know which to return and so while it's the right relationship type + // it might be the backwards direction. We have to pick one to try first. + + $relationshipTypeDisplayLabels = array_column($relationshipTypes, 'id', 'label_a_b'); + if (isset($relationshipTypeDisplayLabels[$lookupString])) { + return [ + "{$relationshipTypeDisplayLabels[$lookupString]}_b_a", + $relationshipTypes[$relationshipTypeDisplayLabels[$lookupString]]['name_a_b'], + ]; } - return (string) $xml->name; + $relationshipTypeDisplayLabels = array_column($relationshipTypes, 'id', 'label_b_a'); + if (isset($relationshipTypeDisplayLabels[$lookupString])) { + return [ + "{$relationshipTypeDisplayLabels[$lookupString]}_a_b", + $relationshipTypes[$relationshipTypeDisplayLabels[$lookupString]]['name_b_a'], + ]; + } + + // Just go with what we were passed in, even though it doesn't seem + // to match *anything*. This was what it did before. + return [FALSE, $lookupString]; } } diff --git a/CRM/Utils/Check/Component/Case.php b/CRM/Utils/Check/Component/Case.php index 3f763e717e..b9108b6654 100644 --- a/CRM/Utils/Check/Component/Case.php +++ b/CRM/Utils/Check/Component/Case.php @@ -162,4 +162,239 @@ class CRM_Utils_Check_Component_Case extends CRM_Utils_Check_Component { return $messages; } + /** + * Check that the relationship types aren't going to cause problems. + * + * @return array + * An empty array, or a list of warnings + */ + public function checkRelationshipTypeProblems() { + $messages = []; + + /** + * There's no use-case to have two different relationship types + * with the same machine name, and it will cause problems because the + * system might match up the wrong type when comparing to xml. + * A single bi-directional one CAN and probably does have the same + * name_a_b and name_b_a and that's ok. + */ + + $dao = CRM_Core_DAO::executeQuery("SELECT rt1.*, rt2.id AS id2, rt2.name_a_b AS nameab2, rt2.name_b_a AS nameba2 FROM civicrm_relationship_type rt1 INNER JOIN civicrm_relationship_type rt2 ON (rt1.name_a_b = rt2.name_a_b OR rt1.name_a_b = rt2.name_b_a) WHERE rt1.id <> rt2.id"); + while ($dao->fetch()) { + $messages[] = new CRM_Utils_Check_Message( + __FUNCTION__ . $dao->id . "dupe1", + ts("Relationship type %1 has the same internal machine name as another type. + + + + +
IDname_a_bname_b_a
%2%3%4
%5%6%7
", [ + 1 => htmlspecialchars($dao->label_a_b), + 2 => $dao->id, + 3 => htmlspecialchars($dao->name_a_b), + 4 => htmlspecialchars($dao->name_b_a), + 5 => $dao->id2, + 6 => htmlspecialchars($dao->nameab2), + 7 => htmlspecialchars($dao->nameba2), + ]) . + '
' . + ts('Read more about this warning') . + '', + ts('Relationship Type Internal Name Duplicates'), + \Psr\Log\LogLevel::ERROR, + 'fa-exchange' + ); + } + + // Ditto for labels + $dao = CRM_Core_DAO::executeQuery("SELECT rt1.*, rt2.id AS id2, rt2.label_a_b AS labelab2, rt2.label_b_a AS labelba2 FROM civicrm_relationship_type rt1 INNER JOIN civicrm_relationship_type rt2 ON (rt1.label_a_b = rt2.label_a_b OR rt1.label_a_b = rt2.label_b_a) WHERE rt1.id <> rt2.id"); + while ($dao->fetch()) { + $messages[] = new CRM_Utils_Check_Message( + __FUNCTION__ . $dao->id . "dupe2", + ts("Relationship type %1 has the same display label as another type. + + + + +
IDlabel_a_blabel_b_a
%2%3%4
%5%6%7
", [ + 1 => htmlspecialchars($dao->label_a_b), + 2 => $dao->id, + 3 => htmlspecialchars($dao->label_a_b), + 4 => htmlspecialchars($dao->label_b_a), + 5 => $dao->id2, + 6 => htmlspecialchars($dao->labelab2), + 7 => htmlspecialchars($dao->labelba2), + ]) . + '
' . + ts('Read more about this warning') . + '', + ts('Relationship Type Display Label Duplicates'), + \Psr\Log\LogLevel::ERROR, + 'fa-exchange' + ); + } + + /** + * If the name of one type matches the label of another type, there may + * also be problems. This can happen if for example you initially set + * it up and then keep changing your mind adding and deleting and renaming + * a couple times in a certain order. + */ + $dao = CRM_Core_DAO::executeQuery("SELECT rt1.*, rt2.id AS id2, rt2.name_a_b AS nameab2, rt2.name_b_a AS nameba2, rt2.label_a_b AS labelab2, rt2.label_b_a AS labelba2 FROM civicrm_relationship_type rt1 INNER JOIN civicrm_relationship_type rt2 ON (rt1.name_a_b = rt2.label_a_b OR rt1.name_b_a = rt2.label_a_b OR rt1.name_a_b = rt2.label_b_a OR rt1.name_b_a = rt2.label_b_a) WHERE rt1.id <> rt2.id"); + // No point displaying the same matching id twice, which can happen with + // the query. + $ids = []; + while ($dao->fetch()) { + if (isset($ids[$dao->id2])) { + continue; + } + $ids[$dao->id] = $dao->id; + $messages[] = new CRM_Utils_Check_Message( + __FUNCTION__ . $dao->id . "dupe3", + ts("Relationship type %1 has an internal machine name that is the same as the display label as another type. + + + + +
IDname_a_bname_b_alabel_a_blabel_b_a
%2%3%4%5%6
%7%8%9%10%11
", [ + 1 => htmlspecialchars($dao->label_a_b), + 2 => $dao->id, + 3 => htmlspecialchars($dao->name_a_b), + 4 => htmlspecialchars($dao->name_b_a), + 5 => htmlspecialchars($dao->label_a_b), + 6 => htmlspecialchars($dao->label_b_a), + 7 => $dao->id2, + 8 => htmlspecialchars($dao->nameab2), + 9 => htmlspecialchars($dao->nameab2), + 10 => htmlspecialchars($dao->labelab2), + 11 => htmlspecialchars($dao->labelba2), + ]) . + '
' . + ts('Read more about this warning') . + '', + ts('Relationship Type Cross-Duplication'), + \Psr\Log\LogLevel::WARNING, + 'fa-exchange' + ); + } + + /** + * Check that ones that appear to be unidirectional don't have the same + * machine name for both a_b and b_a. This can happen for example if you + * forget to fill in the b_a label when creating, then go back and edit. + */ + $dao = CRM_Core_DAO::executeQuery("SELECT rt1.* FROM civicrm_relationship_type rt1 WHERE rt1.name_a_b = rt1.name_b_a AND rt1.label_a_b <> rt1.label_b_a"); + while ($dao->fetch()) { + $messages[] = new CRM_Utils_Check_Message( + __FUNCTION__ . $dao->id . "ambiguousname", + ts("Relationship type %1 appears to be unidirectional, but has the same internal machine name for both sides. + + + +
IDname_a_bname_b_alabel_a_blabel_b_a
%2%3%4%5%6
", [ + 1 => htmlspecialchars($dao->label_a_b), + 2 => $dao->id, + 3 => htmlspecialchars($dao->name_a_b), + 4 => htmlspecialchars($dao->name_b_a), + 5 => htmlspecialchars($dao->label_a_b), + 6 => htmlspecialchars($dao->label_b_a), + ]) . + '
' . + ts('Read more about this warning') . + '', + ts('Relationship Type Ambiguity'), + \Psr\Log\LogLevel::WARNING, + 'fa-exchange' + ); + } + + /** + * Check that ones that appear to be unidirectional don't have the same + * label for both a_b and b_a. This can happen for example if you + * created it as unidirectional, then edited it later trying to make it + * bidirectional. + */ + $dao = CRM_Core_DAO::executeQuery("SELECT rt1.* FROM civicrm_relationship_type rt1 WHERE rt1.label_a_b = rt1.label_b_a AND rt1.name_a_b <> rt1.name_b_a"); + while ($dao->fetch()) { + $messages[] = new CRM_Utils_Check_Message( + __FUNCTION__ . $dao->id . "ambiguouslabel", + ts("Relationship type %1 appears to be unidirectional internally, but has the same display label for both sides. Possibly you created it initially as unidirectional and then made it bidirectional later. + + + +
IDname_a_bname_b_alabel_a_blabel_b_a
%2%3%4%5%6
", [ + 1 => htmlspecialchars($dao->label_a_b), + 2 => $dao->id, + 3 => htmlspecialchars($dao->name_a_b), + 4 => htmlspecialchars($dao->name_b_a), + 5 => htmlspecialchars($dao->label_a_b), + 6 => htmlspecialchars($dao->label_b_a), + ]) . + '
' . + ts('Read more about this warning') . + '', + ts('Relationship Type Ambiguity'), + \Psr\Log\LogLevel::WARNING, + 'fa-exchange' + ); + } + + /** + * Check for missing roles listed in the xml but not defined as + * relationship types. + */ + + // Don't use database since might be in xml files. + $caseTypes = civicrm_api3('CaseType', 'get', [ + 'options' => ['limit' => 0], + ])['values']; + // Don't use pseudoconstant since want all and also name and label. + $relationshipTypes = civicrm_api3('RelationshipType', 'get', [ + 'options' => ['limit' => 0], + ])['values']; + $allConfigured = array_column($relationshipTypes, 'id', 'name_a_b') + + array_column($relationshipTypes, 'id', 'name_b_a') + + array_column($relationshipTypes, 'id', 'label_a_b') + + array_column($relationshipTypes, 'id', 'label_b_a'); + $missing = []; + foreach ($caseTypes as $caseType) { + foreach ($caseType['definition']['caseRoles'] as $role) { + if (!isset($allConfigured[$role['name']])) { + $missing[$role['name']] = $role['name']; + } + } + } + if (!empty($missing)) { + $tableRows = []; + foreach ($relationshipTypes as $relationshipType) { + $tableRows[] = ts('%1%2%3%4%5', [ + 1 => $relationshipType['id'], + 2 => htmlspecialchars($relationshipType['name_a_b']), + 3 => htmlspecialchars($relationshipType['name_b_a']), + 4 => htmlspecialchars($relationshipType['label_a_b']), + 5 => htmlspecialchars($relationshipType['label_b_a']), + ]); + } + $messages[] = new CRM_Utils_Check_Message( + __FUNCTION__ . "missingroles", + ts("

The following roles listed in your case type definitions do not match any relationship type defined in the system: %1.

" + . "

This might be because of a mismatch if you are using external xml files to manage case types. If using xml files, then use either the name_a_b or name_b_a value from the following table. (Out of the box you would use name_b_a, which lists them on the case from the client perspective.) If you are not using xml files, you can edit your case types at Administer - CiviCase - Case Types.

" + . " + " + . implode("\n", $tableRows) + . "
IDname_a_bname_b_alabel_a_blabel_b_a
", [ + 1 => htmlspecialchars(implode(', ', $missing)), + ]) . + '
' . + ts('Read more about this warning') . + '', + ts('Missing Roles'), + \Psr\Log\LogLevel::ERROR, + 'fa-exclamation' + ); + } + + return $messages; + } + } diff --git a/ang/crmCaseType.js b/ang/crmCaseType.js index 6fdc0555f1..0b97432237 100644 --- a/ang/crmCaseType.js +++ b/ang/crmCaseType.js @@ -79,7 +79,6 @@ }]; reqs.relTypes = ['RelationshipType', 'get', { sequential: 1, - is_active: 1, options: { sort: 'label_a_b', limit: 0 @@ -264,8 +263,10 @@ $scope.activityTypes = _.indexBy(apiCalls.actTypes.values, 'name'); $scope.activityTypeOptions = _.map(apiCalls.actTypes.values, formatActivityTypeOption); $scope.defaultAssigneeTypes = apiCalls.defaultAssigneeTypes.values; - $scope.relationshipTypeOptions = getRelationshipTypeOptions(false); - $scope.defaultRelationshipTypeOptions = getRelationshipTypeOptions(true); + // for dropdown lists, only include enabled choices + $scope.relationshipTypeOptions = getRelationshipTypeOptions(true); + // for comparisons, include disabled + $scope.relationshipTypeOptionsAll = getRelationshipTypeOptions(false); // stores the default assignee values indexed by their option name: $scope.defaultAssigneeTypeValues = _.chain($scope.defaultAssigneeTypes) .indexBy('name').mapValues('value').value(); @@ -276,44 +277,66 @@ // two options representing the relationship type directions (Ex: Employee // of, Employer of). // - // The default relationship field needs values that are IDs with direction, - // while the role field needs values that are names (with implicit - // direction). + // The relationship dropdown needs to be given IDs with direction, + // while the role name in the xml needs values that are names (with + // implicit direction). // // At any rate, the labels should follow the convention in the UI of // describing case roles from the perspective of the client, while the - // values must follow the convention in the XML of describing case roles + // names must follow the convention in the XML of describing case roles // from the perspective of the non-client. - function getRelationshipTypeOptions($isDefault) { - return _.transform(apiCalls.relTypes.values, function(result, relType) { + // + // @param onlyActive bool + // If true, only include enabled relationship types. + // @return array[object] + // object: { + // xmlName: The name corresponding to what's stored in xml/caseRoles. + // text: The text in dropdowns, i.e. + // It's called text because that is what select2 is expecting. + // id: The id value in dropdowns, i.e. + // Is the concatenation of id+direction, e.g. 2_a_b. + // It's called id because that is what select2 is expecting. + // } + function getRelationshipTypeOptions(onlyActive) { + var relationshipTypesToUse; + if (onlyActive) { + relationshipTypesToUse = _.filter(apiCalls.relTypes.values, {is_active: "1"}); + } else { + relationshipTypesToUse = apiCalls.relTypes.values; + } + return _.transform(relationshipTypesToUse, function(result, relType) { var isBidirectionalRelationship = relType.label_a_b === relType.label_b_a; - if ($isDefault) { - result.push({ - label: relType.label_b_a, - value: relType.id + '_a_b' - }); - if (!isBidirectionalRelationship) { - result.push({ - label: relType.label_a_b, - value: relType.id + '_b_a' - }); - } - } - // TODO The ids below really should use names not labels see - // https://lab.civicrm.org/dev/core/issues/774 - else { + // The order here of a's and b's here is important regarding + // unidirectional and bidirectional. Because the xml spec DOES support + // direction for activity auto-assignees, if this is changed you might + // end up with activity assignees in existing xml that then come + // through as blank in the dropdown on the timelines tab if it's a + // bidirectional relationship. E.g. if spouse is stored in an existing + // xml definition as 2_a_b, but we only push 2_b_a, then it won't match + // in the dropdown. + // + // This has some implications for when we add a new type on the fly + // later, but it works out ok as long as we also do it in the same + // direction there. See notes in addRoleOnTheFly(). + result.push({ + // This is what we want to store in the caseRoles.name field, + // which corresponds to the xml file, when we send it back to + // the server. + xmlName: relType.name_a_b, + // This has to be called text because select2 is expecting it. + // And yes it's the opposite direction from name. + text: relType.label_b_a, + // This has to be called id because select2 is expecting it. + id: relType.id + '_a_b' + }); + + if (!isBidirectionalRelationship) { result.push({ - text: relType.label_b_a, - id: relType.label_a_b + xmlName: relType.name_b_a, + text: relType.label_a_b, + id: relType.id + '_b_a' }); - - if (!isBidirectionalRelationship) { - result.push({ - text: relType.label_a_b, - id: relType.label_b_a - }); - } } }, []); } @@ -352,12 +375,19 @@ }); }); - // go lookup and add client-perspective labels for $scope.caseType.definition.caseRoles + // Go lookup and add client-perspective labels for + // $scope.caseType.definition.caseRoles, since the xml doesn't have them + // and we need to display them in the roles table. _.each($scope.caseType.definition.caseRoles, function (set) { - _.each($scope.relationshipTypeOptions, function (relationshipTypeOption) { - if (relationshipTypeOption.text == set.name) { - // relationshipTypeOption.id here corresponds to one of the civicrm_relationship_type.label database fields, not civicrm_relationship_type.id - set.displayLabel = relationshipTypeOption.id; + _.each($scope.relationshipTypeOptionsAll, function (relationshipTypeOption) { + if (relationshipTypeOption.xmlName == set.name) { + // relationshipTypeOption.text here corresponds to one of the + // apiCalls.relTypes label fields (i.e. civicrm_relationship_type + // label database fields). It has to be called text because + // it's used in select2 which expects it to be called text. + set.displayLabel = relationshipTypeOption.text; + // break out of inner `each` loop + return false; } }); }); @@ -461,36 +491,95 @@ activity.default_assignee_contact = null; }; - // TODO roleName passed to addRole is a misnomer, its passed as the - // label HOWEVER it should be saved to xml as the name see - // https://lab.civicrm.org/dev/core/issues/774 - - /// Add a new role - $scope.addRole = function(roles, roleName) { + // Add a new role. + // Called from the select2 dropdown when a selection is made. + // + // @param roles array + // The roles currently in the table. + // @param roleIdOrLabel string + // The trick here is that since you can add roles on the fly, the + // roleIdOrLabel parameter can be two different types of things. It can be + // the id, like '2_a_b' if it's an existing choice that was selected, or + // it can be a LABEL if they typed something that isn't in the list. If + // the latter the select2 has no choice but to give us a label because + // there is no id yet. + $scope.addRole = function(roles, roleIdOrLabel) { + var matchingRole; + // First check does what we've been given match up to any relationship + // type, based on id, which is the id from the select2 (i.e. html + //