From c28e1bc8a30076ea6d577f8299ac9422e9791e24 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 6 Nov 2014 16:02:08 +0100 Subject: [PATCH] Bug 13215: The same letter code can be used for several libraries This patch fixes a major issue introduced by the commit 5c4fdcf Bug 11742: A letter code should be unique. The interface should let the possibility to create a default template letter and some specific ones, with the same letter code (letter.code). The patches submitted on bug 11742 tried to fix an issue based on a (very bad) assumption: letter.code should be considered as a primary key and should be uniq. This patch reintroduces this behavior. Note that the interface will block a letter code used in different module (this is consistent not to have the same letter code used for different needs). This patch is absolutely not perfect, it just tries to change as less change as possible and to use new tested subroutines. Test plan: 1/ Verify that the problem raised on bug 11742 does not appears anymore. 2/ Verify there are no regression on adding, editing, copying, deleting letters. 3/ Verify you are allowed to create a default letter template with a letter code and to reuse for a specific letter (i.e. for a given library). Signed-off-by: Chris Cormack --- C4/Letters.pm | 141 ++++++++++++++- .../intranet-tmpl/prog/en/modules/tools/letter.tt | 31 ++-- svc/letters | 2 +- t/db_dependent/Letters/GetLetterTemplates.t | 125 +++++++++++++ .../Letters/GetLettersAvailableForALibrary.t | 195 +++++++++++++++++++++ tools/letter.pl | 57 ++---- tools/overduerules.pl | 7 +- 7 files changed, 501 insertions(+), 57 deletions(-) create mode 100644 t/db_dependent/Letters/GetLetterTemplates.t create mode 100644 t/db_dependent/Letters/GetLettersAvailableForALibrary.t diff --git a/C4/Letters.pm b/C4/Letters.pm index 050ec85..0b307f3 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -44,7 +44,7 @@ BEGIN { $VERSION = 3.07.00.049; @ISA = qw(Exporter); @EXPORT = qw( - &GetLetters &GetPreparedLetter &GetWrappedLetter &addalert &getalert &delalert &findrelatedto &SendAlerts &GetPrintMessages &GetMessageTransportTypes + &GetLetters &GetLettersAvailableForALibrary &GetLetterTemplates &DelLetter &GetPreparedLetter &GetWrappedLetter &addalert &getalert &delalert &findrelatedto &SendAlerts &GetPrintMessages &GetMessageTransportTypes ); } @@ -75,6 +75,7 @@ sub GetLetters { my ($filters) = @_; my $module = $filters->{module}; my $code = $filters->{code}; + my $branchcode = $filters->{branchcode}; my $dbh = C4::Context->dbh; my $letters = $dbh->selectall_arrayref( q| @@ -84,14 +85,120 @@ sub GetLetters { | . ( $module ? q| AND module = ?| : q|| ) . ( $code ? q| AND code = ?| : q|| ) + . ( defined $branchcode ? q| AND branchcode = ?| : q|| ) . q| GROUP BY code ORDER BY name|, { Slice => {} } , ( $module ? $module : () ) , ( $code ? $code : () ) + , ( defined $branchcode ? $branchcode : () ) ); return $letters; } +=head2 GetLetterTemplates + + my $letter_templates = GetLetterTemplates( + { + module => 'circulation', + code => 'my code', + branchcode => 'CPL', # '' for default, + } + ); + + Return a hashref of letter templates. + The key will be the message transport type. + +=cut + +sub GetLetterTemplates { + my ( $params ) = @_; + + my $module = $params->{module}; + my $code = $params->{code}; + my $branchcode = $params->{branchcode}; + my $dbh = C4::Context->dbh; + my $letters = $dbh->selectall_hashref( + q| + SELECT module, code, branchcode, name, is_html, title, content, message_transport_type + FROM letter + WHERE module = ? + AND code = ? + and branchcode = ? + | + , 'message_transport_type' + , undef + , $module, $code, $branchcode + ); + + return $letters; +} + +=head2 GetLettersAvailableForALibrary + + my $letters = GetLettersAvailableForALibrary( + { + branchcode => 'CPL', # '' for default + module => 'circulation', + } + ); + + Return an arrayref of letters, sorted by name. + If a specific letter exist for the given branchcode, it will be retrieve. + Otherwise the default letter will be. + +=cut + +sub GetLettersAvailableForALibrary { + my ($filters) = @_; + my $branchcode = $filters->{branchcode}; + my $module = $filters->{module}; + + croak "module should be provided" unless $module; + + my $dbh = C4::Context->dbh; + my $default_letters = $dbh->selectall_arrayref( + q| + SELECT module, code, branchcode, name + FROM letter + WHERE 1 + | + . q| AND branchcode = ''| + . ( $module ? q| AND module = ?| : q|| ) + . q| ORDER BY name|, { Slice => {} } + , ( $module ? $module : () ) + ); + + my $specific_letters; + if ($branchcode) { + $specific_letters = $dbh->selectall_arrayref( + q| + SELECT module, code, branchcode, name + FROM letter + WHERE 1 + | + . q| AND branchcode = ?| + . ( $module ? q| AND module = ?| : q|| ) + . q| ORDER BY name|, { Slice => {} } + , $branchcode + , ( $module ? $module : () ) + ); + } + + my %letters; + for my $l (@$default_letters) { + $letters{ $l->{code} } = $l; + } + for my $l (@$specific_letters) { + # Overwrite the default letter with the specific one. + $letters{ $l->{code} } = $l; + } + + return [ map { $letters{$_} } + sort { $letters{$a}->{name} cmp $letters{$b}->{name} } + keys %letters ]; + +} + # FIXME: using our here means that a Plack server will need to be # restarted fairly regularly when working with this routine. # A better option would be to use Koha::Cache and use a cache @@ -130,6 +237,38 @@ sub getletter { return { %$line }; } +=head2 DelLetter + + DelLetter( + { + branchcode => 'CPL', + module => 'circulation', + code => 'my code', + [ mtt => 'email', ] + } + ); + + Delete the letter. The mtt parameter is facultative. + If not given, all templates mathing the other parameters will be removed. + +=cut + +sub DelLetter { + my ($params) = @_; + my $branchcode = $params->{branchcode}; + my $module = $params->{module}; + my $code = $params->{code}; + my $mtt = $params->{mtt}; + my $dbh = C4::Context->dbh; + $dbh->do(q| + DELETE FROM letter + WHERE branchcode = ? + AND module = ? + AND code = ? + | . ( $mtt ? q| AND message_transport_type = ?| : q|| ) + , undef, $branchcode, $module, $code, ( $mtt ? $mtt : () ) ); +} + =head2 addalert ($borrowernumber, $type, $externalid) parameters : diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt index 08d3ec5..7c68f22 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt @@ -53,23 +53,21 @@ $(document).ready(function() { // Test if code already exists in DB var new_lettercode = $("#code").val(); - [% IF copy_form %] - if ( new_lettercode == '[% code %]' ) { - alert( _("Please change the code.") ); - return false; - } - [% END %] - + var new_branchcode = $("#branch").val(); [% IF ( add_form and code ) # IF edit %] if ( new_lettercode != '[% code %]' ) { [% END %] $.ajax({ - data: { code: new_lettercode }, + data: { code: new_lettercode, branchcode: new_branchcode }, type: 'GET', url: '/cgi-bin/koha/svc/letters/', success: function (data) { if ( data.letters.length > 0 ) { - alert( _("This letter code is already used for another letter.") ); + if( new_branchcode == '' ) { + alert( _("A default letter with the code '%s' already exists.").format(new_lettercode) ); + } else { + alert( _("A letter with the code '%s' already exists for '%s'.").format(new_lettercode, new_branchcode) ); + } return false; } else { $("#add_notice").submit(); @@ -333,13 +331,15 @@ $(document).ready(function() {
  • - - - Required - [% IF copying %] - You must change this code to reflect the copy. + [% IF adding %] + + + Required + [% ELSE %] + + + [% code %] [% END %] -
  • @@ -448,7 +448,6 @@ $(document).ready(function() { - diff --git a/svc/letters b/svc/letters index 695fe68..cfb77e7 100755 --- a/svc/letters +++ b/svc/letters @@ -53,7 +53,7 @@ Used to get letters with a given letter code. our ( $query, $response ) = C4::Service->init( tools => 'edit_notices' ); sub get_letters { - my $letters = GetLetters({ code => $query->param('code') }); + my $letters = GetLetters({ code => $query->param('code'), branchcode => $query->param('branchcode') }); $response->param( letters => $letters ); C4::Service->return_success( $response ); } diff --git a/t/db_dependent/Letters/GetLetterTemplates.t b/t/db_dependent/Letters/GetLetterTemplates.t new file mode 100644 index 0000000..acde0a6 --- /dev/null +++ b/t/db_dependent/Letters/GetLetterTemplates.t @@ -0,0 +1,125 @@ +use Modern::Perl; +use Test::More tests => 6; + +use C4::Context; +use C4::Letters qw( GetLetterTemplates ); + +my $dbh = C4::Context->dbh; +$dbh->{RaiseError} = 1; +$dbh->{AutoCommit} = 0; + +$dbh->do(q|DELETE FROM letter|); + +my $letters = [ + { + module => 'circulation', + code => 'code1', + branchcode => '', + name => 'B default name for code1 circ', + is_html => 0, + title => 'default title for code1 email', + content => 'default content for code1 email', + message_transport_type => 'email', + }, + { + module => 'circulation', + code => 'code1', + branchcode => '', + name => 'B default name for code1 circ', + is_html => 0, + title => 'default title for code1 sms', + content => 'default content for code1 sms', + message_transport_type => 'sms', + }, + { + module => 'circulation', + code => 'code2', + branchcode => '', + name => 'A default name for code2 circ', + is_html => 0, + title => 'default title for code2 email', + content => 'default content for code2 email', + message_transport_type => 'email', + }, + { + module => 'circulation', + code => 'code3', + branchcode => '', + name => 'C default name for code3 circ', + is_html => 0, + title => 'default title for code3 email', + content => 'default content for code3 email', + message_transport_type => 'email', + }, + + { + module => 'cataloguing', + code => 'code1', + branchcode => '', + name => 'default name for code1 cat', + is_html => 0, + title => 'default title for code1 cat email', + content => 'default content for code1 cat email', + message_transport_type => 'email', + }, + + { + module => 'circulation', + code => 'code1', + branchcode => 'CPL', + name => 'B CPL name for code1 circ', + is_html => 0, + title => 'CPL title for code1 email', + content => 'CPL content for code1 email', + message_transport_type => 'email', + }, + { + module => 'circulation', + code => 'code2', + branchcode => 'CPL', + name => 'A CPL name for code1 circ', + is_html => 0, + title => 'CPL title for code1 sms', + content => 'CPL content for code1 sms', + message_transport_type => 'sms', + }, + { + module => 'circulation', + code => 'code1', + branchcode => 'MPL', + name => 'B MPL name for code1 circ', + is_html => 0, + title => 'MPL title for code1 email', + content => 'MPL content for code1 email', + message_transport_type => 'email', + }, +]; + +my $sth = $dbh->prepare( +q|INSERT INTO letter(module, code, branchcode, name, title, content, message_transport_type) VALUES (?, ?, ?, ?, ?, ?, ?)| +); +for my $l (@$letters) { + $sth->execute( $l->{module}, $l->{code}, $l->{branchcode}, $l->{name}, + $l->{title}, $l->{content}, $l->{message_transport_type} ); +} + +my $letter_templates; +$letter_templates = C4::Letters::GetLetterTemplates; +is_deeply( $letter_templates, {}, + 'GetLetterTemplates should not return templates if not param is given' ); + +$letter_templates = C4::Letters::GetLetterTemplates( + { module => 'circulation', code => 'code1', branchcode => '' } ); +is( scalar( keys %$letter_templates ), + 2, '2 default templates should exist for circulation code1' ); +is( exists( $letter_templates->{email} ), + 1, 'The mtt email should exist for circulation code1' ); +is( exists( $letter_templates->{sms} ), + 1, 'The mtt sms should exist for circulation code1' ); + +$letter_templates = C4::Letters::GetLetterTemplates( + { module => 'circulation', code => 'code1', branchcode => 'CPL' } ); +is( scalar( keys %$letter_templates ), + 1, '1 template should exist for circulation CPL code1' ); +is( exists( $letter_templates->{email} ), + 1, 'The mtt should be email for circulation CPL code1' ); diff --git a/t/db_dependent/Letters/GetLettersAvailableForALibrary.t b/t/db_dependent/Letters/GetLettersAvailableForALibrary.t new file mode 100644 index 0000000..4649a33 --- /dev/null +++ b/t/db_dependent/Letters/GetLettersAvailableForALibrary.t @@ -0,0 +1,195 @@ +use Modern::Perl; +use Test::More tests => 19; + +use C4::Context; +use C4::Letters qw( GetLettersAvailableForALibrary DelLetter ); + +my $dbh = C4::Context->dbh; +$dbh->{RaiseError} = 1; +$dbh->{AutoCommit} = 0; + +$dbh->do(q|DELETE FROM letter|); + +my $letters = [ + { + module => 'circulation', + code => 'code1', + branchcode => '', + name => 'B default name for code1 circ', + is_html => 0, + title => 'default title for code1 email', + content => 'default content for code1 email', + message_transport_type => 'email', + }, + { + module => 'circulation', + code => 'code1', + branchcode => '', + name => 'B default name for code1 circ', + is_html => 0, + title => 'default title for code1 sms', + content => 'default content for code1 sms', + message_transport_type => 'sms', + }, + { + module => 'circulation', + code => 'code2', + branchcode => '', + name => 'A default name for code2 circ', + is_html => 0, + title => 'default title for code2 email', + content => 'default content for code2 email', + message_transport_type => 'email', + }, + { + module => 'circulation', + code => 'code3', + branchcode => '', + name => 'C default name for code3 circ', + is_html => 0, + title => 'default title for code3 email', + content => 'default content for code3 email', + message_transport_type => 'email', + }, + + { + module => 'cataloguing', + code => 'code1', + branchcode => '', + name => 'default name for code1 cat', + is_html => 0, + title => 'default title for code1 cat email', + content => 'default content for code1 cat email', + message_transport_type => 'email', + }, + + { + module => 'circulation', + code => 'code1', + branchcode => 'CPL', + name => 'B CPL name for code1 circ', + is_html => 0, + title => 'CPL title for code1 email', + content => 'CPL content for code1 email', + message_transport_type => 'email', + }, + { + module => 'circulation', + code => 'code2', + branchcode => 'CPL', + name => 'A CPL name for code1 circ', + is_html => 0, + title => 'CPL title for code1 sms', + content => 'CPL content for code1 sms', + message_transport_type => 'sms', + }, + { + module => 'circulation', + code => 'code1', + branchcode => 'MPL', + name => 'B MPL name for code1 circ', + is_html => 0, + title => 'MPL title for code1 email', + content => 'MPL content for code1 email', + message_transport_type => 'email', + }, +]; + +my $sth = $dbh->prepare( +q|INSERT INTO letter(module, code, branchcode, name, title, content, message_transport_type) VALUES (?, ?, ?, ?, ?, ?, ?)| +); +for my $l (@$letters) { + $sth->execute( $l->{module}, $l->{code}, $l->{branchcode}, $l->{name}, + $l->{title}, $l->{content}, $l->{message_transport_type} ); + + # GetLettersAvailableForALibrary does not return these fields + delete $l->{title}; + delete $l->{content}; + delete $l->{is_html}; + delete $l->{message_transport_type}; +} + +my $available_letters; +$available_letters = + C4::Letters::GetLettersAvailableForALibrary( { module => 'circulation' } ); +is( scalar(@$available_letters), + 3, 'There should be 3 default letters for circulation (3 distinct codes)' ); + +$available_letters = C4::Letters::GetLettersAvailableForALibrary( + { module => 'circulation', branchcode => '' } ); +is( scalar(@$available_letters), 3, +'There should be 3 default letters for circulation (3 distinct codes), branchcode=""' +); +is_deeply( $available_letters->[0], + $letters->[2], 'The letters should be sorted by name (A)' ); +is_deeply( $available_letters->[1], + $letters->[0], 'The letters should be sorted by name (B)' ); +is_deeply( $available_letters->[2], + $letters->[3], 'The letters should be sorted by name (C)' ); + +$available_letters = C4::Letters::GetLettersAvailableForALibrary( + { module => 'circulation', branchcode => 'CPL' } ); +is( scalar(@$available_letters), 3, +'There should be 3 default letters for circulation (3 distinct codes), branchcode="CPL"' +); +is_deeply( $available_letters->[0], + $letters->[6], 'The letters should be sorted by name (A)' ); +is_deeply( $available_letters->[1], + $letters->[5], 'The letters should be sorted by name (B)' ); +is_deeply( $available_letters->[2], + $letters->[3], 'The letters should be sorted by name (C)' ); + +$available_letters = C4::Letters::GetLettersAvailableForALibrary( + { module => 'circulation', branchcode => 'MPL' } ); +is( scalar(@$available_letters), 3, +'There should be 3 default letters for circulation (3 distinct codes), branchcode="CPL"' +); +is_deeply( $available_letters->[0], + $letters->[2], 'The letters should be sorted by name (A)' ); +is_deeply( $available_letters->[1], + $letters->[7], 'The letters should be sorted by name (B)' ); +is_deeply( $available_letters->[2], + $letters->[3], 'The letters should be sorted by name (C)' ); + +my $letters_by_module = C4::Letters::GetLetters( { module => 'circulation' } ); +is( scalar(@$letters_by_module), + 3, '3 different letter codes exist for circulation' ); + +my $letters_by_branchcode = C4::Letters::GetLetters( { branchcode => 'CPL' } ); +is( scalar(@$letters_by_branchcode), + 2, '2 different letter codes exist for CPL' ); + +# On the way, we test DelLetter +is( + C4::Letters::DelLetter( + { module => 'cataloguing', code => 'code1', branchcode => 'MPL' } + ), + '0E0', + 'No letter exist for MPL cat/code1' +); +is( + C4::Letters::DelLetter( + { module => 'circulation', code => 'code1', branchcode => '' } + ), + 2, + '2 default letters existed for circ/code1 (1 for email and 1 for sms)' +); +is( + C4::Letters::DelLetter( + { + module => 'circulation', + code => 'code1', + branchcode => 'CPL', + mtt => 'email' + } + ), + 1, + '1 letter existed for CPL circ/code1/email' +); +is( + C4::Letters::DelLetter( + { module => 'circulation', code => 'code1', branchcode => 'MPL' } + ), + 1, + '1 letter existed for MPL circ/code1' +); diff --git a/tools/letter.pl b/tools/letter.pl index 82c6f87..0f2b501 100755 --- a/tools/letter.pl +++ b/tools/letter.pl @@ -50,34 +50,6 @@ use C4::Branch; # GetBranches use C4::Letters; use C4::Members::Attributes; -# _letter_from_where($branchcode,$module, $code, $mtt) -# - return FROM WHERE clause and bind args for a letter -sub _letter_from_where { - my ($branchcode, $module, $code, $mtt) = @_; - my $sql = q{FROM letter WHERE branchcode = ? AND module = ? AND code = ?}; - $sql .= q{ AND message_transport_type = ?} if $mtt ne '*'; - my @args = ( $branchcode || '', $module, $code, ($mtt ne '*' ? $mtt : ()) ); -# Mysql is retarded. cause branchcode is part of the primary key it cannot be null. How does that -# work with foreign key constraint I wonder... - -# if ($branchcode) { -# $sql .= " AND branchcode = ?"; -# push @args, $branchcode; -# } else { -# $sql .= " AND branchcode IS NULL"; -# } - - return ($sql, \@args); -} - -# get_letters($branchcode,$module, $code, $mtt) -# - return letters with the given $branchcode, $module, $code and $mtt exists -sub get_letters { - my ($sql, $args) = _letter_from_where(@_); - my $dbh = C4::Context->dbh; - my $letter = $dbh->selectall_hashref("SELECT * $sql", 'message_transport_type', undef, @$args); - return $letter; -} # $protected_letters = protected_letters() # - return a hashref of letter_codes representing letters that should never be deleted sub protected_letters { @@ -127,13 +99,11 @@ if ( $op eq 'add_validate' or $op eq 'copy_validate' ) { if ($op eq 'copy_form') { my $oldbranchcode = $input->param('oldbranchcode') || q||; my $branchcode = $input->param('branchcode') || q||; - my $oldcode = $input->param('oldcode') || $input->param('code'); add_form($oldbranchcode, $module, $code); $template->param( oldbranchcode => $oldbranchcode, branchcode => $branchcode, branchloop => _branchloop($branchcode), - oldcode => $oldcode, copying => 1, modify => 0, ); @@ -145,8 +115,7 @@ elsif ( $op eq 'delete_confirm' ) { delete_confirm($branchcode, $module, $code); } elsif ( $op eq 'delete_confirmed' ) { - my $mtt = $input->param('message_transport_type'); - delete_confirmed($branchcode, $module, $code, $mtt); + delete_confirmed($branchcode, $module, $code); $op = q{}; # next operation is to return to default screen } else { @@ -168,7 +137,13 @@ sub add_form { my $letters; # if code has been passed we can identify letter and its an update action if ($code) { - $letters = get_letters($branchcode,$module, $code, '*'); + $letters = C4::Letters::GetLetterTemplates( + { + branchcode => $branchcode, + module => $module, + code => $code, + } + ); } my $message_transport_types = GetMessageTransportTypes(); @@ -278,8 +253,9 @@ sub add_validate { my $is_html = $input->param("is_html_$mtt"); my $title = shift @title; my $content = shift @content; - my $letter = get_letters($branchcode,$oldmodule, $code, $mtt); + my $letter = C4::Letters::getletter( $oldmodule, $code, $branchcode, $mtt); unless ( $title and $content ) { + # Delete this mtt if no title or content given delete_confirmed( $branchcode, $oldmodule, $code, $mtt ); next; } @@ -310,7 +286,7 @@ sub add_validate { sub delete_confirm { my ($branchcode, $module, $code) = @_; my $dbh = C4::Context->dbh; - my $letter = get_letters($branchcode, $module, $code, '*'); + my $letter = C4::Letters::getletter($module, $code, $branchcode); my @values = values %$letter; $template->param( branchcode => $branchcode, @@ -324,9 +300,14 @@ sub delete_confirm { sub delete_confirmed { my ($branchcode, $module, $code, $mtt) = @_; - my ($sql, $args) = _letter_from_where($branchcode, $module, $code, $mtt); - my $dbh = C4::Context->dbh; - $dbh->do("DELETE $sql", undef, @$args); + C4::Letters::DelLetter( + { + branchcode => $branchcode, + module => $module, + code => $code, + mtt => $mtt + } + ); # setup default display for screen default_display($branchcode); return; diff --git a/tools/overduerules.pl b/tools/overduerules.pl index 24019a5..a824bd4 100755 --- a/tools/overduerules.pl +++ b/tools/overduerules.pl @@ -201,7 +201,12 @@ if ($op eq 'save') { } my $branchloop = GetBranchesLoop($branch); -my $letters = GetLetters({ module => "circulation" }); +my $letters = C4::Letters::GetLettersAvailableForALibrary( + { + branchcode => $branch, + module => "circulation", + } +); my @line_loop; -- 2.1.0