Bug 30026 - International phone number not supported for sending SMS
Summary: International phone number not supported for sending SMS
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Florian
QA Contact: Testopia
URL:
Keywords: rel_22_11_candidate
Depends on:
Blocks:
 
Reported: 2022-02-04 15:35 UTC by Florian
Modified: 2022-07-12 15:48 UTC (History)
4 users (show)

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


Attachments
Bug 30026: SMSAlert regex fixed for international (2.21 KB, patch)
2022-02-04 16:04 UTC, Florian
Details | Diff | Splinter Review
Bug 30026: Quick fix for SmsAlert Hint (1.63 KB, patch)
2022-03-04 14:53 UTC, Florian
Details | Diff | Splinter Review
Bug 30026: SMSAlert regex fixed for international (2.21 KB, patch)
2022-05-06 18:43 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 30026: Quick fix for SmsAlert Hint (1.68 KB, patch)
2022-05-06 18:43 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 30026: SMSAlert regex fixed for international (2.26 KB, patch)
2022-05-13 08:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30026: Quick fix for SmsAlert Hint (1.73 KB, patch)
2022-05-13 08:22 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Florian 2022-02-04 15:35:45 UTC
The SMSNumber field used when registering a phone number for SMS alerts does not work with some international number formats.

Currently, the format 00 AAAXXXXXXXXX (A = country indicator between 1 and 3 digits, X = regular digits) is not supported. While the format +AAAXXXXXXXXX can sometimes work, some network providers don't accept the latter format.

These formats are very used in Europe for instance (Germany, Sweden, France, etc)
Comment 1 Florian 2022-02-04 16:04:50 UTC Comment hidden (obsolete)
Comment 2 David Cook 2022-02-07 01:28:53 UTC
That's interesting. 

So that 00 is an exit code which is very country specific, so I don't think this patch will work in all contexts. While I wouldn't call it Euro-centric per se, since 00 is used elsewhere, it doesn't cover all international use cases. 

For instance, the exit code for Australia is 0011 (instead of 00). Canada/USA is 011. Russia is 810. Some places like Israel also use carrier codes instead of exit codes. 

Perhaps we should have a system preference that allows a library to specify 1+ exit codes to use in lieu of a "+" for the regex.
Comment 3 David Cook 2022-02-07 01:31:52 UTC
Since the exit code is specific to the dialler rather than the recipient maybe the data input isn't the right place to check this either.

Maybe the SMSAlert plugin should check if the phone number has a "+" prefix and if not... then add an exit code?

Is the use case that the library would be SMSing mobiles outside their own national area?
Comment 4 Florian 2022-02-07 08:30:08 UTC
I did take a look at other countries' exit code to make sure none would be penalized by this regex, and from what I could find online, all of your examples would still work. The regex isn't exactly foolproof but it should be flexible enough to allow for all valid phones numbers.

I did consider the alternative of switching the SMSAlert input field from a text to a tel, since it seems to naturally check for valid phone numbers.

