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.

Sign up for GitHub

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

CRM-18515 - Allow Payment Processor with same name when using multi-d… #8344

Merged
merged 2 commits into from
Oct 10, 2016
Merged

CRM-18515 - Allow Payment Processor with same name when using multi-d… #8344

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
338115d
CRM-18515 - Allow Payment Processor with same name when using multi-d…
darrick May 10, 2016
35b6310
Update to use existing objectExists rule.
darrick Jul 3, 2016
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
33 changes: 32 additions & 1 deletion CRM/Admin/Form/PaymentProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,8 @@ public function buildQuickForm($check = FALSE) {
$attributes['name'], TRUE
);

$this->addRule('name', ts('Name already exists in Database.'), 'objectExists', array(
$this->registerRule('paymentProcessorNameExists', 'callback', 'paymentProcessorNameExists', 'CRM_Admin_Form_PaymentProcessor');
$this->addRule('name', ts('Name already exists in Database.'), 'paymentProcessorNameExists', array(
'CRM_Financial_DAO_PaymentProcessor',
$this->_id,
));
Expand Down Expand Up @@ -400,4 +401,34 @@ public function updatePaymentProcessor(&$values, $domainID, $test) {
civicrm_api3('PaymentProcessor', 'create', $params);
}

/**
* Check if there is a record with the same name in the db.
*
* @param string $value
* The value of the field we are checking.
* @param array $options
* The daoName and fieldName (optional ).
*
* @return bool
* true if object exists
*/
public static function paymentProcessorNameExists($value, $options) {
$fieldName = 'name';
$daoName = CRM_Utils_Array::value(0, $options);
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton Jun 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I think the patch makes sense but stylistically

  1. it seems odd to pass in $daoName when there is no reason for it to be anything other than 'CRM_Financial_DAO_PaymentProcessor'
  2. 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;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I just copied and pasted the objectExists rule and added the domain_id to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah there is lots of bad code in the code base :-)

Copy link
Member

@omarabuhussein omarabuhussein Jun 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 ? ^

Copy link
Member

Choose a reason for hiding this comment

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

also i believe unit tests should written for this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Altering nameExists makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

@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

$daoID = CRM_Utils_Array::value(1, $options);
$domain_id = CRM_Core_Config::domainID();
$object = new $daoName();
$object->$fieldName = $value;
$object->domain_id = $domain_id;

$config = CRM_Core_Config::singleton();

if ($object->find(TRUE)) {
return ($daoID && $object->id == $daoID) ? TRUE : FALSE;
}
else {
return TRUE;
}
}

}