From f20cea7e4f4ca27d27f87ab87dbae208c02203f2 Mon Sep 17 00:00:00 2001 From: Cillian O'Ruanaidh Date: Fri, 12 Feb 2021 14:36:45 +0000 Subject: [PATCH] Allow people to set enterprise latitude/longitude manually or automatically. This for new changes to the enterprise registration/signup flow where a map will be displayed when people are filling in their address. On this map people can check the geocoder has geocoded their address correctly and if not they can manually adjust their latitude/longitude on the map. But currently every time someone changes their address in the Admin > Enterprise > Address section the address would automatically be geocoded so this could overwrite the latitude/longitude that was set during sign up. To prevent the latitude/longitude from being overwritten this add's a checkbox which people need to explicity click if they want their address to be automatically geocoded, otherwise it will just use the manually configured latitude/longitude. Note this new feature which allows people to select their location on a map during registration only works with Google maps so far. So if an instance is using Open Street Map this change also adds support for passing a :use_geocoder parameter to the Api::EnterprisesController during registration so that the address will be geocoded on the backend without the use of a map. --- .../javascripts/darkswarm/all.js.coffee | 2 +- .../registration_controller.js.coffee | 1 - .../enterprise_registration_service.js.coffee | 6 +++ .../admin/enterprises_controller.rb | 5 ++ app/controllers/api/enterprises_controller.rb | 4 ++ .../concerns/geocode_enterprise_address.rb | 9 ++++ app/helpers/map_helper.rb | 11 +++++ app/models/enterprise.rb | 4 -- app/models/spree/address.rb | 14 ------ app/services/address_geocoder.rb | 30 ++++++++++++ app/services/permitted_attributes/address.rb | 2 +- .../admin/enterprises/_new_form.html.haml | 7 ++- .../admin/enterprises/form/_address.html.haml | 4 +- app/views/layouts/registration.html.haml | 3 +- .../registration/steps/_details.html.haml | 37 +++++++------- .../admin/enterprises_controller_spec.rb | 48 +++++++++++++++++-- .../api/enterprises_controller_spec.rb | 14 ++++++ spec/features/consumer/registration_spec.rb | 1 + spec/models/spree/addresses_spec.rb | 18 ------- spec/services/address_geocoder_spec.rb | 47 ++++++++++++++++++ 20 files changed, 199 insertions(+), 68 deletions(-) create mode 100644 app/controllers/concerns/geocode_enterprise_address.rb create mode 100644 app/services/address_geocoder.rb create mode 100644 spec/services/address_geocoder_spec.rb 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