Bug 29336

Summary: Some authorised_value FKs are too short
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Architecture, internals, and plumbingAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: andrewfh, conan, david, fridolin.somers, jonathan.druart, kyle, m.de.rooy, victor
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This fixes the length of the field definitions in the database for several authorised_value and authorised_value_category columns as they are too short. It changes the value to varchar(32).
Version(s) released in:
22.05.00,21.11.03,21.05.11
Bug Depends on:    
Bug Blocks: 29347, 29390    
Attachments: Bug 29336: Identify the fields with wrong length (or name) first
Bug 29336: Database revision
Bug 29336: Database revision
Bug 29336: Identify the fields with wrong length (or name) first
Bug 29336: Database revision
Bug 29336: Allow to check on FK existence with field name
Bug 29336: Add a few missing FK constraints to authorised values
Bug 29336: Identify the fields with wrong length (or name) first
Bug 29336: Database revision
Bug 29336: Identify the fields with wrong length (or name) first
Bug 29336: Database revision
Bug 29336: Remove TODOs

Description Marcel de Rooy 2021-10-27 15:51:10 UTC
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)
Comment 1 Marcel de Rooy 2021-10-27 15:51:47 UTC
Hmm. The categories are a different .. category.
Comment 2 Marcel de Rooy 2021-10-28 11:52:42 UTC
(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.
Comment 3 Marcel de Rooy 2021-10-28 11:56:46 UTC
Bug 29347 is for renaming columns. Cool enhancement.
Comment 4 Marcel de Rooy 2021-10-28 12:25:33 UTC
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>
Comment 5 Marcel de Rooy 2021-10-28 12:25:36 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2021-10-28 12:28:04 UTC
Jonathan, wanna take this along with 21.11. Normal or major? At least an untidy inconsistency..
Comment 7 Marcel de Rooy 2021-10-28 12:32:05 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2021-10-28 12:32:55 UTC
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>
Comment 9 Jonathan Druart 2021-10-28 13:03:13 UTC
Why not adding the FK on the way?
Comment 10 Marcel de Rooy 2021-10-28 13:04:01 UTC
(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
Comment 11 Marcel de Rooy 2021-11-01 14:14:33 UTC
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>
Comment 12 Marcel de Rooy 2021-11-01 14:14:37 UTC
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>
Comment 13 Marcel de Rooy 2021-11-01 14:14:40 UTC
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>
Comment 14 Marcel de Rooy 2021-11-01 14:14:43 UTC
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>
Comment 15 Marcel de Rooy 2021-11-01 14:15:50 UTC
(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.
Comment 16 Marcel de Rooy 2021-11-01 14:17:49 UTC
Wait!
Comment 17 Marcel de Rooy 2021-11-02 12:29:15 UTC
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.
Comment 18 Marcel de Rooy 2021-11-02 12:39:14 UTC
Still nice to have in 21.11 ?
Comment 19 Jonathan Druart 2021-11-04 13:47:59 UTC
(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
Comment 20 Marcel de Rooy 2021-11-05 17:07:25 UTC
(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 ?
Comment 21 Jonathan Druart 2021-11-08 09:30:24 UTC
(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.
Comment 22 Marcel de Rooy 2021-12-17 09:09:21 UTC
(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.
Comment 23 David Nind 2021-12-17 12:30:44 UTC
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>
Comment 24 David Nind 2021-12-17 12:31:12 UTC
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>
Comment 25 David Nind 2021-12-17 13:01:50 UTC
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
Comment 26 Jonathan Druart 2022-01-07 15:03:49 UTC
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.
Comment 27 Jonathan Druart 2022-01-07 15:05:36 UTC
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>
Comment 28 Jonathan Druart 2022-01-07 15:05:41 UTC
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>
Comment 29 Jonathan Druart 2022-01-07 15:05:49 UTC
Created attachment 129189 [details] [review]
Bug 29336: Remove TODOs

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Fridolin Somers 2022-01-19 06:43:06 UTC
(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
Comment 31 Fridolin Somers 2022-01-19 07:20:41 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 32 Kyle M Hall 2022-02-07 11:32:24 UTC
Pushed to 21.11.x for 21.11.03
Comment 33 Andrew Fuerste-Henry 2022-02-16 16:59:47 UTC
Pushed to 21.05.x for 21.05.11
Comment 34 Victor Grousset/tuxayo 2022-02-21 08:50:54 UTC
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.
Comment 35 Marcel de Rooy 2022-09-15 08:50:09 UTC
*** Bug 6175 has been marked as a duplicate of this bug. ***