Bug 22818 - ILL should be able to send notices
Summary: ILL should be able to send notices
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature with 1 vote (vote)
Assignee: Andrew Isherwood
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
: 18588 20917 23614 23615 24256 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-05-01 10:23 UTC by Andrew Isherwood
Modified: 2022-06-06 20:25 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patch adds the ability for ILL to send notices, both triggered by staff and triggered by events. Staff can trigger notices to patrons from the "Manage ILL request" screen: - ILL request ready for pickup - ILL request unavailable - Place request with partners The following notices to staff are triggered automatically: - Request has been modified by patron - Request has been cancelled by patron Branches can now specify an "ILL email" address to which notices intended to inform staff of changes to requests by patrons can be sent. The sending of notices is controlled by a few new sysprefs: - "ILLDefaultStaffEmail" - Fallback email address for staff ILL notices to be sent to in the absence of a branch address - "ILLSendStaffNotices" - To specify which staff notices should be sent automatically when requests are manipulated by patrons Patron notices are also controlled by the patron's messaging preferences.
Version(s) released in:
20.11.00


Attachments
Bug 22818: Add notices sysprefs and DB column (21.26 KB, patch)
2019-05-16 13:50 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add support for ILL notices (4.44 KB, patch)
2019-05-16 13:50 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add generation and sending of notices (23.40 KB, patch)
2019-05-16 13:50 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add MessageAttribut{e,es}.pm (3.06 KB, patch)
2019-05-16 13:50 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Unit tests (9.74 KB, patch)
2019-05-16 13:50 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add notices sysprefs and DB column (21.26 KB, patch)
2019-05-24 15:38 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add support for ILL notices (4.43 KB, patch)
2019-05-24 15:38 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add generation and sending of notices (23.40 KB, patch)
2019-05-24 15:38 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add MessageAttribut{e,es}.pm (3.06 KB, patch)
2019-05-24 15:38 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Unit tests (9.74 KB, patch)
2019-05-24 15:38 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add notices sysprefs and DB column (21.35 KB, patch)
2019-06-18 08:24 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 22818: Add support for ILL notices (4.53 KB, patch)
2019-06-18 08:24 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 22818: Add generation and sending of notices (23.50 KB, patch)
2019-06-18 08:24 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 22818: Add MessageAttribut{e,es}.pm (3.16 KB, patch)
2019-06-18 08:24 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 22818: Unit tests (9.83 KB, patch)
2019-06-18 08:24 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 22818: Add notices sysprefs and DB column (21.35 KB, patch)
2019-09-17 14:14 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add support for ILL notices (4.77 KB, patch)
2019-09-17 14:14 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add generation and sending of notices (23.50 KB, patch)
2019-09-17 14:14 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add MessageAttribut{e,es}.pm (3.16 KB, patch)
2019-09-17 14:14 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Unit tests (9.83 KB, patch)
2019-09-17 14:14 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add notices sysprefs and DB column (21.00 KB, patch)
2020-03-06 10:22 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add support for ILL notices (4.77 KB, patch)
2020-03-06 10:22 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add generation and sending of notices (23.50 KB, patch)
2020-03-06 10:22 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add MessageAttribut{e,es}.pm (3.16 KB, patch)
2020-03-06 10:22 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Unit tests (9.83 KB, patch)
2020-03-06 10:22 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add notices sysprefs and DB column (20.85 KB, patch)
2020-04-20 10:58 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add support for ILL notices (4.78 KB, patch)
2020-04-20 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add generation and sending of notices (23.49 KB, patch)
2020-04-20 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add MessageAttribut{e,es}.pm (3.14 KB, patch)
2020-04-20 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Unit tests (9.80 KB, patch)
2020-04-20 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add notices sysprefs and DB column (20.85 KB, patch)
2020-06-30 08:35 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add support for ILL notices (4.78 KB, patch)
2020-06-30 08:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add generation and sending of notices (23.49 KB, patch)
2020-06-30 08:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add MessageAttribut{e,es}.pm (3.14 KB, patch)
2020-06-30 08:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Unit tests (9.80 KB, patch)
2020-06-30 08:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add notices sysprefs and DB column (20.17 KB, patch)
2020-07-09 07:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: Add support for ILL notices (4.84 KB, patch)
2020-07-09 07:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: Add generation and sending of notices (23.56 KB, patch)
2020-07-09 07:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: Add MessageAttribut{e,es}.pm (3.20 KB, patch)
2020-07-09 07:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: Unit tests (9.87 KB, patch)
2020-07-09 07:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Email addressing corrections (12.58 KB, patch)
2020-07-09 07:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Licence and POD fixes (4.12 KB, patch)
2020-07-09 08:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Template CASE correction (1.42 KB, patch)
2020-07-09 08:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Template CASE correction (1.42 KB, patch)
2020-07-09 09:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Translation fix + Branch notices (1.45 KB, patch)
2020-07-09 09:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: Add notices sysprefs and DB column (20.63 KB, patch)
2020-07-09 11:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: Add support for ILL notices (4.84 KB, patch)
2020-07-09 11:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: Add generation and sending of notices (23.56 KB, patch)
2020-07-09 11:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: Add MessageAttribut{e,es}.pm (3.20 KB, patch)
2020-07-09 11:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: Unit tests (9.87 KB, patch)
2020-07-09 11:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Email addressing corrections (12.79 KB, patch)
2020-07-09 11:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Licence and POD fixes (4.18 KB, patch)
2020-07-09 11:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Template CASE correction (1.49 KB, patch)
2020-07-09 11:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Translation fix + Branch notices (1.51 KB, patch)
2020-07-09 11:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Sample Notices (23.51 KB, patch)
2020-07-09 11:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: Add notices sysprefs and DB column (20.81 KB, patch)
2020-09-16 13:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: Add support for ILL notices (4.84 KB, patch)
2020-09-16 13:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: Add generation and sending of notices (23.56 KB, patch)
2020-09-16 13:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: Add MessageAttribut{e,es}.pm (3.20 KB, patch)
2020-09-16 13:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: Unit tests (9.87 KB, patch)
2020-09-16 13:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Email addressing corrections (12.79 KB, patch)
2020-09-16 13:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Licence and POD fixes (3.61 KB, patch)
2020-09-16 13:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Template CASE correction (1.49 KB, patch)
2020-09-16 13:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Translation fix + Branch notices (1.51 KB, patch)
2020-09-16 13:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Sample Notices (23.51 KB, patch)
2020-09-16 13:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: (DO NOT PUSH) Schema Updates (3.07 KB, patch)
2020-09-16 13:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Add mandatory data at install time (1.94 KB, patch)
2020-09-16 14:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22818: Add notices sysprefs and DB column (20.47 KB, patch)
2020-10-16 09:54 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add support for ILL notices (4.84 KB, patch)
2020-10-16 09:54 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add generation and sending of notices (23.50 KB, patch)
2020-10-16 09:54 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add MessageAttribut{e,es}.pm (3.20 KB, patch)
2020-10-16 09:54 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Unit tests (9.87 KB, patch)
2020-10-16 09:55 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Email addressing corrections (12.79 KB, patch)
2020-10-16 09:55 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Licence and POD fixes (3.61 KB, patch)
2020-10-16 09:55 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Template CASE correction (1.49 KB, patch)
2020-10-16 09:55 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Translation fix + Branch notices (1.51 KB, patch)
2020-10-16 09:55 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Sample Notices (23.51 KB, patch)
2020-10-16 09:55 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Add mandatory data at install time (1.94 KB, patch)
2020-10-16 09:55 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (follow-up) Respond to feedback (21.59 KB, patch)
2020-10-16 09:55 UTC, Andrew Isherwood
Details | Diff | Splinter Review
ILL view in OPAC - button (70.03 KB, image/png)
2020-10-16 13:30 UTC, Katrin Fischer
Details
Bug 22818: Add notices sysprefs and DB column (20.47 KB, patch)
2020-10-16 14:34 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add support for ILL notices (4.84 KB, patch)
2020-10-16 14:35 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add generation and sending of notices (23.50 KB, patch)
2020-10-16 14:35 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add MessageAttribut{e,es}.pm (3.20 KB, patch)
2020-10-16 14:35 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Unit tests (9.87 KB, patch)
2020-10-16 14:35 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Email addressing corrections (12.79 KB, patch)
2020-10-16 14:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Licence and POD fixes (3.61 KB, patch)
2020-10-16 14:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Template CASE correction (1.49 KB, patch)
2020-10-16 14:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Translation fix + Branch notices (1.51 KB, patch)
2020-10-16 14:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Sample Notices (23.51 KB, patch)
2020-10-16 14:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Add mandatory data at install time (1.94 KB, patch)
2020-10-16 14:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (follow-up) Respond to feedback (21.59 KB, patch)
2020-10-16 14:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (follow-up) Respond to feedback (65.77 KB, patch)
2020-10-16 14:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add notices sysprefs and DB column (20.59 KB, patch)
2020-10-16 15:20 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22818: Add support for ILL notices (4.90 KB, patch)
2020-10-16 15:20 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22818: Add generation and sending of notices (23.59 KB, patch)
2020-10-16 15:20 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22818: Add MessageAttribut{e,es}.pm (3.28 KB, patch)
2020-10-16 15:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22818: Unit tests (9.93 KB, patch)
2020-10-16 15:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Email addressing corrections (12.87 KB, patch)
2020-10-16 15:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Licence and POD fixes (3.66 KB, patch)
2020-10-16 15:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Template CASE correction (1.54 KB, patch)
2020-10-16 15:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Translation fix + Branch notices (1.57 KB, patch)
2020-10-16 15:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Sample Notices (23.58 KB, patch)
2020-10-16 15:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Add mandatory data at install time (2.01 KB, patch)
2020-10-16 15:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22818: (follow-up) Respond to feedback (21.70 KB, patch)
2020-10-16 15:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22818: (follow-up) Respond to feedback (65.89 KB, patch)
2020-10-16 15:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Add sample ILL notices to it-IT installer (7.30 KB, patch)
2020-10-16 15:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Add some punctuation and terminology fixes to pref descriptions (1.77 KB, patch)
2020-10-16 15:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Add hint to new library ILL email setting (2.28 KB, patch)
2020-10-16 15:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22818: Add notices sysprefs and DB column (20.53 KB, patch)
2020-11-10 14:17 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add support for ILL notices (4.90 KB, patch)
2020-11-10 14:17 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add generation and sending of notices (23.55 KB, patch)
2020-11-10 14:17 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add MessageAttribut{e,es}.pm (3.26 KB, patch)
2020-11-10 14:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Unit tests (9.92 KB, patch)
2020-11-10 14:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Email addressing corrections (12.85 KB, patch)
2020-11-10 14:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Licence and POD fixes (3.66 KB, patch)
2020-11-10 14:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Template CASE correction (1.54 KB, patch)
2020-11-10 14:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Translation fix + Branch notices (1.57 KB, patch)
2020-11-10 14:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Sample Notices (23.57 KB, patch)
2020-11-10 14:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Add mandatory data at install time (2.00 KB, patch)
2020-11-10 14:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (follow-up) Respond to feedback (21.65 KB, patch)
2020-11-10 14:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (follow-up) Respond to feedback (62.51 KB, patch)
2020-11-10 14:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Add sample ILL notices to it-IT installer (7.30 KB, patch)
2020-11-10 14:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Add some punctuation and terminology fixes to pref descriptions (1.76 KB, patch)
2020-11-10 14:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Add hint to new library ILL email setting (2.28 KB, patch)
2020-11-10 14:19 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add notices sysprefs and DB column (20.53 KB, patch)
2020-11-11 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add support for ILL notices (4.90 KB, patch)
2020-11-11 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add generation and sending of notices (23.55 KB, patch)
2020-11-11 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Add MessageAttribut{e,es}.pm (3.26 KB, patch)
2020-11-11 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Unit tests (9.92 KB, patch)
2020-11-11 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Email addressing corrections (12.85 KB, patch)
2020-11-11 10:59 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Licence and POD fixes (3.66 KB, patch)
2020-11-11 11:00 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Template CASE correction (1.54 KB, patch)
2020-11-11 11:00 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Translation fix + Branch notices (1.57 KB, patch)
2020-11-11 11:00 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Sample Notices (23.57 KB, patch)
2020-11-11 11:00 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Add mandatory data at install time (2.00 KB, patch)
2020-11-11 11:00 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (follow-up) Respond to feedback (21.65 KB, patch)
2020-11-11 11:00 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (follow-up) Respond to feedback (62.51 KB, patch)
2020-11-11 11:00 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Add sample ILL notices to it-IT installer (7.30 KB, patch)
2020-11-11 11:00 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Add some punctuation and terminology fixes to pref descriptions (1.76 KB, patch)
2020-11-11 11:00 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Add hint to new library ILL email setting (2.28 KB, patch)
2020-11-11 11:00 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: (follow-up) "use" correct class (838 bytes, patch)
2020-11-11 11:00 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Fix TestBuilder.t (1.33 KB, patch)
2020-11-11 17:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22818: (QA follow-up) Respond to feedback (3.81 KB, patch)
2020-11-12 11:32 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 22818: Fix the last_insert_id call (3.34 KB, patch)
2020-11-13 08:17 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Isherwood 2019-05-01 10:23:18 UTC
Currently ILL has no ability to send notices. There are a number of events that would warrant a notice being send out, these include:

To patron:
- Request ready for pickup
- Item requested not available

To staff:
- Request cancelled by patron
- Request modified by patron

To supplier:
- "Request from partner"

The last one, "Request from partner", is currently send via an ad-hoc function, but would make sense to convert to a notice.
Comment 1 Andrew Isherwood 2019-05-16 13:50:45 UTC
Created attachment 89831 [details] [review]
Bug 22818: Add notices sysprefs and DB column

This patch adds two new sysprefs. 'ILLDefaultEmail', 'ILLSendStaffNotices' and a new column to the
branches table called 'branchillemail'. It also adds five ILL related
notices.

Here we are providing the ability for a branch to have a dedicated email
address for it's ILL staff, the idea being that any notices sent by ILL
to staff need to go to specific staff, rather than the general branch
email address. If no branch specific address is defined, the address
specified in the ILLDefaultEmail syspref is used.

We're also providing a syspref 'ILLSendStaffNotices' to allow the user to specify which
notices should be sent to staff upon certain events.

We are also providing patron messaging preferences for the two patron
bound notices

We are also providing five notices for various ILL related events.
Comment 2 Andrew Isherwood 2019-05-16 13:50:47 UTC
Created attachment 89832 [details] [review]
Bug 22818: Add support for ILL notices

Here we're just adding ILL notices to the existing "letters"
infrastructure so notices can be displayed and edited
Comment 3 Andrew Isherwood 2019-05-16 13:50:50 UTC
Created attachment 89833 [details] [review]
Bug 22818: Add generation and sending of notices

This patch adds the ability for ILL to send notices, both triggered by
staff and triggered by events.

Staff can trigger notices to patrons from the "Manage ILL request" screen:
- ILL request ready for pickup
- ILL request unavailable
- Place request with partners

The following notices to staff are triggered automatically:
- Request has been modified by patron
- Request has been cancelled by patron

Branches can now specify an "ILL email" address to which notices
intended to inform staff of changes to requests by patrons can be sent.

The sending of notices is controlled by a few new sysprefs:
- "ILLDefaultStaffEmail" - Fallback email address for staff ILL notices
to be sent to in the absence of a branch address
- "ILLSendStaffNotices" - To specify which staff notices should be sent
automatically when requests are manipulated by patrons

Patron notices are also controlled by the patron's messaging
preferences
Comment 4 Andrew Isherwood 2019-05-16 13:50:52 UTC
Created attachment 89834 [details] [review]
Bug 22818: Add MessageAttribut{e,es}.pm

We need to be able to access individual message attributes in tests, so
adding accessors for them
Comment 5 Andrew Isherwood 2019-05-16 13:50:55 UTC
Created attachment 89835 [details] [review]
Bug 22818: Unit tests

This patch adds unit tests for the new methods in Illrequest.pm
Comment 6 Andrew Isherwood 2019-05-16 13:51:23 UTC
- Apply patch
- Update database and schema

- Create a report using the following SQL in order to verify that notices are being generated:
SELECT borrowernumber, subject, content, message_transport_type, to_address, from_address FROM message_queue WHERE letter_code LIKE 'ILL%' ORDER BY message_id DESC

- Create one or more "partners". These are patrons that belong to a patron category that has a code that matches the <partner_code> value in your koha-conf.xml (default is ILLLIBS). Patrons in this category must have a primary email defined. Patrons defined in this way are offered as request partners within the ILL interface.

SYSTEM PREFERENCES AND BRANCH CONFIG
- In the staff interface, go to Tools > Notices & Slips
=> TEST: Observe that there are 5 new notices defined for the "Interlibrary loans" module
- Go to "Koha administration" > "Libraries"
- Choose a library and "Edit" it
=> TEST: Observe there is a new "ILL staff email" field
- Enter an email address in the "ILL staff email" field and save the branch
- Go to "Koha administration" > search for the "ILLDefaultStaffEmail" syspref
=> TEST: Observe the new syspref exists
- Enter an email address in this syspref and then save it
- Go to "Koha administration" > search for the "ILLSendStaffNotices" syspref
=> TEST: Observe the new syspref exists
- Enter the names of the two staff notices in this syspref as "ILL_REQUEST_CANCEL|ILL_REQUEST_MODIFIED" and save the syspref

PATRON MESSAGING PREFERENCES
- Go to a patron's messaging preferences
=> TEST: Observe that two new messaging preferences exist, "Interlibrary loan ready" & "Interlibrary loan unavailable"
- Select email and/or SMS for each of these notices

SENDING PATRON NOTICES FROM ILL REQUEST
- In the staff interface, go to "ILL requests"
- Create an ILL request using the FreeForm backend (for the user you just modified the messaging preferences for), select the branch that you previously added the "ILL staff email" address to
- Go to the "Manage ILL request" screen for the newly created request
=> TEST: Observe that there is a new "Send notice to patron" dropdown in the button bar which offers two notices to send
- Select a notice to be sent
=> TEST: Observe that a message reporting the successful queueing of the notice is displayed
- Click on the "ILL request log" for the request
=> TEST: Observe that a log entry "Patron notice sent:" was added to the log
- Run the report created earlier
=> TEST: Observe that a notice was created for all sending methods selected in the patron's messaging preferences
=> TEST: Observe that the borrowernumber column of the notice is populated

SENDING REQUEST TO PARTNERS
- Go to the "Manage ILL request" screen for a request
- Choose "Place request with partners"
=> TEST: Observe that the text defined in the ILL_PARTNER_REQ notice is displayed and can be modified
- Select one or more partners that were defined earlier, then click "Send email"
=> TEST: Observe that the request's status has changed to "Requested from partners"
- Run the report created earlier
=> TEST: Observe that a notice was created for each selected partner
=> TEST: Observe that the to_address column matches the primary email address for the partner
=> TEST: Observe that the from_address matches the previously defined "ILL staff email" for the request's branch

PATRON GENERATED NOTICES
- As a patron with ILL requests, go to the "your interlibrary loan requests" page in the OPAC
- View a request
- Add some text to the notes field and save
- Run the report created earlier
=> TEST: Observe that a notice was created for the modification
=> TEST: Observe that the to_address matches the previously defined "ILL staff email" for the request's branch
=> TEST: Observe that the from_address matches the previously defined "ILL staff email" for the request's branch
- As a patron with ILL requests, go to the "your interlibrary loan requests" page in the OPAC
- View a request
- Choose "Request cancellation"
- Run the report created earlier
=> TEST: Observe that a notice was created for the modification
=> TEST: Observe that the to_address matches the previously defined "ILL staff email" for the request's branch
=> TEST: Observe that the from_address matches the previously defined "ILL staff email" for the request's branch

There is a fallback hierarchy for the address to which staff notices are sent:

Branch ILL staff email > ILLDefaultStaffEmail > General branch email > Koha admin email

Please test that this hierarchy is adhered to by undefining one or more of these options.
Comment 7 Andrew Isherwood 2019-05-24 11:54:04 UTC
*** Bug 18588 has been marked as a duplicate of this bug. ***
Comment 8 Andrew Isherwood 2019-05-24 15:38:21 UTC
Created attachment 90063 [details] [review]
Bug 22818: Add notices sysprefs and DB column

This patch adds two new sysprefs. 'ILLDefaultEmail', 'ILLSendStaffNotices' and a new column to the
branches table called 'branchillemail'. It also adds five ILL related
notices.

Here we are providing the ability for a branch to have a dedicated email
address for it's ILL staff, the idea being that any notices sent by ILL
to staff need to go to specific staff, rather than the general branch
email address. If no branch specific address is defined, the address
specified in the ILLDefaultEmail syspref is used.

We're also providing a syspref 'ILLSendStaffNotices' to allow the user to specify which
notices should be sent to staff upon certain events.

We are also providing patron messaging preferences for the two patron
bound notices

We are also providing five notices for various ILL related events.
Comment 9 Andrew Isherwood 2019-05-24 15:38:24 UTC
Created attachment 90064 [details] [review]
Bug 22818: Add support for ILL notices

Here we're just adding ILL notices to the existing "letters"
infrastructure so notices can be displayed and edited
Comment 10 Andrew Isherwood 2019-05-24 15:38:27 UTC
Created attachment 90065 [details] [review]
Bug 22818: Add generation and sending of notices

This patch adds the ability for ILL to send notices, both triggered by
staff and triggered by events.

Staff can trigger notices to patrons from the "Manage ILL request" screen:
- ILL request ready for pickup
- ILL request unavailable
- Place request with partners

The following notices to staff are triggered automatically:
- Request has been modified by patron
- Request has been cancelled by patron

Branches can now specify an "ILL email" address to which notices
intended to inform staff of changes to requests by patrons can be sent.

The sending of notices is controlled by a few new sysprefs:
- "ILLDefaultStaffEmail" - Fallback email address for staff ILL notices
to be sent to in the absence of a branch address
- "ILLSendStaffNotices" - To specify which staff notices should be sent
automatically when requests are manipulated by patrons

Patron notices are also controlled by the patron's messaging
preferences
Comment 11 Andrew Isherwood 2019-05-24 15:38:30 UTC
Created attachment 90066 [details] [review]
Bug 22818: Add MessageAttribut{e,es}.pm

We need to be able to access individual message attributes in tests, so
adding accessors for them
Comment 12 Andrew Isherwood 2019-05-24 15:38:32 UTC
Created attachment 90067 [details] [review]
Bug 22818: Unit tests

This patch adds unit tests for the new methods in Illrequest.pm
Comment 13 Andrew Isherwood 2019-06-03 13:11:50 UTC
Slightly revised test plan:

- Apply patch
- Update database and schema

- Create a report using the following SQL in order to verify that notices are being generated:
SELECT borrowernumber, subject, content, message_transport_type, to_address, from_address FROM message_queue WHERE letter_code LIKE 'ILL%' ORDER BY message_id DESC

- Create one or more "partners". These are patrons that belong to a patron category that has a code that matches the <partner_code> value in your koha-conf.xml (default is ILLLIBS). Patrons in this category must have a primary email defined. Patrons defined in this way are offered as request partners within the ILL interface.

- Go to "Koha administration", search for "IllLog" syspref, ensure it is set to "Log"

SYSTEM PREFERENCES AND BRANCH CONFIG
- In the staff interface, go to Tools > Notices & Slips
=> TEST: Observe that there are 5 new notices defined for the "Interlibrary loans" module
- Go to "Koha administration" > "Libraries"
- Choose a library and "Edit" it
=> TEST: Observe there is a new "ILL staff email" field
- Enter an email address in the "ILL staff email" field and save the branch
- Go to "Koha administration" > search for the "ILLDefaultStaffEmail" syspref
=> TEST: Observe the new syspref exists
- Enter an email address in this syspref and then save it
- Go to "Koha administration" > search for the "ILLSendStaffNotices" syspref
=> TEST: Observe the new syspref exists
- Enter the names of the two staff notices in this syspref as "ILL_REQUEST_CANCEL|ILL_REQUEST_MODIFIED" and save the syspref

PATRON MESSAGING PREFERENCES
- Go to a patron's messaging preferences
=> TEST: Observe that two new messaging preferences exist, "Interlibrary loan ready" & "Interlibrary loan unavailable"
- Select email and/or SMS for each of these notices

SENDING PATRON NOTICES FROM ILL REQUEST
- In the staff interface, go to "ILL requests"
- Create an ILL request using the FreeForm backend (for the user you just modified the messaging preferences for), select the branch that you previously added the "ILL staff email" address to
- Go to the "Manage ILL request" screen for the newly created request
=> TEST: Observe that there is a new "Send notice to patron" dropdown in the button bar which offers two notices to send
- Select a notice to be sent
=> TEST: Observe that a message reporting the successful queueing of the notice is displayed
- Click on the "ILL request log" for the request
=> TEST: Observe that a log entry "Patron notice sent:" was added to the log
- Run the report created earlier
=> TEST: Observe that a notice was created for all sending methods selected in the patron's messaging preferences
=> TEST: Observe that the borrowernumber column of the notice is populated

SENDING REQUEST TO PARTNERS
- Go to the "Manage ILL request" screen for a request
- Choose "Place request with partners"
=> TEST: Observe that the text defined in the ILL_PARTNER_REQ notice is displayed and can be modified
- Select one or more partners that were defined earlier, then click "Send email"
=> TEST: Observe that the request's status has changed to "Requested from partners"
- Run the report created earlier
=> TEST: Observe that a notice was created for each selected partner
=> TEST: Observe that the to_address column matches the primary email address for the partner
=> TEST: Observe that the from_address matches the previously defined "ILL staff email" for the request's branch

PATRON GENERATED NOTICES
- As a patron with ILL requests, go to the "your interlibrary loan requests" page in the OPAC
- View a request
- Add some text to the notes field and save
- Run the report created earlier
=> TEST: Observe that a notice was created for the modification
=> TEST: Observe that the to_address matches the previously defined "ILL staff email" for the request's branch
=> TEST: Observe that the from_address matches the previously defined "ILL staff email" for the request's branch
- As a patron with ILL requests, go to the "your interlibrary loan requests" page in the OPAC
- View a request
- Choose "Request cancellation"
- Run the report created earlier
=> TEST: Observe that a notice was created for the modification
=> TEST: Observe that the to_address matches the previously defined "ILL staff email" for the request's branch
=> TEST: Observe that the from_address matches the previously defined "ILL staff email" for the request's branch

There is a fallback hierarchy for the address to which staff notices are sent:

Branch ILL staff email > ILLDefaultStaffEmail > General branch email > Koha admin email

Please test that this hierarchy is adhered to by undefining one or more of these options.
Comment 14 PTFS Europe Sandboxes 2019-06-18 08:24:09 UTC
Created attachment 90715 [details] [review]
Bug 22818: Add notices sysprefs and DB column

This patch adds two new sysprefs. 'ILLDefaultEmail', 'ILLSendStaffNotices' and a new column to the
branches table called 'branchillemail'. It also adds five ILL related
notices.

Here we are providing the ability for a branch to have a dedicated email
address for it's ILL staff, the idea being that any notices sent by ILL
to staff need to go to specific staff, rather than the general branch
email address. If no branch specific address is defined, the address
specified in the ILLDefaultEmail syspref is used.

We're also providing a syspref 'ILLSendStaffNotices' to allow the user to specify which
notices should be sent to staff upon certain events.

We are also providing patron messaging preferences for the two patron
bound notices

We are also providing five notices for various ILL related events.

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 15 PTFS Europe Sandboxes 2019-06-18 08:24:13 UTC
Created attachment 90716 [details] [review]
Bug 22818: Add support for ILL notices

Here we're just adding ILL notices to the existing "letters"
infrastructure so notices can be displayed and edited

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 16 PTFS Europe Sandboxes 2019-06-18 08:24:16 UTC
Created attachment 90717 [details] [review]
Bug 22818: Add generation and sending of notices

This patch adds the ability for ILL to send notices, both triggered by
staff and triggered by events.

Staff can trigger notices to patrons from the "Manage ILL request" screen:
- ILL request ready for pickup
- ILL request unavailable
- Place request with partners

The following notices to staff are triggered automatically:
- Request has been modified by patron
- Request has been cancelled by patron

Branches can now specify an "ILL email" address to which notices
intended to inform staff of changes to requests by patrons can be sent.

The sending of notices is controlled by a few new sysprefs:
- "ILLDefaultStaffEmail" - Fallback email address for staff ILL notices
to be sent to in the absence of a branch address
- "ILLSendStaffNotices" - To specify which staff notices should be sent
automatically when requests are manipulated by patrons

Patron notices are also controlled by the patron's messaging
preferences

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 17 PTFS Europe Sandboxes 2019-06-18 08:24:19 UTC
Created attachment 90718 [details] [review]
Bug 22818: Add MessageAttribut{e,es}.pm

We need to be able to access individual message attributes in tests, so
adding accessors for them

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 18 PTFS Europe Sandboxes 2019-06-18 08:24:22 UTC
Created attachment 90719 [details] [review]
Bug 22818: Unit tests

This patch adds unit tests for the new methods in Illrequest.pm

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 19 Andrew Isherwood 2019-08-02 13:00:29 UTC
*** Bug 20917 has been marked as a duplicate of this bug. ***
Comment 20 Andrew Isherwood 2019-09-17 14:14:41 UTC
Created attachment 92898 [details] [review]
Bug 22818: Add notices sysprefs and DB column

This patch adds two new sysprefs. 'ILLDefaultEmail', 'ILLSendStaffNotices' and a new column to the
branches table called 'branchillemail'. It also adds five ILL related
notices.

Here we are providing the ability for a branch to have a dedicated email
address for it's ILL staff, the idea being that any notices sent by ILL
to staff need to go to specific staff, rather than the general branch
email address. If no branch specific address is defined, the address
specified in the ILLDefaultEmail syspref is used.

We're also providing a syspref 'ILLSendStaffNotices' to allow the user to specify which
notices should be sent to staff upon certain events.

We are also providing patron messaging preferences for the two patron
bound notices

We are also providing five notices for various ILL related events.

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 21 Andrew Isherwood 2019-09-17 14:14:44 UTC
Created attachment 92899 [details] [review]
Bug 22818: Add support for ILL notices

Here we're just adding ILL notices to the existing "letters"
infrastructure so notices can be displayed and edited

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 22 Andrew Isherwood 2019-09-17 14:14:48 UTC
Created attachment 92900 [details] [review]
Bug 22818: Add generation and sending of notices

This patch adds the ability for ILL to send notices, both triggered by
staff and triggered by events.

Staff can trigger notices to patrons from the "Manage ILL request" screen:
- ILL request ready for pickup
- ILL request unavailable
- Place request with partners

The following notices to staff are triggered automatically:
- Request has been modified by patron
- Request has been cancelled by patron

Branches can now specify an "ILL email" address to which notices
intended to inform staff of changes to requests by patrons can be sent.

The sending of notices is controlled by a few new sysprefs:
- "ILLDefaultStaffEmail" - Fallback email address for staff ILL notices
to be sent to in the absence of a branch address
- "ILLSendStaffNotices" - To specify which staff notices should be sent
automatically when requests are manipulated by patrons

Patron notices are also controlled by the patron's messaging
preferences

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 23 Andrew Isherwood 2019-09-17 14:14:51 UTC
Created attachment 92901 [details] [review]
Bug 22818: Add MessageAttribut{e,es}.pm

We need to be able to access individual message attributes in tests, so
adding accessors for them

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 24 Andrew Isherwood 2019-09-17 14:14:55 UTC
Created attachment 92902 [details] [review]
Bug 22818: Unit tests

This patch adds unit tests for the new methods in Illrequest.pm

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 25 Andrew Isherwood 2019-09-17 15:10:31 UTC
*** Bug 23614 has been marked as a duplicate of this bug. ***
Comment 26 Andrew Isherwood 2019-09-17 15:10:54 UTC
*** Bug 23615 has been marked as a duplicate of this bug. ***
Comment 27 Tomás Cohen Arazi 2019-09-17 16:56:45 UTC
I'm starting to test this. I would like to say that I see more and more the need to have an ILL configuration section in the staff interface.
Comment 28 Katrin Fischer 2019-12-22 13:58:28 UTC
*** Bug 24256 has been marked as a duplicate of this bug. ***
Comment 29 Andrew Isherwood 2020-03-06 10:22:34 UTC
Created attachment 100234 [details] [review]
Bug 22818: Add notices sysprefs and DB column

This patch adds two new sysprefs. 'ILLDefaultEmail', 'ILLSendStaffNotices' and a new column to the
branches table called 'branchillemail'. It also adds five ILL related
notices.

Here we are providing the ability for a branch to have a dedicated email
address for it's ILL staff, the idea being that any notices sent by ILL
to staff need to go to specific staff, rather than the general branch
email address. If no branch specific address is defined, the address
specified in the ILLDefaultEmail syspref is used.

We're also providing a syspref 'ILLSendStaffNotices' to allow the user to specify which
notices should be sent to staff upon certain events.

We are also providing patron messaging preferences for the two patron
bound notices

We are also providing five notices for various ILL related events.

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 30 Andrew Isherwood 2020-03-06 10:22:37 UTC
Created attachment 100235 [details] [review]
Bug 22818: Add support for ILL notices

Here we're just adding ILL notices to the existing "letters"
infrastructure so notices can be displayed and edited

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 31 Andrew Isherwood 2020-03-06 10:22:41 UTC
Created attachment 100236 [details] [review]
Bug 22818: Add generation and sending of notices

This patch adds the ability for ILL to send notices, both triggered by
staff and triggered by events.

Staff can trigger notices to patrons from the "Manage ILL request" screen:
- ILL request ready for pickup
- ILL request unavailable
- Place request with partners

The following notices to staff are triggered automatically:
- Request has been modified by patron
- Request has been cancelled by patron

Branches can now specify an "ILL email" address to which notices
intended to inform staff of changes to requests by patrons can be sent.

The sending of notices is controlled by a few new sysprefs:
- "ILLDefaultStaffEmail" - Fallback email address for staff ILL notices
to be sent to in the absence of a branch address
- "ILLSendStaffNotices" - To specify which staff notices should be sent
automatically when requests are manipulated by patrons

Patron notices are also controlled by the patron's messaging
preferences

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 32 Andrew Isherwood 2020-03-06 10:22:44 UTC
Created attachment 100237 [details] [review]
Bug 22818: Add MessageAttribut{e,es}.pm

We need to be able to access individual message attributes in tests, so
adding accessors for them

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 33 Andrew Isherwood 2020-03-06 10:22:48 UTC
Created attachment 100238 [details] [review]
Bug 22818: Unit tests

This patch adds unit tests for the new methods in Illrequest.pm

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 34 Katrin Fischer 2020-04-19 21:55:57 UTC
Hi Andrew, the patches no longer apply, sorry for not getting here earlier. Can you please rebase?
Comment 35 Andrew Isherwood 2020-04-20 10:58:58 UTC
Created attachment 103268 [details] [review]
Bug 22818: Add notices sysprefs and DB column

This patch adds two new sysprefs. 'ILLDefaultEmail', 'ILLSendStaffNotices' and a new column to the
branches table called 'branchillemail'. It also adds five ILL related
notices.

Here we are providing the ability for a branch to have a dedicated email
address for it's ILL staff, the idea being that any notices sent by ILL
to staff need to go to specific staff, rather than the general branch
email address. If no branch specific address is defined, the address
specified in the ILLDefaultEmail syspref is used.

We're also providing a syspref 'ILLSendStaffNotices' to allow the user to specify which
notices should be sent to staff upon certain events.

We are also providing patron messaging preferences for the two patron
bound notices

We are also providing five notices for various ILL related events.

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 36 Andrew Isherwood 2020-04-20 10:59:02 UTC
Created attachment 103269 [details] [review]
Bug 22818: Add support for ILL notices

Here we're just adding ILL notices to the existing "letters"
infrastructure so notices can be displayed and edited

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 37 Andrew Isherwood 2020-04-20 10:59:06 UTC
Created attachment 103270 [details] [review]
Bug 22818: Add generation and sending of notices

This patch adds the ability for ILL to send notices, both triggered by
staff and triggered by events.

Staff can trigger notices to patrons from the "Manage ILL request" screen:
- ILL request ready for pickup
- ILL request unavailable
- Place request with partners

The following notices to staff are triggered automatically:
- Request has been modified by patron
- Request has been cancelled by patron

Branches can now specify an "ILL email" address to which notices
intended to inform staff of changes to requests by patrons can be sent.

The sending of notices is controlled by a few new sysprefs:
- "ILLDefaultStaffEmail" - Fallback email address for staff ILL notices
to be sent to in the absence of a branch address
- "ILLSendStaffNotices" - To specify which staff notices should be sent
automatically when requests are manipulated by patrons

Patron notices are also controlled by the patron's messaging
preferences

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 38 Andrew Isherwood 2020-04-20 10:59:09 UTC
Created attachment 103271 [details] [review]
Bug 22818: Add MessageAttribut{e,es}.pm

We need to be able to access individual message attributes in tests, so
adding accessors for them

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 39 Andrew Isherwood 2020-04-20 10:59:14 UTC
Created attachment 103272 [details] [review]
Bug 22818: Unit tests

This patch adds unit tests for the new methods in Illrequest.pm

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 40 Andrew Isherwood 2020-04-20 11:01:39 UTC
(In reply to Katrin Fischer from comment #34)
> Hi Andrew, the patches no longer apply, sorry for not getting here earlier.
> Can you please rebase?

Hi Katrin

I've rebased. Unfortunately my Koha is in a broken state at the moment following an OS upgrade, but I believe the rebase is correct. See how you get on.
Comment 41 Katrin Fischer 2020-06-29 16:48:04 UTC
> Hi Katrin
> 
> I've rebased. Unfortunately my Koha is in a broken state at the moment
> following an OS upgrade, but I believe the rebase is correct. See how you
> get on.

Hi Andrew, I am sorry again that this took so long :( I have now setup my ILL environment and everything is ready for testing. Could you maybe rebase again? I'll also look at some other ILL related bugs if you have anything else you think of as important to have, please let me know.
Comment 42 Andrew Isherwood 2020-06-30 08:35:58 UTC
Created attachment 106424 [details] [review]
Bug 22818: Add notices sysprefs and DB column

This patch adds two new sysprefs. 'ILLDefaultEmail', 'ILLSendStaffNotices' and a new column to the
branches table called 'branchillemail'. It also adds five ILL related
notices.

Here we are providing the ability for a branch to have a dedicated email
address for it's ILL staff, the idea being that any notices sent by ILL
to staff need to go to specific staff, rather than the general branch
email address. If no branch specific address is defined, the address
specified in the ILLDefaultEmail syspref is used.

We're also providing a syspref 'ILLSendStaffNotices' to allow the user to specify which
notices should be sent to staff upon certain events.

We are also providing patron messaging preferences for the two patron
bound notices

We are also providing five notices for various ILL related events.

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 43 Andrew Isherwood 2020-06-30 08:36:01 UTC
Created attachment 106425 [details] [review]
Bug 22818: Add support for ILL notices

Here we're just adding ILL notices to the existing "letters"
infrastructure so notices can be displayed and edited

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 44 Andrew Isherwood 2020-06-30 08:36:05 UTC
Created attachment 106426 [details] [review]
Bug 22818: Add generation and sending of notices

This patch adds the ability for ILL to send notices, both triggered by
staff and triggered by events.

Staff can trigger notices to patrons from the "Manage ILL request" screen:
- ILL request ready for pickup
- ILL request unavailable
- Place request with partners

The following notices to staff are triggered automatically:
- Request has been modified by patron
- Request has been cancelled by patron

Branches can now specify an "ILL email" address to which notices
intended to inform staff of changes to requests by patrons can be sent.

The sending of notices is controlled by a few new sysprefs:
- "ILLDefaultStaffEmail" - Fallback email address for staff ILL notices
to be sent to in the absence of a branch address
- "ILLSendStaffNotices" - To specify which staff notices should be sent
automatically when requests are manipulated by patrons

Patron notices are also controlled by the patron's messaging
preferences

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 45 Andrew Isherwood 2020-06-30 08:36:09 UTC
Created attachment 106427 [details] [review]
Bug 22818: Add MessageAttribut{e,es}.pm

We need to be able to access individual message attributes in tests, so
adding accessors for them

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 46 Andrew Isherwood 2020-06-30 08:36:13 UTC
Created attachment 106428 [details] [review]
Bug 22818: Unit tests

This patch adds unit tests for the new methods in Illrequest.pm

Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Sponsored-by: PTFS Europe
Comment 47 Andrew Isherwood 2020-06-30 08:37:17 UTC
Hi Katrin - This is now rebased, so all should be well!
Comment 48 Katrin Fischer 2020-07-06 16:37:28 UTC
(In reply to Andrew Isherwood from comment #47)
> Hi Katrin - This is now rebased, so all should be well!

Hi Andrew, I tried to test, but it doesn't apply against master, because of a conflict in branches.tt caused by bug 25288 (pushed on 24 June). It's not a trivial fix, so I'd be happy if you could take a look, before I try.
Comment 49 Katrin Fischer 2020-07-06 16:44:07 UTC
(In reply to Katrin Fischer from comment #48)
> (In reply to Andrew Isherwood from comment #47)
> > Hi Katrin - This is now rebased, so all should be well!
> 
> Hi Andrew, I tried to test, but it doesn't apply against master, because of
> a conflict in branches.tt caused by bug 25288 (pushed on 24 June). It's not
> a trivial fix, so I'd be happy if you could take a look, before I try.

I might have gotten it fixed for now so I can continue testing... fingers crossed. 

Note: might need to re-add ill staff email to the library summary table.
Comment 50 Katrin Fischer 2020-07-06 18:10:04 UTC
I am still working on this, but adding some first notes and questions:

1) QA test tools

 FAIL	Koha/MessageAttributes.pm
   FAIL	  forbidden patterns
		forbidden pattern: Incorrect license statement (using postal address), may be a false positive if the file is coming from outside Koha (bug 24545) (line 16)

 FAIL	Koha/MessageAttribute.pm
   FAIL	  forbidden patterns
		forbidden pattern: Incorrect license statement (using postal address), may be a false positive if the file is coming from outside Koha (bug 24545) (line 16)

2) Unit tests

While tests pass, they produce some non-good looking output:

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Illrequests.t
t/db_dependent/Illrequests.t .. 7/12 Use of uninitialized value $staff_to_send in pattern match (m//) at /home/vagrant/kohaclone/Koha/Illrequest.pm line 1522.
	Koha::Illrequest::send_staff_notice(Koha::Illrequest=HASH(0x55b06c17ba40), "ILL_REQUEST_CANCEL") called at /home/vagrant/kohaclone/Koha/Illrequest.pm line 288
	Koha::Illrequest::status(Koha::Illrequest=HASH(0x55b06c17ba40), "CANCREQ") called at t/db_dependent/Illrequests.t line 788
	main::__ANON__() called at /usr/share/perl5/Test/Builder.pm line 310
	eval {...} called at /usr/share/perl5/Test/Builder.pm line 310
	Test::Builder::subtest(Test::Builder=HASH(0x55b06bac3bf8), "Helpers", CODE(0x55b063112be8)) called at /usr/share/perl5/Test/More.pm line 807
	Test::More::subtest("Helpers", CODE(0x55b063112be8)) called at t/db_dependent/Illrequests.t line 934
t/db_dependent/Illrequests.t .. ok     
All tests successful.
Files=1, Tests=12, 32 wallclock secs ( 0.09 usr  0.03 sys + 21.40 cusr  6.88 csys = 28.40 CPU)
Result: PASS

3) Code Review

3.1) Email addressing

I feel like here is a general issue here in that we will have problems with spam protection if the mail server is not in the same domain as the branchillemail given. 

Is the the new ILL staff email is to be used as the sender or the recipient of the ILL emails to staff? It looks like it's actually used as both? As the pref only reads "to", maybe we could just keep the current from and it would resolve a lot of the possible issues with spam.

I think we need to at least make use of $library->inbound_email_address in a couple of cases replacing the branchemail here:

+        my $from = $branch->branchillemail || $branch->branchemail;

get_staff_to_address seems ok in that regard, but send_staff_notice from_adress does not.
Reply-to and From are the spam critical ones.

Use of Koha::Email is removed?

3.2) Translatability

The "N/A" here are problematic. I think it would be better to just create an empty string? With TT libraries can still resolve that to another text.

+        substitute  => {
+            ill_bib_title      => $title ? $title->value : 'N/A',
+            ill_bib_author     => $author ? $author->value : 'N/A',

3.3) Sample letters

I need to take a closer look for typos and such, but the sample letters need to also be added to the new sample .yml and to the translated installers. 

3.4) Sysprefs.sql

