Skip to content

LG-9295: Rate limit for “Verify your information” must include a link back to SP#8334

Merged
matthinz merged 7 commits intomainfrom
matthinz/9295-verify-rate-limit-sp-link
May 10, 2023
Merged

LG-9295: Rate limit for “Verify your information” must include a link back to SP#8334
matthinz merged 7 commits intomainfrom
matthinz/9295-verify-rate-limit-sp-link

Conversation

@matthinz
Copy link
Contributor

@matthinz matthinz commented May 3, 2023

🎫 Ticket

LG-9295

🛠 Summary of changes

Tweaks to the rate limit error screen shown on the "Verify your information" step of IdV. Includes:

  • Updated heading
  • Tweaked body copy
  • More prominent "exit" link

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

Before:

With SP

with-sp-en-BEFORE

Without SP

without-sp-en-BEFORE

After:

With SP

with-sp-en-AFTER

Without SP

without-sp-en-AFTER

@matthinz matthinz requested a review from a team May 3, 2023 21:27
matthinz added 4 commits May 3, 2023 15:41
View now refers to @sp_name rather than decorated_session.sp_name
Updated heading, modified emphasis in body.
@matthinz matthinz force-pushed the matthinz/9295-verify-rate-limit-sp-link branch from cef81c1 to 88c9818 Compare May 3, 2023 23:35
@amirbey
Copy link
Contributor

amirbey commented May 4, 2023

👋🏿 @matthinz ... looks like i'm not able yet to provide a review ... LGTM, but i think we still have to add the progress bar at the top? it's not annotated on the image in ticket ... happened to me too 😄

@matthinz
Copy link
Contributor Author

matthinz commented May 4, 2023

@amirbey Thanks! Good catch and addressed in 62deae4

'idv/shared/error',
title: t('titles.failure.information_not_verified'),
heading: t('idv.failure.sessions.heading'),
current_step: :verify_info,
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 this may cause an issue with the hybrid flow (which currently can't use the step indicator concern) and could run into the same issue described here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The view doesn't attempt to render a step indicator if StepIndicatorConcern hasn't been included (that's the defined?(step_indicator_steps) here), so it should be to leave this line in, then re-enable StepIndicatorConcern when it's hybrid-compatible.

Copy link
Contributor

@mitchellhenke mitchellhenke May 4, 2023

Choose a reason for hiding this comment

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

Sorry, yes, as long as the StepIndicatorConcern is not reintroduced to the controller before then, it should be fine! The comment above was written before #8339 was merged and I wasn't sure if that was the path we were going to take to resolve the issue in the near-term.

Copy link
Contributor

@solipet solipet left a comment

Choose a reason for hiding this comment

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

Looks good - one comment about a missing translation update.

matthinz and others added 2 commits May 10, 2023 11:26
Co-authored-by: Doug Price <douglas.price@gsa.gov>
@matthinz matthinz merged commit 3a572d8 into main May 10, 2023
@matthinz matthinz deleted the matthinz/9295-verify-rate-limit-sp-link branch May 10, 2023 20:12
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