From 857f1da9429da61e7d44f2a25312b3e528f12406 Mon Sep 17 00:00:00 2001
From: Eugene Burmakin <iamfrey@gmail.com>
Date: Mon, 12 May 2025 23:36:46 +0200
Subject: [PATCH] Unify name builder usage

---
 app/serializers/api/place_serializer.rb       |  20 +--
 app/services/visits/detector.rb               |   8 +-
 app/services/visits/names/builder.rb          |  68 ++++++++++
 app/services/visits/names/fetcher.rb          |  56 ++++++++
 app/services/visits/names/suggester.rb        |  59 ++++++++
 app/services/visits/place_finder.rb           |  38 +++---
 app/services/visits/place_name_suggester.rb   |  70 ----------
 spec/services/visits/names/builder_spec.rb    | 126 ++++++++++++++++++
 .../suggester_spec.rb}                        |   2 +-
 spec/services/visits/place_finder_spec.rb     |   2 +-
 10 files changed, 349 insertions(+), 100 deletions(-)
 create mode 100644 app/services/visits/names/builder.rb
 create mode 100644 app/services/visits/names/fetcher.rb
 create mode 100644 app/services/visits/names/suggester.rb
 delete mode 100644 app/services/visits/place_name_suggester.rb
 create mode 100644 spec/services/visits/names/builder_spec.rb
 rename spec/services/visits/{place_name_suggester_spec.rb => names/suggester_spec.rb} (98%)

diff --git a/app/serializers/api/place_serializer.rb b/app/serializers/api/place_serializer.rb
index 2857379a..787d5815 100644
--- a/app/serializers/api/place_serializer.rb
+++ b/app/serializers/api/place_serializer.rb
@@ -7,15 +7,17 @@ class Api::PlaceSerializer
 
   def call
     {
-      id: place.id,
-      name: place.name,
-      longitude: place.lon,
-      latitude: place.lat,
-      city: place.city,
-      country: place.country,
-      source: place.source,
-      geodata: place.geodata,
-      reverse_geocoded_at: place.reverse_geocoded_at
+      id:         place.id,
+      name:       place.name,
+      longitude:  place.lon,
+      latitude:   place.lat,
+      city:       place.city,
+      country:    place.country,
+      source:     place.source,
+      geodata:    place.geodata,
+      reverse_geocoded_at: place.reverse_geocoded_at,
+      created_at: place.created_at,
+      updated_at: place.updated_at
     }
   end
 
diff --git a/app/services/visits/detector.rb b/app/services/visits/detector.rb
index eced74ba..2a75855a 100644
--- a/app/services/visits/detector.rb
+++ b/app/services/visits/detector.rb
@@ -11,7 +11,7 @@ module Visits
 
     def initialize(points)
       @points = points
-      @place_name_suggester = PlaceNameSuggester
+      @place_name_suggester = Visits::Names::Suggester
     end
 
     def detect_potential_visits
@@ -90,7 +90,7 @@ module Visits
         center_lat: center[0],
         center_lon: center[1],
         radius: calculate_visit_radius(points, center),
-        suggested_name: suggest_place_name(points)
+        suggested_name: suggest_place_name(points) || fetch_place_name(center)
       )
     end
 
@@ -114,5 +114,9 @@ module Visits
     def suggest_place_name(points)
       place_name_suggester.new(points).call
     end
+
+    def fetch_place_name(center)
+      Visits::Names::Fetcher.new(center).call
+    end
   end
 end
