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 98f1c2e..caedcd3 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 c44e5ae..debff8a 100644 (file)
--- a/FS/FS/cust_main/Billing.pm
+++ b/FS/FS/cust_main/Billing.pm
@@ -56,7 +56,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:


@@ -130,8 +130,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);


@@ -139,8 +138,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);


@@ -148,8 +146,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;


@@ -158,8 +155,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;


@@ -168,8 +164,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; }

   }
 
   unless ( $conf->exists('cancelled_cust-noevents')
   }
 
   unless ( $conf->exists('cancelled_cust-noevents')


@@ -181,8 +176,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; }

     }
   }
 
     }
   }
 


@@ -202,12 +196,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 ) {
 


@@ -217,9 +210,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