Bug 29046

Summary: Allow libraries to specify email order for "AutoEmailPrimaryAddress"
Product: Koha Reporter: David Cook <dcook>
Component: PatronsAssignee: Martin Renvoize <martin.renvoize>
Status: RESOLVED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: enhancement    
Priority: P5 - low CC: caroline.cyr-la-rose, dcook, gmcharlt, katrin.fischer, kyle.m.hall, martin.renvoize, matt.blenkinsop
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18398
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33191
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Caroline Cyr La Rose Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/692
Text to go in the release notes:
**Sponsored by** *PTFS Europe* This enhancement adds a new system preference EmailFieldPrecedence, which allows libraries to set the order in which patron email addresses are used when selecting "first valid" in EmailFieldPrimary (formerly AutoEmailPrimaryAddress).
Version(s) released in:
23.05.00
Bug Depends on: 33192    
Bug Blocks: 33592, 34280    
Attachments: Bug 29046: Add 'EmailFieldPrecidence' preference
Bug 29046: Use EmailFieldPrecidence in first_valid_email_address
Bug 29046: Add 'EmailFieldPrecidence' preference
Bug 29046: Use EmailFieldPrecedence in first_valid_email_address
Bug 29046: Add 'EmailFieldPrecidence' preference
Bug 29046: Use EmailFieldPrecedence in first_valid_email_address
Bug 29046: Change link in EmailFieldPrecedence and add link in EmailFieldPrimary
Bug 29046: Change link in EmailFieldPrecedence and add link in EmailFieldPrimary
Bug 29046: Add 'EmailFieldPrecedence' preference
Bug 29046: Use EmailFieldPrecedence in first_valid_email_address
Bug 29046: Change link in EmailFieldPrecedence and add link in EmailFieldPrimary
Bug 29046: Add 'EmailFieldPrecedence' preference
Bug 29046: Use EmailFieldPrecedence in first_valid_email_address
Bug 29046: Change link in EmailFieldPrecedence and add link in EmailFieldPrimary
Bug 29046: Unit tests
Bug 29046: Reword system preference.
Bug 29046: Add 'EmailFieldPrecedence' preference
Bug 29046: Use EmailFieldPrecedence in first_valid_email_address
Bug 29046: Change link in EmailFieldPrecedence and add link in EmailFieldPrimary
Bug 29046: Unit tests
Bug 29046: Reword system preference.
Bug 29046: Fix and amend system preference description

Description David Cook 2021-09-17 03:03:06 UTC
At the moment, Koha::Patron->first_valid_email_address is hard-coded to go "email", "emailpro", and then "B_email". 

Libraries should be able to specify their own preference (at least since historically email was "Home" and emailpro was "Work" and many non-public library Koha databases would rather privilege emailpro over email).
Comment 1 Martin Renvoize 2023-03-10 10:48:58 UTC Comment hidden (obsolete)
Comment 2 Martin Renvoize 2023-03-10 10:49:01 UTC Comment hidden (obsolete)
Comment 3 Martin Renvoize 2023-03-10 10:53:17 UTC
Created attachment 148056 [details] [review]
Bug 29046: Add 'EmailFieldPrecidence' preference

This patch adds a new EmailFieldPrecidence systempreference to allow
users to set the precicence in which patron email fields should be used
for communications.
Comment 4 Martin Renvoize 2023-03-10 10:53:19 UTC
Created attachment 148057 [details] [review]
Bug 29046: Use EmailFieldPrecedence in first_valid_email_address

This patch update the Koha::Patron->first_valid_email_address method to
use the newly defined EmailFieldPrecedence preference.
Comment 5 Caroline Cyr La Rose 2023-03-13 20:07:25 UTC
Created attachment 148145 [details] [review]
Bug 29046: Add 'EmailFieldPrecidence' preference

This patch adds a new EmailFieldPrecidence systempreference to allow
users to set the precicence in which patron email fields should be used
for communications.

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Comment 6 Caroline Cyr La Rose 2023-03-13 20:07:27 UTC
Created attachment 148146 [details] [review]
Bug 29046: Use EmailFieldPrecedence in first_valid_email_address

This patch update the Koha::Patron->first_valid_email_address method to
use the newly defined EmailFieldPrecedence preference.

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Comment 7 Caroline Cyr La Rose 2023-03-13 20:07:29 UTC
Created attachment 148147 [details] [review]
Bug 29046: Change link in EmailFieldPrecedence and add link in EmailFieldPrimary

