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

Add unique index to DedupeRuleGroup.name field #24385

Merged
merged 1 commit into from
Aug 26, 2022

Conversation

colemanw
Copy link
Member

Overview

Ensures DedupeRuleGroup.name field is always unique by adding a db constraint.

Before

Uniqueness was sorta ensured at the form layer.

After

Centralized logic from #22570 plus a unique constraint ensures that it's always unique. Form layer creativity removed.

Comments

Yea... there's kind of no point having a "name" field at all if we don't enforce uniqueness.

@civibot
Copy link

civibot bot commented Aug 25, 2022

(Standard links)

@civibot civibot bot added the master label Aug 25, 2022
@colemanw colemanw force-pushed the dedupeRuleGroupName branch from 9135b37 to a1c8007 Compare August 25, 2022 21:34
@colemanw
Copy link
Member Author

All failures were due to test setup creating duplicate rule names. Fixed.

*/
public static function alterDedupeRuleGroupName(CRM_Queue_TaskContext $ctx) {
CRM_Core_DAO::executeQuery("ALTER TABLE `civicrm_dedupe_rule_group` CHANGE COLUMN `name` `name` varchar(255) COMMENT 'Unique name of rule group'", [], TRUE, NULL, FALSE, FALSE);
CRM_Core_DAO::executeQuery("UPDATE `civicrm_dedupe_rule_group` g1, `civicrm_dedupe_rule_group` g2 SET g1.name = CONCAT(g1.name, '_', g1.id) WHERE g1.name = g2.name AND g1.id > g2.id", [], TRUE, NULL, FALSE, FALSE);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to reverse the order of these two statements

Copy link
Member Author

Choose a reason for hiding this comment

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

@eileenmcnaughton why? The first one just makes the column longer, which makes room for the suffix appended in the 2nd statement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think the id > will be safe in practice - but we don't want to rename anything that is_reserved as the name has meaning then

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually no - the issue is you aren't creating the indexes - CRM_Core_BAO_SchemaHandler::createIndexes('civicrm_dedupe_rule_group', ['name']);

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm - maybe you are but not here...

@@ -39,6 +39,8 @@ public function setPreUpgradeMessage(&$preUpgradeMessage, $rev, $currentVer = NU
public function upgrade_5_54_alpha1($rev): void {
$this->addTask(ts('Upgrade DB to %1: SQL', [1 => $rev]), 'runSql', $rev);
$this->addTask('Add "created_id" column to "civicrm_participant"', 'addCreatedIDColumnToParticipant');
$this->addTask('Increase field length of civicrm_dedupe_rule_group.name', 'alterDedupeRuleGroupName');
$this->addTask('Add index civicrm_dedupe_rule_group.UI_name', 'addIndex', 'civicrm_dedupe_rule_group', 'name', 'UI');
Copy link
Contributor

Choose a reason for hiding this comment

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

ah

@eileenmcnaughton
Copy link
Contributor

OK - I've been my own ruber duck - all good, merging

@eileenmcnaughton eileenmcnaughton merged commit 9137d2a into civicrm:master Aug 26, 2022
@eileenmcnaughton eileenmcnaughton deleted the dedupeRuleGroupName branch August 26, 2022 00:50
@colemanw
Copy link
Member Author

I have no idea what that means, but thanks for merging :)

@seamuslee001
Copy link
Contributor

@colemanw I believe this was in reference to this https://rubberduckdebugging.com/ but Eileen suggesting she was playing the role of the duck as well

@eileenmcnaughton
Copy link
Contributor

quack

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants