https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- It is easy to create overdue triggers that don't go away when a category is deleted. Those rules are hidden from the editor and can cause overdue notices to no longer send! -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 vasanth.k@infoplus.co.in changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |vasanth.k@infoplus.co.in -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Liz Rea <wizzyrea@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wizzyrea@gmail.com --- Comment #2 from Liz Rea <wizzyrea@gmail.com> --- I've just had this problem with a branch that was deleted and the rules didn't go away, crashing out the overdues script! -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #105368|0 |1 is obsolete| | --- Comment #4 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 105369 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=105369&action=edit Bug 18086: Add FK constraints for branchcode and categorycode The overduerules table has no foreign key constraints to any for the related tables. The columns for branchcode, categorycode, letter1, letter2, and letter3 should all be foreign keys. Test Plan: 0) Create some overdue rules with invalid branchcodes and categorycodes 1) Apply this patch 2) Restart all the things! 3) Run updatedatabase.pl 4) Note the invalid rules are deleted 5) Note Default codes are NULL now instead of empty strings 6) Editor should behave as it did before -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #105369|0 |1 is obsolete| | --- Comment #5 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 105380 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=105380&action=edit Bug 18086: Add FK constraints for branchcode and categorycode The overduerules table has no foreign key constraints to any for the related tables. The columns for branchcode, categorycode, letter1, letter2, and letter3 should all be foreign keys. Test Plan: 0) Create some overdue rules with invalid branchcodes and categorycodes 1) Apply this patch 2) Restart all the things! 3) Run updatedatabase.pl 4) Note the invalid rules are deleted 5) Note Default codes are NULL now instead of empty strings 6) Editor should behave as it did before -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #105382|0 |1 is obsolete| | --- Comment #7 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 105387 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=105387&action=edit Bug 18086: Add FK constraints for branchcode and categorycode The overduerules table has no foreign key constraints to any for the related tables. The columns for branchcode, categorycode, letter1, letter2, and letter3 should all be foreign keys. Test Plan: 0) Create some overdue rules with invalid branchcodes and categorycodes 1) Apply this patch 2) Restart all the things! 3) Run updatedatabase.pl 4) Note the invalid rules are deleted 5) Note Default codes are NULL now instead of empty strings 6) Editor should behave as it did before -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 --- Comment #9 from Victor Grousset/tuxayo <victor@tuxayo.net> ---

0) Create some overdue rules with invalid branchcodes and categorycodes

Does it means: - Home › Tools › Overdue notice/status triggers - create triggers - check the overduerules table - delete the branch - check that the overduerules are still there

6) Editor should behave as it did before

Does it means checking that "Home › Tools › Overdue notice/status triggers" still works? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net Status|Needs Signoff |Failed QA --- Comment #11 from Victor Grousset/tuxayo <victor@tuxayo.net> ---

6) Editor should behave as it did before

Everything was fine until this step:

Could not compile /kohadevbox/koha/tools/overduerules.pl: syntax error at /kohadevbox/koha/tools/overduerules.pl line 169, near "};" BEGIN not safe after errors--compilation aborted at /kohadevbox/koha/tools/overduerules.pl line 313. at /usr/share/perl5/CGI/Compile.pm line 132

It's a missing parenthesis: }; ↓ }); Even after fixing there is an issue. "Email" and "print" aren't saved when ticked, they disappear after saving. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 --- Comment #14 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Victor Grousset/tuxayo from comment #12)

BTW, how does one makes "phone" and "SMS" tickable?

You can set the SMS::Send driver syspref to 'Email', and enable the talking tech itiva syspref iirc. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #16 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (In reply to Victor Grousset/tuxayo from comment #15)

Should the following problem be fixed?

...

"Email" and "print" aren't saved when ticked, they disappear after saving.

It seems the lasted patch only fixed the compilation error.

I wanted to doublecheck and something else went wrong - be on master code - reset_all - apply patch - updatedatabase - go to /cgi-bin/koha/tools/overduerules.pl - fill on the first line, some delay, some letter, tick everything you can - submit - "Changes saved" - the UI however shows nothing of what was inputted - try again on the same line - still nothing - pick a library - try again - this time it works So there is something wrong with the default triggers. (Tested on Debian 10 and Debian 9.) - be on master code + the patch - spin up the instance (koha testing docker) - go to /cgi-bin/koha/tools/overduerules.pl - fill on the first line, some delay, some letter, tick everything you can - submit - all saved except email and phone - happens only for default triggers caveat: maybe the 2nd case is wrong. Maybe one shouldn't build a dev env with the path already applied. I don't know how DB are handled when integrated in master which makes this work in master. And might prevent from working on a submitted patch. Anyway, first case is valid, there is still an issue. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #105424|0 |1 is obsolete| | --- Comment #17 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 105551 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=105551&action=edit Bug 18086: Add FK constraints for branchcode and categorycode The overduerules table has no foreign key constraints to any for the related tables. The columns for branchcode, categorycode, letter1, letter2, and letter3 should all be foreign keys. Test Plan: 0) Create some overdue rules with invalid branchcodes and categorycodes 1) Apply this patch 2) Restart all the things! 3) Run updatedatabase.pl 4) Note the invalid rules are deleted 5) Note Default codes are NULL now instead of empty strings 6) Editor should behave as it did before -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #20 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi Kyle, working through this and it looks good overall until now. But the tests need to be updated as well I think: $ prove t/db_dependent/Overdues.t t/db_dependent/Overdues.t .. 1/17 {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`overduerules`, CONSTRAINT `fk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement " INSERT INTO overduerules ( overduerules_id, branchcode, categorycode ) VALUES (1, 'CPL', 'PT'), (2, 'CPL', 'YA'), (3, '', 'PT'), (4, '', 'YA') "] at /usr/share/perl5/DBIx/Class/Schema.pm line 1118. DBIx::Class::Schema::throw_exception(Koha::Schema=HASH(0x5626c3608c58), "DBI Exception: DBD::mysql::db do failed: Cannot add or update"...) called at /usr/share/perl5/DBIx/Class/Storage.pm line 113 DBIx::Class::Storage::throw_exception(DBIx::Class::Storage::DBI::mysql=HASH(0x5626c3e11be8), "DBI Exception: DBD::mysql::db do failed: Cannot add or update"...) called at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1501 DBIx::Class::Storage::DBI::__ANON__("DBD::mysql::db do failed: Cannot add or update a child row: a"..., DBI::db=HASH(0x5626c44f6520), undef) called at t/db_dependent/Overdues.t line 33 # Looks like your test exited with 255 just after 3. t/db_dependent/Overdues.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 14/17 subtests Test Summary Report ------------------- t/db_dependent/Overdues.t (Wstat: 65280 Tests: 3 Failed: 0) Non-zero exit status: 255 Parse errors: Bad plan. You planned 17 tests but ran 3. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #106201|0 |1 is obsolete| | --- Comment #22 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 134730 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=134730&action=edit Bug 18086: Add FK constraints for branchcode and categorycode The overduerules table has no foreign key constraints to any for the related tables. The columns for branchcode, categorycode, letter1, letter2, and letter3 should all be foreign keys. Test Plan: 0) Create some overdue rules with invalid branchcodes and categorycodes 1) Apply this patch 2) Restart all the things! 3) Run updatedatabase.pl 4) Note the invalid rules are deleted 5) Note Default codes are NULL now instead of empty strings 6) Editor should behave as it did before -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #24 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Just a quick rebase.. tests are still failing for obvious reasons (We've set NOT NULL and NULL instead of '') and the tests try to set the data that way. Needs a little digging to understand intentions. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #25 from David Cook <dcook@prosentient.com.au> --- Just got bitten by this one again... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |baptiste.wojtkowski@biblibr | |e.com --- Comment #27 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Hello ! I have been working a bit on this bug and I have a few conclusions, I'll address them in patches following this message First, is is on of my first contribution to a non trivial patch so please tell me if I do some things wrong :) In particular, I did not found the information if tests should consider the db empty or pupulated with populate_database so I ran the tests with an empty db. 1 - I reproduce https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086#c20 , this is due to the foreign key constraint cor categories if 'PT' and 'YA' are not set. I populate the db with these entries. See the dedicated patch for more information. 2 - I do think GetOverdueMessageTransportTypes does not have the correct behaviour with the new patch so we should correct the function and not the test for these tests. # Failed test 'GetOverdueMessageTransportTypes: first overdue is by email for PT (CPL)' # at t/db_dependent/Overdues.t line 124. # Structures begin differing at: # $got->[1] = 'sms' # $expected->[1] = Does not exist helloCPLPT2 at /home/koha/src/C4/Overdues.pm line 769. # Failed test 'GetOverdueMessageTransportTypes: second overdue is by sms for PT (CPL)' # at t/db_dependent/Overdues.t line 127. # Structures begin differing at: # $got->[0] = 'email' # $expected->[0] = 'sms' helloCPLPT3 at /home/koha/src/C4/Overdues.pm line 769. # Failed test 'GetOverdueMessageTransportTypes: third overdue is by email for PT (CPL)' # at t/db_dependent/Overdues.t line 130. # Structures begin differing at: # $got->[1] = 'sms' # $expected->[1] = Does not exist # Looks like you failed 3 tests of 9. I think this is a bug due to "GetOverdueMessageTransportTypes" returning ['email', 'sms', 'email'] in all these three tests, instead of returning ['email'], then ['sms'], and then ['email'] Indeed, Koha::Overdue->getmessagetransporttype returns every entries related to one given rule id, which may have multiple letternumbers, which are not taken anymore in account at this point of the code. I am not sure of the purpose of Koha::Overdue->getmessagetransporttype but i would delete it and directly use the join (not proposed in this patch). 3 - A few tests do not pass because they test the behaviour of functions concerning branches on an empty string, which is now forbidden due to the foreign key constraint. I propose changes for branches and for categories in two different patches, changing the behaviour of functions so that they interprete NULL in db as the default rule. WARNING: Tests seem to pass but I am not sure if coverage is sufficient to guarantee the NULL does not break much things. 4 - A few tests do not pass because they test the behaviour of functions concerning categories on an empty string, which is now forbidden due to the foreign key constraint. Moreover it is forbidden for a categorie to be NULL in overduerules, I remove the constraint there too. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #134731|0 |1 is obsolete| | --- Comment #29 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 168811 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=168811&action=edit Bug 18086: (follow-up) Update for export changes -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 --- Comment #31 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 168813 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=168813&action=edit Bug 18086: Fix issues with NULL for categories See https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086#c27 for more context : This addresses the 4th point of this comment. A few tests do not pass because they test the behaviour of functions concerning categoriees on an empty string, whi ch is now forbidden due to the foreign key constraint. I propose changes for categoriees and for categories in two different patches, changing the behaviour of functions so that they interprete NULL in db as the default rule. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 --- Comment #33 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 168815 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=168815&action=edit Bug 18086: Fix the behaviour of GetOverdueMessageTransportTypes See https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086#c27 for more context : This addresses the second point of this comment. Test concerning GetOverdueMessageTransportTypes were not passing anymore after previous patch. This was due to the behaviour of Koha::Overdue->get_message_types which returns all messages from a rule id, not taking into account the letternumber when considering a OverdueRule (which is totally normal). I fixed the join in GetOverdueMessageTransportTypes to return directly the desired column and use it in the rest of the code Note: Koha::Overdue->get_message_types might not be necessary anymore -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 --- Comment #34 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Hi :)

