Bug 18086 - overduerules table is lacking foreign key constraints
Summary: overduerules table is lacking foreign key constraints
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 5334
  Show dependency treegraph
 
Reported: 2017-02-08 18:19 UTC by Kyle M Hall
Modified: 2023-01-24 00:04 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 18086: Add FK constraints for branchcode and categorycode (13.09 KB, patch)
2020-05-26 19:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18086: Add FK constraints for branchcode and categorycode (12.95 KB, patch)
2020-05-26 19:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18086: Add FK constraints for branchcode and categorycode (13.29 KB, patch)
2020-05-27 10:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18086: Add FK constraints for branchcode and categorycode (13.59 KB, patch)
2020-05-27 10:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18086: Add FK constraints for branchcode and categorycode (14.48 KB, patch)
2020-05-27 10:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18086: Add FK constraints for branchcode and categorycode (14.51 KB, patch)
2020-05-27 10:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18086: Add FK constraints for branchcode and categorycode (14.51 KB, patch)
2020-05-28 17:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18086: Add FK constraints for branchcode and categorycode (20.12 KB, patch)
2020-06-04 13:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18086: Add FK constraints for branchcode and categorycode (20.51 KB, patch)
2020-06-23 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18086: Add FK constraints for branchcode and categorycode (20.72 KB, patch)
2022-05-06 12:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18086: (follow-up) Update for export changes (929 bytes, patch)
2022-05-06 12:52 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2017-02-08 18:19:23 UTC
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.
Comment 1 Kyle M Hall 2017-02-08 18:20:34 UTC
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!
Comment 2 Liz Rea 2020-02-18 20:18:23 UTC
I've just had this problem with a branch that was deleted and the rules didn't go away, crashing out the overdues script!
Comment 3 Kyle M Hall 2020-05-26 19:32:43 UTC
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
Comment 4 Kyle M Hall 2020-05-26 19:33:55 UTC
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
Comment 5 Kyle M Hall 2020-05-27 10:37:48 UTC
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
Comment 6 Kyle M Hall 2020-05-27 10:42:16 UTC
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
Comment 7 Kyle M Hall 2020-05-27 10:44:58 UTC
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
Comment 8 Kyle M Hall 2020-05-27 10:58:21 UTC
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
Comment 9 Victor Grousset/tuxayo 2020-05-27 12:48:13 UTC
> 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?
Comment 10 Kyle M Hall 2020-05-27 14:16:02 UTC
(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!
Comment 11 Victor Grousset/tuxayo 2020-05-28 03:37:05 UTC
> 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.
Comment 12 Victor Grousset/tuxayo 2020-05-28 03:37:45 UTC
BTW, how does one makes "phone" and "SMS" tickable?
Comment 13 Kyle M Hall 2020-05-28 17:58:23 UTC
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
Comment 14 Kyle M Hall 2020-05-28 17:59:39 UTC
(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.
Comment 15 Victor Grousset/tuxayo 2020-05-28 19:18:45 UTC
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.
Comment 16 Victor Grousset/tuxayo 2020-05-29 17:43:47 UTC
(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.
Comment 17 Kyle M Hall 2020-06-04 13:34:07 UTC
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
Comment 18 Jonathan Druart 2020-06-17 12:01:52 UTC
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
Comment 19 Kyle M Hall 2020-06-23 12:58:46 UTC
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
Comment 20 Katrin Fischer 2020-09-16 06:16:45 UTC
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.
Comment 21 Kyle M Hall 2020-11-16 18:58:14 UTC
(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?
Comment 22 Martin Renvoize 2022-05-06 12:52:07 UTC
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
Comment 23 Martin Renvoize 2022-05-06 12:52:12 UTC
Created attachment 134731 [details] [review]
Bug 18086: (follow-up) Update for export changes
Comment 24 Martin Renvoize 2022-05-06 12:53:31 UTC
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.
Comment 25 David Cook 2023-01-23 23:58:51 UTC
Just got bitten by this one again...
Comment 26 David Cook 2023-01-24 00:04:37 UTC
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 :/