Bug 33791 - $hold->fill does not set itemnumber when checking out without confirming hold
Summary: $hold->fill does not set itemnumber when checking out without confirming hold
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on: 29869
Blocks:
  Show dependency treegraph
 
Reported: 2023-05-22 09:54 UTC by Marcel de Rooy
Modified: 2023-12-28 20:47 UTC (History)
2 users (show)

See Also:
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


Attachments
Bug 33791: Pass itemnumber to $hold->fill (2.15 KB, patch)
2023-05-22 11:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33791: Add unit test (1.75 KB, patch)
2023-05-22 11:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33791: Pass itemnumber to $hold->fill (2.21 KB, patch)
2023-05-22 11:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33791: Add unit test (1.77 KB, patch)
2023-05-22 13:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33791: Pass itemnumber to $hold->fill (2.23 KB, patch)
2023-05-22 13:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33791: Add unit test (1.76 KB, patch)
2023-05-22 13:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33791: Pass itemnumber to $hold->fill (2.22 KB, patch)
2023-05-22 13:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33791: (QA follow-up) Stick to 'item_id' for the parameter name (3.03 KB, patch)
2023-05-22 17:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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