This patch modifies the link in EmailFieldPrecedence to reflect the new
name of EmailFieldPrimary. It also adds a note in the latter to set the
'first valid' order in the former.

To test:
1. Go to Administration > Global system preferences
2. Search for EmailFieldPrecedence
3. Click on the link in the description
   --> It should lead to EmailFieldPrimary
4. Read the note in EmailFieldPrimary, make sure the grammar and
   spelling are correct.
5. Click the link to EmailFieldPrecedence
   --> It should lead to EmailFieldPrecedence
Comment 8 Caroline Cyr La Rose 2023-03-13 20:09:10 UTC
I tested the new syspref and it works.

1. Add primary, secondary and alternate email addresses in a patron's account
   - Primary email = primary@...
   - Secondary email = secondary@...
   - Email, under Alternate address = alternate@...

2. Set EmailFieldPrimary to first valid

3. In the patron's account click More > Send welcome email

4. Check the message_queue, the to_address should be primary@...

select to_address, message_transport_type, letter_code, time_queued from message_queue where date(time_queued) = curdate();

5. Set EmailFieldPrecedence to emailpro|B_email|email

6. Repeat steps 3 and 4, the to_address should be secondary@...

7. Set EmailFieldPrecedence to B_email|email|emailpro

8. Repeat steps 3 and 4, the to_address should be alternate@...

9. Set EmailFieldPrecedence to email|emailpro|B_email

10. Remove the primary email from the patron's account

14. Repeat steps 3 and 4, the to_address should be secondary@...

15. Remove the secondary email from the patron's account

16. Repeat steps 3 and 4, the to_address should be alternate@...

Try other orders in EmailFieldPrecedence and removing the emails in a different order.

I added a link from EmailFieldPrimary to the new syspref and changed the link to reflect the changes in the dependant bug 33192...
Comment 9 Martin Renvoize 2023-03-14 11:02:49 UTC
Created attachment 148155 [details] [review]
Bug 29046: Change link in EmailFieldPrecedence and add link in EmailFieldPrimary

This patch modifies the link in EmailFieldPrecedence to reflect the new
name of EmailFieldPrimary. It also adds a note in the latter to set the
'first valid' order in the former.

To test:
1. Go to Administration > Global system preferences
2. Search for EmailFieldPrecedence
3. Click on the link in the description
   --> It should lead to EmailFieldPrimary
4. Read the note in EmailFieldPrimary, make sure the grammar and
   spelling are correct.
5. Click the link to EmailFieldPrecedence
   --> It should lead to EmailFieldPrecedence

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2023-03-14 11:03:28 UTC
Thanks for the follow-up Caroline, looks great.

Signing off on that bit and updating the bug status :)
Comment 11 Katrin Fischer 2023-04-10 15:01:25 UTC
Overall this looks good, but we got one blocker:

1) Unit tests are missing.

2) I wonder if we could simplify the system preference description a little:

Use the following patron email fields in order of highest precedence first [email|emailpro|B_email] when selecting which email address to use to send patron notices. 

2 things:
* I think we should list the valid options in the system preference so that when one was deleted, it can be restored more easily.

* I have no idea how to translate highest precedence... but what it means is that the first field with an email address will be used, right? Maybe we could phrase it along the lines of:

When EmailFieldPrimary is set to 'first valid', check the patron email address fields in the following order and use the first valid email address found:

* Not blocker: For languages we can sort the entries with drag&drop. Could this be a possible option here? Or maybe I could file as a separate bug?
Comment 12 Katrin Fischer 2023-04-10 15:02:39 UTC
Created attachment 149385 [details] [review]
Bug 29046: Add 'EmailFieldPrecedence' preference

This patch adds a new EmailFieldPrecidence systempreference to allow
users to set the precicence in which patron email fields should be used
for communications.

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Comment 13 Katrin Fischer 2023-04-10 15:02:42 UTC
Created attachment 149386 [details] [review]
Bug 29046: Use EmailFieldPrecedence in first_valid_email_address

This patch update the Koha::Patron->first_valid_email_address method to
use the newly defined EmailFieldPrecedence preference.

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Comment 14 Katrin Fischer 2023-04-10 15:02:45 UTC
Created attachment 149387 [details] [review]
Bug 29046: Change link in EmailFieldPrecedence and add link in EmailFieldPrimary

