Merge pull request #26878 from colemanw/locationTypeFixes

[REF] LocationType - Set defaults, modernize form and BAO
civicrm · Jul 20, 2023 · ac9ed92 · ac9ed92
2 parents 97c67a6 + 7f7d8c7
commit ac9ed92
Show file tree

Hide file tree

Showing 12 changed files with 106 additions and 73 deletions.
diff --git a/CRM/Admin/Form/LocationType.php b/CRM/Admin/Form/LocationType.php
@@ -65,8 +65,8 @@ public function buildQuickForm() {
 
     $this->add('text', 'description', ts('Description'), CRM_Core_DAO::getAttribute('CRM_Core_DAO_LocationType', 'description'));
 
-    $this->add('checkbox', 'is_active', ts('Enabled?'));
-    $this->add('checkbox', 'is_default', ts('Default?'));
+    $this->add('advcheckbox', 'is_active', ts('Enabled?'));
+    $this->add('advcheckbox', 'is_default', ts('Default?'));
 
     if ($this->_action & CRM_Core_Action::UPDATE) {
       if (CRM_Core_DAO::getFieldValue('CRM_Core_DAO_LocationType', $this->_id, 'is_reserved')) {
@@ -82,38 +82,19 @@ public function buildQuickForm() {
    * Process the form submission.
    */
   public function postProcess() {
-    CRM_Utils_System::flushCache();
-
+    // Delete action
     if ($this->_action & CRM_Core_Action::DELETE) {
       CRM_Core_BAO_LocationType::deleteRecord(['id' => $this->_id]);
       CRM_Core_Session::setStatus(ts('Selected Location type has been deleted.'), ts('Record Deleted'), 'success');
       return;
     }
-
-    // store the submitted values in an array
+    // Create or update actions
     $params = $this->exportValues();
-    $params['is_active'] = CRM_Utils_Array::value('is_active', $params, FALSE);
-    $params['is_default'] = CRM_Utils_Array::value('is_default', $params, FALSE);
-
-    // action is taken depending upon the mode
-    $locationType = new CRM_Core_DAO_LocationType();
-    $locationType->name = $params['name'];
-    $locationType->display_name = $params['display_name'];
-    $locationType->vcard_name = $params['vcard_name'];
-    $locationType->description = $params['description'];
-    $locationType->is_active = $params['is_active'];
-    $locationType->is_default = $params['is_default'];
-
-    if ($params['is_default']) {
-      $query = "UPDATE civicrm_location_type SET is_default = 0";
-      CRM_Core_DAO::executeQuery($query);
-    }
-
     if ($this->_action & CRM_Core_Action::UPDATE) {
-      $locationType->id = $this->_id;
+      $params['id'] = $this->_id;
     }
 
-    $locationType->save();
+    $locationType = CRM_Core_BAO_LocationType::writeRecord($params);
 
     CRM_Core_Session::setStatus(ts("The location type '%1' has been saved.",
       [1 => $locationType->name]

diff --git a/CRM/Admin/Page/LocationType.php b/CRM/Admin/Page/LocationType.php
@@ -76,7 +76,7 @@ protected function getRows($sort, $action, array $links): array {
     foreach ($rows as &$row) {
       // prevent smarty notices.
       foreach (['is_default', 'class', 'vcard_name'] as $expectedField) {
-        if (!isset($row['is_default'])) {
+        if (!isset($row[$expectedField])) {
           $row[$expectedField] = NULL;
         }
       }

diff --git a/CRM/Core/BAO/LocationType.php b/CRM/Core/BAO/LocationType.php
@@ -80,27 +80,13 @@ public static function getBilling() {
    * Add a Location Type.
    *
    * @param array $params
-   *   Reference array contains the values submitted by the form.
    *
-   *
-   * @return object
+   * @deprecated
+   * @return CRM_Core_DAO_LocationType
    */
   public static function create(&$params) {
-    if (empty($params['id'])) {
-      $params['is_active'] = CRM_Utils_Array::value('is_active', $params, FALSE);
-      $params['is_default'] = CRM_Utils_Array::value('is_default', $params, FALSE);
-      $params['is_reserved'] = CRM_Utils_Array::value('is_reserved', $params, FALSE);
-    }
-
-    $locationType = new CRM_Core_DAO_LocationType();
-    $locationType->copyValues($params);
-    if (!empty($params['is_default'])) {
-      $query = "UPDATE civicrm_location_type SET is_default = 0";
-      CRM_Core_DAO::executeQuery($query);
-    }
-
-    $locationType->save();
-    return $locationType;
+    CRM_Core_Error::deprecatedFunctionWarning('writeRecord');
+    return self::writeRecord($params);
   }
 
   /**
@@ -129,6 +115,14 @@ public static function self_hook_civicrm_pre(\Civi\Core\Event\PreEvent $event) {
         ]);
       }
     }
+    elseif (in_array($event->action, ['create', 'edit'])) {
+      if (!empty($event->params['is_default'])) {
+        $query = "UPDATE civicrm_location_type SET is_default = 0";
+        CRM_Core_DAO::executeQuery($query);
+      }
+    }
+    // Todo: This was moved from CRM_Admin_Form_LocationType::postProcess but is probably unnecessarily broad.
+    CRM_Utils_System::flushCache();
   }
 
 }
diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php
@@ -3315,7 +3315,8 @@ private function makeNameFromLabel(): void {
       // No unique index on "name", do nothing
       return;
     }
-    $label = $this->label ?? $this->title ?? NULL;
+    $labelField = $this::$_labelField;
+    $label = $this->$labelField ?? NULL;
     if (!$label && $label !== '0') {
       // No label supplied, do nothing
       return;

diff --git a/CRM/Core/DAO/LocationType.php b/CRM/Core/DAO/LocationType.php
@@ -6,7 +6,7 @@
  *
  * Generated from xml/schema/CRM/Core/LocationType.xml
  * DO NOT EDIT.  Generated by CRM_Core_CodeGen
- * (GenCodeChecksum:035ebc0bafa88ffbc21d969b983399ef)
+ * (GenCodeChecksum:64f83ce54ddb0f9cc115a807e773f9cf)
  */
 
 /**
@@ -23,6 +23,13 @@ class CRM_Core_DAO_LocationType extends CRM_Core_DAO {
    */
   public static $_tableName = 'civicrm_location_type';
 
+  /**
+   * Field to show when displaying a record.
+   *
+   * @var string
+   */
+  public static $_labelField = 'display_name';
+
   /**
    * Should CiviCRM log any modifications to this table in the civicrm_log table.
    *
@@ -53,7 +60,7 @@ class CRM_Core_DAO_LocationType extends CRM_Core_DAO {
   /**
    * Location Type Name.
    *
-   * @var string|null
+   * @var string
    *   (SQL type: varchar(64))
    *   Note that values will be retrieved from the database as a string.
    */
@@ -62,7 +69,7 @@ class CRM_Core_DAO_LocationType extends CRM_Core_DAO {
   /**
    * Location Type Display Name.
    *
-   * @var string|null
+   * @var string
    *   (SQL type: varchar(64))
    *   Note that values will be retrieved from the database as a string.
    */
@@ -89,7 +96,7 @@ class CRM_Core_DAO_LocationType extends CRM_Core_DAO {
   /**
    * Is this location type a predefined system location?
    *
-   * @var bool|string|null
+   * @var bool|string
    *   (SQL type: tinyint)
    *   Note that values will be retrieved from the database as a string.
    */
@@ -98,7 +105,7 @@ class CRM_Core_DAO_LocationType extends CRM_Core_DAO {
   /**
    * Is this property active?
    *
-   * @var bool|string|null
+   * @var bool|string
    *   (SQL type: tinyint)
    *   Note that values will be retrieved from the database as a string.
    */
@@ -107,7 +114,7 @@ class CRM_Core_DAO_LocationType extends CRM_Core_DAO {
   /**
    * Is this location type the default?
    *
-   * @var bool|string|null
+   * @var bool|string
    *   (SQL type: tinyint)
    *   Note that values will be retrieved from the database as a string.
    */
@@ -167,6 +174,7 @@ public static function &fields() {
           'type' => CRM_Utils_Type::T_STRING,
           'title' => ts('Location Type'),
           'description' => ts('Location Type Name.'),
+          'required' => TRUE,
           'maxlength' => 64,
           'size' => CRM_Utils_Type::BIG,
           'usage' => [
@@ -180,13 +188,17 @@ public static function &fields() {
           'entity' => 'LocationType',
           'bao' => 'CRM_Core_BAO_LocationType',
           'localizable' => 0,
+          'html' => [
+            'type' => 'Text',
+          ],
           'add' => '1.1',
         ],
         'display_name' => [
           'name' => 'display_name',
           'type' => CRM_Utils_Type::T_STRING,
           'title' => ts('Display Name'),
           'description' => ts('Location Type Display Name.'),
+          'required' => TRUE,
           'maxlength' => 64,
           'size' => CRM_Utils_Type::BIG,
           'usage' => [
@@ -200,6 +212,9 @@ public static function &fields() {
           'entity' => 'LocationType',
           'bao' => 'CRM_Core_BAO_LocationType',
           'localizable' => 1,
+          'html' => [
+            'type' => 'Text',
+          ],
           'add' => '4.1',
         ],
         'vcard_name' => [
@@ -220,6 +235,9 @@ public static function &fields() {
           'entity' => 'LocationType',
           'bao' => 'CRM_Core_BAO_LocationType',
           'localizable' => 0,
+          'html' => [
+            'type' => 'Text',
+          ],
           'add' => '1.1',
         ],
         'description' => [
@@ -241,7 +259,7 @@ public static function &fields() {
           'bao' => 'CRM_Core_BAO_LocationType',
           'localizable' => 0,
           'html' => [
-            'label' => ts("Description"),
+            'type' => 'Text',
           ],
           'add' => '1.1',
         ],
@@ -250,31 +268,39 @@ public static function &fields() {
           'type' => CRM_Utils_Type::T_BOOLEAN,
           'title' => ts('Location Type is Reserved?'),
           'description' => ts('Is this location type a predefined system location?'),
+          'required' => TRUE,
           'usage' => [
             'import' => FALSE,
             'export' => FALSE,
             'duplicate_matching' => FALSE,
             'token' => FALSE,
           ],
           'where' => 'civicrm_location_type.is_reserved',
+          'default' => '0',
           'table_name' => 'civicrm_location_type',
           'entity' => 'LocationType',
           'bao' => 'CRM_Core_BAO_LocationType',
           'localizable' => 0,
+          'html' => [
+            'type' => 'CheckBox',
+            'label' => ts("Reserved"),
+          ],
           'add' => '1.1',
         ],
         'is_active' => [
           'name' => 'is_active',
           'type' => CRM_Utils_Type::T_BOOLEAN,
           'title' => ts('Location Type is Active?'),
           'description' => ts('Is this property active?'),
+          'required' => TRUE,
           'usage' => [
             'import' => FALSE,
             'export' => FALSE,
             'duplicate_matching' => FALSE,
             'token' => FALSE,
           ],
           'where' => 'civicrm_location_type.is_active',
+          'default' => '1',
           'table_name' => 'civicrm_location_type',
           'entity' => 'LocationType',
           'bao' => 'CRM_Core_BAO_LocationType',
@@ -290,13 +316,15 @@ public static function &fields() {
           'type' => CRM_Utils_Type::T_BOOLEAN,
           'title' => ts('Default Location Type?'),
           'description' => ts('Is this location type the default?'),
+          'required' => TRUE,
           'usage' => [
             'import' => FALSE,
             'export' => FALSE,
             'duplicate_matching' => FALSE,
             'token' => FALSE,
           ],
           'where' => 'civicrm_location_type.is_default',
+          'default' => '0',
           'table_name' => 'civicrm_location_type',
           'entity' => 'LocationType',
           'bao' => 'CRM_Core_BAO_LocationType',

diff --git a/CRM/Core/I18n/SchemaStructure.php b/CRM/Core/I18n/SchemaStructure.php
@@ -30,7 +30,7 @@ public static function &columns() {
     if (!$result) {
       $result = [
         'civicrm_location_type' => [
-          'display_name' => "varchar(64) COMMENT 'Location Type Display Name.'",
+          'display_name' => "varchar(64) NOT NULL COMMENT 'Location Type Display Name.'",
         ],
         'civicrm_option_group' => [
           'title' => "varchar(255) COMMENT 'Option Group title.'",
@@ -266,6 +266,7 @@ public static function &widgets() {
         'civicrm_location_type' => [
           'display_name' => [
             'type' => "Text",
+            'required' => "true",
           ],
         ],
         'civicrm_option_group' => [

diff --git a/CRM/Upgrade/Incremental/php/FiveSixtyFive.php b/CRM/Upgrade/Incremental/php/FiveSixtyFive.php
@@ -29,7 +29,12 @@ class CRM_Upgrade_Incremental_php_FiveSixtyFive extends CRM_Upgrade_Incremental_
    */
   public function upgrade_5_65_alpha1($rev): void {
     $this->addTask(ts('Upgrade DB to %1: SQL', [1 => $rev]), 'runSql', $rev);
-    // These 3 should run after the sql file.
+    // These should run after the sql file.
+    $this->addTask('Make LocationType.name required', 'alterColumn', 'civicrm_location_type', 'name', "varchar(64) NOT NULL COMMENT 'Location Type Name.'");
+    $this->addTask('Make LocationType.display_name required', 'alterColumn', 'civicrm_location_type', 'display_name', "varchar(64) NOT NULL COMMENT 'Location Type Display Name.'", TRUE);
+    $this->addTask('Make LocationType.is_reserved required', 'alterColumn', 'civicrm_location_type', 'is_reserved', "tinyint NOT NULL DEFAULT 0 COMMENT 'Is this location type a predefined system location?'");
+    $this->addTask('Make LocationType.is_active required', 'alterColumn', 'civicrm_location_type', 'is_active', "tinyint NOT NULL DEFAULT 1 COMMENT 'Is this property active?'");
+    $this->addTask('Make LocationType.is_default required', 'alterColumn', 'civicrm_location_type', 'is_default', "tinyint NOT NULL DEFAULT 0 COMMENT 'Is this location type the default?'");
     $this->addTask('Make Group.name required', 'alterColumn', 'civicrm_group', 'name', "varchar(255) NOT NULL COMMENT 'Internal name of Group.'");
     $this->addTask('Make Group.title required', 'alterColumn', 'civicrm_group', 'title', "varchar(255) NOT NULL COMMENT 'Alternative public title for this Group.'", TRUE);
     $this->addTask('Make Group.frontend_title required', 'alterColumn', 'civicrm_group', 'frontend_title', "varchar(255) NOT NULL COMMENT 'Alternative public description of the group.'", TRUE);

diff --git a/CRM/Upgrade/Incremental/sql/5.65.alpha1.mysql.tpl b/CRM/Upgrade/Incremental/sql/5.65.alpha1.mysql.tpl
@@ -36,3 +36,9 @@ body_text = REPLACE(body_text, '{welcome.group}', '{group.frontend_title}'),
 subject = REPLACE(subject, '{welcome.group}', '{group.frontend_title}')
 WHERE component_type = 'Welcome';
 {/literal}
+
+UPDATE `civicrm_location_type` SET `is_reserved` = 0 WHERE `is_reserved` IS NULL;
+UPDATE `civicrm_location_type` SET `is_active` = 0 WHERE `is_active` IS NULL;
+UPDATE `civicrm_location_type` SET `is_default` = 0 WHERE `is_default` IS NULL;
+UPDATE `civicrm_location_type` SET `name` = CONCAT('location_', id) WHERE `name` IS NULL;
+UPDATE `civicrm_location_type` SET {localize field=display_name}`display_name` = COALESCE(`display_name`, `name`){/localize};
diff --git a/sql/civicrm_data/civicrm_location_type.sqldata.php b/sql/civicrm_data/civicrm_location_type.sqldata.php
@@ -2,35 +2,31 @@
 
 return CRM_Core_CodeGen_SqlData::create('civicrm_location_type')
   ->addValues([
-    // CRM-9120 for legacy reasons we are continuing to translate the 'name', but this
-    // field is used mainly as an ID, and display_name will be shown to the user, but
-    // we have not yet finished modifying all places where the 'name' is shown.
     [
-      'name' => ts('Home'),
+      'name' => 'Home',
       'display_name' => ts('Home'),
       'vcard_name' => 'HOME',
       'description' => ts('Place of residence'),
       'is_default' => 1,
     ],
     [
-      'name' => ts('Work'),
+      'name' => 'Work',
       'display_name' => ts('Work'),
       'vcard_name' => 'WORK',
       'description' => ts('Work location'),
     ],
     [
-      'name' => ts('Main'),
+      'name' => 'Main',
       'display_name' => ts('Main'),
       'vcard_name' => NULL,
       'description' => ts('Main office location'),
     ],
     [
-      'name' => ts('Other'),
+      'name' => 'Other',
       'display_name' => ts('Other'),
       'vcard_name' => NULL,
       'description' => ts('Other location'),
     ],
-    // -- the following location must stay with the untranslated Billing name, CRM-2064
     [
       'name' => 'Billing',
       'display_name' => ts('Billing'),
@@ -41,7 +37,6 @@
   ])
   ->addDefaults([
     'is_active' => 1,
-    'is_default' => NULL,
+    'is_default' => 0,
     'is_reserved' => 0,
-    // FIXME: Doesn't 0 make more sense than NULL?
   ]);