From 2249faf70210748d0ca1f5e1ff3d9ad63b113054 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Tue, 23 Sep 2025 17:28:27 +0100 Subject: [PATCH] Bug 38489: Update Koha::Edifact::Transport to use new file transport system This patch completely refactors the EDI transport handling to use the modern file_transports system instead of legacy direct connections. Changes: - Remove legacy FTP/SFTP connection code - Update constructor to load file_transport from vendor_edi_accounts - Update download_messages/upload_messages to use modern transport methods - Use new rename_file() method for EDI file processing workflow - Remove dependencies on Net::FTP and Net::SFTP::Foreign - Maintain all EDI-specific functionality (file filtering, ingestion) Cleanup: - Remove obsolete methods: file_download, sftp_download, ftp_download, sftp_upload, ftp_upload, file_upload, _abort_download - Update POD documentation to reflect new architecture - 184 lines of legacy code removed The transport system now provides: - Unified connection handling across all transport types - Enhanced error reporting and status tracking - Encrypted credential storage - Connection testing capabilities - Support for local directories (testing/development) Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie --- Koha/Edifact/Transport.pm | 425 ++++++++++---------------------------- 1 file changed, 108 insertions(+), 317 deletions(-) diff --git a/Koha/Edifact/Transport.pm b/Koha/Edifact/Transport.pm index f89868ae6a5..88838fad147 100644 --- a/Koha/Edifact/Transport.pm +++ b/Koha/Edifact/Transport.pm @@ -27,23 +27,26 @@ 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; use Koha::Database; use Koha::DateUtils qw( dt_from_string ); -use Koha::Encryption; +use Koha::File::Transports; sub new { my ( $class, $account_id ) = @_; my $database = Koha::Database->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 - transfer_date => dt_from_string(), + + # Get the file transport if configured + my $file_transport = $acct->file_transport_id ? Koha::File::Transports->find( $acct->file_transport_id ) : undef; + + my $self = { + account => $acct, + schema => $schema, + file_transport => $file_transport, + working_dir => C4::Context::temporary_directory, #temporary work directory + transfer_date => dt_from_string(), }; bless $self, $class; @@ -62,328 +65,147 @@ sub download_messages { my ( $self, $message_type ) = @_; $self->{message_type} = $message_type; - my @retrieved_files; - - if ( $self->{account}->transport eq 'SFTP' ) { - @retrieved_files = $self->sftp_download(); - } elsif ( $self->{account}->transport eq 'FILE' ) { - @retrieved_files = $self->file_download(); - } else { # assume FTP - @retrieved_files = $self->ftp_download(); + unless ( $self->{file_transport} ) { + carp "No file transport configured for EDI account " . $self->{account}->id; + return; } - return @retrieved_files; -} - -sub upload_messages { - my ( $self, @messages ) = @_; - if (@messages) { - if ( $self->{account}->transport eq 'SFTP' ) { - $self->sftp_upload(@messages); - } elsif ( $self->{account}->transport eq 'FILE' ) { - $self->file_upload(@messages); - } else { # assume FTP - $self->ftp_upload(@messages); - } - } - return; -} - -sub file_download { - my $self = shift; - my @downloaded_files; 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 $msg_hash = $self->message_hash(); - if ( opendir my $dh, $dir ) { - my @file_list = readdir $dh; - closedir $dh; - foreach my $filename (@file_list) { + my @downloaded_files; - if ( $filename =~ m/[.]$file_ext$/ ) { - if ( copy( "$dir/$filename", $self->{working_dir} ) ) { - } else { - carp "copy of $filename failed"; - next; - } - push @downloaded_files, $filename; - my $processed_name = $filename; - substr $processed_name, -3, 1, 'E'; - move( "$dir/$filename", "$dir/$processed_name" ); - } - } - $self->ingest( $msg_hash, @downloaded_files ); - } else { - carp "Cannot open $dir"; + # Connect to the transport + unless ( $self->{file_transport}->connect() ) { + carp "Failed to connect to file transport: " . $self->{file_transport}->id; return; } - return @downloaded_files; -} -sub sftp_download { - my $self = shift; - - my $file_ext = _get_file_ext( $self->{message_type} ); + # Change to download directory + my $download_dir = $self->{file_transport}->download_directory; + if ( $download_dir && !$self->{file_transport}->change_directory($download_dir) ) { + carp "Failed to change to download directory: $download_dir"; + return; + } - # C = ready to retrieve E = Edifact - my $msg_hash = $self->message_hash(); - my @downloaded_files; - my $port = $self->{account}->download_port ? $self->{account}->download_port : '22'; - my $sftp = Net::SFTP::Foreign->new( - host => $self->{account}->host, - user => $self->{account}->username, - password => Koha::Encryption->new->decrypt_hex( $self->{account}->password ), - port => $port, - timeout => 10, - ); - if ( $sftp->error ) { - return $self->_abort_download( - undef, - 'Unable to connect to remote host: ' . $sftp->error - ); + # Get file list + my $file_list = $self->{file_transport}->list_files(); + unless ($file_list) { + carp "Failed to get file list from transport"; + return; } - $sftp->setcwd( $self->{account}->download_directory ) - 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 - ); + + # Process files matching our criteria 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 - ); - last; - } - push @downloaded_files, $filename; - my $processed_name = $filename; - substr $processed_name, -3, 1, 'E'; - - #$sftp->atomic_rename( $filename, $processed_name ); - my $ret = $sftp->rename( $filename, $processed_name ); - if ( !$ret ) { - $self->_abort_download( - $sftp, - "Error renaming $filename: " . $sftp->error - ); - last; - } + my $local_file = "$self->{working_dir}/$filename"; - } - } - $sftp->disconnect; - $self->ingest( $msg_hash, @downloaded_files ); - - return @downloaded_files; -} - -sub ingest { - my ( $self, $msg_hash, @downloaded_files ) = @_; - foreach my $f (@downloaded_files) { - - # Check file has not been downloaded already - 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' ); - if ( !defined $file_content ) { - carp "Unable to read download file $f"; - next; - } - from_to( $file_content, 'iso-8859-1', 'utf8' ); - $msg_hash->{raw_msg} = $file_content; - $self->{schema}->resultset('EdifactMessage')->create($msg_hash); - } - return; -} - -sub ftp_download { - my $self = shift; - - my $file_ext = _get_file_ext( $self->{message_type} ); - - # C = ready to retrieve E = Edifact - - my $msg_hash = $self->message_hash(); - my @downloaded_files; - my $port = $self->{account}->download_port ? $self->{account}->download_port : '21'; - my $ftp = Net::FTP->new( - $self->{account}->host, - Port => $port, - Timeout => 10, - Passive => 1 - ) - or return $self->_abort_download( - undef, - "Cannot connect to " . $self->{account}->host . ": $EVAL_ERROR" - ); - $ftp->login( $self->{account}->username, Koha::Encryption->new->decrypt_hex( $self->{account}->password ) ) - 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() - ); - my $file_list = $ftp->ls() - or return $self->_abort_download( $ftp, 'cannot get file list from server' ); - - foreach my $filename ( @{$file_list} ) { + # Download the file + if ( $self->{file_transport}->download_file( $filename, $local_file ) ) { + push @downloaded_files, $filename; - if ( $filename =~ m/[.]$file_ext$/ ) { + # Rename file on server to mark as processed (EDI-specific behavior) + my $processed_name = $filename; + substr $processed_name, -3, 1, 'E'; - if ( !$ftp->get( $filename, "$self->{working_dir}/$filename" ) ) { - $self->_abort_download( - $ftp, - "Error retrieving $filename: " . $ftp->message - ); - last; + # Mark file as processed using the transport's rename functionality + $self->{file_transport}->rename_file( $filename, $processed_name ); + } else { + carp "Failed to download file: $filename"; } - - push @downloaded_files, $filename; - my $processed_name = $filename; - substr $processed_name, -3, 1, 'E'; - $ftp->rename( $filename, $processed_name ); } } - $ftp->quit; + # Ingest downloaded files $self->ingest( $msg_hash, @downloaded_files ); + # Clean up connection + $self->{file_transport}->disconnect(); + return @downloaded_files; } -sub ftp_upload { +sub upload_messages { my ( $self, @messages ) = @_; - my $port = $self->{account}->upload_port ? $self->{account}->upload_port : '21'; - my $ftp = Net::FTP->new( - $self->{account}->host, - Port => $port, - Timeout => 10, - Passive => 1 - ) - or return $self->_abort_download( - undef, - "Cannot connect to " . $self->{account}->host . ": $EVAL_ERROR" - ); - $ftp->login( $self->{account}->username, Koha::Encryption->new->decrypt_hex( $self->{account}->password ) ) - 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() - ); - foreach my $m (@messages) { - my $content = $m->raw_msg; - if ($content) { - open my $fh, '<', \$content; - if ( $ftp->put( $fh, $m->filename ) ) { - close $fh; - $m->transfer_date( $self->{transfer_date} ); - $m->status('sent'); - $m->update; - } else { - # error in transfer + unless (@messages) { + return; + } - } - } + unless ( $self->{file_transport} ) { + carp "No file transport configured for EDI account " . $self->{account}->id; + return; } - $ftp->quit; - return; -} + # Connect to the transport + unless ( $self->{file_transport}->connect() ) { + carp "Failed to connect to file transport: " . $self->{file_transport}->id; + return; + } + + # Change to upload directory + my $upload_dir = $self->{file_transport}->upload_directory; + if ( $upload_dir && !$self->{file_transport}->change_directory($upload_dir) ) { + carp "Failed to change to upload directory: $upload_dir"; + return; + } -sub sftp_upload { - my ( $self, @messages ) = @_; - my $port = $self->{account}->upload_port ? $self->{account}->upload_port : '22'; - my $sftp = Net::SFTP::Foreign->new( - host => $self->{account}->host, - user => $self->{account}->username, - password => Koha::Encryption->new->decrypt_hex( $self->{account}->password ), - port => $port, - timeout => 10, - ); - $sftp->die_on_error( "Cannot ssh to " . $self->{account}->host ); - $sftp->setcwd( $self->{account}->upload_directory ) - or return $self->_abort_download( - $sftp, - "Cannot change remote dir : " . $sftp->error - ); foreach my $m (@messages) { my $content = $m->raw_msg; if ($content) { - open my $fh, '<', \$content; - if ( $sftp->put( $fh, $m->filename ) ) { - close $fh; - $m->transfer_date( $self->{transfer_date} ); - $m->status('sent'); - $m->update; - } else { - # error in transfer + # Create temporary file for upload + my $temp_file = "$self->{working_dir}/" . $m->filename; - } - } - } - - # sftp will be closed on object destructor - return; -} + if ( open my $fh, '>', $temp_file ) { + print {$fh} $content; + close $fh; -sub file_upload { - my ( $self, @messages ) = @_; - my $dir = $self->{account}->upload_directory; - if ( -d $dir ) { - foreach my $m (@messages) { - my $content = $m->raw_msg; - if ($content) { - my $filename = $m->filename; - my $new_filename = "$dir/$filename"; - if ( open my $fh, '>', $new_filename ) { - print {$fh} $content; - close $fh; + # Upload the file + if ( $self->{file_transport}->upload_file( $temp_file, $m->filename ) ) { $m->transfer_date( $self->{transfer_date} ); $m->status('sent'); $m->update; } else { - carp "Could not transfer " . $m->filename . " : $ERRNO"; - next; + carp "Failed to upload file: " . $m->filename; } + + # Clean up temp file + unlink $temp_file; + } else { + carp "Could not create temporary file for upload: " . $m->filename; } } - } else { - carp "Upload directory $dir does not exist"; } + + # Clean up connection + $self->{file_transport}->disconnect(); + return; } -sub _abort_download { - my ( $self, $handle, $log_message ) = @_; +sub ingest { + my ( $self, $msg_hash, @downloaded_files ) = @_; + foreach my $f (@downloaded_files) { - my $a = $self->{account}->description; + # Check file has not been downloaded already + my $existing_file = $self->{schema}->resultset('EdifactMessage')->find( { filename => $f, } ); + if ($existing_file) { + carp "skipping ingest of $f : filename exists"; + next; + } - if ($handle) { - $handle->abort(); + $msg_hash->{filename} = $f; + my $file_content = read_file( "$self->{working_dir}/$f", binmode => ':raw' ); + if ( !defined $file_content ) { + carp "Unable to read download file $f"; + next; + } + from_to( $file_content, 'iso-8859-1', 'utf8' ); + $msg_hash->{raw_msg} = $file_content; + $self->{schema}->resultset('EdifactMessage')->create($msg_hash); } - $log_message .= ": $a"; - carp $log_message; - - #returns undef i.e. an empty array return; } @@ -435,17 +257,17 @@ $downlowd->download_messages('QUOTE'); =head1 DESCRIPTION -Module that handles Edifact download and upload transport -currently can use sftp or ftp -Or FILE to access a local directory (useful for testing) - +Module that handles Edifact download and upload transport using the modern +Koha::File::Transport system. Supports SFTP, FTP, and local directory +operations through a unified interface. =head1 METHODS =head2 new Creates an object of Edifact::Transport requires to be passed the id - identifying the relevant edi vendor account + identifying the relevant edi vendor account. The account must have a + file_transport_id configured to use the modern transport system. =head2 working_directory @@ -454,57 +276,26 @@ Or FILE to access a local directory (useful for testing) =head2 download_messages called with the message type to download will perform the download - using the appropriate transport method + using the configured file transport =head2 upload_messages passed an array of messages will upload them to the supplier site + using the configured file transport -=head2 file_download - -Missing POD for file_download. - -=head2 sftp_download - - called by download_messages to perform the download using SFTP =head2 ingest loads downloaded files into the database -=head2 ftp_download - - called by download_messages to perform the download using FTP - -=head2 ftp_upload - - called by upload_messages to perform the upload using ftp - -=head2 sftp_upload - - called by upload_messages to perform the upload using sftp - -=head2 file_upload - -Missing POD for file_upload. - -=head2 _abort_download - - 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 -=head2 set_transport_direct - - sets the direct ingest flag so that the object reads files from - the local file system useful in debugging - =head2 message_hash -Missing POD for message_hash. + creates the message hash structure for storing in the database =head1 AUTHOR -- 2.51.0