Skip to content

Commit

Permalink
dev/core#3063 APIv3 - Fix numeric option matching
Browse files Browse the repository at this point in the history
Before: Option matching was skipped for all FK fields if a numeric value was given
After: Only skipped for `campaign_id` field, if positive integer given

The optimization was overly broad and had unintended side-effects
  • Loading branch information
colemanw committed Feb 10, 2022
1 parent c27d601 commit 5727486
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 4 deletions.
8 changes: 4 additions & 4 deletions api/v3/utils.php
Original file line number Diff line number Diff line change
Expand Up @@ -2077,10 +2077,10 @@ function _civicrm_api3_validate_integer(&$params, $fieldName, &$fieldInfo, $enti
}
}
if (
(!empty($fieldInfo['pseudoconstant']) || !empty($fieldInfo['options']) || $fieldName === 'campaign_id')
// if it is already numeric AND it is an FK field we don't need to validate because
// sql will do that for us on insert (this also saves a big lookup)
&& (!is_numeric($fieldValue) || empty($fieldInfo['FKClassName']))
!empty($fieldInfo['pseudoconstant']) ||
!empty($fieldInfo['options']) ||
// Special case for campaign_id which is no longer a pseudoconstant
($fieldName === 'campaign_id' && !CRM_Utils_Rule::positiveInteger($fieldValue))
) {
$additional_lookup_params = [];
if (strtolower($entity) === 'address' && $fieldName == 'state_province_id') {
Expand Down
44 changes: 44 additions & 0 deletions tests/phpunit/api/v3/FinancialTypeTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -114,4 +114,48 @@ public function testAssociatedFinancialAccountGetsCreated($apiVersion) {
$this->assertEquals('INC', $result['account_type_code'], 'Financial account created is not an income account.');
}

public function testMatchFinancialTypeOptions() {
// Just a string name, should be simple to match on
$nonNumericOption = $this->callAPISuccess('FinancialType', 'create', [
'name' => 'StringName',
])['id'];
// A numeric name, but a number that won't match any existing id
$numericOptionUnique = $this->callAPISuccess('FinancialType', 'create', [
'name' => '999',
])['id'];
// Here's the kicker, a numeric name that matches an existing id!
$numericOptionMatchingExistingId = $this->callAPISuccess('FinancialType', 'create', [
'name' => $nonNumericOption,
])['id'];
$cid = $this->individualCreate();

// Create a contribution matching non-numeric name
$contributionWithNonNumericType = $this->callAPISuccess('Contribution', 'create', [
'financial_type_id' => 'StringName',
'total_amount' => 100,
'contact_id' => $cid,
'sequential' => TRUE,
]);
$this->assertEquals($nonNumericOption, $contributionWithNonNumericType['values'][0]['financial_type_id']);

// Create a contribution matching unique numeric name
$contributionWithUniqueNumericType = $this->callAPISuccess('Contribution', 'create', [
'financial_type_id' => '999',
'total_amount' => 100,
'contact_id' => $cid,
'sequential' => TRUE,
]);
$this->assertEquals($numericOptionUnique, $contributionWithUniqueNumericType['values'][0]['financial_type_id']);

// Create a contribution matching the id of the non-numeric option, which is ambiguously the name of another option
$contributionWithAmbiguousNumericType = $this->callAPISuccess('Contribution', 'create', [
'financial_type_id' => "$nonNumericOption",
'total_amount' => 100,
'contact_id' => $cid,
'sequential' => TRUE,
]);
// The id should have taken priority over matching by name
$this->assertEquals($nonNumericOption, $contributionWithAmbiguousNumericType['values'][0]['financial_type_id']);
}

}

0 comments on commit 5727486

Please sign in to comment.