Skip to content

LG-5025: Create separate throttle error screen for proof_ssn throttle type #5346

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

Merged
aduth merged 1 commit into from
Aug 30, 2021
Merged

LG-5025: Create separate throttle error screen for proof_ssn throttle type #5346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions app/controllers/idv/session_errors_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ def warning
@remaining_step_attempts = remaining_step_attempts
end

def ssn_failure
Copy link
Copy Markdown
Contributor Author

@aduth aduth Aug 27, 2021

Choose a reason for hiding this comment

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

Could make a query parameter for the failure route, I suppose? 🤔

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Aug 27, 2021

Choose a reason for hiding this comment

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

If we wanted to have one route and parameterize, I'd consider a path param:

      get '/session/errors/:error' => 'session_errors#show'

and then have #show do a 404 or something with an unknown params[:error]

Copy link
Copy Markdown
Contributor Author

@aduth aduth Aug 30, 2021

Choose a reason for hiding this comment

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

If we wanted to have one route and parameterize, I'd consider a path param:

      get '/session/errors/:error' => 'session_errors#show'

and then have #show do a 404 or something with an unknown params[:error]

I explored this a bit, and I think there's room for refactoring here, though ultimately I struggled to find a reasonable improvement over just keeping the separate method here.

A few factors:

  • There's several different warnings and errors for this controller, so it's unclear if we'd want a route segment for just the failures, or for every one of the possible screens. Maybe we could refactor this for one controller method which builds a presenter object for the specific throttle / messaging to be shown?
  • We use a lot of URL helpers for these routes, which would need to be updated, e.g. idv_session_errors_failure_url to idv_session_errors_url(:failure). Not difficult, but widespread.
  • Maybe at some point we'd want to have specific texts for the SSN proofing failure, at which point we'd not need to reuse the failure template like we are here, and can follow the established pattern.
  • Considering LG-4637: Show accurate time remaining for IAL2 throttle screens #5301, we'll also need to set an instance variable for the appropriate throttle type, else we could just point directly to the controller method i.e. get '/session/errors/ssn_failure' => 'session_errors#failure'

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Aug 30, 2021

Choose a reason for hiding this comment

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

bummer, thanks for checking!

render 'idv/session_errors/failure'
end

private

def remaining_step_attempts
Expand Down
2 changes: 1 addition & 1 deletion app/services/idv/steps/verify_step.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def enqueue_job
throttle_type: :proof_ssn,
step_name: self.class,
)
redirect_to idv_session_errors_failure_url
redirect_to idv_session_errors_ssn_failure_url
return
end
end
Expand Down
1 change: 1 addition & 0 deletions config/routes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@
put '/review' => 'review#create'
get '/session/errors/warning' => 'session_errors#warning'
get '/session/errors/failure' => 'session_errors#failure'
get '/session/errors/ssn_failure' => 'session_errors#ssn_failure'
get '/session/errors/exception' => 'session_errors#exception'
get '/session/errors/throttled' => 'session_errors#throttled'
delete '/session' => 'sessions#destroy'
Expand Down
7 changes: 7 additions & 0 deletions spec/controllers/idv/session_errors_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,4 +72,11 @@

it_behaves_like 'an idv session errors controller action'
end

describe '#ssn_failure' do
let(:action) { :ssn_failure }
let(:template) { 'idv/session_errors/failure' }

it_behaves_like 'an idv session errors controller action'
end
end
4 changes: 2 additions & 2 deletions spec/services/idv/steps/verify_step_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,11 @@ def redirect(step)

step = build_step(controller)
expect(step.call).to be_nil, 'does not enqueue a job'
expect(redirect(step)).to eq(idv_session_errors_failure_url)
expect(redirect(step)).to eq(idv_session_errors_ssn_failure_url)

step2 = build_step(controller2)
expect(step2.call).to be_nil, 'does not enqueue a job'
expect(redirect(step2)).to eq(idv_session_errors_failure_url)
expect(redirect(step2)).to eq(idv_session_errors_ssn_failure_url)
end
end
end
Expand Down