From 06543fab72053442d3ba80740401c090c7d59b4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joonas=20Kylm=C3=A4l=C3=A4?= Date: Wed, 22 Jul 2020 18:45:36 +0300 Subject: [PATCH] Bug 12556: Add new "in processing" state to holds This adds new syspref, HoldsNeedProcessingSIP, which controls whether a hold that is related to item will be filled automatically or not. If the user has enabled the syspref then instead of fulfilling the hold automatically the hold will go to "in processing" state. To test: 1. Checkout a book to patron A 2. Place a bib level hold to the book for B 3. Patron A returns the book via SIP, to simulate this use: ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --item -m checkin 4. Notice that no notification is generated for Patron B about hold and that the hold status in intranet and opac is "In Processing". 5. Notice that patron A (or other patrons) cannot checkout a book that is in processing, because it is considered to be attached to the holdee (similarly to the waiting state): ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --patron --item -m checkout Signed-off-by: John Doe --- C4/Reserves.pm | 25 ++++++++++------ C4/RotatingCollections.pm | 2 +- C4/SIP/ILS/Item.pm | 24 +++++++-------- C4/SIP/ILS/Transaction/Checkout.pm | 6 ++-- Koha/Hold.pm | 35 ++++++++++++++++++++-- circ/returns.pl | 28 ++++++++--------- installer/data/mysql/atomicupdate/bug_12556.perl | 8 +++++ installer/data/mysql/sysprefs.sql | 1 + .../intranet-tmpl/prog/en/includes/holds_table.inc | 10 +++---- .../en/modules/admin/preferences/circulation.pref | 6 ++++ .../prog/en/modules/members/holdshistory.tt | 2 ++ .../bootstrap/en/includes/holds-table.inc | 4 ++- reserve/request.pl | 1 + t/db_dependent/Hold.t | 20 +++++++++++-- 14 files changed, 121 insertions(+), 51 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_12556.perl diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 322002d66f..73e0f767c1 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -714,6 +714,7 @@ sub GetReserveStatus { if(defined $found) { return 'Waiting' if $found eq 'W' and $priority == 0; + return 'Processing' if $found eq 'P'; return 'Finished' if $found eq 'F'; } @@ -828,7 +829,9 @@ sub CheckReserves { if ( $res->{'itemnumber'} && $res->{'itemnumber'} == $itemnumber && $res->{'priority'} == 0) { if ($res->{'found'} eq 'W') { return ( "Waiting", $res, \@reserves ); # Found it, it is waiting - } else { + } elsif ($res->{'found'} eq 'P') { + return ( "Processing", $res, \@reserves ); # Found determinated hold, e. g. the tranferred one + } else { return ( "Reserved", $res, \@reserves ); # Found determinated hold, e. g. the tranferred one } } else { @@ -1159,7 +1162,13 @@ sub ModReserveAffect { $hold->itemnumber($itemnumber); - if( !$transferToDo ){ + if ($transferToDo) { + $hold->set_transfer(); + } elsif (C4::Context->preference('HoldsNeedProcessingSIP') + && C4::Context->interface eq 'sip' + && !$already_on_shelf) { + $hold->set_processing(); + } else { $hold->set_waiting(); _koha_notify_reserve( $hold->reserve_id ) unless $already_on_shelf; my $transfers = Koha::Item::Transfers->search({ @@ -1169,9 +1178,7 @@ sub ModReserveAffect { while( my $transfer = $transfers->next ){ $transfer->datearrived( dt_from_string() )->store; }; - } else { - $hold->set_transfer(); - } + } _FixPriority( { biblionumber => $biblionumber } ); @@ -1550,7 +1557,7 @@ sub _FixPriority { UPDATE reserves SET priority = 0 WHERE reserve_id = ? - AND found IN ('W', 'T') + AND found IN ('W', 'T', 'P') "; my $sth = $dbh->prepare($query); $sth->execute( $reserve_id ); @@ -1562,7 +1569,7 @@ sub _FixPriority { SELECT reserve_id, borrowernumber, reservedate FROM reserves WHERE biblionumber = ? - AND ((found <> 'W' AND found <> 'T') OR found IS NULL) + AND ((found <> 'W' AND found <> 'T' AND found <> 'P') OR found IS NULL) ORDER BY priority ASC "; my $sth = $dbh->prepare($query); @@ -1977,7 +1984,7 @@ sub MergeHolds { "UPDATE reserves SET priority = ? WHERE biblionumber = ? AND borrowernumber = ? AND reservedate = ? AND (itemnumber = ? or itemnumber is NULL) " ); - $sth->execute( $to_biblio, 'W', 'T' ); + $sth->execute( $to_biblio, 'W', 'T', 'P' ); my $priority = 1; while ( my $reserve = $sth->fetchrow_hashref() ) { $upd_sth->execute( @@ -2147,7 +2154,7 @@ sub CalculatePriority { AND priority > 0 AND (found IS NULL OR found = '') }; - #skip found==W or found==T (waiting or transit holds) + #skip found==W or found==T or found==P (waiting, transit or processing holds) if( $resdate ) { $sql.= ' AND ( reservedate <= ? )'; } diff --git a/C4/RotatingCollections.pm b/C4/RotatingCollections.pm index c41d5780e9..954c2d419b 100644 --- a/C4/RotatingCollections.pm +++ b/C4/RotatingCollections.pm @@ -452,7 +452,7 @@ sub TransferCollection { barcode => $item->{barcode}, ignore_reserves => 1, trigger => 'RotatingCollection' - }) unless ( $status eq 'Waiting' || @transfers ); + }) unless ( $status eq 'Waiting' || $status eq 'Processing' || @transfers ); } return 1; diff --git a/C4/SIP/ILS/Item.pm b/C4/SIP/ILS/Item.pm index 056c214be4..651fbc338a 100644 --- a/C4/SIP/ILS/Item.pm +++ b/C4/SIP/ILS/Item.pm @@ -106,8 +106,8 @@ sub new { my $biblio = Koha::Biblios->find( $self->{biblionumber} ); my $holds = $biblio->current_holds->unblessed; $self->{hold_queue} = $holds; - $self->{hold_shelf} = [( grep { defined $_->{found} and $_->{found} eq 'W' } @{$self->{hold_queue}} )]; - $self->{pending_queue} = [( grep {(! defined $_->{found}) or $_->{found} ne 'W' } @{$self->{hold_queue}} )]; + $self->{hold_attached} = [( grep { defined $_->{found} and ( $_->{found} eq 'W' or $_->{found} eq 'P' ) } @{$self->{hold_queue}} )]; + $self->{pending_queue} = [( grep {(! defined $_->{found}) or ( $_->{found} ne 'W' and $_->{found} ne 'P' ) } @{$self->{hold_queue}} )]; $self->{title} = $biblio->title; $self->{author} = $biblio->author; bless $self, $type; @@ -144,8 +144,8 @@ my %fields = ( sub next_hold { my $self = shift; - # use Data::Dumper; warn "next_hold() hold_shelf: " . Dumper($self->{hold_shelf}); warn "next_hold() pending_queue: " . $self->{pending_queue}; - foreach (@{$self->hold_shelf}) { # If this item was taken from the hold shelf, then that reserve still governs + # use Data::Dumper; warn "next_hold() hold_attached: " . Dumper($self->{hold_attached}); warn "next_hold() pending_queue: " . $self->{pending_queue}; + foreach (@{$self->hold_attached}) { # If this item was taken from the hold shelf, then that reserve still governs next unless ($_->{itemnumber} and $_->{itemnumber} == $self->{itemnumber}); return $_; } @@ -271,7 +271,7 @@ sub sip_circulation_status { elsif ( $self->{borrowernumber} ) { return '04'; # charged } - elsif ( grep { $_->{itemnumber} == $self->{itemnumber} } @{ $self->{hold_shelf} } ) { + elsif ( grep { $_->{itemnumber} == $self->{itemnumber} } @{ $self->{hold_attached} } ) { return '08'; # waiting on hold shelf } else { @@ -310,10 +310,10 @@ sub pending_queue { (defined $self->{pending_queue}) or return []; return $self->{pending_queue}; } -sub hold_shelf { +sub hold_attached { my $self = shift; - (defined $self->{hold_shelf}) or return []; - return $self->{hold_shelf}; + (defined $self->{hold_attached}) or return []; + return $self->{hold_attached}; } sub hold_queue_position { @@ -355,7 +355,7 @@ sub hold_pickup_date { # AND no pending (i.e. non-W) hold queue # OR # 2) not checked out -# AND (not on hold_shelf OR is on hold_shelf for patron) +# AND (not on hold_attached OR is on hold_attached for patron) # # What this means is we are consciously allowing the patron to checkout (but not renew) an item that DOES # have non-W holds on it, but has not been "picked" from the stacks. That is to say, the @@ -367,13 +367,13 @@ sub hold_pickup_date { sub available { my ($self, $for_patron) = @_; my $count = (defined $self->{pending_queue}) ? scalar @{$self->{pending_queue}} : 0; - my $count2 = (defined $self->{hold_shelf} ) ? scalar @{$self->{hold_shelf} } : 0; - $debug and print STDERR "availability check: pending_queue size $count, hold_shelf size $count2\n"; + my $count2 = (defined $self->{hold_attached} ) ? scalar @{$self->{hold_attached} } : 0; + $debug and print STDERR "availability check: pending_queue size $count, hold_attached size $count2\n"; if (defined($self->{borrowernumber})) { ($self->{borrowernumber} eq $for_patron) or return 0; return ($count ? 0 : 1); } else { # not checked out - ($count2) and return $self->barcode_is_borrowernumber($for_patron, $self->{hold_shelf}[0]->{borrowernumber}); + ($count2) and return $self->barcode_is_borrowernumber($for_patron, $self->{hold_attached}[0]->{borrowernumber}); } return 0; } diff --git a/C4/SIP/ILS/Transaction/Checkout.pm b/C4/SIP/ILS/Transaction/Checkout.pm index 01aa8fbc60..a34f348b01 100644 --- a/C4/SIP/ILS/Transaction/Checkout.pm +++ b/C4/SIP/ILS/Transaction/Checkout.pm @@ -48,7 +48,7 @@ sub new { sub do_checkout { my $self = shift; siplog('LOG_DEBUG', "ILS::Transaction::Checkout performing checkout..."); - my $shelf = $self->{item}->hold_shelf; + my $shelf = $self->{item}->hold_attached; my $barcode = $self->{item}->id; my $patron = Koha::Patrons->find($self->{patron}->{borrowernumber}); my $overridden_duedate; # usually passed as undef to AddIssue @@ -111,8 +111,8 @@ sub do_checkout { ($_->{itemnumber} eq $itemnumber) or next; # skip it if not this item ($_->{borrowernumber} == $patron->borrowernumber) and last; # if item was waiting for this patron, we're done. AddIssue takes care of the "W" hold. - $debug and warn "Item is on hold shelf for another patron."; - $self->screen_msg("Item is on hold shelf for another patron."); + $debug and warn "Item is on hold for another patron."; + $self->screen_msg("Item is on hold for another patron."); $noerror = 0; } my ($fee, undef) = GetIssuingCharges($itemnumber, $patron->borrowernumber); diff --git a/Koha/Hold.pm b/Koha/Hold.pm index 845028da97..7375a8fdca 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -96,6 +96,9 @@ sub suspend_hold { elsif ( $self->is_in_transit ) { Koha::Exceptions::Hold::CannotSuspendFound->throw( status => 'T' ); } + elsif ( $self->is_in_processing ) { + Koha::Exceptions::Hold::CannotSuspendFound->throw( status => 'P' ); + } else { Koha::Exceptions::Hold::CannotSuspendFound->throw( 'Unhandled data exception on found hold (id=' @@ -217,9 +220,23 @@ sub set_waiting { return $self; } +=head3 set_processing + +=cut + +sub set_processing { + my ( $self ) = @_; + + $self->priority(0); + $self->found('P'); + $self->store(); + + return $self; +} + =head3 is_found -Returns true if hold is a waiting or in transit +Returns true if hold is waiting, in transit or in processing =cut @@ -229,6 +246,7 @@ sub is_found { return 0 unless $self->found(); return 1 if $self->found() eq 'W'; return 1 if $self->found() eq 'T'; + return 1 if $self->found() eq 'P'; } =head3 is_waiting @@ -257,6 +275,19 @@ sub is_in_transit { return $self->found() eq 'T'; } +=head3 is_in_processing + +Returns true if hold is a in_processing hold + +=cut + +sub is_in_processing { + my ($self) = @_; + + return 0 unless $self->found(); + return $self->found() eq 'P'; +} + =head3 is_cancelable_from_opac Returns true if hold is a cancelable hold @@ -271,7 +302,7 @@ sub is_cancelable_from_opac { my ($self) = @_; return 1 unless $self->is_found(); - return 0; # if ->is_in_transit or if ->is_waiting + return 0; # if ->is_in_transit or if ->is_waiting or ->is_in_processing } =head3 is_at_destination diff --git a/circ/returns.pl b/circ/returns.pl index a7556c7b07..8b6585c993 100755 --- a/circ/returns.pl +++ b/circ/returns.pl @@ -433,22 +433,18 @@ if ( $messages->{'ResFound'}) { diffbranch => 1, ); } - } - elsif ( $reserve->{'ResFound'} eq "Waiting" or $reserve->{'ResFound'} eq "Reserved" ) { - if ( $reserve->{'ResFound'} eq "Waiting" ) { - $template->param( - waiting => $branchCheck ? 1 : undef, - ); - } elsif ( $reserve->{'ResFound'} eq "Reserved" ) { - $template->param( - intransit => $branchCheck ? undef : 1, - transfertodo => $branchCheck ? undef : 1, - reserve_id => $reserve->{reserve_id}, - reserved => 1, - itemnumber => $itemnumber, - ); - } - + } elsif ( $reserve->{'ResFound'} eq "Waiting" ) { + $template->param( + waiting => $branchCheck ? 1 : undef, + ); + } elsif ( $reserve->{'ResFound'} eq "Reserved" || $reserve->{'ResFound'} eq "Processing" ) { + $template->param( + intransit => $branchCheck ? undef : 1, + transfertodo => $branchCheck ? undef : 1, + reserve_id => $reserve->{reserve_id}, + reserved => 1, + itemnumber => $itemnumber, + ); } # else { ; } # error? # same params for Waiting or Reserved diff --git a/installer/data/mysql/atomicupdate/bug_12556.perl b/installer/data/mysql/atomicupdate/bug_12556.perl new file mode 100644 index 0000000000..06cae68b96 --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_12556.perl @@ -0,0 +1,8 @@ +$DBversion = 'XXX'; # will be replaced by the RM +if( CheckVersion( $DBversion ) ) { + $dbh->do(q{ + INSERT IGNORE INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `type` ) VALUES + ('HoldsNeedProcessingSIP', '0', NULL, 'Require staff to check-in before hold is set to waiting state', 'YesNo' ) + }); + NewVersion( $DBversion, 12556, "Add new syspref HoldsNeedProcessingSIP"); +} diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql index 1df6a1fb53..a60e022035 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/installer/data/mysql/sysprefs.sql @@ -228,6 +228,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('HoldsAutoFill','0',NULL,'If on, librarian will not be asked if hold should be filled, it will be filled automatically','YesNo'), ('HoldsAutoFillPrintSlip','0',NULL,'If on, hold slip print dialog will be displayed automatically','YesNo'), ('HoldsLog','0',NULL,'If ON, log create/cancel/suspend/resume actions on holds.','YesNo'), +('HoldsNeedProcessingSIP', '0', NULL, 'Require staff to check-in before hold is set to waiting state', 'YesNo' ), ('HoldsQueueSkipClosed', '0', NULL, 'If enabled, any libraries that are closed when the holds queue is built will be ignored for the purpose of filling holds.', 'YesNo'), ('HoldsSplitQueue','nothing','nothing|branch|itemtype|branch_itemtype','In the staff interface, split the holds view by the given criteria','Choice'), ('HoldsToPullStartDate','2',NULL,'Set the default start date for the Holds to pull list to this many days ago','Integer'), diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc index febcf4e1a7..5fef382ee6 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc @@ -41,6 +41,8 @@ [% IF ( hold.found ) %] [% IF ( hold.intransit ) %] + [% ELSIF (hold.inprocessing) %] + [% ELSE %] [% END %] @@ -120,13 +122,11 @@ [% IF ( hold.found ) %] [% IF ( hold.atdestination ) %] - [% IF ( hold.found ) %] Item waiting at [% hold.wbrname | html %] since [% hold.waiting_date | $KohaDates %] - [% ELSE %] - Waiting to be pulled - [% END %] - [% ELSE %] + [% ELSIF (hold.intransit) %] Item being transferred to [% hold.wbrname | html %] + [% ELSIF (hold.inprocessing) %] + Item being processed at [% hold.wbrname | html %] [% END %] [% ELSE %] [% IF Koha.Preference('IndependentBranches') && Branches.all().size == 1 %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref index 9e6dbe3d4f..d128e0143b 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref @@ -545,6 +545,12 @@ Circulation: no: "Don't" - automatically fill holds instead of asking the librarian. - + - pref: HoldsNeedProcessingSIP + choices: + yes: "Don't fulfill" + no: Fulfill + - holds automatically if matching item is returned via SIP protocol. + - - pref: HoldsAutoFillPrintSlip choices: yes: Do diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/holdshistory.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/holdshistory.tt index c0d574a763..bb9b84d5da 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/holdshistory.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/holdshistory.tt @@ -102,6 +102,8 @@ [% END %] [% ELSIF hold.found == 'W' %] Waiting + [% ELSIF hold.found == 'P' %] + Processing [% ELSIF hold.found == 'T' %] In transit [% ELSE %] diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/includes/holds-table.inc b/koha-tmpl/opac-tmpl/bootstrap/en/includes/holds-table.inc index 332480072e..3675569068 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/includes/holds-table.inc +++ b/koha-tmpl/opac-tmpl/bootstrap/en/includes/holds-table.inc @@ -34,7 +34,7 @@ [% SET all_holds_waiting = 1 %] [% FOREACH HOLD IN HOLDS %] - [% UNLESS ( HOLD.is_waiting || HOLD.is_in_transit ) %] + [% UNLESS ( HOLD.is_waiting || HOLD.is_in_transit || HOLD.is_in_processing) %] [% SET all_holds_waiting = 0 %] [% END %] [% IF ( HOLD.is_at_destination ) %] @@ -114,6 +114,8 @@ [% SET transfer = HOLD.item.get_transfer %] Item in transit from [% Branches.GetName( transfer.frombranch ) | html %] since [% transfer.datesent | $KohaDates %] + [% ELSIF ( HOLD.is_in_processing ) %] + Item in processing [% ELSIF ( HOLD.suspend ) %] Suspended [% IF ( HOLD.suspend_until ) %] until [% HOLD.suspend_until | $KohaDates %] [% END %] [% ELSE %] diff --git a/reserve/request.pl b/reserve/request.pl index 3de92dd848..bff61851d2 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -665,6 +665,7 @@ foreach my $biblionumber (@biblionumbers) { $reserve{'wbrname'} = $res->branch()->branchname(); $reserve{'atdestination'} = $res->is_at_destination(); $reserve{'found'} = $res->is_found(); + $reserve{'inprocessing'} = $res->is_in_processing(); $reserve{'intransit'} = $res->is_in_transit(); } elsif ( $res->priority() > 0 ) { diff --git a/t/db_dependent/Hold.t b/t/db_dependent/Hold.t index d58b0a243c..5b71b64292 100755 --- a/t/db_dependent/Hold.t +++ b/t/db_dependent/Hold.t @@ -29,7 +29,7 @@ use Koha::Item; use Koha::DateUtils; use t::lib::TestBuilder; -use Test::More tests => 29; +use Test::More tests => 33; use Test::Exception; use Test::Warn; @@ -112,10 +112,15 @@ isnt( $hold->is_waiting, 1, 'The hold is not waiting (T)' ); is( $hold->is_found, 1, 'The hold is found'); is( $hold->is_in_transit, 1, 'The hold is in transit' ); +$hold->found('P'); +is( $hold->is_found, 1, 'The hold is found'); +is( $hold->is_in_processing, 1, 'The hold is in processing' ); + $hold->found(q{}); isnt( $hold->is_waiting, 1, 'The hold is not waiting (W)' ); is( $hold->is_found, 0, 'The hold is not found' ); ok( !$hold->is_in_transit, 'The hold is not in transit' ); +ok( !$hold->is_in_processing, 'The hold is not in processing' ); # Test method is_cancelable_from_opac $hold->found(undef); @@ -124,6 +129,8 @@ $hold->found('W'); is( $hold->is_cancelable_from_opac, 0, "Waiting hold is not cancelable" ); $hold->found('T'); is( $hold->is_cancelable_from_opac, 0, "In transit hold is not cancelable" ); +$hold->found('P'); +is( $hold->is_cancelable_from_opac, 0, "In processing hold is not cancelable" ); # Test method is_at_destination $hold->found(undef); @@ -178,7 +185,7 @@ subtest "delete() tests" => sub { subtest 'suspend() tests' => sub { - plan tests => 16; + plan tests => 18; $schema->storage->txn_begin; @@ -232,6 +239,15 @@ subtest 'suspend() tests' => sub { is( $@->status, 'T', 'Exception gets the \'status\' parameter set correctly' ); + # set hold found=T + $hold->set_processing(); + throws_ok + { $hold->suspend_hold; } + 'Koha::Exceptions::Hold::CannotSuspendFound', + 'Exception is thrown when a found hold is tried to suspend'; + + is( $@->status, 'P', 'Exception gets the \'status\' parameter set correctly' ); + my $holds_module = Test::MockModule->new('Koha::Hold'); $holds_module->mock( 'is_found', 1 ); -- 2.11.0