Bug 24168 - Errors with use of CanItemBeReserved return value
Summary: Errors with use of CanItemBeReserved return value
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 15524 23116
Blocks:
  Show dependency treegraph
 
Reported: 2019-12-04 10:56 UTC by Fridolin SOMERS
Modified: 2019-12-20 15:08 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.01


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

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 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 2019-12-20 15:08:52 UTC
backported to 19.05.x for 19.05.06