From b109f6d78c131594ea9ba65245f6622bd093671c Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 16 Oct 2018 11:57:21 +1100 Subject: [PATCH 01/10] Remove temporary reporting code --- app/models/spree/address_decorator.rb | 23 ----------------------- spec/models/spree/addresses_spec.rb | 15 --------------- 2 files changed, 38 deletions(-) diff --git a/app/models/spree/address_decorator.rb b/app/models/spree/address_decorator.rb index db092f5eb2a..8309083c18f 100644 --- a/app/models/spree/address_decorator.rb +++ b/app/models/spree/address_decorator.rb @@ -37,27 +37,4 @@ def address_part2 def touch_enterprise 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) - end end diff --git a/spec/models/spree/addresses_spec.rb b/spec/models/spree/addresses_spec.rb index f4532186101..2e2d760104d 100644 --- a/spec/models/spree/addresses_spec.rb +++ b/spec/models/spree/addresses_spec.rb @@ -54,19 +54,4 @@ 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 - end - end end From c4437a643753f4c2dd3f84423450b150ef9b9589 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 16 Oct 2018 12:03:11 +1100 Subject: [PATCH 02/10] Style address decorator --- .rubocop_todo.yml | 3 --- app/models/spree/address_decorator.rb | 4 ++-- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 378129593e8..cfc4e57e907 100644 --- a/.rubocop_todo.yml +++ b/.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' @@ -1955,7 +1954,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 +2311,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' diff --git a/app/models/spree/address_decorator.rb b/app/models/spree/address_decorator.rb index 8309083c18f..8e8fc287ce2 100644 --- a/app/models/spree/address_decorator.rb +++ b/app/models/spree/address_decorator.rb @@ -6,7 +6,7 @@ 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] @@ -27,7 +27,7 @@ def address_part1 end def address_part2 - address_part2= [city, zipcode, state.andand.name] + address_part2 = [city, zipcode, state.andand.name] filtered_address = address_part2.select{ |field| !field.nil? && field != '' } filtered_address.compact.join(', ') end From 34849c441a5c6880387560e5e6bf8833321ff7bf Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 16 Oct 2018 12:09:18 +1100 Subject: [PATCH 03/10] Reduce complexity and duplication --- app/models/spree/address_decorator.rb | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/app/models/spree/address_decorator.rb b/app/models/spree/address_decorator.rb index 8e8fc287ce2..ea20b1cf718 100644 --- a/app/models/spree/address_decorator.rb +++ b/app/models/spree/address_decorator.rb @@ -10,26 +10,22 @@ 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(geocode_address) 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(full_address) end def address_part1 address_part1 = [address1, address2] - filtered_address = address_part1.select{ |field| !field.nil? && field != '' } - filtered_address.compact.join(', ') + render_address(address_part1) 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(address_part2) end private @@ -37,4 +33,9 @@ def address_part2 def touch_enterprise enterprise.andand.touch end + + def render_address(parts) + filtered_address = parts.select{ |field| !field.nil? && field != '' } + filtered_address.compact.join(', ') + end end From 5021ed9c6942c89321ce67286909fed5c155a687 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 16 Oct 2018 12:11:31 +1100 Subject: [PATCH 04/10] Simplify by using Rails tools --- app/models/spree/address_decorator.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/models/spree/address_decorator.rb b/app/models/spree/address_decorator.rb index ea20b1cf718..592f9703ccc 100644 --- a/app/models/spree/address_decorator.rb +++ b/app/models/spree/address_decorator.rb @@ -35,7 +35,6 @@ def touch_enterprise end def render_address(parts) - filtered_address = parts.select{ |field| !field.nil? && field != '' } - filtered_address.compact.join(', ') + parts.select(&:present?).join(', ') end end From a8705ca179494ddd75edb529137de579fce219ad Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 16 Oct 2018 12:13:08 +1100 Subject: [PATCH 05/10] Simplify address methods --- app/models/spree/address_decorator.rb | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/app/models/spree/address_decorator.rb b/app/models/spree/address_decorator.rb index 592f9703ccc..04c8014d460 100644 --- a/app/models/spree/address_decorator.rb +++ b/app/models/spree/address_decorator.rb @@ -9,23 +9,19 @@ delegate :name, to: :state, prefix: true, allow_nil: true def geocode_address - geocode_address = [address1, address2, zipcode, city, country.andand.name, state.andand.name] - render_address(geocode_address) + 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] - render_address(full_address) + render_address([address1, address2, city, zipcode, state.andand.name]) end def address_part1 - address_part1 = [address1, address2] - render_address(address_part1) + render_address([address1, address2]) end def address_part2 - address_part2 = [city, zipcode, state.andand.name] - render_address(address_part2) + render_address([city, zipcode, state.andand.name]) end private From 61797fff56dac6c5a0e7357c9331d2af3b0ff82c Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 16 Oct 2018 15:18:44 +1100 Subject: [PATCH 06/10] Restrict deletion of address explicitely Enterprises have an `address_id` which must point to a valid `Spree::Address`. As Rubocop suggested, I restricted the deletion of addresses when they are still associated to an enterprise. Without declaring `dependent: :restrict`, trying to delete the address would raise `ActiveRecord::InvalidForeignKey`. Now it is more specific and raises `ActiveRecord::DeleteRestrictionError`. I didn't find code rescuing the InvalidForeignKey when deleting addresses. I actually think that we never delete addresses. So this change should not have any impact on the execution. --- .rubocop_todo.yml | 1 - app/models/spree/address_decorator.rb | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index cfc4e57e907..8d06d3cfadc 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1440,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' diff --git a/app/models/spree/address_decorator.rb b/app/models/spree/address_decorator.rb index 04c8014d460..ff7c1f06659 100644 --- a/app/models/spree/address_decorator.rb +++ b/app/models/spree/address_decorator.rb @@ -1,5 +1,5 @@ Spree::Address.class_eval do - has_one :enterprise + has_one :enterprise, dependent: :restrict belongs_to :country, class_name: "Spree::Country" after_save :touch_enterprise From 8fb81bb6a7da16f8ca0124b448e5315aa5c8fba2 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 16 Oct 2018 16:49:52 +1100 Subject: [PATCH 07/10] Configure Geocoder with API key as required by Google --- config/initializers/geocoder.rb | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 config/initializers/geocoder.rb diff --git a/config/initializers/geocoder.rb b/config/initializers/geocoder.rb new file mode 100644 index 00000000000..35b5510d5e3 --- /dev/null +++ b/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) +) From e96cab957a745d7d04f30f269e75664853a78d97 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 18 Oct 2018 10:52:46 +1100 Subject: [PATCH 08/10] Convert specs to RSpec 3.7.1 syntax with Transpec This conversion is done by Transpec 3.3.0 with the following command: transpec spec/models/spree/addresses_spec.rb * 13 conversions from: obj.should to: expect(obj).to * 2 conversions from: it { should ... } to: it { is_expected.to ... } * 1 conversion from: obj.should_not to: expect(obj).not_to For more details: https://github.com/yujinakayama/transpec#supported-conversions --- spec/models/spree/addresses_spec.rb | 32 ++++++++++++++--------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/spec/models/spree/addresses_spec.rb b/spec/models/spree/addresses_spec.rb index 2e2d760104d..8d464461812 100644 --- a/spec/models/spree/addresses_spec.rb +++ b/spec/models/spree/addresses_spec.rb @@ -2,30 +2,30 @@ describe Spree::Address do 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 "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,19 +33,19 @@ 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 From d197c8587f10e3d7de69771a2debba12c08b6d26 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 18 Oct 2018 11:06:32 +1100 Subject: [PATCH 09/10] Test address deletion --- spec/models/spree/addresses_spec.rb | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/spec/models/spree/addresses_spec.rb b/spec/models/spree/addresses_spec.rb index 8d464461812..321d6b71efe 100644 --- a/spec/models/spree/addresses_spec.rb +++ b/spec/models/spree/addresses_spec.rb @@ -1,6 +1,9 @@ require 'spec_helper' describe Spree::Address do + let(:address) { build(:address) } + let(:enterprise_address) { build(:address, enterprise: build(:enterprise)) } + describe "associations" do it { is_expected.to have_one(:enterprise) } end @@ -9,9 +12,19 @@ it { is_expected.to delegate(:name).to(:state).with_prefix } end - describe "geocode address" do - let(:address) { FactoryBot.build(:address) } + 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 it "should include address1, address2, zipcode, city, state and country" do expect(address.geocode_address).to include(address.address1) expect(address.geocode_address).to include(address.address2) From 9698fd3c5ae76513c377a203ad301769179b7d2d Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 18 Oct 2018 11:07:16 +1100 Subject: [PATCH 10/10] Style spec --- spec/models/spree/addresses_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/models/spree/addresses_spec.rb b/spec/models/spree/addresses_spec.rb index 321d6b71efe..5d78cadcf2b 100644 --- a/spec/models/spree/addresses_spec.rb +++ b/spec/models/spree/addresses_spec.rb @@ -64,7 +64,9 @@ describe "setters" do it "lets us set a country" do - expect { Spree::Address.new.country = "A country" }.to raise_error ActiveRecord::AssociationTypeMismatch + expect do + Spree::Address.new.country = "A country" + end.to raise_error ActiveRecord::AssociationTypeMismatch end end end