diff --git a/Changes b/Changes index 86cac4124..f34ef4bba 100644 --- a/Changes +++ b/Changes @@ -1,12 +1,17 @@ LIST OF CHANGES FOR NPG-QC PACKAGE - - Previously the code allowed for an empty applicability_criteria hash, which - resulted in a particular set of QC criteria being applied to every and any - product. Very early on this was an intended behaviour for UKB data. The main - filter was the study id. The default section of the product configuration - file does not have an external filter, so there is a real danger of the - review check being run indiscriminately for any product. While this will - never be an intention, small errors in the YML file might have this effect. + - npg_qc::autoqc::check::review: + 1. Previously the code allowed for an empty applicability_criteria hash, + which resulted in a particular set of QC criteria being applied to every + and any product. Very early on this was an intended behaviour for UKB + data. The main filter was the study id. The default section of the product + configuration file does not have an external filter, so there is a real + danger of the review check being run indiscriminately for any product. + While this will never be an intention, small errors in the YML file might + have this effect. + 2. Ability to set UQC outcomes was introduced for the Heron project. This + functionality was never used and is unlikely to be used in future, it is + now removed. release 72.2.1 (2024-10-04) - Added .github/dependabot.yml file to auto-update GitHub actions diff --git a/MANIFEST b/MANIFEST index 7951deeb2..dcb933207 100644 --- a/MANIFEST +++ b/MANIFEST @@ -523,7 +523,6 @@ t/data/autoqc/review/mqc_type/product_release.yml t/data/autoqc/review/lims_applicability_empty/product_release.yml t/data/autoqc/review/no_known_applicability_type/product_release.yml t/data/autoqc/review/unknown_qc_type/product_release.yml -t/data/autoqc/review/uqc_type/product_release.yml t/data/autoqc/review/default_and_study_section/product_release.yml t/data/autoqc/review/default_section/product_release.yml t/data/autoqc/review/wrong_default_and_study_section/product_release.yml diff --git a/lib/npg_qc/autoqc/checks/review.pm b/lib/npg_qc/autoqc/checks/review.pm index 85eb3bc5e..74ef255be 100644 --- a/lib/npg_qc/autoqc/checks/review.pm +++ b/lib/npg_qc/autoqc/checks/review.pm @@ -35,7 +35,7 @@ Readonly::Array my @APPLICABILITY_CRITERIA_TYPES => ( Readonly::Scalar my $ACCEPTANCE_CRITERIA_KEY => q[acceptance_criteria]; Readonly::Scalar my $QC_TYPE_DEFAULT => q[mqc]; -Readonly::Array my @VALID_QC_TYPES => ($QC_TYPE_DEFAULT, q[uqc]); +Readonly::Array my @VALID_QC_TYPES => ($QC_TYPE_DEFAULT); Readonly::Scalar my $TIMESTAMP_FORMAT_WOFFSET => q[%Y-%m-%dT%T%z]; @@ -106,11 +106,6 @@ marked as 'Preliminary' (examples: 'Accepted Final', 'Rejected Preliminary'). By default the final_qc_outcome flag is false and the produced outcomes are preliminary. -A valid User QC outcome is one of the values from the -uqc_outcome_dict table of the npg_qc database. A concept of -the finality and, hence, immutability of the outcome is not -applicable to user QC outcome. - The type of QC outcome can be configured within the Robo QC section of product configuration. The default type is library Manual QC. @@ -332,8 +327,9 @@ sub execute { } $self->result->criteria($self->_criteria); - my $md5 = $self->result->generate_checksum4data($self->result->criteria); - $self->result->criteria_md5($md5); + $self->result->criteria_md5( + $self->result->generate_checksum4data($self->result->criteria) + ); my $err; try { @@ -344,7 +340,7 @@ sub execute { $self->result->add_comment($err); }; not $err and $self->result->qc_outcome( - $self->generate_qc_outcome($self->_outcome_type(), $md5)); + $self->generate_qc_outcome($self->_outcome_type())); return; } @@ -371,13 +367,12 @@ sub evaluate { Returns a hash reference representing the QC outcome. - my $u_outcome = $r->generate_qc_outcome('uqc', $md5); my $m_outcome = $r->generate_qc_outcome('mqc'); - + =cut sub generate_qc_outcome { - my ($self, $outcome_type, $md5) = @_; + my ($self, $outcome_type) = @_; $outcome_type or croak 'outcome type should be defined'; @@ -385,20 +380,13 @@ sub generate_qc_outcome { my $pass = $self->result->pass; ##### # Any of Accepted, Rejected, Undecided outcomes can be returned here - my $outcome = ($outcome_type eq $QC_TYPE_DEFAULT) - ? $package_name->generate_short_description( - $self->final_qc_outcome ? 1 : 0, $pass) - : $package_name->generate_short_description_prefix($pass); + my $outcome = $package_name->generate_short_description( + $self->final_qc_outcome ? 1 : 0, $pass); $outcome_type .= '_outcome'; my $outcome_info = { $outcome_type => $outcome, timestamp => create_current_timestamp(), username => $ROBO_KEY}; - if ($outcome_type =~ /\Auqc/xms) { - my @r = ($ROBO_KEY, $VERSION); - $md5 and push @r, $md5; - $outcome_info->{'rationale'} = join q[ ], @r; - } return $outcome_info; } diff --git a/t/50-schema-result-Review.t b/t/50-schema-result-Review.t index 789bedca5..f02c7628d 100644 --- a/t/50-schema-result-Review.t +++ b/t/50-schema-result-Review.t @@ -1,6 +1,6 @@ use strict; use warnings; -use Test::More tests => 6; +use Test::More tests => 5; use Test::Exception; use Moose::Meta::Class; use Digest::MD5 qw/md5_hex/; @@ -306,135 +306,17 @@ subtest 'a full insert/update record with mqc outcome' => sub { is ($rs->next->description, 'Accepted final', 'outcome has not changed'); }; -subtest 'a full insert/update record with uqc outcome' => sub { - plan tests => 36; - - my $initial_num_records = $schema->resultset($table)->search({})->count(); - - my $uqc_table = 'UqcOutcomeEnt'; - my $id_seq_composition = t::autoqc_util::find_or_save_composition( - $schema, {'id_run' => 1111, - 'position' => 1, - 'tag_index' => 33}); - - my $uqc_rs = $schema->resultset($uqc_table)->search({id_seq_composition => $id_seq_composition}); - is ($uqc_rs->count, 0, 'no uqc records for this entity'); - my $rs = $schema->resultset($table)->search({id_seq_composition => $id_seq_composition}); - is ($rs->count, 0, 'no review records for this entity'); - - my $qc_outcome = {"uqc_outcome" => "Rejected", - "timestamp" => "2018-06-03T12:53:46+0000", - "username" => "robo_qc", - "rationale" => "testrobo"}; - my $values = { - id_seq_composition => $id_seq_composition, - library_type => 'common_type', - evaluation_results => {"e1"=>1,"e2"=>0}, - criteria => {"and"=>["e1","e2"]}, - qc_outcome => $qc_outcome, - pass => 0, - path => 't/data' - }; - my $cmd5 = md5_hex(JSON::XS->new()->canonical(1)->encode($values->{criteria})); - - isa_ok($schema->resultset($table)->create($values), 'npg_qc::Schema::Result::' . $table); - is ($schema->resultset($table)->search({})->count(), $initial_num_records + 1, - 'total number of records in teh review table went up by one'); - $rs = $schema->resultset($table)->search({id_seq_composition => $id_seq_composition}); - is ($rs->count, 1, 'one row created in the review table'); - my $row = $rs->next; - is_deeply ($row->qc_outcome, $qc_outcome, 'qc outcome saved'); - is_deeply ($row->evaluation_results, {"e1"=>1,"e2"=>0}, 'evaluation results saved'); - is_deeply ($row->criteria, {"and"=>["e1","e2"]}, 'criteria saved'); - is ($row->criteria_md5, $cmd5, 'checksum saved'); - $uqc_rs = $schema->resultset($uqc_table)->search({id_seq_composition => $id_seq_composition}); - is ($uqc_rs->count, 1, 'one row created in the uqc table'); - my $outcome = $uqc_rs->next; - is ($outcome->rationale, 'testrobo', 'rationale recorded'); - is ($outcome->description, 'Rejected', 'correct uqc outcome'); - is ($outcome->modified_by, $ENV{USER}, 'correct user'); - is ($outcome->username, 'robo_qc', 'correct user'); - my $dt = $outcome->last_modified(); - is ($dt->year, 2018, 'correct year'); - is ($dt->month, '6', 'correct month'); - is ($dt->minute, '53', 'correct minute'); - is ($dt->second, '46', 'correct second'); - - $qc_outcome = {"uqc_outcome" => "Accepted", - "timestamp" => "2018-08-05T12:53:46+0000", - "username" => "robo_qc", - "rationale" => "testrobo1"}; - $values = { - id_seq_composition => $id_seq_composition, - library_type => 'common_type', - evaluation_results => {"e1"=>1,"e2"=>1}, - criteria => {"and"=>["e1","e2"]}, - qc_outcome => $qc_outcome, - }; - $row->update($values); - is_deeply ($row->evaluation_results, {"e1"=>1,"e2"=>1}, 'evaluation results updated'); - is_deeply ($row->qc_outcome, $qc_outcome, 'qc outcome updated'); - $outcome = $schema->resultset($uqc_table)->search({id_seq_composition => $id_seq_composition}) - ->next; - is ($outcome->rationale, 'testrobo1', 'rationale updated'); - is ($outcome->description, 'Accepted', 'uqc outcome updated'); - is ($outcome->last_modified()->month, 8, 'month updated'); - is ($outcome->last_modified()->year, 2018, 'correct year'); - - $id_seq_composition = t::autoqc_util::find_or_save_composition( - $schema, {'id_run' => 1111, - 'position' => 1, - 'tag_index' => 34}); - $uqc_rs = $schema->resultset($uqc_table)->search({id_seq_composition => $id_seq_composition}); - is ($uqc_rs->count, 0, 'no uqc records for this entity'); - my $new_uqc = $schema->resultset($uqc_table) - ->new_result({id_seq_composition => $id_seq_composition}); - $new_uqc->update_outcome({uqc_outcome => 'Accepted', rationale => 'test1'}, 'user1', 'test'); - $uqc_rs = $schema->resultset($uqc_table)->search({id_seq_composition => $id_seq_composition}); - is ($uqc_rs->count, 1, 'record created'); - - $values = { - id_seq_composition => $id_seq_composition, - library_type => 'common_type', - evaluation_results => '{"e1":1,"e2":1}', - criteria => '{"and":["e1","e2"]}', - qc_outcome => {"uqc_outcome" => "Undecided", - "timestamp" => "2018-09-03T12:58:43+0000", - "username" => "robo_qc", - "rationale" => "testrobo2"}, - pass => undef - }; - my $created=$schema->resultset($table)->create($values); - - is ($schema->resultset($table)->search({})->count(), $initial_num_records + 2, - 'total number of records in teh review table went up by one'); - $uqc_rs = $schema->resultset($uqc_table)->search({id_seq_composition => $id_seq_composition}); - is ($uqc_rs->count, 1, 'one uqc record for the entity'); - $outcome = $uqc_rs->next; - is ($outcome->rationale, 'testrobo2', 'rationale updated'); - is ($outcome->description, 'Undecided', 'outcome updated'); - is ($outcome->modified_by, $ENV{USER}, 'correct user'); - is ($outcome->username, 'robo_qc', 'correct user'); - $dt = $outcome->last_modified(); - is ($dt->year, 2018, 'correct year'); - is ($dt->month, '9', 'correct month'); - is ($dt->minute, '58', 'correct minute'); - is ($dt->second, '43', 'correct second'); -}; - - subtest 'unknown outcome type should give an error' => sub { plan tests => 5; - my $uqc_table = 'UqcOutcomeEnt'; my $id_seq_composition = t::autoqc_util::find_or_save_composition( $schema, {'id_run' => 1111, 'position' => 1, 'tag_index' => 43}); my $no_records = sub { - is ($schema->resultset($uqc_table)->search({id_seq_composition => $id_seq_composition})->count, - 0, 'no uqc records for this entity'); + is ($schema->resultset($mqc_table)->search({id_seq_composition => $id_seq_composition})->count, + 0, 'no mqc records for this entity'); is ($schema->resultset($table)->search({id_seq_composition => $id_seq_composition})->count, 0, 'no review records for this entity'); }; diff --git a/t/60-autoqc-checks-review.t b/t/60-autoqc-checks-review.t index 968dd2a23..3395d6b4f 100644 --- a/t/60-autoqc-checks-review.t +++ b/t/60-autoqc-checks-review.t @@ -417,7 +417,7 @@ subtest 'single expression evaluation' => sub { }; subtest 'evaluation within the execute method' => sub { - plan tests => 48; + plan tests => 40; local $ENV{NPG_CACHED_SAMPLESHEET_FILE} = 't/data/autoqc/review/samplesheet_29524.csv'; @@ -436,7 +436,6 @@ subtest 'evaluation within the execute method' => sub { is ($o->result->pass, undef, 'result pass attribute is unset'); my @check_objects = (); - push @check_objects, npg_qc::autoqc::checks::review->new( runfolder_path => $rf_path, conf_path => $test_data_dir, @@ -446,12 +445,7 @@ subtest 'evaluation within the execute method' => sub { conf_path => "$test_data_dir/mqc_type", qc_in => $dir, rpt_list => $rpt_list); - push @check_objects, npg_qc::autoqc::checks::review->new( - conf_path => "$test_data_dir/uqc_type", - qc_in => $dir, - rpt_list => $rpt_list); - my $count = 0; 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'); @@ -461,15 +455,9 @@ subtest 'evaluation within the execute method' => sub { is_deeply ($check->result->evaluation_results(), \%expected, 'evaluation results are saved'); my $outcome = $check->result->qc_outcome; - if ($count < 2) { - is ($outcome->{'mqc_outcome'} , 'Accepted preliminary', 'correct outcome string'); - } elsif ($count == 2) { - is ($outcome->{'uqc_outcome'} , 'Accepted', 'correct outcome string'); - ok ($outcome->{'rationale'} , 'rationale is set'); - } + is ($outcome->{'mqc_outcome'} , 'Accepted preliminary', 'correct outcome string'); is ($outcome->{'username'}, 'robo_qc', 'correct process id'); ok ($outcome->{'timestamp'}, 'timestamp saved'); - $count++; } # Undefined library type should not be a problem. diff --git a/t/data/autoqc/review/uqc_type/product_release.yml b/t/data/autoqc/review/uqc_type/product_release.yml deleted file mode 100644 index cae9ff254..000000000 --- a/t/data/autoqc/review/uqc_type/product_release.yml +++ /dev/null @@ -1,38 +0,0 @@ ---- -default: - s3: - enable: false - url: null - notify: false - irods: - enable: true - notify: false - -study: - - study_id: "5392" - s3: - enable: true - url: "gs://profile_one-europe-west2" - date_binning: true - customer_name: "UK Organisation" - profile: "profile_one" - notify: true - receipts: "/data_product_receipts/5392/" - irods: - enable: false - notify: true - merge: - component_cache_dir: "/merge_component_cache/5392/" - robo_qc: - qc_type: "uqc" - criteria: - - applicability_criteria: - lims: - library_type: - - "HiSeqX PCR free" - acceptance_criteria : - - "( bam_flagstats.target_proper_pair_mapped_reads / bam_flagstats.target_mapped_reads ) > 0.95" - - "bam_flagstats.target_mapped_bases > 85_000_000_000" - - "bam_flagstats.target_percent_gt_coverage_threshold > 95" - - "verify_bam_id.freemix < 0.01" - - "( bcfstats.genotypes_nrd_dividend / bcfstats.genotypes_nrd_divisor ) < 0.02"