Bug 27729 - Code around SkipHoldTrapOnNotForLoanValue contains two perl bugs
Summary: Code around SkipHoldTrapOnNotForLoanValue contains two perl bugs
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-02-19 10:17 UTC by Marcel de Rooy
Modified: 2021-12-13 21:09 UTC (History)
7 users (show)

See Also:
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 (1.50 KB, patch)
2021-02-19 10:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27729: Fix use of grep and split in CheckReserves (1.53 KB, patch)
2021-02-19 12:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27729: Fix use of grep and split in CheckReserves (1.58 KB, patch)
2021-02-22 16:26 UTC, Martin Renvoize
Details | Diff | Splinter Review

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