[Bug 42656] New: File transport test responses can be unclear
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 Bug ID: 42656 Summary: File transport test responses can be unclear Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Staff interface Assignee: koha-bugs@lists.koha-community.org Reporter: nick@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com When testing a file transport, we change directory four times: once to undef once to incoming again to undef once to outgoing When these fail for the incoming/outgoing, the errors look like: Change directory passed Change directory failed Change directory passed Change directory failed Which is unclear at best. Also, when updating a transport, the status before tests complete is: Never used Which is confusing to users who have jobs (e.g. EDI) that use these transports regularly -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 --- Comment #1 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 199352 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=199352&action=edit Bug 42656: Improve file transport test display This patch displays the directory tested in the background jobs results It updates the language when test results are not stored for the transport and links to the job when results are stored To test: 1 - Setup an ftp file transport with good credentials 2 - Enter non-existent directories in incoming/outgoing 3 - Save 4 - Verify on save 'Never used' shows for status 5 - Refresh the page, status now: Connection ok Change directory ok Change directory failed Change directory ok Change directory failed 6 - Apply patch, restart all 7 - Edit/save the transport again, change nothing 8 - Confirm the page says 'No test results stored' 9 - Reload the page 10 - Confirm results are now: Connection ok Change directory ok Change directory failed Change directory ok Change directory failed Test job details 11 - Click on the job details link 12 - Confirm actual directory and errors are shown 13 - Edit the transport, use existing directories 14 - Save, reload 15 - Confirm the success results and link t background job -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 --- Comment #2 from John Vinke <jvinke@austinseminary.edu> --- Created attachment 201837 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201837&action=edit Bug 42656: Improve file transport test display This patch displays the directory tested in the background jobs results It updates the language when test results are not stored for the transport and links to the job when results are stored To test: 1 - Setup an ftp file transport with good credentials 2 - Enter non-existent directories in incoming/outgoing 3 - Save 4 - Verify on save 'Never used' shows for status 5 - Refresh the page, status now: Connection ok Change directory ok Change directory failed Change directory ok Change directory failed 6 - Apply patch, restart all 7 - Edit/save the transport again, change nothing 8 - Confirm the page says 'No test results stored' 9 - Reload the page 10 - Confirm results are now: Connection ok Change directory ok Change directory failed Change directory ok Change directory failed Test job details 11 - Click on the job details link 12 - Confirm actual directory and errors are shown 13 - Edit the transport, use existing directories 14 - Save, reload 15 - Confirm the success results and link t background job Signed-off-by: John Vinke <jvinke@austinseminary.edu> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@openfifth.c | |o.uk QA Contact|testopia@bugs.koha-communit |martin.renvoize@openfifth.c |y.org |o.uk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |lucas@bywatersolutions.com --- Comment #4 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- 1. The code between the SWITCH and the first CASE will never execute in Template Toolkit. 2. I believe you need to add a space in between these: <span>Upload</span>[% status | html %] or else you'll get a string like 'Uploadfailed' -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |40811 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40811 [Bug 40811] Enhance Koha::File::Transport API -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|40811 |43092, 43088 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40811 [Bug 40811] Enhance Koha::File::Transport API https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 [Bug 43088] Koha::File::Transport lacks a consistent current_directory() accessor across FTP/SFTP/Local https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43092 [Bug 43092] Koha::File::Transport::change_directory(undef) means three different things across FTP, SFTP and Local -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #201837|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 --- Comment #5 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 202021 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202021&action=edit Bug 42656: Improve file transport test display This patch displays the directory tested in the background jobs results It updates the language when test results are not stored for the transport and links to the job when results are stored To test: 1 - Setup an ftp file transport with good credentials 2 - Enter non-existent directories in incoming/outgoing 3 - Save 4 - Verify on save 'Never used' shows for status 5 - Refresh the page, status now: Connection ok Change directory ok Change directory failed Change directory ok Change directory failed 6 - Apply patch, restart all 7 - Edit/save the transport again, change nothing 8 - Confirm the page says 'No test results stored' 9 - Reload the page 10 - Confirm results are now: Connection ok Change directory ok Change directory failed Change directory ok Change directory failed Test job details 11 - Click on the job details link 12 - Confirm actual directory and errors are shown 13 - Edit the transport, use existing directories 14 - Save, reload 15 - Confirm the success results and link t background job Signed-off-by: John Vinke <jvinke@austinseminary.edu> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 --- Comment #6 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 202022 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202022&action=edit Bug 42656: (QA follow-up) Fix dead status code in file_transport_test.inc Two issues in the operation_description BLOCK's status text, both flagged in review: 1) The IF/SET that computed the " passed."/" failed." status text sat between the SWITCH statement and its first CASE. Template Toolkit only executes CASE-labelled content inside a SWITCH block, so this was dead code - status was never actually set, and every operation row silently rendered with no passed/failed text at all. It also checked operation.error, a field that doesn't exist on the operation hash (the error message lives under operation.detail.error, and whether the operation failed is operation.status eq 'error'); even if the code path were reachable it would always have taken the "passed" branch. 2) Relying on a leading space baked into the " passed."/" failed." string literals to separate them from the preceding </span> is fragile. Made the space explicit in the markup instead, and dropped the leading space from the string values. Moves the status computation above the SWITCH (using operation.status), and adds an explicit space before [% status | html %] on every CASE line. Also adds t/db_dependent/Koha/BackgroundJob/TestTransport.t, which was entirely missing: it exercises process() end-to-end (enqueue, then process, then reload both the job and the transport) and confirms the persisted trace records every operation in sequence - successes before the failure that ended the run - rather than collapsing to just the final error. Test plan: 1. prove t/db_dependent/Koha/BackgroundJob/TestTransport.t => All tests successful, confirming process() persists the full operation sequence (order preserved, at least one success and one error present) on both the job and the transport's own status column. 2. Manually rendered the fixed operation_description BLOCK with Template Toolkit against mock operations (mixed success/error) and confirmed every row now shows "Connection passed.", "Change directory (/incoming) passed.", ..., "Change directory (/outgoing) failed.- error:No such directory" - status text present with correct spacing, where before the fix no status text rendered at all. 3. prove -r t/db_dependent/Koha/File/ t/db_dependent/Koha/Edifact/Transport.t t/db_dependent/api/v1/file_transports.t => All tests successful (47 tests total), no regressions. 4. koha-qa.pl -v 2 --more-tests => [PASS], no critic/pod warnings. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 --- Comment #7 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 202023 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202023&action=edit Bug 42656: (QA follow-up) Make error status persistence consistent, with a full trace, across FTP, SFTP and Local Two related inconsistencies remained across the three transport backends' error-handling paths: 1) Only Koha::File::Transport::SFTP's _abort_operation() persisted an "errors" status snapshot to the status column on every failed operation. FTP never did, and Local had no _abort_operation() at all - so an SFTP transport's displayed status could reflect a single transient failure from real usage (e.g. an EDI cron job) while the exact same failure on FTP/Local left the stored status untouched (stale, possibly still showing the last explicit test_connection() result). This is exactly the kind of unclear status reporting bug 42656 is about. 2) FTP's _abort_operation() carried its extra detail under an operation_params key, while SFTP (and Local's plain add_message calls) used path. The change_directory() success payload had the same split: FTP used directory, SFTP and Local used path. The admin template's change_directory row display depended on FTP's mismatched key names, which meant it silently failed to show the tested directory for SFTP transports. Adds a shared _record_error($operation, \%payload) to the Koha::File::Transport base class - add_message() plus status persistence - and routes all three backends' _abort_operation() (Local gains one) through it. Renames FTP's operation_params/directory keys to path throughout, matching SFTP and Local, and simplifies the template to read operation.detail.path unconditionally instead of ORing two differently-named keys. _record_error() persists the full trace of operations recorded on the transport object so far this session (every success leading up to the failure), not just the failing operation in isolation - the same "full trace" shape TestTransport::process() already builds for an explicit test_connection() run. A long-lived transport used across a multi-step operation (e.g. an EDI download loop touching several files before one fails) will therefore show every successful step leading up to the failure. It also logs the failure - including the built trace - via Koha::Logger at warn level, so it's visible in the Koha logs without needing to query the transport's status column. Test plan: 1. prove -r t/db_dependent/Koha/File/ t/db_dependent/Koha/BackgroundJob/TestTransport.t t/db_dependent/Koha/Edifact/Transport.t t/db_dependent/api/v1/file_transports.t => All tests successful (48 tests total), including: - new "_abort_operation persists status consistently" subtests in FTP.t, SFTP.t and Local.t that reload the transport from the DB after a simulated failure and confirm the status column was updated with the correct operation code and path detail; - a new "_record_error() tests" subtest in Transport.t proving the persisted trace includes a prior success message plus the failure (not just the failure alone), and that a warn-level log line mentioning the operation and error was emitted. 2. koha-qa.pl -v 2 --more-tests => [PASS], no critic/pod warnings. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #8 from David Nind <david@davidnind.com> --- Would like to test, so that this can go through the QA process with all the dependent and related bugs. However, I'm not able to test the s/ftp file transport part in the first patch. I've created an issue for KTD to look at having this as an option when starting up KTD https://gitlab.com/koha-community/koha-testing-docker/-/work_items/556 (or even some updated instructions would help). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43146 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43146 [Bug 43146] Add Koha::Health::SFTP health check -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|43088 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 [Bug 43088] Koha::File::Transport lacks a consistent current_directory() accessor across FTP/SFTP/Local -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off Patch complexity|Trivial patch |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #202021|0 |1 is obsolete| | Attachment #202022|0 |1 is obsolete| | Attachment #202023|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 --- Comment #9 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 202925 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202925&action=edit Bug 42656: Improve file transport test display This patch displays the directory tested in the background jobs results It updates the language when test results are not stored for the transport and links to the job when results are stored To test: 1 - Setup an ftp file transport with good credentials 2 - Enter non-existent directories in incoming/outgoing 3 - Save 4 - Verify on save 'Never used' shows for status 5 - Refresh the page, status now: Connection ok Change directory ok Change directory failed Change directory ok Change directory failed 6 - Apply patch, restart all 7 - Edit/save the transport again, change nothing 8 - Confirm the page says 'No test results stored' 9 - Reload the page 10 - Confirm results are now: Connection ok Change directory ok Change directory failed Change directory ok Change directory failed Test job details 11 - Click on the job details link 12 - Confirm actual directory and errors are shown 13 - Edit the transport, use existing directories 14 - Save, reload 15 - Confirm the success results and link t background job Signed-off-by: John Vinke <jvinke@austinseminary.edu> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 --- Comment #10 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 202926 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202926&action=edit Bug 42656: (QA follow-up) Fix dead status code in file_transport_test.inc Two issues in the operation_description BLOCK's status text, both flagged in review: 1) The IF/SET that computed the " passed."/" failed." status text sat between the SWITCH statement and its first CASE. Template Toolkit only executes CASE-labelled content inside a SWITCH block, so this was dead code - status was never actually set, and every operation row silently rendered with no passed/failed text at all. It also checked operation.error, a field that doesn't exist on the operation hash (the error message lives under operation.detail.error, and whether the operation failed is operation.status eq 'error'); even if the code path were reachable it would always have taken the "passed" branch. 2) Relying on a leading space baked into the " passed."/" failed." string literals to separate them from the preceding </span> is fragile. Made the space explicit in the markup instead, and dropped the leading space from the string values. Moves the status computation above the SWITCH (using operation.status), and adds an explicit space before [% status | html %] on every CASE line. Also adds t/db_dependent/Koha/BackgroundJob/TestTransport.t, which was entirely missing: it exercises process() end-to-end (enqueue, then process, then reload both the job and the transport) and confirms the persisted trace records every operation in sequence - successes before the failure that ended the run - rather than collapsing to just the final error. Test plan: 1. prove t/db_dependent/Koha/BackgroundJob/TestTransport.t => All tests successful, confirming process() persists the full operation sequence (order preserved, at least one success and one error present) on both the job and the transport's own status column. 2. Manually rendered the fixed operation_description BLOCK with Template Toolkit against mock operations (mixed success/error) and confirmed every row now shows "Connection passed.", "Change directory (/incoming) passed.", ..., "Change directory (/outgoing) failed.- error:No such directory" - status text present with correct spacing, where before the fix no status text rendered at all. 3. prove -r t/db_dependent/Koha/File/ t/db_dependent/Koha/Edifact/Transport.t t/db_dependent/api/v1/file_transports.t => All tests successful (47 tests total), no regressions. 4. koha-qa.pl -v 2 --more-tests => [PASS], no critic/pod warnings. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 --- Comment #11 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 202927 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202927&action=edit Bug 42656: (QA follow-up) Make error status persistence consistent, with a full trace, across FTP, SFTP and Local Two related inconsistencies remained across the three transport backends' error-handling paths: 1) Only Koha::File::Transport::SFTP's _abort_operation() persisted an "errors" status snapshot to the status column on every failed operation. FTP never did, and Local had no _abort_operation() at all - so an SFTP transport's displayed status could reflect a single transient failure from real usage (e.g. an EDI cron job) while the exact same failure on FTP/Local left the stored status untouched (stale, possibly still showing the last explicit test_connection() result). This is exactly the kind of unclear status reporting bug 42656 is about. 2) FTP's _abort_operation() carried its extra detail under an operation_params key, while SFTP (and Local's plain add_message calls) used path. The change_directory() success payload had the same split: FTP used directory, SFTP and Local used path. The admin template's change_directory row display depended on FTP's mismatched key names, which meant it silently failed to show the tested directory for SFTP transports. Adds a shared _record_error($operation, \%payload) to the Koha::File::Transport base class - add_message() plus status persistence - and routes all three backends' _abort_operation() (Local gains one) through it. Renames FTP's operation_params/directory keys to path throughout, matching SFTP and Local, and simplifies the template to read operation.detail.path unconditionally instead of ORing two differently-named keys. _record_error() persists the full trace of operations recorded on the transport object so far this session (every success leading up to the failure), not just the failing operation in isolation - the same "full trace" shape TestTransport::process() already builds for an explicit test_connection() run. A long-lived transport used across a multi-step operation (e.g. an EDI download loop touching several files before one fails) will therefore show every successful step leading up to the failure. It also logs the failure - including the built trace - via Koha::Logger at warn level, so it's visible in the Koha logs without needing to query the transport's status column. Test plan: 1. prove -r t/db_dependent/Koha/File/ t/db_dependent/Koha/BackgroundJob/TestTransport.t t/db_dependent/Koha/Edifact/Transport.t t/db_dependent/api/v1/file_transports.t => All tests successful (48 tests total), including: - new "_abort_operation persists status consistently" subtests in FTP.t, SFTP.t and Local.t that reload the transport from the DB after a simulated failure and confirm the status column was updated with the correct operation code and path detail; - a new "_record_error() tests" subtest in Transport.t proving the persisted trace includes a prior success message plus the failure (not just the failure alone), and that a warn-level log line mentioning the operation and error was emitted. 2. koha-qa.pl -v 2 --more-tests => [PASS], no critic/pod warnings. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org