Because it is too much work :) Moving patches from 29336 to this one.
Created attachment 127214 [details] [review] 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>
Created attachment 127215 [details] [review] 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
Any advice, Jonathan, as to best way to proceed here ?
(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?
Created attachment 128985 [details] [review] Bug 29390: Fix syntax error in atomic update
Coming back here soon.
(In reply to Marcel de Rooy from comment #6) > Coming back here soon. Still interested in working on this?
(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 ;)
Ping?
(In reply to Katrin Fischer from comment #9) > Ping? Working on this right now.
Created attachment 152730 [details] [review] 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>
Created attachment 152731 [details] [review] 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>
Created attachment 152732 [details] [review] 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>
Created attachment 152733 [details] [review] 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>
Created attachment 152734 [details] [review] 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>
Created attachment 155529 [details] [review] 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>
Created attachment 155530 [details] [review] 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>
Created attachment 155531 [details] [review] 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>
Created attachment 155532 [details] [review] 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>
Created attachment 155533 [details] [review] 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>
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.
(In reply to David Nind from comment #21) > This is how I tested the bug using koha-testing-docker: Thx David for testing !
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?
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.
(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.
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.
(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.