From f8cf2aea42378776bece39bb2ce9576f7ce64e5f Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Thu, 16 Jul 2026 16:34:23 +0100 Subject: [PATCH] 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 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. Signed-off-by: Pedro Amorim --- .../background_jobs/file_transport_test.inc | 15 ++- .../Koha/BackgroundJob/TestTransport.t | 126 ++++++++++++++++++ 2 files changed, 134 insertions(+), 7 deletions(-) create mode 100755 t/db_dependent/Koha/BackgroundJob/TestTransport.t diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/file_transport_test.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/file_transport_test.inc index 4ed7f7beaa..5ca42c1c87 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/file_transport_test.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/file_transport_test.inc @@ -1,21 +1,22 @@ [% USE Koha %] [%- BLOCK operation_description -%] + [%# Content between SWITCH and its first CASE never executes in Template Toolkit, so "status" must be computed here, before the SWITCH - and against operation.status (the field that actually holds 'error'/'success'), not the nonexistent operation.error. #%] + [%- SET status = operation.status == 'error' ? "failed." : "passed." -%] [%- SWITCH operation.code -%] - [%- IF operation.error %][% SET status = " failed. " %][% ELSE %][% SET status = " passed. " %][% END -%] [%- CASE 'connection' -%] - Connection[% status | html %] + Connection [% status | html %] [%- CASE 'upload' -%] - Upload[% status | html %] + Upload [% status | html %] [%- CASE 'download' -%] - Download[% status | html %] + Download [% status | html %] [%- CASE 'list' -%] - List files[% status | html %] + List files [% status | html %] [%- CASE 'change_directory' -%] - Change directory ([% operation.detail.directory | html %][% operation.detail.operation_params | html %])[% status | html %] + Change directory ([% operation.detail.directory | html %][% operation.detail.operation_params | html %]) [% status | html %] [%- CASE -%] - [% operation.code | html %][% status | html %] + [% operation.code | html %] [% status | html %] [%- END -%] [%- END -%] diff --git a/t/db_dependent/Koha/BackgroundJob/TestTransport.t b/t/db_dependent/Koha/BackgroundJob/TestTransport.t new file mode 100755 index 0000000000..b440ffbc50 --- /dev/null +++ b/t/db_dependent/Koha/BackgroundJob/TestTransport.t @@ -0,0 +1,126 @@ +#!/usr/bin/perl + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use Test::More tests => 2; +use Test::NoWarnings; +use Test::MockModule; +use JSON qw( decode_json ); +use File::Temp qw( tempdir ); +use File::Spec; + +use Koha::Database; +use Koha::BackgroundJobs; +use Koha::BackgroundJob::TestTransport; +use Koha::File::Transports; + +use t::lib::TestBuilder; + +my $schema = Koha::Database->new->schema; +my $builder = t::lib::TestBuilder->new; + +subtest 'process() records the full trace (successes and the failure), not only the last error' => sub { + plan tests => 8; + + $schema->storage->txn_begin; + + my $tempdir = tempdir( CLEANUP => 1 ); + my $download_dir = File::Spec->catdir( $tempdir, 'download' ); + my $upload_dir = File::Spec->catdir( $tempdir, 'upload' ); + mkdir $download_dir or die "Cannot create download_dir: $!"; + mkdir $upload_dir or die "Cannot create upload_dir: $!"; + + my $transport = $builder->build_object( + { + class => 'Koha::File::Transports', + value => { + transport => 'local', + download_directory => $download_dir, + upload_directory => $upload_dir, + } + } + ); + + # test_connection() calls list_files() once per configured direction + # (download, then upload). Simulate a real "some steps pass, one fails" + # run: the first call (download) succeeds, the second (upload) fails. + my $call_count = 0; + my $mock_local = Test::MockModule->new('Koha::File::Transport::Local'); + $mock_local->mock( + '_list_files', + sub { + my ($self) = @_; + $call_count++; + if ( $call_count == 1 ) { + $self->add_message( + { message => 'list', type => 'success', payload => { path => $download_dir, count => 0 } } ); + return []; + } else { + $self->add_message( + { + message => 'list', type => 'error', + payload => { error => 'Simulated failure', path => $upload_dir } + } + ); + return; + } + } + ); + + my $job_id = Koha::BackgroundJob::TestTransport->new->enqueue( { transport_id => $transport->id } ); + my $job = Koha::BackgroundJobs->find($job_id)->_derived_class; + + $job->process( { transport_id => $transport->id } ); + + is( $job->status, 'finished', 'job finished' ); + + my $data = $job->decoded_data; + is( $data->{status}, 'errors', 'overall job status is "errors" because one operation failed' ); + + my @codes = map { $_->{code} } @{ $data->{operations} }; + ok( + scalar(@codes) > 1, + 'more than one operation was recorded - the trace is not collapsed to just the final error' + ); + + my @statuses = map { $_->{status} } @{ $data->{operations} }; + ok( ( grep { $_ eq 'success' } @statuses ), 'at least one recorded operation is a success' ); + ok( ( grep { $_ eq 'error' } @statuses ), 'at least one recorded operation is an error' ); + + # Order matters for display: successes should appear before the failure + # they preceded, not just the failure on its own. + my ($first_error_index) = grep { $statuses[$_] eq 'error' } 0 .. $#statuses; + ok( + ( grep { $_ eq 'success' } @statuses[ 0 .. $first_error_index - 1 ] ), + 'a success is recorded before the failing operation, preserving the full sequence' + ); + + # The transport's own status column (read by the admin UI) should match + # what was persisted by TestTransport::process(), not something narrower. + my $reloaded = Koha::File::Transports->find( $transport->id ); + my $persisted_status = decode_json( $reloaded->status ); + is( $persisted_status->{status}, 'errors', q{the transport's own status column matches the job's} ); + is( + scalar @{ $persisted_status->{operations} }, scalar @codes, + q{the transport's own status column carries the same full trace, not a single collapsed entry} + ); + + $schema->storage->txn_rollback; +}; + +1;