From: Ivan Kohler <ivan@freeside.biz>
Date: Mon, 27 Nov 2017 20:25:35 +0000 (-0800)
Subject: better error handling when a package change fails, RT#78504
X-Git-Url: http://git.freeside.biz/gitweb/?p=freeside.git;a=commitdiff_plain;h=2cdb0b3f9e3b778fd914d847fc7851948a9930e4;hp=b278990fcae28c2f2c09a66ed8f388ac0fa478f8

better error handling when a package change fails, RT#78504
---

diff --git a/FS/FS/Cron/bill.pm b/FS/FS/Cron/bill.pm
index 2c9468b14..30eb1ab28 100644
--- a/FS/FS/Cron/bill.pm
+++ b/FS/FS/Cron/bill.pm
@@ -123,7 +123,7 @@ sub bill {
             'priority' => 99, #don't get in the way of provisioning jobs
           };
           my $error = $queue->insert( 'custnum'=>$custnum, %args );
-
+          die $error if $error;
         }
 
       } else {
@@ -132,7 +132,12 @@ sub bill {
         if ( $disable_bill ) {
           $cust_main->collect( %args, 'debug' => $debug );
         } else {
-          $cust_main->bill_and_collect( %args, 'debug' => $debug );
+          my $error = $cust_main->bill_and_collect( %args, 'fatal' => 'return',
+                                                           'debug' => $debug, );
+          if ( $error ) {
+            $log->error($error);
+            warn $error; #die $error;
+          }
         }
 
       }
diff --git a/FS/FS/cust_main/Billing.pm b/FS/FS/cust_main/Billing.pm
index 69326470a..08b10c1ff 100644
--- a/FS/FS/cust_main/Billing.pm
+++ b/FS/FS/cust_main/Billing.pm
@@ -57,7 +57,7 @@ Cancels and suspends any packages due, generates bills, applies payments and
 credits, and applies collection events to run cards, send bills and notices,
 etc.
 
-By default, warns on errors and continues with the next operation (but see the
+Any errors prevent subsequent operations from continuing and die (but see the
 "fatal" flag below).
 
 Options are passed as name-value pairs.  Currently available options are:
@@ -131,8 +131,7 @@ sub bill_and_collect {
   if ( $error ) {
     $error = "Error expiring custnum ". $self->custnum. ": $error";
     if    ( $options{fatal} && $options{fatal} eq 'return' ) { return $error; }
-    elsif ( $options{fatal}                                ) { die    $error; }
-    else                                                     { warn   $error; }
+    else                                                     { die    $error; }
   }
 
   $log->debug('suspending adjourned packages', %logopt);
@@ -140,8 +139,7 @@ sub bill_and_collect {
   if ( $error ) {
     $error = "Error adjourning custnum ". $self->custnum. ": $error";
     if    ( $options{fatal} && $options{fatal} eq 'return' ) { return $error; }
-    elsif ( $options{fatal}                                ) { die    $error; }
-    else                                                     { warn   $error; }
+    else                                                     { die    $error; }
   }
 
   $log->debug('unsuspending resumed packages', %logopt);
@@ -149,8 +147,7 @@ sub bill_and_collect {
   if ( $error ) {
     $error = "Error resuming custnum ".$self->custnum. ": $error";
     if    ( $options{fatal} && $options{fatal} eq 'return' ) { return $error; }
-    elsif ( $options{fatal}                                ) { die    $error; }
-    else                                                     { warn   $error; }
+    else                                                     { die    $error; }
   }
 
   $job->update_statustext('20,billing packages') if $job;
@@ -159,8 +156,7 @@ sub bill_and_collect {
   if ( $error ) {
     $error = "Error billing custnum ". $self->custnum. ": $error";
     if    ( $options{fatal} && $options{fatal} eq 'return' ) { return $error; }
-    elsif ( $options{fatal}                                ) { die    $error; }
-    else                                                     { warn   $error; }
+    else                                                     { die    $error; }
   }
 
   $job->update_statustext('50,applying payments and credits') if $job;
@@ -169,8 +165,7 @@ sub bill_and_collect {
   if ( $error ) {
     $error = "Error applying custnum ". $self->custnum. ": $error";
     if    ( $options{fatal} && $options{fatal} eq 'return' ) { return $error; }
-    elsif ( $options{fatal}                                ) { die    $error; }
-    else                                                     { warn   $error; }
+    else                                                     { die    $error; }
   }
 
   # In a batch tax environment, do not run collection if any pending 
@@ -195,8 +190,7 @@ sub bill_and_collect {
     if ( $error ) {
       $error = "Error collecting custnum ". $self->custnum. ": $error";
       if    ($options{fatal} && $options{fatal} eq 'return') { return $error; }
-      elsif ($options{fatal}                               ) { die    $error; }
-      else                                                   { warn   $error; }
+      else                                                   { die    $error; }
     }
   }
 
@@ -216,12 +210,11 @@ sub cancel_expired_pkgs {
 
   my @errors = ();
 
-  my @really_cancel_pkgs;
-  my @cancel_reasons;
+  my @really_cancel_pkgs = ();
+  my @cancel_reasons = ();
 
   CUST_PKG: foreach my $cust_pkg ( @cancel_pkgs ) {
     my $cpr = $cust_pkg->last_cust_pkg_reason('expire');
-    my $error;
 
     if ( $cust_pkg->change_to_pkgnum ) {
 
@@ -231,9 +224,10 @@ sub cancel_expired_pkgs {
                       $cust_pkg->change_to_pkgnum.'; not expiring';
         next CUST_PKG;
       }
-      $error = $cust_pkg->change( 'cust_pkg'        => $new_pkg,
-                                  'unprotect_svcs'  => 1 );
-      $error = '' if ref $error eq 'FS::cust_pkg';
+      my $error = $cust_pkg->change( 'cust_pkg'        => $new_pkg,
+                                     'unprotect_svcs'  => 1,
+                                   );
+      push @errors, $error if $error && ref($error) ne 'FS::cust_pkg';
 
     } else { # just cancel it