With bug 14695 patrons can now place multiple holds on same biblio. The ReserveSlip function used by hold-transfer-slip.pl though doesn't take itemnumber as param, so any item returned belonging to the same patron's reservation on the same biblio will trigger the same slip, which is the first match. It itemnumber (or barcode) is included in request to ReserveSlip, the correct hold slip should be returned. GetReserveId used in ReserveSlip takes itemnumber anyways, so it makes sense to include it.
Created attachment 57239 [details] [review] Bug 17561 - ReserveSlip needs itemnumber for item level holds on same biblio This patch fixes a regression after bug 14695. This patch adds itemnumber and barcode as optional params in ReserveSlip used by hold-transfer-slip.pl to generate HOLD_SLIP. This is for ReserveSlip to be able to generate correct slips when items in multi-item holds are checked in. Test plan: 1) activate a circulation rule with multi-item holds 2) Place two holds on same biblio for patron 3) for debugging, either use browser console to observe POST request and responses or use info from reserves, e.g. reserve_id in the HOLD_SLIP 4) checkin both items on pickup branch 5) note that both holds are effectuated, but reserve_id is the same on both slips 6) also note that there is no itemnumber or barcode in the requests from returns.pl 7) Apply this patch 8) repeat 2-4 9) note that reserve_id is now different on the two slips
Is it ready for sign-off process?
I'll just throw in a few tests on ReserveSlip, as it is part of C4::Reserves. then it can be tested
Created attachment 57293 [details] [review] Bug 17561 - ReserveSlip needs itemnumber for item level holds on same biblio This patch fixes a regression after bug 14695. This patch adds itemnumber and barcode as optional params in ReserveSlip used by hold-transfer-slip.pl to generate HOLD_SLIP. This is for ReserveSlip to be able to generate correct slips when items in multi-item holds are checked in. Test plan: 1) activate a circulation rule with multi-item holds 2) Place two holds on same biblio for patron 3) for debugging, either use browser console to observe POST request and responses or use info from reserves, e.g. reserve_id in the HOLD_SLIP 4) checkin two items from same biblio on pickup branch 5) note that both holds are effectuated, but reserve_id is the same on both slips 6) also note that there is no itemnumber or barcode in the requests from returns.pl 7) Apply this patch 8) repeat 2-4 9) note that reserve_id is now different on the two slips and/or: Run tests: t/db_dependent/Reserves/ReserveSlip.t should fail before patch and pass after
t/db_dependent/Reserves/ReserveSlip.t .. DBIx::Class::Storage::DBI::select_single(): Query returned more than one row. SQL that returns multiple rows is DEPRECATED for ->find and ->single at /home/vagrant/kohaclone/Koha/Objects.pm line 83 t/db_dependent/Reserves/ReserveSlip.t .. 1/5 DBIx::Class::Storage::DBI::select_single(): Query returned more than one row. SQL that returns multiple rows is DEPRECATED for ->find and ->single at /home/vagrant/kohaclone/Koha/Objects.pm line 83 Can't use an undefined value as a HASH reference at t/db_dependent/Reserves/ReserveSlip.t line 112. # Looks like your test exited with 255 just after 1. t/db_dependent/Reserves/ReserveSlip.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 4/5 subtests Test Summary Report ------------------- t/db_dependent/Reserves/ReserveSlip.t (Wstat: 65280 Tests: 1 Failed: 0) Non-zero exit status: 255 Parse errors: Bad plan. You planned 5 tests but ran 1. Files=1, Tests=1, 2 wallclock secs ( 0.02 usr 0.00 sys + 1.68 cusr 0.07 csys = 1.77 CPU) Result: FAIL
Created attachment 72678 [details] [review] Bug 17561 - ReserveSlip needs itemnumber for item level holds on same biblio This patch fixes a regression after bug 14695. This patch adds itemnumber and barcode as optional params in ReserveSlip used by hold-transfer-slip.pl to generate HOLD_SLIP. This is for ReserveSlip to be able to generate correct slips when items in multi-item holds are checked in. Test plan: 1) activate a circulation rule with multi-item holds 2) Place two holds on same biblio for patron 3) for debugging, either use browser console to observe POST request and responses or use info from reserves, e.g. reserve_id in the HOLD_SLIP 4) checkin two items from same biblio on pickup branch 5) note that both holds are effectuated, but reserve_id is the same on both slips 6) also note that there is no itemnumber or barcode in the requests from returns.pl 7) Apply this patch 8) repeat 2-4 9) note that reserve_id is now different on the two slips and/or: Run tests: t/db_dependent/Reserves/ReserveSlip.t should fail before patch and pass after
Why not refactor to a hash reference parameter format? It makes adding/removing parameters much easier. Also, when a function starts to break three parameters, it probably should use a hashref. And lastly, it's a really small refactor, and we do have some coding guidelines: https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL16:_Hashrefs_should_be_used_as_arguments
Created attachment 72939 [details] [review] Bug 17561 - ReserveSlip needs itemnumber for item level holds on same biblio This patch fixes a regression after bug 14695. This patch adds itemnumber and barcode as optional params in ReserveSlip used by hold-transfer-slip.pl to generate HOLD_SLIP. This is for ReserveSlip to be able to generate correct slips when items in multi-item holds are checked in. Test plan: 1) activate a circulation rule with multi-item holds 2) Place two holds on same biblio for patron 3) for debugging, either use browser console to observe POST request and responses or use info from reserves, e.g. reserve_id in the HOLD_SLIP 4) checkin two items from same biblio on pickup branch 5) note that both holds are effectuated, but reserve_id is the same on both slips 6) also note that there is no itemnumber or barcode in the requests from returns.pl 7) Apply this patch 8) repeat 2-4 9) note that reserve_id is now different on the two slips and/or: Run tests: t/db_dependent/Reserves/ReserveSlip.t
rebased and changed to args hash
Created attachment 72967 [details] [review] Bug 17561 - ReserveSlip needs itemnumber for item level holds on same biblio This patch fixes a regression after bug 14695. This patch adds itemnumber and barcode as optional params in ReserveSlip used by hold-transfer-slip.pl to generate HOLD_SLIP. This is for ReserveSlip to be able to generate correct slips when items in multi-item holds are checked in. Test plan: 1) activate a circulation rule with multi-item holds 2) Place two holds on same biblio for patron 3) for debugging, either use browser console to observe POST request and responses or use info from reserves, e.g. reserve_id in the HOLD_SLIP 4) checkin two items from same biblio on pickup branch 5) note that both holds are effectuated, but reserve_id is the same on both slips 6) also note that there is no itemnumber or barcode in the requests from returns.pl 7) Apply this patch 8) repeat 2-4 9) note that reserve_id is now different on the two slips and/or: Run tests: t/db_dependent/Reserves/ReserveSlip.t Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 73064 [details] [review] Bug 17561 - ReserveSlip needs itemnumber for item level holds on same biblio This patch fixes a regression after bug 14695. This patch adds itemnumber and barcode as optional params in ReserveSlip used by hold-transfer-slip.pl to generate HOLD_SLIP. This is for ReserveSlip to be able to generate correct slips when items in multi-item holds are checked in. Test plan: 1) activate a circulation rule with multi-item holds 2) Place two holds on same biblio for patron 3) for debugging, either use browser console to observe POST request and responses or use info from reserves, e.g. reserve_id in the HOLD_SLIP 4) checkin two items from same biblio on pickup branch 5) note that both holds are effectuated, but reserve_id is the same on both slips 6) also note that there is no itemnumber or barcode in the requests from returns.pl 7) Apply this patch 8) repeat 2-4 9) note that reserve_id is now different on the two slips and/or: Run tests: t/db_dependent/Reserves/ReserveSlip.t Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Maksim Sen <maksim.sen@inlibro.com>
Comment on attachment 73064 [details] [review] Bug 17561 - ReserveSlip needs itemnumber for item level holds on same biblio Review of attachment 73064 [details] [review]: ----------------------------------------------------------------- ::: C4/Reserves.pm @@ +1857,4 @@ > > =head2 ReserveSlip > > + ReserveSlip($args => { This description doesn't reflect how to actually call the sub now ( using a hashref ). Please update. @@ +1886,5 @@ > + my $hold; > + if ($args->{itemnumber}) { > + $hold = Koha::Holds->search({biblionumber => $args->{biblionumber}, borrowernumber => $args->{borrowernumber}, itemnumber => $args->{itemnumber} })->next; > + } elsif ($args->{barcode}) { > + my $itemnumber = Koha::Items->find({ barcode => $args->{barcode} }); $itemnumber is never used @@ +1888,5 @@ > + $hold = Koha::Holds->search({biblionumber => $args->{biblionumber}, borrowernumber => $args->{borrowernumber}, itemnumber => $args->{itemnumber} })->next; > + } elsif ($args->{barcode}) { > + my $itemnumber = Koha::Items->find({ barcode => $args->{barcode} }); > + if ($args->{itemnumber}) { > + $hold = Koha::Holds->search({biblionumber => $args->{biblionumber}, borrowernumber => $args->{borrowernumber}, itemnumber => $args->{itemnumber} })->next; this makes no sense. This is exactly the same call is if(itemnumber) but is inside "if barcode && itemnumber" but barcode is never used
There are some issues with the changes to ReserveSlip that need fixed. In addition, other calls to ReserveSlip need to be updated for the new hashref parameter.
Not sure if this is related or not, but we have seen holds trapped and filled not roll off of a patron's hold list. As in, a specific item will show as both checked out to that person and on hold for them.
Possible duplicate 19549?
Created attachment 75421 [details] [review] Bug 17561: ReserveSlip needs itemnumber for item level holds on same biblio This patch fixes a regression after bug 14695. This patch adds itemnumber and barcode as optional params in ReserveSlip used by hold-transfer-slip.pl to generate HOLD_SLIP. This is for ReserveSlip to be able to generate correct slips when items in multi-item holds are checked in. Test plan: 1) activate a circulation rule with multi-item holds 2) Place two holds on same biblio for patron 3) for debugging, either use browser console to observe POST request and responses or use info from reserves, e.g. reserve_id in the HOLD_SLIP 4) checkin two items from same biblio on pickup branch 5) note that both holds are effectuated, but reserve_id is the same on both slips 6) also note that there is no itemnumber or barcode in the requests from returns.pl 7) Apply this patch 8) repeat 2-4 9) note that reserve_id is now different on the two slips and/or: Run tests: t/db_dependent/Reserves/ReserveSlip.t Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Maksim Sen <maksim.sen@inlibro.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 75422 [details] [review] Bug 17561 [QA Followup]: Simplify new ReserveSlip code Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 75423 [details] [review] Bug 17561 [QA Followup]: Update existing unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 75424 [details] [review] Bug 17561: ReserveSlip needs itemnumber for item level holds on same biblio This patch fixes a regression after bug 14695. This patch adds itemnumber and barcode as optional params in ReserveSlip used by hold-transfer-slip.pl to generate HOLD_SLIP. This is for ReserveSlip to be able to generate correct slips when items in multi-item holds are checked in. Test plan: 1) activate a circulation rule with multi-item holds 2) Place two holds on same biblio for patron 3) for debugging, either use browser console to observe POST request and responses or use info from reserves, e.g. reserve_id in the HOLD_SLIP 4) checkin two items from same biblio on pickup branch 5) note that both holds are effectuated, but reserve_id is the same on both slips 6) also note that there is no itemnumber or barcode in the requests from returns.pl 7) Apply this patch 8) repeat 2-4 9) note that reserve_id is now different on the two slips and/or: Run tests: t/db_dependent/Reserves/ReserveSlip.t Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Maksim Sen <maksim.sen@inlibro.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 75425 [details] [review] Bug 17561 [QA Followup]: Simplify new ReserveSlip code Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 75426 [details] [review] Bug 17561 [QA Followup]: Update existing unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
*** Bug 19549 has been marked as a duplicate of this bug. ***
Created attachment 75659 [details] [review] Bug 17561: (follow-up) Pass itemnumber on returns and correct itemnumber param in circulation To test: 1 - Place two holds on a bib for a single patron 2 - Check in both items, confirm the screen message and slip are correct 3 - Checkout both items to another patron, confirm the screen message and slip are correct
There were a few parts missing in my testing, can I get a second QA on this one?
Created attachment 75675 [details] [review] Bug 17561: ReserveSlip needs itemnumber for item level holds on same biblio This patch fixes a regression after bug 14695. This patch adds itemnumber and barcode as optional params in ReserveSlip used by hold-transfer-slip.pl to generate HOLD_SLIP. This is for ReserveSlip to be able to generate correct slips when items in multi-item holds are checked in. Test plan: 1) activate a circulation rule with multi-item holds 2) Place two holds on same biblio for patron 3) for debugging, either use browser console to observe POST request and responses or use info from reserves, e.g. reserve_id in the HOLD_SLIP 4) checkin two items from same biblio on pickup branch 5) note that both holds are effectuated, but reserve_id is the same on both slips 6) also note that there is no itemnumber or barcode in the requests from returns.pl 7) Apply this patch 8) repeat 2-4 9) note that reserve_id is now different on the two slips and/or: Run tests: t/db_dependent/Reserves/ReserveSlip.t Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Maksim Sen <maksim.sen@inlibro.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 75676 [details] [review] Bug 17561: (QA follow-up) Simplify new ReserveSlip code Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 75677 [details] [review] Bug 17561: (QA follow-up) Update existing unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 75678 [details] [review] Bug 17561: (follow-up) Pass itemnumber on returns and correct itemnumber param in circulation To test: 1 - Place two holds on a bib for a single patron 2 - Check in both items, confirm the screen message and slip are correct 3 - Checkout both items to another patron, confirm the screen message and slip are correct Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Pushed to master for 18.11, awesome work everybody!
This patch has been pushed to 18.05.x and will be in 18.05.01 Nice to see this bug squashed.
Pushed to 17.11.x for 17.11.07