|
Description
Kyle M Hall (khall)
2025-10-14 19:01:23 UTC
Created attachment 187931 [details] [review] Bug 41020: Add ability to delete files to Koha::File::Transport and derived classes Created attachment 187932 [details] [review] Bug 41020: Add ability to delete files to Koha::File::Transport and derived classes Created attachment 187933 [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 Created attachment 187934 [details] [review] Bug 41020: Update schema files [DO NOT PUSH] Created attachment 187935 [details] [review] Bug 41020: Add ability to delete files to Koha::File::Transport and derived classes Created attachment 187936 [details] [review] Bug 41020: Add ability to delete files to Koha::File::Transport and derived classes Created attachment 187937 [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 Created attachment 187938 [details] [review] Bug 41020: Update schema files [DO NOT PUSH] Created attachment 187939 [details] [review] Bug 41020: Add ability to delete files to Koha::File::Transport and derived classes Created attachment 187940 [details] [review] Bug 41020: Add ability to delete files to Koha::File::Transport and derived classes Created attachment 187941 [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 Created attachment 187942 [details] [review] Bug 41020: Update schema files [DO NOT PUSH] 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? Created attachment 188012 [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: John Doe <you@example.com> Created attachment 188013 [details] [review] Bug 41020: Update FTP.pm _abort_operation prototype to match SFTP.pm Signed-off-by: John Doe <you@example.com> Created attachment 188014 [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: John Doe <you@example.com> Created attachment 188015 [details] [review] Bug 41020: Update schema files [DO NOT PUSH] Signed-off-by: John Doe <you@example.com> Created attachment 188016 [details] [review] Bug 41020: Update FTP.pm _abort_operation prototype to match SFTP.pm Signed-off-by: John Doe <you@example.com> Created attachment 188017 [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: John Doe <you@example.com> Created attachment 188018 [details] [review] Bug 41020: Update schema files [DO NOT PUSH] Signed-off-by: John Doe <you@example.com> Signed-off-by: John Doe <you@example.com> Created attachment 188030 [details] [review] Bug 41020: Add ability to delete files to Koha::File::Transport and derived classes Created attachment 188031 [details] [review] Bug 41020: Add ability to delete files to Koha::File::Transport and derived classes Created attachment 188032 [details] [review] Bug 41020: Update FTP.pm _abort_operation prototype to match SFTP.pm Signed-off-by: John Doe <you@example.com> Created attachment 188033 [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: John Doe <you@example.com> Created attachment 188034 [details] [review] Bug 41020: Update schema files [DO NOT PUSH] Signed-off-by: John Doe <you@example.com> Created attachment 188146 [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> Created attachment 188147 [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> Created attachment 188148 [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> Created attachment 188149 [details] [review] Bug 41020: Update schema files [DO NOT PUSH] Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 188150 [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> 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> 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> 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> Created attachment 188156 [details] [review] Bug 41020: Update schema files [DO NOT PUSH] Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> 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> |