Here is a proposal for a script that would check SMS alert number field. Our need is to have number that are correctly formatted as expetced by the SMS provider (ie: 0033678240967 or +33678240967 and not 0678240967).
Created attachment 65396 [details] [review] Checks and transform SMS alert number This patch adds a new package C4::SMSNumber::Fr (new ones could be added for other countries) and a scrip (misc/check_smsalertnumber.pl): check_smsalertnumber.pl -a|--area fr [-h|--help] [-c|--confirm] [-o|--country-option] [-l|--liste-mode] Test plan #1: - Create or modify patrons and add some SMS alert numbers like: 06 98 35 72 28, 0656871221, +336-01-55-83-11, 0033 6.23.54.54.00, 0126698376 - launch perl misc/check_smsalertnumber.pl -a Fr - Transformation proposal should be like: - 06 98 35 72 28 => 0698357228, - 0656871221 => no proposal (number Ok), - +336-01-55-83-11 => 0601558311, - 0033 6.23.54.54.00 => 0623545400, - 0126698376 => Can't find a replacement Test plan #2: - with the same numbers launch perl misc/check_smsalertnumber.pl -a Fr -o +33 - Transformation proposal should be like: - 06 98 35 72 28 => +33698357228, - 0656871221 => +33656871221, - +336-01-55-83-11 => +33601558311, - 0033 6.23.54.54.00 => +33623545400, - 0126698376 => Can't find a replacement Test plan #3: - with the same numbers launch perl misc/check_smsalertnumber.pl -a Fr -o 0033 - Transformation proposal should be like: - 06 98 35 72 28 => 0033698357228, - 0656871221 => 0033656871221, - +336-01-55-83-11 => 0033601558311, - 0033 6.23.54.54.00 => 0033623545400, - 0126698376 => Can't find a replacement
This works as it should, but I get a couple of errors from the qa script. OK C4/SMSNumber/Fr.pm OK critic OK forbidden patterns OK git manipulation OK pod OK spelling OK valid FAIL misc/check_smsalertnumber.pl FAIL critic Stricture disabled at line 60, column 5. See page 429 of PBP. OK forbidden patterns OK git manipulation OK pod FAIL spelling tranform ==> transform OK valid
Created attachment 65498 [details] [review] Checks and transform SMS alert number This patch adds a new package C4::SMSNumber::Fr (new ones could be added for other countries) and a scrip (misc/check_smsalertnumber.pl): check_smsalertnumber.pl -a|--area fr [-h|--help] [-c|--confirm] [-o|--country-option] [-l|--liste-mode] Test plan #1: - Create or modify patrons and add some SMS alert numbers like: 06 98 35 72 28, 0656871221, +336-01-55-83-11, 0033 6.23.54.54.00, 0126698376 - launch perl misc/check_smsalertnumber.pl -a Fr - Transformation proposal should be like: - 06 98 35 72 28 => 0698357228, - 0656871221 => no proposal (number Ok), - +336-01-55-83-11 => 0601558311, - 0033 6.23.54.54.00 => 0623545400, - 0126698376 => Can't find a replacement Test plan #2: - with the same numbers launch perl misc/check_smsalertnumber.pl -a Fr -o +33 - Transformation proposal should be like: - 06 98 35 72 28 => +33698357228, - 0656871221 => +33656871221, - +336-01-55-83-11 => +33601558311, - 0033 6.23.54.54.00 => +33623545400, - 0126698376 => Can't find a replacement Test plan #3: - with the same numbers launch perl misc/check_smsalertnumber.pl -a Fr -o 0033 - Transformation proposal should be like: - 06 98 35 72 28 => 0033698357228, - 0656871221 => 0033656871221, - +336-01-55-83-11 => 0033601558311, - 0033 6.23.54.54.00 => 0033623545400, - 0126698376 => Can't find a replacement
This isn't correct is it? ------------ perl check_smsalertnumber.pl -a Fr Number: 0033 6.23.54.54.00 at /home/vagrant/kohaclone/C4/SMSNumber/Fr.pm line 24. Patron n° 58808 has an invalid phone number for area Fr ( 0033 6.23.54.54.00). Can't find a replacement
Created attachment 68319 [details] [review] Checks and transform SMS alert number This patch adds a new package C4::SMSNumber::Fr (new ones could be added for other countries) and a scrip (misc/check_smsalertnumber.pl): check_smsalertnumber.pl -a|--area fr [-h|--help] [-c|--confirm] [-o|--country-option] [-l|--liste-mode] Test plan #1: - Create or modify patrons and add some SMS alert numbers like: 06 98 35 72 28, 0656871221, +336-01-55-83-11, 0033 6.23.54.54.00, 0126698376 - launch perl misc/check_smsalertnumber.pl -a Fr - Transformation proposal should be like: - 06 98 35 72 28 => 0698357228, - 0656871221 => no proposal (number Ok), - +336-01-55-83-11 => 0601558311, - 0033 6.23.54.54.00 => 0623545400, - 0126698376 => Can't find a replacement Test plan #2: - with the same numbers launch perl misc/check_smsalertnumber.pl -a Fr -o +33 - Transformation proposal should be like: - 06 98 35 72 28 => +33698357228, - 0656871221 => +33656871221, - +336-01-55-83-11 => +33601558311, - 0033 6.23.54.54.00 => +33623545400, - 0126698376 => Can't find a replacement Test plan #3: - with the same numbers launch perl misc/check_smsalertnumber.pl -a Fr -o 0033 - Transformation proposal should be like: - 06 98 35 72 28 => 0033698357228, - 0656871221 => 0033656871221, - +336-01-55-83-11 => 0033601558311, - 0033 6.23.54.54.00 => 0033623545400, - 0126698376 => Can't find a replacement https://bugs.koha-community.org/show_bug.cgi?id=19017 Signed-off-by: Simon Pouchol <simon.pouchol@biblibre.com>
This is too specific, we should provide a generic way to do that. The prefix code should be a parameter of the script.
Should we set this to In discussion or failed?
I did not switch the status to get another QA point of view.
(In reply to Jonathan Druart from comment #6) > This is too specific, we should provide a generic way to do that. > The prefix code should be a parameter of the script. Prefix code is passed to the script with -o|--options. It is intentionally generic in the case an other country package needs a different parameter.
I agree with Jonathan: - The French country code +33 is hard coded into the script which makes it only useful for French telephone numbers, but not usable for example for German ones (+41). This could easily be more generic and then much more useful. - There is an option --country-option, but it doesn't appear to be used? - It adds a new module to C4, which we should not do anymore. - It doesn't include tests. I think if we want something like that, it might be better as a clean-up script in misc/maintenance. Possibly without a separate module? I think it's not quite ready, sorry!
(In reply to Katrin Fischer from comment #10) > I agree with Jonathan: > > - The French country code +33 is hard coded into the script which makes it > only useful for French telephone numbers, but not usable for example for > German ones (+41). This could easily be more generic and then much more > useful. +33 (or 0033) is not hard coded into the script. Only in the pod. It is only in the C4::SMSNumber::Fr package which is dedicated to french number. You can add a C4::SMSNumber::De package with a german logic for transforming number and add to the pod: - De: Checks german mobile number. options: '+41', ... \n" like i did for french > - There is an option --country-option, but it doesn't appear to be used? Yes, forgot to remove this from the pod > - It adds a new module to C4, which we should not do anymore. Better in Koha/SMSNumber/* ? > - It doesn't include tests. Right. > > I think if we want something like that, it might be better as a clean-up > script in misc/maintenance. Possibly without a separate module? Separate modules contain the logic specific to each area. It's not easy/clear/possible to have a script that check and transform number for all the areas as the format of number could be different (i.e not the number of digit). So the area paramter determines which area module to use. > > I think it's not quite ready, sorry!
An other point i'm wondering about is the libraries having borrowers from different areas. It is the case for many universities. But, for now, the script runs for all the users in database without being aware of their country/area
You are right, the country-code is in the module. I am worried that different modules will be hard to maintain while containing a lot of duplicated code, at least for the European countries. I think better in Koha - but please check with Joubu for general approach.
Like Katrin, I'm also worried about having numbers from different countries. Couldn't this functionality basically be done with a phone number validator written in javascript and hooked on to the borrower edit/add form inputs? And for checking the phone numbers in the database, a simple report could be done.