Commit 6f599652 authored by joubu's avatar joubu Committed by Tomas Cohen Arazi

Bug 13215: The same letter code can be used for several libraries

This patch fixes a major issue introduced by the
commit 5c4fdcf7 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's avatarChris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Katrin Fischer's avatarKatrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi's avatarTomas Cohen Arazi <tomascohen@gmail.com>
parent 2973e20c
......@@ -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 :
......
......@@ -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() {
</select>
</li>
<li>
<label for="code" class="required">Code:</label>
<input type="text" id="code" name="code" size="20" maxlength="20" value="[% code %]" required="required"/>
<span class="required">Required</span>
[% IF copying %]
You must change this code to reflect the copy.
[% IF adding %]
<label for="code" class="required">Code:</label>
<input type="text" id="code" name="code" size="20" maxlength="20" value="" required="required"/>
<span class="required">Required</span>
[% ELSE %]
<label for="code">Code:</label>
<input type="hidden" id="code" name="code" size="20" maxlength="20" value="[% code %]" required="required"/>
[% code %]
[% END %]
<input type="hidden" name="oldcode" value="[% oldcode %]" />
</li>
<li>
<label for="name" class="required">Name:</label>
......@@ -448,7 +448,6 @@ $(document).ready(function() {
<input type="hidden" name="branchcode" value="[% branchcode %]" />
<input type="hidden" name="code" value="[% code %]" />
<input type="hidden" name="module" value="[% module %]" />
<input type="hidden" name="message_transport_type" value="*" />
<input type="submit" value="Yes, delete" class="approve" />
</form>
......
......@@ -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 );
}
......
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' );
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'
);
......@@ -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;
......
......@@ -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;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment