| Summary: | International phone number not supported for sending SMS | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Florian <florian.bontemps> |
| Component: | Patrons | Assignee: | Florian <florian.bontemps> |
| Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
| Severity: | minor | ||
| Priority: | P5 - low | CC: | dcook, gmcharlt, kyle.m.hall, lucas |
| Version: | Main | Keywords: | rel_22_11_candidate |
| Hardware: | All | ||
| OS: | All | ||
| See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25520 | ||
| 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
|
|
| Circulation function: | |||
| Attachments: |
Bug 30026: SMSAlert regex fixed for international
Bug 30026: Quick fix for SmsAlert Hint Bug 30026: SMSAlert regex fixed for international Bug 30026: Quick fix for SmsAlert Hint Bug 30026: SMSAlert regex fixed for international Bug 30026: Quick fix for SmsAlert Hint |
||
|
Description
Florian
2022-02-04 15:35:45 UTC
Created attachment 130167 [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> 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. 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? 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.
(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. (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. (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. (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. +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. 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. (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! Created attachment 131375 [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. 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> 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> 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> 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> 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. Pushed to master for 22.11. Nice work everyone, thanks! Backported to 22.05.x for 22.05.03 |