Bug 27424 - One should be able to assign an SMTP server as the default
Summary: One should be able to assign an SMTP server as the default
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Martin Renvoize
URL:
Keywords: rel_23_05_candidate
Depends on: 22343
Blocks: 34616 34622
  Show dependency treegraph
 
Reported: 2021-01-13 14:29 UTC by Martin Renvoize
Modified: 2023-09-08 09:41 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
We have been able to define SMTP servers in the staff interface for a while now. But to utilize them you had to set the SMTP server for each library individually. With this you can now chose to apply an SMTP server as default to all your libraries.
Version(s) released in:
23.05.02,23.05.00


Attachments
Bug 27424: Update schema file (1.40 KB, patch)
2022-01-20 19:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27424: Update schema file (1.40 KB, patch)
2022-01-20 19:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27424: Add ability to specify an SMTP server in the database as the default server (10.03 KB, patch)
2022-01-20 19:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27424: Update database (975 bytes, patch)
2022-01-25 19:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27424: Update schema file (1.57 KB, patch)
2022-02-04 17:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27424: Add ability to specify an SMTP server in the database as the default server (10.07 KB, patch)
2022-02-04 17:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27424: Update database (1.00 KB, patch)
2022-02-04 17:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27424: (QA follow-up) Make Koha::SMTP::Server->store deal with defaults (4.78 KB, patch)
2022-02-04 17:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27424: Clarify logic (1.57 KB, patch)
2022-02-04 17:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27424: Update schema file (1.64 KB, patch)
2022-02-08 15:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27424: Add ability to specify an SMTP server in the database as the default server (10.14 KB, patch)
2022-02-08 15:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27424: Update database (1.07 KB, patch)
2022-02-08 15:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27424: (QA follow-up) Make Koha::SMTP::Server->store deal with defaults (4.84 KB, patch)
2022-02-08 15:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27424: Clarify logic (1.64 KB, patch)
2022-02-08 15:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27424: Update schema file (1.64 KB, patch)
2023-01-26 18:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27424: Update schema file (1.64 KB, patch)
2023-01-26 18:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27424: Add ability to specify an SMTP server in the database as the default server (10.11 KB, patch)
2023-01-26 18:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27424: Update database (1.07 KB, patch)
2023-01-26 18:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27424: (QA follow-up) Make Koha::SMTP::Server->store deal with defaults (4.84 KB, patch)
2023-01-26 18:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27424: Clarify logic (1.64 KB, patch)
2023-01-26 18:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27424: (QA follow-up) Highlight the default row in the SMTP servers table (1.02 KB, patch)
2023-01-26 20:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27424: (QA follow-up) Show current default on the editor for non-default servers (2.10 KB, patch)
2023-01-27 11:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27424: (QA follow-up) Add confirmation if (un)checking the default smtp server checkbox (2.98 KB, patch)
2023-01-27 11:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27424: (QA follow-up) Add confirmation if (un)checking the default smtp server checkbox (2.95 KB, patch)
2023-01-27 12:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27424: (QA follow-up) Make db update idempotent (1009 bytes, patch)
2023-06-08 13:45 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2021-01-13 14:29:08 UTC
Bug 22343 adds the ability to defined SMTP servers via the UI; But to then utilise them you have to go to each individual library via the libraries admin area and select the SMTP server.

We should have a way to override the fallback/default SMTP server right from the SMTP servers administration page.. setting one of our defined SMTP Servers as the system default rather than using the hard coded fallback options.
Comment 1 Kyle M Hall 2022-01-20 19:28:40 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2022-01-20 19:29:00 UTC
Created attachment 129674 [details] [review]
Bug 27424: Update schema file
Comment 3 Kyle M Hall 2022-01-20 19:29:09 UTC
Created attachment 129675 [details] [review]
Bug 27424: Add ability to specify an SMTP server in the database as the default server

Bug 22343 adds the ability to defined SMTP servers via the UI; But to then utilise them you have to go to each individual library via the libraries admin area and select the SMTP server.

We should have a way to override the fallback/default SMTP server right from the SMTP servers administration page.. setting one of our defined SMTP Servers as the system default rather than using the hard coded fallback options.

Test Plan:
1) Apply this patch set
2) Restart all the things!
3) Browser to the SMTP servers editor,
   verify only one server can be set as the default server
4) Set a default server, verify that server was used to send email from
   a cronjob, AND/OR prove t/db_dependent/Koha/SMTP/Server.t
Comment 4 Owen Leonard 2022-01-25 15:49:41 UTC
Is this missing an atomicupdate file?

DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'me.is_default' in 'field list' at /kohadevbox/koha/Koha/Objects.pm line 317
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77
Comment 5 Kyle M Hall 2022-01-25 19:05:50 UTC
(In reply to Owen Leonard from comment #4)
> Is this missing an atomicupdate file?
> 
> DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st
> execute failed: Unknown column 'me.is_default' in 'field list' at
> /kohadevbox/koha/Koha/Objects.pm line 317
>  at /usr/share/perl5/DBIx/Class/Exception.pm line 77

Yep! I'll attach that missing atomic update right now!
Comment 6 Kyle M Hall 2022-01-25 19:06:56 UTC
Created attachment 129814 [details] [review]
Bug 27424: Update database
Comment 7 Tomás Cohen Arazi 2022-02-04 17:44:01 UTC
Created attachment 130168 [details] [review]
Bug 27424: Update schema file

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi 2022-02-04 17:44:06 UTC
Created attachment 130169 [details] [review]
Bug 27424: Add ability to specify an SMTP server in the database as the default server

Bug 22343 adds the ability to defined SMTP servers via the UI; But to then utilise them you have to go to each individual library via the libraries admin area and select the SMTP server.

We should have a way to override the fallback/default SMTP server right from the SMTP servers administration page.. setting one of our defined SMTP Servers as the system default rather than using the hard coded fallback options.

Test Plan:
1) Apply this patch set
2) Restart all the things!
3) Browser to the SMTP servers editor,
   verify only one server can be set as the default server
4) Set a default server, verify that server was used to send email from
   a cronjob, AND/OR prove t/db_dependent/Koha/SMTP/Server.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2022-02-04 17:44:11 UTC
Created attachment 130170 [details] [review]
Bug 27424: Update database

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2022-02-04 17:44:15 UTC
Created attachment 130171 [details] [review]
Bug 27424: (QA follow-up) Make Koha::SMTP::Server->store deal with defaults

This patch makes the ->store method take care of setting the server as
the only default one.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/SMTP/Server.t
=> SUCCESS: Tests pass!
3. The UI keeps working as it should.
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2022-02-04 17:44:19 UTC
Created attachment 130172 [details] [review]
Bug 27424: Clarify logic

This patch simplifies how Koha::SMTP::Servers->get_default is written,
to help readability.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2022-02-04 18:10:05 UTC
OK, works as expected. Moved the 'set default' logic to Koha::SMTP::Server->store proper and added tests. Also minor style tweaks.

Good job!
Comment 13 David Cook 2022-02-07 01:18:18 UTC
Glad to see this one!
Comment 14 Martin Renvoize 2022-02-08 15:52:52 UTC
Created attachment 130300 [details] [review]
Bug 27424: Update schema file

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2022-02-08 15:52:56 UTC
Created attachment 130301 [details] [review]
Bug 27424: Add ability to specify an SMTP server in the database as the default server

Bug 22343 adds the ability to defined SMTP servers via the UI; But to then utilise them you have to go to each individual library via the libraries admin area and select the SMTP server.

We should have a way to override the fallback/default SMTP server right from the SMTP servers administration page.. setting one of our defined SMTP Servers as the system default rather than using the hard coded fallback options.

Test Plan:
1) Apply this patch set
2) Restart all the things!
3) Browser to the SMTP servers editor,
   verify only one server can be set as the default server
4) Set a default server, verify that server was used to send email from
   a cronjob, AND/OR prove t/db_dependent/Koha/SMTP/Server.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2022-02-08 15:53:01 UTC
Created attachment 130302 [details] [review]
Bug 27424: Update database

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2022-02-08 15:53:06 UTC
Created attachment 130303 [details] [review]
Bug 27424: (QA follow-up) Make Koha::SMTP::Server->store deal with defaults

This patch makes the ->store method take care of setting the server as
the only default one.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/SMTP/Server.t
=> SUCCESS: Tests pass!
3. The UI keeps working as it should.
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2022-02-08 15:53:10 UTC
Created attachment 130304 [details] [review]
Bug 27424: Clarify logic

This patch simplifies how Koha::SMTP::Servers->get_default is written,
to help readability.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2022-02-08 15:53:49 UTC
Nice one!

Works as described, code is good and covered by tests.. QA script is happy.

Passing QA
Comment 20 Fridolin Somers 2022-02-10 08:25:28 UTC
Seeing both 'Default configuration' and 'Is default' looks very confusing.

Same for setting checkbox 'is default' when already defined.
Could we add a 'is default' checkbox in table ? with JavaScript to ensure only one is checked ?
Comment 21 Nick Clemens 2022-03-09 11:08:43 UTC
When editing:
Maybe just list the current default and note that it will be overridden if checked - list 'Koha configuration file' if none selected?

On 'STMP servers' screen:
Add the server name to 'Default configuration' as above, and highlight the row differently?

