Bug 24168 - Errors with use of CanItemBeReserved return value
Summary: Errors with use of CanItemBeReserved return value
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 15524 23116
Blocks:
  Show dependency tree / graph
 
Reported: 2019-12-04 10:56 UTC by Fridolin Somers
Modified: 2021-06-14 21:29 UTC (History)
9 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:
20.05.00, 19.11.01
Circulation function:

Attachments
Bug 24168: (bug 23116 follow-up) AllowHoldPolicyOverride allows a librarian to almost place a hold on an item already on hold (1.78 KB, patch)
2019-12-04 15:07 UTC, Jonathan Druart 		Details | Diff | Splinter Review
Bug 24168: (bug 23116 follow-up) AllowHoldPolicyOverride allows a librarian to almost place a hold on an item already on hold (2.00 KB, patch)
2019-12-09 12:55 UTC, Marcel de Rooy 		Details | Diff | Splinter Review
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2019-12-04 10:56:34 UTC 
Bug 15524 changed CanItemBeReserved to return a hashref with status inside.

Looks like But 23116 was intruced with previous behavior.

So now there a uses of CanItemBeReserved return value as a string :
if ( $can_item_be_reserved eq 'OK' || ( $can_item_be_reserved ne 'itemAlreadyOnHold' && $can_override ) ) {

use git grep can_item_be_reserved.
Comment 1 Fridolin Somers 2019-12-04 11:06:01 UTC 
Also impact on Bug 22330
Comment 2 Fridolin Somers 2019-12-04 11:27:16 UTC 
(In reply to Fridolin SOMERS from comment #1)
> Also impact on Bug 22330

Oups sorry no its correct : 
  my $canitembereserved = C4::Reserves::CanItemBeReserved( $borrowernumber, $itemnumber, $branch )->{status};
Comment 3 Jonathan Druart 2019-12-04 15:07:54 UTC 
Created attachment 95994 [details] [review]
Bug 24168: (bug 23116 follow-up) AllowHoldPolicyOverride allows a librarian to almost place a hold on an item already on hold

This patch actually fixes the issue described on bug 23116.

Test plan:
See bug 23116.
Comment 4 Marcel de Rooy 2019-12-06 10:20:10 UTC 
Hmm. I actually do not see the difference this patch makes. Looks to me if nothing really changes here.
Before we tested $var->{status} eq 'OK' and now we put status into $var and test $var eq 'OK'.
Where is the bug that we should solve ?
Comment 5 Jonathan Druart 2019-12-06 13:10:47 UTC 
Take a look at the file and search for "can_item_be_reserved".
There is another occurrence.
Comment 6 Marcel de Rooy 2019-12-09 12:55:12 UTC 
Created attachment 96099 [details] [review]
Bug 24168: (bug 23116 follow-up) AllowHoldPolicyOverride allows a librarian to almost place a hold on an item already on hold

This patch actually fixes the issue described on bug 23116.

Test plan:
See bug 23116.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
See also comment5. Important to note that the later comparison with
itemAlreadyOnHold assumes that the variable is a string.
Comment 7 Marcel de Rooy 2019-12-09 12:56:08 UTC 
(In reply to Jonathan Druart from comment #5)
> Take a look at the file and search for "can_item_be_reserved".
> There is another occurrence.

OK Thats the point.
Passed QA
Comment 8 Martin Renvoize (ashimema) 2019-12-09 14:19:44 UTC 
Nice work everyone!

Pushed to master for 20.05
Comment 9 Joy Nelson 2019-12-10 20:16:30 UTC 
Pushed to stable 19.11.01
Comment 10 Lucas Gass (lukeg) 2019-12-20 15:08:52 UTC 
backported to 19.05.x for 19.05.06