Bug 18086

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!

I've just had this problem with a branch that was deleted and the rules didn't go away, crashing out the overdues script!

Created attachment 105368 [details] [review]
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

Created attachment 105369 [details] [review]
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

Created attachment 105380 [details] [review]
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

Created attachment 105382 [details] [review]
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

Created attachment 105387 [details] [review]
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

Created attachment 105388 [details] [review]
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

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

(In reply to Victor Grousset/tuxayo from comment #9)
> > 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

Yes, that would work. I was lazy and just inserted them directly from the mysql cli :)


> > 6) Editor should behave as it did before
> 
> Does it means checking that "Home › Tools › Overdue notice/status triggers"
> still works?

Yes!

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

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

Created attachment 105424 [details] [review]
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

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

Thanks!

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.

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

Created attachment 105551 [details] [review]
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

Kyle, we are loosing the default values.

To recreate:
On master, add some rule for the "Default" library (branchcode="")
Apply the patches, run the udpate DB

Created attachment 106201 [details] [review]
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

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.

(In reply to Katrin Fischer from comment #20)
> 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.

I'm unable to recreate your error. Are you using a fresh database?

Created attachment 134730 [details] [review]
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

Created attachment 134731 [details] [review]
Bug 18086: (follow-up) Update for export changes

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.

Just got bitten by this one again...

I reckon this bug could be split into 2.

Why don't we just add the foreign keys to the database, and then do the Perl changes in a separate bug? 

This report has been stuck for 5 years now :/

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.

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)

Comment on attachment 168810 [details] [review]
Bug 18086: Add FK constraints for branchcode and categorycode

Review of attachment 168810 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/kohastructure.sql
@@ +4962,5 @@
>  /*!40101 SET character_set_client = utf8 */;
>  CREATE TABLE `overduerules` (
>    `overduerules_id` int(11) NOT NULL AUTO_INCREMENT COMMENT 'unique identifier for the overduerules',
> +  `branchcode` varchar(10) COLLATE utf8mb4_unicode_ci NULL DEFAULT NULL COMMENT 'foreign key from the branches table to define which branch this rule is for (if blank it''s all libraries)',
> +  `categorycode` varchar(10) COLLATE utf8mb4_unicode_ci NOT NULL COMMENT 'foreign key from the categories table to define which patron category this rule is for',

Is there a reason for adding "COLLATE utf8mb4_unicode_ci" ?

It's already set exactly like that on the whole table.

Comment on attachment 168814 [details] [review]
Bug 18086: Add categories to the db in tests

Review of attachment 168814 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/Overdues.t
@@ +92,5 @@
> +    $samplecat2->{bulk},                  $samplecat2->{enrolmentfee},
> +    $samplecat2->{overduenoticerequired}, $samplecat2->{issuelimit},
> +    $samplecat2->{reservefee},            $samplecat2->{hidelostitems},
> +    $samplecat2->{category_type}
> +);

This is a case were it's sure it's needed to use TestBuilder instead: https://wiki.koha-community.org/wiki/Unit_Tests#Using_TestBuilder

Comment on attachment 168812 [details] [review]
Bug 18086: Fix issues with NULL for branches

Review of attachment 168812 [details] [review]:
-----------------------------------------------------------------

::: C4/Overdues.pm
@@ +686,5 @@
>      |);
> +    unless ( $branchcodes->[0] ) {
> +        my $default_rule = Koha::OverdueRules->search({ branchcode => undef }, {})->next;
> +        if ($default_rule){
> +            warn "default rule";

Is that a remaining debug statement from the development?

::: t/db_dependent/Overdues.t
@@ +36,5 @@
> +$dbh->do(q|
> +    INSERT INTO branches ( branchcode, branchname ) VALUES
> +    ('CPL', 'foo'),
> +    ('MPL', 'bar')
> +|);

Might be worth using TestBuilder.
Something like:

$builder->build_object( { class => 'Koha::Libraries', value => { branchcode => 'CPL' } } );

(In reply to Baptiste Wojtkowski (bwoj) from comment #27)
> 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).

Where is that existing `Koha::Overdue->getmessagetransporttype` whose purpose needs to be confirmed?

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

----

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

(In reply to Victor Grousset/tuxayo from comment #38)
> (In reply to Baptiste Wojtkowski (bwoj) from comment #27)
> > 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).
> 
> Where is that existing `Koha::Overdue->getmessagetransporttype` whose
> purpose needs to be confirmed?

Sorry, I am talking about the method added by the first version of the patch Koha::Overduerule::transport_types. It is the method I stop using line 790 in the followup https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=168815&action=diff

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

Created attachment 169295 [details] [review]
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

Created attachment 169296 [details] [review]
Bug 18086: (follow-up) Update for export changes

Created attachment 169297 [details] [review]
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.

Created attachment 169298 [details] [review]
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.

Created attachment 169299 [details] [review]
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

Created attachment 169300 [details] [review]
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

Created attachment 169301 [details] [review]
Bug 18086: (follow-up) remove COLLATE from sql update

Created attachment 169302 [details] [review]
Bug 18086: (follow-up) Update atomic update

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

errata:
> 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.

It's not missing, it's there ^^" «Update DBIx classes»


> 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 get it, I just switched back to the patches as they were in my 1st testing and the failure was already there but it managed to do some tests so Devel::Cover was able to output it's long report at the end which hid the failure messages.

With the current patches, it's just that this time I noticed the error and didn't let the tests finish.

Created attachment 169396 [details] [review]
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

Created attachment 169397 [details] [review]
Bug 18086: (follow-up) Update for export changes

Created attachment 169398 [details] [review]
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.

Created attachment 169399 [details] [review]
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.

Created attachment 169400 [details] [review]
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

Created attachment 169401 [details] [review]
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

Created attachment 169402 [details] [review]
Bug 18086: (follow-up) remove COLLATE from sql update

Created attachment 169403 [details] [review]
Bug 18086: (follow-up) Update atomic update

Created attachment 169404 [details] [review]
Bug 18086: (follow-up) change the order of branches deletion

> 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

Yep, totally forgot I had been testing on an empty db and branches have to be removed in a propper order according to the FK constraints.

> It's not missing, it's there ^^" «Update DBIx classes»

tbh I had forgotten it on the first try that's why there are 8 obsolete patches ^^

Comment on attachment 169401 [details] [review]
Bug 18086: Fix the behaviour of GetOverdueMessageTransportTypes

Review of attachment 169401 [details] [review]:
-----------------------------------------------------------------

::: C4/Overdues.pm
@@ +789,2 @@
>      my @mtts;
> +    while ( my $mtt = $rule->next ) {

$mtt needs to be renamed here. It's not a message_transport_type anymore.

Created attachment 169455 [details] [review]
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

Created attachment 169456 [details] [review]
Bug 18086: (follow-up) Update for export changes

Created attachment 169457 [details] [review]
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.

Created attachment 169458 [details] [review]
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.

Created attachment 169459 [details] [review]
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

Created attachment 169460 [details] [review]
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

Created attachment 169461 [details] [review]
Bug 18086: (follow-up) remove COLLATE from sql update

Created attachment 169462 [details] [review]
Bug 18086: (follow-up) Update atomic update

Created attachment 169463 [details] [review]
Bug 18086: (follow-up) change the order of branches deletion

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

Created attachment 169543 [details] [review]
Bug 18086: (follow-up) Update atomic update

Created attachment 169544 [details] [review]
Bug 18086: (follow-up) change the order of branches deletion

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.

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

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

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

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