From 8f806f1c799a822d526d2e0d7bda37d51644409a Mon Sep 17 00:00:00 2001 From: Jake Deery Date: Fri, 3 Jan 2025 13:24:24 +0000 Subject: [PATCH] Bug 38489: (follow-up) tidy EDIfact files This patch follows-up by by tidying the following files: * Koha/Edifact/Transport.pm * admin/edi_accounts.pl --- Koha/Edifact/Transport.pm | 154 ++++++++++++++++++++------------------ admin/edi_accounts.pl | 47 ++++++------ 2 files changed, 105 insertions(+), 96 deletions(-) diff --git a/Koha/Edifact/Transport.pm b/Koha/Edifact/Transport.pm index 64bd0f8997..480a382313 100644 --- a/Koha/Edifact/Transport.pm +++ b/Koha/Edifact/Transport.pm @@ -23,9 +23,9 @@ use utf8; use Carp qw( carp ); use DateTime; -use Encode qw( from_to ); -use English qw{ -no_match_vars }; -use File::Copy qw( copy move ); +use Encode qw( from_to ); +use English qw{ -no_match_vars }; +use File::Copy qw( copy move ); use File::Slurp qw( read_file ); use Net::FTP; use Net::SFTP::Foreign; @@ -40,9 +40,9 @@ sub new { my $schema = $database->schema(); my $acct = $schema->resultset('VendorEdiAccount')->find($account_id); my $self = { - account => $acct, - schema => $schema, - working_dir => C4::Context::temporary_directory, #temporary work directory + account => $acct, + schema => $schema, + working_dir => C4::Context::temporary_directory, #temporary work directory transfer_date => dt_from_string(), }; @@ -65,7 +65,7 @@ sub download_messages { my $sftp_server_id = $self->{account}->download_sftp_server_id; my $sftp_server_transport; $sftp_server_transport = Koha::SFTP::Servers->find($sftp_server_id)->transport - if ( $sftp_server_id ); + if ($sftp_server_id); my @retrieved_files; @@ -88,7 +88,7 @@ sub upload_messages { my $sftp_server_id = $self->{account}->upload_sftp_server_id; my $sftp_server_transport; $sftp_server_transport = Koha::SFTP::Servers->find($sftp_server_id)->transport - if ( $sftp_server_id ); + if ($sftp_server_id); if (@messages) { if ( !$sftp_server_transport ) { @@ -109,8 +109,9 @@ sub file_download { my $file_ext = _get_file_ext( $self->{message_type} ); - my $dir = $self->{account}->download_directory; # makes code more readable - # C = ready to retrieve E = Edifact + my $dir = $self->{account}->download_directory; # makes code more readable + + # C = ready to retrieve E = Edifact my $msg_hash = $self->message_hash(); if ( opendir my $dh, $dir ) { my @file_list = readdir $dh; @@ -119,8 +120,7 @@ sub file_download { if ( $filename =~ m/[.]$file_ext$/ ) { if ( copy( "$dir/$filename", $self->{working_dir} ) ) { - } - else { + } else { carp "copy of $filename failed"; next; } @@ -131,8 +131,7 @@ sub file_download { } } $self->ingest( $msg_hash, @downloaded_files ); - } - else { + } else { carp "Cannot open $dir"; return; } @@ -162,23 +161,31 @@ sub sftp_download { timeout => 10, ); if ( $sftp->error ) { - return $self->_abort_download( undef, - 'Unable to connect to remote host: ' . $sftp->error ); + return $self->_abort_download( + undef, + 'Unable to connect to remote host: ' . $sftp->error + ); } $sftp->setcwd( $self->{account}->download_directory ) - or return $self->_abort_download( $sftp, - "Cannot change remote dir: " . $sftp->error ); + or return $self->_abort_download( + $sftp, + "Cannot change remote dir: " . $sftp->error + ); my $file_list = $sftp->ls() - or return $self->_abort_download( $sftp, - "cannot get file list from server: " . $sftp->error ); + or return $self->_abort_download( + $sftp, + "cannot get file list from server: " . $sftp->error + ); foreach my $file ( @{$file_list} ) { my $filename = $file->{filename}; if ( $filename =~ m/[.]$file_ext$/ ) { $sftp->get( $filename, "$self->{working_dir}/$filename" ); if ( $sftp->error ) { - $self->_abort_download( $sftp, - "Error retrieving $filename: " . $sftp->error ); + $self->_abort_download( + $sftp, + "Error retrieving $filename: " . $sftp->error + ); last; } push @downloaded_files, $filename; @@ -188,8 +195,10 @@ sub sftp_download { #$sftp->atomic_rename( $filename, $processed_name ); my $ret = $sftp->rename( $filename, $processed_name ); if ( !$ret ) { - $self->_abort_download( $sftp, - "Error renaming $filename: " . $sftp->error ); + $self->_abort_download( + $sftp, + "Error renaming $filename: " . $sftp->error + ); last; } @@ -206,16 +215,14 @@ sub ingest { foreach my $f (@downloaded_files) { # Check file has not been downloaded already - my $existing_file = $self->{schema}->resultset('EdifactMessage') - ->find( { filename => $f, } ); + my $existing_file = $self->{schema}->resultset('EdifactMessage')->find( { filename => $f, } ); if ($existing_file) { carp "skipping ingest of $f : filename exists"; next; } $msg_hash->{filename} = $f; - my $file_content = - read_file( "$self->{working_dir}/$f", binmode => ':raw' ); + my $file_content = read_file( "$self->{working_dir}/$f", binmode => ':raw' ); if ( !defined $file_content ) { carp "Unable to read download file $f"; next; @@ -243,7 +250,7 @@ sub ftp_download { my $msg_hash = $self->message_hash(); my @downloaded_files; - my $ftp = Net::FTP->new( + my $ftp = Net::FTP->new( $sftp_server->host, Port => $sftp_server->port, Timeout => 10, @@ -254,21 +261,24 @@ sub ftp_download { "Cannot connect to " . $sftp_server->host . ": " . $EVAL_ERROR ); $ftp->login( $sftp_server->user_name, $sftp_server->plain_text_password ) - or return $self->_abort_download( $ftp, "Cannot login: $ftp->message()" ); + or return $self->_abort_download( $ftp, "Cannot login: $ftp->message()" ); $ftp->cwd( $self->{account}->download_directory ) - or return $self->_abort_download( $ftp, - "Cannot change remote dir : $ftp->message()" ); + or return $self->_abort_download( + $ftp, + "Cannot change remote dir : $ftp->message()" + ); my $file_list = $ftp->ls() - or - return $self->_abort_download( $ftp, 'cannot get file list from server' ); + or return $self->_abort_download( $ftp, 'cannot get file list from server' ); foreach my $filename ( @{$file_list} ) { if ( $filename =~ m/[.]$file_ext$/ ) { if ( !$ftp->get( $filename, "$self->{working_dir}/$filename" ) ) { - $self->_abort_download( $ftp, - "Error retrieving $filename: $ftp->message" ); + $self->_abort_download( + $ftp, + "Error retrieving $filename: $ftp->message" + ); last; } @@ -295,7 +305,7 @@ sub ftp_upload { return unless $sftp_server; - my $ftp = Net::FTP->new( + my $ftp = Net::FTP->new( $sftp_server->host, Port => $sftp_server->port, Timeout => 10, @@ -306,10 +316,12 @@ sub ftp_upload { "Cannot connect to " . $sftp_server->host . ": " . $EVAL_ERROR ); $ftp->login( $sftp_server->user_name, $sftp_server->plain_text_password ) - or return $self->_abort_download( $ftp, "Cannot login: $ftp->message()" ); + or return $self->_abort_download( $ftp, "Cannot login: $ftp->message()" ); $ftp->cwd( $self->{account}->upload_directory ) - or return $self->_abort_download( $ftp, - "Cannot change remote dir : $ftp->message()" ); + or return $self->_abort_download( + $ftp, + "Cannot change remote dir : $ftp->message()" + ); foreach my $m (@messages) { my $content = $m->raw_msg; if ($content) { @@ -319,8 +331,8 @@ sub ftp_upload { $m->transfer_date( $self->{transfer_date} ); $m->status('sent'); $m->update; - } - else { + } else { + # error in transfer } @@ -348,10 +360,12 @@ sub sftp_upload { port => $sftp_server->port, timeout => 10, ); - $sftp->die_on_error("Cannot ssh to " . $sftp_server->host); + $sftp->die_on_error( "Cannot ssh to " . $sftp_server->host ); $sftp->setcwd( $self->{account}->upload_directory ) - or return $self->_abort_download( $sftp, - "Cannot change remote dir : " . $sftp->error ); + or return $self->_abort_download( + $sftp, + "Cannot change remote dir : " . $sftp->error + ); foreach my $m (@messages) { my $content = $m->raw_msg; if ($content) { @@ -361,8 +375,8 @@ sub sftp_upload { $m->transfer_date( $self->{transfer_date} ); $m->status('sent'); $m->update; - } - else { + } else { + # error in transfer } @@ -388,15 +402,13 @@ sub file_upload { $m->transfer_date( $self->{transfer_date} ); $m->status('sent'); $m->update; - } - else { + } else { carp "Could not transfer $m->filename : $ERRNO"; next; } } } - } - else { + } else { carp "Upload directory $dir does not exist"; } return; @@ -474,66 +486,66 @@ Or FILE to access a local directory (useful for testing) =head2 new - Creates an object of Edifact::Transport requires to be passed the id - identifying the relevant edi vendor account +Creates an object of Edifact::Transport requires to be passed the id +identifying the relevant edi vendor account =head2 working_directory - getter and setter for the working_directory attribute +getter and setter for the working_directory attribute =head2 download_messages - called with the message type to download will perform the download - using the appropriate transport method +called with the message type to download will perform the download +using the appropriate transport method =head2 upload_messages - passed an array of messages will upload them to the supplier site +passed an array of messages will upload them to the supplier site =head2 sftp_download - called by download_messages to perform the download using SFTP +called by download_messages to perform the download using SFTP =head2 ingest - loads downloaded files into the database +loads downloaded files into the database =head2 ftp_download - called by download_messages to perform the download using FTP +called by download_messages to perform the download using FTP =head2 ftp_upload - called by upload_messages to perform the upload using ftp +called by upload_messages to perform the upload using ftp =head2 sftp_upload - called by upload_messages to perform the upload using sftp +called by upload_messages to perform the upload using sftp =head2 _abort_download - internal routine to halt operation on error and supply a stacktrace +internal routine to halt operation on error and supply a stacktrace =head2 _get_file_ext - internal method returning standard suffix for file names - according to message type +internal method returning standard suffix for file names +according to message type =head2 set_transport_direct - sets the direct ingest flag so that the object reads files from - the local file system useful in debugging +sets the direct ingest flag so that the object reads files from +the local file system useful in debugging =head1 AUTHOR - Colin Campbell +Colin Campbell =head1 COPYRIGHT - Copyright 2014,2015 PTFS-Europe Ltd - This program is free software, You may redistribute it under - under the terms of the GNU General Public License +Copyright 2014,2015 PTFS-Europe Ltd +This program is free software, You may redistribute it under +under the terms of the GNU General Public License =cut diff --git a/admin/edi_accounts.pl b/admin/edi_accounts.pl index 2b045cb4ea..d474992add 100755 --- a/admin/edi_accounts.pl +++ b/admin/edi_accounts.pl @@ -21,20 +21,20 @@ use Modern::Perl; use CGI; -use C4::Auth qw( get_template_and_user ); +use C4::Auth qw( get_template_and_user ); use C4::Output qw( output_html_with_http_headers ); use Koha::Database; use Koha::Plugins; -our $input = CGI->new(); +our $input = CGI->new(); our $schema = Koha::Database->new()->schema(); our ( $template, $loggedinuser, $cookie ) = get_template_and_user( { - template_name => 'admin/edi_accounts.tt', - query => $input, - type => 'intranet', - flagsrequired => { acquisition => 'edi_manage' }, + template_name => 'admin/edi_accounts.tt', + query => $input, + type => 'intranet', + flagsrequired => { acquisition => 'edi_manage' }, } ); @@ -47,7 +47,7 @@ if ( $op eq 'acct_form' ) { my @vendors = $schema->resultset('Aqbookseller')->search( undef, { - columns => [ 'name', 'id' ], + columns => [ 'name', 'id' ], order_by => { -asc => 'name' } } ); @@ -63,35 +63,35 @@ if ( $op eq 'acct_form' ) { $template->param( sftp_servers => \@sftp_servers ); if ( C4::Context->config("enable_plugins") ) { - my @plugins = Koha::Plugins->new()->GetPlugins({ - method => 'edifact', - }); + my @plugins = Koha::Plugins->new()->GetPlugins( + { + method => 'edifact', + } + ); $template->param( plugins => \@plugins ); } -} -elsif ( $op eq 'delete_confirm' ) { +} elsif ( $op eq 'delete_confirm' ) { show_account(); $template->param( delete_confirm => 1 ); -} -else { +} else { if ( $op eq 'cud-save' ) { # validate & display my $id = $input->param('id'); my $fields = { description => scalar $input->param('description'), - upload_sftp_server_id => $input->param('upload_sftp_server_id') || undef, + upload_sftp_server_id => $input->param('upload_sftp_server_id') || undef, download_sftp_server_id => $input->param('download_sftp_server_id') || undef, vendor_id => scalar $input->param('vendor_id'), upload_directory => scalar $input->param('upload_directory'), download_directory => scalar $input->param('download_directory'), san => scalar $input->param('san'), standard => scalar $input->param('standard'), - quotes_enabled => $input->param('quotes_enabled') ? 1 : 0, - invoices_enabled => $input->param('invoices_enabled') ? 1 : 0, - orders_enabled => $input->param('orders_enabled') ? 1 : 0, + quotes_enabled => $input->param('quotes_enabled') ? 1 : 0, + invoices_enabled => $input->param('invoices_enabled') ? 1 : 0, + orders_enabled => $input->param('orders_enabled') ? 1 : 0, responses_enabled => $input->param('responses_enabled') ? 1 : 0, - auto_orders => $input->param('auto_orders') ? 1 : 0, + auto_orders => $input->param('auto_orders') ? 1 : 0, id_code_qualifier => scalar $input->param('id_code_qualifier'), plugin => scalar $input->param('plugin'), }; @@ -102,15 +102,12 @@ else { id => $id, } )->update_all($fields); - } - else { # new record + } else { # new record $schema->resultset('VendorEdiAccount')->create($fields); } - } - elsif ( $op eq 'cud-delete_confirmed' ) { + } elsif ( $op eq 'cud-delete_confirmed' ) { - $schema->resultset('VendorEdiAccount') - ->search( { id => scalar $input->param('id'), } )->delete_all; + $schema->resultset('VendorEdiAccount')->search( { id => scalar $input->param('id'), } )->delete_all; } # we do a default dispaly after deletes and saves -- 2.43.0