From a864eb3881d76279ec8b75af40e36047f8103de5 Mon Sep 17 00:00:00 2001 From: demeritcowboy Date: Tue, 20 Jul 2021 09:10:59 -0400 Subject: [PATCH] convert foreign key ids to something readable from the foreign record --- CRM/Logging/ReportDetail.php | 35 ++++++ tests/phpunit/api/v3/ReportTemplateTest.php | 111 +++++++++++++++++++- 2 files changed, 144 insertions(+), 2 deletions(-) diff --git a/CRM/Logging/ReportDetail.php b/CRM/Logging/ReportDetail.php index 99ed827d14..bbf889a1bc 100644 --- a/CRM/Logging/ReportDetail.php +++ b/CRM/Logging/ReportDetail.php @@ -215,12 +215,25 @@ class CRM_Logging_ReportDetail extends CRM_Report_Form { $to = implode(', ', array_filter($tos)); } + $tableDAOClass = CRM_Core_DAO_AllCoreTables::getClassForTable($table); + if (!empty($tableDAOClass)) { + $tableDAOFields = (new $tableDAOClass())->fields(); + // If this field is a foreign key, then we can later use the foreign + // class to translate the id into something more useful for display. + $fkClassName = $tableDAOFields[$field]['FKClassName'] ?? NULL; + } if (isset($values[$field][$from])) { $from = $values[$field][$from]; } + elseif (!empty($from) && !empty($fkClassName)) { + $from = $this->convertForeignKeyValuesToLabels($fkClassName, $field, $from); + } if (isset($values[$field][$to])) { $to = $values[$field][$to]; } + elseif (!empty($to) && !empty($fkClassName)) { + $to = $this->convertForeignKeyValuesToLabels($fkClassName, $field, $to); + } if (isset($titles[$field])) { $field = $titles[$field]; } @@ -449,4 +462,26 @@ class CRM_Logging_ReportDetail extends CRM_Report_Form { } } + /** + * Given a key value that we know is a foreign key to another table, return + * what the DAO thinks is the "label" for the foreign entity. For example + * if it's referencing a contact then return the contact name, or if it's an + * activity then return the activity subject. + * If it's the type of DAO that doesn't have such a thing, just echo back + * what we were given. + * + * @param string $fkClassName + * @param string $field + * @param int $keyval + * @return string + */ + private function convertForeignKeyValuesToLabels(string $fkClassName, string $field, int $keyval): string { + if (property_exists($fkClassName, '_labelField')) { + $labelValue = CRM_Core_DAO::getFieldValue($fkClassName, $keyval, $fkClassName::$_labelField); + // Not sure if this should use ts - there's not a lot of context (`%1 (id: %2)`) - and also the similar field labels above don't use ts. + return "{$labelValue} (id: {$keyval})"; + } + return (string) $keyval; + } + } diff --git a/tests/phpunit/api/v3/ReportTemplateTest.php b/tests/phpunit/api/v3/ReportTemplateTest.php index f16eded23e..ebb9820857 100644 --- a/tests/phpunit/api/v3/ReportTemplateTest.php +++ b/tests/phpunit/api/v3/ReportTemplateTest.php @@ -1348,7 +1348,7 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase { $this->contactIDs[] = $this->individualCreate(['last_name' => 'Łąchowski-Roberts']); $this->contactIDs[] = $this->individualCreate(['last_name' => 'Łąchowski-Roberts']); - $this->callAPISuccess('Activity', 'create', [ + $this->activityID = $this->callAPISuccess('Activity', 'create', [ 'subject' => 'Very secret meeting', 'activity_date_time' => date('Y-m-d 23:59:58'), 'duration' => 120, @@ -1359,7 +1359,7 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase { 'source_contact_id' => $this->contactIDs[2], 'target_contact_id' => [$this->contactIDs[0], $this->contactIDs[1]], 'assignee_contact_id' => $this->contactIDs[1], - ]); + ])['id']; } /** @@ -1622,4 +1622,111 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase { ]); } + /** + * Convoluted test of the convoluted logging detail report. + * + * In principle it's just make an update and get the report and see if it + * matches the update. + * In practice, besides some setup and trigger-wrangling, the report isn't + * useful for activities, so we're checking activity_contact records, and + * because of how an activity update works that's actually a delete+insert. + */ + public function testLoggingDetail() { + \Civi::settings()->set('logging', 1); + $this->createContactsWithActivities(); + $this->doQuestionableStuffInASeparateFunctionSoNobodyNotices(); + + // Do something that creates an update record. + $this->callAPISuccess('Activity', 'create', [ + 'id' => $this->activityID, + 'assignee_contact_id' => $this->contactIDs[0], + 'details' => 'Edited details', + ]); + + // In normal UI flow you would go to the summary report and drill down, + // but here we need to go directly to the connection id, so find out what + // it was. + $queryParams = [1 => [$this->activityID, 'Integer']]; + $log_conn_id = CRM_Core_DAO::singleValueQuery("SELECT log_conn_id FROM log_civicrm_activity WHERE id = %1 AND log_action='UPDATE' LIMIT 1", $queryParams); + + // There should be only one instance of this after enabling so we can + // just specify the template id as the lookup criteria. + $instance_id = $this->callAPISuccess('report_instance', 'getsingle', [ + 'return' => ['id'], + 'report_id' => 'logging/contact/detail', + ])['id']; + + $_GET = $_REQUEST = [ + 'reset' => '1', + 'log_conn_id' => $log_conn_id, + 'q' => "civicrm/report/instance/$instance_id", + ]; + $values = $this->callAPISuccess('report_template', 'getrows', [ + 'report_id' => 'logging/contact/detail', + ])['values']; + + // Note this is a delete+insert which is logically equivalent to update + $expectedValues = [ + // here's the delete + 0 => [ + 'field' => [ + 0 => 'Activity ID (id: 2)', + 1 => 'Contact ID (id: 2)', + 2 => 'Activity Contact Type (id: 2)', + ], + 'from' => [ + 0 => 'Very secret meeting (id: 1)', + 1 => 'Mr. Anthony Łąchowski-Roberts II (id: 4)', + 2 => 'Activity Assignees', + ], + 'to' => [ + 0 => '', + 1 => '', + 2 => '', + ], + ], + // this is the insert + 1 => [ + 'field' => [ + 0 => 'Activity ID (id: 5)', + 1 => 'Contact ID (id: 5)', + 2 => 'Activity Contact Type (id: 5)', + ], + 'from' => [ + 0 => '', + 1 => '', + 2 => '', + ], + 'to' => [ + 0 => 'Very secret meeting (id: 1)', + 1 => 'Mr. Anthony Brzęczysław II (id: 3)', + 2 => 'Activity Assignees', + ], + ], + ]; + $this->assertEquals($expectedValues, $values); + + \Civi::settings()->set('logging', 0); + } + + /** + * The issue is that in a unit test the log_conn_id is going to + * be the same throughout the entire test, which is not how it works normally + * when you have separate page requests. So use the fact that the conn_id + * format is controlled by a hidden variable, so we can force different + * conn_id's during initialization and after. + * If we don't do this, then the report thinks EVERY log record is part + * of the one change detail. + * + * On the plus side, this doesn't affect other tests since if they enable + * logging then that'll just recreate the variable and triggers. + */ + private function doQuestionableStuffInASeparateFunctionSoNobodyNotices(): void { + CRM_Core_DAO::executeQuery("DELETE FROM civicrm_setting WHERE name='logging_uniqueid_date'"); + // Now we have to rebuild triggers because the format formula is stored in + // every trigger. + CRM_Core_Config::singleton(TRUE, TRUE); + \Civi::service('sql_triggers')->rebuild(NULL, TRUE); + } + } -- 2.25.1