throw an exception if Avalara is misconfigured, and clean up exception handling for tax engines in general, #25718, #31639

5 files changed, 29 insertions, 19 deletions

diff --git a/FS/FS/TaxEngine.pm b/FS/FS/TaxEngine.pm
index 70f1f92..ac30eb1 100644
--- a/FS/FS/TaxEngine.pm
+++ b/FS/FS/TaxEngine.pm
@@ -95,6 +95,11 @@ L<FS::cust_bill_pkg> objects to add to the invoice.  The base implementation
 is to call L</make_taxlines> to produce a list of "raw" tax line items, 
 then L</consolidate_taxlines> to combine those with the same itemdesc.
 
+If this fails, it will throw an exception. (Accordingly it should not trap
+exceptions from internal methods that it calls, except to translate error 
+messages into a more meaningful form.) If it succeeds, it MUST return an
+arrayref (even if the arrayref is empty).
+
 =cut
 
 sub calculate_taxes {
diff --git a/FS/FS/TaxEngine/avalara.pm b/FS/FS/TaxEngine/avalara.pm
index a532d9a..fd6e324 100644
--- a/FS/FS/TaxEngine/avalara.pm
+++ b/FS/FS/TaxEngine/avalara.pm
@@ -35,10 +35,6 @@ sub add_sale {}
 sub build_request {
   my ($self, %opt) = @_;
 
-  my $oldAutoCommit = $FS::UID::AutoCommit;
-  local $FS::UID::AutoCommit = 0;
-  my $dbh = dbh;
-
   my $cust_bill = $self->{cust_bill};
   my $cust_main = $cust_bill->cust_main;
 
@@ -91,6 +87,12 @@ sub build_request {
     $conf->config('company_address', $cust_main->agentnum)
   );
   my $company_address = Geo::StreetAddress::US->parse_location($our_address);
+  if (!$company_address->{street}
+      or !$company_address->{city}
+      or !$company_address->{zip}) {
+    die "Your company address could not be parsed. Avalara tax calculation requires a company address with street, city, and zip code.\n";
+  }
+
   my $address1 = join(' ', grep $_, @{$company_address}{qw(
       number prefix street type suffix
   )});
@@ -162,8 +164,8 @@ sub calculate_taxes {
 
   my $cust_bill = shift;
   if (!$cust_bill->invnum) {
-    warn "FS::TaxEngine::avalara: can't calculate taxes on a non-inserted invoice";
-    return;
+    # then something is wrong
+    die "FS::TaxEngine::avalara: can't calculate taxes on a non-inserted invoice\n";
   }
   $self->{cust_bill} = $cust_bill;
 
@@ -212,8 +214,9 @@ account number, and license key.
   my %tax_item_named;
 
   if ( $response->{ResultCode} ne 'Success' ) {
-    return "invoice#".$cust_bill->invnum.": ".
-           join("\n", @{ $response->{Messages} });
+    die "Avalara tax error on invoice#".$cust_bill->invnum.": ".
+           join("\n", @{ $response->{Messages} }).
+           "\n";
   }
   warn "creating taxes for inv#$invnum\n" if $DEBUG > 1;
   foreach my $TaxLine (@{ $response->{TaxLines} }) {
@@ -248,7 +251,7 @@ account number, and license key.
           fee         => 0,
       });
       my $error = $tax_rate->find_or_insert;
-      return "error inserting tax_rate record for '$taxname': $error\n"
+      die "error inserting tax_rate record for '$taxname': $error\n"
         if $error;
       $tax_rate = $tax_rate->replace_old; # get its taxnum if there wasn't one
 
@@ -264,7 +267,7 @@ account number, and license key.
                         # country?
       });
       $error = $tax_rate_location->find_or_insert;
-      return "error inserting tax_rate_location record for ".
+      die "error inserting tax_rate_location record for ".
               $TaxDetail->{JurisCode} .": $error\n"
         if $error;
 
diff --git a/FS/FS/cust_credit.pm b/FS/FS/cust_credit.pm
index 76fdecb..91bbf79 100644
--- a/FS/FS/cust_credit.pm
+++ b/FS/FS/cust_credit.pm
@@ -852,16 +852,16 @@ sub credit_lineitems {
 
   foreach my $invnum ( sort { $a <=> $b } keys %cust_credit_bill ) {
 
-    my $arrayref_or_error =
-      $cust_main->calculate_taxes(
+    local $@;
+    my $arrayref_or_error = eval { $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
-      );
+      ) };
 
-    unless ( ref( $arrayref_or_error ) ) {
+    if ( $@ ) {
       $dbh->rollback if $oldAutoCommit;
-      return "Error calculating taxes: $arrayref_or_error";
+      return "Error calculating taxes: $@";
     }
     
     my %tax_links; # {tax billpkgnum}{nontax billpkgnum}
diff --git a/FS/FS/cust_main/Billing.pm b/FS/FS/cust_main/Billing.pm
index 367745d..75dca34 100644
--- a/FS/FS/cust_main/Billing.pm
+++ b/FS/FS/cust_main/Billing.pm
@@ -818,16 +818,17 @@ sub bill {
 
     # calculate and append taxes
     if ( ! $tax_is_batch) {
-      my $arrayref_or_error = $tax_engines{$pass}->calculate_taxes($cust_bill);
+      local $@;
+      my $arrayref = eval { $tax_engines{$pass}->calculate_taxes($cust_bill) };
 
-      unless ( ref( $arrayref_or_error ) ) {
+      if ( $@ ) {
         $dbh->rollback if $oldAutoCommit && !$options{no_commit};
-        return $arrayref_or_error;
+        return $@;
       }
 
       # or should this be in TaxEngine?
       my $total_tax = 0;
-      foreach my $taxline ( @$arrayref_or_error ) {
+      foreach my $taxline ( @$arrayref ) {
         $total_tax += $taxline->setup;
         $taxline->set('invnum' => $cust_bill->invnum); # just to be sure
         push @cust_bill_pkg, $taxline; # for return_bill
diff --git a/FS/FS/quotation.pm b/FS/FS/quotation.pm
index 45f3522..8843a87 100644
--- a/FS/FS/quotation.pm
+++ b/FS/FS/quotation.pm
@@ -571,6 +571,7 @@ sub estimate {
 
   ###### BEGIN TRANSACTION ######
   local $@;
+  local $SIG{__DIE__};
   eval {
     my $temp_dbh = myconnect();
     local $FS::UID::dbh = $temp_dbh;