Bug 22343 - Add configuration options for SMTP servers
Summary: Add configuration options for SMTP servers
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
: 22838 (view as bug list)
Depends on:
Blocks: 26188 27060 27424 28996 29365 31946 26290 26595 26705 26922 26948 27250 27356 28017 28023 28729 29330 29427 29501
  Show dependency treegraph
 
Reported: 2019-02-15 01:05 UTC by Father Vlasie
Modified: 2022-10-24 12:25 UTC (History)
15 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patchset adds the ability to set SMTP servers and then pick them for using on each library when sending notices, cart, etc. SSL/TLS authentication is supported [*] A new administration page is added for managing the servers. [*] ssl_mode=starttls is not supported under Ubuntu 16 due to old library versions.
Version(s) released in:
20.11.00


Attachments
Bug 22343: Add a new table to store SMTP servers configs (3.92 KB, patch)
2020-07-30 20:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add new table schema file (3.48 KB, patch)
2020-07-30 20:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: DBIx::Class schema updates [DO NOT PUSH] (1.39 KB, patch)
2020-07-30 20:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add classes for handling SMTP servers (19.16 KB, patch)
2020-07-30 20:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: API routes for SMTP servers CRUD (17.01 KB, patch)
2020-07-30 20:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add CRUD page for SMTP servers (26.15 KB, patch)
2020-07-30 20:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add CRUD page for SMTP servers (24.73 KB, patch)
2020-07-30 20:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add CRUD page for SMTP servers (24.73 KB, patch)
2020-07-30 20:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add a new table to store SMTP servers configs (3.99 KB, patch)
2020-08-10 13:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add new table schema file (3.72 KB, patch)
2020-08-10 13:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: DBIx::Class schema updates [DO NOT PUSH] (1.39 KB, patch)
2020-08-10 13:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add classes for handling SMTP servers (19.19 KB, patch)
2020-08-10 13:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: API routes for SMTP servers CRUD (17.21 KB, patch)
2020-08-10 13:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add CRUD page for SMTP servers (26.81 KB, patch)
2020-08-10 13:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add ->transport method (3.56 KB, patch)
2020-08-10 13:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Revamped Koha::Email class (10.52 KB, patch)
2020-08-10 13:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Make C4::Letters use the new SMTP server config (19.56 KB, patch)
2020-08-10 13:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Adapt Koha::Illrequest (3.49 KB, patch)
2020-08-10 13:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Adapt controller scripts (15.75 KB, patch)
2020-08-10 13:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Adapt runreport.pl (3.81 KB, patch)
2020-08-10 13:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Revamped Koha::Email class (10.94 KB, patch)
2020-08-10 13:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Prevent passing password to the UI (1.78 KB, patch)
2020-08-10 15:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add a new table to store SMTP servers configs (3.99 KB, patch)
2020-08-12 13:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add new table schema file (3.72 KB, patch)
2020-08-12 13:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: DBIx::Class schema updates [DO NOT PUSH] (1.39 KB, patch)
2020-08-12 13:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add classes for handling SMTP servers (19.19 KB, patch)
2020-08-12 13:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: API routes for SMTP servers CRUD (17.21 KB, patch)
2020-08-12 13:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add CRUD page for SMTP servers (26.81 KB, patch)
2020-08-12 13:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add ->transport method (3.58 KB, patch)
2020-08-12 13:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Revamped Koha::Email class (10.94 KB, patch)
2020-08-12 13:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Make C4::Letters use the new SMTP server config (19.56 KB, patch)
2020-08-12 13:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Adapt Koha::Illrequest (3.73 KB, patch)
2020-08-12 13:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Adapt controller scripts (19.65 KB, patch)
2020-08-12 13:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Adapt runreport.pl (3.81 KB, patch)
2020-08-12 13:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Prevent passing password to the UI (1.78 KB, patch)
2020-08-12 13:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add a new table to store SMTP servers configs (4.05 KB, patch)
2020-08-12 16:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Add new table schema file (3.79 KB, patch)
2020-08-12 16:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: DBIx::Class schema updates [DO NOT PUSH] (1.46 KB, patch)
2020-08-12 16:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Add classes for handling SMTP servers (19.25 KB, patch)
2020-08-12 16:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: API routes for SMTP servers CRUD (17.27 KB, patch)
2020-08-12 16:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Add CRUD page for SMTP servers (26.87 KB, patch)
2020-08-12 16:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Add ->transport method (3.64 KB, patch)
2020-08-12 16:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Revamped Koha::Email class (11.00 KB, patch)
2020-08-12 16:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Make C4::Letters use the new SMTP server config (19.62 KB, patch)
2020-08-12 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Adapt Koha::Illrequest (3.80 KB, patch)
2020-08-12 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Adapt controller scripts (19.72 KB, patch)
2020-08-12 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Adapt runreport.pl (3.88 KB, patch)
2020-08-12 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Prevent passing password to the UI (1.84 KB, patch)
2020-08-12 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Explicit testing of charset (2.39 KB, patch)
2020-08-18 17:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add a new table to store SMTP servers configs (4.05 KB, patch)
2020-08-18 19:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add new table schema file (3.79 KB, patch)
2020-08-18 19:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: DBIx::Class schema updates [DO NOT PUSH] (1.46 KB, patch)
2020-08-18 19:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add classes for handling SMTP servers (19.25 KB, patch)
2020-08-18 19:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: API routes for SMTP servers CRUD (28.93 KB, patch)
2020-08-18 19:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add CRUD page for SMTP servers (26.87 KB, patch)
2020-08-18 19:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add ->transport method (3.64 KB, patch)
2020-08-18 19:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Revamped Koha::Email class (11.23 KB, patch)
2020-08-18 19:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Make C4::Letters use the new SMTP server config (19.62 KB, patch)
2020-08-18 19:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Adapt Koha::Illrequest (3.80 KB, patch)
2020-08-18 19:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Adapt controller scripts (19.72 KB, patch)
2020-08-18 19:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Adapt runreport.pl (3.88 KB, patch)
2020-08-18 19:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Prevent passing password to the UI (1.84 KB, patch)
2020-08-18 19:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: (QA follow-up) Do not use input type=number (2.66 KB, patch)
2020-08-19 12:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: (QA follow-up) Fix unit tests (1.75 KB, patch)
2020-08-19 14:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Add a new table to store SMTP servers configs (4.12 KB, patch)
2020-08-19 15:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Add new table schema file (3.86 KB, patch)
2020-08-19 15:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: DBIx::Class schema updates [DO NOT PUSH] (1.53 KB, patch)
2020-08-19 15:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Add classes for handling SMTP servers (19.32 KB, patch)
2020-08-19 15:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: API routes for SMTP servers CRUD (29.00 KB, patch)
2020-08-19 15:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Add CRUD page for SMTP servers (26.94 KB, patch)
2020-08-19 15:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Add ->transport method (3.71 KB, patch)
2020-08-19 15:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Revamped Koha::Email class (11.29 KB, patch)
2020-08-19 15:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Make C4::Letters use the new SMTP server config (19.69 KB, patch)
2020-08-19 15:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Adapt Koha::Illrequest (3.87 KB, patch)
2020-08-19 15:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Adapt controller scripts (19.79 KB, patch)
2020-08-19 15:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Adapt runreport.pl (3.95 KB, patch)
2020-08-19 15:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Prevent passing password to the UI (1.91 KB, patch)
2020-08-19 15:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: (QA follow-up) Do not use input type=number (2.73 KB, patch)
2020-08-19 15:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: (QA follow-up) Fix unit tests (1.80 KB, patch)
2020-08-19 15:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: having fun with uri components (2.77 KB, patch)
2020-08-20 09:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22343: Trivial code simplification (2.42 KB, patch)
2020-08-20 09:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22343: Add a new table to store SMTP servers configs (5.11 KB, patch)
2020-08-24 15:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add parameters => manage_smtp_servers subpermission (2.10 KB, patch)
2020-08-24 15:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add new tables schema file (5.69 KB, patch)
2020-08-24 15:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: DBIx::Class schema updates [DO NOT PUSH] (1.36 KB, patch)
2020-08-24 15:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add classes for handling SMTP servers (10.17 KB, patch)
2020-08-24 15:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add Koha::Library->smtp_server (5.83 KB, patch)
2020-08-24 15:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Update cpanfile (666 bytes, patch)
2020-08-24 15:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: API routes for SMTP servers CRUD (28.64 KB, patch)
2020-08-24 15:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add CRUD page for SMTP servers (28.90 KB, patch)
2020-08-24 15:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Revamped Koha::Email class (15.05 KB, patch)
2020-08-24 15:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Make C4::Letters use the new SMTP server config (21.79 KB, patch)
2020-08-24 15:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Adapt Koha::Illrequest (3.79 KB, patch)
2020-08-24 15:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Adapt controller scripts (18.77 KB, patch)
2020-08-24 15:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Adapt runreport.pl (3.89 KB, patch)
2020-08-24 15:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Prevent passing password to the UI (1.74 KB, patch)
2020-08-24 15:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Allow choosing an SMTP server in branches.pl (7.67 KB, patch)
2020-08-24 15:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add a new table to store SMTP servers configs (5.11 KB, patch)
2020-08-26 12:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add parameters => manage_smtp_servers subpermission (2.10 KB, patch)
2020-08-26 12:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add new tables schema file (5.69 KB, patch)
2020-08-26 12:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: DBIx::Class schema updates [DO NOT PUSH] (1.36 KB, patch)
2020-08-26 12:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add classes for handling SMTP servers (10.17 KB, patch)
2020-08-26 12:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add Koha::Library->smtp_server (5.83 KB, patch)
2020-08-26 12:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Update cpanfile (666 bytes, patch)
2020-08-26 12:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: API routes for SMTP servers CRUD (28.64 KB, patch)
2020-08-26 12:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add CRUD page for SMTP servers (28.74 KB, patch)
2020-08-26 12:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Revamped Koha::Email class (15.05 KB, patch)
2020-08-26 12:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Make C4::Letters use the new SMTP server config (21.77 KB, patch)
2020-08-26 12:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Adapt Koha::Illrequest (3.79 KB, patch)
2020-08-26 12:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Adapt controller scripts (18.77 KB, patch)
2020-08-26 12:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Adapt runreport.pl (3.89 KB, patch)
2020-08-26 12:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Prevent passing password to the UI (1.74 KB, patch)
2020-08-26 12:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Allow choosing an SMTP server in branches.pl (7.67 KB, patch)
2020-08-26 12:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add a new table to store SMTP servers configs (5.18 KB, patch)
2020-09-03 10:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Add parameters => manage_smtp_servers subpermission (2.17 KB, patch)
2020-09-03 10:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Add new tables schema file (5.76 KB, patch)
2020-09-03 10:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: DBIx::Class schema updates [DO NOT PUSH] (1.43 KB, patch)
2020-09-03 10:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Add classes for handling SMTP servers (10.25 KB, patch)
2020-09-03 10:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Add Koha::Library->smtp_server (5.90 KB, patch)
2020-09-03 10:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Update cpanfile (737 bytes, patch)
2020-09-03 10:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: API routes for SMTP servers CRUD (28.71 KB, patch)
2020-09-03 10:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Add CRUD page for SMTP servers (28.81 KB, patch)
2020-09-03 10:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Revamped Koha::Email class (15.12 KB, patch)
2020-09-03 10:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Make C4::Letters use the new SMTP server config (21.84 KB, patch)
2020-09-03 10:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Adapt Koha::Illrequest (3.86 KB, patch)
2020-09-03 10:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Adapt controller scripts (18.84 KB, patch)
2020-09-03 10:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Adapt runreport.pl (3.96 KB, patch)
2020-09-03 10:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Prevent passing password to the UI (1.81 KB, patch)
2020-09-03 10:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Allow choosing an SMTP server in branches.pl (7.74 KB, patch)
2020-09-03 10:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22343: Add a new table to store SMTP servers configs (5.22 KB, patch)
2020-09-08 08:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Add parameters => manage_smtp_servers subpermission (2.22 KB, patch)
2020-09-08 08:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Add new tables schema file (5.81 KB, patch)
2020-09-08 08:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: DBIx::Class schema updates [DO NOT PUSH] (1.48 KB, patch)
2020-09-08 08:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Add classes for handling SMTP servers (10.29 KB, patch)
2020-09-08 08:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Add Koha::Library->smtp_server (5.95 KB, patch)
2020-09-08 08:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Update cpanfile (786 bytes, patch)
2020-09-08 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: API routes for SMTP servers CRUD (28.76 KB, patch)
2020-09-08 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Add CRUD page for SMTP servers (28.86 KB, patch)
2020-09-08 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Revamped Koha::Email class (15.17 KB, patch)
2020-09-08 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Make C4::Letters use the new SMTP server config (21.89 KB, patch)
2020-09-08 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Adapt Koha::Illrequest (3.90 KB, patch)
2020-09-08 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Adapt controller scripts (18.88 KB, patch)
2020-09-08 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Adapt runreport.pl (4.01 KB, patch)
2020-09-08 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Prevent passing password to the UI (1.86 KB, patch)
2020-09-08 08:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: Allow choosing an SMTP server in branches.pl (7.79 KB, patch)
2020-09-08 08:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: (QA follow-up) Ensure FK names don't conflict (2.79 KB, patch)
2020-09-08 08:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: (QA follow-up) Fix some comments (1.44 KB, patch)
2020-09-08 08:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: (follow-up) Highlight debug mode enabled (1.87 KB, patch)
2020-09-09 10:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: (follow-up) Add a hint to the debug form option (1.14 KB, patch)
2020-09-09 10:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22343: (QA follow-up) Wrap email creation inside the try/catch block (9.35 KB, patch)
2020-10-01 14:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add warning to the about page if KohaAdminEmailAddress is invalid (6.10 KB, patch)
2020-10-02 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22343: Correctly handle password on editing (1.88 KB, patch)
2020-10-02 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22343: Display the correct default values on the admin page (1.01 KB, patch)
2020-10-02 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22343: Add the 'required' class when needed (2.33 KB, patch)
2020-10-02 10:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22343: Fix runreport encoding issues (1.11 KB, patch)
2020-10-02 13:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22343: Compiled CSS (120.48 KB, patch)
2020-10-02 16:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22343: Add exec flag on .t files (2.03 KB, patch)
2020-10-02 16:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22343: (follow-up) Make sure no invalid address is used in tests (1.72 KB, patch)
2020-10-02 19:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: (follow-up) Remove traces of Mail::Sendmail (1.12 KB, patch)
2020-10-02 19:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22343: Add exec flag on .t files (1.31 KB, patch)
2020-10-05 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22343: (follow-up) Remove traces of Mail::Sendmail - update POD (849 bytes, patch)
2020-10-05 09:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22343: Remove traces of libmail-sendmail-perl (989 bytes, patch)
2020-10-05 09:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22343: Add configuration options for SMTP servers (1.14 KB, patch)
2020-10-10 01:23 UTC, Mason James
Details | Diff | Splinter Review
Bug 22343: Make the tests pass on U16 (1.09 KB, patch)
2020-10-12 08:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22343: (QA follow-up) Remove double encoding from sent emails (1.77 KB, patch)
2020-11-06 11:45 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 22343: (follow-up) Correctly handle password on editing (1.33 KB, patch)
2020-11-19 16:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22343: (follow-up) Correctly handle password on editing (1.38 KB, patch)
2020-11-19 19:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Father Vlasie 2019-02-15 01:05:36 UTC
I would like to add fields to Koha>Administration for SMTP settings to make it easy to use external mail servers without having to edit files on the server.

