Bug 41020 - Add ability to use file transports for MARC ordering accounts
Summary: Add ability to use file transports for MARC ordering accounts
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Kyle M Hall (khall)
QA Contact: Tomás Cohen Arazi (tcohen)
URL:
Keywords:
Depends on: 39190 40811
Blocks: 41050
  Show dependency treegraph
 
Reported: 2025-10-14 19:01 UTC by Kyle M Hall (khall)
Modified: 2025-10-25 10:45 UTC (History)
1 user (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Comma delimited list of Sponsors:
Crowdfunding goal: 0
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 41020: Add ability to delete files to Koha::File::Transport and derived classes (5.59 KB, patch)
2025-10-15 18:36 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Add ability to delete files to Koha::File::Transport and derived classes (5.59 KB, patch)
2025-10-15 18:37 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Add ability to use file transports for marc ordering accounts (11.84 KB, patch)
2025-10-15 18:37 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Update schema files [DO NOT PUSH] (3.17 KB, patch)
2025-10-15 18:37 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Add ability to delete files to Koha::File::Transport and derived classes (5.57 KB, patch)
2025-10-15 18:45 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Add ability to delete files to Koha::File::Transport and derived classes (5.57 KB, patch)
2025-10-15 18:45 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Add ability to use file transports for marc ordering accounts (11.84 KB, patch)
2025-10-15 18:45 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Update schema files [DO NOT PUSH] (3.17 KB, patch)
2025-10-15 18:45 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Add ability to delete files to Koha::File::Transport and derived classes (5.39 KB, patch)
2025-10-15 18:47 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Add ability to delete files to Koha::File::Transport and derived classes (5.39 KB, patch)
2025-10-15 18:48 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Add ability to use file transports for marc ordering accounts (11.84 KB, patch)
2025-10-15 18:48 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Update schema files [DO NOT PUSH] (3.17 KB, patch)
2025-10-15 18:48 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Add ability to use file transports for marc ordering accounts (11.99 KB, patch)
2025-10-16 14:49 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Update FTP.pm _abort_operation prototype to match SFTP.pm (1.60 KB, patch)
2025-10-16 14:49 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Add ability to use file transports for marc ordering accounts (11.99 KB, patch)
2025-10-16 14:49 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Update schema files [DO NOT PUSH] (3.21 KB, patch)
2025-10-16 14:49 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Update FTP.pm _abort_operation prototype to match SFTP.pm (1.60 KB, patch)
2025-10-16 14:50 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Add ability to use file transports for marc ordering accounts (11.99 KB, patch)
2025-10-16 14:50 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Update schema files [DO NOT PUSH] (3.21 KB, patch)
2025-10-16 14:50 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Add ability to delete files to Koha::File::Transport and derived classes (5.58 KB, patch)
2025-10-16 19:25 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Add ability to delete files to Koha::File::Transport and derived classes (5.58 KB, patch)
2025-10-16 19:25 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Update FTP.pm _abort_operation prototype to match SFTP.pm (1.60 KB, patch)
2025-10-16 19:26 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Add ability to use file transports for marc ordering accounts (11.99 KB, patch)
2025-10-16 19:26 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Update schema files [DO NOT PUSH] (3.21 KB, patch)
2025-10-16 19:26 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 41020: Add delete_file() method to file transport classes (6.72 KB, patch)
2025-10-20 09:33 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41020: Standardize _abort_operation() signature in FTP transport (2.59 KB, patch)
2025-10-20 09:33 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41020: Add ability to use file transports for marc ordering accounts (12.00 KB, patch)
2025-10-20 09:33 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41020: Update schema files [DO NOT PUSH] (3.22 KB, patch)
2025-10-20 09:33 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41020: (follow-up) Modernize code to use Koha objects and relationship accessors (11.75 KB, patch)
2025-10-20 09:33 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41020: Add delete_file() method to file transport classes (8.78 KB, patch)
2025-10-20 11:37 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41020: Standardize _abort_operation() signature in FTP transport (2.59 KB, patch)
2025-10-20 11:37 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41020: Add ability to use file transports for marc ordering accounts (12.00 KB, patch)
2025-10-20 11:37 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41020: Update schema files [DO NOT PUSH] (3.22 KB, patch)
2025-10-20 11:38 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41020: (follow-up) Modernize code to use Koha objects and relationship accessors (11.75 KB, patch)
2025-10-20 11:38 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall (khall) 2025-10-14 19:01:23 UTC
FTP and SFTP server management was added to Koha in bug 39190. This feature should be made available to the marc order accounts feature so external processes are not needed to downloading the marc order files.
Comment 1 Kyle M Hall (khall) 2025-10-15 18:36:43 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall (khall) 2025-10-15 18:37:07 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall (khall) 2025-10-15 18:37:15 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall (khall) 2025-10-15 18:37:18 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall (khall) 2025-10-15 18:45:10 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall (khall) 2025-10-15 18:45:31 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall (khall) 2025-10-15 18:45:39 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall (khall) 2025-10-15 18:45:41 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall (khall) 2025-10-15 18:47:50 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall (khall) 2025-10-15 18:48:09 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall (khall) 2025-10-15 18:48:18 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall (khall) 2025-10-15 18:48:20 UTC Comment hidden (obsolete)
Comment 13 Martin Renvoize (ashimema) 2025-10-16 10:23:32 UTC
Comment on attachment 187941 [details] [review]
Bug 41020: Add ability to use file transports for marc ordering accounts

Review of attachment 187941 [details] [review]:
-----------------------------------------------------------------

Generally looks good, couple of minor queries of why we took a few shortcuts ;)

