Description
David Cook
2020-10-15 00:14:56 UTC
Actually, we'll also need to re-generate Koha/Schema/Result/AuthHeader.pm Created attachment 111670 [details] [review] Bug 26684: Remove marc from auth_header for new installs Created attachment 111671 [details] [review] Bug 26684: Remove marc column from existing databases Created attachment 111672 [details] [review] Bug 26684: [Do not push] Remember to update Koha::Schema::Result::AuthHeader Created attachment 112016 [details] [review] Bug 26684: Remove marc from auth_header for new installs Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com> Created attachment 112018 [details] [review] Bug 26684: Remove marc from auth_header for new installs Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com> Created attachment 112019 [details] [review] Bug 26684: Remove marc column from existing databases Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com> Qa tools complaints: FAIL installer/data/mysql/atomicupdate/001-26684-drop_marc_column_from_auth_header.pl FAIL critic Code before strictures are enabled at line 1, column 1. See page 429 of PBP. FAIL file permissions File must have the exec flag FAIL valid Name "main::dbh" used only once: possible typo if you move the file to 'perl' many disappear, however, the bug number is missing from the update so it fails right now, also we should check the existence of the column before droppping Created attachment 112087 [details] [review] Bug 26684: Remove marc from auth_header for new installs Created attachment 112088 [details] [review] Bug 26684: Remove marc column from existing databases Created attachment 112089 [details] [review] Bug 26684: [Do not push] Remember to update Koha::Schema::Result::AuthHeader Created attachment 112090 [details] [review] Bug 26684: Fix atomic update QA scripts and updatedatabase.pl should be happy now Patch no longer applies 8-(.. Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 26684: Remove marc from auth_header for new installs Using index info to reconstruct a base tree... M C4/AuthoritiesMarc.pm M installer/data/mysql/kohastructure.sql Falling back to patching base and 3-way merge... Auto-merging installer/data/mysql/kohastructure.sql CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql Auto-merging C4/AuthoritiesMarc.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 26684: Remove marc from auth_header for new installs Also, is there a test plan? I have no idea how to test this one... Created attachment 177445 [details] [review] Bug 26684: Remove marc from auth_header for new installs Created attachment 177446 [details] [review] Bug 26684: Remove marc column from existing databases Created attachment 177447 [details] [review] Bug 26684: [Do not push] Remember to update Koha::Schema::Result::AuthHeader Created attachment 177448 [details] [review] Bug 26684: Fix atomic update The auth_header.marc column should definitely be remover. It is outdated, never used (only stored), and we have removed marc from biblio long time ago. Morover, it is of size blob (64K), which means it cannot properly host large ISO 2709 records (up to 99K). While saving large records we get warning: DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Data too long for column 'marc' at row 1 at /kohadevbox/koha/Koha/Object.pm line 172 and creation / modification is not properly performed. Created attachment 177449 [details] [review] Bug 26684: Remove marc from Koha::Authority::to_api_mapping Created attachment 177450 [details] [review] Bug 26684: Remove misc/exportauth.pl as a consequence of removing auth_header.marc Since we have misc/export_records.pl and we want to remove auth_header.marc, the ancient script misc/exportauth.pl can be removed as well (it is relied upon auth_header.marc). Created attachment 177455 [details] [review] Bug 26684: Remove marc from auth_header for new installs Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Created attachment 177456 [details] [review] Bug 26684: Remove marc column from existing databases Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Created attachment 177457 [details] [review] Bug 26684: [Do not push] Remember to update Koha::Schema::Result::AuthHeader Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Created attachment 177458 [details] [review] Bug 26684: Fix atomic update Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Created attachment 177459 [details] [review] Bug 26684: Remove marc from Koha::Authority::to_api_mapping Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Created attachment 177460 [details] [review] Bug 26684: Remove misc/exportauth.pl as a consequence of removing auth_header.marc Since we have misc/export_records.pl and we want to remove auth_header.marc, the ancient script misc/exportauth.pl can be removed as well (it is relied upon auth_header.marc). Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Please add a test plan, and I'd be happy to test. (Replicate the issue (and what the issue is), apply patches, demonstrate that the issue is fixed.) Sorry, I thought this was waiting for sign off (has now been signed off). A test plan would be ideal for the QA step, and a release note would be great to explain what this fixes and why. Created attachment 177461 [details]
Test data (one auth record) of size 92771 B
(In reply to David Nind from comment #29) > A test plan would be ideal for the QA step, and a release note would be > great to explain what this fixes and why. It is hard to say about a strict test plan. One aspect of problems causing by this column can be illustrates by the following sequence of steps. 1. Take the provided test auth record (100 Baggins, Bilbo) with lots od 670 fields, of size 92771 B: still in the limits of ISO 2709, but significantly bigger than blob size (64 KB). 2. Try to import the record into ktd installation with: misc/migration_tools/bulkmarcimport.pl -a -d -file Baggins_Bilbo.mrc -fk You should see a series of warnings: DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Data too long for column 'marc' at row 1 at /kohadevbox/koha/Koha/Object.pm line 172 Use of uninitialized value $originalid in concatenation (.) or string at ./misc/migration_tools/bulkmarcimport.pl line 520, <GEN3> line 1. ERROR: Insert authority failed: DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Data too long for column 'marc' at row 1 at /kohadevbox/koha/Koha/Object.pm line 172 3. Control in ktd --dbshell with: select * from auth_header; You should see one row with no data in marc nor in marcxml. 4. Apply the patch ; updatedatabase. 5. Repeat p. 2 and p. 3. You should get no warnings and expected (huge) result from MySQL. Other aspects should be evaluated by code reviewers. The koha_header.marc column should not be used in the code anymore with this patch. Thanks Janusz! |