From 5bb2297be091e7f38508e80ff1dde3ecfdb02528 Mon Sep 17 00:00:00 2001
From: Marina Gourtovaia <mg8@sanger.ac.uk>
Date: Fri, 9 Aug 2024 10:28:14 +0100
Subject: [PATCH] Consistent logging of error messages in can_run

---
 Changes                            |  2 +-
 lib/npg_qc/autoqc/checks/review.pm | 40 +++++++++++++++++++-----------
 t/60-autoqc-checks-review.t        | 16 ++++++------
 3 files changed, 35 insertions(+), 23 deletions(-)

diff --git a/Changes b/Changes
index d0d7b7af..c6cee83e 100644
--- a/Changes
+++ b/Changes
@@ -5,7 +5,7 @@ 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,
+   3. To avoid running the review check unnecessarily in the pipeline context,
       moved ROBO criteria validation to the can_run method.
 
 release 72.1.2 (2024-08-05)
diff --git a/lib/npg_qc/autoqc/checks/review.pm b/lib/npg_qc/autoqc/checks/review.pm
index 193f44b4..aadab26f 100644
--- a/lib/npg_qc/autoqc/checks/review.pm
+++ b/lib/npg_qc/autoqc/checks/review.pm
@@ -260,29 +260,41 @@ sub BUILD {
 
 =head2 can_run
 
-Returns true if the check can be run, meaning a robo configuration
-for this product exists.
+Returns true if the check can be run, ie a valid RoboQC configuration exists
+and one of the applicability criteria is satisfied for this product.
 
 =cut
 
 sub can_run {
   my $self = shift;
+
+  my $can_run = 1;
+  my $message;
+
   if (!keys %{$self->_robo_config}) {
-    $self->result->add_comment('Product configuration for RoboQC is absent');
-    return 0;
+    $message = 'RoboQC configuration is absent';
+    $can_run = 0;
+  } else {
+    my $num_criteria;
+    try {
+      $num_criteria = @{$self->_applicable_criteria};
+    } catch {
+      $message = "Error validating RoboQC criteria: $_";
+      $can_run = 0;
+    };
+    if ($can_run && !$num_criteria) {
+      $message = 'None of the RoboQC applicability criteria is satisfied';
+      $can_run = 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;
+
+  if (!$can_run && $message) {
+    $self->result->add_comment($message);
+    carp sprintf 'Review check cannot be run for %s . Reason: %s',
+      $self->_entity_desc, $message;
   }
 
-  return 1;
+  return $can_run;
 }
 
 =head2 execute
diff --git a/t/60-autoqc-checks-review.t b/t/60-autoqc-checks-review.t
index 8d1d2ef4..77279219 100644
--- a/t/60-autoqc-checks-review.t
+++ b/t/60-autoqc-checks-review.t
@@ -58,11 +58,10 @@ subtest 'constructing object, deciding whether to run' => sub {
 
   my $can_run;
   warnings_like { $can_run = $check->can_run }
-    [qr/Study config not found for/],
+    [qr/Study config not found/, qr/RoboQC configuration is absent/],
     'can_run is accompanied by warnings';
   ok (!$can_run, 'can_run returns false - no study config');
-  is ($check->result->comments,
-    'Product configuration for RoboQC is absent',
+  like ($check->result->comments, qr/RoboQC configuration is absent/,
     'reason logged');
   lives_ok { $check->execute() } 'cannot run, but execute method runs OK';
   is ($check->result->pass, undef,
@@ -75,11 +74,10 @@ subtest 'constructing object, deciding whether to run' => sub {
     qc_in     => $test_data_dir,
     rpt_list  => '27483:1:2');
   warnings_like { $can_run = $check->can_run }
-    [qr/robo_qc section is not present for/],
+    [qr/robo_qc section is not present for/, qr/Review check cannot be run/],
     'can_run is accompanied by warnings';
   ok (!$can_run, 'can_run returns false - no robo config');
-  is ($check->result->comments,
-    'Product configuration for RoboQC is absent',
+  is ($check->result->comments, 'RoboQC configuration is absent',
     'reason logged');
 
   $check = npg_qc::autoqc::checks::review->new(
@@ -157,7 +155,8 @@ subtest 'constructing object, deciding whether to run' => sub {
     qc_in     => $test_data_dir,
     rpt_list  => '27483:1:2');
   ok (!$check->can_run, 'can_run returns false');
-  is ($check->result->comments, 'None of the applicability criteria is satisfied',
+  is ($check->result->comments,
+    'None of the RoboQC applicability criteria is satisfied',
     'Comment logged');
 };
 
@@ -196,7 +195,8 @@ 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, 'None of the applicability criteria is satisfied',
+  is ($result->comments,
+    'None of the RoboQC applicability criteria is satisfied',
     'correct comment logged');
   is_deeply ($result->criteria, {}, 'empty criteria hash');
   is_deeply ($result->qc_outcome, {}, 'empty qc_outcome hash');