Skip to content

LG-7925 Backend pings USPS API for IPP locations#7386

Merged
eileen-nava merged 12 commits intomainfrom
em/7925-PO-search-controller
Nov 30, 2022
Merged

LG-7925 Backend pings USPS API for IPP locations#7386
eileen-nava merged 12 commits intomainfrom
em/7925-PO-search-controller

Conversation

@eileen-nava
Copy link
Contributor

@eileen-nava eileen-nava commented Nov 22, 2022

🎫 Ticket

LG-7925: Results - controller search USPS endpoint

🛠 Summary of changes

  • Add a call to the USPS's GetIppFacilityList route, via the UspsInPersonProofing's request_facilities method
  • Put work behind the arcgis_search_enabled feature flag

📜 Testing Plan

  • Run automated tests

@eileen-nava eileen-nava changed the title Em/7925 po search controller LG-7925 Backend pings USPS API for IPP locations Nov 28, 2022
@eileen-nava eileen-nava marked this pull request as ready for review November 28, 2022 21:39
@eileen-nava eileen-nava requested review from a team and allthesignals November 28, 2022 21:56
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
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

Comment on lines +24 to +28
if IdentityConfig.store.arcgis_search_enabled
usps_response = Proofer.new.request_facilities(candidate)
else
usps_response = Proofer.new.request_pilot_facilities
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Great.

Since I'm also hiding the frontend behavior behind feature flag, it should be aligned.

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand, there's no additional work needed to make request_facilities ready to go...

Copy link
Contributor

Choose a reason for hiding this comment

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

Looping in @tomas-nava for context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That’s my understanding, too. FYSA, when I logged into dev, I was able to successfully use the request_facilities(location) method.

Comment on lines +49 to +97
allow(proofer).to receive(:request_pilot_facilities).and_return(locations)
allow(proofer).to receive(:request_facilities).with(address).and_return(locations)
Copy link
Contributor

Choose a reason for hiding this comment

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

So, is coverage of request_pilot_facilities something we really want to remove during this transitional period? It'll still be needed for production until we flip the switch on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I can add the test coverage for request_pilot_facilities back into this PR. 🙏🏻

@allthesignals allthesignals self-requested a review November 29, 2022 17:23
context 'with arcgis search enabled' do
context 'with successful fetch' do
before do
allow(proofer).to receive(:request_facilities).with(address).and_return(locations)
Copy link
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
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
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
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
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
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
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
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.

Copy link
Contributor

@allthesignals allthesignals left a comment

Choose a reason for hiding this comment

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

Approving now with understanding that we should stub the new proofer method in tests rather than stubbing Proofer class directly

@eileen-nava eileen-nava merged commit 6fe7ed7 into main Nov 30, 2022
@eileen-nava eileen-nava deleted the em/7925-PO-search-controller branch November 30, 2022 18:05
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.

3 participants