Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: matthinz/15479-nil-zipcode-part-1
Choose a base branch
from
+1 −4
Open

Stop returning redirect_url from LinkSentPollController for IPP #11821

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e419a25
Stop returning redirect_url from LinkSentPollController for IPP
matthinz Jan 30, 2025
50150ef
Clarify spec a little
matthinz Jan 30, 2025
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
2 changes: 0 additions & 2 deletions app/controllers/idv/link_sent_poll_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,6 @@ def redirect_url

if rate_limiter.limited?
idv_session_errors_rate_limited_url
elsif user_has_establishing_in_person_enrollment?
Copy link
Contributor

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?

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. 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

Copy link
Contributor Author

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.

Copy link
Contributor

@aduth aduth Jan 30, 2025 •
edited
Loading

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.

idv_in_person_url
end
end

Expand Down
2 changes: 1 addition & 1 deletion spec/controllers/idv/link_sent_poll_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

end
end
end
Expand Down