New prefs are in the wrong alphabetic spot :)

3.5) Notices & slips

The module in the letters summary table is not translatable yet and shows as "ill".

-------------

Most of those are not bad ones, only 3.1 Email addressing worries me a fair bit. Clearing up intentions and intended behaviour would be a good firs step here.
Comment 51 Martin Renvoize 2020-07-09 07:45:54 UTC
Created attachment 106675 [details] [review]
Bug 22818: Add notices sysprefs and DB column

This patch adds two new sysprefs. 'ILLDefaultEmail', 'ILLSendStaffNotices' and a new column to the
branches table called 'branchillemail'. It also adds five ILL related
notices.

Here we are providing the ability for a branch to have a dedicated email
address for it's ILL staff, the idea being that any notices sent by ILL
to staff need to go to specific staff, rather than the general branch
email address. If no branch specific address is defined, the address
specified in the ILLDefaultEmail syspref is used.

We're also providing a syspref 'ILLSendStaffNotices' to allow the user to specify which
notices should be sent to staff upon certain events.

We are also providing patron messaging preferences for the two patron
bound notices

We are also providing five notices for various ILL related events.

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 52 Martin Renvoize 2020-07-09 07:45:58 UTC
Created attachment 106676 [details] [review]
Bug 22818: Add support for ILL notices

Here we're just adding ILL notices to the existing "letters"
infrastructure so notices can be displayed and edited

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 53 Martin Renvoize 2020-07-09 07:46:02 UTC
Created attachment 106677 [details] [review]
Bug 22818: Add generation and sending of notices

This patch adds the ability for ILL to send notices, both triggered by
staff and triggered by events.

Staff can trigger notices to patrons from the "Manage ILL request" screen:
- ILL request ready for pickup
- ILL request unavailable
- Place request with partners

The following notices to staff are triggered automatically:
- Request has been modified by patron
- Request has been cancelled by patron

Branches can now specify an "ILL email" address to which notices
intended to inform staff of changes to requests by patrons can be sent.

The sending of notices is controlled by a few new sysprefs:
- "ILLDefaultStaffEmail" - Fallback email address for staff ILL notices
to be sent to in the absence of a branch address
- "ILLSendStaffNotices" - To specify which staff notices should be sent
automatically when requests are manipulated by patrons

Patron notices are also controlled by the patron's messaging
preferences

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 54 Martin Renvoize 2020-07-09 07:46:06 UTC
Created attachment 106678 [details] [review]
Bug 22818: Add MessageAttribut{e,es}.pm

We need to be able to access individual message attributes in tests, so
adding accessors for them

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 55 Martin Renvoize 2020-07-09 07:46:10 UTC
Created attachment 106679 [details] [review]
Bug 22818: Unit tests

This patch adds unit tests for the new methods in Illrequest.pm

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 56 Martin Renvoize 2020-07-09 07:46:15 UTC
Created attachment 106680 [details] [review]
Bug 22818: (QA follow-up) Email addressing corrections

This patch updates the patchset to use recnetly introduced methods for
obtaining the correct inbound and reply email addresses for branches.

We also move get_staff_to_address into Koha::Library alongside
inbound_email_address and rename it to inbound_ill_address.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 57 Martin Renvoize 2020-07-09 08:30:56 UTC
Created attachment 106681 [details] [review]
Bug 22818: (QA follow-up) Licence and POD fixes
Comment 58 Martin Renvoize 2020-07-09 08:43:16 UTC
Created attachment 106691 [details] [review]
Bug 22818: (QA follow-up) Template CASE correction
Comment 59 Martin Renvoize 2020-07-09 09:27:34 UTC
Created attachment 106693 [details] [review]
Bug 22818: (QA follow-up) Template CASE correction
Comment 60 Martin Renvoize 2020-07-09 09:27:38 UTC
Created attachment 106694 [details] [review]
Bug 22818: (QA follow-up) Translation fix + Branch notices

This patch removes the hard coded 'N/A' being passed to
GetPreparedLetter and passes in the branchcode of the ILLRequest so
notices can be branch specific.
Comment 61 Martin Renvoize 2020-07-09 11:23:22 UTC
Created attachment 106704 [details] [review]
Bug 22818: Add notices sysprefs and DB column

This patch adds two new sysprefs. 'ILLDefaultEmail', 'ILLSendStaffNotices' and a new column to the
branches table called 'branchillemail'. It also adds five ILL related
notices.

Here we are providing the ability for a branch to have a dedicated email
address for it's ILL staff, the idea being that any notices sent by ILL
to staff need to go to specific staff, rather than the general branch
email address. If no branch specific address is defined, the address
specified in the ILLDefaultEmail syspref is used.

We're also providing a syspref 'ILLSendStaffNotices' to allow the user to specify which
notices should be sent to staff upon certain events.

We are also providing patron messaging preferences for the two patron
bound notices

We are also providing five notices for various ILL related events.

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 62 Martin Renvoize 2020-07-09 11:23:26 UTC
Created attachment 106705 [details] [review]
Bug 22818: Add support for ILL notices

Here we're just adding ILL notices to the existing "letters"
infrastructure so notices can be displayed and edited

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 63 Martin Renvoize 2020-07-09 11:23:30 UTC
Created attachment 106706 [details] [review]
Bug 22818: Add generation and sending of notices

This patch adds the ability for ILL to send notices, both triggered by
staff and triggered by events.

Staff can trigger notices to patrons from the "Manage ILL request" screen:
- ILL request ready for pickup
- ILL request unavailable
- Place request with partners

The following notices to staff are triggered automatically:
- Request has been modified by patron
- Request has been cancelled by patron

Branches can now specify an "ILL email" address to which notices
intended to inform staff of changes to requests by patrons can be sent.

The sending of notices is controlled by a few new sysprefs:
- "ILLDefaultStaffEmail" - Fallback email address for staff ILL notices
to be sent to in the absence of a branch address
- "ILLSendStaffNotices" - To specify which staff notices should be sent
automatically when requests are manipulated by patrons

Patron notices are also controlled by the patron's messaging
preferences

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 64 Martin Renvoize 2020-07-09 11:23:34 UTC
Created attachment 106707 [details] [review]
Bug 22818: Add MessageAttribut{e,es}.pm

We need to be able to access individual message attributes in tests, so
adding accessors for them

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 65 Martin Renvoize 2020-07-09 11:23:38 UTC
Created attachment 106708 [details] [review]
Bug 22818: Unit tests

This patch adds unit tests for the new methods in Illrequest.pm

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 66 Martin Renvoize 2020-07-09 11:23:43 UTC
Created attachment 106709 [details] [review]
Bug 22818: (QA follow-up) Email addressing corrections

This patch updates the patchset to use recnetly introduced methods for
obtaining the correct inbound and reply email addresses for branches.

We also move get_staff_to_address into Koha::Library alongside
inbound_email_address and rename it to inbound_ill_address.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 67 Martin Renvoize 2020-07-09 11:23:48 UTC
Created attachment 106710 [details] [review]
Bug 22818: (QA follow-up) Licence and POD fixes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 68 Martin Renvoize 2020-07-09 11:23:52 UTC
Created attachment 106711 [details] [review]
Bug 22818: (QA follow-up) Template CASE correction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 69 Martin Renvoize 2020-07-09 11:23:57 UTC
Created attachment 106712 [details] [review]
Bug 22818: (QA follow-up) Translation fix + Branch notices

This patch removes the hard coded 'N/A' being passed to
GetPreparedLetter and passes in the branchcode of the ILLRequest so
notices can be branch specific.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 70 Martin Renvoize 2020-07-09 11:24:02 UTC
Created attachment 106713 [details] [review]
Bug 22818: (QA follow-up) Sample Notices

Convert the update notices to TT syntax and add them into the
sample_notices file.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 71 Martin Renvoize 2020-07-09 11:28:16 UTC
(In reply to Katrin Fischer from comment #50)
> I am still working on this, but adding some first notes and questions:
> 
> 1) QA test tools
> 
>  FAIL	Koha/MessageAttributes.pm
>    FAIL	  forbidden patterns
> 		forbidden pattern: Incorrect license statement (using postal address), may
> be a false positive if the file is coming from outside Koha (bug 24545)
> (line 16)
> 
>  FAIL	Koha/MessageAttribute.pm
>    FAIL	  forbidden patterns
> 		forbidden pattern: Incorrect license statement (using postal address), may
> be a false positive if the file is coming from outside Koha (bug 24545)
> (line 16)

Fixed 

> 
> 2) Unit tests
> 
> While tests pass, they produce some non-good looking output:
> 
> kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove
> t/db_dependent/Illrequests.t
> t/db_dependent/Illrequests.t .. 7/12 Use of uninitialized value
> $staff_to_send in pattern match (m//) at
> /home/vagrant/kohaclone/Koha/Illrequest.pm line 1522.
> 	Koha::Illrequest::send_staff_notice(Koha::Illrequest=HASH(0x55b06c17ba40),
> "ILL_REQUEST_CANCEL") called at /home/vagrant/kohaclone/Koha/Illrequest.pm
> line 288
> 	Koha::Illrequest::status(Koha::Illrequest=HASH(0x55b06c17ba40), "CANCREQ")
> called at t/db_dependent/Illrequests.t line 788
> 	main::__ANON__() called at /usr/share/perl5/Test/Builder.pm line 310
> 	eval {...} called at /usr/share/perl5/Test/Builder.pm line 310
> 	Test::Builder::subtest(Test::Builder=HASH(0x55b06bac3bf8), "Helpers",
> CODE(0x55b063112be8)) called at /usr/share/perl5/Test/More.pm line 807
> 	Test::More::subtest("Helpers", CODE(0x55b063112be8)) called at
> t/db_dependent/Illrequests.t line 934
> t/db_dependent/Illrequests.t .. ok     
> All tests successful.
> Files=1, Tests=12, 32 wallclock secs ( 0.09 usr  0.03 sys + 21.40 cusr  6.88
> csys = 28.40 CPU)
> Result: PASS

Fixed

> 3) Code Review
> 
> 3.1) Email addressing
> 
> I feel like here is a general issue here in that we will have problems with
> spam protection if the mail server is not in the same domain as the
> branchillemail given. 
> 
> Is the the new ILL staff email is to be used as the sender or the recipient
> of the ILL emails to staff? It looks like it's actually used as both? As the
> pref only reads "to", maybe we could just keep the current from and it would
> resolve a lot of the possible issues with spam.
> 
> I think we need to at least make use of $library->inbound_email_address in a
> couple of cases replacing the branchemail here:
> 
> +        my $from = $branch->branchillemail || $branch->branchemail;
> 
> get_staff_to_address seems ok in that regard, but send_staff_notice
> from_adress does not.
> Reply-to and From are the spam critical ones.
> 
> Use of Koha::Email is removed?


Fixed

> 3.2) Translatability
> 
> The "N/A" here are problematic. I think it would be better to just create an
> empty string? With TT libraries can still resolve that to another text.
> 
> +        substitute  => {
> +            ill_bib_title      => $title ? $title->value : 'N/A',
> +            ill_bib_author     => $author ? $author->value : 'N/A',

Fixed

> 3.3) Sample letters
> 
> I need to take a closer look for typos and such, but the sample letters need
> to also be added to the new sample .yml and to the translated installers. 

Converted to TT syntax and added to sampl_notices.yml

> 3.4) Sysprefs.sql
> 
> New prefs are in the wrong alphabetic spot :)

FIxed
 
> 3.5) Notices & slips
> 
> The module in the letters summary table is not translatable yet and shows as
> "ill".

Was a copy/paste error.. also fixed
 
> -------------
> 
> Most of those are not bad ones, only 3.1 Email addressing worries me a fair
> bit. Clearing up intentions and intended behaviour would be a good first step
> here.
Comment 72 Katrin Fischer 2020-08-23 21:57:15 UTC
I am sorry, but the tests are not passing, can you please have a look?

Test Summary Report
-------------------
t/db_dependent/Koha/Libraries.t (Wstat: 65280 Tests: 8 Failed: 1)
  Failed test:  8
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 10 tests but ran 8.
t/db_dependent/Illrequests.t   (Wstat: 65280 Tests: 8 Failed: 1)
  Failed test:  8
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 13 tests but ran 8.

Without the patches I still have one failing test - 
not ok 10 - Checking out
Comment 73 Martin Renvoize 2020-09-16 13:43:48 UTC
Created attachment 110196 [details] [review]
Bug 22818: Add notices sysprefs and DB column

