diff --git a/app/assets/javascripts/darkswarm/all.js.coffee b/app/assets/javascripts/darkswarm/all.js.coffee index f6baa6f5bf..66ac04511d 100644 --- a/app/assets/javascripts/darkswarm/all.js.coffee +++ b/app/assets/javascripts/darkswarm/all.js.coffee @@ -59,4 +59,4 @@ $ -> # Hacky fix for issue - http://foundation.zurb.com/forum/posts/2112-foundation-5100-syntax-error-in-js Foundation.set_namespace "" - $(document).foundation() \ No newline at end of file + $(document).foundation() diff --git a/app/assets/javascripts/darkswarm/controllers/registration/registration_controller.js.coffee b/app/assets/javascripts/darkswarm/controllers/registration/registration_controller.js.coffee index 276f9da3fe..a373740779 100644 --- a/app/assets/javascripts/darkswarm/controllers/registration/registration_controller.js.coffee +++ b/app/assets/javascripts/darkswarm/controllers/registration/registration_controller.js.coffee @@ -6,7 +6,6 @@ Darkswarm.controller "RegistrationCtrl", ($scope, RegistrationService, Enterpris $scope.latLong = null $scope.addressConfirmed $scope.steps = ['details', 'contact', 'type', 'about', 'images', 'social'] - $scope.enableMapConfirm = GmapsGeo and GmapsGeo.OK # Filter countries without states since the form requires a state to be selected. # Consider changing the form to require a state only if a country requires them (Spree option). diff --git a/app/assets/javascripts/darkswarm/services/enterprise_registration_service.js.coffee b/app/assets/javascripts/darkswarm/services/enterprise_registration_service.js.coffee index dc9f6617ee..4d1bda61c8 100644 --- a/app/assets/javascripts/darkswarm/services/enterprise_registration_service.js.coffee +++ b/app/assets/javascripts/darkswarm/services/enterprise_registration_service.js.coffee @@ -21,6 +21,7 @@ Darkswarm.factory "EnterpriseRegistrationService", ($http, RegistrationService, url: "/api/enterprises" data: enterprise: @prepare() + use_geocoder: @useGeocoder() params: token: spreeApiKey ).success((data) => @@ -45,6 +46,7 @@ Darkswarm.factory "EnterpriseRegistrationService", ($http, RegistrationService, url: "/api/enterprises/#{@enterprise.id}" data: enterprise: @prepare() + use_geocoder: @useGeocoder() params: token: spreeApiKey ).success((data) -> @@ -63,3 +65,7 @@ Darkswarm.factory "EnterpriseRegistrationService", ($http, RegistrationService, enterprise.address_attributes = @enterprise.address if @enterprise.address? enterprise.address_attributes.country_id = @enterprise.country.id if @enterprise.country? enterprise + + useGeocoder: => + if @enterprise.address? && !@enterprise.address.latitude? && !@enterprise.address.longitude? + return "1" diff --git a/app/controllers/admin/enterprises_controller.rb b/app/controllers/admin/enterprises_controller.rb index 9b42b39174..86b521cb1c 100644 --- a/app/controllers/admin/enterprises_controller.rb +++ b/app/controllers/admin/enterprises_controller.rb @@ -4,6 +4,8 @@ require 'open_food_network/order_cycle_permissions' module Admin class EnterprisesController < Admin::ResourceController + include GeocodeEnterpriseAddress + # These need to run before #load_resource so that @object is initialised with sanitised values prepend_before_action :override_owner, only: :create prepend_before_action :override_sells, only: :create @@ -22,6 +24,9 @@ module Admin before_action :load_properties, only: [:edit, :update] before_action :setup_property, only: [:edit] + update.after :geocode_address_if_use_geocoder + create.after :geocode_address_if_use_geocoder + helper 'spree/products' include OrderCyclesHelper diff --git a/app/controllers/api/enterprises_controller.rb b/app/controllers/api/enterprises_controller.rb index bcec80f3a6..76ad7782c2 100644 --- a/app/controllers/api/enterprises_controller.rb +++ b/app/controllers/api/enterprises_controller.rb @@ -1,5 +1,7 @@ module Api class EnterprisesController < Api::BaseController + include GeocodeEnterpriseAddress + before_action :override_owner, only: [:create, :update] before_action :check_type, only: :update before_action :override_sells, only: [:create, :update] @@ -14,6 +16,7 @@ module Api user_ids = enterprise_params.delete(:user_ids) @enterprise = Enterprise.new(enterprise_params) if @enterprise.save + geocode_address_if_use_geocoder @enterprise.user_ids = user_ids render json: @enterprise.id, status: :created else @@ -26,6 +29,7 @@ module Api authorize! :update, @enterprise if @enterprise.update(enterprise_params) + geocode_address_if_use_geocoder render json: @enterprise.id, status: :ok else invalid_resource!(@enterprise) diff --git a/app/controllers/concerns/geocode_enterprise_address.rb b/app/controllers/concerns/geocode_enterprise_address.rb new file mode 100644 index 0000000000..9f72e8b292 --- /dev/null +++ b/app/controllers/concerns/geocode_enterprise_address.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module GeocodeEnterpriseAddress + extend ActiveSupport::Concern + + def geocode_address_if_use_geocoder + AddressGeocoder.new(@enterprise.address).geocode if params[:use_geocoder] == "1" + end +end diff --git a/app/helpers/map_helper.rb b/app/helpers/map_helper.rb index 65a9561697..b2ef609c77 100644 --- a/app/helpers/map_helper.rb +++ b/app/helpers/map_helper.rb @@ -1,2 +1,13 @@ module MapHelper + def using_google_maps? + ENV["GOOGLE_MAPS_API_KEY"].present? || google_maps_configured_with_geocoder_api_key? + end + + private + + def google_maps_configured_with_geocoder_api_key? + ENV["GEOCODER_API_KEY"].present? && ( + ENV["GEOCODER_SERVICE"].to_s == "google" || ENV["GEOCODER_SERVICE"].blank? + ) + end end diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index c902490edd..e73f715598 100644 --- a/app/models/enterprise.rb +++ b/app/models/enterprise.rb @@ -410,10 +410,6 @@ class Enterprise < ActiveRecord::Base address.firstname = address.lastname = address.phone = 'unused' if address.present? end - def geocode_address - address.geocode if address.andand.changed? - end - def ensure_owner_is_manager users << owner unless users.include?(owner) end diff --git a/app/models/spree/address.rb b/app/models/spree/address.rb index b8cc969cc4..28bc7511d7 100644 --- a/app/models/spree/address.rb +++ b/app/models/spree/address.rb @@ -10,24 +10,18 @@ module Spree has_one :enterprise, dependent: :restrict_with_exception has_many :shipments - before_validation :geocode, if: :use_geocoder? - validates :firstname, :lastname, :address1, :city, :country, presence: true validates :zipcode, presence: true, if: :require_zipcode? validates :phone, presence: true, if: :require_phone? validate :state_validate - attr_accessor :use_geocoder - after_save :touch_enterprise alias_attribute :first_name, :firstname alias_attribute :last_name, :lastname delegate :name, to: :state, prefix: true, allow_nil: true - geocoded_by :geocode_address - def self.default country = begin Spree::Country.find(Spree::Config[:default_country_id]) @@ -93,10 +87,6 @@ module Spree } end - def geocode_address - render_address([address1, address2, zipcode, city, country.andand.name, state.andand.name]) - end - def full_address render_address([address1, address2, city, zipcode, state.andand.name]) end @@ -111,10 +101,6 @@ module Spree private - def use_geocoder? - @use_geocoder == true || @use_geocoder == 'true' || @use_geocoder == '1' - end - def require_phone? true end diff --git a/app/services/address_geocoder.rb b/app/services/address_geocoder.rb new file mode 100644 index 0000000000..b090afb569 --- /dev/null +++ b/app/services/address_geocoder.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +class AddressGeocoder + def initialize(address) + @address = address + end + + def geocode + latitude, longitude = Geocoder.coordinates(geocode_address) + + return unless latitude.present? && longitude.present? + + address.update(latitude: latitude, longitude: longitude) + end + + private + + attr_reader :address + + def geocode_address + [ + address.address1, + address.address2, + address.zipcode, + address.city, + address.country.andand.name, + address.state.andand.name + ].select(&:present?).join(', ') + end +end diff --git a/app/services/permitted_attributes/address.rb b/app/services/permitted_attributes/address.rb index a527efb22d..df2a505608 100644 --- a/app/services/permitted_attributes/address.rb +++ b/app/services/permitted_attributes/address.rb @@ -7,7 +7,7 @@ module PermittedAttributes :firstname, :lastname, :address1, :address2, :city, :country_id, :state_id, :zipcode, :phone, :state_name, :alternative_phone, :company, - :latitude, :longitude, :use_geocoder + :latitude, :longitude ] end end diff --git a/app/views/admin/enterprises/_new_form.html.haml b/app/views/admin/enterprises/_new_form.html.haml index de522c5a7a..99737eb635 100644 --- a/app/views/admin/enterprises/_new_form.html.haml +++ b/app/views/admin/enterprises/_new_form.html.haml @@ -113,12 +113,11 @@ .three.columns.alpha = " ".html_safe .five.columns.omega - = af.check_box :use_geocoder - = af.label :use_geocoder, t('use_geocoder') + = check_box_tag "use_geocoder" + = label_tag "use_geocoder", t('use_geocoder') .row .twelve.columns.alpha   .row .twelve.columns.alpha - = render partial: "spree/admin/shared/new_resource_links" - \ No newline at end of file + = render partial: "spree/admin/shared/new_resource_links" \ No newline at end of file diff --git a/app/views/admin/enterprises/form/_address.html.haml b/app/views/admin/enterprises/form/_address.html.haml index ee728cb545..810953d4ea 100644 --- a/app/views/admin/enterprises/form/_address.html.haml +++ b/app/views/admin/enterprises/form/_address.html.haml @@ -51,5 +51,5 @@ .three.columns.alpha = " ".html_safe .five.columns.omega - = af.check_box :use_geocoder - = af.label :use_geocoder, t('use_geocoder') \ No newline at end of file + = check_box_tag "use_geocoder" + = label_tag "use_geocoder", t('use_geocoder') diff --git a/app/views/layouts/registration.html.haml b/app/views/layouts/registration.html.haml index a60c58f2e1..ff42853481 100644 --- a/app/views/layouts/registration.html.haml +++ b/app/views/layouts/registration.html.haml @@ -35,5 +35,4 @@ = yield :injection_data = render "layouts/i18n_script" - = render "layouts/bugherd_script" - \ No newline at end of file + = render "layouts/bugherd_script" \ No newline at end of file diff --git a/app/views/registration/steps/_details.html.haml b/app/views/registration/steps/_details.html.haml index d105fae4ca..40c993bccc 100644 --- a/app/views/registration/steps/_details.html.haml +++ b/app/views/registration/steps/_details.html.haml @@ -56,25 +56,26 @@ %select.chunky{ id: 'enterprise_country', name: 'country', required: true, ng: { init: "setDefaultCountry(#{Spree::Config[:default_country_id]})", model: 'enterprise.country', options: 'c as c.name for c in countries' } } %span.error{ ng: { show: "details.country.$error.required && submitted" } } = t(".country_field_error") - %div{ ng: {if: "enableMapConfirm"}} - .center{ ng: {if: "latLong"}} - %strong {{'registration.steps.details.drag_pin' | t}} - .small-12.medium-9.large-12.columns.end - .map-container--registration{id: "registration-map", ng: {if: "!!map"}} - %ui-gmap-google-map{ center: "map.center", zoom: "map.zoom"} - %ui-gmap-marker{idKey: 1, coords: "latLong", options: '{ draggable: true}'} - - .center - %input.button.primary{ type: "button", value: "{{'registration.steps.details.locate_address' | t}}", ng: { click: "locateAddress()" } } - .center{ ng: {if: "latLong"}} - .field - %input{ type: 'checkbox', id: 'confirm_address', name: 'confirm_address', ng: { model: 'addressConfirmed', change: 'confirmAddressChange(confirmAddress)'} } - %label{ for: 'confirm_address' } {{'registration.steps.details.confirm_address' | t}} - .small-12.medium-9.large-12.columns.end{ ng: {if: "latLong"}} - .field - %strong {{'registration.steps.details.drag_map_marker' | t}} + - if using_google_maps? + %div + .center{ ng: {if: "latLong"}} + %strong {{'registration.steps.details.drag_pin' | t}} + .small-12.medium-9.large-12.columns.end + .map-container--registration{id: "registration-map", ng: {if: "!!map"}} + %ui-gmap-google-map{ center: "map.center", zoom: "map.zoom"} + %ui-gmap-marker{idKey: 1, coords: "latLong", options: '{ draggable: true}'} + + .center + %input.button.primary{ type: "button", value: "{{'registration.steps.details.locate_address' | t}}", ng: { click: "locateAddress()" } } + .center{ ng: {if: "latLong"}} + .field + %input{ type: 'checkbox', id: 'confirm_address', name: 'confirm_address', ng: { model: 'addressConfirmed', change: 'confirmAddressChange(confirmAddress)'} } + %label{ for: 'confirm_address' } {{'registration.steps.details.confirm_address' | t}} + .small-12.medium-9.large-12.columns.end{ ng: {if: "latLong"}} + .field + %strong {{'registration.steps.details.drag_map_marker' | t}} + .row.buttons .small-12.columns %hr %input.button.primary.right{ type: "submit", value: "{{'continue' | t}}" } - \ No newline at end of file diff --git a/spec/controllers/admin/enterprises_controller_spec.rb b/spec/controllers/admin/enterprises_controller_spec.rb index 0a4e00b472..fc6ddeb4ee 100644 --- a/spec/controllers/admin/enterprises_controller_spec.rb +++ b/spec/controllers/admin/enterprises_controller_spec.rb @@ -13,13 +13,14 @@ describe Admin::EnterprisesController, type: :controller do let(:distributor) { create(:distributor_enterprise, owner: distributor_owner ) } let(:supplier) { create(:supplier_enterprise, owner: supplier_owner) } + let(:country) { Spree::Country.find_by name: 'Australia' } + let(:state) { Spree::State.find_by name: 'Victoria' } + let(:address_params) { { address1: 'a', city: 'a', zipcode: 'a', country_id: country.id, state_id: state.id } } before { @request.env['HTTP_REFERER'] = 'http://test.com/' } describe "creating an enterprise" do - let(:country) { Spree::Country.find_by name: 'Australia' } - let(:state) { Spree::State.find_by name: 'Victoria' } - let(:enterprise_params) { { enterprise: { name: 'zzz', permalink: 'zzz', is_primary_producer: '0', address_attributes: { address1: 'a', city: 'a', zipcode: 'a', country_id: country.id, state_id: state.id } } } } + let(:enterprise_params) { { enterprise: { name: 'zzz', permalink: 'zzz', is_primary_producer: '0', address_attributes: address_params } } } it "grants management permission if the current user is an enterprise user" do allow(controller).to receive_messages spree_current_user: distributor_manager @@ -99,6 +100,27 @@ describe Admin::EnterprisesController, type: :controller do expect(enterprise.sells).to eq('any') end end + + context "geocoding" do + before do + allow(controller).to receive_messages spree_current_user: admin_user + enterprise_params[:enterprise][:owner_id] = admin_user + end + + it "geocodes the address when the :use_geocoder parameter is set" do + expect_any_instance_of(AddressGeocoder).to receive(:geocode) + enterprise_params[:use_geocoder] = "1" + + spree_put :create, enterprise_params + end + + it "doesn't geocode the address when the :use_geocoder parameter is not set" do + expect_any_instance_of(AddressGeocoder).not_to receive(:geocode) + enterprise_params[:use_geocoder] = "0" + + spree_put :create, enterprise_params + end + end end describe "updating an enterprise" do @@ -291,6 +313,26 @@ describe Admin::EnterprisesController, type: :controller do expect(distributor.users).to include user end end + + context "geocoding" do + before do + allow(controller).to receive_messages spree_current_user: profile_enterprise.owner + end + + it "geocodes the address when the :use_geocoder parameter is set" do + expect_any_instance_of(AddressGeocoder).to receive(:geocode) + enterprise_params = { id: profile_enterprise, enterprise: {}, use_geocoder: "1" } + + spree_put :update, enterprise_params + end + + it "doesn't geocode the address when the :use_geocoder parameter is not set" do + expect_any_instance_of(AddressGeocoder).not_to receive(:geocode) + enterprise_params = { id: profile_enterprise, enterprise: {}, use_geocoder: "0" } + + spree_put :update, enterprise_params + end + end end describe "register" do diff --git a/spec/controllers/api/enterprises_controller_spec.rb b/spec/controllers/api/enterprises_controller_spec.rb index cfc29f9358..9ba546e50e 100644 --- a/spec/controllers/api/enterprises_controller_spec.rb +++ b/spec/controllers/api/enterprises_controller_spec.rb @@ -49,6 +49,20 @@ describe Api::EnterprisesController, type: :controller do enterprise = Enterprise.last expect(enterprise.user_ids).to match_array([enterprise_owner.id, manager1.id, manager2.id]) end + + context "geocoding" do + it "geocodes the address when the :use_geocoder parameter is set" do + expect_any_instance_of(AddressGeocoder).to receive(:geocode) + + api_post :create, { enterprise: new_enterprise_params, use_geocoder: "1" } + end + + it "doesn't geocode the address when the :use_geocoder parameter is not set" do + expect_any_instance_of(AddressGeocoder).not_to receive(:geocode) + + api_post :create, { enterprise: new_enterprise_params, use_geocoder: "0" } + end + end end end diff --git a/spec/features/consumer/registration_spec.rb b/spec/features/consumer/registration_spec.rb index 25d442bf76..9dc76828b5 100644 --- a/spec/features/consumer/registration_spec.rb +++ b/spec/features/consumer/registration_spec.rb @@ -15,6 +15,7 @@ feature "Registration", js: true do albania = Spree::Country.create!({ name: "Albania", iso3: "ALB", iso: "AL", iso_name: "ALBANIA", numcode: "8" }) Spree::State.create!({ name: "Berat", abbr: "BRA", country: albania }) Spree::Country.create!({ name: "Chad", iso3: "TCD", iso: "TD", iso_name: "CHAD", numcode: "148" }) + AddressGeocoder.any_instance.stub(:geocode) end after do diff --git a/spec/models/spree/addresses_spec.rb b/spec/models/spree/addresses_spec.rb index 52f51cc91f..fcb31c1210 100644 --- a/spec/models/spree/addresses_spec.rb +++ b/spec/models/spree/addresses_spec.rb @@ -42,24 +42,6 @@ describe Spree::Address do 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) - 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 = "" - - expect(address.geocode_address.split(',').length).to eql(4) - end - end - describe "full address" do let(:address) { FactoryBot.build(:address) } diff --git a/spec/services/address_geocoder_spec.rb b/spec/services/address_geocoder_spec.rb new file mode 100644 index 0000000000..2d9754846e --- /dev/null +++ b/spec/services/address_geocoder_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe AddressGeocoder do + let(:australia) { Spree::Country.find_or_create_by!(name: "Australia") } + let(:victoria) { Spree::State.find_or_create_by(name: "Victoria", country: australia) } + let(:address) do + create(:address, + address1: "12 Galvin Street", + address2: "Unit 1", + city: "Altona", + country: australia, + state: victoria, + zipcode: 3018, + latitude: nil, + longitude: nil) + end + + it "formats the address into a single comma separated string when passing it to the geocoder" do + expect(Geocoder).to receive(:coordinates).with("12 Galvin Street, Unit 1, 3018, Altona, Australia, Victoria") + + AddressGeocoder.new(address).geocode + end + + describe "when the geocoder can determine the latitude and longitude" do + it "updates the address's latitude and longitude" do + allow(Geocoder).to receive(:coordinates).and_return([-37.47, 144.78]) + + AddressGeocoder.new(address).geocode + + expect(address.latitude).to eq(-37.47) + expect(address.longitude).to eq(144.78) + end + end + + describe "when the geocoder cannot determine the latitude and longitude" do + it "doesn't update the address's latitude and longitude" do + allow(Geocoder).to receive(:coordinates).and_return([nil, nil]) + + AddressGeocoder.new(address).geocode + + expect(address.latitude).to be_nil + expect(address.longitude).to be_nil + end + end +end