To mention a few: authorised_value | varchar(10) in authority framework additional fields `authorised_value_category` varchar(16) auth_tag_structure `authorised_value` varchar(10) COLLATE utf8mb4_unicode_ci DEFAULT NULL, borrower_attribute_types `authorised_value_category` varchar(32)
Hmm. The categories are a different .. category.
(In reply to Marcel de Rooy from comment #1) > Hmm. The categories are a different .. category. Actually not. There is a confusing difference betweeen value and category here. In most cases we mean the category but named the field value. Too bad. This report will only fix the lengths now first.
Bug 29347 is for renaming columns. Cool enhancement.
Created attachment 127051 [details] [review] Bug 29336: Identify the fields with wrong length (or name) first This report will fix the bad lengths first. And mark the 'bad' names with a TODO for follow-up. Bad name means: refers to value, should be category. Scanning kohastructure.sql. Note: Bug 29347 has been opened to fix the names. Test plan: Search for another AV category field with length<>32. Bonus: Reinstall and check database. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 127052 [details] [review] Bug 29336: Database revision Resizes the six fields identified earlier: additional_fields.authorised_value_category 16=>32 auth_subfield_structure.authorised_value 10=>32 auth_tag_structure.authorised_value 10=>32 club_template_enrollment_fields.authorised_value_category 16=>32 club_template_fields.authorised_value_category 16=>32 marc_tag_structure.authorised_value 10=>32 Test plan: Run dbrev, check database. Bonus: Connect an AV category with a long name to a authority subfield. Check if you see its values in the authority editor. (This was the way I found the problem.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Jonathan, wanna take this along with 21.11. Normal or major? At least an untidy inconsistency..
Oops submitting 2nd again
Created attachment 127053 [details] [review] Bug 29336: Database revision Resizes the six fields identified earlier: additional_fields.authorised_value_category 16=>32 auth_subfield_structure.authorised_value 10=>32 auth_tag_structure.authorised_value 10=>32 club_template_enrollment_fields.authorised_value_category 16=>32 club_template_fields.authorised_value_category 16=>32 marc_tag_structure.authorised_value 10=>32 Test plan: Run dbrev, check database. Bonus: Connect an AV category with a long name to a authority subfield. Check if you see its values in the authority editor. (This was the way I found the problem.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Why not adding the FK on the way?
(In reply to Jonathan Druart from comment #9) > Why not adding the FK on the way? Missed the missing ones :) Focused on length Will do
Created attachment 127169 [details] [review] Bug 29336: Identify the fields with wrong length (or name) first This report will fix the bad lengths first. And mark the 'bad' names with a TODO for follow-up. Bad name means: refers to value, should be category. Scanning kohastructure.sql. Note: Bug 29347 has been opened to fix the names. Test plan: Search for another AV category field with length<>32. Bonus: Reinstall and check database. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 127170 [details] [review] Bug 29336: Database revision Resizes the six fields identified earlier: additional_fields.authorised_value_category 16=>32 auth_subfield_structure.authorised_value 10=>32 auth_tag_structure.authorised_value 10=>32 club_template_enrollment_fields.authorised_value_category 16=>32 club_template_fields.authorised_value_category 16=>32 marc_tag_structure.authorised_value 10=>32 Test plan: Run dbrev, check database. Bonus: Connect an AV category with a long name to a authority subfield. Check if you see its values in the authority editor. (This was the way I found the problem.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 127171 [details] [review] Bug 29336: 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 127172 [details] [review] Bug 29336: Add a few missing FK constraints to authorised values Test plan: Run dbrev or new install. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #10) > (In reply to Jonathan Druart from comment #9) > > Why not adding the FK on the way? > > Missed the missing ones :) Focused on length > Will do Bit more work than anticipated. But there we go.
Wait!
I am spending too much time right now on this. I have stumbled over several things, might be too long constraint names, etc. Now in the installer the order of the tables must be rearranged in kohastructure to get FKs in. Will move the FK stuff to another report.
Still nice to have in 21.11 ?
(In reply to Marcel de Rooy from comment #18) > Still nice to have in 21.11 ? I would prefer to not rush it and wait for 22.05
(In reply to Jonathan Druart from comment #19) > (In reply to Marcel de Rooy from comment #18) > > Still nice to have in 21.11 ? > > I would prefer to not rush it and wait for 22.05 Could you please tell me what is the fundamental difference with the patches on bug 29386, that got pushed today ?
(In reply to Marcel de Rooy from comment #20) > (In reply to Jonathan Druart from comment #19) > > (In reply to Marcel de Rooy from comment #18) > > > Still nice to have in 21.11 ? > > > > I would prefer to not rush it and wait for 22.05 > > Could you please tell me what is the fundamental difference with the patches > on bug 29386, that got pushed today ? Bug 29386 fixes a critical bug that must be backported into stable branches ASAP. This one should be pushed along with bug 29390 in my opinion.
(In reply to Jonathan Druart from comment #21) > This one should be pushed along with bug 29390 in my opinion. It can go on its own.
Created attachment 128655 [details] [review] Bug 29336: Identify the fields with wrong length (or name) first This report will fix the bad lengths first. And mark the 'bad' names with a TODO for follow-up. Bad name means: refers to value, should be category. Scanning kohastructure.sql. Note: Bug 29347 has been opened to fix the names. Test plan: Search for another AV category field with length<>32. Bonus: Reinstall and check database. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 128656 [details] [review] Bug 29336: Database revision Resizes the six fields identified earlier: additional_fields.authorised_value_category 16=>32 auth_subfield_structure.authorised_value 10=>32 auth_tag_structure.authorised_value 10=>32 club_template_enrollment_fields.authorised_value_category 16=>32 club_template_fields.authorised_value_category 16=>32 marc_tag_structure.authorised_value 10=>32 Test plan: Run dbrev, check database. Bonus: Connect an AV category with a long name to a authority subfield. Check if you see its values in the authority editor. (This was the way I found the problem.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com>
Testing notes (koha-testing-docker): - Access the database and use describe <table-name>; to show current definitions - See the database update for the table names - Run the tests - should pass - Apply the patch - Run databaseupdate - Access the database again and see that the definitions are updated - reset_all - Access the database again and see that the definitions are updated - Run the tests again - should pass
All good for me, however I would remove the comments you added in the kohastructure.sql. We usually don't adjust the comment in the atomic update, so people we still hve the TODO even if we fix the name of the column.
Created attachment 129187 [details] [review] Bug 29336: Identify the fields with wrong length (or name) first This report will fix the bad lengths first. And mark the 'bad' names with a TODO for follow-up. Bad name means: refers to value, should be category. Scanning kohastructure.sql. Note: Bug 29347 has been opened to fix the names. Test plan: Search for another AV category field with length<>32. Bonus: Reinstall and check database. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 129188 [details] [review] Bug 29336: Database revision Resizes the six fields identified earlier: additional_fields.authorised_value_category 16=>32 auth_subfield_structure.authorised_value 10=>32 auth_tag_structure.authorised_value 10=>32 club_template_enrollment_fields.authorised_value_category 16=>32 club_template_fields.authorised_value_category 16=>32 marc_tag_structure.authorised_value 10=>32 Test plan: Run dbrev, check database. Bonus: Connect an AV category with a long name to a authority subfield. Check if you see its values in the authority editor. (This was the way I found the problem.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 129189 [details] [review] Bug 29336: Remove TODOs Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Marcel de Rooy from comment #22) > (In reply to Jonathan Druart from comment #21) > > > This one should be pushed along with bug 29390 in my opinion. > > It can go on its own. I agree, so pushing master
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Pushed to 21.11.x for 21.11.03
Pushed to 21.05.x for 21.05.11
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.
*** Bug 6175 has been marked as a duplicate of this bug. ***