diff --git a/app/services/visits/names/builder.rb b/app/services/visits/names/builder.rb
new file mode 100644
index 00000000..c3b6bf93
--- /dev/null
+++ b/app/services/visits/names/builder.rb
@@ -0,0 +1,68 @@
+# frozen_string_literal: true
+
+module Visits
+  module Names
+    # Builds descriptive names for places from geodata features
+    class Builder
+      def self.build_from_properties(properties)
+        return nil if properties.blank?
+
+        name_components = [
+          properties['name'],
+          properties['street'],
+          properties['housenumber'],
+          properties['city'],
+          properties['state']
+        ].compact.reject(&:empty?).uniq
+
+        name_components.any? ? name_components.join(', ') : nil
+      end
+
+      def initialize(features, feature_type, name)
+        @features = features
+        @feature_type = feature_type
+        @name = name
+      end
+
+      def call
+        return nil if features.blank? || feature_type.blank? || name.blank?
+        return nil unless feature
+
+        [
+          name,
+          properties['street'],
+          properties['city'],
+          properties['state']
+        ].compact.uniq.join(', ')
+      end
+
+      private
+
+      attr_reader :features, :feature_type, :name
+
+      def feature
+        @feature ||= find_feature
+      end
+
+      def find_feature
+        features.find do |f|
+          f.dig('properties', 'type') == feature_type &&
+            f.dig('properties', 'name') == name
+        end || find_feature_by_osm_value
+      end
+
+      def find_feature_by_osm_value
+        features.find do |f|
+          f.dig('properties', 'osm_value') == feature_type &&
+            f.dig('properties', 'name') == name
+        end
+      end
+
+      def properties
+        return {} unless feature && feature['properties'].is_a?(Hash)
+
+        feature['properties']
+      end
+    end
+  end
+end
diff --git a/app/services/visits/names/fetcher.rb b/app/services/visits/names/fetcher.rb
new file mode 100644
index 00000000..6a3ed157
--- /dev/null
+++ b/app/services/visits/names/fetcher.rb
@@ -0,0 +1,56 @@
+# frozen_string_literal: true
+
+module Visits
+  module Names
+    # Fetches names for places from reverse geocoding API
+    class Fetcher
+      def initialize(center)
+        @center = center
+      end
+
+      def call
+        return nil if geocoder_results.blank?
+
+        build_place_name
+      end
+
+      private
+
+      attr_reader :center
+
+      def geocoder_results
+        @geocoder_results ||= Geocoder.search(
+          center, limit: 10, distance_sort: true, radius: 1, units: ::DISTANCE_UNIT
+        )
+      end
+
+      def build_place_name
+        return nil if geocoder_results.first&.data.blank?
+
+        properties = geocoder_results.first.data['properties']
+        return nil unless properties.present?
+
+        # First try the direct properties approach
+        name = Visits::Names::Builder.build_from_properties(properties)
+        return name if name.present?
+
+        # Fall back to the instance-based approach
+        return nil unless properties['name'] && properties['osm_value']
+
+        Visits::Names::Builder.new(
+          features,
+          properties['osm_value'],
+          properties['name']
+        ).call
+      end
+
+      def features
+        geocoder_results.map do |result|
+          {
+            'properties' => result.data['properties']
+          }
+        end.compact
+      end
+    end
+  end
+end
diff --git a/app/services/visits/names/suggester.rb b/app/services/visits/names/suggester.rb
new file mode 100644
index 00000000..f3c7a945
--- /dev/null
+++ b/app/services/visits/names/suggester.rb
@@ -0,0 +1,59 @@
+# frozen_string_literal: true
+
+module Visits
+  module Names
+    # Suggests names for places based on geodata from tracked points
+    class Suggester
+      def initialize(points)
+        @points = points
+      end
+
+      def call
+        geocoded_points = extract_geocoded_points(points)
+        return nil if geocoded_points.empty?
+
+        features = extract_features(geocoded_points)
+        return nil if features.empty?
+
+        most_common_type = find_most_common_feature_type(features)
+        return nil unless most_common_type
+
+        most_common_name = find_most_common_name(features, most_common_type)
+        return nil if most_common_name.blank?
+
+        Visits::Names::Builder.new(
+          features, most_common_type, most_common_name
+        ).call
+      end
+
+      private
+
+      attr_reader :points
+
+      def extract_geocoded_points(points)
+        points.select { |p| p.geodata.present? && !p.geodata.empty? }
+      end
+
+      def extract_features(geocoded_points)
+        geocoded_points.flat_map do |point|
+          next [] unless point.geodata['features'].is_a?(Array)
+
+          point.geodata['features']
+        end.compact
+      end
+
+      def find_most_common_feature_type(features)
+        feature_counts = features.group_by { |f| f.dig('properties', 'type') }
+                                 .transform_values(&:size)
+        feature_counts.max_by { |_, count| count }&.first
+      end
+
+      def find_most_common_name(features, feature_type)
+        common_features = features.select { |f| f.dig('properties', 'type') == feature_type }
+        name_counts = common_features.group_by { |f| f.dig('properties', 'name') }
+                                     .transform_values(&:size)
+        name_counts.max_by { |_, count| count }&.first
+      end
+    end
+  end
+end
diff --git a/app/services/visits/place_finder.rb b/app/services/visits/place_finder.rb
index 72a35e72..86f0a547 100644
--- a/app/services/visits/place_finder.rb
+++ b/app/services/visits/place_finder.rb
@@ -51,7 +51,7 @@ module Visits
       return existing_by_location if existing_by_location
 
       # Then try by name if available
-      return nil unless name.present?
+      return nil if name.blank?
 
       Place.where(name: name)
            .near([lat, lon], SEARCH_RADIUS, :m)
@@ -64,16 +64,13 @@ module Visits
       lon = visit_data[:center_lon]
 
       # Get places from points' geodata