First, is is on of my first contribution to a non trivial patch so please tell me if I do some things wrong :) In particular, I did not found the information if tests should consider the db empty or pupulated with populate_database so I ran the tests with an empty db.

Yes, empty DB and the test must provide it's data an clean it. See https://wiki.koha-community.org/wiki/Unit_Tests#Where_and_how_to_test (the rest of page is worth reading, not only the middle part about data) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 --- Comment #39 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Are there other tests relevant to run than t/db_dependent/Overdues.t ? Because a coverage analysis[1] shows that GetBranchcodesWithOverdueRules and GetOverdueMessageTransportTypes aren't executed during Overdues.t. [1] https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL17%3A_Unit_tests_... ---- The test plan lacks a step to update the DBIC schema files with the command "dbic" or equivalent. This causes the ORM to be out of sync with the database schema. --- The database upgrade fails when ran twice. kohadev-koha@kohadevbox:koha(testing-SO-QA)$ updatedatabase Upgrade to XXX [22:13:40]: Bug 18086 - Add FK constraints for branchcode and categorycode kohadev-koha@kohadevbox:koha(testing-SO-QA)$ updatedatabase C4::Installer::run_atomic_updates(): DBI Exception: DBD::mysql::db do failed: Can't create table `koha_kohadev`.`overduerules` (errno: 121 "Duplicate key on write or update") at /kohadevbox/koha/installer/data/mysql/updatedatabase.pl line 24309 DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_18086.perl [22:13:44] ERROR - C4::Installer::run_atomic_updates(): DBI Exception: DBD::mysql::db do failed: Can't create table `koha_kohadev`.`overduerules` (errno: 121 "Duplicate key on write or update") at /kohadevbox/koha/installer/data/mysql/updatedatabase.pl line 24309 Here is more info :) https://wiki.koha-community.org/wiki/Database_updates#Idempotent It's needed because such a change might be backported to a previous release of Koha. And then when doing the upgrade to the next release of Koha, part of the database migrations will be ran a second time because they would have been backported to the previous version and ran a first time. --- test plan:

