Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2765 Fix geocoding to display enterprises on map #2872

Merged
merged 10 commits into from
Oct 22, 2018
Merged

2765 Fix geocoding to display enterprises on map #2872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b109f6d
Remove temporary reporting code
mkllnk Oct 16, 2018
c4437a6
Style address decorator
mkllnk Oct 16, 2018
34849c4
Reduce complexity and duplication
mkllnk Oct 16, 2018
5021ed9
Simplify by using Rails tools
mkllnk Oct 16, 2018
a8705ca
Simplify address methods
mkllnk Oct 16, 2018
61797ff
Restrict deletion of address explicitely
mkllnk Oct 16, 2018
8fb81bb
Configure Geocoder with API key as required by Google
mkllnk Oct 16, 2018
e96cab9
Convert specs to RSpec 3.7.1 syntax with Transpec
mkllnk Oct 17, 2018
d197c85
Test address deletion
mkllnk Oct 18, 2018
9698fd3
Style spec
mkllnk Oct 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions .rubocop_todo.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -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'
Expand Down
43 changes: 8 additions & 35 deletions app/models/spree/address_decorator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,27 @@
Spree::Address.class_eval do
has_one :enterprise
has_one :enterprise, dependent: :restrict
belongs_to :country, class_name: "Spree::Country"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a test. Could be a simple one trying to delete an address used in an enterprise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think I agree that a unit test could be useful to validate that this Rails magic works as intended. But I hesitated because we are not deleting address anywhere in the code. So if it's not a requirement, if it's not a specification to delete an address, why should it be added to the specs? I'll still do it for code coverage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, thanks!
imo, if it's worth a change in the code, it's worth a test. the only case I would skip the test is if it is too difficult to test for the value it adds, which is not the case. this is only my opinion :-)


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])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these methods should be in a separate service, some sort of geocoder_service that formats addresses according to different given inputs. And this service methods should be unit tested at least with simple positive examples.

another option, is to add functional tests to avoid this bug from re-appearing without us noticing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that these methods would be better in a separate service. But I didn't want to go too far out of scope. All commits except the last one with the initializer are pure refactoring and don't contribute anything to fixing the original issue. It was just me understanding the code and cleaning it up along the way. I thought about not including them in the PR, but then I thought it might be useful to others to know where the geocoding is actually happening.

Before we pull the rendering out into a service, we should discuss using our own model and table for enterprise addresses. We have some ugly hacks to use Spree::Address for enterprise locations. For example, Spree::Address requires first and last name, but enterprises don't have that. So in the enterprise model we set these fields to "unused" to make the addresses valid.

We also added the has_one :enterprise relationship which adds unneeded bulk to all address objects that are used for orders. All addresses for orders get automatically geolocated even though we never use that. Point is: enterprise addresses have a very different purpose and hold different data to Spree's addresses.

By the way, the rendering method is unit tested:
https://github.com/openfoodfoundation/openfoodnetwork/blob/8fb81bb6a7da16f8ca0124b448e5315aa5c8fba2/spec/models/spree/addresses_spec.rb
But that wasn't the problem. The problem was that Google decided to give different responses to API requests. The only way to catch that is a live test. Usually all specs are set up to not use an internet connection. External services are mocked or HTTP responses are recorded and replayed. Hitting a live API is very bad practice in tests. The correct HTTP response depends on the API key, the setup in the developer console, the availability of a valid credit card and Google's terms of service. I'm not sure we can test that in a reasonable way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahaha, I meant unit tests on the rendering methods only! the tests were there already. sorry about that.

the full refactor of address is a more complicated issue, agree. thanks for the details!

I think we could still just extract these methods to a geocoding service separate from the address class but fully appreciate what you said about refactoring. it's important we feel encouraged to refactor but don't feel pressed to make it perfect. a little improvement is always very welcome!

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
Expand All @@ -38,26 +30,7 @@ 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's exactly 5 years later!!!

#
#-- 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
6 changes: 6 additions & 0 deletions config/initializers/geocoder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
)
68 changes: 34 additions & 34 deletions spec/models/spree/addresses_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,72 +1,72 @@
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 "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
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

describe "full 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