better internal tax links and various credit_lineitems fixes, #20629

7 files changed, 442 insertions, 87 deletions

diff --git a/FS/FS/Schema.pm b/FS/FS/Schema.pm
index 172ac82..b6fd3b6 100644
--- a/FS/FS/Schema.pm
+++ b/FS/FS/Schema.pm
@@ -805,13 +805,19 @@ sub tables_hashref {
         'billpkgnum',               'int',      '', '', '', '',
         'taxnum',                   'int',      '', '', '', '',
         'taxtype',              'varchar',      '', $char_d, '', '',
-        'pkgnum',                   'int',      '', '', '', '',
-        'locationnum',              'int',      '', '', '', '', #redundant?
+        'pkgnum',                   'int',      '', '', '', '', #redundant
+        'locationnum',              'int',      '', '', '', '', #redundant
         'amount',                   @money_type,        '', '',
+        'taxable_billpkgnum',       'int',  'NULL', '', '', '',
       ],
       'primary_key' => 'billpkgtaxlocationnum',
       'unique' => [],
-      'index'  => [ [ 'billpkgnum' ], [ 'taxnum' ], [ 'pkgnum' ], [ 'locationnum' ] ],
+      'index'  => [ [ 'billpkgnum' ], 
+                    [ 'taxnum' ],
+                    [ 'pkgnum' ],
+                    [ 'locationnum' ],
+                    [ 'taxable_billpkgnum' ],
+                  ],
     },
 
     'cust_bill_pkg_tax_rate_location' => {
@@ -823,10 +829,12 @@ sub tables_hashref {
         'locationtaxid',            'varchar',  'NULL', $char_d, '', '',
         'taxratelocationnum',           'int',      '', '', '', '',
         'amount',                       @money_type,        '', '',
+        'taxable_billpkgnum',       'int',  'NULL', '', '', '',
       ],
       'primary_key' => 'billpkgtaxratelocationnum',
       'unique' => [],
-      'index'  => [ [ 'billpkgnum' ], [ 'taxnum' ], [ 'taxratelocationnum' ] ],
+      'index'  => [ [ 'billpkgnum' ], [ 'taxnum' ], [ 'taxratelocationnum' ],
+                    [ 'taxable_billpkgnum' ], ],
     },
 
     'cust_bill_pkg_void' => {
diff --git a/FS/FS/Upgrade.pm b/FS/FS/Upgrade.pm
index 45cba82..fea53a2 100644
--- a/FS/FS/Upgrade.pm
+++ b/FS/FS/Upgrade.pm
@@ -305,6 +305,9 @@ sub upgrade_data {
 
     #kick off tax location history upgrade
     'cust_bill_pkg' => [],
+
+    #fix taxable line item links
+    'cust_bill_pkg_tax_location' => [],
   ;
 
   \%hash;
diff --git a/FS/FS/cust_bill_pkg.pm b/FS/FS/cust_bill_pkg.pm
index a83af13..5cff140 100644
--- a/FS/FS/cust_bill_pkg.pm
+++ b/FS/FS/cust_bill_pkg.pm
@@ -1310,9 +1310,10 @@ sub upgrade_tax_location {
                        );
           $cents_remaining -= $part;
           push @tax_links, {
-            taxnum => $taxdef->taxnum,
-            pkgnum => $nontax->pkgnum,
-            cents  => $part,
+            taxnum      => $taxdef->taxnum,
+            pkgnum      => $nontax->pkgnum,
+            billpkgnum  => $nontax->billpkgnum,
+            cents       => $part,
           };
         } #foreach $nontax
       } #foreach $taxclass