This patch adds two new sysprefs. 'ILLDefaultEmail', 'ILLSendStaffNotices' and a new column to the
branches table called 'branchillemail'. It also adds five ILL related
notices.

Here we are providing the ability for a branch to have a dedicated email
address for it's ILL staff, the idea being that any notices sent by ILL
to staff need to go to specific staff, rather than the general branch
email address. If no branch specific address is defined, the address
specified in the ILLDefaultEmail syspref is used.

We're also providing a syspref 'ILLSendStaffNotices' to allow the user to specify which
notices should be sent to staff upon certain events.

We are also providing patron messaging preferences for the two patron
bound notices

We are also providing five notices for various ILL related events.

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 74 Martin Renvoize 2020-09-16 13:43:56 UTC
Created attachment 110197 [details] [review]
Bug 22818: Add support for ILL notices

Here we're just adding ILL notices to the existing "letters"
infrastructure so notices can be displayed and edited

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 75 Martin Renvoize 2020-09-16 13:44:03 UTC
Created attachment 110198 [details] [review]
Bug 22818: Add generation and sending of notices

This patch adds the ability for ILL to send notices, both triggered by
staff and triggered by events.

Staff can trigger notices to patrons from the "Manage ILL request" screen:
- ILL request ready for pickup
- ILL request unavailable
- Place request with partners

The following notices to staff are triggered automatically:
- Request has been modified by patron
- Request has been cancelled by patron

Branches can now specify an "ILL email" address to which notices
intended to inform staff of changes to requests by patrons can be sent.

The sending of notices is controlled by a few new sysprefs:
- "ILLDefaultStaffEmail" - Fallback email address for staff ILL notices
to be sent to in the absence of a branch address
- "ILLSendStaffNotices" - To specify which staff notices should be sent
automatically when requests are manipulated by patrons

Patron notices are also controlled by the patron's messaging
preferences

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 76 Martin Renvoize 2020-09-16 13:44:13 UTC
Created attachment 110199 [details] [review]
Bug 22818: Add MessageAttribut{e,es}.pm

We need to be able to access individual message attributes in tests, so
adding accessors for them

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 77 Martin Renvoize 2020-09-16 13:44:24 UTC
Created attachment 110201 [details] [review]
Bug 22818: Unit tests

This patch adds unit tests for the new methods in Illrequest.pm

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 78 Martin Renvoize 2020-09-16 13:44:32 UTC
Created attachment 110202 [details] [review]
Bug 22818: (QA follow-up) Email addressing corrections

This patch updates the patchset to use recnetly introduced methods for
obtaining the correct inbound and reply email addresses for branches.

We also move get_staff_to_address into Koha::Library alongside
inbound_email_address and rename it to inbound_ill_address.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 79 Martin Renvoize 2020-09-16 13:44:38 UTC
Created attachment 110203 [details] [review]
Bug 22818: (QA follow-up) Licence and POD fixes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 80 Martin Renvoize 2020-09-16 13:44:45 UTC
Created attachment 110204 [details] [review]
Bug 22818: (QA follow-up) Template CASE correction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 81 Martin Renvoize 2020-09-16 13:44:59 UTC
Created attachment 110205 [details] [review]
Bug 22818: (QA follow-up) Translation fix + Branch notices

This patch removes the hard coded 'N/A' being passed to
GetPreparedLetter and passes in the branchcode of the ILLRequest so
notices can be branch specific.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 82 Martin Renvoize 2020-09-16 13:45:06 UTC
Created attachment 110206 [details] [review]
Bug 22818: (QA follow-up) Sample Notices

Convert the update notices to TT syntax and add them into the
sample_notices file.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 83 Martin Renvoize 2020-09-16 13:45:14 UTC
Created attachment 110207 [details] [review]
Bug 22818: (DO NOT PUSH) Schema Updates
Comment 84 Martin Renvoize 2020-09-16 13:46:40 UTC
(In reply to Katrin Fischer from comment #72)
> I am sorry, but the tests are not passing, can you please have a look?
> 
> Test Summary Report
> -------------------
> t/db_dependent/Koha/Libraries.t (Wstat: 65280 Tests: 8 Failed: 1)
>   Failed test:  8
>   Non-zero exit status: 255
>   Parse errors: Bad plan.  You planned 10 tests but ran 8.
> t/db_dependent/Illrequests.t   (Wstat: 65280 Tests: 8 Failed: 1)
>   Failed test:  8
>   Non-zero exit status: 255
>   Parse errors: Bad plan.  You planned 13 tests but ran 8.
> 
> Without the patches I still have one failing test - 
> not ok 10 - Checking out

I've rebased the patchset as it didn't apply on master for me.

I've also added a final 'Schema Updates' patch to simplify testing.
Comment 85 Martin Renvoize 2020-09-16 14:02:43 UTC
Created attachment 110208 [details] [review]
Bug 22818: (QA follow-up) Add mandatory data at install time

This patch adds the data from the update script into the mandatory sql
files such that it is also populated at install time.
Comment 86 Martin Renvoize 2020-09-16 14:04:13 UTC
The failing tests were a sign of missing data at install time.. the final patch should correct that and mean the tests pasts now.
Comment 87 Katrin Fischer 2020-10-14 22:52:33 UTC
I am really sorry, but patches no longer apply again :(

I fixed the first conflict in the messaging templates that was caused by my own change (so easy) but then got stuck on another conflict in IllRequest.pm that is more complicated.

I promise to test this or next week if rebased.

Note: While fixing the conflict in messaging preferences I noticed that the new options will always show, I think we should add a check on ILLModule pref so prevent it showing for libraries that don't use the ILL module.
Comment 88 Andrew Isherwood 2020-10-16 09:54:38 UTC
Created attachment 111837 [details] [review]
Bug 22818: Add notices sysprefs and DB column

This patch adds two new sysprefs. 'ILLDefaultEmail', 'ILLSendStaffNotices' and a new column to the
branches table called 'branchillemail'. It also adds five ILL related
notices.

Here we are providing the ability for a branch to have a dedicated email
address for it's ILL staff, the idea being that any notices sent by ILL
to staff need to go to specific staff, rather than the general branch
email address. If no branch specific address is defined, the address
specified in the ILLDefaultEmail syspref is used.

We're also providing a syspref 'ILLSendStaffNotices' to allow the user to specify which
notices should be sent to staff upon certain events.

We are also providing patron messaging preferences for the two patron
bound notices

We are also providing five notices for various ILL related events.

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 89 Andrew Isherwood 2020-10-16 09:54:45 UTC
Created attachment 111838 [details] [review]
Bug 22818: Add support for ILL notices

Here we're just adding ILL notices to the existing "letters"
infrastructure so notices can be displayed and edited

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 90 Andrew Isherwood 2020-10-16 09:54:50 UTC
Created attachment 111839 [details] [review]
Bug 22818: Add generation and sending of notices

This patch adds the ability for ILL to send notices, both triggered by
staff and triggered by events.

Staff can trigger notices to patrons from the "Manage ILL request" screen:
- ILL request ready for pickup
- ILL request unavailable
- Place request with partners

The following notices to staff are triggered automatically:
- Request has been modified by patron
- Request has been cancelled by patron

Branches can now specify an "ILL email" address to which notices
intended to inform staff of changes to requests by patrons can be sent.

The sending of notices is controlled by a few new sysprefs:
- "ILLDefaultStaffEmail" - Fallback email address for staff ILL notices
to be sent to in the absence of a branch address
- "ILLSendStaffNotices" - To specify which staff notices should be sent
automatically when requests are manipulated by patrons

Patron notices are also controlled by the patron's messaging
preferences

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 91 Andrew Isherwood 2020-10-16 09:54:56 UTC
Created attachment 111840 [details] [review]
Bug 22818: Add MessageAttribut{e,es}.pm

We need to be able to access individual message attributes in tests, so
adding accessors for them

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 92 Andrew Isherwood 2020-10-16 09:55:01 UTC
Created attachment 111841 [details] [review]
Bug 22818: Unit tests

This patch adds unit tests for the new methods in Illrequest.pm

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 93 Andrew Isherwood 2020-10-16 09:55:06 UTC
Created attachment 111842 [details] [review]
Bug 22818: (QA follow-up) Email addressing corrections

This patch updates the patchset to use recnetly introduced methods for
obtaining the correct inbound and reply email addresses for branches.

We also move get_staff_to_address into Koha::Library alongside
inbound_email_address and rename it to inbound_ill_address.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 94 Andrew Isherwood 2020-10-16 09:55:12 UTC
Created attachment 111843 [details] [review]
Bug 22818: (QA follow-up) Licence and POD fixes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 95 Andrew Isherwood 2020-10-16 09:55:17 UTC
Created attachment 111844 [details] [review]
Bug 22818: (QA follow-up) Template CASE correction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 96 Andrew Isherwood 2020-10-16 09:55:22 UTC
Created attachment 111845 [details] [review]
Bug 22818: (QA follow-up) Translation fix + Branch notices

This patch removes the hard coded 'N/A' being passed to
GetPreparedLetter and passes in the branchcode of the ILLRequest so
notices can be branch specific.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 97 Andrew Isherwood 2020-10-16 09:55:30 UTC
Created attachment 111846 [details] [review]
Bug 22818: (QA follow-up) Sample Notices

Convert the update notices to TT syntax and add them into the
sample_notices file.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 98 Andrew Isherwood 2020-10-16 09:55:36 UTC
Created attachment 111847 [details] [review]
Bug 22818: (QA follow-up) Add mandatory data at install time

This patch adds the data from the update script into the mandatory sql
files such that it is also populated at install time.
Comment 99 Andrew Isherwood 2020-10-16 09:55:41 UTC
Created attachment 111848 [details] [review]
Bug 22818: (follow-up) Respond to feedback

This commit fixes this bug, it was broken in a number of ways.

Fixes include:

- Added necessary config block in C4::Letters to enable the TT notice
syntax introduced in an earlier commit to work
- Changed template variables to refer to singular objects rather than
multiple e.g. borrowers -> borrower
- Fixed missing / misnamed variables

This commit also implements the additional syspref checks suggested by
Katrin in comment #87
Comment 100 Andrew Isherwood 2020-10-16 10:02:03 UTC
Hi Katrin - I've now fixed this bug and implemented your syspref suggestion.
Comment 101 Andrew Isherwood 2020-10-16 10:02:24 UTC
Updated test plan:

- Apply patch
- Update database and schema

- Create a report using the following SQL in order to verify that notices are being generated:
SELECT borrowernumber, subject, content, message_transport_type, to_address, from_address FROM message_queue WHERE letter_code LIKE 'ILL%' ORDER BY message_id DESC

- Create one or more "partners". These are patrons that belong to a patron category that has a code that matches the <partner_code> value in your koha-conf.xml (default is ILLLIBS). Patrons in this category must have a primary email defined. Patrons defined in this way are offered as request partners within the ILL interface.

- Go to "Koha administration", search for "ILLModule" syspref, ensure it is set to "Enable"
- Go to "Koha administration", search for "IllLog" syspref, ensure it is set to "Log"

SYSTEM PREFERENCES AND BRANCH CONFIG
- In the staff interface, go to Tools > Notices & Slips
=> TEST: Observe that there are 5 new notices defined for the "Interlibrary loans" module
- Go to "Koha administration" > "Libraries"
- Choose a library and "Edit" it
- Ensure the "Email" field for the library is populated
=> TEST: Observe there is a new "ILL staff email" field
- Enter an email address in the "ILL staff email" field and save the branch
- Go to "Koha administration" > search for the "ILLDefaultStaffEmail" syspref
=> TEST: Observe the new syspref exists
- Enter an email address in this syspref and then save it
- Go to "Koha administration" > search for the "ILLSendStaffNotices" syspref
=> TEST: Observe the new syspref exists
- Enter the names of the two staff notices in this syspref as "ILL_REQUEST_CANCEL|ILL_REQUEST_MODIFIED" and save the syspref

PATRON MESSAGING PREFERENCES
- Go to a patron's messaging preferences
=> TEST: Observe that two new messaging preferences exist, "Interlibrary loan ready" & "Interlibrary loan unavailable"
- Select email and/or SMS for each of these notices

SENDING PATRON NOTICES FROM ILL REQUEST
- In the staff interface, go to "ILL requests"
- Create an ILL request using the FreeForm backend (for the user you just modified the messaging preferences for), select the branch that you previously added the "ILL staff email" address to
- Go to the "Manage ILL request" screen for the newly created request
=> TEST: Observe that there is a new "Send notice to patron" dropdown in the button bar which offers two notices to send
- Select a notice to be sent
=> TEST: Observe that a message reporting the successful queueing of the notice is displayed
- Click on the "ILL request log" for the request
=> TEST: Observe that a log entry "Patron notice sent:" was added to the log
- Run the report created earlier
=> TEST: Observe that a notice was created for all sending methods selected in the patron's messaging preferences
=> TEST: Observe that the borrowernumber column of the notice is populated

SENDING REQUEST TO PARTNERS
- Go to the "Manage ILL request" screen for a request
- Choose "Place request with partners"
=> TEST: Observe that the text defined in the ILL_PARTNER_REQ notice is displayed and can be modified
- Select one or more partners that were defined earlier, then click "Send email"
=> TEST: Observe that the request's status has changed to "Requested from partners"
- Run the report created earlier
=> TEST: Observe that a notice was created
=> TEST: Observe that the to_address column matches the primary email addresses for the selected partners
=> TEST: Observe that the from_address matches the previously defined "ILL staff email" for the request's branch

PATRON GENERATED NOTICES
- As a patron with ILL requests, go to the "your interlibrary loan requests" page in the OPAC
- View a request
- Add some text to the notes field and save
- Run the report created earlier
=> TEST: Observe that a notice was created for the modification
=> TEST: Observe that the to_address matches the previously defined "ILL staff email" for the request's branch
=> TEST: Observe that the from_address matches the previously defined "ILL staff email" for the request's branch
- As a patron with ILL requests, go to the "your interlibrary loan requests" page in the OPAC
- View a request
- Choose "Request cancellation"
- Run the report created earlier
=> TEST: Observe that a notice was created for the cancellation
=> TEST: Observe that the to_address matches the previously defined "ILL staff email" for the request's branch
=> TEST: Observe that the from_address matches the previously defined "ILL staff email" for the request's branch

There is a fallback hierarchy for the address to which staff notices are sent:

Branch ILL staff email > ILLDefaultStaffEmail > General branch email > Koha admin email

Please test that this hierarchy is adhered to by undefining one or more of these options.
Comment 102 Katrin Fischer 2020-10-16 10:40:49 UTC
Starting work here now - fingers crossed! Patches apply and QA tests pass, so far so good.
Comment 103 Katrin Fischer 2020-10-16 11:29:49 UTC
I think I spotted a rebase issue ( :( ) in Illrequest.pm:

In the last iteration pushed by Martin we had:
-        my $from = $branch->branchillemail || $branch->branchemail;
-        my $replyto = $branch->branchreplyto || $from;
+        my $from = $branch->branchemail;
+        my $replyto = $branch->inbound_ill_address;

But now it reads:
-        my $from = $library->branchemail;
-        my $reply_to = $library->branchreplyto || $from;
+        my $from = $branch->branchillemail || $branch->branchemail;
+        my $replyto = $branch->branchreplyto || $from;


I feel the first one is correct as it will work for libraries using a mail server outside their domain and thus allow wider use of the feature. It looks like the very last patch changed it back.
Comment 104 Katrin Fischer 2020-10-16 11:30:22 UTC
(In reply to Katrin Fischer from comment #103)
> I think I spotted a rebase issue ( :( ) in Illrequest.pm:

I'll keep on working at this, if you agree I can provide a little follow-up myself.
Comment 105 Katrin Fischer 2020-10-16 12:00:42 UTC
Sadly the ILLModule check on the system preference doesn't work the intended way. It only removes the labels, the ILL notices still show up as "Unknown". I think a  slightly different approach is needed. Not sure I see how to fix that myself right now, so putting it here while I continue testing.
Comment 106 Katrin Fischer 2020-10-16 12:09:30 UTC
Ok, still testing! But it gets a little too much for me to fix myself, so I'll need follow-ups for:

1) Email issue noted in comment#103

2) Messaging preferences: Hide ILL notices if ILLModule is turned off. As this is visible to patrons in the OPAC I think we should really have it and the current approach breaks the table labels.

3) Log viewer: There is a new log action PATRON_NOTICE for ILL, that needs to show up on the log viewer search interface as a new checkbox (easy one :) )

4) Translated installers. You can leave out de-DE (because of bug 24972), but we will need the others fixed. There is a test to help with that: 
prove xt/sample_notices.t
Comment 107 Katrin Fischer 2020-10-16 13:24:21 UTC
5) Metadata missing from text to partners

