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 DedupeRule, DedupeRuleGroup and DedupeException API4 entity #20466

Merged
merged 2 commits into from
Jun 3, 2021

Conversation

monishdeb
Copy link
Member

@monishdeb monishdeb commented Jun 1, 2021

Overview

Add DedupeRule, DedupeRuleGroup and DedupeException APIv4 entity

Comments

ping @colemanw @eileenmcnaughton @seamuslee001

@civibot
Copy link

civibot bot commented Jun 1, 2021

(Standard links)

@civibot civibot bot added the master label Jun 1, 2021
@monishdeb monishdeb changed the title WIP Add DedupeRule API4 entity [WIP] Add DedupeRule API4 entity Jun 1, 2021
@monishdeb
Copy link
Member Author

monishdeb commented Jun 1, 2021

@eileenmcnaughton I have prepared the patch as per the suggestion in https://lab.civicrm.org/dev/core/-/issues/2486#note_56902 but only for DedupeRule entity. So as per the plan:

  1. I have declared a separate DedupeRule entity in CRM/Core/DAO/AllCoreTables.data.php along with Rule entity. I haven't renamed the existing one otherwise it will fail for api3 Rule entity.

  2. Added Duperule DAO/BAO

  3. Updated old DAO/BAO Rule class which extends DupeRule DAO/BAO respectively.

  4. Added DedupeRule API4 entity.

I am not sure if we need need to entirely discard the Rule entity declaration from xml and api3, in other words:

  1. Do we need to move xml/schema/Rule.xml to xml/schema/DupeRule.xml
  2. Move api/v3/Rule.php to api/v3/DedupeRule.php
  3. Make necessary changes in CRM/api3 unit tests

@@ -217,6 +217,11 @@
'class' => 'CRM_Dedupe_DAO_Rule',
'table' => 'civicrm_dedupe_rule',
],
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to get rid of CRM_Dedupe_DAO_Rule here.
Isn't this file autogenerated? I think that means it would disappear anyway the next time CodeGen is run.

What about adding this to fix APIv3:

diff --git a/api/v3/utils.php b/api/v3/utils.php
index d96aae6785..5abea6650c 100644
--- a/api/v3/utils.php
+++ b/api/v3/utils.php
@@ -309,6 +309,11 @@ function _civicrm_api3_get_DAO($name) {
     $name = 'Contact';
   }
 
+  // Entity was renamed for APIv4
+  if ($name === 'Rule') {
+    return 'CRM_Dedupe_DAO_DedupeRule';
+  }
+
   // hack to deal with incorrectly named BAO/DAO - see CRM-10859
 
   // FIXME: DAO should be renamed CRM_Mailing_DAO_MailingEventQueue

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm right, yeah thats another place in api3 to indicate the mapping between the old and new entity name. Updated the PR. Thanks for the patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

@colemanw keeping the old dao for a bit reduces the chance we break things as last dao renaming we had a few issues

@colemanw
Copy link
Member

colemanw commented Jun 2, 2021

Looks like the APIv4 ConformanceTest isn't going to be happy unless we also add the RuleGroup api.
That one could be renamed DedupeRuleGroup to match this one :)

@monishdeb monishdeb changed the title [WIP] Add DedupeRule API4 entity [WIP] Add DedupeRule, DedupeRuleGroup and DedupeException API4 entity Jun 2, 2021
@monishdeb
Copy link
Member Author

Jenkins test this please

1 similar comment
@seamuslee001
Copy link
Contributor

Jenkins test this please

@monishdeb
Copy link
Member Author

monishdeb commented Jun 2, 2021

Hmm , as per test build log something wrong with the xml

[[Restore "/home/jenkins/bknix-dfl/build/core-20466-33hpo/web" DB (test) from file (/home/jenkins/bknix-dfl/build/.civibuild/snapshot/core-20466-33hpo/civi.sql.gz)]]
Check test results and set exit code
Fatal: Found <error> in XML
Build step 'Execute shell' marked build as failure

Not sure about the error, but maybe due to renaming the schema xml lead to this error somehow.

@seamuslee001
Copy link
Contributor

PHP Fatal error:  Class 'CRM_Core_DedupeRuleGroup' not found in /home/jenkins/bknix-dfl/build/core-20466-33hpo/web/sites/all/modules/civicrm/CRM/Dedupe/DAO/RuleGroup.php on line 12

(in the full console log)

@monishdeb monishdeb force-pushed the add_deduperule_api4 branch 2 times, most recently from 1acd1f1 to 12d0326 Compare June 2, 2021 09:34
@monishdeb monishdeb force-pushed the add_deduperule_api4 branch from 12d0326 to bb4187d Compare June 2, 2021 10:14
@monishdeb monishdeb changed the title [WIP] Add DedupeRule, DedupeRuleGroup and DedupeException API4 entity Add DedupeRule, DedupeRuleGroup and DedupeException API4 entity Jun 3, 2021
@colemanw
Copy link
Member

colemanw commented Jun 3, 2021

Hooray! Fantastic work Monish!

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.

4 participants