Bug 32455 - Don't send hold notices from the library's inbound email address
Summary: Don't send hold notices from the library's inbound email address
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 14364
Blocks:
  Show dependency treegraph
 
Reported: 2022-12-13 11:41 UTC by Nick Clemens
Modified: 2023-12-28 20:42 UTC (History)
4 users (show)

See Also:
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


Attachments
Bug 32455: Use from_email_address for 'from' field for hold notices (2.36 KB, patch)
2022-12-13 12:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32455: Unit tests (4.13 KB, patch)
2022-12-14 14:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32455: Cleanup and expand ExpireReservesAutoFill.t (8.20 KB, patch)
2022-12-14 14:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32455: Use from_email_address for 'from' field for hold notices (2.41 KB, patch)
2023-01-28 18:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 32455: Unit tests (4.17 KB, patch)
2023-01-28 18:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 32455: Cleanup and expand ExpireReservesAutoFill.t (8.25 KB, patch)
2023-01-28 18:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 32455: Use from_email_address for 'from' field for hold notices (2.47 KB, patch)
2023-02-01 13:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32455: Unit tests (4.24 KB, patch)
2023-02-01 13:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32455: Cleanup and expand ExpireReservesAutoFill.t (8.31 KB, patch)
2023-02-01 13:37 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 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 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 2022-12-14 14:22:43 UTC
Created attachment 144579 [details] [review]
Bug 32455: Unit tests
Comment 5 Nick Clemens 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 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 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 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 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 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 2023-02-14 22:41:26 UTC
Missing dependencies for 22.05.x, no backport