-      places_from_points = extract_places_from_points(visit_data[:points], lat, lon)
-
-      # Get places from external API
-      places_from_api = fetch_places_from_api(lat, lon)
+      places_from_points = extract_places_from_points(visit_data[:points])
 
       # Combine and deduplicate by name
       combined_places = []
 
       # Add API places first (usually better quality)
-      places_from_api.each do |api_place|
+      reverse_geocoded_places(lat, lon).each do |api_place|
         combined_places << api_place unless place_name_exists?(combined_places, api_place.name)
       end
 
@@ -86,7 +83,7 @@ module Visits
     end
 
     # Step 3: Extract places from points
-    def extract_places_from_points(points, center_lat, center_lon)
+    def extract_places_from_points(points)
       return [] if points.blank?
 
       # Filter points with geodata
@@ -101,7 +98,7 @@ module Visits
         places << place if place
       end
 
-      places.uniq { |place| place.name }
+      places.uniq(&:name)
     end
 
     # Step 4: Create place from point
@@ -141,7 +138,7 @@ module Visits
     end
 
     # Step 5: Fetch places from API
-    def fetch_places_from_api(lat, lon)
+    def reverse_geocoded_places(lat, lon)
       # Get broader search results from Geocoder
       geocoder_results = Geocoder.search([lat, lon], units: :km, limit: 20, distance_sort: true)
       return [] if geocoder_results.blank?
@@ -228,15 +225,22 @@ module Visits
     # Helper methods
 
     def build_place_name(properties)
-      name_components = [
-        properties['name'],
-        properties['street'],
-        properties['housenumber'],
-        properties['postcode'],
-        properties['city']
-      ].compact.reject(&:empty?).uniq
+      # First try building with our name builder
+      built_name = Visits::Names::Builder.build_from_properties(properties)
+      return built_name if built_name.present?
 
-      name_components.any? ? name_components.join(', ') : Place::DEFAULT_NAME
+      # Try using the instance-based approach as a fallback
+      features = [{ 'properties' => properties }]
+      feature_type = properties['type'] || properties['osm_value']
+      name = properties['name']
+
+      if feature_type.present? && name.present?
+        built_name = Visits::Names::Builder.new(features, feature_type, name).call
+        return built_name if built_name.present?
+      end
+
+      # Fallback to the default name if all else fails
+      Place::DEFAULT_NAME
     end
 
     def place_name_exists?(places, name)
