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?
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
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.
Created attachment 39272 [details] [review] Bug 14206 - Fixes deletion issue for non-email notices message_transport_type was not being passed to routine in letter.pl routines, so notices that did not include an email template couldn't be deleted from Tools -> Notices & Slips page. This patch fixes that issue. 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 the 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 sql statement SELECT * FROM `letter` WHERE `code` = 'KOHA_14206' to check actual deletion from the database. No record should be returned. 8/ Re-add notice KOHA_14206, but this time also add fill in print, email, phone and sms delivery options. 9/ Delete the newly re-added notice KOHA_14206. It should no longer be shown in the listing. Confirm deletion by running step #7. No record should be returned. 10/ run koha qa test tools
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.
(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';
Created attachment 39283 [details] [review] Bug 14206 - message_transport_type handling in C4::Letters::getletter Revised patch based on Jonathan Druart's comment. Addresses the default message_transport_type value in C4::Letters::getletter() for notices that do not use an email template (i.e. uses phone / print / sms). Fixes the issue of not being able to delete notices with non-email templates from Tools -> Notices & Slips 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 the 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 sql statement SELECT * FROM `letter` WHERE `code` = 'KOHA_14206' to check actual deletion from the database. No record should be returned. 8/ Re-add notice KOHA_14206, but this time also add fill in print, email, phone and sms delivery options. 9/ Delete the newly re-added notice KOHA_14206. It should no longer be shown in the listing. Confirm deletion by running step #7. No record should be returned.
Created attachment 39315 [details] [review] Bug 14206 Adds two tests to t/db_dependent/Letters.t 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 2 tests to t/db_dependent/Letters.t to test the bug. Test plan ========= 1/ Apply the patch and run prove -v t/db_dependent/Letters.t 2/ Test 38 should pass demonstrating unpatched getletter() behaviour Test 39 should FAIL, highlighting the problem # Failed test 'Bug 14206 - returns actual message_transport_type' # at t/db_dependent/Letters.t line 181. # got: undef # expected: 'print' (Test plan continues in the second commit)
Created attachment 39316 [details] [review] Bug 14206 Adds delete function for non email templates 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. Test plan (Continued) ===================== 3/ 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. 4/ 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'. 5/ 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*. 6/ Apply this patch 7/ 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. 8/ Click on 'Yes, delete'. The page should submit and the listing page reload. This time KOHA_14206 will be gone. 9/ Re-run prove -v t/db_dependent/Letters.t This time all tests should PASS without any error.
Created attachment 39321 [details] [review] [Signed-off] Bug 14206 Adds two tests to t/db_dependent/Letters.t 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 2 tests to t/db_dependent/Letters.t to test the bug. Test plan ========= 1/ Apply the patch and run prove -v t/db_dependent/Letters.t 2/ Test 38 should pass demonstrating unpatched getletter() behaviour Test 39 should FAIL, highlighting the problem # Failed test 'Bug 14206 - returns actual message_transport_type' # at t/db_dependent/Letters.t line 181. # got: undef # expected: 'print' (Test plan continues in the second commit) Followed test plan, works as expected Signed-off-by: Marc Véron <veron@veron.ch>
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
(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.
Created attachment 39342 [details] [review] Bug 14206 Adds delete function for non email templates 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. Test plan (Continued) ===================== 3/ 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. 4/ 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'. 5/ 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*. 6/ Apply this patch 7/ 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. 8/ Click on 'Yes, delete'. The page should submit and the listing page reload. This time KOHA_14206 will be gone. 9/ Re-run prove -v t/db_dependent/Letters.t This time all tests should PASS without any error.
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.
Created attachment 39353 [details] [review] Bug 14206 Adds two tests to t/db_dependent/Letters.t 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 2 tests to t/db_dependent/Letters.t to test the bug. Test plan ========= 1/ Apply the patch and run prove -v t/db_dependent/Letters.t 2/ Test 38 should FAIL - getletter() fails when message_transport_type is not passed by caller function i.e. delete_confirm() in letter.pl # Failed test 'Bug 14206 - message_transport_type not passed, correct mtt detected' # at t/db_dependent/Letters.t line 179. # got: undef # expected: 'print' Test 39 should pass - getletter() functions correctly when passed message_transport_type param by caller function i.e. add_validate() (Test plan continues in the second commit)
Created attachment 39354 [details] [review] Bug 14206 Adds delete function for non email templates 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. Test plan (Continued) ===================== 3/ 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. 4/ 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'. 5/ 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*. 6/ Apply this patch 7/ 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. 8/ Click on 'Yes, delete'. The page should submit and the listing page reload. This time KOHA_14206 will be gone. 9/ Re-run prove -v t/db_dependent/Letters.t This time all tests should PASS without any error.
modified the patch including the tests
Created attachment 39376 [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.
Converted into a single commit. LIMIT 1 added back in. Test plan revised.
Created attachment 39380 [details] [review] [Signed-off] 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>
Please test and provide a test plan for overdue_notices.pl.
Created attachment 39467 [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
Created attachment 39512 [details] [review] [Signed-off] 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>
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>
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>
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.
Patches pushed to master. Thanks Indranil!
Pushed to 3.20.x will be in 3.20.1
Patch pushed to 3.18.x will be in 3.18.08
Skipping for 3.16.x series, due to patch conflicts