Bug 20889

Summary: Items marked as not for loan can be checked out
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: CirculationAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: critical    
Priority: P5 - low CC: fridolin.somers, gmcharlt, katrin.fischer, kyle.m.hall, m.de.rooy, martin.renvoize, nick
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20963
Change sponsored?: --- Patch complexity: Small patch
Who signed the patch off: Text to go in the release notes:
Version(s) released in:
Bug Depends on: 19943    
Bug Blocks:    
Attachments: Bug 20889: Add tests - not for loan items cannot be checked out
Bug 20889: Prevent not for loan items to be checked out
Bug 20889: Add tests - not for loan items cannot be checked out
Bug 20889: Prevent not for loan items to be checked out
Bug 20889: Add tests - not for loan items cannot be checked out
Bug 20889: Prevent not for loan items to be checked out

Description Jonathan Druart 2018-06-06 17:23:52 UTC
To recreate:

1. Item type defined at item level (item-level_itypes=1)
2. Mark an item type not for loan (itemtypes.notforloan=1)
3. Checkout an item using this item type (items.itype="BK" for instance)
=> Checkout is not blocked!

I suspect
  commit 3953fdb921d7f280ffbaca813956357aa67d42ca
  Bug 19943: Remove itemtype vs itype confusion in CanBookBeIssued
to be the root of this issue. One occurrence of $item->{itemtype} has not been replaced. In this case it refers to the biblioitem->{itemtype} value whereas we want to use $item->{itype}. So this issue does not happen if items.itype==biblioitem.itemtype (just in case you are not reproducing the problem).
Comment 1 Jonathan Druart 2018-06-06 17:26:52 UTC
Created attachment 75859 [details] [review]
Bug 20889: Add tests - not for loan items cannot be checked out
Comment 2 Jonathan Druart 2018-06-06 17:26:56 UTC
Created attachment 75860 [details] [review]
Bug 20889: Prevent not for loan items to be checked out

1. Item type defined at item level (item-level_itypes=1)
2. Mark an item type not for loan (itemtypes.notforloan=1)
3. Checkout an item using this item type (items.itype="BK" for instance)
=> Checkout is not blocked!

I suspect
  commit 3953fdb921d7f280ffbaca813956357aa67d42ca
  Bug 19943: Remove itemtype vs itype confusion in CanBookBeIssued
to be the root of this issue.
One occurrence of $item->{itemtype} has not been replaced.
In this case it refers to the biblioitem->{itemtype} value whereas we want to use
$item->{itype}. So this issue does not happen if items.itype==biblioitem.itemtype
(just in case you are not reproducing the problem).

Test plan:
Make sure not for loan items cannot be checked out
Comment 3 Jonathan Druart 2018-06-06 17:27:34 UTC
Fridolin, I would suggest you to backport the unit tests.
Comment 4 Martin Renvoize 2018-06-07 07:49:27 UTC
Created attachment 75865 [details] [review]
Bug 20889: Add tests - not for loan items cannot be checked out

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2018-06-07 07:49:29 UTC
Created attachment 75866 [details] [review]
Bug 20889: Prevent not for loan items to be checked out

1. Item type defined at item level (item-level_itypes=1)
2. Mark an item type not for loan (itemtypes.notforloan=1)
3. Checkout an item using this item type (items.itype="BK" for instance)
=> Checkout is not blocked!

I suspect
  commit 3953fdb921d7f280ffbaca813956357aa67d42ca
  Bug 19943: Remove itemtype vs itype confusion in CanBookBeIssued
to be the root of this issue.
One occurrence of $item->{itemtype} has not been replaced.
In this case it refers to the biblioitem->{itemtype} value whereas we want to use
$item->{itype}. So this issue does not happen if items.itype==biblioitem.itemtype
(just in case you are not reproducing the problem).

Test plan:
Make sure not for loan items cannot be checked out

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Marcel de Rooy 2018-06-07 13:39:23 UTC
Just a description
not for loan at item type level
second time should be item level
Comment 7 Marcel de Rooy 2018-06-07 13:40:24 UTC
Created attachment 75876 [details] [review]
Bug 20889: Add tests - not for loan items cannot be checked out

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2018-06-07 13:40:28 UTC
Created attachment 75877 [details] [review]
Bug 20889: Prevent not for loan items to be checked out

1. Item type defined at item level (item-level_itypes=1)
2. Mark an item type not for loan (itemtypes.notforloan=1)
3. Checkout an item using this item type (items.itype="BK" for instance)
=> Checkout is not blocked!

I suspect
  commit 3953fdb921d7f280ffbaca813956357aa67d42ca
  Bug 19943: Remove itemtype vs itype confusion in CanBookBeIssued
to be the root of this issue.
One occurrence of $item->{itemtype} has not been replaced.
In this case it refers to the biblioitem->{itemtype} value whereas we want to use
$item->{itype}. So this issue does not happen if items.itype==biblioitem.itemtype
(just in case you are not reproducing the problem).

Test plan:
Make sure not for loan items cannot be checked out

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Nick Clemens 2018-06-08 13:58:27 UTC
Pushed to master for 18.11, awesome work all!
Comment 10 Martin Renvoize 2018-06-11 07:54:38 UTC
This patch has been pushed to 18.05.x and will be in 18.05.01
Comment 11 Fridolin SOMERS 2018-06-13 06:33:45 UTC
Depends on Bug 19943 not in 17.11.x
Comment 12 Katrin Fischer 2018-06-18 22:00:58 UTC
There still seems something off - if the user has a restriction and you use the 'temporary override' you can check out the item still. Even when AllowNotForLoanOverride doesn't allow that. Separate bug?
Comment 13 Martin Renvoize 2018-06-19 07:53:30 UTC
Ack, there are sooo many combinations ;).

A new bug I reckon as this one is pushed and closed please Katrin :)
Comment 14 Katrin Fischer 2018-06-19 08:08:11 UTC
You are right ;) Bug 20963