Bugzilla – Attachment 29050 Details for
Bug 11742
Overdue notice/status triggers displaying the wrong notice for default
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
[PASSED QA] Bug 11742: A letter code should be unique.
PASSED-QA-Bug-11742-A-letter-code-should-be-unique.patch (text/plain), 19.58 KB, created by
Katrin Fischer
on 2014-06-19 15:24:00 UTC
(
hide
)
Description:
[PASSED QA] Bug 11742: A letter code should be unique.
Filename:
MIME Type:
Creator:
Katrin Fischer
Created:
2014-06-19 15:24:00 UTC
Size:
19.58 KB
patch
obsolete
>From f5285b992d3a40f2e4cd743306518e37c3756627 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@biblibre.com> >Date: Wed, 7 May 2014 16:12:39 +0200 >Subject: [PATCH] [PASSED QA] Bug 11742: A letter code should be unique. > >This patch is a dirty way to fix a design issue on notices. >Currently the code assumes that a letter code is unique. Which is wrong, >the primary key is module, code, branchcode. > >Maybe we should add a primary key (id) for the letter table in order to >pass the id to the template and correctly manage the letter code >duplication. > >Test plan: >Try to duplicate a letter code using edit, add and copy actions. >If you manage to do it, please describe how you did. > >Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> >Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> >--- > C4/Letters.pm | 2 + > .../intranet-tmpl/prog/en/modules/tools/letter.tt | 92 +++++++++++++++------- > .../prog/en/modules/tools/overduerules.tt | 12 +-- > svc/letters | 63 +++++++++++++++ > tools/letter.pl | 62 ++++++--------- > 5 files changed, 154 insertions(+), 77 deletions(-) > create mode 100755 svc/letters > >diff --git a/C4/Letters.pm b/C4/Letters.pm >index 4e0ea20..ff63aac 100644 >--- a/C4/Letters.pm >+++ b/C4/Letters.pm >@@ -73,6 +73,7 @@ C4::Letters - Give functions for Letters management > sub GetLetters { > my ($filters) = @_; > my $module = $filters->{module}; >+ my $code = $filters->{code}; > my $dbh = C4::Context->dbh; > my $letters = $dbh->selectall_arrayref( > q| >@@ -81,6 +82,7 @@ sub GetLetters { > WHERE 1 > | > . ( $module ? q| AND module = ?| : q|| ) >+ . ( $code ? q| AND code = ?| : q|| ) > . q| GROUP BY code ORDER BY name|, { Slice => {} } > , ( $module ? $module : () ) > ); >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 b6bdf88..34df1e7 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt >@@ -1,6 +1,6 @@ > [% USE Koha %] > [% INCLUDE 'doc-head-open.inc' %] >-<title>Koha › Tools › Notices[% IF ( add_form ) %][% IF ( modify ) %] › Modify notice[% ELSE %] › Add notice[% END %][% END %][% IF ( add_validate ) %] › Notice added[% END %][% IF ( delete_confirm ) %] › Confirm deletion[% END %]</title> >+<title>Koha › Tools › Notices[% IF ( add_form or copy_form ) %][% IF ( modify ) %] › Modify notice[% ELSE %] › Add notice[% END %][% END %][% IF ( add_validate or copy_validate) %] › Notice added[% END %][% IF ( delete_confirm ) %] › Confirm deletion[% END %]</title> > [% INCLUDE 'doc-head-close.inc' %] > <link rel="stylesheet" type="text/css" href="[% themelang %]/css/datatables.css" /> > [% INCLUDE 'datatables.inc' %] >@@ -26,7 +26,8 @@ $(document).ready(function() { > }); > [% END %] > >- $("#submit").click( function(event) { >+ $("#submit_form").click( function(event) { >+ event.preventDefault(); > var at_least_one_exists = 0; > $("fieldset.mtt").each( function(){ > var title = $(this).find('input[name="title"]').val(); >@@ -40,7 +41,6 @@ $(document).ready(function() { > msg = msg.replace( "%s", mtt ); > at_least_one_exists = 1; > alert(msg) >- event.preventDefault(); > return false; > } else if ( title.length > 0 && content.length > 0 ) { > at_least_one_exists = 1; >@@ -48,10 +48,34 @@ $(document).ready(function() { > } ); > if ( ! at_least_one_exists ) { > alert( _("Please fill at least one template.") ); >- event.preventDefault(); > return false; > } >- return true; >+ >+ // 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 %] >+ if ( new_lettercode != '[% code %]' ) { >+ $.ajax({ >+ data: { code: new_lettercode }, >+ 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.") ); >+ return false; >+ } else { >+ $("#add_notice").submit(); >+ } >+ }, >+ }); >+ } else { >+ $("#add_notice").submit(); >+ } > }); > > var sms_limit = 160; >@@ -65,7 +89,7 @@ $(document).ready(function() { > } > }); > }); >-[% IF ( add_form ) %] >+[% IF add_form or copy_form %] > > function cancel(f) { > $('#op').val(""); >@@ -117,9 +141,9 @@ $(document).ready(function() { > [% INCLUDE 'header.inc' %] > [% INCLUDE 'letters-search.inc' %] > >-<div id="breadcrumbs"><a href="/cgi-bin/koha/mainpage.pl">Home</a> › <a href="/cgi-bin/koha/tools/tools-home.pl">Tools</a> › [% IF ( add_form ) %][% IF ( modify ) %]<a href="/cgi-bin/koha/tools/letter.pl">Notices & Slips</a> › Modify notice[% ELSE %] <a href="/cgi-bin/koha/tools/letter.pl">Notices & Slips</a> › Add notice[% END %][% ELSE %][% IF ( add_validate ) %] <a href="/cgi-bin/koha/tools/letter.pl">Notices & Slips</a> › Notice added[% ELSE %][% IF ( delete_confirm ) %] <a href="/cgi-bin/koha/tools/letter.pl">Notices & Slips</a> › Confirm deletion[% ELSE %]Notices & Slips[% END %][% END %][% END %]</div> >+<div id="breadcrumbs"><a href="/cgi-bin/koha/mainpage.pl">Home</a> › <a href="/cgi-bin/koha/tools/tools-home.pl">Tools</a> › [% IF ( add_form or copy_form) %][% IF ( modify ) %]<a href="/cgi-bin/koha/tools/letter.pl">Notices & Slips</a> › Modify notice[% ELSE %] <a href="/cgi-bin/koha/tools/letter.pl">Notices & Slips</a> › Add notice[% END %][% ELSE %][% IF ( add_validate or copy_validate) %] <a href="/cgi-bin/koha/tools/letter.pl">Notices & Slips</a> › Notice added[% ELSE %][% IF ( delete_confirm ) %] <a href="/cgi-bin/koha/tools/letter.pl">Notices & Slips</a> › Confirm deletion[% ELSE %]Notices & Slips[% END %][% END %][% END %]</div> > >-[% IF ( add_form ) %]<div id="doc" class="yui-t7">[% ELSE %]<div id="doc3" class="yui-t2">[% END %] >+[% IF add_form or copy_form %]<div id="doc" class="yui-t7">[% ELSE %]<div id="doc3" class="yui-t2">[% END %] > > <div id="bd"> > <div id="yui-main"> >@@ -182,7 +206,7 @@ $(document).ready(function() { > <td style="white-space: nowrap"> > [% IF !independant_branch || !lette.branchcode %] > <form method="post" action="/cgi-bin/koha/tools/letter.pl"> >- <input type="hidden" name="op" value="copy" /> >+ <input type="hidden" name="op" value="copy_form" /> > <input type="hidden" name="oldbranchcode" value="[% lette.branchcode %]" /> > <input type="hidden" name="module" value="[% lette.module %]" /> > <input type="hidden" name="code" value="[% lette.code %]" /> >@@ -221,10 +245,15 @@ $(document).ready(function() { > [% END %] > > >-[% IF ( add_form ) %] >+[% IF add_form or copy_form %] > <h1>[% IF ( modify ) %]Modify notice[% ELSE %]Add notice[% END %]</h1> >- <form name="Aform" method="post" enctype="multipart/form-data" class="validate"> >- <input type="hidden" name="op" id="op" value="add_validate" /> >+ <form id="add_notice" name="Aform" method="post" enctype="multipart/form-data" class="validate"> >+ [% IF add_form %] >+ <input type="hidden" name="op" id="op" value="add_validate" /> >+ [% ELSE %] >+ <input type="hidden" name="op" id="op" value="copy_validate" /> >+ [% END %] >+ > <input type="hidden" name="checked" value="0" /> > [% IF ( modify ) %] > <input type="hidden" name="add" value="0" /> >@@ -232,7 +261,7 @@ $(document).ready(function() { > <input type="hidden" name="add" value="1" /> > [% END %] > <fieldset class="rows"> >- <input type="hidden" name="oldbranchcode" value="[% branchcode %]" /> >+ <input type="hidden" name="oldbranchcode" value="[% oldbranchcode %]" /> > [% IF independant_branch %] > <input type="hidden" name="branchcode" value="[% independant_branch %]" /> > [% ELSE %] >@@ -250,7 +279,11 @@ $(document).ready(function() { > <li> > <label for="module">Koha module:</label> > <input type="hidden" name="oldmodule" value="[% module %]" /> >- [% IF ( modify ) %]<select name="module" id="module">[% END %] [% IF ( adding ) %] <select name="module" id="module" onchange="javascript:window.location.href = unescape(window.location.pathname)+'?op=add_form&module='+this.value+'&content='+window.document.forms['Aform'].elements['content'].value;">[% END %] >+ [% IF adding %] >+ <select name="module" id="module" onchange="javascript:window.location.href = unescape(window.location.pathname)+'?op=add_form&module='+this.value+'&content='+window.document.forms['Aform'].elements['content'].value;"> >+ [% ELSE %] >+ <select name="module" id="module"> >+ [% END %] > [% IF ( module == "catalogue" ) %] > <option value="catalogue" selected="selected">Catalog</option> > [% ELSE %] >@@ -294,20 +327,19 @@ $(document).ready(function() { > </select> > </li> > <li> >- [% IF adding %] >- <label for"code" class="required">Code:</label> >- <input type="text" id="code" name="code" size="20" maxlength="20" required="required" /> >- <span class="required">Required</span> >- [% ELSE %] >- <label for="code">Code:</label> >- <input type="hidden" id="code" name="code" value="[% code %]" />[% code %] >- [% END %] >+ <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. >+ [% END %] >+ <input type="hidden" id="code" name="oldcode" value="[% oldcode %]" /> > </li> >- <li> >- <label for="name" class="required">Name:</label> >- <input type="text" id="name" name="name" size="60" value="[% letter_name %]" required="required" /> >- <span class="required">Required</span> >- </li> >+ <li> >+ <label for="name" class="required">Name:</label> >+ <input type="text" id="name" name="name" size="60" value="[% letter_name %]" required="required" /> >+ <span class="required">Required</span> >+ </li> > > [% FOREACH letter IN letters %] > <li> >@@ -376,12 +408,12 @@ $(document).ready(function() { > > [% IF code.search('DGST') %] <span class="overdue">Warning, this is a template for a Digest, as such, any references to branch data ( e.g. branches.branchname ) will refer to the borrower's home branch.</span> [% END %] > </fieldset> >- <fieldset class="action"><input type="submit" id="submit" value="Submit" class="button" /> <a class="cancel" href="/cgi-bin/koha/tools/letter.pl">Cancel</a></fieldset> >+ <fieldset class="action"><input type="submit" id="submit_form" value="Submit" class="button" /> <a class="cancel" href="/cgi-bin/koha/tools/letter.pl">Cancel</a></fieldset> > <input type="hidden" name="searchfield" value="[% searchfield %]" /> > </form> > [% END %] > >-[% IF ( add_validate ) %] >+[% IF ( add_validate or copy_validate) %] > Data recorded > <form action="[% action %]" method="post"> > <input type="submit" value="OK" /> >@@ -431,7 +463,7 @@ $(document).ready(function() { > > </div> > </div> >-[% UNLESS ( add_form ) %] >+[% UNLESS add_form or copy_form %] > <div class="yui-b noprint"> > [% INCLUDE 'tools-menu.inc' %] > </div> >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/overduerules.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/overduerules.tt >index 8c109a5..e561859 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/overduerules.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/overduerules.tt >@@ -120,20 +120,16 @@ $(document).ready(function() { > <input type="text" name="delay[% tab.number %]-[% value.overduename %]" size="5" value="[% value.delay %]" /> > </td> > <td> >- [% IF ( value.noletter ) %] >- <input type="text" name="letter[% tab.number %]-[% value.overduename %]" value="[% value.letter %]" /> >- [% ELSE %] > <select name="letter[% tab.number %]-[% value.overduename %]"> > <option value="">No notice</option> >- [% FOREACH letterloop IN value.letterloop %] >- [% IF ( letterloop.selected ) %] >- <option value="[% letterloop.value %]" selected="selected">[% letterloop.lettername %]</option> >+ [% FOREACH letter IN letters %] >+ [% IF letter.code == value.selected_lettercode %] >+ <option value="[% letter.code %]" selected="selected">[% letter.name %]</option> > [% ELSE %] >- <option value="[% letterloop.value %]">[% letterloop.lettername %]</option> >+ <option value="[% letter.code %]">[% letter.name %]</option> > [% END %] > [% END %] > </select> >- [% END %] > </td> > <td> > [% IF ( value.debarred ) %] >diff --git a/svc/letters b/svc/letters >new file mode 100755 >index 0000000..695fe68 >--- /dev/null >+++ b/svc/letters >@@ -0,0 +1,63 @@ >+#!/usr/bin/perl >+ >+# This file is part of Koha. >+# >+# Copyright 2014 BibLibre >+# >+# 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 <http://www.gnu.org/licenses>. >+ >+use Modern::Perl; >+ >+use C4::Service; >+use C4::Letters qw( GetLetters ); >+ >+=head1 NAME >+ >+svc/letters - Web service for getting letters >+ >+=head1 SYNOPSIS >+ >+ GET /svc/letters >+ >+=head1 DESCRIPTION >+ >+For the moment, this service is only used to get a letter from a letter code. >+ >+=head1 METHODS >+ >+=cut >+ >+=head2 set_preference >+ >+=over 4 >+ >+GET /svc/letters/$code >+ >+=back >+ >+Used to get letters with a given letter code. >+ >+=cut >+ >+our ( $query, $response ) = C4::Service->init( tools => 'edit_notices' ); >+ >+sub get_letters { >+ my $letters = GetLetters({ code => $query->param('code') }); >+ $response->param( letters => $letters ); >+ C4::Service->return_success( $response ); >+} >+ >+C4::Service->dispatch( >+ [ 'GET /', [ 'code' ], \&get_letters ], >+); >diff --git a/tools/letter.pl b/tools/letter.pl >index 06666de..c19d995 100755 >--- a/tools/letter.pl >+++ b/tools/letter.pl >@@ -78,7 +78,6 @@ sub get_letters { > 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 { >@@ -121,17 +120,26 @@ $template->param( > action => $script_name > ); > >-if ($op eq 'copy') { >- add_copy(); >- $op = 'add_form'; >+if ( $op eq 'add_validate' or $op eq 'copy_validate' ) { >+ add_validate(); >+ $op = q{}; # we return to the default screen for the next operation > } >- >-if ($op eq 'add_form') { >- add_form($branchcode, $module, $code); >+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, >+ ); > } >-elsif ( $op eq 'add_validate' ) { >- add_validate(); >- $op = q{}; # next operation is to return to default screen >+elsif ( $op eq 'add_form' ) { >+ add_form($branchcode, $module, $code); > } > elsif ( $op eq 'delete_confirm' ) { > delete_confirm($branchcode, $module, $code); >@@ -258,7 +266,6 @@ sub add_form { > > sub add_validate { > my $dbh = C4::Context->dbh; >- my $oldbranchcode = $input->param('oldbranchcode'); > my $branchcode = $input->param('branchcode') || ''; > my $module = $input->param('module'); > my $oldmodule = $input->param('oldmodule'); >@@ -271,12 +278,12 @@ sub add_validate { > my $is_html = $input->param("is_html_$mtt"); > my $title = shift @title; > my $content = shift @content; >- my $letter = get_letters($oldbranchcode,$oldmodule, $code, $mtt); >+ my $letter = get_letters($branchcode,$oldmodule, $code, $mtt); > unless ( $title and $content ) { >- delete_confirmed( $oldbranchcode, $oldmodule, $code, $mtt ); >+ delete_confirmed( $branchcode, $oldmodule, $code, $mtt ); > next; > } >- if ( exists $letter->{$mtt} ) { >+ elsif ( exists $letter->{$mtt} ) { > $dbh->do( > q{ > UPDATE letter >@@ -285,7 +292,7 @@ sub add_validate { > }, > undef, > $branchcode, $module, $name, $is_html || 0, $title, $content, >- $oldbranchcode, $oldmodule, $code, $mtt >+ $branchcode, $oldmodule, $code, $mtt > ); > } else { > $dbh->do( >@@ -297,30 +304,7 @@ sub add_validate { > } > # set up default display > default_display($branchcode); >-} >- >-sub add_copy { >- my $dbh = C4::Context->dbh; >- my $oldbranchcode = $input->param('oldbranchcode'); >- my $branchcode = $input->param('branchcode'); >- my $module = $input->param('module'); >- my $code = $input->param('code'); >- >- return if keys %{ get_letters($branchcode,$module, $code, '*') }; >- >- my $old_letters = get_letters($oldbranchcode,$module, $code, '*'); >- >- my $message_transport_types = GetMessageTransportTypes(); >- for my $mtt ( @$message_transport_types ) { >- next unless exists $old_letters->{$mtt}; >- my $old_letter = $old_letters->{$mtt}; >- >- $dbh->do( >- q{INSERT INTO letter (branchcode,module,code,name,is_html,title,content,message_transport_type) VALUES (?,?,?,?,?,?,?,?)}, >- undef, >- $branchcode, $module, $code, $old_letter->{name}, $old_letter->{is_html}, $old_letter->{title}, $old_letter->{content}, $mtt >- ); >- } >+ return 1; > } > > sub delete_confirm { >-- >1.8.3.2
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 11742
:
25892
|
28020
|
28021
|
28687
|
28688
|
28729
|
28956
|
29049
| 29050 |
29051
|
29180