From af62b675c3f1b8f5996561de7e6b28020479a7d6 Mon Sep 17 00:00:00 2001 From: Mark Wells Date: Tue, 26 May 2015 11:31:27 -0700 Subject: [PATCH] throw an exception if Avalara is misconfigured, and clean up exception handling for tax engines in general, #25718, #31639 --- FS/FS/TaxEngine.pm | 5 +++++ FS/FS/TaxEngine/avalara.pm | 23 +++++++++++++---------- FS/FS/cust_credit.pm | 10 +++++----- FS/FS/cust_main/Billing.pm | 9 +++++---- FS/FS/quotation.pm | 1 + httemplate/elements/freeside.css | 5 ++++- httemplate/view/quotation.html | 2 +- 7 files changed, 34 insertions(+), 21 deletions(-) diff --git a/FS/FS/TaxEngine.pm b/FS/FS/TaxEngine.pm index 70f1f9223..ac30eb1fc 100644 --- a/FS/FS/TaxEngine.pm +++ b/FS/FS/TaxEngine.pm @@ -95,6 +95,11 @@ L objects to add to the invoice. The base implementation is to call L to produce a list of "raw" tax line items, then L 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 a532d9a9e..fd6e324ec 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 76fdecbdf..91bbf790b 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 367745ddb..75dca3426 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 45f35229f..8843a8709 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; diff --git a/httemplate/elements/freeside.css b/httemplate/elements/freeside.css index 4fd06a0bf..ebeee985d 100644 --- a/httemplate/elements/freeside.css +++ b/httemplate/elements/freeside.css @@ -287,4 +287,7 @@ td.label { color: #ff0000; } - +.error { + font-size: large; + color: #ff0000; +} diff --git a/httemplate/view/quotation.html b/httemplate/view/quotation.html index 4abef9f78..67609a1c6 100755 --- a/httemplate/view/quotation.html +++ b/httemplate/view/quotation.html @@ -73,7 +73,7 @@ function areyousure(href, message) { % } % if ( $error ) { -<& /elements/error.html, $error &> +
<% emt('Error calculating quotation: [_1]', $error) %>
% } % if ( $conf->exists('quotation_html') ) { -- 2.11.0