Bug 30838 - to_address is misleading for SMS transports
Summary: to_address is misleading for SMS transports
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Martin Renvoize
QA Contact: Katrin Fischer
URL:
Keywords: release-notes-needed
Depends on:
Blocks:
 
Reported: 2022-05-24 11:17 UTC by Martin Renvoize
Modified: 2023-06-08 22:26 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00,21.11, 22.05.05


Attachments
Bug 30838: Don't fallback through phone numbers for sms (1.03 KB, patch)
2022-05-24 11:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30838: Set to_address to smsalertnumber at send (965 bytes, patch)
2022-05-24 11:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30838: Don't fallback through phone numbers for sms (1.09 KB, patch)
2022-06-28 11:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30838: Set to_address to smsalertnumber at send (1023 bytes, patch)
2022-06-28 11:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30838: (QA Follow-up) Add missing semicolon (811 bytes, patch)
2022-06-28 11:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30838: (QA follow-up) Add unit tests (3.92 KB, patch)
2022-07-06 10:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30838: Don't fallback through phone numbers for sms (1.14 KB, patch)
2022-07-22 13:47 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30838: Set to_address to smsalertnumber at send (1.05 KB, patch)
2022-07-22 13:47 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30838: (QA Follow-up) Add missing semicolon (867 bytes, patch)
2022-07-22 13:47 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30838: (QA follow-up) Add unit tests (3.98 KB, patch)
2022-07-22 13:47 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30838: (QA follow-up) Add missing semicolon (924 bytes, patch)
2022-07-22 17:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2022-05-24 11:17:47 UTC
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.
Comment 1 Martin Renvoize 2022-05-24 11:34:30 UTC
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
Comment 2 Martin Renvoize 2022-05-24 11:52:51 UTC
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
Comment 3 Martin Renvoize 2022-05-24 11:58:16 UTC
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.
Comment 4 Martin Renvoize 2022-05-24 11:59:35 UTC
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
Comment 5 Kyle M Hall 2022-06-28 11:27:49 UTC
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>
Comment 6 Kyle M Hall 2022-06-28 11:28:02 UTC
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>
Comment 7 Kyle M Hall 2022-06-28 11:28:07 UTC
Created attachment 136676 [details] [review]
Bug 30838: (QA Follow-up) Add missing semicolon

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Jonathan Druart 2022-07-04 13:16:04 UTC
Can you add a test for this change, please?
Comment 9 Martin Renvoize 2022-07-05 15:42:28 UTC
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.
Comment 10 Martin Renvoize 2022-07-06 10:56:01 UTC
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.
Comment 11 Katrin Fischer 2022-07-22 13:47:08 UTC
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>
Comment 12 Katrin Fischer 2022-07-22 13:47:12 UTC
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>
Comment 13 Katrin Fischer 2022-07-22 13:47:16 UTC
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>
Comment 14 Katrin Fischer 2022-07-22 13:47:20 UTC
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>
Comment 15 Katrin Fischer 2022-07-22 13:48:25 UTC
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?
Comment 16 Tomás Cohen Arazi 2022-07-22 17:46:43 UTC
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>
Comment 17 Tomás Cohen Arazi 2022-07-22 17:52:14 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 18 Lucas Gass 2022-09-02 17:39:42 UTC
Backported to 22.05.x for 22.05.05
Comment 19 Arthur Suzuki 2022-09-14 12:58:45 UTC
Thanks!

Pushed to 21.11 for 21.11.12
Comment 20 Victor Grousset/tuxayo 2022-09-20 16:31:48 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document it seems, marking resolved.