Bug 38489 - EDI should be updated to use the new FTP/SFTP Servers management page
Summary: EDI should be updated to use the new FTP/SFTP Servers management page
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Martin Renvoize (ashimema)
QA Contact: Kyle M Hall (khall)
URL:
Keywords:
Depends on: 39190
Blocks: 20253 33308 38115 38195 38629
  Show dependency treegraph
 
Reported: 2024-11-20 09:23 UTC by Jake Deery
Modified: 2025-10-02 14:03 UTC (History)
3 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
EDI (EDIFACT) transport configuration has been completely refactored to use the unified file transport system introduced in Bug 35761 and Bug 39190, eliminating the duplication of FTP/SFTP server configuration that previously existed between EDI accounts and the file transport administration. Previously, EDI accounts stored their own FTP/SFTP connection details (host, port, username, password) separately from the FTP/SFTP servers configuration, leading to duplicate maintenance. Now, EDI accounts simply reference a configured file transport, with the transport handling all connection management. **Key changes:** - **Unified configuration**: EDI accounts now select an existing file transport for upload and download operations rather than storing duplicate connection details - **Automatic migration**: Existing EDI transport configurations are automatically migrated to file transport records during upgrade - **Enhanced transport capabilities**: EDI now benefits from the file transport system's connection testing, status monitoring, and encrypted credential storage - **New 'local' transport type**: Added support for local directory operations, useful for development and testing - **Simplified EDI interface**: The EDI account form now uses dropdown selectors for file transports instead of requiring manual entry of connection details **Technical improvements:** - Removed 184 lines of legacy FTP/SFTP connection code from `Koha::Edifact::Transport` - Added `rename_file()` method to transport classes for EDI file processing workflow - Added `disconnect()` method for proper connection cleanup - Standardised `list_files()` API across all transport types - All transport classes now return consistent data structures **For administrators:** When upgrading, your existing EDI transport configurations will be automatically converted to file transport records. After upgrade, you'll manage FTP/SFTP credentials in one place (Administration → File transports) rather than duplicating them for each EDI account.
Version(s) released in:
Circulation function:


Attachments
Bug 35761: Integrate EDI into new FTP/SFTP management UI (33.54 KB, patch)
2024-12-04 17:41 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 38489: (follow-up) tidy EDIfact files (14.60 KB, patch)
2024-12-04 17:41 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 38489: Integrate EDI into new FTP/SFTP management UI (33.54 KB, patch)
2024-12-04 17:42 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 38489: (follow-up) tidy EDIfact files (14.60 KB, patch)
2024-12-04 17:42 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 38489: Integrate EDI into new FTP/SFTP management UI (35.56 KB, patch)
2025-01-03 13:19 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 38489: (follow-up) tidy EDIfact files (16.97 KB, patch)
2025-01-03 13:25 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 38489: (follow-up) tidy EDIfact files (16.97 KB, patch)
2025-01-06 15:50 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 38489: Integrate EDI into new FTP/SFTP management UI (41.55 KB, patch)
2025-08-12 12:53 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 38489: Integrate EDI into new FTP/SFTP management UI (33.09 KB, patch)
2025-09-23 15:04 UTC, Jake Deery
Details | Diff | Splinter Review
DBIC - DO NOT PUSH (3.02 KB, patch)
2025-09-23 15:06 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 38489: Add file_transport_id to vendor_edi_accounts and migrate to new transport system (9.37 KB, patch)
2025-09-23 20:06 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Update EDI accounts UI to use file transport selection (9.39 KB, patch)
2025-09-23 20:06 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Add rename_file method to transport system and Local transport class (16.19 KB, patch)
2025-09-23 20:06 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Add disconnect method and proper connection cleanup to file transport system (3.87 KB, patch)
2025-09-23 20:06 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Standardize list_files API across all transport classes (4.94 KB, patch)
2025-09-23 20:07 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Update Koha::Edifact::Transport to use new file transport system (18.89 KB, patch)
2025-09-23 20:07 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Schema Update (5.87 KB, patch)
2025-09-23 20:07 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Update EDI tests to use new file transport system (8.70 KB, patch)
2025-09-23 20:07 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Standardize FTP transport API to match SFTP and Local classes (6.42 KB, patch)
2025-09-23 20:39 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Follow-up to bug 39190 to fix administration (42.74 KB, patch)
2025-09-30 12:04 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Add file_transport_id to vendor_edi_accounts and migrate to new transport system (9.47 KB, patch)
2025-09-30 12:04 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Update EDI accounts UI to use file transport selection (9.49 KB, patch)
2025-09-30 12:04 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Add rename_file method to transport system and Local transport class (16.29 KB, patch)
2025-09-30 12:04 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Add disconnect method and proper connection cleanup to file transport system (3.97 KB, patch)
2025-09-30 12:04 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Standardize list_files API across all transport classes (5.04 KB, patch)
2025-09-30 12:04 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Update Koha::Edifact::Transport to use new file transport system (19.00 KB, patch)
2025-09-30 12:04 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Schema Update (5.97 KB, patch)
2025-09-30 12:04 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Update EDI tests to use new file transport system (8.80 KB, patch)
2025-09-30 12:04 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Standardize FTP transport API to match SFTP and Local classes (6.53 KB, patch)
2025-09-30 12:04 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: (follow-up) Use 'TableExists' in atomicupdate (11.88 KB, patch)
2025-09-30 15:12 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: (follow-up) Update EDI accounts list display (6.56 KB, patch)
2025-10-01 05:41 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Follow-up to bug 39190 to fix administration (42.74 KB, patch)
2025-10-01 07:03 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Add file_transport_id to vendor_edi_accounts and migrate to new transport system (9.47 KB, patch)
2025-10-01 07:03 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Update EDI accounts UI to use file transport selection (9.49 KB, patch)
2025-10-01 07:03 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Add rename_file method to transport system and Local transport class (16.29 KB, patch)
2025-10-01 07:03 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Add disconnect method and proper connection cleanup to file transport system (3.97 KB, patch)
2025-10-01 07:03 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Standardize list_files API across all transport classes (5.04 KB, patch)
2025-10-01 07:03 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Update Koha::Edifact::Transport to use new file transport system (19.33 KB, patch)
2025-10-01 07:03 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Schema Update (5.97 KB, patch)
2025-10-01 07:03 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Update EDI tests to use new file transport system (8.80 KB, patch)
2025-10-01 07:03 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: Standardize FTP transport API to match SFTP and Local classes (6.53 KB, patch)
2025-10-01 07:03 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: (follow-up) Use 'TableExists' in atomicupdate (11.88 KB, patch)
2025-10-01 07:03 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: (follow-up) Update EDI accounts list display (6.56 KB, patch)
2025-10-01 07:03 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38489: (follow-up) Update EDI accounts list display (6.63 KB, patch)
2025-10-02 13:35 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: (follow-up) Use 'TableExists' in atomicupdate (11.96 KB, patch)
2025-10-02 13:35 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Standardize FTP transport API to match SFTP and Local classes (6.60 KB, patch)
2025-10-02 13:35 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Update EDI tests to use new file transport system (8.87 KB, patch)
2025-10-02 13:35 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Schema Update (6.04 KB, patch)
2025-10-02 13:35 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Update Koha::Edifact::Transport to use new file transport system (19.40 KB, patch)
2025-10-02 13:35 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Standardize list_files API across all transport classes (5.11 KB, patch)
2025-10-02 13:35 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Add disconnect method and proper connection cleanup to file transport system (4.04 KB, patch)
2025-10-02 13:35 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Add rename_file method to transport system and Local transport class (16.36 KB, patch)
2025-10-02 13:35 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Update EDI accounts UI to use file transport selection (9.56 KB, patch)
2025-10-02 13:35 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Add file_transport_id to vendor_edi_accounts and migrate to new transport system (9.54 KB, patch)
2025-10-02 13:35 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Follow-up to bug 39190 to fix administration (42.81 KB, patch)
2025-10-02 13:35 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Follow-up to bug 39190 to fix administration (42.81 KB, patch)
2025-10-02 13:52 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Add file_transport_id to vendor_edi_accounts and migrate to new transport system (9.54 KB, patch)
2025-10-02 13:52 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Update EDI accounts UI to use file transport selection (9.56 KB, patch)
2025-10-02 13:52 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Add rename_file method to transport system and Local transport class (16.36 KB, patch)
2025-10-02 13:52 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Add disconnect method and proper connection cleanup to file transport system (4.04 KB, patch)
2025-10-02 13:52 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Standardize list_files API across all transport classes (5.11 KB, patch)
2025-10-02 13:52 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Update Koha::Edifact::Transport to use new file transport system (19.40 KB, patch)
2025-10-02 13:52 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Schema Update (6.04 KB, patch)
2025-10-02 13:52 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Update EDI tests to use new file transport system (8.87 KB, patch)
2025-10-02 13:52 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: Standardize FTP transport API to match SFTP and Local classes (6.60 KB, patch)
2025-10-02 13:52 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: (follow-up) Use 'TableExists' in atomicupdate (11.95 KB, patch)
2025-10-02 13:52 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 38489: (follow-up) Update EDI accounts list display (6.63 KB, patch)
2025-10-02 13:52 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 Jake Deery 2024-11-20 09:23:09 UTC
See Bug 35761. This bug should continue this work, by moving the EDI transport information to the sftp_servers table, replacing the FTP/SFTP server details instead with a foreign key.
Comment 1 Jake Deery 2024-12-04 17:41:17 UTC Comment hidden (obsolete)
Comment 2 Jake Deery 2024-12-04 17:41:28 UTC Comment hidden (obsolete)
Comment 3 Jake Deery 2024-12-04 17:42:39 UTC Comment hidden (obsolete)
Comment 4 Jake Deery 2024-12-04 17:42:48 UTC Comment hidden (obsolete)
Comment 5 Jake Deery 2025-01-03 13:19:32 UTC
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!
Comment 6 Jake Deery 2025-01-03 13:25:21 UTC
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
Comment 7 Jake Deery 2025-01-06 15:50:24 UTC
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
Comment 8 Andrew Fuerste-Henry 2025-02-14 16:16:29 UTC
Please rebase for signoff. Thanks!
Comment 9 Jake Deery 2025-08-12 12:53:55 UTC
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!
Comment 10 Jake Deery 2025-08-12 12:58:30 UTC
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.
Comment 11 Jake Deery 2025-09-23 15:04:31 UTC
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!
Comment 12 Jake Deery 2025-09-23 15:06:15 UTC
Created attachment 186820 [details] [review]
DBIC - DO NOT PUSH
Comment 13 Jake Deery 2025-09-23 15:11:07 UTC
Another rebase..! Also, split out the DBIC code for easier pushing to main.

Ta,
Jake.
Comment 14 Martin Renvoize (ashimema) 2025-09-23 20:06:52 UTC
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
Comment 15 Martin Renvoize (ashimema) 2025-09-23 20:06:54 UTC
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
Comment 16 Martin Renvoize (ashimema) 2025-09-23 20:06:56 UTC
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.
Comment 17 Martin Renvoize (ashimema) 2025-09-23 20:06:59 UTC
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.
Comment 18 Martin Renvoize (ashimema) 2025-09-23 20:07:01 UTC
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.
Comment 19 Martin Renvoize (ashimema) 2025-09-23 20:07:03 UTC
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)
Comment 20 Martin Renvoize (ashimema) 2025-09-23 20:07:05 UTC
Created attachment 186851 [details] [review]
Bug 38489: Schema Update
Comment 21 Martin Renvoize (ashimema) 2025-09-23 20:07:07 UTC
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.
Comment 22 Martin Renvoize (ashimema) 2025-09-23 20:09:02 UTC
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.
Comment 23 Martin Renvoize (ashimema) 2025-09-23 20:09:56 UTC
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!
Comment 24 Martin Renvoize (ashimema) 2025-09-23 20:39:13 UTC
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
Comment 25 Kyle M Hall (khall) 2025-09-30 10:56:20 UTC
Error when editing a saved file transport:  An error occurred trying to open the server for editing. The passed ID is invalid.
Comment 26 Martin Renvoize (ashimema) 2025-09-30 12:04:07 UTC
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>
Comment 27 Martin Renvoize (ashimema) 2025-09-30 12:04:16 UTC
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>
Comment 28 Martin Renvoize (ashimema) 2025-09-30 12:04:21 UTC
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>
Comment 29 Martin Renvoize (ashimema) 2025-09-30 12:04:24 UTC
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>
Comment 30 Martin Renvoize (ashimema) 2025-09-30 12:04:29 UTC
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>
Comment 31 Martin Renvoize (ashimema) 2025-09-30 12:04:33 UTC
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>
Comment 32 Martin Renvoize (ashimema) 2025-09-30 12:04:39 UTC
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>
Comment 33 Martin Renvoize (ashimema) 2025-09-30 12:04:41 UTC
Created attachment 187087 [details] [review]
Bug 38489: Schema Update

Sponsored-by: ByWater Solutions
Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk>
Comment 34 Martin Renvoize (ashimema) 2025-09-30 12:04:44 UTC
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>
Comment 35 Martin Renvoize (ashimema) 2025-09-30 12:04:47 UTC
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>
Comment 36 Kyle M Hall (khall) 2025-09-30 12:30:37 UTC
The edi accounts table still has columns for the s/ftp data. Those should be removed!
Comment 37 Martin Renvoize (ashimema) 2025-09-30 15:12:37 UTC
Created attachment 187108 [details] [review]
Bug 38489: (follow-up) Use 'TableExists' in atomicupdate
Comment 38 Martin Renvoize (ashimema) 2025-09-30 15:12:55 UTC
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?
Comment 39 Martin Renvoize (ashimema) 2025-09-30 15:17:26 UTC
My mistake.. you mean the display table on the edi accounts management page
Comment 40 Martin Renvoize (ashimema) 2025-10-01 05:41:19 UTC
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).
Comment 41 Martin Renvoize (ashimema) 2025-10-01 07:03:33 UTC
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>
Comment 42 Martin Renvoize (ashimema) 2025-10-01 07:03:36 UTC
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>
Comment 43 Martin Renvoize (ashimema) 2025-10-01 07:03:38 UTC
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>
Comment 44 Martin Renvoize (ashimema) 2025-10-01 07:03:40 UTC
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>
Comment 45 Martin Renvoize (ashimema) 2025-10-01 07:03:43 UTC
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>
Comment 46 Martin Renvoize (ashimema) 2025-10-01 07:03:45 UTC
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>
Comment 47 Martin Renvoize (ashimema) 2025-10-01 07:03:47 UTC
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>
Comment 48 Martin Renvoize (ashimema) 2025-10-01 07:03:50 UTC
Created attachment 187155 [details] [review]
Bug 38489: Schema Update

Sponsored-by: ByWater Solutions
Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk>
Comment 49 Martin Renvoize (ashimema) 2025-10-01 07:03:52 UTC
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>
Comment 50 Martin Renvoize (ashimema) 2025-10-01 07:03:54 UTC
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>
Comment 51 Martin Renvoize (ashimema) 2025-10-01 07:03:57 UTC
Created attachment 187158 [details] [review]
Bug 38489: (follow-up) Use 'TableExists' in atomicupdate
Comment 52 Martin Renvoize (ashimema) 2025-10-01 07:03:59 UTC
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).
Comment 53 Kyle M Hall (khall) 2025-10-02 13:35:33 UTC
Created attachment 187270 [details] [review]
Bug 38489: (follow-up) Update EDI accounts list display

