Bug 14549

Summary: Hold not removed when item is checked out to patron who is not next in priority list
Product: Koha Reporter: Amanda <amanda.kelly>
Component: Hold requestsAssignee: Nick Clemens (kidclamp) <nick>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: major    
Priority: P5 - low CC: fridolin.somers, gmcharlt, katrin.fischer, kyle, nick, timdhannah, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.03
Attachments: shows AU member is Priority #1 and NZ member in #2 position
Bug 14549: Unit tests
Bug 14549: Check patrons reserves for any holds on a bib when checking out an item
Bug 14549: POD and variable cleanup
Bug 14549: Unit tests
Bug 14549: Check patrons reserves for any holds on a bib when checking out an item
Bug 14549: POD and variable cleanup
Bug 14549: Unit tests
Bug 14549: Check patrons reserves for any holds on a bib when checking out an item
Bug 14549: POD and variable cleanup

Description Amanda 2015-07-17 01:38:42 UTC Comment hidden (obsolete)
Comment 1 Owen Leonard 2016-08-12 13:57:17 UTC
I think the "'invalid' reserve queue' is a red herring since it's (I assume) a custom report and isn't related to the functionality of the holds system.

However, I am able to reproduce this problem in master:

1. Find a title with multiple holds.
2. Check an available item out to any patron who
   is not at the top of the hold priority list.
3. The hold for that patron is not removed -- it's
   still on hold for them.
Comment 2 Nick Clemens (kidclamp) 2016-08-24 07:02:08 UTC
Ah, I can recreate, but only after build_holds_queue has been run and picked up the hold for the first patron
Comment 3 Amanda 2016-08-26 04:29:19 UTC
Owen and Nick, 

yes you are correct. This was a customisation - the creation of an "invalid reserves queue". The fix was - Catalyst made the Invalid RQ just a report - the queue had been assigning an item to fill a reserve, it now ignores that. And fills it with whatever item is issued to the borrower.  That made the reserve come off.

So I will close this bug. 

thanks for looking into it.
Comment 4 Owen Leonard 2016-08-26 11:55:15 UTC
Reopening because this issue still exists:

> 1. Find a title with multiple holds.
> 2. Check an available item out to any patron who
>    is not at the top of the hold priority list.
> 3. The hold for that patron is not removed -- it's
>    still on hold for them.
Comment 5 Katrin Fischer 2017-08-15 19:59:21 UTC
Hi Owen, I just checked this on master and can't replicate:

1) Find record
2) place holds for 2 different patrons (record level)
3) check out any item to the person last in the holds queue
4) Hold has been removed from the list / has been filled

Does the problem still appear for you?
Comment 6 Nick Clemens (kidclamp) 2019-06-27 17:07:46 UTC
(In reply to Katrin Fischer from comment #5)
Confirmed on master:

   1) Find record
   2) place record level holds for 2 different patrons (record level)
 2.5) Run holds queue builder
   3) check out the item queued for patronith priority 1 to the second 
   4) Hold has been removed from the list / has been filled

CheckReserves seems to promise to return 'all reserves', however, it returns only a single hold from the holds queue if that has been built
Comment 7 Nick Clemens (kidclamp) 2019-06-28 12:54:02 UTC
Created attachment 91067 [details] [review]
Bug 14549: Unit tests

To test:
1 - Apply just this patch
2 - prove -v t/db_dependent/Reserves.t
3 - Failure
4 - Apply second patch
5 - prove -v t/db_dependent/Reserves.t
6 - Success!
Comment 8 Nick Clemens (kidclamp) 2019-06-28 12:54:05 UTC
Created attachment 91068 [details] [review]
Bug 14549: Check patrons reserves for any holds on a bib when checking out an item

MoveReserve uses CheckReserves to see if the current patron has any holds
on the title they are checking out, however, CheckReserves doesn't return
all holds on a biblio, it returns holds on the item from the holdsqueue
if they exist

This can create a condition where we check holds on an item, find we have it planned for
another borrower, confirm checkout to the current borrower, but don't fill their hold

