The way we manage authorized values is not consistent: Some are present in the DB, others are not (see bug 15799 comment 4 for more details). Worse, most of the time the FK does not exist (and datatype is not consistent: varchar 32 vs 20, 16,...). To help cleaning up this area, a new table to store authorized value categories would be helpful.
Created attachment 54979 [details] [review] Bug 17216: Add Koha::AuthorisedValueCategory|ies classes
Created attachment 54980 [details] [review] Bug 17216: Add new table authorised_value_categories and populate it This patch set adds a new table authorised_value_categories to store authori(s|z)ed value categories into a separate table. The problematic is explained on bug 15799 comment 4: We need FK to the AV categories but some may not have authorized values yet. What does this patch set: - Add a new authorised_value_categories table - Populate it with known categories - Update FK items_search_fields.authorised_values_category - Create a new FK marc_subfield_structure.authorised_value (FIXME should be authorised_value_categories instead) They are some problems this patch set do not take into account: - The .sql installer files won't insert correctly (will have to be updated when this patch set will be ready to be pushed) - All the categories (even the ones without authorized values defined) are listed when you edit frameworks (marc_subfield_structure.pl) - There is no way to delete a category (TODO). But to do so it would be good to have a authorised_value_categories.is_internal field to mark some categories as "cannot be deleted". Test plan: 0/ Execute the DB entry to create and populate the new table and set the FK 1/ Create a new AV category from the admin module (admin/authorised_values.pl) 2/ Add/edit subfield linked to a AV category (admin/marc_subfield_structure.pl) 3/ You won't be allowed to add AV for branches, itemtypes or cn_source. They are used internally.
Created attachment 54981 [details] [review] Bug 17216: Add some other AV categories Found in installer/data/mysql/en/marcflavour/marc21/mandatory/marc21_framework_DEFAULT.sql
Created attachment 54982 [details] [review] Bug 17216: DBIC Schema changes
Created attachment 54983 [details] [review] Bug 17216: Update the admin interface
Created attachment 54984 [details] [review] Bug 17216: Add FK on mss.authorised_value
Created attachment 54985 [details] [review] Bug 17216: Use Koha::AVC from mss.pl
Created attachment 54987 [details] [review] Bug 17216: Add Koha::AuthorisedValueCategory|ies classes
Created attachment 54988 [details] [review] Bug 17216: Add new table authorised_value_categories and populate it This patch set adds a new table authorised_value_categories to store authori(s|z)ed value categories into a separate table. The problematic is explained on bug 15799 comment 4: We need FK to the AV categories but some may not have authorized values yet. What does this patch set: - Add a new authorised_value_categories table - Populate it with known categories - Update FK items_search_fields.authorised_values_category - Create a new FK marc_subfield_structure.authorised_value (FIXME should be authorised_value_categories instead) They are some problems this patch set do not take into account: - The .sql installer files won't insert correctly (will have to be updated when this patch set will be ready to be pushed) - All the categories (even the ones without authorized values defined) are listed when you edit frameworks (marc_subfield_structure.pl) - There is no way to delete a category (TODO). But to do so it would be good to have a authorised_value_categories.is_internal field to mark some categories as "cannot be deleted". Test plan: 0/ Execute the DB entry to create and populate the new table and set the FK 1/ Create a new AV category from the admin module (admin/authorised_values.pl) 2/ Add/edit subfield linked to a AV category (admin/marc_subfield_structure.pl) 3/ You won't be allowed to add AV for branches, itemtypes or cn_source. They are used internally.
Created attachment 54989 [details] [review] Bug 17216: Add some other AV categories Found in installer/data/mysql/en/marcflavour/marc21/mandatory/marc21_framework_DEFAULT.sql
Created attachment 54990 [details] [review] Bug 17216: DBIC Schema changes
Created attachment 54991 [details] [review] Bug 17216: Update the admin interface
Created attachment 54992 [details] [review] Bug 17216: Add FK on mss.authorised_value
Created attachment 54993 [details] [review] Bug 17216: Use Koha::AVC from mss.pl
- bug_17216_2_add_fk.sql is still missing a semicolon on the 'UPDATE marc_subfield_structure' line. - Shouldn't category be required in the "New category" form? - In testing the effect of these patches on marc_subfield_structure.pl, what should we be looking for? Simply that the categories show up correctly?
Created attachment 55003 [details] [review] Bug 17216: Add FK on mss.authorised_value
Created attachment 55004 [details] [review] Bug 17216: Use Koha::AVC from mss.pl
Created attachment 55005 [details] [review] Bug 17216: Category name is required when adding a new AV cat
(In reply to Owen Leonard from comment #15) > - bug_17216_2_add_fk.sql is still missing a semicolon on the 'UPDATE > marc_subfield_structure' line. Oops, sorry about that. I have tested the file directly in the CLI, line by line, that's why I did not spot it. > - Shouldn't category be required in the "New category" form? Yep, done. > - In testing the effect of these patches on marc_subfield_structure.pl, what > should we be looking for? Simply that the categories show up correctly? Yes. You will also think about the structure changes: now all the AV cat is displayed, when before only the AV cat *in used* were displayed. Think also about the "branches", "itemtypes" and "cn_source". does that make sense to have them in the table, etc.
Created attachment 55008 [details] [review] Bug 17216: Add FK on authorised_values.category
Created attachment 55009 [details] [review] Bug 17216: Update DBIC Schema
Created attachment 55010 [details] [review] Bug 17216: Update DBIC schema for MSS
Created attachment 55011 [details] [review] Bug 17216: Fix tests
Created attachment 55021 [details] [review] [SIGNED-OFF] Bug 17216: Add Koha::AuthorisedValueCategory|ies classes Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55022 [details] [review] [SIGNED-OFF] Bug 17216: Add new table authorised_value_categories and populate it This patch set adds a new table authorised_value_categories to store authori(s|z)ed value categories into a separate table. The problematic is explained on bug 15799 comment 4: We need FK to the AV categories but some may not have authorized values yet. What does this patch set: - Add a new authorised_value_categories table - Populate it with known categories - Update FK items_search_fields.authorised_values_category - Create a new FK marc_subfield_structure.authorised_value (FIXME should be authorised_value_categories instead) They are some problems this patch set do not take into account: - The .sql installer files won't insert correctly (will have to be updated when this patch set will be ready to be pushed) - All the categories (even the ones without authorized values defined) are listed when you edit frameworks (marc_subfield_structure.pl) - There is no way to delete a category (TODO). But to do so it would be good to have a authorised_value_categories.is_internal field to mark some categories as "cannot be deleted". Test plan: 0/ Execute the DB entry to create and populate the new table and set the FK 1/ Create a new AV category from the admin module (admin/authorised_values.pl) 2/ Add/edit subfield linked to a AV category (admin/marc_subfield_structure.pl) 3/ You won't be allowed to add AV for branches, itemtypes or cn_source. They are used internally. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55023 [details] [review] [SIGNED-OFF] Bug 17216: Add some other AV categories Found in installer/data/mysql/en/marcflavour/marc21/mandatory/marc21_framework_DEFAULT.sql Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55024 [details] [review] [SIGNED-OFF] Bug 17216: DBIC Schema changes Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55025 [details] [review] [SIGNED-OFF] Bug 17216: Update the admin interface Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55026 [details] [review] [SIGNED-OFF] Bug 17216: Add FK on mss.authorised_value Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55027 [details] [review] [SIGNED-OFF] Bug 17216: Use Koha::AVC from mss.pl Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55028 [details] [review] [SIGNED-OFF] Bug 17216: Category name is required when adding a new AV cat Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55029 [details] [review] [SIGNED-OFF] Bug 17216: Add FK on authorised_values.category Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55030 [details] [review] [SIGNED-OFF] Bug 17216: Update DBIC Schema Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55031 [details] [review] [SIGNED-OFF] Bug 17216: Update DBIC schema for MSS Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55032 [details] [review] [SIGNED-OFF] Bug 17216: Fix tests Signed-off-by: Owen Leonard <oleonard@myacpl.org>
These patches work well for me from the point of view of an end user. The patches apply, QA tools don't complain, and database updates correctly. I have followed the test plan successfully.
Created attachment 55047 [details] [review] Bug 17216: Fix other tests
could not apply patch to test bug 17248. CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/authorised_values.tt Failed to merge in the changes.
Created attachment 55207 [details] [review] Bug 17216: Add Koha::AuthorisedValueCategory|ies classes Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55208 [details] [review] Bug 17216: Add new table authorised_value_categories and populate it This patch set adds a new table authorised_value_categories to store authori(s|z)ed value categories into a separate table. The problematic is explained on bug 15799 comment 4: We need FK to the AV categories but some may not have authorized values yet. What does this patch set: - Add a new authorised_value_categories table - Populate it with known categories - Update FK items_search_fields.authorised_values_category - Create a new FK marc_subfield_structure.authorised_value (FIXME should be authorised_value_categories instead) They are some problems this patch set do not take into account: - The .sql installer files won't insert correctly (will have to be updated when this patch set will be ready to be pushed) - All the categories (even the ones without authorized values defined) are listed when you edit frameworks (marc_subfield_structure.pl) - There is no way to delete a category (TODO). But to do so it would be good to have a authorised_value_categories.is_internal field to mark some categories as "cannot be deleted". Test plan: 0/ Execute the DB entry to create and populate the new table and set the FK 1/ Create a new AV category from the admin module (admin/authorised_values.pl) 2/ Add/edit subfield linked to a AV category (admin/marc_subfield_structure.pl) 3/ You won't be allowed to add AV for branches, itemtypes or cn_source. They are used internally. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55209 [details] [review] Bug 17216: Add some other AV categories Found in installer/data/mysql/en/marcflavour/marc21/mandatory/marc21_framework_DEFAULT.sql Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55210 [details] [review] Bug 17216: DBIC Schema changes Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55211 [details] [review] Bug 17216: Update the admin interface Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55212 [details] [review] Bug 17216: Add FK on mss.authorised_value Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55213 [details] [review] Bug 17216: Use Koha::AVC from mss.pl Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55214 [details] [review] Bug 17216: Add FK on authorised_values.category Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55215 [details] [review] Bug 17216: Update DBIC Schema Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55216 [details] [review] Bug 17216: Update DBIC schema for MSS Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55217 [details] [review] Bug 17216: Fix tests Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55218 [details] [review] Bug 17216: Fix other tests
Last patch set fixes conflict with bug 11019.
Created attachment 55365 [details] [review] Bug 17216: Add Koha::AuthorisedValueCategory|ies classes Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55366 [details] [review] Bug 17216: Add new table authorised_value_categories and populate it This patch set adds a new table authorised_value_categories to store authori(s|z)ed value categories into a separate table. The problematic is explained on bug 15799 comment 4: We need FK to the AV categories but some may not have authorized values yet. What does this patch set: - Add a new authorised_value_categories table - Populate it with known categories - Update FK items_search_fields.authorised_values_category - Create a new FK marc_subfield_structure.authorised_value (FIXME should be authorised_value_categories instead) They are some problems this patch set do not take into account: - The .sql installer files won't insert correctly (will have to be updated when this patch set will be ready to be pushed) - All the categories (even the ones without authorized values defined) are listed when you edit frameworks (marc_subfield_structure.pl) - There is no way to delete a category (TODO). But to do so it would be good to have a authorised_value_categories.is_internal field to mark some categories as "cannot be deleted". Test plan: 0/ Execute the DB entry to create and populate the new table and set the FK 1/ Create a new AV category from the admin module (admin/authorised_values.pl) 2/ Add/edit subfield linked to a AV category (admin/marc_subfield_structure.pl) 3/ You won't be allowed to add AV for branches, itemtypes or cn_source. They are used internally. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55367 [details] [review] Bug 17216: Add some other AV categories Found in installer/data/mysql/en/marcflavour/marc21/mandatory/marc21_framework_DEFAULT.sql Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55368 [details] [review] Bug 17216: DBIC Schema changes Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55369 [details] [review] Bug 17216: Update the admin interface Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55370 [details] [review] Bug 17216: Add FK on mss.authorised_value Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55371 [details] [review] Bug 17216: Use Koha::AVC from mss.pl Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55372 [details] [review] Bug 17216: Add FK on authorised_values.category Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55373 [details] [review] Bug 17216: Update DBIC Schema Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55374 [details] [review] Bug 17216: Update DBIC schema for MSS Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55375 [details] [review] Bug 17216: Fix tests Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55376 [details] [review] Bug 17216: Fix other tests
Hi Jonathan, mostly good - please take a look: Administration > Authorized values 1) I think the 'virtual' authorized values branches, itemtypes and cn_sort should not show up in the authorized value list in administration. It's not a nice effect that it looks like you can add to them and then get an error message - we should avoid that kind of user experience. 2) Small typo when adding to 'branches'. Should be: The authorized value categories 'branches', 'itemtypes' and 'cn_source' are used internally by Koha and are not valid. 3) If you typo in an authorized value category, there is no way to remove it. That can get annoying pretty fast. We should offer a way to delete an empty authorized value category (maybe if not empty too?). 4) The new empty option in the pull down list leads to some confusing display: Authorized values for category : There are no authorized values defined for Editing items 5) There is a problem with the STACK on 952$j as this doesn't exist. Should we clean it up by removing it from the frameworks if there are no values defined for it? Authority types 6) Should we add the same FK relationship there? (separate bug?) Successfully tested: - Bibliographic MARC frameworks - Cataloguing - Patron attribute types / Patrons
Created attachment 55692 [details] [review] Bug 17216: Do not display branches, itemtypes and cn_source
Created attachment 55693 [details] [review] Bug 17216: Do no display the empty string category name
(In reply to Katrin Fischer from comment #64) > Hi Jonathan, mostly good - please take a look: > > Administration > Authorized values > > 1) I think the 'virtual' authorized values branches, itemtypes and cn_sort > should not show up in the authorized value list in administration. It's not > a nice effect that it looks like you can add to them and then get an error > message - we should avoid that kind of user experience. Done, removed. > 2) Small typo when adding to 'branches'. Should be: > The authorized value categories 'branches', 'itemtypes' and 'cn_source' are > used internally by Koha and are not valid. Done, thanks. > 3) If you typo in an authorized value category, there is no way to remove > it. That can get annoying pretty fast. We should offer a way to delete an > empty authorized value category (maybe if not empty too?). Yes, it's in the TODO section of the commit msg. > 4) The new empty option in the pull down list leads to some confusing > display: > Authorized values for category : > There are no authorized values defined for Fixed, the empty one is no longer displayed. > Editing items > > 5) There is a problem with the STACK on 952$j as this doesn't exist. Should > we clean it up by removing it from the frameworks if there are no values > defined for it? I have not found regression with master. So let's deal with it on another bug report. > Authority types > 6) Should we add the same FK relationship there? (separate bug?) Yes, and others as well. But that should be done on another bug report.
Hi Jonathan, this is looking good, but I found something a bit disturbing (to me :) ): Try to add an authorized value category "Branches". The system tells you, that the category already exists. But: It will allow you then to add a new value to it. Can we show the "used internally message" and now allow to do that? I will move the "delete category" to a separate bug.
Created attachment 55835 [details] [review] Bug 17216: Internal AV should be correctly handled when creating a new category
Cool, thx Jonathan - one last thing - could you take a look at the installer files? You noted that before this is pushed we need to fix the files there. I think this is really close to PQA now. Could we handle this like sysprefs.sql having one file on top level used for all? I think we got no need for translations here.
Created attachment 55840 [details] [review] Bug 17216: Move the AV cat list file to the top level This way it will get used for any languages.
(In reply to Katrin Fischer from comment #70) > Cool, thx Jonathan - one last thing - could you take a look at the installer > files? > > You noted that before this is pushed we need to fix the files there. I think > this is really close to PQA now. > > Could we handle this like sysprefs.sql having one file on top level used for > all? I think we got no need for translations here. Done, see the last patch. You can use the script from bug 17360 to test it.
Created attachment 56202 [details] [review] [PASSED QA] Bug 17216: Add Koha::AuthorisedValueCategory|ies classes Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 56203 [details] [review] [PASSED QA] Bug 17216: Add new table authorised_value_categories and populate it This patch set adds a new table authorised_value_categories to store authori(s|z)ed value categories into a separate table. The problematic is explained on bug 15799 comment 4: We need FK to the AV categories but some may not have authorized values yet. What does this patch set: - Add a new authorised_value_categories table - Populate it with known categories - Update FK items_search_fields.authorised_values_category - Create a new FK marc_subfield_structure.authorised_value (FIXME should be authorised_value_categories instead) They are some problems this patch set do not take into account: - The .sql installer files won't insert correctly (will have to be updated when this patch set will be ready to be pushed) - All the categories (even the ones without authorized values defined) are listed when you edit frameworks (marc_subfield_structure.pl) - There is no way to delete a category (TODO). But to do so it would be good to have a authorised_value_categories.is_internal field to mark some categories as "cannot be deleted". Test plan: 0/ Execute the DB entry to create and populate the new table and set the FK 1/ Create a new AV category from the admin module (admin/authorised_values.pl) 2/ Add/edit subfield linked to a AV category (admin/marc_subfield_structure.pl) 3/ You won't be allowed to add AV for branches, itemtypes or cn_source. They are used internally. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 56204 [details] [review] [PASSED QA] Bug 17216: Add some other AV categories Found in installer/data/mysql/en/marcflavour/marc21/mandatory/marc21_framework_DEFAULT.sql Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 56205 [details] [review] [PASSED QA] Bug 17216: DBIC Schema changes Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 56206 [details] [review] [PASSED QA] Bug 17216: Update the admin interface Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 56207 [details] [review] [PASSED QA] Bug 17216: Add FK on mss.authorised_value Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 56208 [details] [review] [PASSED QA] Bug 17216: Use Koha::AVC from mss.pl Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 56209 [details] [review] [PASSED QA] Bug 17216: Add FK on authorised_values.category Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 56210 [details] [review] [PASSED QA] Bug 17216: Update DBIC Schema Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 56211 [details] [review] [PASSED QA] Bug 17216: Update DBIC schema for MSS Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 56212 [details] [review] [PASSED QA] Bug 17216: Fix tests Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 56213 [details] [review] [PASSED QA] Bug 17216: Fix other tests Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 56214 [details] [review] [PASSED QA] Bug 17216: Do not display branches, itemtypes and cn_source Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 56215 [details] [review] [PASSED QA] Bug 17216: Do no display the empty string category name Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 56216 [details] [review] [PASSED QA] Bug 17216: Internal AV should be correctly handled when creating a new category Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 56217 [details] [review] [PASSED QA] Bug 17216: Move the AV cat list file to the top level This way it will get used for any languages. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Pushed to Master - Should be in the November 2016 Release. Thanks!
Created attachment 56619 [details] [review] Bug 17216: Revert changes to ItemsSearchField schema file
(In reply to Jonathan Druart from comment #90) > Created attachment 56619 [details] [review] [review] > Bug 17216: Revert changes to ItemsSearchField schema file @RM, could you please push this follow-up please? It seems that I have generated a wrong file (80 vs 32).
(In reply to Jonathan Druart from comment #91) > (In reply to Jonathan Druart from comment #90) > > Created attachment 56619 [details] [review] [review] [review] > > Bug 17216: Revert changes to ItemsSearchField schema file > > @RM, could you please push this follow-up please? It seems that I have > generated a wrong file (80 vs 32). Thanks! Pushed
With a Debian package for 16.11, on upgrade from 16.05 the following happens: Upgrade to 16.06.00.032 done (Bug 17357 - WTHDRAWN is still used in installer files) DBD::mysql::db do failed: Can't create table 'koha_upgrade.#sql-329_a5' (errno: 150) [for Statement " ALTER TABLE marc_subfield_structure MODIFY COLUMN authorised_value VARCHAR(32) DEFAULT NULL, ADD CONSTRAINT marc_subfield_structure_ibfk_1 FOREIGN KEY (authorised_value) REFERENCES authorised_value_categories (category_name) ON UPDATE CASCADE ON DELETE SET NULL; "] at /usr/share/koha/intranet/cgi-bin/installer/data/mysql/updatedatabase.pl line 13175. Upgrade to 16.06.00.033 done (Bug 17216 - Add a new table to store authorized value categories) I assume it is related to this bug? Is it safe to ignore? It does sound more scary than the duplicate warnings we get from time to time.
FYI: I'll wait with the package release for 16.11.00 until this is solved. :)
Created attachment 57729 [details] [review] Bug 17216: Fix SQL statement to setting marc_subfield_structure.authorised_value to NULL We want to update it if it's empty, not equal to ';' nopaste vim mode is to blame
(In reply to Mirko Tietgen from comment #93) > With a Debian package for 16.11, on upgrade from 16.05 the following happens: > > Upgrade to 16.06.00.032 done (Bug 17357 - WTHDRAWN is still used in > installer files) > DBD::mysql::db do failed: Can't create table 'koha_upgrade.#sql-329_a5' > (errno: 150) [for Statement " > ALTER TABLE marc_subfield_structure > MODIFY COLUMN authorised_value VARCHAR(32) DEFAULT NULL, > ADD CONSTRAINT marc_subfield_structure_ibfk_1 FOREIGN KEY > (authorised_value) REFERENCES authorised_value_categories (category_name) ON > UPDATE CASCADE ON DELETE SET NULL; > "] at > /usr/share/koha/intranet/cgi-bin/installer/data/mysql/updatedatabase.pl line > 13175. > Upgrade to 16.06.00.033 done (Bug 17216 - Add a new table to store > authorized value categories) > > I assume it is related to this bug? Is it safe to ignore? It does sound more > scary than the duplicate warnings we get from time to time. Got the same error in my test setup. For undetermined reason, authorised_value field in marc_subfield_structure table did not have utf8_unicode_ci collation. I replaced MODIFY COLUMN authorised_value VARCHAR(32) DEFAULT NULL, with MODIFY COLUMN authorised_value VARCHAR(32) CHARACTER SET utf8 COLLATE utf8_unicode_ci DEFAULT NULL, and after that it seems to work fine. No idea why this DB update is tagged with "Bug 17357 - WTHDRAWN is still used in installer files)" and not with a "Bug 17216 ..." instead. BTW, there are some other text fields in my test DB with inconsistent collation too, but this is a database with a long history of accumulated DB updates (including various unstable / atomic ones).
>Got the same error in my test setup. For undetermined reason, authorised_value >field in marc_subfield_structure table did not have utf8_unicode_ci collation. That's intentional. ci = case insensitive breaks uppercase subfield codes which are often used to create additional fields in 952.
(In reply to Katrin Fischer from comment #97) > >Got the same error in my test setup. For undetermined reason, authorised_value >field in marc_subfield_structure table did not have utf8_unicode_ci collation. > > That's intentional. ci = case insensitive breaks uppercase subfield codes > which are often used to create additional fields in 952. marc_subfield_structure.tagsubfield indeed has utf8_bin collation, but should the same rule apply for authorised_value? By default (in fresh installs), marc_subfield_structure.authorised_value has utf8_unicode_ci collation, for some reasons (I still have no idea why, maybe update from Bug 11944 partially failed) in my old test DB, this field has no collation defined, and that's why adding FK failed for me with error 150. But it can be failing for different reason[s] for other people - and probably is, this collation issue seems to be some kind of problem which was specific to this particular DB. There is one other table (overduerules) in that DB which does not have utf8_unicode_ci collation set by default for all text fields, but it looks more like an omission in updatedatabase (Bug 13624, DB version: 3.23.00.007).
(In reply to Jacek Ablewicz from comment #96) > No idea why this DB update is tagged > with "Bug 17357 - WTHDRAWN is still used in installer files)" and not with a > "Bug 17216 ..." instead. Uhm, scratch that; in fact it is tagged correctly
Huh, this is what happens (mysql 5.5.49): for a table with no default collation defined on table level (like marc_subfield_structure) MODIFY COLUMN authorised_value VARCHAR(32) DEFAULT NULL does not preserve existing collation for that field (which was, after all, utf8_unicode_ci in my test DB before attempting that upgrade: manually set up to utf8_unicode_ci for a bunch of marc_subfield_structure fields in DB update from Bug 11944), and it subsequently fails with error 150 on adding FK constraint because of collations inconsistency (I think.. not quite sure, but a solution from comment #96 fixes this problem). It's reproductible; note that I have different default collation defined mysql server-side on that box (utf8_general_ci) - if it was utf8_unicode_ci server-side, I guess there is a possibility this issue does not manifest itself (?).
Did an install with the latest patch added, got Update errors : [Thu Nov 24 00:06:08 2016] updatedatabase.pl: DBD::mysql::db do failed: Duplicate column name 'updated_on' [for Statement " [Thu Nov 24 00:06:08 2016] updatedatabase.pl: ALTER TABLE borrowers [Thu Nov 24 00:06:08 2016] updatedatabase.pl: ADD COLUMN updated_on timestamp NULL DEFAULT CURRENT_TIMESTAMP [Thu Nov 24 00:06:08 2016] updatedatabase.pl: ON UPDATE CURRENT_TIMESTAMP [Thu Nov 24 00:06:08 2016] updatedatabase.pl: AFTER privacy_guarantor_checkouts; [Thu Nov 24 00:06:08 2016] updatedatabase.pl: "] at /usr/share/koha/intranet/cgi-bin/installer/data/mysql/updatedatabase.pl line 12626. [Thu Nov 24 00:06:08 2016] updatedatabase.pl: DBD::mysql::db do failed: Duplicate column name 'updated_on' [for Statement " [Thu Nov 24 00:06:08 2016] updatedatabase.pl: ALTER TABLE deletedborrowers [Thu Nov 24 00:06:08 2016] updatedatabase.pl: ADD COLUMN updated_on timestamp NULL DEFAULT CURRENT_TIMESTAMP [Thu Nov 24 00:06:08 2016] updatedatabase.pl: ON UPDATE CURRENT_TIMESTAMP [Thu Nov 24 00:06:08 2016] updatedatabase.pl: AFTER privacy_guarantor_checkouts; [Thu Nov 24 00:06:08 2016] updatedatabase.pl: "] at /usr/share/koha/intranet/cgi-bin/installer/data/mysql/updatedatabase.pl line 12632. [Thu Nov 24 00:06:15 2016] updatedatabase.pl: DBD::mysql::db do failed: Can't create table 'koha_koha.#sql-2221_18d' (errno: 150) [for Statement " [Thu Nov 24 00:06:15 2016] updatedatabase.pl: ALTER TABLE marc_subfield_structure [Thu Nov 24 00:06:15 2016] updatedatabase.pl: MODIFY COLUMN authorised_value VARCHAR(32) DEFAULT NULL, [Thu Nov 24 00:06:15 2016] updatedatabase.pl: ADD CONSTRAINT marc_subfield_structure_ibfk_1 FOREIGN KEY (authorised_value) REFERENCES authorised_value_categories (category_name) ON UPDATE CASCADE ON DELETE SET NULL; [Thu Nov 24 00:06:15 2016] updatedatabase.pl: "] at /usr/share/koha/intranet/cgi-bin/installer/data/mysql/updatedatabase.pl line 13175. @Jacek: that is what you are talking about, right?
(In reply to Mirko Tietgen from comment #101) > [Thu Nov 24 00:06:08 2016] updatedatabase.pl: "] at > /usr/share/koha/intranet/cgi-bin/installer/data/mysql/updatedatabase.pl line > 12632. > [Thu Nov 24 00:06:15 2016] updatedatabase.pl: DBD::mysql::db do failed: > Can't create table 'koha_koha.#sql-2221_18d' (errno: 150) [for Statement " > [Thu Nov 24 00:06:15 2016] updatedatabase.pl: ALTER TABLE > marc_subfield_structure > [Thu Nov 24 00:06:15 2016] updatedatabase.pl: MODIFY COLUMN > authorised_value VARCHAR(32) DEFAULT NULL, > [Thu Nov 24 00:06:15 2016] updatedatabase.pl: ADD CONSTRAINT > marc_subfield_structure_ibfk_1 FOREIGN KEY (authorised_value) REFERENCES > authorised_value_categories (category_name) ON UPDATE CASCADE ON DELETE SET > NULL; > [Thu Nov 24 00:06:15 2016] updatedatabase.pl: "] at > /usr/share/koha/intranet/cgi-bin/installer/data/mysql/updatedatabase.pl line > 13175. > > > @Jacek: that is what you are talking about, right? Possibly ;), hard to tell (errno: 150 by itself is not very informative). What I did while trying to troubleshoot this problem (following the guidelines from <http://stackoverflow.com/questions/4061293/mysql-cant-create-table-errno-150>, especially nr 4 is quite helpful): - split erroneous statement in two 1) ALTER TABLE marc_subfield_structure MODIFY COLUMN authorised_value VARCHAR(32) DEFAULT NULL; 2) ALTER TABLE marc_subfield_structure ADD CONSTRAINT marc_subfield_structure_ibfk_1 FOREIGN KEY (authorised_value) REFERENCES authorised_value_categories (category_name) ON UPDATE CASCADE ON DELETE SET NULL; - part 1) executed OK, but part 2) still resulted in the same error (150) - noticed that after 1), utf8_unicode_ci collation is no longer there for authorised_value field (mysqldump --no-data marc_subfield_structure) - replaced 1) with ALTER TABLE marc_subfield_structure MODIFY COLUMN authorised_value VARCHAR(32) CHARACTER SET utf8 COLLATE utf8_unicode_ci DEFAULT NULL; - re-run 1) and 2) - no error, FK constrain got created successfully and collation for authorised_value is utf8_unicode_ci.
All of this sounds related to bug 13810
I am going to provide a patch on bug 17676
(In reply to Jonathan Druart from comment #95) > Created attachment 57729 [details] [review] [review] > Bug 17216: Fix SQL statement to setting > marc_subfield_structure.authorised_value to NULL > > We want to update it if it's empty, not equal to ';' > nopaste vim mode is to blame This is not in, we absolutely need it!
This changed an existing database entry - what about the installations that have already updated to 16.11.x?
Please use bug 18248 for follow-ups!