@@ -1355,6 +1356,7 @@ sub upgrade_tax_location {
             taxnum      => $_->{taxnum},
             pkgnum      => $_->{pkgnum},
             amount      => sprintf('%.2f', $_->{cents} / 100),
+            taxable_billpkgnum => $_->{billpkgnum},
         });
         my $error = $link->insert;
         if ( $error ) {
@@ -1443,6 +1445,9 @@ sub _upgrade_data {
   # Then mark the upgrade as done, so that we don't queue the job twice
   # and somehow run two of them concurrently.
   FS::upgrade_journal->set_done($upgrade);
+  # This upgrade now does the job of assigning taxable_billpkgnums to 
+  # cust_bill_pkg_tax_location, so set that task done also.
+  FS::upgrade_journal->set_done('tax_location_taxable_billpkgnum');
 }
 
 =back
diff --git a/FS/FS/cust_bill_pkg_tax_location.pm b/FS/FS/cust_bill_pkg_tax_location.pm
index 44dd6e3..723d6e0 100644
--- a/FS/FS/cust_bill_pkg_tax_location.pm
+++ b/FS/FS/cust_bill_pkg_tax_location.pm
@@ -9,6 +9,9 @@ use FS::cust_location;
 use FS::cust_bill_pay_pkg;
 use FS::cust_credit_bill_pkg;
 use FS::cust_main_county;
+use FS::Log;
+
+use List::Util qw(sum min);
 
 =head1 NAME
 
@@ -65,6 +68,11 @@ locationnum
 
 amount
 
+=item taxable_billpkgnum
+
+The billpkgnum of the L<FS::cust_bill_pkg> that this tax was charged on.
+It may specifically be on any portion of that line item (setup, recurring,
+or a usage class).
 
 =back
 
@@ -119,6 +127,7 @@ sub check {
     || $self->ut_foreign_key('pkgnum', 'cust_pkg', 'pkgnum' )
     || $self->ut_foreign_key('locationnum', 'cust_location', 'locationnum' )
     || $self->ut_money('amount')
+    || $self->ut_foreign_key('taxable_billpkgnum', 'cust_bill_pkg', 'billpkgnum')
   ;
   return $error if $error;
 
@@ -127,7 +136,7 @@ sub check {
 
 =item cust_bill_pkg
 
-Returns the associated cust_bill_pkg object
+Returns the associated cust_bill_pkg object (i.e. the tax charge).
 
 =cut
 
@@ -136,6 +145,10 @@ sub cust_bill_pkg {
   qsearchs( 'cust_bill_pkg', { 'billpkgnum' => $self->billpkgnum }  );
 }
 
+=item taxable_cust_bill_pkg
+
+Returns the cust_bill_pkg object for the I<taxable> charge.
+
 =item cust_location
 
 Returns the associated cust_location object
@@ -208,12 +221,274 @@ sub cust_main_county {
   }
 }
 
+sub _upgrade_data {
+  eval {
+    use FS::queue;
+    use Date::Parse 'str2time';
+  };
+  my $class = shift;
+  my $upgrade = 'tax_location_taxable_billpkgnum';
+  return if FS::upgrade_journal->is_done($upgrade);
+  my $job = FS::queue->new({ job => 
+      'FS::cust_bill_pkg_tax_location::upgrade_taxable_billpkgnum'
+  });
+  $job->insert($class, 's' => str2time('2012-01-01'));
+  FS::upgrade_journal->set_done($upgrade);
+}
+
+sub upgrade_taxable_billpkgnum {
+  # Associate these records to the correct taxable line items.
+  # The cust_bill_pkg upgrade now does this also for pre-3.0 records that 
+  # aren't broken out by pkgnum, so we only need to deal with the case of 
+  # multiple line items for the same pkgnum.
+  # Despite appearances, this has almost no relation to the upgrade in
+  # FS::cust_bill_pkg.
+
+  my ($class, %opt) = @_;
+  my $dbh = FS::UID::dbh();
+  my $oldAutoCommit = $FS::UID::AutoCommit;
+  local $FS::UID::AutoCommit = 0;
+  my $log = FS::Log->new('upgrade_taxable_billpkgnum');
+
+  my $date_where = '';
+  if ( $opt{s} ) {
+    $date_where .= " AND cust_bill._date >= $opt{s}";
+  }
+  if ( $opt{e} ) {
+    $date_where .= " AND cust_bill._date < $opt{e}";
+  }
+
+  my @need_to_upgrade = qsearch({
+      select => 'cust_bill_pkg_tax_location.*',
+      table => 'cust_bill_pkg_tax_location',
+      hashref => { taxable_billpkgnum => '' },
+      addl_from => 'JOIN cust_bill_pkg USING (billpkgnum)'.
+                   'JOIN cust_bill     USING (invnum)',
+      extra_sql => $date_where,
+  });
+  $log->info('Starting upgrade of '.scalar(@need_to_upgrade).
+      ' cust_bill_pkg_tax_location records.');
+
+  # keys are billpkgnums
+  my %cust_bill_pkg;
+  my %tax_location;
+  foreach (@need_to_upgrade) {
+    my $tax_billpkgnum = $_->billpkgnum;
+    $cust_bill_pkg{ $tax_billpkgnum } ||= FS::cust_bill_pkg->by_key($tax_billpkgnum);
+    $tax_location{ $tax_billpkgnum } ||= [];
+    push @{ $tax_location{ $tax_billpkgnum } }, $_;
+  }
+
+  TAX_ITEM: foreach my $tax_item (values %cust_bill_pkg) {
+    my $tax_locations = $tax_location{ $tax_item->billpkgnum };
+    my $invnum = $tax_item->invnum;
+    my $cust_bill = FS::cust_bill->by_key($tax_item->invnum);
+    my %tax_on_pkg; # keys are tax identifiers
+    TAX_LOCATION: foreach my $tax_location (@$tax_locations) {
+    # recapitulate the "cust_main_county $taxnum $pkgnum" tax identifier,
+    # in a way
+      my $taxid = join(' ',
+        $tax_location->taxtype,
+        $tax_location->taxnum,
+        $tax_location->pkgnum,
+        $tax_location->locationnum
+      );
+      $tax_on_pkg{$taxid} ||= [];
+      push @{ $tax_on_pkg{$taxid} }, $tax_location;
+    }
+    PKGNUM: foreach my $taxid (keys %tax_on_pkg) {
+      my ($taxtype, $taxnum, $pkgnum, $locationnum) = split(' ', $taxid);
+      $log->info("tax#$taxnum, pkg#$pkgnum", object => $cust_bill);
+      my @pkg_items = $cust_bill->cust_bill_pkg_pkgnum($pkgnum);
+      if (!@pkg_items) {
+        # then how is there tax on it? should never happen
+        $log->error("no line items with pkg#$pkgnum", object => $cust_bill);
+        next PKGNUM;
+      }
+      my $pkg_amount = 0;
+      foreach my $pkg_item (@pkg_items) {
+        # find the taxable amount of each one
+        my $amount = $pkg_item->setup + $pkg_item->recur;
+        # subtract any exemptions that apply to this taxdef
+        foreach (qsearch('cust_tax_exempt_pkg', {
+                  taxnum      => $taxnum,
+                  billpkgnum  => $pkg_item->billpkgnum
+                 }) )
+        {
+          $amount -= $_->amount;
+        }
+        $pkg_item->set('amount' => $pkg_item->setup + $pkg_item->recur);
+        $pkg_amount += $amount;
+      } #$pkg_item
+      next PKGNUM if $pkg_amount == 0; # probably because it's fully exempted
+      # now sort them descending by taxable amount
+      @pkg_items = sort { $b->amount <=> $a->amount }
+                   @pkg_items;
+      # and do the same with the tax links
+      # (there should be one per taxed item)
+      my @tax_links = sort { $b->amount <=> $a->amount }
+                      @{ $tax_on_pkg{$taxid} };
+
+      if (scalar(@tax_links) == scalar(@pkg_items)) {
+        # the relatively simple case: they match 1:1
+        for my $i (0 .. scalar(@tax_links) - 1) {
+          $tax_links[$i]->set('taxable_billpkgnum', 
+                              $pkg_items[$i]->billpkgnum);
+          my $error = $tax_links[$i]->replace;
+          if ( $error ) {
+            $log->error("failed to set taxable_billpkgnum in tax on pkg#$pkgnum",
+              object => $cust_bill);
+            next PKGNUM;
+          }
+        } #for $i
+      } else {
+        # the more complicated case
+        $log->warn("mismatched charges and tax links in pkg#$pkgnum",
+          object => $cust_bill);
+        my $tax_amount = sum(map {$_->amount} @tax_links);
+        # remove all tax link records and recreate them to be 1:1 with 
+        # taxable items
+        my (%billpaynum, %creditbillnum);
+        my $link_type;
+        foreach my $tax_link (@tax_links) {
+          $link_type ||= ref($tax_link);
+          my $error = $tax_link->delete;
+          if ( $error ) {
+            $log->error("error unlinking tax#$taxnum pkg#$pkgnum",
+              object => $cust_bill);
+            next PKGNUM;
+          }
+          my $pkey = $tax_link->primary_key;
+          # also remove all applications that reference this tax link
+          # (they will be applications to the tax item)
+          my %hash = ($pkey => $tax_link->get($pkey));
+          foreach (qsearch('cust_bill_pay_pkg', \%hash)) {
+            $billpaynum{$_->billpaynum} += $_->amount;
+            my $error = $_->delete;
+            die "error unapplying payment: $error" if ( $error );
+          }
+          foreach (qsearch('cust_credit_bill_pkg', \%hash)) {
+            $creditbillnum{$_->creditbillnum} += $_->amount;
+            my $error = $_->delete;
+            die "error unapplying credit: $error" if ( $error );
+          }
+        }
+        @tax_links = ();
+        my $cents_remaining = int(100 * $tax_amount);
+        foreach my $pkg_item (@pkg_items) {
+          my $cents = int(100 * $pkg_item->amount * $tax_amount / $pkg_amount);
+          my $tax_link = $link_type->new({
+              taxable_billpkgnum => $pkg_item->billpkgnum,
+              billpkgnum  => $tax_item->billpkgnum,
+              taxnum      => $taxnum,
+              taxtype     => $taxtype,
+              pkgnum      => $pkgnum,
+              locationnum => $locationnum,
+              cents       => $cents,
+          });
+          push @tax_links, $tax_link;
+          $cents_remaining -= $cents;
+        }
+        my $nlinks = scalar @tax_links;
+        my $i = 0;
+        while ($cents_remaining) {
+          $tax_links[$i % $nlinks]->set('cents' =>
+            $tax_links[$i % $nlinks]->cents + 1
+          );
+          $cents_remaining--;
+          $i++;
+        }
+        foreach my $tax_link (@tax_links) {
+          $tax_link->set('amount' => sprintf('%.2f', $tax_link->cents / 100));
+          my $error = $tax_link->insert;
+          if ( $error ) {
+            $log->error("error relinking tax#$taxnum pkg#$pkgnum",
+              object => $cust_bill);
+            next PKGNUM;
+          }
+        }
+
+        $i = 0;
+        my $error;
+        my $left = 0; # the amount "left" on the last tax link after 
+                      # applying payments, but before credits, so that 
+                      # it can receive both a payment and a credit if 
+                      # necessary
+        # reapply payments/credits...this sucks
+        foreach my $billpaynum (keys %billpaynum) {
+          my $pay_amount = $billpaynum{$billpaynum};
+          while ($i < $nlinks and $pay_amount > 0) {
+            my $this_amount = min($pay_amount, $tax_links[$i]->amount);
+            $left = $tax_links[$i]->amount - $this_amount;
+            my $app = FS::cust_bill_pay_pkg->new({
+                billpaynum            => $billpaynum,
+                billpkgnum            => $tax_links[$i]->billpkgnum,
+                billpkgtaxlocationnum => $tax_links[$i]->billpkgtaxlocationnum,
+                amount                => $this_amount,
+                setuprecur            => 'setup',
+                # sdate/edate are null
+            });
+            my $error ||= $app->insert;
+            $pay_amount -= $this_amount;
+            $i++ if $left == 0;
+          }
+        }
+        foreach my $creditbillnum (keys %creditbillnum) {
+          my $credit_amount = $creditbillnum{$creditbillnum};
+          while ($i < $nlinks and $credit_amount > 0) {
+            my $this_amount = min($left, $credit_amount, $tax_links[$i]->amount);
+            $left = $credit_amount * 2; # just so it can't be selected twice
+            $i++ if    $this_amount == $left 
+                    or $this_amount == $tax_links[$i]->amount;
+            my $app = FS::cust_credit_bill_pkg->new({
+                creditbillnum         => $creditbillnum,
+                billpkgnum            => $tax_links[$i]->billpkgnum,
+                billpkgtaxlocationnum => $tax_links[$i]->billpkgtaxlocationnum,
+                amount                => $this_amount,
+                setuprecur            => 'setup',
+                # sdate/edate are null
+            });
+            my $error ||= $app->insert;
+            $credit_amount -= $this_amount;
+          }
+        }
+        if ( $error ) {
+          # we've just unapplied a bunch of stuff, so if it won't reapply
+          # we really need to revert the whole transaction
+          die "error reapplying payments/credits: $error; upgrade halted";
+        }
+      } # scalar(@tax_links) ?= scalar(@pkg_items)
+    } #taxnum/pkgnum
+  } #TAX_ITEM
+
+  $log->info('finish');
+
+  $dbh->commit if $oldAutoCommit;
+  return;
+}
+
+=cut
+
 =back
 
 =head1 BUGS
 
-The presense of FS::cust_main_county::delete makes the cust_main_county method
-unreliable
+The presence of FS::cust_main_county::delete makes the cust_main_county method
+unreliable.
+
+Pre-3.0 versions of Freeside would only create one cust_bill_pkg_tax_location
+per tax definition (taxtype/taxnum) per invoice.  The pkgnum and locationnum 
+fields were arbitrarily set to those of the first line item subject to the 
+tax.  This created problems if the tax contribution of each line item ever 
+needed to be determined (for example, when applying credits).  For several
+months in 2012, this was changed to create one record per tax definition 
+per I<package> per invoice, which was still not specific enough to identify
+a line item.
+
+The current behavior is to create one record per tax definition per taxable
+line item, and to store the billpkgnum of the taxed line item in the record.
+The upgrade will try to convert existing records to the new format, but this 
+is not perfectly reliable.
 
 =head1 SEE ALSO
 
diff --git a/FS/FS/cust_credit.pm b/FS/FS/cust_credit.pm
index fe9572f..7741bbe 100644
--- a/FS/FS/cust_credit.pm
+++ b/FS/FS/cust_credit.pm
@@ -646,7 +646,6 @@ Example:
 use FS::cust_bill_pkg;
 sub credit_lineitems {
   my( $class, %arg ) = @_;
-
   my $curuser = $FS::CurrentUser::CurrentUser;
 
   #some false laziness w/misc/xmlhttp-cust_bill_pkg-calculate_taxes.html
@@ -713,10 +712,11 @@ sub credit_lineitems {
   }
 
   #my $subtotal = 0;
-  my $taxlisthash = {};
+  # keys in all of these are invoice numbers
   my %cust_credit_bill = ();
   my %cust_bill_pkg = ();
   my %cust_credit_bill_pkg = ();
+  my %taxlisthash = ();
   foreach my $billpkgnum ( @{$arg{billpkgnums}} ) {
     my $setuprecur = shift @{$arg{setuprecurs}};
     my $amount = shift @{$arg{amounts}};
@@ -727,6 +727,8 @@ sub credit_lineitems {
       'addl_from' => 'LEFT JOIN cust_bill USING (invnum)',
       'extra_sql' => 'AND custnum = '. $cust_main->custnum,
     }) or die "unknown billpkgnum $billpkgnum";
+  
+    my $invnum = $cust_bill_pkg->invnum;
 
     if ( $setuprecur eq 'setup' ) {
       $cust_bill_pkg->setup($amount);
@@ -740,8 +742,9 @@ sub credit_lineitems {
       $cust_bill_pkg->unitsetup(0);
     }
 
-    push @{$cust_bill_pkg{$cust_bill_pkg->invnum}}, $cust_bill_pkg;
+    push @{$cust_bill_pkg{$invnum}}, $cust_bill_pkg;
 
+    my %unapplied_payments; # billpaynum => amount
     #unapply any payments applied to this line item (other credits too?)
     foreach my $cust_bill_pay_pkg ( $cust_bill_pkg->cust_bill_pay_pkg($setuprecur) ) {
       $error = $cust_bill_pay_pkg->delete;
@@ -749,25 +752,51 @@ sub credit_lineitems {
         $dbh->rollback if $oldAutoCommit;
         return "Error unapplying payment: $error";
       }
+      $unapplied_payments{$cust_bill_pay_pkg->billpaynum}
+        += $cust_bill_pay_pkg->amount;
+    }
+    # also unapply that amount from the invoice so it doesn't screw up 
+    # application of the credit
+    foreach my $billpaynum (keys %unapplied_payments) {
+      my $cust_bill_pay = FS::cust_bill_pay->by_key($billpaynum)
+        or die "broken payment application $billpaynum";
+      $error = $cust_bill_pay->delete; # can't replace
+
+      my $new_cust_bill_pay = FS::cust_bill_pay->new({
+          $cust_bill_pay->hash,
+          billpaynum => '',
+          amount => sprintf('%.2f', 
+              $cust_bill_pay->amount - $unapplied_payments{$billpaynum}),
+      });
+
+      if ( $new_cust_bill_pay->amount > 0 ) {
+        $error ||= $new_cust_bill_pay->insert;
+      }
+      if ( $error ) {
+        $dbh->rollback if $oldAutoCommit;
+        return "Error unapplying payment: $error";
+      }
     }
 
     #$subtotal += $amount;
-    $cust_credit_bill{$cust_bill_pkg->invnum} += $amount;
-    push @{ $cust_credit_bill_pkg{$cust_bill_pkg->invnum} },
+    $cust_credit_bill{$invnum} += $amount;
+    push @{ $cust_credit_bill_pkg{$invnum} },
       new FS::cust_credit_bill_pkg {
         'billpkgnum' => $cust_bill_pkg->billpkgnum,
-        'amount'     => $amount,
+        'amount'     => sprintf('%.2f',$amount),
         'setuprecur' => $setuprecur,
         'sdate'      => $cust_bill_pkg->sdate,
         'edate'      => $cust_bill_pkg->edate,
       };
 
+    # recalculate taxes with new amounts
+    $taxlisthash{$invnum} ||= {};
     my $part_pkg = $cust_bill_pkg->part_pkg;
     $cust_main->_handle_taxes( $part_pkg,
-                               $taxlisthash,
+                               $taxlisthash{$invnum},
                                $cust_bill_pkg,
                                $cust_bill_pkg->cust_pkg,
-                               $cust_bill_pkg->cust_bill->_date,
+                               $cust_bill_pkg->cust_bill->_date, #invoice time
                                $cust_bill_pkg->cust_pkg->pkgpart,
                              );
   }
@@ -781,83 +810,108 @@ sub credit_lineitems {
 
   foreach my $invnum ( sort { $a <=> $b } keys %cust_credit_bill ) {
 
-    #taxes
+    my $arrayref_or_error =
+      $cust_main->calculate_taxes(
+        $cust_bill_pkg{$invnum}, # list of taxable items that we're crediting
+        $taxlisthash{$invnum},   # list of tax-item bindings
+        $cust_bill_pkg{$invnum}->[0]->cust_bill->_date, # invoice time
+      );
 
-    if ( @{ $cust_bill_pkg{$invnum} } ) {
-
-      my $listref_or_error = 
-        $cust_main->calculate_taxes( $cust_bill_pkg{$invnum}, $taxlisthash, $cust_bill_pkg{$invnum}->[0]->cust_bill->_date );
+    unless ( ref( $arrayref_or_error ) ) {
+      $dbh->rollback if $oldAutoCommit;
+      return "Error calculating taxes: $arrayref_or_error";
+    }
+    
+    my %tax_links; # {tax billpkgnum}{nontax billpkgnum}
 
-      unless ( ref( $listref_or_error ) ) {
-        $dbh->rollback if $oldAutoCommit;
-        return "Error calculating taxes: $listref_or_error";
+    #taxes
+    foreach my $cust_bill_pkg ( @{ $cust_bill_pkg{$invnum} } ) {
+      my $billpkgnum = $cust_bill_pkg->billpkgnum;
+      my %hash = ( 'taxable_billpkgnum' => $billpkgnum );
+      # gather up existing tax links (we need their billpkgtaxlocationnums)
+      my @tax_links = qsearch('cust_bill_pkg_tax_location', \%hash),
+                      qsearch('cust_bill_pkg_tax_rate_location', \%hash);
+
+      foreach ( @tax_links ) {
+        $tax_links{$_->billpkgnum} ||= {};
+        $tax_links{$_->billpkgnum}{$_->taxable_billpkgnum} = $_;
       }
+    }
 
-      # so, loop through the taxlines, apply just that amount to the tax line
-      #  item (save for later insert) & add to $
-
-      #my @taxlines = ();
-      #my $taxtotal = 0;
-      foreach my $taxline ( @$listref_or_error ) {
-
-        #find equivalent tax line items on the existing invoice
-        # (XXX need a more specific/deterministic way to find these than itemdesc..)
-        my $tax_cust_bill_pkg = qsearchs('cust_bill_pkg', {
-          'invnum'   => $invnum,
-          'pkgnum'   => 0, #$taxline->invnum
-          'itemdesc' => $taxline->desc,
-        });
-
-        my $amount = $taxline->setup;
-        my $desc = $taxline->desc;
-
-        foreach my $location ( $tax_cust_bill_pkg->cust_bill_pkg_tax_Xlocation ) {
-
-          #support partial credits: use $amount if smaller
-          # (so just distribute to the first location?   perhaps should
-          #  do so evenly...)
-          my $loc_amount = min( $amount, $location->amount);
-
-          $amount -= $loc_amount;
-
-          #push @taxlines,
-          #  #[ $location->desc, $taxline->setup, $taxlocnum, $taxratelocnum ];
-          #  [ $location->desc, $location->amount, $taxlocnum, $taxratelocnum ];
-          $cust_credit_bill{$invnum} += $loc_amount;
-          push @{ $cust_credit_bill_pkg{$invnum} },
-            new FS::cust_credit_bill_pkg {
-              'billpkgnum'                => $tax_cust_bill_pkg->billpkgnum,
-              'amount'                    => $loc_amount,
-              'setuprecur'                => 'setup',
-              'billpkgtaxlocationnum'     => $location->billpkgtaxlocationnum,
-              'billpkgtaxratelocationnum' => $location->billpkgtaxratelocationnum,
-            };
-
-        }
-        if ($amount > 0) {
-          #$taxtotal += $amount;
-          #push @taxlines,
-          #  [ $taxline->itemdesc. ' (default)', sprintf('%.2f', $amount), '', '' ];
-
-          $cust_credit_bill{$invnum} += $amount;
-          push @{ $cust_credit_bill_pkg{$invnum} },
-            new FS::cust_credit_bill_pkg {
-              'billpkgnum' => $tax_cust_bill_pkg->billpkgnum,
-              'amount'     => $amount,
-              'setuprecur' => 'setup',
-            };
-
+    foreach my $taxline ( @$arrayref_or_error ) {
+
+      my $amount = $taxline->setup;
+
+      # find equivalent tax line item on the existing invoice
+      my $tax_item = qsearchs('cust_bill_pkg', {
+          'invnum'    => $invnum,
+          'pkgnum'    => 0,
+          'itemdesc'  => $taxline->desc,
+      });
+      if (!$tax_item) {
+        # or should we just exit if this happens?
+        $cust_credit->set('amount', 
+          sprintf('%.2f', $cust_credit->get('amount') - $amount)
+        );
+        my $error = $cust_credit->replace;
+        if ( $error ) {
+          $dbh->rollback if $oldAutoCommit;
+          return "error correcting credit for missing tax line: $error";
         }
       }
 
-    }
+      # but in the new era, we no longer have the problem of uniquely
+      # identifying the tax_Xlocation record.  The billpkgnums of the 
+      # tax and the taxed item are known.
+      foreach my $new_loc
+        ( @{ $taxline->get('cust_bill_pkg_tax_location') },
+          @{ $taxline->get('cust_bill_pkg_tax_rate_location') } )
+      {
+        # the existing tax_Xlocation object
+        my $old_loc =
+          $tax_links{$tax_item->billpkgnum}{$new_loc->taxable_billpkgnum};
+
+        next if !$old_loc; # apply the leftover amount nonspecifically
+
+        #support partial credits: use $amount if smaller
+        # (so just distribute to the first location?   perhaps should
+        #  do so evenly...)
+        my $loc_amount = min( $amount, $new_loc->amount);
+
+        $amount -= $loc_amount;
+
+        $cust_credit_bill{$invnum} += $loc_amount;
+        push @{ $cust_credit_bill_pkg{$invnum} },
+          new FS::cust_credit_bill_pkg {
+            'billpkgnum'                => $tax_item->billpkgnum,
+            'amount'                    => $loc_amount,
+            'setuprecur'                => 'setup',
+            'billpkgtaxlocationnum'     => $old_loc->billpkgtaxlocationnum,
+            'billpkgtaxratelocationnum' => $old_loc->billpkgtaxratelocationnum,
+          };
+
+      } #foreach my $new_loc
+
+      # 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) {
+        $cust_credit_bill{$invnum} += $amount;
+        push @{ $cust_credit_bill_pkg{$invnum} },
+          new FS::cust_credit_bill_pkg {
+            'billpkgnum' => $tax_item->billpkgnum,
+            'amount'     => $amount,
+            'setuprecur' => 'setup',
+          };
+
+      } # if $amount > 0
+    } #foreach $taxline
 
     #insert cust_credit_bill
 
     my $cust_credit_bill = new FS::cust_credit_bill {
       'crednum' => $cust_credit->crednum,
       'invnum'  => $invnum,
-      'amount'  => $cust_credit_bill{$invnum},
+      'amount'  => sprintf('%.2f', $cust_credit_bill{$invnum}),
     };
     $error = $cust_credit_bill->insert;
     if ( $error ) {
diff --git a/FS/FS/cust_main/Billing.pm b/FS/FS/cust_main/Billing.pm
index 0ec6b54..6d3ff91 100644
--- a/FS/FS/cust_main/Billing.pm
+++ b/FS/FS/cust_main/Billing.pm
@@ -760,11 +760,13 @@ sub calculate_taxes {
   my %tax_exemption;
 
   foreach my $tax ( keys %$taxlisthash ) {
-    # $tax is a tax identifier
+    # $tax is a tax identifier (intersection of a tax definition record
+    # and a cust_bill_pkg record)
     my $tax_object = shift @{ $taxlisthash->{$tax} };
     # $tax_object is a cust_main_county or tax_rate 
-    # (with pkgnum and locationnum set)
-    # the rest of @{ $taxlisthash->{$tax} } is cust_bill_pkg objects
+    # (with billpkgnum, pkgnum, locationnum set)
+    # the rest of @{ $taxlisthash->{$tax} } is cust_bill_pkg component objects
+    # (setup, recurring, usage classes)
     warn "found ". $tax_object->taxname. " as $tax\n" if $DEBUG > 2;
     warn " ". join('/', @{ $taxlisthash->{$tax} } ). "\n" if $DEBUG > 2;
     # taxline calculates the tax on all cust_bill_pkgs in the 
@@ -808,6 +810,7 @@ sub calculate_taxes {
           'pkgnum'      => $tax_object->get('pkgnum'),
           'locationnum' => $tax_object->get('locationnum'),
           'amount'      => sprintf('%.2f', $amount ),
+          'taxable_billpkgnum' => $tax_object->get('billpkgnum'),
         };
     }
     elsif ( ref($tax_object) eq 'FS::tax_rate' ) {
@@ -820,6 +823,7 @@ sub calculate_taxes {
           'amount'             => sprintf('%.2f', $amount ),
           'locationtaxid'      => $tax_object->location,
           'taxratelocationnum' => $taxratelocationnum,
+          'taxable_billpkgnum' => $tax_object->get('billpkgnum'),
         };
     }
 
@@ -1284,6 +1288,10 @@ sub _handle_taxes {
       foreach (@taxes) {
         # These could become cust_bill_pkg_tax_location records,
         # or cust_tax_exempt_pkg.  We'll decide later.
+        #
+        # The most important thing here: record which charge is being taxed.
+        $_->set('billpkgnum',   $cust_bill_pkg->billpkgnum);
+        # also these, for historical reasons
         $_->set('pkgnum',       $cust_pkg->pkgnum);
         $_->set('locationnum',  $cust_pkg->tax_locationnum);
       }
@@ -1325,8 +1333,8 @@ sub _handle_taxes {
 
       # this is the tax identifier, not the taxname
       my $taxname = ref( $tax ). ' '. $tax->taxnum;
-      $taxname .= ' pkgnum'. $cust_pkg->pkgnum;
-      # We need to create a separate $taxlisthash entry for each pkgnum
+      $taxname .= ' billpkgnum'. $cust_bill_pkg->billpkgnum;
+      # We need to create a separate $taxlisthash entry for each billpkgnum
       # on the invoice, so that cust_bill_pkg_tax_location records will
       # be linked correctly.
 
diff --git a/FS/FS/log_context.pm b/FS/FS/log_context.pm
index 372bdaa..a254905 100644
--- a/FS/FS/log_context.pm
+++ b/FS/FS/log_context.pm
@@ -12,6 +12,8 @@ my @contexts = ( qw(
   spool_upload
   daily
   queue
+  upgrade
+  upgrade_taxable_billpkgnum
 ) );
 
 =head1 NAME