Bug 29390 - Authorised values: Add a few missing foreign keys
Summary: Authorised values: Add a few missing foreign keys
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Emily Lamancusa
URL:
Keywords:
Depends on: 29336 35190
Blocks: 5334
  Show dependency treegraph
 
Reported: 2021-11-02 12:28 UTC by Marcel de Rooy
Modified: 2023-10-31 12:11 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
The dbrev of this patch adds the following (7) foreign keys to authorised value categories: additional_fields_authorised_value_categories_fk auth_subfield_structure_authorised_value_categories_fk auth_tag_structure_authorised_value_categories_fk borrower_attribute_types_authorised_value_categories_fk club_template_enrollment_fields_authorised_value_categories_fk club_template_fields_authorised_value_categories_fk marc_tag_structure_authorised_value_categories_fk
Version(s) released in:


Attachments
Bug 29390: Allow to check on FK existence with field name (2.19 KB, patch)
2021-11-02 12:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29390: Add a few missing FK constraints to authorised values (8.95 KB, patch)
2021-11-02 12:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29390: Fix syntax error in atomic update (1.36 KB, patch)
2022-01-04 13:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29390: Allow to check on FK existence with field name (2.19 KB, patch)
2023-06-27 12:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29390: Add a few missing FK constraints to authorised values (8.92 KB, patch)
2023-06-27 12:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29390: Fix syntax error in atomic update (1.49 KB, patch)
2023-06-27 12:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29390: Rename atomicupdate file (1.82 KB, patch)
2023-06-27 12:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29390: Remove unexisting Unititle from marc21_framework_DEFAULT (1.32 KB, patch)
2023-06-27 12:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29390: Allow to check on FK existence with field name (2.20 KB, patch)
2023-09-12 05:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 29390: Add a few missing FK constraints to authorised values (8.93 KB, patch)
2023-09-12 05:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 29390: Fix syntax error in atomic update (1.50 KB, patch)
2023-09-12 05:19 UTC, David Nind
Details | Diff | Splinter Review
Bug 29390: Rename atomicupdate file (1.83 KB, patch)
2023-09-12 05:19 UTC, David Nind
Details | Diff | Splinter Review
Bug 29390: Remove unexisting Unititle from marc21_framework_DEFAULT (1.32 KB, patch)
2023-09-12 05:19 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2021-11-02 12:28:46 UTC
Because it is too much work :)

Moving patches from 29336 to this one.
Comment 1 Marcel de Rooy 2021-11-02 12:32:57 UTC
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>
Comment 2 Marcel de Rooy 2021-11-02 12:33:01 UTC
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
Comment 3 Marcel de Rooy 2021-11-02 12:33:51 UTC
Any advice, Jonathan, as to best way to proceed here ?
Comment 4 Jonathan Druart 2022-01-04 13:20:52 UTC
(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?
Comment 5 Jonathan Druart 2022-01-04 13:21:35 UTC
Created attachment 128985 [details] [review]
Bug 29390: Fix syntax error in atomic update
Comment 6 Marcel de Rooy 2022-01-10 15:41:49 UTC
Coming back here soon.
Comment 7 Jonathan Druart 2022-11-17 06:29:32 UTC
(In reply to Marcel de Rooy from comment #6)
> Coming back here soon.

Still interested in working on this?
Comment 8 Marcel de Rooy 2022-11-17 06:31:24 UTC
(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 ;)
Comment 9 Katrin Fischer 2023-06-24 21:30:38 UTC
Ping?
Comment 10 Marcel de Rooy 2023-06-26 13:12:06 UTC
(In reply to Katrin Fischer from comment #9)
> Ping?

Working on this right now.
Comment 11 Marcel de Rooy 2023-06-27 12:53:30 UTC
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>
Comment 12 Marcel de Rooy 2023-06-27 12:53:32 UTC
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>
Comment 13 Marcel de Rooy 2023-06-27 12:53:35 UTC
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>
Comment 14 Marcel de Rooy 2023-06-27 12:53:37 UTC
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>
Comment 15 Marcel de Rooy 2023-06-27 12:53:40 UTC
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>
Comment 16 David Nind 2023-09-12 05:18:41 UTC
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>
Comment 17 David Nind 2023-09-12 05:18:54 UTC
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>
Comment 18 David Nind 2023-09-12 05:19:12 UTC
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>
Comment 19 David Nind 2023-09-12 05:19:20 UTC
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>
Comment 20 David Nind 2023-09-12 05:19:42 UTC
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>
Comment 21 David Nind 2023-09-12 05:22:29 UTC
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.
Comment 22 Marcel de Rooy 2023-09-12 07:15:13 UTC
(In reply to David Nind from comment #21)
> This is how I tested the bug using koha-testing-docker:

Thx David for testing !
Comment 23 Emily Lamancusa 2023-10-27 21:29:46 UTC
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?
Comment 24 Katrin Fischer 2023-10-28 22:27:22 UTC
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.
Comment 25 Marcel de Rooy 2023-10-30 08:07:32 UTC
(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.
Comment 26 Emily Lamancusa 2023-10-30 15:08:43 UTC
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.
Comment 27 Marcel de Rooy 2023-10-31 12:11:00 UTC
(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.