CRM-19961 Make civicrm_sms_provider able to be domain specific #9796
Conversation
seamuslee001
commented
Feb 6, 2017
•
seamuslee001
commented
- CRM-19961: Make CiviCRM SMS Providers multisite aware
|
@eileenmcnaughton does this look good to you? |
|
Just tagging in @MrLavaLava to be a watcher on this PR |
|
I ran my eye over it & it looks good - I haven't done any in depth QA |
|
pinging @nganivet I think you maybe interested in this as this assists with the multisite functionality in CiviCRM |
|
thanks @seamuslee001. Have not looked at the patch but can you make sure that NULL in the column translates to 'valid for all domain'? I have enforced this on all my multi-site improvements to ensure compatibility with previous data (ie. if someone previously had defined an SMS provider and upgrades CiviCRM we want this SMS provider to be valid for all domain to keep the same behavior). |
|
thanks @nganivet I have altered the function on the retrieve to handle that, I have also ensured that if domain_id is not set when an SMS provider is updated that the current domain id is set i think that is pretty normal |
CRM/SMS/BAO/Provider.php
Outdated
| @@ -93,7 +93,9 @@ public static function getProviders($selectArr = NULL, $filter = NULL, $getActiv | |||
| public static function saveRecord($values) { | |||
| $dao = new CRM_SMS_DAO_Provider(); | |||
| $dao->copyValues($values); | |||
| $dao->domain_id = CRM_Core_Config::domainID(); | |||
| if (!empty($dao->domain_id)) { | |||
There was a problem hiding this comment.
Is this IF right? It's confusing me
There was a problem hiding this comment.
- oh it's a domain-theft pattern? I edit something on another domain & I steal the record? That seems like maybe a form-layer thing to me
seamuslee001
force-pushed
the
CRM-19961
branch
from
7dd6776
to
3f5d6ab
Compare
CRM/SMS/BAO/Provider.php
Outdated
| @@ -92,6 +93,9 @@ public static function getProviders($selectArr = NULL, $filter = NULL, $getActiv | |||
| public static function saveRecord($values) { | |||
| $dao = new CRM_SMS_DAO_Provider(); | |||
| $dao->copyValues($values); | |||
| if (empty($dao->domain_id)) { | |||
| $dao->domain_id = CRM_Core_Config::domainID(); | |||
There was a problem hiding this comment.
Is this correct - I thought 'NULL' was 'valid'
There was a problem hiding this comment.
Yes but here your saving a record into the db so i guess the question is should we update NULL values = the current domain or leave them as they are?
There was a problem hiding this comment.
Have now updated using the style found here
seamuslee001
force-pushed
the
CRM-19961
branch
2 times, most recently
from
cccf9ae
to
55ac47c
Compare
Add in domain id filtering to delete function CRM-19961 Treat NULL domain_id values as ok when retriving SMS providers Ensure domain id only gets set if its not already set Treat NULL domain_id as valid and also allow for any domain to delete providers that have domain_id which is NULL Add in API default for domain_id for sms provider Use value from database if provided even if null otherwise fall back to current domain as default but use the value submitted first Fix null test on update
CRM/SMS/BAO/Provider.php
Outdated
| if (is_null($current_domain_id)) { | ||
| $current_domain_id = NULL; | ||
| } | ||
| elseif (!isset($current_domain_id)) { |
There was a problem hiding this comment.
this seems impossible
|
I've added merge on pass, but have also discussed with @seamuslee001 that once this is merged he is going to work on changing it to use the api & associated clean up |
| @@ -131,6 +137,7 @@ public static function del($providerID) { | |||
|
|
|||
| $dao = new CRM_SMS_DAO_Provider(); | |||
| $dao->id = $providerID; | |||
| $dao->whereAdd = "(domain_id = " . CRM_Core_Config::domainID() . "OR domain_id IS NULL)"; | |||
There was a problem hiding this comment.
we don't have a precedent for this - @seamuslee001 says it provides a safety net & I think it's ok
There was a problem hiding this comment.
Is there a reason we can't we use the new hook_civicrm_selectWhereClause hook to implement this?
There was a problem hiding this comment.
I don't believe the hook is invoked here
There was a problem hiding this comment.
this is the delete function - we can remove this line again if it's problematic since it's precautionary. Not sure we could move it to the extension
| * @param $values | ||
| */ | ||
| public static function saveRecord($values) { | ||
| $values['domain_id'] = CRM_Utils_Array::value('domain_id', $values, CRM_Core_Config::domainID()); |
There was a problem hiding this comment.
we discussed that with the api the default can be set & overridden at the api level rather than here
There was a problem hiding this comment.
Would it be possible to set this in the civicrm_pre hook implementation within the org.civicrm.multisite extension? This would go towards the goal of moving all multisite logic out of core.
Also Eileen suggested we might use an entity_domain table rather than adding domain_id columns in all entity tables.
There was a problem hiding this comment.
Yes, I plan to have a new function to sort some of this out
There was a problem hiding this comment.
@seamuslee001 that's a good point - @nganivet is suggestion putting the default in the extension rather than the api - so the api would go back to no default when you do that part.
There was a problem hiding this comment.
@nganivet this is my new PR to clean up this BAO a bit and use standard functions https://github.com/civicrm/civicrm-core/pull/10135/files i have also put in Pre n post hooks as they weren't there
remove changes to update function Fix style
|
I've merged per comments -leaving entity_domain table out of scope for this round |
