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

Jump to bottom

CRM-19961 Make civicrm_sms_provider able to be domain specific #9796

Merged
merged 2 commits into from
Apr 11, 2017
Merged

CRM-19961 Make civicrm_sms_provider able to be domain specific #9796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eca6c81
Add in Domain id handling into SMS provider BAO
seamuslee001 Feb 3, 2017
c1ab6fe
CRM-19961 Add in tests to prove logic and fix logic
seamuslee001 Apr 10, 2017
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
2 changes: 1 addition & 1 deletion CRM/Core/DAO/AllCoreTables.data.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
| see the CiviCRM license FAQ at http://civicrm.org/licensing |
+--------------------------------------------------------------------+
*/
// (GenCodeChecksum:fe1a575dafdc824e3ce8a7e0d7fc49bf)
// (GenCodeChecksum:53f1cd5e913b2f82abfc7097127caafc)
return array(
'CRM_Core_DAO_AddressFormat' => array(
'name' => 'AddressFormat',
Expand Down
7 changes: 7 additions & 0 deletions CRM/SMS/BAO/Provider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ public static function getProviders($selectArr = NULL, $filter = NULL, $getActiv
$select = implode(',', $selectArr);
$dao->selectAdd($select);
}
$dao->whereAdd("(domain_id = " . CRM_Core_Config::domainID() . " OR domain_id IS NULL)");
$dao->orderBy($orderBy);
$dao->find();
while ($dao->fetch()) {
Expand All @@ -87,15 +88,20 @@ public static function getProviders($selectArr = NULL, $filter = NULL, $getActiv
}

/**
* Save a new record into the database
* @todo create a create function to do this work
* @param $values
*/
public static function saveRecord($values) {
$values['domain_id'] = CRM_Utils_Array::value('domain_id', $values, CRM_Core_Config::domainID());
Copy link
Contributor

Choose a reason for hiding this comment

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

we discussed that with the api the default can be set & overridden at the api level rather than here

Copy link
Contributor

@nganivet nganivet Apr 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I plan to have a new function to sort some of this out

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

$dao = new CRM_SMS_DAO_Provider();
$dao->copyValues($values);
$dao->save();
}

/**
* Update an SMS provider in the database.
* @todo combine with saveRecord in a create function
* @param $values
* @param int $providerId
*/
Expand Down Expand 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)";
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't have a precedent for this - @seamuslee001 says it provides a safety net & I think it's ok

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we can't we use the new hook_civicrm_selectWhereClause hook to implement this?

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 don't believe the hook is invoked here

Copy link
Contributor

Choose a reason for hiding this comment

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

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

if (!$dao->find(TRUE)) {
return NULL;
}
Expand Down
12 changes: 5 additions & 7 deletions CRM/Upgrade/Incremental/php/FourSeven.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,6 @@ public function setPreUpgradeMessage(&$preUpgradeMessage, $rev, $currentVer = NU
if ($rev == '4.7.13') {
$preUpgradeMessage .= '<p>' . ts('A new permission has been added called %1 This Permission is now used to control access to the Manage Tags screen', array(1 => 'manage tags')) . '</p>';
}
if ($rev == '4.7.19') {
$check = CRM_Core_DAO::singleValueQuery("SELECT count(id) FROM civicrm_domain");
$smsCheck = CRM_Core_DAO::singleValueQuery("SELECT count(id) FROM civicrm_sms_provider");
if ($check > 1 && (bool) $smsCheck) {
$postUpgradeMessage .= '<p>civicrm_sms_provider ' . ts('has now had a domain id column added. As there is more than 1 domains in this install you need to manually set the domain id for the providers in this install') . '</p>';
}
}
}

/**
Expand Down Expand Up @@ -123,6 +116,11 @@ public function setPostUpgradeMessage(&$postUpgradeMessage, $rev) {
}
if ($rev == '4.7.19') {
$postUpgradeMessage .= '<br /><br />' . ts('Default version of the following System Workflow Message Templates have been modified: <ul><li>Additional Payment Receipt or Refund Notification</li></ul> If you have modified these templates, please review the new default versions and implement updates as needed to your copies (Administer > Communications > Message Templates > System Workflow Messages).');
$check = CRM_Core_DAO::singleValueQuery("SELECT count(id) FROM civicrm_domain");
$smsCheck = CRM_Core_DAO::singleValueQuery("SELECT count(id) FROM civicrm_sms_provider");
if ($check > 1 && (bool) $smsCheck) {
$postUpgradeMessage .= '<p>civicrm_sms_provider ' . ts('has now had a domain id column added. As there is more than 1 domains in this install you need to manually set the domain id for the providers in this install') . '</p>';
}
}
}

Expand Down
12 changes: 12 additions & 0 deletions api/v3/SmsProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,18 @@ function civicrm_api3_sms_provider_create($params) {
return _civicrm_api3_basic_create(_civicrm_api3_get_BAO(__FUNCTION__), $params);
}

/**
* Adjust Metadata for Create action.
*
* The metadata is used for setting defaults, documentation & validation.
*
* @param array $params
* Array of parameters determined by getfields.
*/
function _civicrm_api3_sms_provider_create_spec(&$params) {
$params['domain_id']['api.default'] = CRM_Core_Config::domainID();
}

/**
* Get an sms_provider.
*
Expand Down
99 changes: 99 additions & 0 deletions tests/phpunit/CRM/SMS/BAO/ProviderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
<?php

/*
+--------------------------------------------------------------------+
| CiviCRM version 4.7 |
+--------------------------------------------------------------------+
| Copyright CiviCRM LLC (c) 2004-2017 |
+--------------------------------------------------------------------+
| This file is a part of CiviCRM. |
| |
| CiviCRM is free software; you can copy, modify, and distribute it |
| under the terms of the GNU Affero General Public License |
| Version 3, 19 November 2007 and the CiviCRM Licensing Exception. |
| |
| CiviCRM is distributed in the hope that it will be useful, but |
| WITHOUT ANY WARRANTY; without even the implied warranty of |
| MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. |
| See the GNU Affero General Public License for more details. |
| |
| You should have received a copy of the GNU Affero General Public |
| License and the CiviCRM Licensing Exception along |
| with this program; if not, contact CiviCRM LLC |
| at info[AT]civicrm[DOT]org. If you have questions about the |
| GNU Affero General Public License or the licensing of CiviCRM, |
| see the CiviCRM license FAQ at http://civicrm.org/licensing |
+--------------------------------------------------------------------+
*/
/**
* Test CRM_SMS_BAO_Provider functions
*
* @package CiviCRM_APIv3
* @subpackage API_Contribution
* @group headless
*/
class CRM_SMS_BAO_ProviderTest extends CiviUnitTestCase {

/**
* Set Up Funtion
*/
public function setUp() {
parent::setUp();
$option = $this->callAPISuccess('option_value', 'create', array('option_group_id' => 'sms_provider_name', 'name' => 'test_provider_name', 'label' => 'test_provider_name', 'value' => 1));
$this->option_value = $option['id'];
}

/**
* Clean up after each test.
*/
public function tearDown() {
parent::tearDown();
$this->callAPISuccess('option_value', 'delete', array('id' => $this->option_value));
}

/**
* CRM-19961 Check that when saving and updating a SMS provider with domain as NULL that it stays null
*/
public function testCreateAndUpdateProvider() {
$values = array(
'domain_id' => NULL,
'title' => 'test SMS provider',
'username' => 'test',
'password' => 'dummpy password',
'name' => 1,
'is_active' => 1,
'api_type' => 1,
);
CRM_SMS_BAO_Provider::saveRecord($values);
$provider = $this->callAPISuccess('SmsProvider', 'getsingle', array('title' => 'test SMS provider'));
$domain_id = CRM_Core_DAO::getFieldValue('CRM_SMS_DAO_Provider', $provider['id'], 'domain_id');
$this->assertNull($domain_id);
$values2 = array('title' => 'Test SMS Provider2');
CRM_SMS_BAO_Provider::updateRecord($values2, $provider['id']);
$provider = $this->callAPISuccess('SmsProvider', 'getsingle', array('id' => $provider['id']));
$this->assertEquals('Test SMS Provider2', $provider['title']);
$domain_id = CRM_Core_DAO::getFieldValue('CRM_SMS_DAO_Provider', $provider['id'], 'domain_id');
$this->assertNull($domain_id);
CRM_SMS_BAO_Provider::del($provider['id']);
}

/**
* CRM-19961 Check that when a domain is not passed when saving it defaults to current domain when create
*/
public function testCreateWithoutDomain() {
$values = array(
'title' => 'test SMS provider',
'username' => 'test',
'password' => 'dummpy password',
'name' => 1,
'is_active' => 1,
'api_type' => 1,
);
CRM_SMS_BAO_Provider::saveRecord($values);
$provider = $this->callAPISuccess('SmsProvider', 'getsingle', array('title' => 'test SMS provider'));
$domain_id = CRM_Core_DAO::getFieldValue('CRM_SMS_DAO_Provider', $provider['id'], 'domain_id');
$this->assertEquals(CRM_Core_Config::domainID(), $domain_id);
CRM_SMS_BAO_Provider::del($provider['id']);
}

}