Bug 13215 - letter.code is considered as a primary key
Summary: letter.code is considered as a primary key
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 11742
Blocks:
  Show dependency treegraph
 
Reported: 2014-11-06 15:40 UTC by Jonathan Druart
Modified: 2019-06-27 09:24 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 13215: The same letter code can be used for several libraries (28.16 KB, patch)
2014-11-06 15:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13215: The same letter code can be used for several libraries (28.22 KB, patch)
2014-11-18 06:13 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13215: Fix notice edition (899 bytes, patch)
2014-11-21 09:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13215: Fix notice deletion (4.13 KB, patch)
2014-11-21 09:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13215: (follow-up) Fix notice edition (1.32 KB, patch)
2014-11-21 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13215: Fix GetLetterTemplates should return default templates if branchcode is not defined (1.23 KB, patch)
2014-11-25 14:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13215: The same letter code can be used for several libraries (28.22 KB, patch)
2014-11-26 02:20 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13215: Fix notice deletion (4.19 KB, patch)
2014-11-26 02:21 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13215: (follow-up) Fix notice edition (1.37 KB, patch)
2014-11-26 02:21 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13215: Fix GetLetterTemplates should return default templates if branchcode is not defined (1.29 KB, patch)
2014-11-26 02:22 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13215: Fix notice edition (956 bytes, patch)
2014-11-26 02:23 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13215: The same letter code can be used for several libraries (28.22 KB, patch)
2014-11-26 02:24 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13215: Fix notice edition (956 bytes, patch)
2014-11-26 02:24 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13215: Fix notice deletion (4.19 KB, patch)
2014-11-26 02:24 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13215: (follow-up) Fix notice edition (1.37 KB, patch)
2014-11-26 02:24 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13215: Fix GetLetterTemplates should return default templates if branchcode is not defined (1.29 KB, patch)
2014-11-26 02:25 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 13215: The same letter code can be used for several libraries (28.29 KB, patch)
2014-11-27 07:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13215: Fix notice edition (1.00 KB, patch)
2014-11-27 07:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13215: Fix notice deletion (4.26 KB, patch)
2014-11-27 07:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13215: (follow-up) Fix notice edition (1.44 KB, patch)
2014-11-27 07:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13215: Fix GetLetterTemplates should return default templates if branchcode is not defined (1.35 KB, patch)
2014-11-27 07:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 13215: Fix GetLetterTemplates should return default templates if branchcode is not defined (1.71 KB, patch)
2014-11-27 14:08 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2014-11-06 15:40:43 UTC
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.
Comment 1 Jonathan Druart 2014-11-06 15:56:46 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-11-06 16:22:06 UTC
(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...
Comment 3 Mark Tompsett 2014-11-06 17:40:54 UTC
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.
Comment 4 Katrin Fischer 2014-11-06 17:58:08 UTC
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.
Comment 5 Mark Tompsett 2014-11-06 18:06:34 UTC
(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.
Comment 6 Tomás Cohen Arazi 2014-11-06 18:48:39 UTC
(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.
Comment 7 Mark Tompsett 2014-11-06 19:13:39 UTC
(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.
Comment 8 Jonathan Druart 2014-11-07 11:18:24 UTC
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 9 Mark Tompsett 2014-11-07 14:24:28 UTC
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?
Comment 10 Jonathan Druart 2014-11-07 15:03:07 UTC
(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.
Comment 11 Chris Cormack 2014-11-18 06:13:57 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2014-11-20 07:19:34 UTC
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.
Comment 13 Jonathan Druart 2014-11-20 08:31:47 UTC
(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.
Comment 14 Katrin Fischer 2014-11-21 07:30:11 UTC
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.
Comment 15 Jonathan Druart 2014-11-21 09:41:22 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2014-11-21 09:41:28 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2014-11-21 14:48:57 UTC
* 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?
Comment 18 Jonathan Druart 2014-11-21 15:14:18 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2014-11-21 15:18:13 UTC
(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.
Comment 20 Katrin Fischer 2014-11-21 15:31:42 UTC
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.
Comment 21 Marc Véron 2014-11-25 13:50:25 UTC
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
Comment 22 Jonathan Druart 2014-11-25 14:35:48 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2014-11-25 14:36:24 UTC
(In reply to Marc Véron from comment #21)
> I have the following issue:

Thanks for testing Marc.
That should be fixed now.
Comment 24 Chris Cormack 2014-11-26 02:20:43 UTC Comment hidden (obsolete)
Comment 25 Chris Cormack 2014-11-26 02:21:28 UTC Comment hidden (obsolete)
Comment 26 Chris Cormack 2014-11-26 02:21:37 UTC Comment hidden (obsolete)
Comment 27 Chris Cormack 2014-11-26 02:22:37 UTC Comment hidden (obsolete)
Comment 28 Chris Cormack 2014-11-26 02:23:44 UTC Comment hidden (obsolete)
Comment 29 Chris Cormack 2014-11-26 02:24:35 UTC Comment hidden (obsolete)
Comment 30 Chris Cormack 2014-11-26 02:24:43 UTC Comment hidden (obsolete)
Comment 31 Chris Cormack 2014-11-26 02:24:49 UTC Comment hidden (obsolete)
Comment 32 Chris Cormack 2014-11-26 02:24:59 UTC Comment hidden (obsolete)
Comment 33 Chris Cormack 2014-11-26 02:25:06 UTC Comment hidden (obsolete)
Comment 34 Katrin Fischer 2014-11-26 07:42:48 UTC
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.
Comment 35 Katrin Fischer 2014-11-27 07:14:50 UTC
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.
Comment 36 Katrin Fischer 2014-11-27 07:16:53 UTC
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>
Comment 37 Katrin Fischer 2014-11-27 07:16:57 UTC
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>
Comment 38 Katrin Fischer 2014-11-27 07:17:00 UTC
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>
Comment 39 Katrin Fischer 2014-11-27 07:17:04 UTC
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>
Comment 40 Katrin Fischer 2014-11-27 07:17:07 UTC Comment hidden (obsolete)
Comment 41 Katrin Fischer 2014-11-27 08:30:45 UTC
(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.
Comment 42 Jonathan Druart 2014-11-27 08:32:34 UTC
(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).
Comment 43 Jonathan Druart 2014-11-27 14:08:54 UTC
Created attachment 33995 [details] [review]
Bug 13215: Fix GetLetterTemplates should return default templates if branchcode is not defined
Comment 44 Jonathan Druart 2014-11-27 14:09:53 UTC
(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).
Comment 45 Tomás Cohen Arazi 2014-11-27 14:45:07 UTC
Patches pushed to master.

Thanks Jonathan!