From a68a605db4b1f97ea045cdd2c21dad9f7656c64f Mon Sep 17 00:00:00 2001 From: Marina Gourtovaia Date: Wed, 11 Sep 2024 10:11:38 +0100 Subject: [PATCH] Made library_type optional in the review check. --- Changes | 5 +++ lib/npg_qc/Schema/Result/Review.pm | 14 ++++----- lib/npg_qc/autoqc/checks/review.pm | 8 ++--- t/50-schema-result-Review.t | 49 ++++++++++++------------------ t/60-autoqc-checks-review.t | 16 +++++++++- 5 files changed, 50 insertions(+), 42 deletions(-) diff --git a/Changes b/Changes index 5ec4bf7a0..72100f65e 100644 --- a/Changes +++ b/Changes @@ -7,6 +7,11 @@ LIST OF CHANGES FOR NPG-QC PACKAGE the change originates from GitHub and can be trusted. Our CI flow compares the checksum of the downloaded script to the expected value. We now store an updated expeced checksum value, which corresponds to the latest release. + - npg_qc::autoqc::check::review: + The forthcoming lane-level evaluations are impossible if the code continues + to error when library_type attribute is not defined for the entity under + consideration. The library_type attribute is now set when possible, no error + if it is undefined. release 72.2.0 (2024-08-30) - npg_qc::autoqc::check::review: diff --git a/lib/npg_qc/Schema/Result/Review.pm b/lib/npg_qc/Schema/Result/Review.pm index 1aac0e566..7ce04be99 100644 --- a/lib/npg_qc/Schema/Result/Review.pm +++ b/lib/npg_qc/Schema/Result/Review.pm @@ -357,14 +357,12 @@ around [qw/update insert/] => sub { ##### # Do not accept half-baked results, ie if we have evaluation - # results, we should also have library type and criteria. + # results, we should also have criteria. if ($data->{'evaluation_results'} and keys %{$data->{'evaluation_results'}}) { - foreach my $name (qw/library_type criteria/) { - my $value = $data->{$name}; - my $m = "Evaluation results present, but $name absent"; - $value or croak $m; - ((not ref $value) or keys %{$value}) or croak $m; - } + my $value = $data->{'criteria'}; + my $m = 'Evaluation results present, but criteria absent'; + $value or croak $m; + ((not ref $value) or keys %{$value}) or croak $m; } ##### @@ -475,7 +473,7 @@ Marina Gourtovaia Emg8@sanger.ac.ukE =head1 LICENSE AND COPYRIGHT -Copyright (C) 2019,2020 Genome Research Ltd. +Copyright (C) 2019,2020, 2924 Genome Research Ltd. This file is part of NPG. diff --git a/lib/npg_qc/autoqc/checks/review.pm b/lib/npg_qc/autoqc/checks/review.pm index 08a242676..4c1972aba 100644 --- a/lib/npg_qc/autoqc/checks/review.pm +++ b/lib/npg_qc/autoqc/checks/review.pm @@ -568,11 +568,11 @@ has '_criteria' => ( sub _build__criteria { my $self = shift; - # Save redundant library_type. - # TODO: Save details about applicability instead. + # Library type might be undefined. Example - lane level object. my $lib_type = $self->lims->library_type; - $lib_type or croak 'Library type is not defined for ' . $self->_entity_desc; - $self->result->library_type($lib_type); + if ($lib_type) { + $self->result->library_type($lib_type); + } my $num_criteria = scalar @{$self->_applicable_criteria}; if ($num_criteria == 0) { diff --git a/t/50-schema-result-Review.t b/t/50-schema-result-Review.t index 6be7101c4..789bedca5 100644 --- a/t/50-schema-result-Review.t +++ b/t/50-schema-result-Review.t @@ -19,19 +19,9 @@ my $schema = Moose::Meta::Class->create_anon_class( ->new_object({})->create_test_db(q[npg_qc::Schema], 't/data/fixtures'); subtest 'reject incomplete results on insert' => sub { - plan tests => 4; + plan tests => 2; my $values = { - evaluation_results => {"e1"=>1,"e2"=>0}, - criteria => {"and"=>["e1","e2"]}, - pass => 0, - path => 't/data' - }; - throws_ok {$schema->resultset($table)->create($values)} - qr/Evaluation results present, but library_type absent/, - 'library type is not defined - error on insert'; - - $values = { library_type => 'common_type', criteria => {}, evaluation_results => {"e1"=>1,"e2"=>0}, @@ -41,18 +31,6 @@ subtest 'reject incomplete results on insert' => sub { qr/Evaluation results present, but criteria absent/, 'criteria are not defined - error on insert'; - $values = { - evaluation_results => {"e1"=>1,"e2"=>0}, - criteria => {}, - qc_outcome => - {"mqc_outcome"=>"Rejected final","timestamp"=>"2018-06-03T12:53:46+0000","username"=>"robo_qc"}, - pass => 0, - path => 't/data' - }; - throws_ok {$schema->resultset($table)->create($values)} - qr/Evaluation results present, but library_type absent/, - 'criteria and library type are not defined - error on insert'; - $values = { library_type => 'common_type', evaluation_results => {"e1"=>1,"e2"=>0}, @@ -68,13 +46,27 @@ subtest 'reject incomplete results on insert' => sub { }; subtest 'insert a basic record, do not allow incomplete data in update' => sub { - plan tests => 9; + plan tests => 10; my $id_seq_composition = t::autoqc_util::find_or_save_composition( $schema, {'id_run' => 1111, 'position' => 1, - 'tag_index' => 8}); + 'tag_index' => 7}); my $values = { + evaluation_results => {"e1"=>1,"e2"=>0}, + criteria => {"and"=>["e1","e2"]}, + pass => 0, + path => 't/data', + id_seq_composition => $id_seq_composition + }; + lives_ok {$schema->resultset($table)->create($values)} + 'library type is not defined - no error on insert'; + + $id_seq_composition = t::autoqc_util::find_or_save_composition( + $schema, {'id_run' => 1111, + 'position' => 1, + 'tag_index' => 8}); + $values = { evaluation_results => {}, criteria => {}, qc_outcome => {}, @@ -108,9 +100,8 @@ subtest 'insert a basic record, do not allow incomplete data in update' => sub { pass => 0, path => 't/data' }; - throws_ok {$new->update($values)} - qr/Evaluation results present, but library_type absent/, - 'library type is not defined - error on update'; + lives_ok {$new->update($values)} + 'library type is not defined - no error on update'; $values = { id_seq_composition => $id_seq_composition, @@ -277,7 +268,7 @@ subtest 'a full insert/update record with mqc outcome' => sub { }; my $created=$schema->resultset($table)->create($values); $rs = $schema->resultset($table)->search({}); - is ($rs->count, 2, q[two rows in the table]); + is ($rs->count, 3, q[three rows in the table]); $mqc_rs = $schema->resultset($mqc_table)->search({id_seq_composition => $id_seq_composition}); is ($mqc_rs->count, 1, 'one mqc record for the entity'); diff --git a/t/60-autoqc-checks-review.t b/t/60-autoqc-checks-review.t index 36b769a3c..b2f748534 100644 --- a/t/60-autoqc-checks-review.t +++ b/t/60-autoqc-checks-review.t @@ -401,7 +401,7 @@ subtest 'single expression evaluation' => sub { }; subtest 'evaluation within the execute method' => sub { - plan tests => 42; + plan tests => 48; local $ENV{NPG_CACHED_SAMPLESHEET_FILE} = 't/data/autoqc/review/samplesheet_29524.csv'; @@ -439,6 +439,8 @@ subtest 'evaluation within the execute method' => sub { foreach my $check (@check_objects) { lives_ok { $check->execute } 'execute method runs OK'; is ($check->result->pass, 1, 'result pass attribute is set to 1'); + is ($check->result->library_type, 'HiSeqX PCR free', + 'result library_type attribute is set correctly'); my %expected = map { $_ => 1 } @{$criteria_list}; is_deeply ($check->result->evaluation_results(), \%expected, 'evaluation results are saved'); @@ -454,7 +456,19 @@ subtest 'evaluation within the execute method' => sub { $count++; } + # Undefined library type should not be a problem. + my $lane_lims = (st::api::lims->new(id_run=> 29524)->children())[0]; + is ($lane_lims->library_type, undef, 'library type is undefined on lane level'); my $check = npg_qc::autoqc::checks::review->new( + conf_path => $test_data_dir, + qc_in => $test_data_dir, + rpt_list => $rpt_list, + lims => $lane_lims + ); + lives_ok { $check->execute } 'execute method runs OK'; + is ($check->result->library_type, undef, 'library_type attribute is unset'); + + $check = npg_qc::autoqc::checks::review->new( conf_path => "$test_data_dir/unknown_qc_type", qc_in => $dir, rpt_list => $rpt_list);