I am thinking that we could edit koha/C4/Letters.pm adding $mail{smtp} = 'my.mail.server';

I am not sure how the settings are passed to Letters.pm however.

What does everyone else think? Is this the right way to do it?

Thank you!

FV
Comment 1 Katrin Fischer 2019-04-06 12:49:02 UTC
I have updated the bug description in hope to make this a bit more visible. I am not sure if what you suggest is an option, maybe someone else will comment.
Comment 2 David Cook 2020-07-29 03:04:36 UTC
I'd have to review the code, which I'm probably not going to do myself, but overall I think that this is a great idea. 

Keycloak, for instance, provides a very nice web UI for specifying an external mail server to use (as well as configuring email headers to use).

Keep in mind that you should probably provide both the hostname and the port.
Comment 3 Tomás Cohen Arazi 2020-07-30 20:04:09 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2020-07-30 20:04:13 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2020-07-30 20:04:17 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2020-07-30 20:04:21 UTC Comment hidden (obsolete)
Comment 7 Tomás Cohen Arazi 2020-07-30 20:04:25 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2020-07-30 20:04:29 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2020-07-30 20:19:32 UTC Comment hidden (obsolete)
Comment 10 Tomás Cohen Arazi 2020-07-30 20:21:32 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2020-07-30 20:23:08 UTC
This is not yet used. I'm working on integrating this configurations so they are used instead of the hardcoded localhost:25 Mail::SendMail uses.

I'm almost done!

PS tests for the introduced API are missing
Comment 12 Tomás Cohen Arazi 2020-07-30 20:26:01 UTC
The plan is to use this:

https://metacpan.org/pod/Email::Sender::Transport::SMTP

so the conversion should be straight-forward. Email::Sender provides the same ::sendmail method we use, we just need to instantiate the transport object based on the effective library config. Almost there!
Comment 13 Liz Rea 2020-08-03 15:01:42 UTC
I am 100% for this patch, anything that puts the control in the hands of the libraries and doesn't make linux knowledge a gatekeeping factor in configuring the system is a vast improvement over how it is now.

We need this, freedom to the libraries and less-technical users.

