-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stop returning redirect_url from LinkSentPollController for IPP #11821
base: matthinz/15479-nil-zipcode-part-1
Are you sure you want to change the base?
Stop returning redirect_url from LinkSentPollController for IPP #11821
Conversation
This is no longer needed. [skip changelog]
@@ -218,7 +218,7 @@ | |||
get :show | |||
|
|||
expect(response.status).to eq(200) | |||
expect(JSON.parse(response.body)).to include('redirect' => idv_in_person_url) | |||
expect(JSON.parse(response.body)).not_to include('redirect' => idv_in_person_url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd maybe think we could get rid of this entire "in-person" context
block, if the code is no longer concerned with IPP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that is reasonable.
@@ -48,8 +48,6 @@ def redirect_url | |||
|
|||
if rate_limiter.limited? | |||
idv_session_errors_rate_limited_url | |||
elsif user_has_establishing_in_person_enrollment? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove all other references to user_has_establishing_in_person_enrollment?
(including the method definition) in this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. It looks like it is used to decide whether to return 200 OK
or 202 Accepted
--let me dig in and see what the actual difference in handling those are
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it looks like for anything other than 200
, 410
and 429
, we just keep polling, so we likely want to start returning a 200
rather than a 202
in this case. I will make that change over in #11820. We'll likely still want this user_has_establishing_in_person_enrollment?
method for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the idea with #11820 was that a user with establishing in-person enrollment would no longer be able to reach the link sent screen? (i.e. making it a moot point what would be returned by the polling endpoint in that scenario)
Edit: Although I suppose we'd want to be conscious of 50/50 states and someone already being on the screen.
This PR cannot land until #11820 is in production
🛠 Summary of changes
#11820 removed the need for an IPP-related
redirect_url
to come back fromLinkSentPollController
. This PR stops sendingredirect_url
back to the client entirely, ensuring that all IPP enrollments in the hybrid flow are routed throught theupdate
method ofLinkSentController
.