Skip to content

LG-9168: Set up public mock address search and usps location search endpoints#8869

Merged
allthesignals merged 15 commits intomainfrom
wmg/9168-mock-endpoints
Jul 27, 2023
Merged

LG-9168: Set up public mock address search and usps location search endpoints#8869
allthesignals merged 15 commits intomainfrom
wmg/9168-mock-endpoints

Conversation

@allthesignals
Copy link
Contributor

## 🎫 Ticket

🛠 Summary of changes

Set up public-facing mock address and USPS location endpoints for use by identity-site PO search

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Write simple test checking for a 200 ok response without required CSRF

Copy link
Contributor

@sheldon-b sheldon-b left a comment

Choose a reason for hiding this comment

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

We should also add tests to ensure the new routes return 404 when the feature flag is disabled

module Idv
module InPerson
module Public
class MockAddressSearchController < ApplicationController
Copy link
Contributor

Choose a reason for hiding this comment

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

My first thought was to add barebones versions of the actual controllers we want, eg AddressSearchController and UspsLocationsController, so that we can flesh them out over time

I'm fine with this approach too. Seems like a little more code turnover to later remove 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 agree, I think the controllers should be generic and have the internal behavior change between real/mock results based on configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I can split this out into those two. I agree, I think that's better.

I need to use the respective mock classes because there is internal post-processing that needs to be captured.

Comment on lines +14 to +15
allow(IdentityConfig.store).to receive(:in_person_public_address_search_enabled).
and_return(false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Struggling to get the test to work with this stub... it seems to recognize the routes when I change the default value of this config variable. However, it doesn't recognize routes when I try to stub over the default:

    ActionController::UrlGenerationError:
       No route matches {:action=>"index", :address=>"100 main", :controller=>"idv/in_person/public/address_search", :host=>"www.example.com"}

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 you need to call Rails.application.reload_routes! before/after to reload the specs. We've had lots of issues with test pollution with this in the past, so I'd be careful

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think the preference is to use check_or_render_not_found to avoid that, ex:

check_or_render_not_found -> { IdentityConfig.store.in_person_proofing_enabled }

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 see. I think we cannot test when the flag is off if we wrap the actual routes in routes.rb with a feature flag (looking here for precedent).

The ready_to_verify_controller maps to routes that are not hidden behind a feature flag in routes...

I'll change this around and see what you think.

@allthesignals allthesignals marked this pull request as ready for review July 27, 2023 16:34
allthesignals and others added 3 commits July 27, 2023 12:46
…r_spec.rb

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
…r_spec.rb

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@allthesignals allthesignals merged commit 97ce4ef into main Jul 27, 2023
@allthesignals allthesignals deleted the wmg/9168-mock-endpoints branch July 27, 2023 18:02
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