Not sure if this should be a separate bug:
- Enter a new ILL request, using Freeform
- Type: Book, Title: something random
- Edit request
- Place with partner libraries
- The title information is not showing up
- Edit metadata, add year - same issue. 
- Add author - now it works and the notice shows the information.

Note: the metadata field appear unordered, maybe something for later refinement in the TT notice.

6) Buttons (Bootstrap 4 issue)

In the OPAC the "Submit modifications" button has bad contrast being dark grey with black writing. I think this could be a separate bug and is surely an issue with the Bootstrap 4 update as we had some other button bugs already.

Everything else looks good!

Please provide fixes as follow-ups - that will help to get them tested easier. I think all points found are not too severe and hope we can get that over finish line this time!
Comment 108 Katrin Fischer 2020-10-16 13:30:45 UTC
Created attachment 111874 [details]
ILL view in OPAC - button
Comment 109 Katrin Fischer 2020-10-16 13:41:15 UTC
(In reply to Katrin Fischer from comment #108)
> Created attachment 111874 [details]
> ILL view in OPAC - button

For 6) Checked with Owen:

Since it's a submit button I'd change "btn-default" to "btn-primary," but it should be considered a general bug that input:submit.btn-default has black text.

So we should change it in this template, but I also filed bug 26706. 

I'd be open to fix it here, but we can also move it to a separate bug as it's not strictly related.
Comment 110 Andrew Isherwood 2020-10-16 14:34:59 UTC
Created attachment 111880 [details] [review]
Bug 22818: Add notices sysprefs and DB column

This patch adds two new sysprefs. 'ILLDefaultEmail', 'ILLSendStaffNotices' and a new column to the
branches table called 'branchillemail'. It also adds five ILL related
notices.

Here we are providing the ability for a branch to have a dedicated email
address for it's ILL staff, the idea being that any notices sent by ILL
to staff need to go to specific staff, rather than the general branch
email address. If no branch specific address is defined, the address
specified in the ILLDefaultEmail syspref is used.

We're also providing a syspref 'ILLSendStaffNotices' to allow the user to specify which
notices should be sent to staff upon certain events.

We are also providing patron messaging preferences for the two patron
bound notices

We are also providing five notices for various ILL related events.

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 111 Andrew Isherwood 2020-10-16 14:35:13 UTC
Created attachment 111881 [details] [review]
Bug 22818: Add support for ILL notices

Here we're just adding ILL notices to the existing "letters"
infrastructure so notices can be displayed and edited

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 112 Andrew Isherwood 2020-10-16 14:35:27 UTC
Created attachment 111883 [details] [review]
Bug 22818: Add generation and sending of notices

This patch adds the ability for ILL to send notices, both triggered by
staff and triggered by events.

Staff can trigger notices to patrons from the "Manage ILL request" screen:
- ILL request ready for pickup
- ILL request unavailable
- Place request with partners

The following notices to staff are triggered automatically:
- Request has been modified by patron
- Request has been cancelled by patron

Branches can now specify an "ILL email" address to which notices
intended to inform staff of changes to requests by patrons can be sent.

The sending of notices is controlled by a few new sysprefs:
- "ILLDefaultStaffEmail" - Fallback email address for staff ILL notices
to be sent to in the absence of a branch address
- "ILLSendStaffNotices" - To specify which staff notices should be sent
automatically when requests are manipulated by patrons

Patron notices are also controlled by the patron's messaging
preferences

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 113 Andrew Isherwood 2020-10-16 14:35:37 UTC
Created attachment 111884 [details] [review]
Bug 22818: Add MessageAttribut{e,es}.pm

We need to be able to access individual message attributes in tests, so
adding accessors for them

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 114 Andrew Isherwood 2020-10-16 14:35:50 UTC
Created attachment 111885 [details] [review]
Bug 22818: Unit tests

This patch adds unit tests for the new methods in Illrequest.pm

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 115 Andrew Isherwood 2020-10-16 14:36:00 UTC
Created attachment 111886 [details] [review]
Bug 22818: (QA follow-up) Email addressing corrections

This patch updates the patchset to use recnetly introduced methods for
obtaining the correct inbound and reply email addresses for branches.

We also move get_staff_to_address into Koha::Library alongside
inbound_email_address and rename it to inbound_ill_address.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 116 Andrew Isherwood 2020-10-16 14:36:07 UTC
Created attachment 111887 [details] [review]
Bug 22818: (QA follow-up) Licence and POD fixes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 117 Andrew Isherwood 2020-10-16 14:36:14 UTC
Created attachment 111888 [details] [review]
Bug 22818: (QA follow-up) Template CASE correction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 118 Andrew Isherwood 2020-10-16 14:36:20 UTC
Created attachment 111889 [details] [review]
Bug 22818: (QA follow-up) Translation fix + Branch notices

This patch removes the hard coded 'N/A' being passed to
GetPreparedLetter and passes in the branchcode of the ILLRequest so
notices can be branch specific.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 119 Andrew Isherwood 2020-10-16 14:36:27 UTC
Created attachment 111890 [details] [review]
Bug 22818: (QA follow-up) Sample Notices

Convert the update notices to TT syntax and add them into the
sample_notices file.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 120 Andrew Isherwood 2020-10-16 14:36:35 UTC
Created attachment 111891 [details] [review]
Bug 22818: (QA follow-up) Add mandatory data at install time

This patch adds the data from the update script into the mandatory sql
files such that it is also populated at install time.
Comment 121 Andrew Isherwood 2020-10-16 14:36:43 UTC
Created attachment 111892 [details] [review]
Bug 22818: (follow-up) Respond to feedback

This commit fixes this bug, it was broken in a number of ways.

Fixes include:

- Added necessary config block in C4::Letters to enable the TT notice
syntax introduced in an earlier commit to work
- Changed template variables to refer to singular objects rather than
multiple e.g. borrowers -> borrower
- Fixed missing / misnamed variables

This commit also implements the additional syspref checks suggested by
Katrin in comment #87
Comment 122 Andrew Isherwood 2020-10-16 14:36:51 UTC
Created attachment 111893 [details] [review]
Bug 22818: (follow-up) Respond to feedback

This commit implements the feedback from Katrin in comment #103 ->
comment #108:

- Fixed rebase issue mentioned in comment #103
- Fixed ILL notice display mentioned in comment #106
- Fixed bug with metadata display, though not the issue that was
mentioned in comment #107
- Fixed buttons display mentioned in comment #107, in addition to some
more buttons suffering the same issue
Comment 123 Andrew Isherwood 2020-10-16 14:37:12 UTC
Hi Katrin - Thanks for that. I've now addressed everything you've mentioned apart from the metadata not appearing with just a title, this seemed to work fine for me. There *was* a bug with metadata display, which I've now fixed, but I don't think it would have caused what you were seeing. Try these latest set of patches and see if you still have the problem.
Comment 124 Katrin Fischer 2020-10-16 14:44:02 UTC
Back here :)
Comment 125 Katrin Fischer 2020-10-16 15:04:08 UTC
Hi Andrew,

5) is fixed for me now. 

All others look good too. I am working on some tiny follow-ups. Ready soon.
Comment 126 Katrin Fischer 2020-10-16 15:20:33 UTC
Created attachment 111897 [details] [review]
Bug 22818: Add notices sysprefs and DB column

This patch adds two new sysprefs. 'ILLDefaultEmail', 'ILLSendStaffNotices' and a new column to the
branches table called 'branchillemail'. It also adds five ILL related
notices.

Here we are providing the ability for a branch to have a dedicated email
address for it's ILL staff, the idea being that any notices sent by ILL
to staff need to go to specific staff, rather than the general branch
email address. If no branch specific address is defined, the address
specified in the ILLDefaultEmail syspref is used.

We're also providing a syspref 'ILLSendStaffNotices' to allow the user to specify which
notices should be sent to staff upon certain events.

We are also providing patron messaging preferences for the two patron
bound notices

We are also providing five notices for various ILL related events.

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 127 Katrin Fischer 2020-10-16 15:20:44 UTC
Created attachment 111899 [details] [review]
Bug 22818: Add support for ILL notices

Here we're just adding ILL notices to the existing "letters"
infrastructure so notices can be displayed and edited

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 128 Katrin Fischer 2020-10-16 15:20:53 UTC
Created attachment 111900 [details] [review]
Bug 22818: Add generation and sending of notices

This patch adds the ability for ILL to send notices, both triggered by
staff and triggered by events.

Staff can trigger notices to patrons from the "Manage ILL request" screen:
- ILL request ready for pickup
- ILL request unavailable
- Place request with partners

The following notices to staff are triggered automatically:
- Request has been modified by patron
- Request has been cancelled by patron

Branches can now specify an "ILL email" address to which notices
intended to inform staff of changes to requests by patrons can be sent.

The sending of notices is controlled by a few new sysprefs:
- "ILLDefaultStaffEmail" - Fallback email address for staff ILL notices
to be sent to in the absence of a branch address
- "ILLSendStaffNotices" - To specify which staff notices should be sent
automatically when requests are manipulated by patrons

Patron notices are also controlled by the patron's messaging
preferences

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 129 Katrin Fischer 2020-10-16 15:21:00 UTC
Created attachment 111901 [details] [review]
Bug 22818: Add MessageAttribut{e,es}.pm

We need to be able to access individual message attributes in tests, so
adding accessors for them

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 130 Katrin Fischer 2020-10-16 15:21:08 UTC
Created attachment 111902 [details] [review]
Bug 22818: Unit tests

This patch adds unit tests for the new methods in Illrequest.pm

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 131 Katrin Fischer 2020-10-16 15:21:18 UTC
Created attachment 111903 [details] [review]
Bug 22818: (QA follow-up) Email addressing corrections

This patch updates the patchset to use recnetly introduced methods for
obtaining the correct inbound and reply email addresses for branches.

We also move get_staff_to_address into Koha::Library alongside
inbound_email_address and rename it to inbound_ill_address.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 132 Katrin Fischer 2020-10-16 15:21:35 UTC
Created attachment 111904 [details] [review]
Bug 22818: (QA follow-up) Licence and POD fixes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 133 Katrin Fischer 2020-10-16 15:21:43 UTC
Created attachment 111905 [details] [review]
Bug 22818: (QA follow-up) Template CASE correction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 134 Katrin Fischer 2020-10-16 15:21:50 UTC
Created attachment 111906 [details] [review]
Bug 22818: (QA follow-up) Translation fix + Branch notices

This patch removes the hard coded 'N/A' being passed to
GetPreparedLetter and passes in the branchcode of the ILLRequest so
notices can be branch specific.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 135 Katrin Fischer 2020-10-16 15:21:58 UTC
Created attachment 111907 [details] [review]
Bug 22818: (QA follow-up) Sample Notices

Convert the update notices to TT syntax and add them into the
sample_notices file.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 136 Katrin Fischer 2020-10-16 15:22:05 UTC
Created attachment 111908 [details] [review]
Bug 22818: (QA follow-up) Add mandatory data at install time

This patch adds the data from the update script into the mandatory sql
files such that it is also populated at install time.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 137 Katrin Fischer 2020-10-16 15:22:12 UTC
Created attachment 111909 [details] [review]
Bug 22818: (follow-up) Respond to feedback

This commit fixes this bug, it was broken in a number of ways.

Fixes include:

- Added necessary config block in C4::Letters to enable the TT notice
syntax introduced in an earlier commit to work
- Changed template variables to refer to singular objects rather than
multiple e.g. borrowers -> borrower
- Fixed missing / misnamed variables

This commit also implements the additional syspref checks suggested by
Katrin in comment #87

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 138 Katrin Fischer 2020-10-16 15:22:19 UTC
Created attachment 111910 [details] [review]
Bug 22818: (follow-up) Respond to feedback

This commit implements the feedback from Katrin in comment #103 ->
comment #108:

