improved error detection in AddBiblioAndItems
Introduced new C4::Biblio function CheckItemPreSave, which checks for duplicate barcodes and invalid branch codes. Not yet sure whether this function needs to be exported or whether it will just be used internally to C4::Bibli. Signed-off-by: Chris Cormack <crc@liblime.com> Signed-off-by: Joshua Ferraro <jmf@liblime.com>
This commit is contained in:
Galen Charlton
Joshua Ferraro
parent
eeafadc598
commit
cb6cf680bc
148
C4/Biblio.pm
148
C4/Biblio.pm
|
|
@ -27,6 +27,7 @@ use MARC::File::USMARC;
|
|||
use MARC::File::XML;
|
||||
use ZOOM;
|
||||
use C4::Koha;
|
||||
use C4::Branch;
|
||||
use C4::Dates qw/format_date/;
|
||||
use C4::Log; # logaction
|
||||
use C4::ClassSource;
|
||||
|
|
@ -231,7 +232,7 @@ sub AddBiblio {
|
|||
|
||||
=over 4
|
||||
|
||||
($biblionumber,$biblioitemnumber, $itemnumber_ref) = AddBiblioAndItems($record, $frameworkcode);
|
||||
($biblionumber,$biblioitemnumber, $itemnumber_ref, $error_ref) = AddBiblioAndItems($record, $frameworkcode);
|
||||
|
||||
=back
|
||||
|
||||
|
|
@ -246,12 +247,40 @@ One functional difference is that the duplicate item barcode
|
|||
check is implemented in this API, instead of relying on
|
||||
the caller to do it, like AddItem does.
|
||||
|
||||
This function returns the biblionumber and biblioitemnumber of the
|
||||
new bib, an arrayref of new itemsnumbers, and an arrayref of item
|
||||
errors encountered during the processing. Each entry in the errors
|
||||
list is a hashref containing the following keys:
|
||||
|
||||
=over 2
|
||||
|
||||
=item item_sequence
|
||||
|
||||
Sequence number of original item tag in the MARC record.
|
||||
|
||||
=item item_barcode
|
||||
|
||||
Item barcode, provide to assist in the construction of
|
||||
useful error messages.
|
||||
|
||||
=item error_condition
|
||||
|
||||
Code representing the error condition. Can be 'duplicate_barcode',
|
||||
'invalid_homebranch', or 'invalid_holdingbranch'.
|
||||
|
||||
=item error_information
|
||||
|
||||
Additional information appropriate to the error condition.
|
||||
|
||||
=back
|
||||
|
||||
=cut
|
||||
|
||||
sub AddBiblioAndItems {
|
||||
my ( $record, $frameworkcode ) = @_;
|
||||
my ($biblionumber,$biblioitemnumber,$error);
|
||||
my @itemnumbers = ();
|
||||
my @errors = ();
|
||||
my $dbh = C4::Context->dbh;
|
||||
|
||||
# transform the data into koha-table style data
|
||||
|
|
@ -267,7 +296,9 @@ sub AddBiblioAndItems {
|
|||
# now we loop through the item tags and start creating items
|
||||
my @bad_item_fields = ();
|
||||
my ($itemtag, $itemsubfield) = &GetMarcFromKohaField("items.itemnumber",'');
|
||||
my $item_sequence_num = 0;
|
||||
ITEMFIELD: foreach my $item_field ($record->field($itemtag)) {
|
||||
$item_sequence_num++;
|
||||
# we take the item field and stick it into a new
|
||||
# MARC record -- this is required so far because (FIXME)
|
||||
# TransformMarcToKoha requires a MARC::Record, not a MARC::Field
|
||||
|
|
@ -281,11 +312,15 @@ sub AddBiblioAndItems {
|
|||
$item->{'biblioitemnumber'} = $biblioitemnumber;
|
||||
|
||||
# check for duplicate barcode
|
||||
my %item_errors = CheckItemPreSave($item);
|
||||
if (%item_errors) {
|
||||
push @errors, _repack_item_errors($item_sequence_num, $item, \%item_errors);
|
||||
push @bad_item_fields, $item_field;
|
||||
next ITEMFIELD;
|
||||
}
|
||||
my $duplicate_barcode = exists($item->{'barcode'}) && GetItemnumberFromBarcode($item->{'barcode'});
|
||||
if ($duplicate_barcode) {
|
||||
warn "ERROR: cannot add item $item->{'barcode'} for biblio $biblionumber: duplicate barcode\n";
|
||||
push @bad_item_fields, $item_field;
|
||||
next ITEMFIELD;
|
||||
}
|
||||
|
||||
# figure out what item type to use -- biblioitem-level or item-level
|
||||
|
|
@ -350,10 +385,29 @@ sub AddBiblioAndItems {
|
|||
&logaction(C4::Context->userenv->{'number'},"CATALOGUING","ADD",$biblionumber,"biblio")
|
||||
if C4::Context->preference("CataloguingLog");
|
||||
|
||||
return ( $biblionumber, $biblioitemnumber, \@itemnumbers);
|
||||
return ( $biblionumber, $biblioitemnumber, \@itemnumbers, \@errors);
|
||||
|
||||
}
|
||||
|
||||
sub _repack_item_errors {
|
||||
my $item_sequence_num = shift;
|
||||
my $item_ref = shift;
|
||||
my $error_ref = shift;
|
||||
|
||||
my @repacked_errors = ();
|
||||
|
||||
foreach my $error_code (sort keys %{ $error_ref }) {
|
||||
my $repacked_error = {};
|
||||
$repacked_error->{'item_sequence'} = $item_sequence_num;
|
||||
$repacked_error->{'item_barcode'} = exists($item_ref->{'barcode'}) ? $item_ref->{'barcode'} : '';
|
||||
$repacked_error->{'error_code'} = $error_code;
|
||||
$repacked_error->{'error_information'} = $error_ref->{$error_code};
|
||||
push @repacked_errors, $repacked_error;
|
||||
}
|
||||
|
||||
return @repacked_errors;
|
||||
}
|
||||
|
||||
=head2 AddItem
|
||||
|
||||
=over 2
|
||||
|
|
@ -765,6 +819,92 @@ sub DelItem {
|
|||
if C4::Context->preference("CataloguingLog");
|
||||
}
|
||||
|
||||
=head2 CheckItemPreSave
|
||||
|
||||
=over 4
|
||||
|
||||
my $item_ref = TransformMarcToKoha($marc, 'items');
|
||||
# do stuff
|
||||
my %errors = CheckItemPreSave($item_ref);
|
||||
if (exists $errors{'duplicate_barcode'}) {
|
||||
print "item has duplicate barcode: ", $errors{'duplicate_barcode'}, "\n";
|
||||
} elsif (exists $errors{'invalid_homebranch'}) {
|
||||
print "item has invalid home branch: ", $errors{'invalid_homebranch'}, "\n";
|
||||
} elsif (exists $errors{'invalid_holdingbranch'}) {
|
||||
print "item has invalid holding branch: ", $errors{'invalid_holdingbranch'}, "\n";
|
||||
} else {
|
||||
print "item is OK";
|
||||
}
|
||||
|
||||
=back
|
||||
|
||||
Given a hashref containing item fields, determine if it can be
|
||||
inserted or updated in the database. Specifically, checks for
|
||||
database integrity issues, and returns a hash containing any
|
||||
of the following keys, if applicable.
|
||||
|
||||
=over 2
|
||||
|
||||
=item duplicate_barcode
|
||||
|
||||
Barcode, if it duplicates one already found in the database.
|
||||
|
||||
=item invalid_homebranch
|
||||
|
||||
Home branch, if not defined in branches table.
|
||||
|
||||
=item invalid_holdingbranch
|
||||
|
||||
Holding branch, if not defined in branches table.
|
||||
|
||||
=back
|
||||
|
||||
This function does NOT implement any policy-related checks,
|
||||
e.g., whether current operator is allowed to save an
|
||||
item that has a given branch code.
|
||||
|
||||
=cut
|
||||
|
||||
sub CheckItemPreSave {
|
||||
my $item_ref = shift;
|
||||
|
||||
my %errors = ();
|
||||
|
||||
# check for duplicate barcode
|
||||
if (exists $item_ref->{'barcode'} and defined $item_ref->{'barcode'}) {
|
||||
my $existing_itemnumber = GetItemnumberFromBarcode($item_ref->{'barcode'});
|
||||
if ($existing_itemnumber) {
|
||||
if (!exists $item_ref->{'itemnumber'} # new item
|
||||
or $item_ref->{'itemnumber'} != $existing_itemnumber) { # existing item
|
||||
$errors{'duplicate_barcode'} = $item_ref->{'barcode'};
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
# check for valid home branch
|
||||
if (exists $item_ref->{'homebranch'} and defined $item_ref->{'homebranch'}) {
|
||||
my $branch_name = GetBranchName($item_ref->{'homebranch'});
|
||||
unless (defined $branch_name) {
|
||||
# relies on fact that branches.branchname is a non-NULL column,
|
||||
# so GetBranchName returns undef only if branch does not exist
|
||||
$errors{'invalid_homebranch'} = $item_ref->{'homebranch'};
|
||||
}
|
||||
}
|
||||
|
||||
# check for valid holding branch
|
||||
if (exists $item_ref->{'holdingbranch'} and defined $item_ref->{'holdingbranch'}) {
|
||||
my $branch_name = GetBranchName($item_ref->{'holdingbranch'});
|
||||
unless (defined $branch_name) {
|
||||
# relies on fact that branches.branchname is a non-NULL column,
|
||||
# so GetBranchName returns undef only if branch does not exist
|
||||
$errors{'invalid_holdingbranch'} = $item_ref->{'holdingbranch'};
|
||||
}
|
||||
}
|
||||
|
||||
return %errors;
|
||||
|
||||
}
|
||||
|
||||
=head2 GetBiblioData
|
||||
|
||||
=over 4
|
||||
|
|
|
|||
|
|
@ -230,12 +230,15 @@ while ( my $record = $batch->next() ) {
|
|||
|
||||
unless ($test_parameter) {
|
||||
# FIXME add back dup barcode check
|
||||
my ( $bibid, $oldbibitemnum, $itemnumbers_ref );
|
||||
eval { ( $bibid, $oldbibitemnum, $itemnumbers_ref ) = AddBiblioAndItems( $record, '' ); };
|
||||
warn $@ if $@;
|
||||
my ( $bibid, $oldbibitemnum, $itemnumbers_ref, $errors_ref );
|
||||
eval { ( $bibid, $oldbibitemnum, $itemnumbers_ref, $errors_ref ) = AddBiblioAndItems( $record, '' ); };
|
||||
if ( $@ ) {
|
||||
warn "ERROR: Adding biblio and or items $bibid failed\n" if $verbose
|
||||
warn "ERROR: Adding biblio and or items $bibid failed: $@\n";
|
||||
}
|
||||
if ($#{ $errors_ref } > -1) {
|
||||
report_item_errors($bibid, $errors_ref);
|
||||
}
|
||||
|
||||
$dbh->commit() if (0 == $i % $commitnum);
|
||||
}
|
||||
last if $i == $number;
|
||||
|
|
@ -255,3 +258,18 @@ $dbh->do("UPDATE systempreferences SET value=$CataloguingLog WHERE variable='Cat
|
|||
|
||||
my $timeneeded = gettimeofday - $starttime;
|
||||
print "$i MARC records done in $timeneeded seconds\n";
|
||||
|
||||
exit 0;
|
||||
|
||||
sub report_item_errors {
|
||||
my $bibid = shift;
|
||||
my $errors_ref = shift;
|
||||
|
||||
foreach my $error (@{ $errors_ref }) {
|
||||
my $msg = "Item not added (bib $bibid, item tag #$error->{'item_sequence'}, barcode $error->{'item_barcode'}): ";
|
||||
my $error_code = $error->{'error_code'};
|
||||
$error_code =~ s/_/ /g;
|
||||
$msg .= "$error_code $error->{'error_information'}";
|
||||
print $msg, "\n";
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue