fix(representatives:update): make address validation more lenient and allow partial updates (#24435)

Author: raudabaugh

modules/veteran/app/services/veteran/address_preprocessor.rb

@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+module Veteran
+  module AddressPreprocessor
+    PO_BOX_REGEX = /P\.?O\.?\s*BOX\s*\d+/i
+    ROOM_SUITE_REGEX = /,?\s*(Room|Rm|Suite|Ste?)\.?\s*\d+/i
+
+    module_function
+
+    # Mutates a shallow copy of the address hash and returns it
+    def clean(address)
+      return address unless address.is_a?(Hash) && address['address_line1'].present?
+
+      cleaned = address.dup
+      line1 = cleaned['address_line1'].to_s.dup
+
+      # Extract PO Box to address_line1
+      if (m = line1.match(PO_BOX_REGEX))
+        po = m.to_s.strip
+        cleaned['address_line1'] = po
+        cleaned['address_line2'] = nil
+        return cleaned
+      end
+
+      # Extract suite/room to address_line2
+      address_line, secondary_designator = extract_secondary_designator(line1)
+      if address_line.blank?
+        cleaned['address_line1'] = nil
+        return cleaned
+      end
+
+      cleaned['address_line1'] = address_line
+      cleaned['address_line2'] = cleaned['address_line2'].presence || secondary_designator
+
+      cleaned
+    end
+
+    def extract_secondary_designator(line1)
+      m = line1.match(ROOM_SUITE_REGEX)
+      return [line1.strip, nil] unless m
+
+      address_line = line1.sub(ROOM_SUITE_REGEX, '').strip
+      secondary_designator = m.to_s.strip.sub(/^,\s*/, '')
+
+      [address_line, secondary_designator]
+    end
+  end
+end

modules/veteran/app/sidekiq/representatives/update.rb

@@ -45,22 +45,23 @@ def process_rep_data(rep_data)
 
         if rep_data['address_changed']
           api_response = get_best_address_candidate(rep_data['address'])
-
-          # don't update the record if there is not a valid address with non-zero lat and long at this point
+          # If address validation fails, log and continue to update non-address fields (email/phone)
           if api_response.nil?
-            return
+            log_error("Address validation failed for Rep: #{rep_data['id']}. Proceeding to update non-address fields.")
           else
             address_validation_api_response = api_response
           end
         end
 
-        update_rep_record(rep_data, address_validation_api_response)
+        updated_flags = update_rep_record(rep_data, address_validation_api_response)
       rescue => e
         log_error("Update failed for Rep id: #{rep_data}: #{e.message}")
         return
       end
 
-      update_flagged_records(rep_data)
+      # Update flagged records only for fields that were actually updated
+      updated_flags['representative_id'] = rep_data['id'] if updated_flags.is_a?(Hash)
+      update_flagged_records(updated_flags || {})
     end
 
     def record_can_be_updated?(rep_data)
@@ -73,16 +74,18 @@ def record_can_be_updated?(rep_data)
     def build_validation_address(address)
       validation_model = VAProfile::Models::ValidationAddress
 
+      cleaned = Veteran::AddressPreprocessor.clean(address)
+
       validation_model.new(
-        address_pou: address['address_pou'],
-        address_line1: address['address_line1'],
-        address_line2: address['address_line2'],
-        address_line3: address['address_line3'],
-        city: address['city'],
-        state_code: address['state']['state_code'],
-        zip_code: address['zip_code5'],
-        zip_code_suffix: address['zip_code4'],
-        country_code_iso3: address['country_code_iso3']
+        address_pou: cleaned['address_pou'] || address['address_pou'],
+        address_line1: cleaned['address_line1'],
+        address_line2: cleaned['address_line2'],
+        address_line3: cleaned['address_line3'],
+        city: cleaned['city'] || address['city'],
+        state_code: cleaned.dig('state', 'state_code') || address.dig('state', 'state_code'),
+        zip_code: cleaned['zip_code5'] || address['zip_code5'],
+        zip_code_suffix: cleaned['zip_code4'] || address['zip_code4'],
+        country_code_iso3: cleaned['country_code_iso3'] || address['country_code_iso3']
       )
     end
 
@@ -106,25 +109,56 @@ def address_valid?(response)
     # @param rep_data [Hash] Original rep_data containing the address and other details.
     # @param api_response [Hash] The response from the address validation service.
     def update_rep_record(rep_data, api_response)
-      record =
-        Veteran::Service::Representative.find_by(representative_id: rep_data['id'])
-      if record.nil?
-        raise StandardError, 'Representative not found.'
-      else
-        address_attributes = rep_data['address_changed'] ? build_address_attributes(rep_data, api_response) : {}
-        email_attributes = rep_data['email_changed'] ? build_email_attributes(rep_data) : {}
-        phone_attributes = rep_data['phone_number_changed'] ? build_phone_attributes(rep_data) : {}
-        record.update(merge_attributes(address_attributes, email_attributes, phone_attributes))
-      end
+      record = Veteran::Service::Representative.find_by(representative_id: rep_data['id'])
+      raise StandardError, 'Representative not found.' if record.nil?
+
+      attributes, flags = build_rep_update_payload(rep_data, api_response)
+      record.update(attributes)
+      flags
     end
 
-    # Updates flags for the representative's records based on changes in address, email, or phone number.
-    # @param rep_data [Hash] The representative data including the id and flags for changes.
-    def update_flagged_records(rep_data)
-      representative_id = rep_data['id']
-      update_flags(representative_id, 'address') if rep_data['address_changed']
-      update_flags(representative_id, 'email') if rep_data['email_changed']
-      update_flags(representative_id, 'phone_number') if rep_data['phone_number_changed']
+    # Build attributes hash and flags indicating which fields changed
+    # @return [Array<Hash, Hash>] first element is attributes, second is flags
+    def build_rep_update_payload(rep_data, api_response)
+      flags = {}
+
+      address_attrs = if rep_data['address_changed'] && api_response.present?
+                        flags['address'] = true
+                        build_address_attributes(rep_data, api_response)
+                      else
+                        {}
+                      end
+
+      email_attrs = if rep_data['email_changed']
+                      flags['email'] = true
+                      build_email_attributes(rep_data)
+                    else
+                      {}
+                    end
+
+      phone_attrs = if rep_data['phone_number_changed']
+                      flags['phone_number'] = true
+                      build_phone_attributes(rep_data)
+                    else
+                      {}
+                    end
+
+      [merge_attributes(address_attrs, email_attrs, phone_attrs), flags]
+    end
+
+    # Updates flags for the representative's records based on which fields were actually updated.
+    # @param updated_flags [Hash] Hash with keys 'address', 'email', 'phone_number' set to true when updated.
+    def update_flagged_records(updated_flags)
+      return unless updated_flags.is_a?(Hash)
+
+      representative_id = updated_flags['representative_id']
+
+      # If representative_id is missing, we can't update flags
+      return if representative_id.blank?
+
+      update_flags(representative_id, 'address') if updated_flags['address']
+      update_flags(representative_id, 'email') if updated_flags['email']
+      update_flags(representative_id, 'phone_number') if updated_flags['phone_number']
     end
 
     # Updates the flags for a representative's contact data indicating a change.
@@ -160,51 +194,35 @@ def merge_attributes(address, email, phone)
 
     # Builds the attributes for the record update from the address, geocode, and metadata.
     # @param address [Hash] Address details from the validation response.
-    # @param geocode [Hash] Geocode details from the validation response.
-    # @param meta [Hash] Metadata about the address from the validation response.
     # @return [Hash] The attributes to update the record with.
-    def build_address(address, geocode, meta)
-      {
-        address_type: meta['address_type'],
-        address_line1: address['address_line1'],
-        address_line2: address['address_line2'],
-        address_line3: address['address_line3'],
-        city: address['city'],
-        province: address['state_province']['name'],
-        state_code: address['state_province']['code'],
-        zip_code: address['zip_code5'],
-        zip_suffix: address['zip_code4'],
-        country_code_iso3: address['country']['iso3_code'],
-        country_name: address['country']['name'],
-        county_name: address.dig('county', 'name'),
-        county_code: address.dig('county', 'county_fips_code'),
-        lat: geocode['latitude'],
-        long: geocode['longitude'],
-        location: "POINT(#{geocode['longitude']} #{geocode['latitude']})"
-      }
-    end
-
     def build_v3_address(address)
       {
         address_type: address['address_type'],
         address_line1: address['address_line1'],
         address_line2: address['address_line2'],
         address_line3: address['address_line3'],
         city: address['city_name'],
-        province: address['state']['state_name'],
-        state_code: address['state']['state_code'],
+        province: address.dig('state', 'state_name'),
+        state_code: address.dig('state', 'state_code'),
         zip_code: address['zip_code5'],
         zip_suffix: address['zip_code4'],
-        country_code_iso3: address['country']['iso3_code'],
-        country_name: address['country']['country_name'],
+        country_code_iso3: address.dig('country', 'iso3_code'),
+        country_name: address.dig('country', 'country_name'),
         county_name: address.dig('county', 'county_name'),
         county_code: address.dig('county', 'county_code'),
-        lat: address['geocode']['latitude'],
-        long: address['geocode']['longitude'],
-        location: "POINT(#{address['geocode']['longitude']} #{address['geocode']['latitude']})"
+        lat: address.dig('geocode', 'latitude'),
+        long: address.dig('geocode', 'longitude'),
+        location: build_point(address.dig('geocode', 'longitude'), address.dig('geocode', 'latitude'))
       }
     end
 
+    # Build a WKT point string if both coordinates are present
+    def build_point(longitude, latitude)
+      return nil if longitude.blank? || latitude.blank?
+
+      "POINT(#{longitude} #{latitude})"
+    end
+
     # Logs an error to Datadog and adds an error to the array that will be logged to slack.
     # @param error [Exception] The error string to be logged.
     def log_error(error, send_to_slack: false)
@@ -252,9 +270,7 @@ def modified_validation(address, retry_count)
     # @param response [Hash, Nil] the response from the address validation service
     # @return [Boolean]
     def retriable?(response)
-      return true if response.blank?
-
-      !address_valid?(response) || lat_long_zero?(response)
+      response.blank? || !address_valid?(response) || lat_long_zero?(response)
     end
 
     # Retry address validation
@@ -297,21 +313,16 @@ def get_best_address_candidate(rep_address)
         raise
       end
 
-      return nil unless address_valid?(original_response)
-
-      # retry validation if we get zero as the coordinates - this should indicate some warning with validation that
-      #   is typically seen with addresses that mix street addresses with P.O. Boxes
-      if lat_long_zero?(original_response)
+      # If the original response is blank, invalid, or has zero coords, attempt retry logic which will
+      # try modified address lines (including preprocessor-extracted PO Box) before giving up.
+      if retriable?(original_response)
         retry_response = retry_validation(rep_address)
 
-        if retriable?(retry_response)
-          nil
-        else
-          retry_response
-        end
-      else
-        original_response
+        return retriable?(retry_response) ? nil : retry_response
       end
+
+      # If we get here the original response was valid and not retriable (e.g. has non-zero coords)
+      original_response
     end
 
     # Determine if the backend exception represents a candidate address not found scenario

modules/veteran/spec/services/address_preprocessor_spec.rb

@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe Veteran::AddressPreprocessor do
+  describe '.clean' do
+    it 'extracts room into address_line2' do
+      addr = { 'address_line1' => '11000 Wilshire Blvd., Rm 509', 'address_line2' => nil }
+      cleaned = described_class.clean(addr)
+      expect(cleaned['address_line1']).to eq('11000 Wilshire Blvd.')
+      expect(cleaned['address_line2']).to eq('Rm 509')
+    end
+
+    it 'extracts PO Box and removes prefix' do
+      addr = { 'address_line1' => '123 Main St Suite 5 PO Box 100', 'address_line2' => nil }
+      cleaned = described_class.clean(addr)
+      expect(cleaned['address_line1'].downcase).to include('po box 100')
+      expect(cleaned['address_line2']).to be_nil
+    end
+
+    it 'returns original when no changes' do
+      addr = { 'address_line1' => '123 East Main St', 'address_line2' => 'Suite 2' }
+      expect(described_class.clean(addr)).to eq(addr)
+    end
+
+    it 'returns PO Box only when room and PO Box present' do
+      addr = { 'address_line1' => 'PO Box 123, Rm 5', 'address_line2' => nil }
+      cleaned = described_class.clean(addr)
+      expect(cleaned['address_line1']).to eq('PO Box 123')
+      expect(cleaned['address_line2']).to be_nil
+    end
+
+    it 'returns nil address lines when only room is present' do
+      addr = { 'address_line1' => 'Rm 5', 'address_line2' => nil }
+      cleaned = described_class.clean(addr)
+      expect(cleaned['address_line1']).to be_nil
+      expect(cleaned['address_line2']).to be_nil
+    end
+  end
+end