Would that work Frido?
Comment 22 Tomás Cohen Arazi 2022-03-09 11:10:47 UTC
(In reply to Nick Clemens from comment #21)
> When editing:
> Maybe just list the current default and note that it will be overridden if
> checked - list 'Koha configuration file' if none selected?

Maybe 'Fallback', specifying if it is hardcoded or from koha-conf.xml?
Comment 23 Fridolin Somers 2022-03-10 04:32:38 UTC
(In reply to Nick Clemens from comment #21)
> When editing:
> Maybe just list the current default and note that it will be overridden if
> checked - list 'Koha configuration file' if none selected?
> 
> On 'STMP servers' screen:
> Add the server name to 'Default configuration' as above, and highlight the
> row differently?
> 
> Would that work Frido?

Yep great :)
Comment 24 Liz Rea 2023-01-24 17:27:24 UTC
Hi, this would be SO helpful to have and I think we've solved the discussion items :D

Cheers,
Liz
Comment 25 Martin Renvoize 2023-01-25 13:58:32 UTC
I imagine this needs a little rebase.. is this something you might have time to do Tomas?
Comment 26 Kyle M Hall 2023-01-26 18:43:45 UTC
Created attachment 145697 [details] [review]
Bug 27424: Update schema file

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Kyle M Hall 2023-01-26 18:44:45 UTC
Created attachment 145698 [details] [review]
Bug 27424: Update schema file

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Kyle M Hall 2023-01-26 18:44:56 UTC
Created attachment 145699 [details] [review]
Bug 27424: Add ability to specify an SMTP server in the database as the default server

Bug 22343 adds the ability to defined SMTP servers via the UI; But to then utilise them you have to go to each individual library via the libraries admin area and select the SMTP server.

We should have a way to override the fallback/default SMTP server right from the SMTP servers administration page.. setting one of our defined SMTP Servers as the system default rather than using the hard coded fallback options.

Test Plan:
1) Apply this patch set
2) Restart all the things!
3) Browser to the SMTP servers editor,
   verify only one server can be set as the default server
4) Set a default server, verify that server was used to send email from
   a cronjob, AND/OR prove t/db_dependent/Koha/SMTP/Server.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Kyle M Hall 2023-01-26 18:45:01 UTC
Created attachment 145700 [details] [review]
Bug 27424: Update database

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Kyle M Hall 2023-01-26 18:45:07 UTC
Created attachment 145701 [details] [review]
Bug 27424: (QA follow-up) Make Koha::SMTP::Server->store deal with defaults

This patch makes the ->store method take care of setting the server as
the only default one.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/SMTP/Server.t
=> SUCCESS: Tests pass!
3. The UI keeps working as it should.
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Kyle M Hall 2023-01-26 18:45:12 UTC
Created attachment 145702 [details] [review]
Bug 27424: Clarify logic

This patch simplifies how Koha::SMTP::Servers->get_default is written,
to help readability.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Kyle M Hall 2023-01-26 20:02:27 UTC
Created attachment 145705 [details] [review]
Bug 27424: (QA follow-up) Highlight the default row in the SMTP servers table
Comment 33 Kyle M Hall 2023-01-27 11:57:04 UTC
Created attachment 145723 [details] [review]
Bug 27424: (QA follow-up) Show current default on the editor for non-default servers
Comment 34 Kyle M Hall 2023-01-27 11:57:17 UTC
Created attachment 145724 [details] [review]
Bug 27424: (QA follow-up) Add confirmation if (un)checking the default smtp server checkbox
Comment 35 Kyle M Hall 2023-01-27 12:00:03 UTC
Created attachment 145725 [details] [review]
Bug 27424: (QA follow-up) Add confirmation if (un)checking the default smtp server checkbox
Comment 36 Tomás Cohen Arazi 2023-01-31 13:57:37 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 37 Jonathan Druart 2023-06-07 11:50:19 UTC
We are missing a test on the existence of the column.

The DB rev will fail if executed twice.
Comment 38 Kyle M Hall 2023-06-08 13:43:53 UTC
(In reply to Jonathan Druart from comment #37)
> We are missing a test on the existence of the column.
> 
> The DB rev will fail if executed twice.

will fix
Comment 39 Kyle M Hall 2023-06-08 13:45:32 UTC
Created attachment 152192 [details] [review]
Bug 27424: (QA follow-up) Make db update idempotent
Comment 40 Tomás Cohen Arazi 2023-06-09 11:51:29 UTC
Follow-up pushed.
Comment 41 Jonathan Druart 2023-06-09 11:57:23 UTC
Last patch needed for 23.05
Comment 42 Martin Renvoize 2023-07-12 07:01:29 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 43 Matt Blenkinsop 2023-09-08 09:41:49 UTC
Enhancement - not backporting to 22.11.x

Nice work everyone!