6) Editor should behave as it did before

I forgot how that works, is the editor this page?: http://localhost:8081/cgi-bin/koha/tools/overduerules.pl --- Thanks a lot Baptiste for reviving this old and needed ticket :D -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 --- Comment #41 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- (In reply to Victor Grousset/tuxayo from comment #39)

Are there other tests relevant to run than t/db_dependent/Overdues.t ? Because a coverage analysis[1] shows that GetBranchcodesWithOverdueRules and GetOverdueMessageTransportTypes aren't executed during Overdues.t.

[1] https://wiki.koha-community.org/wiki/ Coding_Guidelines#PERL17%3A_Unit_tests_are_required

Hi ! I don't get your point, tests 4-12 are supposed to test GetOverdueMessageTransportTypes and 13-17 are testing GetBranchcodesWithOverdueRules, am I missing something ?

The database upgrade fails when ran twice.

Done

---

test plan:

...

6) Editor should behave as it did before

I forgot how that works, is the editor this page?: http://localhost:8081/cgi-bin/koha/tools/overduerules.pl

As far as I understand the test plan, yes it is

Thanks a lot Baptiste for reviving this old and needed ticket :D

Thanks for the detailed QA feedback ! :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #168811|0 |1 is obsolete| | --- Comment #43 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 169288 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169288&action=edit Bug 18086: (follow-up) Update for export changes -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #168813|0 |1 is obsolete| | --- Comment #45 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 169290 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169290&action=edit Bug 18086: Fix issues with NULL for categories See https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086#c27 for more context : This addresses the 4th point of this comment. A few tests do not pass because they test the behaviour of functions concerning categoriees on an empty string, whi ch is now forbidden due to the foreign key constraint. I propose changes for categoriees and for categories in two different patches, changing the behaviour of functions so that they interprete NULL in db as the default rule. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #168815|0 |1 is obsolete| | --- Comment #47 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 169292 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169292&action=edit Bug 18086: Fix the behaviour of GetOverdueMessageTransportTypes See https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086#c27 for more context : This addresses the second point of this comment. Test concerning GetOverdueMessageTransportTypes were not passing anymore after previous patch. This was due to the behaviour of Koha::Overdue->get_message_types which returns all messages from a rule id, not taking into account the letternumber when considering a OverdueRule (which is totally normal). I fixed the join in GetOverdueMessageTransportTypes to return directly the desired column and use it in the rest of the code Note: Koha::Overdue->get_message_types might not be necessary anymore -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 --- Comment #49 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 169294 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169294&action=edit Bug 18086: (follow-up) Update atomic update -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169288|0 |1 is obsolete| | --- Comment #51 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 169296 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169296&action=edit Bug 18086: (follow-up) Update for export changes -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169290|0 |1 is obsolete| | --- Comment #53 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 169298 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169298&action=edit Bug 18086: Fix issues with NULL for categories See https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086#c27 for more context : This addresses the 4th point of this comment. A few tests do not pass because they test the behaviour of functions concerning categoriees on an empty string, whi ch is now forbidden due to the foreign key constraint. I propose changes for categoriees and for categories in two different patches, changing the behaviour of functions so that they interprete NULL in db as the default rule. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169292|0 |1 is obsolete| | --- Comment #55 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 169300 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169300&action=edit Bug 18086: Fix the behaviour of GetOverdueMessageTransportTypes See https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086#c27 for more context : This addresses the second point of this comment. Test concerning GetOverdueMessageTransportTypes were not passing anymore after previous patch. This was due to the behaviour of Koha::Overdue->get_message_types which returns all messages from a rule id, not taking into account the letternumber when considering a OverdueRule (which is totally normal). I fixed the join in GetOverdueMessageTransportTypes to return directly the desired column and use it in the rest of the code Note: Koha::Overdue->get_message_types might not be necessary anymore -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169294|0 |1 is obsolete| | --- Comment #57 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 169302 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169302&action=edit Bug 18086: (follow-up) Update atomic update -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #58 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Thanks for the fixes :) --- In the test plan, after updatedatabase, it's missing to run `dbic` (refresh schema). Without that, the ORM is out of sync with the DB so your changes are not effective in the objects. Here, the FK should create relationships between libraries and an overdue rules. ---- (In reply to Baptiste Wojtkowski (bwoj) from comment #41)