- Fixed rebase issue mentioned in comment #103
- Fixed ILL notice display mentioned in comment #106
- Fixed bug with metadata display, though not the issue that was
mentioned in comment #107
- Fixed buttons display mentioned in comment #107, in addition to some
more buttons suffering the same issue

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 139 Katrin Fischer 2020-10-16 15:22:27 UTC
Created attachment 111911 [details] [review]
Bug 22818: (QA follow-up) Add sample ILL notices to it-IT installer

Run: prove xt/sample_notices.t

de-DE are missing on purpose because of bug 24972.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 140 Katrin Fischer 2020-10-16 15:22:33 UTC
Created attachment 111912 [details] [review]
Bug 22818: (QA follow-up) Add some punctuation and terminology fixes to pref descriptions

Just branch => library and some ending punctuation.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 141 Katrin Fischer 2020-10-16 15:22:41 UTC
Created attachment 111913 [details] [review]
Bug 22818: (QA follow-up) Add hint to new library ILL email setting

Copies the hints on the other email fields highlighting the existing
global pref.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 142 Katrin Fischer 2020-10-16 15:24:32 UTC
Thx for the quick work and great test plan, Andrew. I threw in 3 tiny follow-ups at the end - hope they are agreeable :)
Comment 143 Andrew Isherwood 2020-10-16 15:32:49 UTC
Hi Katrin

Thanks for the follow ups, they look great. Sorry for missing the Italian file! Thanks for all your hard work on this, much appreciated!
Comment 144 Jonathan Druart 2020-11-09 13:08:25 UTC
Andrew, I resolved a first conflict on sample_notices_message_transports.sql but I cannot apply the second "Bug 22818: (follow-up) Respond to feedback" patch

error: sha1 information is lacking or useless (installer/data/mysql/en/mandatory/sample_notices.yml).
Comment 145 Andrew Isherwood 2020-11-10 14:17:49 UTC
Created attachment 113398 [details] [review]
Bug 22818: Add notices sysprefs and DB column

This patch adds two new sysprefs. 'ILLDefaultEmail', 'ILLSendStaffNotices' and a new column to the
branches table called 'branchillemail'. It also adds five ILL related
notices.

Here we are providing the ability for a branch to have a dedicated email
address for it's ILL staff, the idea being that any notices sent by ILL
to staff need to go to specific staff, rather than the general branch
email address. If no branch specific address is defined, the address
specified in the ILLDefaultEmail syspref is used.

We're also providing a syspref 'ILLSendStaffNotices' to allow the user to specify which
notices should be sent to staff upon certain events.

We are also providing patron messaging preferences for the two patron
bound notices

We are also providing five notices for various ILL related events.

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 146 Andrew Isherwood 2020-11-10 14:17:54 UTC
Created attachment 113399 [details] [review]
Bug 22818: Add support for ILL notices

Here we're just adding ILL notices to the existing "letters"
infrastructure so notices can be displayed and edited

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 147 Andrew Isherwood 2020-11-10 14:17:59 UTC
Created attachment 113400 [details] [review]
Bug 22818: Add generation and sending of notices

This patch adds the ability for ILL to send notices, both triggered by
staff and triggered by events.

Staff can trigger notices to patrons from the "Manage ILL request" screen:
- ILL request ready for pickup
- ILL request unavailable
- Place request with partners

The following notices to staff are triggered automatically:
- Request has been modified by patron
- Request has been cancelled by patron

Branches can now specify an "ILL email" address to which notices
intended to inform staff of changes to requests by patrons can be sent.

The sending of notices is controlled by a few new sysprefs:
- "ILLDefaultStaffEmail" - Fallback email address for staff ILL notices
to be sent to in the absence of a branch address
- "ILLSendStaffNotices" - To specify which staff notices should be sent
automatically when requests are manipulated by patrons

Patron notices are also controlled by the patron's messaging
preferences

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 148 Andrew Isherwood 2020-11-10 14:18:03 UTC
Created attachment 113401 [details] [review]
Bug 22818: Add MessageAttribut{e,es}.pm

We need to be able to access individual message attributes in tests, so
adding accessors for them

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 149 Andrew Isherwood 2020-11-10 14:18:08 UTC
Created attachment 113402 [details] [review]
Bug 22818: Unit tests

This patch adds unit tests for the new methods in Illrequest.pm

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 150 Andrew Isherwood 2020-11-10 14:18:13 UTC
Created attachment 113403 [details] [review]
Bug 22818: (QA follow-up) Email addressing corrections

This patch updates the patchset to use recnetly introduced methods for
obtaining the correct inbound and reply email addresses for branches.

We also move get_staff_to_address into Koha::Library alongside
inbound_email_address and rename it to inbound_ill_address.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 151 Andrew Isherwood 2020-11-10 14:18:18 UTC
Created attachment 113404 [details] [review]
Bug 22818: (QA follow-up) Licence and POD fixes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 152 Andrew Isherwood 2020-11-10 14:18:23 UTC
Created attachment 113405 [details] [review]
Bug 22818: (QA follow-up) Template CASE correction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 153 Andrew Isherwood 2020-11-10 14:18:28 UTC
Created attachment 113406 [details] [review]
Bug 22818: (QA follow-up) Translation fix + Branch notices

This patch removes the hard coded 'N/A' being passed to
GetPreparedLetter and passes in the branchcode of the ILLRequest so
notices can be branch specific.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 154 Andrew Isherwood 2020-11-10 14:18:32 UTC
Created attachment 113407 [details] [review]
Bug 22818: (QA follow-up) Sample Notices

Convert the update notices to TT syntax and add them into the
sample_notices file.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 155 Andrew Isherwood 2020-11-10 14:18:37 UTC
Created attachment 113408 [details] [review]
Bug 22818: (QA follow-up) Add mandatory data at install time

This patch adds the data from the update script into the mandatory sql
files such that it is also populated at install time.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 156 Andrew Isherwood 2020-11-10 14:18:42 UTC
Created attachment 113409 [details] [review]
Bug 22818: (follow-up) Respond to feedback

This commit fixes this bug, it was broken in a number of ways.

Fixes include:

- Added necessary config block in C4::Letters to enable the TT notice
syntax introduced in an earlier commit to work
- Changed template variables to refer to singular objects rather than
multiple e.g. borrowers -> borrower
- Fixed missing / misnamed variables

This commit also implements the additional syspref checks suggested by
Katrin in comment #87

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 157 Andrew Isherwood 2020-11-10 14:18:47 UTC
Created attachment 113410 [details] [review]
Bug 22818: (follow-up) Respond to feedback

This commit implements the feedback from Katrin in comment #103 ->
comment #108:

- Fixed rebase issue mentioned in comment #103
- Fixed ILL notice display mentioned in comment #106
- Fixed bug with metadata display, though not the issue that was
mentioned in comment #107
- Fixed buttons display mentioned in comment #107, in addition to some
more buttons suffering the same issue

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 158 Andrew Isherwood 2020-11-10 14:18:51 UTC
Created attachment 113411 [details] [review]
Bug 22818: (QA follow-up) Add sample ILL notices to it-IT installer

Run: prove xt/sample_notices.t

de-DE are missing on purpose because of bug 24972.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 159 Andrew Isherwood 2020-11-10 14:18:57 UTC
Created attachment 113412 [details] [review]
Bug 22818: (QA follow-up) Add some punctuation and terminology fixes to pref descriptions

Just branch => library and some ending punctuation.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 160 Andrew Isherwood 2020-11-10 14:19:01 UTC
Created attachment 113413 [details] [review]
Bug 22818: (QA follow-up) Add hint to new library ILL email setting

Copies the hints on the other email fields highlighting the existing
global pref.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 161 Andrew Isherwood 2020-11-10 14:19:23 UTC
Hi Jonathan

I experienced the conflicts you did, but didn't have any problem with the second "Respond to feedback" patch:

Applying: Bug 22818: (follow-up) Respond to feedback
Using index info to reconstruct a base tree...
M       koha-tmpl/intranet-tmpl/prog/en/includes/messaging-preference-form.inc
M       koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt
M       koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt
M       koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-messaging.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-messaging.tt
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/messaging-preference-form.inc

Anyway, I've just attached a new set of patches with the conflicts resolved, try again and see how it goes
Comment 162 Jonathan Druart 2020-11-10 15:21:13 UTC
Only found minor things so far.

1. 

Those changes are not expected:

-    my $library = Koha::Libraries->find($params->{current_branchcode})
+    my $branch = Koha::Libraries->find($params->{current_branchcode})

2. PATRON_NOTICE is (so far) only for ILL notices, should not we make it explicit on the interface to not cause confusion?

3. 
+use Koha::MessageAttributes;
+use Koha::MessageAttribute;

You only need to include the Koha::Object*s* class.
Comment 163 Andrew Isherwood 2020-11-11 10:59:28 UTC
Created attachment 113463 [details] [review]
Bug 22818: Add notices sysprefs and DB column

This patch adds two new sysprefs. 'ILLDefaultEmail', 'ILLSendStaffNotices' and a new column to the
branches table called 'branchillemail'. It also adds five ILL related
notices.

Here we are providing the ability for a branch to have a dedicated email
address for it's ILL staff, the idea being that any notices sent by ILL
to staff need to go to specific staff, rather than the general branch
email address. If no branch specific address is defined, the address
specified in the ILLDefaultEmail syspref is used.

We're also providing a syspref 'ILLSendStaffNotices' to allow the user to specify which
notices should be sent to staff upon certain events.

We are also providing patron messaging preferences for the two patron
bound notices

We are also providing five notices for various ILL related events.

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 164 Andrew Isherwood 2020-11-11 10:59:38 UTC
Created attachment 113464 [details] [review]
Bug 22818: Add support for ILL notices

Here we're just adding ILL notices to the existing "letters"
infrastructure so notices can be displayed and edited

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 165 Andrew Isherwood 2020-11-11 10:59:43 UTC
Created attachment 113465 [details] [review]
Bug 22818: Add generation and sending of notices

This patch adds the ability for ILL to send notices, both triggered by
staff and triggered by events.

Staff can trigger notices to patrons from the "Manage ILL request" screen:
- ILL request ready for pickup
- ILL request unavailable
- Place request with partners

The following notices to staff are triggered automatically:
- Request has been modified by patron
- Request has been cancelled by patron

Branches can now specify an "ILL email" address to which notices
intended to inform staff of changes to requests by patrons can be sent.

The sending of notices is controlled by a few new sysprefs:
- "ILLDefaultStaffEmail" - Fallback email address for staff ILL notices
to be sent to in the absence of a branch address
- "ILLSendStaffNotices" - To specify which staff notices should be sent
automatically when requests are manipulated by patrons

Patron notices are also controlled by the patron's messaging
preferences

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 166 Andrew Isherwood 2020-11-11 10:59:48 UTC
Created attachment 113466 [details] [review]
Bug 22818: Add MessageAttribut{e,es}.pm

We need to be able to access individual message attributes in tests, so
adding accessors for them

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 167 Andrew Isherwood 2020-11-11 10:59:52 UTC
Created attachment 113467 [details] [review]
Bug 22818: Unit tests

This patch adds unit tests for the new methods in Illrequest.pm

Sponsored-by: PTFS Europe
Signed-off-by: Niamh Walker-Headon <Niamh.Walker-Headon@it-tallaght.ie>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 168 Andrew Isherwood 2020-11-11 10:59:57 UTC
Created attachment 113468 [details] [review]
Bug 22818: (QA follow-up) Email addressing corrections

This patch updates the patchset to use recnetly introduced methods for
obtaining the correct inbound and reply email addresses for branches.

We also move get_staff_to_address into Koha::Library alongside
inbound_email_address and rename it to inbound_ill_address.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 169 Andrew Isherwood 2020-11-11 11:00:02 UTC
Created attachment 113469 [details] [review]
Bug 22818: (QA follow-up) Licence and POD fixes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 170 Andrew Isherwood 2020-11-11 11:00:08 UTC
Created attachment 113470 [details] [review]
Bug 22818: (QA follow-up) Template CASE correction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 171 Andrew Isherwood 2020-11-11 11:00:14 UTC
Created attachment 113471 [details] [review]
Bug 22818: (QA follow-up) Translation fix + Branch notices

This patch removes the hard coded 'N/A' being passed to
GetPreparedLetter and passes in the branchcode of the ILLRequest so
notices can be branch specific.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 172 Andrew Isherwood 2020-11-11 11:00:18 UTC
Created attachment 113472 [details] [review]
Bug 22818: (QA follow-up) Sample Notices

Convert the update notices to TT syntax and add them into the
sample_notices file.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 173 Andrew Isherwood 2020-11-11 11:00:23 UTC
Created attachment 113473 [details] [review]
Bug 22818: (QA follow-up) Add mandatory data at install time

This patch adds the data from the update script into the mandatory sql
files such that it is also populated at install time.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 174 Andrew Isherwood 2020-11-11 11:00:28 UTC
Created attachment 113474 [details] [review]
Bug 22818: (follow-up) Respond to feedback

This commit fixes this bug, it was broken in a number of ways.

Fixes include:

- Added necessary config block in C4::Letters to enable the TT notice
syntax introduced in an earlier commit to work
- Changed template variables to refer to singular objects rather than
multiple e.g. borrowers -> borrower
- Fixed missing / misnamed variables

This commit also implements the additional syspref checks suggested by
Katrin in comment #87

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 175 Andrew Isherwood 2020-11-11 11:00:36 UTC
Created attachment 113475 [details] [review]
Bug 22818: (follow-up) Respond to feedback

This commit implements the feedback from Katrin in comment #103 ->
comment #108:

- Fixed rebase issue mentioned in comment #103
- Fixed ILL notice display mentioned in comment #106
- Fixed bug with metadata display, though not the issue that was
mentioned in comment #107
- Fixed buttons display mentioned in comment #107, in addition to some
more buttons suffering the same issue

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 176 Andrew Isherwood 2020-11-11 11:00:41 UTC
Created attachment 113476 [details] [review]
Bug 22818: (QA follow-up) Add sample ILL notices to it-IT installer

Run: prove xt/sample_notices.t

de-DE are missing on purpose because of bug 24972.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 177 Andrew Isherwood 2020-11-11 11:00:47 UTC
Created attachment 113477 [details] [review]
Bug 22818: (QA follow-up) Add some punctuation and terminology fixes to pref descriptions

Just branch => library and some ending punctuation.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 178 Andrew Isherwood 2020-11-11 11:00:51 UTC
Created attachment 113478 [details] [review]
Bug 22818: (QA follow-up) Add hint to new library ILL email setting

