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 ?