(In reply to Victor Grousset/tuxayo from comment #39)

...

Are there other tests relevant to run than t/db_dependent/Overdues.t ? Because a coverage analysis[1] shows that GetBranchcodesWithOverdueRules and GetOverdueMessageTransportTypes aren't executed during Overdues.t.

[1] https://wiki.koha-community.org/wiki/ Coding_Guidelines#PERL17%3A_Unit_tests_are_required

Hi ! I don't get your point, tests 4-12 are supposed to test GetOverdueMessageTransportTypes and 13-17 are testing GetBranchcodesWithOverdueRules, am I missing something ?

I wanted to double check running the tests with Devel::Cover: `perl -MDevel::Cover t/db_dependent/Overdues.t ; cover` But the new patches fail running them. (even after refreshing the schema files) kohadev-koha@kohadevbox:koha(testing-SO-QA)$ prove t/db_dependent/Overdues.t t/db_dependent/Overdues.t .. 1/18 {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot delete or update a parent row: a foreign key constraint fails (`koha_kohadev`.`borrowers`, CONSTRAINT `borrowers_ibfk_2` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`)) at /usr/share/perl5/DBIx/Class/Schema.pm line 1118. DBIx::Class::Schema::throw_exception(Koha::Schema=HASH(0x5b316b2d71c0), "DBI Exception: DBD::mysql::db do failed: Cannot delete or upd"...) called at /usr/share/perl5/DBIx/Class/Storage.pm line 113 DBIx::Class::Storage::throw_exception(DBIx::Class::Storage::DBI::mysql=HASH(0x5b3173104ba8), "DBI Exception: DBD::mysql::db do failed: Cannot delete or upd"...) called at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1501 DBIx::Class::Storage::DBI::__ANON__("DBD::mysql::db do failed: Cannot delete or update a parent ro"..., DBI::db=HASH(0x5b31757496b0), undef) called at t/db_dependent/Overdues.t line 29 # Looks like your test exited with 11 just after 3. t/db_dependent/Overdues.t .. Dubious, test returned 11 (wstat 2816, 0xb00) Failed 15/18 subtests Test Summary Report ------------------- t/db_dependent/Overdues.t (Wstat: 2816 Tests: 3 Failed: 0) Non-zero exit status: 11 Parse errors: Bad plan. You planned 18 tests but ran 3. Files=1, Tests=3, 3 wallclock secs ( 0.04 usr 0.00 sys + 2.43 cusr 0.23 csys = 2.70 CPU) Result: FAIL -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169295|0 |1 is obsolete| | --- Comment #60 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 169396 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169396&action=edit Bug 18086: Add FK constraints for branchcode and categorycode The overduerules table has no foreign key constraints to any for the related tables. The columns for branchcode, categorycode, letter1, letter2, and letter3 should all be foreign keys. Test Plan: 0) Create some overdue rules with invalid branchcodes and categorycodes 1) Apply this patch 2) Restart all the things! 3) Run updatedatabase.pl 4) Update DBIx classes 5) Note the invalid rules are deleted 6) Note Default codes are NULL now instead of empty strings 7) Editor should behave as it did before -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169297|0 |1 is obsolete| | --- Comment #62 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 169398 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169398&action=edit Bug 18086: Fix issues with NULL for branches See https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086#c27 for more context : This addresses the third point of this comment. A few tests do not pass because they test the behaviour of functions concerning branches on an empty string, which is now forbidden due to the foreign key constraint. I propose changes for branches and for categories in two different patches, changing the behaviour of functions so that they interprete NULL in db as the default rule. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169299|0 |1 is obsolete| | --- Comment #64 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 169400 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169400&action=edit Bug 18086: Add categories to the db in tests See https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086#c27 for more context : This addresses the first point of this comment. Due to the constraint applied to categories, it is necessary that categories exist in db so I create them -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169301|0 |1 is obsolete| | --- Comment #66 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 169402 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169402&action=edit Bug 18086: (follow-up) remove COLLATE from sql update -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 --- Comment #68 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 169404 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169404&action=edit Bug 18086: (follow-up) change the order of branches deletion -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169397|0 |1 is obsolete| | --- Comment #72 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 169456 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169456&action=edit Bug 18086: (follow-up) Update for export changes -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169399|0 |1 is obsolete| | --- Comment #74 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 169458 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169458&action=edit Bug 18086: Fix issues with NULL for categories See https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086#c27 for more context : This addresses the 4th point of this comment. A few tests do not pass because they test the behaviour of functions concerning categoriees on an empty string, whi ch is now forbidden due to the foreign key constraint. I propose changes for categoriees and for categories in two different patches, changing the behaviour of functions so that they interprete NULL in db as the default rule. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169401|0 |1 is obsolete| | --- Comment #76 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 169460 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169460&action=edit Bug 18086: Fix the behaviour of GetOverdueMessageTransportTypes See https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086#c27 for more context : This addresses the second point of this comment. Test concerning GetOverdueMessageTransportTypes were not passing anymore after previous patch. This was due to the behaviour of Koha::Overdue->get_message_types which returns all messages from a rule id, not taking into account the letternumber when considering a OverdueRule (which is totally normal). I fixed the join in GetOverdueMessageTransportTypes to return directly the desired column and use it in the rest of the code Note: Koha::Overdue->get_message_types might not be necessary anymore -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169403|0 |1 is obsolete| | --- Comment #78 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 169462 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169462&action=edit Bug 18086: (follow-up) Update atomic update -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #80 from Victor Grousset/tuxayo <victor@tuxayo.net> --- updatedatabase fails to cleanup a case. - DB prior to update MariaDB [koha_kohadev]> select * from overduerules\G *************************** 1. row *************************** overduerules_id: 1 branchcode: tmplib categorycode: B delay1: 1 letter1: ACCOUNT_DEBIT debarred1: 1 delay2: 2 debarred2: 1 letter2: AR_SLIP delay3: 3 letter3: AR_PENDING debarred3: 1 *************************** 2. row *************************** overduerules_id: 2 branchcode: categorycode: TMPCAT delay1: 1 letter1: ISSUEQSLIP debarred1: 1 delay2: 2 debarred2: 1 letter2: ISSUESLIP delay3: 3 letter3: ISSUESLIP debarred3: 1 2 rows in set (0.000 sec) - applied patches - updatedatabase: DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_18086.pl [21:38:51]: Bug 18086 - Add foreign key constraints to table overduerule Added constraint to column of table 'overduerules.branchcode' ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`#sql-alter-1-35`, CONSTRAINT `categorycode_ibfk` FOREIGN KEY (`categorycode`) REFERENCES `categories` (`categorycode`) ON DELETE CASCADE ON UPDATE CASCADE) at /kohadevbox/koha/C4/Installer.pm line 741 - DB after update MariaDB [koha_kohadev]> select * from overduerules\G *************************** 1. row *************************** overduerules_id: 2 branchcode: NULL categorycode: TMPCAT delay1: 1 letter1: ISSUEQSLIP debarred1: 1 delay2: 2 debarred2: 1 letter2: ISSUESLIP delay3: 3 letter3: ISSUESLIP debarred3: 1 1 row in set (0.000 sec) I had two cases of corrupt data: - deleted library and valid patron category: worked - default (empty string before the update) library and deleted patron category: failed -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169463|0 |1 is obsolete| | --- Comment #82 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 169544 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169544&action=edit Bug 18086: (follow-up) change the order of branches deletion -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 --- Comment #83 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- In bug 10190 I'm actually looking to migrate these rules into the circ rules table and do away with the tables here entirely. We're working on a new UI and some additional functionality there to allow for functional expansion and it made sense to push them into the circ rules system as part of it. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 --- Comment #85 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Baptiste Wojtkowski (bwoj) from comment #84)

If I understand well, validating bug 10190 would make this one obsolete ?

I believe that is correct! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18086 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |MOVED Status|Needs Signoff |RESOLVED --- Comment #87 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- (In reply to Magnus Enger from comment #86)

(In reply to Kyle M Hall (khall) from comment #85)

...

(In reply to Baptiste Wojtkowski (bwoj) from comment #84)

...

If I understand well, validating bug 10190 would make this one obsolete ?

I believe that is correct!

Bug 10190 is "signed off". Should we just close the current bug (bug 18086) and concentrate on 10190?

Sounds good to me! -- You are receiving this mail because: You are watching all bug changes.