Bug 38115 - Add FTP support to export_records.pl
Summary: Add FTP support to export_records.pl
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Jake Deery
QA Contact: Kyle M Hall (khall)
URL:
Keywords: release-notes-needed
Depends on: 38489
Blocks: 40811
  Show dependency treegraph
 
Reported: 2024-10-08 09:59 UTC by Martin Renvoize (ashimema)
Modified: 2025-11-03 21:17 UTC (History)
4 users (show)

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


Attachments
Bug 38115: Add destination_server_id to export_records (5.02 KB, patch)
2025-02-27 11:30 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 38115: Add destination_server_id to export_records (5.02 KB, patch)
2025-09-18 19:52 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38115: (follow-up) Update terminology from SFTP/FTP to file transport (4.19 KB, patch)
2025-09-18 19:52 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38115: Add destination_server_id to export_records (5.09 KB, patch)
2025-09-24 06:21 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38115: (follow-up) Update terminology from SFTP/FTP to file transport (4.26 KB, patch)
2025-09-24 06:22 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38115: (follow-up) Improve error handling and connection management (4.02 KB, patch)
2025-09-24 06:22 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38115: Add destination_server_id to export_records (5.09 KB, patch)
2025-10-17 14:45 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38115: (follow-up) Update terminology from SFTP/FTP to file transport (4.26 KB, patch)
2025-10-17 14:45 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38115: (follow-up) Improve error handling and connection management (4.02 KB, patch)
2025-10-17 14:45 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38115: (follow-up) Use File::Basename to extract filename for remote upload (1.57 KB, patch)
2025-10-17 14:45 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38115: (follow-up) Update terminology from SFTP/FTP to file transport (4.33 KB, patch)
2025-10-17 18:03 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38115: (follow-up) Update terminology from SFTP/FTP to file transport (4.33 KB, patch)
2025-10-17 18:03 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38115: (follow-up) Improve error handling and connection management (4.09 KB, patch)
2025-10-17 18:03 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38115: (follow-up) Use File::Basename to extract filename for remote upload (1.64 KB, patch)
2025-10-17 18:03 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38115: Add destination_server_id to export_records (5.16 KB, patch)
2025-10-17 18:04 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38115: (follow-up) Update terminology from SFTP/FTP to file transport (4.33 KB, patch)
2025-10-17 18:05 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38115: (follow-up) Improve error handling and connection management (4.09 KB, patch)
2025-10-17 18:05 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38115: (follow-up) Use File::Basename to extract filename for remote upload (1.64 KB, patch)
2025-10-17 18:05 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize (ashimema) 2024-10-08 09:59:14 UTC
With an administrative UI for configuring FTP connections coming but bug 35761 it would be nice to add support for using those connections in the misc/export_records.pl script.
Comment 1 Jake Deery 2025-02-27 11:30:07 UTC
Created attachment 178781 [details] [review]
Bug 38115: Add destination_server_id to export_records

This patch adds two new flags to the export_records.pl script.
* --destination_server_id=ID
    This flag will, when passed, upload the file generated by this script to
    the destination server, as configured in the FTP/SFTP Servers settings.
* --delete_local_after_run
    This flag will, when passed, cause the script to unlink (delete) the
    generated marc or CSV file found on the local disk. This is useful for folk
    who want to upload the file, but not necessarily keep a local copy, filling
    up the server's storage medium

TO TEST:

a)  APPLY PATCH, reset_all
b)  Create valid FTP/SFTP Server in Koha settings, ensuring that the server
    you create is valid and passes testing
c)  Run ./misc/export_records.pl with no agruments
    1)  Notice how koha.mrc file is generated
d)  Run ./misc/export_records.pl --destination_server_id=1
    1)  Notice how koha.mrc file is generated
    2)  On your FTP/SFTP Server, notice how koha.mrc is now present in the
        upload directory
e)  Run ./misc/export_records.pl --delete_local_after_run
    1)  Notice how koha.mrc file is NOT generated locally
f)  Run ./misc/export_records.pl --destination_server_id=1 --delete_local_after_run
    1)  Notice how koha.mrc file is NOT generated locally
    2)  On your FTP/SFTP Server, notice how koha.mrc is still present in the
        upload directory AND the file age is fresh
