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.
Created attachment 129673 [details] [review] Bug 27424: Update schema file
Created attachment 129674 [details] [review] Bug 27424: Update schema file
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
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
(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!
Created attachment 129814 [details] [review] Bug 27424: Update database
Created attachment 130168 [details] [review] Bug 27424: Update schema file Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
Created attachment 130170 [details] [review] Bug 27424: Update database Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
OK, works as expected. Moved the 'set default' logic to Koha::SMTP::Server->store proper and added tests. Also minor style tweaks. Good job!
Glad to see this one!
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>
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>
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>
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>
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>
Nice one! Works as described, code is good and covered by tests.. QA script is happy. Passing QA
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 ?
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?
(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?
(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 :)
Hi, this would be SO helpful to have and I think we've solved the discussion items :D Cheers, Liz
I imagine this needs a little rebase.. is this something you might have time to do Tomas?
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>
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>
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>
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>
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>
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>
Created attachment 145705 [details] [review] Bug 27424: (QA follow-up) Highlight the default row in the SMTP servers table
Created attachment 145723 [details] [review] Bug 27424: (QA follow-up) Show current default on the editor for non-default servers
Created attachment 145724 [details] [review] Bug 27424: (QA follow-up) Add confirmation if (un)checking the default smtp server checkbox
Created attachment 145725 [details] [review] Bug 27424: (QA follow-up) Add confirmation if (un)checking the default smtp server checkbox
Pushed to master for 23.05. Nice work everyone, thanks!
We are missing a test on the existence of the column. The DB rev will fail if executed twice.
(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
Created attachment 152192 [details] [review] Bug 27424: (QA follow-up) Make db update idempotent
Follow-up pushed.
Last patch needed for 23.05
Thanks for all the hard work! Pushed to 23.05.x for the next release
Enhancement - not backporting to 22.11.x Nice work everyone!