| Summary: | Missing FK on old_reserves.branchcode | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Jonathan Druart <jonathan.druart> |
| Component: | Architecture, internals, and plumbing | Assignee: | Jonathan Druart <jonathan.druart> |
| Status: | RESOLVED FIXED | QA Contact: | Testopia <testopia> |
| Severity: | normal | ||
| Priority: | P5 - low | CC: | fridolin.somers, kyle, lucas, m.de.rooy, nick, teerthacha1993, tomascohen, victor |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | --- |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: |
Add a foreign key on the old_reserves.branchcode database column. This link was missing and the column may contain incorrect data/branchcode.
Note that the values will now be set to NULL when the branchcode is incorrect.
|
Version(s) released in: |
24.05.00,23.11.06
|
| Circulation function: | |||
| Bug Depends on: | |||
| Bug Blocks: | 5334, 35353 | ||
| Attachments: |
Bug 35610: Add FK on old_reserves.branchcode
Bug 35610: Add FK on old_reserves.branchcode Bug 35610: Add FK on old_reserves.branchcode Bug 35610: Set branchcode to NULL before adding the FK Bug 35610: Add FK on old_reserves.branchcode Bug 35610: Set branchcode to NULL before adding the FK Bug 35610: (QA follow-up): tidy up code |
||
|
Description
Jonathan Druart
2023-12-20 09:27:53 UTC
I believe we don't have strict FK on the statistic tables like statistics and action_logs and as old_reserves is also used mainly for statistics, it makes kind of sense. If we decide to merge reserve and old_reserves into a holds table, that will probably have to change. Created attachment 160097 [details] [review] Bug 35610: Add FK on old_reserves.branchcode Created attachment 160098 [details] [review] Bug 35610: Add FK on old_reserves.branchcode (In reply to Katrin Fischer from comment #1) > I believe we don't have strict FK on the statistic tables like statistics > and action_logs and as old_reserves is also used mainly for statistics, it > makes kind of sense. FK can be NULL. Do you mean we want to keep the branchcode values for deleted libraries? (In reply to Jonathan Druart from comment #5) > Do you mean we want to keep the branchcode values for deleted libraries? It might be something people query, but I am not sure how often branches are closed etc. I was thinking maybe a better moment to fix might be when merging, because we could advertise that change a bit more. But if there is a need now, maybe it's ok. For academic libraries in India we have seen several instances of library branch re-organisation due to accreditation compliance purpose. In such cases, we are often faced with a the need for a mechanism to track such changes. (In reply to Teertha Chatterjee from comment #7) > For academic libraries in India we have seen several instances of library > branch re-organisation due to accreditation compliance purpose. In such > cases, we are often faced with a the need for a mechanism to track such > changes. Action logs and statistics exist for that very reason. All other tables need to have proper FK constraints. Created attachment 161540 [details] [review] Bug 35610: Add FK on old_reserves.branchcode Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> As expected, doing an upgrade when there is 1. a hold on a library with no data linked (for easy deletion) in sample data: Liberty 2. cancel the hold 3. delete the library 4. apply the patch 5. run updatedatabase fails with: DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_35610.pl [01:44:28]: Bug 35610 - Add FK on old_reserves.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-2f`, CONSTRAINT `old_reserves_ibfk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE) at /kohadevbox/koha/C4/Installer.pm line 741 That's a rough to have such an upgrade requiring manual DB fixes. And it's not a weird workflow that can be considered messing with the DB. There doesn't seem to be a mention about how to handle that in bug 25260. How about before adding the FK having an UPDATE that NULLs the old_reserves.branchcode that don't match an existing branch? Or another approach? --- (In reply to Katrin Fischer from comment #6) > I was thinking maybe a better moment to fix might be when > merging, because we could advertise that change a bit more. What would be needed to advertise in advance? Like if they are using old_reserves for stats they should stop, use action logs/statistics and backup old_reserve before the future upgrade for past data? Created attachment 163827 [details] [review] Bug 35610: Set branchcode to NULL before adding the FK (In reply to Victor Grousset/tuxayo from comment #10) > As expected, doing an upgrade when there is > 1. a hold on a library with no data linked (for easy deletion) in sample > data: Liberty > 2. cancel the hold > 3. delete the library > 4. apply the patch > 5. run updatedatabase > > fails with: > > DEV atomic update > /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_35610.pl [01:44:28]: > Bug 35610 - Add FK on old_reserves.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-2f`, CONSTRAINT `old_reserves_ibfk_branchcode` > FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE > SET NULL ON UPDATE CASCADE) at /kohadevbox/koha/C4/Installer.pm line 741 Fixed. Created attachment 163966 [details] [review] Bug 35610: Add FK on old_reserves.branchcode Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 163967 [details] [review] Bug 35610: Set branchcode to NULL before adding the FK Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 163968 [details] [review] Bug 35610: (QA follow-up): tidy up code Works, makes sense, QA script happy, code looks good, passing QA :) Test with comment 10 and checked that before the patch, orphan branchcodes where left and that after the patch, existing ones where handled and it was not possible to create new ones. I agree with this change, but please add a note/warning in the release notes so people are aware on update that this might "lose" them data by setting the deleted branchcodes to NULL. Pushed for 24.05! Well done everyone, thank you! Sadly, this broke several tests... prove t/db_dependent/Koha/Old/Hold.t prove t/db_dependent/Koha/Old/Hold.t t/db_dependent/Koha/Old/Hold.t .. # Looks like you planned 8 tests but ran 1. t/db_dependent/Koha/Old/Hold.t .. 1/2 # Failed test 'anonymize() tests' # at t/db_dependent/Koha/Old/Hold.t line 82. DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`old_reserves`, CONSTRAINT `old_reserves_ibfk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE) at /kohadevbox/koha/t/lib/TestBuilder.pm line 384 # Looks like your test exited with 255 just after 1. t/db_dependent/Koha/Old/Hold.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 2/2 subtests prove t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Patrons.t .. 30/43 # Looks like you planned 113 tests but ran 69. t/db_dependent/Koha/Patrons.t .. 31/43 # Failed test 'Test Koha::Patrons::merge' # at t/db_dependent/Koha/Patrons.t line 1844. DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`old_reserves`, CONSTRAINT `old_reserves_ibfk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE) at /kohadevbox/koha/t/lib/TestBuilder.pm line 384 # Looks like your test exited with 11 just after 31. t/db_dependent/Koha/Patrons.t .. Dubious, test returned 11 (wstat 2816, 0xb00) Failed 13/43 subtests Test Summary Report ------------------- t/db_dependent/Koha/Patrons.t (Wstat: 2816 Tests: 31 Failed: 1) Failed test: 31 Non-zero exit status: 11 Parse errors: Bad plan. You planned 43 tests but ran 31. Files=1, Tests=31, 12 wallclock secs ( 0.04 usr 0.01 sys + 8.47 cusr 1.94 csys = 10.46 CPU) Result: FAIL prove t/db_dependent/UsageStats.t t/db_dependent/UsageStats.t .. 1/4 # No tests run! # Failed test 'No tests run for subtest "BuildReport() tests"' # at t/db_dependent/UsageStats.t line 138. DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`old_reserves`, CONSTRAINT `old_reserves_ibfk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE) at /kohadevbox/koha/t/lib/TestBuilder.pm line 384 # Looks like your test exited with 255 just after 4. t/db_dependent/UsageStats.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 1/4 subtests Test Summary Report ------------------- t/db_dependent/UsageStats.t (Wstat: 65280 Tests: 4 Failed: 1) Failed test: 4 Non-zero exit status: 255 Files=1, Tests=4, 1 wallclock secs ( 0.02 usr 0.00 sys + 0.87 cusr 0.11 csys = 1.00 CPU) Result: FAIL prove t/db_dependent/Koha/Old/Holds.t t/db_dependent/Koha/Old/Holds.t .. # Looks like you planned 10 tests but ran 4. t/db_dependent/Koha/Old/Holds.t .. 1/2 # Failed test 'anonymize() tests' # at t/db_dependent/Koha/Old/Holds.t line 114. DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`old_reserves`, CONSTRAINT `old_reserves_ibfk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE) at /kohadevbox/koha/t/lib/TestBuilder.pm line 384 # Looks like your test exited with 255 just after 1. t/db_dependent/Koha/Old/Holds.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 2/2 subtests Test Summary Report ------------------- t/db_dependent/Koha/Old/Holds.t (Wstat: 65280 Tests: 1 Failed: 1) Failed test: 1 Non-zero exit status: 255 Parse errors: Bad plan. You planned 2 tests but ran 1. Files=1, Tests=1, 1 wallclock secs ( 0.01 usr 0.00 sys + 0.83 cusr 0.08 csys = 0.92 CPU) Result: FAIL HELP...
prove t/db_dependent/TestBuilder.t
t/db_dependent/TestBuilder.t .. 1/16
# Failed test 'TestBuilder should be able to create an object for every source'
# at t/db_dependent/TestBuilder.t line 85.
# got: '1'
# expected: '0'
# The following sources have not been generated correctly: OldReserve
# Looks like you failed 1 test of 1.
t/db_dependent/TestBuilder.t .. 3/16
# Failed test 'Build all sources'
# at t/db_dependent/TestBuilder.t line 93.
t/db_dependent/TestBuilder.t .. 7/16 # Looks like you planned 5 tests but ran 4.
t/db_dependent/TestBuilder.t .. 12/16
# Failed test 'build_object() tests'
# at t/db_dependent/TestBuilder.t line 473.
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`old_reserves`, CONSTRAINT `old_reserves_ibfk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE) at /kohadevbox/koha/t/lib/TestBuilder.pm line 384
# Looks like your test exited with 11 just after 12.
t/db_dependent/TestBuilder.t .. Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 6/16 subtests
Test Summary Report
-------------------
t/db_dependent/TestBuilder.t (Wstat: 2816 Tests: 12 Failed: 2)
Failed tests: 3, 12
Non-zero exit status: 11
Parse errors: Bad plan. You planned 16 tests but ran 12.
Files=1, Tests=12, 16 wallclock secs ( 0.01 usr 0.01 sys + 11.46 cusr 3.43 csys = 14.91 CPU)
Result: FAIL
You forget the DBIC changes. It fixed all the failures. I pushed it to master. "Bug 35610: DBIC schema changes" (In reply to Jonathan Druart from comment #24) > You forget the DBIC changes. It fixed all the failures. I pushed it to > master. > > "Bug 35610: DBIC schema changes" I like easy fixes, thanks for the help :) Pushed to 23.11.x for 23.11.06 This 23.05.x dependencies, no backport. |