Bug 27729

Summary: Code around SkipHoldTrapOnNotForLoanValue contains two perl bugs
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Hold requestsAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: andrewfh, fridolin.somers, gmcharlt, joonas.kylmala, kyle, martin.renvoize, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25232
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.04,20.05.10
Attachments: Bug 27729: Fix use of grep and split in CheckReserves
Bug 27729: Fix use of grep and split in CheckReserves
Bug 27729: Fix use of grep and split in CheckReserves

Description Marcel de Rooy 2021-02-19 10:17:33 UTC
A few lines of code were added to CheckReserves containing the wrong use of two perl functions grep and split. A test was added even making these things pass.

grep( $notforloan_per_item, @SkipHoldTrapOnNotForLoanValue )
This does not work. The author hoped that the array was checked for the value but perl doesnt support that. Use a block with eq test.

split( '|', C4::Context->preference('SkipHoldTrapOnNotForLoanValue') )
Perl split needs a regex but pipe is a special character. It needs escaping.
What this does on -1 is, creating an array of - and 1.
Confusingly, you can use this in javascript.
Comment 1 Marcel de Rooy 2021-02-19 10:21:08 UTC
Created attachment 117060 [details] [review]
Bug 27729: Fix use of grep and split in CheckReserves

A few lines of code were added to CheckReserves containing the wrong
use of two perl functions: grep and split on bug 25232.
A test was added even making these things pass.

Test plan:
Run t/db../Holds.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Kyle M Hall 2021-02-19 12:51:57 UTC
Created attachment 117075 [details] [review]
Bug 27729: Fix use of grep and split in CheckReserves

A few lines of code were added to CheckReserves containing the wrong
use of two perl functions: grep and split on bug 25232.
A test was added even making these things pass.

Test plan:
Run t/db../Holds.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 3 Martin Renvoize 2021-02-22 16:26:20 UTC
Created attachment 117170 [details] [review]
Bug 27729: Fix use of grep and split in CheckReserves

A few lines of code were added to CheckReserves containing the wrong
use of two perl functions: grep and split on bug 25232.
A test was added even making these things pass.

Test plan:
Run t/db../Holds.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2021-02-22 16:27:17 UTC
Oops.. easily done..

Works as expected, thanks for the fix.

Passing QA
Comment 5 Jonathan Druart 2021-02-23 12:22:31 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 6 Fridolin Somers 2021-02-25 09:43:36 UTC
Pushed to 20.11.x for 20.11.04
Comment 7 Andrew Fuerste-Henry 2021-03-05 16:44:21 UTC
Pushed to 20.05.x for 20.05.10
Comment 8 Victor Grousset/tuxayo 2021-03-19 00:06:26 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.