dev/core#982 add dedupe.getstatistics api #14298
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(Standard links)
|
7cd216a to
65c137e
Compare
| if (!empty($previousStats)) { | ||
| if ($previousStats[0]['merged']) { | ||
| $merged = $merged + $previousStats[0]['merged']; | ||
| if ($previousStats['merged']) { |
There was a problem hiding this comment.
this was hit by tests so has cover
2df7630 to
39a55a7
Compare
Per gitlab the intent is to add a bunch of api so get the logic out of the form into somewhere more accessible. Following agreement on this I would change all calls to CRM_Dedupe_Merger::getMergeStats to call this api and get rid of the call to CRM_Core_BAO_PrevNextCache::retrieve from getMergeStats since that retrieve function really does differrent stuff & is being kinda pointlessly overloaded here.
39a55a7 to
e13fa54
Compare
|
@colemanw ok - getting this past syntax conformance was a challenge but I got there |
colemanw
reviewed
May 27, 2019
| // This is so bad. We actually have a camel case field name in the DB. Don't do that. | ||
| // Intercept the pain here. | ||
| $params['cacheKey'] = $params['cachekey']; | ||
| unset($params['cachekey']); |
There was a problem hiding this comment.
it's bad isn't it!
|
I wish this were a little bit thinner with more logic in a BAO function and less in an api3 function, because that would make it easier to port it to api4. But it's useful and has a test, so merging. |
|
@colemanw yeah hopefully we can move that way - but I think we have to go from the outside in here.... |
|
@colemanw what we could do is create a NEW BAO that extends the prevnext cache DAO but is called 'Dedupe' - that might help in that direction |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Add dedupe.getstatistics api action
Before
No action for this
After
Action exists
Technical Details
Per gitlab the intent is to add a bunch of api so get the logic out of the form into somewhere more accessible.
One goal is to get rid of calls to CRM_Core_BAO_PrevNextCache::retrieve which just takes awful parameters. In here I add Dedupe.get & Dedupe.getstatistics & use these from CRM_Dedupe_Merger::getMergeStats instead of CRM_Core_BAO_PrevNextCache::retrieve
Comments
I'm open to a different naming option - currently looking at
Dedupe.getstatistics- gets stats about the results of an attempted merge
Dedupe.getmatches - gets matches for a criteria set, optional param as to whether to reload the cache is empty (possibly should also offer empty+ reload option)
Discussed with @colemanw on gitlab & this is implemented according to concept from him https://chat.civicrm.org/civicrm/pl/pqtuciyswtrcmkxedz1aarabcr
@JKingsnorth