Bug 14206

Summary: notices using non email templates can't be deleted from the staff client
Product: Koha Reporter: Indranil Das Gupta <indradg>
Component: NoticesAssignee: Indranil Das Gupta <indradg>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: chris, jonathan.druart, katrin.fischer, mtj, tomascohen, veron, wizzyrea
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 14233    
Attachments: 'print'-only notice deletion confirmation dialog
Bug 14206 - Fixes deletion issue for non-email notices
Bug 14206 - message_transport_type handling in C4::Letters::getletter
Bug 14206 Adds two tests to t/db_dependent/Letters.t
Bug 14206 Adds delete function for non email templates
[Signed-off] Bug 14206 Adds two tests to t/db_dependent/Letters.t
Bug 14206 Adds delete function for non email templates
Bug 14206 Adds two tests to t/db_dependent/Letters.t
Bug 14206 Adds delete function for non email templates
Bug 14206 Adds delete function for non email templates
[Signed-off] Bug 14206 Adds delete function for non email templates
Bug 14206 - Adds test for getletter() call from overdue_notices.pl
[Signed-off] Bug 14206 - Adds test for getletter() call from overdue_notices.pl
Bug 14206 Adds delete function for non email templates
Bug 14206 - Adds test for getletter() call from overdue_notices.pl

Description Indranil Das Gupta 2015-05-15 13:06:51 UTC
Use-case: Wanted a 'letter' that will only be printed and NOT emailed.

What I found:

1/ can define and save such a letter with only the print template filled
2/ letter shows up in the list or letters with 'edit' and 'delete'
links next to it
3/ edit works fine, but delete doesn't work. In fact, with delete, the
<td> values of library, module, code and name are not displayed.
4/ delete does not work for a letter using print only template

To delete: Edit the letter and add a 'print' template (additionally).
Now the letter can be deleted.

Question: is this the correct / expected behaviour?
Comment 1 Indranil Das Gupta 2015-05-15 14:29:50 UTC
I looked into C4/Letters.pm

