Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
32 changes: 28 additions & 4 deletions app/controllers/idv/in_person/usps_locations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,37 @@ class UspsLocationsController < ApplicationController

before_action :confirm_authenticated_for_api, only: [:update]

# get the list of all pilot Post Office locations
# retrieve the list of nearby IPP Post Office locations with a POST request
def index
usps_response = []
begin
usps_response = Proofer.new.request_pilot_facilities
if IdentityConfig.store.arcgis_search_enabled
candidate = UspsInPersonProofing::Applicant.new(
address: search_params['street_address'],
city: search_params['city'], state: search_params['state'],
zip_code: search_params['zip_code']
)
usps_response = proofer.request_facilities(candidate)
else
usps_response = proofer.request_pilot_facilities
end
rescue ActionController::ParameterMissing
usps_response = proofer.request_pilot_facilities
rescue Faraday::ConnectionFailed => _error
nil
end

render json: usps_response.to_json
end

def proofer
@proofer ||= Proofer.new
end

# save the Post Office location the user selected to an enrollment
def update
enrollment.update!(
selected_location_details: permitted_params.as_json,
selected_location_details: update_params.as_json,
issuer: current_sp&.issuer,
)

Expand All @@ -47,7 +62,16 @@ def enrollment
)
end

def permitted_params
def search_params
params.require(:address).permit(
:street_address,
:city,
:state,
:zip_code,
)
end