Copies the hints on the other email fields highlighting the existing
global pref.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 179 Andrew Isherwood 2020-11-11 11:00:56 UTC
Created attachment 113479 [details] [review]
Bug 22818: (follow-up) "use" correct class

Following up on the feedback from Jonathan in comment #162, this commit
removes the use of Koha::MessageAttribute
Comment 180 Andrew Isherwood 2020-11-11 11:01:12 UTC
(In reply to Jonathan Druart from comment #162)
> Those changes are not expected:
> 
> -    my $library = Koha::Libraries->find($params->{current_branchcode})
> +    my $branch = Koha::Libraries->find($params->{current_branchcode})

I'm afraid I don't understand. In this patch https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113409&action=diff I fixed a misnamed variable, from $library -> $branch, which is the change you've highlighted above. Why is this unexpected?

> 2. PATRON_NOTICE is (so far) only for ILL notices, should not we make it
> explicit on the interface to not cause confusion?

Sorry, I don't understand, make it explicit where in the interface? It's listed, and can be selected, along with all the other log actions in the "Browse the system logs" page. Is this where you are talking about?


> 3. 
> +use Koha::MessageAttributes;
> +use Koha::MessageAttribute;
> 
> You only need to include the Koha::Object*s* class.

Good spot, thanks, now fixed.
Comment 181 Katrin Fischer 2020-11-11 11:11:02 UTC
(In reply to Andrew Isherwood from comment #180)
> (In reply to Jonathan Druart from comment #162)
> > Those changes are not expected:
> > 
> > -    my $library = Koha::Libraries->find($params->{current_branchcode})
> > +    my $branch = Koha::Libraries->find($params->{current_branchcode})
> 
> I'm afraid I don't understand. In this patch
> https://bugs.koha-community.org/bugzilla3/attachment.
> cgi?id=113409&action=diff I fixed a misnamed variable, from $library ->
> $branch, which is the change you've highlighted above. Why is this
> unexpected?
> 
> > 2. PATRON_NOTICE is (so far) only for ILL notices, should not we make it
> > explicit on the interface to not cause confusion?
> 
> Sorry, I don't understand, make it explicit where in the interface? It's
> listed, and can be selected, along with all the other log actions in the
> "Browse the system logs" page. Is this where you are talking about?

I am not sure either, but wondering if it was meant to include ILL in the notice code to make it more obvious what it is used for. I didn't consider that in testing, as there is a new notice module for "Interlibrary loans" too for grouping them.
Comment 182 Jonathan Druart 2020-11-11 11:23:55 UTC
(In reply to Andrew Isherwood from comment #180)
> (In reply to Jonathan Druart from comment #162)
> > Those changes are not expected:
> > 
> > -    my $library = Koha::Libraries->find($params->{current_branchcode})
> > +    my $branch = Koha::Libraries->find($params->{current_branchcode})
> 
> I'm afraid I don't understand. In this patch
> https://bugs.koha-community.org/bugzilla3/attachment.
> cgi?id=113409&action=diff I fixed a misnamed variable, from $library ->
> $branch, which is the change you've highlighted above. Why is this
> unexpected?

We prefer "library" over "branch"
https://wiki.koha-community.org/wiki/Terminology

> > 2. PATRON_NOTICE is (so far) only for ILL notices, should not we make it
> > explicit on the interface to not cause confusion?
> 
> Sorry, I don't understand, make it explicit where in the interface? It's
> listed, and can be selected, along with all the other log actions in the
> "Browse the system logs" page. Is this where you are talking about?

On the log viewer, there is a checkbox "Send a notice to a patron" with a code "PATRON_NOTICE". Which implies you will see all the notices sent. But actually we only support 2 notices, the ones you have created.
I think this should cover all notices and be moved to a separate bug report.
Or we improve the wording...
Comment 183 Tomás Cohen Arazi 2020-11-11 11:36:57 UTC
Hi, I suggest the new column for branches is called 'ill_email', and please add it to the api/v1/definitions/library.json object definition.
If you prefer to keep the current name for simplicity, please add a mapping in the to_api_mapping sub.
Comment 184 Katrin Fischer 2020-11-11 11:42:57 UTC
> We prefer "library" over "branch"
> https://wiki.koha-community.org/wiki/Terminology
> 
> > > 2. PATRON_NOTICE is (so far) only for ILL notices, should not we make it
> > > explicit on the interface to not cause confusion?
> > 
> > Sorry, I don't understand, make it explicit where in the interface? It's
> > listed, and can be selected, along with all the other log actions in the
> > "Browse the system logs" page. Is this where you are talking about?
> 
> On the log viewer, there is a checkbox "Send a notice to a patron" with a
> code "PATRON_NOTICE". Which implies you will see all the notices sent. But
> actually we only support 2 notices, the ones you have created.
> I think this should cover all notices and be moved to a separate bug report.
> Or we improve the wording...

Ah! Suggestions: We change the wording on the log viewer to read: "ILL notice sent to patron" and create a new bug report for widening the support of this?
Comment 185 Tomás Cohen Arazi 2020-11-11 11:50:05 UTC
(In reply to Katrin Fischer from comment #184)
> > We prefer "library" over "branch"
> > https://wiki.koha-community.org/wiki/Terminology
> > 
> > > > 2. PATRON_NOTICE is (so far) only for ILL notices, should not we make it
> > > > explicit on the interface to not cause confusion?
> > > 
> > > Sorry, I don't understand, make it explicit where in the interface? It's
> > > listed, and can be selected, along with all the other log actions in the
> > > "Browse the system logs" page. Is this where you are talking about?
> > 
> > On the log viewer, there is a checkbox "Send a notice to a patron" with a
> > code "PATRON_NOTICE". Which implies you will see all the notices sent. But
> > actually we only support 2 notices, the ones you have created.
> > I think this should cover all notices and be moved to a separate bug report.
> > Or we improve the wording...
> 
> Ah! Suggestions: We change the wording on the log viewer to read: "ILL
> notice sent to patron" and create a new bug report for widening the support
> of this?

I understood the same. Do we agree the notices codes could refer to ILL more explicitly as well?
Comment 186 Katrin Fischer 2020-11-11 11:52:37 UTC
> > 
> > Ah! Suggestions: We change the wording on the log viewer to read: "ILL
> > notice sent to patron" and create a new bug report for widening the support
> > of this?
> 
> I understood the same. Do we agree the notices codes could refer to ILL more
> explicitly as well?

No! Sorry, that was my fault. They all include ILL in their codes already nicely.
Comment 187 Jonathan Druart 2020-11-11 15:09:09 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 188 Jonathan Druart 2020-11-11 15:11:24 UTC
Pushed but more work needed:

1. Fix the checkbox wording

2. $dbh->{mysql_insertid}; in the upgrade process should be replaced. It's a MySQLism

3. t/db_dependent/Illrequests.t is passing but, only when run once:

$ prove t/db_dependent/Illrequests.t
OK

$ prove t/db_dependent/Circulation.t t/db_dependent/Circulation/issue.t t/db_dependent/Illrequests.t 

t/db_dependent/Circulation.t ................... ok   
t/db_dependent/Circulation/issue.t ............. ok   
t/db_dependent/Illrequests.t ................... 9/13
    #   Failed test 'Item creation fails on bad parameters'
    #   at t/db_dependent/Illrequests.t line 1008.
    # found warning: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`items`, CONSTRAINT `items_ibfk_2` FOREIGN KEY (`homebranch`) REFERENCES `branches` (`branchcode`) ON UPDATE CASCADE) [for Statement "INSERT INTO `items` ( `barcode`, `biblioitemnumber`, `biblionumber`, `damaged_on`, `dateaccessioned`, `datelastborrowed`, `datelastseen`, `holdingbranch`, `homebranch`, `itemlost_on`, `itype`, `location`, `onloan`, `permanent_location`, `replacementpricedate`, `timestamp`, `withdrawn_on`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, current_timestamp, ? )" with ParamValues: 0='ILL-26', 1=493, 2=497, 3=undef, 4='2020-11-11T15:06:34', 5=undef, 6='2020-11-11T15:06:34', 7='---', 8='---', 9=undef, 10='AfCJ5loFV', 11='---', 12=undef, 13='---', 14='2020-11-11T15:06:34', 15=undef] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.
    # found warning: DBIx::Class::Storage::DBI::_dbh_execute(): Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`items`, CONSTRAINT `items_ibfk_2` FOREIGN KEY (`homebranch`) REFERENCES `branches` (`branchcode`) ON UPDATE CASCADE) at /kohadevbox/koha/Koha/Object.pm line 169
    # expected to find warning: (?^u:DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails)

Feel free to open a separate bug report if it's not directly related to this change.
Comment 189 Jonathan Druart 2020-11-11 17:04:29 UTC
Created attachment 113522 [details] [review]
Bug 22818: Fix TestBuilder.t

t/db_dependent/TestBuilder.t .. 12/13
 #   Failed test 'build_object() tests'
 #   at t/db_dependent/TestBuilder.t line 429.
Can't locate object method "_new_from_dbic" via package "Koha::MessageAttribute" (perhaps you forgot to load "Koha::MessageAttribute"?) at /kohadevbox/koha/Koha/Objects.pm line 98.
Comment 190 Andrew Isherwood 2020-11-12 11:32:03 UTC
Created attachment 113549 [details] [review]
Bug 22818: (QA follow-up) Respond to feedback

This commit addresses the requests from Jonathan in comment #188
Comment 191 Andrew Isherwood 2020-11-12 11:32:26 UTC
(In reply to Jonathan Druart from comment #188)

Hi Jonathan

> 1. Fix the checkbox wording

Done

> 2. $dbh->{mysql_insertid}; in the upgrade process should be replaced. It's a
> MySQLism

Good point, done

> 3. t/db_dependent/Illrequests.t is passing but, only when run once:

I can't replicate this:

root@kohadevbox:koha(master)$ prove t/db_dependent/Illrequests.t
t/db_dependent/Illrequests.t .. ok     
All tests successful.
Files=1, Tests=13, 19 wallclock secs ( 0.08 usr  0.01 sys + 12.17 cusr  3.31 csys = 15.57 CPU)
Result: PASS
root@kohadevbox:koha(master)$ prove t/db_dependent/Illrequests.t
t/db_dependent/Illrequests.t .. ok     
All tests successful.
Files=1, Tests=13, 19 wallclock secs ( 0.06 usr  0.01 sys + 12.25 cusr  2.83 csys = 15.15 CPU)
Result: PASS
Comment 192 Jonathan Druart 2020-11-12 11:48:26 UTC
(In reply to Andrew Isherwood from comment #191)
> (In reply to Jonathan Druart from comment #188)
> > 3. t/db_dependent/Illrequests.t is passing but, only when run once:
> 
> I can't replicate this:

Did you try
$prove t/db_dependent/Circulation.t t/db_dependent/Circulation/issue.t t/db_dependent/Illrequests.t
Comment 193 Andrew Isherwood 2020-11-12 14:00:14 UTC
Arghhh, I missed that you'd run 3 lots of tests there. Anyway, I just tried that, and still can't replicate it:

root@kohadevbox:koha(master)$ prove t/db_dependent/Circulation.t t/db_dependent/Circulation/issue.t t/db_dependent/Illrequests.t
t/db_dependent/Circulation.t ........ ok     
t/db_dependent/Circulation/issue.t .. ok     
t/db_dependent/Illrequests.t ........ ok     
All tests successful.
Files=3, Tests=112, 84 wallclock secs ( 0.19 usr  0.03 sys + 61.96 cusr 10.98 csys = 73.16 CPU)
Result: PASS
root@kohadevbox:koha(master)$ prove t/db_dependent/Circulation.t t/db_dependent/Circulation/issue.t t/db_dependent/Illrequests.t
t/db_dependent/Circulation.t ........ ok     
t/db_dependent/Circulation/issue.t .. ok     
t/db_dependent/Illrequests.t ........ ok     
All tests successful.
Files=3, Tests=112, 83 wallclock secs ( 0.19 usr  0.02 sys + 61.94 cusr 10.65 csys = 72.80 CPU)
Result: PASS
Comment 194 Jonathan Druart 2020-11-12 16:30:20 UTC
Same, I can't recreate anymore, weird! Will see later if it popups.

Last patch pushed to master.
Comment 195 Jonathan Druart 2020-11-13 08:17:46 UTC
Created attachment 113584 [details] [review]
Bug 22818: Fix the last_insert_id call

DBI last_insert_id: invalid number of arguments: got handle + 3, expected handle + between 4 and 5
Usage: $h->last_insert_id($catalog, $schema, $table_name, $field_name [, \%attr ]) at /kohadevbox/koha/installer/data/mysql/updatedatabase.pl line 23326.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 196 Jonathan Druart 2020-11-13 08:18:05 UTC
(In reply to Jonathan Druart from comment #194)
> Same, I can't recreate anymore, weird! Will see later if it popups.
> 
> Last patch pushed to master.

Patch pushed to master.
Comment 197 Didier Gautheron 2020-11-13 08:22:23 UTC
Update datanase isn't working for me

DBI last_insert_id: invalid number of arguments: got handle + 3, expected handle + between 4 and 5
Usage: $h->last_insert_id($catalog, $schema, $table_name, $field_name [, \%attr ]) at ./installer/data/mysql/updatedatabase.pl line 23326.
Comment 198 Jonathan Druart 2020-11-13 08:36:45 UTC
(In reply to Didier Gautheron from comment #197)
> Update datanase isn't working for me
> 
> DBI last_insert_id: invalid number of arguments: got handle + 3, expected
> handle + between 4 and 5
> Usage: $h->last_insert_id($catalog, $schema, $table_name, $field_name [,
> \%attr ]) at ./installer/data/mysql/updatedatabase.pl line 23326.

I pushed a fix more than 1h ago:
commit dcc1b96ea9cba850b1d5cfb84a171685b1b2a5ad
    Bug 22818: Fix the last_insert_id call
Comment 199 Andrew Isherwood 2020-11-13 09:21:37 UTC
Thanks for that fix Jonathan! I actually had 4 parameters when I tested the upgrade, then just prior to committing I removed the last one thinking its absence would infer undef, I didn't realise last_insert_id expected 4. Sorry, silly mistake!
Comment 200 Lucas Gass 2020-11-16 22:38:36 UTC
new feature will not be backported to 20.05.x