Bug 19134 - C4::SMS does not handle drivers with more than two names well
Summary: C4::SMS does not handle drivers with more than two names well
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Magnus Enger
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-08-17 14:44 UTC by Magnus Enger
Modified: 2017-12-07 22:24 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19134 - C4::SMS falils on long driver name (1.60 KB, patch)
2017-08-17 20:57 UTC, Magnus Enger
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 19134: C4::SMS falils on long driver name (1.67 KB, patch)
2017-08-20 23:31 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 19134: C4::SMS falils on long driver name (1.75 KB, patch)
2017-08-25 08:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.