To test:
  1) Find record
  2) place record level holds for 2 different patrons (record level)
  3) Run holds queue builder, check the queue to confirm an item was selected for patron 1
     Circulation->Holds queue->Library="All"
  4) Check out the item queued for patron with priority 1 to the second patron
  5) You should be asked to confirm, do so
  6) Note the item checks out, but both holds remain
  7) Apply patch
  8) Check in the item
  9) Don't confirm the hold
 10) Check the holds on the record and the holds queue
 11) Patron 1 should be priority 1 with an item selected from the holds queue
 12) Checkout to patron 2 as before
 13) Note the hold for patron 2 is filled this time
 14) Prove -v t/db_dependent/Reserves.t
Comment 9 Nick Clemens (kidclamp) 2019-06-28 12:54:08 UTC
Created attachment 91069 [details] [review]
Bug 14549: POD and variable cleanup
Comment 10 Chris Cormack 2019-07-25 22:24:03 UTC
Created attachment 91776 [details] [review]
Bug 14549: Unit tests

To test:
1 - Apply just this patch
2 - prove -v t/db_dependent/Reserves.t
3 - Failure
4 - Apply second patch
5 - prove -v t/db_dependent/Reserves.t
6 - Success!

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 11 Chris Cormack 2019-07-25 22:24:17 UTC
Created attachment 91777 [details] [review]
Bug 14549: Check patrons reserves for any holds on a bib when checking out an item

MoveReserve uses CheckReserves to see if the current patron has any holds
on the title they are checking out, however, CheckReserves doesn't return
all holds on a biblio, it returns holds on the item from the holdsqueue
if they exist

This can create a condition where we check holds on an item, find we have it planned for
another borrower, confirm checkout to the current borrower, but don't fill their hold

To test:
  1) Find record
  2) place record level holds for 2 different patrons (record level)
  3) Run holds queue builder, check the queue to confirm an item was selected for patron 1
     Circulation->Holds queue->Library="All"
  4) Check out the item queued for patron with priority 1 to the second patron
  5) You should be asked to confirm, do so
  6) Note the item checks out, but both holds remain
  7) Apply patch
  8) Check in the item
  9) Don't confirm the hold
 10) Check the holds on the record and the holds queue
 11) Patron 1 should be priority 1 with an item selected from the holds queue
 12) Checkout to patron 2 as before
 13) Note the hold for patron 2 is filled this time
 14) Prove -v t/db_dependent/Reserves.t

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 12 Chris Cormack 2019-07-25 22:24:41 UTC
Created attachment 91778 [details] [review]
Bug 14549: POD and variable cleanup

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 13 Tomás Cohen Arazi 2019-08-02 19:44:21 UTC
Created attachment 91941 [details] [review]
Bug 14549: Unit tests

To test:
1 - Apply just this patch
2 - prove -v t/db_dependent/Reserves.t
3 - Failure
4 - Apply second patch
5 - prove -v t/db_dependent/Reserves.t
6 - Success!

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2019-08-02 19:44:24 UTC
Created attachment 91942 [details] [review]
Bug 14549: Check patrons reserves for any holds on a bib when checking out an item

MoveReserve uses CheckReserves to see if the current patron has any holds
on the title they are checking out, however, CheckReserves doesn't return
all holds on a biblio, it returns holds on the item from the holdsqueue
if they exist

This can create a condition where we check holds on an item, find we have it planned for
another borrower, confirm checkout to the current borrower, but don't fill their hold

To test:
  1) Find record
  2) place record level holds for 2 different patrons (record level)
  3) Run holds queue builder, check the queue to confirm an item was selected for patron 1
     Circulation->Holds queue->Library="All"
  4) Check out the item queued for patron with priority 1 to the second patron
  5) You should be asked to confirm, do so
  6) Note the item checks out, but both holds remain
  7) Apply patch
  8) Check in the item
  9) Don't confirm the hold
 10) Check the holds on the record and the holds queue
 11) Patron 1 should be priority 1 with an item selected from the holds queue
 12) Checkout to patron 2 as before
 13) Note the hold for patron 2 is filled this time
 14) Prove -v t/db_dependent/Reserves.t

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2019-08-02 19:44:28 UTC
Created attachment 91943 [details] [review]
Bug 14549: POD and variable cleanup

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Martin Renvoize 2019-08-05 10:40:14 UTC
Nice work!

Pushed to master for 19.11.00
Comment 17 Fridolin Somers 2019-08-09 13:28:53 UTC
Pushed to 19.05.x for 19.05.03