Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure custom group name does not conflict with existing field #20694

Merged
merged 1 commit into from
Jun 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions CRM/Core/BAO/CustomGroup.php
Original file line number Diff line number Diff line change
Expand Up @@ -142,14 +142,16 @@ public static function create(&$params) {
$group->created_id = $params['created_id'] ?? NULL;
$group->created_date = $params['created_date'] ?? NULL;

// we do this only once, so name never changes
if (isset($params['name'])) {
$group->name = CRM_Utils_String::munge($params['name'], '_', 64);
// Process name only during create, so it never changes
if (!empty($params['name'])) {
$group->name = CRM_Utils_String::munge($params['name']);
}
else {
$group->name = CRM_Utils_String::munge($group->title, '_', 64);
$group->name = CRM_Utils_String::munge($group->title);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting the default max length is 63 so this will work with the current table definition

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this PR changes it from 64 down to 63 (which happens to be the default for that function) to accommodate the potential extra character inserted during validation.

}

self::validateCustomGroupName($group);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colemanw should this only be run on creation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right I see that now


if (isset($params['table_name'])) {
$tableName = $params['table_name'];

Expand Down Expand Up @@ -225,6 +227,22 @@ public static function retrieve(&$params, &$defaults) {
return CRM_Core_DAO::commonRetrieve('CRM_Core_DAO_CustomGroup', $params, $defaults);
}

/**
* Ensure group name does not conflict with an existing field
*
* @param CRM_Core_DAO_CustomGroup $group
*/
public static function validateCustomGroupName(CRM_Core_DAO_CustomGroup $group) {
$extends = in_array($group->extends, CRM_Contact_BAO_ContactType::basicTypes(TRUE)) ? 'Contact' : $group->extends;
$extendsDAO = CRM_Core_DAO_AllCoreTables::getFullName($extends);
if ($extendsDAO) {
$fields = array_column($extendsDAO::fields(), 'name');
if (in_array($group->name, $fields)) {
$group->name .= '0';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colemanw what happens if you try to create a 2nd ActivityType Custom Group? i.e. one already has a 0 on it would that be an issue (this is probably quite the edge case but just thinking)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current behavior is that if you try to create a custom group with a duplicate name (or for that matter, a duplicate title) then you'll get a DB error due to the unique key constraints.

This PR doesn't alter that.

}
}
}

/**
* Update the is_active flag in the db.
*
Expand Down
6 changes: 3 additions & 3 deletions CRM/Core/DAO/AllCoreTables.php
Original file line number Diff line number Diff line change
Expand Up @@ -311,13 +311,13 @@ public static function getClassForTable($tableName) {
/**
* Given a brief-name, determine the full class-name.
*
* @param string $daoName
* @param string $briefName
* Ex: 'Contact'.
* @return string|CRM_Core_DAO|NULL
* Ex: 'CRM_Contact_DAO_Contact'.
*/
public static function getFullName($daoName) {
return CRM_Utils_Array::value($daoName, self::daoToClass());
public static function getFullName($briefName) {
return self::daoToClass()[$briefName] ?? NULL;
}

/**
Expand Down
36 changes: 36 additions & 0 deletions tests/phpunit/CRM/Core/BAO/CustomGroupTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -651,4 +651,40 @@ public function testExtractGetParamsReturnsDates() {
$this->assertEquals($extractedGetParams[$fieldName], '2017-06-13');
}

/**
* @return array
*/
public function getGroupNames(): array {
$data = [
['Individual', 'first_name', FALSE],
['Organization', 'first_name', FALSE],
['Contact', 'not_first_name', TRUE],
['Contact', 'gender_id', FALSE],
['Activity', 'activity_type_id', FALSE],
['Campaign', 'activity_type_id', TRUE],
['Campaign', 'campaign_type_id', FALSE],
];
// Add descriptive keys to data
$dataSet = [];
foreach ($data as $item) {
$dataSet[$item[0] . '.' . $item[1]] = $item;
}
return $dataSet;
}

/**
* @param string $extends
* @param string $name
* @param bool $isAllowed
* @dataProvider getGroupNames
*/
public function testAllowedGroupNames(string $extends, string $name, bool $isAllowed) {
$group = new CRM_Core_DAO_CustomGroup();
$group->name = $name;
$group->extends = $extends;
$expectedName = $isAllowed ? $name : $name . '0';
CRM_Core_BAO_CustomGroup::validateCustomGroupName($group);
$this->assertEquals($expectedName, $group->name);
}

}