Bug 17561 - ReserveSlip needs itemnumber for item level holds on same biblio
Summary: ReserveSlip needs itemnumber for item level holds on same biblio
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Benjamin Rokseth
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 19549 (view as bug list)
Depends on: 14695
Blocks:
  Show dependency treegraph
 
Reported: 2016-11-06 22:35 UTC by Benjamin Rokseth
Modified: 2018-10-24 09:13 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17561 - ReserveSlip needs itemnumber for item level holds on same biblio (6.11 KB, patch)
2016-11-07 00:06 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17561 - ReserveSlip needs itemnumber for item level holds on same biblio (10.68 KB, patch)
2016-11-08 01:36 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17561 - ReserveSlip needs itemnumber for item level holds on same biblio (10.47 KB, patch)
2018-03-12 12:55 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17561 - ReserveSlip needs itemnumber for item level holds on same biblio (11.74 KB, patch)
2018-03-15 12:02 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17561 - ReserveSlip needs itemnumber for item level holds on same biblio (11.80 KB, patch)
2018-03-15 14:42 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 17561 - ReserveSlip needs itemnumber for item level holds on same biblio (11.85 KB, patch)
2018-03-16 20:46 UTC, Maksim Sen
Details | Diff | Splinter Review
Bug 17561: ReserveSlip needs itemnumber for item level holds on same biblio (11.95 KB, patch)
2018-05-18 12:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17561 [QA Followup]: Simplify new ReserveSlip code (3.07 KB, patch)
2018-05-18 12:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17561 [QA Followup]: Update existing unit tests (3.08 KB, patch)
2018-05-18 12:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17561: ReserveSlip needs itemnumber for item level holds on same biblio (11.95 KB, patch)
2018-05-18 12:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17561 [QA Followup]: Simplify new ReserveSlip code (3.08 KB, patch)
2018-05-18 12:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17561 [QA Followup]: Update existing unit tests (3.08 KB, patch)
2018-05-18 12:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17561: (follow-up) Pass itemnumber on returns and correct itemnumber param in circulation (4.06 KB, patch)
2018-05-30 13:10 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17561: ReserveSlip needs itemnumber for item level holds on same biblio (12.00 KB, patch)
2018-05-30 16:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17561: (QA follow-up) Simplify new ReserveSlip code (3.14 KB, patch)
2018-05-30 16:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17561: (QA follow-up) Update existing unit tests (3.14 KB, patch)
2018-05-30 16:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17561: (follow-up) Pass itemnumber on returns and correct itemnumber param in circulation (4.15 KB, patch)
2018-05-30 16:16 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Rokseth 2016-11-06 22:35:27 UTC
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.
Comment 1 Benjamin Rokseth 2016-11-07 00:06:13 UTC
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
Comment 2 Josef Moravec 2016-11-07 09:57:16 UTC
Is it ready for sign-off process?
Comment 3 Benjamin Rokseth 2016-11-07 13:57:19 UTC
I'll just throw in a few tests on ReserveSlip, as it is part of C4::Reserves. then it can be tested
Comment 4 Benjamin Rokseth 2016-11-08 01:36:07 UTC
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
Comment 5 Mika 2016-11-30 21:57:45 UTC
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
Comment 6 Benjamin Rokseth 2018-03-12 12:55:05 UTC
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
Comment 7 M. Tompsett 2018-03-15 01:17:49 UTC
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
Comment 8 Benjamin Rokseth 2018-03-15 12:02:08 UTC
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
Comment 9 Benjamin Rokseth 2018-03-15 12:02:47 UTC
rebased and changed to args hash
Comment 10 Brendan Gallagher 2018-03-15 14:42:57 UTC
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>
Comment 11 Maksim Sen 2018-03-16 20:46:55 UTC
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 12 Kyle M Hall 2018-03-30 12:00:58 UTC
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
Comment 13 Kyle M Hall 2018-03-30 12:10:58 UTC
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.
Comment 14 Martha Fuerst 2018-05-15 19:50:42 UTC
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.
Comment 15 Nick Clemens 2018-05-17 15:36:46 UTC
Possible duplicate 19549?
Comment 16 Kyle M Hall 2018-05-18 12:12:25 UTC
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>
Comment 17 Kyle M Hall 2018-05-18 12:12:32 UTC
Created attachment 75422 [details] [review]
Bug 17561 [QA Followup]: Simplify new ReserveSlip code

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2018-05-18 12:12:36 UTC
Created attachment 75423 [details] [review]
Bug 17561 [QA Followup]: Update existing unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall 2018-05-18 12:14:29 UTC
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>
Comment 20 Kyle M Hall 2018-05-18 12:14:37 UTC
Created attachment 75425 [details] [review]
Bug 17561 [QA Followup]: Simplify new ReserveSlip code

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2018-05-18 12:14:40 UTC
Created attachment 75426 [details] [review]
Bug 17561 [QA Followup]: Update existing unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2018-05-18 12:22:59 UTC
*** Bug 19549 has been marked as a duplicate of this bug. ***
Comment 23 Nick Clemens 2018-05-30 13:10:05 UTC
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
Comment 24 Nick Clemens 2018-05-30 13:11:32 UTC
There were a few parts missing in my testing, can I get a second QA on this one?
Comment 25 Jonathan Druart 2018-05-30 16:15:53 UTC
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>
Comment 26 Jonathan Druart 2018-05-30 16:15:57 UTC
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>
Comment 27 Jonathan Druart 2018-05-30 16:16:01 UTC
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>
Comment 28 Jonathan Druart 2018-05-30 16:16:05 UTC
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>
Comment 29 Nick Clemens 2018-06-04 17:49:17 UTC
Pushed to master for 18.11, awesome work everybody!
Comment 30 Martin Renvoize 2018-06-05 11:49:10 UTC
This patch has been pushed to 18.05.x and will be in 18.05.01

Nice to see this bug squashed.
Comment 31 Fridolin SOMERS 2018-06-07 08:00:14 UTC
Pushed to 17.11.x for 17.11.07