See 33360
Created attachment 153351 [details] [review] Bug 33537: Add better named table_exists alias in Installer module Test plan: Run dbrev later. It is using table_exists. Run t/db_dependent/Installer.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153352 [details] [review] Bug 33537: Fix some POD in Installer.pm Test plan: Run qa tools. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153353 [details] [review] Bug 33537: Database revision, adding table mail_domain_limits Test plan: Run dbrev or new install. Verify that table is created as expected. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153354 [details] [review] Bug 33537: [DBIx] Schema changes for mail_domain_limits Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153355 [details] [review] Bug 33537: Add undefined default value for belongs_to in TestBuilder Test plan: See next patch. It calls build_object without specifying belongs_to. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153356 [details] [review] Bug 33537: Add Koha objects Test plan: Run t/db_dependent/Koha/MailDomainLimits.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153357 [details] [review] Bug 33537: REST API endpoints Test plan: Run t/db_dependent/api/v1/domain_limits.t List, add (post), get, update (put) and delete domain limit(s). Using the paths: - api/v1/smtp/domain_limits - api/v1/smtp/domain_limits/{SOME_ID} Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153358 [details] [review] Bug 33537: Add staff template and script Test plan: Go to Administration/SMTP Servers. Click Domain limits. Do some CRUD operations. Bonus: Add another tab with domain limits. Edit or remove records on tab 1. Try CRUD on changed or removed records on tab 2. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153359 [details] [review] Bug 33537: Connect the dots Use Koha::MailDomainLimits in Koha::Notice::Util->load_domain_limits. Only use koha-conf as fallback when table is still empty. (This is just like SMTP servers.) The test is adjusted accordingly and has moved more to the database oriented side, first proving that both approaches return the same. Test plan: Run t/db_dependent/Koha/Notice_Util.t NOTE: Replace A and B hereunder by two domains of your own choice. Add a limit to KOHA-CONF: domain A, 1 per 1 hour. Add another limit in WEB CONFIG: domain B, 1 per 1 hour Send 2 notices to domain B. Is 1 message delayed? Remove the table entry for B. Run process_message_queue.pl. Is the delayed notice to B sent? Send 2 notices to domain A. Is 1 delayed (using koha-conf fallback)? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Note for testing and QA: Tried to keep the patches clean and clear here. Patch 3 and 4 are database oriented, introducing the new table mail_domain_limits. Patch 6 is the Koha object update. It adds a search_with_group_domain that includes a self join on the new table following the 'foreign' key to itself for grouping domains. Patch 7 is all regular REST API stuff. It uses the search_with_group_domain routine too. The render_helper routine was added to reduce the number of lines here. Note that I preferred smtp/ over config/smtp/ here. Patch 8 is the main interface work. The perl script is short and simple. The template is small too; I moved all js to a separate file. The js includes REST API calls for all CRUD operations. Patch 9 is the switch from koha-conf to table in Koha::Notice::Util in order to get all things working. A test plan per patch is provided. But things can be tested along those lines also with all patches applied.
Created attachment 153361 [details] [review] Bug 33537: Add Koha objects Test plan: Run t/db_dependent/Koha/MailDomainLimits.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Run perltidy on MailDomainLimits.t
Created attachment 153362 [details] [review] Bug 33537: REST API endpoints Test plan: Run t/db_dependent/api/v1/domain_limits.t List, add (post), get, update (put) and delete domain limit(s). Using the paths: - api/v1/smtp/domain_limits - api/v1/smtp/domain_limits/{SOME_ID} Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 153363 [details] [review] Bug 33537: Add staff template and script Test plan: Go to Administration/SMTP Servers. Click Domain limits. Do some CRUD operations. Bonus: Add another tab with domain limits. Edit or remove records on tab 1. Try CRUD on changed or removed records on tab 2. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Added some preventDefault calls, as suggested by JD.
Created attachment 153364 [details] [review] Bug 33537: Connect the dots Use Koha::MailDomainLimits in Koha::Notice::Util->load_domain_limits. Only use koha-conf as fallback when table is still empty. (This is just like SMTP servers.) The test is adjusted accordingly and has moved more to the database oriented side, first proving that both approaches return the same. Test plan: Run t/db_dependent/Koha/Notice_Util.t NOTE: Replace A and B hereunder by two domains of your own choice. Add a limit to KOHA-CONF: domain A, 1 per 1 hour. Add another limit in WEB CONFIG: domain B, 1 per 1 hour Send 2 notices to domain B. Is 1 message delayed? Remove the table entry for B. Run process_message_queue.pl. Is the delayed notice to B sent? Send 2 notices to domain A. Is 1 delayed (using koha-conf fallback)? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Note for QA: I spent a lot of time on this one. It passed qa tools at the time. And now out of nowhere, qa tools has been changed quite a bit. And I am seeing warns about REST API validation and perl tidy. This was written before the cutoff date !
Please wait, rebasing time
Note for QA: FAIL Koha/Schema/Result/MailDomainLimit.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 35) Please ignore this qa tools warning. This file is generated automatically !
Will still have a look at this, aaarggg : WARN C4/Installer.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 170, now: 171) FAIL Koha/Exceptions/DomainLimit.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 4) WARN Koha/MailDomainLimit.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 1) WARN Koha/Notice/Util.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 26, now: 31) FAIL Koha/REST/V1/Config/SMTP/DomainLimits.pm FAIL forbidden patterns forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 57) forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 75) forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 103) forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 106) forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 133) WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 32) WARN admin/domain_limits.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 1) WARN installer/data/mysql/atomicupdate/bug33537.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 5) WARN t/db_dependent/Installer.t WARN tidiness The file is less tidy than before (bad/messy lines before: 18, now: 19) WARN t/db_dependent/Koha/MailDomainLimits.t WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 3) WARN t/db_dependent/Koha/Notice_Util.t WARN tidiness The file is less tidy than before (bad/messy lines before: 37, now: 38) WARN t/db_dependent/api/v1/domain_limits.t WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 7) WARN t/lib/TestBuilder.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 211, now: 212)
QA: Please ignore this one too. In the Exception modules we dont have subroutines or methods to cover. The message is misleading. FAIL Koha/Exceptions/DomainLimit.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
Created attachment 154769 [details] [review] Bug 33537: Add better named table_exists alias in Installer module Test plan: Run dbrev later. It is using table_exists. Run t/db_dependent/Installer.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 154770 [details] [review] Bug 33537: Fix some POD in Installer.pm Test plan: Run qa tools. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 154771 [details] [review] Bug 33537: Database revision, adding table mail_domain_limits Test plan: Run dbrev or new install. Verify that table is created as expected. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 154772 [details] [review] Bug 33537: [DBIx] Schema changes for mail_domain_limits Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 154773 [details] [review] Bug 33537: Add undefined default value for belongs_to in TestBuilder Test plan: See next patch. It calls build_object without specifying belongs_to. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 154774 [details] [review] Bug 33537: Add Koha objects Test plan: Run t/db_dependent/Koha/MailDomainLimits.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Run perltidy on MailDomainLimits.t
Created attachment 154775 [details] [review] Bug 33537: REST API endpoints Test plan: Run t/db_dependent/api/v1/domain_limits.t List, add (post), get, update (put) and delete domain limit(s). Using the paths: - api/v1/smtp/domain_limits - api/v1/smtp/domain_limits/{SOME_ID} Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 154776 [details] [review] Bug 33537: Add staff template and script Test plan: Go to Administration/SMTP Servers. Click Domain limits. Do some CRUD operations. Bonus: Add another tab with domain limits. Edit or remove records on tab 1. Try CRUD on changed or removed records on tab 2. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Added some preventDefault calls, as suggested by JD.
Created attachment 154777 [details] [review] Bug 33537: Connect the dots Use Koha::MailDomainLimits in Koha::Notice::Util->load_domain_limits. Only use koha-conf as fallback when table is still empty. (This is just like SMTP servers.) The test is adjusted accordingly and has moved more to the database oriented side, first proving that both approaches return the same. Test plan: Run t/db_dependent/Koha/Notice_Util.t NOTE: Replace A and B hereunder by two domains of your own choice. Add a limit to KOHA-CONF: domain A, 1 per 1 hour. Add another limit in WEB CONFIG: domain B, 1 per 1 hour Send 2 notices to domain B. Is 1 message delayed? Remove the table entry for B. Run process_message_queue.pl. Is the delayed notice to B sent? Send 2 notices to domain A. Is 1 delayed (using koha-conf fallback)? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 154778 [details] [review] Bug 33537: (QA follow-up) Tidy updated blocks in various files Resolving: WARN C4/Installer.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 170, now: 171) FAIL Koha/Exceptions/DomainLimit.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 4) WARN Koha/MailDomainLimit.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 1) WARN Koha/Notice/Util.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 26, now: 31) WARN admin/domain_limits.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 1) WARN installer/data/mysql/atomicupdate/bug33537.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 5) WARN t/db_dependent/Installer.t WARN tidiness The file is less tidy than before (bad/messy lines before: 18, now: 19) WARN t/db_dependent/Koha/MailDomainLimits.t WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 3) WARN t/db_dependent/Koha/Notice_Util.t WARN tidiness The file is less tidy than before (bad/messy lines before: 37, now: 38) WARN t/db_dependent/api/v1/domain_limits.t WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 7) WARN t/lib/TestBuilder.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 211, now: 212) FAIL Koha/REST/V1/Config/SMTP/DomainLimits.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 32) Test plan: Run qa tools. Note: Forbidden pattern warns in Koha/REST/V1/Config/SMTP/DomainLimits.pm are handled in next patch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 154779 [details] [review] Bug 33537: (QA follow-up) Resolve forbidden pattern in REST API Resolving: FAIL Koha/REST/V1/Config/SMTP/DomainLimits.pm FAIL forbidden patterns forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 57) forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 75) forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 103) forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 106) forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 133) Test plan: Run t/db_dependent/api/v1/domain_limits.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Rebased (tiny change), perl tidied, applied changes for bug 33556. Note the two important comments about a few qa tools warns that can be ignored. Rest is fine now: Processing files before patches |========================>| 21 / 21 (100.00%) Processing files after patches |========================>| 21 / 21 (100.00%) OK C4/Installer.pm OK critic OK forbidden patterns OK git manipulation OK pod OK pod coverage SKIP spelling OK tidiness OK valid FAIL Koha/Exceptions/DomainLimit.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 SKIP spelling OK tidiness OK valid OK Koha/MailDomainLimit.pm OK critic OK forbidden patterns OK git manipulation OK pod OK pod coverage SKIP spelling OK tidiness OK valid OK Koha/MailDomainLimits.pm OK critic OK file permissions OK forbidden patterns OK git manipulation OK pod OK pod coverage SKIP spelling OK tidiness OK valid OK Koha/Notice/Util.pm OK critic OK forbidden patterns OK git manipulation OK pod OK pod coverage SKIP spelling OK tidiness OK valid OK Koha/REST/V1/Config/SMTP/DomainLimits.pm OK critic OK forbidden patterns OK git manipulation OK pod OK pod coverage SKIP spelling OK tidiness OK valid FAIL Koha/Schema/Result/MailDomainLimit.pm OK critic OK file permissions OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 SKIP spelling WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 35) OK valid OK admin/domain_limits.pl OK critic OK forbidden patterns OK git manipulation OK pod SKIP spelling OK tidiness OK valid OK api/v1/swagger/definitions/smtp_domain_limits.yaml OK git manipulation OK rest_api_specs OK yaml_valid OK api/v1/swagger/paths/smtp_domain_limits.yaml OK git manipulation OK rest_api_specs OK yaml_valid OK api/v1/swagger/swagger.yaml OK git manipulation OK rest_api_specs OK yaml_valid OK installer/data/mysql/atomicupdate/bug33537.pl OK critic OK forbidden patterns OK git manipulation OK pod SKIP spelling OK tidiness OK valid OK installer/data/mysql/kohastructure.sql OK boolean_vs_tinyint OK charset_collate OK git manipulation OK tinyint_has_boolean_flag OK koha-tmpl/intranet-tmpl/prog/en/modules/admin/domain_limits.tt OK filters OK forbidden patterns OK git manipulation OK js_in_body SKIP spelling OK tt_valid OK valid_template OK koha-tmpl/intranet-tmpl/prog/en/modules/admin/smtp_servers.tt OK filters OK forbidden patterns OK git manipulation OK js_in_body SKIP spelling OK tt_valid OK valid_template OK koha-tmpl/intranet-tmpl/prog/js/domain_limits.js OK git manipulation OK t/db_dependent/Installer.t OK critic OK forbidden patterns OK git manipulation OK pod SKIP spelling OK tidiness OK valid OK t/db_dependent/Koha/MailDomainLimits.t OK critic OK forbidden patterns OK git manipulation OK pod SKIP spelling OK tidiness OK valid OK t/db_dependent/Koha/Notice_Util.t OK critic OK forbidden patterns OK git manipulation OK pod SKIP spelling OK tidiness OK valid OK t/db_dependent/api/v1/domain_limits.t OK critic OK forbidden patterns OK git manipulation OK pod SKIP spelling OK tidiness OK valid OK t/lib/TestBuilder.pm OK critic OK forbidden patterns OK git manipulation OK pod OK pod coverage SKIP spelling OK tidiness OK valid
Created attachment 154818 [details] [review] Bug 33537: Add better named table_exists alias in Installer module Test plan: Run dbrev later. It is using table_exists. Run t/db_dependent/Installer.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 154819 [details] [review] Bug 33537: Fix some POD in Installer.pm Test plan: Run qa tools. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 154820 [details] [review] Bug 33537: Database revision, adding table mail_domain_limits Test plan: Run dbrev or new install. Verify that table is created as expected. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 154821 [details] [review] Bug 33537: [DBIx] Schema changes for mail_domain_limits Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 154822 [details] [review] Bug 33537: Add undefined default value for belongs_to in TestBuilder Test plan: See next patch. It calls build_object without specifying belongs_to. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 154823 [details] [review] Bug 33537: Add Koha objects Test plan: Run t/db_dependent/Koha/MailDomainLimits.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Run perltidy on MailDomainLimits.t Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 154824 [details] [review] Bug 33537: REST API endpoints Test plan: Run t/db_dependent/api/v1/domain_limits.t List, add (post), get, update (put) and delete domain limit(s). Using the paths: - api/v1/smtp/domain_limits - api/v1/smtp/domain_limits/{SOME_ID} Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 154825 [details] [review] Bug 33537: Add staff template and script Test plan: Go to Administration/SMTP Servers. Click Domain limits. Do some CRUD operations. Bonus: Add another tab with domain limits. Edit or remove records on tab 1. Try CRUD on changed or removed records on tab 2. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Added some preventDefault calls, as suggested by JD. Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 154826 [details] [review] Bug 33537: Connect the dots Use Koha::MailDomainLimits in Koha::Notice::Util->load_domain_limits. Only use koha-conf as fallback when table is still empty. (This is just like SMTP servers.) The test is adjusted accordingly and has moved more to the database oriented side, first proving that both approaches return the same. Test plan: Run t/db_dependent/Koha/Notice_Util.t NOTE: Replace A and B hereunder by two domains of your own choice. Add a limit to KOHA-CONF: domain A, 1 per 1 hour. Add another limit in WEB CONFIG: domain B, 1 per 1 hour Send 2 notices to domain B. Is 1 message delayed? Remove the table entry for B. Run process_message_queue.pl. Is the delayed notice to B sent? Send 2 notices to domain A. Is 1 delayed (using koha-conf fallback)? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 154827 [details] [review] Bug 33537: (QA follow-up) Tidy updated blocks in various files Resolving: WARN C4/Installer.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 170, now: 171) FAIL Koha/Exceptions/DomainLimit.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 4) WARN Koha/MailDomainLimit.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 1) WARN Koha/Notice/Util.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 26, now: 31) WARN admin/domain_limits.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 1) WARN installer/data/mysql/atomicupdate/bug33537.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 5) WARN t/db_dependent/Installer.t WARN tidiness The file is less tidy than before (bad/messy lines before: 18, now: 19) WARN t/db_dependent/Koha/MailDomainLimits.t WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 3) WARN t/db_dependent/Koha/Notice_Util.t WARN tidiness The file is less tidy than before (bad/messy lines before: 37, now: 38) WARN t/db_dependent/api/v1/domain_limits.t WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 7) WARN t/lib/TestBuilder.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 211, now: 212) FAIL Koha/REST/V1/Config/SMTP/DomainLimits.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 32) Test plan: Run qa tools. Note: Forbidden pattern warns in Koha/REST/V1/Config/SMTP/DomainLimits.pm are handled in next patch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 154828 [details] [review] Bug 33537: (QA follow-up) Resolve forbidden pattern in REST API Resolving: FAIL Koha/REST/V1/Config/SMTP/DomainLimits.pm FAIL forbidden patterns forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 57) forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 75) forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 103) forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 106) forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 133) Test plan: Run t/db_dependent/api/v1/domain_limits.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 154829 [details] [review] Bug 33537: (QA follow-up) Modal UI fixes: Use ol instead of ul in modal fieldset inputs: This ol->ul change makes the UI inputs more spaced and overall more in line with other modals in Koha. Use 'btn-primary' on 'Save' button to have it be shown yellow as other confirmation buttons in Koha. Add a11y attributes to modal form Also prettied HTML code: git show --ignore-all-space <commit_hash> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 154830 [details] [review] Bug 33537: (QA follow-up) Use bootstrap modal native functions Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 154831 [details] [review] Bug 33537: (QA follow-up) Update kohastructure.sql to match atomic update script Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 154832 [details] [review] Bug 33537: (QA follow-up) Add 'SMTP Servers' button to domain limits page Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 154833 [details] [review] Bug 33537: (QA follow-up) Improve process_message_queue warning message Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Marcel, please take a look at my patches for SO, or mark obsolete if you don't agree! For reference, here is what I did to follow the test plan: 1) configured <message_domain_limits> in koha-conf.xml 2) configured an smtp server with my own google account app password 3) did an OPAC password reset which prompted an entry in the message_queue table 4) duplicated that message entry a couple times: INSERT INTO message_queue (letter_id, borrowernumber, subject, content, letter_code, message_transport_type, status, to_address) SELECT letter_id, borrowernumber, subject, content, letter_code, message_transport_type, 'pending', 'pedro.amorim@ptfs-europe.com' FROM message_queue; 5) Run: perl misc/cronjobs/process_message_queue.pl
(In reply to Pedro Amorim from comment #48) > Marcel, please take a look at my patches for SO, or mark obsolete if you > don't agree! Thx Pedro. I certainly will. But it sounds very promising!
Created attachment 154869 [details] [review] Bug 33537: (QA follow-up) Modal UI fixes: Use ol instead of ul in modal fieldset inputs: This ol->ul change makes the UI inputs more spaced and overall more in line with other modals in Koha. Use 'btn-primary' on 'Save' button to have it be shown yellow as other confirmation buttons in Koha. Add a11y attributes to modal form Also prettied HTML code: git show --ignore-all-space <commit_hash> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 154870 [details] [review] Bug 33537: (QA follow-up) Use bootstrap modal native functions Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 154871 [details] [review] Bug 33537: (QA follow-up) Update kohastructure.sql to match atomic update script Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 154872 [details] [review] Bug 33537: (QA follow-up) Add 'SMTP Servers' button to domain limits page Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 154873 [details] [review] Bug 33537: (QA follow-up) Improve process_message_queue warning message Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #49) > (In reply to Pedro Amorim from comment #48) > > Marcel, please take a look at my patches for SO, or mark obsolete if you > > don't agree! > > Thx Pedro. I certainly will. But it sounds very promising! Looks good to me. Thanks!
Created attachment 157017 [details] [review] Bug 33537: Add better named table_exists alias in Installer module Test plan: Run dbrev later. It is using table_exists. Run t/db_dependent/Installer.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 157018 [details] [review] Bug 33537: Fix some POD in Installer.pm Test plan: Run qa tools. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 157019 [details] [review] Bug 33537: Database revision, adding table mail_domain_limits Test plan: Run dbrev or new install. Verify that table is created as expected. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 157020 [details] [review] Bug 33537: [DBIx] Schema changes for mail_domain_limits Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 157021 [details] [review] Bug 33537: Add undefined default value for belongs_to in TestBuilder Test plan: See next patch. It calls build_object without specifying belongs_to. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 157022 [details] [review] Bug 33537: Add Koha objects Test plan: Run t/db_dependent/Koha/MailDomainLimits.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Run perltidy on MailDomainLimits.t Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 157023 [details] [review] Bug 33537: REST API endpoints Test plan: Run t/db_dependent/api/v1/domain_limits.t List, add (post), get, update (put) and delete domain limit(s). Using the paths: - api/v1/smtp/domain_limits - api/v1/smtp/domain_limits/{SOME_ID} Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 157024 [details] [review] Bug 33537: Add staff template and script Test plan: Go to Administration/SMTP Servers. Click Domain limits. Do some CRUD operations. Bonus: Add another tab with domain limits. Edit or remove records on tab 1. Try CRUD on changed or removed records on tab 2. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Added some preventDefault calls, as suggested by JD. Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 157025 [details] [review] Bug 33537: Connect the dots Use Koha::MailDomainLimits in Koha::Notice::Util->load_domain_limits. Only use koha-conf as fallback when table is still empty. (This is just like SMTP servers.) The test is adjusted accordingly and has moved more to the database oriented side, first proving that both approaches return the same. Test plan: Run t/db_dependent/Koha/Notice_Util.t NOTE: Replace A and B hereunder by two domains of your own choice. Add a limit to KOHA-CONF: domain A, 1 per 1 hour. Add another limit in WEB CONFIG: domain B, 1 per 1 hour Send 2 notices to domain B. Is 1 message delayed? Remove the table entry for B. Run process_message_queue.pl. Is the delayed notice to B sent? Send 2 notices to domain A. Is 1 delayed (using koha-conf fallback)? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 157026 [details] [review] Bug 33537: (QA follow-up) Tidy updated blocks in various files Resolving: WARN C4/Installer.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 170, now: 171) FAIL Koha/Exceptions/DomainLimit.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 4) WARN Koha/MailDomainLimit.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 1) WARN Koha/Notice/Util.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 26, now: 31) WARN admin/domain_limits.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 1) WARN installer/data/mysql/atomicupdate/bug33537.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 5) WARN t/db_dependent/Installer.t WARN tidiness The file is less tidy than before (bad/messy lines before: 18, now: 19) WARN t/db_dependent/Koha/MailDomainLimits.t WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 3) WARN t/db_dependent/Koha/Notice_Util.t WARN tidiness The file is less tidy than before (bad/messy lines before: 37, now: 38) WARN t/db_dependent/api/v1/domain_limits.t WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 7) WARN t/lib/TestBuilder.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 211, now: 212) FAIL Koha/REST/V1/Config/SMTP/DomainLimits.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 32) Test plan: Run qa tools. Note: Forbidden pattern warns in Koha/REST/V1/Config/SMTP/DomainLimits.pm are handled in next patch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 157027 [details] [review] Bug 33537: (QA follow-up) Resolve forbidden pattern in REST API Resolving: FAIL Koha/REST/V1/Config/SMTP/DomainLimits.pm FAIL forbidden patterns forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 57) forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 75) forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 103) forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 106) forbidden pattern: Use of $c->validation in API controllers should be avoided (bug 33556) (line 133) Test plan: Run t/db_dependent/api/v1/domain_limits.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 157028 [details] [review] Bug 33537: (QA follow-up) Modal UI fixes: Use ol instead of ul in modal fieldset inputs: This ol->ul change makes the UI inputs more spaced and overall more in line with other modals in Koha. Use 'btn-primary' on 'Save' button to have it be shown yellow as other confirmation buttons in Koha. Add a11y attributes to modal form Also prettied HTML code: git show --ignore-all-space <commit_hash> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157029 [details] [review] Bug 33537: (QA follow-up) Use bootstrap modal native functions Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157030 [details] [review] Bug 33537: (QA follow-up) Update kohastructure.sql to match atomic update script Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157031 [details] [review] Bug 33537: (QA follow-up) Add 'SMTP Servers' button to domain limits page Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157032 [details] [review] Bug 33537: (QA follow-up) Improve process_message_queue warning message Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
I cannot add a new entry, because I am silly^Wa developer and just filled inputs with random data. I am expecting the form to tell it, but errors are ignored: 29 if ( !$('#domain_limits_modal form').get(0).checkValidity() ) return; // Do not return false here to allow validation. I think it was working in a previous iteration.
If I remove the line I get the response's server (better than nothing!) "Adding limit for 'eza' failed. (Status 400 - Bad request: expected columns were empty.)"
Working here to restore things like they worked before. Will squash a few patches now too. Removing tidy from the middle. Resolve forbidden patterns squashed with REST API. Modal UI fixes squashed with Add staff template. The Use bootstrap patch has my attention now. It removes some of the previously working checks.
(In reply to Marcel de Rooy from comment #74) > The Use bootstrap patch has my attention now. It removes some of the > previously working checks. Squashed it now too with Add staff template. Added a event stop propagation to get validation back again.
# Failed test 'Check warn for reaching limit A' # at /usr/share/koha/t/db_dependent/Koha/Notice_Util.t line 134. # found warning: Sending messages: The domain a has reached the limit of 3 messages per 5 minutes at /usr/share/koha/Koha/Notice/Util.pm line 134. # expected to find warning: (?^u:Sending messages: domain a reached limit) # Looks like you failed 1 test of 12. Last follow-up reworded the warn. Failing the test. Corrected and squashed it too now with Connect the dots.
Created attachment 157038 [details] [review] Bug 33537: Add better named table_exists alias in Installer module Test plan: Run dbrev later. It is using table_exists. Run t/db_dependent/Installer.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 157039 [details] [review] Bug 33537: Fix some POD in Installer.pm Test plan: Run qa tools. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 157040 [details] [review] Bug 33537: Database revision, adding table mail_domain_limits Test plan: Run dbrev or new install. Verify that table is created as expected. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 157041 [details] [review] Bug 33537: [DBIx] Schema changes for mail_domain_limits Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 157042 [details] [review] Bug 33537: Add undefined default value for belongs_to in TestBuilder Test plan: See next patch. It calls build_object without specifying belongs_to. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 157043 [details] [review] Bug 33537: Add Koha objects Test plan: Run t/db_dependent/Koha/MailDomainLimits.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Run perltidy on MailDomainLimits.t Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 157044 [details] [review] Bug 33537: REST API endpoints Test plan: Run t/db_dependent/api/v1/domain_limits.t List, add (post), get, update (put) and delete domain limit(s). Using the paths: - api/v1/smtp/domain_limits - api/v1/smtp/domain_limits/{SOME_ID} Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> [EDIT] Applied the QA-follow-up-Resolve-forbidden-pattern-in-.patch here.
Created attachment 157045 [details] [review] Bug 33537: Add staff template and script Test plan: Go to Administration/SMTP Servers. Click Domain limits. Do some CRUD operations. Bonus: Add another tab with domain limits. Edit or remove records on tab 1. Try CRUD on changed or removed records on tab 2. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Added some preventDefault calls, as suggested by JD. Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Bug 33537: (QA follow-up) Modal UI fixes: Use ol instead of ul in modal fieldset inputs: This ol->ul change makes the UI inputs more spaced and overall more in line with other modals in Koha. Use 'btn-primary' on 'Save' button to have it be shown yellow as other confirmation buttons in Koha. Add a11y attributes to modal form Also prettied HTML code: git show --ignore-all-space <commit_hash> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 33537: (QA follow-up) Use bootstrap modal native functions Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] To get validation working again, needed to add ev.stopPropagation() in #domain_limits_modal button.save's onclick.
Created attachment 157046 [details] [review] Bug 33537: Connect the dots Use Koha::MailDomainLimits in Koha::Notice::Util->load_domain_limits. Only use koha-conf as fallback when table is still empty. (This is just like SMTP servers.) The test is adjusted accordingly and has moved more to the database oriented side, first proving that both approaches return the same. Test plan: Run t/db_dependent/Koha/Notice_Util.t NOTE: Replace A and B hereunder by two domains of your own choice. Add a limit to KOHA-CONF: domain A, 1 per 1 hour. Add another limit in WEB CONFIG: domain B, 1 per 1 hour Send 2 notices to domain B. Is 1 message delayed? Remove the table entry for B. Run process_message_queue.pl. Is the delayed notice to B sent? Send 2 notices to domain A. Is 1 delayed (using koha-conf fallback)? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Bug 33537: (QA follow-up) Improve process_message_queue warning message Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Adjusted test for rewording of the warn.
Created attachment 157047 [details] [review] Bug 33537: (QA follow-up) Update kohastructure.sql to match atomic update script Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157048 [details] [review] Bug 33537: (QA follow-up) Add 'SMTP Servers' button to domain limits page Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
What I did not yet do, is reinsert some tidy stuff. The patch in the middle for that has been obsoleted. Sure, I am willing to add it again ;) But if QA and RM are going to ask for doing it different again, this is just waste of time (again). Shall i tidy after QA ?
WARN C4/Installer.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 170, now: 171) FAIL Koha/Exceptions/DomainLimit.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 4) WARN Koha/MailDomainLimit.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 1) WARN Koha/Notice/Util.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 19, now: 23) WARN Koha/REST/V1/Config/SMTP/DomainLimits.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 27) WARN admin/domain_limits.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 1) WARN installer/data/mysql/atomicupdate/bug33537.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 5) WARN t/db_dependent/Installer.t WARN tidiness The file is less tidy than before (bad/messy lines before: 18, now: 19) WARN t/db_dependent/Koha/MailDomainLimits.t WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 3) WARN t/db_dependent/Koha/Notice_Util.t WARN tidiness The file is less tidy than before (bad/messy lines before: 37, now: 38) WARN t/db_dependent/api/v1/domain_limits.t WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 7) WARN t/lib/TestBuilder.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 205, now: 206)
Whilst I'm slightly surprised by the way the JS was written here.. I can't honestly fail it.. but it does introduce a somewhat different pattern. What I did find more apparent from an end user perspective however was the different navigation we introduce here. I find the 'Administration -> SMPT Servers -> Button in toolbar to get to Domain limits' flow a bit weird feeling. I will admit, I've forgotten exactly how the actual feature fits together with the SMTP Servers stuff.. I thought it was actually effectively independent.. in which case I would have said it should match other admin pages and have its own independent menu option from the admin home and sidebars.
Thx anyone for trying to get this further but it will not be ready in time for 23.11 anymore. Will resume this later. Changing to FQA for now. The "template/javascript does not meet Koha standards". Some other points are not clear (link with SMTP servers); perhaps that is more a documentation issue btw. If there is additional feedback to support the current stand, please add.