Liz
Comment 14 David Cook 2020-08-04 00:29:25 UTC
(In reply to Liz Rea from comment #13)
> I am 100% for this patch, anything that puts the control in the hands of the
> libraries and doesn't make linux knowledge a gatekeeping factor in
> configuring the system is a vast improvement over how it is now.
> 
> We need this, freedom to the libraries and less-technical users.
> 

Agreed.

Plus, I think it makes it easier to deploy and scale Koha even for technical users when we have this information in the app database rather than having to mess around with third-party MTA apps and what not (or anything else on the file system).
Comment 15 David Cook 2020-08-04 00:30:20 UTC
(In reply to David Cook from comment #14)
> Plus, I think it makes it easier to deploy and scale Koha even for technical
> users when we have this information in the app database rather than having
> to mess around with third-party MTA apps and what not (or anything else on
> the file system).

Well maybe not easier to initially deploy... but easier to later scale and maintain...
Comment 16 Tomás Cohen Arazi 2020-08-04 01:09:52 UTC
(In reply to David Cook from comment #15)
> (In reply to David Cook from comment #14)
> > Plus, I think it makes it easier to deploy and scale Koha even for technical
> > users when we have this information in the app database rather than having
> > to mess around with third-party MTA apps and what not (or anything else on
> > the file system).
> 
> Well maybe not easier to initially deploy... but easier to later scale and
> maintain...

The idea is to allow configuring it so we don't need an SMTP Server running in localhost. Think of a dockered deployment.
Plus, give flexibility to the end users (non tech)
Comment 17 David Cook 2020-08-04 01:32:41 UTC
(In reply to Tomás Cohen Arazi from comment #16)
> The idea is to allow configuring it so we don't need an SMTP Server running
> in localhost. Think of a dockered deployment.

That's exactly the scenario I was thinking of heh.

> Plus, give flexibility to the end users (non tech)

Very handy
Comment 18 Katrin Fischer 2020-08-05 10:48:20 UTC
Is the use of these configuration options optional? Wondering if existing setups will still keep working on update.
Comment 19 Tomás Cohen Arazi 2020-08-05 11:11:42 UTC
(In reply to Katrin Fischer from comment #18)
> Is the use of these configuration options optional? Wondering if existing
> setups will still keep working on update.

If you don't set anything, it defaults to the current localhost:25. It is hardcoded as it is now.
Comment 20 Katrin Fischer 2020-08-05 12:24:59 UTC
So if nothing is set, the MTA settings will be used. Thx!
Comment 21 Tomás Cohen Arazi 2020-08-05 12:32:11 UTC
(In reply to Katrin Fischer from comment #20)
> So if nothing is set, the MTA settings will be used. Thx!

Technically, we currently use Mail::Sendmail which has a hardcoded value (localhost, port 25, no auth, no SSL, etc). The plan is to be able to set it, and obviously keep the current behaviour by default. I wouldn't dare to change that :-D
Comment 22 Tomás Cohen Arazi 2020-08-10 13:29:02 UTC Comment hidden (obsolete)
Comment 23 Tomás Cohen Arazi 2020-08-10 13:29:08 UTC Comment hidden (obsolete)
Comment 24 Tomás Cohen Arazi 2020-08-10 13:29:15 UTC Comment hidden (obsolete)
Comment 25 Tomás Cohen Arazi 2020-08-10 13:29:22 UTC Comment hidden (obsolete)
Comment 26 Tomás Cohen Arazi 2020-08-10 13:29:28 UTC Comment hidden (obsolete)
Comment 27 Tomás Cohen Arazi 2020-08-10 13:29:34 UTC Comment hidden (obsolete)
Comment 28 Tomás Cohen Arazi 2020-08-10 13:29:38 UTC Comment hidden (obsolete)
Comment 29 Tomás Cohen Arazi 2020-08-10 13:29:43 UTC Comment hidden (obsolete)
Comment 30 Tomás Cohen Arazi 2020-08-10 13:29:48 UTC Comment hidden (obsolete)
Comment 31 Tomás Cohen Arazi 2020-08-10 13:29:53 UTC Comment hidden (obsolete)
Comment 32 Tomás Cohen Arazi 2020-08-10 13:29:59 UTC Comment hidden (obsolete)
Comment 33 Tomás Cohen Arazi 2020-08-10 13:30:05 UTC Comment hidden (obsolete)
Comment 34 Tomás Cohen Arazi 2020-08-10 13:33:39 UTC Comment hidden (obsolete)
Comment 35 Tomás Cohen Arazi 2020-08-10 13:35:33 UTC
Ok, this is now complete (i.e. all places that send emails use the new feature). Go test!
Comment 36 Tomás Cohen Arazi 2020-08-10 13:36:05 UTC
You need to install Email::Stuffer:

  $ sudo apt install libemail-stuffer-perl
Comment 37 Tomás Cohen Arazi 2020-08-10 15:58:27 UTC Comment hidden (obsolete)
Comment 38 Tomás Cohen Arazi 2020-08-12 13:34:39 UTC Comment hidden (obsolete)
Comment 39 Tomás Cohen Arazi 2020-08-12 13:34:44 UTC Comment hidden (obsolete)
Comment 40 Tomás Cohen Arazi 2020-08-12 13:34:49 UTC Comment hidden (obsolete)
Comment 41 Tomás Cohen Arazi 2020-08-12 13:34:55 UTC Comment hidden (obsolete)
Comment 42 Tomás Cohen Arazi 2020-08-12 13:35:01 UTC Comment hidden (obsolete)
Comment 43 Tomás Cohen Arazi 2020-08-12 13:35:08 UTC Comment hidden (obsolete)
Comment 44 Tomás Cohen Arazi 2020-08-12 13:35:16 UTC Comment hidden (obsolete)
Comment 45 Tomás Cohen Arazi 2020-08-12 13:35:24 UTC Comment hidden (obsolete)
Comment 46 Tomás Cohen Arazi 2020-08-12 13:35:30 UTC Comment hidden (obsolete)
Comment 47 Tomás Cohen Arazi 2020-08-12 13:35:38 UTC Comment hidden (obsolete)
Comment 48 Tomás Cohen Arazi 2020-08-12 13:35:47 UTC Comment hidden (obsolete)
Comment 49 Tomás Cohen Arazi 2020-08-12 13:35:55 UTC Comment hidden (obsolete)
Comment 50 Tomás Cohen Arazi 2020-08-12 13:36:04 UTC Comment hidden (obsolete)
Comment 51 Martin Renvoize 2020-08-12 16:07:27 UTC Comment hidden (obsolete)
Comment 52 Martin Renvoize 2020-08-12 16:07:31 UTC Comment hidden (obsolete)
Comment 53 Martin Renvoize 2020-08-12 16:07:36 UTC Comment hidden (obsolete)
Comment 54 Martin Renvoize 2020-08-12 16:07:40 UTC Comment hidden (obsolete)
Comment 55 Martin Renvoize 2020-08-12 16:07:44 UTC Comment hidden (obsolete)
Comment 56 Martin Renvoize 2020-08-12 16:07:48 UTC Comment hidden (obsolete)
Comment 57 Martin Renvoize 2020-08-12 16:07:52 UTC Comment hidden (obsolete)
Comment 58 Martin Renvoize 2020-08-12 16:07:56 UTC Comment hidden (obsolete)
Comment 59 Martin Renvoize 2020-08-12 16:08:00 UTC Comment hidden (obsolete)
Comment 60 Martin Renvoize 2020-08-12 16:08:04 UTC Comment hidden (obsolete)
Comment 61 Martin Renvoize 2020-08-12 16:08:08 UTC Comment hidden (obsolete)
Comment 62 Martin Renvoize 2020-08-12 16:08:12 UTC Comment hidden (obsolete)
Comment 63 Martin Renvoize 2020-08-12 16:08:16 UTC Comment hidden (obsolete)
Comment 64 Martin Renvoize 2020-08-12 16:13:53 UTC
Code reads well and is a big improvement. Tested various mail triggers with various configurations and all worked as expected.

Signing off.
Comment 65 Father Vlasie 2020-08-12 16:28:38 UTC
I just want to say thank you for this much needed feature with especial thanks to Tomás for doing the hard work!
Comment 66 David Cook 2020-08-13 04:33:23 UTC
Kudos to Tomas! I'm glad that my comment from last month was able to re-awaken this bug report!
Comment 67 Kyle M Hall 2020-08-18 16:36:34 UTC
Seems like a simple failure to fix, but I'm not sure what the fix should be. Is this a difference in response that will crop up from server differences? Or does it fail on your system as well now?

root@e9e637b7f066:koha(bug22343-qa)$ prove t/Koha/Email.t 
t/Koha/Email.t .. 1/2 
    #   Failed test 'Content type set correctly'
    #   at t/Koha/Email.t line 59.
    #          got: 'text/html; charset=utf-8'
    #     expected: 'text/html; charset="utf-8"'

    #   Failed test 'Content type set correctly'
    #   at t/Koha/Email.t line 84.
    #          got: 'text/plain; charset=utf-8'
    #     expected: 'text/plain; charset="utf-8"; format="flowed"'
    # Looks like you failed 2 tests of 21.
t/Koha/Email.t .. 2/2 
#   Failed test 'create() tests'
#   at t/Koha/Email.t line 85.
# Looks like you failed 1 test of 2.
t/Koha/Email.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/2 subtests 

Test Summary Report
-------------------
t/Koha/Email.t (Wstat: 256 Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 1
Files=1, Tests=2,  2 wallclock secs ( 0.02 usr  0.01 sys +  1.15 cusr  0.19 csys =  1.37 CPU)
Result: FAIL
Comment 68 Tomás Cohen Arazi 2020-08-18 17:06:34 UTC Comment hidden (obsolete)
Comment 69 Tomás Cohen Arazi 2020-08-18 19:39:51 UTC Comment hidden (obsolete)
Comment 70 Tomás Cohen Arazi 2020-08-18 19:40:07 UTC Comment hidden (obsolete)
Comment 71 Tomás Cohen Arazi 2020-08-18 19:40:23 UTC Comment hidden (obsolete)
Comment 72 Tomás Cohen Arazi 2020-08-18 19:40:33 UTC Comment hidden (obsolete)
Comment 73 Tomás Cohen Arazi 2020-08-18 19:40:50 UTC Comment hidden (obsolete)
Comment 74 Tomás Cohen Arazi 2020-08-18 19:41:12 UTC Comment hidden (obsolete)
Comment 75 Tomás Cohen Arazi 2020-08-18 19:41:32 UTC Comment hidden (obsolete)
Comment 76 Tomás Cohen Arazi 2020-08-18 19:41:42 UTC Comment hidden (obsolete)
Comment 77 Tomás Cohen Arazi 2020-08-18 19:41:52 UTC Comment hidden (obsolete)
Comment 78 Tomás Cohen Arazi 2020-08-18 19:42:02 UTC Comment hidden (obsolete)
Comment 79 Tomás Cohen Arazi 2020-08-18 19:42:15 UTC Comment hidden (obsolete)
Comment 80 Tomás Cohen Arazi 2020-08-18 19:42:34 UTC Comment hidden (obsolete)
Comment 81 Tomás Cohen Arazi 2020-08-18 19:42:45 UTC Comment hidden (obsolete)
Comment 82 Tomás Cohen Arazi 2020-08-18 19:52:34 UTC
(In reply to Kyle M Hall from comment #67)
> Seems like a simple failure to fix, but I'm not sure what the fix should be.
> Is this a difference in response that will crop up from server differences?
> Or does it fail on your system as well now?

The test relied too much on a specific way the header was built.

It actually highlighted bad design because the tests were showing the charset was being set as 'utf8' regardless. So I tweaked it a little bit so it actually sets the charset, following the design on Email::MIME and Email::Stuffer.

No place in the codebase is setting it with this rewrite, though. As we return UTF-8 email body. But worth leaving room for passing it, just in case, in the right way. The tests now check iso-8859-1 is being set if requested.
Comment 83 Tomás Cohen Arazi 2020-08-19 12:39:41 UTC Comment hidden (obsolete)
Comment 84 Kyle M Hall 2020-08-19 14:52:03 UTC Comment hidden (obsolete)
Comment 85 Kyle M Hall 2020-08-19 15:16:02 UTC Comment hidden (obsolete)
Comment 86 Kyle M Hall 2020-08-19 15:16:14 UTC Comment hidden (obsolete)
Comment 87 Kyle M Hall 2020-08-19 15:16:19 UTC Comment hidden (obsolete)
Comment 88 Kyle M Hall 2020-08-19 15:16:23 UTC Comment hidden (obsolete)
Comment 89 Kyle M Hall 2020-08-19 15:16:28 UTC Comment hidden (obsolete)
Comment 90 Kyle M Hall 2020-08-19 15:16:32 UTC Comment hidden (obsolete)
Comment 91 Kyle M Hall 2020-08-19 15:16:37 UTC Comment hidden (obsolete)
Comment 92 Kyle M Hall 2020-08-19 15:16:41 UTC Comment hidden (obsolete)
Comment 93 Kyle M Hall 2020-08-19 15:16:46 UTC Comment hidden (obsolete)
Comment 94 Kyle M Hall 2020-08-19 15:16:50 UTC Comment hidden (obsolete)
Comment 95 Kyle M Hall 2020-08-19 15:16:54 UTC Comment hidden (obsolete)
Comment 96 Kyle M Hall 2020-08-19 15:16:58 UTC Comment hidden (obsolete)
Comment 97 Kyle M Hall 2020-08-19 15:17:02 UTC Comment hidden (obsolete)
Comment 98 Kyle M Hall 2020-08-19 15:17:06 UTC Comment hidden (obsolete)
Comment 99 Kyle M Hall 2020-08-19 15:17:10 UTC Comment hidden (obsolete)
Comment 100 Tomás Cohen Arazi 2020-08-19 15:28:21 UTC
Thanks for your time, and the follow-up, Kyle!
Comment 101 Jonathan Druart 2020-08-20 09:30:41 UTC
1. If you edit a server without password, there is "****" anyway
I tried to fix it, and only display '****' if the password is defined. But then found another bug, you cannot remove the password field once it has been filled already.

2. Escaping/unescaping issues. Fixed in a follow-up patch.

3. Deletion - Why are not you using the DELETE route?
Here you are cheating, I don't think we should GET with "op=delete", the parameters stay in the url

4. flagsrequired   => { parameters => 1 },
Are you sure it's what we want? Not a dedicated subperm?

5. "SMTP servers Define which SMTP servers to use." is displayed on the admin home page regardless the logged in user has enough permission (click then get "Error: You do not have permission to access this page.")

6. 
-    $params->{from} ||= C4::Context->preference('KohaAdminEmailAddress');
+    my $from    = $params->{from}    // C4::Context->preference('KohaAdminEmailAddress');

Any good reasons? I'd stick with || to catch ""

7. I have tried to cheat and insert twice a default server, on the form edit the dropdown and select value "" (create it)
I don't get any errors and see in the log "Cannot add or update a child row: a foreign key constraint fails" for the branch fk
I then noticed you used "*"
In admin/smtp_servers.pl:
 77     catch {
 78         if ( blessed $_ and $_->isa('Koha::Exceptions::Object::DuplicateID') ) {

else? We are hiding a potential error/exception. We must log it anyway and make the interface display "something wrong happened"

8. Should not we display the "default config" if no default server is defined?

9. And, finally, is this working? I don't think so.

kohadev-koha@kohadevbox:/kohadevbox/koha$ perl misc/cronjobs/process_message_queue.pl 
Use of uninitialized value for string at /usr/share/perl5/Email/MIME/Encode.pm line 66.
Undefined subroutine &Email::Sender::Simple::sendmail called at /kohadevbox/koha/C4/Letters.pm line 1386.
 at /kohadevbox/koha/C4/Letters.pm line 1405.

% pmvers Email::Sender::Simple
1.300034

If I replace Email::Sender::Simple::sendmail with sendmail (following the POD of the module), I get:
sending email message to patron: 51 at /kohadevbox/koha/C4/Letters.pm line 1007.
Use of uninitialized value for string at /usr/share/perl5/Email/MIME/Encode.pm line 66.
Don't know how to handle Koha::Email at /usr/share/perl5/Email/Sender/Role/CommonSending.pm line 78.
 at /kohadevbox/koha/C4/Letters.pm line 1405.

Proving tests is not a sufficient test plan.
Comment 102 Jonathan Druart 2020-08-20 09:32:20 UTC Comment hidden (obsolete)
Comment 103 Jonathan Druart 2020-08-20 09:32:25 UTC Comment hidden (obsolete)
Comment 104 Tomás Cohen Arazi 2020-08-20 11:04:22 UTC
(In reply to Jonathan Druart from comment #101)
> 1. If you edit a server without password, there is "****" anyway
> I tried to fix it, and only display '****' if the password is defined. But
> then found another bug, you cannot remove the password field once it has
> been filled already.

I didn't think about deleting the password, good catch.

> 2. Escaping/unescaping issues. Fixed in a follow-up patch.

Thanks

> 3. Deletion - Why are not you using the DELETE route?
> Here you are cheating, I don't think we should GET with "op=delete", the
> parameters stay in the url

That's because I based it on branches.pl and didn't want to make this bug dependent on an open discussion on how to use the API. I can anyway make the modal call the DELETE route and refresh the datatable.

> 4. flagsrequired   => { parameters => 1 },
> Are you sure it's what we want? Not a dedicated subperm?

That's actually a good idea, I will probably add it here. Kyle suggested something along the same lines. parameters => smtp_servers?

> 5. "SMTP servers Define which SMTP servers to use." is displayed on the
> admin home page regardless the logged in user has enough permission (click
> then get "Error: You do not have permission to access this page.")

Oops.

> 6. 
> -    $params->{from} ||= C4::Context->preference('KohaAdminEmailAddress');
> +    my $from    = $params->{from}    //
> C4::Context->preference('KohaAdminEmailAddress');
> 
> Any good reasons? I'd stick with || to catch ""

I'll fix it. My opinion is we should (on a separate bug) use Email::Valid against all passed email addresses. But I'd do it on a separate bug because we need to revisit all controller scripts to verify the exception is caught correctly (you guessed, it is not, and there's no eval/try generally).

> 7. I have tried to cheat and insert twice a default server, on the form edit
> the dropdown and select value "" (create it)
> I don't get any errors and see in the log "Cannot add or update a child row:
> a foreign key constraint fails" for the branch fk
> I then noticed you used "*"
> In admin/smtp_servers.pl:
>  77     catch {
>  78         if ( blessed $_ and
> $_->isa('Koha::Exceptions::Object::DuplicateID') ) {
> 
> else? We are hiding a potential error/exception. We must log it anyway and
> make the interface display "something wrong happened"

I'll see what you're talking about and provide a fix.

> 8. Should not we display the "default config" if no default server is
> defined?

Would you display above the datatable? Or just as another row?

> 9. And, finally, is this working? I don't think so.
> 
> kohadev-koha@kohadevbox:/kohadevbox/koha$ perl
> misc/cronjobs/process_message_queue.pl 
> Use of uninitialized value for string at
> /usr/share/perl5/Email/MIME/Encode.pm line 66.
> Undefined subroutine &Email::Sender::Simple::sendmail called at
> /kohadevbox/koha/C4/Letters.pm line 1386.
>  at /kohadevbox/koha/C4/Letters.pm line 1405.
> 
> % pmvers Email::Sender::Simple
> 1.300034
> 
> If I replace Email::Sender::Simple::sendmail with sendmail (following the
> POD of the module), I get:
> sending email message to patron: 51 at /kohadevbox/koha/C4/Letters.pm line
> 1007.
> Use of uninitialized value for string at
> /usr/share/perl5/Email/MIME/Encode.pm line 66.
> Don't know how to handle Koha::Email at
> /usr/share/perl5/Email/Sender/Role/CommonSending.pm line 78.
>  at /kohadevbox/koha/C4/Letters.pm line 1405.

I'll check this. This is probably due to an unadvertised change I made.

> Proving tests is not a sufficient test plan.
Comment 105 Jonathan Druart 2020-08-20 12:13:59 UTC
(In reply to Tomás Cohen Arazi from comment #104)
> (In reply to Jonathan Druart from comment #101)
> That's because I based it on branches.pl and didn't want to make this bug
> dependent on an open discussion on how to use the API. I can anyway make the
> modal call the DELETE route and refresh the datatable.
> 
> > 4. flagsrequired   => { parameters => 1 },
> > Are you sure it's what we want? Not a dedicated subperm?
> 
> That's actually a good idea, I will probably add it here. Kyle suggested
> something along the same lines. parameters => smtp_servers?

Yes

> > 8. Should not we display the "default config" if no default server is
> > defined?
> 
> Would you display above the datatable? Or just as another row?

Above the table sounds good.
Comment 106 Tomás Cohen Arazi 2020-08-24 15:30:57 UTC Comment hidden (obsolete)
Comment 107 Tomás Cohen Arazi 2020-08-24 15:31:08 UTC Comment hidden (obsolete)
Comment 108 Tomás Cohen Arazi 2020-08-24 15:31:18 UTC Comment hidden (obsolete)
Comment 109 Tomás Cohen Arazi 2020-08-24 15:31:28 UTC Comment hidden (obsolete)
Comment 110 Tomás Cohen Arazi 2020-08-24 15:31:34 UTC Comment hidden (obsolete)
Comment 111 Tomás Cohen Arazi 2020-08-24 15:31:39 UTC Comment hidden (obsolete)
Comment 112 Tomás Cohen Arazi 2020-08-24 15:31:45 UTC Comment hidden (obsolete)
Comment 113 Tomás Cohen Arazi 2020-08-24 15:31:51 UTC Comment hidden (obsolete)
Comment 114 Tomás Cohen Arazi 2020-08-24 15:31:57 UTC Comment hidden (obsolete)
Comment 115 Tomás Cohen Arazi 2020-08-24 15:32:04 UTC Comment hidden (obsolete)
Comment 116 Tomás Cohen Arazi 2020-08-24 15:32:10 UTC Comment hidden (obsolete)
Comment 117 Tomás Cohen Arazi 2020-08-24 15:32:17 UTC Comment hidden (obsolete)
Comment 118 Tomás Cohen Arazi 2020-08-24 15:32:25 UTC Comment hidden (obsolete)
Comment 119 Tomás Cohen Arazi 2020-08-24 15:32:35 UTC Comment hidden (obsolete)
Comment 120 Tomás Cohen Arazi 2020-08-24 15:32:47 UTC Comment hidden (obsolete)
Comment 121 Tomás Cohen Arazi 2020-08-24 15:32:57 UTC Comment hidden (obsolete)
Comment 122 Tomás Cohen Arazi 2020-08-24 15:40:39 UTC
Ok, I almost started over with this one. I separated the information about SMTP server from the libraries themselves, which are now linked using a separate table.

Added an accessor to Koha::Library so we don't need to search for SMTP servers and things got cleaner.

Any gentle QA feedback is welcome. This has taken too long already.

TODO (separate bugs):
- Ability to set a default other than the hardcoded one, in koha-conf.xml
- (Maybe, if someone takes the time to ask for it) display information about the libraries that use the server, on the SMTP servers table.
Comment 123 Jonathan Druart 2020-08-25 08:44:16 UTC
(In reply to Tomás Cohen Arazi from comment #122)

Please provide a full test plan. There is still no reference to testing process_message queue and other ways to send email.
Which seems to be the core changes here.

> This has taken too long already.

Well, first patchset attached less than 1 month ago, I would not say "too long"...
Comment 124 Tomás Cohen Arazi 2020-08-25 12:22:13 UTC
(In reply to Jonathan Druart from comment #123)
> (In reply to Tomás Cohen Arazi from comment #122)
> 
> > This has taken too long already.
> 
> Well, first patchset attached less than 1 month ago, I would not say "too
> long"...

I meant the time I spent on rewriting it all after QA feedback, no mean to complain. Just saying there's room for improvement that can be done by me or someone else on another report.
Comment 125 Tomás Cohen Arazi 2020-08-25 12:51:22 UTC
The test plan was spread between different commits and some steps were missing. I'll try to sumarize

Test plan:

0. Install a new dependency:
   $ apt install libemail-stuffer-perl
1. Apply this patchset
2. Run:
   $ updatedatabase
3. Run all the tests:
   $ kshell
  k$ git diff origin/master --name-only | grep -e '\.t$' | xargs prove 
=> SUCCESS: All tests pass
4. Add a privileged user, notice there's a 'manage_smtp_servers' subpermission in 'parameters'
5. Play with adding/removing the permission and reloading the admin page
=> SUCCESS: the admin page displays/doesn't display the entry for SMTP servers management correctly.
6. Enter the SMTP servers page
=> SUCCESS: the left column displays an entry for the SMTP admin page
7. Have a known working SMTP configuration (I used an app-specific password on my Gmail account), enable debug mode if you want deeper knowledge on what's happening, on the logs
8. Add the SMTP server
=> SUCCESS: The table displays the server
9. Add/Edit/Remove dummy servers
=> SUCCESS: Things work as expected
10. Go to the libraries admin page
11. Edit all libraries, choosing the known working SMTP server
12. Try sending a cart or list from the admin interface and the OPAC
=> SUCCESS: It works! Or otherwise the logs show why it failed, YAY!
13. Generate some notices (fulfilled holds, checkin notices, etc)
14. Run:
  k$ perl misc/cronjobs/process_message_queue.pl
=> SUCCESS: Email goes out! 
15. Sign off :-D


A note about 'runreport.pl'.

'runreport.pl' is THE script that doesn't have an obvious 'library' associated to it. At least not without a change in behaviour. As this patches allow you to set an SMTP server for a library, but keep a hardcoded default that replicates the current behaviour (localhost:25, no auth, no ssl/tls) the only way to test this is by having an SMTP server (postfix?) running locally. Or with bug 26290, by setting the default in koha-conf.xml. On my tests, what I did was putting my SMTP config in Koha/SMTP/Servers.pm in the default hashref. That's another option
Comment 126 Jonathan Druart 2020-08-26 09:27:22 UTC
Why using warnings_exist instead of warning_is in the tests?
Comment 127 Tomás Cohen Arazi 2020-08-26 12:32:34 UTC
Created attachment 109155 [details] [review]
Bug 22343: Add a new table to store SMTP servers configs

This patch adds a new table to store SMTP servers configs.
Comment 128 Tomás Cohen Arazi 2020-08-26 12:32:43 UTC
Created attachment 109156 [details] [review]
Bug 22343: Add parameters => manage_smtp_servers subpermission

This patch introduces a new subpermission for managing SMTP servers, for
granularity.
Comment 129 Tomás Cohen Arazi 2020-08-26 12:32:52 UTC
Created attachment 109157 [details] [review]
Bug 22343: Add new tables schema file
Comment 130 Tomás Cohen Arazi 2020-08-26 12:33:02 UTC
Created attachment 109158 [details] [review]
Bug 22343: DBIx::Class schema updates [DO NOT PUSH]
Comment 131 Tomás Cohen Arazi 2020-08-26 12:33:10 UTC
Created attachment 109159 [details] [review]
Bug 22343: Add classes for handling SMTP servers

This patch introduces classes to handle SMTP servers. It is done in a
way that a default server can be set, and then per-library ones can be
set. All should be done with Koha::SMTP::Servers methods, that take care
of overwriting library-specific configs, and handle the defaults
retrieval and setting correctly.

To test:
1. Apply this patches
2. Run:
   $ kshell
   $ perl installer/data/mysql/updatedatabase.pl
=> SUCCESS: Atomic update is ok, smtp_servers table created
3. Run:
  k$ prove t/db_dependent/Koha/SMTP/
=> SUCCESS: Tests pass!
4. Sign off :-D
Comment 132 Tomás Cohen Arazi 2020-08-26 12:33:18 UTC
Created attachment 109160 [details] [review]
Bug 22343: Add Koha::Library->smtp_server
Comment 133 Tomás Cohen Arazi 2020-08-26 12:33:28 UTC
Created attachment 109161 [details] [review]
Bug 22343: Update cpanfile
Comment 134 Tomás Cohen Arazi 2020-08-26 12:33:39 UTC
Created attachment 109162 [details] [review]
Bug 22343: API routes for SMTP servers CRUD

This patch introduces API routes for handling SMTP servers.
Comment 135 Tomás Cohen Arazi 2020-08-26 12:33:46 UTC
Created attachment 109163 [details] [review]
Bug 22343: Add CRUD page for SMTP servers

This patch introduces a way to define SMTP servers either globally or
per-library.

To test:
1. Apply this patch
2. Find the SMTP servers entry in the admin page
3. Play with adding/removing SMTP servers
=> SUCCESS: All works as expected
4. Sign off :-D
Comment 136 Tomás Cohen Arazi 2020-08-26 12:33:53 UTC
Created attachment 109164 [details] [review]
Bug 22343: Revamped Koha::Email class

This patch completely rewrites the Koha::Email class, inheriting from
Email::Stuffer. The latter suits well the use by Email::Sender, which is
to replace Mail::Sendmail on this patchset.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/Koha/Email.t
=> SUCCESS: Tests pass!
3. Verify all conditional codepaths are covered
4. Sign off :-D
Comment 137 Tomás Cohen Arazi 2020-08-26 12:34:00 UTC
Created attachment 109165 [details] [review]
Bug 22343: Make C4::Letters use the new SMTP server config

This patch makes the different methods in C4::Letters use:
- Koha::SMTP::Servers: to get the effective SMTP server for the library
  or the fallback default if no library in context.
- New Koha::Email->create method for crafting the email envelope for
  sending.

The tests are adapted so they behave the same way, but the trapped (in
the mock) $email object has the right type and its attributes are
accessed correctly.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Letters.t
=> SUCCESS: Tests pass. YAY!
3. Sign off :-D
Comment 138 Tomás Cohen Arazi 2020-08-26 12:34:13 UTC
Created attachment 109166 [details] [review]
Bug 22343: Adapt Koha::Illrequest

This patch makes Koha::Illrequest use the new syntax for Koha::Email and
sending the email.
Comment 139 Tomás Cohen Arazi 2020-08-26 12:34:23 UTC
Created attachment 109167 [details] [review]
Bug 22343: Adapt controller scripts

This patch adapts controller scripts that used sendmail. Also the syntax
for Koha::Email has changed and this patch adapts it.
Comment 140 Tomás Cohen Arazi 2020-08-26 12:34:35 UTC
Created attachment 109168 [details] [review]
Bug 22343: Adapt runreport.pl

This script gets adapted to use the new Koha::Email syntax and SMTP
transport.
Comment 141 Tomás Cohen Arazi 2020-08-26 12:34:42 UTC
Created attachment 109169 [details] [review]
Bug 22343: Prevent passing password to the UI
Comment 142 Tomás Cohen Arazi 2020-08-26 12:34:48 UTC
Created attachment 109170 [details] [review]
Bug 22343: Allow choosing an SMTP server in branches.pl

The missing piece on this patchset is choosing the desired SMTP server
for each library. This patch adds the option on editing the library.

The default one is picked for existing libraries, and you can pick the
one you want.

To test:
1. Apply the patchset
2. Run:
   $ updatedatabase
=> SUCCESS: Things go smooth
3. Notice the new permission for managing SMTP servers
4. Notice the entry in the admin page
5. Add/have a few SMTP servers defined.
6. Edit a library
=> SUCCESS: The 'Default' SMTP server is picked
7. Choose any server and save
=> SUCCESS: Saving doesn't explode
8. Edit the server
=> SUCCESS: Your selection is kept (this can also be checked on the DB,
        the library_smtp_server table)
9. Try (7) with many different options
=> SUCCESS: It works
10. Sign off :-D
Comment 143 Tomás Cohen Arazi 2020-08-26 12:37:40 UTC
(In reply to Jonathan Druart from comment #126)
> Why using warnings_exist instead of warning_is in the tests?

That's a left over from a previous iteration. Fixed!
And thanks for pointing it out
Comment 144 Tomás Cohen Arazi 2020-08-26 13:35:16 UTC
(In reply to Tomás Cohen Arazi from comment #122)
> TODO (separate bugs):
> - Ability to set a default other than the hardcoded one, in koha-conf.xml

Submitted to bug 26290.
Comment 145 Kyle M Hall 2020-09-03 10:21:13 UTC
Created attachment 109601 [details] [review]
Bug 22343: Add a new table to store SMTP servers configs

This patch adds a new table to store SMTP servers configs.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 146 Kyle M Hall 2020-09-03 10:21:42 UTC
Created attachment 109602 [details] [review]
Bug 22343: Add parameters => manage_smtp_servers subpermission

This patch introduces a new subpermission for managing SMTP servers, for
granularity.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 147 Kyle M Hall 2020-09-03 10:21:51 UTC
Created attachment 109603 [details] [review]
Bug 22343: Add new tables schema file

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 148 Kyle M Hall 2020-09-03 10:22:01 UTC
Created attachment 109604 [details] [review]
Bug 22343: DBIx::Class schema updates [DO NOT PUSH]

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 149 Kyle M Hall 2020-09-03 10:22:08 UTC
Created attachment 109605 [details] [review]
Bug 22343: Add classes for handling SMTP servers

This patch introduces classes to handle SMTP servers. It is done in a
way that a default server can be set, and then per-library ones can be
set. All should be done with Koha::SMTP::Servers methods, that take care
of overwriting library-specific configs, and handle the defaults
retrieval and setting correctly.

To test:
1. Apply this patches
2. Run:
   $ kshell
   $ perl installer/data/mysql/updatedatabase.pl
=> SUCCESS: Atomic update is ok, smtp_servers table created
3. Run:
  k$ prove t/db_dependent/Koha/SMTP/
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 150 Kyle M Hall 2020-09-03 10:22:14 UTC
Created attachment 109606 [details] [review]
Bug 22343: Add Koha::Library->smtp_server

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 151 Kyle M Hall 2020-09-03 10:22:18 UTC
Created attachment 109607 [details] [review]
Bug 22343: Update cpanfile

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 152 Kyle M Hall 2020-09-03 10:22:27 UTC
Created attachment 109608 [details] [review]
Bug 22343: API routes for SMTP servers CRUD

This patch introduces API routes for handling SMTP servers.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 153 Kyle M Hall 2020-09-03 10:22:36 UTC
Created attachment 109609 [details] [review]
Bug 22343: Add CRUD page for SMTP servers

This patch introduces a way to define SMTP servers either globally or
per-library.

To test:
1. Apply this patch
2. Find the SMTP servers entry in the admin page
3. Play with adding/removing SMTP servers
=> SUCCESS: All works as expected
4. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 154 Kyle M Hall 2020-09-03 10:22:44 UTC
Created attachment 109610 [details] [review]
Bug 22343: Revamped Koha::Email class

This patch completely rewrites the Koha::Email class, inheriting from
Email::Stuffer. The latter suits well the use by Email::Sender, which is
to replace Mail::Sendmail on this patchset.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/Koha/Email.t
=> SUCCESS: Tests pass!
3. Verify all conditional codepaths are covered
4. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 155 Kyle M Hall 2020-09-03 10:22:49 UTC
Created attachment 109611 [details] [review]
Bug 22343: Make C4::Letters use the new SMTP server config

This patch makes the different methods in C4::Letters use:
- Koha::SMTP::Servers: to get the effective SMTP server for the library
  or the fallback default if no library in context.
- New Koha::Email->create method for crafting the email envelope for
  sending.

The tests are adapted so they behave the same way, but the trapped (in
the mock) $email object has the right type and its attributes are
accessed correctly.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Letters.t
=> SUCCESS: Tests pass. YAY!
3. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 156 Kyle M Hall 2020-09-03 10:22:54 UTC
Created attachment 109612 [details] [review]
Bug 22343: Adapt Koha::Illrequest

This patch makes Koha::Illrequest use the new syntax for Koha::Email and
sending the email.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 157 Kyle M Hall 2020-09-03 10:23:07 UTC
Created attachment 109613 [details] [review]
Bug 22343: Adapt controller scripts

This patch adapts controller scripts that used sendmail. Also the syntax
for Koha::Email has changed and this patch adapts it.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 158 Kyle M Hall 2020-09-03 10:23:19 UTC
Created attachment 109614 [details] [review]
Bug 22343: Adapt runreport.pl

This script gets adapted to use the new Koha::Email syntax and SMTP
transport.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 159 Kyle M Hall 2020-09-03 10:23:24 UTC
Created attachment 109615 [details] [review]
Bug 22343: Prevent passing password to the UI

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 160 Kyle M Hall 2020-09-03 10:23:33 UTC
Created attachment 109616 [details] [review]
Bug 22343: Allow choosing an SMTP server in branches.pl

The missing piece on this patchset is choosing the desired SMTP server
for each library. This patch adds the option on editing the library.

The default one is picked for existing libraries, and you can pick the
one you want.

To test:
1. Apply the patchset
2. Run:
   $ updatedatabase
=> SUCCESS: Things go smooth
3. Notice the new permission for managing SMTP servers
4. Notice the entry in the admin page
5. Add/have a few SMTP servers defined.
6. Edit a library
=> SUCCESS: The 'Default' SMTP server is picked
7. Choose any server and save
=> SUCCESS: Saving doesn't explode
8. Edit the server
=> SUCCESS: Your selection is kept (this can also be checked on the DB,
        the library_smtp_server table)
9. Try (7) with many different options
=> SUCCESS: It works
10. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 161 Martin Renvoize 2020-09-08 08:51:15 UTC
Created attachment 109723 [details] [review]
Bug 22343: Add a new table to store SMTP servers configs

This patch adds a new table to store SMTP servers configs.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 162 Martin Renvoize 2020-09-08 08:51:21 UTC
Created attachment 109724 [details] [review]
Bug 22343: Add parameters => manage_smtp_servers subpermission

This patch introduces a new subpermission for managing SMTP servers, for
granularity.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 163 Martin Renvoize 2020-09-08 08:51:31 UTC
Created attachment 109725 [details] [review]
Bug 22343: Add new tables schema file

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 164 Martin Renvoize 2020-09-08 08:51:37 UTC
Created attachment 109726 [details] [review]
Bug 22343: DBIx::Class schema updates [DO NOT PUSH]

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 165 Martin Renvoize 2020-09-08 08:51:43 UTC
Created attachment 109727 [details] [review]
Bug 22343: Add classes for handling SMTP servers

This patch introduces classes to handle SMTP servers. It is done in a
way that a default server can be set, and then per-library ones can be
set. All should be done with Koha::SMTP::Servers methods, that take care
of overwriting library-specific configs, and handle the defaults
retrieval and setting correctly.

To test:
1. Apply this patches
2. Run:
   $ kshell
   $ perl installer/data/mysql/updatedatabase.pl
=> SUCCESS: Atomic update is ok, smtp_servers table created
3. Run:
  k$ prove t/db_dependent/Koha/SMTP/
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 166 Martin Renvoize 2020-09-08 08:51:54 UTC
Created attachment 109728 [details] [review]
Bug 22343: Add Koha::Library->smtp_server

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 167 Martin Renvoize 2020-09-08 08:52:13 UTC
Created attachment 109729 [details] [review]
Bug 22343: Update cpanfile

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 168 Martin Renvoize 2020-09-08 08:52:19 UTC
Created attachment 109730 [details] [review]
Bug 22343: API routes for SMTP servers CRUD

This patch introduces API routes for handling SMTP servers.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 169 Martin Renvoize 2020-09-08 08:52:25 UTC
Created attachment 109731 [details] [review]
Bug 22343: Add CRUD page for SMTP servers

This patch introduces a way to define SMTP servers either globally or
per-library.

To test:
1. Apply this patch
2. Find the SMTP servers entry in the admin page
3. Play with adding/removing SMTP servers
=> SUCCESS: All works as expected
4. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 170 Martin Renvoize 2020-09-08 08:52:30 UTC
Created attachment 109732 [details] [review]
Bug 22343: Revamped Koha::Email class

This patch completely rewrites the Koha::Email class, inheriting from
Email::Stuffer. The latter suits well the use by Email::Sender, which is
to replace Mail::Sendmail on this patchset.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/Koha/Email.t
=> SUCCESS: Tests pass!
3. Verify all conditional codepaths are covered
4. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 171 Martin Renvoize 2020-09-08 08:52:36 UTC
Created attachment 109733 [details] [review]
Bug 22343: Make C4::Letters use the new SMTP server config

This patch makes the different methods in C4::Letters use:
- Koha::SMTP::Servers: to get the effective SMTP server for the library
  or the fallback default if no library in context.
- New Koha::Email->create method for crafting the email envelope for
  sending.

The tests are adapted so they behave the same way, but the trapped (in
the mock) $email object has the right type and its attributes are
accessed correctly.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Letters.t
=> SUCCESS: Tests pass. YAY!
3. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 172 Martin Renvoize 2020-09-08 08:52:43 UTC
Created attachment 109734 [details] [review]
Bug 22343: Adapt Koha::Illrequest

This patch makes Koha::Illrequest use the new syntax for Koha::Email and
sending the email.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 173 Martin Renvoize 2020-09-08 08:52:50 UTC
Created attachment 109735 [details] [review]
Bug 22343: Adapt controller scripts

This patch adapts controller scripts that used sendmail. Also the syntax
for Koha::Email has changed and this patch adapts it.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 174 Martin Renvoize 2020-09-08 08:52:56 UTC
Created attachment 109736 [details] [review]
Bug 22343: Adapt runreport.pl

This script gets adapted to use the new Koha::Email syntax and SMTP
transport.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 175 Martin Renvoize 2020-09-08 08:53:01 UTC
Created attachment 109737 [details] [review]
Bug 22343: Prevent passing password to the UI

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 176 Martin Renvoize 2020-09-08 08:53:06 UTC
Created attachment 109738 [details] [review]
Bug 22343: Allow choosing an SMTP server in branches.pl

The missing piece on this patchset is choosing the desired SMTP server
for each library. This patch adds the option on editing the library.

The default one is picked for existing libraries, and you can pick the
one you want.

To test:
1. Apply the patchset
2. Run:
   $ updatedatabase
=> SUCCESS: Things go smooth
3. Notice the new permission for managing SMTP servers
4. Notice the entry in the admin page
5. Add/have a few SMTP servers defined.
6. Edit a library
=> SUCCESS: The 'Default' SMTP server is picked
7. Choose any server and save
=> SUCCESS: Saving doesn't explode
8. Edit the server
=> SUCCESS: Your selection is kept (this can also be checked on the DB,
        the library_smtp_server table)
9. Try (7) with many different options
=> SUCCESS: It works
10. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 177 Martin Renvoize 2020-09-08 08:53:12 UTC
Created attachment 109739 [details] [review]
Bug 22343: (QA follow-up) Ensure FK names don't conflict

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 178 Martin Renvoize 2020-09-08 08:53:18 UTC
Created attachment 109740 [details] [review]
Bug 22343: (QA follow-up) Fix some comments

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 179 Martin Renvoize 2020-09-08 09:08:26 UTC
This code looks great and I'm happy to SO as is (with the couple of very minor followups I've added).

I have a few points/questions/suggestions

* You introduce a 'debug' mode you can set at the smtp server level.. I like this, but at present I can't see any references to it anywhere.. it would be nice to see it used to enable/disable additional logging in at least one place with this patchset to give it a purpose and example of use.
* With the above debug mode, I'd like to see that state somehow highlighted in the smtp-servers page table if it's enabled (so it's obvious to an admin when it's turned on) (bold/italic row perhaps, similar to the 'default' hinting in cash registers)
* Final note about the debug mode.. it would be awesome as a later followup to expose a 'Send test mail' action into the smpt servers table to allow the immediate send of a test mail from that server with defaults set for from/replyo etc.

* It would be nice to see a list of attached branches as one fo the columns in the smtp servers table.
* Should deletes be blocked/warned about if the smtp server is being utilised by any libraries?
* The 'delete' was slow for me, which meant I could easily double submit the modal form (we should prevent the double submit)

* It would be nice to add the smtp server name as an optional column to display in the libraries table.

* Another enhancement for the future would be to support library groups


Generally, I really like this and it's a huge improvement.
Comment 180 Tomás Cohen Arazi 2020-09-08 13:14:31 UTC
Hi!

(In reply to Martin Renvoize from comment #179)
> This code looks great and I'm happy to SO as is (with the couple of very
> minor followups I've added).

Thanks for the extensive review!

> I have a few points/questions/suggestions
> 
> * You introduce a 'debug' mode you can set at the smtp server level.. I like
> this, but at present I can't see any references to it anywhere.. it would be
> nice to see it used to enable/disable additional logging in at least one
> place with this patchset to give it a purpose and example of use.

The debug flag is actually passed to Email::Sender::Transport::SMTP, and enables verbose logging to STDERR, so I expect logs to show in warnings. I tried it with process_message_queue.pl and it will print the whole SMTP authentication handshake and transaction steps including return codes. It is really helpful for debugging authentication issues. I'd say we should add a 'hint' on the edit page.

> * With the above debug mode, I'd like to see that state somehow highlighted
> in the smtp-servers page table if it's enabled (so it's obvious to an admin
> when it's turned on) (bold/italic row perhaps, similar to the 'default'
> hinting in cash registers)

Will try to add a follow-up for this.

> * Final note about the debug mode.. it would be awesome as a later followup
> to expose a 'Send test mail' action into the smpt servers table to allow the
> immediate send of a test mail from that server with defaults set for
> from/replyo etc.

I love the idea. Would add it to the edit page as it would be handy to have there for tweaking things in-place if something fails. Definitely a separate bug report.

> * It would be nice to see a list of attached branches as one fo the columns
> in the smtp servers table.
> * Should deletes be blocked/warned about if the smtp server is being
> utilised by any libraries?
> * The 'delete' was slow for me, which meant I could easily double submit the
> modal form (we should prevent the double submit)
> 
> * It would be nice to add the smtp server name as an optional column to
> display in the libraries table.

I'll file a new report for this as it requires some (incremental) tweaking to the libraries route.

> * Another enhancement for the future would be to support library groups

Good idea!

> Generally, I really like this and it's a huge improvement.

Thanks!
Comment 181 Martin Renvoize 2020-09-09 10:17:51 UTC
Created attachment 109787 [details] [review]
Bug 22343: (follow-up) Highlight debug mode enabled

This patch adds a debug class to a row if debug mode is enabled for the
smtp server and adds css rules to highlight the row by setting the test
to bold, red and italic.
Comment 182 Martin Renvoize 2020-09-09 10:23:16 UTC
Created attachment 109788 [details] [review]
Bug 22343: (follow-up) Add a hint to the debug form option

This patch appends a small note to the debug form field in the edit/add
smtp server form to hint that debug mode results in additional output in
the logs.
Comment 183 Martin Renvoize 2020-09-17 09:35:37 UTC
Seems I forgot to set PQA on this..
Comment 184 Jonathan Druart 2020-10-01 08:27:40 UTC
Found a 500:
Apply patches, updatedatabase, nothing else.
Send a basket (OPAC):
Invalid 'from' parameter: root@localhost at /usr/share/perl5/Exception/Class/Base.pm line 88
Comment 185 Jonathan Druart 2020-10-01 08:28:49 UTC
Found another one:
Replace KohaAdminEmailAddress with root@example.org
Send a basket (use '❤' in biblio title and "comment" of the email)

Wide character in subroutine entry at /usr/share/perl5/Email/MIME/Encodings.pm line 55
Comment 186 Tomás Cohen Arazi 2020-10-01 14:40:19 UTC
Created attachment 111048 [details] [review]
Bug 22343: (QA follow-up) Wrap email creation inside the try/catch block

At a later development stage, exceptions where added for bad addresses.
This wasn't addressed in the controllers.

This patch makes the basket and list sending controller scripts move
email creation inside the try/catch block to handle those situations. It
also UTF-8 encodes the attached marc.

On broadly testing this I found that if the TT templates that are used
to build the email contains non-latin characters, those get
double-encoded. So this patch also removes an explicit encoding that is
done, which colides with Email::MIME implicit encoding.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 187 Jonathan Druart 2020-10-02 10:04:20 UTC
Created attachment 111087 [details] [review]
Bug 22343: Add warning to the about page if KohaAdminEmailAddress is invalid
Comment 188 Jonathan Druart 2020-10-02 10:04:28 UTC
Created attachment 111088 [details] [review]
Bug 22343: Correctly handle password on editing
Comment 189 Jonathan Druart 2020-10-02 10:06:02 UTC
The "Default configuration" section at the top of the SMTP servers admin page contains the hardcoded values in Koha::SMTP::Servers->default_setting.

Should not it be the values from the config (and default to hardcoded values unless defined)?
Comment 190 Tomás Cohen Arazi 2020-10-02 10:07:10 UTC
(In reply to Jonathan Druart from comment #189)
> The "Default configuration" section at the top of the SMTP servers admin
> page contains the hardcoded values in Koha::SMTP::Servers->default_setting.
> 
> Should not it be the values from the config (and default to hardcoded values
> unless defined)?

That is done on the follow-up bug.
Comment 191 Jonathan Druart 2020-10-02 10:08:07 UTC
Created attachment 111089 [details] [review]
Bug 22343: Display the correct default values on the admin page

The "Default configuration" section at the top of the SMTP servers admin page
contains the hardcoded values in Koha::SMTP::Servers->default_setting.

It should be the values from the config (and default to hardcoded values unless defined)
Comment 192 Jonathan Druart 2020-10-02 10:17:27 UTC
Created attachment 111090 [details] [review]
Bug 22343: Add the 'required' class when needed
Comment 193 Jonathan Druart 2020-10-02 10:24:13 UTC
232                     <th data-class-name="actions">Actions</th>

Where is this data-class-name coming from? How it is useful?
Comment 194 Tomás Cohen Arazi 2020-10-02 10:54:07 UTC
(In reply to Jonathan Druart from comment #193)
> 232                     <th data-class-name="actions">Actions</th>
> 
> Where is this data-class-name coming from? How it is useful?

https://datatables.net/manual/options#HTML-5-data-attributes
Comment 195 Jonathan Druart 2020-10-02 11:25:58 UTC
(In reply to Tomás Cohen Arazi from comment #190)
> (In reply to Jonathan Druart from comment #189)
> > The "Default configuration" section at the top of the SMTP servers admin
> > page contains the hardcoded values in Koha::SMTP::Servers->default_setting.
> > 
> > Should not it be the values from the config (and default to hardcoded values
> > unless defined)?
> 
> That is done on the follow-up bug.

Yes, the question (and my follow-up) is for bug 26290. I am reviewing them together.

(In reply to Tomás Cohen Arazi from comment #194)
> (In reply to Jonathan Druart from comment #193)
> > 232                     <th data-class-name="actions">Actions</th>
> > 
> > Where is this data-class-name coming from? How it is useful?
> 
> https://datatables.net/manual/options#HTML-5-data-attributes

Yes but how is it useful? Is it just a matter of having a class on the "Actions" column or is there a CSS or any related code using it?
Comment 196 Jonathan Druart 2020-10-02 12:16:27 UTC
(In reply to Jonathan Druart from comment #191)
> Created attachment 111089 [details] [review] [review]
> Bug 22343: Display the correct default values on the admin page
> 
> The "Default configuration" section at the top of the SMTP servers admin page
> contains the hardcoded values in Koha::SMTP::Servers->default_setting.
> 
> It should be the values from the config (and default to hardcoded values
> unless defined)

Moved to bug 26290.
Comment 197 Jonathan Druart 2020-10-02 13:03:01 UTC
There is an encoding issue with runreport.pl, can you double check please?
Comment 198 Jonathan Druart 2020-10-02 13:23:19 UTC
Created attachment 111154 [details] [review]
Bug 22343: Fix runreport encoding issues
Comment 199 Jonathan Druart 2020-10-02 13:24:23 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 200 Jonathan Druart 2020-10-02 16:03:37 UTC
t/db_dependent/Passwordrecovery.t is failing, please fix ASAP
Comment 201 Jonathan Druart 2020-10-02 16:06:05 UTC
Created attachment 111165 [details] [review]
Bug 22343: Compiled CSS
Comment 202 Jonathan Druart 2020-10-02 16:06:13 UTC
Created attachment 111166 [details] [review]
Bug 22343: Add exec flag on .t files
Comment 203 Tomás Cohen Arazi 2020-10-02 19:00:35 UTC
(In reply to Jonathan Druart from comment #200)
> t/db_dependent/Passwordrecovery.t is failing, please fix ASAP

I have a fix for the tests. But I will take some more time to check we are not hiding a broader problem by fixing them. There is a behaviour change if things fail because branchemail and branchreplyto are not set.

I will file a separate bug for it.
Comment 204 Tomás Cohen Arazi 2020-10-02 19:19:42 UTC
Created attachment 111175 [details] [review]
Bug 22343: (follow-up) Make sure no invalid address is used in tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 205 Tomás Cohen Arazi 2020-10-02 19:20:11 UTC
Created attachment 111176 [details] [review]
Bug 22343: (follow-up) Remove traces of Mail::Sendmail

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 206 Jonathan Druart 2020-10-05 09:15:04 UTC
Created attachment 111213 [details] [review]
Bug 22343: Add exec flag on .t files
Comment 207 Jonathan Druart 2020-10-05 09:33:07 UTC
Created attachment 111215 [details] [review]
Bug 22343: (follow-up) Remove traces of Mail::Sendmail - update POD
Comment 208 Jonathan Druart 2020-10-05 09:33:16 UTC
Created attachment 111216 [details] [review]
Bug 22343: Remove traces of libmail-sendmail-perl
Comment 209 Jonathan Druart 2020-10-05 09:42:33 UTC
Last patches pushed to master!

ab2760e7e4 Bug 22343: Remove traces of libmail-sendmail-perl
a3a2c4cfff Bug 22343: (follow-up) Remove traces of Mail::Sendmail - update POD
704a053791 Bug 22343: (follow-up) Remove traces of Mail::Sendmail
04f2ecb56c Bug 22343: (follow-up) Make sure no invalid address is used in tests
c54da79431 Bug 22343: Add exec flag on .t files
b9a63feb73 Bug 22343: Compiled CSS
Comment 210 Tomás Cohen Arazi 2020-10-05 10:52:31 UTC
(In reply to Jonathan Druart from comment #188)
> Created attachment 111088 [details] [review] [review]
> Bug 22343: Correctly handle password on editing

My idea was to entirely not pass the password to the template, and only write it on change.
Comment 211 Jonathan Druart 2020-10-05 11:04:21 UTC
(In reply to Tomás Cohen Arazi from comment #210)
> (In reply to Jonathan Druart from comment #188)
> > Created attachment 111088 [details] [review] [review] [review]
> > Bug 22343: Correctly handle password on editing
> 
> My idea was to entirely not pass the password to the template, and only
> write it on change.

Ho right. So maybe we should pass a "password_set" boolean instead?
Comment 212 Jonathan Druart 2020-10-05 11:05:21 UTC
(In reply to Jonathan Druart from comment #211)
> (In reply to Tomás Cohen Arazi from comment #210)
> > (In reply to Jonathan Druart from comment #188)
> > > Created attachment 111088 [details] [review] [review] [review] [review]
> > > Bug 22343: Correctly handle password on editing
> > 
> > My idea was to entirely not pass the password to the template, and only
> > write it on change.
> 
> Ho right. So maybe we should pass a "password_set" boolean instead?

Well, it will be the same. We pass it to the template but we don't display it actually ("****" is displayed).
Comment 213 Jonathan Druart 2020-10-09 13:27:33 UTC
Koha_Master_U16 is failing with:

14:49:23 koha_1       |     # Looks like you planned 4 tests but ran 2.
14:49:23 koha_1       | 
14:49:23 koha_1       | #   Failed test 'transport() tests'
14:49:23 koha_1       | #   at t/db_dependent/Koha/SMTP/Server.t line 57.
14:49:23 koha_1       | isa check for "ssl" failed: ssl is not a Boolean! at (eval 1410) line 315.
14:49:23 koha_1       | 	eval {...} called at (eval 1410) line 313
14:49:23 koha_1       | 	Email::Sender::Transport::SMTP::new("Email::Sender::Transport::SMTP", HASH(0xa0e7dd0)) called at /kohadevbox/koha/Koha/SMTP/Server.pm line 68
14:49:23 koha_1       | 	Koha::SMTP::Server::transport(Koha::SMTP::Server=HASH(0xa0941b0)) called at t/db_dependent/Koha/SMTP/Server.t line 51
14:49:23 koha_1       | 	main::__ANON__() called at /usr/share/perl5/Test/Builder.pm line 310
14:49:23 koha_1       | 	eval {...} called at /usr/share/perl5/Test/Builder.pm line 310
14:49:23 koha_1       | 	Test::Builder::subtest(Test::Builder=HASH(0x1176660), "transport() tests", CODE(0xfca908)) called at /usr/share/perl5/Test/More.pm line 807
14:49:23 koha_1       | 	Test::More::subtest("transport() tests", CODE(0xfca908)) called at t/db_dependent/Koha/SMTP/Server.t line 57
14:49:23 koha_1       | # Looks like your test exited with 255 just after 1.
14:49:23 koha_1       | [12:49:23] t/db_dependent/Koha/SMTP/Server.t

Maybe a version of the lib?
Comment 214 Mason James 2020-10-10 01:23:22 UTC
Created attachment 111442 [details] [review]
Bug 22343: Add configuration options for SMTP servers

fix SSL tests for u16, using older Email-Sender-1.300021
Comment 215 Tomás Cohen Arazi 2020-10-10 01:38:26 UTC
(In reply to Mason James from comment #214)
> Created attachment 111442 [details] [review] [review]
> Bug 22343: Add configuration options for SMTP servers
> 
> fix SSL tests for u16, using older Email-Sender-1.300021

We should note on the release notes that STARTTLS is not supported on Ubuntu 16. Just in case
Comment 216 Jonathan Druart 2020-10-12 08:31:00 UTC
Created attachment 111482 [details] [review]
Bug 22343: Make the tests pass on U16

fix SSL tests for u16, using older Email-Sender-1.300021
Comment 217 Jonathan Druart 2020-10-12 09:32:10 UTC
(In reply to Jonathan Druart from comment #216)
> Created attachment 111482 [details] [review] [review]
> Bug 22343: Make the tests pass on U16
> 
> fix SSL tests for u16, using older Email-Sender-1.300021

Pushed to master.
Comment 218 Lucas Gass 2020-10-20 18:12:43 UTC
new feature will not be backported to 20.05.x
Comment 219 Joonas Kylmälä 2020-11-06 05:56:08 UTC
This seems to cause incorrect double encoding in HOLD slip / pickup notifications. Working on a fix.
Comment 220 Joonas Kylmälä 2020-11-06 11:45:06 UTC
Created attachment 113240 [details] [review]
Bug 22343: (QA follow-up) Remove double encoding from sent emails

The sent emails from Koha were garbled / double encoded because
Koha::Email now after the refactoring and moving to use Email::Stuffer
handles the encoding itself. Previously the Koha::Email callee had to
handle the encoding. This patch removes the double encoding in the
email subject and content.

To test:
 1) Put some UTF-8 characters like ä in the HOLD template subject and body
 2) Create hold and check-in an item to make it Waiting and cause the
 email to be send
 3) Notice the email comes garbled / double encoded
 4) Apply patch
 5) Repeat step 2) and notice the email comes without any issues in
    encoding and look OK.
 6) Make sure "prove t/Koha/Email.t" and "prove
    t/db_dependent/Letters.t" and passes

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 221 Jonathan Druart 2020-11-06 11:50:02 UTC
(In reply to Joonas Kylmälä from comment #219)
> This seems to cause incorrect double encoding in HOLD slip / pickup
> notifications. Working on a fix.

Did you try with other notifications? When I pushed this I tested some of them and it was working correctly.

Maybe your patch will break other than HOLD.
Comment 222 Joonas Kylmälä 2020-11-06 11:51:32 UTC
(In reply to Jonathan Druart from comment #221)
> (In reply to Joonas Kylmälä from comment #219)
> > This seems to cause incorrect double encoding in HOLD slip / pickup
> > notifications. Working on a fix.
> 
> Did you try with other notifications? When I pushed this I tested some of
> them and it was working correctly.
> 
> Maybe your patch will break other than HOLD.

We tested ODUE and MEMBERSHIP_EXPIRY templates and this fixed those as well.

If you think more unit tests are needed for this could you please provide them, I'm unable to find the time right now to investigate what sort of tests are available for _send_message_by_email() and what changes would be needed there. If no one else steps in I could do it in few weeks time but I feel like this is important to get to the master version as soon as possible in case someone is using master branch in production :D
Comment 223 Tomás Cohen Arazi 2020-11-06 12:06:02 UTC
(In reply to Joonas Kylmälä from comment #222)
> (In reply to Jonathan Druart from comment #221)
> > (In reply to Joonas Kylmälä from comment #219)
> > > This seems to cause incorrect double encoding in HOLD slip / pickup
> > > notifications. Working on a fix.
> > 
> > Did you try with other notifications? When I pushed this I tested some of
> > them and it was working correctly.
> > 
> > Maybe your patch will break other than HOLD.
> 
> We tested ODUE and MEMBERSHIP_EXPIRY templates and this fixed those as well.
> 
> If you think more unit tests are needed for this could you please provide
> them, I'm unable to find the time right now to investigate what sort of
> tests are available for _send_message_by_email() and what changes would be
> needed there. If no one else steps in I could do it in few weeks time but I
> feel like this is important to get to the master version as soon as possible
> in case someone is using master branch in production :D

Hi, can we open a separate bug for this, as it is pushed already? Most tests are on t/db_dependent/Letters.t.

If it is a specific kind of message, then we could check on the caller's end. For references on how encoding is handled, refer to the Email::Stuffer and Email::MIME docs.
Comment 224 Joonas Kylmälä 2020-11-06 12:11:23 UTC
(In reply to Tomás Cohen Arazi from comment #223)
> Hi, can we open a separate bug for this, as it is pushed already? Most tests
> are on t/db_dependent/Letters.t.
> 
> If it is a specific kind of message, then we could check on the caller's
> end. For references on how encoding is handled, refer to the Email::Stuffer
> and Email::MIME docs.

Ok, I opened bug 26948 and also attached my patch there, will obsolete it from here.
Comment 225 Jonathan Druart 2020-11-19 16:15:48 UTC
Created attachment 113827 [details] [review]
Bug 22343: (follow-up) Correctly handle password on editing
Comment 226 Jonathan Druart 2020-11-19 16:16:14 UTC
(In reply to Jonathan Druart from comment #225)
> Created attachment 113827 [details] [review] [review]
> Bug 22343: (follow-up) Correctly handle password on editing

Tomas, can you sign this one off please?
Comment 227 Tomás Cohen Arazi 2020-11-19 19:01:47 UTC
Created attachment 113847 [details] [review]
Bug 22343: (follow-up) Correctly handle password on editing

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 228 Tomás Cohen Arazi 2020-11-19 19:02:37 UTC
(In reply to Jonathan Druart from comment #226)
> (In reply to Jonathan Druart from comment #225)
> > Created attachment 113827 [details] [review] [review] [review]
> > Bug 22343: (follow-up) Correctly handle password on editing
> 
> Tomas, can you sign this one off please?

Done! I've also proposed something else on bug 27060 for consideration. But it contains string changes so can wait.
Comment 229 Jonathan Druart 2020-11-20 07:19:13 UTC
(In reply to Tomás Cohen Arazi from comment #227)
> Created attachment 113847 [details] [review] [review]
> Bug 22343: (follow-up) Correctly handle password on editing
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Pushed to master for 20.11.
Comment 230 Andrew Fuerste-Henry 2020-12-09 21:53:01 UTC
New feature not backported to 20.05
Comment 231 Jonathan Druart 2021-02-16 13:48:42 UTC
*** Bug 22838 has been marked as a duplicate of this bug. ***
Comment 232 David Cook 2021-10-27 03:37:30 UTC
Looks like this change has introduced a regression where we aren't passing the content type to Koha::Email, which means we can't send emails with attachments anymore (multi-part/mixed emails).


@@ -1314,29 +1329,66 @@ sub _send_message_by_email {
                 ? ( bcc => C4::Context->preference('NoticeBcc') )
                 : ()
             ),
-            from    => $message->{'from_address'} || $branch_email,
-            replyto => $message->{'reply_address'} || $branch_replyto,
-            sender  => $branch_returnpath,
-            subject => $subject,
-            message => $is_html ? _wrap_html( $content, $subject ) : $content,
-            contenttype => $content_type
+            from     => $message->{'from_address'}  || $branch_email,
+            reply_to => $message->{'reply_address'} || $branch_replyto,
+            sender   => $branch_returnpath,
+            subject  => "" . $message->{subject}
Comment 233 Marcel de Rooy 2021-10-29 08:31:02 UTC
This Koha::Email revamp was pushed a bit too early, i am afraid.
Comment 234 Marcel de Rooy 2021-10-29 09:59:13 UTC
How did we handle bcc previously btw? Email::Simple cant or wont handle bcc, so we are just moving them to recipients now..
Comment 235 Katrin Fischer 2021-10-29 22:39:51 UTC
(In reply to Marcel de Rooy from comment #234)
> How did we handle bcc previously btw? Email::Simple cant or wont handle bcc,
> so we are just moving them to recipients now..

There was bug 26705 about BCC after this was pushed. Maybe the discussion there helps.
Comment 236 Marcel de Rooy 2021-11-01 10:12:01 UTC
(In reply to Marcel de Rooy from comment #233)
> This Koha::Email revamp was pushed a bit too early, i am afraid.

This really was an understatement.
Comment 237 Jonathan Druart 2022-10-24 12:24:39 UTC
+            value => { flags => 3**2 }    # parameters flag = 3

It must be 2**3 instead.