non-package fees, fixes for tax calculation and sales reports, #25899

8 files changed, 136 insertions, 87 deletions

diff --git a/FS/FS/Report/Table.pm b/FS/FS/Report/Table.pm
index 7f59384..17b12ae 100644
--- a/FS/FS/Report/Table.pm
+++ b/FS/FS/Report/Table.pm
@@ -141,7 +141,7 @@ sub payments {
 sub credits {
   my( $self, $speriod, $eperiod, $agentnum, %opt ) = @_;
   $self->scalar_sql("
-    SELECT SUM(amount)
+    SELECT SUM(cust_credit.amount)
       FROM cust_credit
         LEFT JOIN cust_main USING ( custnum )
       WHERE ". $self->in_time_period_and_agent($speriod, $eperiod, $agentnum).
@@ -390,9 +390,6 @@ unspecified, defaults to all three.
 'use_override': for line items generated by an add-on package, use the class
 of the add-on rather than the base package.
 
-'freq': limit to packages with this frequency.  Currently uses the part_pkg 
-frequency, so term discounted packages may give odd results.
-
 'distribute': for non-monthly recurring charges, ignore the invoice 
 date.  Instead, consider the line item's starting/ending dates.  Determine 
 the fraction of the line item duration that falls within the specified 
@@ -421,7 +418,8 @@ my $cust_bill_pkg_join = '
     LEFT JOIN cust_main USING ( custnum )
     LEFT JOIN cust_pkg USING ( pkgnum )
     LEFT JOIN part_pkg USING ( pkgpart )
-    LEFT JOIN part_pkg AS override ON pkgpart_override = override.pkgpart';
+    LEFT JOIN part_pkg AS override ON pkgpart_override = override.pkgpart
+    LEFT JOIN part_fee USING ( feepart )';
 
 sub cust_bill_pkg_setup {
   my $self = shift;
@@ -434,7 +432,7 @@ sub cust_bill_pkg_setup {
   $agentnum ||= $opt{'agentnum'};
 
   my @where = (
-    'pkgnum != 0',
+    '(pkgnum != 0 OR feepart IS NOT NULL)',
     $self->with_classnum($opt{'classnum'}, $opt{'use_override'}),
     $self->with_report_option(%opt),
     $self->in_time_period_and_agent($speriod, $eperiod, $agentnum),
@@ -461,7 +459,7 @@ sub cust_bill_pkg_recur {
   my $cust_bill_pkg = $opt{'project'} ? 'v_cust_bill_pkg' : 'cust_bill_pkg';
 
   my @where = (
-    'pkgnum != 0',
+    '(pkgnum != 0 OR feepart IS NOT NULL)',
     $self->with_classnum($opt{'classnum'}, $opt{'use_override'}),
     $self->with_report_option(%opt),
   );
@@ -476,13 +474,14 @@ sub cust_bill_pkg_recur {
     $item_usage = 'usage'; #already calculated
   }
   else {
-    $item_usage = '( SELECT COALESCE(SUM(amount),0)
+    $item_usage = '( SELECT COALESCE(SUM(cust_bill_pkg_detail.amount),0)
       FROM cust_bill_pkg_detail
       WHERE cust_bill_pkg_detail.billpkgnum = cust_bill_pkg.billpkgnum )';
   }
   my $recur_fraction = '';
 
   if ( $opt{'distribute'} ) {
+    $where[0] = 'pkgnum != 0'; # specifically exclude fees
     push @where, "cust_main.agentnum = $agentnum" if $agentnum;
     push @where,
       "$cust_bill_pkg.sdate <  $eperiod",
@@ -521,7 +520,8 @@ Arguments as for C<cust_bill_pkg>, plus:
 sub cust_bill_pkg_detail {
   my( $self, $speriod, $eperiod, $agentnum, %opt ) = @_;
 
-  my @where = ( "cust_bill_pkg.pkgnum != 0" );
+  my @where = 
+    ( "(cust_bill_pkg.pkgnum != 0 OR cust_bill_pkg.feepart IS NOT NULL)" );
 
   push @where, 'cust_main.refnum = '. $opt{'refnum'} if $opt{'refnum'};
 
@@ -536,7 +536,9 @@ sub cust_bill_pkg_detail {
     ;
 
   if ( $opt{'distribute'} ) {
-    # then limit according to the usage time, not the billing date
+    # exclude fees
+    $where[0] = 'cust_bill_pkg.pkgnum != 0';
+    # and limit according to the usage time, not the billing date
     push @where, $self->in_time_period_and_agent($speriod, $eperiod, $agentnum,
       'cust_bill_pkg_detail.startdate'
     );
@@ -547,7 +549,7 @@ sub cust_bill_pkg_detail {
     );
   }
 
-  my $total_sql = " SELECT SUM(amount) ";
+  my $total_sql = " SELECT SUM(cust_bill_pkg_detail.amount) ";
 
   $total_sql .=
     " / CASE COUNT(cust_pkg.*) WHEN 0 THEN 1 ELSE COUNT(cust_pkg.*) END "
@@ -561,6 +563,7 @@ sub cust_bill_pkg_detail {
         LEFT JOIN cust_pkg ON cust_bill_pkg.pkgnum = cust_pkg.pkgnum
         LEFT JOIN part_pkg USING ( pkgpart )
         LEFT JOIN part_pkg AS override ON pkgpart_override = override.pkgpart
+        LEFT JOIN part_fee USING ( feepart )
       WHERE ".join( ' AND ', grep $_, @where );
 
   $self->scalar_sql($total_sql);
@@ -683,14 +686,14 @@ sub with_classnum {
   @$classnum = grep /^\d+$/, @$classnum;
   my $in = 'IN ('. join(',', @$classnum). ')';
 
-  if ( $use_override ) {
-    "(
+  my $expr = "
          ( COALESCE(part_pkg.classnum, 0) $in AND pkgpart_override IS NULL)
-      OR ( COALESCE(override.classnum, 0) $in AND pkgpart_override IS NOT NULL )
-    )";
-  } else {
-    "COALESCE(part_pkg.classnum, 0) $in";
+      OR ( COALESCE(part_fee.classnum, 0) $in AND feepart IS NOT NULL )";
+  if ( $use_override ) {
+    $expr .= "
+      OR ( COALESCE(override.classnum, 0) $in AND pkgpart_override IS NOT NULL )";
   }
+  "( $expr )";
 }
 
 sub with_usageclass {
@@ -834,7 +837,8 @@ sub init_projection {
       # sdate/edate overlapping the ROI, for performance
       "INSERT INTO v_cust_bill_pkg ( 
         SELECT cust_bill_pkg.*,
-          (SELECT COALESCE(SUM(amount),0) FROM cust_bill_pkg_detail 
+          (SELECT COALESCE(SUM(cust_bill_pkg_detail.amount),0)
+          FROM cust_bill_pkg_detail 
           WHERE cust_bill_pkg_detail.billpkgnum = cust_bill_pkg.billpkgnum),
           cust_bill._date,
           cust_pkg.expire
diff --git a/FS/FS/Template_Mixin.pm b/FS/FS/Template_Mixin.pm
index c4c2d7f..131a236 100644
--- a/FS/FS/Template_Mixin.pm
+++ b/FS/FS/Template_Mixin.pm
@@ -2452,6 +2452,8 @@ sub _items_cust_bill_pkg {
 
         warn "$me _items_cust_bill_pkg cust_bill_pkg is quotation_pkg\n"
           if $DEBUG > 1;
+        # quotation_pkgs are never fees, so don't worry about the case where
+        # part_pkg is undefined
 
         if ( $cust_bill_pkg->setup != 0 ) {
           my $description = $desc;
@@ -2471,7 +2473,7 @@ sub _items_cust_bill_pkg {
           };
         }
 
-      } elsif ( $cust_bill_pkg->pkgnum > 0 ) {
+      } elsif ( $cust_bill_pkg->pkgnum > 0 ) { # and it's not a quotation_pkg
 
         warn "$me _items_cust_bill_pkg cust_bill_pkg is non-tax\n"
           if $DEBUG > 1;
@@ -2739,29 +2741,21 @@ sub _items_cust_bill_pkg {
 
         } # recurring or usage with recurring charge
 
-      } else { #pkgnum tax or one-shot line item (??)
+      } else { # taxes and fees
 
         warn "$me _items_cust_bill_pkg cust_bill_pkg is tax\n"
           if $DEBUG > 1;
 
-        if ( $cust_bill_pkg->setup != 0 ) {
-          push @b, {
-            'description' => $desc,
-            'amount'      => sprintf("%.2f", $cust_bill_pkg->setup),
-          };
-        }
-        if ( $cust_bill_pkg->recur != 0 ) {
-          push @b, {
-            'description' => "$desc (".
-                             $self->time2str_local('short', $cust_bill_pkg->sdate). ' - '.
-                             $self->time2str_local('short', $cust_bill_pkg->edate). ')',
-            'amount'      => sprintf("%.2f", $cust_bill_pkg->recur),
-          };
-        }
+        # items of this kind should normally not have sdate/edate.
+        push @b, {
+          'description' => $desc,
+          'amount'      => sprintf('%.2f', $cust_bill_pkg->setup 
+                                           + $cust_bill_pkg->recur)
+        };
 
-      }
+      } # if quotation / package line item / other line item
 
-    }
+    } # foreach $display
 
     $discount_show_always = ($cust_bill_pkg->cust_bill_pkg_discount
                                 && $conf->exists('discount-show-always'));
diff --git a/FS/FS/cust_bill_pkg.pm b/FS/FS/cust_bill_pkg.pm
index a943921..066ddf1 100644
--- a/FS/FS/cust_bill_pkg.pm
+++ b/FS/FS/cust_bill_pkg.pm
@@ -968,6 +968,31 @@ sub tax_locationnum {
   }
 }
 
+sub tax_location {
+  my $self = shift;
+  FS::cust_location->by_key($self->tax_locationnum);
+}
+
+=item part_X
+
+Returns the L<FS::part_pkg> or L<FS::part_fee> object that defines this
+charge.  If called on a tax line, returns nothing.
+
+=cut
+
+sub part_X {
+  my $self = shift;
+  if ( $self->override_pkgpart ) {
+    return FS::part_pkg->by_key($self->override_pkgpart);
+  } elsif ( $self->pkgnum ) {
+    return $self->cust_pkg->part_pkg;
+  } elsif ( $self->feepart ) {
+    return $self->part_fee;
+  } else {
+    return;
+  }
+}
+
 =back
 
 =head1 CLASS METHODS
diff --git a/FS/FS/cust_credit.pm b/FS/FS/cust_credit.pm
index 7ae6c97..1890845 100644
--- a/FS/FS/cust_credit.pm
+++ b/FS/FS/cust_credit.pm
@@ -910,14 +910,9 @@ sub credit_lineitems {
 
     # recalculate taxes with new amounts
     $taxlisthash{$invnum} ||= {};
-    my $part_pkg = $cust_bill_pkg->part_pkg;
-    $cust_main->_handle_taxes( $part_pkg,
-                               $taxlisthash{$invnum},
-                               $cust_bill_pkg,
-                               $cust_bill_pkg->cust_pkg,
-                               $cust_bill_pkg->cust_bill->_date, #invoice time
-                               $cust_bill_pkg->cust_pkg->pkgpart,
-                             );
+    my $part_pkg = $cust_bill_pkg->part_pkg
+      if $cust_bill_pkg->pkgpart_override;
+    $cust_main->_handle_taxes( $taxlisthash{$invnum}, $cust_bill_pkg );
   }
 
   ###
@@ -1013,12 +1008,12 @@ sub credit_lineitems {
 
       # we still have to deal with the possibility that the tax links don't
       # cover the whole amount of tax because of an incomplete upgrade...
-      if ($amount > 0) {
+      if ($amount > 0.005) {
         $cust_credit_bill{$invnum} += $amount;
         push @{ $cust_credit_bill_pkg{$invnum} },
           new FS::cust_credit_bill_pkg {
             'billpkgnum' => $tax_item->billpkgnum,
-            'amount'     => $amount,
+            'amount'     => sprintf('%.2f', $amount),
             'setuprecur' => 'setup',
           };
 
diff --git a/FS/FS/cust_main/Billing.pm b/FS/FS/cust_main/Billing.pm
index 6bd82d1..f4c30ce 100644
--- a/FS/FS/cust_main/Billing.pm
+++ b/FS/FS/cust_main/Billing.pm
@@ -596,12 +596,9 @@ sub bill {
       my $fee_location = $self->ship_location; # I think?
 
       my $error = $self->_handle_taxes(
-        $part_fee,
         $taxlisthash{$pass},
         $fee_item,
-        $fee_location,
-        $options{invoice_time},
-        {} # no options
+        location => $fee_location
       );
       return $error if $error;
 
@@ -1319,14 +1316,7 @@ sub _make_lines {
       # handle taxes
       ###
 
-      my $error = $self->_handle_taxes(
-        $part_pkg,
-        $taxlisthash,
-        $cust_bill_pkg,
-        $cust_location,
-        $options{invoice_time},
-        \%options # I have serious objections to this
-      );
+      my $error = $self->_handle_taxes( $taxlisthash, $cust_bill_pkg );
       return $error if $error;
 
       $cust_bill_pkg->set_display(
@@ -1423,15 +1413,13 @@ sub _transfer_balance {
   return @transfers;
 }
 
-=item _handle_taxes PART_ITEM TAXLISTHASH CUST_BILL_PKG CUST_LOCATION TIME [ OPTIONS ]
+=item handle_taxes TAXLISTHASH CUST_BILL_PKG [ OPTIONS ]
 
 This is _handle_taxes.  It's called once for each cust_bill_pkg generated
-from _make_lines, along with the part_pkg (or part_fee), cust_location,
-invoice time, a flag indicating whether the package is being canceled, and a 
-partridge in a pear tree.
+from _make_lines.
 
-The most important argument is 'taxlisthash'.  This is shared across the 
-entire invoice.  It looks like this:
+TAXLISTHASH is a hashref shared across the entire invoice.  It looks like 
+this:
 {
   'cust_main_county 1001' => [ [FS::cust_main_county], ... ],
   'cust_main_county 1002' => [ [FS::cust_main_county], ... ],
@@ -1444,16 +1432,27 @@ That "..." is a list of FS::cust_bill_pkg objects that will be fed to
 the 'taxline' method to calculate the amount of the tax.  This doesn't
 happen until calculate_taxes, though.
 
+OPTIONS may include:
+- part_item: a part_pkg or part_fee object to be used as the package/fee 
+  definition.
+- location: a cust_location to be used as the billing location.
+
+If not supplied, part_item will be inferred from the pkgnum or feepart of the
+cust_bill_pkg, and location from the pkgnum (or, for fees, the invnum and 
+the customer's default service location).
+
 =cut
 
 sub _handle_taxes {
   my $self = shift;
-  my $part_item = shift;
   my $taxlisthash = shift;
   my $cust_bill_pkg = shift;
-  my $location = shift;
-  my $invoice_time = shift;
-  my $options = shift;
+  my %options = @_;
+
+  # at this point I realize that we have enough information to infer all this
+  # stuff, instead of passing around giant honking argument lists
+  my $location = $options{location} || $cust_bill_pkg->tax_location;
+  my $part_item = $options{part_item} || $cust_bill_pkg->part_X;
 
   local($DEBUG) = $FS::cust_main::DEBUG if $FS::cust_main::DEBUG > $DEBUG;
 
@@ -1473,9 +1472,8 @@ sub _handle_taxes {
     my @classes;
     #push @classes, $cust_bill_pkg->usage_classes if $cust_bill_pkg->type eq 'U';
     push @classes, $cust_bill_pkg->usage_classes if $cust_bill_pkg->usage;
-    # debatable
-    push @classes, 'setup' if ($cust_bill_pkg->setup && !$options->{cancel});
-    push @classes, 'recur' if ($cust_bill_pkg->recur && !$options->{cancel});
+    push @classes, 'setup' if $cust_bill_pkg->setup;
+    push @classes, 'recur' if $cust_bill_pkg->recur;
 
     my $exempt = $conf->exists('cust_class-tax_exempt')
                    ? ( $self->cust_class ? $self->cust_class->tax : '' )
@@ -1543,10 +1541,7 @@ sub _handle_taxes {
           warn "adding $totname to taxed taxes\n" if $DEBUG > 2;
           # calculate the tax amount that the tax_on_tax will apply to
           my $hashref_or_error = 
-            $tax_object->taxline( $localtaxlisthash{$tax},
-                                  'custnum'      => $self->custnum,
-                                  'invoice_time' => $invoice_time,
-                                );
+            $tax_object->taxline( $localtaxlisthash{$tax} );
           return $hashref_or_error
             unless ref($hashref_or_error);
           
diff --git a/FS/FS/part_event/Action/fee.pm b/FS/FS/part_event/Action/fee.pm
index c2b4673..f1d5891 100644
--- a/FS/FS/part_event/Action/fee.pm
+++ b/FS/FS/part_event/Action/fee.pm
@@ -1,5 +1,7 @@
 package FS::part_event::Action::fee;
 
+# DEPRECATED; will most likely be removed in 4.x
+
 use strict;
 use base qw( FS::part_event::Action );
 
@@ -53,11 +55,9 @@ sub _calc_fee {
       my $part_pkg = FS::part_pkg->new({
           taxclass => $self->option('taxclass')
       });
-      my $error = $cust_main->_handle_taxes(
-        FS::part_pkg->new({ taxclass => ($self->option('taxclass') || '') }),
-        $taxlisthash,
-        $charge,
-        FS::cust_pkg->new({custnum => $cust_main->custnum}),
+      my $error = $cust_main->_handle_taxes( $taxlisthash, $charge,
+        location  => $cust_main->ship_location,
+        part_item => $part_pkg,
       );
       if ( $error ) {
         warn "error estimating taxes for breakage charge: custnum ".$cust_main->custnum."\n";
diff --git a/FS/FS/part_fee.pm b/FS/FS/part_fee.pm
index 67da245..9605d61 100644
--- a/FS/FS/part_fee.pm
+++ b/FS/FS/part_fee.pm
@@ -126,6 +126,8 @@ and replace methods.
 sub check {
   my $self = shift;
 
+  $self->set('amount', 0) unless $self->amount;
+
   my $error = 
     $self->ut_numbern('feepart')
     || $self->ut_textn('comment')
@@ -219,11 +221,17 @@ representing the invoice line item for the fee, with linked
 L<FS::cust_bill_pkg_fee> record(s) allocating the fee to the invoice or 
 its line items, as appropriate.
 
+If the fee is going to be charged on the upcoming invoice (credit card 
+processing fees, postal invoice fees), INVOICE should be an uninserted
+L<FS::cust_bill> object where the 'cust_bill_pkg' property is an arrayref
+of the non-fee line items that will appear on the invoice.
+
 =cut
 
 sub lineitem {
   my $self = shift;
   my $cust_bill = shift;
+  my $cust_main = $cust_bill->cust_main;
 
   my $amount = 0 + $self->get('amount');
   my $total_base;  # sum of base line items
@@ -273,14 +281,15 @@ sub lineitem {
 
   my $maximum = $self->maximum;
   if ( $self->limit_credit ) {
-    my $balance = $cust_bill->cust_main;
+    my $balance = $cust_bill->cust_main->balance;
     if ( $balance >= 0 ) {
-      $maximum = 0;
+      warn "Credit balance is zero, so fee is zero" if $DEBUG;
+      return; # don't bother doing estimated tax, etc.
     } elsif ( -1 * $balance < $maximum ) {
       $maximum = -1 * $balance;
     }
   }
-  if ( $maximum ne '' and $amount > $maximum ) {
+  if ( $maximum ne '' ) {
     warn "Applying maximum fee\n" if $DEBUG;
     $amount = $maximum;
   }
@@ -296,8 +305,35 @@ sub lineitem {
       setup       => 0,
       recur       => 0,
   });
+
+  if ( $maximum and $self->taxable ) {
+    warn "Estimating taxes on fee.\n";
+    # then we need to estimate tax to respect the maximum
+    # XXX currently doesn't work with external (tax_rate) taxes
+    # or batch taxes, obviously
+    my $taxlisthash = {};
+    my $error = $cust_main->_handle_taxes(
+      $taxlisthash,
+      $cust_bill_pkg,
+      location => $cust_main->ship_location
+    );
+    my $total_rate = 0;
+    # $taxlisthash: tax identifier => [ cust_main_county, cust_bill_pkg... ]
+    my @taxes = map { $_->[0] } values %$taxlisthash;
+    foreach (@taxes) {
+      $total_rate += $_->tax;
+    }
+    if ($total_rate > 0) {
+      my $max_cents = $maximum * 100;
+      my $charge_cents = sprintf('%0.f', $max_cents * 100/(100 + $total_rate));
+      $maximum = sprintf('%.2f', $charge_cents / 100.00);
+      $amount = $maximum if $amount > $maximum;
+    }
+  } # if $maximum and $self->taxable
+
+  # set the amount that we'll charge
   $cust_bill_pkg->set( $self->setuprecur, $amount );
-  
+
   if ( $self->classnum ) {
     my $pkg_category = $self->pkg_class->pkg_category;
     $cust_bill_pkg->set('section' => $pkg_category->categoryname)
diff --git a/FS/FS/tax_rate.pm b/FS/FS/tax_rate.pm
index 3d37677..4516004 100644
--- a/FS/FS/tax_rate.pm
+++ b/FS/FS/tax_rate.pm
@@ -371,7 +371,7 @@ sub passtype_name {
   $tax_passtypes{$self->passtype};
 }
 
-=item taxline TAXABLES, [ OPTIONSHASH ]
+=item taxline TAXABLES
 
 Returns a listref of a name and an amount of tax calculated for the list
 of packages/amounts referenced by TAXABLES.  If an error occurs, a message
@@ -381,13 +381,13 @@ is returned as a scalar.
 
 sub taxline {
   my $self = shift;
+  # this used to accept a hash of options but none of them did anything
+  # so it's been removed.
 
   my $taxables;
-  my %opt = ();
 
   if (ref($_[0]) eq 'ARRAY') {
     $taxables = shift;
-    %opt = @_;
   }else{
     $taxables = [ @_ ];
     #exemptions would be broken in this case