sub getletter {
    my ( $module, $code, $branchcode, $message_transport_type ) = @_;
    $message_transport_type ||= 'email';

While tools/letters.pl is invoking getletter with only $module, $code,
$branchcode params
Comment 2 Indranil Das Gupta 2015-05-18 00:07:52 UTC
Created attachment 39270 [details]
'print'-only notice deletion confirmation dialog

Shows the <td>s not being populated. Clicking the 'Yes, delete' button will submit the page without any apparent error. But the record will not be deleted.
Comment 3 Indranil Das Gupta 2015-05-18 01:03:03 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2015-05-18 10:30:54 UTC
I disagree with this patch.
I understand the problem, but this patch does not fix it in the right way.
It does not make sense to add the mtt param to the "delete" link. Indeed, we will delete all mtt for a given letter.

A better fix would be to change the getletter behavior to default on "email", but return the first letter if the email template has not been created.
Comment 5 Indranil Das Gupta 2015-05-18 10:49:23 UTC
(In reply to Jonathan Druart from comment #4)
> I disagree with this patch.
> I understand the problem, but this patch does not fix it in the right way.
> It does not make sense to add the mtt param to the "delete" link. Indeed, we
> will delete all mtt for a given letter.
> 
> A better fix would be to change the getletter behavior to default on
> "email", but return the first letter if the email template has not been
> created.

I agree with your comment, personally I wasn't happy doing it this way, but wasn't sure if it would be OK to mess with an 'in-use' Letters.pm

So, you suggest that I should focus on:

sub getletter {
    my ( $module, $code, $branchcode, $message_transport_type ) = @_;
    $message_transport_type ||= 'email';
Comment 6 Indranil Das Gupta 2015-05-18 12:47:11 UTC Comment hidden (obsolete)
Comment 7 Indranil Das Gupta 2015-05-19 19:43:17 UTC Comment hidden (obsolete)
Comment 8 Indranil Das Gupta 2015-05-19 19:57:09 UTC Comment hidden (obsolete)
Comment 9 Marc Véron 2015-05-20 08:10:34 UTC Comment hidden (obsolete)
Comment 10 Marc Véron 2015-05-20 08:19:27 UTC
First patch (with tests) worked OK (signed-off), but I could not apply main patch:

Applying: Bug 14206 Adds delete function for non email templates
fatal: sha1 information is lacking or useless (C4/Letters.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 14206 Adds delete function for non email templates
Comment 11 Indranil Das Gupta 2015-05-20 10:52:03 UTC
(In reply to Marc Véron from comment #10)
> First patch (with tests) worked OK (signed-off), but I could not apply main
> patch:
> 
> Applying: Bug 14206 Adds delete function for non email templates
> fatal: sha1 information is lacking or useless (C4/Letters.pm).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 Bug 14206 Adds delete function for non email templates


I prolly need to rebase the patch. let me check.
Comment 12 Indranil Das Gupta 2015-05-20 11:24:24 UTC Comment hidden (obsolete)
Comment 13 Mark Tompsett 2015-05-20 20:42:52 UTC
Comment on attachment 39342 [details] [review]
Bug 14206 Adds delete function for non email templates

Review of attachment 39342 [details] [review]:
-----------------------------------------------------------------

::: C4/Letters.pm
@@ +227,4 @@
>          SELECT *
>          FROM letter 
>          WHERE module=? AND code=? AND (branchcode = ? OR branchcode = '')
> +        AND ( message_transport_type = ? OR message_transport_type IN (SELECT message_transport_type FROM message_transport_types))

"message_transport_type IN (SELECT message_transport_type FROM message_transport_types)" is always true, isn't it?! I don't think this is the patch you want.
Comment 14 Indranil Das Gupta 2015-05-21 01:13:34 UTC Comment hidden (obsolete)
Comment 15 Indranil Das Gupta 2015-05-21 01:17:08 UTC Comment hidden (obsolete)
Comment 16 Indranil Das Gupta 2015-05-21 01:21:46 UTC
modified the patch including the tests
Comment 17 Indranil Das Gupta 2015-05-21 22:25:49 UTC Comment hidden (obsolete)
Comment 18 Indranil Das Gupta 2015-05-21 22:28:29 UTC
Converted into a single commit. LIMIT 1 added back in. Test plan revised.
Comment 19 Marc Véron 2015-05-22 07:06:25 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2015-05-22 13:38:03 UTC
Please test and provide a test plan for overdue_notices.pl.
Comment 21 Indranil Das Gupta 2015-05-25 00:00:44 UTC Comment hidden (obsolete)
Comment 22 Marc Véron 2015-05-26 12:33:06 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2015-05-26 13:16:21 UTC
Created attachment 39522 [details] [review]
Bug 14206 Adds delete function for non email templates

C4::Letters::getletter() is called in tools/letter.pl by the function
delete_confirm() to display the selected notice for deletion. Due to
current implementation of getletter(), a notice that does not use
the 'email' template (but uses any/all of the other templates - sms,
print or phone) can't be deleted from the staff client.

This patch adds deletion capability for notices that do not use email
template, but uses any/all of the other templates i.e. sms, print or
phone. This also adds 2 tests to t/db_dependent/Letters.t for testing
both conditions - a) when message_transport_type is specified b) when
it is not.

Test plan
=========

1/ Go to Tools -> Notices & Slips. Add a new notice only for print,
   leave 'Library' and 'Koha module' options as default selections.
   Enter 'KOHA_14206' and 'Koha Test 14206' against Code and Name
   respectively, and 'Test' and 'Test Message' for subject and body.
   Leave the Email, Phone and SMS tabs blank. Save the notice.
2/ On the notices listing page the new notice will be listed. Try to
   delete it. It will load the 'Delete notice' dialog form, but the
   table will not show any data under <th>s - 'Library', 'Module',
   'Code' or 'Name'.
3/ Click the "Yes, delete" button. The page will be submitted and the
   Notices listing reloaded. The print-only KOHA_14206 notice should
   continue to exist. This is *wrong*.
4/ Apply this patch
5/ Reload the listings page and click on the 'Delete' link for Notice
   KOHA_14206. This time, it should show the data under 'Module',
   'Code' or 'Name' at least.
6/ Click on 'Yes, delete'. The page should submit and the listing page
   reload. This time KOHA_14206 will be gone.
7/ Run prove -v t/db_dependent/Letters.t
   All tests should PASS without any error.

Followed test plan. Works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>
Comment 24 Jonathan Druart 2015-05-26 13:16:28 UTC
Created attachment 39523 [details] [review]
Bug 14206 - Adds test for getletter() call from overdue_notices.pl

Adds missing test for getletter() when called from overdue_notices.pl

Test plan
=========

1/ apply this patch
2/ run prove -v t/db_dependent/Letters.t
   all tests should pass, especially test #40 which tests call from
   overdue_notices.pl

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>
Comment 25 Jonathan Druart 2015-05-26 13:17:50 UTC
This looks safe.

Note that this patch also fixes a bug in overdue_notices.pl:
If a letter does not have an email template, the notice was not sent.
With this patch, even if the letter only contains a SMS template, the notice will be generated.
Comment 26 Tomás Cohen Arazi 2015-06-11 13:19:55 UTC
Patches pushed to master.

Thanks Indranil!
Comment 27 Chris Cormack 2015-06-14 21:09:38 UTC
Pushed to 3.20.x will be in 3.20.1
Comment 28 Liz Rea 2015-06-22 23:29:22 UTC
Patch pushed to 3.18.x will be in 3.18.08
Comment 29 Mason James 2015-07-25 05:38:35 UTC
Skipping for 3.16.x series, due to patch conflicts