Bug 22343

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.

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.

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

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!

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

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

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

(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)

(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

Is the use of these configuration options optional? Wondering if existing setups will still keep working on update.

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

So if nothing is set, the MTA settings will be used. Thx!

(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

Ok, this is now complete (i.e. all places that send emails use the new feature). Go test!

You need to install Email::Stuffer:

  $ sudo apt install libemail-stuffer-perl

Code reads well and is a big improvement. Tested various mail triggers with various configurations and all worked as expected.

Signing off.

I just want to say thank you for this much needed feature with especial thanks to Tomás for doing the hard work!

Kudos to Tomas! I'm glad that my comment from last month was able to re-awaken this bug report!

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

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

Thanks for your time, and the follow-up, Kyle!

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.

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

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

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.

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

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

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

Why using warnings_exist instead of warning_is in the tests?

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.

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.

Created attachment 109157 [details] [review]
Bug 22343: Add new tables schema file

Created attachment 109158 [details] [review]
Bug 22343: DBIx::Class schema updates [DO NOT PUSH]

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

Created attachment 109160 [details] [review]
Bug 22343: Add Koha::Library->smtp_server

Created attachment 109161 [details] [review]
Bug 22343: Update cpanfile

Created attachment 109162 [details] [review]
Bug 22343: API routes for SMTP servers CRUD

This patch introduces API routes for handling SMTP servers.

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

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

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

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.

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.

Created attachment 109168 [details] [review]
Bug 22343: Adapt runreport.pl

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

Created attachment 109169 [details] [review]
Bug 22343: Prevent passing password to the UI

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

(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

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

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>

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>

Created attachment 109603 [details] [review]
Bug 22343: Add new tables schema file

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 109604 [details] [review]
Bug 22343: DBIx::Class schema updates [DO NOT PUSH]

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

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>

Created attachment 109606 [details] [review]
Bug 22343: Add Koha::Library->smtp_server

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 109607 [details] [review]
Bug 22343: Update cpanfile

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

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>

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>

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>

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>

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>

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>

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>

Created attachment 109615 [details] [review]
Bug 22343: Prevent passing password to the UI

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

Created attachment 109740 [details] [review]
Bug 22343: (QA follow-up) Fix some comments

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

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.

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!

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.

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.

Seems I forgot to set PQA on this..

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

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

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>

Created attachment 111087 [details] [review]
Bug 22343: Add warning to the about page if KohaAdminEmailAddress is invalid

Created attachment 111088 [details] [review]
Bug 22343: Correctly handle password on editing

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)?

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

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)

Created attachment 111090 [details] [review]
Bug 22343: Add the 'required' class when needed

232                     <th data-class-name="actions">Actions</th>

Where is this data-class-name coming from? How it is useful?

(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

(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?

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

There is an encoding issue with runreport.pl, can you double check please?

Created attachment 111154 [details] [review]
Bug 22343: Fix runreport encoding issues

Pushed to master for 20.11, thanks to everybody involved!

t/db_dependent/Passwordrecovery.t is failing, please fix ASAP

Created attachment 111165 [details] [review]
Bug 22343: Compiled CSS

Created attachment 111166 [details] [review]
Bug 22343: Add exec flag on .t files

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

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>

Created attachment 111176 [details] [review]
Bug 22343: (follow-up) Remove traces of Mail::Sendmail

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 111213 [details] [review]
Bug 22343: Add exec flag on .t files

Created attachment 111215 [details] [review]
Bug 22343: (follow-up) Remove traces of Mail::Sendmail - update POD

Created attachment 111216 [details] [review]
Bug 22343: Remove traces of libmail-sendmail-perl

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

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

(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?

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

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?

Created attachment 111442 [details] [review]
Bug 22343: Add configuration options for SMTP servers

fix SSL tests for u16, using older Email-Sender-1.300021

(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

Created attachment 111482 [details] [review]
Bug 22343: Make the tests pass on U16

fix SSL tests for u16, using older Email-Sender-1.300021

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

new feature will not be backported to 20.05.x

This seems to cause incorrect double encoding in HOLD slip / pickup notifications. Working on a fix.

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>

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

(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

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

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

Created attachment 113827 [details] [review]
Bug 22343: (follow-up) Correctly handle password on editing

(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?

Created attachment 113847 [details] [review]
Bug 22343: (follow-up) Correctly handle password on editing

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

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

New feature not backported to 20.05

*** Bug 22838 has been marked as a duplicate of this bug. ***

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}

This Koha::Email revamp was pushed a bit too early, i am afraid.

How did we handle bcc previously btw? Email::Simple cant or wont handle bcc, so we are just moving them to recipients now..

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

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

+            value => { flags => 3**2 }    # parameters flag = 3

It must be 2**3 instead.