CRM-18515 - Allow Payment Processor with same name when using multi-d… #8344
Conversation
|
Can one of the admins verify this patch? |
|
ok to test |
|
@PalanteJon what is the status here? I recall seeing you saying this had been recently fixed - is this the fix you mentioned, or a duplicate, or? |
|
This was the fix I was thinking of - I hadn't realized it hadn't merged yet! My apologies if that was misleading. |
|
Have you qa'd it @PalanteJon ? It's not in a group this month but you could still do it |
|
I haven't - but I'm quite behind on my work. I opted not to do QA this |
|
Heh @darrick as Release Manager this month, I'm trying to recruit people to help pare down the backlog of almost 100 PRs, some going back to last summer. I'm wondering if you would be able to help QA another PR if I got someone to QA this PR? |
|
@JoeMurray Not sure what all is involved in that, but I'll give it my best shot. |
|
test this please |
| */ | ||
| public static function paymentProcessorNameExists($value, $options) { | ||
| $fieldName = 'name'; | ||
| $daoName = CRM_Utils_Array::value(0, $options); |
There was a problem hiding this comment.
I think the patch makes sense but stylistically
- it seems odd to pass in $daoName when there is no reason for it to be anything other than 'CRM_Financial_DAO_PaymentProcessor'
- there is no reason to instantiate $config - that happens in lots of places randomly but for no reason that is still valid.
I think you could simplify the whole thing to
return civicrm_api3('PaymentProcessor', 'getcount' array(
'id' => array('<>' => $id),
'domain_id' => CRM_Core_Config::domainID(),
'name' => $value,
));
(or is that the opposite ie.)
$count = civicrm_api3('PaymentProcessor', 'getcount' array(
'id' => array('<>' => $id),
'domain_id' => CRM_Core_Config::domainID(),
'name' => $value,
));
return !$count;
There was a problem hiding this comment.
Okay. I just copied and pasted the objectExists rule and added the domain_id to it.
There was a problem hiding this comment.
yeah there is lots of bad code in the code base :-)
There was a problem hiding this comment.
While I agree to what you said above , I am just no convinced that using the API way is better ( even if it look simpler ) .... The API is meant to be used for extensions and other systems that want to interact with civicrm and not to be used inside the core system files .. also I think its kinda slower than using DAO object directly... and also when API v3 in this case get deperacted ( and later on removed ) then you have to update the core everywhere the API get used.
I know the API is already used in a lot of places in the core files ... but I believe it just wrong.
There was a problem hiding this comment.
I agree with that. Also regarding passing in $daoName. I believe my idea was to keep open the possibility of a generic NameExistsForDomainID rule which could possibly be used by other classes.
Another idea is to modify the current objectExists rule to optionally filter by domain id if it's passed in as an option.
There was a problem hiding this comment.
How does that compare to just having a name-exists function? I mean it should be appropriately domain sensitive but the calling code maybe doesn't need to know that
Yes I agree . so to sum up what changes are needed for now :
1- change the method name to nameExists.
2- move the method to CRM_Utils_Rule.
3- use the API instead of DAO inside the method.
4- using the API will require you to pass the entity name instead of DAO name .
@eileenmcnaughton does all of that make sense ? ^
There was a problem hiding this comment.
also i believe unit tests should written for this PR
There was a problem hiding this comment.
I kept this rule local, although with the idea it could maybe be used by something, just to fix the issue at hand. The nameExists rule already exists and is used quite a bit. I'm not sure if limiting it by domain by default would effect other uses of it. As domain id is not included in every entity. Personally I like the idea of passing in the domain id for now and modifying the current nameExists rule to handle that.
As for use of the API by core. I think someone needs to decide the rules about that. I'd imagine you may want to look ahead at a sort headless civicrm. I.e. only have UI code use the api not Core. Granted this is UI code so maybe it works. But absent guidelines rather then try to start that here I think it's best to stick with how all the other rules are written and follow that format. Then when you come to a decision someone can do a PR which modifies all the rules to use the API.
There was a problem hiding this comment.
Altering nameExists makes sense.
There was a problem hiding this comment.
@darrick
An interesting discussing happened between my colleague ( davi ) and (Tim otten) on dev channel regarding using API in core ... it may worth to check it out :
https://chat.civicrm.org/civicrm/pl/8tisabi96pysid8j7ugo6xnrxa
|
Re deprecation - I think there is at least as much chance of the DAO being deprecated before apiv3 as the reverse so it's probably not a good consideration |
|
@PalanteJon I did a quick review of the code and it looks fine. Can you give it a quick test and we'll get it merged? |
|
I have just reviewed this PR and it looks good. I have tested it locally and it seems to work. |
|
@jaapjansma - done! |
https://issues.civicrm.org/jira/browse/CRM-18515