Script overdue_notices.pl creates a printed letter if borrower as no email. Actually, only 'email' field of borrower is tested. Email field should depend on AutoEmailPrimaryAddress syspref like in other letter creations.
Created attachment 13511 [details] [review] Proposed patch
This patch may impact Bug 6835.
tested on koha master (3.09.00.062). Everything seems ok - with patron's email address specified on "primary email" field AND syspref "AutoEmailPrimaryAddress" on "home" => notice sent to patron | OK - with patron's email address specified on "secondary email" field AND syspref "AutoEmailPrimaryAddress" on "work" => notice sent to patron | OK - with patron's email address specified on "alternate email" field AND syspref "AutoEmailPrimaryAddress" on "alternate" => notice sent to patron | OK - with patron's email address specified on "secondary email" OR "alternate email" field AND syspref "AutoEmailPrimaryAddress" on "home" => no notice sent to patron, overdue notice sent to koha admin | OK - with patron's email address specified on "primary email" OR "secondary email" field AND syspref "AutoEmailPrimaryAddress" on "alternate" => no notice sent to patron, overdue notice sent to koha admin | OK - with patron's email address specified on "primary email" OR "secondary email" OR "alternate email" field and syspref "AutoEmailPrimaryAddress" on "first valid" => notice sent to patron | OK Julien Sicot Systems Librarian Rennes 2 University
Hi Fridolyn, I like the idea to have a new sub that returns the correct email for the borrower according to the AutoEmailPrimaryAddress system preference. What bugs me a bit about it is the name of the new sub "GetPrimaryEmailAddress". I am not sure if you are aware of it, but some time ago the email and phone number fields in the patron record got renamed. The first email field in the English templates is now named 'Primary email:' - That makes it a bit confusing. Could you maybe choose a better name? Extra bonus points if you include some unit tests.
Created attachment 15931 [details] [review] Proposed patch (revised) Due to Bug 6835 integration in master, I've rewritten patch. New patch renames as asked method GetPrimaryEmailAddress into GetNoticeEmailAddress, meaning the email used to sent notices. Bug 6835 added a GetMember() call to get emails. I changed to get them in the existing query. A consequence is the use of fetchrow_hashref to have datas into a hash. This is more useful and generally used to get query results. Also, prepare_letter_for_printing was called twice where it can be once. See test plan on comment 3. Note that if --email option is used, it overrides notice email address. I could uses some help on a more complex one and for unit test.
Created attachment 16110 [details] [review] Bug 9103: overdue_notices.pl should use AutoEmailPrimaryAddress syspref Script overdue_notices.pl creates a printed letter if borrower as no email. Actually, unless --email option is used, first valid email of borrower is used. Email field should depend on AutoEmailPrimaryAddress syspref like in other letter creations. Signed-off-by: MJ Ray <mjr@phonecoop.coop>
Looks good to me. perl -c C4/Letters.pm produces lots of warnings, but they were there before.
Back to that now.
Created attachment 16248 [details] [review] [PASSED QA] Bug 9103: overdue_notices.pl should use AutoEmailPrimaryAddress syspref Script overdue_notices.pl creates a printed letter if borrower as no email. Actually, unless --email option is used, first valid email of borrower is used. Email field should depend on AutoEmailPrimaryAddress syspref like in other letter creations. Signed-off-by: MJ Ray <mjr@phonecoop.coop> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> All tests and QA script pass. Following test plan from Julien Sicot from Bugzilla: - with patron's email address specified on "primary email" field AND syspref "AutoEmailPrimaryAddress" on "home" => notice sent to patron | OK - with patron's email address specified on "secondary email" field AND syspref "AutoEmailPrimaryAddress" on "work" => notice sent to patron | OK - with patron's email address specified on "alternate email" field AND syspref "AutoEmailPrimaryAddress" on "alternate" => notice sent to patron | OK - with patron's email address specified on "secondary email" OR "alternate email" field AND syspref "AutoEmailPrimaryAddress" on "home" => no notice sent to patron, overdue notice sent to koha admin | OK - with patron's email address specified on "primary email" OR - with patron's email address specified on "primary email" field AND syspref "AutoEmailPrimaryAddress" on "home" => notice sent to patron | OK - with patron's email address specified on "secondary email" field AND syspref "AutoEmailPrimaryAddress" on "work" => notice sent to patron | OK - with patron's email address specified on "alternate email" field AND syspref "AutoEmailPrimaryAddress" on "alternate" => notice sent to patron | OK - with patron's email address specified on "secondary email" OR "alternate email" field AND syspref "AutoEmailPrimaryAddress" on "home" => no notice sent to patron, overdue notice sent to koha admin | OK - with patron's email address specified on "primary email" OR "secondary email" field AND syspref "AutoEmailPrimaryAddress" on "alternate" => no notice sent to patron, overdue notice sent to koha admin | OK - with patron's email address specified on "primary email" OR "secondary email" OR "alternate email" field and syspref "AutoEmailPrimaryAddress" on "first valid" => notice sent to patron | OK"secondary email" field AND syspref "AutoEmailPrimaryAddress" on "alternate" => no notice sent to patron, overdue notice sent to koha admin | OK - with patron's email address specified on "primary email" OR "secondary email" OR "alternate email" field and syspref "AutoEmailPrimaryAddress" on "first valid" => notice sent to patron | OK Note: Options for AutoEmailPrimaryAddress should be like the field names on the patron form (primary, secondary...), but this is outside the scope of this patch.
C4::Members::GetNoticeEmailAddress needs a unit test.
Created attachment 16452 [details] [review] Bug 9103 : Follow up adding unit test
Passing QA as the unit test follow-up meets my requirements.
This patch has been pushed to master.