Skip to content

Commit

Permalink
dev/core#560 Replace instances of CRM_Core_Error::fatal with Exceptio…
Browse files Browse the repository at this point in the history
…ns or Status bounces in SMS processing and appropriately handle error after
  • Loading branch information
seamuslee001 committed Nov 23, 2019
1 parent ecf32c5 commit 67cb76e
Show file tree
Hide file tree
Showing 9 changed files with 49 additions and 39 deletions.
36 changes: 15 additions & 21 deletions CRM/Activity/BAO/Activity.php
Original file line number Diff line number Diff line change
Expand Up @@ -1336,21 +1336,19 @@ public static function sendSMS(
$errMsgs[] = PEAR::raiseError('Contact Does not accept SMS', NULL, PEAR_ERROR_RETURN);
}
else {
$sendResult = self::sendSMSMessage(
$contactId,
$tokenText,
$smsProviderParams,
$activityID,
$sourceContactId
);

if (PEAR::isError($sendResult)) {
// Collect all of the PEAR_Error objects
$errMsgs[] = $sendResult;
}
else {
try {
$sendResult = self::sendSMSMessage(
$contactId,
$tokenText,
$smsProviderParams,
$activityID,
$sourceContactId
);
$success++;
}
catch (CRM_Core_Exception $e) {
$errMsgs[] = $e->getMessage();
}
}
}

