We add a new UI for managing (S)FTP servers with bug 35761. We want to use these new configurations in other bugs, but the classes currently are just storage and don't offer the transport code. This patchset updates the classes to implement transport methods utilising polymorphism. We also clean up the API's a little to follow more RESTful best practices.
Created attachment 178555 [details] [review] Bug 39190: Add $result to Koha::Objects `object_class` calls This allows us to do dynamic polymorphic classing on calls to 'find', 'find_or_create', 'single', 'next', 'last' and 'as_list' etc. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 178556 [details] [review] Bug 39190: Rework (S)FTP into File::Transport classes This patch adds polymorphic transport classes to Koha for FTP and SFTP file transport configurations. We introduce a base class for interacting with the base configuration storage and subclasses to impliment the protocol specific transport handling.
Created attachment 178557 [details] [review] Bug 39190: Add key handling to SFTP Transports We add triggers to store in the base class and then use them in the SFTP transport class to write out the key file to the filesystem for later use in connect.
Created attachment 178558 [details] [review] Bug 39190: Update API to use new Transport classes As the title says, use the new polymorphic classes in the API controllers
Created attachment 178559 [details] [review] Bug 39190: Update sftp_servers page to use new objects Similar to the previous commit, this time we're updating the page controller to use the new polymorphic classes
Created attachment 178560 [details] [review] Bug 39190: Update status to longtext Update status field to store longtext so we can sore more details information about connection statuses from test connection
Created attachment 178561 [details] [review] Bug 39190: DBIC Update
Created attachment 178562 [details] [review] Bug 39190: Re-implement the connection testing We now use background jobs triggered by Transport->store to update the connection status. Status is now a json blob with more details, so we update the api definition to reflect that.
Created attachment 178563 [details] [review] Bug 39190: Update api tests for status format change
Created attachment 178564 [details] [review] Bug 39190: Start of recording status updates from abort
Created attachment 178565 [details] [review] Bug 39190: Remove old objects
Created attachment 178566 [details] [review] Bug 39190: Remove test routine from API
Created attachment 178567 [details] [review] Bug 39190: Remove replaced test_connection
Created attachment 178568 [details] [review] Bug 39190: Redirect Schema to new classes
Created attachment 178569 [details] [review] Bug 39190: Update API's to reference new classes
Created attachment 178570 [details] [review] Bug 39190: Add transport to test
Created attachment 178571 [details] [review] Bug 39190: Skip polymorphic classes in TestBuilder tests
Created attachment 179252 [details] [review] Bug 39190: Add $result to Koha::Objects `object_class` calls This allows us to do dynamic polymorphic classing on calls to 'find', 'find_or_create', 'single', 'next', 'last' and 'as_list' etc. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 179253 [details] [review] Bug 39190: Rework (S)FTP into File::Transport classes This patch adds polymorphic transport classes to Koha for FTP and SFTP file transport configurations. We introduce a base class for interacting with the base configuration storage and subclasses to impliment the protocol specific transport handling.
Created attachment 179254 [details] [review] Bug 39190: Add key handling to SFTP Transports We add triggers to store in the base class and then use them in the SFTP transport class to write out the key file to the filesystem for later use in connect.
Created attachment 179255 [details] [review] Bug 39190: Update API to use new Transport classes As the title says, use the new polymorphic classes in the API controllers
Created attachment 179256 [details] [review] Bug 39190: Update sftp_servers page to use new objects Similar to the previous commit, this time we're updating the page controller to use the new polymorphic classes
Created attachment 179257 [details] [review] Bug 39190: Update status to longtext Update status field to store longtext so we can sore more details information about connection statuses from test connection
Created attachment 179258 [details] [review] Bug 39190: DBIC Update
Created attachment 179259 [details] [review] Bug 39190: Re-implement the connection testing We now use background jobs triggered by Transport->store to update the connection status. Status is now a json blob with more details, so we update the api definition to reflect that.
Created attachment 179260 [details] [review] Bug 39190: Update api tests for status format change
Created attachment 179261 [details] [review] Bug 39190: Start of recording status updates from abort
Created attachment 179262 [details] [review] Bug 39190: Remove old objects
Created attachment 179263 [details] [review] Bug 39190: Remove test routine from API
Created attachment 179264 [details] [review] Bug 39190: Remove replaced test_connection
Created attachment 179265 [details] [review] Bug 39190: Redirect Schema to new classes
Created attachment 179266 [details] [review] Bug 39190: Update API's to reference new classes
Created attachment 179267 [details] [review] Bug 39190: Add transport to test
Created attachment 179268 [details] [review] Bug 39190: Skip polymorphic classes in TestBuilder tests
Getting at least one unit test failure: kohadev-koha@kohadevbox:koha((63831c2dd5e...))$ prove t/db_dependent/TestBuilder.t t/db_dependent/TestBuilder.t .. 3/16 # Failed test 'Testing Koha::File::Transports' # at t/db_dependent/TestBuilder.t line 495. # got: 'Koha::File::Transport::FTP' # expected: 'Koha::File::Transport' # Looks like you failed 1 test of 1032. # Failed test 'Test all classes' # at t/db_dependent/TestBuilder.t line 515. # Looks like you failed 1 test of 5. t/db_dependent/TestBuilder.t .. 12/16 # Failed test 'build_object() tests' # at t/db_dependent/TestBuilder.t line 535. # Looks like you failed 1 test of 16. t/db_dependent/TestBuilder.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/16 subtests Test Summary Report ------------------- t/db_dependent/TestBuilder.t (Wstat: 256 (exited 1) Tests: 16 Failed: 1) Failed test: 12 Non-zero exit status: 1 Files=1, Tests=16, 26 wallclock secs ( 0.03 usr 0.01 sys + 20.08 cusr 4.58 csys = 24.70 CPU) Result: FAIL
Created attachment 179396 [details] [review] Bug 39190: Add $result to Koha::Objects `object_class` calls This allows us to do dynamic polymorphic classing on calls to 'find', 'find_or_create', 'single', 'next', 'last' and 'as_list' etc. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 179397 [details] [review] Bug 39190: Rework (S)FTP into File::Transport classes This patch adds polymorphic transport classes to Koha for FTP and SFTP file transport configurations. We introduce a base class for interacting with the base configuration storage and subclasses to impliment the protocol specific transport handling.
Created attachment 179398 [details] [review] Bug 39190: Add key handling to SFTP Transports We add triggers to store in the base class and then use them in the SFTP transport class to write out the key file to the filesystem for later use in connect.
Created attachment 179399 [details] [review] Bug 39190: Update API to use new Transport classes As the title says, use the new polymorphic classes in the API controllers
Created attachment 179400 [details] [review] Bug 39190: Update sftp_servers page to use new objects Similar to the previous commit, this time we're updating the page controller to use the new polymorphic classes
Created attachment 179401 [details] [review] Bug 39190: Update status to longtext Update status field to store longtext so we can sore more details information about connection statuses from test connection
Created attachment 179402 [details] [review] Bug 39190: DBIC Update
Created attachment 179403 [details] [review] Bug 39190: Re-implement the connection testing We now use background jobs triggered by Transport->store to update the connection status. Status is now a json blob with more details, so we update the api definition to reflect that.
Created attachment 179404 [details] [review] Bug 39190: Update api tests for status format change
Created attachment 179405 [details] [review] Bug 39190: Start of recording status updates from abort
Created attachment 179406 [details] [review] Bug 39190: Remove old objects
Created attachment 179407 [details] [review] Bug 39190: Remove test routine from API
Created attachment 179408 [details] [review] Bug 39190: Remove replaced test_connection
Created attachment 179409 [details] [review] Bug 39190: Redirect Schema to new classes
Created attachment 179410 [details] [review] Bug 39190: Update API's to reference new classes
Created attachment 179411 [details] [review] Bug 39190: Add transport to test
Created attachment 179412 [details] [review] Bug 39190: Use introspection in TestBuilder tests We update the testbuilder tests here to look for signs of a polymorphic class map and use it to test the different class options for this base class
Created attachment 179413 [details] [review] Bug 39190: Update polymorhpic base class to identify itself This patch adds _polymorphic_field and _polymorphic_map private methods to the Koha::File::Transports class to more clearly identify this as a polymorphic class base for TestBuilder.
Created attachment 179589 [details] [review] Bug 39190: Add $result to Koha::Objects `object_class` calls This allows us to do dynamic polymorphic classing on calls to 'find', 'find_or_create', 'single', 'next', 'last' and 'as_list' etc. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179590 [details] [review] Bug 39190: Rework (S)FTP into File::Transport classes This patch adds polymorphic transport classes to Koha for FTP and SFTP file transport configurations. We introduce a base class for interacting with the base configuration storage and subclasses to impliment the protocol specific transport handling. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179591 [details] [review] Bug 39190: Add key handling to SFTP Transports We add triggers to store in the base class and then use them in the SFTP transport class to write out the key file to the filesystem for later use in connect. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179592 [details] [review] Bug 39190: Update API to use new Transport classes As the title says, use the new polymorphic classes in the API controllers Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179593 [details] [review] Bug 39190: Update sftp_servers page to use new objects Similar to the previous commit, this time we're updating the page controller to use the new polymorphic classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179594 [details] [review] Bug 39190: Update status to longtext Update status field to store longtext so we can sore more details information about connection statuses from test connection Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179595 [details] [review] Bug 39190: DBIC Update Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179596 [details] [review] Bug 39190: Re-implement the connection testing We now use background jobs triggered by Transport->store to update the connection status. Status is now a json blob with more details, so we update the api definition to reflect that. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179597 [details] [review] Bug 39190: Update api tests for status format change Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179598 [details] [review] Bug 39190: Start of recording status updates from abort Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179599 [details] [review] Bug 39190: Remove old objects Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179600 [details] [review] Bug 39190: Remove test routine from API Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179601 [details] [review] Bug 39190: Remove replaced test_connection Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179602 [details] [review] Bug 39190: Redirect Schema to new classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179603 [details] [review] Bug 39190: Update API's to reference new classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179604 [details] [review] Bug 39190: Add transport to test Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179605 [details] [review] Bug 39190: Use introspection in TestBuilder tests We update the testbuilder tests here to look for signs of a polymorphic class map and use it to test the different class options for this base class Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179606 [details] [review] Bug 39190: Update polymorhpic base class to identify itself This patch adds _polymorphic_field and _polymorphic_map private methods to the Koha::File::Transports class to more clearly identify this as a polymorphic class base for TestBuilder. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Transport is listed twice in the form for sftp_servers.pl
Created attachment 179646 [details] [review] Bug 39190: Add $result to Koha::Objects `object_class` calls This allows us to do dynamic polymorphic classing on calls to 'find', 'find_or_create', 'single', 'next', 'last' and 'as_list' etc. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179647 [details] [review] Bug 39190: Rework (S)FTP into File::Transport classes This patch adds polymorphic transport classes to Koha for FTP and SFTP file transport configurations. We introduce a base class for interacting with the base configuration storage and subclasses to impliment the protocol specific transport handling. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179648 [details] [review] Bug 39190: Add key handling to SFTP Transports We add triggers to store in the base class and then use them in the SFTP transport class to write out the key file to the filesystem for later use in connect. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179649 [details] [review] Bug 39190: Update API to use new Transport classes As the title says, use the new polymorphic classes in the API controllers Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179650 [details] [review] Bug 39190: Update sftp_servers page to use new objects Similar to the previous commit, this time we're updating the page controller to use the new polymorphic classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179651 [details] [review] Bug 39190: Update status to longtext Update status field to store longtext so we can sore more details information about connection statuses from test connection Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179652 [details] [review] Bug 39190: DBIC Update Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179653 [details] [review] Bug 39190: Re-implement the connection testing We now use background jobs triggered by Transport->store to update the connection status. Status is now a json blob with more details, so we update the api definition to reflect that. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179654 [details] [review] Bug 39190: Update api tests for status format change Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179655 [details] [review] Bug 39190: Start of recording status updates from abort Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179656 [details] [review] Bug 39190: Remove old objects Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179657 [details] [review] Bug 39190: Remove test routine from API Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179658 [details] [review] Bug 39190: Remove replaced test_connection Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179659 [details] [review] Bug 39190: Redirect Schema to new classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179660 [details] [review] Bug 39190: Update API's to reference new classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179661 [details] [review] Bug 39190: Add transport to test Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179662 [details] [review] Bug 39190: Use introspection in TestBuilder tests We update the testbuilder tests here to look for signs of a polymorphic class map and use it to test the different class options for this base class Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179663 [details] [review] Bug 39190: Update polymorhpic base class to identify itself This patch adds _polymorphic_field and _polymorphic_map private methods to the Koha::File::Transports class to more clearly identify this as a polymorphic class base for TestBuilder. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(In reply to Kyle M Hall (khall) from comment #72) > Transport is listed twice in the form for sftp_servers.pl Well spotted, fixed inline now :) Thanks for reviewing
Created attachment 179992 [details] [review] Bug 39190: Rework (S)FTP into File::Transport classes This patch adds polymorphic transport classes to Koha for FTP and SFTP file transport configurations. We introduce a base class for interacting with the base configuration storage and subclasses to impliment the protocol specific transport handling. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179993 [details] [review] Bug 39190: Add key handling to SFTP Transports We add triggers to store in the base class and then use them in the SFTP transport class to write out the key file to the filesystem for later use in connect. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179994 [details] [review] Bug 39190: Update API to use new Transport classes As the title says, use the new polymorphic classes in the API controllers Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179995 [details] [review] Bug 39190: Update sftp_servers page to use new objects Similar to the previous commit, this time we're updating the page controller to use the new polymorphic classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179996 [details] [review] Bug 39190: Update status to longtext Update status field to store longtext so we can sore more details information about connection statuses from test connection Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179997 [details] [review] Bug 39190: DBIC Update Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179998 [details] [review] Bug 39190: Re-implement the connection testing We now use background jobs triggered by Transport->store to update the connection status. Status is now a json blob with more details, so we update the api definition to reflect that. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 179999 [details] [review] Bug 39190: Update api tests for status format change Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180000 [details] [review] Bug 39190: Start of recording status updates from abort Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180001 [details] [review] Bug 39190: Remove old objects Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180002 [details] [review] Bug 39190: Remove test routine from API Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180003 [details] [review] Bug 39190: Remove replaced test_connection Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180004 [details] [review] Bug 39190: Redirect Schema to new classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180005 [details] [review] Bug 39190: Update API's to reference new classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180006 [details] [review] Bug 39190: Add transport to test Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180010 [details] [review] Bug 39190: Update polymorhpic base class to identify itself This patch adds _polymorphic_field and _polymorphic_map private methods to the Koha::File::Transports class to more clearly identify this as a polymorphic class base for TestBuilder. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180011 [details] [review] Bug 39190: Rework (S)FTP into File::Transport classes This patch adds polymorphic transport classes to Koha for FTP and SFTP file transport configurations. We introduce a base class for interacting with the base configuration storage and subclasses to impliment the protocol specific transport handling. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180012 [details] [review] Bug 39190: Add key handling to SFTP Transports We add triggers to store in the base class and then use them in the SFTP transport class to write out the key file to the filesystem for later use in connect. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180013 [details] [review] Bug 39190: Update API to use new Transport classes As the title says, use the new polymorphic classes in the API controllers Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180014 [details] [review] Bug 39190: Update sftp_servers page to use new objects Similar to the previous commit, this time we're updating the page controller to use the new polymorphic classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180015 [details] [review] Bug 39190: Update status to longtext Update status field to store longtext so we can sore more details information about connection statuses from test connection Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180016 [details] [review] Bug 39190: DBIC Update Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180017 [details] [review] Bug 39190: Re-implement the connection testing We now use background jobs triggered by Transport->store to update the connection status. Status is now a json blob with more details, so we update the api definition to reflect that. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180018 [details] [review] Bug 39190: Update api tests for status format change Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180019 [details] [review] Bug 39190: Start of recording status updates from abort Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180020 [details] [review] Bug 39190: Remove old objects Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180021 [details] [review] Bug 39190: Remove test routine from API Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180022 [details] [review] Bug 39190: Remove replaced test_connection Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180023 [details] [review] Bug 39190: Redirect Schema to new classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180024 [details] [review] Bug 39190: Update API's to reference new classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 180025 [details] [review] Bug 39190: Add transport to test Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
I think that polymorphism (at the Koha::Object level) is not the best solution here. I think the problem is that Koha::File::Transport tries to be many things at once: it is a database entity and a file transporter. Those are two very different things. We could have Koha::File::Transport::FTP and Koha::File::Transport::SFTP as non-Koha::Object and have (for instance) a Koha::File::Transport::transporter method that returns the correct object depending on the value of transport column. Would that work ?
Created attachment 185014 [details] [review] Bug 39190: Update polymorhpic base class to identify itself This patch adds _polymorphic_field and _polymorphic_map private methods to the Koha::File::Transports class to more clearly identify this as a polymorphic class base for TestBuilder. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 185015 [details] [review] Bug 39190: Rework (S)FTP into File::Transport classes This patch adds polymorphic transport classes to Koha for FTP and SFTP file transport configurations. We introduce a base class for interacting with the base configuration storage and subclasses to impliment the protocol specific transport handling. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 185016 [details] [review] Bug 39190: Add key handling to SFTP Transports We add triggers to store in the base class and then use them in the SFTP transport class to write out the key file to the filesystem for later use in connect. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 185017 [details] [review] Bug 39190: Update API to use new Transport classes As the title says, use the new polymorphic classes in the API controllers Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 185018 [details] [review] Bug 39190: Update sftp_servers page to use new objects Similar to the previous commit, this time we're updating the page controller to use the new polymorphic classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 185019 [details] [review] Bug 39190: Update status to longtext Update status field to store longtext so we can sore more details information about connection statuses from test connection Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 185020 [details] [review] Bug 39190: DBIC Update Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 185021 [details] [review] Bug 39190: Re-implement the connection testing We now use background jobs triggered by Transport->store to update the connection status. Status is now a json blob with more details, so we update the api definition to reflect that. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 185022 [details] [review] Bug 39190: Update api tests for status format change Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 185023 [details] [review] Bug 39190: Start of recording status updates from abort Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 185024 [details] [review] Bug 39190: Remove old objects Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 185025 [details] [review] Bug 39190: Remove test routine from API Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 185026 [details] [review] Bug 39190: Remove replaced test_connection Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 185027 [details] [review] Bug 39190: Redirect Schema to new classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 185028 [details] [review] Bug 39190: Update API's to reference new classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 185029 [details] [review] Bug 39190: Add transport to test Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(In reply to Julian Maurice from comment #123) > I think that polymorphism (at the Koha::Object level) is not the best > solution here. I think the problem is that Koha::File::Transport tries to be > many things at once: it is a database entity and a file transporter. Those > are two very different things. We already have (to some extent) some other classes that could be considered polymorphic: * Background jobs: implement the `_derived_class` method i.e. we search for Koha::BackgroundJob objects then call `->derived_class()` in the caller. This happens in the 'workers' * IdP classes: implement the `upgrade_class` method. Only used for OAuth at the moment but other cases coming (I submitted CAS and Shibboleth classes) * Koha::Account::Line: doesn't implement an upgrade method, Koha::Account can be used with `credits()` and `debits()` to retrieve certain type with the appropriate object type. We might need to revisit them. Will keep thinking about it over the weekend.
Created attachment 185972 [details] [review] Bug 39190: Update polymorhpic base class to identify itself This patch adds _polymorphic_field and _polymorphic_map private methods to the Koha::File::Transports class to more clearly identify this as a polymorphic class base for TestBuilder. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185973 [details] [review] Bug 39190: Rework (S)FTP into File::Transport classes This patch adds polymorphic transport classes to Koha for FTP and SFTP file transport configurations. We introduce a base class for interacting with the base configuration storage and subclasses to impliment the protocol specific transport handling. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185974 [details] [review] Bug 39190: Add key handling to SFTP Transports We add triggers to store in the base class and then use them in the SFTP transport class to write out the key file to the filesystem for later use in connect. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185975 [details] [review] Bug 39190: Update API to use new Transport classes As the title says, use the new polymorphic classes in the API controllers Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185976 [details] [review] Bug 39190: Update sftp_servers page to use new objects Similar to the previous commit, this time we're updating the page controller to use the new polymorphic classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185977 [details] [review] Bug 39190: Update status to longtext Update status field to store longtext so we can sore more details information about connection statuses from test connection Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185978 [details] [review] Bug 39190: Re-implement the connection testing We now use background jobs triggered by Transport->store to update the connection status. Status is now a json blob with more details, so we update the api definition to reflect that. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185979 [details] [review] Bug 39190: Update api tests for status format change Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185980 [details] [review] Bug 39190: Start of recording status updates from abort Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185981 [details] [review] Bug 39190: Remove old objects Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185982 [details] [review] Bug 39190: Remove test routine from API Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185983 [details] [review] Bug 39190: Remove replaced test_connection Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185984 [details] [review] Bug 39190: Redirect Schema to new classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185985 [details] [review] Bug 39190: Update API's to reference new classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185986 [details] [review] Bug 39190: Add transport to test Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185987 [details] [review] Bug 39190: Rename 'sftp_servers' to 'file_transports' Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185988 [details] [review] Bug 39190: Adjust schema to new structure Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185989 [details] [review] Bug 39190: Adjust permission across the board Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185990 [details] [review] Bug 39190: Fix circular dependency causing C3 MRO failures Resolved circular dependency chain: Transport.pm → TestTransport.pm → Transports.pm → SFTP.pm → Transport.pm Changed 'use Koha::BackgroundJob::TestTransport' to runtime 'require' in the _post_store_trigger method to break the dependency cycle. This fixes the C3 method resolution order failures in Koha::File::Transport::FTP and Koha::File::Transport::SFTP.
I haven't finished here. The SFTP.t failure hints the class resolution mechanism is flawed. And I think we should rename all 'sftp*' scripts and templates right away too.
Created attachment 185994 [details] [review] Bug 39190: Regression tests for polymorphic object creation This patch adds comprehensive tests to verify that Koha::File::Transports correctly returns polymorphic objects based on the transport field value. The tests cover: - build_object() polymorphic creation - search() with polymorphic resultset iteration - find() with polymorphic object return To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/File/Transports.t => FAIL: Tests fail - objects return base Koha::File::Transport instead of polymorphic Koha::File::Transport::SFTP/FTP classes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185995 [details] [review] Bug 39190: Fix polymorphic object creation in Koha::Objects This patch fixes polymorphic object creation by ensuring all methods in Koha::Objects that instantiate objects pass the DBIC result to the object_class() method for proper polymorphic resolution. Fixed methods: - find(): Now passes $result to object_class($result) - next(): Now passes $result to object_class($result) - _wrap(): Now passes $_ to object_class($_) in map This enables polymorphic classes like Koha::File::Transports to return the correct subclass (Koha::File::Transport::SFTP, Koha::File::Transport::FTP) based on the transport field value, instead of always returning the base Koha::File::Transport class. To test: 1. Apply the regression tests patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/File/Transports.t \ t/db_dependent/Koha/File/Transport/SFTP.t => FAIL: Tests fail! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185996 [details] [review] Bug 39190: Rename admin interface from sftp_servers to file_transports Renamed admin interface files and URLs to reflect the evolution from 'SFTP servers' to 'File transports' functionality: - admin/sftp_servers.pl -> admin/file_transports.pl - sftp_servers.tt -> file_transports.tt - Updated all URL references in templates and admin menu - Updated body id from admin_sftp_servers to admin_file_transports The functionality is now future-proof for the addition of new file transports, so the naming should reflect his broader scope. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185997 [details] [review] Bug 39190: Rename API from sftp_servers to file_transports This patch renames the entire API structure from SFTP-specific to generic file transport terminology to reflect the polymorphic nature of the system. API Changes: - Endpoints: /config/sftp_servers -> /config/file_transports - Parameters: sftp_server_id -> file_transport_id - Definitions: sftp_server -> file_transport - Controller: Config::SFTP::Servers -> Config::File::Transports File Changes: - api/v1/swagger/definitions/sftp_server.yaml -> file_transport.yaml - api/v1/swagger/paths/config_sftp_servers.yaml -> config_file_transports.yaml - Koha/REST/V1/Config/SFTP/Servers.pm -> Koha/REST/V1/Config/File/Transports.pm - t/db_dependent/api/v1/sftp_servers.t -> file_transports.t Updated References: - swagger.yaml: paths, definitions, parameters, tags - Template JavaScript: API URLs and variable names - Controller: package name, variable names, error messages - Tests: endpoints, parameters, descriptions - API mapping: to_api_mapping field name correction To test: 1. Apply this patch 2. Run: yarn api:bundle (to regenerate swagger bundle) 3. Run: prove t/db_dependent/api/v1/file_transports.t 4. All API tests should pass 5. Verify admin interface uses new API endpoints 6. Test CRUD operations work correctly The API now consistently uses 'file_transports' terminology matching the polymorphic file transport system that supports FTP, SFTP, and future transport types. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 185998 [details] [review] Bug 39190: Rename id => file_transport_id and remove mapping Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Tomás Cohen Arazi (tcohen) from comment #160) > I haven't finished here. The SFTP.t failure hints the class resolution > mechanism is flawed. And I think we should rename all 'sftp*' scripts and > templates right away too. I now finished what I believe this patchset should be like. It is all designed to be pushed in one shot along with bug 35761. RM: please don't push until Martin has given his blessing to the followups. I'm happy to roll back and start over if there's no agreement on some of it. Happy weekend!
Setting to BLOCKED until Bug 35761 is out of FQA
Looking good.. It has my blessing.
Created attachment 186408 [details] [review] Bug 39190: Update polymorhpic base class to identify itself This patch adds _polymorphic_field and _polymorphic_map private methods to the Koha::File::Transports class to more clearly identify this as a polymorphic class base for TestBuilder. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186409 [details] [review] Bug 39190: Update polymorhpic base class to identify itself This patch adds _polymorphic_field and _polymorphic_map private methods to the Koha::File::Transports class to more clearly identify this as a polymorphic class base for TestBuilder. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186410 [details] [review] Bug 39190: Rework (S)FTP into File::Transport classes This patch adds polymorphic transport classes to Koha for FTP and SFTP file transport configurations. We introduce a base class for interacting with the base configuration storage and subclasses to impliment the protocol specific transport handling. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186411 [details] [review] Bug 39190: Add key handling to SFTP Transports We add triggers to store in the base class and then use them in the SFTP transport class to write out the key file to the filesystem for later use in connect. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186412 [details] [review] Bug 39190: Update API to use new Transport classes As the title says, use the new polymorphic classes in the API controllers Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186413 [details] [review] Bug 39190: Update sftp_servers page to use new objects Similar to the previous commit, this time we're updating the page controller to use the new polymorphic classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186414 [details] [review] Bug 39190: Update status to longtext Update status field to store longtext so we can sore more details information about connection statuses from test connection Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186415 [details] [review] Bug 39190: Re-implement the connection testing We now use background jobs triggered by Transport->store to update the connection status. Status is now a json blob with more details, so we update the api definition to reflect that. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186416 [details] [review] Bug 39190: Update api tests for status format change Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186417 [details] [review] Bug 39190: Start of recording status updates from abort Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186418 [details] [review] Bug 39190: Remove old objects Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186419 [details] [review] Bug 39190: Remove test routine from API Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186420 [details] [review] Bug 39190: Remove replaced test_connection Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186421 [details] [review] Bug 39190: Redirect Schema to new classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186422 [details] [review] Bug 39190: Update API's to reference new classes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186423 [details] [review] Bug 39190: Add transport to test Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186424 [details] [review] Bug 39190: Rename 'sftp_servers' to 'file_transports' Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186425 [details] [review] Bug 39190: Adjust schema to new structure Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186426 [details] [review] Bug 39190: Adjust permission across the board Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186427 [details] [review] Bug 39190: Fix circular dependency causing C3 MRO failures Resolved circular dependency chain: Transport.pm → TestTransport.pm → Transports.pm → SFTP.pm → Transport.pm Changed 'use Koha::BackgroundJob::TestTransport' to runtime 'require' in the _post_store_trigger method to break the dependency cycle. This fixes the C3 method resolution order failures in Koha::File::Transport::FTP and Koha::File::Transport::SFTP. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186428 [details] [review] Bug 39190: Regression tests for polymorphic object creation This patch adds comprehensive tests to verify that Koha::File::Transports correctly returns polymorphic objects based on the transport field value. The tests cover: - build_object() polymorphic creation - search() with polymorphic resultset iteration - find() with polymorphic object return To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/File/Transports.t => FAIL: Tests fail - objects return base Koha::File::Transport instead of polymorphic Koha::File::Transport::SFTP/FTP classes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186429 [details] [review] Bug 39190: Rename admin interface from sftp_servers to file_transports Renamed admin interface files and URLs to reflect the evolution from 'SFTP servers' to 'File transports' functionality: - admin/sftp_servers.pl -> admin/file_transports.pl - sftp_servers.tt -> file_transports.tt - Updated all URL references in templates and admin menu - Updated body id from admin_sftp_servers to admin_file_transports The functionality is now future-proof for the addition of new file transports, so the naming should reflect his broader scope. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186430 [details] [review] Bug 39190: Rename API from sftp_servers to file_transports This patch renames the entire API structure from SFTP-specific to generic file transport terminology to reflect the polymorphic nature of the system. API Changes: - Endpoints: /config/sftp_servers -> /config/file_transports - Parameters: sftp_server_id -> file_transport_id - Definitions: sftp_server -> file_transport - Controller: Config::SFTP::Servers -> Config::File::Transports File Changes: - api/v1/swagger/definitions/sftp_server.yaml -> file_transport.yaml - api/v1/swagger/paths/config_sftp_servers.yaml -> config_file_transports.yaml - Koha/REST/V1/Config/SFTP/Servers.pm -> Koha/REST/V1/Config/File/Transports.pm - t/db_dependent/api/v1/sftp_servers.t -> file_transports.t Updated References: - swagger.yaml: paths, definitions, parameters, tags - Template JavaScript: API URLs and variable names - Controller: package name, variable names, error messages - Tests: endpoints, parameters, descriptions - API mapping: to_api_mapping field name correction To test: 1. Apply this patch 2. Run: yarn api:bundle (to regenerate swagger bundle) 3. Run: prove t/db_dependent/api/v1/file_transports.t 4. All API tests should pass 5. Verify admin interface uses new API endpoints 6. Test CRUD operations work correctly The API now consistently uses 'file_transports' terminology matching the polymorphic file transport system that supports FTP, SFTP, and future transport types. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 186431 [details] [review] Bug 39190: Rename id => file_transport_id and remove mapping Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>