From 7101e397c56e8ac679185a4eb2d9410529468ab8 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Tue, 26 May 2020 15:30:31 -0400 Subject: [PATCH] Bug 18086: Add FK constraints for branchcode and categorycode The overduerules table has no foreign key constraints to any for the related tables. The columns for branchcode, categorycode, letter1, letter2, and letter3 should all be foreign keys. Test Plan: 0) Create some overdue rules with invalid branchcodes and categorycodes 1) Apply this patch 2) Restart all the things! 3) Run updatedatabase.pl 4) Note the invalid rules are deleted 5) Note Default codes are NULL now instead of empty strings 6) Editor should behave as it did before --- C4/Overdues.pm | 27 +++--- Koha/OverdueRule.pm | 56 ++++++++++++ Koha/OverdueRules.pm | 48 ++++++++++ Koha/OverdueRules/TransportType.pm | 42 +++++++++ Koha/OverdueRules/TransportTypes.pm | 48 ++++++++++ .../data/mysql/atomicupdate/bug_18086.perl | 38 ++++++++ installer/data/mysql/kohastructure.sql | 8 +- tools/overduerules.pl | 89 ++++++++----------- 8 files changed, 292 insertions(+), 64 deletions(-) create mode 100644 Koha/OverdueRule.pm create mode 100644 Koha/OverdueRules.pm create mode 100644 Koha/OverdueRules/TransportType.pm create mode 100644 Koha/OverdueRules/TransportTypes.pm create mode 100644 installer/data/mysql/atomicupdate/bug_18086.perl diff --git a/C4/Overdues.pm b/C4/Overdues.pm index 5db9c22041..486c63cab8 100644 --- a/C4/Overdues.pm +++ b/C4/Overdues.pm @@ -754,18 +754,23 @@ sub GetOverduesForBranch { sub GetOverdueMessageTransportTypes { my ( $branchcode, $categorycode, $letternumber ) = @_; return unless $categorycode and $letternumber; - my $dbh = C4::Context->dbh; - my $sth = $dbh->prepare(" - SELECT message_transport_type - FROM overduerules odr LEFT JOIN overduerules_transport_types ott USING (overduerules_id) - WHERE branchcode = ? - AND categorycode = ? - AND letternumber = ? - "); - $sth->execute( $branchcode, $categorycode, $letternumber ); + my $rule = Koha::OverdueRules->search( + { + branchcode => $branchcode, + categorycode => $categorycode, + letternumber => $letternumber, + }, + { + join => 'overduerules_transport_types', + } + )->next; + + return [] unless $rule; + + my $transport_types = $rule->transport_types; my @mtts; - while ( my $mtt = $sth->fetchrow ) { - push @mtts, $mtt; + while ( my $mtt = $transport_types->next ) { + push @mtts, $mtt->message_transport_type; } # Put 'print' in first if exists diff --git a/Koha/OverdueRule.pm b/Koha/OverdueRule.pm new file mode 100644 index 0000000000..e17bf1989c --- /dev/null +++ b/Koha/OverdueRule.pm @@ -0,0 +1,56 @@ +package Koha::OverdueRule; + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use Carp; + +use Koha::Database; + +use base qw(Koha::Object); + +=head1 NAME + +Koha::OverdueRule - Koha OverdueRule Object class + +=head1 API + +=head2 Class Methods + +=head3 message_transport_types + +my $transport_types = $rule->transport_types; + +=cut + +sub transport_types { + my ( $self ) = @_; + + my $rs = $self->_result->overduerules_transport_types; + return unless $rs; + return Koha::OverdueRules::TransportTypes->_new_from_dbic($rs); +} + +=head3 type + +=cut + +sub _type { + return 'Overduerule'; +} + +1; diff --git a/Koha/OverdueRules.pm b/Koha/OverdueRules.pm new file mode 100644 index 0000000000..e602a69800 --- /dev/null +++ b/Koha/OverdueRules.pm @@ -0,0 +1,48 @@ +package Koha::OverdueRules; + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use Carp; + +use Koha::Database; + +use Koha::OverdueRule; + +use base qw(Koha::Objects); + +=head1 NAME + +Koha::OverdueRules - Koha OverdueRule Object set class + +=head1 API + +=head2 Class Methods + +=head3 _type + +=cut + +sub _type { + return 'Overduerule'; +} + +sub object_class { + return 'Koha::OverdueRule'; +} + +1; diff --git a/Koha/OverdueRules/TransportType.pm b/Koha/OverdueRules/TransportType.pm new file mode 100644 index 0000000000..fd35ee1a8c --- /dev/null +++ b/Koha/OverdueRules/TransportType.pm @@ -0,0 +1,42 @@ +package Koha::OverdueRules::TransportType; + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use Carp; + +use Koha::Database; + +use base qw(Koha::Object); + +=head1 NAME + +Koha::OverdueRules::TransportType - Koha OverdueRules::TransportType Object class + +=head1 API + +=head2 Class Methods + +=head3 _type + +=cut + +sub _type { + return 'OverduerulesTransportType'; +} + +1; diff --git a/Koha/OverdueRules/TransportTypes.pm b/Koha/OverdueRules/TransportTypes.pm new file mode 100644 index 0000000000..de0ba8997e --- /dev/null +++ b/Koha/OverdueRules/TransportTypes.pm @@ -0,0 +1,48 @@ +package Koha::OverdueRules::TransportTypes; + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use Carp; + +use Koha::Database; + +use Koha::OverdueRules::TransportType; + +use base qw(Koha::Objects); + +=head1 NAME + +Koha::OverdueRules::TransportTypes - Koha OverdueRules::TransportType Object set class + +=head1 API + +=head2 Class Methods + +=head3 _type + +=cut + +sub _type { + return 'OverduerulesTransportType'; +} + +sub object_class { + return 'Koha::OverdueRules::TransportType'; +} + +1; diff --git a/installer/data/mysql/atomicupdate/bug_18086.perl b/installer/data/mysql/atomicupdate/bug_18086.perl new file mode 100644 index 0000000000..123dbd0c89 --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_18086.perl @@ -0,0 +1,38 @@ +$DBversion = 'XXX'; # will be replaced by the RM +if( CheckVersion( $DBversion ) ) { + $dbh->do(q{ + UPDATE overduerules SET categorycode = NULL WHERE categorycode = ""; + }); + + $dbh->do(q{ + DELETE overduerules FROM overduerules + LEFT JOIN categories USING ( categorycode ) + WHERE + overduerules.categorycode IS NOT NULL + AND + categories.categorycode IS NULL; + }); + + $dbh->do(q{ + UPDATE overduerules SET branchcode = NULL WHERE branchcode = ""; + }); + + $dbh->do(q{ + DELETE overduerules FROM overduerules + LEFT JOIN branches USING ( branchcode ) + WHERE + branches.branchcode IS NULL + AND + overduerules.branchcode IS NOT NULL + }); + + $dbh->do(q{ + ALTER TABLE overduerules + MODIFY `branchcode` varchar(10) NULL default NULL, + MODIFY `categorycode` varchar(10) NOT NULL, + ADD CONSTRAINT `fk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE CASCADE ON UPDATE CASCADE, + ADD CONSTRAINT `fk_categorycode` FOREIGN KEY (`categorycode`) REFERENCES `categories` (`categorycode`) ON DELETE CASCADE ON UPDATE CASCADE; + }); + + NewVersion( $DBversion, 18086, "Add FK constraints for branchcode and categorycode"); +} diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index 6487ec1fcc..3c4d5793f2 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -3974,8 +3974,8 @@ DROP TABLE IF EXISTS `overduerules`; /*!40101 SET character_set_client = utf8 */; CREATE TABLE `overduerules` ( `overduerules_id` int(11) NOT NULL AUTO_INCREMENT COMMENT 'unique identifier for the overduerules', - `branchcode` varchar(10) COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT '' COMMENT 'foreign key from the branches table to define which branch this rule is for (if blank it''s all libraries)', - `categorycode` varchar(10) COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT '' COMMENT 'foreign key from the categories table to define which patron category this rule is for', + `branchcode` varchar(10) COLLATE utf8mb4_unicode_ci NULL DEFAULT NULL COMMENT 'foreign key from the branches table to define which branch this rule is for (if blank it''s all libraries)', + `categorycode` varchar(10) COLLATE utf8mb4_unicode_ci NOT NULL COMMENT 'foreign key from the categories table to define which patron category this rule is for', `delay1` int(4) DEFAULT NULL COMMENT 'number of days after the item is overdue that the first notice is sent', `letter1` varchar(20) COLLATE utf8mb4_unicode_ci DEFAULT NULL COMMENT 'foreign key from the letter table to define which notice should be sent as the first notice', `debarred1` varchar(1) COLLATE utf8mb4_unicode_ci DEFAULT '0' COMMENT 'is the patron restricted when the first notice is sent (1 for yes, 0 for no)', @@ -3986,7 +3986,9 @@ CREATE TABLE `overduerules` ( `letter3` varchar(20) COLLATE utf8mb4_unicode_ci DEFAULT NULL COMMENT 'foreign key from the letter table to define which notice should be sent as the third notice', `debarred3` int(1) DEFAULT 0 COMMENT 'is the patron restricted when the third notice is sent (1 for yes, 0 for no)', PRIMARY KEY (`overduerules_id`), - UNIQUE KEY `overduerules_branch_cat` (`branchcode`,`categorycode`) + UNIQUE KEY `overduerules_branch_cat` (`branchcode`,`categorycode`), + CONSTRAINT `fk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE CASCADE ON UPDATE CASCADE, + CONSTRAINT `fk_categorycode` FOREIGN KEY (`categorycode`) REFERENCES `categories` (`categorycode`) ON DELETE CASCADE ON UPDATE CASCADE ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; /*!40101 SET character_set_client = @saved_cs_client */; diff --git a/tools/overduerules.pl b/tools/overduerules.pl index 1e5d920a07..3e5e8bdbdf 100755 --- a/tools/overduerules.pl +++ b/tools/overduerules.pl @@ -26,7 +26,8 @@ use C4::Letters; use C4::Members; use C4::Overdues qw( GetOverdueMessageTransportTypes ); use Koha::Libraries; - +use Koha::OverdueRules; +use Koha::OverdueRules::TransportTypes; use Koha::Patron::Categories; our $input = CGI->new; @@ -73,7 +74,7 @@ $branch = : C4::Context->preference('DefaultToLoggedInLibraryOverdueTriggers') ? C4::Context::mybranch() : Koha::Libraries->search->count() == 1 ? undef : undef; -$branch ||= q{}; +$branch ||= undef; my $op = $input->param('op'); $op ||= q{}; @@ -85,22 +86,6 @@ my %temphash; my $input_saved = 0; if ($op eq 'save') { my @names=$input->multi_param(); - my $sth_search = $dbh->prepare("SELECT count(*) AS total FROM overduerules WHERE branchcode=? AND categorycode=?"); - - my $sth_insert = $dbh->prepare("INSERT INTO overduerules (branchcode,categorycode, delay1,letter1,debarred1, delay2,letter2,debarred2, delay3,letter3,debarred3) VALUES (?,?,?,?,?,?,?,?,?,?,?)"); - my $sth_update=$dbh->prepare("UPDATE overduerules SET delay1=?, letter1=?, debarred1=?, delay2=?, letter2=?, debarred2=?, delay3=?, letter3=?, debarred3=? WHERE branchcode=? AND categorycode=?"); - my $sth_delete=$dbh->prepare("DELETE FROM overduerules WHERE branchcode=? AND categorycode=?"); - my $sth_insert_mtt = $dbh->prepare(" - INSERT INTO overduerules_transport_types( - overduerules_id, letternumber, message_transport_type - ) VALUES ( - (SELECT overduerules_id FROM overduerules WHERE branchcode = ? AND categorycode = ?), ?, ? - ) - "); - my $sth_delete_mtt = $dbh->prepare(" - DELETE FROM overduerules_transport_types - WHERE overduerules_id = (SELECT overduerules_id FROM overduerules WHERE branchcode = ? AND categorycode = ?) - "); foreach my $key (@names){ # ISSUES @@ -156,41 +141,45 @@ if ($op eq 'save') { if (($temphash{$bor}->{delay1} and ($temphash{$bor}->{"letter1"} or $temphash{$bor}->{"debarred1"})) or ($temphash{$bor}->{delay2} and ($temphash{$bor}->{"letter2"} or $temphash{$bor}->{"debarred2"})) or ($temphash{$bor}->{delay3} and ($temphash{$bor}->{"letter3"} or $temphash{$bor}->{"debarred3"}))) { - $sth_search->execute($branch,$bor); - my $res = $sth_search->fetchrow_hashref(); - if ($res->{'total'}>0) { - $sth_update->execute( - ($temphash{$bor}->{"delay1"}?$temphash{$bor}->{"delay1"}:undef), - ($temphash{$bor}->{"letter1"}?$temphash{$bor}->{"letter1"}:""), - ($temphash{$bor}->{"debarred1"}?$temphash{$bor}->{"debarred1"}:0), - ($temphash{$bor}->{"delay2"}?$temphash{$bor}->{"delay2"}:undef), - ($temphash{$bor}->{"letter2"}?$temphash{$bor}->{"letter2"}:""), - ($temphash{$bor}->{"debarred2"}?$temphash{$bor}->{"debarred2"}:0), - ($temphash{$bor}->{"delay3"}?$temphash{$bor}->{"delay3"}:undef), - ($temphash{$bor}->{"letter3"}?$temphash{$bor}->{"letter3"}:""), - ($temphash{$bor}->{"debarred3"}?$temphash{$bor}->{"debarred3"}:0), - $branch ,$bor - ); + my $overdue_rule = Koha::OverdueRules->find({ branchcode => $branch, categorycode => $bor }); + if ( $overdue_rule ) { + $overdue_rule->update({ + delay1 => $temphash{$bor}->{"delay1"} ? $temphash{$bor}->{"delay1"} : undef, + letter1 => $temphash{$bor}->{"letter1"} ? $temphash{$bor}->{"letter1"} : "", + debarred1 => $temphash{$bor}->{"debarred1"} ? $temphash{$bor}->{"debarred1"} : 0, + delay2 => $temphash{$bor}->{"delay2"} ? $temphash{$bor}->{"delay2"} : undef, + letter2 => $temphash{$bor}->{"letter2"} ? $temphash{$bor}->{"letter2"} : "", + debarred2 => $temphash{$bor}->{"debarred2"} ? $temphash{$bor}->{"debarred2"} : 0, + delay3 => $temphash{$bor}->{"delay3"} ? $temphash{$bor}->{"delay3"} : undef, + letter3 => $temphash{$bor}->{"letter3"} ? $temphash{$bor}->{"letter3"} : "", + debarred3 => $temphash{$bor}->{"debarred3"} ? $temphash{$bor}->{"debarred3"} : 0, + }); } else { - $sth_insert->execute($branch,$bor, - ($temphash{$bor}->{"delay1"}?$temphash{$bor}->{"delay1"}:0), - ($temphash{$bor}->{"letter1"}?$temphash{$bor}->{"letter1"}:""), - ($temphash{$bor}->{"debarred1"}?$temphash{$bor}->{"debarred1"}:0), - ($temphash{$bor}->{"delay2"}?$temphash{$bor}->{"delay2"}:0), - ($temphash{$bor}->{"letter2"}?$temphash{$bor}->{"letter2"}:""), - ($temphash{$bor}->{"debarred2"}?$temphash{$bor}->{"debarred2"}:0), - ($temphash{$bor}->{"delay3"}?$temphash{$bor}->{"delay3"}:0), - ($temphash{$bor}->{"letter3"}?$temphash{$bor}->{"letter3"}:""), - ($temphash{$bor}->{"debarred3"}?$temphash{$bor}->{"debarred3"}:0) - ); + $overdue_rule = Koha::OverdueRule->new({ + branchcode => $branch, + categorycode => $bor, + delay1 => $temphash{$bor}->{"delay1"} ? $temphash{$bor}->{"delay1"} : undef , + letter1 => $temphash{$bor}->{"letter1"} ? $temphash{$bor}->{"letter1"} : "" , + debarred1 => $temphash{$bor}->{"debarred1"} ? $temphash{$bor}->{"debarred1"} : 0 , + delay2 => $temphash{$bor}->{"delay2"} ? $temphash{$bor}->{"delay2"} : undef , + letter2 => $temphash{$bor}->{"letter2"} ? $temphash{$bor}->{"letter2"} : "" , + debarred2 => $temphash{$bor}->{"debarred2"} ? $temphash{$bor}->{"debarred2"} : 0 , + delay3 => $temphash{$bor}->{"delay3"} ? $temphash{$bor}->{"delay3"} : undef , + letter3 => $temphash{$bor}->{"letter3"} ? $temphash{$bor}->{"letter3"} : "" , + debarred3 => $temphash{$bor}->{"debarred3"} ? $temphash{$bor}->{"debarred3"} : 0 , + })->store(); } - $sth_delete_mtt->execute( $branch, $bor ); + Koha::OverdueRules::TransportTypes->search({ overduerules_id => $overdue_rule->id })->delete(); for my $letternumber ( 1..3 ) { my @mtt = $input->multi_param( "mtt${letternumber}-$bor" ); next unless @mtt; for my $mtt ( @mtt ) { - $sth_insert_mtt->execute( $branch, $bor, $letternumber, $mtt); + Koha::OverdueRules::TransportType->new({ + letternumber => $letternumber, + message_transport_type => $mtt, + overduerules_id => $overdue_rule->id, + })->store(); } } } @@ -198,7 +187,8 @@ if ($op eq 'save') { } unless ($err) { for my $category_code (@rows_to_delete) { - $sth_delete->execute($branch, $category_code); + my $overdue_rule = Koha::OverdueRules->find({ branchcode => $branch, categorycode => $category_code }); + $overdue_rule->delete if $overdue_rule; } $template->param(datasaved => 1); $input_saved = 1; @@ -247,9 +237,8 @@ for my $patron_category (@patron_categories) { } } else { #getting values from table - my $sth2=$dbh->prepare("SELECT * from overduerules WHERE branchcode=? AND categorycode=?"); - $sth2->execute($branch,$patron_category->categorycode); - my $dat=$sth2->fetchrow_hashref; + my $overdue_rule = Koha::OverdueRules->find({ branchcode => $branch, categorycode => $patron_category->categorycode }); + my $dat = $overdue_rule ? $overdue_rule->unblessed : {}; for my $i ( 1..3 ){ my %row = ( overduename => $patron_category->categorycode, -- 2.20.1