dev/core#369 Prevent hard fail of API Job when SMS provider has been deleted #14266
Conversation
|
(Standard links)
|
seamuslee001
force-pushed
the
dev_core_369
branch
2 times, most recently
from
fb80730 to
67639dd
Compare
seamuslee001
changed the title
…on schedule testing to include SMSes dev/core#369 Fix hard failure caused by setting of NULL in database for sms_provider_id when provider is deleted Minor improvment to test ensure that is_error is actually 0
seamuslee001
force-pushed
the
dev_core_369
branch
from
67639dd to
51c566a
Compare
| // dev/core#369 If an SMS provider is deleted then the relevant row in the action_schedule_table is set to NULL | ||
| // So we need to exclude them. | ||
| if (CRM_Utils_System::isNull($schedule->sms_provider_id)) { | ||
| return ["sms_provider_missing" => "SMS Provider is NULL in database cannot send reminder"]; |
There was a problem hiding this comment.
Is this message sent to a log or the end user? It should read: "SMS reminder cannot be sent because the SMS provider has been deleted."
There was a problem hiding this comment.
@seamuslee001 my instinct is we should thow an exception here & the calling function should catch & handle it
There was a problem hiding this comment.
@andrew-cormick-dockery its going into the civicrm_aciton_log wherever the action schedule logs are @eileenmcnaughton possibly but that isn't consistent with how this is written. note that in L546 we pass back an array as well if we can't find a legit phone number
There was a problem hiding this comment.
@seamuslee001 well I'm not thrilled with that idea - but that's not a complete abort as the missing provider is is it?
There was a problem hiding this comment.
@eileenmcnaughton well it aborts for that reminder, like in this case here we abort because we cannot find the relevant SMS provider, In that case it aborts because it cannot find a relevant phone number to send it to.
There was a problem hiding this comment.
OK - I'm not liking it but I guess I can ignore it
There was a problem hiding this comment.
The calling function here https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/BAO/ActionSchedule.php#L307 is expecting an array yo be returned for reasons and the only things in the array are any errors that have occurred.
| * | ||
| * @throws \CiviCRM_API3_Exception | ||
| */ | ||
| public function setupForSmsTests($teardown = FALSE) { |
|
I'm merging this - I have some code quality reservations - eg. I feel like the helper function should be a trait & I'm grumpy about not throwing an exception. But I also think resolving those are clearly in the 'nice to have' category & esp in the second are a scope creep This does a sensible fix and extends our unit test coverage so I think it's good to merge as is. |
Overview
Currently if an SMS provider is deleted from the database the relevant row is set to NULL in the action_schedule table and then when the reminder is processed it can cause a hard fail because it gets caught in a ::fatal, This resolves it by doing an early exist and aiding in logging correct details in the reminder log
Before
Hard Fail and no tests on sending SMS reminders
After
No hard fail and tests on sending reminder smses
ping @eileenmcnaughton @mattwire