This patch modifies the link in EmailFieldPrecedence to reflect the new
name of EmailFieldPrimary. It also adds a note in the latter to set the
'first valid' order in the former.

To test:
1. Go to Administration > Global system preferences
2. Search for EmailFieldPrecedence
3. Click on the link in the description
   --> It should lead to EmailFieldPrimary
4. Read the note in EmailFieldPrimary, make sure the grammar and
   spelling are correct.
5. Click the link to EmailFieldPrecedence
   --> It should lead to EmailFieldPrecedence

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2023-04-20 13:24:18 UTC
Created attachment 149959 [details] [review]
Bug 29046: Add 'EmailFieldPrecedence' preference

This patch adds a new EmailFieldPrecidence systempreference to allow
users to set the precicence in which patron email fields should be used
for communications.

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Comment 16 Martin Renvoize 2023-04-20 13:24:21 UTC
Created attachment 149960 [details] [review]
Bug 29046: Use EmailFieldPrecedence in first_valid_email_address

This patch update the Koha::Patron->first_valid_email_address method to
use the newly defined EmailFieldPrecedence preference.

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Comment 17 Martin Renvoize 2023-04-20 13:24:24 UTC
Created attachment 149961 [details] [review]
Bug 29046: Change link in EmailFieldPrecedence and add link in EmailFieldPrimary

This patch modifies the link in EmailFieldPrecedence to reflect the new
name of EmailFieldPrimary. It also adds a note in the latter to set the
'first valid' order in the former.

To test:
1. Go to Administration > Global system preferences
2. Search for EmailFieldPrecedence
3. Click on the link in the description
   --> It should lead to EmailFieldPrimary
4. Read the note in EmailFieldPrimary, make sure the grammar and
   spelling are correct.
5. Click the link to EmailFieldPrecedence
   --> It should lead to EmailFieldPrecedence

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2023-04-20 13:24:26 UTC
Created attachment 149962 [details] [review]
Bug 29046: Unit tests
Comment 19 Martin Renvoize 2023-04-20 13:24:29 UTC
Created attachment 149963 [details] [review]
Bug 29046: Reword system preference.
Comment 20 Martin Renvoize 2023-04-20 13:26:11 UTC
(In reply to Katrin Fischer from comment #11)
> Overall this looks good, but we got one blocker:
> 
> 1) Unit tests are missing.

Done

> 2) I wonder if we could simplify the system preference description a little:
> 
> Use the following patron email fields in order of highest precedence first
> [email|emailpro|B_email] when selecting which email address to use to send
> patron notices. 
> 
> 2 things:
> * I think we should list the valid options in the system preference so that
> when one was deleted, it can be restored more easily.
> 
> * I have no idea how to translate highest precedence... but what it means is
> that the first field with an email address will be used, right? Maybe we
> could phrase it along the lines of:
> 
> When EmailFieldPrimary is set to 'first valid', check the patron email
> address fields in the following order and use the first valid email address
> found:

Done
 
> * Not blocker: For languages we can sort the entries with drag&drop. Could
> this be a possible option here? Or maybe I could file as a separate bug?

Please do this on another bug... this was meant as a starting point.. I don't have time to polish it any further I'm afraid.
Comment 21 Katrin Fischer 2023-04-23 11:22:57 UTC
> > * Not blocker: For languages we can sort the entries with drag&drop. Could
> > this be a possible option here? Or maybe I could file as a separate bug?
> 
> Please do this on another bug... this was meant as a starting point.. I
> don't have time to polish it any further I'm afraid.

Specifically marked it as 'not blocker' just wanted to get your opinion. I am still not sure if you like the idea, but filed separately now: bug 33592

