Bug 11742: A letter code should be unique.
authorJonathan Druart <jonathan.druart@biblibre.com>
Wed, 7 May 2014 14:12:39 +0000 (16:12 +0200)
committerTomas Cohen Arazi <tomascohen@gmail.com>
Mon, 23 Jun 2014 18:19:55 +0000 (15:19 -0300)
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
koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt
koha-tmpl/intranet-tmpl/prog/en/modules/tools/overduerules.tt
svc/letters [new file with mode: 0755]
tools/letter.pl

index 4e0ea20..ff63aac 100644 (file)
@@ -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 : () )
     );
index b6bdf88..34df1e7 100644 (file)
@@ -1,6 +1,6 @@
 [% USE Koha %]
 [% INCLUDE 'doc-head-open.inc' %]
-<title>Koha &rsaquo; Tools &rsaquo; Notices[% IF ( add_form ) %][% IF ( modify ) %] &rsaquo; Modify notice[% ELSE %] &rsaquo; Add notice[% END %][% END %][% IF ( add_validate ) %] &rsaquo; Notice added[% END %][% IF ( delete_confirm ) %] &rsaquo; Confirm deletion[% END %]</title>
+<title>Koha &rsaquo; Tools &rsaquo; Notices[% IF ( add_form or copy_form ) %][% IF ( modify ) %] &rsaquo; Modify notice[% ELSE %] &rsaquo; Add notice[% END %][% END %][% IF ( add_validate or copy_validate) %] &rsaquo; Notice added[% END %][% IF ( delete_confirm ) %] &rsaquo; 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> &rsaquo; <a href="/cgi-bin/koha/tools/tools-home.pl">Tools</a> &rsaquo; [% IF ( add_form ) %][% IF ( modify ) %]<a href="/cgi-bin/koha/tools/letter.pl">Notices &amp; Slips</a> &rsaquo; Modify notice[% ELSE %] <a href="/cgi-bin/koha/tools/letter.pl">Notices &amp; Slips</a> &rsaquo; Add notice[% END %][% ELSE %][% IF ( add_validate ) %] <a href="/cgi-bin/koha/tools/letter.pl">Notices &amp; Slips</a> &rsaquo; Notice added[% ELSE %][% IF ( delete_confirm ) %] <a href="/cgi-bin/koha/tools/letter.pl">Notices &amp; Slips</a> &rsaquo; Confirm deletion[% ELSE %]Notices &amp; Slips[% END %][% END %][% END %]</div>
+<div id="breadcrumbs"><a href="/cgi-bin/koha/mainpage.pl">Home</a> &rsaquo; <a href="/cgi-bin/koha/tools/tools-home.pl">Tools</a> &rsaquo; [% IF ( add_form or copy_form) %][% IF ( modify ) %]<a href="/cgi-bin/koha/tools/letter.pl">Notices &amp; Slips</a> &rsaquo; Modify notice[% ELSE %] <a href="/cgi-bin/koha/tools/letter.pl">Notices &amp; Slips</a> &rsaquo; Add notice[% END %][% ELSE %][% IF ( add_validate or copy_validate) %] <a href="/cgi-bin/koha/tools/letter.pl">Notices &amp; Slips</a> &rsaquo; Notice added[% ELSE %][% IF ( delete_confirm ) %] <a href="/cgi-bin/koha/tools/letter.pl">Notices &amp; Slips</a> &rsaquo; Confirm deletion[% ELSE %]Notices &amp; 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&amp;module='+this.value+'&amp;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&amp;module='+this.value+'&amp;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>
index 8c109a5..e561859 100644 (file)
@@ -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 (executable)
index 0000000..695fe68
--- /dev/null
@@ -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 ],
+);
index 06666de..c19d995 100755 (executable)
@@ -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 {