> That's interesting. 
> 
> So that 00 is an exit code which is very country specific, so I don't think
> this patch will work in all contexts. While I wouldn't call it Euro-centric
> per se, since 00 is used elsewhere, it doesn't cover all international use
> cases. 
> 
> For instance, the exit code for Australia is 0011 (instead of 00).
> Canada/USA is 011. Russia is 810. Some places like Israel also use carrier
> codes instead of exit codes. 
> 
> Perhaps we should have a system preference that allows a library to specify
> 1+ exit codes to use in lieu of a "+" for the regex.
Comment 5 Florian 2022-02-07 08:37:19 UTC
(In reply to David Cook from comment #3)
> Since the exit code is specific to the dialler rather than the recipient
> maybe the data input isn't the right place to check this either.
> 
> Maybe the SMSAlert plugin should check if the phone number has a "+" prefix
> and if not... then add an exit code?
> 
> Is the use case that the library would be SMSing mobiles outside their own
> national area?

It seems to me that forcing the addition of an exit code might cause a lot more problems down the line, since you would need to check if, for instance, a number starting with 011 is for the USA/Canada or just a local territorial number in your country.

We do have some libraries that are located next to country borders, and it's not uncommon for residents of these areas to use two phones or sim cards, one for each side of the border.
Comment 6 David Cook 2022-02-08 01:53:55 UTC
(In reply to Florian from comment #5)
> It seems to me that forcing the addition of an exit code might cause a lot
> more problems down the line

100% agreed. I think for many/most libraries, they'll never want to have exit codes included. 

I still don't see the utility of adding exit codes. It sounds like it's something you'd need for your plugin, but it seems to me that is something you could add via your plugin and doesn't need to be in Koha.
Comment 7 Florian 2022-02-08 08:17:18 UTC
(In reply to David Cook from comment #6)
> (In reply to Florian from comment #5)
> > It seems to me that forcing the addition of an exit code might cause a lot
> > more problems down the line
> 
> 100% agreed. I think for many/most libraries, they'll never want to have
> exit codes included. 
> 
> I still don't see the utility of adding exit codes. It sounds like it's
> something you'd need for your plugin, but it seems to me that is something
> you could add via your plugin and doesn't need to be in Koha.

But it's already there in the code. The current version already supports vast majority of exit codes, this patch aims to make sure we support even more.
Comment 8 David Cook 2022-02-08 23:35:26 UTC
(In reply to Florian from comment #7)
> But it's already there in the code. The current version already supports
> vast majority of exit codes, this patch aims to make sure we support even
> more.

I'm not sure if you're referring to Koha or your Koha plugin here. 

If it's Koha, no it doesn't?

Anyway, that's just my 2 cents. Testers and QA might think differently.
Comment 9 Katrin Fischer 2022-02-26 20:06:08 UTC
+49 or 0049 are equivalent here and both are accepted for example when adding contacts in my phone. I don't see the issue in making the regex for the SMS number accept more valid combinations.
Comment 10 Katrin Fischer 2022-02-26 20:08:04 UTC
I am putting this back in the queue to get more opinions.

I am ok with this change, but if we do it, we should adapt the hint below the field too. It currently reads:

Please enter numbers only. Prefix the number with + if including the country code.
Comment 11 Florian 2022-02-28 08:09:48 UTC
(In reply to Katrin Fischer from comment #10)
> I am putting this back in the queue to get more opinions.
> 
> I am ok with this change, but if we do it, we should adapt the hint below
> the field too. It currently reads:
> 
> Please enter numbers only. Prefix the number with + if including the country
> code.

Good catch!
Comment 12 Florian 2022-03-04 14:53:07 UTC Comment hidden (obsolete)
Comment 13 Owen Leonard 2022-05-06 18:43:51 UTC
Created attachment 134756 [details] [review]
Bug 30026: SMSAlert regex fixed for international

When registering a phone number for SMS alerts, the number goes through
a regex filter to make sure the number is valid. One of the formats used
in Europe is 00 AAA XXXXXXXXX (A = country indicator) and it is not
supported.

This patch adjusts the regex to either accept 00 AAA XXXXXXXXX or
+AAAXXXXXXXXX in this case. (Note : +AAA and 00AAA are mutually exclusive).

To test:
1) You need to have installed a SMSAlert plugin. Make sure the
   SMSSendDriver syspref is also set up with your SMS plugin of choice.
2) Pick a patron and edit its profile.
3) In the messaging preferences fieldset, under SMS number, enter any
   number with the format 00 AAA XXXXXXXXX (ie : 00111123456789).
   Try to save the modification : you should have an error message.
4) Try instead a number with the format +AAAXXXXXXXXX (ie :
   111123456789), that one should be accepted.
5) Save your modification then check that the number has been changed.
6) Apply patch.
7) Repeat step 3 to 5.
8) Observe the error is gone.
9) Try it once more with the format +00 AAA XXXXXXXXX (+00111123456789),
   that format should not be accepted: this is intentional.
10) Sign off.

    Thanks-to: Didier Gautheron <didier.gautheron@biblibre.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 14 Owen Leonard 2022-05-06 18:43:55 UTC
Created attachment 134757 [details] [review]
Bug 30026: Quick fix for SmsAlert Hint

Just modifying the hint provided in patrons' messaging preferences to
allow either + or 00 for international formats.
No test plan needed.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 15 Katrin Fischer 2022-05-13 08:22:18 UTC
Created attachment 134954 [details] [review]
Bug 30026: SMSAlert regex fixed for international

When registering a phone number for SMS alerts, the number goes through
a regex filter to make sure the number is valid. One of the formats used
in Europe is 00 AAA XXXXXXXXX (A = country indicator) and it is not
supported.

This patch adjusts the regex to either accept 00 AAA XXXXXXXXX or
+AAAXXXXXXXXX in this case. (Note : +AAA and 00AAA are mutually exclusive).

To test:
1) You need to have installed a SMSAlert plugin. Make sure the
   SMSSendDriver syspref is also set up with your SMS plugin of choice.
2) Pick a patron and edit its profile.
3) In the messaging preferences fieldset, under SMS number, enter any
   number with the format 00 AAA XXXXXXXXX (ie : 00111123456789).
   Try to save the modification : you should have an error message.
4) Try instead a number with the format +AAAXXXXXXXXX (ie :
   111123456789), that one should be accepted.
5) Save your modification then check that the number has been changed.
6) Apply patch.
7) Repeat step 3 to 5.
8) Observe the error is gone.
9) Try it once more with the format +00 AAA XXXXXXXXX (+00111123456789),
   that format should not be accepted: this is intentional.
10) Sign off.

    Thanks-to: Didier Gautheron <didier.gautheron@biblibre.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 16 Katrin Fischer 2022-05-13 08:22:24 UTC
Created attachment 134955 [details] [review]
Bug 30026: Quick fix for SmsAlert Hint

Just modifying the hint provided in patrons' messaging preferences to
allow either + or 00 for international formats.
No test plan needed.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 17 Katrin Fischer 2022-05-13 08:23:17 UTC
I think this is in a bit of grey area between enh and bug. As it modifies strings in the OPAC, I think it would be ok to push after release and then backport.
Comment 18 Tomás Cohen Arazi 2022-06-06 17:02:41 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 19 Lucas Gass 2022-07-12 15:48:12 UTC
Backported to 22.05.x for 22.05.03