I have tests failing with this patch set applied that pass without it :(

prove t/db_dependent/Koha/Patron.t t/db_dependent/Koha/Patrons.t
t/db_dependent/Koha/Patron.t ... 10/23 Use of uninitialized value in split at /kohadevbox/koha/Koha/Patron.pm line 1440.
Use of uninitialized value in split at /kohadevbox/koha/Koha/Patron.pm line 1440.
t/db_dependent/Koha/Patron.t ... 13/23 WARNING: The key derivation method "opensslv1" is deprecated. Using -pbkdf=>'pbkdf2' would be better.
Pass -nodeprecate=>1 to inhibit this message.
 at /kohadevbox/koha/Koha/Patron.pm line 2423.
t/db_dependent/Koha/Patron.t ... ok     
t/db_dependent/Koha/Patrons.t .. 31/43 Use of uninitialized value in split at /kohadevbox/koha/Koha/Patron.pm line 1440.
t/db_dependent/Koha/Patrons.t .. 32/43 Use of uninitialized value in split at /kohadevbox/koha/Koha/Patron.pm line 1440.

    #   Failed test 'One notice queued when NotifyPasswordChange enabled'
    #   at t/db_dependent/Koha/Patrons.t line 1972.
    #          got: '0'
    #     expected: '1'
    # Looks like you planned 16 tests but ran 15.
    # Looks like you failed 1 test of 15 run.

#   Failed test '->set_password'
#   at t/db_dependent/Koha/Patrons.t line 1976.
Can't call method "status" on an undefined value at t/db_dependent/Koha/Patrons.t line 1974.
# Looks like your test exited with 11 just after 33.
t/db_dependent/Koha/Patrons.t .. Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 11/43 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/Patrons.t (Wstat: 2816 Tests: 33 Failed: 1)
  Failed test:  33
  Non-zero exit status: 11
  Parse errors: Bad plan.  You planned 43 tests but ran 33.
Comment 22 Katrin Fischer 2023-04-23 11:24:23 UTC
My fault - it works now after running the database update :)
Comment 23 Katrin Fischer 2023-04-23 11:41:53 UTC
Created attachment 150124 [details] [review]
Bug 29046: Add 'EmailFieldPrecedence' preference

This patch adds a new EmailFieldPrecidence systempreference to allow
users to set the precicence in which patron email fields should be used
for communications.

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Katrin Fischer 2023-04-23 11:41:56 UTC
Created attachment 150125 [details] [review]
Bug 29046: Use EmailFieldPrecedence in first_valid_email_address

This patch update the Koha::Patron->first_valid_email_address method to
use the newly defined EmailFieldPrecedence preference.

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Katrin Fischer 2023-04-23 11:42:00 UTC
Created attachment 150126 [details] [review]
Bug 29046: Change link in EmailFieldPrecedence and add link in EmailFieldPrimary

This patch modifies the link in EmailFieldPrecedence to reflect the new
name of EmailFieldPrimary. It also adds a note in the latter to set the
'first valid' order in the former.

To test:
1. Go to Administration > Global system preferences
2. Search for EmailFieldPrecedence
3. Click on the link in the description
   --> It should lead to EmailFieldPrimary
4. Read the note in EmailFieldPrimary, make sure the grammar and
   spelling are correct.
5. Click the link to EmailFieldPrecedence
   --> It should lead to EmailFieldPrecedence

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Katrin Fischer 2023-04-23 11:42:03 UTC
Created attachment 150127 [details] [review]
Bug 29046: Unit tests

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Katrin Fischer 2023-04-23 11:42:07 UTC
Created attachment 150128 [details] [review]
Bug 29046: Reword system preference.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 28 Katrin Fischer 2023-04-23 11:42:10 UTC
Created attachment 150129 [details] [review]
Bug 29046: Fix and amend system preference description

The first line would not show in the system preference editor
due to missing quotes.

Also added information on the option names, link to the schema
and an example.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 29 David Cook 2023-05-03 04:02:21 UTC
I am so excited for this one to get in.
Comment 30 David Cook 2023-05-03 04:57:43 UTC
Comment on attachment 150125 [details] [review]
Bug 29046: Use EmailFieldPrecedence in first_valid_email_address

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

::: Koha/Patron.pm
@@ +1439,5 @@
> +
> +    my @fields = split /\s*\|\s*/,
> +      C4::Context->preference('EmailFieldPrecedence');
> +    for my $field (@fields) {
> +        $email = $self->$field;

You could crash Koha by putting junk into EmailFieldPrecedence.

I think either @fields should be validated or  $self->$field should be validated.
Comment 31 David Cook 2023-05-03 04:58:18 UTC
Despite my above comment, I think this should go ahead. It's a valuable contribution.
Comment 32 Tomás Cohen Arazi 2023-05-18 15:15:16 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 33 Matt Blenkinsop 2023-06-02 16:21:11 UTC
Enhancement - not backporting to 22.11.x
Comment 34 Katrin Fischer 2023-06-17 21:45:24 UTC
*** Bug 30665 has been marked as a duplicate of this bug. ***