Bug 20889 - Items marked as not for loan can be checked out
Summary: Items marked as not for loan can be checked out
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 19943
Blocks:
  Show dependency treegraph
 
Reported: 2018-06-06 17:23 UTC by Jonathan Druart
Modified: 2019-10-14 19:57 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:


Attachments
Bug 20889: Add tests - not for loan items cannot be checked out (5.73 KB, patch)
2018-06-06 17:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20889: Prevent not for loan items to be checked out (1.67 KB, patch)
2018-06-06 17:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20889: Add tests - not for loan items cannot be checked out (5.79 KB, patch)
2018-06-07 07:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20889: Prevent not for loan items to be checked out (1.73 KB, patch)
2018-06-07 07:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20889: Add tests - not for loan items cannot be checked out (5.88 KB, patch)
2018-06-07 13:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20889: Prevent not for loan items to be checked out (1.82 KB, patch)
2018-06-07 13:40 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 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