Merge pull request #28707 from colemanw/reportInstanceCleanup

ReportInstance - Cleanup BAO::add function and DAO metadata
civicrm · Dec 19, 2023 · 907a062 · 907a062
2 parents 9ec0208 + 4b76283
commit 907a062
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 75 deletions.
diff --git a/CRM/Report/BAO/ReportInstance.php b/CRM/Report/BAO/ReportInstance.php
@@ -17,88 +17,30 @@
 class CRM_Report_BAO_ReportInstance extends CRM_Report_DAO_ReportInstance implements Civi\Core\HookInterface {
 
   /**
-   * Takes an associative array and creates an instance object.
-   *
-   * the function extract all the params it needs to initialize the create a
-   * instance object. the params array could contain additional unused name/value
-   * pairs
+   * Function ought to be deprecated in favor of self::writeRecord() once the fixmes are addressed.
    *
    * @param array $params
-   *   (reference ) an assoc array of name/value pairs.
-   *
    * @return CRM_Report_DAO_ReportInstance
    */
-  public static function add(&$params) {
+  public static function add($params) {
     if (empty($params)) {
       return NULL;
     }
 
-    $instanceID = CRM_Utils_Array::value('id', $params, CRM_Utils_Array::value('instance_id', $params));
-
-    // convert roles array to string
-    if (isset($params['grouprole']) && is_array($params['grouprole'])) {
-      $grouprole_array = [];
-      foreach ($params['grouprole'] as $key => $value) {
-        $grouprole_array[$value] = $value;
-      }
-      $params['grouprole'] = implode(CRM_Core_DAO::VALUE_SEPARATOR,
-        array_keys($grouprole_array)
-      );
-    }
-
-    if (!$instanceID || !isset($params['id'])) {
-      $params['is_reserved'] ??= FALSE;
+    if (!isset($params['id'])) {
       $params['domain_id'] ??= CRM_Core_Config::domainID();
       // CRM-17256 set created_id on report creation.
       $params['created_id'] ??= CRM_Core_Session::getLoggedInContactID();
+      $params['grouprole'] ??= '';
+      // FIXME: This probably belongs in the form layer
+      $params['report_id'] ??= CRM_Report_Utils_Report::getValueFromUrl();
     }
-
-    if ($instanceID) {
-      CRM_Utils_Hook::pre('edit', 'ReportInstance', $instanceID, $params);
-    }
-    else {
-      CRM_Utils_Hook::pre('create', 'ReportInstance', NULL, $params);
-    }
-
-    $instance = new CRM_Report_DAO_ReportInstance();
-    $instance->copyValues($params);
-
+    // Fixme: Why is this even necessary?
     if (CRM_Core_Config::singleton()->userFramework == 'Joomla') {
-      $instance->permission = 'null';
-    }
-
-    // explicitly set to null if params value is empty
-    if (!$instanceID && empty($params['grouprole'])) {
-      $instance->grouprole = 'null';
-    }
-
-    if ($instanceID) {
-      $instance->id = $instanceID;
+      $params['permission'] = '';
     }
 
-    if (!$instanceID) {
-      $reportID = $params['report_id'] ?? NULL;
-      if ($reportID) {
-        $instance->report_id = $reportID;
-      }
-      elseif ($instanceID) {
-        $instance->report_id = CRM_Report_Utils_Report::getValueFromUrl($instanceID);
-      }
-      else {
-        // just take it from current url
-        $instance->report_id = CRM_Report_Utils_Report::getValueFromUrl();
-      }
-    }
-
-    $instance->save();
-
-    if ($instanceID) {
-      CRM_Utils_Hook::post('edit', 'ReportInstance', $instance->id, $instance);
-    }
-    else {
-      CRM_Utils_Hook::post('create', 'ReportInstance', $instance->id, $instance);
-    }
-    return $instance;
+    return self::writeRecord($params);
   }
 
   /**
@@ -111,9 +53,11 @@ public static function add(&$params) {
    * @param array $params
    *   (reference ) an assoc array of name/value pairs.
    *
-   * @return CRM_Report_BAO_ReportInstance
+   * @return CRM_Report_DAO_ReportInstance
    */
   public static function &create(&$params) {
+    // Transform nonstandard field names used by quickform
+    $params['id'] ??= ($params['instance_id'] ?? NULL);
     if (isset($params['report_header'])) {
       $params['header'] = $params['report_header'];
     }
@@ -175,7 +119,7 @@ public static function &create(&$params) {
 
     // add / update navigation as required
     if (!empty($navigationParams)) {
-      if (empty($params['id']) && empty($params['instance_id']) && !empty($navigationParams['id'])) {
+      if (empty($params['id']) && !empty($navigationParams['id'])) {
         unset($navigationParams['id']);
       }
       $navigationParams['url'] = "civicrm/report/instance/{$instance->id}" . ($viewMode == 'view' ? '?reset=1&force=1' : '?reset=1&output=criteria');
@@ -387,4 +331,12 @@ public static function getActionMetadata() {
     return $actions;
   }
 
+  /**
+   * Pseudoconstant callback for the `grouprole` field
+   * @return array
+   */
+  public static function getGrouproleOptions(): array {
+    return (array) CRM_Core_Config::singleton()->userSystem->getRoleNames();
+  }
+
 }
diff --git a/CRM/Report/DAO/ReportInstance.php b/CRM/Report/DAO/ReportInstance.php
@@ -6,7 +6,7 @@
  *
  * Generated from xml/schema/CRM/Report/ReportInstance.xml
  * DO NOT EDIT.  Generated by CRM_Core_CodeGen
- * (GenCodeChecksum:fc3071b4f3773306b8688f3a0e3c78be)
+ * (GenCodeChecksum:7434f5737d99543263d96a6b16c9ae99)
  */
 
 /**
@@ -463,7 +463,8 @@ public static function &fields() {
           'bao' => 'CRM_Report_BAO_ReportInstance',
           'localizable' => 0,
           'html' => [
-            'type' => 'Text',
+            'type' => 'Select',
+            'label' => ts("Permission"),
           ],
           'add' => '2.2',
         ],
@@ -485,8 +486,12 @@ public static function &fields() {
           'entity' => 'ReportInstance',
           'bao' => 'CRM_Report_BAO_ReportInstance',
           'localizable' => 0,
+          'serialize' => self::SERIALIZE_SEPARATOR_TRIMMED,
           'html' => [
-            'type' => 'Text',
+            'type' => 'Select',
+          ],
+          'pseudoconstant' => [
+            'callback' => 'CRM_Report_BAO_ReportInstance::getGrouproleOptions',
           ],
           'add' => '4.1',
         ],

diff --git a/CRM/Report/Form/Instance.php b/CRM/Report/Form/Instance.php
@@ -102,7 +102,7 @@ public static function buildForm(&$form) {
       $form->freeze('is_reserved');
     }
 
-    $getPerms = \Civi\Api4\Permission::get(0)
+    $getPerms = \Civi\Api4\Permission::get(FALSE)
       ->addWhere('is_active', '=', 1)
       ->addWhere('group', 'IN', ['civicrm', 'cms', 'const'])
       ->setOrderBy(['title' => 'ASC'])

diff --git a/xml/schema/Report/ReportInstance.xml b/xml/schema/Report/ReportInstance.xml
@@ -109,7 +109,8 @@
     <length>255</length>
     <comment>permission required to be able to run this instance</comment>
     <html>
-      <type>Text</type>
+      <label>Permission</label>
+      <type>Select</type>
     </html>
     <add>2.2</add>
   </field>
@@ -121,8 +122,14 @@
     <comment>role required to be able to run this instance</comment>
     <add>4.1</add>
     <html>
-      <type>Text</type>
+      <title>ACL Group/Role</title>
+      <type>Select</type>
+      <multiple>1</multiple>
     </html>
+    <serialize>SEPARATOR_TRIMMED</serialize>
+    <pseudoconstant>
+      <callback>CRM_Report_BAO_ReportInstance::getGrouproleOptions</callback>
+    </pseudoconstant>
   </field>
   <field>
     <name>form_values</name>