From 3e78b4561cffe4b84235e339a1d6008eaec2fd71 Mon Sep 17 00:00:00 2001 From: Aleisha Amohia Date: Mon, 20 Apr 2020 07:12:59 +0000 Subject: [PATCH] Bug 25193: Add AllowRenewalIfOtherItemsAvailableLocation syspref This syspref works in conjunction with AllowRenewalIfOtherItemsAvailable syspref. It has two options - allow renewal if other items are available to fill holds at any branch, or only at the hold's pickup branch. To test: 1) Apply patch, update database, restart services 2) Go to Administration -> system preferences. Search for AllowRenewalIfOtherItemsAvailable. Notice the two sysprefs exist in the same row. By default they should be set to "Don't allow" and "at any library". 3) Set up a biblio with two items. Have both items held at Branch A. 4) Check out Item A to Borrower A. 5) Place a biblio-level reserve for Borrower B with pickup branch at Branch A. 6) View Borrower A's checkouts. Confirm you cannot renew Item A ('On hold' message). 7) Edit Item B, set the current location to Branch B. The items should now have different locations. 8) Enable the AllowRenewalIfOtherItemsAvailable syspref. Leave the AllowRenewalIfOtherItemsAvailableLocation syspref set to "at any library". 9) View Borrower A's checkouts. Confirm you CAN renew Item A (because Item B can fill Borrower B's reserve). 10) Set the AllowRenewalIfOtherItemsAvailableLocation syspref to "at the hold's pickup library". Keep AllowRenewalIfOtherItemsAvailable syspref enabled. 11) View Borrower A's checkouts. Confirm you CANNOT renew Item A (because Item B cannot fill Borrower B's reserve due to being at a different branch). 12) Edit Item B, set the current location to Branch A. The items should have the same location now. 13) View Borrower A's checkouts. Confirm you CAN renew Item A. Sponsored-by: Chartered Accountants Australia & New Zealand Signed-off-by: David Nind Signed-off-by: David Nind Signed-off-by: Katrin Fischer --- 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 diff --git a/C4/Circulation.pm b/C4/Circulation.pm index bd8ecfc58fa..7ffa46a6a94 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -3104,16 +3104,24 @@ sub CanBookBeRenewed { my $patron_with_reserve = Koha::Patrons->find($possible_hold->{borrowernumber}); foreach my $other_item (@other_items) { - next if defined $matched_items{$other_item->itemnumber}; - next if $other_item->holds->filter_by_found->count; - 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 $other_item->holds->filter_by_found->count; + 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; } diff --git a/installer/data/mysql/atomicupdate/bug_25193-add_AllowRenewalIfOtherItemsAvailableLocation_syspref.pl b/installer/data/mysql/atomicupdate/bug_25193-add_AllowRenewalIfOtherItemsAvailableLocation_syspref.pl new file mode 100755 index 00000000000..23888958a8e --- /dev/null +++ b/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'"; + }, +}; diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql index 66c7b008f21..efd5a40c798 100644 --- a/installer/data/mysql/mandatory/sysprefs.sql +++ b/installer/data/mysql/mandatory/sysprefs.sql @@ -45,6 +45,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'), 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 317e4238ec2..2c4b008983e 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 @@ -784,7 +784,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: diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 009c968c445..4f196e7db27 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -18,7 +18,7 @@ use Modern::Perl; use utf8; -use Test::More tests => 75; +use Test::More tests => 76; use Test::Exception; use Test::MockModule; use Test::Deep qw( cmp_deeply ); @@ -2257,6 +2257,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; -- 2.39.5