From 15224b2d289ae827444fcbe956c98aafd2fc2094 Mon Sep 17 00:00:00 2001 From: John Doe Date: Wed, 15 Oct 2025 18:06:47 +0000 Subject: [PATCH] Bug 41020: Add delete_file() method to file transport classes This patch adds the ability to delete files from remote servers through the file transport API. This is needed for the MARC ordering feature to clean up processed files from FTP/SFTP servers. Changes: - Added abstract delete_file() method to Koha::File::Transport base class - Implemented delete_file() in Koha::File::Transport::FTP using Net::FTP->delete() - Implemented delete_file() in Koha::File::Transport::SFTP using Net::SFTP::Foreign->remove() - Implemented delete_file() in Koha::File::Transport::Local using unlink() - All implementations follow the standard message/error handling pattern - Updated polymorphic tests to verify delete_file() capability The delete_file() method: - Takes a filename as parameter (relative to current directory) - Returns true on success, undef on failure - Adds appropriate success/error messages to the transport message queue - Follows the same pattern as existing methods (download_file, rename_file) Test plan: 1. prove t/db_dependent/Koha/File/Transports.t 2. Verify all transport types support delete_file() 3. Test with MARC ordering cronjob using --dr flag (added in main patch) Signed-off-by: Martin Renvoize --- Koha/File/Transport.pm | 76 +++++++++++++++++++++++++++ Koha/File/Transport/FTP.pm | 30 +++++++++++ Koha/File/Transport/Local.pm | 58 ++++++++++++++++++++ Koha/File/Transport/SFTP.pm | 30 +++++++++++ t/db_dependent/Koha/File/Transports.t | 4 +- 5 files changed, 197 insertions(+), 1 deletion(-) diff --git a/Koha/File/Transport.pm b/Koha/File/Transport.pm index 30375cb24fb..01bda8ea691 100644 --- a/Koha/File/Transport.pm +++ b/Koha/File/Transport.pm @@ -548,6 +548,82 @@ sub download_file { return $self->_download_file( $remote_file, $local_file ); } +=head3 delete_file + + my $success = $transport->delete_file($remote_file, \%options); + +Deletes a file from the remote server. Automatically establishes a connection if needed. + +B + +=over 4 + +=item * C<$remote_file> - Remote filename (not a path, just the filename) (required) + +=item * C<\%options> - Optional hashref with keys: + +=over 4 + +=item * C - Directory path containing the file. If provided, uses this directory +for this operation only (simplified API). If omitted, behavior depends on whether +change_directory() has been called explicitly (see DESCRIPTION). + +=back + +=back + +B + + # Pattern 1: Simplified API with custom path + $transport->delete_file('old.txt', { path => '/temp/' }); + + # Pattern 2: Simplified API with configured download_directory + $transport->delete_file('old.txt'); + + # Pattern 3: Traditional API with explicit directory + $transport->change_directory('/temp/'); + $transport->delete_file('old.txt'); + +B True on success, undef on failure. Check object_messages() for details. + +=cut + +sub delete_file { + my ( $self, $remote_file, $options ) = @_; + + return unless $self->_ensure_connected(); + + # Only auto-change directory if: + # 1. Options provided with custom path (simplified API), OR + # 2. No explicit directory set by user AND default download_directory exists (traditional API) + if ( $options && $options->{path} ) { + + # Simplified API - use custom path + return unless $self->_auto_change_directory( 'download', $options->{path} ); + } elsif ( !$self->{_user_set_directory} ) { + + # Traditional API - use default directory only if user hasn't set one + return unless $self->_auto_change_directory( 'download', undef ); + } + + return $self->_delete_file($remote_file); +} + +=head3 _delete_file + + $transport->_delete_file($remote_file); + +Internal method that performs the protocol-specific file deletion operation. +Must be implemented by subclasses. Called by delete_file after connection +and directory management. + +=cut + +sub _delete_file { + my ($self) = @_; + die "Subclass must implement _delete_file"; +} + =head3 _download_file $transport->_download_file($remote_file, $local_file); diff --git a/Koha/File/Transport/FTP.pm b/Koha/File/Transport/FTP.pm index 47bc264054f..fd484a6623f 100644 --- a/Koha/File/Transport/FTP.pm +++ b/Koha/File/Transport/FTP.pm @@ -122,8 +122,38 @@ sub _download_file { return 1; } +=head3 _delete_file + + my $success = $server->_delete_file($filename); + +Passed a filename, this will delete the file from the current directory of the server connection. + +Returns true on success or undefined on failure. + +=cut + +sub _delete_file { + my ( $self, $remote_file ) = @_; + my $operation = "delete"; + + $self->{connection}->delete($remote_file) or return $self->_abort_operation( $operation, $remote_file ); + + $self->add_message( + { + message => $operation, + type => 'success', + payload => { + remote_file => $remote_file, + } + } + ); + + return 1; +} + =head3 _change_directory + my $success = $server->_change_directory($directory); Passed a directory name, this will change the current directory of the server connection. diff --git a/Koha/File/Transport/Local.pm b/Koha/File/Transport/Local.pm index 568f30cf5b4..0e6bbc49748 100644 --- a/Koha/File/Transport/Local.pm +++ b/Koha/File/Transport/Local.pm @@ -390,6 +390,64 @@ sub _rename_file { return 1; } +=head3 _delete_file + + $server->_delete_file(); + +Deletes a file in the current directory. + +Returns true on success or undefined on failure. + +=cut + +sub _delete_file { + my ( $self, $remote_file ) = @_; + my $operation = "delete"; + + my $directory = $self->{current_directory} || $self->download_directory || '.'; + my $source = File::Spec->catfile( $directory, $remote_file ); + + # Check if the file exists + unless ( -e $source ) { + $self->add_message( + { + message => $operation, + type => 'error', + payload => { + error => "File not found: $source", + path => $source + } + } + ); + return; + } + + # Attempt to delete the file + unless ( unlink $source ) { + $self->add_message( + { + message => $operation, + type => 'error', + payload => { + error => "File could not be deleted: $!", + path => $source + } + } + ); + return; # Return undef on failure + } + + $self->add_message( + { + message => $operation, + type => 'success', + payload => { path => $source } + } + ); + + return 1; +} + =head3 _is_connected Internal method to check if transport is currently connected. diff --git a/Koha/File/Transport/SFTP.pm b/Koha/File/Transport/SFTP.pm index 0df9c30b756..9f070427e65 100644 --- a/Koha/File/Transport/SFTP.pm +++ b/Koha/File/Transport/SFTP.pm @@ -21,6 +21,7 @@ use Koha::Logger; use File::Spec; use IO::File; +use JSON qw( encode_json ); use Net::SFTP::Foreign; use Try::Tiny; @@ -134,6 +135,35 @@ sub _download_file { return 1; } +=head3 _delete_file + + my $success = $server->_delete_file($filename); + +Passed a filename, this will delete the file from the current directory of the server connection. + +Returns true on success or undefined on failure. + +=cut + +sub _delete_file { + my ( $self, $remote_file ) = @_; + my $operation = "delete"; + + $self->{connection}->remove($remote_file) or return $self->_abort_operation( $operation, $remote_file ); + + $self->add_message( + { + message => $operation, + type => 'success', + payload => { + remote_file => $remote_file, + } + } + ); + + return 1; +} + =head3 _change_directory my $success = $server->_change_directory($directory); diff --git a/t/db_dependent/Koha/File/Transports.t b/t/db_dependent/Koha/File/Transports.t index 75af7a5055d..3dbc52f888c 100755 --- a/t/db_dependent/Koha/File/Transports.t +++ b/t/db_dependent/Koha/File/Transports.t @@ -31,7 +31,7 @@ my $schema = Koha::Database->new->schema; my $builder = t::lib::TestBuilder->new; subtest 'Polymorphic object creation' => sub { - plan tests => 6; + plan tests => 7; $schema->storage->txn_begin; @@ -92,6 +92,8 @@ subtest 'Polymorphic object creation' => sub { can_ok( $local_transport, 'rename_file' ); + can_ok( $local_transport, 'delete_file' ); + $schema->storage->txn_rollback; }; -- 2.51.0