Merge pull request #27302 from colemanw/api4RequiredFix

APIv4 - Fix setting nullable/required/default_value field metadata
civicrm · Sep 5, 2023 · 390189a · 390189a
2 parents 2aabd2f + e420fe8
commit 390189a
Show file tree

Hide file tree

Showing 8 changed files with 53 additions and 46 deletions.
diff --git a/Civi/Api4/Service/Spec/Provider/ACLEntityRoleCreationSpecProvider.php b/Civi/Api4/Service/Spec/Provider/ACLEntityRoleCreationSpecProvider.php
diff --git a/Civi/Api4/Service/Spec/Provider/CustomValueSpecProvider.php b/Civi/Api4/Service/Spec/Provider/CustomValueSpecProvider.php
@@ -31,8 +31,10 @@ public function modifySpec(RequestSpec $spec) {
     $idField->setType('Field');
     $idField->setInputType('Number');
     $idField->setColumnName('id');
+    $idField->setNullable('false');
     $idField->setTitle(ts('Custom Value ID'));
     $idField->setReadonly(TRUE);
+    $idField->setNullable(FALSE);
     $spec->addFieldSpec($idField);
 
     $entityField = new FieldSpec('entity_id', $spec->getEntity(), 'Integer');
@@ -43,6 +45,7 @@ public function modifySpec(RequestSpec $spec) {
     $entityField->setRequired($action === 'create');
     $entityField->setFkEntity('Contact');
     $entityField->setReadonly(TRUE);
+    $entityField->setNullable(FALSE);
     $entityField->setInputType('EntityRef');
     $spec->addFieldSpec($entityField);
   }

diff --git a/Civi/Api4/Service/Spec/Provider/EmailCreationSpecProvider.php b/Civi/Api4/Service/Spec/Provider/EmailCreationSpecProvider.php
@@ -30,7 +30,7 @@ public function modifySpec(RequestSpec $spec) {
     $spec->getFieldByName('is_primary')->setRequired(FALSE);
 
     $defaultLocationType = \CRM_Core_BAO_LocationType::getDefault()->id ?? NULL;
-    $spec->getFieldByName('location_type_id')->setDefaultValue($defaultLocationType);
+    $spec->getFieldByName('location_type_id')->setDefaultValue($defaultLocationType ? (int) $defaultLocationType : NULL);
   }
 
   /**

diff --git a/Civi/Api4/Service/Spec/Provider/GetActionDefaultsProvider.php b/Civi/Api4/Service/Spec/Provider/GetActionDefaultsProvider.php
@@ -27,18 +27,18 @@ public function modifySpec(RequestSpec $spec) {
     // Exclude deleted records from api Get by default
     $isDeletedField = $spec->getFieldByName('is_deleted');
     if ($isDeletedField) {
-      $isDeletedField->setDefaultValue('0');
+      $isDeletedField->setDefaultValue(FALSE);
     }
 
     // Exclude test records from api Get by default
     $isTestField = $spec->getFieldByName('is_test');
     if ($isTestField) {
-      $isTestField->setDefaultValue('0');
+      $isTestField->setDefaultValue(FALSE);
     }
 
     $isTemplateField = $spec->getFieldByName('is_template');
     if ($isTemplateField) {
-      $isTemplateField->setDefaultValue('0');
+      $isTemplateField->setDefaultValue(FALSE);
     }
   }
 

diff --git a/Civi/Api4/Service/Spec/SpecFormatter.php b/Civi/Api4/Service/Spec/SpecFormatter.php
@@ -26,6 +26,8 @@ class SpecFormatter {
   public static function arrayToField(array $data, $entity) {
     $dataTypeName = self::getDataType($data);
 
+    $hasDefault = isset($data['default']) && $data['default'] !== '';
+    // Custom field
     if (!empty($data['custom_group_id'])) {
       $field = new CustomFieldSpec($data['name'], $entity, $dataTypeName);
       if (strpos($entity, 'Custom_') !== 0) {
@@ -57,13 +59,14 @@ public static function arrayToField(array $data, $entity) {
       }
       $field->setReadonly($data['is_view']);
     }
+    // Core field
     else {
       $name = $data['name'] ?? NULL;
       $field = new FieldSpec($name, $entity, $dataTypeName);
       $field->setType('Field');
       $field->setColumnName($name);
       $field->setNullable(empty($data['required']));
-      $field->setRequired(!empty($data['required']) && empty($data['default']));
+      $field->setRequired(!empty($data['required']) && !$hasDefault && $name !== 'id');
       $field->setTitle($data['title'] ?? NULL);
       $field->setLabel($data['html']['label'] ?? NULL);
       $field->setLocalizable($data['localizable'] ?? FALSE);
@@ -93,8 +96,10 @@ public static function arrayToField(array $data, $entity) {
       }
       $field->setReadonly(!empty($data['readonly']));
     }
+    if ($hasDefault) {
+      $field->setDefaultValue(FormattingUtil::convertDataType($data['default'], $dataTypeName));
+    }
     $field->setSerialize($data['serialize'] ?? NULL);
-    $field->setDefaultValue($data['default'] ?? NULL);
     $field->setDescription($data['description'] ?? NULL);
     $field->setDeprecated($data['deprecated'] ?? FALSE);
     self::setInputTypeAndAttrs($field, $data, $dataTypeName);

diff --git a/Civi/Api4/Service/Spec/SpecGatherer.php b/Civi/Api4/Service/Spec/SpecGatherer.php
@@ -91,9 +91,6 @@ private function addDAOFields($entity, $action, RequestSpec $spec, array $values
     $DAOFields = $this->getDAOFields($entity);
 
     foreach ($DAOFields as $DAOField) {
-      if ($DAOField['name'] == 'id' && $action == 'create') {
-        $DAOField['required'] = FALSE;
-      }
       if (array_key_exists('contactType', $DAOField) && $spec->getValue('contact_type') && $DAOField['contactType'] != $spec->getValue('contact_type')) {
         continue;
       }

diff --git a/tests/phpunit/api/v4/Action/GetFieldsTest.php b/tests/phpunit/api/v4/Action/GetFieldsTest.php
@@ -20,6 +20,7 @@
 namespace api\v4\Action;
 
 use api\v4\Api4TestBase;
+use Civi\Api4\ACLEntityRole;
 use Civi\Api4\Activity;
 use Civi\Api4\Address;
 use Civi\Api4\Campaign;
@@ -68,6 +69,13 @@ public function testContactGetFields(): void {
     // Check suffixes
     $this->assertEquals(['name', 'label', 'icon'], $fields['contact_type']['suffixes']);
     $this->assertEquals(['name', 'label', 'icon'], $fields['contact_sub_type']['suffixes']);
+
+    // Check `required` and `nullable`
+    $this->assertFalse($fields['is_deleted']['required']);
+    $this->assertFalse($fields['is_deleted']['nullable']);
+    $this->assertFalse($fields['id']['nullable']);
+    $this->assertFalse($fields['id']['required']);
+    $this->assertNull($fields['id']['default_value']);
   }
 
   public function testComponentFields(): void {
@@ -95,6 +103,15 @@ public function testEmailFields(): void {
       ->execute()->indexBy('name');
 
     $this->assertEquals('Email', $createFields['email']['input_type']);
+    $this->assertIsInt($createFields['location_type_id']['default_value']);
+
+    // Check `required` and `nullable`
+    $this->assertFalse($createFields['is_primary']['required']);
+    $this->assertFalse($createFields['is_primary']['nullable']);
+    $this->assertFalse($createFields['is_primary']['default_value']);
+    $this->assertFalse($createFields['id']['required']);
+    $this->assertFalse($createFields['id']['nullable']);
+    $this->assertNull($createFields['id']['default_value']);
   }
 
   public function testInternalPropsAreHidden(): void {
@@ -131,12 +148,32 @@ public function testRequiredAndNullableAndDeprecated(): void {
       ->execute()->indexBy('name');
 
     $this->assertFalse($actFields['id']['required']);
+    $this->assertFalse($actFields['id']['nullable']);
     $this->assertTrue($actFields['activity_type_id']['required']);
     $this->assertFalse($actFields['activity_type_id']['nullable']);
+    $this->assertNull($actFields['activity_type_id']['default_value']);
+    $this->assertFalse($actFields['is_deleted']['required']);
+    $this->assertFalse($actFields['is_deleted']['nullable']);
+    $this->assertFalse($actFields['is_deleted']['default_value']);
     $this->assertFalse($actFields['subject']['required']);
     $this->assertTrue($actFields['subject']['nullable']);
     $this->assertFalse($actFields['subject']['deprecated']);
     $this->assertTrue($actFields['phone_id']['deprecated']);
+
+    $getFields = Activity::getFields(FALSE)
+      ->setAction('get')
+      ->execute()->indexBy('name');
+
+    $this->assertFalse($getFields['is_deleted']['required']);
+    $this->assertFalse($getFields['is_deleted']['nullable']);
+    $this->assertFalse($getFields['is_deleted']['default_value']);
+
+    $aclFields = ACLEntityRole::getFields(FALSE)
+      ->setAction('create')
+      ->execute()->indexBy('name');
+    $this->assertTrue($aclFields['is_active']['default_value']);
+    $this->assertFalse($aclFields['is_active']['nullable']);
+    $this->assertFalse($aclFields['is_active']['required']);
   }
 
   public function testGetSuffixes(): void {

diff --git a/tests/phpunit/api/v4/Custom/CustomValueTest.php b/tests/phpunit/api/v4/Custom/CustomValueTest.php
@@ -138,6 +138,7 @@ public function testCRUD(): void {
         'entity' => "Custom_$group",
         'table_name' => $customGroup['table_name'],
         'column_name' => 'id',
+        'nullable' => FALSE,
         'data_type' => 'Integer',
         'fk_entity' => NULL,
       ],
@@ -148,6 +149,7 @@ public function testCRUD(): void {
         'table_name' => $customGroup['table_name'],
         'column_name' => 'entity_id',
         'entity' => "Custom_$group",
+        'nullable' => FALSE,
         'data_type' => 'Integer',
         'fk_entity' => 'Contact',
       ],