customer bill/ship location refactoring, #940
[freeside.git] / FS / FS / cust_main.pm
index 9766579..56338e5 100644 (file)
@@ -6,6 +6,7 @@ use strict;
 use base qw( FS::cust_main::Packages FS::cust_main::Status
              FS::cust_main::Billing FS::cust_main::Billing_Realtime
              FS::cust_main::Billing_Discount
+             FS::cust_main::Location
              FS::otaker_Mixin FS::payinfo_Mixin FS::cust_main_Mixin
              FS::geocode_Mixin
              FS::o2m_Common
@@ -14,7 +15,7 @@ use base qw( FS::cust_main::Packages FS::cust_main::Status
 use vars qw( $DEBUG $me $conf
              @encrypted_fields
              $import
-             $ignore_expired_card $ignore_illegal_zip $ignore_banned_card
+             $ignore_expired_card $ignore_banned_card $ignore_illegal_zip
              $skip_fuzzyfiles
              @paytypes
            );
@@ -80,7 +81,6 @@ $me = '[FS::cust_main]';
 
 $import = 0;
 $ignore_expired_card = 0;
-$ignore_illegal_zip = 0;
 $ignore_banned_card = 0;
 
 $skip_fuzzyfiles = 0;
@@ -178,28 +178,6 @@ Cocial security number (optional)
 
 (optional)
 
-=item address1
-
-=item address2
-
-(optional)
-
-=item city
-
-=item county
-
-(optional, see L<FS::cust_main_county>)
-
-=item state
-
-(see L<FS::cust_main_county>)
-
-=item zip
-
-=item country
-
-(see L<FS::cust_main_county>)
-
 =item daytime
 
 phone (optional)
@@ -216,56 +194,6 @@ phone (optional)
 
 phone (optional)
 
-=item ship_first
-
-Shipping first name
-
-=item ship_last
-
-Shipping last name
-
-=item ship_company
-
-(optional)
-
-=item ship_address1
-
-=item ship_address2
-
-(optional)
-
-=item ship_city
-
-=item ship_county
-
-(optional, see L<FS::cust_main_county>)
-
-=item ship_state
-
-(see L<FS::cust_main_county>)
-
-=item ship_zip
-
-=item ship_country
-
-(see L<FS::cust_main_county>)
-
-=item ship_daytime
-
-phone (optional)
-
-=item ship_night
-
-phone (optional)
-
-=item ship_fax
-
-phone (optional)
-
-=item ship_mobile
-
-phone (optional)
-
 =item payby
 
 Payment Type (See L<FS::payinfo_Mixin> for valid payby values)
@@ -364,6 +292,12 @@ sub table { 'cust_main'; }
 Adds this customer to the database.  If there is an error, returns the error,
 otherwise returns false.
 
+Usually the customer's location will not yet exist in the database, and
+the C<bill_location> and C<ship_location> pseudo-fields must be set to 
+uninserted L<FS::cust_location> objects.  These will be inserted and linked
+(in both directions) to the new customer record.  If they're references 
+to the same object, they will become the same location.
+
 CUST_PKG_HASHREF: If you pass a Tie::RefHash data structure to the insert
 method containing FS::cust_pkg and FS::svc_I<tablename> objects, all records
 are inserted atomicly, or the transaction is rolled back.  Passing an empty
@@ -462,13 +396,44 @@ sub insert {
 
   }
 
+  # insert locations
+  foreach my $l (qw(bill_location ship_location)) {
+    my $loc = delete $self->hashref->{$l};
+    # XXX if we're moving a prospect's locations, do that here
+    
+    if ( !$loc->locationnum ) {
+      # warn the location that we're going to insert it with no custnum
+      $loc->set(custnum_pending => 1);
+      warn "  inserting $l\n"
+        if $DEBUG > 1;
+      my $error = $loc->insert;
+      if ( $error ) {
+        $dbh->rollback if $oldAutoCommit;
+        my $label = $l eq 'ship_location' ? 'service' : 'billing';
+        return "$error (in $label location)";
+      }
+    }
+    elsif ( $loc->custnum != $self->custnum or $loc->prospectnum > 0 ) {
+      # this shouldn't happen
+      $dbh->rollback if $oldAutoCommit;
+      return "$l belongs to customer ".$loc->custnum;
+    }
+    # else it already belongs to this customer 
+    # (happens when ship_location is identical to bill_location)
+
+    $self->set($l.'num', $loc->locationnum);
+
+    if ( $self->get($l.'num') eq '' ) {
+      $dbh->rollback if $oldAutoCommit;
+      return "$l not set";
+    }
+  }
+
   warn "  inserting $self\n"
     if $DEBUG > 1;
 
   $self->signupdate(time) unless $self->signupdate;
 
-  $self->censusyear($conf->config('census_year')||'2012') if $self->censustract;
-
   $self->auto_agent_custid()
     if $conf->config('cust_main-auto_agent_custid') && ! $self->agent_custid;
 
@@ -479,6 +444,20 @@ sub insert {
     return $error;
   }
 
+  # now set cust_location.custnum
+  foreach my $l (qw(bill_location ship_location)) {
+    warn "  setting $l.custnum\n"
+      if $DEBUG > 1;
+    my $loc = $self->$l;
+    $loc->set(custnum => $self->custnum);
+    $error ||= $loc->replace;
+
+    if ( $error ) {
+      $dbh->rollback if $oldAutoCommit;
+      return "error setting $l custnum: $error";
+    }
+  }
+
   warn "  setting invoicing list\n"
     if $DEBUG > 1;
 
@@ -1318,7 +1297,7 @@ sub merge {
 
   }
 
-  my $name = $self->ship_name;
+  my $name = $self->ship_name; #?
 
   my $locationnum = '';
   foreach my $cust_pkg ( $self->all_pkgs ) {
@@ -1454,10 +1433,13 @@ sub merge {
 
 =item replace [ OLD_RECORD ] [ INVOICING_LIST_ARYREF ] [ , OPTION => VALUE ... ] ]
 
-
 Replaces the OLD_RECORD with this one in the database.  If there is an error,
 returns the error, otherwise returns false.
 
+To change the customer's address, set the pseudo-fields C<bill_location> and
+C<ship_location>.  The address will still only change if at least one of the
+address fields differs from the existing values.
+
 INVOICING_LIST_ARYREF: If you pass an arrarref to the insert method, it will
 be set as the invoicing list (see L<"invoicing_list">).  Errors return as
 expected and rollback the entire transaction; it is not necessary to call 
@@ -1494,41 +1476,19 @@ sub replace {
     return "You are not permitted to create complimentary accounts.";
   }
 
-  if ( $old->get('geocode') && $old->get('geocode') eq $self->get('geocode')
-       && $conf->exists('enable_taxproducts')
-     )
-  {
-    my $pre = ($conf->exists('tax-ship_address') && $self->ship_zip)
-                ? 'ship_' : '';
-    $self->set('geocode', '')
-      if $old->get($pre.'zip') ne $self->get($pre.'zip')
-      && length($self->get($pre.'zip')) >= 10;
-  }
-
-  for my $pre ( grep $old->get($_.'coord_auto'), ( '', 'ship_' ) ) {
-
-    $self->set($pre.'coord_auto', '') && next
-      if $self->get($pre.'latitude') && $self->get($pre.'longitude')
-      && (    $self->get($pre.'latitude')  != $old->get($pre.'latitude')
-           || $self->get($pre.'longitude') != $old->get($pre.'longitude')
-         );
-
-    $self->set_coord($pre)
-      if $old->get($pre.'address1') ne $self->get($pre.'address1')
-      || $old->get($pre.'city')     ne $self->get($pre.'city')
-      || $old->get($pre.'state')    ne $self->get($pre.'state')
-      || $old->get($pre.'country')  ne $self->get($pre.'country');
-
-  }
+  # should be unnecessary--geocode will default to null on new locations
+  #if ( $old->get('geocode') && $old->get('geocode') eq $self->get('geocode')
+  #     && $conf->exists('enable_taxproducts')
+  #   )
+  #{
+  #  my $pre = ($conf->exists('tax-ship_address') && $self->ship_zip)
+  #              ? 'ship_' : '';
+  #  $self->set('geocode', '')
+  #    if $old->get($pre.'zip') ne $self->get($pre.'zip')
+  #    && length($self->get($pre.'zip')) >= 10;
+  #}
 
-  unless ( $import ) {
-    $self->set_coord
-      if ! $self->coord_auto && ! $self->latitude && ! $self->longitude;
-
-    $self->set_coord('ship_')
-      if $self->has_ship_address && ! $self->ship_coord_auto
-      && ! $self->ship_latitude && ! $self->ship_longitude;
-  }
+  # set_coord/coord_auto stuff is now handled by cust_location
 
   local($ignore_expired_card) = 1
     if $old->payby  =~ /^(CARD|DCRD)$/
@@ -1540,11 +1500,6 @@ sub replace {
          || $old->payby  =~ /^(CHEK|DCHK)$/ && $self->payby =~ /^(CHEK|DCHK)$/ )
     && ( $old->payinfo eq $self->payinfo || $old->paymask eq $self->paymask );
 
-  if ( $self->censustract ne '' and $self->censustract ne $old->censustract ) {
-    # update censusyear whenever tract code changes
-    $self->censusyear($conf->config('census_year')||'2012');
-  }
-
   return "Invoicing locale is required"
     if $old->locale
     && ! $self->locale
@@ -1561,6 +1516,47 @@ sub replace {
   local $FS::UID::AutoCommit = 0;
   my $dbh = dbh;
 
+  for my $l (qw(bill_location ship_location)) {
+    my $old_loc = $old->$l;
+    my $new_loc = $self->$l;
+
+    if ( !$new_loc->locationnum ) {
+      # changing location
+      # If the new location is all empty fields, or if it's identical to 
+      # the old location in all fields, don't replace.
+      my @nonempty = grep { $new_loc->$_ } $self->location_fields;
+      next if !@nonempty;
+      my @unlike = grep { $new_loc->$_ ne $old_loc->$_ } $self->location_fields;
+
+      if ( @unlike or $old_loc->disabled ) {
+        warn "  changed $l fields: ".join(',',@unlike)."\n"
+          if $DEBUG;
+        $new_loc->set(custnum => $self->custnum);
+
+        # insert it--the old location will be disabled later
+        my $error = $new_loc->insert;
+        if ( $error ) {
+          $dbh->rollback if $oldAutoCommit;
+          return $error;
+        }
+
+      } else {
+      # no fields have changed and $old_loc isn't disabled, so don't change it
+        next;
+      }
+
+    }
+    elsif ( $new_loc->custnum ne $self->custnum or $new_loc->prospectnum ) {
+      $dbh->rollback if $oldAutoCommit;
+      return "$l belongs to customer ".$new_loc->custnum;
+    }
+    # else the new location belongs to this customer so we're good
+
+    # set the foo_locationnum now that we have one.
+    $self->set($l.'num', $new_loc->locationnum);
+
+  } #for $l
+
   my $error = $self->SUPER::replace($old);
 
   if ( $error ) {
@@ -1568,6 +1564,27 @@ sub replace {
     return $error;
   }
 
+  # now move packages to the new service location
+  $self->set('ship_location', ''); #flush cache
+  if ( $old->ship_locationnum and # should only be null during upgrade...
+       $old->ship_locationnum != $self->ship_locationnum ) {
+    $error = $old->ship_location->move_to($self->ship_location);
+    if ( $error ) {
+      $dbh->rollback if $oldAutoCommit;
+      return $error;
+    }
+  }
+  # don't move packages based on the billing location, but 
+  # disable it if it's no longer in use
+  if ( $old->bill_locationnum and
+       $old->bill_locationnum != $self->bill_locationnum ) {
+    $error = $old->bill_location->disable_if_unused;
+    if ( $error ) {
+      $dbh->rollback if $oldAutoCommit;
+      return $error;
+    }
+  }
+
   if ( @param && ref($param[0]) eq 'ARRAY' ) { # INVOICING_LIST_ARYREF
     my $invoicing_list = shift @param;
     $error = $self->check_invoicing_list( $invoicing_list );
@@ -1669,24 +1686,7 @@ sub replace {
     }
   }
 
-  # FS::geocode_Mixin::after_replace ?
-  # though this will go away anyway once we move customer bill/service 
-  # locations into cust_location
-  # We can trigger this on any address change--just have to make sure 
-  # not to trigger it on itself.
-  if ( $conf->config('tax_district_method') and !$import 
-      and ( $self->get('ship_address1') ne $old->get('ship_address1')
-        or  $self->get('address1')      ne $old->get('address1') ) ) {
-    my $queue = new FS::queue {
-      'job'     => 'FS::geocode_Mixin::process_district_update',
-      'custnum' => $self->custnum,
-    };
-    my $error = $queue->insert( ref($self), $self->custnum );
-    if ( $error ) {
-      $dbh->rollback if $oldAutoCommit;
-      return "queueing tax district update: $error";
-    }
-  }
+  # tax district update in cust_location
 
   # cust_main exports!
 
@@ -1731,16 +1731,14 @@ sub queue_fuzzyfiles_update {
   local $FS::UID::AutoCommit = 0;
   my $dbh = dbh;
 
-  my $queue = new FS::queue { 'job' => 'FS::cust_main::Search::append_fuzzyfiles' };
-  my $error = $queue->insert( map $self->getfield($_), @FS::cust_main::Search::fuzzyfields );
-  if ( $error ) {
-    $dbh->rollback if $oldAutoCommit;
-    return "queueing job (transaction rolled back): $error";
-  }
-
-  if ( $self->ship_last ) {
-    $queue = new FS::queue { 'job' => 'FS::cust_main::Search::append_fuzzyfiles' };
-    $error = $queue->insert( map $self->getfield("ship_$_"), @FS::cust_main::Search::fuzzyfields );
+  my @locations = $self->bill_location;
+  push @locations, $self->ship_location if $self->has_ship_address;
+  foreach my $location (@locations) {
+    my $queue = new FS::queue { 
+      'job' => 'FS::cust_main::Search::append_fuzzyfiles'
+    };
+    my @args = map $location->get($_), @FS::cust_main::Search::fuzzyfields;
+    my $error = $queue->insert( @args );
     if ( $error ) {
       $dbh->rollback if $oldAutoCommit;
       return "queueing job (transaction rolled back): $error";
@@ -1771,6 +1769,8 @@ sub check {
     || $self->ut_number('agentnum')
     || $self->ut_textn('agent_custid')
     || $self->ut_number('refnum')
+    || $self->ut_foreign_key('bill_locationnum', 'cust_location','locationnum')
+    || $self->ut_foreign_key('ship_locationnum', 'cust_location','locationnum')
     || $self->ut_foreign_keyn('classnum', 'cust_class', 'classnum')
     || $self->ut_textn('custbatch')
     || $self->ut_name('last')
@@ -1778,16 +1778,6 @@ sub check {
     || $self->ut_snumbern('birthdate')
     || $self->ut_snumbern('signupdate')
     || $self->ut_textn('company')
-    || $self->ut_text('address1')
-    || $self->ut_textn('address2')
-    || $self->ut_text('city')
-    || $self->ut_textn('county')
-    || $self->ut_textn('state')
-    || $self->ut_country('country')
-    || $self->ut_coordn('latitude')
-    || $self->ut_coordn('longitude')
-    || $self->ut_enum('coord_auto', [ '', 'Y' ])
-    || $self->ut_numbern('censusyear')
     || $self->ut_anything('comments')
     || $self->ut_numbern('referral_custnum')
     || $self->ut_textn('stateid')
@@ -1804,9 +1794,6 @@ sub check {
     || $self->ut_enum('locale', [ '', FS::Locales->locales ])
   ;
 
-  $self->set_coord
-    unless $import || ($self->latitude && $self->longitude);
-
   #barf.  need message catalogs.  i18n.  etc.
   $error .= "Please select an advertising source."
     if $error =~ /^Illegal or empty \(numeric\) refnum: /;
@@ -1822,13 +1809,6 @@ sub check {
     unless ! $self->referral_custnum 
            || qsearchs( 'cust_main', { 'custnum' => $self->referral_custnum } );
 
-  if ( $self->censustract ne '' ) {
-    $self->censustract =~ /^\s*(\d{9})\.?(\d{2})\s*$/
-      or return "Illegal census tract: ". $self->censustract;
-    
-    $self->censustract("$1.$2");
-  }
-
   if ( $self->ss eq '' ) {
     $self->ss('');
   } else {
@@ -1839,23 +1819,7 @@ sub check {
     $self->ss("$1-$2-$3");
   }
 
-
-# bad idea to disable, causes billing to fail because of no tax rates later
-# except we don't fail any more
-  unless ( $import ) {
-    unless ( qsearch('cust_main_county', {
-      'country' => $self->country,
-      'state'   => '',
-     } ) ) {
-      return "Unknown state/county/country: ".
-        $self->state. "/". $self->county. "/". $self->country
-        unless qsearch('cust_main_county',{
-          'state'   => $self->state,
-          'county'  => $self->county,
-          'country' => $self->country,
-        } );
-    }
-  }
+  # cust_main_county verification now handled by cust_location check
 
   $error =
        $self->ut_phonen('daytime', $self->country)
@@ -1865,12 +1829,8 @@ sub check {
   ;
   return $error if $error;
 
-  unless ( $ignore_illegal_zip ) {
-    $error = $self->ut_zip('zip', $self->country);
-    return $error if $error;
-  }
-
   if ( $conf->exists('cust_main-require_phone', $self->agentnum)
+       && ! $import
        && ! length($self->daytime) && ! length($self->night) && ! length($self->mobile)
      ) {
 
@@ -1889,71 +1849,7 @@ sub check {
   
   }
 
-  if ( $self->has_ship_address
-       && scalar ( grep { $self->getfield($_) ne $self->getfield("ship_$_") }
-                        $self->addr_fields )
-     )
-  {
-    my $error =
-      $self->ut_name('ship_last')
-      || $self->ut_name('ship_first')
-      || $self->ut_textn('ship_company')
-      || $self->ut_text('ship_address1')
-      || $self->ut_textn('ship_address2')
-      || $self->ut_text('ship_city')
-      || $self->ut_textn('ship_county')
-      || $self->ut_textn('ship_state')
-      || $self->ut_country('ship_country')
-      || $self->ut_coordn('ship_latitude')
-      || $self->ut_coordn('ship_longitude')
-      || $self->ut_enum('ship_coord_auto', [ '', 'Y' ] )
-    ;
-    return $error if $error;
-
-    $self->set_coord('ship_')
-      unless $import || ($self->ship_latitude && $self->ship_longitude);
-
-    #false laziness with above
-    unless ( qsearchs('cust_main_county', {
-      'country' => $self->ship_country,
-      'state'   => '',
-     } ) ) {
-      return "Unknown ship_state/ship_county/ship_country: ".
-        $self->ship_state. "/". $self->ship_county. "/". $self->ship_country
-        unless qsearch('cust_main_county',{
-          'state'   => $self->ship_state,
-          'county'  => $self->ship_county,
-          'country' => $self->ship_country,
-        } );
-    }
-    #eofalse
-
-    $error =
-         $self->ut_phonen('ship_daytime', $self->ship_country)
-      || $self->ut_phonen('ship_night',   $self->ship_country)
-      || $self->ut_phonen('ship_fax',     $self->ship_country)
-      || $self->ut_phonen('ship_mobile',  $self->ship_country)
-    ;
-    return $error if $error;
-
-    unless ( $ignore_illegal_zip ) {
-      $error = $self->ut_zip('ship_zip', $self->ship_country);
-      return $error if $error;
-    }
-    return "Unit # is required."
-      if $self->ship_address2 =~ /^\s*$/
-      && $conf->exists('cust_main-require_address2');
-
-  } else { # ship_ info eq billing info, so don't store dup info in database
-
-    $self->setfield("ship_$_", '')
-      foreach $self->addr_fields;
-
-    return "Unit # is required."
-      if $self->address2 =~ /^\s*$/
-      && $conf->exists('cust_main-require_address2');
-
-  }
+  #ship_ fields are gone
 
   #$self->payby =~ /^(CARD|DCRD|CHEK|DCHK|LECB|BILL|COMP|PREPAY|CASH|WEST|MCRD)$/
   #  or return "Illegal payby: ". $self->payby;
@@ -1979,7 +1875,9 @@ sub check {
   # check the credit card.
   my $check_payinfo = ! $self->is_encrypted($self->payinfo);
 
-  if ( $check_payinfo && $self->payby =~ /^(CARD|DCRD)$/ ) {
+  # Need some kind of global flag to accept invalid cards, for testing
+  # on scrubbed data.
+  if ( !$import && $check_payinfo && $self->payby =~ /^(CARD|DCRD)$/ ) {
 
     my $payinfo = $self->payinfo;
     $payinfo =~ s/\D//g;
@@ -2201,7 +2099,7 @@ Returns true if this customer record has a separate shipping address.
 
 sub has_ship_address {
   my $self = shift;
-  scalar( grep { $self->getfield("ship_$_") ne '' } $self->addr_fields );
+  $self->bill_locationnum != $self->ship_locationnum;
 }
 
 =item location_hash
@@ -2212,6 +2110,11 @@ shipping address is used if present.
 
 =cut
 
+sub location_hash {
+  my $self = shift;
+  $self->ship_location->location_hash;
+}
+
 =item cust_location
 
 Returns all locations (see L<FS::cust_location>) for this customer.
@@ -2617,6 +2520,8 @@ sub batch_card {
     $options{$_} = '' unless exists($options{$_});
   }
 
+  my $loc = $self->bill_location;
+
   my $cust_pay_batch = new FS::cust_pay_batch ( {
     'batchnum' => $pay_batch->batchnum,
     'invnum'   => $invnum || 0,                    # is there a better value?
@@ -2626,16 +2531,16 @@ sub batch_card {
     'custnum'  => $self->custnum,
     'last'     => $self->getfield('last'),
     'first'    => $self->getfield('first'),
-    'address1' => $options{address1} || $self->address1,
-    'address2' => $options{address2} || $self->address2,
-    'city'     => $options{city}     || $self->city,
-    'state'    => $options{state}    || $self->state,
-    'zip'      => $options{zip}      || $self->zip,
-    'country'  => $options{country}  || $self->country,
-    'payby'    => $options{payby}    || $self->payby,
-    'payinfo'  => $options{payinfo}  || $self->payinfo,
-    'exp'      => $options{paydate}  || $self->paydate,
-    'payname'  => $options{payname}  || $self->payname,
+    'address1' => $options{address1} || $loc->address1,
+    'address2' => $options{address2} || $loc->address2,
+    'city'     => $options{city}     || $loc->city,
+    'state'    => $options{state}    || $loc->state,
+    'zip'      => $options{zip}      || $loc->zip,
+    'country'  => $options{country}  || $loc->country,
+    'payby'    => $options{payby}    || $loc->payby,
+    'payinfo'  => $options{payinfo}  || $loc->payinfo,
+    'exp'      => $options{paydate}  || $loc->paydate,
+    'payname'  => $options{payname}  || $loc->payname,
     'amount'   => $amount,                         # consolidating
   } );
   
@@ -3027,7 +2932,8 @@ sub payment_info {
   $return{payname} = $self->payname
                      || ( $self->first. ' '. $self->get('last') );
 
-  $return{$_} = $self->get($_) for qw(address1 address2 city state zip);
+  $return{$_} = $self->bill_location->$_
+    for qw(address1 address2 city state zip);
 
   $return{payby} = $self->payby;
   $return{stateid_state} = $self->stateid_state;
@@ -4037,6 +3943,27 @@ sub name {
   $name;
 }
 
+=item service_contact
+
+Returns the L<FS::contact> object for this customer that has the 'Service'
+contact class, or undef if there is no such contact.  Deprecated; don't use
+this in new code.
+
+=cut
+
+sub service_contact {
+  my $self = shift;
+  if ( !exists($self->{service_contact}) ) {
+    my $classnum = $self->scalar_sql(
+      'SELECT classnum FROM contact_class WHERE classname = \'Service\''
+    ) || 0; #if it's zero, qsearchs will return nothing
+    $self->{service_contact} = qsearchs('contact', { 
+        'classnum' => $classnum, 'custnum' => $self->custnum
+      }) || undef;
+  }
+  $self->{service_contact};
+}
+
 =item ship_name
 
 Returns a name string for this (service/shipping) contact, either
@@ -4046,13 +3973,10 @@ Returns a name string for this (service/shipping) contact, either
 
 sub ship_name {
   my $self = shift;
-  if ( $self->get('ship_last') ) { 
-    my $name = $self->ship_contact;
-    $name = $self->ship_company. " ($name)" if $self->ship_company;
-    $name;
-  } else {
-    $self->name;
-  }
+
+  my $name = $self->ship_contact;
+  $name = $self->company. " ($name)" if $self->company;
+  $name;
 }
 
 =item name_short
@@ -4075,13 +3999,9 @@ or "First Last".
 
 sub ship_name_short {
   my $self = shift;
-  if ( $self->get('ship_last') ) { 
-    $self->ship_company !~ /^\s*$/
-      ? $self->ship_company
-      : $self->ship_contact_firstlast;
-  } else {
-    $self->name_company_or_firstlast;
-  }
+  $self->service_contact 
+    ? $self->ship_contact_firstlast 
+    : $self->name_company_or_firstlast;
 }
 
 =item contact
@@ -4103,9 +4023,8 @@ Returns this customer's full (shipping) contact name only, "Last, First"
 
 sub ship_contact {
   my $self = shift;
-  $self->get('ship_last')
-    ? $self->get('ship_last'). ', '. $self->ship_first
-    : $self->contact;
+  my $contact = $self->service_contact || $self;
+  $contact->get('last') . ', ' . $contact->get('first');
 }
 
 =item contact_firstlast
@@ -4127,9 +4046,8 @@ Returns this customer's full (shipping) contact name only, "First Last".
 
 sub ship_contact_firstlast {
   my $self = shift;
-  $self->get('ship_last')
-    ? $self->first. ' '. $self->get('ship_last')
-    : $self->contact_firstlast;
+  my $contact = $self->service_contact || $self;
+  $contact->get('first') . ' '. $contact->get('last');
 }
 
 =item country_full
@@ -5113,6 +5031,8 @@ sub process_censustract_update {
 #    upgrade journal again?  this is also an ancient problem
 # - otaker upgrade?  journal and call it good?  (double check to make sure
 #    we're not still setting otaker here)
+#
+#only going to get worse with new location stuff...
 
 sub _upgrade_data { #class method
   my ($class, %opts) = @_;
@@ -5141,12 +5061,13 @@ sub _upgrade_data { #class method
   }
 
   local($ignore_expired_card) = 1;
-  local($ignore_illegal_zip) = 1;
   local($ignore_banned_card) = 1;
   local($skip_fuzzyfiles) = 1;
   local($import) = 1; #prevent automatic geocoding (need its own variable?)
   $class->_upgrade_otaker(%opts);
 
+  FS::cust_main::Location->_upgrade_data(%opts);
+
 }
 
 =back