@@ -, +, @@ syspref hold' message). Item B can fill Borrower B's reserve). different branch). --- C4/Circulation.pm | 28 ++++-- ...alIfOtherItemsAvailableLocation_syspref.pl | 16 ++++ installer/data/mysql/mandatory/sysprefs.sql | 1 + .../admin/preferences/circulation.pref | 7 +- t/db_dependent/Circulation.t | 93 ++++++++++++++++++- 5 files changed, 133 insertions(+), 12 deletions(-) create mode 100755 installer/data/mysql/atomicupdate/bug_25193-add_AllowRenewalIfOtherItemsAvailableLocation_syspref.pl --- a/C4/Circulation.pm +++ a/C4/Circulation.pm @@ -3099,16 +3099,24 @@ sub CanBookBeRenewed { # FIXME: We are not checking whether the item we are renewing can fill the hold foreach my $other_item (@other_items) { - next if defined $matched_items{$other_item->itemnumber}; - next if IsItemOnHoldAndFound( $other_item->itemnumber ); - next unless IsAvailableForItemLevelRequest($other_item, $patron_with_reserve, undef); - next unless CanItemBeReserved($patron_with_reserve,$other_item,undef,{ignore_hold_counts=>1})->{status} eq 'OK'; - # NOTE: At checkin we call 'CheckReserves' which checks hold 'policy' - # CanItemBeReserved checks 'rules' and 'policies' which means - # items will fill holds at checkin that are rejected here - $fillable = 1; - $matched_items{$other_item->itemnumber} = 1; - last; + next if defined $matched_items{ $other_item->itemnumber }; + next if IsItemOnHoldAndFound( $other_item->itemnumber ); + next unless IsAvailableForItemLevelRequest( $other_item, $patron_with_reserve, undef ); + next + unless CanItemBeReserved( + $patron_with_reserve, $other_item, undef, + { ignore_hold_counts => 1 } + )->{status} eq 'OK'; + if ( C4::Context->preference('AllowRenewalIfOtherItemsAvailableLocation') eq 'holdbranch' ) { + next unless $other_item->holdingbranch eq $possible_hold->branchcode; + } + + # NOTE: At checkin we call 'CheckReserves' which checks hold 'policy' + # CanItemBeReserved checks 'rules' and 'policies' which means + # items will fill holds at checkin that are rejected here + $fillable = 1; + $matched_items{ $other_item->itemnumber } = 1; + last; } return ( 0, "on_reserve" ) unless $fillable; } --- a/installer/data/mysql/atomicupdate/bug_25193-add_AllowRenewalIfOtherItemsAvailableLocation_syspref.pl +++ a/installer/data/mysql/atomicupdate/bug_25193-add_AllowRenewalIfOtherItemsAvailableLocation_syspref.pl @@ -0,0 +1,16 @@ +use Modern::Perl; + +return { + bug_number => "25193", + description => "Add syspref to control location for use of AllowRenewalIfOtherItemsAvailable", + up => sub { + my ($args) = @_; + my ( $dbh, $out ) = @$args{qw(dbh out)}; + + $dbh->do( + q{INSERT IGNORE INTO systempreferences (variable,value,options,explanation,type) VALUES ('AllowRenewalIfOtherItemsAvailableLocation', 'any', 'any|holdbranch', 'Specify where items must be available for fulfilling holds to allow a renewal.', 'Choice') } + ); + + say $out "Added system preference 'AllowRenewalIfOtherItemsAvailableLocation'"; + }, +}; --- a/installer/data/mysql/mandatory/sysprefs.sql +++ a/installer/data/mysql/mandatory/sysprefs.sql @@ -43,6 +43,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('AllowPatronToSetFinesVisibilityForGuarantor', '0', NULL, 'If enabled, the patron can set fines to be visible to their guarantor', 'YesNo'), ('AllowPKIAuth','None','None|Common Name|emailAddress','Use the field from a client-side SSL certificate to look a user in the Koha database','Choice'), ('AllowRenewalIfOtherItemsAvailable','0',NULL,'If enabled, allow a patron to renew an item with unfilled holds if other available items can fill that hold.','YesNo'), +('AllowRenewalIfOtherItemsAvailableLocation', 'any', 'any|holdbranch', 'Specify where items must be available for fulfilling holds to allow a renewal.', 'Choice'), ('AllowRenewalLimitOverride','0',NULL,'if ON, allows renewal limits to be overridden on the circulation screen','YesNo'), ('AllowRenewalOnHoldOverride','0',NULL,'If ON, allow items on hold to be renewed with a specified due date','YesNo'), ('AllowReturnToBranch','anywhere','anywhere|homebranch|holdingbranch|homeorholdingbranch','Where an item may be returned','Choice'), --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref +++ a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref @@ -728,7 +728,12 @@ Circulation: choices: 1: Allow 0: "Don't allow" - - a patron to renew an item with unfilled holds if other available items can fill that hold. + - a patron to renew an item with unfilled holds if other available items + - pref: AllowRenewalIfOtherItemsAvailableLocation + choices: + any: "at any library" + holdbranch: "at the hold's pickup library" + - can fill that hold. - - pref: AllowHoldPolicyOverride choices: --- a/t/db_dependent/Circulation.t +++ a/t/db_dependent/Circulation.t @@ -18,7 +18,7 @@ use Modern::Perl; use utf8; -use Test::More tests => 68; +use Test::More tests => 69; use Test::Exception; use Test::MockModule; use Test::Deep qw( cmp_deeply ); @@ -2090,6 +2090,97 @@ subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { is( Koha::Account::Lines->search({ issue_id => $issue->id })->count, 1, 'UpdateFine should not create a new accountline when updating an existing fine'); } +subtest "AllowRenewalIfOtherItemsAvailableLocation tests" => sub { + plan tests => 3; + + $dbh->do('DELETE FROM issues'); + $dbh->do('DELETE FROM items'); + $dbh->do('DELETE FROM circulation_rules'); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => undef, + branchcode => undef, + rules => { + reservesallowed => 25, + issuelength => 14, + lengthunit => 'days', + renewalsallowed => 1, + renewalperiod => 7, + norenewalbefore => undef, + auto_renew => 0, + fine => .10, + chargeperiod => 1, + maxissueqty => 20, + onshelfholds => 1, + } + } + ); + my $biblio = $builder->build_sample_biblio(); + + my $item_1 = $builder->build_sample_item( + { + biblionumber => $biblio->biblionumber, + library => $library2->{branchcode}, + itype => $itemtype, + } + ); + + # add second item with different branchcode + my $item_2 = $builder->build_sample_item( + { + biblionumber => $biblio->biblionumber, + library => $library->{branchcode}, + itype => $itemtype, + } + ); + + my $borrower1 = Koha::Patron->new( + { + firstname => 'Kyle', + surname => 'Hall', + categorycode => $patron_category->{categorycode}, + branchcode => $library2->{branchcode}, + } + )->store; + my $borrowernumber1 = $borrower1->id; + my $borrower2 = Koha::Patron->new( + { + firstname => 'Chelsea', + surname => 'Hall', + categorycode => $patron_category->{categorycode}, + branchcode => $library2->{branchcode}, + } + )->store; + my $borrowernumber2 = $borrower2->id; + + my $issue = AddIssue( $borrower1, $item_1->barcode ); + + # place bib-level reserve + AddReserve( + { + branchcode => $library2->{branchcode}, + borrowernumber => $borrowernumber2, + biblionumber => $biblio->biblionumber, + priority => 1, + } + ); + + t::lib::Mocks::mock_preference( 'AllowRenewalIfOtherItemsAvailable', 1 ); + t::lib::Mocks::mock_preference( 'AllowRenewalIfOtherItemsAvailableLocation', 'any' ); + my ( $renewokay, $error ) = CanBookBeRenewed( $borrower1, $issue ); + is( $renewokay, 1, 'Renewal allowed if items available at any branch' ); + + t::lib::Mocks::mock_preference( 'AllowRenewalIfOtherItemsAvailableLocation', 'holdbranch' ); + ( $renewokay, $error ) = CanBookBeRenewed( $borrower1, $issue ); + is( $renewokay, 0, 'Renewal not allowed as available item is at a different branch' ); + + # adjust 2nd item to have same branchcode + $item_2->update( { homebranch => $library2->{branchcode}, holdingbranch => $library2->{branchcode} } )->store; + ( $renewokay, $error ) = CanBookBeRenewed( $borrower1, $issue ); + is( $renewokay, 1, 'Renewal allowed if items available at hold branch' ); +}; + subtest 'CanBookBeIssued & AllowReturnToBranch' => sub { plan tests => 24; --