From bd57491f499618eae612abbb81de1265acc18c09 Mon Sep 17 00:00:00 2001 From: Arthur Suzuki Date: Tue, 19 Nov 2024 11:25:43 +0100 Subject: [PATCH] Bug 20551: (QA follow-up) perltidy and test fixes --- Koha/Exporter/Record.pm | 136 +++++++++++++++---------------- misc/export_records.pl | 30 ++++--- t/db_dependent/Exporter/Record.t | 21 +++-- 3 files changed, 96 insertions(+), 91 deletions(-) diff --git a/Koha/Exporter/Record.pm b/Koha/Exporter/Record.pm index bd5d4ea58ae..ed2c6cd730c 100644 --- a/Koha/Exporter/Record.pm +++ b/Koha/Exporter/Record.pm @@ -5,7 +5,7 @@ use MARC::File::XML; use MARC::File::USMARC; use C4::AuthoritiesMarc; -use C4::Biblio qw( GetMarcFromKohaField ); +use C4::Biblio qw( GetMarcFromKohaField ); use C4::Charset qw( StripNonXmlChars ); use C4::Record; use Koha::Biblios; @@ -34,10 +34,10 @@ sub _get_record_for_export { } elsif ( $record_type eq 'deleted_bibs' ) { $record = _get_deleted_biblio_for_export( { %$params, biblionumber => $record_id } ); } else { - Koha::Logger->get->warn( "Record type $record_type not supported." ); + Koha::Logger->get->warn("Record type $record_type not supported."); } if ( !$record ) { - Koha::Logger->get->warn( "Record $record_id with record type $record_type could not be exported." ); + Koha::Logger->get->warn("Record $record_id with record type $record_type could not be exported."); return; } @@ -58,32 +58,30 @@ sub _get_record_for_export { }, ); if ($conditions) { - foreach my $condition (@{$conditions}) { - my ($field_tag, $subfield, $operator, $match_value) = @{$condition}; - my @fields = $record->field($field_tag); + foreach my $condition ( @{$conditions} ) { + my ( $field_tag, $subfield, $operator, $match_value ) = @{$condition}; + my @fields = $record->field($field_tag); my $no_target = 0; - if (!@fields) { + if ( !@fields ) { $no_target = 1; - } - else { - if ($operator eq '?') { + } else { + if ( $operator eq '?' ) { return unless any { $subfield ? $_->subfield($subfield) : $_->data() } @fields; - } elsif ($operator eq '!?') { + } elsif ( $operator eq '!?' ) { return if any { $subfield ? $_->subfield($subfield) : $_->data() } @fields; } else { my $op; - if (exists $operators{$operator}) { + if ( exists $operators{$operator} ) { $op = $operators{$operator}; } else { die("Invalid operator: $op"); } - my @target_values = map { $subfield ? $_->subfield($subfield) : ($_->data()) } @fields; - if (!@target_values) { + my @target_values = map { $subfield ? $_->subfield($subfield) : ( $_->data() ) } @fields; + if ( !@target_values ) { $no_target = 1; - } - else { - return unless all { $op->($_, $match_value) } @target_values; + } else { + return unless all { $op->( $_, $match_value ) } @target_values; } } } @@ -115,31 +113,29 @@ sub _get_record_for_export { } sub _get_deleted_biblio_for_export { - my ($params) = @_; + my ($params) = @_; my $biblionumber = $params->{biblionumber}; + # Creating schema is expensive, allow caller to # pass it so don't have to recreate for each call - my $resultset = $params->{resultset} || Koha::Database->new() - ->schema() - ->resultset('DeletedbiblioMetadata'); - my $marc_flavour = C4::Context->preference('marcflavour'); - my $biblio_metadata = $resultset->find({ - 'biblionumber' => $biblionumber, - 'format' => 'marcxml', - 'schema' => $marc_flavour - }); + my $resultset = $params->{resultset} || Koha::Database->new()->schema()->resultset('DeletedbiblioMetadata'); + my $marc_flavour = C4::Context->preference('marcflavour'); + my $biblio_metadata = $resultset->find( + { + 'biblionumber' => $biblionumber, + 'format' => 'marcxml', + 'schema' => $marc_flavour + } + ); my $marc_xml = $biblio_metadata->metadata; $marc_xml = StripNonXmlChars($marc_xml); - my $record = eval { - MARC::Record::new_from_xml($marc_xml, 'UTF-8', $marc_flavour) - }; - if (!$record) { - Koha::Logger->get->warn( - "Failed to load MARCXML for deleted biblio with biblionumber \"$biblionumber\": $@" - ); + my $record = eval { MARC::Record::new_from_xml( $marc_xml, 'UTF-8', $marc_flavour ) }; + if ( !$record ) { + Koha::Logger->get->warn("Failed to load MARCXML for deleted biblio with biblionumber \"$biblionumber\": $@"); return; } + # Set deleted flag (record status, position 05) my $leader = $record->leader; substr $leader, 5, 1, 'd'; @@ -148,8 +144,8 @@ sub _get_deleted_biblio_for_export { } sub _get_authority_for_export { - my ($params) = @_; - my $authid = $params->{authid} || return; + my ($params) = @_; + my $authid = $params->{authid} || return; my $authority = Koha::MetadataRecord::Authority->get_from_authid($authid); return unless $authority; return $authority->record; @@ -166,10 +162,8 @@ sub _get_biblio_for_export { my $biblio = Koha::Biblios->find($biblionumber); my $record = eval { $biblio->metadata->record }; - if (!$record) { - Koha::Logger->get->warn( - "Failed to load MARCXML for biblio with biblionumber \"$biblionumber\": $@" - ); + if ( !$record ) { + Koha::Logger->get->warn("Failed to load MARCXML for biblio with biblionumber \"$biblionumber\": $@"); return; } @@ -184,12 +178,12 @@ sub _get_biblio_for_export { record => $record, embed_items => 1, biblionumber => $biblionumber, - itemnumbers => $itemnumbers, + itemnumbers => $itemnumbers, } ); - if ($only_export_items_for_branches && @$only_export_items_for_branches) { + if ( $only_export_items_for_branches && @$only_export_items_for_branches ) { my %export_items_for_branches = map { $_ => 1 } @$only_export_items_for_branches; - my ( $homebranchfield, $homebranchsubfield ) = GetMarcFromKohaField( 'items.homebranch' ); + my ( $homebranchfield, $homebranchsubfield ) = GetMarcFromKohaField('items.homebranch'); for my $itemfield ( $record->field($homebranchfield) ) { my $homebranch = $itemfield->subfield($homebranchsubfield); @@ -206,7 +200,7 @@ sub export { my ($params) = @_; my $record_type = $params->{record_type}; - my $record_ids = $params->{record_ids} || []; + my $record_ids = $params->{record_ids} || []; my $deleted_record_ids = $params->{deleted_record_ids} || []; my $format = $params->{format}; my $itemnumbers = $params->{itemnumbers} || []; # Does not make sense with record_type eq auths @@ -215,14 +209,14 @@ sub export { my $csv_profile_id = $params->{csv_profile_id}; my $output_filepath = $params->{output_filepath}; - if( !$record_type ) { - Koha::Logger->get->warn( "No record_type given." ); + if ( !$record_type ) { + Koha::Logger->get->warn("No record_type given."); return; } return unless @{$record_ids} || @{$deleted_record_ids} && $format ne 'csv'; my $fh; - if ( $output_filepath ) { + if ($output_filepath) { open $fh, '>', $output_filepath or die "Cannot open file $output_filepath ($!)"; select $fh; binmode $fh, ':encoding(UTF-8)' unless $format eq 'csv'; @@ -230,57 +224,59 @@ sub export { binmode STDOUT, ':encoding(UTF-8)' unless $format eq 'csv'; } - if ($format eq 'xml' || $format eq 'iso2709') { + if ( $format eq 'xml' || $format eq 'iso2709' ) { my @records; @records = map { - my $record = _get_record_for_export({ %{$params}, record_id => $_ }); + my $record = _get_record_for_export( { %{$params}, record_id => $_ } ); $record ? $record : (); } @{$record_ids}; my @deleted_records; - if (@{$deleted_record_ids}) { - my $resultset = Koha::Database->new() - ->schema() - ->resultset('DeletedbiblioMetadata'); + if ( @{$deleted_record_ids} ) { + my $resultset = Koha::Database->new()->schema()->resultset('DeletedbiblioMetadata'); @deleted_records = map { - my $record = _get_record_for_export({ - %{$params}, - record_type => 'deleted_bibs', - record_id => $_, - resultset => $resultset - }); + my $record = _get_record_for_export( + { + %{$params}, + record_type => 'deleted_bibs', + record_id => $_, + resultset => $resultset + } + ); $record ? $record : (); } @{$deleted_record_ids}; } if ( $format eq 'iso2709' ) { my $encoding_validator = sub { - my ($record, $record_type) = @_; - my $errorcount_on_decode = eval { scalar(MARC::File::USMARC->decode($record->as_usmarc)->warnings()) }; - if ($errorcount_on_decode || $@) { - my ($id_tag, $id_subfield) = GetMarcFromKohaField('biblio.biblionumber', ''); - my $record_id = $record->subfield($id_tag, $id_subfield); - my $msg = "$record_type $record_id could not be USMARC decoded/encoded. " . ($@ // ''); + my ( $record, $record_type ) = @_; + my $errorcount_on_decode = + eval { scalar( MARC::File::USMARC->decode( $record->as_usmarc )->warnings() ) }; + if ( $errorcount_on_decode || $@ ) { + my ( $id_tag, $id_subfield ) = GetMarcFromKohaField( 'biblio.biblionumber', '' ); + my $record_id = $record->subfield( $id_tag, $id_subfield ); + my $msg = "$record_type $record_id could not be USMARC decoded/encoded. " . ( $@ // '' ); chomp $msg; Koha::Logger->get->warn($msg); return 0; } return 1; }; - for my $record (grep { $encoding_validator->($_, 'Record') } @records) { + for my $record ( grep { $encoding_validator->( $_, 'Record' ) } @records ) { print $record->as_usmarc(); } if (@deleted_records) { - for my $deleted_record (grep { $encoding_validator->($_, 'Deleted record') } @deleted_records) { + for my $deleted_record ( grep { $encoding_validator->( $_, 'Deleted record' ) } @deleted_records ) { print $deleted_record->as_usmarc(); } } } elsif ( $format eq 'xml' ) { my $marcflavour = C4::Context->preference("marcflavour"); - MARC::File::XML->default_record_format( ( $marcflavour eq 'UNIMARC' && $record_type eq 'auths' ) ? 'UNIMARCAUTH' : $marcflavour ); + MARC::File::XML->default_record_format( + ( $marcflavour eq 'UNIMARC' && $record_type eq 'auths' ) ? 'UNIMARCAUTH' : $marcflavour ); print MARC::File::XML::header(); print "\n"; - for my $record (@records, @deleted_records) { + for my $record ( @records, @deleted_records ) { print MARC::File::XML::record($record); print "\n"; } @@ -289,7 +285,7 @@ sub export { } } elsif ( $format eq 'csv' ) { die 'There is no valid csv profile defined for this export' - unless Koha::CsvProfiles->find( $csv_profile_id ); + unless Koha::CsvProfiles->find($csv_profile_id); print marc2csv( $record_ids, $csv_profile_id, $itemnumbers ); } close $fh if $output_filepath; diff --git a/misc/export_records.pl b/misc/export_records.pl index 5364289a693..b4f1c4967d6 100755 --- a/misc/export_records.pl +++ b/misc/export_records.pl @@ -107,14 +107,14 @@ $record_type ||= 'bibs'; # Retrocompatibility for the format parameter $output_format = 'iso2709' if $output_format eq 'marc'; -if ($include_deleted || $deleted_only) { - if ($record_type ne 'bibs') { +if ( $include_deleted || $deleted_only ) { + if ( $record_type ne 'bibs' ) { pod2usage(q|Option "--include_deleted" or "--deleted_only" can only be used with "--record-type=bibs"|); } - if ($output_format eq 'csv') { + if ( $output_format eq 'csv' ) { pod2usage(q|Option "--include_deleted" or "--deleted_only" cannot be used with "--format=csv"|); } - if (!$timestamp) { + if ( !$timestamp ) { pod2usage(q|Option "--include_deleted" or "--deleted_only" must be combined with "--date"|); } } @@ -205,9 +205,10 @@ if ( $record_type eq 'bibs' ) { } } elsif ($timestamp) { unless ($deleted_only) { - if (!$dont_export_items) { + if ( !$dont_export_items ) { push @record_ids, $_->{biblionumber} for @{ - $dbh->selectall_arrayref(q| ( + $dbh->selectall_arrayref( + q| ( SELECT biblio_metadata.biblionumber FROM biblio_metadata LEFT JOIN items USING(biblionumber) @@ -219,25 +220,30 @@ if ( $record_type eq 'bibs' ) { LEFT JOIN deleteditems USING(biblionumber) WHERE biblio_metadata.timestamp >= ? OR deleteditems.timestamp >= ? - ) |, { Slice => {} }, ( $timestamp ) x 4 ); + ) |, { Slice => {} }, ($timestamp) x 4 + ); }; } else { push @record_ids, $_->{biblionumber} for @{ - $dbh->selectall_arrayref(q| ( + $dbh->selectall_arrayref( + q| ( SELECT biblio_metadata.biblionumber FROM biblio_metadata WHERE biblio_metadata.timestamp >= ? - ) |, { Slice => {} }, $timestamp ); + ) |, { Slice => {} }, $timestamp + ); }; } } - if ($include_deleted || $deleted_only) { + if ( $include_deleted || $deleted_only ) { push @deleted_record_ids, $_->{biblionumber} for @{ - $dbh->selectall_arrayref(q| + $dbh->selectall_arrayref( + q| SELECT `biblionumber` FROM `deletedbiblio` WHERE `timestamp` >= ? - |, { Slice => {} }, $timestamp); + |, { Slice => {} }, $timestamp + ); }; } } else { diff --git a/t/db_dependent/Exporter/Record.t b/t/db_dependent/Exporter/Record.t index ab63c6a1c02..8ffbaf50b78 100755 --- a/t/db_dependent/Exporter/Record.t +++ b/t/db_dependent/Exporter/Record.t @@ -79,10 +79,10 @@ if ($marcflavour eq 'UNIMARC') { my $deleted_biblio = MARC::Record->new(); $deleted_biblio->leader('00136nam a22000617a 4500'); $deleted_biblio->append_fields( - MARC::Field->new('100', ' ', ' ', a => 'Chopra, Deepak'), - MARC::Field->new('245', ' ', ' ', a => 'The seven spiritual laws of success'), + MARC::Field->new( '100', ' ', ' ', a => 'Chopra, Deepak'), + MARC::Field->new( $title_field_tag, ' ', ' ', a => 'The seven spiritual laws of success'), ); -my ($deleted_biblionumber) = AddBiblio($deleted_biblio, ''); +my ($deleted_biblionumber) = AddBiblio( $deleted_biblio, '' ); DelBiblio($deleted_biblionumber); my $bad_biblio = Koha::Biblio->new()->store(); @@ -164,9 +164,10 @@ subtest 'export xml' => sub { my $generated_xml_file = '/tmp/test_export.xml'; warning_like { Koha::Exporter::Record::export( - { record_type => 'bibs', + { + record_type => 'bibs', record_ids => [ $biblionumber_1, $bad_biblionumber, $biblionumber_2 ], - deleted_record_ids => [ $deleted_biblionumber ], + deleted_record_ids => [$deleted_biblionumber], format => 'xml', output_filepath => $generated_xml_file, } @@ -195,18 +196,20 @@ subtest 'export xml' => sub { # Leader has the expected value (and record status "d", length may have # changed) - ok( $deleted_record->leader =~ '\d{5}dam a22000617a 4500', 'Deleted record has the expected leader value' ); + ok( $deleted_record->leader =~ '\d{5}dam a22000\d{2}7a 4500', 'Deleted record has the expected leader value' ); }; subtest 'export iso2709' => sub { plan tests => 4; my $generated_mrc_file = '/tmp/test_export.mrc'; + # Get all item infos warning_like { Koha::Exporter::Record::export( - { record_type => 'bibs', + { + record_type => 'bibs', record_ids => [ $biblionumber_1, $bad_biblionumber, $biblionumber_2 ], - deleted_record_ids => [ $deleted_biblionumber ], + deleted_record_ids => [$deleted_biblionumber], format => 'iso2709', output_filepath => $generated_mrc_file, } @@ -228,7 +231,7 @@ subtest 'export iso2709' => sub { my $deleted_record = $records[2]; # Leader has the expected value (and record status "d", length may have # changed) - ok( $deleted_record->leader =~ '\d{5}dam a22000617a 4500', 'Deleted record has the expected leader value' ); + ok( $deleted_record->leader =~ '\d{5}dam a22000\d{2}7a 4500', 'Deleted record has the expected leader value' ); }; subtest 'export without record_type' => sub { -- 2.39.5