Patch from commit 0f82848
Comment 54 Kyle M Hall (khall) 2025-10-02 13:35:35 UTC
Created attachment 187271 [details] [review]
Bug 38489: (follow-up) Use 'TableExists' in atomicupdate

Patch from commit 87ae5f4
Comment 55 Kyle M Hall (khall) 2025-10-02 13:35:36 UTC
Created attachment 187272 [details] [review]
Bug 38489: Standardize FTP transport API to match SFTP and Local classes

Patch from commit 8ed390d
Comment 56 Kyle M Hall (khall) 2025-10-02 13:35:38 UTC
Created attachment 187273 [details] [review]
Bug 38489: Update EDI tests to use new file transport system

Patch from commit c4a8c84
Comment 57 Kyle M Hall (khall) 2025-10-02 13:35:40 UTC
Created attachment 187274 [details] [review]
Bug 38489: Schema Update

Patch from commit b8a3a58
Comment 58 Kyle M Hall (khall) 2025-10-02 13:35:41 UTC
Created attachment 187275 [details] [review]
Bug 38489: Update Koha::Edifact::Transport to use new file transport system

Patch from commit 2081539
Comment 59 Kyle M Hall (khall) 2025-10-02 13:35:43 UTC
Created attachment 187276 [details] [review]
Bug 38489: Standardize list_files API across all transport classes

Patch from commit be5bbe4
Comment 60 Kyle M Hall (khall) 2025-10-02 13:35:44 UTC
Created attachment 187277 [details] [review]
Bug 38489: Add disconnect method and proper connection cleanup to file transport system

Patch from commit 87f917a
Comment 61 Kyle M Hall (khall) 2025-10-02 13:35:46 UTC
Created attachment 187278 [details] [review]
Bug 38489: Add rename_file method to transport system and Local transport class

Patch from commit a7d5087
Comment 62 Kyle M Hall (khall) 2025-10-02 13:35:47 UTC
Created attachment 187279 [details] [review]
Bug 38489: Update EDI accounts UI to use file transport selection

Patch from commit 0152894
Comment 63 Kyle M Hall (khall) 2025-10-02 13:35:49 UTC
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
Comment 64 Kyle M Hall (khall) 2025-10-02 13:35:51 UTC
Created attachment 187281 [details] [review]
Bug 38489: Follow-up to bug 39190 to fix administration

Patch from commit 5194b73
Comment 65 Kyle M Hall (khall) 2025-10-02 13:52:11 UTC
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>
Comment 66 Kyle M Hall (khall) 2025-10-02 13:52:14 UTC
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>
Comment 67 Kyle M Hall (khall) 2025-10-02 13:52:18 UTC
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>
Comment 68 Kyle M Hall (khall) 2025-10-02 13:52:22 UTC
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>
Comment 69 Kyle M Hall (khall) 2025-10-02 13:52:25 UTC
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>
Comment 70 Kyle M Hall (khall) 2025-10-02 13:52:29 UTC
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>
Comment 71 Kyle M Hall (khall) 2025-10-02 13:52:33 UTC
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>
Comment 72 Kyle M Hall (khall) 2025-10-02 13:52:36 UTC
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>
Comment 73 Kyle M Hall (khall) 2025-10-02 13:52:40 UTC
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>
Comment 74 Kyle M Hall (khall) 2025-10-02 13:52:44 UTC
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>
Comment 75 Kyle M Hall (khall) 2025-10-02 13:52:47 UTC
Created attachment 187293 [details] [review]
Bug 38489: (follow-up) Use 'TableExists' in atomicupdate

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 76 Kyle M Hall (khall) 2025-10-02 13:52:51 UTC
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>