Comment 2 Tomás Cohen Arazi (tcohen) 2025-09-15 15:08:26 UTC
After the changes in bug 39190, terminology should be revisited. And so variable names, etc.
Comment 3 Tomás Cohen Arazi (tcohen) 2025-09-18 19:52:45 UTC
Created attachment 186589 [details] [review]
Bug 38115: Add destination_server_id to export_records

This patch adds two new flags to the export_records.pl script.
* --destination_server_id=ID
    This flag will, when passed, upload the file generated by this script to
    the destination server, as configured in the FTP/SFTP Servers settings.
* --delete_local_after_run
    This flag will, when passed, cause the script to unlink (delete) the
    generated marc or CSV file found on the local disk. This is useful for folk
    who want to upload the file, but not necessarily keep a local copy, filling
    up the server's storage medium

TO TEST:

a)  APPLY PATCH, reset_all
b)  Create valid FTP/SFTP Server in Koha settings, ensuring that the server
    you create is valid and passes testing
c)  Run ./misc/export_records.pl with no agruments
    1)  Notice how koha.mrc file is generated
d)  Run ./misc/export_records.pl --destination_server_id=1
    1)  Notice how koha.mrc file is generated
    2)  On your FTP/SFTP Server, notice how koha.mrc is now present in the
        upload directory
e)  Run ./misc/export_records.pl --delete_local_after_run
    1)  Notice how koha.mrc file is NOT generated locally
f)  Run ./misc/export_records.pl --destination_server_id=1 --delete_local_after_run
    1)  Notice how koha.mrc file is NOT generated locally
    2)  On your FTP/SFTP Server, notice how koha.mrc is still present in the
        upload directory AND the file age is fresh
Comment 4 Tomás Cohen Arazi (tcohen) 2025-09-18 19:52:50 UTC
Created attachment 186590 [details] [review]
Bug 38115: (follow-up) Update terminology from SFTP/FTP to file transport

This follow-up patch updates the terminology in export_records.pl to use
generic 'file transport' language instead of specific 'FTP/SFTP' references,
aligning with the recent renaming of the underlying classes to File::Transports.

Changes:
- Replace $sftp_server variable with $file_transport for clarity
- Update error messages to use 'file transport server' terminology
- Fix incorrect error message for report_id validation
- Update POD documentation to reference 'file transport servers'
- Maintain consistency with the renamed admin interface and API

This ensures the script terminology matches the current class structure
and admin interface naming conventions.
Comment 5 Martin Renvoize (ashimema) 2025-09-24 06:21:59 UTC
Created attachment 186855 [details] [review]
Bug 38115: Add destination_server_id to export_records

This patch adds two new flags to the export_records.pl script.
* --destination_server_id=ID
    This flag will, when passed, upload the file generated by this script to
    the destination server, as configured in the FTP/SFTP Servers settings.
* --delete_local_after_run
    This flag will, when passed, cause the script to unlink (delete) the
    generated marc or CSV file found on the local disk. This is useful for folk
    who want to upload the file, but not necessarily keep a local copy, filling
    up the server's storage medium

TO TEST:

a)  APPLY PATCH, reset_all
b)  Create valid FTP/SFTP Server in Koha settings, ensuring that the server
    you create is valid and passes testing
c)  Run ./misc/export_records.pl with no agruments
    1)  Notice how koha.mrc file is generated
d)  Run ./misc/export_records.pl --destination_server_id=1
    1)  Notice how koha.mrc file is generated
    2)  On your FTP/SFTP Server, notice how koha.mrc is now present in the
        upload directory
e)  Run ./misc/export_records.pl --delete_local_after_run
    1)  Notice how koha.mrc file is NOT generated locally
f)  Run ./misc/export_records.pl --destination_server_id=1 --delete_local_after_run
    1)  Notice how koha.mrc file is NOT generated locally
    2)  On your FTP/SFTP Server, notice how koha.mrc is still present in the
        upload directory AND the file age is fresh

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 6 Martin Renvoize (ashimema) 2025-09-24 06:22:05 UTC
Created attachment 186856 [details] [review]
Bug 38115: (follow-up) Update terminology from SFTP/FTP to file transport

