Description
Jake Deery
2024-11-20 09:23:09 UTC
Created attachment 175204 [details] [review] Bug 35761: Integrate EDI into new FTP/SFTP management UI This patch integrates the EDI Transport.pm module into the new FTP/SFTP management UI by tweaking the existing methods to look at the new SftpServer object, instead of its own. Included also is a change to the database structure to make use of the new sftp_servers table, as well as an atomic update to copy the schema changes, and existing config to the new sftp_servers table. NOTE: To test this bug, you'll need to have access to a valid FTP/SFTP server, within which you are able to generate exemplar quotes, orders, and invoices. If you do not have access to this, contact me, and I will try to arrange something. Thanks To test: a) Apply Bug 35761, follow that test plan, and enable EDIFACT syspref b) Notice how there is a duplication of FTP/SFTP details in Koha, between EDI and FTP/SFTP Servers c) APPLY THIS BUG, reset_all, and enable EDIFACT syspref d) Create an FTP/SFTP Server under Koha Administration -> FTP/SFTP Server e) Create an EDI account 1) Notice how there is now two dropdowns for upload & download server 2) Choose the same server for both options 3) Set valid download & upload directories 4) Enable quotes, orders, and invoices f) Run ./misc/cronjobs/edi_cron.pl 1) Check that a quote is now in Koha from the EDI vendor 2) Confirm the quote via the staff client g) Run ./misc/cronjobs/edi_cron.pl 1) Check that the order is now on the FTP/SFTP server 2) Generate an invoice on the FTP/SFTP server h) Run ./misc/cronjobs/edi_cron.pl 1) Confirm Koha has received the invoice 2) Mark the invoice as paid in the staff client i) Run ./misc/cronjobs/edi_cron.pl 1) Check that the invoice is now on the FTP/SFTP server j) SIGN OFF! Created attachment 175205 [details] [review] Bug 38489: (follow-up) tidy EDIfact files This patch follows-up by by tidying the following files: * Koha/Edifact/Transport.pm * admin/edi_accounts.pl Created attachment 175206 [details] [review] Bug 38489: Integrate EDI into new FTP/SFTP management UI This patch integrates the EDI Transport.pm module into the new FTP/SFTP management UI by tweaking the existing methods to look at the new SftpServer object, instead of its own. Included also is a change to the database structure to make use of the new sftp_servers table, as well as an atomic update to copy the schema changes, and existing config to the new sftp_servers table. NOTE: To test this bug, you'll need to have access to a valid FTP/SFTP server, within which you are able to generate exemplar quotes, orders, and invoices. If you do not have access to this, contact me, and I will try to arrange something. Thanks To test: a) Apply Bug 35761, follow that test plan, and enable EDIFACT syspref b) Notice how there is a duplication of FTP/SFTP details in Koha, between EDI and FTP/SFTP Servers c) APPLY THIS BUG, reset_all, and enable EDIFACT syspref d) Create an FTP/SFTP Server under Koha Administration -> FTP/SFTP Server e) Create an EDI account 1) Notice how there is now two dropdowns for upload & download server 2) Choose the same server for both options 3) Set valid download & upload directories 4) Enable quotes, orders, and invoices f) Run ./misc/cronjobs/edi_cron.pl 1) Check that a quote is now in Koha from the EDI vendor 2) Confirm the quote via the staff client g) Run ./misc/cronjobs/edi_cron.pl 1) Check that the order is now on the FTP/SFTP server 2) Generate an invoice on the FTP/SFTP server h) Run ./misc/cronjobs/edi_cron.pl 1) Confirm Koha has received the invoice 2) Mark the invoice as paid in the staff client i) Run ./misc/cronjobs/edi_cron.pl 1) Check that the invoice is now on the FTP/SFTP server j) SIGN OFF! Created attachment 175207 [details] [review] Bug 38489: (follow-up) tidy EDIfact files This patch follows-up by by tidying the following files: * Koha/Edifact/Transport.pm * admin/edi_accounts.pl Created attachment 176118 [details] [review] Bug 38489: Integrate EDI into new FTP/SFTP management UI This patch integrates the EDI Transport.pm module into the new FTP/SFTP management UI by tweaking the existing methods to look at the new SftpServer object, instead of its own. Included also is a change to the database structure to make use of the new sftp_servers table, as well as an atomic update to copy the schema changes, and existing config to the new sftp_servers table. NOTE: To test this bug, you'll need to have access to a valid FTP/SFTP server, within which you are able to generate exemplar quotes, orders, and invoices. If you do not have access to this, contact me, and I will try to arrange something. Thanks To test: a) Apply Bug 35761, follow that test plan, and enable EDIFACT syspref b) Notice how there is a duplication of FTP/SFTP details in Koha, between EDI and FTP/SFTP Servers c) APPLY THIS BUG, reset_all, and enable EDIFACT syspref d) Create an FTP/SFTP Server under Koha Administration -> FTP/SFTP Server e) Create an EDI account 1) Notice how there is now two dropdowns for upload & download server 2) Choose the same server for both options 3) Set valid download & upload directories 4) Enable quotes, orders, and invoices f) Run ./misc/cronjobs/edi_cron.pl 1) Check that a quote is now in Koha from the EDI vendor 2) Confirm the quote via the staff client g) Run ./misc/cronjobs/edi_cron.pl 1) Check that the order is now on the FTP/SFTP server 2) Generate an invoice on the FTP/SFTP server h) Run ./misc/cronjobs/edi_cron.pl 1) Confirm Koha has received the invoice 2) Mark the invoice as paid in the staff client i) Run ./misc/cronjobs/edi_cron.pl 1) Check that the invoice is now on the FTP/SFTP server j) SIGN OFF! Created attachment 176119 [details] [review] Bug 38489: (follow-up) tidy EDIfact files This patch follows-up by by tidying the following files: * Koha/Edifact/Transport.pm * admin/edi_accounts.pl Created attachment 176161 [details] [review] Bug 38489: (follow-up) tidy EDIfact files This patch follows-up by by tidying the following files: * Koha/Edifact/Transport.pm * admin/edi_accounts.pl Please rebase for signoff. Thanks! Created attachment 185324 [details] [review] Bug 38489: Integrate EDI into new FTP/SFTP management UI This patch integrates the EDI Transport.pm module into the new FTP/SFTP management UI by tweaking the existing methods to look at the new SftpServer object, instead of its own. Included also is a change to the database structure to make use of the new sftp_servers table, as well as an atomic update to copy the schema changes, and existing config to the new sftp_servers table. NOTE: To test this bug, you'll need to have access to a valid FTP/SFTP server, within which you are able to generate exemplar quotes, orders, and invoices. If you do not have access to this, contact me, and I will try to arrange something. Thanks To test: a) Apply Bug 35761, follow that test plan, and enable EDIFACT syspref b) Notice how there is a duplication of FTP/SFTP details in Koha, between EDI and FTP/SFTP Servers c) APPLY THIS BUG, reset_all, and enable EDIFACT syspref d) Create an FTP/SFTP Server under Koha Administration -> FTP/SFTP Server e) Create an EDI account 1) Notice how there is now two dropdowns for upload & download server 2) Choose the same server for both options 3) Set valid download & upload directories 4) Enable quotes, orders, and invoices f) Run ./misc/cronjobs/edi_cron.pl 1) Check that a quote is now in Koha from the EDI vendor 2) Confirm the quote via the staff client g) Run ./misc/cronjobs/edi_cron.pl 1) Check that the order is now on the FTP/SFTP server 2) Generate an invoice on the FTP/SFTP server h) Run ./misc/cronjobs/edi_cron.pl 1) Confirm Koha has received the invoice 2) Mark the invoice as paid in the staff client i) Run ./misc/cronjobs/edi_cron.pl 1) Check that the invoice is now on the FTP/SFTP server j) SIGN OFF! Just obsoleted the tidy patch, and attached a new work. Hopefully this should still all behave itself (I had a short test run, worksforme, though some additional thoroughness from someone else in Community would be appreciated). Ta, Jake. Created attachment 186819 [details] [review] Bug 38489: Integrate EDI into new FTP/SFTP management UI This patch integrates the EDI Transport.pm module into the new FTP/SFTP management UI by tweaking the existing methods to look at the new SftpServer object, instead of its own. Included also is a change to the database structure to make use of the new sftp_servers table, as well as an atomic update to copy the schema changes, and existing config to the new sftp_servers table. NOTE: To test this bug, you'll need to have access to a valid FTP/SFTP server, within which you are able to generate exemplar quotes, orders, and invoices. If you do not have access to this, contact me, and I will try to arrange something. Thanks To test: a) Apply Bug 35761, follow that test plan, and enable EDIFACT syspref b) Notice how there is a duplication of FTP/SFTP details in Koha, between EDI and FTP/SFTP Servers c) APPLY THIS BUG, reset_all, and enable EDIFACT syspref d) Create an FTP/SFTP Server under Koha Administration -> FTP/SFTP Server e) Create an EDI account 1) Notice how there is now two dropdowns for upload & download server 2) Choose the same server for both options 3) Set valid download & upload directories 4) Enable quotes, orders, and invoices f) Run ./misc/cronjobs/edi_cron.pl 1) Check that a quote is now in Koha from the EDI vendor 2) Confirm the quote via the staff client g) Run ./misc/cronjobs/edi_cron.pl 1) Check that the order is now on the FTP/SFTP server 2) Generate an invoice on the FTP/SFTP server h) Run ./misc/cronjobs/edi_cron.pl 1) Confirm Koha has received the invoice 2) Mark the invoice as paid in the staff client i) Run ./misc/cronjobs/edi_cron.pl 1) Check that the invoice is now on the FTP/SFTP server j) SIGN OFF! Created attachment 186820 [details] [review] DBIC - DO NOT PUSH Another rebase..! Also, split out the DBIC code for easier pushing to main. Ta, Jake. Created attachment 186845 [details] [review] Bug 38489: Add file_transport_id to vendor_edi_accounts and migrate to new transport system This patch adds database migration to move EDI transport configurations from vendor_edi_accounts to the new file_transports system introduced in Bug 39190. Changes: - Add file_transport_id column to vendor_edi_accounts with foreign key - Add 'local' transport type to file_transports enum - Remove old transport fields from vendor_edi_accounts in kohastructure - Migration script converts existing EDI configurations to file transports - Handles FILE -> local transport type conversion Created attachment 186846 [details] [review] Bug 38489: Update EDI accounts UI to use file transport selection This patch updates the EDI accounts administration interface to use the new file_transports system instead of direct transport configuration. Changes: - Replace transport configuration fields with transport selection dropdown - Add file_transports data to template for selection options - Remove password decryption since credentials are now managed by transports - Simplify form handling to only manage EDI-specific fields - Add helpful link to file transport administration Created attachment 186847 [details] [review] Bug 38489: Add rename_file method to transport system and Local transport class This patch enhances the file transport system with file renaming capabilities and adds a Local transport class for development/testing. Changes: - Add rename_file() method to base Transport class and SFTP/FTP subclasses - Create Koha::File::Transport::Local for local directory operations - Add 'local' transport type to polymorphic mapping - Update transport instantiation to handle Local transport - Add Local transport tests to verify polymorphic behavior - Enhanced connect() method for Local transport validates directories The rename_file functionality is needed for EDI file processing workflow where files are renamed after download to mark them as processed. Created attachment 186848 [details] [review] Bug 38489: Add disconnect method and proper connection cleanup to file transport system This patch adds proper connection cleanup functionality to the file transport system, addressing a gap identified during EDI migration. Changes: - Add disconnect() method to base Transport class interface - Implement SFTP disconnect using connection->disconnect() - Implement FTP disconnect using connection->quit() - Add DESTROY methods to ensure connections are cleaned up on object destruction - Local transport includes no-op disconnect for API consistency This ensures that FTP/SFTP connections are properly closed, preventing connection leaks and potential resource exhaustion issues. Created attachment 186849 [details] [review] Bug 38489: Standardize list_files API across all transport classes Fix API inconsistency where different transport types returned different formats from list_files(): - SFTP: returned array of hashrefs with detailed file information - FTP: returned simple array of filenames - Local: returned simple array of filenames Now all transports return an array reference of hashrefs with consistent structure containing filename, longname, and additional metadata. This eliminates the need for workarounds like the ref($file) eq 'HASH' check in EDI code and provides a more robust, consistent API. Created attachment 186850 [details] [review] Bug 38489: Update Koha::Edifact::Transport to use new file transport system This patch completely refactors the EDI transport handling to use the modern file_transports system instead of legacy direct connections. Changes: - Remove legacy FTP/SFTP connection code - Update constructor to load file_transport from vendor_edi_accounts - Update download_messages/upload_messages to use modern transport methods - Use new rename_file() method for EDI file processing workflow - Remove dependencies on Net::FTP and Net::SFTP::Foreign - Maintain all EDI-specific functionality (file filtering, ingestion) Cleanup: - Remove obsolete methods: file_download, sftp_download, ftp_download, sftp_upload, ftp_upload, file_upload, _abort_download - Update POD documentation to reflect new architecture - 184 lines of legacy code removed The transport system now provides: - Unified connection handling across all transport types - Enhanced error reporting and status tracking - Encrypted credential storage - Connection testing capabilities - Support for local directories (testing/development) Created attachment 186851 [details] [review] Bug 38489: Schema Update Created attachment 186852 [details] [review] Bug 38489: Update EDI tests to use new file transport system The EDI test was still creating vendor_edi_accounts with transport='FILE' which no longer works after the migration to the file_transports system. Updated all test account creation to: - Create file_transports records with transport='local' - Link EDI accounts via file_transport_id instead of direct transport field - Set appropriate download/upload directories for local testing All EDI tests now pass with the new transport architecture. Sorry Jake, I started to review this knowing that a fair chunk had changed under the hood since you last looked. I'm afraid it highlighted a few oversights in your rebase, but more importantly, a few oversights in the work I did reworking the Transport classes behind the scenes.. As such, I've knicked the bug and added a set of patches that I believe now do the right thing. You're test plan is still good for the new patchset however: This patch integrates the EDI Transport.pm module into the new FTP/SFTP management UI by tweaking the existing methods to look at the new SftpServer object, instead of its own. Included also is a change to the database structure to make use of the new sftp_servers table, as well as an atomic update to copy the schema changes, and existing config to the new sftp_servers table. NOTE: To test this bug, you'll need to have access to a valid FTP/SFTP server, within which you are able to generate exemplar quotes, orders, and invoices. If you do not have access to this, contact me, and I will try to arrange something. Thanks To test: a) Apply Bug 35761, follow that test plan, and enable EDIFACT syspref b) Notice how there is a duplication of FTP/SFTP details in Koha, between EDI and FTP/SFTP Servers c) APPLY THIS BUG, reset_all, and enable EDIFACT syspref d) Create an FTP/SFTP Server under Koha Administration -> FTP/SFTP Server e) Create an EDI account 1) Notice how there is now two dropdowns for upload & download server 2) Choose the same server for both options 3) Set valid download & upload directories 4) Enable quotes, orders, and invoices f) Run ./misc/cronjobs/edi_cron.pl 1) Check that a quote is now in Koha from the EDI vendor 2) Confirm the quote via the staff client g) Run ./misc/cronjobs/edi_cron.pl 1) Check that the order is now on the FTP/SFTP server 2) Generate an invoice on the FTP/SFTP server h) Run ./misc/cronjobs/edi_cron.pl 1) Confirm Koha has received the invoice 2) Mark the invoice as paid in the staff client i) Run ./misc/cronjobs/edi_cron.pl 1) Check that the invoice is now on the FTP/SFTP server j) SIGN OFF! Created attachment 186854 [details] [review] Bug 38489: Standardize FTP transport API to match SFTP and Local classes Fix several API inconsistencies in the FTP transport class: 1. Message format: Use standardized `message => $operation` format instead of hardcoded strings like "Upload succeeded" 2. Operation tracking: Add $operation variable to all methods for consistent error reporting and logging 3. Error handling: Update _abort_operation() to accept operation parameter and provide more detailed error payloads 4. Success payloads: Replace empty `detail => ''` payloads with meaningful operation-specific information (host, port, file paths, directory info, file counts) All transport classes now have consistent APIs for: - Standardized message formats - Consistent operation tracking - Uniform error handling - Informative success payloads Error when editing a saved file transport: An error occurred trying to open the server for editing. The passed ID is invalid. Created attachment 187080 [details] [review] Bug 38489: Follow-up to bug 39190 to fix administration We did some last minute renaming of things in bug 39190 but didn't catch all the places. This patch updates the old 'sftp_servers' naming scheme to 'file_transports' following the new pattern and restores edit and delete functionality in the File transports management area. Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187081 [details] [review] Bug 38489: Add file_transport_id to vendor_edi_accounts and migrate to new transport system This patch adds database migration to move EDI transport configurations from vendor_edi_accounts to the new file_transports system introduced in Bug 39190. Changes: - Add file_transport_id column to vendor_edi_accounts with foreign key - Add 'local' transport type to file_transports enum - Remove old transport fields from vendor_edi_accounts in kohastructure - Migration script converts existing EDI configurations to file transports - Handles FILE -> local transport type conversion Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187082 [details] [review] Bug 38489: Update EDI accounts UI to use file transport selection This patch updates the EDI accounts administration interface to use the new file_transports system instead of direct transport configuration. Changes: - Replace transport configuration fields with transport selection dropdown - Add file_transports data to template for selection options - Remove password decryption since credentials are now managed by transports - Simplify form handling to only manage EDI-specific fields - Add helpful link to file transport administration Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187083 [details] [review] Bug 38489: Add rename_file method to transport system and Local transport class This patch enhances the file transport system with file renaming capabilities and adds a Local transport class for development/testing. Changes: - Add rename_file() method to base Transport class and SFTP/FTP subclasses - Create Koha::File::Transport::Local for local directory operations - Add 'local' transport type to polymorphic mapping - Update transport instantiation to handle Local transport - Add Local transport tests to verify polymorphic behavior - Enhanced connect() method for Local transport validates directories The rename_file functionality is needed for EDI file processing workflow where files are renamed after download to mark them as processed. Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187084 [details] [review] Bug 38489: Add disconnect method and proper connection cleanup to file transport system This patch adds proper connection cleanup functionality to the file transport system, addressing a gap identified during EDI migration. Changes: - Add disconnect() method to base Transport class interface - Implement SFTP disconnect using connection->disconnect() - Implement FTP disconnect using connection->quit() - Add DESTROY methods to ensure connections are cleaned up on object destruction - Local transport includes no-op disconnect for API consistency This ensures that FTP/SFTP connections are properly closed, preventing connection leaks and potential resource exhaustion issues. Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187085 [details] [review] Bug 38489: Standardize list_files API across all transport classes Fix API inconsistency where different transport types returned different formats from list_files(): - SFTP: returned array of hashrefs with detailed file information - FTP: returned simple array of filenames - Local: returned simple array of filenames Now all transports return an array reference of hashrefs with consistent structure containing filename, longname, and additional metadata. This eliminates the need for workarounds like the ref($file) eq 'HASH' check in EDI code and provides a more robust, consistent API. Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187086 [details] [review] Bug 38489: Update Koha::Edifact::Transport to use new file transport system This patch completely refactors the EDI transport handling to use the modern file_transports system instead of legacy direct connections. Changes: - Remove legacy FTP/SFTP connection code - Update constructor to load file_transport from vendor_edi_accounts - Update download_messages/upload_messages to use modern transport methods - Use new rename_file() method for EDI file processing workflow - Remove dependencies on Net::FTP and Net::SFTP::Foreign - Maintain all EDI-specific functionality (file filtering, ingestion) Cleanup: - Remove obsolete methods: file_download, sftp_download, ftp_download, sftp_upload, ftp_upload, file_upload, _abort_download - Update POD documentation to reflect new architecture - 184 lines of legacy code removed The transport system now provides: - Unified connection handling across all transport types - Enhanced error reporting and status tracking - Encrypted credential storage - Connection testing capabilities - Support for local directories (testing/development) Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187087 [details] [review] Bug 38489: Schema Update Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187088 [details] [review] Bug 38489: Update EDI tests to use new file transport system The EDI test was still creating vendor_edi_accounts with transport='FILE' which no longer works after the migration to the file_transports system. Updated all test account creation to: - Create file_transports records with transport='local' - Link EDI accounts via file_transport_id instead of direct transport field - Set appropriate download/upload directories for local testing All EDI tests now pass with the new transport architecture. Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187089 [details] [review] Bug 38489: Standardize FTP transport API to match SFTP and Local classes Fix several API inconsistencies in the FTP transport class: 1. Message format: Use standardized `message => $operation` format instead of hardcoded strings like "Upload succeeded" 2. Operation tracking: Add $operation variable to all methods for consistent error reporting and logging 3. Error handling: Update _abort_operation() to accept operation parameter and provide more detailed error payloads 4. Success payloads: Replace empty `detail => ''` payloads with meaningful operation-specific information (host, port, file paths, directory info, file counts) All transport classes now have consistent APIs for: - Standardized message formats - Consistent operation tracking - Uniform error handling - Informative success payloads Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> The edi accounts table still has columns for the s/ftp data. Those should be removed! Created attachment 187108 [details] [review] Bug 38489: (follow-up) Use 'TableExists' in atomicupdate Comment on attachment 187081 [details] [review] Bug 38489: Add file_transport_id to vendor_edi_accounts and migrate to new transport system Review of attachment 187081 [details] [review]: ----------------------------------------------------------------- Sorry Kyle, which fields do you mean here? My mistake.. you mean the display table on the edi accounts management page Created attachment 187147 [details] [review] Bug 38489: (follow-up) Update EDI accounts list display This patch updates the EDI accounts management page to display the new file transport system instead of the old inline transport configuration fields. Changes: - Replaced multiple transport-related columns (Transport, Remote host, Username, Password, Upload/Download ports and directories) with a single "File transport" column - The file transport column displays the transport name with its connection test status in brackets - Status indicators match the file_transports page: * "Tests passing" (green with checkmark icon) for successful tests * "Errors detected" (red with x icon) for failed tests * "Never used" (italic) for untested transports - Updated the Perl controller to decode the JSON status field from the file_transports table and pass it to the template - Uses get_inflated_columns to convert DBIx::Class results to plain hashrefs for template consumption The display now correctly reflects the architectural change where transport configuration is managed separately in the file_transports table rather than duplicated in each EDI account. Note: At a future date we should replace the raw DBIx::Class methods with Koha::Object based classes. I propose Koha::EDI::Account(s). Created attachment 187148 [details] [review] Bug 38489: Follow-up to bug 39190 to fix administration We did some last minute renaming of things in bug 39190 but didn't catch all the places. This patch updates the old 'sftp_servers' naming scheme to 'file_transports' following the new pattern and restores edit and delete functionality in the File transports management area. Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187149 [details] [review] Bug 38489: Add file_transport_id to vendor_edi_accounts and migrate to new transport system This patch adds database migration to move EDI transport configurations from vendor_edi_accounts to the new file_transports system introduced in Bug 39190. Changes: - Add file_transport_id column to vendor_edi_accounts with foreign key - Add 'local' transport type to file_transports enum - Remove old transport fields from vendor_edi_accounts in kohastructure - Migration script converts existing EDI configurations to file transports - Handles FILE -> local transport type conversion Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187150 [details] [review] Bug 38489: Update EDI accounts UI to use file transport selection This patch updates the EDI accounts administration interface to use the new file_transports system instead of direct transport configuration. Changes: - Replace transport configuration fields with transport selection dropdown - Add file_transports data to template for selection options - Remove password decryption since credentials are now managed by transports - Simplify form handling to only manage EDI-specific fields - Add helpful link to file transport administration Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187151 [details] [review] Bug 38489: Add rename_file method to transport system and Local transport class This patch enhances the file transport system with file renaming capabilities and adds a Local transport class for development/testing. Changes: - Add rename_file() method to base Transport class and SFTP/FTP subclasses - Create Koha::File::Transport::Local for local directory operations - Add 'local' transport type to polymorphic mapping - Update transport instantiation to handle Local transport - Add Local transport tests to verify polymorphic behavior - Enhanced connect() method for Local transport validates directories The rename_file functionality is needed for EDI file processing workflow where files are renamed after download to mark them as processed. Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187152 [details] [review] Bug 38489: Add disconnect method and proper connection cleanup to file transport system This patch adds proper connection cleanup functionality to the file transport system, addressing a gap identified during EDI migration. Changes: - Add disconnect() method to base Transport class interface - Implement SFTP disconnect using connection->disconnect() - Implement FTP disconnect using connection->quit() - Add DESTROY methods to ensure connections are cleaned up on object destruction - Local transport includes no-op disconnect for API consistency This ensures that FTP/SFTP connections are properly closed, preventing connection leaks and potential resource exhaustion issues. Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187153 [details] [review] Bug 38489: Standardize list_files API across all transport classes Fix API inconsistency where different transport types returned different formats from list_files(): - SFTP: returned array of hashrefs with detailed file information - FTP: returned simple array of filenames - Local: returned simple array of filenames Now all transports return an array reference of hashrefs with consistent structure containing filename, longname, and additional metadata. This eliminates the need for workarounds like the ref($file) eq 'HASH' check in EDI code and provides a more robust, consistent API. Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187154 [details] [review] Bug 38489: Update Koha::Edifact::Transport to use new file transport system This patch completely refactors the EDI transport handling to use the modern file_transports system instead of legacy direct connections. Changes: - Remove legacy FTP/SFTP connection code - Update constructor to load file_transport from vendor_edi_accounts - Update download_messages/upload_messages to use modern transport methods - Use new rename_file() method for EDI file processing workflow - Remove dependencies on Net::FTP and Net::SFTP::Foreign - Maintain all EDI-specific functionality (file filtering, ingestion) Cleanup: - Remove obsolete methods: file_download, sftp_download, ftp_download, sftp_upload, ftp_upload, file_upload, _abort_download - Update POD documentation to reflect new architecture - 184 lines of legacy code removed Removed imports: - use DateTime; - Not used, dt_from_string() used instead - use English qw{ -no_match_vars }; - No English variables used - use File::Copy qw( copy move ); - File operations now handled by Koha::File::Transport internally The transport system now provides: - Unified connection handling across all transport types - Enhanced error reporting and status tracking - Encrypted credential storage - Connection testing capabilities - Support for local directories (testing/development) Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187155 [details] [review] Bug 38489: Schema Update Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187156 [details] [review] Bug 38489: Update EDI tests to use new file transport system The EDI test was still creating vendor_edi_accounts with transport='FILE' which no longer works after the migration to the file_transports system. Updated all test account creation to: - Create file_transports records with transport='local' - Link EDI accounts via file_transport_id instead of direct transport field - Set appropriate download/upload directories for local testing All EDI tests now pass with the new transport architecture. Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187157 [details] [review] Bug 38489: Standardize FTP transport API to match SFTP and Local classes Fix several API inconsistencies in the FTP transport class: 1. Message format: Use standardized `message => $operation` format instead of hardcoded strings like "Upload succeeded" 2. Operation tracking: Add $operation variable to all methods for consistent error reporting and logging 3. Error handling: Update _abort_operation() to accept operation parameter and provide more detailed error payloads 4. Success payloads: Replace empty `detail => ''` payloads with meaningful operation-specific information (host, port, file paths, directory info, file counts) All transport classes now have consistent APIs for: - Standardized message formats - Consistent operation tracking - Uniform error handling - Informative success payloads Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Created attachment 187158 [details] [review] Bug 38489: (follow-up) Use 'TableExists' in atomicupdate Created attachment 187159 [details] [review] Bug 38489: (follow-up) Update EDI accounts list display This patch updates the EDI accounts management page to display the new file transport system instead of the old inline transport configuration fields. Changes: - Replaced multiple transport-related columns (Transport, Remote host, Username, Password, Upload/Download ports and directories) with a single "File transport" column - The file transport column displays the transport name with its connection test status in brackets - Status indicators match the file_transports page: * "Tests passing" (green with checkmark icon) for successful tests * "Errors detected" (red with x icon) for failed tests * "Never used" (italic) for untested transports - Updated the Perl controller to decode the JSON status field from the file_transports table and pass it to the template - Uses get_inflated_columns to convert DBIx::Class results to plain hashrefs for template consumption The display now correctly reflects the architectural change where transport configuration is managed separately in the file_transports table rather than duplicated in each EDI account. Note: At a future date we should replace the raw DBIx::Class methods with Koha::Object based classes. I propose Koha::EDI::Account(s). Created attachment 187270 [details] [review] Bug 38489: (follow-up) Update EDI accounts list display Patch from commit 0f82848 Created attachment 187271 [details] [review] Bug 38489: (follow-up) Use 'TableExists' in atomicupdate Patch from commit 87ae5f4 Created attachment 187272 [details] [review] Bug 38489: Standardize FTP transport API to match SFTP and Local classes Patch from commit 8ed390d Created attachment 187273 [details] [review] Bug 38489: Update EDI tests to use new file transport system Patch from commit c4a8c84 Created attachment 187274 [details] [review] Bug 38489: Schema Update Patch from commit b8a3a58 Created attachment 187275 [details] [review] Bug 38489: Update Koha::Edifact::Transport to use new file transport system Patch from commit 2081539 Created attachment 187276 [details] [review] Bug 38489: Standardize list_files API across all transport classes Patch from commit be5bbe4 Created attachment 187277 [details] [review] Bug 38489: Add disconnect method and proper connection cleanup to file transport system Patch from commit 87f917a Created attachment 187278 [details] [review] Bug 38489: Add rename_file method to transport system and Local transport class Patch from commit a7d5087 Created attachment 187279 [details] [review] Bug 38489: Update EDI accounts UI to use file transport selection Patch from commit 0152894 Created attachment 187280 [details] [review] Bug 38489: Add file_transport_id to vendor_edi_accounts and migrate to new transport system Patch from commit 50849f7 Created attachment 187281 [details] [review] Bug 38489: Follow-up to bug 39190 to fix administration Patch from commit 5194b73 Created attachment 187283 [details] [review] Bug 38489: Follow-up to bug 39190 to fix administration We did some last minute renaming of things in bug 39190 but didn't catch all the places. This patch updates the old 'sftp_servers' naming scheme to 'file_transports' following the new pattern and restores edit and delete functionality in the File transports management area. Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 187284 [details] [review] Bug 38489: Add file_transport_id to vendor_edi_accounts and migrate to new transport system This patch adds database migration to move EDI transport configurations from vendor_edi_accounts to the new file_transports system introduced in Bug 39190. Changes: - Add file_transport_id column to vendor_edi_accounts with foreign key - Add 'local' transport type to file_transports enum - Remove old transport fields from vendor_edi_accounts in kohastructure - Migration script converts existing EDI configurations to file transports - Handles FILE -> local transport type conversion Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 187285 [details] [review] Bug 38489: Update EDI accounts UI to use file transport selection This patch updates the EDI accounts administration interface to use the new file_transports system instead of direct transport configuration. Changes: - Replace transport configuration fields with transport selection dropdown - Add file_transports data to template for selection options - Remove password decryption since credentials are now managed by transports - Simplify form handling to only manage EDI-specific fields - Add helpful link to file transport administration Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 187286 [details] [review] Bug 38489: Add rename_file method to transport system and Local transport class This patch enhances the file transport system with file renaming capabilities and adds a Local transport class for development/testing. Changes: - Add rename_file() method to base Transport class and SFTP/FTP subclasses - Create Koha::File::Transport::Local for local directory operations - Add 'local' transport type to polymorphic mapping - Update transport instantiation to handle Local transport - Add Local transport tests to verify polymorphic behavior - Enhanced connect() method for Local transport validates directories The rename_file functionality is needed for EDI file processing workflow where files are renamed after download to mark them as processed. Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 187287 [details] [review] Bug 38489: Add disconnect method and proper connection cleanup to file transport system This patch adds proper connection cleanup functionality to the file transport system, addressing a gap identified during EDI migration. Changes: - Add disconnect() method to base Transport class interface - Implement SFTP disconnect using connection->disconnect() - Implement FTP disconnect using connection->quit() - Add DESTROY methods to ensure connections are cleaned up on object destruction - Local transport includes no-op disconnect for API consistency This ensures that FTP/SFTP connections are properly closed, preventing connection leaks and potential resource exhaustion issues. Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 187288 [details] [review] Bug 38489: Standardize list_files API across all transport classes Fix API inconsistency where different transport types returned different formats from list_files(): - SFTP: returned array of hashrefs with detailed file information - FTP: returned simple array of filenames - Local: returned simple array of filenames Now all transports return an array reference of hashrefs with consistent structure containing filename, longname, and additional metadata. This eliminates the need for workarounds like the ref($file) eq 'HASH' check in EDI code and provides a more robust, consistent API. Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 187289 [details] [review] Bug 38489: Update Koha::Edifact::Transport to use new file transport system This patch completely refactors the EDI transport handling to use the modern file_transports system instead of legacy direct connections. Changes: - Remove legacy FTP/SFTP connection code - Update constructor to load file_transport from vendor_edi_accounts - Update download_messages/upload_messages to use modern transport methods - Use new rename_file() method for EDI file processing workflow - Remove dependencies on Net::FTP and Net::SFTP::Foreign - Maintain all EDI-specific functionality (file filtering, ingestion) Cleanup: - Remove obsolete methods: file_download, sftp_download, ftp_download, sftp_upload, ftp_upload, file_upload, _abort_download - Update POD documentation to reflect new architecture - 184 lines of legacy code removed Removed imports: - use DateTime; - Not used, dt_from_string() used instead - use English qw{ -no_match_vars }; - No English variables used - use File::Copy qw( copy move ); - File operations now handled by Koha::File::Transport internally The transport system now provides: - Unified connection handling across all transport types - Enhanced error reporting and status tracking - Encrypted credential storage - Connection testing capabilities - Support for local directories (testing/development) Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 187290 [details] [review] Bug 38489: Schema Update Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 187291 [details] [review] Bug 38489: Update EDI tests to use new file transport system The EDI test was still creating vendor_edi_accounts with transport='FILE' which no longer works after the migration to the file_transports system. Updated all test account creation to: - Create file_transports records with transport='local' - Link EDI accounts via file_transport_id instead of direct transport field - Set appropriate download/upload directories for local testing All EDI tests now pass with the new transport architecture. Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 187292 [details] [review] Bug 38489: Standardize FTP transport API to match SFTP and Local classes Fix several API inconsistencies in the FTP transport class: 1. Message format: Use standardized `message => $operation` format instead of hardcoded strings like "Upload succeeded" 2. Operation tracking: Add $operation variable to all methods for consistent error reporting and logging 3. Error handling: Update _abort_operation() to accept operation parameter and provide more detailed error payloads 4. Success payloads: Replace empty `detail => ''` payloads with meaningful operation-specific information (host, port, file paths, directory info, file counts) All transport classes now have consistent APIs for: - Standardized message formats - Consistent operation tracking - Uniform error handling - Informative success payloads Sponsored-by: ByWater Solutions Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 187293 [details] [review] Bug 38489: (follow-up) Use 'TableExists' in atomicupdate Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 187294 [details] [review] Bug 38489: (follow-up) Update EDI accounts list display This patch updates the EDI accounts management page to display the new file transport system instead of the old inline transport configuration fields. Changes: - Replaced multiple transport-related columns (Transport, Remote host, Username, Password, Upload/Download ports and directories) with a single "File transport" column - The file transport column displays the transport name with its connection test status in brackets - Status indicators match the file_transports page: * "Tests passing" (green with checkmark icon) for successful tests * "Errors detected" (red with x icon) for failed tests * "Never used" (italic) for untested transports - Updated the Perl controller to decode the JSON status field from the file_transports table and pass it to the template - Uses get_inflated_columns to convert DBIx::Class results to plain hashrefs for template consumption The display now correctly reflects the architectural change where transport configuration is managed separately in the file_transports table rather than duplicated in each EDI account. Note: At a future date we should replace the raw DBIx::Class methods with Koha::Object based classes. I propose Koha::EDI::Account(s). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> |