Bug 19134

Summary: C4::SMS does not handle drivers with more than two names well
Product: Koha Reporter: Magnus Enger <magnus>
Component: NoticesAssignee: Magnus Enger <magnus>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, katrin.fischer, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 19134 - C4::SMS falils on long driver name
[SIGNED-OFF] Bug 19134: C4::SMS falils on long driver name
Bug 19134: C4::SMS falils on long driver name

Description Magnus Enger 2017-08-17 14:44:54 UTC
This code:

 94     my $subpath = $driver;
 95     $subpath =~ s|::|/|;

is supposed to turn the "A::B" part of e.g. SMS::Send::A::B into "A/B", so it can be used as part of the path to the extra config file introduced in bug 13029. But it will only change the first occurence of :: into a /, so a driver with a name like SMS::Send::A::B::C will result in "A/B::C" being used as part of the path, which fails. 

A "g" modifier needs to be added to the regex, to do the substitution gloablly, thusly: 

 95     $subpath =~ s|::|/|g;
Comment 1 Magnus Enger 2017-08-17 20:57:02 UTC
Created attachment 66151 [details] [review]
Bug 19134 - C4::SMS falils on long driver name

Code in C4::SMS takes the part of the SMS::Send-driver that comes after
SMS::Send and tries to turn it into part of a path to a YAML file
that can contain additional parameters to SMS::Send. The current
code works for e.g. SMS::Send::A::B, but if there is one or more
extra names, it fails to turn :: into /. So we have:
SMS::Send::A::B    -> SMS/Send/A/B
SMS::Send::A::B::C -> SMS/Send/A/B::C
This patch makes sure all occurrences of :: are turned into /, by
adding a "g" modifier at the end of the regex.

Testing:
Testing this preperly would take a whole lot of setup for a very
small change. I would suggest that the following two oneliners
are enough to demonstrate that the change makes sense:
$ perl -e '$x = "a::b::c"; $x =~ s|::|/|; print $x, "\n";'
$ perl -e '$x = "a::b::c"; $x =~ s|::|/|g; print $x, "\n";'

So:
- Check that the output of these oneliners make sense
- Check that the patch changes the code in a similar way to the
  change from the first oneliner to the second.
Comment 2 Aleisha Amohia 2017-08-20 23:31:55 UTC
Created attachment 66256 [details] [review]
[SIGNED-OFF] Bug 19134: C4::SMS falils on long driver name

Code in C4::SMS takes the part of the SMS::Send-driver that comes after
SMS::Send and tries to turn it into part of a path to a YAML file
that can contain additional parameters to SMS::Send. The current
code works for e.g. SMS::Send::A::B, but if there is one or more
extra names, it fails to turn :: into /. So we have:
SMS::Send::A::B    -> SMS/Send/A/B
SMS::Send::A::B::C -> SMS/Send/A/B::C
This patch makes sure all occurrences of :: are turned into /, by
adding a "g" modifier at the end of the regex.

Testing:
Testing this preperly would take a whole lot of setup for a very
small change. I would suggest that the following two oneliners
are enough to demonstrate that the change makes sense:
$ perl -e '$x = "a::b::c"; $x =~ s|::|/|; print $x, "\n";'
$ perl -e '$x = "a::b::c"; $x =~ s|::|/|g; print $x, "\n";'

So:
- Check that the output of these oneliners make sense
- Check that the patch changes the code in a similar way to the
  change from the first oneliner to the second.

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 3 Marcel de Rooy 2017-08-25 08:11:43 UTC
Created attachment 66463 [details] [review]
Bug 19134: C4::SMS falils on long driver name

Code in C4::SMS takes the part of the SMS::Send-driver that comes after
SMS::Send and tries to turn it into part of a path to a YAML file
that can contain additional parameters to SMS::Send. The current
code works for e.g. SMS::Send::A::B, but if there is one or more
extra names, it fails to turn :: into /. So we have:
SMS::Send::A::B    -> SMS/Send/A/B
SMS::Send::A::B::C -> SMS/Send/A/B::C
This patch makes sure all occurrences of :: are turned into /, by
adding a "g" modifier at the end of the regex.

Testing:
Testing this preperly would take a whole lot of setup for a very
small change. I would suggest that the following two oneliners
are enough to demonstrate that the change makes sense:
$ perl -e '$x = "a::b::c"; $x =~ s|::|/|; print $x, "\n";'
$ perl -e '$x = "a::b::c"; $x =~ s|::|/|g; print $x, "\n";'

So:
- Check that the output of these oneliners make sense
- Check that the patch changes the code in a similar way to the
  change from the first oneliner to the second.

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2017-08-25 08:12:18 UTC
Magnus: Assignee field !
Comment 5 Jonathan Druart 2017-08-25 14:02:34 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 6 Magnus Enger 2017-08-30 07:39:57 UTC
This is needed in 17.05.x too.
Comment 7 Fridolin Somers 2017-09-29 11:29:01 UTC
Pushed to 17.05.x, will be in 17.05.05.
Comment 8 Katrin Fischer 2017-10-01 22:18:00 UTC
Doesn't apply cleanly to 16.11.x - please provide a rebased patch if you want this to be included.