Skip to content

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

Merged
NavaTim merged 2 commits intomainfrom
tbradley/LG-9449-arcgis-token-refresh-mock
Jun 30, 2023
Merged

LG-9449: Configure ArcGIS token job to respect mock geocoder setting#8702
NavaTim merged 2 commits intomainfrom
tbradley/LG-9449-arcgis-token-refresh-mock

Conversation

@NavaTim
Copy link
Contributor

@NavaTim NavaTim commented Jun 30, 2023

🎫 Ticket

🛠 Summary of changes

  • Update the ArcGIS token job to respect the mock geocoder setting

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Configure environment to use the mock geocoder
  • Verify that no calls are made to ArcGIS and that no related errors occur

@NavaTim NavaTim closed this Jun 30, 2023
@NavaTim NavaTim reopened this Jun 30, 2023
Comment on lines +2 to +10
class GeocoderFactory
def create
if IdentityConfig.store.arcgis_mock_fallback
ArcgisApi::Mock::Geocoder.new
else
ArcgisApi::Geocoder.new
end
end
end
Copy link
Contributor

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

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

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

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.

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

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)

@NavaTim NavaTim merged commit 0cab381 into main Jun 30, 2023
@NavaTim NavaTim deleted the tbradley/LG-9449-arcgis-token-refresh-mock branch June 30, 2023 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants