The patches submitted on bug 11742 tried to fix an issue based on a (very) assertion: letter.code should be considered as a primary key and should be uniq. This is completely wrong, we can have a code for the default letter (branchcode=='') and the same code for a specific library. The interface should not block the creation of new letter with the same letter code.
Created attachment 33316 [details] [review] 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) assertion: 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).
(In reply to Jonathan Druart from comment #1) > The patches submitted on bug 11742 tried to fix an issue based on a > (very) assertion: letter.code should be considered as a primary key and > should be uniq. ...based on a (very) *bad* assertion...
I'm wondering if this would pass QA. Marcel suggested that things with SQL in them might not pass QA here: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8753#c64 Just thought I'd note this here to, perhaps, get clarification of whether the added functions should have explicit SQL queries in them like they do.
I am not sure how the discussion on bug 8753 relates to this patch - I think the point there was to not have SQL in the .pl files? Joubu moved a lot of the logic into the module and wrote tests. This seems like a major improvement of the code in terms of coding guidelines. Also, there is a difference between new features and trying to improve old code.
(In reply to Katrin Fischer from comment #4) > I am not sure how the discussion on bug 8753 relates to this patch - I think > the point there was to not have SQL in the .pl files? So, SQL in .pm files is okay? I thought we were trying to move to DBIx as much as possible.
(In reply to M. Tompsett from comment #5) > (In reply to Katrin Fischer from comment #4) > > I am not sure how the discussion on bug 8753 relates to this patch - I think > > the point there was to not have SQL in the .pl files? > > So, SQL in .pm files is okay? I thought we were trying to move to DBIx as > much as possible. Mark, you are right about using DBIx. I'll talk to Jonathan about it. I might push it as-is if the release date approaches, because it is important to have it ASAP integrated for the beta.
(In reply to Katrin Fischer from comment #4) > This seems like a major > improvement of the code in terms of coding guidelines. It is. I'm not critiquing that. Frankly, the code is beautiful. > Also, there is a > difference between new features and trying to improve old code. &GetLettersAvailableForALibrary &GetLetterTemplates &DelLetter These are new functions, which is why I was asking about DBIx. Thank you for the reply, Tomas. I'll work on signing these off if needed, just let me know.
This is a quite sensitive bug and need to be fixed before the release. Since we don't have a clear and documented way to use DBIC in our Koha namespace, I preferred to use the old-way: C4 and DBI (and avoid discussion as on bug 12892). Moreover the Koha::Letter module does not exist yet and I tried to add as less changes as possible.
Comment on attachment 33316 [details] [review] Bug 13215: The same letter code can be used for several libraries Review of attachment 33316 [details] [review]: ----------------------------------------------------------------- ::: C4/Letters.pm @@ +123,5 @@ > + FROM letter > + WHERE module = ? > + AND code = ? > + and branchcode = ? > + | http://wiki.koha-community.org/wiki/Coding_Guidelines#SQL9:_SELECT Does SQL9 rule apply? @@ +163,5 @@ > + WHERE 1 > + | > + . q| AND branchcode = ''| > + . ( $module ? q| AND module = ?| : q|| ) > + . q| ORDER BY name|, { Slice => {} } Does SQL9 rule apply? @@ +177,5 @@ > + WHERE 1 > + | > + . q| AND branchcode = ?| > + . ( $module ? q| AND module = ?| : q|| ) > + . q| ORDER BY name|, { Slice => {} } Does SQL9 rule apply? @@ +264,5 @@ > + DELETE FROM letter > + WHERE branchcode = ? > + AND module = ? > + AND code = ? > + | . ( $mtt ? q| AND message_transport_type = ?| : q|| ) Does SQL9 rule apply? ::: t/db_dependent/Letters/GetLetterTemplates.t @@ +95,5 @@ > + }, > +]; > + > +my $sth = $dbh->prepare( > +q|INSERT INTO letter(module, code, branchcode, name, title, content, message_transport_type) VALUES (?, ?, ?, ?, ?, ?, ?)| See SQL9 rule, I believe. ::: t/db_dependent/Letters/GetLettersAvailableForALibrary.t @@ +95,5 @@ > + }, > +]; > + > +my $sth = $dbh->prepare( > +q|INSERT INTO letter(module, code, branchcode, name, title, content, message_transport_type) VALUES (?, ?, ?, ?, ?, ?, ?)| SQL9 rule, I believe. ::: tools/letter.pl @@ +115,4 @@ > delete_confirm($branchcode, $module, $code); > } > elsif ( $op eq 'delete_confirmed' ) { > + delete_confirmed($branchcode, $module, $code); mtt was removed, because it is never passed, correct?
(In reply to M. Tompsett from comment #9) > Comment on attachment 33316 [details] [review] [review] > Bug 13215: The same letter code can be used for several libraries > Does SQL9 rule apply? Not sure this is used. Some queries are constructed dynamically, it's not possible to write them using the rules. For the test files, I used perltidy. > ::: tools/letter.pl > @@ +115,4 @@ > > delete_confirm($branchcode, $module, $code); > > } > > elsif ( $op eq 'delete_confirmed' ) { > > + delete_confirmed($branchcode, $module, $code); > > mtt was removed, because it is never passed, correct? Yes, it's not possible to delete only 1 template, all templates should be removed when a letter is removed.
Created attachment 33637 [details] [review] 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 <chris@bigballofwax.co.nz>
Only got as far as running all the tests this morning - they all pass. We need this urgently, but it needs good testing. If someone has time, please help out.
(In reply to Katrin Fischer from comment #12) > Only got as far as running all the tests this morning - they all pass. We > need this urgently, but it needs good testing. If someone has time, please > help out. Yes, a second signoff on this one won't be too much.
Started testing: 1) Notices and slips - can copy a notice to another branch with the same code - cannot copy a notice to another branch, when notice with the same code already exists for the branch ? Copy action no longer allows to change the letter code, is this by intention? ? If you try and change a letter at a specific branch, the change is not saved [Fri Nov 21 08:28:53.142175 2014] [cgi:error] [pid 6494] [client 127.0.0.1:55971] AH01215: [Fri Nov 21 08:28:53 2014] letter.pl: DBD::mysql::db do failed: Duplicate entry 'circulation-ODUE-FPL-print' for key 'PRIMARY' at /home/katrin/kohaclone/tools/letter.pl line 274., referer: http://localhost:8080/cgi-bin/koha/tools/letter.pl?op=add_form&branchcode=FPL&module=circulation&code=ODUE I had one ODUE in default, and 1 for Fairview.
Created attachment 33772 [details] [review] Bug 13215: Fix notice edition C4::Letters::getletter does not set mtt in value ( i.e. { email => "my email notice} ) at the contrary of the get_letter routine defined in tools/letters.pl.
Created attachment 33773 [details] [review] Bug 13215: Fix notice deletion This patch could have only been - name => $values[0]->{name}, + name => $letter->{name}, Other changes are just indentation and variable names (send an hashref $letter to the template and use the Branches TT plugin to display the branch name)
* I can't copy a notice from default to a branch. * As Katrin stated, code on a copy is not editable. Is this a bug?
Created attachment 33788 [details] [review] Bug 13215: (follow-up) Fix notice edition
(In reply to Kyle M Hall from comment #17) > * I can't copy a notice from default to a branch. Should be fixed now. > * As Katrin stated, code on a copy is not editable. Is this a bug? No, it's intended. I don't see why someone would like to change the code on copying. And it avoids writing complicated code. I know it's not a good reason, but few days before the release, to fix a critical bug, I think it is. Back to needs signoff, we definitely need more testing on this patch set.
I agree, first of all we need to fix the main problem. 2 thoughts about editing the code of a notice, though those should maybe go on a separate bug report later: - In master it's currently possible, but I haven't checked in 3.16. - For overdues where you might have multiple notices for 1st/2nd/3rd notice and per patron category - so there it would be ergonomical to copy and use another code.
I have the following issue: 1) Create a notice e.g with code DELETEBUG for All libraries Fill in Subject and body of one message type, e.g. for print Submit Check with MySQL for contents 2) Edit the same notice Name is empty (!) Subject for body and message are empty. Fill it again. Edit print Message subject and body are empty (!) Fill in Message subject again, but not message body Submit You get a message "Please specify title and content for print" Confim with OK Result: List of Notices and Slips is loaded. Notice with code DELETEBUG is deleted! Confirm deletion with MySQL
Created attachment 33909 [details] [review] Bug 13215: Fix GetLetterTemplates should return default templates if branchcode is not defined
(In reply to Marc Véron from comment #21) > I have the following issue: Thanks for testing Marc. That should be fixed now.
Created attachment 33930 [details] [review] 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 <chris@bigballofwax.co.nz>
Created attachment 33931 [details] [review] Bug 13215: Fix notice deletion This patch could have only been - name => $values[0]->{name}, + name => $letter->{name}, Other changes are just indentation and variable names (send an hashref $letter to the template and use the Branches TT plugin to display the branch name) Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 33932 [details] [review] Bug 13215: (follow-up) Fix notice edition Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 33933 [details] [review] Bug 13215: Fix GetLetterTemplates should return default templates if branchcode is not defined Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 33934 [details] [review] Bug 13215: Fix notice edition C4::Letters::getletter does not set mtt in value ( i.e. { email => "my email notice} ) at the contrary of the get_letter routine defined in tools/letters.pl. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 33935 [details] [review] 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 <chris@bigballofwax.co.nz>
Created attachment 33936 [details] [review] Bug 13215: Fix notice edition C4::Letters::getletter does not set mtt in value ( i.e. { email => "my email notice} ) at the contrary of the get_letter routine defined in tools/letters.pl. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 33937 [details] [review] Bug 13215: Fix notice deletion This patch could have only been - name => $values[0]->{name}, + name => $letter->{name}, Other changes are just indentation and variable names (send an hashref $letter to the template and use the Branches TT plugin to display the branch name) Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 33938 [details] [review] Bug 13215: (follow-up) Fix notice edition Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 33939 [details] [review] Bug 13215: Fix GetLetterTemplates should return default templates if branchcode is not defined Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
I have started testing again, these seem to work alright for me: - Copying a notice to another branch Note: You can not copy to 'default' only to a real library. This is the behaviour before and after the patch. - Edit a notice. - Delete a notice from "Default" leaves the other notices. - Delete a notice from the library. - Add a new notice. - !Add a notice with the same code at the same branch Note: The error message has the branchcode, could be a little bit nicer with the branch name, definitely not a blocker. So far this looks alright, running out of time now, but will test next where the notices are selected/used (overdue notice triggers, serial claims etc.) Please feel free to jump in.
Overdue notice triggers - Default: shows only default notices - Library: shows default and library specific notices. If there are 2 notices with the same code, the library specific is shown. Acq and serial claims - Only shows default notice, if there are 2 of the same code. (ACQCLAIM) - Only shows branch specific if the code is unique. I think this looks not quite right, BUT behaviour is them same as without the patch.
Created attachment 33971 [details] [review] [PASSED QA] 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 <chris@bigballofwax.co.nz> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 33972 [details] [review] [PASSED QA] Bug 13215: Fix notice edition C4::Letters::getletter does not set mtt in value ( i.e. { email => "my email notice} ) at the contrary of the get_letter routine defined in tools/letters.pl. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 33973 [details] [review] [PASSED QA] Bug 13215: Fix notice deletion This patch could have only been - name => $values[0]->{name}, + name => $letter->{name}, Other changes are just indentation and variable names (send an hashref $letter to the template and use the Branches TT plugin to display the branch name) Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 33974 [details] [review] [PASSED QA] Bug 13215: (follow-up) Fix notice edition Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 33975 [details] [review] [PASSED QA] Bug 13215: Fix GetLetterTemplates should return default templates if branchcode is not defined Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
(In reply to Katrin Fischer from comment #35) > Overdue notice triggers > - Default: shows only default notices > - Library: shows default and library specific notices. > If there are 2 notices with the same code, the library specific is shown. > > Acq and serial claims > - Only shows default notice, if there are 2 of the same code. (ACQCLAIM) > - Only shows branch specific if the code is unique. > > I think this looks not quite right, BUT behaviour is them same as without > the patch. ... and the 'not quite right' is only meant for the acq and serials, the overdues make sense to me.
(In reply to Katrin Fischer from comment #35) > Acq and serial claims > - Only shows default notice, if there are 2 of the same code. (ACQCLAIM) > - Only shows branch specific if the code is unique. > > I think this looks not quite right, BUT behaviour is them same as without > the patch. Yes, the library specific notices are only used for notices sent to patrons (maybe this is not exhaustive).
Created attachment 33995 [details] [review] Bug 13215: Fix GetLetterTemplates should return default templates if branchcode is not defined
(In reply to Jonathan Druart from comment #43) > Created attachment 33995 [details] [review] [review] > Bug 13215: Fix GetLetterTemplates should return default templates if > branchcode is not defined 1 line was missing in this patch (change in C4::Letters).
Patches pushed to master. Thanks Jonathan!