Bug 22343 - Add SMTP configuration options to Administration
Summary: Add SMTP configuration options to Administration
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords: dependency
Depends on:
Blocks: 26188 26290
  Show dependency treegraph
 
Reported: 2019-02-15 01:05 UTC by Father Vlasie
Modified: 2020-09-17 09:35 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


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

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..