The SMS sending system relies solely on smsalertnumber, but some notices get queued via C4::Message->enqueue which has a fallback mechanism to populate the to_address field in the message queue. If you then write a report to show all delivery failures, the to_address can be populated with an entirely unrelated number and is very misleading.
Created attachment 135298 [details] [review] Bug 30838: Don't fallback through phone numbers for sms This patch removes the fallback handling for smsalartnumber as the to_address in notices. We ignore the to_address field in the message queue at send time for sms anyway and use smsalertnumber exclusively so having this field populated is just confusing to the end user
Test plan, 1) Create a report to list all failed notices SELECT borrowernumber, letter_code, message_transport_type, status, failure_code, time_queued, updated_on, to_address FROM message_queue WHERE date(time_queued) BETWEEN <<Sent BETWEEN (yyyy-mm-dd)|date>> AND <<and (yyyy-mm-dd)|date>> ORDER BY time_queued DESC 2) Add an SMS template for the CHECKIN notice 3) Set a patron to use SMS as their preferred notice transport 4) Ensure the patron does not have smsalertnumber set (but does have another phone number field populated) 5) Trigger the notice 6) Send notices using process_message_queue.pl 7) Confirm you get a failure and that the to_address is populated with a number that's not in the smsalertnumber 8) Apply the patch 9) Repeat and confirm the to_address is no longer populated 10) Confirm that _send_message_by_sms only ever refers to smsalertnumber
Created attachment 135299 [details] [review] Bug 30838: Set to_address to smsalertnumber at send It may be helpful to know exactly what number was used for the sms alert that was sent. As such, we should ensure it's set at the time of sending.
Test plan for followup patch.. As above, but now confirm that smsalertnumber should appear in the to_address for the report after running process_messagequeue.pl
Created attachment 136674 [details] [review] Bug 30838: Don't fallback through phone numbers for sms This patch removes the fallback handling for smsalartnumber as the to_address in notices. We ignore the to_address field in the message queue at send time for sms anyway and use smsalertnumber exclusively so having this field populated is just confusing to the end user Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 136675 [details] [review] Bug 30838: Set to_address to smsalertnumber at send It may be helpful to know exactly what number was used for the sms alert that was sent. As such, we should ensure it's set at the time of sending. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 136676 [details] [review] Bug 30838: (QA Follow-up) Add missing semicolon Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Can you add a test for this change, please?
The more I read this code, the more funky I find it.. I feel like we could actually get rid of C4::Message entirely.. it's only used in one place and that use feels a bit strange already. Anyways.. I'm working on a unit test here for now.
Created attachment 137202 [details] [review] Bug 30838: (QA follow-up) Add unit tests Two tests added, one in t/db_dependent/Circulation.t to catch the initial setting of to_address at enqueue time and a second in t/db_dependent/Letters.t to catch the correcting at send time.
Created attachment 138025 [details] [review] Bug 30838: Don't fallback through phone numbers for sms This patch removes the fallback handling for smsalartnumber as the to_address in notices. We ignore the to_address field in the message queue at send time for sms anyway and use smsalertnumber exclusively so having this field populated is just confusing to the end user Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 138026 [details] [review] Bug 30838: Set to_address to smsalertnumber at send It may be helpful to know exactly what number was used for the sms alert that was sent. As such, we should ensure it's set at the time of sending. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 138027 [details] [review] Bug 30838: (QA Follow-up) Add missing semicolon Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 138028 [details] [review] Bug 30838: (QA follow-up) Add unit tests Two tests added, one in t/db_dependent/Circulation.t to catch the initial setting of to_address at enqueue time and a second in t/db_dependent/Letters.t to catch the correcting at send time. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
I think using the smsalertnumber is the correct behaviour, but I am a little worried about someone relying on the fallback. I think because of the behaviour change this might not be for backporting and needs a note?
Created attachment 138052 [details] [review] Bug 30838: (QA follow-up) Add missing semicolon Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 22.11. Nice work everyone, thanks!
Backported to 22.05.x for 22.05.05
Thanks! Pushed to 21.11 for 21.11.12
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed. Nothing to document it seems, marking resolved.