Merge pull request #2872 from mkllnk/2765-fix-geocoding

2765 Fix geocoding to display enterprises on map

.rubocop_todo.yml

@@ -674,7 +674,6 @@ Layout/SpaceAroundOperators:
     - 'app/controllers/checkout_controller.rb'
     - 'app/helpers/admin/business_model_configuration_helper.rb'
     - 'app/jobs/update_billable_periods.rb'
-    - 'app/models/spree/address_decorator.rb'
     - 'app/overrides/remove_search_bar.rb'
     - 'app/overrides/remove_side_bar.rb'
     - 'app/overrides/replace_shipping_address_form_with_distributor_details.rb'
@@ -1441,7 +1440,6 @@ Rails/HasManyOrHasOneDependent:
     - 'app/models/customer.rb'
     - 'app/models/enterprise.rb'
     - 'app/models/order_cycle.rb'
-    - 'app/models/spree/address_decorator.rb'
     - 'app/models/spree/adjustment_decorator.rb'
     - 'app/models/spree/order_decorator.rb'
     - 'app/models/spree/payment_method_decorator.rb'
@@ -1955,7 +1953,6 @@ Style/HashSyntax:
     - 'app/models/open_food_network/calculator/weight.rb'
     - 'app/models/order_cycle.rb'
     - 'app/models/product_distribution.rb'
-    - 'app/models/spree/address_decorator.rb'
     - 'app/models/spree/adjustment_decorator.rb'
     - 'app/models/spree/classification_decorator.rb'
     - 'app/models/spree/gateway/migs.rb'
@@ -2313,7 +2310,6 @@ Style/RedundantSelf:
     - 'app/models/open_food_network/calculator/weight.rb'
     - 'app/models/order_cycle.rb'
     - 'app/models/producer_property.rb'
-    - 'app/models/spree/address_decorator.rb'
     - 'app/models/spree/calculator/flat_percent_item_total_decorator.rb'
     - 'app/models/spree/calculator/flexi_rate_decorator.rb'
     - 'app/models/spree/calculator/per_item_decorator.rb'

app/models/spree/address_decorator.rb

@@ -1,35 +1,27 @@
 Spree::Address.class_eval do
-  has_one :enterprise
+  has_one :enterprise, dependent: :restrict
   belongs_to :country, class_name: "Spree::Country"
 
   after_save :touch_enterprise
 
   geocoded_by :geocode_address
 
-  delegate :name, :to => :state, :prefix => true, :allow_nil => true
+  delegate :name, to: :state, prefix: true, allow_nil: true
 
   def geocode_address
-    geocode_address = [address1, address2, zipcode, city, country.andand.name, state.andand.name]
-    filtered_address = geocode_address.select{ |field| !field.nil? && field != '' }
-    filtered_address.compact.join(', ')
+    render_address([address1, address2, zipcode, city, country.andand.name, state.andand.name])
   end
 
   def full_address
-    full_address = [address1, address2, city, zipcode, state.andand.name]
-    filtered_address = full_address.select{ |field| !field.nil? && field != '' }
-    filtered_address.compact.join(', ')
+    render_address([address1, address2, city, zipcode, state.andand.name])
   end
 
   def address_part1
-    address_part1 = [address1, address2]
-    filtered_address = address_part1.select{ |field| !field.nil? && field != '' }
-    filtered_address.compact.join(', ')
+    render_address([address1, address2])
   end
 
   def address_part2
-    address_part2= [city, zipcode, state.andand.name]
-    filtered_address = address_part2.select{ |field| !field.nil? && field != '' }
-    filtered_address.compact.join(', ')
+    render_address([city, zipcode, state.andand.name])
   end
 
   private
@@ -38,26 +30,7 @@ Spree::Address.class_eval do
     enterprise.andand.touch
   end
 
-  # We have a hard-to-track-down bug around invalid addresses with all-nil fields finding
-  # their way into the database. I don't know what the source of them is, so this patch
-  # is designed to track them down.
-  # This is intended to be a temporary investigative measure, and should be removed from the
-  # code base shortly. If it's past 17-10-2013, take it out.
-  #
-  #-- Rohan, 17-9-2913
-  def create
-    if self.zipcode.nil?
-      Bugsnag.notify RuntimeError.new('Creating a Spree::Address with nil values')
-    end
-
-    super
-  end
-
-  def update(attribute_names = @attributes.keys)
-    if self.zipcode.nil?
-      Bugsnag.notify RuntimeError.new('Updating a Spree::Address with nil values')
-    end
-
-    super(attribute_names)
+  def render_address(parts)
+    parts.select(&:present?).join(', ')
   end
 end