::: admin/marc_order_accounts.pl
@@ +61,5 @@
>      my @matchers = C4::Matcher::GetMatcherList();
>      $template->param( available_matchers => \@matchers );
>  
> +    # Get available file transports for selection
> +    my @file_transports = $schema->resultset('FileTransport')->search(

Was it intentional to use DBIC directly instead of doing a Koha::File::Transports->search() call?

::: misc/cronjobs/marc_ordering_process.pl
@@ +111,5 @@
>      }
>  
>      my $working_dir = $acct->download_directory;
> +
> +    my $file_transport = $acct->file_transport_id ? Koha::File::Transports->find( $acct->file_transport_id ) : undef;

Any reason to do this here rather than introducing a 'transport' relation accessor to give you back a Koha::File::Transport object directly if one exists?  i.e. why in the script not the object as a relation?
Comment 14 Kyle M Hall (khall) 2025-10-16 14:49:16 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall (khall) 2025-10-16 14:49:21 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall (khall) 2025-10-16 14:49:22 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall (khall) 2025-10-16 14:49:24 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall (khall) 2025-10-16 14:50:17 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall (khall) 2025-10-16 14:50:18 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall (khall) 2025-10-16 14:50:20 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall (khall) 2025-10-16 14:50:21 UTC
Signed-off-by: John Doe <you@example.com>
Comment 22 Kyle M Hall (khall) 2025-10-16 19:25:42 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall (khall) 2025-10-16 19:25:57 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall (khall) 2025-10-16 19:26:06 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall (khall) 2025-10-16 19:26:10 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall (khall) 2025-10-16 19:26:12 UTC Comment hidden (obsolete)
Comment 27 Martin Renvoize (ashimema) 2025-10-20 09:33:21 UTC Comment hidden (obsolete)
Comment 28 Martin Renvoize (ashimema) 2025-10-20 09:33:23 UTC Comment hidden (obsolete)
Comment 29 Martin Renvoize (ashimema) 2025-10-20 09:33:25 UTC Comment hidden (obsolete)
Comment 30 Martin Renvoize (ashimema) 2025-10-20 09:33:28 UTC Comment hidden (obsolete)
Comment 31 Martin Renvoize (ashimema) 2025-10-20 09:33:30 UTC Comment hidden (obsolete)
Comment 32 Martin Renvoize (ashimema) 2025-10-20 11:37:54 UTC
Created attachment 188153 [details] [review]
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>
Comment 33 Martin Renvoize (ashimema) 2025-10-20 11:37:56 UTC
Created attachment 188154 [details] [review]
Bug 41020: Standardize _abort_operation() signature in FTP transport

This patch updates Koha::File::Transport::FTP::_abort_operation() to
accept an optional $path parameter, matching the signature used in
Koha::File::Transport::SFTP.

This improves error reporting by including the path/file that caused
the operation to fail in the error message payload. Previously, the
FTP implementation only included the error detail and message, making
it harder to debug which file or directory caused the problem.

Changes:
- Updated _abort_operation($operation) to _abort_operation($operation, $path)
- Updated change_directory() call to pass $remote_directory to _abort_operation
- Added path to error payload, using either the passed path or pwd() as fallback

This standardization ensures consistent error reporting across all
transport implementations and makes debugging transport issues easier.

Test plan:
1. Verify FTP transport operations still work correctly
2. Check that error messages now include the path that failed
3. Confirm no regressions in existing FTP functionality

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 34 Martin Renvoize (ashimema) 2025-10-20 11:37:58 UTC
Created attachment 188155 [details] [review]
Bug 41020: Add ability to use file transports for marc ordering accounts

FTP and SFTP server management was added to Koha in bug 39190. This feature
should be made available to the marc order accounts feature so external
processes are not needed to downloading the marc order files.

Test Plan:
1) Run updatedatabase.pl
2) Restart all the things!
3) Create one or more file transports
4) Attach those transports to one or more MARC order accounts
5) Run marc_order_accounts.tt, note the files are transferred from the
   remove server to the "Download directory" of the MARC order account
6) Add another file to the remote server
7) Run script with "--rr dl"
8) Note the remote file has ".dl" append to the filename
9) Remove the .dl file extension from the remote file
10) Run script with "--dr"
11) Note the remote file is deleted

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 35 Martin Renvoize (ashimema) 2025-10-20 11:38:01 UTC Comment hidden (obsolete)
Comment 36 Martin Renvoize (ashimema) 2025-10-20 11:38:03 UTC
Created attachment 188157 [details] [review]
Bug 41020: (follow-up) Modernize code to use Koha objects and relationship accessors

This follow-up improves the code to better follow Koha best practices:

1. Added file_transport() relationship accessor to Koha::MarcOrderAccount
   - Returns the correct polymorphic subclass (SFTP, FTP, or Local)
   - Respects prefetched relationships to avoid N+1 queries
   - Follows the same pattern as vendor() and budget() accessors

2. Updated admin/marc_order_accounts.pl to use Koha::File::Transports
   - Replaced direct DBIx::Class schema access with Koha objects
   - Removed unused $schema variable

3. Updated misc/cronjobs/marc_ordering_process.pl
   - Changed manual transport lookup to use the new relationship accessor
   - Cleaner code that can benefit from prefetching

4. Added unit tests (t/db_dependent/Koha/MarcOrderAccount.t)
   - Tests for all relationship accessors (vendor, budget, file_transport)
   - Verifies polymorphism works correctly for all transport types
   - Includes test demonstrating that prefetched relationships don't
     trigger additional database queries

Test plan:
1. prove t/db_dependent/Koha/MarcOrderAccount.t
2. Verify all tests pass
3. Run marc_ordering_process.pl cronjob and verify it still works

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>