allow certain minor location edits without moving packages, #940

11 files changed, 111 insertions, 90 deletions

diff --git a/FS/FS/cust_bill_pkg.pm b/FS/FS/cust_bill_pkg.pm
index d8cbf59..0c8c0bb 100644
--- a/FS/FS/cust_bill_pkg.pm
+++ b/FS/FS/cust_bill_pkg.pm
@@ -1104,15 +1104,12 @@ sub upgrade_tax_location {
     delete @hash{qw(censustract censusyear latitude longitude coord_auto)};
 
     $hash{custnum} = $h_cust_main->custnum;
-    my $tax_loc = FS::cust_location->new_or_existing(\%hash);
-    if ( !$tax_loc->locationnum ) {
-      $tax_loc->disabled('Y');
-      my $error = $tax_loc->insert;
-      if ( $error ) {
-        warn "couldn't create historical location record for cust#".
-        $h_cust_main->custnum.": $error\n";
-        next INVOICE;
-      }
+    my $tax_loc = FS::cust_location->new(\%hash);
+    my $error = $tax_loc->find_or_insert || $tax_loc->disable_if_unused;
+    if ( $error ) {
+      warn "couldn't create historical location record for cust#".
+      $h_cust_main->custnum.": $error\n";
+      next INVOICE;
     }
     my $exempt_cust = 1 if $h_cust_main->tax;
 
diff --git a/FS/FS/cust_location.pm b/FS/FS/cust_location.pm
index b12a161..4560716 100644
--- a/FS/FS/cust_location.pm
+++ b/FS/FS/cust_location.pm
@@ -104,33 +104,93 @@ points to.  You can ask the object for a copy with the I<hash> method.
 
 sub table { 'cust_location'; }
 
-=item new_or_existing HASHREF
+=item find_or_insert
 
-Returns an existing location matching the customer and address fields in 
-HASHREF, if one exists; otherwise returns a new location containing those 
-fields.  The following fields must match: address1, address2, city, county,
-state, zip, country, geocode, disabled.  Other fields are only required
-to match if they're specified in HASHREF.
+Finds an existing location matching the customer and address values in this
+location, if one exists, and sets the contents of this location equal to that
+one (including its locationnum).
 
-The new location will not be inserted; the calling code must call C<insert>
-(or a method such as C<move_to>) to insert it, and check for errors at that
-point.
+If an existing location is not found, this one I<will> be inserted.  (This is a
+change from the "new_or_existing" method that this replaces.)
+
+The following fields are considered "essential" and I<must> match: custnum,
+address1, address2, city, county, state, zip, country, location_number,
+location_type, location_kind.  Disabled locations will be found only if this
+location is set to disabled.
+
+If 'coord_auto' is null, and latitude and longitude are not null, then 
+latitude and longitude are also essential fields.
+
+All other fields are considered "non-essential".  If a non-essential field is
+empty in this location, it will be ignored in determining whether an existing
+location matches.
+
+If a non-essential field is non-empty in this location, existing locations 
+that contain a different non-empty value for that field will not match.  An 
+existing location in which the field is I<empty> will match, but will be 
+updated in-place with the value of that field.
+
+Returns an error string if inserting or updating a location failed.
+
+It is unfortunately hard to determine if this created a new location or not.
 
 =cut
 
-sub new_or_existing {
-  my $class = shift;
-  my %hash = ref($_[0]) ? %{$_[0]} : @_;
-  # if coords are empty, then it doesn't matter if they're auto or not
-  if ( !$hash{'latitude'} and !$hash{'longitude'} ) {
-    delete $hash{'coord_auto'};
+sub find_or_insert {
+  my $self = shift;
+
+  my @essential = (qw(custnum address1 address2 city county state zip country
+    location_number location_type location_kind disabled));
+
+  if ( !$self->coord_auto and $self->latitude and $self->longitude ) {
+    push @essential, qw(latitude longitude);
+    # but NOT coord_auto; if the latitude and longitude match the geocoded
+    # values then that's good enough
   }
-  foreach ( qw(address1 address2 city county state zip country geocode
-              disabled ) ) {
-    # empty fields match only empty fields
-    $hash{$_} = '' if !defined($hash{$_});
+
+  # put nonempty, nonessential fields/values into this hash
+  my %nonempty = map { $_ => $self->get($_) }
+                 grep {$self->get($_)} $self->fields;
+  delete @nonempty{@essential};
+  delete $nonempty{'locationnum'};
+
+  my %hash = map { $_ => $self->get($_) } @essential;
+  my @matches = qsearch('cust_location', \%hash);
+
+  # consider candidate locations
+  MATCH: foreach my $old (@matches) {
+    my $reject = 0;
+    foreach my $field (keys %nonempty) {
+      my $old_value = $old->get($field);
+      if ( length($old_value) > 0 ) {
+        if ( $field eq 'latitude' or $field eq 'longitude' ) {
+          # special case, because these are decimals
+          if ( abs($old_value - $nonempty{$field}) > 0.000001 ) {
+            $reject = 1;
+          }
+        } elsif ( $old_value ne $nonempty{$field} ) {
+          $reject = 1;
+        }
+      } else {
+        # it's empty in $old, has a value in $self
+        $old->set($field, $nonempty{$field});
+      }
+      next MATCH if $reject;
+    } # foreach $field
+
+    if ( $old->modified ) {
+      my $error = $old->replace;
+      return $error if $error;
+    }
+    # set $self equal to $old
+    foreach ($self->fields) {
+      $self->set($_, $old->get($_));
+    }
+    return "";
   }
-  return qsearchs('cust_location', \%hash) || $class->new(\%hash);
+
+  # didn't find a match
+  return $self->insert;
 }
 
 =item insert
diff --git a/FS/FS/cust_main.pm b/FS/FS/cust_main.pm
index 1cf0365..f21932c 100644
--- a/FS/FS/cust_main.pm
+++ b/FS/FS/cust_main.pm
@@ -1509,43 +1509,17 @@ sub replace {
     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 ) {
+    # find the existing location if there is one
+    $new_loc->set('custnum' => $self->custnum);
+    my $error = $new_loc->find_or_insert;
+    if ( $error ) {
       $dbh->rollback if $oldAutoCommit;
-      return "$l belongs to customer ".$new_loc->custnum;
+      return $error;
     }
-    # 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
 
+  # replace the customer record
   my $error = $self->SUPER::replace($old);
 
   if ( $error ) {
diff --git a/FS/FS/cust_main/Packages.pm b/FS/FS/cust_main/Packages.pm
index 41ef228..8484df5 100644
--- a/FS/FS/cust_main/Packages.pm
+++ b/FS/FS/cust_main/Packages.pm
@@ -130,13 +130,10 @@ sub order_pkg {
 
   } elsif ( $opt->{'cust_location'} ) {
 
-    if ( ! $opt->{'cust_location'}->locationnum ) {
-      # not inserted yet
-      my $error = $opt->{'cust_location'}->insert;
-      if ( $error ) {
-        $dbh->rollback if $oldAutoCommit;
-        return "inserting cust_location (transaction rolled back): $error";
-      }
+    my $error = $opt->{'cust_location'}->find_or_insert;
+    if ( $error ) {
+      $dbh->rollback if $oldAutoCommit;
+      return "inserting cust_location (transaction rolled back): $error";
     }
     $cust_pkg->locationnum($opt->{'cust_location'}->locationnum);
 
diff --git a/FS/FS/cust_pkg.pm b/FS/FS/cust_pkg.pm
index 4464aa5..c49007c 100644
--- a/FS/FS/cust_pkg.pm
+++ b/FS/FS/cust_pkg.pm
@@ -1773,19 +1773,13 @@ sub change {
   $hash{"change_$_"}  = $self->$_()
     foreach qw( pkgnum pkgpart locationnum );
 
-  if ( $opt->{'cust_location'} &&
-       ( ! $opt->{'locationnum'} || $opt->{'locationnum'} == -1 ) ) {
-
-    if ( ! $opt->{'cust_location'}->locationnum ) {
-      # not inserted yet
-      $error = $opt->{'cust_location'}->insert;
-      if ( $error ) {
-        $dbh->rollback if $oldAutoCommit;
-        return "inserting cust_location (transaction rolled back): $error";
-      }
+  if ( $opt->{'cust_location'} ) {
+    $error = $opt->{'cust_location'}->find_or_insert;
+    if ( $error ) {
+      $dbh->rollback if $oldAutoCommit;
+      return "inserting cust_location (transaction rolled back): $error";
     }
     $opt->{'locationnum'} = $opt->{'cust_location'}->locationnum;
-
   }
 
   # whether to override pkgpart checking on the new package
diff --git a/FS/FS/svc_phone.pm b/FS/FS/svc_phone.pm
index 3cc1adc..bab8537 100644
--- a/FS/FS/svc_phone.pm
+++ b/FS/FS/svc_phone.pm
@@ -288,9 +288,8 @@ sub insert {
 
   #false laziness w/cust_pkg.pm... move this to location_Mixin?  that would
   #make it more of a base class than a mixin... :)
-  if ( $options{'cust_location'}
-         && ( ! $self->locationnum || $self->locationnum == -1 ) ) {
-    my $error = $options{'cust_location'}->insert;
+  if ( $options{'cust_location'} ) {
+    my $error = $options{'cust_location'}->find_or_insert;
     if ( $error ) {
       $dbh->rollback if $oldAutoCommit;
       return "inserting cust_location (transaction rolled back): $error";
diff --git a/httemplate/edit/process/change-cust_pkg.html b/httemplate/edit/process/change-cust_pkg.html
index 77f261d..c893f13 100644
--- a/httemplate/edit/process/change-cust_pkg.html
+++ b/httemplate/edit/process/change-cust_pkg.html
@@ -32,7 +32,7 @@ my %change = map { $_ => scalar($cgi->param($_)) }
 $change{'keep_dates'} = 1;
 
 if ( $cgi->param('locationnum') == -1 ) {
-  my $cust_location = FS::cust_location->new_or_existing({
+  my $cust_location = FS::cust_location->new({
     'custnum' => $cust_pkg->custnum,
     map { $_ => scalar($cgi->param($_)) }
         qw( address1 address2 city county state zip country )
diff --git a/httemplate/edit/process/cust_location.cgi b/httemplate/edit/process/cust_location.cgi
index 56c3968..fd1b874 100644
--- a/httemplate/edit/process/cust_location.cgi
+++ b/httemplate/edit/process/cust_location.cgi
@@ -28,12 +28,12 @@ my $cust_location = qsearchs({
 });
 die "unknown locationnum $locationnum" unless $cust_location;
 
-my $new = FS::cust_location->new_or_existing({
+my $new = FS::cust_location->new({
   custnum     => $cust_location->custnum,
   prospectnum => $cust_location->prospectnum,
   map { $_ => scalar($cgi->param($_)) } FS::cust_main->location_fields
 });
-
-my $error = $cust_location->move_to($new);
+my $error = $new->find_or_insert;
+$error  ||= $cust_location->move_to($new);
 
 </%init>
diff --git a/httemplate/edit/process/cust_main.cgi b/httemplate/edit/process/cust_main.cgi
index c1f8155..d295ed3 100755
--- a/httemplate/edit/process/cust_main.cgi
+++ b/httemplate/edit/process/cust_main.cgi
@@ -11,7 +11,7 @@
 <%once>
 
 my $me = '[edit/process/cust_main.cgi]';
-my $DEBUG = 0;
+my $DEBUG = 1;
 
 </%once>
 <%init>
@@ -83,7 +83,7 @@ for my $pre (qw(bill ship)) {
   }
   $hash{'custnum'} = $cgi->param('custnum');
   warn Dumper \%hash if $DEBUG;
-  $locations{$pre} = FS::cust_location->new_or_existing(\%hash);
+  $locations{$pre} = FS::cust_location->new(\%hash);
 }
 
 if ( ($cgi->param('same') || '') eq 'Y' ) {
diff --git a/httemplate/edit/process/quick-cust_pkg.cgi b/httemplate/edit/process/quick-cust_pkg.cgi
index 0cc17d3..14dbda1 100644
--- a/httemplate/edit/process/quick-cust_pkg.cgi
+++ b/httemplate/edit/process/quick-cust_pkg.cgi
@@ -155,7 +155,7 @@ if ( $quotationnum ) {
   }
 
   if ( $locationnum == -1 ) {
-    my $cust_location = FS::cust_location->new_or_existing({
+    my $cust_location = FS::cust_location->new({
       map { $_ => scalar($cgi->param($_)) }
           ('custnum', FS::cust_main->location_fields)
     });
diff --git a/httemplate/edit/process/svc_phone.html b/httemplate/edit/process/svc_phone.html
index 9983ea2..09398fd 100644
--- a/httemplate/edit/process/svc_phone.html
+++ b/httemplate/edit/process/svc_phone.html
@@ -40,7 +40,7 @@ my $args_callback = sub {
 
   my %opt = ();
   if ( $cgi->param('locationnum') == -1 ) {
-    my $cust_location = FS::cust_location->new_or_existing({
+    my $cust_location = FS::cust_location->new({
       map { $_ => scalar($cgi->param($_)) }
           qw( custnum address1 address2 city county state zip country )
     });