[Bug 29390] New: Authorised values: Why not adding the FK on the way?
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 Bug ID: 29390 Summary: Authorised values: Why not adding the FK on the way? Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: m.de.rooy@rijksmuseum.nl QA Contact: testopia@bugs.koha-community.org Because it is too much work :) Moving patches from 29336 to this one. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |29336 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29336 [Bug 29336] Some authorised_value FKs are too short -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |5334 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5334 [Bug 5334] Omnibus: Add explicit foreign key constraints -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 --- Comment #1 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 127214 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127214&action=edit Bug 29390: Allow to check on FK existence with field name Sometimes easier (or even safer) to check for FK on specific field name rather than constraint name (which may be created automatically). Test plan: Run t/db_dependent/Installer.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 --- Comment #2 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 127215 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127215&action=edit Bug 29390: Add a few missing FK constraints to authorised values Test plan: Run dbrev or new install. WORK IN PROGRESS -- POSTPONED 02-11-2021 * Fix constraint names * Fix table order -- 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=29390 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |m.de.rooy@rijksmuseum.nl |ity.org | CC| |jonathan.druart+koha@gmail. | |com Status|NEW |ASSIGNED --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Any advice, Jonathan, as to best way to proceed here ? -- 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=29390 --- Comment #4 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Marcel de Rooy from comment #3)
Any advice, Jonathan, as to best way to proceed here ?
About? The atomic updates look good to me. There are some typos I am going to fix. One thing maybe: installer/data/mysql/en/marcflavour/marc21/mandatory/marc21_framework_DEFAULT.yml: authorised_value: Unititle "Unititle" does not exist in the AV list of the sample data. What else would be needed? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 --- Comment #5 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 128985 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128985&action=edit Bug 29390: Fix syntax error in atomic update -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Coming back here soon. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 Bug 29390 depends on bug 29336, which changed state. Bug 29336 Summary: Some authorised_value FKs are too short https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29336 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to oldstable |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 --- Comment #7 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Marcel de Rooy from comment #6)
Coming back here soon.
Still interested in working on this? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #7)
(In reply to Marcel de Rooy from comment #6)
Coming back here soon.
Still interested in working on this?
Yes. Sorry, it dropped a bit on my list. But it is still there ;) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 --- Comment #9 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Ping? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Katrin Fischer from comment #9)
Ping?
Working on this right now. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127214|0 |1 is obsolete| | Attachment #127215|0 |1 is obsolete| | Attachment #128985|0 |1 is obsolete| | --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 152730 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152730&action=edit Bug 29390: Allow to check on FK existence with field name Sometimes easier (or even safer) to check for FK on specific field name rather than constraint name (which may be created automatically). Test plan: Run t/db_dependent/Installer.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 152731 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152731&action=edit Bug 29390: Add a few missing FK constraints to authorised values Test plan: See next patch. Resolving syntax error here. Run dbrev or new install. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 152732 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152732&action=edit Bug 29390: Fix syntax error in atomic update Test plan: Run dbrev or new install. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 152733 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152733&action=edit Bug 29390: Rename atomicupdate file Renamed bug number too. Added a count since an existing FK or an error may impact the count (not to mention running the dbrev multiple times). Remove Dumper. Test plan: If running it previously worked, run it again and see 0 added. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 152734 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152734&action=edit Bug 29390: Remove unexisting Unititle from marc21_framework_DEFAULT This applies to new installs only. If you run the dbrev, you may have seen: Found the following bad authorised values categories in marc_tag_structure: Unititle Have set these values in marc_tag_structure to NULL No separate test plan. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Authorised values: Why not |Authorised values: Add a |adding the FK on the way? |few missing foreign keys -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |The dbrev of this patch release notes| |adds the following (7) | |foreign keys to authorised | |value categories: | | | |additional_fields_authorise | |d_value_categories_fk | |auth_subfield_structure_aut | |horised_value_categories_fk | |auth_tag_structure_authoris | |ed_value_categories_fk | |borrower_attribute_types_au | |thorised_value_categories_f | |k | |club_template_enrollment | |_fields_authorised_value_ca | |tegories_fk | |club_template_fields_author | |ised_value_categories_fk | |marc_tag_structure_authoris | |ed_value_categories_fk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152730|0 |1 is obsolete| | --- Comment #16 from David Nind <david@davidnind.com> --- Created attachment 155529 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155529&action=edit Bug 29390: Allow to check on FK existence with field name Sometimes easier (or even safer) to check for FK on specific field name rather than constraint name (which may be created automatically). Test plan: Run t/db_dependent/Installer.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152731|0 |1 is obsolete| | --- Comment #17 from David Nind <david@davidnind.com> --- Created attachment 155530 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155530&action=edit Bug 29390: Add a few missing FK constraints to authorised values Test plan: See next patch. Resolving syntax error here. Run dbrev or new install. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152732|0 |1 is obsolete| | --- Comment #18 from David Nind <david@davidnind.com> --- Created attachment 155531 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155531&action=edit Bug 29390: Fix syntax error in atomic update Test plan: Run dbrev or new install. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152733|0 |1 is obsolete| | --- Comment #19 from David Nind <david@davidnind.com> --- Created attachment 155532 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155532&action=edit Bug 29390: Rename atomicupdate file Renamed bug number too. Added a count since an existing FK or an error may impact the count (not to mention running the dbrev multiple times). Remove Dumper. Test plan: If running it previously worked, run it again and see 0 added. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152734|0 |1 is obsolete| | --- Comment #20 from David Nind <david@davidnind.com> --- Created attachment 155533 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155533&action=edit Bug 29390: Remove unexisting Unititle from marc21_framework_DEFAULT This applies to new installs only. If you run the dbrev, you may have seen: Found the following bad authorised values categories in marc_tag_structure: Unititle Have set these values in marc_tag_structure to NULL No separate test plan. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #21 from David Nind <david@davidnind.com> --- This is how I tested the bug using koha-testing-docker: 1. Applied patch 2. Updated database - result: updatedatabase DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug29390.pl [05:14:16]: Bug 29390 - Add missing FK constraints to authorised value categories Found the following bad authorised values categories in marc_tag_structure: Unititle Have set these values in marc_tag_structure to NULL Added 7 foreign key constraints. 3. Ran flush_memcached and restart_all 4. Ran updatedatabase again - result: updatedatabase DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug29390.pl [05:15:11]: Bug 29390 - Add missing FK constraints to authorised value categories Added 0 foreign key constraints. 5. Tests passed: t/db_dependent/Installer.t Hope this is sufficient. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 --- Comment #22 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to David Nind from comment #21)
This is how I tested the bug using koha-testing-docker:
Thx David for testing ! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |emily.lamancusa@montgomeryc | |ountymd.gov QA Contact|testopia@bugs.koha-communit |emily.lamancusa@montgomeryc |y.org |ountymd.gov -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #23 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Generally the code looks good, and the dbrev is idempotent. One big concern, though - should all of the foreign keys be ON DELETE CASCADE? With the exception of additional_fields, the field is nullable, and when removing conflicts before applying the foreign key, the dbrev sets them to null. Shouldn't the foreign key constraint do the same if the authorized value is deleted? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 --- Comment #24 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I agree with Emily, I think we should set NULL and not delete on cascade here. + 'additional_fields' => 'authorised_value_category', + 'auth_subfield_structure' => 'authorised_value', + 'auth_tag_structure' => 'authorised_value', + 'borrower_attribute_types' => 'authorised_value_category', + 'club_template_enrollment_fields' => 'authorised_value_category', + 'club_template_fields' => 'authorised_value_category', + 'marc_tag_structure' => 'authorised_value', Especially the removals from the MARC bibliographic/authority data can lead to data loss easily if we remove the tag and subfield definitions. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 --- Comment #25 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Emily Lamancusa from comment #23)
Generally the code looks good, and the dbrev is idempotent.
One big concern, though - should all of the foreign keys be ON DELETE CASCADE? With the exception of additional_fields, the field is nullable, and when removing conflicts before applying the foreign key, the dbrev sets them to null. Shouldn't the foreign key constraint do the same if the authorized value is deleted?
Good catch. (In reply to Katrin Fischer from comment #24)
I agree with Emily, I think we should set NULL and not delete on cascade here.
+ 'additional_fields' => 'authorised_value_category', + 'auth_subfield_structure' => 'authorised_value', + 'auth_tag_structure' => 'authorised_value', + 'borrower_attribute_types' => 'authorised_value_category', + 'club_template_enrollment_fields' => 'authorised_value_category', + 'club_template_fields' => 'authorised_value_category', + 'marc_tag_structure' => 'authorised_value',
Especially the removals from the MARC bibliographic/authority data can lead to data loss easily if we remove the tag and subfield definitions.
Agreed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |35190 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35190 [Bug 35190] additional_fields table should allow null values for authorised_value_category -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 --- Comment #26 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Hmm, looking more at additional_fields, it doesn't make sense that its authorised_value_category field isn't nullable like the other tables. The UI explicitly has "None" as the default option when selecting an authorized value category. Seems to me like the best approach would be to make that nullable, then remove the special handling for additional_fields on this bug and simply make everything ON DELETE SET NULL. I filed bug 35190 for that and will upload a patch shortly. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 --- Comment #27 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Emily Lamancusa from comment #26)
Hmm, looking more at additional_fields, it doesn't make sense that its authorised_value_category field isn't nullable like the other tables. The UI explicitly has "None" as the default option when selecting an authorized value category.
Seems to me like the best approach would be to make that nullable, then remove the special handling for additional_fields on this bug and simply make everything ON DELETE SET NULL. I filed bug 35190 for that and will upload a patch shortly.
Thx for your follow-up. I will have a look at the other report too. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 Bug 29390 depends on bug 35190, which changed state. Bug 35190 Summary: Additional_fields table should allow null values for authorised_value_category https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35190 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29390 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|m.de.rooy@rijksmuseum.nl |koha-bugs@lists.koha-commun | |ity.org --- Comment #28 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Abandoning this report. Resetting Assignee. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org