Bugzilla – Attachment 61097 Details for
Bug 18265
Should biblio_metadata.biblionumber be biblioitemnumber instead?
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 18265 - Followup to Buuug 17196. deletedbiblio_metadata is not used. biblioitemnumber was forgot. FK to wrong table.
Bug-18265---Followup-to-Buuug-17196-deletedbibliom.patch (text/plain), 14.81 KB, created by
Olli-Antti Kivilahti
on 2017-03-15 10:39:22 UTC
(
hide
)
Description:
Bug 18265 - Followup to Buuug 17196. deletedbiblio_metadata is not used. biblioitemnumber was forgot. FK to wrong table.
Filename:
MIME Type:
Creator:
Olli-Antti Kivilahti
Created:
2017-03-15 10:39:22 UTC
Size:
14.81 KB
patch
obsolete
>From 6aa90b6498dc1eecde446f50545b5b0dee4fe277 Mon Sep 17 00:00:00 2001 >From: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> >Date: Tue, 14 Mar 2017 16:28:58 +0200 >Subject: [PATCH] Bug 18265 - Followup to Buuug 17196. deletedbiblio_metadata > is not used. biblioitemnumber was forgot. FK to wrong table. > >Exhibit 1: > >The MARCXML is lost when the biblio is deleted because the values in biblio_metadata-table from biblioitems-table are never moved to deletedbiblio_metadata-table. >Previously the marcxml was preserved in the oldbiblioitems-table >This is a challenge for gathering statistics >For example item has been ordered, received, and removed from collection during the statistics period >Information in marcxml is needed to accurately categorize the item > >Exhibit 2: > >[15:22:38] <kivilahtio> also. In Koha there once was a great idea that one biblio-entry can have multiple biblioitems >[15:22:45] <kivilahtio> and the marcxml was tied to the biblioitems-table >[15:23:11] <kivilahtio> this is a nice idea, but nobdy really cared for it and now biblionumber and biblioitemnumber are used interchangeably >[15:23:22] <kivilahtio> except when they get out of sync during a data migration >[15:23:41] <kivilahtio> then things start to work really interestingly >[15:23:47] <kivilahtio> but the point is >[15:23:57] <kivilahtio> biblio_metadata has a biblionumber-column >[15:24:15] <kivilahtio> shouldn't it have a biblioitemnumber-column, just for old time's sake? > >In the biblioitems-table we had marcxml. primary keyed with biblioitemnumber. >Now the marcxml is moved to biblio_metadata and the foreign key reference was simplified to biblionumber. >This makes the existing FRBR-bug worse. > >Exhibit 3: > >Just noticed while doing DB changes, that biblio_metadata is linked to the biblio table instead of the biblioitems-table! > >No FRBR there. > >Is being fixed. > >REPLICATE ISSUE: > >Create biblio >Delete biblio >Where is deletedbiblio_metadata-row? > >TEST PLAN: > >perl misc/devel/update_dbix_class_files.pl > >perl t/db_dependent/Biblio_crud.t > >perl t/db_dependent/Holds/HoldFulfillmentPolicy.t > + other tests from Buug 17196 >--- > C4/Biblio.pm | 42 +++++++- > Koha/Biblio/DeletedMetadata.pm | 32 +++++++ > Koha/Biblio/DeletedMetadatas.pm | 38 ++++++++ > Koha/Biblio/Metadata.pm | 2 +- > Koha/Deletedbiblio.pm | 34 +++++++ > Koha/Deletedbiblios.pm | 44 +++++++++ > .../Bug-18265-Followup_to_Buuug_17196.sql | 20 ++++ > t/db_dependent/Biblio_crud.t | 106 +++++++++++++++++++++ > 8 files changed, 312 insertions(+), 6 deletions(-) > create mode 100644 Koha/Biblio/DeletedMetadata.pm > create mode 100644 Koha/Biblio/DeletedMetadatas.pm > create mode 100644 Koha/Deletedbiblio.pm > create mode 100644 Koha/Deletedbiblios.pm > create mode 100644 installer/data/mysql/atomicupdate/Bug-18265-Followup_to_Buuug_17196.sql > create mode 100644 t/db_dependent/Biblio_crud.t > >diff --git a/C4/Biblio.pm b/C4/Biblio.pm >index ef2e432..6faa947 100644 >--- a/C4/Biblio.pm >+++ b/C4/Biblio.pm >@@ -42,7 +42,9 @@ use Koha::Caches; > use Koha::Authority::Types; > use Koha::Acquisition::Currencies; > use Koha::Biblio::Metadata; >+use Koha::Biblio::DeletedMetadata; > use Koha::Biblio::Metadatas; >+use Koha::Biblio::DeletedMetadatas; > use Koha::SearchEngine; > use Koha::Libraries; > >@@ -257,7 +259,7 @@ sub AddBiblio { > _koha_marc_update_biblioitem_cn_sort( $record, $olddata, $frameworkcode ); > > # now add the record >- ModBiblioMarc( $record, $biblionumber, $frameworkcode ) unless $defer_marc_save; >+ ModBiblioMarc( $record, $biblionumber, $biblioitemnumber, $frameworkcode ) unless $defer_marc_save; > > # update OAI-PMH sets > if(C4::Context->preference("OAI-PMH:AutoUpdateSets")) { >@@ -335,7 +337,7 @@ sub ModBiblio { > _koha_marc_update_biblioitem_cn_sort( $record, $oldbiblio, $frameworkcode ); > > # update the MARC record (that now contains biblio and items) with the new record data >- &ModBiblioMarc( $record, $biblionumber, $frameworkcode ); >+ &ModBiblioMarc( $record, $biblionumber, $biblioitemnumber, $frameworkcode ); > > # modify the other koha tables > _koha_modify_biblio( $dbh, $oldbiblio, $frameworkcode ); >@@ -426,7 +428,10 @@ sub DelBiblio { > $sth->execute($biblionumber); > while ( my $biblioitemnumber = $sth->fetchrow ) { > >- # delete this biblioitem >+ #Handle biblio_metadata and deletedbiblio_metadata first before biblioitems operations CASCADE there >+ $error = _koha_delete_biblio_metadata( $dbh, $biblioitemnumber, $biblionumber ); >+ return $error if $error; >+ > $error = _koha_delete_biblioitems( $dbh, $biblioitemnumber ); > return $error if $error; > } >@@ -436,6 +441,7 @@ sub DelBiblio { > # delete cascade will prevent deletedbiblioitems rows > # from being generated by _koha_delete_biblioitems > $error = _koha_delete_biblio( $dbh, $biblionumber ); >+ return $error if $error; > > logaction( "CATALOGUING", "DELETE", $biblionumber, "biblio" ) if C4::Context->preference("CataloguingLog"); > >@@ -3438,11 +3444,36 @@ sub _koha_delete_biblioitems { > return; > } > >+=head2 _koha_delete_biblio_metadata >+ >+ $error = _koha_delete_biblio_metadata($dbh, $biblioitemnumber, $biblionumber); >+ >+Internal sub for deleting from biblio_metadata table -- also saves to deletedbiblio_metadata >+ >+C<$dbh> - the database handle >+C<$biblioitemnumber> - the biblioitemnumber of the biblio_metadata to be deleted >+ >+=cut >+ >+# FIXME: add error handling >+ >+sub _koha_delete_biblio_metadata { >+ my ( $dbh, $biblioitemnumber ) = @_; >+ >+ my $m = Koha::Biblio::Metadatas->find({biblioitemnumber => $biblioitemnumber}); >+ die "_koha_delete_biblio_metadata($biblioitemnumber):> No biblio_metadata-row!" unless $m; >+ >+ my $dm = Koha::Biblio::DeletedMetadata->new( $m->_result->{_column_data} ); >+ $dm->store; >+ $m->delete(); >+ return undef; >+} >+ > =head1 UNEXPORTED FUNCTIONS > > =head2 ModBiblioMarc > >- &ModBiblioMarc($newrec,$biblionumber,$frameworkcode); >+ &ModBiblioMarc($newrec,$biblionumber,$biblioitemnumber,$frameworkcode); > > Add MARC XML data for a biblio to koha > >@@ -3453,7 +3484,7 @@ Function exported, but should NOT be used, unless you really know what you're do > sub ModBiblioMarc { > # pass the MARC::Record to this function, and it will create the records in > # the marcxml field >- my ( $record, $biblionumber, $frameworkcode ) = @_; >+ my ( $record, $biblionumber, $biblioitemnumber, $frameworkcode ) = @_; > if ( !$record ) { > carp 'ModBiblioMarc passed an undefined record'; > return; >@@ -3500,6 +3531,7 @@ sub ModBiblioMarc { > } > > my $metadata = { >+ biblioitemnumber => $biblioitemnumber, > biblionumber => $biblionumber, > format => 'marcxml', > marcflavour => C4::Context->preference('marcflavour'), >diff --git a/Koha/Biblio/DeletedMetadata.pm b/Koha/Biblio/DeletedMetadata.pm >new file mode 100644 >index 0000000..5a96ff2 >--- /dev/null >+++ b/Koha/Biblio/DeletedMetadata.pm >@@ -0,0 +1,32 @@ >+package Koha::Biblio::DeletedMetadata; >+ >+# This file is part of Koha. >+# >+ >+use Modern::Perl; >+ >+use Carp; >+ >+use Koha::Database; >+ >+use base qw(Koha::Object); >+ >+=head1 NAME >+ >+Koha::Biblio::DeletedMetadata - Koha Metadata Object class >+ >+=head1 API >+ >+=head2 Class Methods >+ >+=cut >+ >+=head3 type >+ >+=cut >+ >+sub _type { >+ return 'DeletedbiblioMetadata'; >+} >+ >+1; >diff --git a/Koha/Biblio/DeletedMetadatas.pm b/Koha/Biblio/DeletedMetadatas.pm >new file mode 100644 >index 0000000..23e4e08 >--- /dev/null >+++ b/Koha/Biblio/DeletedMetadatas.pm >@@ -0,0 +1,38 @@ >+package Koha::Biblio::DeletedMetadatas; >+ >+# This file is part of Koha. >+# >+ >+use Modern::Perl; >+ >+use Carp; >+ >+use Koha::Database; >+ >+use Koha::Biblio::Metadata; >+ >+use base qw(Koha::Objects); >+ >+=head1 NAME >+ >+Koha::Biblio::DeletedMetadatas - Koha Metadata Object set class >+ >+=head1 API >+ >+=head2 Class Methods >+ >+=cut >+ >+=head3 type >+ >+=cut >+ >+sub _type { >+ return 'DeletedbiblioMetadata'; >+} >+ >+sub object_class { >+ return 'Koha::Biblio::DeletedMetadata'; >+} >+ >+1; >diff --git a/Koha/Biblio/Metadata.pm b/Koha/Biblio/Metadata.pm >index f271f6f..9c5dbe3 100644 >--- a/Koha/Biblio/Metadata.pm >+++ b/Koha/Biblio/Metadata.pm >@@ -25,7 +25,7 @@ use base qw(Koha::Object); > > =head1 NAME > >-Koha::Metadata - Koha Metadata Object class >+Koha::Biblio::Metadata - Koha Metadata Object class > > =head1 API > >diff --git a/Koha/Deletedbiblio.pm b/Koha/Deletedbiblio.pm >new file mode 100644 >index 0000000..8bda55a >--- /dev/null >+++ b/Koha/Deletedbiblio.pm >@@ -0,0 +1,34 @@ >+package Koha::Deletedbiblio; >+ >+# Copyright KohaSuomi 2017 >+# >+# This file is part of Koha. >+# >+ >+use Modern::Perl; >+ >+use Carp; >+ >+use Koha::Database; >+ >+use base qw(Koha::Object); >+ >+=head1 NAME >+ >+Koha::DeletedBiblio >+ >+=head1 API >+ >+=head2 Class Methods >+ >+=cut >+ >+=head3 type >+ >+=cut >+ >+sub _type { >+ return 'Deletedbiblio'; >+} >+ >+1; >diff --git a/Koha/Deletedbiblios.pm b/Koha/Deletedbiblios.pm >new file mode 100644 >index 0000000..aca538c >--- /dev/null >+++ b/Koha/Deletedbiblios.pm >@@ -0,0 +1,44 @@ >+package Koha::Deletedbiblios; >+ >+# Copyright KohaSuomi 2017 >+# >+# This file is part of Koha. >+# >+ >+use Modern::Perl; >+ >+use Carp; >+ >+use Koha::Database; >+ >+use Koha::Deletedbiblio; >+ >+use base qw(Koha::Objects); >+ >+=head1 NAME >+ >+Koha::Deletedbiblios - Koha Biblio object set class >+ >+=head1 API >+ >+=head2 Class Methods >+ >+=cut >+ >+=head3 type >+ >+=cut >+ >+sub _type { >+ return 'Deletedbiblio'; >+} >+ >+=head3 object_class >+ >+=cut >+ >+sub object_class { >+ return 'Koha::Deletedbiblio'; >+} >+ >+1; >diff --git a/installer/data/mysql/atomicupdate/Bug-18265-Followup_to_Buuug_17196.sql b/installer/data/mysql/atomicupdate/Bug-18265-Followup_to_Buuug_17196.sql >new file mode 100644 >index 0000000..80e7bac >--- /dev/null >+++ b/installer/data/mysql/atomicupdate/Bug-18265-Followup_to_Buuug_17196.sql >@@ -0,0 +1,20 @@ >+ALTER TABLE biblio_metadata DROP FOREIGN KEY `record_metadata_fk_1`; >+ALTER TABLE biblio_metadata DROP KEY `biblio_metadata_uniq_key`; >+ALTER TABLE biblio_metadata ADD COLUMN `biblioitemnumber` int(11) NOT NULL; >+ALTER TABLE biblio_metadata ADD FOREIGN KEY `biblioitems_metadata_fk_1` (biblioitemnumber) REFERENCES biblioitems (biblioitemnumber) ON DELETE CASCADE ON UPDATE CASCADE; >+ALTER TABLE biblio_metadata ADD UNIQUE KEY `biblio_metadata_uniq_key` (`biblioitemnumber`, `biblionumber`,`format`,`marcflavour`); >+ >+ >+ >+ALTER TABLE deletedbiblio_metadata DROP FOREIGN KEY `deletedrecord_metadata_fk_1`; >+ALTER TABLE deletedbiblio_metadata DROP KEY `deletedbiblio_metadata_uniq_key`; >+ALTER TABLE deletedbiblio_metadata ADD COLUMN `biblioitemnumber` int(11) NOT NULL; >+## If this FK is preserved, most of the DelBiblio needs to be rewritten to gracefully >+## move biblio and friends to deletedbiblio and deletedfriends without a chance for >+## data loss in between. >+## DelBiblio is implemented in a leaf-first way of handling the joint tables. >+## You cannot INSERT leaf-first to deletedbiblio_metadata without the presence of deletedbiblioitems >+#ALTER TABLE deletedbiblio_metadata ADD FOREIGN KEY `deletedbiblioitems_metadata_fk_1` (biblioitemnumber) REFERENCES deletedbiblioitems (biblioitemnumber) ON DELETE CASCADE ON UPDATE CASCADE; >+ALTER TABLE deletedbiblio_metadata ADD UNIQUE KEY `deletedbiblio_metadata_uniq_key` (`biblioitemnumber`, `biblionumber`,`format`,`marcflavour`); >+ >+ >diff --git a/t/db_dependent/Biblio_crud.t b/t/db_dependent/Biblio_crud.t >new file mode 100644 >index 0000000..8948140 >--- /dev/null >+++ b/t/db_dependent/Biblio_crud.t >@@ -0,0 +1,106 @@ >+#!/usr/bin/perl >+ >+# This file is part of Koha. >+ >+use Modern::Perl; >+use Test::More; >+ >+use C4::Biblio; >+use Koha::Biblios; >+use Koha::Deletedbiblios; >+use Koha::Biblio::Metadatas; >+use Koha::Biblio::DeletedMetadatas; >+ >+ >+ >+##This test is not intended to fully cover all aspects of Biblio creation and it's linked business objects. >+##It is simply intended for now to test biblio_metadata-table population. >+subtest "Scenario: CRUD a Biblio, happy path", \&crudBiblio; >+sub crudBiblio { >+ my ($record, $kohaBiblio, $deletedKohaBiblio, $biblionumber, $biblioitemnumber, $metadata, $deletedMetadata); >+ my $title = 'Incurable disease of test apparatuses'; >+ eval { >+ >+ $record = C4::Biblio::TransformKohaToMarc({ >+ 'biblio.title' => $title, >+ 'biblioitems.isbn' => '0123456789', >+ }); >+ ok($record, >+ "Given a fresh Biblio from the top of my head"); >+ >+ >+ subtest "Create", sub { >+ >+ ($biblionumber, $biblioitemnumber) = C4::Biblio::AddBiblio($record,''); >+ ok($biblionumber && $biblioitemnumber, >+ "When the Biblio is added to DB"); >+ }; >+ >+ >+ subtest "Read", sub { >+ >+ $kohaBiblio = Koha::Biblios->find($biblionumber); >+ ok($kohaBiblio, >+ "Then it can be found from the DB"); >+ >+ ok($kohaBiblio->biblionumber, >+ "And it looks like a real biblio"); >+ >+ $metadata = Koha::Biblio::Metadatas->find({biblioitemnumber => $biblioitemnumber}); >+ ok($metadata, >+ "When I fetch the Metadata record from DB"); >+ >+ is($metadata->format, 'marcxml', >+ "Then the Metadata record format is 'marcxml'"); >+ >+ is($metadata->biblionumber, $biblionumber, >+ "And biblionumber matches the original Biblio"); >+ >+ is($metadata->biblioitemnumber, $biblioitemnumber, >+ "And the biblioitemnumber matches the original Biblio"); >+ >+ #Shouldn't marcflavour be just flavour since we don't always store MARC to the biblio_metadata? >+ is($metadata->marcflavour, C4::Context->preference("marcflavour"), >+ "And the marcflavour is the default"); >+ >+ like($metadata->metadata, qr/$title/, >+ "And the record itself contains the correct title"); >+ }; >+ >+ >+ subtest "Delete", sub { >+ >+ ok(! C4::Biblio::DelBiblio($biblionumber), #return value here doesnt matter. Should throw exceptions instead. >+ "When I delete the Biblio"); >+ >+ ok(! Koha::Biblios->find({biblionumber => $biblionumber}), >+ "Then I get nothing when I fetch it from DB"); >+ >+ $deletedKohaBiblio = Koha::Deletedbiblios->find({biblionumber => $biblionumber}); >+ ok($deletedKohaBiblio, >+ "But I find it in the deletedbiblios"); >+ >+ is($deletedKohaBiblio->title, $kohaBiblio->title, >+ "And the Biblio looks just like before!"); >+ >+ ok(! Koha::Biblio::Metadatas->find({biblioitemnumber => $biblioitemnumber}), >+ "When I fetch the Metadata record, I get nothing"); >+ >+ $deletedMetadata = Koha::Biblio::DeletedMetadatas->find({biblioitemnumber => $biblioitemnumber}); >+ ok($deletedMetadata, >+ "But I found it in the deletedbiblio_metadata"); >+ >+ is_deeply($deletedMetadata->_result->{_column_data}, $metadata->_result->{_column_data}, >+ "And the Metadata looks just like before!"); >+ }; >+ >+ }; >+ ok(0, $@) if $@; >+} >+ >+ >+ >+ >+ >+done_testing; >+ >-- >2.7.4
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 18265
:
61090
|
61096
|
61097
|
61098
|
61101
|
61199