Bugzilla – Attachment 188153 Details for
Bug 41020
Add ability to use file transports for MARC ordering accounts
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 41020: Add delete_file() method to file transport classes
Bug-41020-Add-deletefile-method-to-file-transport-.patch (text/plain), 8.78 KB, created by
Martin Renvoize (ashimema)
on 2025-10-20 11:37:54 UTC
(
hide
)
Description:
Bug 41020: Add delete_file() method to file transport classes
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2025-10-20 11:37:54 UTC
Size:
8.78 KB
patch
obsolete
>From 15224b2d289ae827444fcbe956c98aafd2fc2094 Mon Sep 17 00:00:00 2001 >From: John Doe <you@example.com> >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 <martin.renvoize@openfifth.co.uk> >--- > 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<Parameters:> >+ >+=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<path> - 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<Usage Patterns:> >+ >+ # 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<Returns:> 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
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 41020
:
187931
|
187932
|
187933
|
187934
|
187935
|
187936
|
187937
|
187938
|
187939
|
187940
|
187941
|
187942
|
188012
|
188013
|
188014
|
188015
|
188016
|
188017
|
188018
|
188030
|
188031
|
188032
|
188033
|
188034
|
188146
|
188147
|
188148
|
188149
|
188150
| 188153 |
188154
|
188155
|
188156
|
188157