This follow-up patch updates the terminology in export_records.pl to use
generic 'file transport' language instead of specific 'FTP/SFTP' references,
aligning with the recent renaming of the underlying classes to File::Transports.

Changes:
- Replace $sftp_server variable with $file_transport for clarity
- Update error messages to use 'file transport server' terminology
- Fix incorrect error message for report_id validation
- Update POD documentation to reference 'file transport servers'
- Maintain consistency with the renamed admin interface and API

This ensures the script terminology matches the current class structure
and admin interface naming conventions.

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 7 Martin Renvoize (ashimema) 2025-09-24 06:22:10 UTC
Created attachment 186857 [details] [review]
Bug 38115: (follow-up) Improve error handling and connection management

Enhance the export_records.pl script with better error handling
and connection lifecycle management:

1. Connection Management:
   - Always call disconnect() after file transport operations
   - Ensure connections are cleaned up even on errors

2. Error Handling:
   - Replace unusual die+pod2usage with clear error messages
   - Add file existence validation before upload attempts
   - Provide detailed error messages with context
   - Consistent error message formatting

3. User Experience:
   - Add validation for --delete_local_after_run without --destination_server_id
   - Provide informative success messages to STDERR
   - Better error messages with specific failure reasons

4. Robustness:
   - Check if output file was created before upload
   - Check if file exists before attempting deletion
   - Graceful handling of missing upload directories

These improvements make the script more reliable and user-friendly
while maintaining all existing functionality.
Comment 8 Kyle M Hall (khall) 2025-10-16 19:42:04 UTC
This looks pretty good, but what if --filename is a path? Let's say it's /tmp/records.mrc. Let's say the upload dir for the transport is 'uploads'. If I read the code correctly, it's going to upload the file to uploads/tmp/records.mrc.

We could use File::Basename to get just the filename from the --filename option if it does indeed include a path.
Comment 9 Martin Renvoize (ashimema) 2025-10-17 14:45:07 UTC
Created attachment 188090 [details] [review]
Bug 38115: Add destination_server_id to export_records

This patch adds two new flags to the export_records.pl script.
* --destination_server_id=ID
    This flag will, when passed, upload the file generated by this script to
    the destination server, as configured in the FTP/SFTP Servers settings.
* --delete_local_after_run
    This flag will, when passed, cause the script to unlink (delete) the
    generated marc or CSV file found on the local disk. This is useful for folk
    who want to upload the file, but not necessarily keep a local copy, filling
    up the server's storage medium

TO TEST:

a)  APPLY PATCH, reset_all
b)  Create valid FTP/SFTP Server in Koha settings, ensuring that the server
    you create is valid and passes testing
c)  Run ./misc/export_records.pl with no agruments
    1)  Notice how koha.mrc file is generated
d)  Run ./misc/export_records.pl --destination_server_id=1
    1)  Notice how koha.mrc file is generated
    2)  On your FTP/SFTP Server, notice how koha.mrc is now present in the
        upload directory
e)  Run ./misc/export_records.pl --delete_local_after_run
    1)  Notice how koha.mrc file is NOT generated locally
f)  Run ./misc/export_records.pl --destination_server_id=1 --delete_local_after_run
    1)  Notice how koha.mrc file is NOT generated locally
    2)  On your FTP/SFTP Server, notice how koha.mrc is still present in the
        upload directory AND the file age is fresh

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 10 Martin Renvoize (ashimema) 2025-10-17 14:45:09 UTC
Created attachment 188091 [details] [review]
Bug 38115: (follow-up) Update terminology from SFTP/FTP to file transport

This follow-up patch updates the terminology in export_records.pl to use
generic 'file transport' language instead of specific 'FTP/SFTP' references,
aligning with the recent renaming of the underlying classes to File::Transports.

Changes:
- Replace $sftp_server variable with $file_transport for clarity
- Update error messages to use 'file transport server' terminology
- Fix incorrect error message for report_id validation
- Update POD documentation to reference 'file transport servers'
- Maintain consistency with the renamed admin interface and API

