Skip to content

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

Merged
aduth merged 1 commit intomainfrom
aduth-lg-5025-proof-ssn-throttle
Aug 30, 2021
Merged

LG-5025: Create separate throttle error screen for proof_ssn throttle type#5346
aduth merged 1 commit intomainfrom
aduth-lg-5025-proof-ssn-throttle

Conversation

@aduth
Copy link
Copy Markdown
Contributor

@aduth aduth commented Aug 27, 2021

Why: So that each throttle error screen corresponds to exactly one throttle type, to accommodate accurate time labels proposed in #5301 (LG-4637).

Copy link
Copy Markdown
Contributor Author

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

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

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

Choose a reason for hiding this comment

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

bummer, thanks for checking!

Copy link
Copy Markdown
Contributor

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]

… type

**Why**: So that each throttle error screen corresponds to exactly one throttle type, to accommodate accurate time labels proposed in #5301 (LG-4637).
@aduth aduth force-pushed the aduth-lg-5025-proof-ssn-throttle branch from 4379919 to 52effd3 Compare August 30, 2021 13:47
Copy link
Copy Markdown
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

@aduth aduth merged commit 721dd5a into main Aug 30, 2021
@aduth aduth deleted the aduth-lg-5025-proof-ssn-throttle branch August 30, 2021 15:59
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.

2 participants