From 7b828bb1c5478a1f634ceefc6753a591c2048ab7 Mon Sep 17 00:00:00 2001
From: Marina Gourtovaia <mg8@sanger.ac.uk>
Date: Thu, 8 Aug 2024 22:08:32 +0100
Subject: [PATCH] Added ROBO criteria validation to the can_run method

---
 Changes                            |  2 ++
 lib/npg_qc/autoqc/checks/review.pm | 48 +++++++++++++++++++-----------
 t/60-autoqc-checks-review.t        |  9 +++---
 3 files changed, 38 insertions(+), 21 deletions(-)

diff --git a/Changes b/Changes
index 947aeef6..d0d7b7af 100644
--- a/Changes
+++ b/Changes
@@ -5,6 +5,8 @@ LIST OF CHANGES FOR NPG-QC PACKAGE
       {r,R}unParameters.xml), added an optional 'runfolder_path' attribute.
       The attribute will have to be set by the caller.
    2. Added a new applicability criteria type - 'sequencing_run'
+   3. To avoid running the reviewcheck unnecessary in the pipeline context,
+      moved ROBO criteria validation to the can_run method.
 
 release 72.1.2 (2024-08-05)
  - handle cases where the same tag sequence is seen in both index reads
diff --git a/lib/npg_qc/autoqc/checks/review.pm b/lib/npg_qc/autoqc/checks/review.pm
index 08429cdd..193f44b4 100644
--- a/lib/npg_qc/autoqc/checks/review.pm
+++ b/lib/npg_qc/autoqc/checks/review.pm
@@ -267,10 +267,22 @@ for this product exists.
 
 sub can_run {
   my $self = shift;
-  (keys %{$self->_robo_config()}) and return 1;
-  $self->result->add_comment(
-    'Product configuration for RoboQC is absent');
-  return 0;
+  if (!keys %{$self->_robo_config}) {
+    $self->result->add_comment('Product configuration for RoboQC is absent');
+    return 0;
+  }
+  my $num_criteria;
+  try {
+    $num_criteria = @{$self->_applicable_criteria};
+  } catch {
+    $self->result->add_comment("Error validating RoboQC criteria: $_");
+  };
+  if (!$num_criteria) {
+    $self->result->add_comment('None of the applicability criteria is satisfied');
+    return 0;
+  }
+
+  return 1;
 }
 
 =head2 execute
@@ -476,9 +488,15 @@ sub _validate_criteria {
   return;
 }
 
-sub _applicability_all {
-  my ($self, $criteria_objs) = @_;
+has '_applicable_criteria' => (
+  isa        => 'ArrayRef',
+  is         => 'ro',
+  lazy_build => 1,
+);
+sub _build__applicable_criteria {
+  my $self = shift;
 
+  my $criteria_objs = $self->_robo_config->{$CRITERIA_KEY};
   my @applicable = ();
   foreach my $co ( @{$criteria_objs} ) {
     my $c_applicable = 1;
@@ -538,23 +556,19 @@ sub _build__criteria {
   $lib_type or croak 'Library type is not defined for ' .  $self->_entity_desc;
   $self->result->library_type($lib_type);
 
-  my $criteria_objs = $self->_robo_config->{$CRITERIA_KEY};
-  $criteria_objs = $self->_applicability_all($criteria_objs);
-  # Criteria objects with no lims criteria defined should be returned
-  # in the above array. An empty array means that all objects had lims
-  # criteria and none of them was satisfied.
-  @{$criteria_objs} or return {};
-
-  (@{$criteria_objs} == 1) or carp 'Multiple criteria sets are satisfied ' .
-    'in a robo config for ' . $self->_entity_desc;
-
+  my $num_criteria = scalar @{$self->_applicable_criteria};
+  if ($num_criteria == 0) {
+    return {};
+  } elsif ($num_criteria > 1) {
+    croak 'Multiple criteria sets are satisfied in a robo config';
+  }
   #####
   # A very simple criteria format - a list of strings - is used for now.
   # Each string represents a math expression. It is assumed that the
   # conjunction operator should be used to form the boolean expression
   # that should give the result of the evaluation.
   #
-  my @c = uniq sort @{$criteria_objs->[0]->{$ACCEPTANCE_CRITERIA_KEY}};
+  my @c = uniq sort @{$self->_applicable_criteria->[0]->{$ACCEPTANCE_CRITERIA_KEY}};
 
   return @c ? {$CONJUNCTION_OP => \@c} : {};
 }
diff --git a/t/60-autoqc-checks-review.t b/t/60-autoqc-checks-review.t
index 1c7d86f0..8d1d2ef4 100644
--- a/t/60-autoqc-checks-review.t
+++ b/t/60-autoqc-checks-review.t
@@ -156,8 +156,9 @@ subtest 'constructing object, deciding whether to run' => sub {
     conf_path => "$test_data_dir/with_na_criteria",
     qc_in     => $test_data_dir,
     rpt_list  => '27483:1:2');
-  ok ($check->can_run, 'can_run returns true');
-  ok (!$check->result->comments, 'No comments logged');
+  ok (!$check->can_run, 'can_run returns false');
+  is ($check->result->comments, 'None of the applicability criteria is satisfied',
+    'Comment logged');
 };
 
 subtest 'caching appropriate criteria object' => sub {
@@ -195,7 +196,7 @@ subtest 'execute when no criteria apply' => sub {
   lives_ok { $check->execute }
     'no error in execute when no criteria apply';
   my $result = $check->result;
-  is ($result->comments, 'RoboQC is not applicable',
+  is ($result->comments, 'None of the applicability criteria is satisfied',
     'correct comment logged');
   is_deeply ($result->criteria, {}, 'empty criteria hash');
   is_deeply ($result->qc_outcome, {}, 'empty qc_outcome hash');
@@ -413,7 +414,7 @@ subtest 'evaluation within the execute method' => sub {
     qc_in     => $dir,
     rpt_list  => $rpt_list
   );
-  ok ($o->can_run, 'the check can be run');
+  ok (!$o->can_run, 'the check cannot be run');
   is_deeply($o->_criteria, {}, 'no criteria to evaluate');
   lives_ok { $o->execute } 'execute method runs OK';
   is ($o->result->pass, undef, 'result pass attribute is unset');