Bug 33791

Summary: $hold->fill does not set itemnumber when checking out without confirming hold
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Hold requestsAssignee: Marcel de Rooy <m.de.rooy>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: gmcharlt, martin.renvoize
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33792
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.07
Bug Depends on: 29869    
Bug Blocks:    
Attachments: Bug 33791: Pass itemnumber to $hold->fill
Bug 33791: Add unit test
Bug 33791: Pass itemnumber to $hold->fill
Bug 33791: Add unit test
Bug 33791: Pass itemnumber to $hold->fill
Bug 33791: Add unit test
Bug 33791: Pass itemnumber to $hold->fill
Bug 33791: (QA follow-up) Stick to 'item_id' for the parameter name

Description Marcel de Rooy 2023-05-22 09:54:50 UTC
If you add a hold on a book (next available), and then checkout an item of this book to the same patron, the hold is filled but without itemnumber.

Since we normally expect an itemnumber in old_reserves, this may be confusing. For instance, reserve_stats.pl ignores these holds because it filters on itemnumber.

Is this simply a bug or are there circumstances that this behavior is preferred? In other words, should we fix reserve_stats.pl and other code that assumes the itemnumber or should we fix MoveReserve/$hold->fill ?
Comment 1 Marcel de Rooy 2023-05-22 10:58:41 UTC
Interestingly, CheckReserves puts the given itemnumber (from the checkout) into its highest reserve of the result:
$highest->{'itemnumber'} = $item->itemnumber;

But MoveReserves does ignore it in the $res (==$highest) hash:
if ($res->{borrowernumber} == $borrowernumber) {
    my $hold = Koha::Holds->find( $res->{reserve_id} );
    $hold->fill;
So it may have worked, and was perhaps undone by refactoring?
Comment 2 Marcel de Rooy 2023-05-22 11:23:27 UTC
Created attachment 151507 [details] [review]
Bug 33791: Pass itemnumber to $hold->fill

Test plan:
Without this patch:
Place next available level on some book for patron A.
Checkout this book directly to patron A.
Check old_reserves table for this reserve; does not have itemnumber.
With this patch:
Do the same.
In old_reserves the itemnumber should be saved.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2023-05-22 11:39:15 UTC
Created attachment 151508 [details] [review]
Bug 33791: Add unit test

Test plan:
Run t/db_dependent/Koha/Hold.t
NOTE: This test should fail without following patch, but pass with it.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2023-05-22 11:39:17 UTC
Created attachment 151509 [details] [review]
Bug 33791: Pass itemnumber to $hold->fill

Test plan:

Without this patch:
Place next available level on some book for patron A.
Checkout this book directly to patron A.
Check old_reserves table for this reserve; does not have itemnumber.

With this patch:
Do the same.
In old_reserves the itemnumber should be saved.
Run again t/db_dependent/Koha/Hold.t. Should pass.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Martin Renvoize 2023-05-22 13:15:45 UTC
Created attachment 151524 [details] [review]
Bug 33791: Add unit test

Test plan:
Run t/db_dependent/Koha/Hold.t
NOTE: This test should fail without following patch, but pass with it.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2023-05-22 13:15:47 UTC
Created attachment 151525 [details] [review]
Bug 33791: Pass itemnumber to $hold->fill

Test plan:

Without this patch:
Place next available level on some book for patron A.
Checkout this book directly to patron A.
Check old_reserves table for this reserve; does not have itemnumber.

With this patch:
Do the same.
In old_reserves the itemnumber should be saved.
Run again t/db_dependent/Koha/Hold.t. Should pass.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Kyle M Hall 2023-05-22 13:16:00 UTC
Created attachment 151526 [details] [review]
Bug 33791: Add unit test

Test plan:
Run t/db_dependent/Koha/Hold.t
NOTE: This test should fail without following patch, but pass with it.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2023-05-22 13:16:07 UTC
Created attachment 151527 [details] [review]
Bug 33791: Pass itemnumber to $hold->fill

Test plan:

Without this patch:
Place next available level on some book for patron A.
Checkout this book directly to patron A.
Check old_reserves table for this reserve; does not have itemnumber.

With this patch:
Do the same.
In old_reserves the itemnumber should be saved.
Run again t/db_dependent/Koha/Hold.t. Should pass.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Martin Renvoize 2023-05-22 13:17:49 UTC
Looks like Kyle and I both PQA'd on this one simultaneously.. lol.. it's certainly PQA anyways.
Comment 10 Tomás Cohen Arazi 2023-05-22 17:45:17 UTC
Created attachment 151539 [details] [review]
Bug 33791: (QA follow-up) Stick to 'item_id' for the parameter name
Comment 11 Tomás Cohen Arazi 2023-05-22 17:46:21 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 12 Matt Blenkinsop 2023-06-08 17:09:39 UTC
Nice work everyone!

Pushed to stable for 22.11.x