Bugzilla – Attachment 186850 Details for
Bug 38489
EDI should be updated to use the new FTP/SFTP Servers management page
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 38489: Update Koha::Edifact::Transport to use new file transport system
Bug-38489-Update-KohaEdifactTransport-to-use-new-f.patch (text/plain), 18.89 KB, created by
Martin Renvoize (ashimema)
on 2025-09-23 20:07:03 UTC
(
hide
)
Description:
Bug 38489: Update Koha::Edifact::Transport to use new file transport system
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2025-09-23 20:07:03 UTC
Size:
18.89 KB
patch
obsolete
>From 173b180b9e4c0d86a275c3ab6ea2c12d71ebd7e7 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >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) >--- > 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 ec59ae41109..d9e15d36d85 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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 38489
:
175204
|
175205
|
175206
|
175207
|
176118
|
176119
|
176161
|
185324
|
186819
|
186820
|
186845
|
186846
|
186847
|
186848
|
186849
|
186850
|
186851
|
186852
|
186854
|
187080
|
187081
|
187082
|
187083
|
187084
|
187085
|
187086
|
187087
|
187088
|
187089
|
187108
|
187147
|
187148
|
187149
|
187150
|
187151
|
187152
|
187153
|
187154
|
187155
|
187156
|
187157
|
187158
|
187159
|
187270
|
187271
|
187272
|
187273
|
187274
|
187275
|
187276
|
187277
|
187278
|
187279
|
187280
|
187281
|
187283
|
187284
|
187285
|
187286
|
187287
|
187288
|
187289
|
187290
|
187291
|
187292
|
187293
|
187294