| Summary: | SMS::Send driver errors are not captured and stored | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Kyle M Hall (khall) <kyle> |
| Component: | Architecture, internals, and plumbing | Assignee: | Kyle M Hall (khall) <kyle> |
| Status: | CLOSED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
| Severity: | normal | ||
| Priority: | P5 - low | CC: | andrew, dcook, fridolin.somers, lucas, m.de.rooy |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| GIT URL: | Initiative type: | --- | |
| Sponsorship status: | --- | Crowdfunding goal: | 0 |
| Patch complexity: | Small patch | Documentation contact: | |
| Documentation submission: | Text to go in the release notes: | ||
| Version(s) released in: |
24.05.00,23.11.06
|
Circulation function: | |
| Attachments: |
Bug 36307 - SMS::Send driver errors are not captured and stored
Bug 36307 - SMS::Send driver errors are not captured and stored Bug 36307: SMS::Send driver errors are not captured and stored |
||
|
Description
Kyle M Hall (khall)
2024-03-13 17:16:29 UTC
Created attachment 163116 [details] [review] Bug 36307 - SMS::Send driver errors are not captured and stored If an SMS::Send driver succeeds, it returns a value that evaluates to true. Every driver I've inspected uses croak when it encounters a failure state. When an SMS message fails to send, code hard codes the failure code to NO_NOTES (No notes from SMS driver). We should store the real error in `failure_code` and display that if the failure code doesn't match a known failure code. Test Plan: 1) Apply this patch 2) Set SMSSendDriver to any value 3) Generate a pending sms message 4) Run the following query: update message_queue set status = 'failed', failure_code = "This is a test"; 5) View the patron's messages, note the delivery note contains the contents of the failure code 6) Run the following query: update message_queue set status = 'failed', failure_code = "SMS_SEND_DRIVER_MISSING"; 7) Reload the patron's messages, not the delivery note is now "The SMS driver could not be loaded". *** Bug 32381 has been marked as a duplicate of this bug. *** Just bumped into this yesterday so I understand better now. Created attachment 163156 [details] [review] Bug 36307 - SMS::Send driver errors are not captured and stored If an SMS::Send driver succeeds, it returns a value that evaluates to true. Every driver I've inspected uses croak when it encounters a failure state. When an SMS message fails to send, code hard codes the failure code to NO_NOTES (No notes from SMS driver). We should store the real error in `failure_code` and display that if the failure code doesn't match a known failure code. Test Plan: 1) Apply this patch 2) Set SMSSendDriver to any value 3) Generate a pending sms message 4) Run the following query: update message_queue set status = 'failed', failure_code = "This is a test"; 5) View the patron's messages, note the delivery note contains the contents of the failure code 6) Run the following query: update message_queue set status = 'failed', failure_code = "SMS_SEND_DRIVER_MISSING"; 7) Reload the patron's messages, not the delivery note is now "The SMS driver could not be loaded". Signed-off-by: David Nind <david@davidnind.com> Created attachment 164463 [details] [review] Bug 36307: SMS::Send driver errors are not captured and stored If an SMS::Send driver succeeds, it returns a value that evaluates to true. Every driver I've inspected uses croak when it encounters a failure state. When an SMS message fails to send, code hard codes the failure code to NO_NOTES (No notes from SMS driver). We should store the real error in `failure_code` and display that if the failure code doesn't match a known failure code. Test Plan: 1) Apply this patch 2) Set SMSSendDriver to any value 3) Generate a pending sms message 4) Run the following query: update message_queue set status = 'failed', failure_code = "This is a test"; 5) View the patron's messages, note the delivery note contains the contents of the failure code 6) Run the following query: update message_queue set status = 'failed', failure_code = "SMS_SEND_DRIVER_MISSING"; 7) Reload the patron's messages, not the delivery note is now "The SMS driver could not be loaded". Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> FIXED * Commit title does not start with 'Bug XXXXX: ' - eb3ef09d69
Following code could still be improved with error codes. (No blocker for me)
foreach my $required_parameter ( qw( message destination ) ) {
# Should I warn in some way?
return unless defined $params->{ $required_parameter };
}
eval { require SMS::Send; };
if ( $@ ) {
# we apparently don't have SMS::Send. Return a failure.
return;
}
Similarly (no blocker for me):
return $sent;
If $sent is 0, this could use a specific error code, I would say. Now we fallback to NO_NOTES.
SMS::Send does not tell us much more, I am afraid.
Provide a follow-up if you like.
Pushed for 24.05! Well done everyone, thank you! Pushed to 23.11.x for 23.11.06 Doesn't apply cleanly to 23.05.x, no backport. |