From 9b386125d473244e4c2b76f1d0d330f8a121769f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joonas=20Kylm=C3=A4l=C3=A4?= Date: Tue, 16 Oct 2018 14:36:50 +0300 Subject: [PATCH] Bug 21572: Unify checkout policy of reserved items in SIP, SCO, and intranet Instead of having two different system preferences, AllowItemsOnHoldCheckout and AllowItemsOnHoldCheckoutSCO, now we have only the former one. The preference is also moved now in the C4::Circulation::CanBookBeIssued subroutine so that we have only one single truth about whether the book can be checked out. This patch changes the current checkout policy a bit. Without this patch patron could check out item in Waiting state with the SCO module but after this patch it is not possible anymore. Only checking out items in Reserved state is possible. This is how the SIP server already currently works so now these two would be unified. Allowing to checkout items in the Waiting state doesn't make any sense because it would defeat the purpose of holds, therefore the SIP server approach on only allowing checking out Reserved state items was chosen. The unwanted message "Item XXXXX / (yyyy) has been on hold for Joe (yyyy) at My Library since 15.10.2018" on intranet will be gone now after this patch if AllowItemsOnHoldCheckout is enabled. To test: Testing in SCO, SIP, Intranet with AllowItemsOnHoldCheckout set 'Don't allow': 1. Apply patch 2. Have a biblio record with one item A 3. Make a hold on item A for patron Y (bib level or item, doesn't matter) 4. Try to checkout item A for patron X 5. It should fail (in case of intranet a warning/confirmation dialog should come) Testing in SCO, SIP, Intranet with AllowItemsOnHoldCheckout set 'Allow': 1. Apply patch 2. Have a biblio record with one item A 3. Make a hold on item A for patron Y (bib level or item, doesn't matter) 4. Try to checkout item A for patron X 5. It should succeed (in case of intranet there should be no confirmation dialog) Testing in SCO, SIP, Intranet with AllowItemsOnHoldCheckout set 'Allow' or 'Don't allow': 1. Apply patch 2. Have a biblio record with one item A 3. Make a hold on item A for patron Y (bib level or item, doesn't matter) 4. Check in item A in intranet so that it goes to Waiting status 5. Try to checkout item A for patron X 6. It should fail (in case of intranet a warning/confirmation dialog should come) --- C4/Circulation.pm | 62 +++++++++---------- C4/SIP/ILS/Transaction/Checkout.pm | 8 +-- C4/UsageStats.pm | 1 - circ/circulation.pl | 1 - .../data/mysql/atomicupdate/bug_21572.perl | 15 +++++ installer/data/mysql/sysprefs.sql | 1 - .../admin/preferences/circulation.pref | 8 +-- opac/sco/sco-main.pl | 5 +- t/db_dependent/Circulation.t | 16 ++--- t/db_dependent/DecreaseLoanHighHolds.t | 2 +- t/db_dependent/UsageStats.t | 1 - 11 files changed, 59 insertions(+), 61 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_21572.perl diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 9c8d5ba13e..018aae0e13 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -556,7 +556,7 @@ sub TooMany { =head2 CanBookBeIssued ( $issuingimpossible, $needsconfirmation, [ $alerts ] ) = CanBookBeIssued( $patron, - $barcode, $duedate, $inprocess, $ignore_reserves, $params ); + $barcode, $duedate, $inprocess, $params ); Check if a book can be issued. @@ -576,8 +576,6 @@ data is keyed in lower case! =item C<$inprocess> boolean switch -=item C<$ignore_reserves> boolean switch - =item C<$params> Hashref of additional parameters Available keys: @@ -659,7 +657,7 @@ if the borrower borrows to much things =cut sub CanBookBeIssued { - my ( $patron, $barcode, $duedate, $inprocess, $ignore_reserves, $params ) = @_; + my ( $patron, $barcode, $duedate, $inprocess, $params ) = @_; my %needsconfirmation; # filled with problems that needs confirmations my %issuingimpossible; # filled with problems that causes the issue to be IMPOSSIBLE my %alerts; # filled with messages that shouldn't stop issuing, but the librarian should be aware of. @@ -980,35 +978,33 @@ sub CanBookBeIssued { } } - unless ( $ignore_reserves ) { - # See if the item is on reserve. - my ( $restype, $res ) = C4::Reserves::CheckReserves( $item->{'itemnumber'} ); - if ($restype) { - my $resbor = $res->{'borrowernumber'}; - if ( $resbor ne $patron->borrowernumber ) { - my $patron = Koha::Patrons->find( $resbor ); - if ( $restype eq "Waiting" ) - { - # The item is on reserve and waiting, but has been - # reserved by some other patron. - $needsconfirmation{RESERVE_WAITING} = 1; - $needsconfirmation{'resfirstname'} = $patron->firstname; - $needsconfirmation{'ressurname'} = $patron->surname; - $needsconfirmation{'rescardnumber'} = $patron->cardnumber; - $needsconfirmation{'resborrowernumber'} = $patron->borrowernumber; - $needsconfirmation{'resbranchcode'} = $res->{branchcode}; - $needsconfirmation{'reswaitingdate'} = $res->{'waitingdate'}; - } - elsif ( $restype eq "Reserved" ) { - # The item is on reserve for someone else. - $needsconfirmation{RESERVED} = 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}; - } + # See if the item is on reserve. + my ( $restype, $res ) = C4::Reserves::CheckReserves( $item->{'itemnumber'} ); + if ($restype) { + my $resbor = $res->{'borrowernumber'}; + if ( $resbor ne $patron->borrowernumber ) { + my $patron = Koha::Patrons->find( $resbor ); + if ( $restype eq "Waiting" ) + { + # The item is on reserve and waiting, but has been + # reserved by some other patron. + $needsconfirmation{RESERVE_WAITING} = 1; + $needsconfirmation{'resfirstname'} = $patron->firstname; + $needsconfirmation{'ressurname'} = $patron->surname; + $needsconfirmation{'rescardnumber'} = $patron->cardnumber; + $needsconfirmation{'resborrowernumber'} = $patron->borrowernumber; + $needsconfirmation{'resbranchcode'} = $res->{branchcode}; + $needsconfirmation{'reswaitingdate'} = $res->{'waitingdate'}; + } + elsif ( $restype eq "Reserved" && !C4::Context->preference("AllowItemsOnHoldCheckout") ) { + # The item is on reserve for someone else. + $needsconfirmation{RESERVED} = 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}; } } } diff --git a/C4/SIP/ILS/Transaction/Checkout.pm b/C4/SIP/ILS/Transaction/Checkout.pm index 23cdec92c4..e05166a1a8 100644 --- a/C4/SIP/ILS/Transaction/Checkout.pm +++ b/C4/SIP/ILS/Transaction/Checkout.pm @@ -57,9 +57,7 @@ sub do_checkout { my $patron = Koha::Patrons->find( { cardnumber => $patron_barcode } ); my $borrower = $patron->unblessed; $debug and warn "do_checkout borrower: . " . Dumper $borrower; - my ($issuingimpossible, $needsconfirmation) = _can_we_issue($patron, $barcode, - C4::Context->preference("AllowItemsOnHoldCheckout") - ); + my ($issuingimpossible, $needsconfirmation) = _can_we_issue($patron, $barcode ); my $noerror=1; # If set to zero we block the issue if (keys %{$issuingimpossible}) { @@ -150,10 +148,10 @@ sub do_checkout { } sub _can_we_issue { - my ( $patron, $barcode, $pref ) = @_; + my ( $patron, $barcode ) = @_; my ( $issuingimpossible, $needsconfirmation, $alerts ) = - CanBookBeIssued( $patron, $barcode, undef, 0, $pref ); + CanBookBeIssued( $patron, $barcode, undef, 0 ); for my $href ( $issuingimpossible, $needsconfirmation ) { # some data is returned using lc keys we only diff --git a/C4/UsageStats.pm b/C4/UsageStats.pm index 4f66111cd2..95250f2197 100644 --- a/C4/UsageStats.pm +++ b/C4/UsageStats.pm @@ -135,7 +135,6 @@ sub BuildReport { AllFinesNeedOverride AllowFineOverride AllowItemsOnHoldCheckout - AllowItemsOnHoldCheckoutSCO AllowNotForLoanOverride AllowRenewalLimitOverride AllowReturnToBranch diff --git a/circ/circulation.pl b/circ/circulation.pl index 5c858f717c..995a9eeeea 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -335,7 +335,6 @@ if (@$barcodes) { $patron, $barcode, $datedue, $inprocess, - undef, { onsite_checkout => $onsite_checkout, override_high_holds => $override_high_holds || $override_high_holds_tmp || 0, diff --git a/installer/data/mysql/atomicupdate/bug_21572.perl b/installer/data/mysql/atomicupdate/bug_21572.perl new file mode 100644 index 0000000000..5a6dadfa20 --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_21572.perl @@ -0,0 +1,15 @@ +$DBversion = 'XXX'; # will be replaced by the RM +if( CheckVersion( $DBversion ) ) { + my ( $SCOvalue ) = $dbh->selectrow_array(q| + SELECT value FROM systempreferences WHERE variable = 'AllowItemsOnHoldCheckoutSCO' + |); + + if ( $SCOvalue ) { + $dbh->do("UPDATE systempreferences SET value = 1 WHERE variable = 'AllowItemsOnHoldCheckout'"); + } + + $dbh->do("DELETE FROM systempreferences WHERE variable = 'AllowItemsOnHoldCheckoutSCO'"); + + SetVersion( $DBversion ); + print "Upgrade to $DBversion done (Bug 21572 - Allow checkout an item with reserve that is not yet in waiting state)\n"; +} diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql index 0f86627024..6a4c928057 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/installer/data/mysql/sysprefs.sql @@ -25,7 +25,6 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('AllowHoldsOnPatronsPossessions','1',NULL,'Allow holds on records that patron have items of it','YesNo'), ('AllowCheckoutNotes', '0', NULL, 'Allow patrons to submit notes about checked out items.','YesNo'), ('AllowItemsOnHoldCheckout','0','','Do not generate RESERVE_WAITING and RESERVED warning when checking out items reserved to someone else. This allows self checkouts for those items.','YesNo'), -('AllowItemsOnHoldCheckoutSCO','0','','Do not generate RESERVE_WAITING and RESERVED warning in the SCO module when checking out items reserved to someone else. This allows self checkouts for those items.','YesNo'), ('AllowMultipleCovers','0','1','Allow multiple cover images to be attached to each bibliographic record.','YesNo'), ('AllowMultipleIssuesOnABiblio',1,'Allow/Don\'t allow patrons to check out multiple items from one biblio','','YesNo'), ('AllowNotForLoanOverride','0','','If ON, Koha will allow the librarian to loan a not for loan item.','YesNo'), 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 980bdf0dbf..b08737a323 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 @@ -188,13 +188,7 @@ Circulation: choices: yes: Allow no: "Don't allow" - - checkouts of items reserved to someone else. If allowed do not generate RESERVE_WAITING and RESERVED warning. This allows self checkouts for those items. - - - - pref: AllowItemsOnHoldCheckoutSCO - choices: - yes: Allow - no: "Don't allow" - - checkouts of items reserved to someone else in the SCO module. If allowed do not generate RESERVE_WAITING and RESERVED warning. This allows self checkouts for those items. + - checkouts of items reserved to someone else. If allowed do not generate RESERVED warning. This allows self checkouts for those items. - - pref: AllFinesNeedOverride choices: diff --git a/opac/sco/sco-main.pl b/opac/sco/sco-main.pl index 9fcf7293b5..dea7c7b6c6 100755 --- a/opac/sco/sco-main.pl +++ b/opac/sco/sco-main.pl @@ -135,12 +135,11 @@ elsif ( $patron and $op eq "checkout" ) { $patron, $barcode, undef, - 0, - C4::Context->preference("AllowItemsOnHoldCheckoutSCO") + 0 ); my $issue_error; if ( $confirm_required = scalar keys %$needconfirm ) { - for my $error ( qw( UNKNOWN_BARCODE max_loans_allowed ISSUED_TO_ANOTHER NO_MORE_RENEWALS NOT_FOR_LOAN DEBT WTHDRAWN RESTRICTED RESERVED ITEMNOTSAMEBRANCH EXPIRED DEBARRED CARD_LOST GNA INVALID_DATE UNKNOWN_BARCODE TOO_MANY DEBT_GUARANTEES USERBLOCKEDOVERDUE PATRON_CANT PREVISSUE NOT_FOR_LOAN_FORCING ITEM_LOST) ) { + for my $error ( qw( UNKNOWN_BARCODE max_loans_allowed ISSUED_TO_ANOTHER NO_MORE_RENEWALS NOT_FOR_LOAN DEBT WTHDRAWN RESTRICTED RESERVED RESERVE_WAITING ITEMNOTSAMEBRANCH EXPIRED DEBARRED CARD_LOST GNA INVALID_DATE UNKNOWN_BARCODE TOO_MANY DEBT_GUARANTEES USERBLOCKEDOVERDUE PATRON_CANT PREVISSUE NOT_FOR_LOAN_FORCING ITEM_LOST) ) { if ( $needconfirm->{$error} ) { $issue_error = $error; $confirmed = 0; diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 09d4d0adb6..b6da161c28 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -2206,7 +2206,7 @@ subtest 'CanBookBeIssued | is_overdue' => sub { my $issue = AddIssue( $patron->unblessed, $item->{barcode}, $five_days_go ); # date due was 10d ago my $actualissue = Koha::Checkouts->find( { itemnumber => $item->{itemnumber} } ); is( output_pref({ str => $actualissue->date_due, dateonly => 1}), $five_days_go, "First issue works"); - my ($issuingimpossible, $needsconfirmation) = CanBookBeIssued($patron,$item->{barcode},$ten_days_go, undef, undef, undef); + my ($issuingimpossible, $needsconfirmation) = CanBookBeIssued($patron,$item->{barcode},$ten_days_go, undef, undef); is( $needsconfirmation->{RENEW_ISSUE}, 1, "This is a renewal"); is( $needsconfirmation->{TOO_MANY}, undef, "Not too many, is a renewal"); @@ -2242,7 +2242,7 @@ subtest 'CanBookBeIssued | item-level_itypes=biblio' => sub { } )->store; - my ( $issuingimpossible, $needsconfirmation ) = CanBookBeIssued( $patron, $item->barcode, undef, undef, undef, undef ); + my ( $issuingimpossible, $needsconfirmation ) = CanBookBeIssued( $patron, $item->barcode, undef, undef, undef ); is_deeply( $needsconfirmation, {}, 'Item can be issued to this patron' ); is_deeply( $issuingimpossible, {}, 'Item can be issued to this patron' ); }; @@ -2287,13 +2287,13 @@ subtest 'CanBookBeIssued | notforloan' => sub { t::lib::Mocks::mock_preference('item-level_itypes', 1); # item # Is for loan at item type and item level - ( $issuingimpossible, $needsconfirmation ) = CanBookBeIssued( $patron, $item->barcode, undef, undef, undef, undef ); + ( $issuingimpossible, $needsconfirmation ) = CanBookBeIssued( $patron, $item->barcode, undef, undef, undef ); is_deeply( $needsconfirmation, {}, 'Item can be issued to this patron' ); is_deeply( $issuingimpossible, {}, 'Item can be issued to this patron' ); # not for loan at item type level Koha::ItemTypes->find( $itemtype->{itemtype} )->notforloan(1)->store; - ( $issuingimpossible, $needsconfirmation ) = CanBookBeIssued( $patron, $item->barcode, undef, undef, undef, undef ); + ( $issuingimpossible, $needsconfirmation ) = CanBookBeIssued( $patron, $item->barcode, undef, undef, undef ); is_deeply( $needsconfirmation, {}, 'No confirmation needed, AllowNotForLoanOverride=0' ); is_deeply( $issuingimpossible, @@ -2304,7 +2304,7 @@ subtest 'CanBookBeIssued | notforloan' => sub { # not for loan at item level Koha::ItemTypes->find( $itemtype->{itemtype} )->notforloan(undef)->store; $item->notforloan( 1 )->store; - ( $issuingimpossible, $needsconfirmation ) = CanBookBeIssued( $patron, $item->barcode, undef, undef, undef, undef ); + ( $issuingimpossible, $needsconfirmation ) = CanBookBeIssued( $patron, $item->barcode, undef, undef, undef ); is_deeply( $needsconfirmation, {}, 'No confirmation needed, AllowNotForLoanOverride=0' ); is_deeply( $issuingimpossible, @@ -2329,13 +2329,13 @@ subtest 'CanBookBeIssued | notforloan' => sub { # for loan at item type and item level $item->notforloan(undef)->store; $item->biblioitem->itemtype($itemtype->{itemtype})->store; - ( $issuingimpossible, $needsconfirmation ) = CanBookBeIssued( $patron, $item->barcode, undef, undef, undef, undef ); + ( $issuingimpossible, $needsconfirmation ) = CanBookBeIssued( $patron, $item->barcode, undef, undef, undef ); is_deeply( $needsconfirmation, {}, 'Item can be issued to this patron' ); is_deeply( $issuingimpossible, {}, 'Item can be issued to this patron' ); # not for loan at item type level Koha::ItemTypes->find( $itemtype->{itemtype} )->notforloan(1)->store; - ( $issuingimpossible, $needsconfirmation ) = CanBookBeIssued( $patron, $item->barcode, undef, undef, undef, undef ); + ( $issuingimpossible, $needsconfirmation ) = CanBookBeIssued( $patron, $item->barcode, undef, undef, undef ); is_deeply( $needsconfirmation, {}, 'No confirmation needed, AllowNotForLoanOverride=0' ); is_deeply( $issuingimpossible, @@ -2346,7 +2346,7 @@ subtest 'CanBookBeIssued | notforloan' => sub { # not for loan at item level Koha::ItemTypes->find( $itemtype->{itemtype} )->notforloan(undef)->store; $item->notforloan( 1 )->store; - ( $issuingimpossible, $needsconfirmation ) = CanBookBeIssued( $patron, $item->barcode, undef, undef, undef, undef ); + ( $issuingimpossible, $needsconfirmation ) = CanBookBeIssued( $patron, $item->barcode, undef, undef, undef ); is_deeply( $needsconfirmation, {}, 'No confirmation needed, AllowNotForLoanOverride=0' ); is_deeply( $issuingimpossible, diff --git a/t/db_dependent/DecreaseLoanHighHolds.t b/t/db_dependent/DecreaseLoanHighHolds.t index a85048f4b9..d2c6517cf2 100755 --- a/t/db_dependent/DecreaseLoanHighHolds.t +++ b/t/db_dependent/DecreaseLoanHighHolds.t @@ -193,7 +193,7 @@ my $patron_object = Koha::Patrons->find( $patron_hr->{borrowernumber} ); my ( undef, $needsconfirmation ) = CanBookBeIssued( $patron_object, $item->barcode ); ok( $needsconfirmation->{HIGHHOLDS}, "High holds checkout needs confirmation" ); -( undef, $needsconfirmation ) = CanBookBeIssued( $patron_object, $item->barcode, undef, undef, undef, { override_high_holds => 1 } ); +( undef, $needsconfirmation ) = CanBookBeIssued( $patron_object, $item->barcode, undef, undef, { override_high_holds => 1 } ); ok( !$needsconfirmation->{HIGHHOLDS}, "High holds checkout does not need confirmation" ); $schema->storage->txn_rollback(); diff --git a/t/db_dependent/UsageStats.t b/t/db_dependent/UsageStats.t index 9d11af1674..910c2053b4 100644 --- a/t/db_dependent/UsageStats.t +++ b/t/db_dependent/UsageStats.t @@ -387,7 +387,6 @@ sub mocking_systempreferences_to_a_set_value { AllFinesNeedOverride AllowFineOverride AllowItemsOnHoldCheckout - AllowItemsOnHoldCheckoutSCO AllowNotForLoanOverride AllowRenewalLimitOverride AllowReturnToBranch -- 2.17.1