Description
Maxime Pelletier
2013-04-18 18:52:53 UTC
Created attachment 17853 [details] [review] Patch to add the syspref SendAlerts() is not used only from serials/claims.pl, but from members/memberentry.pl as well, which would cause unwanted Bcc. I suggest type is examined for Bcc as well Created attachment 30632 [details] [review] Bug 10076 - Fixed merge conflicts. Patch now applies properly modified: C4/Letters.pm modified: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/acquisitions.pref Type is checked before sending Bcc. I've applied the patch against master 3.17.00.016 I set the pref ClaimsBccCopy to "Send". Everything is OK as required. So I pass the patch to "Signed Off" status. Created attachment 31262 [details] [review] Bug 10076 - Add Bcc syspref for claimacquisition and claimissues This patch does not add the new system preference to updatedatabase.pl or sysprefs.sql. Created attachment 31397 [details] [review] syspref.sql and updatedatabase.pl Added simith any plan to rebase this patch on master? Hi maxime.beaulieu@inlibro.com, Need this patch one of our client. Can i take on this bug? Created attachment 40843 [details] [review] Bug 10076 - Add Bcc syspref for claimacquisition and claimissues Patch now applies properly modified: C4/Letters.pm modified: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/acquisitions.pref Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Created attachment 40844 [details] [review] syspref.sql and updatedatabase.pl Added Created attachment 40845 [details] [review] Bug 10076 - Atomic Update new file: installer/data/mysql/atomicupdate/Bug10076_ClaimsBccCopy.sql Patch has been rebased to current master. Since it's a small patch, it would be better to package it in a single patch. And could you use the capitalized version of the new syspref in SQL? ClaimsBccCopy rather than claimsbcccopy. Here: VALUES ('claimsbcccopy','0','','bcc the claimacquisition and clamissues alerts','YesNo'); ('claimsbcccopy','0','','bcc the claimacquisition and clamissues alerts','YesNo'), Please make the small fixes suggested by Frederic! :) Created attachment 45181 [details] [review] Bug 10076 - Add Bcc syspref for ClaimAcquisition and ClaimIssues This is a squash of the previous three patches, with capitalization of the syspref 'ClaimsBccCopy' Created attachment 45183 [details] [review] Bug 10076 - Add Bcc syspref for ClaimAcquisition and ClaimIssues There is already a syspref called "OverdueNoticeBcc" for sending Bcc copies of mails sent for overdues and other notices. This patch add a new syspref ClaimsBccCopy to bcc the claimacquisition and clamissues alerts. Signed-off-by: Frédéric Demians <f.demians@tamil.fr> Works as described. Just a request for a small follow-up here: The system preference is not nicely formatted and I think the wording could be a bit more clear. Currently it reads: Send a copy in bcc of the ClaimAcquisition and ClaimIssues letters to the active user [Send|Don't send] My suggestion after discussing with a native speaker (thx Liz :) ) would be: [Send|Don't send] blind copy (BCC) to logged in user when sending serial or acquisitions claims notices. Created attachment 45264 [details] [review] Bug 10076 - Add Bcc syspref for ClaimAcquisition and ClaimIssues There is already a syspref called "OverdueNoticeBcc" for sending Bcc copies of mails sent for overdues and other notices. This patch add a new syspref ClaimsBccCopy to bcc the claimacquisition and clamissues alerts. Changed the wording of the system preference to: [Send|Don't send] blind copy (BCC) to logged in user when sending serial or acquisitions claims notices. Hi Charles, for a small change request like this doing a follow-up patch and keep the 'Signed-off' would be ok next time! I just noticed comment#2 from Srdjan - can you comment on that? (In reply to Katrin Fischer from comment #20) > Hi Charles, > for a small change request like this doing a follow-up patch and keep the > 'Signed-off' would be ok next time! > I just noticed comment#2 from Srdjan - can you comment on that? Hi Katrin, Maxime already took care of it. This problem is avoided in the patch by first checking if the letter type is 'claimacquisition' or 'claimissues'. Never should a BCC be sent otherwise. I unfortunately cannot unit test this part though, because the email is never queued anywhere. But from the code, it looks like no unwanted BCC can be sent. Thx, I can see it now! So all we need is another sign-off now - or the old signed patch + a new follow-up dealing with the string changes. Created attachment 45350 [details] [review] Bug 10076 - Add Bcc syspref for ClaimAcquisition and ClaimIssues There is already a syspref called "OverdueNoticeBcc" for sending Bcc copies of mails sent for overdues and other notices. This patch add a new syspref ClaimsBccCopy to bcc the claimacquisition and clamissues alerts. Changed the wording of the system preference to: [Send|Don't send] blind copy (BCC) to logged in user when sending serial or acquisitions claims notices. Signed-off-by: Frédéric Demians <f.demians@tamil.fr> Created attachment 45361 [details] [review] [PASSED QA] Bug 10076 - Add Bcc syspref for ClaimAcquisition and ClaimIssues There is already a syspref called "OverdueNoticeBcc" for sending Bcc copies of mails sent for overdues and other notices. This patch add a new syspref ClaimsBccCopy to bcc the claimacquisition and clamissues alerts. Changed the wording of the system preference to: [Send|Don't send] blind copy (BCC) to logged in user when sending serial or acquisitions claims notices. Signed-off-by: Frédéric Demians <f.demians@tamil.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Pushed to master - Thanks |