Bug 9103 - overdue_notices.pl should use AutoEmailPrimaryAddress syspref
Summary: overdue_notices.pl should use AutoEmailPrimaryAddress syspref
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Fridolin Somers
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-19 11:18 UTC by Fridolin Somers
Modified: 2014-05-26 21:05 UTC (History)
4 users (show)

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


Attachments
Proposed patch (5.23 KB, patch)
2012-11-19 13:02 UTC, Fridolin Somers
Details | Diff | Splinter Review
Proposed patch (revised) (11.19 KB, patch)
2013-03-07 14:07 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 9103: overdue_notices.pl should use AutoEmailPrimaryAddress syspref (11.23 KB, patch)
2013-03-14 09:29 UTC, MJ Ray (software.coop)
Details | Diff | Splinter Review
[PASSED QA] Bug 9103: overdue_notices.pl should use AutoEmailPrimaryAddress syspref (13.63 KB, patch)
2013-03-18 14:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 9103 : Follow up adding unit test (2.37 KB, patch)
2013-03-20 02:38 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2012-11-19 11:18:48 UTC
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.
Comment 1 Fridolin Somers 2012-11-19 13:02:51 UTC Comment hidden (obsolete)
Comment 2 Fridolin Somers 2012-11-19 13:04:35 UTC
This patch may impact Bug 6835.
Comment 3 Julien Sicot 2012-11-20 13:31:08 UTC
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
Comment 4 Katrin Fischer 2012-11-25 17:56:59 UTC
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.
Comment 5 Fridolin Somers 2013-03-07 14:07:10 UTC Comment hidden (obsolete)
Comment 6 MJ Ray (software.coop) 2013-03-14 09:29:56 UTC Comment hidden (obsolete)
Comment 7 MJ Ray (software.coop) 2013-03-14 09:30:35 UTC
Looks good to me.

perl -c C4/Letters.pm produces lots of warnings, but they were there before.
Comment 8 Katrin Fischer 2013-03-18 13:50:10 UTC
Back to that now.
Comment 9 Katrin Fischer 2013-03-18 14:30:02 UTC
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.
Comment 10 Jared Camins-Esakov 2013-03-20 02:14:40 UTC
C4::Members::GetNoticeEmailAddress needs a unit test.
Comment 11 Chris Cormack 2013-03-20 02:38:35 UTC
Created attachment 16452 [details] [review]
Bug 9103 : Follow up adding unit test
Comment 12 Jared Camins-Esakov 2013-03-20 02:50:02 UTC
Passing QA as the unit test follow-up meets my requirements.
Comment 13 Jared Camins-Esakov 2013-03-20 02:51:18 UTC
This patch has been pushed to master.