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 2c9468b..30eb1ab 100644 (file)
--- 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 );
             'priority' => 99, #don't get in the way of provisioning jobs
           };
           my $error = $queue->insert( 'custnum'=>$custnum, %args );

-
+          die $error if $error;

         }
 
       } else {
         }
 
       } else {


@@ -132,7 +132,12 @@ sub bill {
         if ( $disable_bill ) {
           $cust_main->collect( %args, 'debug' => $debug );
         } else {
         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 6932647..08b10c1 100644 (file)
--- 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.
 
 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:
 "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; }
   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);
   }
 
   $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; }
   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);
   }
 
   $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; }
   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;
   }
 
   $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; }
   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;
   }
 
   $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; }
   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 
   }
 
   # 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; }
     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 @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');
 
   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 ) {
 
 
     if ( $cust_pkg->change_to_pkgnum ) {
 


@@ -231,9 +224,10 @@ sub cancel_expired_pkgs {
                       $cust_pkg->change_to_pkgnum.'; not expiring';
         next CUST_PKG;
       }
                       $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
 
 
     } else { # just cancel it