From cf2ad385e534d79484dcc4f39b4763e4baaf84a0 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Tue, 11 Jul 2017 11:10:01 -0400 Subject: [PATCH] Bug 18928: Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules Test Plan: 1) Apply dependancies 2) Apply this patch set 3) Run updatedatabase.pl 4) Ensure holdallowed and hold_fulfillment_policy rules behavior remains unchanged Signed-off-by: Tomas Cohen Arazi Signed-off-by: Agustin Moyano Signed-off-by: Liz Rea --- C4/Circulation.pm | 82 +++-- admin/smart-rules.pl | 365 +++++++++------------ installer/data/mysql/atomicupdate/bug_18928.perl | 81 +++++ .../prog/en/modules/admin/smart-rules.tt | 131 ++++---- t/db_dependent/Circulation/Branch.t | 61 ++-- t/db_dependent/Holds.t | 36 +- t/db_dependent/Holds/HoldFulfillmentPolicy.t | 48 ++- t/db_dependent/Holds/HoldItemtypeLimit.t | 19 +- t/db_dependent/HoldsQueue.t | 133 +++++--- t/db_dependent/Reserves.t | 36 +- 10 files changed, 595 insertions(+), 397 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_18928.perl diff --git a/C4/Circulation.pm b/C4/Circulation.pm index b9ff0f2824..cca10d706e 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -1721,43 +1721,61 @@ Neither C<$branchcode> nor C<$itemtype> should be '*'. sub GetBranchItemRule { my ( $branchcode, $itemtype ) = @_; - my $dbh = C4::Context->dbh(); - my $result = {}; - - my @attempts = ( - ['SELECT holdallowed, returnbranch, hold_fulfillment_policy - FROM branch_item_rules - WHERE branchcode = ? - AND itemtype = ?', $branchcode, $itemtype], - ['SELECT holdallowed, returnbranch, hold_fulfillment_policy - FROM default_branch_circ_rules - WHERE branchcode = ?', $branchcode], - ['SELECT holdallowed, returnbranch, hold_fulfillment_policy - FROM default_branch_item_rules - WHERE itemtype = ?', $itemtype], - ['SELECT holdallowed, returnbranch, hold_fulfillment_policy - FROM default_circ_rules'], + + # Set search precedences + my @params = ( + { + branchcode => $branchcode, + categorycode => undef, + itemtype => $itemtype, + }, + { + branchcode => $branchcode, + categorycode => undef, + itemtype => undef, + }, + { + branchcode => undef, + categorycode => undef, + itemtype => $itemtype, + }, + { + branchcode => undef, + categorycode => undef, + itemtype => undef, + }, ); - foreach my $attempt (@attempts) { - my ($query, @bind_params) = @{$attempt}; - my $search_result = $dbh->selectrow_hashref ( $query , {}, @bind_params ) - or next; - - # Since branch/category and branch/itemtype use the same per-branch - # defaults tables, we have to check that the key we want is set, not - # just that a row was returned - $result->{'holdallowed'} = $search_result->{'holdallowed'} unless ( defined $result->{'holdallowed'} ); - $result->{'hold_fulfillment_policy'} = $search_result->{'hold_fulfillment_policy'} unless ( defined $result->{'hold_fulfillment_policy'} ); - $result->{'returnbranch'} = $search_result->{'returnbranch'} unless ( defined $result->{'returnbranch'} ); + # Initialize default values + my $rules = { + holdallowed => undef, + hold_fulfillment_policy => undef, + returnbranch => undef, + }; + + # Search for rules! + foreach my $rule_name (qw( holdallowed hold_fulfillment_policy returnbranch )) { + foreach my $params (@params) { + my $rule = Koha::CirculationRules->search( + { + rule_name => $rule_name, + %$params, + } + )->next(); + + if ( $rule ) { + $rules->{$rule_name} = $rule->rule_value; + last; + } + } } - + # built-in default circulation rule - $result->{'holdallowed'} = 2 unless ( defined $result->{'holdallowed'} ); - $result->{'hold_fulfillment_policy'} = 'any' unless ( defined $result->{'hold_fulfillment_policy'} ); - $result->{'returnbranch'} = 'homebranch' unless ( defined $result->{'returnbranch'} ); + $rules->{holdallowed} = 2 unless ( defined $rules->{holdallowed} ); + $rules->{hold_fulfillment_policy} = 'any' unless ( defined $rules->{hold_fulfillment_policy} ); + $rules->{returnbranch} = 'homebranch' unless ( defined $rules->{returnbranch} ); - return $result; + return $rules; } =head2 AddReturn diff --git a/admin/smart-rules.pl b/admin/smart-rules.pl index 927533d14d..41c61f733d 100755 --- a/admin/smart-rules.pl +++ b/admin/smart-rules.pl @@ -93,47 +93,124 @@ elsif ($op eq 'delete-branch-cat') { my $categorycode = $input->param('categorycode'); if ($branch eq "*") { if ($categorycode eq "*") { - my $sth_delete = $dbh->prepare("DELETE FROM default_circ_rules"); - $sth_delete->execute(); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => undef, + itemtype => undef, + rules => { + patron_maxissueqty => undef, + patron_maxonsiteissueqty => undef, + holdallowed => undef, + hold_fulfillment_policy => undef, + returnbranch => undef, + } + } + ); + } else { + Koha::CirculationRules->set_rules( + { + categorycode => $categorycode, + branchcode => undef, + itemtype => undef, + rules => { + max_holds => undef, + patron_maxissueqty => undef, + patron_maxonsiteissueqty => undef, + } + } + ); } } elsif ($categorycode eq "*") { - my $sth_delete = $dbh->prepare("DELETE FROM default_branch_circ_rules - WHERE branchcode = ?"); - $sth_delete->execute($branch); - } - Koha::CirculationRules->set_rules( - { - categorycode => $categorycode eq '*' ? undef : $categorycode, - branchcode => $branch eq '*' ? undef : $branch, - itemtype => undef, - rules => { - max_holds => undef, - patron_maxissueqty => undef, - patron_maxonsiteissueqty => undef, + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => $branch, + itemtype => undef, + rules => { + patron_maxissueqty => undef, + patron_maxonsiteissueqty => undef, + holdallowed => undef, + hold_fulfillment_policy => undef, + returnbranch => undef, + } } - } - ); + ); + } else { + Koha::CirculationRules->set_rules( + { + categorycode => $categorycode, + branchcode => $branch, + itemtype => undef, + rules => { + max_holds => undef, + patron_maxissueqty => undef, + patron_maxonsiteissueqty => undef, + } + } + ); + } } elsif ($op eq 'delete-branch-item') { my $itemtype = $input->param('itemtype'); if ($branch eq "*") { if ($itemtype eq "*") { - my $sth_delete = $dbh->prepare("DELETE FROM default_circ_rules"); - $sth_delete->execute(); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => undef, + itemtype => undef, + rules => { + patron_maxissueqty => undef, + patron_maxonsiteissueqty => undef, + holdallowed => undef, + hold_fulfillment_policy => undef, + returnbranch => undef, + } + } + ); } else { - my $sth_delete = $dbh->prepare("DELETE FROM default_branch_item_rules - WHERE itemtype = ?"); - $sth_delete->execute($itemtype); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => undef, + itemtype => $itemtype, + rules => { + holdallowed => undef, + hold_fulfillment_policy => undef, + returnbranch => undef, + } + } + ); } } elsif ($itemtype eq "*") { - my $sth_delete = $dbh->prepare("DELETE FROM default_branch_circ_rules - WHERE branchcode = ?"); - $sth_delete->execute($branch); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => $branch, + itemtype => undef, + rules => { + maxissueqty => undef, + maxonsiteissueqty => undef, + holdallowed => undef, + hold_fulfillment_policy => undef, + returnbranch => undef, + } + } + ); } else { - my $sth_delete = $dbh->prepare("DELETE FROM branch_item_rules - WHERE branchcode = ? - AND itemtype = ?"); - $sth_delete->execute($branch, $itemtype); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => $branch, + itemtype => $itemtype, + rules => { + holdallowed => undef, + hold_fulfillment_policy => undef, + returnbranch => undef, + } + } + ); } } # save the values entered @@ -256,59 +333,32 @@ elsif ($op eq "set-branch-defaults") { $max_holds = '' if $max_holds !~ /^\d+/; if ($branch eq "*") { - my $sth_search = $dbh->prepare("SELECT count(*) AS total - FROM default_circ_rules"); - my $sth_insert = $dbh->prepare("INSERT INTO default_circ_rules - (holdallowed, hold_fulfillment_policy, returnbranch) - VALUES (?, ?, ?)"); - my $sth_update = $dbh->prepare("UPDATE default_circ_rules - SET holdallowed = ?, hold_fulfillment_policy = ?, returnbranch = ?"); - - $sth_search->execute(); - my $res = $sth_search->fetchrow_hashref(); - if ($res->{total}) { - $sth_update->execute($holdallowed, $hold_fulfillment_policy, $returnbranch); - } else { - $sth_insert->execute($holdallowed, $hold_fulfillment_policy, $returnbranch); - } - Koha::CirculationRules->set_rules( { categorycode => undef, itemtype => undef, branchcode => undef, rules => { - patron_maxissueqty => $patron_maxissueqty, - patron_maxonsiteissueqty => $patron_maxonsiteissueqty, + patron_maxissueqty => $patron_maxissueqty, + patron_maxonsiteissueqty => $patron_maxonsiteissueqty, + holdallowed => $holdallowed, + hold_fulfillment_policy => $hold_fulfillment_policy, + returnbranch => $returnbranch, } } ); } else { - my $sth_search = $dbh->prepare("SELECT count(*) AS total - FROM default_branch_circ_rules - WHERE branchcode = ?"); - my $sth_insert = $dbh->prepare("INSERT INTO default_branch_circ_rules - (branchcode, holdallowed, hold_fulfillment_policy, returnbranch) - VALUES (?, ?, ?, ?)"); - my $sth_update = $dbh->prepare("UPDATE default_branch_circ_rules - SET holdallowed = ?, hold_fulfillment_policy = ?, returnbranch = ? - WHERE branchcode = ?"); - $sth_search->execute($branch); - my $res = $sth_search->fetchrow_hashref(); - if ($res->{total}) { - $sth_update->execute($holdallowed, $hold_fulfillment_policy, $returnbranch, $branch); - } else { - $sth_insert->execute($branch, $holdallowed, $hold_fulfillment_policy, $returnbranch); - } - Koha::CirculationRules->set_rules( { categorycode => undef, itemtype => undef, branchcode => $branch, rules => { - patron_maxissueqty => $patron_maxissueqty, - patron_maxonsiteissueqty => $patron_maxonsiteissueqty, + patron_maxissueqty => $patron_maxissueqty, + patron_maxonsiteissueqty => $patron_maxonsiteissueqty, + holdallowed => $holdallowed, + hold_fulfillment_policy => $hold_fulfillment_policy, + returnbranch => $returnbranch, } } ); @@ -402,76 +452,58 @@ elsif ($op eq "add-branch-item") { if ($branch eq "*") { if ($itemtype eq "*") { - my $sth_search = $dbh->prepare("SELECT count(*) AS total - FROM default_circ_rules"); - my $sth_insert = $dbh->prepare("INSERT INTO default_circ_rules - (holdallowed, hold_fulfillment_policy, returnbranch) - VALUES (?, ?, ?)"); - my $sth_update = $dbh->prepare("UPDATE default_circ_rules - SET holdallowed = ?, hold_fulfillment_policy = ?, returnbranch = ?"); - - $sth_search->execute(); - my $res = $sth_search->fetchrow_hashref(); - if ($res->{total}) { - $sth_update->execute($holdallowed, $hold_fulfillment_policy, $returnbranch); - } else { - $sth_insert->execute($holdallowed, $hold_fulfillment_policy, $returnbranch); - } + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => undef, + branchcode => undef, + rules => { + holdallowed => $holdallowed, + hold_fulfillment_policy => $hold_fulfillment_policy, + returnbranch => $returnbranch, + } + } + ); } else { - my $sth_search = $dbh->prepare("SELECT count(*) AS total - FROM default_branch_item_rules - WHERE itemtype = ?"); - my $sth_insert = $dbh->prepare("INSERT INTO default_branch_item_rules - (itemtype, holdallowed, hold_fulfillment_policy, returnbranch) - VALUES (?, ?, ?, ?)"); - my $sth_update = $dbh->prepare("UPDATE default_branch_item_rules - SET holdallowed = ?, hold_fulfillment_policy = ?, returnbranch = ? - WHERE itemtype = ?"); - $sth_search->execute($itemtype); - my $res = $sth_search->fetchrow_hashref(); - if ($res->{total}) { - $sth_update->execute($holdallowed, $hold_fulfillment_policy, $returnbranch, $itemtype); - } else { - $sth_insert->execute($itemtype, $holdallowed, $hold_fulfillment_policy, $returnbranch); - } + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => $itemtype, + branchcode => undef, + rules => { + holdallowed => $holdallowed, + hold_fulfillment_policy => $hold_fulfillment_policy, + returnbranch => $returnbranch, + } + } + ); } } elsif ($itemtype eq "*") { - my $sth_search = $dbh->prepare("SELECT count(*) AS total - FROM default_branch_circ_rules - WHERE branchcode = ?"); - my $sth_insert = $dbh->prepare("INSERT INTO default_branch_circ_rules - (branchcode, holdallowed, hold_fulfillment_policy, returnbranch) - VALUES (?, ?, ?, ?)"); - my $sth_update = $dbh->prepare("UPDATE default_branch_circ_rules - SET holdallowed = ?, hold_fulfillment_policy = ?, returnbranch = ? - WHERE branchcode = ?"); - $sth_search->execute($branch); - my $res = $sth_search->fetchrow_hashref(); - if ($res->{total}) { - $sth_update->execute($holdallowed, $hold_fulfillment_policy, $returnbranch, $branch); - } else { - $sth_insert->execute($branch, $holdallowed, $hold_fulfillment_policy, $returnbranch); - } + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => undef, + branchcode => $branch, + rules => { + holdallowed => $holdallowed, + hold_fulfillment_policy => $hold_fulfillment_policy, + returnbranch => $returnbranch, + } + } + ); } else { - my $sth_search = $dbh->prepare("SELECT count(*) AS total - FROM branch_item_rules - WHERE branchcode = ? - AND itemtype = ?"); - my $sth_insert = $dbh->prepare("INSERT INTO branch_item_rules - (branchcode, itemtype, holdallowed, hold_fulfillment_policy, returnbranch) - VALUES (?, ?, ?, ?, ?)"); - my $sth_update = $dbh->prepare("UPDATE branch_item_rules - SET holdallowed = ?, hold_fulfillment_policy = ?, returnbranch = ? - WHERE branchcode = ? - AND itemtype = ?"); - - $sth_search->execute($branch, $itemtype); - my $res = $sth_search->fetchrow_hashref(); - if ($res->{total}) { - $sth_update->execute($holdallowed, $hold_fulfillment_policy, $returnbranch, $branch, $itemtype); - } else { - $sth_insert->execute($branch, $itemtype, $holdallowed, $hold_fulfillment_policy, $returnbranch); - } + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => $itemtype, + branchcode => $branch, + rules => { + holdallowed => $holdallowed, + hold_fulfillment_policy => $hold_fulfillment_policy, + returnbranch => $returnbranch, + } + } + ); } } elsif ( $op eq 'mod-refund-lost-item-fee-rule' ) { @@ -554,76 +586,7 @@ while (my $row = $sth2->fetchrow_hashref) { my @sorted_row_loop = sort by_category_and_itemtype @row_loop; -my $sth_branch_item; -if ($branch eq "*") { - $sth_branch_item = $dbh->prepare(" - SELECT default_branch_item_rules.*, - COALESCE( localization.translation, itemtypes.description ) AS translated_description - FROM default_branch_item_rules - JOIN itemtypes USING (itemtype) - LEFT JOIN localization ON itemtypes.itemtype = localization.code - AND localization.entity = 'itemtypes' - AND localization.lang = ? - "); - $sth_branch_item->execute($language); -} else { - $sth_branch_item = $dbh->prepare(" - SELECT branch_item_rules.*, - COALESCE( localization.translation, itemtypes.description ) AS translated_description - FROM branch_item_rules - JOIN itemtypes USING (itemtype) - LEFT JOIN localization ON itemtypes.itemtype = localization.code - AND localization.entity = 'itemtypes' - AND localization.lang = ? - WHERE branch_item_rules.branchcode = ? - "); - $sth_branch_item->execute($language, $branch); -} - -my @branch_item_rules = (); -while (my $row = $sth_branch_item->fetchrow_hashref) { - push @branch_item_rules, $row; -} -my @sorted_branch_item_rules = sort { lc $a->{translated_description} cmp lc $b->{translated_description} } @branch_item_rules; - -# note undef holdallowed so that template can deal with them -foreach my $entry (@sorted_branch_item_rules) { - $entry->{holdallowed_any} = 1 if ( $entry->{holdallowed} == 2 ); - $entry->{holdallowed_same} = 1 if ( $entry->{holdallowed} == 1 ); -} - $template->param(show_branch_cat_rule_form => 1); -$template->param(branch_item_rule_loop => \@sorted_branch_item_rules); - -my $sth_defaults; -if ($branch eq "*") { - $sth_defaults = $dbh->prepare(" - SELECT * - FROM default_circ_rules - "); - $sth_defaults->execute(); -} else { - $sth_defaults = $dbh->prepare(" - SELECT * - FROM default_branch_circ_rules - WHERE branchcode = ? - "); - $sth_defaults->execute($branch); -} - -my $defaults = $sth_defaults->fetchrow_hashref; - -if ($defaults) { - $template->param( default_holdallowed_none => 1 ) if ( $defaults->{holdallowed} == 0 ); - $template->param( default_holdallowed_same => 1 ) if ( $defaults->{holdallowed} == 1 ); - $template->param( default_holdallowed_any => 1 ) if ( $defaults->{holdallowed} == 2 ); - $template->param( default_hold_fulfillment_policy => $defaults->{hold_fulfillment_policy} ); - $template->param( default_maxissueqty => $defaults->{maxissueqty} ); - $template->param( default_maxonsiteissueqty => $defaults->{maxonsiteissueqty} ); - $template->param( default_returnbranch => $defaults->{returnbranch} ); -} - -$template->param(default_rules => ($defaults ? 1 : 0)); $template->param( patron_categories => $patron_categories, diff --git a/installer/data/mysql/atomicupdate/bug_18928.perl b/installer/data/mysql/atomicupdate/bug_18928.perl new file mode 100644 index 0000000000..3fcb9d1628 --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_18928.perl @@ -0,0 +1,81 @@ +$DBversion = 'XXX'; # will be replaced by the RM +if( CheckVersion( $DBversion ) ) { + if ( column_exists( 'default_circ_rules', 'holdallowed' ) ) { + $dbh->do(" + INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) + SELECT NULL, NULL, NULL, 'holdallowed', holdallowed + FROM default_circ_rules + "); + $dbh->do(" + INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) + SELECT NULL, NULL, NULL, 'hold_fulfillment_policy', hold_fulfillment_policy + FROM default_circ_rules + "); + $dbh->do(" + INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) + SELECT NULL, NULL, NULL, 'returnbranch', returnbranch + FROM default_circ_rules + "); + $dbh->do("DROP TABLE default_circ_rules"); + } + + if ( column_exists( 'default_branch_circ_rules', 'holdallowed' ) ) { + $dbh->do(" + INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) + SELECT NULL, branchcode, NULL, 'holdallowed', holdallowed + FROM default_branch_circ_rules + "); + $dbh->do(" + INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) + SELECT NULL, branchcode, NULL, 'hold_fulfillment_policy', hold_fulfillment_policy + FROM default_branch_circ_rules + "); + $dbh->do(" + INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) + SELECT NULL, branchcode, NULL, 'returnbranch', returnbranch + FROM default_branch_circ_rules + "); + $dbh->do("DROP TABLE default_branch_circ_rules"); + } + + if ( column_exists( 'branch_item_rules', 'holdallowed' ) ) { + $dbh->do(" + INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) + SELECT NULL, branchcode, itemtype, 'holdallowed', holdallowed + FROM branch_item_rules + "); + $dbh->do(" + INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) + SELECT NULL, branchcode, itemtype, 'hold_fulfillment_policy', hold_fulfillment_policy + FROM branch_item_rules + "); + $dbh->do(" + INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) + SELECT NULL, branchcode, itemtype, 'returnbranch', returnbranch + FROM branch_item_rules + "); + $dbh->do("DROP TABLE branch_item_rules"); + } + + if ( column_exists( 'default_branch_item_rules', 'holdallowed' ) ) { + $dbh->do(" + INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) + SELECT NULL, NULL, itemtype, 'holdallowed', holdallowed + FROM default_branch_item_rules + "); + $dbh->do(" + INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) + SELECT NULL, NULL, itemtype, 'hold_fulfillment_policy', hold_fulfillment_policy + FROM default_branch_item_rules + "); + $dbh->do(" + INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) + SELECT NULL, NULL, itemtype, 'returnbranch', returnbranch + FROM default_branch_item_rules + "); + $dbh->do("DROP TABLE default_branch_item_rules"); + } + + SetVersion( $DBversion ); + print "Upgrade to $DBversion done (Bug 18928 - Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules)\n"; +} diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt index 0f26380c9b..0d78dbe16c 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt @@ -424,24 +424,31 @@ - [% IF default_hold_fulfillment_policy == 'any' %] + [% SET hold_fulfillment_policy = CirculationRules.Get( branchcode, undef, undef, 'hold_fulfillment_policy' ) %] + + + + [% IF hold_fulfillment_policy == 'any' %] @@ -459,7 +472,7 @@ [% END %] - [% IF default_hold_fulfillment_policy == 'homebranch' %] + [% IF hold_fulfillment_policy == 'homebranch' %] @@ -469,7 +482,7 @@ [% END %] - [% IF default_hold_fulfillment_policy == 'holdingbranch' %] + [% IF hold_fulfillment_policy == 'holdingbranch' %] @@ -482,21 +495,27 @@