Bugzilla – Attachment 80714 Details for
Bug 21572
Allow checkout an item with pending hold that is not yet in waiting state
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 21572: Unify checkout policy of reserved items in SIP, SCO, and intranet
Bug-21572-Unify-checkout-policy-of-reserved-items-.patch (text/plain), 20.42 KB, created by
Joonas Kylmälä
on 2018-10-17 14:27:03 UTC
(
hide
)
Description:
Bug 21572: Unify checkout policy of reserved items in SIP, SCO, and intranet
Filename:
MIME Type:
Creator:
Joonas Kylmälä
Created:
2018-10-17 14:27:03 UTC
Size:
20.42 KB
patch
obsolete
>From 9b386125d473244e4c2b76f1d0d330f8a121769f Mon Sep 17 00:00:00 2001 >From: =?UTF-8?q?Joonas=20Kylm=C3=A4l=C3=A4?= <joonas.kylmala@helsinki.fi> >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
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=80714">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 21572
: 80714 |
80715