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.
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This commit is contained in:
Martin Renvoize
Pedro Amorim
parent
7d6dafef6d
commit
f8cf2aea42
|
|
@ -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' -%]
|
||||
<span>Connection</span>[% status | html %]
|
||||
<span>Connection</span> [% status | html %]
|
||||
[%- CASE 'upload' -%]
|
||||
<span>Upload</span>[% status | html %]
|
||||
<span>Upload</span> [% status | html %]
|
||||
[%- CASE 'download' -%]
|
||||
<span>Download</span>[% status | html %]
|
||||
<span>Download</span> [% status | html %]
|
||||
[%- CASE 'list' -%]
|
||||
<span>List files</span>[% status | html %]
|
||||
<span>List files</span> [% status | html %]
|
||||
[%- CASE 'change_directory' -%]
|
||||
<span>Change directory ([% operation.detail.directory | html %][% operation.detail.operation_params | html %])</span>[% status | html %]
|
||||
<span>Change directory ([% operation.detail.directory | html %][% operation.detail.operation_params | html %])</span> [% status | html %]
|
||||
[%- CASE -%]
|
||||
<span>[% operation.code | html %]</span>[% status | html %]
|
||||
<span>[% operation.code | html %]</span> [% status | html %]
|
||||
[%- END -%]
|
||||
[%- END -%]
|
||||
|
||||
|
|
|
|||
|
|
@ -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 <https://www.gnu.org/licenses>.
|
||||
|
||||
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;
|
||||
Loading…
Reference in New Issue