From ad38b24308df1cd3d451302d2ada3a4e59c45b62 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Mon, 30 Jan 2017 15:19:35 +0100 Subject: [PATCH] Bug 18014: AddAuthority should respect AUTO_INCREMENT Instead of using the MAX(authid)+1 logic, AddAuthority should just save the record and get the new id. The authid column is an autoincrement. This eliminates problems where a newly assigned authid also refers to a previously deleted record. (And it will not cause problems when refining the dontmerge functionality on report 9988.) Note: ModAuthority also calls AddAuthority to update an existing record; in that case we should not create a new record even if the record should not be found any more (which should be exceptional). This patch also simplifies handling of 001 in the authority record: in all cases this field is updated now; no need to check its contents. Test plan: [1] Run t/db_dependent/AuthoritiesMarc.t [2] Add a new authority record via the interface Signed-off-by: Marcel de Rooy Signed-off-by: Mark Tompsett Signed-off-by: Nick Clemens Signed-off-by: Kyle M Hall --- C4/AuthoritiesMarc.pm | 45 ++++++++++---------------------- t/db_dependent/AuthoritiesMarc.t | 9 ++++--- 2 files changed, 20 insertions(+), 34 deletions(-) diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm index 0c410a2bbf..679ca7d673 100644 --- a/C4/AuthoritiesMarc.pm +++ b/C4/AuthoritiesMarc.pm @@ -663,38 +663,21 @@ sub AddAuthority { $record->add_fields($auth_type_tag,'','', $auth_type_subfield=>$authtypecode); } - my $auth_exists=0; - my $oldRecord; - if (!$authid) { - my $sth=$dbh->prepare("select max(authid) from auth_header"); - $sth->execute; - ($authid)=$sth->fetchrow; - $authid=$authid+1; - ##Insert the recordID in MARC record - unless ($record->field('001') && $record->field('001')->data() eq $authid){ - $record->delete_field($record->field('001')); - $record->insert_fields_ordered(MARC::Field->new('001',$authid)); + # Save record into auth_header, update 001 + if (!$authid ) { + # Save a blank record, get authid + $dbh->do( "INSERT INTO auth_header (datecreated,marcxml) values (NOW(),?)", undef, '' ); + $authid = $dbh->last_insert_id( undef, undef, 'auth_header', 'authid' ); + logaction( "AUTHORITIES", "ADD", $authid, "authority" ) if C4::Context->preference("AuthoritiesLog"); } - } else { - $auth_exists=$dbh->do(qq(select authid from auth_header where authid=?),undef,$authid); -# warn "auth_exists = $auth_exists"; - } - if ($auth_exists>0){ - $oldRecord=GetAuthority($authid); - $record->add_fields('001',$authid) unless ($record->field('001')); -# warn "\n\n\n enregistrement".$record->as_formatted; - my $sth=$dbh->prepare("update auth_header set authtypecode=?,marc=?,marcxml=? where authid=?"); - $sth->execute($authtypecode,$record->as_usmarc,$record->as_xml_record($format),$authid) or die $sth->errstr; - $sth->finish; - } - else { - my $sth=$dbh->prepare("insert into auth_header (authid,datecreated,authtypecode,marc,marcxml) values (?,now(),?,?,?)"); - $sth->execute($authid,$authtypecode,$record->as_usmarc,$record->as_xml_record($format)); - $sth->finish; - logaction( "AUTHORITIES", "ADD", $authid, "authority" ) if C4::Context->preference("AuthoritiesLog"); - } - ModZebra($authid,'specialUpdate',"authorityserver",$oldRecord,$record); - return ($authid); + # Insert/update the recordID in MARC record + $record->delete_field( $record->field('001') ); + $record->insert_fields_ordered( MARC::Field->new( '001', $authid ) ); + # Update + $dbh->do( "UPDATE auth_header SET authtypecode=?, marc=?, marcxml=? WHERE authid=?", undef, $authtypecode, $record->as_usmarc, $record->as_xml_record($format), $authid ) or die $DBI::errstr; + ModZebra( $authid, 'specialUpdate', 'authorityserver', $record ); + + return ( $authid ); } diff --git a/t/db_dependent/AuthoritiesMarc.t b/t/db_dependent/AuthoritiesMarc.t index 91025cf328..d3ebc25f1a 100755 --- a/t/db_dependent/AuthoritiesMarc.t +++ b/t/db_dependent/AuthoritiesMarc.t @@ -192,15 +192,18 @@ is_deeply( ); subtest 'AddAuthority should respect AUTO_INCREMENT (BZ 18104)' => sub { - plan tests => 1; + plan tests => 3; t::lib::Mocks::mock_preference( 'marcflavour', 'MARC21' ); my $record = C4::AuthoritiesMarc::GetAuthority(1); my $id1 = AddAuthority( $record, undef, 'GEOGR_NAME' ); DelAuthority( $id1 ); my $id2 = AddAuthority( $record, undef, 'GEOGR_NAME' ); - is( $id1, $id2, 'FIXME: Got the same id back, let\'s fix that behavior' ); - + isnt( $id1, $id2, 'Do not return the same id again' ); + t::lib::Mocks::mock_preference( 'marcflavour', 'UNIMARC' ); + my $id3 = AddAuthority( $record, undef, 'GEOGR_NAME' ); + is( $id3 > 0, 1, 'Tested AddAuthority with UNIMARC' ); + is( $record->field('001')->data, $id3, 'Check updated 001' ); }; $schema->storage->txn_rollback; -- 2.39.5