From 0caf2b9f555a5d6a4a54273a9a43b717ab637fd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joonas=20Kylm=C3=A4l=C3=A4?= Date: Wed, 17 Feb 2021 14:04:47 +0200 Subject: [PATCH] Bug 25690: Remove double usage of 'Reserved' return value The patch "Bug 19116: Hold not set to waiting after transfer" added a new meaning to 'Reserved' return value of C4::Reserves::CheckReserves function. Let's remove double usage and have separate Transferred return value so we can differentiate between attached and non-attached holds. This will come useful in future refactorings. This patch does no changes to the logic except in the /cgi-bin/koha/circ/branchtransfers.pl and circulation.pl we now give similarly to waiting state notice about hold being transferred. To test: 1) Apply this patch 2) Create a new item level hold so that pickup library is different than where the item is currently. Then return the item so that hold is being attached and transferred. 3) Go to branchtransfers.pl and try to create a new transfer: it should prompt you with message "Item is attached to a hold and being transferred for XXX" and provide you with option to cancel the hold or to ignore the transfer. --- C4/Circulation.pm | 15 +++++++++++++ C4/Reserves.pm | 14 ++++++------ C4/SIP/ILS/Transaction/Checkout.pm | 2 +- circ/branchtransfers.pl | 7 ++++++ circ/circulation.pl | 2 +- .../prog/en/modules/circ/branchtransfers.tt | 7 ++++-- .../prog/en/modules/circ/circulation.tt | 25 ++++++++++++---------- .../en/modules/circ/circulation_batch_checkouts.tt | 5 ++++- t/db_dependent/Circulation.t | 2 +- t/db_dependent/Reserves.t | 2 +- 10 files changed, 55 insertions(+), 26 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 5b5a0d2c23..ffbb4f1bea 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -719,6 +719,10 @@ issued to someone else. reserved for someone else. +=head3 TRANSFERRED + +reserved and being transferred for someone else. + =head3 INVALID_DATE sticky due date is invalid or due date in the past @@ -1131,6 +1135,17 @@ sub CanBookBeIssued { $needsconfirmation{'resreservedate'} = $res->{reservedate}; $needsconfirmation{'reserve_id'} = $res->{reserve_id}; } + elsif ( $restype eq "Transferred" ) { + # The item is determined hold being transferred for someone else. + $needsconfirmation{TRANSFERRED} = 1; + $needsconfirmation{'resfirstname'} = $patron->firstname; + $needsconfirmation{'ressurname'} = $patron->surname; + $needsconfirmation{'rescardnumber'} = $patron->cardnumber; + $needsconfirmation{'resborrowernumber'} = $patron->borrowernumber; + $needsconfirmation{'resbranchcode'} = $patron->branchcode; + $needsconfirmation{'resreservedate'} = $res->{reservedate}; + $needsconfirmation{'reserve_id'} = $res->{reserve_id}; + } } } } diff --git a/C4/Reserves.pm b/C4/Reserves.pm index a658646407..373771e08d 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -853,14 +853,12 @@ sub CheckReserves { my $priority = 10000000; foreach my $res (@reserves) { - if ( $res->{'itemnumber'} && $res->{'itemnumber'} == $itemnumber && $res->{'priority'} == 0) { - if ($res->{'found'} eq 'W') { - return ( "Waiting", $res, \@reserves ); # Found it, it is waiting - } elsif ($res->{'found'} eq 'P') { - return ( "Processing", $res, \@reserves ); # Found determinated hold, e. g. the transferred one - } else { - return ( "Reserved", $res, \@reserves ); # Found determinated hold, e. g. the transferred one - } + if ($res->{'found'} eq 'W') { + return ( "Waiting", $res, \@reserves ); # Found it, it is waiting + } elsif ($res->{'found'} eq 'P') { + return ( "Processing", $res, \@reserves ); # Found determinated hold, e. g. the transferred one + } elsif ($res->{'found'} eq 'T') { + return ( "Transferred", $res, \@reserves ); # Found determinated hold, e. g. the transferred one } else { my $patron; my $item; diff --git a/C4/SIP/ILS/Transaction/Checkout.pm b/C4/SIP/ILS/Transaction/Checkout.pm index aabe5e6830..57a3c97307 100644 --- a/C4/SIP/ILS/Transaction/Checkout.pm +++ b/C4/SIP/ILS/Transaction/Checkout.pm @@ -69,7 +69,7 @@ sub do_checkout { foreach my $confirmation (keys %{$needsconfirmation}) { if ($confirmation eq 'RENEW_ISSUE'){ $self->screen_msg("Item already checked out to you: renewing item."); - } elsif ($confirmation eq 'RESERVED' or $confirmation eq 'RESERVE_WAITING') { + } elsif ($confirmation eq 'RESERVED' or $confirmation eq 'RESERVE_WAITING' or $confirmation eq 'TRANSFERRED') { my $x = $self->{item}->available($patron->borrowernumber); if ($x) { $self->screen_msg("Item was reserved for you."); diff --git a/circ/branchtransfers.pl b/circ/branchtransfers.pl index 14ee57eca1..a63d7453cf 100755 --- a/circ/branchtransfers.pl +++ b/circ/branchtransfers.pl @@ -73,6 +73,7 @@ my $messages; my $found; my $reserved; my $waiting; +my $hold_transferred; my $reqmessage; my $cancelled; my $setwaiting; @@ -176,6 +177,11 @@ if ($found) { if ( $res->{'ResFound'} eq "Waiting" ) { $waiting = 1; } + + if ( $res->{'ResFound'} eq "Transferred" ) { + $hold_transferred = 1; + } + elsif ( $res->{'ResFound'} eq "Reserved" ) { $reserved = 1; $biblionumber = $res->{'biblionumber'}; @@ -218,6 +224,7 @@ $template->param( found => $found, reserved => $reserved, waiting => $waiting, + transferred => $hold_transferred, borrowernumber => $borrowernumber, itemnumber => $itemnumber, barcode => $barcode, diff --git a/circ/circulation.pl b/circ/circulation.pl index ee3878ca13..9923f42f74 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -422,7 +422,7 @@ if (@$barcodes) { } } - if ($question->{RESERVE_WAITING} or $question->{RESERVED}){ + if ($question->{RESERVE_WAITING} or $question->{RESERVED} or $question->{TRANSFERRED}){ $template->param( reserveborrowernumber => $question->{'resborrowernumber'}, reserve_id => $question->{reserve_id}, diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/branchtransfers.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/branchtransfers.tt index b29fac2c09..9b563c86b8 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/branchtransfers.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/branchtransfers.tt @@ -31,11 +31,14 @@ [% IF ( waiting ) %] Item is marked waiting at [% branchname | html %] for [% name | html %] ([% borrowernumber | html %]). [% END %] + [% IF ( transferred ) %] + Item is attached to a hold and being transferred for [% name | html %] ([% borrowernumber | html %]). + [% END %] [% IF ( reserved ) %]Set reserve to waiting and transfer book to [% branchname | html %]: [% END %] - [% IF ( waiting ) %]Cancel reservation and then attempt transfer: [% END %] + [% IF ( waiting or transferred ) %]Cancel reservation and then attempt transfer: [% END %]
@@ -47,7 +50,7 @@ - [% IF ( waiting ) %] + [% IF ( waiting or transferred ) %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt index a4f7369bb2..f91ef45cf1 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt @@ -122,6 +122,10 @@
  • Item [% getTitleMessageIteminfo | html %] ([% getBarcodeMessageIteminfo | html %]) has been waiting for [% resfirstname | html %] [% ressurname | html %] ([% rescardnumber | html %]) at [% Branches.GetName( resbranchcode ) | html %] since [% reswaitingdate | $KohaDates %]
  • [% END %] + [% IF ( TRANSFERRED ) %] +
  • Item [% getTitleMessageIteminfo | html %] ([% getBarcodeMessageIteminfo | html %]) is on hold for [% resfirstname | html %] [% ressurname | html %] ([% rescardnumber | html %]) and being transferred to [% Branches.GetName( resbranchcode ) | html %]
  • + [% END %] + [% IF ( RESERVED ) %]
  • Item [% getTitleMessageIteminfo | html %] ([% getBarcodeMessageIteminfo | html %]) has been on hold for [% resfirstname | html %] [% ressurname | html %] ([% rescardnumber | html %]) at [% Branches.GetName( resbranchcode ) | html %] since [% resreservedate | $KohaDates %]
  • [% END %] @@ -237,6 +241,15 @@

    [% END %] + [% IF ( TRANSFERRED ) %] +

    + +
    + + +

    + [% END %] + @@ -267,17 +280,7 @@
    [% END # /IF CAN_user_circulate_force_checkout or HIGHHOLDS %] - [% IF ( RESERVED ) %] -
    - - - - - -
    - [% END %] - - [% IF ( RESERVE_WAITING ) %] + [% IF ( RESERVED or RESERVE_WAITING or TRANSFERRED ) %]
    diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation_batch_checkouts.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation_batch_checkouts.tt index 6793b95c91..c21b2e79ae 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation_batch_checkouts.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation_batch_checkouts.tt @@ -147,6 +147,9 @@ [% IF checkout_info.RESERVE_WAITING %]
  • This item is waiting for another patron.
  • [% END %] + [% IF checkout_info.TRANSFERRED %] +
  • This item is on hold and being transferred to another patron.
  • + [% END %] [% IF checkout_info.ISSUED_TO_ANOTHER %]
  • This item is checked out to another patron. [% IF CAN_user_circulate_force_checkout %] @@ -187,7 +190,7 @@ [% END %] - [% IF NOT checkout_info.IMPOSSIBLE && ( CAN_user_circulate_force_checkout || checkout_info.HIGHHOLDS ) && ( checkout_info.RESERVED || checkout_info.RESERVE_WAITING ) %] + [% IF NOT checkout_info.IMPOSSIBLE && ( CAN_user_circulate_force_checkout || checkout_info.HIGHHOLDS ) && ( checkout_info.RESERVED || checkout_info.RESERVE_WAITING || checkout_info.TRANSFERRED ) %]
  • This item is on hold for another patron. The hold will be overridden, but not cancelled.
  • [% ELSIF checkout_info.RESERVED %]
  • This item is on hold for another patron.
  • diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 97ca6492e2..17ab5b92a6 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -3364,7 +3364,7 @@ subtest 'Set waiting flag' => sub { is( $hold->found, 'T', 'Hold is in transit' ); my ( $status ) = CheckReserves($item->itemnumber); - is( $status, 'Reserved', 'Hold is not waiting yet'); + is( $status, 'Transferred', 'Hold is not waiting yet'); set_userenv( $library_2 ); $do_transfer = 0; diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 4faab3c218..a79cd0c458 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -1146,7 +1146,7 @@ subtest 'CheckReserves additional test' => sub { my ( $status, $matched_reserve, $possible_reserves ) = CheckReserves( $item->itemnumber ); - is( $status, 'Reserved', "We found a reserve" ); + is( $status, 'Transferred', "We found a reserve" ); is( $matched_reserve->{reserve_id}, $reserve1->reserve_id, "We got the Transit reserve" ); is( scalar @$possible_reserves, 2, 'We do get both reserves' ); -- 2.11.0