diff --git a/CRM/Upgrade/Incremental/php/FiveTwenty.php b/CRM/Upgrade/Incremental/php/FiveTwenty.php index 155713a2ab43..287c40995bbb 100644 --- a/CRM/Upgrade/Incremental/php/FiveTwenty.php +++ b/CRM/Upgrade/Incremental/php/FiveTwenty.php @@ -46,16 +46,21 @@ class CRM_Upgrade_Incremental_php_FiveTwenty extends CRM_Upgrade_Incremental_Bas * @param null $currentVer */ public function setPreUpgradeMessage(&$preUpgradeMessage, $rev, $currentVer = NULL) { - // Example: Generate a pre-upgrade message. - // if ($rev == '5.12.34') { - // $preUpgradeMessage .= '

' . ts('A new permission, "%1", has been added. This permission is now used to control access to the Manage Tags screen.', array(1 => ts('manage tags'))) . '

'; - // } if ($rev == '5.20.alpha1') { if (CRM_Core_DAO::checkTableExists('civicrm_persistent') && CRM_Core_DAO::checkTableHasData('civicrm_persistent')) { $preUpgradeMessage .= '
' . ts("WARNING: The table \"civicrm_persistent\" is flagged for removal because all official records show it being unused. However, the upgrader has detected data in this copy of \"civicrm_persistent\". Please report anything you can about the usage of this table. In the mean-time, the data will be preserved.", [ 1 => 'https://civicrm.org/bug-reporting', ]); } + + $config = CRM_Core_Config::singleton(); + if (in_array('CiviCase', $config->enableComponents)) { + // Do dry-run to get warning messages. + $messages = self::_changeCaseTypeLabelToName(TRUE); + foreach ($messages as $message) { + $preUpgradeMessage .= "

{$message}

\n"; + } + } } } @@ -99,6 +104,7 @@ public function upgrade_5_20_alpha1($rev) { $config = CRM_Core_Config::singleton(); if (in_array('CiviCase', $config->enableComponents)) { $this->addTask('Change direction of autoassignees in case type xml', 'changeCaseTypeAutoassignee'); + $this->addTask('Change labels back to names in case type xml', 'changeCaseTypeLabelToName'); } $this->addTask(ts('Upgrade DB to %1: SQL', [1 => $rev]), 'runSql', $rev); $this->addTask('Add "Template" contribution status', 'templateStatus'); @@ -248,4 +254,194 @@ private static function isBidirectionalRelationship($relationshipTypeId) { return FALSE; } + /** + * Change labels in case type xml definition back to names. (dev/core#1046) + * ONLY for ones using database storage - don't want to "fork" case types + * that aren't currently forked. + * + * @return bool + */ + public static function changeCaseTypeLabelToName() { + self::_changeCaseTypeLabelToName(FALSE); + return TRUE; + } + + /** + * Change labels in case type xml definition back to names. (dev/core#1046) + * ONLY for ones using database storage - don't want to "fork" case types + * that aren't currently forked. + * + * @param $isDryRun bool + * If TRUE then don't actually change anything just report warnings. + * + * @return array List of warning messages. + */ + public static function _changeCaseTypeLabelToName($isDryRun = FALSE) { + $messages = []; + self::$relationshipTypes = civicrm_api3('RelationshipType', 'get', [ + 'options' => ['limit' => 0], + ])['values']; + + // Get all case types definitions that are using db storage + $dao = CRM_Core_DAO::executeQuery("SELECT id FROM civicrm_case_type WHERE definition IS NOT NULL AND definition <> ''"); + while ($dao->fetch()) { + // array_merge so that existing numeric keys don't get overwritten + $messages = array_merge($messages, self::_processCaseTypeLabelName($isDryRun, $dao->id)); + } + return $messages; + } + + /** + * Process a single case type for _changeCaseTypeLabelToName() + * + * @param $isDryRun bool + * If TRUE then don't actually change anything just report warnings. + * @param $caseTypeId int + */ + private static function _processCaseTypeLabelName($isDryRun, $caseTypeId) { + $messages = []; + $isDirty = FALSE; + + // Get the case type definition + $caseType = civicrm_api3( + 'CaseType', + 'get', + ['id' => $caseTypeId] + )['values'][$caseTypeId]; + + foreach ($caseType['definition']['caseRoles'] as $roleSequenceId => $role) { + // First double-check that there is a unique match on label so we + // don't get it wrong. + // There's maybe a fancy way to do this with array_XXX functions but + // need to take into account edge cases where bidirectional but name + // is different, or where somehow two labels are the same across types, + // so do old-fashioned loop. + + $cantConvertMessage = NULL; + $foundName = NULL; + foreach (self::$relationshipTypes as $relationshipType) { + // does it match one of our existing labels + if ($relationshipType['label_a_b'] === $role['name'] || $relationshipType['label_b_a'] === $role['name']) { + // So either it's ambiguous, in which case exit loop with a message, + // or we have the name, so exit loop with that. + $cantConvertMessage = self::checkAmbiguous($relationshipType, $caseType['name'], $role['name']); + if (empty($cantConvertMessage)) { + // not ambiguous, so note the corresponding name for the direction + $foundName = ($relationshipType['label_a_b'] === $role['name']) ? $relationshipType['name_a_b'] : $relationshipType['name_b_a']; + } + break; + } + } + + if (empty($foundName) && empty($cantConvertMessage)) { + // It's possible we went through all relationship types and didn't + // find any match, so don't change anything. + $cantConvertMessage = ts("Case Type '%1', role '%2' doesn't seem to be a valid role. See the administration console status messages for more info.", [ + 1 => htmlspecialchars($caseType['name']), + 2 => htmlspecialchars($role['name']), + ]); + } + // Only two possibilities now are we have a name, or we have a message. + // So the if($foundName) is redundant, but seems clearer somehow. + if ($foundName && empty($cantConvertMessage)) { + // If name and label are the same don't need to update anything. + if ($foundName !== $role['name']) { + $caseType['definition']['caseRoles'][$roleSequenceId]['name'] = $foundName; + $isDirty = TRUE; + } + } + else { + $messages[] = $cantConvertMessage; + } + + // end looping thru all roles in definition + } + + // If this is a dry run during preupgrade checks we can skip this and + // just return any messages. + // If for real, then update the case type and here if there's errors + // we don't really have a choice but to stop the entire upgrade + // completely. There's no way to just send back messages during a queue + // run. But we can log a message to error log so that the user has a + // little more specific info about which case type. + if ($isDirty && !$isDryRun) { + $exception = NULL; + try { + $api_result = civicrm_api3('CaseType', 'create', $caseType); + } + catch (Exception $e) { + $exception = $e; + $errorMessage = ts("Error updating case type '%1': %2", [ + 1 => htmlspecialchars($caseType['name']), + 2 => htmlspecialchars($e->getMessage()), + ]); + CRM_Core_Error::debug_log_message($errorMessage); + } + if (!empty($api_result['is_error'])) { + $errorMessage = ts("Error updating case type '%1': %2", [ + 1 => htmlspecialchars($caseType['name']), + 2 => htmlspecialchars($api_result['error_message']), + ]); + CRM_Core_Error::debug_log_message($errorMessage); + $exception = new Exception($errorMessage); + } + // We need to rethrow the error which unfortunately stops the + // entire upgrade including any further tasks. But otherwise + // the only way to notify the user something went wrong is with a + // crazy workaround. + if ($exception) { + throw $exception; + } + } + + return $messages; + } + + /** + * Helper for _processCaseTypeLabelName to check if a label can't be + * converted unambiguously to name. + * + * If it's bidirectional, we can't convert it if there's an edge case + * where the two names are different. + * + * If it's unidirectional, we can't convert it if there's an edge case + * where there's another type that has the same label. + * + * @param $relationshipType array + * @param $caseTypeName string + * @param $xmlRoleName string + * + * @return string|NULL + */ + private static function checkAmbiguous($relationshipType, $caseTypeName, $xmlRoleName) { + $cantConvertMessage = NULL; + if ($relationshipType['label_a_b'] === $relationshipType['label_b_a']) { + // bidirectional, so check if names are different for some reason + if ($relationshipType['name_a_b'] !== $relationshipType['name_b_a']) { + $cantConvertMessage = ts("Case Type '%1', role '%2' has an ambiguous configuration and can't be automatically updated. See the administration console status messages for more info.", [ + 1 => htmlspecialchars($caseTypeName), + 2 => htmlspecialchars($xmlRoleName), + ]); + } + } + else { + // Check if it matches either label_a_b or label_b_a for another type + foreach (self::$relationshipTypes as $innerLoopId => $innerLoopType) { + if ($innerLoopId == $relationshipType['id']) { + // Only check types that aren't the same one we're on. + // Sidenote: The loop index is integer but the 'id' member is string + continue; + } + if ($innerLoopType['label_a_b'] === $xmlRoleName || $innerLoopType['label_b_a'] === $xmlRoleName) { + $cantConvertMessage = ts("Case Type '%1', role '%2' has an ambiguous configuration where the role matches multiple labels and so can't be automatically updated. See the administration console status messages for more info.", [ + 1 => htmlspecialchars($caseTypeName), + 2 => htmlspecialchars($xmlRoleName), + ]); + break; + } + } + } + return $cantConvertMessage; + } + } diff --git a/tests/phpunit/CRM/Upgrade/Incremental/php/FiveTwentyTest.php b/tests/phpunit/CRM/Upgrade/Incremental/php/FiveTwentyTest.php index 5cbf00f78587..fe01cc3ca5d5 100644 --- a/tests/phpunit/CRM/Upgrade/Incremental/php/FiveTwentyTest.php +++ b/tests/phpunit/CRM/Upgrade/Incremental/php/FiveTwentyTest.php @@ -191,4 +191,752 @@ public function testChangeCaseTypeAutoassignee() { $this->assertEquals($expectedCaseTypeXml, $dao->definition); } + /** + * Test that the upgrade task converts case role 's that + * are labels to their name. + */ + public function testConvertRoleLabelsToNames() { + + // We don't know what the ids are for the relationship types since it + // seems to depend what ran before us, so retrieve them first and go by + // name. + // Also spouse might not exist. + + $result = $this->callAPISuccess('RelationshipType', 'get', ['limit' => 0])['values']; + // Get list of ids keyed on name. + $relationshipTypeNames = array_column($result, 'id', 'name_b_a'); + + // Create spouse if none. + if (!isset($relationshipTypeNames['Spouse of'])) { + $spouseId = $this->relationshipTypeCreate([ + 'name_a_b' => 'Spouse of', + 'name_b_a' => 'Spouse of', + 'label_a_b' => 'Spouse of', + 'label_b_a' => 'Spouse of', + 'contact_type_a' => 'Individual', + 'contact_type_b' => 'Individual', + ]); + $relationshipTypeNames['Spouse of'] = $spouseId; + } + // Maybe unnecessary but why not. Slightly different than an undefined + // index later when it doesn't exist at all. + $this->assertGreaterThan(0, $relationshipTypeNames['Spouse of']); + + // Add one with changed labels + $id = $this->relationshipTypeCreate([ + 'name_a_b' => 'Wallet Inspector is', + 'name_b_a' => 'Wallet Inspector', + 'label_a_b' => 'has as Wallet Inspector', + 'label_b_a' => 'is Wallet Inspector of', + 'contact_type_a' => 'Individual', + 'contact_type_b' => 'Individual', + ]); + $relationshipTypeNames['Wallet Inspector'] = $id; + + // Add one with non-ascii characters. + $id = $this->relationshipTypeCreate([ + 'name_a_b' => 'абвгде is', + 'name_b_a' => 'абвгде', + 'label_a_b' => 'абвгде is', + 'label_b_a' => 'абвгде', + 'contact_type_a' => 'Individual', + 'contact_type_b' => 'Individual', + ]); + $relationshipTypeNames['Ascii'] = $id; + + // Add one with non-ascii characters changed labels. + $id = $this->relationshipTypeCreate([ + 'name_a_b' => 'αβγδ is', + 'name_b_a' => 'αβγδ', + 'label_a_b' => 'αβγδ is changed', + 'label_b_a' => 'αβγδ changed', + 'contact_type_a' => 'Individual', + 'contact_type_b' => 'Individual', + ]); + $relationshipTypeNames['Ascii changed'] = $id; + + // Create some case types + $caseTypes = $this->createCaseTypes($relationshipTypeNames, 1); + + // run the task + $upgrader = new CRM_Upgrade_Incremental_php_FiveTwenty(); + // first, preupgrade messages should be blank here + $preupgradeMessages = $upgrader->_changeCaseTypeLabelToName(TRUE); + $this->assertEmpty($preupgradeMessages); + + // Now the actual run + $upgrader->changeCaseTypeLabelToName(); + + // Get the updated case types and check. + $sqlParams = [ + 1 => [implode(',', array_keys($caseTypes)), 'String'], + ]; + $dao = CRM_Core_DAO::executeQuery("SELECT id, name, definition FROM civicrm_case_type WHERE id IN (%1)", $sqlParams); + while ($dao->fetch()) { + $this->assertEquals($caseTypes[$dao->id]['expected'], $dao->definition, "Case type {$dao->name}"); + // clean up + CRM_Core_DAO::executeQuery("DELETE FROM civicrm_case_type WHERE id = {$dao->id}"); + } + + // + // Second pass, where we have some edge cases. + // + + // Add a relationship type that has the same labels as another. + $id = $this->relationshipTypeCreate([ + 'name_a_b' => 'mixedupab', + 'name_b_a' => 'mixedupba', + 'label_a_b' => 'Benefits Specialist', + 'label_b_a' => 'Benefits Specialist is', + 'contact_type_a' => 'Individual', + 'contact_type_b' => 'Individual', + ]); + $relationshipTypeNames['mixedup'] = $id; + + // Add a relationship type that appears to be bidirectional but different + // names. + $id = $this->relationshipTypeCreate([ + 'name_a_b' => 'diffnameab', + 'name_b_a' => 'diffnameba', + 'label_a_b' => 'Archenemy of', + 'label_b_a' => 'Archenemy of', + 'contact_type_a' => 'Individual', + 'contact_type_b' => 'Individual', + ]); + $relationshipTypeNames['diffname'] = $id; + + // Second pass for case type creation. + $caseTypes = $this->createCaseTypes($relationshipTypeNames, 2); + + // run the task + $upgrader = new CRM_Upgrade_Incremental_php_FiveTwenty(); + // first, check preupgrade messages + $preupgradeMessages = $upgrader->_changeCaseTypeLabelToName(TRUE); + $this->assertEquals($this->getExpectedUpgradeMessages(), $preupgradeMessages); + + // Now the actual run + $upgrader->changeCaseTypeLabelToName(); + + // Get the updated case types and check. + $sqlParams = [ + 1 => [implode(',', array_keys($caseTypes)), 'String'], + ]; + $dao = CRM_Core_DAO::executeQuery("SELECT id, name, definition FROM civicrm_case_type WHERE id IN (%1)", $sqlParams); + while ($dao->fetch()) { + $this->assertEquals($caseTypes[$dao->id]['expected'], $dao->definition, "Case type {$dao->name}"); + // clean up + CRM_Core_DAO::executeQuery("DELETE FROM civicrm_case_type WHERE id = {$dao->id}"); + } + } + + /** + * Set up some original and expected xml pairs. + * + * @param $relationshipTypeNames array + * @param $stage int + * We run it in a couple passes because we want to test with and without + * warning messages. + * @return array + */ + private function createCaseTypes($relationshipTypeNames, $stage) { + $xmls = []; + + switch ($stage) { + case 1: + $newCaseTypeXml = << + + +simple + + +Open Case +1 + + +Email + + +Follow up + + +Meeting + + +Phone Call + + + + +standard_timeline + +true + + +Open Case +Completed + +1 + + + + +timeline_1 + +true + + +Follow up + +Scheduled +Open Case +7 +newest +2 +{$relationshipTypeNames['Senior Services Coordinator']}_b_a + + + + + + + +Senior Services Coordinator +1 +1 + + +Spouse of + + +Benefits Specialist is + + +is Wallet Inspector of + + +has as Wallet Inspector + + +абвгде + + +αβγδ changed + + +0 + +ENDXMLSIMPLE; + + $expectedCaseTypeXml = << + + +simple + + +Open Case +1 + + +Email + + +Follow up + + +Meeting + + +Phone Call + + + + +standard_timeline + +true + + +Open Case +Completed + +1 + + + + +timeline_1 + +true + + +Follow up + +Scheduled +Open Case +7 +newest +2 +{$relationshipTypeNames['Senior Services Coordinator']}_b_a + + + + + + + +Senior Services Coordinator +1 +1 + + +Spouse of + + +Benefits Specialist is + + +Wallet Inspector + + +Wallet Inspector is + + +абвгде + + +αβγδ + + +0 + +ENDXMLSIMPLEEXPECTED; + + $caseTypeId = $this->addCaseType('simple', $newCaseTypeXml); + $xmls[$caseTypeId] = [ + 'id' => $caseTypeId, + 'expected' => $expectedCaseTypeXml, + ]; + break; + + case 2: + // Note for these ones the roles that have warnings should remain + // unchanged if they choose to continue with the upgrade. + + $newCaseTypeXml = << + + +mixedup + + +Open Case +1 + + +Email + + +Follow up + + +Meeting + + +Phone Call + + + + +standard_timeline + +true + + +Open Case +Completed + +1 + + + + +timeline_1 + +true + + +Follow up + +Scheduled +Open Case +7 +newest +2 +{$relationshipTypeNames['Senior Services Coordinator']}_b_a + + + + + + + +Senior Services Coordinator +1 +1 + + +Spouse of + + +Benefits Specialist is + + +is Wallet Inspector of + + +has as Wallet Inspector + + +абвгде + + +αβγδ changed + + +Benefits Specialist + + +Mythical Unicorn + + +0 + +ENDXMLMIXEDUP; + + $expectedCaseTypeXml = << + + +mixedup + + +Open Case +1 + + +Email + + +Follow up + + +Meeting + + +Phone Call + + + + +standard_timeline + +true + + +Open Case +Completed + +1 + + + + +timeline_1 + +true + + +Follow up + +Scheduled +Open Case +7 +newest +2 +{$relationshipTypeNames['Senior Services Coordinator']}_b_a + + + + + + + +Senior Services Coordinator +1 +1 + + +Spouse of + + +Benefits Specialist is + + +Wallet Inspector + + +Wallet Inspector is + + +абвгде + + +αβγδ + + +Benefits Specialist + + +Mythical Unicorn + + +0 + +ENDXMLMIXEDUPEXPECTED; + + $caseTypeId = $this->addCaseType('mixedup', $newCaseTypeXml); + $xmls[$caseTypeId] = [ + 'id' => $caseTypeId, + 'expected' => $expectedCaseTypeXml, + ]; + + $newCaseTypeXml = << + + +diffname + + +Open Case +1 + + +Email + + +Follow up + + +Meeting + + +Phone Call + + + + +standard_timeline + +true + + +Open Case +Completed + +1 + + + + +timeline_1 + +true + + +Follow up + +Scheduled +Open Case +7 +newest +2 +{$relationshipTypeNames['Senior Services Coordinator']}_b_a + + + + + + + +Senior Services Coordinator +1 +1 + + +Spouse of + + +Benefits Specialist is + + +is Wallet Inspector of + + +has as Wallet Inspector + + +абвгде + + +αβγδ changed + + +Archenemy of + + +0 + +ENDXMLDIFFNAME; + + $expectedCaseTypeXml = << + + +diffname + + +Open Case +1 + + +Email + + +Follow up + + +Meeting + + +Phone Call + + + + +standard_timeline + +true + + +Open Case +Completed + +1 + + + + +timeline_1 + +true + + +Follow up + +Scheduled +Open Case +7 +newest +2 +{$relationshipTypeNames['Senior Services Coordinator']}_b_a + + + + + + + +Senior Services Coordinator +1 +1 + + +Spouse of + + +Benefits Specialist is + + +Wallet Inspector + + +Wallet Inspector is + + +абвгде + + +αβγδ + + +Archenemy of + + +0 + +ENDXMLDIFFNAMEEXPECTED; + + $caseTypeId = $this->addCaseType('diffname', $newCaseTypeXml); + $xmls[$caseTypeId] = [ + 'id' => $caseTypeId, + 'expected' => $expectedCaseTypeXml, + ]; + + break; + + default: + break; + } + + return $xmls; + } + + /** + * @return array + */ + private function getExpectedUpgradeMessages() { + return [ + "Case Type 'mixedup', role 'Benefits Specialist is' has an ambiguous configuration where the role matches multiple labels and so can't be automatically updated. See the administration console status messages for more info.", + + "Case Type 'mixedup', role 'Benefits Specialist' has an ambiguous configuration where the role matches multiple labels and so can't be automatically updated. See the administration console status messages for more info.", + + "Case Type 'mixedup', role 'Mythical Unicorn' doesn't seem to be a valid role. See the administration console status messages for more info.", + + "Case Type 'diffname', role 'Benefits Specialist is' has an ambiguous configuration where the role matches multiple labels and so can't be automatically updated. See the administration console status messages for more info.", + + "Case Type 'diffname', role 'Archenemy of' has an ambiguous configuration and can't be automatically updated. See the administration console status messages for more info.", + ]; + } + + /** + * Helper to add a case type to the database. + * + * @param $name string + * @param $xml string + * + * @return int + */ + private function addCaseType($name, $xml) { + $dao = new CRM_Case_DAO_CaseType(); + $dao->name = $name; + $dao->title = $name; + $dao->is_active = 1; + $dao->definition = $xml; + $dao->insert(); + + return $dao->id; + } + }