From d02f1d8fb338e4dd1b401892b3408a65cad3b4d4 Mon Sep 17 00:00:00 2001
From: Mark Wells <mark@freeside.biz>
Date: Thu, 9 Jun 2016 15:56:36 -0700
Subject: [PATCH] remove "failed billing events" report, and add "completed
 with an error" status search, #42637

---
 FS/FS/cust_event.pm                      | 40 +++++++++++++++++++++++++-------
 httemplate/elements/menu.html            |  3 +--
 httemplate/search/report_cust_event.html | 10 ++++----
 3 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/FS/FS/cust_event.pm b/FS/FS/cust_event.pm
index 93743c447..094c4fa8b 100644
--- a/FS/FS/cust_event.pm
+++ b/FS/FS/cust_event.pm
@@ -245,7 +245,13 @@ sub do_event {
     $statustext = "Error running ". $part_event->action. " action: $@";
   } elsif ( $error ) {
     $status = 'done';
-    $statustext = $error;
+    if ( $error eq 'N/A' ) {
+      # archaic way to indicate no-op completion of spool_csv (and maybe
+      # other events)?
+      $self->no_action('Y');
+    } else {
+      $statustext = $error;
+    }
   } else {
     $status = 'done';
   }
@@ -393,17 +399,22 @@ sub search_sql_where {
   if ( @event_status ) {
     my @status;
 
-    my ($done_Y, $done_N);
+    my ($done_Y, $done_N, $done_S);
+    # done_Y: action was taken
+    # done_N: action was not taken
+    # done_S: status message returned
     foreach (@event_status) {
       if ($_ eq 'done_Y') {
         $done_Y = 1;
       } elsif ( $_ eq 'done_N' ) {
         $done_N = 1;
+      } elsif ( $_ eq 'done_S' ) {
+        $done_S = 1;
       } else {
         push @status, $_;
       }
     }
-    if ( $done_Y or $done_N ) {
+    if ( $done_Y or $done_N or $done_S ) {
       push @status, 'done';
     }
     if ( @status ) {
@@ -412,12 +423,23 @@ sub search_sql_where {
                     ')';
     }
 
-    if ( $done_Y and not $done_N ) {
-      push @search, "cust_event.no_action IS NULL";
-    } elsif ( $done_N and not $done_Y ) {
-      push @search, "cust_event.no_action = 'Y'";
-    } # else they're both true, so don't add a constraint, or both false,
-      # and it doesn't matter.
+    # done_S status should include only those where statustext is not null,
+    # and done_Y should include only those where it is.
+    if ( $done_Y and $done_N and $done_S ) {
+      # then not necessary
+    } else {
+      my @done_status;
+      if ( $done_Y ) {
+        push @done_status, "(cust_event.no_action IS NULL AND cust_event.statustext IS NULL)";
+      }
+      if ( $done_N ) {
+        push @done_status, "(cust_event.no_action = 'Y')";
+      }
+      if ( $done_S ) {
+        push @done_status, "(cust_event.no_action IS NULL AND cust_event.statustext IS NOT NULL)";
+      }
+      push @search, join(' OR ', @done_status) if @done_status;
+    }
 
   } # event_status
 
diff --git a/httemplate/elements/menu.html b/httemplate/elements/menu.html
index c385cb4c7..93bebb17a 100644
--- a/httemplate/elements/menu.html
+++ b/httemplate/elements/menu.html
@@ -320,7 +320,6 @@ tie my %report_ticketing, 'Tie::IxHash',
 ;
 
 tie my %report_bill_event, 'Tie::IxHash',
-  'All billing events' => [ $fsurl.'search/report_cust_event.html', 'All billing events for a date range' ],
   'Billing event errors' => [ $fsurl.'search/report_cust_event.html?failed=1', 'Failed credit cards, processor or printer problems, etc.' ],
 ;
 
@@ -404,7 +403,7 @@ tie my %report_payable, 'Tie::IxHash',
 ;
 
 tie my %report_logs, 'Tie::IxHash';
-$report_logs{'Billing events'} =  [ \%report_bill_event, 'Billing events' ]
+$report_logs{'Billing events'} =  [ $fsurl.'search/report_cust_event.html', 'Search billing events by date and status' ]
   if $curuser->access_right('Billing event reports');
 $report_logs{'Credit limit incidents'} = [ $fsurl.'search/report_cust_main_credit_limit.html', '' ]
   if $curuser->access_right('List rating data');
diff --git a/httemplate/search/report_cust_event.html b/httemplate/search/report_cust_event.html
index 6453500ad..7aa4ff9d7 100644
--- a/httemplate/search/report_cust_event.html
+++ b/httemplate/search/report_cust_event.html
@@ -36,16 +36,18 @@
 %     } else {
 
 % # 'initial' is not on here, since nobody needs to see it. also,
-% # 'done_Y' and 'done_N' are shorthand for "done, and no_action
-% # is null" and "done, and no_action = 'Y'".
+% # 'done_Y' = "done, and no_action is null, and statustext is null"
+% # 'done_S' = "done, and no_action is null, and statustext is not null"
+% # 'done_N' = "done, and no_action = 'Y'".
       <& /elements/tr-select.html,
         'label'         => 'Event status',
         'field'         => 'event_status',
         'multiple'      => 1,
         'all_selected'  => 1,
         'size'          => 5,
-        'options'       => [ qw( done_Y done_N failed new locked ) ],
-        'option_labels' => { done_Y => 'Completed',
+        'options'       => [ qw( done_Y done_S done_N failed new locked ) ],
+        'option_labels' => { done_Y => 'Completed normally',
+                             done_S => 'Completed, with an error',
                              done_N => 'Completed, no action taken',
                              failed => 'Failed',
                              new    => 'Not yet processed',
-- 
2.11.0