config/initializers/geocoder.rb

@@ -0,0 +1,6 @@
+# Google requires an API key with a billing account to use their API.
+# The key is stored in config/application.yml.
+Geocoder.configure(
+  use_https: true,
+  api_key: ENV.fetch('GOOGLE_MAPS_API_KEY', nil)
+)

spec/models/spree/addresses_spec.rb

@@ -1,31 +1,44 @@
 require 'spec_helper'
 
 describe Spree::Address do
+  let(:address) { build(:address) }
+  let(:enterprise_address) { build(:address, enterprise: build(:enterprise)) }
+
   describe "associations" do
-    it { should have_one(:enterprise) }
+    it { is_expected.to have_one(:enterprise) }
   end
 
   describe "delegation" do
-    it { should delegate(:name).to(:state).with_prefix }
+    it { is_expected.to delegate(:name).to(:state).with_prefix }
+  end
+
+  describe "destroy" do
+    it "can be deleted" do
+      expect { address.destroy }.to_not raise_error
+    end
+
+    it "cannot be deleted with associated enterprise" do
+      expect do
+        enterprise_address.destroy
+      end.to raise_error ActiveRecord::DeleteRestrictionError
+    end
   end
 
   describe "geocode address" do
-    let(:address) { FactoryBot.build(:address) }
-
     it "should include address1, address2, zipcode, city, state and country" do
-      address.geocode_address.should include(address.address1)
-      address.geocode_address.should include(address.address2)
-      address.geocode_address.should include(address.zipcode)
-      address.geocode_address.should include(address.city)
-      address.geocode_address.should include(address.state.name)
-      address.geocode_address.should include(address.country.name)
+      expect(address.geocode_address).to include(address.address1)
+      expect(address.geocode_address).to include(address.address2)
+      expect(address.geocode_address).to include(address.zipcode)
+      expect(address.geocode_address).to include(address.city)
+      expect(address.geocode_address).to include(address.state.name)
+      expect(address.geocode_address).to include(address.country.name)
     end
 
     it "should not include empty fields" do
       address.address2 = nil
       address.city = ""
 
-      address.geocode_address.split(',').length.should eql(4)
+      expect(address.geocode_address.split(',').length).to eql(4)
     end
   end
 
@@ -33,40 +46,27 @@ describe Spree::Address do
     let(:address) { FactoryBot.build(:address) }
 
     it "should include address1, address2, zipcode, city and state" do
-      address.full_address.should include(address.address1)
-      address.full_address.should include(address.address2)
-      address.full_address.should include(address.zipcode)
-      address.full_address.should include(address.city)
-      address.full_address.should include(address.state.name)
-      address.full_address.should_not include(address.country.name)
+      expect(address.full_address).to include(address.address1)
+      expect(address.full_address).to include(address.address2)
+      expect(address.full_address).to include(address.zipcode)
+      expect(address.full_address).to include(address.city)
+      expect(address.full_address).to include(address.state.name)
+      expect(address.full_address).not_to include(address.country.name)
     end
 
     it "should not include empty fields" do
       address.address2 = nil
       address.city = ""
 
-      address.full_address.split(',').length.should eql(3)
+      expect(address.full_address.split(',').length).to eql(3)
     end
   end
 
   describe "setters" do
     it "lets us set a country" do
-      expect { Spree::Address.new.country = "A country" }.to raise_error ActiveRecord::AssociationTypeMismatch
-    end
-  end
-
-  describe "notifying bugsnag when saved with missing data" do
-    it "notifies on create" do
-      Bugsnag.should_receive(:notify)
-      a = Spree::Address.new zipcode: nil
-      a.save validate: false
-    end
-
-    it "notifies on update" do
-      Bugsnag.should_receive(:notify)
-      a = create(:address)
-      a.zipcode = nil
-      a.save validate: false
+      expect do
+        Spree::Address.new.country = "A country"
+      end.to raise_error ActiveRecord::AssociationTypeMismatch
     end
   end
 end