Skip to content

LG-9449: Configure ArcGIS token job to respect mock geocoder setting #8702

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

Merged
NavaTim merged 2 commits into from
Jun 30, 2023
Merged

LG-9449: Configure ArcGIS token job to respect mock geocoder setting #8702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f2a9b96
LG-9449: Configure ArcGIS token job to respect mock geocoder setting
NavaTim Jun 30, 2023
cec10fb
[skip changelog]
NavaTim Jun 30, 2023
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
3 changes: 1 addition & 2 deletions app/controllers/idv/in_person/address_search_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@ def addresses(search_term)
end

def geocoder
@geocoder ||= IdentityConfig.store.arcgis_mock_fallback ?
ArcgisApi::Mock::Geocoder.new : ArcgisApi::Geocoder.new
@geocoder ||= ArcgisApi::GeocoderFactory.new.create
end

def report_errors(error)
Expand Down
2 changes: 1 addition & 1 deletion app/jobs/arcgis_token_job.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def perform
private

def geocoder
@geocoder ||= ArcgisApi::Geocoder.new
@geocoder ||= ArcgisApi::GeocoderFactory.new.create
end

def analytics
Expand Down
11 changes: 11 additions & 0 deletions app/services/arcgis_api/geocoder_factory.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
module ArcgisApi
class GeocoderFactory
def create
if IdentityConfig.store.arcgis_mock_fallback
ArcgisApi::Mock::Geocoder.new
else
ArcgisApi::Geocoder.new
end
end
end
Comment on lines +2 to +10
Copy link
Contributor

@zachmargolis zachmargolis Jun 30, 2023

Choose a reason for hiding this comment

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

proposal:

  1. call this a Router to match our DocAuthRouter
  2. make it a module, or at least ditch the instance method, there's nothing really to instantiate here because there's no state?
Suggested change
class GeocoderFactory
def create
if IdentityConfig.store.arcgis_mock_fallback
ArcgisApi::Mock::Geocoder.new
else
ArcgisApi::Geocoder.new
end
end
end
module GeocoderRouter
module_function
def geocoder
if IdentityConfig.store.arcgis_mock_fallback
ArcgisApi::Mock::Geocoder.new
else
ArcgisApi::Geocoder.new
end
end
end

so callsites would look like:

ArcgisApi::GeocoderRouter.geocoder

Copy link
Contributor

@gina-yamada gina-yamada Jun 30, 2023

Choose a reason for hiding this comment

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

Everything looks good in here. I like this suggestion.

Copy link
Contributor Author

@NavaTim NavaTim Jun 30, 2023

Choose a reason for hiding this comment

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

This is a factory pattern, so using Factory in the name makes more sense and is more informative to developers who aren't as familiar with this specific codebase. The Router naming is confusing because that is normally associated with routing web requests to a controller.

Copy link
Contributor Author

@NavaTim NavaTim Jun 30, 2023

Choose a reason for hiding this comment

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

I thought about making this a module, but based on my prior experience using an instance tends to be more useful and reusable. The decision does seem a little arbitrary without more established conventions around IoC though.

end
4 changes: 3 additions & 1 deletion spec/jobs/arcgis_token_job_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
RSpec.describe ArcgisTokenJob, type: :job do
let(:job) { described_class.new }
let(:geocoder) { instance_double(ArcgisApi::Geocoder) }
let(:geocoder_factory) { instance_double(ArcgisApi::GeocoderFactory) }
let(:analytics) { instance_double(Analytics) }
describe 'arcgis token job' do
before do
Expand All @@ -13,7 +14,8 @@
session: {},
sp: nil,
).and_return(analytics)
allow(ArcgisApi::Geocoder).to receive(:new).and_return(geocoder)
allow(ArcgisApi::GeocoderFactory).to receive(:new).and_return(geocoder_factory)
Copy link
Contributor

@zachmargolis zachmargolis Jun 30, 2023

Choose a reason for hiding this comment

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

bonus to switching to a class/module method: we don't need to stub :new (which is one of my biggest pet peeves)

allow(geocoder_factory).to receive(:create).and_return(geocoder)
end

it 'fetches token and logs analytics' do
Expand Down
20 changes: 20 additions & 0 deletions spec/services/arcgis_api/geocoder_factory_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
require 'rails_helper'

RSpec.describe ArcgisApi::GeocoderFactory do
subject(:geocoder_factory) { described_class.new }
context 'mock is enabled' do
it 'returns a mock geocoder' do
expect(IdentityConfig.store).to receive(:arcgis_mock_fallback).
and_return(true)
expect(geocoder_factory.create).to be_instance_of(ArcgisApi::Mock::Geocoder)
end
end

context 'mock is disabled' do
it 'returns a geocoder' do
expect(IdentityConfig.store).to receive(:arcgis_mock_fallback).
and_return(false)
expect(geocoder_factory.create).to be_instance_of(ArcgisApi::Geocoder)
end
end
end