Bug 19017 - Script that checks and transforms SMS alert numbers
Summary: Script that checks and transforms SMS alert numbers
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Arnaud
QA Contact: Testopia
URL:
Keywords: dependency
Depends on:
Blocks:
 
Reported: 2017-08-01 13:08 UTC by Alex Arnaud
Modified: 2018-02-12 12:11 UTC (History)
5 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:


Attachments
Checks and transform SMS alert number (8.27 KB, patch)
2017-08-01 13:10 UTC, Alex Arnaud
Details | Diff | Splinter Review
Checks and transform SMS alert number (8.26 KB, patch)
2017-08-04 12:32 UTC, Alex Arnaud
Details | Diff | Splinter Review
Checks and transform SMS alert number (8.32 KB, patch)
2017-10-20 14:14 UTC, Simon Pouchol
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Arnaud 2017-08-01 13:08:04 UTC
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).
Comment 1 Alex Arnaud 2017-08-01 13:10:45 UTC
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
Comment 2 Owen Leonard 2017-08-02 15:57:23 UTC
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
Comment 3 Alex Arnaud 2017-08-04 12:32:32 UTC
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
Comment 4 Owen Leonard 2017-08-14 13:28:08 UTC
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
Comment 5 Simon Pouchol 2017-10-20 14:14:35 UTC
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>
Comment 6 Jonathan Druart 2018-01-10 13:30:08 UTC
This is too specific, we should provide a generic way to do that.
The prefix code should be a parameter of the script.
Comment 7 Katrin Fischer 2018-01-10 22:41:52 UTC
Should we set this to In discussion or failed?
Comment 8 Jonathan Druart 2018-01-15 13:16:35 UTC
I did not switch the status to get another QA point of view.
Comment 9 Alex Arnaud 2018-01-17 15:10:07 UTC
(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.
Comment 10 Katrin Fischer 2018-02-07 07:08:28 UTC
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!
Comment 11 Alex Arnaud 2018-02-12 08:52:16 UTC
(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!
Comment 12 Alex Arnaud 2018-02-12 08:57:14 UTC
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
Comment 13 Katrin Fischer 2018-02-12 10:28:55 UTC
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.
Comment 14 paxed 2018-02-12 12:11:07 UTC
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.