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 <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm
index 0c410a2..679ca7d 100644 (file)
--- 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 91025cf..d3ebc25 100755 (executable)
--- 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;