diff --git a/app/services/visits/place_name_suggester.rb b/app/services/visits/place_name_suggester.rb
deleted file mode 100644
index d86e5573..00000000
--- a/app/services/visits/place_name_suggester.rb
+++ /dev/null
@@ -1,70 +0,0 @@
-# frozen_string_literal: true
-
-module Visits
-  # Suggests names for places based on geodata from tracked points
-  class PlaceNameSuggester
-    def initialize(points)
-      @points = points
-    end
-
-    def call
-      geocoded_points = extract_geocoded_points(points)
-      return nil if geocoded_points.empty?
-
-      features = extract_features(geocoded_points)
-      return nil if features.empty?
-
-      most_common_type = find_most_common_feature_type(features)
-      return nil unless most_common_type
-
-      most_common_name = find_most_common_name(features, most_common_type)
-      return nil if most_common_name.blank?
-
-      build_descriptive_name(features, most_common_type, most_common_name)
-    end
-
-    private
-
-    attr_reader :points
-
-    def extract_geocoded_points(points)
-      points.select { |p| p.geodata.present? && !p.geodata.empty? }
-    end
-
-    def extract_features(geocoded_points)
-      geocoded_points.flat_map do |point|
-        next [] unless point.geodata['features'].is_a?(Array)
-
-        point.geodata['features']
-      end.compact
-    end
-
-    def find_most_common_feature_type(features)
-      feature_counts = features.group_by { |f| f.dig('properties', 'type') }
-                               .transform_values(&:size)
-      feature_counts.max_by { |_, count| count }&.first
-    end
-
-    def find_most_common_name(features, feature_type)
-      common_features = features.select { |f| f.dig('properties', 'type') == feature_type }
-      name_counts = common_features.group_by { |f| f.dig('properties', 'name') }
-                                   .transform_values(&:size)
-      name_counts.max_by { |_, count| count }&.first
-    end
-
-    def build_descriptive_name(features, feature_type, name)
-      feature = features.find do |f|
-        f.dig('properties', 'type') == feature_type &&
-          f.dig('properties', 'name') == name
-      end
-
-      properties = feature['properties']
-      [
-        name,
-        properties['street'],
-        properties['city'],
-        properties['state']
-      ].compact.uniq.join(', ')
-    end
-  end
-end
diff --git a/spec/services/visits/names/builder_spec.rb b/spec/services/visits/names/builder_spec.rb
new file mode 100644
index 00000000..47d2586c
--- /dev/null
+++ b/spec/services/visits/names/builder_spec.rb
@@ -0,0 +1,126 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe Visits::Names::Builder do
+  describe '.build_from_properties' do
+    it 'builds a name from all available properties' do
+      properties = {
+        'name' => 'Coffee Shop',
+        'street' => 'Main St',
+        'housenumber' => '123',
+        'city' => 'New York',
+        'state' => 'NY'
+      }
+
+      result = described_class.build_from_properties(properties)
+      expect(result).to eq('Coffee Shop, Main St, 123, New York, NY')
+    end
+
+    it 'handles missing properties' do
+      properties = {
+        'name' => 'Coffee Shop',
+        'city' => 'New York',
+        'state' => 'NY'
+      }
+
+      result = described_class.build_from_properties(properties)
+      expect(result).to eq('Coffee Shop, New York, NY')
+    end
+
+    it 'deduplicates components' do
+      properties = {
+        'name' => 'New York Cafe',
+        'city' => 'New York',
+        'state' => 'NY'
+      }
+
+      result = described_class.build_from_properties(properties)
+      expect(result).to eq('New York Cafe, New York, NY')
+    end
+
+    it 'returns nil for empty properties' do
+      result = described_class.build_from_properties({})
+      expect(result).to be_nil
+    end
+
+    it 'returns nil for nil properties' do
+      result = described_class.build_from_properties(nil)
+      expect(result).to be_nil
+    end
+  end
+
+  describe '#call' do
+    subject { described_class.new(features, feature_type, name).call }
+
+    let(:feature_type) { 'amenity' }
+    let(:name) { 'Coffee Shop' }
+    let(:features) do
+      [
+        {
+          'properties' => {
+            'type' => 'amenity',
+            'name' => 'Coffee Shop',
+            'street' => '123 Main St',
+            'city' => 'San Francisco',
+            'state' => 'CA'
+          }
+        },
+        {
+          'properties' => {
+            'type' => 'park',
+            'name' => 'Central Park',
+            'city' => 'New York',
+            'state' => 'NY'
+          }
+        }
+      ]
+    end
+
+    it 'returns a descriptive name with all available components' do
+      expect(subject).to eq('Coffee Shop, 123 Main St, San Francisco, CA')
+    end
+
+    context 'when feature uses osm_value instead of type' do
+      let(:features) do
+        [
+          {
+            'properties' => {
+              'osm_value' => 'amenity',
+              'name' => 'Coffee Shop',
+              'street' => '123 Main St',
+              'city' => 'San Francisco',
+              'state' => 'CA'
+            }
+          }
+        ]
+      end
+
+      it 'finds the feature using osm_value' do
+        expect(subject).to eq('Coffee Shop, 123 Main St, San Francisco, CA')
+      end
+    end
+
+    context 'when no matching feature is found' do
+      let(:name) { 'Non-existent Shop' }
+
+      it 'returns nil' do
+        expect(subject).to be_nil
+      end
+    end
+
+    context 'with empty inputs' do
+      it 'returns nil for empty features' do
+        expect(described_class.new([], feature_type, name).call).to be_nil
+      end
+
+      it 'returns nil for blank feature_type' do
+        expect(described_class.new(features, '', name).call).to be_nil
+      end
+
+      it 'returns nil for blank name' do
+        expect(described_class.new(features, feature_type, '').call).to be_nil
+      end
+    end
+  end
+end
diff --git a/spec/services/visits/place_name_suggester_spec.rb b/spec/services/visits/names/suggester_spec.rb
similarity index 98%
rename from spec/services/visits/place_name_suggester_spec.rb
rename to spec/services/visits/names/suggester_spec.rb
index 8b326d8a..dff5ff91 100644
--- a/spec/services/visits/place_name_suggester_spec.rb
+++ b/spec/services/visits/names/suggester_spec.rb
@@ -2,7 +2,7 @@
 
 require 'rails_helper'
 
-RSpec.describe Visits::PlaceNameSuggester do
+RSpec.describe Visits::Names::Suggester do
   subject(:suggester) { described_class.new(points) }
 
   describe '#call' do
diff --git a/spec/services/visits/place_finder_spec.rb b/spec/services/visits/place_finder_spec.rb
index 493ceec5..b924ffae 100644
--- a/spec/services/visits/place_finder_spec.rb
+++ b/spec/services/visits/place_finder_spec.rb
@@ -75,7 +75,7 @@ RSpec.describe Visits::PlaceFinder do
 
       before do
         allow(Geocoder).to receive(:search).and_return([])
-        allow(subject).to receive(:fetch_places_from_api).and_return([])
+        allow(subject).to receive(:reverse_geocoded_places).and_return([])
       end
 
       it 'extracts and creates places from point geodata' do