From f8467a8eb7a11a2730badf562bb22d3327512db0 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 22 Jan 2020 12:27:22 +0100 Subject: [PATCH] Bug 18936: Fix some more tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * CanItemBeReserved Prior to "Bug 18936: Convert issuingrules fields to circulation_rules", GetHoldRule returned holds_per_record even if no reservesallowed was defined. This change restores this behavior. FIXME Note: In GetHoldRule we return itemtype only if reservesallowed is set, not sure it is correct. * t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t When setting returnbranch, holdallowed and hold_fulfillment_policy, we should not provide categorycode. * t/db_dependent/Holds.t Prefer to keep the existing rules instead of removing them. It got quite hard to understand what was going on here because of the mixup with the rule reservesallowed that was in issuingrules, and the other rules we used for the tests. Also, categorycode should not be passed to set those 3 rules (holdallowed, hold_fulfillment_policy and returnbranch) * t/db_dependent/Circulation.t Setting lengthunit to 'hours', no need to make sure the rule has been correctly be saved * t/db_dependent/Circulation/CalcDateDue.t It uses hardcoded data that is not in the sample data (categorycode=C). Let use K that exists and postpone a refactore of the whole script (to make it create the data it needs). * t/db_dependent/Circulation/ReturnClaims.t * t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t Simple replace Koha::IssuingRule with Koha::CirculationRules * t/db_dependent/Koha/Charges/Fees.t => FIXME Still failing, stuck here, need help Signed-off-by: Minna Kivinen Signed-off-by: Joonas Kylmälä --- C4/Reserves.pm | 15 ++--- t/db_dependent/Circulation.t | 12 ++-- t/db_dependent/Circulation/CalcDateDue.t | 25 +++++--- .../Circulation/IssuingRules/maxsuspensiondays.t | 24 +++---- t/db_dependent/Circulation/ReturnClaims.t | 13 ++-- t/db_dependent/Holds.t | 73 ++++++---------------- .../Holds/DisallowHoldIfItemsAvailable.t | 1 - t/db_dependent/Koha/Charges/Fees.t | 12 ++-- 8 files changed, 71 insertions(+), 104 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index aa4c038f6d..86ff1faebb 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -379,8 +379,8 @@ sub CanItemBeReserved { # we retrieve rights if ( my $rights = GetHoldRule( $borrower->{'categorycode'}, $item->effective_itemtype, $branchcode ) ) { $ruleitemtype = $rights->{itemtype}; - $allowedreserves = $rights->{reservesallowed}; - $holds_per_record = $rights->{holds_per_record}; + $allowedreserves = $rights->{reservesallowed} // $allowedreserves; + $holds_per_record = $rights->{holds_per_record} // $holds_per_record; $holds_per_day = $rights->{holds_per_day}; } else { @@ -2216,13 +2216,14 @@ sub GetHoldRule { } } ); - return unless $reservesallowed;; my $rules; - $rules->{reservesallowed} = $reservesallowed->rule_value; - $rules->{itemtype} = $reservesallowed->itemtype; - $rules->{categorycode} = $reservesallowed->categorycode; - $rules->{branchcode} = $reservesallowed->branchcode; + if ( $reservesallowed ) { + $rules->{reservesallowed} = $reservesallowed->rule_value; + $rules->{itemtype} = $reservesallowed->itemtype; + $rules->{categorycode} = $reservesallowed->categorycode; + $rules->{branchcode} = $reservesallowed->branchcode; + } my $holds_per_x_rules = Koha::CirculationRules->get_effective_rules( { diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index f96cb9c7db..4b17bf5d61 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -3358,7 +3358,7 @@ subtest 'ProcessOfflinePayment() tests' => sub { }; subtest 'Incremented fee tests' => sub { - plan tests => 20; + plan tests => 19; my $dt = dt_from_string(); Time::Fake->offset( $dt->epoch ); @@ -3486,16 +3486,15 @@ subtest 'Incremented fee tests' => sub { 'Rental charge reset and retreived correctly' ); # Hourly - my $issuingrule = Koha::IssuingRules->get_effective_issuing_rule( + Koha::CirculationRules->set_rule( { categorycode => $patron->categorycode, itemtype => $itemtype->id, - branchcode => $library->id + branchcode => $library->id, + rule_name => 'lengthunit', + rule_value => 'hours', } ); - $issuingrule->lengthunit('hours')->store(); - is( $issuingrule->lengthunit, 'hours', - 'Issuingrule updated and retrieved correctly' ); $itemtype->rentalcharge_hourly('0.25')->store(); is( $itemtype->rentalcharge_hourly, @@ -3545,7 +3544,6 @@ subtest 'Incremented fee tests' => sub { "Hourly rental charge calculated correctly with finesCalendar = noFinesWhenClosed, for renewal (312h - 168h - 0h * 0.25u)" ); $accountline->delete(); $issue->delete(); - $issuingrule->lengthunit('days')->store(); Time::Fake->reset; }; diff --git a/t/db_dependent/Circulation/CalcDateDue.t b/t/db_dependent/Circulation/CalcDateDue.t index b154c1fd5e..4c0999ef0d 100644 --- a/t/db_dependent/Circulation/CalcDateDue.t +++ b/t/db_dependent/Circulation/CalcDateDue.t @@ -157,22 +157,27 @@ $calendar->delete_holiday( # Now we test it does the right thing if the loan and renewal periods # are a multiple of 7 days -my $dayweek_categorycode = 'C'; +my $dayweek_categorycode = 'K'; my $dayweek_itemtype = 'MX'; my $dayweek_branchcode = 'FPL'; my $dayweek_issuelength = 14; my $dayweek_renewalperiod = 7; my $dayweek_lengthunit = 'days'; -Koha::Database->schema->resultset('Issuingrule')->create({ - categorycode => $dayweek_categorycode, - itemtype => $dayweek_itemtype, - branchcode => $dayweek_branchcode, - issuelength => $dayweek_issuelength, - renewalperiod => $dayweek_renewalperiod, - lengthunit => $dayweek_lengthunit, -}); -my $dayweek_borrower = {categorycode => 'C', dateexpiry => $dateexpiry}; +Koha::CirculationRules->set_rules( + { + categorycode => $dayweek_categorycode, + itemtype => $dayweek_itemtype, + branchcode => $dayweek_branchcode, + rules => { + issuelength => $dayweek_issuelength, + renewalperiod => $dayweek_renewalperiod, + lengthunit => $dayweek_lengthunit, + } + } +); + +my $dayweek_borrower = {categorycode => 'K', dateexpiry => $dateexpiry}; # For issues... $start_date = DateTime->new({year => 2013, month => 2, day => 9}); diff --git a/t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t b/t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t index 2cbc02e73d..f29df5f94c 100644 --- a/t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t +++ b/t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t @@ -109,14 +109,12 @@ is( DelDebarment( $debarments->[0]->{borrower_debarment_id} ); subtest "suspension_chargeperiod" => sub { - Koha::IssuingRules->search->delete; - $builder->build( + Koha::CirculationRules->set_rules( { - source => 'Issuingrule', - value => { - categorycode => '*', - itemtype => '*', - branchcode => '*', + categorycode => undef, + itemtype => undef, + branchcode => undef, + rules => { firstremind => 0, finedays => 7, lengthunit => 'days', @@ -137,14 +135,12 @@ subtest "suspension_chargeperiod" => sub { }; subtest "maxsuspensiondays" => sub { - Koha::IssuingRules->search->delete; - $builder->build( + Koha::CirculationRules->set_rules( { - source => 'Issuingrule', - value => { - categorycode => '*', - itemtype => '*', - branchcode => '*', + categorycode => undef, + itemtype => undef, + branchcode => undef, + rules => { firstremind => 0, finedays => 15, lengthunit => 'days', diff --git a/t/db_dependent/Circulation/ReturnClaims.t b/t/db_dependent/Circulation/ReturnClaims.t index 3681650464..464420ead2 100644 --- a/t/db_dependent/Circulation/ReturnClaims.t +++ b/t/db_dependent/Circulation/ReturnClaims.t @@ -46,16 +46,15 @@ my $schema = Koha::Database->schema; $schema->storage->txn_begin; my $builder = t::lib::TestBuilder->new(); -Koha::IssuingRules->search->delete; -my $rule = Koha::IssuingRule->new( +Koha::CirculationRules->set_rule( { - categorycode => '*', - itemtype => '*', - branchcode => '*', - issuelength => 1, + branchcode => undef, + categorycode => undef, + itemtype => undef, + rule_name => 'issuelength', + rule_value => 1 } ); -$rule->store(); $branch = $builder->build( { source => 'Branch' } )->{branchcode}; diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index aee25a8759..d5a2f8b58a 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -458,7 +458,6 @@ subtest 'Test max_holds per library/patron category' => sub { plan tests => 6; $dbh->do('DELETE FROM reserves'); - $dbh->do('DELETE FROM circulation_rules'); $biblio = $builder->build_sample_biblio; ( $item_bibnum, $item_bibitemnum, $itemnumber ) = @@ -586,8 +585,6 @@ subtest 'CanItemBeReserved / holds_per_day tests' => sub { Koha::Holds->search->delete; $dbh->do('DELETE FROM issues'); - $dbh->do('DELETE FROM issuingrules'); - $dbh->do('DELETE FROM circulation_rules'); Koha::Items->search->delete; Koha::Biblios->search->delete; @@ -753,14 +750,15 @@ subtest 'CanItemBeReserved / branch_not_in_hold_group' => sub { # Cleanup database Koha::Holds->search->delete; $dbh->do('DELETE FROM issues'); - $dbh->do('DELETE FROM issuingrules'); - $dbh->do( - q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed) - VALUES (?, ?, ?, ?)}, - {}, - '*', '*', '*', 25 + Koha::CirculationRules->set_rule( + { + branchcode => undef, + categorycode => undef, + itemtype => undef, + rule_name => 'reservesallowed', + rule_value => 25, + } ); - $dbh->do('DELETE FROM circulation_rules'); Koha::Items->search->delete; Koha::Biblios->search->delete; @@ -818,7 +816,6 @@ subtest 'CanItemBeReserved / branch_not_in_hold_group' => sub { { branchcode => undef, itemtype => undef, - categorycode => undef, rules => { holdallowed => 3, hold_fulfillment_policy => 'any', @@ -841,15 +838,11 @@ subtest 'CanItemBeReserved / branch_not_in_hold_group' => sub { 'Patron cannot place hold because patron\'s home library is not part of hold group' ); - # Cleanup default_cirt_rules - $dbh->do('DELETE FROM circulation_rules'); - # Insert default circ rule to "any" for library 2 Koha::CirculationRules->set_rules( { branchcode => $library2->branchcode, itemtype => undef, - categorycode => undef, rules => { holdallowed => 2, hold_fulfillment_policy => 'any', @@ -870,7 +863,6 @@ subtest 'CanItemBeReserved / branch_not_in_hold_group' => sub { { branchcode => $library2->branchcode, itemtype => undef, - categorycode => undef, rules => { holdallowed => 3, hold_fulfillment_policy => 'any', @@ -886,15 +878,11 @@ subtest 'CanItemBeReserved / branch_not_in_hold_group' => sub { 'Patron cannot place hold if holdallowed is set to "hold group" for library 2' ); - # Cleanup default_branch_cirt_rules - $dbh->do('DELETE FROM circulation_rules'); - # Insert default item rule to "any" for itemtype 2 Koha::CirculationRules->set_rules( { - branchcode => undef, + branchcode => $library2->branchcode, itemtype => $itemtype2->itemtype, - categorycode => undef, rules => { holdallowed => 2, hold_fulfillment_policy => 'any', @@ -913,9 +901,8 @@ subtest 'CanItemBeReserved / branch_not_in_hold_group' => sub { # Update default item rule to "hold group" for itemtype 2 Koha::CirculationRules->set_rules( { - branchcode => undef, + branchcode => $library2->branchcode, itemtype => $itemtype2->itemtype, - categorycode => undef, rules => { holdallowed => 3, hold_fulfillment_policy => 'any', @@ -931,15 +918,11 @@ subtest 'CanItemBeReserved / branch_not_in_hold_group' => sub { 'Patron cannot place hold if holdallowed is set to "hold group" for itemtype 2' ); - # Cleanup default_branch_item_rules - $dbh->do('DELETE FROM circulation_rules'); - # Insert branch item rule to "any" for itemtype 2 and library 2 Koha::CirculationRules->set_rules( { branchcode => $library2->branchcode, itemtype => $itemtype2->itemtype, - categorycode => undef, rules => { holdallowed => 2, hold_fulfillment_policy => 'any', @@ -960,7 +943,6 @@ subtest 'CanItemBeReserved / branch_not_in_hold_group' => sub { { branchcode => $library2->branchcode, itemtype => $itemtype2->itemtype, - categorycode => undef, rules => { holdallowed => 3, hold_fulfillment_policy => 'any', @@ -988,14 +970,15 @@ subtest 'CanItemBeReserved / pickup_not_in_hold_group' => sub { # Cleanup database Koha::Holds->search->delete; $dbh->do('DELETE FROM issues'); - $dbh->do('DELETE FROM issuingrules'); - $dbh->do( - q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed) - VALUES (?, ?, ?, ?)}, - {}, - '*', '*', '*', 25 + Koha::CirculationRules->set_rule( + { + branchcode => undef, + categorycode => undef, + itemtype => undef, + rule_name => 'reservesallowed', + rule_value => 25, + } ); - $dbh->do('DELETE FROM circulation_rules'); Koha::Items->search->delete; Koha::Biblios->search->delete; @@ -1053,7 +1036,6 @@ subtest 'CanItemBeReserved / pickup_not_in_hold_group' => sub { { branchcode => undef, itemtype => undef, - categorycode => undef, rules => { holdallowed => 2, hold_fulfillment_policy => 'holdgroup', @@ -1076,15 +1058,11 @@ subtest 'CanItemBeReserved / pickup_not_in_hold_group' => sub { 'Patron cannot place hold because pickup location is not part of hold group' ); - # Cleanup default_cirt_rules - $dbh->do('DELETE FROM circulation_rules'); - # Insert default circ rule to "any" for library 2 Koha::CirculationRules->set_rules( { branchcode => $library2->branchcode, itemtype => undef, - categorycode => undef, rules => { holdallowed => 2, hold_fulfillment_policy => 'any', @@ -1105,7 +1083,6 @@ subtest 'CanItemBeReserved / pickup_not_in_hold_group' => sub { { branchcode => $library2->branchcode, itemtype => undef, - categorycode => undef, rules => { holdallowed => 2, hold_fulfillment_policy => 'holdgroup', @@ -1121,15 +1098,11 @@ subtest 'CanItemBeReserved / pickup_not_in_hold_group' => sub { 'Patron cannot place hold if hold_fulfillment_policy is set to "hold group" for library 2' ); - # Cleanup default_branch_cirt_rules - $dbh->do('DELETE FROM circulation_rules'); - # Insert default item rule to "any" for itemtype 2 Koha::CirculationRules->set_rules( { - branchcode => undef, + branchcode => $library2->branchcode, itemtype => $itemtype2->itemtype, - categorycode => undef, rules => { holdallowed => 2, hold_fulfillment_policy => 'any', @@ -1148,9 +1121,8 @@ subtest 'CanItemBeReserved / pickup_not_in_hold_group' => sub { # Update default item rule to "hold group" for itemtype 2 Koha::CirculationRules->set_rules( { - branchcode => undef, + branchcode => $library2->branchcode, itemtype => $itemtype2->itemtype, - categorycode => undef, rules => { holdallowed => 2, hold_fulfillment_policy => 'holdgroup', @@ -1166,15 +1138,11 @@ subtest 'CanItemBeReserved / pickup_not_in_hold_group' => sub { 'Patron cannot place hold if hold_fulfillment_policy is set to "hold group" for itemtype 2' ); - # Cleanup default_branch_item_rules - $dbh->do('DELETE FROM circulation_rules'); - # Insert branch item rule to "any" for itemtype 2 and library 2 Koha::CirculationRules->set_rules( { branchcode => $library2->branchcode, itemtype => $itemtype2->itemtype, - categorycode => undef, rules => { holdallowed => 2, hold_fulfillment_policy => 'any', @@ -1195,7 +1163,6 @@ subtest 'CanItemBeReserved / pickup_not_in_hold_group' => sub { { branchcode => $library2->branchcode, itemtype => $itemtype2->itemtype, - categorycode => undef, rules => { holdallowed => 2, hold_fulfillment_policy => 'holdgroup', diff --git a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t b/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t index f49aa107a9..3df3d97daa 100755 --- a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t +++ b/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t @@ -239,7 +239,6 @@ sub set_holdallowed_rule { Koha::CirculationRules->set_rules( { branchcode => $branchcode || undef, - categorycode => undef, itemtype => undef, rules => { holdallowed => $holdallowed, diff --git a/t/db_dependent/Koha/Charges/Fees.t b/t/db_dependent/Koha/Charges/Fees.t index 544d9050bd..431e292a54 100644 --- a/t/db_dependent/Koha/Charges/Fees.t +++ b/t/db_dependent/Koha/Charges/Fees.t @@ -364,15 +364,17 @@ subtest 'accumulate_rentalcharge tests' => sub { ); # Hourly tests - my $issuingrule = Koha::IssuingRules->get_effective_issuing_rule( - { + Koha::CirculationRules->set_rules({ categorycode => $patron->categorycode, itemtype => $itemtype->id, - branchcode => $library->id + branchcode => $library->id, + rules => { + lengthunit => 'hours', + } } + ); - $issuingrule->lengthunit('hours'); - $issuingrule->store(); + $itemtype->rentalcharge_hourly("0.25"); $itemtype->store(); -- 2.11.0