This ensures the script terminology matches the current class structure
and admin interface naming conventions.

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 11 Martin Renvoize (ashimema) 2025-10-17 14:45:11 UTC
Created attachment 188092 [details] [review]
Bug 38115: (follow-up) Improve error handling and connection management

Enhance the export_records.pl script with better error handling
and connection lifecycle management:

1. Connection Management:
   - Always call disconnect() after file transport operations
   - Ensure connections are cleaned up even on errors

2. Error Handling:
   - Replace unusual die+pod2usage with clear error messages
   - Add file existence validation before upload attempts
   - Provide detailed error messages with context
   - Consistent error message formatting

3. User Experience:
   - Add validation for --delete_local_after_run without --destination_server_id
   - Provide informative success messages to STDERR
   - Better error messages with specific failure reasons

4. Robustness:
   - Check if output file was created before upload
   - Check if file exists before attempting deletion
   - Graceful handling of missing upload directories

These improvements make the script more reliable and user-friendly
while maintaining all existing functionality.
Comment 12 Martin Renvoize (ashimema) 2025-10-17 14:45:13 UTC
Created attachment 188093 [details] [review]
Bug 38115: (follow-up) Use File::Basename to extract filename for remote upload

When --filename contains a path (e.g., /tmp/records.mrc), the script
was uploading to the remote server with the full path, creating
unwanted directory structures (e.g., uploads/tmp/records.mrc).

This patch uses File::Basename::fileparse() to extract just the
filename portion for the remote upload, while still using the full
path for local file operations.
Comment 13 Martin Renvoize (ashimema) 2025-10-17 14:45:30 UTC
Back at you Kyle, nice spot :)
Comment 14 Kyle M Hall (khall) 2025-10-17 18:03:27 UTC
Created attachment 188115 [details] [review]
Bug 38115: (follow-up) Update terminology from SFTP/FTP to file transport

This follow-up patch updates the terminology in export_records.pl to use
generic 'file transport' language instead of specific 'FTP/SFTP' references,
aligning with the recent renaming of the underlying classes to File::Transports.

Changes:
- Replace $sftp_server variable with $file_transport for clarity
- Update error messages to use 'file transport server' terminology
- Fix incorrect error message for report_id validation
- Update POD documentation to reference 'file transport servers'
- Maintain consistency with the renamed admin interface and API

This ensures the script terminology matches the current class structure
and admin interface naming conventions.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall (khall) 2025-10-17 18:03:44 UTC
Created attachment 188116 [details] [review]
Bug 38115: (follow-up) Update terminology from SFTP/FTP to file transport

This follow-up patch updates the terminology in export_records.pl to use
generic 'file transport' language instead of specific 'FTP/SFTP' references,
aligning with the recent renaming of the underlying classes to File::Transports.

Changes:
- Replace $sftp_server variable with $file_transport for clarity
- Update error messages to use 'file transport server' terminology
- Fix incorrect error message for report_id validation
- Update POD documentation to reference 'file transport servers'
- Maintain consistency with the renamed admin interface and API

This ensures the script terminology matches the current class structure
and admin interface naming conventions.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall (khall) 2025-10-17 18:03:45 UTC
Created attachment 188117 [details] [review]
Bug 38115: (follow-up) Improve error handling and connection management

Enhance the export_records.pl script with better error handling
and connection lifecycle management:

1. Connection Management:
   - Always call disconnect() after file transport operations
   - Ensure connections are cleaned up even on errors

2. Error Handling:
   - Replace unusual die+pod2usage with clear error messages
   - Add file existence validation before upload attempts
   - Provide detailed error messages with context
   - Consistent error message formatting

3. User Experience:
   - Add validation for --delete_local_after_run without --destination_server_id
   - Provide informative success messages to STDERR
   - Better error messages with specific failure reasons

4. Robustness:
   - Check if output file was created before upload
   - Check if file exists before attempting deletion
   - Graceful handling of missing upload directories

These improvements make the script more reliable and user-friendly
while maintaining all existing functionality.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall (khall) 2025-10-17 18:03:47 UTC
Created attachment 188118 [details] [review]
Bug 38115: (follow-up) Use File::Basename to extract filename for remote upload

When --filename contains a path (e.g., /tmp/records.mrc), the script
was uploading to the remote server with the full path, creating
unwanted directory structures (e.g., uploads/tmp/records.mrc).

This patch uses File::Basename::fileparse() to extract just the
filename portion for the remote upload, while still using the full
path for local file operations.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall (khall) 2025-10-17 18:04:56 UTC
Created attachment 188119 [details] [review]
Bug 38115: Add destination_server_id to export_records

This patch adds two new flags to the export_records.pl script.
* --destination_server_id=ID
    This flag will, when passed, upload the file generated by this script to
    the destination server, as configured in the FTP/SFTP Servers settings.
* --delete_local_after_run
    This flag will, when passed, cause the script to unlink (delete) the
    generated marc or CSV file found on the local disk. This is useful for folk
    who want to upload the file, but not necessarily keep a local copy, filling
    up the server's storage medium

TO TEST:

a)  APPLY PATCH, reset_all
b)  Create valid FTP/SFTP Server in Koha settings, ensuring that the server
    you create is valid and passes testing
c)  Run ./misc/export_records.pl with no agruments
    1)  Notice how koha.mrc file is generated
d)  Run ./misc/export_records.pl --destination_server_id=1
    1)  Notice how koha.mrc file is generated
    2)  On your FTP/SFTP Server, notice how koha.mrc is now present in the
        upload directory
e)  Run ./misc/export_records.pl --delete_local_after_run
    1)  Notice how koha.mrc file is NOT generated locally
f)  Run ./misc/export_records.pl --destination_server_id=1 --delete_local_after_run
    1)  Notice how koha.mrc file is NOT generated locally
    2)  On your FTP/SFTP Server, notice how koha.mrc is still present in the
        upload directory AND the file age is fresh

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall (khall) 2025-10-17 18:05:05 UTC
Created attachment 188120 [details] [review]
Bug 38115: (follow-up) Update terminology from SFTP/FTP to file transport

This follow-up patch updates the terminology in export_records.pl to use
generic 'file transport' language instead of specific 'FTP/SFTP' references,
aligning with the recent renaming of the underlying classes to File::Transports.

Changes:
- Replace $sftp_server variable with $file_transport for clarity
- Update error messages to use 'file transport server' terminology
- Fix incorrect error message for report_id validation
- Update POD documentation to reference 'file transport servers'
- Maintain consistency with the renamed admin interface and API

This ensures the script terminology matches the current class structure
and admin interface naming conventions.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Kyle M Hall (khall) 2025-10-17 18:05:08 UTC
Created attachment 188121 [details] [review]
Bug 38115: (follow-up) Improve error handling and connection management

Enhance the export_records.pl script with better error handling
and connection lifecycle management:

1. Connection Management:
   - Always call disconnect() after file transport operations
   - Ensure connections are cleaned up even on errors

2. Error Handling:
   - Replace unusual die+pod2usage with clear error messages
   - Add file existence validation before upload attempts
   - Provide detailed error messages with context
   - Consistent error message formatting

3. User Experience:
   - Add validation for --delete_local_after_run without --destination_server_id
   - Provide informative success messages to STDERR
   - Better error messages with specific failure reasons

4. Robustness:
   - Check if output file was created before upload
   - Check if file exists before attempting deletion
   - Graceful handling of missing upload directories

These improvements make the script more reliable and user-friendly
while maintaining all existing functionality.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall (khall) 2025-10-17 18:05:11 UTC
Created attachment 188122 [details] [review]
Bug 38115: (follow-up) Use File::Basename to extract filename for remote upload

When --filename contains a path (e.g., /tmp/records.mrc), the script
was uploading to the remote server with the full path, creating
unwanted directory structures (e.g., uploads/tmp/records.mrc).

This patch uses File::Basename::fileparse() to extract just the
filename portion for the remote upload, while still using the full
path for local file operations.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Lucas Gass (lukeg) 2025-11-03 21:15:52 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 23 Lucas Gass (lukeg) 2025-11-03 21:17:36 UTC
Please add release notes