Skip to content

LG-10681: Remove ArcGIS API Usage#9154

Merged
NavaTim merged 7 commits intomainfrom
tbradley/lg-10681-remove-arcgis-usage
Sep 6, 2023
Merged

LG-10681: Remove ArcGIS API Usage#9154
NavaTim merged 7 commits intomainfrom
tbradley/lg-10681-remove-arcgis-usage

Conversation

@NavaTim
Copy link
Contributor

@NavaTim NavaTim commented Sep 6, 2023

🎫 Ticket

  • LG-10681

🛠 Summary of changes

  • Remove all backend code related to communication with the ArcGIS API
    • Frontend single-field search that uses geocoding endpoint remains under in_person_full_address_entry_enabled feature flag

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Check that full address search works for in-person proofing flow
  • Ensure that full address search is enabled for production

@NavaTim NavaTim requested review from a team, allthesignals and racingspider September 6, 2023 18:17

# location page
complete_location_step
complete_full_address_location_step
Copy link
Contributor

Choose a reason for hiding this comment

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

now, the "full address" location step is our only location step, right? Should we rename the helper methods and keep these specs otherwise unchanged?

@NavaTim NavaTim merged commit 689bef4 into main Sep 6, 2023
@NavaTim NavaTim deleted the tbradley/lg-10681-remove-arcgis-usage branch September 6, 2023 22:13
@gina-yamada gina-yamada mentioned this pull request Sep 6, 2023
2 tasks
post '/api/service_provider' => 'service_provider#update'
post '/api/verify/images' => 'idv/image_uploads#create'
post '/api/logger' => 'frontend_log#create'
post '/api/addresses' => 'idv/in_person/address_search#index'
Copy link
Contributor

Choose a reason for hiding this comment

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

There's still a couple lingering references to this path. Do they need to be cleaned up?

export const ADDRESSES_URL = new URL('/api/addresses', window.location.href).toString();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the AC said to leave this React component in place, I judged this update to be out-of-scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah the idea is that this'll be ready to use again once we implement the AWS-based geocoding service... since it's not being used in prod anymore it should be okay. But it's a little risky if that flag gets switched accidentally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I'd rather us having removed the unused code and rely on git history if/when we need it in the future, since requirements change and we otherwise have no obligation to clean this up.

get '/api/openid_connect/userinfo' => 'openid_connect/user_info#show'
post '/api/risc/security_events' => 'risc/security_events#create'

post '/api/address_search' => 'idv/in_person/public/address_search#index'
Copy link
Contributor

@aduth aduth Sep 8, 2023

Choose a reason for hiding this comment

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

@soniaconnolly 's comment at #9136 (comment) reminded me here, is the removal of these paths going to cause some issues during deployment for users running JavaScript served by old boxes sending requests to new servers? Or is this already disabled in production?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the current status of the in_person_full_address_entry_enabled ensures that users will not be using this endpoint in production.

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