Bug 32455

Summary: Don't send hold notices from the library's inbound email address
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: Hold requestsAssignee: Nick Clemens (kidclamp) <nick>
Status: CLOSED FIXED QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: gmcharlt, jonathan.druart, lucas, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.03
Circulation function:
Bug Depends on: 14364    
Bug Blocks:    
Attachments: Bug 32455: Use from_email_address for 'from' field for hold notices
Bug 32455: Unit tests
Bug 32455: Cleanup and expand ExpireReservesAutoFill.t
Bug 32455: Use from_email_address for 'from' field for hold notices
Bug 32455: Unit tests
Bug 32455: Cleanup and expand ExpireReservesAutoFill.t
Bug 32455: Use from_email_address for 'from' field for hold notices
Bug 32455: Unit tests
Bug 32455: Cleanup and expand ExpireReservesAutoFill.t

Description Nick Clemens (kidclamp) 2022-12-13 11:41:59 UTC
Bug 14364 made a change that seems advised against by the code. koha_notify_reserve uses the inbound_email_address for the 'from' field, it should use 'from_email_address'
Comment 1 Katrin Fischer 2022-12-13 12:04:18 UTC
That is correct, the From should be branchemail/KohaAdminEmailAddress.
Comment 2 Nick Clemens (kidclamp) 2022-12-13 12:19:01 UTC
Created attachment 144552 [details] [review]
Bug 32455: Use from_email_address for 'from' field for hold notices

This patch updates two occurrences where the inbound library email is
used as the from address

To test:
 1 - Set a unique 'Email' and 'Reply to' address for a library
 2 - Find a patron of that library, ensure they have an email
 3 - Ensure their messaging preference for holds is 'email'
 4 - Set system preference ReservesMaxPickupDelay to -1
 5 - Set system preference  ExpireReservesMaxPickUpDelay to Allow
 6 - Set system preference ExpireReservesAutoFill to Do
 7 - Place and fill a hold for that patron at that library
 8 - Check the patron's notification tab, confirm the from address is the 'Reply to'
 9 - Place a hold for another patron on the same item
10 - Run the expired holds cronjob:
     perl misc/cronjobs/holds/cancel_expired_holds.pl --reason=whatever
11 - Check the message_queue - notice the from address is the 'reply to'
12 - Apply patch
13 - Repeat 1-11, confirm the from addresses are correct now
Comment 3 Jonathan Druart 2022-12-14 09:49:10 UTC
Please provide tests.
Comment 4 Nick Clemens (kidclamp) 2022-12-14 14:22:43 UTC
Created attachment 144579 [details] [review]
Bug 32455: Unit tests
Comment 5 Nick Clemens (kidclamp) 2022-12-14 14:22:47 UTC
Created attachment 144580 [details] [review]
Bug 32455: Cleanup and expand ExpireReservesAutoFill.t

The tests here reused data, and deleted the tables before starting,
with a little shifting we can use specific data and avoid mass deletion

To test:
prove -v t/db_dependent/Holds/ExpireReservesAutoFill.t
Comment 6 David Nind 2023-01-28 18:29:08 UTC
Created attachment 145783 [details] [review]
Bug 32455: Use from_email_address for 'from' field for hold notices

This patch updates two occurrences where the inbound library email is
used as the from address

To test:
 1 - Set a unique 'Email' and 'Reply to' address for a library
 2 - Find a patron of that library, ensure they have an email
 3 - Ensure their messaging preference for holds is 'email'
 4 - Set system preference ReservesMaxPickupDelay to -1
 5 - Set system preference  ExpireReservesMaxPickUpDelay to Allow
 6 - Set system preference ExpireReservesAutoFill to Do
 7 - Place and fill a hold for that patron at that library
 8 - Check the patron's notification tab, confirm the from address is the 'Reply to'
 9 - Place a hold for another patron on the same item
10 - Run the expired holds cronjob:
     perl misc/cronjobs/holds/cancel_expired_holds.pl --reason=whatever
11 - Check the message_queue - notice the from address is the 'reply to'
12 - Apply patch
13 - Repeat 1-11, confirm the from addresses are correct now

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2023-01-28 18:29:14 UTC
Created attachment 145784 [details] [review]
Bug 32455: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2023-01-28 18:29:20 UTC
Created attachment 145785 [details] [review]
Bug 32455: Cleanup and expand ExpireReservesAutoFill.t

The tests here reused data, and deleted the tables before starting,
with a little shifting we can use specific data and avoid mass deletion

To test:
prove -v t/db_dependent/Holds/ExpireReservesAutoFill.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 Martin Renvoize (ashimema) 2023-02-01 13:36:59 UTC
Created attachment 145940 [details] [review]
Bug 32455: Use from_email_address for 'from' field for hold notices

This patch updates two occurrences where the inbound library email is
used as the from address

To test:
 1 - Set a unique 'Email' and 'Reply to' address for a library
 2 - Find a patron of that library, ensure they have an email
 3 - Ensure their messaging preference for holds is 'email'
 4 - Set system preference ReservesMaxPickupDelay to -1
 5 - Set system preference  ExpireReservesMaxPickUpDelay to Allow
 6 - Set system preference ExpireReservesAutoFill to Do
 7 - Place and fill a hold for that patron at that library
 8 - Check the patron's notification tab, confirm the from address is the 'Reply to'
 9 - Place a hold for another patron on the same item
10 - Run the expired holds cronjob:
     perl misc/cronjobs/holds/cancel_expired_holds.pl --reason=whatever
11 - Check the message_queue - notice the from address is the 'reply to'
12 - Apply patch
13 - Repeat 1-11, confirm the from addresses are correct now

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize (ashimema) 2023-02-01 13:37:04 UTC
Created attachment 145941 [details] [review]
Bug 32455: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize (ashimema) 2023-02-01 13:37:09 UTC
Created attachment 145942 [details] [review]
Bug 32455: Cleanup and expand ExpireReservesAutoFill.t

The tests here reused data, and deleted the tables before starting,
with a little shifting we can use specific data and avoid mass deletion

To test:
prove -v t/db_dependent/Holds/ExpireReservesAutoFill.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize (ashimema) 2023-02-01 13:37:43 UTC
Trivial fix, great unit test improvements, no regressions and QA scripts are happy.

Passing QA
Comment 13 Tomás Cohen Arazi (tcohen) 2023-02-07 13:51:50 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 14 Jacob O'Mara 2023-02-08 16:38:01 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 15 Lucas Gass (lukeg) 2023-02-14 22:41:26 UTC
Missing dependencies for 22.05.x, no backport