Expand Down Expand Up @@ -1381,8 +1379,8 @@ public static function sendSMS(
* The activity ID that tracks the message.
* @param int $sourceContactID
*
* @return bool|PEAR_Error
* true on success or PEAR_Error object
* @return bool true on success
* @throws CRM_Core_Exception
*/
public static function sendSMSMessage(
$toID,
Expand Down Expand Up @@ -1411,11 +1409,7 @@ public static function sendSMSMessage(
// make sure both phone are valid
// and that the recipient wants to receive sms
if (empty($toPhoneNumber)) {
return PEAR::raiseError(
'Recipient phone number is invalid or recipient does not want to receive SMS',
NULL,
PEAR_ERROR_RETURN
);
throw new CRM_Core_Exception('Recipient phone number is invalid or recipient does not want to receive SMS');
}

$recipient = $toPhoneNumber;
Expand All @@ -1425,7 +1419,7 @@ public static function sendSMSMessage(
$providerObj = CRM_SMS_Provider::singleton(['provider_id' => $smsProviderParams['provider_id']]);
$sendResult = $providerObj->send($recipient, $smsProviderParams, $tokenText, NULL, $sourceContactID);
if (PEAR::isError($sendResult)) {
return $sendResult;
throw new CRM_Core_Exception($sendResult->getMessgae());
}

// add activity target record for every sms that is sent
Expand Down
17 changes: 11 additions & 6 deletions CRM/Core/BAO/ActionSchedule.php
Original file line number Diff line number Diff line change
Expand Up @@ -558,12 +558,17 @@ protected static function sendReminderSms($tokenRow, $schedule, $toContactID) {

$activity = CRM_Activity_BAO_Activity::create($activityParams);

CRM_Activity_BAO_Activity::sendSMSMessage($tokenRow->context['contactId'],
$sms_body_text,
$smsParams,
$activity->id,
$userID
);
try {
CRM_Activity_BAO_Activity::sendSMSMessage($tokenRow->context['contactId'],
$sms_body_text,
$smsParams,
$activity->id,
$userID
);
}
catch (CRM_Core_Exception $e) {
return ["sms_send_error" => $e->getMessage()];
}

return [];
}
Expand Down
4 changes: 2 additions & 2 deletions CRM/SMS/BAO/Provider.php
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,11 @@ public static function setIsActive($id, $is_active) {
* @param int $providerID
*
* @return null
* @throws Exception
* @throws CRM_Core_Exception
*/
public static function del($providerID) {
if (!$providerID) {
CRM_Core_Error::fatal(ts('Invalid value passed to delete function.'));
throw new CRM_Core_Exception(ts('Invalid value passed to delete function.'));
}

$dao = new CRM_SMS_DAO_Provider();
Expand Down
2 changes: 1 addition & 1 deletion CRM/SMS/Form/Group.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class CRM_SMS_Form_Group extends CRM_Contact_Form_Task {
*/
public function preProcess() {
if (!CRM_SMS_BAO_Provider::activeProviderCount()) {
CRM_Core_Error::fatal(ts('The <a href="%1">SMS Provider</a> has not been configured or is not active.', [1 => CRM_Utils_System::url('civicrm/admin/sms/provider', 'reset=1')]));
CRM_Core_Error::statusBounce(ts('The <a href="%1">SMS Provider</a> has not been configured or is not active.', [1 => CRM_Utils_System::url('civicrm/admin/sms/provider', 'reset=1')]));
}

$session = CRM_Core_Session::singleton();
Expand Down
8 changes: 6 additions & 2 deletions CRM/SMS/Form/Provider.php
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,12 @@ public function setDefaultValues() {
$name = CRM_Utils_Request::retrieve('key', 'String', $this, FALSE, NULL);
if ($name) {
$defaults['name'] = $name;
$provider = CRM_SMS_Provider::singleton(['provider' => $name]);
$defaults['api_url'] = $provider->_apiURL;
try {
$provider = CRM_SMS_Provider::singleton(['provider' => $name]);
$defaults['api_url'] = $provider->_apiURL;
}
catch (CRM_Core_Exception $e) {
}
}

if (!$this->_id) {
Expand Down
2 changes: 1 addition & 1 deletion CRM/SMS/Form/Schedule.php
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ public function postProcess() {
$params['mailing_id'] = $ids['mailing_id'] = $this->_mailingID;

if (empty($params['mailing_id'])) {
CRM_Core_Error::fatal(ts('Could not find a mailing id'));
CRM_Core_Error::statusBounce(ts('Could not find a mailing id'));
}

$params['send_option'] = $this->controller->exportValue($this->_name, 'send_option');
Expand Down
8 changes: 7 additions & 1 deletion CRM/SMS/Page/Callback.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,13 @@
class CRM_SMS_Page_Callback {

public function run() {
$provider = CRM_SMS_Provider::singleton($_REQUEST);
try {
$provider = CRM_SMS_Provider::singleton($_REQUEST);
}
catch (CRM_Core_exception $e) {
Civi::log()->error('CiviCRM SMS Page Callback error ' . $e->getMessage());
return;
}

if (array_key_exists('status', $_REQUEST)) {
$provider->callback();
Expand Down
5 changes: 3 additions & 2 deletions CRM/SMS/Provider.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ abstract class CRM_SMS_Provider {
* @param bool $force
*
* @return object
* @throws CRM_Core_Exception
*/
public static function &singleton($providerParams = array(), $force = FALSE) {
$mailingID = CRM_Utils_Array::value('mailing_id', $providerParams);
Expand All @@ -47,7 +48,7 @@ public static function &singleton($providerParams = array(), $force = FALSE) {
}

if (!$providerName) {
CRM_Core_Error::fatal('Provider not known or not provided.');
throw new CRM_Core_Exception('Provider not known or not provided.');
}

$providerName = CRM_Utils_Type::escape($providerName, 'String');
Expand All @@ -62,7 +63,7 @@ public static function &singleton($providerParams = array(), $force = FALSE) {
else {
// If we are running unit tests we simulate an SMS provider with the name "CiviTestSMSProvider"
if ($providerName !== 'CiviTestSMSProvider') {
CRM_Core_Error::fatal("Could not locate extension for {$providerName}.");
throw new CRM_Core_Exception("Could not locate extension for {$providerName}.");
}
$providerClass = 'CiviTestSMSProvider';
}
Expand Down
6 changes: 3 additions & 3 deletions tests/phpunit/CRM/Activity/BAO/ActivityTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -1178,7 +1178,7 @@ public function testSendSmsNoPhoneNumber() {
$this->assertEquals($activity['status_id'], $activityStatusCompleted, 'Expected activity status Completed.');
$this->assertEquals($activity['subject'], 'createSendSmsTest subject', 'Activity subject does not match.');
$this->assertEquals($activity['details'], $details, 'Activity details does not match.');
$this->assertEquals("Recipient phone number is invalid or recipient does not want to receive SMS", $sent[0]->message, "Expected error doesn't match");
$this->assertEquals("Recipient phone number is invalid or recipient does not want to receive SMS", $sent[0], "Expected error doesn't match");
$this->assertEquals(0, $success, "Expected success to be 0");
}

Expand All @@ -1193,7 +1193,7 @@ public function testSendSmsFixedPhoneNumber() {
$this->assertEquals($activity['status_id'], $activityStatusCompleted, 'Expected activity status Completed.');
$this->assertEquals($activity['subject'], 'createSendSmsTest subject', 'Activity subject does not match.');
$this->assertEquals($activity['details'], $details, 'Activity details does not match.');
$this->assertEquals("Recipient phone number is invalid or recipient does not want to receive SMS", $sent[0]->message, "Expected error doesn't match");
$this->assertEquals("Recipient phone number is invalid or recipient does not want to receive SMS", $sent[0], "Expected error doesn't match");
$this->assertEquals(0, $success, "Expected success to be 0");
}

Expand Down Expand Up @@ -1229,7 +1229,7 @@ public function testSendSMSMobileInToProviderParam() {
public function testSendSMSMobileInToProviderParamWithDoNotSMS() {
list($sent, $activityId, $success) = $this->createSendSmsTest(2, TRUE, ['do_not_sms' => 1]);
foreach ($sent as $error) {
$this->assertEquals('Contact Does not accept SMS', $error->getMessage());
$this->assertEquals('Contact Does not accept SMS', $error);
}
$this->assertEquals(1, count($sent), "Expected sent should a PEAR Error");
$this->assertEquals(0, $success, "Expected success to be 0");
Expand Down

0 comments on commit 67cb76e

Please sign in to comment.