From: Mitch Jackson <mitch@freeside.biz>
Date: Sun, 23 Dec 2018 20:36:51 +0000 (-0500)
Subject: RT# 80488 Prevent rollback of system log messages
X-Git-Url: http://git.freeside.biz/gitweb/?p=freeside.git;a=commitdiff_plain;h=ad2c21d213088e731b33264e9bf3f868bb4689dd

RT# 80488 Prevent rollback of system log messages
---

diff --git a/FS/FS/Cron/tax_rate_update.pm b/FS/FS/Cron/tax_rate_update.pm
index b0745e409..72ca14535 100755
--- a/FS/FS/Cron/tax_rate_update.pm
+++ b/FS/FS/Cron/tax_rate_update.pm
@@ -117,10 +117,14 @@ sub wa_sales {
 
 =head2 wa_sales_log_customer_without_tax_district
 
-For any active customers with cust_location records in WA state,
-if a cust_location record has no tax district, find the correct
-district using WA DOR API, or if not possible, generate an error
-message into system log so address can be corrected
+For any cust_location records
+* In WA state
+* Attached to non cancelled packages
+* With no tax district
+
+Classify the tax district for the record using the WA State Dept of
+Revenue API.  If this fails, generate an error into system log so
+address can be corrected
 
 =cut
 
@@ -144,12 +148,31 @@ sub wa_sales_log_customer_without_tax_district {
       state    => 'WA',
       district => undef,
     },
-    addl_from => 'LEFT JOIN cust_main USING (custnum)',
-    extra_sql => sprintf 'AND ( %s ) ', FS::cust_main->active_sql,
+    addl_from => '
+      LEFT JOIN cust_main USING (custnum)
+      LEFT JOIN cust_pkg ON cust_location.locationnum = cust_pkg.locationnum
+    ',
+    extra_sql => sprintf(
+      '
+        AND cust_pkg.pkgnum IS NOT NULL
+        AND (
+             cust_pkg.cancel > %s
+          OR cust_pkg.cancel IS NULL
+        )
+      ', time()
+    ),
   );
 
   for my $cust_location ( qsearch( \%qsearch_cust_location )) {
     local $@;
+    log_info_and_warn(
+      sprintf
+        'Attempting to classify district for cust_location ' .
+        'locationnum(%s) address(%s)',
+          $cust_location->locationnum,
+          $cust_location->address1,
+    );
+
     eval {
       FS::geocode_Mixin::process_district_update(
         'FS::cust_location',
@@ -158,16 +181,13 @@ sub wa_sales_log_customer_without_tax_district {
     };
 
     if ( $@ ) {
+      # Error indicates a crash, not an error looking up district
+      # process_district_udpate will generate log messages for those errors
       log_error_and_warn(
-        sprintf "Failed to classify district for cust_location(%s): %s",
+        sprintf "Classify district error for cust_location(%s): %s",
           $cust_location->locationnum,
           $@
       );
-    } else {
-      log_info_and_warn(
-        sprintf "Classified district for cust_location(%s)",
-          $cust_location->locationnum
-      );
     }
 
     sleep 1; # Be polite to WA DOR API
@@ -289,8 +309,6 @@ sub wa_sales_update_tax_table {
 
 Create or update the L<FS::cust_main_county> records with new data
 
-
-
 =cut
 
 sub wa_sales_update_cust_main_county {
@@ -337,7 +355,9 @@ sub wa_sales_update_cust_main_county {
 
         if (
           $row->tax == ( $district->{tax_combined} * 100 )
-          && $row->taxname eq $args->{taxname}
+          &&    $row->taxname eq    $args->{taxname}
+          && uc $row->county  eq uc $district->{county}
+          && uc $row->city    eq uc $district->{city}
         ) {
           $same_count++;
           next;
diff --git a/FS/FS/Misc/Geo.pm b/FS/FS/Misc/Geo.pm
index 96ce0764b..2e44364f2 100644
--- a/FS/FS/Misc/Geo.pm
+++ b/FS/FS/Misc/Geo.pm
@@ -147,7 +147,7 @@ sub get_district {
 
 Expects output of location_hash() as parameter
 
-Dies on error, or if tax rate cannot be found using given address
+Returns undef on error, or if tax rate cannot be found using given address
 
 Query the WA State Dept of Revenue API with an address, and return
 tax district information for that address.
@@ -172,12 +172,23 @@ Returns a hashref with the following keys:
   - country         US
   - exempt_amount   0
 
+If api returns no district for address, generates system log error
+and returns undef
+
 =cut
 
 sub wa_sales {
+
+  #
+  # no die():
+  # freeside-queued will issue dbh->rollback on die() ... this will
+  # also roll back system log messages about errors :/  freeside-queued
+  # doesn't propgate die messages into the system log.
+  #
+
   my $location_hash = shift;
 
-  # Return without die() when called with pointless context
+  # Return when called with pointless context
   return
     unless $location_hash
         && ref $location_hash
@@ -227,8 +238,10 @@ sub wa_sales {
     my $error =
       sprintf "Problem parsing XML from API URL(%s): %s",
       $prepared_url, $@;
+
     $log->error( $error );
-    die $error;
+    warn $error;
+    return;
   }
 
   my ($res_root)        = $dom->findnodes('/response');
@@ -255,8 +268,10 @@ sub wa_sales {
           $res_code ? $api_response_codes[$res_code] : 'n/a',
           $location_hash->{address1},
           $prepared_url;
+
       $log->error( $error );
-      die "$error\n";
+      warn "$error\n";
+      return;
   }
 
   my %response = (
diff --git a/bin/wa_tax_rate_update b/bin/wa_tax_rate_update
old mode 100644
new mode 100755
index fef126d34..cb5814537
--- a/bin/wa_tax_rate_update
+++ b/bin/wa_tax_rate_update
@@ -131,7 +131,7 @@ sub HELP_MESSAGE {
     Tool to update city/district sales tax rates in I<cust_main_county> from
     the Washington State Department of Revenue website.
 
-    Usage: [-f filename] [-t taxname] [-y year] [-q quarter] [-l] freeside_username
+    Usage: wa_tax_rate_update [-f filename] [-t taxname] [-y year] [-q quarter] [-l] freeside_username
 
     Optional Options:
       -f filename   Skip download, and process the specified filename