def update_params
params.require(:usps_location).permit(
:formatted_city_state_zip,
:name,
Expand Down
1 change: 0 additions & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,6 @@
get '/in_person' => 'in_person#index'
get '/in_person/ready_to_verify' => 'in_person/ready_to_verify#show',
as: :in_person_ready_to_verify
get '/in_person/usps_locations' => 'in_person/usps_locations#index'
Copy link
Copy Markdown
Contributor

@allthesignals allthesignals Nov 28, 2022

Choose a reason for hiding this comment

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

Now that this switch from post to get has been deployed, it should be safe to remove this

post '/in_person/usps_locations' => 'in_person/usps_locations#index'
put '/in_person/usps_locations' => 'in_person/usps_locations#update'
post '/in_person/addresses' => 'in_person/address_search#index'
Expand Down
131 changes: 111 additions & 20 deletions spec/controllers/idv/in_person/usps_locations_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,19 @@
let(:user) { create(:user) }
let(:sp) { nil }
let(:in_person_proofing_enabled) { true }
let(:arcgis_search_enabled) { true }
let(:address) do
UspsInPersonProofing::Applicant.new(
address: '1600 Pennsylvania Ave',
city: 'Washington', state: 'DC', zip_code: '20500'
)
end
let(:fake_address) do
UspsInPersonProofing::Applicant.new(
address: '742 Evergreen Terrace',
city: 'Springfield', state: 'MO', zip_code: '89011'
)
end
let(:selected_location) do
{
usps_location: {
Expand All @@ -25,53 +38,131 @@
stub_sign_in(user) if user
allow(IdentityConfig.store).to receive(:in_person_proofing_enabled).
and_return(in_person_proofing_enabled)
allow(IdentityConfig.store).to receive(:arcgis_search_enabled).
and_return(arcgis_search_enabled)
allow(controller).to receive(:current_sp).and_return(sp)
end

describe '#index' do
let(:proofer) { double('Proofer') }
let(:locations) do
[
{ address: '3118 WASHINGTON BLVD',
city: 'ARLINGTON',
distance: '6.02 mi',
name: 'ARLINGTON',
phone: '703-993-0072',
saturday_hours: '9:00 AM - 1:00 PM',
state: 'VA',
sunday_hours: 'Closed',
weekday_hours: '9:00 AM - 5:00 PM',
zip_code_4: '9998',
zip_code_5: '22201' },
{ address: '4005 WISCONSIN AVE NW',
city: 'WASHINGTON',
distance: '6.59 mi',
name: 'FRIENDSHIP',
phone: '202-842-3332',
saturday_hours: '8:00 AM - 4:00 PM',
state: 'DC',
sunday_hours: '10:00 AM - 4:00 PM',
weekday_hours: '8:00 AM - 6:00 PM',
zip_code_4: '9997',
zip_code_5: '20016' },
{ address: '6900 WISCONSIN AVE STE 100',
city: 'CHEVY CHASE',
distance: '8.99 mi',
name: 'BETHESDA',
phone: '301-941-2670',
saturday_hours: '9:00 AM - 4:00 PM',
state: 'MD',
sunday_hours: 'Closed',
weekday_hours: '9:00 AM - 5:00 PM',
zip_code_4: '9996',
zip_code_5: '20815' },
]
end
let(:pilot_locations) do
[
{ name: 'Location 1' },
{ name: 'Location 2' },
{ name: 'Location 3' },
{ name: 'Location 4' },
]
end
subject(:response) { get :index }
subject(:response) do
post :index, params: { address: { street_address: '1600 Pennsylvania Ave',
city: 'Washington',
state: 'DC', zip_code: '20500' } }
end

before do
allow(UspsInPersonProofing::Proofer).to receive(:new).and_return(proofer)
end

context 'with successful fetch' do
before do
allow(proofer).to receive(:request_pilot_facilities).and_return(locations)
context 'with arcgis search enabled' do
context 'with a nil address in params' do
before do
allow(proofer).to receive(:request_pilot_facilities).and_return(pilot_locations)
end

subject(:response) do
post :index, params: { address: nil }
end

it 'returns the pilot locations' do
json = response.body
facilities = JSON.parse(json)
expect(facilities.length).to eq 4
end
end

it 'gets successful pilot response' do
response = get :index
json = response.body
facilities = JSON.parse(json)
expect(facilities.length).to eq 4
context 'with successful fetch' do
before do
allow(proofer).to receive(:request_facilities).with(address).and_return(locations)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

non-blocking observation:

one pattern I've learned about from zach is to wrap an "external" class like this in a method and stub that method instead: https://github.com/18F/identity-idp/blob/main/app/controllers/idv/in_person/address_search_controller.rb#L22-L24.

So, you could have a proofer method, and you'd simply mock that instead of needed to mock UspsInPersonProofing::Proofer directly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

zach agrees!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I implemented this change in the controller. I haven't changed the test setup, but can do so tomorrow. I'm logging off for the day, but I am also considering mocking the proofer. Sheldon suggested this refactor, and noted this method in the enrollment_helper.rb as potential inspiration.

      def usps_proofer
        if IdentityConfig.store.usps_mock_fallback
          UspsInPersonProofing::Mock::Proofer.new
        else
          UspsInPersonProofing::Proofer.new
        end
      end

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

zach also likes mocking the proofer, esp useful for lower envs

that said, I'm wondering if it would be ok to use "real" arcgis in a lower env, like we use real SMS and real voice so people can acually use the app

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good question. I'll bring this up during the next PO search check-in meeting.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i think it's probably fine. i suspect voice and SMS billing is done on a credit system? we're using GSA's hosted service, and it isn't constrained in that way. open to other angles, though!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Another thought: if the idea is to have addresses that are close to PO locations... and in mock world the PO locations are fake, maybe it does make sense to have a fake here? So that it can return things that the mock USPS service recognizes? I could go either way on this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I misunderstood! I think that makes sense, for the local environment.

end

it 'returns a successful response' do
json = response.body
facilities = JSON.parse(json)
expect(facilities.length).to eq 3
end
end
end

context 'with unsuccessful fetch' do
before do
exception = Faraday::ConnectionFailed.new('error')
allow(proofer).to receive(:request_pilot_facilities).and_raise(exception)
context 'with unsuccessful fetch' do
before do
exception = Faraday::ConnectionFailed.new('error')
allow(proofer).to receive(:request_facilities).with(fake_address).and_raise(exception)
end

it 'gets an empty response' do
response = post :index,
params: { address: { street_address: '742 Evergreen Terrace',
city: 'Springfield',
state: 'MO', zip_code: '89011' } }
json = response.body
facilities = JSON.parse(json)
expect(facilities.length).to eq 0
end
end
end

it 'gets an empty pilot response' do
response = get :index
json = response.body
facilities = JSON.parse(json)
expect(facilities.length).to eq 0
context 'with arcgis search disabled' do
let(:arcgis_search_enabled) { false }
context 'with successful fetch' do
before do
allow(proofer).to receive(:request_pilot_facilities).and_return(pilot_locations)
end

it 'returns a successful response' do
json = response.body
facilities = JSON.parse(json)
expect(facilities.length).to eq 4
end
end
end

context 'with feature disabled' do
context 'with in person proofing disabled' do
let(:in_person_proofing_enabled) { false }

it 'renders 404' do
Expand Down