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. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> (cherry picked from commit fbef547832) Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
2021-02-17 14:04:47 +02:00 · 2021-02-17 14:04:47 +02:00 · e07c676e43
parent 138aa35c09
commit e07c676e43
10 changed files with 55 additions and 26 deletions
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@ -717,6 +717,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
@ -1129,6 +1133,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};
+                }
            }
        }
    }
--- 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;
--- a/C4/SIP/ILS/Transaction/Checkout.pm
+++ b/C4/SIP/ILS/Transaction/Checkout.pm
@ -71,7 +71,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.");
--- 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,
--- 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},
--- 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 %] (<a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% borrowernumber | uri %]">[% borrowernumber | html %]</a>).
            [% END %]
+            [% IF ( transferred ) %]
+                Item is attached to a hold and being transferred for [% name | html %] (<a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% borrowernumber | uri %]">[% borrowernumber | html %]</a>).
+            [% END %]
        </caption>
        <tr>
            <th>
                [% 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 %]
            </th>
            <td>
                <form method="post" name="mainform" id="mainform" action="branchtransfers.pl">
@ -47,7 +50,7 @@
                    <input type="hidden" name="itemnumber" value="[% itemnumber | html %]" />
                    <input type="hidden" name="borrowernumber" value="[% borrowernumber | html %]" />
                    <input type="hidden" name="tobranchcd" value="[% tobranchcd | html %]" />
-                    [% IF ( waiting ) %]
+                    [% IF ( waiting or transferred ) %]
                        <input type="hidden" name="barcode" value="[% barcode | html %]" />
                        <input type="hidden" name="request" value="KillWaiting" />
                        <input type="submit" value="Cancel" />
--- 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 @@
                                    <li>Item <em>[% getTitleMessageIteminfo | html %]</em> ([% getBarcodeMessageIteminfo | html %]) has been waiting for <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% resborrowernumber | uri %]">[% resfirstname | html %] [% ressurname | html %]</a> ([% rescardnumber | html %]) at [% Branches.GetName( resbranchcode ) | html %] since [% reswaitingdate | $KohaDates %]</li>
                                [% END %]

+                                [% IF ( TRANSFERRED ) %]
+                                    <li>Item <em>[% getTitleMessageIteminfo | html %]</em> ([% getBarcodeMessageIteminfo | html %]) is on hold for <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% resborrowernumber | uri %]">[% resfirstname | html %] [% ressurname | html %]</a> ([% rescardnumber | html %]) and being transferred to [% Branches.GetName( resbranchcode ) | html %]</li>
+                                [% END %]
+
                                [% IF ( RESERVED ) %]
                                    <li>Item <em>[% getTitleMessageIteminfo | html %]</em> ([% getBarcodeMessageIteminfo | html %]) has been on hold for <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% resborrowernumber | uri %]">[% resfirstname | html %] [% ressurname | html %]</a> ([% rescardnumber | html %]) at [% Branches.GetName( resbranchcode ) | html %] since [% resreservedate | $KohaDates %]</li>
                                [% END %]
@ -237,6 +241,15 @@
                                        </p>
                                    [% END %]

+                                    [% IF ( TRANSFERRED ) %]
+                                        <p>
+                                            <label for="cancelreserve">Cancel hold</label>
+                                            <input type="radio" value="cancel" name="cancelreserve" id="cancelreserve" /><br />
+                                            <label for="revertreserve">Revert hold transfer status</label>
+                                            <input type="radio" value="revert" name="cancelreserve" id="revertreserve" checked="checked"/>
+                                        </p>
+                                    [% END %]
+
                                    <input type="hidden" name="barcode" value="[% barcode | html %]" />
                                    <input type="hidden" name="borrowernumber" value="[% patron.borrowernumber | html %]" />
                                    <input type="hidden" name="issueconfirmed" value="1" />
@ -267,17 +280,7 @@
                                </form>
                            [% END # /IF CAN_user_circulate_force_checkout or HIGHHOLDS %]

-                            [% IF ( RESERVED ) %]
-                                <form method="get" action="/cgi-bin/koha/circ/circulation.pl">
-                                    <input type="hidden" name="restoreduedatespec" />
-                                    <input type="hidden" name="borrowernumber" value="[% patron.borrowernumber | html %]" />
-                                    <input type="hidden" name="duedatespec" value="[% duedatespec | html %]" />
-                                    <input type="hidden" name="stickyduedate" value="[% stickyduedate | html %]" />
-                                    <button class="print" type="submit" onclick="Dopop('hold-transfer-slip.pl?reserve_id=[% reserve_id | uri %]');this.form.submit();"><i class="fa fa-print"></i> Don't check out and print slip (P)</button>
-                                </form>
-                            [% END %]
-
-                            [% IF ( RESERVE_WAITING ) %]
+                            [% IF ( RESERVED or RESERVE_WAITING or TRANSFERRED ) %]
                                <form method="get" action="/cgi-bin/koha/circ/circulation.pl">
                                    <input type="hidden" name="restoreduedatespec" />
                                    <input type="hidden" name="borrowernumber" value="[% patron.borrowernumber | html %]" />
--- 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 %]
                      <li><i class="fa fa-li fa-warning"></i>This item is waiting for another patron.</li>
                  [% END %]
+                  [% IF checkout_info.TRANSFERRED %]
+                      <li><i class="fa fa-li fa-warning"></i>This item is on hold and being transferred to another patron.</li>
+                  [% END %]
                  [% IF checkout_info.ISSUED_TO_ANOTHER %]
                      <li><i class="fa fa-li fa-warning"></i>This item is checked out to another patron.
                    [% IF CAN_user_circulate_force_checkout %]
@ -187,7 +190,7 @@
                      </script>
                  [% END %]

-                  [% IF NOT checkout_info.IMPOSSIBLE && ( CAN_user_circulate_force_checkout || checkout_info.HIGHHOLDS ) && ( checkout_info.RESERVED || checkout_info.RESERVE_WAITING ) %] <!-- arbitrary choice, revert the reserve is not possible-->
+                  [% IF NOT checkout_info.IMPOSSIBLE && ( CAN_user_circulate_force_checkout || checkout_info.HIGHHOLDS ) && ( checkout_info.RESERVED || checkout_info.RESERVE_WAITING || checkout_info.TRANSFERRED ) %] <!-- arbitrary choice, revert the reserve is not possible-->
                      <li><i class="fa fa-li fa-warning"></i>This item is on hold for another patron. The hold will be overridden, but not cancelled.</li>
                  [% ELSIF checkout_info.RESERVED %]
                      <li><i class="fa fa-li fa-warning"></i>This item is on hold for another patron.</li>
--- a/t/db_dependent/Circulation.t
+++ b/t/db_dependent/Circulation.t
@ -3347,7 +3347,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;
--- 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' );