From e66bb1fd7c14904103fbf28daa1d9370ad36beba Mon Sep 17 00:00:00 2001
From: ivan <ivan>
Date: Mon, 21 Dec 2009 02:00:56 +0000
Subject: [PATCH] have 'Bill now' link cancel expired (and suspend adjourned)
 packages, and catch and return errors in all cases, RT#6627

---
 FS/FS/cust_main.pm       | 83 +++++++++++++++++++++++++++++++---------
 httemplate/misc/bill.cgi | 13 ++-----
 2 files changed, 67 insertions(+), 29 deletions(-)

diff --git a/FS/FS/cust_main.pm b/FS/FS/cust_main.pm
index 250c40f69..78a41f7ae 100644
--- a/FS/FS/cust_main.pm
+++ b/FS/FS/cust_main.pm
@@ -2333,9 +2333,11 @@ sub agent {
 =item bill_and_collect 
 
 Cancels and suspends any packages due, generates bills, applies payments and
-cred
+credits, and applies collection events to run cards, send bills and notices,
+etc.
 
-Warns on errors (Does not currently: If there is an error, returns the error, otherwise returns false.)
+By default, warns on errors and continues with the next operation (but see the
+"fatal" flag below).
 
 Options are passed as name-value pairs.  Currently available options are:
 
@@ -2361,6 +2363,12 @@ Used in conjunction with the I<time> option, this option specifies the date of f
 
 If set true, re-charges setup fees.
 
+=item fatal
+
+If set any errors prevent subsequent operations from continusing.  If set
+specifically to "return", returns the error (or false, if there is no error).
+Any other true value causes errors to die.
+
 =item debug
 
 Debugging level.  Default is 0 (no debugging), or can be set to 1 (passed-in options), 2 (traces progress), 3 (more information), or 4 (include full search queries)
@@ -2375,34 +2383,70 @@ options of those methods are also available.
 sub bill_and_collect {
   my( $self, %options ) = @_;
 
+  my $error;
+
   #$options{actual_time} not $options{time} because freeside-daily -d is for
   #pre-printing invoices
-  $self->cancel_expired_pkgs(    $options{actual_time} );
-  $self->suspend_adjourned_pkgs( $options{actual_time} );
 
-  my $error = $self->bill( %options );
-  warn "Error billing, custnum ". $self->custnum. ": $error" if $error;
+  $options{'actual_time'} ||= time;
+
+  $error = $self->cancel_expired_pkgs( $options{actual_time} );
+  if ( $error ) {
+    $error = "Error expiring custnum ". $self->custnum. ": $error";
+    if    ( $options{'fatal'} eq 'return' ) { return $error; }
+    elsif ( $options{'fatal'}             ) { die    $error; }
+    else                                    { warn   $error; }
+  }
+
+  $error = $self->suspend_adjourned_pkgs( $options{actual_time} );
+  if ( $error ) {
+    $error = "Error adjourning custnum ". $self->custnum. ": $error";
+    if    ( $options{'fatal'} eq 'return' ) { return $error; }
+    elsif ( $options{'fatal'}             ) { die    $error; }
+    else                                    { warn   $error; }
+  }
 
-  $self->apply_payments_and_credits;
+  $error = $self->bill( %options );
+  if ( $error ) {
+    $error = "Error billing custnum ". $self->custnum. ": $error";
+    if    ( $options{'fatal'} eq 'return' ) { return $error; }
+    elsif ( $options{'fatal'}             ) { die    $error; }
+    else                                    { warn   $error; }
+  }
+
+  $error = $self->apply_payments_and_credits;
+  if ( $error ) {
+    $error = "Error applying custnum ". $self->custnum. ": $error";
+    if    ( $options{'fatal'} eq 'return' ) { return $error; }
+    elsif ( $options{'fatal'}             ) { die    $error; }
+    else                                    { warn   $error; }
+  }
 
   unless ( $conf->exists('cancelled_cust-noevents')
            && ! $self->num_ncancelled_pkgs
   ) {
-
     $error = $self->collect( %options );
-    warn "Error collecting, custnum". $self->custnum. ": $error" if $error;
-
+    if ( $error ) {
+      $error = "Error collecting custnum ". $self->custnum. ": $error";
+      if    ( $options{'fatal'} eq 'return' ) { return $error; }
+      elsif ( $options{'fatal'}             ) { die    $error; }
+      else                                    { warn   $error; }
+    }
   }
 
+  '';
+
 }
 
 sub cancel_expired_pkgs {
-  my ( $self, $time ) = @_;
+  my ( $self, $time, %options ) = @_;
 
   my @cancel_pkgs = $self->ncancelled_pkgs( { 
     'extra_sql' => " AND expire IS NOT NULL AND expire > 0 AND expire <= $time "
   } );
 
+  my @errors = ();
+
   foreach my $cust_pkg ( @cancel_pkgs ) {
     my $cpr = $cust_pkg->last_cust_pkg_reason('expire');
     my $error = $cust_pkg->cancel($cpr ? ( 'reason'        => $cpr->reasonnum,
@@ -2410,15 +2454,15 @@ sub cancel_expired_pkgs {
                                          )
                                        : ()
                                  );
-    warn "Error cancelling expired pkg ". $cust_pkg->pkgnum.
-         " for custnum ". $self->custnum. ": $error"
-      if $error;
+    push @errors, 'pkgnum '.$cust_pkg->pkgnum.": $error" if $error;
   }
 
+  scalar(@errors) ? join(' / ', @errors) : '';
+
 }
 
 sub suspend_adjourned_pkgs {
-  my ( $self, $time ) = @_;
+  my ( $self, $time, %options ) = @_;
 
   my @susp_pkgs = $self->ncancelled_pkgs( {
     'extra_sql' =>
@@ -2442,6 +2486,8 @@ sub suspend_adjourned_pkgs {
          }
          @susp_pkgs;
 
+  my @errors = ();
+
   foreach my $cust_pkg ( @susp_pkgs ) {
     my $cpr = $cust_pkg->last_cust_pkg_reason('adjourn')
       if ($cust_pkg->adjourn && $cust_pkg->adjourn < $^T);
@@ -2450,12 +2496,11 @@ sub suspend_adjourned_pkgs {
                                           )
                                         : ()
                                   );
-
-    warn "Error suspending package ". $cust_pkg->pkgnum.
-         " for custnum ". $self->custnum. ": $error"
-      if $error;
+    push @errors, 'pkgnum '.$cust_pkg->pkgnum.": $error" if $error;
   }
 
+  scalar(@errors) ? join(' / ', @errors) : '';
+
 }
 
 =item bill OPTIONS
diff --git a/httemplate/misc/bill.cgi b/httemplate/misc/bill.cgi
index 3c3c48c54..6151dce51 100755
--- a/httemplate/misc/bill.cgi
+++ b/httemplate/misc/bill.cgi
@@ -17,21 +17,16 @@ die "Can't find customer!\n" unless $cust_main;
 
 my $conf = new FS::Conf;
 
-my $error = $cust_main->bill(
-#                          'time'=>$time
-                         );
+my $error = $cust_main->bill_and_collect( 'fatal' => 'return',
+                                          'retry' => 'yes',
+                                        );
 
-unless ( $error ) {
-  $error = $cust_main->apply_payments_and_credits
-           || $cust_main->collect(
                                   #'invoice-time'=>$time,
                                   #'batch_card'=> 'yes',
                                   #'batch_card'=> 'no',
                                   #'report_badcard'=> 'yes',
                                   #'retry_card' => 'yes',
 
-                                  'retry' => 'yes',
-                                   
                                   #this is used only by cust_main::batch_card
                                   #need to pick & create an actual config
                                   #value if we're going to turn this on
@@ -39,7 +34,5 @@ unless ( $error ) {
                                   # "backend-realtime" is for something
                                   #  entirely different)
                                   #'realtime' => $conf->exists('realtime-backend'),
-                                 );
-}
 
 </%init>
-- 
2.20.1